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;
> }
> }
--
Peter Xu
