[PATCH vfio 0/2] hisi_acc_vfio_pci: locking updates

[PATCH vfio 0/2] hisi_acc_vfio_pci: locking updates

[Brett Creeley]

The vfio/pds series for locking updates/fixes in the following link
made some changes that can also be done for other vendor's vfio
drivers. Specifically, changing the reset lock from a spinlock to mutex
and also calling mutex_destroy() in the vfio device release callback.

https://lore.kernel.org/kvm/20231122192532.25791-1-brett.creeley@amd.com/

So, this series makes these changes in order to remain separate from
the vfio/pds series linked above.

Note, that I don't have the required hardware to test on this vendor's
hardware, so help would be appreciated.

Brett Creeley (2):
  hisi_acc_vfio_pci: Change reset_lock to mutex_lock
  hisi_acc_vfio_pci: Destroy the [state|reset]_mutex on release

 .../vfio/pci/hisilicon/hisi_acc_vfio_pci.c    | 25 +++++++++++++------
 .../vfio/pci/hisilicon/hisi_acc_vfio_pci.h    |  3 +--
 2 files changed, 19 insertions(+), 9 deletions(-)

-- 
2.17.1

[PATCH vfio 1/2] hisi_acc_vfio_pci: Change reset_lock to mutex_lock

[Brett Creeley]

Based on comments from other vfio vendors and the
maintainer the vfio/pds driver changed the reset_lock
to a mutex_lock. As part of that change it was requested
that the other vendor drivers be changed as well. So,
make the change.

The comment that requested the change for reference:
https://lore.kernel.org/kvm/BN9PR11MB52769E037CB356AB15A0D9B88CA0A@BN9PR11MB5276.namprd11.prod.outlook.com/

Also, make checkpatch happy by moving the lock comment.

Signed-off-by: Brett Creeley <brett.creeley@amd.com>
---
 drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c | 13 +++++++------
 drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.h |  3 +--
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
index b2f9778c8366..2c049b8de4b4 100644
--- a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
+++ b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
@@ -638,17 +638,17 @@ static void
 hisi_acc_vf_state_mutex_unlock(struct hisi_acc_vf_core_device *hisi_acc_vdev)
 {
 again:
-	spin_lock(&hisi_acc_vdev->reset_lock);
+	mutex_lock(&hisi_acc_vdev->reset_mutex);
 	if (hisi_acc_vdev->deferred_reset) {
 		hisi_acc_vdev->deferred_reset = false;
-		spin_unlock(&hisi_acc_vdev->reset_lock);
+		mutex_unlock(&hisi_acc_vdev->reset_mutex);
 		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);
+	mutex_unlock(&hisi_acc_vdev->reset_mutex);
 }
 
 static void hisi_acc_vf_start_device(struct hisi_acc_vf_core_device *hisi_acc_vdev)
@@ -1108,13 +1108,13 @@ static void hisi_acc_vf_pci_aer_reset_done(struct pci_dev *pdev)
 	 * 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);
+	mutex_lock(&hisi_acc_vdev->reset_mutex);
 	hisi_acc_vdev->deferred_reset = true;
 	if (!mutex_trylock(&hisi_acc_vdev->state_mutex)) {
-		spin_unlock(&hisi_acc_vdev->reset_lock);
+		mutex_unlock(&hisi_acc_vdev->reset_mutex);
 		return;
 	}
-	spin_unlock(&hisi_acc_vdev->reset_lock);
+	mutex_unlock(&hisi_acc_vdev->reset_mutex);
 	hisi_acc_vf_state_mutex_unlock(hisi_acc_vdev);
 }
 
@@ -1350,6 +1350,7 @@ static int hisi_acc_vfio_pci_migrn_init_dev(struct vfio_device *core_vdev)
 	hisi_acc_vdev->pf_qm = pf_qm;
 	hisi_acc_vdev->vf_dev = pdev;
 	mutex_init(&hisi_acc_vdev->state_mutex);
+	mutex_init(&hisi_acc_vdev->reset_mutex);
 
 	core_vdev->migration_flags = VFIO_MIGRATION_STOP_COPY | VFIO_MIGRATION_PRE_COPY;
 	core_vdev->mig_ops = &hisi_acc_vfio_pci_migrn_state_ops;
diff --git a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.h b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.h
index dcabfeec6ca1..ed5ab332d0f3 100644
--- a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.h
+++ b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.h
@@ -109,8 +109,7 @@ 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 mutex reset_mutex; /* For reset handler */
 	struct hisi_acc_vf_migration_file *resuming_migf;
 	struct hisi_acc_vf_migration_file *saving_migf;
 };
-- 
2.17.1

[PATCH vfio 2/2] hisi_acc_vfio_pci: Destroy the [state|reset]_mutex on release

[Brett Creeley]

The [state|reset]_mutex are initialized in vfio init, but
never destroyed. This isn't required as mutex_destroy()
doesn't do anything unless lock debugging is enabled.
However, for completeness, fix it by implementing a
driver specific release function.

No fixes tag is added as it doesn't seem worthwhile
for such a trivial and debug only change.

Signed-off-by: Brett Creeley <brett.creeley@amd.com>
---
 drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
index 2c049b8de4b4..dc1e376e1b8a 100644
--- a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
+++ b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
@@ -1358,10 +1358,20 @@ static int hisi_acc_vfio_pci_migrn_init_dev(struct vfio_device *core_vdev)
 	return vfio_pci_core_init_dev(core_vdev);
 }
 
+static void hisi_acc_vfio_pci_migrn_release_dev(struct vfio_device *core_vdev)
+{
+	struct hisi_acc_vf_core_device *hisi_acc_vdev = container_of(core_vdev,
+			struct hisi_acc_vf_core_device, core_device.vdev);
+
+	mutex_destroy(&hisi_acc_vdev->reset_mutex);
+	mutex_destroy(&hisi_acc_vdev->state_mutex);
+	vfio_pci_core_release_dev(core_vdev);
+}
+
 static const struct vfio_device_ops hisi_acc_vfio_pci_migrn_ops = {
 	.name = "hisi-acc-vfio-pci-migration",
 	.init = hisi_acc_vfio_pci_migrn_init_dev,
-	.release = vfio_pci_core_release_dev,
+	.release = hisi_acc_vfio_pci_migrn_release_dev,
 	.open_device = hisi_acc_vfio_pci_open_device,
 	.close_device = hisi_acc_vfio_pci_close_device,
 	.ioctl = hisi_acc_vfio_pci_ioctl,
-- 
2.17.1

RE: [PATCH vfio 1/2] hisi_acc_vfio_pci: Change reset_lock to mutex_lock

[Shameerali Kolothum Thodi]

> -----Original Message-----
> From: Brett Creeley [mailto:brett.creeley@amd.com]
> Sent: 22 November 2023 19:37
> To: jgg@ziepe.ca; yishaih@nvidia.com; liulongfang
> <liulongfang@huawei.com>; Shameerali Kolothum Thodi
> <shameerali.kolothum.thodi@huawei.com>; kevin.tian@intel.com;
> alex.williamson@redhat.com; kvm@vger.kernel.org;
> linux-kernel@vger.kernel.org
> Cc: shannon.nelson@amd.com; brett.creeley@amd.com
> Subject: [PATCH vfio 1/2] hisi_acc_vfio_pci: Change reset_lock to mutex_lock
> 
> Based on comments from other vfio vendors and the
> maintainer the vfio/pds driver changed the reset_lock
> to a mutex_lock. As part of that change it was requested
> that the other vendor drivers be changed as well. So,
> make the change.
> 
> The comment that requested the change for reference:
> https://lore.kernel.org/kvm/BN9PR11MB52769E037CB356AB15A0D9B88CA
> 0A@BN9PR11MB5276.namprd11.prod.outlook.com/
> 
> Also, make checkpatch happy by moving the lock comment.
> 
> Signed-off-by: Brett Creeley <brett.creeley@amd.com>
> ---
>  drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c | 13 +++++++------
>  drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.h |  3 +--
>  2 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
> b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
> index b2f9778c8366..2c049b8de4b4 100644
> --- a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
> +++ b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
> @@ -638,17 +638,17 @@ static void
>  hisi_acc_vf_state_mutex_unlock(struct hisi_acc_vf_core_device
> *hisi_acc_vdev)
>  {
>  again:
> -	spin_lock(&hisi_acc_vdev->reset_lock);
> +	mutex_lock(&hisi_acc_vdev->reset_mutex);
>  	if (hisi_acc_vdev->deferred_reset) {
>  		hisi_acc_vdev->deferred_reset = false;
> -		spin_unlock(&hisi_acc_vdev->reset_lock);
> +		mutex_unlock(&hisi_acc_vdev->reset_mutex);

Don't think we have that sleeping while atomic case for this here.
Same for mlx5 as well. But if the idea is to have a common locking
across vendor drivers, it is fine.

Reviewed-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>

Thanks,
Shameer

>  		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);
> +	mutex_unlock(&hisi_acc_vdev->reset_mutex);
>  }
> 
>  static void hisi_acc_vf_start_device(struct hisi_acc_vf_core_device
> *hisi_acc_vdev)
> @@ -1108,13 +1108,13 @@ static void
> hisi_acc_vf_pci_aer_reset_done(struct pci_dev *pdev)
>  	 * 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);
> +	mutex_lock(&hisi_acc_vdev->reset_mutex);
>  	hisi_acc_vdev->deferred_reset = true;
>  	if (!mutex_trylock(&hisi_acc_vdev->state_mutex)) {
> -		spin_unlock(&hisi_acc_vdev->reset_lock);
> +		mutex_unlock(&hisi_acc_vdev->reset_mutex);
>  		return;
>  	}
> -	spin_unlock(&hisi_acc_vdev->reset_lock);
> +	mutex_unlock(&hisi_acc_vdev->reset_mutex);
>  	hisi_acc_vf_state_mutex_unlock(hisi_acc_vdev);
>  }
> 
> @@ -1350,6 +1350,7 @@ static int hisi_acc_vfio_pci_migrn_init_dev(struct
> vfio_device *core_vdev)
>  	hisi_acc_vdev->pf_qm = pf_qm;
>  	hisi_acc_vdev->vf_dev = pdev;
>  	mutex_init(&hisi_acc_vdev->state_mutex);
> +	mutex_init(&hisi_acc_vdev->reset_mutex);
> 
>  	core_vdev->migration_flags = VFIO_MIGRATION_STOP_COPY |
> VFIO_MIGRATION_PRE_COPY;
>  	core_vdev->mig_ops = &hisi_acc_vfio_pci_migrn_state_ops;
> diff --git a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.h
> b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.h
> index dcabfeec6ca1..ed5ab332d0f3 100644
> --- a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.h
> +++ b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.h
> @@ -109,8 +109,7 @@ 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 mutex reset_mutex; /* For reset handler */
>  	struct hisi_acc_vf_migration_file *resuming_migf;
>  	struct hisi_acc_vf_migration_file *saving_migf;
>  };
> --
> 2.17.1

RE: [PATCH vfio 2/2] hisi_acc_vfio_pci: Destroy the [state|reset]_mutex on release

[Shameerali Kolothum Thodi]

> -----Original Message-----
> From: Brett Creeley [mailto:brett.creeley@amd.com]
> Sent: 22 November 2023 19:37
> To: jgg@ziepe.ca; yishaih@nvidia.com; liulongfang
> <liulongfang@huawei.com>; Shameerali Kolothum Thodi
> <shameerali.kolothum.thodi@huawei.com>; kevin.tian@intel.com;
> alex.williamson@redhat.com; kvm@vger.kernel.org;
> linux-kernel@vger.kernel.org
> Cc: shannon.nelson@amd.com; brett.creeley@amd.com
> Subject: [PATCH vfio 2/2] hisi_acc_vfio_pci: Destroy the [state|reset]_mutex
> on release
> 
> The [state|reset]_mutex are initialized in vfio init, but
> never destroyed. This isn't required as mutex_destroy()
> doesn't do anything unless lock debugging is enabled.
> However, for completeness, fix it by implementing a
> driver specific release function.
> 
> No fixes tag is added as it doesn't seem worthwhile
> for such a trivial and debug only change.
> 
> Signed-off-by: Brett Creeley <brett.creeley@amd.com>

Reviewed-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>

Thanks.

> ---
>  drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
> b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
> index 2c049b8de4b4..dc1e376e1b8a 100644
> --- a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
> +++ b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
> @@ -1358,10 +1358,20 @@ static int
> hisi_acc_vfio_pci_migrn_init_dev(struct vfio_device *core_vdev)
>  	return vfio_pci_core_init_dev(core_vdev);
>  }
> 
> +static void hisi_acc_vfio_pci_migrn_release_dev(struct vfio_device
> *core_vdev)
> +{
> +	struct hisi_acc_vf_core_device *hisi_acc_vdev =
> container_of(core_vdev,
> +			struct hisi_acc_vf_core_device, core_device.vdev);
> +
> +	mutex_destroy(&hisi_acc_vdev->reset_mutex);
> +	mutex_destroy(&hisi_acc_vdev->state_mutex);
> +	vfio_pci_core_release_dev(core_vdev);
> +}
> +
>  static const struct vfio_device_ops hisi_acc_vfio_pci_migrn_ops = {
>  	.name = "hisi-acc-vfio-pci-migration",
>  	.init = hisi_acc_vfio_pci_migrn_init_dev,
> -	.release = vfio_pci_core_release_dev,
> +	.release = hisi_acc_vfio_pci_migrn_release_dev,
>  	.open_device = hisi_acc_vfio_pci_open_device,
>  	.close_device = hisi_acc_vfio_pci_close_device,
>  	.ioctl = hisi_acc_vfio_pci_ioctl,
> --
> 2.17.1

Re: [PATCH vfio 1/2] hisi_acc_vfio_pci: Change reset_lock to mutex_lock

[Jason Gunthorpe]

On Fri, Nov 24, 2023 at 08:46:58AM +0000, Shameerali Kolothum Thodi wrote:
> > diff --git a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
> > b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
> > index b2f9778c8366..2c049b8de4b4 100644
> > --- a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
> > +++ b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
> > @@ -638,17 +638,17 @@ static void
> >  hisi_acc_vf_state_mutex_unlock(struct hisi_acc_vf_core_device
> > *hisi_acc_vdev)
> >  {
> >  again:
> > -	spin_lock(&hisi_acc_vdev->reset_lock);
> > +	mutex_lock(&hisi_acc_vdev->reset_mutex);
> >  	if (hisi_acc_vdev->deferred_reset) {
> >  		hisi_acc_vdev->deferred_reset = false;
> > -		spin_unlock(&hisi_acc_vdev->reset_lock);
> > +		mutex_unlock(&hisi_acc_vdev->reset_mutex);
> 
> Don't think we have that sleeping while atomic case for this here.
> Same for mlx5 as well. But if the idea is to have a common locking
> across vendor drivers, it is fine.

Yeah, I'm not sure about changing spinlocks to mutex's for no reason..
If we don't sleep and don't hold it for very long then the spinlock is
appropriate

Jason

RE: [PATCH vfio 1/2] hisi_acc_vfio_pci: Change reset_lock to mutex_lock

[Tian, Kevin]

> From: Jason Gunthorpe <jgg@ziepe.ca>
> Sent: Tuesday, November 28, 2023 8:46 AM
> 
> On Fri, Nov 24, 2023 at 08:46:58AM +0000, Shameerali Kolothum Thodi wrote:
> > > diff --git a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
> > > b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
> > > index b2f9778c8366..2c049b8de4b4 100644
> > > --- a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
> > > +++ b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
> > > @@ -638,17 +638,17 @@ static void
> > >  hisi_acc_vf_state_mutex_unlock(struct hisi_acc_vf_core_device
> > > *hisi_acc_vdev)
> > >  {
> > >  again:
> > > -	spin_lock(&hisi_acc_vdev->reset_lock);
> > > +	mutex_lock(&hisi_acc_vdev->reset_mutex);
> > >  	if (hisi_acc_vdev->deferred_reset) {
> > >  		hisi_acc_vdev->deferred_reset = false;
> > > -		spin_unlock(&hisi_acc_vdev->reset_lock);
> > > +		mutex_unlock(&hisi_acc_vdev->reset_mutex);
> >
> > Don't think we have that sleeping while atomic case for this here.
> > Same for mlx5 as well. But if the idea is to have a common locking
> > across vendor drivers, it is fine.
> 
> Yeah, I'm not sure about changing spinlocks to mutex's for no reason..
> If we don't sleep and don't hold it for very long then the spinlock is
> appropriate
> 

It's me suggesting Brett to fix other two drivers, expecting a common
locking pattern would cause less confusion to future new variant drivers.
If both of you don't think it necessary then let's drop it. 😊