diff mbox series

[v9,2/3] KVM: selftests: aarch64: Introduce pmu_event_filter_test

Message ID 20240613082345.132336-3-shahuang@redhat.com
State New
Headers show
Series KVM: selftests: aarch64: Introduce pmu_event_filter_test | expand

Commit Message

Shaoqin Huang June 13, 2024, 8:23 a.m. UTC
Introduce pmu_event_filter_test for arm64 platforms. The test configures
PMUv3 for a vCPU, and sets different pmu event filters for the vCPU, and
check if the guest can see those events which user allow and can't use
those events which use deny.

This test refactor the create_vpmu_vm() and make it a wrapper for
__create_vpmu_vm(), which allows some extra init code before
KVM_ARM_VCPU_PMU_V3_INIT.

And this test use the KVM_ARM_VCPU_PMU_V3_FILTER attribute to set the
pmu event filter in KVM. And choose to filter two common event
branches_retired and instructions_retired, and let the guest to check if
it see the right pmceid register.

Signed-off-by: Shaoqin Huang <shahuang@redhat.com>
---
 tools/testing/selftests/kvm/Makefile          |   1 +
 .../kvm/aarch64/pmu_event_filter_test.c       | 303 ++++++++++++++++++
 2 files changed, 304 insertions(+)
 create mode 100644 tools/testing/selftests/kvm/aarch64/pmu_event_filter_test.c

Comments

Raghavendra Rao Ananta June 18, 2024, 12:01 a.m. UTC | #1
Hi Shaoqin


On Thu, Jun 13, 2024 at 1:28 AM Shaoqin Huang <shahuang@redhat.com> wrote:

> +static void prepare_expected_pmce(struct kvm_pmu_event_filter *filter)
> +{
> +       struct pmu_common_event_ids pmce_mask = { ~0, ~0 };
> +       bool first_filter = true;
> +       int i;
> +
> +       while (filter && filter->nevents != 0) {
Do you also want to add a check to ensure we aren't running over
FILTER_NR (I'd expect a compiler warning/error though)?

> +               if (first_filter) {
> +                       if (filter->action == KVM_PMU_EVENT_ALLOW)
> +                               memset(&pmce_mask, 0, sizeof(pmce_mask));
> +                       first_filter = false;
> +               }
nit: Probably we can make the 'first_filter' part a little cleaner by
checking this outside the loop.

if (filter && filter->action == KVM_PMU_EVENT_ALLOW)
        memset(&pmce_mask, 0, sizeof(pmce_mask));

while (filter && filter->nevents != 0) {
    ...
}

> +static struct test_desc tests[] = {
> +       {
> +               .name = "without_filter",
> +               .filter = {
> +                       { 0 }
> +               },
> +       },
> +       {
> +               .name = "member_allow_filter",
> +               .filter = {
> +                       DEFINE_FILTER(ARMV8_PMUV3_PERFCTR_SW_INCR, 0),
In terms of readability, do you think it's better to use
KVM_PMU_EVENT_{ALLOW|DENY}, instead of 0 and 1?

Or, if that's coming out to be too long, may be create another wrapper
over DEFINE_FILTER, and simply use that in the array:

#define EVENT_ALLOW(event) DEFINE_FILTER(event, KVM_PMU_EVENT_ALLOW)
#define EVENT_DENY(event) DEFINE_FILTER(event, KVM_PMU_EVENT_DENY)

.filter = {
    EVENT_ALLOW(ARMV8_PMUV3_PERFCTR_SW_INCR),

> +                       DEFINE_FILTER(ARMV8_PMUV3_PERFCTR_INST_RETIRED, 0),
> +                       DEFINE_FILTER(ARMV8_PMUV3_PERFCTR_BR_RETIRED, 0),
> +                       { 0 },
> +               },
> +       },

> +       {
> +               .name = "cancel_filter",
> +               .filter = {
> +                       DEFINE_FILTER(ARMV8_PMUV3_PERFCTR_CPU_CYCLES, 0),
> +                       DEFINE_FILTER(ARMV8_PMUV3_PERFCTR_CPU_CYCLES, 1),
> +               },
Since the initial filter map depends on the event being allowed or
denied, do you think another "cancel_filter" case to first deny and
then allow would also be better?

> +       },
> +       {
> +               .name = "multiple_filter",
> +               .filter = {
> +                       __DEFINE_FILTER(0x0, 0x10, 0),
> +                       __DEFINE_FILTER(0x6, 0x3, 1),
> +               },
> +       },
> +       { 0 }
> +};
> +
> +static void run_tests(void)
> +{
> +       struct test_desc *t;
> +
> +       for (t = &tests[0]; t->name; t++)
> +               run_test(t);
> +}
> +
> +int used_pmu_events[] = {
nit: static int used_pmu_events[] = {

Thank you.
Raghavendra


> +       ARMV8_PMUV3_PERFCTR_BR_RETIRED,
> +       ARMV8_PMUV3_PERFCTR_INST_RETIRED,
> +       ARMV8_PMUV3_PERFCTR_CHAIN,
> +};
> +
> +static bool kvm_pmu_support_events(void)
> +{
> +       struct pmu_common_event_ids used_pmce = { 0, 0 };
> +
> +       create_vpmu_vm(guest_get_pmceid);
> +
> +       memset(&max_pmce, 0, sizeof(max_pmce));
> +       sync_global_to_guest(vpmu_vm.vm, max_pmce);
> +       run_vcpu(vpmu_vm.vcpu);
> +       sync_global_from_guest(vpmu_vm.vm, max_pmce);
> +       destroy_vpmu_vm();
> +
> +       for (int i = 0; i < ARRAY_SIZE(used_pmu_events); i++)
> +               set_pmce(&used_pmce, KVM_PMU_EVENT_ALLOW, used_pmu_events[i]);
> +
> +       return ((max_pmce.pmceid0 & used_pmce.pmceid0) == used_pmce.pmceid0) &&
> +              ((max_pmce.pmceid1 & used_pmce.pmceid1) == used_pmce.pmceid1);
> +}
> +
> +int main(void)
> +{
> +       TEST_REQUIRE(kvm_has_cap(KVM_CAP_ARM_PMU_V3));
> +       TEST_REQUIRE(kvm_pmu_support_events());
> +
> +       run_tests();
> +}
> --
> 2.40.1
>
>
Shaoqin Huang June 18, 2024, 7:20 a.m. UTC | #2
Hi Raghavendra,

Thanks for helping review this series.

On 6/18/24 08:01, Raghavendra Rao Ananta wrote:
> Hi Shaoqin
> 
> 
> On Thu, Jun 13, 2024 at 1:28 AM Shaoqin Huang <shahuang@redhat.com> wrote:
> 
>> +static void prepare_expected_pmce(struct kvm_pmu_event_filter *filter)
>> +{
>> +       struct pmu_common_event_ids pmce_mask = { ~0, ~0 };
>> +       bool first_filter = true;
>> +       int i;
>> +
>> +       while (filter && filter->nevents != 0) {
> Do you also want to add a check to ensure we aren't running over
> FILTER_NR (I'd expect a compiler warning/error though)?

The FILTER_NR is only used to assign the length of the filter array, if 
the defined filter array length is larger than the FILTER_NR, I believe 
there will be a compiling warning.

> 
>> +               if (first_filter) {
>> +                       if (filter->action == KVM_PMU_EVENT_ALLOW)
>> +                               memset(&pmce_mask, 0, sizeof(pmce_mask));
>> +                       first_filter = false;
>> +               }
> nit: Probably we can make the 'first_filter' part a little cleaner by
> checking this outside the loop.
> 
> if (filter && filter->action == KVM_PMU_EVENT_ALLOW)
>          memset(&pmce_mask, 0, sizeof(pmce_mask));
> 
> while (filter && filter->nevents != 0) {
>      ...
> }

Thanks, this looks much better and I will change the code to it.

> 
>> +static struct test_desc tests[] = {
>> +       {
>> +               .name = "without_filter",
>> +               .filter = {
>> +                       { 0 }
>> +               },
>> +       },
>> +       {
>> +               .name = "member_allow_filter",
>> +               .filter = {
>> +                       DEFINE_FILTER(ARMV8_PMUV3_PERFCTR_SW_INCR, 0),
> In terms of readability, do you think it's better to use
> KVM_PMU_EVENT_{ALLOW|DENY}, instead of 0 and 1?
> 
> Or, if that's coming out to be too long, may be create another wrapper
> over DEFINE_FILTER, and simply use that in the array:
> 
> #define EVENT_ALLOW(event) DEFINE_FILTER(event, KVM_PMU_EVENT_ALLOW)
> #define EVENT_DENY(event) DEFINE_FILTER(event, KVM_PMU_EVENT_DENY)
> 
> .filter = {
>      EVENT_ALLOW(ARMV8_PMUV3_PERFCTR_SW_INCR),
> 

Pretty good idea. I will take your code which looks much clean.

>> +                       DEFINE_FILTER(ARMV8_PMUV3_PERFCTR_INST_RETIRED, 0),
>> +                       DEFINE_FILTER(ARMV8_PMUV3_PERFCTR_BR_RETIRED, 0),
>> +                       { 0 },
>> +               },
>> +       },
> 
>> +       {
>> +               .name = "cancel_filter",
>> +               .filter = {
>> +                       DEFINE_FILTER(ARMV8_PMUV3_PERFCTR_CPU_CYCLES, 0),
>> +                       DEFINE_FILTER(ARMV8_PMUV3_PERFCTR_CPU_CYCLES, 1),
>> +               },
> Since the initial filter map depends on the event being allowed or
> denied, do you think another "cancel_filter" case to first deny and
> then allow would also be better?

Yes. That would be better, I will add another test which first deny and 
then allow it.

> 
>> +       },
>> +       {
>> +               .name = "multiple_filter",
>> +               .filter = {
>> +                       __DEFINE_FILTER(0x0, 0x10, 0),
>> +                       __DEFINE_FILTER(0x6, 0x3, 1),
>> +               },
>> +       },
>> +       { 0 }
>> +};
>> +
>> +static void run_tests(void)
>> +{
>> +       struct test_desc *t;
>> +
>> +       for (t = &tests[0]; t->name; t++)
>> +               run_test(t);
>> +}
>> +
>> +int used_pmu_events[] = {
> nit: static int used_pmu_events[] = {
> 

Got it.

Thanks,
Shaoqin

> Thank you.
> Raghavendra
> 
> 
>> +       ARMV8_PMUV3_PERFCTR_BR_RETIRED,
>> +       ARMV8_PMUV3_PERFCTR_INST_RETIRED,
>> +       ARMV8_PMUV3_PERFCTR_CHAIN,
>> +};
>> +
>> +static bool kvm_pmu_support_events(void)
>> +{
>> +       struct pmu_common_event_ids used_pmce = { 0, 0 };
>> +
>> +       create_vpmu_vm(guest_get_pmceid);
>> +
>> +       memset(&max_pmce, 0, sizeof(max_pmce));
>> +       sync_global_to_guest(vpmu_vm.vm, max_pmce);
>> +       run_vcpu(vpmu_vm.vcpu);
>> +       sync_global_from_guest(vpmu_vm.vm, max_pmce);
>> +       destroy_vpmu_vm();
>> +
>> +       for (int i = 0; i < ARRAY_SIZE(used_pmu_events); i++)
>> +               set_pmce(&used_pmce, KVM_PMU_EVENT_ALLOW, used_pmu_events[i]);
>> +
>> +       return ((max_pmce.pmceid0 & used_pmce.pmceid0) == used_pmce.pmceid0) &&
>> +              ((max_pmce.pmceid1 & used_pmce.pmceid1) == used_pmce.pmceid1);
>> +}
>> +
>> +int main(void)
>> +{
>> +       TEST_REQUIRE(kvm_has_cap(KVM_CAP_ARM_PMU_V3));
>> +       TEST_REQUIRE(kvm_pmu_support_events());
>> +
>> +       run_tests();
>> +}
>> --
>> 2.40.1
>>
>>
>
diff mbox series

Patch

diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
index e78cac712229..964443e1a603 100644
--- a/tools/testing/selftests/kvm/Makefile
+++ b/tools/testing/selftests/kvm/Makefile
@@ -152,6 +152,7 @@  TEST_GEN_PROGS_aarch64 += aarch64/aarch32_id_regs
 TEST_GEN_PROGS_aarch64 += aarch64/debug-exceptions
 TEST_GEN_PROGS_aarch64 += aarch64/hypercalls
 TEST_GEN_PROGS_aarch64 += aarch64/page_fault_test
+TEST_GEN_PROGS_aarch64 += aarch64/pmu_event_filter_test
 TEST_GEN_PROGS_aarch64 += aarch64/psci_test
 TEST_GEN_PROGS_aarch64 += aarch64/set_id_regs
 TEST_GEN_PROGS_aarch64 += aarch64/smccc_filter
diff --git a/tools/testing/selftests/kvm/aarch64/pmu_event_filter_test.c b/tools/testing/selftests/kvm/aarch64/pmu_event_filter_test.c
new file mode 100644
index 000000000000..fb0fde1ed436
--- /dev/null
+++ b/tools/testing/selftests/kvm/aarch64/pmu_event_filter_test.c
@@ -0,0 +1,303 @@ 
+
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * pmu_event_filter_test - Test user limit pmu event for guest.
+ *
+ * Copyright (c) 2023 Red Hat, Inc.
+ *
+ * This test checks if the guest only see the limited pmu event that userspace
+ * sets, if the guest can use those events which user allow, and if the guest
+ * can't use those events which user deny.
+ * This test runs only when KVM_CAP_ARM_PMU_V3, KVM_ARM_VCPU_PMU_V3_FILTER
+ * is supported on the host.
+ */
+#include <kvm_util.h>
+#include <processor.h>
+#include <vgic.h>
+#include <vpmu.h>
+#include <test_util.h>
+#include <perf/arm_pmuv3.h>
+
+struct pmu_common_event_ids {
+	uint64_t pmceid0;
+	uint64_t pmceid1;
+} max_pmce, expected_pmce;
+
+struct vpmu_vm {
+	struct kvm_vm *vm;
+	struct kvm_vcpu *vcpu;
+	int gic_fd;
+};
+
+static struct vpmu_vm vpmu_vm;
+
+#define FILTER_NR 10
+
+struct test_desc {
+	const char *name;
+	struct kvm_pmu_event_filter filter[FILTER_NR];
+};
+
+#define __DEFINE_FILTER(base, num, act)		\
+	((struct kvm_pmu_event_filter) {	\
+		.base_event	= base,		\
+		.nevents	= num,		\
+		.action		= act,		\
+	})
+
+#define DEFINE_FILTER(base, act) __DEFINE_FILTER(base, 1, act)
+
+static void guest_code(void)
+{
+	uint64_t pmceid0 = read_sysreg(pmceid0_el0);
+	uint64_t pmceid1 = read_sysreg(pmceid1_el0);
+
+	GUEST_ASSERT_EQ(expected_pmce.pmceid0, pmceid0);
+	GUEST_ASSERT_EQ(expected_pmce.pmceid1, pmceid1);
+
+	GUEST_DONE();
+}
+
+static void guest_get_pmceid(void)
+{
+	max_pmce.pmceid0 = read_sysreg(pmceid0_el0);
+	max_pmce.pmceid1 = read_sysreg(pmceid1_el0);
+
+	GUEST_DONE();
+}
+
+static void run_vcpu(struct kvm_vcpu *vcpu)
+{
+	struct ucall uc;
+
+	while (1) {
+		vcpu_run(vcpu);
+		switch (get_ucall(vcpu, &uc)) {
+		case UCALL_DONE:
+			return;
+		case UCALL_ABORT:
+			REPORT_GUEST_ASSERT(uc);
+			break;
+		default:
+			TEST_FAIL("Unknown ucall %lu", uc.cmd);
+		}
+	}
+}
+
+static void set_pmce(struct pmu_common_event_ids *pmce, int action, int event)
+{
+	int base = 0;
+	uint64_t *pmceid = NULL;
+
+	if (event >= 0x4000) {
+		event -= 0x4000;
+		base = 32;
+	}
+
+	if (event >= 0 && event <= 0x1F) {
+		pmceid = &pmce->pmceid0;
+	} else if (event >= 0x20 && event <= 0x3F) {
+		event -= 0x20;
+		pmceid = &pmce->pmceid1;
+	} else {
+		return;
+	}
+
+	event += base;
+	if (action == KVM_PMU_EVENT_ALLOW)
+		*pmceid |= BIT(event);
+	else
+		*pmceid &= ~BIT(event);
+}
+
+static void prepare_expected_pmce(struct kvm_pmu_event_filter *filter)
+{
+	struct pmu_common_event_ids pmce_mask = { ~0, ~0 };
+	bool first_filter = true;
+	int i;
+
+	while (filter && filter->nevents != 0) {
+		if (first_filter) {
+			if (filter->action == KVM_PMU_EVENT_ALLOW)
+				memset(&pmce_mask, 0, sizeof(pmce_mask));
+			first_filter = false;
+		}
+
+		for (i = 0; i < filter->nevents; i++)
+			set_pmce(&pmce_mask, filter->action,
+				 filter->base_event + i);
+
+		filter++;
+	}
+
+	expected_pmce.pmceid0 = max_pmce.pmceid0 & pmce_mask.pmceid0;
+	expected_pmce.pmceid1 = max_pmce.pmceid1 & pmce_mask.pmceid1;
+}
+
+static void pmu_event_filter_init(struct kvm_pmu_event_filter *filter)
+{
+	while (filter && filter->nevents != 0) {
+		kvm_device_attr_set(vpmu_vm.vcpu->fd,
+				    KVM_ARM_VCPU_PMU_V3_CTRL,
+				    KVM_ARM_VCPU_PMU_V3_FILTER,
+				    filter);
+		filter++;
+	}
+}
+
+/* Create a VM that has one vCPU with PMUv3 configured. */
+static void create_vpmu_vm_with_filter(void *guest_code,
+				       struct kvm_pmu_event_filter *filter)
+{
+	uint64_t irq = 23;
+
+	/* The test creates the vpmu_vm multiple times. Ensure a clean state */
+	memset(&vpmu_vm, 0, sizeof(vpmu_vm));
+
+	vpmu_vm.vm = vm_create(1);
+	vpmu_vm.vcpu = vm_vcpu_add_with_vpmu(vpmu_vm.vm, 0, guest_code);
+	vpmu_vm.gic_fd = vgic_v3_setup(vpmu_vm.vm, 1, 64);
+	__TEST_REQUIRE(vpmu_vm.gic_fd >= 0,
+		       "Failed to create vgic-v3, skipping");
+
+	pmu_event_filter_init(filter);
+
+	/* Initialize vPMU */
+	vpmu_set_irq(vpmu_vm.vcpu, irq);
+	vpmu_init(vpmu_vm.vcpu);
+}
+
+static void create_vpmu_vm(void *guest_code)
+{
+	create_vpmu_vm_with_filter(guest_code, NULL);
+}
+
+static void destroy_vpmu_vm(void)
+{
+	close(vpmu_vm.gic_fd);
+	kvm_vm_free(vpmu_vm.vm);
+}
+
+static void run_test(struct test_desc *t)
+{
+	pr_info("Test: %s\n", t->name);
+
+	create_vpmu_vm_with_filter(guest_code, t->filter);
+	prepare_expected_pmce(t->filter);
+	sync_global_to_guest(vpmu_vm.vm, expected_pmce);
+
+	run_vcpu(vpmu_vm.vcpu);
+
+	destroy_vpmu_vm();
+}
+
+static struct test_desc tests[] = {
+	{
+		.name = "without_filter",
+		.filter = {
+			{ 0 }
+		},
+	},
+	{
+		.name = "member_allow_filter",
+		.filter = {
+			DEFINE_FILTER(ARMV8_PMUV3_PERFCTR_SW_INCR, 0),
+			DEFINE_FILTER(ARMV8_PMUV3_PERFCTR_INST_RETIRED, 0),
+			DEFINE_FILTER(ARMV8_PMUV3_PERFCTR_BR_RETIRED, 0),
+			{ 0 },
+		},
+	},
+	{
+		.name = "member_deny_filter",
+		.filter = {
+			DEFINE_FILTER(ARMV8_PMUV3_PERFCTR_SW_INCR, 1),
+			DEFINE_FILTER(ARMV8_PMUV3_PERFCTR_INST_RETIRED, 1),
+			DEFINE_FILTER(ARMV8_PMUV3_PERFCTR_BR_RETIRED, 1),
+			{ 0 },
+		},
+	},
+	{
+		.name = "not_member_deny_filter",
+		.filter = {
+			DEFINE_FILTER(ARMV8_PMUV3_PERFCTR_SW_INCR, 1),
+			{ 0 },
+		},
+	},
+	{
+		.name = "not_member_allow_filter",
+		.filter = {
+			DEFINE_FILTER(ARMV8_PMUV3_PERFCTR_SW_INCR, 0),
+			{ 0 },
+		},
+	},
+	{
+		.name = "deny_chain_filter",
+		.filter = {
+			DEFINE_FILTER(ARMV8_PMUV3_PERFCTR_CHAIN, 1),
+			{ 0 },
+		},
+	},
+	{
+		.name = "deny_cpu_cycles_filter",
+		.filter = {
+			DEFINE_FILTER(ARMV8_PMUV3_PERFCTR_CPU_CYCLES, 1),
+			{ 0 },
+		},
+	},
+	{
+		.name = "cancel_filter",
+		.filter = {
+			DEFINE_FILTER(ARMV8_PMUV3_PERFCTR_CPU_CYCLES, 0),
+			DEFINE_FILTER(ARMV8_PMUV3_PERFCTR_CPU_CYCLES, 1),
+		},
+	},
+	{
+		.name = "multiple_filter",
+		.filter = {
+			__DEFINE_FILTER(0x0, 0x10, 0),
+			__DEFINE_FILTER(0x6, 0x3, 1),
+		},
+	},
+	{ 0 }
+};
+
+static void run_tests(void)
+{
+	struct test_desc *t;
+
+	for (t = &tests[0]; t->name; t++)
+		run_test(t);
+}
+
+int used_pmu_events[] = {
+       ARMV8_PMUV3_PERFCTR_BR_RETIRED,
+       ARMV8_PMUV3_PERFCTR_INST_RETIRED,
+       ARMV8_PMUV3_PERFCTR_CHAIN,
+};
+
+static bool kvm_pmu_support_events(void)
+{
+	struct pmu_common_event_ids used_pmce = { 0, 0 };
+
+	create_vpmu_vm(guest_get_pmceid);
+
+	memset(&max_pmce, 0, sizeof(max_pmce));
+	sync_global_to_guest(vpmu_vm.vm, max_pmce);
+	run_vcpu(vpmu_vm.vcpu);
+	sync_global_from_guest(vpmu_vm.vm, max_pmce);
+	destroy_vpmu_vm();
+
+	for (int i = 0; i < ARRAY_SIZE(used_pmu_events); i++)
+		set_pmce(&used_pmce, KVM_PMU_EVENT_ALLOW, used_pmu_events[i]);
+
+	return ((max_pmce.pmceid0 & used_pmce.pmceid0) == used_pmce.pmceid0) &&
+	       ((max_pmce.pmceid1 & used_pmce.pmceid1) == used_pmce.pmceid1);
+}
+
+int main(void)
+{
+	TEST_REQUIRE(kvm_has_cap(KVM_CAP_ARM_PMU_V3));
+	TEST_REQUIRE(kvm_pmu_support_events());
+
+	run_tests();
+}