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

[Duan, Zhenzhong]

>-----Original Message-----
>From: Joao Martins <joao.m.martins@oracle.com>
>Sent: Tuesday, June 20, 2023 5:28 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 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.

OK, let me write one for discuss, having per device vIOMMU blocker, global 
multiple devices blocker, etc.
>
>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.
Yes.

Thanks
Zhenzhong