Skip to contentSkip to conversation

KVM: x86 emulator: fix pusha instruction emulation

PATCH Applied Wei Yongjun 3 participants 12 messages Jun 15, 2010 – Jun 21, 2010

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

Wei Yongjun

author
#0 lore raw

PATCH 1/1 KVM: x86 emulator: fix pusha instruction emulation

emulate pusha instruction only writeback the last
EDI register, but the other registers which need
to be writeback is ignored. This patch fixed it.

---
 arch/x86/kvm/emulate.c |  133 ++++++++++++++++++++++++++----------------------
 1 files changed, 73 insertions(+), 60 deletions(-)
arch/x86/kvm/emulate.c +73 -60
index a4c2dcd..c990db0 100644
@@ -1553,6 +1553,64 @@ exception:
 	return X86EMUL_PROPAGATE_FAULT;
 }
 
+static inline int writeback(struct x86_emulate_ctxt *ctxt,
+			    struct x86_emulate_ops *ops)
+{
+	int rc;
+	struct decode_cache *c = &ctxt->decode;
+	u32 err;
+
+	switch (c->dst.type) {
+	case OP_REG:
+		/* The 4-byte case *is* correct:
+		 * in 64-bit mode we zero-extend.
+		 */
+		switch (c->dst.bytes) {
+		case 1:
+			*(u8 *)c->dst.ptr = (u8)c->dst.val;
+			break;
+		case 2:
+			*(u16 *)c->dst.ptr = (u16)c->dst.val;
+			break;
+		case 4:
+			*c->dst.ptr = (u32)c->dst.val;
+			break;	/* 64b: zero-ext */
+		case 8:
+			*c->dst.ptr = c->dst.val;
+			break;
+		}
+		break;
+	case OP_MEM:
+		if (c->lock_prefix)
+			rc = ops->cmpxchg_emulated(
+					(unsigned long)c->dst.ptr,
+					&c->dst.orig_val,
+					&c->dst.val,
+					c->dst.bytes,
+					&err,
+					ctxt->vcpu);
+		else
+			rc = ops->write_emulated(
+					(unsigned long)c->dst.ptr,
+					&c->dst.val,
+					c->dst.bytes,
+					&err,
+					ctxt->vcpu);
+		if (rc == X86EMUL_PROPAGATE_FAULT)
+			emulate_pf(ctxt,
+					      (unsigned long)c->dst.ptr, err);
+		if (rc != X86EMUL_CONTINUE)
+			return rc;
+		break;
+	case OP_NONE:
+		/* no writeback */
+		break;
+	default:
+		break;
+	}
+	return X86EMUL_CONTINUE;
+}
+
 static inline void emulate_push(struct x86_emulate_ctxt *ctxt,
 				struct x86_emulate_ops *ops)
 {

@@ -1651,11 +1709,12 @@ static int emulate_pop_sreg(struct x86_emulate_ctxt *ctxt,
 	return rc;
 }
 
-static void emulate_pusha(struct x86_emulate_ctxt *ctxt,
+static int emulate_pusha(struct x86_emulate_ctxt *ctxt,
 			  struct x86_emulate_ops *ops)
 {
 	struct decode_cache *c = &ctxt->decode;
 	unsigned long old_esp = c->regs[VCPU_REGS_RSP];
+	int rc = X86EMUL_CONTINUE;
 	int reg = VCPU_REGS_RAX;
 
 	while (reg <= VCPU_REGS_RDI) {

@@ -1663,8 +1722,18 @@ static void emulate_pusha(struct x86_emulate_ctxt *ctxt,
 		(c->src.val = old_esp) : (c->src.val = c->regs[reg]);
 
 		emulate_push(ctxt, ops);
+
+		rc = writeback(ctxt, ops);
+		if (rc != X86EMUL_CONTINUE)
+			return rc;
+
 		++reg;
 	}
+
+	/* Disable writeback. */
+	c->dst.type = OP_NONE;
+
+	return rc;
 }
 
 static int emulate_popa(struct x86_emulate_ctxt *ctxt,

@@ -1817,64 +1886,6 @@ static int emulate_ret_far(struct x86_emulate_ctxt *ctxt,
 	return rc;
 }
 
-static inline int writeback(struct x86_emulate_ctxt *ctxt,
-			    struct x86_emulate_ops *ops)
-{
-	int rc;
-	struct decode_cache *c = &ctxt->decode;
-	u32 err;
-
-	switch (c->dst.type) {
-	case OP_REG:
-		/* The 4-byte case *is* correct:
-		 * in 64-bit mode we zero-extend.
-		 */
-		switch (c->dst.bytes) {
-		case 1:
-			*(u8 *)c->dst.ptr = (u8)c->dst.val;
-			break;
-		case 2:
-			*(u16 *)c->dst.ptr = (u16)c->dst.val;
-			break;
-		case 4:
-			*c->dst.ptr = (u32)c->dst.val;
-			break;	/* 64b: zero-ext */
-		case 8:
-			*c->dst.ptr = c->dst.val;
-			break;
-		}
-		break;
-	case OP_MEM:
-		if (c->lock_prefix)
-			rc = ops->cmpxchg_emulated(
-					(unsigned long)c->dst.ptr,
-					&c->dst.orig_val,
-					&c->dst.val,
-					c->dst.bytes,
-					&err,
-					ctxt->vcpu);
-		else
-			rc = ops->write_emulated(
-					(unsigned long)c->dst.ptr,
-					&c->dst.val,
-					c->dst.bytes,
-					&err,
-					ctxt->vcpu);
-		if (rc == X86EMUL_PROPAGATE_FAULT)
-			emulate_pf(ctxt,
-					      (unsigned long)c->dst.ptr, err);
-		if (rc != X86EMUL_CONTINUE)
-			return rc;
-		break;
-	case OP_NONE:
-		/* no writeback */
-		break;
-	default:
-		break;
-	}
-	return X86EMUL_CONTINUE;
-}
-
 static inline void
 setup_syscalls_segments(struct x86_emulate_ctxt *ctxt,
 			struct x86_emulate_ops *ops, struct desc_struct *cs,

@@ -2689,7 +2700,9 @@ special_insn:
 			goto done;
 		break;
 	case 0x60:	/* pusha */
-		emulate_pusha(ctxt, ops);
+		rc = emulate_pusha(ctxt, ops);
+		if (rc != X86EMUL_CONTINUE)
+			goto done;
 		break;
 	case 0x61:	/* popa */
 		rc = emulate_popa(ctxt, ops);
Signature · 1 line
1.7.0.4
author
#1 lore raw

PATCH v2 1/1 KVM: x86 emulator: fix pusha instruction emulation

emulate pusha instruction only writeback the last
EDI register, but the other registers which need
to be writeback is ignored. This patch fixed it.

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

---
v1 -> v2 add missing signed off by

---
 arch/x86/kvm/emulate.c |  133 ++++++++++++++++++++++++++----------------------
 1 files changed, 73 insertions(+), 60 deletions(-)
arch/x86/kvm/emulate.c +73 -60
index a4c2dcd..c990db0 100644
@@ -1553,6 +1553,64 @@ exception:
 	return X86EMUL_PROPAGATE_FAULT;
 }
 
+static inline int writeback(struct x86_emulate_ctxt *ctxt,
+			    struct x86_emulate_ops *ops)
+{
+	int rc;
+	struct decode_cache *c = &ctxt->decode;
+	u32 err;
+
+	switch (c->dst.type) {
+	case OP_REG:
+		/* The 4-byte case *is* correct:
+		 * in 64-bit mode we zero-extend.
+		 */
+		switch (c->dst.bytes) {
+		case 1:
+			*(u8 *)c->dst.ptr = (u8)c->dst.val;
+			break;
+		case 2:
+			*(u16 *)c->dst.ptr = (u16)c->dst.val;
+			break;
+		case 4:
+			*c->dst.ptr = (u32)c->dst.val;
+			break;	/* 64b: zero-ext */
+		case 8:
+			*c->dst.ptr = c->dst.val;
+			break;
+		}
+		break;
+	case OP_MEM:
+		if (c->lock_prefix)
+			rc = ops->cmpxchg_emulated(
+					(unsigned long)c->dst.ptr,
+					&c->dst.orig_val,
+					&c->dst.val,
+					c->dst.bytes,
+					&err,
+					ctxt->vcpu);
+		else
+			rc = ops->write_emulated(
+					(unsigned long)c->dst.ptr,
+					&c->dst.val,
+					c->dst.bytes,
+					&err,
+					ctxt->vcpu);
+		if (rc == X86EMUL_PROPAGATE_FAULT)
+			emulate_pf(ctxt,
+					      (unsigned long)c->dst.ptr, err);
+		if (rc != X86EMUL_CONTINUE)
+			return rc;
+		break;
+	case OP_NONE:
+		/* no writeback */
+		break;
+	default:
+		break;
+	}
+	return X86EMUL_CONTINUE;
+}
+
 static inline void emulate_push(struct x86_emulate_ctxt *ctxt,
 				struct x86_emulate_ops *ops)
 {

@@ -1651,11 +1709,12 @@ static int emulate_pop_sreg(struct x86_emulate_ctxt *ctxt,
 	return rc;
 }
 
-static void emulate_pusha(struct x86_emulate_ctxt *ctxt,
+static int emulate_pusha(struct x86_emulate_ctxt *ctxt,
 			  struct x86_emulate_ops *ops)
 {
 	struct decode_cache *c = &ctxt->decode;
 	unsigned long old_esp = c->regs[VCPU_REGS_RSP];
+	int rc = X86EMUL_CONTINUE;
 	int reg = VCPU_REGS_RAX;
 
 	while (reg <= VCPU_REGS_RDI) {

@@ -1663,8 +1722,18 @@ static void emulate_pusha(struct x86_emulate_ctxt *ctxt,
 		(c->src.val = old_esp) : (c->src.val = c->regs[reg]);
 
 		emulate_push(ctxt, ops);
+
+		rc = writeback(ctxt, ops);
+		if (rc != X86EMUL_CONTINUE)
+			return rc;
+
 		++reg;
 	}
+
+	/* Disable writeback. */
+	c->dst.type = OP_NONE;
+
+	return rc;
 }
 
 static int emulate_popa(struct x86_emulate_ctxt *ctxt,

@@ -1817,64 +1886,6 @@ static int emulate_ret_far(struct x86_emulate_ctxt *ctxt,
 	return rc;
 }
 
-static inline int writeback(struct x86_emulate_ctxt *ctxt,
-			    struct x86_emulate_ops *ops)
-{
-	int rc;
-	struct decode_cache *c = &ctxt->decode;
-	u32 err;
-
-	switch (c->dst.type) {
-	case OP_REG:
-		/* The 4-byte case *is* correct:
-		 * in 64-bit mode we zero-extend.
-		 */
-		switch (c->dst.bytes) {
-		case 1:
-			*(u8 *)c->dst.ptr = (u8)c->dst.val;
-			break;
-		case 2:
-			*(u16 *)c->dst.ptr = (u16)c->dst.val;
-			break;
-		case 4:
-			*c->dst.ptr = (u32)c->dst.val;
-			break;	/* 64b: zero-ext */
-		case 8:
-			*c->dst.ptr = c->dst.val;
-			break;
-		}
-		break;
-	case OP_MEM:
-		if (c->lock_prefix)
-			rc = ops->cmpxchg_emulated(
-					(unsigned long)c->dst.ptr,
-					&c->dst.orig_val,
-					&c->dst.val,
-					c->dst.bytes,
-					&err,
-					ctxt->vcpu);
-		else
-			rc = ops->write_emulated(
-					(unsigned long)c->dst.ptr,
-					&c->dst.val,
-					c->dst.bytes,
-					&err,
-					ctxt->vcpu);
-		if (rc == X86EMUL_PROPAGATE_FAULT)
-			emulate_pf(ctxt,
-					      (unsigned long)c->dst.ptr, err);
-		if (rc != X86EMUL_CONTINUE)
-			return rc;
-		break;
-	case OP_NONE:
-		/* no writeback */
-		break;
-	default:
-		break;
-	}
-	return X86EMUL_CONTINUE;
-}
-
 static inline void
 setup_syscalls_segments(struct x86_emulate_ctxt *ctxt,
 			struct x86_emulate_ops *ops, struct desc_struct *cs,

@@ -2689,7 +2700,9 @@ special_insn:
 			goto done;
 		break;
 	case 0x60:	/* pusha */
-		emulate_pusha(ctxt, ops);
+		rc = emulate_pusha(ctxt, ops);
+		if (rc != X86EMUL_CONTINUE)
+			goto done;
 		break;
 	case 0x61:	/* popa */
 		rc = emulate_popa(ctxt, ops);
Signature · 1 line
1.7.0.4
author
#2 lore raw

PATCH 1/1 test: add test for pusha and popa instructions

This patch add test for pusha and popa instructions.

Signed-off-by Wei Yongjun <[email protected]>
---
 kvm/test/x86/realmode.c |   46 ++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 46 insertions(+), 0 deletions(-)
kvm/test/x86/realmode.c +46 -0
index 70a1e05..bd79348 100644
@@ -820,12 +820,58 @@ void test_null(void)
 		print_serial("null test: PASS\n");
 }
 
+void test_pusha_popa()
+{
+	struct regs inregs = { .eax = 0, .ebx = 1, .ecx = 2, .edx = 3, .esi = 4, .edi = 5, .ebp = 6, .esp = 7}, outregs;
+
+	MK_INSN(pusha, "pusha\n\t"
+		       "pop %edi\n\t"
+		       "pop %esi\n\t"
+		       "pop %ebp\n\t"
+		       "pop %eax\n\t"
+		       "pop %ebx\n\t"
+		       "pop %edx\n\t"
+		       "pop %ecx\n\t"
+		       "pop %esp\n\t"
+		       "xchg %esp, %eax\n\t"
+		       );
+
+	MK_INSN(popa, "push %eax\n\t"
+		      "push %ecx\n\t"
+		      "push %edx\n\t"
+		      "push %ebx\n\t"
+		      "push %esp\n\t"
+		      "push %ebp\n\t"
+		      "push %esi\n\t"
+		      "push %edi\n\t"
+		      "popa\n\t"
+		      );
+
+	exec_in_big_real_mode(&inregs, &outregs,
+			      insn_pusha,
+			      insn_pusha_end - insn_pusha);
+
+	if (!regs_equal(&inregs, &outregs, 0))
+		print_serial("Pusha/Popa Test1: FAIL\n");
+	else
+		print_serial("Pusha/Popa Test1: PASS\n");
+
+	exec_in_big_real_mode(&inregs, &outregs,
+			      insn_popa,
+			      insn_popa_end - insn_popa);
+	if (!regs_equal(&inregs, &outregs, 0))
+		print_serial("Pusha/Popa Test2: FAIL\n");
+	else
+		print_serial("Pusha/Popa Test2: PASS\n");
+}
+
 void realmode_start(void)
 {
 	test_null();
 
 	test_shld();
 	test_push_pop();
+	test_pusha_popa();
 	test_mov_imm();
 	test_cmp_imm();
 	test_add_imm();
Signature · 1 line
1.7.0.4
#3 lore raw

Re: [PATCH] test: add test for pusha and popa instructions

Quoted 3 lines · Wei Yongjun

This patch add test for pusha and popa instructions.

Did you test the test? These tests require 'emulate_invalid_guest_state=1' and to run on Intel to actually test anything. You can check with ftrace whether kvm actually emulated pusha/popa.

Signature · 1 line
error compiling committee.c: too many arguments to function
author
#4 lore raw

Re: [PATCH] test: add test for pusha and popa instructions

Quoted 12 lines · Wei Yongjun
Quoted 5 lines

This patch add test for pusha and popa instructions.

Did you test the test? These tests require 'emulate_invalid_guest_state=1' and to run on Intel to actually test anything. You can check with ftrace whether kvm actually emulated pusha/popa.

Yes, I did test this case, and checked kvm actually emulated pusha/popa by add printk to kernel source.

Quoted 2 lines
#5 lore raw

Re: [PATCH] test: add test for pusha and popa instructions

Quoted 10 lines · Wei Yongjun
Quoted 7 lines

Did you test the test? These tests require 'emulate_invalid_guest_state=1' and to run on Intel to actually test anything. You can check with ftrace whether kvm actually emulated pusha/popa.

Yes, I did test this case, and checked kvm actually emulated pusha/popa by add printk to kernel source.

I see, thanks. Anyway Marcelo already applied it.

Signature · 1 line
error compiling committee.c: too many arguments to function
#6 lore raw
In reply to Wei Yongjun
Quoted 5 lines · Wei Yongjun

emulate pusha instruction only writeback the last
EDI register, but the other registers which need
to be writeback is ignored. This patch fixed it.

Ouch.

Quoted 12 lines
arch/x86/kvm/emulate.c +2 -1
-static void emulate_pusha(struct x86_emulate_ctxt *ctxt,
+static int emulate_pusha(struct x86_emulate_ctxt *ctxt,
 			  struct x86_emulate_ops *ops)
 {
 	struct decode_cache *c = &ctxt->decode;
 	unsigned long old_esp = c->regs[VCPU_REGS_RSP];
+	int rc = X86EMUL_CONTINUE;
 	int reg = VCPU_REGS_RAX;
 
 	while (reg <= VCPU_REGS_RDI) {

arch/x86/kvm/emulate.c static void emulate_pusha(struct x86_emulate_ctxt *ctxt, · line 1730

Quoted 9 lines
arch/x86/kvm/emulate.c +5 -0
@@ -1663,8 +1722,18 @@ static void emulate_pusha(struct x86_emulate_ctxt *ctxt,
 		(c->src.val = old_esp) : (c->src.val = c->regs[reg]);
 
 		emulate_push(ctxt, ops);
+
+		rc = writeback(ctxt, ops);
+		if (rc != X86EMUL_CONTINUE)
+			return rc;
+

We might have partial completion if we fault on the one the second or later register. Still, it's an improvement over what we have now.

Quoted 10 lines
arch/x86/kvm/emulate.c +5 -0
 		++reg;
 	}
+
+	/* Disable writeback. */
+	c->dst.type = OP_NONE;
+
+	return rc;
 }
Signature · 1 line
error compiling committee.c: too many arguments to function
Applied
#7 lore raw
In reply to Wei Yongjun
Quoted 7 lines · Wei Yongjun

emulate pusha instruction only writeback the last
EDI register, but the other registers which need
to be writeback is ignored. This patch fixed it.

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

Applied, thanks.

author
#8 lore raw

PATCH 1/1 KVM: x86 emulator: fix group3 instruction decoding

Group 3 instruction with ModRM reg field as 001 is defined as test instruction under AMD arch, and emulate_grp3() is ready for emulate it, so fix the decoding.

static inline int emulate_grp3(...)

{
	...
	switch (c->modrm_reg) {
	case 0 ... 1:   /* test */
		emulate_2op_SrcV("test", c->src, c->dst, ctxt->eflags);
	...
}
Signed-off-by Wei Yongjun <[email protected]>
---
 arch/x86/kvm/emulate.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
arch/x86/kvm/emulate.c +2 -2
index c990db0..abb8cec 100644
@@ -336,11 +336,11 @@ static u32 group_table[] = {
 	[Group1A*8] =
 	DstMem | SrcNone | ModRM | Mov | Stack, 0, 0, 0, 0, 0, 0, 0,
 	[Group3_Byte*8] =
-	ByteOp | SrcImm | DstMem | ModRM, 0,
+	ByteOp | SrcImm | DstMem | ModRM, ByteOp | SrcImm | DstMem | ModRM,
 	ByteOp | DstMem | SrcNone | ModRM, ByteOp | DstMem | SrcNone | ModRM,
 	0, 0, 0, 0,
 	[Group3*8] =
-	DstMem | SrcImm | ModRM, 0,
+	DstMem | SrcImm | ModRM, DstMem | SrcImm | ModRM,
 	DstMem | SrcNone | ModRM, DstMem | SrcNone | ModRM,
 	0, 0, 0, 0,
 	[Group4*8] =
Signature · 1 line
1.7.0.4
#9 lore raw

Re: [PATCH] KVM: x86 emulator: fix group3 instruction decoding

Quoted 6 lines · Wei Yongjun

Group 3 instruction with ModRM reg field as 001 is defined as test instruction under AMD arch, and emulate_grp3() is ready for emulate it, so fix the decoding.

Strange but true.

Did you encounter any situation which actually needed this, or did you find this by code inspection?

Signature · 1 line
error compiling committee.c: too many arguments to function
author
#10 lore raw

Re: [PATCH] KVM: x86 emulator: fix group3 instruction decoding

Quoted 9 lines · Wei Yongjun
Quoted 6 lines

Group 3 instruction with ModRM reg field as 001 is defined as test instruction under AMD arch, and emulate_grp3() is ready for emulate it, so fix the decoding.

Quoted 4 lines

Strange but true.

Did you encounter any situation which actually needed this, or did you find this by code inspection?

Just found it by source review, looks strange, so...

#11 lore raw

Re: [PATCH] KVM: x86 emulator: fix group3 instruction decoding

Quoted 10 lines · Wei Yongjun
Quoted 10 lines · Wei Yongjun
Quoted 8 lines

Group 3 instruction with ModRM reg field as 001 is defined as test instruction under AMD arch, and emulate_grp3() is ready for emulate it, so fix the decoding.

Quoted 7 lines
Quoted 5 lines

Strange but true.

Did you encounter any situation which actually needed this, or did you find this by code inspection?

Just found it by source review, looks strange, so...

Well, the emulator usually puts me to sleep, so good catch. Applied the patch, thanks.

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