On 9/15/26 16:22, Matt Evans wrote: > Hi Christian, > > On 15/09/2026 12:13, Christian König wrote: >> On 9/15/26 09:20, Leon Romanovsky wrote: >>> On Mon, Sep 14, 2026 at 01:13:36PM +0100, Matt Evans wrote: >>>> Hi Leon, >>>> >>>> On 14/09/2026 12:54, Leon Romanovsky wrote: >>>>> On Mon, Sep 14, 2026 at 08:36:47AM -0300, Jason Gunthorpe wrote: >>>>>> On Sun, Sep 13, 2026 at 07:52:44PM +0300, Leon Romanovsky wrote: >>>>>>> On Fri, Sep 11, 2026 at 10:41:57PM +0100, Matt Evans wrote: >>>>>>>> Expand the VFIO DMABUF revocation state to three states: >>>>>>>> Not revoked, temporarily revoked, and permanently revoked. >>>>>>> >>>>>>> The thing is that "temporarily revoked" is actually the standard >>>>>>> invalidate_mappings/move_notify mechanism of DMABUF, which wasn't good >>>>>>> for VFIO. >>>>>> >>>>>> I think temporarily revokes here means it is revoked from a dmabuf >>>>>> perspective >>>>> >>>>> My guess is that this is more of a "change owner" operation than a >>>>> revoke operation. >>>>> >>>>> The main issue here is that we have to guess the semantics instead of >>>>> having a properly named and documented operation. >>>> >>>> Apologies if the cover letter for the series and patch commit message >>>> (which >>>> cover this) are unclear about the motivations and semantics. On the commit >>>> message, can you suggest clarifications: >>>> >>>> "This is useful for lifecycle management, to reclaim VFIO PCI BAR >>>> ranges previously delegated to a subordinate client process: by >>>> revoking, the driver process can ensure that the loaned resources are >>>> made inaccessible when the client is deemed "done". The original >>>> DMABUF is defunct, and BAR resources can then be safely re-exported >>>> for use by new clients." >>>> >>>> Given what I'll explain below, do give suggestions please. There is more >>>> context in the cover letter (the volume of which I didn't think appropriate >>>> for the commit message). >>> >>> 1. Do not mix "driver" and "client" in the same description. For a >>> non-native English speaker, "driver" has a very specific meaning in the >>> context of the Linux kernel. >> >> In the context of DMA-buf it has also proven vital to clearly use the terms >> importer and exporter to describe the different roles a driver can have. >> >> I can't count how often there was confusion because people (me included) >> just used "driver" and it wasn't clear which role was meant. > > :) I see where the quote can be clearer. FWIW the "driver process" was > referring to a userspace driver (which is a legitimate use of the D-word > with VFIO, but still). I'll clarify that, and will indicate this > mechanism is used by userspace to influence the VFIO _exporter_ behaviour. > >>> 2. Explain the lifecycle in the commit message, and why "revoke", which >>> is effectively what the importer does, is not sufficient. >>> >>> 3. The more you put in the cover letter, the less likely people are to >>> read it. >>> >>> 4. Commit messages should describe the patches themselves, since they >>> are what remains visible in the git log, unlike the cover letter. >>> >>> 5. I would call what you describe as "temporarily revoke" is actually >>> "reclaim". >> >> +1 > > I don't follow here, sorry. Would you please elaborate? > > Currently VFIO uses the priv->revoked flag to track whether > it-the-exporter had previously done dma_buf_invalidate_mappings() on a > buffer and is now causing all .attach requests to fail. Do you mean > that (even without this series) you want to call that concept > priv->reclaimed instead?
That sounds like a permanent revoke. > Or do you mean that you don't like the words "temporary"/"permanent" and > are looking for another name for a temporarily unavailable buffer? (If > so, I find "not revoked", "reclaimed", "revoked" much less clear than > not/temp/perm revoked, as such names give no hint as to what to expect. > But I may have misunderstood what you're getting at.) Yeah it's pretty much the naming I would clarify. The original idea of notifying the importer that it need to re-create the mapping was resource reclaim. But when you have some IOCTL or sysfs or whatever to disable a DMA-buf permanently I would call that revoke. I'm still not 100% sure what a temporary revoke should be. If you have some IOCTL/sysfs/whatever to temporary say to importers "You can't use that resource" then that most likely won't fly. > From the importer side, there is no change from this patch: they might > observe an invalidate_mappings() and find attach() of a given buffer now > fails, same as before this patch. This is only about guaranteeing the > impossibility of an importer ever being able to re-attach in future. That sounds reasonable. When a resource becomes unavailable you seriously need completely destroy it, re-create it and then import it again into other drivers which want to use it should it ever become available again. Regards, Christian. > > > Thanks, > > > Matt > > > PS: A hypothetical alternative way of doing what this patch is doing is > instead to have: > > - existing priv->revoked > - new priv->revoked_flag_is_immutable > > Then, the ioctl triggers an invalidate_mappings() for a targeted DMABUF, > sets revoked = true, and revoked_flag_is_immutable = true. The new flag > prevents a future vfio_pci_dma_buf_move(false) from clearing revoked. > For example, a reset doing `move(true); reset; move(false);` can make > all DMABUFs available to attach again, except for those marked > immutably-revoked. (That is equivalent to this new "permanent" state.) > >
