[kvm-unit-tests PATCH 0/5] Collection of tests for canonical checks on LA57 enabled CPUs

[kvm-unit-tests PATCH 0/5] Collection of tests for canonical checks on LA57 enabled CPUs

[Maxim Levitsky]

This is a set of tests that checks KVM and CPU behaviour in regard to
canonical checks of various msrs, segment bases, instructions that
were found to ignore CR4.LA57 on CPUs that support 5 level paging.

Best regards,
	Maxim Levitsky

Maxim Levitsky (5):
  x86: add _safe and _fep_safe variants to segment base load
    instructions
  x86: add a few functions for gdt manipulation
  x86: move struct invpcid_desc descriptor to processor.h
  Add a test for writing canonical values to various msrs and fields
  nVMX: add a test for canonical checks of various host state vmcs12
    fields.

 lib/x86/desc.c      |  39 ++++-
 lib/x86/desc.h      |   9 +-
 lib/x86/msr.h       |  42 ++++++
 lib/x86/processor.h |  58 +++++++-
 x86/Makefile.x86_64 |   1 +
 x86/canonical_57.c  | 346 ++++++++++++++++++++++++++++++++++++++++++++
 x86/pcid.c          |   6 -
 x86/vmx_tests.c     | 183 +++++++++++++++++++++++
 8 files changed, 667 insertions(+), 17 deletions(-)
 create mode 100644 x86/canonical_57.c

-- 
2.26.3

[kvm-unit-tests PATCH 1/5] x86: add _safe and _fep_safe variants to segment base load instructions

[Maxim Levitsky]

_safe and _fep_safe functions will be used to validate various ways of
setting the segment bases and GDT/LDT bases.

Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 lib/x86/desc.h      |  4 ++--
 lib/x86/processor.h | 45 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 47 insertions(+), 2 deletions(-)

diff --git a/lib/x86/desc.h b/lib/x86/desc.h
index 92c45a48f..5349ea572 100644
--- a/lib/x86/desc.h
+++ b/lib/x86/desc.h
@@ -286,8 +286,8 @@ extern unsigned long get_gdt_entry_limit(gdt_entry_t *entry);
 #define asm_safe(insn, inputs...)					\
 	__asm_safe("", insn, inputs)
 
-#define asm_fep_safe(insn, output, inputs...)				\
-	__asm_safe_out1(KVM_FEP, insn, output, inputs)
+#define asm_fep_safe(insn, inputs...)				\
+	__asm_safe_out1(KVM_FEP, insn,, inputs)
 
 #define __asm_safe_out1(fep, insn, output, inputs...)			\
 ({									\
diff --git a/lib/x86/processor.h b/lib/x86/processor.h
index da1ed6628..9248a06b2 100644
--- a/lib/x86/processor.h
+++ b/lib/x86/processor.h
@@ -468,6 +468,11 @@ static inline int rdmsr_safe(u32 index, uint64_t *val)
 	return rdreg64_safe("rdmsr", index, val);
 }
 
+static inline int rdmsr_fep_safe(u32 index, uint64_t *val)
+{
+	return __rdreg64_safe(KVM_FEP, "rdmsr", index, val);
+}
+
 static inline int wrmsr_safe(u32 index, u64 val)
 {
 	return wrreg64_safe("wrmsr", index, val);
@@ -597,6 +602,16 @@ static inline void lgdt(const struct descriptor_table_ptr *ptr)
 	asm volatile ("lgdt %0" : : "m"(*ptr));
 }
 
+static inline int lgdt_safe(const struct descriptor_table_ptr *ptr)
+{
+	return asm_safe("lgdt %0", "m"(*ptr));
+}
+
+static inline int lgdt_fep_safe(const struct descriptor_table_ptr *ptr)
+{
+	return asm_fep_safe("lgdt %0", "m"(*ptr));
+}
+
 static inline void sgdt(struct descriptor_table_ptr *ptr)
 {
 	asm volatile ("sgdt %0" : "=m"(*ptr));
@@ -607,6 +622,16 @@ static inline void lidt(const struct descriptor_table_ptr *ptr)
 	asm volatile ("lidt %0" : : "m"(*ptr));
 }
 
+static inline int lidt_safe(const struct descriptor_table_ptr *ptr)
+{
+	return asm_safe("lidt %0", "m"(*ptr));
+}
+
+static inline int lidt_fep_safe(const struct descriptor_table_ptr *ptr)
+{
+	return asm_fep_safe("lidt %0", "m"(*ptr));
+}
+
 static inline void sidt(struct descriptor_table_ptr *ptr)
 {
 	asm volatile ("sidt %0" : "=m"(*ptr));
@@ -617,6 +642,16 @@ static inline void lldt(u16 val)
 	asm volatile ("lldt %0" : : "rm"(val));
 }
 
+static inline int lldt_safe(u16 val)
+{
+	return asm_safe("lldt %0", "rm"(val));
+}
+
+static inline int lldt_fep_safe(u16 val)
+{
+	return asm_safe("lldt %0", "rm"(val));
+}
+
 static inline u16 sldt(void)
 {
 	u16 val;
@@ -629,6 +664,16 @@ static inline void ltr(u16 val)
 	asm volatile ("ltr %0" : : "rm"(val));
 }
 
+static inline int ltr_safe(u16 val)
+{
+	return asm_safe("ltr %0", "rm"(val));
+}
+
+static inline int ltr_fep_safe(u16 val)
+{
+	return asm_safe("ltr %0", "rm"(val));
+}
+
 static inline u16 str(void)
 {
 	u16 val;
-- 
2.26.3

[kvm-unit-tests PATCH 2/5] x86: add a few functions for gdt manipulation

[Maxim Levitsky]

Add a few functions that will be used to manipulate various
segment bases that are loaded via GDT.

Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 lib/x86/desc.c | 39 ++++++++++++++++++++++++++++++++-------
 lib/x86/desc.h |  5 +++++
 2 files changed, 37 insertions(+), 7 deletions(-)

diff --git a/lib/x86/desc.c b/lib/x86/desc.c
index d054899c6..52e33f201 100644
--- a/lib/x86/desc.c
+++ b/lib/x86/desc.c
@@ -338,7 +338,7 @@ bool exception_rflags_rf(void)
 
 static char intr_alt_stack[4096];
 
-void set_gdt_entry(int sel, unsigned long base,  u32 limit, u8 type, u8 flags)
+void set_gdt_entry_base(int sel, unsigned long base)
 {
 	gdt_entry_t *entry = &gdt[sel >> 3];
 
@@ -347,10 +347,6 @@ void set_gdt_entry(int sel, unsigned long base,  u32 limit, u8 type, u8 flags)
 	entry->base2 = (base >> 16) & 0xFF;
 	entry->base3 = (base >> 24) & 0xFF;
 
-	/* Setup the descriptor limits, type and flags */
-	entry->limit1 = (limit & 0xFFFF);
-	entry->type_limit_flags = ((limit & 0xF0000) >> 8) | ((flags & 0xF0) << 8) | type;
-
 #ifdef __x86_64__
 	if (!entry->s) {
 		struct system_desc64 *entry16 = (struct system_desc64 *)entry;
@@ -360,6 +356,25 @@ void set_gdt_entry(int sel, unsigned long base,  u32 limit, u8 type, u8 flags)
 #endif
 }
 
+void set_gdt_entry(int sel, unsigned long base,  u32 limit, u8 type, u8 flags)
+{
+	gdt_entry_t *entry = &gdt[sel >> 3];
+
+	/* Setup the descriptor limits, type and flags */
+	entry->limit1 = (limit & 0xFFFF);
+	entry->type_limit_flags = ((limit & 0xF0000) >> 8) | ((flags & 0xF0) << 8) | type;
+	set_gdt_entry_base(sel, base);
+}
+
+void clear_tss_busy(int sel)
+{
+	gdt_entry_t *entry = &gdt[sel >> 3];
+
+	entry->type_limit_flags &= ~0xFF;
+	entry->type_limit_flags |= 0x89;
+}
+
+
 void load_gdt_tss(size_t tss_offset)
 {
 	lgdt(&gdt_descr);
@@ -483,14 +498,24 @@ void __set_exception_jmpbuf(jmp_buf *addr)
 	exception_jmpbuf = addr;
 }
 
-gdt_entry_t *get_tss_descr(void)
+gdt_entry_t *get_gdt_entry(u16 sel)
 {
 	struct descriptor_table_ptr gdt_ptr;
 	gdt_entry_t *gdt;
 
 	sgdt(&gdt_ptr);
 	gdt = (gdt_entry_t *)gdt_ptr.base;
-	return &gdt[str() / 8];
+	return &gdt[sel / 8];
+}
+
+gdt_entry_t *get_tss_descr(void)
+{
+	return get_gdt_entry(str());
+}
+
+gdt_entry_t *get_ldt_descr(void)
+{
+	return get_gdt_entry(sldt());
 }
 
 unsigned long get_gdt_entry_base(gdt_entry_t *entry)
diff --git a/lib/x86/desc.h b/lib/x86/desc.h
index 5349ea572..a50c8f61b 100644
--- a/lib/x86/desc.h
+++ b/lib/x86/desc.h
@@ -246,6 +246,8 @@ void set_idt_entry(int vec, void *addr, int dpl);
 void set_idt_sel(int vec, u16 sel);
 void set_idt_dpl(int vec, u16 dpl);
 void set_gdt_entry(int sel, unsigned long base, u32 limit, u8 access, u8 gran);
+void set_gdt_entry_base(int sel, unsigned long base);
+void clear_tss_busy(int sel);
 void load_gdt_tss(size_t tss_offset);
 void set_intr_alt_stack(int e, void *fn);
 void print_current_tss_info(void);
@@ -268,7 +270,10 @@ static inline void *get_idt_addr(idt_entry_t *entry)
 	return (void *)addr;
 }
 
+extern gdt_entry_t *get_gdt_entry(u16 sel);
 extern gdt_entry_t *get_tss_descr(void);
+gdt_entry_t *get_ldt_descr(void);
+
 extern unsigned long get_gdt_entry_base(gdt_entry_t *entry);
 extern unsigned long get_gdt_entry_limit(gdt_entry_t *entry);
 
-- 
2.26.3

[kvm-unit-tests PATCH 3/5] x86: move struct invpcid_desc descriptor to processor.h

[Maxim Levitsky]

Move struct invpcid_desc descriptor to processor.h so that
it can be used in tests that are external to pcid.c

Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 lib/x86/processor.h | 7 ++++++-
 x86/pcid.c          | 6 ------
 2 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/lib/x86/processor.h b/lib/x86/processor.h
index 9248a06b2..bb54ec610 100644
--- a/lib/x86/processor.h
+++ b/lib/x86/processor.h
@@ -836,8 +836,13 @@ static inline void invlpg(volatile void *va)
 	asm volatile("invlpg (%0)" ::"r" (va) : "memory");
 }
 
+struct invpcid_desc {
+	u64 pcid : 12;
+	u64 rsv  : 52;
+	u64 addr : 64;
+};
 
-static inline int invpcid_safe(unsigned long type, void *desc)
+static inline int invpcid_safe(unsigned long type, struct invpcid_desc *desc)
 {
 	/* invpcid (%rax), %rbx */
 	return asm_safe(".byte 0x66,0x0f,0x38,0x82,0x18", "a" (desc), "b" (type));
diff --git a/x86/pcid.c b/x86/pcid.c
index c503efb83..7425e0fe8 100644
--- a/x86/pcid.c
+++ b/x86/pcid.c
@@ -4,12 +4,6 @@
 #include "processor.h"
 #include "desc.h"
 
-struct invpcid_desc {
-    u64 pcid : 12;
-    u64 rsv  : 52;
-    u64 addr : 64;
-};
-
 static void test_pcid_enabled(void)
 {
     int passed = 0;
-- 
2.26.3

[kvm-unit-tests PATCH 4/5] Add a test for writing canonical values to various msrs and fields

[Maxim Levitsky]

Add a test that thoroughly tests the canonical checks
that are done when setting various msrs and cpu registers,
especially on CPUs that support 5 level paging.

Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 lib/x86/msr.h       |  42 ++++++
 lib/x86/processor.h |   6 +-
 x86/Makefile.x86_64 |   1 +
 x86/canonical_57.c  | 346 ++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 394 insertions(+), 1 deletion(-)
 create mode 100644 x86/canonical_57.c

diff --git a/lib/x86/msr.h b/lib/x86/msr.h
index 8abccf867..658d237fd 100644
--- a/lib/x86/msr.h
+++ b/lib/x86/msr.h
@@ -131,6 +131,48 @@
 #define MSR_P6_EVNTSEL0			0x00000186
 #define MSR_P6_EVNTSEL1			0x00000187
 
+#define MSR_IA32_RTIT_CTL		0x00000570
+#define RTIT_CTL_TRACEEN		BIT(0)
+#define RTIT_CTL_CYCLEACC		BIT(1)
+#define RTIT_CTL_OS			BIT(2)
+#define RTIT_CTL_USR			BIT(3)
+#define RTIT_CTL_PWR_EVT_EN		BIT(4)
+#define RTIT_CTL_FUP_ON_PTW		BIT(5)
+#define RTIT_CTL_FABRIC_EN		BIT(6)
+#define RTIT_CTL_CR3EN			BIT(7)
+#define RTIT_CTL_TOPA			BIT(8)
+#define RTIT_CTL_MTC_EN			BIT(9)
+#define RTIT_CTL_TSC_EN			BIT(10)
+#define RTIT_CTL_DISRETC		BIT(11)
+#define RTIT_CTL_PTW_EN			BIT(12)
+#define RTIT_CTL_BRANCH_EN		BIT(13)
+#define RTIT_CTL_EVENT_EN		BIT(31)
+#define RTIT_CTL_NOTNT			BIT_ULL(55)
+#define RTIT_CTL_MTC_RANGE_OFFSET	14
+#define RTIT_CTL_MTC_RANGE		(0x0full << RTIT_CTL_MTC_RANGE_OFFSET)
+#define RTIT_CTL_CYC_THRESH_OFFSET	19
+#define RTIT_CTL_CYC_THRESH		(0x0full << RTIT_CTL_CYC_THRESH_OFFSET)
+#define RTIT_CTL_PSB_FREQ_OFFSET	24
+#define RTIT_CTL_PSB_FREQ		(0x0full << RTIT_CTL_PSB_FREQ_OFFSET)
+#define RTIT_CTL_ADDR0_OFFSET		32
+#define RTIT_CTL_ADDR0			(0x0full << RTIT_CTL_ADDR0_OFFSET)
+#define RTIT_CTL_ADDR1_OFFSET		36
+#define RTIT_CTL_ADDR1			(0x0full << RTIT_CTL_ADDR1_OFFSET)
+#define RTIT_CTL_ADDR2_OFFSET		40
+#define RTIT_CTL_ADDR2			(0x0full << RTIT_CTL_ADDR2_OFFSET)
+#define RTIT_CTL_ADDR3_OFFSET		44
+#define RTIT_CTL_ADDR3			(0x0full << RTIT_CTL_ADDR3_OFFSET)
+
+
+#define MSR_IA32_RTIT_ADDR0_A		0x00000580
+#define MSR_IA32_RTIT_ADDR0_B		0x00000581
+#define MSR_IA32_RTIT_ADDR1_A		0x00000582
+#define MSR_IA32_RTIT_ADDR1_B		0x00000583
+#define MSR_IA32_RTIT_ADDR2_A		0x00000584
+#define MSR_IA32_RTIT_ADDR2_B		0x00000585
+#define MSR_IA32_RTIT_ADDR3_A		0x00000586
+#define MSR_IA32_RTIT_ADDR3_B		0x00000587
+
 /* AMD64 MSRs. Not complete. See the architecture manual for a more
    complete list. */
 
diff --git a/lib/x86/processor.h b/lib/x86/processor.h
index bb54ec610..f05175af5 100644
--- a/lib/x86/processor.h
+++ b/lib/x86/processor.h
@@ -7,7 +7,9 @@
 #include <bitops.h>
 #include <stdint.h>
 
-#define NONCANONICAL	0xaaaaaaaaaaaaaaaaull
+#define CANONICAL_48_VAL 0xffffaaaaaaaaaaaaull
+#define CANONICAL_57_VAL 0xffaaaaaaaaaaaaaaull
+#define NONCANONICAL	 0xaaaaaaaaaaaaaaaaull
 
 #ifdef __x86_64__
 #  define R "r"
@@ -241,6 +243,7 @@ static inline bool is_intel(void)
 #define	X86_FEATURE_MCE			(CPUID(0x1, 0, EDX, 7))
 #define	X86_FEATURE_APIC		(CPUID(0x1, 0, EDX, 9))
 #define	X86_FEATURE_CLFLUSH		(CPUID(0x1, 0, EDX, 19))
+#define	X86_FEATURE_DS			(CPUID(0x1, 0, EDX, 21))
 #define	X86_FEATURE_XMM			(CPUID(0x1, 0, EDX, 25))
 #define	X86_FEATURE_XMM2		(CPUID(0x1, 0, EDX, 26))
 #define	X86_FEATURE_TSC_ADJUST		(CPUID(0x7, 0, EBX, 1))
@@ -252,6 +255,7 @@ static inline bool is_intel(void)
 #define	X86_FEATURE_PCOMMIT		(CPUID(0x7, 0, EBX, 22))
 #define	X86_FEATURE_CLFLUSHOPT		(CPUID(0x7, 0, EBX, 23))
 #define	X86_FEATURE_CLWB		(CPUID(0x7, 0, EBX, 24))
+#define X86_FEATURE_INTEL_PT		(CPUID(0x7, 0, EBX, 25))
 #define	X86_FEATURE_UMIP		(CPUID(0x7, 0, ECX, 2))
 #define	X86_FEATURE_PKU			(CPUID(0x7, 0, ECX, 3))
 #define	X86_FEATURE_LA57		(CPUID(0x7, 0, ECX, 16))
diff --git a/x86/Makefile.x86_64 b/x86/Makefile.x86_64
index 2771a6fad..0a7eb2c34 100644
--- a/x86/Makefile.x86_64
+++ b/x86/Makefile.x86_64
@@ -38,6 +38,7 @@ tests += $(TEST_DIR)/rdpru.$(exe)
 tests += $(TEST_DIR)/pks.$(exe)
 tests += $(TEST_DIR)/pmu_lbr.$(exe)
 tests += $(TEST_DIR)/pmu_pebs.$(exe)
+tests += $(TEST_DIR)/canonical_57.$(exe)
 
 ifeq ($(CONFIG_EFI),y)
 tests += $(TEST_DIR)/amd_sev.$(exe)
diff --git a/x86/canonical_57.c b/x86/canonical_57.c
new file mode 100644
index 000000000..a2f2438b5
--- /dev/null
+++ b/x86/canonical_57.c
@@ -0,0 +1,346 @@
+#include "libcflat.h"
+#include "apic.h"
+#include "processor.h"
+#include "msr.h"
+#include "x86/vm.h"
+#include "asm/setup.h"
+
+enum TEST_REGISTER {
+	TEST_REGISTER_GDTR_BASE,
+	TEST_REGISTER_IDTR_BASE,
+	TEST_REGISTER_TR_BASE,
+	TEST_REGISTER_LDT_BASE,
+	TEST_REGISTER_MSR /* upper 32 bits = msr address */
+};
+
+static u64 get_test_register_value(u64 test_register)
+{
+	struct descriptor_table_ptr dt_ptr;
+	u32 msr = test_register >> 32;
+
+	/*
+	 * Note: value for LDT and TSS base might not reflect the actual base
+	 * that the CPU currently uses, because the (hidden) base value can't be
+	 * directly read.
+	 */
+
+	switch ((u32)test_register) {
+	case TEST_REGISTER_GDTR_BASE:
+		sgdt(&dt_ptr);
+		return  dt_ptr.base;
+	case TEST_REGISTER_IDTR_BASE:
+		sidt(&dt_ptr);
+		return dt_ptr.base;
+	case TEST_REGISTER_TR_BASE:
+		return get_gdt_entry_base(get_tss_descr());
+	case TEST_REGISTER_LDT_BASE:
+		return get_gdt_entry_base(get_ldt_descr());
+	case TEST_REGISTER_MSR:
+		return rdmsr(msr);
+	default:
+		assert(0);
+		return 0;
+	}
+}
+
+enum SET_REGISTER_MODE {
+	SET_REGISTER_MODE_UNSAFE,
+	SET_REGISTER_MODE_SAFE,
+	SET_REGISTER_MODE_FEP,
+};
+
+static bool set_test_register_value(u64 test_register, int test_mode, u64 value)
+{
+	struct descriptor_table_ptr dt_ptr;
+	u32 msr = test_register >> 32;
+	u16 sel;
+
+	switch ((u32)test_register) {
+	case TEST_REGISTER_GDTR_BASE:
+		sgdt(&dt_ptr);
+		dt_ptr.base = value;
+
+		switch (test_mode) {
+		case SET_REGISTER_MODE_UNSAFE:
+			lgdt(&dt_ptr);
+			return true;
+		case SET_REGISTER_MODE_SAFE:
+			return lgdt_safe(&dt_ptr) == 0;
+		case SET_REGISTER_MODE_FEP:
+			return lgdt_fep_safe(&dt_ptr) == 0;
+		}
+	case TEST_REGISTER_IDTR_BASE:
+		sidt(&dt_ptr);
+		dt_ptr.base = value;
+
+		switch (test_mode) {
+		case SET_REGISTER_MODE_UNSAFE:
+			lidt(&dt_ptr);
+			return true;
+		case SET_REGISTER_MODE_SAFE:
+			return lidt_safe(&dt_ptr) == 0;
+		case SET_REGISTER_MODE_FEP:
+			return lidt_fep_safe(&dt_ptr) == 0;
+		}
+	case TEST_REGISTER_TR_BASE:
+		sel = str();
+		set_gdt_entry_base(sel, value);
+		clear_tss_busy(sel);
+
+		switch (test_mode) {
+		case SET_REGISTER_MODE_UNSAFE:
+			ltr(sel);
+			return true;
+		case SET_REGISTER_MODE_SAFE:
+			return ltr_safe(sel) == 0;
+		case SET_REGISTER_MODE_FEP:
+			return ltr_fep_safe(sel) == 0;
+		}
+
+	case TEST_REGISTER_LDT_BASE:
+		sel = sldt();
+		set_gdt_entry_base(sel, value);
+
+		switch (test_mode) {
+		case SET_REGISTER_MODE_UNSAFE:
+			lldt(sel);
+			return true;
+		case SET_REGISTER_MODE_SAFE:
+			return lldt_safe(sel) == 0;
+		case SET_REGISTER_MODE_FEP:
+			return lldt_fep_safe(sel) == 0;
+		}
+	case TEST_REGISTER_MSR:
+		switch (test_mode) {
+		case SET_REGISTER_MODE_UNSAFE:
+			wrmsr(msr, value);
+			return true;
+		case SET_REGISTER_MODE_SAFE:
+			return wrmsr_safe(msr, value) == 0;
+		case SET_REGISTER_MODE_FEP:
+			return wrmsr_fep_safe(msr, value) == 0;
+		}
+	default:
+		assert(false);
+		return 0;
+	}
+}
+
+static void test_register_write(const char *register_name, u64 test_register,
+				bool force_emulation, u64 test_value,
+				bool expect_success)
+{
+	u64 old_value, expected_value;
+	bool success, test_passed = false;
+	int test_mode = (force_emulation ? SET_REGISTER_MODE_FEP : SET_REGISTER_MODE_SAFE);
+
+	old_value = get_test_register_value(test_register);
+	expected_value = expect_success ? test_value : old_value;
+
+	/*
+	 * TODO: Successful write to the MSR_GS_BASE corrupts it,
+	 * and that breaks the wrmsr_safe macro.
+	 */
+	if ((test_register >> 32) == MSR_GS_BASE && expect_success)
+		test_mode = SET_REGISTER_MODE_UNSAFE;
+
+	/* Write the test value*/
+	success =  set_test_register_value(test_register, test_mode, test_value);
+
+	if (success != expect_success) {
+		report(false,
+		       "Write of test register %s with value %lx unexpectedly %s",
+		       register_name, test_value,
+		       (success ? "succeeded" : "failed"));
+		goto exit;
+	}
+
+	/*
+	 * Check that the value was really written.
+	 * Don't test TR and LDTR, because it's not possible to read them
+	 * directly.
+	 */
+
+	if (test_register != TEST_REGISTER_TR_BASE &&
+	    test_register != TEST_REGISTER_LDT_BASE) {
+		u64 new_value = get_test_register_value(test_register);
+
+		if (new_value != expected_value) {
+			report(false,
+			       "Register %s wasn't set to %lx as expected (actual value %lx)",
+			       register_name, expected_value, new_value);
+			goto exit;
+		}
+	}
+
+	/*
+	 * Restore the old value directly without safety wrapper,
+	 * to avoid test crashes related to temporary clobbered GDT/IDT/etc bases.
+	 */
+
+	set_test_register_value(test_register, SET_REGISTER_MODE_UNSAFE, old_value);
+	test_passed = true;
+exit:
+	report(test_passed, "Tested setting %s to 0x%lx value - %s", register_name,
+	       test_value, success ? "success" : "failure");
+}
+
+static void test_register(const char *register_name, u64 test_register,
+			  bool force_emulation)
+{
+	/* Canonical 48 bit value should always succeed */
+	test_register_write(register_name, test_register, force_emulation,
+			    CANONICAL_48_VAL, true);
+
+	/* 57-canonical value will work on CPUs that *support* LA57 */
+	test_register_write(register_name, test_register, force_emulation,
+			    CANONICAL_57_VAL, this_cpu_has(X86_FEATURE_LA57));
+
+	/* Non 57 canonical value should never work */
+	test_register_write(register_name, test_register, force_emulation,
+			    NONCANONICAL, false);
+}
+
+
+#define TEST_REGISTER(register_name, force_emulation) \
+		      test_register(#register_name, register_name, force_emulation)
+
+#define __TEST_MSR(msr_name, address, force_emulation) \
+		   test_register(msr_name, ((u64)TEST_REGISTER_MSR |  \
+		   ((u64)(address) << 32)), force_emulation)
+
+#define TEST_MSR(msr_name, force_emulation) \
+	__TEST_MSR(#msr_name, msr_name, force_emulation)
+
+static void __test_invpcid(u64 test_value, bool expect_success)
+{
+	struct invpcid_desc desc;
+
+	memset(&desc, 0, sizeof(desc));
+	bool success;
+
+	desc.addr = test_value;
+	desc.pcid = 10; /* Arbitrary number*/
+
+	success = invpcid_safe(0, &desc) == 0;
+
+	report(success == expect_success,
+	       "Tested invpcid type 0 with 0x%lx value - %s",
+	       test_value, success ? "success" : "failure");
+}
+
+static void test_invpcid(void)
+{
+	/*
+	 * Note that this test tests the kvm's behavior only when ept=0.
+	 * Otherwise invpcid is not intercepted.
+	 *
+	 * Also KVM's x86 emulator doesn't support invpcid, thus testing invpcid
+	 * with FEP is pointless.
+	 */
+
+	assert(write_cr4_safe(read_cr4() | X86_CR4_PCIDE) == 0);
+
+	__test_invpcid(CANONICAL_48_VAL, true);
+	__test_invpcid(CANONICAL_57_VAL, this_cpu_has(X86_FEATURE_LA57));
+	__test_invpcid(NONCANONICAL, false);
+}
+
+static void __do_test(bool force_emulation)
+{
+	/* Direct DT addresses */
+	TEST_REGISTER(TEST_REGISTER_GDTR_BASE, force_emulation);
+	TEST_REGISTER(TEST_REGISTER_IDTR_BASE, force_emulation);
+
+	/* Indirect DT addresses */
+	TEST_REGISTER(TEST_REGISTER_TR_BASE, force_emulation);
+	TEST_REGISTER(TEST_REGISTER_LDT_BASE, force_emulation);
+
+	/* x86_64 extended segment bases */
+	TEST_MSR(MSR_FS_BASE, force_emulation);
+	TEST_MSR(MSR_GS_BASE, force_emulation);
+	TEST_MSR(MSR_KERNEL_GS_BASE, force_emulation);
+
+	/*
+	 * SYSENTER ESP/EIP MSRs have canonical checks only on Intel,
+	 * because only on Intel these instructions were extended to 64 bit.
+	 *
+	 * TODO: KVM emulation however ignores canonical checks for these MSRs
+	 * even on Intel, to support cross-vendor migration.
+	 *
+	 * Thus only run the check on bare metal.
+	 *
+	 */
+	if (is_intel() && !force_emulation) {
+		TEST_MSR(MSR_IA32_SYSENTER_ESP, force_emulation);
+		TEST_MSR(MSR_IA32_SYSENTER_EIP, force_emulation);
+	} else
+		report_skip("skipping MSR_IA32_SYSENTER_ESP/MSR_IA32_SYSENTER_EIP %s",
+			    (is_intel() ? "due to known errata in KVM": "due to AMD host"));
+
+	/*  SYSCALL target MSRs */
+	TEST_MSR(MSR_CSTAR, force_emulation);
+	TEST_MSR(MSR_LSTAR, force_emulation);
+
+	/* PEBS DS area */
+	if (this_cpu_has(X86_FEATURE_DS))
+		TEST_MSR(MSR_IA32_DS_AREA, force_emulation);
+	else
+		report_skip("Skipping MSR_IA32_DS_AREA - PEBS not supported");
+
+	/* PT filter ranges */
+	if (this_cpu_has(X86_FEATURE_INTEL_PT)) {
+		int n_ranges = cpuid_indexed(0x14, 0x1).a & 0x7;
+		int i;
+
+		for (i = 0 ; i < n_ranges ; i++) {
+			wrmsr(MSR_IA32_RTIT_CTL, (1ull << (RTIT_CTL_ADDR0_OFFSET+i*4)));
+			__TEST_MSR("MSR_IA32_RTIT_ADDR_A",
+				   MSR_IA32_RTIT_ADDR0_A + i*2, force_emulation);
+			__TEST_MSR("MSR_IA32_RTIT_ADDR_B",
+				   MSR_IA32_RTIT_ADDR0_B + i*2, force_emulation);
+		}
+	} else
+		report_skip("Skipping MSR_IA32_RTIT_ADDR* - Intel PT is not supported");
+
+	/* Test that INVPCID type 0 #GPs correctly */
+	if (this_cpu_has(X86_FEATURE_INVPCID))
+		test_invpcid();
+	else
+		report_skip("Skipping INVPCID - not supported");
+}
+
+static void do_test(void)
+{
+	printf("\n");
+	printf("Running the test without emulation:\n");
+	__do_test(false);
+
+	printf("\n");
+
+	if (is_fep_available()) {
+		printf("Running the test with forced emulation:\n");
+		__do_test(true);
+	} else
+		report_skip("force emulation prefix not enabled - skipping");
+}
+
+int main(int ac, char **av)
+{
+	/* set dummy LDTR pointer */
+	set_gdt_entry(FIRST_SPARE_SEL, 0xffaabb, 0xffff, 0x82, 0);
+	lldt(FIRST_SPARE_SEL);
+
+	do_test();
+
+	printf("\n");
+
+	if (this_cpu_has(X86_FEATURE_LA57)) {
+		printf("Switching to 5 level paging mode and rerunning the test...\n");
+		setup_5level_page_table();
+		do_test();
+	} else
+		report_skip("Skipping the test in 5-level paging mode - not supported on the host");
+
+	return report_summary();
+}
-- 
2.26.3

[kvm-unit-tests PATCH 5/5] nVMX: add a test for canonical checks of various host state vmcs12 fields.

[Maxim Levitsky]

This test tests canonical VM entry checks of various host state fields
(mostly segment bases) in vmcs12.

Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 x86/vmx_tests.c | 183 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 183 insertions(+)

diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
index ffe7064c9..b6f3d634d 100644
--- a/x86/vmx_tests.c
+++ b/x86/vmx_tests.c
@@ -10732,6 +10732,7 @@ static void handle_exception_in_l1(u32 vector)
 	vmcs_write(EXC_BITMAP, old_eb);
 }
 
+
 static void vmx_exception_test(void)
 {
 	struct vmx_exception_test *t;
@@ -10754,6 +10755,187 @@ static void vmx_exception_test(void)
 	test_set_guest_finished();
 }
 
+
+#define TEST_VALUE1  0xff45454545000000
+#define TEST_VALUE2  0xff55555555000000
+
+static void vmx_canonical_test_guest(void)
+{
+	while (true)
+		vmcall();
+}
+
+static int get_host_value(u64 vmcs_field, u64 *value)
+{
+	struct descriptor_table_ptr dt_ptr;
+
+	switch (vmcs_field) {
+	case HOST_SYSENTER_ESP:
+		*value = rdmsr(MSR_IA32_SYSENTER_ESP);
+		break;
+	case HOST_SYSENTER_EIP:
+		*value =  rdmsr(MSR_IA32_SYSENTER_EIP);
+		break;
+	case HOST_BASE_FS:
+		*value =  rdmsr(MSR_FS_BASE);
+		break;
+	case HOST_BASE_GS:
+		*value =  rdmsr(MSR_GS_BASE);
+		break;
+	case HOST_BASE_GDTR:
+		sgdt(&dt_ptr);
+		*value =  dt_ptr.base;
+		break;
+	case HOST_BASE_IDTR:
+		sidt(&dt_ptr);
+		*value =  dt_ptr.base;
+		break;
+	case HOST_BASE_TR:
+		*value = get_gdt_entry_base(get_tss_descr());
+		/* value might not reflect the actual base if changed by VMX */
+		return 1;
+	default:
+		assert(0);
+	}
+	return 0;
+}
+
+static int set_host_value(u64 vmcs_field, u64 value)
+{
+	struct descriptor_table_ptr dt_ptr;
+
+	switch (vmcs_field) {
+	case HOST_SYSENTER_ESP:
+		return wrmsr_safe(MSR_IA32_SYSENTER_ESP, value);
+	case HOST_SYSENTER_EIP:
+		return wrmsr_safe(MSR_IA32_SYSENTER_EIP, value);
+	case HOST_BASE_FS:
+		return wrmsr_safe(MSR_FS_BASE, value);
+	case HOST_BASE_GS:
+		/* TODO: _safe variants assume per-cpu gs base*/
+		wrmsr(MSR_GS_BASE, value);
+		return 0;
+	case HOST_BASE_GDTR:
+		sgdt(&dt_ptr);
+		dt_ptr.base = value;
+		lgdt(&dt_ptr);
+		return lgdt_fep_safe(&dt_ptr);
+	case HOST_BASE_IDTR:
+		sidt(&dt_ptr);
+		dt_ptr.base = value;
+		return lidt_fep_safe(&dt_ptr);
+	case HOST_BASE_TR:
+		/* Set the base and clear the busy bit */
+		set_gdt_entry(FIRST_SPARE_SEL, value, 0x200, 0x89, 0);
+		return ltr_safe(FIRST_SPARE_SEL);
+	default:
+		assert(0);
+	}
+}
+
+static void test_host_value_natively(const char *field_name, u64 vmcs_field, u64 value)
+{
+	int vector;
+	u64 actual_value;
+
+	/*
+	 * Set the register via host native interface (e.g lgdt) and check
+	 * that we got no exception
+	 */
+	vector = set_host_value(vmcs_field, value);
+	if (vector) {
+		report(false,
+		       "Exception %d when setting %s to 0x%lx via host",
+		       vector, field_name, value);
+		return;
+	}
+
+	/*
+	 * Now check that the host value matches what we expect for fields
+	 * that can be read back (these that we can't we assume that are correct)
+	 */
+
+	if (get_host_value(vmcs_field, &actual_value))
+		actual_value = value;
+
+	report(actual_value == value,
+	       "%s: HOST value is set to test value 0x%lx directly",
+	       field_name, value);
+}
+
+static void test_host_value_via_guest(const char *field_name, u64 vmcs_field, u64 value)
+{
+	u64 actual_value;
+
+	/* Set host state field in the vmcs and do the VM entry
+	 * Success of VM entry already shows that L0 accepted the value
+	 */
+	vmcs_write(vmcs_field, TEST_VALUE2);
+	enter_guest();
+	skip_exit_vmcall();
+
+	/*
+	 * Now check that the host value matches what we expect for fields
+	 * that can be read back (these that we can't we assume that are correct)
+	 */
+
+	if (get_host_value(vmcs_field, &actual_value))
+		actual_value = value;
+
+	/* Check that now the msr value is the same as the field value */
+	report(actual_value == TEST_VALUE2,
+	       "%s: HOST value is set to test value 0x%lx via VMLAUNCH/VMRESUME",
+	       field_name, value);
+}
+
+static void do_vmx_canonical_test_one_field(const char *field_name, u64 field)
+{
+	u64 host_org_value, field_org_value;
+
+	/* Backup the current host value and vmcs field value values */
+	get_host_value(field, &host_org_value);
+	field_org_value = vmcs_read(field);
+
+	/*
+	 * Write TEST_VALUE1 57-canonical value on the host
+	 * and check that it was written correctly
+	 */
+	test_host_value_natively(field_name, field, TEST_VALUE1);
+
+	/*
+	 * Write TEST_VALUE2 57-canonical value on the host
+	 * via VM entry/exit and check that it was written correctly
+	 */
+	test_host_value_via_guest(field_name, field, TEST_VALUE2);
+
+	/* Restore original values */
+	vmcs_write(field, field_org_value);
+	set_host_value(field, host_org_value);
+}
+
+#define vmx_canonical_test_one_field(field) \
+	do_vmx_canonical_test_one_field(#field, field)
+
+static void vmx_canonical_test(void)
+{
+	report(!(read_cr4() & X86_CR4_LA57), "4 level paging");
+
+	if (!this_cpu_has(X86_FEATURE_LA57))
+		test_skip("5 level paging not supported");
+
+	test_set_guest(vmx_canonical_test_guest);
+
+	vmx_canonical_test_one_field(HOST_SYSENTER_ESP);
+	vmx_canonical_test_one_field(HOST_SYSENTER_EIP);
+	vmx_canonical_test_one_field(HOST_BASE_FS);
+	vmx_canonical_test_one_field(HOST_BASE_GS);
+	vmx_canonical_test_one_field(HOST_BASE_GDTR);
+	vmx_canonical_test_one_field(HOST_BASE_IDTR);
+	vmx_canonical_test_one_field(HOST_BASE_TR);
+
+	test_set_guest_finished();
+}
+
 enum Vid_op {
 	VID_OP_SET_ISR,
 	VID_OP_NOP,
@@ -11262,5 +11444,6 @@ struct vmx_test vmx_tests[] = {
 	TEST(vmx_pf_invvpid_test),
 	TEST(vmx_pf_vpid_test),
 	TEST(vmx_exception_test),
+	TEST(vmx_canonical_test),
 	{ NULL, NULL, NULL, NULL, NULL, {0} },
 };
-- 
2.26.3

Re: [kvm-unit-tests PATCH 0/5] Collection of tests for canonical checks on LA57 enabled CPUs

[Maxim Levitsky]

On Fri, 2024-09-06 at 20:54 -0400, Maxim Levitsky wrote:
> This is a set of tests that checks KVM and CPU behaviour in regard to
> canonical checks of various msrs, segment bases, instructions that
> were found to ignore CR4.LA57 on CPUs that support 5 level paging.
> 
> Best regards,
> 	Maxim Levitsky
> 
> Maxim Levitsky (5):
>   x86: add _safe and _fep_safe variants to segment base load
>     instructions
>   x86: add a few functions for gdt manipulation
>   x86: move struct invpcid_desc descriptor to processor.h
>   Add a test for writing canonical values to various msrs and fields
>   nVMX: add a test for canonical checks of various host state vmcs12
>     fields.
> 
>  lib/x86/desc.c      |  39 ++++-
>  lib/x86/desc.h      |   9 +-
>  lib/x86/msr.h       |  42 ++++++
>  lib/x86/processor.h |  58 +++++++-
>  x86/Makefile.x86_64 |   1 +
>  x86/canonical_57.c  | 346 ++++++++++++++++++++++++++++++++++++++++++++
>  x86/pcid.c          |   6 -
>  x86/vmx_tests.c     | 183 +++++++++++++++++++++++
>  8 files changed, 667 insertions(+), 17 deletions(-)
>  create mode 100644 x86/canonical_57.c
> 
> -- 
> 2.26.3
> 
> 

Hi,
A very kind ping on this patch series.

Best regards,
     Maxim Levitsky

Re: [kvm-unit-tests PATCH 0/5] Collection of tests for canonical checks on LA57 enabled CPUs

[Maxim Levitsky]

On Sun, 2024-11-03 at 16:08 -0500, Maxim Levitsky wrote:
> On Fri, 2024-09-06 at 20:54 -0400, Maxim Levitsky wrote:
> > This is a set of tests that checks KVM and CPU behaviour in regard to
> > canonical checks of various msrs, segment bases, instructions that
> > were found to ignore CR4.LA57 on CPUs that support 5 level paging.
> > 
> > Best regards,
> > 	Maxim Levitsky
> > 
> > Maxim Levitsky (5):
> >   x86: add _safe and _fep_safe variants to segment base load
> >     instructions
> >   x86: add a few functions for gdt manipulation
> >   x86: move struct invpcid_desc descriptor to processor.h
> >   Add a test for writing canonical values to various msrs and fields
> >   nVMX: add a test for canonical checks of various host state vmcs12
> >     fields.
> > 
> >  lib/x86/desc.c      |  39 ++++-
> >  lib/x86/desc.h      |   9 +-
> >  lib/x86/msr.h       |  42 ++++++
> >  lib/x86/processor.h |  58 +++++++-
> >  x86/Makefile.x86_64 |   1 +
> >  x86/canonical_57.c  | 346 ++++++++++++++++++++++++++++++++++++++++++++
> >  x86/pcid.c          |   6 -
> >  x86/vmx_tests.c     | 183 +++++++++++++++++++++++
> >  8 files changed, 667 insertions(+), 17 deletions(-)
> >  create mode 100644 x86/canonical_57.c
> > 
> > -- 
> > 2.26.3
> > 
> > 
> 
> Hi,
> A very kind ping on this patch series.
> 
> Best regards,
>      Maxim Levitsky
> 

Another very kind ping on this patch series.

Best regards,
	Maxim Levitsky

Re: [kvm-unit-tests PATCH 0/5] Collection of tests for canonical checks on LA57 enabled CPUs

[Maxim Levitsky]

On Thu, 2024-11-21 at 20:31 -0500, Maxim Levitsky wrote:
> On Sun, 2024-11-03 at 16:08 -0500, Maxim Levitsky wrote:
> > On Fri, 2024-09-06 at 20:54 -0400, Maxim Levitsky wrote:
> > > This is a set of tests that checks KVM and CPU behaviour in regard to
> > > canonical checks of various msrs, segment bases, instructions that
> > > were found to ignore CR4.LA57 on CPUs that support 5 level paging.
> > > 
> > > Best regards,
> > > 	Maxim Levitsky
> > > 
> > > Maxim Levitsky (5):
> > >   x86: add _safe and _fep_safe variants to segment base load
> > >     instructions
> > >   x86: add a few functions for gdt manipulation
> > >   x86: move struct invpcid_desc descriptor to processor.h
> > >   Add a test for writing canonical values to various msrs and fields
> > >   nVMX: add a test for canonical checks of various host state vmcs12
> > >     fields.
> > > 
> > >  lib/x86/desc.c      |  39 ++++-
> > >  lib/x86/desc.h      |   9 +-
> > >  lib/x86/msr.h       |  42 ++++++
> > >  lib/x86/processor.h |  58 +++++++-
> > >  x86/Makefile.x86_64 |   1 +
> > >  x86/canonical_57.c  | 346 ++++++++++++++++++++++++++++++++++++++++++++
> > >  x86/pcid.c          |   6 -
> > >  x86/vmx_tests.c     | 183 +++++++++++++++++++++++
> > >  8 files changed, 667 insertions(+), 17 deletions(-)
> > >  create mode 100644 x86/canonical_57.c
> > > 
> > > -- 
> > > 2.26.3
> > > 
> > > 
> > 
> > Hi,
> > A very kind ping on this patch series.
> > 
> > Best regards,
> >      Maxim Levitsky
> > 
> 
> Another very kind ping on this patch series.


Any update?



> 
> Best regards,
> 	Maxim Levitsky
> 

Re: [kvm-unit-tests PATCH 0/5] Collection of tests for canonical checks on LA57 enabled CPUs

[Sean Christopherson]

On Fri, Sep 06, 2024, Maxim Levitsky wrote:
> This is a set of tests that checks KVM and CPU behaviour in regard to
> canonical checks of various msrs, segment bases, instructions that
> were found to ignore CR4.LA57 on CPUs that support 5 level paging.

I have a variety of comments, but nothing that meaningfully changes the
functionality of the testcases (mostly cosmetic stuff).  I have very limited
cycles for KUT right now, so I'm going to send a v2 with my changes, but I'll
respond to each patch with my feedback/changes.  Please holler if anything is
too objectionable.

> Maxim Levitsky (5):
>   x86: add _safe and _fep_safe variants to segment base load
>     instructions
>   x86: add a few functions for gdt manipulation
>   x86: move struct invpcid_desc descriptor to processor.h
>   Add a test for writing canonical values to various msrs and fields
>   nVMX: add a test for canonical checks of various host state vmcs12
>     fields.
> 
>  lib/x86/desc.c      |  39 ++++-
>  lib/x86/desc.h      |   9 +-
>  lib/x86/msr.h       |  42 ++++++
>  lib/x86/processor.h |  58 +++++++-
>  x86/Makefile.x86_64 |   1 +
>  x86/canonical_57.c  | 346 ++++++++++++++++++++++++++++++++++++++++++++
>  x86/pcid.c          |   6 -
>  x86/vmx_tests.c     | 183 +++++++++++++++++++++++

There's no unittest.cfg change, which makes it annoyingly difficult to run the
new tests.

And there's already kinda sorta an LA57 test, la57.c, which just needs minor
tweaking to play nice with x86-64, so I think the easiest approach is to modify
la57.c to run on x86-64 and then lands these testcases there.

>  8 files changed, 667 insertions(+), 17 deletions(-)
>  create mode 100644 x86/canonical_57.c

Re: [kvm-unit-tests PATCH 4/5] Add a test for writing canonical values to various msrs and fields

[Sean Christopherson]

On Fri, Sep 06, 2024, Maxim Levitsky wrote:
> +static void test_register_write(const char *register_name, u64 test_register,
> +				bool force_emulation, u64 test_value,
> +				bool expect_success)
> +{
> +	u64 old_value, expected_value;
> +	bool success, test_passed = false;
> +	int test_mode = (force_emulation ? SET_REGISTER_MODE_FEP : SET_REGISTER_MODE_SAFE);
> +
> +	old_value = get_test_register_value(test_register);
> +	expected_value = expect_success ? test_value : old_value;
> +
> +	/*
> +	 * TODO: Successful write to the MSR_GS_BASE corrupts it,
> +	 * and that breaks the wrmsr_safe macro.
> +	 */
> +	if ((test_register >> 32) == MSR_GS_BASE && expect_success)
> +		test_mode = SET_REGISTER_MODE_UNSAFE;
> +
> +	/* Write the test value*/
> +	success =  set_test_register_value(test_register, test_mode, test_value);
> +
> +	if (success != expect_success) {
> +		report(false,

This can be report_fail().  But that's a moot point because skipping the cleanup
on unexpected *success* leaves registers in a bad state and crashes the test.

> +		       "Write of test register %s with value %lx unexpectedly %s",
> +		       register_name, test_value,
> +		       (success ? "succeeded" : "failed"));
> +		goto exit;
> +	}
> +
> +	/*
> +	 * Check that the value was really written.
> +	 * Don't test TR and LDTR, because it's not possible to read them
> +	 * directly.
> +	 */
> +
> +	if (test_register != TEST_REGISTER_TR_BASE &&
> +	    test_register != TEST_REGISTER_LDT_BASE) {
> +		u64 new_value = get_test_register_value(test_register);
> +
> +		if (new_value != expected_value) {
> +			report(false,

Same thing here (on both fronts).

> +			       "Register %s wasn't set to %lx as expected (actual value %lx)",
> +			       register_name, expected_value, new_value);
> +			goto exit;
> +		}
> +	}
> +
> +	/*
> +	 * Restore the old value directly without safety wrapper,
> +	 * to avoid test crashes related to temporary clobbered GDT/IDT/etc bases.
> +	 */
> +
> +	set_test_register_value(test_register, SET_REGISTER_MODE_UNSAFE, old_value);
> +	test_passed = true;
> +exit:
> +	report(test_passed, "Tested setting %s to 0x%lx value - %s", register_name,
> +	       test_value, success ? "success" : "failure");
> +}

...

> +#define TEST_REGISTER(register_name, force_emulation) \
> +		      test_register(#register_name, register_name, force_emulation)

Rather than print the entire name, concatenate TEST_REGISTER with the register
name, e.g. GDTR_BASE, so that the outputs are simply GDTR_BASE.

> +	/*
> +	 * SYSENTER ESP/EIP MSRs have canonical checks only on Intel,
> +	 * because only on Intel these instructions were extended to 64 bit.
> +	 *
> +	 * TODO: KVM emulation however ignores canonical checks for these MSRs
> +	 * even on Intel, to support cross-vendor migration.
> +	 *
> +	 * Thus only run the check on bare metal.

Sadly, KVM's behavior also applies for nested VMX, i.e. when running the test
nested, there is no bare metal.  AFAIK, there's no easy way to detect a nested
run from within the test; it would have to be fed in from the test runner.

For now, I'll make the whole thing a TODO so that people don't have to re-debug
why the test fails when run in a VM (and because testing bare metal isn't all
that interesting).

> +static void do_test(void)
> +{
> +	printf("\n");
> +	printf("Running the test without emulation:\n");
> +	__do_test(false);

Do the printf()s in the inner helper, it's easy to pivot on @forced_emulation.

> +	printf("\n");
> +
> +	if (is_fep_available()) {
> +		printf("Running the test with forced emulation:\n");
> +		__do_test(true);
> +	} else

Needs curly braces (moot point if the printf() is moved).

> +		report_skip("force emulation prefix not enabled - skipping");
> +}
> +
> +int main(int ac, char **av)
> +{
> +	/* set dummy LDTR pointer */
> +	set_gdt_entry(FIRST_SPARE_SEL, 0xffaabb, 0xffff, 0x82, 0);
> +	lldt(FIRST_SPARE_SEL);
> +
> +	do_test();
> +
> +	printf("\n");
> +
> +	if (this_cpu_has(X86_FEATURE_LA57)) {
> +		printf("Switching to 5 level paging mode and rerunning the test...\n");
> +		setup_5level_page_table();
> +		do_test();
> +	} else

Curly braces.

> +		report_skip("Skipping the test in 5-level paging mode - not supported on the host");
> +
> +	return report_summary();
> +}
> -- 
> 2.26.3
> 

Re: [kvm-unit-tests PATCH 5/5] nVMX: add a test for canonical checks of various host state vmcs12 fields.

[Sean Christopherson]

On Fri, Sep 06, 2024, Maxim Levitsky wrote:
> +#define TEST_VALUE1  0xff45454545000000
> +#define TEST_VALUE2  0xff55555555000000
> +
> +static void vmx_canonical_test_guest(void)
> +{
> +	while (true)
> +		vmcall();
> +}

...

> +static void test_host_value_natively(const char *field_name, u64 vmcs_field, u64 value)

The error messages say "directly", wheras this says "natively".  I don't have a
strong preference, but they should use the same terminology.  I'll go with
"direct", because "native" has paravirt connotations that could confuse things.

> +{
> +	int vector;
> +	u64 actual_value;
> +
> +	/*
> +	 * Set the register via host native interface (e.g lgdt) and check
> +	 * that we got no exception
> +	 */
> +	vector = set_host_value(vmcs_field, value);
> +	if (vector) {
> +		report(false,

report_fail()

> +		       "Exception %d when setting %s to 0x%lx via host",
> +		       vector, field_name, value);
> +		return;
> +	}
> +
> +	/*
> +	 * Now check that the host value matches what we expect for fields
> +	 * that can be read back (these that we can't we assume that are correct)
> +	 */
> +
> +	if (get_host_value(vmcs_field, &actual_value))
> +		actual_value = value;
> +
> +	report(actual_value == value,

Rather than clobber actual_value, incorporate the get() in the report, e.g.

	report(get_host_value(vmcs_field, &actual_value) || actual_value == value

> +	       "%s: HOST value is set to test value 0x%lx directly",
> +	       field_name, value);

Print the actual value as well, otherwise debugging is painful (well, more painful).

> +}
> +
> +static void test_host_value_via_guest(const char *field_name, u64 vmcs_field, u64 value)

It's not via "guest", it's via the HOST_xxx fields in the VMCS.  test_host_value_vmcs()?

> +{
> +	u64 actual_value;
> +
> +	/* Set host state field in the vmcs and do the VM entry
> +	 * Success of VM entry already shows that L0 accepted the value
> +	 */
> +	vmcs_write(vmcs_field, TEST_VALUE2);

This should be value, not TEST_VALUE2.  Ditto below.  The whole @value idea is
rather pointless though.  There's one caller, and it passes one value.  The only
requirement is that the "direct" vs. "vmcs" settings use different values, e.g.
to avoid false passes.  To handle that, just use more descriptive names for the
#defines.

> +	enter_guest();
> +	skip_exit_vmcall();
> +
> +	/*
> +	 * Now check that the host value matches what we expect for fields
> +	 * that can be read back (these that we can't we assume that are correct)
> +	 */
> +
> +	if (get_host_value(vmcs_field, &actual_value))
> +		actual_value = value;
> +
> +	/* Check that now the msr value is the same as the field value */
> +	report(actual_value == TEST_VALUE2,
> +	       "%s: HOST value is set to test value 0x%lx via VMLAUNCH/VMRESUME",
> +	       field_name, value);
> +}

Re: [kvm-unit-tests PATCH 0/5] Collection of tests for canonical checks on LA57 enabled CPUs

[Sean Christopherson]

On Fri, 06 Sep 2024 20:54:35 -0400, Maxim Levitsky wrote:
> This is a set of tests that checks KVM and CPU behaviour in regard to
> canonical checks of various msrs, segment bases, instructions that
> were found to ignore CR4.LA57 on CPUs that support 5 level paging.
> 
> Best regards,
> 	Maxim Levitsky
> 
> [...]

Applied to kvm-x86 next (and now pulled by Paolo).  Thanks!

[1/5] x86: add _safe and _fep_safe variants to segment base load instructions
      https://github.com/kvm-x86/kvm-unit-tests/commit/5047281ab3e1
[2/5] x86: add a few functions for gdt manipulation
      https://github.com/kvm-x86/kvm-unit-tests/commit/b1f3eec1b59b
[3/5] x86: move struct invpcid_desc descriptor to processor.h
      https://github.com/kvm-x86/kvm-unit-tests/commit/b88e90e64526
[4/5] Add a test for writing canonical values to various msrs and fields
      https://github.com/kvm-x86/kvm-unit-tests/commit/f6257e242a52
[5/5] nVMX: add a test for canonical checks of various host state vmcs12 fields.
      https://github.com/kvm-x86/kvm-unit-tests/commit/05fbb364b5b2

--
https://github.com/kvm-x86/kvm-unit-tests/tree/next