From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44777) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZhCc4-0003Kr-7p for qemu-devel@nongnu.org; Wed, 30 Sep 2015 04:19:48 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZhCc0-0000xD-UP for qemu-devel@nongnu.org; Wed, 30 Sep 2015 04:19:44 -0400 References: <1443579237-9636-1-git-send-email-david@gibson.dropbear.id.au> <1443579237-9636-2-git-send-email-david@gibson.dropbear.id.au> From: Laurent Vivier Message-ID: <560B9B17.5020605@redhat.com> Date: Wed, 30 Sep 2015 10:19:35 +0200 MIME-Version: 1.0 In-Reply-To: <1443579237-9636-2-git-send-email-david@gibson.dropbear.id.au> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCHv3 1/7] vfio: Remove unneeded union from VFIOContainer List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: David Gibson , alex.williamson@redhat.com, pbonzini@redhat.com Cc: thuth@redhat.com, qemu-devel@nongnu.org, mdroth@linux.vnet.ibm.com, aik@ozlabs.ru, abologna@redhat.com, gwshan@linux.vnet.ibm.com, qemu-ppc@nongnu.org On 30/09/2015 04:13, David Gibson wrote: > Currently the VFIOContainer iommu_data field contains a union with > different information for different host iommu types. However: > * It only actually contains information for the x86-like "Type1" iommu > * Because we have a common listener the Type1 fields are actually used > on all IOMMU types, including the SPAPR TCE type as well > > In fact we now have a general structure for the listener which is unlikely > to ever need per-iommu-type information, so this patch removes the union. > > In a similar way we can unify the setup of the vfio memory listener in > vfio_connect_container() that is currently split across a switch on iommu > type, but is effectively the same in both cases. > > The iommu_data.release pointer was only needed as a cleanup function > which would handle potentially different data in the union. With the > union gone, it too can be removed. > > Signed-off-by: David Gibson > --- > hw/vfio/common.c | 52 ++++++++++++++++--------------------------- > include/hw/vfio/vfio-common.h | 16 +++---------- > 2 files changed, 22 insertions(+), 46 deletions(-) > > diff --git a/hw/vfio/common.c b/hw/vfio/common.c > index 0d341a3..1545f62 100644 > --- a/hw/vfio/common.c > +++ b/hw/vfio/common.c > @@ -315,8 +315,7 @@ out: > static void vfio_listener_region_add(MemoryListener *listener, > MemoryRegionSection *section) > { > - VFIOContainer *container = container_of(listener, VFIOContainer, > - iommu_data.type1.listener); > + VFIOContainer *container = container_of(listener, VFIOContainer, listener); > hwaddr iova, end; > Int128 llend; > void *vaddr; > @@ -406,9 +405,9 @@ static void vfio_listener_region_add(MemoryListener *listener, > * can gracefully fail. Runtime, there's not much we can do other > * than throw a hardware error. > */ > - if (!container->iommu_data.type1.initialized) { > - if (!container->iommu_data.type1.error) { > - container->iommu_data.type1.error = ret; > + if (!container->initialized) { > + if (!container->error) { > + container->error = ret; > } > } else { > hw_error("vfio: DMA mapping failed, unable to continue"); > @@ -419,8 +418,7 @@ static void vfio_listener_region_add(MemoryListener *listener, > static void vfio_listener_region_del(MemoryListener *listener, > MemoryRegionSection *section) > { > - VFIOContainer *container = container_of(listener, VFIOContainer, > - iommu_data.type1.listener); > + VFIOContainer *container = container_of(listener, VFIOContainer, listener); > hwaddr iova, end; > int ret; > > @@ -485,7 +483,7 @@ static const MemoryListener vfio_memory_listener = { > > static void vfio_listener_release(VFIOContainer *container) > { > - memory_listener_unregister(&container->iommu_data.type1.listener); > + memory_listener_unregister(&container->listener); > } > > int vfio_mmap_region(Object *obj, VFIORegion *region, > @@ -683,21 +681,6 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as) > ret = -errno; > goto free_container_exit; > } > - > - container->iommu_data.type1.listener = vfio_memory_listener; > - container->iommu_data.release = vfio_listener_release; > - > - memory_listener_register(&container->iommu_data.type1.listener, > - container->space->as); > - > - if (container->iommu_data.type1.error) { > - ret = container->iommu_data.type1.error; > - error_report("vfio: memory listener initialization failed for container"); > - goto listener_release_exit; > - } > - > - container->iommu_data.type1.initialized = true; > - > } else if (ioctl(fd, VFIO_CHECK_EXTENSION, VFIO_SPAPR_TCE_IOMMU)) { > ret = ioctl(group->fd, VFIO_GROUP_SET_CONTAINER, &fd); > if (ret) { > @@ -723,19 +706,24 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as) > ret = -errno; > goto free_container_exit; > } > - > - container->iommu_data.type1.listener = vfio_memory_listener; > - container->iommu_data.release = vfio_listener_release; > - > - memory_listener_register(&container->iommu_data.type1.listener, > - container->space->as); > - > } else { > error_report("vfio: No available IOMMU models"); > ret = -EINVAL; > goto free_container_exit; > } > > + container->listener = vfio_memory_listener; > + > + memory_listener_register(&container->listener, container->space->as); > + > + if (container->error) { > + ret = container->error; > + error_report("vfio: memory listener initialization failed for container"); > + goto listener_release_exit; > + } > + > + container->initialized = true; > + > QLIST_INIT(&container->group_list); > QLIST_INSERT_HEAD(&space->containers, container, next); > > @@ -774,9 +762,7 @@ static void vfio_disconnect_container(VFIOGroup *group) > VFIOAddressSpace *space = container->space; > VFIOGuestIOMMU *giommu, *tmp; > > - if (container->iommu_data.release) { > - container->iommu_data.release(container); > - } > + vfio_listener_release(container); > QLIST_REMOVE(container, next); > > QLIST_FOREACH_SAFE(giommu, &container->giommu_list, giommu_next, tmp) { > diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h > index 9b9901f..fbbe6de 100644 > --- a/include/hw/vfio/vfio-common.h > +++ b/include/hw/vfio/vfio-common.h > @@ -59,22 +59,12 @@ typedef struct VFIOAddressSpace { > > struct VFIOGroup; > > -typedef struct VFIOType1 { > - MemoryListener listener; > - int error; > - bool initialized; > -} VFIOType1; > - > typedef struct VFIOContainer { > VFIOAddressSpace *space; > int fd; /* /dev/vfio/vfio, empowered by the attached groups */ > - struct { > - /* enable abstraction to support various iommu backends */ > - union { > - VFIOType1 type1; > - }; > - void (*release)(struct VFIOContainer *); > - } iommu_data; > + MemoryListener listener; > + int error; > + bool initialized; > QLIST_HEAD(, VFIOGuestIOMMU) giommu_list; > QLIST_HEAD(, VFIOGroup) group_list; > QLIST_ENTRY(VFIOContainer) next; > Reviewed-by: Laurent Vivier