| Message ID | 1405067941-27134-7-git-send-email-Bharat.Bhushan@freescale.com |
|---|---|
| State | New, archived |
| Headers | show |
On 11.07.14 10:39, Bharat Bhushan wrote: > This patch emulates debug registers and debug exception > to support guest using debug resource. This enables running > gdb/kgdb etc in guest. > > On BOOKE architecture we cannot share debug resources between QEMU and > guest because: > When QEMU is using debug resources then debug exception must > be always enabled. To achieve this we set MSR_DE and also set > MSRP_DEP so guest cannot change MSR_DE. > > When emulating debug resource for guest we want guest > to control MSR_DE (enable/disable debug interrupt on need). > > So above mentioned two configuration cannot be supported > at the same time. So the result is that we cannot share > debug resources between QEMU and Guest on BOOKE architecture. > > In the current design QEMU gets priority over guest, this means that if > QEMU is using debug resources then guest cannot use them and if guest is > using debug resource then QEMU can overwrite them. > > Signed-off-by: Bharat Bhushan <Bharat.Bhushan@freescale.com> > --- > Hi Alex, > > I thought of having some print in register emulation if QEMU > is using debug resource, Also when QEMU overwrites guest written > values but that looks excessive. If I uses some variable which > get set when guest starts using debug registers and check in > debug set ioctl then that look ugly. Looking for suggestions Whatever you do, have QEMU do the print, not the kernel. > > arch/powerpc/include/asm/kvm_ppc.h | 3 + > arch/powerpc/kvm/booke.c | 27 +++++++ > arch/powerpc/kvm/booke_emulate.c | 157 +++++++++++++++++++++++++++++++++++++ > 3 files changed, 187 insertions(+) > > diff --git a/arch/powerpc/include/asm/kvm_ppc.h b/arch/powerpc/include/asm/kvm_ppc.h > index e2fd5a1..f3f7611 100644 > --- a/arch/powerpc/include/asm/kvm_ppc.h > +++ b/arch/powerpc/include/asm/kvm_ppc.h > @@ -173,6 +173,9 @@ extern int kvmppc_xics_get_xive(struct kvm *kvm, u32 irq, u32 *server, > extern int kvmppc_xics_int_on(struct kvm *kvm, u32 irq); > extern int kvmppc_xics_int_off(struct kvm *kvm, u32 irq); > > +void kvmppc_core_dequeue_debug(struct kvm_vcpu *vcpu); > +void kvmppc_core_queue_debug(struct kvm_vcpu *vcpu); > + > union kvmppc_one_reg { > u32 wval; > u64 dval; > diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c > index fadfe76..c2471ed 100644 > --- a/arch/powerpc/kvm/booke.c > +++ b/arch/powerpc/kvm/booke.c > @@ -264,6 +264,16 @@ static void kvmppc_core_dequeue_watchdog(struct kvm_vcpu *vcpu) > clear_bit(BOOKE_IRQPRIO_WATCHDOG, &vcpu->arch.pending_exceptions); > } > > +void kvmppc_core_queue_debug(struct kvm_vcpu *vcpu) > +{ > + kvmppc_booke_queue_irqprio(vcpu, BOOKE_IRQPRIO_DEBUG); > +} > + > +void kvmppc_core_dequeue_debug(struct kvm_vcpu *vcpu) > +{ > + clear_bit(BOOKE_IRQPRIO_DEBUG, &vcpu->arch.pending_exceptions); > +} > + > static void set_guest_srr(struct kvm_vcpu *vcpu, unsigned long srr0, u32 srr1) > { > #ifdef CONFIG_KVM_BOOKE_HV > @@ -783,6 +793,23 @@ static int kvmppc_handle_debug(struct kvm_run *run, struct kvm_vcpu *vcpu) > struct debug_reg *dbg_reg = &(vcpu->arch.shadow_dbg_reg); > u32 dbsr = vcpu->arch.dbsr; > > + if (vcpu->guest_debug == 0) { > + /* Debug resources belong to Guest */ > + if (dbsr && (vcpu->arch.shared->msr & MSR_DE)) > + kvmppc_core_queue_debug(vcpu); > + > + /* Inject a program interrupt if trap debug is not allowed */ > + if ((dbsr & DBSR_TIE) && !(vcpu->arch.shared->msr & MSR_DE)) > + kvmppc_core_queue_program(vcpu, ESR_PTR); In that case we would've received a program interrupt and never entered this code path, no? Alex -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 2014-07-11 at 14:09 +0530, Bharat Bhushan wrote: > This patch emulates debug registers and debug exception > to support guest using debug resource. This enables running > gdb/kgdb etc in guest. > > On BOOKE architecture we cannot share debug resources between QEMU and > guest because: > When QEMU is using debug resources then debug exception must > be always enabled. To achieve this we set MSR_DE and also set > MSRP_DEP so guest cannot change MSR_DE. > > When emulating debug resource for guest we want guest > to control MSR_DE (enable/disable debug interrupt on need). > > So above mentioned two configuration cannot be supported > at the same time. So the result is that we cannot share > debug resources between QEMU and Guest on BOOKE architecture. > > In the current design QEMU gets priority over guest, this means that if > QEMU is using debug resources then guest cannot use them and if guest is > using debug resource then QEMU can overwrite them. > > Signed-off-by: Bharat Bhushan <Bharat.Bhushan@freescale.com> > --- > Hi Alex, > > I thought of having some print in register emulation if QEMU > is using debug resource, Also when QEMU overwrites guest written > values but that looks excessive. If I uses some variable which > get set when guest starts using debug registers and check in > debug set ioctl then that look ugly. Looking for suggestions > > arch/powerpc/include/asm/kvm_ppc.h | 3 + > arch/powerpc/kvm/booke.c | 27 +++++++ > arch/powerpc/kvm/booke_emulate.c | 157 +++++++++++++++++++++++++++++++++++++ > 3 files changed, 187 insertions(+) > > diff --git a/arch/powerpc/include/asm/kvm_ppc.h b/arch/powerpc/include/asm/kvm_ppc.h > index e2fd5a1..f3f7611 100644 > --- a/arch/powerpc/include/asm/kvm_ppc.h > +++ b/arch/powerpc/include/asm/kvm_ppc.h > @@ -173,6 +173,9 @@ extern int kvmppc_xics_get_xive(struct kvm *kvm, u32 irq, u32 *server, > extern int kvmppc_xics_int_on(struct kvm *kvm, u32 irq); > extern int kvmppc_xics_int_off(struct kvm *kvm, u32 irq); > > +void kvmppc_core_dequeue_debug(struct kvm_vcpu *vcpu); > +void kvmppc_core_queue_debug(struct kvm_vcpu *vcpu); > + > union kvmppc_one_reg { > u32 wval; > u64 dval; > diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c > index fadfe76..c2471ed 100644 > --- a/arch/powerpc/kvm/booke.c > +++ b/arch/powerpc/kvm/booke.c > @@ -264,6 +264,16 @@ static void kvmppc_core_dequeue_watchdog(struct kvm_vcpu *vcpu) > clear_bit(BOOKE_IRQPRIO_WATCHDOG, &vcpu->arch.pending_exceptions); > } > > +void kvmppc_core_queue_debug(struct kvm_vcpu *vcpu) > +{ > + kvmppc_booke_queue_irqprio(vcpu, BOOKE_IRQPRIO_DEBUG); > +} > + > +void kvmppc_core_dequeue_debug(struct kvm_vcpu *vcpu) > +{ > + clear_bit(BOOKE_IRQPRIO_DEBUG, &vcpu->arch.pending_exceptions); > +} > + > static void set_guest_srr(struct kvm_vcpu *vcpu, unsigned long srr0, u32 srr1) > { > #ifdef CONFIG_KVM_BOOKE_HV > @@ -783,6 +793,23 @@ static int kvmppc_handle_debug(struct kvm_run *run, struct kvm_vcpu *vcpu) > struct debug_reg *dbg_reg = &(vcpu->arch.shadow_dbg_reg); > u32 dbsr = vcpu->arch.dbsr; > > + if (vcpu->guest_debug == 0) { > + /* Debug resources belong to Guest */ > + if (dbsr && (vcpu->arch.shared->msr & MSR_DE)) > + kvmppc_core_queue_debug(vcpu); Should also check for DBCR0[IDM]. > + > + /* Inject a program interrupt if trap debug is not allowed */ > + if ((dbsr & DBSR_TIE) && !(vcpu->arch.shared->msr & MSR_DE)) > + kvmppc_core_queue_program(vcpu, ESR_PTR); > + > + return RESUME_GUEST; > + } > + > + /* > + * Prepare for userspace exit as debug resources > + * are owned by userspace. > + */ > + vcpu->arch.dbsr = 0; > run->debug.arch.status = 0; > run->debug.arch.address = vcpu->arch.pc; Why are you clearing vcpu->arch.dbsr? Userspace might be interested in the raw value, plus it's a change from the current API semantics. Where is this run->debug.arch.<foo> stuff and KVM_EXIT_DEBUG documented? > diff --git a/arch/powerpc/kvm/booke_emulate.c b/arch/powerpc/kvm/booke_emulate.c > index 2a20194..3d143fe 100644 > --- a/arch/powerpc/kvm/booke_emulate.c > +++ b/arch/powerpc/kvm/booke_emulate.c > @@ -139,6 +139,7 @@ int kvmppc_booke_emulate_op(struct kvm_run *run, struct kvm_vcpu *vcpu, > int kvmppc_booke_emulate_mtspr(struct kvm_vcpu *vcpu, int sprn, ulong spr_val) > { > int emulated = EMULATE_DONE; > + bool debug_inst = false; > > switch (sprn) { > case SPRN_DEAR: > @@ -153,14 +154,137 @@ int kvmppc_booke_emulate_mtspr(struct kvm_vcpu *vcpu, int sprn, ulong spr_val) > case SPRN_CSRR1: > vcpu->arch.csrr1 = spr_val; > break; > + case SPRN_DSRR0: > + vcpu->arch.dsrr0 = spr_val; > + break; > + case SPRN_DSRR1: > + vcpu->arch.dsrr1 = spr_val; > + break; > + case SPRN_IAC1: > + /* > + * If userspace is debugging guest then guest > + * can not access debug registers. > + */ > + if (vcpu->guest_debug) > + break; > + > + debug_inst = true; > + vcpu->arch.dbg_reg.iac1 = spr_val; > + vcpu->arch.shadow_dbg_reg.iac1 = spr_val; > + break; > + case SPRN_IAC2: > + /* > + * If userspace is debugging guest then guest > + * can not access debug registers. > + */ > + if (vcpu->guest_debug) > + break; > + > + debug_inst = true; > + vcpu->arch.dbg_reg.iac2 = spr_val; > + vcpu->arch.shadow_dbg_reg.iac2 = spr_val; > + break; > +#if CONFIG_PPC_ADV_DEBUG_IACS > 2 > + case SPRN_IAC3: > + /* > + * If userspace is debugging guest then guest > + * can not access debug registers. > + */ > + if (vcpu->guest_debug) > + break; > + > + debug_inst = true; > + vcpu->arch.dbg_reg.iac3 = spr_val; > + vcpu->arch.shadow_dbg_reg.iac3 = spr_val; > + break; > + case SPRN_IAC4: > + /* > + * If userspace is debugging guest then guest > + * can not access debug registers. > + */ > + if (vcpu->guest_debug) > + break; > + > + debug_inst = true; > + vcpu->arch.dbg_reg.iac4 = spr_val; > + vcpu->arch.shadow_dbg_reg.iac4 = spr_val; > + break; > +#endif > + case SPRN_DAC1: > + /* > + * If userspace is debugging guest then guest > + * can not access debug registers. > + */ > + if (vcpu->guest_debug) > + break; > + > + debug_inst = true; > + vcpu->arch.dbg_reg.dac1 = spr_val; > + vcpu->arch.shadow_dbg_reg.dac1 = spr_val; > + break; > + case SPRN_DAC2: > + /* > + * If userspace is debugging guest then guest > + * can not access debug registers. > + */ > + if (vcpu->guest_debug) > + break; > + > + debug_inst = true; > + vcpu->arch.dbg_reg.dac2 = spr_val; > + vcpu->arch.shadow_dbg_reg.dac2 = spr_val; > + break; > case SPRN_DBCR0: > + /* > + * If userspace is debugging guest then guest > + * can not access debug registers. > + */ > + if (vcpu->guest_debug) > + break; > + > + debug_inst = true; > + spr_val &= (DBCR0_IDM | DBCR0_IC | DBCR0_BT | DBCR0_TIE | > + DBCR0_IAC1 | DBCR0_IAC2 | DBCR0_IAC3 | DBCR0_IAC4 | > + DBCR0_DAC1R | DBCR0_DAC1W | DBCR0_DAC2R | DBCR0_DAC2W); > + > vcpu->arch.dbg_reg.dbcr0 = spr_val; > + vcpu->arch.shadow_dbg_reg.dbcr0 = spr_val; > break; > case SPRN_DBCR1: > + /* > + * If userspace is debugging guest then guest > + * can not access debug registers. > + */ > + if (vcpu->guest_debug) > + break; > + > + debug_inst = true; > vcpu->arch.dbg_reg.dbcr1 = spr_val; > + vcpu->arch.shadow_dbg_reg.dbcr1 = spr_val; > + break; > + case SPRN_DBCR2: > + /* > + * If userspace is debugging guest then guest > + * can not access debug registers. > + */ > + if (vcpu->guest_debug) > + break; > + > + debug_inst = true; > + vcpu->arch.dbg_reg.dbcr2 = spr_val; > + vcpu->arch.shadow_dbg_reg.dbcr2 = spr_val; > break; In what circumstances can the architected and shadow registers differ? > case SPRN_DBSR: > + /* > + * If userspace is debugging guest then guest > + * can not access debug registers. > + */ > + if (vcpu->guest_debug) > + break; > + > vcpu->arch.dbsr &= ~spr_val; > + if (vcpu->arch.dbsr == 0) > + kvmppc_core_dequeue_debug(vcpu); > break; Not all DBSR bits cause an exception, e.g. IDE and MRR. > case SPRN_TSR: > kvmppc_clr_tsr_bits(vcpu, spr_val); > @@ -273,6 +397,10 @@ int kvmppc_booke_emulate_mtspr(struct kvm_vcpu *vcpu, int sprn, ulong spr_val) > emulated = EMULATE_FAIL; > } > > + if (debug_inst) { > + switch_booke_debug_regs(&vcpu->arch.shadow_dbg_reg); > + current->thread.debug = vcpu->arch.shadow_dbg_reg; > + } Could you explain what's going on with regard to copying the registers into current->thread.debug? Why is it done after loading the registers into the hardware (is there a race if we get preempted in the middle)? -Scott ; -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2014-07-28 at 16:04 +0200, Alexander Graf wrote: > On 11.07.14 10:39, Bharat Bhushan wrote: > > This patch emulates debug registers and debug exception > > to support guest using debug resource. This enables running > > gdb/kgdb etc in guest. > > > > On BOOKE architecture we cannot share debug resources between QEMU and > > guest because: > > When QEMU is using debug resources then debug exception must > > be always enabled. To achieve this we set MSR_DE and also set > > MSRP_DEP so guest cannot change MSR_DE. > > > > When emulating debug resource for guest we want guest > > to control MSR_DE (enable/disable debug interrupt on need). > > > > So above mentioned two configuration cannot be supported > > at the same time. So the result is that we cannot share > > debug resources between QEMU and Guest on BOOKE architecture. > > > > In the current design QEMU gets priority over guest, this means that if > > QEMU is using debug resources then guest cannot use them and if guest is > > using debug resource then QEMU can overwrite them. > > > > Signed-off-by: Bharat Bhushan <Bharat.Bhushan@freescale.com> > > --- > > Hi Alex, > > > > I thought of having some print in register emulation if QEMU > > is using debug resource, Also when QEMU overwrites guest written > > values but that looks excessive. If I uses some variable which > > get set when guest starts using debug registers and check in > > debug set ioctl then that look ugly. Looking for suggestions > > Whatever you do, have QEMU do the print, not the kernel. How would that be accomplished? How would the kernel know to exit to QEMU, and how would the exit reason be conveyed? -Scott -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 29.07.14 00:33, Scott Wood wrote: > On Mon, 2014-07-28 at 16:04 +0200, Alexander Graf wrote: >> On 11.07.14 10:39, Bharat Bhushan wrote: >>> This patch emulates debug registers and debug exception >>> to support guest using debug resource. This enables running >>> gdb/kgdb etc in guest. >>> >>> On BOOKE architecture we cannot share debug resources between QEMU and >>> guest because: >>> When QEMU is using debug resources then debug exception must >>> be always enabled. To achieve this we set MSR_DE and also set >>> MSRP_DEP so guest cannot change MSR_DE. >>> >>> When emulating debug resource for guest we want guest >>> to control MSR_DE (enable/disable debug interrupt on need). >>> >>> So above mentioned two configuration cannot be supported >>> at the same time. So the result is that we cannot share >>> debug resources between QEMU and Guest on BOOKE architecture. >>> >>> In the current design QEMU gets priority over guest, this means that if >>> QEMU is using debug resources then guest cannot use them and if guest is >>> using debug resource then QEMU can overwrite them. >>> >>> Signed-off-by: Bharat Bhushan <Bharat.Bhushan@freescale.com> >>> --- >>> Hi Alex, >>> >>> I thought of having some print in register emulation if QEMU >>> is using debug resource, Also when QEMU overwrites guest written >>> values but that looks excessive. If I uses some variable which >>> get set when guest starts using debug registers and check in >>> debug set ioctl then that look ugly. Looking for suggestions >> Whatever you do, have QEMU do the print, not the kernel. > How would that be accomplished? How would the kernel know to exit to > QEMU, and how would the exit reason be conveyed? QEMU is the one forcefully enabling debug and overwriting guest debug registers, so it also knows when it did overwrite valid ones. Alex -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2014-07-29 at 16:06 +0200, Alexander Graf wrote: > On 29.07.14 00:33, Scott Wood wrote: > > On Mon, 2014-07-28 at 16:04 +0200, Alexander Graf wrote: > >> On 11.07.14 10:39, Bharat Bhushan wrote: > >>> This patch emulates debug registers and debug exception > >>> to support guest using debug resource. This enables running > >>> gdb/kgdb etc in guest. > >>> > >>> On BOOKE architecture we cannot share debug resources between QEMU and > >>> guest because: > >>> When QEMU is using debug resources then debug exception must > >>> be always enabled. To achieve this we set MSR_DE and also set > >>> MSRP_DEP so guest cannot change MSR_DE. > >>> > >>> When emulating debug resource for guest we want guest > >>> to control MSR_DE (enable/disable debug interrupt on need). > >>> > >>> So above mentioned two configuration cannot be supported > >>> at the same time. So the result is that we cannot share > >>> debug resources between QEMU and Guest on BOOKE architecture. > >>> > >>> In the current design QEMU gets priority over guest, this means that if > >>> QEMU is using debug resources then guest cannot use them and if guest is > >>> using debug resource then QEMU can overwrite them. > >>> > >>> Signed-off-by: Bharat Bhushan <Bharat.Bhushan@freescale.com> > >>> --- > >>> Hi Alex, > >>> > >>> I thought of having some print in register emulation if QEMU > >>> is using debug resource, Also when QEMU overwrites guest written > >>> values but that looks excessive. If I uses some variable which > >>> get set when guest starts using debug registers and check in > >>> debug set ioctl then that look ugly. Looking for suggestions > >> Whatever you do, have QEMU do the print, not the kernel. > > How would that be accomplished? How would the kernel know to exit to > > QEMU, and how would the exit reason be conveyed? > > QEMU is the one forcefully enabling debug and overwriting guest debug > registers, so it also knows when it did overwrite valid ones. QEMU knows when it overwrites the guest values, but it doesn't know if, after enabling host debug, the guest tries to write to the debug registers and it gets nopped. If we keep the EDM setting, then we can at least say the situation is no worse than with a JTAG. -Scott -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> Am 29.07.2014 um 19:50 schrieb Scott Wood <scottwood@freescale.com>: > >> On Tue, 2014-07-29 at 16:06 +0200, Alexander Graf wrote: >>> On 29.07.14 00:33, Scott Wood wrote: >>>> On Mon, 2014-07-28 at 16:04 +0200, Alexander Graf wrote: >>>>> On 11.07.14 10:39, Bharat Bhushan wrote: >>>>> This patch emulates debug registers and debug exception >>>>> to support guest using debug resource. This enables running >>>>> gdb/kgdb etc in guest. >>>>> >>>>> On BOOKE architecture we cannot share debug resources between QEMU and >>>>> guest because: >>>>> When QEMU is using debug resources then debug exception must >>>>> be always enabled. To achieve this we set MSR_DE and also set >>>>> MSRP_DEP so guest cannot change MSR_DE. >>>>> >>>>> When emulating debug resource for guest we want guest >>>>> to control MSR_DE (enable/disable debug interrupt on need). >>>>> >>>>> So above mentioned two configuration cannot be supported >>>>> at the same time. So the result is that we cannot share >>>>> debug resources between QEMU and Guest on BOOKE architecture. >>>>> >>>>> In the current design QEMU gets priority over guest, this means that if >>>>> QEMU is using debug resources then guest cannot use them and if guest is >>>>> using debug resource then QEMU can overwrite them. >>>>> >>>>> Signed-off-by: Bharat Bhushan <Bharat.Bhushan@freescale.com> >>>>> --- >>>>> Hi Alex, >>>>> >>>>> I thought of having some print in register emulation if QEMU >>>>> is using debug resource, Also when QEMU overwrites guest written >>>>> values but that looks excessive. If I uses some variable which >>>>> get set when guest starts using debug registers and check in >>>>> debug set ioctl then that look ugly. Looking for suggestions >>>> Whatever you do, have QEMU do the print, not the kernel. >>> How would that be accomplished? How would the kernel know to exit to >>> QEMU, and how would the exit reason be conveyed? >> >> QEMU is the one forcefully enabling debug and overwriting guest debug >> registers, so it also knows when it did overwrite valid ones. > > QEMU knows when it overwrites the guest values, but it doesn't know if, > after enabling host debug, the guest tries to write to the debug > registers and it gets nopped. If we keep the EDM setting, then we can > at least say the situation is no worse than with a JTAG. Yeah, I think that's perfectly reasonable. I don't think it'll be likely that a user starts debugging with qemu and then expects guest debugging to work. The other way around is more likely and would warrant a warning to the user - if we care. Alex -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> Am 30.07.2014 um 07:43 schrieb "Bharat.Bhushan@freescale.com" <Bharat.Bhushan@freescale.com>: > > > >> -----Original Message----- >> From: Wood Scott-B07421 >> Sent: Tuesday, July 29, 2014 11:20 PM >> To: Alexander Graf >> Cc: Bhushan Bharat-R65777; kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; Yoder >> Stuart-B08248 >> Subject: Re: [PATCH 6/6] KVM: PPC: BOOKE: Emulate debug registers and exception >> >>> On Tue, 2014-07-29 at 16:06 +0200, Alexander Graf wrote: >>>> On 29.07.14 00:33, Scott Wood wrote: >>>>> On Mon, 2014-07-28 at 16:04 +0200, Alexander Graf wrote: >>>>>> On 11.07.14 10:39, Bharat Bhushan wrote: >>>>>> This patch emulates debug registers and debug exception to support >>>>>> guest using debug resource. This enables running gdb/kgdb etc in >>>>>> guest. >>>>>> >>>>>> On BOOKE architecture we cannot share debug resources between QEMU >>>>>> and guest because: >>>>>> When QEMU is using debug resources then debug exception must >>>>>> be always enabled. To achieve this we set MSR_DE and also set >>>>>> MSRP_DEP so guest cannot change MSR_DE. >>>>>> >>>>>> When emulating debug resource for guest we want guest >>>>>> to control MSR_DE (enable/disable debug interrupt on need). >>>>>> >>>>>> So above mentioned two configuration cannot be supported >>>>>> at the same time. So the result is that we cannot share >>>>>> debug resources between QEMU and Guest on BOOKE architecture. >>>>>> >>>>>> In the current design QEMU gets priority over guest, this means >>>>>> that if QEMU is using debug resources then guest cannot use them >>>>>> and if guest is using debug resource then QEMU can overwrite them. >>>>>> >>>>>> Signed-off-by: Bharat Bhushan <Bharat.Bhushan@freescale.com> >>>>>> --- >>>>>> Hi Alex, >>>>>> >>>>>> I thought of having some print in register emulation if QEMU is >>>>>> using debug resource, Also when QEMU overwrites guest written >>>>>> values but that looks excessive. If I uses some variable which get >>>>>> set when guest starts using debug registers and check in debug set >>>>>> ioctl then that look ugly. Looking for suggestions >>>>> Whatever you do, have QEMU do the print, not the kernel. >>>> How would that be accomplished? How would the kernel know to exit >>>> to QEMU, and how would the exit reason be conveyed? >>> >>> QEMU is the one forcefully enabling debug and overwriting guest debug >>> registers, so it also knows when it did overwrite valid ones. >> >> QEMU knows when it overwrites the guest values, but it doesn't know if, after >> enabling host debug, the guest tries to write to the debug registers and it gets >> nopped. > > Do we want that QEMU first get DBCR0 to know whether it is overwriting whenever set/clear debug register? If you want to implement a warning, yes. But that csn easily be a follow-up. Let's get something properly working upstream first. Alex -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: Alexander Graf [mailto:agraf@suse.de] > Sent: Monday, July 28, 2014 7:35 PM > To: Bhushan Bharat-R65777; kvm-ppc@vger.kernel.org > Cc: kvm@vger.kernel.org; Wood Scott-B07421; Yoder Stuart-B08248 > Subject: Re: [PATCH 6/6] KVM: PPC: BOOKE: Emulate debug registers and exception > > > On 11.07.14 10:39, Bharat Bhushan wrote: > > This patch emulates debug registers and debug exception to support > > guest using debug resource. This enables running gdb/kgdb etc in > > guest. > > > > On BOOKE architecture we cannot share debug resources between QEMU and > > guest because: > > When QEMU is using debug resources then debug exception must > > be always enabled. To achieve this we set MSR_DE and also set > > MSRP_DEP so guest cannot change MSR_DE. > > > > When emulating debug resource for guest we want guest > > to control MSR_DE (enable/disable debug interrupt on need). > > > > So above mentioned two configuration cannot be supported > > at the same time. So the result is that we cannot share > > debug resources between QEMU and Guest on BOOKE architecture. > > > > In the current design QEMU gets priority over guest, this means that > > if QEMU is using debug resources then guest cannot use them and if > > guest is using debug resource then QEMU can overwrite them. > > > > Signed-off-by: Bharat Bhushan <Bharat.Bhushan@freescale.com> > > --- > > Hi Alex, > > > > I thought of having some print in register emulation if QEMU is using > > debug resource, Also when QEMU overwrites guest written values but > > that looks excessive. If I uses some variable which get set when guest > > starts using debug registers and check in debug set ioctl then that > > look ugly. Looking for suggestions > > Whatever you do, have QEMU do the print, not the kernel. > > > > > arch/powerpc/include/asm/kvm_ppc.h | 3 + > > arch/powerpc/kvm/booke.c | 27 +++++++ > > arch/powerpc/kvm/booke_emulate.c | 157 > +++++++++++++++++++++++++++++++++++++ > > 3 files changed, 187 insertions(+) > > > > diff --git a/arch/powerpc/include/asm/kvm_ppc.h > > b/arch/powerpc/include/asm/kvm_ppc.h > > index e2fd5a1..f3f7611 100644 > > --- a/arch/powerpc/include/asm/kvm_ppc.h > > +++ b/arch/powerpc/include/asm/kvm_ppc.h > > @@ -173,6 +173,9 @@ extern int kvmppc_xics_get_xive(struct kvm *kvm, u32 irq, > u32 *server, > > extern int kvmppc_xics_int_on(struct kvm *kvm, u32 irq); > > extern int kvmppc_xics_int_off(struct kvm *kvm, u32 irq); > > > > +void kvmppc_core_dequeue_debug(struct kvm_vcpu *vcpu); void > > +kvmppc_core_queue_debug(struct kvm_vcpu *vcpu); > > + > > union kvmppc_one_reg { > > u32 wval; > > u64 dval; > > diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c index > > fadfe76..c2471ed 100644 > > --- a/arch/powerpc/kvm/booke.c > > +++ b/arch/powerpc/kvm/booke.c > > @@ -264,6 +264,16 @@ static void kvmppc_core_dequeue_watchdog(struct kvm_vcpu > *vcpu) > > clear_bit(BOOKE_IRQPRIO_WATCHDOG, &vcpu->arch.pending_exceptions); > > } > > > > +void kvmppc_core_queue_debug(struct kvm_vcpu *vcpu) { > > + kvmppc_booke_queue_irqprio(vcpu, BOOKE_IRQPRIO_DEBUG); } > > + > > +void kvmppc_core_dequeue_debug(struct kvm_vcpu *vcpu) { > > + clear_bit(BOOKE_IRQPRIO_DEBUG, &vcpu->arch.pending_exceptions); } > > + > > static void set_guest_srr(struct kvm_vcpu *vcpu, unsigned long srr0, u32 > srr1) > > { > > #ifdef CONFIG_KVM_BOOKE_HV > > @@ -783,6 +793,23 @@ static int kvmppc_handle_debug(struct kvm_run *run, > struct kvm_vcpu *vcpu) > > struct debug_reg *dbg_reg = &(vcpu->arch.shadow_dbg_reg); > > u32 dbsr = vcpu->arch.dbsr; > > > > + if (vcpu->guest_debug == 0) { > > + /* Debug resources belong to Guest */ > > + if (dbsr && (vcpu->arch.shared->msr & MSR_DE)) > > + kvmppc_core_queue_debug(vcpu); > > + > > + /* Inject a program interrupt if trap debug is not allowed */ > > + if ((dbsr & DBSR_TIE) && !(vcpu->arch.shared->msr & MSR_DE)) > > + kvmppc_core_queue_program(vcpu, ESR_PTR); > > In that case we would've received a program interrupt and never entered this > code path, no? Yes for HV. But for PR we can be here, MSR_DE is set in h/w msr and guest MSR_DE is not set. Having a ifdef does not look good but we can have a comment here. Thanks -Bharat > > > Alex -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2014-07-30 at 01:43 -0500, Bhushan Bharat-R65777 wrote: > > > -----Original Message----- > > From: Wood Scott-B07421 > > Sent: Tuesday, July 29, 2014 3:58 AM > > To: Bhushan Bharat-R65777 > > Cc: agraf@suse.de; kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; Yoder Stuart- > > B08248 > > Subject: Re: [PATCH 6/6] KVM: PPC: BOOKE: Emulate debug registers and exception > > > > Userspace might be interested in > > the raw value, > > With the current design, If userspace is interested then it will not > get the DBSR. Oh, because DBSR isn't currently implemented in sregs or one reg? > But why userspace will be interested? Do you expose all of the hardware's debugging features in your high-level interface? > > plus it's a change from the current API semantics. > > Can you please let us know how ? It looked like it was removing dbsr visibility and the requirement for userspace to clear dbsr. I guess the old way was that the value in vcpu->arch.dbsr didn't matter until the next debug exception, when it would be overwritten by the new SPRN_DBSR? > > > + case SPRN_DBCR2: > > > + /* > > > + * If userspace is debugging guest then guest > > > + * can not access debug registers. > > > + */ > > > + if (vcpu->guest_debug) > > > + break; > > > + > > > + debug_inst = true; > > > + vcpu->arch.dbg_reg.dbcr2 = spr_val; > > > + vcpu->arch.shadow_dbg_reg.dbcr2 = spr_val; > > > break; > > > > In what circumstances can the architected and shadow registers differ? > > As of now they are same. But I think that if we want to implement other features like "Freeze Timer (FT)" then they can be different. I don't think we can possibly implement Freeze Timer. > > > case SPRN_DBSR: > > > + /* > > > + * If userspace is debugging guest then guest > > > + * can not access debug registers. > > > + */ > > > + if (vcpu->guest_debug) > > > + break; > > > + > > > vcpu->arch.dbsr &= ~spr_val; > > > + if (vcpu->arch.dbsr == 0) > > > + kvmppc_core_dequeue_debug(vcpu); > > > break; > > > > Not all DBSR bits cause an exception, e.g. IDE and MRR. > > I am not sure what we should in that case ? > > As we are currently emulating a subset of debug events (IAC, DAC, IC, > BT and TIE --- DBCR0 emulation) then we should expose status of those > events in guest dbsr and rest should be cleared ? I'm not saying they need to be exposed to the guest, but I don't see where you filter out bits like these. > > > @@ -273,6 +397,10 @@ int kvmppc_booke_emulate_mtspr(struct kvm_vcpu *vcpu, int > > sprn, ulong spr_val) > > > emulated = EMULATE_FAIL; > > > } > > > > > > + if (debug_inst) { > > > + switch_booke_debug_regs(&vcpu->arch.shadow_dbg_reg); > > > + current->thread.debug = vcpu->arch.shadow_dbg_reg; > > > + } > > > > Could you explain what's going on with regard to copying the registers > > into current->thread.debug? Why is it done after loading the registers > > into the hardware (is there a race if we get preempted in the middle)? > > Yes, and this was something I was not clear when writing this code. > Should we have preempt disable-enable around this. Can they be reordered instead? -Scott -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: Wood Scott-B07421 > Sent: Thursday, July 31, 2014 8:18 AM > To: Bhushan Bharat-R65777 > Cc: agraf@suse.de; kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; Yoder Stuart- > B08248 > Subject: Re: [PATCH 6/6] KVM: PPC: BOOKE: Emulate debug registers and exception > > On Wed, 2014-07-30 at 01:43 -0500, Bhushan Bharat-R65777 wrote: > > > > > -----Original Message----- > > > From: Wood Scott-B07421 > > > Sent: Tuesday, July 29, 2014 3:58 AM > > > To: Bhushan Bharat-R65777 > > > Cc: agraf@suse.de; kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; > > > Yoder Stuart- > > > B08248 > > > Subject: Re: [PATCH 6/6] KVM: PPC: BOOKE: Emulate debug registers > > > and exception > > > > > > Userspace might be interested in > > > the raw value, > > > > With the current design, If userspace is interested then it will not > > get the DBSR. > > Oh, because DBSR isn't currently implemented in sregs or one reg? That is one reason. Another is that if we give dbsr visibility to userspace then userspace have to clear dbsr in handling KVM_EXIT_DEBUG. And we think there is no gain in doing that because " - QEMU cannot inject debug interrupt to guest (as this does not know guest ability to handle debug interrupt; MSR_DE), so will always clear DBSR. - If QEMU has to always clear DBSR in handling KVM_EXIT_DEBUG then this (clearing dbsr in kernel) avoid doing in SET_SREGS/set_one_reg() " This makes dbsr not visible to userspace. Also this (clearing of dbsr) should not be part of this patch, this should be a separate patch. I will do that in next version. > > > But why userspace will be interested? > > Do you expose all of the hardware's debugging features in your high-level > interface? We support h/w breakpoint, watchpoint and IC (single stepping) and status in userspace exit provide all required information to userspace. > > > > plus it's a change from the current API semantics. > > > > Can you please let us know how ? > > It looked like it was removing dbsr visibility and the requirement for userspace > to clear dbsr. I guess the old way was that the value in > vcpu->arch.dbsr didn't matter until the next debug exception, when it > would be overwritten by the new SPRN_DBSR? But that means old dbsr will be visibility to userspace, which is even bad than not visible, no? Also this can lead to old dbsr visible to guest once userspace releases debug resources, but this can be solved by clearing dbsr in kvm_arch_vcpu_ioctl_set_guest_debug() -> " if (!(dbg->control & KVM_GUESTDBG_ENABLE)) { }". > > > > > + case SPRN_DBCR2: > > > > + /* > > > > + * If userspace is debugging guest then guest > > > > + * can not access debug registers. > > > > + */ > > > > + if (vcpu->guest_debug) > > > > + break; > > > > + > > > > + debug_inst = true; > > > > + vcpu->arch.dbg_reg.dbcr2 = spr_val; > > > > + vcpu->arch.shadow_dbg_reg.dbcr2 = spr_val; > > > > break; > > > > > > In what circumstances can the architected and shadow registers differ? > > > > As of now they are same. But I think that if we want to implement other > features like "Freeze Timer (FT)" then they can be different. > > I don't think we can possibly implement Freeze Timer. May be, but in my opinion we should keep this open. > > > > > case SPRN_DBSR: > > > > + /* > > > > + * If userspace is debugging guest then guest > > > > + * can not access debug registers. > > > > + */ > > > > + if (vcpu->guest_debug) > > > > + break; > > > > + > > > > vcpu->arch.dbsr &= ~spr_val; > > > > + if (vcpu->arch.dbsr == 0) > > > > + kvmppc_core_dequeue_debug(vcpu); > > > > break; > > > > > > Not all DBSR bits cause an exception, e.g. IDE and MRR. > > > > I am not sure what we should in that case ? > > > > As we are currently emulating a subset of debug events (IAC, DAC, IC, > > BT and TIE --- DBCR0 emulation) then we should expose status of those > > events in guest dbsr and rest should be cleared ? > > I'm not saying they need to be exposed to the guest, but I don't see where you > filter out bits like these. I am trying to get what all bits should be filtered out, all bits except IACx, DACx, IC, BT and TIE (same as event set filtering done when setting DBCR0) ? i.e IDE, UDE, MRR, IRPT, RET, CIRPT, CRET should be filtered out? > > > > > @@ -273,6 +397,10 @@ int kvmppc_booke_emulate_mtspr(struct > > > > kvm_vcpu *vcpu, int > > > sprn, ulong spr_val) > > > > emulated = EMULATE_FAIL; > > > > } > > > > > > > > + if (debug_inst) { > > > > + switch_booke_debug_regs(&vcpu->arch.shadow_dbg_reg); > > > > + current->thread.debug = vcpu->arch.shadow_dbg_reg; > > > > + } > > > > > > Could you explain what's going on with regard to copying the > > > registers into current->thread.debug? Why is it done after loading > > > the registers into the hardware (is there a race if we get preempted in the > middle)? > > > > Yes, and this was something I was not clear when writing this code. > > Should we have preempt disable-enable around this. > > Can they be reordered instead? Thanks; Yes, that will work :) Regards -Bharat > > -Scott >
On Thu, 2014-07-31 at 01:15 -0500, Bhushan Bharat-R65777 wrote: > > > -----Original Message----- > > From: Wood Scott-B07421 > > Sent: Thursday, July 31, 2014 8:18 AM > > To: Bhushan Bharat-R65777 > > Cc: agraf@suse.de; kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; Yoder Stuart- > > B08248 > > Subject: Re: [PATCH 6/6] KVM: PPC: BOOKE: Emulate debug registers and exception > > > > On Wed, 2014-07-30 at 01:43 -0500, Bhushan Bharat-R65777 wrote: > > > > > > > -----Original Message----- > > > > From: Wood Scott-B07421 > > > > Sent: Tuesday, July 29, 2014 3:58 AM > > > > To: Bhushan Bharat-R65777 > > > > Cc: agraf@suse.de; kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; > > > > Yoder Stuart- > > > > B08248 > > > > Subject: Re: [PATCH 6/6] KVM: PPC: BOOKE: Emulate debug registers > > > > and exception > > > > > > > > Userspace might be interested in > > > > the raw value, > > > > > > With the current design, If userspace is interested then it will not > > > get the DBSR. > > > > Oh, because DBSR isn't currently implemented in sregs or one reg? > > That is one reason. Another is that if we give dbsr visibility to > userspace then userspace have to clear dbsr in handling KVM_EXIT_DEBUG. Right -- since I didn't realize DBSR wasn't already exposed, I thought userspace already had this responsibility. > > It looked like it was removing dbsr visibility and the requirement for userspace > > to clear dbsr. I guess the old way was that the value in > > vcpu->arch.dbsr didn't matter until the next debug exception, when it > > would be overwritten by the new SPRN_DBSR? > > But that means old dbsr will be visibility to userspace, which is even bad than not visible, no? > > Also this can lead to old dbsr visible to guest once userspace releases > debug resources, but this can be solved by clearing dbsr in > kvm_arch_vcpu_ioctl_set_guest_debug() -> " if (!(dbg->control & > KVM_GUESTDBG_ENABLE)) { }". I wasn't suggesting that you keep it that way, just clarifying my understanding of the current code. > > > > > > > > + case SPRN_DBCR2: > > > > > + /* > > > > > + * If userspace is debugging guest then guest > > > > > + * can not access debug registers. > > > > > + */ > > > > > + if (vcpu->guest_debug) > > > > > + break; > > > > > + > > > > > + debug_inst = true; > > > > > + vcpu->arch.dbg_reg.dbcr2 = spr_val; > > > > > + vcpu->arch.shadow_dbg_reg.dbcr2 = spr_val; > > > > > break; > > > > > > > > In what circumstances can the architected and shadow registers differ? > > > > > > As of now they are same. But I think that if we want to implement other > > features like "Freeze Timer (FT)" then they can be different. > > > > I don't think we can possibly implement Freeze Timer. > > May be, but in my opinion we should keep this open. We're not talking about API here -- the implementation should be kept simple if there's no imminent need for shadow registers. > > > I am not sure what we should in that case ? > > > > > > As we are currently emulating a subset of debug events (IAC, DAC, IC, > > > BT and TIE --- DBCR0 emulation) then we should expose status of those > > > events in guest dbsr and rest should be cleared ? > > > > I'm not saying they need to be exposed to the guest, but I don't see where you > > filter out bits like these. > > I am trying to get what all bits should be filtered out, all bits > except IACx, DACx, IC, BT and TIE (same as event set filtering done > when setting DBCR0) ? > > i.e IDE, UDE, MRR, IRPT, RET, CIRPT, CRET should be filtered out? Bits like IRPT and RET don't really matter, as you shouldn't see them happen. Likewise MRR if you're sure you've cleared it since boot. But IDE could be set any time an asynchronous exception happens. I don't think you should filter it out, but instead make sure that it doesn't cause an exception to be delivered. -Scott -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: Wood Scott-B07421 > Sent: Friday, August 01, 2014 2:16 AM > To: Bhushan Bharat-R65777 > Cc: agraf@suse.de; kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; Yoder Stuart- > B08248 > Subject: Re: [PATCH 6/6] KVM: PPC: BOOKE: Emulate debug registers and exception > > On Thu, 2014-07-31 at 01:15 -0500, Bhushan Bharat-R65777 wrote: > > > > > -----Original Message----- > > > From: Wood Scott-B07421 > > > Sent: Thursday, July 31, 2014 8:18 AM > > > To: Bhushan Bharat-R65777 > > > Cc: agraf@suse.de; kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; Yoder > Stuart- > > > B08248 > > > Subject: Re: [PATCH 6/6] KVM: PPC: BOOKE: Emulate debug registers and > exception > > > > > > On Wed, 2014-07-30 at 01:43 -0500, Bhushan Bharat-R65777 wrote: > > > > > > > > > -----Original Message----- > > > > > From: Wood Scott-B07421 > > > > > Sent: Tuesday, July 29, 2014 3:58 AM > > > > > To: Bhushan Bharat-R65777 > > > > > Cc: agraf@suse.de; kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; > > > > > Yoder Stuart- > > > > > B08248 > > > > > Subject: Re: [PATCH 6/6] KVM: PPC: BOOKE: Emulate debug registers > > > > > and exception > > > > > > > > > > Userspace might be interested in > > > > > the raw value, > > > > > > > > With the current design, If userspace is interested then it will not > > > > get the DBSR. > > > > > > Oh, because DBSR isn't currently implemented in sregs or one reg? > > > > That is one reason. Another is that if we give dbsr visibility to > > userspace then userspace have to clear dbsr in handling KVM_EXIT_DEBUG. > > Right -- since I didn't realize DBSR wasn't already exposed, I thought > userspace already had this responsibility. > > > > It looked like it was removing dbsr visibility and the requirement for > userspace > > > to clear dbsr. I guess the old way was that the value in > > > vcpu->arch.dbsr didn't matter until the next debug exception, when it > > > would be overwritten by the new SPRN_DBSR? > > > > But that means old dbsr will be visibility to userspace, which is even bad > than not visible, no? > > > > Also this can lead to old dbsr visible to guest once userspace releases > > debug resources, but this can be solved by clearing dbsr in > > kvm_arch_vcpu_ioctl_set_guest_debug() -> " if (!(dbg->control & > > KVM_GUESTDBG_ENABLE)) { }". > > I wasn't suggesting that you keep it that way, just clarifying my > understanding of the current code. > > > > > > > > > > > > > + case SPRN_DBCR2: > > > > > > + /* > > > > > > + * If userspace is debugging guest then guest > > > > > > + * can not access debug registers. > > > > > > + */ > > > > > > + if (vcpu->guest_debug) > > > > > > + break; > > > > > > + > > > > > > + debug_inst = true; > > > > > > + vcpu->arch.dbg_reg.dbcr2 = spr_val; > > > > > > + vcpu->arch.shadow_dbg_reg.dbcr2 = spr_val; > > > > > > break; > > > > > > > > > > In what circumstances can the architected and shadow registers differ? > > > > > > > > As of now they are same. But I think that if we want to implement other > > > features like "Freeze Timer (FT)" then they can be different. > > > > > > I don't think we can possibly implement Freeze Timer. > > > > May be, but in my opinion we should keep this open. > > We're not talking about API here -- the implementation should be kept > simple if there's no imminent need for shadow registers. > > > > > I am not sure what we should in that case ? > > > > > > > > As we are currently emulating a subset of debug events (IAC, DAC, IC, > > > > BT and TIE --- DBCR0 emulation) then we should expose status of those > > > > events in guest dbsr and rest should be cleared ? > > > > > > I'm not saying they need to be exposed to the guest, but I don't see where > you > > > filter out bits like these. > > > > I am trying to get what all bits should be filtered out, all bits > > except IACx, DACx, IC, BT and TIE (same as event set filtering done > > when setting DBCR0) ? > > > > i.e IDE, UDE, MRR, IRPT, RET, CIRPT, CRET should be filtered out? > > Bits like IRPT and RET don't really matter, as you shouldn't see them > happen. Likewise MRR if you're sure you've cleared it since boot. We can clear MRR bits when update vcpu->arch->dbsr with SPRM_DBSR in kvm debug handler > But > IDE could be set any time an asynchronous exception happens. I don't > think you should filter it out, but instead make sure that it doesn't > cause an exception to be delivered. So this means that in kvmpp_handle_debug() if DBSR_IDE is set then do not inject debug interrupt and on dbsr write emulation, deque the debug interrupt even if DBSR_IDE is set. case SPRN_DBSR: vcpu->arch.dbsr &= ~spr_val; if (!(vcpu->arch.dbsr & ~DBSR_IDE)) kvmppc_core_dequeue_debug(vcpu); break; or vcpu->arch.dbsr &= ~(spr_val | DBSR_IDE); if (!vcpu->arch.dbsr) kvmppc_core_dequeue_debug(vcpu); break; Thanks -Bharat > > -Scott >
On Fri, 2014-08-01 at 04:34 -0500, Bhushan Bharat-R65777 wrote: > on dbsr write emulation, deque the debug interrupt even if DBSR_IDE is set. > > case SPRN_DBSR: > > vcpu->arch.dbsr &= ~spr_val; > if (!(vcpu->arch.dbsr & ~DBSR_IDE)) > kvmppc_core_dequeue_debug(vcpu); > break; > > or > vcpu->arch.dbsr &= ~(spr_val | DBSR_IDE); > if (!vcpu->arch.dbsr) > kvmppc_core_dequeue_debug(vcpu); > break; The first option. I see no reason to have KVM forcibly clear DBSR[IDE]. -Scott -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/powerpc/include/asm/kvm_ppc.h b/arch/powerpc/include/asm/kvm_ppc.h index e2fd5a1..f3f7611 100644 --- a/arch/powerpc/include/asm/kvm_ppc.h +++ b/arch/powerpc/include/asm/kvm_ppc.h @@ -173,6 +173,9 @@ extern int kvmppc_xics_get_xive(struct kvm *kvm, u32 irq, u32 *server, extern int kvmppc_xics_int_on(struct kvm *kvm, u32 irq); extern int kvmppc_xics_int_off(struct kvm *kvm, u32 irq); +void kvmppc_core_dequeue_debug(struct kvm_vcpu *vcpu); +void kvmppc_core_queue_debug(struct kvm_vcpu *vcpu); + union kvmppc_one_reg { u32 wval; u64 dval; diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c index fadfe76..c2471ed 100644 --- a/arch/powerpc/kvm/booke.c +++ b/arch/powerpc/kvm/booke.c @@ -264,6 +264,16 @@ static void kvmppc_core_dequeue_watchdog(struct kvm_vcpu *vcpu) clear_bit(BOOKE_IRQPRIO_WATCHDOG, &vcpu->arch.pending_exceptions); } +void kvmppc_core_queue_debug(struct kvm_vcpu *vcpu) +{ + kvmppc_booke_queue_irqprio(vcpu, BOOKE_IRQPRIO_DEBUG); +} + +void kvmppc_core_dequeue_debug(struct kvm_vcpu *vcpu) +{ + clear_bit(BOOKE_IRQPRIO_DEBUG, &vcpu->arch.pending_exceptions); +} + static void set_guest_srr(struct kvm_vcpu *vcpu, unsigned long srr0, u32 srr1) { #ifdef CONFIG_KVM_BOOKE_HV @@ -783,6 +793,23 @@ static int kvmppc_handle_debug(struct kvm_run *run, struct kvm_vcpu *vcpu) struct debug_reg *dbg_reg = &(vcpu->arch.shadow_dbg_reg); u32 dbsr = vcpu->arch.dbsr; + if (vcpu->guest_debug == 0) { + /* Debug resources belong to Guest */ + if (dbsr && (vcpu->arch.shared->msr & MSR_DE)) + kvmppc_core_queue_debug(vcpu); + + /* Inject a program interrupt if trap debug is not allowed */ + if ((dbsr & DBSR_TIE) && !(vcpu->arch.shared->msr & MSR_DE)) + kvmppc_core_queue_program(vcpu, ESR_PTR); + + return RESUME_GUEST; + } + + /* + * Prepare for userspace exit as debug resources + * are owned by userspace. + */ + vcpu->arch.dbsr = 0; run->debug.arch.status = 0; run->debug.arch.address = vcpu->arch.pc; diff --git a/arch/powerpc/kvm/booke_emulate.c b/arch/powerpc/kvm/booke_emulate.c index 2a20194..3d143fe 100644 --- a/arch/powerpc/kvm/booke_emulate.c +++ b/arch/powerpc/kvm/booke_emulate.c @@ -139,6 +139,7 @@ int kvmppc_booke_emulate_op(struct kvm_run *run, struct kvm_vcpu *vcpu, int kvmppc_booke_emulate_mtspr(struct kvm_vcpu *vcpu, int sprn, ulong spr_val) { int emulated = EMULATE_DONE; + bool debug_inst = false; switch (sprn) { case SPRN_DEAR: @@ -153,14 +154,137 @@ int kvmppc_booke_emulate_mtspr(struct kvm_vcpu *vcpu, int sprn, ulong spr_val) case SPRN_CSRR1: vcpu->arch.csrr1 = spr_val; break; + case SPRN_DSRR0: + vcpu->arch.dsrr0 = spr_val; + break; + case SPRN_DSRR1: + vcpu->arch.dsrr1 = spr_val; + break; + case SPRN_IAC1: + /* + * If userspace is debugging guest then guest + * can not access debug registers. + */ + if (vcpu->guest_debug) + break; + + debug_inst = true; + vcpu->arch.dbg_reg.iac1 = spr_val; + vcpu->arch.shadow_dbg_reg.iac1 = spr_val; + break; + case SPRN_IAC2: + /* + * If userspace is debugging guest then guest + * can not access debug registers. + */ + if (vcpu->guest_debug) + break; + + debug_inst = true; + vcpu->arch.dbg_reg.iac2 = spr_val; + vcpu->arch.shadow_dbg_reg.iac2 = spr_val; + break; +#if CONFIG_PPC_ADV_DEBUG_IACS > 2 + case SPRN_IAC3: + /* + * If userspace is debugging guest then guest + * can not access debug registers. + */ + if (vcpu->guest_debug) + break; + + debug_inst = true; + vcpu->arch.dbg_reg.iac3 = spr_val; + vcpu->arch.shadow_dbg_reg.iac3 = spr_val; + break; + case SPRN_IAC4: + /* + * If userspace is debugging guest then guest + * can not access debug registers. + */ + if (vcpu->guest_debug) + break; + + debug_inst = true; + vcpu->arch.dbg_reg.iac4 = spr_val; + vcpu->arch.shadow_dbg_reg.iac4 = spr_val; + break; +#endif + case SPRN_DAC1: + /* + * If userspace is debugging guest then guest + * can not access debug registers. + */ + if (vcpu->guest_debug) + break; + + debug_inst = true; + vcpu->arch.dbg_reg.dac1 = spr_val; + vcpu->arch.shadow_dbg_reg.dac1 = spr_val; + break; + case SPRN_DAC2: + /* + * If userspace is debugging guest then guest + * can not access debug registers. + */ + if (vcpu->guest_debug) + break; + + debug_inst = true; + vcpu->arch.dbg_reg.dac2 = spr_val; + vcpu->arch.shadow_dbg_reg.dac2 = spr_val; + break; case SPRN_DBCR0: + /* + * If userspace is debugging guest then guest + * can not access debug registers. + */ + if (vcpu->guest_debug) + break; + + debug_inst = true; + spr_val &= (DBCR0_IDM | DBCR0_IC | DBCR0_BT | DBCR0_TIE | + DBCR0_IAC1 | DBCR0_IAC2 | DBCR0_IAC3 | DBCR0_IAC4 | + DBCR0_DAC1R | DBCR0_DAC1W | DBCR0_DAC2R | DBCR0_DAC2W); + vcpu->arch.dbg_reg.dbcr0 = spr_val; + vcpu->arch.shadow_dbg_reg.dbcr0 = spr_val; break; case SPRN_DBCR1: + /* + * If userspace is debugging guest then guest + * can not access debug registers. + */ + if (vcpu->guest_debug) + break; + + debug_inst = true; vcpu->arch.dbg_reg.dbcr1 = spr_val; + vcpu->arch.shadow_dbg_reg.dbcr1 = spr_val; + break; + case SPRN_DBCR2: + /* + * If userspace is debugging guest then guest + * can not access debug registers. + */ + if (vcpu->guest_debug) + break; + + debug_inst = true; + vcpu->arch.dbg_reg.dbcr2 = spr_val; + vcpu->arch.shadow_dbg_reg.dbcr2 = spr_val; break; case SPRN_DBSR: + /* + * If userspace is debugging guest then guest + * can not access debug registers. + */ + if (vcpu->guest_debug) + break; + vcpu->arch.dbsr &= ~spr_val; + if (vcpu->arch.dbsr == 0) + kvmppc_core_dequeue_debug(vcpu); break; case SPRN_TSR: kvmppc_clr_tsr_bits(vcpu, spr_val); @@ -273,6 +397,10 @@ int kvmppc_booke_emulate_mtspr(struct kvm_vcpu *vcpu, int sprn, ulong spr_val) emulated = EMULATE_FAIL; } + if (debug_inst) { + switch_booke_debug_regs(&vcpu->arch.shadow_dbg_reg); + current->thread.debug = vcpu->arch.shadow_dbg_reg; + } return emulated; } @@ -299,12 +427,41 @@ int kvmppc_booke_emulate_mfspr(struct kvm_vcpu *vcpu, int sprn, ulong *spr_val) case SPRN_CSRR1: *spr_val = vcpu->arch.csrr1; break; + case SPRN_DSRR0: + *spr_val = vcpu->arch.dsrr0; + break; + case SPRN_DSRR1: + *spr_val = vcpu->arch.dsrr1; + break; + case SPRN_IAC1: + *spr_val = vcpu->arch.dbg_reg.iac1; + break; + case SPRN_IAC2: + *spr_val = vcpu->arch.dbg_reg.iac2; + break; +#if CONFIG_PPC_ADV_DEBUG_IACS > 2 + case SPRN_IAC3: + *spr_val = vcpu->arch.dbg_reg.iac3; + break; + case SPRN_IAC4: + *spr_val = vcpu->arch.dbg_reg.iac4; + break; +#endif + case SPRN_DAC1: + *spr_val = vcpu->arch.dbg_reg.dac1; + break; + case SPRN_DAC2: + *spr_val = vcpu->arch.dbg_reg.dac2; + break; case SPRN_DBCR0: *spr_val = vcpu->arch.dbg_reg.dbcr0; break; case SPRN_DBCR1: *spr_val = vcpu->arch.dbg_reg.dbcr1; break; + case SPRN_DBCR2: + *spr_val = vcpu->arch.dbg_reg.dbcr2; + break; case SPRN_DBSR: *spr_val = vcpu->arch.dbsr; break;
This patch emulates debug registers and debug exception to support guest using debug resource. This enables running gdb/kgdb etc in guest. On BOOKE architecture we cannot share debug resources between QEMU and guest because: When QEMU is using debug resources then debug exception must be always enabled. To achieve this we set MSR_DE and also set MSRP_DEP so guest cannot change MSR_DE. When emulating debug resource for guest we want guest to control MSR_DE (enable/disable debug interrupt on need). So above mentioned two configuration cannot be supported at the same time. So the result is that we cannot share debug resources between QEMU and Guest on BOOKE architecture. In the current design QEMU gets priority over guest, this means that if QEMU is using debug resources then guest cannot use them and if guest is using debug resource then QEMU can overwrite them. Signed-off-by: Bharat Bhushan <Bharat.Bhushan@freescale.com> --- Hi Alex, I thought of having some print in register emulation if QEMU is using debug resource, Also when QEMU overwrites guest written values but that looks excessive. If I uses some variable which get set when guest starts using debug registers and check in debug set ioctl then that look ugly. Looking for suggestions arch/powerpc/include/asm/kvm_ppc.h | 3 + arch/powerpc/kvm/booke.c | 27 +++++++ arch/powerpc/kvm/booke_emulate.c | 157 +++++++++++++++++++++++++++++++++++++ 3 files changed, 187 insertions(+)