Re: [PATCH v9 07/14] vfio/migration: Block multiple devices migration

["Cédric Le Goater" <clg@redhat.com>]

On 2/8/23 14:08, Avihai Horon wrote:
> 
> On 08/02/2023 0:34, Alex Williamson wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> On Mon, 6 Feb 2023 14:31:30 +0200
>> Avihai Horon <avihaih@nvidia.com> wrote:
>>
>>> Currently VFIO migration doesn't implement some kind of intermediate
>>> quiescent state in which P2P DMAs are quiesced before stopping or
>>> running the device. This can cause problems in multi-device migration
>>> where the devices are doing P2P DMAs, since the devices are not stopped
>>> together at the same time.
>>>
>>> Until such support is added, block migration of multiple devices.
>>>
>>> Signed-off-by: Avihai Horon <avihaih@nvidia.com>
>>> ---
>>>   include/hw/vfio/vfio-common.h |  2 ++
>>>   hw/vfio/common.c              | 51 +++++++++++++++++++++++++++++++++++
>>>   hw/vfio/migration.c           |  6 +++++
>>>   3 files changed, 59 insertions(+)
>>>
>>> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
>>> index e573f5a9f1..56b1683824 100644
>>> --- a/include/hw/vfio/vfio-common.h
>>> +++ b/include/hw/vfio/vfio-common.h
>>> @@ -218,6 +218,8 @@ typedef QLIST_HEAD(VFIOGroupList, VFIOGroup) VFIOGroupList;
>>>   extern VFIOGroupList vfio_group_list;
>>>
>>>   bool vfio_mig_active(void);
>>> +int vfio_block_multiple_devices_migration(Error **errp);
>>> +void vfio_unblock_multiple_devices_migration(void);
>>>   int64_t vfio_mig_bytes_transferred(void);
>>>
>>>   #ifdef CONFIG_LINUX
>>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>>> index 3a35f4afad..01db41b735 100644
>>> --- a/hw/vfio/common.c
>>> +++ b/hw/vfio/common.c
>>> @@ -41,6 +41,7 @@
>>>   #include "qapi/error.h"
>>>   #include "migration/migration.h"
>>>   #include "migration/misc.h"
>>> +#include "migration/blocker.h"
>>>   #include "sysemu/tpm.h"
>>>
>>>   VFIOGroupList vfio_group_list =
>>> @@ -337,6 +338,56 @@ bool vfio_mig_active(void)
>>>       return true;
>>>   }
>>>
>>> +Error *multiple_devices_migration_blocker;

static ?

So we have two migration blockers, one per device and one global. I guess
it is easier that way than trying to update the per device Error*.

C.


>>> +
>>> +static unsigned int vfio_migratable_device_num(void)
>>> +{
>>> +    VFIOGroup *group;
>>> +    VFIODevice *vbasedev;
>>> +    unsigned int device_num = 0;
>>> +
>>> +    QLIST_FOREACH(group, &vfio_group_list, next) {
>>> +        QLIST_FOREACH(vbasedev, &group->device_list, next) {
>>> +            if (vbasedev->migration) {
>>> +                device_num++;
>>> +            }
>>> +        }
>>> +    }
>>> +
>>> +    return device_num;
>>> +}
>>> +
>>> +int vfio_block_multiple_devices_migration(Error **errp)
>>> +{
>>> +    int ret;
>>> +
>>> +    if (vfio_migratable_device_num() != 2) {
>>> +        return 0;
>>> +    }
>>> +
>>> +    error_setg(&multiple_devices_migration_blocker,
>>> +               "Migration is currently not supported with multiple "
>>> +               "VFIO devices");
>>> +    ret = migrate_add_blocker(multiple_devices_migration_blocker, errp);
>>> +    if (ret < 0) {
>>> +        error_free(multiple_devices_migration_blocker);
>>> +        multiple_devices_migration_blocker = NULL;
>>> +    }
>>> +
>>> +    return ret;
>>> +}
>>> +
>>> +void vfio_unblock_multiple_devices_migration(void)
>>> +{
>>> +    if (vfio_migratable_device_num() != 2) {
>>> +        return;
>>> +    }
>>> +
>>> +    migrate_del_blocker(multiple_devices_migration_blocker);
>>> +    error_free(multiple_devices_migration_blocker);
>>> +    multiple_devices_migration_blocker = NULL;
>>> +}
>> A couple awkward things here.  First I wish we could do something
>> cleaner or more intuitive than the != 2 test.  I get that we're trying
>> to do this on the addition of the 2nd device supporting migration, or
>> the removal of the next to last device independent of all other devices,
>> but I wonder if it wouldn't be better to remove the multiple-device
>> blocker after migration is torn down for the device so we can test
>> device >1 or ==1 in combination with whether
>> multiple_devices_migration_blocker is NULL.
>>
>> Which comes to the second awkwardness, if we fail to add the blocker we
>> free and clear the blocker, but when we tear down the device due to that
>> failure we'll remove the blocker that doesn't exist, free NULL, and
>> clear it again.  Thanks to the glib slist the migration blocker is
>> using, I think that all works, but I'd rather not be dependent on that
>> implementation to avoid a segfault here.  Incorporating a test of
>> multiple_devices_migration_blocker as above would avoid this too.
> 
> You mean something like this?
> 
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index 3a35f4afad..f3e08eff58 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> 
> [...]
> 
> +int vfio_block_multiple_devices_migration(Error **errp)
> +{
> +    int ret;
> +
> +    if (vfio_migratable_device_num() <= 1 ||
> +        multiple_devices_migration_blocker) {
> +        return 0;
> +    }
> +
> +    error_setg(&multiple_devices_migration_blocker,
> +               "Migration is currently not supported with multiple "
> +               "VFIO devices");
> +    ret = migrate_add_blocker(multiple_devices_migration_blocker, errp);
> +    if (ret < 0) {
> +        error_free(multiple_devices_migration_blocker);
> +        multiple_devices_migration_blocker = NULL;
> +    }
> +
> +    return ret;
> +}
> +
> +void vfio_unblock_multiple_devices_migration(void)
> +{
> +    if (vfio_migratable_device_num() > 1 ||
> +        !multiple_devices_migration_blocker) {
> +        return;
> +    }
> +
> +    migrate_del_blocker(multiple_devices_migration_blocker);
> +    error_free(multiple_devices_migration_blocker);
> +    multiple_devices_migration_blocker = NULL;
> +}
> +
>   static bool vfio_devices_all_dirty_tracking(VFIOContainer *container)
>   {
>       VFIOGroup *group;
> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
> index 552c2313b2..15b446c0ec 100644
> --- a/hw/vfio/migration.c
> +++ b/hw/vfio/migration.c
> @@ -880,6 +880,11 @@ int vfio_migration_probe(VFIODevice *vbasedev, Error **errp)
>           goto add_blocker;
>       }
> 
> +    ret = vfio_block_multiple_devices_migration(errp);
> +    if (ret) {
> +        return ret;
> +    }
> +
>       trace_vfio_migration_probe(vbasedev->name, info->index);
>       g_free(info);
>       return 0;
> @@ -906,6 +911,7 @@ void vfio_migration_finalize(VFIODevice *vbasedev)
>           qemu_del_vm_change_state_handler(migration->vm_state);
>           unregister_savevm(VMSTATE_IF(vbasedev->dev), "vfio", vbasedev);
>           vfio_migration_exit(vbasedev);
> +        vfio_unblock_multiple_devices_migration();
>       }
> 
>       if (vbasedev->migration_blocker) {
> 
> 
> Maybe also negate the if conditions and put the add/remove blocker code inside it? Is it more readable this way?
> E.g.:
> 
> +int vfio_block_multiple_devices_migration(Error **errp)
> +{
> +    int ret = 0;
> +
> +    if (vfio_migratable_device_num() > 1 &&
> +        !multiple_devices_migration_blocker) {
> +        error_setg(&multiple_devices_migration_blocker,
> +                   "Migration is currently not supported with multiple "
> +                   "VFIO devices");
> +        ret = migrate_add_blocker(multiple_devices_migration_blocker, errp);
> +        if (ret < 0) {
> +            error_free(multiple_devices_migration_blocker);
> +            multiple_devices_migration_blocker = NULL;
> +        }
> +    }
> +
> +    return ret;
> +}
> +
> +void vfio_unblock_multiple_devices_migration(void)
> +{
> +    if (vfio_migratable_device_num() <= 1 &&
> +        multiple_devices_migration_blocker) {
> +        migrate_del_blocker(multiple_devices_migration_blocker);
> +        error_free(multiple_devices_migration_blocker);
> +        multiple_devices_migration_blocker = NULL;
> +    }
> +}
> 
> Thanks.
>