[kvm-unit-tests PATCH v4 0/9] x86/access: CR0.WP=0/1 r/o tests

[kvm-unit-tests PATCH v4 0/9] x86/access: CR0.WP=0/1 r/o tests

[Sean Christopherson]

Mathias' series to add explicit CR0.WP toggling tests, along with related
cleanups from me to allow enabling forced emulation for the "core" of the
access test.

The potentially controversial change is to make the extra-slow variants,
including forced emulation, nodefault, i.e. so called "manual" testcases.
Using nodefault is the most elegant solution I could find, e.g. makes it
relatively simple to run all tests on bare metal without triggering a pile
of timeouts when running in a VM.

v4:
 - Make FEP manual-only, but included in the core loop.
 - Mark the extra-slow (4+ minutes each in a VM) VPID-based variants
   manual-only
 - Use the standard reporting machinery so that SKIPs show up
   correctly.

v3:
 - https://lore.kernel.org/all/20230403105618.41118-1-minipli@grsecurity.net 
 - Rewrite changelogs.
 - Use helper for guts of CR0.WP test.

v2: https://lore.kernel.org/kvm/20230331135709.132713-1-minipli@grsecurity.net

Mathias Krause (4):
  x86: Use existing CR0.WP / CR4.SMEP bit definitions
  x86/access: CR0.WP toggling write to r/o data test
  x86/access: Add forced emulation support
  x86/access: Try forced emulation for CR0.WP test as well

Sean Christopherson (5):
  x86/access: Replace spaces with tabs in access_test.c
  x86/access: Use standard pass/fail reporting machinery
  x86: Drop VPID invl tests from nested reduced MAXPHYADDR access
    testcase
  x86: Mark the VPID invalidation nested VMX access tests nodefault
  nVMX: Add forced emulation variant of #PF access test

 x86/access.c      | 114 +++++++++++++++++++++++++++++++++++++++-------
 x86/access.h      |   2 +-
 x86/access_test.c |  28 ++++++------
 x86/pks.c         |   5 +-
 x86/pku.c         |   5 +-
 x86/unittests.cfg |  20 ++++++--
 x86/vmx_tests.c   |  27 ++++++++---
 7 files changed, 154 insertions(+), 47 deletions(-)


base-commit: 5b5d27da2973b20ec29b18df4d749fb2190458af
-- 
2.40.0.348.gf938b09366-goog

[kvm-unit-tests PATCH v4 1/9] x86: Use existing CR0.WP / CR4.SMEP bit definitions

[Sean Christopherson]

From: Mathias Krause <minipli@grsecurity.net>

Use the existing bit definitions from x86/processor.h instead of
defining one-off versions in individual tests.

Signed-off-by: Mathias Krause <minipli@grsecurity.net>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 x86/access.c | 11 ++++-------
 x86/pks.c    |  5 ++---
 x86/pku.c    |  5 ++---
 3 files changed, 8 insertions(+), 13 deletions(-)

diff --git a/x86/access.c b/x86/access.c
index 4dfec230..203353a3 100644
--- a/x86/access.c
+++ b/x86/access.c
@@ -20,9 +20,6 @@ static int invalid_mask;
 #define PT_BASE_ADDR_MASK ((pt_element_t)((((pt_element_t)1 << 36) - 1) & PAGE_MASK))
 #define PT_PSE_BASE_ADDR_MASK (PT_BASE_ADDR_MASK & ~(1ull << 21))
 
-#define CR0_WP_MASK (1UL << 16)
-#define CR4_SMEP_MASK (1UL << 20)
-
 #define PFERR_PRESENT_MASK (1U << 0)
 #define PFERR_WRITE_MASK (1U << 1)
 #define PFERR_USER_MASK (1U << 2)
@@ -239,9 +236,9 @@ static void set_cr0_wp(int wp)
 {
 	unsigned long cr0 = shadow_cr0;
 
-	cr0 &= ~CR0_WP_MASK;
+	cr0 &= ~X86_CR0_WP;
 	if (wp)
-		cr0 |= CR0_WP_MASK;
+		cr0 |= X86_CR0_WP;
 	if (cr0 != shadow_cr0) {
 		write_cr0(cr0);
 		shadow_cr0 = cr0;
@@ -272,9 +269,9 @@ static unsigned set_cr4_smep(ac_test_t *at, int smep)
 	unsigned long cr4 = shadow_cr4;
 	unsigned r;
 
-	cr4 &= ~CR4_SMEP_MASK;
+	cr4 &= ~X86_CR4_SMEP;
 	if (smep)
-		cr4 |= CR4_SMEP_MASK;
+		cr4 |= X86_CR4_SMEP;
 	if (cr4 == shadow_cr4)
 		return 0;
 
diff --git a/x86/pks.c b/x86/pks.c
index ef95fb96..bda15efc 100644
--- a/x86/pks.c
+++ b/x86/pks.c
@@ -5,7 +5,6 @@
 #include "x86/vm.h"
 #include "x86/msr.h"
 
-#define CR0_WP_MASK      (1UL << 16)
 #define PTE_PKEY_BIT     59
 #define SUPER_BASE        (1 << 23)
 #define SUPER_VAR(v)      (*((__typeof__(&(v))) (((unsigned long)&v) + SUPER_BASE)))
@@ -18,9 +17,9 @@ static void set_cr0_wp(int wp)
 {
     unsigned long cr0 = read_cr0();
 
-    cr0 &= ~CR0_WP_MASK;
+    cr0 &= ~X86_CR0_WP;
     if (wp)
-        cr0 |= CR0_WP_MASK;
+        cr0 |= X86_CR0_WP;
     write_cr0(cr0);
 }
 
diff --git a/x86/pku.c b/x86/pku.c
index 51ff412c..6c0d72cc 100644
--- a/x86/pku.c
+++ b/x86/pku.c
@@ -5,7 +5,6 @@
 #include "x86/vm.h"
 #include "x86/msr.h"
 
-#define CR0_WP_MASK      (1UL << 16)
 #define PTE_PKEY_BIT     59
 #define USER_BASE        (1 << 23)
 #define USER_VAR(v)      (*((__typeof__(&(v))) (((unsigned long)&v) + USER_BASE)))
@@ -18,9 +17,9 @@ static void set_cr0_wp(int wp)
 {
     unsigned long cr0 = read_cr0();
 
-    cr0 &= ~CR0_WP_MASK;
+    cr0 &= ~X86_CR0_WP;
     if (wp)
-        cr0 |= CR0_WP_MASK;
+        cr0 |= X86_CR0_WP;
     write_cr0(cr0);
 }
 
-- 
2.40.0.348.gf938b09366-goog

[kvm-unit-tests PATCH v4 2/9] x86/access: CR0.WP toggling write to r/o data test

[Sean Christopherson]

From: Mathias Krause <minipli@grsecurity.net>

KUT has tests that verify a supervisor write access to an r/o page is
successful when CR0.WP=0, but lacks a test that explicitly verifies that
the same access faults after setting CR0.WP=1 without flushing any
associated TLB entries, either explicitly (INVLPG) or implicitly (write
to CR3). Add such a test.

Signed-off-by: Mathias Krause <minipli@grsecurity.net>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 x86/access.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 58 insertions(+), 2 deletions(-)

diff --git a/x86/access.c b/x86/access.c
index 203353a3..2d3d7c7b 100644
--- a/x86/access.c
+++ b/x86/access.c
@@ -575,9 +575,10 @@ fault:
 		at->expected_error &= ~PFERR_FETCH_MASK;
 }
 
-static void ac_set_expected_status(ac_test_t *at)
+static void __ac_set_expected_status(ac_test_t *at, bool flush)
 {
-	invlpg(at->virt);
+	if (flush)
+		invlpg(at->virt);
 
 	if (at->ptep)
 		at->expected_pte = *at->ptep;
@@ -599,6 +600,11 @@ static void ac_set_expected_status(ac_test_t *at)
 	ac_emulate_access(at, at->flags);
 }
 
+static void ac_set_expected_status(ac_test_t *at)
+{
+	__ac_set_expected_status(at, true);
+}
+
 static pt_element_t ac_get_pt(ac_test_t *at, int i, pt_element_t *ptep)
 {
 	pt_element_t pte;
@@ -1061,6 +1067,55 @@ err:
 	return 0;
 }
 
+#define TOGGLE_CR0_WP_TEST_BASE_FLAGS \
+	(AC_PDE_PRESENT_MASK | AC_PDE_ACCESSED_MASK | \
+	 AC_PTE_PRESENT_MASK | AC_PTE_ACCESSED_MASK | \
+	 AC_ACCESS_WRITE_MASK)
+
+static int do_cr0_wp_access(ac_test_t *at, int flags)
+{
+	const bool cr0_wp = !!(flags & AC_CPU_CR0_WP_MASK);
+
+	at->flags = TOGGLE_CR0_WP_TEST_BASE_FLAGS | flags;
+	__ac_set_expected_status(at, false);
+
+	/*
+	 * Under VMX the guest might own the CR0.WP bit, requiring KVM to
+	 * manually keep track of it where needed, e.g. in the guest page
+	 * table walker.
+	 *
+	 * Load CR0.WP with the inverse value of what will be used during
+	 * the access test and toggle EFER.NX to coerce KVM into rebuilding
+	 * the current MMU context based on the soon-to-be-stale CR0.WP.
+	 */
+	set_cr0_wp(!cr0_wp);
+	set_efer_nx(1);
+	set_efer_nx(0);
+
+	if (!ac_test_do_access(at)) {
+		printf("%s: supervisor write with CR0.WP=%d did not %s\n",
+		       __FUNCTION__, cr0_wp, cr0_wp ? "FAULT" : "SUCCEED");
+		return 1;
+	}
+
+	return 0;
+}
+
+static int check_toggle_cr0_wp(ac_pt_env_t *pt_env)
+{
+	ac_test_t at;
+	int err = 0;
+
+	ac_test_init(&at, 0xffff923042007000ul, pt_env);
+	at.flags = TOGGLE_CR0_WP_TEST_BASE_FLAGS;
+	ac_test_setup_ptes(&at);
+
+	err += do_cr0_wp_access(&at, 0);
+	err += do_cr0_wp_access(&at, AC_CPU_CR0_WP_MASK);
+
+	return err == 0;
+}
+
 static int check_effective_sp_permissions(ac_pt_env_t *pt_env)
 {
 	unsigned long ptr1 = 0xffff923480000000;
@@ -1150,6 +1205,7 @@ const ac_test_fn ac_test_cases[] =
 	check_pfec_on_prefetch_pte,
 	check_large_pte_dirty_for_nowp,
 	check_smep_andnot_wp,
+	check_toggle_cr0_wp,
 	check_effective_sp_permissions,
 };
 
-- 
2.40.0.348.gf938b09366-goog

[kvm-unit-tests PATCH v4 3/9] x86/access: Replace spaces with tabs in access_test.c

[Sean Christopherson]

Replace spaces with tabs in the access test wrapper to adhere to the
preferred style.

No functional change intended.

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

diff --git a/x86/access_test.c b/x86/access_test.c
index 67e9a080..74360698 100644
--- a/x86/access_test.c
+++ b/x86/access_test.c
@@ -5,22 +5,22 @@
 
 int main(void)
 {
-    int r;
+	int r;
 
-    printf("starting test\n\n");
-    r = ac_test_run(PT_LEVEL_PML4);
+	printf("starting test\n\n");
+	r = ac_test_run(PT_LEVEL_PML4);
 
 #ifndef CONFIG_EFI
-    /*
-     * Not supported yet for UEFI, because setting up 5
-     * level page table requires entering real mode.
-     */
-    if (this_cpu_has(X86_FEATURE_LA57)) {
-        printf("starting 5-level paging test.\n\n");
-        setup_5level_page_table();
-        r = ac_test_run(PT_LEVEL_PML5);
-    }
+	/*
+	* Not supported yet for UEFI, because setting up 5
+	* level page table requires entering real mode.
+	*/
+	if (this_cpu_has(X86_FEATURE_LA57)) {
+		printf("starting 5-level paging test.\n\n");
+		setup_5level_page_table();
+		r = ac_test_run(PT_LEVEL_PML5);
+	}
 #endif
 
-    return r ? 0 : 1;
+	return r ? 0 : 1;
 }
-- 
2.40.0.348.gf938b09366-goog

[kvm-unit-tests PATCH v4 4/9] x86/access: Use standard pass/fail reporting machinery

[Sean Christopherson]

Use the standard reporting machinery in the access test so that future
changes to skip variants of the test actually get reported as SKIPs.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 x86/access.c      | 4 ++--
 x86/access.h      | 2 +-
 x86/access_test.c | 8 +++-----
 3 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/x86/access.c b/x86/access.c
index 2d3d7c7b..1677d52e 100644
--- a/x86/access.c
+++ b/x86/access.c
@@ -1209,7 +1209,7 @@ const ac_test_fn ac_test_cases[] =
 	check_effective_sp_permissions,
 };
 
-int ac_test_run(int pt_levels)
+void ac_test_run(int pt_levels)
 {
 	ac_test_t at;
 	ac_pt_env_t pt_env;
@@ -1292,5 +1292,5 @@ int ac_test_run(int pt_levels)
 
 	printf("\n%d tests, %d failures\n", tests, tests - successes);
 
-	return successes == tests;
+	report(successes == tests, "%d-level paging tests", pt_levels);
 }
diff --git a/x86/access.h b/x86/access.h
index bcfa7b26..9a6c5628 100644
--- a/x86/access.h
+++ b/x86/access.h
@@ -4,6 +4,6 @@
 #define PT_LEVEL_PML4 4
 #define PT_LEVEL_PML5 5
 
-int ac_test_run(int page_table_levels);
+void ac_test_run(int page_table_levels);
 
 #endif // X86_ACCESS_H
\ No newline at end of file
diff --git a/x86/access_test.c b/x86/access_test.c
index 74360698..2ac649d2 100644
--- a/x86/access_test.c
+++ b/x86/access_test.c
@@ -5,10 +5,8 @@
 
 int main(void)
 {
-	int r;
-
 	printf("starting test\n\n");
-	r = ac_test_run(PT_LEVEL_PML4);
+	ac_test_run(PT_LEVEL_PML4);
 
 #ifndef CONFIG_EFI
 	/*
@@ -18,9 +16,9 @@ int main(void)
 	if (this_cpu_has(X86_FEATURE_LA57)) {
 		printf("starting 5-level paging test.\n\n");
 		setup_5level_page_table();
-		r = ac_test_run(PT_LEVEL_PML5);
+		ac_test_run(PT_LEVEL_PML5);
 	}
 #endif
 
-	return r ? 0 : 1;
+	return report_summary();
 }
-- 
2.40.0.348.gf938b09366-goog

[kvm-unit-tests PATCH v4 5/9] x86/access: Add forced emulation support

[Sean Christopherson]

From: Mathias Krause <minipli@grsecurity.net>

Add support to force access tests to be handled by the emulator, if
supported by KVM.  Due to emulation being rather slow, make the forced
emulation nodefault, i.e. a manual-only testcase.  Note, "manual" just
means the test runner needs to specify a specific group(s), i.e. the
test is still easy to enable in CI for compatible setups.

Manually check for FEP support in the test instead of using the "check"
option to query the module param, as any non-zero force_emulation_prefix
value enables FEP (see KVM commit d500e1ed3dc8 ("KVM: x86: Allow clearing
RFLAGS.RF on forced emulation to test code #DBs") and scripts/runtime.bash
only supports checking for a single value.

Defer enabling FEP for the nested VMX variants to a future patch.  KVM has
a bug related to emulating NOP (yes, NOP) for L2, and the nVMX varaints
will require additional massaging to propagate the "allow emulation" flag.

Signed-off-by: Mathias Krause <minipli@grsecurity.net>
[sean: make generic fep testcase nodefault instead of impossible]
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 x86/access.c      | 38 +++++++++++++++++++++++++++++++-------
 x86/access.h      |  2 +-
 x86/access_test.c |  8 +++++---
 x86/unittests.cfg |  6 ++++++
 x86/vmx_tests.c   |  2 +-
 5 files changed, 44 insertions(+), 12 deletions(-)

diff --git a/x86/access.c b/x86/access.c
index 1677d52e..4a3ca265 100644
--- a/x86/access.c
+++ b/x86/access.c
@@ -82,7 +82,9 @@ enum {
 	AC_CPU_CR4_SMEP_BIT,
 	AC_CPU_CR4_PKE_BIT,
 
-	NR_AC_FLAGS
+	AC_FEP_BIT,
+
+	NR_AC_FLAGS,
 };
 
 #define AC_PTE_PRESENT_MASK   (1 << AC_PTE_PRESENT_BIT)
@@ -121,6 +123,8 @@ enum {
 #define AC_CPU_CR4_SMEP_MASK  (1 << AC_CPU_CR4_SMEP_BIT)
 #define AC_CPU_CR4_PKE_MASK   (1 << AC_CPU_CR4_PKE_BIT)
 
+#define AC_FEP_MASK           (1 << AC_FEP_BIT)
+
 const char *ac_names[] = {
 	[AC_PTE_PRESENT_BIT] = "pte.p",
 	[AC_PTE_ACCESSED_BIT] = "pte.a",
@@ -152,6 +156,7 @@ const char *ac_names[] = {
 	[AC_CPU_CR0_WP_BIT] = "cr0.wp",
 	[AC_CPU_CR4_SMEP_BIT] = "cr4.smep",
 	[AC_CPU_CR4_PKE_BIT] = "cr4.pke",
+	[AC_FEP_BIT] = "fep",
 };
 
 static inline void *va(pt_element_t phys)
@@ -799,10 +804,13 @@ static int ac_test_do_access(ac_test_t *at)
 
 	if (F(AC_ACCESS_TWICE)) {
 		asm volatile ("mov $fixed2, %%rsi \n\t"
-			      "mov (%[addr]), %[reg] \n\t"
+			      "cmp $0, %[fep] \n\t"
+			      "jz 1f \n\t"
+			      KVM_FEP
+			      "1: mov (%[addr]), %[reg] \n\t"
 			      "fixed2:"
 			      : [reg]"=r"(r), [fault]"=a"(fault), "=b"(e)
-			      : [addr]"r"(at->virt)
+			      : [addr]"r"(at->virt), [fep]"r"(F(AC_FEP))
 			      : "rsi");
 		fault = 0;
 	}
@@ -823,9 +831,15 @@ static int ac_test_do_access(ac_test_t *at)
 		      "jnz 2f \n\t"
 		      "cmp $0, %[write] \n\t"
 		      "jnz 1f \n\t"
-		      "mov (%[addr]), %[reg] \n\t"
+		      "cmp $0, %[fep] \n\t"
+		      "jz 0f \n\t"
+		      KVM_FEP
+		      "0: mov (%[addr]), %[reg] \n\t"
 		      "jmp done \n\t"
-		      "1: mov %[reg], (%[addr]) \n\t"
+		      "1: cmp $0, %[fep] \n\t"
+		      "jz 0f \n\t"
+		      KVM_FEP
+		      "0: mov %[reg], (%[addr]) \n\t"
 		      "jmp done \n\t"
 		      "2: call *%[addr] \n\t"
 		      "done: \n"
@@ -843,6 +857,7 @@ static int ac_test_do_access(ac_test_t *at)
 			[write]"r"(F(AC_ACCESS_WRITE)),
 			[user]"r"(F(AC_ACCESS_USER)),
 			[fetch]"r"(F(AC_ACCESS_FETCH)),
+			[fep]"r"(F(AC_FEP)),
 			[user_ds]"i"(USER_DS),
 			[user_cs]"i"(USER_CS),
 			[user_stack_top]"r"(user_stack + sizeof user_stack),
@@ -1209,12 +1224,17 @@ const ac_test_fn ac_test_cases[] =
 	check_effective_sp_permissions,
 };
 
-void ac_test_run(int pt_levels)
+void ac_test_run(int pt_levels, bool force_emulation)
 {
 	ac_test_t at;
 	ac_pt_env_t pt_env;
 	int i, tests, successes;
 
+	if (force_emulation && !is_fep_available()) {
+		report_skip("Forced emulation prefix (FEP) not available\n");
+		return;
+	}
+
 	printf("run\n");
 	tests = successes = 0;
 
@@ -1232,6 +1252,9 @@ void ac_test_run(int pt_levels)
 		invalid_mask |= AC_PTE_BIT36_MASK;
 	}
 
+	if (!force_emulation)
+		invalid_mask |= AC_FEP_MASK;
+
 	ac_env_int(&pt_env, pt_levels);
 	ac_test_init(&at, 0xffff923400000000ul, &pt_env);
 
@@ -1292,5 +1315,6 @@ void ac_test_run(int pt_levels)
 
 	printf("\n%d tests, %d failures\n", tests, tests - successes);
 
-	report(successes == tests, "%d-level paging tests", pt_levels);
+	report(successes == tests, "%d-level paging tests%s", pt_levels,
+	       force_emulation ? " (with forced emulation)" : "");
 }
diff --git a/x86/access.h b/x86/access.h
index 9a6c5628..206a1c86 100644
--- a/x86/access.h
+++ b/x86/access.h
@@ -4,6 +4,6 @@
 #define PT_LEVEL_PML4 4
 #define PT_LEVEL_PML5 5
 
-void ac_test_run(int page_table_levels);
+void ac_test_run(int page_table_levels, bool force_emulation);
 
 #endif // X86_ACCESS_H
\ No newline at end of file
diff --git a/x86/access_test.c b/x86/access_test.c
index 2ac649d2..025294da 100644
--- a/x86/access_test.c
+++ b/x86/access_test.c
@@ -3,10 +3,12 @@
 #include "x86/vm.h"
 #include "access.h"
 
-int main(void)
+int main(int argc, const char *argv[])
 {
+	bool force_emulation = argc >= 2 && !strcmp(argv[1], "force_emulation");
+
 	printf("starting test\n\n");
-	ac_test_run(PT_LEVEL_PML4);
+	ac_test_run(PT_LEVEL_PML4, force_emulation);
 
 #ifndef CONFIG_EFI
 	/*
@@ -16,7 +18,7 @@ int main(void)
 	if (this_cpu_has(X86_FEATURE_LA57)) {
 		printf("starting 5-level paging test.\n\n");
 		setup_5level_page_table();
-		ac_test_run(PT_LEVEL_PML5);
+		ac_test_run(PT_LEVEL_PML5, force_emulation);
 	}
 #endif
 
diff --git a/x86/unittests.cfg b/x86/unittests.cfg
index f324e32d..6194e0ea 100644
--- a/x86/unittests.cfg
+++ b/x86/unittests.cfg
@@ -143,6 +143,12 @@ file = access_test.flat
 arch = x86_64
 extra_params = -cpu max
 
+[access_fep]
+file = access_test.flat
+arch = x86_64
+extra_params = -cpu max -append force_emulation
+groups = nodefault
+
 [access-reduced-maxphyaddr]
 file = access_test.flat
 arch = x86_64
diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
index 7bba8165..617b97b3 100644
--- a/x86/vmx_tests.c
+++ b/x86/vmx_tests.c
@@ -10460,7 +10460,7 @@ static void atomic_switch_overflow_msrs_test(void)
 
 static void vmx_pf_exception_test_guest(void)
 {
-	ac_test_run(PT_LEVEL_PML4);
+	ac_test_run(PT_LEVEL_PML4, false);
 }
 
 typedef void (*invalidate_tlb_t)(void *data);
-- 
2.40.0.348.gf938b09366-goog

[kvm-unit-tests PATCH v4 6/9] x86/access: Try forced emulation for CR0.WP test as well

[Sean Christopherson]

From: Mathias Krause <minipli@grsecurity.net>

Enhance the CR0.WP toggling test to do additional tests via the emulator
as forcing KVM to emulate page protections exercises different flows than
shoving the correct bits into hardware, e.g. KVM has had at least one bug
when CR0.WP is guest owned.

Link: https://lore.kernel.org/kvm/ea3a8fbc-2bf8-7442-e498-3e5818384c83@grsecurity.net
Signed-off-by: Mathias Krause <minipli@grsecurity.net>
[sean: check AC_FEP_MASK instead of fep_available()]
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 x86/access.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/x86/access.c b/x86/access.c
index 4a3ca265..70d81bf0 100644
--- a/x86/access.c
+++ b/x86/access.c
@@ -1108,8 +1108,9 @@ static int do_cr0_wp_access(ac_test_t *at, int flags)
 	set_efer_nx(0);
 
 	if (!ac_test_do_access(at)) {
-		printf("%s: supervisor write with CR0.WP=%d did not %s\n",
-		       __FUNCTION__, cr0_wp, cr0_wp ? "FAULT" : "SUCCEED");
+		printf("%s: %ssupervisor write with CR0.WP=%d did not %s\n",
+		       __FUNCTION__, (flags & AC_FEP_MASK) ? "emulated " : "",
+		       cr0_wp, cr0_wp ? "FAULT" : "SUCCEED");
 		return 1;
 	}
 
@@ -1127,6 +1128,10 @@ static int check_toggle_cr0_wp(ac_pt_env_t *pt_env)
 
 	err += do_cr0_wp_access(&at, 0);
 	err += do_cr0_wp_access(&at, AC_CPU_CR0_WP_MASK);
+	if (!(invalid_mask & AC_FEP_MASK)) {
+		err += do_cr0_wp_access(&at, AC_FEP_MASK);
+		err += do_cr0_wp_access(&at, AC_FEP_MASK | AC_CPU_CR0_WP_MASK);
+	}
 
 	return err == 0;
 }
-- 
2.40.0.348.gf938b09366-goog

[kvm-unit-tests PATCH v4 7/9] x86: Drop VPID invl tests from nested reduced MAXPHYADDR access testcase

[Sean Christopherson]

Remove the variants of the nested VMX reduced MAXPHYADDR access test that
require a nested VM-Exit in order to invalidate the to-be-tested
translation, as the combination is quite uninteresting, and those tests
have a painfully long runtime (upwards of 10+ minutes).  Alternatively,
the tests could be split out a la commit ca785dae ("vmx: separate VPID
tests"), but the probability of finding a KVM bug that only affects
VPID-based invladations in combination with a smaller MAXPHYADDR is
extremely low.

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

diff --git a/x86/unittests.cfg b/x86/unittests.cfg
index 6194e0ea..e325667b 100644
--- a/x86/unittests.cfg
+++ b/x86/unittests.cfg
@@ -432,7 +432,7 @@ timeout = 240
 
 [vmx_pf_exception_test_reduced_maxphyaddr]
 file = vmx.flat
-extra_params = -cpu IvyBridge,phys-bits=36,host-phys-bits=off,+vmx -append "vmx_pf_exception_test vmx_pf_no_vpid_test vmx_pf_vpid_test vmx_pf_invvpid_test"
+extra_params = -cpu IvyBridge,phys-bits=36,host-phys-bits=off,+vmx -append "vmx_pf_exception_test"
 arch = x86_64
 groups = vmx nested_exception
 check = /sys/module/kvm_intel/parameters/allow_smaller_maxphyaddr=Y
-- 
2.40.0.348.gf938b09366-goog

[kvm-unit-tests PATCH v4 8/9] x86: Mark the VPID invalidation nested VMX access tests nodefault

[Sean Christopherson]

Make the painfully slow VPID-focused nested VMX access tests manual-only,
as they add marginal value (have never found a bug that wasn't found by
the base access test or non-VPID vmx_pf_exception_test), and are all but
guarantee to timeout when run in a VM despite their 240s limit.

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

diff --git a/x86/unittests.cfg b/x86/unittests.cfg
index e325667b..c878363e 100644
--- a/x86/unittests.cfg
+++ b/x86/unittests.cfg
@@ -413,21 +413,21 @@ groups = vmx nested_exception
 file = vmx.flat
 extra_params = -cpu max,+vmx -append "vmx_pf_vpid_test"
 arch = x86_64
-groups = vmx nested_exception
+groups = vmx nested_exception nodefault
 timeout = 240
 
 [vmx_pf_invvpid_test]
 file = vmx.flat
 extra_params = -cpu max,+vmx -append "vmx_pf_invvpid_test"
 arch = x86_64
-groups = vmx nested_exception
+groups = vmx nested_exception nodefault
 timeout = 240
 
 [vmx_pf_no_vpid_test]
 file = vmx.flat
 extra_params = -cpu max,+vmx -append "vmx_pf_no_vpid_test"
 arch = x86_64
-groups = vmx nested_exception
+groups = vmx nested_exception nodefault
 timeout = 240
 
 [vmx_pf_exception_test_reduced_maxphyaddr]
-- 
2.40.0.348.gf938b09366-goog

[kvm-unit-tests PATCH v4 9/9] nVMX: Add forced emulation variant of #PF access test

[Sean Christopherson]

Add a forced emulation variant of vmx_pf_exception_test to exercise KVM
emulation of L2 instructions and accesses.  Like the non-nested version,
make the test nodefault as forcing KVM to emulate drastically increases
the runtime.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 x86/unittests.cfg |  6 ++++++
 x86/vmx_tests.c   | 25 ++++++++++++++++++++-----
 2 files changed, 26 insertions(+), 5 deletions(-)

diff --git a/x86/unittests.cfg b/x86/unittests.cfg
index c878363e..0971bb3f 100644
--- a/x86/unittests.cfg
+++ b/x86/unittests.cfg
@@ -409,6 +409,12 @@ extra_params = -cpu max,+vmx -append "vmx_pf_exception_test"
 arch = x86_64
 groups = vmx nested_exception
 
+[vmx_pf_exception_test_fep]
+file = vmx.flat
+extra_params = -cpu max,+vmx -append "vmx_pf_exception_forced_emulation_test"
+arch = x86_64
+groups = vmx nested_exception nodefault
+
 [vmx_pf_vpid_test]
 file = vmx.flat
 extra_params = -cpu max,+vmx -append "vmx_pf_vpid_test"
diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
index 617b97b3..5a0415f9 100644
--- a/x86/vmx_tests.c
+++ b/x86/vmx_tests.c
@@ -10463,14 +10463,22 @@ static void vmx_pf_exception_test_guest(void)
 	ac_test_run(PT_LEVEL_PML4, false);
 }
 
+static void vmx_pf_exception_forced_emulation_test_guest(void)
+{
+	ac_test_run(PT_LEVEL_PML4, true);
+}
+
 typedef void (*invalidate_tlb_t)(void *data);
+typedef void (*pf_exception_test_guest_t)(void);
 
-static void __vmx_pf_exception_test(invalidate_tlb_t inv_fn, void *data)
+
+static void __vmx_pf_exception_test(invalidate_tlb_t inv_fn, void *data,
+				    pf_exception_test_guest_t guest_fn)
 {
 	u64 efer;
 	struct cpuid cpuid;
 
-	test_set_guest(vmx_pf_exception_test_guest);
+	test_set_guest(guest_fn);
 
 	/* Intercept INVLPG when to perform TLB invalidation from L1 (this). */
 	if (inv_fn)
@@ -10519,7 +10527,12 @@ static void __vmx_pf_exception_test(invalidate_tlb_t inv_fn, void *data)
 
 static void vmx_pf_exception_test(void)
 {
-	__vmx_pf_exception_test(NULL, NULL);
+	__vmx_pf_exception_test(NULL, NULL, vmx_pf_exception_test_guest);
+}
+
+static void vmx_pf_exception_forced_emulation_test(void)
+{
+	__vmx_pf_exception_test(NULL, NULL, vmx_pf_exception_forced_emulation_test_guest);
 }
 
 static void invalidate_tlb_no_vpid(void *data)
@@ -10532,7 +10545,8 @@ static void vmx_pf_no_vpid_test(void)
 	if (is_vpid_supported())
 		vmcs_clear_bits(CPU_EXEC_CTRL1, CPU_VPID);
 
-	__vmx_pf_exception_test(invalidate_tlb_no_vpid, NULL);
+	__vmx_pf_exception_test(invalidate_tlb_no_vpid, NULL,
+				vmx_pf_exception_test_guest);
 }
 
 static void invalidate_tlb_invvpid_addr(void *data)
@@ -10569,7 +10583,7 @@ static void __vmx_pf_vpid_test(invalidate_tlb_t inv_fn, u16 vpid)
 	vmcs_set_bits(CPU_EXEC_CTRL1, CPU_VPID);
 	vmcs_write(VPID, vpid);
 
-	__vmx_pf_exception_test(inv_fn, &vpid);
+	__vmx_pf_exception_test(inv_fn, &vpid, vmx_pf_exception_test_guest);
 }
 
 static void vmx_pf_invvpid_test(void)
@@ -10792,6 +10806,7 @@ struct vmx_test vmx_tests[] = {
 	TEST(vmx_mtf_test),
 	TEST(vmx_mtf_pdpte_test),
 	TEST(vmx_pf_exception_test),
+	TEST(vmx_pf_exception_forced_emulation_test),
 	TEST(vmx_pf_no_vpid_test),
 	TEST(vmx_pf_invvpid_test),
 	TEST(vmx_pf_vpid_test),
-- 
2.40.0.348.gf938b09366-goog

Re: [kvm-unit-tests PATCH v4 5/9] x86/access: Add forced emulation support

[Sean Christopherson]

On Tue, Apr 04, 2023, Sean Christopherson wrote:
> diff --git a/x86/unittests.cfg b/x86/unittests.cfg
> index f324e32d..6194e0ea 100644
> --- a/x86/unittests.cfg
> +++ b/x86/unittests.cfg
> @@ -143,6 +143,12 @@ file = access_test.flat
>  arch = x86_64
>  extra_params = -cpu max
>  
> +[access_fep]
> +file = access_test.flat
> +arch = x86_64
> +extra_params = -cpu max -append force_emulation
> +groups = nodefault

Argh, this needs to override the timeout.  Ditto for the vmx_pf_exception_test_fep
variant.  If there are no other issues or objections in the series, I'll set the
timeouts to 240 when applying.

Re: [kvm-unit-tests PATCH v4 9/9] nVMX: Add forced emulation variant of #PF access test

[Sean Christopherson]

On Tue, Apr 04, 2023, Sean Christopherson wrote:
> Add a forced emulation variant of vmx_pf_exception_test to exercise KVM
> emulation of L2 instructions and accesses.  Like the non-nested version,
> make the test nodefault as forcing KVM to emulate drastically increases
> the runtime.
> 
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  x86/unittests.cfg |  6 ++++++
>  x86/vmx_tests.c   | 25 ++++++++++++++++++++-----
>  2 files changed, 26 insertions(+), 5 deletions(-)
> 
> diff --git a/x86/unittests.cfg b/x86/unittests.cfg
> index c878363e..0971bb3f 100644
> --- a/x86/unittests.cfg
> +++ b/x86/unittests.cfg
> @@ -409,6 +409,12 @@ extra_params = -cpu max,+vmx -append "vmx_pf_exception_test"
>  arch = x86_64
>  groups = vmx nested_exception
>  
> +[vmx_pf_exception_test_fep]
> +file = vmx.flat
> +extra_params = -cpu max,+vmx -append "vmx_pf_exception_forced_emulation_test"
> +arch = x86_64
> +groups = vmx nested_exception nodefault

And I forgot how the nVMX "test" works.  vmx_pf_exception_forced_emulation_test
needs to be explicitly filtered out of the primary "vmx" test config.

Re: [kvm-unit-tests PATCH v4 5/9] x86/access: Add forced emulation support

[Sean Christopherson]

On Tue, Apr 04, 2023, Sean Christopherson wrote:
> On Tue, Apr 04, 2023, Sean Christopherson wrote:
> > diff --git a/x86/unittests.cfg b/x86/unittests.cfg
> > index f324e32d..6194e0ea 100644
> > --- a/x86/unittests.cfg
> > +++ b/x86/unittests.cfg
> > @@ -143,6 +143,12 @@ file = access_test.flat
> >  arch = x86_64
> >  extra_params = -cpu max
> >  
> > +[access_fep]
> > +file = access_test.flat
> > +arch = x86_64
> > +extra_params = -cpu max -append force_emulation
> > +groups = nodefault
> 
> Argh, this needs to override the timeout.  Ditto for the vmx_pf_exception_test_fep
> variant.  If there are no other issues or objections in the series, I'll set the
> timeouts to 240 when applying.

FYI Mathias, Paolo grabbed these already but missed my mea culpas.  Posted fixes
for the configs: https://lore.kernel.org/all/20230404234112.367850-1-seanjc@google.com

Re: [kvm-unit-tests PATCH v4 5/9] x86/access: Add forced emulation support

[Mathias Krause]

On 04.04.23 18:53, Sean Christopherson wrote:
> From: Mathias Krause <minipli@grsecurity.net>

I saw you did some additional changes. Comments below...

> 
> Add support to force access tests to be handled by the emulator, if
> supported by KVM.  Due to emulation being rather slow, make the forced
> emulation nodefault, i.e. a manual-only testcase.  Note, "manual" just
> means the test runner needs to specify a specific group(s), i.e. the
> test is still easy to enable in CI for compatible setups.
> 
> Manually check for FEP support in the test instead of using the "check"
> option to query the module param, as any non-zero force_emulation_prefix
> value enables FEP (see KVM commit d500e1ed3dc8 ("KVM: x86: Allow clearing
> RFLAGS.RF on forced emulation to test code #DBs") and scripts/runtime.bash
> only supports checking for a single value.
> 
> Defer enabling FEP for the nested VMX variants to a future patch.  KVM has
> a bug related to emulating NOP (yes, NOP) for L2, and the nVMX varaints
                                                                    ~~
typo: variants

> will require additional massaging to propagate the "allow emulation" flag.
> 
> Signed-off-by: Mathias Krause <minipli@grsecurity.net>
> [sean: make generic fep testcase nodefault instead of impossible]
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  x86/access.c      | 38 +++++++++++++++++++++++++++++++-------
>  x86/access.h      |  2 +-
>  x86/access_test.c |  8 +++++---
>  x86/unittests.cfg |  6 ++++++
>  x86/vmx_tests.c   |  2 +-
>  5 files changed, 44 insertions(+), 12 deletions(-)
> 
> diff --git a/x86/access.c b/x86/access.c
> index 1677d52e..4a3ca265 100644
> --- a/x86/access.c
> +++ b/x86/access.c
> @@ -82,7 +82,9 @@ enum {
>  	AC_CPU_CR4_SMEP_BIT,
>  	AC_CPU_CR4_PKE_BIT,
>  
> -	NR_AC_FLAGS
> +	AC_FEP_BIT,
> +
> +	NR_AC_FLAGS,
>  };
>  
>  #define AC_PTE_PRESENT_MASK   (1 << AC_PTE_PRESENT_BIT)
> @@ -121,6 +123,8 @@ enum {
>  #define AC_CPU_CR4_SMEP_MASK  (1 << AC_CPU_CR4_SMEP_BIT)
>  #define AC_CPU_CR4_PKE_MASK   (1 << AC_CPU_CR4_PKE_BIT)
>  
> +#define AC_FEP_MASK           (1 << AC_FEP_BIT)
> +
>  const char *ac_names[] = {
>  	[AC_PTE_PRESENT_BIT] = "pte.p",
>  	[AC_PTE_ACCESSED_BIT] = "pte.a",
> @@ -152,6 +156,7 @@ const char *ac_names[] = {
>  	[AC_CPU_CR0_WP_BIT] = "cr0.wp",
>  	[AC_CPU_CR4_SMEP_BIT] = "cr4.smep",
>  	[AC_CPU_CR4_PKE_BIT] = "cr4.pke",
> +	[AC_FEP_BIT] = "fep",
>  };
>  
>  static inline void *va(pt_element_t phys)
> @@ -799,10 +804,13 @@ static int ac_test_do_access(ac_test_t *at)
>  
>  	if (F(AC_ACCESS_TWICE)) {
>  		asm volatile ("mov $fixed2, %%rsi \n\t"
> -			      "mov (%[addr]), %[reg] \n\t"
> +			      "cmp $0, %[fep] \n\t"
> +			      "jz 1f \n\t"
> +			      KVM_FEP
> +			      "1: mov (%[addr]), %[reg] \n\t"
>  			      "fixed2:"
>  			      : [reg]"=r"(r), [fault]"=a"(fault), "=b"(e)
> -			      : [addr]"r"(at->virt)
> +			      : [addr]"r"(at->virt), [fep]"r"(F(AC_FEP))
>  			      : "rsi");
>  		fault = 0;
>  	}
> @@ -823,9 +831,15 @@ static int ac_test_do_access(ac_test_t *at)
>  		      "jnz 2f \n\t"
>  		      "cmp $0, %[write] \n\t"
>  		      "jnz 1f \n\t"
> -		      "mov (%[addr]), %[reg] \n\t"
> +		      "cmp $0, %[fep] \n\t"
> +		      "jz 0f \n\t"
> +		      KVM_FEP
> +		      "0: mov (%[addr]), %[reg] \n\t"
>  		      "jmp done \n\t"
> -		      "1: mov %[reg], (%[addr]) \n\t"
> +		      "1: cmp $0, %[fep] \n\t"
> +		      "jz 0f \n\t"
> +		      KVM_FEP
> +		      "0: mov %[reg], (%[addr]) \n\t"
>  		      "jmp done \n\t"
>  		      "2: call *%[addr] \n\t"
>  		      "done: \n"
> @@ -843,6 +857,7 @@ static int ac_test_do_access(ac_test_t *at)
>  			[write]"r"(F(AC_ACCESS_WRITE)),
>  			[user]"r"(F(AC_ACCESS_USER)),
>  			[fetch]"r"(F(AC_ACCESS_FETCH)),
> +			[fep]"r"(F(AC_FEP)),
>  			[user_ds]"i"(USER_DS),
>  			[user_cs]"i"(USER_CS),
>  			[user_stack_top]"r"(user_stack + sizeof user_stack),
> @@ -1209,12 +1224,17 @@ const ac_test_fn ac_test_cases[] =
>  	check_effective_sp_permissions,
>  };
>  
> -void ac_test_run(int pt_levels)
> +void ac_test_run(int pt_levels, bool force_emulation)
>  {
>  	ac_test_t at;
>  	ac_pt_env_t pt_env;
>  	int i, tests, successes;
>  
> +	if (force_emulation && !is_fep_available()) {
> +		report_skip("Forced emulation prefix (FEP) not available\n");

Trailing "\n" isn't needed, report_skip() already emits one.

> +		return;
> +	}
> +
>  	printf("run\n");
>  	tests = successes = 0;
>  
> @@ -1232,6 +1252,9 @@ void ac_test_run(int pt_levels)
>  		invalid_mask |= AC_PTE_BIT36_MASK;
>  	}
>  
> +	if (!force_emulation)
> +		invalid_mask |= AC_FEP_MASK;
> +
>  	ac_env_int(&pt_env, pt_levels);
>  	ac_test_init(&at, 0xffff923400000000ul, &pt_env);
>  
> @@ -1292,5 +1315,6 @@ void ac_test_run(int pt_levels)
>  
>  	printf("\n%d tests, %d failures\n", tests, tests - successes);
>  
> -	report(successes == tests, "%d-level paging tests", pt_levels);
> +	report(successes == tests, "%d-level paging tests%s", pt_levels,
> +	       force_emulation ? " (with forced emulation)" : "");
>  }
> diff --git a/x86/access.h b/x86/access.h
> index 9a6c5628..206a1c86 100644
> --- a/x86/access.h
> +++ b/x86/access.h
> @@ -4,6 +4,6 @@
>  #define PT_LEVEL_PML4 4
>  #define PT_LEVEL_PML5 5
>  
> -void ac_test_run(int page_table_levels);
> +void ac_test_run(int page_table_levels, bool force_emulation);
>  
>  #endif // X86_ACCESS_H

> \ No newline at end of file

Maybe fix that while already touching the file?

> diff --git a/x86/access_test.c b/x86/access_test.c
> index 2ac649d2..025294da 100644
> --- a/x86/access_test.c
> +++ b/x86/access_test.c
> @@ -3,10 +3,12 @@
>  #include "x86/vm.h"
>  #include "access.h"
>  
> -int main(void)
> +int main(int argc, const char *argv[])
>  {
> +	bool force_emulation = argc >= 2 && !strcmp(argv[1], "force_emulation");
> +
>  	printf("starting test\n\n");
> -	ac_test_run(PT_LEVEL_PML4);
> +	ac_test_run(PT_LEVEL_PML4, force_emulation);
>  
>  #ifndef CONFIG_EFI
>  	/*
> @@ -16,7 +18,7 @@ int main(void)
>  	if (this_cpu_has(X86_FEATURE_LA57)) {
>  		printf("starting 5-level paging test.\n\n");
>  		setup_5level_page_table();
> -		ac_test_run(PT_LEVEL_PML5);
> +		ac_test_run(PT_LEVEL_PML5, force_emulation);
>  	}
>  #endif
>  
> diff --git a/x86/unittests.cfg b/x86/unittests.cfg
> index f324e32d..6194e0ea 100644
> --- a/x86/unittests.cfg
> +++ b/x86/unittests.cfg
> @@ -143,6 +143,12 @@ file = access_test.flat
>  arch = x86_64
>  extra_params = -cpu max
>  
> +[access_fep]
> +file = access_test.flat
> +arch = x86_64
> +extra_params = -cpu max -append force_emulation
> +groups = nodefault
> +
>  [access-reduced-maxphyaddr]
>  file = access_test.flat
>  arch = x86_64
> diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
> index 7bba8165..617b97b3 100644
> --- a/x86/vmx_tests.c
> +++ b/x86/vmx_tests.c
> @@ -10460,7 +10460,7 @@ static void atomic_switch_overflow_msrs_test(void)
>  
>  static void vmx_pf_exception_test_guest(void)
>  {
> -	ac_test_run(PT_LEVEL_PML4);
> +	ac_test_run(PT_LEVEL_PML4, false);
>  }
>  
>  typedef void (*invalidate_tlb_t)(void *data);

Re: [kvm-unit-tests PATCH v4 6/9] x86/access: Try forced emulation for CR0.WP test as well

[Mathias Krause]

On 04.04.23 18:53, Sean Christopherson wrote:
> From: Mathias Krause <minipli@grsecurity.net>
> 
> Enhance the CR0.WP toggling test to do additional tests via the emulator
> as forcing KVM to emulate page protections exercises different flows than
> shoving the correct bits into hardware, e.g. KVM has had at least one bug
> when CR0.WP is guest owned.
> 
> Link: https://lore.kernel.org/kvm/ea3a8fbc-2bf8-7442-e498-3e5818384c83@grsecurity.net
> Signed-off-by: Mathias Krause <minipli@grsecurity.net>

> [sean: check AC_FEP_MASK instead of fep_available()]

Hmm. But this excludes the emulator CR0.WP tests by default, even when
FEP is supported by KVM. :(

> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  x86/access.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/x86/access.c b/x86/access.c
> index 4a3ca265..70d81bf0 100644
> --- a/x86/access.c
> +++ b/x86/access.c
> @@ -1108,8 +1108,9 @@ static int do_cr0_wp_access(ac_test_t *at, int flags)
>  	set_efer_nx(0);
>  
>  	if (!ac_test_do_access(at)) {
> -		printf("%s: supervisor write with CR0.WP=%d did not %s\n",
> -		       __FUNCTION__, cr0_wp, cr0_wp ? "FAULT" : "SUCCEED");
> +		printf("%s: %ssupervisor write with CR0.WP=%d did not %s\n",
> +		       __FUNCTION__, (flags & AC_FEP_MASK) ? "emulated " : "",
> +		       cr0_wp, cr0_wp ? "FAULT" : "SUCCEED");
>  		return 1;
>  	}
>  
> @@ -1127,6 +1128,10 @@ static int check_toggle_cr0_wp(ac_pt_env_t *pt_env)
>  
>  	err += do_cr0_wp_access(&at, 0);
>  	err += do_cr0_wp_access(&at, AC_CPU_CR0_WP_MASK);

> +	if (!(invalid_mask & AC_FEP_MASK)) {

Can we *please* change this back to 'if (is_fep_available()) {'...? I
really would like to get these tests exercised by default if possible.
Runtime slowdown is no argument here, as that's only a whopping two
emulated accesses.

What was the reason to exclude them? Less test coverage can't be it,
right? ;)

> +		err += do_cr0_wp_access(&at, AC_FEP_MASK);
> +		err += do_cr0_wp_access(&at, AC_FEP_MASK | AC_CPU_CR0_WP_MASK);
> +	}
>  
>  	return err == 0;
>  }

Re: [kvm-unit-tests PATCH v4 6/9] x86/access: Try forced emulation for CR0.WP test as well

[Sean Christopherson]

On Wed, Apr 05, 2023, Mathias Krause wrote:
> On 04.04.23 18:53, Sean Christopherson wrote:
> > @@ -1127,6 +1128,10 @@ static int check_toggle_cr0_wp(ac_pt_env_t *pt_env)
> >  
> >  	err += do_cr0_wp_access(&at, 0);
> >  	err += do_cr0_wp_access(&at, AC_CPU_CR0_WP_MASK);
> 
> > +	if (!(invalid_mask & AC_FEP_MASK)) {
> 
> Can we *please* change this back to 'if (is_fep_available()) {'...? I
> really would like to get these tests exercised by default if possible.

"by default" is a bit misleading IMO.  The vast majority of developers almost
certainly do not do testing with FEP enabled.

> Runtime slowdown is no argument here, as that's only a whopping two
> emulated accesses.
> 
> What was the reason to exclude them? Less test coverage can't be it,
> right? ;)

The goal is to reach a balance between the cost of maintenance, principle of least
surprise, and test coverage.  Ease of debugging also factors in (if the FEP version
fails but the non-FEP versions does not), but that's largely a bonus.

Defining a @force_emulation but then ignoring it for a one-off test violates the
principle of least suprise.

Plumbing a second param/flag into check_toggle_cr0_wp() would, IMO, unnecessarily
increase the maintenance cost.  Ditto for creating a more complex param.

As for test coverage side, I doubt that honoring @force_emulation reduces test
coverage in practice.  As above, most developers likely do not test with FEP.  I
doubt most CI setups that run KUT enable FEP either.  And if CI/developers do
automatically enable FEP, I would be shocked/saddened if adding an additional
configuration is more difficult than overiding a module param.  E.g. I will soon
be modifying my scripts to do both.

Re: [kvm-unit-tests PATCH v4 6/9] x86/access: Try forced emulation for CR0.WP test as well

[Mathias Krause]

On 05.04.23 16:29, Sean Christopherson wrote:
> On Wed, Apr 05, 2023, Mathias Krause wrote:
>> On 04.04.23 18:53, Sean Christopherson wrote:
>>> @@ -1127,6 +1128,10 @@ static int check_toggle_cr0_wp(ac_pt_env_t *pt_env)
>>>  
>>>  	err += do_cr0_wp_access(&at, 0);
>>>  	err += do_cr0_wp_access(&at, AC_CPU_CR0_WP_MASK);
>>
>>> +	if (!(invalid_mask & AC_FEP_MASK)) {
>>
>> Can we *please* change this back to 'if (is_fep_available()) {'...? I
>> really would like to get these tests exercised by default if possible.
> 
> "by default" is a bit misleading IMO.  The vast majority of developers almost
> certainly do not do testing with FEP enabled.

Fair enough. But with "by default if possible" I meant, if kvm.ko was
already loaded with force_emulation_prefix=1, the CR0.WP access tests
should automatically make use of it -- much like it's done in other
tests, like x86/emulator.c, x86/emulator64.c and x86/pmu.c. Or do you
want to change these tests to get a new "force_emulation" parameter as
well and disable the automatic detection and usage of FEP support in
tests completely? That would be quite counter-intuitive to reach a good
test coverage goal.

> 
>> Runtime slowdown is no argument here, as that's only a whopping two
>> emulated accesses.
>>
>> What was the reason to exclude them? Less test coverage can't be it,
>> right? ;)
> 
> The goal is to reach a balance between the cost of maintenance, principle of least
> surprise, and test coverage.  Ease of debugging also factors in (if the FEP version
> fails but the non-FEP versions does not), but that's largely a bonus.

It's a bonus on the test coverage side, IMHO. If the FEP version fails
but the non-FEP one doesn't, apparently something is broken somewhere
and should be fixed.

> 
> Defining a @force_emulation but then ignoring it for a one-off test violates the
> principle of least suprise.

Do we need additional parameters for PKU / SMEP / SMAP / LA57 as well or
leave the automatic detection in place? </rhetorical question>

We only need the "force_emulation" parameter because the ac_test_bump()
loop is so much slower with forced emulation. That's the only reason for
it to exists. We can rename it to "full" and do the force emulation
tests for ac_test_exec() if FEP is available. But just excluding some
(cheap) tests because some command line argument wasn't provided would
be surprising to me. Tests should be simple to use, IMO.

> 
> Plumbing a second param/flag into check_toggle_cr0_wp() would, IMO, unnecessarily
> increase the maintenance cost.  Ditto for creating a more complex param.

Fully agree, no need for additional parameters. The existing one should
simply be renamed to "full" and just control ac_test_exec()'s behavior.

> 
> As for test coverage side, I doubt that honoring @force_emulation reduces test
> coverage in practice.  As above, most developers likely do not test with FEP.

Well, I do ;)

>  I
> doubt most CI setups that run KUT enable FEP either.  And if CI/developers do
> automatically enable FEP, I would be shocked/saddened if adding an additional
> configuration is more difficult than overiding a module param.  E.g. I will soon
> be modifying my scripts to do both.

Well, the force emulation access tests take a significant amount of time
to run, so will likely be disabled for CI systems that run on a free
tier basis. But do we need to disable the possibility to run the corner
case test as well? I don't think so. If some CI system already takes the
effort to manually load kvm.ko with force_emulation_prefix=1, it should
get these additional cheap tests automatically instead of having the
need to carry additional patches to get them.

Thanks,
Mathias

Re: [kvm-unit-tests PATCH v4 6/9] x86/access: Try forced emulation for CR0.WP test as well

[Sean Christopherson]

On Wed, Apr 05, 2023, Mathias Krause wrote:
> On 05.04.23 16:29, Sean Christopherson wrote:
> > On Wed, Apr 05, 2023, Mathias Krause wrote:
> >> On 04.04.23 18:53, Sean Christopherson wrote:
> >>> @@ -1127,6 +1128,10 @@ static int check_toggle_cr0_wp(ac_pt_env_t *pt_env)
> >>>  
> >>>  	err += do_cr0_wp_access(&at, 0);
> >>>  	err += do_cr0_wp_access(&at, AC_CPU_CR0_WP_MASK);
> >>
> >>> +	if (!(invalid_mask & AC_FEP_MASK)) {
> >>
> >> Can we *please* change this back to 'if (is_fep_available()) {'...? I
> >> really would like to get these tests exercised by default if possible.
> > 
> > "by default" is a bit misleading IMO.  The vast majority of developers almost
> > certainly do not do testing with FEP enabled.
> 
> Fair enough. But with "by default if possible" I meant, if kvm.ko was
> already loaded with force_emulation_prefix=1, the CR0.WP access tests
> should automatically make use of it -- much like it's done in other
> tests, like x86/emulator.c, x86/emulator64.c and x86/pmu.c. Or do you
> want to change these tests to get a new "force_emulation" parameter as
> well and disable the automatic detection and usage of FEP support in
> tests completely?

In an ideal world, the access test would use FEP if it's available, i.e. not make
it a separate test case.  The unfortunate reality is that the runtime is simply
too long to do that for the test as a whole.

> > The goal is to reach a balance between the cost of maintenance, principle of least
> > surprise, and test coverage.  Ease of debugging also factors in (if the FEP version
> > fails but the non-FEP versions does not), but that's largely a bonus.
> 
> It's a bonus on the test coverage side, IMHO. If the FEP version fails
> but the non-FEP one doesn't, apparently something is broken somewhere
> and should be fixed.

For sure.  What I'm saying is that on my end, I can skip a non-trivial amount of
triage/debug if "access" passes but "access_fep" fails.  That info _should_ also
be captured in the log, but not all CI setups actually do that, e.g. the "official"
gitlab based CI doesn't capture logs (and it's a giant pain).  But again, this is
not a primary motivator, it's just a happy bonus.

> > Defining a @force_emulation but then ignoring it for a one-off test violates the
> > principle of least suprise.
> 
> Do we need additional parameters for PKU / SMEP / SMAP / LA57 as well or
> leave the automatic detection in place? </rhetorical question>
> 
> We only need the "force_emulation" parameter because the ac_test_bump()
> loop is so much slower with forced emulation. That's the only reason for
> it to exists. We can rename it to "full" and do the force emulation
> tests for ac_test_exec() if FEP is available. But just excluding some
> (cheap) tests because some command line argument wasn't provided would
> be surprising to me. Tests should be simple to use, IMO.

Look at it from the perspective of someone who has never run these tests and has
no clue what the test does, let alone the gory details of FEP.  The most straightforward
interpretation of force_emulation=false is that the test does not force emulation,
thus having a one-off testcase that forces emulation anyways would be surprising.

E.g. imagine being the person that discovered the VMX #PF test failed because of
the KVM bug where the emulator barfs on a NOP for L2.  Before they even get near
the KVM bug itself, the person would be wondering why on earth a NOP is getting a
#UD.  They would likely then discover FEP and ask the obvious question of why the
test is trying to use FEP even though forced emulation is allegedly disabled.

That can obviously be solved by comments to explain what "full" means, but as below,
I would prefer to avoid that if possible.

> > Plumbing a second param/flag into check_toggle_cr0_wp() would, IMO, unnecessarily
> > increase the maintenance cost.  Ditto for creating a more complex param.
> 
> Fully agree, no need for additional parameters. The existing one should
> simply be renamed to "full" and just control ac_test_exec()'s behavior.
>
> > I doubt most CI setups that run KUT enable FEP either.  And if
> > CI/developers do automatically enable FEP, I would be shocked/saddened if
> > adding an additional configuration is more difficult than overiding a
> > module param.  E.g. I will soon be modifying my scripts to do both.
> 
> Well, the force emulation access tests take a significant amount of time
> to run, so will likely be disabled for CI systems that run on a free
> tier basis. But do we need to disable the possibility to run the corner
> case test as well? I don't think so. If some CI system already takes the
> effort to manually load kvm.ko with force_emulation_prefix=1, it should
> get these additional cheap tests automatically instead of having the
> need to carry additional patches to get them.

If that ends up being the case then I'm totally fine tweaking the param to gate
only the main loop, but my preference is to wait until it's actually a real
problem.  I.e. take on more complexity, even though it is minor, if and only if
we really need to.