Hi Akihiko, > Subject: Re: [PATCH v1 2/7] virtio-gpu: Don't rely on res->blob to identify > blob > resources > > On 2025/10/15 15:11, Akihiko Odaki wrote: > > On 2025/10/15 14:07, Kasireddy, Vivek wrote: > >> Hi Akihiko, > >> > >>> Subject: Re: [PATCH v1 2/7] virtio-gpu: Don't rely on res->blob to > >>> identify blob > >>> resources > >>> > >>>> > >>>>> Subject: Re: [PATCH v1 2/7] virtio-gpu: Don't rely on res->blob to > >>>>> identify > >>> blob > >>>>> resources > >>>>> > >>>>> On 2025/10/04 8:35, Vivek Kasireddy wrote: > >>>>>> The res->blob pointer may not be valid (non-NULL) for some blobs > >>>>>> where the backing storage is not memfd based. 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 think this patch is no longer necessary since now you add code to > >>>>> mmap() VFIO storage with "[PATCH v1 7/7] virtio-gpu-udmabuf: Create > >>>>> dmabuf for blobs associated with VFIO devices". > >>>> Right, but given that mmap() can still fail for various reasons and > >>>> this > >>>> use-case can work as long as dmabuf creation succeeds, I think it makes > >>>> sense to not rely on res->blob to determine if a resource is blob or > >>>> not. > >>> > >>> I think the code will be simpler by making resource creation fail when > >>> mmap() fails, and I am concerned that the guest may mulfunction with > >>> such an incomplete resource. > >> AFAICT, mmap() is a slow, optional path except for the cursor (which > >> needs > >> further improvement). So, failing resource creation when mmap() fails > >> does not seem like a good idea to me given the fact that supporting > >> mmap() > >> is considered optional for dmabuf providers. And, even with vfio, mmap() > >> can be blocked for various reasons by the kernel driver IIUC. > > Reviewing the code again, I don't think mmap() can fail with the current > version of this series. > > udmabuf obviously always supports mmap(). > > For VFIO, checking memory_region_is_ram_device() ensures that VFIO > supports mmap(); memory_region_init_ram_device_ptr() is called from > vfio_region_mmap(), which is only called when VFIO supports mmap(). My point is not whether a dmabuf provider provides support for mmap() or not but about the fact that mmap() can fail (for various reasons because it is not a guarantee) making res->blob NULL. But we are incorrectly using res->blob pointer to determine whether a resource is a blob (and usable) or not which doesn't make sense because even if res->blob is NULL, the resource is still valid and usable via the dmabuf fd, which is the preferred, accelerated path.
Thanks, Vivek > > The use of ram_device needs to be removed if it is really wanted to > create resources with VFIO devices lacking mmap(). This patch is > unnecessary otherwise. > > > > > Being slow doesn't matter, and it is not just for the cursor, but there > > are several other functions modified; I guess the resulting resource > > will be unusable except for accelerations like virgl. > > > >> > >>> > >>> To motivate the proposed patch, there should be a use-case that requires > >>> to have a resource without mmap(), not one that "can work" a resource > >>> without mmap(). It is extraneous complexity otherwise. > >>> > >>> Such a use case should be explained in the patch message and perhaps > >>> also with a comment in the code. The current patch message needs an > >>> update as it sounds like it is unnecessary when theere is code to mmap() > >>> VFIO-based backing storage, which this series has already gained. > >> Although VFIO supports mmap(), it is not guaranteed to work in all cases > >> and with a different dmabuf provider (in the future), it may not be > >> possible > >> at all. > > > > The fact that mmap() is being optional for DMA-BUF and VFIO is > > insufficient, but what matters here is whether a DMA-BUF that lacks > > mmap() is usable for graphics. > > > > Reading the cover letter, I suppose you are importing a mmap-incapable > > DMA-BUF exported by a dGPU, and the imported DMA-BUF is used with virgl > > or something. Explaining the use case will show that there is a mmap- > > incapable DMA-BUF usable for graphics. > > > > Regards, > > Akihiko Odaki
