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

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 <yamahatavalinux.co.jp>
Cc: Jes Sorensen <jessgi.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 <jessgi.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

_______________________________________________
Xen-ia64-devel mailing list
Xen-ia64-devellists.xensource.com
http://list
s.xensource.com/xen-ia64-devel

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 <yamahatavalinux.co.jp>
> Cc: Jes Sorensen <jessgi.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 <jessgi.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:
> 
> 


_______________________________________________
Xen-ia64-devel mailing list
Xen-ia64-devellists.xensource.com
http://list
s.xensource.com/xen-ia64-devel

>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 <yamahatavalinux.co.jp>
> Cc: Jes Sorensen <jessgi.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 <jessgi.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:

_______________________________________________
Xen-ia64-devel mailing list
Xen-ia64-devellists.xensource.com
http://list
s.xensource.com/xen-ia64-devel