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

["tiejun.chen" <tiejun.chen@windriver.com>]

On 06/26/2013 04:44 PM, Bhushan Bharat-R65777 wrote:
>
>
>> -----Original Message-----
>> From: tiejun.chen [mailto:tiejun.chen@windriver.com]
>> Sent: Wednesday, June 26, 2013 12:25 PM
>> To: Bhushan Bharat-R65777
>> Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; agraf@suse.de; Wood Scott-
>> B07421; benh@kernel.crashing.org; linuxppc-dev@lists.ozlabs.org; linux-
>> kernel@vger.kernel.org; mikey@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@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 ;-)

Tiejun

>
> Thanks
> -Bharat
>
>>
>> And if possible, we'd better add some comments to describe this to make the OC
>> definition readable.
>>
>> Tiejun
>>
>>>
>>>    #ifdef CONFIG_KVM_E500MC
>>>    static int dbell2prio(ulong param)
>>> @@ -82,6 +84,26 @@ static int kvmppc_e500_emul_msgsnd(struct kvm_vcpu *vcpu,
>> int rb)
>>>    }
>>>    #endif
>>>
>>> +static int kvmppc_e500_emul_ehpriv(struct kvm_run *run, struct kvm_vcpu
>> *vcpu,
>>> +				   unsigned int inst, int *advance)
>>> +{
>>> +	int emulated = EMULATE_DONE;
>>> +
>>> +	switch (get_oc(inst)) {
>>> +	case EHPRIV_OC_DEBUG:
>>> +		run->exit_reason = KVM_EXIT_DEBUG;
>>> +		run->debug.arch.address = vcpu->arch.pc;
>>> +		run->debug.arch.status = 0;
>>> +		kvmppc_account_exit(vcpu, DEBUG_EXITS);
>>> +		emulated = EMULATE_EXIT_USER;
>>> +		*advance = 0;
>>> +		break;
>>> +	default:
>>> +		emulated = EMULATE_FAIL;
>>> +	}
>>> +	return emulated;
>>> +}
>>> +
>>>    int kvmppc_core_emulate_op(struct kvm_run *run, struct kvm_vcpu *vcpu,
>>>                               unsigned int inst, int *advance)
>>>    {
>>> @@ -130,6 +152,11 @@ int kvmppc_core_emulate_op(struct kvm_run *run, struct
>> kvm_vcpu *vcpu,
>>>    			emulated = kvmppc_e500_emul_tlbivax(vcpu, ea);
>>>    			break;
>>>
>>> +		case XOP_EHPRIV:
>>> +			emulated = kvmppc_e500_emul_ehpriv(run, vcpu, inst,
>>> +							   advance);
>>> +			break;
>>> +
>>>    		default:
>>>    			emulated = EMULATE_FAIL;
>>>    		}
>>>
>>
>