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 <[email protected]>
Signed-off-by: Shaoqin Huang <[email protected]>
---
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
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
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