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

[Alexander Graf]

Am 23.07.2014 um 10:24 schrieb "mihai.caraman at freescale.com" <mihai.caraman at freescale.com>:

>> -----Original Message-----
>> From: kvm-ppc-owner at vger.kernel.org [mailto:kvm-ppc-
>> owner at vger.kernel.org] On Behalf Of Alexander Graf
>> Sent: Wednesday, July 23, 2014 12:21 AM
>> To: Caraman Mihai Claudiu-B02008
>> Cc: kvm-ppc at vger.kernel.org; linuxppc-dev at lists.ozlabs.org;
>> kvm at vger.kernel.org
>> Subject: Re: [PATCH v5 4/5] KVM: PPC: Alow kvmppc_get_last_inst() to fail
>> 
>> 
>> On 21.07.14 11:59, mihai.caraman at freescale.com wrote:
>>>> -----Original Message-----
>>>> From: Linuxppc-dev [mailto:linuxppc-dev-
>>>> bounces+mihai.caraman=freescale.com at lists.ozlabs.org] On Behalf Of
>>>> mihai.caraman at freescale.com
>>>> Sent: Friday, July 18, 2014 12:06 PM
>>>> To: Alexander Graf; kvm-ppc at vger.kernel.org
>>>> Cc: linuxppc-dev at lists.ozlabs.org; kvm at 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 at suse.de]
>>>>> Sent: Thursday, July 17, 2014 5:21 PM
>>>>> To: Caraman Mihai Claudiu-B02008; kvm-ppc at vger.kernel.org
>>>>> Cc: kvm at vger.kernel.org; linuxppc-dev at 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 at 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.
> 
> Right. With this do you acknowledge that v5 (definitely overwritten approach)
> is ok?

I think I'm starting to understand your logic of v5. You write fetch_failed into *inst unswapped if the fetch failed.

I think that's ok, but I definitely do not like the code flow - it's too hard to understand at a glimpse. Just rewrite it to swab at local variable level, preferably with if()s and comments what this is about and have a single unconditional *inst = fetched_inst; at the end of the function.

Alex