> -----Original Message----- > From: Alexander Graf [mailto:agraf@xxxxxxx] > Sent: Thursday, January 31, 2013 5:47 PM > To: Bhushan Bharat-R65777 > Cc: kvm-ppc@xxxxxxxxxxxxxxx; kvm@xxxxxxxxxxxxxxx > Subject: Re: [PATCH 3/8] KVM: PPC: booke: Added debug handler > > > On 30.01.2013, at 12:30, Bhushan Bharat-R65777 wrote: > > > > > > >> -----Original Message----- > >> From: Alexander Graf [mailto:agraf@xxxxxxx] > >> Sent: Friday, January 25, 2013 5:13 PM > >> To: Bhushan Bharat-R65777 > >> Cc: kvm-ppc@xxxxxxxxxxxxxxx; kvm@xxxxxxxxxxxxxxx; Bhushan > >> Bharat-R65777 > >> Subject: Re: [PATCH 3/8] KVM: PPC: booke: Added debug handler > >> > >> > >> On 16.01.2013, at 09:24, Bharat Bhushan wrote: > >> > >>> From: Bharat Bhushan <Bharat.Bhushan@xxxxxxxxxxxxx> > >>> > >>> Installed debug handler will be used for guest debug support and > >>> debug facility emulation features (patches for these features will > >>> follow this patch). > >>> > >>> Signed-off-by: Liu Yu <yu.liu@xxxxxxxxxxxxx> > >>> [bharat.bhushan@xxxxxxxxxxxxx: Substantial changes] > >>> Signed-off-by: Bharat Bhushan <bharat.bhushan@xxxxxxxxxxxxx> > >>> --- > >>> arch/powerpc/include/asm/kvm_host.h | 1 + > >>> arch/powerpc/kernel/asm-offsets.c | 1 + > >>> arch/powerpc/kvm/booke_interrupts.S | 49 ++++++++++++++++++++++++++++++--- > -- > >>> 3 files changed, 44 insertions(+), 7 deletions(-) > >>> > >>> diff --git a/arch/powerpc/include/asm/kvm_host.h > >>> b/arch/powerpc/include/asm/kvm_host.h > >>> index 8a72d59..f4ba881 100644 > >>> --- a/arch/powerpc/include/asm/kvm_host.h > >>> +++ b/arch/powerpc/include/asm/kvm_host.h > >>> @@ -503,6 +503,7 @@ struct kvm_vcpu_arch { > >>> u32 tlbcfg[4]; > >>> u32 mmucfg; > >>> u32 epr; > >>> + u32 crit_save; > >>> struct kvmppc_booke_debug_reg dbg_reg; #endif > >>> gpa_t paddr_accessed; > >>> diff --git a/arch/powerpc/kernel/asm-offsets.c > >>> b/arch/powerpc/kernel/asm-offsets.c > >>> index 46f6afd..02048f3 100644 > >>> --- a/arch/powerpc/kernel/asm-offsets.c > >>> +++ b/arch/powerpc/kernel/asm-offsets.c > >>> @@ -562,6 +562,7 @@ int main(void) > >>> DEFINE(VCPU_LAST_INST, offsetof(struct kvm_vcpu, arch.last_inst)); > >>> DEFINE(VCPU_FAULT_DEAR, offsetof(struct kvm_vcpu, arch.fault_dear)); > >>> DEFINE(VCPU_FAULT_ESR, offsetof(struct kvm_vcpu, arch.fault_esr)); > >>> + DEFINE(VCPU_CRIT_SAVE, offsetof(struct kvm_vcpu, arch.crit_save)); > >>> #endif /* CONFIG_PPC_BOOK3S */ > >>> #endif /* CONFIG_KVM */ > >>> > >>> diff --git a/arch/powerpc/kvm/booke_interrupts.S > >>> b/arch/powerpc/kvm/booke_interrupts.S > >>> index eae8483..dd9c5d4 100644 > >>> --- a/arch/powerpc/kvm/booke_interrupts.S > >>> +++ b/arch/powerpc/kvm/booke_interrupts.S > >>> @@ -52,12 +52,7 @@ > >>> (1<<BOOKE_INTERRUPT_PROGRAM) | \ > >>> (1<<BOOKE_INTERRUPT_DTLB_MISS)) > >>> > >>> -.macro KVM_HANDLER ivor_nr scratch srr0 > >>> -_GLOBAL(kvmppc_handler_\ivor_nr) > >>> - /* Get pointer to vcpu and record exit number. */ > >>> - mtspr \scratch , r4 > >>> - mfspr r4, SPRN_SPRG_THREAD > >>> - lwz r4, THREAD_KVM_VCPU(r4) > >>> +.macro __KVM_HANDLER ivor_nr scratch srr0 > >>> stw r3, VCPU_GPR(R3)(r4) > >>> stw r5, VCPU_GPR(R5)(r4) > >>> stw r6, VCPU_GPR(R6)(r4) > >>> @@ -74,6 +69,46 @@ _GLOBAL(kvmppc_handler_\ivor_nr) > >>> bctr > >>> .endm > >>> > >>> +.macro KVM_HANDLER ivor_nr scratch srr0 > >>> +_GLOBAL(kvmppc_handler_\ivor_nr) > >>> + /* Get pointer to vcpu and record exit number. */ > >>> + mtspr \scratch , r4 > >>> + mfspr r4, SPRN_SPRG_THREAD > >>> + lwz r4, THREAD_KVM_VCPU(r4) > >>> + __KVM_HANDLER \ivor_nr \scratch \srr0 .endm > >>> + > >>> +.macro KVM_DBG_HANDLER ivor_nr scratch srr0 > >>> +_GLOBAL(kvmppc_handler_\ivor_nr) > >>> + mtspr \scratch, r4 > >>> + mfspr r4, SPRN_SPRG_THREAD > >>> + lwz r4, THREAD_KVM_VCPU(r4) > >>> + stw r3, VCPU_CRIT_SAVE(r4) > >>> + mfcr r3 > >>> + mfspr r4, SPRN_CSRR1 > >>> + andi. r4, r4, MSR_PR > >>> + bne 1f > >> > >> > >>> + /* debug interrupt happened in enter/exit path */ > >>> + mfspr r4, SPRN_CSRR1 > >>> + rlwinm r4, r4, 0, ~MSR_DE > >>> + mtspr SPRN_CSRR1, r4 > >>> + lis r4, 0xffff > >>> + ori r4, r4, 0xffff > >>> + mtspr SPRN_DBSR, r4 > >>> + mfspr r4, SPRN_SPRG_THREAD > >>> + lwz r4, THREAD_KVM_VCPU(r4) > >>> + mtcr r3 > >>> + lwz r3, VCPU_CRIT_SAVE(r4) > >>> + mfspr r4, \scratch > >>> + rfci > >> > >> What is this part doing? Try to ignore the debug exit? > > > > As BOOKE doesn't have hardware support for virtualization, hardware never know > current pc is in guest or in host. > > So when enable hardware single step for guest, it cannot be disabled at the > time guest exit. Thus, we'll see that an single step interrupt happens at the > beginning of guest exit path. > > > > With the above code we recognize this kind of single step interrupt disable > single step and rfci. > > > >> Why would we have MSR_DE > >> enabled in the first place when we can't handle it? > > > > When QEMU is using hardware debug resource then we always set MSR_DE during > guest is running. > > Right, but why is MSR_DE enabled during the exit path? If MSR_DE wasn't set, you > wouldn't get a single step exit. We always set MSR_DE in hw MSR when qemu using the debug resource. > During the exit code path, you could then swap > DBSR back to what the host expects (which means no single step). Only after that > enable MSR_DE again. We do not support deferred debug interrupt, so we do save restore dbsr. > > > > >> > >>> +1: /* debug interrupt happened in guest */ > >>> + mtcr r3 > >>> + mfspr r4, SPRN_SPRG_THREAD > >>> + lwz r4, THREAD_KVM_VCPU(r4) > >>> + lwz r3, VCPU_CRIT_SAVE(r4) > >>> + __KVM_HANDLER \ivor_nr \scratch \srr0 > >> > >> I don't think you need the __KVM_HANDLER split. This should be quite > >> easily refactorable into a simple DBG prolog. > > > > Can you please elaborate how you are envisioning this? > > With this patch, you have > > KVM_HANLDER: > > <code> > __KVM_HANDLER > > KVM_DBG_HANDLER: > > <code> > __KVM_HANDLER > > Right? > > In KVM_HANDLER, you get: > > > .macro KVM_HANDLER ivor_nr scratch srr0 > > _GLOBAL(kvmppc_handler_\ivor_nr) > > /* Get pointer to vcpu and record exit number. */ > > mtspr \scratch , r4 > > mfspr r4, SPRN_SPRG_THREAD > > lwz r4, THREAD_KVM_VCPU(r4) > > __KVM_HANDLER \ivor_nr \scratch \srr0 .endm > > > while KVM_DBG_HANDLER is: > > > +.macro KVM_DBG_HANDLER ivor_nr scratch srr0 > > +_GLOBAL(kvmppc_handler_\ivor_nr) > > <debug specific handling> > > +1: /* debug interrupt happened in guest */ > > + mtcr r3 > > + mfspr r4, SPRN_SPRG_THREAD > > + lwz r4, THREAD_KVM_VCPU(r4) > > + lwz r3, VCPU_CRIT_SAVE(r4) > > + __KVM_HANDLER \ivor_nr \scratch \srr0 .endm > > > So if you write this as > > KVM_DBG_HANDLER: > <debug specific handling> > 1: > mtcr r3 > mfspr r4, SPRN_SPRG_THREAD > lwz r4, THREAD_KVM_VCPU(r4) > lwz r3, VCPU_CRIT_SAVE(r4) > lwz r4, \scratch > <KVM_HANDLER> > > then you get code that is slower :) but it should be easier to read, since the > interface between the individual pieces is always the same. Debug shouldn't be a > fast path anyway, right? Frankly speaking I do not see much difference :). If we have to do as you mentioned then I think we can just do KVM_DBG_HANDLER: <debug specific handling> 1: mtcr r3 lwz r3, VCPU_CRIT_SAVE(r4) lwz r4, \scratch <KVM_HANDLER> Thanks -Bharat > > > Alex > -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
