[PATCH v5 4/5] KVM: PPC: Alow kvmppc_get_last_inst() to fail

mihai.caraman at freescale.com mihai.caraman at freescale.com
Fri Jul 18 19:05:32 EST 2014


> -----Original Message-----
> From: Alexander Graf [mailto:agraf at suse.de]
> Sent: Thursday, July 17, 2014 5:21 PM
> To: Caraman Mihai Claudiu-B02008; kvm-ppc at vger.kernel.org
> Cc: kvm at vger.kernel.org; linuxppc-dev at 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.caraman at 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.

> or you do
> 
> if (ret == EMULATE_DONE)
>      *inst = kvmppc_need_byteswap(vcpu) ? swab32(vcpu->arch.last_inst) :
> vcpu->arch.last_inst;

-Mike



More information about the Linuxppc-dev mailing list