[PATCH v1 0/4] KVM selftests enhancements

[PATCH v1 0/4] KVM selftests enhancements

[Manali Shukla]

This series was prompted by feedback given in [1].
Patch 1   : Adds safe_hlt() and cli() helpers.
Patch 2, 3: Adds an interface to read vcpu stat in selftest.  Adds
            a macro to generate compiler error to detect typos at
            compile time while parsing vcpu and vm stats.
Patch 4   : Fix few of the selftests based on newly defined macro.

This series was split from the Idle HLT intercept support series [2]
because the series has a few changes in the vm_get_stat() interface
as suggested in [1] and a few changes in two of the self-tests
(nx_huge_pages_test.c and dirty_log_page_splitting_test.c) which use
vm_get_stat() functionality to retrieve specified VM stats. These
changes are unrelated to the Idle HLT intercept support series [2].

[1] https://lore.kernel.org/kvm/ZruDweYzQRRcJeTO@google.com/T/#m7cd7a110f0fcff9a03176935ed016f49088df6b0

[2] https://lore.kernel.org/kvm/ZruDweYzQRRcJeTO@google.com/T/#m6c67ca8ccb226e5f357d943da2a2965a4224f69a


Manali Shukla (4):
  KVM: selftests: Add safe_halt() and cli() helpers to common code
  KVM: selftests: Add an interface to read the data of named vcpu stat
  KVM: selftests: convert vm_get_stat to macro
  KVM: selftests: Replace previously used vm_get_stat() to macro

 .../testing/selftests/kvm/include/kvm_util.h  | 83 +++++++++++++++++--
 .../kvm/include/x86_64/kvm_util_arch.h        | 52 ++++++++++++
 .../selftests/kvm/include/x86_64/processor.h  | 17 ++++
 tools/testing/selftests/kvm/lib/kvm_util.c    | 40 +++++++++
 .../x86_64/dirty_log_page_splitting_test.c    |  6 +-
 .../selftests/kvm/x86_64/nx_huge_pages_test.c |  4 +-
 6 files changed, 191 insertions(+), 11 deletions(-)


base-commit: c8d430db8eec7d4fd13a6bea27b7086a54eda6da
-- 
2.34.1

[PATCH v1 1/4] KVM: selftests: Add safe_halt() and cli() helpers to common code

[Manali Shukla]

Add safe_halt() and cli() helpers to processor.h to make them broadly
available in KVM selftests.

Suggested-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Manali Shukla <manali.shukla@amd.com>
---
 .../selftests/kvm/include/x86_64/processor.h    | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/tools/testing/selftests/kvm/include/x86_64/processor.h b/tools/testing/selftests/kvm/include/x86_64/processor.h
index e247f99e0473..8e36de85b68f 100644
--- a/tools/testing/selftests/kvm/include/x86_64/processor.h
+++ b/tools/testing/selftests/kvm/include/x86_64/processor.h
@@ -1325,6 +1325,23 @@ static inline void kvm_hypercall_map_gpa_range(uint64_t gpa, uint64_t size,
 	GUEST_ASSERT(!ret);
 }
 
+/*
+ * Execute HLT in an STI interrupt shadow to ensure that a pending IRQ that's
+ * intended to be a wake event arrives *after* HLT is executed.  Modern CPUs,
+ * except for a few oddballs that KVM is unlikely to run on, block IRQs for one
+ * instruction after STI, *if* RFLAGS.IF=0 before STI.  Note, Intel CPUs may
+ * block other events beyond regular IRQs, e.g. may block NMIs and SMIs too.
+ */
+static inline void safe_halt(void)
+{
+	asm volatile("sti; hlt");
+}
+
+static inline void cli(void)
+{
+	asm volatile ("cli");
+}
+
 void __vm_xsave_require_permission(uint64_t xfeature, const char *name);
 
 #define vm_xsave_require_permission(xfeature)	\

base-commit: c8d430db8eec7d4fd13a6bea27b7086a54eda6da
-- 
2.34.1

Re: [PATCH v1 1/4] KVM: selftests: Add safe_halt() and cli() helpers to common code

[Sean Christopherson]

On Mon, Oct 21, 2024, Manali Shukla wrote:
> Add safe_halt() and cli() helpers to processor.h to make them broadly
> available in KVM selftests.
> 
> Suggested-by: Sean Christopherson <seanjc@google.com>
> Signed-off-by: Manali Shukla <manali.shukla@amd.com>
> ---
>  .../selftests/kvm/include/x86_64/processor.h    | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)

The whole point of adding helpers is to use them.  I'll post a patch that adds
all of the helpers and uses them where possible.

[PATCH v1 2/4] KVM: selftests: Add an interface to read the data of named vcpu stat

[Manali Shukla]

From: Manali Shukla <Manali.Shukla@amd.com>

The interface is used to read the data values of a specified vcpu stat
from the currenly available binary stats interface.

Add a concatenation trickery to trigger compiler error if the stat
doesn't exist, so that it is not possible to pass a per-VM stat into
vcpu_get_stat().

Suggested-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Manali Shukla <Manali.Shukla@amd.com>
---
 .../testing/selftests/kvm/include/kvm_util.h  | 52 +++++++++++++++++++
 .../kvm/include/x86_64/kvm_util_arch.h        | 36 +++++++++++++
 tools/testing/selftests/kvm/lib/kvm_util.c    | 40 ++++++++++++++
 3 files changed, 128 insertions(+)

diff --git a/tools/testing/selftests/kvm/include/kvm_util.h b/tools/testing/selftests/kvm/include/kvm_util.h
index bc7c242480d6..5dd3acf174f8 100644
--- a/tools/testing/selftests/kvm/include/kvm_util.h
+++ b/tools/testing/selftests/kvm/include/kvm_util.h
@@ -531,6 +531,14 @@ void read_stat_data(int stats_fd, struct kvm_stats_header *header,
 		    struct kvm_stats_desc *desc, uint64_t *data,
 		    size_t max_elements);
 
+#define DEFINE_CHECK_STAT(type, stat)			\
+static inline int check_##type##_##stat##_exists(void)	\
+{							\
+	return 1;					\
+}							\
+
+#define STAT_EXISTS(type, stat) (check_##type##_##stat##_exists())
+
 void __vm_get_stat(struct kvm_vm *vm, const char *stat_name, uint64_t *data,
 		   size_t max_elements);
 
@@ -542,6 +550,50 @@ static inline uint64_t vm_get_stat(struct kvm_vm *vm, const char *stat_name)
 	return data;
 }
 
+#define DEFINE_GENERIC_VCPU_STAT				\
+	DEFINE_CHECK_STAT(vcpu, halt_successfull_poll)		\
+	DEFINE_CHECK_STAT(vcpu, halt_attempted_poll)		\
+	DEFINE_CHECK_STAT(vcpu, halt_poll_invalid)		\
+	DEFINE_CHECK_STAT(vcpu, halt_wakeup)			\
+	DEFINE_CHECK_STAT(vcpu, halt_poll_success_ns)		\
+	DEFINE_CHECK_STAT(vcpu, halt_poll_fail_ns)		\
+	DEFINE_CHECK_STAT(vcpu, halt_wait_ns)			\
+	DEFINE_CHECK_STAT(vcpu, halt_poll_success_hist)		\
+	DEFINE_CHECK_STAT(vcpu, halt_poll_fail_hist)		\
+	DEFINE_CHECK_STAT(vcpu, halt_wait_hist)			\
+	DEFINE_CHECK_STAT(vcpu, blocking)			\
+
+/*
+ * Define a default empty macro for architectures which do not specify
+ * arch specific vcpu stats
+ */
+
+#ifndef DEFINE_ARCH_VCPU_STAT
+#define DEFINE_ARCH_VCPU_STAT
+#endif
+
+DEFINE_GENERIC_VCPU_STAT
+DEFINE_ARCH_VCPU_STAT
+
+#undef DEFINE_CHECK_STAT
+#undef DEFINE_GENERIC_VCPU_STAT
+#undef DEFINE_ARCH_VCPU_STAT
+
+void __vcpu_get_stat(struct kvm_vcpu *vcpu, const char *stat_name, uint64_t *data,
+		   size_t max_elements);
+
+#define vcpu_get_stat(vcpu, stat_name)				\
+({								\
+	uint64_t data;						\
+								\
+	STAT_EXISTS(vcpu, stat_name);				\
+	__vcpu_get_stat(vcpu, #stat_name, &data, 1);		\
+	data;							\
+})								\
+
+#undef DEFINE_CHECK_STAT
+#undef DEFINE_GENERIC_VCPU_STAT
+
 void vm_create_irqchip(struct kvm_vm *vm);
 
 static inline int __vm_create_guest_memfd(struct kvm_vm *vm, uint64_t size,
diff --git a/tools/testing/selftests/kvm/include/x86_64/kvm_util_arch.h b/tools/testing/selftests/kvm/include/x86_64/kvm_util_arch.h
index 972bb1c4ab4c..3cdc3c856ed2 100644
--- a/tools/testing/selftests/kvm/include/x86_64/kvm_util_arch.h
+++ b/tools/testing/selftests/kvm/include/x86_64/kvm_util_arch.h
@@ -48,4 +48,40 @@ do {											\
 	}										\
 } while (0)
 
+#define DEFINE_ARCH_VCPU_STAT					\
+	DEFINE_CHECK_STAT(vcpu, pf_taken)			\
+	DEFINE_CHECK_STAT(vcpu, pf_fixed)			\
+	DEFINE_CHECK_STAT(vcpu, pf_emulate)			\
+	DEFINE_CHECK_STAT(vcpu, pf_spurious)			\
+	DEFINE_CHECK_STAT(vcpu, pf_fast)			\
+	DEFINE_CHECK_STAT(vcpu, pf_mmio_spte_created)		\
+	DEFINE_CHECK_STAT(vcpu, pf_guest)			\
+	DEFINE_CHECK_STAT(vcpu, tlb_flush)			\
+	DEFINE_CHECK_STAT(vcpu, invlpg)				\
+	DEFINE_CHECK_STAT(vcpu, exits)				\
+	DEFINE_CHECK_STAT(vcpu, io_exits)			\
+	DEFINE_CHECK_STAT(vcpu, mmio_exits)			\
+	DEFINE_CHECK_STAT(vcpu, signal_exits)			\
+	DEFINE_CHECK_STAT(vcpu, irq_window_exits)		\
+	DEFINE_CHECK_STAT(vcpu, nmi_window_exits)		\
+	DEFINE_CHECK_STAT(vcpu, l1d_flush)			\
+	DEFINE_CHECK_STAT(vcpu, halt_exits)			\
+	DEFINE_CHECK_STAT(vcpu, request_irq_exits)		\
+	DEFINE_CHECK_STAT(vcpu, irq_exits)			\
+	DEFINE_CHECK_STAT(vcpu, host_state_reload)		\
+	DEFINE_CHECK_STAT(vcpu, fpu_reload)			\
+	DEFINE_CHECK_STAT(vcpu, insn_emulation)			\
+	DEFINE_CHECK_STAT(vcpu, insn_emulation_fail)		\
+	DEFINE_CHECK_STAT(vcpu, hypercalls)			\
+	DEFINE_CHECK_STAT(vcpu, irq_injections)			\
+	DEFINE_CHECK_STAT(vcpu, nmi_injections)			\
+	DEFINE_CHECK_STAT(vcpu, req_event)			\
+	DEFINE_CHECK_STAT(vcpu, nested_run)			\
+	DEFINE_CHECK_STAT(vcpu, directed_yield_attempted)	\
+	DEFINE_CHECK_STAT(vcpu, directed_yield_successful)	\
+	DEFINE_CHECK_STAT(vcpu, preemption_reported)		\
+	DEFINE_CHECK_STAT(vcpu, preemption_other)		\
+	DEFINE_CHECK_STAT(vcpu, guest_mode)			\
+	DEFINE_CHECK_STAT(vcpu, notify_window_exits)		\
+
 #endif  // SELFTEST_KVM_UTIL_ARCH_H
diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
index a2b7df5f1d39..3ee84e117a04 100644
--- a/tools/testing/selftests/kvm/lib/kvm_util.c
+++ b/tools/testing/selftests/kvm/lib/kvm_util.c
@@ -2195,6 +2195,46 @@ void read_stat_data(int stats_fd, struct kvm_stats_header *header,
 		    desc->name, size, ret);
 }
 
+/*
+ * Read the data of the named vcpu stat
+ *
+ * Input Args:
+ *   vcpu - the vcpu for which the stat should be read
+ *   stat_name - the name of the stat to read
+ *   max_elements - the maximum number of 8-byte values to read into data
+ *
+ * Output Args:
+ *   data - the buffer into which stat data should be read
+ *
+ * Read the data values of a specified stat from the binary stats interface.
+ */
+void __vcpu_get_stat(struct kvm_vcpu *vcpu, const char *stat_name, uint64_t *data,
+		   size_t max_elements)
+{
+	int vcpu_stats_fd;
+	struct kvm_stats_header header;
+	struct kvm_stats_desc *desc, *t_desc;
+	size_t size_desc;
+	int i;
+
+	vcpu_stats_fd = vcpu_get_stats_fd(vcpu);
+	read_stats_header(vcpu_stats_fd, &header);
+
+	desc = read_stats_descriptors(vcpu_stats_fd, &header);
+	size_desc = get_stats_descriptor_size(&header);
+
+	for (i = 0; i < header.num_desc; ++i) {
+		t_desc = (void *)desc + (i * size_desc);
+
+		if (strcmp(t_desc->name, stat_name))
+			continue;
+
+		read_stat_data(vcpu_stats_fd, &header, t_desc,
+			       data, max_elements);
+		break;
+	}
+}
+
 /*
  * Read the data of the named stat
  *
-- 
2.34.1

Re: [PATCH v1 2/4] KVM: selftests: Add an interface to read the data of named vcpu stat

[Sean Christopherson]

On Mon, Oct 21, 2024, Manali Shukla wrote:
> From: Manali Shukla <Manali.Shukla@amd.com>
> 
> The interface is used to read the data values of a specified vcpu stat
> from the currenly available binary stats interface.
> 
> Add a concatenation trickery to trigger compiler error if the stat
> doesn't exist, so that it is not possible to pass a per-VM stat into
> vcpu_get_stat().
> 
> Suggested-by: Sean Christopherson <seanjc@google.com>
> Signed-off-by: Manali Shukla <Manali.Shukla@amd.com>
> ---
>  .../testing/selftests/kvm/include/kvm_util.h  | 52 +++++++++++++++++++
>  .../kvm/include/x86_64/kvm_util_arch.h        | 36 +++++++++++++
>  tools/testing/selftests/kvm/lib/kvm_util.c    | 40 ++++++++++++++
>  3 files changed, 128 insertions(+)
> 
> diff --git a/tools/testing/selftests/kvm/include/kvm_util.h b/tools/testing/selftests/kvm/include/kvm_util.h
> index bc7c242480d6..5dd3acf174f8 100644
> --- a/tools/testing/selftests/kvm/include/kvm_util.h
> +++ b/tools/testing/selftests/kvm/include/kvm_util.h
> @@ -531,6 +531,14 @@ void read_stat_data(int stats_fd, struct kvm_stats_header *header,
>  		    struct kvm_stats_desc *desc, uint64_t *data,
>  		    size_t max_elements);
>  
> +#define DEFINE_CHECK_STAT(type, stat)			\
> +static inline int check_##type##_##stat##_exists(void)	\
> +{							\
> +	return 1;					\
> +}							\
> +
> +#define STAT_EXISTS(type, stat) (check_##type##_##stat##_exists())

This is all unnecessary complicated.  To trigger a compilation error, the set
of knnown stats just needs to be defined as _something_ and then referenced.
There's no need for layers of macros and a function for each stat.  The fact that
a stat is defined is proof of its existence.

> +
>  void __vm_get_stat(struct kvm_vm *vm, const char *stat_name, uint64_t *data,
>  		   size_t max_elements);
>  
> @@ -542,6 +550,50 @@ static inline uint64_t vm_get_stat(struct kvm_vm *vm, const char *stat_name)
>  	return data;
>  }
>  
> +#define DEFINE_GENERIC_VCPU_STAT				\
> +	DEFINE_CHECK_STAT(vcpu, halt_successfull_poll)		\
> +	DEFINE_CHECK_STAT(vcpu, halt_attempted_poll)		\
> +	DEFINE_CHECK_STAT(vcpu, halt_poll_invalid)		\
> +	DEFINE_CHECK_STAT(vcpu, halt_wakeup)			\
> +	DEFINE_CHECK_STAT(vcpu, halt_poll_success_ns)		\
> +	DEFINE_CHECK_STAT(vcpu, halt_poll_fail_ns)		\
> +	DEFINE_CHECK_STAT(vcpu, halt_wait_ns)			\
> +	DEFINE_CHECK_STAT(vcpu, halt_poll_success_hist)		\
> +	DEFINE_CHECK_STAT(vcpu, halt_poll_fail_hist)		\
> +	DEFINE_CHECK_STAT(vcpu, halt_wait_hist)			\
> +	DEFINE_CHECK_STAT(vcpu, blocking)			\
> +
> +/*
> + * Define a default empty macro for architectures which do not specify
> + * arch specific vcpu stats
> + */
> +
> +#ifndef DEFINE_ARCH_VCPU_STAT
> +#define DEFINE_ARCH_VCPU_STAT
> +#endif
> +
> +DEFINE_GENERIC_VCPU_STAT

There's also no need to define macros in arch code just to expand them in common
code.  Add simple macros in kvm_util_types.h and this goes away.

> +DEFINE_ARCH_VCPU_STAT
> +
> +#undef DEFINE_CHECK_STAT
> +#undef DEFINE_GENERIC_VCPU_STAT
> +#undef DEFINE_ARCH_VCPU_STAT

> +void __vcpu_get_stat(struct kvm_vcpu *vcpu, const char *stat_name, uint64_t *data,
> +		   size_t max_elements);
> +
> +#define vcpu_get_stat(vcpu, stat_name)				\
> +({								\
> +	uint64_t data;						\
> +								\
> +	STAT_EXISTS(vcpu, stat_name);				\
> +	__vcpu_get_stat(vcpu, #stat_name, &data, 1);		\
> +	data;							\
> +})								\
> +
> +#undef DEFINE_CHECK_STAT
> +#undef DEFINE_GENERIC_VCPU_STAT
> +

...

> +void __vcpu_get_stat(struct kvm_vcpu *vcpu, const char *stat_name, uint64_t *data,
> +		   size_t max_elements)
> +{
> +	int vcpu_stats_fd;
> +	struct kvm_stats_header header;
> +	struct kvm_stats_desc *desc, *t_desc;
> +	size_t size_desc;
> +	int i;
> +
> +	vcpu_stats_fd = vcpu_get_stats_fd(vcpu);
> +	read_stats_header(vcpu_stats_fd, &header);
> +
> +	desc = read_stats_descriptors(vcpu_stats_fd, &header);
> +	size_desc = get_stats_descriptor_size(&header);
> +
> +	for (i = 0; i < header.num_desc; ++i) {
> +		t_desc = (void *)desc + (i * size_desc);
> +
> +		if (strcmp(t_desc->name, stat_name))
> +			continue;
> +
> +		read_stat_data(vcpu_stats_fd, &header, t_desc,
> +			       data, max_elements);
> +		break;
> +	}
> +}

This is copy-pasted nearly verbatim from the VM-scoped code.  It even has the
same bugs (doesn't assert the stat exists), along with new bugs (leaks the fd
and header).

It takes a bit of work, but not _that_ much work, to genericize the VM-scoped
infrastructure and reuse it for vCPU-scoped stats.

[PATCH v1 3/4] KVM: selftests: convert vm_get_stat to macro

[Manali Shukla]

Convert vm_get_stat() to macro to detect typos at compile time.

Add a concatenation trickery to trigger compiler error if vm stat
doesn't exist, so that it is not possible to pass a vcpu stat into
vm_get_stat().

Suggested-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Manali Shukla <manali.shukla@amd.com>
---
 .../testing/selftests/kvm/include/kvm_util.h  | 33 +++++++++++++++----
 .../kvm/include/x86_64/kvm_util_arch.h        | 16 +++++++++
 2 files changed, 42 insertions(+), 7 deletions(-)

diff --git a/tools/testing/selftests/kvm/include/kvm_util.h b/tools/testing/selftests/kvm/include/kvm_util.h
index 5dd3acf174f8..bd486a2899ca 100644
--- a/tools/testing/selftests/kvm/include/kvm_util.h
+++ b/tools/testing/selftests/kvm/include/kvm_util.h
@@ -539,16 +539,35 @@ static inline int check_##type##_##stat##_exists(void)	\
 
 #define STAT_EXISTS(type, stat) (check_##type##_##stat##_exists())
 
+#define DEFINE_GENERIC_VM_STAT					\
+	DEFINE_CHECK_STAT(vm, remote_tlb_flush)			\
+	DEFINE_CHECK_STAT(vm, remote_tlb_flush_requests)	\
+
+/*
+ * Define a default empty macro for architectures which do not specify
+ * arch specific vm stats.
+ */
+#ifndef DEFINE_ARCH_VM_STAT
+#define DEFINE_ARCH_VM_STAT
+#endif
+
+DEFINE_GENERIC_VM_STAT
+DEFINE_ARCH_VM_STAT
+
+#undef DEFINE_GENERIC_VM_STAT
+#undef DEFINE_ARCH_VM_STAT
+
 void __vm_get_stat(struct kvm_vm *vm, const char *stat_name, uint64_t *data,
 		   size_t max_elements);
 
-static inline uint64_t vm_get_stat(struct kvm_vm *vm, const char *stat_name)
-{
-	uint64_t data;
-
-	__vm_get_stat(vm, stat_name, &data, 1);
-	return data;
-}
+#define vm_get_stat(vm, stat_name)				\
+({								\
+	uint64_t data;						\
+								\
+	STAT_EXISTS(vm, stat_name);				\
+	__vm_get_stat(vm, #stat_name, &data, 1);		\
+	data;							\
+})
 
 #define DEFINE_GENERIC_VCPU_STAT				\
 	DEFINE_CHECK_STAT(vcpu, halt_successfull_poll)		\
diff --git a/tools/testing/selftests/kvm/include/x86_64/kvm_util_arch.h b/tools/testing/selftests/kvm/include/x86_64/kvm_util_arch.h
index 3cdc3c856ed2..6341c786dc9a 100644
--- a/tools/testing/selftests/kvm/include/x86_64/kvm_util_arch.h
+++ b/tools/testing/selftests/kvm/include/x86_64/kvm_util_arch.h
@@ -48,6 +48,22 @@ do {											\
 	}										\
 } while (0)
 
+#define DEFINE_ARCH_VM_STAT					\
+	DEFINE_CHECK_STAT(vm, mmu_shadow_zapped)		\
+	DEFINE_CHECK_STAT(vm, mmu_pte_write)			\
+	DEFINE_CHECK_STAT(vm, mmu_pde_zapped)			\
+	DEFINE_CHECK_STAT(vm, mmu_flooded)			\
+	DEFINE_CHECK_STAT(vm, mmu_recycled)			\
+	DEFINE_CHECK_STAT(vm, mmu_cache_miss)			\
+	DEFINE_CHECK_STAT(vm, mmu_unsync)			\
+	DEFINE_CHECK_STAT(vm, pages_4k)				\
+	DEFINE_CHECK_STAT(vm, pages_2m)				\
+	DEFINE_CHECK_STAT(vm, pages_1g)				\
+	DEFINE_CHECK_STAT(vm, pages)				\
+	DEFINE_CHECK_STAT(vm, nx_lpage_splits)			\
+	DEFINE_CHECK_STAT(vm, max_mmu_page_hash_collisions)	\
+	DEFINE_CHECK_STAT(vm, max_mmu_rmap_size)		\
+
 #define DEFINE_ARCH_VCPU_STAT					\
 	DEFINE_CHECK_STAT(vcpu, pf_taken)			\
 	DEFINE_CHECK_STAT(vcpu, pf_fixed)			\
-- 
2.34.1

[PATCH v1 4/4] KVM: selftests: Replace previously used vm_get_stat() to macro

[Manali Shukla]

Previous patch converts vm_get_stat() to macro and adds a concatenation
trickery to generate compilation error if the stat doesn't exist.

Improve nx_huge_pages_test.c and dirty_log_page_splitting_test.c based
on the macro.

Compile tested both the selftests.

Signed-off-by: Manali Shukla <manali.shukla@amd.com>
---
 .../selftests/kvm/x86_64/dirty_log_page_splitting_test.c    | 6 +++---
 tools/testing/selftests/kvm/x86_64/nx_huge_pages_test.c     | 4 ++--
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/tools/testing/selftests/kvm/x86_64/dirty_log_page_splitting_test.c b/tools/testing/selftests/kvm/x86_64/dirty_log_page_splitting_test.c
index 2929c067c207..b0d2b04a7ff2 100644
--- a/tools/testing/selftests/kvm/x86_64/dirty_log_page_splitting_test.c
+++ b/tools/testing/selftests/kvm/x86_64/dirty_log_page_splitting_test.c
@@ -41,9 +41,9 @@ struct kvm_page_stats {
 
 static void get_page_stats(struct kvm_vm *vm, struct kvm_page_stats *stats, const char *stage)
 {
-	stats->pages_4k = vm_get_stat(vm, "pages_4k");
-	stats->pages_2m = vm_get_stat(vm, "pages_2m");
-	stats->pages_1g = vm_get_stat(vm, "pages_1g");
+	stats->pages_4k = vm_get_stat(vm, pages_4k);
+	stats->pages_2m = vm_get_stat(vm, pages_2m);
+	stats->pages_1g = vm_get_stat(vm, pages_1g);
 	stats->hugepages = stats->pages_2m + stats->pages_1g;
 
 	pr_debug("\nPage stats after %s: 4K: %ld 2M: %ld 1G: %ld huge: %ld\n",
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 e7efb2b35f8b..c0d84827f736 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
@@ -73,7 +73,7 @@ static void check_2m_page_count(struct kvm_vm *vm, int expected_pages_2m)
 {
 	int actual_pages_2m;
 
-	actual_pages_2m = vm_get_stat(vm, "pages_2m");
+	actual_pages_2m = vm_get_stat(vm, pages_2m);
 
 	TEST_ASSERT(actual_pages_2m == expected_pages_2m,
 		    "Unexpected 2m page count. Expected %d, got %d",
@@ -84,7 +84,7 @@ static void check_split_count(struct kvm_vm *vm, int expected_splits)
 {
 	int actual_splits;
 
-	actual_splits = vm_get_stat(vm, "nx_lpage_splits");
+	actual_splits = vm_get_stat(vm, nx_lpage_splits);
 
 	TEST_ASSERT(actual_splits == expected_splits,
 		    "Unexpected NX huge page split count. Expected %d, got %d",
-- 
2.34.1

Re: [PATCH v1 4/4] KVM: selftests: Replace previously used vm_get_stat() to macro

[Sean Christopherson]

On Mon, Oct 21, 2024, Manali Shukla wrote:
> Compile tested both the selftests.

To be very blunt, not actually testing changes of this nature isn't acceptable.
The HugeTLB dependency of dirty_log_page_splitting_test makes it annoying to run,
as does the root user dependency of nx_huge_pages_test.sh, but I wouldn't say
that running them is hard.

It's a-ok to not fully test code when you don't have the necessary hardware,
configuration, etc., but generally speaking the expectation is that you test the
code you post.

>  static void get_page_stats(struct kvm_vm *vm, struct kvm_page_stats *stats, const char *stage)
>  {
> -	stats->pages_4k = vm_get_stat(vm, "pages_4k");
> -	stats->pages_2m = vm_get_stat(vm, "pages_2m");
> -	stats->pages_1g = vm_get_stat(vm, "pages_1g");
> +	stats->pages_4k = vm_get_stat(vm, pages_4k);
> +	stats->pages_2m = vm_get_stat(vm, pages_2m);
> +	stats->pages_1g = vm_get_stat(vm, pages_1g);

Converting vm_get_stats() to a macro, or rather, changing its parameter to do
token pasting, absolutely must update all users at the same time.  With only the
previous patch applied, the affected tests fail because they try to get stats
for the string "\"<stat>\"".

Re: [PATCH v1 0/4] KVM selftests enhancements

[Manali Shukla]

On 10/21/2024 11:52 AM, Manali Shukla wrote:
> This series was prompted by feedback given in [1].
> Patch 1   : Adds safe_hlt() and cli() helpers.
> Patch 2, 3: Adds an interface to read vcpu stat in selftest.  Adds
>             a macro to generate compiler error to detect typos at
>             compile time while parsing vcpu and vm stats.
> Patch 4   : Fix few of the selftests based on newly defined macro.
> 
> This series was split from the Idle HLT intercept support series [2]
> because the series has a few changes in the vm_get_stat() interface
> as suggested in [1] and a few changes in two of the self-tests
> (nx_huge_pages_test.c and dirty_log_page_splitting_test.c) which use
> vm_get_stat() functionality to retrieve specified VM stats. These
> changes are unrelated to the Idle HLT intercept support series [2].
> 
> [1] https://lore.kernel.org/kvm/ZruDweYzQRRcJeTO@google.com/T/#m7cd7a110f0fcff9a03176935ed016f49088df6b0
> 
> [2] https://lore.kernel.org/kvm/ZruDweYzQRRcJeTO@google.com/T/#m6c67ca8ccb226e5f357d943da2a2965a4224f69a
> 
> 
> Manali Shukla (4):
>   KVM: selftests: Add safe_halt() and cli() helpers to common code
>   KVM: selftests: Add an interface to read the data of named vcpu stat
>   KVM: selftests: convert vm_get_stat to macro
>   KVM: selftests: Replace previously used vm_get_stat() to macro
> 
>  .../testing/selftests/kvm/include/kvm_util.h  | 83 +++++++++++++++++--
>  .../kvm/include/x86_64/kvm_util_arch.h        | 52 ++++++++++++
>  .../selftests/kvm/include/x86_64/processor.h  | 17 ++++
>  tools/testing/selftests/kvm/lib/kvm_util.c    | 40 +++++++++
>  .../x86_64/dirty_log_page_splitting_test.c    |  6 +-
>  .../selftests/kvm/x86_64/nx_huge_pages_test.c |  4 +-
>  6 files changed, 191 insertions(+), 11 deletions(-)
> 
> 
> base-commit: c8d430db8eec7d4fd13a6bea27b7086a54eda6da

A gentle reminder.

-Manali

Re: [PATCH v1 0/4] KVM selftests enhancements

[Sean Christopherson]

On Mon, Oct 21, 2024, Manali Shukla wrote:
> This series was prompted by feedback given in [1].
> Patch 1   : Adds safe_hlt() and cli() helpers.
> Patch 2, 3: Adds an interface to read vcpu stat in selftest.  Adds
>             a macro to generate compiler error to detect typos at
>             compile time while parsing vcpu and vm stats.
> Patch 4   : Fix few of the selftests based on newly defined macro.
> 
> This series was split from the Idle HLT intercept support series [2]
> because the series has a few changes in the vm_get_stat() interface
> as suggested in [1] and a few changes in two of the self-tests
> (nx_huge_pages_test.c and dirty_log_page_splitting_test.c) which use
> vm_get_stat() functionality to retrieve specified VM stats. These
> changes are unrelated to the Idle HLT intercept support series [2].
> 
> [1] https://lore.kernel.org/kvm/ZruDweYzQRRcJeTO@google.com/T/#m7cd7a110f0fcff9a03176935ed016f49088df6b0
> 
> [2] https://lore.kernel.org/kvm/ZruDweYzQRRcJeTO@google.com/T/#m6c67ca8ccb226e5f357d943da2a2965a4224f69a
> 
> 
> Manali Shukla (4):
>   KVM: selftests: Add safe_halt() and cli() helpers to common code
>   KVM: selftests: Add an interface to read the data of named vcpu stat
>   KVM: selftests: convert vm_get_stat to macro
>   KVM: selftests: Replace previously used vm_get_stat() to macro

Thanks for giving this a shot.  I appreciate the effort, especially on the stats
code.  But unfortunately, very little of the code in this series moves things in
the right direction.  There's too much copy+paste, and too much unnecessary
complexity.

I'll post a patch for the STI/CLI helpers and a series for the stats changes,
and will review the Idle HLT series.  Please plan on posting v5 of that series
on top of kvm-x86/next plus the to-be-posted patches (I highly doubt I will get
them queued before you are ready to post v5).