On Fri, 15 Oct 2021 16:59:37 -0300
Jason Gunthorpe [off-list ref] wrote:

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

quoted

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.

if (!VFIO_DEVICE_STATE_VALID(old_state) || !VFIO_DEVICE_STATE_VALID(state)) || (state & ~VFIO_DEVICE_STATE_MASK))

old_state is controlled by the driver and can never have random bits
set, user state should be sanitized to prevent setting undefined bits.

quoted

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

That's why I inserted my comment after the "it changed" test and not
after the "and the old old value was..." test below.

quoted

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

quoted

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.

Fair enough.  Thanks,

Alex