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

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

[Wei Yongjun]

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

Signed-off-by: Wei Yongjun <yjwei@cn.fujitsu.com>
---
 arch/x86/kvm/emulate.c |   53 +++++++++++++++++++----------------------------
 1 files changed, 22 insertions(+), 31 deletions(-)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index c476a67..8bf80a9 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -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.
-- 
1.7.0.4

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

[Wei Yongjun]

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

Signed-off-by: Wei Yongjun <yjwei@cn.fujitsu.com>
---
 arch/x86/kvm/emulate.c |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 8bf80a9..7c47e37 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -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 :
-- 
1.7.0.4

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

[Paolo Bonzini]

On 08/12/2010 09:41 AM, Wei Yongjun wrote:
> +	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

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

[Wei Yongjun]

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

Signed-off-by: Wei Yongjun <yjwei@cn.fujitsu.com>
---
v1 -> v2: remove implicit LOCK prefix
---
 arch/x86/kvm/emulate.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 8bf80a9..279547a 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -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 :
-- 
1.7.0.4

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

[Wei Yongjun]

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

Signed-off-by: Wei Yongjun <yjwei@cn.fujitsu.com>
---
v2 -> v3: add Lock prefix to decode
---
 arch/x86/kvm/emulate.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 8bf80a9..e091718 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -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 :
-- 
1.7.0.4

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

[Avi Kivity]

 On 08/12/2010 04:38 PM, Wei Yongjun wrote:
> Introduce function write_register_operand() to write back the
> register operand.
>
>
>  
> +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;
> +	}
> +}

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

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

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

[Avi Kivity]

 On 08/16/2010 04:58 AM, Wei Yongjun wrote:
>     
>> It's cleaner to take val and bytes from struct operand, and do the
>> assignment from the callers, no?
>>   
> 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.

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