Add mlx5 live migration driver

PATCH In review Leon Romanovsky 9 participants 58 messages Sep 22, 2021 – Sep 30, 2021

Subsystems (8)

Networking Network drivers KVM PCI +4 more subsystems

lore

Share & tools

lore

Leon Romanovsky

author

2021-09-22 10:39 UTC #0 lore raw

cover Add mlx5 live migration driver

From: Leon Romanovsky <[email protected]>

From Yishai:

This series adds mlx5 live migration driver for VFs that are migrated capable.

It uses vfio_pci_core to register to the VFIO subsystem and then implements the mlx5 specific logic in the migration area.

The migration implementation follows the definition from uapi/vfio.h and uses the mlx5 VF->PF command channel to achieve it.

The series adds an API in the vfio core layer to check migration state transition validity as part of a migration flow. This ensures that all migration implementations follow a consistent migration state machine.

As part of the migration process the VF doesn't ride on mlx5_core, the device is driving *two* different PCI devices, the PF owned by mlx5_core and the VF owned by the mlx5 vfio driver.

The mlx5_core of the PF is accessed only during the narrow window of the VF's ioctl that requires its services.

The series also exposes from the PCI sub system an API named pci_iov_vf_id() to get the index of the VF. The PCI core uses this index internally, often called the vf_id, during the setup of the VF, eg pci_iov_add_virtfn().

The returned VF index is needed by the mlx5 vfio driver for its internal operations to configure/control its VFs as part of the migration process.

With the above functionality in place the driver implements the suspend/resume flows to work over QEMU.

Thanks

-----------------------------------------------------
Alex,

This series touches our ethernet and RDMA drivers, so we will need to route the patches through separate shared branch (mlx5-next) in order to eliminate the chances of merge conflicts between different subsystems.

Thanks

Jason Gunthorpe (1):
PCI/IOV: Provide internal VF index

Yishai Hadas (6):
vfio: Add an API to check migration state transition validity vfio/pci_core: Make the region->release() function optional net/mlx5: Introduce migration bits and structures net/mlx5: Expose APIs to get/put the mlx5 core device
mlx5_vfio_pci: Expose migration commands over mlx5 device
mlx5_vfio_pci: Implement vfio_pci driver for mlx5 devices

 .../net/ethernet/mellanox/mlx5/core/main.c    |  43 +
 drivers/pci/iov.c                             |  14 +
 drivers/vfio/pci/Kconfig                      |  11 +
 drivers/vfio/pci/Makefile                     |   3 +
 drivers/vfio/pci/mlx5_vfio_pci.c              | 736 ++++++++++++++++++
 drivers/vfio/pci/mlx5_vfio_pci_cmd.c          | 358 +++++++++
 drivers/vfio/pci/mlx5_vfio_pci_cmd.h          |  43 +
 drivers/vfio/pci/vfio_pci_core.c              |   3 +-
 drivers/vfio/vfio.c                           |  41 +
 include/linux/mlx5/driver.h                   |   3 +
 include/linux/mlx5/mlx5_ifc.h                 | 145 +++-
 include/linux/pci.h                           |   7 +-
 include/linux/vfio.h                          |   1 +
 13 files changed, 1405 insertions(+), 3 deletions(-)
 create mode 100644 drivers/vfio/pci/mlx5_vfio_pci.c
 create mode 100644 drivers/vfio/pci/mlx5_vfio_pci_cmd.c
 create mode 100644 drivers/vfio/pci/mlx5_vfio_pci_cmd.h

Signature · 1 line

2.31.1

Leon Romanovsky

author

2021-09-22 10:39 UTC #1 lore raw

PATCH 1/7 PCI/IOV: Provide internal VF index

From: Jason Gunthorpe <[email protected]>

The PCI core uses the VF index internally, often called the vf_id, during the setup of the VF, eg pci_iov_add_virtfn().

This index is needed for device drivers that implement live migration for their internal operations that configure/control their VFs.

Specifically, mlx5_vfio_pci driver that is introduced in coming patches from this series needs it and not the bus/device/function which is exposed today.

Add pci_iov_vf_id() which computes the vf_id by reversing the math that was used to create the bus/device/function.

Signed-off-by Yishai Hadas <[email protected]>

Signed-off-by Jason Gunthorpe <[email protected]>

Signed-off-by Leon Romanovsky <[email protected]>

---
 drivers/pci/iov.c   | 14 ++++++++++++++
 include/linux/pci.h |  7 ++++++-
 2 files changed, 20 insertions(+), 1 deletion(-)

drivers/pci/iov.c +14 -0

index dafdc652fcd0..e7751fa3fe0b 100644
@@ -33,6 +33,20 @@ int pci_iov_virtfn_devfn(struct pci_dev *dev, int vf_id)
 }
 EXPORT_SYMBOL_GPL(pci_iov_virtfn_devfn);
 
+int pci_iov_vf_id(struct pci_dev *dev)
+{
+	struct pci_dev *pf;
+
+	if (!dev->is_virtfn)
+		return -EINVAL;
+
+	pf = pci_physfn(dev);
+	return (((dev->bus->number << 8) + dev->devfn) -
+		((pf->bus->number << 8) + pf->devfn + pf->sriov->offset)) /
+	       pf->sriov->stride;
+}
+EXPORT_SYMBOL_GPL(pci_iov_vf_id);
+
 /*
  * Per SR-IOV spec sec 3.3.10 and 3.3.11, First VF Offset and VF Stride may
  * change when NumVFs changes.

include/linux/pci.h +6 -1

index cd8aa6fce204..4d6c73506e18 100644
@@ -2153,7 +2153,7 @@ void __iomem *pci_ioremap_wc_bar(struct pci_dev *pdev, int bar);
 #ifdef CONFIG_PCI_IOV
 int pci_iov_virtfn_bus(struct pci_dev *dev, int id);
 int pci_iov_virtfn_devfn(struct pci_dev *dev, int id);
-
+int pci_iov_vf_id(struct pci_dev *dev);
 int pci_enable_sriov(struct pci_dev *dev, int nr_virtfn);
 void pci_disable_sriov(struct pci_dev *dev);

@@ -2181,6 +2181,11 @@ static inline int pci_iov_virtfn_devfn(struct pci_dev *dev, int id)
 {
 	return -ENOSYS;
 }
+static inline int pci_iov_vf_id(struct pci_dev *dev)
+{
+	return -ENOSYS;
+}
+
 static inline int pci_enable_sriov(struct pci_dev *dev, int nr_virtfn)
 { return -ENODEV; }

Signature · 1 line

2.31.1

Bjorn Helgaas

Acked-by on v1 1/7

2021-09-22 21:59 UTC #2 lore raw

Re: [PATCH mlx5-next 1/7] PCI/IOV: Provide internal VF index

Quoted 12 lines · Leon Romanovsky

From: Jason Gunthorpe <[email protected]>
The PCI core uses the VF index internally, often called the vf_id, during the setup of the VF, eg pci_iov_add_virtfn().
This index is needed for device drivers that implement live migration for their internal operations that configure/control their VFs.
Specifically, mlx5_vfio_pci driver that is introduced in coming patches from this series needs it and not the bus/device/function which is exposed today.

Quoted 6 lines

Add pci_iov_vf_id() which computes the vf_id by reversing the math that was used to create the bus/device/function.
Signed-off-by Yishai Hadas <[email protected]>
Signed-off-by Jason Gunthorpe <[email protected]>
Signed-off-by Leon Romanovsky <[email protected]>

Acked-by Bjorn Helgaas <[email protected]>

mlx5_core_sriov_set_msix_vec_count() looks like it does basically the same thing as pci_iov_vf_id() by iterating through VFs until it finds one with a matching devfn (although it *doesn't* check for a matching bus number, which seems like a bug).

Maybe that should use pci_iov_vf_id()?

Quoted 44 lines

---
 drivers/pci/iov.c   | 14 ++++++++++++++
 include/linux/pci.h |  7 ++++++-
 2 files changed, 20 insertions(+), 1 deletion(-)

drivers/pci/iov.c +14 -0

index dafdc652fcd0..e7751fa3fe0b 100644
@@ -33,6 +33,20 @@ int pci_iov_virtfn_devfn(struct pci_dev *dev, int vf_id)
 }
 EXPORT_SYMBOL_GPL(pci_iov_virtfn_devfn);
 
+int pci_iov_vf_id(struct pci_dev *dev)
+{
+	struct pci_dev *pf;
+
+	if (!dev->is_virtfn)
+		return -EINVAL;
+
+	pf = pci_physfn(dev);
+	return (((dev->bus->number << 8) + dev->devfn) -
+		((pf->bus->number << 8) + pf->devfn + pf->sriov->offset)) /
+	       pf->sriov->stride;
+}
+EXPORT_SYMBOL_GPL(pci_iov_vf_id);
+
 /*
  * Per SR-IOV spec sec 3.3.10 and 3.3.11, First VF Offset and VF Stride may
  * change when NumVFs changes.

include/linux/pci.h +1 -1

index cd8aa6fce204..4d6c73506e18 100644
@@ -2153,7 +2153,7 @@ void __iomem *pci_ioremap_wc_bar(struct pci_dev *pdev, int bar);
 #ifdef CONFIG_PCI_IOV
 int pci_iov_virtfn_bus(struct pci_dev *dev, int id);
 int pci_iov_virtfn_devfn(struct pci_dev *dev, int id);
-
+int pci_iov_vf_id(struct pci_dev *dev);
 int pci_enable_sriov(struct pci_dev *dev, int nr_virtfn);
 void pci_disable_sriov(struct pci_dev *dev);

include/linux/pci.h static inline int pci_iov_virtfn_devfn(struct pci_dev *dev, int id) · line 2188

Quoted 8 lines

include/linux/pci.h +5 -0

@@ -2181,6 +2181,11 @@ static inline int pci_iov_virtfn_devfn(struct pci_dev *dev, int id)
 {
 	return -ENOSYS;
 }
+static inline int pci_iov_vf_id(struct pci_dev *dev)
+{
+	return -ENOSYS;
+}
+

Drop the blank line to match the surrounding stubs.

Quoted 2 lines

include/linux/pci.h +0 -0

 static inline int pci_enable_sriov(struct pci_dev *dev, int nr_virtfn)
 { return -ENODEV; }

Leon Romanovsky

author

2021-09-23 06:35 UTC #3 lore raw

Re: [PATCH mlx5-next 1/7] PCI/IOV: Provide internal VF index

Quoted 20 lines · Bjorn Helgaas

Quoted 12 lines · Leon Romanovsky
From: Jason Gunthorpe <[email protected]>
The PCI core uses the VF index internally, often called the vf_id, during the setup of the VF, eg pci_iov_add_virtfn().
This index is needed for device drivers that implement live migration for their internal operations that configure/control their VFs.
Specifically, mlx5_vfio_pci driver that is introduced in coming patches from this series needs it and not the bus/device/function which is exposed today.
Quoted 6 lines
Add pci_iov_vf_id() which computes the vf_id by reversing the math that was used to create the bus/device/function.
Signed-off-by Yishai Hadas <[email protected]>
Signed-off-by Jason Gunthorpe <[email protected]>
Signed-off-by Leon Romanovsky <[email protected]>

Quoted 8 lines

Acked-by Bjorn Helgaas <[email protected]>
mlx5_core_sriov_set_msix_vec_count() looks like it does basically the same thing as pci_iov_vf_id() by iterating through VFs until it finds one with a matching devfn (although it *doesn't* check for a matching bus number, which seems like a bug).
Maybe that should use pci_iov_vf_id()?

Yes, I gave same comment internally and we decided to simply reduce the amount of changes in mlx5_core to have less distractions and submit as a followup. Most likely will add this hunk in v1.

drivers/net/ethernet/mellanox/mlx5/core/sriov.c +2 -9

index e8185b69ac6c..b66be0b4244a 100644
@@ -209,15 +209,8 @@ int mlx5_core_sriov_set_msix_vec_count(struct pci_dev *vf, int msix_vec_count)
        /* Reversed translation of PCI VF function number to the internal
         * function_id, which exists in the name of virtfn symlink.
         */
-       for (id = 0; id < pci_num_vf(pf); id++) {
-               if (!sriov->vfs_ctx[id].enabled)
-                       continue;
-
-               if (vf->devfn == pci_iov_virtfn_devfn(pf, id))
-                       break;
-       }
-
-       if (id == pci_num_vf(pf) || !sriov->vfs_ctx[id].enabled)
+       id = pci_iov_vf_id(vf);
+       if (id < 0 || !sriov->vfs_ctx[id].enabled)
                return -EINVAL;
 
        return mlx5_set_msix_vec_count(dev, id + 1, msix_vec_count);

Thanks

Quoted 44 lines

---
 drivers/pci/iov.c   | 14 ++++++++++++++
 include/linux/pci.h |  7 ++++++-
 2 files changed, 20 insertions(+), 1 deletion(-)

drivers/pci/iov.c +14 -0

index dafdc652fcd0..e7751fa3fe0b 100644
@@ -33,6 +33,20 @@ int pci_iov_virtfn_devfn(struct pci_dev *dev, int vf_id)
 }
 EXPORT_SYMBOL_GPL(pci_iov_virtfn_devfn);
 
+int pci_iov_vf_id(struct pci_dev *dev)
+{
+	struct pci_dev *pf;
+
+	if (!dev->is_virtfn)
+		return -EINVAL;
+
+	pf = pci_physfn(dev);
+	return (((dev->bus->number << 8) + dev->devfn) -
+		((pf->bus->number << 8) + pf->devfn + pf->sriov->offset)) /
+	       pf->sriov->stride;
+}
+EXPORT_SYMBOL_GPL(pci_iov_vf_id);
+
 /*
  * Per SR-IOV spec sec 3.3.10 and 3.3.11, First VF Offset and VF Stride may
  * change when NumVFs changes.

include/linux/pci.h +1 -1

index cd8aa6fce204..4d6c73506e18 100644
@@ -2153,7 +2153,7 @@ void __iomem *pci_ioremap_wc_bar(struct pci_dev *pdev, int bar);
 #ifdef CONFIG_PCI_IOV
 int pci_iov_virtfn_bus(struct pci_dev *dev, int id);
 int pci_iov_virtfn_devfn(struct pci_dev *dev, int id);
-
+int pci_iov_vf_id(struct pci_dev *dev);
 int pci_enable_sriov(struct pci_dev *dev, int nr_virtfn);
 void pci_disable_sriov(struct pci_dev *dev);

Quoted 11 lines

include/linux/pci.h static inline int pci_iov_virtfn_devfn(struct pci_dev *dev, int id) · line 2188
Quoted 9 lines
include/linux/pci.h +5 -0
@@ -2181,6 +2181,11 @@ static inline int pci_iov_virtfn_devfn(struct pci_dev *dev, int id)
 {
 	return -ENOSYS;
 }
+static inline int pci_iov_vf_id(struct pci_dev *dev)
+{
+	return -ENOSYS;
+}
+
Drop the blank line to match the surrounding stubs.

Sure, thanks

Quoted 4 lines

Quoted 3 lines

include/linux/pci.h +0 -0

 static inline int pci_enable_sriov(struct pci_dev *dev, int nr_virtfn)
 { return -ENODEV; }

Bjorn Helgaas

2021-09-24 13:13 UTC #4 lore raw

Re: [PATCH mlx5-next 1/7] PCI/IOV: Provide internal VF index

Quoted 28 lines · Leon Romanovsky

Quoted 27 lines · Bjorn Helgaas
Quoted 12 lines · Leon Romanovsky
From: Jason Gunthorpe <[email protected]>
The PCI core uses the VF index internally, often called the vf_id, during the setup of the VF, eg pci_iov_add_virtfn().
This index is needed for device drivers that implement live migration for their internal operations that configure/control their VFs.
Specifically, mlx5_vfio_pci driver that is introduced in coming patches from this series needs it and not the bus/device/function which is exposed today.
Quoted 6 lines
Add pci_iov_vf_id() which computes the vf_id by reversing the math that was used to create the bus/device/function.
Signed-off-by Yishai Hadas <[email protected]>
Signed-off-by Jason Gunthorpe <[email protected]>
Signed-off-by Leon Romanovsky <[email protected]>
Acked-by Bjorn Helgaas <[email protected]>
mlx5_core_sriov_set_msix_vec_count() looks like it does basically the same thing as pci_iov_vf_id() by iterating through VFs until it finds one with a matching devfn (although it *doesn't* check for a matching bus number, which seems like a bug).

Quoted 5 lines

Quoted 1 line
Maybe that should use pci_iov_vf_id()?
Yes, I gave same comment internally and we decided to simply reduce the amount of changes in mlx5_core to have less distractions and submit as a followup. Most likely will add this hunk in v1.

I guess it backfired as far as reducing distractions, because now it just looks like a job half-done.

And it still looks like the existing code is buggy. This is called via sysfs, so if the PF is on bus X and the user writes to sriov_vf_msix_count for a VF on bus X+1, it looks like mlx5_core_sriov_set_msix_vec_count() will set the count for the wrong VF.

Bjorn

Leon Romanovsky

author

2021-09-25 10:10 UTC #5 lore raw

Re: [PATCH mlx5-next 1/7] PCI/IOV: Provide internal VF index

Quoted 29 lines · Bjorn Helgaas

Quoted 28 lines · Leon Romanovsky
Quoted 27 lines · Bjorn Helgaas
Quoted 12 lines · Leon Romanovsky
From: Jason Gunthorpe <[email protected]>
The PCI core uses the VF index internally, often called the vf_id, during the setup of the VF, eg pci_iov_add_virtfn().
This index is needed for device drivers that implement live migration for their internal operations that configure/control their VFs.
Specifically, mlx5_vfio_pci driver that is introduced in coming patches from this series needs it and not the bus/device/function which is exposed today.
Quoted 6 lines
Add pci_iov_vf_id() which computes the vf_id by reversing the math that was used to create the bus/device/function.
Signed-off-by Yishai Hadas <[email protected]>
Signed-off-by Jason Gunthorpe <[email protected]>
Signed-off-by Leon Romanovsky <[email protected]>
Acked-by Bjorn Helgaas <[email protected]>
mlx5_core_sriov_set_msix_vec_count() looks like it does basically the same thing as pci_iov_vf_id() by iterating through VFs until it finds one with a matching devfn (although it *doesn't* check for a matching bus number, which seems like a bug).

Quoted 8 lines

Quoted 5 lines
Quoted 1 line
Maybe that should use pci_iov_vf_id()?
Yes, I gave same comment internally and we decided to simply reduce the amount of changes in mlx5_core to have less distractions and submit as a followup. Most likely will add this hunk in v1.
I guess it backfired as far as reducing distractions, because now it just looks like a job half-done.

Partially :)
I didn't expect to see acceptance of this series in v0, we wanted to gather feedback as early as possible.

Quoted 6 lines

And it still looks like the existing code is buggy. This is called via sysfs, so if the PF is on bus X and the user writes to sriov_vf_msix_count for a VF on bus X+1, it looks like mlx5_core_sriov_set_msix_vec_count() will set the count for the wrong VF.

In mlx5_core_sriov_set_msix_vec_count(), we receive VF that is connected to PF which has "struct mlx5_core_dev". My expectation is that they share same bus as that PF was the one who created VFs. The mlx5 devices supports upto 256 VFs and it is far below the bus split mentioned in PCI spec.

How can VF and their respective PF have different bus numbers?

Thanks

Quoted 2 lines

Bjorn

Bjorn Helgaas

2021-09-25 17:41 UTC #6 lore raw

Re: [PATCH mlx5-next 1/7] PCI/IOV: Provide internal VF index

Quoted 37 lines · Leon Romanovsky

Quoted 28 lines · Bjorn Helgaas
Quoted 27 lines · Leon Romanovsky
Quoted 26 lines · Bjorn Helgaas
Quoted 12 lines · Leon Romanovsky
From: Jason Gunthorpe <[email protected]>
The PCI core uses the VF index internally, often called the vf_id, during the setup of the VF, eg pci_iov_add_virtfn().
This index is needed for device drivers that implement live migration for their internal operations that configure/control their VFs.
Specifically, mlx5_vfio_pci driver that is introduced in coming patches from this series needs it and not the bus/device/function which is exposed today.
Quoted 6 lines
Add pci_iov_vf_id() which computes the vf_id by reversing the math that was used to create the bus/device/function.
Signed-off-by Yishai Hadas <[email protected]>
Signed-off-by Jason Gunthorpe <[email protected]>
Signed-off-by Leon Romanovsky <[email protected]>
Acked-by Bjorn Helgaas <[email protected]>
mlx5_core_sriov_set_msix_vec_count() looks like it does basically the same thing as pci_iov_vf_id() by iterating through VFs until it finds one with a matching devfn (although it *doesn't* check for a matching bus number, which seems like a bug).
...
Quoted 5 lines
And it still looks like the existing code is buggy. This is called via sysfs, so if the PF is on bus X and the user writes to sriov_vf_msix_count for a VF on bus X+1, it looks like mlx5_core_sriov_set_msix_vec_count() will set the count for the wrong VF.

Quoted 6 lines

In mlx5_core_sriov_set_msix_vec_count(), we receive VF that is connected to PF which has "struct mlx5_core_dev". My expectation is that they share same bus as that PF was the one who created VFs. The mlx5 devices supports upto 256 VFs and it is far below the bus split mentioned in PCI spec.
How can VF and their respective PF have different bus numbers?

See PCIe r5.0, sec 9.2.1.2. For example,

PF 0 on bus 20
First VF Offset 1
VF Stride 1
NumVFs 511
VF 0,1 through VF 0,255 on bus 20
VF 0,256 through VF 0,511 on bus 21

This is implemented in pci_iov_add_virtfn(), which computes the bus number and devfn from the VF ID.

pci_iov_virtfn_devfn(VF 0,1) == pci_iov_virtfn_devfn(VF 0,256), so if the user writes to sriov_vf_msix_count for VF 0,256, it looks like we'll call mlx5_set_msix_vec_count() for VF 0,1 instead of VF 0,256.

The spec encourages devices that require no more than 256 devices to locate them all on the same bus number (PCIe r5.0, sec 9.1), so if you only have 255 VFs, you may avoid the problem.

But in mlx5_core_sriov_set_msix_vec_count(), it's not obvious that it is safe to assume the bus number is the same.

Bjorn

Leon Romanovsky

author

2021-09-26 06:36 UTC #7 lore raw

Re: [PATCH mlx5-next 1/7] PCI/IOV: Provide internal VF index

Quoted 54 lines · Bjorn Helgaas

Quoted 37 lines · Leon Romanovsky
Quoted 28 lines · Bjorn Helgaas
Quoted 27 lines · Leon Romanovsky
Quoted 26 lines · Bjorn Helgaas
Quoted 12 lines · Leon Romanovsky
From: Jason Gunthorpe <[email protected]>
The PCI core uses the VF index internally, often called the vf_id, during the setup of the VF, eg pci_iov_add_virtfn().
This index is needed for device drivers that implement live migration for their internal operations that configure/control their VFs.
Specifically, mlx5_vfio_pci driver that is introduced in coming patches from this series needs it and not the bus/device/function which is exposed today.
Quoted 6 lines
Add pci_iov_vf_id() which computes the vf_id by reversing the math that was used to create the bus/device/function.
Signed-off-by Yishai Hadas <[email protected]>
Signed-off-by Jason Gunthorpe <[email protected]>
Signed-off-by Leon Romanovsky <[email protected]>
Acked-by Bjorn Helgaas <[email protected]>
mlx5_core_sriov_set_msix_vec_count() looks like it does basically the same thing as pci_iov_vf_id() by iterating through VFs until it finds one with a matching devfn (although it *doesn't* check for a matching bus number, which seems like a bug).
...
Quoted 5 lines
And it still looks like the existing code is buggy. This is called via sysfs, so if the PF is on bus X and the user writes to sriov_vf_msix_count for a VF on bus X+1, it looks like mlx5_core_sriov_set_msix_vec_count() will set the count for the wrong VF.
Quoted 6 lines
In mlx5_core_sriov_set_msix_vec_count(), we receive VF that is connected to PF which has "struct mlx5_core_dev". My expectation is that they share same bus as that PF was the one who created VFs. The mlx5 devices supports upto 256 VFs and it is far below the bus split mentioned in PCI spec.
How can VF and their respective PF have different bus numbers?
See PCIe r5.0, sec 9.2.1.2. For example,
PF 0 on bus 20
First VF Offset 1
VF Stride 1
NumVFs 511
VF 0,1 through VF 0,255 on bus 20
VF 0,256 through VF 0,511 on bus 21

Quoted 6 lines

This is implemented in pci_iov_add_virtfn(), which computes the bus number and devfn from the VF ID.
pci_iov_virtfn_devfn(VF 0,1) == pci_iov_virtfn_devfn(VF 0,256), so if the user writes to sriov_vf_msix_count for VF 0,256, it looks like we'll call mlx5_set_msix_vec_count() for VF 0,1 instead of VF 0,256.

This is PCI spec split that I mentioned.

Quoted 7 lines

The spec encourages devices that require no more than 256 devices to locate them all on the same bus number (PCIe r5.0, sec 9.1), so if you only have 255 VFs, you may avoid the problem.
But in mlx5_core_sriov_set_msix_vec_count(), it's not obvious that it is safe to assume the bus number is the same.

No problem, we will make it more clear.

Quoted 2 lines

Bjorn

Bjorn Helgaas

2021-09-26 20:23 UTC #8 lore raw

Re: [PATCH mlx5-next 1/7] PCI/IOV: Provide internal VF index

Quoted 65 lines · Leon Romanovsky

Quoted 54 lines · Bjorn Helgaas
Quoted 37 lines · Leon Romanovsky
Quoted 28 lines · Bjorn Helgaas
Quoted 27 lines · Leon Romanovsky
Quoted 26 lines · Bjorn Helgaas
Quoted 12 lines · Leon Romanovsky
From: Jason Gunthorpe <[email protected]>
The PCI core uses the VF index internally, often called the vf_id, during the setup of the VF, eg pci_iov_add_virtfn().
This index is needed for device drivers that implement live migration for their internal operations that configure/control their VFs.
Specifically, mlx5_vfio_pci driver that is introduced in coming patches from this series needs it and not the bus/device/function which is exposed today.
Quoted 6 lines
Add pci_iov_vf_id() which computes the vf_id by reversing the math that was used to create the bus/device/function.
Signed-off-by Yishai Hadas <[email protected]>
Signed-off-by Jason Gunthorpe <[email protected]>
Signed-off-by Leon Romanovsky <[email protected]>
Acked-by Bjorn Helgaas <[email protected]>
mlx5_core_sriov_set_msix_vec_count() looks like it does basically the same thing as pci_iov_vf_id() by iterating through VFs until it finds one with a matching devfn (although it *doesn't* check for a matching bus number, which seems like a bug).
...
Quoted 5 lines
And it still looks like the existing code is buggy. This is called via sysfs, so if the PF is on bus X and the user writes to sriov_vf_msix_count for a VF on bus X+1, it looks like mlx5_core_sriov_set_msix_vec_count() will set the count for the wrong VF.
Quoted 6 lines
In mlx5_core_sriov_set_msix_vec_count(), we receive VF that is connected to PF which has "struct mlx5_core_dev". My expectation is that they share same bus as that PF was the one who created VFs. The mlx5 devices supports upto 256 VFs and it is far below the bus split mentioned in PCI spec.
How can VF and their respective PF have different bus numbers?
See PCIe r5.0, sec 9.2.1.2. For example,
PF 0 on bus 20
First VF Offset 1
VF Stride 1
NumVFs 511
VF 0,1 through VF 0,255 on bus 20
VF 0,256 through VF 0,511 on bus 21
Quoted 6 lines
This is implemented in pci_iov_add_virtfn(), which computes the bus number and devfn from the VF ID.
pci_iov_virtfn_devfn(VF 0,1) == pci_iov_virtfn_devfn(VF 0,256), so if the user writes to sriov_vf_msix_count for VF 0,256, it looks like we'll call mlx5_set_msix_vec_count() for VF 0,1 instead of VF 0,256.
This is PCI spec split that I mentioned.
Quoted 1 line

Quoted 8 lines

Quoted 6 lines
The spec encourages devices that require no more than 256 devices to locate them all on the same bus number (PCIe r5.0, sec 9.1), so if you only have 255 VFs, you may avoid the problem.
But in mlx5_core_sriov_set_msix_vec_count(), it's not obvious that it is safe to assume the bus number is the same.
No problem, we will make it more clear.

IMHO you should resolve it by using the new interface. Better performing, unambiguous regardless of how many VFs the device supports. What's the down side?

Leon Romanovsky

author

2021-09-27 11:55 UTC #9 lore raw

Re: [PATCH mlx5-next 1/7] PCI/IOV: Provide internal VF index

Quoted 70 lines · Bjorn Helgaas

Quoted 69 lines · Leon Romanovsky
Quoted 54 lines · Bjorn Helgaas
Quoted 37 lines · Leon Romanovsky
Quoted 28 lines · Bjorn Helgaas
Quoted 27 lines · Leon Romanovsky
Quoted 26 lines · Bjorn Helgaas
Quoted 12 lines · Leon Romanovsky
From: Jason Gunthorpe <[email protected]>
The PCI core uses the VF index internally, often called the vf_id, during the setup of the VF, eg pci_iov_add_virtfn().
This index is needed for device drivers that implement live migration for their internal operations that configure/control their VFs.
Specifically, mlx5_vfio_pci driver that is introduced in coming patches from this series needs it and not the bus/device/function which is exposed today.
Quoted 6 lines
Add pci_iov_vf_id() which computes the vf_id by reversing the math that was used to create the bus/device/function.
Signed-off-by Yishai Hadas <[email protected]>
Signed-off-by Jason Gunthorpe <[email protected]>
Signed-off-by Leon Romanovsky <[email protected]>
Acked-by Bjorn Helgaas <[email protected]>
mlx5_core_sriov_set_msix_vec_count() looks like it does basically the same thing as pci_iov_vf_id() by iterating through VFs until it finds one with a matching devfn (although it *doesn't* check for a matching bus number, which seems like a bug).
...
Quoted 5 lines
And it still looks like the existing code is buggy. This is called via sysfs, so if the PF is on bus X and the user writes to sriov_vf_msix_count for a VF on bus X+1, it looks like mlx5_core_sriov_set_msix_vec_count() will set the count for the wrong VF.
Quoted 6 lines
In mlx5_core_sriov_set_msix_vec_count(), we receive VF that is connected to PF which has "struct mlx5_core_dev". My expectation is that they share same bus as that PF was the one who created VFs. The mlx5 devices supports upto 256 VFs and it is far below the bus split mentioned in PCI spec.
How can VF and their respective PF have different bus numbers?
See PCIe r5.0, sec 9.2.1.2. For example,
PF 0 on bus 20
First VF Offset 1
VF Stride 1
NumVFs 511
VF 0,1 through VF 0,255 on bus 20
VF 0,256 through VF 0,511 on bus 21
Quoted 6 lines
This is implemented in pci_iov_add_virtfn(), which computes the bus number and devfn from the VF ID.
pci_iov_virtfn_devfn(VF 0,1) == pci_iov_virtfn_devfn(VF 0,256), so if the user writes to sriov_vf_msix_count for VF 0,256, it looks like we'll call mlx5_set_msix_vec_count() for VF 0,1 instead of VF 0,256.
This is PCI spec split that I mentioned.
Quoted 5 lines
The spec encourages devices that require no more than 256 devices to locate them all on the same bus number (PCIe r5.0, sec 9.1), so if you only have 255 VFs, you may avoid the problem.

Quoted 8 lines

Quoted 4 lines
Quoted 2 lines
But in mlx5_core_sriov_set_msix_vec_count(), it's not obvious that it is safe to assume the bus number is the same.
No problem, we will make it more clear.
IMHO you should resolve it by using the new interface. Better performing, unambiguous regardless of how many VFs the device supports. What's the down side?

I don't see any. My previous answer worth to be written. "No problem, we will make it more clear with this new function".

Thanks

Bjorn Helgaas

2021-09-27 14:48 UTC #10 lore raw

Re: [PATCH mlx5-next 1/7] PCI/IOV: Provide internal VF index

Quoted 74 lines · Leon Romanovsky

Quoted 73 lines · Bjorn Helgaas
Quoted 72 lines · Leon Romanovsky
Quoted 54 lines · Bjorn Helgaas
Quoted 37 lines · Leon Romanovsky
Quoted 28 lines · Bjorn Helgaas
Quoted 27 lines · Leon Romanovsky
Quoted 26 lines · Bjorn Helgaas
Quoted 12 lines · Leon Romanovsky
From: Jason Gunthorpe <[email protected]>
The PCI core uses the VF index internally, often called the vf_id, during the setup of the VF, eg pci_iov_add_virtfn().
This index is needed for device drivers that implement live migration for their internal operations that configure/control their VFs.
Specifically, mlx5_vfio_pci driver that is introduced in coming patches from this series needs it and not the bus/device/function which is exposed today.
Quoted 6 lines
Add pci_iov_vf_id() which computes the vf_id by reversing the math that was used to create the bus/device/function.
Signed-off-by Yishai Hadas <[email protected]>
Signed-off-by Jason Gunthorpe <[email protected]>
Signed-off-by Leon Romanovsky <[email protected]>
Acked-by Bjorn Helgaas <[email protected]>
mlx5_core_sriov_set_msix_vec_count() looks like it does basically the same thing as pci_iov_vf_id() by iterating through VFs until it finds one with a matching devfn (although it *doesn't* check for a matching bus number, which seems like a bug).
...
Quoted 5 lines
And it still looks like the existing code is buggy. This is called via sysfs, so if the PF is on bus X and the user writes to sriov_vf_msix_count for a VF on bus X+1, it looks like mlx5_core_sriov_set_msix_vec_count() will set the count for the wrong VF.
Quoted 6 lines
In mlx5_core_sriov_set_msix_vec_count(), we receive VF that is connected to PF which has "struct mlx5_core_dev". My expectation is that they share same bus as that PF was the one who created VFs. The mlx5 devices supports upto 256 VFs and it is far below the bus split mentioned in PCI spec.
How can VF and their respective PF have different bus numbers?
See PCIe r5.0, sec 9.2.1.2. For example,
PF 0 on bus 20
First VF Offset 1
VF Stride 1
NumVFs 511
VF 0,1 through VF 0,255 on bus 20
VF 0,256 through VF 0,511 on bus 21
Quoted 6 lines
This is implemented in pci_iov_add_virtfn(), which computes the bus number and devfn from the VF ID.
pci_iov_virtfn_devfn(VF 0,1) == pci_iov_virtfn_devfn(VF 0,256), so if the user writes to sriov_vf_msix_count for VF 0,256, it looks like we'll call mlx5_set_msix_vec_count() for VF 0,1 instead of VF 0,256.
This is PCI spec split that I mentioned.
Quoted 7 lines
The spec encourages devices that require no more than 256 devices to locate them all on the same bus number (PCIe r5.0, sec 9.1), so if you only have 255 VFs, you may avoid the problem.
But in mlx5_core_sriov_set_msix_vec_count(), it's not obvious that it is safe to assume the bus number is the same.

Quoted 8 lines

Quoted 5 lines
Quoted 1 line
No problem, we will make it more clear.
IMHO you should resolve it by using the new interface. Better performing, unambiguous regardless of how many VFs the device supports. What's the down side?
I don't see any. My previous answer worth to be written. "No problem, we will make it more clear with this new function".

Great, sorry I missed that nuance :)

Leon Romanovsky

author

2021-09-22 10:39 UTC #11 lore raw

PATCH 2/7 vfio: Add an API to check migration state transition validity

In reply to Leon Romanovsky

From: Yishai Hadas <[email protected]>

Add an API in the core layer to check migration state transition validity as part of a migration flow.

The valid transitions follow the expected usage as described in uapi/vfio.h and triggered by QEMU.

This ensures that all migration implementations follow a consistent migration state machine.

Signed-off-by Yishai Hadas <[email protected]>

Reviewed-by Kirti Wankhede <[email protected]>

Signed-off-by Jason Gunthorpe <[email protected]>

Signed-off-by Leon Romanovsky <[email protected]>

---
 drivers/vfio/vfio.c  | 41 +++++++++++++++++++++++++++++++++++++++++
 include/linux/vfio.h |  1 +
 2 files changed, 42 insertions(+)

drivers/vfio/vfio.c +41 -0

index 3c034fe14ccb..c3ca33e513c8 100644
@@ -1664,6 +1664,47 @@ static int vfio_device_fops_release(struct inode *inode, struct file *filep)
 	return 0;
 }
 
+/**
+ * vfio_change_migration_state_allowed - Checks whether a migration state
+ *   transition is valid.
+ * @new_state: The new state to move to.
+ * @old_state: The old state.
+ * Return: true if the transition is valid.
+ */
+bool vfio_change_migration_state_allowed(u32 new_state, u32 old_state)
+{
+	enum { MAX_STATE = VFIO_DEVICE_STATE_RESUMING };
+	static const u8 vfio_from_state_table[MAX_STATE + 1][MAX_STATE + 1] = {
+		[VFIO_DEVICE_STATE_STOP] = {
+			[VFIO_DEVICE_STATE_RUNNING] = 1,
+			[VFIO_DEVICE_STATE_RESUMING] = 1,
+		},
+		[VFIO_DEVICE_STATE_RUNNING] = {
+			[VFIO_DEVICE_STATE_STOP] = 1,
+			[VFIO_DEVICE_STATE_SAVING] = 1,
+			[VFIO_DEVICE_STATE_SAVING | VFIO_DEVICE_STATE_RUNNING] = 1,
+		},
+		[VFIO_DEVICE_STATE_SAVING] = {
+			[VFIO_DEVICE_STATE_STOP] = 1,
+			[VFIO_DEVICE_STATE_RUNNING] = 1,
+		},
+		[VFIO_DEVICE_STATE_SAVING | VFIO_DEVICE_STATE_RUNNING] = {
+			[VFIO_DEVICE_STATE_RUNNING] = 1,
+			[VFIO_DEVICE_STATE_SAVING] = 1,
+		},
+		[VFIO_DEVICE_STATE_RESUMING] = {
+			[VFIO_DEVICE_STATE_RUNNING] = 1,
+			[VFIO_DEVICE_STATE_STOP] = 1,
+		},
+	};
+
+	if (new_state > MAX_STATE || old_state > MAX_STATE)
+		return false;
+
+	return vfio_from_state_table[old_state][new_state];
+}
+EXPORT_SYMBOL_GPL(vfio_change_migration_state_allowed);
+
 static long vfio_device_fops_unl_ioctl(struct file *filep,
 				       unsigned int cmd, unsigned long arg)
 {

include/linux/vfio.h +1 -0

index b53a9557884a..e65137a708f1 100644
@@ -83,6 +83,7 @@ extern struct vfio_device *vfio_device_get_from_dev(struct device *dev);
 extern void vfio_device_put(struct vfio_device *device);
 
 int vfio_assign_device_set(struct vfio_device *device, void *set_id);
+bool vfio_change_migration_state_allowed(u32 new_state, u32 old_state);
 
 /* events for the backend driver notify callback */
 enum vfio_iommu_notify_type {

Signature · 1 line

2.31.1

Shameerali Kolothum Thodi

2021-09-23 10:33 UTC #12 lore raw

RE: [PATCH mlx5-next 2/7] vfio: Add an API to check migration state transition validity

Quoted 55 lines

-----Original Message-----
From: Leon Romanovsky [mailto:[email protected]]
Sent: 22 September 2021 11:39
To: Doug Ledford <[email protected]>; Jason Gunthorpe <[email protected]>
Cc: Yishai Hadas <[email protected]>; Alex Williamson <[email protected]>; Bjorn Helgaas <[email protected]>; David S. Miller <[email protected]>; Jakub Kicinski <[email protected]>; Kirti Wankhede <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Saeed Mahameed <[email protected]>
Subject: [PATCH mlx5-next 2/7] vfio: Add an API to check migration state transition validity
From: Yishai Hadas <[email protected]>
Add an API in the core layer to check migration state transition validity as part of a migration flow.
The valid transitions follow the expected usage as described in uapi/vfio.h and triggered by QEMU.
This ensures that all migration implementations follow a consistent migration state machine.
Signed-off-by Yishai Hadas <[email protected]>
Reviewed-by Kirti Wankhede <[email protected]>
Signed-off-by Jason Gunthorpe <[email protected]>
Signed-off-by Leon Romanovsky <[email protected]>
---
 drivers/vfio/vfio.c  | 41 +++++++++++++++++++++++++++++++++++++++++
 include/linux/vfio.h |  1 +
 2 files changed, 42 insertions(+)
drivers/vfio/vfio.c +10 -0
index 3c034fe14ccb..c3ca33e513c8 100644
@@ -1664,6 +1664,47 @@ static int vfio_device_fops_release(struct inode
 *inode, struct file *filep)
 	return 0;
 }
 
+/**
+ * vfio_change_migration_state_allowed - Checks whether a migration state
+ *   transition is valid.
+ * @new_state: The new state to move to.
+ * @old_state: The old state.
+ * Return: true if the transition is valid.
+ */
+bool vfio_change_migration_state_allowed(u32 new_state, u32 old_state)
+{
+	enum { MAX_STATE = VFIO_DEVICE_STATE_RESUMING };

drivers/vfio/vfio.c static int vfio_device_fops_release(struct inode · line 1687

Quoted 10 lines

drivers/vfio/vfio.c +9 -0

+	static const u8 vfio_from_state_table[MAX_STATE + 1][MAX_STATE + 1] = {
+		[VFIO_DEVICE_STATE_STOP] = {
+			[VFIO_DEVICE_STATE_RUNNING] = 1,
+			[VFIO_DEVICE_STATE_RESUMING] = 1,
+		},
+		[VFIO_DEVICE_STATE_RUNNING] = {
+			[VFIO_DEVICE_STATE_STOP] = 1,
+			[VFIO_DEVICE_STATE_SAVING] = 1,
+			[VFIO_DEVICE_STATE_SAVING | VFIO_DEVICE_STATE_RUNNING]
 = 1,

Do we need to allow _RESUMING state here or not? As per the "State transitions" section from uapi/linux/vfio.h,

" * 4. To start the resuming phase, the device state should be transitioned from
* the _RUNNING to the _RESUMING state."

IIRC, I have seen that transition happening on the destination dev while testing the HiSilicon ACC dev migration.

Thanks,
Shameer

Quoted 40 lines

drivers/vfio/vfio.c +22 -0

+		},
+		[VFIO_DEVICE_STATE_SAVING] = {
+			[VFIO_DEVICE_STATE_STOP] = 1,
+			[VFIO_DEVICE_STATE_RUNNING] = 1,
+		},
+		[VFIO_DEVICE_STATE_SAVING | VFIO_DEVICE_STATE_RUNNING] = {
+			[VFIO_DEVICE_STATE_RUNNING] = 1,
+			[VFIO_DEVICE_STATE_SAVING] = 1,
+		},
+		[VFIO_DEVICE_STATE_RESUMING] = {
+			[VFIO_DEVICE_STATE_RUNNING] = 1,
+			[VFIO_DEVICE_STATE_STOP] = 1,
+		},
+	};
+
+	if (new_state > MAX_STATE || old_state > MAX_STATE)
+		return false;
+
+	return vfio_from_state_table[old_state][new_state];
+}
+EXPORT_SYMBOL_GPL(vfio_change_migration_state_allowed);
+
 static long vfio_device_fops_unl_ioctl(struct file *filep,

drivers/vfio/vfio.c +0 -0

 				       unsigned int cmd, unsigned long arg)
 {

include/linux/vfio.h +1 -0

index b53a9557884a..e65137a708f1 100644
@@ -83,6 +83,7 @@ extern struct vfio_device
 *vfio_device_get_from_dev(struct device *dev);
 extern void vfio_device_put(struct vfio_device *device);
 
 int vfio_assign_device_set(struct vfio_device *device, void *set_id);
+bool vfio_change_migration_state_allowed(u32 new_state, u32 old_state);
 
 /* events for the backend driver notify callback */

enum vfio_iommu_notify_type {
--
2.31.1

Leon Romanovsky

author

2021-09-23 11:17 UTC #13 lore raw

Re: [PATCH mlx5-next 2/7] vfio: Add an API to check migration state transition validity

Quoted 60 lines · Shameerali Kolothum Thodi

Quoted 60 lines
-----Original Message-----
From: Leon Romanovsky [mailto:[email protected]]
Sent: 22 September 2021 11:39
To: Doug Ledford <[email protected]>; Jason Gunthorpe <[email protected]>
Cc: Yishai Hadas <[email protected]>; Alex Williamson <[email protected]>; Bjorn Helgaas <[email protected]>; David S. Miller <[email protected]>; Jakub Kicinski <[email protected]>; Kirti Wankhede <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Saeed Mahameed <[email protected]>
Subject: [PATCH mlx5-next 2/7] vfio: Add an API to check migration state transition validity
From: Yishai Hadas <[email protected]>
Add an API in the core layer to check migration state transition validity as part of a migration flow.
The valid transitions follow the expected usage as described in uapi/vfio.h and triggered by QEMU.
This ensures that all migration implementations follow a consistent migration state machine.
Signed-off-by Yishai Hadas <[email protected]>
Reviewed-by Kirti Wankhede <[email protected]>
Signed-off-by Jason Gunthorpe <[email protected]>
Signed-off-by Leon Romanovsky <[email protected]>
---
 drivers/vfio/vfio.c  | 41 +++++++++++++++++++++++++++++++++++++++++
 include/linux/vfio.h |  1 +
 2 files changed, 42 insertions(+)
drivers/vfio/vfio.c +15 -0
index 3c034fe14ccb..c3ca33e513c8 100644
@@ -1664,6 +1664,47 @@ static int vfio_device_fops_release(struct inode
 *inode, struct file *filep)
 	return 0;
 }
 
+/**
+ * vfio_change_migration_state_allowed - Checks whether a migration state
+ *   transition is valid.
+ * @new_state: The new state to move to.
+ * @old_state: The old state.
+ * Return: true if the transition is valid.
+ */
+bool vfio_change_migration_state_allowed(u32 new_state, u32 old_state)
+{
+	enum { MAX_STATE = VFIO_DEVICE_STATE_RESUMING };
+	static const u8 vfio_from_state_table[MAX_STATE + 1][MAX_STATE + 1] = {
+		[VFIO_DEVICE_STATE_STOP] = {
+			[VFIO_DEVICE_STATE_RUNNING] = 1,
+			[VFIO_DEVICE_STATE_RESUMING] = 1,
+		},

Quoted 8 lines

drivers/vfio/vfio.c static int vfio_device_fops_release(struct inode *inode, struct file *filep)
Quoted 5 lines
drivers/vfio/vfio.c +4 -0
+		[VFIO_DEVICE_STATE_RUNNING] = {
+			[VFIO_DEVICE_STATE_STOP] = 1,
+			[VFIO_DEVICE_STATE_SAVING] = 1,
+			[VFIO_DEVICE_STATE_SAVING | VFIO_DEVICE_STATE_RUNNING]
 = 1,
Do we need to allow _RESUMING state here or not? As per the "State transitions" section from uapi/linux/vfio.h,

It looks like we missed this state transition.

Thanks

Quoted 51 lines

" * 4. To start the resuming phase, the device state should be transitioned from
* the _RUNNING to the _RESUMING state."

IIRC, I have seen that transition happening on the destination dev while testing the HiSilicon ACC dev migration.

Thanks,
Shameer

Quoted 40 lines

drivers/vfio/vfio.c +22 -0

+		},
+		[VFIO_DEVICE_STATE_SAVING] = {
+			[VFIO_DEVICE_STATE_STOP] = 1,
+			[VFIO_DEVICE_STATE_RUNNING] = 1,
+		},
+		[VFIO_DEVICE_STATE_SAVING | VFIO_DEVICE_STATE_RUNNING] = {
+			[VFIO_DEVICE_STATE_RUNNING] = 1,
+			[VFIO_DEVICE_STATE_SAVING] = 1,
+		},
+		[VFIO_DEVICE_STATE_RESUMING] = {
+			[VFIO_DEVICE_STATE_RUNNING] = 1,
+			[VFIO_DEVICE_STATE_STOP] = 1,
+		},
+	};
+
+	if (new_state > MAX_STATE || old_state > MAX_STATE)
+		return false;
+
+	return vfio_from_state_table[old_state][new_state];
+}
+EXPORT_SYMBOL_GPL(vfio_change_migration_state_allowed);
+
 static long vfio_device_fops_unl_ioctl(struct file *filep,

drivers/vfio/vfio.c +0 -0

 				       unsigned int cmd, unsigned long arg)
 {

include/linux/vfio.h +1 -0

index b53a9557884a..e65137a708f1 100644
@@ -83,6 +83,7 @@ extern struct vfio_device
 *vfio_device_get_from_dev(struct device *dev);
 extern void vfio_device_put(struct vfio_device *device);
 
 int vfio_assign_device_set(struct vfio_device *device, void *set_id);
+bool vfio_change_migration_state_allowed(u32 new_state, u32 old_state);
 
 /* events for the backend driver notify callback */

enum vfio_iommu_notify_type {
--
2.31.1

Max Gurtovoy

2021-09-23 13:55 UTC #14 lore raw

Re: [PATCH mlx5-next 2/7] vfio: Add an API to check migration state transition validity

Quoted 67 lines · Leon Romanovsky

Quoted 58 lines · Shameerali Kolothum Thodi
Quoted 58 lines
-----Original Message-----
From: Leon Romanovsky [mailto:[email protected]]
Sent: 22 September 2021 11:39
To: Doug Ledford <[email protected]>; Jason Gunthorpe <[email protected]>
Cc: Yishai Hadas <[email protected]>; Alex Williamson <[email protected]>; Bjorn Helgaas <[email protected]>; David S. Miller <[email protected]>; Jakub Kicinski <[email protected]>; Kirti Wankhede <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Saeed Mahameed <[email protected]>
Subject: [PATCH mlx5-next 2/7] vfio: Add an API to check migration state transition validity
From: Yishai Hadas <[email protected]>
Add an API in the core layer to check migration state transition validity as part of a migration flow.
The valid transitions follow the expected usage as described in uapi/vfio.h and triggered by QEMU.
This ensures that all migration implementations follow a consistent migration state machine.
Signed-off-by Yishai Hadas <[email protected]>
Reviewed-by Kirti Wankhede <[email protected]>
Signed-off-by Jason Gunthorpe <[email protected]>
Signed-off-by Leon Romanovsky <[email protected]>
---
  drivers/vfio/vfio.c  | 41 +++++++++++++++++++++++++++++++++++++++++
  include/linux/vfio.h |  1 +
  2 files changed, 42 insertions(+)
drivers/vfio/vfio.c +14 -0
index 3c034fe14ccb..c3ca33e513c8 100644
@@ -1664,6 +1664,47 @@ static int vfio_device_fops_release(struct inode
 *inode, struct file *filep)
  	return 0;
  }
 
+/**
+ * vfio_change_migration_state_allowed - Checks whether a migration state
+ *   transition is valid.
+ * @new_state: The new state to move to.
+ * @old_state: The old state.
+ * Return: true if the transition is valid.
+ */
+bool vfio_change_migration_state_allowed(u32 new_state, u32 old_state)
+{
+	enum { MAX_STATE = VFIO_DEVICE_STATE_RESUMING };
+	static const u8 vfio_from_state_table[MAX_STATE + 1][MAX_STATE + 1] = {
+		[VFIO_DEVICE_STATE_STOP] = {
+			[VFIO_DEVICE_STATE_RUNNING] = 1,
+			[VFIO_DEVICE_STATE_RESUMING] = 1,
Quoted 8 lines
drivers/vfio/vfio.c static int vfio_device_fops_release(struct inode *inode, struct file *filep)
Quoted 6 lines
drivers/vfio/vfio.c +5 -0
+		},
+		[VFIO_DEVICE_STATE_RUNNING] = {
+			[VFIO_DEVICE_STATE_STOP] = 1,
+			[VFIO_DEVICE_STATE_SAVING] = 1,
+			[VFIO_DEVICE_STATE_SAVING | VFIO_DEVICE_STATE_RUNNING]
 = 1,
Do we need to allow _RESUMING state here or not? As per the "State transitions" section from uapi/linux/vfio.h,

Quoted 3 lines

It looks like we missed this state transition.
Thanks

I'm not sure this state transition is valid.

Kirti, When we would like to move from RUNNING to RESUMING ?

Sameerali, can you please re-test and update if you see this transition ?

Quoted 50 lines

" * 4. To start the resuming phase, the device state should be transitioned from
* the _RUNNING to the _RESUMING state."

IIRC, I have seen that transition happening on the destination dev while testing the HiSilicon ACC dev migration.

Thanks,
Shameer

Quoted 40 lines

drivers/vfio/vfio.c +22 -0

+		},
+		[VFIO_DEVICE_STATE_SAVING] = {
+			[VFIO_DEVICE_STATE_STOP] = 1,
+			[VFIO_DEVICE_STATE_RUNNING] = 1,
+		},
+		[VFIO_DEVICE_STATE_SAVING | VFIO_DEVICE_STATE_RUNNING] = {
+			[VFIO_DEVICE_STATE_RUNNING] = 1,
+			[VFIO_DEVICE_STATE_SAVING] = 1,
+		},
+		[VFIO_DEVICE_STATE_RESUMING] = {
+			[VFIO_DEVICE_STATE_RUNNING] = 1,
+			[VFIO_DEVICE_STATE_STOP] = 1,
+		},
+	};
+
+	if (new_state > MAX_STATE || old_state > MAX_STATE)
+		return false;
+
+	return vfio_from_state_table[old_state][new_state];
+}
+EXPORT_SYMBOL_GPL(vfio_change_migration_state_allowed);
+
  static long vfio_device_fops_unl_ioctl(struct file *filep,

unsigned int cmd, unsigned long arg)
{

include/linux/vfio.h +1 -0

index b53a9557884a..e65137a708f1 100644
@@ -83,6 +83,7 @@ extern struct vfio_device
 *vfio_device_get_from_dev(struct device *dev);
  extern void vfio_device_put(struct vfio_device *device);
 
  int vfio_assign_device_set(struct vfio_device *device, void *set_id);
+bool vfio_change_migration_state_allowed(u32 new_state, u32 old_state);
 
  /* events for the backend driver notify callback */

enum vfio_iommu_notify_type {
--
2.31.1

Shameerali Kolothum Thodi

2021-09-24 07:44 UTC #15 lore raw

RE: [PATCH mlx5-next 2/7] vfio: Add an API to check migration state transition validity

Quoted 96 lines

-----Original Message-----
From: Max Gurtovoy [mailto:[email protected]]
Sent: 23 September 2021 14:56
To: Leon Romanovsky <[email protected]>; Shameerali Kolothum Thodi <[email protected]>
Cc: Doug Ledford <[email protected]>; Jason Gunthorpe <[email protected]>; Yishai Hadas <[email protected]>; Alex Williamson <[email protected]>; Bjorn Helgaas <[email protected]>; David S. Miller <[email protected]>; Jakub Kicinski <[email protected]>; Kirti Wankhede <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Saeed Mahameed <[email protected]>
Subject: Re: [PATCH mlx5-next 2/7] vfio: Add an API to check migration state transition validity
Quoted 1 line · Leon Romanovsky
On Thu, Sep 23, 2021 at 10:33:10AM +0000, Shameerali Kolothum Thodi
Quoted 5 lines · wrote:
Quoted 5 lines
Quoted 5 lines
-----Original Message-----
From: Leon Romanovsky [mailto:[email protected]]
Sent: 22 September 2021 11:39
To: Doug Ledford <[email protected]>; Jason Gunthorpe
<[email protected]>
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Cc: Yishai Hadas <[email protected]>; Alex Williamson <[email protected]>; Bjorn Helgaas <[email protected]>;
David
Quoted 19 lines
Quoted 19 lines
Quoted 19 lines
S. Miller <[email protected]>; Jakub Kicinski <[email protected]>; Kirti Wankhede <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Saeed Mahameed <[email protected]>
Subject: [PATCH mlx5-next 2/7] vfio: Add an API to check migration state transition validity
From: Yishai Hadas <[email protected]>
Add an API in the core layer to check migration state transition validity as part of a migration flow.
The valid transitions follow the expected usage as described in uapi/vfio.h and triggered by QEMU.
This ensures that all migration implementations follow a consistent migration state machine.
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Signed-off-by Yishai Hadas <[email protected]>
Reviewed-by Kirti Wankhede <[email protected]>
Signed-off-by Jason Gunthorpe <[email protected]>
Signed-off-by Leon Romanovsky <[email protected]>
---
  drivers/vfio/vfio.c  | 41
+++++++++++++++++++++++++++++++++++++++++
Quoted 8 lines
Quoted 8 lines
drivers/vfio/vfio.c static int vfio_device_fops_release(struct
Quoted 8 lines
  include/linux/vfio.h |  1 +
  2 files changed, 42 insertions(+)
drivers/vfio/vfio.c +0 -0
index 3c034fe14ccb..c3ca33e513c8 100644
@@ -1664,6 +1664,47 @@ static int vfio_device_fops_release(struct
inode
Quoted 6 lines
Quoted 6 lines
drivers/vfio/vfio.c static int vfio_device_fops_release(struct inode *inode, struct file *filep)
Quoted 6 lines
drivers/vfio/vfio.c +2 -0
 *inode, struct file *filep)
  	return 0;
  }
 
+/**
+ * vfio_change_migration_state_allowed - Checks whether a migration
state
Quoted 6 lines
Quoted 6 lines
drivers/vfio/vfio.c static int vfio_device_fops_release(struct inode *inode, struct file *filep)
Quoted 6 lines
drivers/vfio/vfio.c +6 -0
+ *   transition is valid.
+ * @new_state: The new state to move to.
+ * @old_state: The old state.
+ * Return: true if the transition is valid.
+ */
+bool vfio_change_migration_state_allowed(u32 new_state, u32
old_state)
Quoted 3 lines
Quoted 3 lines
drivers/vfio/vfio.c static int vfio_device_fops_release(struct inode *inode, struct file *filep)
Quoted 3 lines
drivers/vfio/vfio.c +3 -0
+{
+	enum { MAX_STATE = VFIO_DEVICE_STATE_RESUMING };
+	static const u8 vfio_from_state_table[MAX_STATE + 1][MAX_STATE +
1] = {
Quoted 8 lines
Quoted 8 lines
drivers/vfio/vfio.c static int vfio_device_fops_release(struct inode *inode, struct file *filep)
Quoted 8 lines
drivers/vfio/vfio.c +8 -0
+		[VFIO_DEVICE_STATE_STOP] = {
+			[VFIO_DEVICE_STATE_RUNNING] = 1,
+			[VFIO_DEVICE_STATE_RESUMING] = 1,
+		},
+		[VFIO_DEVICE_STATE_RUNNING] = {
+			[VFIO_DEVICE_STATE_STOP] = 1,
+			[VFIO_DEVICE_STATE_SAVING] = 1,
+			[VFIO_DEVICE_STATE_SAVING |
VFIO_DEVICE_STATE_RUNNING]
Quoted 2 lines
Quoted 2 lines
drivers/vfio/vfio.c static int vfio_device_fops_release(struct · line 1687
Quoted 1 line
drivers/vfio/vfio.c +0 -0
 = 1,
Do we need to allow _RESUMING state here or not? As per the "State
transitions"

Quoted 8 lines

Quoted 4 lines
Quoted 1 line
section from uapi/linux/vfio.h,
It looks like we missed this state transition.
Thanks
I'm not sure this state transition is valid.
Kirti, When we would like to move from RUNNING to RESUMING ?

I guess it depends on what you report as your dev default state.

For HiSilicon ACC migration driver, we set the default to _RUNNING.

And when the migration starts, the destination side Qemu, set the device state to _RESUMING(vfio_load_state()).

From the documentation, it looks like the assumption on default state of the VFIO dev is _RUNNING.

"
* 001b => Device running, which is the default state "

Quoted 2 lines

Sameerali, can you please re-test and update if you see this transition ?

Yes. And if I change the default state to _STOP, then the transition is from _STOP --> _RESUMING.

But the documentation on State transitions doesn't have _STOP --> _RESUMING transition as valid.

Thanks,
Shameer

Quoted 56 lines

Quoted 4 lines
Quoted 4 lines
" * 4. To start the resuming phase, the device state should be transitioned
from
Quoted 3 lines
Quoted 3 lines
* the _RUNNING to the _RESUMING state."
IIRC, I have seen that transition happening on the destination dev while
testing the
Quoted 11 lines
Quoted 11 lines
HiSilicon ACC dev migration.
Thanks,
Shameer
drivers/vfio/vfio.c static int vfio_device_fops_release(struct inode *inode, struct file *filep)
Quoted 6 lines
drivers/vfio/vfio.c +6 -0
+		},
+		[VFIO_DEVICE_STATE_SAVING] = {
+			[VFIO_DEVICE_STATE_STOP] = 1,
+			[VFIO_DEVICE_STATE_RUNNING] = 1,
+		},
+		[VFIO_DEVICE_STATE_SAVING | VFIO_DEVICE_STATE_RUNNING]
= {
Quoted 27 lines
Quoted 27 lines
Quoted 27 lines
drivers/vfio/vfio.c +16 -0
+			[VFIO_DEVICE_STATE_RUNNING] = 1,
+			[VFIO_DEVICE_STATE_SAVING] = 1,
+		},
+		[VFIO_DEVICE_STATE_RESUMING] = {
+			[VFIO_DEVICE_STATE_RUNNING] = 1,
+			[VFIO_DEVICE_STATE_STOP] = 1,
+		},
+	};
+
+	if (new_state > MAX_STATE || old_state > MAX_STATE)
+		return false;
+
+	return vfio_from_state_table[old_state][new_state];
+}
+EXPORT_SYMBOL_GPL(vfio_change_migration_state_allowed);
+
  static long vfio_device_fops_unl_ioctl(struct file *filep,
unsigned int cmd, unsigned long arg)
{
include/linux/vfio.h +0 -0
index b53a9557884a..e65137a708f1 100644
@@ -83,6 +83,7 @@ extern struct vfio_device
 *vfio_device_get_from_dev(struct device *dev);
  extern void vfio_device_put(struct vfio_device *device);
 
Quoted 2 lines
Quoted 2 lines
include/linux/vfio.h extern struct vfio_device · line 87
Quoted 2 lines
include/linux/vfio.h +1 -0
  int vfio_assign_device_set(struct vfio_device *device, void *set_id);
+bool vfio_change_migration_state_allowed(u32 new_state, u32
old_state);
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
include/linux/vfio.h +0 -1
  /* events for the backend driver notify callback */
  enum vfio_iommu_notify_type {
--
2.31.1

Kirti Wankhede

2021-09-24 09:37 UTC #16 lore raw

Re: [PATCH mlx5-next 2/7] vfio: Add an API to check migration state transition validity

Quoted 107 lines · Shameerali Kolothum Thodi

Quoted 96 lines
-----Original Message-----
From: Max Gurtovoy [mailto:[email protected]]
Sent: 23 September 2021 14:56
To: Leon Romanovsky <[email protected]>; Shameerali Kolothum Thodi <[email protected]>
Cc: Doug Ledford <[email protected]>; Jason Gunthorpe <[email protected]>; Yishai Hadas <[email protected]>; Alex Williamson <[email protected]>; Bjorn Helgaas <[email protected]>; David S. Miller <[email protected]>; Jakub Kicinski <[email protected]>; Kirti Wankhede <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Saeed Mahameed <[email protected]>
Subject: Re: [PATCH mlx5-next 2/7] vfio: Add an API to check migration state transition validity
Quoted 1 line · Leon Romanovsky
On Thu, Sep 23, 2021 at 10:33:10AM +0000, Shameerali Kolothum Thodi
Quoted 5 lines · wrote:
Quoted 5 lines
Quoted 5 lines
-----Original Message-----
From: Leon Romanovsky [mailto:[email protected]]
Sent: 22 September 2021 11:39
To: Doug Ledford <[email protected]>; Jason Gunthorpe
<[email protected]>
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Cc: Yishai Hadas <[email protected]>; Alex Williamson <[email protected]>; Bjorn Helgaas <[email protected]>;
David
Quoted 19 lines
Quoted 19 lines
Quoted 19 lines
S. Miller <[email protected]>; Jakub Kicinski <[email protected]>; Kirti Wankhede <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Saeed Mahameed <[email protected]>
Subject: [PATCH mlx5-next 2/7] vfio: Add an API to check migration state transition validity
From: Yishai Hadas <[email protected]>
Add an API in the core layer to check migration state transition validity as part of a migration flow.
The valid transitions follow the expected usage as described in uapi/vfio.h and triggered by QEMU.
This ensures that all migration implementations follow a consistent migration state machine.
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Signed-off-by Yishai Hadas <[email protected]>
Reviewed-by Kirti Wankhede <[email protected]>
Signed-off-by Jason Gunthorpe <[email protected]>
Signed-off-by Leon Romanovsky <[email protected]>
---
   drivers/vfio/vfio.c  | 41
+++++++++++++++++++++++++++++++++++++++++
Quoted 8 lines
Quoted 8 lines
drivers/vfio/vfio.c static int vfio_device_fops_release(struct
Quoted 8 lines
   include/linux/vfio.h |  1 +
   2 files changed, 42 insertions(+)
drivers/vfio/vfio.c +0 -0
index 3c034fe14ccb..c3ca33e513c8 100644
@@ -1664,6 +1664,47 @@ static int vfio_device_fops_release(struct
inode
Quoted 6 lines
Quoted 6 lines
drivers/vfio/vfio.c static int vfio_device_fops_release(struct inode *inode, struct file *filep)
Quoted 6 lines
drivers/vfio/vfio.c +2 -0
 *inode, struct file *filep)
   	return 0;
   }
 
+/**
+ * vfio_change_migration_state_allowed - Checks whether a migration
state
Quoted 6 lines
Quoted 6 lines
drivers/vfio/vfio.c static int vfio_device_fops_release(struct inode *inode, struct file *filep)
Quoted 6 lines
drivers/vfio/vfio.c +6 -0
+ *   transition is valid.
+ * @new_state: The new state to move to.
+ * @old_state: The old state.
+ * Return: true if the transition is valid.
+ */
+bool vfio_change_migration_state_allowed(u32 new_state, u32
old_state)
Quoted 3 lines
Quoted 3 lines
drivers/vfio/vfio.c static int vfio_device_fops_release(struct inode *inode, struct file *filep)
Quoted 3 lines
drivers/vfio/vfio.c +3 -0
+{
+	enum { MAX_STATE = VFIO_DEVICE_STATE_RESUMING };
+	static const u8 vfio_from_state_table[MAX_STATE + 1][MAX_STATE +
1] = {
Quoted 8 lines
Quoted 8 lines
drivers/vfio/vfio.c static int vfio_device_fops_release(struct inode *inode, struct file *filep)
Quoted 8 lines
drivers/vfio/vfio.c +8 -0
+		[VFIO_DEVICE_STATE_STOP] = {
+			[VFIO_DEVICE_STATE_RUNNING] = 1,
+			[VFIO_DEVICE_STATE_RESUMING] = 1,
+		},
+		[VFIO_DEVICE_STATE_RUNNING] = {
+			[VFIO_DEVICE_STATE_STOP] = 1,
+			[VFIO_DEVICE_STATE_SAVING] = 1,
+			[VFIO_DEVICE_STATE_SAVING |
VFIO_DEVICE_STATE_RUNNING]
Quoted 2 lines
Quoted 2 lines
drivers/vfio/vfio.c static int vfio_device_fops_release(struct · line 1687
Quoted 1 line
drivers/vfio/vfio.c +0 -0
 = 1,
Do we need to allow _RESUMING state here or not? As per the "State
transitions"
Quoted 8 lines
Quoted 4 lines
Quoted 1 line
section from uapi/linux/vfio.h,
It looks like we missed this state transition.
Thanks
I'm not sure this state transition is valid.
Kirti, When we would like to move from RUNNING to RESUMING ?
I guess it depends on what you report as your dev default state.

Quoted 8 lines

For HiSilicon ACC migration driver, we set the default to _RUNNING.
And when the migration starts, the destination side Qemu, set the device state to _RESUMING(vfio_load_state()).
From the documentation, it looks like the assumption on default state of
the VFIO dev is _RUNNING.

That's right. in QEMU VFIO device state at init is running to maintain backward compatibility since migration support was added later.

RUNNING -> RESUMING state transition is valid.

Thanks,
Kirti

Quoted 72 lines

"
* 001b => Device running, which is the default state "
Quoted 2 lines
Sameerali, can you please re-test and update if you see this transition ?
Yes. And if I change the default state to _STOP, then the transition is from _STOP --> _RESUMING.
But the documentation on State transitions doesn't have _STOP --> _RESUMING transition as valid.
Thanks,
Shameer
Quoted 56 lines
Quoted 4 lines
Quoted 4 lines
" * 4. To start the resuming phase, the device state should be transitioned
from
Quoted 3 lines
Quoted 3 lines
* the _RUNNING to the _RESUMING state."
IIRC, I have seen that transition happening on the destination dev while
testing the
Quoted 11 lines
Quoted 11 lines
HiSilicon ACC dev migration.
Thanks,
Shameer
drivers/vfio/vfio.c static int vfio_device_fops_release(struct inode *inode, struct file *filep)
Quoted 6 lines
drivers/vfio/vfio.c +6 -0
+		},
+		[VFIO_DEVICE_STATE_SAVING] = {
+			[VFIO_DEVICE_STATE_STOP] = 1,
+			[VFIO_DEVICE_STATE_RUNNING] = 1,
+		},
+		[VFIO_DEVICE_STATE_SAVING | VFIO_DEVICE_STATE_RUNNING]
= {
Quoted 27 lines
Quoted 27 lines
Quoted 27 lines
drivers/vfio/vfio.c +16 -0
+			[VFIO_DEVICE_STATE_RUNNING] = 1,
+			[VFIO_DEVICE_STATE_SAVING] = 1,
+		},
+		[VFIO_DEVICE_STATE_RESUMING] = {
+			[VFIO_DEVICE_STATE_RUNNING] = 1,
+			[VFIO_DEVICE_STATE_STOP] = 1,
+		},
+	};
+
+	if (new_state > MAX_STATE || old_state > MAX_STATE)
+		return false;
+
+	return vfio_from_state_table[old_state][new_state];
+}
+EXPORT_SYMBOL_GPL(vfio_change_migration_state_allowed);
+
   static long vfio_device_fops_unl_ioctl(struct file *filep,
unsigned int cmd, unsigned long arg)
{
include/linux/vfio.h +0 -0
index b53a9557884a..e65137a708f1 100644
@@ -83,6 +83,7 @@ extern struct vfio_device
 *vfio_device_get_from_dev(struct device *dev);
   extern void vfio_device_put(struct vfio_device *device);
 
Quoted 2 lines
Quoted 2 lines
include/linux/vfio.h extern struct vfio_device · line 87
Quoted 2 lines
include/linux/vfio.h +1 -0
   int vfio_assign_device_set(struct vfio_device *device, void *set_id);
+bool vfio_change_migration_state_allowed(u32 new_state, u32
old_state);
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
include/linux/vfio.h +0 -1
   /* events for the backend driver notify callback */
   enum vfio_iommu_notify_type {
--
2.31.1

Max Gurtovoy

2021-09-26 09:09 UTC #17 lore raw

Re: [PATCH mlx5-next 2/7] vfio: Add an API to check migration state transition validity

In reply to Shameerali Kolothum Thodi

Quoted 97 lines · Shameerali Kolothum Thodi

Quoted 97 lines
-----Original Message-----
From: Max Gurtovoy [mailto:[email protected]]
Sent: 23 September 2021 14:56
To: Leon Romanovsky <[email protected]>; Shameerali Kolothum Thodi <[email protected]>
Cc: Doug Ledford <[email protected]>; Jason Gunthorpe <[email protected]>; Yishai Hadas <[email protected]>; Alex Williamson <[email protected]>; Bjorn Helgaas <[email protected]>; David S. Miller <[email protected]>; Jakub Kicinski <[email protected]>; Kirti Wankhede <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Saeed Mahameed <[email protected]>
Subject: Re: [PATCH mlx5-next 2/7] vfio: Add an API to check migration state transition validity
Quoted 1 line · Leon Romanovsky
On Thu, Sep 23, 2021 at 10:33:10AM +0000, Shameerali Kolothum Thodi
Quoted 4 lines · wrote:
Quoted 4 lines
Quoted 4 lines
-----Original Message-----
From: Leon Romanovsky [mailto:[email protected]]
Sent: 22 September 2021 11:39
To: Doug Ledford <[email protected]>; Jason Gunthorpe
<[email protected]>
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Cc: Yishai Hadas <[email protected]>; Alex Williamson <[email protected]>; Bjorn Helgaas <[email protected]>;
David
Quoted 19 lines
Quoted 19 lines
Quoted 19 lines
S. Miller <[email protected]>; Jakub Kicinski <[email protected]>; Kirti Wankhede <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Saeed Mahameed <[email protected]>
Subject: [PATCH mlx5-next 2/7] vfio: Add an API to check migration state transition validity
From: Yishai Hadas <[email protected]>
Add an API in the core layer to check migration state transition validity as part of a migration flow.
The valid transitions follow the expected usage as described in uapi/vfio.h and triggered by QEMU.
This ensures that all migration implementations follow a consistent migration state machine.
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Signed-off-by Yishai Hadas <[email protected]>
Reviewed-by Kirti Wankhede <[email protected]>
Signed-off-by Jason Gunthorpe <[email protected]>
Signed-off-by Leon Romanovsky <[email protected]>
---
   drivers/vfio/vfio.c  | 41
+++++++++++++++++++++++++++++++++++++++++
Quoted 8 lines
Quoted 8 lines
drivers/vfio/vfio.c static int vfio_device_fops_release(struct
Quoted 8 lines
   include/linux/vfio.h |  1 +
   2 files changed, 42 insertions(+)
drivers/vfio/vfio.c +0 -0
index 3c034fe14ccb..c3ca33e513c8 100644
@@ -1664,6 +1664,47 @@ static int vfio_device_fops_release(struct
inode
Quoted 6 lines
Quoted 6 lines
drivers/vfio/vfio.c static int vfio_device_fops_release(struct inode *inode, struct file *filep)
Quoted 6 lines
drivers/vfio/vfio.c +2 -0
 *inode, struct file *filep)
   	return 0;
   }
 
+/**
+ * vfio_change_migration_state_allowed - Checks whether a migration
state
Quoted 6 lines
Quoted 6 lines
drivers/vfio/vfio.c static int vfio_device_fops_release(struct inode *inode, struct file *filep)
Quoted 6 lines
drivers/vfio/vfio.c +6 -0
+ *   transition is valid.
+ * @new_state: The new state to move to.
+ * @old_state: The old state.
+ * Return: true if the transition is valid.
+ */
+bool vfio_change_migration_state_allowed(u32 new_state, u32
old_state)
Quoted 3 lines
Quoted 3 lines
drivers/vfio/vfio.c static int vfio_device_fops_release(struct inode *inode, struct file *filep)
Quoted 3 lines
drivers/vfio/vfio.c +3 -0
+{
+	enum { MAX_STATE = VFIO_DEVICE_STATE_RESUMING };
+	static const u8 vfio_from_state_table[MAX_STATE + 1][MAX_STATE +
1] = {
Quoted 8 lines
Quoted 8 lines
drivers/vfio/vfio.c static int vfio_device_fops_release(struct inode *inode, struct file *filep)
Quoted 8 lines
drivers/vfio/vfio.c +8 -0
+		[VFIO_DEVICE_STATE_STOP] = {
+			[VFIO_DEVICE_STATE_RUNNING] = 1,
+			[VFIO_DEVICE_STATE_RESUMING] = 1,
+		},
+		[VFIO_DEVICE_STATE_RUNNING] = {
+			[VFIO_DEVICE_STATE_STOP] = 1,
+			[VFIO_DEVICE_STATE_SAVING] = 1,
+			[VFIO_DEVICE_STATE_SAVING |
VFIO_DEVICE_STATE_RUNNING]
Quoted 2 lines
Quoted 2 lines
drivers/vfio/vfio.c static int vfio_device_fops_release(struct · line 1687
Quoted 1 line
drivers/vfio/vfio.c +0 -0
 = 1,
Do we need to allow _RESUMING state here or not? As per the "State
transitions"
Quoted 3 lines
Quoted 1 line
section from uapi/linux/vfio.h,
It looks like we missed this state transition.

Quoted 7 lines

Quoted 4 lines
Quoted 1 line
Thanks
I'm not sure this state transition is valid.
Kirti, When we would like to move from RUNNING to RESUMING ?
I guess it depends on what you report as your dev default state.
For HiSilicon ACC migration driver, we set the default to _RUNNING.

Where do you set it and report it ?

Quoted 74 lines

And when the migration starts, the destination side Qemu, set the device state to _RESUMING(vfio_load_state()).
From the documentation, it looks like the assumption on default state of
the VFIO dev is _RUNNING.
"
* 001b => Device running, which is the default state "
Quoted 1 line
Sameerali, can you please re-test and update if you see this transition ?
Yes. And if I change the default state to _STOP, then the transition is from _STOP --> _RESUMING.
But the documentation on State transitions doesn't have _STOP --> _RESUMING transition as valid.
Thanks,
Shameer
Quoted 53 lines
Quoted 2 lines
Quoted 2 lines
" * 4. To start the resuming phase, the device state should be transitioned
from
Quoted 3 lines
Quoted 3 lines
* the _RUNNING to the _RESUMING state."
IIRC, I have seen that transition happening on the destination dev while
testing the
Quoted 11 lines
Quoted 11 lines
HiSilicon ACC dev migration.
Thanks,
Shameer
drivers/vfio/vfio.c static int vfio_device_fops_release(struct inode *inode, struct file *filep)
Quoted 6 lines
drivers/vfio/vfio.c +6 -0
+		},
+		[VFIO_DEVICE_STATE_SAVING] = {
+			[VFIO_DEVICE_STATE_STOP] = 1,
+			[VFIO_DEVICE_STATE_RUNNING] = 1,
+		},
+		[VFIO_DEVICE_STATE_SAVING | VFIO_DEVICE_STATE_RUNNING]
= {
Quoted 27 lines
Quoted 27 lines
Quoted 27 lines
drivers/vfio/vfio.c +16 -0
+			[VFIO_DEVICE_STATE_RUNNING] = 1,
+			[VFIO_DEVICE_STATE_SAVING] = 1,
+		},
+		[VFIO_DEVICE_STATE_RESUMING] = {
+			[VFIO_DEVICE_STATE_RUNNING] = 1,
+			[VFIO_DEVICE_STATE_STOP] = 1,
+		},
+	};
+
+	if (new_state > MAX_STATE || old_state > MAX_STATE)
+		return false;
+
+	return vfio_from_state_table[old_state][new_state];
+}
+EXPORT_SYMBOL_GPL(vfio_change_migration_state_allowed);
+
   static long vfio_device_fops_unl_ioctl(struct file *filep,
unsigned int cmd, unsigned long arg)
{
include/linux/vfio.h +0 -0
index b53a9557884a..e65137a708f1 100644
@@ -83,6 +83,7 @@ extern struct vfio_device
 *vfio_device_get_from_dev(struct device *dev);
   extern void vfio_device_put(struct vfio_device *device);
 
Quoted 2 lines
Quoted 2 lines
include/linux/vfio.h extern struct vfio_device · line 87
Quoted 2 lines
include/linux/vfio.h +1 -0
   int vfio_assign_device_set(struct vfio_device *device, void *set_id);
+bool vfio_change_migration_state_allowed(u32 new_state, u32
old_state);
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
include/linux/vfio.h +0 -1
   /* events for the backend driver notify callback */
   enum vfio_iommu_notify_type {
--
2.31.1

Shameerali Kolothum Thodi

2021-09-26 16:17 UTC #18 lore raw

RE: [PATCH mlx5-next 2/7] vfio: Add an API to check migration state transition validity

Quoted 119 lines

-----Original Message-----
From: Max Gurtovoy [mailto:[email protected]]
Sent: 26 September 2021 10:10
To: Shameerali Kolothum Thodi <[email protected]>; Leon Romanovsky <[email protected]>
Cc: Doug Ledford <[email protected]>; Jason Gunthorpe <[email protected]>; Yishai Hadas <[email protected]>; Alex Williamson <[email protected]>; Bjorn Helgaas <[email protected]>; David S. Miller <[email protected]>; Jakub Kicinski <[email protected]>; Kirti Wankhede <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Saeed Mahameed <[email protected]>; liulongfang <[email protected]>
Subject: Re: [PATCH mlx5-next 2/7] vfio: Add an API to check migration state transition validity
Quoted 99 lines · Shameerali Kolothum Thodi
Quoted 99 lines
-----Original Message-----
From: Max Gurtovoy [mailto:[email protected]]
Sent: 23 September 2021 14:56
To: Leon Romanovsky <[email protected]>; Shameerali Kolothum Thodi <[email protected]>
Cc: Doug Ledford <[email protected]>; Jason Gunthorpe <[email protected]>; Yishai Hadas <[email protected]>; Alex Williamson <[email protected]>; Bjorn Helgaas <[email protected]>; David S. Miller <[email protected]>; Jakub Kicinski <[email protected]>; Kirti Wankhede <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Saeed Mahameed <[email protected]>
Subject: Re: [PATCH mlx5-next 2/7] vfio: Add an API to check migration state transition validity
Quoted 1 line · Leon Romanovsky
On Thu, Sep 23, 2021 at 10:33:10AM +0000, Shameerali Kolothum Thodi
Quoted 4 lines · wrote:
Quoted 4 lines
Quoted 4 lines
-----Original Message-----
From: Leon Romanovsky [mailto:[email protected]]
Sent: 22 September 2021 11:39
To: Doug Ledford <[email protected]>; Jason Gunthorpe
<[email protected]>
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Cc: Yishai Hadas <[email protected]>; Alex Williamson <[email protected]>; Bjorn Helgaas <[email protected]>;
David
Quoted 19 lines
Quoted 19 lines
Quoted 19 lines
S. Miller <[email protected]>; Jakub Kicinski <[email protected]>; Kirti Wankhede <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Saeed Mahameed <[email protected]>
Subject: [PATCH mlx5-next 2/7] vfio: Add an API to check migration state transition validity
From: Yishai Hadas <[email protected]>
Add an API in the core layer to check migration state transition validity as part of a migration flow.
The valid transitions follow the expected usage as described in uapi/vfio.h and triggered by QEMU.
This ensures that all migration implementations follow a consistent migration state machine.
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Signed-off-by Yishai Hadas <[email protected]>
Reviewed-by Kirti Wankhede <[email protected]>
Signed-off-by Jason Gunthorpe <[email protected]>
Signed-off-by Leon Romanovsky <[email protected]>
---
   drivers/vfio/vfio.c  | 41
+++++++++++++++++++++++++++++++++++++++++
Quoted 8 lines
Quoted 8 lines
drivers/vfio/vfio.c static int vfio_device_fops_release(struct
Quoted 8 lines
   include/linux/vfio.h |  1 +
   2 files changed, 42 insertions(+)
drivers/vfio/vfio.c index renamed from drivers/vfio/vfio.c +0 -0
3c034fe14ccb..c3ca33e513c8 100644
drivers/vfio/vfio.c +0 -0
@@ -1664,6 +1664,47 @@ static int vfio_device_fops_release(struct
inode
Quoted 7 lines
Quoted 7 lines
drivers/vfio/vfio.c static int vfio_device_fops_release(struct inode *inode, struct file *filep)
Quoted 7 lines
drivers/vfio/vfio.c +3 -0
 *inode, struct file *filep)
   	return 0;
   }
 
+/**
+ * vfio_change_migration_state_allowed - Checks whether a
+migration
state
Quoted 6 lines
Quoted 6 lines
drivers/vfio/vfio.c static int vfio_device_fops_release(struct inode *inode, struct file *filep)
Quoted 6 lines
drivers/vfio/vfio.c +6 -0
+ *   transition is valid.
+ * @new_state: The new state to move to.
+ * @old_state: The old state.
+ * Return: true if the transition is valid.
+ */
+bool vfio_change_migration_state_allowed(u32 new_state, u32
old_state)
Quoted 3 lines
Quoted 3 lines
drivers/vfio/vfio.c static int vfio_device_fops_release(struct inode *inode, struct file *filep)
Quoted 3 lines
drivers/vfio/vfio.c +3 -0
+{
+	enum { MAX_STATE = VFIO_DEVICE_STATE_RESUMING };
+	static const u8 vfio_from_state_table[MAX_STATE + 1][MAX_STATE +
1] = {
Quoted 8 lines
Quoted 8 lines
drivers/vfio/vfio.c static int vfio_device_fops_release(struct inode *inode, struct file *filep)
Quoted 8 lines
drivers/vfio/vfio.c +8 -0
+		[VFIO_DEVICE_STATE_STOP] = {
+			[VFIO_DEVICE_STATE_RUNNING] = 1,
+			[VFIO_DEVICE_STATE_RESUMING] = 1,
+		},
+		[VFIO_DEVICE_STATE_RUNNING] = {
+			[VFIO_DEVICE_STATE_STOP] = 1,
+			[VFIO_DEVICE_STATE_SAVING] = 1,
+			[VFIO_DEVICE_STATE_SAVING |
VFIO_DEVICE_STATE_RUNNING]
Quoted 2 lines
Quoted 2 lines
drivers/vfio/vfio.c static int vfio_device_fops_release(struct · line 1688
Quoted 1 line
drivers/vfio/vfio.c +0 -0
 = 1,
Do we need to allow _RESUMING state here or not? As per the "State
transitions"
Quoted 4 lines
Quoted 1 line
section from uapi/linux/vfio.h,
It looks like we missed this state transition.
Thanks

Quoted 8 lines

Quoted 6 lines
Quoted 3 lines
I'm not sure this state transition is valid.
Kirti, When we would like to move from RUNNING to RESUMING ?
I guess it depends on what you report as your dev default state.
For HiSilicon ACC migration driver, we set the default to _RUNNING.
Where do you set it and report it ?

Currently, in _open_device() we set the device_state to _RUNNING.

I think in your case the default of vmig->vfio_dev_state == 0 (_STOP).

Quoted 77 lines

Quoted 77 lines
And when the migration starts, the destination side Qemu, set the device state to _RESUMING(vfio_load_state()).
From the documentation, it looks like the assumption on default state
of the VFIO dev is _RUNNING.
"
* 001b => Device running, which is the default state "
Quoted 1 line
Sameerali, can you please re-test and update if you see this transition ?
Yes. And if I change the default state to _STOP, then the transition is from _STOP --> _RESUMING.
But the documentation on State transitions doesn't have _STOP --> _RESUMING transition as valid.
Thanks,
Shameer
Quoted 56 lines
Quoted 3 lines
Quoted 3 lines
" * 4. To start the resuming phase, the device state should be transitioned
from
Quoted 4 lines
Quoted 4 lines
* the _RUNNING to the _RESUMING state."
IIRC, I have seen that transition happening on the destination dev while
testing the
Quoted 11 lines
Quoted 11 lines
HiSilicon ACC dev migration.
Thanks,
Shameer
drivers/vfio/vfio.c static int vfio_device_fops_release(struct inode *inode, struct file *filep)
Quoted 6 lines
drivers/vfio/vfio.c +6 -0
+		},
+		[VFIO_DEVICE_STATE_SAVING] = {
+			[VFIO_DEVICE_STATE_STOP] = 1,
+			[VFIO_DEVICE_STATE_RUNNING] = 1,
+		},
+		[VFIO_DEVICE_STATE_SAVING | VFIO_DEVICE_STATE_RUNNING]
= {
Quoted 27 lines
Quoted 27 lines
Quoted 27 lines
drivers/vfio/vfio.c +16 -0
+			[VFIO_DEVICE_STATE_RUNNING] = 1,
+			[VFIO_DEVICE_STATE_SAVING] = 1,
+		},
+		[VFIO_DEVICE_STATE_RESUMING] = {
+			[VFIO_DEVICE_STATE_RUNNING] = 1,
+			[VFIO_DEVICE_STATE_STOP] = 1,
+		},
+	};
+
+	if (new_state > MAX_STATE || old_state > MAX_STATE)
+		return false;
+
+	return vfio_from_state_table[old_state][new_state];
+}
+EXPORT_SYMBOL_GPL(vfio_change_migration_state_allowed);
+
   static long vfio_device_fops_unl_ioctl(struct file *filep,
   				       unsigned int cmd, unsigned long arg)
   {
include/linux/vfio.h index renamed from include/linux/vfio.h +0 -0
b53a9557884a..e65137a708f1 100644
include/linux/vfio.h +0 -0
@@ -83,6 +83,7 @@ extern struct vfio_device
 *vfio_device_get_from_dev(struct device *dev);
   extern void vfio_device_put(struct vfio_device *device);
 
Quoted 3 lines
Quoted 3 lines
include/linux/vfio.h extern struct vfio_device · line 88
Quoted 3 lines
include/linux/vfio.h +1 -0
   int vfio_assign_device_set(struct vfio_device *device, void
 *set_id);
+bool vfio_change_migration_state_allowed(u32 new_state, u32
old_state);
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
include/linux/vfio.h +0 -0
   /* events for the backend driver notify callback */
   enum vfio_iommu_notify_type {
--
2.31.1

Max Gurtovoy

2021-09-27 18:24 UTC #19 lore raw

Re: [PATCH mlx5-next 2/7] vfio: Add an API to check migration state transition validity

Quoted 117 lines · Shameerali Kolothum Thodi

Quoted 117 lines
-----Original Message-----
From: Max Gurtovoy [mailto:[email protected]]
Sent: 26 September 2021 10:10
To: Shameerali Kolothum Thodi <[email protected]>; Leon Romanovsky <[email protected]>
Cc: Doug Ledford <[email protected]>; Jason Gunthorpe <[email protected]>; Yishai Hadas <[email protected]>; Alex Williamson <[email protected]>; Bjorn Helgaas <[email protected]>; David S. Miller <[email protected]>; Jakub Kicinski <[email protected]>; Kirti Wankhede <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Saeed Mahameed <[email protected]>; liulongfang <[email protected]>
Subject: Re: [PATCH mlx5-next 2/7] vfio: Add an API to check migration state transition validity
Quoted 98 lines · Shameerali Kolothum Thodi
Quoted 98 lines
-----Original Message-----
From: Max Gurtovoy [mailto:[email protected]]
Sent: 23 September 2021 14:56
To: Leon Romanovsky <[email protected]>; Shameerali Kolothum Thodi <[email protected]>
Cc: Doug Ledford <[email protected]>; Jason Gunthorpe <[email protected]>; Yishai Hadas <[email protected]>; Alex Williamson <[email protected]>; Bjorn Helgaas <[email protected]>; David S. Miller <[email protected]>; Jakub Kicinski <[email protected]>; Kirti Wankhede <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Saeed Mahameed <[email protected]>
Subject: Re: [PATCH mlx5-next 2/7] vfio: Add an API to check migration state transition validity
Quoted 1 line · Leon Romanovsky
On Thu, Sep 23, 2021 at 10:33:10AM +0000, Shameerali Kolothum Thodi
Quoted 4 lines · wrote:
Quoted 4 lines
Quoted 4 lines
-----Original Message-----
From: Leon Romanovsky [mailto:[email protected]]
Sent: 22 September 2021 11:39
To: Doug Ledford <[email protected]>; Jason Gunthorpe
<[email protected]>
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Cc: Yishai Hadas <[email protected]>; Alex Williamson <[email protected]>; Bjorn Helgaas <[email protected]>;
David
Quoted 19 lines
Quoted 19 lines
Quoted 19 lines
S. Miller <[email protected]>; Jakub Kicinski <[email protected]>; Kirti Wankhede <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Saeed Mahameed <[email protected]>
Subject: [PATCH mlx5-next 2/7] vfio: Add an API to check migration state transition validity
From: Yishai Hadas <[email protected]>
Add an API in the core layer to check migration state transition validity as part of a migration flow.
The valid transitions follow the expected usage as described in uapi/vfio.h and triggered by QEMU.
This ensures that all migration implementations follow a consistent migration state machine.
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Signed-off-by Yishai Hadas <[email protected]>
Reviewed-by Kirti Wankhede <[email protected]>
Signed-off-by Jason Gunthorpe <[email protected]>
Signed-off-by Leon Romanovsky <[email protected]>
---
    drivers/vfio/vfio.c  | 41
+++++++++++++++++++++++++++++++++++++++++
Quoted 8 lines
Quoted 8 lines
drivers/vfio/vfio.c static int vfio_device_fops_release(struct
Quoted 8 lines
    include/linux/vfio.h |  1 +
    2 files changed, 42 insertions(+)
drivers/vfio/vfio.c index renamed from drivers/vfio/vfio.c +0 -0
3c034fe14ccb..c3ca33e513c8 100644
drivers/vfio/vfio.c +0 -0
@@ -1664,6 +1664,47 @@ static int vfio_device_fops_release(struct
inode
Quoted 7 lines
Quoted 7 lines
drivers/vfio/vfio.c static int vfio_device_fops_release(struct · line 1670
Quoted 7 lines
drivers/vfio/vfio.c +3 -0
 *inode, struct file *filep)
    	return 0;
    }
 
+/**
+ * vfio_change_migration_state_allowed - Checks whether a
+migration
state
Quoted 6 lines
Quoted 6 lines
drivers/vfio/vfio.c static int vfio_device_fops_release(struct · line 1676
Quoted 6 lines
drivers/vfio/vfio.c +6 -0
+ *   transition is valid.
+ * @new_state: The new state to move to.
+ * @old_state: The old state.
+ * Return: true if the transition is valid.
+ */
+bool vfio_change_migration_state_allowed(u32 new_state, u32
old_state)
Quoted 3 lines
Quoted 3 lines
drivers/vfio/vfio.c static int vfio_device_fops_release(struct · line 1679
Quoted 3 lines
drivers/vfio/vfio.c +3 -0
+{
+	enum { MAX_STATE = VFIO_DEVICE_STATE_RESUMING };
+	static const u8 vfio_from_state_table[MAX_STATE + 1][MAX_STATE +
1] = {
Quoted 8 lines
Quoted 8 lines
drivers/vfio/vfio.c static int vfio_device_fops_release(struct · line 1687
Quoted 8 lines
drivers/vfio/vfio.c +8 -0
+		[VFIO_DEVICE_STATE_STOP] = {
+			[VFIO_DEVICE_STATE_RUNNING] = 1,
+			[VFIO_DEVICE_STATE_RESUMING] = 1,
+		},
+		[VFIO_DEVICE_STATE_RUNNING] = {
+			[VFIO_DEVICE_STATE_STOP] = 1,
+			[VFIO_DEVICE_STATE_SAVING] = 1,
+			[VFIO_DEVICE_STATE_SAVING |
VFIO_DEVICE_STATE_RUNNING]
Quoted 2 lines
Quoted 2 lines
drivers/vfio/vfio.c static int vfio_device_fops_release(struct · line 1688
Quoted 1 line
drivers/vfio/vfio.c +0 -0
 = 1,
Do we need to allow _RESUMING state here or not? As per the "State
transitions"
Quoted 4 lines
Quoted 1 line
section from uapi/linux/vfio.h,
It looks like we missed this state transition.
Thanks

Quoted 8 lines

Quoted 7 lines
Quoted 6 lines
Quoted 3 lines
I'm not sure this state transition is valid.
Kirti, When we would like to move from RUNNING to RESUMING ?
I guess it depends on what you report as your dev default state.
For HiSilicon ACC migration driver, we set the default to _RUNNING.
Where do you set it and report it ?
Currently, in _open_device() we set the device_state to _RUNNING.

Why do you do it ?

Quoted 77 lines

I think in your case the default of vmig->vfio_dev_state == 0 (_STOP).
Quoted 74 lines
Quoted 74 lines
And when the migration starts, the destination side Qemu, set the device state to _RESUMING(vfio_load_state()).
From the documentation, it looks like the assumption on default state
of the VFIO dev is _RUNNING.
"
* 001b => Device running, which is the default state "
Quoted 1 line
Sameerali, can you please re-test and update if you see this transition ?
Yes. And if I change the default state to _STOP, then the transition is from _STOP --> _RESUMING.
But the documentation on State transitions doesn't have _STOP --> _RESUMING transition as valid.
Thanks,
Shameer
Quoted 55 lines
Quoted 2 lines
Quoted 2 lines
" * 4. To start the resuming phase, the device state should be transitioned
from
Quoted 4 lines
Quoted 4 lines
* the _RUNNING to the _RESUMING state."
IIRC, I have seen that transition happening on the destination dev while
testing the
Quoted 11 lines
Quoted 11 lines
HiSilicon ACC dev migration.
Thanks,
Shameer
drivers/vfio/vfio.c static int vfio_device_fops_release(struct · line 1694
Quoted 6 lines
drivers/vfio/vfio.c +6 -0
+		},
+		[VFIO_DEVICE_STATE_SAVING] = {
+			[VFIO_DEVICE_STATE_STOP] = 1,
+			[VFIO_DEVICE_STATE_RUNNING] = 1,
+		},
+		[VFIO_DEVICE_STATE_SAVING | VFIO_DEVICE_STATE_RUNNING]
= {
Quoted 27 lines
Quoted 27 lines
Quoted 27 lines
drivers/vfio/vfio.c +16 -0
+			[VFIO_DEVICE_STATE_RUNNING] = 1,
+			[VFIO_DEVICE_STATE_SAVING] = 1,
+		},
+		[VFIO_DEVICE_STATE_RESUMING] = {
+			[VFIO_DEVICE_STATE_RUNNING] = 1,
+			[VFIO_DEVICE_STATE_STOP] = 1,
+		},
+	};
+
+	if (new_state > MAX_STATE || old_state > MAX_STATE)
+		return false;
+
+	return vfio_from_state_table[old_state][new_state];
+}
+EXPORT_SYMBOL_GPL(vfio_change_migration_state_allowed);
+
    static long vfio_device_fops_unl_ioctl(struct file *filep,
    				       unsigned int cmd, unsigned long arg)
    {
include/linux/vfio.h index renamed from include/linux/vfio.h +0 -0
b53a9557884a..e65137a708f1 100644
include/linux/vfio.h +0 -0
@@ -83,6 +83,7 @@ extern struct vfio_device
 *vfio_device_get_from_dev(struct device *dev);
    extern void vfio_device_put(struct vfio_device *device);
 
Quoted 3 lines
Quoted 3 lines
include/linux/vfio.h extern struct vfio_device · line 88
Quoted 3 lines
include/linux/vfio.h +1 -0
    int vfio_assign_device_set(struct vfio_device *device, void
 *set_id);
+bool vfio_change_migration_state_allowed(u32 new_state, u32
old_state);
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
include/linux/vfio.h +0 -0
    /* events for the backend driver notify callback */
    enum vfio_iommu_notify_type {
--
2.31.1

Shameerali Kolothum Thodi

2021-09-27 18:29 UTC #20 lore raw

RE: [PATCH mlx5-next 2/7] vfio: Add an API to check migration state transition validity

Quoted 148 lines

-----Original Message-----
From: Max Gurtovoy [mailto:[email protected]]
Sent: 27 September 2021 19:24
To: Shameerali Kolothum Thodi <[email protected]>; Leon Romanovsky <[email protected]>
Cc: Doug Ledford <[email protected]>; Jason Gunthorpe <[email protected]>; Yishai Hadas <[email protected]>; Alex Williamson <[email protected]>; Bjorn Helgaas <[email protected]>; David S. Miller <[email protected]>; Jakub Kicinski <[email protected]>; Kirti Wankhede <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Saeed Mahameed <[email protected]>; liulongfang <[email protected]>
Subject: Re: [PATCH mlx5-next 2/7] vfio: Add an API to check migration state transition validity
Quoted 5 lines · Shameerali Kolothum Thodi
Quoted 5 lines
-----Original Message-----
From: Max Gurtovoy [mailto:[email protected]]
Sent: 26 September 2021 10:10
To: Shameerali Kolothum Thodi
<[email protected]>;
Quoted 15 lines
Quoted 15 lines
Leon Romanovsky <[email protected]>
Cc: Doug Ledford <[email protected]>; Jason Gunthorpe <[email protected]>; Yishai Hadas <[email protected]>; Alex Williamson <[email protected]>; Bjorn Helgaas <[email protected]>; David S. Miller <[email protected]>; Jakub Kicinski <[email protected]>; Kirti Wankhede <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Saeed Mahameed <[email protected]>; liulongfang <[email protected]>
Subject: Re: [PATCH mlx5-next 2/7] vfio: Add an API to check migration state transition validity
On 9/24/2021 10:44 AM, Shameerali Kolothum Thodi wrote:
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
-----Original Message-----
From: Max Gurtovoy [mailto:[email protected]]
Sent: 23 September 2021 14:56
To: Leon Romanovsky <[email protected]>; Shameerali Kolothum Thodi <[email protected]>
Cc: Doug Ledford <[email protected]>; Jason Gunthorpe <[email protected]>; Yishai Hadas <[email protected]>; Alex Williamson <[email protected]>; Bjorn Helgaas <[email protected]>; David S. Miller <[email protected]>;
Jakub
Quoted 71 lines
Quoted 71 lines
Quoted 71 lines
Quoted 71 lines
Kicinski <[email protected]>; Kirti Wankhede <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Saeed Mahameed <[email protected]>
Subject: Re: [PATCH mlx5-next 2/7] vfio: Add an API to check migration state transition validity
Quoted 2 lines · Leon Romanovsky
On Thu, Sep 23, 2021 at 10:33:10AM +0000, Shameerali Kolothum Thodi
Quoted 4 lines · wrote:
Quoted 4 lines
Quoted 4 lines
-----Original Message-----
From: Leon Romanovsky [mailto:[email protected]]
Sent: 22 September 2021 11:39
To: Doug Ledford <[email protected]>; Jason Gunthorpe
<[email protected]>
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Cc: Yishai Hadas <[email protected]>; Alex Williamson <[email protected]>; Bjorn Helgaas <[email protected]>;
David
Quoted 19 lines
Quoted 19 lines
Quoted 19 lines
S. Miller <[email protected]>; Jakub Kicinski <[email protected]>; Kirti Wankhede <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Saeed Mahameed <[email protected]>
Subject: [PATCH mlx5-next 2/7] vfio: Add an API to check migration state transition validity
From: Yishai Hadas <[email protected]>
Add an API in the core layer to check migration state transition validity as part of a migration flow.
The valid transitions follow the expected usage as described in uapi/vfio.h and triggered by QEMU.
This ensures that all migration implementations follow a consistent migration state machine.
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Signed-off-by Yishai Hadas <[email protected]>
Reviewed-by Kirti Wankhede <[email protected]>
Signed-off-by Jason Gunthorpe <[email protected]>
Signed-off-by Leon Romanovsky <[email protected]>
---
    drivers/vfio/vfio.c  | 41
+++++++++++++++++++++++++++++++++++++++++
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
    include/linux/vfio.h |  1 +
    2 files changed, 42 insertions(+)
drivers/vfio/vfio.c index renamed from drivers/vfio/vfio.c +0 -0
3c034fe14ccb..c3ca33e513c8 100644
drivers/vfio/vfio.c +0 -0
@@ -1664,6 +1664,47 @@ static int
vfio_device_fops_release(struct
inode
Quoted 7 lines
Quoted 7 lines
drivers/vfio/vfio.c
Quoted 7 lines
*inode, struct file *filep)
    	return 0;
    }
drivers/vfio/vfio.c +3 -0
+/**
+ * vfio_change_migration_state_allowed - Checks whether a
+migration
state
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
+ * transition is valid.
+ * @new_state: The new state to move to.
+ * @old_state: The old state.
+ * Return: true if the transition is valid.
+ */
+bool vfio_change_migration_state_allowed(u32 new_state, u32
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
old_state)
Quoted 3 lines
Quoted 3 lines
drivers/vfio/vfio.c
Quoted 3 lines
drivers/vfio/vfio.c +3 -0
+{
+	enum { MAX_STATE = VFIO_DEVICE_STATE_RESUMING };
+	static const u8 vfio_from_state_table[MAX_STATE +
1][MAX_STATE
Quoted 21 lines
Quoted 21 lines
Quoted 21 lines
Quoted 21 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
++
1] = {
Quoted 8 lines
Quoted 8 lines
drivers/vfio/vfio.c
Quoted 8 lines
drivers/vfio/vfio.c +8 -0
+		[VFIO_DEVICE_STATE_STOP] = {
+			[VFIO_DEVICE_STATE_RUNNING] = 1,
+			[VFIO_DEVICE_STATE_RESUMING] = 1,
+		},
+		[VFIO_DEVICE_STATE_RUNNING] = {
+			[VFIO_DEVICE_STATE_STOP] = 1,
+			[VFIO_DEVICE_STATE_SAVING] = 1,
+			[VFIO_DEVICE_STATE_SAVING |
VFIO_DEVICE_STATE_RUNNING]
Quoted 3 lines
Quoted 3 lines
Quoted 1 line
= 1,
Do we need to allow _RESUMING state here or not? As per the "State
transitions"
Quoted 4 lines
Quoted 1 line
section from uapi/linux/vfio.h,
It looks like we missed this state transition.
Thanks
I'm not sure this state transition is valid.

Quoted 8 lines

Quoted 6 lines
Quoted 5 lines
Quoted 4 lines
Quoted 1 line
Kirti, When we would like to move from RUNNING to RESUMING ?
I guess it depends on what you report as your dev default state.
For HiSilicon ACC migration driver, we set the default to _RUNNING.
Where do you set it and report it ?
Currently, in _open_device() we set the device_state to _RUNNING.
Why do you do it ?

It is by the assumption that the default state to be _RUNNING and then we take it from there for migration state changes.

Any particular reason why it needs to be in _STOP state? We need to update the documentation in that case to allow _STOP --> _RESUMING.

Quoted 79 lines

Quoted 36 lines
I think in your case the default of vmig->vfio_dev_state == 0 (_STOP).
Quoted 32 lines
Quoted 32 lines
And when the migration starts, the destination side Qemu, set the device state to _RESUMING(vfio_load_state()).
From the documentation, it looks like the assumption on default
state of the VFIO dev is _RUNNING.
"
* 001b => Device running, which is the default state "
Quoted 1 line
Sameerali, can you please re-test and update if you see this transition ?
Yes. And if I change the default state to _STOP, then the transition is from _STOP --> _RESUMING.
But the documentation on State transitions doesn't have _STOP --> _RESUMING transition as valid.
Thanks,
Shameer
Quoted 13 lines
Quoted 2 lines
Quoted 2 lines
" * 4. To start the resuming phase, the device state should be transitioned
from
Quoted 4 lines
Quoted 4 lines
* the _RUNNING to the _RESUMING state."
IIRC, I have seen that transition happening on the destination dev while
testing the
Quoted 5 lines
Quoted 5 lines
HiSilicon ACC dev migration.
Thanks,
Shameer
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
drivers/vfio/vfio.c
Quoted 6 lines
drivers/vfio/vfio.c +6 -0
+		},
+		[VFIO_DEVICE_STATE_SAVING] = {
+			[VFIO_DEVICE_STATE_STOP] = 1,
+			[VFIO_DEVICE_STATE_RUNNING] = 1,
+		},
+		[VFIO_DEVICE_STATE_SAVING |
VFIO_DEVICE_STATE_RUNNING]
Quoted 36 lines
Quoted 36 lines
Quoted 36 lines
Quoted 36 lines
= {
Quoted 27 lines
Quoted 27 lines
Quoted 27 lines
drivers/vfio/vfio.c +16 -0
+			[VFIO_DEVICE_STATE_RUNNING] = 1,
+			[VFIO_DEVICE_STATE_SAVING] = 1,
+		},
+		[VFIO_DEVICE_STATE_RESUMING] = {
+			[VFIO_DEVICE_STATE_RUNNING] = 1,
+			[VFIO_DEVICE_STATE_STOP] = 1,
+		},
+	};
+
+	if (new_state > MAX_STATE || old_state > MAX_STATE)
+		return false;
+
+	return vfio_from_state_table[old_state][new_state];
+}
+EXPORT_SYMBOL_GPL(vfio_change_migration_state_allowed);
+
    static long vfio_device_fops_unl_ioctl(struct file *filep,
    				       unsigned int cmd, unsigned long arg)
    {
include/linux/vfio.h index renamed from include/linux/vfio.h +0 -0
b53a9557884a..e65137a708f1 100644
include/linux/vfio.h +0 -0
@@ -83,6 +83,7 @@ extern struct vfio_device
 *vfio_device_get_from_dev(struct device *dev);
    extern void vfio_device_put(struct vfio_device *device);
 
Quoted 3 lines
Quoted 3 lines
include/linux/vfio.h extern struct vfio_device · line 88
Quoted 3 lines
include/linux/vfio.h +1 -0
    int vfio_assign_device_set(struct vfio_device *device, void
 *set_id);
+bool vfio_change_migration_state_allowed(u32 new_state, u32
old_state);
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
include/linux/vfio.h +0 -0
    /* events for the backend driver notify callback */
    enum vfio_iommu_notify_type {
--
2.31.1

Alex Williamson

2021-09-27 22:46 UTC #21 lore raw

Re: [PATCH mlx5-next 2/7] vfio: Add an API to check migration state transition validity

In reply to Leon Romanovsky

Quoted 33 lines · On Wed, 22 Sep 2021 13:38:51 +0300 Leon Romanovsky <[email protected]> wrote:

From: Yishai Hadas <[email protected]>
Add an API in the core layer to check migration state transition validity as part of a migration flow.
The valid transitions follow the expected usage as described in uapi/vfio.h and triggered by QEMU.
This ensures that all migration implementations follow a consistent migration state machine.
Signed-off-by Yishai Hadas <[email protected]>
Reviewed-by Kirti Wankhede <[email protected]>
Signed-off-by Jason Gunthorpe <[email protected]>
Signed-off-by Leon Romanovsky <[email protected]>
---
 drivers/vfio/vfio.c  | 41 +++++++++++++++++++++++++++++++++++++++++
 include/linux/vfio.h |  1 +
 2 files changed, 42 insertions(+)
drivers/vfio/vfio.c +5 -0
index 3c034fe14ccb..c3ca33e513c8 100644
@@ -1664,6 +1664,47 @@ static int vfio_device_fops_release(struct inode *inode, struct file *filep)
 	return 0;
 }
 
+/**
+ * vfio_change_migration_state_allowed - Checks whether a migration state
+ *   transition is valid.
+ * @new_state: The new state to move to.
+ * @old_state: The old state.

drivers/vfio/vfio.c static int vfio_device_fops_release(struct inode *inode, struct file *filep) · line 1681

Quoted 10 lines

drivers/vfio/vfio.c +10 -0

+ * Return: true if the transition is valid.
+ */
+bool vfio_change_migration_state_allowed(u32 new_state, u32 old_state)
+{
+	enum { MAX_STATE = VFIO_DEVICE_STATE_RESUMING };
+	static const u8 vfio_from_state_table[MAX_STATE + 1][MAX_STATE + 1] = {
+		[VFIO_DEVICE_STATE_STOP] = {
+			[VFIO_DEVICE_STATE_RUNNING] = 1,
+			[VFIO_DEVICE_STATE_RESUMING] = 1,
+		},

Our state transition diagram is pretty weak on reachable transitions out of the _STOP state, why do we select only these two as valid?

Consistent behavior to userspace is of course nice, but I wonder if we were expecting a device reset to get us back to _RUNNING, or if the drivers would make use of the protocol through which a driver can nak (write error, no state change) or fault (_ERROR device state) a state change.

There does need to be a way to get back to _RUNNING to support a migration failure without a reset, but would that be from _SAVING or from _STOP and what's our rationale for the excluded states?

I'll see if I can dig through emails to find what was intended to be reachable from _STOP. Kirti or Connie, do you recall?

Also, I think the _ERROR state is implicitly handled correctly here, its value is >MAX_STATE so we can't transition into or out of it, but a comment to indicate that it's been considered for this would be nice.

drivers/vfio/vfio.c static int vfio_device_fops_release(struct inode *inode, struct file *filep) · line 1686

Quoted 5 lines

drivers/vfio/vfio.c +5 -0

+		[VFIO_DEVICE_STATE_RUNNING] = {
+			[VFIO_DEVICE_STATE_STOP] = 1,
+			[VFIO_DEVICE_STATE_SAVING] = 1,
+			[VFIO_DEVICE_STATE_SAVING | VFIO_DEVICE_STATE_RUNNING] = 1,
+		},

Shameer's comment is correct here, _RESUMING is a valid next state since the default state is _RUNNING.

drivers/vfio/vfio.c static int vfio_device_fops_release(struct inode *inode, struct file *filep) · line 1690

Quoted 4 lines

drivers/vfio/vfio.c +4 -0

+		[VFIO_DEVICE_STATE_SAVING] = {
+			[VFIO_DEVICE_STATE_STOP] = 1,
+			[VFIO_DEVICE_STATE_RUNNING] = 1,
+		},

What's the rationale that we can't return to _SAVING|_RUNNING here?

drivers/vfio/vfio.c static int vfio_device_fops_release(struct inode *inode, struct file *filep) · line 1694

Quoted 4 lines

drivers/vfio/vfio.c +4 -0

+		[VFIO_DEVICE_STATE_SAVING | VFIO_DEVICE_STATE_RUNNING] = {
+			[VFIO_DEVICE_STATE_RUNNING] = 1,
+			[VFIO_DEVICE_STATE_SAVING] = 1,
+		},

Can't we always _STOP the device at any point?

drivers/vfio/vfio.c static int vfio_device_fops_release(struct inode *inode, struct file *filep) · line 1698

Quoted 4 lines

drivers/vfio/vfio.c +4 -0

+		[VFIO_DEVICE_STATE_RESUMING] = {
+			[VFIO_DEVICE_STATE_RUNNING] = 1,
+			[VFIO_DEVICE_STATE_STOP] = 1,
+		},

Couldn't it be possible to switch immediately to _RUNNING|_SAVING for tracing purposes? Or _SAVING, perhaps to validate the restored state without starting the device? Thanks,

Alex

Quoted 24 lines

drivers/vfio/vfio.c +9 -0

+	};
+
+	if (new_state > MAX_STATE || old_state > MAX_STATE)
+		return false;
+
+	return vfio_from_state_table[old_state][new_state];
+}
+EXPORT_SYMBOL_GPL(vfio_change_migration_state_allowed);
+
 static long vfio_device_fops_unl_ioctl(struct file *filep,
 				       unsigned int cmd, unsigned long arg)
 {

include/linux/vfio.h +1 -0

index b53a9557884a..e65137a708f1 100644
@@ -83,6 +83,7 @@ extern struct vfio_device *vfio_device_get_from_dev(struct device *dev);
 extern void vfio_device_put(struct vfio_device *device);
 
 int vfio_assign_device_set(struct vfio_device *device, void *set_id);
+bool vfio_change_migration_state_allowed(u32 new_state, u32 old_state);
 
 /* events for the backend driver notify callback */
 enum vfio_iommu_notify_type {

Jason Gunthorpe

2021-09-27 23:12 UTC #22 lore raw

Re: [PATCH mlx5-next 2/7] vfio: Add an API to check migration state transition validity

Quoted 9 lines · Alex Williamson

drivers/vfio/vfio.c static int vfio_device_fops_release(struct inode *inode, struct file *filep) · line 1681
Quoted 6 lines
drivers/vfio/vfio.c +6 -0
+	enum { MAX_STATE = VFIO_DEVICE_STATE_RESUMING };
+	static const u8 vfio_from_state_table[MAX_STATE + 1][MAX_STATE + 1] = {
+		[VFIO_DEVICE_STATE_STOP] = {
+			[VFIO_DEVICE_STATE_RUNNING] = 1,
+			[VFIO_DEVICE_STATE_RESUMING] = 1,
+		},
Our state transition diagram is pretty weak on reachable transitions out of the _STOP state, why do we select only these two as valid?

I have no particular opinion on specific states here, however adding more states means more stuff for drivers to implement and more risk driver writers will mess up this uAPI.

So only on those grounds I'd suggest to keep this to the minimum needed instead of the maximum logically possible..

Also, probably the FSM comment from the uapi header file should be moved into a function comment above this function?

Jason

Alex Williamson

2021-09-28 19:20 UTC #23 lore raw

Re: [PATCH mlx5-next 2/7] vfio: Add an API to check migration state transition validity

Quoted 11 lines · On Mon, 27 Sep 2021 20:12:39 -0300 Jason Gunthorpe <[email protected]> wrote:

Quoted 9 lines · Alex Williamson
drivers/vfio/vfio.c static int vfio_device_fops_release(struct inode *inode, struct file *filep) · line 1681
Quoted 6 lines
drivers/vfio/vfio.c +6 -0
+	enum { MAX_STATE = VFIO_DEVICE_STATE_RESUMING };
+	static const u8 vfio_from_state_table[MAX_STATE + 1][MAX_STATE + 1] = {
+		[VFIO_DEVICE_STATE_STOP] = {
+			[VFIO_DEVICE_STATE_RUNNING] = 1,
+			[VFIO_DEVICE_STATE_RESUMING] = 1,
+		},  
Our state transition diagram is pretty weak on reachable transitions out of the _STOP state, why do we select only these two as valid?

Quoted 3 lines

I have no particular opinion on specific states here, however adding more states means more stuff for drivers to implement and more risk driver writers will mess up this uAPI.

It looks like state transitions were largely discussed in v9 and v10 of the migration proposals:

https://lore.kernel.org/all/[email protected]/
https://lore.kernel.org/all/[email protected]/

I'm not seeing that we really excluded many transitions there.

Quoted 5 lines

So only on those grounds I'd suggest to keep this to the minimum needed instead of the maximum logically possible..
Also, probably the FSM comment from the uapi header file should be moved into a function comment above this function?

It's not clear this function shouldn't be anything more than:

	if (new_state > MAX_STATE || old_state > MAX_STATE)
		return false;	/* exited via device reset, */
				/* entered via transition fault */

	return true;

That's still only 5 fully interconnected states to work between, and potentially a 6th if we decide _RESUMING|_RUNNING is valid for a device supporting post-copy.

In defining the device state, we tried to steer away from defining it in terms of the QEMU migration API, but rather as a set of controls that could be used to support that API to leave us some degree of independence that QEMU implementation might evolve.

To that extent, it actually seems easier for a device implementation to focus on bit definition rather than the state machine node.

I'd also vote that any clarification of state validity and transitions belongs in the uAPI header and a transition test function should reference that header as the source of truth, rather than the other way around. Thanks,

Alex

Jason Gunthorpe

2021-09-28 19:35 UTC #24 lore raw

Re: [PATCH mlx5-next 2/7] vfio: Add an API to check migration state transition validity

Quoted 4 lines · Alex Williamson

In defining the device state, we tried to steer away from defining it in terms of the QEMU migration API, but rather as a set of controls that could be used to support that API to leave us some degree of independence that QEMU implementation might evolve.

That is certainly a different perspective, it would have been better to not express this idea as a FSM in that case...

So each state in mlx5vf_pci_set_device_state() should call the correct combination of (un)freeze, (un)quiesce and so on so each state reflects a defined operation of the device?

Jason

Alex Williamson

2021-09-28 20:18 UTC #25 lore raw

Re: [PATCH mlx5-next 2/7] vfio: Add an API to check migration state transition validity

Quoted 7 lines · On Tue, 28 Sep 2021 16:35:50 -0300 Jason Gunthorpe <[email protected]> wrote:

Quoted 4 lines · Alex Williamson
In defining the device state, we tried to steer away from defining it in terms of the QEMU migration API, but rather as a set of controls that could be used to support that API to leave us some degree of independence that QEMU implementation might evolve.

Quoted 6 lines

That is certainly a different perspective, it would have been better to not express this idea as a FSM in that case...
So each state in mlx5vf_pci_set_device_state() should call the correct combination of (un)freeze, (un)quiesce and so on so each state reflects a defined operation of the device?

I'd expect so, for instance the implementation of entering the _STOP state presumes a previous state that where the device is apparently already quiesced. That doesn't support a direct _RUNNING -> _STOP transition where I argued in the linked threads that those states should be reachable from any other state. Thanks,

Alex

Jason Gunthorpe

maintainer

2021-09-29 16:16 UTC #26 lore raw

Re: [PATCH mlx5-next 2/7] vfio: Add an API to check migration state transition validity

Quoted 17 lines · Alex Williamson

Quoted 7 lines · On Tue, 28 Sep 2021 16:35:50 -0300 Jason Gunthorpe <[email protected]> wrote:
Quoted 4 lines · Alex Williamson
In defining the device state, we tried to steer away from defining it in terms of the QEMU migration API, but rather as a set of controls that could be used to support that API to leave us some degree of independence that QEMU implementation might evolve.
Quoted 6 lines
That is certainly a different perspective, it would have been better to not express this idea as a FSM in that case...
So each state in mlx5vf_pci_set_device_state() should call the correct combination of (un)freeze, (un)quiesce and so on so each state reflects a defined operation of the device?

Quoted 5 lines

I'd expect so, for instance the implementation of entering the _STOP state presumes a previous state that where the device is apparently already quiesced. That doesn't support a direct _RUNNING -> _STOP transition where I argued in the linked threads that those states should be reachable from any other state. Thanks,

If we focus on mlx5 there are two device 'flags' to manage:
- Device cannot issue DMAs
- Device internal state cannot change (ie cannot receive DMAs)

This is necessary to co-ordinate across multiple devices that might be doing peer to peer DMA between them. The whole multi-device complex should be moved to "cannot issue DMA's" then the whole complex would go to "state cannot change" and be serialized.

The expected sequence at the device is thus

Resuming
full stop -> does not issue DMAs -> full operation
Suspend
full operation -> does not issue DMAs -> full stop

Further the device has two actions
- Trigger serializating the device state
- Trigger de-serializing the device state

So, what is the behavior upon each state:

* 000b => Device Stopped, not saving or resuming Does not issue DMAs
Internal state cannot change

* 001b => Device running, which is the default state Neither flags

* 010b => Stop the device & save the device state, stop-and-copy state Does not issue DMAs
Internal state cannot change

* 011b => Device running and save the device state, pre-copy state Neither flags
(future, DMA tracking turned on)

* 100b => Device stopped and the device state is resuming Does not issue DMAs
Internal state cannot change

* 110b => Error state

???

* 101b => Invalid state
* 111b => Invalid state

???

What should the ??'s be? It looks like mlx5 doesn't use these, so it should just refuse to enter these states in the first place..

The two actions:
trigger serializing the device state
Done when asked to go to 010b ?

trigger de-serializing the device state
Done when transition from 100b -> 000b ?

There is a missing state "Stop Active Transactions" which would be only "does not issue DMAs". I've seen a proposal to add that.

I'm happy enough with this and it seems clean and easy enough to implement.

Jason

Alex Williamson

2021-09-29 18:07 UTC #27 lore raw

Re: [PATCH mlx5-next 2/7] vfio: Add an API to check migration state transition validity

Quoted 24 lines · On Wed, 29 Sep 2021 13:16:02 -0300 Jason Gunthorpe <[email protected]> wrote:

Quoted 17 lines · Alex Williamson
Quoted 7 lines · On Tue, 28 Sep 2021 16:35:50 -0300 Jason Gunthorpe <[email protected]> wrote:
Quoted 4 lines · Alex Williamson
In defining the device state, we tried to steer away from defining it in terms of the QEMU migration API, but rather as a set of controls that could be used to support that API to leave us some degree of independence that QEMU implementation might evolve.
Quoted 6 lines
That is certainly a different perspective, it would have been better to not express this idea as a FSM in that case...
So each state in mlx5vf_pci_set_device_state() should call the correct combination of (un)freeze, (un)quiesce and so on so each state reflects a defined operation of the device?
Quoted 5 lines
I'd expect so, for instance the implementation of entering the _STOP state presumes a previous state that where the device is apparently already quiesced. That doesn't support a direct _RUNNING -> _STOP transition where I argued in the linked threads that those states should be reachable from any other state. Thanks,

Quoted 8 lines

If we focus on mlx5 there are two device 'flags' to manage:
- Device cannot issue DMAs
- Device internal state cannot change (ie cannot receive DMAs)
This is necessary to co-ordinate across multiple devices that might be doing peer to peer DMA between them. The whole multi-device complex should be moved to "cannot issue DMA's" then the whole complex would go to "state cannot change" and be serialized.

Are you anticipating p2p from outside the VM? The typical scenario here would be that p2p occurs only intra-VM, so all the devices would stop issuing DMA (modulo trying to quiesce devices simultaneously).

Quoted 24 lines

The expected sequence at the device is thus
Resuming
full stop -> does not issue DMAs -> full operation
Suspend
full operation -> does not issue DMAs -> full stop
Further the device has two actions
- Trigger serializating the device state
- Trigger de-serializing the device state
So, what is the behavior upon each state:
* 000b => Device Stopped, not saving or resuming Does not issue DMAs
Internal state cannot change
* 001b => Device running, which is the default state Neither flags
* 010b => Stop the device & save the device state, stop-and-copy state Does not issue DMAs
Internal state cannot change

Quoted 7 lines

* 011b => Device running and save the device state, pre-copy state Neither flags
(future, DMA tracking turned on)
* 100b => Device stopped and the device state is resuming Does not issue DMAs
Internal state cannot change

cannot change... other that as loaded via migration region.

Quoted 11 lines

* 110b => Error state
    ???
* 101b => Invalid state
* 111b => Invalid state
    ???
What should the ??'s be? It looks like mlx5 doesn't use these, so it should just refuse to enter these states in the first place..

_SAVING and _RESUMING are considered mutually exclusive, therefore any combination of both of them is invalid. We've chosen to use the combination of 110b as an error state to indicate the device state is undefined, but not _RUNNING. This state is only reachable by an internal error of the driver during a state transition.

The expected protocol is that if the user write to the device_state register returns an errno, the user reevaluates the device_state to determine if the desired transition is unavailable (previous state value is returned) or generated a fault (error state value returned). Due to the undefined state of the device, the only exit from the error state is to re-initialize the device state via a reset. Therefore a successful device reset should always return the device to the 001b state.

The 111b state is also considered unreachable through normal means due to the _SAVING | _RESUMING conflict, but suggests the device is also _RUNNING in this undefined state. This combination has no currently defined use case and should not be reachable.

The 101b state indicates _RUNNING while _RESUMING, which is simply not a mode that has been spec'd at this time as it would require some mechanism for the device to fault in state on demand.

Quoted 3 lines

The two actions:
trigger serializing the device state
Done when asked to go to 010b ?

When the _SAVING bit is set. The exact mechanics depends on the size and volatility of the device state. A GPU might begin in pre-copy (011b) to transmit chunks of framebuffer data, recording hashes of blocks read by the user to avoid re-sending them during the stop-and-copy (010b) phase. A device with a small internal state representation may choose to forgo providing data in the pre-copy phase and entirely serialize internal state at stop-and-copy.

Quoted 2 lines

trigger de-serializing the device state
Done when transition from 100b -> 000b ?

100b -> 000b is not a required transition, generally this would be 100b -> 001b, ie. end state of _RUNNING vs _STOP.

I think the requirement is that de-serialization is complete when the _RESUMING bit is cleared. Whether the driver chooses to de-serialize piece-wise as each block of data is written to the device or in bulk from a buffer is left to the implementation. In either case, the driver can fail the transition to !_RESUMING if the state is incomplete or otherwise corrupt. It would again be the driver's discretion if the device enters the error state or remains in _RESUMING. If the user has no valid state with which to exit the _RESUMING phase, a device reset should return the device to _RUNNING with a default initial state.

Quoted 2 lines

There is a missing state "Stop Active Transactions" which would be only "does not issue DMAs". I've seen a proposal to add that.

This would be to get all devices to stop issuing DMA while internal state can be modified to avoid the synchronization issue of trying to stop devices concurrently? For PCI devices we obviously have the bus master bit to manage that, but I could see how a migration extension for such support (perhaps even just wired through to BM for PCI) could be useful. Thanks,

Alex

Jason Gunthorpe

maintainer

2021-09-29 18:26 UTC #28 lore raw

Re: [PATCH mlx5-next 2/7] vfio: Add an API to check migration state transition validity

Quoted 31 lines · Alex Williamson

Quoted 28 lines · On Wed, 29 Sep 2021 13:16:02 -0300 Jason Gunthorpe <[email protected]> wrote:
Quoted 17 lines · Alex Williamson
Quoted 7 lines · On Tue, 28 Sep 2021 16:35:50 -0300 Jason Gunthorpe <[email protected]> wrote:
Quoted 4 lines · Alex Williamson
In defining the device state, we tried to steer away from defining it in terms of the QEMU migration API, but rather as a set of controls that could be used to support that API to leave us some degree of independence that QEMU implementation might evolve.
Quoted 6 lines
That is certainly a different perspective, it would have been better to not express this idea as a FSM in that case...
So each state in mlx5vf_pci_set_device_state() should call the correct combination of (un)freeze, (un)quiesce and so on so each state reflects a defined operation of the device?
Quoted 5 lines
I'd expect so, for instance the implementation of entering the _STOP state presumes a previous state that where the device is apparently already quiesced. That doesn't support a direct _RUNNING -> _STOP transition where I argued in the linked threads that those states should be reachable from any other state. Thanks,
If we focus on mlx5 there are two device 'flags' to manage:
- Device cannot issue DMAs
- Device internal state cannot change (ie cannot receive DMAs)

Quoted 8 lines

Quoted 4 lines
This is necessary to co-ordinate across multiple devices that might be doing peer to peer DMA between them. The whole multi-device complex should be moved to "cannot issue DMA's" then the whole complex would go to "state cannot change" and be serialized.
Are you anticipating p2p from outside the VM? The typical scenario here would be that p2p occurs only intra-VM, so all the devices would stop issuing DMA (modulo trying to quiesce devices simultaneously).

Inside the VM.

Your 'modulo trying to quiesce devices simultaneously' is correct - this is a real issue that needs to be solved.

If we put one device in a state where it's internal state is immutable it can no longer accept DMA messages from the other devices. So there are two states in the HW model - do not generate DMAs and finally the immutable internal state where even external DMAs are refused.

Quoted 28 lines

Quoted 28 lines
The expected sequence at the device is thus
Resuming
full stop -> does not issue DMAs -> full operation
Suspend
full operation -> does not issue DMAs -> full stop
Further the device has two actions
- Trigger serializating the device state
- Trigger de-serializing the device state
So, what is the behavior upon each state:
* 000b => Device Stopped, not saving or resuming Does not issue DMAs
Internal state cannot change
* 001b => Device running, which is the default state Neither flags
* 010b => Stop the device & save the device state, stop-and-copy state Does not issue DMAs
Internal state cannot change
* 011b => Device running and save the device state, pre-copy state Neither flags
(future, DMA tracking turned on)

Quoted 5 lines

Quoted 3 lines
* 100b => Device stopped and the device state is resuming Does not issue DMAs
Internal state cannot change
cannot change... other that as loaded via migration region.

Yes

Quoted 5 lines

The expected protocol is that if the user write to the device_state register returns an errno, the user reevaluates the device_state to determine if the desired transition is unavailable (previous state value is returned) or generated a fault (error state value returned).

Hmm, interesting, mlx5 should be doing this as well. Eg resuming with corrupt state should fail and cannot be recovered except via reset.

Quoted 3 lines

The 101b state indicates _RUNNING while _RESUMING, which is simply not a mode that has been spec'd at this time as it would require some mechanism for the device to fault in state on demand.

So lets error on these requests since we don't know what state to put the device into.

Quoted 9 lines

Quoted 3 lines
The two actions:
trigger serializing the device state
Done when asked to go to 010b ?
When the _SAVING bit is set. The exact mechanics depends on the size and volatility of the device state. A GPU might begin in pre-copy (011b) to transmit chunks of framebuffer data, recording hashes of blocks read by the user to avoid re-sending them during the stop-and-copy (010b) phase.

Here I am talking specifically about mlx5 which does not have a state capture in pre-copy. So mlx5 should capture state on 010b only, and the 011b is a NOP.

Quoted 5 lines

Quoted 2 lines
trigger de-serializing the device state
Done when transition from 100b -> 000b ?
100b -> 000b is not a required transition, generally this would be 100b -> 001b, ie. end state of _RUNNING vs _STOP.

Sorry, I typo'd it, yes to _RUNNING

Quoted 9 lines

I think the requirement is that de-serialization is complete when the _RESUMING bit is cleared. Whether the driver chooses to de-serialize piece-wise as each block of data is written to the device or in bulk from a buffer is left to the implementation. In either case, the driver can fail the transition to !_RESUMING if the state is incomplete or otherwise corrupt. It would again be the driver's discretion if the device enters the error state or remains in _RESUMING. If the user has no valid state with which to exit the _RESUMING phase, a device reset should return the device to _RUNNING with a default initial state.

That makes sense enough.

Quoted 6 lines

Quoted 2 lines
There is a missing state "Stop Active Transactions" which would be only "does not issue DMAs". I've seen a proposal to add that.
This would be to get all devices to stop issuing DMA while internal state can be modified to avoid the synchronization issue of trying to stop devices concurrently?

Yes, as above

Quoted 3 lines

For PCI devices we obviously have the bus master bit to manage that, but I could see how a migration extension for such support (perhaps even just wired through to BM for PCI) could be useful. Thanks,

I'm nervous to override the BM bit for something like this, the BM bit isn't a gentle "please coherently stop what you are doing" it is a hanbrake the OS pulls to ensure any PCI device becomes quiet.

Thanks,
Jason

Max Gurtovoy

2021-09-29 10:57 UTC #29 lore raw

Re: [PATCH mlx5-next 2/7] vfio: Add an API to check migration state transition validity

In reply to Alex Williamson

Quoted 36 lines · Alex Williamson

Quoted 9 lines · On Mon, 27 Sep 2021 20:12:39 -0300 Jason Gunthorpe <[email protected]> wrote:
Quoted 8 lines · Alex Williamson
drivers/vfio/vfio.c static int vfio_device_fops_release(struct inode *inode, struct file *filep) · line 1681
Quoted 6 lines
drivers/vfio/vfio.c +6 -0
+	enum { MAX_STATE = VFIO_DEVICE_STATE_RESUMING };
+	static const u8 vfio_from_state_table[MAX_STATE + 1][MAX_STATE + 1] = {
+		[VFIO_DEVICE_STATE_STOP] = {
+			[VFIO_DEVICE_STATE_RUNNING] = 1,
+			[VFIO_DEVICE_STATE_RESUMING] = 1,
+		},
Our state transition diagram is pretty weak on reachable transitions out of the _STOP state, why do we select only these two as valid?
Quoted 3 lines
I have no particular opinion on specific states here, however adding more states means more stuff for drivers to implement and more risk driver writers will mess up this uAPI.
It looks like state transitions were largely discussed in v9 and v10 of the migration proposals:
https://lore.kernel.org/all/[email protected]/
https://lore.kernel.org/all/[email protected]/
I'm not seeing that we really excluded many transitions there.
Quoted 5 lines
So only on those grounds I'd suggest to keep this to the minimum needed instead of the maximum logically possible..
Also, probably the FSM comment from the uapi header file should be moved into a function comment above this function?
It's not clear this function shouldn't be anything more than:
	if (new_state > MAX_STATE || old_state > MAX_STATE)
		return false;	/* exited via device reset, */
				/* entered via transition fault */

	return true;

Quoted 8 lines

That's still only 5 fully interconnected states to work between, and potentially a 6th if we decide _RESUMING|_RUNNING is valid for a device supporting post-copy.
In defining the device state, we tried to steer away from defining it in terms of the QEMU migration API, but rather as a set of controls that could be used to support that API to leave us some degree of independence that QEMU implementation might evolve.

The state machine is not related to QEMU specifically.

The state machine defines an agreement between user application (let's say QEMU) and VFIO.

If a user application would like to move, for example, from RESUMING to SAVING state, then the kernel should fail. I don't that there is a device that can support it.

If you prefer we check this inside our mlx5 vfio driver, we can do it. But we think that this is a common logic according to the defined FSM.

Do you prefer code duplication in vendor vfio-pci drivers ?

Quoted 7 lines

To that extent, it actually seems easier for a device implementation to focus on bit definition rather than the state machine node.
I'd also vote that any clarification of state validity and transitions belongs in the uAPI header and a transition test function should reference that header as the source of truth, rather than the other way around. Thanks,

Yes, I guess this is possible.

Quoted 3 lines

Alex

Max Gurtovoy

2021-09-29 10:44 UTC #30 lore raw

Re: [PATCH mlx5-next 2/7] vfio: Add an API to check migration state transition validity

In reply to Jason Gunthorpe

Quoted 9 lines · Jason Gunthorpe

Quoted 8 lines · Alex Williamson
drivers/vfio/vfio.c static int vfio_device_fops_release(struct inode *inode, struct file *filep) · line 1681
Quoted 6 lines
drivers/vfio/vfio.c +6 -0
+	enum { MAX_STATE = VFIO_DEVICE_STATE_RESUMING };
+	static const u8 vfio_from_state_table[MAX_STATE + 1][MAX_STATE + 1] = {
+		[VFIO_DEVICE_STATE_STOP] = {
+			[VFIO_DEVICE_STATE_RUNNING] = 1,
+			[VFIO_DEVICE_STATE_RESUMING] = 1,
+		},
Our state transition diagram is pretty weak on reachable transitions out of the _STOP state, why do we select only these two as valid?

Quoted 3 lines

I have no particular opinion on specific states here, however adding more states means more stuff for drivers to implement and more risk driver writers will mess up this uAPI.

_STOP == 000b => Device Stopped, not saving or resuming (from UAPI).

This is the default initial state and not RUNNING.

The user application should move device from STOP => RUNNING or STOP => RESUMING.

Maybe we need to extend the comment in the UAPI file.

Quoted 8 lines

So only on those grounds I'd suggest to keep this to the minimum needed instead of the maximum logically possible..
Also, probably the FSM comment from the uapi header file should be moved into a function comment above this function?
Jason

Alex Williamson

2021-09-29 12:35 UTC #31 lore raw

Re: [PATCH mlx5-next 2/7] vfio: Add an API to check migration state transition validity

Quoted 14 lines · On Wed, 29 Sep 2021 13:44:10 +0300 Max Gurtovoy <[email protected]> wrote:

Quoted 9 lines · Jason Gunthorpe
Quoted 8 lines · Alex Williamson
drivers/vfio/vfio.c static int vfio_device_fops_release(struct inode *inode, struct file *filep) · line 1681
Quoted 6 lines
drivers/vfio/vfio.c +6 -0
+	enum { MAX_STATE = VFIO_DEVICE_STATE_RESUMING };
+	static const u8 vfio_from_state_table[MAX_STATE + 1][MAX_STATE + 1] = {
+		[VFIO_DEVICE_STATE_STOP] = {
+			[VFIO_DEVICE_STATE_RUNNING] = 1,
+			[VFIO_DEVICE_STATE_RESUMING] = 1,
+		},  
Our state transition diagram is pretty weak on reachable transitions out of the _STOP state, why do we select only these two as valid?
Quoted 3 lines
I have no particular opinion on specific states here, however adding more states means more stuff for drivers to implement and more risk driver writers will mess up this uAPI.

Quoted 8 lines

_STOP == 000b => Device Stopped, not saving or resuming (from UAPI).
This is the default initial state and not RUNNING.
The user application should move device from STOP => RUNNING or STOP => RESUMING.
Maybe we need to extend the comment in the UAPI file.

include/uapi/linux/vfio.h:
...

 *  +------- _RESUMING
 *  |+------ _SAVING
 *  ||+----- _RUNNING
 *  |||
 *  000b => Device Stopped, not saving or resuming
 *  001b => Device running, which is the default state
                            ^^^^^^^^^^^^^^^^^^^^^^^^^^

...
* State transitions:
*
* _RESUMING _RUNNING Pre-copy Stop-and-copy _STOP
* (100b) (001b) (011b) (010b) (000b)
* 0. Running or default state
* |

                 ^^^^^^^^^^^^^
...
 * 0. Default state of VFIO device is _RUNNING when the user application starts.
      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

The uAPI is pretty clear here. A default state of _STOP is not compatible with existing devices and userspace that does not support migration. Thanks,

Alex

Max Gurtovoy

2021-09-29 13:27 UTC #32 lore raw

Re: [PATCH mlx5-next 2/7] vfio: Add an API to check migration state transition validity

Quoted 41 lines · Alex Williamson

Quoted 13 lines · On Wed, 29 Sep 2021 13:44:10 +0300 Max Gurtovoy <[email protected]> wrote:
Quoted 9 lines · Jason Gunthorpe
Quoted 8 lines · Alex Williamson
drivers/vfio/vfio.c static int vfio_device_fops_release(struct inode *inode, struct file *filep) · line 1681
Quoted 6 lines
drivers/vfio/vfio.c +6 -0
+	enum { MAX_STATE = VFIO_DEVICE_STATE_RESUMING };
+	static const u8 vfio_from_state_table[MAX_STATE + 1][MAX_STATE + 1] = {
+		[VFIO_DEVICE_STATE_STOP] = {
+			[VFIO_DEVICE_STATE_RUNNING] = 1,
+			[VFIO_DEVICE_STATE_RESUMING] = 1,
+		},
Our state transition diagram is pretty weak on reachable transitions out of the _STOP state, why do we select only these two as valid?
Quoted 3 lines
I have no particular opinion on specific states here, however adding more states means more stuff for drivers to implement and more risk driver writers will mess up this uAPI.
Quoted 8 lines
_STOP == 000b => Device Stopped, not saving or resuming (from UAPI).
This is the default initial state and not RUNNING.
The user application should move device from STOP => RUNNING or STOP => RESUMING.
Maybe we need to extend the comment in the UAPI file.
include/uapi/linux/vfio.h:
...
  *  +------- _RESUMING
  *  |+------ _SAVING
  *  ||+----- _RUNNING
  *  |||
  *  000b => Device Stopped, not saving or resuming
  *  001b => Device running, which is the default state
                             ^^^^^^^^^^^^^^^^^^^^^^^^^^
...
* State transitions:
*
* _RESUMING _RUNNING Pre-copy Stop-and-copy _STOP
* (100b) (001b) (011b) (010b) (000b)
* 0. Running or default state
* |

Quoted 8 lines

                  ^^^^^^^^^^^^^
...
  * 0. Default state of VFIO device is _RUNNING when the user application starts.
       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
The uAPI is pretty clear here. A default state of _STOP is not compatible with existing devices and userspace that does not support migration. Thanks,

Why do you need this state machine for userspace that doesn't support migration ?

What is the definition of RUNNING state for a paused VM that is waiting for incoming migration blob ?

Quoted 3 lines

Alex

Alex Williamson

2021-09-29 13:50 UTC #33 lore raw

Re: [PATCH mlx5-next 2/7] vfio: Add an API to check migration state transition validity

Quoted 47 lines · On Wed, 29 Sep 2021 16:26:55 +0300 Max Gurtovoy <[email protected]> wrote:

Quoted 46 lines · Alex Williamson
Quoted 13 lines · On Wed, 29 Sep 2021 13:44:10 +0300 Max Gurtovoy <[email protected]> wrote:
Quoted 9 lines · Jason Gunthorpe
Quoted 8 lines · Alex Williamson
drivers/vfio/vfio.c static int vfio_device_fops_release(struct inode *inode, struct file *filep) · line 1681
Quoted 6 lines
drivers/vfio/vfio.c +6 -0
+	enum { MAX_STATE = VFIO_DEVICE_STATE_RESUMING };
+	static const u8 vfio_from_state_table[MAX_STATE + 1][MAX_STATE + 1] = {
+		[VFIO_DEVICE_STATE_STOP] = {
+			[VFIO_DEVICE_STATE_RUNNING] = 1,
+			[VFIO_DEVICE_STATE_RESUMING] = 1,
+		},  
Our state transition diagram is pretty weak on reachable transitions out of the _STOP state, why do we select only these two as valid?
Quoted 3 lines
I have no particular opinion on specific states here, however adding more states means more stuff for drivers to implement and more risk driver writers will mess up this uAPI.
Quoted 8 lines
_STOP == 000b => Device Stopped, not saving or resuming (from UAPI).
This is the default initial state and not RUNNING.
The user application should move device from STOP => RUNNING or STOP => RESUMING.
Maybe we need to extend the comment in the UAPI file.
include/uapi/linux/vfio.h:
...
  *  +------- _RESUMING
  *  |+------ _SAVING
  *  ||+----- _RUNNING
  *  |||
  *  000b => Device Stopped, not saving or resuming
  *  001b => Device running, which is the default state
                             ^^^^^^^^^^^^^^^^^^^^^^^^^^
...
* State transitions:
*
* _RESUMING _RUNNING Pre-copy Stop-and-copy _STOP
* (100b) (001b) (011b) (010b) (000b)
* 0. Running or default state
* |
                  ^^^^^^^^^^^^^
...
  * 0. Default state of VFIO device is _RUNNING when the user application starts.
       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Quoted 6 lines

Quoted 3 lines
The uAPI is pretty clear here. A default state of _STOP is not compatible with existing devices and userspace that does not support migration. Thanks,
Why do you need this state machine for userspace that doesn't support migration ?

For userspace that doesn't support migration, there's one state, _RUNNING. That's what we're trying to be compatible and consistent with. Migration is an extension, not a base requirement.

Quoted 2 lines

What is the definition of RUNNING state for a paused VM that is waiting for incoming migration blob ?

A VM supporting migration of the device would move the device to _RESUMING to load the incoming data. If the VM leaves the device in _RUNNING, then it doesn't support migration of the device and it's out of scope how it handles that device state. Existing devices continue running regardless of whether the VM state is paused, it's only devices supporting migration where userspace could optionally have the device run state follow the VM run state. Thanks,

Alex

Max Gurtovoy

2021-09-29 14:37 UTC #34 lore raw

Re: [PATCH mlx5-next 2/7] vfio: Add an API to check migration state transition validity

Quoted 49 lines · Alex Williamson

Quoted 46 lines · On Wed, 29 Sep 2021 16:26:55 +0300 Max Gurtovoy <[email protected]> wrote:
Quoted 45 lines · Alex Williamson
Quoted 13 lines · On Wed, 29 Sep 2021 13:44:10 +0300 Max Gurtovoy <[email protected]> wrote:
Quoted 9 lines · Jason Gunthorpe
Quoted 8 lines · Alex Williamson
Review
Quoted 6 lines
(quoted diff) +6 -0
+	enum { MAX_STATE = VFIO_DEVICE_STATE_RESUMING };
+	static const u8 vfio_from_state_table[MAX_STATE + 1][MAX_STATE + 1] = {
+		[VFIO_DEVICE_STATE_STOP] = {
+			[VFIO_DEVICE_STATE_RUNNING] = 1,
+			[VFIO_DEVICE_STATE_RESUMING] = 1,
+		},
Our state transition diagram is pretty weak on reachable transitions out of the _STOP state, why do we select only these two as valid?
Quoted 3 lines
I have no particular opinion on specific states here, however adding more states means more stuff for drivers to implement and more risk driver writers will mess up this uAPI.
Quoted 8 lines
_STOP == 000b => Device Stopped, not saving or resuming (from UAPI).
This is the default initial state and not RUNNING.
The user application should move device from STOP => RUNNING or STOP => RESUMING.
Maybe we need to extend the comment in the UAPI file.
include/uapi/linux/vfio.h:
...
   *  +------- _RESUMING
   *  |+------ _SAVING
   *  ||+----- _RUNNING
   *  |||
   *  000b => Device Stopped, not saving or resuming
   *  001b => Device running, which is the default state
                              ^^^^^^^^^^^^^^^^^^^^^^^^^^
...
* State transitions:
*
* _RESUMING _RUNNING Pre-copy Stop-and-copy _STOP
* (100b) (001b) (011b) (010b) (000b)
* 0. Running or default state
* |
                   ^^^^^^^^^^^^^
...
   * 0. Default state of VFIO device is _RUNNING when the user application starts.
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Quoted 8 lines

Quoted 5 lines
Quoted 3 lines
The uAPI is pretty clear here. A default state of _STOP is not compatible with existing devices and userspace that does not support migration. Thanks,
Why do you need this state machine for userspace that doesn't support migration ?
For userspace that doesn't support migration, there's one state, _RUNNING. That's what we're trying to be compatible and consistent with. Migration is an extension, not a base requirement.

Userspace without migration doesn't care about this state.

We left with kernel now. vfio-pci today doesn't support migration, right ? state is in theory is 0 (STOP).

This state machine is controlled by the migration SW. The drivers don't move state implicitly.

mlx5-vfio-pci support migration and will work fine with non-migration SW (it will stay with state = 0 unless someone will move it. but nobody will) exactly like vfio-pci does today.

So where is the problem ?

Quoted 12 lines

Quoted 2 lines
What is the definition of RUNNING state for a paused VM that is waiting for incoming migration blob ?
A VM supporting migration of the device would move the device to _RESUMING to load the incoming data. If the VM leaves the device in _RUNNING, then it doesn't support migration of the device and it's out of scope how it handles that device state. Existing devices continue running regardless of whether the VM state is paused, it's only devices supporting migration where userspace could optionally have the device run state follow the VM run state. Thanks,
Alex

Alex Williamson

2021-09-29 15:17 UTC #35 lore raw

Re: [PATCH mlx5-next 2/7] vfio: Add an API to check migration state transition validity

Quoted 64 lines · On Wed, 29 Sep 2021 17:36:59 +0300 Max Gurtovoy <[email protected]> wrote:

Quoted 49 lines · Alex Williamson
Quoted 46 lines · On Wed, 29 Sep 2021 16:26:55 +0300 Max Gurtovoy <[email protected]> wrote:
Quoted 45 lines · Alex Williamson
Quoted 13 lines · On Wed, 29 Sep 2021 13:44:10 +0300 Max Gurtovoy <[email protected]> wrote:
Quoted 9 lines · Jason Gunthorpe
Quoted 8 lines · Alex Williamson
Review
Quoted 6 lines
(quoted diff) +6 -0
+	enum { MAX_STATE = VFIO_DEVICE_STATE_RESUMING };
+	static const u8 vfio_from_state_table[MAX_STATE + 1][MAX_STATE + 1] = {
+		[VFIO_DEVICE_STATE_STOP] = {
+			[VFIO_DEVICE_STATE_RUNNING] = 1,
+			[VFIO_DEVICE_STATE_RESUMING] = 1,
+		},  
Our state transition diagram is pretty weak on reachable transitions out of the _STOP state, why do we select only these two as valid?
Quoted 3 lines
I have no particular opinion on specific states here, however adding more states means more stuff for drivers to implement and more risk driver writers will mess up this uAPI.
Quoted 8 lines
_STOP == 000b => Device Stopped, not saving or resuming (from UAPI).
This is the default initial state and not RUNNING.
The user application should move device from STOP => RUNNING or STOP => RESUMING.
Maybe we need to extend the comment in the UAPI file.
include/uapi/linux/vfio.h:
...
   *  +------- _RESUMING
   *  |+------ _SAVING
   *  ||+----- _RUNNING
   *  |||
   *  000b => Device Stopped, not saving or resuming
   *  001b => Device running, which is the default state
                              ^^^^^^^^^^^^^^^^^^^^^^^^^^
...
* State transitions:
*
* _RESUMING _RUNNING Pre-copy Stop-and-copy _STOP
* (100b) (001b) (011b) (010b) (000b)
* 0. Running or default state
* |
                   ^^^^^^^^^^^^^
...
   * 0. Default state of VFIO device is _RUNNING when the user application starts.
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
The uAPI is pretty clear here. A default state of _STOP is not compatible with existing devices and userspace that does not support migration. Thanks,
Why do you need this state machine for userspace that doesn't support migration ?
For userspace that doesn't support migration, there's one state, _RUNNING. That's what we're trying to be compatible and consistent with. Migration is an extension, not a base requirement.
Userspace without migration doesn't care about this state.
We left with kernel now. vfio-pci today doesn't support migration, right ? state is in theory is 0 (STOP).

Quoted 8 lines

This state machine is controlled by the migration SW. The drivers don't move state implicitly.
mlx5-vfio-pci support migration and will work fine with non-migration SW (it will stay with state = 0 unless someone will move it. but nobody will) exactly like vfio-pci does today.
So where is the problem ?

So you have a device that's actively modifying its internal state, performing I/O, including DMA (thereby dirtying VM memory), all while in the _STOP state? And you don't see this as a problem?

There's a major inconsistency if the migration interface is telling us something different than we can actually observe through the behavior of the device. Thanks,

Alex

Max Gurtovoy

2021-09-29 15:29 UTC #36 lore raw

Re: [PATCH mlx5-next 2/7] vfio: Add an API to check migration state transition validity

Quoted 69 lines · Alex Williamson

Quoted 66 lines · On Wed, 29 Sep 2021 17:36:59 +0300 Max Gurtovoy <[email protected]> wrote:
Quoted 49 lines · Alex Williamson
Quoted 46 lines · On Wed, 29 Sep 2021 16:26:55 +0300 Max Gurtovoy <[email protected]> wrote:
Quoted 45 lines · Alex Williamson
Quoted 13 lines · On Wed, 29 Sep 2021 13:44:10 +0300 Max Gurtovoy <[email protected]> wrote:
Quoted 9 lines · Jason Gunthorpe
Quoted 8 lines · Alex Williamson
Review
Quoted 6 lines
(quoted diff) +6 -0
+	enum { MAX_STATE = VFIO_DEVICE_STATE_RESUMING };
+	static const u8 vfio_from_state_table[MAX_STATE + 1][MAX_STATE + 1] = {
+		[VFIO_DEVICE_STATE_STOP] = {
+			[VFIO_DEVICE_STATE_RUNNING] = 1,
+			[VFIO_DEVICE_STATE_RESUMING] = 1,
+		},
Our state transition diagram is pretty weak on reachable transitions out of the _STOP state, why do we select only these two as valid?
Quoted 3 lines
I have no particular opinion on specific states here, however adding more states means more stuff for drivers to implement and more risk driver writers will mess up this uAPI.
Quoted 8 lines
_STOP == 000b => Device Stopped, not saving or resuming (from UAPI).
This is the default initial state and not RUNNING.
The user application should move device from STOP => RUNNING or STOP => RESUMING.
Maybe we need to extend the comment in the UAPI file.
include/uapi/linux/vfio.h:
...
    *  +------- _RESUMING
    *  |+------ _SAVING
    *  ||+----- _RUNNING
    *  |||
    *  000b => Device Stopped, not saving or resuming
    *  001b => Device running, which is the default state
                               ^^^^^^^^^^^^^^^^^^^^^^^^^^
...
* State transitions:
*
* _RESUMING _RUNNING Pre-copy Stop-and-copy _STOP
* (100b) (001b) (011b) (010b) (000b)
* 0. Running or default state
* |
^^^^^^^^^^^^^
...
* 0. Default state of VFIO device is _RUNNING when the user application starts. ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
The uAPI is pretty clear here. A default state of _STOP is not compatible with existing devices and userspace that does not support migration. Thanks,
Why do you need this state machine for userspace that doesn't support migration ?
For userspace that doesn't support migration, there's one state, _RUNNING. That's what we're trying to be compatible and consistent with. Migration is an extension, not a base requirement.
Userspace without migration doesn't care about this state.
We left with kernel now. vfio-pci today doesn't support migration, right ? state is in theory is 0 (STOP).
This state machine is controlled by the migration SW. The drivers don't move state implicitly.

Quoted 8 lines

Quoted 5 lines
mlx5-vfio-pci support migration and will work fine with non-migration SW (it will stay with state = 0 unless someone will move it. but nobody will) exactly like vfio-pci does today.
So where is the problem ?
So you have a device that's actively modifying its internal state, performing I/O, including DMA (thereby dirtying VM memory), all while in the _STOP state? And you don't see this as a problem?

I don't see how is it different from vfio-pci situation.

And you said you're worried from compatibility. I can't see a compatibility issue here.

Maybe we need to rename STOP state. We can call it READY or LIVE or NON_MIGRATION_STATE.

Quoted 7 lines

There's a major inconsistency if the migration interface is telling us something different than we can actually observe through the behavior of the device. Thanks,
Alex

Jason Gunthorpe

2021-09-29 16:14 UTC #37 lore raw

Re: [PATCH mlx5-next 2/7] vfio: Add an API to check migration state transition validity

Quoted 5 lines · Max Gurtovoy

Quoted 3 lines
So you have a device that's actively modifying its internal state, performing I/O, including DMA (thereby dirtying VM memory), all while in the _STOP state? And you don't see this as a problem?
I don't see how is it different from vfio-pci situation.

vfio-pci provides no way to observe the migration state. It isn't "000b"

Quoted 2 lines

Maybe we need to rename STOP state. We can call it READY or LIVE or NON_MIGRATION_STATE.

It was a poor choice to use 000b as stop, but it doesn't really matter. The mlx5 driver should just pre-init this readable to running.

Jason

Max Gurtovoy

2021-09-29 21:49 UTC #38 lore raw

Re: [PATCH mlx5-next 2/7] vfio: Add an API to check migration state transition validity

Quoted 8 lines · Jason Gunthorpe

Quoted 4 lines · Max Gurtovoy
Quoted 3 lines
So you have a device that's actively modifying its internal state, performing I/O, including DMA (thereby dirtying VM memory), all while in the _STOP state? And you don't see this as a problem?
I don't see how is it different from vfio-pci situation.
vfio-pci provides no way to observe the migration state. It isn't "000b"

Alex said that there is a problem of compatibility.

I migration SW is not involved, nobody will read this migration state.

Quoted 4 lines

Quoted 2 lines
Maybe we need to rename STOP state. We can call it READY or LIVE or NON_MIGRATION_STATE.
It was a poor choice to use 000b as stop, but it doesn't really matter. The mlx5 driver should just pre-init this readable to running.

I guess we can do it for this reason. There is no functional problem nor compatibility issue here as was mentioned.

But still we need the kernel to track transitions. We don't want to allow moving from RESUMING to SAVING state for example. How this transition can be allowed ?

In this case we need to fail the request from the migration SW...

Quoted 2 lines

Jason

Alex Williamson

2021-09-29 22:44 UTC #39 lore raw

Re: [PATCH mlx5-next 2/7] vfio: Add an API to check migration state transition validity

Quoted 6 lines · On Thu, 30 Sep 2021 00:48:55 +0300 Max Gurtovoy <[email protected]> wrote:

Quoted 5 lines · Jason Gunthorpe
Quoted 3 lines · Max Gurtovoy
Quoted 3 lines
So you have a device that's actively modifying its internal state, performing I/O, including DMA (thereby dirtying VM memory), all while in the _STOP state? And you don't see this as a problem?

Quoted 7 lines

Quoted 3 lines
Quoted 1 line
I don't see how is it different from vfio-pci situation.
vfio-pci provides no way to observe the migration state. It isn't "000b"
Alex said that there is a problem of compatibility.
I migration SW is not involved, nobody will read this migration state.

The _STOP state has a specific meaning regardless of whether userspace reads the device state value. I think what you're suggesting is that the device reports itself as _STOP'd but it's actually _RUNNING. Is that the compatibility workaround, create a self inconsistency?

We cannot impose on userspace to move a device from _STOP to _RUNNING simply because the device supports the migration region, nor should we report a device state that is inconsistent with the actual device state.

Quoted 5 lines

Quoted 4 lines
Quoted 2 lines
Maybe we need to rename STOP state. We can call it READY or LIVE or NON_MIGRATION_STATE.
It was a poor choice to use 000b as stop, but it doesn't really matter. The mlx5 driver should just pre-init this readable to running.

Quoted 8 lines

I guess we can do it for this reason. There is no functional problem nor compatibility issue here as was mentioned.
But still we need the kernel to track transitions. We don't want to allow moving from RESUMING to SAVING state for example. How this transition can be allowed ?
In this case we need to fail the request from the migration SW...

_RESUMING to _SAVING seems like a good way to test round trip migration without running the device to modify the state. Potentially it's a means to update a saved device migration data stream to a newer format using an intermediate driver version.

If a driver is written such that it simply sees clearing the _RESUME bit as an indicator to de-serialize the data stream to the device, and setting the _SAVING flag as an indicator to re-serialize that data stream from the device, then this is just a means to make use of existing data paths.

The uAPI specifies a means for drivers to reject a state change, but that risks failing to support a transition which might find mainstream use cases. I don't think common code should be responsible for filtering out viable transitions. Thanks,

Alex

Max Gurtovoy

2021-09-30 09:25 UTC #40 lore raw

Re: [PATCH mlx5-next 2/7] vfio: Add an API to check migration state transition validity

Quoted 12 lines · Alex Williamson

Quoted 9 lines · On Thu, 30 Sep 2021 00:48:55 +0300 Max Gurtovoy <[email protected]> wrote:
Quoted 8 lines · Jason Gunthorpe
Quoted 4 lines · Max Gurtovoy
Quoted 3 lines
So you have a device that's actively modifying its internal state, performing I/O, including DMA (thereby dirtying VM memory), all while in the _STOP state? And you don't see this as a problem?
I don't see how is it different from vfio-pci situation.
vfio-pci provides no way to observe the migration state. It isn't "000b"

Quoted 7 lines

Quoted 3 lines
Alex said that there is a problem of compatibility.
I migration SW is not involved, nobody will read this migration state.
The _STOP state has a specific meaning regardless of whether userspace reads the device state value. I think what you're suggesting is that the device reports itself as _STOP'd but it's actually _RUNNING. Is that the compatibility workaround, create a self inconsistency?

From migration point of view the device is stopped.

Quoted 4 lines

We cannot impose on userspace to move a device from _STOP to _RUNNING simply because the device supports the migration region, nor should we report a device state that is inconsistent with the actual device state.

In this case we can think maybe moving to running during enabling the bus master..

Quoted 12 lines

Quoted 12 lines
Quoted 5 lines
Quoted 3 lines
Maybe we need to rename STOP state. We can call it READY or LIVE or NON_MIGRATION_STATE.
It was a poor choice to use 000b as stop, but it doesn't really matter. The mlx5 driver should just pre-init this readable to running.
I guess we can do it for this reason. There is no functional problem nor compatibility issue here as was mentioned.
But still we need the kernel to track transitions. We don't want to allow moving from RESUMING to SAVING state for example. How this transition can be allowed ?

Quoted 5 lines

Quoted 1 line
In this case we need to fail the request from the migration SW...
_RESUMING to _SAVING seems like a good way to test round trip migration without running the device to modify the state. Potentially it's a means to update a saved device migration data stream to a newer format using an intermediate driver version.

what do you mean by "without running the device to modify the state." ?

did you describe a case where you migrate from source to dst and then back to source with a new migration data format ?

Quoted 14 lines

If a driver is written such that it simply sees clearing the _RESUME bit as an indicator to de-serialize the data stream to the device, and setting the _SAVING flag as an indicator to re-serialize that data stream from the device, then this is just a means to make use of existing data paths.
The uAPI specifies a means for drivers to reject a state change, but that risks failing to support a transition which might find mainstream use cases. I don't think common code should be responsible for filtering out viable transitions. Thanks,
Alex

Alex Williamson

2021-09-30 12:41 UTC #41 lore raw

Re: [PATCH mlx5-next 2/7] vfio: Add an API to check migration state transition validity

Quoted 15 lines · On Thu, 30 Sep 2021 12:25:23 +0300 Max Gurtovoy <[email protected]> wrote:

Quoted 14 lines · Alex Williamson
Quoted 11 lines · On Thu, 30 Sep 2021 00:48:55 +0300 Max Gurtovoy <[email protected]> wrote:
Quoted 8 lines · Jason Gunthorpe
Quoted 4 lines · Max Gurtovoy
Quoted 3 lines
So you have a device that's actively modifying its internal state, performing I/O, including DMA (thereby dirtying VM memory), all while in the _STOP state? And you don't see this as a problem?
I don't see how is it different from vfio-pci situation.
vfio-pci provides no way to observe the migration state. It isn't "000b"
Alex said that there is a problem of compatibility.

Quoted 7 lines

Quoted 5 lines
Quoted 1 line
I migration SW is not involved, nobody will read this migration state.
The _STOP state has a specific meaning regardless of whether userspace reads the device state value. I think what you're suggesting is that the device reports itself as _STOP'd but it's actually _RUNNING. Is that the compatibility workaround, create a self inconsistency?
From migration point of view the device is stopped.

The _RESUMING and _SAVING bits control the migration activity, the _RUNNING bit controls the ability of the device to modify its internal state and affect external state. The initial state of the device is absolutely not stopped.

Quoted 6 lines

Quoted 3 lines
We cannot impose on userspace to move a device from _STOP to _RUNNING simply because the device supports the migration region, nor should we report a device state that is inconsistent with the actual device state.
In this case we can think maybe moving to running during enabling the bus master..

There are no spontaneous state transitions, device_state changes only via user manipulation of the register.

Quoted 11 lines

Quoted 11 lines
Quoted 11 lines
Quoted 4 lines
Quoted 2 lines
Maybe we need to rename STOP state. We can call it READY or LIVE or NON_MIGRATION_STATE.
It was a poor choice to use 000b as stop, but it doesn't really matter. The mlx5 driver should just pre-init this readable to running.
I guess we can do it for this reason. There is no functional problem nor compatibility issue here as was mentioned.
But still we need the kernel to track transitions. We don't want to allow moving from RESUMING to SAVING state for example. How this transition can be allowed ?

Quoted 7 lines

Quoted 5 lines
Quoted 1 line
In this case we need to fail the request from the migration SW...
_RESUMING to _SAVING seems like a good way to test round trip migration without running the device to modify the state. Potentially it's a means to update a saved device migration data stream to a newer format using an intermediate driver version.
what do you mean by "without running the device to modify the state." ?

If a device is !_RUNNING it should not be advancing its internal state, therefore state-in == state-out.

Quoted 2 lines

did you describe a case where you migrate from source to dst and then back to source with a new migration data format ?

I'm speculating that as the driver evolves, the migration data stream generated from the device's migration region can change. Hopefully in compatible ways. The above sequence of restoring and extracting state without the complication of the device running could help to validate compatibility. Thanks,

Alex

Jason Gunthorpe

2021-09-29 23:21 UTC #42 lore raw

Re: [PATCH mlx5-next 2/7] vfio: Add an API to check migration state transition validity

In reply to Max Gurtovoy

Quoted 4 lines · Max Gurtovoy

Quoted 2 lines · Jason Gunthorpe
On Wed, Sep 29, 2021 at 06:28:44PM +0300, Max Gurtovoy wrote:

Quoted 8 lines

Quoted 6 lines
Quoted 4 lines
Quoted 3 lines
So you have a device that's actively modifying its internal state, performing I/O, including DMA (thereby dirtying VM memory), all while in the _STOP state? And you don't see this as a problem?
I don't see how is it different from vfio-pci situation.
vfio-pci provides no way to observe the migration state. It isn't "000b"
Alex said that there is a problem of compatibility.

Yes, when a vfio_device first opens it must be running - ie able to do DMA and otherwise operational.

When we add the migration extension this cannot change, so after open_device() the device should be operational.

The reported state in the migration region should accurately reflect what the device is currently doing. If the device is operational then it must report running, not stopped.

Thus a driver cannot just zero initalize the migration "registers", they have to be accurate.

Quoted 11 lines

Quoted 4 lines
Quoted 2 lines
Maybe we need to rename STOP state. We can call it READY or LIVE or NON_MIGRATION_STATE.
It was a poor choice to use 000b as stop, but it doesn't really matter. The mlx5 driver should just pre-init this readable to running.
I guess we can do it for this reason. There is no functional problem nor compatibility issue here as was mentioned.
But still we need the kernel to track transitions. We don't want to allow moving from RESUMING to SAVING state for example. How this transition can be allowed ?

It seems semantically fine to me, as per Alex's note what will happen is defined:

driver will see RESUMING toggle off so it will trigger a de-serialization

driver will see SAVING toggled on so it will serialize the new state (either the pre-copy state or the post-copy state dpending on the running bit)

Depending on the running bit the device may or may not be woken up.

If de-serialization fails then the state goes to error and SAVING is ignored.

The driver logic probably looks something like this:

// Running toggles off
if (oldstate & RUNNING != newstate & RUNNING && oldstate & RUNNING)
queice
freeze

// Resuming toggles off
if (oldstate & RESUMING != newstate & RESUMING && oldstate & RESUMING)
deserialize

// Saving toggles on
if (oldstate & SAVING != newstate & SAVING && newstate & SAVING)
if (!(newstate & RUNNING))
serialize post copy

// Running toggles on
if (oldstate & RUNNING != newstate & RUNNING && newstate & RUNNING)
unfreeze
unqueice

I'd have to check that carefully against the state chart from my last email though..

And need to check how the "Stop Active Transactions" bit fits in there

Jason

Max Gurtovoy

2021-09-30 09:34 UTC #43 lore raw

Re: [PATCH mlx5-next 2/7] vfio: Add an API to check migration state transition validity

Quoted 7 lines · Jason Gunthorpe

Quoted 6 lines · Max Gurtovoy
Quoted 5 lines · Jason Gunthorpe
Quoted 3 lines · Max Gurtovoy
Quoted 3 lines
So you have a device that's actively modifying its internal state, performing I/O, including DMA (thereby dirtying VM memory), all while in the _STOP state? And you don't see this as a problem?

Quoted 6 lines

Quoted 4 lines
Quoted 3 lines
Quoted 1 line
I don't see how is it different from vfio-pci situation.
vfio-pci provides no way to observe the migration state. It isn't "000b"
Alex said that there is a problem of compatibility.
Yes, when a vfio_device first opens it must be running - ie able to do DMA and otherwise operational.

how can non resumed device do DMA ?

Also the bus master is not set.

Quoted 3 lines

When we add the migration extension this cannot change, so after open_device() the device should be operational.

if it's waiting for incoming migration blob, it is not running.

Quoted 4 lines

The reported state in the migration region should accurately reflect what the device is currently doing. If the device is operational then it must report running, not stopped.

STOP in migration meaning.

Quoted 11 lines

Thus a driver cannot just zero initalize the migration "registers", they have to be accurate.
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
Maybe we need to rename STOP state. We can call it READY or LIVE or NON_MIGRATION_STATE.
It was a poor choice to use 000b as stop, but it doesn't really matter. The mlx5 driver should just pre-init this readable to running.
I guess we can do it for this reason. There is no functional problem nor compatibility issue here as was mentioned.

Quoted 8 lines

Quoted 3 lines
But still we need the kernel to track transitions. We don't want to allow moving from RESUMING to SAVING state for example. How this transition can be allowed ?
It seems semantically fine to me, as per Alex's note what will happen is defined:
driver will see RESUMING toggle off so it will trigger a de-serialization

You mean stop serialization ?

Quoted 4 lines

driver will see SAVING toggled on so it will serialize the new state (either the pre-copy state or the post-copy state dpending on the running bit)

lets leave the bits and how you implement the state numbering aside.

If you finish resuming you can move to a new state (that we should add) => RESUMED.

Now you suggested moving from RESUMED to SAVING to get the state again from the dst device ? and send it back to src ? before staring the VM and moving to RUNNING ?

where this is coming from ?

Quoted 2 lines

Depending on the running bit the device may or may not be woken up.

lets take about logic here and not bits.

Quoted 31 lines

If de-serialization fails then the state goes to error and SAVING is ignored.
The driver logic probably looks something like this:
// Running toggles off
if (oldstate & RUNNING != newstate & RUNNING && oldstate & RUNNING)
queice
freeze
// Resuming toggles off
if (oldstate & RESUMING != newstate & RESUMING && oldstate & RESUMING)
deserialize
// Saving toggles on
if (oldstate & SAVING != newstate & SAVING && newstate & SAVING)
if (!(newstate & RUNNING))
serialize post copy
// Running toggles on
if (oldstate & RUNNING != newstate & RUNNING && newstate & RUNNING)
unfreeze
unqueice
I'd have to check that carefully against the state chart from my last email though..
And need to check how the "Stop Active Transactions" bit fits in there
Jason

Jason Gunthorpe

2021-09-30 14:47 UTC #44 lore raw

Re: [PATCH mlx5-next 2/7] vfio: Add an API to check migration state transition validity

Quoted 4 lines · Max Gurtovoy

Quoted 2 lines
When we add the migration extension this cannot change, so after open_device() the device should be operational.
if it's waiting for incoming migration blob, it is not running.

It cannot be waiting for a migration blob after open_device, that is not backwards compatible.

Just prior to open device the vfio pci layer will generate a FLR to the function so we expect that post open_device has a fresh from reset fully running device state.

Quoted 5 lines

Quoted 3 lines
The reported state in the migration region should accurately reflect what the device is currently doing. If the device is operational then it must report running, not stopped.
STOP in migration meaning.

As Alex and I have said several times STOP means the internal state is not allowed to change.

Quoted 4 lines

Quoted 2 lines
driver will see RESUMING toggle off so it will trigger a de-serialization
You mean stop serialization ?

No, I mean it will take all the migration data that has been uploaded through the migration region and de-serialize it into active device state.

Quoted 5 lines

Quoted 3 lines
driver will see SAVING toggled on so it will serialize the new state (either the pre-copy state or the post-copy state dpending on the running bit)
lets leave the bits and how you implement the state numbering aside.

You've missed the point. This isn't a FSM. It is a series of three control bits that we have assigned logical meaning their combinatoins.

The algorithm I gave is a control centric algorithm not a state centric algorithm and matches the direction Alex thought this was being designed for.

Quoted 2 lines

If you finish resuming you can move to a new state (that we should add) => RESUMED.

It is not a state machine. Once you stop prentending this is implementing a FSM Alex's position makes perfect sense.

Jason

Max Gurtovoy

2021-09-30 15:32 UTC #45 lore raw

Re: [PATCH mlx5-next 2/7] vfio: Add an API to check migration state transition validity

Quoted 11 lines · Jason Gunthorpe

Quoted 3 lines · Max Gurtovoy
Quoted 2 lines
When we add the migration extension this cannot change, so after open_device() the device should be operational.
if it's waiting for incoming migration blob, it is not running.
It cannot be waiting for a migration blob after open_device, that is not backwards compatible.
Just prior to open device the vfio pci layer will generate a FLR to the function so we expect that post open_device has a fresh from reset fully running device state.

running also mean that the device doesn't have a clue on its internal state ? or running means unfreezed and unquiesced ?

Quoted 8 lines

Quoted 5 lines
Quoted 4 lines
The reported state in the migration region should accurately reflect what the device is currently doing. If the device is operational then it must report running, not stopped.
STOP in migration meaning.
As Alex and I have said several times STOP means the internal state is not allowed to change.

Quoted 6 lines

Quoted 3 lines
Quoted 2 lines
driver will see RESUMING toggle off so it will trigger a de-serialization
You mean stop serialization ?
No, I mean it will take all the migration data that has been uploaded through the migration region and de-serialize it into active device state.

you should feed the device way before that.

Quoted 8 lines

Quoted 5 lines
Quoted 4 lines
driver will see SAVING toggled on so it will serialize the new state (either the pre-copy state or the post-copy state dpending on the running bit)
lets leave the bits and how you implement the state numbering aside.
You've missed the point. This isn't a FSM. It is a series of three control bits that we have assigned logical meaning their combinatoins.

Quoted 8 lines

The algorithm I gave is a control centric algorithm not a state centric algorithm and matches the direction Alex thought this was being designed for.
Quoted 2 lines
If you finish resuming you can move to a new state (that we should add) => RESUMED.
It is not a state machine. Once you stop prentending this is implementing a FSM Alex's position makes perfect sense.

You can look on it anyway you want. Three control bits or FSM. And I can look on it anyway I want.

The point is what bits/state you set during the resume phase:

1. you initialize at _RUNNING bit == 001b. No problem.

2. state stream arrives, migration SW raise _RESUMING bit. should it be 101b or 100b ? for now it's 100b. But according to your statement is should be 101b (invalid today) since device state can change. right ?

3. Then you should indicate that all the state was serialized to the device (actually to all the pci devices). 100b mean RESUMING and not RUNNING so maybe this can say RESUMED and state can't change now ?

4. all devices move to running 001b only after all devices moved to 100b.

Otherwise, devices will start changing each other internal states.

-Max.

Quoted 2 lines

Jason

Jason Gunthorpe

2021-09-30 16:24 UTC #46 lore raw

Re: [PATCH mlx5-next 2/7] vfio: Add an API to check migration state transition validity

Quoted 6 lines · Max Gurtovoy

Quoted 3 lines
Just prior to open device the vfio pci layer will generate a FLR to the function so we expect that post open_device has a fresh from reset fully running device state.
running also mean that the device doesn't have a clue on its internal state ? or running means unfreezed and unquiesced ?

The device just got FLR'd and it should be in a clean state and operating. Think the VM is booting for the first time.

Quoted 8 lines

Quoted 6 lines
Quoted 3 lines
Quoted 2 lines
driver will see RESUMING toggle off so it will trigger a de-serialization
You mean stop serialization ?
No, I mean it will take all the migration data that has been uploaded through the migration region and de-serialize it into active device state.
you should feed the device way before that.

I don't know what this means, when the resuming bit is set the migration data buffer is wiped and userspace should beging loading it. When the resuming bit is cleared whatever is in the migration buffer is deserialized into the current device internal state.

It is the opposite of saving. When the saving bit is set the current device state is serialized into the migration buffer and userspace and reads it out.

Quoted 5 lines

1. you initialize at _RUNNING bit == 001b. No problem.
2. state stream arrives, migration SW raise _RESUMING bit. should it be 101b or 100b ? for now it's 100b. But according to your statement is should be 101b (invalid today) since device state can change. right ?

Running means the device state chanages independently, the controlled change of the device state via deserializing the migration buffer is different. Both running and saving commands need running to be zero.

ie commands that are marked invalid in the uapi comment are rejected at the start - and that is probably the core helper we should provide.

Quoted 3 lines

3. Then you should indicate that all the state was serialized to the device (actually to all the pci devices). 100b mean RESUMING and not RUNNING so maybe this can say RESUMED and state can't change now ?

State is not loaded into the device until the resuming bit is cleared. There is no RESUMED state until we incorporate Artem's proposal for an additional bit eg 1001b - running with DMA master disabled.

Jason

Max Gurtovoy

2021-09-30 16:51 UTC #47 lore raw

Re: [PATCH mlx5-next 2/7] vfio: Add an API to check migration state transition validity

Quoted 8 lines · Jason Gunthorpe

Quoted 5 lines · Max Gurtovoy
Quoted 3 lines
Just prior to open device the vfio pci layer will generate a FLR to the function so we expect that post open_device has a fresh from reset fully running device state.
running also mean that the device doesn't have a clue on its internal state ? or running means unfreezed and unquiesced ?
The device just got FLR'd and it should be in a clean state and operating. Think the VM is booting for the first time.

During the resume phase in the dst, the VM is paused and not booting. Migration SW is waiting to get memory and state from SRC. The device will start from the exact point that was in the src.

it's exactly "000b => Device Stopped, not saving or resuming"

Quoted 4 lines

Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 3 lines
driver will see RESUMING toggle off so it will trigger a de-serialization
You mean stop serialization ?

Quoted 8 lines

Quoted 4 lines
Quoted 3 lines
No, I mean it will take all the migration data that has been uploaded through the migration region and de-serialize it into active device state.
you should feed the device way before that.
I don't know what this means, when the resuming bit is set the migration data buffer is wiped and userspace should beging loading it. When the resuming bit is cleared whatever is in the migration buffer is deserialized into the current device internal state.

Well, this is your design for the driver implementation. Nobody is preventing other drivers to start deserializing device state into the device during RESUMING bit on.

Or is this a must ?

Quoted 4 lines

It is the opposite of saving. When the saving bit is set the current device state is serialized into the migration buffer and userspace and reads it out.

This is not new.

Quoted 12 lines

Quoted 5 lines
1. you initialize at _RUNNING bit == 001b. No problem.
2. state stream arrives, migration SW raise _RESUMING bit. should it be 101b or 100b ? for now it's 100b. But according to your statement is should be 101b (invalid today) since device state can change. right ?
Running means the device state chanages independently, the controlled change of the device state via deserializing the migration buffer is different. Both running and saving commands need running to be zero.
ie commands that are marked invalid in the uapi comment are rejected at the start - and that is probably the core helper we should provide.

Quoted 7 lines

Quoted 3 lines
3. Then you should indicate that all the state was serialized to the device (actually to all the pci devices). 100b mean RESUMING and not RUNNING so maybe this can say RESUMED and state can't change now ?
State is not loaded into the device until the resuming bit is cleared. There is no RESUMED state until we incorporate Artem's proposal for an additional bit eg 1001b - running with DMA master disabled.

So if we moved from 100b to 010b somehow, one should deserialized its buffer to the device, and then serialize it to migration region again ?

I guess its doable since the device is freeze and quiesced. But moving from 100b to 011b is not possible, right ?

Quoted 2 lines

Jason

Jason Gunthorpe

2021-09-30 17:01 UTC #48 lore raw

Re: [PATCH mlx5-next 2/7] vfio: Add an API to check migration state transition validity

Quoted 8 lines · Max Gurtovoy

Quoted 6 lines · Jason Gunthorpe
Quoted 5 lines · Max Gurtovoy
Quoted 3 lines
Just prior to open device the vfio pci layer will generate a FLR to the function so we expect that post open_device has a fresh from reset fully running device state.
running also mean that the device doesn't have a clue on its internal state ? or running means unfreezed and unquiesced ?

Quoted 8 lines

Quoted 2 lines
The device just got FLR'd and it should be in a clean state and operating. Think the VM is booting for the first time.
During the resume phase in the dst, the VM is paused and not booting. Migration SW is waiting to get memory and state from SRC. The device will start from the exact point that was in the src.
it's exactly "000b => Device Stopped, not saving or resuming"

For this case qmeu should open the VFIO device and immediately issue a command to go to resuming. The kernel cannot know at open_device time which case userspace is trying to do. Due to backwards compat we assume userspace is going to boot a fresh VM.

Quoted 3 lines

Well, this is your design for the driver implementation. Nobody is preventing other drivers to start deserializing device state into the device during RESUMING bit on.

It is a logical model. Devices can stream the migration data directly into the internal state if they like. It just creates more conditions where they have report an error state.

Quoted 2 lines

So if we moved from 100b to 010b somehow, one should deserialized its buffer to the device, and then serialize it to migration region again ?

Yes.

Quoted 2 lines

I guess its doable since the device is freeze and quiesced. But moving from 100b to 011b is not possible, right ?

Why not?

100b to 011b is no different than going indirectly 100b -> 001b -> 011b

The time spent in 001b is just negligable.

Jason

Leon Romanovsky

author

2021-09-22 10:39 UTC #49 lore raw

PATCH 3/7 vfio/pci_core: Make the region->release() function optional

In reply to Leon Romanovsky

From: Yishai Hadas <[email protected]>

Make the region->release() function optional as in some cases there is nothing to do by driver as part of it.

This is needed for coming patch from this series once we add mlx5_vfio_cpi driver to support live migration but we don't need a migration release function.

Signed-off-by Yishai Hadas <[email protected]>

Signed-off-by Leon Romanovsky <[email protected]>

---
 drivers/vfio/pci/vfio_pci_core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

drivers/vfio/pci/vfio_pci_core.c +2 -1

index 68198e0f2a63..3ddc3adb24de 100644
@@ -341,7 +341,8 @@ void vfio_pci_core_disable(struct vfio_pci_core_device *vdev)
 	vdev->virq_disabled = false;
 
 	for (i = 0; i < vdev->num_regions; i++)
-		vdev->region[i].ops->release(vdev, &vdev->region[i]);
+		if (vdev->region[i].ops->release)
+			vdev->region[i].ops->release(vdev, &vdev->region[i]);
 
 	vdev->num_regions = 0;
 	kfree(vdev->region);

Signature · 1 line

2.31.1

Max Gurtovoy

Reviewed-by on v1 3/7

2021-09-23 13:57 UTC #50 lore raw

Re: [PATCH mlx5-next 3/7] vfio/pci_core: Make the region->release() function optional

Quoted 7 lines · Leon Romanovsky

From: Yishai Hadas <[email protected]>
Make the region->release() function optional as in some cases there is nothing to do by driver as part of it.
This is needed for coming patch from this series once we add mlx5_vfio_cpi driver to support live migration but we don't need a

mlx5_vfio_pci *typo

Quoted 13 lines

migration release function.
Signed-off-by Yishai Hadas <[email protected]>
Signed-off-by Leon Romanovsky <[email protected]>
---
  drivers/vfio/pci/vfio_pci_core.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)
drivers/vfio/pci/vfio_pci_core.c +0 -0
index 68198e0f2a63..3ddc3adb24de 100644

drivers/vfio/pci/vfio_pci_core.c void vfio_pci_core_disable(struct vfio_pci_core_device *vdev) · line 348

Quoted 9 lines

drivers/vfio/pci/vfio_pci_core.c +2 -1

@@ -341,7 +341,8 @@ void vfio_pci_core_disable(struct vfio_pci_core_device *vdev)
  	vdev->virq_disabled = false;
 
  	for (i = 0; i < vdev->num_regions; i++)
-		vdev->region[i].ops->release(vdev, &vdev->region[i]);
+		if (vdev->region[i].ops->release)
+			vdev->region[i].ops->release(vdev, &vdev->region[i]);
 
  	vdev->num_regions = 0;
  	kfree(vdev->region);

Looks good,

Reviewed-by Max Gurtovoy <[email protected]>

Leon Romanovsky

author

2021-09-22 10:39 UTC #51 lore raw

PATCH 4/7 net/mlx5: Introduce migration bits and structures

In reply to Leon Romanovsky

From: Yishai Hadas <[email protected]>

Introduce migration IFC related stuff to enable migration commands.

Signed-off-by Yishai Hadas <[email protected]>

Signed-off-by Leon Romanovsky <[email protected]>

---
 include/linux/mlx5/mlx5_ifc.h | 145 +++++++++++++++++++++++++++++++++-
 1 file changed, 144 insertions(+), 1 deletion(-)

include/linux/mlx5/mlx5_ifc.h +144 -1

index d90a65b6824f..366c7b030eb7 100644
@@ -126,6 +126,11 @@ enum {
 	MLX5_CMD_OP_QUERY_SF_PARTITION            = 0x111,
 	MLX5_CMD_OP_ALLOC_SF                      = 0x113,
 	MLX5_CMD_OP_DEALLOC_SF                    = 0x114,
+	MLX5_CMD_OP_SUSPEND_VHCA                  = 0x115,
+	MLX5_CMD_OP_RESUME_VHCA                   = 0x116,
+	MLX5_CMD_OP_QUERY_VHCA_MIGRATION_STATE    = 0x117,
+	MLX5_CMD_OP_SAVE_VHCA_STATE               = 0x118,
+	MLX5_CMD_OP_LOAD_VHCA_STATE               = 0x119,
 	MLX5_CMD_OP_CREATE_MKEY                   = 0x200,
 	MLX5_CMD_OP_QUERY_MKEY                    = 0x201,
 	MLX5_CMD_OP_DESTROY_MKEY                  = 0x202,

@@ -1719,7 +1724,9 @@ struct mlx5_ifc_cmd_hca_cap_bits {
 	u8         reserved_at_682[0x1];
 	u8         log_max_sf[0x5];
 	u8         apu[0x1];
-	u8         reserved_at_689[0x7];
+	u8         reserved_at_689[0x4];
+	u8         migration[0x1];
+	u8         reserved_at_68d[0x2];
 	u8         log_min_sf_size[0x8];
 	u8         max_num_sf_partitions[0x8];

@@ -11146,4 +11153,140 @@ enum {
 	MLX5_MTT_PERM_RW	= MLX5_MTT_PERM_READ | MLX5_MTT_PERM_WRITE,
 };
 
+enum {
+	MLX5_SUSPEND_VHCA_IN_OP_MOD_SUSPEND_MASTER  = 0x0,
+	MLX5_SUSPEND_VHCA_IN_OP_MOD_SUSPEND_SLAVE   = 0x1,
+};
+
+struct mlx5_ifc_suspend_vhca_in_bits {
+	u8         opcode[0x10];
+	u8         uid[0x10];
+
+	u8         reserved_at_20[0x10];
+	u8         op_mod[0x10];
+
+	u8         reserved_at_40[0x10];
+	u8         vhca_id[0x10];
+
+	u8         reserved_at_60[0x20];
+};
+
+struct mlx5_ifc_suspend_vhca_out_bits {
+	u8         status[0x8];
+	u8         reserved_at_8[0x18];
+
+	u8         syndrome[0x20];
+
+	u8         reserved_at_40[0x40];
+};
+
+enum {
+	MLX5_RESUME_VHCA_IN_OP_MOD_RESUME_SLAVE   = 0x0,
+	MLX5_RESUME_VHCA_IN_OP_MOD_RESUME_MASTER  = 0x1,
+};
+
+struct mlx5_ifc_resume_vhca_in_bits {
+	u8         opcode[0x10];
+	u8         uid[0x10];
+
+	u8         reserved_at_20[0x10];
+	u8         op_mod[0x10];
+
+	u8         reserved_at_40[0x10];
+	u8         vhca_id[0x10];
+
+	u8         reserved_at_60[0x20];
+};
+
+struct mlx5_ifc_resume_vhca_out_bits {
+	u8         status[0x8];
+	u8         reserved_at_8[0x18];
+
+	u8         syndrome[0x20];
+
+	u8         reserved_at_40[0x40];
+};
+
+struct mlx5_ifc_query_vhca_migration_state_in_bits {
+	u8         opcode[0x10];
+	u8         uid[0x10];
+
+	u8         reserved_at_20[0x10];
+	u8         op_mod[0x10];
+
+	u8         reserved_at_40[0x10];
+	u8         vhca_id[0x10];
+
+	u8         reserved_at_60[0x20];
+};
+
+struct mlx5_ifc_query_vhca_migration_state_out_bits {
+	u8         status[0x8];
+	u8         reserved_at_8[0x18];
+
+	u8         syndrome[0x20];
+
+	u8         reserved_at_40[0x40];
+
+	u8         required_umem_size[0x20];
+
+	u8         reserved_at_a0[0x160];
+};
+
+struct mlx5_ifc_save_vhca_state_in_bits {
+	u8         opcode[0x10];
+	u8         uid[0x10];
+
+	u8         reserved_at_20[0x10];
+	u8         op_mod[0x10];
+
+	u8         reserved_at_40[0x10];
+	u8         vhca_id[0x10];
+
+	u8         reserved_at_60[0x20];
+
+	u8         va[0x40];
+
+	u8         mkey[0x20];
+
+	u8         size[0x20];
+};
+
+struct mlx5_ifc_save_vhca_state_out_bits {
+	u8         status[0x8];
+	u8         reserved_at_8[0x18];
+
+	u8         syndrome[0x20];
+
+	u8         reserved_at_40[0x40];
+};
+
+struct mlx5_ifc_load_vhca_state_in_bits {
+	u8         opcode[0x10];
+	u8         uid[0x10];
+
+	u8         reserved_at_20[0x10];
+	u8         op_mod[0x10];
+
+	u8         reserved_at_40[0x10];
+	u8         vhca_id[0x10];
+
+	u8         reserved_at_60[0x20];
+
+	u8         va[0x40];
+
+	u8         mkey[0x20];
+
+	u8         size[0x20];
+};
+
+struct mlx5_ifc_load_vhca_state_out_bits {
+	u8         status[0x8];
+	u8         reserved_at_8[0x18];
+
+	u8         syndrome[0x20];
+
+	u8         reserved_at_40[0x40];
+};
+
 #endif /* MLX5_IFC_H */

Signature · 1 line

2.31.1

Mark Zhang

2021-09-24 05:48 UTC #52 lore raw

Re: [PATCH mlx5-next 4/7] net/mlx5: Introduce migration bits and structures

Quoted 27 lines · Leon Romanovsky

From: Yishai Hadas <[email protected]>

Introduce migration IFC related stuff to enable migration commands.

Signed-off-by Yishai Hadas <[email protected]>

Signed-off-by Leon Romanovsky <[email protected]>

---
  include/linux/mlx5/mlx5_ifc.h | 145 +++++++++++++++++++++++++++++++++-
  1 file changed, 144 insertions(+), 1 deletion(-)

include/linux/mlx5/mlx5_ifc.h +5 -0

index d90a65b6824f..366c7b030eb7 100644
@@ -126,6 +126,11 @@ enum {
  	MLX5_CMD_OP_QUERY_SF_PARTITION            = 0x111,
  	MLX5_CMD_OP_ALLOC_SF                      = 0x113,
  	MLX5_CMD_OP_DEALLOC_SF                    = 0x114,
+	MLX5_CMD_OP_SUSPEND_VHCA                  = 0x115,
+	MLX5_CMD_OP_RESUME_VHCA                   = 0x116,
+	MLX5_CMD_OP_QUERY_VHCA_MIGRATION_STATE    = 0x117,
+	MLX5_CMD_OP_SAVE_VHCA_STATE               = 0x118,
+	MLX5_CMD_OP_LOAD_VHCA_STATE               = 0x119,
  	MLX5_CMD_OP_CREATE_MKEY                   = 0x200,
  	MLX5_CMD_OP_QUERY_MKEY                    = 0x201,
  	MLX5_CMD_OP_DESTROY_MKEY                  = 0x202,

include/linux/mlx5/mlx5_ifc.h struct mlx5_ifc_cmd_hca_cap_bits { · line 1729

Quoted 7 lines

include/linux/mlx5/mlx5_ifc.h +3 -1

@@ -1719,7 +1724,9 @@ struct mlx5_ifc_cmd_hca_cap_bits {
  	u8         reserved_at_682[0x1];
  	u8         log_max_sf[0x5];
  	u8         apu[0x1];
-	u8         reserved_at_689[0x7];
+	u8         reserved_at_689[0x4];
+	u8         migration[0x1];
+	u8         reserved_at_68d[0x2];

Should it be "reserved_at_68e[0x2]"?

Quoted 3 lines

include/linux/mlx5/mlx5_ifc.h +0 -0

  	u8         log_min_sf_size[0x8];
  	u8         max_num_sf_partitions[0x8];

Leon Romanovsky

author

2021-09-22 10:39 UTC #53 lore raw

PATCH 5/7 net/mlx5: Expose APIs to get/put the mlx5 core device

In reply to Leon Romanovsky

From: Yishai Hadas <[email protected]>

Expose an API to get the mlx5 core device from a given PCI device if mlx5_core is its driver.

Upon the get API we stay with the intf_state_mutex locked to make sure that the device can't be gone/unloaded till the caller will complete its job over the device, this expects to be for a short period of time for any flow that the lock is taken.

Upon the put API we unlock the intf_state_mutex.

The use case for those APIs is the migration flow of a VF over VFIO PCI. In that case the VF doesn't ride on mlx5_core, because the device is driving *two* different PCI devices, the PF owned by mlx5_core and the VF owned by the vfio driver.

The mlx5_core of the PF is accessed only during the narrow window of the VF's ioctl that requires its services.

This allows the PF driver to be more independent of the VF driver, so long as it doesn't reset the FW.

Signed-off-by Yishai Hadas <[email protected]>

Signed-off-by Leon Romanovsky <[email protected]>

---
 .../net/ethernet/mellanox/mlx5/core/main.c    | 43 +++++++++++++++++++
 include/linux/mlx5/driver.h                   |  3 ++
 2 files changed, 46 insertions(+)

drivers/net/ethernet/mellanox/mlx5/core/main.c +43 -0

index 79482824c64f..fcc8b7830421 100644
@@ -1795,6 +1795,49 @@ static struct pci_driver mlx5_core_driver = {
 	.sriov_set_msix_vec_count = mlx5_core_sriov_set_msix_vec_count,
 };
 
+/**
+ * mlx5_get_core_dev - Get the mlx5 core device from a given PCI device if
+ *                     mlx5_core is its driver.
+ * @pdev: The associated PCI device.
+ *
+ * Upon return the interface state lock stay held to let caller uses it safely.
+ * Caller must ensure to use the returned mlx5 device for a narrow window
+ * and put it back with mlx5_put_core_dev() immediately once usage was over.
+ *
+ * Return: Pointer to the associated mlx5_core_dev or NULL.
+ */
+struct mlx5_core_dev *mlx5_get_core_dev(struct pci_dev *pdev)
+			__acquires(&mdev->intf_state_mutex)
+{
+	struct mlx5_core_dev *mdev;
+
+	device_lock(&pdev->dev);
+	if (pdev->driver != &mlx5_core_driver) {
+		device_unlock(&pdev->dev);
+		return NULL;
+	}
+
+	mdev = pci_get_drvdata(pdev);
+	mutex_lock(&mdev->intf_state_mutex);
+	device_unlock(&pdev->dev);
+
+	return mdev;
+}
+EXPORT_SYMBOL(mlx5_get_core_dev);
+
+/**
+ * mlx5_put_core_dev - Put the mlx5 core device back.
+ * @mdev: The mlx5 core device.
+ *
+ * Upon return the interface state lock is unlocked and caller should not
+ * access the mdev any more.
+ */
+void mlx5_put_core_dev(struct mlx5_core_dev *mdev)
+{
+	mutex_unlock(&mdev->intf_state_mutex);
+}
+EXPORT_SYMBOL(mlx5_put_core_dev);
+
 static void mlx5_core_verify_params(void)
 {
 	if (prof_sel >= ARRAY_SIZE(profile)) {

include/linux/mlx5/driver.h +3 -0

index 1b8bae246b28..e9a96904d6f1 100644
@@ -1156,6 +1156,9 @@ int mlx5_dm_sw_icm_alloc(struct mlx5_core_dev *dev, enum mlx5_sw_icm_type type,
 int mlx5_dm_sw_icm_dealloc(struct mlx5_core_dev *dev, enum mlx5_sw_icm_type type,
 			   u64 length, u16 uid, phys_addr_t addr, u32 obj_id);
 
+struct mlx5_core_dev *mlx5_get_core_dev(struct pci_dev *pdev);
+void mlx5_put_core_dev(struct mlx5_core_dev *mdev);
+
 #ifdef CONFIG_MLX5_CORE_IPOIB
 struct net_device *mlx5_rdma_netdev_alloc(struct mlx5_core_dev *mdev,
 					  struct ib_device *ibdev,

Signature · 1 line

2.31.1

Leon Romanovsky

author

2021-09-22 10:39 UTC #54 lore raw

PATCH 6/7 mlx5_vfio_pci: Expose migration commands over mlx5 device

In reply to Leon Romanovsky

From: Yishai Hadas <[email protected]>

Expose migration commands over the device, it includes: suspend, resume, get vhca id, query/save/load state.

As part of this adds the APIs and data structure that are needed to manage the migration data.

Signed-off-by Yishai Hadas <[email protected]>

Signed-off-by Leon Romanovsky <[email protected]>

---
 drivers/vfio/pci/mlx5_vfio_pci_cmd.c | 358 +++++++++++++++++++++++++++
 drivers/vfio/pci/mlx5_vfio_pci_cmd.h |  43 ++++
 2 files changed, 401 insertions(+)
 create mode 100644 drivers/vfio/pci/mlx5_vfio_pci_cmd.c
 create mode 100644 drivers/vfio/pci/mlx5_vfio_pci_cmd.h

drivers/vfio/pci/mlx5_vfio_pci_cmd.c new file +358 -0

new file mode 100644
index 000000000000..7e4f83d196a8
@@ -0,0 +1,358 @@
+// SPDX-License-Identifier: GPL-2.0 OR Linux-OpenIB
+/*
+ * Copyright (c) 2021, NVIDIA CORPORATION & AFFILIATES. All rights reserved
+ */
+
+#include "mlx5_vfio_pci_cmd.h"
+
+int mlx5vf_cmd_suspend_vhca(struct pci_dev *pdev, u16 vhca_id, u16 op_mod)
+{
+	struct mlx5_core_dev *mdev = mlx5_get_core_dev(pci_physfn(pdev));
+	u32 out[MLX5_ST_SZ_DW(suspend_vhca_out)] = {};
+	u32 in[MLX5_ST_SZ_DW(suspend_vhca_in)] = {};
+	int ret;
+
+	if (!mdev)
+		return -ENOTCONN;
+
+	MLX5_SET(suspend_vhca_in, in, opcode, MLX5_CMD_OP_SUSPEND_VHCA);
+	MLX5_SET(suspend_vhca_in, in, vhca_id, vhca_id);
+	MLX5_SET(suspend_vhca_in, in, op_mod, op_mod);
+
+	ret = mlx5_cmd_exec_inout(mdev, suspend_vhca, in, out);
+	mlx5_put_core_dev(mdev);
+	return ret;
+}
+
+int mlx5vf_cmd_resume_vhca(struct pci_dev *pdev, u16 vhca_id, u16 op_mod)
+{
+	struct mlx5_core_dev *mdev = mlx5_get_core_dev(pci_physfn(pdev));
+	u32 out[MLX5_ST_SZ_DW(resume_vhca_out)] = {};
+	u32 in[MLX5_ST_SZ_DW(resume_vhca_in)] = {};
+	int ret;
+
+	if (!mdev)
+		return -ENOTCONN;
+
+	MLX5_SET(resume_vhca_in, in, opcode, MLX5_CMD_OP_RESUME_VHCA);
+	MLX5_SET(resume_vhca_in, in, vhca_id, vhca_id);
+	MLX5_SET(resume_vhca_in, in, op_mod, op_mod);
+
+	ret = mlx5_cmd_exec_inout(mdev, resume_vhca, in, out);
+	mlx5_put_core_dev(mdev);
+	return ret;
+}
+
+int mlx5vf_cmd_query_vhca_migration_state(struct pci_dev *pdev, u16 vhca_id,
+					  u32 *state_size)
+{
+	struct mlx5_core_dev *mdev = mlx5_get_core_dev(pci_physfn(pdev));
+	u32 out[MLX5_ST_SZ_DW(query_vhca_migration_state_out)] = {};
+	u32 in[MLX5_ST_SZ_DW(query_vhca_migration_state_in)] = {};
+	int ret;
+
+	if (!mdev)
+		return -ENOTCONN;
+
+	MLX5_SET(query_vhca_migration_state_in, in, opcode,
+		 MLX5_CMD_OP_QUERY_VHCA_MIGRATION_STATE);
+	MLX5_SET(query_vhca_migration_state_in, in, vhca_id, vhca_id);
+	MLX5_SET(query_vhca_migration_state_in, in, op_mod, 0);
+
+	ret = mlx5_cmd_exec_inout(mdev, query_vhca_migration_state, in, out);
+	if (ret)
+		goto end;
+
+	*state_size = MLX5_GET(query_vhca_migration_state_out, out,
+			       required_umem_size);
+
+end:
+	mlx5_put_core_dev(mdev);
+	return ret;
+}
+
+int mlx5vf_cmd_get_vhca_id(struct pci_dev *pdev, u16 function_id, u16 *vhca_id)
+{
+	struct mlx5_core_dev *mdev = mlx5_get_core_dev(pci_physfn(pdev));
+	u32 in[MLX5_ST_SZ_DW(query_hca_cap_in)] = {};
+	int out_size;
+	void *out;
+	int ret;
+
+	if (!mdev)
+		return -ENOTCONN;
+
+	out_size = MLX5_ST_SZ_BYTES(query_hca_cap_out);
+	out = kzalloc(out_size, GFP_KERNEL);
+	if (!out) {
+		ret = -ENOMEM;
+		goto end;
+	}
+
+	MLX5_SET(query_hca_cap_in, in, opcode, MLX5_CMD_OP_QUERY_HCA_CAP);
+	MLX5_SET(query_hca_cap_in, in, other_function, 1);
+	MLX5_SET(query_hca_cap_in, in, function_id, function_id);
+	MLX5_SET(query_hca_cap_in, in, op_mod,
+		 MLX5_SET_HCA_CAP_OP_MOD_GENERAL_DEVICE << 1 |
+		 HCA_CAP_OPMOD_GET_CUR);
+
+	ret = mlx5_cmd_exec_inout(mdev, query_hca_cap, in, out);
+	if (ret)
+		goto err_exec;
+
+	*vhca_id = MLX5_GET(query_hca_cap_out, out, capability.cmd_hca_cap.vhca_id);
+
+err_exec:
+	kfree(out);
+end:
+	mlx5_put_core_dev(mdev);
+	return ret;
+}
+
+static int _create_state_mkey(struct mlx5_core_dev *mdev, u32 pdn,
+			      struct mlx5_vhca_state_data *state,
+			      struct mlx5_core_mkey *mkey)
+{
+	struct sg_dma_page_iter dma_iter;
+	int err = 0, inlen;
+	__be64 *mtt;
+	void *mkc;
+	u32 *in;
+
+	inlen = MLX5_ST_SZ_BYTES(create_mkey_in) +
+			sizeof(*mtt) * round_up(state->num_pages, 2);
+
+	in = kvzalloc(inlen, GFP_KERNEL);
+	if (!in)
+		return -ENOMEM;
+
+	MLX5_SET(create_mkey_in, in, translations_octword_actual_size,
+		 DIV_ROUND_UP(state->num_pages, 2));
+	mtt = (__be64 *)MLX5_ADDR_OF(create_mkey_in, in, klm_pas_mtt);
+
+	for_each_sgtable_dma_page(&state->mig_data.table.sgt, &dma_iter, 0)
+		*mtt++ = cpu_to_be64(sg_page_iter_dma_address(&dma_iter));
+
+	mkc = MLX5_ADDR_OF(create_mkey_in, in, memory_key_mkey_entry);
+	MLX5_SET(mkc, mkc, access_mode_1_0, MLX5_MKC_ACCESS_MODE_MTT);
+	MLX5_SET(mkc, mkc, lr, 1);
+	MLX5_SET(mkc, mkc, lw, 1);
+	MLX5_SET(mkc, mkc, pd, pdn);
+	MLX5_SET(mkc, mkc, bsf_octword_size, 0);
+	MLX5_SET(mkc, mkc, qpn, 0xffffff);
+	MLX5_SET(mkc, mkc, log_page_size, PAGE_SHIFT);
+	MLX5_SET(mkc, mkc, translations_octword_size,
+		 DIV_ROUND_UP(state->num_pages, 2));
+	MLX5_SET64(mkc, mkc, start_addr, mkey->iova);
+	MLX5_SET64(mkc, mkc, len, state->num_pages * PAGE_SIZE);
+	err = mlx5_core_create_mkey(mdev, mkey, in, inlen);
+
+	kvfree(in);
+
+	return err;
+}
+
+struct page *mlx5vf_get_migration_page(struct migration_data *data,
+				       unsigned long offset)
+{
+	unsigned long cur_offset = 0;
+	struct scatterlist *sg;
+	unsigned int i;
+
+	if (offset < data->last_offset || !data->last_offset_sg) {
+		data->last_offset = 0;
+		data->last_offset_sg = data->table.sgt.sgl;
+		data->sg_last_entry = 0;
+	}
+
+	cur_offset = data->last_offset;
+
+	for_each_sg(data->last_offset_sg, sg,
+			data->table.sgt.orig_nents - data->sg_last_entry, i) {
+		if (offset < sg->length + cur_offset) {
+			data->last_offset_sg = sg;
+			data->sg_last_entry += i;
+			data->last_offset = cur_offset;
+			return nth_page(sg_page(sg),
+					(offset - cur_offset) / PAGE_SIZE);
+		}
+		cur_offset += sg->length;
+	}
+	return NULL;
+}
+
+void mlx5vf_reset_vhca_state(struct mlx5_vhca_state_data *state)
+{
+	struct migration_data *data = &state->mig_data;
+	struct sg_page_iter sg_iter;
+
+	if (!data->table.prv)
+		goto end;
+
+	/* Undo alloc_pages_bulk_array() */
+	for_each_sgtable_page(&data->table.sgt, &sg_iter, 0)
+		__free_page(sg_page_iter_page(&sg_iter));
+	sg_free_append_table(&data->table);
+end:
+	memset(state, 0, sizeof(*state));
+}
+
+int mlx5vf_add_migration_pages(struct mlx5_vhca_state_data *state,
+			       unsigned int npages)
+{
+	unsigned int to_alloc = npages;
+	struct page **page_list;
+	unsigned long filled;
+	unsigned int to_fill;
+	int ret = 0;
+
+	to_fill = min_t(unsigned int, npages, PAGE_SIZE / sizeof(*page_list));
+	page_list = kvzalloc(to_fill * sizeof(*page_list), GFP_KERNEL);
+	if (!page_list)
+		return -ENOMEM;
+
+	do {
+		filled = alloc_pages_bulk_array(GFP_KERNEL, to_fill,
+						page_list);
+		if (!filled) {
+			ret = -ENOMEM;
+			goto err;
+		}
+		to_alloc -= filled;
+		ret = sg_alloc_append_table_from_pages(
+			&state->mig_data.table, page_list, filled, 0,
+			filled << PAGE_SHIFT, UINT_MAX, SG_MAX_SINGLE_ALLOC,
+			GFP_KERNEL);
+
+		if (ret)
+			goto err;
+		/* clean input for another bulk allocation */
+		memset(page_list, 0, filled * sizeof(*page_list));
+		to_fill = min_t(unsigned int, to_alloc,
+				PAGE_SIZE / sizeof(*page_list));
+	} while (to_alloc > 0);
+
+	kvfree(page_list);
+	state->num_pages += npages;
+
+	return 0;
+
+err:
+	kvfree(page_list);
+	return ret;
+}
+
+int mlx5vf_cmd_save_vhca_state(struct pci_dev *pdev, u16 vhca_id,
+			       u64 state_size,
+			       struct mlx5_vhca_state_data *state)
+{
+	struct mlx5_core_dev *mdev = mlx5_get_core_dev(pci_physfn(pdev));
+	u32 out[MLX5_ST_SZ_DW(save_vhca_state_out)] = {};
+	u32 in[MLX5_ST_SZ_DW(save_vhca_state_in)] = {};
+	struct mlx5_core_mkey mkey = {};
+	u32 pdn;
+	int err;
+
+	if (!mdev)
+		return -ENOTCONN;
+
+	err = mlx5_core_alloc_pd(mdev, &pdn);
+	if (err)
+		goto end;
+
+	err = mlx5vf_add_migration_pages(state,
+				DIV_ROUND_UP_ULL(state_size, PAGE_SIZE));
+	if (err < 0)
+		goto err_alloc_pages;
+
+	err = dma_map_sgtable(mdev->device, &state->mig_data.table.sgt,
+			      DMA_FROM_DEVICE, 0);
+	if (err)
+		goto err_reg_dma;
+
+	err = _create_state_mkey(mdev, pdn, state, &mkey);
+	if (err)
+		goto err_create_mkey;
+
+	MLX5_SET(save_vhca_state_in, in, opcode,
+		 MLX5_CMD_OP_SAVE_VHCA_STATE);
+	MLX5_SET(save_vhca_state_in, in, op_mod, 0);
+	MLX5_SET(save_vhca_state_in, in, vhca_id, vhca_id);
+
+	MLX5_SET64(save_vhca_state_in, in, va, mkey.iova);
+	MLX5_SET(save_vhca_state_in, in, mkey, mkey.key);
+	MLX5_SET(save_vhca_state_in, in, size, mkey.size);
+
+	err = mlx5_cmd_exec_inout(mdev, save_vhca_state, in, out);
+	if (err)
+		goto err_exec;
+
+	state->state_size = state_size;
+
+	mlx5_core_destroy_mkey(mdev, &mkey);
+	mlx5_core_dealloc_pd(mdev, pdn);
+	dma_unmap_sgtable(mdev->device, &state->mig_data.table.sgt,
+			  DMA_FROM_DEVICE, 0);
+	mlx5_put_core_dev(mdev);
+
+	return 0;
+
+err_exec:
+	mlx5_core_destroy_mkey(mdev, &mkey);
+err_create_mkey:
+	dma_unmap_sgtable(mdev->device, &state->mig_data.table.sgt,
+			  DMA_FROM_DEVICE, 0);
+err_reg_dma:
+	mlx5vf_reset_vhca_state(state);
+err_alloc_pages:
+	mlx5_core_dealloc_pd(mdev, pdn);
+end:
+	mlx5_put_core_dev(mdev);
+	return err;
+}
+
+int mlx5vf_cmd_load_vhca_state(struct pci_dev *pdev, u16 vhca_id,
+			       struct mlx5_vhca_state_data *state)
+{
+	struct mlx5_core_dev *mdev = mlx5_get_core_dev(pci_physfn(pdev));
+	u32 out[MLX5_ST_SZ_DW(save_vhca_state_out)] = {};
+	u32 in[MLX5_ST_SZ_DW(save_vhca_state_in)] = {};
+	struct mlx5_core_mkey mkey = {};
+	u32 pdn;
+	int err;
+
+	if (!mdev)
+		return -ENOTCONN;
+
+	err = mlx5_core_alloc_pd(mdev, &pdn);
+	if (err)
+		goto end;
+
+	err = dma_map_sgtable(mdev->device, &state->mig_data.table.sgt,
+			      DMA_TO_DEVICE, 0);
+	if (err)
+		goto err_reg;
+
+	err = _create_state_mkey(mdev, pdn, state, &mkey);
+	if (err)
+		goto err_mkey;
+
+	MLX5_SET(load_vhca_state_in, in, opcode,
+		 MLX5_CMD_OP_LOAD_VHCA_STATE);
+	MLX5_SET(load_vhca_state_in, in, op_mod, 0);
+	MLX5_SET(load_vhca_state_in, in, vhca_id, vhca_id);
+	MLX5_SET64(load_vhca_state_in, in, va, mkey.iova);
+	MLX5_SET(load_vhca_state_in, in, mkey, mkey.key);
+	MLX5_SET(load_vhca_state_in, in, size, state->state_size);
+
+	err = mlx5_cmd_exec_inout(mdev, load_vhca_state, in, out);
+	mlx5_core_destroy_mkey(mdev, &mkey);
+err_mkey:
+	dma_unmap_sgtable(mdev->device, &state->mig_data.table.sgt,
+			  DMA_TO_DEVICE, 0);
+err_reg:
+	mlx5_core_dealloc_pd(mdev, pdn);
+end:
+	mlx5_put_core_dev(mdev);
+	return err;
+}

drivers/vfio/pci/mlx5_vfio_pci_cmd.h new file +43 -0

43 lines not on this page · raw message

Signature · 1 line

2.31.1

Alex Williamson

2021-09-28 20:22 UTC #55 lore raw

Re: [PATCH mlx5-next 6/7] mlx5_vfio_pci: Expose migration commands over mlx5 device

Quoted 8 lines · On Wed, 22 Sep 2021 13:38:55 +0300 Leon Romanovsky <[email protected]> wrote:

From: Yishai Hadas <[email protected]>
Expose migration commands over the device, it includes: suspend, resume, get vhca id, query/save/load state.
As part of this adds the APIs and data structure that are needed to manage the migration data.

Quoted 8 lines

Signed-off-by Yishai Hadas <[email protected]>

Signed-off-by Leon Romanovsky <[email protected]>

---
 drivers/vfio/pci/mlx5_vfio_pci_cmd.c | 358 +++++++++++++++++++++++++++
 drivers/vfio/pci/mlx5_vfio_pci_cmd.h |  43 ++++
 2 files changed, 401 insertions(+)
 create mode 100644 drivers/vfio/pci/mlx5_vfio_pci_cmd.c
 create mode 100644 drivers/vfio/pci/mlx5_vfio_pci_cmd.h

Should we set the precedent of a vendor sub-directory like we have elsewhere? Either way I'd like to see a MAINTAINERS file update for the new driver. Thanks,

Alex

Leon Romanovsky

author

2021-09-29 05:36 UTC #56 lore raw

Re: [PATCH mlx5-next 6/7] mlx5_vfio_pci: Expose migration commands over mlx5 device

Quoted 20 lines · Alex Williamson

Quoted 8 lines · On Wed, 22 Sep 2021 13:38:55 +0300 Leon Romanovsky <[email protected]> wrote:
From: Yishai Hadas <[email protected]>
Expose migration commands over the device, it includes: suspend, resume, get vhca id, query/save/load state.
As part of this adds the APIs and data structure that are needed to manage the migration data.
Quoted 8 lines
Signed-off-by Yishai Hadas <[email protected]>
Signed-off-by Leon Romanovsky <[email protected]>
---
 drivers/vfio/pci/mlx5_vfio_pci_cmd.c | 358 +++++++++++++++++++++++++++
 drivers/vfio/pci/mlx5_vfio_pci_cmd.h |  43 ++++
 2 files changed, 401 insertions(+)
 create mode 100644 drivers/vfio/pci/mlx5_vfio_pci_cmd.c
 create mode 100644 drivers/vfio/pci/mlx5_vfio_pci_cmd.h

Quoted 3 lines

Should we set the precedent of a vendor sub-directory like we have elsewhere? Either way I'd like to see a MAINTAINERS file update for the new driver. Thanks,

I would like to see subfolders, because all these vendor_xxxx.c filenames look awful to me.

Thanks

Quoted 3 lines

Alex

Leon Romanovsky

author

2021-09-22 10:39 UTC #57 lore raw

PATCH 7/7 mlx5_vfio_pci: Implement vfio_pci driver for mlx5 devices

In reply to Leon Romanovsky

From: Yishai Hadas <[email protected]>

This patch adds support for vfio_pci driver for mlx5 devices.

It uses vfio_pci_core to register to the VFIO subsystem and then implements the mlx5 specific logic in the migration area.

The migration implementation follows the definition from uapi/vfio.h and uses the mlx5 VF->PF command channel to achieve it.

This patch implements the suspend/resume flows.

Signed-off-by Yishai Hadas <[email protected]>

Signed-off-by Leon Romanovsky <[email protected]>

---
 drivers/vfio/pci/Kconfig         |  11 +
 drivers/vfio/pci/Makefile        |   3 +
 drivers/vfio/pci/mlx5_vfio_pci.c | 736 +++++++++++++++++++++++++++++++
 3 files changed, 750 insertions(+)
 create mode 100644 drivers/vfio/pci/mlx5_vfio_pci.c

drivers/vfio/pci/Kconfig +11 -0

index 860424ccda1b..c10b53028309 100644
@@ -43,4 +43,15 @@ config VFIO_PCI_IGD
 
 	  To enable Intel IGD assignment through vfio-pci, say Y.
 endif
+
+config MLX5_VFIO_PCI
+	tristate "VFIO support for MLX5 PCI devices"
+	depends on MLX5_CORE
+	select VFIO_PCI_CORE
+	help
+	  This provides a PCI support for MLX5 devices using the VFIO
+	  framework. The device specific driver supports suspend/resume
+	  of the MLX5 device.
+
+	  If you don't know what to do here, say N.
 endif

drivers/vfio/pci/Makefile +3 -0

index 349d68d242b4..b9448bba0c83 100644
@@ -7,3 +7,6 @@ obj-$(CONFIG_VFIO_PCI_CORE) += vfio-pci-core.o
 vfio-pci-y := vfio_pci.o
 vfio-pci-$(CONFIG_VFIO_PCI_IGD) += vfio_pci_igd.o
 obj-$(CONFIG_VFIO_PCI) += vfio-pci.o
+
+mlx5-vfio-pci-y := mlx5_vfio_pci.o mlx5_vfio_pci_cmd.o
+obj-$(CONFIG_MLX5_VFIO_PCI) += mlx5-vfio-pci.o

drivers/vfio/pci/mlx5_vfio_pci.c new file +736 -0

736 lines not on this page · raw message

Signature · 1 line

2.31.1