[v3,1/2] vfio: Turn the container error into an Error handle
diff mbox series

Message ID 20190923065552.10602-2-eric.auger@redhat.com
State New
Headers show
Series
  • Allow memory_region_register_iommu_notifier() to fail
Related show

Commit Message

Auger Eric Sept. 23, 2019, 6:55 a.m. UTC
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 <eric.auger@redhat.com>
---
 hw/vfio/common.c              | 61 +++++++++++++++++++++--------------
 hw/vfio/spapr.c               |  4 ++-
 include/hw/vfio/vfio-common.h |  2 +-
 3 files changed, 40 insertions(+), 27 deletions(-)

Comments

Peter Xu Sept. 23, 2019, 7:51 a.m. UTC | #1
On Mon, Sep 23, 2019 at 08:55:51AM +0200, 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.

Thanks for working on this.  Mostly good at least to me, though I
still have a few nitpickings below.

> @@ -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;

This line seems to be useless now after we dropped the integer version
of container->error and start to use Error*.

>                  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);
>          ret = -EFAULT;

Same here.

>          goto fail;
>      }

[...]

> @@ -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");
> +        error_reportf_err(err,
> +                          "vfio: DMA mapping failed, unable to continue: ");

Probably need to keep hw_error() because it asserts inside.  Maybe an
error_report_err() before hw_error()?

>      }
>  }
>  
> @@ -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: ");

(I saw that we've got plenty of prepended prefixes for an error
 messages.  For me I'll disgard quite a few of them because the errors
 will directly be delivered to the top level user, but this might be
 too personal as a comment)

Thanks,

>                  goto free_container_exit;
>              }
>          }
Auger Eric Sept. 23, 2019, 11:43 a.m. UTC | #2
Hi Peter,

On 9/23/19 9:51 AM, Peter Xu wrote:
> On Mon, Sep 23, 2019 at 08:55:51AM +0200, 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.
> 
> Thanks for working on this.  Mostly good at least to me, though I
> still have a few nitpickings below.
> 
>> @@ -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;
> 
> This line seems to be useless now after we dropped the integer version
> of container->error and start to use Error*.
correct
> 
>>                  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);
>>          ret = -EFAULT;
> 
> Same here.
OK
> 
>>          goto fail;
>>      }
> 
> [...]
> 
>> @@ -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");
>> +        error_reportf_err(err,
>> +                          "vfio: DMA mapping failed, unable to continue: ");
> 
> Probably need to keep hw_error() because it asserts inside.  Maybe an
> error_report_err() before hw_error()?
that's correct.
> 
>>      }
>>  }
>>  
>> @@ -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: ");
> 
> (I saw that we've got plenty of prepended prefixes for an error
>  messages.  For me I'll disgard quite a few of them because the errors
>  will directly be delivered to the top level user, but this might be
>  too personal as a comment)
That's true we have a lot of prefix messages.

The output message now is:

"vfio 0000:89:00.0: failed
to setup container for group 2: memory listener initialization failed:
Region smmuv3-iommu-memory-region-0-6: device 01.00.0 requires iommu MAP
notifier which is not currently supported"

Alex, any opinion?

Thanks

Eric


> 
> Thanks,
> 
>>                  goto free_container_exit;
>>              }
>>          }
>
Alex Williamson Sept. 23, 2019, 11:05 p.m. UTC | #3
On Mon, 23 Sep 2019 08:55:51 +0200
Eric Auger <eric.auger@redhat.com> 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 <eric.auger@redhat.com>
> ---
>  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.


>      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.

>                  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.

>          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,

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;
Alex Williamson Sept. 23, 2019, 11:10 p.m. UTC | #4
On Mon, 23 Sep 2019 13:43:08 +0200
Auger Eric <eric.auger@redhat.com> wrote:

> On 9/23/19 9:51 AM, Peter Xu wrote:
> > On Mon, Sep 23, 2019 at 08:55:51AM +0200, Eric Auger wrote:  
> >> @@ -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: ");  
> > 
> > (I saw that we've got plenty of prepended prefixes for an error
> >  messages.  For me I'll disgard quite a few of them because the errors
> >  will directly be delivered to the top level user, but this might be
> >  too personal as a comment)  
> That's true we have a lot of prefix messages.
> 
> The output message now is:
> 
> "vfio 0000:89:00.0: failed
> to setup container for group 2: memory listener initialization failed:
> Region smmuv3-iommu-memory-region-0-6: device 01.00.0 requires iommu MAP
> notifier which is not currently supported"
> 
> Alex, any opinion?

Peter, I don't really understand what the comment is here.  Is it the
number of prepends on the error message?  I don't really have an
opinion on that so long as the end message makes sense.  Seems like if
we're familiar with the error generation it helps to unwind the
context.  Thanks,

Alex
Peter Xu Sept. 23, 2019, 11:49 p.m. UTC | #5
On Mon, Sep 23, 2019 at 05:10:43PM -0600, Alex Williamson wrote:
> On Mon, 23 Sep 2019 13:43:08 +0200
> Auger Eric <eric.auger@redhat.com> wrote:
> 
> > On 9/23/19 9:51 AM, Peter Xu wrote:
> > > On Mon, Sep 23, 2019 at 08:55:51AM +0200, Eric Auger wrote:  
> > >> @@ -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: ");  
> > > 
> > > (I saw that we've got plenty of prepended prefixes for an error
> > >  messages.  For me I'll disgard quite a few of them because the errors
> > >  will directly be delivered to the top level user, but this might be
> > >  too personal as a comment)  
> > That's true we have a lot of prefix messages.
> > 
> > The output message now is:
> > 
> > "vfio 0000:89:00.0: failed
> > to setup container for group 2: memory listener initialization failed:
> > Region smmuv3-iommu-memory-region-0-6: device 01.00.0 requires iommu MAP
> > notifier which is not currently supported"
> > 
> > Alex, any opinion?
> 
> Peter, I don't really understand what the comment is here.  Is it the
> number of prepends on the error message?  I don't really have an
> opinion on that so long as the end message makes sense.  Seems like if
> we're familiar with the error generation it helps to unwind the
> context.  Thanks,

True, the only major difference of the error that this series is
working on is that the user can easily trigger this simply by plugging
a device hence I'm not sure whether that's too long (it's not really a
comment and that's why I put it in brackets :).  Let's just keep them.

Thanks,
Auger Eric Sept. 24, 2019, 9:42 a.m. UTC | #6
Hi Alex,

On 9/24/19 1:05 AM, Alex Williamson wrote:
> On Mon, 23 Sep 2019 08:55:51 +0200
> Eric Auger <eric.auger@redhat.com> 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 <eric.auger@redhat.com>
>> ---
>>  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;
>

Patch
diff mbox series

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;
     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;
                 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);
         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");
+        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;