[patch] fix zero extending for mmio ld1/2/4 emulation in KVM

[patch] fix zero extending for mmio ld1/2/4 emulation in KVM

[Jes Sorensen]

[-- Attachment #1: Type: text/plain, Size: 188 bytes --]

Hi,

This one seems to solve the problem I have been seeing with ld2.acq
not being zero extended under KVM.

I believe this patch is correct - please shoot me if I am wrong.

Cheers,
Jes


[-- Attachment #2: mmio-zero-extend.diff --]
[-- Type: text/plain, Size: 834 bytes --]

Only copy in the data actually requested by the instruction emulation
and zero pad the destination register first. This avoids the problem
where emulated mmio access got garbled data from ld2.acq instructions
in the vga console driver.

Signed-off-by: Jes Sorensen <jes@sgi.com>

---
 arch/ia64/kvm/mmio.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Index: linux-2.6.git/arch/ia64/kvm/mmio.c
===================================================================
--- linux-2.6.git.orig/arch/ia64/kvm/mmio.c
+++ linux-2.6.git/arch/ia64/kvm/mmio.c
@@ -158,8 +158,10 @@
 	vmm_transition(vcpu);
 
 	if (p->u.ioreq.state == STATE_IORESP_READY) {
-		if (dir == IOREQ_READ)
-			*dest = p->u.ioreq.data;
+		if (dir == IOREQ_READ) {
+			*dest = 0;
+			memcpy(dest, &p->u.ioreq.data, s);
+		}
 	} else
 		panic_vm(vcpu);
 out:

Re: [patch] fix zero extending for mmio ld1/2/4 emulation in KVM

[Matthew Chapman]

Jes,

Glad you tracked it down.  Can I suggest rather than using memcpy, a
more efficient way might be something like...

#define ZERO_EXTEND(x,bits) ((x) & (~0UL >> (64-(bits))))

*dest = ZERO_EXTEND(p->u.ioreq.data, 8*s);

Matt


On Tue, May 20, 2008 at 11:57:58AM +0200, Jes Sorensen wrote:
> Hi,
>
> This one seems to solve the problem I have been seeing with ld2.acq
> not being zero extended under KVM.
>
> I believe this patch is correct - please shoot me if I am wrong.
>
> Cheers,
> Jes
>

> Only copy in the data actually requested by the instruction emulation
> and zero pad the destination register first. This avoids the problem
> where emulated mmio access got garbled data from ld2.acq instructions
> in the vga console driver.
> 
> Signed-off-by: Jes Sorensen <jes@sgi.com>
> 
> ---
>  arch/ia64/kvm/mmio.c |    6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> Index: linux-2.6.git/arch/ia64/kvm/mmio.c
> =================================> --- linux-2.6.git.orig/arch/ia64/kvm/mmio.c
> +++ linux-2.6.git/arch/ia64/kvm/mmio.c
> @@ -158,8 +158,10 @@
>  	vmm_transition(vcpu);
>  
>  	if (p->u.ioreq.state = STATE_IORESP_READY) {
> -		if (dir = IOREQ_READ)
> -			*dest = p->u.ioreq.data;
> +		if (dir = IOREQ_READ) {
> +			*dest = 0;
> +			memcpy(dest, &p->u.ioreq.data, s);
> +		}
>  	} else
>  		panic_vm(vcpu);
>  out:

Re: [patch] fix zero extending for mmio ld1/2/4 emulation in KVM

[Jes Sorensen]

[-- Attachment #1: Type: text/plain, Size: 363 bytes --]

Matthew Chapman wrote:
> Jes,
> 
> Glad you tracked it down.  Can I suggest rather than using memcpy, a
> more efficient way might be something like...
> 
> #define ZERO_EXTEND(x,bits) ((x) & (~0UL >> (64-(bits))))
> 
> *dest = ZERO_EXTEND(p->u.ioreq.data, 8*s);

Much nicer indeed!

Here's a pretty version - Tony will you apply this one instead.

Cheers,
Jes



[-- Attachment #2: mmio-zero-extend.diff --]
[-- Type: text/plain, Size: 820 bytes --]

Only copy in the data actually requested by the instruction emulation
and zero pad the destination register first. This avoids the problem
where emulated mmio access got garbled data from ld2.acq instructions
in the vga console driver.

Signed-off-by: Jes Sorensen <jes@sgi.com>

---
 arch/ia64/kvm/mmio.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Index: linux-2.6.git/arch/ia64/kvm/mmio.c
===================================================================
--- linux-2.6.git.orig/arch/ia64/kvm/mmio.c
+++ linux-2.6.git/arch/ia64/kvm/mmio.c
@@ -159,7 +159,8 @@
 
 	if (p->u.ioreq.state == STATE_IORESP_READY) {
 		if (dir == IOREQ_READ)
-			*dest = p->u.ioreq.data;
+			/* it's necessary to ensure zero extending */
+			*dest = p->u.ioreq.data & (~0UL >> (64-(s*8)));
 	} else
 		panic_vm(vcpu);
 out:

RE: [patch] fix zero extending for mmio ld1/2/4 emulation in KVM

[Zhang, Xiantao]

Jes Sorensen wrote:
> Matthew Chapman wrote:
>> Jes,
>> 
>> Glad you tracked it down.  Can I suggest rather than using memcpy, a
>> more efficient way might be something like...
>> 
>> #define ZERO_EXTEND(x,bits) ((x) & (~0UL >> (64-(bits))))
>> 
>> *dest = ZERO_EXTEND(p->u.ioreq.data, 8*s);
> 
> Much nicer indeed!
> 
> Here's a pretty version - Tony will you apply this one instead.
Hi, Jes
    Good catch!!   Since this is a key fix for 2.6.26, Avi or Tony may
help to push the fix into upstream.  :-)
Xiantao

Re: [patch] fix zero extending for mmio ld1/2/4 emulation in KVM

[Avi Kivity]

Zhang, Xiantao wrote:
> Jes Sorensen wrote:
>   
>> Matthew Chapman wrote:
>>     
>>> Jes,
>>>
>>> Glad you tracked it down.  Can I suggest rather than using memcpy, a
>>> more efficient way might be something like...
>>>
>>> #define ZERO_EXTEND(x,bits) ((x) & (~0UL >> (64-(bits))))
>>>
>>> *dest = ZERO_EXTEND(p->u.ioreq.data, 8*s);
>>>       
>> Much nicer indeed!
>>
>> Here's a pretty version - Tony will you apply this one instead.
>>     
> Hi, Jes
>     Good catch!!   Since this is a key fix for 2.6.26, Avi or Tony may
> help to push the fix into upstream.  :-)
>   

Applied, and also queued for 2.6.26.


-- 
error compiling committee.c: too many arguments to function

Re: [patch] fix zero extending for mmio ld1/2/4 emulation in KVM

[Isaku Yamahata]

Hi Jes.

Good catch.
I thought similar fix is necessary for xen/ia64 and checked the code.
It was fixed differently. I think the unnecessary divergence is undesirable.
What do you think the following fix according?


Only copy in the data actually requested by the instruction emulation
and zero pad the destination register first. This avoids the problem
where emulated mmio access got garbled data from ld2.acq instructions
in the vga console driver.

Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
Cc: Jes Sorensen <jes@sgi.com>

diff --git a/arch/ia64/kvm/mmio.c b/arch/ia64/kvm/mmio.c
index 351bf70..e6f194a 100644
--- a/arch/ia64/kvm/mmio.c
+++ b/arch/ia64/kvm/mmio.c
@@ -154,6 +154,9 @@ static void mmio_access(struct kvm_vcpu *vcpu, u64 src_pa, u64 *dest,
 	p->u.ioreq.dir = dir;
 	if (dir = IOREQ_WRITE)
 		p->u.ioreq.data = *dest;
+	else
+		/* it's necessary to ensure zero extending */
+		p->u.ioreq.data = 0;
 	p->u.ioreq.state = STATE_IOREQ_READY;
 	vmm_transition(vcpu);
 


On Tue, May 20, 2008 at 01:13:50PM +0200, Jes Sorensen wrote:
> Matthew Chapman wrote:
> >Jes,
> >
> >Glad you tracked it down.  Can I suggest rather than using memcpy, a
> >more efficient way might be something like...
> >
> >#define ZERO_EXTEND(x,bits) ((x) & (~0UL >> (64-(bits))))
> >
> >*dest = ZERO_EXTEND(p->u.ioreq.data, 8*s);
> 
> Much nicer indeed!
> 
> Here's a pretty version - Tony will you apply this one instead.
> 
> Cheers,
> Jes
> 
> 

> Only copy in the data actually requested by the instruction emulation
> and zero pad the destination register first. This avoids the problem
> where emulated mmio access got garbled data from ld2.acq instructions
> in the vga console driver.
> 
> Signed-off-by: Jes Sorensen <jes@sgi.com>
> 
> ---
>  arch/ia64/kvm/mmio.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> Index: linux-2.6.git/arch/ia64/kvm/mmio.c
> =================================> --- linux-2.6.git.orig/arch/ia64/kvm/mmio.c
> +++ linux-2.6.git/arch/ia64/kvm/mmio.c
> @@ -159,7 +159,8 @@
>  
>  	if (p->u.ioreq.state = STATE_IORESP_READY) {
>  		if (dir = IOREQ_READ)
> -			*dest = p->u.ioreq.data;
> +			/* it's necessary to ensure zero extending */
> +			*dest = p->u.ioreq.data & (~0UL >> (64-(s*8)));
>  	} else
>  		panic_vm(vcpu);
>  out:


-- 
yamahata

Re: [patch] fix zero extending for mmio ld1/2/4 emulation in KVM

[Jes Sorensen]

Isaku Yamahata wrote:
> Hi Jes.
> 
> Good catch.
> I thought similar fix is necessary for xen/ia64 and checked the code.
> It was fixed differently. I think the unnecessary divergence is undesirable.
> What do you think the following fix according?

Hi Isaku,

I tried this fix for KVM, but it didn't work since the data returned is
a full word (64 bit) and it seems to get crippled in the process, so
we cannot use your patch :(

Cheers,
Jes


> 
> Only copy in the data actually requested by the instruction emulation
> and zero pad the destination register first. This avoids the problem
> where emulated mmio access got garbled data from ld2.acq instructions
> in the vga console driver.
> 
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> Cc: Jes Sorensen <jes@sgi.com>
> 
> diff --git a/arch/ia64/kvm/mmio.c b/arch/ia64/kvm/mmio.c
> index 351bf70..e6f194a 100644
> --- a/arch/ia64/kvm/mmio.c
> +++ b/arch/ia64/kvm/mmio.c
> @@ -154,6 +154,9 @@ static void mmio_access(struct kvm_vcpu *vcpu, u64 src_pa, u64 *dest,
>  	p->u.ioreq.dir = dir;
>  	if (dir = IOREQ_WRITE)
>  		p->u.ioreq.data = *dest;
> +	else
> +		/* it's necessary to ensure zero extending */
> +		p->u.ioreq.data = 0;
>  	p->u.ioreq.state = STATE_IOREQ_READY;
>  	vmm_transition(vcpu);
>  
> 
> 
> On Tue, May 20, 2008 at 01:13:50PM +0200, Jes Sorensen wrote:
>> Matthew Chapman wrote:
>>> Jes,
>>>
>>> Glad you tracked it down.  Can I suggest rather than using memcpy, a
>>> more efficient way might be something like...
>>>
>>> #define ZERO_EXTEND(x,bits) ((x) & (~0UL >> (64-(bits))))
>>>
>>> *dest = ZERO_EXTEND(p->u.ioreq.data, 8*s);
>> Much nicer indeed!
>>
>> Here's a pretty version - Tony will you apply this one instead.
>>
>> Cheers,
>> Jes
>>
>>
> 
>> Only copy in the data actually requested by the instruction emulation
>> and zero pad the destination register first. This avoids the problem
>> where emulated mmio access got garbled data from ld2.acq instructions
>> in the vga console driver.
>>
>> Signed-off-by: Jes Sorensen <jes@sgi.com>
>>
>> ---
>>  arch/ia64/kvm/mmio.c |    3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> Index: linux-2.6.git/arch/ia64/kvm/mmio.c
>> =================================>> --- linux-2.6.git.orig/arch/ia64/kvm/mmio.c
>> +++ linux-2.6.git/arch/ia64/kvm/mmio.c
>> @@ -159,7 +159,8 @@
>>  
>>  	if (p->u.ioreq.state = STATE_IORESP_READY) {
>>  		if (dir = IOREQ_READ)
>> -			*dest = p->u.ioreq.data;
>> +			/* it's necessary to ensure zero extending */
>> +			*dest = p->u.ioreq.data & (~0UL >> (64-(s*8)));
>>  	} else
>>  		panic_vm(vcpu);
>>  out:
> 
> 

RE: [patch] fix zero extending for mmio ld1/2/4 emulation in KVM

[Xu, Anthony]

>unnecessary divergence is
> undesirable. 

Good point, we should keep kvm/ia64 and xen/ia64 the same code base as
possible.

Anthony




Isaku Yamahata wrote:
> Hi Jes.
> 
> Good catch.
> I thought similar fix is necessary for xen/ia64 and checked the code.
> It was fixed differently. I think the unnecessary divergence is
> undesirable. 
> What do you think the following fix according?
> 
> 
> Only copy in the data actually requested by the instruction emulation
> and zero pad the destination register first. This avoids the problem
> where emulated mmio access got garbled data from ld2.acq instructions
> in the vga console driver.
> 
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> Cc: Jes Sorensen <jes@sgi.com>
> 
> diff --git a/arch/ia64/kvm/mmio.c b/arch/ia64/kvm/mmio.c
> index 351bf70..e6f194a 100644
> --- a/arch/ia64/kvm/mmio.c
> +++ b/arch/ia64/kvm/mmio.c
> @@ -154,6 +154,9 @@ static void mmio_access(struct kvm_vcpu *vcpu,
>  	u64 src_pa, u64 *dest, p->u.ioreq.dir = dir;
>  	if (dir = IOREQ_WRITE)
>  		p->u.ioreq.data = *dest;
> +	else
> +		/* it's necessary to ensure zero extending */
> +		p->u.ioreq.data = 0;
>  	p->u.ioreq.state = STATE_IOREQ_READY;
>  	vmm_transition(vcpu);
> 
> 
> 
> On Tue, May 20, 2008 at 01:13:50PM +0200, Jes Sorensen wrote:
>> Matthew Chapman wrote:
>>> Jes,
>>> 
>>> Glad you tracked it down.  Can I suggest rather than using memcpy, a
>>> more efficient way might be something like...
>>> 
>>> #define ZERO_EXTEND(x,bits) ((x) & (~0UL >> (64-(bits))))
>>> 
>>> *dest = ZERO_EXTEND(p->u.ioreq.data, 8*s);
>> 
>> Much nicer indeed!
>> 
>> Here's a pretty version - Tony will you apply this one instead.
>> 
>> Cheers,
>> Jes
>> 
>> 
> 
>> Only copy in the data actually requested by the instruction emulation
>> and zero pad the destination register first. This avoids the problem
>> where emulated mmio access got garbled data from ld2.acq
>> instructions in the vga console driver. 
>> 
>> Signed-off-by: Jes Sorensen <jes@sgi.com>
>> 
>> ---
>>  arch/ia64/kvm/mmio.c |    3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>> 
>> Index: linux-2.6.git/arch/ia64/kvm/mmio.c
>> =================================>> --- linux-2.6.git.orig/arch/ia64/kvm/mmio.c
>> +++ linux-2.6.git/arch/ia64/kvm/mmio.c
>> @@ -159,7 +159,8 @@
>> 
>>  	if (p->u.ioreq.state = STATE_IORESP_READY) {
>>  		if (dir = IOREQ_READ)
>> -			*dest = p->u.ioreq.data;
>> +			/* it's necessary to ensure zero extending */
>> +			*dest = p->u.ioreq.data & (~0UL >> (64-(s*8)));
} else
>>  		panic_vm(vcpu);
>>  out:

Re: [patch] fix zero extending for mmio ld1/2/4 emulation in KVM

[Jes Sorensen]

Xu, Anthony wrote:
>> unnecessary divergence is
>> undesirable. 
> 
> Good point, we should keep kvm/ia64 and xen/ia64 the same code base as
> possible.

It would be nice, but the KVM codebase has already been modified a lot,
just the reformatting in order to be acceptable to the Linux kernel. I
don't think it's realistic to keep them identical.

Cheers,
Jes