On Wed, 27 May 2026 03:23:09 -0700 Matt Evans mattev@meta.com wrote:
Previously, vfio_pci_zap_bars() (and the wrapper vfio_pci_zap_and_down_write_memory_lock()) calls were paired with calls of vfio_pci_dma_buf_move().
This commit replaces them 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.
However, as of "vfio/pci: Convert BAR mmap() to use a DMABUF" the unmap_mapping_range() to zap is entirely redundant for plain vfio-pci, since the DMABUFs used for BAR mappings already zap PTEs when the vfio_pci_dma_buf_move() occurs.
One exception remains as a FIXME: in nvgrace-gpu, some BAR VMAs conditionally use custom vm_ops, which have not moved to be backed by DMABUFs. If these BARs are mmap()ed, the vdev enables the existing behaviour of unmap_mapping_range() for the device fd address space.
Signed-off-by: Matt Evans mattev@meta.com
drivers/vfio/pci/nvgrace-gpu/main.c | 5 +++ drivers/vfio/pci/vfio_pci_config.c | 30 ++++++------- drivers/vfio/pci/vfio_pci_core.c | 66 ++++++++++++++++++++--------- drivers/vfio/pci/vfio_pci_priv.h | 3 +- include/linux/vfio_pci_core.h | 1 + 5 files changed, 66 insertions(+), 39 deletions(-)
diff --git a/drivers/vfio/pci/nvgrace-gpu/main.c b/drivers/vfio/pci/nvgrace-gpu/main.c index 15e2f03c6cd4..cfa649200a7f 100644 --- a/drivers/vfio/pci/nvgrace-gpu/main.c +++ b/drivers/vfio/pci/nvgrace-gpu/main.c @@ -364,6 +364,8 @@ static int nvgrace_gpu_mmap(struct vfio_device *core_vdev, struct nvgrace_gpu_pci_core_device *nvdev = container_of(core_vdev, struct nvgrace_gpu_pci_core_device, core_device.vdev);
- struct vfio_pci_core_device *vdev =
container_of(core_vdev, struct vfio_pci_core_device, vdev);@@ -374,6 +376,9 @@ static int nvgrace_gpu_mmap(struct vfio_device *core_vdev, if (!memregion) return vfio_pci_core_mmap(core_vdev, vma);
- /* Non-DMABUF BAR mappings need an extra zap */
- vdev->bar_needs_zap = true;
This works, but it's subtle, and failing to opt-in is dangerous. The name, to me, also suggests some transient state of the device rather than an ongoing property. What if we used "zap_bars_on_revoke" and made vfio_pci_core_register_device() set this to true when:
if (vdev->vdev.ops->mmap != vfio_pci_core_mmap) vdev->zap_bars_on_revoke = true;
That gives us automatic opt-in to the safe default. The hisi_acc driver .mmap is just a wrapper to vfio_pci_core_mmap(), so it could manually opt-out:
ret = vfio_pci_core_register_device(&hisi_acc_vdev->core_device); if (ret) return ret;
/* mmaps are to the dmabuf, not the region */ hisi_acc_vdev->core_device.zap_bars_on_revoke = false;
We can't have the opt-out in vfio_pci_core_mmap() as drivers like nvgrace-gpu can use the core function for some BARs and their own handling for others. Thanks,
Alex
/* * Request to mmap the BAR. Map to the CPU accessible memory on the * GPU using the memory information gathered from the system ACPI diff --git a/drivers/vfio/pci/vfio_pci_config.c b/drivers/vfio/pci/vfio_pci_config.c index a10ed733f0e3..8bfab0da481c 100644 --- a/drivers/vfio/pci/vfio_pci_config.c +++ b/drivers/vfio/pci/vfio_pci_config.c @@ -590,12 +590,10 @@ static int vfio_basic_config_write(struct vfio_pci_core_device *vdev, int pos, virt_mem = !!(le16_to_cpu(*virt_cmd) & PCI_COMMAND_MEMORY); new_mem = !!(new_cmd & PCI_COMMAND_MEMORY);
if (!new_mem) {vfio_pci_zap_and_down_write_memory_lock(vdev);vfio_pci_dma_buf_move(vdev, true);} else {
if (!new_mem)vfio_pci_lock_zap_revoke_bars(vdev);else down_write(&vdev->memory_lock);
}/* * If the user is writing mem/io enable (new_mem/io) and we @@ -631,7 +629,7 @@ static int vfio_basic_config_write(struct vfio_pci_core_device *vdev, int pos, *virt_cmd |= cpu_to_le16(new_cmd & mask); if (__vfio_pci_memory_enabled(vdev))
vfio_pci_dma_buf_move(vdev, false);
vfio_pci_unrevoke_bars(vdev);@@ -712,16 +710,14 @@ static int __init init_pci_cap_basic_perm(struct perm_bits *perm) static void vfio_lock_and_set_power_state(struct vfio_pci_core_device *vdev, pci_power_t state) {
- if (state >= PCI_D3hot) {
vfio_pci_zap_and_down_write_memory_lock(vdev);vfio_pci_dma_buf_move(vdev, true);- } else {
- if (state >= PCI_D3hot)
vfio_pci_lock_zap_revoke_bars(vdev);- else down_write(&vdev->memory_lock);
- }
vfio_pci_set_power_state(vdev, state); if (__vfio_pci_memory_enabled(vdev))
vfio_pci_dma_buf_move(vdev, false);
vfio_pci_unrevoke_bars(vdev);} @@ -908,11 +904,10 @@ static int vfio_exp_config_write(struct vfio_pci_core_device *vdev, int pos, &cap); if (!ret && (cap & PCI_EXP_DEVCAP_FLR)) {
vfio_pci_zap_and_down_write_memory_lock(vdev);vfio_pci_dma_buf_move(vdev, true);
vfio_pci_lock_zap_revoke_bars(vdev); pci_try_reset_function(vdev->pdev); if (__vfio_pci_memory_enabled(vdev))
vfio_pci_dma_buf_move(vdev, false);
vfio_pci_unrevoke_bars(vdev); up_write(&vdev->memory_lock);@@ -993,11 +988,10 @@ static int vfio_af_config_write(struct vfio_pci_core_device *vdev, int pos, &cap); if (!ret && (cap & PCI_AF_CAP_FLR) && (cap & PCI_AF_CAP_TP)) {
vfio_pci_zap_and_down_write_memory_lock(vdev);vfio_pci_dma_buf_move(vdev, true);
vfio_pci_lock_zap_revoke_bars(vdev); pci_try_reset_function(vdev->pdev); if (__vfio_pci_memory_enabled(vdev))
vfio_pci_dma_buf_move(vdev, false);
vfio_pci_unrevoke_bars(vdev); up_write(&vdev->memory_lock);diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c index c5f934905ce0..cfea59806a4f 100644 --- a/drivers/vfio/pci/vfio_pci_core.c +++ b/drivers/vfio/pci/vfio_pci_core.c @@ -319,8 +319,7 @@ static int vfio_pci_runtime_pm_entry(struct vfio_pci_core_device *vdev, * The vdev power related flags are protected with 'memory_lock' * semaphore. */
- vfio_pci_zap_and_down_write_memory_lock(vdev);
- vfio_pci_dma_buf_move(vdev, true);
- vfio_pci_lock_zap_revoke_bars(vdev);
if (vdev->pm_runtime_engaged) { up_write(&vdev->memory_lock); @@ -406,7 +405,7 @@ static void vfio_pci_runtime_pm_exit(struct vfio_pci_core_device *vdev) down_write(&vdev->memory_lock); __vfio_pci_runtime_pm_exit(vdev); if (__vfio_pci_memory_enabled(vdev))
vfio_pci_dma_buf_move(vdev, false);
vfio_pci_unrevoke_bars(vdev);} @@ -1256,6 +1255,8 @@ static int vfio_pci_ioctl_set_irqs(struct vfio_pci_core_device *vdev, return ret; } +static void vfio_pci_zap_revoke_bars(struct vfio_pci_core_device *vdev);
static int vfio_pci_ioctl_reset(struct vfio_pci_core_device *vdev, void __user *arg) { @@ -1264,7 +1265,7 @@ static int vfio_pci_ioctl_reset(struct vfio_pci_core_device *vdev, if (!vdev->reset_works) return -EINVAL;
- vfio_pci_zap_and_down_write_memory_lock(vdev);
- down_write(&vdev->memory_lock);
/* * This function can be invoked while the power state is non-D0. If @@ -1277,10 +1278,11 @@ static int vfio_pci_ioctl_reset(struct vfio_pci_core_device *vdev, */ vfio_pci_set_power_state(vdev, PCI_D0);
- vfio_pci_dma_buf_move(vdev, true);
- vfio_pci_zap_revoke_bars(vdev);
- ret = pci_try_reset_function(vdev->pdev); if (__vfio_pci_memory_enabled(vdev))
vfio_pci_dma_buf_move(vdev, false);
vfio_pci_unrevoke_bars(vdev);return ret; @@ -1648,20 +1650,44 @@ ssize_t vfio_pci_core_write(struct vfio_device *core_vdev, const char __user *bu } EXPORT_SYMBOL_GPL(vfio_pci_core_write); -static void vfio_pci_zap_bars(struct vfio_pci_core_device *vdev) +static void vfio_pci_zap_revoke_bars(struct vfio_pci_core_device *vdev) {
- struct vfio_device *core_vdev = &vdev->vdev;
- loff_t start = VFIO_PCI_INDEX_TO_OFFSET(VFIO_PCI_BAR0_REGION_INDEX);
- loff_t end = VFIO_PCI_INDEX_TO_OFFSET(VFIO_PCI_ROM_REGION_INDEX);
- loff_t len = end - start;
- lockdep_assert_held_write(&vdev->memory_lock);
- vfio_pci_dma_buf_move(vdev, true);
- unmap_mapping_range(core_vdev->inode->i_mapping, start, len, true);
- /*
* All VFIO PCI BARs are backed by DMABUFs, with the current* exception of the nvgrace-gpu device which uses its own* vm_ops for a subset of BARs. For this, BAR mappings are* still made in the vdev's address_space, and a zap is* required. The tracking is crude, and will (harmlessly)* continue to zap if the special BAR is unmapped, but that* behaviour isn't the common case.** FIXME: This can go away if the special nvgrace-gpu mapping* is converted to use DMABUF.*/- if (vdev->bar_needs_zap) {
struct vfio_device *core_vdev = &vdev->vdev;loff_t start = VFIO_PCI_INDEX_TO_OFFSET(VFIO_PCI_BAR0_REGION_INDEX);loff_t end = VFIO_PCI_INDEX_TO_OFFSET(VFIO_PCI_ROM_REGION_INDEX);loff_t len = end - start;unmap_mapping_range(core_vdev->inode->i_mapping,start, len, true);- }
} -void vfio_pci_zap_and_down_write_memory_lock(struct vfio_pci_core_device *vdev) +void vfio_pci_lock_zap_revoke_bars(struct vfio_pci_core_device *vdev) { down_write(&vdev->memory_lock);
- vfio_pci_zap_bars(vdev);
- vfio_pci_zap_revoke_bars(vdev);
+}
+void vfio_pci_unrevoke_bars(struct vfio_pci_core_device *vdev) +{
- lockdep_assert_held_write(&vdev->memory_lock);
- vfio_pci_dma_buf_move(vdev, false);
} u16 vfio_pci_memory_lock_and_enable(struct vfio_pci_core_device *vdev) @@ -2517,9 +2543,10 @@ static int vfio_pci_dev_set_hot_reset(struct vfio_device_set *dev_set, } /*
* Take the memory write lock for each device and zap BAR* mappings to prevent the user accessing the device while in* reset. Locking multiple devices is prone to deadlock,
* Take the memory write lock for each device and* zap/revoke BAR mappings to prevent the user (or* peers) accessing the device while in reset.* Locking multiple devices is prone to deadlock,*/ if (!down_write_trylock(&vdev->memory_lock)) {
- runaway and unwind if we hit contention.
@@ -2527,8 +2554,7 @@ static int vfio_pci_dev_set_hot_reset(struct vfio_device_set *dev_set, break; }
vfio_pci_dma_buf_move(vdev, true);vfio_pci_zap_bars(vdev);
vfio_pci_zap_revoke_bars(vdev);if (!list_entry_is_head(vdev, @@ -2558,7 +2584,7 @@ static int vfio_pci_dev_set_hot_reset(struct vfio_device_set *dev_set, list_for_each_entry_from_reverse(vdev, &dev_set->device_list, vdev.dev_set_list) { if (vdev->vdev.open_count && __vfio_pci_memory_enabled(vdev))
vfio_pci_dma_buf_move(vdev, false);
vfio_pci_unrevoke_bars(vdev);diff --git a/drivers/vfio/pci/vfio_pci_priv.h b/drivers/vfio/pci/vfio_pci_priv.h index d38e1b98b2e9..10833aabd7fb 100644 --- a/drivers/vfio/pci/vfio_pci_priv.h +++ b/drivers/vfio/pci/vfio_pci_priv.h @@ -83,7 +83,8 @@ void vfio_config_free(struct vfio_pci_core_device *vdev); int vfio_pci_set_power_state(struct vfio_pci_core_device *vdev, pci_power_t state); -void vfio_pci_zap_and_down_write_memory_lock(struct vfio_pci_core_device *vdev); +void vfio_pci_lock_zap_revoke_bars(struct vfio_pci_core_device *vdev); +void vfio_pci_unrevoke_bars(struct vfio_pci_core_device *vdev); u16 vfio_pci_memory_lock_and_enable(struct vfio_pci_core_device *vdev); void vfio_pci_memory_unlock_and_restore(struct vfio_pci_core_device *vdev, u16 cmd); diff --git a/include/linux/vfio_pci_core.h b/include/linux/vfio_pci_core.h index 7accd0eac457..e35e82c24c8c 100644 --- a/include/linux/vfio_pci_core.h +++ b/include/linux/vfio_pci_core.h @@ -127,6 +127,7 @@ struct vfio_pci_core_device { bool needs_pm_restore:1; bool pm_intx_masked:1; bool pm_runtime_engaged:1;
- bool bar_needs_zap:1; struct pci_saved_state *pci_saved_state; struct pci_saved_state *pm_save; int ioeventfds_nr;