On 07.02.2013, at 15:48, 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 :) >>>> >>> >>> With my current Patch it is defined as: >>> >>> .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) >> >> Move these into __KVM_HANDLER (aka: keep the code in there the same as >> KVM_HANDLER today) >> >>> __KVM_HANDLER \ivor_nr \scratch \srr0 .endm >>> >>> .macro KVM_DBG_HANDLER ivor_nr scratch srr0 >>> _GLOBAL(kvmppc_handler_\ivor_nr) >>> >>> <<<<<<Debug related 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) >> >> Restore the state here as if a non-debug interrupt occurred. __KVM_HANDLER will >> fetch r4 itself from SPRG_THREAD. >> >> I'm basically advocating to not optimize the debug case at all. Instead, I would >> prefer to have the exception ABI be identical to the fallback case ABI. That way >> we don't have to worry about 4 code paths, but only about 3, keeping the >> complexity of the code low. >> >> >> Alex >> >>> __KVM_HANDLER \ivor_nr \scratch \srr0 .endm >>> > > Do you mean something like this ? > > .macro __KVM_HANDLER > + /* Get pointer to vcpu and record exit number. */ > + mtspr \scratch , r4 > + mfspr r4, SPRN_SPRG_THREAD > + lwz r4, THREAD_KVM_VCPU(r4) This wouldn't be a +, but rather just stay the exact same code as it is, right? :) > << Existing code >> > > > .macro KVM_HANDLER ivor_nr scratch srr0 > _GLOBAL(kvmppc_handler_\ivor_nr) > __KVM_HANDLER \ivor_nr \scratch \srr0 .endm > > > > > .macro KVM_DBG_HANDLER ivor_nr scratch srr0 > _GLOBAL(kvmppc_handler_\ivor_nr) > <<<<<<Debug related 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) You need to swap the above 2 operations. > lwz r4, \scratch s/lwz/mfspr/ > __KVM_HANDLER \ivor_nr \scratch \srr0 .endm Otherwise, pretty much, yeah :) 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
