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

PATCH Applied Jes Sorensen 6 participants 9 messages May 20, 2008 – May 22, 2008

lore

Share & tools

lore

Jes Sorensen

author

2008-05-20 09:57 UTC #0 lore raw

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

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 <[email protected]>

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

arch/ia64/kvm/mmio.c +4 -2

Index: 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:

Matthew Chapman

2008-05-20 10:33 UTC #1 lore raw

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

Quoted 38 lines · Jes Sorensen

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 <[email protected]>
---
 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
arch/ia64/kvm/mmio.c +4 -2
@@ -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:

Jes Sorensen

author

2008-05-20 11:13 UTC #2 lore raw

In reply to Jes Sorensen

Quoted 8 lines · 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);

Much nicer indeed!

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

Cheers,
Jes

Signed-off-by Jes Sorensen <[email protected]>

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

arch/ia64/kvm/mmio.c +2 -1

Index: 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:

Zhang, Xiantao

2008-05-21 09:46 UTC #3 lore raw

In reply to Jes Sorensen

Quoted 6 lines · Jes Sorensen

Quoted 5 lines · Matthew Chapman
Jes,
Glad you tracked it down. Can I suggest rather than using memcpy, a more efficient way might be something like...

Quoted 7 lines

Quoted 3 lines
#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

Avi Kivity

Applied

2008-05-21 10:05 UTC #4 lore raw

In reply to Jes Sorensen

Quoted 13 lines · Xiantao

Quoted 11 lines · Jes Sorensen
Quoted 9 lines · 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);

Quoted 8 lines

Quoted 4 lines
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.

Signature · 1 line

error compiling committee.c: too many arguments to function

Isaku Yamahata

2008-05-22 03:45 UTC #5 lore raw

In reply to Jes Sorensen

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?

Signed-off-by Isaku Yamahata <[email protected]>

Cc Jes Sorensen <[email protected]>

arch/ia64/kvm/mmio.c +3 -0

index 351bf70..e6f194a 100644
@@ -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);

Quoted 43 lines · Jes Sorensen

Quoted 8 lines · 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);
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 <[email protected]>
---
 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
(quoted diff) +2 -1
@@ -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:

Signature · 1 line

yamahata

Jes Sorensen

author

2008-05-22 08:08 UTC #6 lore raw

In reply to Jes Sorensen

Quoted 6 lines · 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?

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

Quoted 70 lines

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 <[email protected]>
Cc Jes Sorensen <[email protected]>
arch/ia64/kvm/mmio.c +3 -0
index 351bf70..e6f194a 100644
@@ -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);
 
Quoted 42 lines · Jes Sorensen
Quoted 8 lines · 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);
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 <[email protected]>
---
 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
arch/ia64/kvm/mmio.c +2 -1
@@ -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:

Xu, Anthony

2008-05-22 15:16 UTC #7 lore raw

In reply to Jes Sorensen

Quoted 2 lines

unnecessary divergence is
undesirable.

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

Anthony

Quoted 69 lines · 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 <[email protected]>
Cc Jes Sorensen <[email protected]>
arch/ia64/kvm/mmio.c +3 -0
index 351bf70..e6f194a 100644
@@ -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);
 
Quoted 35 lines · Jes Sorensen
Quoted 8 lines · 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);
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 <[email protected]>
---
 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
(quoted diff) +0 -0
@@ -159,7 +159,8 @@
 

Quoted 5 lines

Review line 163

Quoted 5 lines

(quoted diff) +2 -1

 	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

Quoted 2 lines

Quoted 2 lines
arch/ia64/kvm/mmio.c +0 -0
 		panic_vm(vcpu);
 out:

Jes Sorensen

author

2008-05-22 15:23 UTC #8 lore raw

In reply to Jes Sorensen

Quoted 5 lines · Anthony

Quoted 2 lines
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