Re: [PATCH mlx5-next 3/5] vfio/mlx5: Manage the VF attach/detach callback from the PF

[Yishai Hadas <yishaih@xxxxxxxxxx>]

On 04/05/2022 23:34, Alex Williamson wrote:
> On Wed, 27 Apr 2022 12:31:18 +0300
> Yishai Hadas <yishaih@xxxxxxxxxx> wrote:
>
> > Manage the VF attach/detach callback from the PF.
> >
> > This lets the driver to enable parallel VFs migration as will be
> > introduced in the next patch.
> >
> > Signed-off-by: Yishai Hadas <yishaih@xxxxxxxxxx>
> > Signed-off-by: Leon Romanovsky <leonro@xxxxxxxxxx>
> > ---
> >   drivers/vfio/pci/mlx5/cmd.c  | 59 +++++++++++++++++++++++++++++++++---
> >   drivers/vfio/pci/mlx5/cmd.h  | 23 +++++++++++++-
> >   drivers/vfio/pci/mlx5/main.c | 25 ++++-----------
> >   3 files changed, 82 insertions(+), 25 deletions(-)
> >
> > diff --git a/drivers/vfio/pci/mlx5/cmd.c b/drivers/vfio/pci/mlx5/cmd.c
> > index d608b8167f58..1f84d7b9b9e5 100644
> > --- a/drivers/vfio/pci/mlx5/cmd.c
> > +++ b/drivers/vfio/pci/mlx5/cmd.c
> > @@ -71,21 +71,70 @@ int mlx5vf_cmd_query_vhca_migration_state(struct pci_dev *pdev, u16 vhca_id,
> >   	return ret;
> >   }
> >   
> > -bool mlx5vf_cmd_is_migratable(struct pci_dev *pdev)
> > +static int mlx5fv_vf_event(struct notifier_block *nb,
> > +			   unsigned long event, void *data)
> >   {
> > -	struct mlx5_core_dev *mdev = mlx5_vf_get_core_dev(pdev);
> > +	struct mlx5vf_pci_core_device *mvdev =
> > +		container_of(nb, struct mlx5vf_pci_core_device, nb);
> > +
> > +	mutex_lock(&mvdev->state_mutex);
> > +	switch (event) {
> > +	case MLX5_PF_NOTIFY_ENABLE_VF:
> > +		mvdev->mdev_detach = false;
> > +		break;
> > +	case MLX5_PF_NOTIFY_DISABLE_VF:
> > +		mvdev->mdev_detach = true;
> > +		break;
> > +	default:
> > +		break;
> > +	}
> > +	mlx5vf_state_mutex_unlock(mvdev);
> > +	return 0;
> > +}
> > +
> > +void mlx5vf_cmd_remove_migratable(struct mlx5vf_pci_core_device *mvdev)
> > +{
> > +	mlx5_sriov_blocking_notifier_unregister(mvdev->mdev, mvdev->vf_id,
> > +						&mvdev->nb);
> > +}
> > +
> > +bool mlx5vf_cmd_is_migratable(struct mlx5vf_pci_core_device *mvdev)
> Why did the original implementation take a pdev knowing we're going to
> gut it in the next patch to use an mvdev?  The diff would be easier to
> read.


Agree, in V1 I just combined this patch with the changes from patch #1.


> There's also quite a lot of setup here now, it's no longer a simple
> test whether the device supports migration which makes the name
> misleading.  This looks like a "setup migration" function that should
> return 0/-errno.


Thanks, makes sense.

> > +{
> > +	struct pci_dev *pdev = mvdev->core_device.pdev;
> >   	bool migratable = false;
> > +	int ret;
> >   
> > -	if (!mdev)
> > +	mvdev->mdev = mlx5_vf_get_core_dev(pdev);
> > +	if (!mvdev->mdev)
> >   		return false;
> > +	if (!MLX5_CAP_GEN(mvdev->mdev, migration))
> > +		goto end;
> > +	mvdev->vf_id = pci_iov_vf_id(pdev);
> > +	if (mvdev->vf_id < 0)
> > +		goto end;
> >   
> > -	if (!MLX5_CAP_GEN(mdev, migration))
> > +	mutex_init(&mvdev->state_mutex);
> > +	spin_lock_init(&mvdev->reset_lock);
> > +	mvdev->nb.notifier_call = mlx5fv_vf_event;
> > +	ret = mlx5_sriov_blocking_notifier_register(mvdev->mdev, mvdev->vf_id,
> > +						    &mvdev->nb);
> > +	if (ret)
> >   		goto end;
> >   
> > +	mutex_lock(&mvdev->state_mutex);
> > +	if (mvdev->mdev_detach)
> > +		goto unreg;
> > +
> > +	mlx5vf_state_mutex_unlock(mvdev);
> >   	migratable = true;
> > +	goto end;
> >   
> > +unreg:
> > +	mlx5vf_state_mutex_unlock(mvdev);
> > +	mlx5_sriov_blocking_notifier_unregister(mvdev->mdev, mvdev->vf_id,
> > +						&mvdev->nb);
> >   end:
> > -	mlx5_vf_put_core_dev(mdev);
> > +	mlx5_vf_put_core_dev(mvdev->mdev);
> >   	return migratable;
> >   }
> >   
> > diff --git a/drivers/vfio/pci/mlx5/cmd.h b/drivers/vfio/pci/mlx5/cmd.h
> > index 2da6a1c0ec5c..f47174eab4b8 100644
> > --- a/drivers/vfio/pci/mlx5/cmd.h
> > +++ b/drivers/vfio/pci/mlx5/cmd.h
> > @@ -7,6 +7,7 @@
> >   #define MLX5_VFIO_CMD_H
> >   
> >   #include <linux/kernel.h>
> > +#include <linux/vfio_pci_core.h>
> >   #include <linux/mlx5/driver.h>
> >   
> >   struct mlx5_vf_migration_file {
> > @@ -24,14 +25,34 @@ struct mlx5_vf_migration_file {
> >   	unsigned long last_offset;
> >   };
> >   
> > +struct mlx5vf_pci_core_device {
> > +	struct vfio_pci_core_device core_device;
> > +	int vf_id;
> > +	u16 vhca_id;
> > +	u8 migrate_cap:1;
> > +	u8 deferred_reset:1;
> > +	/* protect migration state */
> > +	struct mutex state_mutex;
> > +	enum vfio_device_mig_state mig_state;
> > +	/* protect the reset_done flow */
> > +	spinlock_t reset_lock;
> > +	struct mlx5_vf_migration_file *resuming_migf;
> > +	struct mlx5_vf_migration_file *saving_migf;
> > +	struct notifier_block nb;
> > +	struct mlx5_core_dev *mdev;
> > +	u8 mdev_detach:1;
> > +};
> > +
> >   int mlx5vf_cmd_suspend_vhca(struct pci_dev *pdev, u16 vhca_id, u16 op_mod);
> >   int mlx5vf_cmd_resume_vhca(struct pci_dev *pdev, u16 vhca_id, u16 op_mod);
> >   int mlx5vf_cmd_query_vhca_migration_state(struct pci_dev *pdev, u16 vhca_id,
> >   					  size_t *state_size);
> >   int mlx5vf_cmd_get_vhca_id(struct pci_dev *pdev, u16 function_id, u16 *vhca_id);
> > -bool mlx5vf_cmd_is_migratable(struct pci_dev *pdev);
> > +bool mlx5vf_cmd_is_migratable(struct mlx5vf_pci_core_device *mvdev);
> > +void mlx5vf_cmd_remove_migratable(struct mlx5vf_pci_core_device *mvdev);
> >   int mlx5vf_cmd_save_vhca_state(struct pci_dev *pdev, u16 vhca_id,
> >   			       struct mlx5_vf_migration_file *migf);
> >   int mlx5vf_cmd_load_vhca_state(struct pci_dev *pdev, u16 vhca_id,
> >   			       struct mlx5_vf_migration_file *migf);
> > +void mlx5vf_state_mutex_unlock(struct mlx5vf_pci_core_device *mvdev);
> >   #endif /* MLX5_VFIO_CMD_H */
> > diff --git a/drivers/vfio/pci/mlx5/main.c b/drivers/vfio/pci/mlx5/main.c
> > index 2578f61eaeae..445c516d38d9 100644
> > --- a/drivers/vfio/pci/mlx5/main.c
> > +++ b/drivers/vfio/pci/mlx5/main.c
> > @@ -17,7 +17,6 @@
> >   #include <linux/uaccess.h>
> >   #include <linux/vfio.h>
> >   #include <linux/sched/mm.h>
> > -#include <linux/vfio_pci_core.h>
> >   #include <linux/anon_inodes.h>
> >   
> >   #include "cmd.h"
> > @@ -25,20 +24,6 @@
> >   /* Arbitrary to prevent userspace from consuming endless memory */
> >   #define MAX_MIGRATION_SIZE (512*1024*1024)
> >   
> > -struct mlx5vf_pci_core_device {
> > -	struct vfio_pci_core_device core_device;
> > -	u16 vhca_id;
> > -	u8 migrate_cap:1;
> > -	u8 deferred_reset:1;
> > -	/* protect migration state */
> > -	struct mutex state_mutex;
> > -	enum vfio_device_mig_state mig_state;
> > -	/* protect the reset_done flow */
> > -	spinlock_t reset_lock;
> > -	struct mlx5_vf_migration_file *resuming_migf;
> > -	struct mlx5_vf_migration_file *saving_migf;
> > -};
> > -
> >   static struct page *
> >   mlx5vf_get_migration_page(struct mlx5_vf_migration_file *migf,
> >   			  unsigned long offset)
> > @@ -444,7 +429,7 @@ mlx5vf_pci_step_device_state_locked(struct mlx5vf_pci_core_device *mvdev,
> >    * This function is called in all state_mutex unlock cases to
> >    * handle a 'deferred_reset' if exists.
> >    */
> > -static void mlx5vf_state_mutex_unlock(struct mlx5vf_pci_core_device *mvdev)
> > +void mlx5vf_state_mutex_unlock(struct mlx5vf_pci_core_device *mvdev)
> >   {
> >   again:
> >   	spin_lock(&mvdev->reset_lock);
> > @@ -597,13 +582,11 @@ static int mlx5vf_pci_probe(struct pci_dev *pdev,
> >   		return -ENOMEM;
> >   	vfio_pci_core_init_device(&mvdev->core_device, pdev, &mlx5vf_pci_ops);
> >   
> > -	if (pdev->is_virtfn && mlx5vf_cmd_is_migratable(pdev)) {
> > +	if (pdev->is_virtfn && mlx5vf_cmd_is_migratable(mvdev)) {
> >   		mvdev->migrate_cap = 1;
> >   		mvdev->core_device.vdev.migration_flags =
> >   			VFIO_MIGRATION_STOP_COPY |
> >   			VFIO_MIGRATION_P2P;
> Why do these aspects of setting up migration remain here?  Do we even
> need this new function to have a return value?  It looks like all of
> this and testing whether the pdev->is_virtfn could be pushed into the
> new function, which could then return void.  Thanks,


Makes sense, will be part of V1, thanks.


> Alex
>
> > -		mutex_init(&mvdev->state_mutex);
> > -		spin_lock_init(&mvdev->reset_lock);
> >   	}
> >   
> >   	ret = vfio_pci_core_register_device(&mvdev->core_device);
> > @@ -614,6 +597,8 @@ static int mlx5vf_pci_probe(struct pci_dev *pdev,
> >   	return 0;
> >   
> >   out_free:
> > +	if (mvdev->migrate_cap)
> > +		mlx5vf_cmd_remove_migratable(mvdev);
> >   	vfio_pci_core_uninit_device(&mvdev->core_device);
> >   	kfree(mvdev);
> >   	return ret;
> > @@ -624,6 +609,8 @@ static void mlx5vf_pci_remove(struct pci_dev *pdev)
> >   	struct mlx5vf_pci_core_device *mvdev = dev_get_drvdata(&pdev->dev);
> >   
> >   	vfio_pci_core_unregister_device(&mvdev->core_device);
> > +	if (mvdev->migrate_cap)
> > +		mlx5vf_cmd_remove_migratable(mvdev);
> >   	vfio_pci_core_uninit_device(&mvdev->core_device);
> >   	kfree(mvdev);
> >   }