From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47098) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zeh0p-0000B9-F7 for qemu-devel@nongnu.org; Wed, 23 Sep 2015 06:10:56 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Zeh0m-00032p-87 for qemu-devel@nongnu.org; Wed, 23 Sep 2015 06:10:55 -0400 References: <1442495357-26547-1-git-send-email-david@gibson.dropbear.id.au> <1442495357-26547-4-git-send-email-david@gibson.dropbear.id.au> From: Thomas Huth Message-ID: <56027AA6.8090504@redhat.com> Date: Wed, 23 Sep 2015 12:10:46 +0200 MIME-Version: 1.0 In-Reply-To: <1442495357-26547-4-git-send-email-david@gibson.dropbear.id.au> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable 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: David Gibson , alex.williamson@redhat.com, aik@ozlabs.ru, gwshan@linux.vnet.ibm.com Cc: lvivier@redhat.com, pbonzini@redhat.com, qemu-ppc@nongnu.org, qemu-devel@nongnu.org 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, rea= l > 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 gues= t > 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)) { (Too much paranthesis for my taste ;-)) > + 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? */ Maybe -EINVAL? ... but -EFAULT also sounds ok for me. > + 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"); 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... > + 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; Should that maybe be dma_addr_t instead of hwaddr ? > } iommu_data; > QLIST_HEAD(, VFIOGuestIOMMU) giommu_list; > QLIST_HEAD(, VFIOGroup) group_list; >=20 Thomas