[RFC PATCH 0/4] Linaro restricted heap

[RFC PATCH 0/4] Linaro restricted heap

[Jens Wiklander]

Hi,

This patch set is based on top of Yong Wu's restricted heap patch set [1].
It's also a continuation on Olivier's Add dma-buf secure-heap patch set [2].

The Linaro restricted heap uses genalloc in the kernel to manage the heap
carvout. This is a difference from the Mediatek restricted heap which
relies on the secure world to manage the carveout.

I've tried to adress the comments on [2], but [1] introduces changes so I'm
afraid I've had to skip some comments.

This can be tested on QEMU with the following steps:
repo init -u https://github.com/jenswi-linaro/manifest.git -m qemu_v8.xml \
        -b prototype/sdp-v1
repo sync -j8
cd build
make toolchains -j4
make all -j$(nproc)
make run-only
# login and at the prompt:
xtest --sdp-basic

https://optee.readthedocs.io/en/latest/building/prerequisites.html
list dependencies needed to build the above.

The tests are pretty basic, mostly checking that a Trusted Application in
the secure world can access and manipulate the memory.

Cheers,
Jens

[1] https://lore.kernel.org/dri-devel/20240515112308.10171-1-yong.wu@mediatek.com/
[2] https://lore.kernel.org/lkml/20220805135330.970-1-olivier.masse@nxp.com/

Changes since Olivier's post [2]:
* Based on Yong Wu's post [1] where much of dma-buf handling is done in
  the generic restricted heap
* Simplifications and cleanup
* New commit message for "dma-buf: heaps: add Linaro restricted dmabuf heap
  support"
* Replaced the word "secure" with "restricted" where applicable

Etienne Carriere (1):
  tee: new ioctl to a register tee_shm from a dmabuf file descriptor

Jens Wiklander (2):
  dma-buf: heaps: restricted_heap: add no_map attribute
  dma-buf: heaps: add Linaro restricted dmabuf heap support

Olivier Masse (1):
  dt-bindings: reserved-memory: add linaro,restricted-heap

 .../linaro,restricted-heap.yaml               |  56 ++++++
 drivers/dma-buf/heaps/Kconfig                 |  10 ++
 drivers/dma-buf/heaps/Makefile                |   1 +
 drivers/dma-buf/heaps/restricted_heap.c       |  17 +-
 drivers/dma-buf/heaps/restricted_heap.h       |   2 +
 .../dma-buf/heaps/restricted_heap_linaro.c    | 165 ++++++++++++++++++
 drivers/tee/tee_core.c                        |  38 ++++
 drivers/tee/tee_shm.c                         | 104 ++++++++++-
 include/linux/tee_drv.h                       |  11 ++
 include/uapi/linux/tee.h                      |  29 +++
 10 files changed, 426 insertions(+), 7 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/reserved-memory/linaro,restricted-heap.yaml
 create mode 100644 drivers/dma-buf/heaps/restricted_heap_linaro.c

-- 
2.34.1

[RFC PATCH 1/4] dma-buf: heaps: restricted_heap: add no_map attribute

[Jens Wiklander]

Add a no_map attribute to struct restricted_heap_attachment and struct
restricted_heap to skip the call to dma_map_sgtable() if set. This
avoids trying to map a dma-buf that doens't refer to memory accessible
by the kernel.

Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
---
 drivers/dma-buf/heaps/restricted_heap.c | 17 +++++++++++++----
 drivers/dma-buf/heaps/restricted_heap.h |  2 ++
 2 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/dma-buf/heaps/restricted_heap.c b/drivers/dma-buf/heaps/restricted_heap.c
index 8bc8a5e3f969..4bf28e3727ca 100644
--- a/drivers/dma-buf/heaps/restricted_heap.c
+++ b/drivers/dma-buf/heaps/restricted_heap.c
@@ -16,6 +16,7 @@
 struct restricted_heap_attachment {
 	struct sg_table			*table;
 	struct device			*dev;
+	bool no_map;
 };
 
 static int
@@ -54,6 +55,8 @@ restricted_heap_memory_free(struct restricted_heap *rheap, struct restricted_buf
 static int restricted_heap_attach(struct dma_buf *dmabuf, struct dma_buf_attachment *attachment)
 {
 	struct restricted_buffer *restricted_buf = dmabuf->priv;
+	struct dma_heap *heap = restricted_buf->heap;
+	struct restricted_heap *rheap = dma_heap_get_drvdata(heap);
 	struct restricted_heap_attachment *a;
 	struct sg_table *table;
 
@@ -70,6 +73,7 @@ static int restricted_heap_attach(struct dma_buf *dmabuf, struct dma_buf_attachm
 	sg_dma_mark_restricted(table->sgl);
 	a->table = table;
 	a->dev = attachment->dev;
+	a->no_map = rheap->no_map;
 	attachment->priv = a;
 
 	return 0;
@@ -92,9 +96,12 @@ restricted_heap_map_dma_buf(struct dma_buf_attachment *attachment,
 	struct sg_table *table = a->table;
 	int ret;
 
-	ret = dma_map_sgtable(attachment->dev, table, direction, DMA_ATTR_SKIP_CPU_SYNC);
-	if (ret)
-		return ERR_PTR(ret);
+	if (!a->no_map) {
+		ret = dma_map_sgtable(attachment->dev, table, direction,
+				      DMA_ATTR_SKIP_CPU_SYNC);
+		if (ret)
+			return ERR_PTR(ret);
+	}
 	return table;
 }
 
@@ -106,7 +113,9 @@ restricted_heap_unmap_dma_buf(struct dma_buf_attachment *attachment, struct sg_t
 
 	WARN_ON(a->table != table);
 
-	dma_unmap_sgtable(attachment->dev, table, direction, DMA_ATTR_SKIP_CPU_SYNC);
+	if (!a->no_map)
+		dma_unmap_sgtable(attachment->dev, table, direction,
+				  DMA_ATTR_SKIP_CPU_SYNC);
 }
 
 static int
diff --git a/drivers/dma-buf/heaps/restricted_heap.h b/drivers/dma-buf/heaps/restricted_heap.h
index 7dec4b8a471b..94cc0842f70d 100644
--- a/drivers/dma-buf/heaps/restricted_heap.h
+++ b/drivers/dma-buf/heaps/restricted_heap.h
@@ -27,6 +27,8 @@ struct restricted_heap {
 	unsigned long		cma_paddr;
 	unsigned long		cma_size;
 
+	bool			no_map;
+
 	void			*priv_data;
 };
 
-- 
2.34.1

[RFC PATCH 2/4] tee: new ioctl to a register tee_shm from a dmabuf file descriptor

[Jens Wiklander]

From: Etienne Carriere <etienne.carriere@linaro.org>

Enable userspace to create a tee_shm object that refers to a dmabuf
reference.

Userspace registers the dmabuf file descriptor as in a tee_shm object.
The registration is completed with a tee_shm file descriptor returned to
userspace.

Userspace is free to close the dmabuf file descriptor now since all the
resources are now held via the tee_shm object.

Closing the tee_shm file descriptor will release all resources used by the
tee_shm object.

This change only support dmabuf references that relates to physically
contiguous memory buffers.

New tee_shm flag to identify tee_shm objects built from a registered
dmabuf, TEE_SHM_DMA_BUF.

Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
Signed-off-by: Olivier Masse <olivier.masse@nxp.com>
Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
---
 drivers/tee/tee_core.c   |  38 ++++++++++++++
 drivers/tee/tee_shm.c    | 104 +++++++++++++++++++++++++++++++++++++--
 include/linux/tee_drv.h  |  11 +++++
 include/uapi/linux/tee.h |  29 +++++++++++
 4 files changed, 179 insertions(+), 3 deletions(-)

diff --git a/drivers/tee/tee_core.c b/drivers/tee/tee_core.c
index e59c20d74b36..3dfd5428d58c 100644
--- a/drivers/tee/tee_core.c
+++ b/drivers/tee/tee_core.c
@@ -356,6 +356,42 @@ tee_ioctl_shm_register(struct tee_context *ctx,
 	return ret;
 }
 
+static int tee_ioctl_shm_register_fd(struct tee_context *ctx,
+				     struct tee_ioctl_shm_register_fd_data __user *udata)
+{
+	struct tee_ioctl_shm_register_fd_data data;
+	struct tee_shm *shm;
+	long ret;
+
+	if (copy_from_user(&data, udata, sizeof(data)))
+		return -EFAULT;
+
+	/* Currently no input flags are supported */
+	if (data.flags)
+		return -EINVAL;
+
+	shm = tee_shm_register_fd(ctx, data.fd);
+	if (IS_ERR(shm))
+		return -EINVAL;
+
+	data.id = shm->id;
+	data.flags = shm->flags;
+	data.size = shm->size;
+
+	if (copy_to_user(udata, &data, sizeof(data)))
+		ret = -EFAULT;
+	else
+		ret = tee_shm_get_fd(shm);
+
+	/*
+	 * When user space closes the file descriptor the shared memory
+	 * should be freed or if tee_shm_get_fd() failed then it will
+	 * be freed immediately.
+	 */
+	tee_shm_put(shm);
+	return ret;
+}
+
 static int params_from_user(struct tee_context *ctx, struct tee_param *params,
 			    size_t num_params,
 			    struct tee_ioctl_param __user *uparams)
@@ -830,6 +866,8 @@ static long tee_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 		return tee_ioctl_shm_alloc(ctx, uarg);
 	case TEE_IOC_SHM_REGISTER:
 		return tee_ioctl_shm_register(ctx, uarg);
+	case TEE_IOC_SHM_REGISTER_FD:
+		return tee_ioctl_shm_register_fd(ctx, uarg);
 	case TEE_IOC_OPEN_SESSION:
 		return tee_ioctl_open_session(ctx, uarg);
 	case TEE_IOC_INVOKE:
diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c
index 731d9028b67f..a1cb3c8b6423 100644
--- a/drivers/tee/tee_shm.c
+++ b/drivers/tee/tee_shm.c
@@ -4,6 +4,7 @@
  */
 #include <linux/anon_inodes.h>
 #include <linux/device.h>
+#include <linux/dma-buf.h>
 #include <linux/idr.h>
 #include <linux/mm.h>
 #include <linux/sched.h>
@@ -14,6 +15,14 @@
 #include <linux/highmem.h>
 #include "tee_private.h"
 
+/* extra references appended to shm object for registered shared memory */
+struct tee_shm_dmabuf_ref {
+	struct tee_shm shm;
+	struct dma_buf *dmabuf;
+	struct dma_buf_attachment *attach;
+	struct sg_table *sgt;
+};
+
 static void shm_put_kernel_pages(struct page **pages, size_t page_count)
 {
 	size_t n;
@@ -44,7 +53,16 @@ static void release_registered_pages(struct tee_shm *shm)
 
 static void tee_shm_release(struct tee_device *teedev, struct tee_shm *shm)
 {
-	if (shm->flags & TEE_SHM_POOL) {
+	if (shm->flags & TEE_SHM_DMA_BUF) {
+		struct tee_shm_dmabuf_ref *ref;
+
+		ref = container_of(shm, struct tee_shm_dmabuf_ref, shm);
+		dma_buf_unmap_attachment(ref->attach, ref->sgt,
+		DMA_BIDIRECTIONAL);
+
+		dma_buf_detach(ref->dmabuf, ref->attach);
+		dma_buf_put(ref->dmabuf);
+	} else if (shm->flags & TEE_SHM_POOL) {
 		teedev->pool->ops->free(teedev->pool, shm);
 	} else if (shm->flags & TEE_SHM_DYNAMIC) {
 		int rc = teedev->desc->ops->shm_unregister(shm->ctx, shm);
@@ -56,7 +74,8 @@ static void tee_shm_release(struct tee_device *teedev, struct tee_shm *shm)
 		release_registered_pages(shm);
 	}
 
-	teedev_ctx_put(shm->ctx);
+	if (shm->ctx)
+		teedev_ctx_put(shm->ctx);
 
 	kfree(shm);
 
@@ -168,7 +187,7 @@ struct tee_shm *tee_shm_alloc_user_buf(struct tee_context *ctx, size_t size)
  * tee_client_invoke_func(). The memory allocated is later freed with a
  * call to tee_shm_free().
  *
- * @returns a pointer to 'struct tee_shm'
+ * @returns a pointer to 'struct tee_shm' on success, and ERR_PTR on failure
  */
 struct tee_shm *tee_shm_alloc_kernel_buf(struct tee_context *ctx, size_t size)
 {
@@ -178,6 +197,85 @@ struct tee_shm *tee_shm_alloc_kernel_buf(struct tee_context *ctx, size_t size)
 }
 EXPORT_SYMBOL_GPL(tee_shm_alloc_kernel_buf);
 
+struct tee_shm *tee_shm_register_fd(struct tee_context *ctx, int fd)
+{
+	struct tee_shm_dmabuf_ref *ref;
+	int rc;
+
+	if (!tee_device_get(ctx->teedev))
+		return ERR_PTR(-EINVAL);
+
+	teedev_ctx_get(ctx);
+
+	ref = kzalloc(sizeof(*ref), GFP_KERNEL);
+	if (!ref) {
+		rc = -ENOMEM;
+		goto err_put_tee;
+	}
+
+	refcount_set(&ref->shm.refcount, 1);
+	ref->shm.ctx = ctx;
+	ref->shm.id = -1;
+
+	ref->dmabuf = dma_buf_get(fd);
+	if (IS_ERR(ref->dmabuf)) {
+		rc = PTR_ERR(ref->dmabuf);
+		goto err_put_dmabuf;
+	}
+
+	ref->attach = dma_buf_attach(ref->dmabuf, &ref->shm.ctx->teedev->dev);
+	if (IS_ERR(ref->attach)) {
+		rc = PTR_ERR(ref->attach);
+		goto err_detach;
+	}
+
+	ref->sgt = dma_buf_map_attachment(ref->attach, DMA_BIDIRECTIONAL);
+	if (IS_ERR(ref->sgt)) {
+		rc = PTR_ERR(ref->sgt);
+		goto err_unmap_attachement;
+	}
+
+	if (sg_nents(ref->sgt->sgl) != 1) {
+		rc = PTR_ERR(ref->sgt->sgl);
+		goto err_unmap_attachement;
+	}
+
+	ref->shm.paddr = page_to_phys(sg_page(ref->sgt->sgl));
+	ref->shm.size = ref->sgt->sgl->length;
+	ref->shm.flags = TEE_SHM_DMA_BUF;
+
+	mutex_lock(&ref->shm.ctx->teedev->mutex);
+	ref->shm.id = idr_alloc(&ref->shm.ctx->teedev->idr, &ref->shm,
+				1, 0, GFP_KERNEL);
+	mutex_unlock(&ref->shm.ctx->teedev->mutex);
+	if (ref->shm.id < 0) {
+		rc = ref->shm.id;
+		goto err_idr_remove;
+	}
+
+	return &ref->shm;
+
+err_idr_remove:
+	mutex_lock(&ctx->teedev->mutex);
+	idr_remove(&ctx->teedev->idr, ref->shm.id);
+	mutex_unlock(&ctx->teedev->mutex);
+err_unmap_attachement:
+	dma_buf_unmap_attachment(ref->attach, ref->sgt, DMA_BIDIRECTIONAL);
+err_detach:
+	dma_buf_detach(ref->dmabuf, ref->attach);
+err_put_dmabuf:
+	dma_buf_put(ref->dmabuf);
+	kfree(ref);
+err_put_tee:
+	teedev_ctx_put(ctx);
+	tee_device_put(ctx->teedev);
+
+	return ERR_PTR(rc);
+}
+EXPORT_SYMBOL_GPL(tee_shm_register_fd);
+
+
+
 /**
  * tee_shm_alloc_priv_buf() - Allocate shared memory for a privately shared
  *			      kernel buffer
diff --git a/include/linux/tee_drv.h b/include/linux/tee_drv.h
index 71632e3c5f18..6a1fee689007 100644
--- a/include/linux/tee_drv.h
+++ b/include/linux/tee_drv.h
@@ -25,6 +25,7 @@
 #define TEE_SHM_USER_MAPPED	BIT(1)  /* Memory mapped in user space */
 #define TEE_SHM_POOL		BIT(2)  /* Memory allocated from pool */
 #define TEE_SHM_PRIV		BIT(3)  /* Memory private to TEE driver */
+#define TEE_SHM_DMA_BUF		BIT(4)	/* Memory with dma-buf handle */
 
 struct device;
 struct tee_device;
@@ -275,6 +276,16 @@ void *tee_get_drvdata(struct tee_device *teedev);
 struct tee_shm *tee_shm_alloc_priv_buf(struct tee_context *ctx, size_t size);
 struct tee_shm *tee_shm_alloc_kernel_buf(struct tee_context *ctx, size_t size);
 
+/**
+ * tee_shm_register_fd() - Register shared memory from file descriptor
+ *
+ * @ctx:	Context that allocates the shared memory
+ * @fd:		Shared memory file descriptor reference
+ *
+ * @returns a pointer to 'struct tee_shm' on success, and ERR_PTR on failure
+ */
+struct tee_shm *tee_shm_register_fd(struct tee_context *ctx, int fd);
+
 struct tee_shm *tee_shm_register_kernel_buf(struct tee_context *ctx,
 					    void *addr, size_t length);
 
diff --git a/include/uapi/linux/tee.h b/include/uapi/linux/tee.h
index 23e57164693c..77bc8ef24d3c 100644
--- a/include/uapi/linux/tee.h
+++ b/include/uapi/linux/tee.h
@@ -117,6 +117,35 @@ struct tee_ioctl_shm_alloc_data {
 #define TEE_IOC_SHM_ALLOC	_IOWR(TEE_IOC_MAGIC, TEE_IOC_BASE + 1, \
 				     struct tee_ioctl_shm_alloc_data)
 
+/**
+ * struct tee_ioctl_shm_register_fd_data - Shared memory registering argument
+ * @fd:		[in] File descriptor identifying the shared memory
+ * @size:	[out] Size of shared memory to allocate
+ * @flags:	[in] Flags to/from allocation.
+ * @id:		[out] Identifier of the shared memory
+ *
+ * The flags field should currently be zero as input. Updated by the call
+ * with actual flags as defined by TEE_IOCTL_SHM_* above.
+ * This structure is used as argument for TEE_IOC_SHM_REGISTER_FD below.
+ */
+struct tee_ioctl_shm_register_fd_data {
+	__s64 fd;
+	__u64 size;
+	__u32 flags;
+	__s32 id;
+} __aligned(8);
+
+/**
+ * TEE_IOC_SHM_REGISTER_FD - register a shared memory from a file descriptor
+ *
+ * Returns a file descriptor on success or < 0 on failure
+ *
+ * The returned file descriptor refers to the shared memory object in kernel
+ * land. The shared memory is freed when the descriptor is closed.
+ */
+#define TEE_IOC_SHM_REGISTER_FD	_IOWR(TEE_IOC_MAGIC, TEE_IOC_BASE + 8, \
+				     struct tee_ioctl_shm_register_fd_data)
+
 /**
  * struct tee_ioctl_buf_data - Variable sized buffer
  * @buf_ptr:	[in] A __user pointer to a buffer
-- 
2.34.1

[RFC PATCH 3/4] dt-bindings: reserved-memory: add linaro,restricted-heap

[Jens Wiklander]

From: Olivier Masse <olivier.masse@nxp.com>

DMABUF reserved memory definition for OP-TEE secure data path feature.

Signed-off-by: Olivier Masse <olivier.masse@nxp.com>
Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
---
 .../linaro,restricted-heap.yaml               | 56 +++++++++++++++++++
 1 file changed, 56 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/reserved-memory/linaro,restricted-heap.yaml

diff --git a/Documentation/devicetree/bindings/reserved-memory/linaro,restricted-heap.yaml b/Documentation/devicetree/bindings/reserved-memory/linaro,restricted-heap.yaml
new file mode 100644
index 000000000000..0ab87cf02775
--- /dev/null
+++ b/Documentation/devicetree/bindings/reserved-memory/linaro,restricted-heap.yaml
@@ -0,0 +1,56 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/reserved-memory/linaro,restricted-heap.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Linaro Secure DMABUF Heap
+
+maintainers:
+  - Olivier Masse <olivier.masse@nxp.com>
+
+description:
+  Linaro OP-TEE firmware needs a reserved memory for the
+  Secure Data Path feature (aka SDP).
+  The purpose is to provide a restricted memory heap which allow
+  the normal world OS (REE) to allocate/free restricted buffers.
+  The TEE is reponsible for protecting the SDP memory buffers.
+  TEE Trusted Application can access restricted memory references
+  provided as parameters (DMABUF file descriptor).
+
+allOf:
+  - $ref: "reserved-memory.yaml"
+
+properties:
+  compatible:
+    const: linaro,restricted-heap
+
+  reg:
+    description:
+      Region of memory reserved for OP-TEE SDP feature
+
+  no-map:
+    $ref: /schemas/types.yaml#/definitions/flag
+    description:
+      Avoid creating a virtual mapping of the region as part of the OS'
+      standard mapping of system memory.
+
+unevaluatedProperties: false
+
+required:
+  - compatible
+  - reg
+  - no-map
+
+examples:
+  - |
+  reserved-memory {
+    #address-cells = <2>;
+    #size-cells = <2>;
+
+    sdp@3e800000 {
+      compatible = "linaro,restricted-heap";
+      no-map;
+      reg = <0 0x3E800000 0 0x00400000>;
+    };
+  };
-- 
2.34.1

[RFC PATCH 4/4] dma-buf: heaps: add Linaro restricted dmabuf heap support

[Jens Wiklander]

Add a Linaro restricted heap using the linaro,restricted-heap bindings
implemented based on the generic restricted heap.

The bindings defines a range of physical restricted memory. The heap
manages this address range using genalloc. The allocated dma-buf file
descriptor can later be registered with the TEE subsystem for later use
via Trusted Applications in the secure world.

Co-developed-by: Olivier Masse <olivier.masse@nxp.com>
Signed-off-by: Olivier Masse <olivier.masse@nxp.com>
Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
---
 drivers/dma-buf/heaps/Kconfig                 |  10 ++
 drivers/dma-buf/heaps/Makefile                |   1 +
 .../dma-buf/heaps/restricted_heap_linaro.c    | 165 ++++++++++++++++++
 3 files changed, 176 insertions(+)
 create mode 100644 drivers/dma-buf/heaps/restricted_heap_linaro.c

diff --git a/drivers/dma-buf/heaps/Kconfig b/drivers/dma-buf/heaps/Kconfig
index 58903bc62ac8..82e2c5d09242 100644
--- a/drivers/dma-buf/heaps/Kconfig
+++ b/drivers/dma-buf/heaps/Kconfig
@@ -28,3 +28,13 @@ config DMABUF_HEAPS_RESTRICTED_MTK
 	help
 	  Enable restricted dma-buf heaps for MediaTek platform. This heap is backed by
 	  TEE client interfaces. If in doubt, say N.
+
+config DMABUF_HEAPS_RESTRICTED_LINARO
+	bool "Linaro DMA-BUF Restricted Heap"
+	depends on DMABUF_HEAPS_RESTRICTED
+	help
+	  Choose this option to enable the Linaro restricted dma-buf heap.
+	  The restricted heap pools are defined according to the DT. Heaps
+	  are allocated in the pools using gen allocater.
+	  If in doubt, say N.
+
diff --git a/drivers/dma-buf/heaps/Makefile b/drivers/dma-buf/heaps/Makefile
index 0028aa9d875f..66b2f67c47b5 100644
--- a/drivers/dma-buf/heaps/Makefile
+++ b/drivers/dma-buf/heaps/Makefile
@@ -2,4 +2,5 @@
 obj-$(CONFIG_DMABUF_HEAPS_CMA)		+= cma_heap.o
 obj-$(CONFIG_DMABUF_HEAPS_RESTRICTED)	+= restricted_heap.o
 obj-$(CONFIG_DMABUF_HEAPS_RESTRICTED_MTK)	+= restricted_heap_mtk.o
+obj-$(CONFIG_DMABUF_HEAPS_RESTRICTED_LINARO)	+= restricted_heap_linaro.o
 obj-$(CONFIG_DMABUF_HEAPS_SYSTEM)	+= system_heap.o
diff --git a/drivers/dma-buf/heaps/restricted_heap_linaro.c b/drivers/dma-buf/heaps/restricted_heap_linaro.c
new file mode 100644
index 000000000000..4b08ed514023
--- /dev/null
+++ b/drivers/dma-buf/heaps/restricted_heap_linaro.c
@@ -0,0 +1,165 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * DMABUF secure heap exporter
+ *
+ * Copyright 2021 NXP.
+ * Copyright 2024 Linaro Limited.
+ */
+
+#define pr_fmt(fmt)     "rheap_linaro: " fmt
+
+#include <linux/dma-buf.h>
+#include <linux/err.h>
+#include <linux/genalloc.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_fdt.h>
+#include <linux/of_reserved_mem.h>
+#include <linux/scatterlist.h>
+#include <linux/slab.h>
+
+#include "restricted_heap.h"
+
+#define MAX_HEAP_COUNT	2
+#define HEAP_NAME_LEN	32
+
+struct resmem_restricted {
+	phys_addr_t base;
+	phys_addr_t size;
+
+	char name[HEAP_NAME_LEN];
+
+	bool no_map;
+};
+
+static struct resmem_restricted restricted_data[MAX_HEAP_COUNT] = {0};
+static unsigned int restricted_data_count;
+
+static int linaro_restricted_memory_allocate(struct restricted_heap *heap,
+					     struct restricted_buffer *buf)
+{
+	struct gen_pool *pool = heap->priv_data;
+	unsigned long pa;
+	int ret;
+
+	buf->size = ALIGN(buf->size, PAGE_SIZE);
+	pa = gen_pool_alloc(pool, buf->size);
+	if (!pa)
+		return -ENOMEM;
+
+	ret = sg_alloc_table(&buf->sg_table, 1, GFP_KERNEL);
+	if (ret) {
+		gen_pool_free(pool, pa, buf->size);
+		return ret;
+	}
+
+	sg_set_page(buf->sg_table.sgl, phys_to_page(pa), buf->size, 0);
+
+	return 0;
+}
+
+static void linaro_restricted_memory_free(struct restricted_heap *heap,
+					  struct restricted_buffer *buf)
+{
+	struct gen_pool *pool = heap->priv_data;
+	struct scatterlist *sg;
+	unsigned int i;
+
+	for_each_sg(buf->sg_table.sgl, sg, buf->sg_table.nents, i)
+		gen_pool_free(pool, page_to_phys(sg_page(sg)), sg->length);
+	sg_free_table(&buf->sg_table);
+}
+
+static const struct restricted_heap_ops linaro_restricted_heap_ops = {
+	.alloc = linaro_restricted_memory_allocate,
+	.free = linaro_restricted_memory_free,
+};
+
+static int add_heap(struct resmem_restricted *mem)
+{
+	struct restricted_heap *heap;
+	struct gen_pool *pool;
+	int ret;
+
+	if (mem->base == 0 || mem->size == 0) {
+		pr_err("restricted_data base or size is not correct\n");
+		return -EINVAL;
+	}
+
+	heap = kzalloc(sizeof(*heap), GFP_KERNEL);
+	if (!heap)
+		return -ENOMEM;
+
+	pool = gen_pool_create(PAGE_SHIFT, -1);
+	if (!pool) {
+		ret = -ENOMEM;
+		goto err_free_heap;
+	}
+
+	ret = gen_pool_add(pool, mem->base, mem->size, -1);
+	if (ret)
+		goto err_free_pool;
+
+	heap->no_map = mem->no_map;
+	heap->priv_data = pool;
+	heap->name = mem->name;
+	heap->ops = &linaro_restricted_heap_ops;
+
+	ret = restricted_heap_add(heap);
+	if (ret)
+		goto err_free_pool;
+
+	return 0;
+
+err_free_pool:
+	gen_pool_destroy(pool);
+err_free_heap:
+	kfree(heap);
+
+	return ret;
+}
+
+static int __init rmem_restricted_heap_setup(struct reserved_mem *rmem)
+{
+	size_t len = HEAP_NAME_LEN;
+	const char *s;
+	bool no_map;
+
+	if (WARN_ONCE(restricted_data_count >= MAX_HEAP_COUNT,
+		      "Cannot handle more than %u restricted heaps\n",
+		      MAX_HEAP_COUNT))
+		return -EINVAL;
+
+	no_map = of_get_flat_dt_prop(rmem->fdt_node, "no-map", NULL);
+	s = strchr(rmem->name, '@');
+	if (s)
+		len = umin(s - rmem->name + 1, len);
+
+	restricted_data[restricted_data_count].base = rmem->base;
+	restricted_data[restricted_data_count].size = rmem->size;
+	restricted_data[restricted_data_count].no_map = no_map;
+	strscpy(restricted_data[restricted_data_count].name, rmem->name, len);
+
+	restricted_data_count++;
+	return 0;
+}
+
+RESERVEDMEM_OF_DECLARE(linaro_restricted_heap, "linaro,restricted-heap",
+		       rmem_restricted_heap_setup);
+
+static int linaro_restricted_heap_init(void)
+{
+	unsigned int i;
+	int ret;
+
+	for (i = 0; i < restricted_data_count; i++) {
+		ret = add_heap(&restricted_data[i]);
+		if (ret)
+			return ret;
+	}
+	return 0;
+}
+
+module_init(linaro_restricted_heap_init);
+MODULE_DESCRIPTION("Linaro Restricted Heap Driver");
+MODULE_LICENSE("GPL");
-- 
2.34.1

Re: [RFC PATCH 3/4] dt-bindings: reserved-memory: add linaro,restricted-heap

[Krzysztof Kozlowski]

On Fri, Aug 30, 2024 at 09:03:50AM +0200, Jens Wiklander wrote:
> From: Olivier Masse <olivier.masse@nxp.com>
> 
> DMABUF reserved memory definition for OP-TEE secure data path feature.
> 
> Signed-off-by: Olivier Masse <olivier.masse@nxp.com>
> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> ---
>  .../linaro,restricted-heap.yaml               | 56 +++++++++++++++++++
>  1 file changed, 56 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/reserved-memory/linaro,restricted-heap.yaml
> 
> diff --git a/Documentation/devicetree/bindings/reserved-memory/linaro,restricted-heap.yaml b/Documentation/devicetree/bindings/reserved-memory/linaro,restricted-heap.yaml
> new file mode 100644
> index 000000000000..0ab87cf02775
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/reserved-memory/linaro,restricted-heap.yaml
> @@ -0,0 +1,56 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/reserved-memory/linaro,restricted-heap.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Linaro Secure DMABUF Heap
> +
> +maintainers:
> +  - Olivier Masse <olivier.masse@nxp.com>
> +
> +description:
> +  Linaro OP-TEE firmware needs a reserved memory for the
> +  Secure Data Path feature (aka SDP).
> +  The purpose is to provide a restricted memory heap which allow
> +  the normal world OS (REE) to allocate/free restricted buffers.
> +  The TEE is reponsible for protecting the SDP memory buffers.
> +  TEE Trusted Application can access restricted memory references
> +  provided as parameters (DMABUF file descriptor).

And what is the difference from regular reserved memory? Why it cannot
be used?

> +
> +allOf:
> +  - $ref: "reserved-memory.yaml"

It does not look like you tested the bindings, at least after quick
look. Please run  (see
Documentation/devicetree/bindings/writing-schema.rst for instructions).
Maybe you need to update your dtschema and yamllint.

> +
> +properties:
> +  compatible:
> +    const: linaro,restricted-heap
> +
> +  reg:
> +    description:
> +      Region of memory reserved for OP-TEE SDP feature
> +
> +  no-map:
> +    $ref: /schemas/types.yaml#/definitions/flag
> +    description:
> +      Avoid creating a virtual mapping of the region as part of the OS'
> +      standard mapping of system memory.
> +
> +unevaluatedProperties: false

This goes after "required:" block.

> +
> +required:
> +  - compatible
> +  - reg
> +  - no-map
> +
> +examples:
> +  - |
> +  reserved-memory {
> +    #address-cells = <2>;
> +    #size-cells = <2>;
> +
> +    sdp@3e800000 {
> +      compatible = "linaro,restricted-heap";
> +      no-map;
> +      reg = <0 0x3E800000 0 0x00400000>;

lowercase hex

Best regards,
Krzysztof

Re: [RFC PATCH 3/4] dt-bindings: reserved-memory: add linaro,restricted-heap

[Jens Wiklander]

On Fri, Aug 30, 2024 at 10:20 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On Fri, Aug 30, 2024 at 09:03:50AM +0200, Jens Wiklander wrote:
> > From: Olivier Masse <olivier.masse@nxp.com>
> >
> > DMABUF reserved memory definition for OP-TEE secure data path feature.
> >
> > Signed-off-by: Olivier Masse <olivier.masse@nxp.com>
> > Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> > ---
> >  .../linaro,restricted-heap.yaml               | 56 +++++++++++++++++++
> >  1 file changed, 56 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/reserved-memory/linaro,restricted-heap.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/reserved-memory/linaro,restricted-heap.yaml b/Documentation/devicetree/bindings/reserved-memory/linaro,restricted-heap.yaml
> > new file mode 100644
> > index 000000000000..0ab87cf02775
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/reserved-memory/linaro,restricted-heap.yaml
> > @@ -0,0 +1,56 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/reserved-memory/linaro,restricted-heap.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Linaro Secure DMABUF Heap
> > +
> > +maintainers:
> > +  - Olivier Masse <olivier.masse@nxp.com>
> > +
> > +description:
> > +  Linaro OP-TEE firmware needs a reserved memory for the
> > +  Secure Data Path feature (aka SDP).
> > +  The purpose is to provide a restricted memory heap which allow
> > +  the normal world OS (REE) to allocate/free restricted buffers.
> > +  The TEE is reponsible for protecting the SDP memory buffers.
> > +  TEE Trusted Application can access restricted memory references
> > +  provided as parameters (DMABUF file descriptor).
>
> And what is the difference from regular reserved memory? Why it cannot
> be used?

Good question. I need a compatible = "linaro,restricted-heap" to find
it, but it appears that's permitted with regular reserved memory.
Let's drop this patch. Thanks for pointing me in the right direction.

>
> > +
> > +allOf:
> > +  - $ref: "reserved-memory.yaml"
>
> It does not look like you tested the bindings, at least after quick
> look. Please run  (see
> Documentation/devicetree/bindings/writing-schema.rst for instructions).
> Maybe you need to update your dtschema and yamllint.

You're right, sorry.

>
> > +
> > +properties:
> > +  compatible:
> > +    const: linaro,restricted-heap
> > +
> > +  reg:
> > +    description:
> > +      Region of memory reserved for OP-TEE SDP feature
> > +
> > +  no-map:
> > +    $ref: /schemas/types.yaml#/definitions/flag
> > +    description:
> > +      Avoid creating a virtual mapping of the region as part of the OS'
> > +      standard mapping of system memory.
> > +
> > +unevaluatedProperties: false
>
> This goes after "required:" block.

OK

>
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - no-map
> > +
> > +examples:
> > +  - |
> > +  reserved-memory {
> > +    #address-cells = <2>;
> > +    #size-cells = <2>;
> > +
> > +    sdp@3e800000 {
> > +      compatible = "linaro,restricted-heap";
> > +      no-map;
> > +      reg = <0 0x3E800000 0 0x00400000>;
>
> lowercase hex
>

OK


Thanks,
Jens

> Best regards,
> Krzysztof
>

Re: [RFC PATCH 1/4] dma-buf: heaps: restricted_heap: add no_map attribute

[Christian König]

Am 30.08.24 um 09:03 schrieb Jens Wiklander:
> Add a no_map attribute to struct restricted_heap_attachment and struct
> restricted_heap to skip the call to dma_map_sgtable() if set. This
> avoids trying to map a dma-buf that doens't refer to memory accessible
> by the kernel.

You seem to have a misunderstanding here dma_map_sgtable() is called to 
map a table into IOMMU and not any kernel address space.

So please explain why you need that?

Regards,
Christian.

>
> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> ---
>   drivers/dma-buf/heaps/restricted_heap.c | 17 +++++++++++++----
>   drivers/dma-buf/heaps/restricted_heap.h |  2 ++
>   2 files changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/dma-buf/heaps/restricted_heap.c b/drivers/dma-buf/heaps/restricted_heap.c
> index 8bc8a5e3f969..4bf28e3727ca 100644
> --- a/drivers/dma-buf/heaps/restricted_heap.c
> +++ b/drivers/dma-buf/heaps/restricted_heap.c
> @@ -16,6 +16,7 @@
>   struct restricted_heap_attachment {
>   	struct sg_table			*table;
>   	struct device			*dev;
> +	bool no_map;
>   };
>   
>   static int
> @@ -54,6 +55,8 @@ restricted_heap_memory_free(struct restricted_heap *rheap, struct restricted_buf
>   static int restricted_heap_attach(struct dma_buf *dmabuf, struct dma_buf_attachment *attachment)
>   {
>   	struct restricted_buffer *restricted_buf = dmabuf->priv;
> +	struct dma_heap *heap = restricted_buf->heap;
> +	struct restricted_heap *rheap = dma_heap_get_drvdata(heap);
>   	struct restricted_heap_attachment *a;
>   	struct sg_table *table;
>   
> @@ -70,6 +73,7 @@ static int restricted_heap_attach(struct dma_buf *dmabuf, struct dma_buf_attachm
>   	sg_dma_mark_restricted(table->sgl);
>   	a->table = table;
>   	a->dev = attachment->dev;
> +	a->no_map = rheap->no_map;
>   	attachment->priv = a;
>   
>   	return 0;
> @@ -92,9 +96,12 @@ restricted_heap_map_dma_buf(struct dma_buf_attachment *attachment,
>   	struct sg_table *table = a->table;
>   	int ret;
>   
> -	ret = dma_map_sgtable(attachment->dev, table, direction, DMA_ATTR_SKIP_CPU_SYNC);
> -	if (ret)
> -		return ERR_PTR(ret);
> +	if (!a->no_map) {
> +		ret = dma_map_sgtable(attachment->dev, table, direction,
> +				      DMA_ATTR_SKIP_CPU_SYNC);
> +		if (ret)
> +			return ERR_PTR(ret);
> +	}
>   	return table;
>   }
>   
> @@ -106,7 +113,9 @@ restricted_heap_unmap_dma_buf(struct dma_buf_attachment *attachment, struct sg_t
>   
>   	WARN_ON(a->table != table);
>   
> -	dma_unmap_sgtable(attachment->dev, table, direction, DMA_ATTR_SKIP_CPU_SYNC);
> +	if (!a->no_map)
> +		dma_unmap_sgtable(attachment->dev, table, direction,
> +				  DMA_ATTR_SKIP_CPU_SYNC);
>   }
>   
>   static int
> diff --git a/drivers/dma-buf/heaps/restricted_heap.h b/drivers/dma-buf/heaps/restricted_heap.h
> index 7dec4b8a471b..94cc0842f70d 100644
> --- a/drivers/dma-buf/heaps/restricted_heap.h
> +++ b/drivers/dma-buf/heaps/restricted_heap.h
> @@ -27,6 +27,8 @@ struct restricted_heap {
>   	unsigned long		cma_paddr;
>   	unsigned long		cma_size;
>   
> +	bool			no_map;
> +
>   	void			*priv_data;
>   };
>   

Re: [RFC PATCH 2/4] tee: new ioctl to a register tee_shm from a dmabuf file descriptor

[T.J. Mercier]

On Fri, Aug 30, 2024 at 12:04 AM Jens Wiklander
<jens.wiklander@linaro.org> wrote:
>
> From: Etienne Carriere <etienne.carriere@linaro.org>
>
> Enable userspace to create a tee_shm object that refers to a dmabuf
> reference.
>
> Userspace registers the dmabuf file descriptor as in a tee_shm object.
> The registration is completed with a tee_shm file descriptor returned to
> userspace.
>
> Userspace is free to close the dmabuf file descriptor now since all the
> resources are now held via the tee_shm object.
>
> Closing the tee_shm file descriptor will release all resources used by the
> tee_shm object.
>
> This change only support dmabuf references that relates to physically
> contiguous memory buffers.
>
> New tee_shm flag to identify tee_shm objects built from a registered
> dmabuf, TEE_SHM_DMA_BUF.
>
> Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
> Signed-off-by: Olivier Masse <olivier.masse@nxp.com>
> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> ---
>  drivers/tee/tee_core.c   |  38 ++++++++++++++
>  drivers/tee/tee_shm.c    | 104 +++++++++++++++++++++++++++++++++++++--
>  include/linux/tee_drv.h  |  11 +++++
>  include/uapi/linux/tee.h |  29 +++++++++++
>  4 files changed, 179 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/tee/tee_core.c b/drivers/tee/tee_core.c
> index e59c20d74b36..3dfd5428d58c 100644
> --- a/drivers/tee/tee_core.c
> +++ b/drivers/tee/tee_core.c
> @@ -356,6 +356,42 @@ tee_ioctl_shm_register(struct tee_context *ctx,
>         return ret;
>  }
>
> +static int tee_ioctl_shm_register_fd(struct tee_context *ctx,
> +                                    struct tee_ioctl_shm_register_fd_data __user *udata)
> +{
> +       struct tee_ioctl_shm_register_fd_data data;
> +       struct tee_shm *shm;
> +       long ret;
> +
> +       if (copy_from_user(&data, udata, sizeof(data)))
> +               return -EFAULT;
> +
> +       /* Currently no input flags are supported */
> +       if (data.flags)
> +               return -EINVAL;
> +
> +       shm = tee_shm_register_fd(ctx, data.fd);
> +       if (IS_ERR(shm))
> +               return -EINVAL;
> +
> +       data.id = shm->id;
> +       data.flags = shm->flags;
> +       data.size = shm->size;
> +
> +       if (copy_to_user(udata, &data, sizeof(data)))
> +               ret = -EFAULT;
> +       else
> +               ret = tee_shm_get_fd(shm);
> +
> +       /*
> +        * When user space closes the file descriptor the shared memory
> +        * should be freed or if tee_shm_get_fd() failed then it will
> +        * be freed immediately.
> +        */
> +       tee_shm_put(shm);
> +       return ret;
> +}
> +
>  static int params_from_user(struct tee_context *ctx, struct tee_param *params,
>                             size_t num_params,
>                             struct tee_ioctl_param __user *uparams)
> @@ -830,6 +866,8 @@ static long tee_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>                 return tee_ioctl_shm_alloc(ctx, uarg);
>         case TEE_IOC_SHM_REGISTER:
>                 return tee_ioctl_shm_register(ctx, uarg);
> +       case TEE_IOC_SHM_REGISTER_FD:
> +               return tee_ioctl_shm_register_fd(ctx, uarg);
>         case TEE_IOC_OPEN_SESSION:
>                 return tee_ioctl_open_session(ctx, uarg);
>         case TEE_IOC_INVOKE:
> diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c
> index 731d9028b67f..a1cb3c8b6423 100644
> --- a/drivers/tee/tee_shm.c
> +++ b/drivers/tee/tee_shm.c
> @@ -4,6 +4,7 @@
>   */
>  #include <linux/anon_inodes.h>
>  #include <linux/device.h>
> +#include <linux/dma-buf.h>
>  #include <linux/idr.h>
>  #include <linux/mm.h>
>  #include <linux/sched.h>
> @@ -14,6 +15,14 @@
>  #include <linux/highmem.h>
>  #include "tee_private.h"
>
> +/* extra references appended to shm object for registered shared memory */
> +struct tee_shm_dmabuf_ref {
> +       struct tee_shm shm;
> +       struct dma_buf *dmabuf;
> +       struct dma_buf_attachment *attach;
> +       struct sg_table *sgt;
> +};
> +
>  static void shm_put_kernel_pages(struct page **pages, size_t page_count)
>  {
>         size_t n;
> @@ -44,7 +53,16 @@ static void release_registered_pages(struct tee_shm *shm)
>
>  static void tee_shm_release(struct tee_device *teedev, struct tee_shm *shm)
>  {
> -       if (shm->flags & TEE_SHM_POOL) {
> +       if (shm->flags & TEE_SHM_DMA_BUF) {
> +               struct tee_shm_dmabuf_ref *ref;
> +
> +               ref = container_of(shm, struct tee_shm_dmabuf_ref, shm);
> +               dma_buf_unmap_attachment(ref->attach, ref->sgt,
> +               DMA_BIDIRECTIONAL);
> +
> +               dma_buf_detach(ref->dmabuf, ref->attach);
> +               dma_buf_put(ref->dmabuf);
> +       } else if (shm->flags & TEE_SHM_POOL) {
>                 teedev->pool->ops->free(teedev->pool, shm);
>         } else if (shm->flags & TEE_SHM_DYNAMIC) {
>                 int rc = teedev->desc->ops->shm_unregister(shm->ctx, shm);
> @@ -56,7 +74,8 @@ static void tee_shm_release(struct tee_device *teedev, struct tee_shm *shm)
>                 release_registered_pages(shm);
>         }
>
> -       teedev_ctx_put(shm->ctx);
> +       if (shm->ctx)
> +               teedev_ctx_put(shm->ctx);
>
>         kfree(shm);
>
> @@ -168,7 +187,7 @@ struct tee_shm *tee_shm_alloc_user_buf(struct tee_context *ctx, size_t size)
>   * tee_client_invoke_func(). The memory allocated is later freed with a
>   * call to tee_shm_free().
>   *
> - * @returns a pointer to 'struct tee_shm'
> + * @returns a pointer to 'struct tee_shm' on success, and ERR_PTR on failure
>   */
>  struct tee_shm *tee_shm_alloc_kernel_buf(struct tee_context *ctx, size_t size)
>  {
> @@ -178,6 +197,85 @@ struct tee_shm *tee_shm_alloc_kernel_buf(struct tee_context *ctx, size_t size)
>  }
>  EXPORT_SYMBOL_GPL(tee_shm_alloc_kernel_buf);
>
> +struct tee_shm *tee_shm_register_fd(struct tee_context *ctx, int fd)
> +{
> +       struct tee_shm_dmabuf_ref *ref;
> +       int rc;
> +
> +       if (!tee_device_get(ctx->teedev))
> +               return ERR_PTR(-EINVAL);
> +
> +       teedev_ctx_get(ctx);
> +
> +       ref = kzalloc(sizeof(*ref), GFP_KERNEL);
> +       if (!ref) {
> +               rc = -ENOMEM;
> +               goto err_put_tee;
> +       }
> +
> +       refcount_set(&ref->shm.refcount, 1);
> +       ref->shm.ctx = ctx;
> +       ref->shm.id = -1;
> +
> +       ref->dmabuf = dma_buf_get(fd);
> +       if (IS_ERR(ref->dmabuf)) {
> +               rc = PTR_ERR(ref->dmabuf);
> +               goto err_put_dmabuf;
> +       }

Hi,

Most of the gotos in the errors paths from here on look offset by one
to me. Attempting to put a dmabuf after failing to get, detaching
after failing to attach, unmapping after failing to map, removing an
IDR after failing to allocate one.


> +
> +       ref->attach = dma_buf_attach(ref->dmabuf, &ref->shm.ctx->teedev->dev);
> +       if (IS_ERR(ref->attach)) {
> +               rc = PTR_ERR(ref->attach);
> +               goto err_detach;
> +       }
> +
> +       ref->sgt = dma_buf_map_attachment(ref->attach, DMA_BIDIRECTIONAL);
> +       if (IS_ERR(ref->sgt)) {
> +               rc = PTR_ERR(ref->sgt);
> +               goto err_unmap_attachement;
> +       }
> +
> +       if (sg_nents(ref->sgt->sgl) != 1) {
> +               rc = PTR_ERR(ref->sgt->sgl);
> +               goto err_unmap_attachement;
> +       }
> +
> +       ref->shm.paddr = page_to_phys(sg_page(ref->sgt->sgl));
> +       ref->shm.size = ref->sgt->sgl->length;
> +       ref->shm.flags = TEE_SHM_DMA_BUF;
> +
> +       mutex_lock(&ref->shm.ctx->teedev->mutex);
> +       ref->shm.id = idr_alloc(&ref->shm.ctx->teedev->idr, &ref->shm,
> +                               1, 0, GFP_KERNEL);
> +       mutex_unlock(&ref->shm.ctx->teedev->mutex);
> +       if (ref->shm.id < 0) {
> +               rc = ref->shm.id;
> +               goto err_idr_remove;
> +       }
> +
> +       return &ref->shm;
> +
> +err_idr_remove:
> +       mutex_lock(&ctx->teedev->mutex);
> +       idr_remove(&ctx->teedev->idr, ref->shm.id);
> +       mutex_unlock(&ctx->teedev->mutex);
> +err_unmap_attachement:
> +       dma_buf_unmap_attachment(ref->attach, ref->sgt, DMA_BIDIRECTIONAL);
> +err_detach:
> +       dma_buf_detach(ref->dmabuf, ref->attach);
> +err_put_dmabuf:
> +       dma_buf_put(ref->dmabuf);
> +       kfree(ref);
> +err_put_tee:
> +       teedev_ctx_put(ctx);
> +       tee_device_put(ctx->teedev);
> +
> +       return ERR_PTR(rc);
> +}
> +EXPORT_SYMBOL_GPL(tee_shm_register_fd);
> +
> +
> +
>  /**
>   * tee_shm_alloc_priv_buf() - Allocate shared memory for a privately shared
>   *                           kernel buffer
> diff --git a/include/linux/tee_drv.h b/include/linux/tee_drv.h
> index 71632e3c5f18..6a1fee689007 100644
> --- a/include/linux/tee_drv.h
> +++ b/include/linux/tee_drv.h
> @@ -25,6 +25,7 @@
>  #define TEE_SHM_USER_MAPPED    BIT(1)  /* Memory mapped in user space */
>  #define TEE_SHM_POOL           BIT(2)  /* Memory allocated from pool */
>  #define TEE_SHM_PRIV           BIT(3)  /* Memory private to TEE driver */
> +#define TEE_SHM_DMA_BUF                BIT(4)  /* Memory with dma-buf handle */
>
>  struct device;
>  struct tee_device;
> @@ -275,6 +276,16 @@ void *tee_get_drvdata(struct tee_device *teedev);
>  struct tee_shm *tee_shm_alloc_priv_buf(struct tee_context *ctx, size_t size);
>  struct tee_shm *tee_shm_alloc_kernel_buf(struct tee_context *ctx, size_t size);
>
> +/**
> + * tee_shm_register_fd() - Register shared memory from file descriptor
> + *
> + * @ctx:       Context that allocates the shared memory
> + * @fd:                Shared memory file descriptor reference
> + *
> + * @returns a pointer to 'struct tee_shm' on success, and ERR_PTR on failure
> + */
> +struct tee_shm *tee_shm_register_fd(struct tee_context *ctx, int fd);
> +
>  struct tee_shm *tee_shm_register_kernel_buf(struct tee_context *ctx,
>                                             void *addr, size_t length);
>
> diff --git a/include/uapi/linux/tee.h b/include/uapi/linux/tee.h
> index 23e57164693c..77bc8ef24d3c 100644
> --- a/include/uapi/linux/tee.h
> +++ b/include/uapi/linux/tee.h
> @@ -117,6 +117,35 @@ struct tee_ioctl_shm_alloc_data {
>  #define TEE_IOC_SHM_ALLOC      _IOWR(TEE_IOC_MAGIC, TEE_IOC_BASE + 1, \
>                                      struct tee_ioctl_shm_alloc_data)
>
> +/**
> + * struct tee_ioctl_shm_register_fd_data - Shared memory registering argument
> + * @fd:                [in] File descriptor identifying the shared memory
> + * @size:      [out] Size of shared memory to allocate
> + * @flags:     [in] Flags to/from allocation.
> + * @id:                [out] Identifier of the shared memory
> + *
> + * The flags field should currently be zero as input. Updated by the call
> + * with actual flags as defined by TEE_IOCTL_SHM_* above.
> + * This structure is used as argument for TEE_IOC_SHM_REGISTER_FD below.
> + */
> +struct tee_ioctl_shm_register_fd_data {
> +       __s64 fd;
> +       __u64 size;
> +       __u32 flags;
> +       __s32 id;
> +} __aligned(8);
> +
> +/**
> + * TEE_IOC_SHM_REGISTER_FD - register a shared memory from a file descriptor
> + *
> + * Returns a file descriptor on success or < 0 on failure
> + *
> + * The returned file descriptor refers to the shared memory object in kernel
> + * land. The shared memory is freed when the descriptor is closed.
> + */
> +#define TEE_IOC_SHM_REGISTER_FD        _IOWR(TEE_IOC_MAGIC, TEE_IOC_BASE + 8, \
> +                                    struct tee_ioctl_shm_register_fd_data)
> +
>  /**
>   * struct tee_ioctl_buf_data - Variable sized buffer
>   * @buf_ptr:   [in] A __user pointer to a buffer
> --
> 2.34.1
>

Re: [RFC PATCH 4/4] dma-buf: heaps: add Linaro restricted dmabuf heap support

[T.J. Mercier]

On Fri, Aug 30, 2024 at 12:04 AM Jens Wiklander
<jens.wiklander@linaro.org> wrote:
>
> Add a Linaro restricted heap using the linaro,restricted-heap bindings
> implemented based on the generic restricted heap.
>
> The bindings defines a range of physical restricted memory. The heap
> manages this address range using genalloc. The allocated dma-buf file
> descriptor can later be registered with the TEE subsystem for later use
> via Trusted Applications in the secure world.
>
> Co-developed-by: Olivier Masse <olivier.masse@nxp.com>
> Signed-off-by: Olivier Masse <olivier.masse@nxp.com>
> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> ---
>  drivers/dma-buf/heaps/Kconfig                 |  10 ++
>  drivers/dma-buf/heaps/Makefile                |   1 +
>  .../dma-buf/heaps/restricted_heap_linaro.c    | 165 ++++++++++++++++++
>  3 files changed, 176 insertions(+)
>  create mode 100644 drivers/dma-buf/heaps/restricted_heap_linaro.c
>
> diff --git a/drivers/dma-buf/heaps/Kconfig b/drivers/dma-buf/heaps/Kconfig
> index 58903bc62ac8..82e2c5d09242 100644
> --- a/drivers/dma-buf/heaps/Kconfig
> +++ b/drivers/dma-buf/heaps/Kconfig
> @@ -28,3 +28,13 @@ config DMABUF_HEAPS_RESTRICTED_MTK
>         help
>           Enable restricted dma-buf heaps for MediaTek platform. This heap is backed by
>           TEE client interfaces. If in doubt, say N.
> +
> +config DMABUF_HEAPS_RESTRICTED_LINARO
> +       bool "Linaro DMA-BUF Restricted Heap"
> +       depends on DMABUF_HEAPS_RESTRICTED
> +       help
> +         Choose this option to enable the Linaro restricted dma-buf heap.
> +         The restricted heap pools are defined according to the DT. Heaps
> +         are allocated in the pools using gen allocater.
> +         If in doubt, say N.
> +
> diff --git a/drivers/dma-buf/heaps/Makefile b/drivers/dma-buf/heaps/Makefile
> index 0028aa9d875f..66b2f67c47b5 100644
> --- a/drivers/dma-buf/heaps/Makefile
> +++ b/drivers/dma-buf/heaps/Makefile
> @@ -2,4 +2,5 @@
>  obj-$(CONFIG_DMABUF_HEAPS_CMA)         += cma_heap.o
>  obj-$(CONFIG_DMABUF_HEAPS_RESTRICTED)  += restricted_heap.o
>  obj-$(CONFIG_DMABUF_HEAPS_RESTRICTED_MTK)      += restricted_heap_mtk.o
> +obj-$(CONFIG_DMABUF_HEAPS_RESTRICTED_LINARO)   += restricted_heap_linaro.o
>  obj-$(CONFIG_DMABUF_HEAPS_SYSTEM)      += system_heap.o
> diff --git a/drivers/dma-buf/heaps/restricted_heap_linaro.c b/drivers/dma-buf/heaps/restricted_heap_linaro.c
> new file mode 100644
> index 000000000000..4b08ed514023
> --- /dev/null
> +++ b/drivers/dma-buf/heaps/restricted_heap_linaro.c
> @@ -0,0 +1,165 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * DMABUF secure heap exporter
> + *
> + * Copyright 2021 NXP.
> + * Copyright 2024 Linaro Limited.
> + */
> +
> +#define pr_fmt(fmt)     "rheap_linaro: " fmt
> +
> +#include <linux/dma-buf.h>
> +#include <linux/err.h>
> +#include <linux/genalloc.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_fdt.h>
> +#include <linux/of_reserved_mem.h>
> +#include <linux/scatterlist.h>
> +#include <linux/slab.h>
> +
> +#include "restricted_heap.h"
> +
> +#define MAX_HEAP_COUNT 2

Are multiple supported because of what Cyrille mentioned here about permissions?
https://lore.kernel.org/lkml/DBBPR04MB7514E006455AEA407041E4F788709@DBBPR04MB7514.eurprd04.prod.outlook.com/

So this is just some arbitrary limit? I'd prefer to have some sort of
documentation about this.


> +#define HEAP_NAME_LEN  32
> +
> +struct resmem_restricted {
> +       phys_addr_t base;
> +       phys_addr_t size;
> +
> +       char name[HEAP_NAME_LEN];
> +
> +       bool no_map;
> +};
> +
> +static struct resmem_restricted restricted_data[MAX_HEAP_COUNT] = {0};
> +static unsigned int restricted_data_count;
> +
> +static int linaro_restricted_memory_allocate(struct restricted_heap *heap,
> +                                            struct restricted_buffer *buf)
> +{
> +       struct gen_pool *pool = heap->priv_data;
> +       unsigned long pa;
> +       int ret;
> +
> +       buf->size = ALIGN(buf->size, PAGE_SIZE);
> +       pa = gen_pool_alloc(pool, buf->size);
> +       if (!pa)
> +               return -ENOMEM;
> +
> +       ret = sg_alloc_table(&buf->sg_table, 1, GFP_KERNEL);
> +       if (ret) {
> +               gen_pool_free(pool, pa, buf->size);
> +               return ret;
> +       }
> +
> +       sg_set_page(buf->sg_table.sgl, phys_to_page(pa), buf->size, 0);
> +
> +       return 0;
> +}
> +
> +static void linaro_restricted_memory_free(struct restricted_heap *heap,
> +                                         struct restricted_buffer *buf)
> +{
> +       struct gen_pool *pool = heap->priv_data;
> +       struct scatterlist *sg;
> +       unsigned int i;
> +
> +       for_each_sg(buf->sg_table.sgl, sg, buf->sg_table.nents, i)
> +               gen_pool_free(pool, page_to_phys(sg_page(sg)), sg->length);
> +       sg_free_table(&buf->sg_table);
> +}
> +
> +static const struct restricted_heap_ops linaro_restricted_heap_ops = {
> +       .alloc = linaro_restricted_memory_allocate,
> +       .free = linaro_restricted_memory_free,
> +};
> +
> +static int add_heap(struct resmem_restricted *mem)
> +{
> +       struct restricted_heap *heap;
> +       struct gen_pool *pool;
> +       int ret;
> +
> +       if (mem->base == 0 || mem->size == 0) {
> +               pr_err("restricted_data base or size is not correct\n");
> +               return -EINVAL;
> +       }
> +
> +       heap = kzalloc(sizeof(*heap), GFP_KERNEL);
> +       if (!heap)
> +               return -ENOMEM;
> +
> +       pool = gen_pool_create(PAGE_SHIFT, -1);
> +       if (!pool) {
> +               ret = -ENOMEM;
> +               goto err_free_heap;
> +       }
> +
> +       ret = gen_pool_add(pool, mem->base, mem->size, -1);
> +       if (ret)
> +               goto err_free_pool;
> +
> +       heap->no_map = mem->no_map;
> +       heap->priv_data = pool;
> +       heap->name = mem->name;
> +       heap->ops = &linaro_restricted_heap_ops;
> +
> +       ret = restricted_heap_add(heap);
> +       if (ret)
> +               goto err_free_pool;
> +
> +       return 0;
> +
> +err_free_pool:
> +       gen_pool_destroy(pool);
> +err_free_heap:
> +       kfree(heap);
> +
> +       return ret;
> +}
> +
> +static int __init rmem_restricted_heap_setup(struct reserved_mem *rmem)
> +{
> +       size_t len = HEAP_NAME_LEN;
> +       const char *s;
> +       bool no_map;
> +
> +       if (WARN_ONCE(restricted_data_count >= MAX_HEAP_COUNT,
> +                     "Cannot handle more than %u restricted heaps\n",
> +                     MAX_HEAP_COUNT))
> +               return -EINVAL;
> +
> +       no_map = of_get_flat_dt_prop(rmem->fdt_node, "no-map", NULL);
> +       s = strchr(rmem->name, '@');
> +       if (s)
> +               len = umin(s - rmem->name + 1, len);
> +
> +       restricted_data[restricted_data_count].base = rmem->base;
> +       restricted_data[restricted_data_count].size = rmem->size;
> +       restricted_data[restricted_data_count].no_map = no_map;
> +       strscpy(restricted_data[restricted_data_count].name, rmem->name, len);
> +
> +       restricted_data_count++;
> +       return 0;
> +}
> +
> +RESERVEDMEM_OF_DECLARE(linaro_restricted_heap, "linaro,restricted-heap",
> +                      rmem_restricted_heap_setup);
> +
> +static int linaro_restricted_heap_init(void)
> +{
> +       unsigned int i;
> +       int ret;
> +
> +       for (i = 0; i < restricted_data_count; i++) {
> +               ret = add_heap(&restricted_data[i]);
> +               if (ret)
> +                       return ret;
> +       }
> +       return 0;
> +}
> +
> +module_init(linaro_restricted_heap_init);
> +MODULE_DESCRIPTION("Linaro Restricted Heap Driver");
> +MODULE_LICENSE("GPL");
> --
> 2.34.1
>

Re: [RFC PATCH 4/4] dma-buf: heaps: add Linaro restricted dmabuf heap support

[Jens Wiklander]

On Tue, Sep 3, 2024 at 7:50 PM T.J. Mercier <tjmercier@google.com> wrote:
>
> On Fri, Aug 30, 2024 at 12:04 AM Jens Wiklander
> <jens.wiklander@linaro.org> wrote:
> >
> > Add a Linaro restricted heap using the linaro,restricted-heap bindings
> > implemented based on the generic restricted heap.
> >
> > The bindings defines a range of physical restricted memory. The heap
> > manages this address range using genalloc. The allocated dma-buf file
> > descriptor can later be registered with the TEE subsystem for later use
> > via Trusted Applications in the secure world.
> >
> > Co-developed-by: Olivier Masse <olivier.masse@nxp.com>
> > Signed-off-by: Olivier Masse <olivier.masse@nxp.com>
> > Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> > ---
> >  drivers/dma-buf/heaps/Kconfig                 |  10 ++
> >  drivers/dma-buf/heaps/Makefile                |   1 +
> >  .../dma-buf/heaps/restricted_heap_linaro.c    | 165 ++++++++++++++++++
> >  3 files changed, 176 insertions(+)
> >  create mode 100644 drivers/dma-buf/heaps/restricted_heap_linaro.c
> >
> > diff --git a/drivers/dma-buf/heaps/Kconfig b/drivers/dma-buf/heaps/Kconfig
> > index 58903bc62ac8..82e2c5d09242 100644
> > --- a/drivers/dma-buf/heaps/Kconfig
> > +++ b/drivers/dma-buf/heaps/Kconfig
> > @@ -28,3 +28,13 @@ config DMABUF_HEAPS_RESTRICTED_MTK
> >         help
> >           Enable restricted dma-buf heaps for MediaTek platform. This heap is backed by
> >           TEE client interfaces. If in doubt, say N.
> > +
> > +config DMABUF_HEAPS_RESTRICTED_LINARO
> > +       bool "Linaro DMA-BUF Restricted Heap"
> > +       depends on DMABUF_HEAPS_RESTRICTED
> > +       help
> > +         Choose this option to enable the Linaro restricted dma-buf heap.
> > +         The restricted heap pools are defined according to the DT. Heaps
> > +         are allocated in the pools using gen allocater.
> > +         If in doubt, say N.
> > +
> > diff --git a/drivers/dma-buf/heaps/Makefile b/drivers/dma-buf/heaps/Makefile
> > index 0028aa9d875f..66b2f67c47b5 100644
> > --- a/drivers/dma-buf/heaps/Makefile
> > +++ b/drivers/dma-buf/heaps/Makefile
> > @@ -2,4 +2,5 @@
> >  obj-$(CONFIG_DMABUF_HEAPS_CMA)         += cma_heap.o
> >  obj-$(CONFIG_DMABUF_HEAPS_RESTRICTED)  += restricted_heap.o
> >  obj-$(CONFIG_DMABUF_HEAPS_RESTRICTED_MTK)      += restricted_heap_mtk.o
> > +obj-$(CONFIG_DMABUF_HEAPS_RESTRICTED_LINARO)   += restricted_heap_linaro.o
> >  obj-$(CONFIG_DMABUF_HEAPS_SYSTEM)      += system_heap.o
> > diff --git a/drivers/dma-buf/heaps/restricted_heap_linaro.c b/drivers/dma-buf/heaps/restricted_heap_linaro.c
> > new file mode 100644
> > index 000000000000..4b08ed514023
> > --- /dev/null
> > +++ b/drivers/dma-buf/heaps/restricted_heap_linaro.c
> > @@ -0,0 +1,165 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * DMABUF secure heap exporter
> > + *
> > + * Copyright 2021 NXP.
> > + * Copyright 2024 Linaro Limited.
> > + */
> > +
> > +#define pr_fmt(fmt)     "rheap_linaro: " fmt
> > +
> > +#include <linux/dma-buf.h>
> > +#include <linux/err.h>
> > +#include <linux/genalloc.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/of_fdt.h>
> > +#include <linux/of_reserved_mem.h>
> > +#include <linux/scatterlist.h>
> > +#include <linux/slab.h>
> > +
> > +#include "restricted_heap.h"
> > +
> > +#define MAX_HEAP_COUNT 2
>
> Are multiple supported because of what Cyrille mentioned here about permissions?
> https://lore.kernel.org/lkml/DBBPR04MB7514E006455AEA407041E4F788709@DBBPR04MB7514.eurprd04.prod.outlook.com/

Yes, I kept that as is.

>
> So this is just some arbitrary limit? I'd prefer to have some sort of
> documentation about this.

How about removing the limit and using dynamic allocation instead?

Thanks,
Jens

>
>
> > +#define HEAP_NAME_LEN  32
> > +
> > +struct resmem_restricted {
> > +       phys_addr_t base;
> > +       phys_addr_t size;
> > +
> > +       char name[HEAP_NAME_LEN];
> > +
> > +       bool no_map;
> > +};
> > +
> > +static struct resmem_restricted restricted_data[MAX_HEAP_COUNT] = {0};
> > +static unsigned int restricted_data_count;
> > +
> > +static int linaro_restricted_memory_allocate(struct restricted_heap *heap,
> > +                                            struct restricted_buffer *buf)
> > +{
> > +       struct gen_pool *pool = heap->priv_data;
> > +       unsigned long pa;
> > +       int ret;
> > +
> > +       buf->size = ALIGN(buf->size, PAGE_SIZE);
> > +       pa = gen_pool_alloc(pool, buf->size);
> > +       if (!pa)
> > +               return -ENOMEM;
> > +
> > +       ret = sg_alloc_table(&buf->sg_table, 1, GFP_KERNEL);
> > +       if (ret) {
> > +               gen_pool_free(pool, pa, buf->size);
> > +               return ret;
> > +       }
> > +
> > +       sg_set_page(buf->sg_table.sgl, phys_to_page(pa), buf->size, 0);
> > +
> > +       return 0;
> > +}
> > +
> > +static void linaro_restricted_memory_free(struct restricted_heap *heap,
> > +                                         struct restricted_buffer *buf)
> > +{
> > +       struct gen_pool *pool = heap->priv_data;
> > +       struct scatterlist *sg;
> > +       unsigned int i;
> > +
> > +       for_each_sg(buf->sg_table.sgl, sg, buf->sg_table.nents, i)
> > +               gen_pool_free(pool, page_to_phys(sg_page(sg)), sg->length);
> > +       sg_free_table(&buf->sg_table);
> > +}
> > +
> > +static const struct restricted_heap_ops linaro_restricted_heap_ops = {
> > +       .alloc = linaro_restricted_memory_allocate,
> > +       .free = linaro_restricted_memory_free,
> > +};
> > +
> > +static int add_heap(struct resmem_restricted *mem)
> > +{
> > +       struct restricted_heap *heap;
> > +       struct gen_pool *pool;
> > +       int ret;
> > +
> > +       if (mem->base == 0 || mem->size == 0) {
> > +               pr_err("restricted_data base or size is not correct\n");
> > +               return -EINVAL;
> > +       }
> > +
> > +       heap = kzalloc(sizeof(*heap), GFP_KERNEL);
> > +       if (!heap)
> > +               return -ENOMEM;
> > +
> > +       pool = gen_pool_create(PAGE_SHIFT, -1);
> > +       if (!pool) {
> > +               ret = -ENOMEM;
> > +               goto err_free_heap;
> > +       }
> > +
> > +       ret = gen_pool_add(pool, mem->base, mem->size, -1);
> > +       if (ret)
> > +               goto err_free_pool;
> > +
> > +       heap->no_map = mem->no_map;
> > +       heap->priv_data = pool;
> > +       heap->name = mem->name;
> > +       heap->ops = &linaro_restricted_heap_ops;
> > +
> > +       ret = restricted_heap_add(heap);
> > +       if (ret)
> > +               goto err_free_pool;
> > +
> > +       return 0;
> > +
> > +err_free_pool:
> > +       gen_pool_destroy(pool);
> > +err_free_heap:
> > +       kfree(heap);
> > +
> > +       return ret;
> > +}
> > +
> > +static int __init rmem_restricted_heap_setup(struct reserved_mem *rmem)
> > +{
> > +       size_t len = HEAP_NAME_LEN;
> > +       const char *s;
> > +       bool no_map;
> > +
> > +       if (WARN_ONCE(restricted_data_count >= MAX_HEAP_COUNT,
> > +                     "Cannot handle more than %u restricted heaps\n",
> > +                     MAX_HEAP_COUNT))
> > +               return -EINVAL;
> > +
> > +       no_map = of_get_flat_dt_prop(rmem->fdt_node, "no-map", NULL);
> > +       s = strchr(rmem->name, '@');
> > +       if (s)
> > +               len = umin(s - rmem->name + 1, len);
> > +
> > +       restricted_data[restricted_data_count].base = rmem->base;
> > +       restricted_data[restricted_data_count].size = rmem->size;
> > +       restricted_data[restricted_data_count].no_map = no_map;
> > +       strscpy(restricted_data[restricted_data_count].name, rmem->name, len);
> > +
> > +       restricted_data_count++;
> > +       return 0;
> > +}
> > +
> > +RESERVEDMEM_OF_DECLARE(linaro_restricted_heap, "linaro,restricted-heap",
> > +                      rmem_restricted_heap_setup);
> > +
> > +static int linaro_restricted_heap_init(void)
> > +{
> > +       unsigned int i;
> > +       int ret;
> > +
> > +       for (i = 0; i < restricted_data_count; i++) {
> > +               ret = add_heap(&restricted_data[i]);
> > +               if (ret)
> > +                       return ret;
> > +       }
> > +       return 0;
> > +}
> > +
> > +module_init(linaro_restricted_heap_init);
> > +MODULE_DESCRIPTION("Linaro Restricted Heap Driver");
> > +MODULE_LICENSE("GPL");
> > --
> > 2.34.1
> >

Re: [RFC PATCH 2/4] tee: new ioctl to a register tee_shm from a dmabuf file descriptor

[Jens Wiklander]

On Tue, Sep 3, 2024 at 7:50 PM T.J. Mercier <tjmercier@google.com> wrote:
>
> On Fri, Aug 30, 2024 at 12:04 AM Jens Wiklander
> <jens.wiklander@linaro.org> wrote:
> >
> > From: Etienne Carriere <etienne.carriere@linaro.org>
> >
> > Enable userspace to create a tee_shm object that refers to a dmabuf
> > reference.
> >
> > Userspace registers the dmabuf file descriptor as in a tee_shm object.
> > The registration is completed with a tee_shm file descriptor returned to
> > userspace.
> >
> > Userspace is free to close the dmabuf file descriptor now since all the
> > resources are now held via the tee_shm object.
> >
> > Closing the tee_shm file descriptor will release all resources used by the
> > tee_shm object.
> >
> > This change only support dmabuf references that relates to physically
> > contiguous memory buffers.
> >
> > New tee_shm flag to identify tee_shm objects built from a registered
> > dmabuf, TEE_SHM_DMA_BUF.
> >
> > Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
> > Signed-off-by: Olivier Masse <olivier.masse@nxp.com>
> > Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> > ---
> >  drivers/tee/tee_core.c   |  38 ++++++++++++++
> >  drivers/tee/tee_shm.c    | 104 +++++++++++++++++++++++++++++++++++++--
> >  include/linux/tee_drv.h  |  11 +++++
> >  include/uapi/linux/tee.h |  29 +++++++++++
> >  4 files changed, 179 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/tee/tee_core.c b/drivers/tee/tee_core.c
> > index e59c20d74b36..3dfd5428d58c 100644
> > --- a/drivers/tee/tee_core.c
> > +++ b/drivers/tee/tee_core.c
> > @@ -356,6 +356,42 @@ tee_ioctl_shm_register(struct tee_context *ctx,
> >         return ret;
> >  }
> >
> > +static int tee_ioctl_shm_register_fd(struct tee_context *ctx,
> > +                                    struct tee_ioctl_shm_register_fd_data __user *udata)
> > +{
> > +       struct tee_ioctl_shm_register_fd_data data;
> > +       struct tee_shm *shm;
> > +       long ret;
> > +
> > +       if (copy_from_user(&data, udata, sizeof(data)))
> > +               return -EFAULT;
> > +
> > +       /* Currently no input flags are supported */
> > +       if (data.flags)
> > +               return -EINVAL;
> > +
> > +       shm = tee_shm_register_fd(ctx, data.fd);
> > +       if (IS_ERR(shm))
> > +               return -EINVAL;
> > +
> > +       data.id = shm->id;
> > +       data.flags = shm->flags;
> > +       data.size = shm->size;
> > +
> > +       if (copy_to_user(udata, &data, sizeof(data)))
> > +               ret = -EFAULT;
> > +       else
> > +               ret = tee_shm_get_fd(shm);
> > +
> > +       /*
> > +        * When user space closes the file descriptor the shared memory
> > +        * should be freed or if tee_shm_get_fd() failed then it will
> > +        * be freed immediately.
> > +        */
> > +       tee_shm_put(shm);
> > +       return ret;
> > +}
> > +
> >  static int params_from_user(struct tee_context *ctx, struct tee_param *params,
> >                             size_t num_params,
> >                             struct tee_ioctl_param __user *uparams)
> > @@ -830,6 +866,8 @@ static long tee_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> >                 return tee_ioctl_shm_alloc(ctx, uarg);
> >         case TEE_IOC_SHM_REGISTER:
> >                 return tee_ioctl_shm_register(ctx, uarg);
> > +       case TEE_IOC_SHM_REGISTER_FD:
> > +               return tee_ioctl_shm_register_fd(ctx, uarg);
> >         case TEE_IOC_OPEN_SESSION:
> >                 return tee_ioctl_open_session(ctx, uarg);
> >         case TEE_IOC_INVOKE:
> > diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c
> > index 731d9028b67f..a1cb3c8b6423 100644
> > --- a/drivers/tee/tee_shm.c
> > +++ b/drivers/tee/tee_shm.c
> > @@ -4,6 +4,7 @@
> >   */
> >  #include <linux/anon_inodes.h>
> >  #include <linux/device.h>
> > +#include <linux/dma-buf.h>
> >  #include <linux/idr.h>
> >  #include <linux/mm.h>
> >  #include <linux/sched.h>
> > @@ -14,6 +15,14 @@
> >  #include <linux/highmem.h>
> >  #include "tee_private.h"
> >
> > +/* extra references appended to shm object for registered shared memory */
> > +struct tee_shm_dmabuf_ref {
> > +       struct tee_shm shm;
> > +       struct dma_buf *dmabuf;
> > +       struct dma_buf_attachment *attach;
> > +       struct sg_table *sgt;
> > +};
> > +
> >  static void shm_put_kernel_pages(struct page **pages, size_t page_count)
> >  {
> >         size_t n;
> > @@ -44,7 +53,16 @@ static void release_registered_pages(struct tee_shm *shm)
> >
> >  static void tee_shm_release(struct tee_device *teedev, struct tee_shm *shm)
> >  {
> > -       if (shm->flags & TEE_SHM_POOL) {
> > +       if (shm->flags & TEE_SHM_DMA_BUF) {
> > +               struct tee_shm_dmabuf_ref *ref;
> > +
> > +               ref = container_of(shm, struct tee_shm_dmabuf_ref, shm);
> > +               dma_buf_unmap_attachment(ref->attach, ref->sgt,
> > +               DMA_BIDIRECTIONAL);
> > +
> > +               dma_buf_detach(ref->dmabuf, ref->attach);
> > +               dma_buf_put(ref->dmabuf);
> > +       } else if (shm->flags & TEE_SHM_POOL) {
> >                 teedev->pool->ops->free(teedev->pool, shm);
> >         } else if (shm->flags & TEE_SHM_DYNAMIC) {
> >                 int rc = teedev->desc->ops->shm_unregister(shm->ctx, shm);
> > @@ -56,7 +74,8 @@ static void tee_shm_release(struct tee_device *teedev, struct tee_shm *shm)
> >                 release_registered_pages(shm);
> >         }
> >
> > -       teedev_ctx_put(shm->ctx);
> > +       if (shm->ctx)
> > +               teedev_ctx_put(shm->ctx);
> >
> >         kfree(shm);
> >
> > @@ -168,7 +187,7 @@ struct tee_shm *tee_shm_alloc_user_buf(struct tee_context *ctx, size_t size)
> >   * tee_client_invoke_func(). The memory allocated is later freed with a
> >   * call to tee_shm_free().
> >   *
> > - * @returns a pointer to 'struct tee_shm'
> > + * @returns a pointer to 'struct tee_shm' on success, and ERR_PTR on failure
> >   */
> >  struct tee_shm *tee_shm_alloc_kernel_buf(struct tee_context *ctx, size_t size)
> >  {
> > @@ -178,6 +197,85 @@ struct tee_shm *tee_shm_alloc_kernel_buf(struct tee_context *ctx, size_t size)
> >  }
> >  EXPORT_SYMBOL_GPL(tee_shm_alloc_kernel_buf);
> >
> > +struct tee_shm *tee_shm_register_fd(struct tee_context *ctx, int fd)
> > +{
> > +       struct tee_shm_dmabuf_ref *ref;
> > +       int rc;
> > +
> > +       if (!tee_device_get(ctx->teedev))
> > +               return ERR_PTR(-EINVAL);
> > +
> > +       teedev_ctx_get(ctx);
> > +
> > +       ref = kzalloc(sizeof(*ref), GFP_KERNEL);
> > +       if (!ref) {
> > +               rc = -ENOMEM;
> > +               goto err_put_tee;
> > +       }
> > +
> > +       refcount_set(&ref->shm.refcount, 1);
> > +       ref->shm.ctx = ctx;
> > +       ref->shm.id = -1;
> > +
> > +       ref->dmabuf = dma_buf_get(fd);
> > +       if (IS_ERR(ref->dmabuf)) {
> > +               rc = PTR_ERR(ref->dmabuf);
> > +               goto err_put_dmabuf;
> > +       }
>
> Hi,
>
> Most of the gotos in the errors paths from here on look offset by one
> to me. Attempting to put a dmabuf after failing to get, detaching
> after failing to attach, unmapping after failing to map, removing an
> IDR after failing to allocate one.

You're right, I'll fix them.

Thanks,
Jens

>
>
> > +
> > +       ref->attach = dma_buf_attach(ref->dmabuf, &ref->shm.ctx->teedev->dev);
> > +       if (IS_ERR(ref->attach)) {
> > +               rc = PTR_ERR(ref->attach);
> > +               goto err_detach;
> > +       }
> > +
> > +       ref->sgt = dma_buf_map_attachment(ref->attach, DMA_BIDIRECTIONAL);
> > +       if (IS_ERR(ref->sgt)) {
> > +               rc = PTR_ERR(ref->sgt);
> > +               goto err_unmap_attachement;
> > +       }
> > +
> > +       if (sg_nents(ref->sgt->sgl) != 1) {
> > +               rc = PTR_ERR(ref->sgt->sgl);
> > +               goto err_unmap_attachement;
> > +       }
> > +
> > +       ref->shm.paddr = page_to_phys(sg_page(ref->sgt->sgl));
> > +       ref->shm.size = ref->sgt->sgl->length;
> > +       ref->shm.flags = TEE_SHM_DMA_BUF;
> > +
> > +       mutex_lock(&ref->shm.ctx->teedev->mutex);
> > +       ref->shm.id = idr_alloc(&ref->shm.ctx->teedev->idr, &ref->shm,
> > +                               1, 0, GFP_KERNEL);
> > +       mutex_unlock(&ref->shm.ctx->teedev->mutex);
> > +       if (ref->shm.id < 0) {
> > +               rc = ref->shm.id;
> > +               goto err_idr_remove;
> > +       }
> > +
> > +       return &ref->shm;
> > +
> > +err_idr_remove:
> > +       mutex_lock(&ctx->teedev->mutex);
> > +       idr_remove(&ctx->teedev->idr, ref->shm.id);
> > +       mutex_unlock(&ctx->teedev->mutex);
> > +err_unmap_attachement:
> > +       dma_buf_unmap_attachment(ref->attach, ref->sgt, DMA_BIDIRECTIONAL);
> > +err_detach:
> > +       dma_buf_detach(ref->dmabuf, ref->attach);
> > +err_put_dmabuf:
> > +       dma_buf_put(ref->dmabuf);
> > +       kfree(ref);
> > +err_put_tee:
> > +       teedev_ctx_put(ctx);
> > +       tee_device_put(ctx->teedev);
> > +
> > +       return ERR_PTR(rc);
> > +}
> > +EXPORT_SYMBOL_GPL(tee_shm_register_fd);
> > +
> > +
> > +
> >  /**
> >   * tee_shm_alloc_priv_buf() - Allocate shared memory for a privately shared
> >   *                           kernel buffer
> > diff --git a/include/linux/tee_drv.h b/include/linux/tee_drv.h
> > index 71632e3c5f18..6a1fee689007 100644
> > --- a/include/linux/tee_drv.h
> > +++ b/include/linux/tee_drv.h
> > @@ -25,6 +25,7 @@
> >  #define TEE_SHM_USER_MAPPED    BIT(1)  /* Memory mapped in user space */
> >  #define TEE_SHM_POOL           BIT(2)  /* Memory allocated from pool */
> >  #define TEE_SHM_PRIV           BIT(3)  /* Memory private to TEE driver */
> > +#define TEE_SHM_DMA_BUF                BIT(4)  /* Memory with dma-buf handle */
> >
> >  struct device;
> >  struct tee_device;
> > @@ -275,6 +276,16 @@ void *tee_get_drvdata(struct tee_device *teedev);
> >  struct tee_shm *tee_shm_alloc_priv_buf(struct tee_context *ctx, size_t size);
> >  struct tee_shm *tee_shm_alloc_kernel_buf(struct tee_context *ctx, size_t size);
> >
> > +/**
> > + * tee_shm_register_fd() - Register shared memory from file descriptor
> > + *
> > + * @ctx:       Context that allocates the shared memory
> > + * @fd:                Shared memory file descriptor reference
> > + *
> > + * @returns a pointer to 'struct tee_shm' on success, and ERR_PTR on failure
> > + */
> > +struct tee_shm *tee_shm_register_fd(struct tee_context *ctx, int fd);
> > +
> >  struct tee_shm *tee_shm_register_kernel_buf(struct tee_context *ctx,
> >                                             void *addr, size_t length);
> >
> > diff --git a/include/uapi/linux/tee.h b/include/uapi/linux/tee.h
> > index 23e57164693c..77bc8ef24d3c 100644
> > --- a/include/uapi/linux/tee.h
> > +++ b/include/uapi/linux/tee.h
> > @@ -117,6 +117,35 @@ struct tee_ioctl_shm_alloc_data {
> >  #define TEE_IOC_SHM_ALLOC      _IOWR(TEE_IOC_MAGIC, TEE_IOC_BASE + 1, \
> >                                      struct tee_ioctl_shm_alloc_data)
> >
> > +/**
> > + * struct tee_ioctl_shm_register_fd_data - Shared memory registering argument
> > + * @fd:                [in] File descriptor identifying the shared memory
> > + * @size:      [out] Size of shared memory to allocate
> > + * @flags:     [in] Flags to/from allocation.
> > + * @id:                [out] Identifier of the shared memory
> > + *
> > + * The flags field should currently be zero as input. Updated by the call
> > + * with actual flags as defined by TEE_IOCTL_SHM_* above.
> > + * This structure is used as argument for TEE_IOC_SHM_REGISTER_FD below.
> > + */
> > +struct tee_ioctl_shm_register_fd_data {
> > +       __s64 fd;
> > +       __u64 size;
> > +       __u32 flags;
> > +       __s32 id;
> > +} __aligned(8);
> > +
> > +/**
> > + * TEE_IOC_SHM_REGISTER_FD - register a shared memory from a file descriptor
> > + *
> > + * Returns a file descriptor on success or < 0 on failure
> > + *
> > + * The returned file descriptor refers to the shared memory object in kernel
> > + * land. The shared memory is freed when the descriptor is closed.
> > + */
> > +#define TEE_IOC_SHM_REGISTER_FD        _IOWR(TEE_IOC_MAGIC, TEE_IOC_BASE + 8, \
> > +                                    struct tee_ioctl_shm_register_fd_data)
> > +
> >  /**
> >   * struct tee_ioctl_buf_data - Variable sized buffer
> >   * @buf_ptr:   [in] A __user pointer to a buffer
> > --
> > 2.34.1
> >

Re: [RFC PATCH 4/4] dma-buf: heaps: add Linaro restricted dmabuf heap support

[T.J. Mercier]

On Wed, Sep 4, 2024 at 2:44 AM Jens Wiklander <jens.wiklander@linaro.org> wrote:
>
> On Tue, Sep 3, 2024 at 7:50 PM T.J. Mercier <tjmercier@google.com> wrote:
> >
> > On Fri, Aug 30, 2024 at 12:04 AM Jens Wiklander
> > <jens.wiklander@linaro.org> wrote:
> > >
> > > Add a Linaro restricted heap using the linaro,restricted-heap bindings
> > > implemented based on the generic restricted heap.
> > >
> > > The bindings defines a range of physical restricted memory. The heap
> > > manages this address range using genalloc. The allocated dma-buf file
> > > descriptor can later be registered with the TEE subsystem for later use
> > > via Trusted Applications in the secure world.
> > >
> > > Co-developed-by: Olivier Masse <olivier.masse@nxp.com>
> > > Signed-off-by: Olivier Masse <olivier.masse@nxp.com>
> > > Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> > > ---
> > >  drivers/dma-buf/heaps/Kconfig                 |  10 ++
> > >  drivers/dma-buf/heaps/Makefile                |   1 +
> > >  .../dma-buf/heaps/restricted_heap_linaro.c    | 165 ++++++++++++++++++
> > >  3 files changed, 176 insertions(+)
> > >  create mode 100644 drivers/dma-buf/heaps/restricted_heap_linaro.c
> > >
> > > diff --git a/drivers/dma-buf/heaps/Kconfig b/drivers/dma-buf/heaps/Kconfig
> > > index 58903bc62ac8..82e2c5d09242 100644
> > > --- a/drivers/dma-buf/heaps/Kconfig
> > > +++ b/drivers/dma-buf/heaps/Kconfig
> > > @@ -28,3 +28,13 @@ config DMABUF_HEAPS_RESTRICTED_MTK
> > >         help
> > >           Enable restricted dma-buf heaps for MediaTek platform. This heap is backed by
> > >           TEE client interfaces. If in doubt, say N.
> > > +
> > > +config DMABUF_HEAPS_RESTRICTED_LINARO
> > > +       bool "Linaro DMA-BUF Restricted Heap"
> > > +       depends on DMABUF_HEAPS_RESTRICTED
> > > +       help
> > > +         Choose this option to enable the Linaro restricted dma-buf heap.
> > > +         The restricted heap pools are defined according to the DT. Heaps
> > > +         are allocated in the pools using gen allocater.
> > > +         If in doubt, say N.
> > > +
> > > diff --git a/drivers/dma-buf/heaps/Makefile b/drivers/dma-buf/heaps/Makefile
> > > index 0028aa9d875f..66b2f67c47b5 100644
> > > --- a/drivers/dma-buf/heaps/Makefile
> > > +++ b/drivers/dma-buf/heaps/Makefile
> > > @@ -2,4 +2,5 @@
> > >  obj-$(CONFIG_DMABUF_HEAPS_CMA)         += cma_heap.o
> > >  obj-$(CONFIG_DMABUF_HEAPS_RESTRICTED)  += restricted_heap.o
> > >  obj-$(CONFIG_DMABUF_HEAPS_RESTRICTED_MTK)      += restricted_heap_mtk.o
> > > +obj-$(CONFIG_DMABUF_HEAPS_RESTRICTED_LINARO)   += restricted_heap_linaro.o
> > >  obj-$(CONFIG_DMABUF_HEAPS_SYSTEM)      += system_heap.o
> > > diff --git a/drivers/dma-buf/heaps/restricted_heap_linaro.c b/drivers/dma-buf/heaps/restricted_heap_linaro.c
> > > new file mode 100644
> > > index 000000000000..4b08ed514023
> > > --- /dev/null
> > > +++ b/drivers/dma-buf/heaps/restricted_heap_linaro.c
> > > @@ -0,0 +1,165 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * DMABUF secure heap exporter
> > > + *
> > > + * Copyright 2021 NXP.
> > > + * Copyright 2024 Linaro Limited.
> > > + */
> > > +
> > > +#define pr_fmt(fmt)     "rheap_linaro: " fmt
> > > +
> > > +#include <linux/dma-buf.h>
> > > +#include <linux/err.h>
> > > +#include <linux/genalloc.h>
> > > +#include <linux/module.h>
> > > +#include <linux/of.h>
> > > +#include <linux/of_fdt.h>
> > > +#include <linux/of_reserved_mem.h>
> > > +#include <linux/scatterlist.h>
> > > +#include <linux/slab.h>
> > > +
> > > +#include "restricted_heap.h"
> > > +
> > > +#define MAX_HEAP_COUNT 2
> >
> > Are multiple supported because of what Cyrille mentioned here about permissions?
> > https://lore.kernel.org/lkml/DBBPR04MB7514E006455AEA407041E4F788709@DBBPR04MB7514.eurprd04.prod.outlook.com/
>
> Yes, I kept that as is.

Ok thanks.

> > So this is just some arbitrary limit? I'd prefer to have some sort of
> > documentation about this.
>
> How about removing the limit and using dynamic allocation instead?

That works too!

>
> Thanks,
> Jens
>
> >
> >
> > > +#define HEAP_NAME_LEN  32
> > > +
> > > +struct resmem_restricted {
> > > +       phys_addr_t base;
> > > +       phys_addr_t size;
> > > +
> > > +       char name[HEAP_NAME_LEN];
> > > +
> > > +       bool no_map;
> > > +};
> > > +
> > > +static struct resmem_restricted restricted_data[MAX_HEAP_COUNT] = {0};
> > > +static unsigned int restricted_data_count;
> > > +
> > > +static int linaro_restricted_memory_allocate(struct restricted_heap *heap,
> > > +                                            struct restricted_buffer *buf)
> > > +{
> > > +       struct gen_pool *pool = heap->priv_data;
> > > +       unsigned long pa;
> > > +       int ret;
> > > +
> > > +       buf->size = ALIGN(buf->size, PAGE_SIZE);
> > > +       pa = gen_pool_alloc(pool, buf->size);
> > > +       if (!pa)
> > > +               return -ENOMEM;
> > > +
> > > +       ret = sg_alloc_table(&buf->sg_table, 1, GFP_KERNEL);
> > > +       if (ret) {
> > > +               gen_pool_free(pool, pa, buf->size);
> > > +               return ret;
> > > +       }
> > > +
> > > +       sg_set_page(buf->sg_table.sgl, phys_to_page(pa), buf->size, 0);
> > > +
> > > +       return 0;
> > > +}
> > > +
> > > +static void linaro_restricted_memory_free(struct restricted_heap *heap,
> > > +                                         struct restricted_buffer *buf)
> > > +{
> > > +       struct gen_pool *pool = heap->priv_data;
> > > +       struct scatterlist *sg;
> > > +       unsigned int i;
> > > +
> > > +       for_each_sg(buf->sg_table.sgl, sg, buf->sg_table.nents, i)
> > > +               gen_pool_free(pool, page_to_phys(sg_page(sg)), sg->length);
> > > +       sg_free_table(&buf->sg_table);
> > > +}
> > > +
> > > +static const struct restricted_heap_ops linaro_restricted_heap_ops = {
> > > +       .alloc = linaro_restricted_memory_allocate,
> > > +       .free = linaro_restricted_memory_free,
> > > +};
> > > +
> > > +static int add_heap(struct resmem_restricted *mem)
> > > +{
> > > +       struct restricted_heap *heap;
> > > +       struct gen_pool *pool;
> > > +       int ret;
> > > +
> > > +       if (mem->base == 0 || mem->size == 0) {
> > > +               pr_err("restricted_data base or size is not correct\n");
> > > +               return -EINVAL;
> > > +       }
> > > +
> > > +       heap = kzalloc(sizeof(*heap), GFP_KERNEL);
> > > +       if (!heap)
> > > +               return -ENOMEM;
> > > +
> > > +       pool = gen_pool_create(PAGE_SHIFT, -1);
> > > +       if (!pool) {
> > > +               ret = -ENOMEM;
> > > +               goto err_free_heap;
> > > +       }
> > > +
> > > +       ret = gen_pool_add(pool, mem->base, mem->size, -1);
> > > +       if (ret)
> > > +               goto err_free_pool;
> > > +
> > > +       heap->no_map = mem->no_map;
> > > +       heap->priv_data = pool;
> > > +       heap->name = mem->name;
> > > +       heap->ops = &linaro_restricted_heap_ops;
> > > +
> > > +       ret = restricted_heap_add(heap);
> > > +       if (ret)
> > > +               goto err_free_pool;
> > > +
> > > +       return 0;
> > > +
> > > +err_free_pool:
> > > +       gen_pool_destroy(pool);
> > > +err_free_heap:
> > > +       kfree(heap);
> > > +
> > > +       return ret;
> > > +}
> > > +
> > > +static int __init rmem_restricted_heap_setup(struct reserved_mem *rmem)
> > > +{
> > > +       size_t len = HEAP_NAME_LEN;
> > > +       const char *s;
> > > +       bool no_map;
> > > +
> > > +       if (WARN_ONCE(restricted_data_count >= MAX_HEAP_COUNT,
> > > +                     "Cannot handle more than %u restricted heaps\n",
> > > +                     MAX_HEAP_COUNT))
> > > +               return -EINVAL;
> > > +
> > > +       no_map = of_get_flat_dt_prop(rmem->fdt_node, "no-map", NULL);
> > > +       s = strchr(rmem->name, '@');
> > > +       if (s)
> > > +               len = umin(s - rmem->name + 1, len);
> > > +
> > > +       restricted_data[restricted_data_count].base = rmem->base;
> > > +       restricted_data[restricted_data_count].size = rmem->size;
> > > +       restricted_data[restricted_data_count].no_map = no_map;
> > > +       strscpy(restricted_data[restricted_data_count].name, rmem->name, len);
> > > +
> > > +       restricted_data_count++;
> > > +       return 0;
> > > +}
> > > +
> > > +RESERVEDMEM_OF_DECLARE(linaro_restricted_heap, "linaro,restricted-heap",
> > > +                      rmem_restricted_heap_setup);
> > > +
> > > +static int linaro_restricted_heap_init(void)
> > > +{
> > > +       unsigned int i;
> > > +       int ret;
> > > +
> > > +       for (i = 0; i < restricted_data_count; i++) {
> > > +               ret = add_heap(&restricted_data[i]);
> > > +               if (ret)
> > > +                       return ret;
> > > +       }
> > > +       return 0;
> > > +}
> > > +
> > > +module_init(linaro_restricted_heap_init);
> > > +MODULE_DESCRIPTION("Linaro Restricted Heap Driver");
> > > +MODULE_LICENSE("GPL");
> > > --
> > > 2.34.1
> > >

Re: [RFC PATCH 1/4] dma-buf: heaps: restricted_heap: add no_map attribute

[Jens Wiklander]

On Fri, Aug 30, 2024 at 10:47 AM Christian König
<christian.koenig@amd.com> wrote:
>
> Am 30.08.24 um 09:03 schrieb Jens Wiklander:
> > Add a no_map attribute to struct restricted_heap_attachment and struct
> > restricted_heap to skip the call to dma_map_sgtable() if set. This
> > avoids trying to map a dma-buf that doens't refer to memory accessible
> > by the kernel.
>
> You seem to have a misunderstanding here dma_map_sgtable() is called to
> map a table into IOMMU and not any kernel address space.
>
> So please explain why you need that?

You're right, I had misunderstood dma_map_sgtable(). There's no need
for the no_map attribute, so I'll remove it.

Perhaps you have a suggestion on how to fix a problem when using
dma_map_sgtable()?

Without it, I'll have to assign a pointer to teedev->dev.dma_mask when
using the restricted heap with the OP-TEE driver or there will be a
warning in __dma_map_sg_attrs() ending with a failure when trying to
register the dma-buf fd. OP-TEE is probed with a platform device, and
taking the dma_mask pointer from that device works. Is that a good
approach or is there a better way of assigning dma_mask?

Thanks,
Jens

>
> Regards,
> Christian.
>
> >
> > Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> > ---
> >   drivers/dma-buf/heaps/restricted_heap.c | 17 +++++++++++++----
> >   drivers/dma-buf/heaps/restricted_heap.h |  2 ++
> >   2 files changed, 15 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/dma-buf/heaps/restricted_heap.c b/drivers/dma-buf/heaps/restricted_heap.c
> > index 8bc8a5e3f969..4bf28e3727ca 100644
> > --- a/drivers/dma-buf/heaps/restricted_heap.c
> > +++ b/drivers/dma-buf/heaps/restricted_heap.c
> > @@ -16,6 +16,7 @@
> >   struct restricted_heap_attachment {
> >       struct sg_table                 *table;
> >       struct device                   *dev;
> > +     bool no_map;
> >   };
> >
> >   static int
> > @@ -54,6 +55,8 @@ restricted_heap_memory_free(struct restricted_heap *rheap, struct restricted_buf
> >   static int restricted_heap_attach(struct dma_buf *dmabuf, struct dma_buf_attachment *attachment)
> >   {
> >       struct restricted_buffer *restricted_buf = dmabuf->priv;
> > +     struct dma_heap *heap = restricted_buf->heap;
> > +     struct restricted_heap *rheap = dma_heap_get_drvdata(heap);
> >       struct restricted_heap_attachment *a;
> >       struct sg_table *table;
> >
> > @@ -70,6 +73,7 @@ static int restricted_heap_attach(struct dma_buf *dmabuf, struct dma_buf_attachm
> >       sg_dma_mark_restricted(table->sgl);
> >       a->table = table;
> >       a->dev = attachment->dev;
> > +     a->no_map = rheap->no_map;
> >       attachment->priv = a;
> >
> >       return 0;
> > @@ -92,9 +96,12 @@ restricted_heap_map_dma_buf(struct dma_buf_attachment *attachment,
> >       struct sg_table *table = a->table;
> >       int ret;
> >
> > -     ret = dma_map_sgtable(attachment->dev, table, direction, DMA_ATTR_SKIP_CPU_SYNC);
> > -     if (ret)
> > -             return ERR_PTR(ret);
> > +     if (!a->no_map) {
> > +             ret = dma_map_sgtable(attachment->dev, table, direction,
> > +                                   DMA_ATTR_SKIP_CPU_SYNC);
> > +             if (ret)
> > +                     return ERR_PTR(ret);
> > +     }
> >       return table;
> >   }
> >
> > @@ -106,7 +113,9 @@ restricted_heap_unmap_dma_buf(struct dma_buf_attachment *attachment, struct sg_t
> >
> >       WARN_ON(a->table != table);
> >
> > -     dma_unmap_sgtable(attachment->dev, table, direction, DMA_ATTR_SKIP_CPU_SYNC);
> > +     if (!a->no_map)
> > +             dma_unmap_sgtable(attachment->dev, table, direction,
> > +                               DMA_ATTR_SKIP_CPU_SYNC);
> >   }
> >
> >   static int
> > diff --git a/drivers/dma-buf/heaps/restricted_heap.h b/drivers/dma-buf/heaps/restricted_heap.h
> > index 7dec4b8a471b..94cc0842f70d 100644
> > --- a/drivers/dma-buf/heaps/restricted_heap.h
> > +++ b/drivers/dma-buf/heaps/restricted_heap.h
> > @@ -27,6 +27,8 @@ struct restricted_heap {
> >       unsigned long           cma_paddr;
> >       unsigned long           cma_size;
> >
> > +     bool                    no_map;
> > +
> >       void                    *priv_data;
> >   };
> >
>

Re: [RFC PATCH 1/4] dma-buf: heaps: restricted_heap: add no_map attribute

[Christian König]

Am 05.09.24 um 08:56 schrieb Jens Wiklander:
> On Fri, Aug 30, 2024 at 10:47 AM Christian König
> <christian.koenig@amd.com> wrote:
>> Am 30.08.24 um 09:03 schrieb Jens Wiklander:
>>> Add a no_map attribute to struct restricted_heap_attachment and struct
>>> restricted_heap to skip the call to dma_map_sgtable() if set. This
>>> avoids trying to map a dma-buf that doens't refer to memory accessible
>>> by the kernel.
>> You seem to have a misunderstanding here dma_map_sgtable() is called to
>> map a table into IOMMU and not any kernel address space.
>>
>> So please explain why you need that?
> You're right, I had misunderstood dma_map_sgtable(). There's no need
> for the no_map attribute, so I'll remove it.
>
> Perhaps you have a suggestion on how to fix a problem when using
> dma_map_sgtable()?
>
> Without it, I'll have to assign a pointer to teedev->dev.dma_mask when
> using the restricted heap with the OP-TEE driver or there will be a
> warning in __dma_map_sg_attrs() ending with a failure when trying to
> register the dma-buf fd. OP-TEE is probed with a platform device, and
> taking the dma_mask pointer from that device works. Is that a good
> approach or is there a better way of assigning dma_mask?

Mhm, I don't know the full picture so I have to make some assumptions.

The teedev is just a virtual device which represents the restricted 
memory access paths of a real device?

If that is true then taking the dma-mask from the real device is most 
likely the right thing to do.

Regards,
Christian.

>
> Thanks,
> Jens
>
>> Regards,
>> Christian.
>>
>>> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
>>> ---
>>>    drivers/dma-buf/heaps/restricted_heap.c | 17 +++++++++++++----
>>>    drivers/dma-buf/heaps/restricted_heap.h |  2 ++
>>>    2 files changed, 15 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/dma-buf/heaps/restricted_heap.c b/drivers/dma-buf/heaps/restricted_heap.c
>>> index 8bc8a5e3f969..4bf28e3727ca 100644
>>> --- a/drivers/dma-buf/heaps/restricted_heap.c
>>> +++ b/drivers/dma-buf/heaps/restricted_heap.c
>>> @@ -16,6 +16,7 @@
>>>    struct restricted_heap_attachment {
>>>        struct sg_table                 *table;
>>>        struct device                   *dev;
>>> +     bool no_map;
>>>    };
>>>
>>>    static int
>>> @@ -54,6 +55,8 @@ restricted_heap_memory_free(struct restricted_heap *rheap, struct restricted_buf
>>>    static int restricted_heap_attach(struct dma_buf *dmabuf, struct dma_buf_attachment *attachment)
>>>    {
>>>        struct restricted_buffer *restricted_buf = dmabuf->priv;
>>> +     struct dma_heap *heap = restricted_buf->heap;
>>> +     struct restricted_heap *rheap = dma_heap_get_drvdata(heap);
>>>        struct restricted_heap_attachment *a;
>>>        struct sg_table *table;
>>>
>>> @@ -70,6 +73,7 @@ static int restricted_heap_attach(struct dma_buf *dmabuf, struct dma_buf_attachm
>>>        sg_dma_mark_restricted(table->sgl);
>>>        a->table = table;
>>>        a->dev = attachment->dev;
>>> +     a->no_map = rheap->no_map;
>>>        attachment->priv = a;
>>>
>>>        return 0;
>>> @@ -92,9 +96,12 @@ restricted_heap_map_dma_buf(struct dma_buf_attachment *attachment,
>>>        struct sg_table *table = a->table;
>>>        int ret;
>>>
>>> -     ret = dma_map_sgtable(attachment->dev, table, direction, DMA_ATTR_SKIP_CPU_SYNC);
>>> -     if (ret)
>>> -             return ERR_PTR(ret);
>>> +     if (!a->no_map) {
>>> +             ret = dma_map_sgtable(attachment->dev, table, direction,
>>> +                                   DMA_ATTR_SKIP_CPU_SYNC);
>>> +             if (ret)
>>> +                     return ERR_PTR(ret);
>>> +     }
>>>        return table;
>>>    }
>>>
>>> @@ -106,7 +113,9 @@ restricted_heap_unmap_dma_buf(struct dma_buf_attachment *attachment, struct sg_t
>>>
>>>        WARN_ON(a->table != table);
>>>
>>> -     dma_unmap_sgtable(attachment->dev, table, direction, DMA_ATTR_SKIP_CPU_SYNC);
>>> +     if (!a->no_map)
>>> +             dma_unmap_sgtable(attachment->dev, table, direction,
>>> +                               DMA_ATTR_SKIP_CPU_SYNC);
>>>    }
>>>
>>>    static int
>>> diff --git a/drivers/dma-buf/heaps/restricted_heap.h b/drivers/dma-buf/heaps/restricted_heap.h
>>> index 7dec4b8a471b..94cc0842f70d 100644
>>> --- a/drivers/dma-buf/heaps/restricted_heap.h
>>> +++ b/drivers/dma-buf/heaps/restricted_heap.h
>>> @@ -27,6 +27,8 @@ struct restricted_heap {
>>>        unsigned long           cma_paddr;
>>>        unsigned long           cma_size;
>>>
>>> +     bool                    no_map;
>>> +
>>>        void                    *priv_data;
>>>    };
>>>

Re: [RFC PATCH 4/4] dma-buf: heaps: add Linaro restricted dmabuf heap support

[Jens Wiklander]

On Wed, Sep 4, 2024 at 11:42 PM T.J. Mercier <tjmercier@google.com> wrote:
>
> On Wed, Sep 4, 2024 at 2:44 AM Jens Wiklander <jens.wiklander@linaro.org> wrote:
> >
> > On Tue, Sep 3, 2024 at 7:50 PM T.J. Mercier <tjmercier@google.com> wrote:
> > >
> > > On Fri, Aug 30, 2024 at 12:04 AM Jens Wiklander
> > > <jens.wiklander@linaro.org> wrote:
> > > >
> > > > Add a Linaro restricted heap using the linaro,restricted-heap bindings
> > > > implemented based on the generic restricted heap.
> > > >
> > > > The bindings defines a range of physical restricted memory. The heap
> > > > manages this address range using genalloc. The allocated dma-buf file
> > > > descriptor can later be registered with the TEE subsystem for later use
> > > > via Trusted Applications in the secure world.
> > > >
> > > > Co-developed-by: Olivier Masse <olivier.masse@nxp.com>
> > > > Signed-off-by: Olivier Masse <olivier.masse@nxp.com>
> > > > Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> > > > ---
> > > >  drivers/dma-buf/heaps/Kconfig                 |  10 ++
> > > >  drivers/dma-buf/heaps/Makefile                |   1 +
> > > >  .../dma-buf/heaps/restricted_heap_linaro.c    | 165 ++++++++++++++++++
> > > >  3 files changed, 176 insertions(+)
> > > >  create mode 100644 drivers/dma-buf/heaps/restricted_heap_linaro.c
> > > >
> > > > diff --git a/drivers/dma-buf/heaps/Kconfig b/drivers/dma-buf/heaps/Kconfig
> > > > index 58903bc62ac8..82e2c5d09242 100644
> > > > --- a/drivers/dma-buf/heaps/Kconfig
> > > > +++ b/drivers/dma-buf/heaps/Kconfig
> > > > @@ -28,3 +28,13 @@ config DMABUF_HEAPS_RESTRICTED_MTK
> > > >         help
> > > >           Enable restricted dma-buf heaps for MediaTek platform. This heap is backed by
> > > >           TEE client interfaces. If in doubt, say N.
> > > > +
> > > > +config DMABUF_HEAPS_RESTRICTED_LINARO
> > > > +       bool "Linaro DMA-BUF Restricted Heap"
> > > > +       depends on DMABUF_HEAPS_RESTRICTED
> > > > +       help
> > > > +         Choose this option to enable the Linaro restricted dma-buf heap.
> > > > +         The restricted heap pools are defined according to the DT. Heaps
> > > > +         are allocated in the pools using gen allocater.
> > > > +         If in doubt, say N.
> > > > +
> > > > diff --git a/drivers/dma-buf/heaps/Makefile b/drivers/dma-buf/heaps/Makefile
> > > > index 0028aa9d875f..66b2f67c47b5 100644
> > > > --- a/drivers/dma-buf/heaps/Makefile
> > > > +++ b/drivers/dma-buf/heaps/Makefile
> > > > @@ -2,4 +2,5 @@
> > > >  obj-$(CONFIG_DMABUF_HEAPS_CMA)         += cma_heap.o
> > > >  obj-$(CONFIG_DMABUF_HEAPS_RESTRICTED)  += restricted_heap.o
> > > >  obj-$(CONFIG_DMABUF_HEAPS_RESTRICTED_MTK)      += restricted_heap_mtk.o
> > > > +obj-$(CONFIG_DMABUF_HEAPS_RESTRICTED_LINARO)   += restricted_heap_linaro.o
> > > >  obj-$(CONFIG_DMABUF_HEAPS_SYSTEM)      += system_heap.o
> > > > diff --git a/drivers/dma-buf/heaps/restricted_heap_linaro.c b/drivers/dma-buf/heaps/restricted_heap_linaro.c
> > > > new file mode 100644
> > > > index 000000000000..4b08ed514023
> > > > --- /dev/null
> > > > +++ b/drivers/dma-buf/heaps/restricted_heap_linaro.c
> > > > @@ -0,0 +1,165 @@
> > > > +// SPDX-License-Identifier: GPL-2.0
> > > > +/*
> > > > + * DMABUF secure heap exporter
> > > > + *
> > > > + * Copyright 2021 NXP.
> > > > + * Copyright 2024 Linaro Limited.
> > > > + */
> > > > +
> > > > +#define pr_fmt(fmt)     "rheap_linaro: " fmt
> > > > +
> > > > +#include <linux/dma-buf.h>
> > > > +#include <linux/err.h>
> > > > +#include <linux/genalloc.h>
> > > > +#include <linux/module.h>
> > > > +#include <linux/of.h>
> > > > +#include <linux/of_fdt.h>
> > > > +#include <linux/of_reserved_mem.h>
> > > > +#include <linux/scatterlist.h>
> > > > +#include <linux/slab.h>
> > > > +
> > > > +#include "restricted_heap.h"
> > > > +
> > > > +#define MAX_HEAP_COUNT 2
> > >
> > > Are multiple supported because of what Cyrille mentioned here about permissions?
> > > https://lore.kernel.org/lkml/DBBPR04MB7514E006455AEA407041E4F788709@DBBPR04MB7514.eurprd04.prod.outlook.com/
> >
> > Yes, I kept that as is.
>
> Ok thanks.
>
> > > So this is just some arbitrary limit? I'd prefer to have some sort of
> > > documentation about this.
> >
> > How about removing the limit and using dynamic allocation instead?
>
> That works too!

It turns out that was easier said than done. The limit is hardcoded
because dynamic memory allocation isn't available at that stage during
boot. We have a short description of this heap in Kconfig. I'll add
something about the limit there if that makes sense.

Thanks,
Jens

Re: [RFC PATCH 4/4] dma-buf: heaps: add Linaro restricted dmabuf heap support

[T.J. Mercier]

On Mon, Sep 9, 2024 at 11:06 PM Jens Wiklander
<jens.wiklander@linaro.org> wrote:
>
> On Wed, Sep 4, 2024 at 11:42 PM T.J. Mercier <tjmercier@google.com> wrote:
> >
> > On Wed, Sep 4, 2024 at 2:44 AM Jens Wiklander <jens.wiklander@linaro.org> wrote:
> > >
> > > On Tue, Sep 3, 2024 at 7:50 PM T.J. Mercier <tjmercier@google.com> wrote:
> > > >
> > > > On Fri, Aug 30, 2024 at 12:04 AM Jens Wiklander
> > > > <jens.wiklander@linaro.org> wrote:
> > > > >
> > > > > Add a Linaro restricted heap using the linaro,restricted-heap bindings
> > > > > implemented based on the generic restricted heap.
> > > > >
> > > > > The bindings defines a range of physical restricted memory. The heap
> > > > > manages this address range using genalloc. The allocated dma-buf file
> > > > > descriptor can later be registered with the TEE subsystem for later use
> > > > > via Trusted Applications in the secure world.
> > > > >
> > > > > Co-developed-by: Olivier Masse <olivier.masse@nxp.com>
> > > > > Signed-off-by: Olivier Masse <olivier.masse@nxp.com>
> > > > > Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> > > > > ---
> > > > >  drivers/dma-buf/heaps/Kconfig                 |  10 ++
> > > > >  drivers/dma-buf/heaps/Makefile                |   1 +
> > > > >  .../dma-buf/heaps/restricted_heap_linaro.c    | 165 ++++++++++++++++++
> > > > >  3 files changed, 176 insertions(+)
> > > > >  create mode 100644 drivers/dma-buf/heaps/restricted_heap_linaro.c
> > > > >
> > > > > diff --git a/drivers/dma-buf/heaps/Kconfig b/drivers/dma-buf/heaps/Kconfig
> > > > > index 58903bc62ac8..82e2c5d09242 100644
> > > > > --- a/drivers/dma-buf/heaps/Kconfig
> > > > > +++ b/drivers/dma-buf/heaps/Kconfig
> > > > > @@ -28,3 +28,13 @@ config DMABUF_HEAPS_RESTRICTED_MTK
> > > > >         help
> > > > >           Enable restricted dma-buf heaps for MediaTek platform. This heap is backed by
> > > > >           TEE client interfaces. If in doubt, say N.
> > > > > +
> > > > > +config DMABUF_HEAPS_RESTRICTED_LINARO
> > > > > +       bool "Linaro DMA-BUF Restricted Heap"
> > > > > +       depends on DMABUF_HEAPS_RESTRICTED
> > > > > +       help
> > > > > +         Choose this option to enable the Linaro restricted dma-buf heap.
> > > > > +         The restricted heap pools are defined according to the DT. Heaps
> > > > > +         are allocated in the pools using gen allocater.
> > > > > +         If in doubt, say N.
> > > > > +
> > > > > diff --git a/drivers/dma-buf/heaps/Makefile b/drivers/dma-buf/heaps/Makefile
> > > > > index 0028aa9d875f..66b2f67c47b5 100644
> > > > > --- a/drivers/dma-buf/heaps/Makefile
> > > > > +++ b/drivers/dma-buf/heaps/Makefile
> > > > > @@ -2,4 +2,5 @@
> > > > >  obj-$(CONFIG_DMABUF_HEAPS_CMA)         += cma_heap.o
> > > > >  obj-$(CONFIG_DMABUF_HEAPS_RESTRICTED)  += restricted_heap.o
> > > > >  obj-$(CONFIG_DMABUF_HEAPS_RESTRICTED_MTK)      += restricted_heap_mtk.o
> > > > > +obj-$(CONFIG_DMABUF_HEAPS_RESTRICTED_LINARO)   += restricted_heap_linaro.o
> > > > >  obj-$(CONFIG_DMABUF_HEAPS_SYSTEM)      += system_heap.o
> > > > > diff --git a/drivers/dma-buf/heaps/restricted_heap_linaro.c b/drivers/dma-buf/heaps/restricted_heap_linaro.c
> > > > > new file mode 100644
> > > > > index 000000000000..4b08ed514023
> > > > > --- /dev/null
> > > > > +++ b/drivers/dma-buf/heaps/restricted_heap_linaro.c
> > > > > @@ -0,0 +1,165 @@
> > > > > +// SPDX-License-Identifier: GPL-2.0
> > > > > +/*
> > > > > + * DMABUF secure heap exporter
> > > > > + *
> > > > > + * Copyright 2021 NXP.
> > > > > + * Copyright 2024 Linaro Limited.
> > > > > + */
> > > > > +
> > > > > +#define pr_fmt(fmt)     "rheap_linaro: " fmt
> > > > > +
> > > > > +#include <linux/dma-buf.h>
> > > > > +#include <linux/err.h>
> > > > > +#include <linux/genalloc.h>
> > > > > +#include <linux/module.h>
> > > > > +#include <linux/of.h>
> > > > > +#include <linux/of_fdt.h>
> > > > > +#include <linux/of_reserved_mem.h>
> > > > > +#include <linux/scatterlist.h>
> > > > > +#include <linux/slab.h>
> > > > > +
> > > > > +#include "restricted_heap.h"
> > > > > +
> > > > > +#define MAX_HEAP_COUNT 2
> > > >
> > > > Are multiple supported because of what Cyrille mentioned here about permissions?
> > > > https://lore.kernel.org/lkml/DBBPR04MB7514E006455AEA407041E4F788709@DBBPR04MB7514.eurprd04.prod.outlook.com/
> > >
> > > Yes, I kept that as is.
> >
> > Ok thanks.
> >
> > > > So this is just some arbitrary limit? I'd prefer to have some sort of
> > > > documentation about this.
> > >
> > > How about removing the limit and using dynamic allocation instead?
> >
> > That works too!
>
> It turns out that was easier said than done. The limit is hardcoded
> because dynamic memory allocation isn't available at that stage during
> boot. We have a short description of this heap in Kconfig. I'll add
> something about the limit there if that makes sense.
>
> Thanks,
> Jens

Ah ok sounds good.

I noticed one other thing, linaro_restricted_heap_init and add_heap
should probably have __init. Last week I sent a patch to add that for
the CMA and system heaps.

Re: [RFC PATCH 4/4] dma-buf: heaps: add Linaro restricted dmabuf heap support

[Jens Wiklander]

On Tue, Sep 10, 2024 at 5:08 PM T.J. Mercier <tjmercier@google.com> wrote:
>
> On Mon, Sep 9, 2024 at 11:06 PM Jens Wiklander
> <jens.wiklander@linaro.org> wrote:
> >
> > On Wed, Sep 4, 2024 at 11:42 PM T.J. Mercier <tjmercier@google.com> wrote:
> > >
> > > On Wed, Sep 4, 2024 at 2:44 AM Jens Wiklander <jens.wiklander@linaro.org> wrote:
> > > >
> > > > On Tue, Sep 3, 2024 at 7:50 PM T.J. Mercier <tjmercier@google.com> wrote:
> > > > >
> > > > > On Fri, Aug 30, 2024 at 12:04 AM Jens Wiklander
> > > > > <jens.wiklander@linaro.org> wrote:
> > > > > >
> > > > > > Add a Linaro restricted heap using the linaro,restricted-heap bindings
> > > > > > implemented based on the generic restricted heap.
> > > > > >
> > > > > > The bindings defines a range of physical restricted memory. The heap
> > > > > > manages this address range using genalloc. The allocated dma-buf file
> > > > > > descriptor can later be registered with the TEE subsystem for later use
> > > > > > via Trusted Applications in the secure world.
> > > > > >
> > > > > > Co-developed-by: Olivier Masse <olivier.masse@nxp.com>
> > > > > > Signed-off-by: Olivier Masse <olivier.masse@nxp.com>
> > > > > > Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> > > > > > ---
> > > > > >  drivers/dma-buf/heaps/Kconfig                 |  10 ++
> > > > > >  drivers/dma-buf/heaps/Makefile                |   1 +
> > > > > >  .../dma-buf/heaps/restricted_heap_linaro.c    | 165 ++++++++++++++++++
> > > > > >  3 files changed, 176 insertions(+)
> > > > > >  create mode 100644 drivers/dma-buf/heaps/restricted_heap_linaro.c
> > > > > >
> > > > > > diff --git a/drivers/dma-buf/heaps/Kconfig b/drivers/dma-buf/heaps/Kconfig
> > > > > > index 58903bc62ac8..82e2c5d09242 100644
> > > > > > --- a/drivers/dma-buf/heaps/Kconfig
> > > > > > +++ b/drivers/dma-buf/heaps/Kconfig
> > > > > > @@ -28,3 +28,13 @@ config DMABUF_HEAPS_RESTRICTED_MTK
> > > > > >         help
> > > > > >           Enable restricted dma-buf heaps for MediaTek platform. This heap is backed by
> > > > > >           TEE client interfaces. If in doubt, say N.
> > > > > > +
> > > > > > +config DMABUF_HEAPS_RESTRICTED_LINARO
> > > > > > +       bool "Linaro DMA-BUF Restricted Heap"
> > > > > > +       depends on DMABUF_HEAPS_RESTRICTED
> > > > > > +       help
> > > > > > +         Choose this option to enable the Linaro restricted dma-buf heap.
> > > > > > +         The restricted heap pools are defined according to the DT. Heaps
> > > > > > +         are allocated in the pools using gen allocater.
> > > > > > +         If in doubt, say N.
> > > > > > +
> > > > > > diff --git a/drivers/dma-buf/heaps/Makefile b/drivers/dma-buf/heaps/Makefile
> > > > > > index 0028aa9d875f..66b2f67c47b5 100644
> > > > > > --- a/drivers/dma-buf/heaps/Makefile
> > > > > > +++ b/drivers/dma-buf/heaps/Makefile
> > > > > > @@ -2,4 +2,5 @@
> > > > > >  obj-$(CONFIG_DMABUF_HEAPS_CMA)         += cma_heap.o
> > > > > >  obj-$(CONFIG_DMABUF_HEAPS_RESTRICTED)  += restricted_heap.o
> > > > > >  obj-$(CONFIG_DMABUF_HEAPS_RESTRICTED_MTK)      += restricted_heap_mtk.o
> > > > > > +obj-$(CONFIG_DMABUF_HEAPS_RESTRICTED_LINARO)   += restricted_heap_linaro.o
> > > > > >  obj-$(CONFIG_DMABUF_HEAPS_SYSTEM)      += system_heap.o
> > > > > > diff --git a/drivers/dma-buf/heaps/restricted_heap_linaro.c b/drivers/dma-buf/heaps/restricted_heap_linaro.c
> > > > > > new file mode 100644
> > > > > > index 000000000000..4b08ed514023
> > > > > > --- /dev/null
> > > > > > +++ b/drivers/dma-buf/heaps/restricted_heap_linaro.c
> > > > > > @@ -0,0 +1,165 @@
> > > > > > +// SPDX-License-Identifier: GPL-2.0
> > > > > > +/*
> > > > > > + * DMABUF secure heap exporter
> > > > > > + *
> > > > > > + * Copyright 2021 NXP.
> > > > > > + * Copyright 2024 Linaro Limited.
> > > > > > + */
> > > > > > +
> > > > > > +#define pr_fmt(fmt)     "rheap_linaro: " fmt
> > > > > > +
> > > > > > +#include <linux/dma-buf.h>
> > > > > > +#include <linux/err.h>
> > > > > > +#include <linux/genalloc.h>
> > > > > > +#include <linux/module.h>
> > > > > > +#include <linux/of.h>
> > > > > > +#include <linux/of_fdt.h>
> > > > > > +#include <linux/of_reserved_mem.h>
> > > > > > +#include <linux/scatterlist.h>
> > > > > > +#include <linux/slab.h>
> > > > > > +
> > > > > > +#include "restricted_heap.h"
> > > > > > +
> > > > > > +#define MAX_HEAP_COUNT 2
> > > > >
> > > > > Are multiple supported because of what Cyrille mentioned here about permissions?
> > > > > https://lore.kernel.org/lkml/DBBPR04MB7514E006455AEA407041E4F788709@DBBPR04MB7514.eurprd04.prod.outlook.com/
> > > >
> > > > Yes, I kept that as is.
> > >
> > > Ok thanks.
> > >
> > > > > So this is just some arbitrary limit? I'd prefer to have some sort of
> > > > > documentation about this.
> > > >
> > > > How about removing the limit and using dynamic allocation instead?
> > >
> > > That works too!
> >
> > It turns out that was easier said than done. The limit is hardcoded
> > because dynamic memory allocation isn't available at that stage during
> > boot. We have a short description of this heap in Kconfig. I'll add
> > something about the limit there if that makes sense.
> >
> > Thanks,
> > Jens
>
> Ah ok sounds good.
>
> I noticed one other thing, linaro_restricted_heap_init and add_heap
> should probably have __init. Last week I sent a patch to add that for
> the CMA and system heaps.

Thanks, I'll add it.

Cheers,
Jens

Re: [RFC PATCH 0/4] Linaro restricted heap

[Dmitry Baryshkov]

Hi,

On Fri, Aug 30, 2024 at 09:03:47AM GMT, Jens Wiklander wrote:
> Hi,
> 
> This patch set is based on top of Yong Wu's restricted heap patch set [1].
> It's also a continuation on Olivier's Add dma-buf secure-heap patch set [2].
> 
> The Linaro restricted heap uses genalloc in the kernel to manage the heap
> carvout. This is a difference from the Mediatek restricted heap which
> relies on the secure world to manage the carveout.
> 
> I've tried to adress the comments on [2], but [1] introduces changes so I'm
> afraid I've had to skip some comments.

I know I have raised the same question during LPC (in connection to
Qualcomm's dma-heap implementation). Is there any reason why we are
using generic heaps instead of allocating the dma-bufs on the device
side?

In your case you already have TEE device, you can use it to allocate and
export dma-bufs, which then get imported by the V4L and DRM drivers.

I have a feeling (I might be completely wrong here) that by using
generic dma-buf heaps we can easily end up in a situation when the
userspace depends heavily on the actual platform being used (to map the
platform to heap names). I think we should instead depend on the
existing devices (e.g. if there is a TEE device, use an IOCTL to
allocate secured DMA BUF from it, otherwise check for QTEE device,
otherwise check for some other vendor device).

The mental experiment to check if the API is correct is really simple:
Can you use exactly the same rootfs on several devices without
any additional tuning (e.g. your QEMU, HiKey, a Mediatek board, Qualcomm
laptop, etc)?

> 
> This can be tested on QEMU with the following steps:
> repo init -u https://github.com/jenswi-linaro/manifest.git -m qemu_v8.xml \
>         -b prototype/sdp-v1
> repo sync -j8
> cd build
> make toolchains -j4
> make all -j$(nproc)
> make run-only
> # login and at the prompt:
> xtest --sdp-basic
> 
> https://optee.readthedocs.io/en/latest/building/prerequisites.html
> list dependencies needed to build the above.
> 
> The tests are pretty basic, mostly checking that a Trusted Application in
> the secure world can access and manipulate the memory.

- Can we test that the system doesn't crash badly if user provides
  non-secured memory to the users which expect a secure buffer?

- At the same time corresponding entities shouldn't decode data to the
  buffers accessible to the rest of the sytem.

> 
> Cheers,
> Jens
> 
> [1] https://lore.kernel.org/dri-devel/20240515112308.10171-1-yong.wu@mediatek.com/
> [2] https://lore.kernel.org/lkml/20220805135330.970-1-olivier.masse@nxp.com/
> 
> Changes since Olivier's post [2]:
> * Based on Yong Wu's post [1] where much of dma-buf handling is done in
>   the generic restricted heap
> * Simplifications and cleanup
> * New commit message for "dma-buf: heaps: add Linaro restricted dmabuf heap
>   support"
> * Replaced the word "secure" with "restricted" where applicable
> 
> Etienne Carriere (1):
>   tee: new ioctl to a register tee_shm from a dmabuf file descriptor
> 
> Jens Wiklander (2):
>   dma-buf: heaps: restricted_heap: add no_map attribute
>   dma-buf: heaps: add Linaro restricted dmabuf heap support
> 
> Olivier Masse (1):
>   dt-bindings: reserved-memory: add linaro,restricted-heap
> 
>  .../linaro,restricted-heap.yaml               |  56 ++++++
>  drivers/dma-buf/heaps/Kconfig                 |  10 ++
>  drivers/dma-buf/heaps/Makefile                |   1 +
>  drivers/dma-buf/heaps/restricted_heap.c       |  17 +-
>  drivers/dma-buf/heaps/restricted_heap.h       |   2 +
>  .../dma-buf/heaps/restricted_heap_linaro.c    | 165 ++++++++++++++++++
>  drivers/tee/tee_core.c                        |  38 ++++
>  drivers/tee/tee_shm.c                         | 104 ++++++++++-
>  include/linux/tee_drv.h                       |  11 ++
>  include/uapi/linux/tee.h                      |  29 +++
>  10 files changed, 426 insertions(+), 7 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/reserved-memory/linaro,restricted-heap.yaml
>  create mode 100644 drivers/dma-buf/heaps/restricted_heap_linaro.c
> 
> -- 
> 2.34.1
> 

-- 
With best wishes
Dmitry

Re: [RFC PATCH 0/4] Linaro restricted heap

[Andrew Davis]

On 9/23/24 1:33 AM, Dmitry Baryshkov wrote:
> Hi,
> 
> On Fri, Aug 30, 2024 at 09:03:47AM GMT, Jens Wiklander wrote:
>> Hi,
>>
>> This patch set is based on top of Yong Wu's restricted heap patch set [1].
>> It's also a continuation on Olivier's Add dma-buf secure-heap patch set [2].
>>
>> The Linaro restricted heap uses genalloc in the kernel to manage the heap
>> carvout. This is a difference from the Mediatek restricted heap which
>> relies on the secure world to manage the carveout.
>>
>> I've tried to adress the comments on [2], but [1] introduces changes so I'm
>> afraid I've had to skip some comments.
> 
> I know I have raised the same question during LPC (in connection to
> Qualcomm's dma-heap implementation). Is there any reason why we are
> using generic heaps instead of allocating the dma-bufs on the device
> side?
> 
> In your case you already have TEE device, you can use it to allocate and
> export dma-bufs, which then get imported by the V4L and DRM drivers.
> 

This goes to the heart of why we have dma-heaps in the first place.
We don't want to burden userspace with having to figure out the right
place to get a dma-buf for a given use-case on a given hardware.
That would be very non-portable, and fail at the core purpose of
a kernel: to abstract hardware specifics away.

Worse, the actual interface for dma-buf exporting changes from
framework to framework (getting a dma-buf from DRM is different
than V4L, and there would be yet another API for TEE, etc..)

Most subsystem don't need an allocator, they work just fine
simply being only dma-bufs importers. Recent example being the
IIO subsystem[0], for which some early posting included an
allocator, but in the end, all that was needed was to consume
buffers.

For devices that don't actually contain memory there is no
reason to be an exporter. What most want is just to consume
normal system memory. Or system memory with some constraints
(e.g. contiguous, coherent, restricted, etc..).

> I have a feeling (I might be completely wrong here) that by using
> generic dma-buf heaps we can easily end up in a situation when the
> userspace depends heavily on the actual platform being used (to map the
> platform to heap names). I think we should instead depend on the
> existing devices (e.g. if there is a TEE device, use an IOCTL to
> allocate secured DMA BUF from it, otherwise check for QTEE device,
> otherwise check for some other vendor device).
> 
> The mental experiment to check if the API is correct is really simple:
> Can you use exactly the same rootfs on several devices without
> any additional tuning (e.g. your QEMU, HiKey, a Mediatek board, Qualcomm
> laptop, etc)?
> 

This is a great north star to follow. And exactly the reason we should
*not* be exposing device specific constraints to userspace. The constrains
change based on the platform. So a userspace would have to also pick
a different set of constraints based on each platform.

Userspace knows which subsystems it will attach a buffer, and the
kernel knows what constraints those devices have on a given platform.
Ideal case is then allocate from the one exporter, attach to various
devices, and have the constraints solved at map time by the exporter
based on the set of attached devices.

For example, on one platform the display needs contiguous buffers,
but on a different platform the display can scatter-gather. So
what heap should our generic application allocate from when it
wants a buffer consumable by the display, CMA or System?
Answer *should* be always use the generic exporter, and that
exporter then picks the right backing type based on the platform.

Userspace shouldn't be dealing with any of these constraints
(looking back, adding the CMA heap was probably incorrect,
and the System heap should have been the only one. Idea back
then was a userspace helper would show up to do the constraint
solving and pick the right heap. That has yet to materialize and
folks are still just hardcoding which heap to use..).

Same for this restricted heap, I'd like to explore if we can
enhance the System heap such that when attached to the TEE framework,
the backing memory is either made restricted by fire-walling,
or allocating from a TEE carveout (based on platform).

This will mean more inter-subsystem coordination, but we can
iterate on these in kernel interfaces. We cannot iterate on
userspace interfaces, those have to be correct the first time.

Andrew

[0] https://www.kernel.org/doc/html/next/iio/iio_dmabuf_api.html

>>
>> This can be tested on QEMU with the following steps:
>> repo init -u https://github.com/jenswi-linaro/manifest.git -m qemu_v8.xml \
>>          -b prototype/sdp-v1
>> repo sync -j8
>> cd build
>> make toolchains -j4
>> make all -j$(nproc)
>> make run-only
>> # login and at the prompt:
>> xtest --sdp-basic
>>
>> https://optee.readthedocs.io/en/latest/building/prerequisites.html
>> list dependencies needed to build the above.
>>
>> The tests are pretty basic, mostly checking that a Trusted Application in
>> the secure world can access and manipulate the memory.
> 
> - Can we test that the system doesn't crash badly if user provides
>    non-secured memory to the users which expect a secure buffer?
> 
> - At the same time corresponding entities shouldn't decode data to the
>    buffers accessible to the rest of the sytem.
> 
>>
>> Cheers,
>> Jens
>>
>> [1] https://lore.kernel.org/dri-devel/20240515112308.10171-1-yong.wu@mediatek.com/
>> [2] https://lore.kernel.org/lkml/20220805135330.970-1-olivier.masse@nxp.com/
>>
>> Changes since Olivier's post [2]:
>> * Based on Yong Wu's post [1] where much of dma-buf handling is done in
>>    the generic restricted heap
>> * Simplifications and cleanup
>> * New commit message for "dma-buf: heaps: add Linaro restricted dmabuf heap
>>    support"
>> * Replaced the word "secure" with "restricted" where applicable
>>
>> Etienne Carriere (1):
>>    tee: new ioctl to a register tee_shm from a dmabuf file descriptor
>>
>> Jens Wiklander (2):
>>    dma-buf: heaps: restricted_heap: add no_map attribute
>>    dma-buf: heaps: add Linaro restricted dmabuf heap support
>>
>> Olivier Masse (1):
>>    dt-bindings: reserved-memory: add linaro,restricted-heap
>>
>>   .../linaro,restricted-heap.yaml               |  56 ++++++
>>   drivers/dma-buf/heaps/Kconfig                 |  10 ++
>>   drivers/dma-buf/heaps/Makefile                |   1 +
>>   drivers/dma-buf/heaps/restricted_heap.c       |  17 +-
>>   drivers/dma-buf/heaps/restricted_heap.h       |   2 +
>>   .../dma-buf/heaps/restricted_heap_linaro.c    | 165 ++++++++++++++++++
>>   drivers/tee/tee_core.c                        |  38 ++++
>>   drivers/tee/tee_shm.c                         | 104 ++++++++++-
>>   include/linux/tee_drv.h                       |  11 ++
>>   include/uapi/linux/tee.h                      |  29 +++
>>   10 files changed, 426 insertions(+), 7 deletions(-)
>>   create mode 100644 Documentation/devicetree/bindings/reserved-memory/linaro,restricted-heap.yaml
>>   create mode 100644 drivers/dma-buf/heaps/restricted_heap_linaro.c
>>
>> -- 
>> 2.34.1
>>
> 

Re: [RFC PATCH 0/4] Linaro restricted heap

[Daniel Stone]

Hi Jens,

On Fri, 30 Aug 2024 at 08:04, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> This patch set is based on top of Yong Wu's restricted heap patch set [1].
> It's also a continuation on Olivier's Add dma-buf secure-heap patch set [2].
>
> The Linaro restricted heap uses genalloc in the kernel to manage the heap
> carvout. This is a difference from the Mediatek restricted heap which
> relies on the secure world to manage the carveout.

Calling this the 'genalloc heap' would be much more clear.

Cheers,
Daniel

Re: [RFC PATCH 0/4] Linaro restricted heap

[Dmitry Baryshkov]

On Tue, Sep 24, 2024 at 01:13:18PM GMT, Andrew Davis wrote:
> On 9/23/24 1:33 AM, Dmitry Baryshkov wrote:
> > Hi,
> > 
> > On Fri, Aug 30, 2024 at 09:03:47AM GMT, Jens Wiklander wrote:
> > > Hi,
> > > 
> > > This patch set is based on top of Yong Wu's restricted heap patch set [1].
> > > It's also a continuation on Olivier's Add dma-buf secure-heap patch set [2].
> > > 
> > > The Linaro restricted heap uses genalloc in the kernel to manage the heap
> > > carvout. This is a difference from the Mediatek restricted heap which
> > > relies on the secure world to manage the carveout.
> > > 
> > > I've tried to adress the comments on [2], but [1] introduces changes so I'm
> > > afraid I've had to skip some comments.
> > 
> > I know I have raised the same question during LPC (in connection to
> > Qualcomm's dma-heap implementation). Is there any reason why we are
> > using generic heaps instead of allocating the dma-bufs on the device
> > side?
> > 
> > In your case you already have TEE device, you can use it to allocate and
> > export dma-bufs, which then get imported by the V4L and DRM drivers.
> > 
> 
> This goes to the heart of why we have dma-heaps in the first place.
> We don't want to burden userspace with having to figure out the right
> place to get a dma-buf for a given use-case on a given hardware.
> That would be very non-portable, and fail at the core purpose of
> a kernel: to abstract hardware specifics away.

Unfortunately all proposals to use dma-buf heaps were moving in the
described direction: let app select (somehow) from a platform- and
vendor- specific list of dma-buf heaps. In the kernel we at least know
the platform on which the system is running. Userspace generally doesn't
(and shouldn't). As such, it seems better to me to keep the knowledge in
the kernel and allow userspace do its job by calling into existing
device drivers.

> 
> Worse, the actual interface for dma-buf exporting changes from
> framework to framework (getting a dma-buf from DRM is different
> than V4L, and there would be yet another API for TEE, etc..)

But if the app is working with the particular subsystem, then it already
talks its language. Allocating a dma-buf is just another part of the
interface, which the app already has to support.

> Most subsystem don't need an allocator, they work just fine
> simply being only dma-bufs importers. Recent example being the
> IIO subsystem[0], for which some early posting included an
> allocator, but in the end, all that was needed was to consume
> buffers.
> 
> For devices that don't actually contain memory there is no
> reason to be an exporter. What most want is just to consume
> normal system memory. Or system memory with some constraints
> (e.g. contiguous, coherent, restricted, etc..).

... secure, accessible only to the camera and video encoder, ... or
accessible only to the video decoder and the display unit. Who specifies
those restrictions? Can we express them in a platform-neutral way?

> 
> > I have a feeling (I might be completely wrong here) that by using
> > generic dma-buf heaps we can easily end up in a situation when the
> > userspace depends heavily on the actual platform being used (to map the
> > platform to heap names). I think we should instead depend on the
> > existing devices (e.g. if there is a TEE device, use an IOCTL to
> > allocate secured DMA BUF from it, otherwise check for QTEE device,
> > otherwise check for some other vendor device).
> > 
> > The mental experiment to check if the API is correct is really simple:
> > Can you use exactly the same rootfs on several devices without
> > any additional tuning (e.g. your QEMU, HiKey, a Mediatek board, Qualcomm
> > laptop, etc)?
> > 
> 
> This is a great north star to follow. And exactly the reason we should
> *not* be exposing device specific constraints to userspace. The constrains
> change based on the platform. So a userspace would have to also pick
> a different set of constraints based on each platform.

Great, I totally agree here.

> Userspace knows which subsystems it will attach a buffer, and the
> kernel knows what constraints those devices have on a given platform.
> Ideal case is then allocate from the one exporter, attach to various
> devices, and have the constraints solved at map time by the exporter
> based on the set of attached devices.
> 
> For example, on one platform the display needs contiguous buffers,
> but on a different platform the display can scatter-gather. So
> what heap should our generic application allocate from when it
> wants a buffer consumable by the display, CMA or System?
> Answer *should* be always use the generic exporter, and that
> exporter then picks the right backing type based on the platform.

The display can support scather-gather, the GPU needs bigger stride for
this particular format and the video encoder decoder can not support SG.
Which set of constraints and which buffer size should generic exporter
select?

> Userspace shouldn't be dealing with any of these constraints
> (looking back, adding the CMA heap was probably incorrect,
> and the System heap should have been the only one. Idea back
> then was a userspace helper would show up to do the constraint
> solving and pick the right heap. That has yet to materialize and
> folks are still just hardcoding which heap to use..).
> 
> Same for this restricted heap, I'd like to explore if we can
> enhance the System heap such that when attached to the TEE framework,
> the backing memory is either made restricted by fire-walling,
> or allocating from a TEE carveout (based on platform).

Firewalling from which devices? Or rather allowing access from which
devices? Is it possible to specify that somehow?

> This will mean more inter-subsystem coordination, but we can
> iterate on these in kernel interfaces. We cannot iterate on
> userspace interfaces, those have to be correct the first time.
> 
> Andrew
> 
> [0] https://www.kernel.org/doc/html/next/iio/iio_dmabuf_api.html
> 
> > > 
> > > This can be tested on QEMU with the following steps:
> > > repo init -u https://github.com/jenswi-linaro/manifest.git -m qemu_v8.xml \
> > >          -b prototype/sdp-v1
> > > repo sync -j8
> > > cd build
> > > make toolchains -j4
> > > make all -j$(nproc)
> > > make run-only
> > > # login and at the prompt:
> > > xtest --sdp-basic
> > > 
> > > https://optee.readthedocs.io/en/latest/building/prerequisites.html
> > > list dependencies needed to build the above.
> > > 
> > > The tests are pretty basic, mostly checking that a Trusted Application in
> > > the secure world can access and manipulate the memory.
> > 
> > - Can we test that the system doesn't crash badly if user provides
> >    non-secured memory to the users which expect a secure buffer?
> > 
> > - At the same time corresponding entities shouldn't decode data to the
> >    buffers accessible to the rest of the sytem.
> > 
> > > 
> > > Cheers,
> > > Jens
> > > 
> > > [1] https://lore.kernel.org/dri-devel/20240515112308.10171-1-yong.wu@mediatek.com/
> > > [2] https://lore.kernel.org/lkml/20220805135330.970-1-olivier.masse@nxp.com/
> > > 
> > > Changes since Olivier's post [2]:
> > > * Based on Yong Wu's post [1] where much of dma-buf handling is done in
> > >    the generic restricted heap
> > > * Simplifications and cleanup
> > > * New commit message for "dma-buf: heaps: add Linaro restricted dmabuf heap
> > >    support"
> > > * Replaced the word "secure" with "restricted" where applicable
> > > 
> > > Etienne Carriere (1):
> > >    tee: new ioctl to a register tee_shm from a dmabuf file descriptor
> > > 
> > > Jens Wiklander (2):
> > >    dma-buf: heaps: restricted_heap: add no_map attribute
> > >    dma-buf: heaps: add Linaro restricted dmabuf heap support
> > > 
> > > Olivier Masse (1):
> > >    dt-bindings: reserved-memory: add linaro,restricted-heap
> > > 
> > >   .../linaro,restricted-heap.yaml               |  56 ++++++
> > >   drivers/dma-buf/heaps/Kconfig                 |  10 ++
> > >   drivers/dma-buf/heaps/Makefile                |   1 +
> > >   drivers/dma-buf/heaps/restricted_heap.c       |  17 +-
> > >   drivers/dma-buf/heaps/restricted_heap.h       |   2 +
> > >   .../dma-buf/heaps/restricted_heap_linaro.c    | 165 ++++++++++++++++++
> > >   drivers/tee/tee_core.c                        |  38 ++++
> > >   drivers/tee/tee_shm.c                         | 104 ++++++++++-
> > >   include/linux/tee_drv.h                       |  11 ++
> > >   include/uapi/linux/tee.h                      |  29 +++
> > >   10 files changed, 426 insertions(+), 7 deletions(-)
> > >   create mode 100644 Documentation/devicetree/bindings/reserved-memory/linaro,restricted-heap.yaml
> > >   create mode 100644 drivers/dma-buf/heaps/restricted_heap_linaro.c
> > > 
> > > -- 
> > > 2.34.1
> > > 
> > 

-- 
With best wishes
Dmitry

Re: [RFC PATCH 0/4] Linaro restricted heap

[Jens Wiklander]

On Mon, Sep 23, 2024 at 09:33:29AM +0300, Dmitry Baryshkov wrote:
> Hi,
> 
> On Fri, Aug 30, 2024 at 09:03:47AM GMT, Jens Wiklander wrote:
> > Hi,
> > 
> > This patch set is based on top of Yong Wu's restricted heap patch set [1].
> > It's also a continuation on Olivier's Add dma-buf secure-heap patch set [2].
> > 
> > The Linaro restricted heap uses genalloc in the kernel to manage the heap
> > carvout. This is a difference from the Mediatek restricted heap which
> > relies on the secure world to manage the carveout.
> > 
> > I've tried to adress the comments on [2], but [1] introduces changes so I'm
> > afraid I've had to skip some comments.
> 
> I know I have raised the same question during LPC (in connection to
> Qualcomm's dma-heap implementation). Is there any reason why we are
> using generic heaps instead of allocating the dma-bufs on the device
> side?
> 
> In your case you already have TEE device, you can use it to allocate and
> export dma-bufs, which then get imported by the V4L and DRM drivers.
> 
> I have a feeling (I might be completely wrong here) that by using
> generic dma-buf heaps we can easily end up in a situation when the
> userspace depends heavily on the actual platform being used (to map the
> platform to heap names). I think we should instead depend on the
> existing devices (e.g. if there is a TEE device, use an IOCTL to
> allocate secured DMA BUF from it, otherwise check for QTEE device,
> otherwise check for some other vendor device).

That makes sense, it's similar to what we do with TEE_IOC_SHM_ALLOC
where we allocate from a carveout reserverd for shared memory with the
secure world. It was even based on dma-buf until commit dfd0743f1d9e
("tee: handle lookup of shm with reference count 0").

We should use a new TEE_IOC_*_ALLOC for these new dma-bufs to avoid
confusion and to have more freedom when designing the interface.

> 
> The mental experiment to check if the API is correct is really simple:
> Can you use exactly the same rootfs on several devices without
> any additional tuning (e.g. your QEMU, HiKey, a Mediatek board, Qualcomm
> laptop, etc)?

No, I don't think so.

> 
> > 
> > This can be tested on QEMU with the following steps:
> > repo init -u https://github.com/jenswi-linaro/manifest.git -m qemu_v8.xml \
> >         -b prototype/sdp-v1
> > repo sync -j8
> > cd build
> > make toolchains -j4
> > make all -j$(nproc)
> > make run-only
> > # login and at the prompt:
> > xtest --sdp-basic
> > 
> > https://optee.readthedocs.io/en/latest/building/prerequisites.html
> > list dependencies needed to build the above.
> > 
> > The tests are pretty basic, mostly checking that a Trusted Application in
> > the secure world can access and manipulate the memory.
> 
> - Can we test that the system doesn't crash badly if user provides
>   non-secured memory to the users which expect a secure buffer?
> 
> - At the same time corresponding entities shouldn't decode data to the
>   buffers accessible to the rest of the sytem.

I'll a few tests along that.

Thanks,
Jens

> 
> > 
> > Cheers,
> > Jens
> > 
> > [1] https://lore.kernel.org/dri-devel/20240515112308.10171-1-yong.wu@mediatek.com/
> > [2] https://lore.kernel.org/lkml/20220805135330.970-1-olivier.masse@nxp.com/
> > 
> > Changes since Olivier's post [2]:
> > * Based on Yong Wu's post [1] where much of dma-buf handling is done in
> >   the generic restricted heap
> > * Simplifications and cleanup
> > * New commit message for "dma-buf: heaps: add Linaro restricted dmabuf heap
> >   support"
> > * Replaced the word "secure" with "restricted" where applicable
> > 
> > Etienne Carriere (1):
> >   tee: new ioctl to a register tee_shm from a dmabuf file descriptor
> > 
> > Jens Wiklander (2):
> >   dma-buf: heaps: restricted_heap: add no_map attribute
> >   dma-buf: heaps: add Linaro restricted dmabuf heap support
> > 
> > Olivier Masse (1):
> >   dt-bindings: reserved-memory: add linaro,restricted-heap
> > 
> >  .../linaro,restricted-heap.yaml               |  56 ++++++
> >  drivers/dma-buf/heaps/Kconfig                 |  10 ++
> >  drivers/dma-buf/heaps/Makefile                |   1 +
> >  drivers/dma-buf/heaps/restricted_heap.c       |  17 +-
> >  drivers/dma-buf/heaps/restricted_heap.h       |   2 +
> >  .../dma-buf/heaps/restricted_heap_linaro.c    | 165 ++++++++++++++++++
> >  drivers/tee/tee_core.c                        |  38 ++++
> >  drivers/tee/tee_shm.c                         | 104 ++++++++++-
> >  include/linux/tee_drv.h                       |  11 ++
> >  include/uapi/linux/tee.h                      |  29 +++
> >  10 files changed, 426 insertions(+), 7 deletions(-)
> >  create mode 100644 Documentation/devicetree/bindings/reserved-memory/linaro,restricted-heap.yaml
> >  create mode 100644 drivers/dma-buf/heaps/restricted_heap_linaro.c
> > 
> > -- 
> > 2.34.1
> > 
> 
> -- 
> With best wishes
> Dmitry

Re: [RFC PATCH 0/4] Linaro restricted heap

[Jens Wiklander]

Hi,

On Tue, Sep 24, 2024 at 01:13:18PM -0500, Andrew Davis wrote:
> On 9/23/24 1:33 AM, Dmitry Baryshkov wrote:
> > Hi,
> > 
> > On Fri, Aug 30, 2024 at 09:03:47AM GMT, Jens Wiklander wrote:
> > > Hi,
> > > 
> > > This patch set is based on top of Yong Wu's restricted heap patch set [1].
> > > It's also a continuation on Olivier's Add dma-buf secure-heap patch set [2].
> > > 
> > > The Linaro restricted heap uses genalloc in the kernel to manage the heap
> > > carvout. This is a difference from the Mediatek restricted heap which
> > > relies on the secure world to manage the carveout.
> > > 
> > > I've tried to adress the comments on [2], but [1] introduces changes so I'm
> > > afraid I've had to skip some comments.
> > 
> > I know I have raised the same question during LPC (in connection to
> > Qualcomm's dma-heap implementation). Is there any reason why we are
> > using generic heaps instead of allocating the dma-bufs on the device
> > side?
> > 
> > In your case you already have TEE device, you can use it to allocate and
> > export dma-bufs, which then get imported by the V4L and DRM drivers.
> > 
> 
> This goes to the heart of why we have dma-heaps in the first place.
> We don't want to burden userspace with having to figure out the right
> place to get a dma-buf for a given use-case on a given hardware.
> That would be very non-portable, and fail at the core purpose of
> a kernel: to abstract hardware specifics away.
> 
> Worse, the actual interface for dma-buf exporting changes from
> framework to framework (getting a dma-buf from DRM is different
> than V4L, and there would be yet another API for TEE, etc..)
> 
> Most subsystem don't need an allocator, they work just fine
> simply being only dma-bufs importers. Recent example being the
> IIO subsystem[0], for which some early posting included an
> allocator, but in the end, all that was needed was to consume
> buffers.
> 
> For devices that don't actually contain memory there is no
> reason to be an exporter. What most want is just to consume
> normal system memory. Or system memory with some constraints
> (e.g. contiguous, coherent, restricted, etc..).
> 
> > I have a feeling (I might be completely wrong here) that by using
> > generic dma-buf heaps we can easily end up in a situation when the
> > userspace depends heavily on the actual platform being used (to map the
> > platform to heap names). I think we should instead depend on the
> > existing devices (e.g. if there is a TEE device, use an IOCTL to
> > allocate secured DMA BUF from it, otherwise check for QTEE device,
> > otherwise check for some other vendor device).
> > 
> > The mental experiment to check if the API is correct is really simple:
> > Can you use exactly the same rootfs on several devices without
> > any additional tuning (e.g. your QEMU, HiKey, a Mediatek board, Qualcomm
> > laptop, etc)?
> > 
> 
> This is a great north star to follow. And exactly the reason we should
> *not* be exposing device specific constraints to userspace. The constrains
> change based on the platform. So a userspace would have to also pick
> a different set of constraints based on each platform.
> 
> Userspace knows which subsystems it will attach a buffer, and the
> kernel knows what constraints those devices have on a given platform.
> Ideal case is then allocate from the one exporter, attach to various
> devices, and have the constraints solved at map time by the exporter
> based on the set of attached devices.
> 
> For example, on one platform the display needs contiguous buffers,
> but on a different platform the display can scatter-gather. So
> what heap should our generic application allocate from when it
> wants a buffer consumable by the display, CMA or System?
> Answer *should* be always use the generic exporter, and that
> exporter then picks the right backing type based on the platform.
> 
> Userspace shouldn't be dealing with any of these constraints
> (looking back, adding the CMA heap was probably incorrect,
> and the System heap should have been the only one. Idea back
> then was a userspace helper would show up to do the constraint
> solving and pick the right heap. That has yet to materialize and
> folks are still just hardcoding which heap to use..).
> 
> Same for this restricted heap, I'd like to explore if we can
> enhance the System heap such that when attached to the TEE framework,
> the backing memory is either made restricted by fire-walling,
> or allocating from a TEE carveout (based on platform).

So the exporter (you mentioned System heap) will somehow know how to
interact with the TEE subsystem to allocate suitable memory?

I suppose the memory could be from a static carveout, dynamic restricted
memory allocation, or how to turn normal memory into restricted memory
(fire-walling), depending on the platform.

> 
> This will mean more inter-subsystem coordination, but we can
> iterate on these in kernel interfaces. We cannot iterate on
> userspace interfaces, those have to be correct the first time.

Good point, this approach should make it easier for userspace.

Thanks,
Jens

> 
> Andrew
> 
> [0] https://www.kernel.org/doc/html/next/iio/iio_dmabuf_api.html
> 
> > > 
> > > This can be tested on QEMU with the following steps:
> > > repo init -u https://github.com/jenswi-linaro/manifest.git -m qemu_v8.xml \
> > >          -b prototype/sdp-v1
> > > repo sync -j8
> > > cd build
> > > make toolchains -j4
> > > make all -j$(nproc)
> > > make run-only
> > > # login and at the prompt:
> > > xtest --sdp-basic
> > > 
> > > https://optee.readthedocs.io/en/latest/building/prerequisites.html
> > > list dependencies needed to build the above.
> > > 
> > > The tests are pretty basic, mostly checking that a Trusted Application in
> > > the secure world can access and manipulate the memory.
> > 
> > - Can we test that the system doesn't crash badly if user provides
> >    non-secured memory to the users which expect a secure buffer?
> > 
> > - At the same time corresponding entities shouldn't decode data to the
> >    buffers accessible to the rest of the sytem.
> > 
> > > 
> > > Cheers,
> > > Jens
> > > 
> > > [1] https://lore.kernel.org/dri-devel/20240515112308.10171-1-yong.wu@mediatek.com/
> > > [2] https://lore.kernel.org/lkml/20220805135330.970-1-olivier.masse@nxp.com/
> > > 
> > > Changes since Olivier's post [2]:
> > > * Based on Yong Wu's post [1] where much of dma-buf handling is done in
> > >    the generic restricted heap
> > > * Simplifications and cleanup
> > > * New commit message for "dma-buf: heaps: add Linaro restricted dmabuf heap
> > >    support"
> > > * Replaced the word "secure" with "restricted" where applicable
> > > 
> > > Etienne Carriere (1):
> > >    tee: new ioctl to a register tee_shm from a dmabuf file descriptor
> > > 
> > > Jens Wiklander (2):
> > >    dma-buf: heaps: restricted_heap: add no_map attribute
> > >    dma-buf: heaps: add Linaro restricted dmabuf heap support
> > > 
> > > Olivier Masse (1):
> > >    dt-bindings: reserved-memory: add linaro,restricted-heap
> > > 
> > >   .../linaro,restricted-heap.yaml               |  56 ++++++
> > >   drivers/dma-buf/heaps/Kconfig                 |  10 ++
> > >   drivers/dma-buf/heaps/Makefile                |   1 +
> > >   drivers/dma-buf/heaps/restricted_heap.c       |  17 +-
> > >   drivers/dma-buf/heaps/restricted_heap.h       |   2 +
> > >   .../dma-buf/heaps/restricted_heap_linaro.c    | 165 ++++++++++++++++++
> > >   drivers/tee/tee_core.c                        |  38 ++++
> > >   drivers/tee/tee_shm.c                         | 104 ++++++++++-
> > >   include/linux/tee_drv.h                       |  11 ++
> > >   include/uapi/linux/tee.h                      |  29 +++
> > >   10 files changed, 426 insertions(+), 7 deletions(-)
> > >   create mode 100644 Documentation/devicetree/bindings/reserved-memory/linaro,restricted-heap.yaml
> > >   create mode 100644 drivers/dma-buf/heaps/restricted_heap_linaro.c
> > > 
> > > -- 
> > > 2.34.1
> > > 
> > 

Re: [RFC PATCH 0/4] Linaro restricted heap

[Dmitry Baryshkov]

On Wed, Sep 25, 2024 at 09:15:04AM GMT, Jens Wiklander wrote:
> On Mon, Sep 23, 2024 at 09:33:29AM +0300, Dmitry Baryshkov wrote:
> > Hi,
> > 
> > On Fri, Aug 30, 2024 at 09:03:47AM GMT, Jens Wiklander wrote:
> > > Hi,
> > > 
> > > This patch set is based on top of Yong Wu's restricted heap patch set [1].
> > > It's also a continuation on Olivier's Add dma-buf secure-heap patch set [2].
> > > 
> > > The Linaro restricted heap uses genalloc in the kernel to manage the heap
> > > carvout. This is a difference from the Mediatek restricted heap which
> > > relies on the secure world to manage the carveout.
> > > 
> > > I've tried to adress the comments on [2], but [1] introduces changes so I'm
> > > afraid I've had to skip some comments.
> > 
> > I know I have raised the same question during LPC (in connection to
> > Qualcomm's dma-heap implementation). Is there any reason why we are
> > using generic heaps instead of allocating the dma-bufs on the device
> > side?
> > 
> > In your case you already have TEE device, you can use it to allocate and
> > export dma-bufs, which then get imported by the V4L and DRM drivers.
> > 
> > I have a feeling (I might be completely wrong here) that by using
> > generic dma-buf heaps we can easily end up in a situation when the
> > userspace depends heavily on the actual platform being used (to map the
> > platform to heap names). I think we should instead depend on the
> > existing devices (e.g. if there is a TEE device, use an IOCTL to
> > allocate secured DMA BUF from it, otherwise check for QTEE device,
> > otherwise check for some other vendor device).
> 
> That makes sense, it's similar to what we do with TEE_IOC_SHM_ALLOC
> where we allocate from a carveout reserverd for shared memory with the
> secure world. It was even based on dma-buf until commit dfd0743f1d9e
> ("tee: handle lookup of shm with reference count 0").
> 
> We should use a new TEE_IOC_*_ALLOC for these new dma-bufs to avoid
> confusion and to have more freedom when designing the interface.
> 
> > 
> > The mental experiment to check if the API is correct is really simple:
> > Can you use exactly the same rootfs on several devices without
> > any additional tuning (e.g. your QEMU, HiKey, a Mediatek board, Qualcomm
> > laptop, etc)?
> 
> No, I don't think so.

Then the API needs to be modified.

Or the userspace needs to be modified in the way similar to Vulkan /
OpenCL / glvnd / VA / VDPU: platform-specific backends, coexisting on a
single rootfs.

It is more or less fine to have platform-specific rootfs when we are
talking about the embedded, resource-limited devices. But for the
end-user devices we must be able to install a generic distro with no
device-specific packages being selected.

> 
> > 
> > > 
> > > This can be tested on QEMU with the following steps:
> > > repo init -u https://github.com/jenswi-linaro/manifest.git -m qemu_v8.xml \
> > >         -b prototype/sdp-v1
> > > repo sync -j8
> > > cd build
> > > make toolchains -j4
> > > make all -j$(nproc)
> > > make run-only
> > > # login and at the prompt:
> > > xtest --sdp-basic
> > > 
> > > https://optee.readthedocs.io/en/latest/building/prerequisites.html
> > > list dependencies needed to build the above.
> > > 
> > > The tests are pretty basic, mostly checking that a Trusted Application in
> > > the secure world can access and manipulate the memory.
> > 
> > - Can we test that the system doesn't crash badly if user provides
> >   non-secured memory to the users which expect a secure buffer?
> > 
> > - At the same time corresponding entities shouldn't decode data to the
> >   buffers accessible to the rest of the sytem.
> 
> I'll a few tests along that.
> 
> Thanks,
> Jens
> 
> > 
> > > 
> > > Cheers,
> > > Jens
> > > 
> > > [1] https://lore.kernel.org/dri-devel/20240515112308.10171-1-yong.wu@mediatek.com/
> > > [2] https://lore.kernel.org/lkml/20220805135330.970-1-olivier.masse@nxp.com/
> > > 
> > > Changes since Olivier's post [2]:
> > > * Based on Yong Wu's post [1] where much of dma-buf handling is done in
> > >   the generic restricted heap
> > > * Simplifications and cleanup
> > > * New commit message for "dma-buf: heaps: add Linaro restricted dmabuf heap
> > >   support"
> > > * Replaced the word "secure" with "restricted" where applicable
> > > 
> > > Etienne Carriere (1):
> > >   tee: new ioctl to a register tee_shm from a dmabuf file descriptor
> > > 
> > > Jens Wiklander (2):
> > >   dma-buf: heaps: restricted_heap: add no_map attribute
> > >   dma-buf: heaps: add Linaro restricted dmabuf heap support
> > > 
> > > Olivier Masse (1):
> > >   dt-bindings: reserved-memory: add linaro,restricted-heap
> > > 
> > >  .../linaro,restricted-heap.yaml               |  56 ++++++
> > >  drivers/dma-buf/heaps/Kconfig                 |  10 ++
> > >  drivers/dma-buf/heaps/Makefile                |   1 +
> > >  drivers/dma-buf/heaps/restricted_heap.c       |  17 +-
> > >  drivers/dma-buf/heaps/restricted_heap.h       |   2 +
> > >  .../dma-buf/heaps/restricted_heap_linaro.c    | 165 ++++++++++++++++++
> > >  drivers/tee/tee_core.c                        |  38 ++++
> > >  drivers/tee/tee_shm.c                         | 104 ++++++++++-
> > >  include/linux/tee_drv.h                       |  11 ++
> > >  include/uapi/linux/tee.h                      |  29 +++
> > >  10 files changed, 426 insertions(+), 7 deletions(-)
> > >  create mode 100644 Documentation/devicetree/bindings/reserved-memory/linaro,restricted-heap.yaml
> > >  create mode 100644 drivers/dma-buf/heaps/restricted_heap_linaro.c
> > > 
> > > -- 
> > > 2.34.1
> > > 
> > 
> > -- 
> > With best wishes
> > Dmitry

-- 
With best wishes
Dmitry

Re: [Linaro-mm-sig] Re: [RFC PATCH 0/4] Linaro restricted heap

[Dmitry Baryshkov]

On Wed, Sep 25, 2024 at 10:51:15AM GMT, Christian König wrote:
> Am 25.09.24 um 01:05 schrieb Dmitry Baryshkov:
> > On Tue, Sep 24, 2024 at 01:13:18PM GMT, Andrew Davis wrote:
> > > On 9/23/24 1:33 AM, Dmitry Baryshkov wrote:
> > > > Hi,
> > > > 
> > > > On Fri, Aug 30, 2024 at 09:03:47AM GMT, Jens Wiklander wrote:
> > > > > Hi,
> > > > > 
> > > > > This patch set is based on top of Yong Wu's restricted heap patch set [1].
> > > > > It's also a continuation on Olivier's Add dma-buf secure-heap patch set [2].
> > > > > 
> > > > > The Linaro restricted heap uses genalloc in the kernel to manage the heap
> > > > > carvout. This is a difference from the Mediatek restricted heap which
> > > > > relies on the secure world to manage the carveout.
> > > > > 
> > > > > I've tried to adress the comments on [2], but [1] introduces changes so I'm
> > > > > afraid I've had to skip some comments.
> > > > I know I have raised the same question during LPC (in connection to
> > > > Qualcomm's dma-heap implementation). Is there any reason why we are
> > > > using generic heaps instead of allocating the dma-bufs on the device
> > > > side?
> > > > 
> > > > In your case you already have TEE device, you can use it to allocate and
> > > > export dma-bufs, which then get imported by the V4L and DRM drivers.
> > > > 
> > > This goes to the heart of why we have dma-heaps in the first place.
> > > We don't want to burden userspace with having to figure out the right
> > > place to get a dma-buf for a given use-case on a given hardware.
> > > That would be very non-portable, and fail at the core purpose of
> > > a kernel: to abstract hardware specifics away.
> > Unfortunately all proposals to use dma-buf heaps were moving in the
> > described direction: let app select (somehow) from a platform- and
> > vendor- specific list of dma-buf heaps. In the kernel we at least know
> > the platform on which the system is running. Userspace generally doesn't
> > (and shouldn't). As such, it seems better to me to keep the knowledge in
> > the kernel and allow userspace do its job by calling into existing
> > device drivers.
> 
> The idea of letting the kernel fully abstract away the complexity of inter
> device data exchange is a completely failed design. There has been plenty of
> evidence for that over the years.
> 
> Because of this in DMA-buf it's an intentional design decision that
> userspace and *not* the kernel decides where and what to allocate from.

Hmm, ok.

> 
> What the kernel should provide are the necessary information what type of
> memory a device can work with and if certain memory is accessible or not.
> This is the part which is unfortunately still not well defined nor
> implemented at the moment.
> 
> Apart from that there are a whole bunch of intentional design decision which
> should prevent developers to move allocation decision inside the kernel. For
> example DMA-buf doesn't know what the content of the buffer is (except for
> it's total size) and which use cases a buffer will be used with.
> 
> So the question if memory should be exposed through DMA-heaps or a driver
> specific allocator is not a question of abstraction, but rather one of the
> physical location and accessibility of the memory.
> 
> If the memory is attached to any physical device, e.g. local memory on a
> dGPU, FPGA PCIe BAR, RDMA, camera internal memory etc, then expose the
> memory as device specific allocator.

So, for embedded systems with unified memory all buffers (maybe except
PCIe BARs) should come from DMA-BUF heaps, correct?

> 
> If the memory is not physically attached to any device, but rather just
> memory attached to the CPU or a system wide memory controller then expose
> the memory as DMA-heap with specific requirements (e.g. certain sized pages,
> contiguous, restricted, encrypted, ...).

Is encrypted / protected a part of the allocation contract or should it
be enforced separately via a call to TEE / SCM / anything else?

-- 
With best wishes
Dmitry

Re: [RFC PATCH 0/4] Linaro restricted heap

[Jens Wiklander]

On Wed, Sep 25, 2024 at 1:41 PM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On Wed, Sep 25, 2024 at 09:15:04AM GMT, Jens Wiklander wrote:
> > On Mon, Sep 23, 2024 at 09:33:29AM +0300, Dmitry Baryshkov wrote:
> > > Hi,
> > >
> > > On Fri, Aug 30, 2024 at 09:03:47AM GMT, Jens Wiklander wrote:
> > > > Hi,
> > > >
> > > > This patch set is based on top of Yong Wu's restricted heap patch set [1].
> > > > It's also a continuation on Olivier's Add dma-buf secure-heap patch set [2].
> > > >
> > > > The Linaro restricted heap uses genalloc in the kernel to manage the heap
> > > > carvout. This is a difference from the Mediatek restricted heap which
> > > > relies on the secure world to manage the carveout.
> > > >
> > > > I've tried to adress the comments on [2], but [1] introduces changes so I'm
> > > > afraid I've had to skip some comments.
> > >
> > > I know I have raised the same question during LPC (in connection to
> > > Qualcomm's dma-heap implementation). Is there any reason why we are
> > > using generic heaps instead of allocating the dma-bufs on the device
> > > side?
> > >
> > > In your case you already have TEE device, you can use it to allocate and
> > > export dma-bufs, which then get imported by the V4L and DRM drivers.
> > >
> > > I have a feeling (I might be completely wrong here) that by using
> > > generic dma-buf heaps we can easily end up in a situation when the
> > > userspace depends heavily on the actual platform being used (to map the
> > > platform to heap names). I think we should instead depend on the
> > > existing devices (e.g. if there is a TEE device, use an IOCTL to
> > > allocate secured DMA BUF from it, otherwise check for QTEE device,
> > > otherwise check for some other vendor device).
> >
> > That makes sense, it's similar to what we do with TEE_IOC_SHM_ALLOC
> > where we allocate from a carveout reserverd for shared memory with the
> > secure world. It was even based on dma-buf until commit dfd0743f1d9e
> > ("tee: handle lookup of shm with reference count 0").
> >
> > We should use a new TEE_IOC_*_ALLOC for these new dma-bufs to avoid
> > confusion and to have more freedom when designing the interface.
> >
> > >
> > > The mental experiment to check if the API is correct is really simple:
> > > Can you use exactly the same rootfs on several devices without
> > > any additional tuning (e.g. your QEMU, HiKey, a Mediatek board, Qualcomm
> > > laptop, etc)?
> >
> > No, I don't think so.
>
> Then the API needs to be modified.

I don't think that is enough. I would have answered no even without
the secure data path in mind. Communication with the secure world is
still too fragmented.

>
> Or the userspace needs to be modified in the way similar to Vulkan /
> OpenCL / glvnd / VA / VDPU: platform-specific backends, coexisting on a
> single rootfs.

Yes, that's likely a needed step. But the first step is to have
something to relate to upstream, without that there's only an
ever-changing downstream ABI.

>
> It is more or less fine to have platform-specific rootfs when we are
> talking about the embedded, resource-limited devices. But for the
> end-user devices we must be able to install a generic distro with no
> device-specific packages being selected.

I'm not sure we can solve that problem here. But we should of course
not make matters worse. In the restricted heap patch set which this
patchset builds on we define a way to allocate memory from a
restricted heap, but we leave the problem of finding the right heap to
userspace.

Thanks,
Jens

>
> >
> > >
> > > >
> > > > This can be tested on QEMU with the following steps:
> > > > repo init -u https://github.com/jenswi-linaro/manifest.git -m qemu_v8.xml \
> > > >         -b prototype/sdp-v1
> > > > repo sync -j8
> > > > cd build
> > > > make toolchains -j4
> > > > make all -j$(nproc)
> > > > make run-only
> > > > # login and at the prompt:
> > > > xtest --sdp-basic
> > > >
> > > > https://optee.readthedocs.io/en/latest/building/prerequisites.html
> > > > list dependencies needed to build the above.
> > > >
> > > > The tests are pretty basic, mostly checking that a Trusted Application in
> > > > the secure world can access and manipulate the memory.
> > >
> > > - Can we test that the system doesn't crash badly if user provides
> > >   non-secured memory to the users which expect a secure buffer?
> > >
> > > - At the same time corresponding entities shouldn't decode data to the
> > >   buffers accessible to the rest of the sytem.
> >
> > I'll a few tests along that.
> >
> > Thanks,
> > Jens
> >
> > >
> > > >
> > > > Cheers,
> > > > Jens
> > > >
> > > > [1] https://lore.kernel.org/dri-devel/20240515112308.10171-1-yong.wu@mediatek.com/
> > > > [2] https://lore.kernel.org/lkml/20220805135330.970-1-olivier.masse@nxp.com/
> > > >
> > > > Changes since Olivier's post [2]:
> > > > * Based on Yong Wu's post [1] where much of dma-buf handling is done in
> > > >   the generic restricted heap
> > > > * Simplifications and cleanup
> > > > * New commit message for "dma-buf: heaps: add Linaro restricted dmabuf heap
> > > >   support"
> > > > * Replaced the word "secure" with "restricted" where applicable
> > > >
> > > > Etienne Carriere (1):
> > > >   tee: new ioctl to a register tee_shm from a dmabuf file descriptor
> > > >
> > > > Jens Wiklander (2):
> > > >   dma-buf: heaps: restricted_heap: add no_map attribute
> > > >   dma-buf: heaps: add Linaro restricted dmabuf heap support
> > > >
> > > > Olivier Masse (1):
> > > >   dt-bindings: reserved-memory: add linaro,restricted-heap
> > > >
> > > >  .../linaro,restricted-heap.yaml               |  56 ++++++
> > > >  drivers/dma-buf/heaps/Kconfig                 |  10 ++
> > > >  drivers/dma-buf/heaps/Makefile                |   1 +
> > > >  drivers/dma-buf/heaps/restricted_heap.c       |  17 +-
> > > >  drivers/dma-buf/heaps/restricted_heap.h       |   2 +
> > > >  .../dma-buf/heaps/restricted_heap_linaro.c    | 165 ++++++++++++++++++
> > > >  drivers/tee/tee_core.c                        |  38 ++++
> > > >  drivers/tee/tee_shm.c                         | 104 ++++++++++-
> > > >  include/linux/tee_drv.h                       |  11 ++
> > > >  include/uapi/linux/tee.h                      |  29 +++
> > > >  10 files changed, 426 insertions(+), 7 deletions(-)
> > > >  create mode 100644 Documentation/devicetree/bindings/reserved-memory/linaro,restricted-heap.yaml
> > > >  create mode 100644 drivers/dma-buf/heaps/restricted_heap_linaro.c
> > > >
> > > > --
> > > > 2.34.1
> > > >
> > >
> > > --
> > > With best wishes
> > > Dmitry
>
> --
> With best wishes
> Dmitry

Re: [Linaro-mm-sig] Re: [RFC PATCH 0/4] Linaro restricted heap

[Sumit Garg]

[Resend in plain text format as my earlier message was rejected by
some mailing lists]

On Thu, 26 Sept 2024 at 19:17, Sumit Garg <sumit.garg@linaro.org> wrote:
>
> On 9/25/24 19:31, Christian König wrote:
>
> Am 25.09.24 um 14:51 schrieb Dmitry Baryshkov:
>
> On Wed, Sep 25, 2024 at 10:51:15AM GMT, Christian König wrote:
>
> Am 25.09.24 um 01:05 schrieb Dmitry Baryshkov:
>
> On Tue, Sep 24, 2024 at 01:13:18PM GMT, Andrew Davis wrote:
>
> On 9/23/24 1:33 AM, Dmitry Baryshkov wrote:
>
> Hi,
>
> On Fri, Aug 30, 2024 at 09:03:47AM GMT, Jens Wiklander wrote:
>
> Hi,
>
> This patch set is based on top of Yong Wu's restricted heap patch set [1].
> It's also a continuation on Olivier's Add dma-buf secure-heap patch set [2].
>
> The Linaro restricted heap uses genalloc in the kernel to manage the heap
> carvout. This is a difference from the Mediatek restricted heap which
> relies on the secure world to manage the carveout.
>
> I've tried to adress the comments on [2], but [1] introduces changes so I'm
> afraid I've had to skip some comments.
>
> I know I have raised the same question during LPC (in connection to
> Qualcomm's dma-heap implementation). Is there any reason why we are
> using generic heaps instead of allocating the dma-bufs on the device
> side?
>
> In your case you already have TEE device, you can use it to allocate and
> export dma-bufs, which then get imported by the V4L and DRM drivers.
>
> This goes to the heart of why we have dma-heaps in the first place.
> We don't want to burden userspace with having to figure out the right
> place to get a dma-buf for a given use-case on a given hardware.
> That would be very non-portable, and fail at the core purpose of
> a kernel: to abstract hardware specifics away.
>
> Unfortunately all proposals to use dma-buf heaps were moving in the
> described direction: let app select (somehow) from a platform- and
> vendor- specific list of dma-buf heaps. In the kernel we at least know
> the platform on which the system is running. Userspace generally doesn't
> (and shouldn't). As such, it seems better to me to keep the knowledge in
> the kernel and allow userspace do its job by calling into existing
> device drivers.
>
> The idea of letting the kernel fully abstract away the complexity of inter
> device data exchange is a completely failed design. There has been plenty of
> evidence for that over the years.
>
> Because of this in DMA-buf it's an intentional design decision that
> userspace and *not* the kernel decides where and what to allocate from.
>
> Hmm, ok.
>
> What the kernel should provide are the necessary information what type of
> memory a device can work with and if certain memory is accessible or not.
> This is the part which is unfortunately still not well defined nor
> implemented at the moment.
>
> Apart from that there are a whole bunch of intentional design decision which
> should prevent developers to move allocation decision inside the kernel. For
> example DMA-buf doesn't know what the content of the buffer is (except for
> it's total size) and which use cases a buffer will be used with.
>
> So the question if memory should be exposed through DMA-heaps or a driver
> specific allocator is not a question of abstraction, but rather one of the
> physical location and accessibility of the memory.
>
> If the memory is attached to any physical device, e.g. local memory on a
> dGPU, FPGA PCIe BAR, RDMA, camera internal memory etc, then expose the
> memory as device specific allocator.
>
> So, for embedded systems with unified memory all buffers (maybe except
> PCIe BARs) should come from DMA-BUF heaps, correct?
>
>
> From what I know that is correct, yes. Question is really if that will stay this way.
>
> Neural accelerators look a lot stripped down FPGAs these days and the benefit of local memory for GPUs is known for decades.
>
> Could be that designs with local specialized memory see a revival any time, who knows.
>
> If the memory is not physically attached to any device, but rather just
> memory attached to the CPU or a system wide memory controller then expose
> the memory as DMA-heap with specific requirements (e.g. certain sized pages,
> contiguous, restricted, encrypted, ...).
>
> Is encrypted / protected a part of the allocation contract or should it
> be enforced separately via a call to TEE / SCM / anything else?
>
>
> Well that is a really good question I can't fully answer either. From what I know now I would say it depends on the design.
>

IMHO, I think Dmitry's proposal to rather allow the TEE device to be
the allocator and exporter of DMA-bufs related to restricted memory
makes sense to me. Since it's really the TEE implementation (OP-TEE,
AMD-TEE, TS-TEE or future QTEE) which sets up the restrictions on a
particular piece of allocated memory. AFAIK, that happens after the
DMA-buf gets allocated and then user-space calls into TEE to set up
which media pipeline is going to access that particular DMA-buf. It
can also be a static contract depending on a particular platform
design.

As Jens noted in the other thread, we already manage shared memory
allocations (from a static carve-out or dynamically mapped) for
communications among Linux and TEE that were based on DMA-bufs earlier
but since we didn't required them to be shared with other devices, so
we rather switched to anonymous memory.

From user-space perspective, it's cleaner to use TEE device IOCTLs for
DMA-buf allocations since it already knows which underlying TEE
implementation it's communicating with rather than first figuring out
which DMA heap to use for allocation and then communicating with TEE
implementation.

-Sumit

Re: [Linaro-mm-sig] Re: [RFC PATCH 0/4] Linaro restricted heap

[Christian König]

Am 26.09.24 um 15:52 schrieb Sumit Garg:
> [Resend in plain text format as my earlier message was rejected by
> some mailing lists]
>
> On Thu, 26 Sept 2024 at 19:17, Sumit Garg <sumit.garg@linaro.org> wrote:
>> On 9/25/24 19:31, Christian König wrote:
>>
>> Am 25.09.24 um 14:51 schrieb Dmitry Baryshkov:
>>
>> On Wed, Sep 25, 2024 at 10:51:15AM GMT, Christian König wrote:
>>
>> Am 25.09.24 um 01:05 schrieb Dmitry Baryshkov:
>>
>> On Tue, Sep 24, 2024 at 01:13:18PM GMT, Andrew Davis wrote:
>>
>> On 9/23/24 1:33 AM, Dmitry Baryshkov wrote:
>>
>> Hi,
>>
>> On Fri, Aug 30, 2024 at 09:03:47AM GMT, Jens Wiklander wrote:
>>
>> Hi,
>>
>> This patch set is based on top of Yong Wu's restricted heap patch set [1].
>> It's also a continuation on Olivier's Add dma-buf secure-heap patch set [2].
>>
>> The Linaro restricted heap uses genalloc in the kernel to manage the heap
>> carvout. This is a difference from the Mediatek restricted heap which
>> relies on the secure world to manage the carveout.
>>
>> I've tried to adress the comments on [2], but [1] introduces changes so I'm
>> afraid I've had to skip some comments.
>>
>> I know I have raised the same question during LPC (in connection to
>> Qualcomm's dma-heap implementation). Is there any reason why we are
>> using generic heaps instead of allocating the dma-bufs on the device
>> side?
>>
>> In your case you already have TEE device, you can use it to allocate and
>> export dma-bufs, which then get imported by the V4L and DRM drivers.
>>
>> This goes to the heart of why we have dma-heaps in the first place.
>> We don't want to burden userspace with having to figure out the right
>> place to get a dma-buf for a given use-case on a given hardware.
>> That would be very non-portable, and fail at the core purpose of
>> a kernel: to abstract hardware specifics away.
>>
>> Unfortunately all proposals to use dma-buf heaps were moving in the
>> described direction: let app select (somehow) from a platform- and
>> vendor- specific list of dma-buf heaps. In the kernel we at least know
>> the platform on which the system is running. Userspace generally doesn't
>> (and shouldn't). As such, it seems better to me to keep the knowledge in
>> the kernel and allow userspace do its job by calling into existing
>> device drivers.
>>
>> The idea of letting the kernel fully abstract away the complexity of inter
>> device data exchange is a completely failed design. There has been plenty of
>> evidence for that over the years.
>>
>> Because of this in DMA-buf it's an intentional design decision that
>> userspace and *not* the kernel decides where and what to allocate from.
>>
>> Hmm, ok.
>>
>> What the kernel should provide are the necessary information what type of
>> memory a device can work with and if certain memory is accessible or not.
>> This is the part which is unfortunately still not well defined nor
>> implemented at the moment.
>>
>> Apart from that there are a whole bunch of intentional design decision which
>> should prevent developers to move allocation decision inside the kernel. For
>> example DMA-buf doesn't know what the content of the buffer is (except for
>> it's total size) and which use cases a buffer will be used with.
>>
>> So the question if memory should be exposed through DMA-heaps or a driver
>> specific allocator is not a question of abstraction, but rather one of the
>> physical location and accessibility of the memory.
>>
>> If the memory is attached to any physical device, e.g. local memory on a
>> dGPU, FPGA PCIe BAR, RDMA, camera internal memory etc, then expose the
>> memory as device specific allocator.
>>
>> So, for embedded systems with unified memory all buffers (maybe except
>> PCIe BARs) should come from DMA-BUF heaps, correct?
>>
>>
>>  From what I know that is correct, yes. Question is really if that will stay this way.
>>
>> Neural accelerators look a lot stripped down FPGAs these days and the benefit of local memory for GPUs is known for decades.
>>
>> Could be that designs with local specialized memory see a revival any time, who knows.
>>
>> If the memory is not physically attached to any device, but rather just
>> memory attached to the CPU or a system wide memory controller then expose
>> the memory as DMA-heap with specific requirements (e.g. certain sized pages,
>> contiguous, restricted, encrypted, ...).
>>
>> Is encrypted / protected a part of the allocation contract or should it
>> be enforced separately via a call to TEE / SCM / anything else?
>>
>>
>> Well that is a really good question I can't fully answer either. From what I know now I would say it depends on the design.
>>
> IMHO, I think Dmitry's proposal to rather allow the TEE device to be
> the allocator and exporter of DMA-bufs related to restricted memory
> makes sense to me. Since it's really the TEE implementation (OP-TEE,
> AMD-TEE, TS-TEE or future QTEE) which sets up the restrictions on a
> particular piece of allocated memory. AFAIK, that happens after the
> DMA-buf gets allocated and then user-space calls into TEE to set up
> which media pipeline is going to access that particular DMA-buf. It
> can also be a static contract depending on a particular platform
> design.
>
> As Jens noted in the other thread, we already manage shared memory
> allocations (from a static carve-out or dynamically mapped) for
> communications among Linux and TEE that were based on DMA-bufs earlier
> but since we didn't required them to be shared with other devices, so
> we rather switched to anonymous memory.
>
>  From user-space perspective, it's cleaner to use TEE device IOCTLs for
> DMA-buf allocations since it already knows which underlying TEE
> implementation it's communicating with rather than first figuring out
> which DMA heap to use for allocation and then communicating with TEE
> implementation.

+1

I'm not that deeply into the functionality the TEE device IOCTLs expose, 
so can't judge if what's said above is correct or not.

But in general building on top of existing infrastructure and 
information is a really strong argument for a design.

So from my 10 mile high point of view that sounds like the way to go.

Regards,
Christian.

>
> -Sumit

Re: [Linaro-mm-sig] Re: [RFC PATCH 0/4] Linaro restricted heap

[Andrew Davis]

On 9/25/24 3:51 AM, Christian König wrote:
> Am 25.09.24 um 01:05 schrieb Dmitry Baryshkov:
>> On Tue, Sep 24, 2024 at 01:13:18PM GMT, Andrew Davis wrote:
>>> On 9/23/24 1:33 AM, Dmitry Baryshkov wrote:
>>>> Hi,
>>>>
>>>> On Fri, Aug 30, 2024 at 09:03:47AM GMT, Jens Wiklander wrote:
>>>>> Hi,
>>>>>
>>>>> This patch set is based on top of Yong Wu's restricted heap patch set[1].
>>>>> It's also a continuation on Olivier's Add dma-buf secure-heap patch set [2].
>>>>>
>>>>> The Linaro restricted heap uses genalloc in the kernel to manage the heap
>>>>> carvout. This is a difference from the Mediatek restricted heap which
>>>>> relies on the secure world to manage the carveout.
>>>>>
>>>>> I've tried to adress the comments on [2], but [1] introduces changes so I'm
>>>>> afraid I've had to skip some comments.
>>>> I know I have raised the same question during LPC (in connection to
>>>> Qualcomm's dma-heap implementation). Is there any reason why we are
>>>> using generic heaps instead of allocating the dma-bufs on the device
>>>> side?
>>>>
>>>> In your case you already have TEE device, you can use it to allocate and
>>>> export dma-bufs, which then get imported by the V4L and DRM drivers.
>>>>
>>> This goes to the heart of why we have dma-heaps in the first place.
>>> We don't want to burden userspace with having to figure out the right
>>> place to get a dma-buf for a given use-case on a given hardware.
>>> That would be very non-portable, and fail at the core purpose of
>>> a kernel: to abstract hardware specifics away.
>> Unfortunately all proposals to use dma-buf heaps were moving in the
>> described direction: let app select (somehow) from a platform- and
>> vendor- specific list of dma-buf heaps. In the kernel we at least know
>> the platform on which the system is running. Userspace generally doesn't
>> (and shouldn't). As such, it seems better to me to keep the knowledge in
>> the kernel and allow userspace do its job by calling into existing
>> device drivers.
> 
> The idea of letting the kernel fully abstract away the complexity of inter device data exchange is a completely failed design. There has been plentyof evidence for that over the years.
> 

And forcing userspace to figure it all out is also an unsolved problem
after all these years. Neither side wants to get their hands dirty, but
it is fundamentally a kernel problem to handle these device complexities.

> Because of this in DMA-buf it's an intentional design decision that userspace and *not* the kernel decides where and what to allocate from.
> 

DMA-buf attach and map stages are split from each other, to me this indicates
the design intended the actual backing allocation to be chosen based on attached
devices at map time, not at allocation time. Meaning userspace doesn't really get
to choose the backing storage.

> What the kernel should provide are the necessary information what type ofmemory a device can work with and if certain memory is accessible or not. This is the part which is unfortunately still not well defined nor implemented at the moment.
> 

This sounds like the kernel provided "hints" solution. Given enough hints,
the correct Heap would become obvious, and so a complete hint based solution
is one step away from just having the kernel simply make that one correct
selection for you.

> Apart from that there are a whole bunch of intentional design decision which should prevent developers to move allocation decision inside the kernel. For example DMA-buf doesn't know what the content of the buffer is (except for it's total size) and which use cases a buffer will be used with.
> 
> So the question if memory should be exposed through DMA-heaps or a driverspecific allocator is not a question of abstraction, but rather one of thephysical location and accessibility of the memory.
> 
> If the memory is attached to any physical device, e.g. local memory on a dGPU, FPGA PCIe BAR, RDMA, camera internal memory etc, then expose the memory as device specific allocator.
> 
> If the memory is not physically attached to any device, but rather just memory attached to the CPU or a system wide memory controller then expose the memory as DMA-heap with specific requirements (e.g. certain sized pages, contiguous, restricted, encrypted, ...).
> 

Agree with the first part, some exporters are just giving out CPU memory
but with some specific constraint (some subsystems get a pass like V4L2
as it came out before DMA-heaps, but no reason to keep making new allocators
for each and every subsystem just because they may have some memory
constraint on the consumption side).

>>> Worse, the actual interface for dma-buf exporting changes from
>>> framework to framework (getting a dma-buf from DRM is different
>>> than V4L, and there would be yet another API for TEE, etc..)
>> But if the app is working with the particular subsystem, then it already
>> talks its language. Allocating a dma-buf is just another part of the
>> interface, which the app already has to support.
> 
> Correct, yes.
> 

Importing the buffer to a given subsystem with be subsystem specific yes,
but I don't see how that means allocating the buffer should have to be too.
For instance I need one of these new "restricted" heaps to pass to V4L2,
I'll need to know how to use V4L2 sure, but why should I have to deal with
the TEE subsystem to get that buffer as proposed? The common allocator
could have handled this.

>>> Most subsystem don't need an allocator, they work just fine
>>> simply being only dma-bufs importers. Recent example being the
>>> IIO subsystem[0], for which some early posting included an
>>> allocator, but in the end, all that was needed was to consume
>>> buffers.
>>>
>>> For devices that don't actually contain memory there is no
>>> reason to be an exporter. What most want is just to consume
>>> normal system memory. Or system memory with some constraints
>>> (e.g. contiguous, coherent, restricted, etc..).
>> ... secure, accessible only to the camera and video encoder, ... or
>> accessible only to the video decoder and the display unit. Who specifies
>> those restrictions? Can we express them in a platform-neutral way?
> 
> I once create a prototype for letting kernel drivers expose hints to which DMA-heap they want to work with.
> 
> The problem is that there are tons of different use cases and you need touse specific allocations for specific use cases.
> 
>>>> I have a feeling (I might be completely wrong here) that by using
>>>> generic dma-buf heaps we can easily end up in a situation when the
>>>> userspace depends heavily on the actual platform being used (to map the
>>>> platform to heap names). I think we should instead depend on the
>>>> existing devices (e.g. if there is a TEE device, use an IOCTL to
>>>> allocate secured DMA BUF from it, otherwise check for QTEE device,
>>>> otherwise check for some other vendor device).
>>>>
>>>> The mental experiment to check if the API is correct is really simple:
>>>> Can you use exactly the same rootfs on several devices without
>>>> any additional tuning (e.g. your QEMU, HiKey, a Mediatek board, Qualcomm
>>>> laptop, etc)?
>>>>
>>> This is a great north star to follow. And exactly the reason we should
>>> *not* be exposing device specific constraints to userspace. The constrains
>>> change based on the platform. So a userspace would have to also pick
>>> a different set of constraints based on each platform.
>> Great, I totally agree here.
> 
> That sounds reasonable, but depends on the restriction.
> 
> For example a lot of GPUs can work with any imported memory as long as itis DMA accessible for them, but they can scanout a picture to display on amonitor only from their local memory.
> 

And in this case, attaching the DMA-buf to that monitor for scanout
should fail at attach time, if we had a way to properly communicate
constraints between importer and exporter..

>>> Userspace knows which subsystems it will attach a buffer, and the
>>> kernel knows what constraints those devices have on a given platform.
>>> Ideal case is then allocate from the one exporter, attach to various
>>> devices, and have the constraints solved at map time by the exporter
>>> based on the set of attached devices.
> 
> That approach doesn't work. We have already tried stuff like that multiple times.
> 

Past failures don't guarantee future failure :)

>>> For example, on one platform the display needs contiguous buffers,
>>> but on a different platform the display can scatter-gather. So
>>> what heap should our generic application allocate from when it
>>> wants a buffer consumable by the display, CMA or System?
>>> Answer *should* be always use the generic exporter, and that
>>> exporter then picks the right backing type based on the platform.
>> The display can support scather-gather, the GPU needs bigger stride for
>> this particular format and the video encoder decoder can not support SG.
>> Which set of constraints and which buffer size should generic exporter
>> select?
> 
> Yeah, exactly that's the problem. The kernel doesn't know all the necessary information to make an informed allocation decision.
> 

Why not? For instance when we attach a buffer for scanout we provide
all the format/stride/etc info. What we are missing is a backchannel
to provide these to the exporter as a set of constraints that those
formats impose on a memory area for a given device.

> Sometimes you even have to insert format conversation steps and doing that transparently for userspace inside the kernel is really a no-go from the design side.
> 

Agree, we cannot be doing conversions like that in-kernel. We need
the format and constraints solved at an earlier step.

Andrew

>>> Userspace shouldn't be dealing with any of these constraints
>>> (looking back, adding the CMA heap was probably incorrect,
>>> and the System heap should have been the only one. Idea back
>>> then was a userspace helper would show up to do the constraint
>>> solving and pick the right heap. That has yet to materialize and
>>> folks are still just hardcoding which heap to use..).
>>>
>>> Same for this restricted heap, I'd like to explore if we can
>>> enhance the System heap such that when attached to the TEE framework,
>>> the backing memory is either made restricted by fire-walling,
>>> or allocating from a TEE carveout (based on platform).
> 
> Clearly NAK from my side to that design.
> 
> Regards,
> Christian.
> 
>> Firewalling from which devices? Or rather allowing access from which
>> devices? Is it possible to specify that somehow?
>>
>>> This will mean more inter-subsystem coordination, but we can
>>> iterate on these in kernel interfaces. We cannot iterate on
>>> userspace interfaces, those have to be correct the first time.
>>>
>>> Andrew
>>>
>>> [0]https://www.kernel.org/doc/html/next/iio/iio_dmabuf_api.html
>>>
>>>>> This can be tested on QEMU with the following steps:
>>>>> repo init -uhttps://github.com/jenswi-linaro/manifest.git  -m qemu_v8.xml \
>>>>>           -b prototype/sdp-v1
>>>>> repo sync -j8
>>>>> cd build
>>>>> make toolchains -j4
>>>>> make all -j$(nproc)
>>>>> make run-only
>>>>> # login and at the prompt:
>>>>> xtest --sdp-basic
>>>>>
>>>>> https://optee.readthedocs.io/en/latest/building/prerequisites.html
>>>>> list dependencies needed to build the above.
>>>>>
>>>>> The tests are pretty basic, mostly checking that a Trusted Application in
>>>>> the secure world can access and manipulate the memory.
>>>> - Can we test that the system doesn't crash badly if user provides
>>>>     non-secured memory to the users which expect a secure buffer?
>>>>
>>>> - At the same time corresponding entities shouldn't decode data to the
>>>>     buffers accessible to the rest of the sytem.
>>>>
>>>>> Cheers,
>>>>> Jens
>>>>>
>>>>> [1]https://lore.kernel.org/dri-devel/20240515112308.10171-1-yong.wu@mediatek.com/
>>>>> [2]https://lore.kernel.org/lkml/20220805135330.970-1-olivier.masse@nxp.com/
>>>>>
>>>>> Changes since Olivier's post [2]:
>>>>> * Based on Yong Wu's post [1] where much of dma-buf handling is done in
>>>>>     the generic restricted heap
>>>>> * Simplifications and cleanup
>>>>> * New commit message for "dma-buf: heaps: add Linaro restricted dmabuf heap
>>>>>     support"
>>>>> * Replaced the word "secure" with "restricted" where applicable
>>>>>
>>>>> Etienne Carriere (1):
>>>>>     tee: new ioctl to a register tee_shm from a dmabuf file descriptor
>>>>>
>>>>> Jens Wiklander (2):
>>>>>     dma-buf: heaps: restricted_heap: add no_map attribute
>>>>>     dma-buf: heaps: add Linaro restricted dmabuf heap support
>>>>>
>>>>> Olivier Masse (1):
>>>>>     dt-bindings: reserved-memory: add linaro,restricted-heap
>>>>>
>>>>>    .../linaro,restricted-heap.yaml               |  56 ++++++
>>>>>    drivers/dma-buf/heaps/Kconfig                 |  10 ++
>>>>>    drivers/dma-buf/heaps/Makefile                |   1 +
>>>>>    drivers/dma-buf/heaps/restricted_heap.c       |  17 +-
>>>>>    drivers/dma-buf/heaps/restricted_heap.h       |   2 +
>>>>>    .../dma-buf/heaps/restricted_heap_linaro.c    | 165 ++++++++++++++++++
>>>>>    drivers/tee/tee_core.c                        |  38 ++++
>>>>>    drivers/tee/tee_shm.c                         | 104 ++++++++++-
>>>>>    include/linux/tee_drv.h                       |  11 ++
>>>>>    include/uapi/linux/tee.h                      |  29 +++
>>>>>    10 files changed, 426 insertions(+), 7 deletions(-)
>>>>>    create mode 100644 Documentation/devicetree/bindings/reserved-memory/linaro,restricted-heap.yaml
>>>>>    create mode 100644 drivers/dma-buf/heaps/restricted_heap_linaro.c
>>>>>
>>>>> -- 
>>>>> 2.34.1
>>>>>
> 

Re: [Linaro-mm-sig] Re: [RFC PATCH 0/4] Linaro restricted heap

[Jens Wiklander]

Hi,

On Thu, Sep 26, 2024 at 4:03 PM Christian König
<christian.koenig@amd.com> wrote:
>
> Am 26.09.24 um 15:52 schrieb Sumit Garg:
> > [Resend in plain text format as my earlier message was rejected by
> > some mailing lists]
> >
> > On Thu, 26 Sept 2024 at 19:17, Sumit Garg <sumit.garg@linaro.org> wrote:
> >> On 9/25/24 19:31, Christian König wrote:
> >>
> >> Am 25.09.24 um 14:51 schrieb Dmitry Baryshkov:
> >>
> >> On Wed, Sep 25, 2024 at 10:51:15AM GMT, Christian König wrote:
> >>
> >> Am 25.09.24 um 01:05 schrieb Dmitry Baryshkov:
> >>
> >> On Tue, Sep 24, 2024 at 01:13:18PM GMT, Andrew Davis wrote:
> >>
> >> On 9/23/24 1:33 AM, Dmitry Baryshkov wrote:
> >>
> >> Hi,
> >>
> >> On Fri, Aug 30, 2024 at 09:03:47AM GMT, Jens Wiklander wrote:
> >>
> >> Hi,
> >>
> >> This patch set is based on top of Yong Wu's restricted heap patch set [1].
> >> It's also a continuation on Olivier's Add dma-buf secure-heap patch set [2].
> >>
> >> The Linaro restricted heap uses genalloc in the kernel to manage the heap
> >> carvout. This is a difference from the Mediatek restricted heap which
> >> relies on the secure world to manage the carveout.
> >>
> >> I've tried to adress the comments on [2], but [1] introduces changes so I'm
> >> afraid I've had to skip some comments.
> >>
> >> I know I have raised the same question during LPC (in connection to
> >> Qualcomm's dma-heap implementation). Is there any reason why we are
> >> using generic heaps instead of allocating the dma-bufs on the device
> >> side?
> >>
> >> In your case you already have TEE device, you can use it to allocate and
> >> export dma-bufs, which then get imported by the V4L and DRM drivers.
> >>
> >> This goes to the heart of why we have dma-heaps in the first place.
> >> We don't want to burden userspace with having to figure out the right
> >> place to get a dma-buf for a given use-case on a given hardware.
> >> That would be very non-portable, and fail at the core purpose of
> >> a kernel: to abstract hardware specifics away.
> >>
> >> Unfortunately all proposals to use dma-buf heaps were moving in the
> >> described direction: let app select (somehow) from a platform- and
> >> vendor- specific list of dma-buf heaps. In the kernel we at least know
> >> the platform on which the system is running. Userspace generally doesn't
> >> (and shouldn't). As such, it seems better to me to keep the knowledge in
> >> the kernel and allow userspace do its job by calling into existing
> >> device drivers.
> >>
> >> The idea of letting the kernel fully abstract away the complexity of inter
> >> device data exchange is a completely failed design. There has been plenty of
> >> evidence for that over the years.
> >>
> >> Because of this in DMA-buf it's an intentional design decision that
> >> userspace and *not* the kernel decides where and what to allocate from.
> >>
> >> Hmm, ok.
> >>
> >> What the kernel should provide are the necessary information what type of
> >> memory a device can work with and if certain memory is accessible or not.
> >> This is the part which is unfortunately still not well defined nor
> >> implemented at the moment.
> >>
> >> Apart from that there are a whole bunch of intentional design decision which
> >> should prevent developers to move allocation decision inside the kernel. For
> >> example DMA-buf doesn't know what the content of the buffer is (except for
> >> it's total size) and which use cases a buffer will be used with.
> >>
> >> So the question if memory should be exposed through DMA-heaps or a driver
> >> specific allocator is not a question of abstraction, but rather one of the
> >> physical location and accessibility of the memory.
> >>
> >> If the memory is attached to any physical device, e.g. local memory on a
> >> dGPU, FPGA PCIe BAR, RDMA, camera internal memory etc, then expose the
> >> memory as device specific allocator.
> >>
> >> So, for embedded systems with unified memory all buffers (maybe except
> >> PCIe BARs) should come from DMA-BUF heaps, correct?
> >>
> >>
> >>  From what I know that is correct, yes. Question is really if that will stay this way.
> >>
> >> Neural accelerators look a lot stripped down FPGAs these days and the benefit of local memory for GPUs is known for decades.
> >>
> >> Could be that designs with local specialized memory see a revival any time, who knows.
> >>
> >> If the memory is not physically attached to any device, but rather just
> >> memory attached to the CPU or a system wide memory controller then expose
> >> the memory as DMA-heap with specific requirements (e.g. certain sized pages,
> >> contiguous, restricted, encrypted, ...).
> >>
> >> Is encrypted / protected a part of the allocation contract or should it
> >> be enforced separately via a call to TEE / SCM / anything else?
> >>
> >>
> >> Well that is a really good question I can't fully answer either. From what I know now I would say it depends on the design.
> >>
> > IMHO, I think Dmitry's proposal to rather allow the TEE device to be
> > the allocator and exporter of DMA-bufs related to restricted memory
> > makes sense to me. Since it's really the TEE implementation (OP-TEE,
> > AMD-TEE, TS-TEE or future QTEE) which sets up the restrictions on a
> > particular piece of allocated memory. AFAIK, that happens after the
> > DMA-buf gets allocated and then user-space calls into TEE to set up
> > which media pipeline is going to access that particular DMA-buf. It
> > can also be a static contract depending on a particular platform
> > design.
> >
> > As Jens noted in the other thread, we already manage shared memory
> > allocations (from a static carve-out or dynamically mapped) for
> > communications among Linux and TEE that were based on DMA-bufs earlier
> > but since we didn't required them to be shared with other devices, so
> > we rather switched to anonymous memory.
> >
> >  From user-space perspective, it's cleaner to use TEE device IOCTLs for
> > DMA-buf allocations since it already knows which underlying TEE
> > implementation it's communicating with rather than first figuring out
> > which DMA heap to use for allocation and then communicating with TEE
> > implementation.
>
> +1
>
> I'm not that deeply into the functionality the TEE device IOCTLs expose,
> so can't judge if what's said above is correct or not.
>
> But in general building on top of existing infrastructure and
> information is a really strong argument for a design.
>
> So from my 10 mile high point of view that sounds like the way to go.

That sounds good, I'll prepare another patch set based on that
approach so we can see all the details.

Thanks,
Jens

Re: [Linaro-mm-sig] Re: [RFC PATCH 0/4] Linaro restricted heap

[Nicolas Dufresne]

Le jeudi 26 septembre 2024 à 19:22 +0530, Sumit Garg a écrit :
> [Resend in plain text format as my earlier message was rejected by
> some mailing lists]
> 
> On Thu, 26 Sept 2024 at 19:17, Sumit Garg <sumit.garg@linaro.org> wrote:
> > 
> > On 9/25/24 19:31, Christian König wrote:
> > 
> > Am 25.09.24 um 14:51 schrieb Dmitry Baryshkov:
> > 
> > On Wed, Sep 25, 2024 at 10:51:15AM GMT, Christian König wrote:
> > 
> > Am 25.09.24 um 01:05 schrieb Dmitry Baryshkov:
> > 
> > On Tue, Sep 24, 2024 at 01:13:18PM GMT, Andrew Davis wrote:
> > 
> > On 9/23/24 1:33 AM, Dmitry Baryshkov wrote:
> > 
> > Hi,
> > 
> > On Fri, Aug 30, 2024 at 09:03:47AM GMT, Jens Wiklander wrote:
> > 
> > Hi,
> > 
> > This patch set is based on top of Yong Wu's restricted heap patch set [1].
> > It's also a continuation on Olivier's Add dma-buf secure-heap patch set [2].
> > 
> > The Linaro restricted heap uses genalloc in the kernel to manage the heap
> > carvout. This is a difference from the Mediatek restricted heap which
> > relies on the secure world to manage the carveout.
> > 
> > I've tried to adress the comments on [2], but [1] introduces changes so I'm
> > afraid I've had to skip some comments.
> > 
> > I know I have raised the same question during LPC (in connection to
> > Qualcomm's dma-heap implementation). Is there any reason why we are
> > using generic heaps instead of allocating the dma-bufs on the device
> > side?
> > 
> > In your case you already have TEE device, you can use it to allocate and
> > export dma-bufs, which then get imported by the V4L and DRM drivers.
> > 
> > This goes to the heart of why we have dma-heaps in the first place.
> > We don't want to burden userspace with having to figure out the right
> > place to get a dma-buf for a given use-case on a given hardware.
> > That would be very non-portable, and fail at the core purpose of
> > a kernel: to abstract hardware specifics away.
> > 
> > Unfortunately all proposals to use dma-buf heaps were moving in the
> > described direction: let app select (somehow) from a platform- and
> > vendor- specific list of dma-buf heaps. In the kernel we at least know
> > the platform on which the system is running. Userspace generally doesn't
> > (and shouldn't). As such, it seems better to me to keep the knowledge in
> > the kernel and allow userspace do its job by calling into existing
> > device drivers.
> > 
> > The idea of letting the kernel fully abstract away the complexity of inter
> > device data exchange is a completely failed design. There has been plenty of
> > evidence for that over the years.
> > 
> > Because of this in DMA-buf it's an intentional design decision that
> > userspace and *not* the kernel decides where and what to allocate from.
> > 
> > Hmm, ok.
> > 
> > What the kernel should provide are the necessary information what type of
> > memory a device can work with and if certain memory is accessible or not.
> > This is the part which is unfortunately still not well defined nor
> > implemented at the moment.
> > 
> > Apart from that there are a whole bunch of intentional design decision which
> > should prevent developers to move allocation decision inside the kernel. For
> > example DMA-buf doesn't know what the content of the buffer is (except for
> > it's total size) and which use cases a buffer will be used with.
> > 
> > So the question if memory should be exposed through DMA-heaps or a driver
> > specific allocator is not a question of abstraction, but rather one of the
> > physical location and accessibility of the memory.
> > 
> > If the memory is attached to any physical device, e.g. local memory on a
> > dGPU, FPGA PCIe BAR, RDMA, camera internal memory etc, then expose the
> > memory as device specific allocator.
> > 
> > So, for embedded systems with unified memory all buffers (maybe except
> > PCIe BARs) should come from DMA-BUF heaps, correct?
> > 
> > 
> > From what I know that is correct, yes. Question is really if that will stay this way.
> > 
> > Neural accelerators look a lot stripped down FPGAs these days and the benefit of local memory for GPUs is known for decades.
> > 
> > Could be that designs with local specialized memory see a revival any time, who knows.
> > 
> > If the memory is not physically attached to any device, but rather just
> > memory attached to the CPU or a system wide memory controller then expose
> > the memory as DMA-heap with specific requirements (e.g. certain sized pages,
> > contiguous, restricted, encrypted, ...).
> > 
> > Is encrypted / protected a part of the allocation contract or should it
> > be enforced separately via a call to TEE / SCM / anything else?
> > 
> > 
> > Well that is a really good question I can't fully answer either. From what I know now I would say it depends on the design.
> > 
> 
> IMHO, I think Dmitry's proposal to rather allow the TEE device to be
> the allocator and exporter of DMA-bufs related to restricted memory
> makes sense to me. Since it's really the TEE implementation (OP-TEE,
> AMD-TEE, TS-TEE or future QTEE) which sets up the restrictions on a
> particular piece of allocated memory. AFAIK, that happens after the
> DMA-buf gets allocated and then user-space calls into TEE to set up
> which media pipeline is going to access that particular DMA-buf. It
> can also be a static contract depending on a particular platform
> design.

When the memory get the protection is hardware specific. Otherwise the design
would be really straightforward, allocate from the a heap or any random driver
API and protect that memory through an call into the TEE. Clear seperation would
be amazingly better, but this is not how hardware and firmware designer have
seen it. 

In some implementation, there is a carving of memory that be protected before
the kernel is booted. I believe (but I'm not affiliated with them) that MTK has
hardware restriction making that design the only usable method.

In general, the handling of secure memory is bound to the TEE application for
the specific platform, it has to be separated from the generic part of tee
drivers anyway, and dmabuf heaps is in my opinion the right API for the task.

On MTK, if you have followed, when the SCP (their co-processor) is handling
restricted video, you can't even call into it anymore directly. So to drive the
CODECs, everything has to be routed through the TEE. Would you say that because
of that this should not be a V4L2 driver anymore ?

> 
> As Jens noted in the other thread, we already manage shared memory
> allocations (from a static carve-out or dynamically mapped) for
> communications among Linux and TEE that were based on DMA-bufs earlier
> but since we didn't required them to be shared with other devices, so
> we rather switched to anonymous memory.
> 
> From user-space perspective, it's cleaner to use TEE device IOCTLs for
> DMA-buf allocations since it already knows which underlying TEE
> implementation it's communicating with rather than first figuring out
> which DMA heap to use for allocation and then communicating with TEE
> implementation.

As a user-space developer in the majority of my time, adding common code to
handle dma heaps is a lot easier and straight forward then having to glue all
the different allocators implement in various subsystems. Communicating which
heap to work can be generic and simple.

Nicolas

Re: [Linaro-mm-sig] Re: [RFC PATCH 0/4] Linaro restricted heap

[Sumit Garg]

On Sat, 28 Sept 2024 at 01:20, Nicolas Dufresne <nicolas@ndufresne.ca> wrote:
>
> Le jeudi 26 septembre 2024 à 19:22 +0530, Sumit Garg a écrit :
> > [Resend in plain text format as my earlier message was rejected by
> > some mailing lists]
> >
> > On Thu, 26 Sept 2024 at 19:17, Sumit Garg <sumit.garg@linaro.org> wrote:
> > >
> > > On 9/25/24 19:31, Christian König wrote:
> > >
> > > Am 25.09.24 um 14:51 schrieb Dmitry Baryshkov:
> > >
> > > On Wed, Sep 25, 2024 at 10:51:15AM GMT, Christian König wrote:
> > >
> > > Am 25.09.24 um 01:05 schrieb Dmitry Baryshkov:
> > >
> > > On Tue, Sep 24, 2024 at 01:13:18PM GMT, Andrew Davis wrote:
> > >
> > > On 9/23/24 1:33 AM, Dmitry Baryshkov wrote:
> > >
> > > Hi,
> > >
> > > On Fri, Aug 30, 2024 at 09:03:47AM GMT, Jens Wiklander wrote:
> > >
> > > Hi,
> > >
> > > This patch set is based on top of Yong Wu's restricted heap patch set [1].
> > > It's also a continuation on Olivier's Add dma-buf secure-heap patch set [2].
> > >
> > > The Linaro restricted heap uses genalloc in the kernel to manage the heap
> > > carvout. This is a difference from the Mediatek restricted heap which
> > > relies on the secure world to manage the carveout.
> > >
> > > I've tried to adress the comments on [2], but [1] introduces changes so I'm
> > > afraid I've had to skip some comments.
> > >
> > > I know I have raised the same question during LPC (in connection to
> > > Qualcomm's dma-heap implementation). Is there any reason why we are
> > > using generic heaps instead of allocating the dma-bufs on the device
> > > side?
> > >
> > > In your case you already have TEE device, you can use it to allocate and
> > > export dma-bufs, which then get imported by the V4L and DRM drivers.
> > >
> > > This goes to the heart of why we have dma-heaps in the first place.
> > > We don't want to burden userspace with having to figure out the right
> > > place to get a dma-buf for a given use-case on a given hardware.
> > > That would be very non-portable, and fail at the core purpose of
> > > a kernel: to abstract hardware specifics away.
> > >
> > > Unfortunately all proposals to use dma-buf heaps were moving in the
> > > described direction: let app select (somehow) from a platform- and
> > > vendor- specific list of dma-buf heaps. In the kernel we at least know
> > > the platform on which the system is running. Userspace generally doesn't
> > > (and shouldn't). As such, it seems better to me to keep the knowledge in
> > > the kernel and allow userspace do its job by calling into existing
> > > device drivers.
> > >
> > > The idea of letting the kernel fully abstract away the complexity of inter
> > > device data exchange is a completely failed design. There has been plenty of
> > > evidence for that over the years.
> > >
> > > Because of this in DMA-buf it's an intentional design decision that
> > > userspace and *not* the kernel decides where and what to allocate from.
> > >
> > > Hmm, ok.
> > >
> > > What the kernel should provide are the necessary information what type of
> > > memory a device can work with and if certain memory is accessible or not.
> > > This is the part which is unfortunately still not well defined nor
> > > implemented at the moment.
> > >
> > > Apart from that there are a whole bunch of intentional design decision which
> > > should prevent developers to move allocation decision inside the kernel. For
> > > example DMA-buf doesn't know what the content of the buffer is (except for
> > > it's total size) and which use cases a buffer will be used with.
> > >
> > > So the question if memory should be exposed through DMA-heaps or a driver
> > > specific allocator is not a question of abstraction, but rather one of the
> > > physical location and accessibility of the memory.
> > >
> > > If the memory is attached to any physical device, e.g. local memory on a
> > > dGPU, FPGA PCIe BAR, RDMA, camera internal memory etc, then expose the
> > > memory as device specific allocator.
> > >
> > > So, for embedded systems with unified memory all buffers (maybe except
> > > PCIe BARs) should come from DMA-BUF heaps, correct?
> > >
> > >
> > > From what I know that is correct, yes. Question is really if that will stay this way.
> > >
> > > Neural accelerators look a lot stripped down FPGAs these days and the benefit of local memory for GPUs is known for decades.
> > >
> > > Could be that designs with local specialized memory see a revival any time, who knows.
> > >
> > > If the memory is not physically attached to any device, but rather just
> > > memory attached to the CPU or a system wide memory controller then expose
> > > the memory as DMA-heap with specific requirements (e.g. certain sized pages,
> > > contiguous, restricted, encrypted, ...).
> > >
> > > Is encrypted / protected a part of the allocation contract or should it
> > > be enforced separately via a call to TEE / SCM / anything else?
> > >
> > >
> > > Well that is a really good question I can't fully answer either. From what I know now I would say it depends on the design.
> > >
> >
> > IMHO, I think Dmitry's proposal to rather allow the TEE device to be
> > the allocator and exporter of DMA-bufs related to restricted memory
> > makes sense to me. Since it's really the TEE implementation (OP-TEE,
> > AMD-TEE, TS-TEE or future QTEE) which sets up the restrictions on a
> > particular piece of allocated memory. AFAIK, that happens after the
> > DMA-buf gets allocated and then user-space calls into TEE to set up
> > which media pipeline is going to access that particular DMA-buf. It
> > can also be a static contract depending on a particular platform
> > design.
>
> When the memory get the protection is hardware specific. Otherwise the design
> would be really straightforward, allocate from the a heap or any random driver
> API and protect that memory through an call into the TEE. Clear seperation would
> be amazingly better, but this is not how hardware and firmware designer have
> seen it.
>
> In some implementation, there is a carving of memory that be protected before
> the kernel is booted. I believe (but I'm not affiliated with them) that MTK has
> hardware restriction making that design the only usable method.

Yeah I agree with that. The point I am making here is that the TEE
subsystem can abstract all that platform/vendor specific methods for
user-space to allocate restricted memory. We already have a similar
infrastructure for shared memory among Linux and TEE implementation.
The user-space only uses TEE_IOC_SHM_ALLOC [1] where underneath it can
either allocate from static carveout of shared memory (as a reserved
memory region) OR simply allocate from the kernel heap which is
dynamically mapped into the TEE implementation. The choice here
depends on the platform/TEE implementation capability.

[1] https://docs.kernel.org/userspace-api/tee.html

>
> In general, the handling of secure memory is bound to the TEE application for
> the specific platform, it has to be separated from the generic part of tee
> drivers anyway,

It is really the TEE implementation core which has the privileges to
mark a piece of memory as restricted/secure. The TEE application in
MTK is likely a pseudo TA (a terminology similar to Linux kernel
modules in the TEE world). So it is rather easier for TEE
implementation drivers to abstract out the communication with the
vendor specific TEE core implementation.

> and dmabuf heaps is in my opinion the right API for the task.

Do you really think it is better for user-space to deal with vendor
specific dmabuf heaps?

>
> On MTK, if you have followed, when the SCP (their co-processor) is handling
> restricted video, you can't even call into it anymore directly. So to drive the
> CODECs, everything has to be routed through the TEE. Would you say that because
> of that this should not be a V4L2 driver anymore ?

I am not conversant with the MTK hardware/firmware implementation. But
my point is the kernel shouldn't be exposing 10s of vendor specific
DMAbuf heaps to the user-space to choose from which can rather be just
a single TEE device IOCTL used to allocate restricted memory.

>
> >
> > As Jens noted in the other thread, we already manage shared memory
> > allocations (from a static carve-out or dynamically mapped) for
> > communications among Linux and TEE that were based on DMA-bufs earlier
> > but since we didn't required them to be shared with other devices, so
> > we rather switched to anonymous memory.
> >
> > From user-space perspective, it's cleaner to use TEE device IOCTLs for
> > DMA-buf allocations since it already knows which underlying TEE
> > implementation it's communicating with rather than first figuring out
> > which DMA heap to use for allocation and then communicating with TEE
> > implementation.
>
> As a user-space developer in the majority of my time, adding common code to
> handle dma heaps is a lot easier and straight forward then having to glue all
> the different allocators implement in various subsystems. Communicating which
> heap to work can be generic and simple.

Yeah I agree with that notion but IMHO having ifdefry to select vendor
specific DMA heaps isn't something user-space should be dealing with.

-Sumit

>
> Nicolas
>