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 > >>>>> > >>>>>> > >>>>>>> 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. > > Failing to mmap something that is already mmap-ed to another address is > very unrealistic and I can't really think of a possibility of such a > failure aside bugs. The fact that it is already mmap'd to another address would only be true for VFIO devices but as I mentioned previously, we cannot make such assumptions with other (future) dmabuf providers.
> > If this condition (a valid resource with a NULL res->blob) could only > happen due to a bug, then, in my opinion, marking such a resource as > invalid is actually a more defensive and desirable approach. If a core > operation like mmap fails unexpectedly on a resource that should support But mmap is not considered as a core operation for dmabuf. It is considered optional by dmabuf providers. For example, although very unlikely, it might be possible that support for mmap() can be removed from udmabuf driver driver for some reason. And, when this happens, the only adverse effect would be that gl=off would not work, which is not great but definitely not catastrophic. Thanks, Vivek > it, continuing to treat it as functional could lead to worse, > harder-to-debug failures later in the pipeline. And I hope it can be > done without complicating the code (or it even simplifies). > > Regards, > Akihiko Odaki
