Skip to contentSkip to conversation

Split the emulator: decode & execute

PATCH RESEND Applied Laurent Vivier 2 participants 9 messages Sep 18, 2007 – Sep 20, 2007 7 messages not in the archive KVM

lore
Share & tools
lore

Laurent Vivier

author
#0 lore raw

cover Split the emulator: decode & execute

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/[email protected]>
Signature · 2 lines
------------- Laurent.Vivier-6ktuUTfB/[email protected]  --------------
          "Software is hard" - Donald Knuth

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

_______________________________________________
kvm-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/kvm-devel

A message that is not in the archive (sent off-list or not yet received): 46EF99C1.4070801-6ktuUTfB/[email protected]

Applied
#2 lore raw
Quoted 4 lines · Laurent Vivier

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.

Signature · 7 lines
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/

A message that is not in the archive (sent off-list or not yet received): 46EFA593.2010706-atKUWr5tajBWk0Htik3J/[email protected]

author
#4 lore raw
Quoted 10 lines · Avi Kivity
Quoted 4 lines · Laurent Vivier

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

Quoted 5 lines

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

Signature · 2 lines
------------- Laurent.Vivier-6ktuUTfB/[email protected]  --------------
          "Software is hard" - Donald Knuth

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

_______________________________________________
kvm-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/kvm-devel

author
#5 lore raw

PATCH 1/1 move grp decoding to functions to make x86_emulate_insn() clearer

In reply to a message not in the archive

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/[email protected]>
---
 drivers/kvm/x86_emulate.c |  447 ++++++++++++++++++++++++++-------------------
 1 files changed, 262 insertions(+), 185 deletions(-)
drivers/kvm/x86_emulate.c +262 -185
index 663dc57..b036ff8 100644
@@ -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;
Signature · 7 lines
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/

A message that is not in the archive (sent off-list or not yet received): 11903005973031-git-send-email-Laurent.Vivier-6ktuUTfB/[email protected]

#7 lore raw

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

drivers/kvm/x86_emulate.c done: · line 1008

Quoted 6 lines · Laurent Vivier

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

drivers/kvm/x86_emulate.c +4 -0
+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.

Signature · 7 lines
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/

A message that is not in the archive (sent off-list or not yet received): 46F2B129.9060603-atKUWr5tajBWk0Htik3J/[email protected]

author
#9 lore raw

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

Quoted 10 lines · Avi Kivity

drivers/kvm/x86_emulate.c done: · line 1008

Quoted 6 lines · Laurent Vivier

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

drivers/kvm/x86_emulate.c +4 -0
+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/

A message that is not in the archive (sent off-list or not yet received): 46F2BAD9.1080209-6ktuUTfB/[email protected]

#11 lore raw

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

Quoted 10 lines · Laurent Vivier
Quoted 9 lines · Avi Kivity
Quoted 7 lines · Laurent Vivier

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

drivers/kvm/x86_emulate.c +4 -0
+static inline int emulate_grp45(struct x86_emulate_ctxt *ctxt,
+                   struct x86_emulate_ops *ops,
+                   unsigned long *_eflags,
+                   int *no_wb)
Quoted 7 lines
Quoted 2 lines

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.

Signature · 7 lines
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/

A message that is not in the archive (sent off-list or not yet received): 46F2BB21.2080209-atKUWr5tajBWk0Htik3J/[email protected]

author
#13 lore raw

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

Quoted 14 lines · Avi Kivity
Quoted 13 lines · Laurent Vivier
Quoted 11 lines · Avi Kivity

Review

Quoted 7 lines · Laurent Vivier

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

(quoted diff) +4 -0
+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.

Quoted 6 lines
Quoted 4 lines

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

Quoted 1 line

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

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

Quoted 2 lines

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/

A message that is not in the archive (sent off-list or not yet received): 46F2C053.90207-6ktuUTfB/[email protected]

#15 lore raw

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

Quoted 6 lines · Laurent Vivier
Quoted 6 lines
Quoted 5 lines

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 ?

Quoted 8 lines
Quoted 1 line

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

OK

Quoted 1 line

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

Signature · 7 lines
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/