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.