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

[Auger Eric]

Hi Alex,

On 9/24/19 1:05 AM, Alex Williamson wrote:
> On Mon, 23 Sep 2019 08:55:51 +0200
> Eric Auger <address@hidden> 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>
>> ---
>>  hw/vfio/common.c              | 61 +++++++++++++++++++++--------------
>>  hw/vfio/spapr.c               |  4 ++-
>>  include/hw/vfio/vfio-common.h |  2 +-
>>  3 files changed, 40 insertions(+), 27 deletions(-)
>>
>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>> index 3e03c495d8..a0670cc63a 100644
>> --- a/hw/vfio/common.c
>> +++ b/hw/vfio/common.c
>> @@ -503,12 +503,14 @@ static void vfio_listener_region_add(MemoryListener 
>> *listener,
>>                                       MemoryRegionSection *section)
>>  {
>>      VFIOContainer *container = container_of(listener, VFIOContainer, 
>> listener);
>> +    MemoryRegion *mr = section->mr;
> 
> This looks like an entirely secondary change that obscures the primary
> purpose of this patch and isn't mentioned in the changelog.  It's also a
> bit inconsistent in places where we're references section->size and
> section->offset_within_address_space, but now mr instead of section->mr.
OK. I removed it.
> 
> 
>>      hwaddr iova, end;
>>      Int128 llend, llsize;
>>      void *vaddr;
>>      int ret;
>>      VFIOHostDMAWindow *hostwin;
>>      bool hostwin_found;
>> +    Error *err = NULL;
>>  
>>      if (vfio_listener_skipped_section(section)) {
>>          trace_vfio_listener_region_add_skip(
>> @@ -543,6 +545,9 @@ static void vfio_listener_region_add(MemoryListener 
>> *listener,
>>                                 hostwin->max_iova - hostwin->min_iova + 1,
>>                                 section->offset_within_address_space,
>>                                 int128_get64(section->size))) {
>> +                error_setg(&err, "Overlap with existing IOMMU range "
>> +                                 "[0x%"PRIx64",0x%"PRIx64"]", 
>> hostwin->min_iova,
>> +                                 hostwin->max_iova - hostwin->min_iova + 1);
>>                  ret = -1;
> 
> Agree with Peter here, we should no longer be gratuitously setting ret
> when it's not consumed.
> 
> Alexey or David might want to comment on the error message here since
> we didn't have one previously, but we're only providing half the story
> above, the existing window that interferes but not the range we
> attempted to add that it interferes with.
Now both the new range and the existing window are output
> 
>>                  goto fail;
>>              }
>> @@ -550,6 +555,7 @@ static void vfio_listener_region_add(MemoryListener 
>> *listener,
>>  
>>          ret = vfio_spapr_create_window(container, section, &pgsize);
>>          if (ret) {
>> +            error_setg_errno(&err, -ret, "Failed to create SPAPR window");
>>              goto fail;
>>          }
>>  
>> @@ -559,7 +565,7 @@ static void vfio_listener_region_add(MemoryListener 
>> *listener,
>>  #ifdef CONFIG_KVM
>>          if (kvm_enabled()) {
>>              VFIOGroup *group;
>> -            IOMMUMemoryRegion *iommu_mr = IOMMU_MEMORY_REGION(section->mr);
>> +            IOMMUMemoryRegion *iommu_mr = IOMMU_MEMORY_REGION(mr);
>>              struct kvm_vfio_spapr_tce param;
>>              struct kvm_device_attr attr = {
>>                  .group = KVM_DEV_VFIO_GROUP,
>> @@ -594,18 +600,17 @@ 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);
>> +        error_setg(&err, "Container %p can't map guest IOVA region"
>> +                   " 0x%"HWADDR_PRIx"..0x%"HWADDR_PRIx, container, iova, 
>> end);
> 
> Note that here we print the start and end addresses, so I'm not sure
> why we chose to print [start,size] in the new message commented on
> above.
fixed
> 
>>          ret = -EFAULT;
>>          goto fail;
>>      }
>>  
>> -    memory_region_ref(section->mr);
>> +    memory_region_ref(mr);
>>  
>> -    if (memory_region_is_iommu(section->mr)) {
>> +    if (memory_region_is_iommu(mr)) {
>>          VFIOGuestIOMMU *giommu;
>> -        IOMMUMemoryRegion *iommu_mr = IOMMU_MEMORY_REGION(section->mr);
>> +        IOMMUMemoryRegion *iommu_mr = IOMMU_MEMORY_REGION(mr);
>>          int iommu_idx;
>>  
>>          trace_vfio_listener_region_add_iommu(iova, end);
>> @@ -632,15 +637,15 @@ static void vfio_listener_region_add(MemoryListener 
>> *listener,
>>                              iommu_idx);
>>          QLIST_INSERT_HEAD(&container->giommu_list, giommu, giommu_next);
>>  
>> -        memory_region_register_iommu_notifier(section->mr, &giommu->n);
>> +        memory_region_register_iommu_notifier(mr, &giommu->n);
>>          memory_region_iommu_replay(giommu->iommu, &giommu->n);
>>  
>>          return;
>>      }
>>  
>> -    /* Here we assume that memory_region_is_ram(section->mr)==true */
>> +    /* Here we assume that memory_region_is_ram(mr)==true */
>>  
>> -    vaddr = memory_region_get_ram_ptr(section->mr) +
>> +    vaddr = memory_region_get_ram_ptr(mr) +
>>              section->offset_within_region +
>>              (iova - section->offset_within_address_space);
>>  
>> @@ -648,12 +653,12 @@ static void vfio_listener_region_add(MemoryListener 
>> *listener,
>>  
>>      llsize = int128_sub(llend, int128_make64(iova));
>>  
>> -    if (memory_region_is_ram_device(section->mr)) {
>> +    if (memory_region_is_ram_device(mr)) {
>>          hwaddr pgmask = (1ULL << ctz64(hostwin->iova_pgsizes)) - 1;
>>  
>>          if ((iova & pgmask) || (int128_get64(llsize) & pgmask)) {
>>              trace_vfio_listener_region_add_no_dma_map(
>> -                memory_region_name(section->mr),
>> +                memory_region_name(mr),
>>                  section->offset_within_address_space,
>>                  int128_getlo(section->size),
>>                  pgmask + 1);
>> @@ -664,11 +669,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);
>> -        if (memory_region_is_ram_device(section->mr)) {
>> +        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(mr)) {
>>              /* Allow unexpected mappings not to be fatal for RAM devices */
>> +            error_report_err(err);
>>              return;
>>          }
>>          goto fail;
>> @@ -677,7 +683,7 @@ static void vfio_listener_region_add(MemoryListener 
>> *listener,
>>      return;
>>  
>>  fail:
>> -    if (memory_region_is_ram_device(section->mr)) {
>> +    if (memory_region_is_ram_device(mr)) {
>>          error_report("failed to vfio_dma_map. pci p2p may not work");
>>          return;
>>      }
>> @@ -688,10 +694,14 @@ fail:
>>       */
>>      if (!container->initialized) {
>>          if (!container->error) {
>> -            container->error = ret;
>> +            error_propagate_prepend(&container->error, err,
>> +                                    "Region %s: ", memory_region_name(mr));
>> +        } else {
>> +            error_free(err);
>>          }
>>      } else {
>> -        hw_error("vfio: DMA mapping failed, unable to continue");
> 
> As Peter notes, this removal is troubling.  Thanks,
Corrected.

Thanks

Eric
> 
> Alex
> 
>> +        error_reportf_err(err,
>> +                          "vfio: DMA mapping failed, unable to continue: ");
>>      }
>>  }
>>  
>> @@ -1251,6 +1261,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 +1319,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 +1376,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;
>