Re: [PATCH v2] virtio: improve virtqueue mapping error messages

["Daniel P. Berrangé" <berrange@redhat.com>]

On Tue, Sep 23, 2025 at 05:09:25PM +0200, Alessandro Ratti wrote:
> Improve error reporting when virtqueue ring mapping fails by including a
> device identifier in the error message.
> 
> Introduce a helper virtio_get_pretty_dev_name() that returns either:
> 
>  - the device ID, if explicitly provided (e.g. -device ...,id=foo)
>  - the QOM path from qdev_get_dev_path(dev) otherwise
>  - "<unknown device>" as a fallback when no identifier is present
> 
> This makes it easier to identify which device triggered the error in
> multi-device setups or when debugging complex guest configurations.
> 
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/230
> Buglink: https://bugs.launchpad.net/qemu/+bug/1919021
> 
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Alessandro Ratti <alessandro@0x65c.net>
> ---
>  hw/virtio/virtio.c | 46 +++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 43 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 9a81ad912e..f5adc381a4 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -235,6 +235,37 @@ static void virtio_virtqueue_reset_region_cache(struct VirtQueue *vq)
>      }
>  }
>  
> +static const char *virtio_get_pretty_dev_name(VirtIODevice *vdev)

I'd suggest this be  'const char *qdev_get_printable_name(DeviceState *dev)'
and live in  the same header & source files as qdev_get_dev_path.

I used 'printable' rather than 'pretty' as I'm not sure I'd claim
that qdev_get_dev_path() results can be said to be pretty :-)

> +{
> +    DeviceState *dev = DEVICE(vdev);
> +
> +    /*
> +     * Return device ID if explicity set
> +     * (e.g. -device virtio-blk-pci,id=foo)
> +     * This allows users to correlate errors with their custom device
> +     * names.
> +     */
> +    if (dev->id) {
> +        return dev->id;
> +    }
> +    /*
> +     * Fall back to the canonical QOM device path (eg. ID for PCI
> +     * devices).
> +     * This ensures the device is still uniquely and meaningfully
> +     * identified.
> +     */
> +    const char *path = qdev_get_dev_path(dev);
> +    if (path) {
> +        return path;
> +    }
> +
> +    /*
> +     * Final fallback: if all else fails, return a placeholder string.
> +     * This ensures the error message always contains a valid string.
> +     */
> +    return "<unknow device>";

s/unknow/unknown/

> +}
> +
>  void virtio_init_region_cache(VirtIODevice *vdev, int n)
>  {
>      VirtQueue *vq = &vdev->vq[n];
> @@ -256,7 +287,10 @@ void virtio_init_region_cache(VirtIODevice *vdev, int n)
>      len = address_space_cache_init(&new->desc, vdev->dma_as,
>                                     addr, size, packed);
>      if (len < size) {
> -        virtio_error(vdev, "Cannot map desc");
> +        virtio_error(vdev,
> +                "Failed to map descriptor ring for device %s: "
> +                "invalid guest physical address or corrupted queue setup",
> +                virtio_get_pretty_dev_name(vdev));
>          goto err_desc;
>      }
>  
> @@ -264,7 +298,10 @@ void virtio_init_region_cache(VirtIODevice *vdev, int n)
>      len = address_space_cache_init(&new->used, vdev->dma_as,
>                                     vq->vring.used, size, true);
>      if (len < size) {
> -        virtio_error(vdev, "Cannot map used");
> +        virtio_error(vdev,
> +                "Failed to map used ring for device %s: "
> +                "possible guest misconfiguration or insufficient memory",
> +                virtio_get_pretty_dev_name(vdev));
>          goto err_used;
>      }
>  
> @@ -272,7 +309,10 @@ void virtio_init_region_cache(VirtIODevice *vdev, int n)
>      len = address_space_cache_init(&new->avail, vdev->dma_as,
>                                     vq->vring.avail, size, false);
>      if (len < size) {
> -        virtio_error(vdev, "Cannot map avail");
> +        virtio_error(vdev,
> +                "Failed to map avalaible ring for device %s: "
> +                "possible queue misconfiguration or overlapping memory region",
> +                virtio_get_pretty_dev_name(vdev));
>          goto err_avail;
>      }

This part all looks good


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|