KVM: x86 emulator: put register operand write back to a function

PATCH Superseded Wei Yongjun 3 participants 7 messages Aug 12, 2010 – Aug 16, 2010 1 message not in the archive

Subsystems (3)

x86 KVM KVM x86

lore

Share & tools

lore

Wei Yongjun

author

2010-08-12 13:40 UTC #0 lore raw

PATCH 1/2 KVM: x86 emulator: put register operand write back to a function

Introduce function write_register_operand() to write back the register operand.

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

---
 arch/x86/kvm/emulate.c |   53 +++++++++++++++++++----------------------------
 1 files changed, 22 insertions(+), 31 deletions(-)

arch/x86/kvm/emulate.c +22 ~~-31~~

index c476a67..8bf80a9 100644
@@ -1020,6 +1020,26 @@ exception:
 	return X86EMUL_PROPAGATE_FAULT;
 }
 
+static void write_register_operand(struct operand *op, unsigned long val,
+				   unsigned int bytes)
+{
+	/* The 4-byte case *is* correct: in 64-bit mode we zero-extend. */
+	switch (bytes) {
+	case 1:
+		*(u8 *)op->addr.reg = (u8)val;
+		break;
+	case 2:
+		*(u16 *)op->addr.reg = (u16)val;
+		break;
+	case 4:
+		*op->addr.reg = (u32)val;
+		break;	/* 64b: zero-extend */
+	case 8:
+		*op->addr.reg = val;
+		break;
+	}
+}
+
 static inline int writeback(struct x86_emulate_ctxt *ctxt,
 			    struct x86_emulate_ops *ops)
 {

@@ -1029,23 +1049,7 @@ static inline int writeback(struct x86_emulate_ctxt *ctxt,
 
 	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.addr.reg = (u8)c->dst.val;
-			break;
-		case 2:
-			*(u16 *)c->dst.addr.reg = (u16)c->dst.val;
-			break;
-		case 4:
-			*c->dst.addr.reg = (u32)c->dst.val;
-			break;	/* 64b: zero-ext */
-		case 8:
-			*c->dst.addr.reg = c->dst.val;
-			break;
-		}
+		write_register_operand(&c->dst, c->dst.val, c->dst.bytes);
 		break;
 	case OP_MEM:
 		if (c->lock_prefix)

@@ -2971,20 +2975,7 @@ special_insn:
 	case 0x86 ... 0x87:	/* xchg */
 	xchg:
 		/* Write back the register source. */
-		switch (c->dst.bytes) {
-		case 1:
-			*(u8 *) c->src.addr.reg = (u8) c->dst.val;
-			break;
-		case 2:
-			*(u16 *) c->src.addr.reg = (u16) c->dst.val;
-			break;
-		case 4:
-			*c->src.addr.reg = (u32) c->dst.val;
-			break;	/* 64b reg: zero-extend */
-		case 8:
-			*c->src.addr.reg = c->dst.val;
-			break;
-		}
+		write_register_operand(&c->src, c->dst.val, c->dst.bytes);
 		/*
 		 * Write back the memory destination with implicit LOCK
 		 * prefix.

Signature · 1 line

1.7.0.4

Wei Yongjun

author

2010-08-12 13:44 UTC #1 lore raw

PATCH 2/2 KVM: x86 emulator: add XADD instruction emulation

Add XADD instruction emulation (opcode 0x0f 0xc0~0xc1)

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

---
 arch/x86/kvm/emulate.c |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)

arch/x86/kvm/emulate.c +8 -1

index 8bf80a9..7c47e37 100644
@@ -2387,7 +2387,8 @@ static struct opcode twobyte_table[256] = {
 	D(DstReg | SrcMem | ModRM), D(DstReg | SrcMem | ModRM),
 	D(ByteOp | DstReg | SrcMem | ModRM | Mov), D(DstReg | SrcMem16 | ModRM | Mov),
 	/* 0xC0 - 0xCF */
-	N, N, N, D(DstMem | SrcReg | ModRM | Mov),
+	D(ByteOp | DstMem | SrcReg | ModRM), D(DstMem | SrcReg | ModRM),
+	N, D(DstMem | SrcReg | ModRM | Mov),
 	N, N, N, GD(0, &group9),
 	N, N, N, N, N, N, N, N,
 	/* 0xD0 - 0xDF */

@@ -3532,6 +3533,12 @@ twobyte_insn:
 		c->dst.val = (c->d & ByteOp) ? (s8) c->src.val :
 							(s16) c->src.val;
 		break;
+	case 0xc0 ... 0xc1:	/* xadd */
+		/* Write back the register source. */
+		write_register_operand(&c->src, c->dst.val, c->dst.bytes);
+		/* Write back the memory destination with implicit LOCK prefix. */
+		c->lock_prefix = 1;
+		goto add;
 	case 0xc3:		/* movnti */
 		c->dst.bytes = c->op_bytes;
 		c->dst.val = (c->op_bytes == 4) ? (u32) c->src.val :

Signature · 1 line

1.7.0.4

Paolo Bonzini

maintainer

2010-08-12 15:34 UTC #2 lore raw

Re: [PATCH 2/2] KVM: x86 emulator: add XADD instruction emulation

arch/x86/kvm/emulate.c twobyte_insn: · line 3540

Quoted 5 lines · Wei Yongjun

arch/x86/kvm/emulate.c +5 -0

+	case 0xc0 ... 0xc1:	/* xadd */
+		/* Write back the register source. */
+		write_register_operand(&c->src, c->dst.val, c->dst.bytes);
+		/* Write back the memory destination with implicit LOCK prefix. */
+		c->lock_prefix = 1;

It's not a major performance problem, but xadd does _not_ have an implicit LOCK prefix.

Paolo

Wei Yongjun

author

2010-08-13 05:16 UTC #3 lore raw

PATCH v2 2/2 KVM: x86 emulator: add XADD instruction emulation

Add XADD instruction emulation (opcode 0x0f 0xc0~0xc1)

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

---
v1 -> v2: remove implicit LOCK prefix

---
 arch/x86/kvm/emulate.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

arch/x86/kvm/emulate.c +6 -1

index 8bf80a9..279547a 100644
@@ -2387,7 +2387,8 @@ static struct opcode twobyte_table[256] = {
 	D(DstReg | SrcMem | ModRM), D(DstReg | SrcMem | ModRM),
 	D(ByteOp | DstReg | SrcMem | ModRM | Mov), D(DstReg | SrcMem16 | ModRM | Mov),
 	/* 0xC0 - 0xCF */
-	N, N, N, D(DstMem | SrcReg | ModRM | Mov),
+	D(ByteOp | DstMem | SrcReg | ModRM), D(DstMem | SrcReg | ModRM),
+	N, D(DstMem | SrcReg | ModRM | Mov),
 	N, N, N, GD(0, &group9),
 	N, N, N, N, N, N, N, N,
 	/* 0xD0 - 0xDF */

@@ -3532,6 +3533,10 @@ twobyte_insn:
 		c->dst.val = (c->d & ByteOp) ? (s8) c->src.val :
 							(s16) c->src.val;
 		break;
+	case 0xc0 ... 0xc1:	/* xadd */
+		/* Write back the register source. */
+		write_register_operand(&c->src, c->dst.val, c->dst.bytes);
+		goto add;
 	case 0xc3:		/* movnti */
 		c->dst.bytes = c->op_bytes;
 		c->dst.val = (c->op_bytes == 4) ? (u32) c->src.val :

Signature · 1 line

1.7.0.4

Wei Yongjun

author

2010-08-13 05:35 UTC #4 lore raw

PATCH v3 2/2 KVM: x86 emulator: add XADD instruction emulation

Add XADD instruction emulation (opcode 0x0f 0xc0~0xc1)

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

---
v2 -> v3: add Lock prefix to decode

---
 arch/x86/kvm/emulate.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

arch/x86/kvm/emulate.c +6 -1

index 8bf80a9..e091718 100644
@@ -2387,7 +2387,8 @@ static struct opcode twobyte_table[256] = {
 	D(DstReg | SrcMem | ModRM), D(DstReg | SrcMem | ModRM),
 	D(ByteOp | DstReg | SrcMem | ModRM | Mov), D(DstReg | SrcMem16 | ModRM | Mov),
 	/* 0xC0 - 0xCF */
-	N, N, N, D(DstMem | SrcReg | ModRM | Mov),
+	D(ByteOp | DstMem | SrcReg | ModRM | Lock), D(DstMem | SrcReg | ModRM | Lock),
+	N, D(DstMem | SrcReg | ModRM | Mov),
 	N, N, N, GD(0, &group9),
 	N, N, N, N, N, N, N, N,
 	/* 0xD0 - 0xDF */

@@ -3532,6 +3533,10 @@ twobyte_insn:
 		c->dst.val = (c->d & ByteOp) ? (s8) c->src.val :
 							(s16) c->src.val;
 		break;
+	case 0xc0 ... 0xc1:	/* xadd */
+		/* Write back the register source. */
+		write_register_operand(&c->src, c->dst.val, c->dst.bytes);
+		goto add;
 	case 0xc3:		/* movnti */
 		c->dst.bytes = c->op_bytes;
 		c->dst.val = (c->op_bytes == 4) ? (u32) c->src.val :

Signature · 1 line

1.7.0.4

Avi Kivity

2010-08-15 16:02 UTC #5 lore raw

In reply to Wei Yongjun

Quoted 14 lines · Wei Yongjun

Introduce function write_register_operand() to write back the register operand.

arch/x86/kvm/emulate.c +9 -0

+static void write_register_operand(struct operand *op, unsigned long val,
+				   unsigned int bytes)
+{
+	/* The 4-byte case *is* correct: in 64-bit mode we zero-extend. */
+	switch (bytes) {
+	case 1:
+		*(u8 *)op->addr.reg = (u8)val;
+		break;
+	case 2:

arch/x86/kvm/emulate.c exception: · line 1041

Quoted 10 lines

arch/x86/kvm/emulate.c +10 -0

+		*(u16 *)op->addr.reg = (u16)val;
+		break;
+	case 4:
+		*op->addr.reg = (u32)val;
+		break;	/* 64b: zero-extend */
+	case 8:
+		*op->addr.reg = val;
+		break;
+	}
+}

It's cleaner to take val and bytes from struct operand, and do the assignment from the callers, no?

Signature · 1 line

error compiling committee.c: too many arguments to function

A message that is not in the archive (sent off-list or not yet received): [email protected]

Avi Kivity

2010-08-16 08:56 UTC #7 lore raw

Quoted 4 lines · Wei Yongjun

Quoted 4 lines
It's cleaner to take val and bytes from struct operand, and do the assignment from the callers, no?

Quoted 9 lines

take val and bytes from struct operand may have other issue, when we writeback
the source register, we need do the assignment from the caller, and then change
the val back before write src val to dst val. Such as xadd:
c->src.val = c->dst.val;
write_register_operand(&c->src);
c->src.val = c->src.orig_val;
goto add;

Or avoid the 'goto add'. XADD is not ADD.

write_register_operand(struct operand *) is easy to understand. With the two additional arguments it becomes confusing since it uses some parts of the operand but ignores others.

Signature · 2 lines

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