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

[Duan, Zhenzhong]

>-----Original Message-----
>From: Avihai Horon <avihaih@nvidia.com>
>Sent: Monday, June 26, 2023 5:35 PM
>To: Duan, Zhenzhong <zhenzhong.duan@intel.com>; qemu-
>devel@nongnu.org
>Cc: alex.williamson@redhat.com; clg@redhat.com; Martins, Joao
><joao.m.martins@oracle.com>; Peng, Chao P <chao.p.peng@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.duan@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