From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46118) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1afiTy-0006m7-4y for qemu-devel@nongnu.org; Tue, 15 Mar 2016 02:29:32 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1afiTu-0006Nv-Rr for qemu-devel@nongnu.org; Tue, 15 Mar 2016 02:29:30 -0400 Date: Tue, 15 Mar 2016 16:42:30 +1100 From: David Gibson Message-ID: <20160315054230.GH15272@voom.fritz.box> References: <1456823441-46757-1-git-send-email-aik@ozlabs.ru> <1456823441-46757-12-git-send-email-aik@ozlabs.ru> <20160303063013.GL1620@voom.redhat.com> <56E7793C.3000209@ozlabs.ru> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="UTZ8bGhNySVQ9LYl" Content-Disposition: inline In-Reply-To: <56E7793C.3000209@ozlabs.ru> Subject: Re: [Qemu-devel] [Qemu-ppc] [PATCH qemu v13 11/16] vfio: spapr: Add SPAPR IOMMU v2 support (DMA memory preregistering) List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexey Kardashevskiy Cc: Alex Williamson , qemu-ppc@nongnu.org, qemu-devel@nongnu.org --UTZ8bGhNySVQ9LYl Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Mar 15, 2016 at 01:53:48PM +1100, Alexey Kardashevskiy wrote: > On 03/03/2016 05:30 PM, David Gibson wrote: > >On Tue, Mar 01, 2016 at 08:10:36PM +1100, Alexey Kardashevskiy wrote: > >>This makes use of the new "memory registering" feature. The idea is > >>to provide the userspace ability to notify the host kernel about pages > >>which are going to be used for DMA. Having this information, the host > >>kernel can pin them all once per user process, do locked pages > >>accounting (once) and not spent time on doing that in real time with > >>possible failures which cannot be handled nicely in some cases. > >> > >>This adds a prereg memory listener which listens on address_space_memory > >>and notifies a VFIO container about memory which needs to be > >>pinned/unpinned. VFIO MMIO regions (i.e. "skip dump" regions) are skipp= ed. > >> > >>As there is no per-IOMMU-type release() callback anymore, this stores > >>the IOMMU type in the container so vfio_listener_release() can device > >>if it needs to unregister @prereg_listener. > >> > >>The feature is only enabled for SPAPR IOMMU v2. The host kernel changes > >>are required. Since v2 does not need/support VFIO_IOMMU_ENABLE, this do= es > >>not call it when v2 is detected and enabled. > >> > >>This does not change the guest visible interface. > >> > >>Signed-off-by: Alexey Kardashevskiy > >>--- > >> hw/vfio/Makefile.objs | 1 + > >> hw/vfio/common.c | 39 +++++++++--- > >> hw/vfio/prereg.c | 138 +++++++++++++++++++++++++++++++++= +++++++++ > >> include/hw/vfio/vfio-common.h | 4 ++ > >> trace-events | 2 + > >> 5 files changed, 175 insertions(+), 9 deletions(-) > >> create mode 100644 hw/vfio/prereg.c > >> > >>diff --git a/hw/vfio/Makefile.objs b/hw/vfio/Makefile.objs > >>index ceddbb8..5800e0e 100644 > >>--- a/hw/vfio/Makefile.objs > >>+++ b/hw/vfio/Makefile.objs > >>@@ -4,4 +4,5 @@ obj-$(CONFIG_PCI) +=3D pci.o pci-quirks.o > >> obj-$(CONFIG_SOFTMMU) +=3D platform.o > >> obj-$(CONFIG_SOFTMMU) +=3D calxeda-xgmac.o > >> obj-$(CONFIG_SOFTMMU) +=3D amd-xgbe.o > >>+obj-$(CONFIG_SOFTMMU) +=3D prereg.o > >> endif > >>diff --git a/hw/vfio/common.c b/hw/vfio/common.c > >>index 3aaa6b5..f2a03e0 100644 > >>--- a/hw/vfio/common.c > >>+++ b/hw/vfio/common.c > >>@@ -531,6 +531,9 @@ static const MemoryListener vfio_iommu_listener =3D= { > >> static void vfio_listener_release(VFIOContainer *container) > >> { > >> memory_listener_unregister(&container->iommu_listener.listener); > >>+ if (container->iommu_type =3D=3D VFIO_SPAPR_TCE_v2_IOMMU) { > >>+ memory_listener_unregister(&container->prereg_listener.listene= r); > >>+ } > >> } > >> > >> int vfio_mmap_region(Object *obj, VFIORegion *region, > >>@@ -722,8 +725,8 @@ static int vfio_connect_container(VFIOGroup *group,= AddressSpace *as) > >> goto free_container_exit; > >> } > >> > >>- ret =3D ioctl(fd, VFIO_SET_IOMMU, > >>- v2 ? VFIO_TYPE1v2_IOMMU : VFIO_TYPE1_IOMMU); > >>+ container->iommu_type =3D v2 ? VFIO_TYPE1v2_IOMMU : VFIO_TYPE1= _IOMMU; > >>+ ret =3D ioctl(fd, VFIO_SET_IOMMU, container->iommu_type); > >> if (ret) { > >> error_report("vfio: failed to set iommu for container: %m= "); > >> ret =3D -errno; > >>@@ -748,8 +751,10 @@ static int vfio_connect_container(VFIOGroup *group= , AddressSpace *as) > >> if ((ret =3D=3D 0) && (info.flags & VFIO_IOMMU_INFO_PGSIZES))= { > >> container->iova_pgsizes =3D info.iova_pgsizes; > >> } > >>- } else if (ioctl(fd, VFIO_CHECK_EXTENSION, VFIO_SPAPR_TCE_IOMMU)) { > >>+ } else if (ioctl(fd, VFIO_CHECK_EXTENSION, VFIO_SPAPR_TCE_IOMMU) || > >>+ ioctl(fd, VFIO_CHECK_EXTENSION, VFIO_SPAPR_TCE_v2_IOMMU= )) { > >> struct vfio_iommu_spapr_tce_info info; > >>+ bool v2 =3D !!ioctl(fd, VFIO_CHECK_EXTENSION, VFIO_SPAPR_TCE_v= 2_IOMMU); > >> > >> ret =3D ioctl(group->fd, VFIO_GROUP_SET_CONTAINER, &fd); > >> if (ret) { > >>@@ -757,7 +762,9 @@ static int vfio_connect_container(VFIOGroup *group,= AddressSpace *as) > >> ret =3D -errno; > >> goto free_container_exit; > >> } > >>- ret =3D ioctl(fd, VFIO_SET_IOMMU, VFIO_SPAPR_TCE_IOMMU); > >>+ container->iommu_type =3D > >>+ v2 ? VFIO_SPAPR_TCE_v2_IOMMU : VFIO_SPAPR_TCE_IOMMU; > >>+ ret =3D ioctl(fd, VFIO_SET_IOMMU, container->iommu_type); > > > >It'd be nice to consolidate the setting of container->iommu_type and > >then the SET_IOMMU ioctl() rather than having more or less duplicated > >logic for Type1 and SPAPR, but it's not a big deal. >=20 >=20 > May be but I cannot think of any nice way of doing this though. >=20 >=20 > > > >> if (ret) { > >> error_report("vfio: failed to set iommu for container: %m= "); > >> ret =3D -errno; > >>@@ -769,11 +776,25 @@ static int vfio_connect_container(VFIOGroup *grou= p, AddressSpace *as) > >> * when container fd is closed so we do not call it explicitly > >> * in this file. > >> */ > >>- ret =3D ioctl(fd, VFIO_IOMMU_ENABLE); > >>- if (ret) { > >>- error_report("vfio: failed to enable container: %m"); > >>- ret =3D -errno; > >>- goto free_container_exit; > >>+ if (!v2) { > >>+ ret =3D ioctl(fd, VFIO_IOMMU_ENABLE); > >>+ if (ret) { > >>+ error_report("vfio: failed to enable container: %m"); > >>+ ret =3D -errno; > >>+ goto free_container_exit; > >>+ } > >>+ } else { > >>+ container->prereg_listener.container =3D container; > >>+ container->prereg_listener.listener =3D vfio_prereg_listen= er; > >>+ > >>+ memory_listener_register(&container->prereg_listener.liste= ner, > >>+ &address_space_memory); > > > >This assumes that the target address space of the (guest) IOMMU is > >address_space_memory. Which is fine - vfio already assumes that - but > >it reminds me that it'd be nice to have an explicit check for that (I > >guess it would have to go in vfio_iommu_map_notify()). So that if > >someone constructs a machine where that's not the case, it'll at least > >be obvious why VFIO isn't working. >=20 > Ok, I'll add a small patch for this in the next respin. Ok. > >>+ if (container->error) { > >>+ error_report("vfio: RAM memory listener initialization= failed for container"); > >>+ memory_listener_unregister( > >>+ &container->prereg_listener.listener); > >>+ goto free_container_exit; > >>+ } > >> } > > > >Looks like you don't have an error path which will handle the case > >where the prereg listener is registered, but registering the normal > >PCI AS listener fails - I believe you will fail to unregister the > >prereg listener in that case. >=20 >=20 > In this case, the control goes to listener_release_exit: which calls > vfio_listener_release() which unregisters both listeners (it is a few chu= nks > above). Ah.. yes. In which case this could also jump to listener_release_exit and avoid the explicit unreg(), yes? > >> /* > >>diff --git a/hw/vfio/prereg.c b/hw/vfio/prereg.c > >>new file mode 100644 > >>index 0000000..66cd696 > >>--- /dev/null > >>+++ b/hw/vfio/prereg.c > >>@@ -0,0 +1,138 @@ > >>+/* > >>+ * DMA memory preregistration > >>+ * > >>+ * Authors: > >>+ * Alexey Kardashevskiy > >>+ * > >>+ * This work is licensed under the terms of the GNU GPL, version 2. S= ee > >>+ * the COPYING file in the top-level directory. > >>+ */ > >>+ > >>+#include "qemu/osdep.h" > >>+#include > >>+#include > >>+ > >>+#include "hw/vfio/vfio-common.h" > >>+#include "hw/vfio/vfio.h" > >>+#include "qemu/error-report.h" > >>+#include "trace.h" > >>+ > >>+static bool vfio_prereg_listener_skipped_section(MemoryRegionSection *= section) > >>+{ > >>+ return (!memory_region_is_ram(section->mr) && > >>+ !memory_region_is_iommu(section->mr)) || > >>+ memory_region_is_skip_dump(section->mr); > >>+} > >>+ > >>+static void vfio_prereg_listener_region_add(MemoryListener *listener, > >>+ MemoryRegionSection *secti= on) > >>+{ > >>+ VFIOMemoryListener *vlistener =3D container_of(listener, VFIOMemor= yListener, > >>+ listener); > >>+ VFIOContainer *container =3D vlistener->container; > >>+ hwaddr iova; > >>+ Int128 llend; > >>+ int ret; > >>+ hwaddr page_mask =3D qemu_real_host_page_mask; > >>+ struct vfio_iommu_spapr_register_memory reg =3D { > >>+ .argsz =3D sizeof(reg), > >>+ .flags =3D 0, > >>+ }; > >>+ > >>+ if (vfio_prereg_listener_skipped_section(section)) { > >>+ trace_vfio_listener_region_add_skip( > >>+ section->offset_within_address_space, > >>+ section->offset_within_address_space + > >>+ int128_get64(int128_sub(section->size, int128_one()))); > >>+ return; > >>+ } > > > >You should probably explicitly check for IOMMU regions and abort if > >you find one. An IOMMU AS appearing within address_space_memory would > >be bad news. >=20 >=20 > Oh, vfio_prereg_listener_skipped_section() allows memory_region_is_iommu(= ), > I'll remove it. Well, that's part. But IOMMU regions appearing here shouldn't just be ignored - they should be treated as a fatal error. >=20 >=20 >=20 > > > >>+ if (unlikely((section->offset_within_address_space & ~page_mask) != =3D > >>+ (section->offset_within_region & ~page_mask))) { > >>+ error_report("%s received unaligned region", __func__); > >>+ return; > >>+ } > >>+ > >>+ iova =3D ROUND_UP(section->offset_within_address_space, ~page_mask= + 1); > > > >iova is a terrible name here. This is *not* an IOVA, but a real > >memory address. >=20 >=20 > I'll do s/iova/gpa/ Good. >=20 >=20 > > > >>+ llend =3D int128_make64(section->offset_within_address_space); > >>+ llend =3D int128_add(llend, section->size); > >>+ llend =3D int128_and(llend, int128_exts64(page_mask)); > >>+ > >>+ if (int128_ge(int128_make64(iova), llend)) { > >>+ return; > > > >IIUC, if we get here something has gone horribly wrong in our machine > >setup, and we shold probably just abort. Same goes for the similar > >test in the IOVA listener, of course. >=20 >=20 > I do not have a good message candidate though. May be > hw_error("vfio: Broken section alignment"); > ? Just an assert should be fine. >=20 >=20 > > > >>+ } > >>+ > >>+ memory_region_ref(section->mr); > >>+ > >>+ reg.vaddr =3D (__u64) memory_region_get_ram_ptr(section->mr) + > >>+ section->offset_within_region + > >>+ (iova - section->offset_within_address_space); > >>+ reg.size =3D int128_get64(llend) - iova; > >>+ > >>+ ret =3D ioctl(container->fd, VFIO_IOMMU_SPAPR_REGISTER_MEMORY, &re= g); > >>+ trace_vfio_ram_register(reg.vaddr, reg.size, ret ? -errno : 0); > >>+ if (ret) { > >>+ /* > >>+ * On the initfn path, store the first error in the container = so we > >>+ * can gracefully fail. Runtime, there's not much we can do o= ther > >>+ * than throw a hardware error. > >>+ */ > >>+ if (!container->initialized) { > >>+ if (!container->error) { > >>+ container->error =3D ret; > >>+ } > >>+ } else { > >>+ hw_error("vfio: DMA mapping failed, unable to continue"); > > > >Wrong error message. > > > >>+ } > >>+ } > >>+} > >>+ > >>+static void vfio_prereg_listener_region_del(MemoryListener *listener, > >>+ MemoryRegionSection *secti= on) > >>+{ > >>+ VFIOMemoryListener *vlistener =3D container_of(listener, VFIOMemor= yListener, > >>+ listener); > >>+ VFIOContainer *container =3D vlistener->container; > >>+ hwaddr iova, end; > >>+ int ret; > >>+ hwaddr page_mask =3D qemu_real_host_page_mask; > >>+ struct vfio_iommu_spapr_register_memory reg =3D { > >>+ .argsz =3D sizeof(reg), > >>+ .flags =3D 0, > >>+ }; > >>+ > >>+ if (vfio_prereg_listener_skipped_section(section)) { > >>+ trace_vfio_listener_region_del_skip( > >>+ section->offset_within_address_space, > >>+ section->offset_within_address_space + > >>+ int128_get64(int128_sub(section->size, int128_one()))); > >>+ return; > >>+ } > >>+ > >>+ if (unlikely((section->offset_within_address_space & ~page_mask) != =3D > >>+ (section->offset_within_region & ~page_mask))) { > >>+ error_report("%s received unaligned region", __func__); > >>+ return; > >>+ } > >>+ > >>+ iova =3D ROUND_UP(section->offset_within_address_space, ~page_mask= + 1); > >>+ end =3D (section->offset_within_address_space + int128_get64(secti= on->size)) & > >>+ page_mask; > >>+ > >>+ if (iova >=3D end) { > >>+ return; > >>+ } > >>+ > >>+ reg.vaddr =3D (__u64) memory_region_get_ram_ptr(section->mr) + > >>+ section->offset_within_region + > >>+ (iova - section->offset_within_address_space); > >>+ reg.size =3D end - iova; > >>+ > >>+ ret =3D ioctl(container->fd, VFIO_IOMMU_SPAPR_UNREGISTER_MEMORY, &= reg); > >>+ trace_vfio_ram_unregister(reg.vaddr, reg.size, ret ? -errno : 0); > >>+} > >>+ > >>+const MemoryListener vfio_prereg_listener =3D { > >>+ .region_add =3D vfio_prereg_listener_region_add, > >>+ .region_del =3D vfio_prereg_listener_region_del, > >>+}; > >>diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-commo= n.h > >>index b6b736c..bcbc5cb 100644 > >>--- a/include/hw/vfio/vfio-common.h > >>+++ b/include/hw/vfio/vfio-common.h > >>@@ -70,6 +70,8 @@ typedef struct VFIOContainer { > >> VFIOAddressSpace *space; > >> int fd; /* /dev/vfio/vfio, empowered by the attached groups */ > >> VFIOMemoryListener iommu_listener; > >>+ VFIOMemoryListener prereg_listener; > >>+ unsigned iommu_type; > >> int error; > >> bool initialized; > >> /* > >>@@ -146,4 +148,6 @@ extern const MemoryRegionOps vfio_region_ops; > >> extern QLIST_HEAD(vfio_group_head, VFIOGroup) vfio_group_list; > >> extern QLIST_HEAD(vfio_as_head, VFIOAddressSpace) vfio_address_spaces; > >> > >>+extern const MemoryListener vfio_prereg_listener; > >>+ > >> #endif /* !HW_VFIO_VFIO_COMMON_H */ > >>diff --git a/trace-events b/trace-events > >>index 4b6ea70..f5335ec 100644 > >>--- a/trace-events > >>+++ b/trace-events > >>@@ -1725,6 +1725,8 @@ vfio_disconnect_container(int fd) "close containe= r->fd=3D%d" > >> vfio_put_group(int fd) "close group->fd=3D%d" > >> vfio_get_device(const char * name, unsigned int flags, unsigned int n= um_regions, unsigned int num_irqs) "Device %s flags: %u, regions: %u, irqs:= %u" > >> vfio_put_base_device(int fd) "close vdev->fd=3D%d" > >>+vfio_ram_register(uint64_t va, uint64_t size, int ret) "va=3D%"PRIx64"= size=3D%"PRIx64" ret=3D%d" > >>+vfio_ram_unregister(uint64_t va, uint64_t size, int ret) "va=3D%"PRIx6= 4" size=3D%"PRIx64" ret=3D%d" > >> > >> # hw/vfio/platform.c > >> vfio_platform_populate_regions(int region_index, unsigned long flag, = unsigned long size, int fd, unsigned long offset) "- region %d flags =3D 0x= %lx, size =3D 0x%lx, fd=3D %d, offset =3D 0x%lx" > > >=20 >=20 --=20 David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson --UTZ8bGhNySVQ9LYl Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJW56DGAAoJEGw4ysog2bOSuEwP/jWbWQh+VpMUiDezzgD1GlFU QY7iQtMD0f9E41GswmxgHAxtvGaNq5IyimKoqth3qlXX3+RERhRw26m01pUSF1cA YmMq/2tfe95e8yDHRbRQd2fKuTYLGreNmZ7rqOciPQyNhtv0HTjOO5QQpVWhkmZK tyKTpIOzs+bZRk5V0XKLEEvj5FaTduWHNN31Ov3k+cDuo8OaQW5Ye+Q+kCoc63zR GZ788D15FROLctH6JdsooCvGCtL3tkzE/J3GXRrPBDMDx2+TDJwceNYPlIqEHFE3 /raY3ld7O5m5vBZBJ344cmw5S5P3sOxuPFUhG3GcBxh8LIcpCdID3VKweuLxKcy1 QQSrmhXDCQ4fE5/ZMICTwYgZUr4tf4lV3OU7kpHPmKtwQGsQFG29asdtn+NUAle9 gLksvl0hA+afgi1ZnYIew57vrqVBKy1nII2blLMDmHh96FNREKl6HpBqWUbhW9OS kfWd0M0G9mLzkiWmCTx2E7GOyFMDpHx1mi2nC7jsTnuokbaSv7iZcfp0Gv/efVrl iVHoSp1sjC27PqYZuWbh++PcBcZ1loElzjoxmYAEWTFUO76Gm9btqMfo3/r2xavL mx27Co9qqcuPfDUyPXSyjnwXE6PJsc9e0fi1VSS8vXnHO7fm4qyQi7hNYYe1oRJW O1Ln7Kkm0/NIXkxkYCP6 =CN3N -----END PGP SIGNATURE----- --UTZ8bGhNySVQ9LYl--