From mboxrd@z Thu Jan 1 00:00:00 1970 From: Cedric Le Goater Subject: Re: [PATCH 3/3] KVM: PPC: Book3S: MMIO emulation support for little endian guests Date: Tue, 08 Oct 2013 13:49:26 +0200 Message-ID: <5253F146.8030501@fr.ibm.com> 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> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: agraf@suse.de, kvm-ppc@vger.kernel.org, kvm@vger.kernel.org To: Paul Mackerras Return-path: Received: from e06smtp17.uk.ibm.com ([195.75.94.113]:46804 "EHLO e06smtp17.uk.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752606Ab3JHLth (ORCPT ); Tue, 8 Oct 2013 07:49:37 -0400 Received: from /spool/local by e06smtp17.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Tue, 8 Oct 2013 12:49:34 +0100 In-Reply-To: <20131008112255.GA14050@iris.ozlabs.ibm.com> Sender: kvm-owner@vger.kernel.org List-ID: 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 versi= on=20 of this patch.=20 =20 >> @@ -232,6 +231,7 @@ int kvmppc_emulate_instruction(struct kvm_run *r= un, 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= _vcpu *vcpu, > } >=20 > int kvmppc_handle_load(struct kvm_run *run, struct kvm_vcpu *vcpu, > - unsigned int rt, unsigned int bytes, int is_b= igendian) > + unsigned int rt, unsigned int bytes, int not_= reverse) > { > 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, str= uct 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_= bigendian) > + unsigned int rt, unsigned int bytes, int not= _reverse) > { > 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_bigendia= n) > + 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.