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

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

[Wei Yongjun]

Group 8 instruction, BT[S|R|C] should be mask as BitOp.

Signed-off-by: Wei Yongjun <yjwei@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

[PATCH] KVM: x86 emulator: cleanup for BitOp instruction emulation

[Wei Yongjun]

Signed-off-by: Wei Yongjun <yjwei@cn.fujitsu.com>
---
 arch/x86/kvm/emulate.c |   12 ++++--------
 1 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index eba5a67..c05a5d7 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -2545,6 +2545,10 @@ done_prefixes:
 		break;
 	}
 
+	/* Only subword offset for BitOp: bt/bts/btr/btc. */
+	if (c->d & BitOp)
+		c->src.val &= (c->op_bytes << 3) - 1;
+
 	/*
 	 * Decode and fetch the second source operand: register, memory
 	 * or immediate.
@@ -3303,8 +3307,6 @@ twobyte_insn:
 	case 0xa3:
 	      bt:		/* bt */
 		c->dst.type = OP_NONE;
-		/* only subword offset */
-		c->src.val &= (c->dst.bytes << 3) - 1;
 		emulate_2op_SrcV_nobyte("bt", c->src, c->dst, ctxt->eflags);
 		break;
 	case 0xa4: /* shld imm8, r, r/m */
@@ -3321,8 +3323,6 @@ twobyte_insn:
 		break;
 	case 0xab:
 	      bts:		/* bts */
-		/* only subword offset */
-		c->src.val &= (c->dst.bytes << 3) - 1;
 		emulate_2op_SrcV_nobyte("bts", c->src, c->dst, ctxt->eflags);
 		break;
 	case 0xac: /* shrd imm8, r, r/m */
@@ -3350,8 +3350,6 @@ twobyte_insn:
 		break;
 	case 0xb3:
 	      btr:		/* btr */
-		/* only subword offset */
-		c->src.val &= (c->dst.bytes << 3) - 1;
 		emulate_2op_SrcV_nobyte("btr", c->src, c->dst, ctxt->eflags);
 		break;
 	case 0xb6 ... 0xb7:	/* movzx */
@@ -3373,8 +3371,6 @@ twobyte_insn:
 		break;
 	case 0xbb:
 	      btc:		/* btc */
-		/* only subword offset */
-		c->src.val &= (c->dst.bytes << 3) - 1;
 		emulate_2op_SrcV_nobyte("btc", c->src, c->dst, ctxt->eflags);
 		break;
 	case 0xbe ... 0xbf:	/* movsx */
-- 
1.7.0.4

Re: [PATCH] KVM: x86 emulator: cleanup for BitOp instruction emulation

[Paolo Bonzini]

On 08/04/2010 11:01 AM, Wei Yongjun wrote:
> Signed-off-by: Wei Yongjun<yjwei@cn.fujitsu.com>
> ---
>   arch/x86/kvm/emulate.c |   12 ++++--------
>   1 files changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index eba5a67..c05a5d7 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -2545,6 +2545,10 @@ done_prefixes:
>   		break;
>   	}
> 
> +	/* Only subword offset for BitOp: bt/bts/btr/btc. */
> +	if (c->d&  BitOp)
> +		c->src.val&= (c->op_bytes<<  3) - 1;
> +

You are doing this before the destination operand is decoded, which
means you are not adjusting a memory operand anymore if c->src.val >
(c->op_bytes * 8).

Paolo

Re: [PATCH] KVM: x86 emulator: cleanup for BitOp instruction emulation

[Wei Yongjun]

> On 08/04/2010 11:01 AM, Wei Yongjun wrote:
>   
>> Signed-off-by: Wei Yongjun<yjwei@cn.fujitsu.com>
>> ---
>>   arch/x86/kvm/emulate.c |   12 ++++--------
>>   1 files changed, 4 insertions(+), 8 deletions(-)
>>
>> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
>> index eba5a67..c05a5d7 100644
>> --- a/arch/x86/kvm/emulate.c
>> +++ b/arch/x86/kvm/emulate.c
>> @@ -2545,6 +2545,10 @@ done_prefixes:
>>   		break;
>>   	}
>>
>> +	/* Only subword offset for BitOp: bt/bts/btr/btc. */
>> +	if (c->d&  BitOp)
>> +		c->src.val&= (c->op_bytes<<  3) - 1;
>> +
>>     
> You are doing this before the destination operand is decoded, which
> means you are not adjusting a memory operand anymore if c->src.val >
> (c->op_bytes * 8).
>   

Oh, I forgot this, I will fix it, thanks.

[PATCHv2] KVM: x86 emulator: cleanup for BitOp instruction emulation

[Wei Yongjun]

Signed-off-by: Wei Yongjun <yjwei@cn.fujitsu.com>
---
 arch/x86/kvm/emulate.c |   12 ++++--------
 1 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index eba5a67..74008ed 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -2617,6 +2617,10 @@ done_prefixes:
 		return 0;
 	}
 
+	/* Only subword offset for BitOp: bt/bts/btr/btc. */
+	if (c->d & BitOp)
+		c->src.val &= (c->dst.bytes << 3) - 1;
+
 done:
 	return (rc == X86EMUL_UNHANDLEABLE) ? -1 : 0;
 }
@@ -3303,8 +3307,6 @@ twobyte_insn:
 	case 0xa3:
 	      bt:		/* bt */
 		c->dst.type = OP_NONE;
-		/* only subword offset */
-		c->src.val &= (c->dst.bytes << 3) - 1;
 		emulate_2op_SrcV_nobyte("bt", c->src, c->dst, ctxt->eflags);
 		break;
 	case 0xa4: /* shld imm8, r, r/m */
@@ -3321,8 +3323,6 @@ twobyte_insn:
 		break;
 	case 0xab:
 	      bts:		/* bts */
-		/* only subword offset */
-		c->src.val &= (c->dst.bytes << 3) - 1;
 		emulate_2op_SrcV_nobyte("bts", c->src, c->dst, ctxt->eflags);
 		break;
 	case 0xac: /* shrd imm8, r, r/m */
@@ -3350,8 +3350,6 @@ twobyte_insn:
 		break;
 	case 0xb3:
 	      btr:		/* btr */
-		/* only subword offset */
-		c->src.val &= (c->dst.bytes << 3) - 1;
 		emulate_2op_SrcV_nobyte("btr", c->src, c->dst, ctxt->eflags);
 		break;
 	case 0xb6 ... 0xb7:	/* movzx */
@@ -3373,8 +3371,6 @@ twobyte_insn:
 		break;
 	case 0xbb:
 	      btc:		/* btc */
-		/* only subword offset */
-		c->src.val &= (c->dst.bytes << 3) - 1;
 		emulate_2op_SrcV_nobyte("btc", c->src, c->dst, ctxt->eflags);
 		break;
 	case 0xbe ... 0xbf:	/* movsx */
-- 
1.7.0.4

Re: [PATCHv2] KVM: x86 emulator: cleanup for BitOp instruction emulation

[Paolo Bonzini]

On 08/04/2010 11:37 AM, Wei Yongjun wrote:
> Signed-off-by: Wei Yongjun<yjwei@cn.fujitsu.com>
> ---
>   arch/x86/kvm/emulate.c |   12 ++++--------
>   1 files changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index eba5a67..74008ed 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -2617,6 +2617,10 @@ done_prefixes:
>   		return 0;
>   	}
> 
> +	/* Only subword offset for BitOp: bt/bts/btr/btc. */
> +	if (c->d&  BitOp)
> +		c->src.val&= (c->dst.bytes<<  3) - 1;
> +
>   done:
>   	return (rc == X86EMUL_UNHANDLEABLE) ? -1 : 0;
>   }
> @@ -3303,8 +3307,6 @@ twobyte_insn:
>   	case 0xa3:
>   	      bt:		/* bt */
>   		c->dst.type = OP_NONE;
> -		/* only subword offset */
> -		c->src.val&= (c->dst.bytes<<  3) - 1;
>   		emulate_2op_SrcV_nobyte("bt", c->src, c->dst, ctxt->eflags);
>   		break;
>   	case 0xa4: /* shld imm8, r, r/m */
> @@ -3321,8 +3323,6 @@ twobyte_insn:
>   		break;
>   	case 0xab:
>   	      bts:		/* bts */
> -		/* only subword offset */
> -		c->src.val&= (c->dst.bytes<<  3) - 1;
>   		emulate_2op_SrcV_nobyte("bts", c->src, c->dst, ctxt->eflags);
>   		break;
>   	case 0xac: /* shrd imm8, r, r/m */
> @@ -3350,8 +3350,6 @@ twobyte_insn:
>   		break;
>   	case 0xb3:
>   	      btr:		/* btr */
> -		/* only subword offset */
> -		c->src.val&= (c->dst.bytes<<  3) - 1;
>   		emulate_2op_SrcV_nobyte("btr", c->src, c->dst, ctxt->eflags);
>   		break;
>   	case 0xb6 ... 0xb7:	/* movzx */
> @@ -3373,8 +3371,6 @@ twobyte_insn:
>   		break;
>   	case 0xbb:
>   	      btc:		/* btc */
> -		/* only subword offset */
> -		c->src.val&= (c->dst.bytes<<  3) - 1;
>   		emulate_2op_SrcV_nobyte("btc", c->src, c->dst, ctxt->eflags);
>   		break;
>   	case 0xbe ... 0xbf:	/* movsx */

This has to be squashed with "fix group 8 instruction decoding" for
bisectability.  Also, please provide testcases that pass before, fail
with v1 of your patches, and pass with the final version.

Paolo

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

[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 Yongjun<yjwei@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

[PATCHv3] KVM: x86 emulator: fix BitOp instruction emulation

[Wei Yongjun]

If bit offset operands is a negative number, BitOp instruction
will return wrong value. This patch fix it.

Signed-off-by: Wei Yongjun <yjwei@cn.fujitsu.com>
---
 arch/x86/kvm/emulate.c |   45 ++++++++++++++++++++++++++++++---------------
 1 files changed, 30 insertions(+), 15 deletions(-)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index d197b46..8763708 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -724,6 +724,33 @@ done:
 	return rc;
 }
 
+static void fetch_bit_operand(struct decode_cache *c)
+{
+	unsigned long mask, byte_offset;
+
+	if (c->dst.type != OP_MEM || c->src.type != OP_REG) {
+		/* Only subword offset */
+		c->src.val &= (c->dst.bytes << 3) - 1;
+		return;
+	}
+
+	if (c->src.bytes == 2)
+		c->src.val = (s16)c->src.val;
+	else if (c->src.bytes == 4)
+		c->src.val = (s32)c->src.val;
+
+	mask = ~(c->dst.bytes * 8 - 1);
+
+	if ((long)c->src.val > 0) {
+		c->dst.addr.mem = c->dst.addr.mem + (c->src.val & mask) / 8;
+		c->src.val &= (c->dst.bytes << 3) - 1;
+	} else {
+		byte_offset = c->dst.bytes + ((-c->src.val - 1) & mask) / 8;
+		c->dst.addr.mem -= byte_offset;
+		c->src.val += byte_offset << 3;
+	}
+}
+
 static int read_emulated(struct x86_emulate_ctxt *ctxt,
 			 struct x86_emulate_ops *ops,
 			 unsigned long addr, void *dest, unsigned size)
@@ -2261,7 +2288,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 */
@@ -2587,12 +2614,8 @@ done_prefixes:
 			c->dst.bytes = 8;
 		else
 			c->dst.bytes = (c->d & ByteOp) ? 1 : c->op_bytes;
-		if (c->dst.type == OP_MEM && (c->d & BitOp)) {
-			unsigned long mask = ~(c->dst.bytes * 8 - 1);
-
-			c->dst.addr.mem = c->dst.addr.mem +
-						   (c->src.val & mask) / 8;
-		}
+		if (c->d & BitOp)
+			fetch_bit_operand(c);
 		c->dst.orig_val = c->dst.val;
 		break;
 	case DstAcc:
@@ -3303,8 +3326,6 @@ twobyte_insn:
 	case 0xa3:
 	      bt:		/* bt */
 		c->dst.type = OP_NONE;
-		/* only subword offset */
-		c->src.val &= (c->dst.bytes << 3) - 1;
 		emulate_2op_SrcV_nobyte("bt", c->src, c->dst, ctxt->eflags);
 		break;
 	case 0xa4: /* shld imm8, r, r/m */
@@ -3321,8 +3342,6 @@ twobyte_insn:
 		break;
 	case 0xab:
 	      bts:		/* bts */
-		/* only subword offset */
-		c->src.val &= (c->dst.bytes << 3) - 1;
 		emulate_2op_SrcV_nobyte("bts", c->src, c->dst, ctxt->eflags);
 		break;
 	case 0xac: /* shrd imm8, r, r/m */
@@ -3350,8 +3369,6 @@ twobyte_insn:
 		break;
 	case 0xb3:
 	      btr:		/* btr */
-		/* only subword offset */
-		c->src.val &= (c->dst.bytes << 3) - 1;
 		emulate_2op_SrcV_nobyte("btr", c->src, c->dst, ctxt->eflags);
 		break;
 	case 0xb6 ... 0xb7:	/* movzx */
@@ -3373,8 +3390,6 @@ twobyte_insn:
 		break;
 	case 0xbb:
 	      btc:		/* btc */
-		/* only subword offset */
-		c->src.val &= (c->dst.bytes << 3) - 1;
 		emulate_2op_SrcV_nobyte("btc", c->src, c->dst, ctxt->eflags);
 		break;
 	case 0xbe ... 0xbf:	/* movsx */
-- 
1.7.0.4

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

[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