> -----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. > or you do > > if (ret == EMULATE_DONE) > *inst = kvmppc_need_byteswap(vcpu) ? swab32(vcpu->arch.last_inst) : > vcpu->arch.last_inst; -Mike -- 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
