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.au...@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 <pbonz...@redhat.com>
