Re: [PATCH] hisi_acc_vfio_pci: Remove the deferred_reset logic

[Alex Williamson <alex.williamson@redhat.com>]

On Thu, 29 Feb 2024 09:11:52 +0000
Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:

> The deferred_reset logic was added to vfio migration drivers to prevent
> a circular locking dependency with respect to mm_lock and state mutex.
> This is mainly because of the copy_to/from_user() functions(which takes
> mm_lock) invoked under state mutex. But for HiSilicon driver, the only
> place where we now hold the state mutex for copy_to_user is during the
> PRE_COPY IOCTL. So for pre_copy, release the lock as soon as we have
> updated the data and perform copy_to_user without state mutex. By this,
> we can get rid of the deferred_reset logic.
> 
> Link: https://lore.kernel.org/kvm/20240220132459.GM13330@nvidia.com/
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> ---
>  .../vfio/pci/hisilicon/hisi_acc_vfio_pci.c    | 48 +++++--------------
>  .../vfio/pci/hisilicon/hisi_acc_vfio_pci.h    |  6 +--
>  2 files changed, 14 insertions(+), 40 deletions(-)

Applied to vfio next branch for v6.9.  Thanks,

Alex

> 
> diff --git a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
> index 4d27465c8f1a..9a3e97108ace 100644
> --- a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
> +++ b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
> @@ -630,25 +630,11 @@ static void hisi_acc_vf_disable_fds(struct hisi_acc_vf_core_device *hisi_acc_vde
>  	}
>  }
>  
> -/*
> - * This function is called in all state_mutex unlock cases to
> - * handle a 'deferred_reset' if exists.
> - */
> -static void
> -hisi_acc_vf_state_mutex_unlock(struct hisi_acc_vf_core_device *hisi_acc_vdev)
> +static void hisi_acc_vf_reset(struct hisi_acc_vf_core_device *hisi_acc_vdev)
>  {
> -again:
> -	spin_lock(&hisi_acc_vdev->reset_lock);
> -	if (hisi_acc_vdev->deferred_reset) {
> -		hisi_acc_vdev->deferred_reset = false;
> -		spin_unlock(&hisi_acc_vdev->reset_lock);
> -		hisi_acc_vdev->vf_qm_state = QM_NOT_READY;
> -		hisi_acc_vdev->mig_state = VFIO_DEVICE_STATE_RUNNING;
> -		hisi_acc_vf_disable_fds(hisi_acc_vdev);
> -		goto again;
> -	}
> -	mutex_unlock(&hisi_acc_vdev->state_mutex);
> -	spin_unlock(&hisi_acc_vdev->reset_lock);
> +	hisi_acc_vdev->vf_qm_state = QM_NOT_READY;
> +	hisi_acc_vdev->mig_state = VFIO_DEVICE_STATE_RUNNING;
> +	hisi_acc_vf_disable_fds(hisi_acc_vdev);
>  }
>  
>  static void hisi_acc_vf_start_device(struct hisi_acc_vf_core_device *hisi_acc_vdev)
> @@ -804,8 +790,10 @@ static long hisi_acc_vf_precopy_ioctl(struct file *filp,
>  
>  	info.dirty_bytes = 0;
>  	info.initial_bytes = migf->total_length - *pos;
> +	mutex_unlock(&migf->lock);
> +	mutex_unlock(&hisi_acc_vdev->state_mutex);
>  
> -	ret = copy_to_user((void __user *)arg, &info, minsz) ? -EFAULT : 0;
> +	return copy_to_user((void __user *)arg, &info, minsz) ? -EFAULT : 0;
>  out:
>  	mutex_unlock(&migf->lock);
>  	mutex_unlock(&hisi_acc_vdev->state_mutex);
> @@ -1071,7 +1059,7 @@ hisi_acc_vfio_pci_set_device_state(struct vfio_device *vdev,
>  			break;
>  		}
>  	}
> -	hisi_acc_vf_state_mutex_unlock(hisi_acc_vdev);
> +	mutex_unlock(&hisi_acc_vdev->state_mutex);
>  	return res;
>  }
>  
> @@ -1092,7 +1080,7 @@ hisi_acc_vfio_pci_get_device_state(struct vfio_device *vdev,
>  
>  	mutex_lock(&hisi_acc_vdev->state_mutex);
>  	*curr_state = hisi_acc_vdev->mig_state;
> -	hisi_acc_vf_state_mutex_unlock(hisi_acc_vdev);
> +	mutex_unlock(&hisi_acc_vdev->state_mutex);
>  	return 0;
>  }
>  
> @@ -1104,21 +1092,9 @@ static void hisi_acc_vf_pci_aer_reset_done(struct pci_dev *pdev)
>  				VFIO_MIGRATION_STOP_COPY)
>  		return;
>  
> -	/*
> -	 * As the higher VFIO layers are holding locks across reset and using
> -	 * those same locks with the mm_lock we need to prevent ABBA deadlock
> -	 * with the state_mutex and mm_lock.
> -	 * In case the state_mutex was taken already we defer the cleanup work
> -	 * to the unlock flow of the other running context.
> -	 */
> -	spin_lock(&hisi_acc_vdev->reset_lock);
> -	hisi_acc_vdev->deferred_reset = true;
> -	if (!mutex_trylock(&hisi_acc_vdev->state_mutex)) {
> -		spin_unlock(&hisi_acc_vdev->reset_lock);
> -		return;
> -	}
> -	spin_unlock(&hisi_acc_vdev->reset_lock);
> -	hisi_acc_vf_state_mutex_unlock(hisi_acc_vdev);
> +	mutex_lock(&hisi_acc_vdev->state_mutex);
> +	hisi_acc_vf_reset(hisi_acc_vdev);
> +	mutex_unlock(&hisi_acc_vdev->state_mutex);
>  }
>  
>  static int hisi_acc_vf_qm_init(struct hisi_acc_vf_core_device *hisi_acc_vdev)
> diff --git a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.h b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.h
> index dcabfeec6ca1..5bab46602fad 100644
> --- a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.h
> +++ b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.h
> @@ -98,8 +98,8 @@ struct hisi_acc_vf_migration_file {
>  
>  struct hisi_acc_vf_core_device {
>  	struct vfio_pci_core_device core_device;
> -	u8 match_done:1;
> -	u8 deferred_reset:1;
> +	u8 match_done;
> +
>  	/* For migration state */
>  	struct mutex state_mutex;
>  	enum vfio_device_mig_state mig_state;
> @@ -109,8 +109,6 @@ struct hisi_acc_vf_core_device {
>  	struct hisi_qm vf_qm;
>  	u32 vf_qm_state;
>  	int vf_id;
> -	/* For reset handler */
> -	spinlock_t reset_lock;
>  	struct hisi_acc_vf_migration_file *resuming_migf;
>  	struct hisi_acc_vf_migration_file *saving_migf;
>  };