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