[PATCH 0/2] A General Accelerator Framework, WarpDrive

[PATCH 0/2] A General Accelerator Framework, WarpDrive

[Zhangfei Gao]

*WarpDrive* is a general accelerator framework for the user application to
access the hardware without going through the kernel in data path.

WarpDrive is the name for the whole framework. The component in kernel
is called uacce, meaning "Unified/User-space-access-intended Accelerator
Framework". It makes use of the capability of IOMMU to maintain a
unified virtual address space between the hardware and the process.

WarpDrive is intended to be used with Jean Philippe Brucker's SVA
patchset[1], which enables IO side page fault and PASID support. 
We have keep verifying with Jean's sva/current [2]
We also keep verifying with Eric's SMMUv3 Nested Stage patch [3]

This series and related zip & qm driver as well as dummy driver for qemu test:
https://github.com/Linaro/linux-kernel-warpdrive/tree/5.3-rc1-warpdrive-v1
zip driver already been upstreamed.
zip supporting uacce will be the next step.

The library and user application:
https://github.com/Linaro/warpdrive/tree/wdprd-v1-current

Change History:
v4 changed from V3
1. Rebase to 5.3-rc1
2. Build on iommu interface
3. Verifying with Jean's sva and Eric's nested mode iommu.
4. User library has developed a lot: support zlib, openssl etc.
5. Move to misc first

V3 changed from V2:
https://lkml.org/lkml/2018/11/12/1951
1. Build uacce from original IOMMU interface. V2 is built on VFIO.
   But the VFIO way locking the user memory in place will not
   work properly if the process fork a child. Because the
   copy-on-write strategy will make the parent process lost its
   page. This is not acceptable to accelerator user.
2. The kernel component is renamed to uacce from sdmdev accordingly
3. Document is updated for the new design. The Static Shared
   Virtual Memory concept is introduced to replace the User
	Memory Sharing concept.
4. Rebase to the lastest kernel (4.20.0-rc1)
5. As an RFC, this version is tested only with "test-to-pass"
   test case and not tested with Jean's SVA patch.

V2 changed from V1:
https://lwn.net/Articles/763990/
1. Change kernel framework name from SPIMDEV (Share Parent IOMMU
   Mdev) to SDMDEV (Share Domain Mdev).
2. Allocate Hardware Resource when a new mdev is created (While
   it is allocated when the mdev is openned)
3. Unmap pages from the shared domain when the sdmdev iommu group is
   detached. (This procedure is necessary, but missed in V1)
4. Update document accordingly.
5. Rebase to the latest kernel (4.19.0-rc1)

References:
[1] http://jpbrucker.net/sva/
[2] http://www.linux-arm.org/git?p=linux-jpb.git;a=shortlog;h=refs/heads/sva/current
[3] https://github.com/eauger/linux/tree/v5.3.0-rc0-2stage-v9

Kenneth Lee (2):
  uacce: Add documents for WarpDrive/uacce
  uacce: add uacce module

 Documentation/misc-devices/warpdrive.rst |  351 +++++++++
 drivers/misc/Kconfig                     |    1 +
 drivers/misc/Makefile                    |    1 +
 drivers/misc/uacce/Kconfig               |   13 +
 drivers/misc/uacce/Makefile              |    2 +
 drivers/misc/uacce/uacce.c               | 1186 ++++++++++++++++++++++++++++++
 include/linux/uacce.h                    |  109 +++
 include/uapi/misc/uacce.h                |   44 ++
 8 files changed, 1707 insertions(+)
 create mode 100644 Documentation/misc-devices/warpdrive.rst
 create mode 100644 drivers/misc/uacce/Kconfig
 create mode 100644 drivers/misc/uacce/Makefile
 create mode 100644 drivers/misc/uacce/uacce.c
 create mode 100644 include/linux/uacce.h
 create mode 100644 include/uapi/misc/uacce.h

-- 
2.7.4

[PATCH 1/2] uacce: Add documents for WarpDrive/uacce

[Zhangfei Gao]

From: Kenneth Lee <liguozhu@hisilicon.com>

WarpDrive is a general accelerator framework for the user application to
access the hardware without going through the kernel in data path.

The kernel component to provide kernel facility to driver for expose the
user interface is called uacce. It a short name for
"Unified/User-space-access-intended Accelerator Framework".

This patch add document to explain how it works.

Signed-off-by: Kenneth Lee <liguozhu@hisilicon.com>
Signed-off-by: Zaibo Xu <xuzaibo@huawei.com>
Signed-off-by: Zhou Wang <wangzhou1@hisilicon.com>
Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
---
 Documentation/misc-devices/warpdrive.rst | 351 +++++++++++++++++++++++++++++++
 1 file changed, 351 insertions(+)
 create mode 100644 Documentation/misc-devices/warpdrive.rst

diff --git a/Documentation/misc-devices/warpdrive.rst b/Documentation/misc-devices/warpdrive.rst
new file mode 100644
index 0000000..14e5939
--- /dev/null
+++ b/Documentation/misc-devices/warpdrive.rst
@@ -0,0 +1,351 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+Introduction of WarpDrive
+=========================
+
+*WarpDrive* is a general accelerator framework for the user application to
+communicate with the hardware without going through the kernel in data path.
+
+It can be used as a quick channel for accelerators, network adaptors or
+other hardware for application in user space.
+
+It may also make some exist solution simpler.  E.g.  you can reuse most of the
+*netdev* driver in kernel and just share some ring buffer to the user space
+driver for *DPDK* or *ODP*. Or you can combine the RSA accelerator
+with the *netdev* in the user space as a https reverse proxy, etc.
+
+*WarpDrive* takes the hardware accelerator as a heterogeneous processor which
+can share particular load from the CPU:
+
+	 __________________________       __________________________
+	|                          |     |                          |
+	|  User application (CPU)  |     |   Hardware Accelerator   |
+	|__________________________|     |__________________________|
+
+	             |                                 |
+	             |                                 |
+	             V                                 V
+                 __________                        __________
+                |          |                      |          |
+                |   MMU    |                      |  IOMMU   |
+                |__________|                      |__________|
+		      \                               /
+	               \                             /
+	                \                           /
+			 __________________________
+			|                          |
+			|          Memory          |
+			|__________________________|
+
+
+
+Architecture
+------------
+
+*WarpDrive* includes general user libraries, kernel management modules
+and drivers for the hardware. In kernel, the management module
+is called *uacce*, meaning "Unified/User-space-access-intended
+Accelerator Framework".
+
+A virtual concept, queue, is used for the communication. It provides a
+FIFO-like interface. And it maintains a unified address space between the
+application and all involved hardware.
+
+                             ___________________                  ________________
+                            |                   |   user API     |                |
+                            | WarpDrive library | ------------>  |  user driver   |
+                            |___________________|                |________________|
+                                     |                                    |
+                                     |                                    |
+                                     | queue fd                           |
+                                     |                                    |
+                                     |                                    |
+                                     v                                    |
+     ___________________         _________                                |
+    |                   |       |         |                               | mmap memory
+    | Other framework   |       |  uacce  |                               | r/w interface
+    | crypto/nic/others |       |_________|                               |
+    |___________________|                                                 |
+             |                       |                                    |
+             | register              | register                           |
+             |                       |                                    |
+             |                       |                                    |
+             |                _________________       __________          |
+             |               |                 |     |          |         |
+              -------------  |  Device Driver  |     |  IOMMU   |         |
+                             |_________________|     |__________|         |
+                                     |                                    |
+                                     |                                    V
+                                     |                            ___________________
+                                     |                           |                   |
+                                     --------------------------  |  Device(Hardware) |
+                                                                 |___________________|
+
+The accelerator device present itself as a "uacce" object, which exports as
+chrdev to the user space. The user application communicates with the
+hardware by ioctl (as control path) or share memory (as data path).
+
+
+How does it work
+================
+
+*WarpDrive* uses *mmap* and *IOMMU* to play the trick.
+
+*Uacce* create a chrdev for every device registered to it. New queue is
+created when user application open the chrdev. The file descriptor is used as
+the user handle of the queue.
+
+The control path to the hardware is via file operation, while data path is via
+mmap space of the queue fd.
+
+The queue file address space:
+
+enum uacce_qfrt {
+	UACCE_QFRT_MMIO = 0,	/* device mmio region */
+	UACCE_QFRT_DKO,		/* device kernel-only region */
+	UACCE_QFRT_DUS,		/* device user share region */
+	UACCE_QFRT_SS,		/* static shared memory (for non-sva devices) */
+	UACCE_QFRT_MAX,
+};
+
+All regions are optional and differ from device type to type. The
+communication protocol is wrapped by the user driver.
+
+The device mmio region is mapped to the hardware mmio space. It is generally
+used for doorbell or other notification to the hardware. It is not fast enough
+as data channel.
+
+The device kernel-only region is necessary only if the device IOMMU has no
+PASID support or it cannot send kernel-only address request. In this case, if
+kernel need to share memory with the device, kernel has to share iova address
+space with the user process via mmap, to prevent iova conflict.
+
+The device user share region is used for share data buffer between user process
+and device. It can be merged into other regions. But a separated region can help
+on device state management. For example, the device can be started when this
+region is mapped.
+
+The static share virtual memory region is used for share data buffer with the
+device and can be shared among queues / devices.
+Its size is set according to the application requirement.
+
+
+The user API
+------------
+
+We adopt a polling style interface in the user space: ::
+
+        int wd_request_queue(struct wd_queue *q);
+        void wd_release_queue(struct wd_queue *q);
+        int wd_send(struct wd_queue *q, void *req);
+        int wd_recv(struct wd_queue *q, void **req);
+        int wd_recv_sync(struct wd_queue *q, void **req);
+        void wd_flush(struct wd_queue *q);
+
+wd_recv_sync() is a wrapper to its non-sync version. It will trap into
+kernel and wait until the queue become available.
+
+If the queue do not support SVA/SVM. The following helper functions
+can be used to create Static Virtual Share Memory: ::
+
+        void *wd_reserve_memory(struct wd_queue *q, size_t size);
+	int wd_share_reserved_memory(struct wd_queue *q,
+				     struct wd_queue *target_q);
+
+The user API is not mandatory. It is simply a suggestion and hint what the
+kernel interface is supposed to be.
+
+
+The user driver
+---------------
+
+The queue file mmap space will need a user driver to wrap the communication
+protocol. *UACCE* provides some attributes in sysfs for the user driver to
+match the right accelerator accordingly.
+
+The *UACCE* device attribute is under the following directory:
+
+/sys/class/uacce/<dev-name>/attrs
+
+The following attributes are supported:
+
+id (ro)
+        N. Id of the device. The chrdev of this uacce is /dev/uaN
+api (ro)
+	api of the device, match with driver
+flags (ro)
+        Attributes of the device, see UACCE_DEV_xxx flag defined in uacce.h
+available_instances (ro)
+        available instances left of the device
+algorithms (ro)
+        algorithms supported by this accelerator
+qfrs_offset (ro)
+       qfrs_offset of the device
+numa_distance (ro)
+	distance of device node to cpu node
+node_id (ro)
+	id of the numa node
+
+
+The uacce register API
+-----------------------
+The *uacce* register API is defined in uacce.h. If the hardware support SVM/SVA,
+The driver need only the following API functions: ::
+
+        int uacce_register(uacce);
+        void uacce_unregister(uacce);
+        void uacce_wake_up(q);
+
+*uacce_wake_up* is used to notify the process who epoll() on the queue file.
+
+According to the IOMMU capability, *uacce* categories the devices as below:
+
+UACCE_DEV_NOIOMMU
+        The device has no IOMMU. The user process cannot use VA on the hardware
+        This mode is not recommended.
+
+UACCE_DEV_SVA (UACCE_DEV_PASID | UACCE_DEV_FAULT_FROM_DEV)
+        The device has IOMMU which can share the same page table with user
+        process
+
+UACCE_DEV_SHARE_DOMAIN
+        This is used for device which need QFR_KO.
+
+
+The Memory Sharing Model
+------------------------
+The perfect form of a uacce device is to support SVM/SVA. We built this upon
+Jean Philippe Brucker's SVA patches. [1]
+
+If the hardware support SVA, the user process's page table is shared to the
+opened queue. So the device can access any address in the process address
+space. And it can raise a page fault if the physical page is not available
+yet. It can also access the address in the kernel space, which is referred by
+another page table particular to the kernel. Most of IOMMU implementation can
+handle this by a tag on the address request of the device. For example, ARM
+SMMU uses SSV bit to indicate that the address request is for kernel or user
+space.
+
+The device_attr UACCE_DEV_SVA is used to indicate this capability of the
+device. It is a combination of UACCE_DEV_FAULT_FROM_DEV and UACCE_DEV_PASID.
+
+If the device does not support UACCE_DEV_FAULT_FROM_DEV but UACCE_DEV_PASID.
+*Uacce* will create an unmanaged iommu_domain for the device. So it can be
+bound to multiple processes. In this case, the device cannot share the user
+page table directly. The user process must map the Static Share Queue File
+Region to create the connection. The *Uacce* kernel module will allocate
+physical memory to the region for both the device and the user process.
+
+If the device does not support UACCE_DEV_PASID either. There is no way for
+*uacce* to support multiple process. Every *Uacce* allow only one process at
+the same time. In this case, DMA API cannot be used in this device. If the
+device driver need to share memory with the device, it should use QFRT_KO
+queue file region instead. This region is mmaped from the user space but valid
+only for kernel.
+
+The device can also be declared as UACCE_DEV_NOIOMMU. It can be used when the
+device has no iommu support or the iommu is set in pass through mode.  In this
+case, the driver should map address to device by itself with DMA API. The
+ioctl(UACCE_CMD_GET_SS_DMA) can be used to get the physical address, though it
+is an untrusted and kernel-tainted behavior.
+
+We suggest the driver use uacce_mode module parameter to choose the working
+mode of the device. It can be:
+
+UACCE_MODE_NOUACCE (0)
+        Do not register to uacce. In this mode, the driver can register to
+        other kernel framework, such as crypto
+
+UACCE_MODE_UACCE (1)
+        Register to uacce. In this mode, the driver register to uacce. It can
+        register to other kernel framework according to whether it supports
+        PASID.
+
+UACCE_MODE_NOIOMMU
+        Register to uacce and assume there is no IOMMU or IOMMU in
+        pass-through mode. In this case, DMA API is available, so it can also
+        register to other kernel framework.
+
+        In this case, mmap operations except for QRFT_SS will be passed
+        through to the uacce->ops->mmap() call back.
+
+
+
+The Folk Scenario
+=================
+For a process with allocated queues and shared memory, what happen if it forks
+a child?
+
+The fd of the queue will be duplicated on folk, so the child can send request
+to the same queue as its parent. But the requests which is sent from processes
+except for the one who opens the queue will be blocked.
+
+It is recommended to add O_CLOEXEC to the queue file.
+
+The queue mmap space has a VM_DONTCOPY in its VMA. So the child will lose all
+those VMAs.
+
+This is a reason why *WarpDrive* does not adopt the mode used in *VFIO* and
+*InfiniBand*.  Both solutions can set any user pointer for hardware sharing.
+But they cannot support fork when the dma is in process. Or the
+"Copy-On-Write" procedure will make the parent process lost its physical
+pages.
+
+
+Difference to the VFIO and IB framework
+---------------------------------------
+The essential function of WarpDrive is to let the device access the user
+address directly. There are many device drivers doing the same in the kernel.
+And both VFIO and IB can provide similar function in framework level.
+
+But WarpDrive has a different goal: "share address space". It is
+not taken the request to the accelerator as an enclosure data structure. It
+takes the accelerator as another thread of the same process. So the
+accelerator can refer to any address used by the process.
+
+Both VFIO and IB are taken this as "memory sharing", not "address sharing".
+They care more on sharing the block of memory. But if there is an address
+stored in the block and referring to another memory region. The address may
+not be valid.
+
+By adding more constraints to the VFIO and IB framework, in some sense, we may
+achieve a similar goal. But we gave it up finally. Both VFIO and IB have extra
+assumption which is unnecessary to WarpDrive. They may hurt each other if we
+try to merge them together.
+
+VFIO manages resource of a hardware as a "virtual device". If a device need to
+serve a separated application. It must isolate the resource as separate
+virtual device.  And the life cycle of the application and virtual device are
+unnecessary unrelated. And most concepts, such as bus, driver, probe and
+so on, to make it as a "device" is unnecessary either. And the logic added to
+VFIO to make address sharing do no help on "creating a virtual device".
+
+IB creates a "verbs" standard for sharing memory region to another remote
+entity.  Most of these verbs are to make memory region between entities to be
+synchronized.  This is not what accelerator need. Accelerator is in the same
+memory system with the CPU. It refers to the same memory system among CPU and
+devices. So the local memory terms/verbs are good enough for it. Extra "verbs"
+are not necessary. And its queue (like queue pair in IB) is the communication
+channel direct to the accelerator hardware. There is nothing about memory
+itself.
+
+Further, both VFIO and IB use the "pin" (get_user_page) way to lock local
+memory in place.  This is flexible. But it can cause other problems. For
+example, if the user process fork a child process. The COW procedure may make
+the parent process lost its pages which are sharing with the device. These may
+be fixed in the future. But is not going to be easy. (There is a discussion
+about this on Linux Plumbers Conference 2018 [2])
+
+So we choose to build the solution directly on top of IOMMU interface. IOMMU
+is the essential way for device and process to share their page mapping from
+the hardware perspective. It will be safe to create a software solution on
+this assumption.  Uacce manages the IOMMU interface for the accelerator
+device, so the device driver can export some of the resources to the user
+space. Uacce than can make sure the device and the process have the same
+address space.
+
+
+References
+==========
+.. [1] http://jpbrucker.net/sva/
+.. [2] https://lwn.net/Articles/774411/
-- 
2.7.4

[PATCH 2/2] uacce: add uacce module

[Zhangfei Gao]

From: Kenneth Lee <liguozhu@hisilicon.com>

Uacce is the kernel component to support WarpDrive accelerator
framework. It provides register/unregister interface for device drivers
to expose their hardware resource to the user space. The resource is
taken as "queue" in WarpDrive.

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.

Uacce also manages unify addresses between the hardware and user space
of the process. So they can share the same virtual address in the
communication.

Signed-off-by: Kenneth Lee <liguozhu@hisilicon.com>
Signed-off-by: Zaibo Xu <xuzaibo@huawei.com>
Signed-off-by: Zhou Wang <wangzhou1@hisilicon.com>
Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
---
 drivers/misc/Kconfig        |    1 +
 drivers/misc/Makefile       |    1 +
 drivers/misc/uacce/Kconfig  |   13 +
 drivers/misc/uacce/Makefile |    2 +
 drivers/misc/uacce/uacce.c  | 1186 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/uacce.h       |  109 ++++
 include/uapi/misc/uacce.h   |   44 ++
 7 files changed, 1356 insertions(+)
 create mode 100644 drivers/misc/uacce/Kconfig
 create mode 100644 drivers/misc/uacce/Makefile
 create mode 100644 drivers/misc/uacce/uacce.c
 create mode 100644 include/linux/uacce.h
 create mode 100644 include/uapi/misc/uacce.h

diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index 6abfc8e..8073eb8 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -502,4 +502,5 @@ source "drivers/misc/cxl/Kconfig"
 source "drivers/misc/ocxl/Kconfig"
 source "drivers/misc/cardreader/Kconfig"
 source "drivers/misc/habanalabs/Kconfig"
+source "drivers/misc/uacce/Kconfig"
 endmenu
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index abd8ae2..93a131b 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -58,4 +58,5 @@ obj-$(CONFIG_OCXL)		+= ocxl/
 obj-y				+= cardreader/
 obj-$(CONFIG_PVPANIC)   	+= pvpanic.o
 obj-$(CONFIG_HABANA_AI)		+= habanalabs/
+obj-$(CONFIG_UACCE)		+= uacce/
 obj-$(CONFIG_XILINX_SDFEC)	+= xilinx_sdfec.o
diff --git a/drivers/misc/uacce/Kconfig b/drivers/misc/uacce/Kconfig
new file mode 100644
index 0000000..569669c
--- /dev/null
+++ b/drivers/misc/uacce/Kconfig
@@ -0,0 +1,13 @@
+config UACCE
+	tristate "Accelerator Framework for User Land"
+	depends on IOMMU_API
+	help
+	  UACCE provides interface for the user process to access the hardware
+	  without interaction with the kernel space in data path.
+
+	  The user-space interface is described in
+	  include/uapi/misc/uacce.h
+
+	  See Documentation/misc-devices/warpdrive.rst for more details.
+
+	  If you don't know what to do here, say N.
diff --git a/drivers/misc/uacce/Makefile b/drivers/misc/uacce/Makefile
new file mode 100644
index 0000000..5b4374e
--- /dev/null
+++ b/drivers/misc/uacce/Makefile
@@ -0,0 +1,2 @@
+# SPDX-License-Identifier: GPL-2.0-or-later
+obj-$(CONFIG_UACCE) += uacce.o
diff --git a/drivers/misc/uacce/uacce.c b/drivers/misc/uacce/uacce.c
new file mode 100644
index 0000000..43e0c9b
--- /dev/null
+++ b/drivers/misc/uacce/uacce.c
@@ -0,0 +1,1186 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+#include <linux/compat.h>
+#include <linux/dma-iommu.h>
+#include <linux/dma-mapping.h>
+#include <linux/file.h>
+#include <linux/idr.h>
+#include <linux/irqdomain.h>
+#include <linux/module.h>
+#include <linux/poll.h>
+#include <linux/sched/signal.h>
+#include <linux/uacce.h>
+
+static struct class *uacce_class;
+static DEFINE_IDR(uacce_idr);
+static dev_t uacce_devt;
+static DEFINE_MUTEX(uacce_mutex); /* mutex to protect uacce */
+
+/* lock to protect all queues management */
+static DECLARE_RWSEM(uacce_qs_lock);
+#define uacce_qs_rlock() down_read(&uacce_qs_lock)
+#define uacce_qs_runlock() up_read(&uacce_qs_lock)
+#define uacce_qs_wlock() down_write(&uacce_qs_lock)
+#define uacce_qs_wunlock() up_write(&uacce_qs_lock)
+
+static const struct file_operations uacce_fops;
+
+/* match with enum uacce_qfrt */
+static const char *const qfrt_str[] = {
+	"mmio",
+	"dko",
+	"dus",
+	"ss",
+	"invalid"
+};
+
+static const char *uacce_qfrt_str(struct uacce_qfile_region *qfr)
+{
+	enum uacce_qfrt type = qfr->type;
+
+	if (type > UACCE_QFRT_INVALID)
+		type = UACCE_QFRT_INVALID;
+
+	return qfrt_str[type];
+}
+
+/**
+ * uacce_wake_up - Wake up the process who is waiting this queue
+ * @q the accelerator queue to wake up
+ */
+void uacce_wake_up(struct uacce_queue *q)
+{
+	dev_dbg(&q->uacce->dev, "wake up\n");
+	wake_up_interruptible(&q->wait);
+}
+EXPORT_SYMBOL_GPL(uacce_wake_up);
+
+static int uacce_queue_map_qfr(struct uacce_queue *q,
+			       struct uacce_qfile_region *qfr)
+{
+	struct device *dev = q->uacce->pdev;
+	struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
+	int i, j, ret;
+
+	if (!(qfr->flags & UACCE_QFRF_MAP) || (qfr->flags & UACCE_QFRF_DMA))
+		return 0;
+
+	dev_dbg(dev, "queue map %s qfr(npage=%d, iova=%lx)\n",
+		uacce_qfrt_str(qfr), qfr->nr_pages, qfr->iova);
+
+	if (!domain)
+		return -ENODEV;
+
+	for (i = 0; i < qfr->nr_pages; i++) {
+		ret = iommu_map(domain, qfr->iova + i * PAGE_SIZE,
+				page_to_phys(qfr->pages[i]),
+				PAGE_SIZE, qfr->prot | q->uacce->prot);
+		if (ret) {
+			dev_err(dev, "iommu_map page %i fail %d\n", i, ret);
+			goto err_with_map_pages;
+		}
+		get_page(qfr->pages[i]);
+	}
+
+	return 0;
+
+err_with_map_pages:
+	for (j = i - 1; j >= 0; j--) {
+		iommu_unmap(domain, qfr->iova + j * PAGE_SIZE, PAGE_SIZE);
+		put_page(qfr->pages[j]);
+	}
+	return ret;
+}
+
+static void uacce_queue_unmap_qfr(struct uacce_queue *q,
+				  struct uacce_qfile_region *qfr)
+{
+	struct device *dev = q->uacce->pdev;
+	struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
+	int i;
+
+	if (!(qfr->flags & UACCE_QFRF_MAP) || (qfr->flags & UACCE_QFRF_DMA))
+		return;
+
+	dev_dbg(dev, "queue map %s qfr(npage=%d, iova=%lx)\n",
+		uacce_qfrt_str(qfr), qfr->nr_pages, qfr->iova);
+
+	if (!domain || !qfr)
+		return;
+
+	for (i = qfr->nr_pages - 1; i >= 0; i--) {
+		iommu_unmap(domain, qfr->iova + i * PAGE_SIZE, PAGE_SIZE);
+		put_page(qfr->pages[i]);
+	}
+}
+
+static int uacce_qfr_alloc_pages(struct uacce_qfile_region *qfr)
+{
+	int gfp_mask = GFP_ATOMIC | __GFP_ZERO;
+	int i, j;
+
+	qfr->pages = kcalloc(qfr->nr_pages, sizeof(*qfr->pages), gfp_mask);
+	if (!qfr->pages)
+		return -ENOMEM;
+
+	for (i = 0; i < qfr->nr_pages; i++) {
+		qfr->pages[i] = alloc_page(gfp_mask);
+		if (!qfr->pages[i])
+			goto err_with_pages;
+	}
+
+	return 0;
+
+err_with_pages:
+	for (j = i - 1; j >= 0; j--)
+		put_page(qfr->pages[j]);
+
+	kfree(qfr->pages);
+	return -ENOMEM;
+}
+
+static void uacce_qfr_free_pages(struct uacce_qfile_region *qfr)
+{
+	int i;
+
+	for (i = 0; i < qfr->nr_pages; i++)
+		put_page(qfr->pages[i]);
+
+	kfree(qfr->pages);
+}
+
+static inline int uacce_queue_mmap_qfr(struct uacce_queue *q,
+				       struct uacce_qfile_region *qfr,
+				       struct vm_area_struct *vma)
+{
+	int i, ret;
+
+	if (qfr->nr_pages)
+		dev_dbg(q->uacce->pdev, "mmap qfr (page ref=%d)\n",
+			page_ref_count(qfr->pages[0]));
+	for (i = 0; i < qfr->nr_pages; i++) {
+		ret = remap_pfn_range(vma, vma->vm_start + (i << PAGE_SHIFT),
+				      page_to_pfn(qfr->pages[i]), PAGE_SIZE,
+				      vma->vm_page_prot);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static struct uacce_qfile_region *
+uacce_create_region(struct uacce_queue *q, struct vm_area_struct *vma,
+		    enum uacce_qfrt type, unsigned int flags)
+{
+	struct uacce_qfile_region *qfr;
+	struct uacce *uacce = q->uacce;
+	unsigned long vm_pgoff;
+	int ret = -ENOMEM;
+
+	dev_dbg(uacce->pdev, "create qfr (type=%x, flags=%x)\n", type, flags);
+	qfr = kzalloc(sizeof(*qfr), GFP_ATOMIC);
+	if (!qfr)
+		return ERR_PTR(-ENOMEM);
+
+	qfr->type = type;
+	qfr->flags = flags;
+	qfr->iova = vma->vm_start;
+	qfr->nr_pages = vma_pages(vma);
+
+	if (vma->vm_flags & VM_READ)
+		qfr->prot |= IOMMU_READ;
+
+	if (vma->vm_flags & VM_WRITE)
+		qfr->prot |= IOMMU_WRITE;
+
+	if (flags & UACCE_QFRF_SELFMT) {
+		ret = uacce->ops->mmap(q, vma, qfr);
+		if (ret)
+			goto err_with_qfr;
+		return qfr;
+	}
+
+	/* allocate memory */
+	if (flags & UACCE_QFRF_DMA) {
+		dev_dbg(uacce->pdev, "allocate dma %d pages\n", qfr->nr_pages);
+		qfr->kaddr = dma_alloc_coherent(uacce->pdev, qfr->nr_pages <<
+						PAGE_SHIFT, &qfr->dma,
+						GFP_KERNEL);
+		if (!qfr->kaddr) {
+			ret = -ENOMEM;
+			goto err_with_qfr;
+		}
+	} else {
+		dev_dbg(uacce->pdev, "allocate %d pages\n", qfr->nr_pages);
+		ret = uacce_qfr_alloc_pages(qfr);
+		if (ret)
+			goto err_with_qfr;
+	}
+
+	/* map to device */
+	ret = uacce_queue_map_qfr(q, qfr);
+	if (ret)
+		goto err_with_pages;
+
+	/* mmap to user space */
+	if (flags & UACCE_QFRF_MMAP) {
+		if (flags & UACCE_QFRF_DMA) {
+
+			/* dma_mmap_coherent() requires vm_pgoff as 0
+			 * restore vm_pfoff to initial value for mmap()
+			 */
+			dev_dbg(uacce->pdev, "mmap dma qfr\n");
+			vm_pgoff = vma->vm_pgoff;
+			vma->vm_pgoff = 0;
+			ret = dma_mmap_coherent(uacce->pdev, vma, qfr->kaddr,
+						qfr->dma,
+						qfr->nr_pages << PAGE_SHIFT);
+			vma->vm_pgoff = vm_pgoff;
+		} else {
+			ret = uacce_queue_mmap_qfr(q, qfr, vma);
+		}
+
+		if (ret)
+			goto err_with_mapped_qfr;
+	}
+
+	return qfr;
+
+err_with_mapped_qfr:
+	uacce_queue_unmap_qfr(q, qfr);
+err_with_pages:
+	if (flags & UACCE_QFRF_DMA)
+		dma_free_coherent(uacce->pdev, qfr->nr_pages << PAGE_SHIFT,
+				  qfr->kaddr, qfr->dma);
+	else
+		uacce_qfr_free_pages(qfr);
+err_with_qfr:
+	kfree(qfr);
+
+	return ERR_PTR(ret);
+}
+
+/* we assume you have uacce_queue_unmap_qfr(q, qfr) from all related queues */
+static void uacce_destroy_region(struct uacce_queue *q,
+				 struct uacce_qfile_region *qfr)
+{
+	struct uacce *uacce = q->uacce;
+
+	if (qfr->flags & UACCE_QFRF_DMA) {
+		dev_dbg(uacce->pdev, "free dma qfr %s (kaddr=%lx, dma=%llx)\n",
+			uacce_qfrt_str(qfr), (unsigned long)qfr->kaddr,
+			qfr->dma);
+		dma_free_coherent(uacce->pdev, qfr->nr_pages << PAGE_SHIFT,
+				  qfr->kaddr, qfr->dma);
+	} else if (qfr->pages) {
+		if (qfr->flags & UACCE_QFRF_KMAP && qfr->kaddr) {
+			dev_dbg(uacce->pdev, "vunmap qfr %s\n",
+				uacce_qfrt_str(qfr));
+			vunmap(qfr->kaddr);
+			qfr->kaddr = NULL;
+		}
+
+		uacce_qfr_free_pages(qfr);
+	}
+	kfree(qfr);
+}
+
+static long uacce_cmd_share_qfr(struct uacce_queue *tgt, int fd)
+{
+	struct file *filep = fget(fd);
+	struct uacce_queue *src;
+	int ret = -EINVAL;
+
+	if (!filep)
+		return ret;
+
+	if (filep->f_op != &uacce_fops)
+		goto out_with_fd;
+
+	src = filep->private_data;
+	if (!src)
+		goto out_with_fd;
+
+	/* no share sva is needed if the dev can do fault-from-dev */
+	if (tgt->uacce->flags & UACCE_DEV_FAULT_FROM_DEV)
+		goto out_with_fd;
+
+	dev_dbg(&src->uacce->dev, "share ss with %s\n",
+		dev_name(&tgt->uacce->dev));
+
+	uacce_qs_wlock();
+	if (!src->qfrs[UACCE_QFRT_SS] || tgt->qfrs[UACCE_QFRT_SS])
+		goto out_with_lock;
+
+	ret = uacce_queue_map_qfr(tgt, src->qfrs[UACCE_QFRT_SS]);
+	if (ret)
+		goto out_with_lock;
+
+	tgt->qfrs[UACCE_QFRT_SS] = src->qfrs[UACCE_QFRT_SS];
+	list_add(&tgt->list, &src->qfrs[UACCE_QFRT_SS]->qs);
+	ret = 0;
+
+out_with_lock:
+	uacce_qs_wunlock();
+out_with_fd:
+	fput(filep);
+	return ret;
+}
+
+static int uacce_start_queue(struct uacce_queue *q)
+{
+	int ret, i, j;
+	struct uacce_qfile_region *qfr;
+	struct device *dev = &q->uacce->dev;
+
+	/*
+	 * map KMAP qfr to kernel
+	 * vmap should be done in non-spinlocked context!
+	 */
+	for (i = 0; i < UACCE_QFRT_MAX; i++) {
+		qfr = q->qfrs[i];
+		if (qfr && (qfr->flags & UACCE_QFRF_KMAP) && !qfr->kaddr) {
+			qfr->kaddr = vmap(qfr->pages, qfr->nr_pages, VM_MAP,
+					  PAGE_KERNEL);
+			if (!qfr->kaddr) {
+				ret = -ENOMEM;
+				dev_dbg(dev, "fail to kmap %s qfr(%d pages)\n",
+					uacce_qfrt_str(qfr), qfr->nr_pages);
+				goto err_with_vmap;
+			}
+
+			dev_dbg(dev, "kernel vmap %s qfr(%d pages) to %lx\n",
+				uacce_qfrt_str(qfr), qfr->nr_pages,
+				(unsigned long)qfr->kaddr);
+		}
+	}
+
+	ret = q->uacce->ops->start_queue(q);
+	if (ret < 0)
+		goto err_with_vmap;
+
+	dev_dbg(&q->uacce->dev, "uacce state switch to STARTED\n");
+	atomic_set(&q->uacce->state, UACCE_ST_STARTED);
+	return 0;
+
+err_with_vmap:
+	for (j = i; j >= 0; j--) {
+		qfr = q->qfrs[j];
+		if (qfr && qfr->kaddr) {
+			vunmap(qfr->kaddr);
+			qfr->kaddr = NULL;
+		}
+	}
+	return ret;
+}
+
+static long uacce_get_ss_dma(struct uacce_queue *q, void __user *arg)
+{
+	struct uacce *uacce = q->uacce;
+	long ret = 0;
+	unsigned long dma = 0;
+
+	if (!(uacce->flags & UACCE_DEV_NOIOMMU))
+		return -EINVAL;
+
+	uacce_qs_wlock();
+	if (q->qfrs[UACCE_QFRT_SS]) {
+		dma = (unsigned long)(q->qfrs[UACCE_QFRT_SS]->dma);
+		dev_dbg(&uacce->dev, "%s (%lx)\n", __func__, dma);
+	} else {
+		ret = -EINVAL;
+	}
+	uacce_qs_wunlock();
+
+	if (copy_to_user(arg, &dma, sizeof(dma)))
+		ret = -EFAULT;
+
+	return ret;
+}
+
+static long uacce_fops_unl_ioctl(struct file *filep,
+				 unsigned int cmd, unsigned long arg)
+{
+	struct uacce_queue *q = filep->private_data;
+	struct uacce *uacce = q->uacce;
+
+	switch (cmd) {
+	case UACCE_CMD_SHARE_SVAS:
+		return uacce_cmd_share_qfr(q, arg);
+
+	case UACCE_CMD_START:
+		return uacce_start_queue(q);
+
+	case UACCE_CMD_GET_SS_DMA:
+		return uacce_get_ss_dma(q, (void __user *)arg);
+
+	default:
+		if (uacce->ops->ioctl)
+			return uacce->ops->ioctl(q, cmd, arg);
+
+		dev_err(&uacce->dev, "ioctl cmd (%d) is not supported!\n", cmd);
+		return -EINVAL;
+	}
+}
+
+#ifdef CONFIG_COMPAT
+static long uacce_fops_compat_ioctl(struct file *filep,
+				   unsigned int cmd, unsigned long arg)
+{
+	arg = (unsigned long)compat_ptr(arg);
+	return uacce_fops_unl_ioctl(filep, cmd, arg);
+}
+#endif
+
+static int uacce_dev_open_check(struct uacce *uacce)
+{
+	if (uacce->flags & UACCE_DEV_NOIOMMU)
+		return 0;
+
+	/*
+	 * The device can be opened once if it dose not support multiple page
+	 * table. The better way to check this is counting it per iommu_domain,
+	 * this is just a temporary solution
+	 */
+	if (uacce->flags & (UACCE_DEV_PASID | UACCE_DEV_NOIOMMU))
+		return 0;
+
+	if (atomic_cmpxchg(&uacce->state, UACCE_ST_INIT, UACCE_ST_OPENED) !=
+	    UACCE_ST_INIT) {
+		dev_info(&uacce->dev, "this device can be openned only once\n");
+		return -EBUSY;
+	}
+
+	dev_dbg(&uacce->dev, "state switch to OPENNED");
+
+	return 0;
+}
+
+static int uacce_fops_open(struct inode *inode, struct file *filep)
+{
+	struct uacce_queue *q;
+	struct iommu_sva *handle = NULL;
+	struct uacce *uacce;
+	int ret;
+	int pasid = 0;
+
+	uacce = idr_find(&uacce_idr, iminor(inode));
+	if (!uacce)
+		return -ENODEV;
+
+	if (atomic_read(&uacce->state) == UACCE_ST_RST)
+		return -EINVAL;
+
+	if (!uacce->ops->get_queue)
+		return -EINVAL;
+
+	if (!try_module_get(uacce->pdev->driver->owner))
+		return -ENODEV;
+
+	ret = uacce_dev_open_check(uacce);
+	if (ret)
+		goto open_err;
+
+#ifdef CONFIG_IOMMU_SVA
+	if (uacce->flags & UACCE_DEV_PASID) {
+		handle = iommu_sva_bind_device(uacce->pdev, current->mm, NULL);
+		if (IS_ERR(handle))
+			goto open_err;
+		pasid = iommu_sva_get_pasid(handle);
+	}
+#endif
+	ret = uacce->ops->get_queue(uacce, pasid, &q);
+	if (ret < 0)
+		goto open_err;
+
+	q->pasid = pasid;
+	q->handle = handle;
+	q->uacce = uacce;
+	q->mm = current->mm;
+	memset(q->qfrs, 0, sizeof(q->qfrs));
+	INIT_LIST_HEAD(&q->list);
+	init_waitqueue_head(&q->wait);
+	filep->private_data = q;
+	mutex_lock(&uacce->q_lock);
+	list_add(&q->q_dev, &uacce->qs);
+	mutex_unlock(&uacce->q_lock);
+
+	return 0;
+open_err:
+	module_put(uacce->pdev->driver->owner);
+	return ret;
+}
+
+static int uacce_fops_release(struct inode *inode, struct file *filep)
+{
+	struct uacce_queue *q = (struct uacce_queue *)filep->private_data;
+	struct uacce_qfile_region *qfr;
+	struct uacce *uacce = q->uacce;
+	int i;
+	bool is_to_free_region;
+	int free_pages = 0;
+
+	mutex_lock(&uacce->q_lock);
+	list_del(&q->q_dev);
+	mutex_unlock(&uacce->q_lock);
+
+	if (atomic_read(&uacce->state) == UACCE_ST_STARTED &&
+	    uacce->ops->stop_queue)
+		uacce->ops->stop_queue(q);
+
+	uacce_qs_wlock();
+
+	for (i = 0; i < UACCE_QFRT_MAX; i++) {
+		qfr = q->qfrs[i];
+		if (!qfr)
+			continue;
+
+		is_to_free_region = false;
+		uacce_queue_unmap_qfr(q, qfr);
+		if (i == UACCE_QFRT_SS) {
+			list_del(&q->list);
+			if (list_empty(&qfr->qs))
+				is_to_free_region = true;
+		} else
+			is_to_free_region = true;
+
+		if (is_to_free_region) {
+			free_pages += qfr->nr_pages;
+			uacce_destroy_region(q, qfr);
+		}
+
+		qfr = NULL;
+	}
+
+	uacce_qs_wunlock();
+
+	if (current->mm == q->mm) {
+		down_write(&q->mm->mmap_sem);
+		q->mm->data_vm -= free_pages;
+		up_write(&q->mm->mmap_sem);
+	}
+
+#ifdef CONFIG_IOMMU_SVA
+	if (uacce->flags & UACCE_DEV_PASID)
+		iommu_sva_unbind_device(q->handle);
+#endif
+
+	if (uacce->ops->put_queue)
+		uacce->ops->put_queue(q);
+
+	dev_dbg(&uacce->dev, "uacce state switch to INIT\n");
+	atomic_set(&uacce->state, UACCE_ST_INIT);
+	module_put(uacce->pdev->driver->owner);
+	return 0;
+}
+
+static enum uacce_qfrt uacce_get_region_type(struct uacce *uacce,
+					     struct vm_area_struct *vma)
+{
+	enum uacce_qfrt type = UACCE_QFRT_MAX;
+	int i;
+	size_t next_start = UACCE_QFR_NA;
+
+	for (i = UACCE_QFRT_MAX - 1; i >= 0; i--) {
+		if (vma->vm_pgoff >= uacce->qf_pg_start[i]) {
+			type = i;
+			break;
+		}
+	}
+
+	switch (type) {
+	case UACCE_QFRT_MMIO:
+		if (!uacce->ops->mmap) {
+			dev_err(&uacce->dev, "no driver mmap!\n");
+			return UACCE_QFRT_INVALID;
+		}
+		break;
+
+	case UACCE_QFRT_DKO:
+		if ((uacce->flags & UACCE_DEV_PASID) ||
+		    (uacce->flags & UACCE_DEV_NOIOMMU))
+			return UACCE_QFRT_INVALID;
+		break;
+
+	case UACCE_QFRT_DUS:
+		break;
+
+	case UACCE_QFRT_SS:
+		/* todo: this can be valid to protect the process space */
+		if (uacce->flags & UACCE_DEV_FAULT_FROM_DEV)
+			return UACCE_QFRT_INVALID;
+		break;
+
+	default:
+		dev_err(&uacce->dev, "uacce bug (%d)!\n", type);
+		return UACCE_QFRT_INVALID;
+	}
+
+	/* make sure the mapping size is exactly the same as the region */
+	if (type < UACCE_QFRT_SS) {
+		for (i = type + 1; i < UACCE_QFRT_MAX; i++)
+			if (uacce->qf_pg_start[i] != UACCE_QFR_NA) {
+				next_start = uacce->qf_pg_start[i];
+				break;
+			}
+
+		if (next_start == UACCE_QFR_NA) {
+			dev_err(&uacce->dev, "uacce config error: SS offset set improperly\n");
+			return UACCE_QFRT_INVALID;
+		}
+
+		if (vma_pages(vma) !=
+		    next_start - uacce->qf_pg_start[type]) {
+			dev_err(&uacce->dev, "invalid mmap size (%ld vs %ld pages) for region %s.\n",
+				vma_pages(vma),
+				next_start - uacce->qf_pg_start[type],
+				qfrt_str[type]);
+			return UACCE_QFRT_INVALID;
+		}
+	}
+
+	return type;
+}
+
+static int uacce_fops_mmap(struct file *filep, struct vm_area_struct *vma)
+{
+	struct uacce_queue *q = (struct uacce_queue *)filep->private_data;
+	struct uacce *uacce = q->uacce;
+	enum uacce_qfrt type = uacce_get_region_type(uacce, vma);
+	struct uacce_qfile_region *qfr;
+	unsigned int flags = 0;
+	int ret;
+
+	dev_dbg(&uacce->dev, "mmap q file(t=%s, off=%lx, start=%lx, end=%lx)\n",
+		 qfrt_str[type], vma->vm_pgoff, vma->vm_start, vma->vm_end);
+
+	if (type == UACCE_QFRT_INVALID)
+		return -EINVAL;
+
+	vma->vm_flags |= VM_DONTCOPY | VM_DONTEXPAND;
+
+	uacce_qs_wlock();
+
+	/* fixme: if the region need no pages, we don't need to check it */
+	if (q->mm->data_vm + vma_pages(vma) >
+	    rlimit(RLIMIT_DATA) >> PAGE_SHIFT) {
+		ret = -ENOMEM;
+		goto out_with_lock;
+	}
+
+	if (q->qfrs[type]) {
+		ret = -EBUSY;
+		goto out_with_lock;
+	}
+
+	switch (type) {
+	case UACCE_QFRT_MMIO:
+		flags = UACCE_QFRF_SELFMT;
+		break;
+
+	case UACCE_QFRT_SS:
+		if (atomic_read(&uacce->state) != UACCE_ST_STARTED) {
+			ret = -EINVAL;
+			goto out_with_lock;
+		}
+
+		flags = UACCE_QFRF_MAP | UACCE_QFRF_MMAP;
+
+		if (uacce->flags & UACCE_DEV_NOIOMMU)
+			flags |= UACCE_QFRF_DMA;
+		break;
+
+	case UACCE_QFRT_DKO:
+		flags = UACCE_QFRF_MAP | UACCE_QFRF_KMAP;
+
+		if (uacce->flags & UACCE_DEV_NOIOMMU)
+			flags |= UACCE_QFRF_DMA;
+		break;
+
+	case UACCE_QFRT_DUS:
+		if ((uacce->flags & UACCE_DEV_NOIOMMU) ||
+		    (uacce->flags & UACCE_DEV_PASID)) {
+			flags = UACCE_QFRF_SELFMT;
+			break;
+		}
+
+		flags = UACCE_QFRF_MAP | UACCE_QFRF_MMAP;
+		break;
+
+	default:
+		WARN_ON(&uacce->dev);
+		break;
+	}
+
+	qfr = uacce_create_region(q, vma, type, flags);
+	if (IS_ERR(qfr)) {
+		ret = PTR_ERR(qfr);
+		goto out_with_lock;
+	}
+	q->qfrs[type] = qfr;
+
+	if (type == UACCE_QFRT_SS) {
+		INIT_LIST_HEAD(&qfr->qs);
+		list_add(&q->list, &q->qfrs[type]->qs);
+	}
+
+	uacce_qs_wunlock();
+
+	if (qfr->pages)
+		q->mm->data_vm += qfr->nr_pages;
+
+	return 0;
+
+out_with_lock:
+	uacce_qs_wunlock();
+	return ret;
+}
+
+static __poll_t uacce_fops_poll(struct file *file, poll_table *wait)
+{
+	struct uacce_queue *q = (struct uacce_queue *)file->private_data;
+	struct uacce *uacce = q->uacce;
+
+	poll_wait(file, &q->wait, wait);
+	if (uacce->ops->is_q_updated && uacce->ops->is_q_updated(q))
+		return EPOLLIN | EPOLLRDNORM;
+
+	return 0;
+}
+
+static const struct file_operations uacce_fops = {
+	.owner		= THIS_MODULE,
+	.open		= uacce_fops_open,
+	.release	= uacce_fops_release,
+	.unlocked_ioctl	= uacce_fops_unl_ioctl,
+#ifdef CONFIG_COMPAT
+	.compat_ioctl	= uacce_fops_compat_ioctl,
+#endif
+	.mmap		= uacce_fops_mmap,
+	.poll		= uacce_fops_poll,
+};
+
+#define UACCE_FROM_CDEV_ATTR(dev) container_of(dev, struct uacce, dev)
+
+static ssize_t id_show(struct device *dev,
+				 struct device_attribute *attr, char *buf)
+{
+	struct uacce *uacce = UACCE_FROM_CDEV_ATTR(dev);
+
+	return sprintf(buf, "%d\n", uacce->dev_id);
+}
+
+static ssize_t api_show(struct device *dev,
+				  struct device_attribute *attr, char *buf)
+{
+	struct uacce *uacce = UACCE_FROM_CDEV_ATTR(dev);
+
+	return sprintf(buf, "%s\n", uacce->api_ver);
+}
+
+static ssize_t numa_distance_show(struct device *dev,
+					    struct device_attribute *attr,
+					    char *buf)
+{
+	struct uacce *uacce = UACCE_FROM_CDEV_ATTR(dev);
+	int distance = 0;
+
+#ifdef CONFIG_NUMA
+	distance = cpu_to_node(smp_processor_id()) - uacce->pdev->numa_node;
+#endif
+	return sprintf(buf, "%d\n", abs(distance));
+}
+
+static ssize_t node_id_show(struct device *dev,
+				      struct device_attribute *attr,
+				      char *buf)
+{
+	struct uacce *uacce = UACCE_FROM_CDEV_ATTR(dev);
+	int node_id = -1;
+
+#ifdef CONFIG_NUMA
+	node_id = uacce->pdev->numa_node;
+#endif
+	return sprintf(buf, "%d\n", node_id);
+}
+
+static ssize_t flags_show(struct device *dev,
+				       struct device_attribute *attr,
+				       char *buf)
+{
+	struct uacce *uacce = UACCE_FROM_CDEV_ATTR(dev);
+
+	return sprintf(buf, "%d\n", uacce->flags);
+}
+
+static ssize_t available_instances_show(struct device *dev,
+					  struct device_attribute *attr,
+						  char *buf)
+{
+	struct uacce *uacce = UACCE_FROM_CDEV_ATTR(dev);
+
+	return sprintf(buf, "%d\n", uacce->ops->get_available_instances(uacce));
+}
+
+static ssize_t algorithms_show(struct device *dev,
+					 struct device_attribute *attr,
+					 char *buf)
+{
+	struct uacce *uacce = UACCE_FROM_CDEV_ATTR(dev);
+
+	return sprintf(buf, "%s", uacce->algs);
+}
+
+static ssize_t qfrs_offset_show(struct device *dev,
+					  struct device_attribute *attr,
+					  char *buf)
+{
+	struct uacce *uacce = UACCE_FROM_CDEV_ATTR(dev);
+	int i, ret;
+	unsigned long offset;
+
+	for (i = 0, ret = 0; i < UACCE_QFRT_MAX; i++) {
+		offset = uacce->qf_pg_start[i];
+		if (offset != UACCE_QFR_NA)
+			offset = offset << PAGE_SHIFT;
+		if (i == UACCE_QFRT_SS)
+			break;
+		ret += sprintf(buf + ret, "%lu\t", offset);
+	}
+	ret += sprintf(buf + ret, "%lu\n", offset);
+
+	return ret;
+}
+
+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_offset);
+
+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_offset.attr,
+	NULL,
+};
+
+static const struct attribute_group uacce_dev_attr_group = {
+	.name	= UACCE_DEV_ATTRS,
+	.attrs	= uacce_dev_attrs,
+};
+
+static const struct attribute_group *uacce_dev_attr_groups[] = {
+	&uacce_dev_attr_group,
+	NULL
+};
+
+static int uacce_create_chrdev(struct uacce *uacce)
+{
+	int ret;
+
+	ret = idr_alloc(&uacce_idr, uacce, 0, 0, GFP_KERNEL);
+	if (ret < 0)
+		return ret;
+
+	cdev_init(&uacce->cdev, &uacce_fops);
+	uacce->dev_id = ret;
+	uacce->cdev.owner = THIS_MODULE;
+	device_initialize(&uacce->dev);
+	uacce->dev.devt = MKDEV(MAJOR(uacce_devt), uacce->dev_id);
+	uacce->dev.class = uacce_class;
+	uacce->dev.groups = uacce_dev_attr_groups;
+	uacce->dev.parent = uacce->pdev;
+	dev_set_name(&uacce->dev, "%s-%d", uacce->drv_name, uacce->dev_id);
+	ret = cdev_device_add(&uacce->cdev, &uacce->dev);
+	if (ret)
+		goto err_with_idr;
+
+	dev_dbg(&uacce->dev, "create uacce minior=%d\n", uacce->dev_id);
+	return 0;
+
+err_with_idr:
+	idr_remove(&uacce_idr, uacce->dev_id);
+	return ret;
+}
+
+static void uacce_destroy_chrdev(struct uacce *uacce)
+{
+	cdev_device_del(&uacce->cdev, &uacce->dev);
+	idr_remove(&uacce_idr, uacce->dev_id);
+}
+
+static int uacce_default_get_available_instances(struct uacce *uacce)
+{
+	return -1;
+}
+
+static int uacce_default_start_queue(struct uacce_queue *q)
+{
+	dev_dbg(&q->uacce->dev, "fake start queue");
+	return 0;
+}
+
+static int uacce_dev_match(struct device *dev, void *data)
+{
+	if (dev->parent == data)
+		return -EBUSY;
+
+	return 0;
+}
+
+/* Borrowed from VFIO to fix msi translation */
+static bool uacce_iommu_has_sw_msi(struct iommu_group *group,
+				   phys_addr_t *base)
+{
+	struct list_head group_resv_regions;
+	struct iommu_resv_region *region, *next;
+	bool ret = false;
+
+	INIT_LIST_HEAD(&group_resv_regions);
+	iommu_get_group_resv_regions(group, &group_resv_regions);
+	list_for_each_entry(region, &group_resv_regions, list) {
+		pr_debug("uacce: find a resv region (%d) on %llx\n",
+			 region->type, region->start);
+
+		/*
+		 * The presence of any 'real' MSI regions should take
+		 * precedence over the software-managed one if the
+		 * IOMMU driver happens to advertise both types.
+		 */
+		if (region->type == IOMMU_RESV_MSI) {
+			ret = false;
+			break;
+		}
+
+		if (region->type == IOMMU_RESV_SW_MSI) {
+			*base = region->start;
+			ret = true;
+		}
+	}
+	list_for_each_entry_safe(region, next, &group_resv_regions, list)
+		kfree(region);
+	return ret;
+}
+
+static int uacce_set_iommu_domain(struct uacce *uacce)
+{
+	struct iommu_domain *domain;
+	struct iommu_group *group;
+	struct device *dev = uacce->pdev;
+	bool resv_msi;
+	phys_addr_t resv_msi_base = 0;
+	int ret;
+
+	if ((uacce->flags & UACCE_DEV_NOIOMMU) ||
+	    (uacce->flags & UACCE_DEV_PASID))
+		return 0;
+
+	/*
+	 * We don't support multiple register for the same dev in RFC version ,
+	 * will add it in formal version
+	 */
+	ret = class_for_each_device(uacce_class, NULL, uacce->pdev,
+				    uacce_dev_match);
+	if (ret)
+		return ret;
+
+	/* allocate and attach a unmanged domain */
+	domain = iommu_domain_alloc(uacce->pdev->bus);
+	if (!domain) {
+		dev_dbg(&uacce->dev, "cannot get domain for iommu\n");
+		return -ENODEV;
+	}
+
+	ret = iommu_attach_device(domain, uacce->pdev);
+	if (ret)
+		goto err_with_domain;
+
+	if (iommu_capable(dev->bus, IOMMU_CAP_CACHE_COHERENCY)) {
+		uacce->prot |= IOMMU_CACHE;
+		dev_dbg(dev, "Enable uacce with c-coherent capa\n");
+	} else
+		dev_dbg(dev, "Enable uacce without c-coherent capa\n");
+
+	group = iommu_group_get(dev);
+	if (!group) {
+		ret = -EINVAL;
+		goto err_with_domain;
+	}
+
+	resv_msi = uacce_iommu_has_sw_msi(group, &resv_msi_base);
+	iommu_group_put(group);
+
+	if (resv_msi) {
+		if (!irq_domain_check_msi_remap() &&
+		    !iommu_capable(dev->bus, IOMMU_CAP_INTR_REMAP)) {
+			dev_warn(dev, "No interrupt remapping support!");
+			ret = -EPERM;
+			goto err_with_domain;
+		}
+
+		dev_dbg(dev, "Set resv msi %llx on iommu domain\n",
+			(u64)resv_msi_base);
+		ret = iommu_get_msi_cookie(domain, resv_msi_base);
+		if (ret)
+			goto err_with_domain;
+	}
+
+	return 0;
+
+err_with_domain:
+	iommu_domain_free(domain);
+	return ret;
+}
+
+static void uacce_unset_iommu_domain(struct uacce *uacce)
+{
+	struct iommu_domain *domain;
+
+	if ((uacce->flags & UACCE_DEV_NOIOMMU) ||
+	    (uacce->flags & UACCE_DEV_PASID))
+		return;
+
+	domain = iommu_get_domain_for_dev(uacce->pdev);
+	if (domain) {
+		iommu_detach_device(domain, uacce->pdev);
+		iommu_domain_free(domain);
+	} else
+		dev_err(&uacce->dev, "bug: no domain attached to device\n");
+}
+
+/**
+ *	uacce_register - register an accelerator
+ *	@uacce: the accelerator structure
+ */
+int uacce_register(struct uacce *uacce)
+{
+	int ret;
+
+	if (!uacce->pdev) {
+		pr_debug("uacce parent device not set\n");
+		return -ENODEV;
+	}
+
+	if (uacce->flags & UACCE_DEV_NOIOMMU) {
+		add_taint(TAINT_CRAP, LOCKDEP_STILL_OK);
+		dev_warn(uacce->pdev,
+			 "Register to noiommu mode, which export kernel data to user space and may vulnerable to attack");
+	}
+
+	/* if dev support fault-from-dev, it should support pasid */
+	if ((uacce->flags & UACCE_DEV_FAULT_FROM_DEV) &&
+	    !(uacce->flags & UACCE_DEV_PASID)) {
+		dev_warn(&uacce->dev, "SVM/SAV device should support PASID\n");
+		return -EINVAL;
+	}
+
+	if (!uacce->ops->start_queue)
+		uacce->ops->start_queue = uacce_default_start_queue;
+
+	if (!uacce->ops->get_available_instances)
+		uacce->ops->get_available_instances =
+			uacce_default_get_available_instances;
+
+#ifdef CONFIG_IOMMU_SVA
+	if (uacce->flags & UACCE_DEV_PASID) {
+		ret = iommu_dev_enable_feature(uacce->pdev, IOMMU_DEV_FEAT_SVA);
+		if (ret)
+			uacce->flags &= ~(UACCE_DEV_FAULT_FROM_DEV |
+					  UACCE_DEV_PASID);
+	}
+#endif
+
+	ret = uacce_set_iommu_domain(uacce);
+	if (ret)
+		return ret;
+
+	mutex_lock(&uacce_mutex);
+
+	ret = uacce_create_chrdev(uacce);
+	if (ret)
+		goto err_with_lock;
+
+	dev_dbg(&uacce->dev, "uacce state initialized to INIT");
+	atomic_set(&uacce->state, UACCE_ST_INIT);
+	INIT_LIST_HEAD(&uacce->qs);
+	mutex_init(&uacce->q_lock);
+	mutex_unlock(&uacce_mutex);
+
+
+	return 0;
+
+err_with_lock:
+	mutex_unlock(&uacce_mutex);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(uacce_register);
+
+/**
+ * uacce_unregister - unregisters a uacce
+ * @uacce: the accelerator to unregister
+ *
+ * Unregister an accelerator that wat previously successully registered with
+ * uacce_register().
+ */
+void uacce_unregister(struct uacce *uacce)
+{
+	mutex_lock(&uacce_mutex);
+
+#ifdef CONFIG_IOMMU_SVA
+	if (uacce->flags & UACCE_DEV_PASID)
+		iommu_dev_disable_feature(uacce->pdev, IOMMU_DEV_FEAT_SVA);
+#endif
+	uacce_unset_iommu_domain(uacce);
+
+	uacce_destroy_chrdev(uacce);
+
+	mutex_unlock(&uacce_mutex);
+}
+EXPORT_SYMBOL_GPL(uacce_unregister);
+
+static int __init uacce_init(void)
+{
+	int ret;
+
+	uacce_class = class_create(THIS_MODULE, UACCE_CLASS_NAME);
+	if (IS_ERR(uacce_class)) {
+		ret = PTR_ERR(uacce_class);
+		goto err;
+	}
+
+	ret = alloc_chrdev_region(&uacce_devt, 0, MINORMASK, "uacce");
+	if (ret)
+		goto err_with_class;
+
+	pr_info("uacce init with major number:%d\n", MAJOR(uacce_devt));
+
+	return 0;
+
+err_with_class:
+	class_destroy(uacce_class);
+err:
+	return ret;
+}
+
+static __exit void uacce_exit(void)
+{
+	unregister_chrdev_region(uacce_devt, MINORMASK);
+	class_destroy(uacce_class);
+	idr_destroy(&uacce_idr);
+}
+
+subsys_initcall(uacce_init);
+module_exit(uacce_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Hisilicon Tech. Co., Ltd.");
+MODULE_DESCRIPTION("Accelerator interface for Userland applications");
diff --git a/include/linux/uacce.h b/include/linux/uacce.h
new file mode 100644
index 0000000..fe2f6f4
--- /dev/null
+++ b/include/linux/uacce.h
@@ -0,0 +1,109 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+#ifndef __UACCE_H
+#define __UACCE_H
+
+#include <linux/cdev.h>
+#include <linux/device.h>
+#include <linux/fs.h>
+#include <linux/list.h>
+#include <linux/iommu.h>
+#include <uapi/misc/uacce.h>
+
+struct uacce_queue;
+struct uacce;
+
+/* uacce mode of the driver */
+#define UACCE_MODE_NOUACCE	0 /* don't use uacce */
+#define UACCE_MODE_UACCE	1 /* use uacce exclusively */
+#define UACCE_MODE_NOIOMMU	2 /* use uacce noiommu mode */
+
+#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_qfile_region {
+	enum uacce_qfrt type;
+	unsigned long iova;	/* iova share between user and device space */
+	struct page **pages;
+	int nr_pages;
+	int prot;
+	unsigned int flags;
+	struct list_head qs;	/* qs sharing the same region, for ss */
+	void *kaddr;		/* kernel addr */
+	dma_addr_t dma;		/* dma address, if created by dma api */
+};
+
+/**
+ * struct uacce_ops - WD device operations
+ * @get_queue: get a queue from the device according to algorithm
+ * @put_queue: free a queue to the device
+ * @start_queue: make the queue start work after get_queue
+ * @stop_queue: make the queue stop work before put_queue
+ * @is_q_updated: check whether the task is finished
+ * @mask_notify: mask the task irq of queue
+ * @mmap: mmap addresses of queue to user space
+ * @reset: reset the WD device
+ * @reset_queue: reset the queue
+ * @ioctl:   ioctl for user space users of the queue
+ */
+struct uacce_ops {
+	int (*get_available_instances)(struct uacce *uacce);
+	int (*get_queue)(struct uacce *uacce, unsigned long arg,
+	     struct uacce_queue **q);
+	void (*put_queue)(struct uacce_queue *q);
+	int (*start_queue)(struct uacce_queue *q);
+	void (*stop_queue)(struct uacce_queue *q);
+	int (*is_q_updated)(struct uacce_queue *q);
+	void (*mask_notify)(struct uacce_queue *q, int event_mask);
+	int (*mmap)(struct uacce_queue *q, struct vm_area_struct *vma,
+		    struct uacce_qfile_region *qfr);
+	int (*reset)(struct uacce *uacce);
+	int (*reset_queue)(struct uacce_queue *q);
+	long (*ioctl)(struct uacce_queue *q, unsigned int cmd,
+		      unsigned long arg);
+};
+
+struct uacce_queue {
+	struct uacce *uacce;
+	void *priv;
+	wait_queue_head_t wait;
+	int pasid;
+	struct iommu_sva *handle;
+	struct list_head list; /* share list for qfr->qs */
+	struct mm_struct *mm;
+	struct uacce_qfile_region *qfrs[UACCE_QFRT_MAX];
+	struct list_head q_dev;
+};
+
+#define UACCE_ST_INIT		0
+#define UACCE_ST_OPENED		1
+#define UACCE_ST_STARTED	2
+#define UACCE_ST_RST		3
+
+struct uacce {
+	const char *name;
+	const char *drv_name;
+	const char *algs;
+	const char *api_ver;
+	unsigned int flags;
+	unsigned long qf_pg_start[UACCE_QFRT_MAX];
+	struct uacce_ops *ops;
+	struct device *pdev;
+	bool is_vf;
+	u32 dev_id;
+	struct cdev cdev;
+	struct device dev;
+	void *priv;
+	atomic_t state;
+	int prot;
+	struct mutex q_lock;
+	struct list_head qs;
+};
+
+int uacce_register(struct uacce *uacce);
+void uacce_unregister(struct uacce *uacce);
+void uacce_wake_up(struct uacce_queue *q);
+
+#endif
diff --git a/include/uapi/misc/uacce.h b/include/uapi/misc/uacce.h
new file mode 100644
index 0000000..44a0a5d
--- /dev/null
+++ b/include/uapi/misc/uacce.h
@@ -0,0 +1,44 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+#ifndef _UAPIUUACCE_H
+#define _UAPIUUACCE_H
+
+#include <linux/types.h>
+#include <linux/ioctl.h>
+
+#define UACCE_CLASS_NAME	"uacce"
+#define UACCE_DEV_ATTRS		"attrs"
+#define UACCE_CMD_SHARE_SVAS	_IO('W', 0)
+#define UACCE_CMD_START		_IO('W', 1)
+#define UACCE_CMD_GET_SS_DMA	_IOR('W', 2, unsigned long)
+
+/**
+ * UACCE Device Attributes:
+ *
+ * NOIOMMU: the device has no IOMMU support
+ *	can do share sva, but no map to the dev
+ * PASID: the device has IOMMU which support PASID setting
+ *	can do share sva, mapped to dev per process
+ * FAULT_FROM_DEV: the device has IOMMU which can do page fault request
+ *	no need for share sva, should be used with PASID
+ * SVA: full function device
+ * SHARE_DOMAIN: no PASID, can do share sva only for one process and the kernel
+ */
+#define UACCE_DEV_NOIOMMU		(1 << 0)
+#define UACCE_DEV_PASID			(1 << 1)
+#define UACCE_DEV_FAULT_FROM_DEV	(1 << 2)
+#define UACCE_DEV_SVA		(UACCE_DEV_PASID | UACCE_DEV_FAULT_FROM_DEV)
+#define UACCE_DEV_SHARE_DOMAIN	(0)
+
+#define UACCE_API_VER_NOIOMMU_SUBFIX	"_noiommu"
+
+#define UACCE_QFR_NA ((unsigned long)-1)
+enum uacce_qfrt {
+	UACCE_QFRT_MMIO = 0,	/* device mmio region */
+	UACCE_QFRT_DKO,		/* device kernel-only */
+	UACCE_QFRT_DUS,		/* device user share */
+	UACCE_QFRT_SS,		/* static share memory */
+	UACCE_QFRT_MAX,
+};
+#define UACCE_QFRT_INVALID UACCE_QFRT_MAX
+
+#endif
-- 
2.7.4

Re: [PATCH 2/2] uacce: add uacce module

[Greg Kroah-Hartman]

On Wed, Aug 14, 2019 at 05:34:25PM +0800, Zhangfei Gao wrote:
> diff --git a/include/uapi/misc/uacce.h b/include/uapi/misc/uacce.h
> new file mode 100644
> index 0000000..44a0a5d
> --- /dev/null
> +++ b/include/uapi/misc/uacce.h
> @@ -0,0 +1,44 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +#ifndef _UAPIUUACCE_H
> +#define _UAPIUUACCE_H
> +
> +#include <linux/types.h>
> +#include <linux/ioctl.h>
> +
> +#define UACCE_CLASS_NAME	"uacce"

Why is this in a uapi file?

> +#define UACCE_DEV_ATTRS		"attrs"

Same here.

> +#define UACCE_CMD_SHARE_SVAS	_IO('W', 0)
> +#define UACCE_CMD_START		_IO('W', 1)
> +#define UACCE_CMD_GET_SS_DMA	_IOR('W', 2, unsigned long)
> +
> +/**
> + * UACCE Device Attributes:
> + *
> + * NOIOMMU: the device has no IOMMU support
> + *	can do share sva, but no map to the dev
> + * PASID: the device has IOMMU which support PASID setting
> + *	can do share sva, mapped to dev per process
> + * FAULT_FROM_DEV: the device has IOMMU which can do page fault request
> + *	no need for share sva, should be used with PASID
> + * SVA: full function device
> + * SHARE_DOMAIN: no PASID, can do share sva only for one process and the kernel
> + */
> +#define UACCE_DEV_NOIOMMU		(1 << 0)
> +#define UACCE_DEV_PASID			(1 << 1)
> +#define UACCE_DEV_FAULT_FROM_DEV	(1 << 2)
> +#define UACCE_DEV_SVA		(UACCE_DEV_PASID | UACCE_DEV_FAULT_FROM_DEV)
> +#define UACCE_DEV_SHARE_DOMAIN	(0)
> +
> +#define UACCE_API_VER_NOIOMMU_SUBFIX	"_noiommu"
> +
> +#define UACCE_QFR_NA ((unsigned long)-1)
> +enum uacce_qfrt {
> +	UACCE_QFRT_MMIO = 0,	/* device mmio region */
> +	UACCE_QFRT_DKO,		/* device kernel-only */
> +	UACCE_QFRT_DUS,		/* device user share */
> +	UACCE_QFRT_SS,		/* static share memory */
> +	UACCE_QFRT_MAX,

These enums need to be explicitly set, as per the documentation,
otherwise they could be messed up when dealing with odd compilers.

> +};
> +#define UACCE_QFRT_INVALID UACCE_QFRT_MAX

Why not just use INVALID instead of MAX?

thanks,

greg k-h

Re: [PATCH 2/2] uacce: add uacce module

[Greg Kroah-Hartman]

On Wed, Aug 14, 2019 at 05:34:25PM +0800, Zhangfei Gao wrote:
> +int uacce_register(struct uacce *uacce)
> +{
> +	int ret;
> +
> +	if (!uacce->pdev) {
> +		pr_debug("uacce parent device not set\n");
> +		return -ENODEV;
> +	}
> +
> +	if (uacce->flags & UACCE_DEV_NOIOMMU) {
> +		add_taint(TAINT_CRAP, LOCKDEP_STILL_OK);
> +		dev_warn(uacce->pdev,
> +			 "Register to noiommu mode, which export kernel data to user space and may vulnerable to attack");
> +	}

THat is odd, why even offer this feature then if it is a major issue?

thanks,

greg k-h

Re: [PATCH 2/2] uacce: add uacce module

[Greg Kroah-Hartman]

On Wed, Aug 14, 2019 at 05:34:25PM +0800, Zhangfei Gao wrote:
> +/* lock to protect all queues management */
> +static DECLARE_RWSEM(uacce_qs_lock);
> +#define uacce_qs_rlock() down_read(&uacce_qs_lock)
> +#define uacce_qs_runlock() up_read(&uacce_qs_lock)
> +#define uacce_qs_wlock() down_write(&uacce_qs_lock)
> +#define uacce_qs_wunlock() up_write(&uacce_qs_lock)

Do not define your own locking macros.  That makes the code impossible
to review.

And are you _sure_ you need a rw lock?  You have benchmarked where it
has a performance impact?

> +/**
> + * uacce_wake_up - Wake up the process who is waiting this queue
> + * @q the accelerator queue to wake up
> + */
> +void uacce_wake_up(struct uacce_queue *q)
> +{
> +	dev_dbg(&q->uacce->dev, "wake up\n");

ftrace is your friend, no need for any such logging lines anywhere in
these files.

> +	wake_up_interruptible(&q->wait);
> +}
> +EXPORT_SYMBOL_GPL(uacce_wake_up);

...

> +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_offset.attr,
> +	NULL,
> +};
> +
> +static const struct attribute_group uacce_dev_attr_group = {
> +	.name	= UACCE_DEV_ATTRS,
> +	.attrs	= uacce_dev_attrs,
> +};

Why is your attribute group in a subdirectory?  Why not in the "normal"
class directory?

You are adding sysfs files to the kernel without any Documentation/ABI/
entries, which is a requirement.  Please fix that up for the next time
you send these.

> +static const struct attribute_group *uacce_dev_attr_groups[] = {
> +	&uacce_dev_attr_group,
> +	NULL
> +};
> +
> +static int uacce_create_chrdev(struct uacce *uacce)
> +{
> +	int ret;
> +
> +	ret = idr_alloc(&uacce_idr, uacce, 0, 0, GFP_KERNEL);
> +	if (ret < 0)
> +		return ret;
> +

Shouldn't this function create the memory needed for this structure?
You are relying ont he caller to do it for you, why?


> +	cdev_init(&uacce->cdev, &uacce_fops);
> +	uacce->dev_id = ret;
> +	uacce->cdev.owner = THIS_MODULE;
> +	device_initialize(&uacce->dev);
> +	uacce->dev.devt = MKDEV(MAJOR(uacce_devt), uacce->dev_id);
> +	uacce->dev.class = uacce_class;
> +	uacce->dev.groups = uacce_dev_attr_groups;
> +	uacce->dev.parent = uacce->pdev;
> +	dev_set_name(&uacce->dev, "%s-%d", uacce->drv_name, uacce->dev_id);
> +	ret = cdev_device_add(&uacce->cdev, &uacce->dev);
> +	if (ret)
> +		goto err_with_idr;
> +
> +	dev_dbg(&uacce->dev, "create uacce minior=%d\n", uacce->dev_id);
> +	return 0;
> +
> +err_with_idr:
> +	idr_remove(&uacce_idr, uacce->dev_id);
> +	return ret;
> +}
> +
> +static void uacce_destroy_chrdev(struct uacce *uacce)
> +{
> +	cdev_device_del(&uacce->cdev, &uacce->dev);
> +	idr_remove(&uacce_idr, uacce->dev_id);
> +}
> +
> +static int uacce_default_get_available_instances(struct uacce *uacce)
> +{
> +	return -1;

Do not make up error values, use the proper -EXXXX value instead.

> +}
> +
> +static int uacce_default_start_queue(struct uacce_queue *q)
> +{
> +	dev_dbg(&q->uacce->dev, "fake start queue");
> +	return 0;

Why even have this function if you do not do anything in it?

> +}
> +
> +static int uacce_dev_match(struct device *dev, void *data)
> +{
> +	if (dev->parent == data)
> +		return -EBUSY;

There should be in-kernel functions for this now, no need for you to
roll your own.

thanks,

greg k-h

Re: [PATCH 2/2] uacce: add uacce module

[Jonathan Cameron]

On Wed, 14 Aug 2019 17:34:25 +0800
Zhangfei Gao <zhangfei.gao@linaro.org> wrote:

> From: Kenneth Lee <liguozhu@hisilicon.com>
> 
> Uacce is the kernel component to support WarpDrive accelerator
> framework. It provides register/unregister interface for device drivers
> to expose their hardware resource to the user space. The resource is
> taken as "queue" in WarpDrive.

It's a bit confusing to have both the term UACCE and WarpDrive in here.
I'd just use the uacce name in all comments etc.

> 
> 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.
> 
> Uacce also manages unify addresses between the hardware and user space
> of the process. So they can share the same virtual address in the
> communication.
> 
> Signed-off-by: Kenneth Lee <liguozhu@hisilicon.com>
> Signed-off-by: Zaibo Xu <xuzaibo@huawei.com>
> Signed-off-by: Zhou Wang <wangzhou1@hisilicon.com>
> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>

I would strip this back to which ever case is of most interest (SVA I guess?)
and only think about adding support for the others if necessary at a later date.
(or in later patches).

This is only a fairly superficial review, so I'll have another look when time
allows.

Thanks,

Jonathan

> ---
>  drivers/misc/Kconfig        |    1 +
>  drivers/misc/Makefile       |    1 +
>  drivers/misc/uacce/Kconfig  |   13 +
>  drivers/misc/uacce/Makefile |    2 +
>  drivers/misc/uacce/uacce.c  | 1186 +++++++++++++++++++++++++++++++++++++++++++
>  include/linux/uacce.h       |  109 ++++
>  include/uapi/misc/uacce.h   |   44 ++
>  7 files changed, 1356 insertions(+)
>  create mode 100644 drivers/misc/uacce/Kconfig
>  create mode 100644 drivers/misc/uacce/Makefile
>  create mode 100644 drivers/misc/uacce/uacce.c
>  create mode 100644 include/linux/uacce.h
>  create mode 100644 include/uapi/misc/uacce.h
> 
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index 6abfc8e..8073eb8 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -502,4 +502,5 @@ source "drivers/misc/cxl/Kconfig"
>  source "drivers/misc/ocxl/Kconfig"
>  source "drivers/misc/cardreader/Kconfig"
>  source "drivers/misc/habanalabs/Kconfig"
> +source "drivers/misc/uacce/Kconfig"
>  endmenu
> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> index abd8ae2..93a131b 100644
> --- a/drivers/misc/Makefile
> +++ b/drivers/misc/Makefile
> @@ -58,4 +58,5 @@ obj-$(CONFIG_OCXL)		+= ocxl/
>  obj-y				+= cardreader/
>  obj-$(CONFIG_PVPANIC)   	+= pvpanic.o
>  obj-$(CONFIG_HABANA_AI)		+= habanalabs/
> +obj-$(CONFIG_UACCE)		+= uacce/
>  obj-$(CONFIG_XILINX_SDFEC)	+= xilinx_sdfec.o
> diff --git a/drivers/misc/uacce/Kconfig b/drivers/misc/uacce/Kconfig
> new file mode 100644
> index 0000000..569669c
> --- /dev/null
> +++ b/drivers/misc/uacce/Kconfig
> @@ -0,0 +1,13 @@
> +config UACCE
> +	tristate "Accelerator Framework for User Land"
> +	depends on IOMMU_API
> +	help
> +	  UACCE provides interface for the user process to access the hardware
> +	  without interaction with the kernel space in data path.
> +
> +	  The user-space interface is described in
> +	  include/uapi/misc/uacce.h
> +
> +	  See Documentation/misc-devices/warpdrive.rst for more details.
> +
> +	  If you don't know what to do here, say N.
> diff --git a/drivers/misc/uacce/Makefile b/drivers/misc/uacce/Makefile
> new file mode 100644
> index 0000000..5b4374e
> --- /dev/null
> +++ b/drivers/misc/uacce/Makefile
> @@ -0,0 +1,2 @@
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +obj-$(CONFIG_UACCE) += uacce.o
> diff --git a/drivers/misc/uacce/uacce.c b/drivers/misc/uacce/uacce.c
> new file mode 100644
> index 0000000..43e0c9b
> --- /dev/null
> +++ b/drivers/misc/uacce/uacce.c
> @@ -0,0 +1,1186 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +#include <linux/compat.h>
> +#include <linux/dma-iommu.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/file.h>
> +#include <linux/idr.h>
> +#include <linux/irqdomain.h>
> +#include <linux/module.h>
> +#include <linux/poll.h>
> +#include <linux/sched/signal.h>
> +#include <linux/uacce.h>
> +
> +static struct class *uacce_class;
> +static DEFINE_IDR(uacce_idr);
> +static dev_t uacce_devt;
> +static DEFINE_MUTEX(uacce_mutex); /* mutex to protect uacce */
> +
> +/* lock to protect all queues management */
> +static DECLARE_RWSEM(uacce_qs_lock);
> +#define uacce_qs_rlock() down_read(&uacce_qs_lock)
> +#define uacce_qs_runlock() up_read(&uacce_qs_lock)
> +#define uacce_qs_wlock() down_write(&uacce_qs_lock)
> +#define uacce_qs_wunlock() up_write(&uacce_qs_lock)
> +
> +static const struct file_operations uacce_fops;
> +
> +/* match with enum uacce_qfrt */
> +static const char *const qfrt_str[] = {
> +	"mmio",
> +	"dko",
> +	"dus",
> +	"ss",
> +	"invalid"
> +};
> +
> +static const char *uacce_qfrt_str(struct uacce_qfile_region *qfr)
> +{
> +	enum uacce_qfrt type = qfr->type;
> +
> +	if (type > UACCE_QFRT_INVALID)
> +		type = UACCE_QFRT_INVALID;
> +
> +	return qfrt_str[type];
> +}
> +
> +/**
> + * uacce_wake_up - Wake up the process who is waiting this queue
> + * @q the accelerator queue to wake up
> + */
> +void uacce_wake_up(struct uacce_queue *q)
> +{
> +	dev_dbg(&q->uacce->dev, "wake up\n");
> +	wake_up_interruptible(&q->wait);
> +}
> +EXPORT_SYMBOL_GPL(uacce_wake_up);
> +
> +static int uacce_queue_map_qfr(struct uacce_queue *q,
> +			       struct uacce_qfile_region *qfr)
> +{
> +	struct device *dev = q->uacce->pdev;
> +	struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
> +	int i, j, ret;
> +
> +	if (!(qfr->flags & UACCE_QFRF_MAP) || (qfr->flags & UACCE_QFRF_DMA))
> +		return 0;
> +
> +	dev_dbg(dev, "queue map %s qfr(npage=%d, iova=%lx)\n",
> +		uacce_qfrt_str(qfr), qfr->nr_pages, qfr->iova);
> +
> +	if (!domain)
> +		return -ENODEV;
> +
> +	for (i = 0; i < qfr->nr_pages; i++) {
> +		ret = iommu_map(domain, qfr->iova + i * PAGE_SIZE,
> +				page_to_phys(qfr->pages[i]),
> +				PAGE_SIZE, qfr->prot | q->uacce->prot);
> +		if (ret) {
> +			dev_err(dev, "iommu_map page %i fail %d\n", i, ret);
> +			goto err_with_map_pages;
> +		}
> +		get_page(qfr->pages[i]);
> +	}
> +
> +	return 0;
> +
> +err_with_map_pages:
> +	for (j = i - 1; j >= 0; j--) {
> +		iommu_unmap(domain, qfr->iova + j * PAGE_SIZE, PAGE_SIZE);
> +		put_page(qfr->pages[j]);
> +	}
> +	return ret;
> +}
> +
> +static void uacce_queue_unmap_qfr(struct uacce_queue *q,
> +				  struct uacce_qfile_region *qfr)
> +{
> +	struct device *dev = q->uacce->pdev;
> +	struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
> +	int i;
> +
> +	if (!(qfr->flags & UACCE_QFRF_MAP) || (qfr->flags & UACCE_QFRF_DMA))
> +		return;
> +
> +	dev_dbg(dev, "queue map %s qfr(npage=%d, iova=%lx)\n",
> +		uacce_qfrt_str(qfr), qfr->nr_pages, qfr->iova);
> +
> +	if (!domain || !qfr)
> +		return;
> +
> +	for (i = qfr->nr_pages - 1; i >= 0; i--) {
> +		iommu_unmap(domain, qfr->iova + i * PAGE_SIZE, PAGE_SIZE);
> +		put_page(qfr->pages[i]);
> +	}
> +}
> +
> +static int uacce_qfr_alloc_pages(struct uacce_qfile_region *qfr)
> +{
> +	int gfp_mask = GFP_ATOMIC | __GFP_ZERO;

More readable to just have this inline.

> +	int i, j;
> +
> +	qfr->pages = kcalloc(qfr->nr_pages, sizeof(*qfr->pages), gfp_mask);

kcalloc is always set to zero anyway.

> +	if (!qfr->pages)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < qfr->nr_pages; i++) {
> +		qfr->pages[i] = alloc_page(gfp_mask);
> +		if (!qfr->pages[i])
> +			goto err_with_pages;
> +	}
> +
> +	return 0;
> +
> +err_with_pages:
> +	for (j = i - 1; j >= 0; j--)
> +		put_page(qfr->pages[j]);
> +
> +	kfree(qfr->pages);
> +	return -ENOMEM;
> +}
> +
> +static void uacce_qfr_free_pages(struct uacce_qfile_region *qfr)
> +{
> +	int i;
> +
> +	for (i = 0; i < qfr->nr_pages; i++)
> +		put_page(qfr->pages[i]);
> +
> +	kfree(qfr->pages);
> +}
> +
> +static inline int uacce_queue_mmap_qfr(struct uacce_queue *q,
> +				       struct uacce_qfile_region *qfr,
> +				       struct vm_area_struct *vma)
> +{
> +	int i, ret;
> +
> +	if (qfr->nr_pages)
> +		dev_dbg(q->uacce->pdev, "mmap qfr (page ref=%d)\n",
> +			page_ref_count(qfr->pages[0]));
> +	for (i = 0; i < qfr->nr_pages; i++) {
> +		ret = remap_pfn_range(vma, vma->vm_start + (i << PAGE_SHIFT),
> +				      page_to_pfn(qfr->pages[i]), PAGE_SIZE,
> +				      vma->vm_page_prot);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static struct uacce_qfile_region *
> +uacce_create_region(struct uacce_queue *q, struct vm_area_struct *vma,
> +		    enum uacce_qfrt type, unsigned int flags)
> +{
> +	struct uacce_qfile_region *qfr;
> +	struct uacce *uacce = q->uacce;
> +	unsigned long vm_pgoff;
> +	int ret = -ENOMEM;
> +
> +	dev_dbg(uacce->pdev, "create qfr (type=%x, flags=%x)\n", type, flags);
> +	qfr = kzalloc(sizeof(*qfr), GFP_ATOMIC);
> +	if (!qfr)
> +		return ERR_PTR(-ENOMEM);
> +
> +	qfr->type = type;
> +	qfr->flags = flags;
> +	qfr->iova = vma->vm_start;
> +	qfr->nr_pages = vma_pages(vma);
> +
> +	if (vma->vm_flags & VM_READ)
> +		qfr->prot |= IOMMU_READ;
> +
> +	if (vma->vm_flags & VM_WRITE)
> +		qfr->prot |= IOMMU_WRITE;
> +
> +	if (flags & UACCE_QFRF_SELFMT) {
> +		ret = uacce->ops->mmap(q, vma, qfr);
> +		if (ret)
> +			goto err_with_qfr;
> +		return qfr;
> +	}
> +
> +	/* allocate memory */
> +	if (flags & UACCE_QFRF_DMA) {
> +		dev_dbg(uacce->pdev, "allocate dma %d pages\n", qfr->nr_pages);
> +		qfr->kaddr = dma_alloc_coherent(uacce->pdev, qfr->nr_pages <<
> +						PAGE_SHIFT, &qfr->dma,
> +						GFP_KERNEL);
> +		if (!qfr->kaddr) {
> +			ret = -ENOMEM;
> +			goto err_with_qfr;
> +		}
> +	} else {
> +		dev_dbg(uacce->pdev, "allocate %d pages\n", qfr->nr_pages);
> +		ret = uacce_qfr_alloc_pages(qfr);
> +		if (ret)
> +			goto err_with_qfr;
> +	}
> +
> +	/* map to device */
> +	ret = uacce_queue_map_qfr(q, qfr);
> +	if (ret)
> +		goto err_with_pages;
> +
> +	/* mmap to user space */
> +	if (flags & UACCE_QFRF_MMAP) {
> +		if (flags & UACCE_QFRF_DMA) {
> +
> +			/* dma_mmap_coherent() requires vm_pgoff as 0
> +			 * restore vm_pfoff to initial value for mmap()
> +			 */
> +			dev_dbg(uacce->pdev, "mmap dma qfr\n");
> +			vm_pgoff = vma->vm_pgoff;
> +			vma->vm_pgoff = 0;
> +			ret = dma_mmap_coherent(uacce->pdev, vma, qfr->kaddr,
> +						qfr->dma,
> +						qfr->nr_pages << PAGE_SHIFT);

Does setting vm_pgoff if (ret) make sense?

> +			vma->vm_pgoff = vm_pgoff;
> +		} else {
> +			ret = uacce_queue_mmap_qfr(q, qfr, vma);
> +		}
> +
> +		if (ret)
> +			goto err_with_mapped_qfr;
> +	}
> +
> +	return qfr;
> +
> +err_with_mapped_qfr:
> +	uacce_queue_unmap_qfr(q, qfr);
> +err_with_pages:
> +	if (flags & UACCE_QFRF_DMA)
> +		dma_free_coherent(uacce->pdev, qfr->nr_pages << PAGE_SHIFT,
> +				  qfr->kaddr, qfr->dma);
> +	else
> +		uacce_qfr_free_pages(qfr);
> +err_with_qfr:
> +	kfree(qfr);
> +
> +	return ERR_PTR(ret);
> +}
> +
> +/* we assume you have uacce_queue_unmap_qfr(q, qfr) from all related queues */
> +static void uacce_destroy_region(struct uacce_queue *q,
> +				 struct uacce_qfile_region *qfr)
> +{
> +	struct uacce *uacce = q->uacce;
> +
> +	if (qfr->flags & UACCE_QFRF_DMA) {
> +		dev_dbg(uacce->pdev, "free dma qfr %s (kaddr=%lx, dma=%llx)\n",
> +			uacce_qfrt_str(qfr), (unsigned long)qfr->kaddr,
> +			qfr->dma);
> +		dma_free_coherent(uacce->pdev, qfr->nr_pages << PAGE_SHIFT,
> +				  qfr->kaddr, qfr->dma);
> +	} else if (qfr->pages) {
> +		if (qfr->flags & UACCE_QFRF_KMAP && qfr->kaddr) {
> +			dev_dbg(uacce->pdev, "vunmap qfr %s\n",
> +				uacce_qfrt_str(qfr));
> +			vunmap(qfr->kaddr);
> +			qfr->kaddr = NULL;
> +		}
> +
> +		uacce_qfr_free_pages(qfr);
> +	}
> +	kfree(qfr);
> +}
> +
> +static long uacce_cmd_share_qfr(struct uacce_queue *tgt, int fd)
> +{
> +	struct file *filep = fget(fd);

That's not a trivial assignment so I would not have it up here.

> +	struct uacce_queue *src;
> +	int ret = -EINVAL;
> +
	filep = fget(fd);
> +	if (!filep)
> +		return ret;
> +
> +	if (filep->f_op != &uacce_fops)
> +		goto out_with_fd;
> +
> +	src = filep->private_data;
> +	if (!src)
> +		goto out_with_fd;
> +
> +	/* no share sva is needed if the dev can do fault-from-dev */
> +	if (tgt->uacce->flags & UACCE_DEV_FAULT_FROM_DEV)
> +		goto out_with_fd;
> +
> +	dev_dbg(&src->uacce->dev, "share ss with %s\n",
> +		dev_name(&tgt->uacce->dev));
> +
> +	uacce_qs_wlock();
> +	if (!src->qfrs[UACCE_QFRT_SS] || tgt->qfrs[UACCE_QFRT_SS])
> +		goto out_with_lock;
> +
> +	ret = uacce_queue_map_qfr(tgt, src->qfrs[UACCE_QFRT_SS]);
> +	if (ret)
> +		goto out_with_lock;
> +
> +	tgt->qfrs[UACCE_QFRT_SS] = src->qfrs[UACCE_QFRT_SS];
> +	list_add(&tgt->list, &src->qfrs[UACCE_QFRT_SS]->qs);
> +	ret = 0;
> +
> +out_with_lock:
> +	uacce_qs_wunlock();
> +out_with_fd:
> +	fput(filep);
> +	return ret;
> +}
> +
> +static int uacce_start_queue(struct uacce_queue *q)
> +{
> +	int ret, i, j;
> +	struct uacce_qfile_region *qfr;
> +	struct device *dev = &q->uacce->dev;
> +
> +	/*
> +	 * map KMAP qfr to kernel
> +	 * vmap should be done in non-spinlocked context!
> +	 */
> +	for (i = 0; i < UACCE_QFRT_MAX; i++) {
> +		qfr = q->qfrs[i];
> +		if (qfr && (qfr->flags & UACCE_QFRF_KMAP) && !qfr->kaddr) {
> +			qfr->kaddr = vmap(qfr->pages, qfr->nr_pages, VM_MAP,
> +					  PAGE_KERNEL);
> +			if (!qfr->kaddr) {
> +				ret = -ENOMEM;
> +				dev_dbg(dev, "fail to kmap %s qfr(%d pages)\n",
> +					uacce_qfrt_str(qfr), qfr->nr_pages);
If it's useful, dev_err.

> +				goto err_with_vmap;
> +			}
> +
> +			dev_dbg(dev, "kernel vmap %s qfr(%d pages) to %lx\n",
> +				uacce_qfrt_str(qfr), qfr->nr_pages,
> +				(unsigned long)qfr->kaddr);
> +		}
> +	}
> +
> +	ret = q->uacce->ops->start_queue(q);
> +	if (ret < 0)
> +		goto err_with_vmap;
> +
> +	dev_dbg(&q->uacce->dev, "uacce state switch to STARTED\n");
> +	atomic_set(&q->uacce->state, UACCE_ST_STARTED);
> +	return 0;
> +
> +err_with_vmap:
> +	for (j = i; j >= 0; j--) {
> +		qfr = q->qfrs[j];
> +		if (qfr && qfr->kaddr) {
> +			vunmap(qfr->kaddr);
> +			qfr->kaddr = NULL;
> +		}
> +	}
> +	return ret;
> +}
> +
> +static long uacce_get_ss_dma(struct uacce_queue *q, void __user *arg)
> +{
> +	struct uacce *uacce = q->uacce;
> +	long ret = 0;
> +	unsigned long dma = 0;
> +
> +	if (!(uacce->flags & UACCE_DEV_NOIOMMU))
> +		return -EINVAL;
> +
> +	uacce_qs_wlock();
> +	if (q->qfrs[UACCE_QFRT_SS]) {
> +		dma = (unsigned long)(q->qfrs[UACCE_QFRT_SS]->dma);
> +		dev_dbg(&uacce->dev, "%s (%lx)\n", __func__, dma);
> +	} else {
> +		ret = -EINVAL;
> +	}
> +	uacce_qs_wunlock();
> +
> +	if (copy_to_user(arg, &dma, sizeof(dma)))
> +		ret = -EFAULT;
> +
> +	return ret;
> +}
> +
> +static long uacce_fops_unl_ioctl(struct file *filep,
> +				 unsigned int cmd, unsigned long arg)
> +{
> +	struct uacce_queue *q = filep->private_data;
> +	struct uacce *uacce = q->uacce;
> +
> +	switch (cmd) {
> +	case UACCE_CMD_SHARE_SVAS:
> +		return uacce_cmd_share_qfr(q, arg);
> +
> +	case UACCE_CMD_START:
> +		return uacce_start_queue(q);
> +
> +	case UACCE_CMD_GET_SS_DMA:
> +		return uacce_get_ss_dma(q, (void __user *)arg);
> +
> +	default:
> +		if (uacce->ops->ioctl)
> +			return uacce->ops->ioctl(q, cmd, arg);
> +
> +		dev_err(&uacce->dev, "ioctl cmd (%d) is not supported!\n", cmd);
> +		return -EINVAL;
Flip the logic so the error is the indented path.
		if (!uacce->ops->ioctl) ...

Fits better with typical coding model in a kernel reviewers head
- well mine anyway ;)

> +	}
> +}
> +
> +#ifdef CONFIG_COMPAT
> +static long uacce_fops_compat_ioctl(struct file *filep,
> +				   unsigned int cmd, unsigned long arg)
> +{
> +	arg = (unsigned long)compat_ptr(arg);
> +	return uacce_fops_unl_ioctl(filep, cmd, arg);
> +}
> +#endif
> +
> +static int uacce_dev_open_check(struct uacce *uacce)
> +{
> +	if (uacce->flags & UACCE_DEV_NOIOMMU)
> +		return 0;
> +
> +	/*
> +	 * The device can be opened once if it dose not support multiple page
> +	 * table. The better way to check this is counting it per iommu_domain,
> +	 * this is just a temporary solution
> +	 */
> +	if (uacce->flags & (UACCE_DEV_PASID | UACCE_DEV_NOIOMMU))
> +		return 0;
> +
> +	if (atomic_cmpxchg(&uacce->state, UACCE_ST_INIT, UACCE_ST_OPENED) !=
> +	    UACCE_ST_INIT) {
> +		dev_info(&uacce->dev, "this device can be openned only once\n");
> +		return -EBUSY;
> +	}
> +
> +	dev_dbg(&uacce->dev, "state switch to OPENNED");
> +
> +	return 0;
> +}
> +
> +static int uacce_fops_open(struct inode *inode, struct file *filep)
> +{
> +	struct uacce_queue *q;
> +	struct iommu_sva *handle = NULL;
> +	struct uacce *uacce;
> +	int ret;
> +	int pasid = 0;
> +
> +	uacce = idr_find(&uacce_idr, iminor(inode));
> +	if (!uacce)
> +		return -ENODEV;
> +
> +	if (atomic_read(&uacce->state) == UACCE_ST_RST)
> +		return -EINVAL;
> +
> +	if (!uacce->ops->get_queue)
> +		return -EINVAL;
> +
> +	if (!try_module_get(uacce->pdev->driver->owner))
> +		return -ENODEV;
> +
> +	ret = uacce_dev_open_check(uacce);
> +	if (ret)
> +		goto open_err;
> +
> +#ifdef CONFIG_IOMMU_SVA
> +	if (uacce->flags & UACCE_DEV_PASID) {
> +		handle = iommu_sva_bind_device(uacce->pdev, current->mm, NULL);
> +		if (IS_ERR(handle))
> +			goto open_err;
> +		pasid = iommu_sva_get_pasid(handle);
> +	}
> +#endif
> +	ret = uacce->ops->get_queue(uacce, pasid, &q);
> +	if (ret < 0)
> +		goto open_err;
> +
> +	q->pasid = pasid;
> +	q->handle = handle;
> +	q->uacce = uacce;
> +	q->mm = current->mm;
> +	memset(q->qfrs, 0, sizeof(q->qfrs));
> +	INIT_LIST_HEAD(&q->list);
> +	init_waitqueue_head(&q->wait);
> +	filep->private_data = q;
> +	mutex_lock(&uacce->q_lock);
> +	list_add(&q->q_dev, &uacce->qs);
> +	mutex_unlock(&uacce->q_lock);
> +
> +	return 0;

blank line

> +open_err:
> +	module_put(uacce->pdev->driver->owner);
> +	return ret;
> +}
> +
> +static int uacce_fops_release(struct inode *inode, struct file *filep)
> +{
> +	struct uacce_queue *q = (struct uacce_queue *)filep->private_data;
> +	struct uacce_qfile_region *qfr;
> +	struct uacce *uacce = q->uacce;
> +	int i;
> +	bool is_to_free_region;
> +	int free_pages = 0;
> +
> +	mutex_lock(&uacce->q_lock);
> +	list_del(&q->q_dev);
> +	mutex_unlock(&uacce->q_lock);
> +
> +	if (atomic_read(&uacce->state) == UACCE_ST_STARTED &&
> +	    uacce->ops->stop_queue)
> +		uacce->ops->stop_queue(q);
> +
> +	uacce_qs_wlock();
> +
> +	for (i = 0; i < UACCE_QFRT_MAX; i++) {
> +		qfr = q->qfrs[i];
> +		if (!qfr)
> +			continue;
> +
> +		is_to_free_region = false;
> +		uacce_queue_unmap_qfr(q, qfr);
> +		if (i == UACCE_QFRT_SS) {
> +			list_del(&q->list);
> +			if (list_empty(&qfr->qs))
> +				is_to_free_region = true;
> +		} else
> +			is_to_free_region = true;
> +
> +		if (is_to_free_region) {
> +			free_pages += qfr->nr_pages;
> +			uacce_destroy_region(q, qfr);
> +		}
> +
> +		qfr = NULL;
> +	}
> +
> +	uacce_qs_wunlock();
> +
> +	if (current->mm == q->mm) {
> +		down_write(&q->mm->mmap_sem);
> +		q->mm->data_vm -= free_pages;
> +		up_write(&q->mm->mmap_sem);
> +	}
> +
> +#ifdef CONFIG_IOMMU_SVA
> +	if (uacce->flags & UACCE_DEV_PASID)
> +		iommu_sva_unbind_device(q->handle);
> +#endif
> +
> +	if (uacce->ops->put_queue)
> +		uacce->ops->put_queue(q);
> +
> +	dev_dbg(&uacce->dev, "uacce state switch to INIT\n");
> +	atomic_set(&uacce->state, UACCE_ST_INIT);
> +	module_put(uacce->pdev->driver->owner);

blank line here.

> +	return 0;
> +}
> +
> +static enum uacce_qfrt uacce_get_region_type(struct uacce *uacce,
> +					     struct vm_area_struct *vma)
> +{
> +	enum uacce_qfrt type = UACCE_QFRT_MAX;
> +	int i;
> +	size_t next_start = UACCE_QFR_NA;
> +
> +	for (i = UACCE_QFRT_MAX - 1; i >= 0; i--) {
> +		if (vma->vm_pgoff >= uacce->qf_pg_start[i]) {
> +			type = i;
> +			break;
> +		}
> +	}
> +
> +	switch (type) {
> +	case UACCE_QFRT_MMIO:
> +		if (!uacce->ops->mmap) {
> +			dev_err(&uacce->dev, "no driver mmap!\n");
> +			return UACCE_QFRT_INVALID;
> +		}
> +		break;
> +
> +	case UACCE_QFRT_DKO:
> +		if ((uacce->flags & UACCE_DEV_PASID) ||
> +		    (uacce->flags & UACCE_DEV_NOIOMMU))
> +			return UACCE_QFRT_INVALID;
> +		break;
> +
> +	case UACCE_QFRT_DUS:
> +		break;
> +
> +	case UACCE_QFRT_SS:
> +		/* todo: this can be valid to protect the process space */
> +		if (uacce->flags & UACCE_DEV_FAULT_FROM_DEV)
> +			return UACCE_QFRT_INVALID;
> +		break;
> +
> +	default:
> +		dev_err(&uacce->dev, "uacce bug (%d)!\n", type);
> +		return UACCE_QFRT_INVALID;
> +	}
> +
> +	/* make sure the mapping size is exactly the same as the region */
> +	if (type < UACCE_QFRT_SS) {
> +		for (i = type + 1; i < UACCE_QFRT_MAX; i++)
> +			if (uacce->qf_pg_start[i] != UACCE_QFR_NA) {
> +				next_start = uacce->qf_pg_start[i];
> +				break;
> +			}
> +
> +		if (next_start == UACCE_QFR_NA) {
> +			dev_err(&uacce->dev, "uacce config error: SS offset set improperly\n");
> +			return UACCE_QFRT_INVALID;
> +		}
> +
> +		if (vma_pages(vma) !=
> +		    next_start - uacce->qf_pg_start[type]) {
> +			dev_err(&uacce->dev, "invalid mmap size (%ld vs %ld pages) for region %s.\n",
> +				vma_pages(vma),
> +				next_start - uacce->qf_pg_start[type],
> +				qfrt_str[type]);
> +			return UACCE_QFRT_INVALID;
> +		}
> +	}
> +
> +	return type;
> +}
> +
> +static int uacce_fops_mmap(struct file *filep, struct vm_area_struct *vma)
> +{
> +	struct uacce_queue *q = (struct uacce_queue *)filep->private_data;

As below, cast not needed.

> +	struct uacce *uacce = q->uacce;
> +	enum uacce_qfrt type = uacce_get_region_type(uacce, vma);
> +	struct uacce_qfile_region *qfr;
> +	unsigned int flags = 0;
> +	int ret;
> +
> +	dev_dbg(&uacce->dev, "mmap q file(t=%s, off=%lx, start=%lx, end=%lx)\n",
> +		 qfrt_str[type], vma->vm_pgoff, vma->vm_start, vma->vm_end);
> +
> +	if (type == UACCE_QFRT_INVALID)
> +		return -EINVAL;
> +
> +	vma->vm_flags |= VM_DONTCOPY | VM_DONTEXPAND;
> +
> +	uacce_qs_wlock();
> +
> +	/* fixme: if the region need no pages, we don't need to check it */
> +	if (q->mm->data_vm + vma_pages(vma) >
> +	    rlimit(RLIMIT_DATA) >> PAGE_SHIFT) {
> +		ret = -ENOMEM;
> +		goto out_with_lock;
> +	}
> +
> +	if (q->qfrs[type]) {
> +		ret = -EBUSY;
> +		goto out_with_lock;
> +	}
> +
> +	switch (type) {
> +	case UACCE_QFRT_MMIO:
> +		flags = UACCE_QFRF_SELFMT;
> +		break;
> +
> +	case UACCE_QFRT_SS:
> +		if (atomic_read(&uacce->state) != UACCE_ST_STARTED) {
> +			ret = -EINVAL;
> +			goto out_with_lock;
> +		}
> +
> +		flags = UACCE_QFRF_MAP | UACCE_QFRF_MMAP;
> +
> +		if (uacce->flags & UACCE_DEV_NOIOMMU)
> +			flags |= UACCE_QFRF_DMA;
> +		break;
> +
> +	case UACCE_QFRT_DKO:
> +		flags = UACCE_QFRF_MAP | UACCE_QFRF_KMAP;
> +
> +		if (uacce->flags & UACCE_DEV_NOIOMMU)
> +			flags |= UACCE_QFRF_DMA;
> +		break;
> +
> +	case UACCE_QFRT_DUS:
> +		if ((uacce->flags & UACCE_DEV_NOIOMMU) ||
> +		    (uacce->flags & UACCE_DEV_PASID)) {
> +			flags = UACCE_QFRF_SELFMT;
> +			break;
> +		}
> +
> +		flags = UACCE_QFRF_MAP | UACCE_QFRF_MMAP;
> +		break;
> +
> +	default:
> +		WARN_ON(&uacce->dev);
> +		break;
> +	}
> +
> +	qfr = uacce_create_region(q, vma, type, flags);
> +	if (IS_ERR(qfr)) {
> +		ret = PTR_ERR(qfr);
> +		goto out_with_lock;
> +	}
> +	q->qfrs[type] = qfr;
> +
> +	if (type == UACCE_QFRT_SS) {
> +		INIT_LIST_HEAD(&qfr->qs);
> +		list_add(&q->list, &q->qfrs[type]->qs);
> +	}
> +
> +	uacce_qs_wunlock();
> +
> +	if (qfr->pages)
> +		q->mm->data_vm += qfr->nr_pages;
> +
> +	return 0;
> +
> +out_with_lock:
> +	uacce_qs_wunlock();
> +	return ret;
> +}
> +
> +static __poll_t uacce_fops_poll(struct file *file, poll_table *wait)
> +{
> +	struct uacce_queue *q = (struct uacce_queue *)file->private_data;
Private data is a void * so no need to cast explicitly.

	struct uacce_queue *q = file->private_data;

> +	struct uacce *uacce = q->uacce;
> +
> +	poll_wait(file, &q->wait, wait);
> +	if (uacce->ops->is_q_updated && uacce->ops->is_q_updated(q))
> +		return EPOLLIN | EPOLLRDNORM;
> +
> +	return 0;
> +}
> +
> +static const struct file_operations uacce_fops = {
> +	.owner		= THIS_MODULE,
> +	.open		= uacce_fops_open,
> +	.release	= uacce_fops_release,
> +	.unlocked_ioctl	= uacce_fops_unl_ioctl,
> +#ifdef CONFIG_COMPAT
> +	.compat_ioctl	= uacce_fops_compat_ioctl,
> +#endif
> +	.mmap		= uacce_fops_mmap,
> +	.poll		= uacce_fops_poll,
> +};
> +
> +#define UACCE_FROM_CDEV_ATTR(dev) container_of(dev, struct uacce, dev)
> +
> +static ssize_t id_show(struct device *dev,
> +				 struct device_attribute *attr, char *buf)
> +{
> +	struct uacce *uacce = UACCE_FROM_CDEV_ATTR(dev);
> +
> +	return sprintf(buf, "%d\n", uacce->dev_id);

> +}
> +
> +static ssize_t api_show(struct device *dev,
> +				  struct device_attribute *attr, char *buf)
> +{
> +	struct uacce *uacce = UACCE_FROM_CDEV_ATTR(dev);
> +
> +	return sprintf(buf, "%s\n", uacce->api_ver);
> +}
> +
> +static ssize_t numa_distance_show(struct device *dev,
> +					    struct device_attribute *attr,
> +					    char *buf)
> +{
> +	struct uacce *uacce = UACCE_FROM_CDEV_ATTR(dev);
> +	int distance = 0;
> +
> +#ifdef CONFIG_NUMA
> +	distance = cpu_to_node(smp_processor_id()) - uacce->pdev->numa_node;

What is this function supposed to return? 
It currently returns the absolute difference in node number.
I suppose if that is 0, then it means local node, but all other values
have no sensible meaning.

Perhaps use node_distance()? That should give you the SLIT distance
so 10 for local and bigger for everything else.


> +#endif
> +	return sprintf(buf, "%d\n", abs(distance));
> +}
> +
> +static ssize_t node_id_show(struct device *dev,
> +				      struct device_attribute *attr,
> +				      char *buf)
> +{
> +	struct uacce *uacce = UACCE_FROM_CDEV_ATTR(dev);
> +	int node_id = -1;
> +
> +#ifdef CONFIG_NUMA
> +	node_id = uacce->pdev->numa_node;

use dev_to_node(uacce->pdev) which already does this protection for you.

> +#endif
> +	return sprintf(buf, "%d\n", node_id);
> +}
> +
> +static ssize_t flags_show(struct device *dev,
> +				       struct device_attribute *attr,
> +				       char *buf)
> +{
> +	struct uacce *uacce = UACCE_FROM_CDEV_ATTR(dev);
> +
> +	return sprintf(buf, "%d\n", uacce->flags);
> +}
> +
> +static ssize_t available_instances_show(struct device *dev,
> +					  struct device_attribute *attr,
> +						  char *buf)
> +{
> +	struct uacce *uacce = UACCE_FROM_CDEV_ATTR(dev);
> +
> +	return sprintf(buf, "%d\n", uacce->ops->get_available_instances(uacce));
> +}
> +
> +static ssize_t algorithms_show(struct device *dev,
> +					 struct device_attribute *attr,
> +					 char *buf)
> +{
> +	struct uacce *uacce = UACCE_FROM_CDEV_ATTR(dev);
> +
> +	return sprintf(buf, "%s", uacce->algs);
> +}
> +
> +static ssize_t qfrs_offset_show(struct device *dev,
> +					  struct device_attribute *attr,
> +					  char *buf)
> +{
> +	struct uacce *uacce = UACCE_FROM_CDEV_ATTR(dev);
> +	int i, ret;
> +	unsigned long offset;
> +
> +	for (i = 0, ret = 0; i < UACCE_QFRT_MAX; i++) {
> +		offset = uacce->qf_pg_start[i];
> +		if (offset != UACCE_QFR_NA)
> +			offset = offset << PAGE_SHIFT;
> +		if (i == UACCE_QFRT_SS)
> +			break;
> +		ret += sprintf(buf + ret, "%lu\t", offset);
> +	}
> +	ret += sprintf(buf + ret, "%lu\n", offset);
> +
> +	return ret;
> +}
> +
> +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_offset);
> +
> +static struct attribute *uacce_dev_attrs[] = {
New ABI. All needs documenting in

Documentation/ABI/

> +	&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_offset.attr,
> +	NULL,
> +};
> +
> +static const struct attribute_group uacce_dev_attr_group = {
> +	.name	= UACCE_DEV_ATTRS,
> +	.attrs	= uacce_dev_attrs,
> +};
> +
> +static const struct attribute_group *uacce_dev_attr_groups[] = {
> +	&uacce_dev_attr_group,
> +	NULL
> +};
> +
> +static int uacce_create_chrdev(struct uacce *uacce)
> +{
> +	int ret;
> +
> +	ret = idr_alloc(&uacce_idr, uacce, 0, 0, GFP_KERNEL);
> +	if (ret < 0)
> +		return ret;
> +
> +	cdev_init(&uacce->cdev, &uacce_fops);
> +	uacce->dev_id = ret;
> +	uacce->cdev.owner = THIS_MODULE;
> +	device_initialize(&uacce->dev);
> +	uacce->dev.devt = MKDEV(MAJOR(uacce_devt), uacce->dev_id);
> +	uacce->dev.class = uacce_class;
> +	uacce->dev.groups = uacce_dev_attr_groups;
> +	uacce->dev.parent = uacce->pdev;
> +	dev_set_name(&uacce->dev, "%s-%d", uacce->drv_name, uacce->dev_id);
> +	ret = cdev_device_add(&uacce->cdev, &uacce->dev);
> +	if (ret)
> +		goto err_with_idr;
> +
> +	dev_dbg(&uacce->dev, "create uacce minior=%d\n", uacce->dev_id);
> +	return 0;
> +
> +err_with_idr:
> +	idr_remove(&uacce_idr, uacce->dev_id);
> +	return ret;
> +}
> +
> +static void uacce_destroy_chrdev(struct uacce *uacce)
> +{
> +	cdev_device_del(&uacce->cdev, &uacce->dev);
> +	idr_remove(&uacce_idr, uacce->dev_id);
> +}
> +
> +static int uacce_default_get_available_instances(struct uacce *uacce)
> +{

Does this one ever make sense for a real device?

> +	return -1;
> +}
> +
> +static int uacce_default_start_queue(struct uacce_queue *q)
> +{
> +	dev_dbg(&q->uacce->dev, "fake start queue");

Does this ever make sense on a real device?

> +	return 0;
> +}
> +
> +static int uacce_dev_match(struct device *dev, void *data)
> +{
> +	if (dev->parent == data)
> +		return -EBUSY;
> +
> +	return 0;
> +}
> +
> +/* Borrowed from VFIO to fix msi translation */
> +static bool uacce_iommu_has_sw_msi(struct iommu_group *group,
> +				   phys_addr_t *base)
> +{
> +	struct list_head group_resv_regions;
> +	struct iommu_resv_region *region, *next;
> +	bool ret = false;
> +
> +	INIT_LIST_HEAD(&group_resv_regions);
> +	iommu_get_group_resv_regions(group, &group_resv_regions);
> +	list_for_each_entry(region, &group_resv_regions, list) {
> +		pr_debug("uacce: find a resv region (%d) on %llx\n",
> +			 region->type, region->start);
> +
> +		/*
> +		 * The presence of any 'real' MSI regions should take
> +		 * precedence over the software-managed one if the
> +		 * IOMMU driver happens to advertise both types.
> +		 */
> +		if (region->type == IOMMU_RESV_MSI) {
> +			ret = false;
> +			break;
> +		}
> +
> +		if (region->type == IOMMU_RESV_SW_MSI) {
> +			*base = region->start;
> +			ret = true;
> +		}
> +	}
> +	list_for_each_entry_safe(region, next, &group_resv_regions, list)
> +		kfree(region);
> +	return ret;
> +}
> +
> +static int uacce_set_iommu_domain(struct uacce *uacce)
> +{
> +	struct iommu_domain *domain;
> +	struct iommu_group *group;
> +	struct device *dev = uacce->pdev;
> +	bool resv_msi;
> +	phys_addr_t resv_msi_base = 0;
> +	int ret;
> +
> +	if ((uacce->flags & UACCE_DEV_NOIOMMU) ||
> +	    (uacce->flags & UACCE_DEV_PASID))
> +		return 0;
> +
> +	/*
> +	 * We don't support multiple register for the same dev in RFC version ,
> +	 * will add it in formal version

So this effectively multiple complete uacce interfaces for one device.
Is there a known usecase for that?

> +	 */
> +	ret = class_for_each_device(uacce_class, NULL, uacce->pdev,
> +				    uacce_dev_match);
> +	if (ret)
> +		return ret;
> +
> +	/* allocate and attach a unmanged domain */
> +	domain = iommu_domain_alloc(uacce->pdev->bus);
> +	if (!domain) {
> +		dev_dbg(&uacce->dev, "cannot get domain for iommu\n");
> +		return -ENODEV;
> +	}
> +
> +	ret = iommu_attach_device(domain, uacce->pdev);
> +	if (ret)
> +		goto err_with_domain;
> +
> +	if (iommu_capable(dev->bus, IOMMU_CAP_CACHE_COHERENCY)) {
> +		uacce->prot |= IOMMU_CACHE;
> +		dev_dbg(dev, "Enable uacce with c-coherent capa\n");
> +	} else
> +		dev_dbg(dev, "Enable uacce without c-coherent capa\n");

Do those debug statements add anything?  I'd also like a comment to explain
why we care here.

> +
> +	group = iommu_group_get(dev);
> +	if (!group) {
> +		ret = -EINVAL;
> +		goto err_with_domain;
> +	}
> +
> +	resv_msi = uacce_iommu_has_sw_msi(group, &resv_msi_base);
> +	iommu_group_put(group);
> +
> +	if (resv_msi) {
> +		if (!irq_domain_check_msi_remap() &&
> +		    !iommu_capable(dev->bus, IOMMU_CAP_INTR_REMAP)) {
> +			dev_warn(dev, "No interrupt remapping support!");
> +			ret = -EPERM;
> +			goto err_with_domain;
> +		}
> +
> +		dev_dbg(dev, "Set resv msi %llx on iommu domain\n",
> +			(u64)resv_msi_base);
> +		ret = iommu_get_msi_cookie(domain, resv_msi_base);
> +		if (ret)
> +			goto err_with_domain;
> +	}
> +
> +	return 0;
> +
> +err_with_domain:
> +	iommu_domain_free(domain);
> +	return ret;
> +}
> +
> +static void uacce_unset_iommu_domain(struct uacce *uacce)
> +{
> +	struct iommu_domain *domain;
> +
> +	if ((uacce->flags & UACCE_DEV_NOIOMMU) ||
> +	    (uacce->flags & UACCE_DEV_PASID))
> +		return;
> +
> +	domain = iommu_get_domain_for_dev(uacce->pdev);
> +	if (domain) {
> +		iommu_detach_device(domain, uacce->pdev);
> +		iommu_domain_free(domain);
> +	} else
> +		dev_err(&uacce->dev, "bug: no domain attached to device\n");

Given this is an error path, perhaps the following flow is easier to read (slightly)

	if (!domain) {
		dev_err(&uacce->dev, "bug: no domain attached to device\n");
		return;
	}

	iommu_detach_device(domain, uacce->pdev);
	iommu_domain_free(domain);
}

> +}
> +
> +/**
> + *	uacce_register - register an accelerator
> + *	@uacce: the accelerator structure
> + */
> +int uacce_register(struct uacce *uacce)
> +{
> +	int ret;
> +
> +	if (!uacce->pdev) {
> +		pr_debug("uacce parent device not set\n");
> +		return -ENODEV;
> +	}
> +
> +	if (uacce->flags & UACCE_DEV_NOIOMMU) {
> +		add_taint(TAINT_CRAP, LOCKDEP_STILL_OK);
> +		dev_warn(uacce->pdev,
> +			 "Register to noiommu mode, which export kernel data to user space and may vulnerable to attack");

Greg already covered this one ;)

> +	}
> +
> +	/* if dev support fault-from-dev, it should support pasid */
> +	if ((uacce->flags & UACCE_DEV_FAULT_FROM_DEV) &&
> +	    !(uacce->flags & UACCE_DEV_PASID)) {
> +		dev_warn(&uacce->dev, "SVM/SAV device should support PASID\n");
> +		return -EINVAL;
> +	}
> +
> +	if (!uacce->ops->start_queue)
> +		uacce->ops->start_queue = uacce_default_start_queue;
> +
> +	if (!uacce->ops->get_available_instances)
> +		uacce->ops->get_available_instances =
> +			uacce_default_get_available_instances;
> +
> +#ifdef CONFIG_IOMMU_SVA
> +	if (uacce->flags & UACCE_DEV_PASID) {
> +		ret = iommu_dev_enable_feature(uacce->pdev, IOMMU_DEV_FEAT_SVA);
> +		if (ret)
> +			uacce->flags &= ~(UACCE_DEV_FAULT_FROM_DEV |
> +					  UACCE_DEV_PASID);
> +	}
> +#endif
> +
> +	ret = uacce_set_iommu_domain(uacce);
> +	if (ret)
> +		return ret;
> +
> +	mutex_lock(&uacce_mutex);
> +
> +	ret = uacce_create_chrdev(uacce);
> +	if (ret)
> +		goto err_with_lock;
> +
> +	dev_dbg(&uacce->dev, "uacce state initialized to INIT");

Not sure that tells us much of interest.  Probably clean out most of the
dev_dbg statements. Useful when developing but once it works they
add lines of code that don't do anything useful.

> +	atomic_set(&uacce->state, UACCE_ST_INIT);
> +	INIT_LIST_HEAD(&uacce->qs);
> +	mutex_init(&uacce->q_lock);
> +	mutex_unlock(&uacce_mutex);
> +

One blank line is almost always enough.

> +
> +	return 0;
> +
> +err_with_lock:
> +	mutex_unlock(&uacce_mutex);
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(uacce_register);
> +
> +/**
> + * uacce_unregister - unregisters a uacce
> + * @uacce: the accelerator to unregister
> + *
> + * Unregister an accelerator that wat previously successully registered with
> + * uacce_register().
> + */
> +void uacce_unregister(struct uacce *uacce)
> +{
> +	mutex_lock(&uacce_mutex);
> +
> +#ifdef CONFIG_IOMMU_SVA
> +	if (uacce->flags & UACCE_DEV_PASID)
> +		iommu_dev_disable_feature(uacce->pdev, IOMMU_DEV_FEAT_SVA);
> +#endif
> +	uacce_unset_iommu_domain(uacce);
> +
> +	uacce_destroy_chrdev(uacce);
> +
> +	mutex_unlock(&uacce_mutex);
> +}
> +EXPORT_SYMBOL_GPL(uacce_unregister);
> +
> +static int __init uacce_init(void)
> +{
> +	int ret;
> +
> +	uacce_class = class_create(THIS_MODULE, UACCE_CLASS_NAME);
> +	if (IS_ERR(uacce_class)) {
> +		ret = PTR_ERR(uacce_class);
> +		goto err;
> +	}
> +
> +	ret = alloc_chrdev_region(&uacce_devt, 0, MINORMASK, "uacce");
> +	if (ret)
> +		goto err_with_class;
> +
> +	pr_info("uacce init with major number:%d\n", MAJOR(uacce_devt));
> +
> +	return 0;
> +
> +err_with_class:
> +	class_destroy(uacce_class);
> +err:
> +	return ret;
> +}
> +
> +static __exit void uacce_exit(void)
> +{
> +	unregister_chrdev_region(uacce_devt, MINORMASK);
> +	class_destroy(uacce_class);
> +	idr_destroy(&uacce_idr);
> +}
> +
> +subsys_initcall(uacce_init);
> +module_exit(uacce_exit);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Hisilicon Tech. Co., Ltd.");
> +MODULE_DESCRIPTION("Accelerator interface for Userland applications");
> diff --git a/include/linux/uacce.h b/include/linux/uacce.h
> new file mode 100644
> index 0000000..fe2f6f4
> --- /dev/null
> +++ b/include/linux/uacce.h
> @@ -0,0 +1,109 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +#ifndef __UACCE_H
> +#define __UACCE_H
> +
> +#include <linux/cdev.h>
> +#include <linux/device.h>
> +#include <linux/fs.h>
> +#include <linux/list.h>
> +#include <linux/iommu.h>
> +#include <uapi/misc/uacce.h>
> +
> +struct uacce_queue;
> +struct uacce;
> +
> +/* uacce mode of the driver */
> +#define UACCE_MODE_NOUACCE	0 /* don't use uacce */
> +#define UACCE_MODE_UACCE	1 /* use uacce exclusively */
> +#define UACCE_MODE_NOIOMMU	2 /* use uacce noiommu mode */
> +
> +#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_qfile_region {

I'd like to see kernel doc for all the structures in here.

> +	enum uacce_qfrt type;
> +	unsigned long iova;	/* iova share between user and device space */
> +	struct page **pages;
> +	int nr_pages;
> +	int prot;
> +	unsigned int flags;
> +	struct list_head qs;	/* qs sharing the same region, for ss */
> +	void *kaddr;		/* kernel addr */
> +	dma_addr_t dma;		/* dma address, if created by dma api */
> +};
> +
> +/**
> + * struct uacce_ops - WD device operations

Make sure you don't miss out elements in the docs.

get_available_instances.

Easy to check, just build the kernel doc.

> + * @get_queue: get a queue from the device according to algorithm

Not obvious in this case what 'according to algorithm' is referring to as
the function takes simply "unsigned long arg"

> + * @put_queue: free a queue to the device
> + * @start_queue: make the queue start work after get_queue
> + * @stop_queue: make the queue stop work before put_queue
> + * @is_q_updated: check whether the task is finished
> + * @mask_notify: mask the task irq of queue
> + * @mmap: mmap addresses of queue to user space
> + * @reset: reset the WD device

uacce device?

> + * @reset_queue: reset the queue
> + * @ioctl:   ioctl for user space users of the queue

Extra spaces after : compared to other entries.

> + */
> +struct uacce_ops {
> +	int (*get_available_instances)(struct uacce *uacce);
> +	int (*get_queue)(struct uacce *uacce, unsigned long arg,
> +	     struct uacce_queue **q);
> +	void (*put_queue)(struct uacce_queue *q);
> +	int (*start_queue)(struct uacce_queue *q);
> +	void (*stop_queue)(struct uacce_queue *q);
> +	int (*is_q_updated)(struct uacce_queue *q);
> +	void (*mask_notify)(struct uacce_queue *q, int event_mask);
> +	int (*mmap)(struct uacce_queue *q, struct vm_area_struct *vma,
> +		    struct uacce_qfile_region *qfr);
> +	int (*reset)(struct uacce *uacce);
> +	int (*reset_queue)(struct uacce_queue *q);
> +	long (*ioctl)(struct uacce_queue *q, unsigned int cmd,
> +		      unsigned long arg);
> +};
> +
> +struct uacce_queue {
> +	struct uacce *uacce;
> +	void *priv;
> +	wait_queue_head_t wait;
> +	int pasid;
> +	struct iommu_sva *handle;
> +	struct list_head list; /* share list for qfr->qs */
> +	struct mm_struct *mm;
> +	struct uacce_qfile_region *qfrs[UACCE_QFRT_MAX];
> +	struct list_head q_dev;
> +};
> +
> +#define UACCE_ST_INIT		0
> +#define UACCE_ST_OPENED		1
> +#define UACCE_ST_STARTED	2
> +#define UACCE_ST_RST		3

These seem to be states in a state machine, perhaps an enum
is more suited as their actual values don't matter (I think!)

> +
> +struct uacce {
> +	const char *name;
> +	const char *drv_name;
> +	const char *algs;
> +	const char *api_ver;
> +	unsigned int flags;
> +	unsigned long qf_pg_start[UACCE_QFRT_MAX];
> +	struct uacce_ops *ops;
> +	struct device *pdev;
> +	bool is_vf;
> +	u32 dev_id;
> +	struct cdev cdev;
> +	struct device dev;
> +	void *priv;
> +	atomic_t state;
> +	int prot;
> +	struct mutex q_lock;
> +	struct list_head qs;
> +};
> +
> +int uacce_register(struct uacce *uacce);
> +void uacce_unregister(struct uacce *uacce);
> +void uacce_wake_up(struct uacce_queue *q);
> +
> +#endif
> diff --git a/include/uapi/misc/uacce.h b/include/uapi/misc/uacce.h
> new file mode 100644
> index 0000000..44a0a5d
> --- /dev/null
> +++ b/include/uapi/misc/uacce.h
> @@ -0,0 +1,44 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +#ifndef _UAPIUUACCE_H
> +#define _UAPIUUACCE_H
> +
> +#include <linux/types.h>
> +#include <linux/ioctl.h>
> +
> +#define UACCE_CLASS_NAME	"uacce"
> +#define UACCE_DEV_ATTRS		"attrs"
> +#define UACCE_CMD_SHARE_SVAS	_IO('W', 0)
> +#define UACCE_CMD_START		_IO('W', 1)
> +#define UACCE_CMD_GET_SS_DMA	_IOR('W', 2, unsigned long)
> +
> +/**
> + * UACCE Device Attributes:
> + *
> + * NOIOMMU: the device has no IOMMU support
> + *	can do share sva, but no map to the dev
> + * PASID: the device has IOMMU which support PASID setting
> + *	can do share sva, mapped to dev per process
> + * FAULT_FROM_DEV: the device has IOMMU which can do page fault request
> + *	no need for share sva, should be used with PASID
> + * SVA: full function device
> + * SHARE_DOMAIN: no PASID, can do share sva only for one process and the kernel
> + */
> +#define UACCE_DEV_NOIOMMU		(1 << 0)
> +#define UACCE_DEV_PASID			(1 << 1)
> +#define UACCE_DEV_FAULT_FROM_DEV	(1 << 2)
> +#define UACCE_DEV_SVA		(UACCE_DEV_PASID | UACCE_DEV_FAULT_FROM_DEV)
> +#define UACCE_DEV_SHARE_DOMAIN	(0)
> +
> +#define UACCE_API_VER_NOIOMMU_SUBFIX	"_noiommu"
> +
> +#define UACCE_QFR_NA ((unsigned long)-1)
> +enum uacce_qfrt {
> +	UACCE_QFRT_MMIO = 0,	/* device mmio region */
> +	UACCE_QFRT_DKO,		/* device kernel-only */
> +	UACCE_QFRT_DUS,		/* device user share */
> +	UACCE_QFRT_SS,		/* static share memory */
> +	UACCE_QFRT_MAX,
> +};
> +#define UACCE_QFRT_INVALID UACCE_QFRT_MAX
> +
> +#endif

Re: [PATCH 0/2] A General Accelerator Framework, WarpDrive

[Jerome Glisse]

On Wed, Aug 14, 2019 at 05:34:23PM +0800, Zhangfei Gao wrote:
> *WarpDrive* is a general accelerator framework for the user application to
> access the hardware without going through the kernel in data path.
> 
> WarpDrive is the name for the whole framework. The component in kernel
> is called uacce, meaning "Unified/User-space-access-intended Accelerator
> Framework". It makes use of the capability of IOMMU to maintain a
> unified virtual address space between the hardware and the process.
> 
> WarpDrive is intended to be used with Jean Philippe Brucker's SVA
> patchset[1], which enables IO side page fault and PASID support. 
> We have keep verifying with Jean's sva/current [2]
> We also keep verifying with Eric's SMMUv3 Nested Stage patch [3]
> 
> This series and related zip & qm driver as well as dummy driver for qemu test:
> https://github.com/Linaro/linux-kernel-warpdrive/tree/5.3-rc1-warpdrive-v1
> zip driver already been upstreamed.
> zip supporting uacce will be the next step.
> 
> The library and user application:
> https://github.com/Linaro/warpdrive/tree/wdprd-v1-current

Do we want a new framework ? I think that is the first question that
should be answer here. Accelerator are in many forms and so far they
never have been enough commonality to create a framework, even GPUs
with the drm is an example of that, drm only offer share framework
for the modesetting part of the GPU (as thankfully monitor connector
are not specific to GPU brands :))

FPGA is another example the only common code expose to userspace is
about bitstream management AFAIK.

I would argue that a framework should only be created once there is
enough devices with same userspace API. Meanwhile you can provide
in kernel helper that allow driver to expose same API. If after a
while we have enough device driver which all use that same in kernel
helpers API then it will a good time to introduce a new framework.
Meanwhile this will allow individual device driver to tinker with
their API and maybe get to something useful to more devices in the
end.

Note that what i propose also allow userspace code sharing for all
driver that use the same in kernel helper.

Cheers,
Jérôme

Re: [PATCH 2/2] uacce: add uacce module

[Greg Kroah-Hartman]

On Mon, Aug 19, 2019 at 05:09:23PM +0800, zhangfei.gao@foxmail.com wrote:
> Hi, Greg
> 
> Thanks for your kind suggestion.
> 
> On 2019/8/15 下午10:12, Greg Kroah-Hartman wrote:
> > On Wed, Aug 14, 2019 at 05:34:25PM +0800, Zhangfei Gao wrote:
> > > diff --git a/include/uapi/misc/uacce.h b/include/uapi/misc/uacce.h
> > > new file mode 100644
> > > index 0000000..44a0a5d
> > > --- /dev/null
> > > +++ b/include/uapi/misc/uacce.h
> > > @@ -0,0 +1,44 @@
> > > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > > +#ifndef _UAPIUUACCE_H
> > > +#define _UAPIUUACCE_H
> > > +
> > > +#include <linux/types.h>
> > > +#include <linux/ioctl.h>
> > > +
> > > +#define UACCE_CLASS_NAME	"uacce"
> > Why is this in a uapi file?
> User app need get attribute from /sys/class,
> For example: /sys/class/uacce/hisi_zip-0/xxx
> UACCE_CLASS_NAME here tells app which subsystem to open,
> /sys/class/subsystem/

But that never comes from a uapi file.  No other subsystem does this.

thanks,

greg k-h

Re: [PATCH 2/2] uacce: add uacce module

[Greg Kroah-Hartman]

On Mon, Aug 19, 2019 at 05:43:40PM +0800, zhangfei.gao@foxmail.com wrote:
> Hi, Greg
> 
> On 2019/8/15 下午10:20, Greg Kroah-Hartman wrote:
> > On Wed, Aug 14, 2019 at 05:34:25PM +0800, Zhangfei Gao wrote:
> > > +/* lock to protect all queues management */
> > > +static DECLARE_RWSEM(uacce_qs_lock);
> > > +#define uacce_qs_rlock() down_read(&uacce_qs_lock)
> > > +#define uacce_qs_runlock() up_read(&uacce_qs_lock)
> > > +#define uacce_qs_wlock() down_write(&uacce_qs_lock)
> > > +#define uacce_qs_wunlock() up_write(&uacce_qs_lock)
> > Do not define your own locking macros.  That makes the code impossible
> > to review.
> > 
> > And are you _sure_ you need a rw lock?  You have benchmarked where it
> > has a performance impact?
> OK, will use uacce_mutex for this first version, and put performance tunning
> later.
> > > +/**
> > > + * uacce_wake_up - Wake up the process who is waiting this queue
> > > + * @q the accelerator queue to wake up
> > > + */
> > > +void uacce_wake_up(struct uacce_queue *q)
> > > +{
> > > +	dev_dbg(&q->uacce->dev, "wake up\n");
> > ftrace is your friend, no need for any such logging lines anywhere in
> > these files.
> OK, will remove the dev_dbg.
> > > +	wake_up_interruptible(&q->wait);
> > > +}
> > > +EXPORT_SYMBOL_GPL(uacce_wake_up);
> > ...
> > 
> > > +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_offset.attr,
> > > +	NULL,
> > > +};
> > > +
> > > +static const struct attribute_group uacce_dev_attr_group = {
> > > +	.name	= UACCE_DEV_ATTRS,
> > > +	.attrs	= uacce_dev_attrs,
> > > +};
> > Why is your attribute group in a subdirectory?  Why not in the "normal"
> > class directory?
> Yes,  .name = UACCE_DEV_ATTRS can be removed to make it simple.
> Then attrs are in /sys/class/uacce/hisi_zip-x/xxx
> > 
> > You are adding sysfs files to the kernel without any Documentation/ABI/
> > entries, which is a requirement.  Please fix that up for the next time
> > you send these.
> Will add Documentation/ABI/entries in next version.
> > > +static const struct attribute_group *uacce_dev_attr_groups[] = {
> > > +	&uacce_dev_attr_group,
> > > +	NULL
> > > +};
> > > +
> > > +static int uacce_create_chrdev(struct uacce *uacce)
> > > +{
> > > +	int ret;
> > > +
> > > +	ret = idr_alloc(&uacce_idr, uacce, 0, 0, GFP_KERNEL);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > Shouldn't this function create the memory needed for this structure?
> > You are relying ont he caller to do it for you, why?
> I think you mean uacce structure here.
> Yes, currently we count on caller to prepare uacce structure and call
> uacce_register(uacce).
> We still think this method is simpler, prepare uacce, register uacce.
> And there are other system using the same method, like crypto
> (crypto_register_acomp), nand, etc.

crypto is not a subsystem to ever try to emulate :)

You are creating a structure with a lifetime that you control, don't
have someone else create your memory, that's almost never what you want
to do.  Most all driver subsystems create their own memory chunks for
what they need to do, it's a much better pattern.

Especially when you get into pointer lifetime issues...

> > > +	cdev_init(&uacce->cdev, &uacce_fops);
> > > +	uacce->dev_id = ret;
> > > +	uacce->cdev.owner = THIS_MODULE;
> > > +	device_initialize(&uacce->dev);
> > > +	uacce->dev.devt = MKDEV(MAJOR(uacce_devt), uacce->dev_id);
> > > +	uacce->dev.class = uacce_class;
> > > +	uacce->dev.groups = uacce_dev_attr_groups;
> > > +	uacce->dev.parent = uacce->pdev;
> > > +	dev_set_name(&uacce->dev, "%s-%d", uacce->drv_name, uacce->dev_id);
> > > +	ret = cdev_device_add(&uacce->cdev, &uacce->dev);
> > > +	if (ret)
> > > +		goto err_with_idr;
> > > +
> > > +	dev_dbg(&uacce->dev, "create uacce minior=%d\n", uacce->dev_id);
> > > +	return 0;
> > > +
> > > +err_with_idr:
> > > +	idr_remove(&uacce_idr, uacce->dev_id);
> > > +	return ret;
> > > +}
> > > +
> > > +static void uacce_destroy_chrdev(struct uacce *uacce)
> > > +{
> > > +	cdev_device_del(&uacce->cdev, &uacce->dev);
> > > +	idr_remove(&uacce_idr, uacce->dev_id);
> > > +}
> > > +
> > > +static int uacce_default_get_available_instances(struct uacce *uacce)
> > > +{
> > > +	return -1;
> > Do not make up error values, use the proper -EXXXX value instead.
> > 
> > > +}
> > > +
> > > +static int uacce_default_start_queue(struct uacce_queue *q)
> > > +{
> > > +	dev_dbg(&q->uacce->dev, "fake start queue");
> > > +	return 0;
> > Why even have this function if you do not do anything in it?
> OK, will remove these two funcs.
> > > +}
> > > +
> > > +static int uacce_dev_match(struct device *dev, void *data)
> > > +{
> > > +	if (dev->parent == data)
> > > +		return -EBUSY;
> > There should be in-kernel functions for this now, no need for you to
> > roll your own.
> Sorry, do not find this function.
> Only find class_find_device, which still require match.

It is in linux-next, look there...

thanks,

greg k-h

Re: [PATCH 2/2] uacce: add uacce module

[zhangfei]

Hi, Greg

On 2019/8/19 下午6:24, Greg Kroah-Hartman wrote:
>>>> +static int uacce_create_chrdev(struct uacce *uacce)
>>>> +{
>>>> +	int ret;
>>>> +
>>>> +	ret = idr_alloc(&uacce_idr, uacce, 0, 0, GFP_KERNEL);
>>>> +	if (ret < 0)
>>>> +		return ret;
>>>> +
>>> Shouldn't this function create the memory needed for this structure?
>>> You are relying ont he caller to do it for you, why?
>> I think you mean uacce structure here.
>> Yes, currently we count on caller to prepare uacce structure and call
>> uacce_register(uacce).
>> We still think this method is simpler, prepare uacce, register uacce.
>> And there are other system using the same method, like crypto
>> (crypto_register_acomp), nand, etc.
> crypto is not a subsystem to ever try to emulate :)
>
> You are creating a structure with a lifetime that you control, don't
> have someone else create your memory, that's almost never what you want
> to do.  Most all driver subsystems create their own memory chunks for
> what they need to do, it's a much better pattern.
>
> Especially when you get into pointer lifetime issues...
OK, understand now, thanks for your patience.
will use this instead.
struct uacce_interface {
         char name[32];
         unsigned int flags;
         struct uacce_ops *ops;
};
struct uacce *uacce_register(struct device *dev, struct uacce_interface 
*interface);
>>>> +}
>>>> +
>>>> +static int uacce_dev_match(struct device *dev, void *data)
>>>> +{
>>>> +	if (dev->parent == data)
>>>> +		return -EBUSY;
>>> There should be in-kernel functions for this now, no need for you to
>>> roll your own.
>> Sorry, do not find this function.
>> Only find class_find_device, which still require match.
> It is in linux-next, look there...
>
Suppose you mean the funcs: device_match_name, 
device_match_of_node,device_match_devt etc.
Here we need dev->parent, there still no such func.

Thanks

Re: [PATCH 2/2] uacce: add uacce module

[zhangfei]

On 2019/8/19 下午6:22, Greg Kroah-Hartman wrote:
> On Mon, Aug 19, 2019 at 05:09:23PM +0800, zhangfei.gao@foxmail.com wrote:
>> Hi, Greg
>>
>> Thanks for your kind suggestion.
>>
>> On 2019/8/15 下午10:12, Greg Kroah-Hartman wrote:
>>> On Wed, Aug 14, 2019 at 05:34:25PM +0800, Zhangfei Gao wrote:
>>>> diff --git a/include/uapi/misc/uacce.h b/include/uapi/misc/uacce.h
>>>> new file mode 100644
>>>> index 0000000..44a0a5d
>>>> --- /dev/null
>>>> +++ b/include/uapi/misc/uacce.h
>>>> @@ -0,0 +1,44 @@
>>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>>>> +#ifndef _UAPIUUACCE_H
>>>> +#define _UAPIUUACCE_H
>>>> +
>>>> +#include <linux/types.h>
>>>> +#include <linux/ioctl.h>
>>>> +
>>>> +#define UACCE_CLASS_NAME	"uacce"
>>> Why is this in a uapi file?
>> User app need get attribute from /sys/class,
>> For example: /sys/class/uacce/hisi_zip-0/xxx
>> UACCE_CLASS_NAME here tells app which subsystem to open,
>> /sys/class/subsystem/
> But that never comes from a uapi file.  No other subsystem does this.
OK, got it
Maybe the entry info can come from Documentation/ABI/entries

Thanks

Re: [PATCH 2/2] uacce: add uacce module

[zhangfei]

On 2019/8/15 下午10:13, Greg Kroah-Hartman wrote:
> On Wed, Aug 14, 2019 at 05:34:25PM +0800, Zhangfei Gao wrote:
>> +int uacce_register(struct uacce *uacce)
>> +{
>> +	int ret;
>> +
>> +	if (!uacce->pdev) {
>> +		pr_debug("uacce parent device not set\n");
>> +		return -ENODEV;
>> +	}
>> +
>> +	if (uacce->flags & UACCE_DEV_NOIOMMU) {
>> +		add_taint(TAINT_CRAP, LOCKDEP_STILL_OK);
>> +		dev_warn(uacce->pdev,
>> +			 "Register to noiommu mode, which export kernel data to user space and may vulnerable to attack");
>> +	}
> THat is odd, why even offer this feature then if it is a major issue?
UACCE_DEV_NOIOMMU maybe confusing here.

In this mode, app use ioctl to get dma_handle from dma_alloc_coherent.

It does not matter iommu is enabled or not.
In case iommu is disabled, it maybe dangerous to kernel, so we added warning here, is it required?


We support two modes.
1. support sva, (va = iova)
a. If pasid is supported, multi-process can be supported.
b. If pasid is not supported, multi-process can NOT be supported.
We borrowed va from user space to achieve a single virtual address system.


2. Can not support sva, iova != va,
Here user app get dma_handle from dma_alloc_coherent via ioctl.
We need this mode for two reasons:
1. This mode can support multi-process, to support openssl etc.
2. Some accelerators (crypto like zip, etc) can work without sva, just prepare data and kick start.

Thanks

Re: [PATCH 0/2] A General Accelerator Framework, WarpDrive

[zhangfei]

Hi, Jerome

Thanks for your suggestion

On 2019/8/16 上午1:04, Jerome Glisse wrote:
> On Wed, Aug 14, 2019 at 05:34:23PM +0800, Zhangfei Gao wrote:
>> *WarpDrive* is a general accelerator framework for the user application to
>> access the hardware without going through the kernel in data path.
>>
>> WarpDrive is the name for the whole framework. The component in kernel
>> is called uacce, meaning "Unified/User-space-access-intended Accelerator
>> Framework". It makes use of the capability of IOMMU to maintain a
>> unified virtual address space between the hardware and the process.
>>
>> WarpDrive is intended to be used with Jean Philippe Brucker's SVA
>> patchset[1], which enables IO side page fault and PASID support.
>> We have keep verifying with Jean's sva/current [2]
>> We also keep verifying with Eric's SMMUv3 Nested Stage patch [3]
>>
>> This series and related zip & qm driver as well as dummy driver for qemu test:
>> https://github.com/Linaro/linux-kernel-warpdrive/tree/5.3-rc1-warpdrive-v1
>> zip driver already been upstreamed.
>> zip supporting uacce will be the next step.
>>
>> The library and user application:
>> https://github.com/Linaro/warpdrive/tree/wdprd-v1-current
> Do we want a new framework ? I think that is the first question that
> should be answer here. Accelerator are in many forms and so far they
> never have been enough commonality to create a framework, even GPUs
> with the drm is an example of that, drm only offer share framework
> for the modesetting part of the GPU (as thankfully monitor connector
> are not specific to GPU brands :))
>
> FPGA is another example the only common code expose to userspace is
> about bitstream management AFAIK.
>
> I would argue that a framework should only be created once there is
> enough devices with same userspace API. Meanwhile you can provide
> in kernel helper that allow driver to expose same API. If after a
> while we have enough device driver which all use that same in kernel
> helpers API then it will a good time to introduce a new framework.
> Meanwhile this will allow individual device driver to tinker with
> their API and maybe get to something useful to more devices in the
> end.
>
> Note that what i propose also allow userspace code sharing for all
> driver that use the same in kernel helper.
>
Yes, we understand it is not easy for a new framework.
There are discussions in rfc2 (2018/9) and rfc3 (2018/11).
To make life easier, we plan to move the uacce to driver/misc to support 
our own product first until it is mature.
Using uacce, Currently we get quite a big performance improvement in our 
crypto product, like zip, hpre, sec.
Our final goal is "A General Accelerator Framework", which maybe ambitious.
So uacce is designed to be a common framework, can be easily supporting 
more accelerators.
And we are happy to get more requirements and make it mature.

Another good point is SVA support in ongoing, http://jpbrucker.net/sva/
After sva mature, the accelerators support will be much easier.

Thanks

Re: [PATCH 2/2] uacce: add uacce module

[Greg Kroah-Hartman]

On Tue, Aug 20, 2019 at 08:38:46PM +0800, zhangfei wrote:
> 
> 
> On 2019/8/19 下午6:22, Greg Kroah-Hartman wrote:
> > On Mon, Aug 19, 2019 at 05:09:23PM +0800, zhangfei.gao@foxmail.com wrote:
> > > Hi, Greg
> > > 
> > > Thanks for your kind suggestion.
> > > 
> > > On 2019/8/15 下午10:12, Greg Kroah-Hartman wrote:
> > > > On Wed, Aug 14, 2019 at 05:34:25PM +0800, Zhangfei Gao wrote:
> > > > > diff --git a/include/uapi/misc/uacce.h b/include/uapi/misc/uacce.h
> > > > > new file mode 100644
> > > > > index 0000000..44a0a5d
> > > > > --- /dev/null
> > > > > +++ b/include/uapi/misc/uacce.h
> > > > > @@ -0,0 +1,44 @@
> > > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > > > > +#ifndef _UAPIUUACCE_H
> > > > > +#define _UAPIUUACCE_H
> > > > > +
> > > > > +#include <linux/types.h>
> > > > > +#include <linux/ioctl.h>
> > > > > +
> > > > > +#define UACCE_CLASS_NAME	"uacce"
> > > > Why is this in a uapi file?
> > > User app need get attribute from /sys/class,
> > > For example: /sys/class/uacce/hisi_zip-0/xxx
> > > UACCE_CLASS_NAME here tells app which subsystem to open,
> > > /sys/class/subsystem/
> > But that never comes from a uapi file.  No other subsystem does this.
> OK, got it
> Maybe the entry info can come from Documentation/ABI/entries

Yes it can, we now have a tool in the tree that automatically parses
those entries and outputs it in a variety of different formats.

thanks,

greg k-h

Re: [PATCH 2/2] uacce: add uacce module

[Greg Kroah-Hartman]

On Tue, Aug 20, 2019 at 08:36:50PM +0800, zhangfei wrote:
> Hi, Greg
> 
> On 2019/8/19 下午6:24, Greg Kroah-Hartman wrote:
> > > > > +static int uacce_create_chrdev(struct uacce *uacce)
> > > > > +{
> > > > > +	int ret;
> > > > > +
> > > > > +	ret = idr_alloc(&uacce_idr, uacce, 0, 0, GFP_KERNEL);
> > > > > +	if (ret < 0)
> > > > > +		return ret;
> > > > > +
> > > > Shouldn't this function create the memory needed for this structure?
> > > > You are relying ont he caller to do it for you, why?
> > > I think you mean uacce structure here.
> > > Yes, currently we count on caller to prepare uacce structure and call
> > > uacce_register(uacce).
> > > We still think this method is simpler, prepare uacce, register uacce.
> > > And there are other system using the same method, like crypto
> > > (crypto_register_acomp), nand, etc.
> > crypto is not a subsystem to ever try to emulate :)
> > 
> > You are creating a structure with a lifetime that you control, don't
> > have someone else create your memory, that's almost never what you want
> > to do.  Most all driver subsystems create their own memory chunks for
> > what they need to do, it's a much better pattern.
> > 
> > Especially when you get into pointer lifetime issues...
> OK, understand now, thanks for your patience.
> will use this instead.
> struct uacce_interface {
>         char name[32];
>         unsigned int flags;
>         struct uacce_ops *ops;
> };
> struct uacce *uacce_register(struct device *dev, struct uacce_interface
> *interface);

What?  Why do you need a structure?  A pointer to the name and the ops
should be all that is needed, right?

And 'dev' here is a pointer to the parent, right?  Might want to make
that explicit in the name of the variable :)

> > > > > +
> > > > > +static int uacce_dev_match(struct device *dev, void *data)
> > > > > +{
> > > > > +	if (dev->parent == data)
> > > > > +		return -EBUSY;
> > > > There should be in-kernel functions for this now, no need for you to
> > > > roll your own.
> > > Sorry, do not find this function.
> > > Only find class_find_device, which still require match.
> > It is in linux-next, look there...
> > 
> Suppose you mean the funcs: device_match_name,
> device_match_of_node,device_match_devt etc.
> Here we need dev->parent, there still no such func.

You should NEVER be matching on a parent.  If so, your use of the driver
model is wrong :)

Remind me to really review the use of the driver core code in your next
submission of this series please, I think it needs it.

thanks,

greg k-h

Re: [PATCH 2/2] uacce: add uacce module

[Greg Kroah-Hartman]

On Tue, Aug 20, 2019 at 09:08:55PM +0800, zhangfei wrote:
> 
> 
> On 2019/8/15 下午10:13, Greg Kroah-Hartman wrote:
> > On Wed, Aug 14, 2019 at 05:34:25PM +0800, Zhangfei Gao wrote:
> > > +int uacce_register(struct uacce *uacce)
> > > +{
> > > +	int ret;
> > > +
> > > +	if (!uacce->pdev) {
> > > +		pr_debug("uacce parent device not set\n");
> > > +		return -ENODEV;
> > > +	}
> > > +
> > > +	if (uacce->flags & UACCE_DEV_NOIOMMU) {
> > > +		add_taint(TAINT_CRAP, LOCKDEP_STILL_OK);
> > > +		dev_warn(uacce->pdev,
> > > +			 "Register to noiommu mode, which export kernel data to user space and may vulnerable to attack");
> > > +	}
> > THat is odd, why even offer this feature then if it is a major issue?
> UACCE_DEV_NOIOMMU maybe confusing here.
> 
> In this mode, app use ioctl to get dma_handle from dma_alloc_coherent.

That's odd, why not use the other default apis to do that?

> It does not matter iommu is enabled or not.
> In case iommu is disabled, it maybe dangerous to kernel, so we added warning here, is it required?

You should use the other documentated apis for this, don't create your
own.

thanks,

greg k-h

Re: [PATCH 2/2] uacce: add uacce module

[Greg Kroah-Hartman]

On Wed, Aug 21, 2019 at 03:21:18PM +0800, zhangfei.gao@foxmail.com wrote:
> Hi, Greg
> 
> On 2019/8/21 上午12:59, Greg Kroah-Hartman wrote:
> > On Tue, Aug 20, 2019 at 09:08:55PM +0800, zhangfei wrote:
> > > 
> > > On 2019/8/15 下午10:13, Greg Kroah-Hartman wrote:
> > > > On Wed, Aug 14, 2019 at 05:34:25PM +0800, Zhangfei Gao wrote:
> > > > > +int uacce_register(struct uacce *uacce)
> > > > > +{
> > > > > +	int ret;
> > > > > +
> > > > > +	if (!uacce->pdev) {
> > > > > +		pr_debug("uacce parent device not set\n");
> > > > > +		return -ENODEV;
> > > > > +	}
> > > > > +
> > > > > +	if (uacce->flags & UACCE_DEV_NOIOMMU) {
> > > > > +		add_taint(TAINT_CRAP, LOCKDEP_STILL_OK);
> > > > > +		dev_warn(uacce->pdev,
> > > > > +			 "Register to noiommu mode, which export kernel data to user space and may vulnerable to attack");
> > > > > +	}
> > > > THat is odd, why even offer this feature then if it is a major issue?
> > > UACCE_DEV_NOIOMMU maybe confusing here.
> > > 
> > > In this mode, app use ioctl to get dma_handle from dma_alloc_coherent.
> > That's odd, why not use the other default apis to do that?
> > 
> > > It does not matter iommu is enabled or not.
> > > In case iommu is disabled, it maybe dangerous to kernel, so we added warning here, is it required?
> > You should use the other documentated apis for this, don't create your
> > own.
> I am sorry, not understand here.
> Do you mean there is a standard ioctl or standard api in user space, it can
> get dma_handle from dma_alloc_coherent from kernel?

There should be a standard way to get such a handle from userspace
today.  Isn't that what the ion interface does?  DRM also does this, as
does UIO I think.

Do you have a spec somewhere that shows exactly what you are trying to
do here, along with example userspace code?  It's hard to determine it
given you only have one "half" of the code here and no users of the apis
you are creating.

thanks,

greg k-h

Re: [PATCH 2/2] uacce: add uacce module

[zhangfei]

On 2019/8/21 下午5:17, Greg Kroah-Hartman wrote:
> On Wed, Aug 21, 2019 at 03:21:18PM +0800, zhangfei.gao@foxmail.com wrote:
>> Hi, Greg
>>
>> On 2019/8/21 上午12:59, Greg Kroah-Hartman wrote:
>>> On Tue, Aug 20, 2019 at 09:08:55PM +0800, zhangfei wrote:
>>>> On 2019/8/15 下午10:13, Greg Kroah-Hartman wrote:
>>>>> On Wed, Aug 14, 2019 at 05:34:25PM +0800, Zhangfei Gao wrote:
>>>>>> +int uacce_register(struct uacce *uacce)
>>>>>> +{
>>>>>> +	int ret;
>>>>>> +
>>>>>> +	if (!uacce->pdev) {
>>>>>> +		pr_debug("uacce parent device not set\n");
>>>>>> +		return -ENODEV;
>>>>>> +	}
>>>>>> +
>>>>>> +	if (uacce->flags & UACCE_DEV_NOIOMMU) {
>>>>>> +		add_taint(TAINT_CRAP, LOCKDEP_STILL_OK);
>>>>>> +		dev_warn(uacce->pdev,
>>>>>> +			 "Register to noiommu mode, which export kernel data to user space and may vulnerable to attack");
>>>>>> +	}
>>>>> THat is odd, why even offer this feature then if it is a major issue?
>>>> UACCE_DEV_NOIOMMU maybe confusing here.
>>>>
>>>> In this mode, app use ioctl to get dma_handle from dma_alloc_coherent.
>>> That's odd, why not use the other default apis to do that?
>>>
>>>> It does not matter iommu is enabled or not.
>>>> In case iommu is disabled, it maybe dangerous to kernel, so we added warning here, is it required?
>>> You should use the other documentated apis for this, don't create your
>>> own.
>> I am sorry, not understand here.
>> Do you mean there is a standard ioctl or standard api in user space, it can
>> get dma_handle from dma_alloc_coherent from kernel?
> There should be a standard way to get such a handle from userspace
> today.  Isn't that what the ion interface does?  DRM also does this, as
> does UIO I think.
Thanks Greg,
Still not find it, will do more search.
But this may introduce dependency in our lib, like depend on ion?
> Do you have a spec somewhere that shows exactly what you are trying to
> do here, along with example userspace code?  It's hard to determine it
> given you only have one "half" of the code here and no users of the apis
> you are creating.
>
The purpose is doing dma in user space.
If sva is supported, we can directly use malloc memory.
If sva is not supported, device can not recognize va, so we get 
dma_handle via ioctl.

Sample user code is in
https://github.com/Linaro/warpdrive/blob/wdprd-v1-current/test/test_hisi_zip.c
hizip_wd_sched_init_cache:
     if (sched->qs[0].dev_flags & UACCE_DEV_NOIOMMU) {
         data_in = wd_get_pa_from_va(&sched->qs[0], wd_msg->data_in);
         data_out = wd_get_pa_from_va(&sched->qs[0], wd_msg->data_out);
     } else {
         data_in = wd_msg->data_in;
         data_out = wd_msg->data_out;
     }
     msg->source_addr_l = (__u64)data_in & 0xffffffff;
     msg->source_addr_h = (__u64)data_in >> 32;
     msg->dest_addr_l = (__u64)data_out & 0xffffffff;
     msg->dest_addr_h = (__u64)data_out >> 32;

Have added some explanation in warpdrive.rst, in the first patch.

The device can also be declared as UACCE_DEV_NOIOMMU. It can be used 
when the
device has no iommu support or the iommu is set in pass through mode.  
In this
case, the driver should map address to device by itself with DMA API. The
ioctl(UACCE_CMD_GET_SS_DMA) can be used to get the physical address, 
though it
is an untrusted and kernel-tainted behavior.

Thanks

Re: [PATCH 2/2] uacce: add uacce module

[Greg Kroah-Hartman]

On Wed, Aug 21, 2019 at 10:30:22PM +0800, zhangfei wrote:
> 
> 
> On 2019/8/21 下午5:17, Greg Kroah-Hartman wrote:
> > On Wed, Aug 21, 2019 at 03:21:18PM +0800, zhangfei.gao@foxmail.com wrote:
> > > Hi, Greg
> > > 
> > > On 2019/8/21 上午12:59, Greg Kroah-Hartman wrote:
> > > > On Tue, Aug 20, 2019 at 09:08:55PM +0800, zhangfei wrote:
> > > > > On 2019/8/15 下午10:13, Greg Kroah-Hartman wrote:
> > > > > > On Wed, Aug 14, 2019 at 05:34:25PM +0800, Zhangfei Gao wrote:
> > > > > > > +int uacce_register(struct uacce *uacce)
> > > > > > > +{
> > > > > > > +	int ret;
> > > > > > > +
> > > > > > > +	if (!uacce->pdev) {
> > > > > > > +		pr_debug("uacce parent device not set\n");
> > > > > > > +		return -ENODEV;
> > > > > > > +	}
> > > > > > > +
> > > > > > > +	if (uacce->flags & UACCE_DEV_NOIOMMU) {
> > > > > > > +		add_taint(TAINT_CRAP, LOCKDEP_STILL_OK);
> > > > > > > +		dev_warn(uacce->pdev,
> > > > > > > +			 "Register to noiommu mode, which export kernel data to user space and may vulnerable to attack");
> > > > > > > +	}
> > > > > > THat is odd, why even offer this feature then if it is a major issue?
> > > > > UACCE_DEV_NOIOMMU maybe confusing here.
> > > > > 
> > > > > In this mode, app use ioctl to get dma_handle from dma_alloc_coherent.
> > > > That's odd, why not use the other default apis to do that?
> > > > 
> > > > > It does not matter iommu is enabled or not.
> > > > > In case iommu is disabled, it maybe dangerous to kernel, so we added warning here, is it required?
> > > > You should use the other documentated apis for this, don't create your
> > > > own.
> > > I am sorry, not understand here.
> > > Do you mean there is a standard ioctl or standard api in user space, it can
> > > get dma_handle from dma_alloc_coherent from kernel?
> > There should be a standard way to get such a handle from userspace
> > today.  Isn't that what the ion interface does?  DRM also does this, as
> > does UIO I think.
> Thanks Greg,
> Still not find it, will do more search.
> But this may introduce dependency in our lib, like depend on ion?
> > Do you have a spec somewhere that shows exactly what you are trying to
> > do here, along with example userspace code?  It's hard to determine it
> > given you only have one "half" of the code here and no users of the apis
> > you are creating.
> > 
> The purpose is doing dma in user space.

Oh no, please no.  Are you _SURE_ you want to do this?

Again, look at how ION does this and how the DMAbuff stuff is replacing
it.  Use that api please instead, otherwise you will get it wrong and we
don't want to duplicate efforts.

thanks,

greg k-h

Re: [PATCH 2/2] uacce: add uacce module

[zhangfei]

Hi, Jonathan

Thanks for your careful review and good suggestion.
Sorry for late response, I am checking one detail.

On 2019/8/16 上午12:54, Jonathan Cameron wrote:
> On Wed, 14 Aug 2019 17:34:25 +0800
> Zhangfei Gao <zhangfei.gao@linaro.org> wrote:
>
>> From: Kenneth Lee <liguozhu@hisilicon.com>
>>
>> Uacce is the kernel component to support WarpDrive accelerator
>> framework. It provides register/unregister interface for device drivers
>> to expose their hardware resource to the user space. The resource is
>> taken as "queue" in WarpDrive.
> It's a bit confusing to have both the term UACCE and WarpDrive in here.
> I'd just use the uacce name in all comments etc.
Yes, make sense
>
>> 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.
>>
>> Uacce also manages unify addresses between the hardware and user space
>> of the process. So they can share the same virtual address in the
>> communication.
>>
>> Signed-off-by: Kenneth Lee <liguozhu@hisilicon.com>
>> Signed-off-by: Zaibo Xu <xuzaibo@huawei.com>
>> Signed-off-by: Zhou Wang <wangzhou1@hisilicon.com>
>> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
> I would strip this back to which ever case is of most interest (SVA I guess?)
> and only think about adding support for the others if necessary at a later date.
> (or in later patches).
Do you mean split the patch and send sva part first?
>> +
>> +static int uacce_qfr_alloc_pages(struct uacce_qfile_region *qfr)
>> +{
>> +	int gfp_mask = GFP_ATOMIC | __GFP_ZERO;
> More readable to just have this inline.
Yes, all right.
>
>> +	int i, j;
>> +
>> +	qfr->pages = kcalloc(qfr->nr_pages, sizeof(*qfr->pages), gfp_mask);
> kcalloc is always set to zero anyway.
OK
>
>> +
>> +static struct uacce_qfile_region *
>> +uacce_create_region(struct uacce_queue *q, struct vm_area_struct *vma,
>> +		    enum uacce_qfrt type, unsigned int flags)
>> +{
>> +	struct uacce_qfile_region *qfr;
>> +	struct uacce *uacce = q->uacce;
>> +	unsigned long vm_pgoff;
>> +	int ret = -ENOMEM;
>> +
>> +	dev_dbg(uacce->pdev, "create qfr (type=%x, flags=%x)\n", type, flags);
>> +	qfr = kzalloc(sizeof(*qfr), GFP_ATOMIC);
>> +	if (!qfr)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	qfr->type = type;
>> +	qfr->flags = flags;
>> +	qfr->iova = vma->vm_start;
>> +	qfr->nr_pages = vma_pages(vma);
>> +
>> +	if (vma->vm_flags & VM_READ)
>> +		qfr->prot |= IOMMU_READ;
>> +
>> +	if (vma->vm_flags & VM_WRITE)
>> +		qfr->prot |= IOMMU_WRITE;
>> +
>> +	if (flags & UACCE_QFRF_SELFMT) {
>> +		ret = uacce->ops->mmap(q, vma, qfr);
>> +		if (ret)
>> +			goto err_with_qfr;
>> +		return qfr;
>> +	}
>> +
>> +	/* allocate memory */
>> +	if (flags & UACCE_QFRF_DMA) {
>> +		dev_dbg(uacce->pdev, "allocate dma %d pages\n", qfr->nr_pages);
>> +		qfr->kaddr = dma_alloc_coherent(uacce->pdev, qfr->nr_pages <<
>> +						PAGE_SHIFT, &qfr->dma,
>> +						GFP_KERNEL);
>> +		if (!qfr->kaddr) {
>> +			ret = -ENOMEM;
>> +			goto err_with_qfr;
>> +		}
>> +	} else {
>> +		dev_dbg(uacce->pdev, "allocate %d pages\n", qfr->nr_pages);
>> +		ret = uacce_qfr_alloc_pages(qfr);
>> +		if (ret)
>> +			goto err_with_qfr;
>> +	}
>> +
>> +	/* map to device */
>> +	ret = uacce_queue_map_qfr(q, qfr);
>> +	if (ret)
>> +		goto err_with_pages;
>> +
>> +	/* mmap to user space */
>> +	if (flags & UACCE_QFRF_MMAP) {
>> +		if (flags & UACCE_QFRF_DMA) {
>> +
>> +			/* dma_mmap_coherent() requires vm_pgoff as 0
>> +			 * restore vm_pfoff to initial value for mmap()
>> +			 */
>> +			dev_dbg(uacce->pdev, "mmap dma qfr\n");
>> +			vm_pgoff = vma->vm_pgoff;
>> +			vma->vm_pgoff = 0;
>> +			ret = dma_mmap_coherent(uacce->pdev, vma, qfr->kaddr,
>> +						qfr->dma,
>> +						qfr->nr_pages << PAGE_SHIFT);
> Does setting vm_pgoff if (ret) make sense?
Since we need restore the environment, so restore vm_pgoff no matter 
succeed or not.
>> +			vma->vm_pgoff = vm_pgoff;
>> +		} else {
>> +			ret = uacce_queue_mmap_qfr(q, qfr, vma);
>> +		}
>> +
>> +		if (ret)
>> +			goto err_with_mapped_qfr;
>> +	}
>> +
>> +	return qfr;
>> +
>> +err_with_mapped_qfr:
>> +	uacce_queue_unmap_qfr(q, qfr);
>> +err_with_pages:
>> +	if (flags & UACCE_QFRF_DMA)
>> +		dma_free_coherent(uacce->pdev, qfr->nr_pages << PAGE_SHIFT,
>> +				  qfr->kaddr, qfr->dma);
>> +	else
>> +		uacce_qfr_free_pages(qfr);
>> +err_with_qfr:
>> +	kfree(qfr);
>> +
>> +	return ERR_PTR(ret);
>> +}
>> +
>> +/* we assume you have uacce_queue_unmap_qfr(q, qfr) from all related queues */
>> +static void uacce_destroy_region(struct uacce_queue *q,
>> +				 struct uacce_qfile_region *qfr)
>> +{
>> +	struct uacce *uacce = q->uacce;
>> +
>> +	if (qfr->flags & UACCE_QFRF_DMA) {
>> +		dev_dbg(uacce->pdev, "free dma qfr %s (kaddr=%lx, dma=%llx)\n",
>> +			uacce_qfrt_str(qfr), (unsigned long)qfr->kaddr,
>> +			qfr->dma);
>> +		dma_free_coherent(uacce->pdev, qfr->nr_pages << PAGE_SHIFT,
>> +				  qfr->kaddr, qfr->dma);
>> +	} else if (qfr->pages) {
>> +		if (qfr->flags & UACCE_QFRF_KMAP && qfr->kaddr) {
>> +			dev_dbg(uacce->pdev, "vunmap qfr %s\n",
>> +				uacce_qfrt_str(qfr));
>> +			vunmap(qfr->kaddr);
>> +			qfr->kaddr = NULL;
>> +		}
>> +
>> +		uacce_qfr_free_pages(qfr);
>> +	}
>> +	kfree(qfr);
>> +}
>> +
>> +static long uacce_cmd_share_qfr(struct uacce_queue *tgt, int fd)
>> +{
>> +	struct file *filep = fget(fd);
> That's not a trivial assignment so I would not have it up here.
>
>> +	struct uacce_queue *src;
>> +	int ret = -EINVAL;
>> +
> 	filep = fget(fd);
Yes, make sense.
>> +	if (!filep)
>> +		return ret;
>> +
>> +	if (filep->f_op != &uacce_fops)
>> +		goto out_with_fd;
>> +
>> +	src = filep->private_data;
>> +	if (!src)
>> +		goto out_with_fd;
>> +
>> +	/* no share sva is needed if the dev can do fault-from-dev */
>> +	if (tgt->uacce->flags & UACCE_DEV_FAULT_FROM_DEV)
>> +		goto out_with_fd;
>> +
>> +	dev_dbg(&src->uacce->dev, "share ss with %s\n",
>> +		dev_name(&tgt->uacce->dev));
>> +
>> +	uacce_qs_wlock();
>> +	if (!src->qfrs[UACCE_QFRT_SS] || tgt->qfrs[UACCE_QFRT_SS])
>> +		goto out_with_lock;
>> +
>> +	ret = uacce_queue_map_qfr(tgt, src->qfrs[UACCE_QFRT_SS]);
>> +	if (ret)
>> +		goto out_with_lock;
>> +
>> +	tgt->qfrs[UACCE_QFRT_SS] = src->qfrs[UACCE_QFRT_SS];
>> +	list_add(&tgt->list, &src->qfrs[UACCE_QFRT_SS]->qs);
>> +	ret = 0;
>> +
>> +out_with_lock:
>> +	uacce_qs_wunlock();
>> +out_with_fd:
>> +	fput(filep);
>> +	return ret;
>> +}
>> +
>> +static int uacce_start_queue(struct uacce_queue *q)
>> +{
>> +	int ret, i, j;
>> +	struct uacce_qfile_region *qfr;
>> +	struct device *dev = &q->uacce->dev;
>> +
>> +	/*
>> +	 * map KMAP qfr to kernel
>> +	 * vmap should be done in non-spinlocked context!
>> +	 */
>> +	for (i = 0; i < UACCE_QFRT_MAX; i++) {
>> +		qfr = q->qfrs[i];
>> +		if (qfr && (qfr->flags & UACCE_QFRF_KMAP) && !qfr->kaddr) {
>> +			qfr->kaddr = vmap(qfr->pages, qfr->nr_pages, VM_MAP,
>> +					  PAGE_KERNEL);
>> +			if (!qfr->kaddr) {
>> +				ret = -ENOMEM;
>> +				dev_dbg(dev, "fail to kmap %s qfr(%d pages)\n",
>> +					uacce_qfrt_str(qfr), qfr->nr_pages);
> If it's useful, dev_err.
OK
>> +static long uacce_fops_unl_ioctl(struct file *filep,
>> +				 unsigned int cmd, unsigned long arg)
>> +{
>> +	struct uacce_queue *q = filep->private_data;
>> +	struct uacce *uacce = q->uacce;
>> +
>> +	switch (cmd) {
>> +	case UACCE_CMD_SHARE_SVAS:
>> +		return uacce_cmd_share_qfr(q, arg);
>> +
>> +	case UACCE_CMD_START:
>> +		return uacce_start_queue(q);
>> +
>> +	case UACCE_CMD_GET_SS_DMA:
>> +		return uacce_get_ss_dma(q, (void __user *)arg);
>> +
>> +	default:
>> +		if (uacce->ops->ioctl)
>> +			return uacce->ops->ioctl(q, cmd, arg);
>> +
>> +		dev_err(&uacce->dev, "ioctl cmd (%d) is not supported!\n", cmd);
>> +		return -EINVAL;
> Flip the logic so the error is the indented path.
> 		if (!uacce->ops->ioctl) ...
>
> Fits better with typical coding model in a kernel reviewers head
> - well mine anyway ;)
OK, make sense.
>> +
>> +static int uacce_fops_open(struct inode *inode, struct file *filep)
>> +{
>> +	struct uacce_queue *q;
>> +	struct iommu_sva *handle = NULL;
>> +	struct uacce *uacce;
>> +	int ret;
>> +	int pasid = 0;
>> +
>> +	uacce = idr_find(&uacce_idr, iminor(inode));
>> +	if (!uacce)
>> +		return -ENODEV;
>> +
>> +	if (atomic_read(&uacce->state) == UACCE_ST_RST)
>> +		return -EINVAL;
>> +
>> +	if (!uacce->ops->get_queue)
>> +		return -EINVAL;
>> +
>> +	if (!try_module_get(uacce->pdev->driver->owner))
>> +		return -ENODEV;
>> +
>> +	ret = uacce_dev_open_check(uacce);
>> +	if (ret)
>> +		goto open_err;
>> +
>> +#ifdef CONFIG_IOMMU_SVA
>> +	if (uacce->flags & UACCE_DEV_PASID) {
>> +		handle = iommu_sva_bind_device(uacce->pdev, current->mm, NULL);
>> +		if (IS_ERR(handle))
>> +			goto open_err;
>> +		pasid = iommu_sva_get_pasid(handle);
>> +	}
>> +#endif
>> +	ret = uacce->ops->get_queue(uacce, pasid, &q);
>> +	if (ret < 0)
>> +		goto open_err;
>> +
>> +	q->pasid = pasid;
>> +	q->handle = handle;
>> +	q->uacce = uacce;
>> +	q->mm = current->mm;
>> +	memset(q->qfrs, 0, sizeof(q->qfrs));
>> +	INIT_LIST_HEAD(&q->list);
>> +	init_waitqueue_head(&q->wait);
>> +	filep->private_data = q;
>> +	mutex_lock(&uacce->q_lock);
>> +	list_add(&q->q_dev, &uacce->qs);
>> +	mutex_unlock(&uacce->q_lock);
>> +
>> +	return 0;
> blank line
OK.
>> +open_err:
>> +	module_put(uacce->pdev->driver->owner);
>> +	return ret;
>> +}
>> +
>> +static int uacce_fops_release(struct inode *inode, struct file *filep)
>> +{
>> +	struct uacce_queue *q = (struct uacce_queue *)filep->private_data;
>> +	struct uacce_qfile_region *qfr;
>> +	struct uacce *uacce = q->uacce;
>> +	int i;
>> +	bool is_to_free_region;
>> +	int free_pages = 0;
>> +
>> +	mutex_lock(&uacce->q_lock);
>> +	list_del(&q->q_dev);
>> +	mutex_unlock(&uacce->q_lock);
>> +
>> +	if (atomic_read(&uacce->state) == UACCE_ST_STARTED &&
>> +	    uacce->ops->stop_queue)
>> +		uacce->ops->stop_queue(q);
>> +
>> +	uacce_qs_wlock();
>> +
>> +	for (i = 0; i < UACCE_QFRT_MAX; i++) {
>> +		qfr = q->qfrs[i];
>> +		if (!qfr)
>> +			continue;
>> +
>> +		is_to_free_region = false;
>> +		uacce_queue_unmap_qfr(q, qfr);
>> +		if (i == UACCE_QFRT_SS) {
>> +			list_del(&q->list);
>> +			if (list_empty(&qfr->qs))
>> +				is_to_free_region = true;
>> +		} else
>> +			is_to_free_region = true;
>> +
>> +		if (is_to_free_region) {
>> +			free_pages += qfr->nr_pages;
>> +			uacce_destroy_region(q, qfr);
>> +		}
>> +
>> +		qfr = NULL;
>> +	}
>> +
>> +	uacce_qs_wunlock();
>> +
>> +	if (current->mm == q->mm) {
>> +		down_write(&q->mm->mmap_sem);
>> +		q->mm->data_vm -= free_pages;
>> +		up_write(&q->mm->mmap_sem);
>> +	}
>> +
>> +#ifdef CONFIG_IOMMU_SVA
>> +	if (uacce->flags & UACCE_DEV_PASID)
>> +		iommu_sva_unbind_device(q->handle);
>> +#endif
>> +
>> +	if (uacce->ops->put_queue)
>> +		uacce->ops->put_queue(q);
>> +
>> +	dev_dbg(&uacce->dev, "uacce state switch to INIT\n");
>> +	atomic_set(&uacce->state, UACCE_ST_INIT);
>> +	module_put(uacce->pdev->driver->owner);
> blank line here.
>
>> +	return 0;
>> +}
>> +
>> +static enum uacce_qfrt uacce_get_region_type(struct uacce *uacce,
>> +					     struct vm_area_struct *vma)
>> +{
>> +	enum uacce_qfrt type = UACCE_QFRT_MAX;
>> +	int i;
>> +	size_t next_start = UACCE_QFR_NA;
>> +
>> +	for (i = UACCE_QFRT_MAX - 1; i >= 0; i--) {
>> +		if (vma->vm_pgoff >= uacce->qf_pg_start[i]) {
>> +			type = i;
>> +			break;
>> +		}
>> +	}
>> +
>> +	switch (type) {
>> +	case UACCE_QFRT_MMIO:
>> +		if (!uacce->ops->mmap) {
>> +			dev_err(&uacce->dev, "no driver mmap!\n");
>> +			return UACCE_QFRT_INVALID;
>> +		}
>> +		break;
>> +
>> +	case UACCE_QFRT_DKO:
>> +		if ((uacce->flags & UACCE_DEV_PASID) ||
>> +		    (uacce->flags & UACCE_DEV_NOIOMMU))
>> +			return UACCE_QFRT_INVALID;
>> +		break;
>> +
>> +	case UACCE_QFRT_DUS:
>> +		break;
>> +
>> +	case UACCE_QFRT_SS:
>> +		/* todo: this can be valid to protect the process space */
>> +		if (uacce->flags & UACCE_DEV_FAULT_FROM_DEV)
>> +			return UACCE_QFRT_INVALID;
>> +		break;
>> +
>> +	default:
>> +		dev_err(&uacce->dev, "uacce bug (%d)!\n", type);
>> +		return UACCE_QFRT_INVALID;
>> +	}
>> +
>> +	/* make sure the mapping size is exactly the same as the region */
>> +	if (type < UACCE_QFRT_SS) {
>> +		for (i = type + 1; i < UACCE_QFRT_MAX; i++)
>> +			if (uacce->qf_pg_start[i] != UACCE_QFR_NA) {
>> +				next_start = uacce->qf_pg_start[i];
>> +				break;
>> +			}
>> +
>> +		if (next_start == UACCE_QFR_NA) {
>> +			dev_err(&uacce->dev, "uacce config error: SS offset set improperly\n");
>> +			return UACCE_QFRT_INVALID;
>> +		}
>> +
>> +		if (vma_pages(vma) !=
>> +		    next_start - uacce->qf_pg_start[type]) {
>> +			dev_err(&uacce->dev, "invalid mmap size (%ld vs %ld pages) for region %s.\n",
>> +				vma_pages(vma),
>> +				next_start - uacce->qf_pg_start[type],
>> +				qfrt_str[type]);
>> +			return UACCE_QFRT_INVALID;
>> +		}
>> +	}
>> +
>> +	return type;
>> +}
>> +
>> +static int uacce_fops_mmap(struct file *filep, struct vm_area_struct *vma)
>> +{
>> +	struct uacce_queue *q = (struct uacce_queue *)filep->private_data;
> As below, cast not needed.
OK, thanks
>
>> +	struct uacce *uacce = q->uacce;
>> +	enum uacce_qfrt type = uacce_get_region_type(uacce, vma);
>> +	struct uacce_qfile_region *qfr;
>> +	unsigned int flags = 0;
>> +	int ret;
>> +
>> +	dev_dbg(&uacce->dev, "mmap q file(t=%s, off=%lx, start=%lx, end=%lx)\n",
>> +		 qfrt_str[type], vma->vm_pgoff, vma->vm_start, vma->vm_end);
>> +
>> +	if (type == UACCE_QFRT_INVALID)
>> +		return -EINVAL;
>> +
>> +	vma->vm_flags |= VM_DONTCOPY | VM_DONTEXPAND;
>> +
>> +	uacce_qs_wlock();
>> +
>> +	/* fixme: if the region need no pages, we don't need to check it */
>> +	if (q->mm->data_vm + vma_pages(vma) >
>> +	    rlimit(RLIMIT_DATA) >> PAGE_SHIFT) {
>> +		ret = -ENOMEM;
>> +		goto out_with_lock;
>> +	}
>> +
>> +	if (q->qfrs[type]) {
>> +		ret = -EBUSY;
>> +		goto out_with_lock;
>> +	}
>> +
>> +	switch (type) {
>> +	case UACCE_QFRT_MMIO:
>> +		flags = UACCE_QFRF_SELFMT;
>> +		break;
>> +
>> +	case UACCE_QFRT_SS:
>> +		if (atomic_read(&uacce->state) != UACCE_ST_STARTED) {
>> +			ret = -EINVAL;
>> +			goto out_with_lock;
>> +		}
>> +
>> +		flags = UACCE_QFRF_MAP | UACCE_QFRF_MMAP;
>> +
>> +		if (uacce->flags & UACCE_DEV_NOIOMMU)
>> +			flags |= UACCE_QFRF_DMA;
>> +		break;
>> +
>> +	case UACCE_QFRT_DKO:
>> +		flags = UACCE_QFRF_MAP | UACCE_QFRF_KMAP;
>> +
>> +		if (uacce->flags & UACCE_DEV_NOIOMMU)
>> +			flags |= UACCE_QFRF_DMA;
>> +		break;
>> +
>> +	case UACCE_QFRT_DUS:
>> +		if ((uacce->flags & UACCE_DEV_NOIOMMU) ||
>> +		    (uacce->flags & UACCE_DEV_PASID)) {
>> +			flags = UACCE_QFRF_SELFMT;
>> +			break;
>> +		}
>> +
>> +		flags = UACCE_QFRF_MAP | UACCE_QFRF_MMAP;
>> +		break;
>> +
>> +	default:
>> +		WARN_ON(&uacce->dev);
>> +		break;
>> +	}
>> +
>> +	qfr = uacce_create_region(q, vma, type, flags);
>> +	if (IS_ERR(qfr)) {
>> +		ret = PTR_ERR(qfr);
>> +		goto out_with_lock;
>> +	}
>> +	q->qfrs[type] = qfr;
>> +
>> +	if (type == UACCE_QFRT_SS) {
>> +		INIT_LIST_HEAD(&qfr->qs);
>> +		list_add(&q->list, &q->qfrs[type]->qs);
>> +	}
>> +
>> +	uacce_qs_wunlock();
>> +
>> +	if (qfr->pages)
>> +		q->mm->data_vm += qfr->nr_pages;
>> +
>> +	return 0;
>> +
>> +out_with_lock:
>> +	uacce_qs_wunlock();
>> +	return ret;
>> +}
>> +
>> +static __poll_t uacce_fops_poll(struct file *file, poll_table *wait)
>> +{
>> +	struct uacce_queue *q = (struct uacce_queue *)file->private_data;
> Private data is a void * so no need to cast explicitly.
>
> 	struct uacce_queue *q = file->private_data;
OK
>> +static ssize_t api_show(struct device *dev,
>> +				  struct device_attribute *attr, char *buf)
>> +{
>> +	struct uacce *uacce = UACCE_FROM_CDEV_ATTR(dev);
>> +
>> +	return sprintf(buf, "%s\n", uacce->api_ver);
>> +}
>> +
>> +static ssize_t numa_distance_show(struct device *dev,
>> +					    struct device_attribute *attr,
>> +					    char *buf)
>> +{
>> +	struct uacce *uacce = UACCE_FROM_CDEV_ATTR(dev);
>> +	int distance = 0;
>> +
>> +#ifdef CONFIG_NUMA
>> +	distance = cpu_to_node(smp_processor_id()) - uacce->pdev->numa_node;
> What is this function supposed to return?
> It currently returns the absolute difference in node number.
> I suppose if that is 0, then it means local node, but all other values
> have no sensible meaning.
>
> Perhaps use node_distance()? That should give you the SLIT distance
> so 10 for local and bigger for everything else.
Yes, node_distance is better and consider #ifdef CONFIG_NUMA
>> +#endif
>> +	return sprintf(buf, "%d\n", abs(distance));
>> +}
>> +
>> +static ssize_t node_id_show(struct device *dev,
>> +				      struct device_attribute *attr,
>> +				      char *buf)
>> +{
>> +	struct uacce *uacce = UACCE_FROM_CDEV_ATTR(dev);
>> +	int node_id = -1;
>> +
>> +#ifdef CONFIG_NUMA
>> +	node_id = uacce->pdev->numa_node;
> use dev_to_node(uacce->pdev) which already does this protection for you.
Yes, dev_to_node is better
>> +#endif
>> +	return sprintf(buf, "%d\n", node_id);
>> +}
>> +
>> +static ssize_t flags_show(struct device *dev,
>> +				       struct device_attribute *attr,
>> +				       char *buf)
>> +{
>> +	struct uacce *uacce = UACCE_FROM_CDEV_ATTR(dev);
>> +
>> +	return sprintf(buf, "%d\n", uacce->flags);
>> +}
>> +
>> +static ssize_t available_instances_show(struct device *dev,
>> +					  struct device_attribute *attr,
>> +						  char *buf)
>> +{
>> +	struct uacce *uacce = UACCE_FROM_CDEV_ATTR(dev);
>> +
>> +	return sprintf(buf, "%d\n", uacce->ops->get_available_instances(uacce));
>> +}
>> +
>> +static ssize_t algorithms_show(struct device *dev,
>> +					 struct device_attribute *attr,
>> +					 char *buf)
>> +{
>> +	struct uacce *uacce = UACCE_FROM_CDEV_ATTR(dev);
>> +
>> +	return sprintf(buf, "%s", uacce->algs);
>> +}
>> +
>> +static ssize_t qfrs_offset_show(struct device *dev,
>> +					  struct device_attribute *attr,
>> +					  char *buf)
>> +{
>> +	struct uacce *uacce = UACCE_FROM_CDEV_ATTR(dev);
>> +	int i, ret;
>> +	unsigned long offset;
>> +
>> +	for (i = 0, ret = 0; i < UACCE_QFRT_MAX; i++) {
>> +		offset = uacce->qf_pg_start[i];
>> +		if (offset != UACCE_QFR_NA)
>> +			offset = offset << PAGE_SHIFT;
>> +		if (i == UACCE_QFRT_SS)
>> +			break;
>> +		ret += sprintf(buf + ret, "%lu\t", offset);
>> +	}
>> +	ret += sprintf(buf + ret, "%lu\n", offset);
>> +
>> +	return ret;
>> +}
>> +
>> +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_offset);
>> +
>> +static struct attribute *uacce_dev_attrs[] = {
> New ABI. All needs documenting in
>
> Documentation/ABI/
ok
>
>> +	&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_offset.attr,
>> +	NULL,
>> +};
>> +
>> +static const struct attribute_group uacce_dev_attr_group = {
>> +	.name	= UACCE_DEV_ATTRS,
>> +	.attrs	= uacce_dev_attrs,
>> +};
>> +
>> +static const struct attribute_group *uacce_dev_attr_groups[] = {
>> +	&uacce_dev_attr_group,
>> +	NULL
>> +};
>> +
>> +static int uacce_create_chrdev(struct uacce *uacce)
>> +{
>> +	int ret;
>> +
>> +	ret = idr_alloc(&uacce_idr, uacce, 0, 0, GFP_KERNEL);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	cdev_init(&uacce->cdev, &uacce_fops);
>> +	uacce->dev_id = ret;
>> +	uacce->cdev.owner = THIS_MODULE;
>> +	device_initialize(&uacce->dev);
>> +	uacce->dev.devt = MKDEV(MAJOR(uacce_devt), uacce->dev_id);
>> +	uacce->dev.class = uacce_class;
>> +	uacce->dev.groups = uacce_dev_attr_groups;
>> +	uacce->dev.parent = uacce->pdev;
>> +	dev_set_name(&uacce->dev, "%s-%d", uacce->drv_name, uacce->dev_id);
>> +	ret = cdev_device_add(&uacce->cdev, &uacce->dev);
>> +	if (ret)
>> +		goto err_with_idr;
>> +
>> +	dev_dbg(&uacce->dev, "create uacce minior=%d\n", uacce->dev_id);
>> +	return 0;
>> +
>> +err_with_idr:
>> +	idr_remove(&uacce_idr, uacce->dev_id);
>> +	return ret;
>> +}
>> +
>> +static void uacce_destroy_chrdev(struct uacce *uacce)
>> +{
>> +	cdev_device_del(&uacce->cdev, &uacce->dev);
>> +	idr_remove(&uacce_idr, uacce->dev_id);
>> +}
>> +
>> +static int uacce_default_get_available_instances(struct uacce *uacce)
>> +{
> Does this one ever make sense for a real device?
>
>> +	return -1;
>> +}
>> +
>> +static int uacce_default_start_queue(struct uacce_queue *q)
>> +{
>> +	dev_dbg(&q->uacce->dev, "fake start queue");
> Does this ever make sense on a real device?
Will remove these two default funcs.
>
>> +	return 0;
>> +}
>> +
>> +static int uacce_dev_match(struct device *dev, void *data)
>> +{
>> +	if (dev->parent == data)
>> +		return -EBUSY;
>> +
>> +	return 0;
>> +}
>> +
>> +/* Borrowed from VFIO to fix msi translation */
>> +static bool uacce_iommu_has_sw_msi(struct iommu_group *group,
>> +				   phys_addr_t *base)
>> +{
>> +	struct list_head group_resv_regions;
>> +	struct iommu_resv_region *region, *next;
>> +	bool ret = false;
>> +
>> +	INIT_LIST_HEAD(&group_resv_regions);
>> +	iommu_get_group_resv_regions(group, &group_resv_regions);
>> +	list_for_each_entry(region, &group_resv_regions, list) {
>> +		pr_debug("uacce: find a resv region (%d) on %llx\n",
>> +			 region->type, region->start);
>> +
>> +		/*
>> +		 * The presence of any 'real' MSI regions should take
>> +		 * precedence over the software-managed one if the
>> +		 * IOMMU driver happens to advertise both types.
>> +		 */
>> +		if (region->type == IOMMU_RESV_MSI) {
>> +			ret = false;
>> +			break;
>> +		}
>> +
>> +		if (region->type == IOMMU_RESV_SW_MSI) {
>> +			*base = region->start;
>> +			ret = true;
>> +		}
>> +	}
>> +	list_for_each_entry_safe(region, next, &group_resv_regions, list)
>> +		kfree(region);
>> +	return ret;
>> +}
>> +
>> +static int uacce_set_iommu_domain(struct uacce *uacce)
>> +{
>> +	struct iommu_domain *domain;
>> +	struct iommu_group *group;
>> +	struct device *dev = uacce->pdev;
>> +	bool resv_msi;
>> +	phys_addr_t resv_msi_base = 0;
>> +	int ret;
>> +
>> +	if ((uacce->flags & UACCE_DEV_NOIOMMU) ||
>> +	    (uacce->flags & UACCE_DEV_PASID))
>> +		return 0;
>> +
>> +	/*
>> +	 * We don't support multiple register for the same dev in RFC version ,
>> +	 * will add it in formal version
> So this effectively multiple complete uacce interfaces for one device.
> Is there a known usecase for that?
Here is preventing one device with multiple algorithm and register 
multi-times,
and without sva, they can not be distinguished.
>> +	 */
>> +	ret = class_for_each_device(uacce_class, NULL, uacce->pdev,
>> +				    uacce_dev_match);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* allocate and attach a unmanged domain */
>> +	domain = iommu_domain_alloc(uacce->pdev->bus);
>> +	if (!domain) {
>> +		dev_dbg(&uacce->dev, "cannot get domain for iommu\n");
>> +		return -ENODEV;
>> +	}
>> +
>> +	ret = iommu_attach_device(domain, uacce->pdev);
>> +	if (ret)
>> +		goto err_with_domain;
>> +
>> +	if (iommu_capable(dev->bus, IOMMU_CAP_CACHE_COHERENCY)) {
>> +		uacce->prot |= IOMMU_CACHE;
>> +		dev_dbg(dev, "Enable uacce with c-coherent capa\n");
>> +	} else
>> +		dev_dbg(dev, "Enable uacce without c-coherent capa\n");
> Do those debug statements add anything?  I'd also like a comment to explain
> why we care here.
OK,
>> +
>> +	group = iommu_group_get(dev);
>> +	if (!group) {
>> +		ret = -EINVAL;
>> +		goto err_with_domain;
>> +	}
>> +
>> +	resv_msi = uacce_iommu_has_sw_msi(group, &resv_msi_base);
>> +	iommu_group_put(group);
>> +
>> +	if (resv_msi) {
>> +		if (!irq_domain_check_msi_remap() &&
>> +		    !iommu_capable(dev->bus, IOMMU_CAP_INTR_REMAP)) {
>> +			dev_warn(dev, "No interrupt remapping support!");
>> +			ret = -EPERM;
>> +			goto err_with_domain;
>> +		}
>> +
>> +		dev_dbg(dev, "Set resv msi %llx on iommu domain\n",
>> +			(u64)resv_msi_base);
>> +		ret = iommu_get_msi_cookie(domain, resv_msi_base);
>> +		if (ret)
>> +			goto err_with_domain;
>> +	}
>> +
>> +	return 0;
>> +
>> +err_with_domain:
>> +	iommu_domain_free(domain);
>> +	return ret;
>> +}
>> +
>> +static void uacce_unset_iommu_domain(struct uacce *uacce)
>> +{
>> +	struct iommu_domain *domain;
>> +
>> +	if ((uacce->flags & UACCE_DEV_NOIOMMU) ||
>> +	    (uacce->flags & UACCE_DEV_PASID))
>> +		return;
>> +
>> +	domain = iommu_get_domain_for_dev(uacce->pdev);
>> +	if (domain) {
>> +		iommu_detach_device(domain, uacce->pdev);
>> +		iommu_domain_free(domain);
>> +	} else
>> +		dev_err(&uacce->dev, "bug: no domain attached to device\n");
> Given this is an error path, perhaps the following flow is easier to read (slightly)
>
> 	if (!domain) {
> 		dev_err(&uacce->dev, "bug: no domain attached to device\n");
> 		return;
> 	}
>
> 	iommu_detach_device(domain, uacce->pdev);
> 	iommu_domain_free(domain);
> }
Yes, this is much better.
>> +}
>> +
>> +/**
>> + *	uacce_register - register an accelerator
>> + *	@uacce: the accelerator structure
>> + */
>> +int uacce_register(struct uacce *uacce)
>> +{
>> +	int ret;
>> +
>> +	if (!uacce->pdev) {
>> +		pr_debug("uacce parent device not set\n");
>> +		return -ENODEV;
>> +	}
>> +
>> +	if (uacce->flags & UACCE_DEV_NOIOMMU) {
>> +		add_taint(TAINT_CRAP, LOCKDEP_STILL_OK);
>> +		dev_warn(uacce->pdev,
>> +			 "Register to noiommu mode, which export kernel data to user space and may vulnerable to attack");
> Greg already covered this one ;)
>
>> +	}
>> +
>> +	/* if dev support fault-from-dev, it should support pasid */
>> +	if ((uacce->flags & UACCE_DEV_FAULT_FROM_DEV) &&
>> +	    !(uacce->flags & UACCE_DEV_PASID)) {
>> +		dev_warn(&uacce->dev, "SVM/SAV device should support PASID\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (!uacce->ops->start_queue)
>> +		uacce->ops->start_queue = uacce_default_start_queue;
>> +
>> +	if (!uacce->ops->get_available_instances)
>> +		uacce->ops->get_available_instances =
>> +			uacce_default_get_available_instances;
>> +
>> +#ifdef CONFIG_IOMMU_SVA
>> +	if (uacce->flags & UACCE_DEV_PASID) {
>> +		ret = iommu_dev_enable_feature(uacce->pdev, IOMMU_DEV_FEAT_SVA);
>> +		if (ret)
>> +			uacce->flags &= ~(UACCE_DEV_FAULT_FROM_DEV |
>> +					  UACCE_DEV_PASID);
>> +	}
>> +#endif
>> +
>> +	ret = uacce_set_iommu_domain(uacce);
>> +	if (ret)
>> +		return ret;
>> +
>> +	mutex_lock(&uacce_mutex);
>> +
>> +	ret = uacce_create_chrdev(uacce);
>> +	if (ret)
>> +		goto err_with_lock;
>> +
>> +	dev_dbg(&uacce->dev, "uacce state initialized to INIT");
> Not sure that tells us much of interest.  Probably clean out most of the
> dev_dbg statements. Useful when developing but once it works they
> add lines of code that don't do anything useful.
Will remove most dev_dbg.
>
>> +	atomic_set(&uacce->state, UACCE_ST_INIT);
>> +	INIT_LIST_HEAD(&uacce->qs);
>> +	mutex_init(&uacce->q_lock);
>> +	mutex_unlock(&uacce_mutex);
>> +
> One blank line is almost always enough.
>
>> +
>> +	return 0;
>> +
>> +err_with_lock:
>> +	mutex_unlock(&uacce_mutex);
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(uacce_register);
>> +
>> +/**
>> + * uacce_unregister - unregisters a uacce
>> + * @uacce: the accelerator to unregister
>> + *
>> + * Unregister an accelerator that wat previously successully registered with
>> + * uacce_register().
>> + */
>> +void uacce_unregister(struct uacce *uacce)
>> +{
>> +	mutex_lock(&uacce_mutex);
>> +
>> +#ifdef CONFIG_IOMMU_SVA
>> +	if (uacce->flags & UACCE_DEV_PASID)
>> +		iommu_dev_disable_feature(uacce->pdev, IOMMU_DEV_FEAT_SVA);
>> +#endif
>> +	uacce_unset_iommu_domain(uacce);
>> +
>> +	uacce_destroy_chrdev(uacce);
>> +
>> +	mutex_unlock(&uacce_mutex);
>> +}
>> +EXPORT_SYMBOL_GPL(uacce_unregister);
>> +
>> +static int __init uacce_init(void)
>> +{
>> +	int ret;
>> +
>> +	uacce_class = class_create(THIS_MODULE, UACCE_CLASS_NAME);
>> +	if (IS_ERR(uacce_class)) {
>> +		ret = PTR_ERR(uacce_class);
>> +		goto err;
>> +	}
>> +
>> +	ret = alloc_chrdev_region(&uacce_devt, 0, MINORMASK, "uacce");
>> +	if (ret)
>> +		goto err_with_class;
>> +
>> +	pr_info("uacce init with major number:%d\n", MAJOR(uacce_devt));
>> +
>> +	return 0;
>> +
>> +err_with_class:
>> +	class_destroy(uacce_class);
>> +err:
>> +	return ret;
>> +}
>> +
>> +static __exit void uacce_exit(void)
>> +{
>> +	unregister_chrdev_region(uacce_devt, MINORMASK);
>> +	class_destroy(uacce_class);
>> +	idr_destroy(&uacce_idr);
>> +}
>> +
>> +subsys_initcall(uacce_init);
>> +module_exit(uacce_exit);
>> +
>> +MODULE_LICENSE("GPL");
>> +MODULE_AUTHOR("Hisilicon Tech. Co., Ltd.");
>> +MODULE_DESCRIPTION("Accelerator interface for Userland applications");
>> diff --git a/include/linux/uacce.h b/include/linux/uacce.h
>> new file mode 100644
>> index 0000000..fe2f6f4
>> --- /dev/null
>> +++ b/include/linux/uacce.h
>> @@ -0,0 +1,109 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +#ifndef __UACCE_H
>> +#define __UACCE_H
>> +
>> +#include <linux/cdev.h>
>> +#include <linux/device.h>
>> +#include <linux/fs.h>
>> +#include <linux/list.h>
>> +#include <linux/iommu.h>
>> +#include <uapi/misc/uacce.h>
>> +
>> +struct uacce_queue;
>> +struct uacce;
>> +
>> +/* uacce mode of the driver */
>> +#define UACCE_MODE_NOUACCE	0 /* don't use uacce */
>> +#define UACCE_MODE_UACCE	1 /* use uacce exclusively */
>> +#define UACCE_MODE_NOIOMMU	2 /* use uacce noiommu mode */
>> +
>> +#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_qfile_region {
> I'd like to see kernel doc for all the structures in here.
Will add description of each member above this structure in this file.
>> +	enum uacce_qfrt type;
>> +	unsigned long iova;	/* iova share between user and device space */
>> +	struct page **pages;
>> +	int nr_pages;
>> +	int prot;
>> +	unsigned int flags;
>> +	struct list_head qs;	/* qs sharing the same region, for ss */
>> +	void *kaddr;		/* kernel addr */
>> +	dma_addr_t dma;		/* dma address, if created by dma api */
>> +};
>> +
>> +/**
>> + * struct uacce_ops - WD device operations
> Make sure you don't miss out elements in the docs.
>
> get_available_instances.
>
> Easy to check, just build the kernel doc.
>
>> + * @get_queue: get a queue from the device according to algorithm
> Not obvious in this case what 'according to algorithm' is referring to as
> the function takes simply "unsigned long arg"
Yes, a bit confusing, will remove it.
>
>> + * @put_queue: free a queue to the device
>> + * @start_queue: make the queue start work after get_queue
>> + * @stop_queue: make the queue stop work before put_queue
>> + * @is_q_updated: check whether the task is finished
>> + * @mask_notify: mask the task irq of queue
>> + * @mmap: mmap addresses of queue to user space
>> + * @reset: reset the WD device
> uacce device?
>
>> + * @reset_queue: reset the queue
>> + * @ioctl:   ioctl for user space users of the queue
> Extra spaces after : compared to other entries.
>
>> + */
>> +struct uacce_ops {
>> +	int (*get_available_instances)(struct uacce *uacce);
>> +	int (*get_queue)(struct uacce *uacce, unsigned long arg,
>> +	     struct uacce_queue **q);
>> +	void (*put_queue)(struct uacce_queue *q);
>> +	int (*start_queue)(struct uacce_queue *q);
>> +	void (*stop_queue)(struct uacce_queue *q);
>> +	int (*is_q_updated)(struct uacce_queue *q);
>> +	void (*mask_notify)(struct uacce_queue *q, int event_mask);
>> +	int (*mmap)(struct uacce_queue *q, struct vm_area_struct *vma,
>> +		    struct uacce_qfile_region *qfr);
>> +	int (*reset)(struct uacce *uacce);
>> +	int (*reset_queue)(struct uacce_queue *q);
>> +	long (*ioctl)(struct uacce_queue *q, unsigned int cmd,
>> +		      unsigned long arg);
>> +};
>> +
>> +struct uacce_queue {
>> +	struct uacce *uacce;
>> +	void *priv;
>> +	wait_queue_head_t wait;
>> +	int pasid;
>> +	struct iommu_sva *handle;
>> +	struct list_head list; /* share list for qfr->qs */
>> +	struct mm_struct *mm;
>> +	struct uacce_qfile_region *qfrs[UACCE_QFRT_MAX];
>> +	struct list_head q_dev;
>> +};
>> +
>> +#define UACCE_ST_INIT		0
>> +#define UACCE_ST_OPENED		1
>> +#define UACCE_ST_STARTED	2
>> +#define UACCE_ST_RST		3
> These seem to be states in a state machine, perhaps an enum
> is more suited as their actual values don't matter (I think!)
Yes, enum is  better.

Thanks

Re: [PATCH 2/2] uacce: add uacce module

[Jonathan Cameron]

On Fri, 23 Aug 2019 17:21:33 +0800
zhangfei <zhangfei.gao@linaro.org> wrote:

> Hi, Jonathan
Hi zhangfei,

> 
> Thanks for your careful review and good suggestion.
> Sorry for late response, I am checking one detail.

I have reviews on patches from years ago that I still haven't replied to ;)

> 
> On 2019/8/16 上午12:54, Jonathan Cameron wrote:
> > On Wed, 14 Aug 2019 17:34:25 +0800
> > Zhangfei Gao <zhangfei.gao@linaro.org> wrote:
> >  
> >> From: Kenneth Lee <liguozhu@hisilicon.com>
> >>
> >> Uacce is the kernel component to support WarpDrive accelerator
> >> framework. It provides register/unregister interface for device drivers
> >> to expose their hardware resource to the user space. The resource is
> >> taken as "queue" in WarpDrive.  
> > It's a bit confusing to have both the term UACCE and WarpDrive in here.
> > I'd just use the uacce name in all comments etc.  
> Yes, make sense
> >  
> >> 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.
> >>
> >> Uacce also manages unify addresses between the hardware and user space
> >> of the process. So they can share the same virtual address in the
> >> communication.
> >>
> >> Signed-off-by: Kenneth Lee <liguozhu@hisilicon.com>
> >> Signed-off-by: Zaibo Xu <xuzaibo@huawei.com>
> >> Signed-off-by: Zhou Wang <wangzhou1@hisilicon.com>
> >> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>  
> > I would strip this back to which ever case is of most interest (SVA I guess?)
> > and only think about adding support for the others if necessary at a later date.
> > (or in later patches).  
> Do you mean split the patch and send sva part first?

Either:
1) SVA only in the first series, second series can do other options.
2) Patch N for SVA only, N+1... for other features.

I don't mind which, but I want to be able to see just one case and
review that before taking into account the affect of the more complex cases.


> >> +
> >> +static int uacce_qfr_alloc_pages(struct uacce_qfile_region *qfr)
> >> +{
> >> +	int gfp_mask = GFP_ATOMIC | __GFP_ZERO;  
> > More readable to just have this inline.  
> Yes, all right.
> >  
> >> +	int i, j;
> >> +
...

> >> +static int uacce_set_iommu_domain(struct uacce *uacce)
> >> +{
> >> +	struct iommu_domain *domain;
> >> +	struct iommu_group *group;
> >> +	struct device *dev = uacce->pdev;
> >> +	bool resv_msi;
> >> +	phys_addr_t resv_msi_base = 0;
> >> +	int ret;
> >> +
> >> +	if ((uacce->flags & UACCE_DEV_NOIOMMU) ||
> >> +	    (uacce->flags & UACCE_DEV_PASID))
> >> +		return 0;
> >> +
> >> +	/*
> >> +	 * We don't support multiple register for the same dev in RFC version ,
> >> +	 * will add it in formal version  
> > So this effectively multiple complete uacce interfaces for one device.
> > Is there a known usecase for that?  
> Here is preventing one device with multiple algorithm and register 
> multi-times,
> and without sva, they can not be distinguished.

Isn't that a bug in the device driver?

> >> +	 */
> >> +	ret = class_for_each_device(uacce_class, NULL, uacce->pdev,
> >> +				    uacce_dev_match);
> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	/* allocate and attach a unmanged domain */

...

Re: [PATCH 2/2] uacce: add uacce module

[Jonathan Cameron]

On Fri, 23 Aug 2019 17:21:33 +0800
zhangfei <zhangfei.gao@linaro.org> wrote:

> Hi, Jonathan
> 
> Thanks for your careful review and good suggestion.
> Sorry for late response, I am checking one detail.
> 
> On 2019/8/16 上午12:54, Jonathan Cameron wrote:
> > On Wed, 14 Aug 2019 17:34:25 +0800
> > Zhangfei Gao <zhangfei.gao@linaro.org> wrote:
> >  
> >> From: Kenneth Lee <liguozhu@hisilicon.com>
> >>
> >> Uacce is the kernel component to support WarpDrive accelerator
> >> framework. It provides register/unregister interface for device drivers
> >> to expose their hardware resource to the user space. The resource is
> >> taken as "queue" in WarpDrive.  
> > It's a bit confusing to have both the term UACCE and WarpDrive in here.
> > I'd just use the uacce name in all comments etc.  
> Yes, make sense
> >  
> >> 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.
> >>
> >> Uacce also manages unify addresses between the hardware and user space
> >> of the process. So they can share the same virtual address in the
> >> communication.
> >>
> >> Signed-off-by: Kenneth Lee <liguozhu@hisilicon.com>
> >> Signed-off-by: Zaibo Xu <xuzaibo@huawei.com>
> >> Signed-off-by: Zhou Wang <wangzhou1@hisilicon.com>
> >> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>  
> > I would strip this back to which ever case is of most interest (SVA I guess?)
> > and only think about adding support for the others if necessary at a later date.
> > (or in later patches).  
> Do you mean split the patch and send sva part first?

Yes.  Either send them as two series with SVA only in the first one, or
a single series with SVA only in the early patches.

I want to be able to review one case first then only consider what needs
to be added for the others.

Thanks,

Jonathan

Re: [PATCH 2/2] uacce: add uacce module

[zhangfei]

On 2019/8/20 下午10:33, Greg Kroah-Hartman wrote:
> On Tue, Aug 20, 2019 at 08:36:50PM +0800, zhangfei wrote:
>> Hi, Greg
>>
>> On 2019/8/19 下午6:24, Greg Kroah-Hartman wrote:
>>>>>> +static int uacce_create_chrdev(struct uacce *uacce)
>>>>>> +{
>>>>>> +	int ret;
>>>>>> +
>>>>>> +	ret = idr_alloc(&uacce_idr, uacce, 0, 0, GFP_KERNEL);
>>>>>> +	if (ret < 0)
>>>>>> +		return ret;
>>>>>> +
>>>>> Shouldn't this function create the memory needed for this structure?
>>>>> You are relying ont he caller to do it for you, why?
>>>> I think you mean uacce structure here.
>>>> Yes, currently we count on caller to prepare uacce structure and call
>>>> uacce_register(uacce).
>>>> We still think this method is simpler, prepare uacce, register uacce.
>>>> And there are other system using the same method, like crypto
>>>> (crypto_register_acomp), nand, etc.
>>> crypto is not a subsystem to ever try to emulate :)
>>>
>>> You are creating a structure with a lifetime that you control, don't
>>> have someone else create your memory, that's almost never what you want
>>> to do.  Most all driver subsystems create their own memory chunks for
>>> what they need to do, it's a much better pattern.
>>>
>>> Especially when you get into pointer lifetime issues...
>> OK, understand now, thanks for your patience.
>> will use this instead.
>> struct uacce_interface {
>>          char name[32];
>>          unsigned int flags;
>>          struct uacce_ops *ops;
>> };
>> struct uacce *uacce_register(struct device *dev, struct uacce_interface
>> *interface);
> What?  Why do you need a structure?  A pointer to the name and the ops
> should be all that is needed, right?
We are thinking transfer structure will be more flexible.
And modify api later would be difficult, requiring many drivers modify 
together.
Currently parameters need a flag, a pointer to the name, and ops, but in 
case more requirement from future drivers usage.
Also refer usb_register_dev, sdhci_pltfm_init etc, and the structure 
para can be set as static.
> And 'dev' here is a pointer to the parent, right?  Might want to make
> that explicit in the name of the variable :)
Yes, 'dev' is parent, will change to 'pdev', thanks.
>>>>>> +
>>>>>> +static int uacce_dev_match(struct device *dev, void *data)
>>>>>> +{
>>>>>> +	if (dev->parent == data)
>>>>>> +		return -EBUSY;
>>>>> There should be in-kernel functions for this now, no need for you to
>>>>> roll your own.
>>>> Sorry, do not find this function.
>>>> Only find class_find_device, which still require match.
>>> It is in linux-next, look there...
>>>
>> Suppose you mean the funcs: device_match_name,
>> device_match_of_node,device_match_devt etc.
>> Here we need dev->parent, there still no such func.
> You should NEVER be matching on a parent.  If so, your use of the driver
> model is wrong :)
>
> Remind me to really review the use of the driver core code in your next
> submission of this series please, I think it needs it.
>
>

OK, thanks Greg.

Re: [PATCH 2/2] uacce: add uacce module

[Greg Kroah-Hartman]

On Sat, Aug 24, 2019 at 08:53:01PM +0800, zhangfei wrote:
> 
> 
> On 2019/8/20 下午10:33, Greg Kroah-Hartman wrote:
> > On Tue, Aug 20, 2019 at 08:36:50PM +0800, zhangfei wrote:
> > > Hi, Greg
> > > 
> > > On 2019/8/19 下午6:24, Greg Kroah-Hartman wrote:
> > > > > > > +static int uacce_create_chrdev(struct uacce *uacce)
> > > > > > > +{
> > > > > > > +	int ret;
> > > > > > > +
> > > > > > > +	ret = idr_alloc(&uacce_idr, uacce, 0, 0, GFP_KERNEL);
> > > > > > > +	if (ret < 0)
> > > > > > > +		return ret;
> > > > > > > +
> > > > > > Shouldn't this function create the memory needed for this structure?
> > > > > > You are relying ont he caller to do it for you, why?
> > > > > I think you mean uacce structure here.
> > > > > Yes, currently we count on caller to prepare uacce structure and call
> > > > > uacce_register(uacce).
> > > > > We still think this method is simpler, prepare uacce, register uacce.
> > > > > And there are other system using the same method, like crypto
> > > > > (crypto_register_acomp), nand, etc.
> > > > crypto is not a subsystem to ever try to emulate :)
> > > > 
> > > > You are creating a structure with a lifetime that you control, don't
> > > > have someone else create your memory, that's almost never what you want
> > > > to do.  Most all driver subsystems create their own memory chunks for
> > > > what they need to do, it's a much better pattern.
> > > > 
> > > > Especially when you get into pointer lifetime issues...
> > > OK, understand now, thanks for your patience.
> > > will use this instead.
> > > struct uacce_interface {
> > >          char name[32];
> > >          unsigned int flags;
> > >          struct uacce_ops *ops;
> > > };
> > > struct uacce *uacce_register(struct device *dev, struct uacce_interface
> > > *interface);
> > What?  Why do you need a structure?  A pointer to the name and the ops
> > should be all that is needed, right?
> We are thinking transfer structure will be more flexible.
> And modify api later would be difficult, requiring many drivers modify
> together.
> Currently parameters need a flag, a pointer to the name, and ops, but in
> case more requirement from future drivers usage.
> Also refer usb_register_dev, sdhci_pltfm_init etc, and the structure para
> can be set as static.

Ok, I can live with that, but it is a bit more complex.  However, if you
are creating a new device structure, your "core" has to do it, do not
rely on the driver to do it for you as that is just lots of duplicated
logic everywhere.  Not to mention a reference counting nightmare.

> > And 'dev' here is a pointer to the parent, right?  Might want to make
> > that explicit in the name of the variable :)
> Yes, 'dev' is parent, will change to 'pdev', thanks.

Use "struct device *parent" it's much more obvious that way and does not
look like crazy hungarian notation :)

thanks,

greg k-h

Re: [PATCH 2/2] uacce: add uacce module

[Kenneth Lee]

On Wed, Aug 21, 2019 at 09:05:42AM -0700, Greg Kroah-Hartman wrote:
> Date: Wed, 21 Aug 2019 09:05:42 -0700
> From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> To: zhangfei <zhangfei.gao@linaro.org>
> CC: Arnd Bergmann <arnd@arndb.de>, linux-accelerators@lists.ozlabs.org,
>  linux-kernel@vger.kernel.org, Kenneth Lee <liguozhu@hisilicon.com>, Zaibo
>  Xu <xuzaibo@huawei.com>, Zhou Wang <wangzhou1@hisilicon.com>
> Subject: Re: [PATCH 2/2] uacce: add uacce module
> User-Agent: Mutt/1.12.1 (2019-06-15)
> Message-ID: <20190821160542.GA14760@kroah.com>
> 
> On Wed, Aug 21, 2019 at 10:30:22PM +0800, zhangfei wrote:
> > 
> > 
> > On 2019/8/21 下午5:17, Greg Kroah-Hartman wrote:
> > > On Wed, Aug 21, 2019 at 03:21:18PM +0800, zhangfei.gao@foxmail.com wrote:
> > > > Hi, Greg
> > > > 
> > > > On 2019/8/21 上午12:59, Greg Kroah-Hartman wrote:
> > > > > On Tue, Aug 20, 2019 at 09:08:55PM +0800, zhangfei wrote:
> > > > > > On 2019/8/15 下午10:13, Greg Kroah-Hartman wrote:
> > > > > > > On Wed, Aug 14, 2019 at 05:34:25PM +0800, Zhangfei Gao wrote:
> > > > > > > > +int uacce_register(struct uacce *uacce)
> > > > > > > > +{
> > > > > > > > +	int ret;
> > > > > > > > +
> > > > > > > > +	if (!uacce->pdev) {
> > > > > > > > +		pr_debug("uacce parent device not set\n");
> > > > > > > > +		return -ENODEV;
> > > > > > > > +	}
> > > > > > > > +
> > > > > > > > +	if (uacce->flags & UACCE_DEV_NOIOMMU) {
> > > > > > > > +		add_taint(TAINT_CRAP, LOCKDEP_STILL_OK);
> > > > > > > > +		dev_warn(uacce->pdev,
> > > > > > > > +			 "Register to noiommu mode, which export kernel data to user space and may vulnerable to attack");
> > > > > > > > +	}
> > > > > > > THat is odd, why even offer this feature then if it is a major issue?
> > > > > > UACCE_DEV_NOIOMMU maybe confusing here.
> > > > > > 
> > > > > > In this mode, app use ioctl to get dma_handle from dma_alloc_coherent.
> > > > > That's odd, why not use the other default apis to do that?
> > > > > 
> > > > > > It does not matter iommu is enabled or not.
> > > > > > In case iommu is disabled, it maybe dangerous to kernel, so we added warning here, is it required?
> > > > > You should use the other documentated apis for this, don't create your
> > > > > own.
> > > > I am sorry, not understand here.
> > > > Do you mean there is a standard ioctl or standard api in user space, it can
> > > > get dma_handle from dma_alloc_coherent from kernel?
> > > There should be a standard way to get such a handle from userspace
> > > today.  Isn't that what the ion interface does?  DRM also does this, as
> > > does UIO I think.
> > Thanks Greg,
> > Still not find it, will do more search.
> > But this may introduce dependency in our lib, like depend on ion?
> > > Do you have a spec somewhere that shows exactly what you are trying to
> > > do here, along with example userspace code?  It's hard to determine it
> > > given you only have one "half" of the code here and no users of the apis
> > > you are creating.
> > > 
> > The purpose is doing dma in user space.
> 
> Oh no, please no.  Are you _SURE_ you want to do this?
> 
> Again, look at how ION does this and how the DMAbuff stuff is replacing
> it.  Use that api please instead, otherwise you will get it wrong and we
> don't want to duplicate efforts.
> 
> thanks,
> 
> greg k-h

Dear Greg. I wrote a blog to explain the intention of WarpDrive here:
https://zhuanlan.zhihu.com/p/79680889.

Sharing data is not our intention, Sharing address is. NOIOMMU mode is just a
temporary solution to let some hardware which does not care the security issue
to try WarpDrive for the first step. Some user do not care this much in embedded
scenario. We saw VFIO use the same model so we also want to make a try. If you
insist this is risky, we can remove it.

Thanks.

-- 
			-Kenneth(Hisilicon)

Re: [PATCH 0/2] A General Accelerator Framework, WarpDrive

[Kenneth Lee]

On Thu, Aug 15, 2019 at 01:04:24PM -0400, Jerome Glisse wrote:
> Date: Thu, 15 Aug 2019 13:04:24 -0400
> From: Jerome Glisse <jglisse@redhat.com>
> To: Zhangfei Gao <zhangfei.gao@linaro.org>
> CC: linux-accelerators@lists.ozlabs.org, Greg Kroah-Hartman
>  <gregkh@linuxfoundation.org>, linux-kernel@vger.kernel.org, Arnd Bergmann
>  <arnd@arndb.de>
> Subject: Re: [PATCH 0/2] A General Accelerator Framework, WarpDrive
> User-Agent: Mutt/1.11.3 (2019-02-01)
> Message-ID: <20190815170424.GA30916@redhat.com>
> 
> On Wed, Aug 14, 2019 at 05:34:23PM +0800, Zhangfei Gao wrote:
> > *WarpDrive* is a general accelerator framework for the user application to
> > access the hardware without going through the kernel in data path.
> > 
> > WarpDrive is the name for the whole framework. The component in kernel
> > is called uacce, meaning "Unified/User-space-access-intended Accelerator
> > Framework". It makes use of the capability of IOMMU to maintain a
> > unified virtual address space between the hardware and the process.
> > 
> > WarpDrive is intended to be used with Jean Philippe Brucker's SVA
> > patchset[1], which enables IO side page fault and PASID support. 
> > We have keep verifying with Jean's sva/current [2]
> > We also keep verifying with Eric's SMMUv3 Nested Stage patch [3]
> > 
> > This series and related zip & qm driver as well as dummy driver for qemu test:
> > https://github.com/Linaro/linux-kernel-warpdrive/tree/5.3-rc1-warpdrive-v1
> > zip driver already been upstreamed.
> > zip supporting uacce will be the next step.
> > 
> > The library and user application:
> > https://github.com/Linaro/warpdrive/tree/wdprd-v1-current
> 
> Do we want a new framework ? I think that is the first question that
> should be answer here. Accelerator are in many forms and so far they
> never have been enough commonality to create a framework, even GPUs
> with the drm is an example of that, drm only offer share framework
> for the modesetting part of the GPU (as thankfully monitor connector
> are not specific to GPU brands :))
> 
> FPGA is another example the only common code expose to userspace is
> about bitstream management AFAIK.
> 
> I would argue that a framework should only be created once there is
> enough devices with same userspace API. Meanwhile you can provide
> in kernel helper that allow driver to expose same API. If after a
> while we have enough device driver which all use that same in kernel
> helpers API then it will a good time to introduce a new framework.
> Meanwhile this will allow individual device driver to tinker with
> their API and maybe get to something useful to more devices in the
> end.
> 
> Note that what i propose also allow userspace code sharing for all
> driver that use the same in kernel helper.
> 
> Cheers,
> Jérôme

Hi, Jerome, I explain the idea here: https://zhuanlan.zhihu.com/p/79680889. We
think this is a comment requirement for eveybody. Hope this can help the
discussion. Thanks

-- 
			-Kenneth(Hisilicon)

Re: [PATCH 2/2] uacce: add uacce module

[Greg Kroah-Hartman]

On Mon, Aug 26, 2019 at 12:10:42PM +0800, Kenneth Lee wrote:
> On Wed, Aug 21, 2019 at 09:05:42AM -0700, Greg Kroah-Hartman wrote:
> > Date: Wed, 21 Aug 2019 09:05:42 -0700
> > From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > To: zhangfei <zhangfei.gao@linaro.org>
> > CC: Arnd Bergmann <arnd@arndb.de>, linux-accelerators@lists.ozlabs.org,
> >  linux-kernel@vger.kernel.org, Kenneth Lee <liguozhu@hisilicon.com>, Zaibo
> >  Xu <xuzaibo@huawei.com>, Zhou Wang <wangzhou1@hisilicon.com>
> > Subject: Re: [PATCH 2/2] uacce: add uacce module
> > User-Agent: Mutt/1.12.1 (2019-06-15)
> > Message-ID: <20190821160542.GA14760@kroah.com>
> > 
> > On Wed, Aug 21, 2019 at 10:30:22PM +0800, zhangfei wrote:
> > > 
> > > 
> > > On 2019/8/21 下午5:17, Greg Kroah-Hartman wrote:
> > > > On Wed, Aug 21, 2019 at 03:21:18PM +0800, zhangfei.gao@foxmail.com wrote:
> > > > > Hi, Greg
> > > > > 
> > > > > On 2019/8/21 上午12:59, Greg Kroah-Hartman wrote:
> > > > > > On Tue, Aug 20, 2019 at 09:08:55PM +0800, zhangfei wrote:
> > > > > > > On 2019/8/15 下午10:13, Greg Kroah-Hartman wrote:
> > > > > > > > On Wed, Aug 14, 2019 at 05:34:25PM +0800, Zhangfei Gao wrote:
> > > > > > > > > +int uacce_register(struct uacce *uacce)
> > > > > > > > > +{
> > > > > > > > > +	int ret;
> > > > > > > > > +
> > > > > > > > > +	if (!uacce->pdev) {
> > > > > > > > > +		pr_debug("uacce parent device not set\n");
> > > > > > > > > +		return -ENODEV;
> > > > > > > > > +	}
> > > > > > > > > +
> > > > > > > > > +	if (uacce->flags & UACCE_DEV_NOIOMMU) {
> > > > > > > > > +		add_taint(TAINT_CRAP, LOCKDEP_STILL_OK);
> > > > > > > > > +		dev_warn(uacce->pdev,
> > > > > > > > > +			 "Register to noiommu mode, which export kernel data to user space and may vulnerable to attack");
> > > > > > > > > +	}
> > > > > > > > THat is odd, why even offer this feature then if it is a major issue?
> > > > > > > UACCE_DEV_NOIOMMU maybe confusing here.
> > > > > > > 
> > > > > > > In this mode, app use ioctl to get dma_handle from dma_alloc_coherent.
> > > > > > That's odd, why not use the other default apis to do that?
> > > > > > 
> > > > > > > It does not matter iommu is enabled or not.
> > > > > > > In case iommu is disabled, it maybe dangerous to kernel, so we added warning here, is it required?
> > > > > > You should use the other documentated apis for this, don't create your
> > > > > > own.
> > > > > I am sorry, not understand here.
> > > > > Do you mean there is a standard ioctl or standard api in user space, it can
> > > > > get dma_handle from dma_alloc_coherent from kernel?
> > > > There should be a standard way to get such a handle from userspace
> > > > today.  Isn't that what the ion interface does?  DRM also does this, as
> > > > does UIO I think.
> > > Thanks Greg,
> > > Still not find it, will do more search.
> > > But this may introduce dependency in our lib, like depend on ion?
> > > > Do you have a spec somewhere that shows exactly what you are trying to
> > > > do here, along with example userspace code?  It's hard to determine it
> > > > given you only have one "half" of the code here and no users of the apis
> > > > you are creating.
> > > > 
> > > The purpose is doing dma in user space.
> > 
> > Oh no, please no.  Are you _SURE_ you want to do this?
> > 
> > Again, look at how ION does this and how the DMAbuff stuff is replacing
> > it.  Use that api please instead, otherwise you will get it wrong and we
> > don't want to duplicate efforts.
> > 
> > thanks,
> > 
> > greg k-h
> 
> Dear Greg. I wrote a blog to explain the intention of WarpDrive here:
> https://zhuanlan.zhihu.com/p/79680889.

Putting that information into the changelog and kernel documentation is
a much better idea than putting it there.

> Sharing data is not our intention, Sharing address is. NOIOMMU mode is just a
> temporary solution to let some hardware which does not care the security issue
> to try WarpDrive for the first step. Some user do not care this much in embedded
> scenario. We saw VFIO use the same model so we also want to make a try. If you
> insist this is risky, we can remove it.

Why not just use vfio then?

And yes, for now, please remove it, if you are not requiring it.

thanks,

greg k-h

Re: [PATCH 2/2] uacce: add uacce module

[Kenneth Lee]

On Mon, Aug 26, 2019 at 06:29:10AM +0200, Greg Kroah-Hartman wrote:
> Date: Mon, 26 Aug 2019 06:29:10 +0200
> From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> To: Kenneth Lee <liguozhu@hisilicon.com>
> CC: zhangfei <zhangfei.gao@linaro.org>, Arnd Bergmann <arnd@arndb.de>,
>  linux-accelerators@lists.ozlabs.org, linux-kernel@vger.kernel.org, Zaibo
>  Xu <xuzaibo@huawei.com>, Zhou Wang <wangzhou1@hisilicon.com>
> Subject: Re: [PATCH 2/2] uacce: add uacce module
> User-Agent: Mutt/1.12.1 (2019-06-15)
> Message-ID: <20190826042910.GA26547@kroah.com>
> 
> On Mon, Aug 26, 2019 at 12:10:42PM +0800, Kenneth Lee wrote:
> > On Wed, Aug 21, 2019 at 09:05:42AM -0700, Greg Kroah-Hartman wrote:
> > > Date: Wed, 21 Aug 2019 09:05:42 -0700
> > > From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > To: zhangfei <zhangfei.gao@linaro.org>
> > > CC: Arnd Bergmann <arnd@arndb.de>, linux-accelerators@lists.ozlabs.org,
> > >  linux-kernel@vger.kernel.org, Kenneth Lee <liguozhu@hisilicon.com>, Zaibo
> > >  Xu <xuzaibo@huawei.com>, Zhou Wang <wangzhou1@hisilicon.com>
> > > Subject: Re: [PATCH 2/2] uacce: add uacce module
> > > User-Agent: Mutt/1.12.1 (2019-06-15)
> > > Message-ID: <20190821160542.GA14760@kroah.com>
> > > 
> > > On Wed, Aug 21, 2019 at 10:30:22PM +0800, zhangfei wrote:
> > > > 
> > > > 
> > > > On 2019/8/21 下午5:17, Greg Kroah-Hartman wrote:
> > > > > On Wed, Aug 21, 2019 at 03:21:18PM +0800, zhangfei.gao@foxmail.com wrote:
> > > > > > Hi, Greg
> > > > > > 
> > > > > > On 2019/8/21 上午12:59, Greg Kroah-Hartman wrote:
> > > > > > > On Tue, Aug 20, 2019 at 09:08:55PM +0800, zhangfei wrote:
> > > > > > > > On 2019/8/15 下午10:13, Greg Kroah-Hartman wrote:
> > > > > > > > > On Wed, Aug 14, 2019 at 05:34:25PM +0800, Zhangfei Gao wrote:
> > > > > > > > > > +int uacce_register(struct uacce *uacce)
> > > > > > > > > > +{
> > > > > > > > > > +	int ret;
> > > > > > > > > > +
> > > > > > > > > > +	if (!uacce->pdev) {
> > > > > > > > > > +		pr_debug("uacce parent device not set\n");
> > > > > > > > > > +		return -ENODEV;
> > > > > > > > > > +	}
> > > > > > > > > > +
> > > > > > > > > > +	if (uacce->flags & UACCE_DEV_NOIOMMU) {
> > > > > > > > > > +		add_taint(TAINT_CRAP, LOCKDEP_STILL_OK);
> > > > > > > > > > +		dev_warn(uacce->pdev,
> > > > > > > > > > +			 "Register to noiommu mode, which export kernel data to user space and may vulnerable to attack");
> > > > > > > > > > +	}
> > > > > > > > > THat is odd, why even offer this feature then if it is a major issue?
> > > > > > > > UACCE_DEV_NOIOMMU maybe confusing here.
> > > > > > > > 
> > > > > > > > In this mode, app use ioctl to get dma_handle from dma_alloc_coherent.
> > > > > > > That's odd, why not use the other default apis to do that?
> > > > > > > 
> > > > > > > > It does not matter iommu is enabled or not.
> > > > > > > > In case iommu is disabled, it maybe dangerous to kernel, so we added warning here, is it required?
> > > > > > > You should use the other documentated apis for this, don't create your
> > > > > > > own.
> > > > > > I am sorry, not understand here.
> > > > > > Do you mean there is a standard ioctl or standard api in user space, it can
> > > > > > get dma_handle from dma_alloc_coherent from kernel?
> > > > > There should be a standard way to get such a handle from userspace
> > > > > today.  Isn't that what the ion interface does?  DRM also does this, as
> > > > > does UIO I think.
> > > > Thanks Greg,
> > > > Still not find it, will do more search.
> > > > But this may introduce dependency in our lib, like depend on ion?
> > > > > Do you have a spec somewhere that shows exactly what you are trying to
> > > > > do here, along with example userspace code?  It's hard to determine it
> > > > > given you only have one "half" of the code here and no users of the apis
> > > > > you are creating.
> > > > > 
> > > > The purpose is doing dma in user space.
> > > 
> > > Oh no, please no.  Are you _SURE_ you want to do this?
> > > 
> > > Again, look at how ION does this and how the DMAbuff stuff is replacing
> > > it.  Use that api please instead, otherwise you will get it wrong and we
> > > don't want to duplicate efforts.
> > > 
> > > thanks,
> > > 
> > > greg k-h
> > 
> > Dear Greg. I wrote a blog to explain the intention of WarpDrive here:
> > https://zhuanlan.zhihu.com/p/79680889.
> 
> Putting that information into the changelog and kernel documentation is
> a much better idea than putting it there.

Yes, will do. Thank you.

> 
> > Sharing data is not our intention, Sharing address is. NOIOMMU mode is just a
> > temporary solution to let some hardware which does not care the security issue
> > to try WarpDrive for the first step. Some user do not care this much in embedded
> > scenario. We saw VFIO use the same model so we also want to make a try. If you
> > insist this is risky, we can remove it.
> 
> Why not just use vfio then?

We tried that in previous patches. But it needs to create unnecessary mdev to
fulfill the requirement. So we discard it after the discussion in lkml.

> 
> And yes, for now, please remove it, if you are not requiring it.
> 

We do require it;) But surely we can kept it in our own branch but push only the
main function. Thank you.

> thanks,
> 
> greg k-h

-- 
			-Kenneth(Hisilicon)

================================================================================
本邮件及其附件含有华为公司的保密信息，仅限于发送给上面地址中列出的个人或群组。禁
止任何其他人以任何形式使用（包括但不限于全部或部分地泄露、复制、或散发）本邮件中
的信息。如果您错收了本邮件，请您立即电话或邮件通知发件人并删除本邮件！
This e-mail and its attachments contain confidential information from HUAWEI,
which is intended only for the person or entity whose address is listed above.
Any use of the 
information contained herein in any way (including, but not limited to, total or
partial disclosure, reproduction, or dissemination) by persons other than the
intended 
recipient(s) is prohibited. If you receive this e-mail in error, please notify
the sender by phone or email immediately and delete it!

Re: [PATCH 2/2] uacce: add uacce module

[Greg Kroah-Hartman]

On Tue, Aug 27, 2019 at 07:42:08PM +0800, Kenneth Lee wrote:
> On Mon, Aug 26, 2019 at 06:29:10AM +0200, Greg Kroah-Hartman wrote:
> > Date: Mon, 26 Aug 2019 06:29:10 +0200
> > From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > To: Kenneth Lee <liguozhu@hisilicon.com>
> > CC: zhangfei <zhangfei.gao@linaro.org>, Arnd Bergmann <arnd@arndb.de>,
> >  linux-accelerators@lists.ozlabs.org, linux-kernel@vger.kernel.org, Zaibo
> >  Xu <xuzaibo@huawei.com>, Zhou Wang <wangzhou1@hisilicon.com>
> > Subject: Re: [PATCH 2/2] uacce: add uacce module
> > User-Agent: Mutt/1.12.1 (2019-06-15)
> > Message-ID: <20190826042910.GA26547@kroah.com>
> > 
> > On Mon, Aug 26, 2019 at 12:10:42PM +0800, Kenneth Lee wrote:
> > > On Wed, Aug 21, 2019 at 09:05:42AM -0700, Greg Kroah-Hartman wrote:
> > > > Date: Wed, 21 Aug 2019 09:05:42 -0700
> > > > From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > > To: zhangfei <zhangfei.gao@linaro.org>
> > > > CC: Arnd Bergmann <arnd@arndb.de>, linux-accelerators@lists.ozlabs.org,
> > > >  linux-kernel@vger.kernel.org, Kenneth Lee <liguozhu@hisilicon.com>, Zaibo
> > > >  Xu <xuzaibo@huawei.com>, Zhou Wang <wangzhou1@hisilicon.com>
> > > > Subject: Re: [PATCH 2/2] uacce: add uacce module
> > > > User-Agent: Mutt/1.12.1 (2019-06-15)
> > > > Message-ID: <20190821160542.GA14760@kroah.com>
> > > > 
> > > > On Wed, Aug 21, 2019 at 10:30:22PM +0800, zhangfei wrote:
> > > > > 
> > > > > 
> > > > > On 2019/8/21 下午5:17, Greg Kroah-Hartman wrote:
> > > > > > On Wed, Aug 21, 2019 at 03:21:18PM +0800, zhangfei.gao@foxmail.com wrote:
> > > > > > > Hi, Greg
> > > > > > > 
> > > > > > > On 2019/8/21 上午12:59, Greg Kroah-Hartman wrote:
> > > > > > > > On Tue, Aug 20, 2019 at 09:08:55PM +0800, zhangfei wrote:
> > > > > > > > > On 2019/8/15 下午10:13, Greg Kroah-Hartman wrote:
> > > > > > > > > > On Wed, Aug 14, 2019 at 05:34:25PM +0800, Zhangfei Gao wrote:
> > > > > > > > > > > +int uacce_register(struct uacce *uacce)
> > > > > > > > > > > +{
> > > > > > > > > > > +	int ret;
> > > > > > > > > > > +
> > > > > > > > > > > +	if (!uacce->pdev) {
> > > > > > > > > > > +		pr_debug("uacce parent device not set\n");
> > > > > > > > > > > +		return -ENODEV;
> > > > > > > > > > > +	}
> > > > > > > > > > > +
> > > > > > > > > > > +	if (uacce->flags & UACCE_DEV_NOIOMMU) {
> > > > > > > > > > > +		add_taint(TAINT_CRAP, LOCKDEP_STILL_OK);
> > > > > > > > > > > +		dev_warn(uacce->pdev,
> > > > > > > > > > > +			 "Register to noiommu mode, which export kernel data to user space and may vulnerable to attack");
> > > > > > > > > > > +	}
> > > > > > > > > > THat is odd, why even offer this feature then if it is a major issue?
> > > > > > > > > UACCE_DEV_NOIOMMU maybe confusing here.
> > > > > > > > > 
> > > > > > > > > In this mode, app use ioctl to get dma_handle from dma_alloc_coherent.
> > > > > > > > That's odd, why not use the other default apis to do that?
> > > > > > > > 
> > > > > > > > > It does not matter iommu is enabled or not.
> > > > > > > > > In case iommu is disabled, it maybe dangerous to kernel, so we added warning here, is it required?
> > > > > > > > You should use the other documentated apis for this, don't create your
> > > > > > > > own.
> > > > > > > I am sorry, not understand here.
> > > > > > > Do you mean there is a standard ioctl or standard api in user space, it can
> > > > > > > get dma_handle from dma_alloc_coherent from kernel?
> > > > > > There should be a standard way to get such a handle from userspace
> > > > > > today.  Isn't that what the ion interface does?  DRM also does this, as
> > > > > > does UIO I think.
> > > > > Thanks Greg,
> > > > > Still not find it, will do more search.
> > > > > But this may introduce dependency in our lib, like depend on ion?
> > > > > > Do you have a spec somewhere that shows exactly what you are trying to
> > > > > > do here, along with example userspace code?  It's hard to determine it
> > > > > > given you only have one "half" of the code here and no users of the apis
> > > > > > you are creating.
> > > > > > 
> > > > > The purpose is doing dma in user space.
> > > > 
> > > > Oh no, please no.  Are you _SURE_ you want to do this?
> > > > 
> > > > Again, look at how ION does this and how the DMAbuff stuff is replacing
> > > > it.  Use that api please instead, otherwise you will get it wrong and we
> > > > don't want to duplicate efforts.
> > > > 
> > > > thanks,
> > > > 
> > > > greg k-h
> > > 
> > > Dear Greg. I wrote a blog to explain the intention of WarpDrive here:
> > > https://zhuanlan.zhihu.com/p/79680889.
> > 
> > Putting that information into the changelog and kernel documentation is
> > a much better idea than putting it there.
> 
> Yes, will do. Thank you.
> 
> > 
> > > Sharing data is not our intention, Sharing address is. NOIOMMU mode is just a
> > > temporary solution to let some hardware which does not care the security issue
> > > to try WarpDrive for the first step. Some user do not care this much in embedded
> > > scenario. We saw VFIO use the same model so we also want to make a try. If you
> > > insist this is risky, we can remove it.
> > 
> > Why not just use vfio then?
> 
> We tried that in previous patches. But it needs to create unnecessary mdev to
> fulfill the requirement. So we discard it after the discussion in lkml.

Any pointers to that discussion?

thanks,

greg k-h