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: 5496 bytes --]

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, *and* different notification
functions.  Since they have nothing in common, there's no point trying
to build a common structure for them.

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