Re: [Qemu-devel] [PATCH qemu v14 01/18] memory: Fix IOMMU replay base address

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 7149 bytes --]

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 <aik@ozlabs.ru>
> >>Reviewed-by: David Gibson <david@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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]