[PATCH V3 vfio 0/2] Migration few enhancements

[PATCH V3 vfio 0/2] Migration few enhancements

[Yishai Hadas]

This series includes few enhancements in the migration area and some
fixes in mlx5 driver as of below.

It splits migration ops from the main device ops, this enables a driver
to safely set its migration's ops only when migration is supported and
leave the other code around (e.g., core, driver) clean.

Registering different structs based on the device capabilities might
start to hit combinatorial explosion when we'll introduce ops for dirty
logging that may be optional too.

As part of that, adapt mlx5 to this scheme and fix some issues around
its migration capable usage.

V3:
- Fix a typo in vfio_pci_core_register_device(), to check for both set & get 'mig_ops'.

V2: https://lore.kernel.org/all/20220616170118.497620ba.alex.williamson@redhat.com/T/
- Validate ops construction and migration mandatory flags on
  registration as was asked by Kevin and Jason.
- As of the above move to use a single 'mig_ops' check in vfio before
  calling the driver.

V1: https://lore.kernel.org/all/20220626083958.54175-1-yishaih@nvidia.com/
- Add a comment about the required usage of 'mig_ops' as was suggested
  by Alex.
- Add Kevin's Reviewed-by tag.

V0:
https://lore.kernel.org/all/20220616170118.497620ba.alex.williamson@redhat.com/T/

Yishai
Yishai Hadas (2):
  vfio/mlx5: Protect mlx5vf_disable_fds() upon close device
  vfio: Split migration ops from main device ops

 .../vfio/pci/hisilicon/hisi_acc_vfio_pci.c    | 11 +++++--
 drivers/vfio/pci/mlx5/cmd.c                   | 14 ++++++++-
 drivers/vfio/pci/mlx5/cmd.h                   |  4 ++-
 drivers/vfio/pci/mlx5/main.c                  | 11 ++++---
 drivers/vfio/pci/vfio_pci_core.c              |  7 +++++
 drivers/vfio/vfio.c                           | 11 ++++---
 include/linux/vfio.h                          | 30 ++++++++++++-------
 7 files changed, 63 insertions(+), 25 deletions(-)

-- 
2.18.1

[PATCH V3 vfio 1/2] vfio/mlx5: Protect mlx5vf_disable_fds() upon close device

[Yishai Hadas]

Protect mlx5vf_disable_fds() upon close device to be called under the
state mutex as done in all other places.

This will prevent a race with any other flow which calls
mlx5vf_disable_fds() as of health/recovery upon
MLX5_PF_NOTIFY_DISABLE_VF event.

Encapsulate this functionality in a separate function named
mlx5vf_cmd_close_migratable() to consider migration caps and for further
usage upon close device.

Fixes: 6fadb021266d ("vfio/mlx5: Implement vfio_pci driver for mlx5 devices")
Reviewed-by: Kevin Tian <kevin.tian@intel.com>
Signed-off-by: Yishai Hadas <yishaih@nvidia.com>
---
 drivers/vfio/pci/mlx5/cmd.c  | 10 ++++++++++
 drivers/vfio/pci/mlx5/cmd.h  |  1 +
 drivers/vfio/pci/mlx5/main.c |  2 +-
 3 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/vfio/pci/mlx5/cmd.c b/drivers/vfio/pci/mlx5/cmd.c
index 9b9f33ca270a..cdd0c667dc77 100644
--- a/drivers/vfio/pci/mlx5/cmd.c
+++ b/drivers/vfio/pci/mlx5/cmd.c
@@ -88,6 +88,16 @@ static int mlx5fv_vf_event(struct notifier_block *nb,
 	return 0;
 }
 
+void mlx5vf_cmd_close_migratable(struct mlx5vf_pci_core_device *mvdev)
+{
+	if (!mvdev->migrate_cap)
+		return;
+
+	mutex_lock(&mvdev->state_mutex);
+	mlx5vf_disable_fds(mvdev);
+	mlx5vf_state_mutex_unlock(mvdev);
+}
+
 void mlx5vf_cmd_remove_migratable(struct mlx5vf_pci_core_device *mvdev)
 {
 	if (!mvdev->migrate_cap)
diff --git a/drivers/vfio/pci/mlx5/cmd.h b/drivers/vfio/pci/mlx5/cmd.h
index 6c3112fdd8b1..aa692d9ce656 100644
--- a/drivers/vfio/pci/mlx5/cmd.h
+++ b/drivers/vfio/pci/mlx5/cmd.h
@@ -64,6 +64,7 @@ int mlx5vf_cmd_query_vhca_migration_state(struct mlx5vf_pci_core_device *mvdev,
 					  size_t *state_size);
 void mlx5vf_cmd_set_migratable(struct mlx5vf_pci_core_device *mvdev);
 void mlx5vf_cmd_remove_migratable(struct mlx5vf_pci_core_device *mvdev);
+void mlx5vf_cmd_close_migratable(struct mlx5vf_pci_core_device *mvdev);
 int mlx5vf_cmd_save_vhca_state(struct mlx5vf_pci_core_device *mvdev,
 			       struct mlx5_vf_migration_file *migf);
 int mlx5vf_cmd_load_vhca_state(struct mlx5vf_pci_core_device *mvdev,
diff --git a/drivers/vfio/pci/mlx5/main.c b/drivers/vfio/pci/mlx5/main.c
index 0558d0649ddb..d754990f0662 100644
--- a/drivers/vfio/pci/mlx5/main.c
+++ b/drivers/vfio/pci/mlx5/main.c
@@ -570,7 +570,7 @@ static void mlx5vf_pci_close_device(struct vfio_device *core_vdev)
 	struct mlx5vf_pci_core_device *mvdev = container_of(
 		core_vdev, struct mlx5vf_pci_core_device, core_device.vdev);
 
-	mlx5vf_disable_fds(mvdev);
+	mlx5vf_cmd_close_migratable(mvdev);
 	vfio_pci_core_close_device(core_vdev);
 }
 
-- 
2.18.1

[PATCH V3 vfio 2/2] vfio: Split migration ops from main device ops

[Yishai Hadas]

vfio core checks whether the driver sets some migration op (e.g.
set_state/get_state) and accordingly calls its op.

However, currently mlx5 driver sets the above ops without regards to its
migration caps.

This might lead to unexpected usage/Oops if user space may call to the
above ops even if the driver doesn't support migration. As for example,
the migration state_mutex is not initialized in that case.

The cleanest way to manage that seems to split the migration ops from
the main device ops, this will let the driver setting them separately
from the main ops when it's applicable.

As part of that, validate ops construction on registration and include a
check for VFIO_MIGRATION_STOP_COPY since the uAPI claims it must be set
in migration_flags.

HISI driver was changed as well to match this scheme.

This scheme may enable down the road to come with some extra group of
ops (e.g. DMA log) that can be set without regards to the other options
based on driver caps.

Fixes: 6fadb021266d ("vfio/mlx5: Implement vfio_pci driver for mlx5 devices")
Reviewed-by: Kevin Tian <kevin.tian@intel.com>
Signed-off-by: Yishai Hadas <yishaih@nvidia.com>
---
 .../vfio/pci/hisilicon/hisi_acc_vfio_pci.c    | 11 +++++--
 drivers/vfio/pci/mlx5/cmd.c                   |  4 ++-
 drivers/vfio/pci/mlx5/cmd.h                   |  3 +-
 drivers/vfio/pci/mlx5/main.c                  |  9 ++++--
 drivers/vfio/pci/vfio_pci_core.c              |  7 +++++
 drivers/vfio/vfio.c                           | 11 ++++---
 include/linux/vfio.h                          | 30 ++++++++++++-------
 7 files changed, 51 insertions(+), 24 deletions(-)

diff --git a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
index 4def43f5f7b6..ea762e28c1cc 100644
--- a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
+++ b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
@@ -1185,7 +1185,7 @@ static int hisi_acc_vfio_pci_open_device(struct vfio_device *core_vdev)
 	if (ret)
 		return ret;
 
-	if (core_vdev->ops->migration_set_state) {
+	if (core_vdev->mig_ops) {
 		ret = hisi_acc_vf_qm_init(hisi_acc_vdev);
 		if (ret) {
 			vfio_pci_core_disable(vdev);
@@ -1208,6 +1208,11 @@ static void hisi_acc_vfio_pci_close_device(struct vfio_device *core_vdev)
 	vfio_pci_core_close_device(core_vdev);
 }
 
+static const struct vfio_migration_ops hisi_acc_vfio_pci_migrn_state_ops = {
+	.migration_set_state = hisi_acc_vfio_pci_set_device_state,
+	.migration_get_state = hisi_acc_vfio_pci_get_device_state,
+};
+
 static const struct vfio_device_ops hisi_acc_vfio_pci_migrn_ops = {
 	.name = "hisi-acc-vfio-pci-migration",
 	.open_device = hisi_acc_vfio_pci_open_device,
@@ -1219,8 +1224,6 @@ static const struct vfio_device_ops hisi_acc_vfio_pci_migrn_ops = {
 	.mmap = hisi_acc_vfio_pci_mmap,
 	.request = vfio_pci_core_request,
 	.match = vfio_pci_core_match,
-	.migration_set_state = hisi_acc_vfio_pci_set_device_state,
-	.migration_get_state = hisi_acc_vfio_pci_get_device_state,
 };
 
 static const struct vfio_device_ops hisi_acc_vfio_pci_ops = {
@@ -1272,6 +1275,8 @@ static int hisi_acc_vfio_pci_probe(struct pci_dev *pdev, const struct pci_device
 		if (!ret) {
 			vfio_pci_core_init_device(&hisi_acc_vdev->core_device, pdev,
 						  &hisi_acc_vfio_pci_migrn_ops);
+			hisi_acc_vdev->core_device.vdev.mig_ops =
+					&hisi_acc_vfio_pci_migrn_state_ops;
 		} else {
 			pci_warn(pdev, "migration support failed, continue with generic interface\n");
 			vfio_pci_core_init_device(&hisi_acc_vdev->core_device, pdev,
diff --git a/drivers/vfio/pci/mlx5/cmd.c b/drivers/vfio/pci/mlx5/cmd.c
index cdd0c667dc77..dd5d7bfe0a49 100644
--- a/drivers/vfio/pci/mlx5/cmd.c
+++ b/drivers/vfio/pci/mlx5/cmd.c
@@ -108,7 +108,8 @@ void mlx5vf_cmd_remove_migratable(struct mlx5vf_pci_core_device *mvdev)
 	destroy_workqueue(mvdev->cb_wq);
 }
 
-void mlx5vf_cmd_set_migratable(struct mlx5vf_pci_core_device *mvdev)
+void mlx5vf_cmd_set_migratable(struct mlx5vf_pci_core_device *mvdev,
+			       const struct vfio_migration_ops *mig_ops)
 {
 	struct pci_dev *pdev = mvdev->core_device.pdev;
 	int ret;
@@ -149,6 +150,7 @@ void mlx5vf_cmd_set_migratable(struct mlx5vf_pci_core_device *mvdev)
 	mvdev->core_device.vdev.migration_flags =
 		VFIO_MIGRATION_STOP_COPY |
 		VFIO_MIGRATION_P2P;
+	mvdev->core_device.vdev.mig_ops = mig_ops;
 
 end:
 	mlx5_vf_put_core_dev(mvdev->mdev);
diff --git a/drivers/vfio/pci/mlx5/cmd.h b/drivers/vfio/pci/mlx5/cmd.h
index aa692d9ce656..8208f4701a90 100644
--- a/drivers/vfio/pci/mlx5/cmd.h
+++ b/drivers/vfio/pci/mlx5/cmd.h
@@ -62,7 +62,8 @@ int mlx5vf_cmd_suspend_vhca(struct mlx5vf_pci_core_device *mvdev, u16 op_mod);
 int mlx5vf_cmd_resume_vhca(struct mlx5vf_pci_core_device *mvdev, u16 op_mod);
 int mlx5vf_cmd_query_vhca_migration_state(struct mlx5vf_pci_core_device *mvdev,
 					  size_t *state_size);
-void mlx5vf_cmd_set_migratable(struct mlx5vf_pci_core_device *mvdev);
+void mlx5vf_cmd_set_migratable(struct mlx5vf_pci_core_device *mvdev,
+			       const struct vfio_migration_ops *mig_ops);
 void mlx5vf_cmd_remove_migratable(struct mlx5vf_pci_core_device *mvdev);
 void mlx5vf_cmd_close_migratable(struct mlx5vf_pci_core_device *mvdev);
 int mlx5vf_cmd_save_vhca_state(struct mlx5vf_pci_core_device *mvdev,
diff --git a/drivers/vfio/pci/mlx5/main.c b/drivers/vfio/pci/mlx5/main.c
index d754990f0662..a9b63d15c5d3 100644
--- a/drivers/vfio/pci/mlx5/main.c
+++ b/drivers/vfio/pci/mlx5/main.c
@@ -574,6 +574,11 @@ static void mlx5vf_pci_close_device(struct vfio_device *core_vdev)
 	vfio_pci_core_close_device(core_vdev);
 }
 
+static const struct vfio_migration_ops mlx5vf_pci_mig_ops = {
+	.migration_set_state = mlx5vf_pci_set_device_state,
+	.migration_get_state = mlx5vf_pci_get_device_state,
+};
+
 static const struct vfio_device_ops mlx5vf_pci_ops = {
 	.name = "mlx5-vfio-pci",
 	.open_device = mlx5vf_pci_open_device,
@@ -585,8 +590,6 @@ static const struct vfio_device_ops mlx5vf_pci_ops = {
 	.mmap = vfio_pci_core_mmap,
 	.request = vfio_pci_core_request,
 	.match = vfio_pci_core_match,
-	.migration_set_state = mlx5vf_pci_set_device_state,
-	.migration_get_state = mlx5vf_pci_get_device_state,
 };
 
 static int mlx5vf_pci_probe(struct pci_dev *pdev,
@@ -599,7 +602,7 @@ static int mlx5vf_pci_probe(struct pci_dev *pdev,
 	if (!mvdev)
 		return -ENOMEM;
 	vfio_pci_core_init_device(&mvdev->core_device, pdev, &mlx5vf_pci_ops);
-	mlx5vf_cmd_set_migratable(mvdev);
+	mlx5vf_cmd_set_migratable(mvdev, &mlx5vf_pci_mig_ops);
 	dev_set_drvdata(&pdev->dev, &mvdev->core_device);
 	ret = vfio_pci_core_register_device(&mvdev->core_device);
 	if (ret)
diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
index a0d69ddaf90d..cf875309dac0 100644
--- a/drivers/vfio/pci/vfio_pci_core.c
+++ b/drivers/vfio/pci/vfio_pci_core.c
@@ -1855,6 +1855,13 @@ int vfio_pci_core_register_device(struct vfio_pci_core_device *vdev)
 	if (pdev->hdr_type != PCI_HEADER_TYPE_NORMAL)
 		return -EINVAL;
 
+	if (vdev->vdev.mig_ops) {
+		if ((!(vdev->vdev.mig_ops->migration_get_state &&
+		       vdev->vdev.mig_ops->migration_set_state)) ||
+		    (!(vdev->vdev.migration_flags & VFIO_MIGRATION_STOP_COPY)))
+			return -EINVAL;
+	}
+
 	/*
 	 * Prevent binding to PFs with VFs enabled, the VFs might be in use
 	 * by the host or other users.  We cannot capture the VFs if they
diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
index e22be13e6771..ccbd106b95d8 100644
--- a/drivers/vfio/vfio.c
+++ b/drivers/vfio/vfio.c
@@ -1534,8 +1534,7 @@ vfio_ioctl_device_feature_mig_device_state(struct vfio_device *device,
 	struct file *filp = NULL;
 	int ret;
 
-	if (!device->ops->migration_set_state ||
-	    !device->ops->migration_get_state)
+	if (!device->mig_ops)
 		return -ENOTTY;
 
 	ret = vfio_check_feature(flags, argsz,
@@ -1551,7 +1550,8 @@ vfio_ioctl_device_feature_mig_device_state(struct vfio_device *device,
 	if (flags & VFIO_DEVICE_FEATURE_GET) {
 		enum vfio_device_mig_state curr_state;
 
-		ret = device->ops->migration_get_state(device, &curr_state);
+		ret = device->mig_ops->migration_get_state(device,
+							   &curr_state);
 		if (ret)
 			return ret;
 		mig.device_state = curr_state;
@@ -1559,7 +1559,7 @@ vfio_ioctl_device_feature_mig_device_state(struct vfio_device *device,
 	}
 
 	/* Handle the VFIO_DEVICE_FEATURE_SET */
-	filp = device->ops->migration_set_state(device, mig.device_state);
+	filp = device->mig_ops->migration_set_state(device, mig.device_state);
 	if (IS_ERR(filp) || !filp)
 		goto out_copy;
 
@@ -1582,8 +1582,7 @@ static int vfio_ioctl_device_feature_migration(struct vfio_device *device,
 	};
 	int ret;
 
-	if (!device->ops->migration_set_state ||
-	    !device->ops->migration_get_state)
+	if (!device->mig_ops)
 		return -ENOTTY;
 
 	ret = vfio_check_feature(flags, argsz, VFIO_DEVICE_FEATURE_GET,
diff --git a/include/linux/vfio.h b/include/linux/vfio.h
index aa888cc51757..d6c592565be7 100644
--- a/include/linux/vfio.h
+++ b/include/linux/vfio.h
@@ -32,6 +32,11 @@ struct vfio_device_set {
 struct vfio_device {
 	struct device *dev;
 	const struct vfio_device_ops *ops;
+	/*
+	 * mig_ops is a static property of the vfio_device which must be set
+	 * prior to registering the vfio_device.
+	 */
+	const struct vfio_migration_ops *mig_ops;
 	struct vfio_group *group;
 	struct vfio_device_set *dev_set;
 	struct list_head dev_set_list;
@@ -61,16 +66,6 @@ struct vfio_device {
  *         match, -errno for abort (ex. match with insufficient or incorrect
  *         additional args)
  * @device_feature: Optional, fill in the VFIO_DEVICE_FEATURE ioctl
- * @migration_set_state: Optional callback to change the migration state for
- *         devices that support migration. It's mandatory for
- *         VFIO_DEVICE_FEATURE_MIGRATION migration support.
- *         The returned FD is used for data transfer according to the FSM
- *         definition. The driver is responsible to ensure that FD reaches end
- *         of stream or error whenever the migration FSM leaves a data transfer
- *         state or before close_device() returns.
- * @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.
  */
 struct vfio_device_ops {
 	char	*name;
@@ -87,6 +82,21 @@ struct vfio_device_ops {
 	int	(*match)(struct vfio_device *vdev, char *buf);
 	int	(*device_feature)(struct vfio_device *device, u32 flags,
 				  void __user *arg, size_t argsz);
+};
+
+/**
+ * @migration_set_state: Optional callback to change the migration state for
+ *         devices that support migration. It's mandatory for
+ *         VFIO_DEVICE_FEATURE_MIGRATION migration support.
+ *         The returned FD is used for data transfer according to the FSM
+ *         definition. The driver is responsible to ensure that FD reaches end
+ *         of stream or error whenever the migration FSM leaves a data transfer
+ *         state or before close_device() returns.
+ * @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.
+ */
+struct vfio_migration_ops {
 	struct file *(*migration_set_state)(
 		struct vfio_device *device,
 		enum vfio_device_mig_state new_state);
-- 
2.18.1

Re: [PATCH V3 vfio 2/2] vfio: Split migration ops from main device ops

[Alex Williamson]

On Tue, 28 Jun 2022 18:59:10 +0300
Yishai Hadas <yishaih@nvidia.com> wrote:

> vfio core checks whether the driver sets some migration op (e.g.
> set_state/get_state) and accordingly calls its op.
> 
> However, currently mlx5 driver sets the above ops without regards to its
> migration caps.
> 
> This might lead to unexpected usage/Oops if user space may call to the
> above ops even if the driver doesn't support migration. As for example,
> the migration state_mutex is not initialized in that case.
> 
> The cleanest way to manage that seems to split the migration ops from
> the main device ops, this will let the driver setting them separately
> from the main ops when it's applicable.
> 
> As part of that, validate ops construction on registration and include a
> check for VFIO_MIGRATION_STOP_COPY since the uAPI claims it must be set
> in migration_flags.
> 
> HISI driver was changed as well to match this scheme.
> 
> This scheme may enable down the road to come with some extra group of
> ops (e.g. DMA log) that can be set without regards to the other options
> based on driver caps.
> 
> Fixes: 6fadb021266d ("vfio/mlx5: Implement vfio_pci driver for mlx5 devices")
> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
> Signed-off-by: Yishai Hadas <yishaih@nvidia.com>
> ---
>  .../vfio/pci/hisilicon/hisi_acc_vfio_pci.c    | 11 +++++--
>  drivers/vfio/pci/mlx5/cmd.c                   |  4 ++-
>  drivers/vfio/pci/mlx5/cmd.h                   |  3 +-
>  drivers/vfio/pci/mlx5/main.c                  |  9 ++++--
>  drivers/vfio/pci/vfio_pci_core.c              |  7 +++++
>  drivers/vfio/vfio.c                           | 11 ++++---
>  include/linux/vfio.h                          | 30 ++++++++++++-------
>  7 files changed, 51 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
> index 4def43f5f7b6..ea762e28c1cc 100644
> --- a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
> +++ b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
> @@ -1185,7 +1185,7 @@ static int hisi_acc_vfio_pci_open_device(struct vfio_device *core_vdev)
>  	if (ret)
>  		return ret;
>  
> -	if (core_vdev->ops->migration_set_state) {
> +	if (core_vdev->mig_ops) {
>  		ret = hisi_acc_vf_qm_init(hisi_acc_vdev);
>  		if (ret) {
>  			vfio_pci_core_disable(vdev);
> @@ -1208,6 +1208,11 @@ static void hisi_acc_vfio_pci_close_device(struct vfio_device *core_vdev)
>  	vfio_pci_core_close_device(core_vdev);
>  }
>  
> +static const struct vfio_migration_ops hisi_acc_vfio_pci_migrn_state_ops = {
> +	.migration_set_state = hisi_acc_vfio_pci_set_device_state,
> +	.migration_get_state = hisi_acc_vfio_pci_get_device_state,
> +};
> +
>  static const struct vfio_device_ops hisi_acc_vfio_pci_migrn_ops = {
>  	.name = "hisi-acc-vfio-pci-migration",
>  	.open_device = hisi_acc_vfio_pci_open_device,
> @@ -1219,8 +1224,6 @@ static const struct vfio_device_ops hisi_acc_vfio_pci_migrn_ops = {
>  	.mmap = hisi_acc_vfio_pci_mmap,
>  	.request = vfio_pci_core_request,
>  	.match = vfio_pci_core_match,
> -	.migration_set_state = hisi_acc_vfio_pci_set_device_state,
> -	.migration_get_state = hisi_acc_vfio_pci_get_device_state,
>  };
>  
>  static const struct vfio_device_ops hisi_acc_vfio_pci_ops = {
> @@ -1272,6 +1275,8 @@ static int hisi_acc_vfio_pci_probe(struct pci_dev *pdev, const struct pci_device
>  		if (!ret) {
>  			vfio_pci_core_init_device(&hisi_acc_vdev->core_device, pdev,
>  						  &hisi_acc_vfio_pci_migrn_ops);
> +			hisi_acc_vdev->core_device.vdev.mig_ops =
> +					&hisi_acc_vfio_pci_migrn_state_ops;
>  		} else {
>  			pci_warn(pdev, "migration support failed, continue with generic interface\n");
>  			vfio_pci_core_init_device(&hisi_acc_vdev->core_device, pdev,
> diff --git a/drivers/vfio/pci/mlx5/cmd.c b/drivers/vfio/pci/mlx5/cmd.c
> index cdd0c667dc77..dd5d7bfe0a49 100644
> --- a/drivers/vfio/pci/mlx5/cmd.c
> +++ b/drivers/vfio/pci/mlx5/cmd.c
> @@ -108,7 +108,8 @@ void mlx5vf_cmd_remove_migratable(struct mlx5vf_pci_core_device *mvdev)
>  	destroy_workqueue(mvdev->cb_wq);
>  }
>  
> -void mlx5vf_cmd_set_migratable(struct mlx5vf_pci_core_device *mvdev)
> +void mlx5vf_cmd_set_migratable(struct mlx5vf_pci_core_device *mvdev,
> +			       const struct vfio_migration_ops *mig_ops)
>  {
>  	struct pci_dev *pdev = mvdev->core_device.pdev;
>  	int ret;
> @@ -149,6 +150,7 @@ void mlx5vf_cmd_set_migratable(struct mlx5vf_pci_core_device *mvdev)
>  	mvdev->core_device.vdev.migration_flags =
>  		VFIO_MIGRATION_STOP_COPY |
>  		VFIO_MIGRATION_P2P;
> +	mvdev->core_device.vdev.mig_ops = mig_ops;
>  
>  end:
>  	mlx5_vf_put_core_dev(mvdev->mdev);
> diff --git a/drivers/vfio/pci/mlx5/cmd.h b/drivers/vfio/pci/mlx5/cmd.h
> index aa692d9ce656..8208f4701a90 100644
> --- a/drivers/vfio/pci/mlx5/cmd.h
> +++ b/drivers/vfio/pci/mlx5/cmd.h
> @@ -62,7 +62,8 @@ int mlx5vf_cmd_suspend_vhca(struct mlx5vf_pci_core_device *mvdev, u16 op_mod);
>  int mlx5vf_cmd_resume_vhca(struct mlx5vf_pci_core_device *mvdev, u16 op_mod);
>  int mlx5vf_cmd_query_vhca_migration_state(struct mlx5vf_pci_core_device *mvdev,
>  					  size_t *state_size);
> -void mlx5vf_cmd_set_migratable(struct mlx5vf_pci_core_device *mvdev);
> +void mlx5vf_cmd_set_migratable(struct mlx5vf_pci_core_device *mvdev,
> +			       const struct vfio_migration_ops *mig_ops);
>  void mlx5vf_cmd_remove_migratable(struct mlx5vf_pci_core_device *mvdev);
>  void mlx5vf_cmd_close_migratable(struct mlx5vf_pci_core_device *mvdev);
>  int mlx5vf_cmd_save_vhca_state(struct mlx5vf_pci_core_device *mvdev,
> diff --git a/drivers/vfio/pci/mlx5/main.c b/drivers/vfio/pci/mlx5/main.c
> index d754990f0662..a9b63d15c5d3 100644
> --- a/drivers/vfio/pci/mlx5/main.c
> +++ b/drivers/vfio/pci/mlx5/main.c
> @@ -574,6 +574,11 @@ static void mlx5vf_pci_close_device(struct vfio_device *core_vdev)
>  	vfio_pci_core_close_device(core_vdev);
>  }
>  
> +static const struct vfio_migration_ops mlx5vf_pci_mig_ops = {
> +	.migration_set_state = mlx5vf_pci_set_device_state,
> +	.migration_get_state = mlx5vf_pci_get_device_state,
> +};
> +
>  static const struct vfio_device_ops mlx5vf_pci_ops = {
>  	.name = "mlx5-vfio-pci",
>  	.open_device = mlx5vf_pci_open_device,
> @@ -585,8 +590,6 @@ static const struct vfio_device_ops mlx5vf_pci_ops = {
>  	.mmap = vfio_pci_core_mmap,
>  	.request = vfio_pci_core_request,
>  	.match = vfio_pci_core_match,
> -	.migration_set_state = mlx5vf_pci_set_device_state,
> -	.migration_get_state = mlx5vf_pci_get_device_state,
>  };
>  
>  static int mlx5vf_pci_probe(struct pci_dev *pdev,
> @@ -599,7 +602,7 @@ static int mlx5vf_pci_probe(struct pci_dev *pdev,
>  	if (!mvdev)
>  		return -ENOMEM;
>  	vfio_pci_core_init_device(&mvdev->core_device, pdev, &mlx5vf_pci_ops);
> -	mlx5vf_cmd_set_migratable(mvdev);
> +	mlx5vf_cmd_set_migratable(mvdev, &mlx5vf_pci_mig_ops);
>  	dev_set_drvdata(&pdev->dev, &mvdev->core_device);
>  	ret = vfio_pci_core_register_device(&mvdev->core_device);
>  	if (ret)
> diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
> index a0d69ddaf90d..cf875309dac0 100644
> --- a/drivers/vfio/pci/vfio_pci_core.c
> +++ b/drivers/vfio/pci/vfio_pci_core.c
> @@ -1855,6 +1855,13 @@ int vfio_pci_core_register_device(struct vfio_pci_core_device *vdev)
>  	if (pdev->hdr_type != PCI_HEADER_TYPE_NORMAL)
>  		return -EINVAL;
>  
> +	if (vdev->vdev.mig_ops) {
> +		if ((!(vdev->vdev.mig_ops->migration_get_state &&
> +		       vdev->vdev.mig_ops->migration_set_state)) ||
> +		    (!(vdev->vdev.migration_flags & VFIO_MIGRATION_STOP_COPY)))
> +			return -EINVAL;

A bit excessive on the parenthesis, a logical NOT is just below parens
and well above a logical OR on order of operations, so it should be
fine as:

	if (!(vdev->vdev.mig_ops->migration_get_state &&
	      vdev->vdev.mig_ops->migration_set_state) ||
	    !(vdev->vdev.migration_flags & VFIO_MIGRATION_STOP_COPY))
		return -EINVAL;

Looks ok to me otherwise, so if there are no other changes maybe I can
roll that in on commit.  Thanks,

Alex

> +	}
> +
>  	/*
>  	 * Prevent binding to PFs with VFs enabled, the VFs might be in use
>  	 * by the host or other users.  We cannot capture the VFs if they
> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
> index e22be13e6771..ccbd106b95d8 100644
> --- a/drivers/vfio/vfio.c
> +++ b/drivers/vfio/vfio.c
> @@ -1534,8 +1534,7 @@ vfio_ioctl_device_feature_mig_device_state(struct vfio_device *device,
>  	struct file *filp = NULL;
>  	int ret;
>  
> -	if (!device->ops->migration_set_state ||
> -	    !device->ops->migration_get_state)
> +	if (!device->mig_ops)
>  		return -ENOTTY;
>  
>  	ret = vfio_check_feature(flags, argsz,
> @@ -1551,7 +1550,8 @@ vfio_ioctl_device_feature_mig_device_state(struct vfio_device *device,
>  	if (flags & VFIO_DEVICE_FEATURE_GET) {
>  		enum vfio_device_mig_state curr_state;
>  
> -		ret = device->ops->migration_get_state(device, &curr_state);
> +		ret = device->mig_ops->migration_get_state(device,
> +							   &curr_state);
>  		if (ret)
>  			return ret;
>  		mig.device_state = curr_state;
> @@ -1559,7 +1559,7 @@ vfio_ioctl_device_feature_mig_device_state(struct vfio_device *device,
>  	}
>  
>  	/* Handle the VFIO_DEVICE_FEATURE_SET */
> -	filp = device->ops->migration_set_state(device, mig.device_state);
> +	filp = device->mig_ops->migration_set_state(device, mig.device_state);
>  	if (IS_ERR(filp) || !filp)
>  		goto out_copy;
>  
> @@ -1582,8 +1582,7 @@ static int vfio_ioctl_device_feature_migration(struct vfio_device *device,
>  	};
>  	int ret;
>  
> -	if (!device->ops->migration_set_state ||
> -	    !device->ops->migration_get_state)
> +	if (!device->mig_ops)
>  		return -ENOTTY;
>  
>  	ret = vfio_check_feature(flags, argsz, VFIO_DEVICE_FEATURE_GET,
> diff --git a/include/linux/vfio.h b/include/linux/vfio.h
> index aa888cc51757..d6c592565be7 100644
> --- a/include/linux/vfio.h
> +++ b/include/linux/vfio.h
> @@ -32,6 +32,11 @@ struct vfio_device_set {
>  struct vfio_device {
>  	struct device *dev;
>  	const struct vfio_device_ops *ops;
> +	/*
> +	 * mig_ops is a static property of the vfio_device which must be set
> +	 * prior to registering the vfio_device.
> +	 */
> +	const struct vfio_migration_ops *mig_ops;
>  	struct vfio_group *group;
>  	struct vfio_device_set *dev_set;
>  	struct list_head dev_set_list;
> @@ -61,16 +66,6 @@ struct vfio_device {
>   *         match, -errno for abort (ex. match with insufficient or incorrect
>   *         additional args)
>   * @device_feature: Optional, fill in the VFIO_DEVICE_FEATURE ioctl
> - * @migration_set_state: Optional callback to change the migration state for
> - *         devices that support migration. It's mandatory for
> - *         VFIO_DEVICE_FEATURE_MIGRATION migration support.
> - *         The returned FD is used for data transfer according to the FSM
> - *         definition. The driver is responsible to ensure that FD reaches end
> - *         of stream or error whenever the migration FSM leaves a data transfer
> - *         state or before close_device() returns.
> - * @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.
>   */
>  struct vfio_device_ops {
>  	char	*name;
> @@ -87,6 +82,21 @@ struct vfio_device_ops {
>  	int	(*match)(struct vfio_device *vdev, char *buf);
>  	int	(*device_feature)(struct vfio_device *device, u32 flags,
>  				  void __user *arg, size_t argsz);
> +};
> +
> +/**
> + * @migration_set_state: Optional callback to change the migration state for
> + *         devices that support migration. It's mandatory for
> + *         VFIO_DEVICE_FEATURE_MIGRATION migration support.
> + *         The returned FD is used for data transfer according to the FSM
> + *         definition. The driver is responsible to ensure that FD reaches end
> + *         of stream or error whenever the migration FSM leaves a data transfer
> + *         state or before close_device() returns.
> + * @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.
> + */
> +struct vfio_migration_ops {
>  	struct file *(*migration_set_state)(
>  		struct vfio_device *device,
>  		enum vfio_device_mig_state new_state);

Re: [PATCH V3 vfio 2/2] vfio: Split migration ops from main device ops

[Yishai Hadas]

On 28/06/2022 23:16, Alex Williamson wrote:
> On Tue, 28 Jun 2022 18:59:10 +0300
> Yishai Hadas <yishaih@nvidia.com> wrote:
>
>> vfio core checks whether the driver sets some migration op (e.g.
>> set_state/get_state) and accordingly calls its op.
>>
>> However, currently mlx5 driver sets the above ops without regards to its
>> migration caps.
>>
>> This might lead to unexpected usage/Oops if user space may call to the
>> above ops even if the driver doesn't support migration. As for example,
>> the migration state_mutex is not initialized in that case.
>>
>> The cleanest way to manage that seems to split the migration ops from
>> the main device ops, this will let the driver setting them separately
>> from the main ops when it's applicable.
>>
>> As part of that, validate ops construction on registration and include a
>> check for VFIO_MIGRATION_STOP_COPY since the uAPI claims it must be set
>> in migration_flags.
>>
>> HISI driver was changed as well to match this scheme.
>>
>> This scheme may enable down the road to come with some extra group of
>> ops (e.g. DMA log) that can be set without regards to the other options
>> based on driver caps.
>>
>> Fixes: 6fadb021266d ("vfio/mlx5: Implement vfio_pci driver for mlx5 devices")
>> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
>> Signed-off-by: Yishai Hadas <yishaih@nvidia.com>
>> ---
>>   .../vfio/pci/hisilicon/hisi_acc_vfio_pci.c    | 11 +++++--
>>   drivers/vfio/pci/mlx5/cmd.c                   |  4 ++-
>>   drivers/vfio/pci/mlx5/cmd.h                   |  3 +-
>>   drivers/vfio/pci/mlx5/main.c                  |  9 ++++--
>>   drivers/vfio/pci/vfio_pci_core.c              |  7 +++++
>>   drivers/vfio/vfio.c                           | 11 ++++---
>>   include/linux/vfio.h                          | 30 ++++++++++++-------
>>   7 files changed, 51 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
>> index 4def43f5f7b6..ea762e28c1cc 100644
>> --- a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
>> +++ b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
>> @@ -1185,7 +1185,7 @@ static int hisi_acc_vfio_pci_open_device(struct vfio_device *core_vdev)
>>   	if (ret)
>>   		return ret;
>>   
>> -	if (core_vdev->ops->migration_set_state) {
>> +	if (core_vdev->mig_ops) {
>>   		ret = hisi_acc_vf_qm_init(hisi_acc_vdev);
>>   		if (ret) {
>>   			vfio_pci_core_disable(vdev);
>> @@ -1208,6 +1208,11 @@ static void hisi_acc_vfio_pci_close_device(struct vfio_device *core_vdev)
>>   	vfio_pci_core_close_device(core_vdev);
>>   }
>>   
>> +static const struct vfio_migration_ops hisi_acc_vfio_pci_migrn_state_ops = {
>> +	.migration_set_state = hisi_acc_vfio_pci_set_device_state,
>> +	.migration_get_state = hisi_acc_vfio_pci_get_device_state,
>> +};
>> +
>>   static const struct vfio_device_ops hisi_acc_vfio_pci_migrn_ops = {
>>   	.name = "hisi-acc-vfio-pci-migration",
>>   	.open_device = hisi_acc_vfio_pci_open_device,
>> @@ -1219,8 +1224,6 @@ static const struct vfio_device_ops hisi_acc_vfio_pci_migrn_ops = {
>>   	.mmap = hisi_acc_vfio_pci_mmap,
>>   	.request = vfio_pci_core_request,
>>   	.match = vfio_pci_core_match,
>> -	.migration_set_state = hisi_acc_vfio_pci_set_device_state,
>> -	.migration_get_state = hisi_acc_vfio_pci_get_device_state,
>>   };
>>   
>>   static const struct vfio_device_ops hisi_acc_vfio_pci_ops = {
>> @@ -1272,6 +1275,8 @@ static int hisi_acc_vfio_pci_probe(struct pci_dev *pdev, const struct pci_device
>>   		if (!ret) {
>>   			vfio_pci_core_init_device(&hisi_acc_vdev->core_device, pdev,
>>   						  &hisi_acc_vfio_pci_migrn_ops);
>> +			hisi_acc_vdev->core_device.vdev.mig_ops =
>> +					&hisi_acc_vfio_pci_migrn_state_ops;
>>   		} else {
>>   			pci_warn(pdev, "migration support failed, continue with generic interface\n");
>>   			vfio_pci_core_init_device(&hisi_acc_vdev->core_device, pdev,
>> diff --git a/drivers/vfio/pci/mlx5/cmd.c b/drivers/vfio/pci/mlx5/cmd.c
>> index cdd0c667dc77..dd5d7bfe0a49 100644
>> --- a/drivers/vfio/pci/mlx5/cmd.c
>> +++ b/drivers/vfio/pci/mlx5/cmd.c
>> @@ -108,7 +108,8 @@ void mlx5vf_cmd_remove_migratable(struct mlx5vf_pci_core_device *mvdev)
>>   	destroy_workqueue(mvdev->cb_wq);
>>   }
>>   
>> -void mlx5vf_cmd_set_migratable(struct mlx5vf_pci_core_device *mvdev)
>> +void mlx5vf_cmd_set_migratable(struct mlx5vf_pci_core_device *mvdev,
>> +			       const struct vfio_migration_ops *mig_ops)
>>   {
>>   	struct pci_dev *pdev = mvdev->core_device.pdev;
>>   	int ret;
>> @@ -149,6 +150,7 @@ void mlx5vf_cmd_set_migratable(struct mlx5vf_pci_core_device *mvdev)
>>   	mvdev->core_device.vdev.migration_flags =
>>   		VFIO_MIGRATION_STOP_COPY |
>>   		VFIO_MIGRATION_P2P;
>> +	mvdev->core_device.vdev.mig_ops = mig_ops;
>>   
>>   end:
>>   	mlx5_vf_put_core_dev(mvdev->mdev);
>> diff --git a/drivers/vfio/pci/mlx5/cmd.h b/drivers/vfio/pci/mlx5/cmd.h
>> index aa692d9ce656..8208f4701a90 100644
>> --- a/drivers/vfio/pci/mlx5/cmd.h
>> +++ b/drivers/vfio/pci/mlx5/cmd.h
>> @@ -62,7 +62,8 @@ int mlx5vf_cmd_suspend_vhca(struct mlx5vf_pci_core_device *mvdev, u16 op_mod);
>>   int mlx5vf_cmd_resume_vhca(struct mlx5vf_pci_core_device *mvdev, u16 op_mod);
>>   int mlx5vf_cmd_query_vhca_migration_state(struct mlx5vf_pci_core_device *mvdev,
>>   					  size_t *state_size);
>> -void mlx5vf_cmd_set_migratable(struct mlx5vf_pci_core_device *mvdev);
>> +void mlx5vf_cmd_set_migratable(struct mlx5vf_pci_core_device *mvdev,
>> +			       const struct vfio_migration_ops *mig_ops);
>>   void mlx5vf_cmd_remove_migratable(struct mlx5vf_pci_core_device *mvdev);
>>   void mlx5vf_cmd_close_migratable(struct mlx5vf_pci_core_device *mvdev);
>>   int mlx5vf_cmd_save_vhca_state(struct mlx5vf_pci_core_device *mvdev,
>> diff --git a/drivers/vfio/pci/mlx5/main.c b/drivers/vfio/pci/mlx5/main.c
>> index d754990f0662..a9b63d15c5d3 100644
>> --- a/drivers/vfio/pci/mlx5/main.c
>> +++ b/drivers/vfio/pci/mlx5/main.c
>> @@ -574,6 +574,11 @@ static void mlx5vf_pci_close_device(struct vfio_device *core_vdev)
>>   	vfio_pci_core_close_device(core_vdev);
>>   }
>>   
>> +static const struct vfio_migration_ops mlx5vf_pci_mig_ops = {
>> +	.migration_set_state = mlx5vf_pci_set_device_state,
>> +	.migration_get_state = mlx5vf_pci_get_device_state,
>> +};
>> +
>>   static const struct vfio_device_ops mlx5vf_pci_ops = {
>>   	.name = "mlx5-vfio-pci",
>>   	.open_device = mlx5vf_pci_open_device,
>> @@ -585,8 +590,6 @@ static const struct vfio_device_ops mlx5vf_pci_ops = {
>>   	.mmap = vfio_pci_core_mmap,
>>   	.request = vfio_pci_core_request,
>>   	.match = vfio_pci_core_match,
>> -	.migration_set_state = mlx5vf_pci_set_device_state,
>> -	.migration_get_state = mlx5vf_pci_get_device_state,
>>   };
>>   
>>   static int mlx5vf_pci_probe(struct pci_dev *pdev,
>> @@ -599,7 +602,7 @@ static int mlx5vf_pci_probe(struct pci_dev *pdev,
>>   	if (!mvdev)
>>   		return -ENOMEM;
>>   	vfio_pci_core_init_device(&mvdev->core_device, pdev, &mlx5vf_pci_ops);
>> -	mlx5vf_cmd_set_migratable(mvdev);
>> +	mlx5vf_cmd_set_migratable(mvdev, &mlx5vf_pci_mig_ops);
>>   	dev_set_drvdata(&pdev->dev, &mvdev->core_device);
>>   	ret = vfio_pci_core_register_device(&mvdev->core_device);
>>   	if (ret)
>> diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
>> index a0d69ddaf90d..cf875309dac0 100644
>> --- a/drivers/vfio/pci/vfio_pci_core.c
>> +++ b/drivers/vfio/pci/vfio_pci_core.c
>> @@ -1855,6 +1855,13 @@ int vfio_pci_core_register_device(struct vfio_pci_core_device *vdev)
>>   	if (pdev->hdr_type != PCI_HEADER_TYPE_NORMAL)
>>   		return -EINVAL;
>>   
>> +	if (vdev->vdev.mig_ops) {
>> +		if ((!(vdev->vdev.mig_ops->migration_get_state &&
>> +		       vdev->vdev.mig_ops->migration_set_state)) ||
>> +		    (!(vdev->vdev.migration_flags & VFIO_MIGRATION_STOP_COPY)))
>> +			return -EINVAL;
> A bit excessive on the parenthesis, a logical NOT is just below parens
> and well above a logical OR on order of operations, so it should be
> fine as:
>
> 	if (!(vdev->vdev.mig_ops->migration_get_state &&
> 	      vdev->vdev.mig_ops->migration_set_state) ||
> 	    !(vdev->vdev.migration_flags & VFIO_MIGRATION_STOP_COPY))
> 		return -EINVAL;
>
> Looks ok to me otherwise, so if there are no other changes maybe I can
> roll that in on commit.  Thanks,
>
> Alex

Yes, makes sense,  please change locally.

Yishai

>> +	}
>> +
>>   	/*
>>   	 * Prevent binding to PFs with VFs enabled, the VFs might be in use
>>   	 * by the host or other users.  We cannot capture the VFs if they
>> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
>> index e22be13e6771..ccbd106b95d8 100644
>> --- a/drivers/vfio/vfio.c
>> +++ b/drivers/vfio/vfio.c
>> @@ -1534,8 +1534,7 @@ vfio_ioctl_device_feature_mig_device_state(struct vfio_device *device,
>>   	struct file *filp = NULL;
>>   	int ret;
>>   
>> -	if (!device->ops->migration_set_state ||
>> -	    !device->ops->migration_get_state)
>> +	if (!device->mig_ops)
>>   		return -ENOTTY;
>>   
>>   	ret = vfio_check_feature(flags, argsz,
>> @@ -1551,7 +1550,8 @@ vfio_ioctl_device_feature_mig_device_state(struct vfio_device *device,
>>   	if (flags & VFIO_DEVICE_FEATURE_GET) {
>>   		enum vfio_device_mig_state curr_state;
>>   
>> -		ret = device->ops->migration_get_state(device, &curr_state);
>> +		ret = device->mig_ops->migration_get_state(device,
>> +							   &curr_state);
>>   		if (ret)
>>   			return ret;
>>   		mig.device_state = curr_state;
>> @@ -1559,7 +1559,7 @@ vfio_ioctl_device_feature_mig_device_state(struct vfio_device *device,
>>   	}
>>   
>>   	/* Handle the VFIO_DEVICE_FEATURE_SET */
>> -	filp = device->ops->migration_set_state(device, mig.device_state);
>> +	filp = device->mig_ops->migration_set_state(device, mig.device_state);
>>   	if (IS_ERR(filp) || !filp)
>>   		goto out_copy;
>>   
>> @@ -1582,8 +1582,7 @@ static int vfio_ioctl_device_feature_migration(struct vfio_device *device,
>>   	};
>>   	int ret;
>>   
>> -	if (!device->ops->migration_set_state ||
>> -	    !device->ops->migration_get_state)
>> +	if (!device->mig_ops)
>>   		return -ENOTTY;
>>   
>>   	ret = vfio_check_feature(flags, argsz, VFIO_DEVICE_FEATURE_GET,
>> diff --git a/include/linux/vfio.h b/include/linux/vfio.h
>> index aa888cc51757..d6c592565be7 100644
>> --- a/include/linux/vfio.h
>> +++ b/include/linux/vfio.h
>> @@ -32,6 +32,11 @@ struct vfio_device_set {
>>   struct vfio_device {
>>   	struct device *dev;
>>   	const struct vfio_device_ops *ops;
>> +	/*
>> +	 * mig_ops is a static property of the vfio_device which must be set
>> +	 * prior to registering the vfio_device.
>> +	 */
>> +	const struct vfio_migration_ops *mig_ops;
>>   	struct vfio_group *group;
>>   	struct vfio_device_set *dev_set;
>>   	struct list_head dev_set_list;
>> @@ -61,16 +66,6 @@ struct vfio_device {
>>    *         match, -errno for abort (ex. match with insufficient or incorrect
>>    *         additional args)
>>    * @device_feature: Optional, fill in the VFIO_DEVICE_FEATURE ioctl
>> - * @migration_set_state: Optional callback to change the migration state for
>> - *         devices that support migration. It's mandatory for
>> - *         VFIO_DEVICE_FEATURE_MIGRATION migration support.
>> - *         The returned FD is used for data transfer according to the FSM
>> - *         definition. The driver is responsible to ensure that FD reaches end
>> - *         of stream or error whenever the migration FSM leaves a data transfer
>> - *         state or before close_device() returns.
>> - * @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.
>>    */
>>   struct vfio_device_ops {
>>   	char	*name;
>> @@ -87,6 +82,21 @@ struct vfio_device_ops {
>>   	int	(*match)(struct vfio_device *vdev, char *buf);
>>   	int	(*device_feature)(struct vfio_device *device, u32 flags,
>>   				  void __user *arg, size_t argsz);
>> +};
>> +
>> +/**
>> + * @migration_set_state: Optional callback to change the migration state for
>> + *         devices that support migration. It's mandatory for
>> + *         VFIO_DEVICE_FEATURE_MIGRATION migration support.
>> + *         The returned FD is used for data transfer according to the FSM
>> + *         definition. The driver is responsible to ensure that FD reaches end
>> + *         of stream or error whenever the migration FSM leaves a data transfer
>> + *         state or before close_device() returns.
>> + * @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.
>> + */
>> +struct vfio_migration_ops {
>>   	struct file *(*migration_set_state)(
>>   		struct vfio_device *device,
>>   		enum vfio_device_mig_state new_state);

Re: [PATCH V3 vfio 0/2] Migration few enhancements

[Alex Williamson]

On Tue, 28 Jun 2022 18:59:08 +0300
Yishai Hadas <yishaih@nvidia.com> wrote:

> This series includes few enhancements in the migration area and some
> fixes in mlx5 driver as of below.
> 
> It splits migration ops from the main device ops, this enables a driver
> to safely set its migration's ops only when migration is supported and
> leave the other code around (e.g., core, driver) clean.
> 
> Registering different structs based on the device capabilities might
> start to hit combinatorial explosion when we'll introduce ops for dirty
> logging that may be optional too.
> 
> As part of that, adapt mlx5 to this scheme and fix some issues around
> its migration capable usage.
> 
> V3:
> - Fix a typo in vfio_pci_core_register_device(), to check for both set & get 'mig_ops'.
> 
> V2: https://lore.kernel.org/all/20220616170118.497620ba.alex.williamson@redhat.com/T/
> - Validate ops construction and migration mandatory flags on
>   registration as was asked by Kevin and Jason.
> - As of the above move to use a single 'mig_ops' check in vfio before
>   calling the driver.
> 
> V1: https://lore.kernel.org/all/20220626083958.54175-1-yishaih@nvidia.com/
> - Add a comment about the required usage of 'mig_ops' as was suggested
>   by Alex.
> - Add Kevin's Reviewed-by tag.
> 
> V0:
> https://lore.kernel.org/all/20220616170118.497620ba.alex.williamson@redhat.com/T/
> 
> Yishai
> Yishai Hadas (2):
>   vfio/mlx5: Protect mlx5vf_disable_fds() upon close device
>   vfio: Split migration ops from main device ops
> 
>  .../vfio/pci/hisilicon/hisi_acc_vfio_pci.c    | 11 +++++--
>  drivers/vfio/pci/mlx5/cmd.c                   | 14 ++++++++-
>  drivers/vfio/pci/mlx5/cmd.h                   |  4 ++-
>  drivers/vfio/pci/mlx5/main.c                  | 11 ++++---
>  drivers/vfio/pci/vfio_pci_core.c              |  7 +++++
>  drivers/vfio/vfio.c                           | 11 ++++---
>  include/linux/vfio.h                          | 30 ++++++++++++-------
>  7 files changed, 63 insertions(+), 25 deletions(-)
> 

I see these are included in your new longer series, where the only
change was my suggested removal of parentheses, so I went ahead and
merged this to my next branch for v5.20 and we can drop them from the
other series.  Thanks,

Alex