Re: [PATCH kvmtool v2 7/7] arm64: Improve KVM_ARM_VCPU_PMU_V3_CTRL diagnostics

[Oliver Upton <oupton@kernel.org>]

On Thu, Jun 18, 2026 at 04:50:01PM +0100, Alexandru Elisei wrote:
> kvmtool issues several ioctls when configuring the PMU, and each of them
> can fail for different reasons. Be more specific about the ioctl that
> failed when that happens.
> 
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> ---
>  arm64/pmu.c | 31 ++++++++++++++++++++++++-------
>  1 file changed, 24 insertions(+), 7 deletions(-)
> 
> diff --git a/arm64/pmu.c b/arm64/pmu.c
> index 92cacd62479e..78c15f153fad 100644
> --- a/arm64/pmu.c
> +++ b/arm64/pmu.c
> @@ -12,6 +12,24 @@
>  
>  #include "asm/pmu.h"
>  
> +static const char *pmu_attr_names[] = {
> +	[KVM_ARM_VCPU_PMU_V3_IRQ]	= "KVM_ARM_VCPU_PMU_V3_IRQ",
> +	[KVM_ARM_VCPU_PMU_V3_INIT]	= "KVM_ARM_VCPU_PMU_V3_INIT",
> +	[KVM_ARM_VCPU_PMU_V3_FILTER]	= "KVM_ARM_VCPU_PMU_V3_FILTER",
> +	[KVM_ARM_VCPU_PMU_V3_SET_PMU]	= "KVM_ARM_VCPU_PMU_V3_SET_PMU",
> +	[KVM_ARM_VCPU_PMU_V3_SET_NR_COUNTERS] = "KVM_ARM_VCPU_PMU_V3_SET_NR_COUNTER",
> +};
> +
> +static const char *pmu_get_attr_name(u64 attr)
> +{
> +	switch (attr) {
> +	case KVM_ARM_VCPU_PMU_V3_IRQ ... KVM_ARM_VCPU_PMU_V3_SET_NR_COUNTERS:
> +		return pmu_attr_names[attr];
> +	default:
> +		return "UNKNOWN";
> +	}
> +}
> +
>  static bool pmu_has_attr(struct kvm_cpu *vcpu, u64 attr)
>  {
>  	struct kvm_device_attr pmu_attr = {
> @@ -32,13 +50,12 @@ static void set_pmu_attr(struct kvm_cpu *vcpu, void *addr, u64 attr)
>  	};
>  	int ret;
>  
> -	if (pmu_has_attr(vcpu, attr)) {
> -		ret = ioctl(vcpu->vcpu_fd, KVM_SET_DEVICE_ATTR, &pmu_attr);
> -		if (ret)
> -			die_perror("PMU KVM_SET_DEVICE_ATTR");
> -	} else {
> -		die_perror("PMU KVM_HAS_DEVICE_ATTR");
> -	}
> +	if (!pmu_has_attr(vcpu, attr))
> +		die_perror("KVM_HAS_DEVICE_ATTR(%s)", pmu_get_attr_name(attr));
> +
> +	ret = ioctl(vcpu->vcpu_fd, KVM_SET_DEVICE_ATTR, &pmu_attr);
> +	if (ret)
> +		die_perror("KVM_SET_DEVICE_ATTR(%s)", pmu_get_attr_name(attr));
>  }

The whole if (ret) die_perror(...) thing is a bit repetetive IMO. A
treewide cleanup replacing this with macros would be nice, then you could
stringize the ioctl under the hood.

diff --git a/arm64/pmu.c b/arm64/pmu.c
index 5f31d6b..0d9f3df 100644
--- a/arm64/pmu.c
+++ b/arm64/pmu.c
@@ -23,23 +23,19 @@ static bool pmu_has_attr(struct kvm_cpu *vcpu, u64 attr)
 	return ret == 0;
 }
 
-static void set_pmu_attr(struct kvm_cpu *vcpu, void *addr, u64 attr)
-{
-	struct kvm_device_attr pmu_attr = {
-		.group	= KVM_ARM_VCPU_PMU_V3_CTRL,
-		.addr	= (u64)addr,
-		.attr	= attr,
-	};
-	int ret;
-
-	if (pmu_has_attr(vcpu, attr)) {
-		ret = ioctl(vcpu->vcpu_fd, KVM_SET_DEVICE_ATTR, &pmu_attr);
-		if (ret)
-			die_perror("PMU KVM_SET_DEVICE_ATTR");
-	} else {
-		die_perror("PMU KVM_HAS_DEVICE_ATTR");
-	}
-}
+#define kvm_set_device_attr(fd, _group, _attr, _addr)					\
+do {											\
+	struct kvm_device_attr __attr = {						\
+		.group	= (_group),							\
+		.attr	= (_attr),							\
+		.addr	= (u64)(_addr),							\
+	};										\
+	int r;										\
+											\
+	r = ioctl((fd), KVM_SET_DEVICE_ATTR, &__attr);					\
+	if (r)										\
+		die_perror("KVM_SET_DEVICE_ATTR(group:"#_group", attr:"#_attr")");	\
+} while (0)
 
 #define SYS_EVENT_SOURCE	"/sys/bus/event_source/devices/"
 /*
@@ -218,14 +214,18 @@ void pmu__generate_fdt_nodes(void *fdt, struct kvm *kvm)
 
 	for (i = 0; i < kvm->nrcpus; i++) {
 		vcpu = kvm->cpus[i];
-		set_pmu_attr(vcpu, &irq, KVM_ARM_VCPU_PMU_V3_IRQ);
+		kvm_set_device_attr(vcpu->vcpu_fd, KVM_ARM_VCPU_PMU_V3_CTRL,
+				    KVM_ARM_VCPU_PMU_V3_IRQ, &irq);
 		/*
 		 * PMU IDs 0-5 are reserved; a positive value means a PMU was
 		 * found.
 		 */
 		if (pmu_id > 0)
-			set_pmu_attr(vcpu, &pmu_id, KVM_ARM_VCPU_PMU_V3_SET_PMU);
-		set_pmu_attr(vcpu, NULL, KVM_ARM_VCPU_PMU_V3_INIT);
+			kvm_set_device_attr(vcpu->vcpu_fd, KVM_ARM_VCPU_PMU_V3_CTRL,
+					    KVM_ARM_VCPU_PMU_V3_SET_PMU, &pmu_id);
+
+		kvm_set_device_attr(vcpu->vcpu_fd, KVM_ARM_VCPU_PMU_V3_CTRL,
+				    KVM_ARM_VCPU_PMU_V3_INIT, NULL);
 	}
 
 	_FDT(fdt_begin_node(fdt, "pmu"));