On 01.02.2013, at 06:04, Bhushan Bharat-R65777 wrote: > > >> -----Original Message----- >> From: kvm-ppc-owner@xxxxxxxxxxxxxxx [mailto:kvm-ppc-owner@xxxxxxxxxxxxxxx] On >> Behalf Of Alexander Graf >> Sent: Thursday, January 31, 2013 10:38 PM >> To: Bhushan Bharat-R65777 >> Cc: kvm-ppc@xxxxxxxxxxxxxxx; kvm@xxxxxxxxxxxxxxx >> Subject: Re: [PATCH 3/8] KVM: PPC: booke: Added debug handler >> >> >> On 31.01.2013, at 17:58, Bhushan Bharat-R65777 wrote: >> >>> >>> >>>> -----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. >> >> 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 :). > > I think there will be another problem as the kvmppc_handler_\ivor_nr will not be the starting address which is required as per our ivor/ivpr usages for booke architecture. > > I am thinking of keeping as is :). How about we take a hybrid approach? You write the code as I described above, but call __KVM_HANDLER at the end. The normal KVM_HANDLER would look like: KVM_HANDLER: kvmppc_handler_\ivor_nr: __KVM_HANDLER ... That way the code should still be more understandable :) 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
