Hi Joakim,
Thanks very much for the reviewing.
On Wed, 2023-09-27 at 16:37 +0200, Joakim Bech wrote:
External email : Please do not click links or open attachments until you have verified the sender or the content. On Mon, Sep 11, 2023 at 10:30:35AM +0800, Yong Wu wrote:
Add TEE service call for secure memory allocating/freeing.
Signed-off-by: Anan Sun anan.sun@mediatek.com Signed-off-by: Yong Wu yong.wu@mediatek.com
drivers/dma-buf/heaps/mtk_secure_heap.c | 69
++++++++++++++++++++++++-
1 file changed, 68 insertions(+), 1 deletion(-)
diff --git a/drivers/dma-buf/heaps/mtk_secure_heap.c b/drivers/dma-
buf/heaps/mtk_secure_heap.c
index e3da33a3d083..14c2a16a7164 100644 --- a/drivers/dma-buf/heaps/mtk_secure_heap.c +++ b/drivers/dma-buf/heaps/mtk_secure_heap.c @@ -17,6 +17,9 @@ #define MTK_TEE_PARAM_NUM4 +#define TZCMD_MEM_SECURECM_UNREF7 +#define TZCMD_MEM_SECURECM_ZALLOC15
This is related to the discussion around UUID as well. These numbers here are specific to the MediaTek TA. If we could make things more generic, then these should probably be 0 and 1.
I also find the naming a bit heavy, I think I'd suggest something like: # define TEE_CMD_SECURE_HEAP_ZALLOC ... and so on.
I will check internally and try to follow this. If we can not follow, I'll give feedback here.
/*
- MediaTek secure (chunk) memory type
@@ -29,6 +32,8 @@ enum kree_mem_type {
The "kree" here, is that meant to indicate kernel REE? If yes, then I guess that could be dropped since we know we're already in the kernel context, perhaps instead name it something with "secure_heap_type"?
struct mtk_secure_heap_buffer { struct dma_heap*heap; size_tsize;
+u32sec_handle; }; struct mtk_secure_heap { @@ -80,6 +85,63 @@ static int mtk_kree_secure_session_init(struct
mtk_secure_heap *sec_heap)
return ret; } +static int +mtk_sec_mem_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 = MTK_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; +}
Perhaps not relevant for this patch set, but since this function is just a pure TEE call, I'm inclined to suggest that this could even be moved out as a generic TEE function. I.e., something that could be used elsewhere in the Linux kernel.
Good Suggestion. I've seen many places call this, and they are basically similar. Do you mean we create a simple wrap for this? something like this: int tee_client_invoke_func_wrap(struct tee_context *ctx, u32 session, u32 func_id, unsigned int num_params, struct tee_param *param, int *invoke_arg_ret/* OUT */)
If this makes sense, it should be done in a separate patchset.
+static int mtk_sec_mem_allocate(struct mtk_secure_heap *sec_heap, +struct mtk_secure_heap_buffer *sec_buf) +{ +struct tee_param params[MTK_TEE_PARAM_NUM] = {0}; +u32 mem_session = sec_heap->mem_session;
How about name it tee_session? Alternative ta_session? I think that would better explain what it actually is.
Thanks for the renaming. Will change.
+int ret;
+params[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT; +params[0].u.value.a = SZ_4K;/* alignment */ +params[0].u.value.b = sec_heap->mem_type;/* memory type */ +params[1].attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT; +params[1].u.value.a = sec_buf->size; +params[2].attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INOUT;
+/* Always request zeroed buffer */ +ret = mtk_sec_mem_tee_service_call(sec_heap->tee_ctx, mem_session,
- TZCMD_MEM_SECURECM_ZALLOC, params);
+if (ret) +return -ENOMEM;
+sec_buf->sec_handle = params[2].u.value.a; +return 0; +}
+static void mtk_sec_mem_release(struct mtk_secure_heap *sec_heap, +struct mtk_secure_heap_buffer *sec_buf) +{ +struct tee_param params[MTK_TEE_PARAM_NUM] = {0}; +u32 mem_session = sec_heap->mem_session;
+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;
Perhaps worth having a comment for params[1] explain why we need the VALUE_OUTPUT here?
Will do.
-- // Regards Joakim
+mtk_sec_mem_tee_service_call(sec_heap->tee_ctx, mem_session,
TZCMD_MEM_SECURECM_UNREF, params);+}
static struct dma_buf * mtk_sec_heap_allocate(struct dma_heap *heap, size_t size, unsigned long fd_flags, unsigned long heap_flags) @@ -107,6 +169,9 @@ mtk_sec_heap_allocate(struct dma_heap *heap,
size_t size,
sec_buf->size = size; sec_buf->heap = heap; +ret = mtk_sec_mem_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; @@ -115,11 +180,13 @@ mtk_sec_heap_allocate(struct dma_heap *heap,
size_t 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: +mtk_sec_mem_release(sec_heap, sec_buf); err_free_buf: kfree(sec_buf); return ERR_PTR(ret); -- 2.25.1