> -----Original Message-----
> From: Linuxppc-dev [mailto:linuxppc-dev-
> bounces+mihai.caraman=freescale....@lists.ozlabs.org] On Behalf Of
> mihai.cara...@freescale.com
> Sent: Friday, July 18, 2014 12:06 PM
> To: Alexander Graf; kvm-...@vger.kernel.org
> Cc: linuxppc-dev@lists.ozlabs.org; k...@vger.kernel.org
> Subject: RE: [PATCH v5 4/5] KVM: PPC: Alow kvmppc_get_last_inst() to fail
> 
> > -----Original Message-----
> > From: Alexander Graf [mailto:ag...@suse.de]
> > Sent: Thursday, July 17, 2014 5:21 PM
> > To: Caraman Mihai Claudiu-B02008; kvm-...@vger.kernel.org
> > Cc: k...@vger.kernel.org; linuxppc-dev@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.cara...@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.

-Mike
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Reply via email to