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.
>> +
>> + 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
