[PATCH v3 0/4] KVM: selftests: Improve PMU event filter settings and add test cases

[PATCH v3 0/4] KVM: selftests: Improve PMU event filter settings and add test cases

[Jinrong Liang]

Hi,

This patch series aims to improve the PMU event filter settings with a cleaner
and more organized structure and adds several test cases related to PMU event
filters.

The first patch of this series introduces a custom "__kvm_pmu_event_filter"
structure that simplifies the event filter setup and improves overall code
readability and maintainability.

The second patch adds test cases to check that unsupported input values in the
PMU event filters are rejected, covering unsupported "action" values,
unsupported "flags" values, and unsupported "nevents" values, as well as the
setting of non-existent fixed counters in the fixed bitmap.

The third patch includes tests for the PMU event filter's behavior when applied
to fixed performance counters, ensuring the correct operation in cases where no
fixed counters exist (e.g., Intel guest PMU version=1 or AMD guest).

Finally, the fourth patch adds a test to verify that setting both generic and
fixed performance event filters does not impact the consistency of the fixed
performance filter behavior.

These changes help to ensure that KVM's PMU event filter functions as expected
in all supported use cases. These patches have been tested and verified to
function properly.

Any feedback or suggestions are greatly appreciated.

Please note that following patches should be applied before this patch series:

https://lore.kernel.org/kvm/20230530134248.23998-2-cloudliang@tencent.com
https://lore.kernel.org/kvm/20230530134248.23998-3-cloudliang@tencent.com

This will ensure that macro definitions such as X86_INTEL_MAX_FIXED_CTR_NUM,
INTEL_PMC_IDX_FIXED, etc. can be used.

Sincerely,
Jinrong Liang

Changes log:

v3:
- Rebased to 31b4fc3bc64a(tag: kvm-x86-next-2023.06.02).
- Dropped the patch "KVM: selftests: Replace int with uint32_t for nevents". (Sean)
- Dropped the patch "KVM: selftests: Test pmu event filter with incompatible
  kvm_pmu_event_filter". (Sean)
- Introduce __kvm_pmu_event_filter to replace the original method of creating
  PMU event filters. (Sean)
- Use the macro definition of kvm_cpu_property to find the number of supported
  fixed counters instead of calculating it via the vcpu's cpuid. (Sean)
- Remove the wrappers that are single line passthroughs. (Sean)
- Optimize function names and variable names. (Sean)
- Optimize comments to make them more rigorous. (Sean)

v2:
- Wrap the code from the documentation in a block of code. (Bagas Sanjaya)

v1:
https://lore.kernel.org/kvm/20230414110056.19665-1-cloudliang@tencent.com

Jinrong Liang (4):
  KVM: selftests: Introduce __kvm_pmu_event_filter to improved event
    filter settings
  KVM: selftests: Test unavailable event filters are rejected
  KVM: selftests: Check if event filter meets expectations on fixed
    counters
  KVM: selftests: Test gp event filters don't affect fixed event filters

 .../kvm/x86_64/pmu_event_filter_test.c        | 341 +++++++++++++-----
 1 file changed, 246 insertions(+), 95 deletions(-)


base-commit: 31b4fc3bc64aadd660c5bfa5178c86a7ba61e0f7
prerequisite-patch-id: 909d42f185f596d6e5c5b48b33231c89fa5236e4
prerequisite-patch-id: ba0dd0f97d8db0fb6cdf2c7f1e3a60c206fc9784
-- 
2.31.1

[PATCH v3 1/4] KVM: selftests: Introduce __kvm_pmu_event_filter to improved event filter settings

[Jinrong Liang]

From: Jinrong Liang <cloudliang@tencent.com>

Add custom "__kvm_pmu_event_filter" structure to improve pmu event
filter settings. Simplifies event filter setup by organizing event
filter parameters in a cleaner, more organized way. Improves overall
code readability and maintainability.

Signed-off-by: Jinrong Liang <cloudliang@tencent.com>
---
 .../kvm/x86_64/pmu_event_filter_test.c        | 180 +++++++++---------
 1 file changed, 88 insertions(+), 92 deletions(-)

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 40507ed9fe8a..26f674c32cde 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
@@ -28,6 +28,10 @@
 
 #define NUM_BRANCHES 42
 
+/* Matches KVM_PMU_EVENT_FILTER_MAX_EVENTS in pmu.c */
+#define MAX_FILTER_EVENTS	300
+#define MAX_TEST_EVENTS	10
+
 /*
  * This is how the event selector and unit mask are stored in an AMD
  * core performance event-select register. Intel's format is similar,
@@ -69,21 +73,33 @@
 
 #define INST_RETIRED EVENT(0xc0, 0)
 
+struct __kvm_pmu_event_filter {
+	__u32 action;
+	__u32 nevents;
+	__u32 fixed_counter_bitmap;
+	__u32 flags;
+	__u32 pad[4];
+	__u64 events[MAX_FILTER_EVENTS];
+};
+
 /*
  * This event list comprises Intel's eight architectural events plus
  * AMD's "retired branch instructions" for Zen[123] (and possibly
  * other AMD CPUs).
  */
-static const uint64_t event_list[] = {
-	EVENT(0x3c, 0),
-	INST_RETIRED,
-	EVENT(0x3c, 1),
-	EVENT(0x2e, 0x4f),
-	EVENT(0x2e, 0x41),
-	EVENT(0xc4, 0),
-	EVENT(0xc5, 0),
-	EVENT(0xa4, 1),
-	AMD_ZEN_BR_RETIRED,
+static const struct __kvm_pmu_event_filter base_event_filter = {
+	.nevents = ARRAY_SIZE(base_event_filter.events),
+	.events = {
+		EVENT(0x3c, 0),
+		INST_RETIRED,
+		EVENT(0x3c, 1),
+		EVENT(0x2e, 0x4f),
+		EVENT(0x2e, 0x41),
+		EVENT(0xc4, 0),
+		EVENT(0xc5, 0),
+		EVENT(0xa4, 1),
+		AMD_ZEN_BR_RETIRED,
+	},
 };
 
 struct {
@@ -225,51 +241,16 @@ static bool sanity_check_pmu(struct kvm_vcpu *vcpu)
 	return !r;
 }
 
-static struct kvm_pmu_event_filter *alloc_pmu_event_filter(uint32_t nevents)
-{
-	struct kvm_pmu_event_filter *f;
-	int size = sizeof(*f) + nevents * sizeof(f->events[0]);
-
-	f = malloc(size);
-	TEST_ASSERT(f, "Out of memory");
-	memset(f, 0, size);
-	f->nevents = nevents;
-	return f;
-}
-
-
-static struct kvm_pmu_event_filter *
-create_pmu_event_filter(const uint64_t event_list[], int nevents,
-			uint32_t action, uint32_t flags)
-{
-	struct kvm_pmu_event_filter *f;
-	int i;
-
-	f = alloc_pmu_event_filter(nevents);
-	f->action = action;
-	f->flags = flags;
-	for (i = 0; i < nevents; i++)
-		f->events[i] = event_list[i];
-
-	return f;
-}
-
-static struct kvm_pmu_event_filter *event_filter(uint32_t action)
-{
-	return create_pmu_event_filter(event_list,
-				       ARRAY_SIZE(event_list),
-				       action, 0);
-}
-
 /*
  * Remove the first occurrence of 'event' (if any) from the filter's
  * event list.
  */
-static struct kvm_pmu_event_filter *remove_event(struct kvm_pmu_event_filter *f,
+static struct kvm_pmu_event_filter *remove_event(struct __kvm_pmu_event_filter *__f,
 						 uint64_t event)
 {
 	bool found = false;
 	int i;
+	struct kvm_pmu_event_filter *f = (void *)__f;
 
 	for (i = 0; i < f->nevents; i++) {
 		if (found)
@@ -315,66 +296,70 @@ static void test_without_filter(struct kvm_vcpu *vcpu)
 }
 
 static void test_with_filter(struct kvm_vcpu *vcpu,
-			     struct kvm_pmu_event_filter *f)
+			     struct __kvm_pmu_event_filter *__f)
 {
+	struct kvm_pmu_event_filter *f = (void *)__f;
+
 	vm_ioctl(vcpu->vm, KVM_SET_PMU_EVENT_FILTER, f);
 	run_vcpu_and_sync_pmc_results(vcpu);
 }
 
 static void test_amd_deny_list(struct kvm_vcpu *vcpu)
 {
-	uint64_t event = EVENT(0x1C2, 0);
-	struct kvm_pmu_event_filter *f;
+	struct __kvm_pmu_event_filter f = base_event_filter;
 
-	f = create_pmu_event_filter(&event, 1, KVM_PMU_EVENT_DENY, 0);
-	test_with_filter(vcpu, f);
-	free(f);
+	f.action = KVM_PMU_EVENT_DENY;
+	f.nevents = 1;
+	f.events[0] = EVENT(0x1C2, 0);
+	test_with_filter(vcpu, &f);
 
 	ASSERT_PMC_COUNTING_INSTRUCTIONS();
 }
 
 static void test_member_deny_list(struct kvm_vcpu *vcpu)
 {
-	struct kvm_pmu_event_filter *f = event_filter(KVM_PMU_EVENT_DENY);
+	struct __kvm_pmu_event_filter f = base_event_filter;
 
-	test_with_filter(vcpu, f);
-	free(f);
+	f.action = KVM_PMU_EVENT_DENY;
+	test_with_filter(vcpu, &f);
 
 	ASSERT_PMC_NOT_COUNTING_INSTRUCTIONS();
 }
 
 static void test_member_allow_list(struct kvm_vcpu *vcpu)
 {
-	struct kvm_pmu_event_filter *f = event_filter(KVM_PMU_EVENT_ALLOW);
+	struct __kvm_pmu_event_filter f = base_event_filter;
 
-	test_with_filter(vcpu, f);
-	free(f);
+	f.action = KVM_PMU_EVENT_ALLOW;
+	test_with_filter(vcpu, &f);
 
 	ASSERT_PMC_COUNTING_INSTRUCTIONS();
 }
 
 static void test_not_member_deny_list(struct kvm_vcpu *vcpu)
 {
-	struct kvm_pmu_event_filter *f = event_filter(KVM_PMU_EVENT_DENY);
+	struct __kvm_pmu_event_filter f = base_event_filter;
+
+	f.action = KVM_PMU_EVENT_DENY;
 
-	remove_event(f, INST_RETIRED);
-	remove_event(f, INTEL_BR_RETIRED);
-	remove_event(f, AMD_ZEN_BR_RETIRED);
-	test_with_filter(vcpu, f);
-	free(f);
+	remove_event(&f, INST_RETIRED);
+	remove_event(&f, INTEL_BR_RETIRED);
+	remove_event(&f, AMD_ZEN_BR_RETIRED);
+	test_with_filter(vcpu, &f);
 
 	ASSERT_PMC_COUNTING_INSTRUCTIONS();
 }
 
 static void test_not_member_allow_list(struct kvm_vcpu *vcpu)
 {
-	struct kvm_pmu_event_filter *f = event_filter(KVM_PMU_EVENT_ALLOW);
+	struct __kvm_pmu_event_filter f = base_event_filter;
 
-	remove_event(f, INST_RETIRED);
-	remove_event(f, INTEL_BR_RETIRED);
-	remove_event(f, AMD_ZEN_BR_RETIRED);
-	test_with_filter(vcpu, f);
-	free(f);
+	f.action = KVM_PMU_EVENT_ALLOW;
+
+	remove_event(&f, INST_RETIRED);
+	remove_event(&f, INTEL_BR_RETIRED);
+	remove_event(&f, AMD_ZEN_BR_RETIRED);
+	test_with_filter(vcpu, &f);
 
 	ASSERT_PMC_NOT_COUNTING_INSTRUCTIONS();
 }
@@ -569,19 +554,16 @@ static void run_masked_events_test(struct kvm_vcpu *vcpu,
 				   const uint64_t masked_events[],
 				   const int nmasked_events)
 {
-	struct kvm_pmu_event_filter *f;
+	struct __kvm_pmu_event_filter f = {
+	    .nevents = nmasked_events,
+	    .action = KVM_PMU_EVENT_ALLOW,
+	    .flags = KVM_PMU_EVENT_FLAG_MASKED_EVENTS,
+	};
 
-	f = create_pmu_event_filter(masked_events, nmasked_events,
-				    KVM_PMU_EVENT_ALLOW,
-				    KVM_PMU_EVENT_FLAG_MASKED_EVENTS);
-	test_with_filter(vcpu, f);
-	free(f);
+	memcpy(f.events, masked_events, sizeof(uint64_t) * nmasked_events);
+	test_with_filter(vcpu, &f);
 }
 
-/* Matches KVM_PMU_EVENT_FILTER_MAX_EVENTS in pmu.c */
-#define MAX_FILTER_EVENTS	300
-#define MAX_TEST_EVENTS		10
-
 #define ALLOW_LOADS		BIT(0)
 #define ALLOW_STORES		BIT(1)
 #define ALLOW_LOADS_STORES	BIT(2)
@@ -753,17 +735,27 @@ static void test_masked_events(struct kvm_vcpu *vcpu)
 	run_masked_events_tests(vcpu, events, nevents);
 }
 
-static int run_filter_test(struct kvm_vcpu *vcpu, const uint64_t *events,
-			   int nevents, uint32_t flags)
+static int do_vcpu_set_pmu_event_filter(struct kvm_vcpu *vcpu,
+					struct __kvm_pmu_event_filter *__f)
 {
-	struct kvm_pmu_event_filter *f;
-	int r;
+	struct kvm_pmu_event_filter *f = (void *)__f;
+
+	return __vm_ioctl(vcpu->vm, KVM_SET_PMU_EVENT_FILTER, f);
+}
 
-	f = create_pmu_event_filter(events, nevents, KVM_PMU_EVENT_ALLOW, flags);
-	r = __vm_ioctl(vcpu->vm, KVM_SET_PMU_EVENT_FILTER, f);
-	free(f);
+static int set_pmu_single_event_filter(struct kvm_vcpu *vcpu, uint64_t event,
+				       uint32_t flags, uint32_t action)
+{
+	struct __kvm_pmu_event_filter f = {
+	    .nevents = 1,
+	    .flags = flags,
+	    .action = action,
+	    .events = {
+		event,
+	    },
+	};
 
-	return r;
+	return do_vcpu_set_pmu_event_filter(vcpu, &f);
 }
 
 static void test_filter_ioctl(struct kvm_vcpu *vcpu)
@@ -775,14 +767,18 @@ static void test_filter_ioctl(struct kvm_vcpu *vcpu)
 	 * Unfortunately having invalid bits set in event data is expected to
 	 * pass when flags == 0 (bits other than eventsel+umask).
 	 */
-	r = run_filter_test(vcpu, &e, 1, 0);
+	r = set_pmu_single_event_filter(vcpu, e, 0, KVM_PMU_EVENT_ALLOW);
 	TEST_ASSERT(r == 0, "Valid PMU Event Filter is failing");
 
-	r = run_filter_test(vcpu, &e, 1, KVM_PMU_EVENT_FLAG_MASKED_EVENTS);
+	r = set_pmu_single_event_filter(vcpu, e,
+					KVM_PMU_EVENT_FLAG_MASKED_EVENTS,
+					KVM_PMU_EVENT_ALLOW);
 	TEST_ASSERT(r != 0, "Invalid PMU Event Filter is expected to fail");
 
 	e = KVM_PMU_ENCODE_MASKED_ENTRY(0xff, 0xff, 0xff, 0xf);
-	r = run_filter_test(vcpu, &e, 1, KVM_PMU_EVENT_FLAG_MASKED_EVENTS);
+	r = set_pmu_single_event_filter(vcpu, e,
+					KVM_PMU_EVENT_FLAG_MASKED_EVENTS,
+					KVM_PMU_EVENT_ALLOW);
 	TEST_ASSERT(r == 0, "Valid PMU Event Filter is failing");
 }
 
-- 
2.31.1

[PATCH v3 2/4] KVM: selftests: Test unavailable event filters are rejected

[Jinrong Liang]

From: Jinrong Liang <cloudliang@tencent.com>

This commit adds test cases for unsupported input values in the
PMU event filter. The tests cover unsupported "action" values,
unsupported "flags" values, and unsupported "nevents" values.
All these cases should return an error, as they are currently
not supported by the filter. Additionally, the patch tests setting
non-exist fixed counters in the fixed bitmap doesn't fail.

Signed-off-by: Jinrong Liang <cloudliang@tencent.com>
---
 .../kvm/x86_64/pmu_event_filter_test.c        | 48 +++++++++++++++++--
 1 file changed, 45 insertions(+), 3 deletions(-)

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 26f674c32cde..7555e0f4290c 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,9 +11,7 @@
  */
 
 #define _GNU_SOURCE /* for program_invocation_short_name */
-#include "test_util.h"
-#include "kvm_util.h"
-#include "processor.h"
+#include "pmu.h"
 
 /*
  * In lieu of copying perf_event.h into tools...
@@ -32,6 +30,10 @@
 #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_FLAG_MASKED_EVENTS + 1)
+#define PMU_EVENT_FILTER_INVALID_NEVENTS		(MAX_FILTER_EVENTS + 1)
+
 /*
  * This is how the event selector and unit mask are stored in an AMD
  * core performance event-select register. Intel's format is similar,
@@ -762,6 +764,7 @@ static void test_filter_ioctl(struct kvm_vcpu *vcpu)
 {
 	uint64_t e = ~0ul;
 	int r;
+	struct __kvm_pmu_event_filter f;
 
 	/*
 	 * Unfortunately having invalid bits set in event data is expected to
@@ -780,6 +783,45 @@ static void test_filter_ioctl(struct kvm_vcpu *vcpu)
 					KVM_PMU_EVENT_FLAG_MASKED_EVENTS,
 					KVM_PMU_EVENT_ALLOW);
 	TEST_ASSERT(r == 0, "Valid PMU Event Filter is failing");
+
+	/*
+	 * Testing unsupported "action" input values should return an error.
+	 * Currently, only values 0 or 1 are supported.
+	 */
+	f = base_event_filter;
+	f.action = PMU_EVENT_FILTER_INVALID_ACTION;
+	r = do_vcpu_set_pmu_event_filter(vcpu, &f);
+	TEST_ASSERT(r != 0, "Set invalid action is expected to fail.");
+
+	/*
+	 * Testing unsupported "flags" input values should return an error.
+	 * Currently, only values 0 or 1 are supported.
+	 */
+	f = base_event_filter;
+	f.flags = PMU_EVENT_FILTER_INVALID_FLAGS;
+	r = do_vcpu_set_pmu_event_filter(vcpu, &f);
+	TEST_ASSERT(r != 0, "Set invalid flags is expected to fail.");
+
+	/*
+	 * Testing unsupported "nevents" input values should return an error.
+	 * Currently, only values less than or equal to
+	 * MAX_FILTER_EVENTS are supported.
+	 */
+	f = base_event_filter;
+	f.nevents = PMU_EVENT_FILTER_INVALID_NEVENTS;
+	r = do_vcpu_set_pmu_event_filter(vcpu, &f);
+	TEST_ASSERT(r != 0,
+		    "Setting PMU event filters that exceeds the maximum supported value should fail");
+
+	/*
+	 * In this case, setting non-exist fixed counters in the fixed bitmap
+	 * doesn't fail.
+	 */
+	f = base_event_filter;
+	f.fixed_counter_bitmap = ~GENMASK_ULL(X86_INTEL_MAX_FIXED_CTR_NUM, 0);
+	r = do_vcpu_set_pmu_event_filter(vcpu, &f);
+	TEST_ASSERT(r == 0,
+		    "Setting invalid or non-exist fixed cunters in the fixed bitmap fail.");
 }
 
 int main(int argc, char *argv[])
-- 
2.31.1

[PATCH v3 3/4] KVM: selftests: Check if event filter meets expectations on fixed counters

[Jinrong Liang]

From: Jinrong Liang <cloudliang@tencent.com>

Add tests to cover that pmu event_filter works as expected when
it's applied to fixed performance counters, even if there is none
fixed counter exists (e.g. Intel guest pmu version=1 or AMD guest).

Signed-off-by: Jinrong Liang <cloudliang@tencent.com>
---
 .../kvm/x86_64/pmu_event_filter_test.c        | 81 +++++++++++++++++++
 1 file changed, 81 insertions(+)

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 7555e0f4290c..72f7fdb821e5 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
@@ -824,6 +824,86 @@ static void test_filter_ioctl(struct kvm_vcpu *vcpu)
 		    "Setting invalid or non-exist fixed cunters in the fixed bitmap fail.");
 }
 
+static void intel_run_fixed_counter_guest_code(uint8_t fixed_ctr_idx)
+{
+	for (;;) {
+		wrmsr(MSR_CORE_PERF_GLOBAL_CTRL, 0);
+		wrmsr(MSR_CORE_PERF_FIXED_CTR0 + fixed_ctr_idx, 0);
+
+		/* 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));
+		__asm__ __volatile__("loop ." : "+c"((int){NUM_BRANCHES}));
+		wrmsr(MSR_CORE_PERF_GLOBAL_CTRL, 0);
+
+		GUEST_SYNC(rdmsr(MSR_CORE_PERF_FIXED_CTR0 + fixed_ctr_idx));
+	}
+}
+
+static uint64_t test_with_fixed_counter_filter(struct kvm_vcpu *vcpu,
+					       uint32_t action, uint32_t bitmap)
+{
+	struct __kvm_pmu_event_filter f = {
+		.action = action,
+		.fixed_counter_bitmap = bitmap,
+	};
+	do_vcpu_set_pmu_event_filter(vcpu, &f);
+
+	return run_vcpu_to_sync(vcpu);
+}
+
+static void __test_fixed_counter_bitmap(struct kvm_vcpu *vcpu, uint8_t idx,
+					uint8_t nr_fixed_counters)
+{
+	unsigned int i;
+	uint32_t bitmap;
+	uint64_t count;
+
+	TEST_ASSERT(nr_fixed_counters < sizeof(bitmap),
+		    "Invalid nr_fixed_counters");
+
+	/*
+	 * Check the fixed performance counter can count normally works when
+	 * KVM userspace doesn't set any pmu filter.
+	 */
+	TEST_ASSERT(run_vcpu_to_sync(vcpu),
+		    "Fixed counter does not exist or does not work as expected.");
+
+	for (i = 0; i < BIT(nr_fixed_counters); i++) {
+		bitmap = BIT(i);
+		count = test_with_fixed_counter_filter(vcpu, KVM_PMU_EVENT_ALLOW,
+						       bitmap);
+		TEST_ASSERT(!!count == !!(bitmap & BIT(idx)),
+			    "Fixed event filter does not work as expected.");
+
+		count = test_with_fixed_counter_filter(vcpu, KVM_PMU_EVENT_DENY,
+						       bitmap);
+		TEST_ASSERT(!!count == !(bitmap & BIT(idx)),
+			    "Fixed event filter does not work as expected.");
+	}
+}
+
+static void test_fixed_counter_bitmap(void)
+{
+	struct kvm_vm *vm;
+	struct kvm_vcpu *vcpu;
+	uint8_t idx;
+
+	/*
+	 * Check that pmu_event_filter works as expected when it's applied to
+	 * fixed performance counters.
+	 */
+	for (idx = 0; idx < X86_INTEL_MAX_FIXED_CTR_NUM; idx++) {
+		vm = vm_create_with_one_vcpu(&vcpu,
+					     intel_run_fixed_counter_guest_code);
+		vcpu_args_set(vcpu, 1, idx);
+		__test_fixed_counter_bitmap(vcpu, idx,
+					    X86_INTEL_MAX_FIXED_CTR_NUM);
+		kvm_vm_free(vm);
+	}
+}
+
 int main(int argc, char *argv[])
 {
 	void (*guest_code)(void);
@@ -867,6 +947,7 @@ int main(int argc, char *argv[])
 	kvm_vm_free(vm);
 
 	test_pmu_config_disable(guest_code);
+	test_fixed_counter_bitmap();
 
 	return 0;
 }
-- 
2.31.1

[PATCH v3 4/4] KVM: selftests: Test gp event filters don't affect fixed event filters

[Jinrong Liang]

From: Jinrong Liang <cloudliang@tencent.com>

Add a test to ensure that setting both generic and fixed performance
event filters does not affect the consistency of the fixed performance
filter behavior in KVM. This test helps to ensure that the fixed
performance filter works as expected even when generic performance
event filters are also set.

Signed-off-by: Jinrong Liang <cloudliang@tencent.com>
---
 .../kvm/x86_64/pmu_event_filter_test.c        | 32 +++++++++++++++++++
 1 file changed, 32 insertions(+)

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 72f7fdb821e5..79bfdb64820b 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
@@ -853,6 +853,19 @@ static uint64_t test_with_fixed_counter_filter(struct kvm_vcpu *vcpu,
 	return run_vcpu_to_sync(vcpu);
 }
 
+static uint64_t test_set_gp_and_fixed_event_filter(struct kvm_vcpu *vcpu,
+						   uint32_t action,
+						   uint32_t bitmap)
+{
+	struct __kvm_pmu_event_filter f = base_event_filter;
+
+	f.action = action;
+	f.fixed_counter_bitmap = bitmap;
+	do_vcpu_set_pmu_event_filter(vcpu, &f);
+
+	return run_vcpu_to_sync(vcpu);
+}
+
 static void __test_fixed_counter_bitmap(struct kvm_vcpu *vcpu, uint8_t idx,
 					uint8_t nr_fixed_counters)
 {
@@ -881,6 +894,25 @@ static void __test_fixed_counter_bitmap(struct kvm_vcpu *vcpu, uint8_t idx,
 						       bitmap);
 		TEST_ASSERT(!!count == !(bitmap & BIT(idx)),
 			    "Fixed event filter does not work as expected.");
+
+		/*
+		 * Check that setting both events[] and fixed_counter_bitmap
+		 * does not affect the consistency of the fixed ctrs' behaviour.
+		 *
+		 * Note, the fixed_counter_bitmap has higher priority than the
+		 * events list.
+		 */
+		count = test_set_gp_and_fixed_event_filter(vcpu,
+							   KVM_PMU_EVENT_ALLOW,
+							   bitmap);
+		TEST_ASSERT(!!count == !!(bitmap & BIT(idx)),
+			    "Fixed event filter does not work as expected.");
+
+		count = test_set_gp_and_fixed_event_filter(vcpu,
+							   KVM_PMU_EVENT_DENY,
+							   bitmap);
+		TEST_ASSERT(!!count == !(bitmap & BIT(idx)),
+			    "Fixed event filter does not work as expected.");
 	}
 }
 
-- 
2.31.1

Re: [PATCH v3 1/4] KVM: selftests: Introduce __kvm_pmu_event_filter to improved event filter settings

[Sean Christopherson]

On Wed, Jun 07, 2023, Jinrong Liang wrote:
> -static struct kvm_pmu_event_filter *remove_event(struct kvm_pmu_event_filter *f,
> +static struct kvm_pmu_event_filter *remove_event(struct __kvm_pmu_event_filter *__f,
>  						 uint64_t event)

Can you tack on a patch to drop the return?  None of the callers consume it, and
it incorrectly implies that the incoming filter isn't modified.

>  {
>  	bool found = false;
>  	int i;
> +	struct kvm_pmu_event_filter *f = (void *)__f;

Nit, reverse xmas tree is preferred:

	struct kvm_pmu_event_filter *f = (void *)__f;
	bool found = false;
	int i;

Hoever, I don't think this one needs to cast, the cast is only necessary when
invoking a KVM ioctl(), e.g. I believe this should work:

static void remove_event(struct __kvm_pmu_event_filter *f, uint64_t event)
{
	bool found = false;
	int i;

	for (i = 0; i < f->nevents; i++) {
		if (found)
			f->events[i - 1] = f->events[i];
		else
			found = f->events[i] == event;
	}
	if (found)
		f->nevents--;
}
> @@ -569,19 +554,16 @@ static void run_masked_events_test(struct kvm_vcpu *vcpu,
>  				   const uint64_t masked_events[],
>  				   const int nmasked_events)
>  {
> -	struct kvm_pmu_event_filter *f;
> +	struct __kvm_pmu_event_filter f = {
> +	    .nevents = nmasked_events,
> +	    .action = KVM_PMU_EVENT_ALLOW,
> +	    .flags = KVM_PMU_EVENT_FLAG_MASKED_EVENTS,

Tabs, not spaces please.

> +static int set_pmu_single_event_filter(struct kvm_vcpu *vcpu, uint64_t event,
> +				       uint32_t flags, uint32_t action)
> +{
> +	struct __kvm_pmu_event_filter f = {
> +	    .nevents = 1,
> +	    .flags = flags,
> +	    .action = action,
> +	    .events = {
> +		event,

Tabs.

Re: [PATCH v3 2/4] KVM: selftests: Test unavailable event filters are rejected

[Sean Christopherson]

On Wed, Jun 07, 2023, Jinrong Liang wrote:
> From: Jinrong Liang <cloudliang@tencent.com>
> 
> This commit adds test cases for unsupported input values in the

Avoid "this commit" and "this patch", simply state what the patch does as a
command, e.g. "Add test cases for ...".

> PMU event filter. The tests cover unsupported "action" values,
> unsupported "flags" values, and unsupported "nevents" values.
> All these cases should return an error, as they are currently
> not supported by the filter. Additionally, the patch tests setting
> non-exist fixed counters in the fixed bitmap doesn't fail.
> 
> Signed-off-by: Jinrong Liang <cloudliang@tencent.com>
> ---
>  .../kvm/x86_64/pmu_event_filter_test.c        | 48 +++++++++++++++++--
>  1 file changed, 45 insertions(+), 3 deletions(-)
> 
> 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 26f674c32cde..7555e0f4290c 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,9 +11,7 @@
>   */
>  
>  #define _GNU_SOURCE /* for program_invocation_short_name */
> -#include "test_util.h"
> -#include "kvm_util.h"
> -#include "processor.h"
> +#include "pmu.h"
>  
>  /*
>   * In lieu of copying perf_event.h into tools...
> @@ -32,6 +30,10 @@
>  #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_FLAG_MASKED_EVENTS + 1)
> +#define PMU_EVENT_FILTER_INVALID_NEVENTS		(MAX_FILTER_EVENTS + 1)
> +
>  /*
>   * This is how the event selector and unit mask are stored in an AMD
>   * core performance event-select register. Intel's format is similar,
> @@ -762,6 +764,7 @@ static void test_filter_ioctl(struct kvm_vcpu *vcpu)
>  {
>  	uint64_t e = ~0ul;
>  	int r;
> +	struct __kvm_pmu_event_filter f;

Reverse xmas tree.

>  
>  	/*
>  	 * Unfortunately having invalid bits set in event data is expected to
> @@ -780,6 +783,45 @@ static void test_filter_ioctl(struct kvm_vcpu *vcpu)
>  					KVM_PMU_EVENT_FLAG_MASKED_EVENTS,
>  					KVM_PMU_EVENT_ALLOW);
>  	TEST_ASSERT(r == 0, "Valid PMU Event Filter is failing");
> +
> +	/*
> +	 * Testing unsupported "action" input values should return an error.

Omit the "Testing", KVM's behavior isn't specific to "testing", any unsupported
action should fail.

	/* Unsupported actions should be rejected by KVM. */

Though honestly, I would forego the comments entirely, the macro name plus the
assert make it quite clear what's being tested.

> +	 * Currently, only values 0 or 1 are supported.

Drop this part of the comment, it will become stale if PMU_EVENT_FILTER_INVALID_ACTION
is modified, and readers can look at the definition of PMU_EVENT_FILTER_INVALID_ACTION
if they really care about the actual value.

> +	 */
> +	f = base_event_filter;
> +	f.action = PMU_EVENT_FILTER_INVALID_ACTION;
> +	r = do_vcpu_set_pmu_event_filter(vcpu, &f);
> +	TEST_ASSERT(r != 0, "Set invalid action is expected to fail.");

Ignore the bad precedent set by this test, the preferred way to check for 0 and
!0 is TEST_ASSERT(r, ...) and TEST_ASSERT(!r, ...);

And no punctuation in the assert, i.e. drop the period.

> +
> +	/*
> +	 * Testing unsupported "flags" input values should return an error.
> +	 * Currently, only values 0 or 1 are supported.
> +	 */

Same here.

> +	f = base_event_filter;
> +	f.flags = PMU_EVENT_FILTER_INVALID_FLAGS;
> +	r = do_vcpu_set_pmu_event_filter(vcpu, &f);
> +	TEST_ASSERT(r != 0, "Set invalid flags is expected to fail.");
> +
> +	/*
> +	 * Testing unsupported "nevents" input values should return an error.
> +	 * Currently, only values less than or equal to
> +	 * MAX_FILTER_EVENTS are supported.

And here.

> +	 */
> +	f = base_event_filter;
> +	f.nevents = PMU_EVENT_FILTER_INVALID_NEVENTS;
> +	r = do_vcpu_set_pmu_event_filter(vcpu, &f);
> +	TEST_ASSERT(r != 0,
> +		    "Setting PMU event filters that exceeds the maximum supported value should fail");

To avoid splitting lines,

	TEST_ASSERT(r, "Exceeding the max number of filter events should fail");
> +
> +	/*
> +	 * In this case, setting non-exist fixed counters in the fixed bitmap
> +	 * doesn't fail.
> +	 */

And here.

> +	f = base_event_filter;
> +	f.fixed_counter_bitmap = ~GENMASK_ULL(X86_INTEL_MAX_FIXED_CTR_NUM, 0);
> +	r = do_vcpu_set_pmu_event_filter(vcpu, &f);
> +	TEST_ASSERT(r == 0,
> +		    "Setting invalid or non-exist fixed cunters in the fixed bitmap fail.");

Something like so to avoid multiple lines.

	TEST_ASSERT(!r, "Masking non-existent fixed counters should be allowed");

Re: [PATCH v3 3/4] KVM: selftests: Check if event filter meets expectations on fixed counters

[Sean Christopherson]

On Wed, Jun 07, 2023, Jinrong Liang wrote:
> +	/*
> +	 * Check the fixed performance counter can count normally works when
> +	 * KVM userspace doesn't set any pmu filter.
> +	 */
> +	TEST_ASSERT(run_vcpu_to_sync(vcpu),
> +		    "Fixed counter does not exist or does not work as expected.");

No punctuation, and print the unexpected return value so that debug isn't too
painful.

> +
> +	for (i = 0; i < BIT(nr_fixed_counters); i++) {
> +		bitmap = BIT(i);
> +		count = test_with_fixed_counter_filter(vcpu, KVM_PMU_EVENT_ALLOW,
> +						       bitmap);
> +		TEST_ASSERT(!!count == !!(bitmap & BIT(idx)),
> +			    "Fixed event filter does not work as expected.");

No punctuation please.

> +
> +		count = test_with_fixed_counter_filter(vcpu, KVM_PMU_EVENT_DENY,
> +						       bitmap);
> +		TEST_ASSERT(!!count == !(bitmap & BIT(idx)),
> +			    "Fixed event filter does not work as expected.");
> +	}
> +}

Re: [PATCH v3 4/4] KVM: selftests: Test gp event filters don't affect fixed event filters

[Sean Christopherson]

On Wed, Jun 07, 2023, Jinrong Liang wrote:
>  static void __test_fixed_counter_bitmap(struct kvm_vcpu *vcpu, uint8_t idx,
>  					uint8_t nr_fixed_counters)
>  {
> @@ -881,6 +894,25 @@ static void __test_fixed_counter_bitmap(struct kvm_vcpu *vcpu, uint8_t idx,
>  						       bitmap);
>  		TEST_ASSERT(!!count == !(bitmap & BIT(idx)),
>  			    "Fixed event filter does not work as expected.");
> +
> +		/*
> +		 * Check that setting both events[] and fixed_counter_bitmap
> +		 * does not affect the consistency of the fixed ctrs' behaviour.

Spell out "counters".  And this isn't about "consistency", the fixed_counter_bitmap
should straight up win, i.e.

		/*
		 * Check that fixed_counter_bitmap has higher priority than
		 * events[] when both are set.
		 */

Re: [PATCH v3 1/4] KVM: selftests: Introduce __kvm_pmu_event_filter to improved event filter settings

[Jinrong Liang]

Sean Christopherson <seanjc@google.com> 于2023年6月29日周四 05:19写道：
>
> On Wed, Jun 07, 2023, Jinrong Liang wrote:
> > -static struct kvm_pmu_event_filter *remove_event(struct kvm_pmu_event_filter *f,
> > +static struct kvm_pmu_event_filter *remove_event(struct __kvm_pmu_event_filter *__f,
> >                                                uint64_t event)
>
> Can you tack on a patch to drop the return?  None of the callers consume it, and
> it incorrectly implies that the incoming filter isn't modified.

Thank you very much for your suggestion! I'm more than happy to follow
your advice and modify the code accordingly.

>
> >  {
> >       bool found = false;
> >       int i;
> > +     struct kvm_pmu_event_filter *f = (void *)__f;
>
> Nit, reverse xmas tree is preferred:
>
>         struct kvm_pmu_event_filter *f = (void *)__f;
>         bool found = false;
>         int i;
>
> Hoever, I don't think this one needs to cast, the cast is only necessary when
> invoking a KVM ioctl(), e.g. I believe this should work:
>
> static void remove_event(struct __kvm_pmu_event_filter *f, uint64_t event)
> {
>         bool found = false;
>         int i;
>
>         for (i = 0; i < f->nevents; i++) {
>                 if (found)
>                         f->events[i - 1] = f->events[i];
>                 else
>                         found = f->events[i] == event;
>         }
>         if (found)
>                 f->nevents--;
> }
> > @@ -569,19 +554,16 @@ static void run_masked_events_test(struct kvm_vcpu *vcpu,
> >                                  const uint64_t masked_events[],
> >                                  const int nmasked_events)
> >  {
> > -     struct kvm_pmu_event_filter *f;
> > +     struct __kvm_pmu_event_filter f = {
> > +         .nevents = nmasked_events,
> > +         .action = KVM_PMU_EVENT_ALLOW,
> > +         .flags = KVM_PMU_EVENT_FLAG_MASKED_EVENTS,
>
> Tabs, not spaces please.
>
> > +static int set_pmu_single_event_filter(struct kvm_vcpu *vcpu, uint64_t event,
> > +                                    uint32_t flags, uint32_t action)
> > +{
> > +     struct __kvm_pmu_event_filter f = {
> > +         .nevents = 1,
> > +         .flags = flags,
> > +         .action = action,
> > +         .events = {
> > +             event,
>
> Tabs.

I will include these change in the new patch version and ensure that
any related code is adjusted accordingly.

Once again, I truly appreciate your guidance!