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

From: Yishai Hadas <yishaih@nvidia.com>
To: Jason Gunthorpe <jgg@nvidia.com>
Cc: Alex Williamson <alex.williamson@redhat.com>,
	<bhelgaas@google.com>, <saeedm@nvidia.com>,
	<linux-pci@vger.kernel.org>, <kvm@vger.kernel.org>,
	<netdev@vger.kernel.org>, <kuba@kernel.org>, <leonro@nvidia.com>,
	<kwankhede@nvidia.com>, <mgurtovoy@nvidia.com>,
	<maorg@nvidia.com>
Subject: Re: [PATCH V2 mlx5-next 12/14] vfio/mlx5: Implement vfio_pci driver for mlx5 devices
Date: Thu, 21 Oct 2021 13:46:23 +0300	[thread overview]
Message-ID: <95de8964-af56-7ce0-d79b-20d9f75a292d@nvidia.com> (raw)
In-Reply-To: <20211020162557.GF2744544@nvidia.com>

On 10/20/2021 7:25 PM, Jason Gunthorpe wrote:
> On Wed, Oct 20, 2021 at 11:01:01AM +0300, Yishai Hadas wrote:
>> 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 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.
>
>> 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.
> 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