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

[Jason Gunthorpe]

On Thu, Sep 21, 2023 at 12:51:26AM -0700, Yi Liu wrote:
> From: Nicolin Chen <nicolinc@xxxxxxxxxx>
> 
> 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@xxxxxxxxxx>
> Signed-off-by: Yi Liu <yi.l.liu@xxxxxxxxx>
> ---
>  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