On 01.02.2013, at 10:07, Bhushan Bharat-R65777 wrote: > > >> -----Original Message----- >> From: Alexander Graf [mailto:ag...@suse.de] >> Sent: Friday, February 01, 2013 1:36 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 01.02.2013, at 06:04, Bhushan Bharat-R65777 wrote: >> >>> >>> >>>> -----Original Message----- >>>> From: kvm-ppc-ow...@vger.kernel.org >>>> [mailto:kvm-ppc-ow...@vger.kernel.org] On Behalf Of Alexander Graf >>>> Sent: Thursday, January 31, 2013 10:38 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 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 :). >>> >>> 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 > > 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
