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 <[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. > > 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/[email protected]/ https://lore.kernel.org/dri-devel/[email protected]/ https://lore.kernel.org/dri-devel/[email protected]/
> > > 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
