Re: [PATCH v4 05/17] iommufd: Separate kernel-managed HWPT alloc/destroy/abort functions

[Jason Gunthorpe]

On Tue, Oct 10, 2023 at 03:49:32PM -0300, Jason Gunthorpe wrote:
> > diff --git a/drivers/iommu/iommufd/iommufd_private.h 
> > b/drivers/iommu/iommufd/iommufd_private.h
> > index 1d3b1a74e854..3e89c3d530f3 100644
> > --- a/drivers/iommu/iommufd/iommufd_private.h
> > +++ b/drivers/iommu/iommufd/iommufd_private.h
> > @@ -234,6 +234,9 @@ struct iommufd_hw_pagetable {
> >     struct iommufd_object obj;
> >     struct iommu_domain *domain;
> >  
> > +   void (*abort)(struct iommufd_object *obj);
> > +   void (*destroy)(struct iommufd_object *obj);
> > +
> >     union {
> >             struct { /* kernel-managed */
> >                     struct iommufd_ioas *ioas;
> 
> I think if you are doing this you are trying too hard to share the
> struct.. Defaintely want to avoid function pointers in general, and
> function pointers in a writable struct in particular.

I looked at this for a while and I do still have the feeling that
probably two structs and even two type IDs is probably a cleaner
design.

Like this:

// Or maybe use obj.type ?
enum iommufd_hw_pagetable_type {
        IOMMUFD_HWPT_PAGING,
        IOMMUFD_HWPT_NESTED,
};

struct iommufd_hw_pagetable_common {
        struct iommufd_object obj;
        struct iommu_domain *domain;
        enum iommufd_hw_pagetable_type type;
};

struct iommufd_hw_pagetable {
        struct iommufd_hw_pagetable_common common;
        struct iommufd_ioas *ioas;
        bool auto_domain : 1;
        bool enforce_cache_coherency : 1;
        bool msi_cookie : 1;
        /* Head at iommufd_ioas::hwpt_list */
        struct list_head hwpt_item;
};

struct iommufd_hw_pagetable_nested {
        struct iommufd_hw_pagetable_common common;
        // ??
};

I poked at it in an editor for a bit and it was looking OK but
requires breaking up a bunch of functions then I ran out of time

Also, we probably should feed enforce_cache_coherency through the
alloc_hwpt uapi and not try to autodetect it..

Jason