diff mbox series

[6/9] dma-buf: heaps: mtk_sec_heap: Add tee service call for buffer allocating/freeing

Message ID 20230911023038.30649-7-yong.wu@mediatek.com (mailing list archive)
State New, archived
Headers show
Series dma-buf: heaps: Add MediaTek secure heap | expand

Commit Message

Yong Wu (吴勇) Sept. 11, 2023, 2:30 a.m. UTC 
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(-)

Comments

kernel test robot Sept. 14, 2023, 10:18 a.m. UTC | #1 
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 linus/master v6.6-rc1 next-20230914]
[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-Deduplicate-docs-and-adopt-common-format/20230911-103308
base:   git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
patch link:    https://lore.kernel.org/r/20230911023038.30649-7-yong.wu%40mediatek.com
patch subject: [PATCH 6/9] dma-buf: heaps: mtk_sec_heap: Add tee service call for buffer allocating/freeing
config: loongarch-allmodconfig (https://download.01.org/0day-ci/archive/20230914/202309141707.zdT0yuMT-lkp@intel.com/config)
compiler: loongarch64-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230914/202309141707.zdT0yuMT-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/202309141707.zdT0yuMT-lkp@intel.com/

All errors (new ones prefixed by >>):

   loongarch64-linux-ld: drivers/dma-buf/heaps/mtk_secure_heap.o: in function `mtk_kree_secure_session_init':
   mtk_secure_heap.c:(.text+0x130): undefined reference to `tee_client_open_context'
   loongarch64-linux-ld: drivers/dma-buf/heaps/mtk_secure_heap.o: in function `.L10':
   mtk_secure_heap.c:(.text+0x19c): undefined reference to `tee_client_open_session'
   loongarch64-linux-ld: drivers/dma-buf/heaps/mtk_secure_heap.o: in function `.L12':
   mtk_secure_heap.c:(.text+0x1dc): undefined reference to `tee_client_close_context'
   loongarch64-linux-ld: drivers/dma-buf/heaps/mtk_secure_heap.o: in function `mtk_sec_mem_tee_service_call.constprop.0':
>> mtk_secure_heap.c:(.text+0x274): undefined reference to `tee_client_invoke_func'
   loongarch64-linux-ld: drivers/dma-buf/heaps/mtk_secure_heap.o: in function `mtk_sec_mem_release.isra.0':
   mtk_secure_heap.c:(.text+0x464): undefined reference to `tee_client_invoke_func'
Joakim Bech Sept. 27, 2023, 2:37 p.m. UTC | #2 
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_NUM		4
>  
> +#define TZCMD_MEM_SECURECM_UNREF	7
> +#define TZCMD_MEM_SECURECM_ZALLOC	15
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.

> +
>  /*
>   * 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_t			size;
> +
> +	u32			sec_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.

> +
> +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.

> +	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?
Yong Wu (吴勇) Sept. 28, 2023, 5:24 a.m. UTC | #3 
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
> >
Vijayanand Jitta Oct. 19, 2023, 4:45 a.m. UTC | #4 
On 9/11/2023 8:00 AM, 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_NUM		4
>  
> +#define TZCMD_MEM_SECURECM_UNREF	7
> +#define TZCMD_MEM_SECURECM_ZALLOC	15
> +
>  /*
>   * MediaTek secure (chunk) memory type
>   *
> @@ -29,6 +32,8 @@ enum kree_mem_type {
>  struct mtk_secure_heap_buffer {
>  	struct dma_heap		*heap;
>  	size_t			size;
> +
> +	u32			sec_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;
> +}
> +
> +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;
> +	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);

I see here optee calls are being used to secure memory.

For a secure heap, there can be multiple ways on how we want to secure memory,
for eg : by using qcom_scm_assign_mem.

This interface restricts securing memory to only optee calls.
can we have a way to choose ops that we want to secure memory ? 

Thanks,
Vijay

> +	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;
> +
> +	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);
Yong Wu (吴勇) Oct. 20, 2023, 10:01 a.m. UTC | #5 
Hi Vijayanand,

Thanks very much for your review.

On Thu, 2023-10-19 at 10:15 +0530, Vijayanand Jitta wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  
> 
> On 9/11/2023 8:00 AM, 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
> > +
> >  /*
> >   * MediaTek secure (chunk) memory type
> >   *
> > @@ -29,6 +32,8 @@ enum kree_mem_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;
> > +}
> > +
> > +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;
> > +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);
> 
> I see here optee calls are being used to secure memory.
> 
> For a secure heap, there can be multiple ways on how we want to
> secure memory,
> for eg : by using qcom_scm_assign_mem.
> 
> This interface restricts securing memory to only optee calls.
> can we have a way to choose ops that we want to secure memory ? 

Thanks for this suggestion. So it looks like there are four operations
in the abstract ops. Something like this?

struct sec_memory_ops {
   int (*sec_memory_init)()   //we need initialise tee session here.
   int (*sec_memory_alloc)()
   int (*sec_memory_free)()
   void (*sec_memory_uninit)()
}
   
Do you also need tee operation like tee_client_open_session and
tee_client_invoke_func?
if so, your UUID and TEE command ID value are also different, right?
   
We may also need new macros on how to choose different sec_memory_ops
since we don't have different bindings.

> 
> Thanks,
> Vijay
diff mbox series

Patch

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_NUM		4
 
+#define TZCMD_MEM_SECURECM_UNREF	7
+#define TZCMD_MEM_SECURECM_ZALLOC	15
+
 /*
  * MediaTek secure (chunk) memory type
  *
@@ -29,6 +32,8 @@  enum kree_mem_type {
 struct mtk_secure_heap_buffer {
 	struct dma_heap		*heap;
 	size_t			size;
+
+	u32			sec_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;
+}
+
+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;
+	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;
+
+	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);