Re: [PATCH 5/6] vfio/migration: Block VFIO migration with postcopy migration

[Peter Xu <peterx@redhat.com>]

On Tue, Aug 29, 2023 at 07:20:47PM +0300, Avihai Horon wrote:
> 
> On 29/08/2023 17:53, Peter Xu wrote:
> > External email: Use caution opening links or attachments
> > 
> > 
> > On Mon, Aug 28, 2023 at 06:18:41PM +0300, Avihai Horon wrote:
> > > diff --git a/migration/options.c b/migration/options.c
> > > index 1d1e1321b0..e201053563 100644
> > > --- a/migration/options.c
> > > +++ b/migration/options.c
> > > @@ -499,6 +499,11 @@ bool migrate_caps_check(bool *old_caps, bool *new_caps, Error **errp)
> > >               error_setg(errp, "Postcopy is not yet compatible with multifd");
> > >               return false;
> > >           }
> > > +
> > > +        if (migration_vfio_mig_active()) {
> > > +            error_setg(errp, "Postcopy is not compatible with VFIO migration");
> > > +            return false;
> > > +        }
> > Hmm.. this will add yet another vfio hard-coded line into migration/..
> > 
> > What will happen if the vfio device is hot plugged after enabling
> > postcopy-ram here?
> 
> In that case a migration blocker will be added.
> 
> > 
> > Is it possible to do it in a generic way?
> 
> What comes to my mind is to let devices register a handler for a "caps
> change" notification and allow them to object.
> But maybe that's a bit of an overkill.

This one also sounds better than hard-codes to me.

> 
> > 
> > I was thinking the only unified place to do such check is when migration
> > starts, as long as we switch to SETUP all caps are locked and doesn't allow
> > any change until it finishes or fails.
> > 
> > So, can we do this check inside vfio_save_setup(), allow vfio_save_setup()
> > to fail the whole migration early?  For example, maybe we should have an
> > Error** passed in, then if it fails it calls migrate_set_error, so
> > reflected in query-migrate later too.
> 
> Yes, I think this could work and it will simplify things because we could
> also drop the VFIO migration blockers code.
> The downside is that the user will know migration is blocked only when he
> tries to migrate, and migrate_caps_check() will not block setting postcopy
> when a VFIO device is already attached.
> I don't have a strong opinion here, so if it's fine by you and everyone
> else, I could change that to what you suggested.

Failing later would be fine in this case to me; my expectation is VFIO
users should be advanced already anyway (as the whole solution is still
pretty involved comparing to a generic VM migration) and shouldn't try to
trigger that at all in real life.  IOW I'd expect this check will be there
just for sanity, rather than being relied on to let people be aware of it
by the error message.

Meanwhile the blocker + caps check is slightly complicated to me to guard
both sides.  So I'd vote for failing at the QMP command.  But we can wait
and see whether there's other votes.

Thanks,

-- 
Peter Xu