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

[Kirti Wankhede <kwankhede@nvidia.com>]

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