| Message ID | 20131008112255.GA14050@iris.ozlabs.ibm.com |
|---|---|
| State | New, archived |
| Headers | show |
On 10/08/2013 01:22 PM, Paul Mackerras wrote: > 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 <clg@fr.ibm.com> > > [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. Indeed, this is even better ... rldicl is a vestige of an earlier version of this patch. >> @@ -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: > > 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__, > > 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 time we talked about it. This is more elegant. I will resend a -v2. Thanks for the review Paul, C. -- 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__,