diff mbox series

[v5,10/10] vfio: Extend vfio_set_migration_error() with Error* argument

Message ID 20240506092053.388578-11-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
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, if a migration is progress.

Signed-off-by: Cédric Le Goater <clg@redhat.com>
---

 Changes in v5:
 
 - Rebased on 20c64c8a51a4 ("migration: migration_file_set_error")

 hw/vfio/common.c | 37 ++++++++++++++++++-------------------
 1 file changed, 18 insertions(+), 19 deletions(-)

Comments

Avihai Horon May 13, 2024, 2:26 p.m. UTC | #1
On 06/05/2024 12:20, 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, if a migration is progress.
>
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> ---
>
>   Changes in v5:
>
>   - Rebased on 20c64c8a51a4 ("migration: migration_file_set_error")
>
>   hw/vfio/common.c | 37 ++++++++++++++++++-------------------
>   1 file changed, 18 insertions(+), 19 deletions(-)
>
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index c3d82a9d6e434e33f361e4b96157bf912d5c3a2f..4cf3e13a8439bd1b9a032e9d4e75df676eba457b 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -147,10 +147,10 @@ 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)
>   {
>       if (migration_is_setup_or_active()) {
> -        migration_file_set_error(err, NULL);
> +        migration_file_set_error(ret, err);
>       }
>   }
>
> @@ -295,9 +295,10 @@ static void vfio_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
>                                   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;
>       }
>
> @@ -330,11 +331,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));

Use error_setg_errno()?

> +            vfio_set_migration_error(ret, local_err);

Now dma unmap errors (and also the error before it) are not reported if 
they happen not during migration.

This makes me think, maybe vfio_set_migration_error() is redundant and 
can be replaced by migration_file_set_error()?

Thanks.

>           }
>       }
>   out:
> @@ -1108,8 +1110,7 @@ static void vfio_listener_log_global_stop(MemoryListener *listener)
>       if (ret) {
>           error_prepend(&local_err,
>                         "vfio: Could not stop dirty page tracking - ");
> -        error_report_err(local_err);
> -        vfio_set_migration_error(ret);
> +        vfio_set_migration_error(ret, local_err);
>       }
>   }
>
> @@ -1226,14 +1227,14 @@ static void vfio_iommu_map_dirty_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
>       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;
>       }
>
>       rcu_read_lock();
>       if (!vfio_get_xlat_addr(iotlb, NULL, &translated_addr, NULL, &local_err)) {
> -        error_report_err(local_err);
>           goto out_lock;
>       }
>
> @@ -1244,7 +1245,6 @@ static void vfio_iommu_map_dirty_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
>                         "vfio_iommu_map_dirty_notify(%p, 0x%"HWADDR_PRIx", "
>                         "0x%"HWADDR_PRIx") failed - ", bcontainer, iova,
>                         iotlb->addr_mask + 1);
> -        error_report_err(local_err);
>       }
>
>   out_lock:
> @@ -1252,7 +1252,7 @@ out_lock:
>
>   out:
>       if (ret) {
> -        vfio_set_migration_error(ret);
> +        vfio_set_migration_error(ret, local_err);
>       }
>   }
>
> @@ -1372,8 +1372,7 @@ static void vfio_listener_log_sync(MemoryListener *listener,
>       if (vfio_devices_all_dirty_tracking(bcontainer)) {
>           ret = vfio_sync_dirty_bitmap(bcontainer, section, &local_err);
>           if (ret) {
> -            error_report_err(local_err);
> -            vfio_set_migration_error(ret);
> +            vfio_set_migration_error(ret, local_err);
>           }
>       }
>   }
> --
> 2.45.0
>
Cédric Le Goater May 14, 2024, 11:20 a.m. UTC | #2
On 5/13/24 16:26, Avihai Horon wrote:
> 
> On 06/05/2024 12:20, 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, if a migration is progress.
>>
>> Signed-off-by: Cédric Le Goater <clg@redhat.com>
>> ---
>>
>>   Changes in v5:
>>
>>   - Rebased on 20c64c8a51a4 ("migration: migration_file_set_error")
>>
>>   hw/vfio/common.c | 37 ++++++++++++++++++-------------------
>>   1 file changed, 18 insertions(+), 19 deletions(-)
>>
>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>> index c3d82a9d6e434e33f361e4b96157bf912d5c3a2f..4cf3e13a8439bd1b9a032e9d4e75df676eba457b 100644
>> --- a/hw/vfio/common.c
>> +++ b/hw/vfio/common.c
>> @@ -147,10 +147,10 @@ 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)
>>   {
>>       if (migration_is_setup_or_active()) {
>> -        migration_file_set_error(err, NULL);
>> +        migration_file_set_error(ret, err);
>>       }
>>   }
>>
>> @@ -295,9 +295,10 @@ static void vfio_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
>>                                   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;
>>       }
>>
>> @@ -330,11 +331,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));
> 
> Use error_setg_errno()?

sure.

> 
>> +            vfio_set_migration_error(ret, local_err);
> 
> Now dma unmap errors (and also the error before it) are not reported if they happen not during migration.
> 
> This makes me think, maybe vfio_set_migration_error() is redundant and can be replaced by migration_file_set_error()?


yes. Good suggestion. I would like to get rid of vfio_set_migration_error(),
so that's a good start.

Thanks,

C.
  
> Thanks.
> 
>>           }
>>       }
>>   out:
>> @@ -1108,8 +1110,7 @@ static void vfio_listener_log_global_stop(MemoryListener *listener)
>>       if (ret) {
>>           error_prepend(&local_err,
>>                         "vfio: Could not stop dirty page tracking - ");
>> -        error_report_err(local_err);
>> -        vfio_set_migration_error(ret);
>> +        vfio_set_migration_error(ret, local_err);
>>       }
>>   }
>>
>> @@ -1226,14 +1227,14 @@ static void vfio_iommu_map_dirty_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
>>       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;
>>       }
>>
>>       rcu_read_lock();
>>       if (!vfio_get_xlat_addr(iotlb, NULL, &translated_addr, NULL, &local_err)) {
>> -        error_report_err(local_err);
>>           goto out_lock;
>>       }
>>
>> @@ -1244,7 +1245,6 @@ static void vfio_iommu_map_dirty_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
>>                         "vfio_iommu_map_dirty_notify(%p, 0x%"HWADDR_PRIx", "
>>                         "0x%"HWADDR_PRIx") failed - ", bcontainer, iova,
>>                         iotlb->addr_mask + 1);
>> -        error_report_err(local_err);
>>       }
>>
>>   out_lock:
>> @@ -1252,7 +1252,7 @@ out_lock:
>>
>>   out:
>>       if (ret) {
>> -        vfio_set_migration_error(ret);
>> +        vfio_set_migration_error(ret, local_err);
>>       }
>>   }
>>
>> @@ -1372,8 +1372,7 @@ static void vfio_listener_log_sync(MemoryListener *listener,
>>       if (vfio_devices_all_dirty_tracking(bcontainer)) {
>>           ret = vfio_sync_dirty_bitmap(bcontainer, section, &local_err);
>>           if (ret) {
>> -            error_report_err(local_err);
>> -            vfio_set_migration_error(ret);
>> +            vfio_set_migration_error(ret, local_err);
>>           }
>>       }
>>   }
>> -- 
>> 2.45.0
>>
>
Cédric Le Goater May 14, 2024, 2:35 p.m. UTC | #3
On 5/14/24 13:20, Cédric Le Goater wrote:
> On 5/13/24 16:26, Avihai Horon wrote:
>>
>> On 06/05/2024 12:20, 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, if a migration is progress.
>>>
>>> Signed-off-by: Cédric Le Goater <clg@redhat.com>
>>> ---
>>>
>>>   Changes in v5:
>>>
>>>   - Rebased on 20c64c8a51a4 ("migration: migration_file_set_error")
>>>
>>>   hw/vfio/common.c | 37 ++++++++++++++++++-------------------
>>>   1 file changed, 18 insertions(+), 19 deletions(-)
>>>
>>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>>> index c3d82a9d6e434e33f361e4b96157bf912d5c3a2f..4cf3e13a8439bd1b9a032e9d4e75df676eba457b 100644
>>> --- a/hw/vfio/common.c
>>> +++ b/hw/vfio/common.c
>>> @@ -147,10 +147,10 @@ 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)
>>>   {
>>>       if (migration_is_setup_or_active()) {
>>> -        migration_file_set_error(err, NULL);
>>> +        migration_file_set_error(ret, err);
>>>       }
>>>   }
>>>
>>> @@ -295,9 +295,10 @@ static void vfio_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
>>>                                   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;
>>>       }
>>>
>>> @@ -330,11 +331,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));
>>
>> Use error_setg_errno()?
> 
> sure.
> 
>>
>>> +            vfio_set_migration_error(ret, local_err);
>>
>> Now dma unmap errors (and also the error before it) are not reported if they happen not during migration.
>>
>> This makes me think, maybe vfio_set_migration_error() is redundant and can be replaced by migration_file_set_error()?
> 
> 
> yes. Good suggestion. I would like to get rid of vfio_set_migration_error(),
> so that's a good start.

After taking a closer look, I will drop this patch from v6 because
it needs further splitting and some rework.

Thanks,

C.
diff mbox series

Patch

diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index c3d82a9d6e434e33f361e4b96157bf912d5c3a2f..4cf3e13a8439bd1b9a032e9d4e75df676eba457b 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -147,10 +147,10 @@  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)
 {
     if (migration_is_setup_or_active()) {
-        migration_file_set_error(err, NULL);
+        migration_file_set_error(ret, err);
     }
 }
 
@@ -295,9 +295,10 @@  static void vfio_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
                                 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;
     }
 
@@ -330,11 +331,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:
@@ -1108,8 +1110,7 @@  static void vfio_listener_log_global_stop(MemoryListener *listener)
     if (ret) {
         error_prepend(&local_err,
                       "vfio: Could not stop dirty page tracking - ");
-        error_report_err(local_err);
-        vfio_set_migration_error(ret);
+        vfio_set_migration_error(ret, local_err);
     }
 }
 
@@ -1226,14 +1227,14 @@  static void vfio_iommu_map_dirty_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
     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;
     }
 
     rcu_read_lock();
     if (!vfio_get_xlat_addr(iotlb, NULL, &translated_addr, NULL, &local_err)) {
-        error_report_err(local_err);
         goto out_lock;
     }
 
@@ -1244,7 +1245,6 @@  static void vfio_iommu_map_dirty_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
                       "vfio_iommu_map_dirty_notify(%p, 0x%"HWADDR_PRIx", "
                       "0x%"HWADDR_PRIx") failed - ", bcontainer, iova,
                       iotlb->addr_mask + 1);
-        error_report_err(local_err);
     }
 
 out_lock:
@@ -1252,7 +1252,7 @@  out_lock:
 
 out:
     if (ret) {
-        vfio_set_migration_error(ret);
+        vfio_set_migration_error(ret, local_err);
     }
 }
 
@@ -1372,8 +1372,7 @@  static void vfio_listener_log_sync(MemoryListener *listener,
     if (vfio_devices_all_dirty_tracking(bcontainer)) {
         ret = vfio_sync_dirty_bitmap(bcontainer, section, &local_err);
         if (ret) {
-            error_report_err(local_err);
-            vfio_set_migration_error(ret);
+            vfio_set_migration_error(ret, local_err);
         }
     }
 }