Re: [PATCH] KVM: x86 emulator: fix group 8 instruction decoding

2010-08-05 Thread Avi Kivity 
 On 08/04/2010 11:27 AM, Wei Yongjun wrote:
 Group 8 instruction, BT[S|R|C] should be mask as BitOp.


Please repost as a patch set, all those versions confuse me.

For maximum reviewability, please do:

- a patch that consolidates existing register BitOp decoding
- a patch that adds immediate BitOp decoding
- a patch that adds BitOp to 0f ba

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

--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] KVM: x86 emulator: fix group 8 instruction decoding

2010-08-04 Thread Wei Yongjun 
Group 8 instruction, BT[S|R|C] should be mask as BitOp.

Signed-off-by: Wei Yongjun yj...@cn.fujitsu.com
---
 arch/x86/kvm/emulate.c |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index d197b46..eba5a67 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -2261,7 +2261,7 @@ static struct opcode twobyte_table[256] = {
D(DstReg | SrcMem16 | ModRM | Mov),
/* 0xB8 - 0xBF */
N, N,
-   G(0, group8), D(DstMem | SrcReg | ModRM | BitOp | Lock),
+   G(BitOp, group8), D(DstMem | SrcReg | ModRM | BitOp | Lock),
N, N, D(ByteOp | DstReg | SrcMem | ModRM | Mov),
D(DstReg | SrcMem16 | ModRM | Mov),
/* 0xC0 - 0xCF */
-- 
1.7.0.4


--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] KVM: x86 emulator: fix group 8 instruction decoding

2010-08-04 Thread Paolo Bonzini 
On 08/04/2010 10:27 AM, Wei Yongjun wrote:
 Group 8 instruction, BT[S|R|C] should be mask as BitOp.
 
 Signed-off-by: Wei Yongjunyj...@cn.fujitsu.com
 ---
   arch/x86/kvm/emulate.c |2 +-
   1 files changed, 1 insertions(+), 1 deletions(-)
 
 diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
 index d197b46..eba5a67 100644
 --- a/arch/x86/kvm/emulate.c
 +++ b/arch/x86/kvm/emulate.c
 @@ -2261,7 +2261,7 @@ static struct opcode twobyte_table[256] = {
   D(DstReg | SrcMem16 | ModRM | Mov),
   /* 0xB8 - 0xBF */
   N, N,
 - G(0, group8), D(DstMem | SrcReg | ModRM | BitOp | Lock),
 + G(BitOp, group8), D(DstMem | SrcReg | ModRM | BitOp | Lock),
   N, N, D(ByteOp | DstReg | SrcMem | ModRM | Mov),
   D(DstReg | SrcMem16 | ModRM | Mov),
   /* 0xC0 - 0xCF */

This is correct with your 4/4 patch, but incorrect before it.  It will
incorrectly cause the emulator to adjust the address of the source
operand.  This is documented to happen with a register source but not
with an immediate source.  You can test it with this:

#include stdio.h
int main()
{
  int a[3] = {0, 0, 0};
  asm (btcl $32, %0 :: m (a[0]) : memory);
  asm (btcl $1, %0 :: m (a[1]) : memory);
  asm (btcl %1, %0 :: m (a[0]), r (66) : memory);
  printf (%x %x %x\n, a[0], a[1], a[2]);
}

It prints 1 2 4.  I'm quite confident that instead it would print 0 3
4 with this patch, and 5 2 0 with this patch + 4/4.

Paolo
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html