Re: [PATCH V6 mlx5-next 09/15] vfio: Extend the device migration protocol with RUNNING_P2P

[Jason Gunthorpe <jgg@nvidia.com>]

On Tue, Feb 01, 2022 at 12:13:22PM -0700, Alex Williamson wrote:
> On Tue, 1 Feb 2022 14:53:21 -0400
> Jason Gunthorpe <jgg@nvidia.com> wrote:
> 
> > On Tue, Feb 01, 2022 at 11:31:44AM -0700, Alex Williamson wrote:
> > > > +	bool have_p2p = device->migration_flags & VFIO_MIGRATION_P2P;
> > > > +
> > > >  	if (cur_fsm >= ARRAY_SIZE(vfio_from_fsm_table) ||
> > > >  	    new_fsm >= ARRAY_SIZE(vfio_from_fsm_table))
> > > >  		return VFIO_DEVICE_STATE_ERROR;
> > > >  
> > > > -	return vfio_from_fsm_table[cur_fsm][new_fsm];
> > > > +	if (!have_p2p && (new_fsm == VFIO_DEVICE_STATE_RUNNING_P2P ||
> > > > +			  cur_fsm == VFIO_DEVICE_STATE_RUNNING_P2P))
> > > > +		return VFIO_DEVICE_STATE_ERROR;  
> > > 
> > > new_fsm is provided by the user, we pass set_state.device_state
> > > directly to .migration_set_state.  We should do bounds checking and
> > > compatibility testing on the end state in the core so that we can  
> > 
> > This is the core :)
> 
> But this is the wrong place, we need to do it earlier rather than when
> we're already iterating next states.  I only mention core to avoid that
> I'm suggesting a per driver responsibility.

Only the first vfio_mig_get_next_state() can return ERROR, once it
succeeds the subsequent ones must also succeed.

This is the earliest can be. It is done directly after taking the lock
that allows us to read the current state to call this function to
determine if the requested transition is acceptable.

> > Userspace can never put the device into error. As the function comment
> > says VFIO_DEVICE_STATE_ERROR is returned to indicate the arc is not
> > permitted. The driver is required to reflect that back as an errno
> > like mlx5 shows:
> > 
> > +		next_state = vfio_mig_get_next_state(vdev, mvdev->mig_state,
> > +						     new_state);
> > +		if (next_state == VFIO_DEVICE_STATE_ERROR) {
> > +			res = ERR_PTR(-EINVAL);
> > +			break;
> > +		}
> > 
> > We never get the driver into error, userspaces gets an EINVAL and no
> > change to the device state.
> 
> Hmm, subtle.  I'd argue that if we do a bounds and support check of the
> end state in vfio_ioctl_mig_set_state() before calling
> .migration_set_state() then we could remove ERROR from
> vfio_from_fsm_table[] altogether and simply begin
> vfio_mig_get_next_state() with:

Then we can't reject blocked arcs like STOP_COPY -> PRE_COPY.

It is setup this way to allow the core code to assert all policy, not
just a simple validation of the next_fsm.

> Then we only get to ERROR by the driver placing us in ERROR and things
> feel a bit more sane to me.

This is already true.

Perhaps it is confusing using ERROR to indicate that
vfio_mig_get_next_state() failed. Would you be happier with a -errno
return?

> > It is organized this way because the driver controls the locking for
> > its current state and thus the core code caller along the ioctl path
> > cannot validate the arc before passing it to the driver. The code is
> > shared by having the driver callback to the core to validate the
> > entire fsm arc under its lock.
> 
> P2P is defined in a way that if the endpoint is valid then the arc is
> valid.  We skip intermediate unsupported states.  We need to do that
> for compatibility.  So why do we care about driver locking to do
> that?

Without the driver locking we can't identify the arc because we don't
know the curent state the driver is in. We only know the target
state.

Jason