Re: [PATCH v4 4/7] ocxl: Allow external drivers to use OpenCAPI contexts

[Frederic Barrat]

Le 27/03/2019 Ã 06:31, Alastair D'Silva a ÃcritÂ:
> From: Alastair D'Silva <alastair@xxxxxxxxxxx>
>
> Most OpenCAPI operations require a valid context, so
> exposing these functions to external drivers is necessary.
>
> Signed-off-by: Alastair D'Silva <alastair@xxxxxxxxxxx>
> Reviewed-by: Greg Kurz <groug@xxxxxxxx>
> ---


There's a small memory leak in here on context alloc, if we can't get a 
pasid from the IDR. But that's not new to this patch, I just saw it 
because the code was moved around. And since that's the only thing I've 
seen in the full series and it's super long to review, I'll address it 
with a later patch. So:

Acked-by: Frederic Barrat <fbarrat@xxxxxxxxxxxxx>



>   drivers/misc/ocxl/context.c       | 22 ++++++++++-------
>   drivers/misc/ocxl/file.c          |  8 ++-----
>   drivers/misc/ocxl/ocxl_internal.h |  6 -----
>   include/misc/ocxl.h               | 39 +++++++++++++++++++++++++++++++
>   4 files changed, 55 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/misc/ocxl/context.c b/drivers/misc/ocxl/context.c
> index c73a859d2224..b60e674ac019 100644
> --- a/drivers/misc/ocxl/context.c
> +++ b/drivers/misc/ocxl/context.c
> @@ -4,15 +4,17 @@
>   #include "trace.h"
>   #include "ocxl_internal.h"
>   
> -struct ocxl_context *ocxl_context_alloc(void)
> -{
> -	return kzalloc(sizeof(struct ocxl_context), GFP_KERNEL);
> -}
> -
> -int ocxl_context_init(struct ocxl_context *ctx, struct ocxl_afu *afu,
> +int ocxl_context_alloc(struct ocxl_context **context, struct ocxl_afu *afu,
>   		struct address_space *mapping)
>   {
>   	int pasid;
> +	struct ocxl_context *ctx;
> +
> +	*context = kzalloc(sizeof(struct ocxl_context), GFP_KERNEL);
> +	if (!*context)
> +		return -ENOMEM;
> +
> +	ctx = *context;
>   
>   	ctx->afu = afu;
>   	mutex_lock(&afu->contexts_lock);
> @@ -43,6 +45,7 @@ int ocxl_context_init(struct ocxl_context *ctx, struct ocxl_afu *afu,
>   	ocxl_afu_get(afu);
>   	return 0;
>   }
> +EXPORT_SYMBOL_GPL(ocxl_context_alloc);
>   
>   /*
>    * Callback for when a translation fault triggers an error
> @@ -63,7 +66,7 @@ static void xsl_fault_error(void *data, u64 addr, u64 dsisr)
>   	wake_up_all(&ctx->events_wq);
>   }
>   
> -int ocxl_context_attach(struct ocxl_context *ctx, u64 amr)
> +int ocxl_context_attach(struct ocxl_context *ctx, u64 amr, struct mm_struct *mm)
>   {
>   	int rc;
>   
> @@ -75,7 +78,7 @@ int ocxl_context_attach(struct ocxl_context *ctx, u64 amr)
>   	}
>   
>   	rc = ocxl_link_add_pe(ctx->afu->fn->link, ctx->pasid,
> -			current->mm->context.id, ctx->tidr, amr, current->mm,
> +			mm->context.id, ctx->tidr, amr, mm,
>   			xsl_fault_error, ctx);
>   	if (rc)
>   		goto out;
> @@ -85,6 +88,7 @@ int ocxl_context_attach(struct ocxl_context *ctx, u64 amr)
>   	mutex_unlock(&ctx->status_mutex);
>   	return rc;
>   }
> +EXPORT_SYMBOL_GPL(ocxl_context_attach);
>   
>   static vm_fault_t map_afu_irq(struct vm_area_struct *vma, unsigned long address,
>   		u64 offset, struct ocxl_context *ctx)
> @@ -243,6 +247,7 @@ int ocxl_context_detach(struct ocxl_context *ctx)
>   	}
>   	return 0;
>   }
> +EXPORT_SYMBOL_GPL(ocxl_context_detach);
>   
>   void ocxl_context_detach_all(struct ocxl_afu *afu)
>   {
> @@ -280,3 +285,4 @@ void ocxl_context_free(struct ocxl_context *ctx)
>   	ocxl_afu_put(ctx->afu);
>   	kfree(ctx);
>   }
> +EXPORT_SYMBOL_GPL(ocxl_context_free);
> diff --git a/drivers/misc/ocxl/file.c b/drivers/misc/ocxl/file.c
> index 7a38ea5af9db..8225892a5d77 100644
> --- a/drivers/misc/ocxl/file.c
> +++ b/drivers/misc/ocxl/file.c
> @@ -61,11 +61,7 @@ static int afu_open(struct inode *inode, struct file *file)
>   	if (!info)
>   		return -ENODEV;
>   
> -	ctx = ocxl_context_alloc();
> -	if (!ctx)
> -		return -ENOMEM;
> -
> -	rc = ocxl_context_init(ctx, info->afu, inode->i_mapping);
> +	rc = ocxl_context_alloc(&ctx, info->afu, inode->i_mapping);
>   	if (rc)
>   		return rc;
>   
> @@ -90,7 +86,7 @@ static long afu_ioctl_attach(struct ocxl_context *ctx,
>   		return -EINVAL;
>   
>   	amr = arg.amr & mfspr(SPRN_UAMOR);
> -	rc = ocxl_context_attach(ctx, amr);
> +	rc = ocxl_context_attach(ctx, amr, current->mm);
>   	return rc;
>   }
>   
> diff --git a/drivers/misc/ocxl/ocxl_internal.h b/drivers/misc/ocxl/ocxl_internal.h
> index 53b6c64a1bf0..de6c16237742 100644
> --- a/drivers/misc/ocxl/ocxl_internal.h
> +++ b/drivers/misc/ocxl/ocxl_internal.h
> @@ -130,15 +130,9 @@ int ocxl_config_check_afu_index(struct pci_dev *dev,
>    */
>   int ocxl_link_update_pe(void *link_handle, int pasid, __u16 tid);
>   
> -struct ocxl_context *ocxl_context_alloc(void);
> -int ocxl_context_init(struct ocxl_context *ctx, struct ocxl_afu *afu,
> -			struct address_space *mapping);
> -int ocxl_context_attach(struct ocxl_context *ctx, u64 amr);
>   int ocxl_context_mmap(struct ocxl_context *ctx,
>   			struct vm_area_struct *vma);
> -int ocxl_context_detach(struct ocxl_context *ctx);
>   void ocxl_context_detach_all(struct ocxl_afu *afu);
> -void ocxl_context_free(struct ocxl_context *ctx);
>   
>   int ocxl_sysfs_register_afu(struct ocxl_file_info *info);
>   void ocxl_sysfs_unregister_afu(struct ocxl_file_info *info);
> diff --git a/include/misc/ocxl.h b/include/misc/ocxl.h
> index 8bafd748e380..e4704632eac5 100644
> --- a/include/misc/ocxl.h
> +++ b/include/misc/ocxl.h
> @@ -48,6 +48,7 @@ struct ocxl_fn_config {
>   // These are opaque outside the ocxl driver
>   struct ocxl_afu;
>   struct ocxl_fn;
> +struct ocxl_context;
>   
>   // Device detection & initialisation
>   
> @@ -116,6 +117,44 @@ const struct ocxl_fn_config *ocxl_function_config(struct ocxl_fn *fn);
>    */
>   void ocxl_function_close(struct ocxl_fn *fn);
>   
> +// Context allocation
> +
> +/**
> + * Allocate an OpenCAPI context
> + *
> + * @context: The OpenCAPI context to allocate, must be freed with ocxl_context_free
> + * @afu: The AFU the context belongs to
> + * @mapping: The mapping to unmap when the context is closed (may be NULL)
> + */
> +int ocxl_context_alloc(struct ocxl_context **context, struct ocxl_afu *afu,
> +			struct address_space *mapping);
> +
> +/**
> + * Free an OpenCAPI context
> + *
> + * @ctx: The OpenCAPI context to free
> + */
> +void ocxl_context_free(struct ocxl_context *ctx);
> +
> +/**
> + * Grant access to an MM to an OpenCAPI context
> + * @ctx: The OpenCAPI context to attach
> + * @amr: The value of the AMR register to restrict access
> + * @mm: The mm to attach to the context
> + *
> + * Returns 0 on success, negative on failure
> + */
> +int ocxl_context_attach(struct ocxl_context *ctx, u64 amr,
> +				struct mm_struct *mm);
> +
> +/**
> + * Detach an MM from an OpenCAPI context
> + * @ctx: The OpenCAPI context to attach
> + *
> + * Returns 0 on success, negative on failure
> + */
> +int ocxl_context_detach(struct ocxl_context *ctx);
> +
>   // AFU Metadata
>   
>   /**