Re: [PATCH v4 20/25] vfio: Add Error** argument to .vfio_save_config() handler

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

On 3/7/24 10:13, Eric Auger wrote:
> 
> 
> On 3/6/24 14:34, Cédric Le Goater wrote:
>> Use vmstate_save_state_with_err() to improve error reporting in the
>> callers and store a reported error under the migration stream. Add
>> documentation while at it.
>>
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> Signed-off-by: Cédric Le Goater <clg@redhat.com>
>> ---
>>   include/hw/vfio/vfio-common.h | 25 ++++++++++++++++++++++++-
>>   hw/vfio/migration.c           | 18 ++++++++++++------
>>   hw/vfio/pci.c                 |  5 +++--
>>   3 files changed, 39 insertions(+), 9 deletions(-)
>>
>> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
>> index b9da6c08ef41174610eb92726c590309a53696a3..46f88493634b5634a9c14a5caa33a463fbf2c50d 100644
>> --- a/include/hw/vfio/vfio-common.h
>> +++ b/include/hw/vfio/vfio-common.h
>> @@ -133,7 +133,30 @@ struct VFIODeviceOps {
>>       int (*vfio_hot_reset_multi)(VFIODevice *vdev);
>>       void (*vfio_eoi)(VFIODevice *vdev);
>>       Object *(*vfio_get_object)(VFIODevice *vdev);
>> -    void (*vfio_save_config)(VFIODevice *vdev, QEMUFile *f);
>> +
>> +    /**
>> +     * @vfio_save_config
>> +     *
>> +     * Save device config state
>> +     *
>> +     * @vdev: #VFIODevice for which to save the config
>> +     * @f: #QEMUFile where to send the data
>> +     * @errp: pointer to Error*, to store an error if it happens.
>> +     *
>> +     * Returns zero to indicate success and negative for error
>> +     */
>> +    int (*vfio_save_config)(VFIODevice *vdev, QEMUFile *f, Error **errp);
>> +
>> +    /**
>> +     * @vfio_load_config
>> +     *
>> +     * Load device config state
>> +     *
>> +     * @vdev: #VFIODevice for which to load the config
>> +     * @f: #QEMUFile where to get the data
>> +     *
>> +     * Returns zero to indicate success and negative for error
>> +     */
>>       int (*vfio_load_config)(VFIODevice *vdev, QEMUFile *f);
>>   };
>>   
>> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
>> index 71ade14a7942358094371a86c00718f5979113ea..bd48f2ee472a5230c2c84bff829dae1e217db33f 100644
>> --- a/hw/vfio/migration.c
>> +++ b/hw/vfio/migration.c
>> @@ -190,14 +190,19 @@ static int vfio_load_buffer(QEMUFile *f, VFIODevice *vbasedev,
>>       return ret;
>>   }
>>   
>> -static int vfio_save_device_config_state(QEMUFile *f, void *opaque)
>> +static int vfio_save_device_config_state(QEMUFile *f, void *opaque,
>> +                                         Error **errp)
>>   {
>>       VFIODevice *vbasedev = opaque;
>> +    int ret;
>>   
>>       qemu_put_be64(f, VFIO_MIG_FLAG_DEV_CONFIG_STATE);
>>   
>>       if (vbasedev->ops && vbasedev->ops->vfio_save_config) {
>> -        vbasedev->ops->vfio_save_config(vbasedev, f);
>> +        ret = vbasedev->ops->vfio_save_config(vbasedev, f, errp);
>> +        if (ret) {
> I am not familiar enough with that case but don't you still want to set
> the VFIO_MIG_FLAG_END_OF_STATE to "close" the state?

This is a delimiter used on the target side when loading the state.

When QEMU fails to capture the device state, the whole migration is marked
as in error. There is no need to end cleanly the device state, it is bogus
anyhow.

Thanks,

C.


> 
> Eric
>> +            return ret;
>> +        }
>>       }
>>   
>>       qemu_put_be64(f, VFIO_MIG_FLAG_END_OF_STATE);
>> @@ -587,13 +592,14 @@ static int vfio_save_complete_precopy(QEMUFile *f, void *opaque)
>>   static void vfio_save_state(QEMUFile *f, void *opaque)
>>   {
>>       VFIODevice *vbasedev = opaque;
>> +    Error *local_err = NULL;
>>       int ret;
>>   
>> -    ret = vfio_save_device_config_state(f, opaque);
>> +    ret = vfio_save_device_config_state(f, opaque, &local_err);
>>       if (ret) {
>> -        error_report("%s: Failed to save device config space",
>> -                     vbasedev->name);
>> -        qemu_file_set_error(f, ret);
>> +        error_prepend(&local_err, "%s: Failed to save device config space",
>> +                      vbasedev->name);
>> +        qemu_file_set_error_obj(f, ret, local_err);
>>       }
>>   }
>>   
>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>> index 4fa387f0430d62ca2ba1b5ae5b7037f8f06b33f9..99d86e1d40ef25133fc76ad6e58294b07bd20843 100644
>> --- a/hw/vfio/pci.c
>> +++ b/hw/vfio/pci.c
>> @@ -2585,11 +2585,12 @@ const VMStateDescription vmstate_vfio_pci_config = {
>>       }
>>   };
>>   
>> -static void vfio_pci_save_config(VFIODevice *vbasedev, QEMUFile *f)
>> +static int vfio_pci_save_config(VFIODevice *vbasedev, QEMUFile *f, Error **errp)
>>   {
>>       VFIOPCIDevice *vdev = container_of(vbasedev, VFIOPCIDevice, vbasedev);
>>   
>> -    vmstate_save_state(f, &vmstate_vfio_pci_config, vdev, NULL);
>> +    return vmstate_save_state_with_err(f, &vmstate_vfio_pci_config, vdev, NULL,
>> +                                       errp);
>>   }
>>   
>>   static int vfio_pci_load_config(VFIODevice *vbasedev, QEMUFile *f)
>