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

[Avihai Horon]

On 30/08/2023 12:53, Cédric Le Goater wrote:
> External email: Use caution opening links or attachments
>
>
> On 8/30/23 11:21, Avihai Horon wrote:
> > On 30/08/2023 11:37, Cédric Le Goater wrote:
> > > External email: Use caution opening links or attachments
> > >
> > >
> > > On 8/30/23 09:01, Avihai Horon wrote:
> > > > On 29/08/2023 21:27, Peter Xu wrote:
> > > > > External email: Use caution opening links or attachments
> > > > >
> > > > >
> > > > > 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.
> > > >
> > > > Yes, I agree with you.
> > > >
> > > > > 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.
> > > >
> > > > Sure.
> > > > So I will do the checking in vfio_save_setup(), unless someone else 
> > > > has a better idea.
> > >
> > > Just to recap for my understanding,
> > >
> > > vfio_save_setup() would test migrate_postcopy_ram() and update a new
> > > 'Error *err' parameter of the .save_setup() op which would be taken
> > > into account in qemu_savevm_state_setup(). Is that correct ?
> > Yes.
> > But I wonder if it would be simpler to call migrate_set_error() 
> > directly from vfio_save_setup() instead of adding "Error *err" 
> > argument to .save_setup() and changing all other users.
> > What do you prefer?
>
> Well, with my downstreamer hat, I would prefer a simpler solution for the
> VFIO postcopy limitation first. That said, there is value in adding
> a 'Error *' parameter to the .save_setup() op and letting the top routine
> qemu_savevm_state_setup() propagate. Other SaveVMhandler could start 
> using
> it. even VFIO has multiple error_report() in vfio_save_setup() which 
> could
> be propagated to the top callers.
>
> Let's try that first. I will check your new series on top of 8.0
OK, so just making sure, you want to add "Error *err" argument to 
.save_setup() first and see how it goes, right?

Thanks.