Re: [PATCH v5 2/3] uacce: add uacce driver

[Jonathan Cameron]

On Mon, 14 Oct 2019 14:48:54 +0800
Zhangfei Gao <zhangfei.gao@xxxxxxxxxx> wrote:

> From: Kenneth Lee <liguozhu@xxxxxxxxxxxxx>
> 
> Uacce (Unified/User-space-access-intended Accelerator Framework) targets to
> provide Shared Virtual Addressing (SVA) between accelerators and processes.
> So accelerator can access any data structure of the main cpu.
> This differs from the data sharing between cpu and io device, which share
> data content rather than address.
> Since unified address, hardware and user space of process can share the
> same virtual address in the communication.
> 
> Uacce create a chrdev for every registration, the queue is allocated to
> the process when the chrdev is opened. Then the process can access the
> hardware resource by interact with the queue file. By mmap the queue
> file space to user space, the process can directly put requests to the
> hardware without syscall to the kernel space.
> 
> Signed-off-by: Kenneth Lee <liguozhu@xxxxxxxxxxxxx>
> Signed-off-by: Zaibo Xu <xuzaibo@xxxxxxxxxx>
> Signed-off-by: Zhou Wang <wangzhou1@xxxxxxxxxxxxx>
> Signed-off-by: Zhangfei Gao <zhangfei.gao@xxxxxxxxxx>

Hi,

Some superficial comments from me.

Thanks,

Jonathan


...

> +/*
> + * While user space releases a queue, all the relatives on the queue
> + * should be released immediately by this putting.

This one needs rewording but I'm not quite sure what
relatives are in this case.
 
> + */
> +static long uacce_put_queue(struct uacce_queue *q)
> +{
> +	struct uacce_device *uacce = q->uacce;
> +
> +	mutex_lock(&uacce_mutex);
> +
> +	if ((q->state == UACCE_Q_STARTED) && uacce->ops->stop_queue)
> +		uacce->ops->stop_queue(q);
> +
> +	if ((q->state == UACCE_Q_INIT || q->state == UACCE_Q_STARTED) &&
> +	     uacce->ops->put_queue)
> +		uacce->ops->put_queue(q);
> +
> +	q->state = UACCE_Q_ZOMBIE;
> +	mutex_unlock(&uacce_mutex);
> +
> +	return 0;
> +}
> +
..

> +
> +static ssize_t qfrs_size_show(struct device *dev,
> +				struct device_attribute *attr, char *buf)
> +{
> +	struct uacce_device *uacce = to_uacce_device(dev);
> +	unsigned long size;
> +	int i, ret;
> +
> +	for (i = 0, ret = 0; i < UACCE_QFRT_MAX; i++) {
> +		size = uacce->qf_pg_size[i] << PAGE_SHIFT;
> +		if (i == UACCE_QFRT_SS)
> +			break;
> +		ret += sprintf(buf + ret, "%lu\t", size);
> +	}
> +	ret += sprintf(buf + ret, "%lu\n", size);
> +
> +	return ret;
> +}

This may break the sysfs rule of one thing per file.  If you have
multiple regions, they should probably each have their own file
to give their size.

> +
> +static DEVICE_ATTR_RO(id);
> +static DEVICE_ATTR_RO(api);
> +static DEVICE_ATTR_RO(numa_distance);
> +static DEVICE_ATTR_RO(node_id);
> +static DEVICE_ATTR_RO(flags);
> +static DEVICE_ATTR_RO(available_instances);
> +static DEVICE_ATTR_RO(algorithms);
> +static DEVICE_ATTR_RO(qfrs_size);
> +
> +static struct attribute *uacce_dev_attrs[] = {
> +	&dev_attr_id.attr,
> +	&dev_attr_api.attr,
> +	&dev_attr_node_id.attr,
> +	&dev_attr_numa_distance.attr,
> +	&dev_attr_flags.attr,
> +	&dev_attr_available_instances.attr,
> +	&dev_attr_algorithms.attr,
> +	&dev_attr_qfrs_size.attr,
> +	NULL,
> +};
> +ATTRIBUTE_GROUPS(uacce_dev);
> +
...

> +
> +/**
> + * uacce_unregister - unregisters a uacce
> + * @uacce: the accelerator to unregister
> + *
> + * Unregister an accelerator that wat previously successully registered with

wat -> was
successully -> successfully


> + * uacce_register().
> + */
> +void uacce_unregister(struct uacce_device *uacce)
> +{
> +	mutex_lock(&uacce_mutex);
> +
> +	if (uacce->flags & UACCE_DEV_SVA)
> +		iommu_dev_disable_feature(uacce->pdev, IOMMU_DEV_FEAT_SVA);
> +
> +	uacce_unset_iommu_domain(uacce);
> +	cdev_device_del(uacce->cdev, &uacce->dev);
> +	idr_remove(&uacce_idr, uacce->dev_id);
> +	put_device(&uacce->dev);
> +
> +	mutex_unlock(&uacce_mutex);
> +}
> +EXPORT_SYMBOL_GPL(uacce_unregister);
> +
...
> diff --git a/include/linux/uacce.h b/include/linux/uacce.h
> new file mode 100644
> index 0000000..9137f3d
> --- /dev/null
> +++ b/include/linux/uacce.h
> @@ -0,0 +1,167 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +#ifndef _LINUX_UACCE_H
> +#define _LINUX_UACCE_H
> +
> +#include <linux/cdev.h>
> +#include <uapi/misc/uacce/uacce.h>
> +
> +#define UACCE_NAME		"uacce"
> +
> +struct uacce_queue;
> +struct uacce_device;
> +
> +/* uacce queue file flag, requires different operation */
> +#define UACCE_QFRF_MAP		BIT(0)	/* map to current queue */
> +#define UACCE_QFRF_MMAP		BIT(1)	/* map to user space */
> +#define UACCE_QFRF_KMAP		BIT(2)	/* map to kernel space */
> +#define UACCE_QFRF_DMA		BIT(3)	/* use dma api for the region */
> +#define UACCE_QFRF_SELFMT	BIT(4)	/* self maintained qfr */
> +

...

> +/**
> + * struct uacce_queue
> + * @uacce: pointer to uacce
> + * @priv: private pointer
> + * @wait: wait queue head
> + * @pasid: pasid of the queue
> + * @handle: iommu_sva handle return from iommu_sva_bind_device
> + * @list: share list for qfr->qs
> + * @mm: current->mm
> + * @qfrs: pointer of qfr regions

Missing state.  Make sure to run
./scripts/kernel-doc FILENAME > /dev/null and
fix any errors that show up.

> + */
> +struct uacce_queue {
> +	struct uacce_device *uacce;
> +	void *priv;
> +	wait_queue_head_t wait;
> +	int pasid;
> +	struct iommu_sva *handle;
> +	struct list_head list;
> +	struct mm_struct *mm;
> +	struct uacce_qfile_region *qfrs[UACCE_QFRT_MAX];
> +	enum uacce_q_state state;
> +};
...

Thanks,

Jonathan