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

[Scott Wood <scottwood@freescale.com>]

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@vger.kernel.org; kvm@vger.kernel.org; agraf@suse.de; 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@freescale.com>
>>> Signed-off-by: Varun Sethi <Varun.Sethi@freescale.com>
>>> [bharat.bhushan@freescale.com: Substantial changes]
>>> Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
>>> ---
>>>  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.

-Scott