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