The res->blob pointer is only valid for blobs that have their
backing storage in memfd. Therefore, we cannot use it to determine
if a resource is a blob or not. Instead, we could use res->blob_size
to make this determination as it is non-zero for blob resources
regardless of where their backing storage is located.

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>
Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
---
 hw/display/virtio-gpu.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index 0a1a625b0e..2f9133c3b6 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -57,7 +57,7 @@ void virtio_gpu_update_cursor_data(VirtIOGPU *g,
     }
 
     if (res->blob_size) {
-        if (res->blob_size < (s->current_cursor->width *
+        if (!res->blob || res->blob_size < (s->current_cursor->width *
                               s->current_cursor->height * 4)) {
             return;
         }
@@ -144,7 +144,7 @@ virtio_gpu_find_check_resource(VirtIOGPU *g, uint32_t resource_id,
     }
 
     if (require_backing) {
-        if (!res->iov || (!res->image && !res->blob)) {
+        if (!res->iov || (!res->image && !res->blob_size)) {
             qemu_log_mask(LOG_GUEST_ERROR, "%s: no backing storage %d\n",
                           caller, resource_id);
             if (error) {
@@ -444,7 +444,7 @@ static void virtio_gpu_transfer_to_host_2d(VirtIOGPU *g,
 
     res = virtio_gpu_find_check_resource(g, t2d.resource_id, true,
                                          __func__, &cmd->error);
-    if (!res || res->blob) {
+    if (!res || res->blob_size) {
         return;
     }
 
@@ -507,7 +507,7 @@ static void virtio_gpu_resource_flush(VirtIOGPU *g,
         return;
     }
 
-    if (res->blob) {
+    if (res->blob_size) {
         for (i = 0; i < g->parent_obj.conf.max_outputs; i++) {
             scanout = &g->parent_obj.scanout[i];
             if (scanout->resource_id == res->resource_id &&
@@ -538,7 +538,7 @@ static void virtio_gpu_resource_flush(VirtIOGPU *g,
         }
     }
 
-    if (!res->blob &&
+    if (!res->blob_size &&
         (rf.r.x > res->width ||
         rf.r.y > res->height ||
         rf.r.width > res->width ||
@@ -634,7 +634,7 @@ static bool virtio_gpu_do_set_scanout(VirtIOGPU *g,
 
     g->parent_obj.enable = 1;
 
-    if (res->blob) {
+    if (res->blob_size) {
         if (console_has_gl(scanout->con)) {
             if (!virtio_gpu_update_dmabuf(g, scanout_id, res, fb, r)) {
                 virtio_gpu_update_scanout(g, scanout_id, res, fb, r);
@@ -645,13 +645,16 @@ static bool virtio_gpu_do_set_scanout(VirtIOGPU *g,
             return true;
         }
 
+        if (!res->blob) {
+            return false;
+        }
         data = res->blob;
     } else {
         data = (uint8_t *)pixman_image_get_data(res->image);
     }
 
     /* create a surface for this scanout */
-    if ((res->blob && !console_has_gl(scanout->con)) ||
+    if ((res->blob_size && !console_has_gl(scanout->con)) ||
         !scanout->ds ||
         surface_data(scanout->ds) != data + fb->offset ||
         scanout->width != r->width ||
@@ -899,7 +902,7 @@ void virtio_gpu_cleanup_mapping(VirtIOGPU *g,
     g_free(res->addrs);
     res->addrs = NULL;
 
-    if (res->blob) {
+    if (res->blob_size) {
         virtio_gpu_fini_udmabuf(res);
     }
 }
-- 
2.50.1

On 2025/09/03 7:42, Vivek Kasireddy wrote:
> The res->blob pointer is only valid for blobs that have their
> backing storage in memfd. Therefore, we cannot use it to determine
> if a resource is a blob or not. Instead, we could use res->blob_size
> to make this determination as it is non-zero for blob resources
> regardless of where their backing storage is located.

I guess this change needs to be applied before "[RFC 3/6] 
virtio-gpu-udmabuf: Create dmabuf for blobs associated with VFIO 
devices"; without this patch, the "create dmabuf" patch will probably 
create an invalid blob.

> 
> 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>
> Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
> ---
>   hw/display/virtio-gpu.c | 19 +++++++++++--------
>   1 file changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> index 0a1a625b0e..2f9133c3b6 100644
> --- a/hw/display/virtio-gpu.c
> +++ b/hw/display/virtio-gpu.c
> @@ -57,7 +57,7 @@ void virtio_gpu_update_cursor_data(VirtIOGPU *g,
>       }
>   
>       if (res->blob_size) {
> -        if (res->blob_size < (s->current_cursor->width *
> +        if (!res->blob || res->blob_size < (s->current_cursor->width *

I doubt that rejecting a valid blob due to an implementation concern 
(whether the backing storage is in memfd) is tolerated in the specification.

>                                 s->current_cursor->height * 4)) {
>               return;
>           }
> @@ -144,7 +144,7 @@ virtio_gpu_find_check_resource(VirtIOGPU *g, uint32_t resource_id,
>       }
>   
>       if (require_backing) {
> -        if (!res->iov || (!res->image && !res->blob)) {
> +        if (!res->iov || (!res->image && !res->blob_size)) {
>               qemu_log_mask(LOG_GUEST_ERROR, "%s: no backing storage %d\n",
>                             caller, resource_id);
>               if (error) {
> @@ -444,7 +444,7 @@ static void virtio_gpu_transfer_to_host_2d(VirtIOGPU *g,
>   
>       res = virtio_gpu_find_check_resource(g, t2d.resource_id, true,
>                                            __func__, &cmd->error);
> -    if (!res || res->blob) {
> +    if (!res || res->blob_size) {
>           return;
>       }
>   
> @@ -507,7 +507,7 @@ static void virtio_gpu_resource_flush(VirtIOGPU *g,
>           return;
>       }
>   
> -    if (res->blob) {
> +    if (res->blob_size) {
>           for (i = 0; i < g->parent_obj.conf.max_outputs; i++) {
>               scanout = &g->parent_obj.scanout[i];
>               if (scanout->resource_id == res->resource_id &&
> @@ -538,7 +538,7 @@ static void virtio_gpu_resource_flush(VirtIOGPU *g,
>           }
>       }
>   
> -    if (!res->blob &&
> +    if (!res->blob_size &&
>           (rf.r.x > res->width ||
>           rf.r.y > res->height ||
>           rf.r.width > res->width ||
> @@ -634,7 +634,7 @@ static bool virtio_gpu_do_set_scanout(VirtIOGPU *g,
>   
>       g->parent_obj.enable = 1;
>   
> -    if (res->blob) {
> +    if (res->blob_size) {
>           if (console_has_gl(scanout->con)) {
>               if (!virtio_gpu_update_dmabuf(g, scanout_id, res, fb, r)) {
>                   virtio_gpu_update_scanout(g, scanout_id, res, fb, r);
> @@ -645,13 +645,16 @@ static bool virtio_gpu_do_set_scanout(VirtIOGPU *g,
>               return true;
>           }
>   
> +        if (!res->blob) {
> +            return false;
> +        }
>           data = res->blob;
>       } else {
>           data = (uint8_t *)pixman_image_get_data(res->image);
>       }
>   
>       /* create a surface for this scanout */
> -    if ((res->blob && !console_has_gl(scanout->con)) ||
> +    if ((res->blob_size && !console_has_gl(scanout->con)) ||
>           !scanout->ds ||
>           surface_data(scanout->ds) != data + fb->offset ||
>           scanout->width != r->width ||
> @@ -899,7 +902,7 @@ void virtio_gpu_cleanup_mapping(VirtIOGPU *g,
>       g_free(res->addrs);
>       res->addrs = NULL;
>   
> -    if (res->blob) {
> +    if (res->blob_size) {
>           virtio_gpu_fini_udmabuf(res);
>       }
>   }

Hi Akihiko,

> Subject: Re: [RFC 4/6] virtio-gpu: Don't rely on res->blob to identify blob
> resources
> 
> On 2025/09/03 7:42, Vivek Kasireddy wrote:
> > The res->blob pointer is only valid for blobs that have their
> > backing storage in memfd. Therefore, we cannot use it to determine
> > if a resource is a blob or not. Instead, we could use res->blob_size
> > to make this determination as it is non-zero for blob resources
> > regardless of where their backing storage is located.
> 
> I guess this change needs to be applied before "[RFC 3/6]
> virtio-gpu-udmabuf: Create dmabuf for blobs associated with VFIO
> devices"; without this patch, the "create dmabuf" patch will probably
> create an invalid blob.
Ok, makes sense. I'll move it earlier in the patch series.

> 
> >
> > 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>
> > Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
> > ---
> >   hw/display/virtio-gpu.c | 19 +++++++++++--------
> >   1 file changed, 11 insertions(+), 8 deletions(-)
> >
> > diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> > index 0a1a625b0e..2f9133c3b6 100644
> > --- a/hw/display/virtio-gpu.c
> > +++ b/hw/display/virtio-gpu.c
> > @@ -57,7 +57,7 @@ void virtio_gpu_update_cursor_data(VirtIOGPU *g,
> >       }
> >
> >       if (res->blob_size) {
> > -        if (res->blob_size < (s->current_cursor->width *
> > +        if (!res->blob || res->blob_size < (s->current_cursor->width *
> 
> I doubt that rejecting a valid blob due to an implementation concern
> (whether the backing storage is in memfd) is tolerated in the specification.
Are you suggesting that the whole if (res->blob_size < (s->current_cursor->width *...
check needs to be removed? I think it is just a sanity check to ensure that the blob
size is big enough for cursor.

Thanks,
Vivek

> 
> >                                 s->current_cursor->height * 4)) {
> >               return;
> >           }
> > @@ -144,7 +144,7 @@ virtio_gpu_find_check_resource(VirtIOGPU *g,
> uint32_t resource_id,
> >       }
> >
> >       if (require_backing) {
> > -        if (!res->iov || (!res->image && !res->blob)) {
> > +        if (!res->iov || (!res->image && !res->blob_size)) {
> >               qemu_log_mask(LOG_GUEST_ERROR, "%s: no backing storage
> %d\n",
> >                             caller, resource_id);
> >               if (error) {
> > @@ -444,7 +444,7 @@ static void
> virtio_gpu_transfer_to_host_2d(VirtIOGPU *g,
> >
> >       res = virtio_gpu_find_check_resource(g, t2d.resource_id, true,
> >                                            __func__, &cmd->error);
> > -    if (!res || res->blob) {
> > +    if (!res || res->blob_size) {
> >           return;
> >       }
> >
> > @@ -507,7 +507,7 @@ static void virtio_gpu_resource_flush(VirtIOGPU *g,
> >           return;
> >       }
> >
> > -    if (res->blob) {
> > +    if (res->blob_size) {
> >           for (i = 0; i < g->parent_obj.conf.max_outputs; i++) {
> >               scanout = &g->parent_obj.scanout[i];
> >               if (scanout->resource_id == res->resource_id &&
> > @@ -538,7 +538,7 @@ static void virtio_gpu_resource_flush(VirtIOGPU *g,
> >           }
> >       }
> >
> > -    if (!res->blob &&
> > +    if (!res->blob_size &&
> >           (rf.r.x > res->width ||
> >           rf.r.y > res->height ||
> >           rf.r.width > res->width ||
> > @@ -634,7 +634,7 @@ static bool virtio_gpu_do_set_scanout(VirtIOGPU
> *g,
> >
> >       g->parent_obj.enable = 1;
> >
> > -    if (res->blob) {
> > +    if (res->blob_size) {
> >           if (console_has_gl(scanout->con)) {
> >               if (!virtio_gpu_update_dmabuf(g, scanout_id, res, fb, r)) {
> >                   virtio_gpu_update_scanout(g, scanout_id, res, fb, r);
> > @@ -645,13 +645,16 @@ static bool
> virtio_gpu_do_set_scanout(VirtIOGPU *g,
> >               return true;
> >           }
> >
> > +        if (!res->blob) {
> > +            return false;
> > +        }
> >           data = res->blob;
> >       } else {
> >           data = (uint8_t *)pixman_image_get_data(res->image);
> >       }
> >
> >       /* create a surface for this scanout */
> > -    if ((res->blob && !console_has_gl(scanout->con)) ||
> > +    if ((res->blob_size && !console_has_gl(scanout->con)) ||
> >           !scanout->ds ||
> >           surface_data(scanout->ds) != data + fb->offset ||
> >           scanout->width != r->width ||
> > @@ -899,7 +902,7 @@ void virtio_gpu_cleanup_mapping(VirtIOGPU *g,
> >       g_free(res->addrs);
> >       res->addrs = NULL;
> >
> > -    if (res->blob) {
> > +    if (res->blob_size) {
> >           virtio_gpu_fini_udmabuf(res);
> >       }
> >   }

On 2025/09/13 11:48, Kasireddy, Vivek wrote:
> Hi Akihiko,
> 
>> Subject: Re: [RFC 4/6] virtio-gpu: Don't rely on res->blob to identify blob
>> resources
>>
>> On 2025/09/03 7:42, Vivek Kasireddy wrote:
>>> The res->blob pointer is only valid for blobs that have their
>>> backing storage in memfd. Therefore, we cannot use it to determine
>>> if a resource is a blob or not. Instead, we could use res->blob_size
>>> to make this determination as it is non-zero for blob resources
>>> regardless of where their backing storage is located.
>>
>> I guess this change needs to be applied before "[RFC 3/6]
>> virtio-gpu-udmabuf: Create dmabuf for blobs associated with VFIO
>> devices"; without this patch, the "create dmabuf" patch will probably
>> create an invalid blob.
> Ok, makes sense. I'll move it earlier in the patch series.
> 
>>
>>>
>>> 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>
>>> Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
>>> ---
>>>    hw/display/virtio-gpu.c | 19 +++++++++++--------
>>>    1 file changed, 11 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
>>> index 0a1a625b0e..2f9133c3b6 100644
>>> --- a/hw/display/virtio-gpu.c
>>> +++ b/hw/display/virtio-gpu.c
>>> @@ -57,7 +57,7 @@ void virtio_gpu_update_cursor_data(VirtIOGPU *g,
>>>        }
>>>
>>>        if (res->blob_size) {
>>> -        if (res->blob_size < (s->current_cursor->width *
>>> +        if (!res->blob || res->blob_size < (s->current_cursor->width *
>>
>> I doubt that rejecting a valid blob due to an implementation concern
>> (whether the backing storage is in memfd) is tolerated in the specification.
> Are you suggesting that the whole if (res->blob_size < (s->current_cursor->width *...
> check needs to be removed? I think it is just a sanity check to ensure that the blob
> size is big enough for cursor.

I referred to !res->blob, the new condition. It rejects a valid blob 
that is backed by VFIO.

Regards,
Akihiko Odaki

Hi Akihiko,

> Subject: Re: [RFC 4/6] virtio-gpu: Don't rely on res->blob to identify blob
> resources
> 
> On 2025/09/13 11:48, Kasireddy, Vivek wrote:
> > Hi Akihiko,
> >
> >> Subject: Re: [RFC 4/6] virtio-gpu: Don't rely on res->blob to identify blob
> >> resources
> >>
> >> On 2025/09/03 7:42, Vivek Kasireddy wrote:
> >>> The res->blob pointer is only valid for blobs that have their
> >>> backing storage in memfd. Therefore, we cannot use it to determine
> >>> if a resource is a blob or not. Instead, we could use res->blob_size
> >>> to make this determination as it is non-zero for blob resources
> >>> regardless of where their backing storage is located.
> >>
> >> I guess this change needs to be applied before "[RFC 3/6]
> >> virtio-gpu-udmabuf: Create dmabuf for blobs associated with VFIO
> >> devices"; without this patch, the "create dmabuf" patch will probably
> >> create an invalid blob.
> > Ok, makes sense. I'll move it earlier in the patch series.
> >
> >>
> >>>
> >>> 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>
> >>> Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
> >>> ---
> >>>    hw/display/virtio-gpu.c | 19 +++++++++++--------
> >>>    1 file changed, 11 insertions(+), 8 deletions(-)
> >>>
> >>> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> >>> index 0a1a625b0e..2f9133c3b6 100644
> >>> --- a/hw/display/virtio-gpu.c
> >>> +++ b/hw/display/virtio-gpu.c
> >>> @@ -57,7 +57,7 @@ void virtio_gpu_update_cursor_data(VirtIOGPU *g,
> >>>        }
> >>>
> >>>        if (res->blob_size) {
> >>> -        if (res->blob_size < (s->current_cursor->width *
> >>> +        if (!res->blob || res->blob_size < (s->current_cursor->width *
> >>
> >> I doubt that rejecting a valid blob due to an implementation concern
> >> (whether the backing storage is in memfd) is tolerated in the specification.
> > Are you suggesting that the whole if (res->blob_size < (s->current_cursor-
> >width *...
> > check needs to be removed? I think it is just a sanity check to ensure that the
> blob
> > size is big enough for cursor.
> 
> I referred to !res->blob, the new condition. It rejects a valid blob
> that is backed by VFIO.
The problem is that for blobs backed by VFIO, res->blob can be NULL but this function
(virtio_gpu_update_cursor_data) is relying on res->blob always being valid. That's why
the new condition !res->blob is needed here to check the validity of res->blob.

Thanks,
Vivek

> 
> Regards,
> Akihiko Odaki

On 2025/09/15 15:33, Kasireddy, Vivek wrote:
> Hi Akihiko,
> 
>> Subject: Re: [RFC 4/6] virtio-gpu: Don't rely on res->blob to identify blob
>> resources
>>
>> On 2025/09/13 11:48, Kasireddy, Vivek wrote:
>>> Hi Akihiko,
>>>
>>>> Subject: Re: [RFC 4/6] virtio-gpu: Don't rely on res->blob to identify blob
>>>> resources
>>>>
>>>> On 2025/09/03 7:42, Vivek Kasireddy wrote:
>>>>> The res->blob pointer is only valid for blobs that have their
>>>>> backing storage in memfd. Therefore, we cannot use it to determine
>>>>> if a resource is a blob or not. Instead, we could use res->blob_size
>>>>> to make this determination as it is non-zero for blob resources
>>>>> regardless of where their backing storage is located.
>>>>
>>>> I guess this change needs to be applied before "[RFC 3/6]
>>>> virtio-gpu-udmabuf: Create dmabuf for blobs associated with VFIO
>>>> devices"; without this patch, the "create dmabuf" patch will probably
>>>> create an invalid blob.
>>> Ok, makes sense. I'll move it earlier in the patch series.
>>>
>>>>
>>>>>
>>>>> 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>
>>>>> Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
>>>>> ---
>>>>>     hw/display/virtio-gpu.c | 19 +++++++++++--------
>>>>>     1 file changed, 11 insertions(+), 8 deletions(-)
>>>>>
>>>>> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
>>>>> index 0a1a625b0e..2f9133c3b6 100644
>>>>> --- a/hw/display/virtio-gpu.c
>>>>> +++ b/hw/display/virtio-gpu.c
>>>>> @@ -57,7 +57,7 @@ void virtio_gpu_update_cursor_data(VirtIOGPU *g,
>>>>>         }
>>>>>
>>>>>         if (res->blob_size) {
>>>>> -        if (res->blob_size < (s->current_cursor->width *
>>>>> +        if (!res->blob || res->blob_size < (s->current_cursor->width *
>>>>
>>>> I doubt that rejecting a valid blob due to an implementation concern
>>>> (whether the backing storage is in memfd) is tolerated in the specification.
>>> Are you suggesting that the whole if (res->blob_size < (s->current_cursor-
>>> width *...
>>> check needs to be removed? I think it is just a sanity check to ensure that the
>> blob
>>> size is big enough for cursor.
>>
>> I referred to !res->blob, the new condition. It rejects a valid blob
>> that is backed by VFIO.
> The problem is that for blobs backed by VFIO, res->blob can be NULL but this function
> (virtio_gpu_update_cursor_data) is relying on res->blob always being valid. That's why
> the new condition !res->blob is needed here to check the validity of res->blob.

I understand the host-side implementation difficulty to support this 
operation for VFIO, but the guest may not be prepared for the failure of 
the operation so we shouldn't simply reject it.

By the way, perhaps it may be possible to provide res->blob for VFIO. 
Since "[RFC 3/6] virtio-gpu-udmabuf: Create dmabuf for blobs associated 
with VFIO devices" checks memory_region_is_ram_device(), the VFIO 
backend providing the blob should support mmap().

Regards,
Akihiko Odaki

> Subject: Re: [RFC 4/6] virtio-gpu: Don't rely on res->blob to identify blob
> resources
> 
> On 2025/09/15 15:33, Kasireddy, Vivek wrote:
> > Hi Akihiko,
> >
> >> Subject: Re: [RFC 4/6] virtio-gpu: Don't rely on res->blob to identify blob
> >> resources
> >>
> >> On 2025/09/13 11:48, Kasireddy, Vivek wrote:
> >>> Hi Akihiko,
> >>>
> >>>> Subject: Re: [RFC 4/6] virtio-gpu: Don't rely on res->blob to identify
> blob
> >>>> resources
> >>>>
> >>>> On 2025/09/03 7:42, Vivek Kasireddy wrote:
> >>>>> The res->blob pointer is only valid for blobs that have their
> >>>>> backing storage in memfd. Therefore, we cannot use it to determine
> >>>>> if a resource is a blob or not. Instead, we could use res->blob_size
> >>>>> to make this determination as it is non-zero for blob resources
> >>>>> regardless of where their backing storage is located.
> >>>>
> >>>> I guess this change needs to be applied before "[RFC 3/6]
> >>>> virtio-gpu-udmabuf: Create dmabuf for blobs associated with VFIO
> >>>> devices"; without this patch, the "create dmabuf" patch will probably
> >>>> create an invalid blob.
> >>> Ok, makes sense. I'll move it earlier in the patch series.
> >>>
> >>>>
> >>>>>
> >>>>> 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>
> >>>>> Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
> >>>>> ---
> >>>>>     hw/display/virtio-gpu.c | 19 +++++++++++--------
> >>>>>     1 file changed, 11 insertions(+), 8 deletions(-)
> >>>>>
> >>>>> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> >>>>> index 0a1a625b0e..2f9133c3b6 100644
> >>>>> --- a/hw/display/virtio-gpu.c
> >>>>> +++ b/hw/display/virtio-gpu.c
> >>>>> @@ -57,7 +57,7 @@ void virtio_gpu_update_cursor_data(VirtIOGPU
> *g,
> >>>>>         }
> >>>>>
> >>>>>         if (res->blob_size) {
> >>>>> -        if (res->blob_size < (s->current_cursor->width *
> >>>>> +        if (!res->blob || res->blob_size < (s->current_cursor->width *
> >>>>
> >>>> I doubt that rejecting a valid blob due to an implementation concern
> >>>> (whether the backing storage is in memfd) is tolerated in the
> specification.
> >>> Are you suggesting that the whole if (res->blob_size < (s-
> >current_cursor-
> >>> width *...
> >>> check needs to be removed? I think it is just a sanity check to ensure
> that the
> >> blob
> >>> size is big enough for cursor.
> >>
> >> I referred to !res->blob, the new condition. It rejects a valid blob
> >> that is backed by VFIO.
> > The problem is that for blobs backed by VFIO, res->blob can be NULL but
> this function
> > (virtio_gpu_update_cursor_data) is relying on res->blob always being
> valid. That's why
> > the new condition !res->blob is needed here to check the validity of res-
> >blob.
> 
> I understand the host-side implementation difficulty to support this
> operation for VFIO, but the guest may not be prepared for the failure of
> the operation so we shouldn't simply reject it.
I think the worst case scenario here is Guest VM thinks its cursor is being
drawn using the image it provided but nothing gets drawn. I guess we need
to separately figure out if there are any alternate solutions to address this
issue (such as rendering the cursor on the Host side).

> 
> By the way, perhaps it may be possible to provide res->blob for VFIO.
> Since "[RFC 3/6] virtio-gpu-udmabuf: Create dmabuf for blobs associated
> with VFIO devices" checks memory_region_is_ram_device(), the VFIO
> backend providing the blob should support mmap().
The problem is that for dmabuf implementations in the kernel, providing
mmap() support is optional. And, the current vfio-pci implementation (that
provides dmabuf feature) does not support it as there was some pushback.
So, relying on res->blob always being valid is not going to work regardless.

Thanks,
Vivek

> 
> Regards,
> Akihiko Odaki

On 2025/09/16 3:06, Kasireddy, Vivek wrote:
>> Subject: Re: [RFC 4/6] virtio-gpu: Don't rely on res->blob to identify blob
>> resources
>>
>> On 2025/09/15 15:33, Kasireddy, Vivek wrote:
>>> Hi Akihiko,
>>>
>>>> Subject: Re: [RFC 4/6] virtio-gpu: Don't rely on res->blob to identify blob
>>>> resources
>>>>
>>>> On 2025/09/13 11:48, Kasireddy, Vivek wrote:
>>>>> Hi Akihiko,
>>>>>
>>>>>> Subject: Re: [RFC 4/6] virtio-gpu: Don't rely on res->blob to identify
>> blob
>>>>>> resources
>>>>>>
>>>>>> On 2025/09/03 7:42, Vivek Kasireddy wrote:
>>>>>>> The res->blob pointer is only valid for blobs that have their
>>>>>>> backing storage in memfd. Therefore, we cannot use it to determine
>>>>>>> if a resource is a blob or not. Instead, we could use res->blob_size
>>>>>>> to make this determination as it is non-zero for blob resources
>>>>>>> regardless of where their backing storage is located.
>>>>>>
>>>>>> I guess this change needs to be applied before "[RFC 3/6]
>>>>>> virtio-gpu-udmabuf: Create dmabuf for blobs associated with VFIO
>>>>>> devices"; without this patch, the "create dmabuf" patch will probably
>>>>>> create an invalid blob.
>>>>> Ok, makes sense. I'll move it earlier in the patch series.
>>>>>
>>>>>>
>>>>>>>
>>>>>>> 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>
>>>>>>> Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
>>>>>>> ---
>>>>>>>      hw/display/virtio-gpu.c | 19 +++++++++++--------
>>>>>>>      1 file changed, 11 insertions(+), 8 deletions(-)
>>>>>>>
>>>>>>> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
>>>>>>> index 0a1a625b0e..2f9133c3b6 100644
>>>>>>> --- a/hw/display/virtio-gpu.c
>>>>>>> +++ b/hw/display/virtio-gpu.c
>>>>>>> @@ -57,7 +57,7 @@ void virtio_gpu_update_cursor_data(VirtIOGPU
>> *g,
>>>>>>>          }
>>>>>>>
>>>>>>>          if (res->blob_size) {
>>>>>>> -        if (res->blob_size < (s->current_cursor->width *
>>>>>>> +        if (!res->blob || res->blob_size < (s->current_cursor->width *
>>>>>>
>>>>>> I doubt that rejecting a valid blob due to an implementation concern
>>>>>> (whether the backing storage is in memfd) is tolerated in the
>> specification.
>>>>> Are you suggesting that the whole if (res->blob_size < (s-
>>> current_cursor-
>>>>> width *...
>>>>> check needs to be removed? I think it is just a sanity check to ensure
>> that the
>>>> blob
>>>>> size is big enough for cursor.
>>>>
>>>> I referred to !res->blob, the new condition. It rejects a valid blob
>>>> that is backed by VFIO.
>>> The problem is that for blobs backed by VFIO, res->blob can be NULL but
>> this function
>>> (virtio_gpu_update_cursor_data) is relying on res->blob always being
>> valid. That's why
>>> the new condition !res->blob is needed here to check the validity of res-
>>> blob.
>>
>> I understand the host-side implementation difficulty to support this
>> operation for VFIO, but the guest may not be prepared for the failure of
>> the operation so we shouldn't simply reject it.
> I think the worst case scenario here is Guest VM thinks its cursor is being
> drawn using the image it provided but nothing gets drawn. I guess we need
> to separately figure out if there are any alternate solutions to address this
> issue (such as rendering the cursor on the Host side).
> 
>>
>> By the way, perhaps it may be possible to provide res->blob for VFIO.
>> Since "[RFC 3/6] virtio-gpu-udmabuf: Create dmabuf for blobs associated
>> with VFIO devices" checks memory_region_is_ram_device(), the VFIO
>> backend providing the blob should support mmap().
> The problem is that for dmabuf implementations in the kernel, providing
> mmap() support is optional. And, the current vfio-pci implementation (that
> provides dmabuf feature) does not support it as there was some pushback.

Can you provide a reference of a relevant discussion if any?

> So, relying on res->blob always being valid is not going to work regardless.
It is still possible to mmap() using the standard VFIO device API even 
if we cannot mmap() via DMA-BUF.

Regards,
Akihiko Odaki

Hi Akihiko,

> >>>>>> On 2025/09/03 7:42, Vivek Kasireddy wrote:
> >>>>>>> The res->blob pointer is only valid for blobs that have their
> >>>>>>> backing storage in memfd. Therefore, we cannot use it to
> determine
> >>>>>>> if a resource is a blob or not. Instead, we could use res->blob_size
> >>>>>>> to make this determination as it is non-zero for blob resources
> >>>>>>> regardless of where their backing storage is located.
> >>>>>>
> >>>>>> I guess this change needs to be applied before "[RFC 3/6]
> >>>>>> virtio-gpu-udmabuf: Create dmabuf for blobs associated with VFIO
> >>>>>> devices"; without this patch, the "create dmabuf" patch will
> probably
> >>>>>> create an invalid blob.
> >>>>> Ok, makes sense. I'll move it earlier in the patch series.
> >>>>>
> >>>>>>
> >>>>>>>
> >>>>>>> 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>
> >>>>>>> Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
> >>>>>>> ---
> >>>>>>>      hw/display/virtio-gpu.c | 19 +++++++++++--------
> >>>>>>>      1 file changed, 11 insertions(+), 8 deletions(-)
> >>>>>>>
> >>>>>>> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> >>>>>>> index 0a1a625b0e..2f9133c3b6 100644
> >>>>>>> --- a/hw/display/virtio-gpu.c
> >>>>>>> +++ b/hw/display/virtio-gpu.c
> >>>>>>> @@ -57,7 +57,7 @@ void
> virtio_gpu_update_cursor_data(VirtIOGPU
> >> *g,
> >>>>>>>          }
> >>>>>>>
> >>>>>>>          if (res->blob_size) {
> >>>>>>> -        if (res->blob_size < (s->current_cursor->width *
> >>>>>>> +        if (!res->blob || res->blob_size < (s->current_cursor->width *
> >>>>>>
> >>>>>> I doubt that rejecting a valid blob due to an implementation
> concern
> >>>>>> (whether the backing storage is in memfd) is tolerated in the
> >> specification.
> >>>>> Are you suggesting that the whole if (res->blob_size < (s-
> >>> current_cursor-
> >>>>> width *...
> >>>>> check needs to be removed? I think it is just a sanity check to ensure
> >> that the
> >>>> blob
> >>>>> size is big enough for cursor.
> >>>>
> >>>> I referred to !res->blob, the new condition. It rejects a valid blob
> >>>> that is backed by VFIO.
> >>> The problem is that for blobs backed by VFIO, res->blob can be NULL
> but
> >> this function
> >>> (virtio_gpu_update_cursor_data) is relying on res->blob always being
> >> valid. That's why
> >>> the new condition !res->blob is needed here to check the validity of res-
> >>> blob.
> >>
> >> I understand the host-side implementation difficulty to support this
> >> operation for VFIO, but the guest may not be prepared for the failure of
> >> the operation so we shouldn't simply reject it.
> > I think the worst case scenario here is Guest VM thinks its cursor is being
> > drawn using the image it provided but nothing gets drawn. I guess we
> need
> > to separately figure out if there are any alternate solutions to address this
> > issue (such as rendering the cursor on the Host side).
> >
> >>
> >> By the way, perhaps it may be possible to provide res->blob for VFIO.
> >> Since "[RFC 3/6] virtio-gpu-udmabuf: Create dmabuf for blobs associated
> >> with VFIO devices" checks memory_region_is_ram_device(), the VFIO
> >> backend providing the blob should support mmap().
> > The problem is that for dmabuf implementations in the kernel, providing
> > mmap() support is optional. And, the current vfio-pci implementation
> (that
> > provides dmabuf feature) does not support it as there was some
> pushback.
> 
> Can you provide a reference of a relevant discussion if any?
I meant to say that it was deemed undesirable to add mmap support to
vfio-pci dmabuf implementation considering the Confidential computing
and other use-cases. Here are some references:
https://lore.kernel.org/dri-devel/20240501125309.GB941030@nvidia.com/
https://lore.kernel.org/dri-devel/Zjs2bVVxBHEGUhF_@phenom.ffwll.local/
https://lore.kernel.org/dri-devel/20250107142719.179636-5-yilun.xu@linux.intel.com/

> 
> > So, relying on res->blob always being valid is not going to work regardless.
> It is still possible to mmap() using the standard VFIO device API even
> if we cannot mmap() via DMA-BUF.
Ah, right. I had not considered this idea and it seems viable. I'll try to implement
it for the next version of this series.

Thanks,
Vivek

> 
> Regards,
> Akihiko Odaki