> -----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? -Mike ��.n��������+%������w��{.n�����o�^n�r������&��z�ޗ�zf���h���~����������_��+v���)ߣ�
