Re: [PATCH v4 1/2] vfio: Turn the container error into an Error handle

[Paolo Bonzini]

On 24/09/19 10:25, Eric Auger wrote:
> The container error integer field is currently used to store
> the first error potentially encountered during any
> vfio_listener_region_add() call. However this fails to propagate
> detailed error messages up to the vfio_connect_container caller.
> Instead of using an integer, let's use an Error handle.
> 
> Messages are slightly reworded to accomodate the propagation.
> 
> Signed-off-by: Eric Auger <address@hidden>
> 
> ---
> 
> v3 -> v4:
> - remove ret useless assignments and restore hw_error()
> - remove mr local variable
> - trace [start, end] instead of [start, size] and improve
>   the message for overalp with existing DMA host window
> ---
>  hw/vfio/common.c              | 43 +++++++++++++++++++++++------------
>  hw/vfio/spapr.c               |  4 +++-
>  include/hw/vfio/vfio-common.h |  2 +-
>  3 files changed, 32 insertions(+), 17 deletions(-)
> 
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index 3e03c495d8..cebbb1c28a 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -509,6 +509,7 @@ static void vfio_listener_region_add(MemoryListener 
> *listener,
>      int ret;
>      VFIOHostDMAWindow *hostwin;
>      bool hostwin_found;
> +    Error *err = NULL;
>  
>      if (vfio_listener_skipped_section(section)) {
>          trace_vfio_listener_region_add_skip(
> @@ -543,13 +544,20 @@ static void vfio_listener_region_add(MemoryListener 
> *listener,
>                                 hostwin->max_iova - hostwin->min_iova + 1,
>                                 section->offset_within_address_space,
>                                 int128_get64(section->size))) {
> -                ret = -1;
> +                error_setg(&err,
> +                    "region [0x%"PRIx64",0x%"PRIx64"] overlaps with existing"
> +                    "host DMA window [0x%"PRIx64",0x%"PRIx64"]",
> +                    section->offset_within_address_space,
> +                    section->offset_within_address_space +
> +                        int128_get64(section->size) - 1,
> +                    hostwin->min_iova, hostwin->max_iova);
>                  goto fail;
>              }
>          }
>  
>          ret = vfio_spapr_create_window(container, section, &pgsize);
>          if (ret) {
> +            error_setg_errno(&err, -ret, "Failed to create SPAPR window");
>              goto fail;
>          }
>  
> @@ -594,10 +602,8 @@ static void vfio_listener_region_add(MemoryListener 
> *listener,
>      }
>  
>      if (!hostwin_found) {
> -        error_report("vfio: IOMMU container %p can't map guest IOVA region"
> -                     " 0x%"HWADDR_PRIx"..0x%"HWADDR_PRIx,
> -                     container, iova, end);
> -        ret = -EFAULT;
> +        error_setg(&err, "Container %p can't map guest IOVA region"
> +                   " 0x%"HWADDR_PRIx"..0x%"HWADDR_PRIx, container, iova, 
> end);
>          goto fail;
>      }
>  
> @@ -664,11 +670,12 @@ static void vfio_listener_region_add(MemoryListener 
> *listener,
>      ret = vfio_dma_map(container, iova, int128_get64(llsize),
>                         vaddr, section->readonly);
>      if (ret) {
> -        error_report("vfio_dma_map(%p, 0x%"HWADDR_PRIx", "
> -                     "0x%"HWADDR_PRIx", %p) = %d (%m)",
> -                     container, iova, int128_get64(llsize), vaddr, ret);
> +        error_setg(&err, "vfio_dma_map(%p, 0x%"HWADDR_PRIx", "
> +                   "0x%"HWADDR_PRIx", %p) = %d (%m)",
> +                   container, iova, int128_get64(llsize), vaddr, ret);
>          if (memory_region_is_ram_device(section->mr)) {
>              /* Allow unexpected mappings not to be fatal for RAM devices */
> +            error_report_err(err);
>              return;
>          }
>          goto fail;
> @@ -688,9 +695,14 @@ fail:
>       */
>      if (!container->initialized) {
>          if (!container->error) {
> -            container->error = ret;
> +            error_propagate_prepend(&container->error, err,
> +                                    "Region %s: ",
> +                                    memory_region_name(section->mr));
> +        } else {
> +            error_free(err);
>          }
>      } else {
> +        error_report_err(err);
>          hw_error("vfio: DMA mapping failed, unable to continue");
>      }
>  }
> @@ -1251,6 +1263,7 @@ static int vfio_connect_container(VFIOGroup *group, 
> AddressSpace *as,
>      container = g_malloc0(sizeof(*container));
>      container->space = space;
>      container->fd = fd;
> +    container->error = NULL;
>      QLIST_INIT(&container->giommu_list);
>      QLIST_INIT(&container->hostwin_list);
>  
> @@ -1308,9 +1321,9 @@ static int vfio_connect_container(VFIOGroup *group, 
> AddressSpace *as,
>                                       &address_space_memory);
>              if (container->error) {
>                  memory_listener_unregister(&container->prereg_listener);
> -                ret = container->error;
> -                error_setg(errp,
> -                    "RAM memory listener initialization failed for 
> container");
> +                ret = -1;
> +                error_propagate_prepend(errp, container->error,
> +                    "RAM memory listener initialization failed: ");
>                  goto free_container_exit;
>              }
>          }
> @@ -1365,9 +1378,9 @@ static int vfio_connect_container(VFIOGroup *group, 
> AddressSpace *as,
>      memory_listener_register(&container->listener, container->space->as);
>  
>      if (container->error) {
> -        ret = container->error;
> -        error_setg_errno(errp, -ret,
> -                         "memory listener initialization failed for 
> container");
> +        ret = -1;
> +        error_propagate_prepend(errp, container->error,
> +            "memory listener initialization failed: ");
>          goto listener_release_exit;
>      }
>  
> diff --git a/hw/vfio/spapr.c b/hw/vfio/spapr.c
> index 96c0ad9d9b..e853eebe11 100644
> --- a/hw/vfio/spapr.c
> +++ b/hw/vfio/spapr.c
> @@ -17,6 +17,7 @@
>  #include "hw/hw.h"
>  #include "exec/ram_addr.h"
>  #include "qemu/error-report.h"
> +#include "qapi/error.h"
>  #include "trace.h"
>  
>  static bool vfio_prereg_listener_skipped_section(MemoryRegionSection 
> *section)
> @@ -85,7 +86,8 @@ static void vfio_prereg_listener_region_add(MemoryListener 
> *listener,
>           */
>          if (!container->initialized) {
>              if (!container->error) {
> -                container->error = ret;
> +                error_setg_errno(&container->error, -ret,
> +                                 "Memory registering failed");
>              }
>          } else {
>              hw_error("vfio: Memory registering failed, unable to continue");
> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> index 9107bd41c0..fd564209ac 100644
> --- a/include/hw/vfio/vfio-common.h
> +++ b/include/hw/vfio/vfio-common.h
> @@ -71,7 +71,7 @@ typedef struct VFIOContainer {
>      MemoryListener listener;
>      MemoryListener prereg_listener;
>      unsigned iommu_type;
> -    int error;
> +    Error *error;
>      bool initialized;
>      unsigned long pgsizes;
>      QLIST_HEAD(, VFIOGuestIOMMU) giommu_list;
> 

Acked-by: Paolo Bonzini <address@hidden>