>-----Original Message-----
>From: Avihai Horon <avih...@nvidia.com>
>Sent: Monday, June 26, 2023 5:35 PM
>To: Duan, Zhenzhong <zhenzhong.d...@intel.com>; qemu-
>de...@nongnu.org
>Cc: alex.william...@redhat.com; c...@redhat.com; Martins, Joao
><joao.m.mart...@oracle.com>; Peng, Chao P <chao.p.p...@intel.com>
>Subject: Re: [PATCH v3 3/3] vfio/migration: vfio/migration: Refactor and fix
>print of "Migration disabled"
>
>
>On 21/06/2023 11:02, Zhenzhong Duan wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> This patch refactors vfio_migration_realize() and its dependend code
>> as follows:
>>
>> 1. It's redundant in vfio_migration_realize() to registers multiple blockers,
>> e.g: vIOMMU blocker can be refactored as per device blocker.
>> 2. Change vfio_block_giommu_migration() to be only a per device checker.
>> 3. Remove global vIOMMU blocker related stuff, e.g:
>> giommu_migration_blocker, vfio_unblock_giommu_migration(),
>> vfio_viommu_preset() and vfio_migration_finalize() 4. Change
>> vfio_migration_realize() and dependent vfio_block_*_migration() to
>> return bool type.
>> 5. Change to print "Migration disabled" only after adding blocker succeed.
>> 6. Add device name to errp so "info migrate" could be more informative.
>>
>> migrate_add_blocker() returns 0 when successfully adding the migration
>blocker.
>> However, the caller of vfio_migration_realize() considers that
>> migration was blocked when the latter returned an error. What matters
>> for migration is that the blocker is added in core migration, so this
>> cleans up usability such that user sees "Migrate disabled" when any of the
>vfio migration blockers are active.
>>
>> Signed-off-by: Zhenzhong Duan <zhenzhong.d...@intel.com>
>> ---
>> hw/vfio/common.c | 54 +++++------------------------------
>> hw/vfio/migration.c | 37 +++++++++++-------------
>> hw/vfio/pci.c | 4 +--
>> include/hw/vfio/vfio-common.h | 7 ++---
>> 4 files changed, 29 insertions(+), 73 deletions(-)
>>
>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c index
>> fa8fd949b1cf..cc5f4e805341 100644
>> --- a/hw/vfio/common.c
>> +++ b/hw/vfio/common.c
>> @@ -362,8 +362,6 @@ bool vfio_mig_active(void)
>> }
>>
>> static Error *multiple_devices_migration_blocker;
>> -static Error *giommu_migration_blocker;
>> -
>> static unsigned int vfio_migratable_device_num(void)
>> {
>> VFIOGroup *group;
>> @@ -381,13 +379,13 @@ static unsigned int
>vfio_migratable_device_num(void)
>> return device_num;
>> }
>>
>> -int vfio_block_multiple_devices_migration(Error **errp)
>> +bool vfio_block_multiple_devices_migration(Error **errp)
>> {
>> int ret;
>>
>> if (multiple_devices_migration_blocker ||
>> vfio_migratable_device_num() <= 1) {
>> - return 0;
>> + return true;
>> }
>>
>> error_setg(&multiple_devices_migration_blocker,
>> @@ -397,9 +395,11 @@ int vfio_block_multiple_devices_migration(Error
>**errp)
>> if (ret < 0) {
>> error_free(multiple_devices_migration_blocker);
>> multiple_devices_migration_blocker = NULL;
>> + } else {
>> + error_report("Migration disabled, not support multiple VFIO
>> + devices");
>> }
>>
>> - return ret;
>> + return !ret;
>> }
>>
>> void vfio_unblock_multiple_devices_migration(void)
>> @@ -414,49 +414,9 @@ void vfio_unblock_multiple_devices_migration(void)
>> multiple_devices_migration_blocker = NULL;
>> }
>>
>> -static bool vfio_viommu_preset(void)
>> -{
>> - VFIOAddressSpace *space;
>> -
>> - QLIST_FOREACH(space, &vfio_address_spaces, list) {
>> - if (space->as != &address_space_memory) {
>> - return true;
>> - }
>> - }
>> -
>> - return false;
>> -}
>> -
>> -int vfio_block_giommu_migration(Error **errp) -{
>> - int ret;
>> -
>> - if (giommu_migration_blocker ||
>> - !vfio_viommu_preset()) {
>> - return 0;
>> - }
>> -
>> - error_setg(&giommu_migration_blocker,
>> - "Migration is currently not supported with vIOMMU enabled");
>> - ret = migrate_add_blocker(giommu_migration_blocker, errp);
>> - if (ret < 0) {
>> - error_free(giommu_migration_blocker);
>> - giommu_migration_blocker = NULL;
>> - }
>> -
>> - return ret;
>> -}
>> -
>> -void vfio_migration_finalize(void)
>> +bool vfio_block_giommu_migration(VFIODevice *vbasedev)
>> {
>> - if (!giommu_migration_blocker ||
>> - vfio_viommu_preset()) {
>> - return;
>> - }
>> -
>> - migrate_del_blocker(giommu_migration_blocker);
>> - error_free(giommu_migration_blocker);
>> - giommu_migration_blocker = NULL;
>> + return vbasedev->group->container->space->as !=
>> + &address_space_memory;
>> }
>
>I guess vfio_block_giommu_migration can be moved to migration.c and made
>static?
>Although Joao's vIOMMU series will probably change that back later in some
>way.
Good idea, will do.
>
>>
>> static void vfio_set_migration_error(int err)
>> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
>> index 6b58dddb8859..7621074f156d 100644
>> --- a/hw/vfio/migration.c
>> +++ b/hw/vfio/migration.c
>> @@ -632,42 +632,39 @@ int64_t vfio_mig_bytes_transferred(void)
>> return bytes_transferred;
>> }
>>
>> -int vfio_migration_realize(VFIODevice *vbasedev, Error **errp)
>> +/* Return true when either migration initialized or blocker registered */
>> +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 %s doesn't support migration",
>> vbasedev->name);
>> goto add_blocker;
>> }
>>
>> - ret = vfio_migration_init(vbasedev);
>> - if (ret) {
>> - goto add_blocker;
>> - }
>> -
>> - ret = vfio_block_multiple_devices_migration(errp);
>> - if (ret) {
>> - return ret;
>> + if (!vfio_block_multiple_devices_migration(errp)) {
>> + return false;
>> }
>>
>> - ret = vfio_block_giommu_migration(errp);
>> - if (ret) {
>> - return ret;
>> + if (vfio_block_giommu_migration(vbasedev)) {
>> + error_setg(&vbasedev->migration_blocker,
>> + "Migration is currently not supported on %s "
>> + "with vIOMMU enabled", vbasedev->name);
>> + goto add_blocker;
>> }
>>
>> - trace_vfio_migration_probe(vbasedev->name);
>> - return 0;
>> + return true;
>>
>> add_blocker:
>> - error_setg(&vbasedev->migration_blocker,
>> - "VFIO device doesn't support migration");
>> -
>> ret = migrate_add_blocker(vbasedev->migration_blocker, errp);
>> if (ret < 0) {
>> error_free(vbasedev->migration_blocker);
>> vbasedev->migration_blocker = NULL;
>> + } else {
>> + error_report("%s: Migration disabled", vbasedev->name);
>
>When x-enable-migration=off, "Migration disabled" error will be printed,
>but this is the expected behavior, so we should not print it in this case.
Make sense, will do.
>
>> }
>> - return ret;
>> + return !ret;
>> }
>>
>> void vfio_migration_exit(VFIODevice *vbasedev)
>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>> index 82c4cf4f7609..061ca96cbce2 100644
>> --- a/hw/vfio/pci.c
>> +++ b/hw/vfio/pci.c
>> @@ -3209,7 +3209,8 @@ static void vfio_realize(PCIDevice *pdev, Error
>**errp)
>> if (!pdev->failover_pair_id) {
>> ret = vfio_migration_realize(vbasedev, errp);
>> if (ret) {
>> - error_report("%s: Migration disabled", vbasedev->name);
>> + trace_vfio_migration_probe(vbasedev->name);
>
>While we are here, let's rename trace_vfio_migration_probe() to
>trace_vfio_migration_realize() (I can do it too in my series).
Good finding, will do.
Thanks
Zhenzhong
