| Message ID | 1340627195-11544-7-git-send-email-mihai.caraman@freescale.com |
|---|---|
| State | New, archived |
| Headers | show |
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@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. > 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. Alex -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: kvm-ppc-owner@vger.kernel.org [mailto:kvm-ppc- > owner@vger.kernel.org] On Behalf Of Alexander Graf > Sent: Wednesday, July 04, 2012 4:56 PM > To: Caraman Mihai Claudiu-B02008 > Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; linuxppc- > dev@lists.ozlabs.org; qemu-ppc@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@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 -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05.07.2012, at 13:39, Caraman Mihai Claudiu-B02008 wrote: >> -----Original Message----- >> From: kvm-ppc-owner@vger.kernel.org [mailto:kvm-ppc- >> owner@vger.kernel.org] On Behalf Of Alexander Graf >> Sent: Wednesday, July 04, 2012 4:56 PM >> To: Caraman Mihai Claudiu-B02008 >> Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; linuxppc- >> dev@lists.ozlabs.org; qemu-ppc@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@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? This is generic code to me, so it shouldn't go into booke/book3s specific files. > >> >>> 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. Because usually rt in the PPC ISA denotes a _t_arget _r_egister. The field here really is called "T" to denote the _t_ype of the operation which you correctly pointed out. Could you please change this misnaming along the way and mask it accordingly? Alex -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; +} + 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); break; case XOP_TLBIVAX: - emulated = kvmppc_e500_emul_tlbivax(vcpu, ra, rb); + ea = kvmppc_get_ea_indexed(vcpu, ra, rb); + emulated = kvmppc_e500_emul_tlbivax(vcpu, ea); break; default: diff --git a/arch/powerpc/kvm/e500_tlb.c b/arch/powerpc/kvm/e500_tlb.c index c510fc9..2175a58 100644 --- a/arch/powerpc/kvm/e500_tlb.c +++ b/arch/powerpc/kvm/e500_tlb.c @@ -1,5 +1,5 @@ /* - * Copyright (C) 2008-2011 Freescale Semiconductor, Inc. All rights reserved. + * Copyright (C) 2008-2012 Freescale Semiconductor, Inc. All rights reserved. * * Author: Yu Liu, yu.liu@freescale.com * Scott Wood, scottwood@freescale.com @@ -680,14 +680,11 @@ int kvmppc_e500_emul_mt_mmucsr0(struct kvmppc_vcpu_e500 *vcpu_e500, ulong value) return EMULATE_DONE; } -int kvmppc_e500_emul_tlbivax(struct kvm_vcpu *vcpu, int ra, int rb) +int kvmppc_e500_emul_tlbivax(struct kvm_vcpu *vcpu, gva_t ea) { struct kvmppc_vcpu_e500 *vcpu_e500 = to_e500(vcpu); unsigned int ia; int esel, tlbsel; - gva_t ea; - - ea = ((ra) ? kvmppc_get_gpr(vcpu, ra) : 0) + kvmppc_get_gpr(vcpu, rb); ia = (ea >> 2) & 0x1; @@ -731,14 +728,9 @@ static void tlbilx_all(struct kvmppc_vcpu_e500 *vcpu_e500, int tlbsel, } static void tlbilx_one(struct kvmppc_vcpu_e500 *vcpu_e500, int pid, - int ra, int rb) + gva_t ea) { int tlbsel, esel; - gva_t ea; - - ea = kvmppc_get_gpr(&vcpu_e500->vcpu, rb); - if (ra) - ea += kvmppc_get_gpr(&vcpu_e500->vcpu, ra); for (tlbsel = 0; tlbsel < 2; tlbsel++) { esel = kvmppc_e500_tlb_index(vcpu_e500, ea, tlbsel, pid, -1); @@ -750,7 +742,7 @@ static void tlbilx_one(struct kvmppc_vcpu_e500 *vcpu_e500, int pid, } } -int kvmppc_e500_emul_tlbilx(struct kvm_vcpu *vcpu, int rt, int ra, int rb) +int kvmppc_e500_emul_tlbilx(struct kvm_vcpu *vcpu, int rt, gva_t ea) { struct kvmppc_vcpu_e500 *vcpu_e500 = to_e500(vcpu); int pid = get_cur_spid(vcpu); @@ -759,7 +751,7 @@ int kvmppc_e500_emul_tlbilx(struct kvm_vcpu *vcpu, int rt, int ra, int rb) tlbilx_all(vcpu_e500, 0, pid, rt); tlbilx_all(vcpu_e500, 1, pid, rt); } else if (rt == 3) { - tlbilx_one(vcpu_e500, pid, ra, rb); + tlbilx_one(vcpu_e500, pid, ea); } return EMULATE_DONE; @@ -784,16 +776,13 @@ int kvmppc_e500_emul_tlbre(struct kvm_vcpu *vcpu) return EMULATE_DONE; } -int kvmppc_e500_emul_tlbsx(struct kvm_vcpu *vcpu, int rb) +int kvmppc_e500_emul_tlbsx(struct kvm_vcpu *vcpu, gva_t ea) { struct kvmppc_vcpu_e500 *vcpu_e500 = to_e500(vcpu); int as = !!get_cur_sas(vcpu); unsigned int pid = get_cur_spid(vcpu); int esel, tlbsel; struct kvm_book3e_206_tlb_entry *gtlbe = NULL; - gva_t ea; - - ea = kvmppc_get_gpr(vcpu, rb); for (tlbsel = 0; tlbsel < 2; tlbsel++) { esel = kvmppc_e500_tlb_index(vcpu_e500, ea, tlbsel, pid, as);
Add emulation helper for getting instruction ea and refactor tlb instruction emulation to use it. Signed-off-by: Mihai Caraman <mihai.caraman@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(-)