Re: [PATCH v6 12/31] vfio/pci: Call the CXL open and close hooks around device use

[Alex Williamson <alex@shazbot.org>]

On Fri, 16 Oct 2026 21:48:19 +0530
<mhonap@nvidia.com> wrote:

> From: Manish Honap <mhonap@nvidia.com>
> 
> vfio-pci-core frees device regions and excluded ranges on close and on a
> failed open, so a CXL provider must set them up at each open.
> 
> Add open and close to the CXL ops and call them from the common enable
> and close paths. A failed open unwinds the enable in reverse order:
> release the device regions, unmap the BARs, drop the excluded ranges,
> then free the virtual config.
> 
> Factor the device region release and the BAR unmap out of
> vfio_pci_core_disable() into helpers so the enable failure path and the
> disable path share them.
> 
> Assisted-by: LLM
> Signed-off-by: Manish Honap <mhonap@nvidia.com>
> ---
>  drivers/vfio/pci/vfio_pci_core.c | 57 +++++++++++++++++++++++---------
>  include/linux/vfio_pci_core.h    |  2 ++
>  2 files changed, 44 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
> index 211fff6045e2..3d4e01eea7b1 100644
> --- a/drivers/vfio/pci/vfio_pci_core.c
> +++ b/drivers/vfio/pci/vfio_pci_core.c
> @@ -590,6 +590,31 @@ static const struct dev_pm_ops vfio_pci_core_pm_ops = {
>  
>  static void vfio_pci_free_excluded_ranges(struct vfio_pci_core_device *vdev);
>  
> +static void vfio_pci_core_unmap_bars(struct vfio_pci_core_device *vdev)
> +{
> +	struct pci_dev *pdev = vdev->pdev;
> +
> +	for (int i = 0; i < PCI_STD_NUM_BARS; i++) {
> +		int bar = i + PCI_STD_RESOURCES;
> +
> +		if (IS_ERR_OR_NULL(vdev->barmap[bar]))
> +			continue;
> +		pci_iounmap(pdev, vdev->barmap[bar]);
> +		pci_release_selected_regions(pdev, 1 << bar);
> +		vdev->barmap[bar] = NULL;
> +	}
> +}
> +
> +static void vfio_pci_core_free_dev_regions(struct vfio_pci_core_device *vdev)
> +{
> +	for (int i = 0; i < vdev->num_regions; i++)
> +		vdev->region[i].ops->release(vdev, &vdev->region[i]);
> +
> +	vdev->num_regions = 0;
> +	kfree(vdev->region);
> +	vdev->region = NULL; /* don't krealloc a freed pointer */
> +}

Mechanical changes like this should be done in a separate patch.

> +
>  int vfio_pci_core_enable(struct vfio_pci_core_device *vdev)
>  {
>  	struct pci_dev *pdev = vdev->pdev;
> @@ -691,8 +716,20 @@ int vfio_pci_core_enable(struct vfio_pci_core_device *vdev)
>  
>  	vfio_pci_core_map_bars(vdev);
>  
> +	if (vdev->cxl_ops) {
> +		ret = vdev->cxl_ops->open_device(vdev);
> +		if (ret)
> +			goto out_free_config;
> +	}
> +
>  	return 0;
>  
> +out_free_config:
> +	/* A failed first open never reaches vfio_pci_core_disable(). */
> +	vfio_pci_core_free_dev_regions(vdev);
> +	vfio_pci_core_unmap_bars(vdev);
> +	vfio_pci_free_excluded_ranges(vdev);
> +	vfio_config_free(vdev);

Raising again that this doesn't account for the code previously added
to this function that has its own unwind.

There also remains the question of whether excluded range setup should
be moved to init/release.

>  out_free_zdev:
>  	vfio_pci_zdev_close_device(vdev);
>  out_free_state:
> @@ -713,7 +750,6 @@ 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;
>  
>  	/* For needs_reset */
>  	lockdep_assert_held(&vdev->vdev.dev_set->lock);
> @@ -759,24 +795,12 @@ void vfio_pci_core_disable(struct vfio_pci_core_device *vdev)
>  
>  	vdev->virq_disabled = false;
>  
> -	for (i = 0; i < vdev->num_regions; i++)
> -		vdev->region[i].ops->release(vdev, &vdev->region[i]);
> -
> -	vdev->num_regions = 0;
> -	kfree(vdev->region);
> -	vdev->region = NULL; /* don't krealloc a freed pointer */
> +	vfio_pci_core_free_dev_regions(vdev);
>  
>  	vfio_config_free(vdev);
>  	vfio_pci_free_excluded_ranges(vdev);
>  
> -	for (i = 0; i < PCI_STD_NUM_BARS; i++) {
> -		bar = i + PCI_STD_RESOURCES;
> -		if (IS_ERR_OR_NULL(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) {
> @@ -859,6 +883,9 @@ void vfio_pci_core_close_device(struct vfio_device *core_vdev)
>  #endif
>  	vfio_pci_dma_buf_cleanup(vdev);
>  
> +	if (vdev->cxl_ops)
> +		vdev->cxl_ops->close_device(vdev);
> +

Mismatched in both naming and conventions.

vfio_pci_core_enable() -> cxl_ops->open_device()

vfio_pci_core_close_device() -> cxl_ops->close_device()

Some variant drivers unwind a failure after core_enable() with
core_disable(), so the placement in the close_device() path is not only
inconsistent, it's wrong.  The hooks should also be consistently named
relative to where they're called from the core.  Thanks,

Alex

>  	vfio_pci_core_disable(vdev);
>  
>  	mutex_lock(&vdev->igate);
> diff --git a/include/linux/vfio_pci_core.h b/include/linux/vfio_pci_core.h
> index fd1ad0baf470..6520a2dcfd4a 100644
> --- a/include/linux/vfio_pci_core.h
> +++ b/include/linux/vfio_pci_core.h
> @@ -70,6 +70,8 @@ struct vfio_pci_device_ops {
>  struct vfio_cxl_ops {
>  	int	(*init)(struct vfio_pci_core_device *vdev);
>  	void	(*release)(struct vfio_pci_core_device *vdev);
> +	int	(*open_device)(struct vfio_pci_core_device *vdev);
> +	void	(*close_device)(struct vfio_pci_core_device *vdev);
>  	/* Pinned per bound CXL device so vfio-cxl cannot unload under usage */
>  	struct module *owner;
>  };