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
