Re: [PATCH v2 1/3] vfio/pci: Set up bar resources and maps in vfio_pci_core_enable()

[Alex Williamson <alex@shazbot.org>]

On Thu, 23 Apr 2026 11:25:07 -0700
Matt Evans <mattev@meta.com> wrote:

> Previously BAR resource requests and the corresponding pci_iomap()
> were performed on-demand and without synchronisation, which was racy.
> Rather than add synchronisation, it's simplest to address this by
> doing both activities from vfio_pci_core_enable().
> 
> The resource allocation and/or pci_iomap() can still fail; their
> status is tracked and existing calls to vfio_pci_core_setup_barmap()
> will fail in the same way as before.  This keeps the point of failure
> as observed by userspace the same, i.e. failures to request/map unused
> BARs are benign.
> 
> 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 | 61 +++++++++++++++++++++++++++-----
>  drivers/vfio/pci/vfio_pci_rdwr.c | 29 ++++++---------
>  include/linux/vfio_pci_core.h    |  1 +
>  3 files changed, 64 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
> index 3f8d093aacf8..c59c61861d81 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[bar])
> +			pci_iounmap(pdev, vdev->barmap[bar]);
> +		if (vdev->have_bar_resource[bar])
> +			pci_release_selected_regions(pdev, 1 << bar);
> +		vdev->barmap[bar] = NULL;
> +		vdev->have_bar_resource[bar] = false;
> +	}
> +}
> +
> +static void __vfio_pci_core_map_bars(struct vfio_pci_core_device *vdev)
> +{
> +	struct pci_dev *pdev = vdev->pdev;
> +	int i;
> +
> +	/*
> +	 * Eager-request BAR resources, and iomap; soft failures are
> +	 * allowed, and consumers must check before use.
> +	 */

I'd use this to describe that soft failures maintain compatible error
signatures to previously used on-demand mapping.

> +	for (i = 0; i < PCI_STD_NUM_BARS; i++) {
> +		int ret;
> +		int bar = i + PCI_STD_RESOURCES;
> +		void __iomem *io;

Reverse Christmas tree ordering.

> +
> +		if (pci_resource_len(pdev, i) == 0)
> +			continue;
> +
> +		ret = pci_request_selected_regions(pdev, 1 << bar, "vfio");
> +		if (ret) {
> +			pci_warn(vdev->pdev, "Failed to reserve region %d\n", bar);
> +			continue;
> +		}
> +		vdev->have_bar_resource[bar] = true;
> +
> +		io = pci_iomap(pdev, bar, 0);
> +		if (io)
> +			vdev->barmap[bar] = io;
> +		else
> +			pci_warn(vdev->pdev, "Failed to iomap region %d\n", bar);
> +	}
> +}

I see you making the point in the cover letter about the resource
request vs the iomap resource, but we currently handle these together.
If either fails, setup barmap fails and the path returns error.  I
don't see any justification for now allowing the request resource to
succeed but the iomap fails.

These functions also don't need the double-underscore prefix.

> +
>  /*
>   * 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,7 @@ int vfio_pci_core_enable(struct vfio_pci_core_device *vdev)
>  	if (!vfio_vga_disabled() && vfio_pci_is_vga(pdev))
>  		vdev->has_vga = true;
>  
> +	 __vfio_pci_core_map_bars(vdev);
>  
>  	return 0;
>  
> @@ -591,7 +641,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 +696,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);

I expect this doesn't need to change if we drop the separation between
resources and iomap.
 
>  	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..bf7152316db4 100644
> --- a/drivers/vfio/pci/vfio_pci_rdwr.c
> +++ b/drivers/vfio/pci/vfio_pci_rdwr.c
> @@ -200,25 +200,18 @@ 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);
> +	/*
> +	 * The barmap is now always set up in vfio_pci_core_enable().

"now" is going to read strangely very quickly.

> +	 * Some legacy callers use this function to ensure the BAR
> +	 * resources are requested, and others to ensure the
> +	 * pci_iomap() was done, so check here:
> +	 */
> +	if (bar < 0 || bar >= PCI_STD_NUM_BARS)
> +		return -EINVAL;
> +	if (vdev->barmap[bar] == 0)
>  		return -ENOMEM;
> -	}
> -
> -	vdev->barmap[bar] = io;
> -
> +	if (!vdev->bar_has_rsrc[bar])

Typo, this won't incrementally compile.  Thanks,

Alex

> +		return -EBUSY;
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(vfio_pci_core_setup_barmap);
> diff --git a/include/linux/vfio_pci_core.h b/include/linux/vfio_pci_core.h
> index 2ebba746c18f..1f508b067d82 100644
> --- a/include/linux/vfio_pci_core.h
> +++ b/include/linux/vfio_pci_core.h
> @@ -101,6 +101,7 @@ struct vfio_pci_core_device {
>  	const struct vfio_pci_device_ops *pci_ops;
>  	void __iomem		*barmap[PCI_STD_NUM_BARS];
>  	bool			bar_mmap_supported[PCI_STD_NUM_BARS];
> +	bool			have_bar_resource[PCI_STD_NUM_BARS];
>  	u8			*pci_config_map;
>  	u8			*vconfig;
>  	struct perm_bits	*msi_perm;