[PATCH v2 0/8] dma-buf: heaps: Add secure heap

[PATCH v2 0/8] dma-buf: heaps: Add secure heap

[Yong Wu]

This patchset adds three secure heaps:
1) secure_mtk_cm: secure chunk memory for MediaTek SVP (Secure Video Path).
   The buffer is reserved for the secure world after bootup and it is used
   for vcodec's ES/working buffer;
2) secure_mtk_cma: secure CMA memory for MediaTek SVP. This buffer is
   dynamically reserved for the secure world and will be got when we start
   playing secure videos, Once the security video playing is complete, the
   CMA will be released. This heap is used for the vcodec's frame buffer. 
3) secure_cma: Use the kerne CMA ops as the allocation ops. 
   currently it is a draft version for Vijay and Jaskaran.

For the first two MediaTek heaps will be used v4l2[1] and drm[2], thus we
cannot put it in v4l2 or drm, and create a common heap for them. Meanwhile
We have a limited number of hardware entries to protect memory, we cannot
protect memory arbitrarily, thus the secure memory management is actually
inside OPTEE. The kernel just tells the TEE what size I want and the TEE
will return a "secure handle".

[1] https://lore.kernel.org/linux-mediatek/20231106120423.23364-1-yunfei.dong@mediatek.com/
[2] https://lore.kernel.org/linux-mediatek/20231023044549.21412-1-jason-jh.lin@mediatek.com/

Change note:
v2: 1) Move John's patches into the vcodec patchset since they use the new
       dma heap interface directly.
       https://lore.kernel.org/linux-mediatek/20231106120423.23364-1-yunfei.dong@mediatek.com/
    2) Reword the dt-binding description.
    3) Rename the heap name from mtk_svp to secure_mtk_cm.
       This means the current vcodec/DRM upstream code doesn't match this.
    4) Add a normal CMA heap. currently it should be a draft version.
    5) Regarding the UUID, I still use hard code, but put it in a private
    data which allow the others could set their own UUID. What's more, UUID
    is necessary for the session with TEE. If we don't have it, we can't
    communicate with the TEE, including the get_uuid interface, which tries
    to make uuid more generic, not working. If there is other way to make
    UUID more general, please free to tell me.
    
v1: https://lore.kernel.org/linux-mediatek/20230911023038.30649-1-yong.wu@mediatek.com/
    Base on v6.6-rc1.

Yong Wu (8):
  dma-buf: heaps: Initialize a secure heap
  dma-buf: heaps: secure_heap: Add private heap ops
  dma-buf: heaps: secure_heap: Initialize tee session
  dma-buf: heaps: secure_heap: Add tee memory service call
  dma-buf: heaps: secure_heap: Add dma_ops
  dt-bindings: reserved-memory: Add secure CMA reserved memory range
  dma_buf: heaps: secure_heap: Add a new MediaTek CMA heap
  dma-buf: heaps: secure_heap: Add normal CMA heap

 .../reserved-memory/secure_cma_region.yaml    |  44 ++
 drivers/dma-buf/heaps/Kconfig                 |   7 +
 drivers/dma-buf/heaps/Makefile                |   1 +
 drivers/dma-buf/heaps/secure_heap.c           | 602 ++++++++++++++++++
 4 files changed, 654 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/reserved-memory/secure_cma_region.yaml
 create mode 100644 drivers/dma-buf/heaps/secure_heap.c

-- 
2.25.1

[PATCH v2 1/8] dma-buf: heaps: Initialize a secure heap

[Yong Wu]

Initialize a secure heap. Currently just add a null heap, Prepare for
the later patches.

Signed-off-by: Yong Wu <yong.wu@mediatek.com>
---
 drivers/dma-buf/heaps/Kconfig       |  7 +++
 drivers/dma-buf/heaps/Makefile      |  1 +
 drivers/dma-buf/heaps/secure_heap.c | 98 +++++++++++++++++++++++++++++
 3 files changed, 106 insertions(+)
 create mode 100644 drivers/dma-buf/heaps/secure_heap.c

diff --git a/drivers/dma-buf/heaps/Kconfig b/drivers/dma-buf/heaps/Kconfig
index a5eef06c4226..e358bf711145 100644
--- a/drivers/dma-buf/heaps/Kconfig
+++ b/drivers/dma-buf/heaps/Kconfig
@@ -12,3 +12,10 @@ config DMABUF_HEAPS_CMA
 	  Choose this option to enable dma-buf CMA heap. This heap is backed
 	  by the Contiguous Memory Allocator (CMA). If your system has these
 	  regions, you should say Y here.
+
+config DMABUF_HEAPS_SECURE
+	bool "DMA-BUF Secure Heap"
+	depends on DMABUF_HEAPS && TEE
+	help
+	  Choose this option to enable dma-buf secure heap. This heap is backed by
+	  TEE client interfaces or CMA. If in doubt, say N.
diff --git a/drivers/dma-buf/heaps/Makefile b/drivers/dma-buf/heaps/Makefile
index 974467791032..b1ad9d1f2fbe 100644
--- a/drivers/dma-buf/heaps/Makefile
+++ b/drivers/dma-buf/heaps/Makefile
@@ -1,3 +1,4 @@
 # SPDX-License-Identifier: GPL-2.0
+obj-$(CONFIG_DMABUF_HEAPS_SECURE)	+= secure_heap.o
 obj-$(CONFIG_DMABUF_HEAPS_SYSTEM)	+= system_heap.o
 obj-$(CONFIG_DMABUF_HEAPS_CMA)		+= cma_heap.o
diff --git a/drivers/dma-buf/heaps/secure_heap.c b/drivers/dma-buf/heaps/secure_heap.c
new file mode 100644
index 000000000000..a634051a0a67
--- /dev/null
+++ b/drivers/dma-buf/heaps/secure_heap.c
@@ -0,0 +1,98 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * DMABUF secure heap exporter
+ *
+ * Copyright (C) 2023 MediaTek Inc.
+ */
+
+#include <linux/dma-buf.h>
+#include <linux/dma-heap.h>
+#include <linux/err.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+
+enum secure_memory_type {
+	/*
+	 * MediaTek static chunk memory carved out for TrustZone. The memory
+	 * management is inside the TEE.
+	 */
+	SECURE_MEMORY_TYPE_MTK_CM_TZ	= 1,
+};
+
+struct secure_buffer {
+	struct dma_heap			*heap;
+	size_t				size;
+};
+
+struct secure_heap {
+	const char			*name;
+	const enum secure_memory_type	mem_type;
+};
+
+static struct dma_buf *
+secure_heap_allocate(struct dma_heap *heap, unsigned long size,
+		     unsigned long fd_flags, unsigned long heap_flags)
+{
+	struct secure_buffer *sec_buf;
+	DEFINE_DMA_BUF_EXPORT_INFO(exp_info);
+	struct dma_buf *dmabuf;
+	int ret;
+
+	sec_buf = kzalloc(sizeof(*sec_buf), GFP_KERNEL);
+	if (!sec_buf)
+		return ERR_PTR(-ENOMEM);
+
+	sec_buf->size = ALIGN(size, PAGE_SIZE);
+	sec_buf->heap = heap;
+
+	exp_info.exp_name = dma_heap_get_name(heap);
+	exp_info.size = sec_buf->size;
+	exp_info.flags = fd_flags;
+	exp_info.priv = sec_buf;
+
+	dmabuf = dma_buf_export(&exp_info);
+	if (IS_ERR(dmabuf)) {
+		ret = PTR_ERR(dmabuf);
+		goto err_free_buf;
+	}
+
+	return dmabuf;
+
+err_free_buf:
+	kfree(sec_buf);
+	return ERR_PTR(ret);
+}
+
+static const struct dma_heap_ops sec_heap_ops = {
+	.allocate = secure_heap_allocate,
+};
+
+static struct secure_heap secure_heaps[] = {
+	{
+		.name		= "secure_mtk_cm",
+		.mem_type	= SECURE_MEMORY_TYPE_MTK_CM_TZ,
+	},
+};
+
+static int secure_heap_init(void)
+{
+	struct secure_heap *sec_heap = secure_heaps;
+	struct dma_heap_export_info exp_info;
+	struct dma_heap *heap;
+	unsigned int i;
+
+	for (i = 0; i < ARRAY_SIZE(secure_heaps); i++, sec_heap++) {
+		exp_info.name = sec_heap->name;
+		exp_info.ops = &sec_heap_ops;
+		exp_info.priv = (void *)sec_heap;
+
+		heap = dma_heap_add(&exp_info);
+		if (IS_ERR(heap))
+			return PTR_ERR(heap);
+	}
+	return 0;
+}
+
+module_init(secure_heap_init);
+MODULE_DESCRIPTION("Secure Heap Driver");
+MODULE_LICENSE("GPL");
-- 
2.25.1

[PATCH v2 2/8] dma-buf: heaps: secure_heap: Add private heap ops

[Yong Wu]

For the secure memory, there are two steps:
a) Allocate buffers in kernel side;
b) Secure that buffer.
Different heaps may have different buffer allocation methods and
different memory protection methods. Here abstract the memory
allocation and securing operations.

Signed-off-by: Yong Wu <yong.wu@mediatek.com>
---
 drivers/dma-buf/heaps/secure_heap.c | 58 ++++++++++++++++++++++++++++-
 1 file changed, 57 insertions(+), 1 deletion(-)

diff --git a/drivers/dma-buf/heaps/secure_heap.c b/drivers/dma-buf/heaps/secure_heap.c
index a634051a0a67..87ac23072e9e 100644
--- a/drivers/dma-buf/heaps/secure_heap.c
+++ b/drivers/dma-buf/heaps/secure_heap.c
@@ -24,15 +24,66 @@ struct secure_buffer {
 	size_t				size;
 };
 
+struct secure_heap;
+
+struct secure_heap_prv_data {
+	int	(*memory_alloc)(struct secure_heap *sec_heap, struct secure_buffer *sec_buf);
+	void	(*memory_free)(struct secure_heap *sec_heap, struct secure_buffer *sec_buf);
+
+	/* Protect/unprotect the memory */
+	int	(*secure_the_memory)(struct secure_heap *sec_heap, struct secure_buffer *sec_buf);
+	void	(*unsecure_the_memory)(struct secure_heap *sec_heap, struct secure_buffer *sec_buf);
+};
+
 struct secure_heap {
 	const char			*name;
 	const enum secure_memory_type	mem_type;
+
+	const struct secure_heap_prv_data *data;
 };
 
+static int secure_heap_secure_memory_allocate(struct secure_heap *sec_heap,
+					      struct secure_buffer *sec_buf)
+{
+	const struct secure_heap_prv_data *data = sec_heap->data;
+	int ret;
+
+	if (data->memory_alloc) {
+		ret = data->memory_alloc(sec_heap, sec_buf);
+		if (ret)
+			return ret;
+	}
+
+	if (data->secure_the_memory) {
+		ret = data->secure_the_memory(sec_heap, sec_buf);
+		if (ret)
+			goto sec_memory_free;
+	}
+	return 0;
+
+sec_memory_free:
+	if (data->memory_free)
+		data->memory_free(sec_heap, sec_buf);
+	return ret;
+}
+
+static void secure_heap_secure_memory_free(struct secure_heap *sec_heap,
+					   struct secure_buffer *sec_buf)
+{
+	const struct secure_heap_prv_data *data = sec_heap->data;
+
+	if (data->unsecure_the_memory)
+		data->unsecure_the_memory(sec_heap, sec_buf);
+
+	if (data->memory_free)
+		data->memory_free(sec_heap, sec_buf);
+}
+
 static struct dma_buf *
 secure_heap_allocate(struct dma_heap *heap, unsigned long size,
 		     unsigned long fd_flags, unsigned long heap_flags)
 {
+	struct secure_heap *sec_heap = dma_heap_get_drvdata(heap);
 	struct secure_buffer *sec_buf;
 	DEFINE_DMA_BUF_EXPORT_INFO(exp_info);
 	struct dma_buf *dmabuf;
@@ -45,6 +96,9 @@ secure_heap_allocate(struct dma_heap *heap, unsigned long size,
 	sec_buf->size = ALIGN(size, PAGE_SIZE);
 	sec_buf->heap = heap;
 
+	ret = secure_heap_secure_memory_allocate(sec_heap, sec_buf);
+	if (ret)
+		goto err_free_buf;
 	exp_info.exp_name = dma_heap_get_name(heap);
 	exp_info.size = sec_buf->size;
 	exp_info.flags = fd_flags;
@@ -53,11 +107,13 @@ secure_heap_allocate(struct dma_heap *heap, unsigned long size,
 	dmabuf = dma_buf_export(&exp_info);
 	if (IS_ERR(dmabuf)) {
 		ret = PTR_ERR(dmabuf);
-		goto err_free_buf;
+		goto err_free_sec_mem;
 	}
 
 	return dmabuf;
 
+err_free_sec_mem:
+	secure_heap_secure_memory_free(sec_heap, sec_buf);
 err_free_buf:
 	kfree(sec_buf);
 	return ERR_PTR(ret);
-- 
2.25.1

[PATCH v2 3/8] dma-buf: heaps: secure_heap: Initialize tee session

[Yong Wu]

The TEE probe later than dma-buf heap, and PROBE_DEDER doesn't work
here since this is not a platform driver, therefore initialize the TEE
context/session while we allocate the first secure buffer.

Add our special UUID and tee type in the private data.

If the uuid is zero, it means that it doesn't enter TEE to protect the
buffer, there may be other ways to protect the buffer.

All the MTK chrome projects use this UUID. The UUID is only used in the
kernelspace while userspace never use it. The userspace could allocate the
secure memory via the existing dma-buf ioctl.

Signed-off-by: Yong Wu <yong.wu@mediatek.com>
---
 drivers/dma-buf/heaps/secure_heap.c | 75 +++++++++++++++++++++++++++++
 1 file changed, 75 insertions(+)

diff --git a/drivers/dma-buf/heaps/secure_heap.c b/drivers/dma-buf/heaps/secure_heap.c
index 87ac23072e9e..2a037fc54004 100644
--- a/drivers/dma-buf/heaps/secure_heap.c
+++ b/drivers/dma-buf/heaps/secure_heap.c
@@ -10,6 +10,12 @@
 #include <linux/err.h>
 #include <linux/module.h>
 #include <linux/slab.h>
+#include <linux/tee_drv.h>
+#include <linux/uuid.h>
+
+#define TZ_TA_MEM_UUID_MTK		"4477588a-8476-11e2-ad15-e41f1390d676"
+
+#define TEE_PARAM_NUM			4
 
 enum secure_memory_type {
 	/*
@@ -27,6 +33,9 @@ struct secure_buffer {
 struct secure_heap;
 
 struct secure_heap_prv_data {
+	const char			*uuid;
+	const int			tee_impl_id;
+
 	int	(*memory_alloc)(struct secure_heap *sec_heap, struct secure_buffer *sec_buf);
 	void	(*memory_free)(struct secure_heap *sec_heap, struct secure_buffer *sec_buf);
 
@@ -39,9 +48,62 @@ struct secure_heap {
 	const char			*name;
 	const enum secure_memory_type	mem_type;
 
+	struct tee_context		*tee_ctx;
+	u32				tee_session;
+
 	const struct secure_heap_prv_data *data;
 };
 
+static int tee_ctx_match(struct tee_ioctl_version_data *ver, const void *data)
+{
+	const struct secure_heap_prv_data *d = data;
+
+	return ver->impl_id == d->tee_impl_id;
+}
+
+static int secure_heap_tee_session_init(struct secure_heap *sec_heap)
+{
+	struct tee_param t_param[TEE_PARAM_NUM] = {0};
+	struct tee_ioctl_open_session_arg arg = {0};
+	const struct secure_heap_prv_data *data = sec_heap->data;
+	uuid_t ta_mem_uuid;
+	int ret;
+
+	sec_heap->tee_ctx = tee_client_open_context(NULL, tee_ctx_match, data, NULL);
+	if (IS_ERR(sec_heap->tee_ctx)) {
+		pr_err_once("%s: open context failed, ret=%ld\n", sec_heap->name,
+			    PTR_ERR(sec_heap->tee_ctx));
+		return -ENODEV;
+	}
+
+	arg.num_params = TEE_PARAM_NUM;
+	arg.clnt_login = TEE_IOCTL_LOGIN_PUBLIC;
+	ret = uuid_parse(data->uuid, &ta_mem_uuid);
+	if (ret)
+		goto close_context;
+	memcpy(&arg.uuid, &ta_mem_uuid.b, sizeof(ta_mem_uuid));
+
+	ret = tee_client_open_session(sec_heap->tee_ctx, &arg, t_param);
+	if (ret < 0 || arg.ret) {
+		pr_err_once("%s: open session failed, ret=%d:%d\n",
+			    sec_heap->name, ret, arg.ret);
+		ret = -EINVAL;
+		goto close_context;
+	}
+	sec_heap->tee_session = arg.session;
+	return 0;
+
+close_context:
+	tee_client_close_context(sec_heap->tee_ctx);
+	return ret;
+}
+
+/* The memory allocating is within the TEE. */
+const struct secure_heap_prv_data mtk_sec_mem_data = {
+	.uuid			= TZ_TA_MEM_UUID_MTK,
+	.tee_impl_id		= TEE_IMPL_ID_OPTEE,
+};
+
 static int secure_heap_secure_memory_allocate(struct secure_heap *sec_heap,
 					      struct secure_buffer *sec_buf)
 {
@@ -84,11 +146,23 @@ secure_heap_allocate(struct dma_heap *heap, unsigned long size,
 		     unsigned long fd_flags, unsigned long heap_flags)
 {
 	struct secure_heap *sec_heap = dma_heap_get_drvdata(heap);
+	const struct secure_heap_prv_data *data = sec_heap->data;
 	struct secure_buffer *sec_buf;
 	DEFINE_DMA_BUF_EXPORT_INFO(exp_info);
 	struct dma_buf *dmabuf;
 	int ret;
 
+	/*
+	 * If uuid is valid, It requires enter TEE to protect buffers. However
+	 * TEE probe may be late. Initialize the secure session the first time
+	 * we request the secure buffer.
+	 */
+	if (data->uuid && !sec_heap->tee_session) {
+		ret = secure_heap_tee_session_init(sec_heap);
+		if (ret)
+			return ERR_PTR(ret);
+	}
+
 	sec_buf = kzalloc(sizeof(*sec_buf), GFP_KERNEL);
 	if (!sec_buf)
 		return ERR_PTR(-ENOMEM);
@@ -127,6 +201,7 @@ static struct secure_heap secure_heaps[] = {
 	{
 		.name		= "secure_mtk_cm",
 		.mem_type	= SECURE_MEMORY_TYPE_MTK_CM_TZ,
+		.data		= &mtk_sec_mem_data,
 	},
 };
 
-- 
2.25.1

[PATCH v2 4/8] dma-buf: heaps: secure_heap: Add tee memory service call

[Yong Wu]

Add TEE service call. In the case of MediaTek, secure memory management is
located within the TEE. The kernel just needs to tell TEE what size it
needs and the TEE will return a "security handle" to kernel.

To be consistent with the cma heap later, we put the tee ops into the ops
of secure_the_memory.

It seems that secure_heap_tee_service_call could be a more general
interface, but it could be a new topic.

Signed-off-by: Yong Wu <yong.wu@mediatek.com>
---
 drivers/dma-buf/heaps/secure_heap.c | 97 +++++++++++++++++++++++++++++
 1 file changed, 97 insertions(+)

diff --git a/drivers/dma-buf/heaps/secure_heap.c b/drivers/dma-buf/heaps/secure_heap.c
index 2a037fc54004..05062c14e7c7 100644
--- a/drivers/dma-buf/heaps/secure_heap.c
+++ b/drivers/dma-buf/heaps/secure_heap.c
@@ -17,6 +17,27 @@
 
 #define TEE_PARAM_NUM			4
 
+enum secure_buffer_tee_cmd { /* PARAM NUM always is 4. */
+	/*
+	 * TZCMD_SECMEM_ZALLOC: Allocate the zeroed secure memory from TEE.
+	 *
+	 * [in]  value[0].a: The buffer size.
+	 *       value[0].b: alignment.
+	 * [in]  value[1].a: enum secure_memory_type.
+	 * [out] value[3].a: The secure handle.
+	 */
+	TZCMD_SECMEM_ZALLOC = 0,
+
+	/*
+	 * TZCMD_SECMEM_FREE: Free secure memory.
+	 *
+	 * [in]  value[0].a: The secure handle of this buffer, It's value[3].a of
+	 *                   TZCMD_SECMEM_ZALLOC.
+	 * [out] value[1].a: return value, 0 means successful, otherwise fail.
+	 */
+	TZCMD_SECMEM_FREE = 1,
+};
+
 enum secure_memory_type {
 	/*
 	 * MediaTek static chunk memory carved out for TrustZone. The memory
@@ -28,13 +49,25 @@ enum secure_memory_type {
 struct secure_buffer {
 	struct dma_heap			*heap;
 	size_t				size;
+	/*
+	 * The secure handle is a reference to a buffer within the TEE, this is
+	 * a value got from TEE.
+	 */
+	u32				sec_handle;
 };
 
+#define TEE_MEM_COMMAND_ID_BASE_MTK	0x10000
+
 struct secure_heap;
 
 struct secure_heap_prv_data {
 	const char			*uuid;
 	const int			tee_impl_id;
+	/*
+	 * Different TEEs may implement different commands, and this provides an opportunity
+	 * for TEEs to use the same enum secure_buffer_tee_cmd.
+	 */
+	const int			tee_command_id_base;
 
 	int	(*memory_alloc)(struct secure_heap *sec_heap, struct secure_buffer *sec_buf);
 	void	(*memory_free)(struct secure_heap *sec_heap, struct secure_buffer *sec_buf);
@@ -98,10 +131,74 @@ static int secure_heap_tee_session_init(struct secure_heap *sec_heap)
 	return ret;
 }
 
+static int
+secure_heap_tee_service_call(struct tee_context *tee_ctx, u32 session,
+			     unsigned int command, struct tee_param *params)
+{
+	struct tee_ioctl_invoke_arg arg = {0};
+	int ret;
+
+	arg.num_params = TEE_PARAM_NUM;
+	arg.session = session;
+	arg.func = command;
+
+	ret = tee_client_invoke_func(tee_ctx, &arg, params);
+	if (ret < 0 || arg.ret) {
+		pr_err("%s: cmd %d ret %d:%x.\n", __func__, command, ret, arg.ret);
+		ret = -EOPNOTSUPP;
+	}
+	return ret;
+}
+
+static int secure_heap_tee_secure_memory(struct secure_heap *sec_heap,
+					 struct secure_buffer *sec_buf)
+{
+	const struct secure_heap_prv_data *data = sec_heap->data;
+	struct tee_param params[TEE_PARAM_NUM] = {0};
+	int ret;
+
+	params[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT;
+	params[0].u.value.a = sec_buf->size;
+	params[0].u.value.b = PAGE_SIZE;
+	params[1].attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT;
+	params[1].u.value.a = sec_heap->mem_type;
+	params[2].attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT;
+
+	params[3].attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_OUTPUT;
+	ret = secure_heap_tee_service_call(sec_heap->tee_ctx, sec_heap->tee_session,
+					   data->tee_command_id_base + TZCMD_SECMEM_ZALLOC,
+					   params);
+	if (ret)
+		return -ENOMEM;
+
+	sec_buf->sec_handle = params[3].u.value.a;
+	return 0;
+}
+
+static void secure_heap_tee_unsecure_memory(struct secure_heap *sec_heap,
+					    struct secure_buffer *sec_buf)
+{
+	struct tee_param params[TEE_PARAM_NUM] = {0};
+
+	params[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT;
+	params[0].u.value.a = sec_buf->sec_handle;
+	params[1].attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_OUTPUT;
+
+	secure_heap_tee_service_call(sec_heap->tee_ctx, sec_heap->tee_session,
+				     sec_heap->data->tee_command_id_base + TZCMD_SECMEM_FREE,
+				     params);
+	if (params[1].u.value.a)
+		pr_err("%s, free buffer(0x%x) return fail(%lld) from TEE.\n",
+		       sec_heap->name, sec_buf->sec_handle, params[1].u.value.a);
+}
+
 /* The memory allocating is within the TEE. */
 const struct secure_heap_prv_data mtk_sec_mem_data = {
 	.uuid			= TZ_TA_MEM_UUID_MTK,
 	.tee_impl_id		= TEE_IMPL_ID_OPTEE,
+	.tee_command_id_base	= TEE_MEM_COMMAND_ID_BASE_MTK,
+	.secure_the_memory	= secure_heap_tee_secure_memory,
+	.unsecure_the_memory	= secure_heap_tee_unsecure_memory,
 };
 
 static int secure_heap_secure_memory_allocate(struct secure_heap *sec_heap,
-- 
2.25.1

[PATCH v2 5/8] dma-buf: heaps: secure_heap: Add dma_ops

[Yong Wu]

Add the dma_ops for this secure heap.
a) For secure buffer, cache_ops/mmap are not allowed, thus return
EPERM for them.
b) The secure buffer can't be accessed in kernel, thus it doesn't
have va/dma_address for it. Use the dma_address property to save the
"secure handle".

Signed-off-by: Yong Wu <yong.wu@mediatek.com>
---
 drivers/dma-buf/heaps/secure_heap.c | 120 ++++++++++++++++++++++++++++
 1 file changed, 120 insertions(+)

diff --git a/drivers/dma-buf/heaps/secure_heap.c b/drivers/dma-buf/heaps/secure_heap.c
index 05062c14e7c7..25cc95442c56 100644
--- a/drivers/dma-buf/heaps/secure_heap.c
+++ b/drivers/dma-buf/heaps/secure_heap.c
@@ -9,6 +9,7 @@
 #include <linux/dma-heap.h>
 #include <linux/err.h>
 #include <linux/module.h>
+#include <linux/scatterlist.h>
 #include <linux/slab.h>
 #include <linux/tee_drv.h>
 #include <linux/uuid.h>
@@ -87,6 +88,10 @@ struct secure_heap {
 	const struct secure_heap_prv_data *data;
 };
 
+struct secure_heap_attachment {
+	struct sg_table			*table;
+};
+
 static int tee_ctx_match(struct tee_ioctl_version_data *ver, const void *data)
 {
 	const struct secure_heap_prv_data *d = data;
@@ -238,6 +243,120 @@ static void secure_heap_secure_memory_free(struct secure_heap *sec_heap,
 		data->memory_free(sec_heap, sec_buf);
 }
 
+static int secure_heap_attach(struct dma_buf *dmabuf, struct dma_buf_attachment *attachment)
+{
+	struct secure_buffer *sec_buf = dmabuf->priv;
+	struct secure_heap_attachment *a;
+	struct sg_table *table;
+	int ret;
+
+	a = kzalloc(sizeof(*a), GFP_KERNEL);
+	if (!a)
+		return -ENOMEM;
+
+	table = kzalloc(sizeof(*table), GFP_KERNEL);
+	if (!table) {
+		ret = -ENOMEM;
+		goto err_free_attach;
+	}
+
+	ret = sg_alloc_table(table, 1, GFP_KERNEL);
+	if (ret)
+		goto err_free_sgt;
+	sg_set_page(table->sgl, 0, sec_buf->size, 0);
+
+	a->table = table;
+	attachment->priv = a;
+
+	return 0;
+
+err_free_sgt:
+	kfree(table);
+err_free_attach:
+	kfree(a);
+	return ret;
+}
+
+static void secure_heap_detach(struct dma_buf *dmabuf, struct dma_buf_attachment *attachment)
+{
+	struct secure_heap_attachment *a = attachment->priv;
+
+	sg_free_table(a->table);
+	kfree(a->table);
+	kfree(a);
+}
+
+static struct sg_table *
+secure_heap_map_dma_buf(struct dma_buf_attachment *attachment, enum dma_data_direction direction)
+{
+	struct secure_heap_attachment *a = attachment->priv;
+	struct dma_buf *dmabuf = attachment->dmabuf;
+	struct secure_buffer *sec_buf = dmabuf->priv;
+	struct sg_table *table = a->table;
+
+	/*
+	 * Technically dma_address refers to the address used by HW, But for secure buffer
+	 * we don't know its dma_address in kernel, Instead, we only know its "secure handle".
+	 * Thus use this property to save the "secure handle", and the user will use it to
+	 * obtain the real address in secure world.
+	 *
+	 * Note: CONFIG_DMA_API_DEBUG requires it to be aligned with PAGE_SIZE.
+	 */
+	if (sec_buf->sec_handle) {
+		sg_dma_address(table->sgl) = sec_buf->sec_handle;
+		sg_dma_len(table->sgl) = sec_buf->size;
+	}
+	return table;
+}
+
+static void
+secure_heap_unmap_dma_buf(struct dma_buf_attachment *attachment, struct sg_table *table,
+			  enum dma_data_direction direction)
+{
+	struct secure_heap_attachment *a = attachment->priv;
+
+	WARN_ON(a->table != table);
+	sg_dma_address(table->sgl) = 0;
+	sg_dma_len(table->sgl) = 0;
+}
+
+static int
+secure_heap_dma_buf_begin_cpu_access(struct dma_buf *dmabuf, enum dma_data_direction direction)
+{
+	return -EPERM;
+}
+
+static int
+secure_heap_dma_buf_end_cpu_access(struct dma_buf *dmabuf, enum dma_data_direction direction)
+{
+	return -EPERM;
+}
+
+static int secure_heap_dma_buf_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma)
+{
+	return -EPERM;
+}
+
+static void secure_heap_free(struct dma_buf *dmabuf)
+{
+	struct secure_buffer *sec_buf = dmabuf->priv;
+	struct secure_heap *sec_heap = dma_heap_get_drvdata(sec_buf->heap);
+
+	secure_heap_secure_memory_free(sec_heap, sec_buf);
+	kfree(sec_buf);
+}
+
+static const struct dma_buf_ops sec_heap_buf_ops = {
+	.attach		= secure_heap_attach,
+	.detach		= secure_heap_detach,
+	.map_dma_buf	= secure_heap_map_dma_buf,
+	.unmap_dma_buf	= secure_heap_unmap_dma_buf,
+	.begin_cpu_access = secure_heap_dma_buf_begin_cpu_access,
+	.end_cpu_access	= secure_heap_dma_buf_end_cpu_access,
+	.mmap		= secure_heap_dma_buf_mmap,
+	.release	= secure_heap_free,
+};
+
 static struct dma_buf *
 secure_heap_allocate(struct dma_heap *heap, unsigned long size,
 		     unsigned long fd_flags, unsigned long heap_flags)
@@ -271,6 +390,7 @@ secure_heap_allocate(struct dma_heap *heap, unsigned long size,
 	if (ret)
 		goto err_free_buf;
 	exp_info.exp_name = dma_heap_get_name(heap);
+	exp_info.ops = &sec_heap_buf_ops;
 	exp_info.size = sec_buf->size;
 	exp_info.flags = fd_flags;
 	exp_info.priv = sec_buf;
-- 
2.25.1

[PATCH v2 6/8] dt-bindings: reserved-memory: Add secure CMA reserved memory range

[Yong Wu]

Add a binding for describing the secure CMA reserved memory range. The
memory range also will be defined in the TEE firmware. It means the TEE
will be configured with the same address/size that is being set in this
DT node.

Signed-off-by: Yong Wu <yong.wu@mediatek.com>
---
 .../reserved-memory/secure_cma_region.yaml    | 44 +++++++++++++++++++
 1 file changed, 44 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/reserved-memory/secure_cma_region.yaml

diff --git a/Documentation/devicetree/bindings/reserved-memory/secure_cma_region.yaml b/Documentation/devicetree/bindings/reserved-memory/secure_cma_region.yaml
new file mode 100644
index 000000000000..8ab559595fbe
--- /dev/null
+++ b/Documentation/devicetree/bindings/reserved-memory/secure_cma_region.yaml
@@ -0,0 +1,44 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/reserved-memory/secure_cma_region.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Secure Reserved CMA Region
+
+description:
+  This binding describes a CMA region that can dynamically transition
+between secure and non-secure states that a TEE can allocate memory
+from.
+
+maintainers:
+  - Yong Wu <yong.wu@mediatek.com>
+
+allOf:
+  - $ref: reserved-memory.yaml
+
+properties:
+  compatible:
+    const: secure_cma_region
+
+required:
+  - compatible
+  - reg
+  - reusable
+
+unevaluatedProperties: false
+
+examples:
+  - |
+
+    reserved-memory {
+        #address-cells = <1>;
+        #size-cells = <1>;
+        ranges;
+
+        reserved-memory@80000000 {
+            compatible = "secure_cma_region";
+            reusable;
+            reg = <0x80000000 0x18000000>;
+        };
+    };
-- 
2.25.1

[PATCH v2 7/8] dma_buf: heaps: secure_heap: Add a new MediaTek CMA heap

[Yong Wu]

Create a new MediaTek CMA heap from the CMA reserved buffer.

In this heap, When the first allocating buffer, use cma_alloc to prepare
whole the CMA range, then send its range to TEE to protect and manage.
For the later allocating, we just adds the cma_used_size_mtk.

This CMA flow may be different with the normal CMA heap of next patch.
So I named the variable with _mtk suffix like cma_page_mtk/
cma_used_size_mtk. This is also to distinguish it from the cma_page of
the buffer structure in the next patch.

When SVP done, cma_release will release the buffer, then kernel may
reuse it.

Meanwhile, this patch adds a "heap_init" pointer, while allows some heap
initialization operations. This case also checks if the CMA range is
ready.

Signed-off-by: Yong Wu <yong.wu@mediatek.com>
---
 drivers/dma-buf/heaps/secure_heap.c | 124 +++++++++++++++++++++++++++-
 1 file changed, 122 insertions(+), 2 deletions(-)

diff --git a/drivers/dma-buf/heaps/secure_heap.c b/drivers/dma-buf/heaps/secure_heap.c
index 25cc95442c56..f8b84fd16288 100644
--- a/drivers/dma-buf/heaps/secure_heap.c
+++ b/drivers/dma-buf/heaps/secure_heap.c
@@ -4,11 +4,12 @@
  *
  * Copyright (C) 2023 MediaTek Inc.
  */
-
+#include <linux/cma.h>
 #include <linux/dma-buf.h>
 #include <linux/dma-heap.h>
 #include <linux/err.h>
 #include <linux/module.h>
+#include <linux/of_reserved_mem.h>
 #include <linux/scatterlist.h>
 #include <linux/slab.h>
 #include <linux/tee_drv.h>
@@ -25,6 +26,8 @@ enum secure_buffer_tee_cmd { /* PARAM NUM always is 4. */
 	 * [in]  value[0].a: The buffer size.
 	 *       value[0].b: alignment.
 	 * [in]  value[1].a: enum secure_memory_type.
+	 * [in]  value[2].a: pa base in cma case.
+	 *       value[2].b: The buffer size in cma case.
 	 * [out] value[3].a: The secure handle.
 	 */
 	TZCMD_SECMEM_ZALLOC = 0,
@@ -45,6 +48,13 @@ enum secure_memory_type {
 	 * management is inside the TEE.
 	 */
 	SECURE_MEMORY_TYPE_MTK_CM_TZ	= 1,
+	/*
+	 * MediaTek dynamic chunk memory carved out from CMA.
+	 * In normal case, the CMA could be used in kernel; When SVP start, we will
+	 * allocate whole this CMA and pass whole the CMA PA and size into TEE to
+	 * protect it, then the detail memory management also is inside the TEE.
+	 */
+	SECURE_MEMORY_TYPE_MTK_CM_CMA	= 2,
 };
 
 struct secure_buffer {
@@ -70,6 +80,7 @@ struct secure_heap_prv_data {
 	 */
 	const int			tee_command_id_base;
 
+	int	(*heap_init)(struct secure_heap *sec_heap);
 	int	(*memory_alloc)(struct secure_heap *sec_heap, struct secure_buffer *sec_buf);
 	void	(*memory_free)(struct secure_heap *sec_heap, struct secure_buffer *sec_buf);
 
@@ -86,6 +97,13 @@ struct secure_heap {
 	u32				tee_session;
 
 	const struct secure_heap_prv_data *data;
+
+	struct cma		*cma;
+	struct page		*cma_page_mtk;
+	unsigned long		cma_paddr;
+	unsigned long		cma_size;
+	unsigned long		cma_used_size_mtk;
+	struct mutex		lock; /* lock for cma_used_size_mtk */
 };
 
 struct secure_heap_attachment {
@@ -168,7 +186,10 @@ static int secure_heap_tee_secure_memory(struct secure_heap *sec_heap,
 	params[1].attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT;
 	params[1].u.value.a = sec_heap->mem_type;
 	params[2].attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT;
-
+	if (sec_heap->cma && sec_heap->mem_type == SECURE_MEMORY_TYPE_MTK_CM_CMA) {
+		params[2].u.value.a = sec_heap->cma_paddr;
+		params[2].u.value.b = sec_heap->cma_size;
+	}
 	params[3].attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_OUTPUT;
 	ret = secure_heap_tee_service_call(sec_heap->tee_ctx, sec_heap->tee_session,
 					   data->tee_command_id_base + TZCMD_SECMEM_ZALLOC,
@@ -197,6 +218,66 @@ static void secure_heap_tee_unsecure_memory(struct secure_heap *sec_heap,
 		       sec_heap->name, sec_buf->sec_handle, params[1].u.value.a);
 }
 
+static int mtk_secure_memory_cma_allocate(struct secure_heap *sec_heap,
+					  struct secure_buffer *sec_buf)
+{
+	/*
+	 * Allocate CMA only when allocating buffer for the first time, and just
+	 * increase cma_used_size_mtk at the other time.
+	 */
+	mutex_lock(&sec_heap->lock);
+	if (sec_heap->cma_used_size_mtk)
+		goto add_size;
+
+	mutex_unlock(&sec_heap->lock);
+	sec_heap->cma_page_mtk = cma_alloc(sec_heap->cma, sec_heap->cma_size >> PAGE_SHIFT,
+					   get_order(PAGE_SIZE), false);
+	if (!sec_heap->cma_page_mtk)
+		return -ENOMEM;
+
+	mutex_lock(&sec_heap->lock);
+add_size:
+	sec_heap->cma_used_size_mtk += sec_buf->size;
+	mutex_unlock(&sec_heap->lock);
+
+	return 0;
+}
+
+static void mtk_secure_memory_cma_free(struct secure_heap *sec_heap,
+				       struct secure_buffer *sec_buf)
+{
+	bool cma_is_empty;
+
+	mutex_lock(&sec_heap->lock);
+	sec_heap->cma_used_size_mtk -= sec_buf->size;
+	cma_is_empty = !sec_heap->cma_used_size_mtk;
+	mutex_unlock(&sec_heap->lock);
+
+	if (cma_is_empty)
+		cma_release(sec_heap->cma, sec_heap->cma_page_mtk,
+			    sec_heap->cma_size >> PAGE_SHIFT);
+}
+
+static int mtk_secure_heap_cma_init(struct secure_heap *sec_heap)
+{
+	if (!sec_heap->cma)
+		return -EINVAL;
+	mutex_init(&sec_heap->lock);
+	return 0;
+}
+
+/* Use CMA to prepare the buffer and the memory allocating is within the TEE. */
+const struct secure_heap_prv_data mtk_sec_mem_data_cma = {
+	.uuid			= TZ_TA_MEM_UUID_MTK,
+	.tee_impl_id		= TEE_IMPL_ID_OPTEE,
+	.tee_command_id_base	= TEE_MEM_COMMAND_ID_BASE_MTK,
+	.heap_init		= mtk_secure_heap_cma_init,
+	.memory_alloc		= mtk_secure_memory_cma_allocate,
+	.memory_free		= mtk_secure_memory_cma_free,
+	.secure_the_memory	= secure_heap_tee_secure_memory,
+	.unsecure_the_memory	= secure_heap_tee_unsecure_memory,
+};
+
 /* The memory allocating is within the TEE. */
 const struct secure_heap_prv_data mtk_sec_mem_data = {
 	.uuid			= TZ_TA_MEM_UUID_MTK,
@@ -420,20 +501,59 @@ static struct secure_heap secure_heaps[] = {
 		.mem_type	= SECURE_MEMORY_TYPE_MTK_CM_TZ,
 		.data		= &mtk_sec_mem_data,
 	},
+	{
+		.name		= "secure_mtk_cma",
+		.mem_type	= SECURE_MEMORY_TYPE_MTK_CM_CMA,
+		.data		= &mtk_sec_mem_data_cma,
+	},
 };
 
+static int __init secure_cma_init(struct reserved_mem *rmem)
+{
+	struct secure_heap *sec_heap = secure_heaps;
+	struct cma *sec_cma;
+	int ret, i;
+
+	ret = cma_init_reserved_mem(rmem->base, rmem->size, 0, rmem->name,
+				    &sec_cma);
+	if (ret) {
+		pr_err("%s: %s set up CMA fail\n", __func__, rmem->name);
+		return ret;
+	}
+
+	for (i = 0; i < ARRAY_SIZE(secure_heaps); i++, sec_heap++) {
+		if (sec_heap->mem_type != SECURE_MEMORY_TYPE_MTK_CM_CMA)
+			continue;
+
+		sec_heap->cma = sec_cma;
+		sec_heap->cma_paddr = rmem->base;
+		sec_heap->cma_size = rmem->size;
+	}
+	return 0;
+}
+
+RESERVEDMEM_OF_DECLARE(secure_cma, "secure_cma_region", secure_cma_init);
+
 static int secure_heap_init(void)
 {
 	struct secure_heap *sec_heap = secure_heaps;
 	struct dma_heap_export_info exp_info;
 	struct dma_heap *heap;
 	unsigned int i;
+	int ret;
 
 	for (i = 0; i < ARRAY_SIZE(secure_heaps); i++, sec_heap++) {
 		exp_info.name = sec_heap->name;
 		exp_info.ops = &sec_heap_ops;
 		exp_info.priv = (void *)sec_heap;
 
+		if (sec_heap->data && sec_heap->data->heap_init) {
+			ret = sec_heap->data->heap_init(sec_heap);
+			if (ret) {
+				pr_err("sec_heap %s init fail %d.\n", sec_heap->name, ret);
+				continue;
+			}
+		}
 		heap = dma_heap_add(&exp_info);
 		if (IS_ERR(heap))
 			return PTR_ERR(heap);
-- 
2.25.1

[PATCH v2 8/8] dma-buf: heaps: secure_heap: Add normal CMA heap

[Yong Wu]

Add a normal CMA heap which use the standard cma allocate.

Signed-off-by: Yong Wu <yong.wu@mediatek.com>
---
Hi Vijay and Jaskaran,

For this heap,
1) It uses sec_heap_buf_ops currently. I guess we cann't use the
cma_heap_buf_ops. since if it is secure buffer, some operations such
as mmap should not be allowed.
2) I didn't add how to protect/secure the buffer.

Please feel free to change to meet your requirements.
Thanks.
---
 drivers/dma-buf/heaps/secure_heap.c | 38 ++++++++++++++++++++++++++++-
 1 file changed, 37 insertions(+), 1 deletion(-)

diff --git a/drivers/dma-buf/heaps/secure_heap.c b/drivers/dma-buf/heaps/secure_heap.c
index f8b84fd16288..8989ad5d03e9 100644
--- a/drivers/dma-buf/heaps/secure_heap.c
+++ b/drivers/dma-buf/heaps/secure_heap.c
@@ -43,6 +43,8 @@ enum secure_buffer_tee_cmd { /* PARAM NUM always is 4. */
 };
 
 enum secure_memory_type {
+	/* CMA for the secure memory, Use the normal cma ops to alloc/free. */
+	SECURE_MEMORY_TYPE_CMA		= 0,
 	/*
 	 * MediaTek static chunk memory carved out for TrustZone. The memory
 	 * management is inside the TEE.
@@ -65,6 +67,7 @@ struct secure_buffer {
 	 * a value got from TEE.
 	 */
 	u32				sec_handle;
+	struct page			*cma_page;
 };
 
 #define TEE_MEM_COMMAND_ID_BASE_MTK	0x10000
@@ -287,6 +290,33 @@ const struct secure_heap_prv_data mtk_sec_mem_data = {
 	.unsecure_the_memory	= secure_heap_tee_unsecure_memory,
 };
 
+static int cma_secure_memory_allocate(struct secure_heap *sec_heap,
+				      struct secure_buffer *sec_buf)
+{
+	if (!sec_heap->cma)
+		return -EINVAL;
+
+	sec_buf->cma_page = cma_alloc(sec_heap->cma, sec_buf->size >> PAGE_SHIFT,
+				      get_order(PAGE_SIZE), false);
+	if (!sec_buf->cma_page)
+		return -ENOMEM;
+
+	memset(page_address(sec_buf->cma_page), 0, sec_buf->size);
+	return 0;
+}
+
+static void cma_secure_memory_free(struct secure_heap *sec_heap,
+				   struct secure_buffer *sec_buf)
+{
+	cma_release(sec_heap->cma, sec_buf->cma_page, sec_buf->size >> PAGE_SHIFT);
+}
+
+const struct secure_heap_prv_data cma_sec_mem_data = {
+	.memory_alloc	= cma_secure_memory_allocate,
+	.memory_free	= cma_secure_memory_free,
+	/* TODO : secure the buffer. */
+};
+
 static int secure_heap_secure_memory_allocate(struct secure_heap *sec_heap,
 					      struct secure_buffer *sec_buf)
 {
@@ -496,6 +526,11 @@ static const struct dma_heap_ops sec_heap_ops = {
 };
 
 static struct secure_heap secure_heaps[] = {
+	{
+		.name		= "secure_cma",
+		.mem_type	= SECURE_MEMORY_TYPE_CMA,
+		.data		= &cma_sec_mem_data,
+	},
 	{
 		.name		= "secure_mtk_cm",
 		.mem_type	= SECURE_MEMORY_TYPE_MTK_CM_TZ,
@@ -522,7 +557,8 @@ static int __init secure_cma_init(struct reserved_mem *rmem)
 	}
 
 	for (i = 0; i < ARRAY_SIZE(secure_heaps); i++, sec_heap++) {
-		if (sec_heap->mem_type != SECURE_MEMORY_TYPE_MTK_CM_CMA)
+		if (sec_heap->mem_type != SECURE_MEMORY_TYPE_MTK_CM_CMA &&
+		    sec_heap->mem_type != SECURE_MEMORY_TYPE_CMA)
 			continue;
 
 		sec_heap->cma = sec_cma;
-- 
2.25.1

Re: [PATCH v2 6/8] dt-bindings: reserved-memory: Add secure CMA reserved memory range

[Krzysztof Kozlowski]

On 11/11/2023 12:15, Yong Wu wrote:
> Add a binding for describing the secure CMA reserved memory range. The
> memory range also will be defined in the TEE firmware. It means the TEE
> will be configured with the same address/size that is being set in this
> DT node.
> 
> Signed-off-by: Yong Wu <yong.wu@mediatek.com>
> ---

What was the outcome of previous discussion? I don't see any references
to the conclusion and your changelog "Reword the dt-binding description"
is way too generic.

You must explain what happened here.

>  .../reserved-memory/secure_cma_region.yaml    | 44 +++++++++++++++++++
>  1 file changed, 44 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/reserved-memory/secure_cma_region.yaml
> 
> diff --git a/Documentation/devicetree/bindings/reserved-memory/secure_cma_region.yaml b/Documentation/devicetree/bindings/reserved-memory/secure_cma_region.yaml
> new file mode 100644
> index 000000000000..8ab559595fbe
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/reserved-memory/secure_cma_region.yaml
> @@ -0,0 +1,44 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/reserved-memory/secure_cma_region.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Secure Reserved CMA Region
> +
> +description:
> +  This binding describes a CMA region that can dynamically transition

Describe the hardware or firmware, not the binding. Drop first four
words and rephrase it.

> +between secure and non-secure states that a TEE can allocate memory
> +from.

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

Do not send untested code.

> +
> +maintainers:
> +  - Yong Wu <yong.wu@mediatek.com>
> +
> +allOf:
> +  - $ref: reserved-memory.yaml
> +
> +properties:
> +  compatible:
> +    const: secure_cma_region

Still wrong compatible. Look at other bindings - there is nowhere
underscore. Look at other reserved memory bindings especially.

Also, CMA is a Linux thingy, so either not suitable for bindings at all,
or you need Linux specific compatible. I don't quite get why do you even
put CMA there - adding Linux specific stuff will get obvious pushback...


> +
> +required:
> +  - compatible
> +  - reg
> +  - reusable
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +

Stray blank line.

> +    reserved-memory {
> +        #address-cells = <1>;
> +        #size-cells = <1>;
> +        ranges;
> +
> +        reserved-memory@80000000 {
> +            compatible = "secure_cma_region";
> +            reusable;
> +            reg = <0x80000000 0x18000000>;

reg is second property. Open DTS and check how it is there.

> +        };
> +    };

Best regards,
Krzysztof

Re: [PATCH v2 3/8] dma-buf: heaps: secure_heap: Initialize tee session

[kernel test robot]

Hi Yong,

kernel test robot noticed the following build errors:

[auto build test ERROR on drm-misc/drm-misc-next]
[also build test ERROR on robh/for-next drm-tip/drm-tip linus/master v6.6 next-20231110]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Yong-Wu/dma-buf-heaps-Initialize-a-secure-heap/20231111-192115
base:   git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
patch link:    https://lore.kernel.org/r/20231111111559.8218-4-yong.wu%40mediatek.com
patch subject: [PATCH v2 3/8] dma-buf: heaps: secure_heap: Initialize tee session
config: m68k-allmodconfig (https://download.01.org/0day-ci/archive/20231112/202311120053.qXNIYBzk-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231112/202311120053.qXNIYBzk-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202311120053.qXNIYBzk-lkp@intel.com/

All errors (new ones prefixed by >>):

   m68k-linux-ld: drivers/dma-buf/heaps/secure_heap.o: in function `secure_heap_tee_session_init':
   secure_heap.c:(.text+0xc0): undefined reference to `tee_client_open_context'
>> m68k-linux-ld: secure_heap.c:(.text+0x134): undefined reference to `tee_client_open_session'
>> m68k-linux-ld: secure_heap.c:(.text+0x180): undefined reference to `tee_client_close_context'

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v2 3/8] dma-buf: heaps: secure_heap: Initialize tee session

[kernel test robot]

Hi Yong,

kernel test robot noticed the following build errors:

[auto build test ERROR on drm-misc/drm-misc-next]
[also build test ERROR on robh/for-next drm-tip/drm-tip linus/master v6.6 next-20231110]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Yong-Wu/dma-buf-heaps-Initialize-a-secure-heap/20231111-192115
base:   git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
patch link:    https://lore.kernel.org/r/20231111111559.8218-4-yong.wu%40mediatek.com
patch subject: [PATCH v2 3/8] dma-buf: heaps: secure_heap: Initialize tee session
config: s390-allmodconfig (https://download.01.org/0day-ci/archive/20231112/202311120136.o1VqalXm-lkp@intel.com/config)
compiler: s390-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231112/202311120136.o1VqalXm-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202311120136.o1VqalXm-lkp@intel.com/

All errors (new ones prefixed by >>):

   s390-linux-ld: drivers/dma-buf/heaps/secure_heap.o: in function `secure_heap_tee_session_init':
   secure_heap.c:(.text+0x256): undefined reference to `tee_client_open_context'
>> s390-linux-ld: secure_heap.c:(.text+0x3e0): undefined reference to `tee_client_open_session'
>> s390-linux-ld: secure_heap.c:(.text+0x52a): undefined reference to `tee_client_close_context'

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v2 4/8] dma-buf: heaps: secure_heap: Add tee memory service call

[kernel test robot]

Hi Yong,

kernel test robot noticed the following build errors:

[auto build test ERROR on drm-misc/drm-misc-next]
[also build test ERROR on robh/for-next drm-tip/drm-tip linus/master v6.6 next-20231110]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Yong-Wu/dma-buf-heaps-Initialize-a-secure-heap/20231111-192115
base:   git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
patch link:    https://lore.kernel.org/r/20231111111559.8218-5-yong.wu%40mediatek.com
patch subject: [PATCH v2 4/8] dma-buf: heaps: secure_heap: Add tee memory service call
config: powerpc-allmodconfig (https://download.01.org/0day-ci/archive/20231112/202311120707.FDrzrRME-lkp@intel.com/config)
compiler: powerpc64-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231112/202311120707.FDrzrRME-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202311120707.FDrzrRME-lkp@intel.com/

All errors (new ones prefixed by >>):

   powerpc64-linux-ld: warning: discarding dynamic section .glink
   powerpc64-linux-ld: warning: discarding dynamic section .plt
   powerpc64-linux-ld: linkage table error against `tee_client_open_session'
   powerpc64-linux-ld: stubs don't match calculated size
   powerpc64-linux-ld: can not build stubs: bad value
   powerpc64-linux-ld: drivers/dma-buf/heaps/secure_heap.o: in function `secure_heap_tee_session_init':
   secure_heap.c:(.text.secure_heap_tee_session_init+0xd0): undefined reference to `tee_client_open_context'
   powerpc64-linux-ld: secure_heap.c:(.text.secure_heap_tee_session_init+0x2b4): undefined reference to `tee_client_open_session'
   powerpc64-linux-ld: secure_heap.c:(.text.secure_heap_tee_session_init+0x458): undefined reference to `tee_client_close_context'
   powerpc64-linux-ld: drivers/dma-buf/heaps/secure_heap.o: in function `secure_heap_tee_service_call.constprop.0':
>> secure_heap.c:(.text.secure_heap_tee_service_call.constprop.0+0xbc): undefined reference to `tee_client_invoke_func'

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v2 6/8] dt-bindings: reserved-memory: Add secure CMA reserved memory range

[Yong Wu (吴勇)]

On Sat, 2023-11-11 at 13:48 +0100, Krzysztof Kozlowski wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  On 11/11/2023 12:15, Yong Wu wrote:
> > Add a binding for describing the secure CMA reserved memory range.
> The
> > memory range also will be defined in the TEE firmware. It means the
> TEE
> > will be configured with the same address/size that is being set in
> this
> > DT node.
> > 
> > Signed-off-by: Yong Wu <yong.wu@mediatek.com>
> > ---
> 
> What was the outcome of previous discussion? I don't see any
> references
> to the conclusion and your changelog "Reword the dt-binding
> description"
> is way too generic.
> 
> You must explain what happened here.

I don't think there is a final conclusion yet in v1. Jeff helped
explain that this region also is defined in TEE firmware. I put this a
bit in the commit message above.

Sorry for confusing.

> 
> >  .../reserved-memory/secure_cma_region.yaml    | 44
> +++++++++++++++++++
> >  1 file changed, 44 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/reserved-
> memory/secure_cma_region.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/reserved-
> memory/secure_cma_region.yaml
> b/Documentation/devicetree/bindings/reserved-
> memory/secure_cma_region.yaml
> > new file mode 100644
> > index 000000000000..8ab559595fbe
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/reserved-
> memory/secure_cma_region.yaml
> > @@ -0,0 +1,44 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: 
> http://devicetree.org/schemas/reserved-memory/secure_cma_region.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Secure Reserved CMA Region

Will change to: Secure Region. Is it ok?

> > +
> > +description:
> > +  This binding describes a CMA region that can dynamically
> transition
> 
> Describe the hardware or firmware, not the binding. Drop first four
> words and rephrase it.

Memory region for TEE usage, which is also defined in the TEE firmware.
When an activity (e.g. secure video playback) requiring usage of this
starts, this region will be protected by MPU (Memory Protect Unit) in
the TEE firmware. After the activity is completed, the region will be
unprotected by the TEE and usable by the non-secure side (i.e. kernel
and userspace).

Does this description make sense for you?

> 
> > +between secure and non-secure states that a TEE can allocate
> memory
> > +from.
> 
> It does not look like you tested the bindings, at least after quick
> look. Please run `make dt_binding_check` (see
> Documentation/devicetree/bindings/writing-schema.rst for
> instructions).
> Maybe you need to update your dtschema and yamllint.
> 
> Do not send untested code.

Sorry. I will update them and test this before sending.

> 
> > +
> > +maintainers:
> > +  - Yong Wu <yong.wu@mediatek.com>
> > +
> > +allOf:
> > +  - $ref: reserved-memory.yaml
> > +
> > +properties:
> > +  compatible:
> > +    const: secure_cma_region
> 
> Still wrong compatible. Look at other bindings - there is nowhere
> underscore. Look at other reserved memory bindings especially.
> 
> Also, CMA is a Linux thingy, so either not suitable for bindings at
> all,
> or you need Linux specific compatible. I don't quite get why do you
> evennot
> put CMA there - adding Linux specific stuff will get obvious
> pushback...

Thanks. I will change to: secure-region. Is this ok?

> 
> 
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - reusable
> > +
> > +unevaluatedProperties: false
> > +
> > +examples:
> > +  - |
> > +
> 
> Stray blank line.

Thanks for reviewing so careful. Will fix this and below.

> 
> > +    reserved-memory {
> > +        #address-cells = <1>;
> > +        #size-cells = <1>;
> > +        ranges;
> > +
> > +        reserved-memory@80000000 {
> > +            compatible = "secure_cma_region";
> > +            reusable;
> > +            reg = <0x80000000 0x18000000>;
> 
> reg is second property. Open DTS and check how it is there.
> 
> > +        };
> > +    };
> 
> Best regards,
> Krzysztof
> 

Re: [PATCH v2 0/8] dma-buf: heaps: Add secure heap

[Pavel Machek]

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

Hi!

> This patchset adds three secure heaps:
> 1) secure_mtk_cm: secure chunk memory for MediaTek SVP (Secure Video Path).
>    The buffer is reserved for the secure world after bootup and it is used
>    for vcodec's ES/working buffer;
> 2) secure_mtk_cma: secure CMA memory for MediaTek SVP. This buffer is
>    dynamically reserved for the secure world and will be got when we start
>    playing secure videos, Once the security video playing is complete, the
>    CMA will be released. This heap is used for the vcodec's frame buffer. 
> 3) secure_cma: Use the kerne CMA ops as the allocation ops. 
>    currently it is a draft version for Vijay and Jaskaran.

Is there high-level description of what the security goals here are,
somewhere?

BR,
									Pavel
-- 
People of Russia, stop Putin before his war on Ukraine escalates.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH v2 6/8] dt-bindings: reserved-memory: Add secure CMA reserved memory range

[Rob Herring]

On Sat, 11 Nov 2023 19:15:57 +0800, Yong Wu wrote:
> Add a binding for describing the secure CMA reserved memory range. The
> memory range also will be defined in the TEE firmware. It means the TEE
> will be configured with the same address/size that is being set in this
> DT node.
> 
> Signed-off-by: Yong Wu <yong.wu@mediatek.com>
> ---
>  .../reserved-memory/secure_cma_region.yaml    | 44 +++++++++++++++++++
>  1 file changed, 44 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/reserved-memory/secure_cma_region.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:
./Documentation/devicetree/bindings/reserved-memory/secure_cma_region.yaml:12:1: [error] syntax error: could not find expected ':' (syntax)

dtschema/dtc warnings/errors:
make[2]: *** Deleting file 'Documentation/devicetree/bindings/reserved-memory/secure_cma_region.example.dts'
Documentation/devicetree/bindings/reserved-memory/secure_cma_region.yaml:12:1: could not find expected ':'
make[2]: *** [Documentation/devicetree/bindings/Makefile:26: Documentation/devicetree/bindings/reserved-memory/secure_cma_region.example.dts] Error 1
make[2]: *** Waiting for unfinished jobs....
./Documentation/devicetree/bindings/reserved-memory/secure_cma_region.yaml:12:1: could not find expected ':'
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/reserved-memory/secure_cma_region.yaml: ignoring, error parsing file
make[1]: *** [/builds/robherring/dt-review-ci/linux/Makefile:1427: dt_binding_check] Error 2
make: *** [Makefile:234: __sub-make] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20231111111559.8218-7-yong.wu@mediatek.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH v2 6/8] dt-bindings: reserved-memory: Add secure CMA reserved memory range

[Robin Murphy]

On 13/11/2023 6:37 am, Yong Wu (吴勇) wrote:
[...]
>>> +properties:
>>> +  compatible:
>>> +    const: secure_cma_region
>>
>> Still wrong compatible. Look at other bindings - there is nowhere
>> underscore. Look at other reserved memory bindings especially.
>>
>> Also, CMA is a Linux thingy, so either not suitable for bindings at
>> all,
>> or you need Linux specific compatible. I don't quite get why do you
>> evennot
>> put CMA there - adding Linux specific stuff will get obvious
>> pushback...
> 
> Thanks. I will change to: secure-region. Is this ok?

No, the previous discussion went off in entirely the wrong direction. To 
reiterate, the point of the binding is not to describe the expected 
usage of the thing nor the general concept of the thing, but to describe 
the actual thing itself. There are any number of different ways software 
may interact with a "secure region", so that is meaningless as a 
compatible. It needs to describe *this* secure memory interface offered 
by *this* TEE, so that software knows that to use it requires making 
those particular SiP calls with that particular UUID etc.

Thanks,
Robin.

Re: [PATCH v2 0/8] dma-buf: heaps: Add secure heap

[Jeffrey Kardatzke]

The main goal is for secure video playback, and to also enable other
potential uses of this in the future. The 'secure dma-heap' will be
used to allocate dma_buf objects that reference memory in the secure
world that is inaccessible/unmappable by the non-secure (i.e.
kernel/userspace) world.  That memory will be used by the secure world
to store secure information (i.e. decrypted media content). The
dma_bufs allocated from the kernel will be passed to V4L2 for video
decoding (as input and output). They will also be used by the drm
system for rendering of the content.

Hope that helps.

Cheers,
Jeff

On Mon, Nov 13, 2023 at 3:38 AM Pavel Machek <pavel@ucw.cz> wrote:
>
> Hi!
>
> > This patchset adds three secure heaps:
> > 1) secure_mtk_cm: secure chunk memory for MediaTek SVP (Secure Video Path).
> >    The buffer is reserved for the secure world after bootup and it is used
> >    for vcodec's ES/working buffer;
> > 2) secure_mtk_cma: secure CMA memory for MediaTek SVP. This buffer is
> >    dynamically reserved for the secure world and will be got when we start
> >    playing secure videos, Once the security video playing is complete, the
> >    CMA will be released. This heap is used for the vcodec's frame buffer.
> > 3) secure_cma: Use the kerne CMA ops as the allocation ops.
> >    currently it is a draft version for Vijay and Jaskaran.
>
> Is there high-level description of what the security goals here are,
> somewhere?
>
> BR,
>                                                                         Pavel
> --
> People of Russia, stop Putin before his war on Ukraine escalates.

Re: [PATCH v2 1/8] dma-buf: heaps: Initialize a secure heap

[Jeffrey Kardatzke]

On Sat, Nov 11, 2023 at 3:16 AM Yong Wu <yong.wu@mediatek.com> wrote:
>
> Initialize a secure heap. Currently just add a null heap, Prepare for
> the later patches.
>
> Signed-off-by: Yong Wu <yong.wu@mediatek.com>
> ---
>  drivers/dma-buf/heaps/Kconfig       |  7 +++
>  drivers/dma-buf/heaps/Makefile      |  1 +
>  drivers/dma-buf/heaps/secure_heap.c | 98 +++++++++++++++++++++++++++++
>  3 files changed, 106 insertions(+)
>  create mode 100644 drivers/dma-buf/heaps/secure_heap.c
>
> diff --git a/drivers/dma-buf/heaps/Kconfig b/drivers/dma-buf/heaps/Kconfig
> index a5eef06c4226..e358bf711145 100644
> --- a/drivers/dma-buf/heaps/Kconfig
> +++ b/drivers/dma-buf/heaps/Kconfig
> @@ -12,3 +12,10 @@ config DMABUF_HEAPS_CMA
>           Choose this option to enable dma-buf CMA heap. This heap is backed
>           by the Contiguous Memory Allocator (CMA). If your system has these
>           regions, you should say Y here.
> +
> +config DMABUF_HEAPS_SECURE
> +       bool "DMA-BUF Secure Heap"
> +       depends on DMABUF_HEAPS && TEE
> +       help
> +         Choose this option to enable dma-buf secure heap. This heap is backed by
> +         TEE client interfaces or CMA. If in doubt, say N.

Remove the mention of TEE and CMA from this.

You should probably add two KConfig options. One is for
DMABUF_HEAPS_SECURE which is for the framework for secure heaps. The
other one should be:

config MTK_DMABUF_HEAPS_SECURE
    bool "Mediatek DMA-BUF Secure Heap"
    depends on DMABUF_HEAPS_SECURE && TEE
    help
        Enables secure dma-buf heaps for Mediatek platforms.


> diff --git a/drivers/dma-buf/heaps/Makefile b/drivers/dma-buf/heaps/Makefile
> index 974467791032..b1ad9d1f2fbe 100644
> --- a/drivers/dma-buf/heaps/Makefile
> +++ b/drivers/dma-buf/heaps/Makefile
> @@ -1,3 +1,4 @@
>  # SPDX-License-Identifier: GPL-2.0
> +obj-$(CONFIG_DMABUF_HEAPS_SECURE)      += secure_heap.o
>  obj-$(CONFIG_DMABUF_HEAPS_SYSTEM)      += system_heap.o
>  obj-$(CONFIG_DMABUF_HEAPS_CMA)         += cma_heap.o
> diff --git a/drivers/dma-buf/heaps/secure_heap.c b/drivers/dma-buf/heaps/secure_heap.c
> new file mode 100644
> index 000000000000..a634051a0a67
> --- /dev/null
> +++ b/drivers/dma-buf/heaps/secure_heap.c
> @@ -0,0 +1,98 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * DMABUF secure heap exporter
> + *
> + * Copyright (C) 2023 MediaTek Inc.
> + */
> +
> +#include <linux/dma-buf.h>
> +#include <linux/dma-heap.h>
> +#include <linux/err.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +
> +enum secure_memory_type {
> +       /*
> +        * MediaTek static chunk memory carved out for TrustZone. The memory
> +        * management is inside the TEE.
> +        */
> +       SECURE_MEMORY_TYPE_MTK_CM_TZ    = 1,

Mediatek specific code for secure dma heaps should go into a new file
(maybe secure_heap_mtk.c which the MTK_DMABUF_HEAPS_SECURE option
enables).

> +};
> +
> +struct secure_buffer {
> +       struct dma_heap                 *heap;
> +       size_t                          size;
> +};
> +
> +struct secure_heap {
> +       const char                      *name;
> +       const enum secure_memory_type   mem_type;
secure_memory_type is going to be in the vendor specific
implementation, I don't think you need it in the framework.

> +};

You should probably move these to a <linux/dma-heap-secure.h> file so
they can be shared by the framework and the specific implementation
(in this case vendor specific).

> +
> +static struct dma_buf *
> +secure_heap_allocate(struct dma_heap *heap, unsigned long size,
> +                    unsigned long fd_flags, unsigned long heap_flags)
> +{
> +       struct secure_buffer *sec_buf;
> +       DEFINE_DMA_BUF_EXPORT_INFO(exp_info);
> +       struct dma_buf *dmabuf;
> +       int ret;
> +
> +       sec_buf = kzalloc(sizeof(*sec_buf), GFP_KERNEL);
> +       if (!sec_buf)
> +               return ERR_PTR(-ENOMEM);
> +
> +       sec_buf->size = ALIGN(size, PAGE_SIZE);
> +       sec_buf->heap = heap;
> +
> +       exp_info.exp_name = dma_heap_get_name(heap);
> +       exp_info.size = sec_buf->size;
> +       exp_info.flags = fd_flags;
> +       exp_info.priv = sec_buf;
> +
> +       dmabuf = dma_buf_export(&exp_info);
> +       if (IS_ERR(dmabuf)) {
> +               ret = PTR_ERR(dmabuf);
> +               goto err_free_buf;
> +       }
> +
> +       return dmabuf;
> +
> +err_free_buf:
> +       kfree(sec_buf);
> +       return ERR_PTR(ret);
> +}
> +
> +static const struct dma_heap_ops sec_heap_ops = {
> +       .allocate = secure_heap_allocate,
> +};
> +
> +static struct secure_heap secure_heaps[] = {
> +       {
> +               .name           = "secure_mtk_cm",
> +               .mem_type       = SECURE_MEMORY_TYPE_MTK_CM_TZ,
> +       },
> +};

Move this to the vendor specific implementation.

> +
> +static int secure_heap_init(void)
> +{
> +       struct secure_heap *sec_heap = secure_heaps;
> +       struct dma_heap_export_info exp_info;
> +       struct dma_heap *heap;
> +       unsigned int i;
> +
> +       for (i = 0; i < ARRAY_SIZE(secure_heaps); i++, sec_heap++) {
> +               exp_info.name = sec_heap->name;
> +               exp_info.ops = &sec_heap_ops;
> +               exp_info.priv = (void *)sec_heap;
> +
> +               heap = dma_heap_add(&exp_info);
> +               if (IS_ERR(heap))
> +                       return PTR_ERR(heap);
> +       }
> +       return 0;
> +}

secure_heap_init should take a 'struct secure_heap*' as an argument
and be defined in dma-heap-secure.h.

> +
> +module_init(secure_heap_init);
> +MODULE_DESCRIPTION("Secure Heap Driver");
> +MODULE_LICENSE("GPL");

Remove from this file, it should go in the specific implementations.

> --
> 2.25.1
>

Re: [PATCH v2 2/8] dma-buf: heaps: secure_heap: Add private heap ops

[Jeffrey Kardatzke]

On Sat, Nov 11, 2023 at 3:16 AM Yong Wu <yong.wu@mediatek.com> wrote:
>
> For the secure memory, there are two steps:
> a) Allocate buffers in kernel side;
> b) Secure that buffer.
> Different heaps may have different buffer allocation methods and
> different memory protection methods. Here abstract the memory
> allocation and securing operations.
>
> Signed-off-by: Yong Wu <yong.wu@mediatek.com>
> ---
>  drivers/dma-buf/heaps/secure_heap.c | 58 ++++++++++++++++++++++++++++-
>  1 file changed, 57 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/dma-buf/heaps/secure_heap.c b/drivers/dma-buf/heaps/secure_heap.c
> index a634051a0a67..87ac23072e9e 100644
> --- a/drivers/dma-buf/heaps/secure_heap.c
> +++ b/drivers/dma-buf/heaps/secure_heap.c
> @@ -24,15 +24,66 @@ struct secure_buffer {
>         size_t                          size;
>  };
>
> +struct secure_heap;
> +
> +struct secure_heap_prv_data {
> +       int     (*memory_alloc)(struct secure_heap *sec_heap, struct secure_buffer *sec_buf);
> +       void    (*memory_free)(struct secure_heap *sec_heap, struct secure_buffer *sec_buf);
> +
> +       /* Protect/unprotect the memory */
> +       int     (*secure_the_memory)(struct secure_heap *sec_heap, struct secure_buffer *sec_buf);
> +       void    (*unsecure_the_memory)(struct secure_heap *sec_heap, struct secure_buffer *sec_buf);
> +};
Move these into dma-heap-secure.h per the comments on the prior patch.

> +
>  struct secure_heap {
>         const char                      *name;
>         const enum secure_memory_type   mem_type;
> +
> +       const struct secure_heap_prv_data *data;
>  };
>
> +static int secure_heap_secure_memory_allocate(struct secure_heap *sec_heap,
> +                                             struct secure_buffer *sec_buf)
> +{
> +       const struct secure_heap_prv_data *data = sec_heap->data;
> +       int ret;
> +
> +       if (data->memory_alloc) {
> +               ret = data->memory_alloc(sec_heap, sec_buf);
> +               if (ret)
> +                       return ret;
> +       }
You should probably always require that memory_alloc is defined
(secure_the_memory can be optional, as that may be part of the
allocation).
> +
> +       if (data->secure_the_memory) {
> +               ret = data->secure_the_memory(sec_heap, sec_buf);
> +               if (ret)
> +                       goto sec_memory_free;
> +       }
> +       return 0;
> +
> +sec_memory_free:
> +       if (data->memory_free)
> +               data->memory_free(sec_heap, sec_buf);
You should probably always require that memory_free is defined.
> +       return ret;
> +}
> +
> +static void secure_heap_secure_memory_free(struct secure_heap *sec_heap,
> +                                          struct secure_buffer *sec_buf)
> +{
> +       const struct secure_heap_prv_data *data = sec_heap->data;
> +
> +       if (data->unsecure_the_memory)
> +               data->unsecure_the_memory(sec_heap, sec_buf);
> +
> +       if (data->memory_free)
> +               data->memory_free(sec_heap, sec_buf);
You should probably always require that memory_free is defined.
> +}
> +
>  static struct dma_buf *
>  secure_heap_allocate(struct dma_heap *heap, unsigned long size,
>                      unsigned long fd_flags, unsigned long heap_flags)
>  {
> +       struct secure_heap *sec_heap = dma_heap_get_drvdata(heap);
>         struct secure_buffer *sec_buf;
>         DEFINE_DMA_BUF_EXPORT_INFO(exp_info);
>         struct dma_buf *dmabuf;
> @@ -45,6 +96,9 @@ secure_heap_allocate(struct dma_heap *heap, unsigned long size,
>         sec_buf->size = ALIGN(size, PAGE_SIZE);
>         sec_buf->heap = heap;
>
> +       ret = secure_heap_secure_memory_allocate(sec_heap, sec_buf);
> +       if (ret)
> +               goto err_free_buf;
>         exp_info.exp_name = dma_heap_get_name(heap);
>         exp_info.size = sec_buf->size;
>         exp_info.flags = fd_flags;
> @@ -53,11 +107,13 @@ secure_heap_allocate(struct dma_heap *heap, unsigned long size,
>         dmabuf = dma_buf_export(&exp_info);
>         if (IS_ERR(dmabuf)) {
>                 ret = PTR_ERR(dmabuf);
> -               goto err_free_buf;
> +               goto err_free_sec_mem;
>         }
>
>         return dmabuf;
>
> +err_free_sec_mem:
> +       secure_heap_secure_memory_free(sec_heap, sec_buf);
>  err_free_buf:
>         kfree(sec_buf);
>         return ERR_PTR(ret);
> --
> 2.25.1
>

Re: [PATCH v2 3/8] dma-buf: heaps: secure_heap: Initialize tee session

[Jeffrey Kardatzke]

Everything in this patch set should move into the MTK specific
implementation I suggested in patch 1 (secure_heap_mtk.c)

On Sat, Nov 11, 2023 at 3:17 AM Yong Wu <yong.wu@mediatek.com> wrote:
>
> The TEE probe later than dma-buf heap, and PROBE_DEDER doesn't work
> here since this is not a platform driver, therefore initialize the TEE
> context/session while we allocate the first secure buffer.
>
> Add our special UUID and tee type in the private data.
>
> If the uuid is zero, it means that it doesn't enter TEE to protect the
> buffer, there may be other ways to protect the buffer.
>
> All the MTK chrome projects use this UUID. The UUID is only used in the
> kernelspace while userspace never use it. The userspace could allocate the
> secure memory via the existing dma-buf ioctl.
>
> Signed-off-by: Yong Wu <yong.wu@mediatek.com>
> ---
>  drivers/dma-buf/heaps/secure_heap.c | 75 +++++++++++++++++++++++++++++
>  1 file changed, 75 insertions(+)
>
> diff --git a/drivers/dma-buf/heaps/secure_heap.c b/drivers/dma-buf/heaps/secure_heap.c
> index 87ac23072e9e..2a037fc54004 100644
> --- a/drivers/dma-buf/heaps/secure_heap.c
> +++ b/drivers/dma-buf/heaps/secure_heap.c
> @@ -10,6 +10,12 @@
>  #include <linux/err.h>
>  #include <linux/module.h>
>  #include <linux/slab.h>
> +#include <linux/tee_drv.h>
> +#include <linux/uuid.h>
> +
> +#define TZ_TA_MEM_UUID_MTK             "4477588a-8476-11e2-ad15-e41f1390d676"
> +
> +#define TEE_PARAM_NUM                  4
>
>  enum secure_memory_type {
>         /*
> @@ -27,6 +33,9 @@ struct secure_buffer {
>  struct secure_heap;
>
>  struct secure_heap_prv_data {
> +       const char                      *uuid;
> +       const int                       tee_impl_id;
> +
>         int     (*memory_alloc)(struct secure_heap *sec_heap, struct secure_buffer *sec_buf);
>         void    (*memory_free)(struct secure_heap *sec_heap, struct secure_buffer *sec_buf);
>
> @@ -39,9 +48,62 @@ struct secure_heap {
>         const char                      *name;
>         const enum secure_memory_type   mem_type;
>
> +       struct tee_context              *tee_ctx;
> +       u32                             tee_session;
> +
>         const struct secure_heap_prv_data *data;
>  };
>
> +static int tee_ctx_match(struct tee_ioctl_version_data *ver, const void *data)
> +{
> +       const struct secure_heap_prv_data *d = data;
> +
> +       return ver->impl_id == d->tee_impl_id;
> +}
> +
> +static int secure_heap_tee_session_init(struct secure_heap *sec_heap)
> +{
> +       struct tee_param t_param[TEE_PARAM_NUM] = {0};
> +       struct tee_ioctl_open_session_arg arg = {0};
> +       const struct secure_heap_prv_data *data = sec_heap->data;
> +       uuid_t ta_mem_uuid;
> +       int ret;
> +
> +       sec_heap->tee_ctx = tee_client_open_context(NULL, tee_ctx_match, data, NULL);
> +       if (IS_ERR(sec_heap->tee_ctx)) {
> +               pr_err_once("%s: open context failed, ret=%ld\n", sec_heap->name,
> +                           PTR_ERR(sec_heap->tee_ctx));
> +               return -ENODEV;
> +       }
> +
> +       arg.num_params = TEE_PARAM_NUM;
> +       arg.clnt_login = TEE_IOCTL_LOGIN_PUBLIC;
> +       ret = uuid_parse(data->uuid, &ta_mem_uuid);
> +       if (ret)
> +               goto close_context;
> +       memcpy(&arg.uuid, &ta_mem_uuid.b, sizeof(ta_mem_uuid));
> +
> +       ret = tee_client_open_session(sec_heap->tee_ctx, &arg, t_param);
> +       if (ret < 0 || arg.ret) {
> +               pr_err_once("%s: open session failed, ret=%d:%d\n",
> +                           sec_heap->name, ret, arg.ret);
> +               ret = -EINVAL;
> +               goto close_context;
> +       }
> +       sec_heap->tee_session = arg.session;
> +       return 0;
> +
> +close_context:
> +       tee_client_close_context(sec_heap->tee_ctx);
> +       return ret;
> +}
> +
> +/* The memory allocating is within the TEE. */
> +const struct secure_heap_prv_data mtk_sec_mem_data = {
> +       .uuid                   = TZ_TA_MEM_UUID_MTK,
> +       .tee_impl_id            = TEE_IMPL_ID_OPTEE,
> +};
> +
>  static int secure_heap_secure_memory_allocate(struct secure_heap *sec_heap,
>                                               struct secure_buffer *sec_buf)
>  {
> @@ -84,11 +146,23 @@ secure_heap_allocate(struct dma_heap *heap, unsigned long size,
>                      unsigned long fd_flags, unsigned long heap_flags)
>  {
>         struct secure_heap *sec_heap = dma_heap_get_drvdata(heap);
> +       const struct secure_heap_prv_data *data = sec_heap->data;
>         struct secure_buffer *sec_buf;
>         DEFINE_DMA_BUF_EXPORT_INFO(exp_info);
>         struct dma_buf *dmabuf;
>         int ret;
>
> +       /*
> +        * If uuid is valid, It requires enter TEE to protect buffers. However
> +        * TEE probe may be late. Initialize the secure session the first time
> +        * we request the secure buffer.
> +        */
> +       if (data->uuid && !sec_heap->tee_session) {
> +               ret = secure_heap_tee_session_init(sec_heap);
> +               if (ret)
> +                       return ERR_PTR(ret);
> +       }
> +
>         sec_buf = kzalloc(sizeof(*sec_buf), GFP_KERNEL);
>         if (!sec_buf)
>                 return ERR_PTR(-ENOMEM);
> @@ -127,6 +201,7 @@ static struct secure_heap secure_heaps[] = {
>         {
>                 .name           = "secure_mtk_cm",
>                 .mem_type       = SECURE_MEMORY_TYPE_MTK_CM_TZ,
> +               .data           = &mtk_sec_mem_data,
>         },
>  };
>
> --
> 2.25.1
>

Re: [PATCH v2 4/8] dma-buf: heaps: secure_heap: Add tee memory service call

[Jeffrey Kardatzke]

On Sat, Nov 11, 2023 at 3:17 AM Yong Wu <yong.wu@mediatek.com> wrote:
>
> Add TEE service call. In the case of MediaTek, secure memory management is
> located within the TEE. The kernel just needs to tell TEE what size it
> needs and the TEE will return a "security handle" to kernel.
>
> To be consistent with the cma heap later, we put the tee ops into the ops
> of secure_the_memory.
>
> It seems that secure_heap_tee_service_call could be a more general
> interface, but it could be a new topic.
>
> Signed-off-by: Yong Wu <yong.wu@mediatek.com>
> ---
>  drivers/dma-buf/heaps/secure_heap.c | 97 +++++++++++++++++++++++++++++
>  1 file changed, 97 insertions(+)
>
> diff --git a/drivers/dma-buf/heaps/secure_heap.c b/drivers/dma-buf/heaps/secure_heap.c
> index 2a037fc54004..05062c14e7c7 100644
> --- a/drivers/dma-buf/heaps/secure_heap.c
> +++ b/drivers/dma-buf/heaps/secure_heap.c
> @@ -17,6 +17,27 @@
>
>  #define TEE_PARAM_NUM                  4
>
> +enum secure_buffer_tee_cmd { /* PARAM NUM always is 4. */
> +       /*
> +        * TZCMD_SECMEM_ZALLOC: Allocate the zeroed secure memory from TEE.
> +        *
> +        * [in]  value[0].a: The buffer size.
> +        *       value[0].b: alignment.
> +        * [in]  value[1].a: enum secure_memory_type.
> +        * [out] value[3].a: The secure handle.
> +        */
> +       TZCMD_SECMEM_ZALLOC = 0,
> +
> +       /*
> +        * TZCMD_SECMEM_FREE: Free secure memory.
> +        *
> +        * [in]  value[0].a: The secure handle of this buffer, It's value[3].a of
> +        *                   TZCMD_SECMEM_ZALLOC.
> +        * [out] value[1].a: return value, 0 means successful, otherwise fail.
> +        */
> +       TZCMD_SECMEM_FREE = 1,
> +};
> +

This should go in the MTK specific implementation.

>  enum secure_memory_type {
>         /*
>          * MediaTek static chunk memory carved out for TrustZone. The memory
> @@ -28,13 +49,25 @@ enum secure_memory_type {
>  struct secure_buffer {
>         struct dma_heap                 *heap;
>         size_t                          size;
> +       /*
> +        * The secure handle is a reference to a buffer within the TEE, this is
> +        * a value got from TEE.
> +        */
> +       u32                             sec_handle;
>  };

Change this to a u64 and rename it to 'secure_address', it's up to the
specific implementation what that would actually mean.

>
> +#define TEE_MEM_COMMAND_ID_BASE_MTK    0x10000
> +
Move this into the MTK specific implementation.

>  struct secure_heap;
>
>  struct secure_heap_prv_data {
>         const char                      *uuid;
>         const int                       tee_impl_id;
> +       /*
> +        * Different TEEs may implement different commands, and this provides an opportunity
> +        * for TEEs to use the same enum secure_buffer_tee_cmd.
> +        */
> +       const int                       tee_command_id_base;
Remove this, it can be handled in the MTK specific implementation.
>
>         int     (*memory_alloc)(struct secure_heap *sec_heap, struct secure_buffer *sec_buf);
>         void    (*memory_free)(struct secure_heap *sec_heap, struct secure_buffer *sec_buf);
> @@ -98,10 +131,74 @@ static int secure_heap_tee_session_init(struct secure_heap *sec_heap)
>         return ret;
>  }
>
> +static int
> +secure_heap_tee_service_call(struct tee_context *tee_ctx, u32 session,
> +                            unsigned int command, struct tee_param *params)
> +{
> +       struct tee_ioctl_invoke_arg arg = {0};
> +       int ret;
> +
> +       arg.num_params = TEE_PARAM_NUM;
> +       arg.session = session;
> +       arg.func = command;
> +
> +       ret = tee_client_invoke_func(tee_ctx, &arg, params);
> +       if (ret < 0 || arg.ret) {
> +               pr_err("%s: cmd %d ret %d:%x.\n", __func__, command, ret, arg.ret);
> +               ret = -EOPNOTSUPP;
> +       }
> +       return ret;
> +}
> +
> +static int secure_heap_tee_secure_memory(struct secure_heap *sec_heap,
> +                                        struct secure_buffer *sec_buf)
> +{
> +       const struct secure_heap_prv_data *data = sec_heap->data;
> +       struct tee_param params[TEE_PARAM_NUM] = {0};
> +       int ret;
> +
> +       params[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT;
> +       params[0].u.value.a = sec_buf->size;
> +       params[0].u.value.b = PAGE_SIZE;
> +       params[1].attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT;
> +       params[1].u.value.a = sec_heap->mem_type;
> +       params[2].attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT;
> +
> +       params[3].attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_OUTPUT;
> +       ret = secure_heap_tee_service_call(sec_heap->tee_ctx, sec_heap->tee_session,
> +                                          data->tee_command_id_base + TZCMD_SECMEM_ZALLOC,
> +                                          params);
> +       if (ret)
> +               return -ENOMEM;
> +
> +       sec_buf->sec_handle = params[3].u.value.a;
> +       return 0;
> +}
> +
> +static void secure_heap_tee_unsecure_memory(struct secure_heap *sec_heap,
> +                                           struct secure_buffer *sec_buf)
> +{
> +       struct tee_param params[TEE_PARAM_NUM] = {0};
> +
> +       params[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT;
> +       params[0].u.value.a = sec_buf->sec_handle;
> +       params[1].attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_OUTPUT;
> +
> +       secure_heap_tee_service_call(sec_heap->tee_ctx, sec_heap->tee_session,
> +                                    sec_heap->data->tee_command_id_base + TZCMD_SECMEM_FREE,
> +                                    params);
> +       if (params[1].u.value.a)
> +               pr_err("%s, free buffer(0x%x) return fail(%lld) from TEE.\n",
> +                      sec_heap->name, sec_buf->sec_handle, params[1].u.value.a);
> +}
> +

These are entirely MTK specific, so move them into the MTK specific
implementation.

>  /* The memory allocating is within the TEE. */
>  const struct secure_heap_prv_data mtk_sec_mem_data = {
>         .uuid                   = TZ_TA_MEM_UUID_MTK,
>         .tee_impl_id            = TEE_IMPL_ID_OPTEE,
> +       .tee_command_id_base    = TEE_MEM_COMMAND_ID_BASE_MTK,
> +       .secure_the_memory      = secure_heap_tee_secure_memory,
> +       .unsecure_the_memory    = secure_heap_tee_unsecure_memory,
>  };

This should also go into the MTK specific implementation, and to be
clear, that's where module_init should be as well.

>
>  static int secure_heap_secure_memory_allocate(struct secure_heap *sec_heap,
> --
> 2.25.1
>

Re: [PATCH v2 6/8] dt-bindings: reserved-memory: Add secure CMA reserved memory range

[Jeffrey Kardatzke]

May I suggest the following for the device tree binding? (I'm not very
familiar w/ device trees, so apologies for any oversights, but trying
to process the feedback here and help move Mediatek along). This
should align with my other suggestions for having an MTK specific
portion to their secure heap implementation; which also means there
should be an MTK specific device tree binding.

# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
%YAML 1.2
---
$id: http://devicetree.org/schemas/reserved-memory/mediatek,dynamic-secure-region.yaml#
$schema: http://devicetree.org/meta-schemas/core.yaml#

title: Mediatek Dynamic Reserved Region

description:
  A memory region that can dynamically transition as a whole between
secure and non-secure states. This memory will be protected by OP-TEE
when allocations are active and unprotected otherwise.

maintainers:
  - Yong Wu <yong.wu@mediatek.com>

allOf:
  - $ref: reserved-memory.yaml

properties:
  compatible:
    const: mediatek,dynamic-secure-region

required:
  - compatible
  - reg
  - reusable

unevaluatedProperties: false

examples:
  - |

    reserved-memory {
        #address-cells = <1>;
        #size-cells = <1>;
        ranges;

        reserved-memory@80000000 {
            compatible = "mediatek,dynamic-secure-region";
            reusable;
            reg = <0x80000000 0x18000000>;
        };
    };

On Tue, Nov 14, 2023 at 5:18 AM Robin Murphy <robin.murphy@arm.com> wrote:
>
> On 13/11/2023 6:37 am, Yong Wu (吴勇) wrote:
> [...]
> >>> +properties:
> >>> +  compatible:
> >>> +    const: secure_cma_region
> >>
> >> Still wrong compatible. Look at other bindings - there is nowhere
> >> underscore. Look at other reserved memory bindings especially.
> >>
> >> Also, CMA is a Linux thingy, so either not suitable for bindings at
> >> all,
> >> or you need Linux specific compatible. I don't quite get why do you
> >> evennot
> >> put CMA there - adding Linux specific stuff will get obvious
> >> pushback...
> >
> > Thanks. I will change to: secure-region. Is this ok?
>
> No, the previous discussion went off in entirely the wrong direction. To
> reiterate, the point of the binding is not to describe the expected
> usage of the thing nor the general concept of the thing, but to describe
> the actual thing itself. There are any number of different ways software
> may interact with a "secure region", so that is meaningless as a
> compatible. It needs to describe *this* secure memory interface offered
> by *this* TEE, so that software knows that to use it requires making
> those particular SiP calls with that particular UUID etc.
>
> Thanks,
> Robin.

Re: [PATCH v2 7/8] dma_buf: heaps: secure_heap: Add a new MediaTek CMA heap

[Jeffrey Kardatzke]

Most of the things in this patch should go in the MTK specific
implementation (except for the secure_heap_init changes). Especially
the RESERVEDMEM_OF_DECLARE.

On Sat, Nov 11, 2023 at 3:18 AM Yong Wu <yong.wu@mediatek.com> wrote:
>
> Create a new MediaTek CMA heap from the CMA reserved buffer.
>
> In this heap, When the first allocating buffer, use cma_alloc to prepare
> whole the CMA range, then send its range to TEE to protect and manage.
> For the later allocating, we just adds the cma_used_size_mtk.
>
> This CMA flow may be different with the normal CMA heap of next patch.
> So I named the variable with _mtk suffix like cma_page_mtk/
> cma_used_size_mtk. This is also to distinguish it from the cma_page of
> the buffer structure in the next patch.
>
> When SVP done, cma_release will release the buffer, then kernel may
> reuse it.
>
> Meanwhile, this patch adds a "heap_init" pointer, while allows some heap
> initialization operations. This case also checks if the CMA range is
> ready.
>
> Signed-off-by: Yong Wu <yong.wu@mediatek.com>
> ---
>  drivers/dma-buf/heaps/secure_heap.c | 124 +++++++++++++++++++++++++++-
>  1 file changed, 122 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/dma-buf/heaps/secure_heap.c b/drivers/dma-buf/heaps/secure_heap.c
> index 25cc95442c56..f8b84fd16288 100644
> --- a/drivers/dma-buf/heaps/secure_heap.c
> +++ b/drivers/dma-buf/heaps/secure_heap.c
> @@ -4,11 +4,12 @@
>   *
>   * Copyright (C) 2023 MediaTek Inc.
>   */
> -
> +#include <linux/cma.h>
>  #include <linux/dma-buf.h>
>  #include <linux/dma-heap.h>
>  #include <linux/err.h>
>  #include <linux/module.h>
> +#include <linux/of_reserved_mem.h>
>  #include <linux/scatterlist.h>
>  #include <linux/slab.h>
>  #include <linux/tee_drv.h>
> @@ -25,6 +26,8 @@ enum secure_buffer_tee_cmd { /* PARAM NUM always is 4. */
>          * [in]  value[0].a: The buffer size.
>          *       value[0].b: alignment.
>          * [in]  value[1].a: enum secure_memory_type.
> +        * [in]  value[2].a: pa base in cma case.
> +        *       value[2].b: The buffer size in cma case.
>          * [out] value[3].a: The secure handle.
>          */
>         TZCMD_SECMEM_ZALLOC = 0,
> @@ -45,6 +48,13 @@ enum secure_memory_type {
>          * management is inside the TEE.
>          */
>         SECURE_MEMORY_TYPE_MTK_CM_TZ    = 1,
> +       /*
> +        * MediaTek dynamic chunk memory carved out from CMA.
> +        * In normal case, the CMA could be used in kernel; When SVP start, we will
> +        * allocate whole this CMA and pass whole the CMA PA and size into TEE to
> +        * protect it, then the detail memory management also is inside the TEE.
> +        */
> +       SECURE_MEMORY_TYPE_MTK_CM_CMA   = 2,
>  };
>
>  struct secure_buffer {
> @@ -70,6 +80,7 @@ struct secure_heap_prv_data {
>          */
>         const int                       tee_command_id_base;
>
> +       int     (*heap_init)(struct secure_heap *sec_heap);
>         int     (*memory_alloc)(struct secure_heap *sec_heap, struct secure_buffer *sec_buf);
>         void    (*memory_free)(struct secure_heap *sec_heap, struct secure_buffer *sec_buf);
>
> @@ -86,6 +97,13 @@ struct secure_heap {
>         u32                             tee_session;
>
>         const struct secure_heap_prv_data *data;
> +
> +       struct cma              *cma;
> +       struct page             *cma_page_mtk;
> +       unsigned long           cma_paddr;
> +       unsigned long           cma_size;
> +       unsigned long           cma_used_size_mtk;
> +       struct mutex            lock; /* lock for cma_used_size_mtk */
>  };
>
>  struct secure_heap_attachment {
> @@ -168,7 +186,10 @@ static int secure_heap_tee_secure_memory(struct secure_heap *sec_heap,
>         params[1].attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT;
>         params[1].u.value.a = sec_heap->mem_type;
>         params[2].attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT;
> -
> +       if (sec_heap->cma && sec_heap->mem_type == SECURE_MEMORY_TYPE_MTK_CM_CMA) {
> +               params[2].u.value.a = sec_heap->cma_paddr;
> +               params[2].u.value.b = sec_heap->cma_size;
> +       }
>         params[3].attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_OUTPUT;
>         ret = secure_heap_tee_service_call(sec_heap->tee_ctx, sec_heap->tee_session,
>                                            data->tee_command_id_base + TZCMD_SECMEM_ZALLOC,
> @@ -197,6 +218,66 @@ static void secure_heap_tee_unsecure_memory(struct secure_heap *sec_heap,
>                        sec_heap->name, sec_buf->sec_handle, params[1].u.value.a);
>  }
>
> +static int mtk_secure_memory_cma_allocate(struct secure_heap *sec_heap,
> +                                         struct secure_buffer *sec_buf)
> +{
> +       /*
> +        * Allocate CMA only when allocating buffer for the first time, and just
> +        * increase cma_used_size_mtk at the other time.
> +        */
> +       mutex_lock(&sec_heap->lock);
> +       if (sec_heap->cma_used_size_mtk)
> +               goto add_size;
> +
> +       mutex_unlock(&sec_heap->lock);
> +       sec_heap->cma_page_mtk = cma_alloc(sec_heap->cma, sec_heap->cma_size >> PAGE_SHIFT,
> +                                          get_order(PAGE_SIZE), false);
> +       if (!sec_heap->cma_page_mtk)
> +               return -ENOMEM;
> +
> +       mutex_lock(&sec_heap->lock);
> +add_size:
> +       sec_heap->cma_used_size_mtk += sec_buf->size;
> +       mutex_unlock(&sec_heap->lock);
> +
> +       return 0;
> +}
> +
> +static void mtk_secure_memory_cma_free(struct secure_heap *sec_heap,
> +                                      struct secure_buffer *sec_buf)
> +{
> +       bool cma_is_empty;
> +
> +       mutex_lock(&sec_heap->lock);
> +       sec_heap->cma_used_size_mtk -= sec_buf->size;
> +       cma_is_empty = !sec_heap->cma_used_size_mtk;
> +       mutex_unlock(&sec_heap->lock);
> +
> +       if (cma_is_empty)
> +               cma_release(sec_heap->cma, sec_heap->cma_page_mtk,
> +                           sec_heap->cma_size >> PAGE_SHIFT);
> +}
> +
> +static int mtk_secure_heap_cma_init(struct secure_heap *sec_heap)
> +{
> +       if (!sec_heap->cma)
> +               return -EINVAL;
> +       mutex_init(&sec_heap->lock);
> +       return 0;
> +}
> +
> +/* Use CMA to prepare the buffer and the memory allocating is within the TEE. */
> +const struct secure_heap_prv_data mtk_sec_mem_data_cma = {
> +       .uuid                   = TZ_TA_MEM_UUID_MTK,
> +       .tee_impl_id            = TEE_IMPL_ID_OPTEE,
> +       .tee_command_id_base    = TEE_MEM_COMMAND_ID_BASE_MTK,
> +       .heap_init              = mtk_secure_heap_cma_init,
> +       .memory_alloc           = mtk_secure_memory_cma_allocate,
> +       .memory_free            = mtk_secure_memory_cma_free,
> +       .secure_the_memory      = secure_heap_tee_secure_memory,
> +       .unsecure_the_memory    = secure_heap_tee_unsecure_memory,
> +};
> +
>  /* The memory allocating is within the TEE. */
>  const struct secure_heap_prv_data mtk_sec_mem_data = {
>         .uuid                   = TZ_TA_MEM_UUID_MTK,
> @@ -420,20 +501,59 @@ static struct secure_heap secure_heaps[] = {
>                 .mem_type       = SECURE_MEMORY_TYPE_MTK_CM_TZ,
>                 .data           = &mtk_sec_mem_data,
>         },
> +       {
> +               .name           = "secure_mtk_cma",
> +               .mem_type       = SECURE_MEMORY_TYPE_MTK_CM_CMA,
> +               .data           = &mtk_sec_mem_data_cma,
> +       },
>  };
>
> +static int __init secure_cma_init(struct reserved_mem *rmem)
> +{
> +       struct secure_heap *sec_heap = secure_heaps;
> +       struct cma *sec_cma;
> +       int ret, i;
> +
> +       ret = cma_init_reserved_mem(rmem->base, rmem->size, 0, rmem->name,
> +                                   &sec_cma);
> +       if (ret) {
> +               pr_err("%s: %s set up CMA fail\n", __func__, rmem->name);
> +               return ret;
> +       }
> +
> +       for (i = 0; i < ARRAY_SIZE(secure_heaps); i++, sec_heap++) {
> +               if (sec_heap->mem_type != SECURE_MEMORY_TYPE_MTK_CM_CMA)
> +                       continue;
> +
> +               sec_heap->cma = sec_cma;
> +               sec_heap->cma_paddr = rmem->base;
> +               sec_heap->cma_size = rmem->size;
> +       }
> +       return 0;
> +}
> +
> +RESERVEDMEM_OF_DECLARE(secure_cma, "secure_cma_region", secure_cma_init);
> +
>  static int secure_heap_init(void)
>  {
>         struct secure_heap *sec_heap = secure_heaps;
>         struct dma_heap_export_info exp_info;
>         struct dma_heap *heap;
>         unsigned int i;
> +       int ret;
>
>         for (i = 0; i < ARRAY_SIZE(secure_heaps); i++, sec_heap++) {
>                 exp_info.name = sec_heap->name;
>                 exp_info.ops = &sec_heap_ops;
>                 exp_info.priv = (void *)sec_heap;
>
> +               if (sec_heap->data && sec_heap->data->heap_init) {
> +                       ret = sec_heap->data->heap_init(sec_heap);
> +                       if (ret) {
> +                               pr_err("sec_heap %s init fail %d.\n", sec_heap->name, ret);
> +                               continue;
> +                       }
> +               }
>                 heap = dma_heap_add(&exp_info);
>                 if (IS_ERR(heap))
>                         return PTR_ERR(heap);
> --
> 2.25.1
>

Re: [PATCH v2 8/8] dma-buf: heaps: secure_heap: Add normal CMA heap

[Jeffrey Kardatzke]

You should drop this patch completely.

On Sat, Nov 11, 2023 at 3:18 AM Yong Wu <yong.wu@mediatek.com> wrote:
>
> Add a normal CMA heap which use the standard cma allocate.
>
> Signed-off-by: Yong Wu <yong.wu@mediatek.com>
> ---
> Hi Vijay and Jaskaran,
>
> For this heap,
> 1) It uses sec_heap_buf_ops currently. I guess we cann't use the
> cma_heap_buf_ops. since if it is secure buffer, some operations such
> as mmap should not be allowed.
> 2) I didn't add how to protect/secure the buffer.
>
> Please feel free to change to meet your requirements.
> Thanks.
> ---
>  drivers/dma-buf/heaps/secure_heap.c | 38 ++++++++++++++++++++++++++++-
>  1 file changed, 37 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/dma-buf/heaps/secure_heap.c b/drivers/dma-buf/heaps/secure_heap.c
> index f8b84fd16288..8989ad5d03e9 100644
> --- a/drivers/dma-buf/heaps/secure_heap.c
> +++ b/drivers/dma-buf/heaps/secure_heap.c
> @@ -43,6 +43,8 @@ enum secure_buffer_tee_cmd { /* PARAM NUM always is 4. */
>  };
>
>  enum secure_memory_type {
> +       /* CMA for the secure memory, Use the normal cma ops to alloc/free. */
> +       SECURE_MEMORY_TYPE_CMA          = 0,
>         /*
>          * MediaTek static chunk memory carved out for TrustZone. The memory
>          * management is inside the TEE.
> @@ -65,6 +67,7 @@ struct secure_buffer {
>          * a value got from TEE.
>          */
>         u32                             sec_handle;
> +       struct page                     *cma_page;
>  };
>
>  #define TEE_MEM_COMMAND_ID_BASE_MTK    0x10000
> @@ -287,6 +290,33 @@ const struct secure_heap_prv_data mtk_sec_mem_data = {
>         .unsecure_the_memory    = secure_heap_tee_unsecure_memory,
>  };
>
> +static int cma_secure_memory_allocate(struct secure_heap *sec_heap,
> +                                     struct secure_buffer *sec_buf)
> +{
> +       if (!sec_heap->cma)
> +               return -EINVAL;
> +
> +       sec_buf->cma_page = cma_alloc(sec_heap->cma, sec_buf->size >> PAGE_SHIFT,
> +                                     get_order(PAGE_SIZE), false);
> +       if (!sec_buf->cma_page)
> +               return -ENOMEM;
> +
> +       memset(page_address(sec_buf->cma_page), 0, sec_buf->size);
> +       return 0;
> +}
> +
> +static void cma_secure_memory_free(struct secure_heap *sec_heap,
> +                                  struct secure_buffer *sec_buf)
> +{
> +       cma_release(sec_heap->cma, sec_buf->cma_page, sec_buf->size >> PAGE_SHIFT);
> +}
> +
> +const struct secure_heap_prv_data cma_sec_mem_data = {
> +       .memory_alloc   = cma_secure_memory_allocate,
> +       .memory_free    = cma_secure_memory_free,
> +       /* TODO : secure the buffer. */
> +};
> +
>  static int secure_heap_secure_memory_allocate(struct secure_heap *sec_heap,
>                                               struct secure_buffer *sec_buf)
>  {
> @@ -496,6 +526,11 @@ static const struct dma_heap_ops sec_heap_ops = {
>  };
>
>  static struct secure_heap secure_heaps[] = {
> +       {
> +               .name           = "secure_cma",
> +               .mem_type       = SECURE_MEMORY_TYPE_CMA,
> +               .data           = &cma_sec_mem_data,
> +       },
>         {
>                 .name           = "secure_mtk_cm",
>                 .mem_type       = SECURE_MEMORY_TYPE_MTK_CM_TZ,
> @@ -522,7 +557,8 @@ static int __init secure_cma_init(struct reserved_mem *rmem)
>         }
>
>         for (i = 0; i < ARRAY_SIZE(secure_heaps); i++, sec_heap++) {
> -               if (sec_heap->mem_type != SECURE_MEMORY_TYPE_MTK_CM_CMA)
> +               if (sec_heap->mem_type != SECURE_MEMORY_TYPE_MTK_CM_CMA &&
> +                   sec_heap->mem_type != SECURE_MEMORY_TYPE_CMA)
>                         continue;
>
>                 sec_heap->cma = sec_cma;
> --
> 2.25.1
>

Re: [PATCH v2 0/8] dma-buf: heaps: Add secure heap

[Pratyush Brahma]

Hi

We have sent a patch series at [1] using this series to add support for 
Qualcomm secure heaps.
Instead of TEE calls, it uses qcom_scm_assign_mem() to secure the memory.

Thanks,
Pratyush

[1] 
https://lore.kernel.org/lkml/cover.1700544802.git.quic_vjitta@quicinc.com/