netdev - Re: [PATCH V2 mlx5-next 12/14] vfio/mlx5: Implement vfio

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <95de8964-af56-7ce0-d79b-20d9f75a292d@nvidia.com>
Date:   Thu, 21 Oct 2021 13:46:23 +0300
From:   Yishai Hadas <yishaih@...dia.com>
To:     Jason Gunthorpe <jgg@...dia.com>
CC:     Alex Williamson <alex.williamson@...hat.com>,
        <bhelgaas@...gle.com>, <saeedm@...dia.com>,
        <linux-pci@...r.kernel.org>, <kvm@...r.kernel.org>,
        <netdev@...r.kernel.org>, <kuba@...nel.org>, <leonro@...dia.com>,
        <kwankhede@...dia.com>, <mgurtovoy@...dia.com>, <maorg@...dia.com>
Subject: Re: [PATCH V2 mlx5-next 12/14] vfio/mlx5: Implement vfio_pci driver
 for mlx5 devices

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