Re: [Qemu-ppc] [Qemu-devel] [RFC PATCH 01/10] vfio: Remove unneeded union from VFIOContainer

[David Gibson]

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 <address@hidden>
> > ---
> >  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.

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

pgpm09RhyF7dh.pgp
Description: PGP signature