On 10/20/2021 7:25 PM, Jason Gunthorpe wrote:

On Wed, Oct 20, 2021 at 11:01:01AM +0300, Yishai Hadas wrote:

quoted

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

quoted

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 layout of the if blocks must follow a logical progression of
the actions toward the chip:

  start precopy tracking
  quiesce
  freeze
  stop precopy tracking
  save state to buffer
  clear state buffer
  load state from buffer
  unfreeze
  unquiesce

The order of the if blocks sets the precendence of the requested
actions, because the bit-field view means userspace can request
multiple actions concurrently.

When adding the precopy actions into the above list we can see that
the current patches ordering is backwards, save/load should be
swapped.

Idiomatically each action needs a single edge triggred predicate
listed in the right order.

The predicates we define here will become the true ABI for qemu to
follow to operate the HW

Also, the ordering of the predicates influences what usable state
transitions exist.

So, it is really important to get this right.

quoted

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.

quoted

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.

Like this:

if ((flipped_bits & (RUNNING | SAVING)) &&
      ((old_state & (RUNNING | SAVING)) == (RUNNING|SAVING))
     /* enter pre-copy state */

if ((flipped_bits & (RUNNING | SAVING)) &&
      ((old_state & (RUNNING | SAVING)) != (RUNNING|SAVING))
     /* exit pre-copy state */

if ((flipped_bits & (RUNNING | SAVING)) &&
      ((old_state & (RUNNING | SAVING)) == SAVING))
     mlx5vf_pci_save_device_data()

Jason

OK, V3 will follow that.

Note: In the above old_state needs to be the new state.

Thanks,
Yishai