[PATCH v4 0/5] KVM: selftests: aarch64: Introduce pmu_event_filter_test

[PATCH v4 0/5] KVM: selftests: aarch64: Introduce pmu_event_filter_test

[Shaoqin Huang]

The test is inspired by the pmu_event_filter_test which implemented by x86. On
the arm64 platform, there is the same ability to set the pmu_event_filter
through the KVM_ARM_VCPU_PMU_V3_FILTER attribute. So add the test for arm64.

The series first move some pmu common code from vpmu_counter_access to
lib/aarch64/vpmu.c and include/aarch64/vpmu.h, which can be used by
pmu_event_filter_test. Then fix a bug related to the [enable|disable]_counter,
and at last, implement the test itself.

Changelog:
----------
v3->v4:
  - Rebased to the v6.8-rc2.

v2->v3:
  - Check the pmceid in guest code instead of pmu event count since different
  hardware may have different event count result, check pmceid makes it stable
  on different platform.                        [Eric]
  - Some typo fixed and commit message improved.

v1->v2:
  - Improve the commit message.                 [Eric]
  - Fix the bug in [enable|disable]_counter.    [Raghavendra & Marc]
  - Add the check if kvm has attr KVM_ARM_VCPU_PMU_V3_FILTER.
  - Add if host pmu support the test event throught pmceid0.
  - Split the test_invalid_filter() to another patch. [Eric]

v1: https://lore.kernel.org/all/20231123063750.2176250-1-shahuang@redhat.com/
v2: https://lore.kernel.org/all/20231129072712.2667337-1-shahuang@redhat.com/
v3: https://lore.kernel.org/all/20240116060129.55473-1-shahuang@redhat.com/

Shaoqin Huang (5):
  KVM: selftests: aarch64: Make the [create|destroy]_vpmu_vm() public
  KVM: selftests: aarch64: Move pmu helper functions into vpmu.h
  KVM: selftests: aarch64: Fix the buggy [enable|disable]_counter
  KVM: selftests: aarch64: Introduce pmu_event_filter_test
  KVM: selftests: aarch64: Add invalid filter test in
    pmu_event_filter_test

 tools/testing/selftests/kvm/Makefile          |   2 +
 .../kvm/aarch64/pmu_event_filter_test.c       | 255 ++++++++++++++++++
 .../kvm/aarch64/vpmu_counter_access.c         | 217 ++-------------
 .../selftests/kvm/include/aarch64/vpmu.h      | 134 +++++++++
 .../testing/selftests/kvm/lib/aarch64/vpmu.c  |  74 +++++
 5 files changed, 489 insertions(+), 193 deletions(-)
 create mode 100644 tools/testing/selftests/kvm/aarch64/pmu_event_filter_test.c
 create mode 100644 tools/testing/selftests/kvm/include/aarch64/vpmu.h
 create mode 100644 tools/testing/selftests/kvm/lib/aarch64/vpmu.c


base-commit: 41bccc98fb7931d63d03f326a746ac4d429c1dd3
-- 
2.40.1

[PATCH v4 1/5] KVM: selftests: aarch64: Make the [create|destroy]_vpmu_vm() public

[Shaoqin Huang]

Move the implementation of [create|destroy]_vpmu_vm() into
lib/aarch64/pmu.c and export their declaration in a header so they can
be reused by other tests.

The sync exception handler install is test specific so we move it out of
the helper function.

No functional change intended.

Reviewed-by: Eric Auger <eric.auger@redhat.com>
Signed-off-by: Shaoqin Huang <shahuang@redhat.com>
---
 tools/testing/selftests/kvm/Makefile          |   1 +
 .../kvm/aarch64/vpmu_counter_access.c         | 100 +++++-------------
 .../selftests/kvm/include/aarch64/vpmu.h      |  16 +++
 .../testing/selftests/kvm/lib/aarch64/vpmu.c  |  64 +++++++++++
 4 files changed, 105 insertions(+), 76 deletions(-)
 create mode 100644 tools/testing/selftests/kvm/include/aarch64/vpmu.h
 create mode 100644 tools/testing/selftests/kvm/lib/aarch64/vpmu.c

diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
index 492e937fab00..709a70b31ca2 100644
--- a/tools/testing/selftests/kvm/Makefile
+++ b/tools/testing/selftests/kvm/Makefile
@@ -48,6 +48,7 @@ LIBKVM_aarch64 += lib/aarch64/processor.c
 LIBKVM_aarch64 += lib/aarch64/spinlock.c
 LIBKVM_aarch64 += lib/aarch64/ucall.c
 LIBKVM_aarch64 += lib/aarch64/vgic.c
+LIBKVM_aarch64 += lib/aarch64/vpmu.c
 
 LIBKVM_s390x += lib/s390x/diag318_test_handler.c
 LIBKVM_s390x += lib/s390x/processor.c
diff --git a/tools/testing/selftests/kvm/aarch64/vpmu_counter_access.c b/tools/testing/selftests/kvm/aarch64/vpmu_counter_access.c
index 9d51b5691349..bad0fdbe6d34 100644
--- a/tools/testing/selftests/kvm/aarch64/vpmu_counter_access.c
+++ b/tools/testing/selftests/kvm/aarch64/vpmu_counter_access.c
@@ -16,6 +16,7 @@
 #include <processor.h>
 #include <test_util.h>
 #include <vgic.h>
+#include <vpmu.h>
 #include <perf/arm_pmuv3.h>
 #include <linux/bitfield.h>
 
@@ -25,13 +26,7 @@
 /* The cycle counter bit position that's common among the PMU registers */
 #define ARMV8_PMU_CYCLE_IDX		31
 
-struct vpmu_vm {
-	struct kvm_vm *vm;
-	struct kvm_vcpu *vcpu;
-	int gic_fd;
-};
-
-static struct vpmu_vm vpmu_vm;
+static struct vpmu_vm *vpmu_vm;
 
 struct pmreg_sets {
 	uint64_t set_reg_id;
@@ -420,64 +415,6 @@ static void guest_code(uint64_t expected_pmcr_n)
 	GUEST_DONE();
 }
 
-#define GICD_BASE_GPA	0x8000000ULL
-#define GICR_BASE_GPA	0x80A0000ULL
-
-/* Create a VM that has one vCPU with PMUv3 configured. */
-static void create_vpmu_vm(void *guest_code)
-{
-	struct kvm_vcpu_init init;
-	uint8_t pmuver, ec;
-	uint64_t dfr0, irq = 23;
-	struct kvm_device_attr irq_attr = {
-		.group = KVM_ARM_VCPU_PMU_V3_CTRL,
-		.attr = KVM_ARM_VCPU_PMU_V3_IRQ,
-		.addr = (uint64_t)&irq,
-	};
-	struct kvm_device_attr init_attr = {
-		.group = KVM_ARM_VCPU_PMU_V3_CTRL,
-		.attr = KVM_ARM_VCPU_PMU_V3_INIT,
-	};
-
-	/* 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);
-	vm_init_descriptor_tables(vpmu_vm.vm);
-	for (ec = 0; ec < ESR_EC_NUM; ec++) {
-		vm_install_sync_handler(vpmu_vm.vm, VECTOR_SYNC_CURRENT, ec,
-					guest_sync_handler);
-	}
-
-	/* Create vCPU with PMUv3 */
-	vm_ioctl(vpmu_vm.vm, KVM_ARM_PREFERRED_TARGET, &init);
-	init.features[0] |= (1 << KVM_ARM_VCPU_PMU_V3);
-	vpmu_vm.vcpu = aarch64_vcpu_add(vpmu_vm.vm, 0, &init, guest_code);
-	vcpu_init_descriptor_tables(vpmu_vm.vcpu);
-	vpmu_vm.gic_fd = vgic_v3_setup(vpmu_vm.vm, 1, 64,
-					GICD_BASE_GPA, GICR_BASE_GPA);
-	__TEST_REQUIRE(vpmu_vm.gic_fd >= 0,
-		       "Failed to create vgic-v3, skipping");
-
-	/* Make sure that PMUv3 support is indicated in the ID register */
-	vcpu_get_reg(vpmu_vm.vcpu,
-		     KVM_ARM64_SYS_REG(SYS_ID_AA64DFR0_EL1), &dfr0);
-	pmuver = FIELD_GET(ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_PMUVer), dfr0);
-	TEST_ASSERT(pmuver != ID_AA64DFR0_EL1_PMUVer_IMP_DEF &&
-		    pmuver >= ID_AA64DFR0_EL1_PMUVer_IMP,
-		    "Unexpected PMUVER (0x%x) on the vCPU with PMUv3", pmuver);
-
-	/* Initialize vPMU */
-	vcpu_ioctl(vpmu_vm.vcpu, KVM_SET_DEVICE_ATTR, &irq_attr);
-	vcpu_ioctl(vpmu_vm.vcpu, KVM_SET_DEVICE_ATTR, &init_attr);
-}
-
-static void destroy_vpmu_vm(void)
-{
-	close(vpmu_vm.gic_fd);
-	kvm_vm_free(vpmu_vm.vm);
-}
-
 static void run_vcpu(struct kvm_vcpu *vcpu, uint64_t pmcr_n)
 {
 	struct ucall uc;
@@ -496,13 +433,24 @@ static void run_vcpu(struct kvm_vcpu *vcpu, uint64_t pmcr_n)
 	}
 }
 
+static void create_vpmu_vm_with_handler(void *guest_code)
+{
+	uint8_t ec;
+	vpmu_vm = create_vpmu_vm(guest_code);
+
+	for (ec = 0; ec < ESR_EC_NUM; ec++) {
+		vm_install_sync_handler(vpmu_vm->vm, VECTOR_SYNC_CURRENT, ec,
+					guest_sync_handler);
+	}
+}
+
 static void test_create_vpmu_vm_with_pmcr_n(uint64_t pmcr_n, bool expect_fail)
 {
 	struct kvm_vcpu *vcpu;
 	uint64_t pmcr, pmcr_orig;
 
-	create_vpmu_vm(guest_code);
-	vcpu = vpmu_vm.vcpu;
+	create_vpmu_vm_with_handler(guest_code);
+	vcpu = vpmu_vm->vcpu;
 
 	vcpu_get_reg(vcpu, KVM_ARM64_SYS_REG(SYS_PMCR_EL0), &pmcr_orig);
 	pmcr = pmcr_orig;
@@ -538,7 +486,7 @@ static void run_access_test(uint64_t pmcr_n)
 	pr_debug("Test with pmcr_n %lu\n", pmcr_n);
 
 	test_create_vpmu_vm_with_pmcr_n(pmcr_n, false);
-	vcpu = vpmu_vm.vcpu;
+	vcpu = vpmu_vm->vcpu;
 
 	/* Save the initial sp to restore them later to run the guest again */
 	vcpu_get_reg(vcpu, ARM64_CORE_REG(sp_el1), &sp);
@@ -549,7 +497,7 @@ static void run_access_test(uint64_t pmcr_n)
 	 * Reset and re-initialize the vCPU, and run the guest code again to
 	 * check if PMCR_EL0.N is preserved.
 	 */
-	vm_ioctl(vpmu_vm.vm, KVM_ARM_PREFERRED_TARGET, &init);
+	vm_ioctl(vpmu_vm->vm, KVM_ARM_PREFERRED_TARGET, &init);
 	init.features[0] |= (1 << KVM_ARM_VCPU_PMU_V3);
 	aarch64_vcpu_setup(vcpu, &init);
 	vcpu_init_descriptor_tables(vcpu);
@@ -558,7 +506,7 @@ static void run_access_test(uint64_t pmcr_n)
 
 	run_vcpu(vcpu, pmcr_n);
 
-	destroy_vpmu_vm();
+	destroy_vpmu_vm(vpmu_vm);
 }
 
 static struct pmreg_sets validity_check_reg_sets[] = {
@@ -579,7 +527,7 @@ static void run_pmregs_validity_test(uint64_t pmcr_n)
 	uint64_t valid_counters_mask, max_counters_mask;
 
 	test_create_vpmu_vm_with_pmcr_n(pmcr_n, false);
-	vcpu = vpmu_vm.vcpu;
+	vcpu = vpmu_vm->vcpu;
 
 	valid_counters_mask = get_counters_mask(pmcr_n);
 	max_counters_mask = get_counters_mask(ARMV8_PMU_MAX_COUNTERS);
@@ -620,7 +568,7 @@ static void run_pmregs_validity_test(uint64_t pmcr_n)
 			    KVM_ARM64_SYS_REG(clr_reg_id), reg_val);
 	}
 
-	destroy_vpmu_vm();
+	destroy_vpmu_vm(vpmu_vm);
 }
 
 /*
@@ -633,7 +581,7 @@ static void run_error_test(uint64_t pmcr_n)
 	pr_debug("Error test with pmcr_n %lu (larger than the host)\n", pmcr_n);
 
 	test_create_vpmu_vm_with_pmcr_n(pmcr_n, true);
-	destroy_vpmu_vm();
+	destroy_vpmu_vm(vpmu_vm);
 }
 
 /*
@@ -644,9 +592,9 @@ static uint64_t get_pmcr_n_limit(void)
 {
 	uint64_t pmcr;
 
-	create_vpmu_vm(guest_code);
-	vcpu_get_reg(vpmu_vm.vcpu, KVM_ARM64_SYS_REG(SYS_PMCR_EL0), &pmcr);
-	destroy_vpmu_vm();
+	create_vpmu_vm_with_handler(guest_code);
+	vcpu_get_reg(vpmu_vm->vcpu, KVM_ARM64_SYS_REG(SYS_PMCR_EL0), &pmcr);
+	destroy_vpmu_vm(vpmu_vm);
 	return get_pmcr_n(pmcr);
 }
 
diff --git a/tools/testing/selftests/kvm/include/aarch64/vpmu.h b/tools/testing/selftests/kvm/include/aarch64/vpmu.h
new file mode 100644
index 000000000000..0a56183644ee
--- /dev/null
+++ b/tools/testing/selftests/kvm/include/aarch64/vpmu.h
@@ -0,0 +1,16 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#include <kvm_util.h>
+
+#define GICD_BASE_GPA	0x8000000ULL
+#define GICR_BASE_GPA	0x80A0000ULL
+
+struct vpmu_vm {
+	struct kvm_vm *vm;
+	struct kvm_vcpu *vcpu;
+	int gic_fd;
+};
+
+struct vpmu_vm *create_vpmu_vm(void *guest_code);
+
+void destroy_vpmu_vm(struct vpmu_vm *vpmu_vm);
diff --git a/tools/testing/selftests/kvm/lib/aarch64/vpmu.c b/tools/testing/selftests/kvm/lib/aarch64/vpmu.c
new file mode 100644
index 000000000000..b3de8fdc555e
--- /dev/null
+++ b/tools/testing/selftests/kvm/lib/aarch64/vpmu.c
@@ -0,0 +1,64 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <kvm_util.h>
+#include <processor.h>
+#include <test_util.h>
+#include <vgic.h>
+#include <vpmu.h>
+#include <perf/arm_pmuv3.h>
+
+/* Create a VM that has one vCPU with PMUv3 configured. */
+struct vpmu_vm *create_vpmu_vm(void *guest_code)
+{
+	struct kvm_vcpu_init init;
+	uint8_t pmuver;
+	uint64_t dfr0, irq = 23;
+	struct kvm_device_attr irq_attr = {
+		.group = KVM_ARM_VCPU_PMU_V3_CTRL,
+		.attr = KVM_ARM_VCPU_PMU_V3_IRQ,
+		.addr = (uint64_t)&irq,
+	};
+	struct kvm_device_attr init_attr = {
+		.group = KVM_ARM_VCPU_PMU_V3_CTRL,
+		.attr = KVM_ARM_VCPU_PMU_V3_INIT,
+	};
+	struct vpmu_vm *vpmu_vm;
+
+	vpmu_vm = calloc(1, sizeof(*vpmu_vm));
+	TEST_ASSERT(vpmu_vm != NULL, "Insufficient Memory");
+	memset(vpmu_vm, 0, sizeof(vpmu_vm));
+
+	vpmu_vm->vm = vm_create(1);
+	vm_init_descriptor_tables(vpmu_vm->vm);
+
+	/* Create vCPU with PMUv3 */
+	vm_ioctl(vpmu_vm->vm, KVM_ARM_PREFERRED_TARGET, &init);
+	init.features[0] |= (1 << KVM_ARM_VCPU_PMU_V3);
+	vpmu_vm->vcpu = aarch64_vcpu_add(vpmu_vm->vm, 0, &init, guest_code);
+	vcpu_init_descriptor_tables(vpmu_vm->vcpu);
+	vpmu_vm->gic_fd = vgic_v3_setup(vpmu_vm->vm, 1, 64,
+					GICD_BASE_GPA, GICR_BASE_GPA);
+	__TEST_REQUIRE(vpmu_vm->gic_fd >= 0,
+		       "Failed to create vgic-v3, skipping");
+
+	/* Make sure that PMUv3 support is indicated in the ID register */
+	vcpu_get_reg(vpmu_vm->vcpu,
+		     KVM_ARM64_SYS_REG(SYS_ID_AA64DFR0_EL1), &dfr0);
+	pmuver = FIELD_GET(ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_PMUVer), dfr0);
+	TEST_ASSERT(pmuver != ID_AA64DFR0_EL1_PMUVer_IMP_DEF &&
+		    pmuver >= ID_AA64DFR0_EL1_PMUVer_IMP,
+		    "Unexpected PMUVER (0x%x) on the vCPU with PMUv3", pmuver);
+
+	/* Initialize vPMU */
+	vcpu_ioctl(vpmu_vm->vcpu, KVM_SET_DEVICE_ATTR, &irq_attr);
+	vcpu_ioctl(vpmu_vm->vcpu, KVM_SET_DEVICE_ATTR, &init_attr);
+
+	return vpmu_vm;
+}
+
+void destroy_vpmu_vm(struct vpmu_vm *vpmu_vm)
+{
+	close(vpmu_vm->gic_fd);
+	kvm_vm_free(vpmu_vm->vm);
+	free(vpmu_vm);
+}
-- 
2.40.1

[PATCH v4 2/5] KVM: selftests: aarch64: Move pmu helper functions into vpmu.h

[Shaoqin Huang]

Move those pmu helper functions into include/aarch64/vpmu.h, thus
it can be used by other pmu test.

No functional change intended.

Reviewed-by: Eric Auger <eric.auger@redhat.com>
Signed-off-by: Shaoqin Huang <shahuang@redhat.com>
---
 .../kvm/aarch64/vpmu_counter_access.c         | 117 -----------------
 .../selftests/kvm/include/aarch64/vpmu.h      | 118 ++++++++++++++++++
 2 files changed, 118 insertions(+), 117 deletions(-)

diff --git a/tools/testing/selftests/kvm/aarch64/vpmu_counter_access.c b/tools/testing/selftests/kvm/aarch64/vpmu_counter_access.c
index bad0fdbe6d34..62d6315790ab 100644
--- a/tools/testing/selftests/kvm/aarch64/vpmu_counter_access.c
+++ b/tools/testing/selftests/kvm/aarch64/vpmu_counter_access.c
@@ -20,12 +20,6 @@
 #include <perf/arm_pmuv3.h>
 #include <linux/bitfield.h>
 
-/* The max number of the PMU event counters (excluding the cycle counter) */
-#define ARMV8_PMU_MAX_GENERAL_COUNTERS	(ARMV8_PMU_MAX_COUNTERS - 1)
-
-/* The cycle counter bit position that's common among the PMU registers */
-#define ARMV8_PMU_CYCLE_IDX		31
-
 static struct vpmu_vm *vpmu_vm;
 
 struct pmreg_sets {
@@ -35,117 +29,6 @@ struct pmreg_sets {
 
 #define PMREG_SET(set, clr) {.set_reg_id = set, .clr_reg_id = clr}
 
-static uint64_t get_pmcr_n(uint64_t pmcr)
-{
-	return FIELD_GET(ARMV8_PMU_PMCR_N, pmcr);
-}
-
-static void set_pmcr_n(uint64_t *pmcr, uint64_t pmcr_n)
-{
-	u64p_replace_bits((__u64 *) pmcr, pmcr_n, ARMV8_PMU_PMCR_N);
-}
-
-static uint64_t get_counters_mask(uint64_t n)
-{
-	uint64_t mask = BIT(ARMV8_PMU_CYCLE_IDX);
-
-	if (n)
-		mask |= GENMASK(n - 1, 0);
-	return mask;
-}
-
-/* Read PMEVTCNTR<n>_EL0 through PMXEVCNTR_EL0 */
-static inline unsigned long read_sel_evcntr(int sel)
-{
-	write_sysreg(sel, pmselr_el0);
-	isb();
-	return read_sysreg(pmxevcntr_el0);
-}
-
-/* Write PMEVTCNTR<n>_EL0 through PMXEVCNTR_EL0 */
-static inline void write_sel_evcntr(int sel, unsigned long val)
-{
-	write_sysreg(sel, pmselr_el0);
-	isb();
-	write_sysreg(val, pmxevcntr_el0);
-	isb();
-}
-
-/* Read PMEVTYPER<n>_EL0 through PMXEVTYPER_EL0 */
-static inline unsigned long read_sel_evtyper(int sel)
-{
-	write_sysreg(sel, pmselr_el0);
-	isb();
-	return read_sysreg(pmxevtyper_el0);
-}
-
-/* Write PMEVTYPER<n>_EL0 through PMXEVTYPER_EL0 */
-static inline void write_sel_evtyper(int sel, unsigned long val)
-{
-	write_sysreg(sel, pmselr_el0);
-	isb();
-	write_sysreg(val, pmxevtyper_el0);
-	isb();
-}
-
-static inline void enable_counter(int idx)
-{
-	uint64_t v = read_sysreg(pmcntenset_el0);
-
-	write_sysreg(BIT(idx) | v, pmcntenset_el0);
-	isb();
-}
-
-static inline void disable_counter(int idx)
-{
-	uint64_t v = read_sysreg(pmcntenset_el0);
-
-	write_sysreg(BIT(idx) | v, pmcntenclr_el0);
-	isb();
-}
-
-static void pmu_disable_reset(void)
-{
-	uint64_t pmcr = read_sysreg(pmcr_el0);
-
-	/* Reset all counters, disabling them */
-	pmcr &= ~ARMV8_PMU_PMCR_E;
-	write_sysreg(pmcr | ARMV8_PMU_PMCR_P, pmcr_el0);
-	isb();
-}
-
-#define RETURN_READ_PMEVCNTRN(n) \
-	return read_sysreg(pmevcntr##n##_el0)
-static unsigned long read_pmevcntrn(int n)
-{
-	PMEVN_SWITCH(n, RETURN_READ_PMEVCNTRN);
-	return 0;
-}
-
-#define WRITE_PMEVCNTRN(n) \
-	write_sysreg(val, pmevcntr##n##_el0)
-static void write_pmevcntrn(int n, unsigned long val)
-{
-	PMEVN_SWITCH(n, WRITE_PMEVCNTRN);
-	isb();
-}
-
-#define READ_PMEVTYPERN(n) \
-	return read_sysreg(pmevtyper##n##_el0)
-static unsigned long read_pmevtypern(int n)
-{
-	PMEVN_SWITCH(n, READ_PMEVTYPERN);
-	return 0;
-}
-
-#define WRITE_PMEVTYPERN(n) \
-	write_sysreg(val, pmevtyper##n##_el0)
-static void write_pmevtypern(int n, unsigned long val)
-{
-	PMEVN_SWITCH(n, WRITE_PMEVTYPERN);
-	isb();
-}
-
 /*
  * The pmc_accessor structure has pointers to PMEV{CNTR,TYPER}<n>_EL0
  * accessors that test cases will use. Each of the accessors will
diff --git a/tools/testing/selftests/kvm/include/aarch64/vpmu.h b/tools/testing/selftests/kvm/include/aarch64/vpmu.h
index 0a56183644ee..f78c93a08bff 100644
--- a/tools/testing/selftests/kvm/include/aarch64/vpmu.h
+++ b/tools/testing/selftests/kvm/include/aarch64/vpmu.h
@@ -1,10 +1,17 @@
 /* SPDX-License-Identifier: GPL-2.0 */
 
 #include <kvm_util.h>
+#include <perf/arm_pmuv3.h>
 
 #define GICD_BASE_GPA	0x8000000ULL
 #define GICR_BASE_GPA	0x80A0000ULL
 
+/* The max number of the PMU event counters (excluding the cycle counter) */
+#define ARMV8_PMU_MAX_GENERAL_COUNTERS	(ARMV8_PMU_MAX_COUNTERS - 1)
+
+/* The cycle counter bit position that's common among the PMU registers */
+#define ARMV8_PMU_CYCLE_IDX		31
+
 struct vpmu_vm {
 	struct kvm_vm *vm;
 	struct kvm_vcpu *vcpu;
@@ -14,3 +21,114 @@ struct vpmu_vm {
 struct vpmu_vm *create_vpmu_vm(void *guest_code);
 
 void destroy_vpmu_vm(struct vpmu_vm *vpmu_vm);
+
+static inline uint64_t get_pmcr_n(uint64_t pmcr)
+{
+	return FIELD_GET(ARMV8_PMU_PMCR_N, pmcr);
+}
+
+static inline void set_pmcr_n(uint64_t *pmcr, uint64_t pmcr_n)
+{
+	u64p_replace_bits((__u64 *) pmcr, pmcr_n, ARMV8_PMU_PMCR_N);
+}
+
+static inline uint64_t get_counters_mask(uint64_t n)
+{
+	uint64_t mask = BIT(ARMV8_PMU_CYCLE_IDX);
+
+	if (n)
+		mask |= GENMASK(n - 1, 0);
+	return mask;
+}
+
+/* Read PMEVTCNTR<n>_EL0 through PMXEVCNTR_EL0 */
+static inline unsigned long read_sel_evcntr(int sel)
+{
+	write_sysreg(sel, pmselr_el0);
+	isb();
+	return read_sysreg(pmxevcntr_el0);
+}
+
+/* Write PMEVTCNTR<n>_EL0 through PMXEVCNTR_EL0 */
+static inline void write_sel_evcntr(int sel, unsigned long val)
+{
+	write_sysreg(sel, pmselr_el0);
+	isb();
+	write_sysreg(val, pmxevcntr_el0);
+	isb();
+}
+
+/* Read PMEVTYPER<n>_EL0 through PMXEVTYPER_EL0 */
+static inline unsigned long read_sel_evtyper(int sel)
+{
+	write_sysreg(sel, pmselr_el0);
+	isb();
+	return read_sysreg(pmxevtyper_el0);
+}
+
+/* Write PMEVTYPER<n>_EL0 through PMXEVTYPER_EL0 */
+static inline void write_sel_evtyper(int sel, unsigned long val)
+{
+	write_sysreg(sel, pmselr_el0);
+	isb();
+	write_sysreg(val, pmxevtyper_el0);
+	isb();
+}
+
+static inline void enable_counter(int idx)
+{
+	uint64_t v = read_sysreg(pmcntenset_el0);
+
+	write_sysreg(BIT(idx) | v, pmcntenset_el0);
+	isb();
+}
+
+static inline void disable_counter(int idx)
+{
+	uint64_t v = read_sysreg(pmcntenset_el0);
+
+	write_sysreg(BIT(idx) | v, pmcntenclr_el0);
+	isb();
+}
+
+static inline void pmu_disable_reset(void)
+{
+	uint64_t pmcr = read_sysreg(pmcr_el0);
+
+	/* Reset all counters, disabling them */
+	pmcr &= ~ARMV8_PMU_PMCR_E;
+	write_sysreg(pmcr | ARMV8_PMU_PMCR_P, pmcr_el0);
+	isb();
+}
+
+#define RETURN_READ_PMEVCNTRN(n) \
+	return read_sysreg(pmevcntr##n##_el0)
+static inline unsigned long read_pmevcntrn(int n)
+{
+	PMEVN_SWITCH(n, RETURN_READ_PMEVCNTRN);
+	return 0;
+}
+
+#define WRITE_PMEVCNTRN(n) \
+	write_sysreg(val, pmevcntr##n##_el0)
+static inline void write_pmevcntrn(int n, unsigned long val)
+{
+	PMEVN_SWITCH(n, WRITE_PMEVCNTRN);
+	isb();
+}
+
+#define READ_PMEVTYPERN(n) \
+	return read_sysreg(pmevtyper##n##_el0)
+static inline unsigned long read_pmevtypern(int n)
+{
+	PMEVN_SWITCH(n, READ_PMEVTYPERN);
+	return 0;
+}
+
+#define WRITE_PMEVTYPERN(n) \
+	write_sysreg(val, pmevtyper##n##_el0)
+static inline void write_pmevtypern(int n, unsigned long val)
+{
+	PMEVN_SWITCH(n, WRITE_PMEVTYPERN);
+	isb();
+}
-- 
2.40.1

[PATCH v4 3/5] KVM: selftests: aarch64: Fix the buggy [enable|disable]_counter

[Shaoqin Huang]

In general, the set/clr registers should always be used in their
write form, never in a RMW form (imagine an interrupt disabling
a counter between the read and the write...).

The current implementation of [enable|disable]_counter both use the RMW
form, fix them by directly write to the set/clr registers.

At the same time, it also fix the buggy disable_counter() which would
end up disabling all the counters.

Signed-off-by: Shaoqin Huang <shahuang@redhat.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
---
 tools/testing/selftests/kvm/include/aarch64/vpmu.h | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/tools/testing/selftests/kvm/include/aarch64/vpmu.h b/tools/testing/selftests/kvm/include/aarch64/vpmu.h
index f78c93a08bff..92715021f892 100644
--- a/tools/testing/selftests/kvm/include/aarch64/vpmu.h
+++ b/tools/testing/selftests/kvm/include/aarch64/vpmu.h
@@ -77,17 +77,13 @@ static inline void write_sel_evtyper(int sel, unsigned long val)
 
 static inline void enable_counter(int idx)
 {
-	uint64_t v = read_sysreg(pmcntenset_el0);
-
-	write_sysreg(BIT(idx) | v, pmcntenset_el0);
+	write_sysreg(BIT(idx), pmcntenset_el0);
 	isb();
 }
 
 static inline void disable_counter(int idx)
 {
-	uint64_t v = read_sysreg(pmcntenset_el0);
-
-	write_sysreg(BIT(idx) | v, pmcntenclr_el0);
+	write_sysreg(BIT(idx), pmcntenclr_el0);
 	isb();
 }
 
-- 
2.40.1

[PATCH v4 4/5] KVM: selftests: aarch64: Introduce pmu_event_filter_test

[Shaoqin Huang]

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       | 219 ++++++++++++++++++
 .../selftests/kvm/include/aarch64/vpmu.h      |   4 +
 .../testing/selftests/kvm/lib/aarch64/vpmu.c  |  14 +-
 4 files changed, 236 insertions(+), 2 deletions(-)
 create mode 100644 tools/testing/selftests/kvm/aarch64/pmu_event_filter_test.c

diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
index 709a70b31ca2..733ec86a3385 100644
--- a/tools/testing/selftests/kvm/Makefile
+++ b/tools/testing/selftests/kvm/Makefile
@@ -148,6 +148,7 @@ TEST_GEN_PROGS_aarch64 += aarch64/arch_timer
 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..d280382f362f
--- /dev/null
+++ b/tools/testing/selftests/kvm/aarch64/pmu_event_filter_test.c
@@ -0,0 +1,219 @@
+// 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 pmce{
+	uint64_t pmceid0;
+	uint64_t pmceid1;
+} supported_pmce, guest_pmce;
+
+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)
+
+#define EMPTY_FILTER	{ 0 }
+
+#define SW_INCR		0x0
+#define INST_RETIRED	0x8
+#define BR_RETIRED	0x21
+
+static void guest_code(void)
+{
+	uint64_t pmceid0 = read_sysreg(pmceid0_el0);
+	uint64_t pmceid1 = read_sysreg(pmceid1_el0);
+
+	GUEST_ASSERT_EQ(guest_pmce.pmceid0, pmceid0);
+	GUEST_ASSERT_EQ(guest_pmce.pmceid1, pmceid1);
+
+	GUEST_DONE();
+}
+
+static void guest_get_pmceid(void)
+{
+	supported_pmce.pmceid0 = read_sysreg(pmceid0_el0);
+	supported_pmce.pmceid1 = read_sysreg(pmceid1_el0);
+
+	GUEST_DONE();
+}
+
+static void pmu_event_filter_init(struct vpmu_vm *vm, void *arg)
+{
+	struct kvm_device_attr attr = {
+		.group	= KVM_ARM_VCPU_PMU_V3_CTRL,
+		.attr	= KVM_ARM_VCPU_PMU_V3_FILTER,
+	};
+	struct kvm_pmu_event_filter *filter = (struct kvm_pmu_event_filter *)arg;
+
+	while (filter && filter->nevents != 0) {
+		attr.addr = (uint64_t)filter;
+		vcpu_ioctl(vm->vcpu, KVM_SET_DEVICE_ATTR, &attr);
+		filter++;
+	}
+}
+
+static void create_vpmu_vm_with_filter(void *guest_code,
+				       struct kvm_pmu_event_filter *filter)
+{
+	vpmu_vm = __create_vpmu_vm(guest_code, pmu_event_filter_init, filter);
+}
+
+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;
+		default:
+			TEST_FAIL("Unknown ucall %lu", uc.cmd);
+		}
+	}
+}
+
+static void set_pmce(struct pmce *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_guest_pmce(struct kvm_pmu_event_filter *filter)
+{
+	struct pmce pmce_mask = { ~0, ~0 };
+	bool first_filter = true;
+
+	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;
+		}
+
+		set_pmce(&pmce_mask, filter->action, filter->base_event);
+		filter++;
+	}
+
+	guest_pmce.pmceid0 = supported_pmce.pmceid0 & pmce_mask.pmceid0;
+	guest_pmce.pmceid1 = supported_pmce.pmceid1 & pmce_mask.pmceid1;
+}
+
+static void run_test(struct test_desc *t)
+{
+	pr_debug("Test: %s\n", t->name);
+
+	create_vpmu_vm_with_filter(guest_code, t->filter);
+	prepare_guest_pmce(t->filter);
+	sync_global_to_guest(vpmu_vm->vm, guest_pmce);
+
+	run_vcpu(vpmu_vm->vcpu);
+
+	destroy_vpmu_vm(vpmu_vm);
+}
+
+static struct test_desc tests[] = {
+	{"without_filter", { EMPTY_FILTER }},
+	{"member_allow_filter",
+	 {DEFINE_FILTER(SW_INCR, 0), DEFINE_FILTER(INST_RETIRED, 0),
+	  DEFINE_FILTER(BR_RETIRED, 0), EMPTY_FILTER}},
+	{"member_deny_filter",
+	 {DEFINE_FILTER(SW_INCR, 1), DEFINE_FILTER(INST_RETIRED, 1),
+	  DEFINE_FILTER(BR_RETIRED, 1), EMPTY_FILTER}},
+	{"not_member_deny_filter",
+	 {DEFINE_FILTER(SW_INCR, 1), EMPTY_FILTER}},
+	{"not_member_allow_filter",
+	 {DEFINE_FILTER(SW_INCR, 0), EMPTY_FILTER}},
+	{ 0 }
+};
+
+static void for_each_test(void)
+{
+	struct test_desc *t;
+
+	for (t = &tests[0]; t->name; t++)
+		run_test(t);
+}
+
+static bool kvm_supports_pmu_event_filter(void)
+{
+	int r;
+
+	vpmu_vm = create_vpmu_vm(guest_code);
+
+	r = __kvm_has_device_attr(vpmu_vm->vcpu->fd, KVM_ARM_VCPU_PMU_V3_CTRL,
+				  KVM_ARM_VCPU_PMU_V3_FILTER);
+
+	destroy_vpmu_vm(vpmu_vm);
+	return !r;
+}
+
+static bool host_pmu_supports_events(void)
+{
+	vpmu_vm = create_vpmu_vm(guest_get_pmceid);
+
+	memset(&supported_pmce, 0, sizeof(supported_pmce));
+	sync_global_to_guest(vpmu_vm->vm, supported_pmce);
+	run_vcpu(vpmu_vm->vcpu);
+	sync_global_from_guest(vpmu_vm->vm, supported_pmce);
+	destroy_vpmu_vm(vpmu_vm);
+
+	return supported_pmce.pmceid0 & (BR_RETIRED | INST_RETIRED);
+}
+
+int main(void)
+{
+	TEST_REQUIRE(kvm_has_cap(KVM_CAP_ARM_PMU_V3));
+	TEST_REQUIRE(kvm_supports_pmu_event_filter());
+	TEST_REQUIRE(host_pmu_supports_events());
+
+	for_each_test();
+}
diff --git a/tools/testing/selftests/kvm/include/aarch64/vpmu.h b/tools/testing/selftests/kvm/include/aarch64/vpmu.h
index 92715021f892..ec37dbd0b1c0 100644
--- a/tools/testing/selftests/kvm/include/aarch64/vpmu.h
+++ b/tools/testing/selftests/kvm/include/aarch64/vpmu.h
@@ -18,6 +18,10 @@ struct vpmu_vm {
 	int gic_fd;
 };
 
+struct vpmu_vm *__create_vpmu_vm(void *guest_code,
+				 void (*init_pmu)(struct vpmu_vm *vm, void *arg),
+				 void *arg);
+
 struct vpmu_vm *create_vpmu_vm(void *guest_code);
 
 void destroy_vpmu_vm(struct vpmu_vm *vpmu_vm);
diff --git a/tools/testing/selftests/kvm/lib/aarch64/vpmu.c b/tools/testing/selftests/kvm/lib/aarch64/vpmu.c
index b3de8fdc555e..76ea03d607f1 100644
--- a/tools/testing/selftests/kvm/lib/aarch64/vpmu.c
+++ b/tools/testing/selftests/kvm/lib/aarch64/vpmu.c
@@ -7,8 +7,9 @@
 #include <vpmu.h>
 #include <perf/arm_pmuv3.h>
 
-/* Create a VM that has one vCPU with PMUv3 configured. */
-struct vpmu_vm *create_vpmu_vm(void *guest_code)
+struct vpmu_vm *__create_vpmu_vm(void *guest_code,
+				 void (*init_pmu)(struct vpmu_vm *vm, void *arg),
+				 void *arg)
 {
 	struct kvm_vcpu_init init;
 	uint8_t pmuver;
@@ -50,12 +51,21 @@ struct vpmu_vm *create_vpmu_vm(void *guest_code)
 		    "Unexpected PMUVER (0x%x) on the vCPU with PMUv3", pmuver);
 
 	/* Initialize vPMU */
+	if (init_pmu)
+		init_pmu(vpmu_vm, arg);
+
 	vcpu_ioctl(vpmu_vm->vcpu, KVM_SET_DEVICE_ATTR, &irq_attr);
 	vcpu_ioctl(vpmu_vm->vcpu, KVM_SET_DEVICE_ATTR, &init_attr);
 
 	return vpmu_vm;
 }
 
+/* Create a VM that has one vCPU with PMUv3 configured. */
+struct vpmu_vm *create_vpmu_vm(void *guest_code)
+{
+	return __create_vpmu_vm(guest_code, NULL, NULL);
+}
+
 void destroy_vpmu_vm(struct vpmu_vm *vpmu_vm)
 {
 	close(vpmu_vm->gic_fd);
-- 
2.40.1

[PATCH v4 5/5] KVM: selftests: aarch64: Add invalid filter test in pmu_event_filter_test

[Shaoqin Huang]

Add the invalid filter test includes sets the filter beyond the event
space and sets the invalid action to double check if the
KVM_ARM_VCPU_PMU_V3_FILTER will return the expected error.

Signed-off-by: Shaoqin Huang <shahuang@redhat.com>
---
 .../kvm/aarch64/pmu_event_filter_test.c       | 36 +++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/tools/testing/selftests/kvm/aarch64/pmu_event_filter_test.c b/tools/testing/selftests/kvm/aarch64/pmu_event_filter_test.c
index d280382f362f..68e1f2003312 100644
--- a/tools/testing/selftests/kvm/aarch64/pmu_event_filter_test.c
+++ b/tools/testing/selftests/kvm/aarch64/pmu_event_filter_test.c
@@ -7,6 +7,7 @@
  * 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.
+ * It also checks that setting invalid filter ranges return the expected error.
  * This test runs only when KVM_CAP_ARM_PMU_V3, KVM_ARM_VCPU_PMU_V3_FILTER
  * is supported on the host.
  */
@@ -183,6 +184,39 @@ static void for_each_test(void)
 		run_test(t);
 }
 
+static void set_invalid_filter(struct vpmu_vm *vm, void *arg)
+{
+	struct kvm_pmu_event_filter invalid;
+	struct kvm_device_attr attr = {
+		.group	= KVM_ARM_VCPU_PMU_V3_CTRL,
+		.attr	= KVM_ARM_VCPU_PMU_V3_FILTER,
+		.addr	= (uint64_t)&invalid,
+	};
+	int ret = 0;
+
+	/* The max event number is (1 << 16), set a range largeer than it. */
+	invalid = __DEFINE_FILTER(BIT(15), BIT(15)+1, 0);
+	ret = __vcpu_ioctl(vm->vcpu, KVM_SET_DEVICE_ATTR, &attr);
+	TEST_ASSERT(ret && errno == EINVAL, "Set Invalid filter range "
+		    "ret = %d, errno = %d (expected ret = -1, errno = EINVAL)",
+		    ret, errno);
+
+	ret = 0;
+
+	/* Set the Invalid action. */
+	invalid = __DEFINE_FILTER(0, 1, 3);
+	ret = __vcpu_ioctl(vm->vcpu, KVM_SET_DEVICE_ATTR, &attr);
+	TEST_ASSERT(ret && errno == EINVAL, "Set Invalid filter action "
+		    "ret = %d, errno = %d (expected ret = -1, errno = EINVAL)",
+		    ret, errno);
+}
+
+static void test_invalid_filter(void)
+{
+	vpmu_vm = __create_vpmu_vm(guest_code, set_invalid_filter, NULL);
+	destroy_vpmu_vm(vpmu_vm);
+}
+
 static bool kvm_supports_pmu_event_filter(void)
 {
 	int r;
@@ -216,4 +250,6 @@ int main(void)
 	TEST_REQUIRE(host_pmu_supports_events());
 
 	for_each_test();
+
+	test_invalid_filter();
 }
-- 
2.40.1

Re: [PATCH v4 1/5] KVM: selftests: aarch64: Make the [create|destroy]_vpmu_vm() public

[Oliver Upton]

On Thu, Feb 01, 2024 at 09:56:50PM -0500, Shaoqin Huang wrote:

[...]

> diff --git a/tools/testing/selftests/kvm/include/aarch64/vpmu.h b/tools/testing/selftests/kvm/include/aarch64/vpmu.h
> new file mode 100644
> index 000000000000..0a56183644ee
> --- /dev/null
> +++ b/tools/testing/selftests/kvm/include/aarch64/vpmu.h
> @@ -0,0 +1,16 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#include <kvm_util.h>
> +
> +#define GICD_BASE_GPA	0x8000000ULL
> +#define GICR_BASE_GPA	0x80A0000ULL

Shouldn't a standardized layout of the GIC frames go with the rest of
the GIC stuff?

> +/* Create a VM that has one vCPU with PMUv3 configured. */
> +struct vpmu_vm *create_vpmu_vm(void *guest_code)
> +{
> +	struct kvm_vcpu_init init;
> +	uint8_t pmuver;
> +	uint64_t dfr0, irq = 23;
> +	struct kvm_device_attr irq_attr = {
> +		.group = KVM_ARM_VCPU_PMU_V3_CTRL,
> +		.attr = KVM_ARM_VCPU_PMU_V3_IRQ,
> +		.addr = (uint64_t)&irq,
> +	};
> +	struct kvm_device_attr init_attr = {
> +		.group = KVM_ARM_VCPU_PMU_V3_CTRL,
> +		.attr = KVM_ARM_VCPU_PMU_V3_INIT,
> +	};
> +	struct vpmu_vm *vpmu_vm;
> +
> +	vpmu_vm = calloc(1, sizeof(*vpmu_vm));
> +	TEST_ASSERT(vpmu_vm != NULL, "Insufficient Memory");

!vpmu_vm would be the normal way to test if a pointer is NULL.

> +	memset(vpmu_vm, 0, sizeof(vpmu_vm));

What? man calloc would tell you that the returned object is already
zero-initalized.

> +	vpmu_vm->vm = vm_create(1);
> +	vm_init_descriptor_tables(vpmu_vm->vm);
> +
> +	/* Create vCPU with PMUv3 */
> +	vm_ioctl(vpmu_vm->vm, KVM_ARM_PREFERRED_TARGET, &init);
> +	init.features[0] |= (1 << KVM_ARM_VCPU_PMU_V3);
> +	vpmu_vm->vcpu = aarch64_vcpu_add(vpmu_vm->vm, 0, &init, guest_code);
> +	vcpu_init_descriptor_tables(vpmu_vm->vcpu);

I extremely dislike that the VM is semi-configured by this helper.
You're still expecting the caller to actually install the exception
handler.

> +	vpmu_vm->gic_fd = vgic_v3_setup(vpmu_vm->vm, 1, 64,
> +					GICD_BASE_GPA, GICR_BASE_GPA);
> +	__TEST_REQUIRE(vpmu_vm->gic_fd >= 0,
> +		       "Failed to create vgic-v3, skipping");
> +
> +	/* Make sure that PMUv3 support is indicated in the ID register */
> +	vcpu_get_reg(vpmu_vm->vcpu,
> +		     KVM_ARM64_SYS_REG(SYS_ID_AA64DFR0_EL1), &dfr0);
> +	pmuver = FIELD_GET(ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_PMUVer), dfr0);
> +	TEST_ASSERT(pmuver != ID_AA64DFR0_EL1_PMUVer_IMP_DEF &&
> +		    pmuver >= ID_AA64DFR0_EL1_PMUVer_IMP,
> +		    "Unexpected PMUVER (0x%x) on the vCPU with PMUv3", pmuver);

Not your code, but this assertion is meaningless. KVM does not advertise
an IMP_DEF PMU to guests.

> +	/* Initialize vPMU */
> +	vcpu_ioctl(vpmu_vm->vcpu, KVM_SET_DEVICE_ATTR, &irq_attr);
> +	vcpu_ioctl(vpmu_vm->vcpu, KVM_SET_DEVICE_ATTR, &init_attr);

Not your code, but these should be converted to kvm_device_attr_set()
calls.

Overall I'm somewhat tepid on the idea of the library being so
coarse-grained. It is usually more helpful to expose finer-grained
controls, like a helper that initializes the vPMU state for a
preexisting VM. That way the PMU code can more easily be composed with
other helpers in different tests.

-- 
Thanks,
Oliver

Re: [PATCH v4 2/5] KVM: selftests: aarch64: Move pmu helper functions into vpmu.h

[Oliver Upton]

On Thu, Feb 01, 2024 at 09:56:51PM -0500, Shaoqin Huang wrote:
> -static uint64_t get_pmcr_n(uint64_t pmcr)
> -{
> -	return FIELD_GET(ARMV8_PMU_PMCR_N, pmcr);
> -}
> -
> -static void set_pmcr_n(uint64_t *pmcr, uint64_t pmcr_n)
> -{
> -	u64p_replace_bits((__u64 *) pmcr, pmcr_n, ARMV8_PMU_PMCR_N);
> -}
> -
> -static uint64_t get_counters_mask(uint64_t n)
> -{
> -	uint64_t mask = BIT(ARMV8_PMU_CYCLE_IDX);
> -
> -	if (n)
> -		mask |= GENMASK(n - 1, 0);
> -	return mask;
> -}

I don't see these helpers being used by your test, and they seem rather
specific to what the original test was trying to accomplish. Let's not
move this unnecessarily.

-- 
Thanks,
Oliver

Re: [PATCH v4 4/5] KVM: selftests: aarch64: Introduce pmu_event_filter_test

[Oliver Upton]

On Thu, Feb 01, 2024 at 09:56:53PM -0500, Shaoqin Huang wrote:
> 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       | 219 ++++++++++++++++++
>  .../selftests/kvm/include/aarch64/vpmu.h      |   4 +
>  .../testing/selftests/kvm/lib/aarch64/vpmu.c  |  14 +-
>  4 files changed, 236 insertions(+), 2 deletions(-)
>  create mode 100644 tools/testing/selftests/kvm/aarch64/pmu_event_filter_test.c
> 
> diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
> index 709a70b31ca2..733ec86a3385 100644
> --- a/tools/testing/selftests/kvm/Makefile
> +++ b/tools/testing/selftests/kvm/Makefile
> @@ -148,6 +148,7 @@ TEST_GEN_PROGS_aarch64 += aarch64/arch_timer
>  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..d280382f362f
> --- /dev/null
> +++ b/tools/testing/selftests/kvm/aarch64/pmu_event_filter_test.c
> @@ -0,0 +1,219 @@
> +// 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 pmce{

Missing whitespace before curly brace.

Also -- pmce is an odd name. Maybe common_event_ids or pmu_id_regs.

> +	uint64_t pmceid0;
> +	uint64_t pmceid1;
> +} supported_pmce, guest_pmce;

maybe "max_*" and "expected_*". It took me a bit to understand that
guest_pmce feeds in your expected PMCEID values.

> +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)
> +
> +#define EMPTY_FILTER	{ 0 }
> +
> +#define SW_INCR		0x0
> +#define INST_RETIRED	0x8
> +#define BR_RETIRED	0x21

These event numbers are already defined in tools/include/perf/arm_pmuv3.h,
use those instead.

> +static void guest_code(void)
> +{
> +	uint64_t pmceid0 = read_sysreg(pmceid0_el0);
> +	uint64_t pmceid1 = read_sysreg(pmceid1_el0);
> +
> +	GUEST_ASSERT_EQ(guest_pmce.pmceid0, pmceid0);
> +	GUEST_ASSERT_EQ(guest_pmce.pmceid1, pmceid1);
> +
> +	GUEST_DONE();
> +}
> +
> +static void guest_get_pmceid(void)
> +{
> +	supported_pmce.pmceid0 = read_sysreg(pmceid0_el0);
> +	supported_pmce.pmceid1 = read_sysreg(pmceid1_el0);
> +
> +	GUEST_DONE();
> +}
> +
> +static void pmu_event_filter_init(struct vpmu_vm *vm, void *arg)

Why are you obfuscating the pointer to the filter array?

> +{
> +	struct kvm_device_attr attr = {
> +		.group	= KVM_ARM_VCPU_PMU_V3_CTRL,
> +		.attr	= KVM_ARM_VCPU_PMU_V3_FILTER,
> +	};
> +	struct kvm_pmu_event_filter *filter = (struct kvm_pmu_event_filter *)arg;
> +
> +	while (filter && filter->nevents != 0) {
> +		attr.addr = (uint64_t)filter;
> +		vcpu_ioctl(vm->vcpu, KVM_SET_DEVICE_ATTR, &attr);

Again, kvm_device_attr_set() the right helper to use.

> +static void set_pmce(struct pmce *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_guest_pmce(struct kvm_pmu_event_filter *filter)
> +{
> +	struct pmce pmce_mask = { ~0, ~0 };
> +	bool first_filter = true;
> +
> +	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;
> +		}
> +
> +		set_pmce(&pmce_mask, filter->action, filter->base_event);
> +		filter++;
> +	}
> +
> +	guest_pmce.pmceid0 = supported_pmce.pmceid0 & pmce_mask.pmceid0;
> +	guest_pmce.pmceid1 = supported_pmce.pmceid1 & pmce_mask.pmceid1;
> +}

Why do you need to do this? Can't you tell the guests what events to
expect and have it make sense of the PMCEID values it sees?

You could, for example, pass in a pointer to the test descriptor as an
argument.

> +static void run_test(struct test_desc *t)
> +{
> +	pr_debug("Test: %s\n", t->name);

You may as well just pr_info() this thing.

> +	create_vpmu_vm_with_filter(guest_code, t->filter);
> +	prepare_guest_pmce(t->filter);
> +	sync_global_to_guest(vpmu_vm->vm, guest_pmce);
> +
> +	run_vcpu(vpmu_vm->vcpu);
> +
> +	destroy_vpmu_vm(vpmu_vm);
> +}
> +
> +static struct test_desc tests[] = {
> +	{"without_filter", { EMPTY_FILTER }},
> +	{"member_allow_filter",
> +	 {DEFINE_FILTER(SW_INCR, 0), DEFINE_FILTER(INST_RETIRED, 0),
> +	  DEFINE_FILTER(BR_RETIRED, 0), EMPTY_FILTER}},
> +	{"member_deny_filter",
> +	 {DEFINE_FILTER(SW_INCR, 1), DEFINE_FILTER(INST_RETIRED, 1),
> +	  DEFINE_FILTER(BR_RETIRED, 1), EMPTY_FILTER}},
> +	{"not_member_deny_filter",
> +	 {DEFINE_FILTER(SW_INCR, 1), EMPTY_FILTER}},
> +	{"not_member_allow_filter",
> +	 {DEFINE_FILTER(SW_INCR, 0), EMPTY_FILTER}},

Why is the filter array special enough to get its own sentinel macro
but...

> +	{ 0 }

... the test descriptor array is okay to use a 'raw' initialization. My
vote is to drop the macro, zero-initializing a struct in an array is an
extremely common pattern in the kernel.

Also, these descriptors are dense and hard to read. Working with an
example:

	{
		.name = "member_allow_filter",
		.filter = {
			DEFINE_FILTER(SW_INCR, 0),
			DEFINE_FILTER(INST_RETIRED, 0),
			DEFINE_FILTER(BR_RETIRED, 0),
			{ 0 }
		},
	}

See how much more readable that is?

> +};
> +
> +static void for_each_test(void)
> +{
> +	struct test_desc *t;
> +
> +	for (t = &tests[0]; t->name; t++)
> +		run_test(t);
> +}

for_each_test() sounds like an iterator, but this is not. Call it
run_tests()

> +static bool kvm_supports_pmu_event_filter(void)
> +{
> +	int r;
> +
> +	vpmu_vm = create_vpmu_vm(guest_code);
> +
> +	r = __kvm_has_device_attr(vpmu_vm->vcpu->fd, KVM_ARM_VCPU_PMU_V3_CTRL,
> +				  KVM_ARM_VCPU_PMU_V3_FILTER);
> +
> +	destroy_vpmu_vm(vpmu_vm);
> +	return !r;
> +}

TBH, I don't really care much about the test probing for the event
filter UAPI. It has been upstream for a while, and if folks are trying
to run selftests at HEAD on an old kernel then that's their business.

The other prerequisites make more sense since they actually check if HW
features are present.

> +static bool host_pmu_supports_events(void)
> +{
> +	vpmu_vm = create_vpmu_vm(guest_get_pmceid);
> +
> +	memset(&supported_pmce, 0, sizeof(supported_pmce));
> +	sync_global_to_guest(vpmu_vm->vm, supported_pmce);
> +	run_vcpu(vpmu_vm->vcpu);
> +	sync_global_from_guest(vpmu_vm->vm, supported_pmce);
> +	destroy_vpmu_vm(vpmu_vm);
> +
> +	return supported_pmce.pmceid0 & (BR_RETIRED | INST_RETIRED);
> +}

This helper says its probing the host PMU, but you're actually firing up a
VM to do it.

The events supported by a particular PMU instance are readily available
in sysfs. Furthermore, you can tell KVM to select the exact host PMU
instance you probe.

> diff --git a/tools/testing/selftests/kvm/lib/aarch64/vpmu.c b/tools/testing/selftests/kvm/lib/aarch64/vpmu.c
> index b3de8fdc555e..76ea03d607f1 100644
> --- a/tools/testing/selftests/kvm/lib/aarch64/vpmu.c
> +++ b/tools/testing/selftests/kvm/lib/aarch64/vpmu.c
> @@ -7,8 +7,9 @@
>  #include <vpmu.h>
>  #include <perf/arm_pmuv3.h>
>  
> -/* Create a VM that has one vCPU with PMUv3 configured. */
> -struct vpmu_vm *create_vpmu_vm(void *guest_code)
> +struct vpmu_vm *__create_vpmu_vm(void *guest_code,
> +				 void (*init_pmu)(struct vpmu_vm *vm, void *arg),
> +				 void *arg)
>  {
>  	struct kvm_vcpu_init init;
>  	uint8_t pmuver;
> @@ -50,12 +51,21 @@ struct vpmu_vm *create_vpmu_vm(void *guest_code)
>  		    "Unexpected PMUVER (0x%x) on the vCPU with PMUv3", pmuver);
>  
>  	/* Initialize vPMU */
> +	if (init_pmu)
> +		init_pmu(vpmu_vm, arg);
> +
>  	vcpu_ioctl(vpmu_vm->vcpu, KVM_SET_DEVICE_ATTR, &irq_attr);
>  	vcpu_ioctl(vpmu_vm->vcpu, KVM_SET_DEVICE_ATTR, &init_attr);
>  
>  	return vpmu_vm;
>  }
>  
> +/* Create a VM that has one vCPU with PMUv3 configured. */
> +struct vpmu_vm *create_vpmu_vm(void *guest_code)
> +{
> +	return __create_vpmu_vm(guest_code, NULL, NULL);
> +}
> +

Ok. This completely proves my point in the other patch. You already need
to refactor this helper to cram in what you're trying to do. Think of
ways to move the code that is actually common into libraries and leave
the rest to the tests themselves.

Some slight code duplication isn't the end of the world if it avoids
churning libraries every time someone wants to add a widget.

-- 
Thanks,
Oliver

Re: [PATCH v4 4/5] KVM: selftests: aarch64: Introduce pmu_event_filter_test

[Eric Auger]

Hi Shaoqin,

On 2/2/24 09:34, Oliver Upton wrote:
> On Thu, Feb 01, 2024 at 09:56:53PM -0500, Shaoqin Huang wrote:
>> 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       | 219 ++++++++++++++++++
>>  .../selftests/kvm/include/aarch64/vpmu.h      |   4 +
>>  .../testing/selftests/kvm/lib/aarch64/vpmu.c  |  14 +-
>>  4 files changed, 236 insertions(+), 2 deletions(-)
>>  create mode 100644 tools/testing/selftests/kvm/aarch64/pmu_event_filter_test.c
>>
>> diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
>> index 709a70b31ca2..733ec86a3385 100644
>> --- a/tools/testing/selftests/kvm/Makefile
>> +++ b/tools/testing/selftests/kvm/Makefile
>> @@ -148,6 +148,7 @@ TEST_GEN_PROGS_aarch64 += aarch64/arch_timer
>>  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..d280382f362f
>> --- /dev/null
>> +++ b/tools/testing/selftests/kvm/aarch64/pmu_event_filter_test.c
>> @@ -0,0 +1,219 @@
>> +// 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 pmce{
> 
> Missing whitespace before curly brace.
> 
> Also -- pmce is an odd name. Maybe common_event_ids or pmu_id_regs.
> 
>> +	uint64_t pmceid0;
>> +	uint64_t pmceid1;
>> +} supported_pmce, guest_pmce;
> 
> maybe "max_*" and "expected_*". It took me a bit to understand that
> guest_pmce feeds in your expected PMCEID values.
> 
>> +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)
>> +
>> +#define EMPTY_FILTER	{ 0 }
>> +
>> +#define SW_INCR		0x0
>> +#define INST_RETIRED	0x8
>> +#define BR_RETIRED	0x21
> 
> These event numbers are already defined in tools/include/perf/arm_pmuv3.h,
> use those instead.
> 
>> +static void guest_code(void)
>> +{
>> +	uint64_t pmceid0 = read_sysreg(pmceid0_el0);
>> +	uint64_t pmceid1 = read_sysreg(pmceid1_el0);
>> +
>> +	GUEST_ASSERT_EQ(guest_pmce.pmceid0, pmceid0);
>> +	GUEST_ASSERT_EQ(guest_pmce.pmceid1, pmceid1);
>> +
>> +	GUEST_DONE();
>> +}
>> +
>> +static void guest_get_pmceid(void)
>> +{
>> +	supported_pmce.pmceid0 = read_sysreg(pmceid0_el0);
>> +	supported_pmce.pmceid1 = read_sysreg(pmceid1_el0);
>> +
>> +	GUEST_DONE();
>> +}
>> +
>> +static void pmu_event_filter_init(struct vpmu_vm *vm, void *arg)
> 
> Why are you obfuscating the pointer to the filter array?
> 
>> +{
>> +	struct kvm_device_attr attr = {
>> +		.group	= KVM_ARM_VCPU_PMU_V3_CTRL,
>> +		.attr	= KVM_ARM_VCPU_PMU_V3_FILTER,
>> +	};
>> +	struct kvm_pmu_event_filter *filter = (struct kvm_pmu_event_filter *)arg;
>> +
>> +	while (filter && filter->nevents != 0) {
>> +		attr.addr = (uint64_t)filter;
>> +		vcpu_ioctl(vm->vcpu, KVM_SET_DEVICE_ATTR, &attr);
> 
> Again, kvm_device_attr_set() the right helper to use.
> 
>> +static void set_pmce(struct pmce *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_guest_pmce(struct kvm_pmu_event_filter *filter)
>> +{
>> +	struct pmce pmce_mask = { ~0, ~0 };
>> +	bool first_filter = true;
>> +
>> +	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;
>> +		}
>> +
>> +		set_pmce(&pmce_mask, filter->action, filter->base_event);
>> +		filter++;
>> +	}
>> +
>> +	guest_pmce.pmceid0 = supported_pmce.pmceid0 & pmce_mask.pmceid0;
>> +	guest_pmce.pmceid1 = supported_pmce.pmceid1 & pmce_mask.pmceid1;
>> +}
> 
> Why do you need to do this? Can't you tell the guests what events to
> expect and have it make sense of the PMCEID values it sees?
> 
> You could, for example, pass in a pointer to the test descriptor as an
> argument.
> 
>> +static void run_test(struct test_desc *t)
>> +{
>> +	pr_debug("Test: %s\n", t->name);
> 
> You may as well just pr_info() this thing.
> 
>> +	create_vpmu_vm_with_filter(guest_code, t->filter);
>> +	prepare_guest_pmce(t->filter);
>> +	sync_global_to_guest(vpmu_vm->vm, guest_pmce);
>> +
>> +	run_vcpu(vpmu_vm->vcpu);
>> +
>> +	destroy_vpmu_vm(vpmu_vm);
>> +}
>> +
>> +static struct test_desc tests[] = {
>> +	{"without_filter", { EMPTY_FILTER }},
>> +	{"member_allow_filter",
>> +	 {DEFINE_FILTER(SW_INCR, 0), DEFINE_FILTER(INST_RETIRED, 0),
>> +	  DEFINE_FILTER(BR_RETIRED, 0), EMPTY_FILTER}},
>> +	{"member_deny_filter",
>> +	 {DEFINE_FILTER(SW_INCR, 1), DEFINE_FILTER(INST_RETIRED, 1),
>> +	  DEFINE_FILTER(BR_RETIRED, 1), EMPTY_FILTER}},
>> +	{"not_member_deny_filter",
>> +	 {DEFINE_FILTER(SW_INCR, 1), EMPTY_FILTER}},
>> +	{"not_member_allow_filter",
>> +	 {DEFINE_FILTER(SW_INCR, 0), EMPTY_FILTER}},
from a strict uapi testing you are not testing
- "Cancelling" a filter by registering the opposite action for the same
range doesn't change the default action.
- Event 0 (SW_INCR)
- Filtering event 0x1E (CHAIN) has no effect either
- Filtering the cycle counter is possible using event 0x11 (CPU_CYCLES).

Documentation/virt/kvm/devices/vcpu.rst

Then it obviously depends on how much coverage of the API you want/can
afford to reach.

Eric

> 
> Why is the filter array special enough to get its own sentinel macro
> but...
> 
>> +	{ 0 }
> 
> ... the test descriptor array is okay to use a 'raw' initialization. My
> vote is to drop the macro, zero-initializing a struct in an array is an
> extremely common pattern in the kernel.
> 
> Also, these descriptors are dense and hard to read. Working with an
> example:
> 
> 	{
> 		.name = "member_allow_filter",
> 		.filter = {
> 			DEFINE_FILTER(SW_INCR, 0),
> 			DEFINE_FILTER(INST_RETIRED, 0),
> 			DEFINE_FILTER(BR_RETIRED, 0),
> 			{ 0 }
> 		},
> 	}
> 
> See how much more readable that is?
> 
>> +};
>> +
>> +static void for_each_test(void)
>> +{
>> +	struct test_desc *t;
>> +
>> +	for (t = &tests[0]; t->name; t++)
>> +		run_test(t);
>> +}
> 
> for_each_test() sounds like an iterator, but this is not. Call it
> run_tests()
> 
>> +static bool kvm_supports_pmu_event_filter(void)
>> +{
>> +	int r;
>> +
>> +	vpmu_vm = create_vpmu_vm(guest_code);
>> +
>> +	r = __kvm_has_device_attr(vpmu_vm->vcpu->fd, KVM_ARM_VCPU_PMU_V3_CTRL,
>> +				  KVM_ARM_VCPU_PMU_V3_FILTER);
>> +
>> +	destroy_vpmu_vm(vpmu_vm);
>> +	return !r;
>> +}
> 
> TBH, I don't really care much about the test probing for the event
> filter UAPI. It has been upstream for a while, and if folks are trying
> to run selftests at HEAD on an old kernel then that's their business.
> 
> The other prerequisites make more sense since they actually check if HW
> features are present.
> 
>> +static bool host_pmu_supports_events(void)
>> +{
>> +	vpmu_vm = create_vpmu_vm(guest_get_pmceid);
>> +
>> +	memset(&supported_pmce, 0, sizeof(supported_pmce));
>> +	sync_global_to_guest(vpmu_vm->vm, supported_pmce);
>> +	run_vcpu(vpmu_vm->vcpu);
>> +	sync_global_from_guest(vpmu_vm->vm, supported_pmce);
>> +	destroy_vpmu_vm(vpmu_vm);
>> +
>> +	return supported_pmce.pmceid0 & (BR_RETIRED | INST_RETIRED);
>> +}
> 
> This helper says its probing the host PMU, but you're actually firing up a
> VM to do it.
> 
> The events supported by a particular PMU instance are readily available
> in sysfs. Furthermore, you can tell KVM to select the exact host PMU
> instance you probe.
> 
>> diff --git a/tools/testing/selftests/kvm/lib/aarch64/vpmu.c b/tools/testing/selftests/kvm/lib/aarch64/vpmu.c
>> index b3de8fdc555e..76ea03d607f1 100644
>> --- a/tools/testing/selftests/kvm/lib/aarch64/vpmu.c
>> +++ b/tools/testing/selftests/kvm/lib/aarch64/vpmu.c
>> @@ -7,8 +7,9 @@
>>  #include <vpmu.h>
>>  #include <perf/arm_pmuv3.h>
>>  
>> -/* Create a VM that has one vCPU with PMUv3 configured. */
>> -struct vpmu_vm *create_vpmu_vm(void *guest_code)
>> +struct vpmu_vm *__create_vpmu_vm(void *guest_code,
>> +				 void (*init_pmu)(struct vpmu_vm *vm, void *arg),
>> +				 void *arg)
>>  {
>>  	struct kvm_vcpu_init init;
>>  	uint8_t pmuver;
>> @@ -50,12 +51,21 @@ struct vpmu_vm *create_vpmu_vm(void *guest_code)
>>  		    "Unexpected PMUVER (0x%x) on the vCPU with PMUv3", pmuver);
>>  
>>  	/* Initialize vPMU */
>> +	if (init_pmu)
>> +		init_pmu(vpmu_vm, arg);
>> +
>>  	vcpu_ioctl(vpmu_vm->vcpu, KVM_SET_DEVICE_ATTR, &irq_attr);
>>  	vcpu_ioctl(vpmu_vm->vcpu, KVM_SET_DEVICE_ATTR, &init_attr);
>>  
>>  	return vpmu_vm;
>>  }
>>  
>> +/* Create a VM that has one vCPU with PMUv3 configured. */
>> +struct vpmu_vm *create_vpmu_vm(void *guest_code)
>> +{
>> +	return __create_vpmu_vm(guest_code, NULL, NULL);
>> +}
>> +
> 
> Ok. This completely proves my point in the other patch. You already need
> to refactor this helper to cram in what you're trying to do. Think of
> ways to move the code that is actually common into libraries and leave
> the rest to the tests themselves.
> 
> Some slight code duplication isn't the end of the world if it avoids
> churning libraries every time someone wants to add a widget.
> 

Eric

Re: [PATCH v4 5/5] KVM: selftests: aarch64: Add invalid filter test in pmu_event_filter_test

[Eric Auger]

Hi Shaoqin,

On 2/2/24 03:56, Shaoqin Huang wrote:
> Add the invalid filter test includes sets the filter beyond the event
s/includes/which
> space and sets the invalid action to double check if the
> KVM_ARM_VCPU_PMU_V3_FILTER will return the expected error.
> 
> Signed-off-by: Shaoqin Huang <shahuang@redhat.com>
> ---
>  .../kvm/aarch64/pmu_event_filter_test.c       | 36 +++++++++++++++++++
>  1 file changed, 36 insertions(+)
> 
> diff --git a/tools/testing/selftests/kvm/aarch64/pmu_event_filter_test.c b/tools/testing/selftests/kvm/aarch64/pmu_event_filter_test.c
> index d280382f362f..68e1f2003312 100644
> --- a/tools/testing/selftests/kvm/aarch64/pmu_event_filter_test.c
> +++ b/tools/testing/selftests/kvm/aarch64/pmu_event_filter_test.c
> @@ -7,6 +7,7 @@
>   * 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.
> + * It also checks that setting invalid filter ranges return the expected error.
>   * This test runs only when KVM_CAP_ARM_PMU_V3, KVM_ARM_VCPU_PMU_V3_FILTER
>   * is supported on the host.
>   */
> @@ -183,6 +184,39 @@ static void for_each_test(void)
>  		run_test(t);
>  }
>  
> +static void set_invalid_filter(struct vpmu_vm *vm, void *arg)
> +{
> +	struct kvm_pmu_event_filter invalid;
> +	struct kvm_device_attr attr = {
> +		.group	= KVM_ARM_VCPU_PMU_V3_CTRL,
> +		.attr	= KVM_ARM_VCPU_PMU_V3_FILTER,
> +		.addr	= (uint64_t)&invalid,
> +	};
> +	int ret = 0;
> +
> +	/* The max event number is (1 << 16), set a range largeer than it. */
in  practice it is 16b on ARMv8.1 and 10b on ARMv8.0 but obvioulsy the
check below works for both ;-)

larger typ
> +	invalid = __DEFINE_FILTER(BIT(15), BIT(15)+1, 0);
space between "+"
> +	ret = __vcpu_ioctl(vm->vcpu, KVM_SET_DEVICE_ATTR, &attr);
kvm_device_attr_set() as commented by Oliver
> +	TEST_ASSERT(ret && errno == EINVAL, "Set Invalid filter range "
> +		    "ret = %d, errno = %d (expected ret = -1, errno = EINVAL)",
> +		    ret, errno);
> +
> +	ret = 0;
> +
> +	/* Set the Invalid action. */
> +	invalid = __DEFINE_FILTER(0, 1, 3);
> +	ret = __vcpu_ioctl(vm->vcpu, KVM_SET_DEVICE_ATTR, &attr);
> +	TEST_ASSERT(ret && errno == EINVAL, "Set Invalid filter action "
> +		    "ret = %d, errno = %d (expected ret = -1, errno = EINVAL)",
> +		    ret, errno);
> +}
> +
> +static void test_invalid_filter(void)
> +{
> +	vpmu_vm = __create_vpmu_vm(guest_code, set_invalid_filter, NULL);
> +	destroy_vpmu_vm(vpmu_vm);
> +}
> +
>  static bool kvm_supports_pmu_event_filter(void)
>  {
>  	int r;
> @@ -216,4 +250,6 @@ int main(void)
>  	TEST_REQUIRE(host_pmu_supports_events());
>  
>  	for_each_test();
> +
> +	test_invalid_filter();
>  }
Thanks

Eric

Re: [PATCH v4 1/5] KVM: selftests: aarch64: Make the [create|destroy]_vpmu_vm() public

[Shaoqin Huang]

Hi Oliver,

On 2/2/24 15:36, Oliver Upton wrote:
> On Thu, Feb 01, 2024 at 09:56:50PM -0500, Shaoqin Huang wrote:
> 
> [...]
> 
>> diff --git a/tools/testing/selftests/kvm/include/aarch64/vpmu.h b/tools/testing/selftests/kvm/include/aarch64/vpmu.h
>> new file mode 100644
>> index 000000000000..0a56183644ee
>> --- /dev/null
>> +++ b/tools/testing/selftests/kvm/include/aarch64/vpmu.h
>> @@ -0,0 +1,16 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +
>> +#include <kvm_util.h>
>> +
>> +#define GICD_BASE_GPA	0x8000000ULL
>> +#define GICR_BASE_GPA	0x80A0000ULL
> 
> Shouldn't a standardized layout of the GIC frames go with the rest of
> the GIC stuff?
> 
>> +/* Create a VM that has one vCPU with PMUv3 configured. */
>> +struct vpmu_vm *create_vpmu_vm(void *guest_code)
>> +{
>> +	struct kvm_vcpu_init init;
>> +	uint8_t pmuver;
>> +	uint64_t dfr0, irq = 23;
>> +	struct kvm_device_attr irq_attr = {
>> +		.group = KVM_ARM_VCPU_PMU_V3_CTRL,
>> +		.attr = KVM_ARM_VCPU_PMU_V3_IRQ,
>> +		.addr = (uint64_t)&irq,
>> +	};
>> +	struct kvm_device_attr init_attr = {
>> +		.group = KVM_ARM_VCPU_PMU_V3_CTRL,
>> +		.attr = KVM_ARM_VCPU_PMU_V3_INIT,
>> +	};
>> +	struct vpmu_vm *vpmu_vm;
>> +
>> +	vpmu_vm = calloc(1, sizeof(*vpmu_vm));
>> +	TEST_ASSERT(vpmu_vm != NULL, "Insufficient Memory");
> 
> !vpmu_vm would be the normal way to test if a pointer is NULL.
> 
>> +	memset(vpmu_vm, 0, sizeof(vpmu_vm));
> 
> What? man calloc would tell you that the returned object is already
> zero-initalized.
> 
>> +	vpmu_vm->vm = vm_create(1);
>> +	vm_init_descriptor_tables(vpmu_vm->vm);
>> +
>> +	/* Create vCPU with PMUv3 */
>> +	vm_ioctl(vpmu_vm->vm, KVM_ARM_PREFERRED_TARGET, &init);
>> +	init.features[0] |= (1 << KVM_ARM_VCPU_PMU_V3);
>> +	vpmu_vm->vcpu = aarch64_vcpu_add(vpmu_vm->vm, 0, &init, guest_code);
>> +	vcpu_init_descriptor_tables(vpmu_vm->vcpu);
> 
> I extremely dislike that the VM is semi-configured by this helper.
> You're still expecting the caller to actually install the exception
> handler.
> 
>> +	vpmu_vm->gic_fd = vgic_v3_setup(vpmu_vm->vm, 1, 64,
>> +					GICD_BASE_GPA, GICR_BASE_GPA);
>> +	__TEST_REQUIRE(vpmu_vm->gic_fd >= 0,
>> +		       "Failed to create vgic-v3, skipping");
>> +
>> +	/* Make sure that PMUv3 support is indicated in the ID register */
>> +	vcpu_get_reg(vpmu_vm->vcpu,
>> +		     KVM_ARM64_SYS_REG(SYS_ID_AA64DFR0_EL1), &dfr0);
>> +	pmuver = FIELD_GET(ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_PMUVer), dfr0);
>> +	TEST_ASSERT(pmuver != ID_AA64DFR0_EL1_PMUVer_IMP_DEF &&
>> +		    pmuver >= ID_AA64DFR0_EL1_PMUVer_IMP,
>> +		    "Unexpected PMUVER (0x%x) on the vCPU with PMUv3", pmuver);
> 
> Not your code, but this assertion is meaningless. KVM does not advertise
> an IMP_DEF PMU to guests.
> 
>> +	/* Initialize vPMU */
>> +	vcpu_ioctl(vpmu_vm->vcpu, KVM_SET_DEVICE_ATTR, &irq_attr);
>> +	vcpu_ioctl(vpmu_vm->vcpu, KVM_SET_DEVICE_ATTR, &init_attr);
> 
> Not your code, but these should be converted to kvm_device_attr_set()
> calls.
> 
> Overall I'm somewhat tepid on the idea of the library being so
> coarse-grained. It is usually more helpful to expose finer-grained
> controls, like a helper that initializes the vPMU state for a
> preexisting VM. That way the PMU code can more easily be composed with
> other helpers in different tests.

Thanks for your effort reviewing my code. You're right, the helper is 
too coarse-grained. I'm trying to refactor it and define some 
finer-grained helper which can be reused for futher vpmu tests.

Thanks,
Shaoqin

> 

-- 
Shaoqin

Re: [PATCH v4 4/5] KVM: selftests: aarch64: Introduce pmu_event_filter_test

[Shaoqin Huang]

Hi Oliver,

On 2/2/24 16:34, Oliver Upton wrote:
> On Thu, Feb 01, 2024 at 09:56:53PM -0500, Shaoqin Huang wrote:
>> 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       | 219 ++++++++++++++++++
>>   .../selftests/kvm/include/aarch64/vpmu.h      |   4 +
>>   .../testing/selftests/kvm/lib/aarch64/vpmu.c  |  14 +-
>>   4 files changed, 236 insertions(+), 2 deletions(-)
>>   create mode 100644 tools/testing/selftests/kvm/aarch64/pmu_event_filter_test.c
>>
>> diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
>> index 709a70b31ca2..733ec86a3385 100644
>> --- a/tools/testing/selftests/kvm/Makefile
>> +++ b/tools/testing/selftests/kvm/Makefile
>> @@ -148,6 +148,7 @@ TEST_GEN_PROGS_aarch64 += aarch64/arch_timer
>>   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..d280382f362f
>> --- /dev/null
>> +++ b/tools/testing/selftests/kvm/aarch64/pmu_event_filter_test.c
>> @@ -0,0 +1,219 @@
>> +// 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 pmce{
> 
> Missing whitespace before curly brace.
> 
> Also -- pmce is an odd name. Maybe common_event_ids or pmu_id_regs.

Thanks for pointing this out. I would choose pmu_common_event_ids as its 
name.

> 
>> +	uint64_t pmceid0;
>> +	uint64_t pmceid1;
>> +} supported_pmce, guest_pmce;
> 
> maybe "max_*" and "expected_*". It took me a bit to understand that
> guest_pmce feeds in your expected PMCEID values.
> 

The "max_* and "expected_*" is more clear, I would use it.

>> +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)
>> +
>> +#define EMPTY_FILTER	{ 0 }
>> +
>> +#define SW_INCR		0x0
>> +#define INST_RETIRED	0x8
>> +#define BR_RETIRED	0x21
> 
> These event numbers are already defined in tools/include/perf/arm_pmuv3.h,
> use those instead.
> 

Sure. I would use those defined macro.

>> +static void guest_code(void)
>> +{
>> +	uint64_t pmceid0 = read_sysreg(pmceid0_el0);
>> +	uint64_t pmceid1 = read_sysreg(pmceid1_el0);
>> +
>> +	GUEST_ASSERT_EQ(guest_pmce.pmceid0, pmceid0);
>> +	GUEST_ASSERT_EQ(guest_pmce.pmceid1, pmceid1);
>> +
>> +	GUEST_DONE();
>> +}
>> +
>> +static void guest_get_pmceid(void)
>> +{
>> +	supported_pmce.pmceid0 = read_sysreg(pmceid0_el0);
>> +	supported_pmce.pmceid1 = read_sysreg(pmceid1_el0);
>> +
>> +	GUEST_DONE();
>> +}
>> +
>> +static void pmu_event_filter_init(struct vpmu_vm *vm, void *arg)
> 
> Why are you obfuscating the pointer to the filter array?
> 
>> +{
>> +	struct kvm_device_attr attr = {
>> +		.group	= KVM_ARM_VCPU_PMU_V3_CTRL,
>> +		.attr	= KVM_ARM_VCPU_PMU_V3_FILTER,
>> +	};
>> +	struct kvm_pmu_event_filter *filter = (struct kvm_pmu_event_filter *)arg;
>> +
>> +	while (filter && filter->nevents != 0) {
>> +		attr.addr = (uint64_t)filter;
>> +		vcpu_ioctl(vm->vcpu, KVM_SET_DEVICE_ATTR, &attr);
> 
> Again, kvm_device_attr_set() the right helper to use.
> 
>> +static void set_pmce(struct pmce *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_guest_pmce(struct kvm_pmu_event_filter *filter)
>> +{
>> +	struct pmce pmce_mask = { ~0, ~0 };
>> +	bool first_filter = true;
>> +
>> +	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;
>> +		}
>> +
>> +		set_pmce(&pmce_mask, filter->action, filter->base_event);
>> +		filter++;
>> +	}
>> +
>> +	guest_pmce.pmceid0 = supported_pmce.pmceid0 & pmce_mask.pmceid0;
>> +	guest_pmce.pmceid1 = supported_pmce.pmceid1 & pmce_mask.pmceid1;
>> +}
> 
> Why do you need to do this? Can't you tell the guests what events to
> expect and have it make sense of the PMCEID values it sees?

At here, I prepare the pmceid value which the guest should see, and pass 
it to the guest by sync the global variable. And guest compare this 
value with the value it read through pmu common event register.

Why I don't put the process of generating expected_pmce into the guest 
code is that I want to make sure this value computed in host is totally 
correct, so the guest code is pretty simple, it only needs to compare 
the two value.

> 
> You could, for example, pass in a pointer to the test descriptor as an
> argument.
> 
>> +static void run_test(struct test_desc *t)
>> +{
>> +	pr_debug("Test: %s\n", t->name);
> 
> You may as well just pr_info() this thing.
> 

Ok. I can change it to pr_info().

>> +	create_vpmu_vm_with_filter(guest_code, t->filter);
>> +	prepare_guest_pmce(t->filter);
>> +	sync_global_to_guest(vpmu_vm->vm, guest_pmce);
>> +
>> +	run_vcpu(vpmu_vm->vcpu);
>> +
>> +	destroy_vpmu_vm(vpmu_vm);
>> +}
>> +
>> +static struct test_desc tests[] = {
>> +	{"without_filter", { EMPTY_FILTER }},
>> +	{"member_allow_filter",
>> +	 {DEFINE_FILTER(SW_INCR, 0), DEFINE_FILTER(INST_RETIRED, 0),
>> +	  DEFINE_FILTER(BR_RETIRED, 0), EMPTY_FILTER}},
>> +	{"member_deny_filter",
>> +	 {DEFINE_FILTER(SW_INCR, 1), DEFINE_FILTER(INST_RETIRED, 1),
>> +	  DEFINE_FILTER(BR_RETIRED, 1), EMPTY_FILTER}},
>> +	{"not_member_deny_filter",
>> +	 {DEFINE_FILTER(SW_INCR, 1), EMPTY_FILTER}},
>> +	{"not_member_allow_filter",
>> +	 {DEFINE_FILTER(SW_INCR, 0), EMPTY_FILTER}},
> 
> Why is the filter array special enough to get its own sentinel macro
> but...
> 
>> +	{ 0 }
> 
> ... the test descriptor array is okay to use a 'raw' initialization. My
> vote is to drop the macro, zero-initializing a struct in an array is an
> extremely common pattern in the kernel.
> 
> Also, these descriptors are dense and hard to read. Working with an
> example:
> 
> 	{
> 		.name = "member_allow_filter",
> 		.filter = {
> 			DEFINE_FILTER(SW_INCR, 0),
> 			DEFINE_FILTER(INST_RETIRED, 0),
> 			DEFINE_FILTER(BR_RETIRED, 0),
> 			{ 0 }
> 		},
> 	}
> 
> See how much more readable that is?
> 

It's more clear and readable, thanks for your advice. I will change the 
array definition to the beautiful format.

>> +};
>> +
>> +static void for_each_test(void)
>> +{
>> +	struct test_desc *t;
>> +
>> +	for (t = &tests[0]; t->name; t++)
>> +		run_test(t);
>> +}
> 
> for_each_test() sounds like an iterator, but this is not. Call it
> run_tests()
> 

Ok. Will call it run_tests().

>> +static bool kvm_supports_pmu_event_filter(void)
>> +{
>> +	int r;
>> +
>> +	vpmu_vm = create_vpmu_vm(guest_code);
>> +
>> +	r = __kvm_has_device_attr(vpmu_vm->vcpu->fd, KVM_ARM_VCPU_PMU_V3_CTRL,
>> +				  KVM_ARM_VCPU_PMU_V3_FILTER);
>> +
>> +	destroy_vpmu_vm(vpmu_vm);
>> +	return !r;
>> +}
> 
> TBH, I don't really care much about the test probing for the event
> filter UAPI. It has been upstream for a while, and if folks are trying
> to run selftests at HEAD on an old kernel then that's their business.
> 
> The other prerequisites make more sense since they actually check if HW
> features are present.
> 

If no one cares it, I will delete this function.

>> +static bool host_pmu_supports_events(void)
>> +{
>> +	vpmu_vm = create_vpmu_vm(guest_get_pmceid);
>> +
>> +	memset(&supported_pmce, 0, sizeof(supported_pmce));
>> +	sync_global_to_guest(vpmu_vm->vm, supported_pmce);
>> +	run_vcpu(vpmu_vm->vcpu);
>> +	sync_global_from_guest(vpmu_vm->vm, supported_pmce);
>> +	destroy_vpmu_vm(vpmu_vm);
>> +
>> +	return supported_pmce.pmceid0 & (BR_RETIRED | INST_RETIRED);
>> +}
> 
> This helper says its probing the host PMU, but you're actually firing up a
> VM to do it.
> 
> The events supported by a particular PMU instance are readily available
> in sysfs. Furthermore, you can tell KVM to select the exact host PMU
> instance you probe.

It should call kvm_pmu_support_events, because I want to get the default 
pmce without any filter in the kvm. So I run a guest and get that value 
in the guest.

I've tried get the pmceid0 through the

vcpu_get_reg(vcpu, KVM_ARM64_SYS_REG(SYS_PMCEID0_EL0), &val);

But it always return the -ENOENT, I'm not sure if this is expected. 
Could I can use the KVM interface to get the pmceid0?

> 
>> diff --git a/tools/testing/selftests/kvm/lib/aarch64/vpmu.c b/tools/testing/selftests/kvm/lib/aarch64/vpmu.c
>> index b3de8fdc555e..76ea03d607f1 100644
>> --- a/tools/testing/selftests/kvm/lib/aarch64/vpmu.c
>> +++ b/tools/testing/selftests/kvm/lib/aarch64/vpmu.c
>> @@ -7,8 +7,9 @@
>>   #include <vpmu.h>
>>   #include <perf/arm_pmuv3.h>
>>   
>> -/* Create a VM that has one vCPU with PMUv3 configured. */
>> -struct vpmu_vm *create_vpmu_vm(void *guest_code)
>> +struct vpmu_vm *__create_vpmu_vm(void *guest_code,
>> +				 void (*init_pmu)(struct vpmu_vm *vm, void *arg),
>> +				 void *arg)
>>   {
>>   	struct kvm_vcpu_init init;
>>   	uint8_t pmuver;
>> @@ -50,12 +51,21 @@ struct vpmu_vm *create_vpmu_vm(void *guest_code)
>>   		    "Unexpected PMUVER (0x%x) on the vCPU with PMUv3", pmuver);
>>   
>>   	/* Initialize vPMU */
>> +	if (init_pmu)
>> +		init_pmu(vpmu_vm, arg);
>> +
>>   	vcpu_ioctl(vpmu_vm->vcpu, KVM_SET_DEVICE_ATTR, &irq_attr);
>>   	vcpu_ioctl(vpmu_vm->vcpu, KVM_SET_DEVICE_ATTR, &init_attr);
>>   
>>   	return vpmu_vm;
>>   }
>>   
>> +/* Create a VM that has one vCPU with PMUv3 configured. */
>> +struct vpmu_vm *create_vpmu_vm(void *guest_code)
>> +{
>> +	return __create_vpmu_vm(guest_code, NULL, NULL);
>> +}
>> +
> 
> Ok. This completely proves my point in the other patch. You already need
> to refactor this helper to cram in what you're trying to do. Think of
> ways to move the code that is actually common into libraries and leave
> the rest to the tests themselves.
> 
> Some slight code duplication isn't the end of the world if it avoids
> churning libraries every time someone wants to add a widget.

Thanks for your opinion. I'm thinking about refactor the helper to make 
it can be reuseable by further tests.

Thanks,
Shaoqin

> 

-- 
Shaoqin