>-----Original Message----- >From: Joao Martins <joao.m.mart...@oracle.com> >Sent: Tuesday, June 20, 2023 5:28 PM >To: Duan, Zhenzhong <zhenzhong.d...@intel.com>; Avihai Horon ><avih...@nvidia.com>; qemu-devel@nongnu.org >Cc: alex.william...@redhat.com; c...@redhat.com; Peng, Chao P ><chao.p.p...@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.mart...@oracle.com> >>> Sent: Tuesday, June 20, 2023 4:23 PM >>> To: Duan, Zhenzhong <zhenzhong.d...@intel.com>; Avihai Horon >>> <avih...@nvidia.com>; qemu-devel@nongnu.org >>> Cc: alex.william...@redhat.com; c...@redhat.com; Peng, Chao P >>> <chao.p.p...@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 <avih...@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