Am 23.07.2014 um 10:24 schrieb "mihai.caraman@xxxxxxxxxxxxx" <mihai.caraman@xxxxxxxxxxxxx>: >> -----Original Message----- >> From: kvm-ppc-owner@xxxxxxxxxxxxxxx [mailto:kvm-ppc- >> owner@xxxxxxxxxxxxxxx] On Behalf Of Alexander Graf >> Sent: Wednesday, July 23, 2014 12:21 AM >> To: Caraman Mihai Claudiu-B02008 >> Cc: kvm-ppc@xxxxxxxxxxxxxxx; linuxppc-dev@xxxxxxxxxxxxxxxx; >> kvm@xxxxxxxxxxxxxxx >> Subject: Re: [PATCH v5 4/5] KVM: PPC: Alow kvmppc_get_last_inst() to fail >> >> >> On 21.07.14 11:59, mihai.caraman@xxxxxxxxxxxxx wrote: >>>> -----Original Message----- >>>> From: Linuxppc-dev [mailto:linuxppc-dev- >>>> bounces+mihai.caraman=freescale.com@xxxxxxxxxxxxxxxx] On Behalf Of >>>> mihai.caraman@xxxxxxxxxxxxx >>>> Sent: Friday, July 18, 2014 12:06 PM >>>> To: Alexander Graf; kvm-ppc@xxxxxxxxxxxxxxx >>>> Cc: linuxppc-dev@xxxxxxxxxxxxxxxx; kvm@xxxxxxxxxxxxxxx >>>> Subject: RE: [PATCH v5 4/5] KVM: PPC: Alow kvmppc_get_last_inst() to >> fail >>>> >>>>> -----Original Message----- >>>>> From: Alexander Graf [mailto:agraf@xxxxxxx] >>>>> Sent: Thursday, July 17, 2014 5:21 PM >>>>> To: Caraman Mihai Claudiu-B02008; kvm-ppc@xxxxxxxxxxxxxxx >>>>> Cc: kvm@xxxxxxxxxxxxxxx; linuxppc-dev@xxxxxxxxxxxxxxxx >>>>> 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@xxxxxxxxxxxxx> >>>>>> --- >>>> ... >>>> >>>>>> 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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
