[PATCH 4/6 v5] KVM: PPC: exit to user space on "ehpriv" instruction

[Alexander Graf]

On 26.06.2013, at 11:27, Bhushan Bharat-R65777 wrote:

> 
> 
>> -----Original Message-----
>> From: tiejun.chen [mailto:tiejun.chen at windriver.com]
>> Sent: Wednesday, June 26, 2013 2:47 PM
>> To: Bhushan Bharat-R65777
>> Cc: kvm-ppc at vger.kernel.org; kvm at vger.kernel.org; agraf at suse.de; Wood Scott-
>> B07421; benh at kernel.crashing.org; linuxppc-dev at lists.ozlabs.org; linux-
>> kernel at vger.kernel.org; mikey at neuling.org
>> Subject: Re: [PATCH 4/6 v5] KVM: PPC: exit to user space on "ehpriv" instruction
>> 
>> On 06/26/2013 04:44 PM, Bhushan Bharat-R65777 wrote:
>>> 
>>> 
>>>> -----Original Message-----
>>>> From: tiejun.chen [mailto:tiejun.chen at windriver.com]
>>>> Sent: Wednesday, June 26, 2013 12:25 PM
>>>> To: Bhushan Bharat-R65777
>>>> Cc: kvm-ppc at vger.kernel.org; kvm at vger.kernel.org; agraf at suse.de; Wood
>>>> Scott- B07421; benh at kernel.crashing.org;
>>>> linuxppc-dev at lists.ozlabs.org; linux- kernel at vger.kernel.org;
>>>> mikey at neuling.org; Bhushan Bharat-R65777
>>>> Subject: Re: [PATCH 4/6 v5] KVM: PPC: exit to user space on "ehpriv"
>>>> instruction
>>>> 
>>>> On 06/26/2013 01:42 PM, Bharat Bhushan wrote:
>>>>> "ehpriv" instruction is used for setting software breakpoints by
>>>>> user space. This patch adds support to exit to user space with
>>>>> "run->debug" have relevant information.
>>>>> 
>>>>> As this is the first point we are using run->debug, also defined the
>>>>> run->debug structure.
>>>>> 
>>>>> Signed-off-by: Bharat Bhushan <bharat.bhushan at freescale.com>
>>>>> ---
>>>>>   arch/powerpc/include/asm/disassemble.h |    4 ++++
>>>>>   arch/powerpc/include/uapi/asm/kvm.h    |   21 +++++++++++++++++----
>>>>>   arch/powerpc/kvm/e500_emulate.c        |   27 +++++++++++++++++++++++++++
>>>>>   3 files changed, 48 insertions(+), 4 deletions(-)
>>>>> 
>>>>> diff --git a/arch/powerpc/include/asm/disassemble.h
>>>> b/arch/powerpc/include/asm/disassemble.h
>>>>> index 9b198d1..856f8de 100644
>>>>> --- a/arch/powerpc/include/asm/disassemble.h
>>>>> +++ b/arch/powerpc/include/asm/disassemble.h
>>>>> @@ -77,4 +77,8 @@ static inline unsigned int get_d(u32 inst)
>>>>>   	return inst & 0xffff;
>>>>>   }
>>>>> 
>>>>> +static inline unsigned int get_oc(u32 inst) {
>>>>> +	return (inst >> 11) & 0x7fff;
>>>>> +}
>>>>>   #endif /* __ASM_PPC_DISASSEMBLE_H__ */ diff --git
>>>>> a/arch/powerpc/include/uapi/asm/kvm.h
>>>> b/arch/powerpc/include/uapi/asm/kvm.h
>>>>> index 0fb1a6e..ded0607 100644
>>>>> --- a/arch/powerpc/include/uapi/asm/kvm.h
>>>>> +++ b/arch/powerpc/include/uapi/asm/kvm.h
>>>>> @@ -269,7 +269,24 @@ struct kvm_fpu {
>>>>>   	__u64 fpr[32];
>>>>>   };
>>>>> 
>>>>> +/*
>>>>> + * Defines for h/w breakpoint, watchpoint (read, write or both) and
>>>>> + * software breakpoint.
>>>>> + * These are used as "type" in KVM_SET_GUEST_DEBUG ioctl and "status"
>>>>> + * for KVM_DEBUG_EXIT.
>>>>> + */
>>>>> +#define KVMPPC_DEBUG_NONE		0x0
>>>>> +#define KVMPPC_DEBUG_BREAKPOINT		(1UL << 1)
>>>>> +#define KVMPPC_DEBUG_WATCH_WRITE	(1UL << 2)
>>>>> +#define KVMPPC_DEBUG_WATCH_READ		(1UL << 3)
>>>>>   struct kvm_debug_exit_arch {
>>>>> +	__u64 address;
>>>>> +	/*
>>>>> +	 * exiting to userspace because of h/w breakpoint, watchpoint
>>>>> +	 * (read, write or both) and software breakpoint.
>>>>> +	 */
>>>>> +	__u32 status;
>>>>> +	__u32 reserved;
>>>>>   };
>>>>> 
>>>>>   /* for KVM_SET_GUEST_DEBUG */
>>>>> @@ -281,10 +298,6 @@ struct kvm_guest_debug_arch {
>>>>>   		 * Type denotes h/w breakpoint, read watchpoint, write
>>>>>   		 * watchpoint or watchpoint (both read and write).
>>>>>   		 */
>>>>> -#define KVMPPC_DEBUG_NONE		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];
>>>>> diff --git a/arch/powerpc/kvm/e500_emulate.c
>>>>> b/arch/powerpc/kvm/e500_emulate.c index b10a012..dab9d07 100644
>>>>> --- a/arch/powerpc/kvm/e500_emulate.c
>>>>> +++ b/arch/powerpc/kvm/e500_emulate.c
>>>>> @@ -26,6 +26,8 @@
>>>>>   #define XOP_TLBRE   946
>>>>>   #define XOP_TLBWE   978
>>>>>   #define XOP_TLBILX  18
>>>>> +#define XOP_EHPRIV  270
>>>>> +#define EHPRIV_OC_DEBUG 0
>>>> 
>>>> As I think the case, "OC = 0", is a bit specific since IIRC, if the
>>>> OC operand is omitted, its equal 0 by default. So I think we should
>>>> start this OC value from 1 or other magic number.
>>> 
>>> ehpriv instruction is defined to be used as:
>>> 	ehpriv OC // where OC can be 0,1, ... n and in extended for it can be
>>> used as
>>> 	ehpriv // With no OC, and here it assumes OC = 0 So OC = 0 is not
>>> specific but "ehpriv" is same as "ehpriv 0".
>> 
>> Yes, this is just what I mean.
>> 
>>> 
>>> I do not think of any special reason to reserve "ehpriv" and "ehpriv 0".
>> 
>> So I still prefer we can reserve the 'ehpriv' without OC operand as one simple
>> approach to test or develop something for KVM quickly because its really
>> convenient to trap into the hypervisor only with one 'ehpriv' instruction
>> easily.
>> 
>> But I have no further objection if you guys are fine to this ;-)
> 
> I can see the using "ehpriv" can be a default choice. But all ehvpriv trap is handled at one place (in a single function) so the accidently overlap with debug should not be an issue.
> 
> I too do not have any strong opinion to keep either way, so want to leave as is ;-).

Seconded. On x86 we also just use int3 for soft breakpoints IIRC.


Alex