From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45871) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YnaXj-00013V-W2 for qemu-devel@nongnu.org; Wed, 29 Apr 2015 18:33:25 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YnaXi-0006l9-Cu for qemu-devel@nongnu.org; Wed, 29 Apr 2015 18:33:23 -0400 Date: Wed, 29 Apr 2015 15:58:46 +1000 From: David Gibson Message-ID: <20150429055846.GV32589@voom.redhat.com> References: <1428679484-15451-1-git-send-email-aik@ozlabs.ru> <1428679484-15451-8-git-send-email-aik@ozlabs.ru> <20150422055306.GM31815@voom.redhat.com> <55376C13.9070204@ozlabs.ru> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="+VeMz1SRxFChf6vr" Content-Disposition: inline In-Reply-To: <55376C13.9070204@ozlabs.ru> Subject: Re: [Qemu-devel] [PATCH qemu v6 07/15] 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: Alexander Graf , Michael Roth , qemu-devel@nongnu.org, Alex Williamson , qemu-ppc@nongnu.org, Gavin Shan --+VeMz1SRxFChf6vr Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Apr 22, 2015 at 07:38:27PM +1000, Alexey Kardashevskiy wrote: > On 04/22/2015 03:53 PM, David Gibson wrote: > >On Sat, Apr 11, 2015 at 01:24:36AM +1000, 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 guest RAM memory listener which notifies a VFIO container > >>about memory which needs to be pinned/unpinned. VFIO MMIO regions > >>(i.e. "skip dump" regions) are skipped. > >> > >>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 > > > >Reviewed-by: David Gibson > > > >Albeit with some nits described below. [snip] > >>+ vfio_ram_do_region(container, section, VFIO_IOMMU_SPAPR_UNREGISTER= _MEMORY); > >>+} > >>+ > >>+static const MemoryListener vfio_spapr_ram_memory_listener =3D { > >>+ .region_add =3D vfio_spapr_ram_listener_region_add, > >>+ .region_del =3D vfio_spapr_ram_listener_region_del, > >>+}; > >>+ > >> static void vfio_spapr_listener_release(VFIOContainer *container) > >> { > >> memory_listener_unregister(&container->iommu_data.spapr.listener); > >> } > >> > >>-void spapr_memory_listener_register(VFIOContainer *container) > >>+static void vfio_spapr_listener_release_v2(VFIOContainer *container) > >>+{ > >>+ memory_listener_unregister(&container->iommu_data.spapr.listener); > >>+ vfio_spapr_listener_release(container); > >>+} > >>+ > >>+int spapr_memory_listener_register(VFIOContainer *container, int ver) > >> { > >> container->iommu_data.spapr.listener =3D vfio_spapr_memory_listen= er; > >> container->iommu_data.release =3D vfio_spapr_listener_release; > >>- > >> memory_listener_register(&container->iommu_data.spapr.listener, > >> container->space->as); > >>+ if (ver < 2) { > >>+ return 0; > >>+ } > > > >I wonder if it would make sense to store the IOMMU type value into the > >VFIOContainer (from non-arch specific code). It would avoid the > >ad-hoc passing of version here and also allow for some sanity checks. >=20 >=20 > What kind of checks? Cannot think of any which would make sense to do here > and not in spapr_(pci|rtas)*.c Well, all I was thinking of was something like assert(container->iommu_type =3D=3D SPAPR_TCE_TYPE); In spapr (or x86) callbacks from the core vfio code to make sure we haven't somehow gotten here with a Type1 container. > >>+ > >>+ container->iommu_data.spapr.ramlistener =3D vfio_spapr_ram_memory_= listener; > >>+ container->iommu_data.release =3D vfio_spapr_listener_release_v2; > >>+ memory_listener_register(&container->iommu_data.spapr.ramlistener, > >>+ &address_space_memory); > >>+ > >>+ container->iommu_data.spapr.ram_reg_initialized =3D true; > >>+ > >>+ return container->iommu_data.spapr.ram_reg_error; > >> } > >>diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-commo= n.h > >>index d0b831c..b5ef446 100644 > >>--- a/include/hw/vfio/vfio-common.h > >>+++ b/include/hw/vfio/vfio-common.h > >>@@ -71,6 +71,9 @@ typedef struct VFIOType1 { > >> > >> typedef struct VFIOSPAPR { > >> MemoryListener listener; > >>+ MemoryListener ramlistener; > > > >The names "listener" and "ramlistener" don't exactly give a clear hint > >as to what the difference between them is. Maybe "register_listener" > >for the new one since its purpose is to register dma memory. >=20 >=20 > It is listening for RAM changes. If/when we decide to add something else = to > this listener, we won't have to change its name :) Still change? Ah, I see. I was forgetting that the other listener wasn't really about RAM (I think an understandable error for a *Memory*Listener). But actually I'm having trouble thinking of names that are clearly better than the current ones. So, I guess I don't really care. >=20 >=20 > >I forget > >what the purpose of the old one is, and so what a better name for it > >might be. >=20 > It is listening on PHB address space for IOMMU table updates and translat= es > those to MAP/UNMAP ioctls to VFIO containers. >=20 >=20 >=20 > > > >>+ int ram_reg_error; > >>+ bool ram_reg_initialized; > >> } VFIOSPAPR; > >> > >> typedef struct VFIOContainer { > >>@@ -156,6 +159,6 @@ extern int vfio_dma_unmap(VFIOContainer *container, > >> hwaddr iova, ram_addr_t size); > >> bool vfio_listener_skipped_section(MemoryRegionSection *section); > >> > >>-extern void spapr_memory_listener_register(VFIOContainer *container); > >>+extern int spapr_memory_listener_register(VFIOContainer *container, in= t ver); > >> > >> #endif /* !HW_VFIO_VFIO_COMMON_H */ > >>diff --git a/trace-events b/trace-events > >>index 1231ba4..2739140 100644 > >>--- a/trace-events > >>+++ b/trace-events > >>@@ -1563,6 +1563,7 @@ 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(int req, uint64_t va, uint64_t size, int ret) "req= =3D%d va=3D%"PRIx64" size=3D%"PRIx64" ret=3D%d" > >> > >> #hw/acpi/memory_hotplug.c > >> mhp_acpi_invalid_slot_selected(uint32_t slot) "0x%"PRIx32 > > >=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 --+VeMz1SRxFChf6vr Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJVQHMWAAoJEGw4ysog2bOS2z0P/ist8Oq/zXx4zefbtwqy9JGd fVoSNtEPfCVpjaVu+UUvL19MUNd5QNcRThtJB3dI0oHeHn1Vc+5KJ/1TK9ZG5IVX y+Ak5ZDDXNyNOYyvmTrDWhFw7CK7cKcu/MtzZKcgWiMCkv0M4EjYZ1Js0EOFtGhQ FzsrPWIkF3OsegEGNHVCkfu13RFHbb/m5gDZFb18bfgdHvS2N2vs2VKozvXnRw7u +DydJtI7EIvtqyC2nlh+GBME+IzmcfgY/cyQmhuuKjE3aQCVAuMpMiVDsBWPmDcM NrwAj0GRa7AJ3eB3jpfHGEJkoT+aENZXUznrkV4xX7NB/Dc81jOlS0k4HNUhIV9p xBDbro8ciq44G/ot8xVssOAAZtPtIFVbk5R8sXwWCgO3HURji0fmzfyfnSA4k+mj o4DotKFk6K4TbCFaHfSUrPSDAIY1wFDxitY13DskOzVtzgFTPBm3IwEyVcjFhRQn sR2hs1l7UxixgWSqxUbhx7Ar7Tbyw8Se9Wl4/oYMqzNS+YUFLFd9eqxoxYMFHj5F 3m0Q69zKbyaySMkCpRdOtOOgxO3eJF2k9NI2/IrFkaWn8QqnsQ3yWYNerdZdLBvs 640FE53wMAYKvX1SL9snPYnrqWkLih4ejsHzJRUi2X8ZypKAxUtqgAIJlnBtDUBV AWLRoVJzmlD12cX78rlG =vY/6 -----END PGP SIGNATURE----- --+VeMz1SRxFChf6vr--