Make the error handling more robust in virtio_gpu_init_udmabuf()
by introducing 'Error **' parameter to capture errors and an enum
to categorize Guest and Host 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>
Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
---
hw/display/virtio-gpu-dmabuf.c | 65 +++++++++++++++++++++++++---------
1 file changed, 48 insertions(+), 17 deletions(-)
diff --git a/hw/display/virtio-gpu-dmabuf.c b/hw/display/virtio-gpu-dmabuf.c
index 8d67ef7c2a..fdcf8e53a2 100644
--- a/hw/display/virtio-gpu-dmabuf.c
+++ b/hw/display/virtio-gpu-dmabuf.c
@@ -27,7 +27,17 @@
#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)
+typedef enum VirtIOGPUErrorType {
+ VIRTIO_GPU_NO_ERROR = 0,
+ /* Guest is responsible for this error */
+ VIRTIO_GPU_GUEST_ERROR = 1,
+ /* Host is at fault for this error */
+ VIRTIO_GPU_HOST_ERROR = 2,
+} VirtIOGPUErrorType;
+
+static VirtIOGPUErrorType
+virtio_gpu_create_udmabuf(struct virtio_gpu_simple_resource *res, int *fd,
+ Error **errp)
{
g_autofree struct udmabuf_create_list *list = NULL;
RAMBlock *rb;
@@ -36,7 +46,8 @@ static void virtio_gpu_create_udmabuf(struct virtio_gpu_simple_resource *res)
udmabuf = udmabuf_fd();
if (udmabuf < 0) {
- return;
+ error_setg(errp, "udmabuf device not available");
+ return VIRTIO_GPU_HOST_ERROR;
}
list = g_malloc0(sizeof(struct udmabuf_create_list) +
@@ -45,7 +56,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, "Could not find valid ramblock");
+ return VIRTIO_GPU_GUEST_ERROR;
}
list->list[i].memfd = rb->fd;
@@ -56,22 +68,30 @@ 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 VIRTIO_GPU_GUEST_ERROR;
+ }
+ return VIRTIO_GPU_HOST_ERROR;
}
+ return VIRTIO_GPU_NO_ERROR;
}
-static void virtio_gpu_remap_dmabuf(struct virtio_gpu_simple_resource *res)
+static bool 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));
+ 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");
res->remapped = NULL;
+ return false;
}
+ res->remapped = map;
+ return true;
}
static void virtio_gpu_destroy_dmabuf(struct virtio_gpu_simple_resource *res)
@@ -125,19 +145,30 @@ bool virtio_gpu_have_udmabuf(void)
void virtio_gpu_init_dmabuf(struct virtio_gpu_simple_resource *res)
{
+ Error *local_err = NULL;
+ VirtIOGPUErrorType err;
void *pdata = NULL;
+ int fd;
res->dmabuf_fd = -1;
if (res->iov_cnt == 1 &&
res->iov[0].iov_len < 4096) {
pdata = res->iov[0].iov_base;
} else {
- virtio_gpu_create_udmabuf(res);
- if (res->dmabuf_fd < 0) {
+ err = virtio_gpu_create_udmabuf(res, &fd, &local_err);
+ if (err != VIRTIO_GPU_NO_ERROR) {
+ error_prepend(&local_err, "Cannot create dmabuf: ");
+ if (err == VIRTIO_GPU_GUEST_ERROR) {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "Cannot create dmabuf: incompatible mem\n");
+ }
+ error_report_err(local_err);
return;
}
- virtio_gpu_remap_dmabuf(res);
- if (!res->remapped) {
+
+ res->dmabuf_fd = fd;
+ if (!virtio_gpu_remap_dmabuf(res, &local_err)) {
+ error_report_err(local_err);
return;
}
pdata = res->remapped;
--
2.53.0
On 2026/02/23 17:00, Vivek Kasireddy wrote:
> Make the error handling more robust in virtio_gpu_init_udmabuf()
> by introducing 'Error **' parameter to capture errors and an enum
> to categorize Guest and Host 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>
> Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
> ---
> hw/display/virtio-gpu-dmabuf.c | 65 +++++++++++++++++++++++++---------
> 1 file changed, 48 insertions(+), 17 deletions(-)
>
> diff --git a/hw/display/virtio-gpu-dmabuf.c b/hw/display/virtio-gpu-dmabuf.c
> index 8d67ef7c2a..fdcf8e53a2 100644
> --- a/hw/display/virtio-gpu-dmabuf.c
> +++ b/hw/display/virtio-gpu-dmabuf.c
> @@ -27,7 +27,17 @@
> #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)
> +typedef enum VirtIOGPUErrorType {
> + VIRTIO_GPU_NO_ERROR = 0,
> + /* Guest is responsible for this error */
> + VIRTIO_GPU_GUEST_ERROR = 1,
> + /* Host is at fault for this error */
> + VIRTIO_GPU_HOST_ERROR = 2,
include/qapi/error.h says:
> - Whenever practical, also return a value that indicates success /
> failure. This can make the error checking more concise, and can
> avoid useless error object creation and destruction. Note that
> we still have many functions returning void. We recommend
> • bool-valued functions return true on success / false on failure,
> • pointer-valued functions return non-null / null pointer, and
> • integer-valued functions return non-negative / negative.
Do what it says. Non-negative if *errp is set. Negative otherwise.
> +} VirtIOGPUErrorType;
> +
> +static VirtIOGPUErrorType
> +virtio_gpu_create_udmabuf(struct virtio_gpu_simple_resource *res, int *fd,
> + Error **errp)
> {
> g_autofree struct udmabuf_create_list *list = NULL;
> RAMBlock *rb;
> @@ -36,7 +46,8 @@ static void virtio_gpu_create_udmabuf(struct virtio_gpu_simple_resource *res)
>
> udmabuf = udmabuf_fd();
> if (udmabuf < 0) {
> - return;
> + error_setg(errp, "udmabuf device not available");
> + return VIRTIO_GPU_HOST_ERROR;
> }
>
> list = g_malloc0(sizeof(struct udmabuf_create_list) +
> @@ -45,7 +56,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, "Could not find valid ramblock");
It is a valid ramblock even if rb->fd < 0. It is just "incompatible"
with udmabuf.
> + return VIRTIO_GPU_GUEST_ERROR;
> }
>
> list->list[i].memfd = rb->fd;
> @@ -56,22 +68,30 @@ 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 VIRTIO_GPU_GUEST_ERROR;
> + }
> + return VIRTIO_GPU_HOST_ERROR;
> }
> + return VIRTIO_GPU_NO_ERROR;
> }
>
> -static void virtio_gpu_remap_dmabuf(struct virtio_gpu_simple_resource *res)
> +static bool 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));
> + 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");
> res->remapped = NULL;
> + return false;
> }
> + res->remapped = map;
> + return true;
> }
>
> static void virtio_gpu_destroy_dmabuf(struct virtio_gpu_simple_resource *res)
> @@ -125,19 +145,30 @@ bool virtio_gpu_have_udmabuf(void)
>
> void virtio_gpu_init_dmabuf(struct virtio_gpu_simple_resource *res)
> {
> + Error *local_err = NULL;
> + VirtIOGPUErrorType err;
> void *pdata = NULL;
> + int fd;
>
> res->dmabuf_fd = -1;
> if (res->iov_cnt == 1 &&
> res->iov[0].iov_len < 4096) {
> pdata = res->iov[0].iov_base;
> } else {
> - virtio_gpu_create_udmabuf(res);
> - if (res->dmabuf_fd < 0) {
> + err = virtio_gpu_create_udmabuf(res, &fd, &local_err);
> + if (err != VIRTIO_GPU_NO_ERROR) {
> + error_prepend(&local_err, "Cannot create dmabuf: ");
> + if (err == VIRTIO_GPU_GUEST_ERROR) {
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "Cannot create dmabuf: incompatible mem\n");
s/mem/memory/
The log is not that long so we can have a full correct spelling.
> + }
> + error_report_err(local_err);
This prints a message even for a guest error.
Regards,
Akihiko Odaki
> return;
> }
> - virtio_gpu_remap_dmabuf(res);
> - if (!res->remapped) {
> +
> + res->dmabuf_fd = fd;
> + if (!virtio_gpu_remap_dmabuf(res, &local_err)) {
> + error_report_err(local_err);
> return;
> }
> pdata = res->remapped;
Hi Akihiko,
> Subject: Re: [PATCH v7 06/10] virtio-gpu-dmabuf: Improve error
> handling with 'Error **' and err enum
>
> On 2026/02/23 17:00, Vivek Kasireddy wrote:
> > Make the error handling more robust in virtio_gpu_init_udmabuf()
> > by introducing 'Error **' parameter to capture errors and an enum
> > to categorize Guest and Host 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>
> > Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
> > ---
> > hw/display/virtio-gpu-dmabuf.c | 65 +++++++++++++++++++++++++---
> ------
> > 1 file changed, 48 insertions(+), 17 deletions(-)
> >
> > diff --git a/hw/display/virtio-gpu-dmabuf.c b/hw/display/virtio-gpu-
> dmabuf.c
> > index 8d67ef7c2a..fdcf8e53a2 100644
> > --- a/hw/display/virtio-gpu-dmabuf.c
> > +++ b/hw/display/virtio-gpu-dmabuf.c
> > @@ -27,7 +27,17 @@
> > #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)
> > +typedef enum VirtIOGPUErrorType {
> > + VIRTIO_GPU_NO_ERROR = 0,
> > + /* Guest is responsible for this error */
> > + VIRTIO_GPU_GUEST_ERROR = 1,
> > + /* Host is at fault for this error */
> > + VIRTIO_GPU_HOST_ERROR = 2,
>
> include/qapi/error.h says:
> > - Whenever practical, also return a value that indicates success /
> > failure. This can make the error checking more concise, and can
> > avoid useless error object creation and destruction. Note that
> > we still have many functions returning void. We recommend
> > . bool-valued functions return true on success / false on failure,
> > . pointer-valued functions return non-null / null pointer, and
> > . integer-valued functions return non-negative / negative.
>
> Do what it says. Non-negative if *errp is set. Negative otherwise.
Ok, I'll change the values in the following way:
typedef enum VirtIOGPUErrorType {
VIRTIO_GPU_NO_ERROR = 1,
/* Guest is responsible for this error */
VIRTIO_GPU_GUEST_ERROR = -1,
/* Host is at fault for this error */
VIRTIO_GPU_HOST_ERROR = -2,
} VirtIOGPUErrorType;
>
> > +} VirtIOGPUErrorType;
> > +
> > +static VirtIOGPUErrorType
> > +virtio_gpu_create_udmabuf(struct virtio_gpu_simple_resource *res,
> int *fd,
> > + Error **errp)
> > {
> > g_autofree struct udmabuf_create_list *list = NULL;
> > RAMBlock *rb;
> > @@ -36,7 +46,8 @@ static void virtio_gpu_create_udmabuf(struct
> virtio_gpu_simple_resource *res)
> >
> > udmabuf = udmabuf_fd();
> > if (udmabuf < 0) {
> > - return;
> > + error_setg(errp, "udmabuf device not available");
> > + return VIRTIO_GPU_HOST_ERROR;
> > }
> >
> > list = g_malloc0(sizeof(struct udmabuf_create_list) +
> > @@ -45,7 +56,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, "Could not find valid ramblock");
>
> It is a valid ramblock even if rb->fd < 0. It is just "incompatible"
> with udmabuf.
Does "IOV memory address incompatible with udmabuf " sound better?
>
> > + return VIRTIO_GPU_GUEST_ERROR;
> > }
> >
> > list->list[i].memfd = rb->fd;
> > @@ -56,22 +68,30 @@ 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 VIRTIO_GPU_GUEST_ERROR;
> > + }
> > + return VIRTIO_GPU_HOST_ERROR;
> > }
> > + return VIRTIO_GPU_NO_ERROR;
> > }
> >
> > -static void virtio_gpu_remap_dmabuf(struct
> virtio_gpu_simple_resource *res)
> > +static bool 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));
> > + 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");
> > res->remapped = NULL;
> > + return false;
> > }
> > + res->remapped = map;
> > + return true;
> > }
> >
> > static void virtio_gpu_destroy_dmabuf(struct
> virtio_gpu_simple_resource *res)
> > @@ -125,19 +145,30 @@ bool virtio_gpu_have_udmabuf(void)
> >
> > void virtio_gpu_init_dmabuf(struct virtio_gpu_simple_resource *res)
> > {
> > + Error *local_err = NULL;
> > + VirtIOGPUErrorType err;
> > void *pdata = NULL;
> > + int fd;
> >
> > res->dmabuf_fd = -1;
> > if (res->iov_cnt == 1 &&
> > res->iov[0].iov_len < 4096) {
> > pdata = res->iov[0].iov_base;
> > } else {
> > - virtio_gpu_create_udmabuf(res);
> > - if (res->dmabuf_fd < 0) {
> > + err = virtio_gpu_create_udmabuf(res, &fd, &local_err);
> > + if (err != VIRTIO_GPU_NO_ERROR) {
> > + error_prepend(&local_err, "Cannot create dmabuf: ");
> > + if (err == VIRTIO_GPU_GUEST_ERROR) {
> > + qemu_log_mask(LOG_GUEST_ERROR,
> > + "Cannot create dmabuf: incompatible mem\n");
>
> s/mem/memory/
>
> The log is not that long so we can have a full correct spelling.
It was going over the 80 char limit and I was reluctant to split it
across lines.
>
> > + }
> > + error_report_err(local_err);
>
> This prints a message even for a guest error.
This is intentional. My thinking is that if the user did not enable logging
then it will fail silently in the case of Guest error. So, I thought it makes
sense to print an error regardless of the type of error.
Thanks,
Vivek
>
> Regards,
> Akihiko Odaki
>
> > return;
> > }
> > - virtio_gpu_remap_dmabuf(res);
> > - if (!res->remapped) {
> > +
> > + res->dmabuf_fd = fd;
> > + if (!virtio_gpu_remap_dmabuf(res, &local_err)) {
> > + error_report_err(local_err);
> > return;
> > }
> > pdata = res->remapped;
On 2026/02/25 15:04, Kasireddy, Vivek wrote:
> Hi Akihiko,
>
>> Subject: Re: [PATCH v7 06/10] virtio-gpu-dmabuf: Improve error
>> handling with 'Error **' and err enum
>>
>> On 2026/02/23 17:00, Vivek Kasireddy wrote:
>>> Make the error handling more robust in virtio_gpu_init_udmabuf()
>>> by introducing 'Error **' parameter to capture errors and an enum
>>> to categorize Guest and Host 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>
>>> Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
>>> ---
>>> hw/display/virtio-gpu-dmabuf.c | 65 +++++++++++++++++++++++++---
>> ------
>>> 1 file changed, 48 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/hw/display/virtio-gpu-dmabuf.c b/hw/display/virtio-gpu-
>> dmabuf.c
>>> index 8d67ef7c2a..fdcf8e53a2 100644
>>> --- a/hw/display/virtio-gpu-dmabuf.c
>>> +++ b/hw/display/virtio-gpu-dmabuf.c
>>> @@ -27,7 +27,17 @@
>>> #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)
>>> +typedef enum VirtIOGPUErrorType {
>>> + VIRTIO_GPU_NO_ERROR = 0,
>>> + /* Guest is responsible for this error */
>>> + VIRTIO_GPU_GUEST_ERROR = 1,
>>> + /* Host is at fault for this error */
>>> + VIRTIO_GPU_HOST_ERROR = 2,
>>
>> include/qapi/error.h says:
>> > - Whenever practical, also return a value that indicates success /
>> > failure. This can make the error checking more concise, and can
>> > avoid useless error object creation and destruction. Note that
>> > we still have many functions returning void. We recommend
>> > . bool-valued functions return true on success / false on failure,
>> > . pointer-valued functions return non-null / null pointer, and
>> > . integer-valued functions return non-negative / negative.
>>
>> Do what it says. Non-negative if *errp is set. Negative otherwise.
> Ok, I'll change the values in the following way:
> typedef enum VirtIOGPUErrorType {
> VIRTIO_GPU_NO_ERROR = 1,
> /* Guest is responsible for this error */
> VIRTIO_GPU_GUEST_ERROR = -1,
> /* Host is at fault for this error */
> VIRTIO_GPU_HOST_ERROR = -2,
> } VirtIOGPUErrorType;
>
>>
>>> +} VirtIOGPUErrorType;
>>> +
>>> +static VirtIOGPUErrorType
>>> +virtio_gpu_create_udmabuf(struct virtio_gpu_simple_resource *res,
>> int *fd,
>>> + Error **errp)
>>> {
>>> g_autofree struct udmabuf_create_list *list = NULL;
>>> RAMBlock *rb;
>>> @@ -36,7 +46,8 @@ static void virtio_gpu_create_udmabuf(struct
>> virtio_gpu_simple_resource *res)
>>>
>>> udmabuf = udmabuf_fd();
>>> if (udmabuf < 0) {
>>> - return;
>>> + error_setg(errp, "udmabuf device not available");
>>> + return VIRTIO_GPU_HOST_ERROR;
>>> }
>>>
>>> list = g_malloc0(sizeof(struct udmabuf_create_list) +
>>> @@ -45,7 +56,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, "Could not find valid ramblock");
>>
>> It is a valid ramblock even if rb->fd < 0. It is just "incompatible"
>> with udmabuf.
> Does "IOV memory address incompatible with udmabuf " sound better?
>
>>
>>> + return VIRTIO_GPU_GUEST_ERROR;
>>> }
>>>
>>> list->list[i].memfd = rb->fd;
>>> @@ -56,22 +68,30 @@ 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 VIRTIO_GPU_GUEST_ERROR;
>>> + }
>>> + return VIRTIO_GPU_HOST_ERROR;
>>> }
>>> + return VIRTIO_GPU_NO_ERROR;
>>> }
>>>
>>> -static void virtio_gpu_remap_dmabuf(struct
>> virtio_gpu_simple_resource *res)
>>> +static bool 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));
>>> + 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");
>>> res->remapped = NULL;
>>> + return false;
>>> }
>>> + res->remapped = map;
>>> + return true;
>>> }
>>>
>>> static void virtio_gpu_destroy_dmabuf(struct
>> virtio_gpu_simple_resource *res)
>>> @@ -125,19 +145,30 @@ bool virtio_gpu_have_udmabuf(void)
>>>
>>> void virtio_gpu_init_dmabuf(struct virtio_gpu_simple_resource *res)
>>> {
>>> + Error *local_err = NULL;
>>> + VirtIOGPUErrorType err;
>>> void *pdata = NULL;
>>> + int fd;
>>>
>>> res->dmabuf_fd = -1;
>>> if (res->iov_cnt == 1 &&
>>> res->iov[0].iov_len < 4096) {
>>> pdata = res->iov[0].iov_base;
>>> } else {
>>> - virtio_gpu_create_udmabuf(res);
>>> - if (res->dmabuf_fd < 0) {
>>> + err = virtio_gpu_create_udmabuf(res, &fd, &local_err);
>>> + if (err != VIRTIO_GPU_NO_ERROR) {
>>> + error_prepend(&local_err, "Cannot create dmabuf: ");
>>> + if (err == VIRTIO_GPU_GUEST_ERROR) {
>>> + qemu_log_mask(LOG_GUEST_ERROR,
>>> + "Cannot create dmabuf: incompatible mem\n");
>>
>> s/mem/memory/
>>
>> The log is not that long so we can have a full correct spelling.
> It was going over the 80 char limit and I was reluctant to split it
> across lines.
scripts/checkpatch.pl explicitly allows to have a longer line for string
literals. Priotize the user-visible behavior than keeping the code tidy.
>
>>
>>> + }
>>> + error_report_err(local_err);
>>
>> This prints a message even for a guest error.
> This is intentional. My thinking is that if the user did not enable logging
> then it will fail silently in the case of Guest error. So, I thought it makes
> sense to print an error regardless of the type of error.
Do not locally override the behavior of the codebase-wide
infrastructure. If you think a guest error needs to be logged by
default, you should make a change to the logging infrastructure.
I also think the decision of the logging infrastructure not logging
guest errors by default makes sense since, if you log guest errors by
default, it will allow the guest to spam the output and can cause
security and reliability issues.
Regards,
Akihiko Odaki
>
> Thanks,
> Vivek
>
>>
>> Regards,
>> Akihiko Odaki
>>
>>> return;
>>> }
>>> - virtio_gpu_remap_dmabuf(res);
>>> - if (!res->remapped) {
>>> +
>>> + res->dmabuf_fd = fd;
>>> + if (!virtio_gpu_remap_dmabuf(res, &local_err)) {
>>> + error_report_err(local_err);
>>> return;
>>> }
>>> pdata = res->remapped;
>
Hi Akihiko,
> Subject: Re: [PATCH v7 06/10] virtio-gpu-dmabuf: Improve error handling
> with 'Error **' and err enum
>
> >>
> >> On 2026/02/23 17:00, Vivek Kasireddy wrote:
> >>> Make the error handling more robust in virtio_gpu_init_udmabuf()
> >>> by introducing 'Error **' parameter to capture errors and an enum
> >>> to categorize Guest and Host 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>
> >>> Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
> >>> ---
> >>> hw/display/virtio-gpu-dmabuf.c | 65 +++++++++++++++++++++++++-
> --
> >> ------
> >>> 1 file changed, 48 insertions(+), 17 deletions(-)
> >>>
> >>> diff --git a/hw/display/virtio-gpu-dmabuf.c b/hw/display/virtio-gpu-
> >> dmabuf.c
> >>> index 8d67ef7c2a..fdcf8e53a2 100644
> >>> --- a/hw/display/virtio-gpu-dmabuf.c
> >>> +++ b/hw/display/virtio-gpu-dmabuf.c
> >>> @@ -27,7 +27,17 @@
> >>> #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)
> >>> +typedef enum VirtIOGPUErrorType {
> >>> + VIRTIO_GPU_NO_ERROR = 0,
> >>> + /* Guest is responsible for this error */
> >>> + VIRTIO_GPU_GUEST_ERROR = 1,
> >>> + /* Host is at fault for this error */
> >>> + VIRTIO_GPU_HOST_ERROR = 2,
> >>
> >> include/qapi/error.h says:
> >> > - Whenever practical, also return a value that indicates success /
> >> > failure. This can make the error checking more concise, and can
> >> > avoid useless error object creation and destruction. Note that
> >> > we still have many functions returning void. We recommend
> >> > . bool-valued functions return true on success / false on failure,
> >> > . pointer-valued functions return non-null / null pointer, and
> >> > . integer-valued functions return non-negative / negative.
> >>
> >> Do what it says. Non-negative if *errp is set. Negative otherwise.
> > Ok, I'll change the values in the following way:
> > typedef enum VirtIOGPUErrorType {
> > VIRTIO_GPU_NO_ERROR = 1,
> > /* Guest is responsible for this error */
> > VIRTIO_GPU_GUEST_ERROR = -1,
> > /* Host is at fault for this error */
> > VIRTIO_GPU_HOST_ERROR = -2,
> > } VirtIOGPUErrorType;
> >
> >>
> >>> +} VirtIOGPUErrorType;
> >>> +
> >>> +static VirtIOGPUErrorType
> >>> +virtio_gpu_create_udmabuf(struct virtio_gpu_simple_resource *res,
> >> int *fd,
> >>> + Error **errp)
> >>> {
> >>> g_autofree struct udmabuf_create_list *list = NULL;
> >>> RAMBlock *rb;
> >>> @@ -36,7 +46,8 @@ static void virtio_gpu_create_udmabuf(struct
> >> virtio_gpu_simple_resource *res)
> >>>
> >>> udmabuf = udmabuf_fd();
> >>> if (udmabuf < 0) {
> >>> - return;
> >>> + error_setg(errp, "udmabuf device not available");
> >>> + return VIRTIO_GPU_HOST_ERROR;
> >>> }
> >>>
> >>> list = g_malloc0(sizeof(struct udmabuf_create_list) +
> >>> @@ -45,7 +56,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, "Could not find valid ramblock");
> >>
> >> It is a valid ramblock even if rb->fd < 0. It is just "incompatible"
> >> with udmabuf.
> > Does "IOV memory address incompatible with udmabuf " sound better?
> >
> >>
> >>> + return VIRTIO_GPU_GUEST_ERROR;
> >>> }
> >>>
> >>> list->list[i].memfd = rb->fd;
> >>> @@ -56,22 +68,30 @@ 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 VIRTIO_GPU_GUEST_ERROR;
> >>> + }
> >>> + return VIRTIO_GPU_HOST_ERROR;
> >>> }
> >>> + return VIRTIO_GPU_NO_ERROR;
> >>> }
> >>>
> >>> -static void virtio_gpu_remap_dmabuf(struct
> >> virtio_gpu_simple_resource *res)
> >>> +static bool 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));
> >>> + 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");
> >>> res->remapped = NULL;
> >>> + return false;
> >>> }
> >>> + res->remapped = map;
> >>> + return true;
> >>> }
> >>>
> >>> static void virtio_gpu_destroy_dmabuf(struct
> >> virtio_gpu_simple_resource *res)
> >>> @@ -125,19 +145,30 @@ bool virtio_gpu_have_udmabuf(void)
> >>>
> >>> void virtio_gpu_init_dmabuf(struct virtio_gpu_simple_resource
> *res)
> >>> {
> >>> + Error *local_err = NULL;
> >>> + VirtIOGPUErrorType err;
> >>> void *pdata = NULL;
> >>> + int fd;
> >>>
> >>> res->dmabuf_fd = -1;
> >>> if (res->iov_cnt == 1 &&
> >>> res->iov[0].iov_len < 4096) {
> >>> pdata = res->iov[0].iov_base;
> >>> } else {
> >>> - virtio_gpu_create_udmabuf(res);
> >>> - if (res->dmabuf_fd < 0) {
> >>> + err = virtio_gpu_create_udmabuf(res, &fd, &local_err);
> >>> + if (err != VIRTIO_GPU_NO_ERROR) {
> >>> + error_prepend(&local_err, "Cannot create dmabuf: ");
> >>> + if (err == VIRTIO_GPU_GUEST_ERROR) {
> >>> + qemu_log_mask(LOG_GUEST_ERROR,
> >>> + "Cannot create dmabuf: incompatible mem\n");
> >>
> >> s/mem/memory/
> >>
> >> The log is not that long so we can have a full correct spelling.
> > It was going over the 80 char limit and I was reluctant to split it
> > across lines.
>
> scripts/checkpatch.pl explicitly allows to have a longer line for string
> literals. Priotize the user-visible behavior than keeping the code tidy.
Sounds good. I'll replace mem with memory.
>
> >
> >>
> >>> + }
> >>> + error_report_err(local_err);
> >>
> >> This prints a message even for a guest error.
> > This is intentional. My thinking is that if the user did not enable logging
> > then it will fail silently in the case of Guest error. So, I thought it makes
> > sense to print an error regardless of the type of error.
>
> Do not locally override the behavior of the codebase-wide
> infrastructure. If you think a guest error needs to be logged by
> default, you should make a change to the logging infrastructure.
>
> I also think the decision of the logging infrastructure not logging
> guest errors by default makes sense since, if you log guest errors by
> default, it will allow the guest to spam the output and can cause
> security and reliability issues.
Ok, I'll change it such that error_report_err(local_err) is only done in the
case of Host error. And, in the case of Guest error, I plan to just have
error_free(local_err) in addition to invoking qemu_log_mask().
Thanks,
Vivek
>
> Regards,
> Akihiko Odaki
>
> >
> > Thanks,
> > Vivek
> >
> >>
> >> Regards,
> >> Akihiko Odaki
> >>
> >>> return;
> >>> }
> >>> - virtio_gpu_remap_dmabuf(res);
> >>> - if (!res->remapped) {
> >>> +
> >>> + res->dmabuf_fd = fd;
> >>> + if (!virtio_gpu_remap_dmabuf(res, &local_err)) {
> >>> + error_report_err(local_err);
> >>> return;
> >>> }
> >>> pdata = res->remapped;
> >
© 2016 - 2026 Red Hat, Inc.