Re: [PATCH v26 04/17] vfio: Add migration region initialization and finalize function

[Kirti Wankhede]

On 9/24/2020 7:38 PM, Cornelia Huck wrote:
> On Wed, 23 Sep 2020 04:54:06 +0530
> Kirti Wankhede <kwankhede@nvidia.com> wrote:
>
> > Whether the VFIO device supports migration or not is decided based of
> > migration region query. If migration region query is successful and migration
> > region initialization is successful then migration is supported else
> > migration is blocked.
> >
> > Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com>
> > Reviewed-by: Neo Jia <cjia@nvidia.com>
> > Acked-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > ---
> >   hw/vfio/meson.build           |   1 +
> >   hw/vfio/migration.c           | 142 ++++++++++++++++++++++++++++++++++++++++++
> >   hw/vfio/trace-events          |   5 ++
> >   include/hw/vfio/vfio-common.h |   9 +++
> >   4 files changed, 157 insertions(+)
> >   create mode 100644 hw/vfio/migration.c
>
> (...)
>
> > +static int vfio_migration_region_init(VFIODevice *vbasedev, int index)
> > +{
> > +    VFIOMigration *migration = vbasedev->migration;
> > +    Object *obj = NULL;
> > +    int ret = -EINVAL;
> > +
> > +    obj = vbasedev->ops->vfio_get_object(vbasedev);
> > +    if (!obj) {
> > +        return ret;
> > +    }
> > +
> > +    ret = vfio_region_setup(obj, vbasedev, &migration->region, index,
> > +                            "migration");
> > +    if (ret) {
> > +        error_report("%s: Failed to setup VFIO migration region %d: %s",
> > +                     vbasedev->name, index, strerror(-ret));
> > +        goto err;
> > +    }
> > +
> > +    if (!migration->region.size) {
> > +        ret = -EINVAL;
> > +        error_report("%s: Invalid region size of VFIO migration region %d: %s",
> > +                     vbasedev->name, index, strerror(-ret));
>
> Using strerror on a hardcoded error value is probably not terribly
> helpful. I think printing either region.size (if you plan to extend
> this check later) or something like "Invalid zero-sized VFIO migration
> region" would make more sense.

Updating the error string as you suggested.


> > +        goto err;
> > +    }
> > +
> > +    return 0;
> > +
> > +err:
> > +    vfio_migration_region_exit(vbasedev);
> > +    return ret;
> > +}
>
> (...)
>
> Apart from that, looks good to me.

Thanks.

Kirti