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