[PATCH v2 0/3] KVM: selftests: Fix nx_huge_pages_test when TDP is disabled

[PATCH v2 0/3] KVM: selftests: Fix nx_huge_pages_test when TDP is disabled

[David Matlack]

This builds on the v1 fix for nx_huge_pages_test for TDP-disabled hosts.
Originally this was just 1 patch but now it is 3 to add the necessary
infrastructure to check if TDP is enabled or not so that we can
conditionally use 4KiB or 2MiB mappings in the test.

Sean, I opted not to refactor virt_map() to take nr_bytes since that
will take a bit more work than I have cycles for at the moment and I
want to get this fix in. That being said, I coded up the new
virt_map_level() using nr_bytes so that there will be less to clean up
in the future.

v2:
 - Still use 4K mappins on TDP-enabled hosts [Sean]
 - Generalize virt_map_2m() to virt_map_level() [me]
 - Pass nr_bytes instead of nr_pages to virt_map_level() [Sean]

v1: https://lore.kernel.org/kvm/20220926175219.605113-1-dmatlack@google.com/

David Matlack (3):
  KVM: selftests: Tell the compiler that code after TEST_FAIL() is
    unreachable
  KVM: selftests: Add helper to read boolean module parameters
  KVM: selftests: Fix nx_huge_pages_test on TDP-disabled hosts

 .../testing/selftests/kvm/include/test_util.h |  7 +++-
 .../selftests/kvm/include/x86_64/processor.h  |  4 ++
 tools/testing/selftests/kvm/lib/test_util.c   | 31 ++++++++++++++
 .../selftests/kvm/lib/x86_64/processor.c      | 40 +++++++++++++------
 .../selftests/kvm/x86_64/nx_huge_pages_test.c | 19 ++++++++-
 5 files changed, 85 insertions(+), 16 deletions(-)


base-commit: 372d07084593dc7a399bf9bee815711b1fb1bcf2
prerequisite-patch-id: 2e3661ba8856c29b769499bac525b6943d9284b8
-- 
2.37.3.998.g577e59143f-goog

[PATCH v2 1/3] KVM: selftests: Tell the compiler that code after TEST_FAIL() is unreachable

[David Matlack]

Add __builtin_unreachable() to TEST_FAIL() so that the compiler knows
that any code after a TEST_FAIL() is unreachable.

Signed-off-by: David Matlack <dmatlack@google.com>
---
 tools/testing/selftests/kvm/include/test_util.h | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/kvm/include/test_util.h b/tools/testing/selftests/kvm/include/test_util.h
index 5c5a88180b6c..befc754ce9b3 100644
--- a/tools/testing/selftests/kvm/include/test_util.h
+++ b/tools/testing/selftests/kvm/include/test_util.h
@@ -63,8 +63,10 @@ void test_assert(bool exp, const char *exp_str,
 		    #a, #b, #a, (unsigned long) __a, #b, (unsigned long) __b); \
 } while (0)
 
-#define TEST_FAIL(fmt, ...) \
-	TEST_ASSERT(false, fmt, ##__VA_ARGS__)
+#define TEST_FAIL(fmt, ...) do { \
+	TEST_ASSERT(false, fmt, ##__VA_ARGS__); \
+	__builtin_unreachable(); \
+} while (0)
 
 size_t parse_size(const char *size);
 
-- 
2.37.3.998.g577e59143f-goog

[PATCH v2 2/3] KVM: selftests: Add helper to read boolean module parameters

[David Matlack]

Add a common helper function for reading boolean module parameters and
use it in vm_is_unrestricted_guest() to check the value of
kvm_intel.unrestricted_guest.

Note that vm_is_unrestricted_guest() will now fail with a TEST_ASSERT()
if called on AMD instead of just returning false. However this should
not cause any functional change since all of the callers of
vm_is_unrestricted_guest() first check is_intel_cpu().

No functional change intended.

Signed-off-by: David Matlack <dmatlack@google.com>
---
 .../testing/selftests/kvm/include/test_util.h |  1 +
 tools/testing/selftests/kvm/lib/test_util.c   | 31 +++++++++++++++++++
 .../selftests/kvm/lib/x86_64/processor.c      | 13 +-------
 3 files changed, 33 insertions(+), 12 deletions(-)

diff --git a/tools/testing/selftests/kvm/include/test_util.h b/tools/testing/selftests/kvm/include/test_util.h
index befc754ce9b3..4f119fd84ae5 100644
--- a/tools/testing/selftests/kvm/include/test_util.h
+++ b/tools/testing/selftests/kvm/include/test_util.h
@@ -108,6 +108,7 @@ struct vm_mem_backing_src_alias {
 
 #define MIN_RUN_DELAY_NS	200000UL
 
+bool get_module_param_bool(const char *module_name, const char *param);
 bool thp_configured(void);
 size_t get_trans_hugepagesz(void);
 size_t get_def_hugetlb_pagesz(void);
diff --git a/tools/testing/selftests/kvm/lib/test_util.c b/tools/testing/selftests/kvm/lib/test_util.c
index 6d23878bbfe1..479e482d3202 100644
--- a/tools/testing/selftests/kvm/lib/test_util.c
+++ b/tools/testing/selftests/kvm/lib/test_util.c
@@ -9,6 +9,7 @@
 #include <ctype.h>
 #include <limits.h>
 #include <stdlib.h>
+#include <stdio.h>
 #include <time.h>
 #include <sys/stat.h>
 #include <sys/syscall.h>
@@ -114,6 +115,36 @@ void print_skip(const char *fmt, ...)
 	puts(", skipping test");
 }
 
+bool get_module_param_bool(const char *module_name, const char *param)
+{
+	const int path_size = 1024;
+	char path[path_size];
+	char value;
+	FILE *f;
+	int r;
+
+	r = snprintf(path, path_size, "/sys/module/%s/parameters/%s",
+		     module_name, param);
+	TEST_ASSERT(r < path_size,
+		    "Failed to construct sysfs path in %d bytes.", path_size);
+
+	f = fopen(path, "r");
+	TEST_ASSERT(f, "fopen(%s) failed", path);
+
+	r = fread(&value, 1, 1, f);
+	TEST_ASSERT(r == 1, "fread(%s) failed", path);
+
+	r = fclose(f);
+	TEST_ASSERT(!r, "fclose(%s) failed", path);
+
+	if (value == 'Y')
+		return true;
+	else if (value == 'N')
+		return false;
+
+	TEST_FAIL("Unrecognized value: %c", value);
+}
+
 bool thp_configured(void)
 {
 	int ret;
diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools/testing/selftests/kvm/lib/x86_64/processor.c
index 2e6e61bbe81b..522d3e2009fb 100644
--- a/tools/testing/selftests/kvm/lib/x86_64/processor.c
+++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c
@@ -1294,20 +1294,9 @@ unsigned long vm_compute_max_gfn(struct kvm_vm *vm)
 /* Returns true if kvm_intel was loaded with unrestricted_guest=1. */
 bool vm_is_unrestricted_guest(struct kvm_vm *vm)
 {
-	char val = 'N';
-	size_t count;
-	FILE *f;
-
 	/* Ensure that a KVM vendor-specific module is loaded. */
 	if (vm == NULL)
 		close(open_kvm_dev_path_or_exit());
 
-	f = fopen("/sys/module/kvm_intel/parameters/unrestricted_guest", "r");
-	if (f) {
-		count = fread(&val, sizeof(char), 1, f);
-		TEST_ASSERT(count == 1, "Unable to read from param file.");
-		fclose(f);
-	}
-
-	return val == 'Y';
+	return get_module_param_bool("kvm_intel", "unrestricted_guest");
 }
-- 
2.37.3.998.g577e59143f-goog

[PATCH v2 3/3] KVM: selftests: Fix nx_huge_pages_test on TDP-disabled hosts

[David Matlack]

Map the test's huge page region with 2MiB virtual mappings when TDP is
disabled so that KVM can shadow the region with huge pages. This fixes
nx_huge_pages_test on hosts where TDP hardware support is disabled.

Purposely do not skip this test on TDP-disabled hosts. While we don't
care about NX Huge Pages on TDP-disabled hosts from a security
perspective, KVM does support it, and so we should test it.

For TDP-enabled hosts, continue mapping the region with 4KiB pages to
ensure that KVM can map it with huge pages irrespective of the guest
mappings.

Fixes: 8448ec5993be ("KVM: selftests: Add NX huge pages test")
Signed-off-by: David Matlack <dmatlack@google.com>
---
 .../selftests/kvm/include/x86_64/processor.h  |  4 +++
 .../selftests/kvm/lib/x86_64/processor.c      | 27 +++++++++++++++++++
 .../selftests/kvm/x86_64/nx_huge_pages_test.c | 19 +++++++++++--
 3 files changed, 48 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/kvm/include/x86_64/processor.h b/tools/testing/selftests/kvm/include/x86_64/processor.h
index 0cbc71b7af50..3082c2a4089b 100644
--- a/tools/testing/selftests/kvm/include/x86_64/processor.h
+++ b/tools/testing/selftests/kvm/include/x86_64/processor.h
@@ -825,6 +825,8 @@ static inline uint8_t wrmsr_safe(uint32_t msr, uint64_t val)
 	return kvm_asm_safe("wrmsr", "a"(val & -1u), "d"(val >> 32), "c"(msr));
 }
 
+bool kvm_tdp_enabled(void);
+
 uint64_t vm_get_page_table_entry(struct kvm_vm *vm, struct kvm_vcpu *vcpu,
 				 uint64_t vaddr);
 void vm_set_page_table_entry(struct kvm_vm *vm, struct kvm_vcpu *vcpu,
@@ -855,6 +857,8 @@ enum pg_level {
 #define PG_SIZE_1G PG_LEVEL_SIZE(PG_LEVEL_1G)
 
 void __virt_pg_map(struct kvm_vm *vm, uint64_t vaddr, uint64_t paddr, int level);
+void virt_map_level(struct kvm_vm *vm, uint64_t vaddr, uint64_t paddr,
+		    uint64_t nr_bytes, int level);
 
 /*
  * Basic CPU control in CR0
diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools/testing/selftests/kvm/lib/x86_64/processor.c
index 522d3e2009fb..5b2ee0c32e27 100644
--- a/tools/testing/selftests/kvm/lib/x86_64/processor.c
+++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c
@@ -111,6 +111,14 @@ static void sregs_dump(FILE *stream, struct kvm_sregs *sregs, uint8_t indent)
 	}
 }
 
+bool kvm_tdp_enabled(void)
+{
+	if (is_amd_cpu())
+		return get_module_param_bool("kvm_amd", "npt");
+	else
+		return get_module_param_bool("kvm_intel", "ept");
+}
+
 void virt_arch_pgd_alloc(struct kvm_vm *vm)
 {
 	TEST_ASSERT(vm->mode == VM_MODE_PXXV48_4K, "Attempt to use "
@@ -214,6 +222,25 @@ void virt_arch_pg_map(struct kvm_vm *vm, uint64_t vaddr, uint64_t paddr)
 	__virt_pg_map(vm, vaddr, paddr, PG_LEVEL_4K);
 }
 
+void virt_map_level(struct kvm_vm *vm, uint64_t vaddr, uint64_t paddr,
+		    uint64_t nr_bytes, int level)
+{
+	uint64_t pg_size = PG_LEVEL_SIZE(level);
+	uint64_t nr_pages = nr_bytes / pg_size;
+	int i;
+
+	TEST_ASSERT(nr_bytes % pg_size == 0,
+		    "Region size not aligned: nr_bytes: 0x%lx, page size: 0x%lx",
+		    nr_bytes, pg_size);
+
+	for (i = 0; i < nr_pages; i++) {
+		__virt_pg_map(vm, vaddr, paddr, level);
+
+		vaddr += pg_size;
+		paddr += pg_size;
+	}
+}
+
 static uint64_t *_vm_get_page_table_entry(struct kvm_vm *vm,
 					  struct kvm_vcpu *vcpu,
 					  uint64_t vaddr)
diff --git a/tools/testing/selftests/kvm/x86_64/nx_huge_pages_test.c b/tools/testing/selftests/kvm/x86_64/nx_huge_pages_test.c
index cc6421716400..e50e3a50ed9d 100644
--- a/tools/testing/selftests/kvm/x86_64/nx_huge_pages_test.c
+++ b/tools/testing/selftests/kvm/x86_64/nx_huge_pages_test.c
@@ -112,6 +112,7 @@ void run_test(int reclaim_period_ms, bool disable_nx_huge_pages,
 {
 	struct kvm_vcpu *vcpu;
 	struct kvm_vm *vm;
+	uint64_t nr_bytes;
 	void *hva;
 	int r;
 
@@ -141,10 +142,24 @@ void run_test(int reclaim_period_ms, bool disable_nx_huge_pages,
 				    HPAGE_GPA, HPAGE_SLOT,
 				    HPAGE_SLOT_NPAGES, 0);
 
-	virt_map(vm, HPAGE_GVA, HPAGE_GPA, HPAGE_SLOT_NPAGES);
+	nr_bytes = HPAGE_SLOT_NPAGES * vm->page_size;
+
+	/*
+	 * Ensure that KVM can map HPAGE_SLOT with huge pages by mapping the
+	 * region into the guest with 2MiB pages whenever TDP is disabled (i.e.
+	 * whenever KVM is shadowing the guest page tables).
+	 *
+	 * When TDP is enabled, KVM should be able to map HPAGE_SLOT with huge
+	 * pages irrespective of the guest page size, so map with 4KiB pages
+	 * to test that that is the case.
+	 */
+	if (kvm_tdp_enabled())
+		virt_map_level(vm, HPAGE_GVA, HPAGE_GPA, nr_bytes, PG_LEVEL_4K);
+	else
+		virt_map_level(vm, HPAGE_GVA, HPAGE_GPA, nr_bytes, PG_LEVEL_2M);
 
 	hva = addr_gpa2hva(vm, HPAGE_GPA);
-	memset(hva, RETURN_OPCODE, HPAGE_SLOT_NPAGES * PAGE_SIZE);
+	memset(hva, RETURN_OPCODE, nr_bytes);
 
 	check_2m_page_count(vm, 0);
 	check_split_count(vm, 0);
-- 
2.37.3.998.g577e59143f-goog

Re: [PATCH v2 2/3] KVM: selftests: Add helper to read boolean module parameters

[Sean Christopherson]

On Wed, Sep 28, 2022, David Matlack wrote:
> @@ -114,6 +115,36 @@ void print_skip(const char *fmt, ...)
>  	puts(", skipping test");
>  }
>  
> +bool get_module_param_bool(const char *module_name, const char *param)
> +{
> +	const int path_size = 1024;
> +	char path[path_size];
> +	char value;
> +	FILE *f;
> +	int r;
> +
> +	r = snprintf(path, path_size, "/sys/module/%s/parameters/%s",
> +		     module_name, param);
> +	TEST_ASSERT(r < path_size,
> +		    "Failed to construct sysfs path in %d bytes.", path_size);
> +
> +	f = fopen(path, "r");

Any particular reason for using fopen()?  Oh, because that's what the existing
code does.  More below.

> +	TEST_ASSERT(f, "fopen(%s) failed", path);

I don't actually care myself, but for consistency this should probably be a
skip condition.  The easiest thing would be to use open_path_or_exit().

At that point, assuming read() instead of fread() does the right thin, that seems
like the easiest solution.

> +	TEST_FAIL("Unrecognized value: %c", value);

Maybe be slightly more verbose?  E.g.

	TEST_FAIL("Unrecognized value '%c' for boolean module param", value);

> +}
> +
>  bool thp_configured(void)
>  {
>  	int ret;
> diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools/testing/selftests/kvm/lib/x86_64/processor.c
> index 2e6e61bbe81b..522d3e2009fb 100644
> --- a/tools/testing/selftests/kvm/lib/x86_64/processor.c
> +++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c
> @@ -1294,20 +1294,9 @@ unsigned long vm_compute_max_gfn(struct kvm_vm *vm)
>  /* Returns true if kvm_intel was loaded with unrestricted_guest=1. */
>  bool vm_is_unrestricted_guest(struct kvm_vm *vm)
>  {
> -	char val = 'N';
> -	size_t count;
> -	FILE *f;
> -
>  	/* Ensure that a KVM vendor-specific module is loaded. */
>  	if (vm == NULL)
>  		close(open_kvm_dev_path_or_exit());
>  
> -	f = fopen("/sys/module/kvm_intel/parameters/unrestricted_guest", "r");
> -	if (f) {
> -		count = fread(&val, sizeof(char), 1, f);
> -		TEST_ASSERT(count == 1, "Unable to read from param file.");
> -		fclose(f);
> -	}
> -
> -	return val == 'Y';
> +	return get_module_param_bool("kvm_intel", "unrestricted_guest");

Since there are only three possible modules, what about providing wrappers to
handle "kvm", "kvm_amd", and "kvm_intel"?  I'm guessing we'll end up with wrappers
for each param we care about, but one fewer strings to get right would be nice.

Re: [PATCH v2 3/3] KVM: selftests: Fix nx_huge_pages_test on TDP-disabled hosts

[Sean Christopherson]

On Wed, Sep 28, 2022, David Matlack wrote:
> diff --git a/tools/testing/selftests/kvm/include/x86_64/processor.h b/tools/testing/selftests/kvm/include/x86_64/processor.h
> index 0cbc71b7af50..3082c2a4089b 100644
> --- a/tools/testing/selftests/kvm/include/x86_64/processor.h
> +++ b/tools/testing/selftests/kvm/include/x86_64/processor.h
> @@ -825,6 +825,8 @@ static inline uint8_t wrmsr_safe(uint32_t msr, uint64_t val)
>  	return kvm_asm_safe("wrmsr", "a"(val & -1u), "d"(val >> 32), "c"(msr));
>  }
>  
> +bool kvm_tdp_enabled(void);

Uber nit, maybe kvm_is_tdp_enabled()?

Re: [PATCH v2 2/3] KVM: selftests: Add helper to read boolean module parameters

[David Matlack]

On Wed, Sep 28, 2022 at 3:51 PM Sean Christopherson <seanjc@google.com> wrote:
>
> On Wed, Sep 28, 2022, David Matlack wrote:
> > @@ -114,6 +115,36 @@ void print_skip(const char *fmt, ...)
> >       puts(", skipping test");
> >  }
> >
> > +bool get_module_param_bool(const char *module_name, const char *param)
> > +{
> > +     const int path_size = 1024;

(Unrelated to your review but I noticed 1024 is overkill. e.g.
"/sys/module/kvm_intel/parameters/error_on_inconsistent_vmcs_config"
is only 67 characters. I will reduce this in v3.)

> > +     char path[path_size];
> > +     char value;
> > +     FILE *f;
> > +     int r;
> > +
> > +     r = snprintf(path, path_size, "/sys/module/%s/parameters/%s",
> > +                  module_name, param);
> > +     TEST_ASSERT(r < path_size,
> > +                 "Failed to construct sysfs path in %d bytes.", path_size);
> > +
> > +     f = fopen(path, "r");
>
> Any particular reason for using fopen()?  Oh, because that's what the existing
> code does.  More below.

Heh, yes. I write C code to do file I/O about once every 2 years so I
always need to reference existing code :)

>
> > +     TEST_ASSERT(f, "fopen(%s) failed", path);
>
> I don't actually care myself, but for consistency this should probably be a
> skip condition.  The easiest thing would be to use open_path_or_exit().
>
> At that point, assuming read() instead of fread() does the right thin, that seems
> like the easiest solution.

Fine by me!

>
> > +     TEST_FAIL("Unrecognized value: %c", value);
>
> Maybe be slightly more verbose?  E.g.
>
>         TEST_FAIL("Unrecognized value '%c' for boolean module param", value);

Will do.

>
> > +}
> > +
> >  bool thp_configured(void)
> >  {
> >       int ret;
> > diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools/testing/selftests/kvm/lib/x86_64/processor.c
> > index 2e6e61bbe81b..522d3e2009fb 100644
> > --- a/tools/testing/selftests/kvm/lib/x86_64/processor.c
> > +++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c
> > @@ -1294,20 +1294,9 @@ unsigned long vm_compute_max_gfn(struct kvm_vm *vm)
> >  /* Returns true if kvm_intel was loaded with unrestricted_guest=1. */
> >  bool vm_is_unrestricted_guest(struct kvm_vm *vm)
> >  {
> > -     char val = 'N';
> > -     size_t count;
> > -     FILE *f;
> > -
> >       /* Ensure that a KVM vendor-specific module is loaded. */
> >       if (vm == NULL)
> >               close(open_kvm_dev_path_or_exit());
> >
> > -     f = fopen("/sys/module/kvm_intel/parameters/unrestricted_guest", "r");
> > -     if (f) {
> > -             count = fread(&val, sizeof(char), 1, f);
> > -             TEST_ASSERT(count == 1, "Unable to read from param file.");
> > -             fclose(f);
> > -     }
> > -
> > -     return val == 'Y';
> > +     return get_module_param_bool("kvm_intel", "unrestricted_guest");
>
> Since there are only three possible modules, what about providing wrappers to
> handle "kvm", "kvm_amd", and "kvm_intel"?  I'm guessing we'll end up with wrappers
> for each param we care about, but one fewer strings to get right would be nice.

Will do.