Re: [PATCH v4 1/5] KVM: selftests: aarch64: Make the [create|destroy]_vpmu_vm() public

From: Oliver Upton <oliver.upton@linux.dev>
To: Shaoqin Huang <shahuang@redhat.com>
Cc: Marc Zyngier <maz@kernel.org>,
	kvmarm@lists.linux.dev, Eric Auger <eauger@redhat.com>,
	Eric Auger <eric.auger@redhat.com>,
	Paolo Bonzini <pbonzini@redhat.com>,
	Shuah Khan <shuah@kernel.org>, James Morse <james.morse@arm.com>,
	Suzuki K Poulose <suzuki.poulose@arm.com>,
	Zenghui Yu <yuzenghui@huawei.com>,
	linux-kernel@vger.kernel.org, kvm@vger.kernel.org,
	linux-kselftest@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v4 1/5] KVM: selftests: aarch64: Make the [create|destroy]_vpmu_vm() public
Date: Fri, 2 Feb 2024 07:36:08 +0000	[thread overview]
Message-ID: <ZbybaH2t7Yp9NJOK@linux.dev> (raw)
In-Reply-To: <20240202025659.5065-2-shahuang@redhat.com>

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