> -----Original Message----- > From: Alexander Graf [mailto:agraf@xxxxxxx] > Sent: Friday, February 01, 2013 1:36 PM > To: Bhushan Bharat-R65777 > Cc: kvm-ppc@xxxxxxxxxxxxxxx; kvm@xxxxxxxxxxxxxxx > Subject: Re: [PATCH 3/8] KVM: PPC: booke: Added debug handler > > > 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 :) > 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) __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) __KVM_HANDLER \ivor_nr \scratch \srr0 .endm 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" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
