> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Friday, July 27, 2012 7:00 AM
> To: Bhushan Bharat-R65777
> Cc: kvm-ppc@xxxxxxxxxxxxxxx; kvm@xxxxxxxxxxxxxxx; agraf@xxxxxxx; Bhushan Bharat-
> R65777
> Subject: Re: [PATCH 2/2] KVM: PPC: booke/bookehv: Add guest debug support
>
> On 07/26/2012 12:32 AM, Bharat Bhushan wrote:
> > This patch adds:
> > 1) KVM debug handler added for e500v2.
> > 2) Guest debug by qemu gdb stub.
>
> Does it make sense for these to both be in the same patch? If there's common
> code used by both, that could be added first.
ok
>
> > Signed-off-by: Liu Yu <yu.liu@xxxxxxxxxxxxx>
> > Signed-off-by: Varun Sethi <Varun.Sethi@xxxxxxxxxxxxx>
> > [bharat.bhushan@xxxxxxxxxxxxx: Substantial changes]
> > Signed-off-by: Bharat Bhushan <bharat.bhushan@xxxxxxxxxxxxx>
> > ---
> > arch/powerpc/include/asm/kvm.h | 21 +++++
> > arch/powerpc/include/asm/kvm_host.h | 7 ++
> > arch/powerpc/include/asm/kvm_ppc.h | 2 +
> > arch/powerpc/include/asm/reg_booke.h | 1 +
> > arch/powerpc/kernel/asm-offsets.c | 31 ++++++-
> > arch/powerpc/kvm/booke.c | 146 +++++++++++++++++++++++++++---
> > arch/powerpc/kvm/booke_interrupts.S | 160
> ++++++++++++++++++++++++++++++++-
> > arch/powerpc/kvm/bookehv_interrupts.S | 141 ++++++++++++++++++++++++++++-
> > arch/powerpc/kvm/e500mc.c | 3 +-
> > arch/powerpc/kvm/powerpc.c | 2 +-
> > 10 files changed, 492 insertions(+), 22 deletions(-)
> >
> > diff --git a/arch/powerpc/include/asm/kvm.h
> > b/arch/powerpc/include/asm/kvm.h index 3c14202..da71c84 100644
> > --- a/arch/powerpc/include/asm/kvm.h
> > +++ b/arch/powerpc/include/asm/kvm.h
> > @@ -25,6 +25,7 @@
> > /* Select powerpc specific features in <linux/kvm.h> */ #define
> > __KVM_HAVE_SPAPR_TCE #define __KVM_HAVE_PPC_SMT
> > +#define __KVM_HAVE_GUEST_DEBUG
> >
> > struct kvm_regs {
> > __u64 pc;
> > @@ -265,10 +266,19 @@ struct kvm_fpu { };
> >
> > struct kvm_debug_exit_arch {
> > + __u32 exception;
> > + __u32 pc;
> > + __u32 status;
> > };
>
> PC must be 64-bit. What goes in "status" and "exception"?
ok
>
> > /* for KVM_SET_GUEST_DEBUG */
> > struct kvm_guest_debug_arch {
> > + struct {
> > + __u64 addr;
> > + __u32 type;
> > + __u32 pad1;
> > + __u64 pad2;
> > + } bp[16];
> > };
>
> What goes in "type"?
Type denote breakpoint, read watchpoint, write watchpoint or watchpoint (both read and write). Will adding a comment to describe this is ok?
>
> > /* definition of registers in kvm_run */ @@ -285,6 +295,17 @@ struct
> > kvm_sync_regs {
> > #define KVM_CPU_3S_64 4
> > #define KVM_CPU_E500MC 5
> >
> > +/* Debug related defines */
> > +#define KVM_INST_GUESTGDB 0x7C00021C /* ehpriv OC=0 */
>
> Will this work on all PPC?
>
> It certainly won't work on other architectures, so at a minimum it's
> KVM_PPC_INST_GUEST_GDB, but maybe it needs to be determined at runtime.
How to determine at run time? adding another ioctl ?
>
> > +#define KVM_GUESTDBG_USE_SW_BP 0x00010000
> > +#define KVM_GUESTDBG_USE_HW_BP 0x00020000
>
> Where do these get used? Any reason for these particular values? If you're
> trying to create a partition where the upper half is generic and the lower half
> is arch-specific, say so.
KVM_SET_GUEST_DEBUG ioctl used to set/unset debug interrupts, which have a "u32 control" element. We have inherited this mechanism from x86 implementation and it looks like lower 16 bits are generic (like KVM_GUESTDBG_ENBLE, KVM_GUESTDBG_SINGLESTEP etc and upper 16 bits are Architecture specific.
I will add a comment to describe this.
>
> > diff --git a/arch/powerpc/include/asm/kvm_host.h
> > b/arch/powerpc/include/asm/kvm_host.h
> > index 7a45194..524af7a 100644
> > --- a/arch/powerpc/include/asm/kvm_host.h
> > +++ b/arch/powerpc/include/asm/kvm_host.h
> > @@ -458,7 +458,12 @@ struct kvm_vcpu_arch {
> > u32 ccr0;
> > u32 ccr1;
> > u32 dbsr;
> > + /* guest debug regiters*/
> > struct kvmppc_booke_debug_reg dbg_reg;
> > + /* shadow debug registers */
> > + struct kvmppc_booke_debug_reg shadow_dbg_reg;
> > + /* host debug regiters*/
> > + struct kvmppc_booke_debug_reg host_dbg_reg;
>
> s/regiter/register/g
>
> ...and put a space before */
>
> > @@ -492,6 +497,7 @@ struct kvm_vcpu_arch {
> > u32 tlbcfg[4];
> > u32 mmucfg;
> > u32 epr;
> > + u32 crit_save;
> > #endif
>
> What is crit_save?
>
> > gpa_t paddr_accessed;
> > gva_t vaddr_accessed;
> > @@ -533,6 +539,7 @@ struct kvm_vcpu_arch {
> > struct kvm_vcpu_arch_shared *shared;
> > unsigned long magic_page_pa; /* phys addr to map the magic page to */
> > unsigned long magic_page_ea; /* effect. addr to map the magic page
> > to */
> > + struct kvm_guest_debug_arch dbg; /* debug arg between kvm and qemu
> > +*/
>
> Is kvm_guest_debug_arch generic or PPC-specific? If the former, why is it in a
> ppc struct? If the latter, why doesn't it have "ppc" in the name?
>
> Please separate out generic things in one patch, then PPC-wide things, then
> booke things (but keep things bisectable by adding stubs along the way if
> necessary).
ok
>
> > -#ifdef CONFIG_KVM_BOOKE_HV
> > +#if defined(CONFIG_KVM_E500V2) || defined(CONFIG_KVM_E500MC)
> > DEFINE(THREAD_KVM_VCPU, offsetof(struct thread_struct, kvm_vcpu));
> > #endif
>
> Why not all booke?
Yes, will make is all booke.
>
> > diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c index
> > 6fbdcfc..784a6bf 100644
> > --- a/arch/powerpc/kvm/booke.c
> > +++ b/arch/powerpc/kvm/booke.c
> > @@ -130,6 +130,9 @@ void kvmppc_set_msr(struct kvm_vcpu *vcpu, u32
> > new_msr)
> >
> > #ifdef CONFIG_KVM_BOOKE_HV
> > new_msr |= MSR_GS;
> > +
> > + if (vcpu->guest_debug)
> > + new_msr |= MSR_DE;
> > #endif
> >
> > vcpu->arch.shared->msr = new_msr;
> > @@ -684,10 +687,21 @@ out:
> > return ret;
> > }
> >
> > -static int emulation_exit(struct kvm_run *run, struct kvm_vcpu *vcpu)
> > +static int emulation_exit(struct kvm_run *run, struct kvm_vcpu *vcpu,
> > + int exit_nr)
> > {
> > enum emulation_result er;
> >
> > + if (unlikely(vcpu->guest_debug & KVM_GUESTDBG_USE_SW_BP) &&
> > + (vcpu->arch.last_inst == KVM_INST_GUESTGDB)) {
>
> Unnecessary parens.
>
> > + run->exit_reason = KVM_EXIT_DEBUG;
> > + run->debug.arch.pc = vcpu->arch.pc;
> > + run->debug.arch.exception = exit_nr;
> > + run->debug.arch.status = 0;
> > + kvmppc_account_exit(vcpu, DEBUG_EXITS);
> > + return RESUME_HOST;
>
> The interface isn't (clearly labelled as) booke specific, but you return booke-
> specific exception numbers. How's userspace supposed to know what to do with
> them? What do you plan on doing with them in QEMU?
This is booke specific.
>
> > +#define GET_VCPU_POINT(regd) \
> > + mfspr regd, SPRN_SPRG_THREAD; \
> > + lwz regd, THREAD_KVM_VCPU(regd)
>
> "Point" is not an idiomatic abbreviation for pointer. Does this really need to
> be macroized, which prevents optimization? IIRC, the 64-bit patchset gets rid
> of that on bookehv (where it was called GET_VCPU).
ok
>
> > _GLOBAL(kvmppc_resume_host)
> > + /*
> > + * If guest not used debug facility then hw debug registers
> > + * already have proper host values. If guest used debug
> > + * facility then restore host debug registers.
> > + * No Need to save guest debug registers as they are already intact
> > + * in guest/shadow registers.
> > + */
> > + lwz r9, VCPU_SHADOW_DBG(r4)
> > + rlwinm. r8, r9, 0, ~DBCR0_IDM
> > + beq skip_load_host_debug
> > + lwz r3, VCPU_HOST_DBG(r4)
> > + andis. r9, r9, DBCR0_AC_BITS@h
> > + li r9, 0
> > + mtspr SPRN_DBCR0, r9 /* disable all debug event */
> > + beq ..skip_load_hw_bkpts
>
> We don't currently have that weird leading ".." in the bookehv code -- please
> don't introduce it.
>
> > +#ifndef CONFIG_PPC_FSL_BOOK3E
> > + PPC_LD(r7, VCPU_HOST_DBG+KVMPPC_DBG_IAC3, r4)
> > + PPC_LD(r8, VCPU_HOST_DBG+KVMPPC_DBG_IAC4, r4)
> > + mtspr SPRN_IAC3, r7
> > + mtspr SPRN_IAC4, r8
> > +#endif
>
> Can you handle this at runtime with a feature section?
Why you want this to make run time? Removing config_ ?
>
> > diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
> > index 685829a..38b5d02 100644
> > --- a/arch/powerpc/kvm/powerpc.c
> > +++ b/arch/powerpc/kvm/powerpc.c
> > @@ -427,7 +427,7 @@ void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu) int
> > kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
> > struct kvm_guest_debug *dbg)
> > {
> > - return -EINVAL;
> > + return kvmppc_core_set_guest_debug(vcpu, dbg);
> > }
>
> I don't see a stub implementation for non-booke.
Yes, There is nothing non-booke in this patch, will make this under CONFIG_BOOKE.
Thanks
-Bharat
>
> -Scott
��.n��������+%������w��{.n�����o�^n�r������&��z�ޗ�zf���h���~����������_��+v���)ߣ�
