KVM: x86 emulator: fix negative bit offset BitOp instruction emulation

PATCH Superseded Wei Yongjun 3 participants 10 messages Aug 6, 2010 – Aug 10, 2010

Subsystems (3)

x86 KVM KVM x86

lore

Share & tools

lore

Wei Yongjun

author

2010-08-06 07:19 UTC #0 lore raw

PATCH 1/3 KVM: x86 emulator: fix negative bit offset 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 |   32 ++++++++++++++++++++++++++------
 1 files changed, 26 insertions(+), 6 deletions(-)

arch/x86/kvm/emulate.c +26 -6

index 0e360c6..470c7eb 100644
@@ -724,6 +724,30 @@ done:
 	return rc;
 }
 
+static void fetch_bit_operand(struct decode_cache *c)
+{
+	unsigned long mask, byte_offset;
+
+	if (c->dst.type == OP_MEM) {
+		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) {
+			/* negative bit offset */
+			byte_offset = c->dst.bytes +
+				      ((-c->src.val - 1) & mask) / 8;
+			c->dst.addr.mem -= byte_offset;
+		} else {
+			/* positive bit offset */
+			c->dst.addr.mem += (c->src.val & mask) / 8;
+		}
+	}
+}
+
 static int read_emulated(struct x86_emulate_ctxt *ctxt,
 			 struct x86_emulate_ops *ops,
 			 unsigned long addr, void *dest, unsigned size)

@@ -2646,12 +2670,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:

Signature · 1 line

1.7.0.4

Wei Yongjun

author

2010-08-06 07:22 UTC #1 lore raw

PATCH 2/3] KVM: x86 emulator: do not adjust the address for immediate source

adjust the dst address for a register source but not adjust the address for an immediate source.

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 470c7eb..e7e3d2d 100644
@@ -728,7 +728,7 @@ static void fetch_bit_operand(struct decode_cache *c)
 {
 	unsigned long mask, byte_offset;
 
-	if (c->dst.type == OP_MEM) {
+	if (c->dst.type == OP_MEM && c->src.type == OP_REG) {
 		if (c->src.bytes == 2)
 			c->src.val = (s16)c->src.val;
 		else if (c->src.bytes == 4)

Signature · 1 line

1.7.0.4

Wei Yongjun

author

2010-08-06 07:27 UTC #2 lore raw

PATCH v2 2/3 KVM: x86 emulator: do not adjust the address for immediate source

adjust the dst address for a register source but not adjust the address for an immediate source.

Signed-off-by Wei Yongjun <[email protected]>

---
v1 -> v2 just fix the missing left bracket of mail title

---
 arch/x86/kvm/emulate.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

arch/x86/kvm/emulate.c +1 -1

index 470c7eb..e7e3d2d 100644
@@ -728,7 +728,7 @@ static void fetch_bit_operand(struct decode_cache *c)
 {
 	unsigned long mask, byte_offset;
 
-	if (c->dst.type == OP_MEM) {
+	if (c->dst.type == OP_MEM && c->src.type == OP_REG) {
 		if (c->src.bytes == 2)
 			c->src.val = (s16)c->src.val;
 		else if (c->src.bytes == 4)

Signature · 1 line

1.7.0.4

Wei Yongjun

author

2010-08-06 07:23 UTC #3 lore raw

PATCH 3/3 KVM: x86 emulator: mask group 8 instruction as BitOp

In reply to Wei Yongjun

Mask group 8 instruction as BitOp, so we can share the code for adjust the source operand.

Signed-off-by Wei Yongjun <[email protected]>

---
 arch/x86/kvm/emulate.c |   11 ++++-------
 1 files changed, 4 insertions(+), 7 deletions(-)

arch/x86/kvm/emulate.c +4 -7

index e7e3d2d..dc6a74e 100644
@@ -746,6 +746,9 @@ static void fetch_bit_operand(struct decode_cache *c)
 			c->dst.addr.mem += (c->src.val & mask) / 8;
 		}
 	}
+
+	/* only subword offset */
+	c->src.val &= (c->dst.bytes << 3) - 1;
 }
 
 static int read_emulated(struct x86_emulate_ctxt *ctxt,

@@ -2346,7 +2349,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 */

@@ -3420,8 +3423,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 */

@@ -3449,8 +3450,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 */

@@ -3472,8 +3471,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

Paolo Bonzini

maintainer Reviewed-by on v1 1/3

2010-08-06 08:10 UTC #4 lore raw

In reply to Wei Yongjun

Quoted 48 lines · 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<[email protected]>

---
  arch/x86/kvm/emulate.c |   32 ++++++++++++++++++++++++++------
  1 files changed, 26 insertions(+), 6 deletions(-)

arch/x86/kvm/emulate.c +24 -1

index 0e360c6..470c7eb 100644
@@ -724,6 +724,30 @@ done:
  	return rc;
  }
 
+static void fetch_bit_operand(struct decode_cache *c)
+{
+	unsigned long mask, byte_offset;
+
+	if (c->dst.type == OP_MEM) {
+		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) {
+			/* negative bit offset */
+			byte_offset = c->dst.bytes +
+				      ((-c->src.val - 1)&  mask) / 8;
+			c->dst.addr.mem -= byte_offset;
+		} else {
+			/* positive bit offset */
+			c->dst.addr.mem += (c->src.val&  mask) / 8;
+		}
+	}
+}
+
  static int read_emulated(struct x86_emulate_ctxt *ctxt,
  			 struct x86_emulate_ops *ops,
  			 unsigned long addr, void *dest, unsigned size)

@@ -2646,12 +2670,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)) {

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

Quoted 10 lines

arch/x86/kvm/emulate.c +2 -5

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

The whole series looks good now. Thanks!

Reviewed-by Paolo Bonzini <[email protected]>

Paolo

Avi Kivity

2010-08-08 20:28 UTC #5 lore raw

In reply to Wei Yongjun

arch/x86/kvm/emulate.c done: · line 735

Quoted 12 lines · Wei Yongjun

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

arch/x86/kvm/emulate.c +9 -0

+static void fetch_bit_operand(struct decode_cache *c)
+{
+	unsigned long mask, byte_offset;
+
+	if (c->dst.type == OP_MEM) {
+		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;

Better not to update in place, but instead use a local signed variable.

arch/x86/kvm/emulate.c done: · line 748

Quoted 13 lines

arch/x86/kvm/emulate.c +13 -0

+
+		mask = ~(c->dst.bytes * 8 - 1);
+
+		if ((long)c->src.val < 0) {
+			/* negative bit offset */
+			byte_offset = c->dst.bytes +
+				      ((-c->src.val - 1) & mask) / 8;
+			c->dst.addr.mem -= byte_offset;
+		} else {
+			/* positive bit offset */
+			c->dst.addr.mem += (c->src.val & mask) / 8;
+		}
+	}

Is the if () really necessary? If division translates to arithmetic shift right, it might not be needed.

Quoted 2 lines

(quoted diff) +2 -0
+}
+

Signature · 2 lines

I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Wei Yongjun

author

2010-08-09 03:37 UTC #6 lore raw

PATCH v2 1/3 KVM: x86 emulator: fix negative bit offset 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 |   24 ++++++++++++++++++------
 1 files changed, 18 insertions(+), 6 deletions(-)

arch/x86/kvm/emulate.c +18 -6

index 0e360c6..f48890d 100644
@@ -724,6 +724,22 @@ done:
 	return rc;
 }
 
+static void fetch_bit_operand(struct decode_cache *c)
+{
+	long sv, mask;
+
+	if (c->dst.type == OP_MEM) {
+		mask = ~(c->dst.bytes * 8 - 1);
+
+		if (c->src.bytes == 2)
+			sv = (s16)c->src.val & (s16)mask;
+		else if (c->src.bytes == 4)
+			sv = (s32)c->src.val & (s32)mask;
+
+		c->dst.addr.mem += (sv >> 3);
+	}
+}
+
 static int read_emulated(struct x86_emulate_ctxt *ctxt,
 			 struct x86_emulate_ops *ops,
 			 unsigned long addr, void *dest, unsigned size)

@@ -2646,12 +2662,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:

Signature · 1 line

1.7.0.4

Wei Yongjun

author

2010-08-09 03:39 UTC #7 lore raw

PATCH v2 2/3 KVM: x86 emulator: do not adjust the address for immediate source

adjust the dst address for a register source but not adjust the address for an immediate source.

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 f48890d..161d361 100644
@@ -728,7 +728,7 @@ static void fetch_bit_operand(struct decode_cache *c)
 {
 	long sv, mask;
 
-	if (c->dst.type == OP_MEM) {
+	if (c->dst.type == OP_MEM && c->src.type == OP_REG) {
 		mask = ~(c->dst.bytes * 8 - 1);
 
 		if (c->src.bytes == 2)

Signature · 1 line

1.7.0.4

Wei Yongjun

author

2010-08-09 03:41 UTC #8 lore raw

PATCH v2 3/3 KVM: x86 emulator: mask group 8 instruction as BitOp

Mask group 8 instruction as BitOp, so we can share the code for adjust the source operand.

Signed-off-by Wei Yongjun <[email protected]>

---
 arch/x86/kvm/emulate.c |   11 ++++-------
 1 files changed, 4 insertions(+), 7 deletions(-)

arch/x86/kvm/emulate.c +4 -7

index 161d361..db19bcf 100644
@@ -738,6 +738,9 @@ static void fetch_bit_operand(struct decode_cache *c)
 
 		c->dst.addr.mem += (sv >> 3);
 	}
+
+	/* only subword offset */
+	c->src.val &= (c->dst.bytes << 3) - 1;
 }
 
 static int read_emulated(struct x86_emulate_ctxt *ctxt,

@@ -2338,7 +2341,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 */

@@ -3412,8 +3415,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 */

@@ -3441,8 +3442,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 */

@@ -3464,8 +3463,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

Avi Kivity

Applied

2010-08-10 02:46 UTC #9 lore raw

In reply to Wei Yongjun

Quoted 3 lines · Wei Yongjun

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

Thanks, applied all three patches.

Signature · 2 lines

I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.