RE: [PATCH 2/2] KVM: PPC: booke/bookehv: Add guest debug support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Tuesday, July 31, 2012 3:31 AM
> To: Bhushan Bharat-R65777
> Cc: Wood Scott-B07421; kvm-ppc@xxxxxxxxxxxxxxx; kvm@xxxxxxxxxxxxxxx;
> agraf@xxxxxxx
> Subject: Re: [PATCH 2/2] KVM: PPC: booke/bookehv: Add guest debug support
> 
> On 07/30/2012 02:37 AM, Bhushan Bharat-R65777 wrote:
> >
> >
> >> -----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?
> 
> Yes, please make sure all of this is well documented.
> 
> >>>  /* 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 ?
> 
> Or extend an existing one.  Is there any other information about debug
> capabilities that you expose -- number of hardware breakpoints supported, etc?
> 
> >>> +#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.
> 
> I don't think the sw/hw distinction belongs here -- it should be per breakpoint.
> 
> >>> +		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.
> 
> Then put booke in the name, but what about it really needs to be booke specific?
> Why does QEMU care about the exception type?
> 
> >>> +#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_ ?
> 
> Currently KVM hardcodes the target hardware in a way that is unacceptable in
> much of the rest of the kernel.  We have a long term goal to stop doing that,
> and we should avoid making it worse by adding random ifdefs for specific CPUs.

I do not see any CPU_FTR_* which I can use directly. Should I define a new FTR, something like:

#define CPU_FTR_DEBUG_E500  LONG_ASM_CONST(0x4000000000000000)

Use this in: CPU_FTRS_E500_2, CPU_FTRS_E500MC, CPU_FTRS_E5500 etc

BEGIN_FTR_SECTION
	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
END_FTR_SECTION_IFCLR(CPU_FTR_DEBUG_E500)

Thanks
-Bharat

> 
> -Scott

��.n��������+%������w��{.n�����o�^n�r������&��z�ޗ�zf���h���~����������_��+v���)ߣ�

[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux