Re: [PATCH v6 15/31] vfio/pci: Route every reset point through a try-reset helper

[Alex Williamson]

On Fri, 16 Oct 2026 21:48:22 +0530
<mhonap@xxxxxxxxxx> wrote:

> From: Manish Honap <mhonap@xxxxxxxxxx>
> 
> A provider such as vfio-cxl must reset its device itself: a CXL Type-2
> function must not take an FLR, and its reset changes what the provider
> lets the fault path map. vfio-pci-core resets a function at five points:
> enable, close, VFIO_DEVICE_RESET, a virtualized PCIe or AF FLR, and the
> guest's CXL Reset doorbell in the virtualized CXL DVSEC.

What substantiates the claim that a type-2 device must not take an FLR?

Here we're promoting all resets to a CXL reset, but a CXL reset and an
FLR have different scopes.  FLR is exposed to the user through config
space.  A user might reasonably expect an FLR scoped reset through the
PCIe/AF capabilities.  The CXL spec defines FLR vs CXL reset, so this
actually makes the device non CXL compliant through vfio-pci.  This is
a massive policy imposition.

What makes this necessary?
 
> Add a mandatory reset op to the provider ops, and refuse to register ops
> without it. Add vfio_pci_try_reset_function(), which keeps the trylock
> of pci_try_reset_function(): it returns -EAGAIN when the device lock is
> contended, and otherwise runs the provider's reset with the device lock
> and memory_lock held. The close path keeps its bridge and device trylock
> and calls a locked variant. vfio_pci_try_reset_function() saves the PCI
> state first, as pci_try_reset_function() does, for the provider's reset
> to restore. The locked variant does not save: on close the reset must
> restore the open-time state that vfio_pci_core_disable() has just
> loaded. A failed save is returned and the reset is not attempted.
> 
> The enable path still fails only on -EAGAIN. A provider checks at bind
> that it can reset the device, so reset_works is set for a provider
> device whatever the result of the first reset.
> 
> The CXL Reset doorbell now runs the reset and reports its result in
> Status2: Reset Error on failure, Reset Done on success.
> 
> Suggested-by: Alex Williamson <alex@xxxxxxxxxxx>
> Assisted-by: LLM
> Signed-off-by: Manish Honap <mhonap@xxxxxxxxxx>
> ---
>  drivers/vfio/pci/vfio_pci_config.c | 19 ++++++--
>  drivers/vfio/pci/vfio_pci_core.c   | 71 ++++++++++++++++++++++++++++--
>  drivers/vfio/pci/vfio_pci_priv.h   |  1 +
>  include/linux/vfio_pci_core.h      |  7 +++
>  4 files changed, 90 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/vfio/pci/vfio_pci_config.c b/drivers/vfio/pci/vfio_pci_config.c
> index 1fc463540211..6fbece95cb9f 100644
> --- a/drivers/vfio/pci/vfio_pci_config.c
> +++ b/drivers/vfio/pci/vfio_pci_config.c
> @@ -910,7 +910,7 @@ static int vfio_exp_config_write(struct vfio_pci_core_device *vdev, int pos,
>  		if (!ret && (cap & PCI_EXP_DEVCAP_FLR)) {
>  			vfio_pci_zap_and_down_write_memory_lock(vdev);
>  			vfio_pci_dma_buf_move(vdev, true);
> -			pci_try_reset_function(vdev->pdev);
> +			vfio_pci_try_reset_function(vdev);
>  			if (__vfio_pci_memory_enabled(vdev))
>  				vfio_pci_dma_buf_move(vdev, false);
>  			up_write(&vdev->memory_lock);
> @@ -995,7 +995,7 @@ static int vfio_af_config_write(struct vfio_pci_core_device *vdev, int pos,
>  		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);
> -			pci_try_reset_function(vdev->pdev);
> +			vfio_pci_try_reset_function(vdev);
>  			if (__vfio_pci_memory_enabled(vdev))
>  				vfio_pci_dma_buf_move(vdev, false);
>  			up_write(&vdev->memory_lock);
> @@ -1786,9 +1786,20 @@ static int vfio_cxl_dvsec_write(struct vfio_pci_core_device *vdev, int pos,
>  	if (!(ctrl2 & PCI_DVSEC_CXL_DISABLE_CACHING))
>  		status2 &= ~PCI_DVSEC_CXL_CACHE_INV;
>  	if (ctrl2 & PCI_DVSEC_CXL_INIT_CXL_RST) {
> +		int ret;
> +
>  		ctrl2 &= ~PCI_DVSEC_CXL_INIT_CXL_RST;
> -		status2 &= ~PCI_DVSEC_CXL_RST_ERR;
> -		status2 |= PCI_DVSEC_CXL_RST_DONE;
> +
> +		/* Run the real reset here and report its result in Status2. */
> +		vfio_pci_zap_and_down_write_memory_lock(vdev);
> +		vfio_pci_dma_buf_move(vdev, true);
> +		ret = vfio_pci_try_reset_function(vdev);
> +		if (__vfio_pci_memory_enabled(vdev))
> +			vfio_pci_dma_buf_move(vdev, false);
> +		up_write(&vdev->memory_lock);
> +
> +		status2 &= ~(PCI_DVSEC_CXL_RST_DONE | PCI_DVSEC_CXL_RST_ERR);
> +		status2 |= ret ? PCI_DVSEC_CXL_RST_ERR : PCI_DVSEC_CXL_RST_DONE;
>  	}
>  
>  	*pctrl2 = cpu_to_le16(ctrl2);
> diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
> index 1e0ac28b7564..6676c561465b 100644
> --- a/drivers/vfio/pci/vfio_pci_core.c
> +++ b/drivers/vfio/pci/vfio_pci_core.c
> @@ -605,6 +605,62 @@ static void vfio_pci_core_unmap_bars(struct vfio_pci_core_device *vdev)
>  	}
>  }
>  
> +/**
> + * vfio_pci_try_reset_function() - reset the function without blocking
> + * @vdev: vfio PCI device, with memory_lock held for write
> + *
> + * Reset through the PCI core, or let the provider reset a device it serves.
> + * The provider resets the device with its PCI device lock held, so take that
> + * lock here, without waiting, as pci_try_reset_function() does.
> + *
> + * Return: 0 on success, -EAGAIN when the device lock is contended, or the
> + * reset error. For a provider device a failed pci_save_state() is also
> + * returned, and the reset is not attempted: unlike pci_try_reset_function(),
> + * the provider's reset needs a valid saved state to restore.
> + */
> +int vfio_pci_try_reset_function(struct vfio_pci_core_device *vdev)
> +{
> +	struct pci_dev *pdev = vdev->pdev;
> +	int ret;
> +
> +	lockdep_assert_held_write(&vdev->memory_lock);
> +
> +	if (!vdev->cxl_ops)
> +		return pci_try_reset_function(pdev);
> +
> +	if (!pci_dev_trylock(pdev))
> +		return -EAGAIN;
> +
> +	/*
> +	 * The provider restores PCI state from the saved copy, so save the
> +	 * current state first, as pci_try_reset_function() does. The locked
> +	 * variant on the close path must not: it resets with the state that
> +	 * vfio_pci_core_disable() has just loaded.
> +	 */
> +	ret = pci_save_state(pdev);
> +	if (!ret)
> +		ret = vdev->cxl_ops->reset(vdev);
> +	pci_dev_unlock(pdev);
> +
> +	return ret;
> +}
> +
> +/*
> + * As vfio_pci_try_reset_function(), for a caller that holds the device lock.
> + * Only the close path calls this. It takes memory_lock with the device lock
> + * held, so user config access is blocked meanwhile: the order a sysfs reset
> + * uses. That cannot deadlock against a PCI_COMMAND write holding memory_lock
> + * because no user remains at close.

Please tell the AI not to add any comments, these are difficult to
parse and often unnecessary.  If there are specific locking
requirements for this function, they should be annotated.  If this
function cannot be annotated, and it not general, even within a static
context, maybe it shouldn't be a separate function.

> + */
> +static int vfio_pci_reset_function_locked(struct vfio_pci_core_device *vdev)
> +{
> +	if (!vdev->cxl_ops)
> +		return __pci_reset_function_locked(vdev->pdev);
> +
> +	guard(rwsem_write)(&vdev->memory_lock);
> +	return vdev->cxl_ops->reset(vdev);
> +}
> +
>  static void vfio_pci_core_free_dev_regions(struct vfio_pci_core_device *vdev)
>  {
>  	for (int i = 0; i < vdev->num_regions; i++)
> @@ -636,11 +692,14 @@ int vfio_pci_core_enable(struct vfio_pci_core_device *vdev)
>  		goto out_power;
>  
>  	/* If reset fails because of the device lock, fail this path entirely */
> -	ret = pci_try_reset_function(pdev);
> +	down_write(&vdev->memory_lock);
> +	ret = vfio_pci_try_reset_function(vdev);
> +	up_write(&vdev->memory_lock);
>  	if (ret == -EAGAIN)
>  		goto out_disable_device;
>  
> -	vdev->reset_works = !ret;
> +	/* A provider checks at bind that it can reset the device. */
> +	vdev->reset_works = vdev->cxl_ops || !ret;

I suppose, but why is ret not still a valid indicator?

>  	pci_save_state(pdev);
>  	vdev->pci_saved_state = pci_store_saved_state(pdev);
>  	if (!vdev->pci_saved_state)
> @@ -846,7 +905,7 @@ void vfio_pci_core_disable(struct vfio_pci_core_device *vdev)
>  		if (bridge && !pci_dev_trylock(bridge))
>  			goto out_restore_state;
>  		if (pci_dev_trylock(pdev)) {
> -			if (!__pci_reset_function_locked(pdev))
> +			if (!vfio_pci_reset_function_locked(vdev))
>  				vdev->needs_reset = false;
>  			pci_dev_unlock(pdev);
>  		}
> @@ -1541,7 +1600,7 @@ 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);
> -	ret = pci_try_reset_function(vdev->pdev);
> +	ret = vfio_pci_try_reset_function(vdev);
>  	if (__vfio_pci_memory_enabled(vdev))
>  		vfio_pci_dma_buf_move(vdev, false);
>  	up_write(&vdev->memory_lock);
> @@ -2397,6 +2456,10 @@ int vfio_pci_core_register_cxl_ops(const struct vfio_cxl_ops *ops)
>  {
>  	guard(rwsem_write)(&vfio_pci_cxl_ops_rwsem);
>  
> +	/* vfio-pci-core never resets a provider's device by itself. */

What a bizarre comment.  Relevance?

> +	if (!ops->reset)
> +		return -EINVAL;
> +
>  	if (vfio_pci_cxl_ops)
>  		return -EBUSY;
>  
> diff --git a/drivers/vfio/pci/vfio_pci_priv.h b/drivers/vfio/pci/vfio_pci_priv.h
> index 09a68c99b43c..dbede6e24fa9 100644
> --- a/drivers/vfio/pci/vfio_pci_priv.h
> +++ b/drivers/vfio/pci/vfio_pci_priv.h
> @@ -80,6 +80,7 @@ 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);
> +int vfio_pci_try_reset_function(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 991a3fcd84eb..cb9ba92b696a 100644
> --- a/include/linux/vfio_pci_core.h
> +++ b/include/linux/vfio_pci_core.h
> @@ -87,6 +87,13 @@ struct vfio_cxl_ops {
>  	 */
>  	void	(*reset_prepare)(struct vfio_pci_core_device *vdev);
>  	int	(*reset_done)(struct vfio_pci_core_device *vdev);
> +	/*
> +	 * Reset the function in place of the PCI core; mandatory. Called with
> +	 * the PCI device lock and memory_lock (for write) held. The caller
> +	 * provides the saved PCI state the reset restores: the try-reset path
> +	 * saves it first, the close path keeps the open-time copy.
> +	 */

Reads like more AI generated comments.  These things like to talk,
please curb them.  Look at vfio_device_ops for an example how to
briefly and concisely document callback ops like a human.  Thanks,

Alex

PS - I'm still working on the remainder of the series.

> +	int	(*reset)(struct vfio_pci_core_device *vdev);
>  	/* Pinned per bound CXL device so vfio-cxl cannot unload under usage */
>  	struct module *owner;
>  };