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