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]

Date:   Wed, 20 Oct 2021 13:25:57 -0300
From:   Jason Gunthorpe <jgg@...dia.com>
To:     Yishai Hadas <yishaih@...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 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