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/#m7cd7a110f0fcff9a...
[2] https://lore.kernel.org/kvm/ZruDweYzQRRcJeTO@google.com/T/#m6c67ca8ccb226e5f...
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
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
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.
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 *
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.
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) \
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",
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>"".
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/#m7cd7a110f0fcff9a...
[2] https://lore.kernel.org/kvm/ZruDweYzQRRcJeTO@google.com/T/#m6c67ca8ccb226e5f...
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
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/#m7cd7a110f0fcff9a...
[2] https://lore.kernel.org/kvm/ZruDweYzQRRcJeTO@google.com/T/#m6c67ca8ccb226e5f...
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).
linux-kselftest-mirror@lists.linaro.org
-
Manali Shukla -
Sean Christopherson