[PATCH 0/9] KVM: x86: Fixes for various emulator bugs

[PATCH 0/9] KVM: x86: Fixes for various emulator bugs

[Nadav Amit]

The x86 emulator of KVM is buggy. This series of patches includes fixes for
various bugs which were detected. Each patch stands on its own.  Two patches do
not fix KVM emulation, but cause the emulator to fail more nicely by returning
an unhandlable error, instead of performing wrong emulation (VEX-prefix and
cmpxchg16b).  The fix for rdpmc is a bit intrusive to keep SVM behavior intact.

Thanks for reviewing the patches.

Nadav Amit (9):
  KVM: x86: Mark VEX-prefix instructions emulation as unimplemented
  KVM: x86: Emulator ignores LDTR/TR extended base on LLDT/LTR
  KVM: x86: Loading segments on 64-bit mode may be wrong
  KVM: x86: sgdt and sidt are not privilaged
  KVM: x86: cmpxchg emulation should compare in reverse order
  KVM: x86: movnti minimum op size of 32-bit is not kept
  KVM: x86: rdpmc emulation checks the counter incorrectly
  KVM: x86: Return error on cmpxchg16b emulation
  KVM: x86: smsw emulation is incorrect in 64-bit mode

 arch/x86/include/asm/kvm_emulate.h |  1 +
 arch/x86/include/asm/kvm_host.h    |  1 +
 arch/x86/kvm/emulate.c             | 44 ++++++++++++++++++++++++++++----------
 arch/x86/kvm/pmu.c                 |  9 ++++++++
 arch/x86/kvm/x86.c                 |  7 ++++++
 5 files changed, 51 insertions(+), 11 deletions(-)

-- 
1.9.1

[PATCH 1/9] KVM: x86: Mark VEX-prefix instructions emulation as unimplemented

[Nadav Amit]

Currently the emulator does not recognize vex-prefix instructions.  However, it
may incorrectly decode lgdt/lidt instructions and try to execute them. This
patch returns unhandlable error on their emulation.

Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
---
 arch/x86/kvm/emulate.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index e4e833d..8ec4a3e 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -4314,6 +4314,13 @@ done_prefixes:
 	if (ctxt->d & ModRM)
 		ctxt->modrm = insn_fetch(u8, ctxt);
 
+	/* vex-prefix instructions are not implemented */
+	if (ctxt->opcode_len == 1 && (ctxt->b == 0xc5 || ctxt->b == 0xc4) &&
+	    (mode == X86EMUL_MODE_PROT64 ||
+	    (mode >= X86EMUL_MODE_PROT16 && (ctxt->modrm & 0x80)))) {
+		ctxt->d = NotImpl;
+	}
+
 	while (ctxt->d & GroupMask) {
 		switch (ctxt->d & GroupMask) {
 		case Group:
-- 
1.9.1

[PATCH 2/9] KVM: x86: Emulator ignores LDTR/TR extended base on LLDT/LTR

[Nadav Amit]

The current implementation ignores the LDTR/TR base high 32-bits on long-mode.
As a result the loaded segment descriptor may be incorrect.

Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
---
 arch/x86/kvm/emulate.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 8ec4a3e..136088f 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -1422,6 +1422,7 @@ static int __load_segment_descriptor(struct x86_emulate_ctxt *ctxt,
 	ulong desc_addr;
 	int ret;
 	u16 dummy;
+	u32 base3 = 0;
 
 	memset(&seg_desc, 0, sizeof seg_desc);
 
@@ -1538,9 +1539,14 @@ static int __load_segment_descriptor(struct x86_emulate_ctxt *ctxt,
 		ret = write_segment_descriptor(ctxt, selector, &seg_desc);
 		if (ret != X86EMUL_CONTINUE)
 			return ret;
+	} else if (ctxt->mode == X86EMUL_MODE_PROT64) {
+		ret = ctxt->ops->read_std(ctxt, desc_addr+8, &base3,
+				sizeof(base3), &ctxt->exception);
+		if (ret != X86EMUL_CONTINUE)
+			return ret;
 	}
 load:
-	ctxt->ops->set_segment(ctxt, selector, &seg_desc, 0, seg);
+	ctxt->ops->set_segment(ctxt, selector, &seg_desc, base3, seg);
 	return X86EMUL_CONTINUE;
 exception:
 	emulate_exception(ctxt, err_vec, err_code, true);
-- 
1.9.1

[PATCH 3/9] KVM: x86: Loading segments on 64-bit mode may be wrong

[Nadav Amit]

The current emulator implementation ignores the high 32 bits of the base in
long-mode.  During segment load from the LDT, the base of the LDT is calculated
incorrectly and may cause the wrong segment to be loaded.

Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
---
 arch/x86/kvm/emulate.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 136088f..7e4a45c 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -1358,17 +1358,19 @@ static void get_descriptor_table_ptr(struct x86_emulate_ctxt *ctxt,
 				     u16 selector, struct desc_ptr *dt)
 {
 	const struct x86_emulate_ops *ops = ctxt->ops;
+	u32 base3 = 0;
 
 	if (selector & 1 << 2) {
 		struct desc_struct desc;
 		u16 sel;
 
 		memset (dt, 0, sizeof *dt);
-		if (!ops->get_segment(ctxt, &sel, &desc, NULL, VCPU_SREG_LDTR))
+		if (!ops->get_segment(ctxt, &sel, &desc, &base3,
+				      VCPU_SREG_LDTR))
 			return;
 
 		dt->size = desc_limit_scaled(&desc); /* what if limit > 65535? */
-		dt->address = get_desc_base(&desc);
+		dt->address = get_desc_base(&desc) | ((u64)base3 << 32);
 	} else
 		ops->get_gdt(ctxt, dt);
 }
-- 
1.9.1

[PATCH 4/9] KVM: x86: sgdt and sidt are not privilaged

[Nadav Amit]

The SGDT and SIDT instructions are not privilaged, i.e. they can be executed
with CPL>0.

Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
---
 arch/x86/kvm/emulate.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 7e4a45c..a16bf22 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -3642,8 +3642,8 @@ static const struct opcode group6[] = {
 };
 
 static const struct group_dual group7 = { {
-	II(Mov | DstMem | Priv,			em_sgdt, sgdt),
-	II(Mov | DstMem | Priv,			em_sidt, sidt),
+	II(Mov | DstMem,			em_sgdt, sgdt),
+	II(Mov | DstMem,			em_sidt, sidt),
 	II(SrcMem | Priv,			em_lgdt, lgdt),
 	II(SrcMem | Priv,			em_lidt, lidt),
 	II(SrcNone | DstMem | Mov,		em_smsw, smsw), N,
-- 
1.9.1

[PATCH 5/9] KVM: x86: cmpxchg emulation should compare in reverse order

[Nadav Amit]

The current implementation of cmpxchg does not update the flags correctly,
since the accumulator should be compared with the destination and not the other
way around. The current implementation does not update the flags correctly.

Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
---
 arch/x86/kvm/emulate.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index a16bf22..4cb0da6 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -2052,8 +2052,10 @@ static int em_ret_far_imm(struct x86_emulate_ctxt *ctxt)
 static int em_cmpxchg(struct x86_emulate_ctxt *ctxt)
 {
 	/* Save real source value, then compare EAX against destination. */
+	ctxt->dst.orig_val = ctxt->dst.val;
+	ctxt->dst.val = reg_read(ctxt, VCPU_REGS_RAX);
 	ctxt->src.orig_val = ctxt->src.val;
-	ctxt->src.val = reg_read(ctxt, VCPU_REGS_RAX);
+	ctxt->src.val = ctxt->dst.orig_val;
 	fastop(ctxt, em_cmp);
 
 	if (ctxt->eflags & EFLG_ZF) {
@@ -2063,6 +2065,7 @@ static int em_cmpxchg(struct x86_emulate_ctxt *ctxt)
 		/* Failure: write the value we saw to EAX. */
 		ctxt->dst.type = OP_REG;
 		ctxt->dst.addr.reg = reg_rmw(ctxt, VCPU_REGS_RAX);
+		ctxt->dst.val = ctxt->dst.orig_val;
 	}
 	return X86EMUL_CONTINUE;
 }
-- 
1.9.1

[PATCH 6/9] KVM: x86: movnti minimum op size of 32-bit is not kept

[Nadav Amit]

If the operand-size prefix (0x66) is used in 64-bit mode, the emulator would
assume the destination operand is 64-bit, when it should be 32-bit.

Reminder: movnti does not support 16-bit operands and its default operand size
is 32-bit.

Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
---
 arch/x86/kvm/emulate.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 4cb0da6..be3f764 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -4836,8 +4836,8 @@ twobyte_insn:
 		break;
 	case 0xc3:		/* movnti */
 		ctxt->dst.bytes = ctxt->op_bytes;
-		ctxt->dst.val = (ctxt->op_bytes == 4) ? (u32) ctxt->src.val :
-							(u64) ctxt->src.val;
+		ctxt->dst.val = (ctxt->op_bytes == 8) ? (u64) ctxt->src.val :
+							(u32) ctxt->src.val;
 		break;
 	default:
 		goto cannot_emulate;
-- 
1.9.1

[PATCH 7/9] KVM: x86: rdpmc emulation checks the counter incorrectly

[Nadav Amit]

The rdpmc emulation checks that the counter (ECX) is not higher than 2, without
taking into considerations bits 30:31 role (e.g., bit 30 marks whether the
counter is fixed). The fix uses the pmu information for checking the validity
of the pmu counter.

Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
---
 arch/x86/include/asm/kvm_emulate.h | 1 +
 arch/x86/include/asm/kvm_host.h    | 1 +
 arch/x86/kvm/emulate.c             | 2 +-
 arch/x86/kvm/pmu.c                 | 9 +++++++++
 arch/x86/kvm/x86.c                 | 7 +++++++
 5 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/kvm_emulate.h b/arch/x86/include/asm/kvm_emulate.h
index a04fe4e..ffa2671 100644
--- a/arch/x86/include/asm/kvm_emulate.h
+++ b/arch/x86/include/asm/kvm_emulate.h
@@ -194,6 +194,7 @@ struct x86_emulate_ops {
 	int (*set_dr)(struct x86_emulate_ctxt *ctxt, int dr, ulong value);
 	int (*set_msr)(struct x86_emulate_ctxt *ctxt, u32 msr_index, u64 data);
 	int (*get_msr)(struct x86_emulate_ctxt *ctxt, u32 msr_index, u64 *pdata);
+	int (*check_pmc)(struct x86_emulate_ctxt *ctxt, u32 pmc);
 	int (*read_pmc)(struct x86_emulate_ctxt *ctxt, u32 pmc, u64 *pdata);
 	void (*halt)(struct x86_emulate_ctxt *ctxt);
 	void (*wbinvd)(struct x86_emulate_ctxt *ctxt);
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 4931415..63e020b 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1070,6 +1070,7 @@ void kvm_pmu_cpuid_update(struct kvm_vcpu *vcpu);
 bool kvm_pmu_msr(struct kvm_vcpu *vcpu, u32 msr);
 int kvm_pmu_get_msr(struct kvm_vcpu *vcpu, u32 msr, u64 *data);
 int kvm_pmu_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info);
+int kvm_pmu_check_pmc(struct kvm_vcpu *vcpu, unsigned pmc);
 int kvm_pmu_read_pmc(struct kvm_vcpu *vcpu, unsigned pmc, u64 *data);
 void kvm_handle_pmu_event(struct kvm_vcpu *vcpu);
 void kvm_deliver_pmi(struct kvm_vcpu *vcpu);
diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index be3f764..3da8d82 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -3507,7 +3507,7 @@ static int check_rdpmc(struct x86_emulate_ctxt *ctxt)
 	u64 rcx = reg_read(ctxt, VCPU_REGS_RCX);
 
 	if ((!(cr4 & X86_CR4_PCE) && ctxt->ops->cpl(ctxt)) ||
-	    (rcx > 3))
+	    ctxt->ops->check_pmc(ctxt, rcx))
 		return emulate_gp(ctxt, 0);
 
 	return X86EMUL_CONTINUE;
diff --git a/arch/x86/kvm/pmu.c b/arch/x86/kvm/pmu.c
index cbecaa9..3dd6acc 100644
--- a/arch/x86/kvm/pmu.c
+++ b/arch/x86/kvm/pmu.c
@@ -428,6 +428,15 @@ int kvm_pmu_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 	return 1;
 }
 
+int kvm_pmu_check_pmc(struct kvm_vcpu *vcpu, unsigned pmc)
+{
+	struct kvm_pmu *pmu = &vcpu->arch.pmu;
+	bool fixed = pmc & (1u << 30);
+	pmc &= ~(3u << 30);
+	return (!fixed && pmc >= pmu->nr_arch_gp_counters) ||
+		(fixed && pmc >= pmu->nr_arch_fixed_counters);
+}
+
 int kvm_pmu_read_pmc(struct kvm_vcpu *vcpu, unsigned pmc, u64 *data)
 {
 	struct kvm_pmu *pmu = &vcpu->arch.pmu;
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 57eac30..2c6fccd 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -4759,6 +4759,12 @@ static int emulator_set_msr(struct x86_emulate_ctxt *ctxt,
 	return kvm_set_msr(emul_to_vcpu(ctxt), &msr);
 }
 
+static int emulator_check_pmc(struct x86_emulate_ctxt *ctxt,
+			      u32 pmc)
+{
+	return kvm_pmu_check_pmc(emul_to_vcpu(ctxt), pmc);
+}
+
 static int emulator_read_pmc(struct x86_emulate_ctxt *ctxt,
 			     u32 pmc, u64 *pdata)
 {
@@ -4835,6 +4841,7 @@ static const struct x86_emulate_ops emulate_ops = {
 	.set_dr              = emulator_set_dr,
 	.set_msr             = emulator_set_msr,
 	.get_msr             = emulator_get_msr,
+	.check_pmc	     = emulator_check_pmc,
 	.read_pmc            = emulator_read_pmc,
 	.halt                = emulator_halt,
 	.wbinvd              = emulator_wbinvd,
-- 
1.9.1

[PATCH 8/9] KVM: x86: Return error on cmpxchg16b emulation

[Nadav Amit]

cmpxchg16b is currently unimplemented in the emulator. The least we can do is
return error upon the emulation of this instruction.

Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
---
 arch/x86/kvm/emulate.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 3da8d82..a151f8d 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -1999,6 +1999,9 @@ static int em_cmpxchg8b(struct x86_emulate_ctxt *ctxt)
 {
 	u64 old = ctxt->dst.orig_val64;
 
+	if (ctxt->dst.bytes == 16)
+		return X86EMUL_UNHANDLEABLE;
+
 	if (((u32) (old >> 0) != (u32) reg_read(ctxt, VCPU_REGS_RAX)) ||
 	    ((u32) (old >> 32) != (u32) reg_read(ctxt, VCPU_REGS_RDX))) {
 		*reg_write(ctxt, VCPU_REGS_RAX) = (u32) (old >> 0);
@@ -4077,7 +4080,7 @@ static int decode_operand(struct x86_emulate_ctxt *ctxt, struct operand *op,
 		op->orig_val = op->val;
 		break;
 	case OpMem64:
-		ctxt->memop.bytes = 8;
+		ctxt->memop.bytes = (ctxt->op_bytes == 8) ? 16 : 8;
 		goto mem_common;
 	case OpAcc:
 		op->type = OP_REG;
-- 
1.9.1

[PATCH 9/9] KVM: x86: smsw emulation is incorrect in 64-bit mode

[Nadav Amit]

In 64-bit mode, when the destination is a register, the assignment is done
according to the operand size. Otherwise (memory operand or no 64-bit mode), a
16-bit assignment is performed.

Currently, 16-bit assignment is always done to the destination.

Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
---
 arch/x86/kvm/emulate.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index a151f8d..9b5d97d 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -3235,7 +3235,8 @@ static int em_lidt(struct x86_emulate_ctxt *ctxt)
 
 static int em_smsw(struct x86_emulate_ctxt *ctxt)
 {
-	ctxt->dst.bytes = 2;
+	if (ctxt->dst.type == OP_MEM)
+		ctxt->dst.bytes = 2;
 	ctxt->dst.val = ctxt->ops->get_cr(ctxt, 0);
 	return X86EMUL_CONTINUE;
 }
-- 
1.9.1

Re: [PATCH 9/9] KVM: x86: smsw emulation is incorrect in 64-bit mode

[Paolo Bonzini]

Il 02/06/2014 17:34, Nadav Amit ha scritto:
> In 64-bit mode, when the destination is a register, the assignment is done
> according to the operand size. Otherwise (memory operand or no 64-bit mode), a
> 16-bit assignment is performed.

I'm sorry, I'm missing the place where 64-bit mode is taken into account?

Paolo

Re: [PATCH 9/9] KVM: x86: smsw emulation is incorrect in 64-bit mode

[Nadav Amit]

On Jun 5, 2014, at 5:53 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:

> Il 02/06/2014 17:34, Nadav Amit ha scritto:
>> In 64-bit mode, when the destination is a register, the assignment is done
>> according to the operand size. Otherwise (memory operand or no 64-bit mode), a
>> 16-bit assignment is performed.
> 
> I'm sorry, I'm missing the place where 64-bit mode is taken into account?
It is not, since on 32-bit mode the high-order 16 bits of a register destination are undefined. 
If I recall correctly, in this case the high-order 16-bits on native system actually reflect the high-order 16-bits of CR0.

Therefore, regardless to the mode of operation:
1. If the target is memory - the destination is 16-bit
2. If the target is a register - the register is written according to the operand size.

Nadav

Re: [PATCH 9/9] KVM: x86: smsw emulation is incorrect in 64-bit mode

[H. Peter Anvin]

On 06/05/2014 08:02 AM, Nadav Amit wrote:
>> I'm sorry, I'm missing the place where 64-bit mode is taken into account?
> It is not, since on 32-bit mode the high-order 16 bits of a register destination are undefined. 
> If I recall correctly, in this case the high-order 16-bits on native
system actually reflect the high-order 16-bits of CR0.

This sounds like something that really should be verified
experimentally.  The above claim seems... odd.

	-hpa

Re: [PATCH 9/9] KVM: x86: smsw emulation is incorrect in 64-bit mode

[Paolo Bonzini]

Il 05/06/2014 17:04, H. Peter Anvin ha scritto:
> On 06/05/2014 08:02 AM, Nadav Amit wrote:
>>> I'm sorry, I'm missing the place where 64-bit mode is taken into account?
>> It is not, since on 32-bit mode the high-order 16 bits of a register destination are undefined.
>> If I recall correctly, in this case the high-order 16-bits on native
> system actually reflect the high-order 16-bits of CR0.
>
> This sounds like something that really should be verified
> experimentally.  The above claim seems... odd.

Here is a test program:

#if __SIZEOF_LONG__ == 4
#define V "12345678"
#define R "e"
#else
#define V "1234567812345678"
#define R "r"
#endif

#include <stdio.h>
int main()
{
	register volatile unsigned long ecx asm("ecx");

#if __SIZEOF_LONG__ > 4
	asm volatile("mov $0x" V ", %%" R "cx; smswq %%rcx": : :"ecx");
	printf("smswq: %lx\n", ecx);
#endif

	asm volatile("mov $0x" V ", %%" R "cx; smswl %%ecx": : :"ecx");
	printf("smswl: %lx\n", ecx);

	asm volatile("mov $0x" V ", %%" R "cx; smsww %%cx": : :"ecx");
	printf("smsww: %lx\n", ecx);
}

Output in 32-bit mode:
smswq: 80050033
smswl: 12340033

Output in 64-bit mode:
smswq: 80050033
smswl: 80050033
smsww: 1234567812340033

Can you please make a test case for kvm-unit-tests (x86/emulator.c), in 
order to check the validity of the patch?

Paolo

[PATCH kvm-unit-tests 0/2] x86: Additional smsw tests

[Nadav Amit]

This patch set adds two tests for smsw. The first one is intended to add
coverage of smsw. It covers the case smsw is executed with memory operand in a
page which is write-protected by the hypervisor. Note that the existing smsw
tests are not supposed to be trapped by the hypervisor. This test was added
just for additional coverage.

The realmode smsw test covers the recent patch that saves the high 16-bits to
32-bit register operand. Implementing a long-mode test is difficult since we
need to cause an "invalid guest state" in long-mode.

Nadav Amit (2):
  x86: emulator: additional smsw test-case
  x86: realmode: test smsw behavior with register operand

 x86/emulator.c | 10 ++++++++--
 x86/realmode.c | 15 +++++++++++++++
 2 files changed, 23 insertions(+), 2 deletions(-)

-- 
1.9.1

[PATCH kvm-unit-tests1/2] x86: emulator: additional smsw test-case

[Nadav Amit]

An additional test case for the emulator was added to test smsw which is
trapped by the emulator.  The other existing test-cases occur in the guest (at
least on VMX), since the values are read directly from the CR0 read shadow.

Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
---
 x86/emulator.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/x86/emulator.c b/x86/emulator.c
index 20e3a45..033f246 100644
--- a/x86/emulator.c
+++ b/x86/emulator.c
@@ -336,7 +336,7 @@ void test_incdecnotneg(void *mem)
     report("lock notb", *mb == vb);
 }
 
-void test_smsw(void)
+void test_smsw(uint64_t *h_mem)
 {
 	char mem[16];
 	unsigned short msw, msw_orig, *pmsw;
@@ -355,6 +355,12 @@ void test_smsw(void)
 		if (i != 4 && pmsw[i])
 			zero = 0;
 	report("smsw (2)", msw == pmsw[4] && zero);
+
+	/* Trigger exit on smsw */
+	*h_mem = 0x12345678abcdeful;
+	asm volatile("smsw %0" : "=m"(*h_mem));
+	report("smsw (3)", msw == (unsigned short)*h_mem &&
+		(*h_mem & ~0xfffful) == 0x12345678ab0000ul);
 }
 
 void test_lmsw(void)
@@ -998,7 +1004,7 @@ int main()
 
 	test_cr8();
 
-	test_smsw();
+	test_smsw(mem);
 	test_lmsw();
 	test_ljmp(mem);
 	test_stringio();
-- 
1.9.1

Re: [PATCH kvm-unit-tests1/2] x86: emulator: additional smsw test-case

[Paolo Bonzini]

Il 06/06/2014 01:56, Nadav Amit ha scritto:
> An additional test case for the emulator was added to test smsw which is
> trapped by the emulator.  The other existing test-cases occur in the guest (at
> least on VMX), since the values are read directly from the CR0 read shadow.
>
> Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
> ---
>  x86/emulator.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/x86/emulator.c b/x86/emulator.c
> index 20e3a45..033f246 100644
> --- a/x86/emulator.c
> +++ b/x86/emulator.c
> @@ -336,7 +336,7 @@ void test_incdecnotneg(void *mem)
>      report("lock notb", *mb == vb);
>  }
>
> -void test_smsw(void)
> +void test_smsw(uint64_t *h_mem)
>  {
>  	char mem[16];
>  	unsigned short msw, msw_orig, *pmsw;
> @@ -355,6 +355,12 @@ void test_smsw(void)
>  		if (i != 4 && pmsw[i])
>  			zero = 0;
>  	report("smsw (2)", msw == pmsw[4] && zero);
> +
> +	/* Trigger exit on smsw */
> +	*h_mem = 0x12345678abcdeful;
> +	asm volatile("smsw %0" : "=m"(*h_mem));
> +	report("smsw (3)", msw == (unsigned short)*h_mem &&
> +		(*h_mem & ~0xfffful) == 0x12345678ab0000ul);
>  }
>
>  void test_lmsw(void)
> @@ -998,7 +1004,7 @@ int main()
>
>  	test_cr8();
>
> -	test_smsw();
> +	test_smsw(mem);
>  	test_lmsw();
>  	test_ljmp(mem);
>  	test_stringio();
>

Thanks, applying all after the end of the merge window.

Paolo

[PATCH kvm-unit-tests 2/2] x86: realmode: test smsw behavior with register operand

[Nadav Amit]

The smsw instruction has an undocumented behavior, in which the high-order
16-bits of CR0 are also saved in a 32-bit destination register.  This is
similar to the way smsw behaves in long-mode. However, it is hard to test the
long-mode case, since we need to cause an "invalid guest state" in long-mode.

The test works as follows: it sets CR0.CD (bit 30), so any of the high 16-bits
would be set.  It then executes smsw to register destination and compares the
register value with that of CR0.  CR0 value is restored when the test is done.

This test is expected to fail only when unrestricted mode is disabled or
unsupported.

Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
---
 x86/realmode.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/x86/realmode.c b/x86/realmode.c
index 839ac34..6e74883 100644
--- a/x86/realmode.c
+++ b/x86/realmode.c
@@ -1646,6 +1646,20 @@ void test_dr_mod(void)
 	report("mov dr with mod bits", R_AX | R_BX, outregs.eax == 0xaced);
 }
 
+void test_smsw(void)
+{
+	MK_INSN(smsw, "movl %cr0, %ebx\n\t"
+		      "movl %ebx, %ecx\n\t"
+		      "or $0x40000000, %ebx\n\t"
+		      "movl %ebx, %cr0\n\t"
+		      "smswl %eax\n\t"
+		      "movl %ecx, %cr0\n\t");
+	inregs.eax = 0x12345678;
+	exec_in_big_real_mode(&insn_smsw);
+	report("smsw", R_AX | R_BX | R_CX, outregs.eax == outregs.ebx);
+}
+
+
 void realmode_start(void)
 {
 	test_null();
@@ -1692,6 +1706,7 @@ void realmode_start(void)
 	test_salc();
 	test_fninit();
 	test_dr_mod();
+	test_smsw();
 	test_nopl();
 	test_perf_loop();
 	test_perf_mov();
-- 
1.9.1

Re: [PATCH kvm-unit-tests 0/2] x86: Additional smsw tests

[Paolo Bonzini]

Il 06/06/2014 01:56, Nadav Amit ha scritto:
> This patch set adds two tests for smsw. The first one is intended to add
> coverage of smsw. It covers the case smsw is executed with memory operand in a
> page which is write-protected by the hypervisor. Note that the existing smsw
> tests are not supposed to be trapped by the hypervisor. This test was added
> just for additional coverage.
>
> The realmode smsw test covers the recent patch that saves the high 16-bits to
> 32-bit register operand. Implementing a long-mode test is difficult since we
> need to cause an "invalid guest state" in long-mode.

You can use emulator.flat to test the emulator in long mode.

See test_movabs for an example.

Paolo

> Nadav Amit (2):
>   x86: emulator: additional smsw test-case
>   x86: realmode: test smsw behavior with register operand
>
>  x86/emulator.c | 10 ++++++++--
>  x86/realmode.c | 15 +++++++++++++++
>  2 files changed, 23 insertions(+), 2 deletions(-)
>

[PATCH kvm-unit-tests v2] x86: emulator: long mode smsw tests

[Nadav Amit]

SMSW instruction on long-mode is performed according to the operand size, if
the destination operand is a register.  This patch tests whether it is
performed correctly instead of always using two-bytes operands (as KVM
previously did).  Note that when a dword destination operand is used, the
result is zero-extended to qword on long-mode.

Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
---
 x86/emulator.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/x86/emulator.c b/x86/emulator.c
index 033f246..f653127 100644
--- a/x86/emulator.c
+++ b/x86/emulator.c
@@ -825,6 +825,26 @@ static void test_movabs(uint64_t *mem, uint8_t *insn_page,
     report("64-bit mov imm2", outregs.rcx == 0x9090909090909090);
 }
 
+static void test_smsw_reg(uint64_t *mem, uint8_t *insn_page,
+		      uint8_t *alt_insn_page, void *insn_ram)
+{
+	unsigned long cr0 = read_cr0();
+	inregs = (struct regs){ .rax = 0x1234567890abcdeful };
+
+	MK_INSN(smsww, "smsww %ax\n\t");
+	trap_emulator(mem, alt_insn_page, &insn_smsww);
+	report("16-bit smsw reg", (u16)outregs.rax == (u16)cr0 &&
+				  outregs.rax >> 16 == inregs.rax >> 16);
+
+	MK_INSN(smswl, "smswl %eax\n\t");
+	trap_emulator(mem, alt_insn_page, &insn_smswl);
+	report("32-bit smsw reg", outregs.rax == (u32)cr0);
+
+	MK_INSN(smswq, "smswq %rax\n\t");
+	trap_emulator(mem, alt_insn_page, &insn_smswq);
+	report("64-bit smsw reg", outregs.rax == cr0);
+}
+
 static void test_crosspage_mmio(volatile uint8_t *mem)
 {
     volatile uint16_t w, *pw;
@@ -1024,6 +1044,7 @@ int main()
 
 	test_mmx_movq_mf(mem, insn_page, alt_insn_page, insn_ram);
 	test_movabs(mem, insn_page, alt_insn_page, insn_ram);
+	test_smsw_reg(mem, insn_page, alt_insn_page, insn_ram);
 
 	test_crosspage_mmio(mem);
 
-- 
1.9.1