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
