On 05.07.2012, at 13:39, Caraman Mihai Claudiu-B02008 wrote: >> -----Original Message----- >> From: kvm-ppc-owner@xxxxxxxxxxxxxxx [mailto:kvm-ppc- >> owner@xxxxxxxxxxxxxxx] On Behalf Of Alexander Graf >> Sent: Wednesday, July 04, 2012 4:56 PM >> To: Caraman Mihai Claudiu-B02008 >> Cc: kvm-ppc@xxxxxxxxxxxxxxx; kvm@xxxxxxxxxxxxxxx; linuxppc- >> dev@xxxxxxxxxxxxxxxx; qemu-ppc@xxxxxxxxxx >> 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@xxxxxxxxxxxxx> >>> --- >>> 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" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
