Re: [PATCH v5 4/5] KVM: PPC: Alow kvmppc_get_last_inst() to fail

[Alexander Graf <agraf@suse.de>]

On 21.07.14 11:59, mihai.caraman@freescale.com wrote:
>> -----Original Message-----
>> From: Linuxppc-dev [mailto:linuxppc-dev-
>> bounces+mihai.caraman=freescale.com@lists.ozlabs.org] On Behalf Of
>> mihai.caraman@freescale.com
>> Sent: Friday, July 18, 2014 12:06 PM
>> To: Alexander Graf; kvm-ppc@vger.kernel.org
>> Cc: linuxppc-dev@lists.ozlabs.org; kvm@vger.kernel.org
>> Subject: RE: [PATCH v5 4/5] KVM: PPC: Alow kvmppc_get_last_inst() to fail
>>
>>> -----Original Message-----
>>> From: Alexander Graf [mailto:agraf@suse.de]
>>> Sent: Thursday, July 17, 2014 5:21 PM
>>> To: Caraman Mihai Claudiu-B02008; kvm-ppc@vger.kernel.org
>>> Cc: kvm@vger.kernel.org; linuxppc-dev@lists.ozlabs.org
>>> Subject: Re: [PATCH v5 4/5] KVM: PPC: Alow kvmppc_get_last_inst() to
>> fail
>>>
>>> On 17.07.14 13:22, Mihai Caraman wrote:
>>>> On book3e, guest last instruction is read on the exit path using load
>>>> external pid (lwepx) dedicated instruction. This load operation may
>>> fail
>>>> due to TLB eviction and execute-but-not-read entries.
>>>>
>>>> This patch lay down the path for an alternative solution to read the
>>> guest
>>>> last instruction, by allowing kvmppc_get_lat_inst() function to fail.
>>>> Architecture specific implmentations of kvmppc_load_last_inst() may
>>> read
>>>> last guest instruction and instruct the emulation layer to re-execute
>>> the
>>>> guest in case of failure.
>>>>
>>>> Make kvmppc_get_last_inst() definition common between architectures.
>>>>
>>>> Signed-off-by: Mihai Caraman <mihai.caraman@freescale.com>
>>>> ---
>> ...
>>
>>>> diff --git a/arch/powerpc/include/asm/kvm_ppc.h
>>> b/arch/powerpc/include/asm/kvm_ppc.h
>>>> index e2fd5a1..7f9c634 100644
>>>> --- a/arch/powerpc/include/asm/kvm_ppc.h
>>>> +++ b/arch/powerpc/include/asm/kvm_ppc.h
>>>> @@ -47,6 +47,11 @@ enum emulation_result {
>>>>    	EMULATE_EXIT_USER,    /* emulation requires exit to user-
>> space */
>>>>    };
>>>>
>>>> +enum instruction_type {
>>>> +	INST_GENERIC,
>>>> +	INST_SC,		/* system call */
>>>> +};
>>>> +
>>>>    extern int kvmppc_vcpu_run(struct kvm_run *kvm_run, struct kvm_vcpu
>>> *vcpu);
>>>>    extern int __kvmppc_vcpu_run(struct kvm_run *kvm_run, struct
>> kvm_vcpu
>>> *vcpu);
>>>>    extern void kvmppc_handler_highmem(void);
>>>> @@ -62,6 +67,9 @@ extern int kvmppc_handle_store(struct kvm_run *run,
>>> struct kvm_vcpu *vcpu,
>>>>    			       u64 val, unsigned int bytes,
>>>>    			       int is_default_endian);
>>>>
>>>> +extern int kvmppc_load_last_inst(struct kvm_vcpu *vcpu,
>>>> +				 enum instruction_type type, u32 *inst);
>>>> +
>>>>    extern int kvmppc_emulate_instruction(struct kvm_run *run,
>>>>                                          struct kvm_vcpu *vcpu);
>>>>    extern int kvmppc_emulate_mmio(struct kvm_run *run, struct kvm_vcpu
>>> *vcpu);
>>>> @@ -234,6 +242,23 @@ struct kvmppc_ops {
>>>>    extern struct kvmppc_ops *kvmppc_hv_ops;
>>>>    extern struct kvmppc_ops *kvmppc_pr_ops;
>>>>
>>>> +static inline int kvmppc_get_last_inst(struct kvm_vcpu *vcpu,
>>>> +					enum instruction_type type, u32 *inst)
>>>> +{
>>>> +	int ret = EMULATE_DONE;
>>>> +
>>>> +	/* Load the instruction manually if it failed to do so in the
>>>> +	 * exit path */
>>>> +	if (vcpu->arch.last_inst = KVM_INST_FETCH_FAILED)
>>>> +		ret = kvmppc_load_last_inst(vcpu, type, &vcpu-
>>>> arch.last_inst);
>>>> +
>>>> +
>>>> +	*inst = (ret = EMULATE_DONE && kvmppc_need_byteswap(vcpu)) ?
>>>> +		swab32(vcpu->arch.last_inst) : vcpu->arch.last_inst;
>>> This makes even less sense than the previous version. Either you treat
>>> inst as "definitely overwritten" or as "preserves previous data on
>>> failure".
>> Both v4 and v5 versions treat inst as "definitely overwritten".
>>
>>> So either you unconditionally swap like you did before
>> If we make abstraction of its symmetry, KVM_INST_FETCH_FAILED is operated
>> in host endianness, so it doesn't need byte swap.
>>
>> I agree with your reasoning if last_inst is initialized and compared with
>> data in guest endianess, which is not the case yet for
>> KVM_INST_FETCH_FAILED.
> Alex, are you relying on the fact that KVM_INST_FETCH_FAILED value is symmetrical?
> With a non symmetrical value like 0xDEADBEEF, and considering a little-endian guest
> on a big-endian host, we need to fix kvm logic to initialize and compare last_inst
> with 0xEFBEADDE swaped value.
>
> Your suggestion to unconditionally swap makes sense only with the above fix, otherwise
> inst may end up with 0xEFBEADDE swaped value with is wrong.

Only for *inst which we would treat as "undefined" after the function 
returned EMULATE_AGAIN. last_inst stays  unmodified.


Alex