Re: [PATCH V2 mlx5-next 12/14] vfio/mlx5: Implement vfio_pci driver for mlx5 devices

[Yishai Hadas <yishaih@nvidia.com>]

On 10/19/2021 9:43 PM, Alex Williamson wrote:
>
>> +
>> +	/* 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.

>> +
>> +	/* 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