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

This adds an afu_driver_ops structure with fetch_event() and
event_delivered() callbacks. An AFU driver such as cxlflash can fill
this out and associate it with a context to enable passing custom
AFU specific events to userspace.

This also adds a new kernel API function cxl_context_pending_events(),
that the AFU driver can use to notify the cxl driver that new specific
events are ready to be delivered, and wake up anyone waiting on the
context wait queue.

The current count of AFU driver specific events is stored in the field
afu_driver_events of the context structure.

The cxl driver checks the afu_driver_events count during poll, select,
read, etc. calls to check if an AFU driver specific event is pending,
and calls fetch_event() to obtain and deliver that event. This way,
the cxl driver takes care of all the usual locking semantics around these
calls and handles all the generic cxl events, so that the AFU driver only
needs to worry about it's own events.

fetch_event() return a struct cxl_event_afu_driver_reserved, allocated
by the AFU driver, and filled in with the specific event information and
size. Data size should not be greater than CXL_MAX_EVENT_DATA_SIZE.

Th cxl driver prepends an appropriate cxl event header, copies the event
to userspace, and finally calls event_delivered() to return the status of
the operation to the AFU driver. The event is identified by the context
and cxl_event_afu_driver_reserved pointers.

Since AFU drivers provide their own means for userspace to obtain the
AFU file descriptor (i.e. cxlflash uses an ioctl on their scsi file
descriptor to obtain the AFU file descriptor) and the generic cxl driver
will never use this event, the ABI of the event is up to each individual
AFU driver.

Signed-off-by: Philippe Bergheaud <felix@linux.vnet.ibm.com>
---
Changes since v5:
- s/deliver_event/fetch_event/
- Fixed the handling of fetch_event errors
- Documented the return codes of event_delivered

Changes since v4:
- Addressed comments from Vaibhav:
  - Changed struct cxl_event_afu_driver_reserved from
    { __u64 reserved[4]; } to { size_t data_size; u8 data[]; }
  - Modified deliver_event to return a struct cxl_event_afu_driver_reserved
  - Added new callback event_delivered
  - Added static function afu_driver_event_copy

Changes since v3:
- Removed driver ops callback ctx_event_pending
- Created cxl function cxl_context_pending_events
- Created cxl function cxl_unset_driver_ops
- Added atomic event counter afu_driver_events

Changes since v2:
- Fixed some typos spotted by Matt Ochs

Changes since v1:
- Rebased on upstream
- Bumped cxl api version to 3
- Addressed comments from mpe:
  - Clarified commit message & some comments
  - Mentioned 'cxlflash' as a possible user of this event
  - Check driver ops on registration and warn if missing calls
  - Remove redundant checks where driver ops is used
  - Simplified ctx_event_pending and removed underscore version
  - Changed deliver_event to take the context as the first argument

 drivers/misc/cxl/Kconfig |  5 +++++
 drivers/misc/cxl/api.c   | 27 ++++++++++++++++++++++
 drivers/misc/cxl/cxl.h   |  7 +++++-
 drivers/misc/cxl/file.c  | 58 +++++++++++++++++++++++++++++++++++++++++-------
 include/misc/cxl.h       | 53 +++++++++++++++++++++++++++++++++++++++++++
 include/uapi/misc/cxl.h  | 21 ++++++++++++++++++
 6 files changed, 162 insertions(+), 9 deletions(-)

> On Jun 16, 2016, at 9:13 AM, Philippe Bergheaud <felix@linux.vnet.ibm.com> wrote:
> 
> This adds an afu_driver_ops structure with fetch_event() and
> event_delivered() callbacks. An AFU driver such as cxlflash can fill
> this out and associate it with a context to enable passing custom
> AFU specific events to userspace.
> 
> This also adds a new kernel API function cxl_context_pending_events(),
> that the AFU driver can use to notify the cxl driver that new specific
> events are ready to be delivered, and wake up anyone waiting on the
> context wait queue.
> 
> The current count of AFU driver specific events is stored in the field
> afu_driver_events of the context structure.
> 
> The cxl driver checks the afu_driver_events count during poll, select,
> read, etc. calls to check if an AFU driver specific event is pending,
> and calls fetch_event() to obtain and deliver that event. This way,
> the cxl driver takes care of all the usual locking semantics around these
> calls and handles all the generic cxl events, so that the AFU driver only
> needs to worry about it's own events.
> 
> fetch_event() return a struct cxl_event_afu_driver_reserved, allocated
> by the AFU driver, and filled in with the specific event information and
> size. Data size should not be greater than CXL_MAX_EVENT_DATA_SIZE.
> 
> Th cxl driver prepends an appropriate cxl event header, copies the event
> to userspace, and finally calls event_delivered() to return the status of
> the operation to the AFU driver. The event is identified by the context
> and cxl_event_afu_driver_reserved pointers.
> 
> Since AFU drivers provide their own means for userspace to obtain the
> AFU file descriptor (i.e. cxlflash uses an ioctl on their scsi file
> descriptor to obtain the AFU file descriptor) and the generic cxl driver
> will never use this event, the ABI of the event is up to each individual
> AFU driver.
> 
> Signed-off-by: Philippe Bergheaud <felix@linux.vnet.ibm.com>
> ---
> Changes since v5:
> - s/deliver_event/fetch_event/
> - Fixed the handling of fetch_event errors
> - Documented the return codes of event_delivered
> 
> Changes since v4:
> - Addressed comments from Vaibhav:
>  - Changed struct cxl_event_afu_driver_reserved from
>    { __u64 reserved[4]; } to { size_t data_size; u8 data[]; }
>  - Modified deliver_event to return a struct cxl_event_afu_driver_reserved
>  - Added new callback event_delivered
>  - Added static function afu_driver_event_copy
> 
> Changes since v3:
> - Removed driver ops callback ctx_event_pending
> - Created cxl function cxl_context_pending_events
> - Created cxl function cxl_unset_driver_ops
> - Added atomic event counter afu_driver_events
> 
> Changes since v2:
> - Fixed some typos spotted by Matt Ochs
> 
> Changes since v1:
> - Rebased on upstream
> - Bumped cxl api version to 3
> - Addressed comments from mpe:
>  - Clarified commit message & some comments
>  - Mentioned 'cxlflash' as a possible user of this event
>  - Check driver ops on registration and warn if missing calls
>  - Remove redundant checks where driver ops is used
>  - Simplified ctx_event_pending and removed underscore version
>  - Changed deliver_event to take the context as the first argument
> 
> drivers/misc/cxl/Kconfig |  5 +++++
> drivers/misc/cxl/api.c   | 27 ++++++++++++++++++++++
> drivers/misc/cxl/cxl.h   |  7 +++++-
> drivers/misc/cxl/file.c  | 58 +++++++++++++++++++++++++++++++++++++++++-------
> include/misc/cxl.h       | 53 +++++++++++++++++++++++++++++++++++++++++++
> include/uapi/misc/cxl.h  | 21 ++++++++++++++++++
> 6 files changed, 162 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/misc/cxl/Kconfig b/drivers/misc/cxl/Kconfig
> index 8756d06..560412c 100644
> --- a/drivers/misc/cxl/Kconfig
> +++ b/drivers/misc/cxl/Kconfig
> @@ -15,12 +15,17 @@ config CXL_EEH
> 	bool
> 	default n
> 
> +config CXL_AFU_DRIVER_OPS
> +	bool
> +	default n
> +
> config CXL
> 	tristate "Support for IBM Coherent Accelerators (CXL)"
> 	depends on PPC_POWERNV && PCI_MSI && EEH
> 	select CXL_BASE
> 	select CXL_KERNEL_API
> 	select CXL_EEH
> +	select CXL_AFU_DRIVER_OPS
> 	default m
> 	help
> 	  Select this option to enable driver support for IBM Coherent
> diff --git a/drivers/misc/cxl/api.c b/drivers/misc/cxl/api.c
> index 6d228cc..23f98f4 100644
> --- a/drivers/misc/cxl/api.c
> +++ b/drivers/misc/cxl/api.c
> @@ -323,6 +323,33 @@ struct cxl_context *cxl_fops_get_context(struct file *file)
> }
> EXPORT_SYMBOL_GPL(cxl_fops_get_context);
> 
> +void cxl_set_driver_ops(struct cxl_context *ctx,
> +			struct cxl_afu_driver_ops *ops)
> +{
> +	WARN_ON(!ops->fetch_event || !ops->event_delivered);
> +	atomic_set(&ctx->afu_driver_events, 0);
> +	ctx->afu_driver_ops = ops;
> +}
> +EXPORT_SYMBOL_GPL(cxl_set_driver_ops);
> +
> +int cxl_unset_driver_ops(struct cxl_context *ctx)
> +{
> +	if (atomic_read(&ctx->afu_driver_events))
> +		return -EBUSY;
> +
> +	ctx->afu_driver_ops = NULL;
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(cxl_unset_driver_ops);
> +
> +void cxl_context_events_pending(struct cxl_context *ctx,
> +				unsigned int new_events)
> +{
> +	atomic_add(new_events, &ctx->afu_driver_events);
> +	wake_up_all(&ctx->wq);
> +}
> +EXPORT_SYMBOL_GPL(cxl_context_events_pending);
> +
> int cxl_start_work(struct cxl_context *ctx,
> 		   struct cxl_ioctl_start_work *work)
> {
> diff --git a/drivers/misc/cxl/cxl.h b/drivers/misc/cxl/cxl.h
> index 4fe5078..b0027e6 100644
> --- a/drivers/misc/cxl/cxl.h
> +++ b/drivers/misc/cxl/cxl.h
> @@ -24,6 +24,7 @@
> #include <asm/reg.h>
> #include <misc/cxl-base.h>
> 
> +#include <misc/cxl.h>
> #include <uapi/misc/cxl.h>
> 
> extern uint cxl_verbose;
> @@ -34,7 +35,7 @@ extern uint cxl_verbose;
>  * Bump version each time a user API change is made, whether it is
>  * backwards compatible ot not.
>  */
> -#define CXL_API_VERSION 2
> +#define CXL_API_VERSION 3
> #define CXL_API_VERSION_COMPATIBLE 1
> 
> /*
> @@ -522,6 +523,10 @@ struct cxl_context {
> 	bool pending_fault;
> 	bool pending_afu_err;
> 
> +	/* Used by AFU drivers for driver specific event delivery */
> +	struct cxl_afu_driver_ops *afu_driver_ops;
> +	atomic_t afu_driver_events;
> +
> 	struct rcu_head rcu;
> };
> 
> diff --git a/drivers/misc/cxl/file.c b/drivers/misc/cxl/file.c
> index eec468f..cce2622 100644
> --- a/drivers/misc/cxl/file.c
> +++ b/drivers/misc/cxl/file.c
> @@ -293,6 +293,17 @@ int afu_mmap(struct file *file, struct vm_area_struct *vm)
> 	return cxl_context_iomap(ctx, vm);
> }
> 
> +static inline bool ctx_event_pending(struct cxl_context *ctx)
> +{
> +	if (ctx->pending_irq || ctx->pending_fault || ctx->pending_afu_err)
> +		return true;
> +
> +	if (ctx->afu_driver_ops && atomic_read(&ctx->afu_driver_events))
> +		return true;
> +
> +	return false;
> +}
> +
> unsigned int afu_poll(struct file *file, struct poll_table_struct *poll)
> {
> 	struct cxl_context *ctx = file->private_data;
> @@ -305,8 +316,7 @@ unsigned int afu_poll(struct file *file, struct poll_table_struct *poll)
> 	pr_devel("afu_poll wait done pe: %i\n", ctx->pe);
> 
> 	spin_lock_irqsave(&ctx->lock, flags);
> -	if (ctx->pending_irq || ctx->pending_fault ||
> -	    ctx->pending_afu_err)
> +	if (ctx_event_pending(ctx))
> 		mask |= POLLIN | POLLRDNORM;
> 	else if (ctx->status == CLOSED)
> 		/* Only error on closed when there are no futher events pending
> @@ -319,16 +329,40 @@ unsigned int afu_poll(struct file *file, struct poll_table_struct *poll)
> 	return mask;
> }
> 
> -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;
> +	}
> +
> +	/* Copy event header */
> +	event->header.size += pl->data_size;
> +	if (copy_to_user(buf, event, sizeof(struct cxl_event_header))) {
> +		ctx->afu_driver_ops->event_delivered(ctx, pl, -EFAULT);
> +		return -EFAULT;
> +	}
> +
> +	/* Copy event data */
> +	buf += sizeof(struct cxl_event_header);
> +	if (copy_to_user(buf, &pl->data, pl->data_size)) {
> +		ctx->afu_driver_ops->event_delivered(ctx, pl, -EFAULT);
> +		return -EFAULT;
> +	}
> +
> +	ctx->afu_driver_ops->event_delivered(ctx, pl, 0); /* Success */
> +	return event->header.size;
> }
> 
> ssize_t afu_read(struct file *file, char __user *buf, size_t count,
> 			loff_t *off)
> {
> 	struct cxl_context *ctx = file->private_data;
> +	struct cxl_event_afu_driver_reserved *pl = NULL;
> 	struct cxl_event event;
> 	unsigned long flags;
> 	int rc;
> @@ -344,7 +378,7 @@ ssize_t afu_read(struct file *file, char __user *buf, size_t count,
> 
> 	for (;;) {
> 		prepare_to_wait(&ctx->wq, &wait, TASK_INTERRUPTIBLE);
> -		if (ctx_event_pending(ctx))
> +		if (ctx_event_pending(ctx) || (ctx->status == CLOSED))
> 			break;
> 
> 		if (!cxl_ops->link_ok(ctx->afu->adapter, ctx->afu)) {
> @@ -374,7 +408,12 @@ ssize_t afu_read(struct file *file, char __user *buf, size_t count,
> 	memset(&event, 0, sizeof(event));
> 	event.header.process_element = ctx->pe;
> 	event.header.size = sizeof(struct cxl_event_header);
> -	if (ctx->pending_irq) {
> +	if (ctx->afu_driver_ops && atomic_read(&ctx->afu_driver_events)) {
> +		pr_devel("afu_read delivering AFU driver specific event\n");
> +		pl = ctx->afu_driver_ops->fetch_event(ctx);
> +		atomic_dec(&ctx->afu_driver_events);
> +		event.header.type = CXL_EVENT_AFU_DRIVER;
> +	} else if (ctx->pending_irq) {
> 		pr_devel("afu_read delivering AFU interrupt\n");
> 		event.header.size += sizeof(struct cxl_event_afu_interrupt);
> 		event.header.type = CXL_EVENT_AFU_INTERRUPT;
> @@ -404,6 +443,9 @@ ssize_t afu_read(struct file *file, char __user *buf, size_t count,
> 
> 	spin_unlock_irqrestore(&ctx->lock, flags);
> 
> +	if (event.header.type == CXL_EVENT_AFU_DRIVER)
> +		return afu_driver_event_copy(ctx, buf, &event, pl);
> +
> 	if (copy_to_user(buf, &event, event.header.size))
> 		return -EFAULT;
> 	return event.header.size;
> @@ -558,7 +600,7 @@ int __init cxl_file_init(void)
> 	 * If these change we really need to update API.  Either change some
> 	 * flags or update API version number CXL_API_VERSION.
> 	 */
> -	BUILD_BUG_ON(CXL_API_VERSION != 2);
> +	BUILD_BUG_ON(CXL_API_VERSION != 3);
> 	BUILD_BUG_ON(sizeof(struct cxl_ioctl_start_work) != 64);
> 	BUILD_BUG_ON(sizeof(struct cxl_event_header) != 8);
> 	BUILD_BUG_ON(sizeof(struct cxl_event_afu_interrupt) != 8);
> diff --git a/include/misc/cxl.h b/include/misc/cxl.h
> index 56560c5..1d8dde8 100644
> --- a/include/misc/cxl.h
> +++ b/include/misc/cxl.h
> @@ -220,4 +220,57 @@ void cxl_perst_reloads_same_image(struct cxl_afu *afu,
>  */
> ssize_t cxl_read_adapter_vpd(struct pci_dev *dev, void *buf, size_t count);
> 
> +/*
> + * AFU driver ops allow an AFU driver to create their own events to pass to
> + * userspace through the file descriptor as a simpler alternative to overriding
> + * the read() and poll() calls that works with the generic cxl events. These
> + * events are given priority over the generic cxl events, so they will be
> + * delivered first if multiple types of events are pending.
> + *
> + * The AFU driver must call cxl_context_events_pending() to notify the cxl
> + * driver that new events are ready to be delivered for a specific context.
> + * cxl_context_events_pending() will adjust the current count of AFU driver
> + * events for this context, and wake up anyone waiting on the context wait
> + * queue.
> + *
> + * The cxl driver will then call fetch_event() to get a structure defining
> + * the size and address of the driver specific event data. Data size cannot
> + * be greater than CXL_MAX_EVENT_DATA_SIZE. The cxl driver will build a cxl
> + * header with type and process_element fields filled in, and header.size
> + * set to sizeof(struct cxl_event_header) + data_size.
> + *
> + * fetch_event() is called with a spin lock held, so it must not sleep.
> + *
> + * The cxl driver will then deliver the event to userspace, and finally
> + * call event_delivered() to return the status of the operation, identified
> + * by its context and AFU driver event data pointers:
> + *   0        Success
> + *   -EFAULT  copy_to_user() has failed
> + *   -EINVAL  Event data pointer is NULL, or
> + *            event data size is greater than CXL_MAX_EVENT_DATA_SIZE
> + */
> +struct cxl_afu_driver_ops {
> +	struct cxl_event_afu_driver_reserved *(*fetch_event) (
> +						struct cxl_context *ctx);
> +	void (*event_delivered) (struct cxl_context *ctx,
> +				 struct cxl_event_afu_driver_reserved *event,
> +				 int rc);
> +};
> +
> +/*
> + * Associate the above driver ops with a specific context.
> + * Reset the current count of AFU driver events.
> + */
> +void cxl_set_driver_ops(struct cxl_context *ctx,
> +			struct cxl_afu_driver_ops *ops);
> +/*
> + * Remove the driver ops from a specific context.
> + * The current count of AFU driver events must be 0.
> + */
> +int cxl_unset_driver_ops(struct cxl_context *ctx);
> +
> +/* Notify cxl driver that new events are ready to be delivered for context */
> +void cxl_context_events_pending(struct cxl_context *ctx,
> +				unsigned int new_events);
> +
> #endif /* _MISC_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

Any reason this can't be larger if the user buffer is 4K?

Having a cap is good, and our current plans for exploiting this will
not use anywhere near this limit, however I feel that since we've
moved to a dynamic model, the size limit should be a bit more
accommodating - especially since changing this later would require
a version bump.

How about 2K?

> +
> +struct cxl_event_afu_driver_reserved {
> +	/*
> +	 * Defines the buffer passed to the cxl driver by the AFU driver.
> +	 *
> +	 * This is not ABI since the event header.size passed to the user for
> +	 * existing events is set in the read call to sizeof(cxl_event_header)
> +	 * + sizeof(whatever event is being dispatched) and the user is already
> +	 * required to use a 4K buffer on the read call.
> +	 *
> +	 * Of course the contents will be ABI, but that's up the AFU driver.
> +	 *
> +	 * data_size is limited to MAX_EVENT_DATA_SIZE to prevent abuse.
> +	 */
> +	size_t data_size;
> +	u8 data[];
> +};
> +
> struct cxl_event {
> 	struct cxl_event_header header;
> 	union {
> 		struct cxl_event_afu_interrupt irq;
> 		struct cxl_event_data_storage fault;
> 		struct cxl_event_afu_error afu_error;
> +		struct cxl_event_afu_driver_reserved afu_driver_event;
> 	};
> };
> 
> -- 
> 2.1.0
>

Hi Philippe,

Few points


Philippe Bergheaud <felix@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

Excerpts from Vaibhav Jain's message of 2016-06-20 14:20:16 +0530:
> > +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.

What situation do you think this will help? I haven't looked closely at
the last few iterations of this patch set, but if you're in a situation
where you might be racing with some code doing e.g.

if (ctx->afu_driver_ops)
	ctx->afu_driver_ops->something();

You have a race with or without a memory barrier. Ideally you would just
have the caller guarantee that it will only call cxl_unset_driver_ops if
no further calls to afu_driver_ops is possible, otherwise you may need
locking here which would be far from ideal.


What exactly is the use case for this API? I'd vote to drop it if we can
do without it.

-Ian

Hi Ian,

Ian Munsie <imunsie@au1.ibm.com> writes:

> Excerpts from Vaibhav Jain's message of 2016-06-20 14:20:16 +0530:
>> > +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.
>
> What situation do you think this will help? I haven't looked closely at
> the last few iterations of this patch set, but if you're in a situation
> where you might be racing with some code doing e.g.
>
> if (ctx->afu_driver_ops)
> 	ctx->afu_driver_ops->something();
>
> You have a race with or without a memory barrier. Ideally you would just
> have the caller guarantee that it will only call cxl_unset_driver_ops if
> no further calls to afu_driver_ops is possible, otherwise you may need
> locking here which would be far from ideal.
Yes, agree that wmb wont save against the race condition mentioned and
this is much better handled with locking. But imho having a wmb is still
better compared to having no locking for this shared variable.

>
> What exactly is the use case for this API? I'd vote to drop it if we can
> do without it.
Agree with this. Functionality of this API can be merged with
cxl_set_driver_ops when called with NULL arg for cxl_afu_driver_ops.

~ Vaibhav

> On Jun 21, 2016, at 5:34 AM, Vaibhav Jain <vaibhav@linux.vnet.ibm.com> wrote:
> 
> Hi Ian,
> 
> Ian Munsie <imunsie@au1.ibm.com> writes:
> 
>> Excerpts from Vaibhav Jain's message of 2016-06-20 14:20:16 +0530:
>> 
>> What exactly is the use case for this API? I'd vote to drop it if we can
>> do without it.
> Agree with this. Functionality of this API can be merged with
> cxl_set_driver_ops when called with NULL arg for cxl_afu_driver_ops.

Passing a NULL arg instead of calling an 'unset' API is fine with us.

I'll add that for cxlflash, I can't envision a scenario where we'll unset the
driver ops for a context.

Matthew R. Ochs wrote:
>>On Jun 21, 2016, at 5:34 AM, Vaibhav Jain <vaibhav@linux.vnet.ibm.com> wrote:
>>
>>Hi Ian,
>>
>>Ian Munsie <imunsie@au1.ibm.com> writes:
>>
>>
>>>Excerpts from Vaibhav Jain's message of 2016-06-20 14:20:16 +0530:
>>>
>>>What exactly is the use case for this API? I'd vote to drop it if we can
>>>do without it.
>>
>>Agree with this. Functionality of this API can be merged with
>>cxl_set_driver_ops when called with NULL arg for cxl_afu_driver_ops.
> 
> 
> Passing a NULL arg instead of calling an 'unset' API is fine with us.
> 
> I'll add that for cxlflash, I can't envision a scenario where we'll unset the
> driver ops for a context.
> 
Agreed, thanks. I will drop API cxl_unset_driver_ops() in v7.

Philippe

> 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
> 
[...]
>>+#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.
> 

afu_read() enforces a minimum buffer size of CXL_READ_MIN_SIZE = 4K, as documented in Documentation/powerpc/cxl.txt. This information is missing from the man pages of the libcxl functions cxl_read_event/cxl_read_expected_event. I will fix these.

Regarding the maximum event size, as afu_read returns one event per call, and as there is no API to tell userland the maximum size of a cxl event, I think that we should simply use (and document) the same value (4K) as the maximum cxl event size.

Philippe