Skip to contentSkip to conversation

KVM: x86 emulator: fix group 8 instruction decoding

PATCH In review Wei Yongjun 3 participants 9 messages Aug 4, 2010 – Aug 5, 2010

Subsystems (3)
x86 KVM KVM x86
lore
Share & tools
lore

Wei Yongjun

author
#0 lore raw

PATCH 1/1 KVM: x86 emulator: fix group 8 instruction decoding

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

Signed-off-by Wei Yongjun <[email protected]>
---
 arch/x86/kvm/emulate.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
arch/x86/kvm/emulate.c +1 -1
index d197b46..eba5a67 100644
@@ -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 */
Signature · 1 line
1.7.0.4
author
#1 lore raw

PATCH 1/1 KVM: x86 emulator: cleanup for BitOp instruction emulation

Signed-off-by Wei Yongjun <[email protected]>
---
 arch/x86/kvm/emulate.c |   12 ++++--------
 1 files changed, 4 insertions(+), 8 deletions(-)
arch/x86/kvm/emulate.c +4 -8
index eba5a67..c05a5d7 100644
@@ -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 */
Signature · 1 line
1.7.0.4
maintainer
#2 lore raw

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

Quoted 10 lines · Wei Yongjun
Signed-off-by Wei Yongjun<[email protected]>
---
  arch/x86/kvm/emulate.c |   12 ++++--------
  1 files changed, 4 insertions(+), 8 deletions(-)
arch/x86/kvm/emulate.c +0 -0
index eba5a67..c05a5d7 100644

arch/x86/kvm/emulate.c done_prefixes: · line 2551

Quoted 7 lines
arch/x86/kvm/emulate.c +4 -0
@@ -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

author
#3 lore raw

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

Quoted 16 lines · Wei Yongjun
Quoted 14 lines
Signed-off-by Wei Yongjun<[email protected]>
---
  arch/x86/kvm/emulate.c |   12 ++++--------
  1 files changed, 4 insertions(+), 8 deletions(-)
arch/x86/kvm/emulate.c +0 -0
index eba5a67..c05a5d7 100644
@@ -2545,6 +2545,10 @@ done_prefixes:
  		break;
  	}
Quoted 9 lines

arch/x86/kvm/emulate.c done_prefixes: · line 2552

Quoted 5 lines
arch/x86/kvm/emulate.c +4 -0
+	/* 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.

author
#4 lore raw

PATCH v2 1/1 KVM: x86 emulator: cleanup for BitOp instruction emulation

In reply to Paolo Bonzini
Signed-off-by Wei Yongjun <[email protected]>
---
 arch/x86/kvm/emulate.c |   12 ++++--------
 1 files changed, 4 insertions(+), 8 deletions(-)
arch/x86/kvm/emulate.c +4 -8
index eba5a67..74008ed 100644
@@ -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 */
Signature · 1 line
1.7.0.4
maintainer
#5 lore raw

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

Quoted 48 lines · Wei Yongjun
Signed-off-by Wei Yongjun<[email protected]>
---
  arch/x86/kvm/emulate.c |   12 ++++--------
  1 files changed, 4 insertions(+), 8 deletions(-)
arch/x86/kvm/emulate.c +4 -6
index eba5a67..74008ed 100644
@@ -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 */

arch/x86/kvm/emulate.c twobyte_insn: · line 3376

Quoted 8 lines
arch/x86/kvm/emulate.c +0 -2
@@ -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

maintainer
#6 lore raw
In reply to Wei Yongjun
Quoted 12 lines · Wei Yongjun

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

Signed-off-by Wei Yongjun<[email protected]>
---
  arch/x86/kvm/emulate.c |    2 +-
  1 files changed, 1 insertions(+), 1 deletions(-)
arch/x86/kvm/emulate.c +0 -0
index d197b46..eba5a67 100644

arch/x86/kvm/emulate.c static struct opcode twobyte_table[256] = { · line 2267

Quoted 8 lines
arch/x86/kvm/emulate.c +1 -1
@@ -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

author
#7 lore raw

PATCH v3 1/1 KVM: x86 emulator: fix BitOp instruction emulation

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

Signed-off-by Wei Yongjun <[email protected]>
---
 arch/x86/kvm/emulate.c |   45 ++++++++++++++++++++++++++++++---------------
 1 files changed, 30 insertions(+), 15 deletions(-)
arch/x86/kvm/emulate.c +30 -15
index d197b46..8763708 100644
@@ -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 */
Signature · 1 line
1.7.0.4
#8 lore raw
In reply to Wei Yongjun
Quoted 2 lines · Wei Yongjun

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

Signature · 1 line
error compiling committee.c: too many arguments to function