From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51198) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aiEOY-000103-LI for qemu-devel@nongnu.org; Tue, 22 Mar 2016 00:58:20 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aiEOV-00059a-El for qemu-devel@nongnu.org; Tue, 22 Mar 2016 00:58:18 -0400 Date: Tue, 22 Mar 2016 15:59:22 +1100 From: David Gibson Message-ID: <20160322045922.GF23586@voom.redhat.com> References: <1458546426-26222-1-git-send-email-aik@ozlabs.ru> <1458546426-26222-2-git-send-email-aik@ozlabs.ru> <20160322004956.GS23586@voom.redhat.com> <56F0B81E.9080701@ozlabs.ru> <20160322032626.GA23586@voom.redhat.com> <56F0CA04.4010109@ozlabs.ru> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="4fo3mGi7Q6pk/+I3" Content-Disposition: inline In-Reply-To: <56F0CA04.4010109@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: Paolo Bonzini , Alex Williamson , qemu-ppc@nongnu.org, qemu-devel@nongnu.org --4fo3mGi7Q6pk/+I3 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Mar 22, 2016 at 03:28:52PM +1100, Alexey Kardashevskiy wrote: > On 03/22/2016 02:26 PM, David Gibson wrote: > >On Tue, Mar 22, 2016 at 02:12:30PM +1100, Alexey Kardashevskiy wrote: > >>On 03/22/2016 11:49 AM, David Gibson wrote: > >>>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. > >>>> > >>>>This stores the IOMMU's offset_within_address_space and adjusts > >>>>the IOVA before calling vfio_dma_map/vfio_dma_unmap. > >>>> > >>>>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). > >>>> > >>>>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(-) > >>>> > >>>>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 *tc= et, target_ulong ioba, > >>>> tcet->table[index] =3D tce; > >>>> > >>>> 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_spac= e; > >>> > >>>This bit might be right, depending on how you define giommu->offset_wi= thin_address_space. > >>> > >>>> MemoryRegion *mr; > >>>> hwaddr xlat; > >>>> hwaddr len =3D iotlb->addr_mask + 1; > >>>> void *vaddr; > >>>> int ret; > >>>> > >>>>- trace_vfio_iommu_map_notify(iotlb->iova, > >>>>- iotlb->iova + iotlb->addr_mask); > >>>>+ trace_vfio_iommu_map_notify(iova, iova + iotlb->addr_mask); > >>>> > >>>> /* > >>>> * The IOMMU TLB entry we have just covers translation through > >>>>@@ -291,21 +291,21 @@ static void vfio_iommu_map_notify(Notifier *n, = void *data) > >>>> > >>>> 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->readonl= y); > >>>> 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_m= ask + 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(MemoryListen= er *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. > >> > >>But the IOMMU MR stays the same - size, offset, and iova will be relati= ve to > >>its start, why does it matter if only portion is mapped? > > > >Because the portion mapped may not sit at the start of the MR. For > >example if you had a 2G MR, and the second half is mapped at address 0 > >in the AS, >=20 > My imagination fails here. How could you do this in practice? >=20 > address_space_init(&as, &root) > memory_region_init(&mr, 2GB) > memory_region_add_subregion(&root, -1GB, &mr) >=20 > But offsets are unsigned. >=20 > In general, how to map only a half, what memory_region_add_xxx() > does that? I'm not totally sure, but I think you can do it with: address_space_init(&as, &root) memory_region_init(&mr0, 2GB) memory_region_init_alias(&mr1, &mr0, 1GB, 1GB) memory_region_add_subregion(&root, 0, &mr1) But the point is that it's possible for offset_within_region to be non-zero, and if it is, you need to take it into account. --=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 --4fo3mGi7Q6pk/+I3 Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJW8NEqAAoJEGw4ysog2bOSlwIP/1mZ+1vvHGq1GPnVAovqsRDc QkOpOsSDc/wNQ0Dw+kVZgemSnHsdP4AQsWKzEQFGVmjn3w2F939i8mf44PpfRtl2 PgCvlcZN5hpADR33i3wm9QpLhe+qRR7jA0DAp1loeXS+Y4E9e1uKp8XYkCg092CO 2aerkGBqNLO+RlAL26THMR+vv4IZ1QLCQE1H60smBEjDCvUzkPhufYkZloVsOU1A 7ijRrL1WpPL7yfjJrD3eBzoah4DG0hjEyvUoyMJX29i19QWxTsAlrElvDCrHTH8c FARR2NHLkhFJaB1GFyDWKF4p7zY1CYE6Tyj74BC7gKfx4GO6I7eTr6c2x8oRkdqU cysJ5aMeqhVtUM9wqQxPlLn5DZn4mhJ7IC3s+hiC8KZFXFLPru5Vvyjc82XPijrv 44uIsvMT6v3PTfXuzscr3kmb5NUM/FEI6vacsbg1OPQT2kzFiVCJOjVnPr6Dx9Kc /iEy45QPJKenDVZBMwSeaO3hhXaBRC+nlDEfaCP7SyNTRPRUu4AAkcJIgP07CUVg nrl0wokfwAh2WAgjZzwlciBEn85KKaMzsNExYAk4dj+nt0uVpLYNGq0jsnFrcNxB 362H4TM9HcEfa7soiAicwr48nroy/uS+GY5eJFX85GGAKL4Yt2+3fUKqmduhMZKM Sc4A552hqD8OjCsKgyJJ =gLwi -----END PGP SIGNATURE----- --4fo3mGi7Q6pk/+I3--