Re: [Qemu-devel] [Qemu-ppc] [PATCH qemu v13 09/16] vfio: Generalize IOMMU memory listener

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

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

On Thu, Mar 03, 2016 at 05:07:33PM +1100, Alexey Kardashevskiy wrote:
> On 03/03/2016 04:36 PM, David Gibson wrote:
> >On Tue, Mar 01, 2016 at 08:10:34PM +1100, Alexey Kardashevskiy wrote:
> >>At the moment VFIOContainer uses one memory listener which listens on
> >>PCI address space for both Type1 and sPAPR IOMMUs. Soon we will need
> >>another listener to listen on RAM; this will do DMA memory
> >>pre-registration for sPAPR guests which basically pins all guest
> >>pages in the host physical RAM.
> >>
> >>This introduces VFIOMemoryListener which is wrapper for MemoryListener
> >>and stores a pointer to the container. This allows having multiple
> >>memory listeners for the same container. This replaces the existing
> >>@listener with @iommu_listener.
> >>
> >>This should cause no change in behavior.
> >
> >This is nonsense.
> >
> >The two listeners you're talking about have (or should have) both a
> >different AS they're listening on,
> 
> They do have different AS.
> 
> >*and* different notification
> >functions.
> 
> They do use totally different region_add/region_del, later in the series.
> 
> >Since they have nothing in common, there's no point trying
> >to build a common structure for them.
> 
> They use the same VFIOContainer pointer. VFIOMemoryListener is made of
> MemoryListener and VFIOContainer, and that's it.

Right, but you don't need the container pointer.  In both cases you
can locate the VFIOContainer with container_of.  It's a different
container_of invocation in each case, but since they're different
callback functions, that's no problem.

> Ok, I'll get rid of VFIOMemoryListener. It is just hard sometime to
> understand what bits I have to reuse and which I do not, constant
> argument...

I think the arguments to try to make things re-used here were based on
a mis understanding of what the prereg listener was for and therefore
not realizing that it has basically nothing in common with the regular
listener.

> 
> >
> >>
> >>Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> >>---
> >>  hw/vfio/common.c              | 41 +++++++++++++++++++++++++++++++----------
> >>  include/hw/vfio/vfio-common.h |  9 ++++++++-
> >>  2 files changed, 39 insertions(+), 11 deletions(-)
> >>
> >>diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> >>index ca3fd47..0e67a5a 100644
> >>--- a/hw/vfio/common.c
> >>+++ b/hw/vfio/common.c
> >>@@ -318,10 +318,10 @@ static hwaddr vfio_container_granularity(VFIOContainer *container)
> >>      return (hwaddr)1 << ctz64(container->iova_pgsizes);
> >>  }
> >>
> >>-static void vfio_listener_region_add(MemoryListener *listener,
> >>+static void vfio_listener_region_add(VFIOMemoryListener *vlistener,
> >>                                       MemoryRegionSection *section)
> >>  {
> >>-    VFIOContainer *container = container_of(listener, VFIOContainer, listener);
> >>+    VFIOContainer *container = vlistener->container;
> >>      hwaddr iova, end;
> >>      Int128 llend;
> >>      void *vaddr;
> >>@@ -425,10 +425,10 @@ fail:
> >>      }
> >>  }
> >>
> >>-static void vfio_listener_region_del(MemoryListener *listener,
> >>+static void vfio_listener_region_del(VFIOMemoryListener *vlistener,
> >>                                       MemoryRegionSection *section)
> >>  {
> >>-    VFIOContainer *container = container_of(listener, VFIOContainer, listener);
> >>+    VFIOContainer *container = vlistener->container;
> >>      hwaddr iova, end;
> >>      int ret;
> >>      MemoryRegion *iommu = NULL;
> >>@@ -492,14 +492,33 @@ static void vfio_listener_region_del(MemoryListener *listener,
> >>      }
> >>  }
> >>
> >>-static const MemoryListener vfio_memory_listener = {
> >>-    .region_add = vfio_listener_region_add,
> >>-    .region_del = vfio_listener_region_del,
> >>+static void vfio_iommu_listener_region_add(MemoryListener *listener,
> >>+                                           MemoryRegionSection *section)
> >>+{
> >>+    VFIOMemoryListener *vlistener = container_of(listener, VFIOMemoryListener,
> >>+                                                 listener);
> >>+
> >>+    vfio_listener_region_add(vlistener, section);
> >>+}
> >>+
> >>+
> >>+static void vfio_iommu_listener_region_del(MemoryListener *listener,
> >>+                                           MemoryRegionSection *section)
> >>+{
> >>+    VFIOMemoryListener *vlistener = container_of(listener, VFIOMemoryListener,
> >>+                                                 listener);
> >>+
> >>+    vfio_listener_region_del(vlistener, section);
> >>+}
> >>+
> >>+static const MemoryListener vfio_iommu_listener = {
> >>+    .region_add = vfio_iommu_listener_region_add,
> >>+    .region_del = vfio_iommu_listener_region_del,
> >>  };
> >>
> >>  static void vfio_listener_release(VFIOContainer *container)
> >>  {
> >>-    memory_listener_unregister(&container->listener);
> >>+    memory_listener_unregister(&container->iommu_listener.listener);
> >>  }
> >>
> >>  int vfio_mmap_region(Object *obj, VFIORegion *region,
> >>@@ -768,9 +787,11 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as)
> >>          goto free_container_exit;
> >>      }
> >>
> >>-    container->listener = vfio_memory_listener;
> >>+    container->iommu_listener.container = container;
> >>+    container->iommu_listener.listener = vfio_iommu_listener;
> >>
> >>-    memory_listener_register(&container->listener, container->space->as);
> >>+    memory_listener_register(&container->iommu_listener.listener,
> >>+                             container->space->as);
> >>
> >>      if (container->error) {
> >>          ret = container->error;
> >>diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> >>index 9ffa681..b6b736c 100644
> >>--- a/include/hw/vfio/vfio-common.h
> >>+++ b/include/hw/vfio/vfio-common.h
> >>@@ -57,12 +57,19 @@ typedef struct VFIOAddressSpace {
> >>      QLIST_ENTRY(VFIOAddressSpace) list;
> >>  } VFIOAddressSpace;
> >>
> >>+typedef struct VFIOContainer VFIOContainer;
> >>+
> >>+typedef struct VFIOMemoryListener {
> >>+    struct MemoryListener listener;
> >>+    VFIOContainer *container;
> >>+} VFIOMemoryListener;
> >>+
> >>  struct VFIOGroup;
> >>
> >>  typedef struct VFIOContainer {
> >>      VFIOAddressSpace *space;
> >>      int fd; /* /dev/vfio/vfio, empowered by the attached groups */
> >>-    MemoryListener listener;
> >>+    VFIOMemoryListener iommu_listener;
> >>      int error;
> >>      bool initialized;
> >>      /*
> >
> 
> 

-- 
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 --]