On Fri, Oct 15, 2021 at 01:48:20PM -0600, Alex Williamson wrote:

quoted

+static int mlx5vf_pci_set_device_state(struct mlx5vf_pci_core_device *mvdev,
+				       u32 state)
+{
+	struct mlx5vf_pci_migration_info *vmig = &mvdev->vmig;
+	u32 old_state = vmig->vfio_dev_state;
+	int ret = 0;
+
+	if (vfio_is_state_invalid(state) || vfio_is_state_invalid(old_state))
+		return -EINVAL;

if (!VFIO_DEVICE_STATE_VALID(old_state) || !VFIO_DEVICE_STATE_VALID(state))

AFAICT this macro doesn't do what is needed, eg

VFIO_DEVICE_STATE_VALID(0xF000) == true

What Yishai implemented is at least functionally correct - states this
driver does not support are rejected.

quoted

+	/* Running switches off */
+	if ((old_state & VFIO_DEVICE_STATE_RUNNING) !=
+	    (state & VFIO_DEVICE_STATE_RUNNING) &&

((old_state ^ state) & VFIO_DEVICE_STATE_RUNNING) ?

It is not functionally the same, xor only tells if the bit changed, it
doesn't tell what the current value is, and this needs to know that it
changed to 1

quoted

+	    (old_state & VFIO_DEVICE_STATE_RUNNING)) {
+		ret = mlx5vf_pci_quiesce_device(mvdev);
+		if (ret)
+			return ret;
+		ret = mlx5vf_pci_freeze_device(mvdev);
+		if (ret) {
+			vmig->vfio_dev_state = VFIO_DEVICE_STATE_INVALID;

No, the invalid states are specifically unreachable, the uAPI defines
the error state for this purpose.

Indeed

The states noted as invalid in the
uAPI should be considered reserved at this point.  If only there was a
macro to set an error state... ;)

It should just assign a constant value, there is only one error state.

Jason