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

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

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 ?


Thanks,

C.