On Wed, Jun 10, 2026 at 04:43:20PM +0100, Matt Evans wrote:
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.
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.
Signed-off-by: Matt Evans matt@ozlabs.org
.../vfio/pci/hisilicon/hisi_acc_vfio_pci.c | 8 +++ drivers/vfio/pci/vfio_pci_config.c | 30 ++++---- drivers/vfio/pci/vfio_pci_core.c | 70 +++++++++++++------ drivers/vfio/pci/vfio_pci_priv.h | 3 +- include/linux/vfio_pci_core.h | 1 + 5 files changed, 73 insertions(+), 39 deletions(-)
diff --git a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c index 86362ec424a5..51990f6d66d5 100644 --- a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c +++ b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c @@ -1692,6 +1692,14 @@ static int hisi_acc_vfio_pci_probe(struct pci_dev *pdev, const struct pci_device if (ret) goto out_put_vdev;
- /*
* hisi_acc_vfio_pci_mmap() calls down to* vfio_pci_core_mmap(), so BAR mappings are still* DMABUF-backed. They don't require a zap on revoke, so opt* out:*/- hisi_acc_vdev->core_device.zap_bars_on_revoke = false;
This seems to be happening after we vfio_pci_core_register_device, which could be slightly problematic if another device in the same group races to trigger a hot reset before we can set this to false. Could we initialize this flag before registration instead?
hisi_acc_vfio_debug_init(hisi_acc_vdev); return 0; 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 f9636d8f9e2a..5ea0bd4e7876 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);
I'm wondering if this change in behavior is correct? BEFORE this patch the sequence was:
1. zap vma mappings 2. Enter D0
After this patch the sequence becomes
1. Take the lock 2. Enter D0 3. zap vma mappings
My worry is if user-space accesses a BAR *during* the transition to D0, it could crash since the mappings still exist during the transition?
The old code is immune to it because it removed user-mappings first.
Following the discussion from v1 regarding the ordering of vfio_pci_dma_buf_move() and the D0 transition.. while it makes sense to perform the DMABUF revocation/move after the hardware is in D0.. I'm not too confident about moving zap after D0 :/
I mean, sure, the user would just see all Fs on a read and writes will be dropped silently until we are in D0.. but the behaviour before this change was that the user access will fault and hang on the memory_lock instead which ensures that the user observes a consistent dev state..
- 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,37 @@ ssize_t vfio_pci_core_write(struct vfio_device *core_vdev, const char __user *bu }
Thanks, Praan