Re: [PATCH v3 6/9] vfio/pci: Clean up BAR zap and revocation

[Matt Evans]

Hi Kevin,

On 16/06/2026 10:18, Tian, Kevin wrote:
>> From: Matt Evans <matt@xxxxxxxxxx>
>> Sent: Wednesday, June 10, 2026 11:43 PM
>>
>> Previously, vfio_pci_zap_bars() (and the wrapper
>> vfio_pci_zap_and_down_write_memory_lock()) calls were paired with
>> calls to vfio_pci_dma_buf_move().
>>
>> This commit replaces them with a unified new function,
>> vfio_pci_zap_revoke_bars() containing both the vfio_pci_dma_buf_move()
>> and the unmap_mapping_range(), making it harder for callers to omit
>> one.  It adds a wrapper, vfio_pci_lock_zap_revoke_bars(), which takes
>> the write memory_lock before zapping, and adds a new
>> vfio_pci_unrevoke_bars() for the re-enable path.
> 
> It's unusual to have three verbs (lock/zap/revoke) in one function name.
> 
> I wonder whether it's simpler to have:
> 	vfio_pci_zap_bars_locked() 	// caller already holds the lock
> 	vfio_pci_zap_bars()
> 
> 'revoke' is just a side-effect of 'zap', not necessarily to highlight it in
> the name.

(Just found this one unacknowledged, apologies.)  If you reckon it's a
handful, sure I can shorten them.

As it already has ..._unrevoke_bars(), it makes sense to use
..._revoke_bars() and ..._lock_revoke_bars().  IMHO the zap is a
secondary effect, and "revoke the BARs" means to make them inaccessible
from both DMA and CPU.

I don't want to go down the path of _locked() though right now; I just
want to tidy the current pattern without pulling new duties up to the
call sites.

>> As of "vfio/pci: Convert BAR mmap() to use a DMABUF", the
>> unmap_mapping_range() to zap is no longer performed for vfio-pci since
>> the DMABUFs used for BAR mappings already zap PTEs when the
>> vfio_pci_dma_buf_move() occurs.
>>
>> However, it must be assumed that VFIO drivers which override the .mmap
>> op could create mappings _not_ backed by DMABUFs.  So, the zap is
>> still performed on revoke if .mmap is overridden, using a new
>> zap_bars_on_revoke flag.  A driver can explicitly opt out; the flag is
>> cleared by the hisi_acc_vfio_pci driver, since its .mmap just wraps
>> vfio_pci_core_mmap() and so still uses DMABUFs.
> 
> the cost of unmap_mapping_range() is trivial when there is no mmap
> on the device fd.
> 
> so it could be simpler by always doing:
> 
> 	vfio_pci_dma_buf_move();
> 	unmap_mapping_range();
> 
> and remove the flag.

I did consider this.  I do agree it's cheap, but I still prefer to avoid
unnecessary activities (just to decouple and insulate against things
somehow changing in future).  From previous postings, others didn't seem
to mind the flag approach, at least.

Thanks,


Matt