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