[PATCH 0/5][RESEND] Split the emulator: decode & execute

[PATCH 0/5][RESEND] Split the emulator: decode & execute

[Laurent Vivier]

[-- Attachment #1.1: Type: text/plain, Size: 1907 bytes --]

These patches split the emulator in two parts: one to decode the instruction,
the other to execute it. The decode part is then called only when needed.

[PATCH 1/5] x86_emulate-remove_unused, some cleanup: remove unused functions
[PATCH 2/5] x86_emulate-context, move all x86_emulate_memop() common variables
            between decode and execute to a structure decode_cache.

            struct decode_cache {
                u8 twobyte;
                u8 b;
                u8 lock_prefix;
                u8 rep_prefix;
                u8 op_bytes;
                u8 ad_bytes;
                struct operand src;
                struct operand dst;
                unsigned long *override_base;
                unsigned int d;
                unsigned long regs[NR_VCPU_REGS];
                unsigned long eip;
                /* modrm */
                u8 modrm;
                u8 modrm_mod;
                u8 modrm_reg;
                u8 modrm_rm;
                u8 use_modrm_ea;
                unsigned long modrm_ea;
                unsigned long modrm_val;
           };

[PATCH 3/5] x86_emulate-decode_insn, move all decoding process to function
            x86_decode_insn().
[PATCH 4/5] x86_emulate-split, emulate_instruction() calls now x86_decode_insn()
	    and x86_emulate_insn(). x86_emulate_insn() is x86_emulate_memop()
            without the decoding part.
[PATCH 5/5] x86_emulate-optimize, move emulate_ctxt to kvm_vcpu to keep emulate
            context when we exit from kvm module. Call x86_decode_insn() only
            when needed. Modify x86_emulate_insn() to not modify the context if
            it must be re-entered.

Signed-off-by: Laurent Vivier <Laurent.Vivier-6ktuUTfB/bM@public.gmane.org>
-- 
------------- Laurent.Vivier-6ktuUTfB/bM@public.gmane.org  --------------
          "Software is hard" - Donald Knuth



[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 228 bytes --]

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

[-- Attachment #3: Type: text/plain, Size: 186 bytes --]

_______________________________________________
kvm-devel mailing list
kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/kvm-devel

Re: [PATCH 0/5][RESEND] Split the emulator: decode & execute

[Avi Kivity]

Laurent Vivier wrote:
> These patches split the emulator in two parts: one to decode the instruction,
> the other to execute it. The decode part is then called only when needed.
>
>   

Applied all, thanks.

I changed the name of the variable 'decode' to 'c' to reduce the number 
of line splits.

This patchset, in addition to the correctness issues, also enables 
further cleaning up of x86_emulate.c: since most variables are now in a 
structure, chunks of the code can be moved to a separate function that 
takes just the decode cache.  This will reduce
the high indent level and increase readability.

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


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH 0/5][RESEND] Split the emulator: decode & execute

[Laurent Vivier]

[-- Attachment #1.1: Type: text/plain, Size: 841 bytes --]

Avi Kivity wrote:
> Laurent Vivier wrote:
>> These patches split the emulator in two parts: one to decode the instruction,
>> the other to execute it. The decode part is then called only when needed.
>>
>>   
> 
> Applied all, thanks.
> 
> I changed the name of the variable 'decode' to 'c' to reduce the number 
> of line splits.

No problem

> This patchset, in addition to the correctness issues, also enables 
> further cleaning up of x86_emulate.c: since most variables are now in a 
> structure, chunks of the code can be moved to a separate function that 
> takes just the decode cache.  This will reduce
> the high indent level and increase readability.

I can do that too.

Laurent
-- 
------------- Laurent.Vivier-6ktuUTfB/bM@public.gmane.org  --------------
          "Software is hard" - Donald Knuth


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 228 bytes --]

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

[-- Attachment #3: Type: text/plain, Size: 186 bytes --]

_______________________________________________
kvm-devel mailing list
kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/kvm-devel

[PATCH] move grp decoding to functions to make x86_emulate_insn() clearer

[Laurent Vivier]

To improve readability, move push, writeback, and grp 1a/2/3/4/5 emulation parts to functions.

Signed-off-by: Laurent Vivier <Laurent.Vivier-6ktuUTfB/bM@public.gmane.org>
---
 drivers/kvm/x86_emulate.c |  447 ++++++++++++++++++++++++++-------------------
 1 files changed, 262 insertions(+), 185 deletions(-)

diff --git a/drivers/kvm/x86_emulate.c b/drivers/kvm/x86_emulate.c
index 663dc57..b036ff8 100644
--- a/drivers/kvm/x86_emulate.c
+++ b/drivers/kvm/x86_emulate.c
@@ -897,6 +897,240 @@ done:
 	return (rc == X86EMUL_UNHANDLEABLE) ? -1 : 0;
 }
 
+static inline void emulate_push(struct x86_emulate_ctxt *ctxt)
+{
+	struct decode_cache *c = &ctxt->decode;
+
+	c->dst.type  = OP_MEM;
+	c->dst.bytes = c->op_bytes;
+	c->dst.val = c->src.val;
+	register_address_increment(c->regs[VCPU_REGS_RSP], -c->op_bytes);
+	c->dst.ptr = (void *) register_address(ctxt->ss_base, c->regs[VCPU_REGS_RSP]);
+}
+
+static inline int emulate_grp1a(struct x86_emulate_ctxt *ctxt, struct x86_emulate_ops *ops)
+{
+	struct decode_cache *c = &ctxt->decode;
+	int rc;
+
+	/* 64-bit mode: POP always pops a 64-bit operand. */
+
+	if (ctxt->mode == X86EMUL_MODE_PROT64)
+		c->dst.bytes = 8;
+
+	rc = ops->read_std(register_address(ctxt->ss_base, c->regs[VCPU_REGS_RSP]),
+			   &c->dst.val, c->dst.bytes, ctxt->vcpu);
+	if (rc != 0)
+		return rc;
+
+	register_address_increment(c->regs[VCPU_REGS_RSP], c->dst.bytes);
+
+	return 0;
+}
+
+static inline void emulate_grp2(struct decode_cache *c, unsigned long *_eflags)
+{
+	switch (c->modrm_reg) {
+	case 0:	/* rol */
+		emulate_2op_SrcB("rol", c->src, c->dst, *_eflags);
+		break;
+	case 1:	/* ror */
+		emulate_2op_SrcB("ror", c->src, c->dst, *_eflags);
+		break;
+	case 2:	/* rcl */
+		emulate_2op_SrcB("rcl", c->src, c->dst, *_eflags);
+		break;
+	case 3:	/* rcr */
+		emulate_2op_SrcB("rcr", c->src, c->dst, *_eflags);
+		break;
+	case 4:	/* sal/shl */
+	case 6:	/* sal/shl */
+		emulate_2op_SrcB("sal", c->src, c->dst, *_eflags);
+		break;
+	case 5:	/* shr */
+		emulate_2op_SrcB("shr", c->src, c->dst, *_eflags);
+		break;
+	case 7:	/* sar */
+		emulate_2op_SrcB("sar", c->src, c->dst, *_eflags);
+		break;
+	}
+}
+
+static inline int emulate_grp3(struct x86_emulate_ctxt *ctxt,
+			       struct x86_emulate_ops *ops,
+			       unsigned long *_eflags)
+{
+	struct decode_cache *c = &ctxt->decode;
+	int rc = 0;
+
+	switch (c->modrm_reg) {
+	case 0 ... 1:	/* test */
+		/*
+		 * Special case in Grp3: test has an immediate
+		 * source operand.
+		 */
+		c->src.type = OP_IMM;
+		c->src.ptr = (unsigned long *)c->eip;
+		c->src.bytes = (c->d & ByteOp) ? 1 : c->op_bytes;
+		if (c->src.bytes == 8)
+			c->src.bytes = 4;
+		switch (c->src.bytes) {
+		case 1:
+			c->src.val = insn_fetch(s8, 1, c->eip);
+			break;
+		case 2:
+			c->src.val = insn_fetch(s16, 2, c->eip);
+			break;
+		case 4:
+			c->src.val = insn_fetch(s32, 4, c->eip);
+			break;
+		}
+		emulate_2op_SrcV("test", c->src, c->dst, *_eflags);
+		break;
+	case 2:	/* not */
+		c->dst.val = ~c->dst.val;
+		break;
+	case 3:	/* neg */
+		emulate_1op("neg", c->dst, _eflags);
+		break;
+	default:
+		DPRINTF("Cannot emulate %02x\n", c->b);
+		rc = X86EMUL_UNHANDLEABLE;
+		break;
+	}
+done:
+	return rc;
+}
+
+static inline int emulate_grp45(struct x86_emulate_ctxt *ctxt,
+			       struct x86_emulate_ops *ops,
+			       unsigned long *_eflags,
+			       int *no_wb)
+{
+	struct decode_cache *c = &ctxt->decode;
+	int rc;
+
+	switch (c->modrm_reg) {
+	case 0:	/* inc */
+		emulate_1op("inc", c->dst, *_eflags);
+		break;
+	case 1:	/* dec */
+		emulate_1op("dec", c->dst, *_eflags);
+		break;
+	case 4: /* jmp abs */
+		if (c->b == 0xff)
+			c->eip = c->dst.val;
+		else {
+			DPRINTF("Cannot emulate %02x\n", c->b);
+			return X86EMUL_UNHANDLEABLE;
+		}
+		break;
+	case 6:	/* push */
+
+		/* 64-bit mode: PUSH always pushes a 64-bit operand. */
+
+		if (ctxt->mode == X86EMUL_MODE_PROT64) {
+			c->dst.bytes = 8;
+			rc = ops->read_std((unsigned long)c->dst.ptr,
+					   &c->dst.val, 8, ctxt->vcpu);
+			if (rc != 0)
+				return rc;
+		}
+		register_address_increment(c->regs[VCPU_REGS_RSP],
+					   -c->dst.bytes);
+		rc = ops->write_std(register_address(ctxt->ss_base,
+				    c->regs[VCPU_REGS_RSP]), &c->dst.val,
+				    c->dst.bytes, ctxt->vcpu);
+		if (rc != 0)
+			return rc;
+		*no_wb = 1;
+		break;
+	default:
+		DPRINTF("Cannot emulate %02x\n", c->b);
+		return X86EMUL_UNHANDLEABLE;
+	}
+	return 0;
+}
+
+static inline int emulate_grp9(struct x86_emulate_ctxt *ctxt, 
+			       struct x86_emulate_ops *ops,
+			       unsigned long *_eflags,
+			       unsigned long cr2)
+{
+	struct decode_cache *c = &ctxt->decode;
+	u64 old, new;
+	int rc;
+
+	rc = ops->read_emulated(cr2, &old, 8, ctxt->vcpu);
+	if (rc != 0)
+		return rc;
+
+	if (((u32) (old >> 0) != (u32) c->regs[VCPU_REGS_RAX]) ||
+	    ((u32) (old >> 32) != (u32) c->regs[VCPU_REGS_RDX])) {
+
+		c->regs[VCPU_REGS_RAX] = (u32) (old >> 0);
+		c->regs[VCPU_REGS_RDX] = (u32) (old >> 32);
+		*_eflags &= ~EFLG_ZF;
+
+	} else {
+		new = ((u64)c->regs[VCPU_REGS_RCX] << 32) | (u32) c->regs[VCPU_REGS_RBX];
+
+		rc = ops->cmpxchg_emulated(cr2, &old, &new, 8, ctxt->vcpu);
+		if (rc != 0)
+			return rc;
+		*_eflags |= EFLG_ZF;
+	}
+	return 0;
+}
+
+static inline int writeback(struct x86_emulate_ctxt *ctxt,  
+			    struct x86_emulate_ops *ops)
+{
+	int rc;
+	struct decode_cache *c = &ctxt->decode;
+
+	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,
+					ctxt->vcpu);
+		else
+			rc = ops->write_emulated(
+					(unsigned long)c->dst.ptr,
+					&c->dst.val,
+					c->dst.bytes,
+					ctxt->vcpu);
+		if (rc != 0)
+			return rc;
+	default:
+		break;
+	}
+	return 0;
+}
+
 int
 x86_emulate_insn(struct x86_emulate_ctxt *ctxt, struct x86_emulate_ops *ops)
 {
@@ -1006,14 +1240,7 @@ x86_emulate_insn(struct x86_emulate_ctxt *ctxt, struct x86_emulate_ops *ops)
 	case 0x6a: /* push imm8 */
 		c->src.val = 0L;
 		c->src.val = insn_fetch(s8, 1, c->eip);
-push:
-		c->dst.type  = OP_MEM;
-		c->dst.bytes = c->op_bytes;
-		c->dst.val = c->src.val;
-		register_address_increment(c->regs[VCPU_REGS_RSP],
-					   -c->op_bytes);
-		c->dst.ptr = (void *) register_address(ctxt->ss_base,
-						       c->regs[VCPU_REGS_RSP]);
+		emulate_push(ctxt);
 		break;
 	case 0x80 ... 0x83:	/* Grp1 */
 		switch (c->modrm_reg) {
@@ -1036,7 +1263,6 @@ push:
 		}
 		break;
 	case 0x84 ... 0x85:
-	      test:		/* test */
 		emulate_2op_SrcV("test", c->src, c->dst, _eflags);
 		break;
 	case 0x86 ... 0x87:	/* xchg */
@@ -1068,18 +1294,9 @@ push:
 		c->dst.val = c->modrm_val;
 		break;
 	case 0x8f:		/* pop (sole member of Grp1a) */
-		/* 64-bit mode: POP always pops a 64-bit operand. */
-		if (ctxt->mode == X86EMUL_MODE_PROT64)
-			c->dst.bytes = 8;
-		if ((rc = ops->read_std(register_address(
-						   ctxt->ss_base,
-						   c->regs[VCPU_REGS_RSP]),
-						   &c->dst.val,
-						   c->dst.bytes,
-						   ctxt->vcpu)) != 0)
+		rc = emulate_grp1a(ctxt, ops);
+		if (rc != 0)
 			goto done;
-		register_address_increment(c->regs[VCPU_REGS_RSP],
-					   c->dst.bytes);
 		break;
 	case 0xa0 ... 0xa1:	/* mov */
 		c->dst.ptr = (unsigned long *)&c->regs[VCPU_REGS_RAX];
@@ -1093,31 +1310,7 @@ push:
 		c->eip += c->ad_bytes;
 		break;
 	case 0xc0 ... 0xc1:
-	      grp2:		/* Grp2 */
-		switch (c->modrm_reg) {
-		case 0:	/* rol */
-			emulate_2op_SrcB("rol", c->src, c->dst, _eflags);
-			break;
-		case 1:	/* ror */
-			emulate_2op_SrcB("ror", c->src, c->dst, _eflags);
-			break;
-		case 2:	/* rcl */
-			emulate_2op_SrcB("rcl", c->src, c->dst, _eflags);
-			break;
-		case 3:	/* rcr */
-			emulate_2op_SrcB("rcr", c->src, c->dst, _eflags);
-			break;
-		case 4:	/* sal/shl */
-		case 6:	/* sal/shl */
-			emulate_2op_SrcB("sal", c->src, c->dst, _eflags);
-			break;
-		case 5:	/* shr */
-			emulate_2op_SrcB("shr", c->src, c->dst, _eflags);
-			break;
-		case 7:	/* sar */
-			emulate_2op_SrcB("sar", c->src, c->dst, _eflags);
-			break;
-		}
+		emulate_grp2(c, &_eflags);
 		break;
 	case 0xc6 ... 0xc7:	/* mov (sole member of Grp11) */
 	mov:
@@ -1125,10 +1318,12 @@ push:
 		break;
 	case 0xd0 ... 0xd1:	/* Grp2 */
 		c->src.val = 1;
-		goto grp2;
+		emulate_grp2(c, &_eflags);
+		break;
 	case 0xd2 ... 0xd3:	/* Grp2 */
 		c->src.val = c->regs[VCPU_REGS_RCX];
-		goto grp2;
+		emulate_grp2(c, &_eflags);
+		break;
 	case 0xe8: /* call (near) */ {
 		long int rel;
 		switch (c->op_bytes) {
@@ -1147,7 +1342,8 @@ push:
 		}
 		c->src.val = (unsigned long) c->eip;
 		JMP_REL(rel);
-		goto push;
+		emulate_push(ctxt);
+		break;
 	}
 	case 0xe9: /* jmp rel */
 	case 0xeb: /* jmp rel short */
@@ -1155,121 +1351,22 @@ push:
 		no_wb = 1; /* Disable writeback. */
 		break;
 	case 0xf6 ... 0xf7:	/* Grp3 */
-		switch (c->modrm_reg) {
-		case 0 ... 1:	/* test */
-			/*
-			 * Special case in Grp3: test has an immediate
-			 * source operand.
-			 */
-			c->src.type = OP_IMM;
-			c->src.ptr = (unsigned long *)c->eip;
-			c->src.bytes = (c->d & ByteOp) ? 1 :
-							       c->op_bytes;
-			if (c->src.bytes == 8)
-				c->src.bytes = 4;
-			switch (c->src.bytes) {
-			case 1:
-				c->src.val = insn_fetch(s8, 1, c->eip);
-				break;
-			case 2:
-				c->src.val = insn_fetch(s16, 2, c->eip);
-				break;
-			case 4:
-				c->src.val = insn_fetch(s32, 4, c->eip);
-				break;
-			}
-			goto test;
-		case 2:	/* not */
-			c->dst.val = ~c->dst.val;
-			break;
-		case 3:	/* neg */
-			emulate_1op("neg", c->dst, _eflags);
-			break;
-		default:
-			goto cannot_emulate;
-		}
+		rc = emulate_grp3(ctxt, ops, &_eflags);
+		if (rc != 0)
+			goto done;
 		break;
 	case 0xfe ... 0xff:	/* Grp4/Grp5 */
-		switch (c->modrm_reg) {
-		case 0:	/* inc */
-			emulate_1op("inc", c->dst, _eflags);
-			break;
-		case 1:	/* dec */
-			emulate_1op("dec", c->dst, _eflags);
-			break;
-		case 4: /* jmp abs */
-			if (c->b == 0xff)
-				c->eip = c->dst.val;
-			else
-				goto cannot_emulate;
-			break;
-		case 6:	/* push */
-			/* 64-bit mode: PUSH always pushes a 64-bit operand. */
-			if (ctxt->mode == X86EMUL_MODE_PROT64) {
-				c->dst.bytes = 8;
-				if ((rc = ops->read_std(
-						 (unsigned long)c->dst.ptr,
-						 &c->dst.val, 8,
-						 ctxt->vcpu)) != 0)
-					goto done;
-			}
-			register_address_increment(c->regs[VCPU_REGS_RSP],
-						   -c->dst.bytes);
-			if ((rc = ops->write_std(
-				     register_address(ctxt->ss_base,
-					  c->regs[VCPU_REGS_RSP]),
-					  &c->dst.val,
-					   c->dst.bytes, ctxt->vcpu)) != 0)
-				goto done;
-			no_wb = 1;
-			break;
-		default:
-			goto cannot_emulate;
-		}
+		rc = emulate_grp45(ctxt, ops, &_eflags, &no_wb);
+		if (rc != 0)
+			goto done;
 		break;
 	}
 
 writeback:
 	if (!no_wb) {
-		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,
-						ctxt->vcpu);
-			else
-				rc = ops->write_emulated(
-						(unsigned long)c->dst.ptr,
-						&c->dst.val,
-						c->dst.bytes,
-						ctxt->vcpu);
-			if (rc != 0)
-				goto done;
-		default:
-			break;
-		}
+		rc = writeback(ctxt, ops);
+		if (rc != 0)
+			goto done;
 	}
 
 	/* Commit shadow register state. */
@@ -1298,8 +1395,7 @@ special_insn:
 			ctxt->ss_base, c->regs[VCPU_REGS_RSP]);
 		break;
 	case 0x58 ... 0x5f: /* pop reg */
-		c->dst.ptr =
-				(unsigned long *)&c->regs[c->b & 0x7];
+		c->dst.ptr = (unsigned long *)&c->regs[c->b & 0x7];
 	pop_instruction:
 		if ((rc = ops->read_std(register_address(ctxt->ss_base,
 			c->regs[VCPU_REGS_RSP]), c->dst.ptr,
@@ -1354,7 +1450,8 @@ special_insn:
 	}
 	case 0x9c: /* pushf */
 		c->src.val =  (unsigned long) _eflags;
-		goto push;
+		emulate_push(ctxt);
+		break;
 	case 0x9d: /* popf */
 		c->dst.ptr = (unsigned long *) &_eflags;
 		goto pop_instruction;
@@ -1500,8 +1597,7 @@ twobyte_insn:
 		no_wb = 1;
 		if (c->modrm_mod != 3)
 			goto cannot_emulate;
-		rc = emulator_get_dr(ctxt, c->modrm_reg,
-				     &c->regs[c->modrm_rm]);
+		rc = emulator_get_dr(ctxt, c->modrm_reg, &c->regs[c->modrm_rm]);
 		break;
 	case 0x23: /* mov from reg to dr */
 		no_wb = 1;
@@ -1650,8 +1746,7 @@ twobyte_special_insn:
 		break;
 	case 0x32:
 		/* rdmsr */
-		rc = kvm_get_msr(ctxt->vcpu,
-				 c->regs[VCPU_REGS_RCX], &msr_data);
+		rc = kvm_get_msr(ctxt->vcpu, c->regs[VCPU_REGS_RCX], &msr_data);
 		if (rc) {
 			kvm_x86_ops->inject_gp(ctxt->vcpu, 0);
 			c->eip = ctxt->vcpu->rip;
@@ -1683,28 +1778,10 @@ twobyte_special_insn:
 		break;
 	}
 	case 0xc7:		/* Grp9 (cmpxchg8b) */
-		{
-			u64 old, new;
-			if ((rc = ops->read_emulated(cr2, &old, 8, ctxt->vcpu))
-									!= 0)
-				goto done;
-			if (((u32) (old >> 0) !=
-					(u32) c->regs[VCPU_REGS_RAX]) ||
-			    ((u32) (old >> 32) !=
-					(u32) c->regs[VCPU_REGS_RDX])) {
-				c->regs[VCPU_REGS_RAX] = (u32) (old >> 0);
-				c->regs[VCPU_REGS_RDX] = (u32) (old >> 32);
-				_eflags &= ~EFLG_ZF;
-			} else {
-				new = ((u64)c->regs[VCPU_REGS_RCX] << 32)
-					| (u32) c->regs[VCPU_REGS_RBX];
-				if ((rc = ops->cmpxchg_emulated(cr2, &old,
-							  &new, 8, ctxt->vcpu)) != 0)
-					goto done;
-				_eflags |= EFLG_ZF;
-			}
-			break;
-		}
+		rc = emulate_grp9(ctxt, ops, &_eflags, cr2);
+		if (rc != 0)
+			goto done;
+		break;
 	}
 	goto writeback;
 
-- 
1.5.2.4


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] move grp decoding to functions to make x86_emulate_insn() clearer

[Avi Kivity]

Laurent Vivier wrote:
> To improve readability, move push, writeback, and grp 1a/2/3/4/5 emulation parts to functions.
> +static inline int emulate_grp45(struct x86_emulate_ctxt *ctxt,
> +			       struct x86_emulate_ops *ops,
> +			       unsigned long *_eflags,
> +			       int *no_wb)
>   

If you move _eflags and no_wb to the decode cache (in a separate patch) 
then this patch can be much nicer.


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


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] move grp decoding to functions to make x86_emulate_insn() clearer

[Laurent Vivier]

Avi Kivity wrote:
> Laurent Vivier wrote:
>> To improve readability, move push, writeback, and grp 1a/2/3/4/5 emulation parts to functions.
>> +static inline int emulate_grp45(struct x86_emulate_ctxt *ctxt,
>> +			       struct x86_emulate_ops *ops,
>> +			       unsigned long *_eflags,
>> +			       int *no_wb)
>>   
> 
> If you move _eflags and no_wb to the decode cache (in a separate patch) 
> then this patch can be much nicer.

I agree but this increases the size of the structure shared with the 
userspace with variable used only locally in x86_emulate.c, is it 
acceptable ?

Laurent





-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] move grp decoding to functions to make x86_emulate_insn() clearer

[Avi Kivity]

Laurent Vivier wrote:
> Avi Kivity wrote:
>> Laurent Vivier wrote:
>>> To improve readability, move push, writeback, and grp 1a/2/3/4/5 
>>> emulation parts to functions.
>>> +static inline int emulate_grp45(struct x86_emulate_ctxt *ctxt,
>>> +                   struct x86_emulate_ops *ops,
>>> +                   unsigned long *_eflags,
>>> +                   int *no_wb)
>>>   
>>
>> If you move _eflags and no_wb to the decode cache (in a separate 
>> patch) then this patch can be much nicer.
>
> I agree but this increases the size of the structure shared with the 
> userspace with variable used only locally in x86_emulate.c, is it 
> acceptable ?
>

It isn't shared with userspace, just part of the vcpu.

Looking a bit more, eflags is already present in x86_emulate_ctxt (and 
could be moved from there), and no_wb can be merged into opcode_table 
and twobyte_opcode_table, so long term there is no size change.

-- 
Any sufficiently difficult bug is indistinguishable from a feature.


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] move grp decoding to functions to make x86_emulate_insn() clearer

[Laurent Vivier]

Avi Kivity wrote:
> Laurent Vivier wrote:
>> Avi Kivity wrote:
>>> Laurent Vivier wrote:
>>>> To improve readability, move push, writeback, and grp 1a/2/3/4/5 
>>>> emulation parts to functions.
>>>> +static inline int emulate_grp45(struct x86_emulate_ctxt *ctxt,
>>>> +                   struct x86_emulate_ops *ops,
>>>> +                   unsigned long *_eflags,
>>>> +                   int *no_wb)
>>>>   
>>>
>>> If you move _eflags and no_wb to the decode cache (in a separate 
>>> patch) then this patch can be much nicer.
>>
>> I agree but this increases the size of the structure shared with the 
>> userspace with variable used only locally in x86_emulate.c, is it 
>> acceptable ?
>>
> 
> It isn't shared with userspace, just part of the vcpu.

OK

> Looking a bit more, eflags is already present in x86_emulate_ctxt (and 

OK, I think we can do the same thing with cr2 ?

> could be moved from there), and no_wb can be merged into opcode_table 
> and twobyte_opcode_table, so long term there is no size change.

OK.

Laurent

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] move grp decoding to functions to make x86_emulate_insn() clearer

[Avi Kivity]

Laurent Vivier wrote:
>>>
>>> I agree but this increases the size of the structure shared with the 
>>> userspace with variable used only locally in x86_emulate.c, is it 
>>> acceptable ?
>>>
>>
>> It isn't shared with userspace, just part of the vcpu.
>
> OK
>
>> Looking a bit more, eflags is already present in x86_emulate_ctxt (and 
>
> OK, I think we can do the same thing with cr2 ?
>

For the present, yes.  For the future, cr2 should be killed off since 
it's wrong to depend on it: sometimes we emulate not in response to a 
page fault, so we don't have a cr2, and sometimes, when the access 
crosses a page boundary, cr2 may point at the second half of the access 
instead of the correct location.

I already fixed most of the uses of cr2, but I think some remain (mov 
abs is one example).

-- 
Any sufficiently difficult bug is indistinguishable from a feature.


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/