| Message ID | 20190923065552.10602-2-eric.auger@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Allow memory_region_register_iommu_notifier() to fail | expand |
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; > } > }
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; >> } >> } >
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;
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
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,
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; >
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;
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(-)