* [PATCH v3 0/2] Allow memory_region_register_iommu_notifier() to fail
@ 2019-09-23 6:55 Eric Auger
2019-09-23 6:55 ` [PATCH v3 1/2] vfio: Turn the container error into an Error handle Eric Auger
2019-09-23 6:55 ` [PATCH v3 2/2] memory: allow memory_region_register_iommu_notifier() to fail Eric Auger
0 siblings, 2 replies; 10+ messages in thread
From: Eric Auger @ 2019-09-23 6:55 UTC (permalink / raw)
To: eric.auger.pro, eric.auger, qemu-devel, qemu-arm, peter.maydell,
pbonzini, alex.williamson, peterx, aik, david
Cc: mst
This series allows the memory_region_register_iommu_notifier()
to fail. As of now, when a MAP notifier is attempted to be
registered along with SMMUv3 or AMD IOMMU, we exit in the IOMMU
MR notify_flag_changed() callback.
In case of VFIO assigned device hotplug, this could be handled
more nicely directly within the VFIO code, simply rejecting
the hotplug without exiting. This is what the series achieves
by handling the memory_region_register_iommu_notifier() returned
value and Error object.
To propagate errors collected during vfio_listener_region_add()
we now store the error handle inside the VFIO container instead
of a returned value.
The message now is:
(QEMU) device_add id=hot0 driver=vfio-pci host=0000:89:00.0 bus=pcie.1
{"error": {"class": "GenericError", "desc": "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"}}
Best Regards
Eric
This series can be found at:
https://github.com/eauger/qemu/tree/v4.1.0_register_iommu_notifier_fail_v3
History:
v2 -> v3:
- also pass an Error handle (suggested by Peter)
- VFIO container error becomes and Error handle
v1 -> v2:
- Intel IOMMU now handles the problem differently with machine init done
notifier and machine hotplug allowed hook.
- use assert(!ret)
- message rewording in SMMUv3
Follow-up of "VFIO/SMMUv3: Fail on VFIO/HW nested paging detection"
https://patchew.org/QEMU/20190829090141.21821-1-eric.auger@redhat.com/
Eric Auger (2):
vfio: Turn the container error into an Error handle
memory: allow memory_region_register_iommu_notifier() to fail
exec.c | 10 ++++--
hw/arm/smmuv3.c | 18 +++++-----
hw/i386/amd_iommu.c | 17 +++++----
hw/i386/intel_iommu.c | 8 +++--
hw/ppc/spapr_iommu.c | 8 +++--
hw/vfio/common.c | 67 +++++++++++++++++++++--------------
hw/vfio/spapr.c | 4 ++-
hw/virtio/vhost.c | 9 +++--
include/exec/memory.h | 21 ++++++++---
include/hw/vfio/vfio-common.h | 2 +-
memory.c | 31 ++++++++++------
11 files changed, 126 insertions(+), 69 deletions(-)
--
2.20.1
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH v3 1/2] vfio: Turn the container error into an Error handle 2019-09-23 6:55 [PATCH v3 0/2] Allow memory_region_register_iommu_notifier() to fail Eric Auger @ 2019-09-23 6:55 ` Eric Auger 2019-09-23 7:51 ` Peter Xu 2019-09-23 23:05 ` Alex Williamson 2019-09-23 6:55 ` [PATCH v3 2/2] memory: allow memory_region_register_iommu_notifier() to fail Eric Auger 1 sibling, 2 replies; 10+ messages in thread From: Eric Auger @ 2019-09-23 6:55 UTC (permalink / raw) To: eric.auger.pro, eric.auger, qemu-devel, qemu-arm, peter.maydell, pbonzini, alex.williamson, peterx, aik, david Cc: mst 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; 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; -- 2.20.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v3 1/2] vfio: Turn the container error into an Error handle 2019-09-23 6:55 ` [PATCH v3 1/2] vfio: Turn the container error into an Error handle Eric Auger @ 2019-09-23 7:51 ` Peter Xu 2019-09-23 11:43 ` Auger Eric 2019-09-23 23:05 ` Alex Williamson 1 sibling, 1 reply; 10+ messages in thread From: Peter Xu @ 2019-09-23 7:51 UTC (permalink / raw) To: Eric Auger Cc: peter.maydell, mst, aik, qemu-devel, alex.williamson, qemu-arm, pbonzini, david, eric.auger.pro 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3 1/2] vfio: Turn the container error into an Error handle 2019-09-23 7:51 ` Peter Xu @ 2019-09-23 11:43 ` Auger Eric 2019-09-23 23:10 ` Alex Williamson 0 siblings, 1 reply; 10+ messages in thread From: Auger Eric @ 2019-09-23 11:43 UTC (permalink / raw) To: Peter Xu Cc: peter.maydell, mst, aik, qemu-devel, alex.williamson, qemu-arm, pbonzini, david, eric.auger.pro 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; >> } >> } > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3 1/2] vfio: Turn the container error into an Error handle 2019-09-23 11:43 ` Auger Eric @ 2019-09-23 23:10 ` Alex Williamson 2019-09-23 23:49 ` Peter Xu 0 siblings, 1 reply; 10+ messages in thread From: Alex Williamson @ 2019-09-23 23:10 UTC (permalink / raw) To: Auger Eric Cc: peter.maydell, mst, aik, qemu-devel, Peter Xu, qemu-arm, pbonzini, david, eric.auger.pro 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3 1/2] vfio: Turn the container error into an Error handle 2019-09-23 23:10 ` Alex Williamson @ 2019-09-23 23:49 ` Peter Xu 0 siblings, 0 replies; 10+ messages in thread From: Peter Xu @ 2019-09-23 23:49 UTC (permalink / raw) To: Alex Williamson Cc: peter.maydell, mst, aik, qemu-devel, Auger Eric, qemu-arm, pbonzini, david, eric.auger.pro 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, -- Peter Xu ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3 1/2] vfio: Turn the container error into an Error handle 2019-09-23 6:55 ` [PATCH v3 1/2] vfio: Turn the container error into an Error handle Eric Auger 2019-09-23 7:51 ` Peter Xu @ 2019-09-23 23:05 ` Alex Williamson 2019-09-24 9:42 ` Auger Eric 1 sibling, 1 reply; 10+ messages in thread From: Alex Williamson @ 2019-09-23 23:05 UTC (permalink / raw) To: Eric Auger Cc: peter.maydell, mst, aik, qemu-devel, peterx, qemu-arm, pbonzini, david, eric.auger.pro 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; ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3 1/2] vfio: Turn the container error into an Error handle 2019-09-23 23:05 ` Alex Williamson @ 2019-09-24 9:42 ` Auger Eric 0 siblings, 0 replies; 10+ messages in thread From: Auger Eric @ 2019-09-24 9:42 UTC (permalink / raw) To: Alex Williamson Cc: peter.maydell, mst, aik, qemu-devel, peterx, qemu-arm, pbonzini, david, eric.auger.pro 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; > ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v3 2/2] memory: allow memory_region_register_iommu_notifier() to fail 2019-09-23 6:55 [PATCH v3 0/2] Allow memory_region_register_iommu_notifier() to fail Eric Auger 2019-09-23 6:55 ` [PATCH v3 1/2] vfio: Turn the container error into an Error handle Eric Auger @ 2019-09-23 6:55 ` Eric Auger 2019-09-23 7:59 ` Peter Xu 1 sibling, 1 reply; 10+ messages in thread From: Eric Auger @ 2019-09-23 6:55 UTC (permalink / raw) To: eric.auger.pro, eric.auger, qemu-devel, qemu-arm, peter.maydell, pbonzini, alex.williamson, peterx, aik, david Cc: mst Currently, when a notifier is attempted to be registered and its flags are not supported (especially the MAP one) by the IOMMU MR, we generally abruptly exit in the IOMMU code. The failure could be handled more nicely in the caller and especially in the VFIO code. So let's allow memory_region_register_iommu_notifier() to fail as well as notify_flag_changed() callback. All sites implementing the callback are updated. This patch does not yet remove the exit(1) in the amd_iommu code. in SMMUv3 we turn the warning message into an error message saying that the assigned device would not work properly. Signed-off-by: Eric Auger <eric.auger@redhat.com> --- v2 -> v3: - turn the warning message into an error message --- exec.c | 10 ++++++++-- hw/arm/smmuv3.c | 18 ++++++++++-------- hw/i386/amd_iommu.c | 17 ++++++++++------- hw/i386/intel_iommu.c | 8 +++++--- hw/ppc/spapr_iommu.c | 8 +++++--- hw/vfio/common.c | 8 ++++++-- hw/virtio/vhost.c | 9 +++++++-- include/exec/memory.h | 21 ++++++++++++++++----- memory.c | 31 ++++++++++++++++++++----------- 9 files changed, 87 insertions(+), 43 deletions(-) diff --git a/exec.c b/exec.c index 8b998974f8..7379330756 100644 --- a/exec.c +++ b/exec.c @@ -663,7 +663,8 @@ static void tcg_register_iommu_notifier(CPUState *cpu, */ MemoryRegion *mr = MEMORY_REGION(iommu_mr); TCGIOMMUNotifier *notifier; - int i; + Error *err = NULL; + int i, ret; for (i = 0; i < cpu->iommu_notifiers->len; i++) { notifier = g_array_index(cpu->iommu_notifiers, TCGIOMMUNotifier *, i); @@ -692,7 +693,12 @@ static void tcg_register_iommu_notifier(CPUState *cpu, 0, HWADDR_MAX, iommu_idx); - memory_region_register_iommu_notifier(notifier->mr, ¬ifier->n); + ret = memory_region_register_iommu_notifier(notifier->mr, ¬ifier->n, + &err); + if (ret) { + error_report_err(err); + exit(1); + } } if (!notifier->active) { diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c index db051dcac8..e2fbb8357e 100644 --- a/hw/arm/smmuv3.c +++ b/hw/arm/smmuv3.c @@ -1469,20 +1469,21 @@ static void smmuv3_class_init(ObjectClass *klass, void *data) dc->realize = smmu_realize; } -static void smmuv3_notify_flag_changed(IOMMUMemoryRegion *iommu, - IOMMUNotifierFlag old, - IOMMUNotifierFlag new) +static int smmuv3_notify_flag_changed(IOMMUMemoryRegion *iommu, + IOMMUNotifierFlag old, + IOMMUNotifierFlag new, + Error **errp) { SMMUDevice *sdev = container_of(iommu, SMMUDevice, iommu); SMMUv3State *s3 = sdev->smmu; SMMUState *s = &(s3->smmu_state); if (new & IOMMU_NOTIFIER_MAP) { - int bus_num = pci_bus_num(sdev->bus); - PCIDevice *pcidev = pci_find_device(sdev->bus, bus_num, sdev->devfn); - - warn_report("SMMUv3 does not support notification on MAP: " - "device %s will not function properly", pcidev->name); + error_setg(errp, + "device %02x.%02x.%x requires iommu MAP notifier which is " + "not currently supported", pci_bus_num(sdev->bus), + PCI_SLOT(sdev->devfn), PCI_FUNC(sdev->devfn)); + return -EINVAL; } if (old == IOMMU_NOTIFIER_NONE) { @@ -1492,6 +1493,7 @@ static void smmuv3_notify_flag_changed(IOMMUMemoryRegion *iommu, trace_smmuv3_notify_flag_del(iommu->parent_obj.name); QLIST_REMOVE(sdev, next); } + return 0; } static void smmuv3_iommu_memory_region_class_init(ObjectClass *klass, diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c index 08884523e2..d3726361dd 100644 --- a/hw/i386/amd_iommu.c +++ b/hw/i386/amd_iommu.c @@ -1466,18 +1466,21 @@ static const MemoryRegionOps mmio_mem_ops = { } }; -static void amdvi_iommu_notify_flag_changed(IOMMUMemoryRegion *iommu, - IOMMUNotifierFlag old, - IOMMUNotifierFlag new) +static int amdvi_iommu_notify_flag_changed(IOMMUMemoryRegion *iommu, + IOMMUNotifierFlag old, + IOMMUNotifierFlag new, + Error **errp) { AMDVIAddressSpace *as = container_of(iommu, AMDVIAddressSpace, iommu); if (new & IOMMU_NOTIFIER_MAP) { - error_report("device %02x.%02x.%x requires iommu notifier which is not " - "currently supported", as->bus_num, PCI_SLOT(as->devfn), - PCI_FUNC(as->devfn)); - exit(1); + error_setg(errp, + "device %02x.%02x.%x requires iommu notifier which is not " + "currently supported", as->bus_num, PCI_SLOT(as->devfn), + PCI_FUNC(as->devfn)); + return -EINVAL; } + return 0; } static void amdvi_init(AMDVIState *s) diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c index f1de8fdb75..771bed25c9 100644 --- a/hw/i386/intel_iommu.c +++ b/hw/i386/intel_iommu.c @@ -2929,9 +2929,10 @@ static IOMMUTLBEntry vtd_iommu_translate(IOMMUMemoryRegion *iommu, hwaddr addr, return iotlb; } -static void vtd_iommu_notify_flag_changed(IOMMUMemoryRegion *iommu, - IOMMUNotifierFlag old, - IOMMUNotifierFlag new) +static int vtd_iommu_notify_flag_changed(IOMMUMemoryRegion *iommu, + IOMMUNotifierFlag old, + IOMMUNotifierFlag new, + Error **errp) { VTDAddressSpace *vtd_as = container_of(iommu, VTDAddressSpace, iommu); IntelIOMMUState *s = vtd_as->iommu_state; @@ -2944,6 +2945,7 @@ static void vtd_iommu_notify_flag_changed(IOMMUMemoryRegion *iommu, } else if (new == IOMMU_NOTIFIER_NONE) { QLIST_REMOVE(vtd_as, next); } + return 0; } static int vtd_post_load(void *opaque, int version_id) diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c index e87b3d50f7..3d3bcc8649 100644 --- a/hw/ppc/spapr_iommu.c +++ b/hw/ppc/spapr_iommu.c @@ -205,9 +205,10 @@ static int spapr_tce_get_attr(IOMMUMemoryRegion *iommu, return -EINVAL; } -static void spapr_tce_notify_flag_changed(IOMMUMemoryRegion *iommu, - IOMMUNotifierFlag old, - IOMMUNotifierFlag new) +static int spapr_tce_notify_flag_changed(IOMMUMemoryRegion *iommu, + IOMMUNotifierFlag old, + IOMMUNotifierFlag new, + Error **errp) { struct SpaprTceTable *tbl = container_of(iommu, SpaprTceTable, iommu); @@ -216,6 +217,7 @@ static void spapr_tce_notify_flag_changed(IOMMUMemoryRegion *iommu, } else if (old != IOMMU_NOTIFIER_NONE && new == IOMMU_NOTIFIER_NONE) { spapr_tce_set_need_vfio(tbl, false); } + return 0; } static int spapr_tce_table_post_load(void *opaque, int version_id) diff --git a/hw/vfio/common.c b/hw/vfio/common.c index a0670cc63a..11312fc02f 100644 --- a/hw/vfio/common.c +++ b/hw/vfio/common.c @@ -635,9 +635,13 @@ static void vfio_listener_region_add(MemoryListener *listener, section->offset_within_region, int128_get64(llend), iommu_idx); - QLIST_INSERT_HEAD(&container->giommu_list, giommu, giommu_next); - memory_region_register_iommu_notifier(mr, &giommu->n); + ret = memory_region_register_iommu_notifier(mr, &giommu->n, &err); + if (ret) { + g_free(giommu); + goto fail; + } + QLIST_INSERT_HEAD(&container->giommu_list, giommu, giommu_next); memory_region_iommu_replay(giommu->iommu, &giommu->n); return; diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c index 34accdf615..9a65f034a5 100644 --- a/hw/virtio/vhost.c +++ b/hw/virtio/vhost.c @@ -672,8 +672,9 @@ static void vhost_iommu_region_add(MemoryListener *listener, iommu_listener); struct vhost_iommu *iommu; Int128 end; - int iommu_idx; + int iommu_idx, ret; IOMMUMemoryRegion *iommu_mr; + Error *err = NULL; if (!memory_region_is_iommu(section->mr)) { return; @@ -696,7 +697,11 @@ static void vhost_iommu_region_add(MemoryListener *listener, iommu->iommu_offset = section->offset_within_address_space - section->offset_within_region; iommu->hdev = dev; - memory_region_register_iommu_notifier(section->mr, &iommu->n); + ret = memory_region_register_iommu_notifier(section->mr, &iommu->n, &err); + if (ret) { + error_report_err(err); + exit(1); + } QLIST_INSERT_HEAD(&dev->iommu_list, iommu, iommu_next); /* TODO: can replay help performance here? */ } diff --git a/include/exec/memory.h b/include/exec/memory.h index a30245c25a..c16430726c 100644 --- a/include/exec/memory.h +++ b/include/exec/memory.h @@ -288,10 +288,16 @@ typedef struct IOMMUMemoryRegionClass { * @iommu: the IOMMUMemoryRegion * @old_flags: events which previously needed to be notified * @new_flags: events which now need to be notified + * + * Returns 0 on success, or a negative errno; in particular + * returns -EINVAL if the new flag bitmap is not supported by the + * IOMMU memory region. In case of failure, the error object + * must be created */ - void (*notify_flag_changed)(IOMMUMemoryRegion *iommu, - IOMMUNotifierFlag old_flags, - IOMMUNotifierFlag new_flags); + int (*notify_flag_changed)(IOMMUMemoryRegion *iommu, + IOMMUNotifierFlag old_flags, + IOMMUNotifierFlag new_flags, + Error **errp); /* Called to handle memory_region_iommu_replay(). * * The default implementation of memory_region_iommu_replay() is to @@ -1067,13 +1073,18 @@ void memory_region_notify_one(IOMMUNotifier *notifier, * memory_region_register_iommu_notifier: register a notifier for changes to * IOMMU translation entries. * + * Returns 0 on success, or a negative errno otherwise. In particular, + * -EINVAL indicates that at least one of the attributes of the notifier + * is not supported (flag/range) by the IOMMU memory region. In case of error + * the error object must be created. + * * @mr: the memory region to observe * @n: the IOMMUNotifier to be added; the notify callback receives a * pointer to an #IOMMUTLBEntry as the opaque value; the pointer * ceases to be valid on exit from the notifier. */ -void memory_region_register_iommu_notifier(MemoryRegion *mr, - IOMMUNotifier *n); +int memory_region_register_iommu_notifier(MemoryRegion *mr, + IOMMUNotifier *n, Error **errp); /** * memory_region_iommu_replay: replay existing IOMMU translations to diff --git a/memory.c b/memory.c index b9dd6b94ca..6df84a4f9c 100644 --- a/memory.c +++ b/memory.c @@ -1837,33 +1837,38 @@ bool memory_region_is_logging(MemoryRegion *mr, uint8_t client) return memory_region_get_dirty_log_mask(mr) & (1 << client); } -static void memory_region_update_iommu_notify_flags(IOMMUMemoryRegion *iommu_mr) +static int memory_region_update_iommu_notify_flags(IOMMUMemoryRegion *iommu_mr, + Error **errp) { IOMMUNotifierFlag flags = IOMMU_NOTIFIER_NONE; IOMMUNotifier *iommu_notifier; IOMMUMemoryRegionClass *imrc = IOMMU_MEMORY_REGION_GET_CLASS(iommu_mr); + int ret = 0; IOMMU_NOTIFIER_FOREACH(iommu_notifier, iommu_mr) { flags |= iommu_notifier->notifier_flags; } if (flags != iommu_mr->iommu_notify_flags && imrc->notify_flag_changed) { - imrc->notify_flag_changed(iommu_mr, - iommu_mr->iommu_notify_flags, - flags); + ret = imrc->notify_flag_changed(iommu_mr, + iommu_mr->iommu_notify_flags, + flags, errp); } - iommu_mr->iommu_notify_flags = flags; + if (!ret) { + iommu_mr->iommu_notify_flags = flags; + } + return ret; } -void memory_region_register_iommu_notifier(MemoryRegion *mr, - IOMMUNotifier *n) +int memory_region_register_iommu_notifier(MemoryRegion *mr, + IOMMUNotifier *n, Error **errp) { IOMMUMemoryRegion *iommu_mr; + int ret; if (mr->alias) { - memory_region_register_iommu_notifier(mr->alias, n); - return; + return memory_region_register_iommu_notifier(mr->alias, n, errp); } /* We need to register for at least one bitfield */ @@ -1874,7 +1879,11 @@ void memory_region_register_iommu_notifier(MemoryRegion *mr, n->iommu_idx < memory_region_iommu_num_indexes(iommu_mr)); QLIST_INSERT_HEAD(&iommu_mr->iommu_notify, n, node); - memory_region_update_iommu_notify_flags(iommu_mr); + ret = memory_region_update_iommu_notify_flags(iommu_mr, errp); + if (ret) { + QLIST_REMOVE(n, node); + } + return ret; } uint64_t memory_region_iommu_get_min_page_size(IOMMUMemoryRegion *iommu_mr) @@ -1927,7 +1936,7 @@ void memory_region_unregister_iommu_notifier(MemoryRegion *mr, } QLIST_REMOVE(n, node); iommu_mr = IOMMU_MEMORY_REGION(mr); - memory_region_update_iommu_notify_flags(iommu_mr); + memory_region_update_iommu_notify_flags(iommu_mr, NULL); } void memory_region_notify_one(IOMMUNotifier *notifier, -- 2.20.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v3 2/2] memory: allow memory_region_register_iommu_notifier() to fail 2019-09-23 6:55 ` [PATCH v3 2/2] memory: allow memory_region_register_iommu_notifier() to fail Eric Auger @ 2019-09-23 7:59 ` Peter Xu 0 siblings, 0 replies; 10+ messages in thread From: Peter Xu @ 2019-09-23 7:59 UTC (permalink / raw) To: Eric Auger Cc: peter.maydell, mst, aik, qemu-devel, alex.williamson, qemu-arm, pbonzini, david, eric.auger.pro On Mon, Sep 23, 2019 at 08:55:52AM +0200, Eric Auger wrote: > Currently, when a notifier is attempted to be registered and its > flags are not supported (especially the MAP one) by the IOMMU MR, > we generally abruptly exit in the IOMMU code. The failure could be > handled more nicely in the caller and especially in the VFIO code. > > So let's allow memory_region_register_iommu_notifier() to fail as > well as notify_flag_changed() callback. > > All sites implementing the callback are updated. This patch does > not yet remove the exit(1) in the amd_iommu code. > > in SMMUv3 we turn the warning message into an error message saying > that the assigned device would not work properly. > > Signed-off-by: Eric Auger <eric.auger@redhat.com> Reviewed-by: Peter Xu <peterx@redhat.com> -- Peter Xu ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2019-09-24 9:44 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-09-23 6:55 [PATCH v3 0/2] Allow memory_region_register_iommu_notifier() to fail Eric Auger 2019-09-23 6:55 ` [PATCH v3 1/2] vfio: Turn the container error into an Error handle Eric Auger 2019-09-23 7:51 ` Peter Xu 2019-09-23 11:43 ` Auger Eric 2019-09-23 23:10 ` Alex Williamson 2019-09-23 23:49 ` Peter Xu 2019-09-23 23:05 ` Alex Williamson 2019-09-24 9:42 ` Auger Eric 2019-09-23 6:55 ` [PATCH v3 2/2] memory: allow memory_region_register_iommu_notifier() to fail Eric Auger 2019-09-23 7:59 ` Peter Xu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).