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? 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.) >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. 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.)
