RE: [PATCH v12 09/10] virtio-gpu-dmabuf: Improve error handling with 'Error **' and err enum

["Kasireddy, Vivek" <vivek.kasireddy@intel.com>]

Hi Akihiko,

> Subject: Re: [PATCH v12 09/10] virtio-gpu-dmabuf: Improve error handling
> with 'Error **' and err enum
> 
> >>>>
> >>>> On 3/19/26 06:15, Vivek Kasireddy wrote:
> >>>>> Make the error handling more robust in virtio_gpu_init_udmabuf()
> >>>>> by introducing 'Error **' parameter to capture errors and using
> >>>>> an enum from VFIO to categorize different errors. This allows for
> >>>>> better error reporting and handling of errors from
> >>>>> virtio_gpu_create_udmabuf() and virtio_gpu_remap_dmabuf().
> >>>>>
> >>>>> Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
> >>>>> Cc: Alex Bennée <alex.bennee@linaro.org>
> >>>>> Cc: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
> >>>>> Cc: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> >>>>> Cc: Alex Williamson <alex@shazbot.org>
> >>>>> Cc: Cédric Le Goater <clg@redhat.com>
> >>>>> Reviewed-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
> >>>>> Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
> >>>>> ---
> >>>>>     hw/display/virtio-gpu-dmabuf.c | 67
> +++++++++++++++++++++++--
> >> -----
> >>>> ----
> >>>>>     1 file changed, 45 insertions(+), 22 deletions(-)
> >>>>>
> >>>>> diff --git a/hw/display/virtio-gpu-dmabuf.c b/hw/display/virtio-
> gpu-
> >>>> dmabuf.c
> >>>>> index e35f7714a9..89aa487654 100644
> >>>>> --- a/hw/display/virtio-gpu-dmabuf.c
> >>>>> +++ b/hw/display/virtio-gpu-dmabuf.c
> >>>>> @@ -18,6 +18,7 @@
> >>>>>     #include "ui/console.h"
> >>>>>     #include "hw/virtio/virtio-gpu.h"
> >>>>>     #include "hw/virtio/virtio-gpu-pixman.h"
> >>>>> +#include "hw/vfio/vfio-device.h"
> >>>>>     #include "trace.h"
> >>>>>     #include "system/ramblock.h"
> >>>>>     #include "system/hostmem.h"
> >>>>> @@ -27,16 +28,18 @@
> >>>>>     #include "standard-headers/linux/udmabuf.h"
> >>>>>     #include "standard-headers/drm/drm_fourcc.h"
> >>>>>
> >>>>> -static void virtio_gpu_create_udmabuf(struct
> >>>> virtio_gpu_simple_resource *res)
> >>>>> +static int virtio_gpu_create_udmabuf(struct
> >>>> virtio_gpu_simple_resource *res,
> >>>>> +                                     Error **errp)
> >>>>>     {
> >>>>>         g_autofree struct udmabuf_create_list *list = NULL;
> >>>>>         RAMBlock *rb;
> >>>>>         ram_addr_t offset;
> >>>>> -    int udmabuf, i;
> >>>>> +    int udmabuf, i, fd;
> >>>>>
> >>>>>         udmabuf = udmabuf_fd();
> >>>>>         if (udmabuf < 0) {
> >>>>> -        return;
> >>>>> +        error_setg(errp, "udmabuf device not available or enabled");
> >>>>> +        return VFIO_DMABUF_CREATE_ERR_UNSPEC;
> >>>>
> >>>> The function virtio_gpu_create_udmabuf() is returning
> >> VFIO_DMABUF_*
> >>>> enum
> >>>> values, which is problematic because the function creates a
> >> udmabuf,
> >>>> not
> >>>> a VFIO dmabuf.
> >>>>
> >>>> This creates a layering violation. The virtio-gpu-dmabuf code (which
> >>>> handles both udmabuf and VFIO dmabuf creation) is using error
> >> codes
> >>>> defined in the VFIO-specific header.
> >>
> >> The rationale of using error codes is as follows:
> >>
> >>   > In that case, using it for udmabuf is cheating, but I think it's fine
> >>   > since it's locally-contained in virtio-gpu-dmabuf.c, its intent is
> >>   > still clear, and it has no adverse effect for users (at least there
> >>   > will be no bug).
> >>
> >> https://lore.kernel.org/qemu-devel/becde56b-90bd-40ca-9329-
> >> 0c92b9519a67@rsg.ci.i.u-tokyo.ac.jp/
> >>
> >>>>
> >>>> Please find another solution.
> >>> Other solutions I can think of are either move these error enums into
> >> virtio-gpu
> >>> (and disregard the error return type from vfio) or move them to some
> >> other header
> >>> where they are visible to both virtio-gpu and vfio. I'd like hear
> >> Akihiko's thoughts/
> >>> comments on how to proceed given that he had reviewed virtio-gpu
> >> patches in
> >>> this series.
> >>
> >> Disregarding the error conditions of VFIO is not OK. That can cause a
> >> guest error to be reported as a host error.
> >>
> >> I think the simplest alternative is just to have a duplicate enum for
> >> virtio-gpu.
> > That would address the layering violation but Cedric's other concern is
> > that he believes having errp is sufficient and using these error enums in
> > VFIO would be overkill.
> 
> It would be beneficial to describe the rationale behind the enums; the
> patch message only says "better error reporting", which is quite
> insufficient.
> 
> The rationale is that we need a special handling for the INVALID_IOV
> case. The INVALID_IOV case can happen in two cases:
> - The memory is backed by VFIO. It will result in the INVALID_IOV error
>    for the first attempt to use udmabuf, but this error can be properly
>    recovered by letting the VFIO code to create a DMA-BUF instead.
> - The memory is not backed by memfd nor VFIO. In this case, the error
>    is a guest's fault, so we should:
>    - Use qemu_log_mask(LOG_GUEST_ERROR, ...) instead of
>      error_report_err().
>    - Emit a message useful to diagnose the guest error, which we have
>      discussed in this thread.
> 
> A common pattern is to return -errno to let the caller implement a
> special error handling, but in this case we specifically need to
> distinguish the INVALID_IOV case from the others and these enums are
> more convenient for this.
This definitely helps in understanding the reasoning behind why these error
enums are useful. Before submitting the next version (with the above rationale
added), I'd like to wait and hear Cedric's thoughts/comments on whether
this is acceptable or not.

Thanks,
Vivek
> 
> Regards,
> Akihiko Odaki
> 
> >
> > Thanks,
> > Vivek
> >>
> >>>
> >>>>
> >>>>
> >>>>
> >>>>>         }
> >>>>>
> >>>>>         list = g_malloc0(sizeof(struct udmabuf_create_list) +
> >>>>> @@ -45,7 +48,8 @@ static void virtio_gpu_create_udmabuf(struct
> >>>> virtio_gpu_simple_resource *res)
> >>>>>         for (i = 0; i < res->iov_cnt; i++) {
> >>>>>             rb = qemu_ram_block_from_host(res->iov[i].iov_base, false,
> >>>> &offset);
> >>>>>             if (!rb || rb->fd < 0) {
> >>>>> -            return;
> >>>>> +            error_setg(errp, "IOV memory address incompatible with
> >>>> udmabuf ");
> >>>>> +            return VFIO_DMABUF_CREATE_ERR_INVALID_IOV;
> >>>>>             }
> >>>>>
> >>>>>             list->list[i].memfd  = rb->fd;
> >>>>> @@ -56,22 +60,28 @@ static void
> >> virtio_gpu_create_udmabuf(struct
> >>>> virtio_gpu_simple_resource *res)
> >>>>>         list->count = res->iov_cnt;
> >>>>>         list->flags = UDMABUF_FLAGS_CLOEXEC;
> >>>>>
> >>>>> -    res->dmabuf_fd = ioctl(udmabuf, UDMABUF_CREATE_LIST, list);
> >>>>> -    if (res->dmabuf_fd < 0) {
> >>>>> -        warn_report("%s: UDMABUF_CREATE_LIST: %s", __func__,
> >>>>> -                    strerror(errno));
> >>>>> +    fd = ioctl(udmabuf, UDMABUF_CREATE_LIST, list);
> >>>>> +    if (fd < 0) {
> >>>>> +        error_setg_errno(errp, errno, "UDMABUF_CREATE_LIST: ioctl
> >>>> failed");
> >>>>> +        if (errno == EINVAL || errno == EBADFD) {
> >>>>> +            return VFIO_DMABUF_CREATE_ERR_INVALID_IOV;
> >>>>> +        }
> >>>>> +        return VFIO_DMABUF_CREATE_ERR_UNSPEC;
> >>>>>         }
> >>>>> +    return fd;
> >>>>>     }
> >>>>>
> >>>>> -static void virtio_gpu_remap_dmabuf(struct
> >>>> virtio_gpu_simple_resource *res)
> >>>>> +static void *virtio_gpu_remap_dmabuf(struct
> >>>> virtio_gpu_simple_resource *res,
> >>>>> +                                     Error **errp)
> >>>>>     {
> >>>>> -    res->remapped = mmap(NULL, res->blob_size, PROT_READ,
> >>>>> -                         MAP_SHARED, res->dmabuf_fd, 0);
> >>>>> -    if (res->remapped == MAP_FAILED) {
> >>>>> -        warn_report("%s: dmabuf mmap failed: %s", __func__,
> >>>>> -                    strerror(errno));
> >>>>> -        res->remapped = NULL;
> >>>>> +    void *map;
> >>>>> +
> >>>>> +    map = mmap(NULL, res->blob_size, PROT_READ, MAP_SHARED,
> >>>> res->dmabuf_fd, 0);
> >>>>> +    if (map == MAP_FAILED) {
> >>>>> +        error_setg_errno(errp, errno, "dmabuf mmap failed");
> >>>>> +        return NULL;
> >>>>>         }
> >>>>> +    return map;
> >>>>>     }
> >>>>>
> >>>>>     static void virtio_gpu_destroy_dmabuf(struct
> >>>> virtio_gpu_simple_resource *res)
> >>>>> @@ -125,22 +135,35 @@ bool virtio_gpu_have_udmabuf(void)
> >>>>>
> >>>>>     void virtio_gpu_init_dmabuf(struct virtio_gpu_simple_resource
> >> *res)
> >>>>>     {
> >>>>> +    Error *local_err = NULL;
> >>>>>         void *pdata = NULL;
> >>>>>
> >>>>> -    res->dmabuf_fd = -1;
> >>>>>         if (res->iov_cnt == 1 &&
> >>>>>             res->iov[0].iov_len < 4096) {
> >>>>> +        res->dmabuf_fd = -1;
> >>>>>             pdata = res->iov[0].iov_base;
> >>>>>         } else {
> >>>>> -        virtio_gpu_create_udmabuf(res);
> >>>>> +        res->dmabuf_fd = virtio_gpu_create_udmabuf(res,
> >> &local_err);
> >>>>> +        if (res->dmabuf_fd ==
> >>>> VFIO_DMABUF_CREATE_ERR_INVALID_IOV) {
> >>>>> +            error_free_or_abort(&local_err);
> >>>>
> >>>> Why not report the error in the QEMU log below ?
> >>> I think the idea is that in the case of INVALID_IOV error, it is sufficient
> >>> to just report that the Guest passed in incompatible memory
> >> addresses.
> >>> But I guess we could also just do:
> >>> qemu_log_mask(LOG_GUEST_ERROR, "%s\n",
> >> error_get_pretty(local_err));
> >>
> >> The error message may not be useful. e.g., "UDMABUF_CREATE_LIST:
> >> ioctl
> >> failed" does not tell what error the guest made and how to fix it.
> >>
> >> Regards,
> >> Akihiko Odaki
> >>
> >>>
> >>> Thanks,
> >>> Vivek
> >>>>
> >>>>> +
> >>>>> +            qemu_log_mask(LOG_GUEST_ERROR,
> >>>>> +                          "Cannot create dmabuf: incompatible memory\n");
> >>>>> +            return;
> >>>>> +        } else if (res->dmabuf_fd >= 0) {
> >>>>> +            pdata = virtio_gpu_remap_dmabuf(res, &local_err);
> >>>>> +            if (!pdata) {
> >>>>> +                virtio_gpu_destroy_dmabuf(res);
> >>>>> +            }
> >>>>> +        } else {
> >>>>> +            res->dmabuf_fd = -1;
> >>>>> +        }
> >>>>> +
> >>>>>             if (res->dmabuf_fd < 0) {
> >>>>> +            error_report_err(local_err);
> >>>>>                 return;
> >>>>>             }
> >>>>> -        virtio_gpu_remap_dmabuf(res);
> >>>>> -        if (!res->remapped) {
> >>>>> -            return;
> >>>>> -        }
> >>>>> -        pdata = res->remapped;
> >>>>> +        res->remapped = pdata;
> >>>>>         }
> >>>>>
> >>>>>         res->blob = pdata;
> >>>
> >