Re: [PATCH 3/7] KVM: PPC: debug stub interface parameter defined

[Jan Kiszka <jan.kiszka@siemens.com>]

On 2013-03-14 13:09, Alexander Graf wrote:
> 
> On 14.03.2013, at 12:57, Jan Kiszka wrote:
> 
>> On 2013-03-14 12:54, Alexander Graf wrote:
>>>
>>> On 14.03.2013, at 05:42, Bhushan Bharat-R65777 wrote:
>>>
>>>>
>>>>
>>>>> -----Original Message-----
>>>>> From: Alexander Graf [mailto:agraf@suse.de]
>>>>> Sent: Thursday, March 07, 2013 6:51 PM
>>>>> To: Bhushan Bharat-R65777
>>>>> Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; Wood Scott-B07421; Bhushan
>>>>> Bharat-R65777
>>>>> Subject: Re: [PATCH 3/7] KVM: PPC: debug stub interface parameter defined
>>>>>
>>>>>
>>>>> On 28.02.2013, at 05:13, Bharat Bhushan wrote:
>>>>>
>>>>>> This patch defines the interface parameter for KVM_SET_GUEST_DEBUG
>>>>>> ioctl support. Follow up patches will use this for setting up hardware
>>>>>> breakpoints, watchpoints and software breakpoints.
>>>>>>
>>>>>> Also kvm_arch_vcpu_ioctl_set_guest_debug() is brought one level below.
>>>>>> This is because I am not sure what is required for book3s. So this
>>>>>> ioctl behaviour will not change for book3s.
>>>>>>
>>>>>> Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
>>>>>> ---
>>>>>> arch/powerpc/include/uapi/asm/kvm.h |   23 +++++++++++++++++++++++
>>>>>> arch/powerpc/kvm/book3s.c           |    6 ++++++
>>>>>> arch/powerpc/kvm/booke.c            |    6 ++++++
>>>>>> arch/powerpc/kvm/powerpc.c          |    6 ------
>>>>>> 4 files changed, 35 insertions(+), 6 deletions(-)
>>>>>>
>>>>>> diff --git a/arch/powerpc/include/uapi/asm/kvm.h
>>>>>> b/arch/powerpc/include/uapi/asm/kvm.h
>>>>>> index c2ff99c..15f9a00 100644
>>>>>> --- a/arch/powerpc/include/uapi/asm/kvm.h
>>>>>> +++ b/arch/powerpc/include/uapi/asm/kvm.h
>>>>>> @@ -272,8 +272,31 @@ struct kvm_debug_exit_arch {
>>>>>>
>>>>>> /* for KVM_SET_GUEST_DEBUG */
>>>>>> struct kvm_guest_debug_arch {
>>>>>> +	struct {
>>>>>> +		/* H/W breakpoint/watchpoint address */
>>>>>> +		__u64 addr;
>>>>>> +		/*
>>>>>> +		 * Type denotes h/w breakpoint, read watchpoint, write
>>>>>> +		 * watchpoint or watchpoint (both read and write).
>>>>>> +		 */
>>>>>> +#define KVMPPC_DEBUG_NOTYPE		0x0
>>>>>> +#define KVMPPC_DEBUG_BREAKPOINT		(1UL << 1)
>>>>>> +#define KVMPPC_DEBUG_WATCH_WRITE	(1UL << 2)
>>>>>> +#define KVMPPC_DEBUG_WATCH_READ		(1UL << 3)
>>>>>> +		__u32 type;
>>>>>> +		__u32 reserved;
>>>>>> +	} bp[16];
>>>>>> };
>>>>>>
>>>>>> +/* Debug related defines */
>>>>>> +/*
>>>>>> + * kvm_guest_debug->control is a 32 bit field. The lower 16 bits are
>>>>>> +generic
>>>>>> + * and upper 16 bits are architecture specific. Architecture specific
>>>>>> +defines
>>>>>> + * that ioctl is for setting hardware breakpoint or software breakpoint.
>>>>>> + */
>>>>>> +#define KVM_GUESTDBG_USE_SW_BP		0x00010000
>>>>>> +#define KVM_GUESTDBG_USE_HW_BP		0x00020000
>>>>>
>>>>> You only need
>>>>>
>>>>> #define KVM_GUESTDBG_HW_BP 0x00010000
>>>>>
>>>>> In absence of the flag, it's a SW breakpoint.
>>>>
>>>> We kept this for 2 reasons; 1) Same logic is applied for i386, so trying to keep consistent 2) better clarity.
>>>
>>> Jan, was there any special reason to have 2 flags for HW/SW breakpoint on x86 rather than one bit that indicates which one is used?
>>
>> Different mechanics on x86: HW goes via debug registers and shows up as
>> INT1, SW is INT3 (plus guest patching done by user land).
> 
> Well, the same thing goes for us. What I'm asking is whether there is a specific reason (extensibility, oversight, taste, ...) that you did
> 
>     #define KVM_GUESTDBG_USE_SW_BP               0x00010000
>     #define KVM_GUESTDBG_USE_HW_BP               0x00020000
> 
> rather than
> 
>     #define KVM_GUESTDBG_BP_TYPE			0x00010000
>     #define KVM_GUESTDBG_BP_TYPE_SW		0x00010000
>     #define KVM_GUESTDBG_BP_TYPE_HW		0x00000000
> 
> :)

Those bits enable or disable the features separately. You may also leave
both off if you like (and just use single stepping).

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux