On 31.01.2013, at 17:58, Bhushan Bharat-R65777 wrote: > > >> -----Original Message----- >> From: Alexander Graf [mailto:ag...@suse.de] >> Sent: Thursday, January 31, 2013 5:47 PM >> To: Bhushan Bharat-R65777 >> Cc: kvm-...@vger.kernel.org; kvm@vger.kernel.org >> 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: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. > > We always set MSR_DE in hw MSR when qemu using the debug resource.
In the _guest_ MSR, yes. But once we exit the guest, it shouldn't be set anymore, because we're in an interrupt handler, no? Or is MSR_DE kept alive on interrupts? > >> 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> Whatever it takes to keep the oddball (debug) an oddball and keep the normal case easy :). 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
