From mboxrd@z Thu Jan 1 00:00:00 1970 From: Cedric Le Goater Date: Tue, 08 Oct 2013 11:49:26 +0000 Subject: Re: [PATCH 3/3] KVM: PPC: Book3S: MMIO emulation support for little endian guests Message-Id: <5253F146.8030501@fr.ibm.com> List-Id: References: <1381156067-32095-1-git-send-email-clg@fr.ibm.com> <1381156067-32095-4-git-send-email-clg@fr.ibm.com> <20131008112255.GA14050@iris.ozlabs.ibm.com> In-Reply-To: <20131008112255.GA14050@iris.ozlabs.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable To: Paul Mackerras Cc: agraf@suse.de, kvm-ppc@vger.kernel.org, kvm@vger.kernel.org On 10/08/2013 01:22 PM, Paul Mackerras wrote: > On Mon, Oct 07, 2013 at 04:27:47PM +0200, C=E9dric Le Goater wrote: >> MMIO emulation reads the last instruction executed by the guest >> and then emulates. If the guest is running in Little Endian mode, >> the instruction needs to be byte-swapped before being emulated. >> >> This patch stores the last instruction in the endian order of the >> host, primarily doing a byte-swap if needed. The common code >> which fetches 'last_inst' uses a helper routine kvmppc_need_byteswap(). >> and the exit paths for the Book3S PV and HR guests use their own >> version in assembly. >> >> Finally, kvmppc_emulate_instruction() uses kvmppc_is_bigendian() >> to define in which endian order the mmio needs to be done. >> >> Signed-off-by: C=E9dric Le Goater >=20 > [snip] >=20 >> + ld r0, VCPU_MSR(r9) >> + >> + /* r10 =3D vcpu->arch.msr & MSR_LE */ >> + rldicl. r10, r0, 0, 63 >=20 > I would have written: >=20 > andi. r10, r0, MSR_LE >=20 > which doesn't need the comment, but really the two are equivalent. Indeed, this is even better ... rldicl is a vestige of an earlier version=20 of this patch.=20 =20 >> @@ -232,6 +231,7 @@ int kvmppc_emulate_instruction(struct kvm_run *run, = struct kvm_vcpu *vcpu) >> int sprn =3D get_sprn(inst); >> enum emulation_result emulated =3D EMULATE_DONE; >> int advance =3D 1; >> + int is_bigendian =3D kvmppc_is_bigendian(vcpu); >> =20 >> /* this default type might be overwritten by subcategories */ >> kvmppc_set_exit_type(vcpu, EMULATED_INST_EXITS); >> @@ -266,47 +266,53 @@ int kvmppc_emulate_instruction(struct kvm_run *run= , struct kvm_vcpu *vcpu) >> advance =3D 0; >> break; >> case OP_31_XOP_LWZX: >> - emulated =3D kvmppc_handle_load(run, vcpu, rt, 4, 1); >> + emulated =3D kvmppc_handle_load(run, vcpu, rt, 4, >> + is_bigendian); >=20 > I see you're still hitting all the call sites of kvmppc_handle_load(), > kvmppc_handle_loads() and kvmppc_handle_store(), rather than putting > the big-endian test inside kvmppc_handle_load() and > kvmppc_handle_store(), as in this untested patch: >=20 > diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c > index f55e14c..171bce6 100644 > --- a/arch/powerpc/kvm/powerpc.c > +++ b/arch/powerpc/kvm/powerpc.c > @@ -625,9 +625,13 @@ static void kvmppc_complete_mmio_load(struct kvm_vcp= u *vcpu, > } >=20 > int kvmppc_handle_load(struct kvm_run *run, struct kvm_vcpu *vcpu, > - unsigned int rt, unsigned int bytes, int is_bigen= dian) > + unsigned int rt, unsigned int bytes, int not_reve= rse) > { > int idx, ret; > + int is_bigendian =3D not_reverse; > + > + if (!kvmppc_is_bigendian(vcpu)) > + is_bigendian =3D !not_reverse; >=20 > if (bytes > sizeof(run->mmio.data)) { > printk(KERN_ERR "%s: bad MMIO length: %d\n", __func__, > @@ -662,21 +666,25 @@ int kvmppc_handle_load(struct kvm_run *run, struct = kvm_vcpu *vcpu, >=20 > /* Same as above, but sign extends */ > int kvmppc_handle_loads(struct kvm_run *run, struct kvm_vcpu *vcpu, > - unsigned int rt, unsigned int bytes, int is_bige= ndian) > + unsigned int rt, unsigned int bytes, int not_rev= erse) > { > int r; >=20 > vcpu->arch.mmio_sign_extend =3D 1; > - r =3D kvmppc_handle_load(run, vcpu, rt, bytes, is_bigendian); > + r =3D kvmppc_handle_load(run, vcpu, rt, bytes, not_reverse); >=20 > return r; > } >=20 > int kvmppc_handle_store(struct kvm_run *run, struct kvm_vcpu *vcpu, > - u64 val, unsigned int bytes, int is_bigendian) > + u64 val, unsigned int bytes, int not_reverse) > { > void *data =3D run->mmio.data; > int idx, ret; > + int is_bigendian =3D not_reverse; > + > + if (!kvmppc_is_bigendian(vcpu)) > + is_bigendian =3D !not_reverse; >=20 > if (bytes > sizeof(run->mmio.data)) { > printk(KERN_ERR "%s: bad MMIO length: %d\n", __func__, >=20 > That seems simpler to me -- is there a reason not to do it that way? No. A part from the fact I did not quite get what you meant the first=20 time we talked about it. This is more elegant. I will resend a -v2. Thanks for the review Paul, C.