> >>>>>>>> -----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) << 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) lwz r4, \scratch __KVM_HANDLER \ivor_nr \scratch \srr0 .endm Thanks -Bharat > > So the kvmppc_handler_\ivor_nr is defined and should always be at the start of > exception handling? > > > > So if KVM_DBG_HANDLER need to call KVM_HANDLER then there will be issue of 2 > definition for DBG interrupt. > > > > I am sorry but I did not understood how you want this to define. Can you > please describe ? > > > > Thanks > > -Bharat > > > > > > -- > To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body > of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at > http://vger.kernel.org/majordomo-info.html -- 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
