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

["Cédric Le Goater" <clg@redhat.com>]

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

Thanks,

C.


> 
> Thanks.
>