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@xxxxxxxxxx>
[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:
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?
Paul.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
