Hi Kevin,
Digging this one up again, On 17/06/2026 17:08, Matt Evans wrote: > Hi Kevin, > > On 16/06/2026 10:26, Tian, Kevin wrote: >>> From: Matt Evans <[email protected]> >>> Sent: Wednesday, June 10, 2026 11:43 PM >>> >>> Expand the VFIO DMABUF revocation state to three states: >>> Not revoked, temporarily revoked, and permanently revoked. >>> >>> The first two are for existing transient revocation, e.g. across a >>> function reset, and the DMABUF is put into the last in response to a >>> new VFIO feature VFIO_DEVICE_FEATURE_DMA_BUF. >> >> VFIO_DEVICE_FEATURE_DMA_BUF_REVOKE >> >>> >>> VFIO_DEVICE_FEATURE_DMA_BUF passes a DMABUF by fd and requests that >>> the DMABUF is permanently revoked. On success, it's guaranteed that >> >> ditto > > Argh, thanks for catching these. Fixed. > >>> the buffer can never be imported/attached/mmap()ed in future, that >>> dynamic imports have been cleanly detached, and that all mappings have >>> been made inaccessible/PTEs zapped. >>> >>> This is useful for lifecycle management, to reclaim VFIO PCI BAR >>> ranges previously delegated to a subordinate client process: The >>> driver process can ensure that the loaned resources are revoked when >>> the client is deemed "done", and exported ranges can be safely re-used >>> elsewhere. >> >> probably clarify that re-use by creating a new dmabuf fd as the original >> one is essentially zombie now. > > Reworded this, plus added a note re the change below. > >>> >>> +/* Set the DMABUF's revocation status (OK or temporarily/permanently >>> revoked) */ >>> +static void vfio_pci_dma_buf_set_status(struct vfio_pci_dma_buf *priv, >>> + enum vfio_pci_dma_buf_status >>> new_status) >>> +{ >>> + bool was_revoked; >>> + >>> + lockdep_assert_held_write(&priv->vdev->memory_lock); >>> + >>> + if (priv->status == VFIO_PCI_DMABUF_PERM_REVOKED || >>> + priv->status == new_status) { >>> + return; >>> + } >> >> the only interface to request PERM_REVOKED is via the new ioctl. >> >> vfio_pci_core_feature_dma_buf_revoke() returns -EBADFD if >> it's already in PERM_REVOKED. >> >> so this check shouldn't be reached, suggesting a warning. > > Good point, both any change to PERM_REVOKED or a double-set of the same > state indicate some caller has gone wrong. Added a warning. Well, after the D0/D3 reset thread, I noticed while testing that a double-revoke will naturally happen when cleaning up a buffer that was already revoked by a device having previously transitioned to D3. Similarly, cleaning up a buffer that was explicitly (permanently) revoked leads to an attempt to set TEMP whilst PERM, and this is OK too. So the only "surprising" case is a buffer already in the PERM_REVOKED state getting a second PERM_REVOKED (which is weeded out in the caller as you point out). Any new caller asking for PERM_REVOKED repeatedly is odd, but still gets what it wants. I really don't think a warning is warranted just for that (it's safe either way). Sending this explanation separately, so you are not too disappointed if v4 reverts to this existing condition above... :) Thanks, Matt > >>> + >>> + dma_buf_invalidate_mappings(priv->dmabuf); >>> + dma_resv_wait_timeout(priv->dmabuf->resv, >>> + DMA_RESV_USAGE_BOOKKEEP, false, >>> + MAX_SCHEDULE_TIMEOUT); >>> + dma_resv_unlock(priv->dmabuf->resv); >> >> It's existing code but while at it let's make above conditional to >> the actual revoke path. for unrevoked it's not required given the >> previous revoke already cleans up everything. > > I noticed this too though I was consciously trying to keep the diff as > small as possible. But with this feedback from both you and Praan, I'll > move this. It's still pretty readable before/after. > >> otherwise, >> >> Reviewed-by: Kevin Tian <[email protected]> > > > Thank you. > > > Matt >
