[PATCH 1/3] KVM: x86 emulator: fix negative bit offset BitOp instruction emulation

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

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 0e360c6..470c7eb 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -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:
-- 
1.7.0.4

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

[Wei Yongjun]

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

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 470c7eb..e7e3d2d 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -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)
-- 
1.7.0.4

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

[Wei Yongjun]

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

Signed-off-by: Wei Yongjun <yjwei@cn.fujitsu.com>
---
v1 -> v2 just fix the missing left bracket of mail title
---
 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 470c7eb..e7e3d2d 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -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)
-- 
1.7.0.4

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

[Wei Yongjun]

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

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

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index e7e3d2d..dc6a74e 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -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 */
-- 
1.7.0.4

Re: [PATCH 1/3] KVM: x86 emulator: fix negative bit offset BitOp instruction emulation

[Paolo Bonzini]

On 08/06/2010 09:17 AM, Wei Yongjun wrote:
> 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 |   32 ++++++++++++++++++++++++++------
>   1 files changed, 26 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index 0e360c6..470c7eb 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -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:

The whole series looks good now.  Thanks!

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Paolo

Re: [PATCH 1/3] KVM: x86 emulator: fix negative bit offset BitOp instruction emulation

[Avi Kivity]

 On 08/06/2010 03:17 AM, Wei Yongjun wrote:
> If bit offset operands is a negative number, BitOp instruction
> will return wrong value. This patch fix it.
>
> +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.

> +
> +		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.

> +}
> +

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

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

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 0e360c6..f48890d 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -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:
-- 
1.7.0.4

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

[Wei Yongjun]

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

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 f48890d..161d361 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -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)
-- 
1.7.0.4

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

[Wei Yongjun]

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

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

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 161d361..db19bcf 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -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 */
-- 
1.7.0.4

Re: [PATCH 1/3 v2] KVM: x86 emulator: fix negative bit offset BitOp instruction emulation

[Avi Kivity]

 On 08/08/2010 11:34 PM, Wei Yongjun wrote:
> If bit offset operands is a negative number, BitOp instruction
> will return wrong value. This patch fix it.
>

Thanks, applied all three patches.

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