From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39043) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aiDRs-0002vo-Qd for qemu-devel@nongnu.org; Mon, 21 Mar 2016 23:57:42 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aiDRo-0001VL-PE for qemu-devel@nongnu.org; Mon, 21 Mar 2016 23:57:40 -0400 Date: Tue, 22 Mar 2016 14:26:26 +1100 From: David Gibson Message-ID: <20160322032626.GA23586@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> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="zYtRAvLPyLojxZ7Y" Content-Disposition: inline In-Reply-To: <56F0B81E.9080701@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 --zYtRAvLPyLojxZ7Y Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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, vo= id *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_with= in_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, vo= id *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->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_mas= k + 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. >=20 > But the IOMMU MR stays the same - size, offset, and iova will be relative= 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, then an IOMMUTLBEntry iova of 1G would translated to AS address 0. > >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_nex= t); > >>diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-commo= n.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; > > > >...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. >=20 >=20 > MemoryRegionSection is allocated on stack in listener_add_address_space() > and seems to be in general some sort of temporary object. Ah, right, I guess you'll have to store the delta, then. >=20 >=20 > > > >It might also be worth adding Paolo to the CC for this patch, since he > >knows the MemoryRegion stuff better than anyone. >=20 >=20 > Right, I added him in cc: now. >=20 > > > >> Notifier n; > >> QLIST_ENTRY(VFIOGuestIOMMU) giommu_next; > >> } VFIOGuestIOMMU; > > >=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 --zYtRAvLPyLojxZ7Y Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJW8LtiAAoJEGw4ysog2bOSLUUP/idUnQNXRNgjRKir6gDLN2mW 9fwOV0+bMhDUJKTppvaoG/Nk0/FsG/DTLf7C7qeZjFO3iDNGH/ygkSILDwvzwE5o RWWXun2ZcRLD8NtWDhvjW4f32WAHHlp4XEjJiR202wf9cT+cSl57hTNLQ/xQ0oTF f/STou6amCZ6KEFMJQQa+AIu1o5yhjFQe87fYaKyJEzsNKV/oEsBYVQy+7uo/1MG iSl9ujDvTbNxIg0h8hRNfuikIoL2ajDBrhZEAtd538kAYyOLTrVOjwDcVu9XYLLC rayuj8Hf4FrKk6tONmYSjqimnMtjNh8ZTBnHChZAz73EWYtDNtv+HvPLgPC/jL5b NX1TCxGVpe4VwdR8QBxZCSHHwYuCgQeoPb1KhRoKgDyriKBvs7t0jkIRl0WGvkui W6zGMImcqzZg7hU/NuFcyO9ueoZHpaOjTiCdK/+pT73C0bwArNgNgxQ6hFIG/nTA AVMskXleYJmxi69g4XSQV/GmZgCeCx2F0Ku5HamFCdEC8KBqFAJrJM+IEwphjv5X uVqF1sf+/Uby5r0yuM3SzJxNrFGVu8Dk3hWtmKPlGsh2m5UaNzD1Q1Ur2Jh55Jf4 ZYohpNC/lxnaY2GUxFhrrK6AP73Kx5mRAyHggnXVsbUwLdXPYui6Bs1NBCdKQpL4 llmBn3EakQMkY63uEbhq =2MBR -----END PGP SIGNATURE----- --zYtRAvLPyLojxZ7Y--