> 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 <[email protected]> > >>>>> Cc: Alex Bennée <[email protected]> > >>>>> Cc: Akihiko Odaki <[email protected]> > >>>>> Cc: Dmitry Osipenko <[email protected]> > >>>>> Signed-off-by: Vivek Kasireddy <[email protected]> > >>>>> --- > >>>>> 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
