[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <f557f89c-7699-4c58-ab91-486d57d723e7@redhat.com>
Date: Tue, 27 Feb 2024 11:10:34 +0800
From: Shaoqin Huang <shahuang@...hat.com>
To: Oliver Upton <oliver.upton@...ux.dev>
Cc: Marc Zyngier <maz@...nel.org>, kvmarm@...ts.linux.dev,
Eric Auger <eauger@...hat.com>, Eric Auger <eric.auger@...hat.com>,
Paolo Bonzini <pbonzini@...hat.com>, Shuah Khan <shuah@...nel.org>,
James Morse <james.morse@....com>, Suzuki K Poulose
<suzuki.poulose@....com>, Zenghui Yu <yuzenghui@...wei.com>,
linux-kernel@...r.kernel.org, kvm@...r.kernel.org,
linux-kselftest@...r.kernel.org, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v4 1/5] KVM: selftests: aarch64: Make the
[create|destroy]_vpmu_vm() public
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
Powered by blists - more mailing lists