[PATCH] KVM: x86 emulator: fix pusha instruction emulation

[PATCH] KVM: x86 emulator: fix pusha instruction emulation

[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(-)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index a4c2dcd..c990db0 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -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);
-- 
1.7.0.4

[PATCHv2] KVM: x86 emulator: fix pusha instruction emulation

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

Signed-off-by: Wei Yongjun <yjwei@cn.fujitsu.com>
---
v1 -> v2 add missing signed off by
---
 arch/x86/kvm/emulate.c |  133 ++++++++++++++++++++++++++----------------------
 1 files changed, 73 insertions(+), 60 deletions(-)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index a4c2dcd..c990db0 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -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);
-- 
1.7.0.4

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

[Wei Yongjun]

This patch add test for pusha and popa instructions.

Signed-off-by: Wei Yongjun <yjwei@cn.fujitsu.com>
---
 kvm/test/x86/realmode.c |   46 ++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 46 insertions(+), 0 deletions(-)

diff --git a/kvm/test/x86/realmode.c b/kvm/test/x86/realmode.c
index 70a1e05..bd79348 100644
--- a/kvm/test/x86/realmode.c
+++ b/kvm/test/x86/realmode.c
@@ -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();
-- 
1.7.0.4

Re: [PATCHv2] KVM: x86 emulator: fix pusha instruction emulation

[Avi Kivity]

On 06/15/2010 04:03 AM, Wei Yongjun wrote:
> 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.

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

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.

>  		++reg;
>  	}
> +
> +	/* Disable writeback. */
> +	c->dst.type = OP_NONE;
> +
> +	return rc;
>  }
>  
>   

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH] KVM: x86 emulator: fix pusha instruction emulation

[Marcelo Tosatti]

On Tue, Jun 15, 2010 at 09:00:16AM +0800, Wei Yongjun wrote:
> 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.

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

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

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 <yjwei@cn.fujitsu.com>
---
 arch/x86/kvm/emulate.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index c990db0..abb8cec 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -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] =
-- 
1.7.0.4

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

[Avi Kivity]

On 06/17/2010 12:33 PM, Wei Yongjun wrote:
> 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?

-- 
error compiling committee.c: too many arguments to function

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

[Wei Yongjun]

> On 06/17/2010 12:33 PM, Wei Yongjun wrote:
>> 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?

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

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

[Avi Kivity]

On 06/18/2010 03:51 AM, Wei Yongjun wrote:
>    
>> On 06/17/2010 12:33 PM, Wei Yongjun wrote:
>>      
>>> 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?
>>      
> Just found it by source review, looks strange, so...
>    

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

-- 
error compiling committee.c: too many arguments to function

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

[Avi Kivity]

On 06/15/2010 05:10 AM, Wei Yongjun wrote:
> 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.


-- 
error compiling committee.c: too many arguments to function

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

[Wei Yongjun]

> On 06/15/2010 05:10 AM, Wei Yongjun wrote:
>   
>> 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.

>
>   

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

[Avi Kivity]

On 06/21/2010 05:48 AM, Wei Yongjun wrote:
>>
>> 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.

-- 
error compiling committee.c: too many arguments to function