KVM: x86 emulator: fix 'mov sreg,rm16' instruction decoding

PATCH Applied Wei Yongjun 3 participants 14 messages Jul 6, 2010 – Jul 7, 2010

Subsystems (3)

x86 KVM KVM x86

lore

Share & tools

lore

Wei Yongjun

author

2010-07-06 08:51 UTC #0 lore raw

PATCH 1/6 KVM: x86 emulator: fix 'mov sreg,rm16' instruction decoding

Memory reads for 'mov sreg,rm16' should be 16 bits only.

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

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

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

index e8bdddc..d842a7d 100644
@@ -170,7 +170,7 @@ static u32 opcode_table[256] = {
 	ByteOp | DstMem | SrcReg | ModRM | Mov, DstMem | SrcReg | ModRM | Mov,
 	ByteOp | DstReg | SrcMem | ModRM | Mov, DstReg | SrcMem | ModRM | Mov,
 	DstMem | SrcReg | ModRM | Mov, ModRM | DstReg,
-	ImplicitOps | SrcMem | ModRM, Group | Group1A,
+	ImplicitOps | SrcMem16 | ModRM, Group | Group1A,
 	/* 0x90 - 0x97 */
 	DstReg, DstReg, DstReg, DstReg,	DstReg, DstReg, DstReg, DstReg,
 	/* 0x98 - 0x9F */

Signature · 1 line

1.7.0.4

Wei Yongjun

author

2010-07-06 08:52 UTC #1 lore raw

PATCH 2/6 KVM: x86 emulator: fix the comment of out instruction

Fix the comment of out instruction, using the same style as the other instructions.

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

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

arch/x86/kvm/emulate.c +2 -2

index d842a7d..ad8d7cd 100644
@@ -2949,8 +2949,8 @@ special_insn:
 				     &c->dst.val))
 			goto done; /* IO is needed */
 		break;
-	case 0xee: /* out al,dx */
-	case 0xef: /* out (e/r)ax,dx */
+	case 0xee: /* out dx,al */
+	case 0xef: /* out dx,(e/r)ax */
 		c->src.val = c->regs[VCPU_REGS_RDX];
 	do_io_out:
 		c->dst.bytes = min(c->dst.bytes, 4u);

Signature · 1 line

1.7.0.4

Wei Yongjun

author

2010-07-06 08:53 UTC #2 lore raw

PATCH 3/6 KVM: x86 emulator: fix 'and AL,imm8' instruction decoding

In reply to Wei Yongjun

'and AL,imm8' should be mask as ByteOp, otherwise the dest operand length will no correct and we may fill the full EAX when writeback.

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

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

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

index ad8d7cd..59568ad 100644
@@ -123,7 +123,7 @@ static u32 opcode_table[256] = {
 	/* 0x20 - 0x27 */
 	ByteOp | DstMem | SrcReg | ModRM | Lock, DstMem | SrcReg | ModRM | Lock,
 	ByteOp | DstReg | SrcMem | ModRM, DstReg | SrcMem | ModRM,
-	DstAcc | SrcImmByte, DstAcc | SrcImm, 0, 0,
+	ByteOp | DstAcc | SrcImmByte, DstAcc | SrcImm, 0, 0,
 	/* 0x28 - 0x2F */
 	ByteOp | DstMem | SrcReg | ModRM | Lock, DstMem | SrcReg | ModRM | Lock,
 	ByteOp | DstReg | SrcMem | ModRM, DstReg | SrcMem | ModRM,

Signature · 1 line

1.7.0.4

Wei Yongjun

author

2010-07-06 08:54 UTC #3 lore raw

PATCH 4/6 KVM: x86 emulator: fix 'mov rm,sreg' instruction decoding

In reply to Wei Yongjun

The source operand of 'mov rm,sreg' is segment register, not general-purpose register, so remove SrcReg from decoding.

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

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

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

index 59568ad..8337567 100644
@@ -169,7 +169,7 @@ static u32 opcode_table[256] = {
 	/* 0x88 - 0x8F */
 	ByteOp | DstMem | SrcReg | ModRM | Mov, DstMem | SrcReg | ModRM | Mov,
 	ByteOp | DstReg | SrcMem | ModRM | Mov, DstReg | SrcMem | ModRM | Mov,
-	DstMem | SrcReg | ModRM | Mov, ModRM | DstReg,
+	DstMem | SrcNone | ModRM | Mov, ModRM | DstReg,
 	ImplicitOps | SrcMem16 | ModRM, Group | Group1A,
 	/* 0x90 - 0x97 */
 	DstReg, DstReg, DstReg, DstReg,	DstReg, DstReg, DstReg, DstReg,

Signature · 1 line

1.7.0.4

Wei Yongjun

author

2010-07-06 08:55 UTC #4 lore raw

PATCH 5/6 KVM: x86 emulator: fix 'mov AL,moffs' instruction decoding

In reply to Wei Yongjun

'mov AL,moffs' do not need decode dest operand and 'mov moffs,AL' do not need decode source operand.

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

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

arch/x86/kvm/emulate.c +2 -2

index 8337567..d4526f2 100644
@@ -177,8 +177,8 @@ static u32 opcode_table[256] = {
 	0, 0, SrcImmFAddr | No64, 0,
 	ImplicitOps | Stack, ImplicitOps | Stack, 0, 0,
 	/* 0xA0 - 0xA7 */
-	ByteOp | DstReg | SrcMem | Mov | MemAbs, DstReg | SrcMem | Mov | MemAbs,
-	ByteOp | DstMem | SrcReg | Mov | MemAbs, DstMem | SrcReg | Mov | MemAbs,
+	ByteOp | SrcMem | Mov | MemAbs, SrcMem | Mov | MemAbs,
+	ByteOp | DstMem | Mov | MemAbs, DstMem | Mov | MemAbs,
 	ByteOp | SrcSI | DstDI | Mov | String, SrcSI | DstDI | Mov | String,
 	ByteOp | SrcSI | DstDI | String, SrcSI | DstDI | String,
 	/* 0xA8 - 0xAF */

Signature · 1 line

1.7.0.4

Avi Kivity

2010-07-06 11:57 UTC #5 lore raw

Re: [PATCH 5/6] KVM: x86 emulator: fix 'mov AL,moffs' instruction decoding

Quoted 6 lines · Wei Yongjun

'mov AL,moffs' do not need decode dest operand and 'mov moffs,AL' do not need decode source operand.

arch/x86/kvm/emulate.c static u32 opcode_table[256] = { · line 184

Quoted 10 lines

arch/x86/kvm/emulate.c +2 -2

@@ -177,8 +177,8 @@ static u32 opcode_table[256] = {
 	0, 0, SrcImmFAddr | No64, 0,
 	ImplicitOps | Stack, ImplicitOps | Stack, 0, 0,
 	/* 0xA0 - 0xA7 */
-	ByteOp | DstReg | SrcMem | Mov | MemAbs, DstReg | SrcMem | Mov | MemAbs,
-	ByteOp | DstMem | SrcReg | Mov | MemAbs, DstMem | SrcReg | Mov | MemAbs,
+	ByteOp | SrcMem | Mov | MemAbs, SrcMem | Mov | MemAbs,
+	ByteOp | DstMem | Mov | MemAbs, DstMem | Mov | MemAbs,
 	ByteOp | SrcSI | DstDI | Mov | String, SrcSI | DstDI | Mov | String,
 	ByteOp | SrcSI | DstDI | String, SrcSI | DstDI | String,
 	/* 0xA8 - 0xAF */

Autotest rejected this patch (Windows XP fails to install). I think the reason is that without DstReg, op->type is not initialized and writeback fails.

I dropped this patch. Suggest re-implementing with DstAcc and (new) SrcAcc.

Signature · 1 line

error compiling committee.c: too many arguments to function

Wei Yongjun

author

2010-07-07 06:27 UTC #6 lore raw

Re: [PATCH 5/6] KVM: x86 emulator: fix 'mov AL,moffs' instruction decoding

Quoted 26 lines · Wei Yongjun

Quoted 8 lines
'mov AL,moffs' do not need decode dest operand and 'mov moffs,AL' do not need decode source operand.
arch/x86/kvm/emulate.c static u32 opcode_table[256] = { · line 184
Quoted 10 lines
arch/x86/kvm/emulate.c +2 -2
@@ -177,8 +177,8 @@ static u32 opcode_table[256] = {
 	0, 0, SrcImmFAddr | No64, 0,
 	ImplicitOps | Stack, ImplicitOps | Stack, 0, 0,
 	/* 0xA0 - 0xA7 */
-	ByteOp | DstReg | SrcMem | Mov | MemAbs, DstReg | SrcMem | Mov | MemAbs,
-	ByteOp | DstMem | SrcReg | Mov | MemAbs, DstMem | SrcReg | Mov | MemAbs,
+	ByteOp | SrcMem | Mov | MemAbs, SrcMem | Mov | MemAbs,
+	ByteOp | DstMem | Mov | MemAbs, DstMem | Mov | MemAbs,
 	ByteOp | SrcSI | DstDI | Mov | String, SrcSI | DstDI | Mov | String,
 	ByteOp | SrcSI | DstDI | String, SrcSI | DstDI | String,
 	/* 0xA8 - 0xAF */
Autotest rejected this patch (Windows XP fails to install). I think the reason is that without DstReg, op->type is not initialized and writeback fails.

Quoted 2 lines

I dropped this patch. Suggest re-implementing with DstAcc and (new) SrcAcc.

sorry about that, I will send a patch to do this.

Wei Yongjun

author

2010-07-07 06:28 UTC #7 lore raw

PATCH 1/1 KVM: x86 emulator: re-implementing 'mov AL,moffs' instruction decoding

In reply to Avi Kivity

This patch change to use DstAcc for decoding 'mov AL, moffs' and introduced SrcAcc for decoding 'mov moffs, AL'.

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

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

arch/x86/kvm/emulate.c +23 -7

index 99fa1c7..87289c2 100644
@@ -70,6 +70,7 @@
 #define SrcSI       (0xa<<4)	/* Source is in the DS:RSI */
 #define SrcImmFAddr (0xb<<4)	/* Source is immediate far address */
 #define SrcMemFAddr (0xc<<4)	/* Source is far address in memory */
+#define SrcAcc      (0xd<<4)	/* Source Accumulator */
 #define SrcMask     (0xf<<4)
 /* Generic ModRM decode. */
 #define ModRM       (1<<8)

@@ -177,8 +178,8 @@ static u32 opcode_table[256] = {
 	0, 0, SrcImmFAddr | No64, 0,
 	ImplicitOps | Stack, ImplicitOps | Stack, 0, 0,
 	/* 0xA0 - 0xA7 */
-	ByteOp | SrcMem | Mov | MemAbs, SrcMem | Mov | MemAbs,
-	ByteOp | DstMem | Mov | MemAbs, DstMem | Mov | MemAbs,
+	ByteOp | DstAcc | SrcMem | Mov | MemAbs, DstAcc | SrcMem | Mov | MemAbs,
+	ByteOp | DstMem | SrcAcc | Mov | MemAbs, DstMem | SrcAcc | Mov | MemAbs,
 	ByteOp | SrcSI | DstDI | Mov | String, SrcSI | DstDI | Mov | String,
 	ByteOp | SrcSI | DstDI | String, SrcSI | DstDI | String,
 	/* 0xA8 - 0xAF */

@@ -1186,6 +1187,25 @@ done_prefixes:
 		else
 			c->src.val = insn_fetch(u8, 1, c->eip);
 		break;
+	case SrcAcc:
+		c->src.type = OP_REG;
+		c->src.bytes = (c->d & ByteOp) ? 1 : c->op_bytes;
+		c->src.ptr = &c->regs[VCPU_REGS_RAX];
+		switch (c->src.bytes) {
+			case 1:
+				c->src.val = *(u8 *)c->src.ptr;
+				break;
+			case 2:
+				c->src.val = *(u16 *)c->src.ptr;
+				break;
+			case 4:
+				c->src.val = *(u32 *)c->src.ptr;
+				break;
+			case 8:
+				c->src.val = *(u64 *)c->src.ptr;
+				break;
+		}
+		break;
 	case SrcOne:
 		c->src.bytes = 1;
 		c->src.val = 1;

@@ -2854,13 +2874,9 @@ special_insn:
 		if (rc != X86EMUL_CONTINUE)
 			goto done;
 		break;
-	case 0xa0 ... 0xa1:	/* mov */
-		c->dst.ptr = (unsigned long *)&c->regs[VCPU_REGS_RAX];
+	case 0xa0 ... 0xa3:	/* mov */
 		c->dst.val = c->src.val;
 		break;
-	case 0xa2 ... 0xa3:	/* mov */
-		c->dst.val = (unsigned long)c->regs[VCPU_REGS_RAX];
-		break;
 	case 0xa4 ... 0xa5:	/* movs */
 		goto mov;
 	case 0xa6 ... 0xa7:	/* cmps */

Signature · 1 line

1.7.0.4

Mohammed Gamal

2010-07-07 09:33 UTC #8 lore raw

Re: [PATCH] KVM: x86 emulator: re-implementing 'mov AL,moffs' instruction decoding

2010/7/7 Wei Yongjun <[email protected]>:

Quoted 69 lines

This patch change to use DstAcc for decoding 'mov AL, moffs' and introduced SrcAcc for decoding 'mov moffs, AL'.
Signed-off-by Wei Yongjun <[email protected]>
---
arch/x86/kvm/emulate.c | 30 +++++++++++++++++++++++-------
 1 files changed, 23 insertions(+), 7 deletions(-)
arch/x86/kvm/emulate.c +3 -2
index 99fa1c7..87289c2 100644
@@ -70,6 +70,7 @@
  #define SrcSI       (0xa<<4)   /* Source is in the DS:RSI */
  #define SrcImmFAddr (0xb<<4)   /* Source is immediate far address */
  #define SrcMemFAddr (0xc<<4)   /* Source is far address in memory */
+#define SrcAcc      (0xd<<4)   /* Source Accumulator */
  #define SrcMask     (0xf<<4)
  /* Generic ModRM decode. */
  #define ModRM       (1<<8)
@@ -177,8 +178,8 @@ static u32 opcode_table[256] = {
        0, 0, SrcImmFAddr | No64, 0,
        ImplicitOps | Stack, ImplicitOps | Stack, 0, 0,
        /* 0xA0 - 0xA7 */
-       ByteOp | SrcMem | Mov | MemAbs, SrcMem | Mov | MemAbs,
-       ByteOp | DstMem | Mov | MemAbs, DstMem | Mov | MemAbs,
+       ByteOp | DstAcc | SrcMem | Mov | MemAbs, DstAcc | SrcMem | Mov | MemAbs,
+       ByteOp | DstMem | SrcAcc | Mov | MemAbs, DstMem | SrcAcc | Mov | MemAbs,
        ByteOp | SrcSI | DstDI | Mov | String, SrcSI | DstDI | Mov | String,
        ByteOp | SrcSI | DstDI | String, SrcSI | DstDI | String,
        /* 0xA8 - 0xAF */
@@ -1186,6 +1187,25 @@ done_prefixes:
else
c->src.val = insn_fetch(u8, 1, c->eip); break;
+ case SrcAcc:
+ c->src.type = OP_REG;
+ c->src.bytes = (c->d & ByteOp) ? 1 : c->op_bytes;
+ c->src.ptr = &c->regs[VCPU_REGS_RAX];
+ switch (c->src.bytes) {
+ case 1:
+ c->src.val = *(u8 *)c->src.ptr;
+ break;
+ case 2:
+ c->src.val = *(u16 *)c->src.ptr;
+ break;
+ case 4:
+ c->src.val = *(u32 *)c->src.ptr;
+ break;
+ case 8:
+ c->src.val = *(u64 *)c->src.ptr;
+ break;
+ }
+ break;
case SrcOne:
c->src.bytes = 1;
c->src.val = 1;
arch/x86/kvm/emulate.c +1 -5
@@ -2854,13 +2874,9 @@ special_insn:
                if (rc != X86EMUL_CONTINUE)
                        goto done;
                break;
-       case 0xa0 ... 0xa1:     /* mov */
-               c->dst.ptr = (unsigned long *)&c->regs[VCPU_REGS_RAX];
+       case 0xa0 ... 0xa3:     /* mov */
                c->dst.val = c->src.val;
                break;
-       case 0xa2 ... 0xa3:     /* mov */
-               c->dst.val = (unsigned long)c->regs[VCPU_REGS_RAX];
-               break;

Quoted 2 lines

case 0xa4 ... 0xa5: /* movs */
goto mov;

Instead of duplicating the mov code, you could've changed the case statement to handle 0xa0...0xa5 which will jump to the "mov" label anyway. The decoder flags will correctly handle src and dst values.

Quoted 10 lines

case 0xa6 ... 0xa7: /* cmps */
--
1.7.0.4
--
To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Wei Yongjun

author

2010-07-07 09:38 UTC #9 lore raw

Re: [PATCH] KVM: x86 emulator: re-implementing 'mov AL,moffs' instruction decoding

Quoted 70 lines

2010/7/7 Wei Yongjun <[email protected]>:

Quoted 67 lines

This patch change to use DstAcc for decoding 'mov AL, moffs' and introduced SrcAcc for decoding 'mov moffs, AL'.

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

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

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

index 99fa1c7..87289c2 100644
@@ -70,6 +70,7 @@
 #define SrcSI       (0xa<<4)   /* Source is in the DS:RSI */
 #define SrcImmFAddr (0xb<<4)   /* Source is immediate far address */
 #define SrcMemFAddr (0xc<<4)   /* Source is far address in memory */
+#define SrcAcc      (0xd<<4)   /* Source Accumulator */
 #define SrcMask     (0xf<<4)
 /* Generic ModRM decode. */
 #define ModRM       (1<<8)

@@ -177,8 +178,8 @@ static u32 opcode_table[256] = {
       0, 0, SrcImmFAddr | No64, 0,
       ImplicitOps | Stack, ImplicitOps | Stack, 0, 0,
       /* 0xA0 - 0xA7 */
-       ByteOp | SrcMem | Mov | MemAbs, SrcMem | Mov | MemAbs,
-       ByteOp | DstMem | Mov | MemAbs, DstMem | Mov | MemAbs,
+       ByteOp | DstAcc | SrcMem | Mov | MemAbs, DstAcc | SrcMem | Mov | MemAbs,
+       ByteOp | DstMem | SrcAcc | Mov | MemAbs, DstMem | SrcAcc | Mov | MemAbs,
       ByteOp | SrcSI | DstDI | Mov | String, SrcSI | DstDI | Mov | String,
       ByteOp | SrcSI | DstDI | String, SrcSI | DstDI | String,
       /* 0xA8 - 0xAF */

@@ -1186,6 +1187,25 @@ done_prefixes:
               else
                       c->src.val = insn_fetch(u8, 1, c->eip);
               break;
+       case SrcAcc:
+               c->src.type = OP_REG;
+               c->src.bytes = (c->d & ByteOp) ? 1 : c->op_bytes;
+               c->src.ptr = &c->regs[VCPU_REGS_RAX];
+               switch (c->src.bytes) {
+                       case 1:
+                               c->src.val = *(u8 *)c->src.ptr;
+                               break;
+                       case 2:
+                               c->src.val = *(u16 *)c->src.ptr;
+                               break;
+                       case 4:
+                               c->src.val = *(u32 *)c->src.ptr;
+                               break;
+                       case 8:
+                               c->src.val = *(u64 *)c->src.ptr;
+                               break;
+               }
+               break;
       case SrcOne:
               c->src.bytes = 1;
               c->src.val = 1;

@@ -2854,13 +2874,9 @@ special_insn:
               if (rc != X86EMUL_CONTINUE)
                       goto done;
               break;
-       case 0xa0 ... 0xa1:     /* mov */
-               c->dst.ptr = (unsigned long *)&c->regs[VCPU_REGS_RAX];
+       case 0xa0 ... 0xa3:     /* mov */
               c->dst.val = c->src.val;
               break;
-       case 0xa2 ... 0xa3:     /* mov */

Quoted 9 lines

arch/x86/kvm/emulate.c special_insn:
Quoted 5 lines
arch/x86/kvm/emulate.c +0 -2
-               c->dst.val = (unsigned long)c->regs[VCPU_REGS_RAX];
-               break;
       case 0xa4 ... 0xa5:     /* movs */
               goto mov;
 
Instead of duplicating the mov code, you could've changed the case statement to handle 0xa0...0xa5 which will jump to the "mov" label anyway. The decoder flags will correctly handle src and dst values.

good idea, I will fix this, thanks.

Quoted 3 lines

Wei Yongjun

author

2010-07-07 09:45 UTC #10 lore raw

PATCH v2 1/1 KVM: x86 emulator: re-implementing 'mov AL,moffs' instruction decoding

In reply to Wei Yongjun

This patch change to use DstAcc for decoding 'mov AL, moffs' and introduced SrcAcc for decoding 'mov moffs, AL'.

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

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

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

index 99fa1c7..255473f 100644
@@ -70,6 +70,7 @@
 #define SrcSI       (0xa<<4)	/* Source is in the DS:RSI */
 #define SrcImmFAddr (0xb<<4)	/* Source is immediate far address */
 #define SrcMemFAddr (0xc<<4)	/* Source is far address in memory */
+#define SrcAcc      (0xd<<4)	/* Source Accumulator */
 #define SrcMask     (0xf<<4)
 /* Generic ModRM decode. */
 #define ModRM       (1<<8)

@@ -177,8 +178,8 @@ static u32 opcode_table[256] = {
 	0, 0, SrcImmFAddr | No64, 0,
 	ImplicitOps | Stack, ImplicitOps | Stack, 0, 0,
 	/* 0xA0 - 0xA7 */
-	ByteOp | SrcMem | Mov | MemAbs, SrcMem | Mov | MemAbs,
-	ByteOp | DstMem | Mov | MemAbs, DstMem | Mov | MemAbs,
+	ByteOp | DstAcc | SrcMem | Mov | MemAbs, DstAcc | SrcMem | Mov | MemAbs,
+	ByteOp | DstMem | SrcAcc | Mov | MemAbs, DstMem | SrcAcc | Mov | MemAbs,
 	ByteOp | SrcSI | DstDI | Mov | String, SrcSI | DstDI | Mov | String,
 	ByteOp | SrcSI | DstDI | String, SrcSI | DstDI | String,
 	/* 0xA8 - 0xAF */

@@ -1186,6 +1187,25 @@ done_prefixes:
 		else
 			c->src.val = insn_fetch(u8, 1, c->eip);
 		break;
+	case SrcAcc:
+		c->src.type = OP_REG;
+		c->src.bytes = (c->d & ByteOp) ? 1 : c->op_bytes;
+		c->src.ptr = &c->regs[VCPU_REGS_RAX];
+		switch (c->src.bytes) {
+			case 1:
+				c->src.val = *(u8 *)c->src.ptr;
+				break;
+			case 2:
+				c->src.val = *(u16 *)c->src.ptr;
+				break;
+			case 4:
+				c->src.val = *(u32 *)c->src.ptr;
+				break;
+			case 8:
+				c->src.val = *(u64 *)c->src.ptr;
+				break;
+		}
+		break;
 	case SrcOne:
 		c->src.bytes = 1;
 		c->src.val = 1;

@@ -2854,13 +2874,7 @@ special_insn:
 		if (rc != X86EMUL_CONTINUE)
 			goto done;
 		break;
-	case 0xa0 ... 0xa1:	/* mov */
-		c->dst.ptr = (unsigned long *)&c->regs[VCPU_REGS_RAX];
-		c->dst.val = c->src.val;
-		break;
-	case 0xa2 ... 0xa3:	/* mov */
-		c->dst.val = (unsigned long)c->regs[VCPU_REGS_RAX];
-		break;
+	case 0xa0 ... 0xa3:	/* mov */
 	case 0xa4 ... 0xa5:	/* movs */
 		goto mov;
 	case 0xa6 ... 0xa7:	/* cmps */

Signature · 1 line

1.7.0.4

Avi Kivity

Applied

2010-07-07 09:50 UTC #11 lore raw

Re: [PATCHv2] KVM: x86 emulator: re-implementing 'mov AL,moffs' instruction decoding

Quoted 4 lines · Wei Yongjun

This patch change to use DstAcc for decoding 'mov AL, moffs' and introduced SrcAcc for decoding 'mov moffs, AL'.

Applied, thanks.

Signature · 2 lines

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

Wei Yongjun

author

2010-07-06 08:56 UTC #12 lore raw

PATCH 6/6 KVM: x86 emulator: fix cli/sti instruction emulation

In reply to Wei Yongjun

If IOPL check fail, the cli/sti emulate GP and then we should skip writeback since the default write OP is OP_REG.

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

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

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

index d4526f2..99fa1c7 100644
@@ -2979,17 +2979,19 @@ special_insn:
 		c->dst.type = OP_NONE;	/* Disable writeback. */
 		break;
 	case 0xfa: /* cli */
-		if (emulator_bad_iopl(ctxt, ops))
+		if (emulator_bad_iopl(ctxt, ops)) {
 			emulate_gp(ctxt, 0);
-		else {
+			goto done;
+		} else {
 			ctxt->eflags &= ~X86_EFLAGS_IF;
 			c->dst.type = OP_NONE;	/* Disable writeback. */
 		}
 		break;
 	case 0xfb: /* sti */
-		if (emulator_bad_iopl(ctxt, ops))
+		if (emulator_bad_iopl(ctxt, ops)) {
 			emulate_gp(ctxt, 0);
-		else {
+			goto done;
+		} else {
 			ctxt->interruptibility = KVM_X86_SHADOW_INT_STI;
 			ctxt->eflags |= X86_EFLAGS_IF;
 			c->dst.type = OP_NONE;	/* Disable writeback. */

Signature · 1 line

1.7.0.4

Avi Kivity

Applied

2010-07-06 09:23 UTC #13 lore raw

In reply to Wei Yongjun

Quoted 3 lines · Wei Yongjun

Memory reads for 'mov sreg,rm16' should be 16 bits only.

Applied all 6, thanks.

Signature · 1 line

error compiling committee.c: too many arguments to function