From patchwork Tue Oct 8 11:22:55 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Paul Mackerras X-Patchwork-Id: 281418 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id C1CC02C00C8 for ; Tue, 8 Oct 2013 22:23:41 +1100 (EST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754407Ab3JHLXY (ORCPT ); Tue, 8 Oct 2013 07:23:24 -0400 Received: from ozlabs.org ([203.10.76.45]:35774 "EHLO ozlabs.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753628Ab3JHLXI (ORCPT ); Tue, 8 Oct 2013 07:23:08 -0400 Received: by ozlabs.org (Postfix, from userid 1003) id E49832C00C8; Tue, 8 Oct 2013 22:23:05 +1100 (EST) Date: Tue, 8 Oct 2013 22:22:55 +1100 From: Paul Mackerras To: =?iso-8859-1?Q?C=E9dric?= Le Goater Cc: agraf@suse.de, kvm-ppc@vger.kernel.org, kvm@vger.kernel.org Subject: Re: [PATCH 3/3] KVM: PPC: Book3S: MMIO emulation support for little endian guests 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-Disposition: inline In-Reply-To: <1381156067-32095-4-git-send-email-clg@fr.ibm.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: kvm-ppc-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: kvm-ppc@vger.kernel.org On Mon, Oct 07, 2013 at 04:27:47PM +0200, Cédric 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édric Le Goater [snip] > + ld r0, VCPU_MSR(r9) > + > + /* r10 = 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, struct kvm_vcpu *vcpu) > int sprn = get_sprn(inst); > enum emulation_result emulated = EMULATE_DONE; > int advance = 1; > + int is_bigendian = kvmppc_is_bigendian(vcpu); > > /* 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 = 0; > break; > case OP_31_XOP_LWZX: > - emulated = kvmppc_handle_load(run, vcpu, rt, 4, 1); > + emulated = 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: That seems simpler to me -- is there a reason not to do it that way? Paul. --- 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/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, } int kvmppc_handle_load(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 idx, ret; + int is_bigendian = not_reverse; + + if (!kvmppc_is_bigendian(vcpu)) + is_bigendian = !not_reverse; 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, /* 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; vcpu->arch.mmio_sign_extend = 1; - r = kvmppc_handle_load(run, vcpu, rt, bytes, is_bigendian); + r = kvmppc_handle_load(run, vcpu, rt, bytes, not_reverse); return r; } 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 = run->mmio.data; int idx, ret; + int is_bigendian = not_reverse; + + if (!kvmppc_is_bigendian(vcpu)) + is_bigendian = !not_reverse; if (bytes > sizeof(run->mmio.data)) { printk(KERN_ERR "%s: bad MMIO length: %d\n", __func__,