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

[Alexey Kardashevskiy]

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.

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

> > Signed-off-by: Alexey Kardashevskiy <address@hidden>
> > ---
> >   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;
> >       /*


--
Alexey