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 <a...@ozlabs.ru> > >>Reviewed-by: David Gibson <da...@gibson.dropbear.id.au> > > > >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] = tce; > >> > >> entry.target_as = &address_space_memory, > >>- entry.iova = ioba & page_mask; > >>+ entry.iova = (ioba - tcet->bus_offset) & page_mask; > >> entry.translated_addr = tce & page_mask; > >> entry.addr_mask = ~page_mask; > >> entry.perm = 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 = container_of(n, VFIOGuestIOMMU, n); > >> VFIOContainer *container = giommu->container; > >> IOMMUTLBEntry *iotlb = data; > >>+ hwaddr iova = 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 = 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) != IOMMU_NONE) { > >> vaddr = memory_region_get_ram_ptr(mr) + xlat; > >>- ret = vfio_dma_map(container, iotlb->iova, > >>+ ret = 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) = %d (%m)", > >>- container, iotlb->iova, > >>+ container, iova, > >> iotlb->addr_mask + 1, vaddr, ret); > >> } > >> } else { > >>- ret = vfio_dma_unmap(container, iotlb->iova, iotlb->addr_mask + 1); > >>+ ret = vfio_dma_unmap(container, iova, iotlb->addr_mask + 1); > >> if (ret) { > >> error_report("vfio_dma_unmap(%p, 0x%"HWADDR_PRIx", " > >> "0x%"HWADDR_PRIx") = %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 = g_malloc0(sizeof(*giommu)); > >> giommu->iommu = section->mr; > >>+ giommu->offset_within_address_space = > >>+ 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 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 = container; > >> giommu->n.notify = 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; > > > >...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. > > > 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. > > > > > >It might also be worth adding Paolo to the CC for this patch, since he > >knows the MemoryRegion stuff better than anyone. > > > Right, I added him in cc: now. > > > > >> Notifier n; > >> QLIST_ENTRY(VFIOGuestIOMMU) giommu_next; > >> } VFIOGuestIOMMU; > > > > -- 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
signature.asc
Description: PGP signature