On 5/14/24 17:31, 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é <phi...@linaro.org>
> Signed-off-by: Cédric Le Goater <c...@redhat.com>
> ---
>
> Changes in v6:
>
> - Modified title (Avihai)
> - vfio_save_device_config_state() : Set errp if the migration stream
> is in error (Avihai)
> - vfio_save_state() : Changed error prefix (Avihai)
>
> include/hw/vfio/vfio-common.h | 25 ++++++++++++++++++++++++-
> hw/vfio/migration.c | 25 ++++++++++++++++++-------
> hw/vfio/pci.c | 5 +++--
> 3 files changed, 45 insertions(+), 10 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
> bf11135167ebb162dd41415656bdacfa0e1ca550..d089fa9b937e725307c1a56755495d5b8fae2065
> 100644
> --- a/hw/vfio/migration.c
> +++ b/hw/vfio/migration.c
> @@ -189,21 +189,30 @@ 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) {
> + return ret;
> + }
> }
>
> qemu_put_be64(f, VFIO_MIG_FLAG_END_OF_STATE);
>
> trace_vfio_save_device_config_state(vbasedev->name);
>
> - return qemu_file_get_error(f);
> + ret = qemu_file_get_error(f);
> + if (ret < 0) {
> + error_setg_errno(errp, -ret, "Failed to save state");
> + }
> + return ret;
> }
>
> static int vfio_load_device_config_state(QEMUFile *f, void *opaque)
> @@ -588,13 +597,15 @@ 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,
> + "vfio: Failed to save device config space of %s - ",
> + vbasedev->name);
> + qemu_file_set_error_obj(f, ret, local_err);> }
> }
>
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index
> 64780d1b793345c8e8996fe6b7987059ce831c11..fc6e54e871508bb0e2a3ac9079a195c086531f21
> 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -2586,11 +2586,12 @@ static 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)
Looks good to me
Reviewed-by: Eric Auger <eric.au...@redhat.com>
Eric
