| Message ID | 1442495357-26547-2-git-send-email-david@gibson.dropbear.id.au |
|---|---|
| State | New |
| Headers | show |
On 09/17/2015 11:09 PM, 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 > * There's no tag in the VFIOContainer to tell you which union member is > valid anyway. > > 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 <david@gibson.dropbear.id.au> > --- > hw/vfio/common.c | 51 +++++++++++++++++-------------------------- > include/hw/vfio/vfio-common.h | 14 +++--------- > 2 files changed, 23 insertions(+), 42 deletions(-) > > diff --git a/hw/vfio/common.c b/hw/vfio/common.c > index 6d21311..e3152f6 100644 > --- a/hw/vfio/common.c > +++ b/hw/vfio/common.c > @@ -316,7 +316,7 @@ static void vfio_listener_region_add(MemoryListener *listener, > MemoryRegionSection *section) > { > VFIOContainer *container = container_of(listener, VFIOContainer, > - iommu_data.type1.listener); > + iommu_data.listener); > hwaddr iova, end; > Int128 llend; > void *vaddr; > @@ -406,9 +406,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->iommu_data.initialized) { > + if (!container->iommu_data.error) { > + container->iommu_data.error = ret; > } > } else { > hw_error("vfio: DMA mapping failed, unable to continue"); > @@ -420,7 +420,7 @@ static void vfio_listener_region_del(MemoryListener *listener, > MemoryRegionSection *section) > { > VFIOContainer *container = container_of(listener, VFIOContainer, > - iommu_data.type1.listener); > + iommu_data.listener); > hwaddr iova, end; > int ret; > > @@ -485,7 +485,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->iommu_data.listener); > } > > int vfio_mmap_region(Object *obj, VFIORegion *region, > @@ -683,21 +683,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 +708,25 @@ 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->iommu_data.listener = vfio_memory_listener; > + > + memory_listener_register(&container->iommu_data.listener, > + container->space->as); > + > + if (container->iommu_data.error) { > + ret = container->iommu_data.error; > + error_report("vfio: memory listener initialization failed for container"); > + goto listener_release_exit; > + } > + > + container->iommu_data.initialized = true; > + > QLIST_INIT(&container->group_list); > QLIST_INSERT_HEAD(&space->containers, container, next); > > @@ -774,9 +765,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 59a321d..aff18cd 100644 > --- a/include/hw/vfio/vfio-common.h > +++ b/include/hw/vfio/vfio-common.h > @@ -64,21 +64,13 @@ 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 *); > + MemoryListener listener; > + int error; > + bool initialized; > } iommu_data; The only difference to my patch - "[RFC PATCH qemu 2/4] vfio: Generalize IOMMU memory listener" - is that you keep iommu_data struct which was supposed to have an union for type1/spapr/etc IOMMU data but this is gone. What is its purpose now? > QLIST_HEAD(, VFIOGuestIOMMU) giommu_list; > QLIST_HEAD(, VFIOGroup) group_list; >
On 17/09/15 15:09, 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 > * There's no tag in the VFIOContainer to tell you which union member is > valid anyway. > > 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 <david@gibson.dropbear.id.au> > --- > hw/vfio/common.c | 51 +++++++++++++++++-------------------------- > include/hw/vfio/vfio-common.h | 14 +++--------- > 2 files changed, 23 insertions(+), 42 deletions(-) ... > diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h > index 59a321d..aff18cd 100644 > --- a/include/hw/vfio/vfio-common.h > +++ b/include/hw/vfio/vfio-common.h > @@ -64,21 +64,13 @@ 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 *); > + MemoryListener listener; > + int error; > + bool initialized; > } iommu_data; > QLIST_HEAD(, VFIOGuestIOMMU) giommu_list; > QLIST_HEAD(, VFIOGroup) group_list; > I think I agree with Alexey here ... keeping the iommu_data struct around those fields looks cumbersome. Is there a reason you did not remove the struct completely? Thomas
On 17/09/2015 15:09, 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 > * There's no tag in the VFIOContainer to tell you which union member is > valid anyway. > > 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 <david@gibson.dropbear.id.au> > --- > hw/vfio/common.c | 51 +++++++++++++++++-------------------------- > include/hw/vfio/vfio-common.h | 14 +++--------- > 2 files changed, 23 insertions(+), 42 deletions(-) > > diff --git a/hw/vfio/common.c b/hw/vfio/common.c > index 6d21311..e3152f6 100644 > --- a/hw/vfio/common.c > +++ b/hw/vfio/common.c > @@ -316,7 +316,7 @@ static void vfio_listener_region_add(MemoryListener *listener, > MemoryRegionSection *section) > { > VFIOContainer *container = container_of(listener, VFIOContainer, > - iommu_data.type1.listener); > + iommu_data.listener); > hwaddr iova, end; > Int128 llend; > void *vaddr; > @@ -406,9 +406,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->iommu_data.initialized) { > + if (!container->iommu_data.error) { > + container->iommu_data.error = ret; > } > } else { > hw_error("vfio: DMA mapping failed, unable to continue"); > @@ -420,7 +420,7 @@ static void vfio_listener_region_del(MemoryListener *listener, > MemoryRegionSection *section) > { > VFIOContainer *container = container_of(listener, VFIOContainer, > - iommu_data.type1.listener); > + iommu_data.listener); > hwaddr iova, end; > int ret; > > @@ -485,7 +485,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->iommu_data.listener); > } > > int vfio_mmap_region(Object *obj, VFIORegion *region, > @@ -683,21 +683,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 +708,25 @@ 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->iommu_data.listener = vfio_memory_listener; > + > + memory_listener_register(&container->iommu_data.listener, > + container->space->as); > + > + if (container->iommu_data.error) { > + ret = container->iommu_data.error; > + error_report("vfio: memory listener initialization failed for container"); > + goto listener_release_exit; > + } > + > + container->iommu_data.initialized = true; > + > QLIST_INIT(&container->group_list); > QLIST_INSERT_HEAD(&space->containers, container, next); > > @@ -774,9 +765,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 59a321d..aff18cd 100644 > --- a/include/hw/vfio/vfio-common.h > +++ b/include/hw/vfio/vfio-common.h > @@ -64,21 +64,13 @@ 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 *); > + MemoryListener listener; > + int error; > + bool initialized; > } iommu_data; > QLIST_HEAD(, VFIOGuestIOMMU) giommu_list; > QLIST_HEAD(, VFIOGroup) group_list; > Reviewed-by: Laurent Vivier <lvivier@redhat.com>
On Wed, Sep 23, 2015 at 12:31:25PM +0200, Thomas Huth wrote: > On 17/09/15 15:09, 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 > > * There's no tag in the VFIOContainer to tell you which union member is > > valid anyway. > > > > 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 <david@gibson.dropbear.id.au> > > --- > > hw/vfio/common.c | 51 +++++++++++++++++-------------------------- > > include/hw/vfio/vfio-common.h | 14 +++--------- > > 2 files changed, 23 insertions(+), 42 deletions(-) > ... > > diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h > > index 59a321d..aff18cd 100644 > > --- a/include/hw/vfio/vfio-common.h > > +++ b/include/hw/vfio/vfio-common.h > > @@ -64,21 +64,13 @@ 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 *); > > + MemoryListener listener; > > + int error; > > + bool initialized; > > } iommu_data; > > QLIST_HEAD(, VFIOGuestIOMMU) giommu_list; > > QLIST_HEAD(, VFIOGroup) group_list; > > > > I think I agree with Alexey here ... keeping the iommu_data struct > around those fields looks cumbersome. Is there a reason you did not > remove the struct completely? Uh.. it seemed like a good idea at the time? I'll remove it in the next spin.
diff --git a/hw/vfio/common.c b/hw/vfio/common.c index 6d21311..e3152f6 100644 --- a/hw/vfio/common.c +++ b/hw/vfio/common.c @@ -316,7 +316,7 @@ static void vfio_listener_region_add(MemoryListener *listener, MemoryRegionSection *section) { VFIOContainer *container = container_of(listener, VFIOContainer, - iommu_data.type1.listener); + iommu_data.listener); hwaddr iova, end; Int128 llend; void *vaddr; @@ -406,9 +406,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->iommu_data.initialized) { + if (!container->iommu_data.error) { + container->iommu_data.error = ret; } } else { hw_error("vfio: DMA mapping failed, unable to continue"); @@ -420,7 +420,7 @@ static void vfio_listener_region_del(MemoryListener *listener, MemoryRegionSection *section) { VFIOContainer *container = container_of(listener, VFIOContainer, - iommu_data.type1.listener); + iommu_data.listener); hwaddr iova, end; int ret; @@ -485,7 +485,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->iommu_data.listener); } int vfio_mmap_region(Object *obj, VFIORegion *region, @@ -683,21 +683,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 +708,25 @@ 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->iommu_data.listener = vfio_memory_listener; + + memory_listener_register(&container->iommu_data.listener, + container->space->as); + + if (container->iommu_data.error) { + ret = container->iommu_data.error; + error_report("vfio: memory listener initialization failed for container"); + goto listener_release_exit; + } + + container->iommu_data.initialized = true; + QLIST_INIT(&container->group_list); QLIST_INSERT_HEAD(&space->containers, container, next); @@ -774,9 +765,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 59a321d..aff18cd 100644 --- a/include/hw/vfio/vfio-common.h +++ b/include/hw/vfio/vfio-common.h @@ -64,21 +64,13 @@ 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 *); + MemoryListener listener; + int error; + bool initialized; } iommu_data; QLIST_HEAD(, VFIOGuestIOMMU) giommu_list; QLIST_HEAD(, VFIOGroup) group_list;
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 * There's no tag in the VFIOContainer to tell you which union member is valid anyway. 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 <david@gibson.dropbear.id.au> --- hw/vfio/common.c | 51 +++++++++++++++++-------------------------- include/hw/vfio/vfio-common.h | 14 +++--------- 2 files changed, 23 insertions(+), 42 deletions(-)