From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33887) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aiAVE-0001U2-Eo for qemu-devel@nongnu.org; Mon, 21 Mar 2016 20:48:57 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aiAVB-0005ip-8E for qemu-devel@nongnu.org; Mon, 21 Mar 2016 20:48:56 -0400 Date: Tue, 22 Mar 2016 11:49:56 +1100 From: David Gibson Message-ID: <20160322004956.GS23586@voom.redhat.com> References: <1458546426-26222-1-git-send-email-aik@ozlabs.ru> <1458546426-26222-2-git-send-email-aik@ozlabs.ru> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="FiqEyLLt06qkB6ow" Content-Disposition: inline In-Reply-To: <1458546426-26222-2-git-send-email-aik@ozlabs.ru> Subject: Re: [Qemu-devel] [PATCH qemu v14 01/18] memory: Fix IOMMU replay base address 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 --FiqEyLLt06qkB6ow Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Mar 21, 2016 at 06:46:49PM +1100, Alexey Kardashevskiy wrote: > Since a788f227 "memory: Allow replay of IOMMU mapping notifications" > when new VFIO listener is added, all existing IOMMU mappings are > replayed. However there is a problem that the base address of > an IOMMU memory region (IOMMU MR) is ignored which is not a problem > for the existing user (which is pseries) with its default 32bit DMA > window starting at 0 but it is if there is another DMA window. >=20 > This stores the IOMMU's offset_within_address_space and adjusts > the IOVA before calling vfio_dma_map/vfio_dma_unmap. >=20 > As the IOMMU notifier expects IOVA offset rather than the absolute > address, this also adjusts IOVA in sPAPR H_PUT_TCE handler before > calling notifier(s). >=20 > Signed-off-by: Alexey Kardashevskiy > Reviewed-by: David Gibson On a closer look, I realised this still isn't quite correct, although I don't think any cases which would break it exist or are planned. > --- > hw/ppc/spapr_iommu.c | 2 +- > hw/vfio/common.c | 14 ++++++++------ > include/hw/vfio/vfio-common.h | 1 + > 3 files changed, 10 insertions(+), 7 deletions(-) >=20 > diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c > index 7dd4588..277f289 100644 > --- a/hw/ppc/spapr_iommu.c > +++ b/hw/ppc/spapr_iommu.c > @@ -277,7 +277,7 @@ static target_ulong put_tce_emu(sPAPRTCETable *tcet, = target_ulong ioba, > tcet->table[index] =3D tce; > =20 > entry.target_as =3D &address_space_memory, > - entry.iova =3D ioba & page_mask; > + entry.iova =3D (ioba - tcet->bus_offset) & page_mask; > entry.translated_addr =3D tce & page_mask; > entry.addr_mask =3D ~page_mask; > entry.perm =3D spapr_tce_iommu_access_flags(tce); This bit's right/ > diff --git a/hw/vfio/common.c b/hw/vfio/common.c > index fb588d8..d45e2db 100644 > --- a/hw/vfio/common.c > +++ b/hw/vfio/common.c > @@ -257,14 +257,14 @@ static void vfio_iommu_map_notify(Notifier *n, void= *data) > VFIOGuestIOMMU *giommu =3D container_of(n, VFIOGuestIOMMU, n); > VFIOContainer *container =3D giommu->container; > IOMMUTLBEntry *iotlb =3D data; > + hwaddr iova =3D iotlb->iova + giommu->offset_within_address_space; This bit might be right, depending on how you define giommu->offset_within_= address_space. > MemoryRegion *mr; > hwaddr xlat; > hwaddr len =3D iotlb->addr_mask + 1; > void *vaddr; > int ret; > =20 > - trace_vfio_iommu_map_notify(iotlb->iova, > - iotlb->iova + iotlb->addr_mask); > + trace_vfio_iommu_map_notify(iova, iova + iotlb->addr_mask); > =20 > /* > * The IOMMU TLB entry we have just covers translation through > @@ -291,21 +291,21 @@ static void vfio_iommu_map_notify(Notifier *n, void= *data) > =20 > if ((iotlb->perm & IOMMU_RW) !=3D IOMMU_NONE) { > vaddr =3D memory_region_get_ram_ptr(mr) + xlat; > - ret =3D vfio_dma_map(container, iotlb->iova, > + ret =3D vfio_dma_map(container, iova, > iotlb->addr_mask + 1, vaddr, > !(iotlb->perm & IOMMU_WO) || mr->readonly); > if (ret) { > error_report("vfio_dma_map(%p, 0x%"HWADDR_PRIx", " > "0x%"HWADDR_PRIx", %p) =3D %d (%m)", > - container, iotlb->iova, > + container, iova, > iotlb->addr_mask + 1, vaddr, ret); > } > } else { > - ret =3D vfio_dma_unmap(container, iotlb->iova, iotlb->addr_mask = + 1); > + ret =3D vfio_dma_unmap(container, iova, iotlb->addr_mask + 1); > if (ret) { > error_report("vfio_dma_unmap(%p, 0x%"HWADDR_PRIx", " > "0x%"HWADDR_PRIx") =3D %d (%m)", > - container, iotlb->iova, > + container, iova, > iotlb->addr_mask + 1, ret); > } > } This is fine. > @@ -377,6 +377,8 @@ static void vfio_listener_region_add(MemoryListener *= listener, > */ > giommu =3D g_malloc0(sizeof(*giommu)); > giommu->iommu =3D section->mr; > + giommu->offset_within_address_space =3D > + section->offset_within_address_space; But here there's a problem. The iova in IOMMUTLBEntry is relative to the IOMMU MemoryRegion, but - at least in theory - only a subsection of that MemoryRegion could be mapped into the AddressSpace. So, to find the IOVA within the AddressSpace, from the IOVA within the MemoryRegion, you need to first subtract the section's offset within the MemoryRegion, then add the section's offset within the AddressSpace. You could precalculate the combined delta here, but... > giommu->container =3D container; > giommu->n.notify =3D vfio_iommu_map_notify; > QLIST_INSERT_HEAD(&container->giommu_list, giommu, giommu_next); > diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h > index eb0e1b0..5341e05 100644 > --- a/include/hw/vfio/vfio-common.h > +++ b/include/hw/vfio/vfio-common.h > @@ -90,6 +90,7 @@ typedef struct VFIOContainer { > typedef struct VFIOGuestIOMMU { > VFIOContainer *container; > MemoryRegion *iommu; > + hwaddr offset_within_address_space; =2E..it might be simpler to replace both the iommu and offset_within_address_space fields here with a pointer to the MemoryRegionSection instead, which should give you all the info you need. It might also be worth adding Paolo to the CC for this patch, since he knows the MemoryRegion stuff better than anyone. > Notifier n; > QLIST_ENTRY(VFIOGuestIOMMU) giommu_next; > } VFIOGuestIOMMU; --=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 --FiqEyLLt06qkB6ow Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJW8Ja0AAoJEGw4ysog2bOSFAcQAKs9gECGH+2tiChm+E53Bjz5 CV3A+r0GDRTbbvPbgvH/uy8K0uzIsAf51VEDo9okGVGGQhoOpxAGNqRdzXH3hiwk dV3SAovdQs3k3RuUeg70f8ZNYqGqvD+ASHsei+pruV7fGHCerWiE3K09O01srpQw TcswldNdqw0RZQ1YOmGPOrl4AUqpe9sp7xlh54HLbqGy6TgIh24fcSquDIGfxRao yaPZvqjv2PQW84PEzq1Si9igUgTGZHdwTqBuRU1ZHO5kXh9GLr85wnFSy5WOHa2V FxPW5gsQt0fMVTB001mverf1+qHlZ8GNhhMAztND7OdcNJYXH4wa8IlqOg+vUFt3 XcC+/fVTMeLhJ2VZPGKYtLpvTxpEXYQ0V/2D67jooUi8NoZVEv6ROI3flfZf1OCo wAtV/hj2Ozn6QT0qtZ8kCpFhqNyCVRJQ45wfDaJHH8ce2WjFtF7rKtzWadyifNI9 FB/vtCZqCaJrar4MOdnBTEfIxdNDQ8Mi9c3aa8db4cgRgHHZkUBFxXaG2tJGc1Is cmqrc9LkrhLlQpjPFKsUqKYibAFvE+KgCpqtThVVtOaFp5atk+3NDcNGXmcsxT4k 51duNAz/eVNhUPIhddXpVzPlaaIWd4MzoktlfUfWmWGrYpYBUozfsQOM070s9f9v WsxdWEV6Juf9AAlc+p0V =4svC -----END PGP SIGNATURE----- --FiqEyLLt06qkB6ow--