From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41698) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Yffkh-0000Vh-Q3 for qemu-devel@nongnu.org; Tue, 07 Apr 2015 22:30:05 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Yffke-0003lt-Rq for qemu-devel@nongnu.org; Tue, 07 Apr 2015 22:30:03 -0400 Date: Wed, 8 Apr 2015 12:15:28 +1000 From: David Gibson Message-ID: <20150408021528.GD28909@voom.redhat.com> References: <1427779727-13353-1-git-send-email-aik@ozlabs.ru> <1427779727-13353-7-git-send-email-aik@ozlabs.ru> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="OROCMA9jn6tkzFBc" Content-Disposition: inline In-Reply-To: <1427779727-13353-7-git-send-email-aik@ozlabs.ru> Subject: Re: [Qemu-devel] [PATCH qemu v5 06/12] 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, Alexander Graf --OROCMA9jn6tkzFBc Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Mar 31, 2015 at 04:28:41PM +1100, Alexey Kardashevskiy wrote: > This makes use of the new "memory registering" feature. The idea is > to provide the guest ability to notify the host kernel about pages which AFAICT it's not really the guest informing the host, just qemu informing the host. If I'm reading the code correctly, qemu registers all RAM regionns, without guest intervention. > 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. >=20 > 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. >=20 > 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 does > not call it when v2 is detected and enabled. So I'm clear, the v2 just represents a new userspace<->host kernel interface for controlling the IOMMU, doesn't it? It doesn't change the guest visible IOMMU interface and doesn't represent different IOMMU hardware? > Signed-off-by: Alexey Kardashevskiy > --- > Changes: > v5: > * simplified the patch > * added trace points > * added round_up() for the size > * SPAPR IOMMU v2 used > --- > hw/vfio/common.c | 26 +++++++++----- > hw/vfio/spapr.c | 79 +++++++++++++++++++++++++++++++++++++= ++++-- > include/hw/vfio/vfio-common.h | 5 ++- > trace-events | 1 + > 4 files changed, 100 insertions(+), 11 deletions(-) >=20 > diff --git a/hw/vfio/common.c b/hw/vfio/common.c > index a71f881..e35e478 100644 > --- a/hw/vfio/common.c > +++ b/hw/vfio/common.c > @@ -577,14 +577,18 @@ static int vfio_connect_container(VFIOGroup *group,= AddressSpace *as) > =20 > container->iommu_data.type1.initialized =3D true; > =20 > - } 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))= { > + bool v2 =3D !!ioctl(fd, VFIO_CHECK_EXTENSION, VFIO_SPAPR_TCE_v2_= IOMMU); > + > ret =3D ioctl(group->fd, VFIO_GROUP_SET_CONTAINER, &fd); > if (ret) { > error_report("vfio: failed to set group container: %m"); > ret =3D -errno; > goto free_container_exit; > } > - ret =3D ioctl(fd, VFIO_SET_IOMMU, VFIO_SPAPR_TCE_IOMMU); > + ret =3D ioctl(fd, VFIO_SET_IOMMU, > + v2 ? VFIO_SPAPR_TCE_v2_IOMMU : VFIO_SPAPR_TCE_IOMMU); > if (ret) { > error_report("vfio: failed to set iommu for container: %m"); > ret =3D -errno; > @@ -596,14 +600,20 @@ static int vfio_connect_container(VFIOGroup *group,= 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; > + } > } > =20 > - spapr_memory_listener_register(container); > + ret =3D spapr_memory_listener_register(container, v2 ? 2 : 1); > + if (ret) { > + error_report("vfio: RAM memory listener initialization faile= d for container"); > + goto listener_release_exit; > + } > =20 > } else { > error_report("vfio: No available IOMMU models"); > diff --git a/hw/vfio/spapr.c b/hw/vfio/spapr.c > index 5f79194..a670907 100644 > --- a/hw/vfio/spapr.c > +++ b/hw/vfio/spapr.c > @@ -17,6 +17,9 @@ > * along with this program; if not, see . > */ > =20 > +#include > +#include > + > #include "hw/vfio/vfio-common.h" > #include "qemu/error-report.h" > #include "trace.h" > @@ -211,16 +214,88 @@ static const MemoryListener vfio_spapr_memory_liste= ner =3D { > .region_del =3D vfio_spapr_listener_region_del, > }; > =20 > +static void vfio_ram_do_region(VFIOContainer *container, > + MemoryRegionSection *section, unsigned lon= g req) > +{ > + int ret; > + struct vfio_iommu_spapr_register_memory reg =3D { .argsz =3D sizeof(= reg) }; > + > + if (!memory_region_is_ram(section->mr) || > + memory_region_is_skip_dump(section->mr)) { > + return; > + } > + > + reg.vaddr =3D (__u64) memory_region_get_ram_ptr(section->mr) + > + section->offset_within_region; Is section->offset_within_region always page aligned? > + reg.size =3D ROUND_UP(int128_get64(section->size), TARGET_PAGE_SIZE); > + > + ret =3D ioctl(container->fd, req, ®); > + trace_vfio_ram_register(_IOC_NR(req) - VFIO_BASE, reg.vaddr, reg.siz= e, 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 other > + * than throw a hardware error. > + */ > + if (!container->iommu_data.spapr.ram_reg_initialized) { > + if (!container->iommu_data.spapr.ram_reg_error) { > + container->iommu_data.spapr.ram_reg_error =3D ret; > + } This is pretty clunky, but I don't immediately see a better way. Also.. won't the return value of ioctl() just be -1 on error, which won't tell you much. Do you want to store errno, instead? > + } else { > + hw_error("vfio: RAM registering failed, unable to continue"); > + } > +} > + > +static void vfio_spapr_ram_listener_region_add(MemoryListener *listener, > + MemoryRegionSection *sect= ion) > +{ > + VFIOContainer *container =3D container_of(listener, VFIOContainer, > + iommu_data.spapr.ramlistener= ); > + memory_region_ref(section->mr); > + vfio_ram_do_region(container, section, VFIO_IOMMU_SPAPR_REGISTER_MEM= ORY); > +} > + > +static void vfio_spapr_ram_listener_region_del(MemoryListener *listener, > + MemoryRegionSection *sect= ion) > +{ > + VFIOContainer *container =3D container_of(listener, VFIOContainer, > + iommu_data.spapr.ramlistener= ); > + memory_region_unref(section->mr); > + vfio_ram_do_region(container, section, VFIO_IOMMU_SPAPR_UNREGISTER_M= EMORY); > +} > + > +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); > } > =20 > -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_listener; > 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; > + } > + > + container->iommu_data.spapr.ramlistener =3D vfio_spapr_ram_memory_li= stener; > + 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-common.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 { > =20 > typedef struct VFIOSPAPR { > MemoryListener listener; > + MemoryListener ramlistener; > + int ram_reg_error; > + bool ram_reg_initialized; > } VFIOSPAPR; > =20 > 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); > =20 > -extern void spapr_memory_listener_register(VFIOContainer *container); > +extern int spapr_memory_listener_register(VFIOContainer *container, int = ver); > =20 > #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 container-= >fd=3D%d" > vfio_put_group(int fd) "close group->fd=3D%d" > vfio_get_device(const char * name, unsigned int flags, unsigned int num_= 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" > =20 > #hw/acpi/memory_hotplug.c > mhp_acpi_invalid_slot_selected(uint32_t slot) "0x%"PRIx32 --=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 --OROCMA9jn6tkzFBc Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJVJI9AAAoJEGw4ysog2bOS11EQAI6NH1V0aThMGOTTIqL+5lFb DPmhACuFCU0M+HD14Ax123xA+mycy/PZDSVCKXAen4wcZKddA7bSL0PQBi4clzxH N1SZ64BCRLX30XbUqi+2HcUv++qnXcv/CGp6Z9UM4hYHrcWuLSQi9upnglvF8tSh /Uojys380cbwigvdzyTmzbWYl1YLMSKhU7B3XGOVc8gS7my0aUfRjusYTUAR9lR7 H8DBjnD2Ot6rqfErXtgiDlUMcRptuhnZzeGd6uzw7WqCifk0nlpNAzjff2PT0w5y aGl5zJ1+fcIib9CE3o7zMX2uMvIaKc1Q19oVSxzW6aYS87ZNUDa61/gCMcYUNtIx FxJ/wdQmrz1KwNSpio764L7R1cHHhW9h9u9q0eWx/N5yACeXM+W1e9wSNkZkHDBR zFV9nUXyGvnq8ltbpuL2RCtnyyLCogBUCuBZnn1caScx82j6Nb/Eaf76xeUS6oI/ tPi0Zf4gV3zl5DiPNnMG4TbwEyjqWaZP4W9Fk2iOSvUaUO4RkGzs9Y5GZkpaswUr YpirRQxSv5rR4BLVI6OROJoUz8JARDEjdS/g8JfNlNxsPWTW9BM5bhkc/bfbLOLe Z67xNXSbGZUjc31+SJOswAv8KhljGyMd5EcMp3yAg2SHAgqASC0xT0MkB0v5hlcw meop5lhzjTScRk0pQzzI =Fd/e -----END PGP SIGNATURE----- --OROCMA9jn6tkzFBc--