[PATCH 0/2] vfio/pci: Request resources and map BARs at enable time

[PATCH 0/2] vfio/pci: Request resources and map BARs at enable time

[Matt Evans]

Hi,

This is a replacement for these patches posted to set up the
VFIO barmap in the DMABUF export path, and add serialisation:

 https://lore.kernel.org/kvm/20260415181423.1008458-1-mattev@meta.com

Those patches were motivated by the DMABUF export originally missing a
call to vfio_pci_core_setup_barmap() and so possibly exporting a range
for which resources weren't yet requested.

Responses in the thread indicated there wasn't a strong historical
reason to require the bar-mapping to be performed on-demand at BAR
reference time.  It's much simpler to move this to
vfio_pci_core_enable(), and that then avoids having to deal with
concurrent lazy requests.

Now VFIO drivers requiring the BAR resources (mmap(), DMABUF export,
etc.) don't need to do anything else, and there's nothing to forget in
future code.

The first patch sets up the barmap (requesting PCI resources and
ioremap() of the BARs) in vfio_pci_core_enable().  It can be
considered a bugfix for the VFIO DMABUF path.

The second patch removes calls to vfio_pci_core_setup_barmap(), which
were left untouched but harmless (just a stub) in the first patch.

However, vfio_pci_core_setup_barmap() did two things, requesting
resources and ioremap()ing them.  Some callers relied on it to perform
the ioremap() just before an in-kernel access to the BAR (e.g. the BAR
R/W routines, nvgrace-gpu).  These calls are replaced by a cheap check
that the required BAR has indeed been mapped, keeping the same
behaviour.

In future we could reconsider performing all the ioremap()s at startup
time, since a lot of VFIO users will never access BARs in-kernel and
won't need them.  Mapping huge BARs will cost some time & memory, and
if necessary this could be reduced by doing the ioremap() portion on
demand.


Matt Evans (2):
  vfio/pci: Set up barmap in vfio_pci_core_enable()
  vfio/pci: Remove vfio_pci_core_setup_barmap()

 drivers/vfio/pci/nvgrace-gpu/main.c | 11 ++---
 drivers/vfio/pci/vfio_pci_core.c    | 71 ++++++++++++++++++++++-------
 drivers/vfio/pci/vfio_pci_rdwr.c    | 35 ++------------
 drivers/vfio/pci/virtio/legacy_io.c |  5 +-
 4 files changed, 65 insertions(+), 57 deletions(-)

-- 
2.47.3

[PATCH 1/2] vfio/pci: Set up barmap in vfio_pci_core_enable()

[Matt Evans]

The previous lazy-setup of the barmaps provided opportunities to
forget to do it (for example, DMABUF export).  Since all users will
ultimately require BAR resources to have been requested, request them
in vfio_pci_core_enable.

Existing calls to vfio_pci_core_setup_barmap() are now benign, but
remain because some callers use it to validate a BAR index.  This
fixes at least the case where DMABUF export could succeed before
resources were requested.

This keeps resource request and ioremap() done at the same time, but
in future the ioremap() could be done on-demand (not all VFIO users
need it).

Fixes: 7f5764e179c6 ("vfio: use vfio_pci_core_setup_barmap to map bar in mmap")
Fixes: 0d77ed3589ac0 ("vfio/pci: Pull BAR mapping setup from read-write path")
Signed-off-by: Matt Evans <mattev@meta.com>
---
 drivers/vfio/pci/vfio_pci_core.c | 63 +++++++++++++++++++++++++++-----
 drivers/vfio/pci/vfio_pci_rdwr.c | 25 +++----------
 2 files changed, 60 insertions(+), 28 deletions(-)

diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
index 3f8d093aacf8..4a314213f3ae 100644
--- a/drivers/vfio/pci/vfio_pci_core.c
+++ b/drivers/vfio/pci/vfio_pci_core.c
@@ -482,6 +482,55 @@ static int vfio_pci_core_runtime_resume(struct device *dev)
 }
 #endif /* CONFIG_PM */
 
+static void __vfio_pci_core_unmap_bars(struct vfio_pci_core_device *vdev)
+{
+	struct pci_dev *pdev = vdev->pdev;
+	int i;
+
+	for (i = 0; i < PCI_STD_NUM_BARS; i++) {
+		int bar = i + PCI_STD_RESOURCES;
+
+		if (!vdev->barmap[i])
+			continue;
+		pci_iounmap(pdev, vdev->barmap[bar]);
+		pci_release_selected_regions(pdev, 1 << bar);
+		vdev->barmap[bar] = NULL;
+	}
+}
+
+static int __vfio_pci_core_map_bars(struct vfio_pci_core_device *vdev)
+{
+	struct pci_dev *pdev = vdev->pdev;
+	int ret = 0;
+	int i;
+
+	/* Eager-request BAR resources (and iomap) */
+	for (i = 0; i < PCI_STD_NUM_BARS; i++) {
+		int bar = i + PCI_STD_RESOURCES;
+		void __iomem *io;
+
+		if (pci_resource_len(pdev, i) == 0)
+			continue;
+
+		ret = pci_request_selected_regions(pdev, 1 << bar, "vfio");
+		if (ret)
+			goto err_free_barmap;
+
+		io = pci_iomap(pdev, bar, 0);
+		if (!io) {
+			pci_release_selected_regions(pdev, 1 << bar);
+			ret = -ENOMEM;
+			goto err_free_barmap;
+		}
+		vdev->barmap[bar] = io;
+	}
+	return 0;
+
+err_free_barmap:
+	__vfio_pci_core_unmap_bars(vdev);
+	return ret;
+}
+
 /*
  * The pci-driver core runtime PM routines always save the device state
  * before going into suspended state. If the device is going into low power
@@ -568,6 +617,9 @@ int vfio_pci_core_enable(struct vfio_pci_core_device *vdev)
 	if (!vfio_vga_disabled() && vfio_pci_is_vga(pdev))
 		vdev->has_vga = true;
 
+	ret = __vfio_pci_core_map_bars(vdev);
+	if (ret)
+		goto out_free_zdev;
 
 	return 0;
 
@@ -591,7 +643,7 @@ void vfio_pci_core_disable(struct vfio_pci_core_device *vdev)
 	struct pci_dev *pdev = vdev->pdev;
 	struct vfio_pci_dummy_resource *dummy_res, *tmp;
 	struct vfio_pci_ioeventfd *ioeventfd, *ioeventfd_tmp;
-	int i, bar;
+	int i;
 
 	/* For needs_reset */
 	lockdep_assert_held(&vdev->vdev.dev_set->lock);
@@ -646,14 +698,7 @@ void vfio_pci_core_disable(struct vfio_pci_core_device *vdev)
 
 	vfio_config_free(vdev);
 
-	for (i = 0; i < PCI_STD_NUM_BARS; i++) {
-		bar = i + PCI_STD_RESOURCES;
-		if (!vdev->barmap[bar])
-			continue;
-		pci_iounmap(pdev, vdev->barmap[bar]);
-		pci_release_selected_regions(pdev, 1 << bar);
-		vdev->barmap[bar] = NULL;
-	}
+	__vfio_pci_core_unmap_bars(vdev);
 
 	list_for_each_entry_safe(dummy_res, tmp,
 				 &vdev->dummy_resources_list, res_next) {
diff --git a/drivers/vfio/pci/vfio_pci_rdwr.c b/drivers/vfio/pci/vfio_pci_rdwr.c
index 4251ee03e146..d1386a31a3a3 100644
--- a/drivers/vfio/pci/vfio_pci_rdwr.c
+++ b/drivers/vfio/pci/vfio_pci_rdwr.c
@@ -200,26 +200,13 @@ EXPORT_SYMBOL_GPL(vfio_pci_core_do_io_rw);
 
 int vfio_pci_core_setup_barmap(struct vfio_pci_core_device *vdev, int bar)
 {
-	struct pci_dev *pdev = vdev->pdev;
-	int ret;
-	void __iomem *io;
-
-	if (vdev->barmap[bar])
-		return 0;
-
-	ret = pci_request_selected_regions(pdev, 1 << bar, "vfio");
-	if (ret)
-		return ret;
-
-	io = pci_iomap(pdev, bar, 0);
-	if (!io) {
-		pci_release_selected_regions(pdev, 1 << bar);
-		return -ENOMEM;
-	}
-
-	vdev->barmap[bar] = io;
+	/*
+	 * The barmap is now always set up in vfio_pci_core_enable().
+	 * Some legacy callers use this function to check if the BAR
+	 * is legitimate, so maintain that:
+	 */
 
-	return 0;
+	return vdev->barmap[bar] ? 0 : -EBUSY;
 }
 EXPORT_SYMBOL_GPL(vfio_pci_core_setup_barmap);
 
-- 
2.47.3

[PATCH 2/2] vfio/pci: Remove vfio_pci_core_setup_barmap()

[Matt Evans]

Since "vfio/pci: Set up barmap in vfio_pci_core_enable()", the BAR
mapping and resource acquisition are done at setup time, and most uses
of vfio_pci_core_setup_barmap() are now unnecessary.  Remove the
callers, and the function.

Some callers used it to also test if a mapping is present before a
direct access, and these places now do a simple test of
vdev->barmap[].

Signed-off-by: Matt Evans <mattev@meta.com>
---
 drivers/vfio/pci/nvgrace-gpu/main.c | 11 +++++------
 drivers/vfio/pci/vfio_pci_core.c    |  8 --------
 drivers/vfio/pci/vfio_pci_rdwr.c    | 22 ++++------------------
 drivers/vfio/pci/virtio/legacy_io.c |  5 ++---
 4 files changed, 11 insertions(+), 35 deletions(-)

diff --git a/drivers/vfio/pci/nvgrace-gpu/main.c b/drivers/vfio/pci/nvgrace-gpu/main.c
index fa056b69f899..3712f6b54d46 100644
--- a/drivers/vfio/pci/nvgrace-gpu/main.c
+++ b/drivers/vfio/pci/nvgrace-gpu/main.c
@@ -184,14 +184,13 @@ static int nvgrace_gpu_open_device(struct vfio_device *core_vdev)
 
 	/*
 	 * GPU readiness is checked by reading the BAR0 registers.
-	 *
-	 * ioremap BAR0 to ensure that the BAR0 mapping is present before
-	 * register reads on first fault before establishing any GPU
-	 * memory mapping.
+	 * BARs should have been mapped at device enable, but it's
+	 * good to assert they're present before the access:
 	 */
-	ret = vfio_pci_core_setup_barmap(vdev, 0);
-	if (ret)
+	if (!vdev->barmap[0]) {
+		ret = -EINVAL;
 		goto error_exit;
+	}
 
 	if (nvdev->resmem.memlength) {
 		ret = nvgrace_gpu_vfio_pci_register_pfn_range(core_vdev, &nvdev->resmem);
diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
index 4a314213f3ae..228d92ce61d1 100644
--- a/drivers/vfio/pci/vfio_pci_core.c
+++ b/drivers/vfio/pci/vfio_pci_core.c
@@ -1805,14 +1805,6 @@ int vfio_pci_core_mmap(struct vfio_device *core_vdev, struct vm_area_struct *vma
 	if (req_start + req_len > phys_len)
 		return -EINVAL;
 
-	/*
-	 * Even though we don't make use of the barmap for the mmap,
-	 * we need to request the region and the barmap tracks that.
-	 */
-	ret = vfio_pci_core_setup_barmap(vdev, index);
-	if (ret)
-		return ret;
-
 	vma->vm_private_data = vdev;
 	vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
 	vma->vm_page_prot = pgprot_decrypted(vma->vm_page_prot);
diff --git a/drivers/vfio/pci/vfio_pci_rdwr.c b/drivers/vfio/pci/vfio_pci_rdwr.c
index d1386a31a3a3..21e19ed48bf5 100644
--- a/drivers/vfio/pci/vfio_pci_rdwr.c
+++ b/drivers/vfio/pci/vfio_pci_rdwr.c
@@ -198,18 +198,6 @@ ssize_t vfio_pci_core_do_io_rw(struct vfio_pci_core_device *vdev, bool test_mem,
 }
 EXPORT_SYMBOL_GPL(vfio_pci_core_do_io_rw);
 
-int vfio_pci_core_setup_barmap(struct vfio_pci_core_device *vdev, int bar)
-{
-	/*
-	 * The barmap is now always set up in vfio_pci_core_enable().
-	 * Some legacy callers use this function to check if the BAR
-	 * is legitimate, so maintain that:
-	 */
-
-	return vdev->barmap[bar] ? 0 : -EBUSY;
-}
-EXPORT_SYMBOL_GPL(vfio_pci_core_setup_barmap);
-
 ssize_t vfio_pci_bar_rw(struct vfio_pci_core_device *vdev, char __user *buf,
 			size_t count, loff_t *ppos, bool iswrite)
 {
@@ -261,9 +249,8 @@ ssize_t vfio_pci_bar_rw(struct vfio_pci_core_device *vdev, char __user *buf,
 		 */
 		max_width = VFIO_PCI_IO_WIDTH_4;
 	} else {
-		int ret = vfio_pci_core_setup_barmap(vdev, bar);
-		if (ret) {
-			done = ret;
+		if (!vdev->barmap[bar]) {
+			done = -EINVAL;
 			goto out;
 		}
 
@@ -439,9 +426,8 @@ int vfio_pci_ioeventfd(struct vfio_pci_core_device *vdev, loff_t offset,
 	if (count == 8)
 		return -EINVAL;
 
-	ret = vfio_pci_core_setup_barmap(vdev, bar);
-	if (ret)
-		return ret;
+	if (!vdev->barmap[bar])
+		return -EINVAL;
 
 	mutex_lock(&vdev->ioeventfds_lock);
 
diff --git a/drivers/vfio/pci/virtio/legacy_io.c b/drivers/vfio/pci/virtio/legacy_io.c
index 1ed349a55629..67f93b96c099 100644
--- a/drivers/vfio/pci/virtio/legacy_io.c
+++ b/drivers/vfio/pci/virtio/legacy_io.c
@@ -305,9 +305,8 @@ static int virtiovf_set_notify_addr(struct virtiovf_pci_core_device *virtvdev)
 	 * Setup the BAR where the 'notify' exists to be used by vfio as well
 	 * This will let us mmap it only once and use it when needed.
 	 */
-	ret = vfio_pci_core_setup_barmap(core_device,
-					 virtvdev->notify_bar);
-	if (ret)
+	if (virtvdev->notify_bar >= PCI_STD_NUM_BARS ||
+	    !core_device->barmap[virtvdev->notify_bar])
 		return ret;
 
 	virtvdev->notify_addr = core_device->barmap[virtvdev->notify_bar] +
-- 
2.47.3