> -----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 :). Thanks -Bharat > > > Alex > > -- > 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-ppc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
