| Message ID | 1456823441-46757-10-git-send-email-aik@ozlabs.ru (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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; > /*
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 <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; >> /* >
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; > >> /* > > > >
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; /*
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. 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(-)