Re: [PATCH 1/6] vfio: Introduce .migration_reset_state() callback

[Alex Williamson <alex@shazbot.org>]

On Thu, 20 Nov 2025 13:36:42 +0100
Michał Winiarski <michal.winiarski@intel.com> wrote:

> Resetting the migration device state is typically delegated to PCI
> .reset_done() callback.
> With VFIO, reset is usually called under vdev->memory_lock, which causes
> lockdep to report a following circular locking dependency scenario:
> 
> 0: set_device_state
> driver->state_mutex -> migf->lock
> 1: data_read
> migf->lock -> mm->mmap_lock
> 2: vfio_pin_dma
> mm->mmap_lock -> vdev->memory_lock
> 3: vfio_pci_ioctl_reset
> vdev->memory_lock -> driver->state_mutex
> 
> Introduce a .migration_reset_state() callback called outside of
> vdev->memory_lock to break the dependency chain.
> 
> Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
> ---
>  drivers/vfio/pci/vfio_pci_core.c | 25 ++++++++++++++++++++++---
>  include/linux/vfio.h             |  4 ++++
>  2 files changed, 26 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
> index 7dcf5439dedc9..d919636558ec8 100644
> --- a/drivers/vfio/pci/vfio_pci_core.c
> +++ b/drivers/vfio/pci/vfio_pci_core.c
> @@ -553,6 +553,16 @@ int vfio_pci_core_enable(struct vfio_pci_core_device *vdev)
>  }
>  EXPORT_SYMBOL_GPL(vfio_pci_core_enable);
>  
> +static void vfio_pci_dev_migration_reset_state(struct vfio_pci_core_device *vdev)
> +{
> +	lockdep_assert_not_held(&vdev->memory_lock);
> +
> +	if (!vdev->vdev.mig_ops->migration_reset_state)

mig_ops itself is generally NULL.

> +		return;
> +
> +	vdev->vdev.mig_ops->migration_reset_state(&vdev->vdev);
> +}
> +
>  void vfio_pci_core_disable(struct vfio_pci_core_device *vdev)
>  {
>  	struct pci_dev *pdev = vdev->pdev;
> @@ -662,8 +672,10 @@ void vfio_pci_core_disable(struct vfio_pci_core_device *vdev)
>  	 * overwrite the previously restored configuration information.
>  	 */
>  	if (vdev->reset_works && pci_dev_trylock(pdev)) {
> -		if (!__pci_reset_function_locked(pdev))
> +		if (!__pci_reset_function_locked(pdev)) {
>  			vdev->needs_reset = false;
> +			vfio_pci_dev_migration_reset_state(vdev);
> +		}
>  		pci_dev_unlock(pdev);
>  	}
>  
> @@ -1230,6 +1242,8 @@ static int vfio_pci_ioctl_reset(struct vfio_pci_core_device *vdev,
>  	ret = pci_try_reset_function(vdev->pdev);
>  	up_write(&vdev->memory_lock);
>  
> +	vfio_pci_dev_migration_reset_state(vdev);
> +
>  	return ret;
>  }
>  
> @@ -2129,6 +2143,7 @@ int vfio_pci_core_register_device(struct vfio_pci_core_device *vdev)
>  	if (vdev->vdev.mig_ops) {
>  		if (!(vdev->vdev.mig_ops->migration_get_state &&
>  		      vdev->vdev.mig_ops->migration_set_state &&
> +		      vdev->vdev.mig_ops->migration_reset_state &&

For bisection purposes it would be better to enforce this after all the
drivers are converted.

>  		      vdev->vdev.mig_ops->migration_get_data_size) ||
>  		    !(vdev->vdev.migration_flags & VFIO_MIGRATION_STOP_COPY))
>  			return -EINVAL;
> @@ -2486,8 +2501,10 @@ static int vfio_pci_dev_set_hot_reset(struct vfio_device_set *dev_set,
>  
>  err_undo:
>  	list_for_each_entry_from_reverse(vdev, &dev_set->device_list,
> -					 vdev.dev_set_list)
> +					 vdev.dev_set_list) {
>  		up_write(&vdev->memory_lock);
> +		vfio_pci_dev_migration_reset_state(vdev);
> +	}
>  
>  	list_for_each_entry(vdev, &dev_set->device_list, vdev.dev_set_list)
>  		pm_runtime_put(&vdev->pdev->dev);
> @@ -2543,8 +2560,10 @@ static void vfio_pci_dev_set_try_reset(struct vfio_device_set *dev_set)
>  		reset_done = true;
>  
>  	list_for_each_entry(cur, &dev_set->device_list, vdev.dev_set_list) {
> -		if (reset_done)
> +		if (reset_done) {
>  			cur->needs_reset = false;
> +			vfio_pci_dev_migration_reset_state(cur);
> +		}

This and the core_disable path above are only called in the
close/open-error path.  Do we really need this behavior there?  We
might need separate reconciliation vs .reset_done for these.

As Kevin also noted, we're missing the non-ioctl reset paths.  This
approach seems a bit error prone.  I wonder if instead we need a
counterpart of vfio_pci_zap_and_down_write_memory_lock(), ie.
vfio_pci_up_write_memory_lock_from_reset().  An equal mouthful, but
scopes the problem to be more manageable at memory_lock release.
Thanks,

Alex


>  
>  		if (!disable_idle_d3)
>  			pm_runtime_put(&cur->pdev->dev);
> diff --git a/include/linux/vfio.h b/include/linux/vfio.h
> index eb563f538dee5..36aab2df40700 100644
> --- a/include/linux/vfio.h
> +++ b/include/linux/vfio.h
> @@ -213,6 +213,9 @@ static inline bool vfio_device_cdev_opened(struct vfio_device *device)
>   * @migration_get_state: Optional callback to get the migration state for
>   *         devices that support migration. It's mandatory for
>   *         VFIO_DEVICE_FEATURE_MIGRATION migration support.
> + * @migration_reset_state: Optional callback to reset the migration state for
> + *         devices that support migration. It's mandatory for
> + *         VFIO_DEVICE_FEATURE_MIGRATION migration support.
>   * @migration_get_data_size: Optional callback to get the estimated data
>   *          length that will be required to complete stop copy. It's mandatory for
>   *          VFIO_DEVICE_FEATURE_MIGRATION migration support.
> @@ -223,6 +226,7 @@ struct vfio_migration_ops {
>  		enum vfio_device_mig_state new_state);
>  	int (*migration_get_state)(struct vfio_device *device,
>  				   enum vfio_device_mig_state *curr_state);
> +	void (*migration_reset_state)(struct vfio_device *device);
>  	int (*migration_get_data_size)(struct vfio_device *device,
>  				       unsigned long *stop_copy_length);
>  };