Messages in this thread | | | Date | Tue, 27 Feb 2024 11:10:34 +0800 | Subject | Re: [PATCH v4 1/5] KVM: selftests: aarch64: Make the [create|destroy]_vpmu_vm() public | From | 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
| |