From mboxrd@z Thu Jan 1 00:00:00 1970 From: Paul Mackerras Date: Tue, 08 Oct 2013 11:22:55 +0000 Subject: Re: [PATCH 3/3] KVM: PPC: Book3S: MMIO emulation support for little endian guests Message-Id: <20131008112255.GA14050@iris.ozlabs.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> In-Reply-To: <1381156067-32095-4-git-send-email-clg@fr.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable To: =?iso-8859-1?Q?C=E9dric?= Le Goater Cc: agraf@suse.de, kvm-ppc@vger.kernel.org, kvm@vger.kernel.org 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. >=20 > 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. >=20 > Finally, kvmppc_emulate_instruction() uses kvmppc_is_bigendian() > to define in which endian order the mmio needs to be done. >=20 > Signed-off-by: C=E9dric Le Goater [snip] > + ld r0, VCPU_MSR(r9) > + > + /* r10 =3D vcpu->arch.msr & MSR_LE */ > + rldicl. r10, r0, 0, 63 I would have written: andi. r10, r0, MSR_LE which doesn't need the comment, but really the two are equivalent. > @@ -232,6 +231,7 @@ int kvmppc_emulate_instruction(struct kvm_run *run, s= truct 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); 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: 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_bigendi= an) + unsigned int rt, unsigned int bytes, int not_revers= e) { 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 kv= m_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_bigend= ian) + unsigned int rt, unsigned int bytes, int not_rever= se) { 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__, That seems simpler to me -- is there a reason not to do it that way? Paul. From mboxrd@z Thu Jan 1 00:00:00 1970 From: Paul Mackerras Subject: Re: [PATCH 3/3] KVM: PPC: Book3S: MMIO emulation support for little endian guests Date: Tue, 8 Oct 2013 22:22:55 +1100 Message-ID: <20131008112255.GA14050@iris.ozlabs.ibm.com> References: <1381156067-32095-1-git-send-email-clg@fr.ibm.com> <1381156067-32095-4-git-send-email-clg@fr.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: =?iso-8859-1?Q?C=E9dric?= Le Goater Return-path: Content-Disposition: inline In-Reply-To: <1381156067-32095-4-git-send-email-clg@fr.ibm.com> Sender: kvm-ppc-owner@vger.kernel.org List-Id: kvm.vger.kernel.org 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. >=20 > 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. >=20 > Finally, kvmppc_emulate_instruction() uses kvmppc_is_bigendian() > to define in which endian order the mmio needs to be done. >=20 > Signed-off-by: C=E9dric Le Goater [snip] > + ld r0, VCPU_MSR(r9) > + > + /* r10 =3D vcpu->arch.msr & MSR_LE */ > + rldicl. r10, r0, 0, 63 I would have written: andi. r10, r0, MSR_LE which doesn't need the comment, but really the two are equivalent. > @@ -232,6 +231,7 @@ int kvmppc_emulate_instruction(struct kvm_run *ru= n, 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); 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: 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_v= cpu *vcpu, } =20 int kvmppc_handle_load(struct kvm_run *run, struct kvm_vcpu *vcpu, - unsigned int rt, unsigned int bytes, int is_big= endian) + unsigned int rt, unsigned int bytes, int not_re= verse) { 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, struc= t 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_bi= gendian) + unsigned int rt, unsigned int bytes, int not_r= everse) { 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__, That seems simpler to me -- is there a reason not to do it that way? Paul.