From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58257) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aiVPX-0006kn-2D for qemu-devel@nongnu.org; Tue, 22 Mar 2016 19:08:28 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aiVPV-0005e6-6y for qemu-devel@nongnu.org; Tue, 22 Mar 2016 19:08:26 -0400 Date: Wed, 23 Mar 2016 10:07:00 +1100 From: David Gibson Message-ID: <20160322230700.GI23586@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> <20160322045922.GF23586@voom.redhat.com> <56F0F20C.2040300@ozlabs.ru> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="zuGyJ7KNSMOGsxcs" Content-Disposition: inline In-Reply-To: <56F0F20C.2040300@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 --zuGyJ7KNSMOGsxcs Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Mar 22, 2016 at 06:19:40PM +1100, Alexey Kardashevskiy wrote: > On 03/22/2016 03:59 PM, David Gibson wrote: > >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 *= tcet, 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_sp= ace; > >>>>> > >>>>>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; > >>>>>> > >>>>>>- 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->reado= nly); > >>>>>> 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(MemoryList= ener *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 rela= tive 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, > >> > >>My imagination fails here. How could you do this in practice? > >> > >>address_space_init(&as, &root) > >>memory_region_init(&mr, 2GB) > >>memory_region_add_subregion(&root, -1GB, &mr) > >> > >>But offsets are unsigned. > >> > >>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: >=20 >=20 > Ok. Got it. So, how about this: >=20 > s/offset_within_address_space/iommu_offset/ >=20 > and >=20 > giommu->iommu_offset =3D section->offset_within_address_space - > section->offset_within_region; Yes, that should do it. >=20 > ? >=20 >=20 > > > >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 > I was not arguing this, I was trying to imagine :) >=20 >=20 >=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 --zuGyJ7KNSMOGsxcs Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJW8dAUAAoJEGw4ysog2bOSwcgP/1GCxT1j3PuoalBebxRB4Xgc Uo4f/P8cXYizP61sPSWoqvkHkyuYtUAXD2wHo2+IHjqljgQjt0n7GpylPnXvHztv WS6RCmgaujpF9Wx1RV+d4BWlIrKUMNX/EM6lHn6idLkJmYpoSOoTFZMmGsgG6Rdh uYJb6twftKzgyq0VVNN6LikZnSJCP5HGLimjl+nHAFpr4UkpnSeSNEonDssJtuV8 Ooz79ZsuXVnNbULn7B/9MHCjB+GYAR4aUb7WqceP2y4xOnDOJTbICAj90KWub8QY 5cQT3WseKXa5EPbJniXHVLtu0hTsR/1qL9vAgpmRxM3+iTCyt2o6K7r6PeekT2ip emNz2boBJQplYVd9wEj9nXb1NZqsjMOr8/BAsZnM+/fNg5cQC3iJEyB9cpYTrdxA JgNkhkxkAmkwsJpWvweC4QcKSIeBRCOoy70yx7NWENSFmjrEQQkptUiFWg/GIuMN AymLVtPXFq0rNG32a+8LijCTlPeGnrkYHs3EYKNaLGbfH6eDBefQ+QuWRk9qBg0p rajAg4G4TwFPd6+6F0IBsguvY7X1RhohWUgYSlC1DFcZOxMyoMd4yalc+XLvW9CB AiSqxINrtB6LGp1i7i7hMWwKGnypdPSvbyNQd3rw+EroTBig1mnDDSMf/E3MUBvQ HxG+OC5NdmJhsnEjqipP =SC6/ -----END PGP SIGNATURE----- --zuGyJ7KNSMOGsxcs--