On 10/19/2021 9:43 PM, Alex Williamson wrote:

quoted

+
+	/* Resuming switches off */
+	if (((old_state ^ state) & VFIO_DEVICE_STATE_RESUMING) &&
+	    (old_state & VFIO_DEVICE_STATE_RESUMING)) {
+		/* deserialize state into the device */
+		ret = mlx5vf_load_state(mvdev);
+		if (ret) {
+			vmig->vfio_dev_state = VFIO_DEVICE_STATE_ERROR;
+			return ret;
+		}
+	}
+
+	/* Resuming switches on */
+	if (((old_state ^ state) & VFIO_DEVICE_STATE_RESUMING) &&
+	    (state & VFIO_DEVICE_STATE_RESUMING)) {
+		mlx5vf_reset_mig_state(mvdev);
+		ret = mlx5vf_pci_new_write_window(mvdev);
+		if (ret)
+			return ret;
+	}

A couple nits here...

Perhaps:

	if ((old_state ^ state) & VFIO_DEVICE_STATE_RESUMING)) {
		/* Resuming bit cleared */
		if (old_state & VFIO_DEVICE_STATE_RESUMING) {
			...
		} else { /* Resuming bit set */
			...
		}
	}

  I tried to avoid nested 'if's as of some previous notes.

The 'resuming' two cases are handled already above so functional wise 
the code covers this.

Jason/Alex,

Please recommend what is the preferred way, both options seems to be 
fine for me.

Also

	u32 flipped_bits = old_state ^ state;

or similar would simplify all these cases slightly.

Sure, will use it in V3.

quoted

+
+	/* Saving switches on */
+	if (((old_state ^ state) & VFIO_DEVICE_STATE_SAVING) &&
+	    (state & VFIO_DEVICE_STATE_SAVING)) {
+		if (!(state & VFIO_DEVICE_STATE_RUNNING)) {
+			/* serialize post copy */
+			ret = mlx5vf_pci_save_device_data(mvdev);
+			if (ret)
+				return ret;
+		}
+	}

This doesn't catch all the cases, and in fact misses the most expected
case where userspace clears the _RUNNING bit while _SAVING is already
enabled.  Does that mean this hasn't actually been tested with QEMU?

I run QEMU with 'x-pre-copy-dirty-page-tracking=off' as current driver 
doesn't support dirty-pages.

As so, it seems that this flow wasn't triggered by QEMU in my save/load 
test.

It seems like there also needs to be a clause in the case where
_RUNNING switches off to test if _SAVING is already set and has not
toggled.

This can be achieved by adding the below to current code, this assumes 
that we are fine with nested 'if's coding.

Seems OK ?

@@ -269,6 +269,7 @@ static int mlx5vf_pci_set_device_state(struct 

mlx5vf_pci_core_device *mvdev,
  {
         struct mlx5vf_pci_migration_info *vmig = &mvdev->vmig;
         u32 old_state = vmig->vfio_dev_state;
+       u32 flipped_bits = old_state ^ state;
         int ret = 0;

         if (old_state == VFIO_DEVICE_STATE_ERROR ||

@@ -277,7 +278,7 @@ static int mlx5vf_pci_set_device_state(struct 

mlx5vf_pci_core_device *mvdev,
                 return -EINVAL;

         /* Running switches off */
-       if (((old_state ^ state) & VFIO_DEVICE_STATE_RUNNING) &&
+       if ((flipped_bits & VFIO_DEVICE_STATE_RUNNING) &&
             (old_state & VFIO_DEVICE_STATE_RUNNING)) {
                 ret = mlx5vf_pci_quiesce_device(mvdev);
                 if (ret)

@@ -287,10 +288,18 @@ static int mlx5vf_pci_set_device_state(struct 

mlx5vf_pci_core_device *mvdev,
                         vmig->vfio_dev_state = VFIO_DEVICE_STATE_ERROR;
                         return ret;
                 }
+               if (state & VFIO_DEVICE_STATE_SAVING) {
+                       /* serialize post copy */
+                       ret = mlx5vf_pci_save_device_data(mvdev);
+                       if (ret) {
+                               vmig->vfio_dev_state = 
VFIO_DEVICE_STATE_ERROR;
+                               return ret;
+                       }
+               }
         }


Yishai