Re: [PATCH v3 10/17] vfio/migration: Move migration v1 logic to vfio_migration_init()

[Avihai Horon <avihaih@nvidia.com>]

On 16/11/2022 1:56, Alex Williamson wrote:
> External email: Use caution opening links or attachments
>
>
> On Thu, 3 Nov 2022 18:16:13 +0200
> Avihai Horon <avihaih@nvidia.com> wrote:
>
>> Move vfio_dev_get_region_info() logic from vfio_migration_probe() to
>> vfio_migration_init(). This logic is specific to v1 protocol and moving
>> it will make it easier to add the v2 protocol implementation later.
>> No functional changes intended.
>>
>> Signed-off-by: Avihai Horon <avihaih@nvidia.com>
>> ---
>>   hw/vfio/migration.c  | 30 +++++++++++++++---------------
>>   hw/vfio/trace-events |  2 +-
>>   2 files changed, 16 insertions(+), 16 deletions(-)
>>
>> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
>> index 99ffb75782..0e3a950746 100644
>> --- a/hw/vfio/migration.c
>> +++ b/hw/vfio/migration.c
>> @@ -785,14 +785,14 @@ static void vfio_migration_exit(VFIODevice *vbasedev)
>>       vbasedev->migration = NULL;
>>   }
>>
>> -static int vfio_migration_init(VFIODevice *vbasedev,
>> -                               struct vfio_region_info *info)
>> +static int vfio_migration_init(VFIODevice *vbasedev)
>>   {
>>       int ret;
>>       Object *obj;
>>       VFIOMigration *migration;
>>       char id[256] = "";
>>       g_autofree char *path = NULL, *oid = NULL;
>> +    struct vfio_region_info *info = NULL;
> Nit, I'm not spotting any cases where we need this initialization.  The
> same is not true in the code the info handling was extracted from.
> Thanks,

You are right. I will drop the initialization in v4.
Thanks!

> Alex
>
>>       if (!vbasedev->ops->vfio_get_object) {
>>           return -EINVAL;
>> @@ -803,6 +803,14 @@ static int vfio_migration_init(VFIODevice *vbasedev,
>>           return -EINVAL;
>>       }
>>
>> +    ret = vfio_get_dev_region_info(vbasedev,
>> +                                   VFIO_REGION_TYPE_MIGRATION_DEPRECATED,
>> +                                   VFIO_REGION_SUBTYPE_MIGRATION_DEPRECATED,
>> +                                   &info);
>> +    if (ret) {
>> +        return ret;
>> +    }
>> +
>>       vbasedev->migration = g_new0(VFIOMigration, 1);
>>       vbasedev->migration->device_state = VFIO_DEVICE_STATE_V1_RUNNING;
>>       vbasedev->migration->vm_running = runstate_is_running();
>> @@ -822,6 +830,8 @@ static int vfio_migration_init(VFIODevice *vbasedev,
>>           goto err;
>>       }
>>
>> +    g_free(info);
>> +
>>       migration = vbasedev->migration;
>>       migration->vbasedev = vbasedev;
>>
>> @@ -844,6 +854,7 @@ static int vfio_migration_init(VFIODevice *vbasedev,
>>       return 0;
>>
>>   err:
>> +    g_free(info);
>>       vfio_migration_exit(vbasedev);
>>       return ret;
>>   }
>> @@ -857,34 +868,23 @@ int64_t vfio_mig_bytes_transferred(void)
>>
>>   int vfio_migration_probe(VFIODevice *vbasedev, Error **errp)
>>   {
>> -    struct vfio_region_info *info = NULL;
>>       int ret = -ENOTSUP;
>>
>>       if (!vbasedev->enable_migration) {
>>           goto add_blocker;
>>       }
>>
>> -    ret = vfio_get_dev_region_info(vbasedev,
>> -                                   VFIO_REGION_TYPE_MIGRATION_DEPRECATED,
>> -                                   VFIO_REGION_SUBTYPE_MIGRATION_DEPRECATED,
>> -                                   &info);
>> +    ret = vfio_migration_init(vbasedev);
>>       if (ret) {
>>           goto add_blocker;
>>       }
>>
>> -    ret = vfio_migration_init(vbasedev, info);
>> -    if (ret) {
>> -        goto add_blocker;
>> -    }
>> -
>> -    trace_vfio_migration_probe(vbasedev->name, info->index);
>> -    g_free(info);
>> +    trace_vfio_migration_probe(vbasedev->name);
>>       return 0;
>>
>>   add_blocker:
>>       error_setg(&vbasedev->migration_blocker,
>>                  "VFIO device doesn't support migration");
>> -    g_free(info);
>>
>>       ret = migrate_add_blocker(vbasedev->migration_blocker, errp);
>>       if (ret < 0) {
>> diff --git a/hw/vfio/trace-events b/hw/vfio/trace-events
>> index a21cbd2a56..27c059f96e 100644
>> --- a/hw/vfio/trace-events
>> +++ b/hw/vfio/trace-events
>> @@ -148,7 +148,7 @@ vfio_display_edid_update(uint32_t prefx, uint32_t prefy) "%ux%u"
>>   vfio_display_edid_write_error(void) ""
>>
>>   # migration.c
>> -vfio_migration_probe(const char *name, uint32_t index) " (%s) Region %d"
>> +vfio_migration_probe(const char *name) " (%s)"
>>   vfio_migration_set_state(const char *name, uint32_t state) " (%s) state %d"
>>   vfio_vmstate_change(const char *name, int running, const char *reason, uint32_t dev_state) " (%s) running %d reason %s device state %d"
>>   vfio_migration_state_notifier(const char *name, const char *state) " (%s) state %s"