1. Patchew
  2. QEMU
  3. [PATCH v5 00/10] vfio: Improve error reporting (part 2)
  4. View patch

[PATCH v5 05/10] vfio: Add Error** argument to .vfio_save_config() handler

Cédric Le Goater posted 10 patches 1 year, 11 months ago
There is a newer version of this series
[PATCH v5 05/10] vfio: Add Error** argument to .vfio_save_config() handler
Posted by Cédric Le Goater 1 year, 11 months ago 
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 9b6375c949f7a8dca857ead2506855f63fa051e4..87437490bd50321b3eb27770c932078597053746 100644
--- a/hw/vfio/migration.c
+++ b/hw/vfio/migration.c
@@ -189,14 +189,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) {
+            return ret;
+        }
     }
 
     qemu_put_be64(f, VFIO_MIG_FLAG_END_OF_STATE);
@@ -588,13 +593,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 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)
-- 
2.45.0
Re: [PATCH v5 05/10] vfio: Add Error** argument to .vfio_save_config() handler
Posted by Avihai Horon 1 year, 11 months ago 
On 06/05/2024 12:20, Cédric Le Goater wrote:
> External email: Use caution opening links or attachments
>
>
> 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 9b6375c949f7a8dca857ead2506855f63fa051e4..87437490bd50321b3eb27770c932078597053746 100644
> --- a/hw/vfio/migration.c
> +++ b/hw/vfio/migration.c
> @@ -189,14 +189,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) {
> +            return ret;
> +        }
>       }
>
>       qemu_put_be64(f, VFIO_MIG_FLAG_END_OF_STATE);

Below we have:

return qemu_file_get_error(f);

Need to set errp in case of error.

> @@ -588,13 +593,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",

Add " - " ("... device config space - "), like in the other patches?

Thanks.

> +                      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)
> --
> 2.45.0
>
Re: [PATCH v5 05/10] vfio: Add Error** argument to .vfio_save_config() handler
Posted by Cédric Le Goater 1 year, 11 months ago 
On 5/13/24 15:30, Avihai Horon wrote:
> 
> On 06/05/2024 12:20, Cédric Le Goater wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> 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 9b6375c949f7a8dca857ead2506855f63fa051e4..87437490bd50321b3eb27770c932078597053746 100644
>> --- a/hw/vfio/migration.c
>> +++ b/hw/vfio/migration.c
>> @@ -189,14 +189,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) {
>> +            return ret;
>> +        }
>>       }
>>
>>       qemu_put_be64(f, VFIO_MIG_FLAG_END_OF_STATE);
> 
> Below we have:
> 
> return qemu_file_get_error(f);
> 
> Need to set errp in case of error.

yep. I will change the returned type to bool while at it.

> 
>> @@ -588,13 +593,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",
> 
> Add " - " ("... device config space - "), like in the other patches?

ok. I will check how (in)consistent I was when doing these
error_prepend changes.


Thanks,

C.


> 
> Thanks.
> 
>> +                      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)
>> -- 
>> 2.45.0
>>
>

Patchew 2.1-devel-8c64711

© 2016 - 2026 Red Hat, Inc.