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

[Eric Auger <eauger@redhat.com>]

On 3/7/24 14:55, Cédric Le Goater wrote:
> 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.

OK thanks

Eric
> 
> 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)
>>
>