[PATCH kvmtool 0/6] x86 compilation fixes and arm64 PMU improvements

[PATCH kvmtool 0/6] x86 compilation fixes and arm64 PMU improvements

[Alexandru Elisei]

The first two patches fix the following compilation errors on x86 when
using gcc 15.2.1:

disk/core.c: In function ‘disk_img_name_parser’:
disk/core.c:27:21: error: assignment discards ‘const’ qualifier from pointer target type [-Werror=discarded-qualifiers]
   27 |                 sep = strstr(arg, ":");
      |                     ^
disk/core.c:39:21: error: assignment discards ‘const’ qualifier from pointer target type [-Werror=discarded-qualifiers]
   39 |                 sep = strstr(cur, ",");
      |                     ^
virtio/9p.c: In function ‘virtio_9p_rootdir_parser’:
virtio/9p.c:1503:18: error: assignment discards ‘const’ qualifier from pointer target type [-Werror=discarded-qualifiers]
 1503 |         tag_name = strstr(arg, ",");
      |                  ^

I couldn't reproduce the errors on arm64 using aarch64-linux-gnu-gcc 15.1.0
and 14.2.0. I tried doing make with V=1 and it looked to me like kvmtool
uses the same flags on x86 as on arm64, so I assumed it's because of the
difference in compilers and I didn't dig further.

The remaining of the patches are there to improve PMU initialisation and
error reporting.

Alexandru Elisei (6):
  virtio: Do not modify const strings in virtio_9p_rootdir_parser()
  disk/core: Do not modify const strings in disk_img_name_parser()
  arm64: Initialise the PMU last
  util: Set exit status to errno in die_perror()
  util: Allow die_perror() to take a variable list of argument
  arm64: Improve KVM_ARM_VCPU_PMU_V3_CTRL diagnostics

 arm64/pmu.c              | 60 ++++++++++++++++++++++++++++------------
 disk/core.c              | 44 +++++++++++++++++++----------
 include/kvm/disk-image.h |  6 ++--
 include/kvm/util-init.h  |  6 ++--
 include/kvm/util.h       |  2 +-
 util/util.c              | 21 ++++++++++++--
 virtio/9p.c              | 21 +++++++++-----
 7 files changed, 113 insertions(+), 47 deletions(-)


base-commit: eb915c763e4b95dd4a9d32763fd449459742e053
-- 
2.53.0

[PATCH kvmtool 1/6] virtio: Do not modify const strings in virtio_9p_rootdir_parser()

[Alexandru Elisei]

Even though @arg is of type const char *, virtio_9p_rootdir_parser()
modifies it it if a tag name is specified, leading to the following
compilation error on an x86_64 machine with gcc 15.2.1:

virtio/9p.c:1503:18: error: assignment discards ‘const’ qualifier from pointer target type [-Werror=discarded-qualifiers]
 1503 |         tag_name = strstr(arg, ",");

Fix it by copying @arg to a local string, which can then be modified
in-place.

Also use die_perror() if realpath() fails, to provide more information to
the user.

Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
 virtio/9p.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/virtio/9p.c b/virtio/9p.c
index e6f669c49895..cf3e547d3173 100644
--- a/virtio/9p.c
+++ b/virtio/9p.c
@@ -1491,25 +1491,32 @@ struct virtio_ops p9_dev_virtio_ops = {
 
 int virtio_9p_rootdir_parser(const struct option *opt, const char *arg, int unset)
 {
-	char *tag_name;
-	char tmp[PATH_MAX];
 	struct kvm *kvm = opt->ptr;
+	char *tag_name, *copy;
+	char path[PATH_MAX];
 
+	copy = strdup(arg);
+	if (!copy)
+		die_perror("strdup");
 	/*
 	 * 9p dir can be of the form dirname,tag_name or
 	 * just dirname. In the later case we use the
 	 * default tag name
 	 */
-	tag_name = strstr(arg, ",");
+	tag_name = strstr(copy, ",");
 	if (tag_name) {
 		*tag_name = '\0';
 		tag_name++;
 	}
-	if (realpath(arg, tmp)) {
-		if (virtio_9p__register(kvm, tmp, tag_name) < 0)
+	if (realpath(copy, path)) {
+		if (virtio_9p__register(kvm, path, tag_name) < 0)
 			die("Unable to initialize virtio 9p");
-	} else
-		die("Failed resolving 9p path");
+	} else {
+		die_perror("Failed resolving 9p path");
+	}
+
+	free(copy);
+
 	return 0;
 }
 
-- 
2.53.0

[PATCH kvmtool 2/6] disk/core: Do not modify const strings in disk_img_name_parser()

[Alexandru Elisei]

@arg is const char *, but disk_img_name_parser() modifies it, which leads
to a compilation error on x86 with gcc 15.2.1:

disk/core.c:27:21: error: assignment discards ‘const’ qualifier from pointer target type [-Werror=discarded-qualifiers]
   27 |                 sep = strstr(arg, ":");

Make a copy of @arg and modify it instead, and be very careful to free it
when no longer needed.

Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---

I followed the instruction from Documentation/io-testing, but I couldn't
get a virtio SCSI disk work in a VM, I kept getting this kvmtool error when
the guest tried to use it:

[    0.154837] virtio_scsi virtio1: 1/0/0 default/read/poll queues
[    0.156965] scsi host0: Virtio SCSI HBA
  Fatal: VHOST_SCSI_SET_ENDPOINT failed 19

I did print disk_image_params->{filename,wwpn,readonly,direct} before and
after the patch and they were identical. Also ran valgrind to make sure
there were no leaks, but I didn't cover all the possible execution flows.

Would be really nice if someone with working virtio-scsi could test this :)

 disk/core.c              | 44 +++++++++++++++++++++++++++-------------
 include/kvm/disk-image.h |  6 +++---
 2 files changed, 33 insertions(+), 17 deletions(-)

diff --git a/disk/core.c b/disk/core.c
index 91db277f7846..b232eece9e73 100644
--- a/disk/core.c
+++ b/disk/core.c
@@ -13,35 +13,42 @@ static int disk_image__close(struct disk_image *disk);
 
 int disk_img_name_parser(const struct option *opt, const char *arg, int unset)
 {
-	const char *cur;
-	char *sep;
+	struct disk_image_params *params;
 	struct kvm *kvm = opt->ptr;
+	char *cur, *sep;
 
 	if (kvm->nr_disks >= MAX_DISK_IMAGES)
 		die("Currently only 4 images are supported");
 
-	kvm->cfg.disk_image[kvm->nr_disks].filename = arg;
-	cur = arg;
+	params = &kvm->cfg.disk_image[kvm->nr_disks];
 
 	if (strncmp(arg, "scsi:", 5) == 0) {
-		sep = strstr(arg, ":");
-		kvm->cfg.disk_image[kvm->nr_disks].wwpn = sep + 1;
+		params->wwpn = strdup(strstr(arg, ":") + 1);
+		if (!params->wwpn)
+			die_perror("strdup");
 
 		/* Old invocation had two parameters. Ignore the second one. */
-		sep = strstr(sep + 1, ":");
+		sep = strstr(params->wwpn, ":");
 		if (sep) {
 			*sep = 0;
 			cur = sep + 1;
+		} else {
+			cur = params->wwpn;
 		}
+	} else {
+		params->filename = strdup(arg);
+		if (!params->filename)
+			die_perror("strdup");
+		cur = params->filename;
 	}
 
 	do {
 		sep = strstr(cur, ",");
 		if (sep) {
 			if (strncmp(sep + 1, "ro", 2) == 0)
-				kvm->cfg.disk_image[kvm->nr_disks].readonly = true;
+				params->readonly = true;
 			else if (strncmp(sep + 1, "direct", 6) == 0)
-				kvm->cfg.disk_image[kvm->nr_disks].direct = true;
+				params->direct = true;
 			*sep = 0;
 			cur = sep + 1;
 		}
@@ -145,8 +152,7 @@ static struct disk_image *disk_image__open(const char *filename, bool readonly,
 static struct disk_image **disk_image__open_all(struct kvm *kvm)
 {
 	struct disk_image **disks;
-	const char *filename;
-	const char *wwpn;
+	char *filename, *wwpn;
 	bool readonly;
 	bool direct;
 	void *err;
@@ -189,18 +195,27 @@ static struct disk_image **disk_image__open_all(struct kvm *kvm)
 			goto error;
 		}
 		disks[i]->debug_iodelay = kvm->cfg.debug_iodelay;
+
+		free(params[i].filename);
+		params[i].filename = NULL;
 	}
 
 	return disks;
 error:
 	for (i = 0; i < count; i++) {
-		if (IS_ERR_OR_NULL(disks[i]))
+		free(params[i].filename);
+
+		if (IS_ERR_OR_NULL(disks[i])) {
+			free(params[i].wwpn);
 			continue;
+		}
 
-		if (disks[i]->wwpn)
+		if (disks[i]->wwpn) {
+			free(disks[i]->wwpn);
 			free(disks[i]);
-		else
+		} else {
 			disk_image__close(disks[i]);
+		}
 	}
 	free(disks);
 	return err;
@@ -236,6 +251,7 @@ static int disk_image__close(struct disk_image *disk)
 	if (disk->fd && close(disk->fd) < 0)
 		pr_warning("close() failed");
 
+	free(disk->wwpn);
 	free(disk);
 
 	return 0;
diff --git a/include/kvm/disk-image.h b/include/kvm/disk-image.h
index bf602b582a3b..cbe91b07af0b 100644
--- a/include/kvm/disk-image.h
+++ b/include/kvm/disk-image.h
@@ -47,9 +47,9 @@ struct disk_image_operations {
 };
 
 struct disk_image_params {
-	const char *filename;
+	char *filename;
 	/* wwpn == World Wide Port Number */
-	const char *wwpn;
+	char *wwpn;
 	bool readonly;
 	bool direct;
 };
@@ -69,7 +69,7 @@ struct disk_image {
 	pthread_t			thread;
 	u64				aio_inflight;
 #endif /* CONFIG_HAS_AIO */
-	const char			*wwpn;
+	char				*wwpn;
 	int				debug_iodelay;
 };
 
-- 
2.53.0

Re: [PATCH kvmtool 2/6] disk/core: Do not modify const strings in disk_img_name_parser()

[Will Deacon]

On Mon, Mar 23, 2026 at 03:02:17PM +0000, Alexandru Elisei wrote:
> @arg is const char *, but disk_img_name_parser() modifies it, which leads
> to a compilation error on x86 with gcc 15.2.1:
> 
> disk/core.c:27:21: error: assignment discards ‘const’ qualifier from pointer target type [-Werror=discarded-qualifiers]
>    27 |                 sep = strstr(arg, ":");
> 
> Make a copy of @arg and modify it instead, and be very careful to free it
> when no longer needed.
> 
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> ---
> 
> I followed the instruction from Documentation/io-testing, but I couldn't
> get a virtio SCSI disk work in a VM, I kept getting this kvmtool error when
> the guest tried to use it:
> 
> [    0.154837] virtio_scsi virtio1: 1/0/0 default/read/poll queues
> [    0.156965] scsi host0: Virtio SCSI HBA
>   Fatal: VHOST_SCSI_SET_ENDPOINT failed 19

That's -ENODEV, so presumably vhost_scsi_set_endpoint() (in the host
kernel) is failing to match the device. Can you stick some tracing in
there to figure out what's going wrong?

Will

Re: [PATCH kvmtool 2/6] disk/core: Do not modify const strings in disk_img_name_parser()

[Alexandru Elisei]

Hi Will,

On Sun, May 17, 2026 at 09:29:23AM +0100, Will Deacon wrote:
> On Mon, Mar 23, 2026 at 03:02:17PM +0000, Alexandru Elisei wrote:
> > @arg is const char *, but disk_img_name_parser() modifies it, which leads
> > to a compilation error on x86 with gcc 15.2.1:
> > 
> > disk/core.c:27:21: error: assignment discards ‘const’ qualifier from pointer target type [-Werror=discarded-qualifiers]
> >    27 |                 sep = strstr(arg, ":");
> > 
> > Make a copy of @arg and modify it instead, and be very careful to free it
> > when no longer needed.
> > 
> > Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> > ---
> > 
> > I followed the instruction from Documentation/io-testing, but I couldn't
> > get a virtio SCSI disk work in a VM, I kept getting this kvmtool error when
> > the guest tried to use it:
> > 
> > [    0.154837] virtio_scsi virtio1: 1/0/0 default/read/poll queues
> > [    0.156965] scsi host0: Virtio SCSI HBA
> >   Fatal: VHOST_SCSI_SET_ENDPOINT failed 19
> 
> That's -ENODEV, so presumably vhost_scsi_set_endpoint() (in the host
> kernel) is failing to match the device. Can you stick some tracing in
> there to figure out what's going wrong?

I am pretty sure I made a configuration error when creating the scsi-disk in
userspace. Jean-Philippe was able to run upstream kvmtool (without this series)
with a scsi-disk with his setup. I'll try harder for the next iteration.

Thanks,
Alex

[PATCH kvmtool 3/6] arm64: Initialise the PMU last

[Alexandru Elisei]

PMU initialisation is done in:

setup_fdt()
  vcpu->generate_fdt_nodes()
    pmu__generate_fdt_nodes()

The PMU ioctl KVM_ARM_VCPU_PMU_V3_INIT requires that the GIC has been
initialised, which is done in gic__init_gic().

gic__init_gic() and setup_fdt() are part of the same initialisation list.
The relative order on the list depends on the order of compilation: gic.c
is compiled after fdt.c and this places gic__init_gic() first on the
initialisation list.

If the compilation order changes and gic.c is compiled *before* fdt.c,
gic__init_gic() will be executed *after* setup_fdt() and PMU initialisation
will fail with an error message:

PMU KVM_SET_DEVICE_ATTR: No such device

This is fragile and hard to debug. Improve the situation by creating a new
init list, last_init(), execute it after late_init(), and move PMU
initialisation on that list.

Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---

Another option would be to initialise the gic in:

setup_fdt()
  vcpu->generate_fdt_nodes()
    gic__generate_fdt_nodes() <-- initialise here
    timer__generate_fdt_nodes()
    pmu__generate_fdt_nodes()

but doing device initialisation in the same function that generates the fdt
seems out of place, so I opted to initialise the PMU outside of the
function that generates the FDT, in an init list, just like the GIC.

 arm64/pmu.c             | 29 +++++++++++++++++++----------
 include/kvm/util-init.h |  6 ++++--
 2 files changed, 23 insertions(+), 12 deletions(-)

diff --git a/arm64/pmu.c b/arm64/pmu.c
index 5f31d6b6f346..e15244b51c1f 100644
--- a/arm64/pmu.c
+++ b/arm64/pmu.c
@@ -193,21 +193,32 @@ static int find_pmu(struct kvm *kvm)
 void pmu__generate_fdt_nodes(void *fdt, struct kvm *kvm)
 {
 	const char compatible[] = "arm,armv8-pmuv3";
-	int irq = KVM_ARM_PMUv3_PPI;
-	struct kvm_cpu *vcpu;
-	int pmu_id = -ENXIO;
-	int i;
-
 	u32 cpu_mask = gic__get_fdt_irq_cpumask(kvm);
 	u32 irq_prop[] = {
 		cpu_to_fdt32(GIC_FDT_IRQ_TYPE_PPI),
-		cpu_to_fdt32(irq - 16),
+		cpu_to_fdt32(KVM_ARM_PMUv3_PPI - 16),
 		cpu_to_fdt32(cpu_mask | IRQ_TYPE_LEVEL_HIGH),
 	};
 
 	if (!kvm->cfg.arch.has_pmuv3)
 		return;
 
+	_FDT(fdt_begin_node(fdt, "pmu"));
+	_FDT(fdt_property(fdt, "compatible", compatible, sizeof(compatible)));
+	_FDT(fdt_property(fdt, "interrupts", irq_prop, sizeof(irq_prop)));
+	_FDT(fdt_end_node(fdt));
+}
+
+static int pmu__init(struct kvm *kvm)
+{
+	int irq = KVM_ARM_PMUv3_PPI;
+	struct kvm_cpu *vcpu;
+	int pmu_id = -ENXIO;
+	int i;
+
+	if (!kvm->cfg.arch.has_pmuv3)
+		return 0;
+
 	if (pmu_has_attr(kvm->cpus[0], KVM_ARM_VCPU_PMU_V3_SET_PMU)) {
 		pmu_id = find_pmu(kvm);
 		if (pmu_id < 0) {
@@ -228,8 +239,6 @@ void pmu__generate_fdt_nodes(void *fdt, struct kvm *kvm)
 		set_pmu_attr(vcpu, NULL, KVM_ARM_VCPU_PMU_V3_INIT);
 	}
 
-	_FDT(fdt_begin_node(fdt, "pmu"));
-	_FDT(fdt_property(fdt, "compatible", compatible, sizeof(compatible)));
-	_FDT(fdt_property(fdt, "interrupts", irq_prop, sizeof(irq_prop)));
-	_FDT(fdt_end_node(fdt));
+	return 0;
 }
+last_init(pmu__init);
diff --git a/include/kvm/util-init.h b/include/kvm/util-init.h
index 13d4f04df678..e6a0e1696689 100644
--- a/include/kvm/util-init.h
+++ b/include/kvm/util-init.h
@@ -39,7 +39,8 @@ static void __attribute__ ((constructor)) __init__##cb(void)		\
 #define dev_init(cb) __init_list_add(cb, 5)
 #define virtio_dev_init(cb) __init_list_add(cb, 6)
 #define firmware_init(cb) __init_list_add(cb, 7)
-#define late_init(cb) __init_list_add(cb, 9)
+#define late_init(cb) __init_list_add(cb, 8)
+#define last_init(cb) __init_list_add(cb, 9)
 
 #define core_exit(cb) __exit_list_add(cb, 0)
 #define base_exit(cb) __exit_list_add(cb, 2)
@@ -47,5 +48,6 @@ static void __attribute__ ((constructor)) __init__##cb(void)		\
 #define dev_exit(cb) __exit_list_add(cb, 5)
 #define virtio_dev_exit(cb) __exit_list_add(cb, 6)
 #define firmware_exit(cb) __exit_list_add(cb, 7)
-#define late_exit(cb) __exit_list_add(cb, 9)
+#define late_exit(cb) __exit_list_add(cb, 8)
+#define last_exit(cb) __exit_list_add(cb, 9)
 #endif
-- 
2.53.0

Re: [PATCH kvmtool 3/6] arm64: Initialise the PMU last

[Will Deacon]

On Mon, Mar 23, 2026 at 03:02:18PM +0000, Alexandru Elisei wrote:
> PMU initialisation is done in:
> 
> setup_fdt()
>   vcpu->generate_fdt_nodes()
>     pmu__generate_fdt_nodes()
> 
> The PMU ioctl KVM_ARM_VCPU_PMU_V3_INIT requires that the GIC has been
> initialised, which is done in gic__init_gic().
> 
> gic__init_gic() and setup_fdt() are part of the same initialisation list.
> The relative order on the list depends on the order of compilation: gic.c
> is compiled after fdt.c and this places gic__init_gic() first on the
> initialisation list.
> 
> If the compilation order changes and gic.c is compiled *before* fdt.c,
> gic__init_gic() will be executed *after* setup_fdt() and PMU initialisation
> will fail with an error message:
> 
> PMU KVM_SET_DEVICE_ATTR: No such device
> 
> This is fragile and hard to debug. Improve the situation by creating a new
> init list, last_init(), execute it after late_init(), and move PMU
> initialisation on that list.

Hmm, not a big fan of that. There's always _somebody_ who wants to be
later/earlier than everybody else (see the CCA probing converstion!).

If the PMU genuinely depends on the GIC, then perhaps we should just
call the PMU init function from the GIC init function, along with a
comment to explain the dependency? This is all arm64 code so I think
that's fine.

Will

Re: [PATCH kvmtool 3/6] arm64: Initialise the PMU last

[Alexandru Elisei]

Hi Will,

On Sun, May 17, 2026 at 09:33:42AM +0100, Will Deacon wrote:
> On Mon, Mar 23, 2026 at 03:02:18PM +0000, Alexandru Elisei wrote:
> > PMU initialisation is done in:
> > 
> > setup_fdt()
> >   vcpu->generate_fdt_nodes()
> >     pmu__generate_fdt_nodes()
> > 
> > The PMU ioctl KVM_ARM_VCPU_PMU_V3_INIT requires that the GIC has been
> > initialised, which is done in gic__init_gic().
> > 
> > gic__init_gic() and setup_fdt() are part of the same initialisation list.
> > The relative order on the list depends on the order of compilation: gic.c
> > is compiled after fdt.c and this places gic__init_gic() first on the
> > initialisation list.
> > 
> > If the compilation order changes and gic.c is compiled *before* fdt.c,
> > gic__init_gic() will be executed *after* setup_fdt() and PMU initialisation
> > will fail with an error message:
> > 
> > PMU KVM_SET_DEVICE_ATTR: No such device
> > 
> > This is fragile and hard to debug. Improve the situation by creating a new
> > init list, last_init(), execute it after late_init(), and move PMU
> > initialisation on that list.
> 
> Hmm, not a big fan of that. There's always _somebody_ who wants to be
> later/earlier than everybody else (see the CCA probing converstion!).

Sure.

> 
> If the PMU genuinely depends on the GIC, then perhaps we should just
> call the PMU init function from the GIC init function, along with a
> comment to explain the dependency? This is all arm64 code so I think
> that's fine.

Sounds good to me.

Thanks,
Alex

[PATCH kvmtool 4/6] util: Set exit status to errno in die_perror()

[Alexandru Elisei]

Exit with a return code equal to errno in die_perror() to make it easier
for the user to get the error code directly.

As per man 3 perror, errno is undefined after a successful library call, so
save it when entering die_perror().

Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
 util/util.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/util/util.c b/util/util.c
index bab4bb394111..fc732200f2dc 100644
--- a/util/util.c
+++ b/util/util.c
@@ -100,8 +100,10 @@ void __pr_debug(const char *debug, ...)
 
 void die_perror(const char *s)
 {
+	int e = errno;
+
 	perror(s);
-	exit(1);
+	exit(e);
 }
 
 void *mmap_hugetlbfs(struct kvm *kvm, const char *htlbfs_path, u64 size)
-- 
2.53.0

[PATCH kvmtool 5/6] util: Allow die_perror() to take a variable list of argument

[Alexandru Elisei]

Make die_perror() more similar to die() by allowing the user to specify
a format string and arguments.

Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
 include/kvm/util.h |  2 +-
 util/util.c        | 17 +++++++++++++++--
 2 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/include/kvm/util.h b/include/kvm/util.h
index ac8515f8c46b..a6dba1147d68 100644
--- a/include/kvm/util.h
+++ b/include/kvm/util.h
@@ -41,7 +41,7 @@
 #define MAP_ANON_NORESERVE (MAP_PRIVATE | MAP_ANONYMOUS | MAP_NORESERVE)
 
 extern void die(const char *err, ...) NORETURN __attribute__((format (printf, 1, 2)));
-extern void die_perror(const char *s) NORETURN;
+extern void die_perror(const char *fmt, ...) NORETURN __attribute__((format (printf, 1, 2)));
 extern void pr_err(const char *err, ...) __attribute__((format (printf, 1, 2)));
 extern void pr_warning(const char *err, ...) __attribute__((format (printf, 1, 2)));
 extern void pr_info(const char *err, ...) __attribute__((format (printf, 1, 2)));
diff --git a/util/util.c b/util/util.c
index fc732200f2dc..0f116e0ac459 100644
--- a/util/util.c
+++ b/util/util.c
@@ -1,6 +1,7 @@
 /*
  * Taken from perf which in turn take it from GIT
  */
+#include <stdio.h>
 
 #include "kvm/util.h"
 
@@ -98,11 +99,23 @@ void __pr_debug(const char *debug, ...)
 	va_end(params);
 }
 
-void die_perror(const char *s)
+void die_perror(const char *fmt, ...)
 {
+	char buf[1024];
+	va_list params;
 	int e = errno;
+	int ret;
+
+	va_start(params, fmt);
+	ret = vsnprintf(buf, sizeof(buf), fmt, params);
+	if (ret < 0) {
+		e = errno;
+		strncpy(buf, "vsnprintf", ARRAY_SIZE(buf) - 1);
+	}
+	va_end(params);
 
-	perror(s);
+	errno = e;
+	perror(buf);
 	exit(e);
 }
 
-- 
2.53.0

Re: [PATCH kvmtool 5/6] util: Allow die_perror() to take a variable list of argument

[Will Deacon]

On Mon, Mar 23, 2026 at 03:02:20PM +0000, Alexandru Elisei wrote:
> Make die_perror() more similar to die() by allowing the user to specify
> a format string and arguments.
> 
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> ---
>  include/kvm/util.h |  2 +-
>  util/util.c        | 17 +++++++++++++++--
>  2 files changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/include/kvm/util.h b/include/kvm/util.h
> index ac8515f8c46b..a6dba1147d68 100644
> --- a/include/kvm/util.h
> +++ b/include/kvm/util.h
> @@ -41,7 +41,7 @@
>  #define MAP_ANON_NORESERVE (MAP_PRIVATE | MAP_ANONYMOUS | MAP_NORESERVE)
>  
>  extern void die(const char *err, ...) NORETURN __attribute__((format (printf, 1, 2)));
> -extern void die_perror(const char *s) NORETURN;
> +extern void die_perror(const char *fmt, ...) NORETURN __attribute__((format (printf, 1, 2)));
>  extern void pr_err(const char *err, ...) __attribute__((format (printf, 1, 2)));
>  extern void pr_warning(const char *err, ...) __attribute__((format (printf, 1, 2)));
>  extern void pr_info(const char *err, ...) __attribute__((format (printf, 1, 2)));
> diff --git a/util/util.c b/util/util.c
> index fc732200f2dc..0f116e0ac459 100644
> --- a/util/util.c
> +++ b/util/util.c
> @@ -1,6 +1,7 @@
>  /*
>   * Taken from perf which in turn take it from GIT
>   */
> +#include <stdio.h>
>  
>  #include "kvm/util.h"
>  
> @@ -98,11 +99,23 @@ void __pr_debug(const char *debug, ...)
>  	va_end(params);
>  }
>  
> -void die_perror(const char *s)
> +void die_perror(const char *fmt, ...)
>  {
> +	char buf[1024];
> +	va_list params;
>  	int e = errno;
> +	int ret;
> +
> +	va_start(params, fmt);
> +	ret = vsnprintf(buf, sizeof(buf), fmt, params);
> +	if (ret < 0) {
> +		e = errno;
> +		strncpy(buf, "vsnprintf", ARRAY_SIZE(buf) - 1);

That looks really confusing to me as we're effectively hiding the real
error that caused us to get here just because we couldn't pretty-print
the error string! In the unlikely case that vsnprintf() fails, I think
we should preserve the original errno and either print a string to say
that the formatting failed _or_ pretty-print the original error with
strerror().

Will

Re: [PATCH kvmtool 5/6] util: Allow die_perror() to take a variable list of argument

[Alexandru Elisei]

Hi Will,

On Sun, May 17, 2026 at 09:44:30AM +0100, Will Deacon wrote:
> On Mon, Mar 23, 2026 at 03:02:20PM +0000, Alexandru Elisei wrote:
> > Make die_perror() more similar to die() by allowing the user to specify
> > a format string and arguments.
> > 
> > Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> > ---
> >  include/kvm/util.h |  2 +-
> >  util/util.c        | 17 +++++++++++++++--
> >  2 files changed, 16 insertions(+), 3 deletions(-)
> > 
> > diff --git a/include/kvm/util.h b/include/kvm/util.h
> > index ac8515f8c46b..a6dba1147d68 100644
> > --- a/include/kvm/util.h
> > +++ b/include/kvm/util.h
> > @@ -41,7 +41,7 @@
> >  #define MAP_ANON_NORESERVE (MAP_PRIVATE | MAP_ANONYMOUS | MAP_NORESERVE)
> >  
> >  extern void die(const char *err, ...) NORETURN __attribute__((format (printf, 1, 2)));
> > -extern void die_perror(const char *s) NORETURN;
> > +extern void die_perror(const char *fmt, ...) NORETURN __attribute__((format (printf, 1, 2)));
> >  extern void pr_err(const char *err, ...) __attribute__((format (printf, 1, 2)));
> >  extern void pr_warning(const char *err, ...) __attribute__((format (printf, 1, 2)));
> >  extern void pr_info(const char *err, ...) __attribute__((format (printf, 1, 2)));
> > diff --git a/util/util.c b/util/util.c
> > index fc732200f2dc..0f116e0ac459 100644
> > --- a/util/util.c
> > +++ b/util/util.c
> > @@ -1,6 +1,7 @@
> >  /*
> >   * Taken from perf which in turn take it from GIT
> >   */
> > +#include <stdio.h>
> >  
> >  #include "kvm/util.h"
> >  
> > @@ -98,11 +99,23 @@ void __pr_debug(const char *debug, ...)
> >  	va_end(params);
> >  }
> >  
> > -void die_perror(const char *s)
> > +void die_perror(const char *fmt, ...)
> >  {
> > +	char buf[1024];
> > +	va_list params;
> >  	int e = errno;
> > +	int ret;
> > +
> > +	va_start(params, fmt);
> > +	ret = vsnprintf(buf, sizeof(buf), fmt, params);
> > +	if (ret < 0) {
> > +		e = errno;
> > +		strncpy(buf, "vsnprintf", ARRAY_SIZE(buf) - 1);
> 
> That looks really confusing to me as we're effectively hiding the real
> error that caused us to get here just because we couldn't pretty-print
> the error string! In the unlikely case that vsnprintf() fails, I think
> we should preserve the original errno and either print a string to say
> that the formatting failed _or_ pretty-print the original error with
> strerror().

You have a good point.

I'm not exactly sure what you mean by pretty-printing the original error.
Do you mean that we should print the message passed to die_perror()? I'm
don't know how I can do that reliably. I could use vprintf(), but if
vsnprintf() failed, wouldn't that make it likely that vprintf() will also
fail?

How about this:

diff --git a/util/util.c b/util/util.c
index 0f116e0ac459..4e3704f781e8 100644
--- a/util/util.c
+++ b/util/util.c
@@ -109,8 +109,9 @@ void die_perror(const char *fmt, ...)
        va_start(params, fmt);
        ret = vsnprintf(buf, sizeof(buf), fmt, params);
        if (ret < 0) {
-               e = errno;
-               strncpy(buf, "vsnprintf", ARRAY_SIZE(buf) - 1);
+               fprintf(stderr, "vsnprintf failed with error %d (%s)\n",
+                       errno, strerror(errno));
+               strncpy(buf, "Error", ARRAY_SIZE(buf) - 1);
        }
        va_end(params);

but we're still losing the original message.

Thanks,
Alex

Re: [PATCH kvmtool 5/6] util: Allow die_perror() to take a variable list of argument

[Will Deacon]

On Mon, May 18, 2026 at 11:12:56AM +0100, Alexandru Elisei wrote:
> On Sun, May 17, 2026 at 09:44:30AM +0100, Will Deacon wrote:
> > On Mon, Mar 23, 2026 at 03:02:20PM +0000, Alexandru Elisei wrote:
> > > @@ -98,11 +99,23 @@ void __pr_debug(const char *debug, ...)
> > >  	va_end(params);
> > >  }
> > >  
> > > -void die_perror(const char *s)
> > > +void die_perror(const char *fmt, ...)
> > >  {
> > > +	char buf[1024];
> > > +	va_list params;
> > >  	int e = errno;
> > > +	int ret;
> > > +
> > > +	va_start(params, fmt);
> > > +	ret = vsnprintf(buf, sizeof(buf), fmt, params);
> > > +	if (ret < 0) {
> > > +		e = errno;
> > > +		strncpy(buf, "vsnprintf", ARRAY_SIZE(buf) - 1);
> > 
> > That looks really confusing to me as we're effectively hiding the real
> > error that caused us to get here just because we couldn't pretty-print
> > the error string! In the unlikely case that vsnprintf() fails, I think
> > we should preserve the original errno and either print a string to say
> > that the formatting failed _or_ pretty-print the original error with
> > strerror().
> 
> You have a good point.
> 
> I'm not exactly sure what you mean by pretty-printing the original error.
> Do you mean that we should print the message passed to die_perror()? I'm
> don't know how I can do that reliably. I could use vprintf(), but if
> vsnprintf() failed, wouldn't that make it likely that vprintf() will also
> fail?
> 
> How about this:
> 
> diff --git a/util/util.c b/util/util.c
> index 0f116e0ac459..4e3704f781e8 100644
> --- a/util/util.c
> +++ b/util/util.c
> @@ -109,8 +109,9 @@ void die_perror(const char *fmt, ...)
>         va_start(params, fmt);
>         ret = vsnprintf(buf, sizeof(buf), fmt, params);
>         if (ret < 0) {
> -               e = errno;
> -               strncpy(buf, "vsnprintf", ARRAY_SIZE(buf) - 1);
> +               fprintf(stderr, "vsnprintf failed with error %d (%s)\n",
> +                       errno, strerror(errno));
> +               strncpy(buf, "Error", ARRAY_SIZE(buf) - 1);
>         }
>         va_end(params);
> 
> but we're still losing the original message.

Sorry, I think my suggestion to use strerror() was unhelpful. Maybe just
do something like:

	ret = vsnprintf(buf, sizeof(buf), fmt, params);
	if (ret < 0) {
		perror("vsnprintf");
		buf[0] = '\0';
	}

	...

	errno = e;
	perror(buf);

Will

Re: [PATCH kvmtool 5/6] util: Allow die_perror() to take a variable list of argument

[Alexandru Elisei]

Hi Will,

On Mon, May 18, 2026 at 04:06:29PM +0100, Will Deacon wrote:
> On Mon, May 18, 2026 at 11:12:56AM +0100, Alexandru Elisei wrote:
> > On Sun, May 17, 2026 at 09:44:30AM +0100, Will Deacon wrote:
> > > On Mon, Mar 23, 2026 at 03:02:20PM +0000, Alexandru Elisei wrote:
> > > > @@ -98,11 +99,23 @@ void __pr_debug(const char *debug, ...)
> > > >  	va_end(params);
> > > >  }
> > > >  
> > > > -void die_perror(const char *s)
> > > > +void die_perror(const char *fmt, ...)
> > > >  {
> > > > +	char buf[1024];
> > > > +	va_list params;
> > > >  	int e = errno;
> > > > +	int ret;
> > > > +
> > > > +	va_start(params, fmt);
> > > > +	ret = vsnprintf(buf, sizeof(buf), fmt, params);
> > > > +	if (ret < 0) {
> > > > +		e = errno;
> > > > +		strncpy(buf, "vsnprintf", ARRAY_SIZE(buf) - 1);
> > > 
> > > That looks really confusing to me as we're effectively hiding the real
> > > error that caused us to get here just because we couldn't pretty-print
> > > the error string! In the unlikely case that vsnprintf() fails, I think
> > > we should preserve the original errno and either print a string to say
> > > that the formatting failed _or_ pretty-print the original error with
> > > strerror().
> > 
> > You have a good point.
> > 
> > I'm not exactly sure what you mean by pretty-printing the original error.
> > Do you mean that we should print the message passed to die_perror()? I'm
> > don't know how I can do that reliably. I could use vprintf(), but if
> > vsnprintf() failed, wouldn't that make it likely that vprintf() will also
> > fail?
> > 
> > How about this:
> > 
> > diff --git a/util/util.c b/util/util.c
> > index 0f116e0ac459..4e3704f781e8 100644
> > --- a/util/util.c
> > +++ b/util/util.c
> > @@ -109,8 +109,9 @@ void die_perror(const char *fmt, ...)
> >         va_start(params, fmt);
> >         ret = vsnprintf(buf, sizeof(buf), fmt, params);
> >         if (ret < 0) {
> > -               e = errno;
> > -               strncpy(buf, "vsnprintf", ARRAY_SIZE(buf) - 1);
> > +               fprintf(stderr, "vsnprintf failed with error %d (%s)\n",
> > +                       errno, strerror(errno));
> > +               strncpy(buf, "Error", ARRAY_SIZE(buf) - 1);
> >         }
> >         va_end(params);
> > 
> > but we're still losing the original message.
> 
> Sorry, I think my suggestion to use strerror() was unhelpful. Maybe just
> do something like:
> 
> 	ret = vsnprintf(buf, sizeof(buf), fmt, params);
> 	if (ret < 0) {
> 		perror("vsnprintf");
> 		buf[0] = '\0';
> 	}
> 
> 	...
> 
> 	errno = e;
> 	perror(buf);
> 

Gotcha.

Thanks,
Alex

[PATCH kvmtool 6/6] arm64: Improve KVM_ARM_VCPU_PMU_V3_CTRL diagnostics

[Alexandru Elisei]

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 e15244b51c1f..1f583c16d119 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));
 }
 
 #define SYS_EVENT_SOURCE	"/sys/bus/event_source/devices/"
-- 
2.53.0