From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45810) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZehuX-0002Fr-SN for qemu-devel@nongnu.org; Wed, 23 Sep 2015 07:08:31 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZehuT-0005BX-I9 for qemu-devel@nongnu.org; Wed, 23 Sep 2015 07:08:29 -0400 Date: Wed, 23 Sep 2015 21:07:06 +1000 From: David Gibson Message-ID: <20150923110706.GA15944@voom.fritz.box> References: <1442495357-26547-1-git-send-email-david@gibson.dropbear.id.au> <1442495357-26547-4-git-send-email-david@gibson.dropbear.id.au> <56027AA6.8090504@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="DocE+STaALJfprDB" Content-Disposition: inline In-Reply-To: <56027AA6.8090504@redhat.com> Subject: Re: [Qemu-devel] [RFC PATCH 03/10] vfio: Check guest IOVA ranges against host IOMMU capabilities List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Thomas Huth Cc: lvivier@redhat.com, aik@ozlabs.ru, gwshan@linux.vnet.ibm.com, qemu-devel@nongnu.org, alex.williamson@redhat.com, qemu-ppc@nongnu.org, pbonzini@redhat.com --DocE+STaALJfprDB Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Sep 23, 2015 at 12:10:46PM +0200, Thomas Huth wrote: > On 17/09/15 15:09, David Gibson wrote: > > The current vfio core code assumes that the host IOMMU is capable of > > mapping any IOVA the guest wants to use to where we need. However, real > > IOMMUs generally only support translating a certain range of IOVAs (the > > "DMA window") not a full 64-bit address space. > >=20 > > The common x86 IOMMUs support a wide enough range that guests are very > > unlikely to go beyond it in practice, however the IOMMU used on IBM Pow= er > > machines - in the default configuration - supports only a much more lim= ited > > IOVA range, usually 0..2GiB. > >=20 > > If the guest attempts to set up an IOVA range that the host IOMMU can't > > map, qemu won't report an error until it actually attempts to map a bad > > IOVA. If guest RAM is being mapped directly into the IOMMU (i.e. no gu= est > > visible IOMMU) then this will show up very quickly. If there is a guest > > visible IOMMU, however, the problem might not show up until much later = when > > the guest actually attempt to DMA with an IOVA the host can't handle. > >=20 > > This patch adds a test so that we will detect earlier if the guest is > > attempting to use IOVA ranges that the host IOMMU won't be able to deal > > with. > >=20 > > For now, we assume that "Type1" (x86) IOMMUs can support any IOVA, this= is > > incorrect, but no worse than what we have already. We can't do better = for > > now because the Type1 kernel interface doesn't tell us what IOVA range = the > > IOMMU actually supports. > >=20 > > For the Power "sPAPR TCE" IOMMU, however, we can retrieve the supported > > IOVA range and validate guest IOVA ranges against it, and this patch do= es > > so. > >=20 > > Signed-off-by: David Gibson > > --- > > hw/vfio/common.c | 42 +++++++++++++++++++++++++++++++++++= ++++--- > > include/hw/vfio/vfio-common.h | 6 ++++++ > > 2 files changed, 45 insertions(+), 3 deletions(-) > >=20 > > diff --git a/hw/vfio/common.c b/hw/vfio/common.c > > index 9953b9c..c37f1a1 100644 > > --- a/hw/vfio/common.c > > +++ b/hw/vfio/common.c > > @@ -344,14 +344,23 @@ static void vfio_listener_region_add(MemoryListen= er *listener, > > if (int128_ge(int128_make64(iova), llend)) { > > return; > > } > > + end =3D int128_get64(llend); > > + > > + if ((iova < container->iommu_data.min_iova) > > + || ((end - 1) > container->iommu_data.max_iova)) { >=20 > (Too much paranthesis for my taste ;-)) Yes, well, we've already established our tastes differ on that point. > > + error_report("vfio: IOMMU container %p can't map guest IOVA re= gion" > > + " 0x%"HWADDR_PRIx"..0x%"HWADDR_PRIx, > > + container, iova, end - 1); > > + ret =3D -EFAULT; /* FIXME: better choice here? */ >=20 > Maybe -EINVAL? ... but -EFAULT also sounds ok for me. I try to avoid EINVAL unless it's clearly the only right choice. So many things use it that it tends to be very unhelpful when you get one. > > + goto fail; > > + } > ... > > @@ -712,6 +732,22 @@ static int vfio_connect_container(VFIOGroup *group= , AddressSpace *as) > > ret =3D -errno; > > goto free_container_exit; > > } > > + > > + /* > > + * FIXME: This only considers the host IOMMU' 32-bit window. > > + * At some point we need to add support for the optional > > + * 64-bit window and dynamic windows > > + */ > > + info.argsz =3D sizeof(info); > > + ret =3D ioctl(fd, VFIO_IOMMU_SPAPR_TCE_GET_INFO, &info); > > + if (ret) { > > + error_report("vfio: VFIO_IOMMU_SPAPR_TCE_GET_INFO failed: = %m"); >=20 > Isn't that %m a glibc extension only? ... Well, this code likely only > runs on Linux with a glibc, so it likely doesn't matter, I guess... Yes, it is, but it's already used extensively within qemu. > > + ret =3D -errno; > > + goto free_container_exit; > > + } > > + container->iommu_data.min_iova =3D info.dma32_window_start; > > + container->iommu_data.max_iova =3D container->iommu_data.min_i= ova > > + + info.dma32_window_size - 1; > > } else { > > error_report("vfio: No available IOMMU models"); > > ret =3D -EINVAL; > > diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-commo= n.h > > index aff18cd..88ec213 100644 > > --- a/include/hw/vfio/vfio-common.h > > +++ b/include/hw/vfio/vfio-common.h > > @@ -71,6 +71,12 @@ typedef struct VFIOContainer { > > MemoryListener listener; > > int error; > > bool initialized; > > + /* > > + * FIXME: This assumes the host IOMMU can support only a > > + * single contiguous IOVA window. We may need to generalize > > + * that in future > > + */ > > + hwaddr min_iova, max_iova; >=20 > Should that maybe be dma_addr_t instead of hwaddr ? Ah, yes it probably should. > > } iommu_data; > > QLIST_HEAD(, VFIOGuestIOMMU) giommu_list; > > QLIST_HEAD(, VFIOGroup) group_list; > >=20 >=20 > Thomas >=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 --DocE+STaALJfprDB Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJWAofZAAoJEGw4ysog2bOSiv4P/2yxlVqC8L5gOJbbup7ktk9i uwBbSWaGNy3XlFs5wuKw2yrs9PtsJYTMOfju9YVcAAmezn2IsnUaJigblwLnVFn8 JSooIoF3XXNILl8aLgXuqh/4aIssaWDeMnb9mOkd51jBiTDzn6OtVuyrMQh0NTT0 7F9FpH/ckRbRz3Fngq37UaYi+xrKA9LCLOm6rCx8pTjqeBc4tn2/gyGUvBuxEnay 28uJhkNSUfn6AnAVAziLrX72OCGia4ENDDc7YW2eQ9LdEOQjo51YWwaQ3fyEou8E gmx85M03o0XgQEAcA75ecA1JE1O1g88gElOp7y5EwJkCwP59sKtYQ8eKZdA5vhXN W5KcYKX/Pv1K7Mkr00wnGUigT1jjAHHQ27794H0NjrAhobi22yzlCexoXsMy0kBc KdrvMvCpZfkJVanVW7LcQ6TREA2IE1TPDSvf4XxqRhVulEhG6ewER+7dHyCBWNl1 ag7sjjFowv1f71BIqiX9mEi5EolkQjCptTOQhGFINtjSCFooXZz4wgiAj8QCBwsi nwRo6W2L49FP57b8KezsZoQiFiRuzRH/n0qbFLCr9hi3MkLW/vPdS7iSmGxWy28r OFadNFNTyWKOSgM767waEKdj52UT2vM3UB7kfJ3awpKs5HDz32dGr2LEOVr3LDT/ fzNgwtjy03M16UuOTL+l =2cRo -----END PGP SIGNATURE----- --DocE+STaALJfprDB--