[v6, 1/2] cxl: Add mechanism for delivering AFU driver specific events

[Vaibhav Jain]

Hi Philippe,

Few points


Philippe Bergheaud <felix at linux.vnet.ibm.com> writes:

> +int cxl_unset_driver_ops(struct cxl_context *ctx)
> +{
> +	if (atomic_read(&ctx->afu_driver_events))
> +		return -EBUSY;
> +
> +	ctx->afu_driver_ops = NULL;
Need a write memory barrier so that afu_driver_ops isnt possibly called
after this store.

>  
> -static inline int ctx_event_pending(struct cxl_context *ctx)
> +static ssize_t afu_driver_event_copy(struct cxl_context *ctx,
> +				     char __user *buf,
> +				     struct cxl_event *event,
> +				     struct cxl_event_afu_driver_reserved *pl)
>  {
> -	return (ctx->pending_irq || ctx->pending_fault ||
> -	    ctx->pending_afu_err || (ctx->status == CLOSED));
> +	/* Check data size */
> +	if (!pl || (pl->data_size > CXL_MAX_EVENT_DATA_SIZE)) {
> +		ctx->afu_driver_ops->event_delivered(ctx, pl, -EINVAL);
> +		return -EFAULT;
> +	}
Should also check against the length of user-buffer (count) provided in the read
call.Ideally this condition check should be moved to the read call where
you have access to the count variable.

Right now libcxl is using a harcoded value of CXL_READ_MIN_SIZE to
issue the read call and in kernel code we have a check to ensure that
read buffer is atleast CXL_READ_MIN_SIZE in size.

But it might be a good idea to decouple driver from
CXL_MAX_EVENT_DATA_SIZE. Ideally the maximum event size that we can
support should be dependent on the amount user buffer we receive in the
read call. That way future libcxl can support larger event_data without
needing a change to the cxl.h


> diff --git a/include/uapi/misc/cxl.h b/include/uapi/misc/cxl.h
> index 8cd334f..4fa36e4 100644
> --- a/include/uapi/misc/cxl.h
> +++ b/include/uapi/misc/cxl.h
> @@ -93,6 +93,7 @@ enum cxl_event_type {
>  	CXL_EVENT_AFU_INTERRUPT = 1,
>  	CXL_EVENT_DATA_STORAGE  = 2,
>  	CXL_EVENT_AFU_ERROR     = 3,
> +	CXL_EVENT_AFU_DRIVER    = 4,
>  };
>  
>  struct cxl_event_header {
> @@ -124,12 +125,32 @@ struct cxl_event_afu_error {
>  	__u64 error;
>  };
>  
> +#define CXL_MAX_EVENT_DATA_SIZE 128
> +

Agree with Matt's earlier comments. 128 is very small and I would prefer
for atleast a page size (4k/64K) limit.

Cheers,
~ Vaibhav