diff mbox series

[11/14] vfio: Extend vfio_set_migration_error() with Error* argument

Message ID 20240207133347.1115903-12-clg@redhat.com (mailing list archive)
State New, archived
Headers show
Series migration: Improve error reporting | expand

Commit Message

Cédric Le Goater Feb. 7, 2024, 1:33 p.m. UTC 
vfio_set_migration_error() sets the 'return' error on the migration
stream if a migration is in progress. To improve error reporting, add
a new Error* argument to also set the Error object on the migration
stream.

Signed-off-by: Cédric Le Goater <clg@redhat.com>
---
 hw/vfio/common.c | 50 +++++++++++++++++++++++++++++-------------------
 1 file changed, 30 insertions(+), 20 deletions(-)

Comments

Philippe Mathieu-Daudé Feb. 7, 2024, 8:25 p.m. UTC | #1 
On 7/2/24 14:33, Cédric Le Goater wrote:
> vfio_set_migration_error() sets the 'return' error on the migration
> stream if a migration is in progress. To improve error reporting, add
> a new Error* argument to also set the Error object on the migration
> stream.
> 
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> ---
>   hw/vfio/common.c | 50 +++++++++++++++++++++++++++++-------------------
>   1 file changed, 30 insertions(+), 20 deletions(-)


> -static void vfio_set_migration_error(int err)
> +static void vfio_set_migration_error(int ret, Error *err)

Maybe rename vfio_report_migration_error() for clarity?

>   {
>       MigrationState *ms = migrate_get_current();
>   
>       if (migration_is_setup_or_active(ms->state)) {
>           WITH_QEMU_LOCK_GUARD(&ms->qemu_file_lock) {
>               if (ms->to_dst_file) {
> -                qemu_file_set_error(ms->to_dst_file, err);
> +                qemu_file_set_error_obj(ms->to_dst_file, ret, err);
>               }
>           }
> +    } else {
> +        error_report_err(err);
>       }
>   }
Avihai Horon Feb. 12, 2024, 9:35 a.m. UTC | #2 
Hi Cedric,

On 07/02/2024 15:33, Cédric Le Goater wrote:
> External email: Use caution opening links or attachments
>
>
> vfio_set_migration_error() sets the 'return' error on the migration
> stream if a migration is in progress. To improve error reporting, add
> a new Error* argument to also set the Error object on the migration
> stream.
>
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> ---
>   hw/vfio/common.c | 50 +++++++++++++++++++++++++++++-------------------
>   1 file changed, 30 insertions(+), 20 deletions(-)
>
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index 82173b039c47150f5edd05d329192c5b9c8a9a0f..afe8b6bd294fd5904f394a5db48aae3fd718b14e 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -148,16 +148,18 @@ bool vfio_viommu_preset(VFIODevice *vbasedev)
>       return vbasedev->bcontainer->space->as != &address_space_memory;
>   }
>
> -static void vfio_set_migration_error(int err)
> +static void vfio_set_migration_error(int ret, Error *err)
>   {
>       MigrationState *ms = migrate_get_current();
>
>       if (migration_is_setup_or_active(ms->state)) {
>           WITH_QEMU_LOCK_GUARD(&ms->qemu_file_lock) {
>               if (ms->to_dst_file) {
> -                qemu_file_set_error(ms->to_dst_file, err);
> +                qemu_file_set_error_obj(ms->to_dst_file, ret, err);
>               }
>           }
> +    } else {
> +        error_report_err(err);
>       }
>   }
>
> @@ -296,15 +298,17 @@ static void vfio_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
>       VFIOContainerBase *bcontainer = giommu->bcontainer;
>       hwaddr iova = iotlb->iova + giommu->iommu_offset;
>       void *vaddr;
> +    Error *local_err = NULL;
>       int ret;
>
>       trace_vfio_iommu_map_notify(iotlb->perm == IOMMU_NONE ? "UNMAP" : "MAP",
>                                   iova, iova + iotlb->addr_mask);
>
>       if (iotlb->target_as != &address_space_memory) {
> -        error_report("Wrong target AS \"%s\", only system memory is allowed",
> -                     iotlb->target_as->name ? iotlb->target_as->name : "none");
> -        vfio_set_migration_error(-EINVAL);
> +        error_setg(&local_err,
> +                   "Wrong target AS \"%s\", only system memory is allowed",
> +                   iotlb->target_as->name ? iotlb->target_as->name : "none");
> +        vfio_set_migration_error(-EINVAL, local_err);
>           return;
>       }
>
> @@ -336,11 +340,12 @@ static void vfio_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
>           ret = vfio_container_dma_unmap(bcontainer, iova,
>                                          iotlb->addr_mask + 1, iotlb);
>           if (ret) {
> -            error_report("vfio_container_dma_unmap(%p, 0x%"HWADDR_PRIx", "
> -                         "0x%"HWADDR_PRIx") = %d (%s)",
> -                         bcontainer, iova,
> -                         iotlb->addr_mask + 1, ret, strerror(-ret));
> -            vfio_set_migration_error(ret);
> +            error_setg(&local_err,
> +                       "vfio_container_dma_unmap(%p, 0x%"HWADDR_PRIx", "
> +                       "0x%"HWADDR_PRIx") = %d (%s)",
> +                       bcontainer, iova,
> +                       iotlb->addr_mask + 1, ret, strerror(-ret));
> +            vfio_set_migration_error(ret, local_err);
>           }
>       }
>   out:
> @@ -1224,13 +1229,15 @@ static void vfio_iommu_map_dirty_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
>       VFIOContainerBase *bcontainer = giommu->bcontainer;
>       hwaddr iova = iotlb->iova + giommu->iommu_offset;
>       ram_addr_t translated_addr;
> +    Error *local_err = NULL;
>       int ret = -EINVAL;
>
>       trace_vfio_iommu_map_dirty_notify(iova, iova + iotlb->addr_mask);
>
>       if (iotlb->target_as != &address_space_memory) {
> -        error_report("Wrong target AS \"%s\", only system memory is allowed",
> -                     iotlb->target_as->name ? iotlb->target_as->name : "none");
> +        error_setg(&local_err,
> +                   "Wrong target AS \"%s\", only system memory is allowed",
> +                   iotlb->target_as->name ? iotlb->target_as->name : "none");
>           goto out;
>       }
>
> @@ -1239,17 +1246,18 @@ static void vfio_iommu_map_dirty_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
>           ret = vfio_get_dirty_bitmap(bcontainer, iova, iotlb->addr_mask + 1,
>                                       translated_addr);

If vfio_get_xlat_addr() above (it's not shown here) returns false, we 
will pass a NULL local_err to vfio_set_migration_error() and it may 
de-reference NULL ptr in error_report_err().

Should we refactor vfio_get_xlat_addr() to get errp, or add an else 
branch below, setting -EINVAL (and removing the default -EINVAL from the 
top of the function)?

Thanks.

>           if (ret) {
> -            error_report("vfio_iommu_map_dirty_notify(%p, 0x%"HWADDR_PRIx", "
> -                         "0x%"HWADDR_PRIx") = %d (%s)",
> -                         bcontainer, iova, iotlb->addr_mask + 1, ret,
> -                         strerror(-ret));
> +            error_setg(&local_err,
> +                       "vfio_iommu_map_dirty_notify(%p, 0x%"HWADDR_PRIx", "
> +                       "0x%"HWADDR_PRIx") = %d (%s)",
> +                       bcontainer, iova, iotlb->addr_mask + 1, ret,
> +                       strerror(-ret));
>           }
>       }
>       rcu_read_unlock();
>
>   out:
>       if (ret) {
> -        vfio_set_migration_error(ret);
> +        vfio_set_migration_error(ret, local_err);
>       }
>   }
>
> @@ -1345,6 +1353,7 @@ static void vfio_listener_log_sync(MemoryListener *listener,
>   {
>       VFIOContainerBase *bcontainer = container_of(listener, VFIOContainerBase,
>                                                    listener);
> +    Error *local_err = NULL;
>       int ret;
>
>       if (vfio_listener_skipped_section(section)) {
> @@ -1354,9 +1363,10 @@ static void vfio_listener_log_sync(MemoryListener *listener,
>       if (vfio_devices_all_dirty_tracking(bcontainer)) {
>           ret = vfio_sync_dirty_bitmap(bcontainer, section);
>           if (ret) {
> -            error_report("vfio: Failed to sync dirty bitmap, err: %d (%s)", ret,
> -                         strerror(-ret));
> -            vfio_set_migration_error(ret);
> +            error_setg(&local_err,
> +                       "vfio: Failed to sync dirty bitmap, err: %d (%s)", ret,
> +                       strerror(-ret));
> +            vfio_set_migration_error(ret, local_err);
>           }
>       }
>   }
> --
> 2.43.0
>
>
Cédric Le Goater Feb. 16, 2024, 1:12 p.m. UTC | #3 
Hello Avihai,

On 2/12/24 10:35, Avihai Horon wrote:
> Hi Cedric,
> 
> On 07/02/2024 15:33, Cédric Le Goater wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> vfio_set_migration_error() sets the 'return' error on the migration
>> stream if a migration is in progress. To improve error reporting, add
>> a new Error* argument to also set the Error object on the migration
>> stream.
>>
>> Signed-off-by: Cédric Le Goater <clg@redhat.com>
>> ---
>>   hw/vfio/common.c | 50 +++++++++++++++++++++++++++++-------------------
>>   1 file changed, 30 insertions(+), 20 deletions(-)
>>
>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>> index 82173b039c47150f5edd05d329192c5b9c8a9a0f..afe8b6bd294fd5904f394a5db48aae3fd718b14e 100644
>> --- a/hw/vfio/common.c
>> +++ b/hw/vfio/common.c
>> @@ -148,16 +148,18 @@ bool vfio_viommu_preset(VFIODevice *vbasedev)
>>       return vbasedev->bcontainer->space->as != &address_space_memory;
>>   }
>>
>> -static void vfio_set_migration_error(int err)
>> +static void vfio_set_migration_error(int ret, Error *err)
>>   {
>>       MigrationState *ms = migrate_get_current();
>>
>>       if (migration_is_setup_or_active(ms->state)) {
>>           WITH_QEMU_LOCK_GUARD(&ms->qemu_file_lock) {
>>               if (ms->to_dst_file) {
>> -                qemu_file_set_error(ms->to_dst_file, err);
>> +                qemu_file_set_error_obj(ms->to_dst_file, ret, err);
>>               }
>>           }
>> +    } else {
>> +        error_report_err(err);
>>       }
>>   }
>>
>> @@ -296,15 +298,17 @@ static void vfio_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
>>       VFIOContainerBase *bcontainer = giommu->bcontainer;
>>       hwaddr iova = iotlb->iova + giommu->iommu_offset;
>>       void *vaddr;
>> +    Error *local_err = NULL;
>>       int ret;
>>
>>       trace_vfio_iommu_map_notify(iotlb->perm == IOMMU_NONE ? "UNMAP" : "MAP",
>>                                   iova, iova + iotlb->addr_mask);
>>
>>       if (iotlb->target_as != &address_space_memory) {
>> -        error_report("Wrong target AS \"%s\", only system memory is allowed",
>> -                     iotlb->target_as->name ? iotlb->target_as->name : "none");
>> -        vfio_set_migration_error(-EINVAL);
>> +        error_setg(&local_err,
>> +                   "Wrong target AS \"%s\", only system memory is allowed",
>> +                   iotlb->target_as->name ? iotlb->target_as->name : "none");
>> +        vfio_set_migration_error(-EINVAL, local_err);
>>           return;
>>       }
>>
>> @@ -336,11 +340,12 @@ static void vfio_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
>>           ret = vfio_container_dma_unmap(bcontainer, iova,
>>                                          iotlb->addr_mask + 1, iotlb);
>>           if (ret) {
>> -            error_report("vfio_container_dma_unmap(%p, 0x%"HWADDR_PRIx", "
>> -                         "0x%"HWADDR_PRIx") = %d (%s)",
>> -                         bcontainer, iova,
>> -                         iotlb->addr_mask + 1, ret, strerror(-ret));
>> -            vfio_set_migration_error(ret);
>> +            error_setg(&local_err,
>> +                       "vfio_container_dma_unmap(%p, 0x%"HWADDR_PRIx", "
>> +                       "0x%"HWADDR_PRIx") = %d (%s)",
>> +                       bcontainer, iova,
>> +                       iotlb->addr_mask + 1, ret, strerror(-ret));
>> +            vfio_set_migration_error(ret, local_err);
>>           }
>>       }
>>   out:
>> @@ -1224,13 +1229,15 @@ static void vfio_iommu_map_dirty_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
>>       VFIOContainerBase *bcontainer = giommu->bcontainer;
>>       hwaddr iova = iotlb->iova + giommu->iommu_offset;
>>       ram_addr_t translated_addr;
>> +    Error *local_err = NULL;
>>       int ret = -EINVAL;
>>
>>       trace_vfio_iommu_map_dirty_notify(iova, iova + iotlb->addr_mask);
>>
>>       if (iotlb->target_as != &address_space_memory) {
>> -        error_report("Wrong target AS \"%s\", only system memory is allowed",
>> -                     iotlb->target_as->name ? iotlb->target_as->name : "none");
>> +        error_setg(&local_err,
>> +                   "Wrong target AS \"%s\", only system memory is allowed",
>> +                   iotlb->target_as->name ? iotlb->target_as->name : "none");
>>           goto out;
>>       }
>>
>> @@ -1239,17 +1246,18 @@ static void vfio_iommu_map_dirty_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
>>           ret = vfio_get_dirty_bitmap(bcontainer, iova, iotlb->addr_mask + 1,
>>                                       translated_addr);
> 
> If vfio_get_xlat_addr() above (it's not shown here) returns false, we will pass a NULL local_err to vfio_set_migration_error() and it may de-reference NULL ptr in error_report_err().

Ah yes. Thanks for spotting this.

> 
> Should we refactor vfio_get_xlat_addr() to get errp, 

I think we should add an Error** parameter to vfio_get_xlat_addr() and
memory_get_xlat_addr(). It shouldn't be too much change and would be
cleaner.

Thanks,

C.


> or add an else branch below, setting -EINVAL (and removing the default -EINVAL from the top of the function)?
> 
> Thanks.
> 
>>           if (ret) {
>> -            error_report("vfio_iommu_map_dirty_notify(%p, 0x%"HWADDR_PRIx", "
>> -                         "0x%"HWADDR_PRIx") = %d (%s)",
>> -                         bcontainer, iova, iotlb->addr_mask + 1, ret,
>> -                         strerror(-ret));
>> +            error_setg(&local_err,
>> +                       "vfio_iommu_map_dirty_notify(%p, 0x%"HWADDR_PRIx", "
>> +                       "0x%"HWADDR_PRIx") = %d (%s)",
>> +                       bcontainer, iova, iotlb->addr_mask + 1, ret,
>> +                       strerror(-ret));
>>           }
>>       }
>>       rcu_read_unlock();
>>
>>   out:
>>       if (ret) {
>> -        vfio_set_migration_error(ret);
>> +        vfio_set_migration_error(ret, local_err);
>>       }
>>   }
>>
>> @@ -1345,6 +1353,7 @@ static void vfio_listener_log_sync(MemoryListener *listener,
>>   {
>>       VFIOContainerBase *bcontainer = container_of(listener, VFIOContainerBase,
>>                                                    listener);
>> +    Error *local_err = NULL;
>>       int ret;
>>
>>       if (vfio_listener_skipped_section(section)) {
>> @@ -1354,9 +1363,10 @@ static void vfio_listener_log_sync(MemoryListener *listener,
>>       if (vfio_devices_all_dirty_tracking(bcontainer)) {
>>           ret = vfio_sync_dirty_bitmap(bcontainer, section);
>>           if (ret) {
>> -            error_report("vfio: Failed to sync dirty bitmap, err: %d (%s)", ret,
>> -                         strerror(-ret));
>> -            vfio_set_migration_error(ret);
>> +            error_setg(&local_err,
>> +                       "vfio: Failed to sync dirty bitmap, err: %d (%s)", ret,
>> +                       strerror(-ret));
>> +            vfio_set_migration_error(ret, local_err);
>>           }
>>       }
>>   }
>> -- 
>> 2.43.0
>>
>>
>
diff mbox series

Patch

diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 82173b039c47150f5edd05d329192c5b9c8a9a0f..afe8b6bd294fd5904f394a5db48aae3fd718b14e 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -148,16 +148,18 @@  bool vfio_viommu_preset(VFIODevice *vbasedev)
     return vbasedev->bcontainer->space->as != &address_space_memory;
 }
 
-static void vfio_set_migration_error(int err)
+static void vfio_set_migration_error(int ret, Error *err)
 {
     MigrationState *ms = migrate_get_current();
 
     if (migration_is_setup_or_active(ms->state)) {
         WITH_QEMU_LOCK_GUARD(&ms->qemu_file_lock) {
             if (ms->to_dst_file) {
-                qemu_file_set_error(ms->to_dst_file, err);
+                qemu_file_set_error_obj(ms->to_dst_file, ret, err);
             }
         }
+    } else {
+        error_report_err(err);
     }
 }
 
@@ -296,15 +298,17 @@  static void vfio_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
     VFIOContainerBase *bcontainer = giommu->bcontainer;
     hwaddr iova = iotlb->iova + giommu->iommu_offset;
     void *vaddr;
+    Error *local_err = NULL;
     int ret;
 
     trace_vfio_iommu_map_notify(iotlb->perm == IOMMU_NONE ? "UNMAP" : "MAP",
                                 iova, iova + iotlb->addr_mask);
 
     if (iotlb->target_as != &address_space_memory) {
-        error_report("Wrong target AS \"%s\", only system memory is allowed",
-                     iotlb->target_as->name ? iotlb->target_as->name : "none");
-        vfio_set_migration_error(-EINVAL);
+        error_setg(&local_err,
+                   "Wrong target AS \"%s\", only system memory is allowed",
+                   iotlb->target_as->name ? iotlb->target_as->name : "none");
+        vfio_set_migration_error(-EINVAL, local_err);
         return;
     }
 
@@ -336,11 +340,12 @@  static void vfio_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
         ret = vfio_container_dma_unmap(bcontainer, iova,
                                        iotlb->addr_mask + 1, iotlb);
         if (ret) {
-            error_report("vfio_container_dma_unmap(%p, 0x%"HWADDR_PRIx", "
-                         "0x%"HWADDR_PRIx") = %d (%s)",
-                         bcontainer, iova,
-                         iotlb->addr_mask + 1, ret, strerror(-ret));
-            vfio_set_migration_error(ret);
+            error_setg(&local_err,
+                       "vfio_container_dma_unmap(%p, 0x%"HWADDR_PRIx", "
+                       "0x%"HWADDR_PRIx") = %d (%s)",
+                       bcontainer, iova,
+                       iotlb->addr_mask + 1, ret, strerror(-ret));
+            vfio_set_migration_error(ret, local_err);
         }
     }
 out:
@@ -1224,13 +1229,15 @@  static void vfio_iommu_map_dirty_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
     VFIOContainerBase *bcontainer = giommu->bcontainer;
     hwaddr iova = iotlb->iova + giommu->iommu_offset;
     ram_addr_t translated_addr;
+    Error *local_err = NULL;
     int ret = -EINVAL;
 
     trace_vfio_iommu_map_dirty_notify(iova, iova + iotlb->addr_mask);
 
     if (iotlb->target_as != &address_space_memory) {
-        error_report("Wrong target AS \"%s\", only system memory is allowed",
-                     iotlb->target_as->name ? iotlb->target_as->name : "none");
+        error_setg(&local_err,
+                   "Wrong target AS \"%s\", only system memory is allowed",
+                   iotlb->target_as->name ? iotlb->target_as->name : "none");
         goto out;
     }
 
@@ -1239,17 +1246,18 @@  static void vfio_iommu_map_dirty_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
         ret = vfio_get_dirty_bitmap(bcontainer, iova, iotlb->addr_mask + 1,
                                     translated_addr);
         if (ret) {
-            error_report("vfio_iommu_map_dirty_notify(%p, 0x%"HWADDR_PRIx", "
-                         "0x%"HWADDR_PRIx") = %d (%s)",
-                         bcontainer, iova, iotlb->addr_mask + 1, ret,
-                         strerror(-ret));
+            error_setg(&local_err,
+                       "vfio_iommu_map_dirty_notify(%p, 0x%"HWADDR_PRIx", "
+                       "0x%"HWADDR_PRIx") = %d (%s)",
+                       bcontainer, iova, iotlb->addr_mask + 1, ret,
+                       strerror(-ret));
         }
     }
     rcu_read_unlock();
 
 out:
     if (ret) {
-        vfio_set_migration_error(ret);
+        vfio_set_migration_error(ret, local_err);
     }
 }
 
@@ -1345,6 +1353,7 @@  static void vfio_listener_log_sync(MemoryListener *listener,
 {
     VFIOContainerBase *bcontainer = container_of(listener, VFIOContainerBase,
                                                  listener);
+    Error *local_err = NULL;
     int ret;
 
     if (vfio_listener_skipped_section(section)) {
@@ -1354,9 +1363,10 @@  static void vfio_listener_log_sync(MemoryListener *listener,
     if (vfio_devices_all_dirty_tracking(bcontainer)) {
         ret = vfio_sync_dirty_bitmap(bcontainer, section);
         if (ret) {
-            error_report("vfio: Failed to sync dirty bitmap, err: %d (%s)", ret,
-                         strerror(-ret));
-            vfio_set_migration_error(ret);
+            error_setg(&local_err,
+                       "vfio: Failed to sync dirty bitmap, err: %d (%s)", ret,
+                       strerror(-ret));
+            vfio_set_migration_error(ret, local_err);
         }
     }
 }