[PATCH v3 00/11] KVM: x86: Add a cap to disable NX hugepages on a VM

[PATCH v3 00/11] KVM: x86: Add a cap to disable NX hugepages on a VM

[Ben Gardon]

Given the high cost of NX hugepages in terms of TLB performance, it may
be desirable to disable the mitigation on a per-VM basis. In the case of public
cloud providers with many VMs on a single host, some VMs may be more trusted
than others. In order to maximize performance on critical VMs, while still
providing some protection to the host from iTLB Multihit, allow the mitigation
to be selectively disabled.

Disabling NX hugepages on a VM is relatively straightforward, but I took this
as an opportunity to add some NX hugepages test coverage and clean up selftests
infrastructure a bit.

This series was tested with the new selftest and the rest of the KVM selftests
on an Intel Haswell machine.

The following tests failed, but I do not believe that has anything to do with
this series:
	userspace_io_test
	vmx_nested_tsc_scaling_test
	vmx_preemption_timer_test

Changelog:
v1->v2:
	Dropped the complicated memslot refactor in favor of Ricardo Koller's
	patch with a similar effect.
	Incorporated David Dunn's feedback and reviewed by tag: shortened waits
	to speed up test.
v2->v3:
	Incorporated a suggestion from David on how to build the NX huge pages
	test.
	Fixed a build breakage identified by David.
	Dropped the per-vm nx_huge_pages field in favor of simply checking the
	global + per-VM disable override.
	Documented the new capability
	Separated out the commit to test disabling NX huge pages
	Removed permission check when checking if the disable NX capability is
	supported.
	Added test coverage for the permission check.

Ben Gardon (10):
  KVM: selftests: Dump VM stats in binary stats test
  KVM: selftests: Test reading a single stat
  KVM: selftests: Add memslot parameter to elf_load
  KVM: selftests: Improve error message in vm_phy_pages_alloc
  KVM: selftests: Add NX huge pages test
  KVM: x86/MMU: Factor out updating NX hugepages state for a VM
  KVM: x86/MMU: Allow NX huge pages to be disabled on a per-vm basis
  KVM: x86: Fix errant brace in KVM capability handling
  KVM: x86/MMU: Require reboot permission to disable NX hugepages
  selftests: KVM: Test disabling NX hugepages on a VM

Ricardo Koller (1):
  KVM: selftests: Add vm_alloc_page_table_in_memslot library function

 Documentation/virt/kvm/api.rst                |  13 +
 arch/x86/include/asm/kvm_host.h               |   2 +
 arch/x86/kvm/mmu.h                            |  10 +-
 arch/x86/kvm/mmu/mmu.c                        |  17 +-
 arch/x86/kvm/mmu/spte.c                       |   7 +-
 arch/x86/kvm/mmu/spte.h                       |   3 +-
 arch/x86/kvm/mmu/tdp_mmu.c                    |   3 +-
 arch/x86/kvm/x86.c                            |  17 +-
 include/uapi/linux/kvm.h                      |   1 +
 tools/testing/selftests/kvm/Makefile          |   7 +-
 .../selftests/kvm/include/kvm_util_base.h     |  10 +
 .../selftests/kvm/kvm_binary_stats_test.c     |   6 +
 tools/testing/selftests/kvm/lib/elf.c         |  13 +-
 tools/testing/selftests/kvm/lib/kvm_util.c    | 230 +++++++++++++++++-
 .../kvm/lib/x86_64/nx_huge_pages_guest.S      |  45 ++++
 .../selftests/kvm/x86_64/nx_huge_pages_test.c | 178 ++++++++++++++
 .../kvm/x86_64/nx_huge_pages_test.sh          |  25 ++
 17 files changed, 561 insertions(+), 26 deletions(-)
 create mode 100644 tools/testing/selftests/kvm/lib/x86_64/nx_huge_pages_guest.S
 create mode 100644 tools/testing/selftests/kvm/x86_64/nx_huge_pages_test.c
 create mode 100755 tools/testing/selftests/kvm/x86_64/nx_huge_pages_test.sh

-- 
2.35.1.1021.g381101b075-goog

[PATCH v3 01/11] KVM: selftests: Add vm_alloc_page_table_in_memslot library function

[Ben Gardon]

From: Ricardo Koller <ricarkol@google.com>

Add a library function to allocate a page-table physical page in a
particular memslot.  The default behavior is to create new page-table
pages in memslot 0.

Signed-off-by: Ricardo Koller <ricarkol@google.com>
Reviewed-by: Ben Gardon <bgardon@google.com>
Signed-off-by: Ben Gardon <bgardon@google.com>
---
 tools/testing/selftests/kvm/include/kvm_util_base.h | 1 +
 tools/testing/selftests/kvm/lib/kvm_util.c          | 8 +++++++-
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/kvm/include/kvm_util_base.h b/tools/testing/selftests/kvm/include/kvm_util_base.h
index 92cef0ffb19e..976aaaba8769 100644
--- a/tools/testing/selftests/kvm/include/kvm_util_base.h
+++ b/tools/testing/selftests/kvm/include/kvm_util_base.h
@@ -311,6 +311,7 @@ vm_paddr_t vm_phy_page_alloc(struct kvm_vm *vm, vm_paddr_t paddr_min,
 vm_paddr_t vm_phy_pages_alloc(struct kvm_vm *vm, size_t num,
 			      vm_paddr_t paddr_min, uint32_t memslot);
 vm_paddr_t vm_alloc_page_table(struct kvm_vm *vm);
+vm_paddr_t vm_alloc_page_table_in_memslot(struct kvm_vm *vm, uint32_t pt_memslot);
 
 /*
  * Create a VM with reasonable defaults
diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
index 1665a220abcb..11a692cf4570 100644
--- a/tools/testing/selftests/kvm/lib/kvm_util.c
+++ b/tools/testing/selftests/kvm/lib/kvm_util.c
@@ -2425,9 +2425,15 @@ vm_paddr_t vm_phy_page_alloc(struct kvm_vm *vm, vm_paddr_t paddr_min,
 /* Arbitrary minimum physical address used for virtual translation tables. */
 #define KVM_GUEST_PAGE_TABLE_MIN_PADDR 0x180000
 
+vm_paddr_t vm_alloc_page_table_in_memslot(struct kvm_vm *vm, uint32_t pt_memslot)
+{
+	return vm_phy_page_alloc(vm, KVM_GUEST_PAGE_TABLE_MIN_PADDR,
+			pt_memslot);
+}
+
 vm_paddr_t vm_alloc_page_table(struct kvm_vm *vm)
 {
-	return vm_phy_page_alloc(vm, KVM_GUEST_PAGE_TABLE_MIN_PADDR, 0);
+	return vm_alloc_page_table_in_memslot(vm, 0);
 }
 
 /*
-- 
2.35.1.1021.g381101b075-goog

[PATCH v3 02/11] KVM: selftests: Dump VM stats in binary stats test

[Ben Gardon]

Add kvm_util library functions to read KVM stats through the binary
stats interface and then dump them to stdout when running the binary
stats test. Subsequent commits will extend the kvm_util code and use it
to make assertions in a test for NX hugepages.

CC: Jing Zhang <jingzhangos@google.com>
Signed-off-by: Ben Gardon <bgardon@google.com>
---
 .../selftests/kvm/include/kvm_util_base.h     |   1 +
 .../selftests/kvm/kvm_binary_stats_test.c     |   3 +
 tools/testing/selftests/kvm/lib/kvm_util.c    | 143 ++++++++++++++++++
 3 files changed, 147 insertions(+)

diff --git a/tools/testing/selftests/kvm/include/kvm_util_base.h b/tools/testing/selftests/kvm/include/kvm_util_base.h
index 976aaaba8769..4783fd1cd4cf 100644
--- a/tools/testing/selftests/kvm/include/kvm_util_base.h
+++ b/tools/testing/selftests/kvm/include/kvm_util_base.h
@@ -401,6 +401,7 @@ void assert_on_unhandled_exception(struct kvm_vm *vm, uint32_t vcpuid);
 
 int vm_get_stats_fd(struct kvm_vm *vm);
 int vcpu_get_stats_fd(struct kvm_vm *vm, uint32_t vcpuid);
+void dump_vm_stats(struct kvm_vm *vm);
 
 uint32_t guest_get_vcpuid(void);
 
diff --git a/tools/testing/selftests/kvm/kvm_binary_stats_test.c b/tools/testing/selftests/kvm/kvm_binary_stats_test.c
index 17f65d514915..afc4701ce8dd 100644
--- a/tools/testing/selftests/kvm/kvm_binary_stats_test.c
+++ b/tools/testing/selftests/kvm/kvm_binary_stats_test.c
@@ -174,6 +174,9 @@ static void vm_stats_test(struct kvm_vm *vm)
 	stats_test(stats_fd);
 	close(stats_fd);
 	TEST_ASSERT(fcntl(stats_fd, F_GETFD) == -1, "Stats fd not freed");
+
+	/* Dump VM stats */
+	dump_vm_stats(vm);
 }
 
 static void vcpu_stats_test(struct kvm_vm *vm, int vcpu_id)
diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
index 11a692cf4570..f87df68b150d 100644
--- a/tools/testing/selftests/kvm/lib/kvm_util.c
+++ b/tools/testing/selftests/kvm/lib/kvm_util.c
@@ -2562,3 +2562,146 @@ int vcpu_get_stats_fd(struct kvm_vm *vm, uint32_t vcpuid)
 
 	return ioctl(vcpu->fd, KVM_GET_STATS_FD, NULL);
 }
+
+/* Caller is responsible for freeing the returned kvm_stats_header. */
+static struct kvm_stats_header *read_vm_stats_header(int stats_fd)
+{
+	struct kvm_stats_header *header;
+	ssize_t ret;
+
+	/* Read kvm stats header */
+	header = malloc(sizeof(*header));
+	TEST_ASSERT(header, "Allocate memory for stats header");
+
+	ret = read(stats_fd, header, sizeof(*header));
+	TEST_ASSERT(ret == sizeof(*header), "Read stats header");
+
+	return header;
+}
+
+static void dump_header(int stats_fd, struct kvm_stats_header *header)
+{
+	ssize_t ret;
+	char *id;
+
+	printf("flags: %u\n", header->flags);
+	printf("name size: %u\n", header->name_size);
+	printf("num_desc: %u\n", header->num_desc);
+	printf("id_offset: %u\n", header->id_offset);
+	printf("desc_offset: %u\n", header->desc_offset);
+	printf("data_offset: %u\n", header->data_offset);
+
+	/* Read kvm stats id string */
+	id = malloc(header->name_size);
+	TEST_ASSERT(id, "Allocate memory for id string");
+	ret = pread(stats_fd, id, header->name_size, header->id_offset);
+	TEST_ASSERT(ret == header->name_size, "Read id string");
+
+	printf("id: %s\n", id);
+
+	free(id);
+}
+
+static ssize_t stats_desc_size(struct kvm_stats_header *header)
+{
+	return sizeof(struct kvm_stats_desc) + header->name_size;
+}
+
+/* Caller is responsible for freeing the returned kvm_stats_desc. */
+static struct kvm_stats_desc *read_vm_stats_desc(int stats_fd,
+						 struct kvm_stats_header *header)
+{
+	struct kvm_stats_desc *stats_desc;
+	size_t size_desc;
+	ssize_t ret;
+
+	size_desc = header->num_desc * stats_desc_size(header);
+
+	/* Allocate memory for stats descriptors */
+	stats_desc = malloc(size_desc);
+	TEST_ASSERT(stats_desc, "Allocate memory for stats descriptors");
+
+	/* Read kvm stats descriptors */
+	ret = pread(stats_fd, stats_desc, size_desc, header->desc_offset);
+	TEST_ASSERT(ret == size_desc, "Read KVM stats descriptors");
+
+	return stats_desc;
+}
+
+/* Caller is responsible for freeing the memory *data. */
+static int read_stat_data(int stats_fd, struct kvm_stats_header *header,
+			  struct kvm_stats_desc *desc, uint64_t **data)
+{
+	u64 *stats_data;
+	ssize_t ret;
+
+	stats_data = malloc(desc->size * sizeof(*stats_data));
+
+	ret = pread(stats_fd, stats_data, desc->size * sizeof(*stats_data),
+		    header->data_offset + desc->offset);
+
+	/* ret is in bytes. */
+	ret = ret / sizeof(*stats_data);
+
+	TEST_ASSERT(ret == desc->size,
+		    "Read data of KVM stats: %s", desc->name);
+
+	*data = stats_data;
+
+	return ret;
+}
+
+static void dump_stat(int stats_fd, struct kvm_stats_header *header,
+		      struct kvm_stats_desc *desc)
+{
+	u64 *stats_data;
+	ssize_t ret;
+	int i;
+
+	printf("\tflags: %u\n", desc->flags);
+	printf("\texponent: %u\n", desc->exponent);
+	printf("\tsize: %u\n", desc->size);
+	printf("\toffset: %u\n", desc->offset);
+	printf("\tbucket_size: %u\n", desc->bucket_size);
+	printf("\tname: %s\n", (char *)&desc->name);
+
+	ret = read_stat_data(stats_fd, header, desc, &stats_data);
+
+	printf("\tdata: %lu", *stats_data);
+	for (i = 1; i < ret; i++)
+		printf(", %lu", *(stats_data + i));
+	printf("\n\n");
+
+	free(stats_data);
+}
+
+void dump_vm_stats(struct kvm_vm *vm)
+{
+	struct kvm_stats_desc *stats_desc;
+	struct kvm_stats_header *header;
+	struct kvm_stats_desc *desc;
+	size_t size_desc;
+	int stats_fd;
+	int i;
+
+	stats_fd = vm_get_stats_fd(vm);
+
+	header = read_vm_stats_header(stats_fd);
+	dump_header(stats_fd, header);
+
+	stats_desc = read_vm_stats_desc(stats_fd, header);
+
+	size_desc = stats_desc_size(header);
+
+	/* Read kvm stats data one by one */
+	for (i = 0; i < header->num_desc; ++i) {
+		desc = (void *)stats_desc + (i * size_desc);
+		dump_stat(stats_fd, header, desc);
+	}
+
+	free(stats_desc);
+	free(header);
+
+	close(stats_fd);
+}
+
-- 
2.35.1.1021.g381101b075-goog

Re: [PATCH v3 02/11] KVM: selftests: Dump VM stats in binary stats test

[Jing Zhang]

On Wed, Mar 30, 2022 at 10:46 AM Ben Gardon <bgardon@google.com> wrote:
>
> Add kvm_util library functions to read KVM stats through the binary
> stats interface and then dump them to stdout when running the binary
> stats test. Subsequent commits will extend the kvm_util code and use it
> to make assertions in a test for NX hugepages.
>
> CC: Jing Zhang <jingzhangos@google.com>
> Signed-off-by: Ben Gardon <bgardon@google.com>
> ---
>  .../selftests/kvm/include/kvm_util_base.h     |   1 +
>  .../selftests/kvm/kvm_binary_stats_test.c     |   3 +
>  tools/testing/selftests/kvm/lib/kvm_util.c    | 143 ++++++++++++++++++
>  3 files changed, 147 insertions(+)
>
> diff --git a/tools/testing/selftests/kvm/include/kvm_util_base.h b/tools/testing/selftests/kvm/include/kvm_util_base.h
> index 976aaaba8769..4783fd1cd4cf 100644
> --- a/tools/testing/selftests/kvm/include/kvm_util_base.h
> +++ b/tools/testing/selftests/kvm/include/kvm_util_base.h
> @@ -401,6 +401,7 @@ void assert_on_unhandled_exception(struct kvm_vm *vm, uint32_t vcpuid);
>
>  int vm_get_stats_fd(struct kvm_vm *vm);
>  int vcpu_get_stats_fd(struct kvm_vm *vm, uint32_t vcpuid);
> +void dump_vm_stats(struct kvm_vm *vm);
>
>  uint32_t guest_get_vcpuid(void);
>
> diff --git a/tools/testing/selftests/kvm/kvm_binary_stats_test.c b/tools/testing/selftests/kvm/kvm_binary_stats_test.c
> index 17f65d514915..afc4701ce8dd 100644
> --- a/tools/testing/selftests/kvm/kvm_binary_stats_test.c
> +++ b/tools/testing/selftests/kvm/kvm_binary_stats_test.c
> @@ -174,6 +174,9 @@ static void vm_stats_test(struct kvm_vm *vm)
>         stats_test(stats_fd);
>         close(stats_fd);
>         TEST_ASSERT(fcntl(stats_fd, F_GETFD) == -1, "Stats fd not freed");
> +
> +       /* Dump VM stats */
> +       dump_vm_stats(vm);
>  }
>
>  static void vcpu_stats_test(struct kvm_vm *vm, int vcpu_id)
> diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
> index 11a692cf4570..f87df68b150d 100644
> --- a/tools/testing/selftests/kvm/lib/kvm_util.c
> +++ b/tools/testing/selftests/kvm/lib/kvm_util.c
> @@ -2562,3 +2562,146 @@ int vcpu_get_stats_fd(struct kvm_vm *vm, uint32_t vcpuid)
>
>         return ioctl(vcpu->fd, KVM_GET_STATS_FD, NULL);
>  }
> +
> +/* Caller is responsible for freeing the returned kvm_stats_header. */
> +static struct kvm_stats_header *read_vm_stats_header(int stats_fd)
> +{
> +       struct kvm_stats_header *header;
> +       ssize_t ret;
> +
> +       /* Read kvm stats header */
> +       header = malloc(sizeof(*header));
> +       TEST_ASSERT(header, "Allocate memory for stats header");
> +
> +       ret = read(stats_fd, header, sizeof(*header));
> +       TEST_ASSERT(ret == sizeof(*header), "Read stats header");
> +
> +       return header;
> +}
> +
> +static void dump_header(int stats_fd, struct kvm_stats_header *header)
> +{
> +       ssize_t ret;
> +       char *id;
> +
> +       printf("flags: %u\n", header->flags);
> +       printf("name size: %u\n", header->name_size);
> +       printf("num_desc: %u\n", header->num_desc);
> +       printf("id_offset: %u\n", header->id_offset);
> +       printf("desc_offset: %u\n", header->desc_offset);
> +       printf("data_offset: %u\n", header->data_offset);
> +
> +       /* Read kvm stats id string */
> +       id = malloc(header->name_size);
> +       TEST_ASSERT(id, "Allocate memory for id string");
> +       ret = pread(stats_fd, id, header->name_size, header->id_offset);
> +       TEST_ASSERT(ret == header->name_size, "Read id string");
> +
> +       printf("id: %s\n", id);
> +
> +       free(id);
> +}
> +
> +static ssize_t stats_desc_size(struct kvm_stats_header *header)
> +{
> +       return sizeof(struct kvm_stats_desc) + header->name_size;
> +}
> +
> +/* Caller is responsible for freeing the returned kvm_stats_desc. */
> +static struct kvm_stats_desc *read_vm_stats_desc(int stats_fd,
> +                                                struct kvm_stats_header *header)
> +{
> +       struct kvm_stats_desc *stats_desc;
> +       size_t size_desc;
> +       ssize_t ret;
> +
> +       size_desc = header->num_desc * stats_desc_size(header);
> +
> +       /* Allocate memory for stats descriptors */
> +       stats_desc = malloc(size_desc);
> +       TEST_ASSERT(stats_desc, "Allocate memory for stats descriptors");
> +
> +       /* Read kvm stats descriptors */
> +       ret = pread(stats_fd, stats_desc, size_desc, header->desc_offset);
> +       TEST_ASSERT(ret == size_desc, "Read KVM stats descriptors");
> +
> +       return stats_desc;
> +}
> +
> +/* Caller is responsible for freeing the memory *data. */
> +static int read_stat_data(int stats_fd, struct kvm_stats_header *header,
> +                         struct kvm_stats_desc *desc, uint64_t **data)
> +{
> +       u64 *stats_data;
> +       ssize_t ret;
> +
> +       stats_data = malloc(desc->size * sizeof(*stats_data));
> +
> +       ret = pread(stats_fd, stats_data, desc->size * sizeof(*stats_data),
> +                   header->data_offset + desc->offset);
> +
> +       /* ret is in bytes. */
> +       ret = ret / sizeof(*stats_data);
> +
> +       TEST_ASSERT(ret == desc->size,
> +                   "Read data of KVM stats: %s", desc->name);
> +
> +       *data = stats_data;
> +
> +       return ret;
> +}
> +
> +static void dump_stat(int stats_fd, struct kvm_stats_header *header,
> +                     struct kvm_stats_desc *desc)
> +{
> +       u64 *stats_data;
> +       ssize_t ret;
> +       int i;
> +
> +       printf("\tflags: %u\n", desc->flags);
> +       printf("\texponent: %u\n", desc->exponent);
> +       printf("\tsize: %u\n", desc->size);
> +       printf("\toffset: %u\n", desc->offset);
> +       printf("\tbucket_size: %u\n", desc->bucket_size);
> +       printf("\tname: %s\n", (char *)&desc->name);
> +
> +       ret = read_stat_data(stats_fd, header, desc, &stats_data);
> +
> +       printf("\tdata: %lu", *stats_data);
> +       for (i = 1; i < ret; i++)
> +               printf(", %lu", *(stats_data + i));
> +       printf("\n\n");
> +
> +       free(stats_data);
> +}
> +
> +void dump_vm_stats(struct kvm_vm *vm)
> +{
> +       struct kvm_stats_desc *stats_desc;
> +       struct kvm_stats_header *header;
> +       struct kvm_stats_desc *desc;
> +       size_t size_desc;
> +       int stats_fd;
> +       int i;
> +
> +       stats_fd = vm_get_stats_fd(vm);
> +
> +       header = read_vm_stats_header(stats_fd);
> +       dump_header(stats_fd, header);
> +
> +       stats_desc = read_vm_stats_desc(stats_fd, header);
> +
> +       size_desc = stats_desc_size(header);
> +
> +       /* Read kvm stats data one by one */
> +       for (i = 0; i < header->num_desc; ++i) {
> +               desc = (void *)stats_desc + (i * size_desc);
> +               dump_stat(stats_fd, header, desc);
> +       }
> +
> +       free(stats_desc);
> +       free(header);
> +
> +       close(stats_fd);
> +}
> +
> --
> 2.35.1.1021.g381101b075-goog
>
Reviewed-by: Jing Zhang <jingzhangos@google.com>

Re: [PATCH v3 02/11] KVM: selftests: Dump VM stats in binary stats test

[David Matlack]

On Wed, Mar 30, 2022 at 10:46:12AM -0700, Ben Gardon wrote:
> Add kvm_util library functions to read KVM stats through the binary
> stats interface and then dump them to stdout when running the binary
> stats test. Subsequent commits will extend the kvm_util code and use it
> to make assertions in a test for NX hugepages.
> 
> CC: Jing Zhang <jingzhangos@google.com>
> Signed-off-by: Ben Gardon <bgardon@google.com>
> ---
>  .../selftests/kvm/include/kvm_util_base.h     |   1 +
>  .../selftests/kvm/kvm_binary_stats_test.c     |   3 +
>  tools/testing/selftests/kvm/lib/kvm_util.c    | 143 ++++++++++++++++++
>  3 files changed, 147 insertions(+)
> 
> diff --git a/tools/testing/selftests/kvm/include/kvm_util_base.h b/tools/testing/selftests/kvm/include/kvm_util_base.h
> index 976aaaba8769..4783fd1cd4cf 100644
> --- a/tools/testing/selftests/kvm/include/kvm_util_base.h
> +++ b/tools/testing/selftests/kvm/include/kvm_util_base.h
> @@ -401,6 +401,7 @@ void assert_on_unhandled_exception(struct kvm_vm *vm, uint32_t vcpuid);
>  
>  int vm_get_stats_fd(struct kvm_vm *vm);
>  int vcpu_get_stats_fd(struct kvm_vm *vm, uint32_t vcpuid);
> +void dump_vm_stats(struct kvm_vm *vm);
>  
>  uint32_t guest_get_vcpuid(void);
>  
> diff --git a/tools/testing/selftests/kvm/kvm_binary_stats_test.c b/tools/testing/selftests/kvm/kvm_binary_stats_test.c
> index 17f65d514915..afc4701ce8dd 100644
> --- a/tools/testing/selftests/kvm/kvm_binary_stats_test.c
> +++ b/tools/testing/selftests/kvm/kvm_binary_stats_test.c
> @@ -174,6 +174,9 @@ static void vm_stats_test(struct kvm_vm *vm)
>  	stats_test(stats_fd);
>  	close(stats_fd);
>  	TEST_ASSERT(fcntl(stats_fd, F_GETFD) == -1, "Stats fd not freed");
> +
> +	/* Dump VM stats */
> +	dump_vm_stats(vm);
>  }
>  
>  static void vcpu_stats_test(struct kvm_vm *vm, int vcpu_id)
> diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
> index 11a692cf4570..f87df68b150d 100644
> --- a/tools/testing/selftests/kvm/lib/kvm_util.c
> +++ b/tools/testing/selftests/kvm/lib/kvm_util.c
> @@ -2562,3 +2562,146 @@ int vcpu_get_stats_fd(struct kvm_vm *vm, uint32_t vcpuid)
>  
>  	return ioctl(vcpu->fd, KVM_GET_STATS_FD, NULL);
>  }
> +
> +/* Caller is responsible for freeing the returned kvm_stats_header. */
> +static struct kvm_stats_header *read_vm_stats_header(int stats_fd)
> +{
> +	struct kvm_stats_header *header;
> +	ssize_t ret;
> +
> +	/* Read kvm stats header */
> +	header = malloc(sizeof(*header));
> +	TEST_ASSERT(header, "Allocate memory for stats header");
> +
> +	ret = read(stats_fd, header, sizeof(*header));
> +	TEST_ASSERT(ret == sizeof(*header), "Read stats header");
> +
> +	return header;
> +}

It seems like this helper could be used in kvm_binary_stats_test.c to
eliminate duplicate code.

> +
> +static void dump_header(int stats_fd, struct kvm_stats_header *header)
> +{
> +	ssize_t ret;
> +	char *id;
> +
> +	printf("flags: %u\n", header->flags);
> +	printf("name size: %u\n", header->name_size);
> +	printf("num_desc: %u\n", header->num_desc);
> +	printf("id_offset: %u\n", header->id_offset);
> +	printf("desc_offset: %u\n", header->desc_offset);
> +	printf("data_offset: %u\n", header->data_offset);
> +
> +	/* Read kvm stats id string */
> +	id = malloc(header->name_size);
> +	TEST_ASSERT(id, "Allocate memory for id string");
> +	ret = pread(stats_fd, id, header->name_size, header->id_offset);
> +	TEST_ASSERT(ret == header->name_size, "Read id string");
> +
> +	printf("id: %s\n", id);
> +
> +	free(id);
> +}
> +
> +static ssize_t stats_desc_size(struct kvm_stats_header *header)
> +{
> +	return sizeof(struct kvm_stats_desc) + header->name_size;
> +}
> +
> +/* Caller is responsible for freeing the returned kvm_stats_desc. */
> +static struct kvm_stats_desc *read_vm_stats_desc(int stats_fd,
> +						 struct kvm_stats_header *header)
> +{
> +	struct kvm_stats_desc *stats_desc;
> +	size_t size_desc;
> +	ssize_t ret;
> +
> +	size_desc = header->num_desc * stats_desc_size(header);
> +
> +	/* Allocate memory for stats descriptors */
> +	stats_desc = malloc(size_desc);
> +	TEST_ASSERT(stats_desc, "Allocate memory for stats descriptors");
> +
> +	/* Read kvm stats descriptors */
> +	ret = pread(stats_fd, stats_desc, size_desc, header->desc_offset);
> +	TEST_ASSERT(ret == size_desc, "Read KVM stats descriptors");
> +
> +	return stats_desc;
> +}

Same with this helper.

> +
> +/* Caller is responsible for freeing the memory *data. */
> +static int read_stat_data(int stats_fd, struct kvm_stats_header *header,
> +			  struct kvm_stats_desc *desc, uint64_t **data)
> +{
> +	u64 *stats_data;
> +	ssize_t ret;
> +
> +	stats_data = malloc(desc->size * sizeof(*stats_data));
> +
> +	ret = pread(stats_fd, stats_data, desc->size * sizeof(*stats_data),
> +		    header->data_offset + desc->offset);
> +
> +	/* ret is in bytes. */
> +	ret = ret / sizeof(*stats_data);
> +
> +	TEST_ASSERT(ret == desc->size,
> +		    "Read data of KVM stats: %s", desc->name);
> +
> +	*data = stats_data;
> +
> +	return ret;
> +}

Same with this helper.

> +
> +static void dump_stat(int stats_fd, struct kvm_stats_header *header,
> +		      struct kvm_stats_desc *desc)
> +{
> +	u64 *stats_data;
> +	ssize_t ret;
> +	int i;
> +
> +	printf("\tflags: %u\n", desc->flags);
> +	printf("\texponent: %u\n", desc->exponent);
> +	printf("\tsize: %u\n", desc->size);
> +	printf("\toffset: %u\n", desc->offset);
> +	printf("\tbucket_size: %u\n", desc->bucket_size);
> +	printf("\tname: %s\n", (char *)&desc->name);
> +
> +	ret = read_stat_data(stats_fd, header, desc, &stats_data);
> +
> +	printf("\tdata: %lu", *stats_data);
> +	for (i = 1; i < ret; i++)
> +		printf(", %lu", *(stats_data + i));
> +	printf("\n\n");
> +
> +	free(stats_data);
> +}
> +
> +void dump_vm_stats(struct kvm_vm *vm)
> +{
> +	struct kvm_stats_desc *stats_desc;
> +	struct kvm_stats_header *header;
> +	struct kvm_stats_desc *desc;
> +	size_t size_desc;
> +	int stats_fd;
> +	int i;
> +
> +	stats_fd = vm_get_stats_fd(vm);
> +
> +	header = read_vm_stats_header(stats_fd);
> +	dump_header(stats_fd, header);
> +
> +	stats_desc = read_vm_stats_desc(stats_fd, header);
> +
> +	size_desc = stats_desc_size(header);
> +
> +	/* Read kvm stats data one by one */
> +	for (i = 0; i < header->num_desc; ++i) {
> +		desc = (void *)stats_desc + (i * size_desc);
> +		dump_stat(stats_fd, header, desc);
> +	}
> +
> +	free(stats_desc);
> +	free(header);
> +
> +	close(stats_fd);
> +}
> +
> -- 
> 2.35.1.1021.g381101b075-goog
> 

Re: [PATCH v3 02/11] KVM: selftests: Dump VM stats in binary stats test

[Ben Gardon]

On Tue, Apr 5, 2022 at 3:19 PM David Matlack <dmatlack@google.com> wrote:
>
> On Wed, Mar 30, 2022 at 10:46:12AM -0700, Ben Gardon wrote:
> > Add kvm_util library functions to read KVM stats through the binary
> > stats interface and then dump them to stdout when running the binary
> > stats test. Subsequent commits will extend the kvm_util code and use it
> > to make assertions in a test for NX hugepages.
> >
> > CC: Jing Zhang <jingzhangos@google.com>
> > Signed-off-by: Ben Gardon <bgardon@google.com>
> > ---
> >  .../selftests/kvm/include/kvm_util_base.h     |   1 +
> >  .../selftests/kvm/kvm_binary_stats_test.c     |   3 +
> >  tools/testing/selftests/kvm/lib/kvm_util.c    | 143 ++++++++++++++++++
> >  3 files changed, 147 insertions(+)
> >
> > diff --git a/tools/testing/selftests/kvm/include/kvm_util_base.h b/tools/testing/selftests/kvm/include/kvm_util_base.h
> > index 976aaaba8769..4783fd1cd4cf 100644
> > --- a/tools/testing/selftests/kvm/include/kvm_util_base.h
> > +++ b/tools/testing/selftests/kvm/include/kvm_util_base.h
> > @@ -401,6 +401,7 @@ void assert_on_unhandled_exception(struct kvm_vm *vm, uint32_t vcpuid);
> >
> >  int vm_get_stats_fd(struct kvm_vm *vm);
> >  int vcpu_get_stats_fd(struct kvm_vm *vm, uint32_t vcpuid);
> > +void dump_vm_stats(struct kvm_vm *vm);
> >
> >  uint32_t guest_get_vcpuid(void);
> >
> > diff --git a/tools/testing/selftests/kvm/kvm_binary_stats_test.c b/tools/testing/selftests/kvm/kvm_binary_stats_test.c
> > index 17f65d514915..afc4701ce8dd 100644
> > --- a/tools/testing/selftests/kvm/kvm_binary_stats_test.c
> > +++ b/tools/testing/selftests/kvm/kvm_binary_stats_test.c
> > @@ -174,6 +174,9 @@ static void vm_stats_test(struct kvm_vm *vm)
> >       stats_test(stats_fd);
> >       close(stats_fd);
> >       TEST_ASSERT(fcntl(stats_fd, F_GETFD) == -1, "Stats fd not freed");
> > +
> > +     /* Dump VM stats */
> > +     dump_vm_stats(vm);
> >  }
> >
> >  static void vcpu_stats_test(struct kvm_vm *vm, int vcpu_id)
> > diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
> > index 11a692cf4570..f87df68b150d 100644
> > --- a/tools/testing/selftests/kvm/lib/kvm_util.c
> > +++ b/tools/testing/selftests/kvm/lib/kvm_util.c
> > @@ -2562,3 +2562,146 @@ int vcpu_get_stats_fd(struct kvm_vm *vm, uint32_t vcpuid)
> >
> >       return ioctl(vcpu->fd, KVM_GET_STATS_FD, NULL);
> >  }
> > +
> > +/* Caller is responsible for freeing the returned kvm_stats_header. */
> > +static struct kvm_stats_header *read_vm_stats_header(int stats_fd)
> > +{
> > +     struct kvm_stats_header *header;
> > +     ssize_t ret;
> > +
> > +     /* Read kvm stats header */
> > +     header = malloc(sizeof(*header));
> > +     TEST_ASSERT(header, "Allocate memory for stats header");
> > +
> > +     ret = read(stats_fd, header, sizeof(*header));
> > +     TEST_ASSERT(ret == sizeof(*header), "Read stats header");
> > +
> > +     return header;
> > +}
>
> It seems like this helper could be used in kvm_binary_stats_test.c to
> eliminate duplicate code.

It could, but I think the duplicate code in that test has value in
being verbose and well commented and having a bunch of checks to
assert things the regular library function isn't interested in.
I'd prefer to keep the duplication as-is.

>
> > +
> > +static void dump_header(int stats_fd, struct kvm_stats_header *header)
> > +{
> > +     ssize_t ret;
> > +     char *id;
> > +
> > +     printf("flags: %u\n", header->flags);
> > +     printf("name size: %u\n", header->name_size);
> > +     printf("num_desc: %u\n", header->num_desc);
> > +     printf("id_offset: %u\n", header->id_offset);
> > +     printf("desc_offset: %u\n", header->desc_offset);
> > +     printf("data_offset: %u\n", header->data_offset);
> > +
> > +     /* Read kvm stats id string */
> > +     id = malloc(header->name_size);
> > +     TEST_ASSERT(id, "Allocate memory for id string");
> > +     ret = pread(stats_fd, id, header->name_size, header->id_offset);
> > +     TEST_ASSERT(ret == header->name_size, "Read id string");
> > +
> > +     printf("id: %s\n", id);
> > +
> > +     free(id);
> > +}
> > +
> > +static ssize_t stats_desc_size(struct kvm_stats_header *header)
> > +{
> > +     return sizeof(struct kvm_stats_desc) + header->name_size;
> > +}
> > +
> > +/* Caller is responsible for freeing the returned kvm_stats_desc. */
> > +static struct kvm_stats_desc *read_vm_stats_desc(int stats_fd,
> > +                                              struct kvm_stats_header *header)
> > +{
> > +     struct kvm_stats_desc *stats_desc;
> > +     size_t size_desc;
> > +     ssize_t ret;
> > +
> > +     size_desc = header->num_desc * stats_desc_size(header);
> > +
> > +     /* Allocate memory for stats descriptors */
> > +     stats_desc = malloc(size_desc);
> > +     TEST_ASSERT(stats_desc, "Allocate memory for stats descriptors");
> > +
> > +     /* Read kvm stats descriptors */
> > +     ret = pread(stats_fd, stats_desc, size_desc, header->desc_offset);
> > +     TEST_ASSERT(ret == size_desc, "Read KVM stats descriptors");
> > +
> > +     return stats_desc;
> > +}
>
> Same with this helper.
>
> > +
> > +/* Caller is responsible for freeing the memory *data. */
> > +static int read_stat_data(int stats_fd, struct kvm_stats_header *header,
> > +                       struct kvm_stats_desc *desc, uint64_t **data)
> > +{
> > +     u64 *stats_data;
> > +     ssize_t ret;
> > +
> > +     stats_data = malloc(desc->size * sizeof(*stats_data));
> > +
> > +     ret = pread(stats_fd, stats_data, desc->size * sizeof(*stats_data),
> > +                 header->data_offset + desc->offset);
> > +
> > +     /* ret is in bytes. */
> > +     ret = ret / sizeof(*stats_data);
> > +
> > +     TEST_ASSERT(ret == desc->size,
> > +                 "Read data of KVM stats: %s", desc->name);
> > +
> > +     *data = stats_data;
> > +
> > +     return ret;
> > +}
>
> Same with this helper.
>
> > +
> > +static void dump_stat(int stats_fd, struct kvm_stats_header *header,
> > +                   struct kvm_stats_desc *desc)
> > +{
> > +     u64 *stats_data;
> > +     ssize_t ret;
> > +     int i;
> > +
> > +     printf("\tflags: %u\n", desc->flags);
> > +     printf("\texponent: %u\n", desc->exponent);
> > +     printf("\tsize: %u\n", desc->size);
> > +     printf("\toffset: %u\n", desc->offset);
> > +     printf("\tbucket_size: %u\n", desc->bucket_size);
> > +     printf("\tname: %s\n", (char *)&desc->name);
> > +
> > +     ret = read_stat_data(stats_fd, header, desc, &stats_data);
> > +
> > +     printf("\tdata: %lu", *stats_data);
> > +     for (i = 1; i < ret; i++)
> > +             printf(", %lu", *(stats_data + i));
> > +     printf("\n\n");
> > +
> > +     free(stats_data);
> > +}
> > +
> > +void dump_vm_stats(struct kvm_vm *vm)
> > +{
> > +     struct kvm_stats_desc *stats_desc;
> > +     struct kvm_stats_header *header;
> > +     struct kvm_stats_desc *desc;
> > +     size_t size_desc;
> > +     int stats_fd;
> > +     int i;
> > +
> > +     stats_fd = vm_get_stats_fd(vm);
> > +
> > +     header = read_vm_stats_header(stats_fd);
> > +     dump_header(stats_fd, header);
> > +
> > +     stats_desc = read_vm_stats_desc(stats_fd, header);
> > +
> > +     size_desc = stats_desc_size(header);
> > +
> > +     /* Read kvm stats data one by one */
> > +     for (i = 0; i < header->num_desc; ++i) {
> > +             desc = (void *)stats_desc + (i * size_desc);
> > +             dump_stat(stats_fd, header, desc);
> > +     }
> > +
> > +     free(stats_desc);
> > +     free(header);
> > +
> > +     close(stats_fd);
> > +}
> > +
> > --
> > 2.35.1.1021.g381101b075-goog
> >

Re: [PATCH v3 02/11] KVM: selftests: Dump VM stats in binary stats test

[Sean Christopherson]

On Wed, Mar 30, 2022, Ben Gardon wrote:
> Add kvm_util library functions to read KVM stats through the binary
> stats interface and then dump them to stdout when running the binary
> stats test. Subsequent commits will extend the kvm_util code and use it
> to make assertions in a test for NX hugepages.

Why?  Spamming my console with info that has zero meaning to me and is useless
when the test passes is not helpful.  Even on failure, I don't see what the user
is going to do with this information, all of the asserts are completly unrelated
to the stats themselves.

Re: [PATCH v3 02/11] KVM: selftests: Dump VM stats in binary stats test

[Mingwei Zhang]

On Fri, Apr 8, 2022 at 12:52 PM Sean Christopherson <seanjc@google.com> wrote:
>
> On Wed, Mar 30, 2022, Ben Gardon wrote:
> > Add kvm_util library functions to read KVM stats through the binary
> > stats interface and then dump them to stdout when running the binary
> > stats test. Subsequent commits will extend the kvm_util code and use it
> > to make assertions in a test for NX hugepages.
>
> Why?  Spamming my console with info that has zero meaning to me and is useless
> when the test passes is not helpful.  Even on failure, I don't see what the user
> is going to do with this information, all of the asserts are completly unrelated
> to the stats themselves.

Debugging could be another reason, I suspect? I remember when I tried
to use the interface, there is really no API that tells me "did I add
this stat successfully and/or correctly?" I think having a general
print so that developer/debugging folk could just 'grep mystat' to
verify that would be helpful in the future.

Otherwise, they have to write code themselves to do the dirty print...

Re: [PATCH v3 02/11] KVM: selftests: Dump VM stats in binary stats test

[Sean Christopherson]

On Thu, Jun 30, 2022, Mingwei Zhang wrote:
> On Fri, Apr 8, 2022 at 12:52 PM Sean Christopherson <seanjc@google.com> wrote:
> >
> > On Wed, Mar 30, 2022, Ben Gardon wrote:
> > > Add kvm_util library functions to read KVM stats through the binary
> > > stats interface and then dump them to stdout when running the binary
> > > stats test. Subsequent commits will extend the kvm_util code and use it
> > > to make assertions in a test for NX hugepages.
> >
> > Why?  Spamming my console with info that has zero meaning to me and is useless
> > when the test passes is not helpful.  Even on failure, I don't see what the user
> > is going to do with this information, all of the asserts are completly unrelated
> > to the stats themselves.
> 
> Debugging could be another reason, I suspect? I remember when I tried
> to use the interface, there is really no API that tells me "did I add
> this stat successfully and/or correctly?" I think having a general
> print so that developer/debugging folk could just 'grep mystat' to
> verify that would be helpful in the future.
> 
> Otherwise, they have to write code themselves to do the dirty print...

I've no objection to adding a --verbose option or a #define of some form, but make
it opt-in, not on by default.

[PATCH v3 03/11] KVM: selftests: Test reading a single stat

[Ben Gardon]

Retrieve the value of a single stat by name in the binary stats test to
ensure the kvm_util library functions work.

CC: Jing Zhang <jingzhangos@google.com>
Signed-off-by: Ben Gardon <bgardon@google.com>
---
 .../selftests/kvm/include/kvm_util_base.h     |  1 +
 .../selftests/kvm/kvm_binary_stats_test.c     |  3 ++
 tools/testing/selftests/kvm/lib/kvm_util.c    | 53 +++++++++++++++++++
 3 files changed, 57 insertions(+)

diff --git a/tools/testing/selftests/kvm/include/kvm_util_base.h b/tools/testing/selftests/kvm/include/kvm_util_base.h
index 4783fd1cd4cf..78c4407f36b4 100644
--- a/tools/testing/selftests/kvm/include/kvm_util_base.h
+++ b/tools/testing/selftests/kvm/include/kvm_util_base.h
@@ -402,6 +402,7 @@ void assert_on_unhandled_exception(struct kvm_vm *vm, uint32_t vcpuid);
 int vm_get_stats_fd(struct kvm_vm *vm);
 int vcpu_get_stats_fd(struct kvm_vm *vm, uint32_t vcpuid);
 void dump_vm_stats(struct kvm_vm *vm);
+uint64_t vm_get_single_stat(struct kvm_vm *vm, const char *stat_name);
 
 uint32_t guest_get_vcpuid(void);
 
diff --git a/tools/testing/selftests/kvm/kvm_binary_stats_test.c b/tools/testing/selftests/kvm/kvm_binary_stats_test.c
index afc4701ce8dd..97bde355f105 100644
--- a/tools/testing/selftests/kvm/kvm_binary_stats_test.c
+++ b/tools/testing/selftests/kvm/kvm_binary_stats_test.c
@@ -177,6 +177,9 @@ static void vm_stats_test(struct kvm_vm *vm)
 
 	/* Dump VM stats */
 	dump_vm_stats(vm);
+
+	/* Read a single stat. */
+	printf("remote_tlb_flush: %lu\n", vm_get_single_stat(vm, "remote_tlb_flush"));
 }
 
 static void vcpu_stats_test(struct kvm_vm *vm, int vcpu_id)
diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
index f87df68b150d..9c4574381daa 100644
--- a/tools/testing/selftests/kvm/lib/kvm_util.c
+++ b/tools/testing/selftests/kvm/lib/kvm_util.c
@@ -2705,3 +2705,56 @@ void dump_vm_stats(struct kvm_vm *vm)
 	close(stats_fd);
 }
 
+static int vm_get_stat_data(struct kvm_vm *vm, const char *stat_name,
+			    uint64_t **data)
+{
+	struct kvm_stats_desc *stats_desc;
+	struct kvm_stats_header *header;
+	struct kvm_stats_desc *desc;
+	size_t size_desc;
+	int stats_fd;
+	int ret = -EINVAL;
+	int i;
+
+	*data = NULL;
+
+	stats_fd = vm_get_stats_fd(vm);
+
+	header = read_vm_stats_header(stats_fd);
+
+	stats_desc = read_vm_stats_desc(stats_fd, header);
+
+	size_desc = stats_desc_size(header);
+
+	/* Read kvm stats data one by one */
+	for (i = 0; i < header->num_desc; ++i) {
+		desc = (void *)stats_desc + (i * size_desc);
+
+		if (strcmp(desc->name, stat_name))
+			continue;
+
+		ret = read_stat_data(stats_fd, header, desc, data);
+	}
+
+	free(stats_desc);
+	free(header);
+
+	close(stats_fd);
+
+	return ret;
+}
+
+uint64_t vm_get_single_stat(struct kvm_vm *vm, const char *stat_name)
+{
+	uint64_t *data;
+	uint64_t value;
+	int ret;
+
+	ret = vm_get_stat_data(vm, stat_name, &data);
+	TEST_ASSERT(ret == 1, "Stat %s expected to have 1 element, but has %d",
+		    stat_name, ret);
+	value = *data;
+	free(data);
+	return value;
+}
+
-- 
2.35.1.1021.g381101b075-goog

Re: [PATCH v3 03/11] KVM: selftests: Test reading a single stat

[Jing Zhang]

On Wed, Mar 30, 2022 at 10:46 AM Ben Gardon <bgardon@google.com> wrote:
>
> Retrieve the value of a single stat by name in the binary stats test to
> ensure the kvm_util library functions work.
>
> CC: Jing Zhang <jingzhangos@google.com>
> Signed-off-by: Ben Gardon <bgardon@google.com>
> ---
>  .../selftests/kvm/include/kvm_util_base.h     |  1 +
>  .../selftests/kvm/kvm_binary_stats_test.c     |  3 ++
>  tools/testing/selftests/kvm/lib/kvm_util.c    | 53 +++++++++++++++++++
>  3 files changed, 57 insertions(+)
>
> diff --git a/tools/testing/selftests/kvm/include/kvm_util_base.h b/tools/testing/selftests/kvm/include/kvm_util_base.h
> index 4783fd1cd4cf..78c4407f36b4 100644
> --- a/tools/testing/selftests/kvm/include/kvm_util_base.h
> +++ b/tools/testing/selftests/kvm/include/kvm_util_base.h
> @@ -402,6 +402,7 @@ void assert_on_unhandled_exception(struct kvm_vm *vm, uint32_t vcpuid);
>  int vm_get_stats_fd(struct kvm_vm *vm);
>  int vcpu_get_stats_fd(struct kvm_vm *vm, uint32_t vcpuid);
>  void dump_vm_stats(struct kvm_vm *vm);
> +uint64_t vm_get_single_stat(struct kvm_vm *vm, const char *stat_name);
>
>  uint32_t guest_get_vcpuid(void);
>
> diff --git a/tools/testing/selftests/kvm/kvm_binary_stats_test.c b/tools/testing/selftests/kvm/kvm_binary_stats_test.c
> index afc4701ce8dd..97bde355f105 100644
> --- a/tools/testing/selftests/kvm/kvm_binary_stats_test.c
> +++ b/tools/testing/selftests/kvm/kvm_binary_stats_test.c
> @@ -177,6 +177,9 @@ static void vm_stats_test(struct kvm_vm *vm)
>
>         /* Dump VM stats */
>         dump_vm_stats(vm);
> +
> +       /* Read a single stat. */
> +       printf("remote_tlb_flush: %lu\n", vm_get_single_stat(vm, "remote_tlb_flush"));
>  }
>
>  static void vcpu_stats_test(struct kvm_vm *vm, int vcpu_id)
> diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
> index f87df68b150d..9c4574381daa 100644
> --- a/tools/testing/selftests/kvm/lib/kvm_util.c
> +++ b/tools/testing/selftests/kvm/lib/kvm_util.c
> @@ -2705,3 +2705,56 @@ void dump_vm_stats(struct kvm_vm *vm)
>         close(stats_fd);
>  }
>
> +static int vm_get_stat_data(struct kvm_vm *vm, const char *stat_name,
> +                           uint64_t **data)
> +{
> +       struct kvm_stats_desc *stats_desc;
> +       struct kvm_stats_header *header;
> +       struct kvm_stats_desc *desc;
> +       size_t size_desc;
> +       int stats_fd;
> +       int ret = -EINVAL;
> +       int i;
> +
> +       *data = NULL;
> +
> +       stats_fd = vm_get_stats_fd(vm);
> +
> +       header = read_vm_stats_header(stats_fd);
> +
> +       stats_desc = read_vm_stats_desc(stats_fd, header);
> +
> +       size_desc = stats_desc_size(header);
> +
> +       /* Read kvm stats data one by one */
> +       for (i = 0; i < header->num_desc; ++i) {
> +               desc = (void *)stats_desc + (i * size_desc);
> +
> +               if (strcmp(desc->name, stat_name))
> +                       continue;
> +
> +               ret = read_stat_data(stats_fd, header, desc, data);
> +       }
> +
> +       free(stats_desc);
> +       free(header);
> +
> +       close(stats_fd);
> +
> +       return ret;
> +}
> +
> +uint64_t vm_get_single_stat(struct kvm_vm *vm, const char *stat_name)
> +{
> +       uint64_t *data;
> +       uint64_t value;
> +       int ret;
> +
> +       ret = vm_get_stat_data(vm, stat_name, &data);
> +       TEST_ASSERT(ret == 1, "Stat %s expected to have 1 element, but has %d",
> +                   stat_name, ret);
> +       value = *data;
> +       free(data);
> +       return value;
> +}
> +
> --
> 2.35.1.1021.g381101b075-goog
>
Reviewed-by: Jing Zhang <jingzhangos@google.com>

Re: [PATCH v3 03/11] KVM: selftests: Test reading a single stat

[David Matlack]

On Wed, Mar 30, 2022 at 10:46:13AM -0700, Ben Gardon wrote:
> Retrieve the value of a single stat by name in the binary stats test to
> ensure the kvm_util library functions work.
> 
> CC: Jing Zhang <jingzhangos@google.com>
> Signed-off-by: Ben Gardon <bgardon@google.com>
> ---
>  .../selftests/kvm/include/kvm_util_base.h     |  1 +
>  .../selftests/kvm/kvm_binary_stats_test.c     |  3 ++
>  tools/testing/selftests/kvm/lib/kvm_util.c    | 53 +++++++++++++++++++
>  3 files changed, 57 insertions(+)
> 
> diff --git a/tools/testing/selftests/kvm/include/kvm_util_base.h b/tools/testing/selftests/kvm/include/kvm_util_base.h
> index 4783fd1cd4cf..78c4407f36b4 100644
> --- a/tools/testing/selftests/kvm/include/kvm_util_base.h
> +++ b/tools/testing/selftests/kvm/include/kvm_util_base.h
> @@ -402,6 +402,7 @@ void assert_on_unhandled_exception(struct kvm_vm *vm, uint32_t vcpuid);
>  int vm_get_stats_fd(struct kvm_vm *vm);
>  int vcpu_get_stats_fd(struct kvm_vm *vm, uint32_t vcpuid);
>  void dump_vm_stats(struct kvm_vm *vm);
> +uint64_t vm_get_single_stat(struct kvm_vm *vm, const char *stat_name);
>  
>  uint32_t guest_get_vcpuid(void);
>  
> diff --git a/tools/testing/selftests/kvm/kvm_binary_stats_test.c b/tools/testing/selftests/kvm/kvm_binary_stats_test.c
> index afc4701ce8dd..97bde355f105 100644
> --- a/tools/testing/selftests/kvm/kvm_binary_stats_test.c
> +++ b/tools/testing/selftests/kvm/kvm_binary_stats_test.c
> @@ -177,6 +177,9 @@ static void vm_stats_test(struct kvm_vm *vm)
>  
>  	/* Dump VM stats */
>  	dump_vm_stats(vm);
> +
> +	/* Read a single stat. */
> +	printf("remote_tlb_flush: %lu\n", vm_get_single_stat(vm, "remote_tlb_flush"));
>  }
>  
>  static void vcpu_stats_test(struct kvm_vm *vm, int vcpu_id)
> diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
> index f87df68b150d..9c4574381daa 100644
> --- a/tools/testing/selftests/kvm/lib/kvm_util.c
> +++ b/tools/testing/selftests/kvm/lib/kvm_util.c
> @@ -2705,3 +2705,56 @@ void dump_vm_stats(struct kvm_vm *vm)
>  	close(stats_fd);
>  }
>  
> +static int vm_get_stat_data(struct kvm_vm *vm, const char *stat_name,
> +			    uint64_t **data)
> +{
> +	struct kvm_stats_desc *stats_desc;
> +	struct kvm_stats_header *header;
> +	struct kvm_stats_desc *desc;
> +	size_t size_desc;
> +	int stats_fd;
> +	int ret = -EINVAL;
> +	int i;
> +
> +	*data = NULL;
> +
> +	stats_fd = vm_get_stats_fd(vm);
> +
> +	header = read_vm_stats_header(stats_fd);
> +
> +	stats_desc = read_vm_stats_desc(stats_fd, header);
> +
> +	size_desc = stats_desc_size(header);
> +
> +	/* Read kvm stats data one by one */
> +	for (i = 0; i < header->num_desc; ++i) {
> +		desc = (void *)stats_desc + (i * size_desc);
> +
> +		if (strcmp(desc->name, stat_name))
> +			continue;
> +
> +		ret = read_stat_data(stats_fd, header, desc, data);
> +	}
> +
> +	free(stats_desc);
> +	free(header);
> +
> +	close(stats_fd);
> +
> +	return ret;
> +}
> +
> +uint64_t vm_get_single_stat(struct kvm_vm *vm, const char *stat_name)
> +{
> +	uint64_t *data;
> +	uint64_t value;
> +	int ret;
> +
> +	ret = vm_get_stat_data(vm, stat_name, &data);
> +	TEST_ASSERT(ret == 1, "Stat %s expected to have 1 element, but has %d",
> +		    stat_name, ret);
> +	value = *data;
> +	free(data);

Allocating temporary storage for the data is unnecessary. Just read the
stat directly into &value. You'll need to change read_stat_data() to
accept another parameter that defines the number of elements the caller
wants to read. Otherwise botched stats could trigger a buffer overflow.

> +	return value;
> +}
> +
> -- 
> 2.35.1.1021.g381101b075-goog
> 

Re: [PATCH v3 03/11] KVM: selftests: Test reading a single stat

[Ben Gardon]

On Tue, Apr 5, 2022 at 3:24 PM David Matlack <dmatlack@google.com> wrote:
>
> On Wed, Mar 30, 2022 at 10:46:13AM -0700, Ben Gardon wrote:
> > Retrieve the value of a single stat by name in the binary stats test to
> > ensure the kvm_util library functions work.
> >
> > CC: Jing Zhang <jingzhangos@google.com>
> > Signed-off-by: Ben Gardon <bgardon@google.com>
> > ---
> >  .../selftests/kvm/include/kvm_util_base.h     |  1 +
> >  .../selftests/kvm/kvm_binary_stats_test.c     |  3 ++
> >  tools/testing/selftests/kvm/lib/kvm_util.c    | 53 +++++++++++++++++++
> >  3 files changed, 57 insertions(+)
> >
> > diff --git a/tools/testing/selftests/kvm/include/kvm_util_base.h b/tools/testing/selftests/kvm/include/kvm_util_base.h
> > index 4783fd1cd4cf..78c4407f36b4 100644
> > --- a/tools/testing/selftests/kvm/include/kvm_util_base.h
> > +++ b/tools/testing/selftests/kvm/include/kvm_util_base.h
> > @@ -402,6 +402,7 @@ void assert_on_unhandled_exception(struct kvm_vm *vm, uint32_t vcpuid);
> >  int vm_get_stats_fd(struct kvm_vm *vm);
> >  int vcpu_get_stats_fd(struct kvm_vm *vm, uint32_t vcpuid);
> >  void dump_vm_stats(struct kvm_vm *vm);
> > +uint64_t vm_get_single_stat(struct kvm_vm *vm, const char *stat_name);
> >
> >  uint32_t guest_get_vcpuid(void);
> >
> > diff --git a/tools/testing/selftests/kvm/kvm_binary_stats_test.c b/tools/testing/selftests/kvm/kvm_binary_stats_test.c
> > index afc4701ce8dd..97bde355f105 100644
> > --- a/tools/testing/selftests/kvm/kvm_binary_stats_test.c
> > +++ b/tools/testing/selftests/kvm/kvm_binary_stats_test.c
> > @@ -177,6 +177,9 @@ static void vm_stats_test(struct kvm_vm *vm)
> >
> >       /* Dump VM stats */
> >       dump_vm_stats(vm);
> > +
> > +     /* Read a single stat. */
> > +     printf("remote_tlb_flush: %lu\n", vm_get_single_stat(vm, "remote_tlb_flush"));
> >  }
> >
> >  static void vcpu_stats_test(struct kvm_vm *vm, int vcpu_id)
> > diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
> > index f87df68b150d..9c4574381daa 100644
> > --- a/tools/testing/selftests/kvm/lib/kvm_util.c
> > +++ b/tools/testing/selftests/kvm/lib/kvm_util.c
> > @@ -2705,3 +2705,56 @@ void dump_vm_stats(struct kvm_vm *vm)
> >       close(stats_fd);
> >  }
> >
> > +static int vm_get_stat_data(struct kvm_vm *vm, const char *stat_name,
> > +                         uint64_t **data)
> > +{
> > +     struct kvm_stats_desc *stats_desc;
> > +     struct kvm_stats_header *header;
> > +     struct kvm_stats_desc *desc;
> > +     size_t size_desc;
> > +     int stats_fd;
> > +     int ret = -EINVAL;
> > +     int i;
> > +
> > +     *data = NULL;
> > +
> > +     stats_fd = vm_get_stats_fd(vm);
> > +
> > +     header = read_vm_stats_header(stats_fd);
> > +
> > +     stats_desc = read_vm_stats_desc(stats_fd, header);
> > +
> > +     size_desc = stats_desc_size(header);
> > +
> > +     /* Read kvm stats data one by one */
> > +     for (i = 0; i < header->num_desc; ++i) {
> > +             desc = (void *)stats_desc + (i * size_desc);
> > +
> > +             if (strcmp(desc->name, stat_name))
> > +                     continue;
> > +
> > +             ret = read_stat_data(stats_fd, header, desc, data);
> > +     }
> > +
> > +     free(stats_desc);
> > +     free(header);
> > +
> > +     close(stats_fd);
> > +
> > +     return ret;
> > +}
> > +
> > +uint64_t vm_get_single_stat(struct kvm_vm *vm, const char *stat_name)
> > +{
> > +     uint64_t *data;
> > +     uint64_t value;
> > +     int ret;
> > +
> > +     ret = vm_get_stat_data(vm, stat_name, &data);
> > +     TEST_ASSERT(ret == 1, "Stat %s expected to have 1 element, but has %d",
> > +                 stat_name, ret);
> > +     value = *data;
> > +     free(data);
>
> Allocating temporary storage for the data is unnecessary. Just read the
> stat directly into &value. You'll need to change read_stat_data() to
> accept another parameter that defines the number of elements the caller
> wants to read. Otherwise botched stats could trigger a buffer overflow.

Will do.

>
> > +     return value;
> > +}
> > +
> > --
> > 2.35.1.1021.g381101b075-goog
> >

[PATCH v3 04/11] KVM: selftests: Add memslot parameter to elf_load

[Ben Gardon]

Currently elf_load loads code into memslot 0. Add a parameter to allow
loading code into any memslot. This will be useful for backing code
pages with huge pages in future commits.

No functional change intended.

Signed-off-by: Ben Gardon <bgardon@google.com>
---
 .../testing/selftests/kvm/include/kvm_util_base.h  |  5 +++++
 tools/testing/selftests/kvm/lib/elf.c              | 13 +++++++++++--
 tools/testing/selftests/kvm/lib/kvm_util.c         | 14 ++++++++++----
 3 files changed, 26 insertions(+), 6 deletions(-)

diff --git a/tools/testing/selftests/kvm/include/kvm_util_base.h b/tools/testing/selftests/kvm/include/kvm_util_base.h
index 78c4407f36b4..72163ba2f878 100644
--- a/tools/testing/selftests/kvm/include/kvm_util_base.h
+++ b/tools/testing/selftests/kvm/include/kvm_util_base.h
@@ -122,7 +122,10 @@ uint32_t kvm_vm_reset_dirty_ring(struct kvm_vm *vm);
 int kvm_memcmp_hva_gva(void *hva, struct kvm_vm *vm, const vm_vaddr_t gva,
 		       size_t len);
 
+void kvm_vm_elf_load_memslot(struct kvm_vm *vm, const char *filename,
+			     uint32_t memslot);
 void kvm_vm_elf_load(struct kvm_vm *vm, const char *filename);
+
 int kvm_memfd_alloc(size_t size, bool hugepages);
 
 void vm_dump(FILE *stream, struct kvm_vm *vm, uint8_t indent);
@@ -169,6 +172,8 @@ void vm_mem_region_set_flags(struct kvm_vm *vm, uint32_t slot, uint32_t flags);
 void vm_mem_region_move(struct kvm_vm *vm, uint32_t slot, uint64_t new_gpa);
 void vm_mem_region_delete(struct kvm_vm *vm, uint32_t slot);
 void vm_vcpu_add(struct kvm_vm *vm, uint32_t vcpuid);
+vm_vaddr_t vm_vaddr_alloc_memslot(struct kvm_vm *vm, size_t sz,
+				  vm_vaddr_t vaddr_min, uint32_t memslot);
 vm_vaddr_t vm_vaddr_alloc(struct kvm_vm *vm, size_t sz, vm_vaddr_t vaddr_min);
 vm_vaddr_t vm_vaddr_alloc_pages(struct kvm_vm *vm, int nr_pages);
 vm_vaddr_t vm_vaddr_alloc_page(struct kvm_vm *vm);
diff --git a/tools/testing/selftests/kvm/lib/elf.c b/tools/testing/selftests/kvm/lib/elf.c
index 13e8e3dcf984..899418e65f60 100644
--- a/tools/testing/selftests/kvm/lib/elf.c
+++ b/tools/testing/selftests/kvm/lib/elf.c
@@ -97,6 +97,7 @@ static void elfhdr_get(const char *filename, Elf64_Ehdr *hdrp)
  *
  * Input Args:
  *   filename - Path to ELF file
+ *   memslot - the memslot into which the elf should be loaded
  *
  * Output Args: None
  *
@@ -111,7 +112,8 @@ static void elfhdr_get(const char *filename, Elf64_Ehdr *hdrp)
  * by the image and it needs to have sufficient available physical pages, to
  * back the virtual pages used to load the image.
  */
-void kvm_vm_elf_load(struct kvm_vm *vm, const char *filename)
+void kvm_vm_elf_load_memslot(struct kvm_vm *vm, const char *filename,
+			     uint32_t memslot)
 {
 	off_t offset, offset_rv;
 	Elf64_Ehdr hdr;
@@ -162,7 +164,9 @@ void kvm_vm_elf_load(struct kvm_vm *vm, const char *filename)
 		seg_vend |= vm->page_size - 1;
 		size_t seg_size = seg_vend - seg_vstart + 1;
 
-		vm_vaddr_t vaddr = vm_vaddr_alloc(vm, seg_size, seg_vstart);
+		vm_vaddr_t vaddr = vm_vaddr_alloc_memslot(vm, seg_size,
+							  seg_vstart,
+							  memslot);
 		TEST_ASSERT(vaddr == seg_vstart, "Unable to allocate "
 			"virtual memory for segment at requested min addr,\n"
 			"  segment idx: %u\n"
@@ -191,3 +195,8 @@ void kvm_vm_elf_load(struct kvm_vm *vm, const char *filename)
 		}
 	}
 }
+
+void kvm_vm_elf_load(struct kvm_vm *vm, const char *filename)
+{
+	kvm_vm_elf_load_memslot(vm, filename, 0);
+}
diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
index 9c4574381daa..09742a787546 100644
--- a/tools/testing/selftests/kvm/lib/kvm_util.c
+++ b/tools/testing/selftests/kvm/lib/kvm_util.c
@@ -1336,8 +1336,7 @@ static vm_vaddr_t vm_vaddr_unused_gap(struct kvm_vm *vm, size_t sz,
  *   vm - Virtual Machine
  *   sz - Size in bytes
  *   vaddr_min - Minimum starting virtual address
- *   data_memslot - Memory region slot for data pages
- *   pgd_memslot - Memory region slot for new virtual translation tables
+ *   memslot - Memory region slot for data pages
  *
  * Output Args: None
  *
@@ -1350,13 +1349,15 @@ static vm_vaddr_t vm_vaddr_unused_gap(struct kvm_vm *vm, size_t sz,
  * a unique set of pages, with the minimum real allocation being at least
  * a page.
  */
-vm_vaddr_t vm_vaddr_alloc(struct kvm_vm *vm, size_t sz, vm_vaddr_t vaddr_min)
+vm_vaddr_t vm_vaddr_alloc_memslot(struct kvm_vm *vm, size_t sz,
+				  vm_vaddr_t vaddr_min, uint32_t memslot)
 {
 	uint64_t pages = (sz >> vm->page_shift) + ((sz % vm->page_size) != 0);
 
 	virt_pgd_alloc(vm);
 	vm_paddr_t paddr = vm_phy_pages_alloc(vm, pages,
-					      KVM_UTIL_MIN_PFN * vm->page_size, 0);
+					      KVM_UTIL_MIN_PFN * vm->page_size,
+					      memslot);
 
 	/*
 	 * Find an unused range of virtual page addresses of at least
@@ -1377,6 +1378,11 @@ vm_vaddr_t vm_vaddr_alloc(struct kvm_vm *vm, size_t sz, vm_vaddr_t vaddr_min)
 	return vaddr_start;
 }
 
+vm_vaddr_t vm_vaddr_alloc(struct kvm_vm *vm, size_t sz, vm_vaddr_t vaddr_min)
+{
+	return vm_vaddr_alloc_memslot(vm, sz, vaddr_min, 0);
+}
+
 /*
  * VM Virtual Address Allocate Pages
  *
-- 
2.35.1.1021.g381101b075-goog

Re: [PATCH v3 04/11] KVM: selftests: Add memslot parameter to elf_load

[David Matlack]

On Wed, Mar 30, 2022 at 10:46:14AM -0700, Ben Gardon wrote:
> Currently elf_load loads code into memslot 0. Add a parameter to allow
> loading code into any memslot. This will be useful for backing code
> pages with huge pages in future commits.
> 
> No functional change intended.
> 
> Signed-off-by: Ben Gardon <bgardon@google.com>
> ---
>  .../testing/selftests/kvm/include/kvm_util_base.h  |  5 +++++
>  tools/testing/selftests/kvm/lib/elf.c              | 13 +++++++++++--
>  tools/testing/selftests/kvm/lib/kvm_util.c         | 14 ++++++++++----
>  3 files changed, 26 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/include/kvm_util_base.h b/tools/testing/selftests/kvm/include/kvm_util_base.h
> index 78c4407f36b4..72163ba2f878 100644
> --- a/tools/testing/selftests/kvm/include/kvm_util_base.h
> +++ b/tools/testing/selftests/kvm/include/kvm_util_base.h
> @@ -122,7 +122,10 @@ uint32_t kvm_vm_reset_dirty_ring(struct kvm_vm *vm);
>  int kvm_memcmp_hva_gva(void *hva, struct kvm_vm *vm, const vm_vaddr_t gva,
>  		       size_t len);
>  
> +void kvm_vm_elf_load_memslot(struct kvm_vm *vm, const char *filename,
> +			     uint32_t memslot);
>  void kvm_vm_elf_load(struct kvm_vm *vm, const char *filename);
> +
>  int kvm_memfd_alloc(size_t size, bool hugepages);
>  
>  void vm_dump(FILE *stream, struct kvm_vm *vm, uint8_t indent);
> @@ -169,6 +172,8 @@ void vm_mem_region_set_flags(struct kvm_vm *vm, uint32_t slot, uint32_t flags);
>  void vm_mem_region_move(struct kvm_vm *vm, uint32_t slot, uint64_t new_gpa);
>  void vm_mem_region_delete(struct kvm_vm *vm, uint32_t slot);
>  void vm_vcpu_add(struct kvm_vm *vm, uint32_t vcpuid);
> +vm_vaddr_t vm_vaddr_alloc_memslot(struct kvm_vm *vm, size_t sz,
> +				  vm_vaddr_t vaddr_min, uint32_t memslot);
>  vm_vaddr_t vm_vaddr_alloc(struct kvm_vm *vm, size_t sz, vm_vaddr_t vaddr_min);
>  vm_vaddr_t vm_vaddr_alloc_pages(struct kvm_vm *vm, int nr_pages);
>  vm_vaddr_t vm_vaddr_alloc_page(struct kvm_vm *vm);
> diff --git a/tools/testing/selftests/kvm/lib/elf.c b/tools/testing/selftests/kvm/lib/elf.c
> index 13e8e3dcf984..899418e65f60 100644
> --- a/tools/testing/selftests/kvm/lib/elf.c
> +++ b/tools/testing/selftests/kvm/lib/elf.c
> @@ -97,6 +97,7 @@ static void elfhdr_get(const char *filename, Elf64_Ehdr *hdrp)
>   *
>   * Input Args:
>   *   filename - Path to ELF file
> + *   memslot - the memslot into which the elf should be loaded
>   *
>   * Output Args: None
>   *
> @@ -111,7 +112,8 @@ static void elfhdr_get(const char *filename, Elf64_Ehdr *hdrp)
>   * by the image and it needs to have sufficient available physical pages, to
>   * back the virtual pages used to load the image.
>   */
> -void kvm_vm_elf_load(struct kvm_vm *vm, const char *filename)
> +void kvm_vm_elf_load_memslot(struct kvm_vm *vm, const char *filename,
> +			     uint32_t memslot)

Feedback I've gotten in the past for kernel code and selftests is to
just use double-underscores (i.e. __kvm_vm_elf_load()) for situations
like this, rather than trying to encode the extra parameters in the
function name.

>  {
>  	off_t offset, offset_rv;
>  	Elf64_Ehdr hdr;
> @@ -162,7 +164,9 @@ void kvm_vm_elf_load(struct kvm_vm *vm, const char *filename)
>  		seg_vend |= vm->page_size - 1;
>  		size_t seg_size = seg_vend - seg_vstart + 1;
>  
> -		vm_vaddr_t vaddr = vm_vaddr_alloc(vm, seg_size, seg_vstart);
> +		vm_vaddr_t vaddr = vm_vaddr_alloc_memslot(vm, seg_size,
> +							  seg_vstart,
> +							  memslot);
>  		TEST_ASSERT(vaddr == seg_vstart, "Unable to allocate "
>  			"virtual memory for segment at requested min addr,\n"
>  			"  segment idx: %u\n"
> @@ -191,3 +195,8 @@ void kvm_vm_elf_load(struct kvm_vm *vm, const char *filename)
>  		}
>  	}
>  }
> +
> +void kvm_vm_elf_load(struct kvm_vm *vm, const char *filename)
> +{
> +	kvm_vm_elf_load_memslot(vm, filename, 0);
> +}
> diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
> index 9c4574381daa..09742a787546 100644
> --- a/tools/testing/selftests/kvm/lib/kvm_util.c
> +++ b/tools/testing/selftests/kvm/lib/kvm_util.c
> @@ -1336,8 +1336,7 @@ static vm_vaddr_t vm_vaddr_unused_gap(struct kvm_vm *vm, size_t sz,
>   *   vm - Virtual Machine
>   *   sz - Size in bytes
>   *   vaddr_min - Minimum starting virtual address
> - *   data_memslot - Memory region slot for data pages
> - *   pgd_memslot - Memory region slot for new virtual translation tables
> + *   memslot - Memory region slot for data pages
>   *
>   * Output Args: None
>   *
> @@ -1350,13 +1349,15 @@ static vm_vaddr_t vm_vaddr_unused_gap(struct kvm_vm *vm, size_t sz,
>   * a unique set of pages, with the minimum real allocation being at least
>   * a page.
>   */
> -vm_vaddr_t vm_vaddr_alloc(struct kvm_vm *vm, size_t sz, vm_vaddr_t vaddr_min)
> +vm_vaddr_t vm_vaddr_alloc_memslot(struct kvm_vm *vm, size_t sz,
> +				  vm_vaddr_t vaddr_min, uint32_t memslot)

Same feedback here; use __vm_vaddr_alloc().

>  {
>  	uint64_t pages = (sz >> vm->page_shift) + ((sz % vm->page_size) != 0);
>  
>  	virt_pgd_alloc(vm);
>  	vm_paddr_t paddr = vm_phy_pages_alloc(vm, pages,
> -					      KVM_UTIL_MIN_PFN * vm->page_size, 0);
> +					      KVM_UTIL_MIN_PFN * vm->page_size,
> +					      memslot);
>  
>  	/*
>  	 * Find an unused range of virtual page addresses of at least
> @@ -1377,6 +1378,11 @@ vm_vaddr_t vm_vaddr_alloc(struct kvm_vm *vm, size_t sz, vm_vaddr_t vaddr_min)
>  	return vaddr_start;
>  }
>  
> +vm_vaddr_t vm_vaddr_alloc(struct kvm_vm *vm, size_t sz, vm_vaddr_t vaddr_min)
> +{
> +	return vm_vaddr_alloc_memslot(vm, sz, vaddr_min, 0);
> +}
> +
>  /*
>   * VM Virtual Address Allocate Pages
>   *
> -- 
> 2.35.1.1021.g381101b075-goog
> 

[PATCH v3 05/11] KVM: selftests: Improve error message in vm_phy_pages_alloc

[Ben Gardon]

Make an error message in vm_phy_pages_alloc more specific, and log the
number of pages requested in the allocation.

Signed-off-by: Ben Gardon <bgardon@google.com>
---
 tools/testing/selftests/kvm/lib/kvm_util.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
index 09742a787546..9d72d1bb34fa 100644
--- a/tools/testing/selftests/kvm/lib/kvm_util.c
+++ b/tools/testing/selftests/kvm/lib/kvm_util.c
@@ -2408,9 +2408,10 @@ vm_paddr_t vm_phy_pages_alloc(struct kvm_vm *vm, size_t num,
 	} while (pg && pg != base + num);
 
 	if (pg == 0) {
-		fprintf(stderr, "No guest physical page available, "
+		fprintf(stderr,
+			"Unable to find %ld contiguous guest physical pages. "
 			"paddr_min: 0x%lx page_size: 0x%x memslot: %u\n",
-			paddr_min, vm->page_size, memslot);
+			num, paddr_min, vm->page_size, memslot);
 		fputs("---- vm dump ----\n", stderr);
 		vm_dump(stderr, vm, 2);
 		abort();
-- 
2.35.1.1021.g381101b075-goog

Re: [PATCH v3 05/11] KVM: selftests: Improve error message in vm_phy_pages_alloc

[David Matlack]

On Wed, Mar 30, 2022 at 10:46:15AM -0700, Ben Gardon wrote:
> Make an error message in vm_phy_pages_alloc more specific, and log the

vm_phy_pages_alloc()

> number of pages requested in the allocation.
> 
> Signed-off-by: Ben Gardon <bgardon@google.com>
> ---
>  tools/testing/selftests/kvm/lib/kvm_util.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
> index 09742a787546..9d72d1bb34fa 100644
> --- a/tools/testing/selftests/kvm/lib/kvm_util.c
> +++ b/tools/testing/selftests/kvm/lib/kvm_util.c
> @@ -2408,9 +2408,10 @@ vm_paddr_t vm_phy_pages_alloc(struct kvm_vm *vm, size_t num,
>  	} while (pg && pg != base + num);
>  
>  	if (pg == 0) {
> -		fprintf(stderr, "No guest physical page available, "
> +		fprintf(stderr,
> +			"Unable to find %ld contiguous guest physical pages. "
>  			"paddr_min: 0x%lx page_size: 0x%x memslot: %u\n",
> -			paddr_min, vm->page_size, memslot);
> +			num, paddr_min, vm->page_size, memslot);
>  		fputs("---- vm dump ----\n", stderr);
>  		vm_dump(stderr, vm, 2);
>  		abort();
> -- 
> 2.35.1.1021.g381101b075-goog
> 

[PATCH v3 06/11] KVM: selftests: Add NX huge pages test

[Ben Gardon]

There's currently no test coverage of NX hugepages in KVM selftests, so
add a basic test to ensure that the feature works as intended.

Reviewed-by: David Dunn <daviddunn@google.com>

Signed-off-by: Ben Gardon <bgardon@google.com>
---
 tools/testing/selftests/kvm/Makefile          |   7 +-
 .../kvm/lib/x86_64/nx_huge_pages_guest.S      |  45 ++++++
 .../selftests/kvm/x86_64/nx_huge_pages_test.c | 133 ++++++++++++++++++
 .../kvm/x86_64/nx_huge_pages_test.sh          |  25 ++++
 4 files changed, 209 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/kvm/lib/x86_64/nx_huge_pages_guest.S
 create mode 100644 tools/testing/selftests/kvm/x86_64/nx_huge_pages_test.c
 create mode 100755 tools/testing/selftests/kvm/x86_64/nx_huge_pages_test.sh

diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
index c9cdbd248727..c671224cf755 100644
--- a/tools/testing/selftests/kvm/Makefile
+++ b/tools/testing/selftests/kvm/Makefile
@@ -38,7 +38,7 @@ ifeq ($(ARCH),riscv)
 endif
 
 LIBKVM = lib/assert.c lib/elf.c lib/io.c lib/kvm_util.c lib/rbtree.c lib/sparsebit.c lib/test_util.c lib/guest_modes.c lib/perf_test_util.c
-LIBKVM_x86_64 = lib/x86_64/apic.c lib/x86_64/processor.c lib/x86_64/vmx.c lib/x86_64/svm.c lib/x86_64/ucall.c lib/x86_64/handlers.S
+LIBKVM_x86_64 = lib/x86_64/apic.c lib/x86_64/processor.c lib/x86_64/vmx.c lib/x86_64/svm.c lib/x86_64/ucall.c lib/x86_64/handlers.S lib/x86_64/nx_huge_pages_guest.S
 LIBKVM_aarch64 = lib/aarch64/processor.c lib/aarch64/ucall.c lib/aarch64/handlers.S lib/aarch64/spinlock.c lib/aarch64/gic.c lib/aarch64/gic_v3.c lib/aarch64/vgic.c
 LIBKVM_s390x = lib/s390x/processor.c lib/s390x/ucall.c lib/s390x/diag318_test_handler.c
 LIBKVM_riscv = lib/riscv/processor.c lib/riscv/ucall.c
@@ -57,6 +57,8 @@ TEST_GEN_PROGS_x86_64 += x86_64/kvm_clock_test
 TEST_GEN_PROGS_x86_64 += x86_64/kvm_pv_test
 TEST_GEN_PROGS_x86_64 += x86_64/mmio_warning_test
 TEST_GEN_PROGS_x86_64 += x86_64/mmu_role_test
+TEST_GEN_PROGS_EXTENDED_x86_64 += x86_64/nx_huge_pages_test
+TEST_PROGS_x86_64 += x86_64/nx_huge_pages_test.sh
 TEST_GEN_PROGS_x86_64 += x86_64/platform_info_test
 TEST_GEN_PROGS_x86_64 += x86_64/pmu_event_filter_test
 TEST_GEN_PROGS_x86_64 += x86_64/set_boot_cpu_id
@@ -141,7 +143,9 @@ TEST_GEN_PROGS_riscv += kvm_page_table_test
 TEST_GEN_PROGS_riscv += set_memory_region_test
 TEST_GEN_PROGS_riscv += kvm_binary_stats_test
 
+TEST_PROGS += $(TEST_PROGS_$(UNAME_M))
 TEST_GEN_PROGS += $(TEST_GEN_PROGS_$(UNAME_M))
+TEST_GEN_PROGS_EXTENDED += $(TEST_GEN_PROGS_EXTENDED_$(UNAME_M))
 LIBKVM += $(LIBKVM_$(UNAME_M))
 
 INSTALL_HDR_PATH = $(top_srcdir)/usr
@@ -192,6 +196,7 @@ $(OUTPUT)/libkvm.a: $(LIBKVM_OBJS)
 x := $(shell mkdir -p $(sort $(dir $(TEST_GEN_PROGS))))
 all: $(STATIC_LIBS)
 $(TEST_GEN_PROGS): $(STATIC_LIBS)
+$(TEST_GEN_PROGS_EXTENDED): $(STATIC_LIBS)
 
 cscope: include_paths = $(LINUX_TOOL_INCLUDE) $(LINUX_HDR_PATH) include lib ..
 cscope:
diff --git a/tools/testing/selftests/kvm/lib/x86_64/nx_huge_pages_guest.S b/tools/testing/selftests/kvm/lib/x86_64/nx_huge_pages_guest.S
new file mode 100644
index 000000000000..09c66b9562a3
--- /dev/null
+++ b/tools/testing/selftests/kvm/lib/x86_64/nx_huge_pages_guest.S
@@ -0,0 +1,45 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * tools/testing/selftests/kvm/nx_huge_page_guest.S
+ *
+ * Copyright (C) 2022, Google LLC.
+ */
+
+.include "kvm_util.h"
+
+#define HPAGE_SIZE 	(2*1024*1024)
+#define PORT_SUCCESS	0x70
+
+.global guest_code0
+.global guest_code1
+
+.align HPAGE_SIZE
+exit_vm:
+	mov    $0x1,%edi
+	mov    $0x2,%esi
+	mov    a_string,%edx
+	mov    $0x1,%ecx
+	xor    %eax,%eax
+	jmp    ucall
+
+
+guest_code0:
+	mov data1, %eax
+	mov data2, %eax
+	jmp exit_vm
+
+.align HPAGE_SIZE
+guest_code1:
+	mov data1, %eax
+	mov data2, %eax
+	jmp exit_vm
+data1:
+.quad	0
+
+.align HPAGE_SIZE
+data2:
+.quad	0
+a_string:
+.string "why does the ucall function take a string argument?"
+
+
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
new file mode 100644
index 000000000000..2bcbe4efdc6a
--- /dev/null
+++ b/tools/testing/selftests/kvm/x86_64/nx_huge_pages_test.c
@@ -0,0 +1,133 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * tools/testing/selftests/kvm/nx_huge_page_test.c
+ *
+ * Usage: to be run via nx_huge_page_test.sh, which does the necessary
+ * environment setup and teardown
+ *
+ * Copyright (C) 2022, Google LLC.
+ */
+
+#define _GNU_SOURCE
+
+#include <fcntl.h>
+#include <stdint.h>
+#include <time.h>
+
+#include <test_util.h>
+#include "kvm_util.h"
+
+#define HPAGE_SLOT		10
+#define HPAGE_PADDR_START       (10*1024*1024)
+#define HPAGE_SLOT_NPAGES	(100*1024*1024/4096)
+
+/* Defined in nx_huge_page_guest.S */
+void guest_code0(void);
+void guest_code1(void);
+
+static void run_guest_code(struct kvm_vm *vm, void (*guest_code)(void))
+{
+	struct kvm_regs regs;
+
+	vcpu_regs_get(vm, 0, &regs);
+	regs.rip = (uint64_t)guest_code;
+	vcpu_regs_set(vm, 0, &regs);
+	vcpu_run(vm, 0);
+}
+
+static void check_2m_page_count(struct kvm_vm *vm, int expected_pages_2m)
+{
+	int actual_pages_2m;
+
+	actual_pages_2m = vm_get_single_stat(vm, "pages_2m");
+
+	TEST_ASSERT(actual_pages_2m == expected_pages_2m,
+		    "Unexpected 2m page count. Expected %d, got %d",
+		    expected_pages_2m, actual_pages_2m);
+}
+
+static void check_split_count(struct kvm_vm *vm, int expected_splits)
+{
+	int actual_splits;
+
+	actual_splits = vm_get_single_stat(vm, "nx_lpage_splits");
+
+	TEST_ASSERT(actual_splits == expected_splits,
+		    "Unexpected nx lpage split count. Expected %d, got %d",
+		    expected_splits, actual_splits);
+}
+
+int main(int argc, char **argv)
+{
+	struct kvm_vm *vm;
+	struct timespec ts;
+
+	vm = vm_create(VM_MODE_DEFAULT, DEFAULT_GUEST_PHY_PAGES, O_RDWR);
+
+	vm_userspace_mem_region_add(vm, VM_MEM_SRC_ANONYMOUS_HUGETLB,
+				    HPAGE_PADDR_START, HPAGE_SLOT,
+				    HPAGE_SLOT_NPAGES, 0);
+
+	kvm_vm_elf_load_memslot(vm, program_invocation_name, HPAGE_SLOT);
+
+	vm_vcpu_add_default(vm, 0, guest_code0);
+
+	check_2m_page_count(vm, 0);
+	check_split_count(vm, 0);
+
+	/*
+	 * Running guest_code0 will access data1 and data2.
+	 * This should result in part of the huge page containing guest_code0,
+	 * and part of the hugepage containing the ucall function being mapped
+	 * at 4K. The huge pages containing data1 and data2 will be mapped
+	 * at 2M.
+	 */
+	run_guest_code(vm, guest_code0);
+	check_2m_page_count(vm, 2);
+	check_split_count(vm, 2);
+
+	/*
+	 * guest_code1 is in the same huge page as data1, so it will cause
+	 * that huge page to be remapped at 4k.
+	 */
+	run_guest_code(vm, guest_code1);
+	check_2m_page_count(vm, 1);
+	check_split_count(vm, 3);
+
+	/* Run guest_code0 again to check that is has no effect. */
+	run_guest_code(vm, guest_code0);
+	check_2m_page_count(vm, 1);
+	check_split_count(vm, 3);
+
+	/*
+	 * Give recovery thread time to run. The wrapper script sets
+	 * recovery_period_ms to 100, so wait 1.5x that.
+	 */
+	ts.tv_sec = 0;
+	ts.tv_nsec = 150000000;
+	nanosleep(&ts, NULL);
+
+	/*
+	 * Now that the reclaimer has run, all the split pages should be gone.
+	 */
+	check_2m_page_count(vm, 1);
+	check_split_count(vm, 0);
+
+	/*
+	 * The split 2M pages should have been reclaimed, so run guest_code0
+	 * again to check that pages are mapped at 2M again.
+	 */
+	run_guest_code(vm, guest_code0);
+	check_2m_page_count(vm, 2);
+	check_split_count(vm, 2);
+
+	/* Pages are once again split from running guest_code1. */
+	run_guest_code(vm, guest_code1);
+	check_2m_page_count(vm, 1);
+	check_split_count(vm, 3);
+
+	kvm_vm_free(vm);
+
+	return 0;
+}
+
diff --git a/tools/testing/selftests/kvm/x86_64/nx_huge_pages_test.sh b/tools/testing/selftests/kvm/x86_64/nx_huge_pages_test.sh
new file mode 100755
index 000000000000..19fc95723fcb
--- /dev/null
+++ b/tools/testing/selftests/kvm/x86_64/nx_huge_pages_test.sh
@@ -0,0 +1,25 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0-only */
+
+# tools/testing/selftests/kvm/nx_huge_page_test.sh
+# Copyright (C) 2022, Google LLC.
+
+NX_HUGE_PAGES=$(cat /sys/module/kvm/parameters/nx_huge_pages)
+NX_HUGE_PAGES_RECOVERY_RATIO=$(cat /sys/module/kvm/parameters/nx_huge_pages_recovery_ratio)
+NX_HUGE_PAGES_RECOVERY_PERIOD=$(cat /sys/module/kvm/parameters/nx_huge_pages_recovery_period_ms)
+HUGE_PAGES=$(cat /sys/kernel/mm/hugepages/hugepages-2048kB/nr_hugepages)
+
+echo 1 > /sys/module/kvm/parameters/nx_huge_pages
+echo 1 > /sys/module/kvm/parameters/nx_huge_pages_recovery_ratio
+echo 100 > /sys/module/kvm/parameters/nx_huge_pages_recovery_period_ms
+echo 200 > /sys/kernel/mm/hugepages/hugepages-2048kB/nr_hugepages
+
+./nx_huge_pages_test
+RET=$?
+
+echo $NX_HUGE_PAGES > /sys/module/kvm/parameters/nx_huge_pages
+echo $NX_HUGE_PAGES_RECOVERY_RATIO > /sys/module/kvm/parameters/nx_huge_pages_recovery_ratio
+echo $NX_HUGE_PAGES_RECOVERY_PERIOD > /sys/module/kvm/parameters/nx_huge_pages_recovery_period_ms
+echo $HUGE_PAGES > /sys/kernel/mm/hugepages/hugepages-2048kB/nr_hugepages
+
+exit $RET
-- 
2.35.1.1021.g381101b075-goog

Re: [PATCH v3 06/11] KVM: selftests: Add NX huge pages test

[David Matlack]

On Wed, Mar 30, 2022 at 10:46:16AM -0700, Ben Gardon wrote:
> There's currently no test coverage of NX hugepages in KVM selftests, so
> add a basic test to ensure that the feature works as intended.
> 
> Reviewed-by: David Dunn <daviddunn@google.com>
> 
> Signed-off-by: Ben Gardon <bgardon@google.com>
> ---
>  tools/testing/selftests/kvm/Makefile          |   7 +-
>  .../kvm/lib/x86_64/nx_huge_pages_guest.S      |  45 ++++++
>  .../selftests/kvm/x86_64/nx_huge_pages_test.c | 133 ++++++++++++++++++
>  .../kvm/x86_64/nx_huge_pages_test.sh          |  25 ++++
>  4 files changed, 209 insertions(+), 1 deletion(-)
>  create mode 100644 tools/testing/selftests/kvm/lib/x86_64/nx_huge_pages_guest.S
>  create mode 100644 tools/testing/selftests/kvm/x86_64/nx_huge_pages_test.c
>  create mode 100755 tools/testing/selftests/kvm/x86_64/nx_huge_pages_test.sh
> 
> diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
> index c9cdbd248727..c671224cf755 100644
> --- a/tools/testing/selftests/kvm/Makefile
> +++ b/tools/testing/selftests/kvm/Makefile
> @@ -38,7 +38,7 @@ ifeq ($(ARCH),riscv)
>  endif
>  
>  LIBKVM = lib/assert.c lib/elf.c lib/io.c lib/kvm_util.c lib/rbtree.c lib/sparsebit.c lib/test_util.c lib/guest_modes.c lib/perf_test_util.c
> -LIBKVM_x86_64 = lib/x86_64/apic.c lib/x86_64/processor.c lib/x86_64/vmx.c lib/x86_64/svm.c lib/x86_64/ucall.c lib/x86_64/handlers.S
> +LIBKVM_x86_64 = lib/x86_64/apic.c lib/x86_64/processor.c lib/x86_64/vmx.c lib/x86_64/svm.c lib/x86_64/ucall.c lib/x86_64/handlers.S lib/x86_64/nx_huge_pages_guest.S
>  LIBKVM_aarch64 = lib/aarch64/processor.c lib/aarch64/ucall.c lib/aarch64/handlers.S lib/aarch64/spinlock.c lib/aarch64/gic.c lib/aarch64/gic_v3.c lib/aarch64/vgic.c
>  LIBKVM_s390x = lib/s390x/processor.c lib/s390x/ucall.c lib/s390x/diag318_test_handler.c
>  LIBKVM_riscv = lib/riscv/processor.c lib/riscv/ucall.c
> @@ -57,6 +57,8 @@ TEST_GEN_PROGS_x86_64 += x86_64/kvm_clock_test
>  TEST_GEN_PROGS_x86_64 += x86_64/kvm_pv_test
>  TEST_GEN_PROGS_x86_64 += x86_64/mmio_warning_test
>  TEST_GEN_PROGS_x86_64 += x86_64/mmu_role_test
> +TEST_GEN_PROGS_EXTENDED_x86_64 += x86_64/nx_huge_pages_test
> +TEST_PROGS_x86_64 += x86_64/nx_huge_pages_test.sh

Suggest brearking TEST_PROGS and TEST_GEN_PROGS_EXTENDED out into their
own separate blocks with newlines in between. They capture different
types of files so I think it makes sense to separate them in the
Makefile. I expect both lists will grow over time so the awkwardness of
having them off on their lonesome is temporary :).

It'd also be nice to have some comments above each explaining when they
should be used. A short blurb is fine since the selftest documentation
is the authority.

>  TEST_GEN_PROGS_x86_64 += x86_64/platform_info_test
>  TEST_GEN_PROGS_x86_64 += x86_64/pmu_event_filter_test
>  TEST_GEN_PROGS_x86_64 += x86_64/set_boot_cpu_id
> @@ -141,7 +143,9 @@ TEST_GEN_PROGS_riscv += kvm_page_table_test
>  TEST_GEN_PROGS_riscv += set_memory_region_test
>  TEST_GEN_PROGS_riscv += kvm_binary_stats_test
>  
> +TEST_PROGS += $(TEST_PROGS_$(UNAME_M))
>  TEST_GEN_PROGS += $(TEST_GEN_PROGS_$(UNAME_M))
> +TEST_GEN_PROGS_EXTENDED += $(TEST_GEN_PROGS_EXTENDED_$(UNAME_M))
>  LIBKVM += $(LIBKVM_$(UNAME_M))
>  
>  INSTALL_HDR_PATH = $(top_srcdir)/usr
> @@ -192,6 +196,7 @@ $(OUTPUT)/libkvm.a: $(LIBKVM_OBJS)
>  x := $(shell mkdir -p $(sort $(dir $(TEST_GEN_PROGS))))
>  all: $(STATIC_LIBS)
>  $(TEST_GEN_PROGS): $(STATIC_LIBS)
> +$(TEST_GEN_PROGS_EXTENDED): $(STATIC_LIBS)
>  
>  cscope: include_paths = $(LINUX_TOOL_INCLUDE) $(LINUX_HDR_PATH) include lib ..
>  cscope:
> diff --git a/tools/testing/selftests/kvm/lib/x86_64/nx_huge_pages_guest.S b/tools/testing/selftests/kvm/lib/x86_64/nx_huge_pages_guest.S
> new file mode 100644
> index 000000000000..09c66b9562a3
> --- /dev/null
> +++ b/tools/testing/selftests/kvm/lib/x86_64/nx_huge_pages_guest.S
> @@ -0,0 +1,45 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * tools/testing/selftests/kvm/nx_huge_page_guest.S
> + *
> + * Copyright (C) 2022, Google LLC.
> + */
> +
> +.include "kvm_util.h"
> +
> +#define HPAGE_SIZE 	(2*1024*1024)
> +#define PORT_SUCCESS	0x70
> +
> +.global guest_code0
> +.global guest_code1
> +
> +.align HPAGE_SIZE
> +exit_vm:
> +	mov    $0x1,%edi
> +	mov    $0x2,%esi
> +	mov    a_string,%edx
> +	mov    $0x1,%ecx
> +	xor    %eax,%eax
> +	jmp    ucall
> +
> +
> +guest_code0:
> +	mov data1, %eax
> +	mov data2, %eax
> +	jmp exit_vm
> +
> +.align HPAGE_SIZE
> +guest_code1:
> +	mov data1, %eax
> +	mov data2, %eax
> +	jmp exit_vm
> +data1:
> +.quad	0

What do you think about my idea in [1] of using ret instructions and
function pointers to trigger execution on an arbitrary page? That would
avoid the need for this assembly file and we could probably share the
code between our tests.

Feel free to take the idea and incorporate it directly if you agree, and
I'll rebase on top, since you're series is further along than mine.

[1] https://lore.kernel.org/kvm/20220401233737.3021889-2-dmatlack@google.com/

> +
> +.align HPAGE_SIZE
> +data2:
> +.quad	0
> +a_string:
> +.string "why does the ucall function take a string argument?"
> +
> +
> 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
> new file mode 100644
> index 000000000000..2bcbe4efdc6a
> --- /dev/null
> +++ b/tools/testing/selftests/kvm/x86_64/nx_huge_pages_test.c
> @@ -0,0 +1,133 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * tools/testing/selftests/kvm/nx_huge_page_test.c
> + *
> + * Usage: to be run via nx_huge_page_test.sh, which does the necessary
> + * environment setup and teardown
> + *
> + * Copyright (C) 2022, Google LLC.
> + */
> +
> +#define _GNU_SOURCE
> +
> +#include <fcntl.h>
> +#include <stdint.h>
> +#include <time.h>
> +
> +#include <test_util.h>
> +#include "kvm_util.h"
> +
> +#define HPAGE_SLOT		10
> +#define HPAGE_PADDR_START       (10*1024*1024)
> +#define HPAGE_SLOT_NPAGES	(100*1024*1024/4096)
> +
> +/* Defined in nx_huge_page_guest.S */
> +void guest_code0(void);
> +void guest_code1(void);
> +
> +static void run_guest_code(struct kvm_vm *vm, void (*guest_code)(void))
> +{
> +	struct kvm_regs regs;
> +
> +	vcpu_regs_get(vm, 0, &regs);
> +	regs.rip = (uint64_t)guest_code;
> +	vcpu_regs_set(vm, 0, &regs);
> +	vcpu_run(vm, 0);
> +}
> +
> +static void check_2m_page_count(struct kvm_vm *vm, int expected_pages_2m)
> +{
> +	int actual_pages_2m;
> +
> +	actual_pages_2m = vm_get_single_stat(vm, "pages_2m");
> +
> +	TEST_ASSERT(actual_pages_2m == expected_pages_2m,
> +		    "Unexpected 2m page count. Expected %d, got %d",
> +		    expected_pages_2m, actual_pages_2m);
> +}
> +
> +static void check_split_count(struct kvm_vm *vm, int expected_splits)
> +{
> +	int actual_splits;
> +
> +	actual_splits = vm_get_single_stat(vm, "nx_lpage_splits");
> +
> +	TEST_ASSERT(actual_splits == expected_splits,
> +		    "Unexpected nx lpage split count. Expected %d, got %d",
> +		    expected_splits, actual_splits);
> +}
> +
> +int main(int argc, char **argv)
> +{
> +	struct kvm_vm *vm;
> +	struct timespec ts;
> +
> +	vm = vm_create(VM_MODE_DEFAULT, DEFAULT_GUEST_PHY_PAGES, O_RDWR);
> +
> +	vm_userspace_mem_region_add(vm, VM_MEM_SRC_ANONYMOUS_HUGETLB,
> +				    HPAGE_PADDR_START, HPAGE_SLOT,
> +				    HPAGE_SLOT_NPAGES, 0);
> +
> +	kvm_vm_elf_load_memslot(vm, program_invocation_name, HPAGE_SLOT);
> +
> +	vm_vcpu_add_default(vm, 0, guest_code0);
> +
> +	check_2m_page_count(vm, 0);
> +	check_split_count(vm, 0);
> +
> +	/*
> +	 * Running guest_code0 will access data1 and data2.
> +	 * This should result in part of the huge page containing guest_code0,
> +	 * and part of the hugepage containing the ucall function being mapped
> +	 * at 4K. The huge pages containing data1 and data2 will be mapped
> +	 * at 2M.
> +	 */
> +	run_guest_code(vm, guest_code0);
> +	check_2m_page_count(vm, 2);
> +	check_split_count(vm, 2);
> +
> +	/*
> +	 * guest_code1 is in the same huge page as data1, so it will cause
> +	 * that huge page to be remapped at 4k.
> +	 */
> +	run_guest_code(vm, guest_code1);
> +	check_2m_page_count(vm, 1);
> +	check_split_count(vm, 3);
> +
> +	/* Run guest_code0 again to check that is has no effect. */
> +	run_guest_code(vm, guest_code0);
> +	check_2m_page_count(vm, 1);
> +	check_split_count(vm, 3);
> +
> +	/*
> +	 * Give recovery thread time to run. The wrapper script sets
> +	 * recovery_period_ms to 100, so wait 1.5x that.
> +	 */

So we give it an extra 50ms? That should probably be enough but I'm
paranoid so I'd probably bump it up to 500 ms.

> +	ts.tv_sec = 0;
> +	ts.tv_nsec = 150000000;
> +	nanosleep(&ts, NULL);
> +
> +	/*
> +	 * Now that the reclaimer has run, all the split pages should be gone.
> +	 */
> +	check_2m_page_count(vm, 1);
> +	check_split_count(vm, 0);
> +
> +	/*
> +	 * The split 2M pages should have been reclaimed, so run guest_code0
> +	 * again to check that pages are mapped at 2M again.
> +	 */
> +	run_guest_code(vm, guest_code0);
> +	check_2m_page_count(vm, 2);
> +	check_split_count(vm, 2);
> +
> +	/* Pages are once again split from running guest_code1. */
> +	run_guest_code(vm, guest_code1);
> +	check_2m_page_count(vm, 1);
> +	check_split_count(vm, 3);
> +
> +	kvm_vm_free(vm);
> +
> +	return 0;
> +}
> +
> diff --git a/tools/testing/selftests/kvm/x86_64/nx_huge_pages_test.sh b/tools/testing/selftests/kvm/x86_64/nx_huge_pages_test.sh
> new file mode 100755
> index 000000000000..19fc95723fcb
> --- /dev/null
> +++ b/tools/testing/selftests/kvm/x86_64/nx_huge_pages_test.sh
> @@ -0,0 +1,25 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-2.0-only */
> +
> +# tools/testing/selftests/kvm/nx_huge_page_test.sh
> +# Copyright (C) 2022, Google LLC.
> +
> +NX_HUGE_PAGES=$(cat /sys/module/kvm/parameters/nx_huge_pages)
> +NX_HUGE_PAGES_RECOVERY_RATIO=$(cat /sys/module/kvm/parameters/nx_huge_pages_recovery_ratio)
> +NX_HUGE_PAGES_RECOVERY_PERIOD=$(cat /sys/module/kvm/parameters/nx_huge_pages_recovery_period_ms)
> +HUGE_PAGES=$(cat /sys/kernel/mm/hugepages/hugepages-2048kB/nr_hugepages)
> +
> +echo 1 > /sys/module/kvm/parameters/nx_huge_pages
> +echo 1 > /sys/module/kvm/parameters/nx_huge_pages_recovery_ratio
> +echo 100 > /sys/module/kvm/parameters/nx_huge_pages_recovery_period_ms
> +echo 200 > /sys/kernel/mm/hugepages/hugepages-2048kB/nr_hugepages
> +
> +./nx_huge_pages_test
> +RET=$?
> +
> +echo $NX_HUGE_PAGES > /sys/module/kvm/parameters/nx_huge_pages
> +echo $NX_HUGE_PAGES_RECOVERY_RATIO > /sys/module/kvm/parameters/nx_huge_pages_recovery_ratio
> +echo $NX_HUGE_PAGES_RECOVERY_PERIOD > /sys/module/kvm/parameters/nx_huge_pages_recovery_period_ms
> +echo $HUGE_PAGES > /sys/kernel/mm/hugepages/hugepages-2048kB/nr_hugepages
> +
> +exit $RET
> -- 
> 2.35.1.1021.g381101b075-goog
> 

Re: [PATCH v3 06/11] KVM: selftests: Add NX huge pages test

[Ben Gardon]

On Tue, Apr 5, 2022 at 3:38 PM David Matlack <dmatlack@google.com> wrote:
>
> On Wed, Mar 30, 2022 at 10:46:16AM -0700, Ben Gardon wrote:
> > There's currently no test coverage of NX hugepages in KVM selftests, so
> > add a basic test to ensure that the feature works as intended.
> >
> > Reviewed-by: David Dunn <daviddunn@google.com>
> >
> > Signed-off-by: Ben Gardon <bgardon@google.com>
> > ---
> >  tools/testing/selftests/kvm/Makefile          |   7 +-
> >  .../kvm/lib/x86_64/nx_huge_pages_guest.S      |  45 ++++++
> >  .../selftests/kvm/x86_64/nx_huge_pages_test.c | 133 ++++++++++++++++++
> >  .../kvm/x86_64/nx_huge_pages_test.sh          |  25 ++++
> >  4 files changed, 209 insertions(+), 1 deletion(-)
> >  create mode 100644 tools/testing/selftests/kvm/lib/x86_64/nx_huge_pages_guest.S
> >  create mode 100644 tools/testing/selftests/kvm/x86_64/nx_huge_pages_test.c
> >  create mode 100755 tools/testing/selftests/kvm/x86_64/nx_huge_pages_test.sh
> >
> > diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
> > index c9cdbd248727..c671224cf755 100644
> > --- a/tools/testing/selftests/kvm/Makefile
> > +++ b/tools/testing/selftests/kvm/Makefile
> > @@ -38,7 +38,7 @@ ifeq ($(ARCH),riscv)
> >  endif
> >
> >  LIBKVM = lib/assert.c lib/elf.c lib/io.c lib/kvm_util.c lib/rbtree.c lib/sparsebit.c lib/test_util.c lib/guest_modes.c lib/perf_test_util.c
> > -LIBKVM_x86_64 = lib/x86_64/apic.c lib/x86_64/processor.c lib/x86_64/vmx.c lib/x86_64/svm.c lib/x86_64/ucall.c lib/x86_64/handlers.S
> > +LIBKVM_x86_64 = lib/x86_64/apic.c lib/x86_64/processor.c lib/x86_64/vmx.c lib/x86_64/svm.c lib/x86_64/ucall.c lib/x86_64/handlers.S lib/x86_64/nx_huge_pages_guest.S
> >  LIBKVM_aarch64 = lib/aarch64/processor.c lib/aarch64/ucall.c lib/aarch64/handlers.S lib/aarch64/spinlock.c lib/aarch64/gic.c lib/aarch64/gic_v3.c lib/aarch64/vgic.c
> >  LIBKVM_s390x = lib/s390x/processor.c lib/s390x/ucall.c lib/s390x/diag318_test_handler.c
> >  LIBKVM_riscv = lib/riscv/processor.c lib/riscv/ucall.c
> > @@ -57,6 +57,8 @@ TEST_GEN_PROGS_x86_64 += x86_64/kvm_clock_test
> >  TEST_GEN_PROGS_x86_64 += x86_64/kvm_pv_test
> >  TEST_GEN_PROGS_x86_64 += x86_64/mmio_warning_test
> >  TEST_GEN_PROGS_x86_64 += x86_64/mmu_role_test
> > +TEST_GEN_PROGS_EXTENDED_x86_64 += x86_64/nx_huge_pages_test
> > +TEST_PROGS_x86_64 += x86_64/nx_huge_pages_test.sh
>
> Suggest brearking TEST_PROGS and TEST_GEN_PROGS_EXTENDED out into their
> own separate blocks with newlines in between. They capture different
> types of files so I think it makes sense to separate them in the
> Makefile. I expect both lists will grow over time so the awkwardness of
> having them off on their lonesome is temporary :).
>
> It'd also be nice to have some comments above each explaining when they
> should be used. A short blurb is fine since the selftest documentation
> is the authority.

Will do.

>
> >  TEST_GEN_PROGS_x86_64 += x86_64/platform_info_test
> >  TEST_GEN_PROGS_x86_64 += x86_64/pmu_event_filter_test
> >  TEST_GEN_PROGS_x86_64 += x86_64/set_boot_cpu_id
> > @@ -141,7 +143,9 @@ TEST_GEN_PROGS_riscv += kvm_page_table_test
> >  TEST_GEN_PROGS_riscv += set_memory_region_test
> >  TEST_GEN_PROGS_riscv += kvm_binary_stats_test
> >
> > +TEST_PROGS += $(TEST_PROGS_$(UNAME_M))
> >  TEST_GEN_PROGS += $(TEST_GEN_PROGS_$(UNAME_M))
> > +TEST_GEN_PROGS_EXTENDED += $(TEST_GEN_PROGS_EXTENDED_$(UNAME_M))
> >  LIBKVM += $(LIBKVM_$(UNAME_M))
> >
> >  INSTALL_HDR_PATH = $(top_srcdir)/usr
> > @@ -192,6 +196,7 @@ $(OUTPUT)/libkvm.a: $(LIBKVM_OBJS)
> >  x := $(shell mkdir -p $(sort $(dir $(TEST_GEN_PROGS))))
> >  all: $(STATIC_LIBS)
> >  $(TEST_GEN_PROGS): $(STATIC_LIBS)
> > +$(TEST_GEN_PROGS_EXTENDED): $(STATIC_LIBS)
> >
> >  cscope: include_paths = $(LINUX_TOOL_INCLUDE) $(LINUX_HDR_PATH) include lib ..
> >  cscope:
> > diff --git a/tools/testing/selftests/kvm/lib/x86_64/nx_huge_pages_guest.S b/tools/testing/selftests/kvm/lib/x86_64/nx_huge_pages_guest.S
> > new file mode 100644
> > index 000000000000..09c66b9562a3
> > --- /dev/null
> > +++ b/tools/testing/selftests/kvm/lib/x86_64/nx_huge_pages_guest.S
> > @@ -0,0 +1,45 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/*
> > + * tools/testing/selftests/kvm/nx_huge_page_guest.S
> > + *
> > + * Copyright (C) 2022, Google LLC.
> > + */
> > +
> > +.include "kvm_util.h"
> > +
> > +#define HPAGE_SIZE   (2*1024*1024)
> > +#define PORT_SUCCESS 0x70
> > +
> > +.global guest_code0
> > +.global guest_code1
> > +
> > +.align HPAGE_SIZE
> > +exit_vm:
> > +     mov    $0x1,%edi
> > +     mov    $0x2,%esi
> > +     mov    a_string,%edx
> > +     mov    $0x1,%ecx
> > +     xor    %eax,%eax
> > +     jmp    ucall
> > +
> > +
> > +guest_code0:
> > +     mov data1, %eax
> > +     mov data2, %eax
> > +     jmp exit_vm
> > +
> > +.align HPAGE_SIZE
> > +guest_code1:
> > +     mov data1, %eax
> > +     mov data2, %eax
> > +     jmp exit_vm
> > +data1:
> > +.quad        0
>
> What do you think about my idea in [1] of using ret instructions and
> function pointers to trigger execution on an arbitrary page? That would
> avoid the need for this assembly file and we could probably share the
> code between our tests.
>
> Feel free to take the idea and incorporate it directly if you agree, and
> I'll rebase on top, since you're series is further along than mine.
>
> [1] https://lore.kernel.org/kvm/20220401233737.3021889-2-dmatlack@google.com/
>

Sigh, it means I have to rewrite a lot of the test, but it is the
better way to write this test.

> > +
> > +.align HPAGE_SIZE
> > +data2:
> > +.quad        0
> > +a_string:
> > +.string "why does the ucall function take a string argument?"
> > +
> > +
> > 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
> > new file mode 100644
> > index 000000000000..2bcbe4efdc6a
> > --- /dev/null
> > +++ b/tools/testing/selftests/kvm/x86_64/nx_huge_pages_test.c
> > @@ -0,0 +1,133 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * tools/testing/selftests/kvm/nx_huge_page_test.c
> > + *
> > + * Usage: to be run via nx_huge_page_test.sh, which does the necessary
> > + * environment setup and teardown
> > + *
> > + * Copyright (C) 2022, Google LLC.
> > + */
> > +
> > +#define _GNU_SOURCE
> > +
> > +#include <fcntl.h>
> > +#include <stdint.h>
> > +#include <time.h>
> > +
> > +#include <test_util.h>
> > +#include "kvm_util.h"
> > +
> > +#define HPAGE_SLOT           10
> > +#define HPAGE_PADDR_START       (10*1024*1024)
> > +#define HPAGE_SLOT_NPAGES    (100*1024*1024/4096)
> > +
> > +/* Defined in nx_huge_page_guest.S */
> > +void guest_code0(void);
> > +void guest_code1(void);
> > +
> > +static void run_guest_code(struct kvm_vm *vm, void (*guest_code)(void))
> > +{
> > +     struct kvm_regs regs;
> > +
> > +     vcpu_regs_get(vm, 0, &regs);
> > +     regs.rip = (uint64_t)guest_code;
> > +     vcpu_regs_set(vm, 0, &regs);
> > +     vcpu_run(vm, 0);
> > +}
> > +
> > +static void check_2m_page_count(struct kvm_vm *vm, int expected_pages_2m)
> > +{
> > +     int actual_pages_2m;
> > +
> > +     actual_pages_2m = vm_get_single_stat(vm, "pages_2m");
> > +
> > +     TEST_ASSERT(actual_pages_2m == expected_pages_2m,
> > +                 "Unexpected 2m page count. Expected %d, got %d",
> > +                 expected_pages_2m, actual_pages_2m);
> > +}
> > +
> > +static void check_split_count(struct kvm_vm *vm, int expected_splits)
> > +{
> > +     int actual_splits;
> > +
> > +     actual_splits = vm_get_single_stat(vm, "nx_lpage_splits");
> > +
> > +     TEST_ASSERT(actual_splits == expected_splits,
> > +                 "Unexpected nx lpage split count. Expected %d, got %d",
> > +                 expected_splits, actual_splits);
> > +}
> > +
> > +int main(int argc, char **argv)
> > +{
> > +     struct kvm_vm *vm;
> > +     struct timespec ts;
> > +
> > +     vm = vm_create(VM_MODE_DEFAULT, DEFAULT_GUEST_PHY_PAGES, O_RDWR);
> > +
> > +     vm_userspace_mem_region_add(vm, VM_MEM_SRC_ANONYMOUS_HUGETLB,
> > +                                 HPAGE_PADDR_START, HPAGE_SLOT,
> > +                                 HPAGE_SLOT_NPAGES, 0);
> > +
> > +     kvm_vm_elf_load_memslot(vm, program_invocation_name, HPAGE_SLOT);
> > +
> > +     vm_vcpu_add_default(vm, 0, guest_code0);
> > +
> > +     check_2m_page_count(vm, 0);
> > +     check_split_count(vm, 0);
> > +
> > +     /*
> > +      * Running guest_code0 will access data1 and data2.
> > +      * This should result in part of the huge page containing guest_code0,
> > +      * and part of the hugepage containing the ucall function being mapped
> > +      * at 4K. The huge pages containing data1 and data2 will be mapped
> > +      * at 2M.
> > +      */
> > +     run_guest_code(vm, guest_code0);
> > +     check_2m_page_count(vm, 2);
> > +     check_split_count(vm, 2);
> > +
> > +     /*
> > +      * guest_code1 is in the same huge page as data1, so it will cause
> > +      * that huge page to be remapped at 4k.
> > +      */
> > +     run_guest_code(vm, guest_code1);
> > +     check_2m_page_count(vm, 1);
> > +     check_split_count(vm, 3);
> > +
> > +     /* Run guest_code0 again to check that is has no effect. */
> > +     run_guest_code(vm, guest_code0);
> > +     check_2m_page_count(vm, 1);
> > +     check_split_count(vm, 3);
> > +
> > +     /*
> > +      * Give recovery thread time to run. The wrapper script sets
> > +      * recovery_period_ms to 100, so wait 1.5x that.
> > +      */
>
> So we give it an extra 50ms? That should probably be enough but I'm
> paranoid so I'd probably bump it up to 500 ms.

Will do.

>
> > +     ts.tv_sec = 0;
> > +     ts.tv_nsec = 150000000;
> > +     nanosleep(&ts, NULL);
> > +
> > +     /*
> > +      * Now that the reclaimer has run, all the split pages should be gone.
> > +      */
> > +     check_2m_page_count(vm, 1);
> > +     check_split_count(vm, 0);
> > +
> > +     /*
> > +      * The split 2M pages should have been reclaimed, so run guest_code0
> > +      * again to check that pages are mapped at 2M again.
> > +      */
> > +     run_guest_code(vm, guest_code0);
> > +     check_2m_page_count(vm, 2);
> > +     check_split_count(vm, 2);
> > +
> > +     /* Pages are once again split from running guest_code1. */
> > +     run_guest_code(vm, guest_code1);
> > +     check_2m_page_count(vm, 1);
> > +     check_split_count(vm, 3);
> > +
> > +     kvm_vm_free(vm);
> > +
> > +     return 0;
> > +}
> > +
> > diff --git a/tools/testing/selftests/kvm/x86_64/nx_huge_pages_test.sh b/tools/testing/selftests/kvm/x86_64/nx_huge_pages_test.sh
> > new file mode 100755
> > index 000000000000..19fc95723fcb
> > --- /dev/null
> > +++ b/tools/testing/selftests/kvm/x86_64/nx_huge_pages_test.sh
> > @@ -0,0 +1,25 @@
> > +#!/bin/bash
> > +# SPDX-License-Identifier: GPL-2.0-only */
> > +
> > +# tools/testing/selftests/kvm/nx_huge_page_test.sh
> > +# Copyright (C) 2022, Google LLC.
> > +
> > +NX_HUGE_PAGES=$(cat /sys/module/kvm/parameters/nx_huge_pages)
> > +NX_HUGE_PAGES_RECOVERY_RATIO=$(cat /sys/module/kvm/parameters/nx_huge_pages_recovery_ratio)
> > +NX_HUGE_PAGES_RECOVERY_PERIOD=$(cat /sys/module/kvm/parameters/nx_huge_pages_recovery_period_ms)
> > +HUGE_PAGES=$(cat /sys/kernel/mm/hugepages/hugepages-2048kB/nr_hugepages)
> > +
> > +echo 1 > /sys/module/kvm/parameters/nx_huge_pages
> > +echo 1 > /sys/module/kvm/parameters/nx_huge_pages_recovery_ratio
> > +echo 100 > /sys/module/kvm/parameters/nx_huge_pages_recovery_period_ms
> > +echo 200 > /sys/kernel/mm/hugepages/hugepages-2048kB/nr_hugepages
> > +
> > +./nx_huge_pages_test
> > +RET=$?
> > +
> > +echo $NX_HUGE_PAGES > /sys/module/kvm/parameters/nx_huge_pages
> > +echo $NX_HUGE_PAGES_RECOVERY_RATIO > /sys/module/kvm/parameters/nx_huge_pages_recovery_ratio
> > +echo $NX_HUGE_PAGES_RECOVERY_PERIOD > /sys/module/kvm/parameters/nx_huge_pages_recovery_period_ms
> > +echo $HUGE_PAGES > /sys/kernel/mm/hugepages/hugepages-2048kB/nr_hugepages
> > +
> > +exit $RET
> > --
> > 2.35.1.1021.g381101b075-goog
> >

[PATCH v3 07/11] KVM: x86/MMU: Factor out updating NX hugepages state for a VM

[Ben Gardon]

Factor out the code to update the NX hugepages state for an individual
VM. This will be expanded in future commits to allow per-VM control of
Nx hugepages.

No functional change intended.

Signed-off-by: Ben Gardon <bgardon@google.com>
---
 arch/x86/kvm/mmu/mmu.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
index dbf46dd98618..af428cb65b3f 100644
--- a/arch/x86/kvm/mmu/mmu.c
+++ b/arch/x86/kvm/mmu/mmu.c
@@ -6202,6 +6202,15 @@ static void __set_nx_huge_pages(bool val)
 	nx_huge_pages = itlb_multihit_kvm_mitigation = val;
 }
 
+static void kvm_update_nx_huge_pages(struct kvm *kvm)
+{
+	mutex_lock(&kvm->slots_lock);
+	kvm_mmu_zap_all_fast(kvm);
+	mutex_unlock(&kvm->slots_lock);
+
+	wake_up_process(kvm->arch.nx_lpage_recovery_thread);
+}
+
 static int set_nx_huge_pages(const char *val, const struct kernel_param *kp)
 {
 	bool old_val = nx_huge_pages;
@@ -6224,13 +6233,9 @@ static int set_nx_huge_pages(const char *val, const struct kernel_param *kp)
 
 		mutex_lock(&kvm_lock);
 
-		list_for_each_entry(kvm, &vm_list, vm_list) {
-			mutex_lock(&kvm->slots_lock);
-			kvm_mmu_zap_all_fast(kvm);
-			mutex_unlock(&kvm->slots_lock);
+		list_for_each_entry(kvm, &vm_list, vm_list)
+			kvm_update_nx_huge_pages(kvm);
 
-			wake_up_process(kvm->arch.nx_lpage_recovery_thread);
-		}
 		mutex_unlock(&kvm_lock);
 	}
 
-- 
2.35.1.1021.g381101b075-goog

Re: [PATCH v3 07/11] KVM: x86/MMU: Factor out updating NX hugepages state for a VM

[David Matlack]

On Wed, Mar 30, 2022 at 10:46:17AM -0700, Ben Gardon wrote:
> Factor out the code to update the NX hugepages state for an individual
> VM. This will be expanded in future commits to allow per-VM control of
> Nx hugepages.
> 
> No functional change intended.
> 
> Signed-off-by: Ben Gardon <bgardon@google.com>

Reviewed-by: David Matlack <dmatlack@google.com>

> ---
>  arch/x86/kvm/mmu/mmu.c | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> index dbf46dd98618..af428cb65b3f 100644
> --- a/arch/x86/kvm/mmu/mmu.c
> +++ b/arch/x86/kvm/mmu/mmu.c
> @@ -6202,6 +6202,15 @@ static void __set_nx_huge_pages(bool val)
>  	nx_huge_pages = itlb_multihit_kvm_mitigation = val;
>  }
>  
> +static void kvm_update_nx_huge_pages(struct kvm *kvm)
> +{
> +	mutex_lock(&kvm->slots_lock);
> +	kvm_mmu_zap_all_fast(kvm);
> +	mutex_unlock(&kvm->slots_lock);
> +
> +	wake_up_process(kvm->arch.nx_lpage_recovery_thread);
> +}
> +
>  static int set_nx_huge_pages(const char *val, const struct kernel_param *kp)
>  {
>  	bool old_val = nx_huge_pages;
> @@ -6224,13 +6233,9 @@ static int set_nx_huge_pages(const char *val, const struct kernel_param *kp)
>  
>  		mutex_lock(&kvm_lock);
>  
> -		list_for_each_entry(kvm, &vm_list, vm_list) {
> -			mutex_lock(&kvm->slots_lock);
> -			kvm_mmu_zap_all_fast(kvm);
> -			mutex_unlock(&kvm->slots_lock);
> +		list_for_each_entry(kvm, &vm_list, vm_list)
> +			kvm_update_nx_huge_pages(kvm);
>  
> -			wake_up_process(kvm->arch.nx_lpage_recovery_thread);
> -		}
>  		mutex_unlock(&kvm_lock);
>  	}
>  
> -- 
> 2.35.1.1021.g381101b075-goog
> 

[PATCH v3 08/11] KVM: x86/MMU: Allow NX huge pages to be disabled on a per-vm basis

[Ben Gardon]

In some cases, the NX hugepage mitigation for iTLB multihit is not
needed for all guests on a host. Allow disabling the mitigation on a
per-VM basis to avoid the performance hit of NX hugepages on trusted
workloads.

Signed-off-by: Ben Gardon <bgardon@google.com>
---
 Documentation/virt/kvm/api.rst  | 11 +++++++++++
 arch/x86/include/asm/kvm_host.h |  2 ++
 arch/x86/kvm/mmu.h              | 10 ++++++----
 arch/x86/kvm/mmu/mmu.c          |  2 +-
 arch/x86/kvm/mmu/spte.c         |  7 ++++---
 arch/x86/kvm/mmu/spte.h         |  3 ++-
 arch/x86/kvm/mmu/tdp_mmu.c      |  3 ++-
 arch/x86/kvm/x86.c              |  6 ++++++
 include/uapi/linux/kvm.h        |  1 +
 9 files changed, 35 insertions(+), 10 deletions(-)

diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
index b102ba7cf903..b40c3113b14b 100644
--- a/Documentation/virt/kvm/api.rst
+++ b/Documentation/virt/kvm/api.rst
@@ -7844,6 +7844,17 @@ At this time, KVM_PMU_CAP_DISABLE is the only capability.  Setting
 this capability will disable PMU virtualization for that VM.  Usermode
 should adjust CPUID leaf 0xA to reflect that the PMU is disabled.
 
+8.36 KVM_CAP_VM_DISABLE_NX_HUGE_PAGES
+---------------------------
+
+:Capability KVM_CAP_PMU_CAPABILITY
+:Architectures: x86
+:Type: vm
+
+This capability disables the NX huge pages mitigation for iTLB MULTIHIT.
+
+The capability has no effect if the nx_huge_pages module parameter is not set.
+
 9. Known KVM API problems
 =========================
 
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 676705ad1e23..dcff7709444d 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1246,6 +1246,8 @@ struct kvm_arch {
 	hpa_t	hv_root_tdp;
 	spinlock_t hv_root_tdp_lock;
 #endif
+
+	bool disable_nx_huge_pages;
 };
 
 struct kvm_vm_stat {
diff --git a/arch/x86/kvm/mmu.h b/arch/x86/kvm/mmu.h
index e6cae6f22683..69cffc86b888 100644
--- a/arch/x86/kvm/mmu.h
+++ b/arch/x86/kvm/mmu.h
@@ -173,10 +173,12 @@ struct kvm_page_fault {
 int kvm_tdp_page_fault(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault);
 
 extern int nx_huge_pages;
-static inline bool is_nx_huge_page_enabled(void)
+static inline bool is_nx_huge_page_enabled(struct kvm *kvm)
 {
-	return READ_ONCE(nx_huge_pages);
+	return READ_ONCE(nx_huge_pages) &&
+	       !kvm->arch.disable_nx_huge_pages;
 }
+void kvm_update_nx_huge_pages(struct kvm *kvm);
 
 static inline int kvm_mmu_do_page_fault(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa,
 					u32 err, bool prefetch)
@@ -191,8 +193,8 @@ static inline int kvm_mmu_do_page_fault(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa,
 		.user = err & PFERR_USER_MASK,
 		.prefetch = prefetch,
 		.is_tdp = likely(vcpu->arch.mmu->page_fault == kvm_tdp_page_fault),
-		.nx_huge_page_workaround_enabled = is_nx_huge_page_enabled(),
-
+		.nx_huge_page_workaround_enabled =
+			is_nx_huge_page_enabled(vcpu->kvm),
 		.max_level = KVM_MAX_HUGEPAGE_LEVEL,
 		.req_level = PG_LEVEL_4K,
 		.goal_level = PG_LEVEL_4K,
diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
index af428cb65b3f..eb7b935d3caa 100644
--- a/arch/x86/kvm/mmu/mmu.c
+++ b/arch/x86/kvm/mmu/mmu.c
@@ -6202,7 +6202,7 @@ static void __set_nx_huge_pages(bool val)
 	nx_huge_pages = itlb_multihit_kvm_mitigation = val;
 }
 
-static void kvm_update_nx_huge_pages(struct kvm *kvm)
+void kvm_update_nx_huge_pages(struct kvm *kvm)
 {
 	mutex_lock(&kvm->slots_lock);
 	kvm_mmu_zap_all_fast(kvm);
diff --git a/arch/x86/kvm/mmu/spte.c b/arch/x86/kvm/mmu/spte.c
index 4739b53c9734..877ad30bc7ad 100644
--- a/arch/x86/kvm/mmu/spte.c
+++ b/arch/x86/kvm/mmu/spte.c
@@ -116,7 +116,7 @@ bool make_spte(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp,
 		spte |= spte_shadow_accessed_mask(spte);
 
 	if (level > PG_LEVEL_4K && (pte_access & ACC_EXEC_MASK) &&
-	    is_nx_huge_page_enabled()) {
+	    is_nx_huge_page_enabled(vcpu->kvm)) {
 		pte_access &= ~ACC_EXEC_MASK;
 	}
 
@@ -215,7 +215,8 @@ static u64 make_spte_executable(u64 spte)
  * This is used during huge page splitting to build the SPTEs that make up the
  * new page table.
  */
-u64 make_huge_page_split_spte(u64 huge_spte, int huge_level, int index)
+u64 make_huge_page_split_spte(struct kvm *kvm, u64 huge_spte, int huge_level,
+			      int index)
 {
 	u64 child_spte;
 	int child_level;
@@ -243,7 +244,7 @@ u64 make_huge_page_split_spte(u64 huge_spte, int huge_level, int index)
 		 * When splitting to a 4K page, mark the page executable as the
 		 * NX hugepage mitigation no longer applies.
 		 */
-		if (is_nx_huge_page_enabled())
+		if (is_nx_huge_page_enabled(kvm))
 			child_spte = make_spte_executable(child_spte);
 	}
 
diff --git a/arch/x86/kvm/mmu/spte.h b/arch/x86/kvm/mmu/spte.h
index 73f12615416f..e4142caff4b1 100644
--- a/arch/x86/kvm/mmu/spte.h
+++ b/arch/x86/kvm/mmu/spte.h
@@ -415,7 +415,8 @@ bool make_spte(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp,
 	       unsigned int pte_access, gfn_t gfn, kvm_pfn_t pfn,
 	       u64 old_spte, bool prefetch, bool can_unsync,
 	       bool host_writable, u64 *new_spte);
-u64 make_huge_page_split_spte(u64 huge_spte, int huge_level, int index);
+u64 make_huge_page_split_spte(struct kvm *kvm, u64 huge_spte, int huge_level,
+			      int index);
 u64 make_nonleaf_spte(u64 *child_pt, bool ad_disabled);
 u64 make_mmio_spte(struct kvm_vcpu *vcpu, u64 gfn, unsigned int access);
 u64 mark_spte_for_access_track(u64 spte);
diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c
index a2f9a34a0168..5d82a54924e6 100644
--- a/arch/x86/kvm/mmu/tdp_mmu.c
+++ b/arch/x86/kvm/mmu/tdp_mmu.c
@@ -1469,7 +1469,8 @@ static int tdp_mmu_split_huge_page(struct kvm *kvm, struct tdp_iter *iter,
 	 * not been linked in yet and thus is not reachable from any other CPU.
 	 */
 	for (i = 0; i < PT64_ENT_PER_PAGE; i++)
-		sp->spt[i] = make_huge_page_split_spte(huge_spte, level, i);
+		sp->spt[i] = make_huge_page_split_spte(kvm, huge_spte,
+						       level, i);
 
 	/*
 	 * Replace the huge spte with a pointer to the populated lower level
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 7a066cf92692..ea1d620b35df 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -4268,6 +4268,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
 	case KVM_CAP_SYS_ATTRIBUTES:
 	case KVM_CAP_VAPIC:
 	case KVM_CAP_ENABLE_CAP:
+	case KVM_CAP_VM_DISABLE_NX_HUGE_PAGES:
 		r = 1;
 		break;
 	case KVM_CAP_EXIT_HYPERCALL:
@@ -6061,6 +6062,11 @@ int kvm_vm_ioctl_enable_cap(struct kvm *kvm,
 		}
 		mutex_unlock(&kvm->lock);
 		break;
+	case KVM_CAP_VM_DISABLE_NX_HUGE_PAGES:
+		kvm->arch.disable_nx_huge_pages = true;
+		kvm_update_nx_huge_pages(kvm);
+		r = 0;
+		break;
 	default:
 		r = -EINVAL;
 		break;
diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index 8616af85dc5d..12399c969b42 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -1145,6 +1145,7 @@ struct kvm_ppc_resize_hpt {
 #define KVM_CAP_PMU_CAPABILITY 212
 #define KVM_CAP_DISABLE_QUIRKS2 213
 #define KVM_CAP_VM_TSC_CONTROL 214
+#define KVM_CAP_VM_DISABLE_NX_HUGE_PAGES 215
 
 #ifdef KVM_CAP_IRQ_ROUTING
 
-- 
2.35.1.1021.g381101b075-goog

Re: [PATCH v3 08/11] KVM: x86/MMU: Allow NX huge pages to be disabled on a per-vm basis

[David Matlack]

On Wed, Mar 30, 2022 at 10:46:18AM -0700, Ben Gardon wrote:
> In some cases, the NX hugepage mitigation for iTLB multihit is not
> needed for all guests on a host. Allow disabling the mitigation on a
> per-VM basis to avoid the performance hit of NX hugepages on trusted
> workloads.
> 
> Signed-off-by: Ben Gardon <bgardon@google.com>

Reviewed-by: David Matlack <dmatlack@google.com>

> ---
>  Documentation/virt/kvm/api.rst  | 11 +++++++++++
>  arch/x86/include/asm/kvm_host.h |  2 ++
>  arch/x86/kvm/mmu.h              | 10 ++++++----
>  arch/x86/kvm/mmu/mmu.c          |  2 +-
>  arch/x86/kvm/mmu/spte.c         |  7 ++++---
>  arch/x86/kvm/mmu/spte.h         |  3 ++-
>  arch/x86/kvm/mmu/tdp_mmu.c      |  3 ++-
>  arch/x86/kvm/x86.c              |  6 ++++++
>  include/uapi/linux/kvm.h        |  1 +
>  9 files changed, 35 insertions(+), 10 deletions(-)
> 
> diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
> index b102ba7cf903..b40c3113b14b 100644
> --- a/Documentation/virt/kvm/api.rst
> +++ b/Documentation/virt/kvm/api.rst
> @@ -7844,6 +7844,17 @@ At this time, KVM_PMU_CAP_DISABLE is the only capability.  Setting
>  this capability will disable PMU virtualization for that VM.  Usermode
>  should adjust CPUID leaf 0xA to reflect that the PMU is disabled.
>  
> +8.36 KVM_CAP_VM_DISABLE_NX_HUGE_PAGES
> +---------------------------
> +
> +:Capability KVM_CAP_PMU_CAPABILITY
> +:Architectures: x86
> +:Type: vm
> +
> +This capability disables the NX huge pages mitigation for iTLB MULTIHIT.
> +
> +The capability has no effect if the nx_huge_pages module parameter is not set.
> +
>  9. Known KVM API problems
>  =========================
>  
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 676705ad1e23..dcff7709444d 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -1246,6 +1246,8 @@ struct kvm_arch {
>  	hpa_t	hv_root_tdp;
>  	spinlock_t hv_root_tdp_lock;
>  #endif
> +
> +	bool disable_nx_huge_pages;
>  };
>  
>  struct kvm_vm_stat {
> diff --git a/arch/x86/kvm/mmu.h b/arch/x86/kvm/mmu.h
> index e6cae6f22683..69cffc86b888 100644
> --- a/arch/x86/kvm/mmu.h
> +++ b/arch/x86/kvm/mmu.h
> @@ -173,10 +173,12 @@ struct kvm_page_fault {
>  int kvm_tdp_page_fault(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault);
>  
>  extern int nx_huge_pages;
> -static inline bool is_nx_huge_page_enabled(void)
> +static inline bool is_nx_huge_page_enabled(struct kvm *kvm)
>  {
> -	return READ_ONCE(nx_huge_pages);
> +	return READ_ONCE(nx_huge_pages) &&
> +	       !kvm->arch.disable_nx_huge_pages;
>  }
> +void kvm_update_nx_huge_pages(struct kvm *kvm);
>  
>  static inline int kvm_mmu_do_page_fault(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa,
>  					u32 err, bool prefetch)
> @@ -191,8 +193,8 @@ static inline int kvm_mmu_do_page_fault(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa,
>  		.user = err & PFERR_USER_MASK,
>  		.prefetch = prefetch,
>  		.is_tdp = likely(vcpu->arch.mmu->page_fault == kvm_tdp_page_fault),
> -		.nx_huge_page_workaround_enabled = is_nx_huge_page_enabled(),
> -
> +		.nx_huge_page_workaround_enabled =
> +			is_nx_huge_page_enabled(vcpu->kvm),
>  		.max_level = KVM_MAX_HUGEPAGE_LEVEL,
>  		.req_level = PG_LEVEL_4K,
>  		.goal_level = PG_LEVEL_4K,
> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> index af428cb65b3f..eb7b935d3caa 100644
> --- a/arch/x86/kvm/mmu/mmu.c
> +++ b/arch/x86/kvm/mmu/mmu.c
> @@ -6202,7 +6202,7 @@ static void __set_nx_huge_pages(bool val)
>  	nx_huge_pages = itlb_multihit_kvm_mitigation = val;
>  }
>  
> -static void kvm_update_nx_huge_pages(struct kvm *kvm)
> +void kvm_update_nx_huge_pages(struct kvm *kvm)
>  {
>  	mutex_lock(&kvm->slots_lock);
>  	kvm_mmu_zap_all_fast(kvm);
> diff --git a/arch/x86/kvm/mmu/spte.c b/arch/x86/kvm/mmu/spte.c
> index 4739b53c9734..877ad30bc7ad 100644
> --- a/arch/x86/kvm/mmu/spte.c
> +++ b/arch/x86/kvm/mmu/spte.c
> @@ -116,7 +116,7 @@ bool make_spte(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp,
>  		spte |= spte_shadow_accessed_mask(spte);
>  
>  	if (level > PG_LEVEL_4K && (pte_access & ACC_EXEC_MASK) &&
> -	    is_nx_huge_page_enabled()) {
> +	    is_nx_huge_page_enabled(vcpu->kvm)) {
>  		pte_access &= ~ACC_EXEC_MASK;
>  	}
>  
> @@ -215,7 +215,8 @@ static u64 make_spte_executable(u64 spte)
>   * This is used during huge page splitting to build the SPTEs that make up the
>   * new page table.
>   */
> -u64 make_huge_page_split_spte(u64 huge_spte, int huge_level, int index)
> +u64 make_huge_page_split_spte(struct kvm *kvm, u64 huge_spte, int huge_level,
> +			      int index)
>  {
>  	u64 child_spte;
>  	int child_level;
> @@ -243,7 +244,7 @@ u64 make_huge_page_split_spte(u64 huge_spte, int huge_level, int index)
>  		 * When splitting to a 4K page, mark the page executable as the
>  		 * NX hugepage mitigation no longer applies.
>  		 */
> -		if (is_nx_huge_page_enabled())
> +		if (is_nx_huge_page_enabled(kvm))
>  			child_spte = make_spte_executable(child_spte);
>  	}
>  
> diff --git a/arch/x86/kvm/mmu/spte.h b/arch/x86/kvm/mmu/spte.h
> index 73f12615416f..e4142caff4b1 100644
> --- a/arch/x86/kvm/mmu/spte.h
> +++ b/arch/x86/kvm/mmu/spte.h
> @@ -415,7 +415,8 @@ bool make_spte(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp,
>  	       unsigned int pte_access, gfn_t gfn, kvm_pfn_t pfn,
>  	       u64 old_spte, bool prefetch, bool can_unsync,
>  	       bool host_writable, u64 *new_spte);
> -u64 make_huge_page_split_spte(u64 huge_spte, int huge_level, int index);
> +u64 make_huge_page_split_spte(struct kvm *kvm, u64 huge_spte, int huge_level,
> +			      int index);
>  u64 make_nonleaf_spte(u64 *child_pt, bool ad_disabled);
>  u64 make_mmio_spte(struct kvm_vcpu *vcpu, u64 gfn, unsigned int access);
>  u64 mark_spte_for_access_track(u64 spte);
> diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c
> index a2f9a34a0168..5d82a54924e6 100644
> --- a/arch/x86/kvm/mmu/tdp_mmu.c
> +++ b/arch/x86/kvm/mmu/tdp_mmu.c
> @@ -1469,7 +1469,8 @@ static int tdp_mmu_split_huge_page(struct kvm *kvm, struct tdp_iter *iter,
>  	 * not been linked in yet and thus is not reachable from any other CPU.
>  	 */
>  	for (i = 0; i < PT64_ENT_PER_PAGE; i++)
> -		sp->spt[i] = make_huge_page_split_spte(huge_spte, level, i);
> +		sp->spt[i] = make_huge_page_split_spte(kvm, huge_spte,
> +						       level, i);
>  
>  	/*
>  	 * Replace the huge spte with a pointer to the populated lower level
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 7a066cf92692..ea1d620b35df 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -4268,6 +4268,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
>  	case KVM_CAP_SYS_ATTRIBUTES:
>  	case KVM_CAP_VAPIC:
>  	case KVM_CAP_ENABLE_CAP:
> +	case KVM_CAP_VM_DISABLE_NX_HUGE_PAGES:
>  		r = 1;
>  		break;
>  	case KVM_CAP_EXIT_HYPERCALL:
> @@ -6061,6 +6062,11 @@ int kvm_vm_ioctl_enable_cap(struct kvm *kvm,
>  		}
>  		mutex_unlock(&kvm->lock);
>  		break;
> +	case KVM_CAP_VM_DISABLE_NX_HUGE_PAGES:
> +		kvm->arch.disable_nx_huge_pages = true;
> +		kvm_update_nx_huge_pages(kvm);
> +		r = 0;
> +		break;
>  	default:
>  		r = -EINVAL;
>  		break;
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index 8616af85dc5d..12399c969b42 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -1145,6 +1145,7 @@ struct kvm_ppc_resize_hpt {
>  #define KVM_CAP_PMU_CAPABILITY 212
>  #define KVM_CAP_DISABLE_QUIRKS2 213
>  #define KVM_CAP_VM_TSC_CONTROL 214
> +#define KVM_CAP_VM_DISABLE_NX_HUGE_PAGES 215
>  
>  #ifdef KVM_CAP_IRQ_ROUTING
>  
> -- 
> 2.35.1.1021.g381101b075-goog
> 

[PATCH v3 09/11] KVM: x86: Fix errant brace in KVM capability handling

[Ben Gardon]

The braces around the KVM_CAP_XSAVE2 block also surround the
KVM_CAP_PMU_CAPABILITY block, likely the result of a merge issue. Simply
move the curly brace back to where it belongs.

Fixes: ba7bb663f5547 ("KVM: x86: Provide per VM capability for disabling PMU virtualization")
Signed-off-by: Ben Gardon <bgardon@google.com>
---
 arch/x86/kvm/x86.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index ea1d620b35df..e00dcf19f826 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -4365,10 +4365,10 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
 		if (r < sizeof(struct kvm_xsave))
 			r = sizeof(struct kvm_xsave);
 		break;
+	}
 	case KVM_CAP_PMU_CAPABILITY:
 		r = enable_pmu ? KVM_CAP_PMU_VALID_MASK : 0;
 		break;
-	}
 	case KVM_CAP_DISABLE_QUIRKS2:
 		r = KVM_X86_VALID_QUIRKS;
 		break;
-- 
2.35.1.1021.g381101b075-goog

[PATCH v3 10/11] KVM: x86/MMU: Require reboot permission to disable NX hugepages

[Ben Gardon]

Ensure that the userspace actor attempting to disable NX hugepages has
permission to reboot the system. Since disabling NX hugepages would
allow a guest to crash the system, it is similar to reboot permissions.

This approach is the simplest permission gating, but passing a file
descriptor opened for write for the module parameter would also work
well and be more precise.
The latter approach was suggested by Sean Christopherson.

Suggested-by: Jim Mattson <jmattson@google.com>
Signed-off-by: Ben Gardon <bgardon@google.com>
---
 Documentation/virt/kvm/api.rst | 2 ++
 arch/x86/kvm/x86.c             | 9 +++++++++
 2 files changed, 11 insertions(+)

diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
index b40c3113b14b..ca5674e04474 100644
--- a/Documentation/virt/kvm/api.rst
+++ b/Documentation/virt/kvm/api.rst
@@ -7850,6 +7850,8 @@ should adjust CPUID leaf 0xA to reflect that the PMU is disabled.
 :Capability KVM_CAP_PMU_CAPABILITY
 :Architectures: x86
 :Type: vm
+:Returns 0 on success, -EPERM if the userspace process does not
+	 have CAP_SYS_BOOT
 
 This capability disables the NX huge pages mitigation for iTLB MULTIHIT.
 
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index e00dcf19f826..81e7d825639e 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -6063,6 +6063,15 @@ int kvm_vm_ioctl_enable_cap(struct kvm *kvm,
 		mutex_unlock(&kvm->lock);
 		break;
 	case KVM_CAP_VM_DISABLE_NX_HUGE_PAGES:
+		/*
+		 * Since the risk of disabling NX hugepages is a guest crashing
+		 * the system, ensure the userspace process has permission to
+		 * reboot the system.
+		 */
+		if (!capable(CAP_SYS_BOOT)) {
+			r = -EPERM;
+			break;
+		}
 		kvm->arch.disable_nx_huge_pages = true;
 		kvm_update_nx_huge_pages(kvm);
 		r = 0;
-- 
2.35.1.1021.g381101b075-goog

Re: [PATCH v3 10/11] KVM: x86/MMU: Require reboot permission to disable NX hugepages

[Sean Christopherson]

On Wed, Mar 30, 2022, Ben Gardon wrote:
> Ensure that the userspace actor attempting to disable NX hugepages has
> permission to reboot the system. Since disabling NX hugepages would
> allow a guest to crash the system, it is similar to reboot permissions.

This patch needs to be squashed with the patch that introduces the capability,
otherwise you're introdcuing a bug and then fixing it in the same series.

Re: [PATCH v3 10/11] KVM: x86/MMU: Require reboot permission to disable NX hugepages

[Ben Gardon]

On Wed, Mar 30, 2022 at 11:02 AM Sean Christopherson <seanjc@google.com> wrote:
>
> On Wed, Mar 30, 2022, Ben Gardon wrote:
> > Ensure that the userspace actor attempting to disable NX hugepages has
> > permission to reboot the system. Since disabling NX hugepages would
> > allow a guest to crash the system, it is similar to reboot permissions.
>
> This patch needs to be squashed with the patch that introduces the capability,
> otherwise you're introdcuing a bug and then fixing it in the same series.

I'm happy to do that. I figured that leaving it as a separate patch
would provide an easy way to separate the discussion on how to
implement the new cap versus how to restrict it.
I don't know if I'd consider not having this patch a bug, especially
since it can make testing kind of a pain, but I see your point.

[PATCH v3 11/11] selftests: KVM: Test disabling NX hugepages on a VM

[Ben Gardon]

Add an argument to the NX huge pages test to test disabling the feature
on a VM using the new capability.

Signed-off-by: Ben Gardon <bgardon@google.com>
---
 .../selftests/kvm/include/kvm_util_base.h     |  2 +
 tools/testing/selftests/kvm/lib/kvm_util.c    |  7 ++
 .../selftests/kvm/x86_64/nx_huge_pages_test.c | 67 ++++++++++++++++---
 .../kvm/x86_64/nx_huge_pages_test.sh          |  2 +-
 4 files changed, 66 insertions(+), 12 deletions(-)

diff --git a/tools/testing/selftests/kvm/include/kvm_util_base.h b/tools/testing/selftests/kvm/include/kvm_util_base.h
index 72163ba2f878..4db8251c3ce5 100644
--- a/tools/testing/selftests/kvm/include/kvm_util_base.h
+++ b/tools/testing/selftests/kvm/include/kvm_util_base.h
@@ -411,4 +411,6 @@ uint64_t vm_get_single_stat(struct kvm_vm *vm, const char *stat_name);
 
 uint32_t guest_get_vcpuid(void);
 
+void vm_disable_nx_huge_pages(struct kvm_vm *vm);
+
 #endif /* SELFTEST_KVM_UTIL_BASE_H */
diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
index 9d72d1bb34fa..46a7fa08d3e0 100644
--- a/tools/testing/selftests/kvm/lib/kvm_util.c
+++ b/tools/testing/selftests/kvm/lib/kvm_util.c
@@ -2765,3 +2765,10 @@ uint64_t vm_get_single_stat(struct kvm_vm *vm, const char *stat_name)
 	return value;
 }
 
+void vm_disable_nx_huge_pages(struct kvm_vm *vm)
+{
+	struct kvm_enable_cap cap = { 0 };
+
+	cap.cap = KVM_CAP_VM_DISABLE_NX_HUGE_PAGES;
+	vm_enable_cap(vm, &cap);
+}
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 2bcbe4efdc6a..a0c79f6ddc08 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
@@ -13,6 +13,8 @@
 #include <fcntl.h>
 #include <stdint.h>
 #include <time.h>
+#include <linux/reboot.h>
+#include <sys/syscall.h>
 
 #include <test_util.h>
 #include "kvm_util.h"
@@ -57,13 +59,56 @@ static void check_split_count(struct kvm_vm *vm, int expected_splits)
 		    expected_splits, actual_splits);
 }
 
+static void help(void)
+{
+	puts("");
+	printf("usage: nx_huge_pages_test.sh [-x]\n");
+	puts("");
+	printf(" -x: Allow executable huge pages on the VM.\n");
+	puts("");
+	exit(0);
+}
+
 int main(int argc, char **argv)
 {
 	struct kvm_vm *vm;
 	struct timespec ts;
+	bool disable_nx = false;
+	int opt;
+	int r;
+
+	while ((opt = getopt(argc, argv, "x")) != -1) {
+		switch (opt) {
+		case 'x':
+			disable_nx = true;
+			break;
+		case 'h':
+		default:
+			help();
+			break;
+		}
+	}
 
 	vm = vm_create(VM_MODE_DEFAULT, DEFAULT_GUEST_PHY_PAGES, O_RDWR);
 
+	if (disable_nx) {
+		/*
+		 * Check if this process has the reboot permissions needed to
+		 * disable NX huge pages on a VM.
+		 *
+		 * The reboot call below will never have any effect because
+		 * the magic values are not set correctly, however the
+		 * permission check is done before the magic value check.
+		 */
+		r = syscall(SYS_reboot, 0, 0, 0, NULL);
+		if (r == -EPERM)
+			return KSFT_SKIP;
+		TEST_ASSERT(r == -EINVAL,
+			    "Reboot syscall should fail with -EINVAL");
+
+		vm_disable_nx_huge_pages(vm);
+	}
+
 	vm_userspace_mem_region_add(vm, VM_MEM_SRC_ANONYMOUS_HUGETLB,
 				    HPAGE_PADDR_START, HPAGE_SLOT,
 				    HPAGE_SLOT_NPAGES, 0);
@@ -83,21 +128,21 @@ int main(int argc, char **argv)
 	 * at 2M.
 	 */
 	run_guest_code(vm, guest_code0);
-	check_2m_page_count(vm, 2);
-	check_split_count(vm, 2);
+	check_2m_page_count(vm, disable_nx ? 4 : 2);
+	check_split_count(vm, disable_nx ? 0 : 2);
 
 	/*
 	 * guest_code1 is in the same huge page as data1, so it will cause
 	 * that huge page to be remapped at 4k.
 	 */
 	run_guest_code(vm, guest_code1);
-	check_2m_page_count(vm, 1);
-	check_split_count(vm, 3);
+	check_2m_page_count(vm, disable_nx ? 4 : 1);
+	check_split_count(vm, disable_nx ? 0 : 3);
 
 	/* Run guest_code0 again to check that is has no effect. */
 	run_guest_code(vm, guest_code0);
-	check_2m_page_count(vm, 1);
-	check_split_count(vm, 3);
+	check_2m_page_count(vm, disable_nx ? 4 : 1);
+	check_split_count(vm, disable_nx ? 0 : 3);
 
 	/*
 	 * Give recovery thread time to run. The wrapper script sets
@@ -110,7 +155,7 @@ int main(int argc, char **argv)
 	/*
 	 * Now that the reclaimer has run, all the split pages should be gone.
 	 */
-	check_2m_page_count(vm, 1);
+	check_2m_page_count(vm, disable_nx ? 4 : 1);
 	check_split_count(vm, 0);
 
 	/*
@@ -118,13 +163,13 @@ int main(int argc, char **argv)
 	 * again to check that pages are mapped at 2M again.
 	 */
 	run_guest_code(vm, guest_code0);
-	check_2m_page_count(vm, 2);
-	check_split_count(vm, 2);
+	check_2m_page_count(vm, disable_nx ? 4 : 2);
+	check_split_count(vm, disable_nx ? 0 : 2);
 
 	/* Pages are once again split from running guest_code1. */
 	run_guest_code(vm, guest_code1);
-	check_2m_page_count(vm, 1);
-	check_split_count(vm, 3);
+	check_2m_page_count(vm, disable_nx ? 4 : 1);
+	check_split_count(vm, disable_nx ? 0 : 3);
 
 	kvm_vm_free(vm);
 
diff --git a/tools/testing/selftests/kvm/x86_64/nx_huge_pages_test.sh b/tools/testing/selftests/kvm/x86_64/nx_huge_pages_test.sh
index 19fc95723fcb..29f999f48848 100755
--- a/tools/testing/selftests/kvm/x86_64/nx_huge_pages_test.sh
+++ b/tools/testing/selftests/kvm/x86_64/nx_huge_pages_test.sh
@@ -14,7 +14,7 @@ echo 1 > /sys/module/kvm/parameters/nx_huge_pages_recovery_ratio
 echo 100 > /sys/module/kvm/parameters/nx_huge_pages_recovery_period_ms
 echo 200 > /sys/kernel/mm/hugepages/hugepages-2048kB/nr_hugepages
 
-./nx_huge_pages_test
+./nx_huge_pages_test "${@}"
 RET=$?
 
 echo $NX_HUGE_PAGES > /sys/module/kvm/parameters/nx_huge_pages
-- 
2.35.1.1021.g381101b075-goog

Re: [PATCH v3 11/11] selftests: KVM: Test disabling NX hugepages on a VM

[David Matlack]

On Wed, Mar 30, 2022 at 10:46:21AM -0700, Ben Gardon wrote:
> Add an argument to the NX huge pages test to test disabling the feature
> on a VM using the new capability.
> 
> Signed-off-by: Ben Gardon <bgardon@google.com>
> ---
>  .../selftests/kvm/include/kvm_util_base.h     |  2 +
>  tools/testing/selftests/kvm/lib/kvm_util.c    |  7 ++
>  .../selftests/kvm/x86_64/nx_huge_pages_test.c | 67 ++++++++++++++++---
>  .../kvm/x86_64/nx_huge_pages_test.sh          |  2 +-
>  4 files changed, 66 insertions(+), 12 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/include/kvm_util_base.h b/tools/testing/selftests/kvm/include/kvm_util_base.h
> index 72163ba2f878..4db8251c3ce5 100644
> --- a/tools/testing/selftests/kvm/include/kvm_util_base.h
> +++ b/tools/testing/selftests/kvm/include/kvm_util_base.h
> @@ -411,4 +411,6 @@ uint64_t vm_get_single_stat(struct kvm_vm *vm, const char *stat_name);
>  
>  uint32_t guest_get_vcpuid(void);
>  
> +void vm_disable_nx_huge_pages(struct kvm_vm *vm);
> +
>  #endif /* SELFTEST_KVM_UTIL_BASE_H */
> diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
> index 9d72d1bb34fa..46a7fa08d3e0 100644
> --- a/tools/testing/selftests/kvm/lib/kvm_util.c
> +++ b/tools/testing/selftests/kvm/lib/kvm_util.c
> @@ -2765,3 +2765,10 @@ uint64_t vm_get_single_stat(struct kvm_vm *vm, const char *stat_name)
>  	return value;
>  }
>  
> +void vm_disable_nx_huge_pages(struct kvm_vm *vm)
> +{
> +	struct kvm_enable_cap cap = { 0 };
> +
> +	cap.cap = KVM_CAP_VM_DISABLE_NX_HUGE_PAGES;
> +	vm_enable_cap(vm, &cap);
> +}
> 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 2bcbe4efdc6a..a0c79f6ddc08 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
> @@ -13,6 +13,8 @@
>  #include <fcntl.h>
>  #include <stdint.h>
>  #include <time.h>
> +#include <linux/reboot.h>
> +#include <sys/syscall.h>
>  
>  #include <test_util.h>
>  #include "kvm_util.h"
> @@ -57,13 +59,56 @@ static void check_split_count(struct kvm_vm *vm, int expected_splits)
>  		    expected_splits, actual_splits);
>  }
>  
> +static void help(void)
> +{
> +	puts("");
> +	printf("usage: nx_huge_pages_test.sh [-x]\n");
> +	puts("");
> +	printf(" -x: Allow executable huge pages on the VM.\n");

Making it a flag means we won't exercise it by default. Is there are
reason to avoid exercising KVM_CAP_VM_DISABLE_NX_HUGE_PAGES by default?

Assuming no, I would recommend factoring out the test to a helper
function that takes a parameter that tells it if nx_huge_pages is
enabled or disabled. Then run this helper function multiple times. E.g.
once with nx_huge_pages enabled, once with nx_huge_pages disabled via
KVM_CAP_VM_DISABLE_NX_HUGE_PAGES. This would also then let you test that
disabling via module param also works.

By the way, that brings up another issue. What if NX HugePages is not
enabled on this host? e.g. we're running on AMD, or we're running on a
non-affected Intel host, or we're running on a machine where nx huge
pages has been disabled by the admin? The test should probably return
KSFT_SKIP in those cases.

> +	puts("");
> +	exit(0);
> +}
> +
>  int main(int argc, char **argv)
>  {
>  	struct kvm_vm *vm;
>  	struct timespec ts;
> +	bool disable_nx = false;
> +	int opt;
> +	int r;
> +
> +	while ((opt = getopt(argc, argv, "x")) != -1) {
> +		switch (opt) {
> +		case 'x':
> +			disable_nx = true;
> +			break;
> +		case 'h':
> +		default:
> +			help();
> +			break;
> +		}
> +	}
>  
>  	vm = vm_create(VM_MODE_DEFAULT, DEFAULT_GUEST_PHY_PAGES, O_RDWR);
>  
> +	if (disable_nx) {
> +		/*
> +		 * Check if this process has the reboot permissions needed to
> +		 * disable NX huge pages on a VM.
> +		 *
> +		 * The reboot call below will never have any effect because
> +		 * the magic values are not set correctly, however the
> +		 * permission check is done before the magic value check.
> +		 */
> +		r = syscall(SYS_reboot, 0, 0, 0, NULL);
> +		if (r == -EPERM)
> +			return KSFT_SKIP;
> +		TEST_ASSERT(r == -EINVAL,
> +			    "Reboot syscall should fail with -EINVAL");

Just check if KVM_CAP_VM_DISABLE_NX_HUGE_PAGES returns -EPERM?

> +
> +		vm_disable_nx_huge_pages(vm);
> +	}
> +
>  	vm_userspace_mem_region_add(vm, VM_MEM_SRC_ANONYMOUS_HUGETLB,
>  				    HPAGE_PADDR_START, HPAGE_SLOT,
>  				    HPAGE_SLOT_NPAGES, 0);
> @@ -83,21 +128,21 @@ int main(int argc, char **argv)
>  	 * at 2M.
>  	 */
>  	run_guest_code(vm, guest_code0);
> -	check_2m_page_count(vm, 2);
> -	check_split_count(vm, 2);
> +	check_2m_page_count(vm, disable_nx ? 4 : 2);
> +	check_split_count(vm, disable_nx ? 0 : 2);
>  
>  	/*
>  	 * guest_code1 is in the same huge page as data1, so it will cause
>  	 * that huge page to be remapped at 4k.
>  	 */
>  	run_guest_code(vm, guest_code1);
> -	check_2m_page_count(vm, 1);
> -	check_split_count(vm, 3);
> +	check_2m_page_count(vm, disable_nx ? 4 : 1);
> +	check_split_count(vm, disable_nx ? 0 : 3);
>  
>  	/* Run guest_code0 again to check that is has no effect. */
>  	run_guest_code(vm, guest_code0);
> -	check_2m_page_count(vm, 1);
> -	check_split_count(vm, 3);
> +	check_2m_page_count(vm, disable_nx ? 4 : 1);
> +	check_split_count(vm, disable_nx ? 0 : 3);
>  
>  	/*
>  	 * Give recovery thread time to run. The wrapper script sets
> @@ -110,7 +155,7 @@ int main(int argc, char **argv)
>  	/*
>  	 * Now that the reclaimer has run, all the split pages should be gone.
>  	 */
> -	check_2m_page_count(vm, 1);
> +	check_2m_page_count(vm, disable_nx ? 4 : 1);
>  	check_split_count(vm, 0);
>  
>  	/*
> @@ -118,13 +163,13 @@ int main(int argc, char **argv)
>  	 * again to check that pages are mapped at 2M again.
>  	 */
>  	run_guest_code(vm, guest_code0);
> -	check_2m_page_count(vm, 2);
> -	check_split_count(vm, 2);
> +	check_2m_page_count(vm, disable_nx ? 4 : 2);
> +	check_split_count(vm, disable_nx ? 0 : 2);
>  
>  	/* Pages are once again split from running guest_code1. */
>  	run_guest_code(vm, guest_code1);
> -	check_2m_page_count(vm, 1);
> -	check_split_count(vm, 3);
> +	check_2m_page_count(vm, disable_nx ? 4 : 1);
> +	check_split_count(vm, disable_nx ? 0 : 3);
>  
>  	kvm_vm_free(vm);
>  
> diff --git a/tools/testing/selftests/kvm/x86_64/nx_huge_pages_test.sh b/tools/testing/selftests/kvm/x86_64/nx_huge_pages_test.sh
> index 19fc95723fcb..29f999f48848 100755
> --- a/tools/testing/selftests/kvm/x86_64/nx_huge_pages_test.sh
> +++ b/tools/testing/selftests/kvm/x86_64/nx_huge_pages_test.sh
> @@ -14,7 +14,7 @@ echo 1 > /sys/module/kvm/parameters/nx_huge_pages_recovery_ratio
>  echo 100 > /sys/module/kvm/parameters/nx_huge_pages_recovery_period_ms
>  echo 200 > /sys/kernel/mm/hugepages/hugepages-2048kB/nr_hugepages
>  
> -./nx_huge_pages_test
> +./nx_huge_pages_test "${@}"
>  RET=$?
>  
>  echo $NX_HUGE_PAGES > /sys/module/kvm/parameters/nx_huge_pages
> -- 
> 2.35.1.1021.g381101b075-goog
> 

Re: [PATCH v3 11/11] selftests: KVM: Test disabling NX hugepages on a VM

[Ben Gardon]

On Tue, Apr 5, 2022 at 3:55 PM David Matlack <dmatlack@google.com> wrote:
>
> On Wed, Mar 30, 2022 at 10:46:21AM -0700, Ben Gardon wrote:
> > Add an argument to the NX huge pages test to test disabling the feature
> > on a VM using the new capability.
> >
> > Signed-off-by: Ben Gardon <bgardon@google.com>
> > ---
> >  .../selftests/kvm/include/kvm_util_base.h     |  2 +
> >  tools/testing/selftests/kvm/lib/kvm_util.c    |  7 ++
> >  .../selftests/kvm/x86_64/nx_huge_pages_test.c | 67 ++++++++++++++++---
> >  .../kvm/x86_64/nx_huge_pages_test.sh          |  2 +-
> >  4 files changed, 66 insertions(+), 12 deletions(-)
> >
> > diff --git a/tools/testing/selftests/kvm/include/kvm_util_base.h b/tools/testing/selftests/kvm/include/kvm_util_base.h
> > index 72163ba2f878..4db8251c3ce5 100644
> > --- a/tools/testing/selftests/kvm/include/kvm_util_base.h
> > +++ b/tools/testing/selftests/kvm/include/kvm_util_base.h
> > @@ -411,4 +411,6 @@ uint64_t vm_get_single_stat(struct kvm_vm *vm, const char *stat_name);
> >
> >  uint32_t guest_get_vcpuid(void);
> >
> > +void vm_disable_nx_huge_pages(struct kvm_vm *vm);
> > +
> >  #endif /* SELFTEST_KVM_UTIL_BASE_H */
> > diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
> > index 9d72d1bb34fa..46a7fa08d3e0 100644
> > --- a/tools/testing/selftests/kvm/lib/kvm_util.c
> > +++ b/tools/testing/selftests/kvm/lib/kvm_util.c
> > @@ -2765,3 +2765,10 @@ uint64_t vm_get_single_stat(struct kvm_vm *vm, const char *stat_name)
> >       return value;
> >  }
> >
> > +void vm_disable_nx_huge_pages(struct kvm_vm *vm)
> > +{
> > +     struct kvm_enable_cap cap = { 0 };
> > +
> > +     cap.cap = KVM_CAP_VM_DISABLE_NX_HUGE_PAGES;
> > +     vm_enable_cap(vm, &cap);
> > +}
> > 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 2bcbe4efdc6a..a0c79f6ddc08 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
> > @@ -13,6 +13,8 @@
> >  #include <fcntl.h>
> >  #include <stdint.h>
> >  #include <time.h>
> > +#include <linux/reboot.h>
> > +#include <sys/syscall.h>
> >
> >  #include <test_util.h>
> >  #include "kvm_util.h"
> > @@ -57,13 +59,56 @@ static void check_split_count(struct kvm_vm *vm, int expected_splits)
> >                   expected_splits, actual_splits);
> >  }
> >
> > +static void help(void)
> > +{
> > +     puts("");
> > +     printf("usage: nx_huge_pages_test.sh [-x]\n");
> > +     puts("");
> > +     printf(" -x: Allow executable huge pages on the VM.\n");
>
> Making it a flag means we won't exercise it by default. Is there are
> reason to avoid exercising KVM_CAP_VM_DISABLE_NX_HUGE_PAGES by default?
>
> Assuming no, I would recommend factoring out the test to a helper
> function that takes a parameter that tells it if nx_huge_pages is
> enabled or disabled. Then run this helper function multiple times. E.g.
> once with nx_huge_pages enabled, once with nx_huge_pages disabled via
> KVM_CAP_VM_DISABLE_NX_HUGE_PAGES. This would also then let you test that
> disabling via module param also works.
>
> By the way, that brings up another issue. What if NX HugePages is not
> enabled on this host? e.g. we're running on AMD, or we're running on a
> non-affected Intel host, or we're running on a machine where nx huge
> pages has been disabled by the admin? The test should probably return
> KSFT_SKIP in those cases.

That's all a good idea. Will do.

>
> > +     puts("");
> > +     exit(0);
> > +}
> > +
> >  int main(int argc, char **argv)
> >  {
> >       struct kvm_vm *vm;
> >       struct timespec ts;
> > +     bool disable_nx = false;
> > +     int opt;
> > +     int r;
> > +
> > +     while ((opt = getopt(argc, argv, "x")) != -1) {
> > +             switch (opt) {
> > +             case 'x':
> > +                     disable_nx = true;
> > +                     break;
> > +             case 'h':
> > +             default:
> > +                     help();
> > +                     break;
> > +             }
> > +     }
> >
> >       vm = vm_create(VM_MODE_DEFAULT, DEFAULT_GUEST_PHY_PAGES, O_RDWR);
> >
> > +     if (disable_nx) {
> > +             /*
> > +              * Check if this process has the reboot permissions needed to
> > +              * disable NX huge pages on a VM.
> > +              *
> > +              * The reboot call below will never have any effect because
> > +              * the magic values are not set correctly, however the
> > +              * permission check is done before the magic value check.
> > +              */
> > +             r = syscall(SYS_reboot, 0, 0, 0, NULL);
> > +             if (r == -EPERM)
> > +                     return KSFT_SKIP;
> > +             TEST_ASSERT(r == -EINVAL,
> > +                         "Reboot syscall should fail with -EINVAL");
>
> Just check if KVM_CAP_VM_DISABLE_NX_HUGE_PAGES returns -EPERM?
>
> > +
> > +             vm_disable_nx_huge_pages(vm);
> > +     }
> > +
> >       vm_userspace_mem_region_add(vm, VM_MEM_SRC_ANONYMOUS_HUGETLB,
> >                                   HPAGE_PADDR_START, HPAGE_SLOT,
> >                                   HPAGE_SLOT_NPAGES, 0);
> > @@ -83,21 +128,21 @@ int main(int argc, char **argv)
> >        * at 2M.
> >        */
> >       run_guest_code(vm, guest_code0);
> > -     check_2m_page_count(vm, 2);
> > -     check_split_count(vm, 2);
> > +     check_2m_page_count(vm, disable_nx ? 4 : 2);
> > +     check_split_count(vm, disable_nx ? 0 : 2);
> >
> >       /*
> >        * guest_code1 is in the same huge page as data1, so it will cause
> >        * that huge page to be remapped at 4k.
> >        */
> >       run_guest_code(vm, guest_code1);
> > -     check_2m_page_count(vm, 1);
> > -     check_split_count(vm, 3);
> > +     check_2m_page_count(vm, disable_nx ? 4 : 1);
> > +     check_split_count(vm, disable_nx ? 0 : 3);
> >
> >       /* Run guest_code0 again to check that is has no effect. */
> >       run_guest_code(vm, guest_code0);
> > -     check_2m_page_count(vm, 1);
> > -     check_split_count(vm, 3);
> > +     check_2m_page_count(vm, disable_nx ? 4 : 1);
> > +     check_split_count(vm, disable_nx ? 0 : 3);
> >
> >       /*
> >        * Give recovery thread time to run. The wrapper script sets
> > @@ -110,7 +155,7 @@ int main(int argc, char **argv)
> >       /*
> >        * Now that the reclaimer has run, all the split pages should be gone.
> >        */
> > -     check_2m_page_count(vm, 1);
> > +     check_2m_page_count(vm, disable_nx ? 4 : 1);
> >       check_split_count(vm, 0);
> >
> >       /*
> > @@ -118,13 +163,13 @@ int main(int argc, char **argv)
> >        * again to check that pages are mapped at 2M again.
> >        */
> >       run_guest_code(vm, guest_code0);
> > -     check_2m_page_count(vm, 2);
> > -     check_split_count(vm, 2);
> > +     check_2m_page_count(vm, disable_nx ? 4 : 2);
> > +     check_split_count(vm, disable_nx ? 0 : 2);
> >
> >       /* Pages are once again split from running guest_code1. */
> >       run_guest_code(vm, guest_code1);
> > -     check_2m_page_count(vm, 1);
> > -     check_split_count(vm, 3);
> > +     check_2m_page_count(vm, disable_nx ? 4 : 1);
> > +     check_split_count(vm, disable_nx ? 0 : 3);
> >
> >       kvm_vm_free(vm);
> >
> > diff --git a/tools/testing/selftests/kvm/x86_64/nx_huge_pages_test.sh b/tools/testing/selftests/kvm/x86_64/nx_huge_pages_test.sh
> > index 19fc95723fcb..29f999f48848 100755
> > --- a/tools/testing/selftests/kvm/x86_64/nx_huge_pages_test.sh
> > +++ b/tools/testing/selftests/kvm/x86_64/nx_huge_pages_test.sh
> > @@ -14,7 +14,7 @@ echo 1 > /sys/module/kvm/parameters/nx_huge_pages_recovery_ratio
> >  echo 100 > /sys/module/kvm/parameters/nx_huge_pages_recovery_period_ms
> >  echo 200 > /sys/kernel/mm/hugepages/hugepages-2048kB/nr_hugepages
> >
> > -./nx_huge_pages_test
> > +./nx_huge_pages_test "${@}"
> >  RET=$?
> >
> >  echo $NX_HUGE_PAGES > /sys/module/kvm/parameters/nx_huge_pages
> > --
> > 2.35.1.1021.g381101b075-goog
> >

Re: [PATCH v3 11/11] selftests: KVM: Test disabling NX hugepages on a VM

[Ben Gardon]

On Thu, Apr 7, 2022 at 11:26 AM Ben Gardon <bgardon@google.com> wrote:
>
> On Tue, Apr 5, 2022 at 3:55 PM David Matlack <dmatlack@google.com> wrote:
> >
> > On Wed, Mar 30, 2022 at 10:46:21AM -0700, Ben Gardon wrote:
> > > Add an argument to the NX huge pages test to test disabling the feature
> > > on a VM using the new capability.
> > >
> > > Signed-off-by: Ben Gardon <bgardon@google.com>
> > > ---
> > >  .../selftests/kvm/include/kvm_util_base.h     |  2 +
> > >  tools/testing/selftests/kvm/lib/kvm_util.c    |  7 ++
> > >  .../selftests/kvm/x86_64/nx_huge_pages_test.c | 67 ++++++++++++++++---
> > >  .../kvm/x86_64/nx_huge_pages_test.sh          |  2 +-
> > >  4 files changed, 66 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/tools/testing/selftests/kvm/include/kvm_util_base.h b/tools/testing/selftests/kvm/include/kvm_util_base.h
> > > index 72163ba2f878..4db8251c3ce5 100644
> > > --- a/tools/testing/selftests/kvm/include/kvm_util_base.h
> > > +++ b/tools/testing/selftests/kvm/include/kvm_util_base.h
> > > @@ -411,4 +411,6 @@ uint64_t vm_get_single_stat(struct kvm_vm *vm, const char *stat_name);
> > >
> > >  uint32_t guest_get_vcpuid(void);
> > >
> > > +void vm_disable_nx_huge_pages(struct kvm_vm *vm);
> > > +
> > >  #endif /* SELFTEST_KVM_UTIL_BASE_H */
> > > diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
> > > index 9d72d1bb34fa..46a7fa08d3e0 100644
> > > --- a/tools/testing/selftests/kvm/lib/kvm_util.c
> > > +++ b/tools/testing/selftests/kvm/lib/kvm_util.c
> > > @@ -2765,3 +2765,10 @@ uint64_t vm_get_single_stat(struct kvm_vm *vm, const char *stat_name)
> > >       return value;
> > >  }
> > >
> > > +void vm_disable_nx_huge_pages(struct kvm_vm *vm)
> > > +{
> > > +     struct kvm_enable_cap cap = { 0 };
> > > +
> > > +     cap.cap = KVM_CAP_VM_DISABLE_NX_HUGE_PAGES;
> > > +     vm_enable_cap(vm, &cap);
> > > +}
> > > 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 2bcbe4efdc6a..a0c79f6ddc08 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
> > > @@ -13,6 +13,8 @@
> > >  #include <fcntl.h>
> > >  #include <stdint.h>
> > >  #include <time.h>
> > > +#include <linux/reboot.h>
> > > +#include <sys/syscall.h>
> > >
> > >  #include <test_util.h>
> > >  #include "kvm_util.h"
> > > @@ -57,13 +59,56 @@ static void check_split_count(struct kvm_vm *vm, int expected_splits)
> > >                   expected_splits, actual_splits);
> > >  }
> > >
> > > +static void help(void)
> > > +{
> > > +     puts("");
> > > +     printf("usage: nx_huge_pages_test.sh [-x]\n");
> > > +     puts("");
> > > +     printf(" -x: Allow executable huge pages on the VM.\n");
> >
> > Making it a flag means we won't exercise it by default. Is there are
> > reason to avoid exercising KVM_CAP_VM_DISABLE_NX_HUGE_PAGES by default?
> >
> > Assuming no, I would recommend factoring out the test to a helper
> > function that takes a parameter that tells it if nx_huge_pages is
> > enabled or disabled. Then run this helper function multiple times. E.g.
> > once with nx_huge_pages enabled, once with nx_huge_pages disabled via
> > KVM_CAP_VM_DISABLE_NX_HUGE_PAGES. This would also then let you test that
> > disabling via module param also works.
> >
> > By the way, that brings up another issue. What if NX HugePages is not
> > enabled on this host? e.g. we're running on AMD, or we're running on a
> > non-affected Intel host, or we're running on a machine where nx huge
> > pages has been disabled by the admin? The test should probably return
> > KSFT_SKIP in those cases.

The wrapper script just always turns nx_huge_pages on, which I think
is a better solution, but perhaps it should check for permission
errors when doing that.

>
> That's all a good idea. Will do.
>
> >
> > > +     puts("");
> > > +     exit(0);
> > > +}
> > > +
> > >  int main(int argc, char **argv)
> > >  {
> > >       struct kvm_vm *vm;
> > >       struct timespec ts;
> > > +     bool disable_nx = false;
> > > +     int opt;
> > > +     int r;
> > > +
> > > +     while ((opt = getopt(argc, argv, "x")) != -1) {
> > > +             switch (opt) {
> > > +             case 'x':
> > > +                     disable_nx = true;
> > > +                     break;
> > > +             case 'h':
> > > +             default:
> > > +                     help();
> > > +                     break;
> > > +             }
> > > +     }
> > >
> > >       vm = vm_create(VM_MODE_DEFAULT, DEFAULT_GUEST_PHY_PAGES, O_RDWR);
> > >
> > > +     if (disable_nx) {
> > > +             /*
> > > +              * Check if this process has the reboot permissions needed to
> > > +              * disable NX huge pages on a VM.
> > > +              *
> > > +              * The reboot call below will never have any effect because
> > > +              * the magic values are not set correctly, however the
> > > +              * permission check is done before the magic value check.
> > > +              */
> > > +             r = syscall(SYS_reboot, 0, 0, 0, NULL);
> > > +             if (r == -EPERM)
> > > +                     return KSFT_SKIP;
> > > +             TEST_ASSERT(r == -EINVAL,
> > > +                         "Reboot syscall should fail with -EINVAL");
> >
> > Just check if KVM_CAP_VM_DISABLE_NX_HUGE_PAGES returns -EPERM?

We could do that but then we wouldn't be checking that the permission
checks work as expected.

> >
> > > +
> > > +             vm_disable_nx_huge_pages(vm);
> > > +     }
> > > +
> > >       vm_userspace_mem_region_add(vm, VM_MEM_SRC_ANONYMOUS_HUGETLB,
> > >                                   HPAGE_PADDR_START, HPAGE_SLOT,
> > >                                   HPAGE_SLOT_NPAGES, 0);
> > > @@ -83,21 +128,21 @@ int main(int argc, char **argv)
> > >        * at 2M.
> > >        */
> > >       run_guest_code(vm, guest_code0);
> > > -     check_2m_page_count(vm, 2);
> > > -     check_split_count(vm, 2);
> > > +     check_2m_page_count(vm, disable_nx ? 4 : 2);
> > > +     check_split_count(vm, disable_nx ? 0 : 2);
> > >
> > >       /*
> > >        * guest_code1 is in the same huge page as data1, so it will cause
> > >        * that huge page to be remapped at 4k.
> > >        */
> > >       run_guest_code(vm, guest_code1);
> > > -     check_2m_page_count(vm, 1);
> > > -     check_split_count(vm, 3);
> > > +     check_2m_page_count(vm, disable_nx ? 4 : 1);
> > > +     check_split_count(vm, disable_nx ? 0 : 3);
> > >
> > >       /* Run guest_code0 again to check that is has no effect. */
> > >       run_guest_code(vm, guest_code0);
> > > -     check_2m_page_count(vm, 1);
> > > -     check_split_count(vm, 3);
> > > +     check_2m_page_count(vm, disable_nx ? 4 : 1);
> > > +     check_split_count(vm, disable_nx ? 0 : 3);
> > >
> > >       /*
> > >        * Give recovery thread time to run. The wrapper script sets
> > > @@ -110,7 +155,7 @@ int main(int argc, char **argv)
> > >       /*
> > >        * Now that the reclaimer has run, all the split pages should be gone.
> > >        */
> > > -     check_2m_page_count(vm, 1);
> > > +     check_2m_page_count(vm, disable_nx ? 4 : 1);
> > >       check_split_count(vm, 0);
> > >
> > >       /*
> > > @@ -118,13 +163,13 @@ int main(int argc, char **argv)
> > >        * again to check that pages are mapped at 2M again.
> > >        */
> > >       run_guest_code(vm, guest_code0);
> > > -     check_2m_page_count(vm, 2);
> > > -     check_split_count(vm, 2);
> > > +     check_2m_page_count(vm, disable_nx ? 4 : 2);
> > > +     check_split_count(vm, disable_nx ? 0 : 2);
> > >
> > >       /* Pages are once again split from running guest_code1. */
> > >       run_guest_code(vm, guest_code1);
> > > -     check_2m_page_count(vm, 1);
> > > -     check_split_count(vm, 3);
> > > +     check_2m_page_count(vm, disable_nx ? 4 : 1);
> > > +     check_split_count(vm, disable_nx ? 0 : 3);
> > >
> > >       kvm_vm_free(vm);
> > >
> > > diff --git a/tools/testing/selftests/kvm/x86_64/nx_huge_pages_test.sh b/tools/testing/selftests/kvm/x86_64/nx_huge_pages_test.sh
> > > index 19fc95723fcb..29f999f48848 100755
> > > --- a/tools/testing/selftests/kvm/x86_64/nx_huge_pages_test.sh
> > > +++ b/tools/testing/selftests/kvm/x86_64/nx_huge_pages_test.sh
> > > @@ -14,7 +14,7 @@ echo 1 > /sys/module/kvm/parameters/nx_huge_pages_recovery_ratio
> > >  echo 100 > /sys/module/kvm/parameters/nx_huge_pages_recovery_period_ms
> > >  echo 200 > /sys/kernel/mm/hugepages/hugepages-2048kB/nr_hugepages
> > >
> > > -./nx_huge_pages_test
> > > +./nx_huge_pages_test "${@}"
> > >  RET=$?
> > >
> > >  echo $NX_HUGE_PAGES > /sys/module/kvm/parameters/nx_huge_pages
> > > --
> > > 2.35.1.1021.g381101b075-goog
> > >