> -----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.
> 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>
Thanks
-Bharat
>
>
> Alex
>
--
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
