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

2008-05-22 Thread 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 [EMAIL PROTECTED]
Cc: Jes Sorensen [EMAIL PROTECTED]

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 [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
@@ -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-devel@lists.xensource.com
http://lists.xensource.com/xen-ia64-devel

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

2008-05-22 Thread 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 [EMAIL PROTECTED]
 Cc: Jes Sorensen [EMAIL PROTECTED]
 
 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 [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
 @@ -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-devel@lists.xensource.com
http://lists.xensource.com/xen-ia64-devel

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

2008-05-22 Thread 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

___
Xen-ia64-devel mailing list
Xen-ia64-devel@lists.xensource.com
http://lists.xensource.com/xen-ia64-devel

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

2008-05-21 Thread 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]

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 [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
 @@ -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-devel@lists.xensource.com
http://lists.xensource.com/xen-ia64-devel