[PATCH v4 0/9] KVM: selftests: Test the consistency of the PMU's CPUID and its features

[PATCH v4 0/9] KVM: selftests: Test the consistency of the PMU's CPUID and its features

[Jinrong Liang]

Hi,

The KVM selftests show advantages over KUT in terms of finding defects through
flexible and varied guest settings from the KVM user space.

This patchset tests whether the Intel vPMU works properly with different Intel
CPUID.0xA configurations. It also provides test scaffolding and a sufficient
number of PMU test cases to subsequently offer adequate code coverage of AMD
vPMU or Intel complex features, such as LBR or PEBS, in selftests.

These patches have been tested and have passed all test cases. AMD related tests
will be completed in the future, please consider merge these patches before that.

Any feedback or suggestions are greatly appreciated.

Sincerely,
Jinrong Liang

Changelog:

v4:
- Rebased to e2013f46ee2e(tag: kvm-x86-next-2023.08.25)
- Separate AMD-related tests.
- Moved static arrays to a new file lib/pmu.c and used more descriptive names
  like intel_pmu_arch_events, intel_pmu_fixed_pmc_events, and
  amd_pmu_arch_events. (Sean)
- Clean up pmu_event_filter_test.c by including pmu.h and removing
  unnecessary macros.
- Modified the "anti-feature" framework to extend this_pmu_has() and
  kvm_pmu_has() functions. (Sean)
- Refactor guest_measure_loop() function to simplify logic and improve
  readability. (Sean)
- Refactor guest_wr_and_rd_msrs() function to simplify logic and improve
  readability. (Sean)
- Use GUEST_ASSERT_EQ() directly instead of passing the counter to ucall and
  back to the host. (Sean)
- Refactor test_intel_oob_fixed_ctr() test method. (Sean)
- Avoid using half-baked helpers and optimize the code structure. (Sean)
- Update variable names for better readability and consistency. (Sean)
- Rename functions to better reflect their purpose. (Sean)

v3:
https://lore.kernel.org/kvm/20230814115108.45741-1-cloudliang@tencent.com/T/

Jinrong Liang (9):
  KVM: selftests: Add vcpu_set_cpuid_property() to set properties
  KVM: selftests: Extend this_pmu_has() and kvm_pmu_has() to check arch
    events
  KVM: selftests: Add pmu.h for PMU events and common masks
  KVM: selftests: Test Intel PMU architectural events on gp counters
  KVM: selftests: Test Intel PMU architectural events on fixed counters
  KVM: selftests: Test consistency of CPUID with num of gp counters
  KVM: selftests: Test consistency of CPUID with num of fixed counters
  KVM: selftests: Test Intel supported fixed counters bit mask
  KVM: selftests: Test consistency of PMU MSRs with Intel PMU version

 tools/testing/selftests/kvm/Makefile          |   2 +
 tools/testing/selftests/kvm/include/pmu.h     |  96 ++++
 .../selftests/kvm/include/x86_64/processor.h  |  42 +-
 tools/testing/selftests/kvm/lib/pmu.c         |  38 ++
 .../selftests/kvm/lib/x86_64/processor.c      |  14 +
 .../selftests/kvm/x86_64/pmu_counters_test.c  | 431 ++++++++++++++++++
 .../kvm/x86_64/pmu_event_filter_test.c        |  34 +-
 7 files changed, 623 insertions(+), 34 deletions(-)
 create mode 100644 tools/testing/selftests/kvm/include/pmu.h
 create mode 100644 tools/testing/selftests/kvm/lib/pmu.c
 create mode 100644 tools/testing/selftests/kvm/x86_64/pmu_counters_test.c


base-commit: e2013f46ee2e721567783557c301e5c91d0b74ff
-- 
2.39.3

[PATCH v4 1/9] KVM: selftests: Add vcpu_set_cpuid_property() to set properties

[Jinrong Liang]

From: Jinrong Liang <cloudliang@tencent.com>

Add vcpu_set_cpuid_property() helper function for setting properties,
which simplifies the process of setting CPUID properties for vCPUs.

Suggested-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Jinrong Liang <cloudliang@tencent.com>
---
 .../selftests/kvm/include/x86_64/processor.h       |  4 ++++
 tools/testing/selftests/kvm/lib/x86_64/processor.c | 14 ++++++++++++++
 2 files changed, 18 insertions(+)

diff --git a/tools/testing/selftests/kvm/include/x86_64/processor.h b/tools/testing/selftests/kvm/include/x86_64/processor.h
index 4fd042112526..6b146e1c6736 100644
--- a/tools/testing/selftests/kvm/include/x86_64/processor.h
+++ b/tools/testing/selftests/kvm/include/x86_64/processor.h
@@ -973,6 +973,10 @@ static inline void vcpu_set_cpuid(struct kvm_vcpu *vcpu)
 
 void vcpu_set_cpuid_maxphyaddr(struct kvm_vcpu *vcpu, uint8_t maxphyaddr);
 
+void vcpu_set_cpuid_property(struct kvm_vcpu *vcpu,
+			     struct kvm_x86_cpu_property property,
+			     uint32_t value);
+
 void vcpu_clear_cpuid_entry(struct kvm_vcpu *vcpu, uint32_t function);
 void vcpu_set_or_clear_cpuid_feature(struct kvm_vcpu *vcpu,
 				     struct kvm_x86_cpu_feature feature,
diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools/testing/selftests/kvm/lib/x86_64/processor.c
index d8288374078e..0e029be66783 100644
--- a/tools/testing/selftests/kvm/lib/x86_64/processor.c
+++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c
@@ -760,6 +760,20 @@ void vcpu_set_cpuid_maxphyaddr(struct kvm_vcpu *vcpu, uint8_t maxphyaddr)
 	vcpu_set_cpuid(vcpu);
 }
 
+void vcpu_set_cpuid_property(struct kvm_vcpu *vcpu,
+			     struct kvm_x86_cpu_property property,
+			     uint32_t value)
+{
+	struct kvm_cpuid_entry2 *entry;
+
+	entry = __vcpu_get_cpuid_entry(vcpu, property.function, property.index);
+
+	(&entry->eax)[property.reg] &= ~GENMASK(property.hi_bit, property.lo_bit);
+	(&entry->eax)[property.reg] |= value << (property.lo_bit);
+
+	vcpu_set_cpuid(vcpu);
+}
+
 void vcpu_clear_cpuid_entry(struct kvm_vcpu *vcpu, uint32_t function)
 {
 	struct kvm_cpuid_entry2 *entry = vcpu_get_cpuid_entry(vcpu, function);
-- 
2.39.3

[PATCH v4 2/9] KVM: selftests: Extend this_pmu_has() and kvm_pmu_has() to check arch events

[Jinrong Liang]

From: Jinrong Liang <cloudliang@tencent.com>

The kvm_x86_pmu_feature struct has been updated to use the more
descriptive name "pmu_feature" instead of "anti_feature".

Extend this_pmu_has() and kvm_pmu_has() functions to better support
checking for Intel architectural events. Rename this_pmu_has() and
kvm_pmu_has() to this_pmu_has_arch_event() and kvm_pmu_has_arch_event().

Suggested-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Jinrong Liang <cloudliang@tencent.com>
---
 .../selftests/kvm/include/x86_64/processor.h  | 38 ++++++++++++++-----
 .../kvm/x86_64/pmu_event_filter_test.c        |  2 +-
 2 files changed, 29 insertions(+), 11 deletions(-)

diff --git a/tools/testing/selftests/kvm/include/x86_64/processor.h b/tools/testing/selftests/kvm/include/x86_64/processor.h
index 6b146e1c6736..ede433eb6541 100644
--- a/tools/testing/selftests/kvm/include/x86_64/processor.h
+++ b/tools/testing/selftests/kvm/include/x86_64/processor.h
@@ -280,12 +280,12 @@ struct kvm_x86_cpu_property {
  * architectural event is supported.
  */
 struct kvm_x86_pmu_feature {
-	struct kvm_x86_cpu_feature anti_feature;
+	struct kvm_x86_cpu_feature pmu_feature;
 };
 #define	KVM_X86_PMU_FEATURE(name, __bit)					\
 ({										\
 	struct kvm_x86_pmu_feature feature = {					\
-		.anti_feature = KVM_X86_CPU_FEATURE(0xa, 0, EBX, __bit),	\
+		.pmu_feature = KVM_X86_CPU_FEATURE(0xa, 0, EBX, __bit),		\
 	};									\
 										\
 	feature;								\
@@ -681,12 +681,21 @@ static __always_inline bool this_cpu_has_p(struct kvm_x86_cpu_property property)
 	return max_leaf >= property.function;
 }
 
-static inline bool this_pmu_has(struct kvm_x86_pmu_feature feature)
+static inline bool this_pmu_has_arch_event(struct kvm_x86_pmu_feature feature)
 {
-	uint32_t nr_bits = this_cpu_property(X86_PROPERTY_PMU_EBX_BIT_VECTOR_LENGTH);
+	uint32_t nr_bits;
 
-	return nr_bits > feature.anti_feature.bit &&
-	       !this_cpu_has(feature.anti_feature);
+	if (feature.pmu_feature.reg == KVM_CPUID_EBX) {
+		nr_bits = this_cpu_property(X86_PROPERTY_PMU_EBX_BIT_VECTOR_LENGTH);
+		return nr_bits > feature.pmu_feature.bit &&
+			!this_cpu_has(feature.pmu_feature);
+	} else if (feature.pmu_feature.reg == KVM_CPUID_ECX) {
+		nr_bits = this_cpu_property(X86_PROPERTY_PMU_NR_FIXED_COUNTERS);
+		return nr_bits > feature.pmu_feature.bit ||
+			this_cpu_has(feature.pmu_feature);
+	} else {
+		TEST_FAIL("Invalid register in kvm_x86_pmu_feature");
+	}
 }
 
 static __always_inline uint64_t this_cpu_supported_xcr0(void)
@@ -900,12 +909,21 @@ static __always_inline bool kvm_cpu_has_p(struct kvm_x86_cpu_property property)
 	return max_leaf >= property.function;
 }
 
-static inline bool kvm_pmu_has(struct kvm_x86_pmu_feature feature)
+static inline bool kvm_pmu_has_arch_event(struct kvm_x86_pmu_feature feature)
 {
-	uint32_t nr_bits = kvm_cpu_property(X86_PROPERTY_PMU_EBX_BIT_VECTOR_LENGTH);
+	uint32_t nr_bits;
 
-	return nr_bits > feature.anti_feature.bit &&
-	       !kvm_cpu_has(feature.anti_feature);
+	if (feature.pmu_feature.reg == KVM_CPUID_EBX) {
+		nr_bits = kvm_cpu_property(X86_PROPERTY_PMU_EBX_BIT_VECTOR_LENGTH);
+		return nr_bits > feature.pmu_feature.bit &&
+			!kvm_cpu_has(feature.pmu_feature);
+	} else if (feature.pmu_feature.reg == KVM_CPUID_ECX) {
+		nr_bits = kvm_cpu_property(X86_PROPERTY_PMU_NR_FIXED_COUNTERS);
+		return nr_bits > feature.pmu_feature.bit ||
+			kvm_cpu_has(feature.pmu_feature);
+	} else {
+		TEST_FAIL("Invalid register in kvm_x86_pmu_feature");
+	}
 }
 
 static inline size_t kvm_cpuid2_size(int nr_entries)
diff --git a/tools/testing/selftests/kvm/x86_64/pmu_event_filter_test.c b/tools/testing/selftests/kvm/x86_64/pmu_event_filter_test.c
index 283cc55597a4..b0b91e6e79fb 100644
--- a/tools/testing/selftests/kvm/x86_64/pmu_event_filter_test.c
+++ b/tools/testing/selftests/kvm/x86_64/pmu_event_filter_test.c
@@ -408,7 +408,7 @@ static bool use_intel_pmu(void)
 	return host_cpu_is_intel &&
 	       kvm_cpu_property(X86_PROPERTY_PMU_VERSION) &&
 	       kvm_cpu_property(X86_PROPERTY_PMU_NR_GP_COUNTERS) &&
-	       kvm_pmu_has(X86_PMU_FEATURE_BRANCH_INSNS_RETIRED);
+	       kvm_pmu_has_arch_event(X86_PMU_FEATURE_BRANCH_INSNS_RETIRED);
 }
 
 static bool is_zen1(uint32_t family, uint32_t model)
-- 
2.39.3

[PATCH v4 3/9] KVM: selftests: Add pmu.h for PMU events and common masks

[Jinrong Liang]

From: Jinrong Liang <cloudliang@tencent.com>

By defining the PMU performance events and masks relevant for x86 in
the new pmu.h and pmu.c, it becomes easier to reference them, minimizing
potential errors in code that handles these values.

Clean up pmu_event_filter_test.c by including pmu.h and removing
unnecessary macros.

Suggested-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Jinrong Liang <cloudliang@tencent.com>
---
 tools/testing/selftests/kvm/Makefile          |  1 +
 tools/testing/selftests/kvm/include/pmu.h     | 96 +++++++++++++++++++
 tools/testing/selftests/kvm/lib/pmu.c         | 38 ++++++++
 .../kvm/x86_64/pmu_event_filter_test.c        | 32 ++-----
 4 files changed, 144 insertions(+), 23 deletions(-)
 create mode 100644 tools/testing/selftests/kvm/include/pmu.h
 create mode 100644 tools/testing/selftests/kvm/lib/pmu.c

diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
index 77026907968f..172c4223b286 100644
--- a/tools/testing/selftests/kvm/Makefile
+++ b/tools/testing/selftests/kvm/Makefile
@@ -23,6 +23,7 @@ LIBKVM += lib/guest_modes.c
 LIBKVM += lib/io.c
 LIBKVM += lib/kvm_util.c
 LIBKVM += lib/memstress.c
+LIBKVM += lib/pmu.c
 LIBKVM += lib/guest_sprintf.c
 LIBKVM += lib/rbtree.c
 LIBKVM += lib/sparsebit.c
diff --git a/tools/testing/selftests/kvm/include/pmu.h b/tools/testing/selftests/kvm/include/pmu.h
new file mode 100644
index 000000000000..f9d5bf14cf5f
--- /dev/null
+++ b/tools/testing/selftests/kvm/include/pmu.h
@@ -0,0 +1,96 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * tools/testing/selftests/kvm/include/pmu.h
+ *
+ * Copyright (C) 2023, Tencent, Inc.
+ */
+#ifndef SELFTEST_KVM_PMU_H
+#define SELFTEST_KVM_PMU_H
+
+#include <stdint.h>
+
+#define X86_PMC_IDX_MAX				64
+#define INTEL_PMC_MAX_GENERIC				32
+#define KVM_PMU_EVENT_FILTER_MAX_EVENTS		300
+
+#define GP_COUNTER_NR_OFS_BIT				8
+#define EVENT_LENGTH_OFS_BIT				24
+
+#define PMU_VERSION_MASK				GENMASK_ULL(7, 0)
+#define EVENT_LENGTH_MASK				GENMASK_ULL(31, EVENT_LENGTH_OFS_BIT)
+#define GP_COUNTER_NR_MASK				GENMASK_ULL(15, GP_COUNTER_NR_OFS_BIT)
+#define FIXED_COUNTER_NR_MASK				GENMASK_ULL(4, 0)
+
+#define ARCH_PERFMON_EVENTSEL_EVENT			GENMASK_ULL(7, 0)
+#define ARCH_PERFMON_EVENTSEL_UMASK			GENMASK_ULL(15, 8)
+#define ARCH_PERFMON_EVENTSEL_USR			BIT_ULL(16)
+#define ARCH_PERFMON_EVENTSEL_OS			BIT_ULL(17)
+#define ARCH_PERFMON_EVENTSEL_EDGE			BIT_ULL(18)
+#define ARCH_PERFMON_EVENTSEL_PIN_CONTROL		BIT_ULL(19)
+#define ARCH_PERFMON_EVENTSEL_INT			BIT_ULL(20)
+#define ARCH_PERFMON_EVENTSEL_ANY			BIT_ULL(21)
+#define ARCH_PERFMON_EVENTSEL_ENABLE			BIT_ULL(22)
+#define ARCH_PERFMON_EVENTSEL_INV			BIT_ULL(23)
+#define ARCH_PERFMON_EVENTSEL_CMASK			GENMASK_ULL(31, 24)
+
+#define PMC_MAX_FIXED					16
+#define PMC_IDX_FIXED					32
+
+/* RDPMC offset for Fixed PMCs */
+#define PMC_FIXED_RDPMC_BASE				BIT_ULL(30)
+#define PMC_FIXED_RDPMC_METRICS			BIT_ULL(29)
+
+#define FIXED_BITS_MASK				0xFULL
+#define FIXED_BITS_STRIDE				4
+#define FIXED_0_KERNEL					BIT_ULL(0)
+#define FIXED_0_USER					BIT_ULL(1)
+#define FIXED_0_ANYTHREAD				BIT_ULL(2)
+#define FIXED_0_ENABLE_PMI				BIT_ULL(3)
+
+#define fixed_bits_by_idx(_idx, _bits)			\
+	((_bits) << ((_idx) * FIXED_BITS_STRIDE))
+
+#define AMD64_NR_COUNTERS				4
+#define AMD64_NR_COUNTERS_CORE				6
+
+#define PMU_CAP_FW_WRITES				BIT_ULL(13)
+#define PMU_CAP_LBR_FMT				0x3f
+
+enum intel_pmu_architectural_events {
+	/*
+	 * The order of the architectural events matters as support for each
+	 * event is enumerated via CPUID using the index of the event.
+	 */
+	INTEL_ARCH_CPU_CYCLES,
+	INTEL_ARCH_INSTRUCTIONS_RETIRED,
+	INTEL_ARCH_REFERENCE_CYCLES,
+	INTEL_ARCH_LLC_REFERENCES,
+	INTEL_ARCH_LLC_MISSES,
+	INTEL_ARCH_BRANCHES_RETIRED,
+	INTEL_ARCH_BRANCHES_MISPREDICTED,
+
+	NR_REAL_INTEL_ARCH_EVENTS,
+
+	/*
+	 * Pseudo-architectural event used to implement IA32_FIXED_CTR2, a.k.a.
+	 * TSC reference cycles. The architectural reference cycles event may
+	 * or may not actually use the TSC as the reference, e.g. might use the
+	 * core crystal clock or the bus clock (yeah, "architectural").
+	 */
+	PSEUDO_ARCH_REFERENCE_CYCLES = NR_REAL_INTEL_ARCH_EVENTS,
+	NR_INTEL_ARCH_EVENTS,
+};
+
+enum amd_pmu_k7_events {
+	AMD_ZEN_CORE_CYCLES,
+	AMD_ZEN_INSTRUCTIONS,
+	AMD_ZEN_BRANCHES,
+	AMD_ZEN_BRANCH_MISSES,
+	NR_AMD_ARCH_EVENTS,
+};
+
+extern const uint64_t intel_pmu_arch_events[];
+extern const uint64_t amd_pmu_arch_events[];
+extern const int intel_pmu_fixed_pmc_events[];
+
+#endif /* SELFTEST_KVM_PMU_H */
diff --git a/tools/testing/selftests/kvm/lib/pmu.c b/tools/testing/selftests/kvm/lib/pmu.c
new file mode 100644
index 000000000000..331ddbc12fce
--- /dev/null
+++ b/tools/testing/selftests/kvm/lib/pmu.c
@@ -0,0 +1,38 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Kernel-based Virtual Machine -- Selftests Performance Monitoring Unit support
+ *
+ * Copyright (C) 2023, Tencent, Inc.
+ */
+
+#include <stdint.h>
+
+#include "pmu.h"
+
+/* Definitions for Architectural Performance Events */
+#define ARCH_EVENT(select, umask) (((select) & 0xff) | ((umask) & 0xff) << 8)
+
+const uint64_t intel_pmu_arch_events[] = {
+	[INTEL_ARCH_CPU_CYCLES]			= ARCH_EVENT(0x3c, 0x0),
+	[INTEL_ARCH_INSTRUCTIONS_RETIRED]	= ARCH_EVENT(0xc0, 0x0),
+	[INTEL_ARCH_REFERENCE_CYCLES]		= ARCH_EVENT(0x3c, 0x1),
+	[INTEL_ARCH_LLC_REFERENCES]		= ARCH_EVENT(0x2e, 0x4f),
+	[INTEL_ARCH_LLC_MISSES]			= ARCH_EVENT(0x2e, 0x41),
+	[INTEL_ARCH_BRANCHES_RETIRED]		= ARCH_EVENT(0xc4, 0x0),
+	[INTEL_ARCH_BRANCHES_MISPREDICTED]	= ARCH_EVENT(0xc5, 0x0),
+	[PSEUDO_ARCH_REFERENCE_CYCLES]		= ARCH_EVENT(0xa4, 0x1),
+};
+
+/* mapping between fixed pmc index and intel_arch_events array */
+const int intel_pmu_fixed_pmc_events[] = {
+	[0] = INTEL_ARCH_INSTRUCTIONS_RETIRED,
+	[1] = INTEL_ARCH_CPU_CYCLES,
+	[2] = PSEUDO_ARCH_REFERENCE_CYCLES,
+};
+
+const uint64_t amd_pmu_arch_events[] = {
+	[AMD_ZEN_CORE_CYCLES]			= ARCH_EVENT(0x76, 0x00),
+	[AMD_ZEN_INSTRUCTIONS]			= ARCH_EVENT(0xc0, 0x00),
+	[AMD_ZEN_BRANCHES]			= ARCH_EVENT(0xc2, 0x00),
+	[AMD_ZEN_BRANCH_MISSES]			= ARCH_EVENT(0xc3, 0x00),
+};
diff --git a/tools/testing/selftests/kvm/x86_64/pmu_event_filter_test.c b/tools/testing/selftests/kvm/x86_64/pmu_event_filter_test.c
index b0b91e6e79fb..b5402327d739 100644
--- a/tools/testing/selftests/kvm/x86_64/pmu_event_filter_test.c
+++ b/tools/testing/selftests/kvm/x86_64/pmu_event_filter_test.c
@@ -11,31 +11,18 @@
  */
 
 #define _GNU_SOURCE /* for program_invocation_short_name */
-#include "test_util.h"
+
 #include "kvm_util.h"
+#include "pmu.h"
 #include "processor.h"
-
-/*
- * In lieu of copying perf_event.h into tools...
- */
-#define ARCH_PERFMON_EVENTSEL_OS			(1ULL << 17)
-#define ARCH_PERFMON_EVENTSEL_ENABLE			(1ULL << 22)
-
-/* End of stuff taken from perf_event.h. */
-
-/* Oddly, this isn't in perf_event.h. */
-#define ARCH_PERFMON_BRANCHES_RETIRED		5
+#include "test_util.h"
 
 #define NUM_BRANCHES 42
-#define INTEL_PMC_IDX_FIXED		32
-
-/* Matches KVM_PMU_EVENT_FILTER_MAX_EVENTS in pmu.c */
-#define MAX_FILTER_EVENTS		300
 #define MAX_TEST_EVENTS		10
 
 #define PMU_EVENT_FILTER_INVALID_ACTION		(KVM_PMU_EVENT_DENY + 1)
 #define PMU_EVENT_FILTER_INVALID_FLAGS			(KVM_PMU_EVENT_FLAGS_VALID_MASK << 1)
-#define PMU_EVENT_FILTER_INVALID_NEVENTS		(MAX_FILTER_EVENTS + 1)
+#define PMU_EVENT_FILTER_INVALID_NEVENTS		(KVM_PMU_EVENT_FILTER_MAX_EVENTS + 1)
 
 /*
  * This is how the event selector and unit mask are stored in an AMD
@@ -63,7 +50,6 @@
 
 #define AMD_ZEN_BR_RETIRED EVENT(0xc2, 0)
 
-
 /*
  * "Retired instructions", from Processor Programming Reference
  * (PPR) for AMD Family 17h Model 01h, Revision B1 Processors,
@@ -84,7 +70,7 @@ struct __kvm_pmu_event_filter {
 	__u32 fixed_counter_bitmap;
 	__u32 flags;
 	__u32 pad[4];
-	__u64 events[MAX_FILTER_EVENTS];
+	__u64 events[KVM_PMU_EVENT_FILTER_MAX_EVENTS];
 };
 
 /*
@@ -729,14 +715,14 @@ static void add_dummy_events(uint64_t *events, int nevents)
 
 static void test_masked_events(struct kvm_vcpu *vcpu)
 {
-	int nevents = MAX_FILTER_EVENTS - MAX_TEST_EVENTS;
-	uint64_t events[MAX_FILTER_EVENTS];
+	int nevents = KVM_PMU_EVENT_FILTER_MAX_EVENTS - MAX_TEST_EVENTS;
+	uint64_t events[KVM_PMU_EVENT_FILTER_MAX_EVENTS];
 
 	/* Run the test cases against a sparse PMU event filter. */
 	run_masked_events_tests(vcpu, events, 0);
 
 	/* Run the test cases against a dense PMU event filter. */
-	add_dummy_events(events, MAX_FILTER_EVENTS);
+	add_dummy_events(events, KVM_PMU_EVENT_FILTER_MAX_EVENTS);
 	run_masked_events_tests(vcpu, events, nevents);
 }
 
@@ -818,7 +804,7 @@ static void intel_run_fixed_counter_guest_code(uint8_t fixed_ctr_idx)
 		/* Only OS_EN bit is enabled for fixed counter[idx]. */
 		wrmsr(MSR_CORE_PERF_FIXED_CTR_CTRL, BIT_ULL(4 * fixed_ctr_idx));
 		wrmsr(MSR_CORE_PERF_GLOBAL_CTRL,
-		      BIT_ULL(INTEL_PMC_IDX_FIXED + fixed_ctr_idx));
+		      BIT_ULL(PMC_IDX_FIXED + fixed_ctr_idx));
 		__asm__ __volatile__("loop ." : "+c"((int){NUM_BRANCHES}));
 		wrmsr(MSR_CORE_PERF_GLOBAL_CTRL, 0);
 
-- 
2.39.3

[PATCH v4 4/9] KVM: selftests: Test Intel PMU architectural events on gp counters

[Jinrong Liang]

From: Jinrong Liang <cloudliang@tencent.com>

Add test cases to check if different Architectural events are available
after it's marked as unavailable via CPUID. It covers vPMU event filtering
logic based on Intel CPUID, which is a complement to pmu_event_filter.

According to Intel SDM, the number of architectural events is reported
through CPUID.0AH:EAX[31:24] and the architectural event x is supported
if EBX[x]=0 && EAX[31:24]>x.

Co-developed-by: Like Xu <likexu@tencent.com>
Signed-off-by: Like Xu <likexu@tencent.com>
Signed-off-by: Jinrong Liang <cloudliang@tencent.com>
---
 tools/testing/selftests/kvm/Makefile          |   1 +
 .../selftests/kvm/x86_64/pmu_counters_test.c  | 182 ++++++++++++++++++
 2 files changed, 183 insertions(+)
 create mode 100644 tools/testing/selftests/kvm/x86_64/pmu_counters_test.c

diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
index 172c4223b286..7768e09de96c 100644
--- a/tools/testing/selftests/kvm/Makefile
+++ b/tools/testing/selftests/kvm/Makefile
@@ -81,6 +81,7 @@ TEST_GEN_PROGS_x86_64 += x86_64/mmio_warning_test
 TEST_GEN_PROGS_x86_64 += x86_64/monitor_mwait_test
 TEST_GEN_PROGS_x86_64 += x86_64/nested_exceptions_test
 TEST_GEN_PROGS_x86_64 += x86_64/platform_info_test
+TEST_GEN_PROGS_x86_64 += x86_64/pmu_counters_test
 TEST_GEN_PROGS_x86_64 += x86_64/pmu_event_filter_test
 TEST_GEN_PROGS_x86_64 += x86_64/set_boot_cpu_id
 TEST_GEN_PROGS_x86_64 += x86_64/set_sregs_test
diff --git a/tools/testing/selftests/kvm/x86_64/pmu_counters_test.c b/tools/testing/selftests/kvm/x86_64/pmu_counters_test.c
new file mode 100644
index 000000000000..f47853f3ab84
--- /dev/null
+++ b/tools/testing/selftests/kvm/x86_64/pmu_counters_test.c
@@ -0,0 +1,182 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Test the consistency of the PMU's CPUID and its features
+ *
+ * Copyright (C) 2023, Tencent, Inc.
+ *
+ * Check that the VM's PMU behaviour is consistent with the
+ * VM CPUID definition.
+ */
+
+#define _GNU_SOURCE /* for program_invocation_short_name */
+#include <x86intrin.h>
+
+#include "pmu.h"
+#include "processor.h"
+
+/* Guest payload for any performance counter counting */
+#define NUM_BRANCHES		10
+
+static struct kvm_vm *pmu_vm_create_with_one_vcpu(struct kvm_vcpu **vcpu,
+						  void *guest_code)
+{
+	struct kvm_vm *vm;
+
+	vm = vm_create_with_one_vcpu(vcpu, guest_code);
+	vm_init_descriptor_tables(vm);
+	vcpu_init_descriptor_tables(*vcpu);
+
+	return vm;
+}
+
+static void run_vcpu(struct kvm_vcpu *vcpu)
+{
+	struct ucall uc;
+
+	do {
+		vcpu_run(vcpu);
+		switch (get_ucall(vcpu, &uc)) {
+		case UCALL_SYNC:
+			break;
+		case UCALL_ABORT:
+			REPORT_GUEST_ASSERT(uc);
+			break;
+		case UCALL_DONE:
+			break;
+		default:
+			TEST_FAIL("Unexpected ucall: %lu", uc.cmd);
+		}
+	} while (uc.cmd != UCALL_DONE);
+}
+
+static void guest_measure_pmu_v1(uint8_t idx, uint32_t counter_msr,
+				 uint32_t nr_gp_counters, bool expect)
+{
+	unsigned int i;
+
+	for (i = 0; i < nr_gp_counters; i++) {
+		wrmsr(counter_msr + i, 0);
+		wrmsr(MSR_P6_EVNTSEL0 + i, ARCH_PERFMON_EVENTSEL_OS |
+		      ARCH_PERFMON_EVENTSEL_ENABLE |
+		      intel_pmu_arch_events[idx]);
+		__asm__ __volatile__("loop ." : "+c"((int){NUM_BRANCHES}));
+		GUEST_ASSERT_EQ(expect, !!_rdpmc(i));
+
+		wrmsr(MSR_P6_EVNTSEL0 + i, ARCH_PERFMON_EVENTSEL_OS |
+		      !ARCH_PERFMON_EVENTSEL_ENABLE |
+		      intel_pmu_arch_events[idx]);
+		wrmsr(counter_msr + i, 0);
+		__asm__ __volatile__("loop ." : "+c"((int){NUM_BRANCHES}));
+		GUEST_ASSERT(!_rdpmc(i));
+	}
+
+	GUEST_DONE();
+}
+
+static void guest_measure_loop(uint8_t idx)
+{
+	uint32_t nr_gp_counters = this_cpu_property(X86_PROPERTY_PMU_NR_GP_COUNTERS);
+	uint32_t pmu_version = this_cpu_property(X86_PROPERTY_PMU_VERSION);
+	uint32_t counter_msr;
+	unsigned int i;
+	bool expect;
+
+	if (rdmsr(MSR_IA32_PERF_CAPABILITIES) & PMU_CAP_FW_WRITES)
+		counter_msr = MSR_IA32_PMC0;
+	else
+		counter_msr = MSR_IA32_PERFCTR0;
+
+	expect = this_pmu_has_arch_event(KVM_X86_PMU_FEATURE(UNUSED, idx));
+
+	if (pmu_version < 2) {
+		guest_measure_pmu_v1(idx, counter_msr, nr_gp_counters, expect);
+		return;
+	}
+
+	for (i = 0; i < nr_gp_counters; i++) {
+		wrmsr(counter_msr + i, 0);
+		wrmsr(MSR_P6_EVNTSEL0 + i, ARCH_PERFMON_EVENTSEL_OS |
+		      ARCH_PERFMON_EVENTSEL_ENABLE |
+		      intel_pmu_arch_events[idx]);
+
+		wrmsr(MSR_CORE_PERF_GLOBAL_CTRL, BIT_ULL(i));
+		__asm__ __volatile__("loop ." : "+c"((int){NUM_BRANCHES}));
+		wrmsr(MSR_CORE_PERF_GLOBAL_CTRL, 0);
+
+		GUEST_ASSERT_EQ(expect, !!_rdpmc(i));
+	}
+
+	GUEST_DONE();
+}
+
+static void test_arch_events_cpuid(uint8_t i, uint8_t j, uint8_t idx)
+{
+	uint8_t arch_events_unavailable_mask = BIT_ULL(j);
+	uint8_t arch_events_bitmap_size = BIT_ULL(i);
+	struct kvm_vcpu *vcpu;
+	struct kvm_vm *vm;
+
+	vm = pmu_vm_create_with_one_vcpu(&vcpu, guest_measure_loop);
+
+	vcpu_set_cpuid_property(vcpu, X86_PROPERTY_PMU_EBX_BIT_VECTOR_LENGTH,
+				arch_events_bitmap_size);
+	vcpu_set_cpuid_property(vcpu, X86_PROPERTY_PMU_EVENTS_MASK,
+				arch_events_unavailable_mask);
+
+	vcpu_args_set(vcpu, 1, idx);
+
+	run_vcpu(vcpu);
+
+	kvm_vm_free(vm);
+}
+
+static void check_arch_event_is_unavl(uint8_t idx)
+{
+	uint8_t i, j;
+
+	/*
+	 * A brute force iteration of all combinations of values is likely to
+	 * exhaust the limit of the single-threaded thread fd nums, so it's
+	 * tested here by iterating through all valid values on a single bit.
+	 */
+	for (i = 0; i < NR_INTEL_ARCH_EVENTS; i++) {
+		for (j = 0; j < NR_INTEL_ARCH_EVENTS; j++)
+			test_arch_events_cpuid(i, j, idx);
+	}
+}
+
+static void test_intel_arch_events(void)
+{
+	uint8_t idx;
+
+	for (idx = 0; idx < NR_INTEL_ARCH_EVENTS; idx++) {
+		/*
+		 * Given the stability of performance event recurrence, only
+		 * these arch events are currently being tested:
+		 *
+		 * - Core cycle event (idx = 0)
+		 * - Instruction retired event (idx = 1)
+		 * - Reference cycles event (idx = 2)
+		 * - Branch instruction retired event (idx = 5)
+		 */
+		if (idx > INTEL_ARCH_INSTRUCTIONS_RETIRED &&
+		    idx != INTEL_ARCH_BRANCHES_RETIRED)
+			continue;
+
+		check_arch_event_is_unavl(idx);
+	}
+}
+
+int main(int argc, char *argv[])
+{
+	TEST_REQUIRE(get_kvm_param_bool("enable_pmu"));
+
+	TEST_REQUIRE(host_cpu_is_intel);
+	TEST_REQUIRE(kvm_cpu_has_p(X86_PROPERTY_PMU_VERSION));
+	TEST_REQUIRE(kvm_cpu_property(X86_PROPERTY_PMU_VERSION) > 0);
+	TEST_REQUIRE(kvm_cpu_has(X86_FEATURE_PDCM));
+
+	test_intel_arch_events();
+
+	return 0;
+}
-- 
2.39.3

[PATCH v4 5/9] KVM: selftests: Test Intel PMU architectural events on fixed counters

[Jinrong Liang]

From: Jinrong Liang <cloudliang@tencent.com>

Update test to cover Intel PMU architectural events on fixed counters.
Per Intel SDM, PMU users can also count architecture performance events
on fixed counters (specifically, FIXED_CTR0 for the retired instructions
and FIXED_CTR1 for cpu core cycles event). Therefore, if guest's CPUID
indicates that an architecture event is not available, the corresponding
fixed counter will also not count that event.

Co-developed-by: Like Xu <likexu@tencent.com>
Signed-off-by: Like Xu <likexu@tencent.com>
Signed-off-by: Jinrong Liang <cloudliang@tencent.com>
---
 .../selftests/kvm/x86_64/pmu_counters_test.c  | 22 +++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/tools/testing/selftests/kvm/x86_64/pmu_counters_test.c b/tools/testing/selftests/kvm/x86_64/pmu_counters_test.c
index f47853f3ab84..fe9f38a3557e 100644
--- a/tools/testing/selftests/kvm/x86_64/pmu_counters_test.c
+++ b/tools/testing/selftests/kvm/x86_64/pmu_counters_test.c
@@ -106,6 +106,28 @@ static void guest_measure_loop(uint8_t idx)
 		GUEST_ASSERT_EQ(expect, !!_rdpmc(i));
 	}
 
+	if (this_cpu_property(X86_PROPERTY_PMU_NR_FIXED_COUNTERS) < 1)
+		goto done;
+
+	if (idx == INTEL_ARCH_INSTRUCTIONS_RETIRED)
+		i = 0;
+	else if (idx == INTEL_ARCH_CPU_CYCLES)
+		i = 1;
+	else if (idx == PSEUDO_ARCH_REFERENCE_CYCLES)
+		i = 2;
+	else
+		goto done;
+
+	wrmsr(MSR_CORE_PERF_FIXED_CTR0 + i, 0);
+	wrmsr(MSR_CORE_PERF_FIXED_CTR_CTRL, BIT_ULL(4 * i));
+
+	wrmsr(MSR_CORE_PERF_GLOBAL_CTRL, BIT_ULL(PMC_IDX_FIXED + i));
+	__asm__ __volatile__("loop ." : "+c"((int){NUM_BRANCHES}));
+	wrmsr(MSR_CORE_PERF_GLOBAL_CTRL, 0);
+
+	GUEST_ASSERT_EQ(expect, !!_rdpmc(PMC_FIXED_RDPMC_BASE | i));
+
+done:
 	GUEST_DONE();
 }
 
-- 
2.39.3

[PATCH v4 6/9] KVM: selftests: Test consistency of CPUID with num of gp counters

[Jinrong Liang]

From: Jinrong Liang <cloudliang@tencent.com>

Add test to check if non-existent counters can be accessed in guest after
determining the number of Intel generic performance counters by CPUID.
When the num of counters is less than 3, KVM does not emulate #GP if
a counter isn't present due to compatibility MSR_P6_PERFCTRx handling.
Nor will the KVM emulate more counters than it can support.

Co-developed-by: Like Xu <likexu@tencent.com>
Signed-off-by: Like Xu <likexu@tencent.com>
Signed-off-by: Jinrong Liang <cloudliang@tencent.com>
---
 .../selftests/kvm/x86_64/pmu_counters_test.c  | 85 +++++++++++++++++++
 1 file changed, 85 insertions(+)

diff --git a/tools/testing/selftests/kvm/x86_64/pmu_counters_test.c b/tools/testing/selftests/kvm/x86_64/pmu_counters_test.c
index fe9f38a3557e..e636323e202c 100644
--- a/tools/testing/selftests/kvm/x86_64/pmu_counters_test.c
+++ b/tools/testing/selftests/kvm/x86_64/pmu_counters_test.c
@@ -17,6 +17,11 @@
 /* Guest payload for any performance counter counting */
 #define NUM_BRANCHES		10
 
+static const uint64_t perf_caps[] = {
+	0,
+	PMU_CAP_FW_WRITES,
+};
+
 static struct kvm_vm *pmu_vm_create_with_one_vcpu(struct kvm_vcpu **vcpu,
 						  void *guest_code)
 {
@@ -189,6 +194,85 @@ static void test_intel_arch_events(void)
 	}
 }
 
+static void __guest_wrmsr_rdmsr(uint32_t counter_msr, uint8_t nr_msrs,
+				bool expect_gp)
+{
+	uint64_t msr_val;
+	uint8_t vector;
+
+	vector = wrmsr_safe(counter_msr + nr_msrs, 0xffff);
+	__GUEST_ASSERT(expect_gp ? vector == GP_VECTOR : !vector,
+		       "Expected GP_VECTOR");
+
+	vector = rdmsr_safe(counter_msr + nr_msrs, &msr_val);
+	__GUEST_ASSERT(expect_gp ? vector == GP_VECTOR : !vector,
+		       "Expected GP_VECTOR");
+
+	if (!expect_gp)
+		GUEST_ASSERT_EQ(msr_val, 0);
+
+	GUEST_DONE();
+}
+
+static void guest_rd_wr_gp_counter(void)
+{
+	uint8_t nr_gp_counters = this_cpu_property(X86_PROPERTY_PMU_NR_GP_COUNTERS);
+	uint64_t perf_capabilities = rdmsr(MSR_IA32_PERF_CAPABILITIES);
+	uint32_t counter_msr;
+	bool expect_gp = true;
+
+	if (perf_capabilities & PMU_CAP_FW_WRITES) {
+		counter_msr = MSR_IA32_PMC0;
+	} else {
+		counter_msr = MSR_IA32_PERFCTR0;
+
+		/* KVM drops writes to MSR_P6_PERFCTR[0|1]. */
+		if (nr_gp_counters == 0)
+			expect_gp = false;
+	}
+
+	__guest_wrmsr_rdmsr(counter_msr, nr_gp_counters, expect_gp);
+}
+
+/* Access the first out-of-range counter register to trigger #GP */
+static void test_oob_gp_counter(uint8_t eax_gp_num, uint64_t perf_cap)
+{
+	struct kvm_vcpu *vcpu;
+	struct kvm_vm *vm;
+
+	vm = pmu_vm_create_with_one_vcpu(&vcpu, guest_rd_wr_gp_counter);
+
+	vcpu_set_cpuid_property(vcpu, X86_PROPERTY_PMU_NR_GP_COUNTERS,
+				eax_gp_num);
+	vcpu_set_msr(vcpu, MSR_IA32_PERF_CAPABILITIES, perf_cap);
+
+	run_vcpu(vcpu);
+
+	kvm_vm_free(vm);
+}
+
+static void test_intel_counters_num(void)
+{
+	uint8_t nr_gp_counters = kvm_cpu_property(X86_PROPERTY_PMU_NR_GP_COUNTERS);
+	unsigned int i;
+
+	TEST_REQUIRE(nr_gp_counters > 2);
+
+	for (i = 0; i < ARRAY_SIZE(perf_caps); i++) {
+		/*
+		 * For compatibility reasons, KVM does not emulate #GP
+		 * when MSR_P6_PERFCTR[0|1] is not present, but it doesn't
+		 * affect checking the presence of MSR_IA32_PMCx with #GP.
+		 */
+		test_oob_gp_counter(0, perf_caps[i]);
+		test_oob_gp_counter(2, perf_caps[i]);
+		test_oob_gp_counter(nr_gp_counters, perf_caps[i]);
+
+		/* KVM doesn't emulate more counters than it can support. */
+		test_oob_gp_counter(nr_gp_counters + 1, perf_caps[i]);
+	}
+}
+
 int main(int argc, char *argv[])
 {
 	TEST_REQUIRE(get_kvm_param_bool("enable_pmu"));
@@ -199,6 +283,7 @@ int main(int argc, char *argv[])
 	TEST_REQUIRE(kvm_cpu_has(X86_FEATURE_PDCM));
 
 	test_intel_arch_events();
+	test_intel_counters_num();
 
 	return 0;
 }
-- 
2.39.3

[PATCH v4 7/9] KVM: selftests: Test consistency of CPUID with num of fixed counters

[Jinrong Liang]

From: Jinrong Liang <cloudliang@tencent.com>

Add test to check if non-existent counters can be accessed in guest after
determining the number of Intel generic performance counters by CPUID.
Per SDM, fixed-function performance counter 'i' is supported if ECX[i] ||
(EDX[4:0] > i). KVM doesn't emulate more counters than it can support.

Co-developed-by: Like Xu <likexu@tencent.com>
Signed-off-by: Like Xu <likexu@tencent.com>
Signed-off-by: Jinrong Liang <cloudliang@tencent.com>
---
 .../selftests/kvm/x86_64/pmu_counters_test.c  | 28 +++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/tools/testing/selftests/kvm/x86_64/pmu_counters_test.c b/tools/testing/selftests/kvm/x86_64/pmu_counters_test.c
index e636323e202c..df76f0f2bfd0 100644
--- a/tools/testing/selftests/kvm/x86_64/pmu_counters_test.c
+++ b/tools/testing/selftests/kvm/x86_64/pmu_counters_test.c
@@ -251,8 +251,32 @@ static void test_oob_gp_counter(uint8_t eax_gp_num, uint64_t perf_cap)
 	kvm_vm_free(vm);
 }
 
+static void guest_rd_wr_fixed_counter(void)
+{
+	uint8_t nr_fixed_counters = this_cpu_property(X86_PROPERTY_PMU_NR_FIXED_COUNTERS);
+
+	__guest_wrmsr_rdmsr(MSR_CORE_PERF_FIXED_CTR0, nr_fixed_counters, true);
+}
+
+static void test_oob_fixed_ctr(uint8_t edx_fixed_num)
+{
+	struct kvm_vcpu *vcpu;
+	struct kvm_vm *vm;
+
+	vm = pmu_vm_create_with_one_vcpu(&vcpu, guest_rd_wr_fixed_counter);
+
+	vcpu_set_cpuid_property(vcpu, X86_PROPERTY_PMU_FIXED_COUNTERS_BITMASK, 0);
+	vcpu_set_cpuid_property(vcpu, X86_PROPERTY_PMU_NR_FIXED_COUNTERS,
+				edx_fixed_num);
+
+	run_vcpu(vcpu);
+
+	kvm_vm_free(vm);
+}
+
 static void test_intel_counters_num(void)
 {
+	uint8_t nr_fixed_counters = kvm_cpu_property(X86_PROPERTY_PMU_NR_FIXED_COUNTERS);
 	uint8_t nr_gp_counters = kvm_cpu_property(X86_PROPERTY_PMU_NR_GP_COUNTERS);
 	unsigned int i;
 
@@ -271,6 +295,10 @@ static void test_intel_counters_num(void)
 		/* KVM doesn't emulate more counters than it can support. */
 		test_oob_gp_counter(nr_gp_counters + 1, perf_caps[i]);
 	}
+
+	test_oob_fixed_ctr(0);
+	test_oob_fixed_ctr(nr_fixed_counters);
+	test_oob_fixed_ctr(nr_fixed_counters + 1);
 }
 
 int main(int argc, char *argv[])
-- 
2.39.3

[PATCH v4 8/9] KVM: selftests: Test Intel supported fixed counters bit mask

[Jinrong Liang]

From: Jinrong Liang <cloudliang@tencent.com>

Add a test to check that fixed counters enabled via guest
CPUID.0xA.ECX (instead of EDX[04:00]) work as normal as usual.

Co-developed-by: Like Xu <likexu@tencent.com>
Signed-off-by: Like Xu <likexu@tencent.com>
Signed-off-by: Jinrong Liang <cloudliang@tencent.com>
---
 .../selftests/kvm/x86_64/pmu_counters_test.c  | 54 +++++++++++++++++++
 1 file changed, 54 insertions(+)

diff --git a/tools/testing/selftests/kvm/x86_64/pmu_counters_test.c b/tools/testing/selftests/kvm/x86_64/pmu_counters_test.c
index df76f0f2bfd0..12c00bf94683 100644
--- a/tools/testing/selftests/kvm/x86_64/pmu_counters_test.c
+++ b/tools/testing/selftests/kvm/x86_64/pmu_counters_test.c
@@ -301,6 +301,59 @@ static void test_intel_counters_num(void)
 	test_oob_fixed_ctr(nr_fixed_counters + 1);
 }
 
+static void fixed_counters_guest_code(void)
+{
+	uint64_t supported_bitmask = this_cpu_property(X86_PROPERTY_PMU_FIXED_COUNTERS_BITMASK);
+	uint32_t nr_fixed_counter = this_cpu_property(X86_PROPERTY_PMU_NR_FIXED_COUNTERS);
+	uint64_t msr_val;
+	unsigned int i;
+	bool expected;
+
+	for (i = 0; i < nr_fixed_counter; i++) {
+		expected = supported_bitmask & BIT_ULL(i) || i < nr_fixed_counter;
+
+		wrmsr_safe(MSR_CORE_PERF_FIXED_CTR0 + i, 0);
+		wrmsr_safe(MSR_CORE_PERF_FIXED_CTR_CTRL, BIT_ULL(4 * i));
+		wrmsr_safe(MSR_CORE_PERF_GLOBAL_CTRL, BIT_ULL(PMC_IDX_FIXED + i));
+		__asm__ __volatile__("loop ." : "+c"((int){NUM_BRANCHES}));
+		wrmsr_safe(MSR_CORE_PERF_GLOBAL_CTRL, 0);
+		rdmsr_safe(MSR_CORE_PERF_FIXED_CTR0 + i, &msr_val);
+
+		GUEST_ASSERT_EQ(expected, !!msr_val);
+	}
+
+	GUEST_DONE();
+}
+
+static void __test_fixed_counters(uint32_t fixed_bitmask, uint8_t edx_fixed_num)
+{
+	struct kvm_vcpu *vcpu;
+	struct kvm_vm *vm;
+
+	vm = pmu_vm_create_with_one_vcpu(&vcpu, fixed_counters_guest_code);
+
+	vcpu_set_cpuid_property(vcpu, X86_PROPERTY_PMU_FIXED_COUNTERS_BITMASK,
+				fixed_bitmask);
+	vcpu_set_cpuid_property(vcpu, X86_PROPERTY_PMU_NR_FIXED_COUNTERS,
+				edx_fixed_num);
+
+	run_vcpu(vcpu);
+
+	kvm_vm_free(vm);
+}
+
+static void test_fixed_counters(void)
+{
+	uint8_t nr_fixed_counters = kvm_cpu_property(X86_PROPERTY_PMU_NR_FIXED_COUNTERS);
+	uint32_t ecx;
+	uint8_t edx;
+
+	for (edx = 0; edx <= nr_fixed_counters; edx++)
+		/* KVM doesn't emulate more fixed counters than it can support. */
+		for (ecx = 0; ecx <= (BIT_ULL(nr_fixed_counters) - 1); ecx++)
+			__test_fixed_counters(ecx, edx);
+}
+
 int main(int argc, char *argv[])
 {
 	TEST_REQUIRE(get_kvm_param_bool("enable_pmu"));
@@ -312,6 +365,7 @@ int main(int argc, char *argv[])
 
 	test_intel_arch_events();
 	test_intel_counters_num();
+	test_fixed_counters();
 
 	return 0;
 }
-- 
2.39.3

[PATCH v4 9/9] KVM: selftests: Test consistency of PMU MSRs with Intel PMU version

[Jinrong Liang]

From: Jinrong Liang <cloudliang@tencent.com>

KVM user sapce may control the Intel guest PMU version number via
CPUID.0AH:EAX[07:00]. A test is added to check if a typical PMU register
that is not available at the current version number is leaking.

Co-developed-by: Like Xu <likexu@tencent.com>
Signed-off-by: Like Xu <likexu@tencent.com>
Signed-off-by: Jinrong Liang <cloudliang@tencent.com>
---
 .../selftests/kvm/x86_64/pmu_counters_test.c  | 60 +++++++++++++++++++
 1 file changed, 60 insertions(+)

diff --git a/tools/testing/selftests/kvm/x86_64/pmu_counters_test.c b/tools/testing/selftests/kvm/x86_64/pmu_counters_test.c
index 12c00bf94683..74cb3f647e97 100644
--- a/tools/testing/selftests/kvm/x86_64/pmu_counters_test.c
+++ b/tools/testing/selftests/kvm/x86_64/pmu_counters_test.c
@@ -17,6 +17,12 @@
 /* Guest payload for any performance counter counting */
 #define NUM_BRANCHES		10
 
+/*
+ * KVM implements the first two non-existent counters (MSR_P6_PERFCTRx)
+ * via kvm_pr_unimpl_wrmsr() instead of #GP.
+ */
+#define MSR_INTEL_ARCH_PMU_GPCTR (MSR_IA32_PERFCTR0 + 2)
+
 static const uint64_t perf_caps[] = {
 	0,
 	PMU_CAP_FW_WRITES,
@@ -354,6 +360,59 @@ static void test_fixed_counters(void)
 			__test_fixed_counters(ecx, edx);
 }
 
+static void pmu_version_guest_code(void)
+{
+	uint8_t pmu_version = this_cpu_property(X86_PROPERTY_PMU_VERSION);
+
+	switch (pmu_version) {
+	case 0:
+		GUEST_ASSERT_EQ(wrmsr_safe(MSR_INTEL_ARCH_PMU_GPCTR, 0xffffull),
+				GP_VECTOR);
+	case 1:
+		GUEST_ASSERT_EQ(wrmsr_safe(MSR_CORE_PERF_GLOBAL_CTRL, 0x1ull),
+				GP_VECTOR);
+	case 2:
+		/*
+		 * AnyThread Bit is only supported in version 3
+		 *
+		 * The strange thing is that when version=0, writing ANY-Any
+		 * Thread bit (bit 21) in MSR_P6_EVNTSEL0 and MSR_P6_EVNTSEL1
+		 * will not generate #GP. While writing ANY-Any Thread bit
+		 * (bit 21) in MSR_P6_EVNTSEL0+x (MAX_GP_CTR_NUM > x > 2) to
+		 * ANY-Any Thread bit (bit 21) will generate #GP.
+		 */
+		if (pmu_version == 0)
+			break;
+
+		GUEST_ASSERT_EQ(wrmsr_safe(MSR_P6_EVNTSEL0, ARCH_PERFMON_EVENTSEL_ANY),
+				GP_VECTOR);
+		break;
+	default:
+		/* KVM currently supports up to pmu version 2 */
+		GUEST_DONE();
+	}
+
+	GUEST_DONE();
+}
+
+static void test_intel_pmu_version(void)
+{
+	uint8_t unsupported_version = kvm_cpu_property(X86_PROPERTY_PMU_VERSION) + 1;
+	struct kvm_vcpu *vcpu;
+	struct kvm_vm *vm;
+	uint8_t version;
+
+	TEST_REQUIRE(kvm_cpu_property(X86_PROPERTY_PMU_NR_FIXED_COUNTERS) > 2);
+
+	for (version = 0; version <= unsupported_version; version++) {
+		vm = pmu_vm_create_with_one_vcpu(&vcpu, pmu_version_guest_code);
+		vcpu_set_cpuid_property(vcpu, X86_PROPERTY_PMU_VERSION, version);
+		run_vcpu(vcpu);
+
+		kvm_vm_free(vm);
+	}
+}
+
 int main(int argc, char *argv[])
 {
 	TEST_REQUIRE(get_kvm_param_bool("enable_pmu"));
@@ -366,6 +425,7 @@ int main(int argc, char *argv[])
 	test_intel_arch_events();
 	test_intel_counters_num();
 	test_fixed_counters();
+	test_intel_pmu_version();
 
 	return 0;
 }
-- 
2.39.3

Re: [PATCH v4 0/9] KVM: selftests: Test the consistency of the PMU's CPUID and its features

[Jinrong Liang]

Gentle ping.

Jinrong Liang <ljr.kernel@gmail.com> 于2023年9月11日周一 19:44写道：
>
> Hi,
>
> The KVM selftests show advantages over KUT in terms of finding defects through
> flexible and varied guest settings from the KVM user space.
>
> This patchset tests whether the Intel vPMU works properly with different Intel
> CPUID.0xA configurations. It also provides test scaffolding and a sufficient
> number of PMU test cases to subsequently offer adequate code coverage of AMD
> vPMU or Intel complex features, such as LBR or PEBS, in selftests.
>
> These patches have been tested and have passed all test cases. AMD related tests
> will be completed in the future, please consider merge these patches before that.
>
> Any feedback or suggestions are greatly appreciated.
>
> Sincerely,
> Jinrong Liang
>
> Changelog:
>
> v4:
> - Rebased to e2013f46ee2e(tag: kvm-x86-next-2023.08.25)
> - Separate AMD-related tests.
> - Moved static arrays to a new file lib/pmu.c and used more descriptive names
>   like intel_pmu_arch_events, intel_pmu_fixed_pmc_events, and
>   amd_pmu_arch_events. (Sean)
> - Clean up pmu_event_filter_test.c by including pmu.h and removing
>   unnecessary macros.
> - Modified the "anti-feature" framework to extend this_pmu_has() and
>   kvm_pmu_has() functions. (Sean)
> - Refactor guest_measure_loop() function to simplify logic and improve
>   readability. (Sean)
> - Refactor guest_wr_and_rd_msrs() function to simplify logic and improve
>   readability. (Sean)
> - Use GUEST_ASSERT_EQ() directly instead of passing the counter to ucall and
>   back to the host. (Sean)
> - Refactor test_intel_oob_fixed_ctr() test method. (Sean)
> - Avoid using half-baked helpers and optimize the code structure. (Sean)
> - Update variable names for better readability and consistency. (Sean)
> - Rename functions to better reflect their purpose. (Sean)
>
> v3:
> https://lore.kernel.org/kvm/20230814115108.45741-1-cloudliang@tencent.com/T/
>
> Jinrong Liang (9):
>   KVM: selftests: Add vcpu_set_cpuid_property() to set properties
>   KVM: selftests: Extend this_pmu_has() and kvm_pmu_has() to check arch
>     events
>   KVM: selftests: Add pmu.h for PMU events and common masks
>   KVM: selftests: Test Intel PMU architectural events on gp counters
>   KVM: selftests: Test Intel PMU architectural events on fixed counters
>   KVM: selftests: Test consistency of CPUID with num of gp counters
>   KVM: selftests: Test consistency of CPUID with num of fixed counters
>   KVM: selftests: Test Intel supported fixed counters bit mask
>   KVM: selftests: Test consistency of PMU MSRs with Intel PMU version
>
>  tools/testing/selftests/kvm/Makefile          |   2 +
>  tools/testing/selftests/kvm/include/pmu.h     |  96 ++++
>  .../selftests/kvm/include/x86_64/processor.h  |  42 +-
>  tools/testing/selftests/kvm/lib/pmu.c         |  38 ++
>  .../selftests/kvm/lib/x86_64/processor.c      |  14 +
>  .../selftests/kvm/x86_64/pmu_counters_test.c  | 431 ++++++++++++++++++
>  .../kvm/x86_64/pmu_event_filter_test.c        |  34 +-
>  7 files changed, 623 insertions(+), 34 deletions(-)
>  create mode 100644 tools/testing/selftests/kvm/include/pmu.h
>  create mode 100644 tools/testing/selftests/kvm/lib/pmu.c
>  create mode 100644 tools/testing/selftests/kvm/x86_64/pmu_counters_test.c
>
>
> base-commit: e2013f46ee2e721567783557c301e5c91d0b74ff
> --
> 2.39.3
>

Re: [PATCH v4 1/9] KVM: selftests: Add vcpu_set_cpuid_property() to set properties

[Sean Christopherson]

On Mon, Sep 11, 2023, Jinrong Liang wrote:
> From: Jinrong Liang <cloudliang@tencent.com>
> 
> Add vcpu_set_cpuid_property() helper function for setting properties,
> which simplifies the process of setting CPUID properties for vCPUs.
> 
> Suggested-by: Sean Christopherson <seanjc@google.com>
> Signed-off-by: Jinrong Liang <cloudliang@tencent.com>
> ---
>  .../selftests/kvm/include/x86_64/processor.h       |  4 ++++
>  tools/testing/selftests/kvm/lib/x86_64/processor.c | 14 ++++++++++++++
>  2 files changed, 18 insertions(+)
> 
> diff --git a/tools/testing/selftests/kvm/include/x86_64/processor.h b/tools/testing/selftests/kvm/include/x86_64/processor.h
> index 4fd042112526..6b146e1c6736 100644
> --- a/tools/testing/selftests/kvm/include/x86_64/processor.h
> +++ b/tools/testing/selftests/kvm/include/x86_64/processor.h
> @@ -973,6 +973,10 @@ static inline void vcpu_set_cpuid(struct kvm_vcpu *vcpu)
>  
>  void vcpu_set_cpuid_maxphyaddr(struct kvm_vcpu *vcpu, uint8_t maxphyaddr);
>  
> +void vcpu_set_cpuid_property(struct kvm_vcpu *vcpu,
> +			     struct kvm_x86_cpu_property property,
> +			     uint32_t value);

The vcpu_set_cpuid_maxphyaddr() helper right above this can and should be converted
as part of this patch.  X86_PROPERTY_MAX_PHY_ADDR is already defined, i.e. it's a
trivial conversion, and that way there's an immediate user of the the new helper.

Re: [PATCH v4 2/9] KVM: selftests: Extend this_pmu_has() and kvm_pmu_has() to check arch events

[Sean Christopherson]

On Mon, Sep 11, 2023, Jinrong Liang wrote:
> From: Jinrong Liang <cloudliang@tencent.com>
> 
> The kvm_x86_pmu_feature struct has been updated to use the more
> descriptive name "pmu_feature" instead of "anti_feature".
> 
> Extend this_pmu_has() and kvm_pmu_has() functions to better support
> checking for Intel architectural events. Rename this_pmu_has() and
> kvm_pmu_has() to this_pmu_has_arch_event() and kvm_pmu_has_arch_event().
> 
> Suggested-by: Sean Christopherson <seanjc@google.com>
> Signed-off-by: Jinrong Liang <cloudliang@tencent.com>
> ---
>  .../selftests/kvm/include/x86_64/processor.h  | 38 ++++++++++++++-----
>  .../kvm/x86_64/pmu_event_filter_test.c        |  2 +-
>  2 files changed, 29 insertions(+), 11 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/include/x86_64/processor.h b/tools/testing/selftests/kvm/include/x86_64/processor.h
> index 6b146e1c6736..ede433eb6541 100644
> --- a/tools/testing/selftests/kvm/include/x86_64/processor.h
> +++ b/tools/testing/selftests/kvm/include/x86_64/processor.h
> @@ -280,12 +280,12 @@ struct kvm_x86_cpu_property {
>   * architectural event is supported.
>   */
>  struct kvm_x86_pmu_feature {
> -	struct kvm_x86_cpu_feature anti_feature;
> +	struct kvm_x86_cpu_feature pmu_feature;

Eh, looking at this with fresh eyes, let's just use a single character to keep
the line lengths as short as possible.  There was value in the anti_feature name,
but pmu_feature doesn't add anything IMO.

>  };
>  #define	KVM_X86_PMU_FEATURE(name, __bit)					\
>  ({										\
>  	struct kvm_x86_pmu_feature feature = {					\
> -		.anti_feature = KVM_X86_CPU_FEATURE(0xa, 0, EBX, __bit),	\
> +		.pmu_feature = KVM_X86_CPU_FEATURE(0xa, 0, EBX, __bit),		\

This needs to take in the register (EBX vs. ECX) for this helper to be useful.

>  	};									\
>  										\
>  	feature;								\
> @@ -681,12 +681,21 @@ static __always_inline bool this_cpu_has_p(struct kvm_x86_cpu_property property)
>  	return max_leaf >= property.function;
>  }
>  
> -static inline bool this_pmu_has(struct kvm_x86_pmu_feature feature)
> +static inline bool this_pmu_has_arch_event(struct kvm_x86_pmu_feature feature)

Why?  I don't see the point.  And it's confusing for fixed counters.  Yeah, fixed
counters count architectural events, but the code is asking if a _counter_ is
supported, not if the associated event is supported.  And the darn name gets too
long, too.

>  {
> -	uint32_t nr_bits = this_cpu_property(X86_PROPERTY_PMU_EBX_BIT_VECTOR_LENGTH);
> +	uint32_t nr_bits;
>  
> -	return nr_bits > feature.anti_feature.bit &&
> -	       !this_cpu_has(feature.anti_feature);
> +	if (feature.pmu_feature.reg == KVM_CPUID_EBX) {
> +		nr_bits = this_cpu_property(X86_PROPERTY_PMU_EBX_BIT_VECTOR_LENGTH);
> +		return nr_bits > feature.pmu_feature.bit &&
> +			!this_cpu_has(feature.pmu_feature);
> +	} else if (feature.pmu_feature.reg == KVM_CPUID_ECX) {
> +		nr_bits = this_cpu_property(X86_PROPERTY_PMU_NR_FIXED_COUNTERS);
> +		return nr_bits > feature.pmu_feature.bit ||
> +			this_cpu_has(feature.pmu_feature);
> +	} else {
> +		TEST_FAIL("Invalid register in kvm_x86_pmu_feature");

This needs to be a GUEST_ASSERT(), as the primary usage is in the guest.

And again looking at this with fresh eyes, I'd rather do

	uint32_t nr_bits;

	if (feature.f.reg == KVM_CPUID_EBX) {
		nr_bits = this_cpu_property(X86_PROPERTY_PMU_EBX_BIT_VECTOR_LENGTH);
		return nr_bits > feature.f.bit && !this_cpu_has(feature.f);
	}

	GUEST_ASSERT(feature.f.reg == KVM_CPUID_ECX);
	nr_bits = this_cpu_property(X86_PROPERTY_PMU_NR_FIXED_COUNTERS);
	return nr_bits > feature.f.bit || this_cpu_has(feature.f);

so that the bogus register is printed out on failure.

> +	}
>  }
>  
>  static __always_inline uint64_t this_cpu_supported_xcr0(void)
> @@ -900,12 +909,21 @@ static __always_inline bool kvm_cpu_has_p(struct kvm_x86_cpu_property property)
>  	return max_leaf >= property.function;
>  }
>  
> -static inline bool kvm_pmu_has(struct kvm_x86_pmu_feature feature)
> +static inline bool kvm_pmu_has_arch_event(struct kvm_x86_pmu_feature feature)
>  {
> -	uint32_t nr_bits = kvm_cpu_property(X86_PROPERTY_PMU_EBX_BIT_VECTOR_LENGTH);
> +	uint32_t nr_bits;
>  
> -	return nr_bits > feature.anti_feature.bit &&
> -	       !kvm_cpu_has(feature.anti_feature);
> +	if (feature.pmu_feature.reg == KVM_CPUID_EBX) {
> +		nr_bits = kvm_cpu_property(X86_PROPERTY_PMU_EBX_BIT_VECTOR_LENGTH);
> +		return nr_bits > feature.pmu_feature.bit &&
> +			!kvm_cpu_has(feature.pmu_feature);
> +	} else if (feature.pmu_feature.reg == KVM_CPUID_ECX) {
> +		nr_bits = kvm_cpu_property(X86_PROPERTY_PMU_NR_FIXED_COUNTERS);
> +		return nr_bits > feature.pmu_feature.bit ||
> +			kvm_cpu_has(feature.pmu_feature);
> +	} else {
> +		TEST_FAIL("Invalid register in kvm_x86_pmu_feature");

Same thing here.

> +	}

Re: [PATCH v4 3/9] KVM: selftests: Add pmu.h for PMU events and common masks

[Sean Christopherson]

Shortlog should call out lib/pmu.c (or just say "a library", but naming files is
fine).

On Mon, Sep 11, 2023, Jinrong Liang wrote:
> +#define PMU_CAP_FW_WRITES				BIT_ULL(13)
> +#define PMU_CAP_LBR_FMT				0x3f
> +
> +enum intel_pmu_architectural_events {
> +	/*
> +	 * The order of the architectural events matters as support for each
> +	 * event is enumerated via CPUID using the index of the event.
> +	 */
> +	INTEL_ARCH_CPU_CYCLES,
> +	INTEL_ARCH_INSTRUCTIONS_RETIRED,
> +	INTEL_ARCH_REFERENCE_CYCLES,
> +	INTEL_ARCH_LLC_REFERENCES,
> +	INTEL_ARCH_LLC_MISSES,
> +	INTEL_ARCH_BRANCHES_RETIRED,
> +	INTEL_ARCH_BRANCHES_MISPREDICTED,
> +
> +	NR_REAL_INTEL_ARCH_EVENTS,
> +
> +	/*
> +	 * Pseudo-architectural event used to implement IA32_FIXED_CTR2, a.k.a.
> +	 * TSC reference cycles. The architectural reference cycles event may
> +	 * or may not actually use the TSC as the reference, e.g. might use the
> +	 * core crystal clock or the bus clock (yeah, "architectural").
> +	 */
> +	PSEUDO_ARCH_REFERENCE_CYCLES = NR_REAL_INTEL_ARCH_EVENTS,
> +	NR_INTEL_ARCH_EVENTS,
> +};

...

> +/* Definitions for Architectural Performance Events */
> +#define ARCH_EVENT(select, umask) (((select) & 0xff) | ((umask) & 0xff) << 8)
> +
> +const uint64_t intel_pmu_arch_events[] = {
> +	[INTEL_ARCH_CPU_CYCLES]			= ARCH_EVENT(0x3c, 0x0),
> +	[INTEL_ARCH_INSTRUCTIONS_RETIRED]	= ARCH_EVENT(0xc0, 0x0),
> +	[INTEL_ARCH_REFERENCE_CYCLES]		= ARCH_EVENT(0x3c, 0x1),
> +	[INTEL_ARCH_LLC_REFERENCES]		= ARCH_EVENT(0x2e, 0x4f),
> +	[INTEL_ARCH_LLC_MISSES]			= ARCH_EVENT(0x2e, 0x41),
> +	[INTEL_ARCH_BRANCHES_RETIRED]		= ARCH_EVENT(0xc4, 0x0),
> +	[INTEL_ARCH_BRANCHES_MISPREDICTED]	= ARCH_EVENT(0xc5, 0x0),
> +	[PSEUDO_ARCH_REFERENCE_CYCLES]		= ARCH_EVENT(0xa4, 0x1),

Argh, seriously, why!?!?!  0xa4, 0x1 is Topdown Slots, not TSC reference cycles.
There is _zero_ reason to carry over KVM's godawful pseudo-architectural event
crud that exists purely because KVM uses perf events to virtualized PMCs.

And peeking forward at the usage, the whole thing is completely nonsensical, as
the selftest will set expectations on the TSC reference cycles behavior based
on whether or not the Topdown Slots event is supported.

Grr.

Re: [PATCH v4 4/9] KVM: selftests: Test Intel PMU architectural events on gp counters

[Sean Christopherson]

On Mon, Sep 11, 2023, Jinrong Liang wrote:
> +static void test_intel_arch_events(void)
> +{
> +	uint8_t idx;
> +
> +	for (idx = 0; idx < NR_INTEL_ARCH_EVENTS; idx++) {
> +		/*
> +		 * Given the stability of performance event recurrence, only
> +		 * these arch events are currently being tested:
> +		 *
> +		 * - Core cycle event (idx = 0)
> +		 * - Instruction retired event (idx = 1)
> +		 * - Reference cycles event (idx = 2)
> +		 * - Branch instruction retired event (idx = 5)
> +		 */
> +		if (idx > INTEL_ARCH_INSTRUCTIONS_RETIRED &&
> +		    idx != INTEL_ARCH_BRANCHES_RETIRED)
> +			continue;
> +
> +		check_arch_event_is_unavl(idx);

Rather than completely skip the event, just don't check the counter.  That way
the test still verifies that it can program the event, it's only the result that
isn't stable enough to assert on.

Re: [PATCH v4 5/9] KVM: selftests: Test Intel PMU architectural events on fixed counters

[Sean Christopherson]

On Mon, Sep 11, 2023, Jinrong Liang wrote:
> From: Jinrong Liang <cloudliang@tencent.com>
> 
> Update test to cover Intel PMU architectural events on fixed counters.
> Per Intel SDM, PMU users can also count architecture performance events
> on fixed counters (specifically, FIXED_CTR0 for the retired instructions
> and FIXED_CTR1 for cpu core cycles event). Therefore, if guest's CPUID
> indicates that an architecture event is not available, the corresponding
> fixed counter will also not count that event.
> 
> Co-developed-by: Like Xu <likexu@tencent.com>
> Signed-off-by: Like Xu <likexu@tencent.com>
> Signed-off-by: Jinrong Liang <cloudliang@tencent.com>
> ---
>  .../selftests/kvm/x86_64/pmu_counters_test.c  | 22 +++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/tools/testing/selftests/kvm/x86_64/pmu_counters_test.c b/tools/testing/selftests/kvm/x86_64/pmu_counters_test.c
> index f47853f3ab84..fe9f38a3557e 100644
> --- a/tools/testing/selftests/kvm/x86_64/pmu_counters_test.c
> +++ b/tools/testing/selftests/kvm/x86_64/pmu_counters_test.c
> @@ -106,6 +106,28 @@ static void guest_measure_loop(uint8_t idx)
>  		GUEST_ASSERT_EQ(expect, !!_rdpmc(i));
>  	}
>  
> +	if (this_cpu_property(X86_PROPERTY_PMU_NR_FIXED_COUNTERS) < 1)

The ENTIRE point of reworking this_pmu_has() to be able to query fixed counters
was to avoid having to open code checks like this.  And this has no basis in
reality, the fixed counters aren't all-or-nothing, e.g. if the number of fixed
counters is '1', then the below will explode because the test will try to write
a non-existent MSR.

> +		goto done;
> +
> +	if (idx == INTEL_ARCH_INSTRUCTIONS_RETIRED)
> +		i = 0;
> +	else if (idx == INTEL_ARCH_CPU_CYCLES)
> +		i = 1;
> +	else if (idx == PSEUDO_ARCH_REFERENCE_CYCLES)
> +		i = 2;
> +	else
> +		goto done;
> +
> +	wrmsr(MSR_CORE_PERF_FIXED_CTR0 + i, 0);
> +	wrmsr(MSR_CORE_PERF_FIXED_CTR_CTRL, BIT_ULL(4 * i));
> +
> +	wrmsr(MSR_CORE_PERF_GLOBAL_CTRL, BIT_ULL(PMC_IDX_FIXED + i));
> +	__asm__ __volatile__("loop ." : "+c"((int){NUM_BRANCHES}));
> +	wrmsr(MSR_CORE_PERF_GLOBAL_CTRL, 0);
> +
> +	GUEST_ASSERT_EQ(expect, !!_rdpmc(PMC_FIXED_RDPMC_BASE | i));

Pulling in context from the previous patch, "expect" is set based on the existence
of the architectural event in CPUID.0xA.EBX.  That is completely bogus, especially
for TSC reference cycles which has been incorrectly aliased to Topdown Slots.

	expect = this_pmu_has_arch_event(KVM_X86_PMU_FEATURE(UNUSED, idx));

Nothing in the SDM says that an event that's marked unavailable in CPUID.0xA.EBX
magically makes the fixed counter not work.  It's a rather bogus CPUID, e.g. I
can't imagine real hardware has any such setup, but silently not counting is most
definitely not correct.

Digging around in KVM, I see that KVM _deliberately_ provides this behavior.  That
is just bogus.  If the guest can enable a fixed counter, then it should count.
*If* the interpretation of the SDM is that the fixed counter isn't available when
the associated architectural event isn't available, then the most sane behavior
is to not allow the fixed counter to be enabled in the first place.  Silently
doing nothing is awful.  And again the whole Topdown Slots vs. TSC ref cycles
confusion means this is completely broken regardless of how one interprets the
SDM.

And the above on-demand creation of each KVM_X86_PMU_FEATURE() kinda defeats the
purpose of using well-known names.  Rather than smush everything into the general
purpose architectural events, which is definitely broken and arguably a straight
violation of the SDM, I think the best option is to loosely couple the GP vs.
fixed events so that we can reason about the high-level event type, e.g. to
determine which events are "stable" enough to assert on.

It's not the prettiest due to not being able to directly compare structs, but it
at least allows checking for architectural events vs. fixed counters independently,
without completely losing the common bits.

#define X86_PMU_FEATURE_NULL						\
({									\
	struct kvm_x86_pmu_feature feature = {};			\
									\
	feature;							\
})

static bool pmu_is_null_feature(struct kvm_x86_pmu_feature event)
{
	return !(*(u64 *)&event);
}

static void guest_measure_loop(uint8_t idx)
{
	const struct {
		struct kvm_x86_pmu_feature gp_event;
		struct kvm_x86_pmu_feature fixed_event;
	} intel_event_to_feature[] = {
		[INTEL_ARCH_CPU_CYCLES]		   = { X86_PMU_FEATURE_CPU_CYCLES, X86_PMU_FEATURE_CPU_CYCLES_FIXED },
		[INTEL_ARCH_INSTRUCTIONS_RETIRED]  = { X86_PMU_FEATURE_INSNS_RETIRED, X86_PMU_FEATURE_INSNS_RETIRED_FIXED },
		/*
		 * Note, the fixed counter for reference cycles is NOT the same
		 * as the general purpose architectural event (because the GP
		 * event is garbage).  The fixed counter explicitly counts at
		 * the same frequency as the TSC, whereas the GP event counts
		 * at a fixed, but uarch specific, frequency.  Bundle them here
		 * for simplicity.
		 */
		[INTEL_ARCH_REFERENCE_CYCLES]	   = { X86_PMU_FEATURE_REFERENCE_CYCLES, X86_PMU_FEATURE_REFERENCE_CYCLES_FIXED },
		[INTEL_ARCH_LLC_REFERENCES]	   = { X86_PMU_FEATURE_LLC_REFERENCES, X86_PMU_FEATURE_NULL },
		[INTEL_ARCH_LLC_MISSES]		   = { X86_PMU_FEATURE_LLC_MISSES, X86_PMU_FEATURE_NULL },
		[INTEL_ARCH_BRANCHES_RETIRED]	   = { X86_PMU_FEATURE_BRANCH_INSNS_RETIRED, X86_PMU_FEATURE_NULL },
		[INTEL_ARCH_BRANCHES_MISPREDICTED] = { X86_PMU_FEATURE_BRANCHES_MISPREDICTED, X86_PMU_FEATURE_NULL },
	};

Re: [PATCH v4 8/9] KVM: selftests: Test Intel supported fixed counters bit mask

[Sean Christopherson]

On Mon, Sep 11, 2023, Jinrong Liang wrote:
> +static void test_fixed_counters(void)
> +{
> +	uint8_t nr_fixed_counters = kvm_cpu_property(X86_PROPERTY_PMU_NR_FIXED_COUNTERS);
> +	uint32_t ecx;
> +	uint8_t edx;
> +
> +	for (edx = 0; edx <= nr_fixed_counters; edx++)
> +		/* KVM doesn't emulate more fixed counters than it can support. */
> +		for (ecx = 0; ecx <= (BIT_ULL(nr_fixed_counters) - 1); ecx++)
> +			__test_fixed_counters(ecx, edx);

Outer for-loop needs curly braces.

Re: [PATCH v4 0/9] KVM: selftests: Test the consistency of the PMU's CPUID and its features

[Sean Christopherson]

On Mon, Sep 11, 2023, Jinrong Liang wrote:
> Jinrong Liang (9):
>   KVM: selftests: Add vcpu_set_cpuid_property() to set properties
>   KVM: selftests: Extend this_pmu_has() and kvm_pmu_has() to check arch
>     events
>   KVM: selftests: Add pmu.h for PMU events and common masks
>   KVM: selftests: Test Intel PMU architectural events on gp counters
>   KVM: selftests: Test Intel PMU architectural events on fixed counters
>   KVM: selftests: Test consistency of CPUID with num of gp counters
>   KVM: selftests: Test consistency of CPUID with num of fixed counters
>   KVM: selftests: Test Intel supported fixed counters bit mask
>   KVM: selftests: Test consistency of PMU MSRs with Intel PMU version

I've pushed a modified version to 

  https://github.com/sean-jc/linux/branches x86/pmu_counter_tests

which also has fixes for KVM's funky handling of fixed counters.  I'll wait for
you to respond, but will tentatively plan on posting the above branch as v5
some time next week.

Re: [PATCH v4 0/9] KVM: selftests: Test the consistency of the PMU's CPUID and its features

[Jinrong Liang]

Sean Christopherson <seanjc@google.com> 于2023年10月20日周五 08:28写道：
>
> On Mon, Sep 11, 2023, Jinrong Liang wrote:
> > Jinrong Liang (9):
> >   KVM: selftests: Add vcpu_set_cpuid_property() to set properties
> >   KVM: selftests: Extend this_pmu_has() and kvm_pmu_has() to check arch
> >     events
> >   KVM: selftests: Add pmu.h for PMU events and common masks
> >   KVM: selftests: Test Intel PMU architectural events on gp counters
> >   KVM: selftests: Test Intel PMU architectural events on fixed counters
> >   KVM: selftests: Test consistency of CPUID with num of gp counters
> >   KVM: selftests: Test consistency of CPUID with num of fixed counters
> >   KVM: selftests: Test Intel supported fixed counters bit mask
> >   KVM: selftests: Test consistency of PMU MSRs with Intel PMU version
>
> I've pushed a modified version to
>
>   https://github.com/sean-jc/linux/branches x86/pmu_counter_tests
>
> which also has fixes for KVM's funky handling of fixed counters.  I'll wait for
> you to respond, but will tentatively plan on posting the above branch as v5
> some time next week.

I truly appreciate your time and effort in reviewing my patches and
making the necessary modifications. I've carefully examined the
updated code in the branch you kindly provided:

https://github.com/sean-jc/linux/branches x86/pmu_counter_tests

I completely agree with the changes you made. Please feel free to post
the modified branch as v5 next week. I will add AMD counters related
selftests after this patch set is merged.

Thank you once again for your time and guidance.

Re: [PATCH v4 6/9] KVM: selftests: Test consistency of CPUID with num of gp counters

[Sean Christopherson]

On Mon, Sep 11, 2023, Jinrong Liang wrote:
> From: Jinrong Liang <cloudliang@tencent.com>
> 
> Add test to check if non-existent counters can be accessed in guest after
> determining the number of Intel generic performance counters by CPUID.
> When the num of counters is less than 3, KVM does not emulate #GP if
> a counter isn't present due to compatibility MSR_P6_PERFCTRx handling.
> Nor will the KVM emulate more counters than it can support.
> 
> Co-developed-by: Like Xu <likexu@tencent.com>
> Signed-off-by: Like Xu <likexu@tencent.com>
> Signed-off-by: Jinrong Liang <cloudliang@tencent.com>
> ---
>  .../selftests/kvm/x86_64/pmu_counters_test.c  | 85 +++++++++++++++++++
>  1 file changed, 85 insertions(+)
> 
> diff --git a/tools/testing/selftests/kvm/x86_64/pmu_counters_test.c b/tools/testing/selftests/kvm/x86_64/pmu_counters_test.c
> index fe9f38a3557e..e636323e202c 100644
> --- a/tools/testing/selftests/kvm/x86_64/pmu_counters_test.c
> +++ b/tools/testing/selftests/kvm/x86_64/pmu_counters_test.c
> @@ -17,6 +17,11 @@
>  /* Guest payload for any performance counter counting */
>  #define NUM_BRANCHES		10
>  
> +static const uint64_t perf_caps[] = {
> +	0,
> +	PMU_CAP_FW_WRITES,
> +};

Put this on the stack in the one testcase that uses it.  Placing the array super
far away from its use makes it unnecessarily difficult to see that the testcase
is simply running with and without full-width writes.

>  static struct kvm_vm *pmu_vm_create_with_one_vcpu(struct kvm_vcpu **vcpu,
>  						  void *guest_code)
>  {
> @@ -189,6 +194,85 @@ static void test_intel_arch_events(void)
>  	}
>  }
>  
> +static void __guest_wrmsr_rdmsr(uint32_t counter_msr, uint8_t nr_msrs,
> +				bool expect_gp)

Rather than pass in "expect_gp", compute it in here.  It's easy enough to explicitly
check for MSR_P6_PERFCTR[0|1]

> +{
> +	uint64_t msr_val;
> +	uint8_t vector;
> +
> +	vector = wrmsr_safe(counter_msr + nr_msrs, 0xffff);

Doing all this work to test _one_ MSR at a time is silly.  And I see no reason
to do only negative testing.  Sure, postive testing might be redundant with other
tests (I truly don't know), but _not_ hardcoding one-off tests often ends up
requiring less code, and almost always results in more self-documenting code.
E.g. it took me far too much staring to understand why the "no #GP" case expects
to read back '0'.

> +	__GUEST_ASSERT(expect_gp ? vector == GP_VECTOR : !vector,
> +		       "Expected GP_VECTOR");

Print the actual vector!  And the MSR!  One of my pet peeves with KVM's tests is
not providing information on failure.  Having to hack a test or do interactive
debug just to figure out which MSR failed is *super* frustrating.

And I think it's worth providing a macro to handle the assertion+message, that
way it'll be easier to add more sub-tests, e.g. that the MSR can be written back
to '0'.

> +
> +	vector = rdmsr_safe(counter_msr + nr_msrs, &msr_val);
> +	__GUEST_ASSERT(expect_gp ? vector == GP_VECTOR : !vector,
> +		       "Expected GP_VECTOR");
> +
> +	if (!expect_gp)
> +		GUEST_ASSERT_EQ(msr_val, 0);
> +
> +	GUEST_DONE();
> +}
> +
> +static void guest_rd_wr_gp_counter(void)
> +{
> +	uint8_t nr_gp_counters = this_cpu_property(X86_PROPERTY_PMU_NR_GP_COUNTERS);
> +	uint64_t perf_capabilities = rdmsr(MSR_IA32_PERF_CAPABILITIES);
> +	uint32_t counter_msr;
> +	bool expect_gp = true;
> +
> +	if (perf_capabilities & PMU_CAP_FW_WRITES) {
> +		counter_msr = MSR_IA32_PMC0;
> +	} else {
> +		counter_msr = MSR_IA32_PERFCTR0;
> +
> +		/* KVM drops writes to MSR_P6_PERFCTR[0|1]. */
> +		if (nr_gp_counters == 0)
> +			expect_gp = false;
> +	}
> +
> +	__guest_wrmsr_rdmsr(counter_msr, nr_gp_counters, expect_gp);
> +}
> +
> +/* Access the first out-of-range counter register to trigger #GP */
> +static void test_oob_gp_counter(uint8_t eax_gp_num, uint64_t perf_cap)
> +{
> +	struct kvm_vcpu *vcpu;
> +	struct kvm_vm *vm;
> +
> +	vm = pmu_vm_create_with_one_vcpu(&vcpu, guest_rd_wr_gp_counter);
> +
> +	vcpu_set_cpuid_property(vcpu, X86_PROPERTY_PMU_NR_GP_COUNTERS,
> +				eax_gp_num);
> +	vcpu_set_msr(vcpu, MSR_IA32_PERF_CAPABILITIES, perf_cap);
> +
> +	run_vcpu(vcpu);
> +
> +	kvm_vm_free(vm);
> +}
> +
> +static void test_intel_counters_num(void)
> +{
> +	uint8_t nr_gp_counters = kvm_cpu_property(X86_PROPERTY_PMU_NR_GP_COUNTERS);
> +	unsigned int i;
> +
> +	TEST_REQUIRE(nr_gp_counters > 2);

This is beyond silly.  Just iterate over all possible counter values.  Again,
hardcoding values is almost never the best way to do things.

> +
> +	for (i = 0; i < ARRAY_SIZE(perf_caps); i++) {
> +		/*
> +		 * For compatibility reasons, KVM does not emulate #GP
> +		 * when MSR_P6_PERFCTR[0|1] is not present, but it doesn't
> +		 * affect checking the presence of MSR_IA32_PMCx with #GP.
> +		 */
> +		test_oob_gp_counter(0, perf_caps[i]);
> +		test_oob_gp_counter(2, perf_caps[i]);
> +		test_oob_gp_counter(nr_gp_counters, perf_caps[i]);
> +
> +		/* KVM doesn't emulate more counters than it can support. */
> +		test_oob_gp_counter(nr_gp_counters + 1, perf_caps[i]);

Hmm, so I think we should avoid blindly testing undefined MSRs.  I don't disagree
that expecting #GP is reasonable, but I don't think this is the right place to
test for architecturally undefined MSRs.  E.g. if Intel defines some completely
unrelated MSR at 0xc3 or 0x4c9 then this test will fail.

Rather than assume anything about "nr_gp_counters + 1", I think we should test up
to what Intel has architecturally defined, e.g. define the max number of counters
and then pass that in as the "possible" counters:

#define GUEST_ASSERT_PMC_MSR_ACCESS(insn, msr, expect_gp, vector)		\
__GUEST_ASSERT(expect_gp ? vector == GP_VECTOR : !vector,			\
	       "Expected %s on " #insn "(0x%x), got vector %u",			\
	       expect_gp ? "#GP" : "no fault", msr, vector)			\

static void guest_rd_wr_counters(uint32_t base_msr, uint8_t nr_possible_counters,
				 uint8_t nr_counters, uint32_t or_mask)
{
	uint8_t i;

	for (i = 0; i < nr_possible_counters; i++) {
		const uint32_t msr = base_msr + i;

		/*
		 * Fixed counters are supported if the counter is less than the
		 * number of enumerated contiguous counters *or* the counter is
		 * explicitly enumerated in the supported counters mask.
		 */
		const bool expect_success = i < nr_counters || (or_mask & BIT(i));

		/*
		 * KVM drops writes to MSR_P6_PERFCTR[0|1] if the counters are
		 * unsupported, i.e. doesn't #GP and reads back '0'.
		 */
		const uint64_t expected_val = expect_success ? 0xffff : 0;
		const bool expect_gp = !expect_success && msr != MSR_P6_PERFCTR0 &&
				       msr != MSR_P6_PERFCTR1;
		uint8_t vector;
		uint64_t val;

		vector = wrmsr_safe(msr, 0xffff);
		GUEST_ASSERT_PMC_MSR_ACCESS(WRMSR, msr, expect_gp, vector);

		vector = rdmsr_safe(msr, &val);
		GUEST_ASSERT_PMC_MSR_ACCESS(RDMSR, msr, expect_gp, vector);

		/* On #GP, the result of RDMSR is undefined. */
		if (!expect_gp)
			__GUEST_ASSERT(val == expected_val,
				       "Expected RDMSR(0x%x) to yield 0x%lx, got 0x%lx",
				       msr, expected_val, val);

		vector = wrmsr_safe(msr, 0);
		GUEST_ASSERT_PMC_MSR_ACCESS(WRMSR, msr, expect_gp, vector);
	}
}

Re: [PATCH v4 8/9] KVM: selftests: Test Intel supported fixed counters bit mask

[Sean Christopherson]

On Mon, Sep 11, 2023, Jinrong Liang wrote:
> From: Jinrong Liang <cloudliang@tencent.com>
> 
> Add a test to check that fixed counters enabled via guest
> CPUID.0xA.ECX (instead of EDX[04:00]) work as normal as usual.
> 
> Co-developed-by: Like Xu <likexu@tencent.com>
> Signed-off-by: Like Xu <likexu@tencent.com>
> Signed-off-by: Jinrong Liang <cloudliang@tencent.com>
> ---
>  .../selftests/kvm/x86_64/pmu_counters_test.c  | 54 +++++++++++++++++++
>  1 file changed, 54 insertions(+)
> 
> diff --git a/tools/testing/selftests/kvm/x86_64/pmu_counters_test.c b/tools/testing/selftests/kvm/x86_64/pmu_counters_test.c
> index df76f0f2bfd0..12c00bf94683 100644
> --- a/tools/testing/selftests/kvm/x86_64/pmu_counters_test.c
> +++ b/tools/testing/selftests/kvm/x86_64/pmu_counters_test.c
> @@ -301,6 +301,59 @@ static void test_intel_counters_num(void)
>  	test_oob_fixed_ctr(nr_fixed_counters + 1);
>  }
>  
> +static void fixed_counters_guest_code(void)
> +{
> +	uint64_t supported_bitmask = this_cpu_property(X86_PROPERTY_PMU_FIXED_COUNTERS_BITMASK);
> +	uint32_t nr_fixed_counter = this_cpu_property(X86_PROPERTY_PMU_NR_FIXED_COUNTERS);
> +	uint64_t msr_val;
> +	unsigned int i;
> +	bool expected;
> +
> +	for (i = 0; i < nr_fixed_counter; i++) {
> +		expected = supported_bitmask & BIT_ULL(i) || i < nr_fixed_counter;
> +
> +		wrmsr_safe(MSR_CORE_PERF_FIXED_CTR0 + i, 0);
> +		wrmsr_safe(MSR_CORE_PERF_FIXED_CTR_CTRL, BIT_ULL(4 * i));
> +		wrmsr_safe(MSR_CORE_PERF_GLOBAL_CTRL, BIT_ULL(PMC_IDX_FIXED + i));
> +		__asm__ __volatile__("loop ." : "+c"((int){NUM_BRANCHES}));
> +		wrmsr_safe(MSR_CORE_PERF_GLOBAL_CTRL, 0);
> +		rdmsr_safe(MSR_CORE_PERF_FIXED_CTR0 + i, &msr_val);

Y'all are making this way harder than it needs to be.  The previous patch already
created a testcase to verify fixed counters, just use that!  Then test case verify
that trying to enable unsupported fixed counters results in #GP, as opposed to the
above which doesn't do any actual checking, e.g. KVM could completely botch the
{RD,WR}MSR emulation but pass the test by not programming up a counter in perf.

I.e. rather than have a separate test for the supported bitmask goofiness, have
the fixed counters test iterate over the bitmask.  And then add a patch to verify
the counters can be enabled and actually count.

And peeking ahead at the vPMU version test, it's the exact same story there.
Instead of hardcoding one-off tests, iterate on the version.  The end result is
that the test provides _more_ coverage with _less_ code.  And without any of the
hardcoded magic that takes a crystal ball to understand.

*sigh* 

And even more importantly, this test is complete garbage.  The SDM clearly states
that 

  With Architectural Performance Monitoring Version 5, register CPUID.0AH.ECX
  indicates Fixed Counter enumeration. It is a bit mask which enumerates the
  supported Fixed Counters in a processor. If bit 'i' is set, it implies that
  Fixed Counter 'i' is supported.

*sigh*

The test passes because it only iterates over counters < nr_fixed_counter.  So
as written, the test worse than useless.  It provides no meaningful value and is
actively misleading.

	for (i = 0; i < nr_fixed_counter; i++) {

Maybe I haven't been explicit enough: the point of writing tests is to find and
prevent bugs, not to get the tests passing.  That isn't to say we don't want a
clean testgrid, but writing a "test" that doesn't actually test anything is a
waste of everyone's time.

I appreciate that the PMU is subtle and complex (understatement), but things like
this, where observing that the result of "supported_bitmask & BIT_ULL(i)" doesn't
actually affect anything, doesn't require PMU knowledge.

	for (i = 0; i < nr_fixed_counter; i++) {                                
		expected = supported_bitmask & BIT_ULL(i) || i < nr_fixed_counter;

A concrete suggestion for writing tests: introduce bugs in what you're testing
and verify that the test actually detects the bugs.  If you tried to do that for
the above bitmask test you would have discovered you can't break KVM because KVM
doesn't support this!  And if your test doesn't detect the bugs, that should also
be a big clue that something isn't quite right.

Re: [PATCH v4 8/9] KVM: selftests: Test Intel supported fixed counters bit mask

[Jinrong Liang]

Sean Christopherson <seanjc@google.com> 于2023年10月21日周六 03:06写道：
>
> On Mon, Sep 11, 2023, Jinrong Liang wrote:
> > From: Jinrong Liang <cloudliang@tencent.com>
> >
> > Add a test to check that fixed counters enabled via guest
> > CPUID.0xA.ECX (instead of EDX[04:00]) work as normal as usual.
> >
> > Co-developed-by: Like Xu <likexu@tencent.com>
> > Signed-off-by: Like Xu <likexu@tencent.com>
> > Signed-off-by: Jinrong Liang <cloudliang@tencent.com>
> > ---
> >  .../selftests/kvm/x86_64/pmu_counters_test.c  | 54 +++++++++++++++++++
> >  1 file changed, 54 insertions(+)
> >
> > diff --git a/tools/testing/selftests/kvm/x86_64/pmu_counters_test.c b/tools/testing/selftests/kvm/x86_64/pmu_counters_test.c
> > index df76f0f2bfd0..12c00bf94683 100644
> > --- a/tools/testing/selftests/kvm/x86_64/pmu_counters_test.c
> > +++ b/tools/testing/selftests/kvm/x86_64/pmu_counters_test.c
> > @@ -301,6 +301,59 @@ static void test_intel_counters_num(void)
> >       test_oob_fixed_ctr(nr_fixed_counters + 1);
> >  }
> >
> > +static void fixed_counters_guest_code(void)
> > +{
> > +     uint64_t supported_bitmask = this_cpu_property(X86_PROPERTY_PMU_FIXED_COUNTERS_BITMASK);
> > +     uint32_t nr_fixed_counter = this_cpu_property(X86_PROPERTY_PMU_NR_FIXED_COUNTERS);
> > +     uint64_t msr_val;
> > +     unsigned int i;
> > +     bool expected;
> > +
> > +     for (i = 0; i < nr_fixed_counter; i++) {
> > +             expected = supported_bitmask & BIT_ULL(i) || i < nr_fixed_counter;
> > +
> > +             wrmsr_safe(MSR_CORE_PERF_FIXED_CTR0 + i, 0);
> > +             wrmsr_safe(MSR_CORE_PERF_FIXED_CTR_CTRL, BIT_ULL(4 * i));
> > +             wrmsr_safe(MSR_CORE_PERF_GLOBAL_CTRL, BIT_ULL(PMC_IDX_FIXED + i));
> > +             __asm__ __volatile__("loop ." : "+c"((int){NUM_BRANCHES}));
> > +             wrmsr_safe(MSR_CORE_PERF_GLOBAL_CTRL, 0);
> > +             rdmsr_safe(MSR_CORE_PERF_FIXED_CTR0 + i, &msr_val);
>
> Y'all are making this way harder than it needs to be.  The previous patch already
> created a testcase to verify fixed counters, just use that!  Then test case verify
> that trying to enable unsupported fixed counters results in #GP, as opposed to the
> above which doesn't do any actual checking, e.g. KVM could completely botch the
> {RD,WR}MSR emulation but pass the test by not programming up a counter in perf.
>
> I.e. rather than have a separate test for the supported bitmask goofiness, have
> the fixed counters test iterate over the bitmask.  And then add a patch to verify
> the counters can be enabled and actually count.
>
> And peeking ahead at the vPMU version test, it's the exact same story there.
> Instead of hardcoding one-off tests, iterate on the version.  The end result is
> that the test provides _more_ coverage with _less_ code.  And without any of the
> hardcoded magic that takes a crystal ball to understand.
>
> *sigh*
>
> And even more importantly, this test is complete garbage.  The SDM clearly states
> that
>
>   With Architectural Performance Monitoring Version 5, register CPUID.0AH.ECX
>   indicates Fixed Counter enumeration. It is a bit mask which enumerates the
>   supported Fixed Counters in a processor. If bit 'i' is set, it implies that
>   Fixed Counter 'i' is supported.
>
> *sigh*
>
> The test passes because it only iterates over counters < nr_fixed_counter.  So
> as written, the test worse than useless.  It provides no meaningful value and is
> actively misleading.
>
>         for (i = 0; i < nr_fixed_counter; i++) {
>
> Maybe I haven't been explicit enough: the point of writing tests is to find and
> prevent bugs, not to get the tests passing.  That isn't to say we don't want a
> clean testgrid, but writing a "test" that doesn't actually test anything is a
> waste of everyone's time.
>
> I appreciate that the PMU is subtle and complex (understatement), but things like
> this, where observing that the result of "supported_bitmask & BIT_ULL(i)" doesn't
> actually affect anything, doesn't require PMU knowledge.
>
>         for (i = 0; i < nr_fixed_counter; i++) {
>                 expected = supported_bitmask & BIT_ULL(i) || i < nr_fixed_counter;
>
> A concrete suggestion for writing tests: introduce bugs in what you're testing
> and verify that the test actually detects the bugs.  If you tried to do that for
> the above bitmask test you would have discovered you can't break KVM because KVM
> doesn't support this!  And if your test doesn't detect the bugs, that should also
> be a big clue that something isn't quite right.

Thank you for your detailed feedback on my patch series. I truly
appreciate the time and effort you've put into identifying the issues
in my code and providing valuable suggestions for improvement.

Your guidance have been instrumental in helping me understand the
selftests. I will make sure to strive to create more meaningful and
effective contribution in the future.