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