On Thu, Sep 21, 2023 at 12:51:26AM -0700, Yi Liu wrote:
From: Nicolin Chen nicolinc@nvidia.com
As one of the previous commits mentioned, a user-managed HWPT will have some different attributes/members. It'd be more clear by having separate allocators. Since the existing iommufd_hw_pagetable_alloc() serves well kernel-managed HWPTs, apply some minimal updates to mark it as a kernel- managed HWPT allocator.
Also, add a pair of function pointers (abort and destroy) in the struct, to separate different cleanup routines. Then rename the existing cleanup functions to iommufd_kernel_managed_hwpt_destroy/abort() linked to the HWPT in the allocator.
Signed-off-by: Nicolin Chen nicolinc@nvidia.com Signed-off-by: Yi Liu yi.l.liu@intel.com
drivers/iommu/iommufd/hw_pagetable.c | 34 ++++++++++++++++++++----- drivers/iommu/iommufd/iommufd_private.h | 3 +++ 2 files changed, 31 insertions(+), 6 deletions(-)
diff --git a/drivers/iommu/iommufd/hw_pagetable.c b/drivers/iommu/iommufd/hw_pagetable.c index 554a9c3d740f..1cc7178121d1 100644 --- a/drivers/iommu/iommufd/hw_pagetable.c +++ b/drivers/iommu/iommufd/hw_pagetable.c @@ -8,7 +8,7 @@ #include "../iommu-priv.h" #include "iommufd_private.h" -void iommufd_hw_pagetable_destroy(struct iommufd_object *obj) +static void iommufd_kernel_managed_hwpt_destroy(struct iommufd_object *obj) { struct iommufd_hw_pagetable *hwpt = container_of(obj, struct iommufd_hw_pagetable, obj); @@ -27,7 +27,12 @@ void iommufd_hw_pagetable_destroy(struct iommufd_object *obj) refcount_dec(&hwpt->ioas->obj.users); } -void iommufd_hw_pagetable_abort(struct iommufd_object *obj) +void iommufd_hw_pagetable_destroy(struct iommufd_object *obj) +{
- container_of(obj, struct iommufd_hw_pagetable, obj)->destroy(obj);
+}
+static void iommufd_kernel_managed_hwpt_abort(struct iommufd_object *obj) { struct iommufd_hw_pagetable *hwpt = container_of(obj, struct iommufd_hw_pagetable, obj); @@ -42,6 +47,11 @@ void iommufd_hw_pagetable_abort(struct iommufd_object *obj) iommufd_hw_pagetable_destroy(obj); } +void iommufd_hw_pagetable_abort(struct iommufd_object *obj) +{
- container_of(obj, struct iommufd_hw_pagetable, obj)->abort(obj);
+}
int iommufd_hw_pagetable_enforce_cc(struct iommufd_hw_pagetable *hwpt) { if (hwpt->enforce_cache_coherency) @@ -57,7 +67,7 @@ int iommufd_hw_pagetable_enforce_cc(struct iommufd_hw_pagetable *hwpt) } /**
- iommufd_hw_pagetable_alloc() - Get an iommu_domain for a device
- iommufd_hw_pagetable_alloc() - Get a kernel-managed iommu_domain for a device
- @ictx: iommufd context
- @ioas: IOAS to associate the domain with
- @idev: Device to get an iommu_domain for
@@ -66,9 +76,9 @@ int iommufd_hw_pagetable_enforce_cc(struct iommufd_hw_pagetable *hwpt)
- @user_data: Optional user_data pointer
- @immediate_attach: True if idev should be attached to the hwpt
- Allocate a new iommu_domain and return it as a hw_pagetable. The HWPT
- will be linked to the given ioas and upon return the underlying iommu_domain
- is fully popoulated.
- Allocate a new iommu_domain (must be IOMMU_DOMAIN_UNMANAGED) and return it as
- a kernel-managed hw_pagetable. The HWPT will be linked to the given ioas and
- upon return the underlying iommu_domain is fully popoulated.
- The caller must hold the ioas->mutex until after
- iommufd_object_abort_and_destroy() or iommufd_object_finalize() is called on
@@ -103,6 +113,8 @@ iommufd_hw_pagetable_alloc(struct iommufd_ctx *ictx, struct iommufd_ioas *ioas, /* Pairs with iommufd_hw_pagetable_destroy() */ refcount_inc(&ioas->obj.users); hwpt->ioas = ioas;
- hwpt->abort = iommufd_kernel_managed_hwpt_abort;
- hwpt->destroy = iommufd_kernel_managed_hwpt_destroy;
if (ops->domain_alloc_user) { hwpt->domain = ops->domain_alloc_user(idev->dev, flags, @@ -121,6 +133,16 @@ iommufd_hw_pagetable_alloc(struct iommufd_ctx *ictx, struct iommufd_ioas *ioas, } }
- if (WARN_ON_ONCE(hwpt->domain->type != IOMMU_DOMAIN_UNMANAGED)) {
rc = -EINVAL;goto out_abort;- }
- /* Driver is buggy by mixing user-managed op in kernel-managed ops */
- if (WARN_ON_ONCE(hwpt->domain->ops->cache_invalidate_user)) {
rc = -EINVAL;goto out_abort;- }
- /*
- Set the coherency mode before we do iopt_table_add_domain() as some
- iommus have a per-PTE bit that controls it and need to decide before
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.
An if of some sort in the two functions would be clearer
Jason