[kvm-unit-tests PATCH 00/14] x86: MSR_GS_BASE and friends

[kvm-unit-tests PATCH 00/14] x86: MSR_GS_BASE and friends

[Sean Christopherson]

Fix unit tests on 32-bit SVM, and on VMX if/when 32-bit VMX starts
rejecting MSR_GS_BASE accesses.

Fix a variety of semi-related bugs in the MSR test, rework the test to
make the code maintainable, and expand its test coverage.

Sean Christopherson (14):
  x86/cstart: Don't use MSR_GS_BASE in 32-bit boot code
  x86: msr: Exclude GS/FS_BASE MSRs from 32-bit builds
  x86: msr: Advertise GenuineIntel as vendor to play nice with SYSENTER
  x86: msr: Restore original MSR value after writing arbitrary test
    value
  x86: Force the compiler to retrieve exception info from per-cpu area
  x86: msr: Replace spaces with tabs in all of msr.c
  x86: msr: Use ARRAY_SIZE() instead of open coded equivalent
  x86: msr: Use the #defined MSR indices in favor of open coding the
    values
  x86: msr: Drop the explicit expected value
  x86: msr: Add builder macros to define MSR entries
  x86: msr: Pass msr_info instead of doing a lookup at runtime
  x86: msr: Verify 64-bit only MSRs fault on 32-bit hosts
  x86: msr: Test that always-canonical MSRs #GP on non-canonical value
  x86: msr: Verify that EFER.SCE can be written on 32-bit vCPUs

 lib/x86/desc.c      |   6 +-
 lib/x86/processor.h |  24 +++++++
 x86/cstart.S        |  28 ++++++--
 x86/msr.c           | 163 +++++++++++++++++++++-----------------------
 x86/unittests.cfg   |   5 ++
 x86/vmx_tests.c     |   2 -
 6 files changed, 131 insertions(+), 97 deletions(-)

-- 
2.31.1.498.g6c1eba8ee3d-goog

[kvm-unit-tests PATCH 01/14] x86/cstart: Don't use MSR_GS_BASE in 32-bit boot code

[Sean Christopherson]

Load the per-cpu GS.base for 32-bit build by building a temporary GDT
and loading a "real" segment.  Using MSR_GS_BASE is wrong and broken,
it's a 64-bit only MSR and does not exist on 32-bit CPUs.  The current
code works only because 32-bit KVM VMX incorrectly disables interception
of MSR_GS_BASE, and no one runs KVM on an actual 32-bit physical CPU,
i.e. the MSR exists in hardware and so everything "works".

32-bit KVM SVM is not buggy and correctly injects #GP on the WRMSR, i.e.
the tests have never worked on 32-bit SVM.

Fixes: dfe6cb6 ("Add 32 bit smp initialization code")
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 x86/cstart.S | 28 +++++++++++++++++++++++-----
 1 file changed, 23 insertions(+), 5 deletions(-)

diff --git a/x86/cstart.S b/x86/cstart.S
index 489c561..91970a2 100644
--- a/x86/cstart.S
+++ b/x86/cstart.S
@@ -89,13 +89,31 @@ mb_flags = 0x0
 	.long mb_magic, mb_flags, 0 - (mb_magic + mb_flags)
 mb_cmdline = 16
 
-MSR_GS_BASE = 0xc0000101
-
 .macro setup_percpu_area
 	lea -4096(%esp), %eax
-	mov $0, %edx
-	mov $MSR_GS_BASE, %ecx
-	wrmsr
+
+	mov %eax, %edx
+	shl $16, %edx
+	or  $0xffff, %edx
+	mov %edx, 0x10(%eax)
+
+	mov %eax, %edx
+	and $0xff000000, %edx
+	mov %eax, %ecx
+	shr $16, %ecx
+	and $0xff, %ecx
+	or  %ecx, %edx
+	or  $0x00cf9300, %edx
+	mov %edx, 0x14(%eax)
+
+	movw $0x17, 0(%eax)
+	mov %eax, 2(%eax)
+	lgdtl (%eax)
+
+	mov $0x10, %ax
+	mov %ax, %gs
+
+	lgdtl gdt32_descr
 .endm
 
 .macro setup_segments
-- 
2.31.1.498.g6c1eba8ee3d-goog

[kvm-unit-tests PATCH 02/14] x86: msr: Exclude GS/FS_BASE MSRs from 32-bit builds

[Sean Christopherson]

Don't test GS/FS_BASE and KERNEL_GS_BASE on 32-bit builds, the MSRs are
64-bit only and should #GP when accessed on 32-bit vCPUs.

Fixes: 7d36db3 ("Initial commit from qemu-kvm.git kvm/test/")
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 x86/msr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/x86/msr.c b/x86/msr.c
index ce5dabe..757156d 100644
--- a/x86/msr.c
+++ b/x86/msr.c
@@ -36,6 +36,7 @@ struct msr_info msr_info[] =
     { .index = 0x00000277, .name = "MSR_IA32_CR_PAT",
       .val_pairs = {{ .valid = 1, .value = 0x07070707, .expected = 0x07070707}}
     },
+#ifdef __x86_64__
     { .index = 0xc0000100, .name = "MSR_FS_BASE",
       .val_pairs = {{ .valid = 1, .value = addr_64, .expected = addr_64}}
     },
@@ -45,7 +46,6 @@ struct msr_info msr_info[] =
     { .index = 0xc0000102, .name = "MSR_KERNEL_GS_BASE",
       .val_pairs = {{ .valid = 1, .value = addr_64, .expected = addr_64}}
     },
-#ifdef __x86_64__
     { .index = 0xc0000080, .name = "MSR_EFER",
       .val_pairs = {{ .valid = 1, .value = 0xD00, .expected = 0xD00}}
     },
-- 
2.31.1.498.g6c1eba8ee3d-goog

[kvm-unit-tests PATCH 03/14] x86: msr: Advertise GenuineIntel as vendor to play nice with SYSENTER

[Sean Christopherson]

Run msr.flat as vendor GenuineIntel so that KVM SVM will intercept
SYSENTER_ESP and SYSENTER_EIP in order to preserve bits 63:32.
Alternatively, this could be handled in the test, but fudging the
config is easier and it also adds coverage for KVM's aforementioned
emulation.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 x86/unittests.cfg | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/x86/unittests.cfg b/x86/unittests.cfg
index 0698d15..c2608bc 100644
--- a/x86/unittests.cfg
+++ b/x86/unittests.cfg
@@ -167,7 +167,10 @@ extra_params = -cpu max
 arch = x86_64
 
 [msr]
+# Use GenuineIntel to ensure SYSENTER MSRs are fully preserved, and to test
+# SVM emulation of Intel CPU behavior.
 file = msr.flat
+extra_params = -cpu qemu64,vendor=GenuineIntel
 
 [pmu]
 file = pmu.flat
-- 
2.31.1.498.g6c1eba8ee3d-goog

[kvm-unit-tests PATCH 04/14] x86: msr: Restore original MSR value after writing arbitrary test value

[Sean Christopherson]

Restore the original MSR value after the WRMSR/RDMSR test.  MSR_GS_BASE
in particular needs to be restored as it points at per-cpu data.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 x86/msr.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/x86/msr.c b/x86/msr.c
index 757156d..5a3f1c3 100644
--- a/x86/msr.c
+++ b/x86/msr.c
@@ -77,7 +77,7 @@ static int find_msr_info(int msr_index)
 
 static void test_msr_rw(int msr_index, unsigned long long input, unsigned long long expected)
 {
-    unsigned long long r = 0;
+    unsigned long long r, orig;
     int index;
     const char *sptr;
     if ((index = find_msr_info(msr_index)) != -1) {
@@ -86,8 +86,11 @@ static void test_msr_rw(int msr_index, unsigned long long input, unsigned long l
         printf("couldn't find name for msr # %#x, skipping\n", msr_index);
         return;
     }
+
+    orig = rdmsr(msr_index);
     wrmsr(msr_index, input);
     r = rdmsr(msr_index);
+    wrmsr(msr_index, orig);
     if (expected != r) {
         printf("testing %s: output = %#" PRIx32 ":%#" PRIx32
 	       " expected = %#" PRIx32 ":%#" PRIx32 "\n", sptr,
-- 
2.31.1.498.g6c1eba8ee3d-goog

[kvm-unit-tests PATCH 05/14] x86: Force the compiler to retrieve exception info from per-cpu area

[Sean Christopherson]

Tag the exception vector/error code/flags inline asm as volatile so that
it's not elided by the compiler.  Without "volatile", the compiler may
omit the instruction if it inlines the helper and observes that the
memory isn't modified between the store in TRY_CATCH() and the load in
the helper.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 lib/x86/desc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/lib/x86/desc.c b/lib/x86/desc.c
index 983d4d8..9c1228c 100644
--- a/lib/x86/desc.c
+++ b/lib/x86/desc.c
@@ -249,7 +249,7 @@ unsigned exception_vector(void)
 {
     unsigned char vector;
 
-    asm("movb %%gs:4, %0" : "=q"(vector));
+    asm volatile("movb %%gs:4, %0" : "=q"(vector));
     return vector;
 }
 
@@ -265,7 +265,7 @@ unsigned exception_error_code(void)
 {
     unsigned short error_code;
 
-    asm("mov %%gs:6, %0" : "=r"(error_code));
+    asm volatile("mov %%gs:6, %0" : "=r"(error_code));
     return error_code;
 }
 
@@ -273,7 +273,7 @@ bool exception_rflags_rf(void)
 {
     unsigned char rf_flag;
 
-    asm("movb %%gs:5, %b0" : "=q"(rf_flag));
+    asm volatile("movb %%gs:5, %b0" : "=q"(rf_flag));
     return rf_flag & 1;
 }
 
-- 
2.31.1.498.g6c1eba8ee3d-goog

[kvm-unit-tests PATCH 06/14] x86: msr: Replace spaces with tabs in all of msr.c

[Sean Christopherson]

The bulk of msr.c is about to get rewritten, replace spaces with tabs so
the upcoming changes don't have to propagate the existing indentation,
which is a royal pain for folks whose setup assumes kernel coding style.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 x86/msr.c | 167 +++++++++++++++++++++++++++---------------------------
 1 file changed, 83 insertions(+), 84 deletions(-)

diff --git a/x86/msr.c b/x86/msr.c
index 5a3f1c3..1589b3b 100644
--- a/x86/msr.c
+++ b/x86/msr.c
@@ -5,13 +5,13 @@
 #include "msr.h"
 
 struct msr_info {
-    int index;
-    const char *name;
-    struct tc {
-        int valid;
-        unsigned long long value;
-        unsigned long long expected;
-    } val_pairs[20];
+	int index;
+	const char *name;
+	struct tc {
+		int valid;
+		unsigned long long value;
+		unsigned long long expected;
+	} val_pairs[20];
 };
 
 
@@ -20,98 +20,97 @@ struct msr_info {
 
 struct msr_info msr_info[] =
 {
-    { .index = 0x00000174, .name = "IA32_SYSENTER_CS",
-      .val_pairs = {{ .valid = 1, .value = 0x1234, .expected = 0x1234}}
-    },
-    { .index = 0x00000175, .name = "MSR_IA32_SYSENTER_ESP",
-      .val_pairs = {{ .valid = 1, .value = addr_ul, .expected = addr_ul}}
-    },
-    { .index = 0x00000176, .name = "IA32_SYSENTER_EIP",
-      .val_pairs = {{ .valid = 1, .value = addr_ul, .expected = addr_ul}}
-    },
-    { .index = 0x000001a0, .name = "MSR_IA32_MISC_ENABLE",
-      // reserved: 1:2, 4:6, 8:10, 13:15, 17, 19:21, 24:33, 35:63
-      .val_pairs = {{ .valid = 1, .value = 0x400c51889, .expected = 0x400c51889}}
-    },
-    { .index = 0x00000277, .name = "MSR_IA32_CR_PAT",
-      .val_pairs = {{ .valid = 1, .value = 0x07070707, .expected = 0x07070707}}
-    },
+	{ .index = 0x00000174, .name = "IA32_SYSENTER_CS",
+	  .val_pairs = {{ .valid = 1, .value = 0x1234, .expected = 0x1234}}
+	},
+	{ .index = 0x00000175, .name = "MSR_IA32_SYSENTER_ESP",
+	  .val_pairs = {{ .valid = 1, .value = addr_ul, .expected = addr_ul}}
+	},
+	{ .index = 0x00000176, .name = "IA32_SYSENTER_EIP",
+	  .val_pairs = {{ .valid = 1, .value = addr_ul, .expected = addr_ul}}
+	},
+	{ .index = 0x000001a0, .name = "MSR_IA32_MISC_ENABLE",
+	  // reserved: 1:2, 4:6, 8:10, 13:15, 17, 19:21, 24:33, 35:63
+	  .val_pairs = {{ .valid = 1, .value = 0x400c51889, .expected = 0x400c51889}}
+	},
+	{ .index = 0x00000277, .name = "MSR_IA32_CR_PAT",
+	  .val_pairs = {{ .valid = 1, .value = 0x07070707, .expected = 0x07070707}}
+	},
 #ifdef __x86_64__
-    { .index = 0xc0000100, .name = "MSR_FS_BASE",
-      .val_pairs = {{ .valid = 1, .value = addr_64, .expected = addr_64}}
-    },
-    { .index = 0xc0000101, .name = "MSR_GS_BASE",
-      .val_pairs = {{ .valid = 1, .value = addr_64, .expected = addr_64}}
-    },
-    { .index = 0xc0000102, .name = "MSR_KERNEL_GS_BASE",
-      .val_pairs = {{ .valid = 1, .value = addr_64, .expected = addr_64}}
-    },
-    { .index = 0xc0000080, .name = "MSR_EFER",
-      .val_pairs = {{ .valid = 1, .value = 0xD00, .expected = 0xD00}}
-    },
-    { .index = 0xc0000082, .name = "MSR_LSTAR",
-      .val_pairs = {{ .valid = 1, .value = addr_64, .expected = addr_64}}
-    },
-    { .index = 0xc0000083, .name = "MSR_CSTAR",
-      .val_pairs = {{ .valid = 1, .value = addr_64, .expected = addr_64}}
-    },
-    { .index = 0xc0000084, .name = "MSR_SYSCALL_MASK",
-      .val_pairs = {{ .valid = 1, .value = 0xffffffff, .expected = 0xffffffff}}
-    },
+	{ .index = 0xc0000100, .name = "MSR_FS_BASE",
+	  .val_pairs = {{ .valid = 1, .value = addr_64, .expected = addr_64}}
+	},
+	{ .index = 0xc0000101, .name = "MSR_GS_BASE",
+	  .val_pairs = {{ .valid = 1, .value = addr_64, .expected = addr_64}}
+	},
+	{ .index = 0xc0000102, .name = "MSR_KERNEL_GS_BASE",
+	  .val_pairs = {{ .valid = 1, .value = addr_64, .expected = addr_64}}
+	},
+	{ .index = 0xc0000080, .name = "MSR_EFER",
+	  .val_pairs = {{ .valid = 1, .value = 0xD00, .expected = 0xD00}}
+	},
+	{ .index = 0xc0000082, .name = "MSR_LSTAR",
+	  .val_pairs = {{ .valid = 1, .value = addr_64, .expected = addr_64}}
+	},
+	{ .index = 0xc0000083, .name = "MSR_CSTAR",
+	  .val_pairs = {{ .valid = 1, .value = addr_64, .expected = addr_64}}
+	},
+	{ .index = 0xc0000084, .name = "MSR_SYSCALL_MASK",
+	  .val_pairs = {{ .valid = 1, .value = 0xffffffff, .expected = 0xffffffff}}
+	},
 #endif
 
-//    MSR_IA32_DEBUGCTLMSR needs svm feature LBRV
-//    MSR_VM_HSAVE_PA only AMD host
+//	MSR_IA32_DEBUGCTLMSR needs svm feature LBRV
+//	MSR_VM_HSAVE_PA only AMD host
 };
 
 static int find_msr_info(int msr_index)
 {
-    int i;
-    for (i = 0; i < sizeof(msr_info)/sizeof(msr_info[0]) ; i++) {
-        if (msr_info[i].index == msr_index) {
-            return i;
-        }
-    }
-    return -1;
+	int i;
+
+	for (i = 0; i < sizeof(msr_info)/sizeof(msr_info[0]) ; i++) {
+		if (msr_info[i].index == msr_index)
+			return i;
+	}
+	return -1;
 }
 
 static void test_msr_rw(int msr_index, unsigned long long input, unsigned long long expected)
 {
-    unsigned long long r, orig;
-    int index;
-    const char *sptr;
-    if ((index = find_msr_info(msr_index)) != -1) {
-        sptr = msr_info[index].name;
-    } else {
-        printf("couldn't find name for msr # %#x, skipping\n", msr_index);
-        return;
-    }
+	unsigned long long r, orig;
+	int index;
+	const char *sptr;
 
-    orig = rdmsr(msr_index);
-    wrmsr(msr_index, input);
-    r = rdmsr(msr_index);
-    wrmsr(msr_index, orig);
-    if (expected != r) {
-        printf("testing %s: output = %#" PRIx32 ":%#" PRIx32
-	       " expected = %#" PRIx32 ":%#" PRIx32 "\n", sptr,
-               (u32)(r >> 32), (u32)r, (u32)(expected >> 32), (u32)expected);
-    }
-    report(expected == r, "%s", sptr);
+	if ((index = find_msr_info(msr_index)) != -1) {
+		sptr = msr_info[index].name;
+	} else {
+		printf("couldn't find name for msr # %#x, skipping\n", msr_index);
+		return;
+	}
+	orig = rdmsr(msr_index);
+	wrmsr(msr_index, input);
+	r = rdmsr(msr_index);
+	wrmsr(msr_index, orig);
+	if (expected != r) {
+		printf("testing %s: output = %#" PRIx32 ":%#" PRIx32
+		       " expected = %#" PRIx32 ":%#" PRIx32 "\n", sptr,
+		       (u32)(r >> 32), (u32)r, (u32)(expected >> 32), (u32)expected);
+	}
+	report(expected == r, "%s", sptr);
 }
 
 int main(int ac, char **av)
 {
-    int i, j;
-    for (i = 0 ; i < sizeof(msr_info) / sizeof(msr_info[0]); i++) {
-        for (j = 0; j < sizeof(msr_info[i].val_pairs) / sizeof(msr_info[i].val_pairs[0]); j++) {
-            if (msr_info[i].val_pairs[j].valid) {
-                test_msr_rw(msr_info[i].index, msr_info[i].val_pairs[j].value, msr_info[i].val_pairs[j].expected);
-            } else {
-                break;
-            }
-        }
-    }
+	int i, j;
+	for (i = 0 ; i < sizeof(msr_info) / sizeof(msr_info[0]); i++) {
+		for (j = 0; j < sizeof(msr_info[i].val_pairs) / sizeof(msr_info[i].val_pairs[0]); j++) {
+			if (msr_info[i].val_pairs[j].valid) {
+				test_msr_rw(msr_info[i].index, msr_info[i].val_pairs[j].value, msr_info[i].val_pairs[j].expected);
+			} else {
+				break;
+			}
+		}
+	}
 
-    return report_summary();
+	return report_summary();
 }
-
-- 
2.31.1.498.g6c1eba8ee3d-goog

[kvm-unit-tests PATCH 07/14] x86: msr: Use ARRAY_SIZE() instead of open coded equivalent

[Sean Christopherson]

Use ARRAY_SIZE() to iterate over the MSR values to read/write.

No functional change intended.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 x86/msr.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/x86/msr.c b/x86/msr.c
index 1589b3b..b60ca94 100644
--- a/x86/msr.c
+++ b/x86/msr.c
@@ -102,8 +102,8 @@ static void test_msr_rw(int msr_index, unsigned long long input, unsigned long l
 int main(int ac, char **av)
 {
 	int i, j;
-	for (i = 0 ; i < sizeof(msr_info) / sizeof(msr_info[0]); i++) {
-		for (j = 0; j < sizeof(msr_info[i].val_pairs) / sizeof(msr_info[i].val_pairs[0]); j++) {
+	for (i = 0 ; i < ARRAY_SIZE(msr_info); i++) {
+		for (j = 0; j < ARRAY_SIZE(msr_info[i].val_pairs); j++) {
 			if (msr_info[i].val_pairs[j].valid) {
 				test_msr_rw(msr_info[i].index, msr_info[i].val_pairs[j].value, msr_info[i].val_pairs[j].expected);
 			} else {
-- 
2.31.1.498.g6c1eba8ee3d-goog

[kvm-unit-tests PATCH 08/14] x86: msr: Use the #defined MSR indices in favor of open coding the values

[Sean Christopherson]

Use the #defines from msr.h in the MSR test, and tweak the SYSENTER names
to match for good measure.

No functional change intended.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 x86/msr.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/x86/msr.c b/x86/msr.c
index b60ca94..0fc7978 100644
--- a/x86/msr.c
+++ b/x86/msr.c
@@ -20,42 +20,42 @@ struct msr_info {
 
 struct msr_info msr_info[] =
 {
-	{ .index = 0x00000174, .name = "IA32_SYSENTER_CS",
+	{ .index = MSR_IA32_SYSENTER_CS, .name = "MSR_IA32_SYSENTER_CS",
 	  .val_pairs = {{ .valid = 1, .value = 0x1234, .expected = 0x1234}}
 	},
-	{ .index = 0x00000175, .name = "MSR_IA32_SYSENTER_ESP",
+	{ .index = MSR_IA32_SYSENTER_ESP, .name = "MSR_IA32_SYSENTER_ESP",
 	  .val_pairs = {{ .valid = 1, .value = addr_ul, .expected = addr_ul}}
 	},
-	{ .index = 0x00000176, .name = "IA32_SYSENTER_EIP",
+	{ .index = MSR_IA32_SYSENTER_EIP, .name = "MSR_IA32_SYSENTER_EIP",
 	  .val_pairs = {{ .valid = 1, .value = addr_ul, .expected = addr_ul}}
 	},
-	{ .index = 0x000001a0, .name = "MSR_IA32_MISC_ENABLE",
+	{ .index = MSR_IA32_MISC_ENABLE, .name = "MSR_IA32_MISC_ENABLE",
 	  // reserved: 1:2, 4:6, 8:10, 13:15, 17, 19:21, 24:33, 35:63
 	  .val_pairs = {{ .valid = 1, .value = 0x400c51889, .expected = 0x400c51889}}
 	},
-	{ .index = 0x00000277, .name = "MSR_IA32_CR_PAT",
+	{ .index = MSR_IA32_CR_PAT, .name = "MSR_IA32_CR_PAT",
 	  .val_pairs = {{ .valid = 1, .value = 0x07070707, .expected = 0x07070707}}
 	},
 #ifdef __x86_64__
-	{ .index = 0xc0000100, .name = "MSR_FS_BASE",
+	{ .index = MSR_FS_BASE, .name = "MSR_FS_BASE",
 	  .val_pairs = {{ .valid = 1, .value = addr_64, .expected = addr_64}}
 	},
-	{ .index = 0xc0000101, .name = "MSR_GS_BASE",
+	{ .index = MSR_GS_BASE, .name = "MSR_GS_BASE",
 	  .val_pairs = {{ .valid = 1, .value = addr_64, .expected = addr_64}}
 	},
-	{ .index = 0xc0000102, .name = "MSR_KERNEL_GS_BASE",
+	{ .index = MSR_KERNEL_GS_BASE, .name = "MSR_KERNEL_GS_BASE",
 	  .val_pairs = {{ .valid = 1, .value = addr_64, .expected = addr_64}}
 	},
-	{ .index = 0xc0000080, .name = "MSR_EFER",
+	{ .index = MSR_EFER, .name = "MSR_EFER",
 	  .val_pairs = {{ .valid = 1, .value = 0xD00, .expected = 0xD00}}
 	},
-	{ .index = 0xc0000082, .name = "MSR_LSTAR",
+	{ .index = MSR_LSTAR, .name = "MSR_LSTAR",
 	  .val_pairs = {{ .valid = 1, .value = addr_64, .expected = addr_64}}
 	},
-	{ .index = 0xc0000083, .name = "MSR_CSTAR",
+	{ .index = MSR_CSTAR, .name = "MSR_CSTAR",
 	  .val_pairs = {{ .valid = 1, .value = addr_64, .expected = addr_64}}
 	},
-	{ .index = 0xc0000084, .name = "MSR_SYSCALL_MASK",
+	{ .index = MSR_SYSCALL_MASK, .name = "MSR_SYSCALL_MASK",
 	  .val_pairs = {{ .valid = 1, .value = 0xffffffff, .expected = 0xffffffff}}
 	},
 #endif
-- 
2.31.1.498.g6c1eba8ee3d-goog

[kvm-unit-tests PATCH 09/14] x86: msr: Drop the explicit expected value

[Sean Christopherson]

Use the written value as the expected value.  For the vast majority of
MSRs, including all those currently tested, the expected value will
always be the last written value.  This will simplify handling EFER on
32-bit vCPUs in a future patch.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 x86/msr.c | 37 +++++++++++++++++++------------------
 1 file changed, 19 insertions(+), 18 deletions(-)

diff --git a/x86/msr.c b/x86/msr.c
index 0fc7978..9031043 100644
--- a/x86/msr.c
+++ b/x86/msr.c
@@ -21,42 +21,42 @@ struct msr_info {
 struct msr_info msr_info[] =
 {
 	{ .index = MSR_IA32_SYSENTER_CS, .name = "MSR_IA32_SYSENTER_CS",
-	  .val_pairs = {{ .valid = 1, .value = 0x1234, .expected = 0x1234}}
+	  .val_pairs = {{ .valid = 1, .value = 0x1234 }}
 	},
 	{ .index = MSR_IA32_SYSENTER_ESP, .name = "MSR_IA32_SYSENTER_ESP",
-	  .val_pairs = {{ .valid = 1, .value = addr_ul, .expected = addr_ul}}
+	  .val_pairs = {{ .valid = 1, .value = addr_ul }}
 	},
 	{ .index = MSR_IA32_SYSENTER_EIP, .name = "MSR_IA32_SYSENTER_EIP",
-	  .val_pairs = {{ .valid = 1, .value = addr_ul, .expected = addr_ul}}
+	  .val_pairs = {{ .valid = 1, .value = addr_ul }}
 	},
 	{ .index = MSR_IA32_MISC_ENABLE, .name = "MSR_IA32_MISC_ENABLE",
 	  // reserved: 1:2, 4:6, 8:10, 13:15, 17, 19:21, 24:33, 35:63
-	  .val_pairs = {{ .valid = 1, .value = 0x400c51889, .expected = 0x400c51889}}
+	  .val_pairs = {{ .valid = 1, .value = 0x400c51889 }}
 	},
 	{ .index = MSR_IA32_CR_PAT, .name = "MSR_IA32_CR_PAT",
-	  .val_pairs = {{ .valid = 1, .value = 0x07070707, .expected = 0x07070707}}
+	  .val_pairs = {{ .valid = 1, .value = 0x07070707 }}
 	},
 #ifdef __x86_64__
 	{ .index = MSR_FS_BASE, .name = "MSR_FS_BASE",
-	  .val_pairs = {{ .valid = 1, .value = addr_64, .expected = addr_64}}
+	  .val_pairs = {{ .valid = 1, .value = addr_64 }}
 	},
 	{ .index = MSR_GS_BASE, .name = "MSR_GS_BASE",
-	  .val_pairs = {{ .valid = 1, .value = addr_64, .expected = addr_64}}
+	  .val_pairs = {{ .valid = 1, .value = addr_64 }}
 	},
 	{ .index = MSR_KERNEL_GS_BASE, .name = "MSR_KERNEL_GS_BASE",
-	  .val_pairs = {{ .valid = 1, .value = addr_64, .expected = addr_64}}
+	  .val_pairs = {{ .valid = 1, .value = addr_64 }}
 	},
 	{ .index = MSR_EFER, .name = "MSR_EFER",
-	  .val_pairs = {{ .valid = 1, .value = 0xD00, .expected = 0xD00}}
+	  .val_pairs = {{ .valid = 1, .value = 0xD00 }}
 	},
 	{ .index = MSR_LSTAR, .name = "MSR_LSTAR",
-	  .val_pairs = {{ .valid = 1, .value = addr_64, .expected = addr_64}}
+	  .val_pairs = {{ .valid = 1, .value = addr_64 }}
 	},
 	{ .index = MSR_CSTAR, .name = "MSR_CSTAR",
-	  .val_pairs = {{ .valid = 1, .value = addr_64, .expected = addr_64}}
+	  .val_pairs = {{ .valid = 1, .value = addr_64 }}
 	},
 	{ .index = MSR_SYSCALL_MASK, .name = "MSR_SYSCALL_MASK",
-	  .val_pairs = {{ .valid = 1, .value = 0xffffffff, .expected = 0xffffffff}}
+	  .val_pairs = {{ .valid = 1, .value = 0xffffffff }}
 	},
 #endif
 
@@ -75,7 +75,7 @@ static int find_msr_info(int msr_index)
 	return -1;
 }
 
-static void test_msr_rw(int msr_index, unsigned long long input, unsigned long long expected)
+static void test_msr_rw(int msr_index, unsigned long long val)
 {
 	unsigned long long r, orig;
 	int index;
@@ -87,16 +87,17 @@ static void test_msr_rw(int msr_index, unsigned long long input, unsigned long l
 		printf("couldn't find name for msr # %#x, skipping\n", msr_index);
 		return;
 	}
+
 	orig = rdmsr(msr_index);
-	wrmsr(msr_index, input);
+	wrmsr(msr_index, val);
 	r = rdmsr(msr_index);
 	wrmsr(msr_index, orig);
-	if (expected != r) {
+	if (r != val) {
 		printf("testing %s: output = %#" PRIx32 ":%#" PRIx32
 		       " expected = %#" PRIx32 ":%#" PRIx32 "\n", sptr,
-		       (u32)(r >> 32), (u32)r, (u32)(expected >> 32), (u32)expected);
+		       (u32)(r >> 32), (u32)r, (u32)(val >> 32), (u32)val);
 	}
-	report(expected == r, "%s", sptr);
+	report(val == r, "%s", sptr);
 }
 
 int main(int ac, char **av)
@@ -105,7 +106,7 @@ int main(int ac, char **av)
 	for (i = 0 ; i < ARRAY_SIZE(msr_info); i++) {
 		for (j = 0; j < ARRAY_SIZE(msr_info[i].val_pairs); j++) {
 			if (msr_info[i].val_pairs[j].valid) {
-				test_msr_rw(msr_info[i].index, msr_info[i].val_pairs[j].value, msr_info[i].val_pairs[j].expected);
+				test_msr_rw(msr_info[i].index, msr_info[i].val_pairs[j].value);
 			} else {
 				break;
 			}
-- 
2.31.1.498.g6c1eba8ee3d-goog

[kvm-unit-tests PATCH 10/14] x86: msr: Add builder macros to define MSR entries

[Sean Christopherson]

Add macros to define the MSRs to test, primarily so that the stringified
names can be auto-generated.

No functional change intended.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 x86/msr.c | 74 ++++++++++++++++---------------------------------------
 1 file changed, 21 insertions(+), 53 deletions(-)

diff --git a/x86/msr.c b/x86/msr.c
index 9031043..4473950 100644
--- a/x86/msr.c
+++ b/x86/msr.c
@@ -7,59 +7,33 @@
 struct msr_info {
 	int index;
 	const char *name;
-	struct tc {
-		int valid;
-		unsigned long long value;
-		unsigned long long expected;
-	} val_pairs[20];
+	unsigned long long value;
 };
 
 
 #define addr_64 0x0000123456789abcULL
 #define addr_ul (unsigned long)addr_64
 
+#define MSR_TEST(msr, val)	\
+	{ .index = msr, .name = #msr, .value = val }
+
 struct msr_info msr_info[] =
 {
-	{ .index = MSR_IA32_SYSENTER_CS, .name = "MSR_IA32_SYSENTER_CS",
-	  .val_pairs = {{ .valid = 1, .value = 0x1234 }}
-	},
-	{ .index = MSR_IA32_SYSENTER_ESP, .name = "MSR_IA32_SYSENTER_ESP",
-	  .val_pairs = {{ .valid = 1, .value = addr_ul }}
-	},
-	{ .index = MSR_IA32_SYSENTER_EIP, .name = "MSR_IA32_SYSENTER_EIP",
-	  .val_pairs = {{ .valid = 1, .value = addr_ul }}
-	},
-	{ .index = MSR_IA32_MISC_ENABLE, .name = "MSR_IA32_MISC_ENABLE",
-	  // reserved: 1:2, 4:6, 8:10, 13:15, 17, 19:21, 24:33, 35:63
-	  .val_pairs = {{ .valid = 1, .value = 0x400c51889 }}
-	},
-	{ .index = MSR_IA32_CR_PAT, .name = "MSR_IA32_CR_PAT",
-	  .val_pairs = {{ .valid = 1, .value = 0x07070707 }}
-	},
+	MSR_TEST(MSR_IA32_SYSENTER_CS, 0x1234),
+	MSR_TEST(MSR_IA32_SYSENTER_ESP, addr_ul),
+	MSR_TEST(MSR_IA32_SYSENTER_EIP, addr_ul),
+	// reserved: 1:2, 4:6, 8:10, 13:15, 17, 19:21, 24:33, 35:63
+	MSR_TEST(MSR_IA32_MISC_ENABLE, 0x400c51889),
+	MSR_TEST(MSR_IA32_CR_PAT, 0x07070707),
 #ifdef __x86_64__
-	{ .index = MSR_FS_BASE, .name = "MSR_FS_BASE",
-	  .val_pairs = {{ .valid = 1, .value = addr_64 }}
-	},
-	{ .index = MSR_GS_BASE, .name = "MSR_GS_BASE",
-	  .val_pairs = {{ .valid = 1, .value = addr_64 }}
-	},
-	{ .index = MSR_KERNEL_GS_BASE, .name = "MSR_KERNEL_GS_BASE",
-	  .val_pairs = {{ .valid = 1, .value = addr_64 }}
-	},
-	{ .index = MSR_EFER, .name = "MSR_EFER",
-	  .val_pairs = {{ .valid = 1, .value = 0xD00 }}
-	},
-	{ .index = MSR_LSTAR, .name = "MSR_LSTAR",
-	  .val_pairs = {{ .valid = 1, .value = addr_64 }}
-	},
-	{ .index = MSR_CSTAR, .name = "MSR_CSTAR",
-	  .val_pairs = {{ .valid = 1, .value = addr_64 }}
-	},
-	{ .index = MSR_SYSCALL_MASK, .name = "MSR_SYSCALL_MASK",
-	  .val_pairs = {{ .valid = 1, .value = 0xffffffff }}
-	},
+	MSR_TEST(MSR_FS_BASE, addr_64),
+	MSR_TEST(MSR_GS_BASE, addr_64),
+	MSR_TEST(MSR_KERNEL_GS_BASE, addr_64),
+	MSR_TEST(MSR_EFER, 0xD00),
+	MSR_TEST(MSR_LSTAR, addr_64),
+	MSR_TEST(MSR_CSTAR, addr_64),
+	MSR_TEST(MSR_SYSCALL_MASK, 0xffffffff),
 #endif
-
 //	MSR_IA32_DEBUGCTLMSR needs svm feature LBRV
 //	MSR_VM_HSAVE_PA only AMD host
 };
@@ -102,16 +76,10 @@ static void test_msr_rw(int msr_index, unsigned long long val)
 
 int main(int ac, char **av)
 {
-	int i, j;
-	for (i = 0 ; i < ARRAY_SIZE(msr_info); i++) {
-		for (j = 0; j < ARRAY_SIZE(msr_info[i].val_pairs); j++) {
-			if (msr_info[i].val_pairs[j].valid) {
-				test_msr_rw(msr_info[i].index, msr_info[i].val_pairs[j].value);
-			} else {
-				break;
-			}
-		}
-	}
+	int i;
+
+	for (i = 0 ; i < ARRAY_SIZE(msr_info); i++)
+		test_msr_rw(msr_info[i].index, msr_info[i].value);
 
 	return report_summary();
 }
-- 
2.31.1.498.g6c1eba8ee3d-goog

[kvm-unit-tests PATCH 11/14] x86: msr: Pass msr_info instead of doing a lookup at runtime

[Sean Christopherson]

Pass the msr_info to test_msr_rw() instead of passing a subset of the
struct info and then using that to look up the struct.  Pass the value to
write as a separate parameter, doing so will simplify upcoming patches.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 x86/msr.c | 36 ++++++++----------------------------
 1 file changed, 8 insertions(+), 28 deletions(-)

diff --git a/x86/msr.c b/x86/msr.c
index 4473950..4642451 100644
--- a/x86/msr.c
+++ b/x86/msr.c
@@ -38,40 +38,20 @@ struct msr_info msr_info[] =
 //	MSR_VM_HSAVE_PA only AMD host
 };
 
-static int find_msr_info(int msr_index)
-{
-	int i;
-
-	for (i = 0; i < sizeof(msr_info)/sizeof(msr_info[0]) ; i++) {
-		if (msr_info[i].index == msr_index)
-			return i;
-	}
-	return -1;
-}
-
-static void test_msr_rw(int msr_index, unsigned long long val)
+static void test_msr_rw(struct msr_info *msr, unsigned long long val)
 {
 	unsigned long long r, orig;
-	int index;
-	const char *sptr;
 
-	if ((index = find_msr_info(msr_index)) != -1) {
-		sptr = msr_info[index].name;
-	} else {
-		printf("couldn't find name for msr # %#x, skipping\n", msr_index);
-		return;
-	}
-
-	orig = rdmsr(msr_index);
-	wrmsr(msr_index, val);
-	r = rdmsr(msr_index);
-	wrmsr(msr_index, orig);
+	orig = rdmsr(msr->index);
+	wrmsr(msr->index, val);
+	r = rdmsr(msr->index);
+	wrmsr(msr->index, orig);
 	if (r != val) {
 		printf("testing %s: output = %#" PRIx32 ":%#" PRIx32
-		       " expected = %#" PRIx32 ":%#" PRIx32 "\n", sptr,
+		       " expected = %#" PRIx32 ":%#" PRIx32 "\n", msr->name,
 		       (u32)(r >> 32), (u32)r, (u32)(val >> 32), (u32)val);
 	}
-	report(val == r, "%s", sptr);
+	report(val == r, "%s", msr->name);
 }
 
 int main(int ac, char **av)
@@ -79,7 +59,7 @@ int main(int ac, char **av)
 	int i;
 
 	for (i = 0 ; i < ARRAY_SIZE(msr_info); i++)
-		test_msr_rw(msr_info[i].index, msr_info[i].value);
+		test_msr_rw(&msr_info[i], msr_info[i].value);
 
 	return report_summary();
 }
-- 
2.31.1.498.g6c1eba8ee3d-goog

[kvm-unit-tests PATCH 12/14] x86: msr: Verify 64-bit only MSRs fault on 32-bit hosts

[Sean Christopherson]

Assert that 64-bit only MSRs take a #GP when read or written on 32-bit
hosts, as opposed to simply skipping the MSRs on 32-bit builds.  Force
"cpu -host" so that CPUID can be used to check for 64-bit support.

Technically, the unit test could/should be even more aggressive and
require KVM to inject faults if the vCPU model doesn't support 64-bit
mode.  But, there are no plans to go to that level of emulation in KVM,
and practically speaking there isn't much benefit as allowing a 32-bit
vCPU to access the MSRs on a 64-bit host is a benign virtualization hole.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 lib/x86/processor.h | 22 +++++++++++++++++
 x86/msr.c           | 57 ++++++++++++++++++++++++++++++++-------------
 x86/unittests.cfg   |  6 +++--
 3 files changed, 67 insertions(+), 18 deletions(-)

diff --git a/lib/x86/processor.h b/lib/x86/processor.h
index dda57a1..dfe96d0 100644
--- a/lib/x86/processor.h
+++ b/lib/x86/processor.h
@@ -2,6 +2,7 @@
 #define LIBCFLAT_PROCESSOR_H
 
 #include "libcflat.h"
+#include "desc.h"
 #include "msr.h"
 #include <stdint.h>
 
@@ -163,6 +164,7 @@ static inline u8 cpuid_maxphyaddr(void)
 #define	X86_FEATURE_ARCH_CAPABILITIES	(CPUID(0x7, 0, EDX, 29))
 #define	X86_FEATURE_PKS			(CPUID(0x7, 0, ECX, 31))
 #define	X86_FEATURE_NX			(CPUID(0x80000001, 0, EDX, 20))
+#define	X86_FEATURE_LM			(CPUID(0x80000001, 0, EDX, 29))
 #define	X86_FEATURE_RDPRU		(CPUID(0x80000008, 0, EBX, 4))
 
 /*
@@ -320,6 +322,26 @@ static inline void wrmsr(u32 index, u64 val)
     asm volatile ("wrmsr" : : "a"(a), "d"(d), "c"(index) : "memory");
 }
 
+static inline int rdmsr_checking(u32 index)
+{
+	asm volatile (ASM_TRY("1f")
+		      "rdmsr\n\t"
+		      "1:"
+		      : : "c"(index) : "memory", "eax", "edx");
+	return exception_vector();
+}
+
+static inline int wrmsr_checking(u32 index, u64 val)
+{
+        u32 a = val, d = val >> 32;
+
+	asm volatile (ASM_TRY("1f")
+		      "wrmsr\n\t"
+		      "1:"
+		      : : "a"(a), "d"(d), "c"(index) : "memory");
+	return exception_vector();
+}
+
 static inline uint64_t rdpmc(uint32_t index)
 {
     uint32_t a, d;
diff --git a/x86/msr.c b/x86/msr.c
index 4642451..e7ebe8b 100644
--- a/x86/msr.c
+++ b/x86/msr.c
@@ -6,6 +6,7 @@
 
 struct msr_info {
 	int index;
+	bool is_64bit_only;
 	const char *name;
 	unsigned long long value;
 };
@@ -14,26 +15,26 @@ struct msr_info {
 #define addr_64 0x0000123456789abcULL
 #define addr_ul (unsigned long)addr_64
 
-#define MSR_TEST(msr, val)	\
-	{ .index = msr, .name = #msr, .value = val }
+#define MSR_TEST(msr, val, only64)	\
+	{ .index = msr, .name = #msr, .value = val, .is_64bit_only = only64 }
 
 struct msr_info msr_info[] =
 {
-	MSR_TEST(MSR_IA32_SYSENTER_CS, 0x1234),
-	MSR_TEST(MSR_IA32_SYSENTER_ESP, addr_ul),
-	MSR_TEST(MSR_IA32_SYSENTER_EIP, addr_ul),
+	MSR_TEST(MSR_IA32_SYSENTER_CS, 0x1234, false),
+	MSR_TEST(MSR_IA32_SYSENTER_ESP, addr_ul, false),
+	MSR_TEST(MSR_IA32_SYSENTER_EIP, addr_ul, false),
 	// reserved: 1:2, 4:6, 8:10, 13:15, 17, 19:21, 24:33, 35:63
-	MSR_TEST(MSR_IA32_MISC_ENABLE, 0x400c51889),
-	MSR_TEST(MSR_IA32_CR_PAT, 0x07070707),
+	MSR_TEST(MSR_IA32_MISC_ENABLE, 0x400c51889, false),
+	MSR_TEST(MSR_IA32_CR_PAT, 0x07070707, false),
+	MSR_TEST(MSR_FS_BASE, addr_64, true),
+	MSR_TEST(MSR_GS_BASE, addr_64, true),
+	MSR_TEST(MSR_KERNEL_GS_BASE, addr_64, true),
 #ifdef __x86_64__
-	MSR_TEST(MSR_FS_BASE, addr_64),
-	MSR_TEST(MSR_GS_BASE, addr_64),
-	MSR_TEST(MSR_KERNEL_GS_BASE, addr_64),
-	MSR_TEST(MSR_EFER, 0xD00),
-	MSR_TEST(MSR_LSTAR, addr_64),
-	MSR_TEST(MSR_CSTAR, addr_64),
-	MSR_TEST(MSR_SYSCALL_MASK, 0xffffffff),
+	MSR_TEST(MSR_EFER, 0xD00, false),
 #endif
+	MSR_TEST(MSR_LSTAR, addr_64, true),
+	MSR_TEST(MSR_CSTAR, addr_64, true),
+	MSR_TEST(MSR_SYSCALL_MASK, 0xffffffff, true),
 //	MSR_IA32_DEBUGCTLMSR needs svm feature LBRV
 //	MSR_VM_HSAVE_PA only AMD host
 };
@@ -54,12 +55,36 @@ static void test_msr_rw(struct msr_info *msr, unsigned long long val)
 	report(val == r, "%s", msr->name);
 }
 
+static void test_wrmsr_fault(struct msr_info *msr, unsigned long long val)
+{
+	unsigned char vector = wrmsr_checking(msr->index, val);
+
+	report(vector == GP_VECTOR,
+	       "Expected #GP on WRSMR(%s, 0x%llx), got vector %d",
+	       msr->name, val, vector);
+}
+
+static void test_rdmsr_fault(struct msr_info *msr)
+{
+	unsigned char vector = rdmsr_checking(msr->index);
+
+	report(vector == GP_VECTOR,
+	       "Expected #GP on RDSMR(%s), got vector %d", msr->name, vector);
+}
+
 int main(int ac, char **av)
 {
+	bool is_64bit_host = this_cpu_has(X86_FEATURE_LM);
 	int i;
 
-	for (i = 0 ; i < ARRAY_SIZE(msr_info); i++)
-		test_msr_rw(&msr_info[i], msr_info[i].value);
+	for (i = 0 ; i < ARRAY_SIZE(msr_info); i++) {
+		if (is_64bit_host || !msr_info[i].is_64bit_only) {
+			test_msr_rw(&msr_info[i], msr_info[i].value);
+		} else {
+			test_wrmsr_fault(&msr_info[i], msr_info[i].value);
+			test_rdmsr_fault(&msr_info[i]);
+		}
+	}
 
 	return report_summary();
 }
diff --git a/x86/unittests.cfg b/x86/unittests.cfg
index c2608bc..29cfe51 100644
--- a/x86/unittests.cfg
+++ b/x86/unittests.cfg
@@ -168,9 +168,11 @@ arch = x86_64
 
 [msr]
 # Use GenuineIntel to ensure SYSENTER MSRs are fully preserved, and to test
-# SVM emulation of Intel CPU behavior.
+# SVM emulation of Intel CPU behavior.  Use the host CPU model so that 64-bit
+# support follows the host kernel.  Running a 32-bit guest on a 64-bit host
+# will fail due to shortcomings in KVM.
 file = msr.flat
-extra_params = -cpu qemu64,vendor=GenuineIntel
+extra_params = -cpu host,vendor=GenuineIntel
 
 [pmu]
 file = pmu.flat
-- 
2.31.1.498.g6c1eba8ee3d-goog

[kvm-unit-tests PATCH 13/14] x86: msr: Test that always-canonical MSRs #GP on non-canonical value

[Sean Christopherson]

Verify that WRMSR takes a #GP when writing a non-canonical value to a
MSR that always takes a 64-bit address.  Specifically, AMD doesn't
enforce a canonical address for the SYSENTER MSRs.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 lib/x86/processor.h | 2 ++
 x86/msr.c           | 8 ++++++++
 x86/vmx_tests.c     | 2 --
 3 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/lib/x86/processor.h b/lib/x86/processor.h
index dfe96d0..abc04b0 100644
--- a/lib/x86/processor.h
+++ b/lib/x86/processor.h
@@ -6,6 +6,8 @@
 #include "msr.h"
 #include <stdint.h>
 
+#define NONCANONICAL            0xaaaaaaaaaaaaaaaaull
+
 #ifdef __x86_64__
 #  define R "r"
 #  define W "q"
diff --git a/x86/msr.c b/x86/msr.c
index e7ebe8b..8a1b0b2 100644
--- a/x86/msr.c
+++ b/x86/msr.c
@@ -80,6 +80,14 @@ int main(int ac, char **av)
 	for (i = 0 ; i < ARRAY_SIZE(msr_info); i++) {
 		if (is_64bit_host || !msr_info[i].is_64bit_only) {
 			test_msr_rw(&msr_info[i], msr_info[i].value);
+
+			/*
+			 * The 64-bit only MSRs that take an address always perform
+			 * canonical checks on both Intel and AMD.
+			 */
+			if (msr_info[i].is_64bit_only &&
+			    msr_info[i].value == addr_64)
+				test_wrmsr_fault(&msr_info[i], NONCANONICAL);
 		} else {
 			test_wrmsr_fault(&msr_info[i], msr_info[i].value);
 			test_rdmsr_fault(&msr_info[i]);
diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
index bbb006a..2eb5962 100644
--- a/x86/vmx_tests.c
+++ b/x86/vmx_tests.c
@@ -21,8 +21,6 @@
 #include "smp.h"
 #include "delay.h"
 
-#define NONCANONICAL            0xaaaaaaaaaaaaaaaaull
-
 #define VPID_CAP_INVVPID_TYPES_SHIFT 40
 
 u64 ia32_pat;
-- 
2.31.1.498.g6c1eba8ee3d-goog

[kvm-unit-tests PATCH 14/14] x86: msr: Verify that EFER.SCE can be written on 32-bit vCPUs

[Sean Christopherson]

Verify that EFER can be written, and that the SYSCALL enable bit can be
set, on 32-bit vCPUs.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 x86/msr.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/x86/msr.c b/x86/msr.c
index 8a1b0b2..7a551c4 100644
--- a/x86/msr.c
+++ b/x86/msr.c
@@ -29,9 +29,7 @@ struct msr_info msr_info[] =
 	MSR_TEST(MSR_FS_BASE, addr_64, true),
 	MSR_TEST(MSR_GS_BASE, addr_64, true),
 	MSR_TEST(MSR_KERNEL_GS_BASE, addr_64, true),
-#ifdef __x86_64__
-	MSR_TEST(MSR_EFER, 0xD00, false),
-#endif
+	MSR_TEST(MSR_EFER, EFER_SCE, false),
 	MSR_TEST(MSR_LSTAR, addr_64, true),
 	MSR_TEST(MSR_CSTAR, addr_64, true),
 	MSR_TEST(MSR_SYSCALL_MASK, 0xffffffff, true),
@@ -44,6 +42,13 @@ static void test_msr_rw(struct msr_info *msr, unsigned long long val)
 	unsigned long long r, orig;
 
 	orig = rdmsr(msr->index);
+	/*
+	 * Special case EFER since clearing LME/LMA is not allowed in 64-bit mode,
+	 * and conversely setting those bits on 32-bit CPUs is not allowed.  Treat
+	 * the desired value as extra bits to set.
+	 */
+	if (msr->index == MSR_EFER)
+		val |= orig;
 	wrmsr(msr->index, val);
 	r = rdmsr(msr->index);
 	wrmsr(msr->index, orig);
-- 
2.31.1.498.g6c1eba8ee3d-goog

Re: [kvm-unit-tests PATCH 01/14] x86/cstart: Don't use MSR_GS_BASE in 32-bit boot code

[Paolo Bonzini]

On 22/04/21 05:04, Sean Christopherson wrote:
> Load the per-cpu GS.base for 32-bit build by building a temporary GDT
> and loading a "real" segment.  Using MSR_GS_BASE is wrong and broken,
> it's a 64-bit only MSR and does not exist on 32-bit CPUs.  The current
> code works only because 32-bit KVM VMX incorrectly disables interception
> of MSR_GS_BASE, and no one runs KVM on an actual 32-bit physical CPU,
> i.e. the MSR exists in hardware and so everything "works".
> 
> 32-bit KVM SVM is not buggy and correctly injects #GP on the WRMSR, i.e.
> the tests have never worked on 32-bit SVM.
> 
> Fixes: dfe6cb6 ("Add 32 bit smp initialization code")
> Signed-off-by: Sean Christopherson <seanjc@google.com>

Relying on the descriptor cache is quite ugly but the only alternative 
are setting up extra segments in the GDT or having per-CPU GDTs (which 
I'd rather avoid).

Paolo

> ---
>   x86/cstart.S | 28 +++++++++++++++++++++++-----
>   1 file changed, 23 insertions(+), 5 deletions(-)
> 
> diff --git a/x86/cstart.S b/x86/cstart.S
> index 489c561..91970a2 100644
> --- a/x86/cstart.S
> +++ b/x86/cstart.S
> @@ -89,13 +89,31 @@ mb_flags = 0x0
>   	.long mb_magic, mb_flags, 0 - (mb_magic + mb_flags)
>   mb_cmdline = 16
>   
> -MSR_GS_BASE = 0xc0000101
> -
>   .macro setup_percpu_area
>   	lea -4096(%esp), %eax
> -	mov $0, %edx
> -	mov $MSR_GS_BASE, %ecx
> -	wrmsr
> +
> +	mov %eax, %edx
> +	shl $16, %edx
> +	or  $0xffff, %edx
> +	mov %edx, 0x10(%eax)
> +
> +	mov %eax, %edx
> +	and $0xff000000, %edx
> +	mov %eax, %ecx
> +	shr $16, %ecx
> +	and $0xff, %ecx
> +	or  %ecx, %edx
> +	or  $0x00cf9300, %edx
> +	mov %edx, 0x14(%eax)
> +
> +	movw $0x17, 0(%eax)
> +	mov %eax, 2(%eax)
> +	lgdtl (%eax)
> +
> +	mov $0x10, %ax
> +	mov %ax, %gs
> +
> +	lgdtl gdt32_descr
>   .endm
>   
>   .macro setup_segments
> 

Re: [kvm-unit-tests PATCH 01/14] x86/cstart: Don't use MSR_GS_BASE in 32-bit boot code

[Paolo Bonzini]

On 22/04/21 05:04, Sean Christopherson wrote:
> Load the per-cpu GS.base for 32-bit build by building a temporary GDT
> and loading a "real" segment.  Using MSR_GS_BASE is wrong and broken,
> it's a 64-bit only MSR and does not exist on 32-bit CPUs.  The current
> code works only because 32-bit KVM VMX incorrectly disables interception
> of MSR_GS_BASE, and no one runs KVM on an actual 32-bit physical CPU,
> i.e. the MSR exists in hardware and so everything "works".
> 
> 32-bit KVM SVM is not buggy and correctly injects #GP on the WRMSR, i.e.
> the tests have never worked on 32-bit SVM.

Hmm, this breaks task switch.  But setting up separate descriptors is
not hard:

diff --git a/x86/cstart.S b/x86/cstart.S
index 489c561..7d9ed96 100644
--- a/x86/cstart.S
+++ b/x86/cstart.S
@@ -58,6 +58,10 @@ tss_descr:
          .rept max_cpus
          .quad 0x000089000000ffff // 32-bit avail tss
          .endr
+percpu_descr:
+        .rept max_cpus
+        .quad 0x00cf93000000ffff // 32-bit data segment for perCPU area
+        .endr
  gdt32_end:

  i = 0
@@ -89,13 +93,23 @@ mb_flags = 0x0
  	.long mb_magic, mb_flags, 0 - (mb_magic + mb_flags)
  mb_cmdline = 16

-MSR_GS_BASE = 0xc0000101
-
  .macro setup_percpu_area
  	lea -4096(%esp), %eax
-	mov $0, %edx
-	mov $MSR_GS_BASE, %ecx
-	wrmsr
+
+	/* fill GS_BASE in the GDT */
+	mov $(APIC_DEFAULT_PHYS_BASE + APIC_ID), %ebx
+	mov (%ebx), %ebx
+	shr $24, %ebx
+	or %ax, percpu_descr+2(,%ebx,8)
+
+	shr $16, %eax
+	or %al, percpu_descr+4(,%ebx,8)
+	or %ah, percpu_descr+7(,%ebx,8)
+
+	lgdtl gdt32_descr
+	lea percpu_descr-gdt32(,%ebx,8), %eax
+	mov %ax, %gs
+
  .endm

  .macro setup_segments
@@ -188,16 +202,14 @@ load_tss:
  	mov (%eax), %eax
  	shr $24, %eax
  	mov %eax, %ebx
-	shl $3, %ebx
  	mov $((tss_end - tss) / max_cpus), %edx
  	imul %edx
  	add $tss, %eax
-	mov %ax, tss_descr+2(%ebx)
+	mov %ax, tss_descr+2(,%ebx,8)
  	shr $16, %eax
-	mov %al, tss_descr+4(%ebx)
-	shr $8, %eax
-	mov %al, tss_descr+7(%ebx)
-	lea tss_descr-gdt32(%ebx), %eax
+	mov %al, tss_descr+4(,%ebx,8)
+	mov %ah, tss_descr+7(,%ebx,8)
+	lea tss_descr-gdt32(,%ebx,8), %eax
  	ltr %ax
  	ret


Paolo

Re: [kvm-unit-tests PATCH 03/14] x86: msr: Advertise GenuineIntel as vendor to play nice with SYSENTER

[Paolo Bonzini]

On 22/04/21 05:04, Sean Christopherson wrote:
> Run msr.flat as vendor GenuineIntel so that KVM SVM will intercept
> SYSENTER_ESP and SYSENTER_EIP in order to preserve bits 63:32.
> Alternatively, this could be handled in the test, but fudging the
> config is easier and it also adds coverage for KVM's aforementioned
> emulation.

Oops, I actually had a patch for this but forgot to send it.  I'll queue 
yours instead.

Paolo

> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>   x86/unittests.cfg | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/x86/unittests.cfg b/x86/unittests.cfg
> index 0698d15..c2608bc 100644
> --- a/x86/unittests.cfg
> +++ b/x86/unittests.cfg
> @@ -167,7 +167,10 @@ extra_params = -cpu max
>   arch = x86_64
>   
>   [msr]
> +# Use GenuineIntel to ensure SYSENTER MSRs are fully preserved, and to test
> +# SVM emulation of Intel CPU behavior.
>   file = msr.flat
> +extra_params = -cpu qemu64,vendor=GenuineIntel
>   
>   [pmu]
>   file = pmu.flat
> 

Re: [kvm-unit-tests PATCH 12/14] x86: msr: Verify 64-bit only MSRs fault on 32-bit hosts

[Paolo Bonzini]

On 22/04/21 05:05, Sean Christopherson wrote:
> Force "cpu -host" so that CPUID can be used to check for 64-bit support.

"-cpu max" is preferred because it also works with QEMU binary 
translation, otherwise looks good.  Someone can fix KVM if they're bored.

Paolo

Re: [kvm-unit-tests PATCH 01/14] x86/cstart: Don't use MSR_GS_BASE in 32-bit boot code

[Sean Christopherson]

On Thu, Apr 22, 2021, Paolo Bonzini wrote:
> On 22/04/21 05:04, Sean Christopherson wrote:
> > Load the per-cpu GS.base for 32-bit build by building a temporary GDT
> > and loading a "real" segment.  Using MSR_GS_BASE is wrong and broken,
> > it's a 64-bit only MSR and does not exist on 32-bit CPUs.  The current
> > code works only because 32-bit KVM VMX incorrectly disables interception
> > of MSR_GS_BASE, and no one runs KVM on an actual 32-bit physical CPU,
> > i.e. the MSR exists in hardware and so everything "works".
> > 
> > 32-bit KVM SVM is not buggy and correctly injects #GP on the WRMSR, i.e.
> > the tests have never worked on 32-bit SVM.
> 
> Hmm, this breaks task switch.  But setting up separate descriptors is
> not hard:

Much better.

> diff --git a/x86/cstart.S b/x86/cstart.S
> index 489c561..7d9ed96 100644
> --- a/x86/cstart.S
> +++ b/x86/cstart.S
> @@ -58,6 +58,10 @@ tss_descr:
>          .rept max_cpus
>          .quad 0x000089000000ffff // 32-bit avail tss
>          .endr
> +percpu_descr:
> +        .rept max_cpus
> +        .quad 0x00cf93000000ffff // 32-bit data segment for perCPU area
> +        .endr
>  gdt32_end:
> 
>  i = 0
> @@ -89,13 +93,23 @@ mb_flags = 0x0
>  	.long mb_magic, mb_flags, 0 - (mb_magic + mb_flags)
>  mb_cmdline = 16
> 
> -MSR_GS_BASE = 0xc0000101
> -
>  .macro setup_percpu_area
>  	lea -4096(%esp), %eax
> -	mov $0, %edx
> -	mov $MSR_GS_BASE, %ecx
> -	wrmsr
> +
> +	/* fill GS_BASE in the GDT */
> +	mov $(APIC_DEFAULT_PHYS_BASE + APIC_ID), %ebx

Using %ebx crushes the mbi_bootinfo pointer.  The easiest fix is to use %edx or
%ecx.

> +	mov (%ebx), %ebx

No need to load the address into a reg, just drop the "$" above and encode
"mov [imm32], <reg>".

Want to fold this into your patch?

diff --git a/x86/cstart.S b/x86/cstart.S
index 7d9ed96..fb6eda5 100644
--- a/x86/cstart.S
+++ b/x86/cstart.S
@@ -97,17 +97,16 @@ mb_cmdline = 16
        lea -4096(%esp), %eax

        /* fill GS_BASE in the GDT */
-       mov $(APIC_DEFAULT_PHYS_BASE + APIC_ID), %ebx
-       mov (%ebx), %ebx
-       shr $24, %ebx
-       or %ax, percpu_descr+2(,%ebx,8)
+       mov (APIC_DEFAULT_PHYS_BASE + APIC_ID), %edx
+       shr $24, %edx
+       or %ax, percpu_descr+2(,%edx,8)

        shr $16, %eax
-       or %al, percpu_descr+4(,%ebx,8)
-       or %ah, percpu_descr+7(,%ebx,8)
+       or %al, percpu_descr+4(,%edx,8)
+       or %ah, percpu_descr+7(,%edx,8)

        lgdtl gdt32_descr
-       lea percpu_descr-gdt32(,%ebx,8), %eax
+       lea percpu_descr-gdt32(,%edx,8), %eax
        mov %ax, %gs

 .endm

> +	shr $24, %ebx
> +	or %ax, percpu_descr+2(,%ebx,8)
> +
> +	shr $16, %eax
> +	or %al, percpu_descr+4(,%ebx,8)
> +	or %ah, percpu_descr+7(,%ebx,8)
> +
> +	lgdtl gdt32_descr
> +	lea percpu_descr-gdt32(,%ebx,8), %eax
> +	mov %ax, %gs
> +
>  .endm
> 
>  .macro setup_segments
> @@ -188,16 +202,14 @@ load_tss:
>  	mov (%eax), %eax
>  	shr $24, %eax
>  	mov %eax, %ebx
> -	shl $3, %ebx
>  	mov $((tss_end - tss) / max_cpus), %edx
>  	imul %edx
>  	add $tss, %eax
> -	mov %ax, tss_descr+2(%ebx)
> +	mov %ax, tss_descr+2(,%ebx,8)
>  	shr $16, %eax
> -	mov %al, tss_descr+4(%ebx)
> -	shr $8, %eax
> -	mov %al, tss_descr+7(%ebx)
> -	lea tss_descr-gdt32(%ebx), %eax
> +	mov %al, tss_descr+4(,%ebx,8)
> +	mov %ah, tss_descr+7(,%ebx,8)

Is there a functional change here?  If not, can you throw this into a separate
patch?

Thanks!

> +	lea tss_descr-gdt32(,%ebx,8), %eax
>  	ltr %ax
>  	ret
> 
> 
> Paolo
> 

Re: [kvm-unit-tests PATCH 01/14] x86/cstart: Don't use MSR_GS_BASE in 32-bit boot code

[Paolo Bonzini]

On 22/04/21 19:57, Sean Christopherson wrote:
> On Thu, Apr 22, 2021, Paolo Bonzini wrote:
>> On 22/04/21 05:04, Sean Christopherson wrote:
>>> Load the per-cpu GS.base for 32-bit build by building a temporary GDT
>>> and loading a "real" segment.  Using MSR_GS_BASE is wrong and broken,
>>> it's a 64-bit only MSR and does not exist on 32-bit CPUs.  The current
>>> code works only because 32-bit KVM VMX incorrectly disables interception
>>> of MSR_GS_BASE, and no one runs KVM on an actual 32-bit physical CPU,
>>> i.e. the MSR exists in hardware and so everything "works".
>>>
>>> 32-bit KVM SVM is not buggy and correctly injects #GP on the WRMSR, i.e.
>>> the tests have never worked on 32-bit SVM.
>>
>> Hmm, this breaks task switch.  But setting up separate descriptors is
>> not hard:
> 
> Much better.
> 
> Using %ebx crushes the mbi_bootinfo pointer.  The easiest fix is to use %edx or
> %ecx.
> 
>> +	mov (%ebx), %ebx
> 
> No need to load the address into a reg, just drop the "$" above and encode
> "mov [imm32], <reg>".

Yep, I had already fixed more or less the same things (plus the task 
gate TSS setup, which must not hardcode GS to 0x10; no idea how it 
worked before) before seeing your mail.  I sent the result to the 
mailing list.

Paolo