[RFC PATCH 06/17] KVM: PPC: e500: Add emulation helper for getting instruction ea

[Caraman Mihai Claudiu-B02008]

> -----Original Message-----
> From: kvm-ppc-owner at vger.kernel.org [mailto:kvm-ppc-
> owner at vger.kernel.org] On Behalf Of Alexander Graf
> Sent: Wednesday, July 04, 2012 4:56 PM
> To: Caraman Mihai Claudiu-B02008
> Cc: kvm-ppc at vger.kernel.org; kvm at vger.kernel.org; linuxppc-
> dev at lists.ozlabs.org; qemu-ppc at nongnu.org
> Subject: Re: [RFC PATCH 06/17] KVM: PPC: e500: Add emulation helper for
> getting instruction ea
> 
> 
> On 25.06.2012, at 14:26, Mihai Caraman wrote:
> 
> > Add emulation helper for getting instruction ea and refactor tlb
> instruction
> > emulation to use it.
> >
> > Signed-off-by: Mihai Caraman <mihai.caraman at freescale.com>
> > ---
> > arch/powerpc/kvm/e500.h         |    6 +++---
> > arch/powerpc/kvm/e500_emulate.c |   21 ++++++++++++++++++---
> > arch/powerpc/kvm/e500_tlb.c     |   23 ++++++-----------------
> > 3 files changed, 27 insertions(+), 23 deletions(-)
> >
> > diff --git a/arch/powerpc/kvm/e500.h b/arch/powerpc/kvm/e500.h
> > index 3e31098..70bfed4 100644
> > --- a/arch/powerpc/kvm/e500.h
> > +++ b/arch/powerpc/kvm/e500.h
> > @@ -130,9 +130,9 @@ int kvmppc_e500_emul_mt_mmucsr0(struct
> kvmppc_vcpu_e500 *vcpu_e500,
> > 				ulong value);
> > int kvmppc_e500_emul_tlbwe(struct kvm_vcpu *vcpu);
> > int kvmppc_e500_emul_tlbre(struct kvm_vcpu *vcpu);
> > -int kvmppc_e500_emul_tlbivax(struct kvm_vcpu *vcpu, int ra, int rb);
> > -int kvmppc_e500_emul_tlbilx(struct kvm_vcpu *vcpu, int rt, int ra, int
> rb);
> > -int kvmppc_e500_emul_tlbsx(struct kvm_vcpu *vcpu, int rb);
> > +int kvmppc_e500_emul_tlbivax(struct kvm_vcpu *vcpu, gva_t ea);
> > +int kvmppc_e500_emul_tlbilx(struct kvm_vcpu *vcpu, int rt, gva_t ea);
> > +int kvmppc_e500_emul_tlbsx(struct kvm_vcpu *vcpu, gva_t ea);
> > int kvmppc_e500_tlb_init(struct kvmppc_vcpu_e500 *vcpu_e500);
> > void kvmppc_e500_tlb_uninit(struct kvmppc_vcpu_e500 *vcpu_e500);
> >
> > diff --git a/arch/powerpc/kvm/e500_emulate.c
> b/arch/powerpc/kvm/e500_emulate.c
> > index 8b99e07..81288f7 100644
> > --- a/arch/powerpc/kvm/e500_emulate.c
> > +++ b/arch/powerpc/kvm/e500_emulate.c
> > @@ -82,6 +82,17 @@ static int kvmppc_e500_emul_msgsnd(struct kvm_vcpu
> *vcpu, int rb)
> > }
> > #endif
> >
> > +static inline ulong kvmppc_get_ea_indexed(struct kvm_vcpu *vcpu, int
> ra, int rb)
> > +{
> > +	ulong ea;
> > +
> > +	ea = kvmppc_get_gpr(vcpu, rb);
> > +	if (ra)
> > +		ea += kvmppc_get_gpr(vcpu, ra);
> > +
> > +	return ea;
> > +}
> > +
> 
> Please move this one to arch/powerpc/include/asm/kvm_ppc.h.

Yep. This is similar with what I had in my internal version before emulation
refactoring took place upstream. The only difference is that I split the embedded
and server implementation touching this files:
	arch/powerpc/include/asm/kvm_booke.h
	arch/powerpc/include/asm/kvm_book3s.h

Which approach do you prefer?

> 
> > int kvmppc_core_emulate_op(struct kvm_run *run, struct kvm_vcpu *vcpu,
> >                            unsigned int inst, int *advance)
> > {
> > @@ -89,6 +100,7 @@ int kvmppc_core_emulate_op(struct kvm_run *run,
> struct kvm_vcpu *vcpu,
> > 	int ra = get_ra(inst);
> > 	int rb = get_rb(inst);
> > 	int rt = get_rt(inst);
> > +	gva_t ea;
> >
> > 	switch (get_op(inst)) {
> > 	case 31:
> > @@ -113,15 +125,18 @@ int kvmppc_core_emulate_op(struct kvm_run *run,
> struct kvm_vcpu *vcpu,
> > 			break;
> >
> > 		case XOP_TLBSX:
> > -			emulated = kvmppc_e500_emul_tlbsx(vcpu,rb);
> > +			ea = kvmppc_get_ea_indexed(vcpu, ra, rb);
> > +			emulated = kvmppc_e500_emul_tlbsx(vcpu, ea);
> > 			break;
> >
> > 		case XOP_TLBILX:
> > -			emulated = kvmppc_e500_emul_tlbilx(vcpu, rt, ra, rb);
> > +			ea = kvmppc_get_ea_indexed(vcpu, ra, rb);
> > +			emulated = kvmppc_e500_emul_tlbilx(vcpu, rt, ea);
> 
> What's the point in hiding ra+rb, but not rt? I like the idea of hiding
> the register semantics, but please move rt into a local variable that
> gets passed as pointer to kvmppc_e500_emul_tlbilx.

Why to send it as a pointer? rt which should be rather named t in this case
is an [in] value for tlbilx, according to section 6.11.4.9 in the PowerISA 2.06b.

-Mike