Re: [PATCH v4 25/25] vfio: Extend vfio_set_migration_error() with Error* argument

[Eric Auger]

Hi Cédric,

On 3/6/24 14:34, 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, if a migration is progress.
> 
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> ---
> 
>  Changes in v4:
> 
>  - Dropped log_global_stop() and log_global_sync() changes
>    
>  hw/vfio/common.c | 39 ++++++++++++++++++++-------------------
>  1 file changed, 20 insertions(+), 19 deletions(-)
> 
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index 
> 65a11dc088524647541db97b7b8d6f07e5044728..e26574617e5ef75c27a84dc9bb13c8f040353b6c
>  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 {
does that case exist?

Eric
> +        error_report_err(err);
>      }
>  }
>  
> @@ -304,9 +306,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;
>      }
>  
> @@ -339,11 +342,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:
> @@ -1125,8 +1129,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);
>      }
>  }
>  
> @@ -1243,14 +1246,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;
>      }
>  
> @@ -1261,7 +1264,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:
> @@ -1269,7 +1271,7 @@ out_lock:
>  
>  out:
>      if (ret) {
> -        vfio_set_migration_error(ret);
> +        vfio_set_migration_error(ret, local_err);
>      }
>  }
>  
> @@ -1389,8 +1391,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);
>          }
>      }
>  }