linux-crypto.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v7 0/3] Add uacce module for Accelerator
@ 2019-10-29  6:40 Zhangfei Gao
  2019-10-29  6:40 ` [PATCH v7 1/3] uacce: Add documents for uacce Zhangfei Gao
                   ` (2 more replies)
  0 siblings, 3 replies; 16+ messages in thread
From: Zhangfei Gao @ 2019-10-29  6:40 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Arnd Bergmann, Herbert Xu, jonathan.cameron,
	grant.likely, jean-philippe, Jerome Glisse, ilias.apalodimas,
	francois.ozog, kenneth-lee-2012, Wangzhou, haojian . zhuang,
	guodong.xu
  Cc: linux-accelerators, linux-kernel, linux-crypto, iommu, Zhangfei Gao

Uacce (Unified/User-space-access-intended Accelerator Framework) targets to
provide Shared Virtual Addressing (SVA) between accelerators and processes.
So accelerator can access any data structure of the main cpu.
This differs from the data sharing between cpu and io device, which share
data content rather than address.
Because of unified address, hardware and user space of process can share
the same virtual address in the communication.

Uacce 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
https://github.com/Linaro/linux-kernel-warpdrive/tree/5.4-rc4-uacce-v7

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

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

Change History:
v7:
As suggested by Jean and Jerome
Only consider sva case and remove unused dma apis for the first patch.
Also add mm_exit for sva and vm_ops.close etc


v6: https://lkml.org/lkml/2019/10/16/231
Change sys qfrs_size to different file, suggested by Jonathan
Fix crypto daily build issue and based on crypto code base, also 5.4-rc1.

v5: https://lkml.org/lkml/2019/10/14/74
Add an example patch using the uacce interface, suggested by Greg
0003-crypto-hisilicon-register-zip-engine-to-uacce.patch

v4: https://lkml.org/lkml/2019/9/17/116
Based on 5.4-rc1
Considering other driver integrating uacce, 
if uacce not compiled, uacce_register return error and uacce_unregister is empty.
Simplify uacce flag: UACCE_DEV_SVA.
Address Greg's comments: 
Fix state machine, remove potential syslog triggered from user space etc.

v3: https://lkml.org/lkml/2019/9/2/990
Recommended by Greg, use sturct uacce_device instead of struct uacce,
and use struct *cdev in struct uacce_device, as a result, 
cdev can be released by itself when refcount decreased to 0.
So the two structures are decoupled and self-maintained by themsleves.
Also add dev.release for put_device.

v2: https://lkml.org/lkml/2019/8/28/565
Address comments from Greg and Jonathan
Modify interface uacce_register
Drop noiommu mode first

v1: https://lkml.org/lkml/2019/8/14/277
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

RFC3:
https://lkml.org/lkml/2018/11/12/1951

RFC2:
https://lwn.net/Articles/763990/


Background of why Uacce:
Von Neumann processor is not good at general data manipulation.
It is designed for control-bound rather than data-bound application.
The latter need less control path facility and more/specific ALUs.
So there are more and more heterogeneous processors, such as
encryption/decryption accelerators, TPUs, or
EDGE (Explicated Data Graph Execution) processors, introduced to gain
better performance or power efficiency for particular applications
these days.

There are generally two ways to make use of these heterogeneous processors:

The first is to make them co-processors, just like FPU.
This is good for some application but it has its own cons:
It changes the ISA set permanently.
You must save all state elements when the process is switched out.
But most data-bound processors have a huge set of state elements.
It makes the kernel scheduler more complex.

The second is Accelerator.
It is taken as a IO device from the CPU's point of view
(but it need not to be physically). The process, running on CPU,
hold a context of the accelerator and send instructions to it as if
it calls a function or thread running with FPU.
The context is bound with the processor itself.
So the state elements remain in the hardware context until
the context is released.

We believe this is the core feature of an "Accelerator" vs. Co-processor
or other heterogeneous processors.

The intention of Uacce is to provide the basic facility to backup
this scenario. Its first step is to make sure the accelerator and process
can share the same address space. So the accelerator ISA can directly
address any data structure of the main CPU.
This differs from the data sharing between CPU and IO device,
which share data content rather than address.
So it is different comparing to the other DMA libraries.

In the future, we may add more facility to support linking accelerator
library to the main application, or managing the accelerator context as
special thread.
But no matter how, this can be a solid start point for new processor
to be used as an "accelerator" as this is the essential requirement.


The Fork Scenario
=================
For a process with allocated queues and shared memory, what happen if it forks
a child?

The fd of the queue is duplicated on fork, but requests sent from the child
process are 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 Uacce 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 Uacce 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 functions in framework level.

But Uacce 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 Uacce. 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 a 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 [1])

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] https://lwn.net/Articles/774411/

Kenneth Lee (2):
  uacce: Add documents for uacce
  uacce: add uacce driver

Zhangfei Gao (1):
  crypto: hisilicon - register zip engine to uacce

 Documentation/ABI/testing/sysfs-driver-uacce |  53 +++
 Documentation/misc-devices/uacce.rst         | 160 ++++++++
 drivers/crypto/hisilicon/qm.c                | 253 +++++++++++-
 drivers/crypto/hisilicon/qm.h                |  13 +-
 drivers/crypto/hisilicon/zip/zip_main.c      |  39 +-
 drivers/misc/Kconfig                         |   1 +
 drivers/misc/Makefile                        |   1 +
 drivers/misc/uacce/Kconfig                   |  13 +
 drivers/misc/uacce/Makefile                  |   2 +
 drivers/misc/uacce/uacce.c                   | 574 +++++++++++++++++++++++++++
 include/linux/uacce.h                        | 163 ++++++++
 include/uapi/misc/uacce/qm.h                 |  23 ++
 include/uapi/misc/uacce/uacce.h              |  38 ++
 13 files changed, 1297 insertions(+), 36 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-driver-uacce
 create mode 100644 Documentation/misc-devices/uacce.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/qm.h
 create mode 100644 include/uapi/misc/uacce/uacce.h

-- 
2.7.4


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH v7 1/3] uacce: Add documents for uacce
  2019-10-29  6:40 [PATCH v7 0/3] Add uacce module for Accelerator Zhangfei Gao
@ 2019-10-29  6:40 ` Zhangfei Gao
  2019-10-31 15:37   ` Jonathan Cameron
  2019-10-29  6:40 ` [PATCH v7 2/3] uacce: add uacce driver Zhangfei Gao
  2019-10-29  6:40 ` [PATCH v7 3/3] crypto: hisilicon - register zip engine to uacce Zhangfei Gao
  2 siblings, 1 reply; 16+ messages in thread
From: Zhangfei Gao @ 2019-10-29  6:40 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Arnd Bergmann, Herbert Xu, jonathan.cameron,
	grant.likely, jean-philippe, Jerome Glisse, ilias.apalodimas,
	francois.ozog, kenneth-lee-2012, Wangzhou, haojian . zhuang,
	guodong.xu
  Cc: linux-accelerators, linux-kernel, linux-crypto, iommu,
	Kenneth Lee, Zaibo Xu, Zhangfei Gao

From: Kenneth Lee <liguozhu@hisilicon.com>

Uacce (Unified/User-space-access-intended Accelerator Framework) is
a kernel module targets to provide Shared Virtual Addressing (SVA)
between the accelerator and process.

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/uacce.rst | 160 +++++++++++++++++++++++++++++++++++
 1 file changed, 160 insertions(+)
 create mode 100644 Documentation/misc-devices/uacce.rst

diff --git a/Documentation/misc-devices/uacce.rst b/Documentation/misc-devices/uacce.rst
new file mode 100644
index 0000000..ecd5d8b
--- /dev/null
+++ b/Documentation/misc-devices/uacce.rst
@@ -0,0 +1,160 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+Introduction of Uacce
+=========================
+
+Uacce (Unified/User-space-access-intended Accelerator Framework) targets to
+provide Shared Virtual Addressing (SVA) between accelerators and processes.
+So accelerator can access any data structure of the main cpu.
+This differs from the data sharing between cpu and io device, which share
+data content rather than address.
+Because of the unified address, hardware and user space of process can
+share the same virtual address in the communication.
+Uacce takes the hardware accelerator as a heterogeneous processor, while
+IOMMU share the same CPU page tables and as a result the same translation
+from va to pa.
+
+	 __________________________       __________________________
+	|                          |     |                          |
+	|  User application (CPU)  |     |   Hardware Accelerator   |
+	|__________________________|     |__________________________|
+
+	             |                                 |
+	             | va                              | va
+	             V                                 V
+                 __________                        __________
+                |          |                      |          |
+                |   MMU    |                      |  IOMMU   |
+                |__________|                      |__________|
+		     |                                 |
+	             |                                 |
+	             V pa                              V pa
+		 _______________________________________
+		|                                       |
+		|              Memory                   |
+		|_______________________________________|
+
+
+
+Architecture
+------------
+
+Uacce is the kernel module, taking charge of iommu and address sharing.
+The user drivers and libraries are called WarpDrive.
+
+The uacce device, built around the IOMMU SVA API, can access multiple
+address spaces, including the one without PASID.
+
+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) |
+                                                                 |___________________|
+
+
+How does it work
+================
+
+Uacce 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 accelerator device present itself as an 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).
+
+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: qfrt type
+ * @UACCE_QFRT_MMIO: device mmio region
+ * @UACCE_QFRT_DUS: device user share region
+ */
+enum uacce_qfrt {
+	UACCE_QFRT_MMIO = 0,
+	UACCE_QFRT_DUS = 1,
+};
+
+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 user share region is used for share data buffer between user process
+and device.
+
+
+The Uacce register API
+-----------------------
+The register API is defined in uacce.h.
+
+struct uacce_interface {
+	char name[UACCE_MAX_NAME_SIZE];
+	enum uacce_dev_flag flags;
+	struct uacce_ops *ops;
+};
+
+According to the IOMMU capability, uacce_interface flags can be:
+
+/**
+ * enum uacce_dev_flag: Device flags:
+ * @UACCE_DEV_SVA: Shared Virtual Addresses
+ *		   Support PASID
+ *		   Support device page faults (PCI PRI or SMMU Stall)
+ */
+enum uacce_dev_flag {
+	UACCE_DEV_SVA = BIT(0),
+};
+
+struct uacce_device *uacce_register(struct device *parent,
+				    struct uacce_interface *interface);
+void uacce_unregister(struct uacce_device *uacce);
+
+uacce_register results can be:
+a. If uacce module is not compiled, ERR_PTR(-ENODEV)
+b. Succeed with the desired flags
+c. Succeed with the negotiated flags, for example
+   uacce_interface.flags = UACCE_DEV_SVA but uacce->flags = ~UACCE_DEV_SVA
+So user driver need check return value as well as the negotiated uacce->flags.
+
+
+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.
+More details in Documentation/ABI/testing/sysfs-driver-uacce.
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v7 2/3] uacce: add uacce driver
  2019-10-29  6:40 [PATCH v7 0/3] Add uacce module for Accelerator Zhangfei Gao
  2019-10-29  6:40 ` [PATCH v7 1/3] uacce: Add documents for uacce Zhangfei Gao
@ 2019-10-29  6:40 ` Zhangfei Gao
  2019-10-31 17:13   ` Jonathan Cameron
  2019-11-05 11:48   ` Jean-Philippe Brucker
  2019-10-29  6:40 ` [PATCH v7 3/3] crypto: hisilicon - register zip engine to uacce Zhangfei Gao
  2 siblings, 2 replies; 16+ messages in thread
From: Zhangfei Gao @ 2019-10-29  6:40 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Arnd Bergmann, Herbert Xu, jonathan.cameron,
	grant.likely, jean-philippe, Jerome Glisse, ilias.apalodimas,
	francois.ozog, kenneth-lee-2012, Wangzhou, haojian . zhuang,
	guodong.xu
  Cc: linux-accelerators, linux-kernel, linux-crypto, iommu,
	Kenneth Lee, Zaibo Xu, Zhangfei Gao

From: Kenneth Lee <liguozhu@hisilicon.com>

Uacce (Unified/User-space-access-intended Accelerator Framework) targets to
provide Shared Virtual Addressing (SVA) between accelerators and processes.
So accelerator can access any data structure of the main cpu.
This differs from the data sharing between cpu and io device, which share
data content rather than address.
Since unified address, hardware and user space of process can share the
same virtual address in the communication.

Uacce create a chrdev for every registration, the queue is allocated to
the process when the chrdev is opened. Then the process can access the
hardware resource by interact with the queue file. By mmap the queue
file space to user space, the process can directly put requests to the
hardware without syscall to the kernel space.

Signed-off-by: Kenneth Lee <liguozhu@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/ABI/testing/sysfs-driver-uacce |  53 +++
 drivers/misc/Kconfig                         |   1 +
 drivers/misc/Makefile                        |   1 +
 drivers/misc/uacce/Kconfig                   |  13 +
 drivers/misc/uacce/Makefile                  |   2 +
 drivers/misc/uacce/uacce.c                   | 574 +++++++++++++++++++++++++++
 include/linux/uacce.h                        | 163 ++++++++
 include/uapi/misc/uacce/uacce.h              |  38 ++
 8 files changed, 845 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-driver-uacce
 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/uacce.h

diff --git a/Documentation/ABI/testing/sysfs-driver-uacce b/Documentation/ABI/testing/sysfs-driver-uacce
new file mode 100644
index 0000000..35699dc
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-driver-uacce
@@ -0,0 +1,53 @@
+What:           /sys/class/uacce/<dev_name>/id
+Date:           Oct 2019
+KernelVersion:  5.5
+Contact:        linux-accelerators@lists.ozlabs.org
+Description:    Id of the device.
+
+What:           /sys/class/uacce/<dev_name>/api
+Date:           Oct 2019
+KernelVersion:  5.5
+Contact:        linux-accelerators@lists.ozlabs.org
+Description:    Api of the device, used by application to match the correct driver
+
+What:           /sys/class/uacce/<dev_name>/flags
+Date:           Oct 2019
+KernelVersion:  5.5
+Contact:        linux-accelerators@lists.ozlabs.org
+Description:    Attributes of the device, see UACCE_DEV_xxx flag defined in uacce.h
+
+What:           /sys/class/uacce/<dev_name>/available_instances
+Date:           Oct 2019
+KernelVersion:  5.5
+Contact:        linux-accelerators@lists.ozlabs.org
+Description:    Available instances left of the device
+
+What:           /sys/class/uacce/<dev_name>/algorithms
+Date:           Oct 2019
+KernelVersion:  5.5
+Contact:        linux-accelerators@lists.ozlabs.org
+Description:    Algorithms supported by this accelerator
+
+What:           /sys/class/uacce/<dev_name>/qfrt_mmio_size
+Date:           Oct 2019
+KernelVersion:  5.5
+Contact:        linux-accelerators@lists.ozlabs.org
+Description:    Page size of mmio region queue file
+
+What:           /sys/class/uacce/<dev_name>/qfrt_dus_size
+Date:           Oct 2019
+KernelVersion:  5.5
+Contact:        linux-accelerators@lists.ozlabs.org
+Description:    Page size of dus region queue file
+
+What:           /sys/class/uacce/<dev_name>/numa_distance
+Date:           Oct 2019
+KernelVersion:  5.5
+Contact:        linux-accelerators@lists.ozlabs.org
+Description:    Distance of device node to cpu node
+
+What:           /sys/class/uacce/<dev_name>/node_id
+Date:           Oct 2019
+KernelVersion:  5.5
+Contact:        linux-accelerators@lists.ozlabs.org
+Description:    Id of the numa node
diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index c55b637..929feb0 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -481,4 +481,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 c1860d3..9abf292 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -56,4 +56,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..5e39b60
--- /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/uacce.h
+
+	  See Documentation/misc-devices/uacce.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..2b6b038
--- /dev/null
+++ b/drivers/misc/uacce/uacce.c
@@ -0,0 +1,574 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+#include <linux/compat.h>
+#include <linux/dma-iommu.h>
+#include <linux/module.h>
+#include <linux/poll.h>
+#include <linux/uacce.h>
+
+static struct class *uacce_class;
+static dev_t uacce_devt;
+static DEFINE_MUTEX(uacce_mutex);
+static DEFINE_XARRAY_ALLOC(uacce_xa);
+
+static int uacce_start_queue(struct uacce_queue *q)
+{
+	int ret = -EINVAL;
+
+	mutex_lock(&uacce_mutex);
+
+	if (q->state != UACCE_Q_INIT)
+		goto out_with_lock;
+
+	if (q->uacce->ops->start_queue) {
+		ret = q->uacce->ops->start_queue(q);
+		if (ret < 0)
+			goto out_with_lock;
+	}
+
+	q->state = UACCE_Q_STARTED;
+	mutex_unlock(&uacce_mutex);
+
+	return 0;
+
+out_with_lock:
+	mutex_unlock(&uacce_mutex);
+	return ret;
+}
+
+static int uacce_put_queue(struct uacce_queue *q)
+{
+	struct uacce_device *uacce = q->uacce;
+
+	mutex_lock(&uacce_mutex);
+
+	if (q->state == UACCE_Q_ZOMBIE)
+		goto out;
+
+	if ((q->state == UACCE_Q_STARTED) && uacce->ops->stop_queue)
+		uacce->ops->stop_queue(q);
+
+	if ((q->state == UACCE_Q_INIT || q->state == UACCE_Q_STARTED) &&
+	     uacce->ops->put_queue)
+		uacce->ops->put_queue(q);
+
+	q->state = UACCE_Q_ZOMBIE;
+out:
+	mutex_unlock(&uacce_mutex);
+
+	return 0;
+}
+
+static long uacce_fops_unl_ioctl(struct file *filep,
+				 unsigned int cmd, unsigned long arg)
+{
+	struct uacce_queue *q = filep->private_data;
+	struct uacce_device *uacce = q->uacce;
+
+	switch (cmd) {
+	case UACCE_CMD_START_Q:
+		return uacce_start_queue(q);
+
+	case UACCE_CMD_PUT_Q:
+		return uacce_put_queue(q);
+
+	default:
+		if (!uacce->ops->ioctl)
+			return -EINVAL;
+
+		return uacce->ops->ioctl(q, cmd, arg);
+	}
+}
+
+#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_sva_exit(struct device *dev, struct iommu_sva *handle,
+			  void *data)
+{
+	struct uacce_device *uacce = data;
+	struct uacce_queue *q;
+
+	mutex_lock(&uacce->q_lock);
+	list_for_each_entry(q, &uacce->qs, list) {
+		if (q->pid == task_pid_nr(current))
+			uacce_put_queue(q);
+	}
+	mutex_unlock(&uacce->q_lock);
+
+	return 0;
+}
+
+static struct iommu_sva_ops uacce_sva_ops = {
+	.mm_exit = uacce_sva_exit,
+};
+
+static int uacce_fops_open(struct inode *inode, struct file *filep)
+{
+	struct iommu_sva *handle = NULL;
+	struct uacce_device *uacce;
+	struct uacce_queue *q;
+	int ret = 0;
+	int pasid = 0;
+
+	uacce = xa_load(&uacce_xa, iminor(inode));
+	if (!uacce)
+		return -ENODEV;
+
+	if (!try_module_get(uacce->pdev->driver->owner))
+		return -ENODEV;
+
+	q = kzalloc(sizeof(struct uacce_queue), GFP_KERNEL);
+	if (!q) {
+		ret = -ENOMEM;
+		goto out_with_module;
+	}
+
+	if (uacce->flags & UACCE_DEV_SVA) {
+		handle = iommu_sva_bind_device(uacce->pdev, current->mm, uacce);
+		if (IS_ERR(handle))
+			goto out_with_mem;
+
+		ret = iommu_sva_set_ops(handle, &uacce_sva_ops);
+		if (ret)
+			goto out_unbind;
+
+		pasid = iommu_sva_get_pasid(handle);
+		if (pasid == IOMMU_PASID_INVALID)
+			goto out_unbind;
+	}
+
+	if (uacce->ops->get_queue) {
+		ret = uacce->ops->get_queue(uacce, pasid, q);
+		if (ret < 0)
+			goto out_unbind;
+	}
+
+	q->pid = task_pid_nr(current);
+	q->pasid = pasid;
+	q->handle = handle;
+	q->uacce = uacce;
+	memset(q->qfrs, 0, sizeof(q->qfrs));
+	init_waitqueue_head(&q->wait);
+	filep->private_data = q;
+	q->state = UACCE_Q_INIT;
+
+	mutex_lock(&uacce->q_lock);
+	list_add(&q->list, &uacce->qs);
+	mutex_unlock(&uacce->q_lock);
+
+	return 0;
+
+out_unbind:
+	if (uacce->flags & UACCE_DEV_SVA)
+		iommu_sva_unbind_device(handle);
+out_with_mem:
+	kfree(q);
+out_with_module:
+	module_put(uacce->pdev->driver->owner);
+	return ret;
+}
+
+static int uacce_fops_release(struct inode *inode, struct file *filep)
+{
+	struct uacce_queue *q = filep->private_data;
+	struct uacce_device *uacce = q->uacce;
+
+	uacce_put_queue(q);
+
+	if (uacce->flags & UACCE_DEV_SVA)
+		iommu_sva_unbind_device(q->handle);
+
+	mutex_lock(&uacce->q_lock);
+	list_del(&q->list);
+	mutex_unlock(&uacce->q_lock);
+	kfree(q);
+	module_put(uacce->pdev->driver->owner);
+
+	return 0;
+}
+
+static void uacce_vma_close(struct vm_area_struct *vma)
+{
+	struct uacce_queue *q = vma->vm_private_data;
+	enum uacce_qfrt type = 0;
+
+	if (vma->vm_pgoff < UACCE_QFRT_MAX)
+		type = vma->vm_pgoff;
+
+	kfree(q->qfrs[type]);
+}
+
+static const struct vm_operations_struct uacce_vm_ops = {
+	.close = uacce_vma_close,
+};
+
+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_device *uacce = q->uacce;
+	struct uacce_qfile_region *qfr;
+	int ret = -ENOMEM;
+
+	qfr = kzalloc(sizeof(*qfr), GFP_KERNEL);
+	if (!qfr)
+		return ERR_PTR(-ENOMEM);
+
+	qfr->type = type;
+	qfr->flags = flags;
+
+	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) {
+		if (!uacce->ops->mmap) {
+			ret = -EINVAL;
+			goto err_with_qfr;
+		}
+
+		ret = uacce->ops->mmap(q, vma, qfr);
+		if (ret)
+			goto err_with_qfr;
+		return qfr;
+	}
+
+	return qfr;
+
+err_with_qfr:
+	kfree(qfr);
+	return ERR_PTR(ret);
+}
+
+static int uacce_fops_mmap(struct file *filep, struct vm_area_struct *vma)
+{
+	struct uacce_queue *q = filep->private_data;
+	struct uacce_device *uacce = q->uacce;
+	struct uacce_qfile_region *qfr;
+	enum uacce_qfrt type = 0;
+	unsigned int flags = 0;
+	int ret;
+
+	if (vma->vm_pgoff < UACCE_QFRT_MAX)
+		type = vma->vm_pgoff;
+
+	vma->vm_flags |= VM_DONTCOPY | VM_DONTEXPAND | VM_WIPEONFORK;
+	vma->vm_ops = &uacce_vm_ops;
+	vma->vm_private_data = q;
+
+	mutex_lock(&uacce_mutex);
+
+	if (q->qfrs[type]) {
+		ret = -EEXIST;
+		goto out_with_lock;
+	}
+
+	switch (type) {
+	case UACCE_QFRT_MMIO:
+		flags = UACCE_QFRF_SELFMT;
+		break;
+
+	case UACCE_QFRT_DUS:
+		if (uacce->flags & UACCE_DEV_SVA) {
+			flags = UACCE_QFRF_SELFMT;
+			break;
+		}
+		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;
+
+	mutex_unlock(&uacce_mutex);
+
+	return 0;
+
+out_with_lock:
+	mutex_unlock(&uacce_mutex);
+	return ret;
+}
+
+static __poll_t uacce_fops_poll(struct file *file, poll_table *wait)
+{
+	struct uacce_queue *q = file->private_data;
+	struct uacce_device *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 to_uacce_device(dev) container_of(dev, struct uacce_device, dev)
+
+static ssize_t id_show(struct device *dev,
+		       struct device_attribute *attr, char *buf)
+{
+	struct uacce_device *uacce = to_uacce_device(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_device *uacce = to_uacce_device(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_device *uacce = to_uacce_device(dev);
+	int distance;
+
+	distance = node_distance(smp_processor_id(), uacce->pdev->numa_node);
+
+	return sprintf(buf, "%d\n", abs(distance));
+}
+
+static ssize_t node_id_show(struct device *dev,
+			    struct device_attribute *attr, char *buf)
+{
+	struct uacce_device *uacce = to_uacce_device(dev);
+	int node_id;
+
+	node_id = dev_to_node(uacce->pdev);
+
+	return sprintf(buf, "%d\n", node_id);
+}
+
+static ssize_t flags_show(struct device *dev,
+			  struct device_attribute *attr, char *buf)
+{
+	struct uacce_device *uacce = to_uacce_device(dev);
+
+	return sprintf(buf, "%u\n", uacce->flags);
+}
+
+static ssize_t available_instances_show(struct device *dev,
+					struct device_attribute *attr,
+					char *buf)
+{
+	struct uacce_device *uacce = to_uacce_device(dev);
+	int val = 0;
+
+	if (uacce->ops->get_available_instances)
+		val = uacce->ops->get_available_instances(uacce);
+
+	return sprintf(buf, "%d\n", val);
+}
+
+static ssize_t algorithms_show(struct device *dev,
+			       struct device_attribute *attr, char *buf)
+{
+	struct uacce_device *uacce = to_uacce_device(dev);
+
+	return sprintf(buf, "%s", uacce->algs);
+}
+
+static ssize_t qfrt_mmio_size_show(struct device *dev,
+				   struct device_attribute *attr, char *buf)
+{
+	struct uacce_device *uacce = to_uacce_device(dev);
+
+	return sprintf(buf, "%lu\n",
+		       uacce->qf_pg_size[UACCE_QFRT_MMIO] << PAGE_SHIFT);
+}
+
+static ssize_t qfrt_dus_size_show(struct device *dev,
+				  struct device_attribute *attr, char *buf)
+{
+	struct uacce_device *uacce = to_uacce_device(dev);
+
+	return sprintf(buf, "%lu\n",
+		       uacce->qf_pg_size[UACCE_QFRT_DUS] << PAGE_SHIFT);
+}
+
+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(qfrt_mmio_size);
+static DEVICE_ATTR_RO(qfrt_dus_size);
+
+static struct attribute *uacce_dev_attrs[] = {
+	&dev_attr_id.attr,
+	&dev_attr_api.attr,
+	&dev_attr_node_id.attr,
+	&dev_attr_numa_distance.attr,
+	&dev_attr_flags.attr,
+	&dev_attr_available_instances.attr,
+	&dev_attr_algorithms.attr,
+	&dev_attr_qfrt_mmio_size.attr,
+	&dev_attr_qfrt_dus_size.attr,
+	NULL,
+};
+ATTRIBUTE_GROUPS(uacce_dev);
+
+static void uacce_release(struct device *dev)
+{
+	struct uacce_device *uacce = to_uacce_device(dev);
+
+	kfree(uacce);
+}
+
+/**
+ * uacce_register - register an accelerator
+ * @parent: pointer of uacce parent device
+ * @interface: pointer of uacce_interface for register
+ */
+struct uacce_device *uacce_register(struct device *parent,
+				    struct uacce_interface *interface)
+{
+	unsigned int flags = interface->flags;
+	struct uacce_device *uacce;
+	int ret;
+
+	uacce = kzalloc(sizeof(struct uacce_device), GFP_KERNEL);
+	if (!uacce)
+		return ERR_PTR(-ENOMEM);
+
+	if (flags & UACCE_DEV_SVA) {
+		ret = iommu_dev_enable_feature(parent, IOMMU_DEV_FEAT_SVA);
+		if (ret)
+			flags &= ~UACCE_DEV_SVA;
+	}
+
+	uacce->pdev = parent;
+	uacce->flags = flags;
+	uacce->ops = interface->ops;
+
+	ret = xa_alloc(&uacce_xa, &uacce->dev_id, uacce, xa_limit_32b,
+		       GFP_KERNEL);
+	if (ret < 0)
+		goto err_with_uacce;
+
+	uacce->cdev = cdev_alloc();
+	if (!uacce->cdev) {
+		ret = -ENOMEM;
+		goto err_with_xa;
+	}
+
+	INIT_LIST_HEAD(&uacce->qs);
+	mutex_init(&uacce->q_lock);
+	uacce->cdev->ops = &uacce_fops;
+	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_groups;
+	uacce->dev.parent = uacce->pdev;
+	uacce->dev.release = uacce_release;
+	dev_set_name(&uacce->dev, "%s-%d", interface->name, uacce->dev_id);
+	ret = cdev_device_add(uacce->cdev, &uacce->dev);
+	if (ret)
+		goto err_with_xa;
+
+	return uacce;
+
+err_with_xa:
+	if (uacce->cdev)
+		cdev_del(uacce->cdev);
+	xa_erase(&uacce_xa, uacce->dev_id);
+err_with_uacce:
+	if (flags & UACCE_DEV_SVA)
+		iommu_dev_disable_feature(uacce->pdev, IOMMU_DEV_FEAT_SVA);
+	kfree(uacce);
+	return ERR_PTR(ret);
+}
+EXPORT_SYMBOL_GPL(uacce_register);
+
+/**
+ * uacce_unregister - unregisters an accelerator
+ * @uacce: the accelerator to unregister
+ */
+void uacce_unregister(struct uacce_device *uacce)
+{
+	if (!uacce)
+		return;
+
+	mutex_lock(&uacce->q_lock);
+	if (!list_empty(&uacce->qs)) {
+		struct uacce_queue *q;
+
+		list_for_each_entry(q, &uacce->qs, list) {
+			uacce_put_queue(q);
+			if (uacce->flags & UACCE_DEV_SVA)
+				iommu_sva_unbind_device(q->handle);
+		}
+	}
+	mutex_unlock(&uacce->q_lock);
+
+	if (uacce->flags & UACCE_DEV_SVA)
+		iommu_dev_disable_feature(uacce->pdev, IOMMU_DEV_FEAT_SVA);
+
+	cdev_device_del(uacce->cdev, &uacce->dev);
+	xa_erase(&uacce_xa, uacce->dev_id);
+	put_device(&uacce->dev);
+}
+EXPORT_SYMBOL_GPL(uacce_unregister);
+
+static int __init uacce_init(void)
+{
+	int ret;
+
+	uacce_class = class_create(THIS_MODULE, UACCE_NAME);
+	if (IS_ERR(uacce_class))
+		return PTR_ERR(uacce_class);
+
+	ret = alloc_chrdev_region(&uacce_devt, 0, MINORMASK, UACCE_NAME);
+	if (ret) {
+		class_destroy(uacce_class);
+		return ret;
+	}
+
+	return 0;
+}
+
+static __exit void uacce_exit(void)
+{
+	unregister_chrdev_region(uacce_devt, MINORMASK);
+	class_destroy(uacce_class);
+}
+
+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..04c8643
--- /dev/null
+++ b/include/linux/uacce.h
@@ -0,0 +1,163 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+#ifndef _LINUX_UACCE_H
+#define _LINUX_UACCE_H
+
+#include <linux/cdev.h>
+#include <uapi/misc/uacce/uacce.h>
+
+#define UACCE_NAME		"uacce"
+#define UACCE_QFRT_MAX		16
+#define UACCE_MAX_NAME_SIZE	64
+
+struct uacce_queue;
+struct uacce_device;
+
+/**
+ * enum uacce_qfr_flag: queue file flag:
+ * @UACCE_QFRF_SELFMT: self maintained qfr
+ */
+enum uacce_qfr_flag {
+	UACCE_QFRF_SELFMT = BIT(0),
+};
+
+/**
+ * struct uacce_qfile_region - structure of queue file region
+ * @type: type of the qfr
+ * @flags: flags of qfr
+ * @prot: qfr protection flag
+ */
+struct uacce_qfile_region {
+	enum uacce_qfrt type;
+	enum uacce_qfr_flag flags;
+	u32 prot;
+};
+
+/**
+ * struct uacce_ops - uacce device operations
+ * @get_available_instances:  get available instances left of the device
+ * @get_queue: get a queue from the device
+ * @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 uacce device
+ * @reset_queue: reset the queue
+ * @ioctl: ioctl for user space users of the queue
+ */
+struct uacce_ops {
+	int (*get_available_instances)(struct uacce_device *uacce);
+	int (*get_queue)(struct uacce_device *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_device *uacce);
+	int (*reset_queue)(struct uacce_queue *q);
+	long (*ioctl)(struct uacce_queue *q, unsigned int cmd,
+		      unsigned long arg);
+};
+
+/**
+ * struct uacce_interface
+ * @name: the uacce device name.  Will show up in sysfs
+ * @flags: uacce device attributes
+ * @ops: pointer to the struct uacce_ops
+ *
+ * This structure is used for the uacce_register()
+ */
+struct uacce_interface {
+	char name[UACCE_MAX_NAME_SIZE];
+	enum uacce_dev_flag flags;
+	struct uacce_ops *ops;
+};
+
+enum uacce_q_state {
+	UACCE_Q_INIT,
+	UACCE_Q_STARTED,
+	UACCE_Q_ZOMBIE,
+};
+
+/**
+ * struct uacce_queue
+ * @uacce: pointer to uacce
+ * @priv: private pointer
+ * @wait: wait queue head
+ * @pasid: pasid of the queue
+ * @pid: pid of the process using the queue
+ * @handle: iommu_sva handle return from iommu_sva_bind_device
+ * @list: queue list
+ * @qfrs: pointer of qfr regions
+ * @state: queue state machine
+ */
+struct uacce_queue {
+	struct uacce_device *uacce;
+	void *priv;
+	wait_queue_head_t wait;
+	int pasid;
+	pid_t pid;
+	struct iommu_sva *handle;
+	struct list_head list;
+	struct uacce_qfile_region *qfrs[UACCE_QFRT_MAX];
+	enum uacce_q_state state;
+};
+
+/**
+ * struct uacce_device
+ * @algs: supported algorithms
+ * @api_ver: api version
+ * @qf_pg_size: page size of the queue file regions
+ * @ops: pointer to the struct uacce_ops
+ * @pdev: pointer to the parent device
+ * @is_vf: whether virtual function
+ * @flags: uacce attributes
+ * @dev_id: id of the uacce device
+ * @prot: uacce protection flag
+ * @cdev: cdev of the uacce
+ * @dev: dev of the uacce
+ * @priv: private pointer of the uacce
+ * @qs: list head of queue->list
+ * @q_lock: lock for qs
+ */
+struct uacce_device {
+	const char *algs;
+	const char *api_ver;
+	unsigned long qf_pg_size[UACCE_QFRT_MAX];
+	struct uacce_ops *ops;
+	struct device *pdev;
+	bool is_vf;
+	u32 flags;
+	u32 dev_id;
+	u32 prot;
+	struct cdev *cdev;
+	struct device dev;
+	void *priv;
+	struct list_head qs;
+	struct mutex q_lock;
+};
+
+#if IS_ENABLED(CONFIG_UACCE)
+
+struct uacce_device *uacce_register(struct device *parent,
+				    struct uacce_interface *interface);
+void uacce_unregister(struct uacce_device *uacce);
+
+#else /* CONFIG_UACCE */
+
+static inline
+struct uacce_device *uacce_register(struct device *parent,
+				    struct uacce_interface *interface)
+{
+	return ERR_PTR(-ENODEV);
+}
+
+static inline void uacce_unregister(struct uacce_device *uacce) {}
+
+#endif /* CONFIG_UACCE */
+
+#endif /* _LINUX_UACCE_H */
diff --git a/include/uapi/misc/uacce/uacce.h b/include/uapi/misc/uacce/uacce.h
new file mode 100644
index 0000000..a4f9378
--- /dev/null
+++ b/include/uapi/misc/uacce/uacce.h
@@ -0,0 +1,38 @@
+/* SPDX-License-Identifier: GPL-2.0+ WITH Linux-syscall-note */
+#ifndef _UAPIUUACCE_H
+#define _UAPIUUACCE_H
+
+#include <linux/types.h>
+#include <linux/ioctl.h>
+
+/* UACCE_CMD_START_Q: Start the queue */
+#define UACCE_CMD_START_Q	_IO('W', 0)
+
+/**
+ * UACCE_CMD_PUT_Q:
+ * User actively stop queue and free queue resource immediately
+ * Optimization method since close fd may delay
+ */
+#define UACCE_CMD_PUT_Q		_IO('W', 1)
+
+/**
+ * enum uacce_dev_flag: Device flags:
+ * @UACCE_DEV_SVA: Shared Virtual Addresses
+ *		   Support PASID
+ *		   Support device page faults (PCI PRI or SMMU Stall)
+ */
+enum uacce_dev_flag {
+	UACCE_DEV_SVA = BIT(0),
+};
+
+/**
+ * enum uacce_qfrt: qfrt type
+ * @UACCE_QFRT_MMIO: device mmio region
+ * @UACCE_QFRT_DUS: device user share region
+ */
+enum uacce_qfrt {
+	UACCE_QFRT_MMIO = 0,
+	UACCE_QFRT_DUS = 1,
+};
+
+#endif
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v7 3/3] crypto: hisilicon - register zip engine to uacce
  2019-10-29  6:40 [PATCH v7 0/3] Add uacce module for Accelerator Zhangfei Gao
  2019-10-29  6:40 ` [PATCH v7 1/3] uacce: Add documents for uacce Zhangfei Gao
  2019-10-29  6:40 ` [PATCH v7 2/3] uacce: add uacce driver Zhangfei Gao
@ 2019-10-29  6:40 ` Zhangfei Gao
  2019-10-31 17:53   ` Jonathan Cameron
  2 siblings, 1 reply; 16+ messages in thread
From: Zhangfei Gao @ 2019-10-29  6:40 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Arnd Bergmann, Herbert Xu, jonathan.cameron,
	grant.likely, jean-philippe, Jerome Glisse, ilias.apalodimas,
	francois.ozog, kenneth-lee-2012, Wangzhou, haojian . zhuang,
	guodong.xu
  Cc: linux-accelerators, linux-kernel, linux-crypto, iommu, Zhangfei Gao

Register qm to uacce framework for user crypto driver

Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
Signed-off-by: Zhou Wang <wangzhou1@hisilicon.com>
---
 drivers/crypto/hisilicon/qm.c           | 253 ++++++++++++++++++++++++++++++--
 drivers/crypto/hisilicon/qm.h           |  13 +-
 drivers/crypto/hisilicon/zip/zip_main.c |  39 ++---
 include/uapi/misc/uacce/qm.h            |  23 +++
 4 files changed, 292 insertions(+), 36 deletions(-)
 create mode 100644 include/uapi/misc/uacce/qm.h

diff --git a/drivers/crypto/hisilicon/qm.c b/drivers/crypto/hisilicon/qm.c
index a8ed6990..4b9cced 100644
--- a/drivers/crypto/hisilicon/qm.c
+++ b/drivers/crypto/hisilicon/qm.c
@@ -9,6 +9,9 @@
 #include <linux/log2.h>
 #include <linux/seq_file.h>
 #include <linux/slab.h>
+#include <linux/uacce.h>
+#include <linux/uaccess.h>
+#include <uapi/misc/uacce/qm.h>
 #include "qm.h"
 
 /* eq/aeq irq enable */
@@ -465,17 +468,22 @@ static void qm_cq_head_update(struct hisi_qp *qp)
 
 static void qm_poll_qp(struct hisi_qp *qp, struct hisi_qm *qm)
 {
-	struct qm_cqe *cqe = qp->cqe + qp->qp_status.cq_head;
-
-	if (qp->req_cb) {
-		while (QM_CQE_PHASE(cqe) == qp->qp_status.cqc_phase) {
-			dma_rmb();
-			qp->req_cb(qp, qp->sqe + qm->sqe_size * cqe->sq_head);
-			qm_cq_head_update(qp);
-			cqe = qp->cqe + qp->qp_status.cq_head;
-			qm_db(qm, qp->qp_id, QM_DOORBELL_CMD_CQ,
-			      qp->qp_status.cq_head, 0);
-			atomic_dec(&qp->qp_status.used);
+	struct qm_cqe *cqe;
+
+	if (qp->event_cb) {
+		qp->event_cb(qp);
+	} else {
+		cqe = qp->cqe + qp->qp_status.cq_head;
+
+		if (qp->req_cb) {
+			while (QM_CQE_PHASE(cqe) == qp->qp_status.cqc_phase) {
+				dma_rmb();
+				qp->req_cb(qp, qp->sqe + qm->sqe_size *
+					   cqe->sq_head);
+				qm_cq_head_update(qp);
+				cqe = qp->cqe + qp->qp_status.cq_head;
+				atomic_dec(&qp->qp_status.used);
+			}
 		}
 
 		/* set c_flag */
@@ -1397,6 +1405,220 @@ static void hisi_qm_cache_wb(struct hisi_qm *qm)
 	}
 }
 
+static void qm_qp_event_notifier(struct hisi_qp *qp)
+{
+	wake_up_interruptible(&qp->uacce_q->wait);
+}
+
+static int hisi_qm_get_available_instances(struct uacce_device *uacce)
+{
+	int i, ret;
+	struct hisi_qm *qm = uacce->priv;
+
+	read_lock(&qm->qps_lock);
+	for (i = 0, ret = 0; i < qm->qp_num; i++)
+		if (!qm->qp_array[i])
+			ret++;
+	read_unlock(&qm->qps_lock);
+
+	return ret;
+}
+
+static int hisi_qm_uacce_get_queue(struct uacce_device *uacce,
+				   unsigned long arg,
+				   struct uacce_queue *q)
+{
+	struct hisi_qm *qm = uacce->priv;
+	struct hisi_qp *qp;
+	u8 alg_type = 0;
+
+	qp = hisi_qm_create_qp(qm, alg_type);
+	if (IS_ERR(qp))
+		return PTR_ERR(qp);
+
+	q->priv = qp;
+	q->uacce = uacce;
+	qp->uacce_q = q;
+	qp->event_cb = qm_qp_event_notifier;
+	qp->pasid = arg;
+
+	return 0;
+}
+
+static void hisi_qm_uacce_put_queue(struct uacce_queue *q)
+{
+	struct hisi_qp *qp = q->priv;
+
+	/*
+	 * As put_queue is only called in uacce_mode=1, and only one queue can
+	 * be used in this mode. we flush all sqc cache back in put queue.
+	 */
+	hisi_qm_cache_wb(qp->qm);
+
+	/* need to stop hardware, but can not support in v1 */
+	hisi_qm_release_qp(qp);
+}
+
+/* map sq/cq/doorbell to user space */
+static int hisi_qm_uacce_mmap(struct uacce_queue *q,
+			      struct vm_area_struct *vma,
+			      struct uacce_qfile_region *qfr)
+{
+	struct hisi_qp *qp = q->priv;
+	struct hisi_qm *qm = qp->qm;
+	size_t sz = vma->vm_end - vma->vm_start;
+	struct pci_dev *pdev = qm->pdev;
+	struct device *dev = &pdev->dev;
+	unsigned long vm_pgoff;
+	int ret;
+
+	switch (qfr->type) {
+	case UACCE_QFRT_MMIO:
+		if (qm->ver == QM_HW_V2) {
+			if (sz > PAGE_SIZE * (QM_DOORBELL_PAGE_NR +
+			    QM_DOORBELL_SQ_CQ_BASE_V2 / PAGE_SIZE))
+				return -EINVAL;
+		} else {
+			if (sz > PAGE_SIZE * QM_DOORBELL_PAGE_NR)
+				return -EINVAL;
+		}
+
+		vma->vm_flags |= VM_IO;
+
+		return remap_pfn_range(vma, vma->vm_start,
+				       qm->phys_base >> PAGE_SHIFT,
+				       sz, pgprot_noncached(vma->vm_page_prot));
+	case UACCE_QFRT_DUS:
+		if (sz != qp->qdma.size)
+			return -EINVAL;
+
+		/* dma_mmap_coherent() requires vm_pgoff as 0
+		 * restore vm_pfoff to initial value for mmap()
+		 */
+		vm_pgoff = vma->vm_pgoff;
+		vma->vm_pgoff = 0;
+		ret = dma_mmap_coherent(dev, vma, qp->qdma.va,
+					qp->qdma.dma, sz);
+		vma->vm_pgoff = vm_pgoff;
+		return ret;
+
+	default:
+		return -EINVAL;
+	}
+}
+
+static int hisi_qm_uacce_start_queue(struct uacce_queue *q)
+{
+	struct hisi_qp *qp = q->priv;
+
+	return hisi_qm_start_qp(qp, qp->pasid);
+}
+
+static void hisi_qm_uacce_stop_queue(struct uacce_queue *q)
+{
+	struct hisi_qp *qp = q->priv;
+
+	hisi_qm_stop_qp(qp);
+}
+
+static int qm_set_sqctype(struct uacce_queue *q, u16 type)
+{
+	struct hisi_qm *qm = q->uacce->priv;
+	struct hisi_qp *qp = q->priv;
+
+	write_lock(&qm->qps_lock);
+	qp->alg_type = type;
+	write_unlock(&qm->qps_lock);
+
+	return 0;
+}
+
+static long hisi_qm_uacce_ioctl(struct uacce_queue *q, unsigned int cmd,
+				unsigned long arg)
+{
+	struct hisi_qp *qp = q->priv;
+	struct hisi_qp_ctx qp_ctx;
+
+	if (cmd == UACCE_CMD_QM_SET_QP_CTX) {
+		if (copy_from_user(&qp_ctx, (void __user *)arg,
+				   sizeof(struct hisi_qp_ctx)))
+			return -EFAULT;
+
+		if (qp_ctx.qc_type != 0 && qp_ctx.qc_type != 1)
+			return -EINVAL;
+
+		qm_set_sqctype(q, qp_ctx.qc_type);
+		qp_ctx.id = qp->qp_id;
+
+		if (copy_to_user((void __user *)arg, &qp_ctx,
+				 sizeof(struct hisi_qp_ctx)))
+			return -EFAULT;
+	} else {
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static struct uacce_ops uacce_qm_ops = {
+	.get_available_instances = hisi_qm_get_available_instances,
+	.get_queue = hisi_qm_uacce_get_queue,
+	.put_queue = hisi_qm_uacce_put_queue,
+	.start_queue = hisi_qm_uacce_start_queue,
+	.stop_queue = hisi_qm_uacce_stop_queue,
+	.mmap = hisi_qm_uacce_mmap,
+	.ioctl = hisi_qm_uacce_ioctl,
+};
+
+static int qm_register_uacce(struct hisi_qm *qm)
+{
+	struct pci_dev *pdev = qm->pdev;
+	struct uacce_device *uacce;
+	unsigned long mmio_page_nr;
+	unsigned long dus_page_nr;
+	struct uacce_interface interface = {
+		.flags = UACCE_DEV_SVA,
+		.ops = &uacce_qm_ops,
+	};
+
+	strncpy(interface.name, pdev->driver->name, sizeof(interface.name));
+
+	uacce = uacce_register(&pdev->dev, &interface);
+	if (IS_ERR(uacce))
+		return PTR_ERR(uacce);
+
+	if (uacce->flags & UACCE_DEV_SVA) {
+		qm->use_sva = true;
+	} else {
+		/* only consider sva case */
+		uacce_unregister(uacce);
+		return -EINVAL;
+	}
+
+	uacce->is_vf = pdev->is_virtfn;
+	uacce->priv = qm;
+	uacce->algs = qm->algs;
+
+	if (qm->ver == QM_HW_V1) {
+		mmio_page_nr = QM_DOORBELL_PAGE_NR;
+		uacce->api_ver = HISI_QM_API_VER_BASE;
+	} else {
+		mmio_page_nr = QM_DOORBELL_PAGE_NR +
+			QM_DOORBELL_SQ_CQ_BASE_V2 / PAGE_SIZE;
+		uacce->api_ver = HISI_QM_API_VER2_BASE;
+	}
+
+	dus_page_nr = (PAGE_SIZE - 1 + qm->sqe_size * QM_Q_DEPTH +
+		       sizeof(struct qm_cqe) * QM_Q_DEPTH) >> PAGE_SHIFT;
+
+	uacce->qf_pg_size[UACCE_QFRT_MMIO] = mmio_page_nr;
+	uacce->qf_pg_size[UACCE_QFRT_DUS]  = dus_page_nr;
+
+	qm->uacce = uacce;
+
+	return 0;
+}
+
 /**
  * hisi_qm_init() - Initialize configures about qm.
  * @qm: The qm needing init.
@@ -1421,6 +1643,10 @@ int hisi_qm_init(struct hisi_qm *qm)
 		return -EINVAL;
 	}
 
+	ret = qm_register_uacce(qm);
+	if (ret < 0)
+		dev_warn(&pdev->dev, "fail to register uacce (%d)\n", ret);
+
 	ret = pci_enable_device_mem(pdev);
 	if (ret < 0) {
 		dev_err(&pdev->dev, "Failed to enable device mem!\n");
@@ -1433,6 +1659,8 @@ int hisi_qm_init(struct hisi_qm *qm)
 		goto err_disable_pcidev;
 	}
 
+	qm->phys_base = pci_resource_start(pdev, PCI_BAR_2);
+	qm->size = pci_resource_len(qm->pdev, PCI_BAR_2);
 	qm->io_base = ioremap(pci_resource_start(pdev, PCI_BAR_2),
 			      pci_resource_len(qm->pdev, PCI_BAR_2));
 	if (!qm->io_base) {
@@ -1504,6 +1732,9 @@ void hisi_qm_uninit(struct hisi_qm *qm)
 	iounmap(qm->io_base);
 	pci_release_mem_regions(pdev);
 	pci_disable_device(pdev);
+
+	if (qm->uacce)
+		uacce_unregister(qm->uacce);
 }
 EXPORT_SYMBOL_GPL(hisi_qm_uninit);
 
diff --git a/drivers/crypto/hisilicon/qm.h b/drivers/crypto/hisilicon/qm.h
index 103e2fd..84a3be9 100644
--- a/drivers/crypto/hisilicon/qm.h
+++ b/drivers/crypto/hisilicon/qm.h
@@ -77,6 +77,10 @@
 
 #define HISI_ACC_SGL_SGE_NR_MAX		255
 
+/* page number for queue file region */
+#define QM_DOORBELL_PAGE_NR		1
+
+
 enum qp_state {
 	QP_STOP,
 };
@@ -161,7 +165,12 @@ struct hisi_qm {
 	u32 error_mask;
 	u32 msi_mask;
 
+	const char *algs;
 	bool use_dma_api;
+	bool use_sva;
+	resource_size_t phys_base;
+	resource_size_t size;
+	struct uacce_device *uacce;
 };
 
 struct hisi_qp_status {
@@ -191,10 +200,12 @@ struct hisi_qp {
 	struct hisi_qp_ops *hw_ops;
 	void *qp_ctx;
 	void (*req_cb)(struct hisi_qp *qp, void *data);
+	void (*event_cb)(struct hisi_qp *qp);
 	struct work_struct work;
 	struct workqueue_struct *wq;
-
 	struct hisi_qm *qm;
+	u16 pasid;
+	struct uacce_queue *uacce_q;
 };
 
 int hisi_qm_init(struct hisi_qm *qm);
diff --git a/drivers/crypto/hisilicon/zip/zip_main.c b/drivers/crypto/hisilicon/zip/zip_main.c
index 1b2ee96..48860d2 100644
--- a/drivers/crypto/hisilicon/zip/zip_main.c
+++ b/drivers/crypto/hisilicon/zip/zip_main.c
@@ -316,8 +316,14 @@ static void hisi_zip_set_user_domain_and_cache(struct hisi_zip *hisi_zip)
 	writel(AXUSER_BASE, base + HZIP_BD_RUSER_32_63);
 	writel(AXUSER_BASE, base + HZIP_SGL_RUSER_32_63);
 	writel(AXUSER_BASE, base + HZIP_BD_WUSER_32_63);
-	writel(AXUSER_BASE, base + HZIP_DATA_RUSER_32_63);
-	writel(AXUSER_BASE, base + HZIP_DATA_WUSER_32_63);
+
+	if (hisi_zip->qm.use_sva) {
+		writel(AXUSER_BASE | AXUSER_SSV, base + HZIP_DATA_RUSER_32_63);
+		writel(AXUSER_BASE | AXUSER_SSV, base + HZIP_DATA_WUSER_32_63);
+	} else {
+		writel(AXUSER_BASE, base + HZIP_DATA_RUSER_32_63);
+		writel(AXUSER_BASE, base + HZIP_DATA_WUSER_32_63);
+	}
 
 	/* let's open all compression/decompression cores */
 	writel(DECOMP_CHECK_ENABLE | ALL_COMP_DECOMP_EN,
@@ -671,24 +677,12 @@ static int hisi_zip_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	qm = &hisi_zip->qm;
 	qm->pdev = pdev;
 	qm->ver = rev_id;
-
+	qm->use_dma_api = true;
+	qm->algs = "zlib\ngzip\n";
 	qm->sqe_size = HZIP_SQE_SIZE;
 	qm->dev_name = hisi_zip_name;
 	qm->fun_type = (pdev->device == PCI_DEVICE_ID_ZIP_PF) ? QM_HW_PF :
 								QM_HW_VF;
-	switch (uacce_mode) {
-	case 0:
-		qm->use_dma_api = true;
-		break;
-	case 1:
-		qm->use_dma_api = false;
-		break;
-	case 2:
-		qm->use_dma_api = true;
-		break;
-	default:
-		return -EINVAL;
-	}
 
 	ret = hisi_qm_init(qm);
 	if (ret) {
@@ -976,12 +970,10 @@ static int __init hisi_zip_init(void)
 		goto err_pci;
 	}
 
-	if (uacce_mode == 0 || uacce_mode == 2) {
-		ret = hisi_zip_register_to_crypto();
-		if (ret < 0) {
-			pr_err("Failed to register driver to crypto.\n");
-			goto err_crypto;
-		}
+	ret = hisi_zip_register_to_crypto();
+	if (ret < 0) {
+		pr_err("Failed to register driver to crypto.\n");
+		goto err_crypto;
 	}
 
 	return 0;
@@ -996,8 +988,7 @@ static int __init hisi_zip_init(void)
 
 static void __exit hisi_zip_exit(void)
 {
-	if (uacce_mode == 0 || uacce_mode == 2)
-		hisi_zip_unregister_from_crypto();
+	hisi_zip_unregister_from_crypto();
 	pci_unregister_driver(&hisi_zip_pci_driver);
 	hisi_zip_unregister_debugfs();
 }
diff --git a/include/uapi/misc/uacce/qm.h b/include/uapi/misc/uacce/qm.h
new file mode 100644
index 0000000..d79a8f2
--- /dev/null
+++ b/include/uapi/misc/uacce/qm.h
@@ -0,0 +1,23 @@
+/* SPDX-License-Identifier: GPL-2.0+ WITH Linux-syscall-note */
+#ifndef HISI_QM_USR_IF_H
+#define HISI_QM_USR_IF_H
+
+#include <linux/types.h>
+
+/**
+ * struct hisi_qp_ctx - User data for hisi qp.
+ * @id: Specifies which Turbo decode algorithm to use
+ * @qc_type: Accelerator algorithm type
+ */
+struct hisi_qp_ctx {
+	__u16 id;
+	__u16 qc_type;
+};
+
+#define HISI_QM_API_VER_BASE "hisi_qm_v1"
+#define HISI_QM_API_VER2_BASE "hisi_qm_v2"
+
+/* UACCE_CMD_QM_SET_QP_CTX: Set qp algorithm type */
+#define UACCE_CMD_QM_SET_QP_CTX	_IOWR('H', 10, struct hisi_qp_ctx)
+
+#endif
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH v7 1/3] uacce: Add documents for uacce
  2019-10-29  6:40 ` [PATCH v7 1/3] uacce: Add documents for uacce Zhangfei Gao
@ 2019-10-31 15:37   ` Jonathan Cameron
  0 siblings, 0 replies; 16+ messages in thread
From: Jonathan Cameron @ 2019-10-31 15:37 UTC (permalink / raw)
  To: Zhangfei Gao
  Cc: Greg Kroah-Hartman, Arnd Bergmann, Herbert Xu, grant.likely,
	jean-philippe, Jerome Glisse, ilias.apalodimas, francois.ozog,
	kenneth-lee-2012, Wangzhou, haojian . zhuang, guodong.xu,
	linux-accelerators, linux-kernel, linux-crypto, iommu,
	Kenneth Lee, Zaibo Xu

On Tue, 29 Oct 2019 14:40:14 +0800
Zhangfei Gao <zhangfei.gao@linaro.org> wrote:

> From: Kenneth Lee <liguozhu@hisilicon.com>
> 
> Uacce (Unified/User-space-access-intended Accelerator Framework) is
> a kernel module targets to provide Shared Virtual Addressing (SVA)
> between the accelerator and process.
> 
> 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/uacce.rst | 160 +++++++++++++++++++++++++++++++++++
>  1 file changed, 160 insertions(+)
>  create mode 100644 Documentation/misc-devices/uacce.rst
> 
> diff --git a/Documentation/misc-devices/uacce.rst b/Documentation/misc-devices/uacce.rst
> new file mode 100644
> index 0000000..ecd5d8b
> --- /dev/null
> +++ b/Documentation/misc-devices/uacce.rst
> @@ -0,0 +1,160 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +Introduction of Uacce
> +=========================

Fix the underline length to match the title.

> +
> +Uacce (Unified/User-space-access-intended Accelerator Framework) targets to
> +provide Shared Virtual Addressing (SVA) between accelerators and processes.
> +So accelerator can access any data structure of the main cpu.
> +This differs from the data sharing between cpu and io device, which share
> +data content rather than address.

which share only data content rather than address.

> +Because of the unified address, hardware and user space of process can
> +share the same virtual address in the communication.
> +Uacce takes the hardware accelerator as a heterogeneous processor, while
> +IOMMU share the same CPU page tables and as a result the same translation
> +from va to pa.
> +
> +	 __________________________       __________________________
> +	|                          |     |                          |
> +	|  User application (CPU)  |     |   Hardware Accelerator   |
> +	|__________________________|     |__________________________|
> +
> +	             |                                 |
> +	             | va                              | va
> +	             V                                 V
> +                 __________                        __________
> +                |          |                      |          |
> +                |   MMU    |                      |  IOMMU   |
> +                |__________|                      |__________|
> +		     |                                 |
> +	             |                                 |
> +	             V pa                              V pa
> +		 _______________________________________
> +		|                                       |
> +		|              Memory                   |
> +		|_______________________________________|
> +
> +
> +
> +Architecture
> +------------
> +
> +Uacce is the kernel module, taking charge of iommu and address sharing.
> +The user drivers and libraries are called WarpDrive.
> +
> +The uacce device, built around the IOMMU SVA API, can access multiple
> +address spaces, including the one without PASID.
> +
> +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) |
> +                                                                 |___________________|
> +
> +
> +How does it work
> +================
> +
> +Uacce 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 accelerator device present itself as an 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).
> +
> +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: qfrt type
> + * @UACCE_QFRT_MMIO: device mmio region
> + * @UACCE_QFRT_DUS: device user share region
> + */
> +enum uacce_qfrt {
> +	UACCE_QFRT_MMIO = 0,
> +	UACCE_QFRT_DUS = 1,
> +};
> +
> +All regions are optional and differ from device type to type. The
> +communication protocol is wrapped by the user driver.

Can you have more than one of each? If not state this clearly.

> +
> +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 user share region is used for share data buffer between user process
> +and device.
> +
> +
> +The Uacce register API
> +-----------------------
blank line here.

> +The register API is defined in uacce.h.
> +
> +struct uacce_interface {
> +	char name[UACCE_MAX_NAME_SIZE];
> +	enum uacce_dev_flag flags;

Having an enum for a flags variable / bitmap is odd.  Just use an
appropriate sized integer.

> +	struct uacce_ops *ops;
> +};
Use indenting to get literal formatting of this block. Will
give formatting like the code blocks in here compiled:
https://www.kernel.org/doc/html/latest/driver-api/iio/buffers.html

> +
> +According to the IOMMU capability, uacce_interface flags can be:
> +
> +/**
> + * enum uacce_dev_flag: Device flags:
> + * @UACCE_DEV_SVA: Shared Virtual Addresses
> + *		   Support PASID
> + *		   Support device page faults (PCI PRI or SMMU Stall)
> + */
> +enum uacce_dev_flag {
> +	UACCE_DEV_SVA = BIT(0),
> +};
> +
> +struct uacce_device *uacce_register(struct device *parent,
> +				    struct uacce_interface *interface);
> +void uacce_unregister(struct uacce_device *uacce);
> +
> +uacce_register results can be:
> +a. If uacce module is not compiled, ERR_PTR(-ENODEV)
> +b. Succeed with the desired flags
> +c. Succeed with the negotiated flags, for example
> +   uacce_interface.flags = UACCE_DEV_SVA but uacce->flags = ~UACCE_DEV_SVA
> +So user driver need check return value as well as the negotiated uacce->flags.
> +
> +
> +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.
> +More details in Documentation/ABI/testing/sysfs-driver-uacce.



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v7 2/3] uacce: add uacce driver
  2019-10-29  6:40 ` [PATCH v7 2/3] uacce: add uacce driver Zhangfei Gao
@ 2019-10-31 17:13   ` Jonathan Cameron
  2019-11-05  7:43     ` zhangfei
  2019-11-05 11:48   ` Jean-Philippe Brucker
  1 sibling, 1 reply; 16+ messages in thread
From: Jonathan Cameron @ 2019-10-31 17:13 UTC (permalink / raw)
  To: Zhangfei Gao
  Cc: Greg Kroah-Hartman, Arnd Bergmann, Herbert Xu, grant.likely,
	jean-philippe, Jerome Glisse, ilias.apalodimas, francois.ozog,
	kenneth-lee-2012, Wangzhou, haojian . zhuang, guodong.xu,
	linux-accelerators, linux-kernel, linux-crypto, iommu,
	Kenneth Lee, Zaibo Xu

On Tue, 29 Oct 2019 14:40:15 +0800
Zhangfei Gao <zhangfei.gao@linaro.org> wrote:

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

Great, much more compact.

I've not gone through this in detail yet but a few initial comments inline.

Thanks,

Jonathan

> ---
>  Documentation/ABI/testing/sysfs-driver-uacce |  53 +++
>  drivers/misc/Kconfig                         |   1 +
>  drivers/misc/Makefile                        |   1 +
>  drivers/misc/uacce/Kconfig                   |  13 +
>  drivers/misc/uacce/Makefile                  |   2 +
>  drivers/misc/uacce/uacce.c                   | 574 +++++++++++++++++++++++++++
>  include/linux/uacce.h                        | 163 ++++++++
>  include/uapi/misc/uacce/uacce.h              |  38 ++
>  8 files changed, 845 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-driver-uacce
>  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/uacce.h
> 
> diff --git a/Documentation/ABI/testing/sysfs-driver-uacce b/Documentation/ABI/testing/sysfs-driver-uacce
> new file mode 100644
> index 0000000..35699dc
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-driver-uacce
> @@ -0,0 +1,53 @@
> +What:           /sys/class/uacce/<dev_name>/id
> +Date:           Oct 2019
> +KernelVersion:  5.5
> +Contact:        linux-accelerators@lists.ozlabs.org
> +Description:    Id of the device.
> +
> +What:           /sys/class/uacce/<dev_name>/api
> +Date:           Oct 2019
> +KernelVersion:  5.5
> +Contact:        linux-accelerators@lists.ozlabs.org
> +Description:    Api of the device, used by application to match the correct driver
> +
> +What:           /sys/class/uacce/<dev_name>/flags
> +Date:           Oct 2019
> +KernelVersion:  5.5
> +Contact:        linux-accelerators@lists.ozlabs.org
> +Description:    Attributes of the device, see UACCE_DEV_xxx flag defined in uacce.h
> +
> +What:           /sys/class/uacce/<dev_name>/available_instances
> +Date:           Oct 2019
> +KernelVersion:  5.5
> +Contact:        linux-accelerators@lists.ozlabs.org
> +Description:    Available instances left of the device
> +
> +What:           /sys/class/uacce/<dev_name>/algorithms
> +Date:           Oct 2019
> +KernelVersion:  5.5
> +Contact:        linux-accelerators@lists.ozlabs.org
> +Description:    Algorithms supported by this accelerator
How are they separated?  Userspace code needs to know that.
(comma, tab, newline?)

> +
> +What:           /sys/class/uacce/<dev_name>/qfrt_mmio_size

qfrt is not the most obvious naming ever.  Do we care beyond its
a region for this interface?  region_mmio_size maybe?

> +Date:           Oct 2019
> +KernelVersion:  5.5
> +Contact:        linux-accelerators@lists.ozlabs.org
> +Description:    Page size of mmio region queue file

Size of page in this region, or number of pages in the region?

> +
> +What:           /sys/class/uacce/<dev_name>/qfrt_dus_size
> +Date:           Oct 2019
> +KernelVersion:  5.5
> +Contact:        linux-accelerators@lists.ozlabs.org
> +Description:    Page size of dus region queue file
> +
> +What:           /sys/class/uacce/<dev_name>/numa_distance
> +Date:           Oct 2019
> +KernelVersion:  5.5
> +Contact:        linux-accelerators@lists.ozlabs.org
> +Description:    Distance of device node to cpu node

I wonder if we should be doing this in here. There are other standard
ways of obtaining this for the device.  Follow parent and check node_id
there then use the /sys/bus/node path to find out the distances.

> +
> +What:           /sys/class/uacce/<dev_name>/node_id
> +Date:           Oct 2019
> +KernelVersion:  5.5
> +Contact:        linux-accelerators@lists.ozlabs.org
> +Description:    Id of the numa node
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index c55b637..929feb0 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -481,4 +481,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 c1860d3..9abf292 100644
> --- a/drivers/misc/Makefile
> +++ b/drivers/misc/Makefile
> @@ -56,4 +56,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..5e39b60
> --- /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/uacce.h
> +
> +	  See Documentation/misc-devices/uacce.rst for more details.
> +
> +	  If you don't know what to do here, say N.

Pessimist :) Everyone should want uacce so don't put them off.  Having said
that perhaps for now it should be hidden and enabled on a driver by driver
basis?

> 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..2b6b038
> --- /dev/null
> +++ b/drivers/misc/uacce/uacce.c
> @@ -0,0 +1,574 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +#include <linux/compat.h>
> +#include <linux/dma-iommu.h>
> +#include <linux/module.h>
> +#include <linux/poll.h>
> +#include <linux/uacce.h>
> +
> +static struct class *uacce_class;
> +static dev_t uacce_devt;
> +static DEFINE_MUTEX(uacce_mutex);
> +static DEFINE_XARRAY_ALLOC(uacce_xa);
> +
> +static int uacce_start_queue(struct uacce_queue *q)
> +{
> +	int ret = -EINVAL;
> +
> +	mutex_lock(&uacce_mutex);
> +
> +	if (q->state != UACCE_Q_INIT)
> +		goto out_with_lock;
> +
> +	if (q->uacce->ops->start_queue) {
> +		ret = q->uacce->ops->start_queue(q);
> +		if (ret < 0)
> +			goto out_with_lock;
> +	}
> +
> +	q->state = UACCE_Q_STARTED;
out_with_lock:
> +	mutex_unlock(&uacce_mutex);
> +
return ret;
Though need to handle ret a bit differently above...

> +	return 0;
> +
> +out_with_lock:
> +	mutex_unlock(&uacce_mutex);
> +	return ret;
> +}
> +
> +static int uacce_put_queue(struct uacce_queue *q)
> +{
> +	struct uacce_device *uacce = q->uacce;
> +
> +	mutex_lock(&uacce_mutex);
> +
> +	if (q->state == UACCE_Q_ZOMBIE)
> +		goto out;
> +
> +	if ((q->state == UACCE_Q_STARTED) && uacce->ops->stop_queue)
> +		uacce->ops->stop_queue(q);
> +
> +	if ((q->state == UACCE_Q_INIT || q->state == UACCE_Q_STARTED) &&
> +	     uacce->ops->put_queue)
> +		uacce->ops->put_queue(q);
> +
> +	q->state = UACCE_Q_ZOMBIE;
> +out:
> +	mutex_unlock(&uacce_mutex);
> +
> +	return 0;
> +}
> +
> +static long uacce_fops_unl_ioctl(struct file *filep,
> +				 unsigned int cmd, unsigned long arg)
> +{
> +	struct uacce_queue *q = filep->private_data;
> +	struct uacce_device *uacce = q->uacce;
> +
> +	switch (cmd) {
> +	case UACCE_CMD_START_Q:
> +		return uacce_start_queue(q);
> +
> +	case UACCE_CMD_PUT_Q:
> +		return uacce_put_queue(q);
> +
> +	default:
> +		if (!uacce->ops->ioctl)
> +			return -EINVAL;
> +
> +		return uacce->ops->ioctl(q, cmd, arg);
> +	}
> +}
> +
> +#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_sva_exit(struct device *dev, struct iommu_sva *handle,
> +			  void *data)
> +{
> +	struct uacce_device *uacce = data;
> +	struct uacce_queue *q;
> +
> +	mutex_lock(&uacce->q_lock);
> +	list_for_each_entry(q, &uacce->qs, list) {
> +		if (q->pid == task_pid_nr(current))
> +			uacce_put_queue(q);
> +	}
> +	mutex_unlock(&uacce->q_lock);
> +
> +	return 0;
> +}
> +
> +static struct iommu_sva_ops uacce_sva_ops = {
> +	.mm_exit = uacce_sva_exit,
> +};
> +
> +static int uacce_fops_open(struct inode *inode, struct file *filep)
> +{
> +	struct iommu_sva *handle = NULL;
> +	struct uacce_device *uacce;
> +	struct uacce_queue *q;
> +	int ret = 0;
> +	int pasid = 0;
> +
> +	uacce = xa_load(&uacce_xa, iminor(inode));
> +	if (!uacce)
> +		return -ENODEV;
> +
> +	if (!try_module_get(uacce->pdev->driver->owner))
> +		return -ENODEV;
> +
> +	q = kzalloc(sizeof(struct uacce_queue), GFP_KERNEL);
> +	if (!q) {
> +		ret = -ENOMEM;
> +		goto out_with_module;
> +	}
> +
> +	if (uacce->flags & UACCE_DEV_SVA) {
> +		handle = iommu_sva_bind_device(uacce->pdev, current->mm, uacce);
> +		if (IS_ERR(handle))
> +			goto out_with_mem;
> +
> +		ret = iommu_sva_set_ops(handle, &uacce_sva_ops);
> +		if (ret)
> +			goto out_unbind;
> +
> +		pasid = iommu_sva_get_pasid(handle);
> +		if (pasid == IOMMU_PASID_INVALID)
> +			goto out_unbind;
> +	}
> +
> +	if (uacce->ops->get_queue) {
> +		ret = uacce->ops->get_queue(uacce, pasid, q);
> +		if (ret < 0)
> +			goto out_unbind;
> +	}
> +
> +	q->pid = task_pid_nr(current);
> +	q->pasid = pasid;
> +	q->handle = handle;
> +	q->uacce = uacce;
> +	memset(q->qfrs, 0, sizeof(q->qfrs));
> +	init_waitqueue_head(&q->wait);
> +	filep->private_data = q;
> +	q->state = UACCE_Q_INIT;
> +
> +	mutex_lock(&uacce->q_lock);
> +	list_add(&q->list, &uacce->qs);
> +	mutex_unlock(&uacce->q_lock);
> +
> +	return 0;
> +
> +out_unbind:
> +	if (uacce->flags & UACCE_DEV_SVA)
> +		iommu_sva_unbind_device(handle);
> +out_with_mem:
> +	kfree(q);
> +out_with_module:
> +	module_put(uacce->pdev->driver->owner);
> +	return ret;
> +}
> +
> +static int uacce_fops_release(struct inode *inode, struct file *filep)
> +{
> +	struct uacce_queue *q = filep->private_data;
> +	struct uacce_device *uacce = q->uacce;
> +
> +	uacce_put_queue(q);
> +
> +	if (uacce->flags & UACCE_DEV_SVA)
> +		iommu_sva_unbind_device(q->handle);
> +
> +	mutex_lock(&uacce->q_lock);
> +	list_del(&q->list);
> +	mutex_unlock(&uacce->q_lock);
> +	kfree(q);
> +	module_put(uacce->pdev->driver->owner);
> +
> +	return 0;
> +}
> +
> +static void uacce_vma_close(struct vm_area_struct *vma)
> +{
> +	struct uacce_queue *q = vma->vm_private_data;
> +	enum uacce_qfrt type = 0;
> +
> +	if (vma->vm_pgoff < UACCE_QFRT_MAX)
> +		type = vma->vm_pgoff;
> +
> +	kfree(q->qfrs[type]);
> +}
> +
> +static const struct vm_operations_struct uacce_vm_ops = {
> +	.close = uacce_vma_close,
> +};
> +
> +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_device *uacce = q->uacce;
> +	struct uacce_qfile_region *qfr;
> +	int ret = -ENOMEM;
> +
> +	qfr = kzalloc(sizeof(*qfr), GFP_KERNEL);
> +	if (!qfr)
> +		return ERR_PTR(-ENOMEM);
> +
> +	qfr->type = type;
> +	qfr->flags = flags;
> +
> +	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) {
> +		if (!uacce->ops->mmap) {
> +			ret = -EINVAL;
> +			goto err_with_qfr;
> +		}
> +
> +		ret = uacce->ops->mmap(q, vma, qfr);
> +		if (ret)
> +			goto err_with_qfr;
> +		return qfr;
> +	}
> +
> +	return qfr;
> +
> +err_with_qfr:
> +	kfree(qfr);
> +	return ERR_PTR(ret);
> +}
> +
> +static int uacce_fops_mmap(struct file *filep, struct vm_area_struct *vma)
> +{
> +	struct uacce_queue *q = filep->private_data;
> +	struct uacce_device *uacce = q->uacce;
> +	struct uacce_qfile_region *qfr;
> +	enum uacce_qfrt type = 0;
> +	unsigned int flags = 0;
> +	int ret;
> +
> +	if (vma->vm_pgoff < UACCE_QFRT_MAX)
> +		type = vma->vm_pgoff;
> +
> +	vma->vm_flags |= VM_DONTCOPY | VM_DONTEXPAND | VM_WIPEONFORK;
> +	vma->vm_ops = &uacce_vm_ops;
> +	vma->vm_private_data = q;
> +
> +	mutex_lock(&uacce_mutex);
> +
> +	if (q->qfrs[type]) {
> +		ret = -EEXIST;
> +		goto out_with_lock;
> +	}
> +
> +	switch (type) {
> +	case UACCE_QFRT_MMIO:
> +		flags = UACCE_QFRF_SELFMT;
> +		break;
> +
> +	case UACCE_QFRT_DUS:
> +		if (uacce->flags & UACCE_DEV_SVA) {
> +			flags = UACCE_QFRF_SELFMT;
> +			break;
> +		}
> +		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;
> +

Could put
out_with_lock:
here and return ret instead of 0.
You'll need to set ret to default to 0 in that
case though.

> +	mutex_unlock(&uacce_mutex);
> +
> +	return 0;
> +
> +out_with_lock:
> +	mutex_unlock(&uacce_mutex);
> +	return ret;
> +}
> +
> +static __poll_t uacce_fops_poll(struct file *file, poll_table *wait)
> +{
> +	struct uacce_queue *q = file->private_data;
> +	struct uacce_device *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 to_uacce_device(dev) container_of(dev, struct uacce_device, dev)
> +
> +static ssize_t id_show(struct device *dev,
> +		       struct device_attribute *attr, char *buf)
> +{
> +	struct uacce_device *uacce = to_uacce_device(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_device *uacce = to_uacce_device(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_device *uacce = to_uacce_device(dev);
> +	int distance;
> +
> +	distance = node_distance(smp_processor_id(), uacce->pdev->numa_node);
> +
> +	return sprintf(buf, "%d\n", abs(distance));
> +}
> +
> +static ssize_t node_id_show(struct device *dev,
> +			    struct device_attribute *attr, char *buf)
> +{
> +	struct uacce_device *uacce = to_uacce_device(dev);
> +	int node_id;
> +
> +	node_id = dev_to_node(uacce->pdev);
> +
> +	return sprintf(buf, "%d\n", node_id);
> +}
> +
> +static ssize_t flags_show(struct device *dev,
> +			  struct device_attribute *attr, char *buf)
> +{
> +	struct uacce_device *uacce = to_uacce_device(dev);
> +
> +	return sprintf(buf, "%u\n", uacce->flags);
> +}
> +
> +static ssize_t available_instances_show(struct device *dev,
> +					struct device_attribute *attr,
> +					char *buf)
> +{
> +	struct uacce_device *uacce = to_uacce_device(dev);
> +	int val = 0;
> +
> +	if (uacce->ops->get_available_instances)
> +		val = uacce->ops->get_available_instances(uacce);
> +
> +	return sprintf(buf, "%d\n", val);
> +}
> +
> +static ssize_t algorithms_show(struct device *dev,
> +			       struct device_attribute *attr, char *buf)
> +{
> +	struct uacce_device *uacce = to_uacce_device(dev);
> +
> +	return sprintf(buf, "%s", uacce->algs);
Any risk algs won't have the \n?
I'd kind of expect it to be a null termated arrays to allow the core
to format it however it wants to.

> +}
> +
> +static ssize_t qfrt_mmio_size_show(struct device *dev,
> +				   struct device_attribute *attr, char *buf)
> +{
> +	struct uacce_device *uacce = to_uacce_device(dev);
> +
> +	return sprintf(buf, "%lu\n",
> +		       uacce->qf_pg_size[UACCE_QFRT_MMIO] << PAGE_SHIFT);
> +}
> +
> +static ssize_t qfrt_dus_size_show(struct device *dev,
> +				  struct device_attribute *attr, char *buf)
> +{
> +	struct uacce_device *uacce = to_uacce_device(dev);
> +
> +	return sprintf(buf, "%lu\n",
> +		       uacce->qf_pg_size[UACCE_QFRT_DUS] << PAGE_SHIFT);
> +}
> +
> +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(qfrt_mmio_size);
> +static DEVICE_ATTR_RO(qfrt_dus_size);
> +
> +static struct attribute *uacce_dev_attrs[] = {
> +	&dev_attr_id.attr,
> +	&dev_attr_api.attr,
> +	&dev_attr_node_id.attr,
> +	&dev_attr_numa_distance.attr,
> +	&dev_attr_flags.attr,
> +	&dev_attr_available_instances.attr,
> +	&dev_attr_algorithms.attr,
> +	&dev_attr_qfrt_mmio_size.attr,
> +	&dev_attr_qfrt_dus_size.attr,
> +	NULL,
> +};
> +ATTRIBUTE_GROUPS(uacce_dev);
> +
> +static void uacce_release(struct device *dev)
> +{
> +	struct uacce_device *uacce = to_uacce_device(dev);
> +
> +	kfree(uacce);
> +}
> +
> +/**
> + * uacce_register - register an accelerator
This isn't quite correct kernel-doc.  Please run the
generation script over it and fix any warnings.

	uacce_register() - register an accelerator

> + * @parent: pointer of uacce parent device
> + * @interface: pointer of uacce_interface for register
> + */
> +struct uacce_device *uacce_register(struct device *parent,
> +				    struct uacce_interface *interface)
> +{
> +	unsigned int flags = interface->flags;
> +	struct uacce_device *uacce;
> +	int ret;
> +
> +	uacce = kzalloc(sizeof(struct uacce_device), GFP_KERNEL);
> +	if (!uacce)
> +		return ERR_PTR(-ENOMEM);
> +
> +	if (flags & UACCE_DEV_SVA) {
> +		ret = iommu_dev_enable_feature(parent, IOMMU_DEV_FEAT_SVA);
> +		if (ret)
> +			flags &= ~UACCE_DEV_SVA;
> +	}
> +
> +	uacce->pdev = parent;
> +	uacce->flags = flags;
> +	uacce->ops = interface->ops;
> +
> +	ret = xa_alloc(&uacce_xa, &uacce->dev_id, uacce, xa_limit_32b,
> +		       GFP_KERNEL);
> +	if (ret < 0)
> +		goto err_with_uacce;
> +
> +	uacce->cdev = cdev_alloc();

If we can embed this (see below) then use cdev_init instead.

> +	if (!uacce->cdev) {
> +		ret = -ENOMEM;
> +		goto err_with_xa;
> +	}
> +
> +	INIT_LIST_HEAD(&uacce->qs);
> +	mutex_init(&uacce->q_lock);
> +	uacce->cdev->ops = &uacce_fops;
> +	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_groups;
> +	uacce->dev.parent = uacce->pdev;
> +	uacce->dev.release = uacce_release;
> +	dev_set_name(&uacce->dev, "%s-%d", interface->name, uacce->dev_id);
> +	ret = cdev_device_add(uacce->cdev, &uacce->dev);
> +	if (ret)
> +		goto err_with_xa;
> +
> +	return uacce;
> +
> +err_with_xa:
> +	if (uacce->cdev)
> +		cdev_del(uacce->cdev);
Why not use a separate label to handle the above rather than checking if
it's set?

> +	xa_erase(&uacce_xa, uacce->dev_id);
> +err_with_uacce:
> +	if (flags & UACCE_DEV_SVA)
> +		iommu_dev_disable_feature(uacce->pdev, IOMMU_DEV_FEAT_SVA);
> +	kfree(uacce);
> +	return ERR_PTR(ret);
> +}
> +EXPORT_SYMBOL_GPL(uacce_register);
> +
> +/**
> + * uacce_unregister - unregisters an accelerator
> + * @uacce: the accelerator to unregister
> + */
> +void uacce_unregister(struct uacce_device *uacce)
> +{
> +	if (!uacce)
> +		return;
> +

I'd like to see a comment here on why we are doing things not unwinding
actions from uacce_register.

> +	mutex_lock(&uacce->q_lock);
> +	if (!list_empty(&uacce->qs)) {
> +		struct uacce_queue *q;
> +
> +		list_for_each_entry(q, &uacce->qs, list) {
> +			uacce_put_queue(q);
> +			if (uacce->flags & UACCE_DEV_SVA)
> +				iommu_sva_unbind_device(q->handle);
> +		}
> +	}
> +	mutex_unlock(&uacce->q_lock);
> +

For these next parts which are the unwind of uacce_register, why are they not
in the reverse order of what is happening in there (where possible given
device lifespan). That is why do we not disable the iommu feature much later?

> +	if (uacce->flags & UACCE_DEV_SVA)
> +		iommu_dev_disable_feature(uacce->pdev, IOMMU_DEV_FEAT_SVA);
> +
> +	cdev_device_del(uacce->cdev, &uacce->dev);
> +	xa_erase(&uacce_xa, uacce->dev_id);
> +	put_device(&uacce->dev);
> +}
> +EXPORT_SYMBOL_GPL(uacce_unregister);
> +
> +static int __init uacce_init(void)
> +{
> +	int ret;
> +
> +	uacce_class = class_create(THIS_MODULE, UACCE_NAME);
> +	if (IS_ERR(uacce_class))
> +		return PTR_ERR(uacce_class);
> +
> +	ret = alloc_chrdev_region(&uacce_devt, 0, MINORMASK, UACCE_NAME);
> +	if (ret) {
> +		class_destroy(uacce_class);
> +		return ret;
drop the return ret out of these brackets. i.e.

if (ret)
	class_destroy(uacce_class)

return ret;

> +	}
> +
> +	return 0;
> +}
> +
> +static __exit void uacce_exit(void)
> +{
> +	unregister_chrdev_region(uacce_devt, MINORMASK);
> +	class_destroy(uacce_class);
> +}
> +
> +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..04c8643
> --- /dev/null
> +++ b/include/linux/uacce.h
> @@ -0,0 +1,163 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +#ifndef _LINUX_UACCE_H
> +#define _LINUX_UACCE_H
> +
> +#include <linux/cdev.h>
> +#include <uapi/misc/uacce/uacce.h>
> +
> +#define UACCE_NAME		"uacce"
> +#define UACCE_QFRT_MAX		16
What does QFRT stand for?
> +#define UACCE_MAX_NAME_SIZE	64
> +
> +struct uacce_queue;
> +struct uacce_device;
> +
> +/**
> + * enum uacce_qfr_flag: queue file flag:
> + * @UACCE_QFRF_SELFMT: self maintained qfr
> + */
> +enum uacce_qfr_flag {
> +	UACCE_QFRF_SELFMT = BIT(0),
> +};

Same issue with enums for flags.  Doesn't make much sense to me.
Only one value can be taken which doesn't make it a flag.

> +
> +/**
> + * struct uacce_qfile_region - structure of queue file region
> + * @type: type of the qfr
> + * @flags: flags of qfr
> + * @prot: qfr protection flag
> + */
> +struct uacce_qfile_region {
> +	enum uacce_qfrt type;
> +	enum uacce_qfr_flag flags;
> +	u32 prot;
> +};
> +
> +/**
> + * struct uacce_ops - uacce device operations
> + * @get_available_instances:  get available instances left of the device
> + * @get_queue: get a queue from the device
> + * @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 uacce device
> + * @reset_queue: reset the queue
> + * @ioctl: ioctl for user space users of the queue
> + */
> +struct uacce_ops {
> +	int (*get_available_instances)(struct uacce_device *uacce);
> +	int (*get_queue)(struct uacce_device *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_device *uacce);
> +	int (*reset_queue)(struct uacce_queue *q);

Some of these aren't used on only existing driver.  Introduce them only
in the series that uses them.

> +	long (*ioctl)(struct uacce_queue *q, unsigned int cmd,
> +		      unsigned long arg);
> +};
> +
> +/**
> + * struct uacce_interface
I think this needs a description for kernel doc (even if it's obvious!)
Could be wrong though.

> + * @name: the uacce device name.  Will show up in sysfs
> + * @flags: uacce device attributes
> + * @ops: pointer to the struct uacce_ops
> + *
> + * This structure is used for the uacce_register()
> + */
> +struct uacce_interface {
> +	char name[UACCE_MAX_NAME_SIZE];
> +	enum uacce_dev_flag flags;
> +	struct uacce_ops *ops;
> +};
> +
> +enum uacce_q_state {
> +	UACCE_Q_INIT,
> +	UACCE_Q_STARTED,
> +	UACCE_Q_ZOMBIE,
> +};
> +
> +/**
> + * struct uacce_queue
> + * @uacce: pointer to uacce
> + * @priv: private pointer
> + * @wait: wait queue head
> + * @pasid: pasid of the queue
> + * @pid: pid of the process using the queue
> + * @handle: iommu_sva handle return from iommu_sva_bind_device
> + * @list: queue list
> + * @qfrs: pointer of qfr regions
> + * @state: queue state machine
> + */
> +struct uacce_queue {
> +	struct uacce_device *uacce;
> +	void *priv;
> +	wait_queue_head_t wait;
> +	int pasid;
> +	pid_t pid;
> +	struct iommu_sva *handle;
> +	struct list_head list;
> +	struct uacce_qfile_region *qfrs[UACCE_QFRT_MAX];
> +	enum uacce_q_state state;
> +};
> +
> +/**
> + * struct uacce_device
> + * @algs: supported algorithms
> + * @api_ver: api version
> + * @qf_pg_size: page size of the queue file regions
> + * @ops: pointer to the struct uacce_ops
> + * @pdev: pointer to the parent device
> + * @is_vf: whether virtual function
> + * @flags: uacce attributes
> + * @dev_id: id of the uacce device
> + * @prot: uacce protection flag
> + * @cdev: cdev of the uacce
> + * @dev: dev of the uacce
> + * @priv: private pointer of the uacce
> + * @qs: list head of queue->list
> + * @q_lock: lock for qs
> + */
> +struct uacce_device {
> +	const char *algs;
> +	const char *api_ver;
> +	unsigned long qf_pg_size[UACCE_QFRT_MAX];
> +	struct uacce_ops *ops;

Can we make this ops structure a point to a constant struct?
I'm guessing it'll be fixed for a given driver.

> +	struct device *pdev;

Perhaps just call it parent. pdev will be confusing with
pci devices.

> +	bool is_vf;
> +	u32 flags;
> +	u32 dev_id;
> +	u32 prot;
> +	struct cdev *cdev;

Can we embed the cdev structure rather than use a pointer
and separate allocation?

> +	struct device dev;
> +	void *priv;
> +	struct list_head qs;
> +	struct mutex q_lock;
> +};
> +
> +#if IS_ENABLED(CONFIG_UACCE)
> +
> +struct uacce_device *uacce_register(struct device *parent,
> +				    struct uacce_interface *interface);
> +void uacce_unregister(struct uacce_device *uacce);
> +
> +#else /* CONFIG_UACCE */
> +
> +static inline
> +struct uacce_device *uacce_register(struct device *parent,
> +				    struct uacce_interface *interface)
> +{
> +	return ERR_PTR(-ENODEV);
> +}
> +
> +static inline void uacce_unregister(struct uacce_device *uacce) {}
> +
> +#endif /* CONFIG_UACCE */
> +
> +#endif /* _LINUX_UACCE_H */
> diff --git a/include/uapi/misc/uacce/uacce.h b/include/uapi/misc/uacce/uacce.h
> new file mode 100644
> index 0000000..a4f9378
> --- /dev/null
> +++ b/include/uapi/misc/uacce/uacce.h
> @@ -0,0 +1,38 @@
> +/* SPDX-License-Identifier: GPL-2.0+ WITH Linux-syscall-note */
> +#ifndef _UAPIUUACCE_H
> +#define _UAPIUUACCE_H
> +
> +#include <linux/types.h>
> +#include <linux/ioctl.h>
> +
> +/* UACCE_CMD_START_Q: Start the queue */
> +#define UACCE_CMD_START_Q	_IO('W', 0)
> +
> +/**
> + * UACCE_CMD_PUT_Q:
> + * User actively stop queue and free queue resource immediately
> + * Optimization method since close fd may delay
> + */
> +#define UACCE_CMD_PUT_Q		_IO('W', 1)
> +
> +/**
> + * enum uacce_dev_flag: Device flags:
> + * @UACCE_DEV_SVA: Shared Virtual Addresses
> + *		   Support PASID
> + *		   Support device page faults (PCI PRI or SMMU Stall)
> + */
> +enum uacce_dev_flag {
> +	UACCE_DEV_SVA = BIT(0),

As mentioned in docs review, this doesn't look like an enum to me.
Just use #define for the bit and a suitable sized integer for any
calls using it.

> +};
> +
> +/**
> + * enum uacce_qfrt: qfrt type
> + * @UACCE_QFRT_MMIO: device mmio region
> + * @UACCE_QFRT_DUS: device user share region
> + */
> +enum uacce_qfrt {
> +	UACCE_QFRT_MMIO = 0,
> +	UACCE_QFRT_DUS = 1,
> +};
> +
> +#endif



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v7 3/3] crypto: hisilicon - register zip engine to uacce
  2019-10-29  6:40 ` [PATCH v7 3/3] crypto: hisilicon - register zip engine to uacce Zhangfei Gao
@ 2019-10-31 17:53   ` Jonathan Cameron
  2019-11-05  8:34     ` zhangfei
  0 siblings, 1 reply; 16+ messages in thread
From: Jonathan Cameron @ 2019-10-31 17:53 UTC (permalink / raw)
  To: Zhangfei Gao
  Cc: Greg Kroah-Hartman, Arnd Bergmann, Herbert Xu, grant.likely,
	jean-philippe, Jerome Glisse, ilias.apalodimas, francois.ozog,
	kenneth-lee-2012, Wangzhou, haojian . zhuang, guodong.xu,
	linux-accelerators, linux-kernel, linux-crypto, iommu

On Tue, 29 Oct 2019 14:40:16 +0800
Zhangfei Gao <zhangfei.gao@linaro.org> wrote:

> Register qm to uacce framework for user crypto driver
> 
> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
> Signed-off-by: Zhou Wang <wangzhou1@hisilicon.com>
Hi. 

This shows there is probably a race during setup that you should close.
Userspace interface is exposed before the driver is ready to handle it.

Few other bits inline.

Thanks,

Jonathan

> ---
>  drivers/crypto/hisilicon/qm.c           | 253 ++++++++++++++++++++++++++++++--
>  drivers/crypto/hisilicon/qm.h           |  13 +-
>  drivers/crypto/hisilicon/zip/zip_main.c |  39 ++---
>  include/uapi/misc/uacce/qm.h            |  23 +++
>  4 files changed, 292 insertions(+), 36 deletions(-)
>  create mode 100644 include/uapi/misc/uacce/qm.h
> 
> diff --git a/drivers/crypto/hisilicon/qm.c b/drivers/crypto/hisilicon/qm.c
> index a8ed6990..4b9cced 100644
> --- a/drivers/crypto/hisilicon/qm.c
> +++ b/drivers/crypto/hisilicon/qm.c
> @@ -9,6 +9,9 @@
>  #include <linux/log2.h>
>  #include <linux/seq_file.h>
>  #include <linux/slab.h>
> +#include <linux/uacce.h>
> +#include <linux/uaccess.h>
> +#include <uapi/misc/uacce/qm.h>
>  #include "qm.h"
>  
>  /* eq/aeq irq enable */
> @@ -465,17 +468,22 @@ static void qm_cq_head_update(struct hisi_qp *qp)
>  
>  static void qm_poll_qp(struct hisi_qp *qp, struct hisi_qm *qm)
>  {
> -	struct qm_cqe *cqe = qp->cqe + qp->qp_status.cq_head;
> -
> -	if (qp->req_cb) {
> -		while (QM_CQE_PHASE(cqe) == qp->qp_status.cqc_phase) {
> -			dma_rmb();
> -			qp->req_cb(qp, qp->sqe + qm->sqe_size * cqe->sq_head);
> -			qm_cq_head_update(qp);
> -			cqe = qp->cqe + qp->qp_status.cq_head;
> -			qm_db(qm, qp->qp_id, QM_DOORBELL_CMD_CQ,
> -			      qp->qp_status.cq_head, 0);
> -			atomic_dec(&qp->qp_status.used);
> +	struct qm_cqe *cqe;
> +
> +	if (qp->event_cb) {
> +		qp->event_cb(qp);
> +	} else {
> +		cqe = qp->cqe + qp->qp_status.cq_head;
> +
> +		if (qp->req_cb) {
> +			while (QM_CQE_PHASE(cqe) == qp->qp_status.cqc_phase) {
> +				dma_rmb();
> +				qp->req_cb(qp, qp->sqe + qm->sqe_size *
> +					   cqe->sq_head);
> +				qm_cq_head_update(qp);
> +				cqe = qp->cqe + qp->qp_status.cq_head;
> +				atomic_dec(&qp->qp_status.used);
> +			}
>  		}
>  
>  		/* set c_flag */
> @@ -1397,6 +1405,220 @@ static void hisi_qm_cache_wb(struct hisi_qm *qm)
>  	}
>  }
>  
> +static void qm_qp_event_notifier(struct hisi_qp *qp)
> +{
> +	wake_up_interruptible(&qp->uacce_q->wait);
> +}
> +
> +static int hisi_qm_get_available_instances(struct uacce_device *uacce)
> +{
> +	int i, ret;
> +	struct hisi_qm *qm = uacce->priv;
> +
> +	read_lock(&qm->qps_lock);
> +	for (i = 0, ret = 0; i < qm->qp_num; i++)
> +		if (!qm->qp_array[i])
> +			ret++;
> +	read_unlock(&qm->qps_lock);
> +
> +	return ret;
> +}
> +
> +static int hisi_qm_uacce_get_queue(struct uacce_device *uacce,
> +				   unsigned long arg,
> +				   struct uacce_queue *q)
> +{
> +	struct hisi_qm *qm = uacce->priv;
> +	struct hisi_qp *qp;
> +	u8 alg_type = 0;
> +
> +	qp = hisi_qm_create_qp(qm, alg_type);
> +	if (IS_ERR(qp))
> +		return PTR_ERR(qp);
> +
> +	q->priv = qp;
> +	q->uacce = uacce;
> +	qp->uacce_q = q;
> +	qp->event_cb = qm_qp_event_notifier;
> +	qp->pasid = arg;
> +
> +	return 0;
> +}
> +
> +static void hisi_qm_uacce_put_queue(struct uacce_queue *q)
> +{
> +	struct hisi_qp *qp = q->priv;
> +
> +	/*
> +	 * As put_queue is only called in uacce_mode=1, and only one queue can
We got rid of the modes I think so comment needs an update.

> +	 * be used in this mode. we flush all sqc cache back in put queue.
> +	 */
> +	hisi_qm_cache_wb(qp->qm);
> +
> +	/* need to stop hardware, but can not support in v1 */
> +	hisi_qm_release_qp(qp);

Should we just drop support for the v1 hardware if we can't do this?

> +}
> +
> +/* map sq/cq/doorbell to user space */
> +static int hisi_qm_uacce_mmap(struct uacce_queue *q,
> +			      struct vm_area_struct *vma,
> +			      struct uacce_qfile_region *qfr)
> +{
> +	struct hisi_qp *qp = q->priv;
> +	struct hisi_qm *qm = qp->qm;
> +	size_t sz = vma->vm_end - vma->vm_start;
> +	struct pci_dev *pdev = qm->pdev;
> +	struct device *dev = &pdev->dev;
> +	unsigned long vm_pgoff;
> +	int ret;
> +
> +	switch (qfr->type) {
> +	case UACCE_QFRT_MMIO:
> +		if (qm->ver == QM_HW_V2) {
> +			if (sz > PAGE_SIZE * (QM_DOORBELL_PAGE_NR +
> +			    QM_DOORBELL_SQ_CQ_BASE_V2 / PAGE_SIZE))
> +				return -EINVAL;
> +		} else {
> +			if (sz > PAGE_SIZE * QM_DOORBELL_PAGE_NR)
> +				return -EINVAL;
> +		}
> +
> +		vma->vm_flags |= VM_IO;
> +
> +		return remap_pfn_range(vma, vma->vm_start,
> +				       qm->phys_base >> PAGE_SHIFT,
> +				       sz, pgprot_noncached(vma->vm_page_prot));
> +	case UACCE_QFRT_DUS:
> +		if (sz != qp->qdma.size)
> +			return -EINVAL;
> +

Comment style in here is inconsistent. Match the existing code.

> +		/* dma_mmap_coherent() requires vm_pgoff as 0
> +		 * restore vm_pfoff to initial value for mmap()
> +		 */
> +		vm_pgoff = vma->vm_pgoff;
> +		vma->vm_pgoff = 0;
> +		ret = dma_mmap_coherent(dev, vma, qp->qdma.va,
> +					qp->qdma.dma, sz);
> +		vma->vm_pgoff = vm_pgoff;
> +		return ret;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int hisi_qm_uacce_start_queue(struct uacce_queue *q)
> +{
> +	struct hisi_qp *qp = q->priv;
> +
> +	return hisi_qm_start_qp(qp, qp->pasid);
> +}
> +
> +static void hisi_qm_uacce_stop_queue(struct uacce_queue *q)
> +{
> +	struct hisi_qp *qp = q->priv;
> +
> +	hisi_qm_stop_qp(qp);
I'm a great fan of minimalism on these
	hisi_qm_stop_qp(q->priv); doesn't really loose any clarity.

> +}
> +
> +static int qm_set_sqctype(struct uacce_queue *q, u16 type)
> +{
> +	struct hisi_qm *qm = q->uacce->priv;
> +	struct hisi_qp *qp = q->priv;
> +
> +	write_lock(&qm->qps_lock);
> +	qp->alg_type = type;
> +	write_unlock(&qm->qps_lock);
> +
> +	return 0;
> +}
> +
> +static long hisi_qm_uacce_ioctl(struct uacce_queue *q, unsigned int cmd,
> +				unsigned long arg)
> +{
> +	struct hisi_qp *qp = q->priv;
> +	struct hisi_qp_ctx qp_ctx;
> +
> +	if (cmd == UACCE_CMD_QM_SET_QP_CTX) {
> +		if (copy_from_user(&qp_ctx, (void __user *)arg,
> +				   sizeof(struct hisi_qp_ctx)))
> +			return -EFAULT;
> +
> +		if (qp_ctx.qc_type != 0 && qp_ctx.qc_type != 1)
> +			return -EINVAL;
> +
> +		qm_set_sqctype(q, qp_ctx.qc_type);
> +		qp_ctx.id = qp->qp_id;
> +
> +		if (copy_to_user((void __user *)arg, &qp_ctx,
> +				 sizeof(struct hisi_qp_ctx)))
> +			return -EFAULT;
> +	} else {
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static struct uacce_ops uacce_qm_ops = {
> +	.get_available_instances = hisi_qm_get_available_instances,
> +	.get_queue = hisi_qm_uacce_get_queue,
> +	.put_queue = hisi_qm_uacce_put_queue,
> +	.start_queue = hisi_qm_uacce_start_queue,
> +	.stop_queue = hisi_qm_uacce_stop_queue,
> +	.mmap = hisi_qm_uacce_mmap,
> +	.ioctl = hisi_qm_uacce_ioctl,
> +};
> +
> +static int qm_register_uacce(struct hisi_qm *qm)
> +{
> +	struct pci_dev *pdev = qm->pdev;
> +	struct uacce_device *uacce;
> +	unsigned long mmio_page_nr;
> +	unsigned long dus_page_nr;
> +	struct uacce_interface interface = {
> +		.flags = UACCE_DEV_SVA,
> +		.ops = &uacce_qm_ops,
> +	};
> +
> +	strncpy(interface.name, pdev->driver->name, sizeof(interface.name));
> +
> +	uacce = uacce_register(&pdev->dev, &interface);
> +	if (IS_ERR(uacce))
> +		return PTR_ERR(uacce);

Is there a potential race here as we have exposed the character device before
the driver is ready for it to be used?  Probably need to split the code that
allocates a uacce interface from the bit that actually exposes it to userspace.

> +
> +	if (uacce->flags & UACCE_DEV_SVA) {
> +		qm->use_sva = true;
> +	} else {
> +		/* only consider sva case */
> +		uacce_unregister(uacce);
> +		return -EINVAL;
> +	}
> +
> +	uacce->is_vf = pdev->is_virtfn;
> +	uacce->priv = qm;
> +	uacce->algs = qm->algs;
> +
> +	if (qm->ver == QM_HW_V1) {
> +		mmio_page_nr = QM_DOORBELL_PAGE_NR;
> +		uacce->api_ver = HISI_QM_API_VER_BASE;
> +	} else {
> +		mmio_page_nr = QM_DOORBELL_PAGE_NR +
> +			QM_DOORBELL_SQ_CQ_BASE_V2 / PAGE_SIZE;
> +		uacce->api_ver = HISI_QM_API_VER2_BASE;
> +	}
> +
> +	dus_page_nr = (PAGE_SIZE - 1 + qm->sqe_size * QM_Q_DEPTH +
> +		       sizeof(struct qm_cqe) * QM_Q_DEPTH) >> PAGE_SHIFT;
> +
> +	uacce->qf_pg_size[UACCE_QFRT_MMIO] = mmio_page_nr;
> +	uacce->qf_pg_size[UACCE_QFRT_DUS]  = dus_page_nr;
> +
> +	qm->uacce = uacce;
> +
> +	return 0;
> +}
> +
>  /**
>   * hisi_qm_init() - Initialize configures about qm.
>   * @qm: The qm needing init.
> @@ -1421,6 +1643,10 @@ int hisi_qm_init(struct hisi_qm *qm)
>  		return -EINVAL;
>  	}
>  
> +	ret = qm_register_uacce(qm);
> +	if (ret < 0)
> +		dev_warn(&pdev->dev, "fail to register uacce (%d)\n", ret);
> +

looks like there are error paths in qm_init in which we should call
the uacce_unregister?

>  	ret = pci_enable_device_mem(pdev);
>  	if (ret < 0) {
>  		dev_err(&pdev->dev, "Failed to enable device mem!\n");
> @@ -1433,6 +1659,8 @@ int hisi_qm_init(struct hisi_qm *qm)
>  		goto err_disable_pcidev;
>  	}
>  
> +	qm->phys_base = pci_resource_start(pdev, PCI_BAR_2);
> +	qm->size = pci_resource_len(qm->pdev, PCI_BAR_2);
>  	qm->io_base = ioremap(pci_resource_start(pdev, PCI_BAR_2),
>  			      pci_resource_len(qm->pdev, PCI_BAR_2));

Use qm->phys_base/size in the ioremap here to avoid repeating the code.

>  	if (!qm->io_base) {
> @@ -1504,6 +1732,9 @@ void hisi_qm_uninit(struct hisi_qm *qm)
>  	iounmap(qm->io_base);
>  	pci_release_mem_regions(pdev);
>  	pci_disable_device(pdev);
> +
> +	if (qm->uacce)
> +		uacce_unregister(qm->uacce);

Can we make uacce_unregister check the input?
Might make for cleaner users.

>  }
>  EXPORT_SYMBOL_GPL(hisi_qm_uninit);
>  
> diff --git a/drivers/crypto/hisilicon/qm.h b/drivers/crypto/hisilicon/qm.h
> index 103e2fd..84a3be9 100644
> --- a/drivers/crypto/hisilicon/qm.h
> +++ b/drivers/crypto/hisilicon/qm.h
> @@ -77,6 +77,10 @@
>  
>  #define HISI_ACC_SGL_SGE_NR_MAX		255
>  
> +/* page number for queue file region */
> +#define QM_DOORBELL_PAGE_NR		1
> +

1 blank line only is almost always enough.

> +
>  enum qp_state {
>  	QP_STOP,
>  };
> @@ -161,7 +165,12 @@ struct hisi_qm {
>  	u32 error_mask;
>  	u32 msi_mask;
>  
> +	const char *algs;
>  	bool use_dma_api;
> +	bool use_sva;
> +	resource_size_t phys_base;
> +	resource_size_t size;
> +	struct uacce_device *uacce;
>  };
>  
>  struct hisi_qp_status {
> @@ -191,10 +200,12 @@ struct hisi_qp {
>  	struct hisi_qp_ops *hw_ops;
>  	void *qp_ctx;
>  	void (*req_cb)(struct hisi_qp *qp, void *data);
> +	void (*event_cb)(struct hisi_qp *qp);
>  	struct work_struct work;
>  	struct workqueue_struct *wq;
> -

unrelated change.

>  	struct hisi_qm *qm;
> +	u16 pasid;
> +	struct uacce_queue *uacce_q;
>  };
>  
>  int hisi_qm_init(struct hisi_qm *qm);
> diff --git a/drivers/crypto/hisilicon/zip/zip_main.c b/drivers/crypto/hisilicon/zip/zip_main.c
> index 1b2ee96..48860d2 100644
> --- a/drivers/crypto/hisilicon/zip/zip_main.c
> +++ b/drivers/crypto/hisilicon/zip/zip_main.c
> @@ -316,8 +316,14 @@ static void hisi_zip_set_user_domain_and_cache(struct hisi_zip *hisi_zip)
>  	writel(AXUSER_BASE, base + HZIP_BD_RUSER_32_63);
>  	writel(AXUSER_BASE, base + HZIP_SGL_RUSER_32_63);
>  	writel(AXUSER_BASE, base + HZIP_BD_WUSER_32_63);
> -	writel(AXUSER_BASE, base + HZIP_DATA_RUSER_32_63);
> -	writel(AXUSER_BASE, base + HZIP_DATA_WUSER_32_63);
> +
> +	if (hisi_zip->qm.use_sva) {
> +		writel(AXUSER_BASE | AXUSER_SSV, base + HZIP_DATA_RUSER_32_63);
> +		writel(AXUSER_BASE | AXUSER_SSV, base + HZIP_DATA_WUSER_32_63);
> +	} else {
> +		writel(AXUSER_BASE, base + HZIP_DATA_RUSER_32_63);
> +		writel(AXUSER_BASE, base + HZIP_DATA_WUSER_32_63);
> +	}
>  
>  	/* let's open all compression/decompression cores */
>  	writel(DECOMP_CHECK_ENABLE | ALL_COMP_DECOMP_EN,
> @@ -671,24 +677,12 @@ static int hisi_zip_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  	qm = &hisi_zip->qm;
>  	qm->pdev = pdev;
>  	qm->ver = rev_id;
> -

Try to avoid noise from white space changes.  No huge help to delete the blank line here.

> +	qm->use_dma_api = true;
> +	qm->algs = "zlib\ngzip\n";
>  	qm->sqe_size = HZIP_SQE_SIZE;
>  	qm->dev_name = hisi_zip_name;
>  	qm->fun_type = (pdev->device == PCI_DEVICE_ID_ZIP_PF) ? QM_HW_PF :
>  								QM_HW_VF;

Unrelated changes I think.  Can we clean out the old left overs
of uacce from the driver in a precursor patch? Also if it's no longer
used can we drop the module param?

> -	switch (uacce_mode) {
> -	case 0:
> -		qm->use_dma_api = true;
> -		break;
> -	case 1:
> -		qm->use_dma_api = false;
> -		break;
> -	case 2:
> -		qm->use_dma_api = true;
> -		break;
> -	default:
> -		return -EINVAL;
> -	}
>  
>  	ret = hisi_qm_init(qm);
>  	if (ret) {
> @@ -976,12 +970,10 @@ static int __init hisi_zip_init(void)
>  		goto err_pci;
>  	}
>  
> -	if (uacce_mode == 0 || uacce_mode == 2) {
> -		ret = hisi_zip_register_to_crypto();
> -		if (ret < 0) {
> -			pr_err("Failed to register driver to crypto.\n");
> -			goto err_crypto;
> -		}
> +	ret = hisi_zip_register_to_crypto();
> +	if (ret < 0) {
> +		pr_err("Failed to register driver to crypto.\n");
> +		goto err_crypto;
>  	}
>  
>  	return 0;
> @@ -996,8 +988,7 @@ static int __init hisi_zip_init(void)
>  
>  static void __exit hisi_zip_exit(void)
>  {
> -	if (uacce_mode == 0 || uacce_mode == 2)
> -		hisi_zip_unregister_from_crypto();
> +	hisi_zip_unregister_from_crypto();



>  	pci_unregister_driver(&hisi_zip_pci_driver);
>  	hisi_zip_unregister_debugfs();
>  }
> diff --git a/include/uapi/misc/uacce/qm.h b/include/uapi/misc/uacce/qm.h
> new file mode 100644
> index 0000000..d79a8f2
> --- /dev/null
> +++ b/include/uapi/misc/uacce/qm.h
Given generic directory (assuming uacce becomes heavily used) probably
want to prefix that if it is unique to hisilicon.

hisi_qm.h?

> @@ -0,0 +1,23 @@
> +/* SPDX-License-Identifier: GPL-2.0+ WITH Linux-syscall-note */
> +#ifndef HISI_QM_USR_IF_H
> +#define HISI_QM_USR_IF_H
> +
> +#include <linux/types.h>
> +
> +/**
> + * struct hisi_qp_ctx - User data for hisi qp.
> + * @id: Specifies which Turbo decode algorithm to use

What's a Turbo algorithm?  I don't know and I have the manuals ;)

> + * @qc_type: Accelerator algorithm type
> + */
> +struct hisi_qp_ctx {
> +	__u16 id;
> +	__u16 qc_type;
> +};
> +
> +#define HISI_QM_API_VER_BASE "hisi_qm_v1"
> +#define HISI_QM_API_VER2_BASE "hisi_qm_v2"
> +
> +/* UACCE_CMD_QM_SET_QP_CTX: Set qp algorithm type */
> +#define UACCE_CMD_QM_SET_QP_CTX	_IOWR('H', 10, struct hisi_qp_ctx)
> +
> +#endif



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v7 2/3] uacce: add uacce driver
  2019-10-31 17:13   ` Jonathan Cameron
@ 2019-11-05  7:43     ` zhangfei
  2019-11-11 11:19       ` Jonathan Cameron
  0 siblings, 1 reply; 16+ messages in thread
From: zhangfei @ 2019-11-05  7:43 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: Greg Kroah-Hartman, Arnd Bergmann, Herbert Xu, grant.likely,
	jean-philippe, Jerome Glisse, ilias.apalodimas, francois.ozog,
	kenneth-lee-2012, Wangzhou, haojian . zhuang, guodong.xu,
	linux-accelerators, linux-kernel, linux-crypto, iommu,
	Kenneth Lee, Zaibo Xu

Hi, Jonathan

Thanks for the suggestions

On 2019/11/1 上午1:13, Jonathan Cameron wrote:
> On Tue, 29 Oct 2019 14:40:15 +0800
> Zhangfei Gao <zhangfei.gao@linaro.org> wrote:
>
>> From: Kenneth Lee <liguozhu@hisilicon.com>
>>
>> Uacce (Unified/User-space-access-intended Accelerator Framework) targets to
>> provide Shared Virtual Addressing (SVA) between accelerators and processes.
>> So accelerator can access any data structure of the main cpu.
>> This differs from the data sharing between cpu and io device, which share
>> data content rather than address.
>> Since unified address, hardware and user space of process can share the
>> same virtual address in the communication.
>>
>> Uacce create a chrdev for every registration, the queue is allocated to
>> the process when the chrdev is opened. Then the process can access the
>> hardware resource by interact with the queue file. By mmap the queue
>> file space to user space, the process can directly put requests to the
>> hardware without syscall to the kernel space.
>>
>> Signed-off-by: Kenneth Lee <liguozhu@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>
> Great, much more compact.
>
> I've not gone through this in detail yet but a few initial comments inline.
>
> Thanks,
>
> Jonathan
>
>> ---
>>   Documentation/ABI/testing/sysfs-driver-uacce |  53 +++
>>   drivers/misc/Kconfig                         |   1 +
>>   drivers/misc/Makefile                        |   1 +
>>   drivers/misc/uacce/Kconfig                   |  13 +
>>   drivers/misc/uacce/Makefile                  |   2 +
>>   drivers/misc/uacce/uacce.c                   | 574 +++++++++++++++++++++++++++
>>   include/linux/uacce.h                        | 163 ++++++++
>>   include/uapi/misc/uacce/uacce.h              |  38 ++
>>   8 files changed, 845 insertions(+)
>>   create mode 100644 Documentation/ABI/testing/sysfs-driver-uacce
>>   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/uacce.h
>>
>> diff --git a/Documentation/ABI/testing/sysfs-driver-uacce b/Documentation/ABI/testing/sysfs-driver-uacce
>> new file mode 100644
>> index 0000000..35699dc
>> --- /dev/null
>> +++ b/Documentation/ABI/testing/sysfs-driver-uacce
>> @@ -0,0 +1,53 @@
>> +What:           /sys/class/uacce/<dev_name>/id
>> +Date:           Oct 2019
>> +KernelVersion:  5.5
>> +Contact:        linux-accelerators@lists.ozlabs.org
>> +Description:    Id of the device.
>> +
>> +What:           /sys/class/uacce/<dev_name>/api
>> +Date:           Oct 2019
>> +KernelVersion:  5.5
>> +Contact:        linux-accelerators@lists.ozlabs.org
>> +Description:    Api of the device, used by application to match the correct driver
>> +
>> +What:           /sys/class/uacce/<dev_name>/flags
>> +Date:           Oct 2019
>> +KernelVersion:  5.5
>> +Contact:        linux-accelerators@lists.ozlabs.org
>> +Description:    Attributes of the device, see UACCE_DEV_xxx flag defined in uacce.h
>> +
>> +What:           /sys/class/uacce/<dev_name>/available_instances
>> +Date:           Oct 2019
>> +KernelVersion:  5.5
>> +Contact:        linux-accelerators@lists.ozlabs.org
>> +Description:    Available instances left of the device
>> +
>> +What:           /sys/class/uacce/<dev_name>/algorithms
>> +Date:           Oct 2019
>> +KernelVersion:  5.5
>> +Contact:        linux-accelerators@lists.ozlabs.org
>> +Description:    Algorithms supported by this accelerator
> How are they separated?  Userspace code needs to know that.
> (comma, tab, newline?)
Yes, will add "separated by new line"
>
>> +
>> +What:           /sys/class/uacce/<dev_name>/qfrt_mmio_size
> qfrt is not the most obvious naming ever.  Do we care beyond its
> a region for this interface?  region_mmio_size maybe?
OK,
>
>> +Date:           Oct 2019
>> +KernelVersion:  5.5
>> +Contact:        linux-accelerators@lists.ozlabs.org
>> +Description:    Page size of mmio region queue file
> Size of page in this region, or number of pages in the region?
Change to "Page numbers of mmio region queue file"
>
>> +
>> +What:           /sys/class/uacce/<dev_name>/qfrt_dus_size
>> +Date:           Oct 2019
>> +KernelVersion:  5.5
>> +Contact:        linux-accelerators@lists.ozlabs.org
>> +Description:    Page size of dus region queue file
>> +
>> +What:           /sys/class/uacce/<dev_name>/numa_distance
>> +Date:           Oct 2019
>> +KernelVersion:  5.5
>> +Contact:        linux-accelerators@lists.ozlabs.org
>> +Description:    Distance of device node to cpu node
> I wonder if we should be doing this in here. There are other standard
> ways of obtaining this for the device.  Follow parent and check node_id
> there then use the /sys/bus/node path to find out the distances.
Could you clarify more about this method.
The purpose here is cpu searching the nearest device(zip) doing work.
Does user application know which node it is running and compare distance?
>> +
>> +What:           /sys/class/uacce/<dev_name>/node_id
>> +Date:           Oct 2019
>> +KernelVersion:  5.5
>> +Contact:        linux-accelerators@lists.ozlabs.org
>> +Description:    Id of the numa node
>> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
>> index c55b637..929feb0 100644
>> --- a/drivers/misc/Kconfig
>> +++ b/drivers/misc/Kconfig
>> @@ -481,4 +481,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 c1860d3..9abf292 100644
>> --- a/drivers/misc/Makefile
>> +++ b/drivers/misc/Makefile
>> @@ -56,4 +56,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..5e39b60
>> --- /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/uacce.h
>> +
>> +	  See Documentation/misc-devices/uacce.rst for more details.
>> +
>> +	  If you don't know what to do here, say N.
> Pessimist :) Everyone should want uacce so don't put them off.  Having said
> that perhaps for now it should be hidden and enabled on a driver by driver
> basis?
>
>> 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..2b6b038
>> --- /dev/null
>> +++ b/drivers/misc/uacce/uacce.c
>> @@ -0,0 +1,574 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +#include <linux/compat.h>
>> +#include <linux/dma-iommu.h>
>> +#include <linux/module.h>
>> +#include <linux/poll.h>
>> +#include <linux/uacce.h>
>> +
>> +static struct class *uacce_class;
>> +static dev_t uacce_devt;
>> +static DEFINE_MUTEX(uacce_mutex);
>> +static DEFINE_XARRAY_ALLOC(uacce_xa);
>> +
>> +static int uacce_start_queue(struct uacce_queue *q)
>> +{
>> +	int ret = -EINVAL;
>> +
>> +	mutex_lock(&uacce_mutex);
>> +
>> +	if (q->state != UACCE_Q_INIT)
>> +		goto out_with_lock;
>> +
>> +	if (q->uacce->ops->start_queue) {
>> +		ret = q->uacce->ops->start_queue(q);
>> +		if (ret < 0)
>> +			goto out_with_lock;
>> +	}
>> +
>> +	q->state = UACCE_Q_STARTED;
> out_with_lock:
>> +	mutex_unlock(&uacce_mutex);
>> +
> return ret;
> Though need to handle ret a bit differently above...
OK
>
> +static int uacce_fops_mmap(struct file *filep, struct vm_area_struct *vma)
> +{
> +	struct uacce_queue *q = filep->private_data;
> +	struct uacce_device *uacce = q->uacce;
> +	struct uacce_qfile_region *qfr;
> +	enum uacce_qfrt type = 0;
> +	unsigned int flags = 0;
> +	int ret;
> +
> +	if (vma->vm_pgoff < UACCE_QFRT_MAX)
> +		type = vma->vm_pgoff;
> +
> +	vma->vm_flags |= VM_DONTCOPY | VM_DONTEXPAND | VM_WIPEONFORK;
> +	vma->vm_ops = &uacce_vm_ops;
> +	vma->vm_private_data = q;
> +
> +	mutex_lock(&uacce_mutex);
> +
> +	if (q->qfrs[type]) {
> +		ret = -EEXIST;
> +		goto out_with_lock;
> +	}
> +
> +	switch (type) {
> +	case UACCE_QFRT_MMIO:
> +		flags = UACCE_QFRF_SELFMT;
> +		break;
> +
> +	case UACCE_QFRT_DUS:
> +		if (uacce->flags & UACCE_DEV_SVA) {
> +			flags = UACCE_QFRF_SELFMT;
> +			break;
> +		}
> +		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;
> +
> Could put
> out_with_lock:
> here and return ret instead of 0.
> You'll need to set ret to default to 0 in that
> case though.
OK
>
> +static ssize_t algorithms_show(struct device *dev,
> +			       struct device_attribute *attr, char *buf)
> +{
> +	struct uacce_device *uacce = to_uacce_device(dev);
> +
> +	return sprintf(buf, "%s", uacce->algs);
> Any risk algs won't have the \n?
> I'd kind of expect it to be a null termated arrays to allow the core
> to format it however it wants to.
Yes, adding \n is better.
>
>> +}
>> +
>> +static ssize_t qfrt_mmio_size_show(struct device *dev,
>> +				   struct device_attribute *attr, char *buf)
>> +{
>> +	struct uacce_device *uacce = to_uacce_device(dev);
>> +
>> +	return sprintf(buf, "%lu\n",
>> +		       uacce->qf_pg_size[UACCE_QFRT_MMIO] << PAGE_SHIFT);
>> +}
>> +
>> +static ssize_t qfrt_dus_size_show(struct device *dev,
>> +				  struct device_attribute *attr, char *buf)
>> +{
>> +	struct uacce_device *uacce = to_uacce_device(dev);
>> +
>> +	return sprintf(buf, "%lu\n",
>> +		       uacce->qf_pg_size[UACCE_QFRT_DUS] << PAGE_SHIFT);
>> +}
>> +
>> +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(qfrt_mmio_size);
>> +static DEVICE_ATTR_RO(qfrt_dus_size);
>> +
>> +static struct attribute *uacce_dev_attrs[] = {
>> +	&dev_attr_id.attr,
>> +	&dev_attr_api.attr,
>> +	&dev_attr_node_id.attr,
>> +	&dev_attr_numa_distance.attr,
>> +	&dev_attr_flags.attr,
>> +	&dev_attr_available_instances.attr,
>> +	&dev_attr_algorithms.attr,
>> +	&dev_attr_qfrt_mmio_size.attr,
>> +	&dev_attr_qfrt_dus_size.attr,
>> +	NULL,
>> +};
>> +ATTRIBUTE_GROUPS(uacce_dev);
>> +
>> +static void uacce_release(struct device *dev)
>> +{
>> +	struct uacce_device *uacce = to_uacce_device(dev);
>> +
>> +	kfree(uacce);
>> +}
>> +
>> +/**
>> + * uacce_register - register an accelerator
> This isn't quite correct kernel-doc.  Please run the
> generation script over it and fix any warnings.
>
> 	uacce_register() - register an accelerator
Sure, will add (), though no warning reported from ./scripts/kernel-doc
>
>> + * @parent: pointer of uacce parent device
>> + * @interface: pointer of uacce_interface for register
>> + */
>> +struct uacce_device *uacce_register(struct device *parent,
>> +				    struct uacce_interface *interface)
>> +{
>> +	unsigned int flags = interface->flags;
>> +	struct uacce_device *uacce;
>> +	int ret;
>> +
>> +	uacce = kzalloc(sizeof(struct uacce_device), GFP_KERNEL);
>> +	if (!uacce)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	if (flags & UACCE_DEV_SVA) {
>> +		ret = iommu_dev_enable_feature(parent, IOMMU_DEV_FEAT_SVA);
>> +		if (ret)
>> +			flags &= ~UACCE_DEV_SVA;
>> +	}
>> +
>> +	uacce->pdev = parent;
>> +	uacce->flags = flags;
>> +	uacce->ops = interface->ops;
>> +
>> +	ret = xa_alloc(&uacce_xa, &uacce->dev_id, uacce, xa_limit_32b,
>> +		       GFP_KERNEL);
>> +	if (ret < 0)
>> +		goto err_with_uacce;
>> +
>> +	uacce->cdev = cdev_alloc();
> If we can embed this (see below) then use cdev_init instead.
>
>> +	if (!uacce->cdev) {
>> +		ret = -ENOMEM;
>> +		goto err_with_xa;
>> +	}
>> +
>> +	INIT_LIST_HEAD(&uacce->qs);
>> +	mutex_init(&uacce->q_lock);
>> +	uacce->cdev->ops = &uacce_fops;
>> +	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_groups;
>> +	uacce->dev.parent = uacce->pdev;
>> +	uacce->dev.release = uacce_release;
>> +	dev_set_name(&uacce->dev, "%s-%d", interface->name, uacce->dev_id);
>> +	ret = cdev_device_add(uacce->cdev, &uacce->dev);
>> +	if (ret)
>> +		goto err_with_xa;
>> +
>> +	return uacce;
>> +
>> +err_with_xa:
>> +	if (uacce->cdev)
>> +		cdev_del(uacce->cdev);
> Why not use a separate label to handle the above rather than checking if
> it's set?
ok,
>
>> +	xa_erase(&uacce_xa, uacce->dev_id);
>> +err_with_uacce:
>> +	if (flags & UACCE_DEV_SVA)
>> +		iommu_dev_disable_feature(uacce->pdev, IOMMU_DEV_FEAT_SVA);
>> +	kfree(uacce);
>> +	return ERR_PTR(ret);
>> +}
>> +EXPORT_SYMBOL_GPL(uacce_register);
>> +
>> +/**
>> + * uacce_unregister - unregisters an accelerator
>> + * @uacce: the accelerator to unregister
>> + */
>> +void uacce_unregister(struct uacce_device *uacce)
>> +{
>> +	if (!uacce)
>> +		return;
>> +
> I'd like to see a comment here on why we are doing things not unwinding
> actions from uacce_register.
OK will add comments.
Here is "ensure no open queue remains"
>> +	mutex_lock(&uacce->q_lock);
>> +	if (!list_empty(&uacce->qs)) {
>> +		struct uacce_queue *q;
>> +
>> +		list_for_each_entry(q, &uacce->qs, list) {
>> +			uacce_put_queue(q);
>> +			if (uacce->flags & UACCE_DEV_SVA)
>> +				iommu_sva_unbind_device(q->handle);
>> +		}
>> +	}
>> +	mutex_unlock(&uacce->q_lock);
>> +
> For these next parts which are the unwind of uacce_register, why are they not
> in the reverse order of what is happening in there (where possible given
> device lifespan). That is why do we not disable the iommu feature much later?
First close all queues, then disable sva feature.
>
>> +	if (uacce->flags & UACCE_DEV_SVA)
>> +		iommu_dev_disable_feature(uacce->pdev, IOMMU_DEV_FEAT_SVA);
>> +
>> +	cdev_device_del(uacce->cdev, &uacce->dev);
>> +	xa_erase(&uacce_xa, uacce->dev_id);
>> +	put_device(&uacce->dev);
>> +}
>> +EXPORT_SYMBOL_GPL(uacce_unregister);
>> +
>> +static int __init uacce_init(void)
>> +{
>> +	int ret;
>> +
>> +	uacce_class = class_create(THIS_MODULE, UACCE_NAME);
>> +	if (IS_ERR(uacce_class))
>> +		return PTR_ERR(uacce_class);
>> +
>> +	ret = alloc_chrdev_region(&uacce_devt, 0, MINORMASK, UACCE_NAME);
>> +	if (ret) {
>> +		class_destroy(uacce_class);
>> +		return ret;
> drop the return ret out of these brackets. i.e.
>
> if (ret)
> 	class_destroy(uacce_class)
>
> return ret;
sure, thanks
>
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static __exit void uacce_exit(void)
>> +{
>> +	unregister_chrdev_region(uacce_devt, MINORMASK);
>> +	class_destroy(uacce_class);
>> +}
>> +
>> +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..04c8643
>> --- /dev/null
>> +++ b/include/linux/uacce.h
>> @@ -0,0 +1,163 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +#ifndef _LINUX_UACCE_H
>> +#define _LINUX_UACCE_H
>> +
>> +#include <linux/cdev.h>
>> +#include <uapi/misc/uacce/uacce.h>
>> +
>> +#define UACCE_NAME		"uacce"
>> +#define UACCE_QFRT_MAX		16
> What does QFRT stand for?
change to UACCE_MAX_REGION
>> +#define UACCE_MAX_NAME_SIZE	64
>> +
>> +struct uacce_queue;
>> +struct uacce_device;
>> +
>> +/**
>> + * enum uacce_qfr_flag: queue file flag:
>> + * @UACCE_QFRF_SELFMT: self maintained qfr
>> + */
>> +enum uacce_qfr_flag {
>> +	UACCE_QFRF_SELFMT = BIT(0),
>> +};
> Same issue with enums for flags.  Doesn't make much sense to me.
> Only one value can be taken which doesn't make it a flag.
>
>> +
>> +/**
>> + * struct uacce_qfile_region - structure of queue file region
>> + * @type: type of the qfr
>> + * @flags: flags of qfr
>> + * @prot: qfr protection flag
>> + */
>> +struct uacce_qfile_region {
>> +	enum uacce_qfrt type;
>> +	enum uacce_qfr_flag flags;
>> +	u32 prot;
>> +};
>> +
>> +/**
>> + * struct uacce_ops - uacce device operations
>> + * @get_available_instances:  get available instances left of the device
>> + * @get_queue: get a queue from the device
>> + * @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 uacce device
>> + * @reset_queue: reset the queue
>> + * @ioctl: ioctl for user space users of the queue
>> + */
>> +struct uacce_ops {
>> +	int (*get_available_instances)(struct uacce_device *uacce);
>> +	int (*get_queue)(struct uacce_device *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_device *uacce);
>> +	int (*reset_queue)(struct uacce_queue *q);
> Some of these aren't used on only existing driver.  Introduce them only
> in the series that uses them.
OK
>
>> +	long (*ioctl)(struct uacce_queue *q, unsigned int cmd,
>> +		      unsigned long arg);
>> +};
>> +
>> +/**
>> + * struct uacce_interface
> I think this needs a description for kernel doc (even if it's obvious!)
> Could be wrong though.
OK
>
>> + * @name: the uacce device name.  Will show up in sysfs
>> + * @flags: uacce device attributes
>> + * @ops: pointer to the struct uacce_ops
>> + *
>> + * This structure is used for the uacce_register()
>> + */
>> +struct uacce_interface {
>> +	char name[UACCE_MAX_NAME_SIZE];
>> +	enum uacce_dev_flag flags;
>> +	struct uacce_ops *ops;
>> +};
>> +
>> +enum uacce_q_state {
>> +	UACCE_Q_INIT,
>> +	UACCE_Q_STARTED,
>> +	UACCE_Q_ZOMBIE,
>> +};
>> +
>> +/**
>> + * struct uacce_queue
>> + * @uacce: pointer to uacce
>> + * @priv: private pointer
>> + * @wait: wait queue head
>> + * @pasid: pasid of the queue
>> + * @pid: pid of the process using the queue
>> + * @handle: iommu_sva handle return from iommu_sva_bind_device
>> + * @list: queue list
>> + * @qfrs: pointer of qfr regions
>> + * @state: queue state machine
>> + */
>> +struct uacce_queue {
>> +	struct uacce_device *uacce;
>> +	void *priv;
>> +	wait_queue_head_t wait;
>> +	int pasid;
>> +	pid_t pid;
>> +	struct iommu_sva *handle;
>> +	struct list_head list;
>> +	struct uacce_qfile_region *qfrs[UACCE_QFRT_MAX];
>> +	enum uacce_q_state state;
>> +};
>> +
>> +/**
>> + * struct uacce_device
>> + * @algs: supported algorithms
>> + * @api_ver: api version
>> + * @qf_pg_size: page size of the queue file regions
>> + * @ops: pointer to the struct uacce_ops
>> + * @pdev: pointer to the parent device
>> + * @is_vf: whether virtual function
>> + * @flags: uacce attributes
>> + * @dev_id: id of the uacce device
>> + * @prot: uacce protection flag
>> + * @cdev: cdev of the uacce
>> + * @dev: dev of the uacce
>> + * @priv: private pointer of the uacce
>> + * @qs: list head of queue->list
>> + * @q_lock: lock for qs
>> + */
>> +struct uacce_device {
>> +	const char *algs;
>> +	const char *api_ver;
>> +	unsigned long qf_pg_size[UACCE_QFRT_MAX];
>> +	struct uacce_ops *ops;
> Can we make this ops structure a point to a constant struct?
> I'm guessing it'll be fixed for a given driver.
OK
>
>> +	struct device *pdev;
> Perhaps just call it parent. pdev will be confusing with
> pci devices.
OK
>
>> +	bool is_vf;
>> +	u32 flags;
>> +	u32 dev_id;
>> +	u32 prot;
>> +	struct cdev *cdev;
> Can we embed the cdev structure rather than use a pointer
> and separate allocation?
NO, we can't.
We originally embed the cdev structure, and Greg reminded us these two 
structure have different lifetime.
https://lkml.org/lkml/2019/8/28/771
>> +	struct device dev;
>> +	void *priv;
>> +	struct list_head qs;
>> +	struct mutex q_lock;
>> +};
>> +
>> +#if IS_ENABLED(CONFIG_UACCE)
>> +
>> +struct uacce_device *uacce_register(struct device *parent,
>> +				    struct uacce_interface *interface);
>> +void uacce_unregister(struct uacce_device *uacce);
>> +
>> +#else /* CONFIG_UACCE */
>> +
>> +static inline
>> +struct uacce_device *uacce_register(struct device *parent,
>> +				    struct uacce_interface *interface)
>> +{
>> +	return ERR_PTR(-ENODEV);
>> +}
>> +
>> +static inline void uacce_unregister(struct uacce_device *uacce) {}
>> +
>> +#endif /* CONFIG_UACCE */
>> +
>> +#endif /* _LINUX_UACCE_H */
>> diff --git a/include/uapi/misc/uacce/uacce.h b/include/uapi/misc/uacce/uacce.h
>> new file mode 100644
>> index 0000000..a4f9378
>> --- /dev/null
>> +++ b/include/uapi/misc/uacce/uacce.h
>> @@ -0,0 +1,38 @@
>> +/* SPDX-License-Identifier: GPL-2.0+ WITH Linux-syscall-note */
>> +#ifndef _UAPIUUACCE_H
>> +#define _UAPIUUACCE_H
>> +
>> +#include <linux/types.h>
>> +#include <linux/ioctl.h>
>> +
>> +/* UACCE_CMD_START_Q: Start the queue */
>> +#define UACCE_CMD_START_Q	_IO('W', 0)
>> +
>> +/**
>> + * UACCE_CMD_PUT_Q:
>> + * User actively stop queue and free queue resource immediately
>> + * Optimization method since close fd may delay
>> + */
>> +#define UACCE_CMD_PUT_Q		_IO('W', 1)
>> +
>> +/**
>> + * enum uacce_dev_flag: Device flags:
>> + * @UACCE_DEV_SVA: Shared Virtual Addresses
>> + *		   Support PASID
>> + *		   Support device page faults (PCI PRI or SMMU Stall)
>> + */
>> +enum uacce_dev_flag {
>> +	UACCE_DEV_SVA = BIT(0),
> As mentioned in docs review, this doesn't look like an enum to me.
> Just use #define for the bit and a suitable sized integer for any
> calls using it.
OK, but there are still more features in the future patch.

Thanks


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v7 3/3] crypto: hisilicon - register zip engine to uacce
  2019-10-31 17:53   ` Jonathan Cameron
@ 2019-11-05  8:34     ` zhangfei
  2019-11-11 11:30       ` Jonathan Cameron
  0 siblings, 1 reply; 16+ messages in thread
From: zhangfei @ 2019-11-05  8:34 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: Greg Kroah-Hartman, Arnd Bergmann, Herbert Xu, grant.likely,
	jean-philippe, Jerome Glisse, ilias.apalodimas, francois.ozog,
	kenneth-lee-2012, Wangzhou, haojian . zhuang, guodong.xu,
	linux-accelerators, linux-kernel, linux-crypto, iommu

Hi, Jonathan

On 2019/11/1 上午1:53, Jonathan Cameron wrote:
> On Tue, 29 Oct 2019 14:40:16 +0800
> Zhangfei Gao <zhangfei.gao@linaro.org> wrote:
>
>> Register qm to uacce framework for user crypto driver
>>
>> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
>> Signed-off-by: Zhou Wang <wangzhou1@hisilicon.com>
> Hi.
>
> This shows there is probably a race during setup that you should close.
> Userspace interface is exposed before the driver is ready to handle it.
>
> Few other bits inline.
>
> Thanks,
>
> Jonathan
>
>> ---
>>   drivers/crypto/hisilicon/qm.c           | 253 ++++++++++++++++++++++++++++++--
>>   drivers/crypto/hisilicon/qm.h           |  13 +-
>>   drivers/crypto/hisilicon/zip/zip_main.c |  39 ++---
>>   include/uapi/misc/uacce/qm.h            |  23 +++
>>   4 files changed, 292 insertions(+), 36 deletions(-)
>>   create mode 100644 include/uapi/misc/uacce/qm.h
>>
>> diff --git a/drivers/crypto/hisilicon/qm.c b/drivers/crypto/hisilicon/qm.c
>> index a8ed6990..4b9cced 100644
>> --- a/drivers/crypto/hisilicon/qm.c
>> +++ b/drivers/crypto/hisilicon/qm.c
>> @@ -9,6 +9,9 @@
>>   #include <linux/log2.h>
>>   #include <linux/seq_file.h>
>>   #include <linux/slab.h>
>> +#include <linux/uacce.h>
>> +#include <linux/uaccess.h>
>> +#include <uapi/misc/uacce/qm.h>
>>   #include "qm.h"
>>   
>>   /* eq/aeq irq enable */
>> @@ -465,17 +468,22 @@ static void qm_cq_head_update(struct hisi_qp *qp)
>>   
>>   static void qm_poll_qp(struct hisi_qp *qp, struct hisi_qm *qm)
>>   {
>> -	struct qm_cqe *cqe = qp->cqe + qp->qp_status.cq_head;
>> -
>> -	if (qp->req_cb) {
>> -		while (QM_CQE_PHASE(cqe) == qp->qp_status.cqc_phase) {
>> -			dma_rmb();
>> -			qp->req_cb(qp, qp->sqe + qm->sqe_size * cqe->sq_head);
>> -			qm_cq_head_update(qp);
>> -			cqe = qp->cqe + qp->qp_status.cq_head;
>> -			qm_db(qm, qp->qp_id, QM_DOORBELL_CMD_CQ,
>> -			      qp->qp_status.cq_head, 0);
>> -			atomic_dec(&qp->qp_status.used);
>> +	struct qm_cqe *cqe;
>> +
>> +	if (qp->event_cb) {
>> +		qp->event_cb(qp);
>> +	} else {
>> +		cqe = qp->cqe + qp->qp_status.cq_head;
>> +
>> +		if (qp->req_cb) {
>> +			while (QM_CQE_PHASE(cqe) == qp->qp_status.cqc_phase) {
>> +				dma_rmb();
>> +				qp->req_cb(qp, qp->sqe + qm->sqe_size *
>> +					   cqe->sq_head);
>> +				qm_cq_head_update(qp);
>> +				cqe = qp->cqe + qp->qp_status.cq_head;
>> +				atomic_dec(&qp->qp_status.used);
>> +			}
>>   		}
>>   
>>   		/* set c_flag */
>> @@ -1397,6 +1405,220 @@ static void hisi_qm_cache_wb(struct hisi_qm *qm)
>>   	}
>>   }
>>   
>> +static void qm_qp_event_notifier(struct hisi_qp *qp)
>> +{
>> +	wake_up_interruptible(&qp->uacce_q->wait);
>> +}
>> +
>> +static int hisi_qm_get_available_instances(struct uacce_device *uacce)
>> +{
>> +	int i, ret;
>> +	struct hisi_qm *qm = uacce->priv;
>> +
>> +	read_lock(&qm->qps_lock);
>> +	for (i = 0, ret = 0; i < qm->qp_num; i++)
>> +		if (!qm->qp_array[i])
>> +			ret++;
>> +	read_unlock(&qm->qps_lock);
>> +
>> +	return ret;
>> +}
>> +
>> +static int hisi_qm_uacce_get_queue(struct uacce_device *uacce,
>> +				   unsigned long arg,
>> +				   struct uacce_queue *q)
>> +{
>> +	struct hisi_qm *qm = uacce->priv;
>> +	struct hisi_qp *qp;
>> +	u8 alg_type = 0;
>> +
>> +	qp = hisi_qm_create_qp(qm, alg_type);
>> +	if (IS_ERR(qp))
>> +		return PTR_ERR(qp);
>> +
>> +	q->priv = qp;
>> +	q->uacce = uacce;
>> +	qp->uacce_q = q;
>> +	qp->event_cb = qm_qp_event_notifier;
>> +	qp->pasid = arg;
>> +
>> +	return 0;
>> +}
>> +
>> +static void hisi_qm_uacce_put_queue(struct uacce_queue *q)
>> +{
>> +	struct hisi_qp *qp = q->priv;
>> +
>> +	/*
>> +	 * As put_queue is only called in uacce_mode=1, and only one queue can
> We got rid of the modes I think so comment needs an update.
Yes
>
>> +	 * be used in this mode. we flush all sqc cache back in put queue.
>> +	 */
>> +	hisi_qm_cache_wb(qp->qm);
>> +
>> +	/* need to stop hardware, but can not support in v1 */
>> +	hisi_qm_release_qp(qp);
> Should we just drop support for the v1 hardware if we can't do this?
>
>> +}
>> +
>> +/* map sq/cq/doorbell to user space */
>> +static int hisi_qm_uacce_mmap(struct uacce_queue *q,
>> +			      struct vm_area_struct *vma,
>> +			      struct uacce_qfile_region *qfr)
>> +{
>> +	struct hisi_qp *qp = q->priv;
>> +	struct hisi_qm *qm = qp->qm;
>> +	size_t sz = vma->vm_end - vma->vm_start;
>> +	struct pci_dev *pdev = qm->pdev;
>> +	struct device *dev = &pdev->dev;
>> +	unsigned long vm_pgoff;
>> +	int ret;
>> +
>> +	switch (qfr->type) {
>> +	case UACCE_QFRT_MMIO:
>> +		if (qm->ver == QM_HW_V2) {
>> +			if (sz > PAGE_SIZE * (QM_DOORBELL_PAGE_NR +
>> +			    QM_DOORBELL_SQ_CQ_BASE_V2 / PAGE_SIZE))
>> +				return -EINVAL;
>> +		} else {
>> +			if (sz > PAGE_SIZE * QM_DOORBELL_PAGE_NR)
>> +				return -EINVAL;
>> +		}
>> +
>> +		vma->vm_flags |= VM_IO;
>> +
>> +		return remap_pfn_range(vma, vma->vm_start,
>> +				       qm->phys_base >> PAGE_SHIFT,
>> +				       sz, pgprot_noncached(vma->vm_page_prot));
>> +	case UACCE_QFRT_DUS:
>> +		if (sz != qp->qdma.size)
>> +			return -EINVAL;
>> +
> Comment style in here is inconsistent. Match the existing code.
OK
>> +		/* dma_mmap_coherent() requires vm_pgoff as 0
>> +		 * restore vm_pfoff to initial value for mmap()
>> +		 */
>> +		vm_pgoff = vma->vm_pgoff;
>> +		vma->vm_pgoff = 0;
>> +		ret = dma_mmap_coherent(dev, vma, qp->qdma.va,
>> +					qp->qdma.dma, sz);
>> +		vma->vm_pgoff = vm_pgoff;
>> +		return ret;
>> +
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +}
>> +
>> +static int hisi_qm_uacce_start_queue(struct uacce_queue *q)
>> +{
>> +	struct hisi_qp *qp = q->priv;
>> +
>> +	return hisi_qm_start_qp(qp, qp->pasid);
>> +}
>> +
>> +static void hisi_qm_uacce_stop_queue(struct uacce_queue *q)
>> +{
>> +	struct hisi_qp *qp = q->priv;
>> +
>> +	hisi_qm_stop_qp(qp);
> I'm a great fan of minimalism on these
> 	hisi_qm_stop_qp(q->priv); doesn't really loose any clarity.
OK
>> +}
>> +
>> +static int qm_set_sqctype(struct uacce_queue *q, u16 type)
>> +{
>> +	struct hisi_qm *qm = q->uacce->priv;
>> +	struct hisi_qp *qp = q->priv;
>> +
>> +	write_lock(&qm->qps_lock);
>> +	qp->alg_type = type;
>> +	write_unlock(&qm->qps_lock);
>> +
>> +	return 0;
>> +}
>> +
>> +static long hisi_qm_uacce_ioctl(struct uacce_queue *q, unsigned int cmd,
>> +				unsigned long arg)
>> +{
>> +	struct hisi_qp *qp = q->priv;
>> +	struct hisi_qp_ctx qp_ctx;
>> +
>> +	if (cmd == UACCE_CMD_QM_SET_QP_CTX) {
>> +		if (copy_from_user(&qp_ctx, (void __user *)arg,
>> +				   sizeof(struct hisi_qp_ctx)))
>> +			return -EFAULT;
>> +
>> +		if (qp_ctx.qc_type != 0 && qp_ctx.qc_type != 1)
>> +			return -EINVAL;
>> +
>> +		qm_set_sqctype(q, qp_ctx.qc_type);
>> +		qp_ctx.id = qp->qp_id;
>> +
>> +		if (copy_to_user((void __user *)arg, &qp_ctx,
>> +				 sizeof(struct hisi_qp_ctx)))
>> +			return -EFAULT;
>> +	} else {
>> +		return -EINVAL;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static struct uacce_ops uacce_qm_ops = {
>> +	.get_available_instances = hisi_qm_get_available_instances,
>> +	.get_queue = hisi_qm_uacce_get_queue,
>> +	.put_queue = hisi_qm_uacce_put_queue,
>> +	.start_queue = hisi_qm_uacce_start_queue,
>> +	.stop_queue = hisi_qm_uacce_stop_queue,
>> +	.mmap = hisi_qm_uacce_mmap,
>> +	.ioctl = hisi_qm_uacce_ioctl,
>> +};
>> +
>> +static int qm_register_uacce(struct hisi_qm *qm)
>> +{
>> +	struct pci_dev *pdev = qm->pdev;
>> +	struct uacce_device *uacce;
>> +	unsigned long mmio_page_nr;
>> +	unsigned long dus_page_nr;
>> +	struct uacce_interface interface = {
>> +		.flags = UACCE_DEV_SVA,
>> +		.ops = &uacce_qm_ops,
>> +	};
>> +
>> +	strncpy(interface.name, pdev->driver->name, sizeof(interface.name));
>> +
>> +	uacce = uacce_register(&pdev->dev, &interface);
>> +	if (IS_ERR(uacce))
>> +		return PTR_ERR(uacce);
> Is there a potential race here as we have exposed the character device before
> the driver is ready for it to be used?  Probably need to split the code that
> allocates a uacce interface from the bit that actually exposes it to userspace.
I don't think it is a race condition.
Since no requirement of get sysfs ready then register character device.
Also sysfs does not always refect constant members, like available_instance.
Currently we set the sysfs members after uacce_register, which alloc 
uacce device.
So no problem if they are ready before character device open.

If we split the code, allocate an uacce interface first then expose to 
usersapce, an additional api maybe required.

>
>> +
>> +	if (uacce->flags & UACCE_DEV_SVA) {
>> +		qm->use_sva = true;
>> +	} else {
>> +		/* only consider sva case */
>> +		uacce_unregister(uacce);
>> +		return -EINVAL;
>> +	}
>> +
>> +	uacce->is_vf = pdev->is_virtfn;
>> +	uacce->priv = qm;
>> +	uacce->algs = qm->algs;
>> +
>> +	if (qm->ver == QM_HW_V1) {
>> +		mmio_page_nr = QM_DOORBELL_PAGE_NR;
>> +		uacce->api_ver = HISI_QM_API_VER_BASE;
>> +	} else {
>> +		mmio_page_nr = QM_DOORBELL_PAGE_NR +
>> +			QM_DOORBELL_SQ_CQ_BASE_V2 / PAGE_SIZE;
>> +		uacce->api_ver = HISI_QM_API_VER2_BASE;
>> +	}
>> +
>> +	dus_page_nr = (PAGE_SIZE - 1 + qm->sqe_size * QM_Q_DEPTH +
>> +		       sizeof(struct qm_cqe) * QM_Q_DEPTH) >> PAGE_SHIFT;
>> +
>> +	uacce->qf_pg_size[UACCE_QFRT_MMIO] = mmio_page_nr;
>> +	uacce->qf_pg_size[UACCE_QFRT_DUS]  = dus_page_nr;
>> +
>> +	qm->uacce = uacce;
>> +
>> +	return 0;
>> +}
>> +
>>   /**
>>    * hisi_qm_init() - Initialize configures about qm.
>>    * @qm: The qm needing init.
>> @@ -1421,6 +1643,10 @@ int hisi_qm_init(struct hisi_qm *qm)
>>   		return -EINVAL;
>>   	}
>>   
>> +	ret = qm_register_uacce(qm);
>> +	if (ret < 0)
>> +		dev_warn(&pdev->dev, "fail to register uacce (%d)\n", ret);
>> +
> looks like there are error paths in qm_init in which we should call
> the uacce_unregister?
OK
>
>>   	ret = pci_enable_device_mem(pdev);
>>   	if (ret < 0) {
>>   		dev_err(&pdev->dev, "Failed to enable device mem!\n");
>> @@ -1433,6 +1659,8 @@ int hisi_qm_init(struct hisi_qm *qm)
>>   		goto err_disable_pcidev;
>>   	}
>>   
>> +	qm->phys_base = pci_resource_start(pdev, PCI_BAR_2);
>> +	qm->size = pci_resource_len(qm->pdev, PCI_BAR_2);
>>   	qm->io_base = ioremap(pci_resource_start(pdev, PCI_BAR_2),
>>   			      pci_resource_len(qm->pdev, PCI_BAR_2));
> Use qm->phys_base/size in the ioremap here to avoid repeating the code.
OK
>
>>   	if (!qm->io_base) {
>> @@ -1504,6 +1732,9 @@ void hisi_qm_uninit(struct hisi_qm *qm)
>>   	iounmap(qm->io_base);
>>   	pci_release_mem_regions(pdev);
>>   	pci_disable_device(pdev);
>> +
>> +	if (qm->uacce)
>> +		uacce_unregister(qm->uacce);
> Can we make uacce_unregister check the input?
> Might make for cleaner users.
OK,
>
>>   }
>>   EXPORT_SYMBOL_GPL(hisi_qm_uninit);
>>   
>> diff --git a/drivers/crypto/hisilicon/qm.h b/drivers/crypto/hisilicon/qm.h
>> index 103e2fd..84a3be9 100644
>> --- a/drivers/crypto/hisilicon/qm.h
>> +++ b/drivers/crypto/hisilicon/qm.h
>> @@ -77,6 +77,10 @@
>>   
>>   #define HISI_ACC_SGL_SGE_NR_MAX		255
>>   
>> +/* page number for queue file region */
>> +#define QM_DOORBELL_PAGE_NR		1
>> +
> 1 blank line only is almost always enough.
>
>> +
>>   enum qp_state {
>>   	QP_STOP,
>>   };
>> @@ -161,7 +165,12 @@ struct hisi_qm {
>>   	u32 error_mask;
>>   	u32 msi_mask;
>>   
>> +	const char *algs;
>>   	bool use_dma_api;
>> +	bool use_sva;
>> +	resource_size_t phys_base;
>> +	resource_size_t size;
>> +	struct uacce_device *uacce;
>>   };
>>   
>>   struct hisi_qp_status {
>> @@ -191,10 +200,12 @@ struct hisi_qp {
>>   	struct hisi_qp_ops *hw_ops;
>>   	void *qp_ctx;
>>   	void (*req_cb)(struct hisi_qp *qp, void *data);
>> +	void (*event_cb)(struct hisi_qp *qp);
>>   	struct work_struct work;
>>   	struct workqueue_struct *wq;
>> -
> unrelated change.
>
>>   	struct hisi_qm *qm;
>> +	u16 pasid;
>> +	struct uacce_queue *uacce_q;
>>   };
>>   
>>   int hisi_qm_init(struct hisi_qm *qm);
>> diff --git a/drivers/crypto/hisilicon/zip/zip_main.c b/drivers/crypto/hisilicon/zip/zip_main.c
>> index 1b2ee96..48860d2 100644
>> --- a/drivers/crypto/hisilicon/zip/zip_main.c
>> +++ b/drivers/crypto/hisilicon/zip/zip_main.c
>> @@ -316,8 +316,14 @@ static void hisi_zip_set_user_domain_and_cache(struct hisi_zip *hisi_zip)
>>   	writel(AXUSER_BASE, base + HZIP_BD_RUSER_32_63);
>>   	writel(AXUSER_BASE, base + HZIP_SGL_RUSER_32_63);
>>   	writel(AXUSER_BASE, base + HZIP_BD_WUSER_32_63);
>> -	writel(AXUSER_BASE, base + HZIP_DATA_RUSER_32_63);
>> -	writel(AXUSER_BASE, base + HZIP_DATA_WUSER_32_63);
>> +
>> +	if (hisi_zip->qm.use_sva) {
>> +		writel(AXUSER_BASE | AXUSER_SSV, base + HZIP_DATA_RUSER_32_63);
>> +		writel(AXUSER_BASE | AXUSER_SSV, base + HZIP_DATA_WUSER_32_63);
>> +	} else {
>> +		writel(AXUSER_BASE, base + HZIP_DATA_RUSER_32_63);
>> +		writel(AXUSER_BASE, base + HZIP_DATA_WUSER_32_63);
>> +	}
>>   
>>   	/* let's open all compression/decompression cores */
>>   	writel(DECOMP_CHECK_ENABLE | ALL_COMP_DECOMP_EN,
>> @@ -671,24 +677,12 @@ static int hisi_zip_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>>   	qm = &hisi_zip->qm;
>>   	qm->pdev = pdev;
>>   	qm->ver = rev_id;
>> -
> Try to avoid noise from white space changes.  No huge help to delete the blank line here.
>
>> +	qm->use_dma_api = true;
>> +	qm->algs = "zlib\ngzip\n";
>>   	qm->sqe_size = HZIP_SQE_SIZE;
>>   	qm->dev_name = hisi_zip_name;
>>   	qm->fun_type = (pdev->device == PCI_DEVICE_ID_ZIP_PF) ? QM_HW_PF :
>>   								QM_HW_VF;
> Unrelated changes I think.  Can we clean out the old left overs
> of uacce from the driver in a precursor patch? Also if it's no longer
> used can we drop the module param?
This patch is an example, so just in one patch to make it work.
>
>> -	switch (uacce_mode) {
>> -	case 0:
>> -		qm->use_dma_api = true;
>> -		break;
>> -	case 1:
>> -		qm->use_dma_api = false;
>> -		break;
>> -	case 2:
>> -		qm->use_dma_api = true;
>> -		break;
>> -	default:
>> -		return -EINVAL;
>> -	}
>>   
>>   	ret = hisi_qm_init(qm);
>>   	if (ret) {
>> @@ -976,12 +970,10 @@ static int __init hisi_zip_init(void)
>>   		goto err_pci;
>>   	}
>>   
>> -	if (uacce_mode == 0 || uacce_mode == 2) {
>> -		ret = hisi_zip_register_to_crypto();
>> -		if (ret < 0) {
>> -			pr_err("Failed to register driver to crypto.\n");
>> -			goto err_crypto;
>> -		}
>> +	ret = hisi_zip_register_to_crypto();
>> +	if (ret < 0) {
>> +		pr_err("Failed to register driver to crypto.\n");
>> +		goto err_crypto;
>>   	}
>>   
>>   	return 0;
>> @@ -996,8 +988,7 @@ static int __init hisi_zip_init(void)
>>   
>>   static void __exit hisi_zip_exit(void)
>>   {
>> -	if (uacce_mode == 0 || uacce_mode == 2)
>> -		hisi_zip_unregister_from_crypto();
>> +	hisi_zip_unregister_from_crypto();
>
>
>>   	pci_unregister_driver(&hisi_zip_pci_driver);
>>   	hisi_zip_unregister_debugfs();
>>   }
>> diff --git a/include/uapi/misc/uacce/qm.h b/include/uapi/misc/uacce/qm.h
>> new file mode 100644
>> index 0000000..d79a8f2
>> --- /dev/null
>> +++ b/include/uapi/misc/uacce/qm.h
> Given generic directory (assuming uacce becomes heavily used) probably
> want to prefix that if it is unique to hisilicon.
>
> hisi_qm.h?
OK, good idea.
>
>> @@ -0,0 +1,23 @@
>> +/* SPDX-License-Identifier: GPL-2.0+ WITH Linux-syscall-note */
>> +#ifndef HISI_QM_USR_IF_H
>> +#define HISI_QM_USR_IF_H
>> +
>> +#include <linux/types.h>
>> +
>> +/**
>> + * struct hisi_qp_ctx - User data for hisi qp.
>> + * @id: Specifies which Turbo decode algorithm to use
> What's a Turbo algorithm?  I don't know and I have the manuals ;)
Sorry, will change that

Thanks


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v7 2/3] uacce: add uacce driver
  2019-10-29  6:40 ` [PATCH v7 2/3] uacce: add uacce driver Zhangfei Gao
  2019-10-31 17:13   ` Jonathan Cameron
@ 2019-11-05 11:48   ` Jean-Philippe Brucker
  2019-11-06  8:17     ` zhangfei
  1 sibling, 1 reply; 16+ messages in thread
From: Jean-Philippe Brucker @ 2019-11-05 11:48 UTC (permalink / raw)
  To: Zhangfei Gao
  Cc: Greg Kroah-Hartman, Arnd Bergmann, Herbert Xu, jonathan.cameron,
	grant.likely, Jerome Glisse, ilias.apalodimas, francois.ozog,
	kenneth-lee-2012, Wangzhou, haojian . zhuang, guodong.xu,
	linux-accelerators, linux-kernel, linux-crypto, iommu,
	Kenneth Lee, Zaibo Xu

Hi Zhangfei,

Thanks for simplifying this, it's a lot easier to review. I have some
additional comments.

On Tue, Oct 29, 2019 at 02:40:15PM +0800, Zhangfei Gao wrote:
> +static int uacce_sva_exit(struct device *dev, struct iommu_sva *handle,
> +			  void *data)
> +{
> +	struct uacce_device *uacce = data;
> +	struct uacce_queue *q;
> +
> +	mutex_lock(&uacce->q_lock);
> +	list_for_each_entry(q, &uacce->qs, list) {
> +		if (q->pid == task_pid_nr(current))
> +			uacce_put_queue(q);

This won't work in some cases, because any thread can call __mmput() and
end up here. For example a sibling thread that inherited the queue, or a
workqueue that's executing mmput_async_fn(). In addition I think comparing
PID values is unsafe (see comment in pid.h), we'd need to use the struct
pid if we wanted to do it this way.

But I still believe it would be better to create an uacce_mm structure
that tracks all queues bound to this mm, and pass that to uacce_sva_exit
instead of the uacce_device.

The queue isn't bound to a task, but its address space. With clone() the
address space can be shared between tasks. In addition, whoever has a
queue fd also gets access to this address space. So after a fork() the
child may be able to program the queue to DMA into the parent's address
space, even without CLONE_VM. Users must be aware of this and I think it's
important to explain it very clearly in the UAPI.

[...]
> +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_device *uacce = q->uacce;
> +	struct uacce_qfile_region *qfr;
> +	int ret = -ENOMEM;
> +
> +	qfr = kzalloc(sizeof(*qfr), GFP_KERNEL);
> +	if (!qfr)
> +		return ERR_PTR(-ENOMEM);
> +
> +	qfr->type = type;
> +	qfr->flags = flags;
> +
> +	if (vma->vm_flags & VM_READ)
> +		qfr->prot |= IOMMU_READ;

qfr->prot and qfr->flags aren't used at the moment, you could remove them.

> +
> +	if (vma->vm_flags & VM_WRITE)
> +		qfr->prot |= IOMMU_WRITE;
> +
> +	if (flags & UACCE_QFRF_SELFMT) {
> +		if (!uacce->ops->mmap) {
> +			ret = -EINVAL;
> +			goto err_with_qfr;
> +		}
> +
> +		ret = uacce->ops->mmap(q, vma, qfr);
> +		if (ret)
> +			goto err_with_qfr;
> +		return qfr;
> +	}
> +
> +	return qfr;
> +
> +err_with_qfr:
> +	kfree(qfr);
> +	return ERR_PTR(ret);
> +}
> +
> +static int uacce_fops_mmap(struct file *filep, struct vm_area_struct *vma)
> +{
> +	struct uacce_queue *q = filep->private_data;
> +	struct uacce_device *uacce = q->uacce;
> +	struct uacce_qfile_region *qfr;
> +	enum uacce_qfrt type = 0;
> +	unsigned int flags = 0;
> +	int ret;
> +
> +	if (vma->vm_pgoff < UACCE_QFRT_MAX)
> +		type = vma->vm_pgoff;

Otherwise return -EINVAL?  type probably shouldn't default to MMIO if it
wasn't explicitly requested by the user.

> +
> +	vma->vm_flags |= VM_DONTCOPY | VM_DONTEXPAND | VM_WIPEONFORK;
> +	vma->vm_ops = &uacce_vm_ops;
> +	vma->vm_private_data = q;
> +
> +	mutex_lock(&uacce_mutex);
> +
> +	if (q->qfrs[type]) {
> +		ret = -EEXIST;
> +		goto out_with_lock;
> +	}
> +
> +	switch (type) {
> +	case UACCE_QFRT_MMIO:
> +		flags = UACCE_QFRF_SELFMT;
> +		break;
> +
> +	case UACCE_QFRT_DUS:
> +		if (uacce->flags & UACCE_DEV_SVA) {
> +			flags = UACCE_QFRF_SELFMT;

I'd simplify this even further by getting rid of the SELFMT flag. It's the
only possibility at the moment.

> +			break;
> +		}
> +		break;
> +
> +	default:
> +		WARN_ON(&uacce->dev);

WARN_ON(uacce->dev). But shouldn't we instead return -EINVAL here?
UACCE_QFRT_MAX is currently 16, so users can easily trigger this WARN by
passing an invalid value.

[...]
> +void uacce_unregister(struct uacce_device *uacce)
> +{
> +	if (!uacce)
> +		return;
> +
> +	mutex_lock(&uacce->q_lock);
> +	if (!list_empty(&uacce->qs)) {
> +		struct uacce_queue *q;
> +
> +		list_for_each_entry(q, &uacce->qs, list) {
> +			uacce_put_queue(q);

The open file descriptor will still exist after this function returns.
Can all fops can be called with a stale queue?

Thanks,
Jean

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v7 2/3] uacce: add uacce driver
  2019-11-05 11:48   ` Jean-Philippe Brucker
@ 2019-11-06  8:17     ` zhangfei
  2019-11-06 15:32       ` Jean-Philippe Brucker
  0 siblings, 1 reply; 16+ messages in thread
From: zhangfei @ 2019-11-06  8:17 UTC (permalink / raw)
  To: Jean-Philippe Brucker
  Cc: Greg Kroah-Hartman, Arnd Bergmann, Herbert Xu, jonathan.cameron,
	grant.likely, Jerome Glisse, ilias.apalodimas, francois.ozog,
	kenneth-lee-2012, Wangzhou, haojian . zhuang, guodong.xu,
	linux-accelerators, linux-kernel, linux-crypto, iommu,
	Kenneth Lee, Zaibo Xu

Hi, Jean

Thanks for the review.

On 2019/11/5 下午7:48, Jean-Philippe Brucker wrote:
> Hi Zhangfei,
>
> Thanks for simplifying this, it's a lot easier to review. I have some
> additional comments.
>
> On Tue, Oct 29, 2019 at 02:40:15PM +0800, Zhangfei Gao wrote:
>> +static int uacce_sva_exit(struct device *dev, struct iommu_sva *handle,
>> +			  void *data)
>> +{
>> +	struct uacce_device *uacce = data;
>> +	struct uacce_queue *q;
>> +
>> +	mutex_lock(&uacce->q_lock);
>> +	list_for_each_entry(q, &uacce->qs, list) {
>> +		if (q->pid == task_pid_nr(current))
>> +			uacce_put_queue(q);
> This won't work in some cases, because any thread can call __mmput() and
> end up here. For example a sibling thread that inherited the queue, or a
> workqueue that's executing mmput_async_fn(). In addition I think comparing
> PID values is unsafe (see comment in pid.h), we'd need to use the struct
> pid if we wanted to do it this way.
OK, still in check.
>
> But I still believe it would be better to create an uacce_mm structure
> that tracks all queues bound to this mm, and pass that to uacce_sva_exit
> instead of the uacce_device.
I am afraid this method may not work.
Since currently iommu_sva_bind_device only accept the same drvdata for 
the same dev,
that's the reason we can not directly use "queue" as drvdata.
Each time create an uacce_mm structure should be same problem as queue, 
and fail for same dev.
So we use uacce and pick up the right queue inside.

>
> The queue isn't bound to a task, but its address space. With clone() the
> address space can be shared between tasks. In addition, whoever has a
> queue fd also gets access to this address space. So after a fork() the
> child may be able to program the queue to DMA into the parent's address
> space, even without CLONE_VM. Users must be aware of this and I think it's
> important to explain it very clearly in the UAPI.
>
> [...]
>> +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_device *uacce = q->uacce;
>> +	struct uacce_qfile_region *qfr;
>> +	int ret = -ENOMEM;
>> +
>> +	qfr = kzalloc(sizeof(*qfr), GFP_KERNEL);
>> +	if (!qfr)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	qfr->type = type;
>> +	qfr->flags = flags;
>> +
>> +	if (vma->vm_flags & VM_READ)
>> +		qfr->prot |= IOMMU_READ;
> qfr->prot and qfr->flags aren't used at the moment, you could remove them.
Yes,
>
>> +
>> +	if (vma->vm_flags & VM_WRITE)
>> +		qfr->prot |= IOMMU_WRITE;
>> +
>> +	if (flags & UACCE_QFRF_SELFMT) {
>> +		if (!uacce->ops->mmap) {
>> +			ret = -EINVAL;
>> +			goto err_with_qfr;
>> +		}
>> +
>> +		ret = uacce->ops->mmap(q, vma, qfr);
>> +		if (ret)
>> +			goto err_with_qfr;
>> +		return qfr;
>> +	}
>> +
>> +	return qfr;
>> +
>> +err_with_qfr:
>> +	kfree(qfr);
>> +	return ERR_PTR(ret);
>> +}
>> +
>> +static int uacce_fops_mmap(struct file *filep, struct vm_area_struct *vma)
>> +{
>> +	struct uacce_queue *q = filep->private_data;
>> +	struct uacce_device *uacce = q->uacce;
>> +	struct uacce_qfile_region *qfr;
>> +	enum uacce_qfrt type = 0;
>> +	unsigned int flags = 0;
>> +	int ret;
>> +
>> +	if (vma->vm_pgoff < UACCE_QFRT_MAX)
>> +		type = vma->vm_pgoff;
> Otherwise return -EINVAL?  type probably shouldn't default to MMIO if it
> wasn't explicitly requested by the user.
OK
>
>> +
>> +	vma->vm_flags |= VM_DONTCOPY | VM_DONTEXPAND | VM_WIPEONFORK;
>> +	vma->vm_ops = &uacce_vm_ops;
>> +	vma->vm_private_data = q;
>> +
>> +	mutex_lock(&uacce_mutex);
>> +
>> +	if (q->qfrs[type]) {
>> +		ret = -EEXIST;
>> +		goto out_with_lock;
>> +	}
>> +
>> +	switch (type) {
>> +	case UACCE_QFRT_MMIO:
>> +		flags = UACCE_QFRF_SELFMT;
>> +		break;
>> +
>> +	case UACCE_QFRT_DUS:
>> +		if (uacce->flags & UACCE_DEV_SVA) {
>> +			flags = UACCE_QFRF_SELFMT;
> I'd simplify this even further by getting rid of the SELFMT flag. It's the
> only possibility at the moment.
OK, we can remove this flag for simplicity, may add it back if required 
in future patch.
>
>> +			break;
>> +		}
>> +		break;
>> +
>> +	default:
>> +		WARN_ON(&uacce->dev);
> WARN_ON(uacce->dev). But shouldn't we instead return -EINVAL here?
> UACCE_QFRT_MAX is currently 16, so users can easily trigger this WARN by
> passing an invalid value.
Yes, good idea.
>
> [...]
>> +void uacce_unregister(struct uacce_device *uacce)
>> +{
>> +	if (!uacce)
>> +		return;
>> +
>> +	mutex_lock(&uacce->q_lock);
>> +	if (!list_empty(&uacce->qs)) {
>> +		struct uacce_queue *q;
>> +
>> +		list_for_each_entry(q, &uacce->qs, list) {
>> +			uacce_put_queue(q);
> The open file descriptor will still exist after this function returns.
> Can all fops can be called with a stale queue?
To more clear:.
Do you mean rmmod without fops_release.

Thanks

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v7 2/3] uacce: add uacce driver
  2019-11-06  8:17     ` zhangfei
@ 2019-11-06 15:32       ` Jean-Philippe Brucker
  2019-11-07 13:23         ` zhangfei
  0 siblings, 1 reply; 16+ messages in thread
From: Jean-Philippe Brucker @ 2019-11-06 15:32 UTC (permalink / raw)
  To: zhangfei
  Cc: Greg Kroah-Hartman, Arnd Bergmann, Herbert Xu, jonathan.cameron,
	grant.likely, Jerome Glisse, ilias.apalodimas, francois.ozog,
	kenneth-lee-2012, Wangzhou, haojian . zhuang, guodong.xu,
	linux-accelerators, linux-kernel, linux-crypto, iommu,
	Kenneth Lee, Zaibo Xu

[-- Attachment #1: Type: text/plain, Size: 2630 bytes --]

On Wed, Nov 06, 2019 at 04:17:40PM +0800, zhangfei wrote:
> > But I still believe it would be better to create an uacce_mm structure
> > that tracks all queues bound to this mm, and pass that to uacce_sva_exit
> > instead of the uacce_device.
> I am afraid this method may not work.
> Since currently iommu_sva_bind_device only accept the same drvdata for the
> same dev,
> that's the reason we can not directly use "queue" as drvdata.
> Each time create an uacce_mm structure should be same problem as queue, and
> fail for same dev.
> So we use uacce and pick up the right queue inside.

What I had in mind is keep one uacce_mm per mm and per device, and we can
pass that to iommu_sva_bind_device(). It requires some structure changes,
see the attached patch.

> > The queue isn't bound to a task, but its address space. With clone() the
> > address space can be shared between tasks. In addition, whoever has a
> > queue fd also gets access to this address space. So after a fork() the
> > child may be able to program the queue to DMA into the parent's address
> > space, even without CLONE_VM. Users must be aware of this and I think it's
> > important to explain it very clearly in the UAPI.
> > [...]
> > > +void uacce_unregister(struct uacce_device *uacce)
> > > +{
> > > +	if (!uacce)
> > > +		return;
> > > +
> > > +	mutex_lock(&uacce->q_lock);
> > > +	if (!list_empty(&uacce->qs)) {
> > > +		struct uacce_queue *q;
> > > +
> > > +		list_for_each_entry(q, &uacce->qs, list) {
> > > +			uacce_put_queue(q);
> > The open file descriptor will still exist after this function returns.
> > Can all fops can be called with a stale queue?
> To more clear:.
> Do you mean rmmod without fops_release.

Yes I think so. What happens when userspace starts some queues, and
the device driver suddenly calls uacce_unregister(). We call
cdev_device_del() later in this function, but quoting the documentation:
"any cdevs already open will remain and their fops will still be callable
even after this function returns." So we need to make sure that any of the
fops is safe to run after the uacce device disappears.

I noticed a lock dependency inversion on uacce->q_lock: uacce_unregister()
calls iommu_sva_unbind_device() while holding the uacce->q_lock, but
uacce_sva_exit() takes the uacce->q_lock with the SVA lock held. In theory
we could simply avoid calling iommu_sva_unbind_device() here since it will
be done by fops_release(), but then disabling the SVA feature in
uacce_unregister() won't work (because there still are bonds). The
attached patch should fix it, but I haven't tried running uacce_register()
yet.

Thanks,
Jean

[-- Attachment #2: 0001-uacce-Track-mm-queue-bonds.patch --]
[-- Type: text/plain, Size: 10187 bytes --]

From 49559efc5cb26aadbcf580de03afd6e4ff67cedc Mon Sep 17 00:00:00 2001
From: Jean-Philippe Brucker <jean-philippe@linaro.org>
Date: Wed, 6 Nov 2019 10:10:07 +0000
Subject: [PATCH] uacce: Track mm<->queue bonds

The IOMMU core only tracks mm<->device bonds at the moment, because it
only needs to handle IOTLB invalidation and PASID table entries. However
uacce needs a finer granularity since multiple queues from the same
device can be bound to an mm. When the mm exits, all bound queues must
be stopped so that the IOMMU can safely clear the PASID table entry and
reallocate the PASID.

Introduce an intermediate struct uacce_mm that links uacce devices and
queues. Note that an mm may be bound to multiple devices but an uacce_mm
structure only ever belongs to a single device, because we don't need
anything more complex (if multiple devices are bound to one mm, then
we'll create one uacce_mm for each bond).

        uacce_device --+-- uacce_mm --+-- uacce_queue
                       |              '-- uacce_queue
                       |
                       '-- uacce_mm --+-- uacce_queue
                                      +-- uacce_queue
                                      '-- uacce_queue

If multiple device drivers need this model, it should be possible to
move it to iommu-sva in the future, with some changes to the API, and
have mm_exit() be called for multiple contexts per iommu_bond.

Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
---
 drivers/misc/uacce/uacce.c | 174 +++++++++++++++++++++++++++----------
 include/linux/uacce.h      |  34 ++++++--
 2 files changed, 152 insertions(+), 56 deletions(-)

diff --git a/drivers/misc/uacce/uacce.c b/drivers/misc/uacce/uacce.c
index 2b6b03855ac6..d8a7fbfe7399 100644
--- a/drivers/misc/uacce/uacce.c
+++ b/drivers/misc/uacce/uacce.c
@@ -92,15 +92,19 @@ static long uacce_fops_compat_ioctl(struct file *filep,
 static int uacce_sva_exit(struct device *dev, struct iommu_sva *handle,
 			  void *data)
 {
-	struct uacce_device *uacce = data;
+	struct uacce_mm *uacce_mm = data;
 	struct uacce_queue *q;
 
-	mutex_lock(&uacce->q_lock);
-	list_for_each_entry(q, &uacce->qs, list) {
-		if (q->pid == task_pid_nr(current))
-			uacce_put_queue(q);
-	}
-	mutex_unlock(&uacce->q_lock);
+	/*
+	 * No new queue can be added concurrently because no caller can have a
+	 * reference to this mm. But there may be concurrent calls to
+	 * uacce_mm_put(), so we need the lock.
+	 */
+	mutex_lock(&uacce_mm->lock);
+	list_for_each_entry(q, &uacce_mm->queues, list)
+		uacce_put_queue(q);
+	uacce_mm->mm = NULL;
+	mutex_unlock(&uacce_mm->lock);
 
 	return 0;
 }
@@ -109,13 +113,88 @@ static struct iommu_sva_ops uacce_sva_ops = {
 	.mm_exit = uacce_sva_exit,
 };
 
-static int uacce_fops_open(struct inode *inode, struct file *filep)
+static struct uacce_mm *uacce_mm_get(struct uacce_device *uacce,
+				     struct uacce_queue *q,
+				     struct mm_struct *mm)
 {
+	struct uacce_mm *uacce_mm = NULL;
 	struct iommu_sva *handle = NULL;
+	int ret;
+
+	lockdep_assert_held(&uacce->mm_lock);
+
+	list_for_each_entry(uacce_mm, &uacce->mm_list, list) {
+		if (uacce_mm->mm == mm) {
+			mutex_lock(&uacce_mm->lock);
+			list_add(&q->list, &uacce_mm->queues);
+			mutex_unlock(&uacce_mm->lock);
+			return uacce_mm;
+		}
+	}
+
+	uacce_mm = kzalloc(sizeof(*uacce_mm), GFP_KERNEL);
+	if (!uacce_mm)
+		return NULL;
+
+	if (uacce->flags & UACCE_DEV_SVA) {
+		/*
+		 * Safe to pass an incomplete uacce_mm, since mm_exit cannot
+		 * fire while we hold a reference to the mm.
+		 */
+		handle = iommu_sva_bind_device(uacce->pdev, mm, uacce_mm);
+		if (IS_ERR(handle))
+			goto err_free;
+
+		ret = iommu_sva_set_ops(handle, &uacce_sva_ops);
+		if (ret)
+			goto err_unbind;
+
+		uacce_mm->pasid = iommu_sva_get_pasid(handle);
+		if (uacce_mm->pasid == IOMMU_PASID_INVALID)
+			goto err_unbind;
+	}
+
+	uacce_mm->mm = mm;
+	uacce_mm->handle = handle;
+	INIT_LIST_HEAD(&uacce_mm->queues);
+	mutex_init(&uacce_mm->lock);
+	list_add(&q->list, &uacce_mm->queues);
+	list_add(&uacce_mm->list, &uacce->mm_list);
+
+	return uacce_mm;
+
+err_unbind:
+	if (handle)
+		iommu_sva_unbind_device(handle);
+err_free:
+	kfree(uacce_mm);
+	return NULL;
+}
+
+static void uacce_mm_put(struct uacce_queue *q)
+{
+	struct uacce_mm *uacce_mm = q->uacce_mm;
+
+	lockdep_assert_held(&q->uacce->mm_lock);
+
+	mutex_lock(&uacce_mm->lock);
+	list_del(&q->list);
+	mutex_unlock(&uacce_mm->lock);
+
+	if (list_empty(&uacce_mm->queues)) {
+		if (uacce_mm->handle)
+			iommu_sva_unbind_device(uacce_mm->handle);
+		list_del(&uacce_mm->list);
+		kfree(uacce_mm);
+	}
+}
+
+static int uacce_fops_open(struct inode *inode, struct file *filep)
+{
+	struct uacce_mm *uacce_mm = NULL;
 	struct uacce_device *uacce;
 	struct uacce_queue *q;
 	int ret = 0;
-	int pasid = 0;
 
 	uacce = xa_load(&uacce_xa, iminor(inode));
 	if (!uacce)
@@ -130,44 +209,37 @@ static int uacce_fops_open(struct inode *inode, struct file *filep)
 		goto out_with_module;
 	}
 
-	if (uacce->flags & UACCE_DEV_SVA) {
-		handle = iommu_sva_bind_device(uacce->pdev, current->mm, uacce);
-		if (IS_ERR(handle))
-			goto out_with_mem;
-
-		ret = iommu_sva_set_ops(handle, &uacce_sva_ops);
-		if (ret)
-			goto out_unbind;
+	q->state = UACCE_Q_ZOMBIE;
 
-		pasid = iommu_sva_get_pasid(handle);
-		if (pasid == IOMMU_PASID_INVALID)
-			goto out_unbind;
+	mutex_lock(&uacce->mm_lock);
+	uacce_mm = uacce_mm_get(uacce, q, current->mm);
+	mutex_unlock(&uacce->mm_lock);
+	if (!uacce_mm) {
+		ret = -ENOMEM;
+		goto out_with_mem;
 	}
 
+	q->uacce = uacce;
+	q->uacce_mm = uacce_mm;
+
 	if (uacce->ops->get_queue) {
-		ret = uacce->ops->get_queue(uacce, pasid, q);
+		ret = uacce->ops->get_queue(uacce, uacce_mm->pasid, q);
 		if (ret < 0)
-			goto out_unbind;
+			goto out_with_mm;
 	}
 
 	q->pid = task_pid_nr(current);
-	q->pasid = pasid;
-	q->handle = handle;
-	q->uacce = uacce;
 	memset(q->qfrs, 0, sizeof(q->qfrs));
 	init_waitqueue_head(&q->wait);
 	filep->private_data = q;
 	q->state = UACCE_Q_INIT;
 
-	mutex_lock(&uacce->q_lock);
-	list_add(&q->list, &uacce->qs);
-	mutex_unlock(&uacce->q_lock);
-
 	return 0;
 
-out_unbind:
-	if (uacce->flags & UACCE_DEV_SVA)
-		iommu_sva_unbind_device(handle);
+out_with_mm:
+	mutex_lock(&uacce->mm_lock);
+	uacce_mm_put(q);
+	mutex_unlock(&uacce->mm_lock);
 out_with_mem:
 	kfree(q);
 out_with_module:
@@ -182,12 +254,10 @@ static int uacce_fops_release(struct inode *inode, struct file *filep)
 
 	uacce_put_queue(q);
 
-	if (uacce->flags & UACCE_DEV_SVA)
-		iommu_sva_unbind_device(q->handle);
+	mutex_lock(&uacce->mm_lock);
+	uacce_mm_put(q);
+	mutex_unlock(&uacce->mm_lock);
 
-	mutex_lock(&uacce->q_lock);
-	list_del(&q->list);
-	mutex_unlock(&uacce->q_lock);
 	kfree(q);
 	module_put(uacce->pdev->driver->owner);
 
@@ -484,8 +554,8 @@ struct uacce_device *uacce_register(struct device *parent,
 		goto err_with_xa;
 	}
 
-	INIT_LIST_HEAD(&uacce->qs);
-	mutex_init(&uacce->q_lock);
+	INIT_LIST_HEAD(&uacce->mm_list);
+	mutex_init(&uacce->mm_lock);
 	uacce->cdev->ops = &uacce_fops;
 	uacce->cdev->owner = THIS_MODULE;
 	device_initialize(&uacce->dev);
@@ -519,20 +589,30 @@ EXPORT_SYMBOL_GPL(uacce_register);
  */
 void uacce_unregister(struct uacce_device *uacce)
 {
+	struct uacce_mm *uacce_mm;
+	struct uacce_queue *q;
+
 	if (!uacce)
 		return;
 
-	mutex_lock(&uacce->q_lock);
-	if (!list_empty(&uacce->qs)) {
-		struct uacce_queue *q;
-
-		list_for_each_entry(q, &uacce->qs, list) {
+	mutex_lock(&uacce->mm_lock);
+	list_for_each_entry(uacce_mm, &uacce->mm_list, list) {
+		/*
+		 * We don't take the uacce_mm->lock here. Since we hold the
+		 * device's mm_lock, no queue can be added to or removed from
+		 * this uacce_mm. We may run concurrently with mm_exit, but
+		 * uacce_put_queue() is serialized and iommu_sva_unbind_device()
+		 * waits for the lock that mm_exit is holding.
+		 */
+		list_for_each_entry(q, &uacce_mm->queues, list)
 			uacce_put_queue(q);
-			if (uacce->flags & UACCE_DEV_SVA)
-				iommu_sva_unbind_device(q->handle);
+
+		if (uacce->flags & UACCE_DEV_SVA) {
+			iommu_sva_unbind_device(uacce_mm->handle);
+			uacce_mm->handle = NULL;
 		}
 	}
-	mutex_unlock(&uacce->q_lock);
+	mutex_unlock(&uacce->mm_lock);
 
 	if (uacce->flags & UACCE_DEV_SVA)
 		iommu_dev_disable_feature(uacce->pdev, IOMMU_DEV_FEAT_SVA);
diff --git a/include/linux/uacce.h b/include/linux/uacce.h
index 04c8643c130b..8564e078287a 100644
--- a/include/linux/uacce.h
+++ b/include/linux/uacce.h
@@ -88,10 +88,9 @@ enum uacce_q_state {
  * @uacce: pointer to uacce
  * @priv: private pointer
  * @wait: wait queue head
- * @pasid: pasid of the queue
  * @pid: pid of the process using the queue
- * @handle: iommu_sva handle return from iommu_sva_bind_device
- * @list: queue list
+ * @list: index into uacce_mm
+ * @uacce_mm: the corresponding mm
  * @qfrs: pointer of qfr regions
  * @state: queue state machine
  */
@@ -99,10 +98,9 @@ struct uacce_queue {
 	struct uacce_device *uacce;
 	void *priv;
 	wait_queue_head_t wait;
-	int pasid;
 	pid_t pid;
-	struct iommu_sva *handle;
 	struct list_head list;
+	struct uacce_mm *uacce_mm;
 	struct uacce_qfile_region *qfrs[UACCE_QFRT_MAX];
 	enum uacce_q_state state;
 };
@@ -121,8 +119,8 @@ struct uacce_queue {
  * @cdev: cdev of the uacce
  * @dev: dev of the uacce
  * @priv: private pointer of the uacce
- * @qs: list head of queue->list
- * @q_lock: lock for qs
+ * @mm_list: list head of uacce_mm->list
+ * @mm_lock: lock for mm_list
  */
 struct uacce_device {
 	const char *algs;
@@ -137,8 +135,26 @@ struct uacce_device {
 	struct cdev *cdev;
 	struct device dev;
 	void *priv;
-	struct list_head qs;
-	struct mutex q_lock;
+	struct list_head mm_list;
+	struct mutex mm_lock;
+};
+
+/*
+ * struct uacce_mm - keep track of queues bound to a process
+ * @list: index into uacce_device
+ * @queues: list of queues
+ * @mm: the mm struct
+ * @lock: protects the list of queues
+ * @pasid: pasid of the queue
+ * @handle: iommu_sva handle return from iommu_sva_bind_device
+ */
+struct uacce_mm {
+	struct list_head list;
+	struct list_head queues;
+	struct mm_struct *mm;
+	struct mutex lock;
+	int pasid;
+	struct iommu_sva *handle;
 };
 
 #if IS_ENABLED(CONFIG_UACCE)
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH v7 2/3] uacce: add uacce driver
  2019-11-06 15:32       ` Jean-Philippe Brucker
@ 2019-11-07 13:23         ` zhangfei
  2019-11-08  7:48           ` Jean-Philippe Brucker
  0 siblings, 1 reply; 16+ messages in thread
From: zhangfei @ 2019-11-07 13:23 UTC (permalink / raw)
  To: Jean-Philippe Brucker
  Cc: Greg Kroah-Hartman, Arnd Bergmann, Herbert Xu, jonathan.cameron,
	grant.likely, Jerome Glisse, ilias.apalodimas, francois.ozog,
	kenneth-lee-2012, Wangzhou, haojian . zhuang, guodong.xu,
	linux-accelerators, linux-kernel, linux-crypto, iommu,
	Kenneth Lee, Zaibo Xu



On 2019/11/6 下午11:32, Jean-Philippe Brucker wrote:
> On Wed, Nov 06, 2019 at 04:17:40PM +0800, zhangfei wrote:
>>> But I still believe it would be better to create an uacce_mm structure
>>> that tracks all queues bound to this mm, and pass that to uacce_sva_exit
>>> instead of the uacce_device.
>> I am afraid this method may not work.
>> Since currently iommu_sva_bind_device only accept the same drvdata for the
>> same dev,
>> that's the reason we can not directly use "queue" as drvdata.
>> Each time create an uacce_mm structure should be same problem as queue, and
>> fail for same dev.
>> So we use uacce and pick up the right queue inside.
> What I had in mind is keep one uacce_mm per mm and per device, and we can
> pass that to iommu_sva_bind_device(). It requires some structure changes,
> see the attached patch.
Cool, thanks Jean
How about merge them together.
>
>>> The queue isn't bound to a task, but its address space. With clone() the
>>> address space can be shared between tasks. In addition, whoever has a
>>> queue fd also gets access to this address space. So after a fork() the
>>> child may be able to program the queue to DMA into the parent's address
>>> space, even without CLONE_VM. Users must be aware of this and I think it's
>>> important to explain it very clearly in the UAPI.
>>> [...]
>>>> +void uacce_unregister(struct uacce_device *uacce)
>>>> +{
>>>> +	if (!uacce)
>>>> +		return;
>>>> +
>>>> +	mutex_lock(&uacce->q_lock);
>>>> +	if (!list_empty(&uacce->qs)) {
>>>> +		struct uacce_queue *q;
>>>> +
>>>> +		list_for_each_entry(q, &uacce->qs, list) {
>>>> +			uacce_put_queue(q);
>>> The open file descriptor will still exist after this function returns.
>>> Can all fops can be called with a stale queue?
>> To more clear:.
>> Do you mean rmmod without fops_release.
> Yes I think so. What happens when userspace starts some queues, and
> the device driver suddenly calls uacce_unregister(). We call
> cdev_device_del() later in this function, but quoting the documentation:
> "any cdevs already open will remain and their fops will still be callable
> even after this function returns." So we need to make sure that any of the
> fops is safe to run after the uacce device disappears.
We can protect stale queue via q->state, since q is released later in 
fops_release.
And uacce_unregister: put_queue will set q->state = UACCE_Q_ZOMBIE.
Will add state check in mmap too.
>
> I noticed a lock dependency inversion on uacce->q_lock: uacce_unregister()
> calls iommu_sva_unbind_device() while holding the uacce->q_lock, but
> uacce_sva_exit() takes the uacce->q_lock with the SVA lock held. In theory
> we could simply avoid calling iommu_sva_unbind_device() here since it will
> be done by fops_release(), but then disabling the SVA feature in
> uacce_unregister() won't work (because there still are bonds). The
> attached patch should fix it, but I haven't tried running uacce_register()
> yet.
Have tested, it is OK.

Thanks


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v7 2/3] uacce: add uacce driver
  2019-11-07 13:23         ` zhangfei
@ 2019-11-08  7:48           ` Jean-Philippe Brucker
  0 siblings, 0 replies; 16+ messages in thread
From: Jean-Philippe Brucker @ 2019-11-08  7:48 UTC (permalink / raw)
  To: zhangfei
  Cc: Greg Kroah-Hartman, Arnd Bergmann, Herbert Xu, jonathan.cameron,
	grant.likely, Jerome Glisse, ilias.apalodimas, francois.ozog,
	kenneth-lee-2012, Wangzhou, haojian . zhuang, guodong.xu,
	linux-accelerators, linux-kernel, linux-crypto, iommu,
	Kenneth Lee, Zaibo Xu

On Thu, Nov 07, 2019 at 09:23:50PM +0800, zhangfei wrote:
> > What I had in mind is keep one uacce_mm per mm and per device, and we can
> > pass that to iommu_sva_bind_device(). It requires some structure changes,
> > see the attached patch.
> Cool, thanks Jean
> How about merge them together.

No problem, you can squash it into this patch

Thanks,
Jean

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v7 2/3] uacce: add uacce driver
  2019-11-05  7:43     ` zhangfei
@ 2019-11-11 11:19       ` Jonathan Cameron
  0 siblings, 0 replies; 16+ messages in thread
From: Jonathan Cameron @ 2019-11-11 11:19 UTC (permalink / raw)
  To: zhangfei
  Cc: Greg Kroah-Hartman, Arnd Bergmann, Herbert Xu, grant.likely,
	jean-philippe, Jerome Glisse, ilias.apalodimas, francois.ozog,
	kenneth-lee-2012, Wangzhou, haojian . zhuang, guodong.xu,
	linux-accelerators, linux-kernel, linux-crypto, iommu,
	Kenneth Lee, Zaibo Xu

On Tue, 5 Nov 2019 15:43:31 +0800
zhangfei <zhangfei.gao@linaro.org> wrote:

> Hi, Jonathan
> 
> Thanks for the suggestions
> 
> On 2019/11/1 上午1:13, Jonathan Cameron wrote:
> > On Tue, 29 Oct 2019 14:40:15 +0800
> > Zhangfei Gao <zhangfei.gao@linaro.org> wrote:
> >  
> >> From: Kenneth Lee <liguozhu@hisilicon.com>
> >>
> >> Uacce (Unified/User-space-access-intended Accelerator Framework) targets to
> >> provide Shared Virtual Addressing (SVA) between accelerators and processes.
> >> So accelerator can access any data structure of the main cpu.
> >> This differs from the data sharing between cpu and io device, which share
> >> data content rather than address.
> >> Since unified address, hardware and user space of process can share the
> >> same virtual address in the communication.
> >>
> >> Uacce create a chrdev for every registration, the queue is allocated to
> >> the process when the chrdev is opened. Then the process can access the
> >> hardware resource by interact with the queue file. By mmap the queue
> >> file space to user space, the process can directly put requests to the
> >> hardware without syscall to the kernel space.
> >>
> >> Signed-off-by: Kenneth Lee <liguozhu@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>  
> > Great, much more compact.
> >
> > I've not gone through this in detail yet but a few initial comments inline.
> >
> > Thanks,
> >
> > Jonathan
> >  
> >> ---
> >>   Documentation/ABI/testing/sysfs-driver-uacce |  53 +++
> >>   drivers/misc/Kconfig                         |   1 +
> >>   drivers/misc/Makefile                        |   1 +
> >>   drivers/misc/uacce/Kconfig                   |  13 +
> >>   drivers/misc/uacce/Makefile                  |   2 +
> >>   drivers/misc/uacce/uacce.c                   | 574 +++++++++++++++++++++++++++
> >>   include/linux/uacce.h                        | 163 ++++++++
> >>   include/uapi/misc/uacce/uacce.h              |  38 ++
> >>   8 files changed, 845 insertions(+)
> >>   create mode 100644 Documentation/ABI/testing/sysfs-driver-uacce
> >>   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/uacce.h
> >>
> >> diff --git a/Documentation/ABI/testing/sysfs-driver-uacce b/Documentation/ABI/testing/sysfs-driver-uacce
> >> new file mode 100644
> >> index 0000000..35699dc
> >> --- /dev/null
> >> +++ b/Documentation/ABI/testing/sysfs-driver-uacce
> >> @@ -0,0 +1,53 @@
> >> +What:           /sys/class/uacce/<dev_name>/id
> >> +Date:           Oct 2019
> >> +KernelVersion:  5.5
> >> +Contact:        linux-accelerators@lists.ozlabs.org
> >> +Description:    Id of the device.
> >> +
> >> +What:           /sys/class/uacce/<dev_name>/api
> >> +Date:           Oct 2019
> >> +KernelVersion:  5.5
> >> +Contact:        linux-accelerators@lists.ozlabs.org
> >> +Description:    Api of the device, used by application to match the correct driver
> >> +
> >> +What:           /sys/class/uacce/<dev_name>/flags
> >> +Date:           Oct 2019
> >> +KernelVersion:  5.5
> >> +Contact:        linux-accelerators@lists.ozlabs.org
> >> +Description:    Attributes of the device, see UACCE_DEV_xxx flag defined in uacce.h
> >> +
> >> +What:           /sys/class/uacce/<dev_name>/available_instances
> >> +Date:           Oct 2019
> >> +KernelVersion:  5.5
> >> +Contact:        linux-accelerators@lists.ozlabs.org
> >> +Description:    Available instances left of the device
> >> +
> >> +What:           /sys/class/uacce/<dev_name>/algorithms
> >> +Date:           Oct 2019
> >> +KernelVersion:  5.5
> >> +Contact:        linux-accelerators@lists.ozlabs.org
> >> +Description:    Algorithms supported by this accelerator  
> > How are they separated?  Userspace code needs to know that.
> > (comma, tab, newline?)  
> Yes, will add "separated by new line"
> >  
> >> +
> >> +What:           /sys/class/uacce/<dev_name>/qfrt_mmio_size  
> > qfrt is not the most obvious naming ever.  Do we care beyond its
> > a region for this interface?  region_mmio_size maybe?  
> OK,
> >  
> >> +Date:           Oct 2019
> >> +KernelVersion:  5.5
> >> +Contact:        linux-accelerators@lists.ozlabs.org
> >> +Description:    Page size of mmio region queue file  
> > Size of page in this region, or number of pages in the region?  
> Change to "Page numbers of mmio region queue file"

Number of pages used by queue in mmio region?

> >  
> >> +
> >> +What:           /sys/class/uacce/<dev_name>/qfrt_dus_size
> >> +Date:           Oct 2019
> >> +KernelVersion:  5.5
> >> +Contact:        linux-accelerators@lists.ozlabs.org
> >> +Description:    Page size of dus region queue file
> >> +
> >> +What:           /sys/class/uacce/<dev_name>/numa_distance
> >> +Date:           Oct 2019
> >> +KernelVersion:  5.5
> >> +Contact:        linux-accelerators@lists.ozlabs.org
> >> +Description:    Distance of device node to cpu node  
> > I wonder if we should be doing this in here. There are other standard
> > ways of obtaining this for the device.  Follow parent and check node_id
> > there then use the /sys/bus/node path to find out the distances.  
> Could you clarify more about this method.
> The purpose here is cpu searching the nearest device(zip) doing work.
> Does user application know which node it is running and compare distance?

Exactly.  The parent device will typically be a pci device. The parent
link will point somewhere like

/sys/bus/pci/devices/000:00:10.0/

Under that directory is a numa_node file which will give you which node
the device is assigned to.  

Using that number (N) read

/sys/bus/node/devices/nodeN/distance

Which should be the same as what you have from this interface.
It also provides access to info on latency and bandwidth etc
if HMAT is provided - so more info to make a decision than your
new interface here provides.


> >> +
> >> +What:           /sys/class/uacce/<dev_name>/node_id
> >> +Date:           Oct 2019
> >> +KernelVersion:  5.5
> >> +Contact:        linux-accelerators@lists.ozlabs.org
> >> +Description:    Id of the numa node
> >> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> >> index c55b637..929feb0 100644
> >> --- a/drivers/misc/Kconfig
> >> +++ b/drivers/misc/Kconfig
> >> @@ -481,4 +481,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 c1860d3..9abf292 100644
> >> --- a/drivers/misc/Makefile
> >> +++ b/drivers/misc/Makefile
> >> @@ -56,4 +56,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..5e39b60
> >> --- /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/uacce.h
> >> +
> >> +	  See Documentation/misc-devices/uacce.rst for more details.
> >> +
> >> +	  If you don't know what to do here, say N.  
> > Pessimist :) Everyone should want uacce so don't put them off.  Having said
> > that perhaps for now it should be hidden and enabled on a driver by driver
> > basis?
> >  
> >> 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..2b6b038
> >> --- /dev/null
> >> +++ b/drivers/misc/uacce/uacce.c
> >> @@ -0,0 +1,574 @@
> >> +// SPDX-License-Identifier: GPL-2.0-or-later
> >> +#include <linux/compat.h>
> >> +#include <linux/dma-iommu.h>
> >> +#include <linux/module.h>
> >> +#include <linux/poll.h>
> >> +#include <linux/uacce.h>
> >> +
> >> +static struct class *uacce_class;
> >> +static dev_t uacce_devt;
> >> +static DEFINE_MUTEX(uacce_mutex);
> >> +static DEFINE_XARRAY_ALLOC(uacce_xa);
> >> +
> >> +static int uacce_start_queue(struct uacce_queue *q)
> >> +{
> >> +	int ret = -EINVAL;
> >> +
> >> +	mutex_lock(&uacce_mutex);
> >> +
> >> +	if (q->state != UACCE_Q_INIT)
> >> +		goto out_with_lock;
> >> +
> >> +	if (q->uacce->ops->start_queue) {
> >> +		ret = q->uacce->ops->start_queue(q);
> >> +		if (ret < 0)
> >> +			goto out_with_lock;
> >> +	}
> >> +
> >> +	q->state = UACCE_Q_STARTED;  
> > out_with_lock:  
> >> +	mutex_unlock(&uacce_mutex);
> >> +  
> > return ret;
> > Though need to handle ret a bit differently above...  
> OK
> >
> > +static int uacce_fops_mmap(struct file *filep, struct vm_area_struct *vma)
> > +{
> > +	struct uacce_queue *q = filep->private_data;
> > +	struct uacce_device *uacce = q->uacce;
> > +	struct uacce_qfile_region *qfr;
> > +	enum uacce_qfrt type = 0;
> > +	unsigned int flags = 0;
> > +	int ret;
> > +
> > +	if (vma->vm_pgoff < UACCE_QFRT_MAX)
> > +		type = vma->vm_pgoff;
> > +
> > +	vma->vm_flags |= VM_DONTCOPY | VM_DONTEXPAND | VM_WIPEONFORK;
> > +	vma->vm_ops = &uacce_vm_ops;
> > +	vma->vm_private_data = q;
> > +
> > +	mutex_lock(&uacce_mutex);
> > +
> > +	if (q->qfrs[type]) {
> > +		ret = -EEXIST;
> > +		goto out_with_lock;
> > +	}
> > +
> > +	switch (type) {
> > +	case UACCE_QFRT_MMIO:
> > +		flags = UACCE_QFRF_SELFMT;
> > +		break;
> > +
> > +	case UACCE_QFRT_DUS:
> > +		if (uacce->flags & UACCE_DEV_SVA) {
> > +			flags = UACCE_QFRF_SELFMT;
> > +			break;
> > +		}
> > +		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;
> > +
> > Could put
> > out_with_lock:
> > here and return ret instead of 0.
> > You'll need to set ret to default to 0 in that
> > case though.  
> OK
> >
> > +static ssize_t algorithms_show(struct device *dev,
> > +			       struct device_attribute *attr, char *buf)
> > +{
> > +	struct uacce_device *uacce = to_uacce_device(dev);
> > +
> > +	return sprintf(buf, "%s", uacce->algs);
> > Any risk algs won't have the \n?
> > I'd kind of expect it to be a null termated arrays to allow the core
> > to format it however it wants to.  
> Yes, adding \n is better.

This may then add a bonus new line if you have multiple lines already in
the string.  Probably doesn't do much harm, but it's not ideal.

> >  
> >> +}
> >> +
> >> +static ssize_t qfrt_mmio_size_show(struct device *dev,
> >> +				   struct device_attribute *attr, char *buf)
> >> +{
> >> +	struct uacce_device *uacce = to_uacce_device(dev);
> >> +
> >> +	return sprintf(buf, "%lu\n",
> >> +		       uacce->qf_pg_size[UACCE_QFRT_MMIO] << PAGE_SHIFT);
> >> +}
> >> +
> >> +static ssize_t qfrt_dus_size_show(struct device *dev,
> >> +				  struct device_attribute *attr, char *buf)
> >> +{
> >> +	struct uacce_device *uacce = to_uacce_device(dev);
> >> +
> >> +	return sprintf(buf, "%lu\n",
> >> +		       uacce->qf_pg_size[UACCE_QFRT_DUS] << PAGE_SHIFT);
> >> +}
> >> +
> >> +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(qfrt_mmio_size);
> >> +static DEVICE_ATTR_RO(qfrt_dus_size);
> >> +
> >> +static struct attribute *uacce_dev_attrs[] = {
> >> +	&dev_attr_id.attr,
> >> +	&dev_attr_api.attr,
> >> +	&dev_attr_node_id.attr,
> >> +	&dev_attr_numa_distance.attr,
> >> +	&dev_attr_flags.attr,
> >> +	&dev_attr_available_instances.attr,
> >> +	&dev_attr_algorithms.attr,
> >> +	&dev_attr_qfrt_mmio_size.attr,
> >> +	&dev_attr_qfrt_dus_size.attr,
> >> +	NULL,
> >> +};
> >> +ATTRIBUTE_GROUPS(uacce_dev);
> >> +
> >> +static void uacce_release(struct device *dev)
> >> +{
> >> +	struct uacce_device *uacce = to_uacce_device(dev);
> >> +
> >> +	kfree(uacce);
> >> +}
> >> +
> >> +/**
> >> + * uacce_register - register an accelerator  
> > This isn't quite correct kernel-doc.  Please run the
> > generation script over it and fix any warnings.
> >
> > 	uacce_register() - register an accelerator  
> Sure, will add (), though no warning reported from ./scripts/kernel-doc

I checked that one for another review yesterday.  Seems the kernel
suggested kernel-doc style isn't actually enforced and the brackets
are optional for functions.   It assumes anything it hasn't identified
as something else must be a function hence this is the one case where
careful matching doesn't apply (unlike struct, enum etc).


> >  
> >> + * @parent: pointer of uacce parent device
> >> + * @interface: pointer of uacce_interface for register
> >> + */
> >> +struct uacce_device *uacce_register(struct device *parent,
> >> +				    struct uacce_interface *interface)
> >> +{
> >> +	unsigned int flags = interface->flags;
> >> +	struct uacce_device *uacce;
> >> +	int ret;
> >> +
> >> +	uacce = kzalloc(sizeof(struct uacce_device), GFP_KERNEL);
> >> +	if (!uacce)
> >> +		return ERR_PTR(-ENOMEM);
> >> +
> >> +	if (flags & UACCE_DEV_SVA) {
> >> +		ret = iommu_dev_enable_feature(parent, IOMMU_DEV_FEAT_SVA);
> >> +		if (ret)
> >> +			flags &= ~UACCE_DEV_SVA;
> >> +	}
> >> +
> >> +	uacce->pdev = parent;
> >> +	uacce->flags = flags;
> >> +	uacce->ops = interface->ops;
> >> +
> >> +	ret = xa_alloc(&uacce_xa, &uacce->dev_id, uacce, xa_limit_32b,
> >> +		       GFP_KERNEL);
> >> +	if (ret < 0)
> >> +		goto err_with_uacce;
> >> +
> >> +	uacce->cdev = cdev_alloc();  
> > If we can embed this (see below) then use cdev_init instead.
> >  
> >> +	if (!uacce->cdev) {
> >> +		ret = -ENOMEM;
> >> +		goto err_with_xa;
> >> +	}
> >> +
> >> +	INIT_LIST_HEAD(&uacce->qs);
> >> +	mutex_init(&uacce->q_lock);
> >> +	uacce->cdev->ops = &uacce_fops;
> >> +	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_groups;
> >> +	uacce->dev.parent = uacce->pdev;
> >> +	uacce->dev.release = uacce_release;
> >> +	dev_set_name(&uacce->dev, "%s-%d", interface->name, uacce->dev_id);
> >> +	ret = cdev_device_add(uacce->cdev, &uacce->dev);
> >> +	if (ret)
> >> +		goto err_with_xa;
> >> +
> >> +	return uacce;
> >> +
> >> +err_with_xa:
> >> +	if (uacce->cdev)
> >> +		cdev_del(uacce->cdev);  
> > Why not use a separate label to handle the above rather than checking if
> > it's set?  
> ok,
> >  
> >> +	xa_erase(&uacce_xa, uacce->dev_id);
> >> +err_with_uacce:
> >> +	if (flags & UACCE_DEV_SVA)
> >> +		iommu_dev_disable_feature(uacce->pdev, IOMMU_DEV_FEAT_SVA);
> >> +	kfree(uacce);
> >> +	return ERR_PTR(ret);
> >> +}
> >> +EXPORT_SYMBOL_GPL(uacce_register);
> >> +
> >> +/**
> >> + * uacce_unregister - unregisters an accelerator
> >> + * @uacce: the accelerator to unregister
> >> + */
> >> +void uacce_unregister(struct uacce_device *uacce)
> >> +{
> >> +	if (!uacce)
> >> +		return;
> >> +  
> > I'd like to see a comment here on why we are doing things not unwinding
> > actions from uacce_register.  
> OK will add comments.
> Here is "ensure no open queue remains"
> >> +	mutex_lock(&uacce->q_lock);
> >> +	if (!list_empty(&uacce->qs)) {
> >> +		struct uacce_queue *q;
> >> +
> >> +		list_for_each_entry(q, &uacce->qs, list) {
> >> +			uacce_put_queue(q);
> >> +			if (uacce->flags & UACCE_DEV_SVA)
> >> +				iommu_sva_unbind_device(q->handle);
> >> +		}
> >> +	}
> >> +	mutex_unlock(&uacce->q_lock);
> >> +  
> > For these next parts which are the unwind of uacce_register, why are they not
> > in the reverse order of what is happening in there (where possible given
> > device lifespan). That is why do we not disable the iommu feature much later?  
> First close all queues, then disable sva feature.
> >  
> >> +	if (uacce->flags & UACCE_DEV_SVA)
> >> +		iommu_dev_disable_feature(uacce->pdev, IOMMU_DEV_FEAT_SVA);
> >> +
> >> +	cdev_device_del(uacce->cdev, &uacce->dev);
> >> +	xa_erase(&uacce_xa, uacce->dev_id);
> >> +	put_device(&uacce->dev);
> >> +}
> >> +EXPORT_SYMBOL_GPL(uacce_unregister);
> >> +
> >> +static int __init uacce_init(void)
> >> +{
> >> +	int ret;
> >> +
> >> +	uacce_class = class_create(THIS_MODULE, UACCE_NAME);
> >> +	if (IS_ERR(uacce_class))
> >> +		return PTR_ERR(uacce_class);
> >> +
> >> +	ret = alloc_chrdev_region(&uacce_devt, 0, MINORMASK, UACCE_NAME);
> >> +	if (ret) {
> >> +		class_destroy(uacce_class);
> >> +		return ret;  
> > drop the return ret out of these brackets. i.e.
> >
> > if (ret)
> > 	class_destroy(uacce_class)
> >
> > return ret;  
> sure, thanks
> >  
> >> +	}
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static __exit void uacce_exit(void)
> >> +{
> >> +	unregister_chrdev_region(uacce_devt, MINORMASK);
> >> +	class_destroy(uacce_class);
> >> +}
> >> +
> >> +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..04c8643
> >> --- /dev/null
> >> +++ b/include/linux/uacce.h
> >> @@ -0,0 +1,163 @@
> >> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> >> +#ifndef _LINUX_UACCE_H
> >> +#define _LINUX_UACCE_H
> >> +
> >> +#include <linux/cdev.h>
> >> +#include <uapi/misc/uacce/uacce.h>
> >> +
> >> +#define UACCE_NAME		"uacce"
> >> +#define UACCE_QFRT_MAX		16  
> > What does QFRT stand for?  
> change to UACCE_MAX_REGION

Much better

> >> +#define UACCE_MAX_NAME_SIZE	64
> >> +
> >> +struct uacce_queue;
> >> +struct uacce_device;
> >> +
> >> +/**
> >> + * enum uacce_qfr_flag: queue file flag:
> >> + * @UACCE_QFRF_SELFMT: self maintained qfr
> >> + */
> >> +enum uacce_qfr_flag {
> >> +	UACCE_QFRF_SELFMT = BIT(0),
> >> +};  
> > Same issue with enums for flags.  Doesn't make much sense to me.
> > Only one value can be taken which doesn't make it a flag.
> >  
> >> +
> >> +/**
> >> + * struct uacce_qfile_region - structure of queue file region
> >> + * @type: type of the qfr
> >> + * @flags: flags of qfr
> >> + * @prot: qfr protection flag
> >> + */
> >> +struct uacce_qfile_region {
> >> +	enum uacce_qfrt type;
> >> +	enum uacce_qfr_flag flags;
> >> +	u32 prot;
> >> +};
> >> +
> >> +/**
> >> + * struct uacce_ops - uacce device operations
> >> + * @get_available_instances:  get available instances left of the device
> >> + * @get_queue: get a queue from the device
> >> + * @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 uacce device
> >> + * @reset_queue: reset the queue
> >> + * @ioctl: ioctl for user space users of the queue
> >> + */
> >> +struct uacce_ops {
> >> +	int (*get_available_instances)(struct uacce_device *uacce);
> >> +	int (*get_queue)(struct uacce_device *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_device *uacce);
> >> +	int (*reset_queue)(struct uacce_queue *q);  
> > Some of these aren't used on only existing driver.  Introduce them only
> > in the series that uses them.  
> OK
> >  
> >> +	long (*ioctl)(struct uacce_queue *q, unsigned int cmd,
> >> +		      unsigned long arg);
> >> +};
> >> +
> >> +/**
> >> + * struct uacce_interface  
> > I think this needs a description for kernel doc (even if it's obvious!)
> > Could be wrong though.  
> OK
> >  
> >> + * @name: the uacce device name.  Will show up in sysfs
> >> + * @flags: uacce device attributes
> >> + * @ops: pointer to the struct uacce_ops
> >> + *
> >> + * This structure is used for the uacce_register()
> >> + */
> >> +struct uacce_interface {
> >> +	char name[UACCE_MAX_NAME_SIZE];
> >> +	enum uacce_dev_flag flags;
> >> +	struct uacce_ops *ops;
> >> +};
> >> +
> >> +enum uacce_q_state {
> >> +	UACCE_Q_INIT,
> >> +	UACCE_Q_STARTED,
> >> +	UACCE_Q_ZOMBIE,
> >> +};
> >> +
> >> +/**
> >> + * struct uacce_queue
> >> + * @uacce: pointer to uacce
> >> + * @priv: private pointer
> >> + * @wait: wait queue head
> >> + * @pasid: pasid of the queue
> >> + * @pid: pid of the process using the queue
> >> + * @handle: iommu_sva handle return from iommu_sva_bind_device
> >> + * @list: queue list
> >> + * @qfrs: pointer of qfr regions
> >> + * @state: queue state machine
> >> + */
> >> +struct uacce_queue {
> >> +	struct uacce_device *uacce;
> >> +	void *priv;
> >> +	wait_queue_head_t wait;
> >> +	int pasid;
> >> +	pid_t pid;
> >> +	struct iommu_sva *handle;
> >> +	struct list_head list;
> >> +	struct uacce_qfile_region *qfrs[UACCE_QFRT_MAX];
> >> +	enum uacce_q_state state;
> >> +};
> >> +
> >> +/**
> >> + * struct uacce_device
> >> + * @algs: supported algorithms
> >> + * @api_ver: api version
> >> + * @qf_pg_size: page size of the queue file regions
> >> + * @ops: pointer to the struct uacce_ops
> >> + * @pdev: pointer to the parent device
> >> + * @is_vf: whether virtual function
> >> + * @flags: uacce attributes
> >> + * @dev_id: id of the uacce device
> >> + * @prot: uacce protection flag
> >> + * @cdev: cdev of the uacce
> >> + * @dev: dev of the uacce
> >> + * @priv: private pointer of the uacce
> >> + * @qs: list head of queue->list
> >> + * @q_lock: lock for qs
> >> + */
> >> +struct uacce_device {
> >> +	const char *algs;
> >> +	const char *api_ver;
> >> +	unsigned long qf_pg_size[UACCE_QFRT_MAX];
> >> +	struct uacce_ops *ops;  
> > Can we make this ops structure a point to a constant struct?
> > I'm guessing it'll be fixed for a given driver.  
> OK
> >  
> >> +	struct device *pdev;  
> > Perhaps just call it parent. pdev will be confusing with
> > pci devices.  
> OK
> >  
> >> +	bool is_vf;
> >> +	u32 flags;
> >> +	u32 dev_id;
> >> +	u32 prot;
> >> +	struct cdev *cdev;  
> > Can we embed the cdev structure rather than use a pointer
> > and separate allocation?  
> NO, we can't.
> We originally embed the cdev structure, and Greg reminded us these two 
> structure have different lifetime.
> https://lkml.org/lkml/2019/8/28/771

Ok. Fair enough.


> >> +	struct device dev;
> >> +	void *priv;
> >> +	struct list_head qs;
> >> +	struct mutex q_lock;
> >> +};
> >> +
> >> +#if IS_ENABLED(CONFIG_UACCE)
> >> +
> >> +struct uacce_device *uacce_register(struct device *parent,
> >> +				    struct uacce_interface *interface);
> >> +void uacce_unregister(struct uacce_device *uacce);
> >> +
> >> +#else /* CONFIG_UACCE */
> >> +
> >> +static inline
> >> +struct uacce_device *uacce_register(struct device *parent,
> >> +				    struct uacce_interface *interface)
> >> +{
> >> +	return ERR_PTR(-ENODEV);
> >> +}
> >> +
> >> +static inline void uacce_unregister(struct uacce_device *uacce) {}
> >> +
> >> +#endif /* CONFIG_UACCE */
> >> +
> >> +#endif /* _LINUX_UACCE_H */
> >> diff --git a/include/uapi/misc/uacce/uacce.h b/include/uapi/misc/uacce/uacce.h
> >> new file mode 100644
> >> index 0000000..a4f9378
> >> --- /dev/null
> >> +++ b/include/uapi/misc/uacce/uacce.h
> >> @@ -0,0 +1,38 @@
> >> +/* SPDX-License-Identifier: GPL-2.0+ WITH Linux-syscall-note */
> >> +#ifndef _UAPIUUACCE_H
> >> +#define _UAPIUUACCE_H
> >> +
> >> +#include <linux/types.h>
> >> +#include <linux/ioctl.h>
> >> +
> >> +/* UACCE_CMD_START_Q: Start the queue */
> >> +#define UACCE_CMD_START_Q	_IO('W', 0)
> >> +
> >> +/**
> >> + * UACCE_CMD_PUT_Q:
> >> + * User actively stop queue and free queue resource immediately
> >> + * Optimization method since close fd may delay
> >> + */
> >> +#define UACCE_CMD_PUT_Q		_IO('W', 1)
> >> +
> >> +/**
> >> + * enum uacce_dev_flag: Device flags:
> >> + * @UACCE_DEV_SVA: Shared Virtual Addresses
> >> + *		   Support PASID
> >> + *		   Support device page faults (PCI PRI or SMMU Stall)
> >> + */
> >> +enum uacce_dev_flag {
> >> +	UACCE_DEV_SVA = BIT(0),  
> > As mentioned in docs review, this doesn't look like an enum to me.
> > Just use #define for the bit and a suitable sized integer for any
> > calls using it.  
> OK, but there are still more features in the future patch.
That's not the issue.  An enum should (more or less) use concurrent values.

A = 0,
B = 1,
C = 2, etc
and an instance of it should only take one of them.

Once you are using it as values for a bitmap, the typing becomes irrelevant
as you can't really use it to enforce anything, so you should just use.

#define UACCE_DEV_SVA BIT(0)
#define UACCE_DEV_SOMETHING BIT(1) etc

> 
> Thanks
> 



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v7 3/3] crypto: hisilicon - register zip engine to uacce
  2019-11-05  8:34     ` zhangfei
@ 2019-11-11 11:30       ` Jonathan Cameron
  0 siblings, 0 replies; 16+ messages in thread
From: Jonathan Cameron @ 2019-11-11 11:30 UTC (permalink / raw)
  To: zhangfei
  Cc: Greg Kroah-Hartman, Arnd Bergmann, Herbert Xu, grant.likely,
	jean-philippe, Jerome Glisse, ilias.apalodimas, francois.ozog,
	kenneth-lee-2012, Wangzhou, haojian . zhuang, guodong.xu,
	linux-accelerators, linux-kernel, linux-crypto, iommu

On Tue, 5 Nov 2019 16:34:48 +0800
zhangfei <zhangfei.gao@linaro.org> wrote:

> Hi, Jonathan
> 
> On 2019/11/1 上午1:53, Jonathan Cameron wrote:
> > On Tue, 29 Oct 2019 14:40:16 +0800
> > Zhangfei Gao <zhangfei.gao@linaro.org> wrote:
> >  
> >> Register qm to uacce framework for user crypto driver
> >>
> >> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
> >> Signed-off-by: Zhou Wang <wangzhou1@hisilicon.com>  
> > Hi.
> >
> > This shows there is probably a race during setup that you should close.
> > Userspace interface is exposed before the driver is ready to handle it.
> >
> > Few other bits inline.
> >
> > Thanks,
> >
> > Jonathan
> >  
> >> ---
> >>   drivers/crypto/hisilicon/qm.c           | 253 ++++++++++++++++++++++++++++++--
> >>   drivers/crypto/hisilicon/qm.h           |  13 +-
> >>   drivers/crypto/hisilicon/zip/zip_main.c |  39 ++---
> >>   include/uapi/misc/uacce/qm.h            |  23 +++
> >>   4 files changed, 292 insertions(+), 36 deletions(-)
> >>   create mode 100644 include/uapi/misc/uacce/qm.h
> >>
> >> diff --git a/drivers/crypto/hisilicon/qm.c b/drivers/crypto/hisilicon/qm.c
> >> index a8ed6990..4b9cced 100644
> >> --- a/drivers/crypto/hisilicon/qm.c
> >> +++ b/drivers/crypto/hisilicon/qm.c
> >> @@ -9,6 +9,9 @@
> >>   #include <linux/log2.h>
> >>   #include <linux/seq_file.h>
> >>   #include <linux/slab.h>
> >> +#include <linux/uacce.h>
> >> +#include <linux/uaccess.h>
> >> +#include <uapi/misc/uacce/qm.h>
> >>   #include "qm.h"
> >>   
> >>   /* eq/aeq irq enable */
> >> @@ -465,17 +468,22 @@ static void qm_cq_head_update(struct hisi_qp *qp)
> >>   
> >>   static void qm_poll_qp(struct hisi_qp *qp, struct hisi_qm *qm)
> >>   {
> >> -	struct qm_cqe *cqe = qp->cqe + qp->qp_status.cq_head;
> >> -
> >> -	if (qp->req_cb) {
> >> -		while (QM_CQE_PHASE(cqe) == qp->qp_status.cqc_phase) {
> >> -			dma_rmb();
> >> -			qp->req_cb(qp, qp->sqe + qm->sqe_size * cqe->sq_head);
> >> -			qm_cq_head_update(qp);
> >> -			cqe = qp->cqe + qp->qp_status.cq_head;
> >> -			qm_db(qm, qp->qp_id, QM_DOORBELL_CMD_CQ,
> >> -			      qp->qp_status.cq_head, 0);
> >> -			atomic_dec(&qp->qp_status.used);
> >> +	struct qm_cqe *cqe;
> >> +
> >> +	if (qp->event_cb) {
> >> +		qp->event_cb(qp);
> >> +	} else {
> >> +		cqe = qp->cqe + qp->qp_status.cq_head;
> >> +
> >> +		if (qp->req_cb) {
> >> +			while (QM_CQE_PHASE(cqe) == qp->qp_status.cqc_phase) {
> >> +				dma_rmb();
> >> +				qp->req_cb(qp, qp->sqe + qm->sqe_size *
> >> +					   cqe->sq_head);
> >> +				qm_cq_head_update(qp);
> >> +				cqe = qp->cqe + qp->qp_status.cq_head;
> >> +				atomic_dec(&qp->qp_status.used);
> >> +			}
> >>   		}
> >>   
> >>   		/* set c_flag */
> >> @@ -1397,6 +1405,220 @@ static void hisi_qm_cache_wb(struct hisi_qm *qm)
> >>   	}
> >>   }
> >>   
> >> +static void qm_qp_event_notifier(struct hisi_qp *qp)
> >> +{
> >> +	wake_up_interruptible(&qp->uacce_q->wait);
> >> +}
> >> +
> >> +static int hisi_qm_get_available_instances(struct uacce_device *uacce)
> >> +{
> >> +	int i, ret;
> >> +	struct hisi_qm *qm = uacce->priv;
> >> +
> >> +	read_lock(&qm->qps_lock);
> >> +	for (i = 0, ret = 0; i < qm->qp_num; i++)
> >> +		if (!qm->qp_array[i])
> >> +			ret++;
> >> +	read_unlock(&qm->qps_lock);
> >> +
> >> +	return ret;
> >> +}
> >> +
> >> +static int hisi_qm_uacce_get_queue(struct uacce_device *uacce,
> >> +				   unsigned long arg,
> >> +				   struct uacce_queue *q)
> >> +{
> >> +	struct hisi_qm *qm = uacce->priv;
> >> +	struct hisi_qp *qp;
> >> +	u8 alg_type = 0;
> >> +
> >> +	qp = hisi_qm_create_qp(qm, alg_type);
> >> +	if (IS_ERR(qp))
> >> +		return PTR_ERR(qp);
> >> +
> >> +	q->priv = qp;
> >> +	q->uacce = uacce;
> >> +	qp->uacce_q = q;
> >> +	qp->event_cb = qm_qp_event_notifier;
> >> +	qp->pasid = arg;
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static void hisi_qm_uacce_put_queue(struct uacce_queue *q)
> >> +{
> >> +	struct hisi_qp *qp = q->priv;
> >> +
> >> +	/*
> >> +	 * As put_queue is only called in uacce_mode=1, and only one queue can  
> > We got rid of the modes I think so comment needs an update.  
> Yes
> >  
> >> +	 * be used in this mode. we flush all sqc cache back in put queue.
> >> +	 */
> >> +	hisi_qm_cache_wb(qp->qm);
> >> +
> >> +	/* need to stop hardware, but can not support in v1 */
> >> +	hisi_qm_release_qp(qp);  
> > Should we just drop support for the v1 hardware if we can't do this?
> >  
> >> +}
> >> +
> >> +/* map sq/cq/doorbell to user space */
> >> +static int hisi_qm_uacce_mmap(struct uacce_queue *q,
> >> +			      struct vm_area_struct *vma,
> >> +			      struct uacce_qfile_region *qfr)
> >> +{
> >> +	struct hisi_qp *qp = q->priv;
> >> +	struct hisi_qm *qm = qp->qm;
> >> +	size_t sz = vma->vm_end - vma->vm_start;
> >> +	struct pci_dev *pdev = qm->pdev;
> >> +	struct device *dev = &pdev->dev;
> >> +	unsigned long vm_pgoff;
> >> +	int ret;
> >> +
> >> +	switch (qfr->type) {
> >> +	case UACCE_QFRT_MMIO:
> >> +		if (qm->ver == QM_HW_V2) {
> >> +			if (sz > PAGE_SIZE * (QM_DOORBELL_PAGE_NR +
> >> +			    QM_DOORBELL_SQ_CQ_BASE_V2 / PAGE_SIZE))
> >> +				return -EINVAL;
> >> +		} else {
> >> +			if (sz > PAGE_SIZE * QM_DOORBELL_PAGE_NR)
> >> +				return -EINVAL;
> >> +		}
> >> +
> >> +		vma->vm_flags |= VM_IO;
> >> +
> >> +		return remap_pfn_range(vma, vma->vm_start,
> >> +				       qm->phys_base >> PAGE_SHIFT,
> >> +				       sz, pgprot_noncached(vma->vm_page_prot));
> >> +	case UACCE_QFRT_DUS:
> >> +		if (sz != qp->qdma.size)
> >> +			return -EINVAL;
> >> +  
> > Comment style in here is inconsistent. Match the existing code.  
> OK
> >> +		/* dma_mmap_coherent() requires vm_pgoff as 0
> >> +		 * restore vm_pfoff to initial value for mmap()
> >> +		 */
> >> +		vm_pgoff = vma->vm_pgoff;
> >> +		vma->vm_pgoff = 0;
> >> +		ret = dma_mmap_coherent(dev, vma, qp->qdma.va,
> >> +					qp->qdma.dma, sz);
> >> +		vma->vm_pgoff = vm_pgoff;
> >> +		return ret;
> >> +
> >> +	default:
> >> +		return -EINVAL;
> >> +	}
> >> +}
> >> +
> >> +static int hisi_qm_uacce_start_queue(struct uacce_queue *q)
> >> +{
> >> +	struct hisi_qp *qp = q->priv;
> >> +
> >> +	return hisi_qm_start_qp(qp, qp->pasid);
> >> +}
> >> +
> >> +static void hisi_qm_uacce_stop_queue(struct uacce_queue *q)
> >> +{
> >> +	struct hisi_qp *qp = q->priv;
> >> +
> >> +	hisi_qm_stop_qp(qp);  
> > I'm a great fan of minimalism on these
> > 	hisi_qm_stop_qp(q->priv); doesn't really loose any clarity.  
> OK
> >> +}
> >> +
> >> +static int qm_set_sqctype(struct uacce_queue *q, u16 type)
> >> +{
> >> +	struct hisi_qm *qm = q->uacce->priv;
> >> +	struct hisi_qp *qp = q->priv;
> >> +
> >> +	write_lock(&qm->qps_lock);
> >> +	qp->alg_type = type;
> >> +	write_unlock(&qm->qps_lock);
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static long hisi_qm_uacce_ioctl(struct uacce_queue *q, unsigned int cmd,
> >> +				unsigned long arg)
> >> +{
> >> +	struct hisi_qp *qp = q->priv;
> >> +	struct hisi_qp_ctx qp_ctx;
> >> +
> >> +	if (cmd == UACCE_CMD_QM_SET_QP_CTX) {
> >> +		if (copy_from_user(&qp_ctx, (void __user *)arg,
> >> +				   sizeof(struct hisi_qp_ctx)))
> >> +			return -EFAULT;
> >> +
> >> +		if (qp_ctx.qc_type != 0 && qp_ctx.qc_type != 1)
> >> +			return -EINVAL;
> >> +
> >> +		qm_set_sqctype(q, qp_ctx.qc_type);
> >> +		qp_ctx.id = qp->qp_id;
> >> +
> >> +		if (copy_to_user((void __user *)arg, &qp_ctx,
> >> +				 sizeof(struct hisi_qp_ctx)))
> >> +			return -EFAULT;
> >> +	} else {
> >> +		return -EINVAL;
> >> +	}
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static struct uacce_ops uacce_qm_ops = {
> >> +	.get_available_instances = hisi_qm_get_available_instances,
> >> +	.get_queue = hisi_qm_uacce_get_queue,
> >> +	.put_queue = hisi_qm_uacce_put_queue,
> >> +	.start_queue = hisi_qm_uacce_start_queue,
> >> +	.stop_queue = hisi_qm_uacce_stop_queue,
> >> +	.mmap = hisi_qm_uacce_mmap,
> >> +	.ioctl = hisi_qm_uacce_ioctl,
> >> +};
> >> +
> >> +static int qm_register_uacce(struct hisi_qm *qm)
> >> +{
> >> +	struct pci_dev *pdev = qm->pdev;
> >> +	struct uacce_device *uacce;
> >> +	unsigned long mmio_page_nr;
> >> +	unsigned long dus_page_nr;
> >> +	struct uacce_interface interface = {
> >> +		.flags = UACCE_DEV_SVA,
> >> +		.ops = &uacce_qm_ops,
> >> +	};
> >> +
> >> +	strncpy(interface.name, pdev->driver->name, sizeof(interface.name));
> >> +
> >> +	uacce = uacce_register(&pdev->dev, &interface);
> >> +	if (IS_ERR(uacce))
> >> +		return PTR_ERR(uacce);  
> > Is there a potential race here as we have exposed the character device before
> > the driver is ready for it to be used?  Probably need to split the code that
> > allocates a uacce interface from the bit that actually exposes it to userspace.  
> I don't think it is a race condition.
> Since no requirement of get sysfs ready then register character device.
> Also sysfs does not always refect constant members, like available_instance.
> Currently we set the sysfs members after uacce_register, which alloc 
> uacce device.
> So no problem if they are ready before character device open.

The ordering of sysfs and chrdev inside the uacce_register wasn't what
I was referring to. It is the remaining setup done on the uacce below
after both chrdev and sysfs interface are ready.

> 
> If we split the code, allocate an uacce interface first then expose to 
> usersapce, an additional api maybe required.

I'd pay the price of the extra api.  It's still far from clear to me
why we don't have a race.   For example, if we stop right here for
a long time, the entire UACCE interface is exposed but we haven't
set the various priv pointers etc below.  Seems unlikely to be safe.

> 
> >  
> >> +
> >> +	if (uacce->flags & UACCE_DEV_SVA) {
> >> +		qm->use_sva = true;
> >> +	} else {
> >> +		/* only consider sva case */
> >> +		uacce_unregister(uacce);
> >> +		return -EINVAL;
> >> +	}
> >> +
> >> +	uacce->is_vf = pdev->is_virtfn;
> >> +	uacce->priv = qm;
> >> +	uacce->algs = qm->algs;
> >> +
> >> +	if (qm->ver == QM_HW_V1) {
> >> +		mmio_page_nr = QM_DOORBELL_PAGE_NR;
> >> +		uacce->api_ver = HISI_QM_API_VER_BASE;
> >> +	} else {
> >> +		mmio_page_nr = QM_DOORBELL_PAGE_NR +
> >> +			QM_DOORBELL_SQ_CQ_BASE_V2 / PAGE_SIZE;
> >> +		uacce->api_ver = HISI_QM_API_VER2_BASE;
> >> +	}
> >> +
> >> +	dus_page_nr = (PAGE_SIZE - 1 + qm->sqe_size * QM_Q_DEPTH +
> >> +		       sizeof(struct qm_cqe) * QM_Q_DEPTH) >> PAGE_SHIFT;
> >> +
> >> +	uacce->qf_pg_size[UACCE_QFRT_MMIO] = mmio_page_nr;
> >> +	uacce->qf_pg_size[UACCE_QFRT_DUS]  = dus_page_nr;
> >> +
> >> +	qm->uacce = uacce;
> >> +
> >> +	return 0;
> >> +}
> >> +
> >>   /**
> >>    * hisi_qm_init() - Initialize configures about qm.
> >>    * @qm: The qm needing init.
> >> @@ -1421,6 +1643,10 @@ int hisi_qm_init(struct hisi_qm *qm)
> >>   		return -EINVAL;
> >>   	}
> >>   
> >> +	ret = qm_register_uacce(qm);
> >> +	if (ret < 0)
> >> +		dev_warn(&pdev->dev, "fail to register uacce (%d)\n", ret);
> >> +  
> > looks like there are error paths in qm_init in which we should call
> > the uacce_unregister?  
> OK
> >  
> >>   	ret = pci_enable_device_mem(pdev);
> >>   	if (ret < 0) {
> >>   		dev_err(&pdev->dev, "Failed to enable device mem!\n");
> >> @@ -1433,6 +1659,8 @@ int hisi_qm_init(struct hisi_qm *qm)
> >>   		goto err_disable_pcidev;
> >>   	}
> >>   
> >> +	qm->phys_base = pci_resource_start(pdev, PCI_BAR_2);
> >> +	qm->size = pci_resource_len(qm->pdev, PCI_BAR_2);
> >>   	qm->io_base = ioremap(pci_resource_start(pdev, PCI_BAR_2),
> >>   			      pci_resource_len(qm->pdev, PCI_BAR_2));  
> > Use qm->phys_base/size in the ioremap here to avoid repeating the code.  
> OK
> >  
> >>   	if (!qm->io_base) {
> >> @@ -1504,6 +1732,9 @@ void hisi_qm_uninit(struct hisi_qm *qm)
> >>   	iounmap(qm->io_base);
> >>   	pci_release_mem_regions(pdev);
> >>   	pci_disable_device(pdev);
> >> +
> >> +	if (qm->uacce)
> >> +		uacce_unregister(qm->uacce);  
> > Can we make uacce_unregister check the input?
> > Might make for cleaner users.  
> OK,
> >  
> >>   }
> >>   EXPORT_SYMBOL_GPL(hisi_qm_uninit);
> >>   
> >> diff --git a/drivers/crypto/hisilicon/qm.h b/drivers/crypto/hisilicon/qm.h
> >> index 103e2fd..84a3be9 100644
> >> --- a/drivers/crypto/hisilicon/qm.h
> >> +++ b/drivers/crypto/hisilicon/qm.h
> >> @@ -77,6 +77,10 @@
> >>   
> >>   #define HISI_ACC_SGL_SGE_NR_MAX		255
> >>   
> >> +/* page number for queue file region */
> >> +#define QM_DOORBELL_PAGE_NR		1
> >> +  
> > 1 blank line only is almost always enough.
> >  
> >> +
> >>   enum qp_state {
> >>   	QP_STOP,
> >>   };
> >> @@ -161,7 +165,12 @@ struct hisi_qm {
> >>   	u32 error_mask;
> >>   	u32 msi_mask;
> >>   
> >> +	const char *algs;
> >>   	bool use_dma_api;
> >> +	bool use_sva;
> >> +	resource_size_t phys_base;
> >> +	resource_size_t size;
> >> +	struct uacce_device *uacce;
> >>   };
> >>   
> >>   struct hisi_qp_status {
> >> @@ -191,10 +200,12 @@ struct hisi_qp {
> >>   	struct hisi_qp_ops *hw_ops;
> >>   	void *qp_ctx;
> >>   	void (*req_cb)(struct hisi_qp *qp, void *data);
> >> +	void (*event_cb)(struct hisi_qp *qp);
> >>   	struct work_struct work;
> >>   	struct workqueue_struct *wq;
> >> -  
> > unrelated change.
> >  
> >>   	struct hisi_qm *qm;
> >> +	u16 pasid;
> >> +	struct uacce_queue *uacce_q;
> >>   };
> >>   
> >>   int hisi_qm_init(struct hisi_qm *qm);
> >> diff --git a/drivers/crypto/hisilicon/zip/zip_main.c b/drivers/crypto/hisilicon/zip/zip_main.c
> >> index 1b2ee96..48860d2 100644
> >> --- a/drivers/crypto/hisilicon/zip/zip_main.c
> >> +++ b/drivers/crypto/hisilicon/zip/zip_main.c
> >> @@ -316,8 +316,14 @@ static void hisi_zip_set_user_domain_and_cache(struct hisi_zip *hisi_zip)
> >>   	writel(AXUSER_BASE, base + HZIP_BD_RUSER_32_63);
> >>   	writel(AXUSER_BASE, base + HZIP_SGL_RUSER_32_63);
> >>   	writel(AXUSER_BASE, base + HZIP_BD_WUSER_32_63);
> >> -	writel(AXUSER_BASE, base + HZIP_DATA_RUSER_32_63);
> >> -	writel(AXUSER_BASE, base + HZIP_DATA_WUSER_32_63);
> >> +
> >> +	if (hisi_zip->qm.use_sva) {
> >> +		writel(AXUSER_BASE | AXUSER_SSV, base + HZIP_DATA_RUSER_32_63);
> >> +		writel(AXUSER_BASE | AXUSER_SSV, base + HZIP_DATA_WUSER_32_63);
> >> +	} else {
> >> +		writel(AXUSER_BASE, base + HZIP_DATA_RUSER_32_63);
> >> +		writel(AXUSER_BASE, base + HZIP_DATA_WUSER_32_63);
> >> +	}
> >>   
> >>   	/* let's open all compression/decompression cores */
> >>   	writel(DECOMP_CHECK_ENABLE | ALL_COMP_DECOMP_EN,
> >> @@ -671,24 +677,12 @@ static int hisi_zip_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> >>   	qm = &hisi_zip->qm;
> >>   	qm->pdev = pdev;
> >>   	qm->ver = rev_id;
> >> -  
> > Try to avoid noise from white space changes.  No huge help to delete the blank line here.
> >  
> >> +	qm->use_dma_api = true;
> >> +	qm->algs = "zlib\ngzip\n";
> >>   	qm->sqe_size = HZIP_SQE_SIZE;
> >>   	qm->dev_name = hisi_zip_name;
> >>   	qm->fun_type = (pdev->device == PCI_DEVICE_ID_ZIP_PF) ? QM_HW_PF :
> >>   								QM_HW_VF;  
> > Unrelated changes I think.  Can we clean out the old left overs
> > of uacce from the driver in a precursor patch? Also if it's no longer
> > used can we drop the module param?  
> This patch is an example, so just in one patch to make it work.
> >  
> >> -	switch (uacce_mode) {
> >> -	case 0:
> >> -		qm->use_dma_api = true;
> >> -		break;
> >> -	case 1:
> >> -		qm->use_dma_api = false;
> >> -		break;
> >> -	case 2:
> >> -		qm->use_dma_api = true;
> >> -		break;
> >> -	default:
> >> -		return -EINVAL;
> >> -	}
> >>   
> >>   	ret = hisi_qm_init(qm);
> >>   	if (ret) {
> >> @@ -976,12 +970,10 @@ static int __init hisi_zip_init(void)
> >>   		goto err_pci;
> >>   	}
> >>   
> >> -	if (uacce_mode == 0 || uacce_mode == 2) {
> >> -		ret = hisi_zip_register_to_crypto();
> >> -		if (ret < 0) {
> >> -			pr_err("Failed to register driver to crypto.\n");
> >> -			goto err_crypto;
> >> -		}
> >> +	ret = hisi_zip_register_to_crypto();
> >> +	if (ret < 0) {
> >> +		pr_err("Failed to register driver to crypto.\n");
> >> +		goto err_crypto;
> >>   	}
> >>   
> >>   	return 0;
> >> @@ -996,8 +988,7 @@ static int __init hisi_zip_init(void)
> >>   
> >>   static void __exit hisi_zip_exit(void)
> >>   {
> >> -	if (uacce_mode == 0 || uacce_mode == 2)
> >> -		hisi_zip_unregister_from_crypto();
> >> +	hisi_zip_unregister_from_crypto();  
> >
> >  
> >>   	pci_unregister_driver(&hisi_zip_pci_driver);
> >>   	hisi_zip_unregister_debugfs();
> >>   }
> >> diff --git a/include/uapi/misc/uacce/qm.h b/include/uapi/misc/uacce/qm.h
> >> new file mode 100644
> >> index 0000000..d79a8f2
> >> --- /dev/null
> >> +++ b/include/uapi/misc/uacce/qm.h  
> > Given generic directory (assuming uacce becomes heavily used) probably
> > want to prefix that if it is unique to hisilicon.
> >
> > hisi_qm.h?  
> OK, good idea.
> >  
> >> @@ -0,0 +1,23 @@
> >> +/* SPDX-License-Identifier: GPL-2.0+ WITH Linux-syscall-note */
> >> +#ifndef HISI_QM_USR_IF_H
> >> +#define HISI_QM_USR_IF_H
> >> +
> >> +#include <linux/types.h>
> >> +
> >> +/**
> >> + * struct hisi_qp_ctx - User data for hisi qp.
> >> + * @id: Specifies which Turbo decode algorithm to use  
> > What's a Turbo algorithm?  I don't know and I have the manuals ;)  
> Sorry, will change that
> 
> Thanks
> 



^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2019-11-11 11:31 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-29  6:40 [PATCH v7 0/3] Add uacce module for Accelerator Zhangfei Gao
2019-10-29  6:40 ` [PATCH v7 1/3] uacce: Add documents for uacce Zhangfei Gao
2019-10-31 15:37   ` Jonathan Cameron
2019-10-29  6:40 ` [PATCH v7 2/3] uacce: add uacce driver Zhangfei Gao
2019-10-31 17:13   ` Jonathan Cameron
2019-11-05  7:43     ` zhangfei
2019-11-11 11:19       ` Jonathan Cameron
2019-11-05 11:48   ` Jean-Philippe Brucker
2019-11-06  8:17     ` zhangfei
2019-11-06 15:32       ` Jean-Philippe Brucker
2019-11-07 13:23         ` zhangfei
2019-11-08  7:48           ` Jean-Philippe Brucker
2019-10-29  6:40 ` [PATCH v7 3/3] crypto: hisilicon - register zip engine to uacce Zhangfei Gao
2019-10-31 17:53   ` Jonathan Cameron
2019-11-05  8:34     ` zhangfei
2019-11-11 11:30       ` Jonathan Cameron

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).