Re: [PATCH v2] vfio/migration: Refactor and fix print of "Migration disabled"

[Joao Martins]

On 20/06/2023 09:55, Duan, Zhenzhong wrote:
>> -----Original Message-----
>> From: Joao Martins <joao.m.martins@oracle.com>
>> Sent: Tuesday, June 20, 2023 4:23 PM
>> To: Duan, Zhenzhong <zhenzhong.duan@intel.com>; Avihai Horon
>> <avihaih@nvidia.com>; qemu-devel@nongnu.org
>> Cc: alex.williamson@redhat.com; clg@redhat.com; Peng, Chao P
>> <chao.p.peng@intel.com>
>> Subject: Re: [PATCH v2] vfio/migration: Refactor and fix print of "Migration
>> disabled"
>>
>> On 20/06/2023 04:04, Duan, Zhenzhong wrote:
>>>> -----Original Message-----
>>>> From: Avihai Horon <avihaih@nvidia.com>
>>>> Sent: Monday, June 19, 2023 7:14 PM
>>> ...
>>>>> a/hw/vfio/migration.c b/hw/vfio/migration.c index
>>>>> 6b58dddb8859..bc51aa765cb8 100644
>>>>> --- a/hw/vfio/migration.c
>>>>> +++ b/hw/vfio/migration.c
>>>>> @@ -632,42 +632,41 @@ int64_t vfio_mig_bytes_transferred(void)
>>>>>       return bytes_transferred;
>>>>>   }
>>>>>
>>>>> -int vfio_migration_realize(VFIODevice *vbasedev, Error **errp)
>>>>> +bool vfio_migration_realize(VFIODevice *vbasedev, Error **errp)
>>>>>   {
>>>>> -    int ret = -ENOTSUP;
>>>>> +    int ret;
>>>>>
>>>>> -    if (!vbasedev->enable_migration) {
>>>>> +    if (!vbasedev->enable_migration || vfio_migration_init(vbasedev)) {
>>>>> +        error_setg(&vbasedev->migration_blocker,
>>>>> +                   "VFIO device doesn't support migration");
>>>>>           goto add_blocker;
>>>>>       }
>>>>>
>>>>> -    ret = vfio_migration_init(vbasedev);
>>>>> -    if (ret) {
>>>>> +    if (vfio_block_multiple_devices_migration(errp)) {
>>>>> +        error_setg(&vbasedev->migration_blocker,
>>>>> +                   "Migration is currently not supported with multiple "
>>>>> +                   "VFIO devices");
>>>>>           goto add_blocker;
>>>>>       }
>>>>
>>>> Here you are tying the multiple devices blocker to a specific device.
>>>> This could be problematic:
>>>> If you add vfio device #1 and then device #2 then the blocker will be
>>>> added to device #2. If you then remove device #1, migration will
>>>> still be blocked although it shouldn't.
>>>>
>>>> I think we should keep it as a global blocker and not a per-device blocker.
>>>
>>> Thanks for point out, you are right, seems I need to restore the multiple
>> devices part code.
>>
>> It's the same for vIOMMU migration blocker. You could have a machine with
>> default_bus_bypass_iommu=on and add device #1 with bypass_iommu=off
>> attribute in pxb PCI port, and then add device #2 with bypass_iommu=on. The
>> blocker is added because of device #1 but then it will remain blocked if you
>> remove it.
> 
> Right, thanks for point out, I'm thinking about changing vfio_viommu_preset()
> to check corresponding device's address space rather than all vfio devices'.
> 
> Let me know if you prefer to restore vIOMMU blocker as global too, then I'll 
> not
> try with my idea furtherly.

The vIOMMU migration blocker doesn't need to be global, true, as it doesn't care
about others address space -- if each device has a blocker as long as the one
device blocker is removed it should become make VM migratable again (but atm we
will be blocked by the multi device blocker anyway). This should consolidate
things into a single migration blocker and avoid the special path. I am not
enterily sure if the refactor will give *that* much gain but that's probably
because I haven't seen the final result.

IIUC the problem with this patch is that you remove what unblocks the migration,
and I guess that need to stay there for the global case.