Patchwork [3/3] KVM: PPC: Book3S: MMIO emulation support for little endian guests

login
register
mail settings
Submitter Paul Mackerras
Date Oct. 8, 2013, 11:22 a.m.
Message ID <20131008112255.GA14050@iris.ozlabs.ibm.com>
Download mbox | patch
Permalink /patch/281418/
State New
Headers show

Comments

Paul Mackerras - Oct. 8, 2013, 11:22 a.m.
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.

> @@ -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
Cédric Le Goater - Oct. 8, 2013, 11:49 a.m.
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

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__,