On 30.01.2013, at 12:30, Bhushan Bharat-R65777 wrote: > > >> -----Original Message----- >> From: Alexander Graf [mailto:ag...@suse.de] >> Sent: Friday, January 25, 2013 5:13 PM >> To: Bhushan Bharat-R65777 >> Cc: kvm-...@vger.kernel.org; kvm@vger.kernel.org; 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.bhus...@freescale.com> >>> >>> 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....@freescale.com> >>> [bharat.bhus...@freescale.com: Substantial changes] >>> Signed-off-by: Bharat Bhushan <bharat.bhus...@freescale.com> >>> --- >>> 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. 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. > >> >>> +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? Alex -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html