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

[Avihai Horon]

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"