diff mbox series

[v5,02/10] vfio: Add Error** argument to vfio_devices_dma_logging_start()

Message ID 20240506092053.388578-3-clg@redhat.com (mailing list archive)
State New, archived
Headers show
Series vfio: Improve error reporting (part 2) | expand

Commit Message

Cédric Le Goater May 6, 2024, 9:20 a.m. UTC 
This allows to update the Error argument of the VFIO log_global_start()
handler. Errors detected when device level logging is started will be
propagated up to qemu_savevm_state_setup() when the ram save_setup()
handler is executed.

The vfio_set_migration_error() call becomes redundant in
vfio_devices_dma_logging_start(). Remove it.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Cédric Le Goater <clg@redhat.com>
---

 Changes in v5:

 - Used error_setg_errno() in vfio_devices_dma_logging_start()
 
 hw/vfio/common.c | 26 +++++++++++++++-----------
 1 file changed, 15 insertions(+), 11 deletions(-)

Comments

Avihai Horon May 13, 2024, 1:08 p.m. UTC | #1 
On 06/05/2024 12:20, Cédric Le Goater wrote:
> External email: Use caution opening links or attachments
>
>
> This allows to update the Error argument of the VFIO log_global_start()
> handler. Errors detected when device level logging is started will be
> propagated up to qemu_savevm_state_setup() when the ram save_setup()
> handler is executed.

Errors for container based logging will also be propagated now.

>
> The vfio_set_migration_error() call becomes redundant in
> vfio_devices_dma_logging_start(). Remove it.

Becomes redundant in vfio_listener_log_global_start()?

Other than that,
Reviewed-by: Avihai Horon <avihaih@nvidia.com>

>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> ---
>
>   Changes in v5:
>
>   - Used error_setg_errno() in vfio_devices_dma_logging_start()
>
>   hw/vfio/common.c | 26 +++++++++++++++-----------
>   1 file changed, 15 insertions(+), 11 deletions(-)
>
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index 485e53916491f1164d29e739fb7106c0c77df737..b5102f54a6474a50c6366e8fbce23812d55e384e 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -1027,7 +1027,8 @@ static void vfio_device_feature_dma_logging_start_destroy(
>       g_free(feature);
>   }
>
> -static int vfio_devices_dma_logging_start(VFIOContainerBase *bcontainer)
> +static int vfio_devices_dma_logging_start(VFIOContainerBase *bcontainer,
> +                                          Error **errp)
>   {
>       struct vfio_device_feature *feature;
>       VFIODirtyRanges ranges;
> @@ -1038,6 +1039,7 @@ static int vfio_devices_dma_logging_start(VFIOContainerBase *bcontainer)
>       feature = vfio_device_feature_dma_logging_start_create(bcontainer,
>                                                              &ranges);
>       if (!feature) {
> +        error_setg_errno(errp, errno, "Failed to prepare DMA logging");
>           return -errno;
>       }
>
> @@ -1049,8 +1051,8 @@ static int vfio_devices_dma_logging_start(VFIOContainerBase *bcontainer)
>           ret = ioctl(vbasedev->fd, VFIO_DEVICE_FEATURE, feature);
>           if (ret) {
>               ret = -errno;
> -            error_report("%s: Failed to start DMA logging, err %d (%s)",
> -                         vbasedev->name, ret, strerror(errno));
> +            error_setg_errno(errp, errno, "%s: Failed to start DMA logging",
> +                             vbasedev->name);
>               goto out;
>           }
>           vbasedev->dirty_tracking = true;
> @@ -1069,20 +1071,19 @@ out:
>   static bool vfio_listener_log_global_start(MemoryListener *listener,
>                                              Error **errp)
>   {
> +    ERRP_GUARD();
>       VFIOContainerBase *bcontainer = container_of(listener, VFIOContainerBase,
>                                                    listener);
>       int ret;
>
>       if (vfio_devices_all_device_dirty_tracking(bcontainer)) {
> -        ret = vfio_devices_dma_logging_start(bcontainer);
> +        ret = vfio_devices_dma_logging_start(bcontainer, errp);
>       } else {
> -        ret = vfio_container_set_dirty_page_tracking(bcontainer, true, NULL);
> +        ret = vfio_container_set_dirty_page_tracking(bcontainer, true, errp);
>       }
>
>       if (ret) {
> -        error_report("vfio: Could not start dirty page tracking, err: %d (%s)",
> -                     ret, strerror(-ret));
> -        vfio_set_migration_error(ret);
> +        error_prepend(errp, "vfio: Could not start dirty page tracking - ");
>       }
>       return !ret;
>   }
> @@ -1091,17 +1092,20 @@ static void vfio_listener_log_global_stop(MemoryListener *listener)
>   {
>       VFIOContainerBase *bcontainer = container_of(listener, VFIOContainerBase,
>                                                    listener);
> +    Error *local_err = NULL;
>       int ret = 0;
>
>       if (vfio_devices_all_device_dirty_tracking(bcontainer)) {
>           vfio_devices_dma_logging_stop(bcontainer);
>       } else {
> -        ret = vfio_container_set_dirty_page_tracking(bcontainer, false, NULL);
> +        ret = vfio_container_set_dirty_page_tracking(bcontainer, false,
> +                                                     &local_err);
>       }
>
>       if (ret) {
> -        error_report("vfio: Could not stop dirty page tracking, err: %d (%s)",
> -                     ret, strerror(-ret));
> +        error_prepend(&local_err,
> +                      "vfio: Could not stop dirty page tracking - ");
> +        error_report_err(local_err);
>           vfio_set_migration_error(ret);
>       }
>   }
> --
> 2.45.0
>
Cédric Le Goater May 13, 2024, 4:49 p.m. UTC | #2 
On 5/13/24 15:08, Avihai Horon wrote:
> 
> On 06/05/2024 12:20, Cédric Le Goater wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> This allows to update the Error argument of the VFIO log_global_start()
>> handler. Errors detected when device level logging is started will be
>> propagated up to qemu_savevm_state_setup() when the ram save_setup()
>> handler is executed.
> 
> Errors for container based logging will also be propagated now.
> 
>>
>> The vfio_set_migration_error() call becomes redundant in
>> vfio_devices_dma_logging_start(). Remove it.
> 
> Becomes redundant in vfio_listener_log_global_start()?


Both sentences updated.


> Other than that,
> Reviewed-by: Avihai Horon <avihaih@nvidia.com>



Thanks,

C.


>>
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> Signed-off-by: Cédric Le Goater <clg@redhat.com>
>> ---
>>
>>   Changes in v5:
>>
>>   - Used error_setg_errno() in vfio_devices_dma_logging_start()
>>
>>   hw/vfio/common.c | 26 +++++++++++++++-----------
>>   1 file changed, 15 insertions(+), 11 deletions(-)
>>
>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>> index 485e53916491f1164d29e739fb7106c0c77df737..b5102f54a6474a50c6366e8fbce23812d55e384e 100644
>> --- a/hw/vfio/common.c
>> +++ b/hw/vfio/common.c
>> @@ -1027,7 +1027,8 @@ static void vfio_device_feature_dma_logging_start_destroy(
>>       g_free(feature);
>>   }
>>
>> -static int vfio_devices_dma_logging_start(VFIOContainerBase *bcontainer)
>> +static int vfio_devices_dma_logging_start(VFIOContainerBase *bcontainer,
>> +                                          Error **errp)
>>   {
>>       struct vfio_device_feature *feature;
>>       VFIODirtyRanges ranges;
>> @@ -1038,6 +1039,7 @@ static int vfio_devices_dma_logging_start(VFIOContainerBase *bcontainer)
>>       feature = vfio_device_feature_dma_logging_start_create(bcontainer,
>>                                                              &ranges);
>>       if (!feature) {
>> +        error_setg_errno(errp, errno, "Failed to prepare DMA logging");
>>           return -errno;
>>       }
>>
>> @@ -1049,8 +1051,8 @@ static int vfio_devices_dma_logging_start(VFIOContainerBase *bcontainer)
>>           ret = ioctl(vbasedev->fd, VFIO_DEVICE_FEATURE, feature);
>>           if (ret) {
>>               ret = -errno;
>> -            error_report("%s: Failed to start DMA logging, err %d (%s)",
>> -                         vbasedev->name, ret, strerror(errno));
>> +            error_setg_errno(errp, errno, "%s: Failed to start DMA logging",
>> +                             vbasedev->name);
>>               goto out;
>>           }
>>           vbasedev->dirty_tracking = true;
>> @@ -1069,20 +1071,19 @@ out:
>>   static bool vfio_listener_log_global_start(MemoryListener *listener,
>>                                              Error **errp)
>>   {
>> +    ERRP_GUARD();
>>       VFIOContainerBase *bcontainer = container_of(listener, VFIOContainerBase,
>>                                                    listener);
>>       int ret;
>>
>>       if (vfio_devices_all_device_dirty_tracking(bcontainer)) {
>> -        ret = vfio_devices_dma_logging_start(bcontainer);
>> +        ret = vfio_devices_dma_logging_start(bcontainer, errp);
>>       } else {
>> -        ret = vfio_container_set_dirty_page_tracking(bcontainer, true, NULL);
>> +        ret = vfio_container_set_dirty_page_tracking(bcontainer, true, errp);
>>       }
>>
>>       if (ret) {
>> -        error_report("vfio: Could not start dirty page tracking, err: %d (%s)",
>> -                     ret, strerror(-ret));
>> -        vfio_set_migration_error(ret);
>> +        error_prepend(errp, "vfio: Could not start dirty page tracking - ");
>>       }
>>       return !ret;
>>   }
>> @@ -1091,17 +1092,20 @@ static void vfio_listener_log_global_stop(MemoryListener *listener)
>>   {
>>       VFIOContainerBase *bcontainer = container_of(listener, VFIOContainerBase,
>>                                                    listener);
>> +    Error *local_err = NULL;
>>       int ret = 0;
>>
>>       if (vfio_devices_all_device_dirty_tracking(bcontainer)) {
>>           vfio_devices_dma_logging_stop(bcontainer);
>>       } else {
>> -        ret = vfio_container_set_dirty_page_tracking(bcontainer, false, NULL);
>> +        ret = vfio_container_set_dirty_page_tracking(bcontainer, false,
>> +                                                     &local_err);
>>       }
>>
>>       if (ret) {
>> -        error_report("vfio: Could not stop dirty page tracking, err: %d (%s)",
>> -                     ret, strerror(-ret));
>> +        error_prepend(&local_err,
>> +                      "vfio: Could not stop dirty page tracking - ");
>> +        error_report_err(local_err);
>>           vfio_set_migration_error(ret);
>>       }
>>   }
>> -- 
>> 2.45.0
>>
>
diff mbox series

Patch

diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 485e53916491f1164d29e739fb7106c0c77df737..b5102f54a6474a50c6366e8fbce23812d55e384e 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -1027,7 +1027,8 @@  static void vfio_device_feature_dma_logging_start_destroy(
     g_free(feature);
 }
 
-static int vfio_devices_dma_logging_start(VFIOContainerBase *bcontainer)
+static int vfio_devices_dma_logging_start(VFIOContainerBase *bcontainer,
+                                          Error **errp)
 {
     struct vfio_device_feature *feature;
     VFIODirtyRanges ranges;
@@ -1038,6 +1039,7 @@  static int vfio_devices_dma_logging_start(VFIOContainerBase *bcontainer)
     feature = vfio_device_feature_dma_logging_start_create(bcontainer,
                                                            &ranges);
     if (!feature) {
+        error_setg_errno(errp, errno, "Failed to prepare DMA logging");
         return -errno;
     }
 
@@ -1049,8 +1051,8 @@  static int vfio_devices_dma_logging_start(VFIOContainerBase *bcontainer)
         ret = ioctl(vbasedev->fd, VFIO_DEVICE_FEATURE, feature);
         if (ret) {
             ret = -errno;
-            error_report("%s: Failed to start DMA logging, err %d (%s)",
-                         vbasedev->name, ret, strerror(errno));
+            error_setg_errno(errp, errno, "%s: Failed to start DMA logging",
+                             vbasedev->name);
             goto out;
         }
         vbasedev->dirty_tracking = true;
@@ -1069,20 +1071,19 @@  out:
 static bool vfio_listener_log_global_start(MemoryListener *listener,
                                            Error **errp)
 {
+    ERRP_GUARD();
     VFIOContainerBase *bcontainer = container_of(listener, VFIOContainerBase,
                                                  listener);
     int ret;
 
     if (vfio_devices_all_device_dirty_tracking(bcontainer)) {
-        ret = vfio_devices_dma_logging_start(bcontainer);
+        ret = vfio_devices_dma_logging_start(bcontainer, errp);
     } else {
-        ret = vfio_container_set_dirty_page_tracking(bcontainer, true, NULL);
+        ret = vfio_container_set_dirty_page_tracking(bcontainer, true, errp);
     }
 
     if (ret) {
-        error_report("vfio: Could not start dirty page tracking, err: %d (%s)",
-                     ret, strerror(-ret));
-        vfio_set_migration_error(ret);
+        error_prepend(errp, "vfio: Could not start dirty page tracking - ");
     }
     return !ret;
 }
@@ -1091,17 +1092,20 @@  static void vfio_listener_log_global_stop(MemoryListener *listener)
 {
     VFIOContainerBase *bcontainer = container_of(listener, VFIOContainerBase,
                                                  listener);
+    Error *local_err = NULL;
     int ret = 0;
 
     if (vfio_devices_all_device_dirty_tracking(bcontainer)) {
         vfio_devices_dma_logging_stop(bcontainer);
     } else {
-        ret = vfio_container_set_dirty_page_tracking(bcontainer, false, NULL);
+        ret = vfio_container_set_dirty_page_tracking(bcontainer, false,
+                                                     &local_err);
     }
 
     if (ret) {
-        error_report("vfio: Could not stop dirty page tracking, err: %d (%s)",
-                     ret, strerror(-ret));
+        error_prepend(&local_err,
+                      "vfio: Could not stop dirty page tracking - ");
+        error_report_err(local_err);
         vfio_set_migration_error(ret);
     }
 }