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

[Cédric Le Goater]

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)