[0/9] dma-buf: heaps: Add MediaTek secure heap

This patchset consists of two parts, the first is from John and TJ.
It adds some heap interfaces, then our kernel users could allocate buffer
from special heap. The second part is adding MTK secure heap for SVP
(Secure Video Path). A total of two heaps are added, one is mtk_svp and
the other is mtk_svp_cma. The mtk_svp buffer is reserved for the secure
world after bootup and it is used for ES/working buffer, while the
mtk_svp_cma buffer is dynamically reserved for the secure world and will
be get ready when we start playing secure videos, this heap is used for the
frame buffer. Once the security video playing is complete, the CMA will be
released.

For easier viewing, I've split the new heap file into several patches.

The consumers of new heap and new interfaces are our codec and drm which
will send upstream soon, probably this week.

Base on v6.6-rc1.

John Stultz (2):
  dma-heap: Add proper kref handling on dma-buf heaps
  dma-heap: Provide accessors so that in-kernel drivers can allocate
    dmabufs from specific heaps

T.J. Mercier (1):
  dma-buf: heaps: Deduplicate docs and adopt common format

Yong Wu (6):
  dma-buf: heaps: Initialise MediaTek secure heap
  dma-buf: heaps: mtk_sec_heap: Initialise tee session
  dma-buf: heaps: mtk_sec_heap: Add tee service call for buffer
    allocating/freeing
  dma-buf: heaps: mtk_sec_heap: Add dma_ops
  dt-bindings: reserved-memory: MediaTek: Add reserved memory for SVP
  dma_buf: heaps: mtk_sec_heap: Add a new CMA heap

 .../mediatek,secure_cma_chunkmem.yaml         |  42 ++
 drivers/dma-buf/dma-heap.c                    | 127 +++--
 drivers/dma-buf/heaps/Kconfig                 |   8 +
 drivers/dma-buf/heaps/Makefile                |   1 +
 drivers/dma-buf/heaps/mtk_secure_heap.c       | 458 ++++++++++++++++++
 include/linux/dma-heap.h                      |  42 +-
 6 files changed, 630 insertions(+), 48 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/reserved-memory/mediatek,secure_cma_chunkmem.yaml
 create mode 100644 drivers/dma-buf/heaps/mtk_secure_heap.c

Il 11/09/23 04:30, Yong Wu ha scritto:
> The TEE probe later than dma-buf heap, and PROBE_DEDER doesn't work
> here since this is not a platform driver, therefore initialise the TEE
> context/session while we allocate the first secure buffer.
> 
> Signed-off-by: Yong Wu <yong.wu@mediatek.com>
> ---
>   drivers/dma-buf/heaps/mtk_secure_heap.c | 61 +++++++++++++++++++++++++
>   1 file changed, 61 insertions(+)
> 
> diff --git a/drivers/dma-buf/heaps/mtk_secure_heap.c b/drivers/dma-buf/heaps/mtk_secure_heap.c
> index bbf1c8dce23e..e3da33a3d083 100644
> --- a/drivers/dma-buf/heaps/mtk_secure_heap.c
> +++ b/drivers/dma-buf/heaps/mtk_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		"4477588a-8476-11e2-ad15-e41f1390d676"
> +

Is this UUID the same for all SoCs and all TZ versions?

Thanks,
Angelo


> +#define MTK_TEE_PARAM_NUM		4
>   
>   /*
>    * MediaTek secure (chunk) memory type
> @@ -28,17 +34,72 @@ struct mtk_secure_heap_buffer {
>   struct mtk_secure_heap {
>   	const char		*name;
>   	const enum kree_mem_type mem_type;
> +	u32			 mem_session;
> +	struct tee_context	*tee_ctx;
>   };
>   
> +static int mtk_optee_ctx_match(struct tee_ioctl_version_data *ver, const void *data)
> +{
> +	return ver->impl_id == TEE_IMPL_ID_OPTEE;
> +}
> +
> +static int mtk_kree_secure_session_init(struct mtk_secure_heap *sec_heap)
> +{
> +	struct tee_param t_param[MTK_TEE_PARAM_NUM] = {0};
> +	struct tee_ioctl_open_session_arg arg = {0};
> +	uuid_t ta_mem_uuid;
> +	int ret;
> +
> +	sec_heap->tee_ctx = tee_client_open_context(NULL, mtk_optee_ctx_match,
> +						    NULL, NULL);
> +	if (IS_ERR(sec_heap->tee_ctx)) {
> +		pr_err("%s: open context failed, ret=%ld\n", sec_heap->name,
> +		       PTR_ERR(sec_heap->tee_ctx));
> +		return -ENODEV;
> +	}
> +
> +	arg.num_params = MTK_TEE_PARAM_NUM;
> +	arg.clnt_login = TEE_IOCTL_LOGIN_PUBLIC;
> +	ret = uuid_parse(TZ_TA_MEM_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("%s: open session failed, ret=%d:%d\n",
> +		       sec_heap->name, ret, arg.ret);
> +		ret = -EINVAL;
> +		goto close_context;
> +	}
> +	sec_heap->mem_session = arg.session;
> +	return 0;
> +
> +close_context:
> +	tee_client_close_context(sec_heap->tee_ctx);
> +	return ret;
> +}
> +
>   static struct dma_buf *
>   mtk_sec_heap_allocate(struct dma_heap *heap, size_t size,
>   		      unsigned long fd_flags, unsigned long heap_flags)
>   {
> +	struct mtk_secure_heap *sec_heap = dma_heap_get_drvdata(heap);
>   	struct mtk_secure_heap_buffer *sec_buf;
>   	DEFINE_DMA_BUF_EXPORT_INFO(exp_info);
>   	struct dma_buf *dmabuf;
>   	int ret;
>   
> +	/*
> +	 * TEE probe may be late. Initialise the secure session in the first
> +	 * allocating secure buffer.
> +	 */
> +	if (!sec_heap->mem_session) {
> +		ret = mtk_kree_secure_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);

Il 11/09/23 04:30, Yong Wu ha scritto:
> Create a new mtk_svp_cma heap from the CMA reserved buffer.
> 
> 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.
> 
> When SVP done, cma_release will release the buffer, then kernel may
> reuse it.
> 
> Signed-off-by: Yong Wu <yong.wu@mediatek.com>
> ---
>   drivers/dma-buf/heaps/Kconfig           |   2 +-
>   drivers/dma-buf/heaps/mtk_secure_heap.c | 121 +++++++++++++++++++++++-
>   2 files changed, 119 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/dma-buf/heaps/Kconfig b/drivers/dma-buf/heaps/Kconfig
> index 729c0cf3eb7c..e101f788ecbf 100644
> --- a/drivers/dma-buf/heaps/Kconfig
> +++ b/drivers/dma-buf/heaps/Kconfig
> @@ -15,7 +15,7 @@ config DMABUF_HEAPS_CMA
>   
>   config DMABUF_HEAPS_MTK_SECURE
>   	bool "DMA-BUF MediaTek Secure Heap"
> -	depends on DMABUF_HEAPS && TEE
> +	depends on DMABUF_HEAPS && TEE && CMA
>   	help
>   	  Choose this option to enable dma-buf MediaTek secure heap for Secure
>   	  Video Path. This heap is backed by TEE client interfaces. If in
> diff --git a/drivers/dma-buf/heaps/mtk_secure_heap.c b/drivers/dma-buf/heaps/mtk_secure_heap.c
> index daf6cf2121a1..3f568fe6b569 100644
> --- a/drivers/dma-buf/heaps/mtk_secure_heap.c
> +++ b/drivers/dma-buf/heaps/mtk_secure_heap.c

..snip..

> +}
> +
> +RESERVEDMEM_OF_DECLARE(mtk_secure_cma, "mediatek,secure_cma_chunkmem",

I'd suggest "mediatek,secure-heap" as compatible name.

> +		       mtk_secure_cma_init);
> +

Regards,
Angelo

m 11.09.23 um 04:30 schrieb Yong Wu:
> From: "T.J. Mercier" <tjmercier@google.com>
>
> The docs for dma_heap_get_name were incorrect, and since they were
> duplicated in the implementation file they were wrong there too.
>
> The docs formatting was inconsistent so I tried to make it more
> consistent across functions since I'm already in here doing cleanup.
>
> Remove multiple unused includes.
>
> Signed-off-by: T.J. Mercier <tjmercier@google.com>
> Signed-off-by: Yong Wu <yong.wu@mediatek.com>
> [Yong: Just add a comment for "priv" to mute build warning]
> ---
>   drivers/dma-buf/dma-heap.c | 29 +++++++----------------------
>   include/linux/dma-heap.h   | 11 +++++------
>   2 files changed, 12 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/dma-buf/dma-heap.c b/drivers/dma-buf/dma-heap.c
> index 84ae708fafe7..51030f6c9d6e 100644
> --- a/drivers/dma-buf/dma-heap.c
> +++ b/drivers/dma-buf/dma-heap.c
> @@ -7,17 +7,15 @@
>    */
>   
>   #include <linux/cdev.h>
> -#include <linux/debugfs.h>
>   #include <linux/device.h>
>   #include <linux/dma-buf.h>
> +#include <linux/dma-heap.h>
>   #include <linux/err.h>
> -#include <linux/xarray.h>
>   #include <linux/list.h>
> -#include <linux/slab.h>
>   #include <linux/nospec.h>
> -#include <linux/uaccess.h>
>   #include <linux/syscalls.h>
> -#include <linux/dma-heap.h>
> +#include <linux/uaccess.h>
> +#include <linux/xarray.h>
>   #include <uapi/linux/dma-heap.h>
>   
>   #define DEVNAME "dma_heap"
> @@ -28,9 +26,10 @@
>    * struct dma_heap - represents a dmabuf heap in the system
>    * @name:		used for debugging/device-node name
>    * @ops:		ops struct for this heap
> - * @heap_devt		heap device node
> - * @list		list head connecting to list of heaps
> - * @heap_cdev		heap char device
> + * @priv:		private data for this heap
> + * @heap_devt:		heap device node
> + * @list:		list head connecting to list of heaps
> + * @heap_cdev:		heap char device
>    *
>    * Represents a heap of memory from which buffers can be made.
>    */
> @@ -192,25 +191,11 @@ static const struct file_operations dma_heap_fops = {
>   #endif
>   };
>   
> -/**
> - * dma_heap_get_drvdata() - get per-subdriver data for the heap
> - * @heap: DMA-Heap to retrieve private data for
> - *
> - * Returns:
> - * The per-subdriver data for the heap.
> - */

Kernel documentation is usually kept on the implementation and not the 
definition.

So I strongly suggest to remove the documentation from the header 
instead and if there is any additional information in there add it here.

Regards,
Christian.

>   void *dma_heap_get_drvdata(struct dma_heap *heap)
>   {
>   	return heap->priv;
>   }
>   
> -/**
> - * dma_heap_get_name() - get heap name
> - * @heap: DMA-Heap to retrieve private data for
> - *
> - * Returns:
> - * The char* for the heap name.
> - */
>   const char *dma_heap_get_name(struct dma_heap *heap)
>   {
>   	return heap->name;
> diff --git a/include/linux/dma-heap.h b/include/linux/dma-heap.h
> index 0c05561cad6e..c7c29b724ad6 100644
> --- a/include/linux/dma-heap.h
> +++ b/include/linux/dma-heap.h
> @@ -9,14 +9,13 @@
>   #ifndef _DMA_HEAPS_H
>   #define _DMA_HEAPS_H
>   
> -#include <linux/cdev.h>
>   #include <linux/types.h>
>   
>   struct dma_heap;
>   
>   /**
>    * struct dma_heap_ops - ops to operate on a given heap
> - * @allocate:		allocate dmabuf and return struct dma_buf ptr
> + * @allocate: allocate dmabuf and return struct dma_buf ptr
>    *
>    * allocate returns dmabuf on success, ERR_PTR(-errno) on error.
>    */
> @@ -42,7 +41,7 @@ struct dma_heap_export_info {
>   };
>   
>   /**
> - * dma_heap_get_drvdata() - get per-heap driver data
> + * dma_heap_get_drvdata - get per-heap driver data
>    * @heap: DMA-Heap to retrieve private data for
>    *
>    * Returns:
> @@ -51,8 +50,8 @@ struct dma_heap_export_info {
>   void *dma_heap_get_drvdata(struct dma_heap *heap);
>   
>   /**
> - * dma_heap_get_name() - get heap name
> - * @heap: DMA-Heap to retrieve private data for
> + * dma_heap_get_name - get heap name
> + * @heap: DMA-Heap to retrieve the name of
>    *
>    * Returns:
>    * The char* for the heap name.
> @@ -61,7 +60,7 @@ const char *dma_heap_get_name(struct dma_heap *heap);
>   
>   /**
>    * dma_heap_add - adds a heap to dmabuf heaps
> - * @exp_info:		information needed to register this heap
> + * @exp_info: information needed to register this heap
>    */
>   struct dma_heap *dma_heap_add(const struct dma_heap_export_info *exp_info);
>

Am 11.09.23 um 04:30 schrieb Yong Wu:
> From: John Stultz <jstultz@google.com>
>
> Add proper refcounting on the dma_heap structure.
> While existing heaps are built-in, we may eventually
> have heaps loaded from modules, and we'll need to be
> able to properly handle the references to the heaps
>
> Also moves minor tracking into the heap structure so
> we can properly free things.

This is completely unnecessary, see below.

>
> Signed-off-by: John Stultz <jstultz@google.com>
> Signed-off-by: T.J. Mercier <tjmercier@google.com>
> Signed-off-by: Yong Wu <yong.wu@mediatek.com>
> [Yong: Just add comment for "minor" and "refcount"]
> ---
>   drivers/dma-buf/dma-heap.c | 38 ++++++++++++++++++++++++++++++++++----
>   include/linux/dma-heap.h   |  6 ++++++
>   2 files changed, 40 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/dma-buf/dma-heap.c b/drivers/dma-buf/dma-heap.c
> index 51030f6c9d6e..dcc0e38c61fa 100644
> --- a/drivers/dma-buf/dma-heap.c
> +++ b/drivers/dma-buf/dma-heap.c
> @@ -11,6 +11,7 @@
>   #include <linux/dma-buf.h>
>   #include <linux/dma-heap.h>
>   #include <linux/err.h>
> +#include <linux/kref.h>
>   #include <linux/list.h>
>   #include <linux/nospec.h>
>   #include <linux/syscalls.h>
> @@ -30,6 +31,8 @@
>    * @heap_devt:		heap device node
>    * @list:		list head connecting to list of heaps
>    * @heap_cdev:		heap char device
> + * @minor:		heap device node minor number
> + * @refcount:		reference counter for this heap device
>    *
>    * Represents a heap of memory from which buffers can be made.
>    */
> @@ -40,6 +43,8 @@ struct dma_heap {
>   	dev_t heap_devt;
>   	struct list_head list;
>   	struct cdev heap_cdev;
> +	int minor;
> +	struct kref refcount;
>   };
>   
>   static LIST_HEAD(heap_list);
> @@ -205,7 +210,6 @@ struct dma_heap *dma_heap_add(const struct dma_heap_export_info *exp_info)
>   {
>   	struct dma_heap *heap, *h, *err_ret;
>   	struct device *dev_ret;
> -	unsigned int minor;
>   	int ret;
>   
>   	if (!exp_info->name || !strcmp(exp_info->name, "")) {
> @@ -222,12 +226,13 @@ struct dma_heap *dma_heap_add(const struct dma_heap_export_info *exp_info)
>   	if (!heap)
>   		return ERR_PTR(-ENOMEM);
>   
> +	kref_init(&heap->refcount);
>   	heap->name = exp_info->name;
>   	heap->ops = exp_info->ops;
>   	heap->priv = exp_info->priv;
>   
>   	/* Find unused minor number */
> -	ret = xa_alloc(&dma_heap_minors, &minor, heap,
> +	ret = xa_alloc(&dma_heap_minors, &heap->minor, heap,
>   		       XA_LIMIT(0, NUM_HEAP_MINORS - 1), GFP_KERNEL);
>   	if (ret < 0) {
>   		pr_err("dma_heap: Unable to get minor number for heap\n");
> @@ -236,7 +241,7 @@ struct dma_heap *dma_heap_add(const struct dma_heap_export_info *exp_info)
>   	}
>   
>   	/* Create device */
> -	heap->heap_devt = MKDEV(MAJOR(dma_heap_devt), minor);
> +	heap->heap_devt = MKDEV(MAJOR(dma_heap_devt), heap->minor);
>   
>   	cdev_init(&heap->heap_cdev, &dma_heap_fops);
>   	ret = cdev_add(&heap->heap_cdev, heap->heap_devt, 1);
> @@ -280,12 +285,37 @@ struct dma_heap *dma_heap_add(const struct dma_heap_export_info *exp_info)
>   err2:
>   	cdev_del(&heap->heap_cdev);
>   err1:
> -	xa_erase(&dma_heap_minors, minor);
> +	xa_erase(&dma_heap_minors, heap->minor);
>   err0:
>   	kfree(heap);
>   	return err_ret;
>   }
>   
> +static void dma_heap_release(struct kref *ref)
> +{
> +	struct dma_heap *heap = container_of(ref, struct dma_heap, refcount);
> +
> +	/* Note, we already holding the heap_list_lock here */
> +	list_del(&heap->list);
> +
> +	device_destroy(dma_heap_class, heap->heap_devt);
> +	cdev_del(&heap->heap_cdev);
> +	xa_erase(&dma_heap_minors, heap->minor);

You can just use MINOR(heap->heap_devt) here instead.

> +
> +	kfree(heap);
> +}
> +
> +void dma_heap_put(struct dma_heap *h)
> +{
> +	/*
> +	 * Take the heap_list_lock now to avoid racing with code
> +	 * scanning the list and then taking a kref.
> +	 */

This is usually considered a bad idea since it makes the kref approach 
superfluous.

There are multiple possibilities how handle this, the most common one is 
to use kref_get_unless_zero() in your list traversal code and ignore the 
entry when that fails.

Alternatively you could use kref_put_mutex() instead. This gives you the 
same functionality as this here, but as far as I know it's normally only 
used in a couple of special cases.

> +	mutex_lock(&heap_list_lock);
> +	kref_put(&h->refcount, dma_heap_release);
> +	mutex_unlock(&heap_list_lock);
> +}
> +
>   static char *dma_heap_devnode(const struct device *dev, umode_t *mode)
>   {
>   	return kasprintf(GFP_KERNEL, "dma_heap/%s", dev_name(dev));
> diff --git a/include/linux/dma-heap.h b/include/linux/dma-heap.h
> index c7c29b724ad6..f3c678892c5c 100644
> --- a/include/linux/dma-heap.h
> +++ b/include/linux/dma-heap.h
> @@ -64,4 +64,10 @@ const char *dma_heap_get_name(struct dma_heap *heap);
>    */
>   struct dma_heap *dma_heap_add(const struct dma_heap_export_info *exp_info);
>   
> +/**
> + * dma_heap_put - drops a reference to a dmabuf heap, potentially freeing it
> + * @heap: the heap whose reference count to decrement
> + */

Please don't add kerneldoc to the definition, add it to the 
implementation of the function.

Regards,
Christian.

> +void dma_heap_put(struct dma_heap *heap);
> +
>   #endif /* _DMA_HEAPS_H */

Am 11.09.23 um 04:30 schrieb Yong Wu:
> From: John Stultz <jstultz@google.com>
>
> This allows drivers who don't want to create their own
> DMA-BUF exporter to be able to allocate DMA-BUFs directly
> from existing DMA-BUF Heaps.
>
> There is some concern that the premise of DMA-BUF heaps is
> that userland knows better about what type of heap memory
> is needed for a pipeline, so it would likely be best for
> drivers to import and fill DMA-BUFs allocated by userland
> instead of allocating one themselves, but this is still
> up for debate.

The main design goal of having DMA-heaps in the first place is to avoid 
per driver allocation and this is not necessary because userland know 
better what type of memory it wants.

The background is rather that we generally want to decouple allocation 
from having a device driver connection so that we have better chance 
that multiple devices can work with the same memory.

I once create a prototype which gives userspace a hint which DMA-heap to 
user for which device: 
https://patchwork.kernel.org/project/linux-media/patch/20230123123756.401692-2-christian.koenig@amd.com/

Problem is that I don't really have time to look into it and maintain 
that stuff, but I think from the high level design that is rather the 
general direction we should push at.

Regards,
Christian.

>
> Signed-off-by: John Stultz <jstultz@google.com>
> Signed-off-by: T.J. Mercier <tjmercier@google.com>
> Signed-off-by: Yong Wu <yong.wu@mediatek.com>
> [Yong: Fix the checkpatch alignment warning]
> ---
>   drivers/dma-buf/dma-heap.c | 60 ++++++++++++++++++++++++++++----------
>   include/linux/dma-heap.h   | 25 ++++++++++++++++
>   2 files changed, 69 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/dma-buf/dma-heap.c b/drivers/dma-buf/dma-heap.c
> index dcc0e38c61fa..908bb30dc864 100644
> --- a/drivers/dma-buf/dma-heap.c
> +++ b/drivers/dma-buf/dma-heap.c
> @@ -53,12 +53,15 @@ static dev_t dma_heap_devt;
>   static struct class *dma_heap_class;
>   static DEFINE_XARRAY_ALLOC(dma_heap_minors);
>   
> -static int dma_heap_buffer_alloc(struct dma_heap *heap, size_t len,
> -				 unsigned int fd_flags,
> -				 unsigned int heap_flags)
> +struct dma_buf *dma_heap_buffer_alloc(struct dma_heap *heap, size_t len,
> +				      unsigned int fd_flags,
> +				      unsigned int heap_flags)
>   {
> -	struct dma_buf *dmabuf;
> -	int fd;
> +	if (fd_flags & ~DMA_HEAP_VALID_FD_FLAGS)
> +		return ERR_PTR(-EINVAL);
> +
> +	if (heap_flags & ~DMA_HEAP_VALID_HEAP_FLAGS)
> +		return ERR_PTR(-EINVAL);
>   
>   	/*
>   	 * Allocations from all heaps have to begin
> @@ -66,9 +69,20 @@ static int dma_heap_buffer_alloc(struct dma_heap *heap, size_t len,
>   	 */
>   	len = PAGE_ALIGN(len);
>   	if (!len)
> -		return -EINVAL;
> +		return ERR_PTR(-EINVAL);
>   
> -	dmabuf = heap->ops->allocate(heap, len, fd_flags, heap_flags);
> +	return heap->ops->allocate(heap, len, fd_flags, heap_flags);
> +}
> +EXPORT_SYMBOL_GPL(dma_heap_buffer_alloc);
> +
> +static int dma_heap_bufferfd_alloc(struct dma_heap *heap, size_t len,
> +				   unsigned int fd_flags,
> +				   unsigned int heap_flags)
> +{
> +	struct dma_buf *dmabuf;
> +	int fd;
> +
> +	dmabuf = dma_heap_buffer_alloc(heap, len, fd_flags, heap_flags);
>   	if (IS_ERR(dmabuf))
>   		return PTR_ERR(dmabuf);
>   
> @@ -106,15 +120,9 @@ static long dma_heap_ioctl_allocate(struct file *file, void *data)
>   	if (heap_allocation->fd)
>   		return -EINVAL;
>   
> -	if (heap_allocation->fd_flags & ~DMA_HEAP_VALID_FD_FLAGS)
> -		return -EINVAL;
> -
> -	if (heap_allocation->heap_flags & ~DMA_HEAP_VALID_HEAP_FLAGS)
> -		return -EINVAL;
> -
> -	fd = dma_heap_buffer_alloc(heap, heap_allocation->len,
> -				   heap_allocation->fd_flags,
> -				   heap_allocation->heap_flags);
> +	fd = dma_heap_bufferfd_alloc(heap, heap_allocation->len,
> +				     heap_allocation->fd_flags,
> +				     heap_allocation->heap_flags);
>   	if (fd < 0)
>   		return fd;
>   
> @@ -205,6 +213,7 @@ const char *dma_heap_get_name(struct dma_heap *heap)
>   {
>   	return heap->name;
>   }
> +EXPORT_SYMBOL_GPL(dma_heap_get_name);
>   
>   struct dma_heap *dma_heap_add(const struct dma_heap_export_info *exp_info)
>   {
> @@ -290,6 +299,24 @@ struct dma_heap *dma_heap_add(const struct dma_heap_export_info *exp_info)
>   	kfree(heap);
>   	return err_ret;
>   }
> +EXPORT_SYMBOL_GPL(dma_heap_add);
> +
> +struct dma_heap *dma_heap_find(const char *name)
> +{
> +	struct dma_heap *h;
> +
> +	mutex_lock(&heap_list_lock);
> +	list_for_each_entry(h, &heap_list, list) {
> +		if (!strcmp(h->name, name)) {
> +			kref_get(&h->refcount);
> +			mutex_unlock(&heap_list_lock);
> +			return h;
> +		}
> +	}
> +	mutex_unlock(&heap_list_lock);
> +	return NULL;
> +}
> +EXPORT_SYMBOL_GPL(dma_heap_find);
>   
>   static void dma_heap_release(struct kref *ref)
>   {
> @@ -315,6 +342,7 @@ void dma_heap_put(struct dma_heap *h)
>   	kref_put(&h->refcount, dma_heap_release);
>   	mutex_unlock(&heap_list_lock);
>   }
> +EXPORT_SYMBOL_GPL(dma_heap_put);
>   
>   static char *dma_heap_devnode(const struct device *dev, umode_t *mode)
>   {
> diff --git a/include/linux/dma-heap.h b/include/linux/dma-heap.h
> index f3c678892c5c..59e70f6c7a60 100644
> --- a/include/linux/dma-heap.h
> +++ b/include/linux/dma-heap.h
> @@ -64,10 +64,35 @@ const char *dma_heap_get_name(struct dma_heap *heap);
>    */
>   struct dma_heap *dma_heap_add(const struct dma_heap_export_info *exp_info);
>   
> +/**
> + * dma_heap_find - get the heap registered with the specified name
> + * @name: Name of the DMA-Heap to find
> + *
> + * Returns:
> + * The DMA-Heap with the provided name.
> + *
> + * NOTE: DMA-Heaps returned from this function MUST be released using
> + * dma_heap_put() when the user is done to enable the heap to be unloaded.
> + */
> +struct dma_heap *dma_heap_find(const char *name);
> +
>   /**
>    * dma_heap_put - drops a reference to a dmabuf heap, potentially freeing it
>    * @heap: the heap whose reference count to decrement
>    */
>   void dma_heap_put(struct dma_heap *heap);
>   
> +/**
> + * dma_heap_buffer_alloc - Allocate dma-buf from a dma_heap
> + * @heap:	DMA-Heap to allocate from
> + * @len:	size to allocate in bytes
> + * @fd_flags:	flags to set on returned dma-buf fd
> + * @heap_flags: flags to pass to the dma heap
> + *
> + * This is for internal dma-buf allocations only. Free returned buffers with dma_buf_put().
> + */
> +struct dma_buf *dma_heap_buffer_alloc(struct dma_heap *heap, size_t len,
> +				      unsigned int fd_flags,
> +				      unsigned int heap_flags);
> +
>   #endif /* _DMA_HEAPS_H */

Am 11.09.23 um 11:29 schrieb AngeloGioacchino Del Regno:
> Il 11/09/23 04:30, Yong Wu ha scritto:
>> The TEE probe later than dma-buf heap, and PROBE_DEDER doesn't work
>> here since this is not a platform driver, therefore initialise the TEE
>> context/session while we allocate the first secure buffer.
>>
>> Signed-off-by: Yong Wu <yong.wu@mediatek.com>
>> ---
>>   drivers/dma-buf/heaps/mtk_secure_heap.c | 61 +++++++++++++++++++++++++
>>   1 file changed, 61 insertions(+)
>>
>> diff --git a/drivers/dma-buf/heaps/mtk_secure_heap.c 
>> b/drivers/dma-buf/heaps/mtk_secure_heap.c
>> index bbf1c8dce23e..e3da33a3d083 100644
>> --- a/drivers/dma-buf/heaps/mtk_secure_heap.c
>> +++ b/drivers/dma-buf/heaps/mtk_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 "4477588a-8476-11e2-ad15-e41f1390d676"
>> +
>
> Is this UUID the same for all SoCs and all TZ versions?

And how is this exposed? If it's part of the UAPI then it should 
probably better be defined somewhere in include/uapi.

Regards,
Christian.

>
> Thanks,
> Angelo
>
>
>> +#define MTK_TEE_PARAM_NUM        4
>>     /*
>>    * MediaTek secure (chunk) memory type
>> @@ -28,17 +34,72 @@ struct mtk_secure_heap_buffer {
>>   struct mtk_secure_heap {
>>       const char        *name;
>>       const enum kree_mem_type mem_type;
>> +    u32             mem_session;
>> +    struct tee_context    *tee_ctx;
>>   };
>>   +static int mtk_optee_ctx_match(struct tee_ioctl_version_data *ver, 
>> const void *data)
>> +{
>> +    return ver->impl_id == TEE_IMPL_ID_OPTEE;
>> +}
>> +
>> +static int mtk_kree_secure_session_init(struct mtk_secure_heap 
>> *sec_heap)
>> +{
>> +    struct tee_param t_param[MTK_TEE_PARAM_NUM] = {0};
>> +    struct tee_ioctl_open_session_arg arg = {0};
>> +    uuid_t ta_mem_uuid;
>> +    int ret;
>> +
>> +    sec_heap->tee_ctx = tee_client_open_context(NULL, 
>> mtk_optee_ctx_match,
>> +                            NULL, NULL);
>> +    if (IS_ERR(sec_heap->tee_ctx)) {
>> +        pr_err("%s: open context failed, ret=%ld\n", sec_heap->name,
>> +               PTR_ERR(sec_heap->tee_ctx));
>> +        return -ENODEV;
>> +    }
>> +
>> +    arg.num_params = MTK_TEE_PARAM_NUM;
>> +    arg.clnt_login = TEE_IOCTL_LOGIN_PUBLIC;
>> +    ret = uuid_parse(TZ_TA_MEM_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("%s: open session failed, ret=%d:%d\n",
>> +               sec_heap->name, ret, arg.ret);
>> +        ret = -EINVAL;
>> +        goto close_context;
>> +    }
>> +    sec_heap->mem_session = arg.session;
>> +    return 0;
>> +
>> +close_context:
>> +    tee_client_close_context(sec_heap->tee_ctx);
>> +    return ret;
>> +}
>> +
>>   static struct dma_buf *
>>   mtk_sec_heap_allocate(struct dma_heap *heap, size_t size,
>>                 unsigned long fd_flags, unsigned long heap_flags)
>>   {
>> +    struct mtk_secure_heap *sec_heap = dma_heap_get_drvdata(heap);
>>       struct mtk_secure_heap_buffer *sec_buf;
>>       DEFINE_DMA_BUF_EXPORT_INFO(exp_info);
>>       struct dma_buf *dmabuf;
>>       int ret;
>>   +    /*
>> +     * TEE probe may be late. Initialise the secure session in the 
>> first
>> +     * allocating secure buffer.
>> +     */
>> +    if (!sec_heap->mem_session) {
>> +        ret = mtk_kree_secure_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);
>

Hi,

Le lundi 11 septembre 2023 à 10:30 +0800, Yong Wu a écrit :
> From: John Stultz <jstultz@google.com>
> 
> This allows drivers who don't want to create their own
> DMA-BUF exporter to be able to allocate DMA-BUFs directly
> from existing DMA-BUF Heaps.
> 
> There is some concern that the premise of DMA-BUF heaps is
> that userland knows better about what type of heap memory
> is needed for a pipeline, so it would likely be best for
> drivers to import and fill DMA-BUFs allocated by userland
> instead of allocating one themselves, but this is still
> up for debate.


Would be nice for the reviewers to provide the information about the user of
this new in-kernel API. I noticed it because I was CCed, but strangely it didn't
make it to the mailing list yet and its not clear in the cover what this is used
with. 

I can explain in my words though, my read is that this is used to allocate both
user visible and driver internal memory segments in MTK VCODEC driver.

I'm somewhat concerned that DMABuf objects are used to abstract secure memory
allocation from tee. For framebuffers that are going to be exported and shared
its probably fair use, but it seems that internal shared memory and codec
specific reference buffers also endup with a dmabuf fd (often called a secure fd
in the v4l2 patchset) for data that is not being shared, and requires a 1:1
mapping to a tee handle anyway. Is that the design we'd like to follow ? Can't
we directly allocate from the tee, adding needed helper to make this as simple
as allocating from a HEAP ?

Nicolas

> 
> Signed-off-by: John Stultz <jstultz@google.com>
> Signed-off-by: T.J. Mercier <tjmercier@google.com>
> Signed-off-by: Yong Wu <yong.wu@mediatek.com>
> [Yong: Fix the checkpatch alignment warning]
> ---
>  drivers/dma-buf/dma-heap.c | 60 ++++++++++++++++++++++++++++----------
>  include/linux/dma-heap.h   | 25 ++++++++++++++++
>  2 files changed, 69 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/dma-buf/dma-heap.c b/drivers/dma-buf/dma-heap.c
> index dcc0e38c61fa..908bb30dc864 100644
> --- a/drivers/dma-buf/dma-heap.c
> +++ b/drivers/dma-buf/dma-heap.c
> @@ -53,12 +53,15 @@ static dev_t dma_heap_devt;
>  static struct class *dma_heap_class;
>  static DEFINE_XARRAY_ALLOC(dma_heap_minors);
>  
> -static int dma_heap_buffer_alloc(struct dma_heap *heap, size_t len,
> -				 unsigned int fd_flags,
> -				 unsigned int heap_flags)
> +struct dma_buf *dma_heap_buffer_alloc(struct dma_heap *heap, size_t len,
> +				      unsigned int fd_flags,
> +				      unsigned int heap_flags)
>  {
> -	struct dma_buf *dmabuf;
> -	int fd;
> +	if (fd_flags & ~DMA_HEAP_VALID_FD_FLAGS)
> +		return ERR_PTR(-EINVAL);
> +
> +	if (heap_flags & ~DMA_HEAP_VALID_HEAP_FLAGS)
> +		return ERR_PTR(-EINVAL);
>  
>  	/*
>  	 * Allocations from all heaps have to begin
> @@ -66,9 +69,20 @@ static int dma_heap_buffer_alloc(struct dma_heap *heap, size_t len,
>  	 */
>  	len = PAGE_ALIGN(len);
>  	if (!len)
> -		return -EINVAL;
> +		return ERR_PTR(-EINVAL);
>  
> -	dmabuf = heap->ops->allocate(heap, len, fd_flags, heap_flags);
> +	return heap->ops->allocate(heap, len, fd_flags, heap_flags);
> +}
> +EXPORT_SYMBOL_GPL(dma_heap_buffer_alloc);
> +
> +static int dma_heap_bufferfd_alloc(struct dma_heap *heap, size_t len,
> +				   unsigned int fd_flags,
> +				   unsigned int heap_flags)
> +{
> +	struct dma_buf *dmabuf;
> +	int fd;
> +
> +	dmabuf = dma_heap_buffer_alloc(heap, len, fd_flags, heap_flags);
>  	if (IS_ERR(dmabuf))
>  		return PTR_ERR(dmabuf);
>  
> @@ -106,15 +120,9 @@ static long dma_heap_ioctl_allocate(struct file *file, void *data)
>  	if (heap_allocation->fd)
>  		return -EINVAL;
>  
> -	if (heap_allocation->fd_flags & ~DMA_HEAP_VALID_FD_FLAGS)
> -		return -EINVAL;
> -
> -	if (heap_allocation->heap_flags & ~DMA_HEAP_VALID_HEAP_FLAGS)
> -		return -EINVAL;
> -
> -	fd = dma_heap_buffer_alloc(heap, heap_allocation->len,
> -				   heap_allocation->fd_flags,
> -				   heap_allocation->heap_flags);
> +	fd = dma_heap_bufferfd_alloc(heap, heap_allocation->len,
> +				     heap_allocation->fd_flags,
> +				     heap_allocation->heap_flags);
>  	if (fd < 0)
>  		return fd;
>  
> @@ -205,6 +213,7 @@ const char *dma_heap_get_name(struct dma_heap *heap)
>  {
>  	return heap->name;
>  }
> +EXPORT_SYMBOL_GPL(dma_heap_get_name);
>  
>  struct dma_heap *dma_heap_add(const struct dma_heap_export_info *exp_info)
>  {
> @@ -290,6 +299,24 @@ struct dma_heap *dma_heap_add(const struct dma_heap_export_info *exp_info)
>  	kfree(heap);
>  	return err_ret;
>  }
> +EXPORT_SYMBOL_GPL(dma_heap_add);
> +
> +struct dma_heap *dma_heap_find(const char *name)
> +{
> +	struct dma_heap *h;
> +
> +	mutex_lock(&heap_list_lock);
> +	list_for_each_entry(h, &heap_list, list) {
> +		if (!strcmp(h->name, name)) {
> +			kref_get(&h->refcount);
> +			mutex_unlock(&heap_list_lock);
> +			return h;
> +		}
> +	}
> +	mutex_unlock(&heap_list_lock);
> +	return NULL;
> +}
> +EXPORT_SYMBOL_GPL(dma_heap_find);
>  
>  static void dma_heap_release(struct kref *ref)
>  {
> @@ -315,6 +342,7 @@ void dma_heap_put(struct dma_heap *h)
>  	kref_put(&h->refcount, dma_heap_release);
>  	mutex_unlock(&heap_list_lock);
>  }
> +EXPORT_SYMBOL_GPL(dma_heap_put);
>  
>  static char *dma_heap_devnode(const struct device *dev, umode_t *mode)
>  {
> diff --git a/include/linux/dma-heap.h b/include/linux/dma-heap.h
> index f3c678892c5c..59e70f6c7a60 100644
> --- a/include/linux/dma-heap.h
> +++ b/include/linux/dma-heap.h
> @@ -64,10 +64,35 @@ const char *dma_heap_get_name(struct dma_heap *heap);
>   */
>  struct dma_heap *dma_heap_add(const struct dma_heap_export_info *exp_info);
>  
> +/**
> + * dma_heap_find - get the heap registered with the specified name
> + * @name: Name of the DMA-Heap to find
> + *
> + * Returns:
> + * The DMA-Heap with the provided name.
> + *
> + * NOTE: DMA-Heaps returned from this function MUST be released using
> + * dma_heap_put() when the user is done to enable the heap to be unloaded.
> + */
> +struct dma_heap *dma_heap_find(const char *name);
> +
>  /**
>   * dma_heap_put - drops a reference to a dmabuf heap, potentially freeing it
>   * @heap: the heap whose reference count to decrement
>   */
>  void dma_heap_put(struct dma_heap *heap);
>  
> +/**
> + * dma_heap_buffer_alloc - Allocate dma-buf from a dma_heap
> + * @heap:	DMA-Heap to allocate from
> + * @len:	size to allocate in bytes
> + * @fd_flags:	flags to set on returned dma-buf fd
> + * @heap_flags: flags to pass to the dma heap
> + *
> + * This is for internal dma-buf allocations only. Free returned buffers with dma_buf_put().
> + */
> +struct dma_buf *dma_heap_buffer_alloc(struct dma_heap *heap, size_t len,
> +				      unsigned int fd_flags,
> +				      unsigned int heap_flags);
> +
>  #endif /* _DMA_HEAPS_H */

On Mon, Sep 11, 2023 at 3:14 AM Christian König
<christian.koenig@amd.com> wrote:
> Am 11.09.23 um 04:30 schrieb Yong Wu:
> > From: John Stultz <jstultz@google.com>
> >
> > This allows drivers who don't want to create their own
> > DMA-BUF exporter to be able to allocate DMA-BUFs directly
> > from existing DMA-BUF Heaps.
> >
> > There is some concern that the premise of DMA-BUF heaps is
> > that userland knows better about what type of heap memory
> > is needed for a pipeline, so it would likely be best for
> > drivers to import and fill DMA-BUFs allocated by userland
> > instead of allocating one themselves, but this is still
> > up for debate.
>
> The main design goal of having DMA-heaps in the first place is to avoid
> per driver allocation and this is not necessary because userland know
> better what type of memory it wants.
>
> The background is rather that we generally want to decouple allocation
> from having a device driver connection so that we have better chance
> that multiple devices can work with the same memory.

Yep, very much agreed, and this is what the comment above is trying to describe.

Ideally user-allocated buffers would be used to ensure driver's don't
create buffers with constraints that limit which devices the buffers
might later be shared with.

However, this patch was created as a hold-over from the old ION logic
to help vendors transition to dmabuf heaps, as vendors had situations
where they still wanted to export dmabufs that were not to be
generally shared and folks wanted to avoid duplication of logic
already in existing heaps.  At the time, I never pushed it upstream as
there were no upstream users.  But I think if there is now a potential
upstream user, it's worth having the discussion to better understand
the need.

So I think this patch is a little confusing in this series, as I don't
see much of it actually being used here (though forgive me if I'm
missing it).

Instead, It seems it get used in a separate patch series here:
  https://lore.kernel.org/all/20230911125936.10648-1-yunfei.dong@mediatek.com/

Yong, I appreciate you sending this out! But maybe if the secure heap
submission doesn't depend on this functionality, I might suggest
moving this patch (or at least the majority of it) to be part of the
vcodec series instead?  That way reviewers will have more context for
how the code being added is used?

thanks
-john

On Mon, Sep 11, 2023 at 2:36 AM Christian König
<christian.koenig@amd.com> wrote:
>
> m 11.09.23 um 04:30 schrieb Yong Wu:
> > From: "T.J. Mercier" <tjmercier@google.com>
> >
> > The docs for dma_heap_get_name were incorrect, and since they were
> > duplicated in the implementation file they were wrong there too.
> >
> > The docs formatting was inconsistent so I tried to make it more
> > consistent across functions since I'm already in here doing cleanup.
> >
> > Remove multiple unused includes.
> >
> > Signed-off-by: T.J. Mercier <tjmercier@google.com>
> > Signed-off-by: Yong Wu <yong.wu@mediatek.com>
> > [Yong: Just add a comment for "priv" to mute build warning]
> > ---
> >   drivers/dma-buf/dma-heap.c | 29 +++++++----------------------
> >   include/linux/dma-heap.h   | 11 +++++------
> >   2 files changed, 12 insertions(+), 28 deletions(-)
> >
> > diff --git a/drivers/dma-buf/dma-heap.c b/drivers/dma-buf/dma-heap.c
> > index 84ae708fafe7..51030f6c9d6e 100644
> > --- a/drivers/dma-buf/dma-heap.c
> > +++ b/drivers/dma-buf/dma-heap.c
> > @@ -7,17 +7,15 @@
> >    */
> >
> >   #include <linux/cdev.h>
> > -#include <linux/debugfs.h>
> >   #include <linux/device.h>
> >   #include <linux/dma-buf.h>
> > +#include <linux/dma-heap.h>
> >   #include <linux/err.h>
> > -#include <linux/xarray.h>
> >   #include <linux/list.h>
> > -#include <linux/slab.h>
> >   #include <linux/nospec.h>
> > -#include <linux/uaccess.h>
> >   #include <linux/syscalls.h>
> > -#include <linux/dma-heap.h>
> > +#include <linux/uaccess.h>
> > +#include <linux/xarray.h>
> >   #include <uapi/linux/dma-heap.h>
> >
> >   #define DEVNAME "dma_heap"
> > @@ -28,9 +26,10 @@
> >    * struct dma_heap - represents a dmabuf heap in the system
> >    * @name:           used for debugging/device-node name
> >    * @ops:            ops struct for this heap
> > - * @heap_devt                heap device node
> > - * @list             list head connecting to list of heaps
> > - * @heap_cdev                heap char device
> > + * @priv:            private data for this heap
> > + * @heap_devt:               heap device node
> > + * @list:            list head connecting to list of heaps
> > + * @heap_cdev:               heap char device
> >    *
> >    * Represents a heap of memory from which buffers can be made.
> >    */
> > @@ -192,25 +191,11 @@ static const struct file_operations dma_heap_fops = {
> >   #endif
> >   };
> >
> > -/**
> > - * dma_heap_get_drvdata() - get per-subdriver data for the heap
> > - * @heap: DMA-Heap to retrieve private data for
> > - *
> > - * Returns:
> > - * The per-subdriver data for the heap.
> > - */
>
> Kernel documentation is usually kept on the implementation and not the
> definition.
>
> So I strongly suggest to remove the documentation from the header
> instead and if there is any additional information in there add it here.
>
> Regards,
> Christian.
>
Ok thanks for looking. I'll move all the function docs over to the
implementation.

On Mon, 2023-09-11 at 11:29 +0200, AngeloGioacchino Del Regno wrote:
> Il 11/09/23 04:30, Yong Wu ha scritto:
> > The TEE probe later than dma-buf heap, and PROBE_DEDER doesn't work
> > here since this is not a platform driver, therefore initialise the
> > TEE
> > context/session while we allocate the first secure buffer.
> > 
> > Signed-off-by: Yong Wu <yong.wu@mediatek.com>
> > ---
> >   drivers/dma-buf/heaps/mtk_secure_heap.c | 61
> > +++++++++++++++++++++++++
> >   1 file changed, 61 insertions(+)
> > 
> > diff --git a/drivers/dma-buf/heaps/mtk_secure_heap.c b/drivers/dma-
> > buf/heaps/mtk_secure_heap.c
> > index bbf1c8dce23e..e3da33a3d083 100644
> > --- a/drivers/dma-buf/heaps/mtk_secure_heap.c
> > +++ b/drivers/dma-buf/heaps/mtk_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		"4477588a-8476-11e2-ad15-
> > e41f1390d676"
> > +
> 
> Is this UUID the same for all SoCs and all TZ versions?

Yes. It is the same for all SoCs and all TZ versions currently.

> 
> Thanks,
> Angelo
> 
> 
> > +#define MTK_TEE_PARAM_NUM		4
> >   
> >   /*
> >    * MediaTek secure (chunk) memory type
> > @@ -28,17 +34,72 @@ struct mtk_secure_heap_buffer {
> >   struct mtk_secure_heap {
> >   	const char		*name;
> >   	const enum kree_mem_type mem_type;
> > +	u32			 mem_session;
> > +	struct tee_context	*tee_ctx;
> >   };
> >   
> > +static int mtk_optee_ctx_match(struct tee_ioctl_version_data *ver,
> > const void *data)
> > +{
> > +	return ver->impl_id == TEE_IMPL_ID_OPTEE;
> > +}
> > +
> > +static int mtk_kree_secure_session_init(struct mtk_secure_heap
> > *sec_heap)
> > +{
> > +	struct tee_param t_param[MTK_TEE_PARAM_NUM] = {0};
> > +	struct tee_ioctl_open_session_arg arg = {0};
> > +	uuid_t ta_mem_uuid;
> > +	int ret;
> > +
> > +	sec_heap->tee_ctx = tee_client_open_context(NULL,
> > mtk_optee_ctx_match,
> > +						    NULL, NULL);
> > +	if (IS_ERR(sec_heap->tee_ctx)) {
> > +		pr_err("%s: open context failed, ret=%ld\n", sec_heap-
> > >name,
> > +		       PTR_ERR(sec_heap->tee_ctx));
> > +		return -ENODEV;
> > +	}
> > +
> > +	arg.num_params = MTK_TEE_PARAM_NUM;
> > +	arg.clnt_login = TEE_IOCTL_LOGIN_PUBLIC;
> > +	ret = uuid_parse(TZ_TA_MEM_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("%s: open session failed, ret=%d:%d\n",
> > +		       sec_heap->name, ret, arg.ret);
> > +		ret = -EINVAL;
> > +		goto close_context;
> > +	}
> > +	sec_heap->mem_session = arg.session;
> > +	return 0;
> > +
> > +close_context:
> > +	tee_client_close_context(sec_heap->tee_ctx);
> > +	return ret;
> > +}
> > +
> >   static struct dma_buf *
> >   mtk_sec_heap_allocate(struct dma_heap *heap, size_t size,
> >   		      unsigned long fd_flags, unsigned long heap_flags)
> >   {
> > +	struct mtk_secure_heap *sec_heap = dma_heap_get_drvdata(heap);
> >   	struct mtk_secure_heap_buffer *sec_buf;
> >   	DEFINE_DMA_BUF_EXPORT_INFO(exp_info);
> >   	struct dma_buf *dmabuf;
> >   	int ret;
> >   
> > +	/*
> > +	 * TEE probe may be late. Initialise the secure session in the
> > first
> > +	 * allocating secure buffer.
> > +	 */
> > +	if (!sec_heap->mem_session) {
> > +		ret = mtk_kree_secure_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);
> 
>

Am 11.09.23 um 20:29 schrieb John Stultz:
> On Mon, Sep 11, 2023 at 3:14 AM Christian König
> <christian.koenig@amd.com> wrote:
>> Am 11.09.23 um 04:30 schrieb Yong Wu:
>>> From: John Stultz <jstultz@google.com>
>>>
>>> This allows drivers who don't want to create their own
>>> DMA-BUF exporter to be able to allocate DMA-BUFs directly
>>> from existing DMA-BUF Heaps.
>>>
>>> There is some concern that the premise of DMA-BUF heaps is
>>> that userland knows better about what type of heap memory
>>> is needed for a pipeline, so it would likely be best for
>>> drivers to import and fill DMA-BUFs allocated by userland
>>> instead of allocating one themselves, but this is still
>>> up for debate.
>> The main design goal of having DMA-heaps in the first place is to avoid
>> per driver allocation and this is not necessary because userland know
>> better what type of memory it wants.
>>
>> The background is rather that we generally want to decouple allocation
>> from having a device driver connection so that we have better chance
>> that multiple devices can work with the same memory.
> Yep, very much agreed, and this is what the comment above is trying to describe.
>
> Ideally user-allocated buffers would be used to ensure driver's don't
> create buffers with constraints that limit which devices the buffers
> might later be shared with.
>
> However, this patch was created as a hold-over from the old ION logic
> to help vendors transition to dmabuf heaps, as vendors had situations
> where they still wanted to export dmabufs that were not to be
> generally shared and folks wanted to avoid duplication of logic
> already in existing heaps.  At the time, I never pushed it upstream as
> there were no upstream users.  But I think if there is now a potential
> upstream user, it's worth having the discussion to better understand
> the need.

Yeah, that indeed makes much more sense.

When existing drivers want to avoid their own handling and move their 
memory management over to using DMA-heaps even for internal allocations 
then no objections from my side. That is certainly something we should 
aim for if possible.

But what we should try to avoid is that newly merged drivers provide 
both a driver specific UAPI and DMA-heaps. The justification that this 
makes it easier to transit userspace to the new UAPI doesn't really count.

That would be adding UAPI already with a plan to deprecate it and that 
is most likely not helpful considering that UAPI must be supported 
forever as soon as it is upstream.

> So I think this patch is a little confusing in this series, as I don't
> see much of it actually being used here (though forgive me if I'm
> missing it).
>
> Instead, It seems it get used in a separate patch series here:
>    https://lore.kernel.org/all/20230911125936.10648-1-yunfei.dong@mediatek.com/

Please try to avoid stuff like that it is really confusing and eats 
reviewers time.

Regards,
Christian.

>
> Yong, I appreciate you sending this out! But maybe if the secure heap
> submission doesn't depend on this functionality, I might suggest
> moving this patch (or at least the majority of it) to be part of the
> vcodec series instead?  That way reviewers will have more context for
> how the code being added is used?
>
> thanks
> -john

On Mon, 2023-09-11 at 12:12 -0400, Nicolas Dufresne wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  Hi,
> 
> Le lundi 11 septembre 2023 à 10:30 +0800, Yong Wu a écrit :
> > From: John Stultz <jstultz@google.com>
> > 
> > This allows drivers who don't want to create their own
> > DMA-BUF exporter to be able to allocate DMA-BUFs directly
> > from existing DMA-BUF Heaps.
> > 
> > There is some concern that the premise of DMA-BUF heaps is
> > that userland knows better about what type of heap memory
> > is needed for a pipeline, so it would likely be best for
> > drivers to import and fill DMA-BUFs allocated by userland
> > instead of allocating one themselves, but this is still
> > up for debate.
> 
> 
> Would be nice for the reviewers to provide the information about the
> user of
> this new in-kernel API. I noticed it because I was CCed, but
> strangely it didn't
> make it to the mailing list yet and its not clear in the cover what
> this is used
> with. 
> 
> I can explain in my words though, my read is that this is used to
> allocate both
> user visible and driver internal memory segments in MTK VCODEC
> driver.
> 
> I'm somewhat concerned that DMABuf objects are used to abstract
> secure memory
> allocation from tee. For framebuffers that are going to be exported
> and shared
> its probably fair use, but it seems that internal shared memory and
> codec
> specific reference buffers also endup with a dmabuf fd (often called
> a secure fd
> in the v4l2 patchset) for data that is not being shared, and requires
> a 1:1
> mapping to a tee handle anyway. Is that the design we'd like to
> follow ? 

Yes. basically this is right.

> Can't
> we directly allocate from the tee, adding needed helper to make this
> as simple
> as allocating from a HEAP ?

If this happens, the memory will always be inside TEE. Here we create a
new _CMA heap, it will cma_alloc/free dynamically. Reserve it before
SVP start, and release to kernel after SVP done.
  
Secondly. the v4l2/drm has the mature driver control flow, like
drm_gem_prime_import_dev that always use dma_buf ops. So we can use the
current flow as much as possible without having to re-plan a flow in
the TEE.

> 
> Nicolas
> 
> > 
> > Signed-off-by: John Stultz <jstultz@google.com>
> > Signed-off-by: T.J. Mercier <tjmercier@google.com>
> > Signed-off-by: Yong Wu <yong.wu@mediatek.com>
> > [Yong: Fix the checkpatch alignment warning]
> > ---
> >  drivers/dma-buf/dma-heap.c | 60 ++++++++++++++++++++++++++++----
> ------
> >  include/linux/dma-heap.h   | 25 ++++++++++++++++
> >  2 files changed, 69 insertions(+), 16 deletions(-)
> > 
[snip]

On Tue, 2023-09-12 at 09:06 +0200, Christian König wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  Am 11.09.23 um 20:29 schrieb John Stultz:
> > On Mon, Sep 11, 2023 at 3:14 AM Christian König
> > <christian.koenig@amd.com> wrote:
> >> Am 11.09.23 um 04:30 schrieb Yong Wu:
> >>> From: John Stultz <jstultz@google.com>
> >>>
> >>> This allows drivers who don't want to create their own
> >>> DMA-BUF exporter to be able to allocate DMA-BUFs directly
> >>> from existing DMA-BUF Heaps.
> >>>
> >>> There is some concern that the premise of DMA-BUF heaps is
> >>> that userland knows better about what type of heap memory
> >>> is needed for a pipeline, so it would likely be best for
> >>> drivers to import and fill DMA-BUFs allocated by userland
> >>> instead of allocating one themselves, but this is still
> >>> up for debate.
> >> The main design goal of having DMA-heaps in the first place is to
> avoid
> >> per driver allocation and this is not necessary because userland
> know
> >> better what type of memory it wants.
> >>
> >> The background is rather that we generally want to decouple
> allocation
> >> from having a device driver connection so that we have better
> chance
> >> that multiple devices can work with the same memory.
> > Yep, very much agreed, and this is what the comment above is trying
> to describe.
> >
> > Ideally user-allocated buffers would be used to ensure driver's
> don't
> > create buffers with constraints that limit which devices the
> buffers
> > might later be shared with.
> >
> > However, this patch was created as a hold-over from the old ION
> logic
> > to help vendors transition to dmabuf heaps, as vendors had
> situations
> > where they still wanted to export dmabufs that were not to be
> > generally shared and folks wanted to avoid duplication of logic
> > already in existing heaps.  At the time, I never pushed it upstream
> as
> > there were no upstream users.  But I think if there is now a
> potential
> > upstream user, it's worth having the discussion to better
> understand
> > the need.
> 
> Yeah, that indeed makes much more sense.
> 
> When existing drivers want to avoid their own handling and move
> their 
> memory management over to using DMA-heaps even for internal
> allocations 
> then no objections from my side. That is certainly something we
> should 
> aim for if possible.

Thanks.

> 
> But what we should try to avoid is that newly merged drivers provide 
> both a driver specific UAPI and DMA-heaps. The justification that
> this 
> makes it easier to transit userspace to the new UAPI doesn't really
> count.
> 
> That would be adding UAPI already with a plan to deprecate it and
> that 
> is most likely not helpful considering that UAPI must be supported 
> forever as soon as it is upstream.

Sorry, I didn't understand this. I think we have not change the UAPI.
Which code are you referring to?

> 
> > So I think this patch is a little confusing in this series, as I
> don't
> > see much of it actually being used here (though forgive me if I'm
> > missing it).
> >
> > Instead, It seems it get used in a separate patch series here:
> >    
> https://lore.kernel.org/all/20230911125936.10648-1-yunfei.dong@mediatek.com/
> 
> Please try to avoid stuff like that it is really confusing and eats 
> reviewers time.

My fault, I thought dma-buf and media belonged to the different tree,
so I send them separately. The cover letter just said "The consumers of
the new heap and new interface are our codecs and DRM, which will be
sent upstream soon", and there was no vcodec link at that time.

In the next version, we will put the first three patches into the
vcodec patchset.

Thanks.

> 
> Regards,
> Christian.
> 
> >
> > Yong, I appreciate you sending this out! But maybe if the secure
> heap
> > submission doesn't depend on this functionality, I might suggest
> > moving this patch (or at least the majority of it) to be part of
> the
> > vcodec series instead?  That way reviewers will have more context
> for
> > how the code being added is used?

Will do.
Thanks.

> >
> > thanks
> > -john
>

Il 12/09/23 08:17, Yong Wu (吴勇) ha scritto:
> On Mon, 2023-09-11 at 11:29 +0200, AngeloGioacchino Del Regno wrote:
>> Il 11/09/23 04:30, Yong Wu ha scritto:
>>> The TEE probe later than dma-buf heap, and PROBE_DEDER doesn't work
>>> here since this is not a platform driver, therefore initialise the
>>> TEE
>>> context/session while we allocate the first secure buffer.
>>>
>>> Signed-off-by: Yong Wu <yong.wu@mediatek.com>
>>> ---
>>>    drivers/dma-buf/heaps/mtk_secure_heap.c | 61
>>> +++++++++++++++++++++++++
>>>    1 file changed, 61 insertions(+)
>>>
>>> diff --git a/drivers/dma-buf/heaps/mtk_secure_heap.c b/drivers/dma-
>>> buf/heaps/mtk_secure_heap.c
>>> index bbf1c8dce23e..e3da33a3d083 100644
>>> --- a/drivers/dma-buf/heaps/mtk_secure_heap.c
>>> +++ b/drivers/dma-buf/heaps/mtk_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		"4477588a-8476-11e2-ad15-
>>> e41f1390d676"
>>> +
>>
>> Is this UUID the same for all SoCs and all TZ versions?
> 
> Yes. It is the same for all SoCs and all TZ versions currently.
> 

That's good news!

Is this UUID used in any userspace component? (example: Android HALs?)
If it is (and I somehow expect that it is), then this definition should go
to a UAPI header, as suggested by Christian.

Cheers!

>>
>> Thanks,
>> Angelo
>>
>>
>>> +#define MTK_TEE_PARAM_NUM		4
>>>    
>>>    /*
>>>     * MediaTek secure (chunk) memory type
>>> @@ -28,17 +34,72 @@ struct mtk_secure_heap_buffer {
>>>    struct mtk_secure_heap {
>>>    	const char		*name;
>>>    	const enum kree_mem_type mem_type;
>>> +	u32			 mem_session;
>>> +	struct tee_context	*tee_ctx;
>>>    };
>>>    
>>> +static int mtk_optee_ctx_match(struct tee_ioctl_version_data *ver,
>>> const void *data)
>>> +{
>>> +	return ver->impl_id == TEE_IMPL_ID_OPTEE;
>>> +}
>>> +
>>> +static int mtk_kree_secure_session_init(struct mtk_secure_heap
>>> *sec_heap)
>>> +{
>>> +	struct tee_param t_param[MTK_TEE_PARAM_NUM] = {0};
>>> +	struct tee_ioctl_open_session_arg arg = {0};
>>> +	uuid_t ta_mem_uuid;
>>> +	int ret;
>>> +
>>> +	sec_heap->tee_ctx = tee_client_open_context(NULL,
>>> mtk_optee_ctx_match,
>>> +						    NULL, NULL);
>>> +	if (IS_ERR(sec_heap->tee_ctx)) {
>>> +		pr_err("%s: open context failed, ret=%ld\n", sec_heap-
>>>> name,
>>> +		       PTR_ERR(sec_heap->tee_ctx));
>>> +		return -ENODEV;
>>> +	}
>>> +
>>> +	arg.num_params = MTK_TEE_PARAM_NUM;
>>> +	arg.clnt_login = TEE_IOCTL_LOGIN_PUBLIC;
>>> +	ret = uuid_parse(TZ_TA_MEM_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("%s: open session failed, ret=%d:%d\n",
>>> +		       sec_heap->name, ret, arg.ret);
>>> +		ret = -EINVAL;
>>> +		goto close_context;
>>> +	}
>>> +	sec_heap->mem_session = arg.session;
>>> +	return 0;
>>> +
>>> +close_context:
>>> +	tee_client_close_context(sec_heap->tee_ctx);
>>> +	return ret;
>>> +}
>>> +
>>>    static struct dma_buf *
>>>    mtk_sec_heap_allocate(struct dma_heap *heap, size_t size,
>>>    		      unsigned long fd_flags, unsigned long heap_flags)
>>>    {
>>> +	struct mtk_secure_heap *sec_heap = dma_heap_get_drvdata(heap);
>>>    	struct mtk_secure_heap_buffer *sec_buf;
>>>    	DEFINE_DMA_BUF_EXPORT_INFO(exp_info);
>>>    	struct dma_buf *dmabuf;
>>>    	int ret;
>>>    
>>> +	/*
>>> +	 * TEE probe may be late. Initialise the secure session in the
>>> first
>>> +	 * allocating secure buffer.
>>> +	 */
>>> +	if (!sec_heap->mem_session) {
>>> +		ret = mtk_kree_secure_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);
>>
>>

Am 12.09.23 um 10:52 schrieb Yong Wu (吴勇):
> [SNIP]
>> But what we should try to avoid is that newly merged drivers provide
>> both a driver specific UAPI and DMA-heaps. The justification that
>> this
>> makes it easier to transit userspace to the new UAPI doesn't really
>> count.
>>
>> That would be adding UAPI already with a plan to deprecate it and
>> that
>> is most likely not helpful considering that UAPI must be supported
>> forever as soon as it is upstream.
> Sorry, I didn't understand this. I think we have not change the UAPI.
> Which code are you referring to?

Well, what do you need this for if not a new UAPI?

My assumption here is that you need to export the DMA-heap allocation 
function so that you can server an UAPI in your new driver. Or what else 
is that good for?

As far as I understand you try to upstream your new vcodec driver. So 
while this change here seems to be a good idea to clean up existing 
drivers it doesn't look like a good idea for a newly created driver.

Regards,
Christian.

>>> So I think this patch is a little confusing in this series, as I
>> don't
>>> see much of it actually being used here (though forgive me if I'm
>>> missing it).
>>>
>>> Instead, It seems it get used in a separate patch series here:
>>>     
>> https://lore.kernel.org/all/20230911125936.10648-1-yunfei.dong@mediatek.com/
>>
>> Please try to avoid stuff like that it is really confusing and eats
>> reviewers time.
> My fault, I thought dma-buf and media belonged to the different tree,
> so I send them separately. The cover letter just said "The consumers of
> the new heap and new interface are our codecs and DRM, which will be
> sent upstream soon", and there was no vcodec link at that time.
>
> In the next version, we will put the first three patches into the
> vcodec patchset.
>
> Thanks.
>

Le lundi 11 septembre 2023 à 12:13 +0200, Christian König a écrit :
> Am 11.09.23 um 04:30 schrieb Yong Wu:
> > From: John Stultz <jstultz@google.com>
> > 
> > This allows drivers who don't want to create their own
> > DMA-BUF exporter to be able to allocate DMA-BUFs directly
> > from existing DMA-BUF Heaps.
> > 
> > There is some concern that the premise of DMA-BUF heaps is
> > that userland knows better about what type of heap memory
> > is needed for a pipeline, so it would likely be best for
> > drivers to import and fill DMA-BUFs allocated by userland
> > instead of allocating one themselves, but this is still
> > up for debate.
> 
> The main design goal of having DMA-heaps in the first place is to avoid 
> per driver allocation and this is not necessary because userland know 
> better what type of memory it wants.

If the memory is user visible, yes. When I look at the MTK VCODEC changes, this
seems to be used for internal codec state and SHM buffers used to communicate
with firmware.

> 
> The background is rather that we generally want to decouple allocation 
> from having a device driver connection so that we have better chance 
> that multiple devices can work with the same memory.
> 
> I once create a prototype which gives userspace a hint which DMA-heap to 
> user for which device: 
> https://patchwork.kernel.org/project/linux-media/patch/20230123123756.401692-2-christian.koenig@amd.com/
> 
> Problem is that I don't really have time to look into it and maintain 
> that stuff, but I think from the high level design that is rather the 
> general direction we should push at.
> 
> Regards,
> Christian.
> 
> > 
> > Signed-off-by: John Stultz <jstultz@google.com>
> > Signed-off-by: T.J. Mercier <tjmercier@google.com>
> > Signed-off-by: Yong Wu <yong.wu@mediatek.com>
> > [Yong: Fix the checkpatch alignment warning]
> > ---
> >   drivers/dma-buf/dma-heap.c | 60 ++++++++++++++++++++++++++++----------
> >   include/linux/dma-heap.h   | 25 ++++++++++++++++
> >   2 files changed, 69 insertions(+), 16 deletions(-)
> > 
> > diff --git a/drivers/dma-buf/dma-heap.c b/drivers/dma-buf/dma-heap.c
> > index dcc0e38c61fa..908bb30dc864 100644
> > --- a/drivers/dma-buf/dma-heap.c
> > +++ b/drivers/dma-buf/dma-heap.c
> > @@ -53,12 +53,15 @@ static dev_t dma_heap_devt;
> >   static struct class *dma_heap_class;
> >   static DEFINE_XARRAY_ALLOC(dma_heap_minors);
> >   
> > -static int dma_heap_buffer_alloc(struct dma_heap *heap, size_t len,
> > -				 unsigned int fd_flags,
> > -				 unsigned int heap_flags)
> > +struct dma_buf *dma_heap_buffer_alloc(struct dma_heap *heap, size_t len,
> > +				      unsigned int fd_flags,
> > +				      unsigned int heap_flags)
> >   {
> > -	struct dma_buf *dmabuf;
> > -	int fd;
> > +	if (fd_flags & ~DMA_HEAP_VALID_FD_FLAGS)
> > +		return ERR_PTR(-EINVAL);
> > +
> > +	if (heap_flags & ~DMA_HEAP_VALID_HEAP_FLAGS)
> > +		return ERR_PTR(-EINVAL);
> >   
> >   	/*
> >   	 * Allocations from all heaps have to begin
> > @@ -66,9 +69,20 @@ static int dma_heap_buffer_alloc(struct dma_heap *heap, size_t len,
> >   	 */
> >   	len = PAGE_ALIGN(len);
> >   	if (!len)
> > -		return -EINVAL;
> > +		return ERR_PTR(-EINVAL);
> >   
> > -	dmabuf = heap->ops->allocate(heap, len, fd_flags, heap_flags);
> > +	return heap->ops->allocate(heap, len, fd_flags, heap_flags);
> > +}
> > +EXPORT_SYMBOL_GPL(dma_heap_buffer_alloc);
> > +
> > +static int dma_heap_bufferfd_alloc(struct dma_heap *heap, size_t len,
> > +				   unsigned int fd_flags,
> > +				   unsigned int heap_flags)
> > +{
> > +	struct dma_buf *dmabuf;
> > +	int fd;
> > +
> > +	dmabuf = dma_heap_buffer_alloc(heap, len, fd_flags, heap_flags);
> >   	if (IS_ERR(dmabuf))
> >   		return PTR_ERR(dmabuf);
> >   
> > @@ -106,15 +120,9 @@ static long dma_heap_ioctl_allocate(struct file *file, void *data)
> >   	if (heap_allocation->fd)
> >   		return -EINVAL;
> >   
> > -	if (heap_allocation->fd_flags & ~DMA_HEAP_VALID_FD_FLAGS)
> > -		return -EINVAL;
> > -
> > -	if (heap_allocation->heap_flags & ~DMA_HEAP_VALID_HEAP_FLAGS)
> > -		return -EINVAL;
> > -
> > -	fd = dma_heap_buffer_alloc(heap, heap_allocation->len,
> > -				   heap_allocation->fd_flags,
> > -				   heap_allocation->heap_flags);
> > +	fd = dma_heap_bufferfd_alloc(heap, heap_allocation->len,
> > +				     heap_allocation->fd_flags,
> > +				     heap_allocation->heap_flags);
> >   	if (fd < 0)
> >   		return fd;
> >   
> > @@ -205,6 +213,7 @@ const char *dma_heap_get_name(struct dma_heap *heap)
> >   {
> >   	return heap->name;
> >   }
> > +EXPORT_SYMBOL_GPL(dma_heap_get_name);
> >   
> >   struct dma_heap *dma_heap_add(const struct dma_heap_export_info *exp_info)
> >   {
> > @@ -290,6 +299,24 @@ struct dma_heap *dma_heap_add(const struct dma_heap_export_info *exp_info)
> >   	kfree(heap);
> >   	return err_ret;
> >   }
> > +EXPORT_SYMBOL_GPL(dma_heap_add);
> > +
> > +struct dma_heap *dma_heap_find(const char *name)
> > +{
> > +	struct dma_heap *h;
> > +
> > +	mutex_lock(&heap_list_lock);
> > +	list_for_each_entry(h, &heap_list, list) {
> > +		if (!strcmp(h->name, name)) {
> > +			kref_get(&h->refcount);
> > +			mutex_unlock(&heap_list_lock);
> > +			return h;
> > +		}
> > +	}
> > +	mutex_unlock(&heap_list_lock);
> > +	return NULL;
> > +}
> > +EXPORT_SYMBOL_GPL(dma_heap_find);
> >   
> >   static void dma_heap_release(struct kref *ref)
> >   {
> > @@ -315,6 +342,7 @@ void dma_heap_put(struct dma_heap *h)
> >   	kref_put(&h->refcount, dma_heap_release);
> >   	mutex_unlock(&heap_list_lock);
> >   }
> > +EXPORT_SYMBOL_GPL(dma_heap_put);
> >   
> >   static char *dma_heap_devnode(const struct device *dev, umode_t *mode)
> >   {
> > diff --git a/include/linux/dma-heap.h b/include/linux/dma-heap.h
> > index f3c678892c5c..59e70f6c7a60 100644
> > --- a/include/linux/dma-heap.h
> > +++ b/include/linux/dma-heap.h
> > @@ -64,10 +64,35 @@ const char *dma_heap_get_name(struct dma_heap *heap);
> >    */
> >   struct dma_heap *dma_heap_add(const struct dma_heap_export_info *exp_info);
> >   
> > +/**
> > + * dma_heap_find - get the heap registered with the specified name
> > + * @name: Name of the DMA-Heap to find
> > + *
> > + * Returns:
> > + * The DMA-Heap with the provided name.
> > + *
> > + * NOTE: DMA-Heaps returned from this function MUST be released using
> > + * dma_heap_put() when the user is done to enable the heap to be unloaded.
> > + */
> > +struct dma_heap *dma_heap_find(const char *name);
> > +
> >   /**
> >    * dma_heap_put - drops a reference to a dmabuf heap, potentially freeing it
> >    * @heap: the heap whose reference count to decrement
> >    */
> >   void dma_heap_put(struct dma_heap *heap);
> >   
> > +/**
> > + * dma_heap_buffer_alloc - Allocate dma-buf from a dma_heap
> > + * @heap:	DMA-Heap to allocate from
> > + * @len:	size to allocate in bytes
> > + * @fd_flags:	flags to set on returned dma-buf fd
> > + * @heap_flags: flags to pass to the dma heap
> > + *
> > + * This is for internal dma-buf allocations only. Free returned buffers with dma_buf_put().
> > + */
> > +struct dma_buf *dma_heap_buffer_alloc(struct dma_heap *heap, size_t len,
> > +				      unsigned int fd_flags,
> > +				      unsigned int heap_flags);
> > +
> >   #endif /* _DMA_HEAPS_H */
>

Le mardi 12 septembre 2023 à 16:46 +0200, Christian König a écrit :
> Am 12.09.23 um 10:52 schrieb Yong Wu (吴勇):
> > [SNIP]
> > > But what we should try to avoid is that newly merged drivers provide
> > > both a driver specific UAPI and DMA-heaps. The justification that
> > > this
> > > makes it easier to transit userspace to the new UAPI doesn't really
> > > count.
> > > 
> > > That would be adding UAPI already with a plan to deprecate it and
> > > that
> > > is most likely not helpful considering that UAPI must be supported
> > > forever as soon as it is upstream.
> > Sorry, I didn't understand this. I think we have not change the UAPI.
> > Which code are you referring to?
> 
> Well, what do you need this for if not a new UAPI?
> 
> My assumption here is that you need to export the DMA-heap allocation 
> function so that you can server an UAPI in your new driver. Or what else 
> is that good for?
> 
> As far as I understand you try to upstream your new vcodec driver. So 
> while this change here seems to be a good idea to clean up existing 
> drivers it doesn't look like a good idea for a newly created driver.

MTK VCODEC has been upstream for quite some time now. The other patchset is
trying to add secure decoding/encoding support to that existing upstream driver.

Regarding the uAPI, it seems that this addition to dmabuf heap internal API is
exactly the opposite. By making heaps available to drivers, modification to the
V4L2 uAPI is being reduced to adding "SECURE_MODE" + "SECURE_HEAP_ID" controls
(this is not debated yet has an approach). The heaps is being used internally in
replacement to every allocation, user visible or not.

Nicolas

> 
> Regards,
> Christian.
> 
> > > > So I think this patch is a little confusing in this series, as I
> > > don't
> > > > see much of it actually being used here (though forgive me if I'm
> > > > missing it).
> > > > 
> > > > Instead, It seems it get used in a separate patch series here:
> > > >     
> > > https://lore.kernel.org/all/20230911125936.10648-1-yunfei.dong@mediatek.com/
> > > 
> > > Please try to avoid stuff like that it is really confusing and eats
> > > reviewers time.
> > My fault, I thought dma-buf and media belonged to the different tree,
> > so I send them separately. The cover letter just said "The consumers of
> > the new heap and new interface are our codecs and DRM, which will be
> > sent upstream soon", and there was no vcodec link at that time.
> > 
> > In the next version, we will put the first three patches into the
> > vcodec patchset.
> > 
> > Thanks.
> > 
>

Le mardi 12 septembre 2023 à 08:47 +0000, Yong Wu (吴勇) a écrit :
> On Mon, 2023-09-11 at 12:12 -0400, Nicolas Dufresne wrote:
> >  	 
> > External email : Please do not click links or open attachments until
> > you have verified the sender or the content.
> >  Hi,
> > 
> > Le lundi 11 septembre 2023 à 10:30 +0800, Yong Wu a écrit :
> > > From: John Stultz <jstultz@google.com>
> > > 
> > > This allows drivers who don't want to create their own
> > > DMA-BUF exporter to be able to allocate DMA-BUFs directly
> > > from existing DMA-BUF Heaps.
> > > 
> > > There is some concern that the premise of DMA-BUF heaps is
> > > that userland knows better about what type of heap memory
> > > is needed for a pipeline, so it would likely be best for
> > > drivers to import and fill DMA-BUFs allocated by userland
> > > instead of allocating one themselves, but this is still
> > > up for debate.
> > 
> > 
> > Would be nice for the reviewers to provide the information about the
> > user of
> > this new in-kernel API. I noticed it because I was CCed, but
> > strangely it didn't
> > make it to the mailing list yet and its not clear in the cover what
> > this is used
> > with. 
> > 
> > I can explain in my words though, my read is that this is used to
> > allocate both
> > user visible and driver internal memory segments in MTK VCODEC
> > driver.
> > 
> > I'm somewhat concerned that DMABuf objects are used to abstract
> > secure memory
> > allocation from tee. For framebuffers that are going to be exported
> > and shared
> > its probably fair use, but it seems that internal shared memory and
> > codec
> > specific reference buffers also endup with a dmabuf fd (often called
> > a secure fd
> > in the v4l2 patchset) for data that is not being shared, and requires
> > a 1:1
> > mapping to a tee handle anyway. Is that the design we'd like to
> > follow ? 
> 
> Yes. basically this is right.
> 
> > Can't
> > we directly allocate from the tee, adding needed helper to make this
> > as simple
> > as allocating from a HEAP ?
> 
> If this happens, the memory will always be inside TEE. Here we create a
> new _CMA heap, it will cma_alloc/free dynamically. Reserve it before
> SVP start, and release to kernel after SVP done.

Ok, I see the benefit of having a common driver then. It would add to the
complexity, but having a driver for the tee allocator and v4l2/heaps would be
another option?

>   
> Secondly. the v4l2/drm has the mature driver control flow, like
> drm_gem_prime_import_dev that always use dma_buf ops. So we can use the
> current flow as much as possible without having to re-plan a flow in
> the TEE.

From what I've read from Yunfei series, this is only partially true for V4L2.
The vb2 queue MMAP feature have dmabuf exportation as optional, but its not a
problem to always back it up with a dmabuf object. But for internal SHM buffers
used for firmware communication, I've never seen any driver use a DMABuf.

Same applies for primary decode buffers when frame buffer compression or post-
processing it used (or reconstruction buffer in encoders), these are not user
visible and are usually not DMABuf.

> 
> > 
> > Nicolas
> > 
> > > 
> > > Signed-off-by: John Stultz <jstultz@google.com>
> > > Signed-off-by: T.J. Mercier <tjmercier@google.com>
> > > Signed-off-by: Yong Wu <yong.wu@mediatek.com>
> > > [Yong: Fix the checkpatch alignment warning]
> > > ---
> > >  drivers/dma-buf/dma-heap.c | 60 ++++++++++++++++++++++++++++----
> > ------
> > >  include/linux/dma-heap.h   | 25 ++++++++++++++++
> > >  2 files changed, 69 insertions(+), 16 deletions(-)
> > > 
> [snip]

Am 12.09.23 um 16:58 schrieb Nicolas Dufresne:
> Le mardi 12 septembre 2023 à 16:46 +0200, Christian König a écrit :
>> Am 12.09.23 um 10:52 schrieb Yong Wu (吴勇):
>>> [SNIP]
>>>> But what we should try to avoid is that newly merged drivers provide
>>>> both a driver specific UAPI and DMA-heaps. The justification that
>>>> this
>>>> makes it easier to transit userspace to the new UAPI doesn't really
>>>> count.
>>>>
>>>> That would be adding UAPI already with a plan to deprecate it and
>>>> that
>>>> is most likely not helpful considering that UAPI must be supported
>>>> forever as soon as it is upstream.
>>> Sorry, I didn't understand this. I think we have not change the UAPI.
>>> Which code are you referring to?
>> Well, what do you need this for if not a new UAPI?
>>
>> My assumption here is that you need to export the DMA-heap allocation
>> function so that you can server an UAPI in your new driver. Or what else
>> is that good for?
>>
>> As far as I understand you try to upstream your new vcodec driver. So
>> while this change here seems to be a good idea to clean up existing
>> drivers it doesn't look like a good idea for a newly created driver.
> MTK VCODEC has been upstream for quite some time now. The other patchset is
> trying to add secure decoding/encoding support to that existing upstream driver.
>
> Regarding the uAPI, it seems that this addition to dmabuf heap internal API is
> exactly the opposite. By making heaps available to drivers, modification to the
> V4L2 uAPI is being reduced to adding "SECURE_MODE" + "SECURE_HEAP_ID" controls
> (this is not debated yet has an approach). The heaps is being used internally in
> replacement to every allocation, user visible or not.

Thanks a lot for that explanation, I was really wondering what the use 
case for this is if it's not to serve new UAPI.

In this case I don't see any reason why we shouldn't do it. It's indeed 
much cleaner.

Christian.

>
> Nicolas
>
>> Regards,
>> Christian.
>>
>>>>> So I think this patch is a little confusing in this series, as I
>>>> don't
>>>>> see much of it actually being used here (though forgive me if I'm
>>>>> missing it).
>>>>>
>>>>> Instead, It seems it get used in a separate patch series here:
>>>>>      
>>>> https://lore.kernel.org/all/20230911125936.10648-1-yunfei.dong@mediatek.com/
>>>>
>>>> Please try to avoid stuff like that it is really confusing and eats
>>>> reviewers time.
>>> My fault, I thought dma-buf and media belonged to the different tree,
>>> so I send them separately. The cover letter just said "The consumers of
>>> the new heap and new interface are our codecs and DRM, which will be
>>> sent upstream soon", and there was no vcodec link at that time.
>>>
>>> In the next version, we will put the first three patches into the
>>> vcodec patchset.
>>>
>>> Thanks.
>>>

On Tue, 2023-09-12 at 11:05 -0400, Nicolas Dufresne wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  Le mardi 12 septembre 2023 à 08:47 +0000, Yong Wu (吴勇) a écrit :
> > On Mon, 2023-09-11 at 12:12 -0400, Nicolas Dufresne wrote:
> > >   
> > > External email : Please do not click links or open attachments
> until
> > > you have verified the sender or the content.
> > >  Hi,
> > > 
> > > Le lundi 11 septembre 2023 à 10:30 +0800, Yong Wu a écrit :
> > > > From: John Stultz <jstultz@google.com>
> > > > 
> > > > This allows drivers who don't want to create their own
> > > > DMA-BUF exporter to be able to allocate DMA-BUFs directly
> > > > from existing DMA-BUF Heaps.
> > > > 
> > > > There is some concern that the premise of DMA-BUF heaps is
> > > > that userland knows better about what type of heap memory
> > > > is needed for a pipeline, so it would likely be best for
> > > > drivers to import and fill DMA-BUFs allocated by userland
> > > > instead of allocating one themselves, but this is still
> > > > up for debate.
> > > 
> > > 
> > > Would be nice for the reviewers to provide the information about
> the
> > > user of
> > > this new in-kernel API. I noticed it because I was CCed, but
> > > strangely it didn't
> > > make it to the mailing list yet and its not clear in the cover
> what
> > > this is used
> > > with. 
> > > 
> > > I can explain in my words though, my read is that this is used to
> > > allocate both
> > > user visible and driver internal memory segments in MTK VCODEC
> > > driver.
> > > 
> > > I'm somewhat concerned that DMABuf objects are used to abstract
> > > secure memory
> > > allocation from tee. For framebuffers that are going to be
> exported
> > > and shared
> > > its probably fair use, but it seems that internal shared memory
> and
> > > codec
> > > specific reference buffers also endup with a dmabuf fd (often
> called
> > > a secure fd
> > > in the v4l2 patchset) for data that is not being shared, and
> requires
> > > a 1:1
> > > mapping to a tee handle anyway. Is that the design we'd like to
> > > follow ? 
> > 
> > Yes. basically this is right.
> > 
> > > Can't
> > > we directly allocate from the tee, adding needed helper to make
> this
> > > as simple
> > > as allocating from a HEAP ?
> > 
> > If this happens, the memory will always be inside TEE. Here we
> create a
> > new _CMA heap, it will cma_alloc/free dynamically. Reserve it
> before
> > SVP start, and release to kernel after SVP done.
> 
> Ok, I see the benefit of having a common driver then. It would add to
> the
> complexity, but having a driver for the tee allocator and v4l2/heaps
> would be
> another option?

It's ok for v4l2. But our DRM also use this new heap and it will be
sent upstream in the next few days.

> 
> >   
> > Secondly. the v4l2/drm has the mature driver control flow, like
> > drm_gem_prime_import_dev that always use dma_buf ops. So we can use
> the
> > current flow as much as possible without having to re-plan a flow
> in
> > the TEE.
> 
> From what I've read from Yunfei series, this is only partially true
> for V4L2.
> The vb2 queue MMAP feature have dmabuf exportation as optional, but
> its not a
> problem to always back it up with a dmabuf object. But for internal
> SHM buffers
> used for firmware communication, I've never seen any driver use a
> DMABuf.
> 
> Same applies for primary decode buffers when frame buffer compression
> or post-
> processing it used (or reconstruction buffer in encoders), these are
> not user
> visible and are usually not DMABuf.

If they aren't dmabuf, of course it is ok. I guess we haven't used
these. The SHM buffer is got by tee_shm_register_kernel_buf in this
case and we just use the existed dmabuf ops to complete SVP.

In our case, the vcodec input/output/working buffers and DRM input
buffer all use this new secure heap during secure video play.

> 
> > 
> > > 
> > > Nicolas
> > > 
> > > > 
> > > > Signed-off-by: John Stultz <jstultz@google.com>
> > > > Signed-off-by: T.J. Mercier <tjmercier@google.com>
> > > > Signed-off-by: Yong Wu <yong.wu@mediatek.com>
> > > > [Yong: Fix the checkpatch alignment warning]
> > > > ---
> > > >  drivers/dma-buf/dma-heap.c | 60 ++++++++++++++++++++++++++++
> ----
> > > ------
> > > >  include/linux/dma-heap.h   | 25 ++++++++++++++++
> > > >  2 files changed, 69 insertions(+), 16 deletions(-)
> > > > 
> > [snip]
>

On Mon, Sep 11, 2023 at 2:49 AM Christian König
<christian.koenig@amd.com> wrote:
>
> Am 11.09.23 um 04:30 schrieb Yong Wu:
> > From: John Stultz <jstultz@google.com>
> >
> > Add proper refcounting on the dma_heap structure.
> > While existing heaps are built-in, we may eventually
> > have heaps loaded from modules, and we'll need to be
> > able to properly handle the references to the heaps
> >
> > Also moves minor tracking into the heap structure so
> > we can properly free things.
>
> This is completely unnecessary, see below.
>
> >
> > Signed-off-by: John Stultz <jstultz@google.com>
> > Signed-off-by: T.J. Mercier <tjmercier@google.com>
> > Signed-off-by: Yong Wu <yong.wu@mediatek.com>
> > [Yong: Just add comment for "minor" and "refcount"]
> > ---
> >   drivers/dma-buf/dma-heap.c | 38 ++++++++++++++++++++++++++++++++++----
> >   include/linux/dma-heap.h   |  6 ++++++
> >   2 files changed, 40 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/dma-buf/dma-heap.c b/drivers/dma-buf/dma-heap.c
> > index 51030f6c9d6e..dcc0e38c61fa 100644
> > --- a/drivers/dma-buf/dma-heap.c
> > +++ b/drivers/dma-buf/dma-heap.c
> > @@ -11,6 +11,7 @@
> >   #include <linux/dma-buf.h>
> >   #include <linux/dma-heap.h>
> >   #include <linux/err.h>
> > +#include <linux/kref.h>
> >   #include <linux/list.h>
> >   #include <linux/nospec.h>
> >   #include <linux/syscalls.h>
> > @@ -30,6 +31,8 @@
> >    * @heap_devt:              heap device node
> >    * @list:           list head connecting to list of heaps
> >    * @heap_cdev:              heap char device
> > + * @minor:           heap device node minor number
> > + * @refcount:                reference counter for this heap device
> >    *
> >    * Represents a heap of memory from which buffers can be made.
> >    */
> > @@ -40,6 +43,8 @@ struct dma_heap {
> >       dev_t heap_devt;
> >       struct list_head list;
> >       struct cdev heap_cdev;
> > +     int minor;
> > +     struct kref refcount;
> >   };
> >
> >   static LIST_HEAD(heap_list);
> > @@ -205,7 +210,6 @@ struct dma_heap *dma_heap_add(const struct dma_heap_export_info *exp_info)
> >   {
> >       struct dma_heap *heap, *h, *err_ret;
> >       struct device *dev_ret;
> > -     unsigned int minor;
> >       int ret;
> >
> >       if (!exp_info->name || !strcmp(exp_info->name, "")) {
> > @@ -222,12 +226,13 @@ struct dma_heap *dma_heap_add(const struct dma_heap_export_info *exp_info)
> >       if (!heap)
> >               return ERR_PTR(-ENOMEM);
> >
> > +     kref_init(&heap->refcount);
> >       heap->name = exp_info->name;
> >       heap->ops = exp_info->ops;
> >       heap->priv = exp_info->priv;
> >
> >       /* Find unused minor number */
> > -     ret = xa_alloc(&dma_heap_minors, &minor, heap,
> > +     ret = xa_alloc(&dma_heap_minors, &heap->minor, heap,
> >                      XA_LIMIT(0, NUM_HEAP_MINORS - 1), GFP_KERNEL);
> >       if (ret < 0) {
> >               pr_err("dma_heap: Unable to get minor number for heap\n");
> > @@ -236,7 +241,7 @@ struct dma_heap *dma_heap_add(const struct dma_heap_export_info *exp_info)
> >       }
> >
> >       /* Create device */
> > -     heap->heap_devt = MKDEV(MAJOR(dma_heap_devt), minor);
> > +     heap->heap_devt = MKDEV(MAJOR(dma_heap_devt), heap->minor);
> >
> >       cdev_init(&heap->heap_cdev, &dma_heap_fops);
> >       ret = cdev_add(&heap->heap_cdev, heap->heap_devt, 1);
> > @@ -280,12 +285,37 @@ struct dma_heap *dma_heap_add(const struct dma_heap_export_info *exp_info)
> >   err2:
> >       cdev_del(&heap->heap_cdev);
> >   err1:
> > -     xa_erase(&dma_heap_minors, minor);
> > +     xa_erase(&dma_heap_minors, heap->minor);
> >   err0:
> >       kfree(heap);
> >       return err_ret;
> >   }
> >
> > +static void dma_heap_release(struct kref *ref)
> > +{
> > +     struct dma_heap *heap = container_of(ref, struct dma_heap, refcount);
> > +
> > +     /* Note, we already holding the heap_list_lock here */
> > +     list_del(&heap->list);
> > +
> > +     device_destroy(dma_heap_class, heap->heap_devt);
> > +     cdev_del(&heap->heap_cdev);
> > +     xa_erase(&dma_heap_minors, heap->minor);
>
> You can just use MINOR(heap->heap_devt) here instead.
>
Got it, thanks.

> > +
> > +     kfree(heap);
> > +}
> > +
> > +void dma_heap_put(struct dma_heap *h)
> > +{
> > +     /*
> > +      * Take the heap_list_lock now to avoid racing with code
> > +      * scanning the list and then taking a kref.
> > +      */
>
> This is usually considered a bad idea since it makes the kref approach
> superfluous.
>
> There are multiple possibilities how handle this, the most common one is
> to use kref_get_unless_zero() in your list traversal code and ignore the
> entry when that fails.
>
> Alternatively you could use kref_put_mutex() instead. This gives you the
> same functionality as this here, but as far as I know it's normally only
> used in a couple of special cases.
>
Ok, I'll move this mutex acquisition to dma_heap_release so that it
guards just the list_del, and change dma_heap_find to use
kref_get_unless_zero. Thanks.

> > +     mutex_lock(&heap_list_lock);
> > +     kref_put(&h->refcount, dma_heap_release);
> > +     mutex_unlock(&heap_list_lock);
> > +}
> > +
> >   static char *dma_heap_devnode(const struct device *dev, umode_t *mode)
> >   {
> >       return kasprintf(GFP_KERNEL, "dma_heap/%s", dev_name(dev));
> > diff --git a/include/linux/dma-heap.h b/include/linux/dma-heap.h
> > index c7c29b724ad6..f3c678892c5c 100644
> > --- a/include/linux/dma-heap.h
> > +++ b/include/linux/dma-heap.h
> > @@ -64,4 +64,10 @@ const char *dma_heap_get_name(struct dma_heap *heap);
> >    */
> >   struct dma_heap *dma_heap_add(const struct dma_heap_export_info *exp_info);
> >
> > +/**
> > + * dma_heap_put - drops a reference to a dmabuf heap, potentially freeing it
> > + * @heap: the heap whose reference count to decrement
> > + */
>
> Please don't add kerneldoc to the definition, add it to the
> implementation of the function.
>
Will fix.




> Regards,
> Christian.
>
> > +void dma_heap_put(struct dma_heap *heap);
> > +
> >   #endif /* _DMA_HEAPS_H */
>

On Tue, 2023-09-12 at 11:32 +0200, AngeloGioacchino Del Regno wrote:
> Il 12/09/23 08:17, Yong Wu (吴勇) ha scritto:
> > On Mon, 2023-09-11 at 11:29 +0200, AngeloGioacchino Del Regno
> > wrote:
> > > Il 11/09/23 04:30, Yong Wu ha scritto:
> > > > The TEE probe later than dma-buf heap, and PROBE_DEDER doesn't
> > > > work
> > > > here since this is not a platform driver, therefore initialise
> > > > the
> > > > TEE
> > > > context/session while we allocate the first secure buffer.
> > > > 
> > > > Signed-off-by: Yong Wu <yong.wu@mediatek.com>
> > > > ---
> > > >    drivers/dma-buf/heaps/mtk_secure_heap.c | 61
> > > > +++++++++++++++++++++++++
> > > >    1 file changed, 61 insertions(+)
> > > > 
> > > > diff --git a/drivers/dma-buf/heaps/mtk_secure_heap.c
> > > > b/drivers/dma-
> > > > buf/heaps/mtk_secure_heap.c
> > > > index bbf1c8dce23e..e3da33a3d083 100644
> > > > --- a/drivers/dma-buf/heaps/mtk_secure_heap.c
> > > > +++ b/drivers/dma-buf/heaps/mtk_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		"4477588a-8476-11e2-ad15-
> > > > e41f1390d676"
> > > > +
> > > 
> > > Is this UUID the same for all SoCs and all TZ versions?
> > 
> > Yes. It is the same for all SoCs and all TZ versions currently.
> > 
> 
> That's good news!
> 
> Is this UUID used in any userspace component? (example: Android
> HALs?)

No. Userspace never use it. If userspace would like to allocate this
secure buffer, it can achieve through the existing dmabuf IOCTL via
/dev/dma_heap/mtk_svp node.


> If it is (and I somehow expect that it is), then this definition
> should go
> to a UAPI header, as suggested by Christian.
> 
> Cheers!
> 
> > > 
> > > Thanks,
> > > Angelo
> > > 
> > > 
> > > > +#define MTK_TEE_PARAM_NUM		4
> > > >    
> > > >    /*
> > > >     * MediaTek secure (chunk) memory type
> > > > @@ -28,17 +34,72 @@ struct mtk_secure_heap_buffer {
> > > >    struct mtk_secure_heap {
> > > >    	const char		*name;
> > > >    	const enum kree_mem_type mem_type;
> > > > +	u32			 mem_session;
> > > > +	struct tee_context	*tee_ctx;
> > > >    };
> > > >    
> > > > +static int mtk_optee_ctx_match(struct tee_ioctl_version_data
> > > > *ver,
> > > > const void *data)
> > > > +{
> > > > +	return ver->impl_id == TEE_IMPL_ID_OPTEE;
> > > > +}
> > > > +
> > > > +static int mtk_kree_secure_session_init(struct mtk_secure_heap
> > > > *sec_heap)
> > > > +{
> > > > +	struct tee_param t_param[MTK_TEE_PARAM_NUM] = {0};
> > > > +	struct tee_ioctl_open_session_arg arg = {0};
> > > > +	uuid_t ta_mem_uuid;
> > > > +	int ret;
> > > > +
> > > > +	sec_heap->tee_ctx = tee_client_open_context(NULL,
> > > > mtk_optee_ctx_match,
> > > > +						    NULL,
> > > > NULL);
> > > > +	if (IS_ERR(sec_heap->tee_ctx)) {
> > > > +		pr_err("%s: open context failed, ret=%ld\n",
> > > > sec_heap-
> > > > > name,
> > > > 
> > > > +		       PTR_ERR(sec_heap->tee_ctx));
> > > > +		return -ENODEV;
> > > > +	}
> > > > +
> > > > +	arg.num_params = MTK_TEE_PARAM_NUM;
> > > > +	arg.clnt_login = TEE_IOCTL_LOGIN_PUBLIC;
> > > > +	ret = uuid_parse(TZ_TA_MEM_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("%s: open session failed, ret=%d:%d\n",
> > > > +		       sec_heap->name, ret, arg.ret);
> > > > +		ret = -EINVAL;
> > > > +		goto close_context;
> > > > +	}
> > > > +	sec_heap->mem_session = arg.session;
> > > > +	return 0;
> > > > +
> > > > +close_context:
> > > > +	tee_client_close_context(sec_heap->tee_ctx);
> > > > +	return ret;
> > > > +}
> > > > +
> > > >    static struct dma_buf *
> > > >    mtk_sec_heap_allocate(struct dma_heap *heap, size_t size,
> > > >    		      unsigned long fd_flags, unsigned long
> > > > heap_flags)
> > > >    {
> > > > +	struct mtk_secure_heap *sec_heap =
> > > > dma_heap_get_drvdata(heap);
> > > >    	struct mtk_secure_heap_buffer *sec_buf;
> > > >    	DEFINE_DMA_BUF_EXPORT_INFO(exp_info);
> > > >    	struct dma_buf *dmabuf;
> > > >    	int ret;
> > > >    
> > > > +	/*
> > > > +	 * TEE probe may be late. Initialise the secure session
> > > > in the
> > > > first
> > > > +	 * allocating secure buffer.
> > > > +	 */
> > > > +	if (!sec_heap->mem_session) {
> > > > +		ret = mtk_kree_secure_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);
> > > 
> > > 
> 
>

On Mon, Sep 25, 2023 at 12:49:50PM +0000, Yong Wu (吴勇) wrote:
> On Tue, 2023-09-12 at 11:32 +0200, AngeloGioacchino Del Regno wrote:
> > Il 12/09/23 08:17, Yong Wu (吴勇) ha scritto:
> > > On Mon, 2023-09-11 at 11:29 +0200, AngeloGioacchino Del Regno
> > > wrote:
> > > > Il 11/09/23 04:30, Yong Wu ha scritto:
> > > > > The TEE probe later than dma-buf heap, and PROBE_DEDER doesn't
> > > > > work
> > > > > here since this is not a platform driver, therefore initialise
> > > > > the
> > > > > TEE
> > > > > context/session while we allocate the first secure buffer.
> > > > > 
> > > > > Signed-off-by: Yong Wu <yong.wu@mediatek.com>
> > > > > ---
> > > > >    drivers/dma-buf/heaps/mtk_secure_heap.c | 61
> > > > > +++++++++++++++++++++++++
> > > > >    1 file changed, 61 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/dma-buf/heaps/mtk_secure_heap.c
> > > > > b/drivers/dma-
> > > > > buf/heaps/mtk_secure_heap.c
> > > > > index bbf1c8dce23e..e3da33a3d083 100644
> > > > > --- a/drivers/dma-buf/heaps/mtk_secure_heap.c
> > > > > +++ b/drivers/dma-buf/heaps/mtk_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		"4477588a-8476-11e2-ad15-
> > > > > e41f1390d676"
> > > > > +
> > > > 
> > > > Is this UUID the same for all SoCs and all TZ versions?
> > > 
> > > Yes. It is the same for all SoCs and all TZ versions currently.
> > > 
> > 
> > That's good news!
> > 
> > Is this UUID used in any userspace component? (example: Android
> > HALs?)
> 
> No. Userspace never use it. If userspace would like to allocate this
> secure buffer, it can achieve through the existing dmabuf IOCTL via
> /dev/dma_heap/mtk_svp node.
> 
In general I think as mentioned elsewhere in comments, that there isn't
that much here that seems to be unique for MediaTek in this patch
series, so I think it worth to see whether this whole patch set can be
made more generic. Having said that, the UUID is always unique for a
certain Trusted Application. So, it's not entirely true saying that the
UUID is the same for all SoCs and all TrustZone versions. It might be
true for a family of MediaTek devices and the TEE in use, but not
generically.

So, if we need to differentiate between different TA implementations,
then we need different UUIDs. If it would be possible to make this patch
set generic, then it sounds like a single UUID would be sufficient, but
that would imply that all TA's supporting such a generic UUID would be
implemented the same from an API point of view. Which also means that
for example Trusted Application function ID's needs to be the same etc.
Not impossible to achieve, but still not easy (different TEE follows
different specifications) and it's not typically something we've done in
the past.

Unfortunately there is no standardized database of TA's describing what
they implement and support.

As an alternative, we could implement a query call in the TEE answering,
"What UUID does your TA have that implements secure unmapped heap?".
I.e., something that reminds of a lookup table. Then we wouldn't have to
carry this in UAPI, DT or anywhere else.

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?

On Mon, Sep 11, 2023 at 10:30:33AM +0800, Yong Wu wrote:
> Initialise a mtk_svp 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           |  8 ++
>  drivers/dma-buf/heaps/Makefile          |  1 +
>  drivers/dma-buf/heaps/mtk_secure_heap.c | 99 +++++++++++++++++++++++++
>  3 files changed, 108 insertions(+)
>  create mode 100644 drivers/dma-buf/heaps/mtk_secure_heap.c
> 
> diff --git a/drivers/dma-buf/heaps/Kconfig b/drivers/dma-buf/heaps/Kconfig
> index a5eef06c4226..729c0cf3eb7c 100644
> --- a/drivers/dma-buf/heaps/Kconfig
> +++ b/drivers/dma-buf/heaps/Kconfig
> @@ -12,3 +12,11 @@ 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_MTK_SECURE
> +	bool "DMA-BUF MediaTek Secure Heap"
> +	depends on DMABUF_HEAPS && TEE
> +	help
> +	  Choose this option to enable dma-buf MediaTek secure heap for Secure
> +	  Video Path. This heap is backed by TEE client interfaces. If in
Although this is intended for SVP right now, this is something that very
well could work for other use cases. So, I think I'd not mention "Secure
Video Path" and just mention "secure heap".

> +	  doubt, say N.
> diff --git a/drivers/dma-buf/heaps/Makefile b/drivers/dma-buf/heaps/Makefile
> index 974467791032..df559dbe33fe 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_SYSTEM)	+= system_heap.o
>  obj-$(CONFIG_DMABUF_HEAPS_CMA)		+= cma_heap.o
> +obj-$(CONFIG_DMABUF_HEAPS_MTK_SECURE)	+= mtk_secure_heap.o
> diff --git a/drivers/dma-buf/heaps/mtk_secure_heap.c b/drivers/dma-buf/heaps/mtk_secure_heap.c
> new file mode 100644
> index 000000000000..bbf1c8dce23e
> --- /dev/null
> +++ b/drivers/dma-buf/heaps/mtk_secure_heap.c
> @@ -0,0 +1,99 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * DMABUF mtk_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>
> +
> +/*
> + * MediaTek secure (chunk) memory type
> + *
> + * @KREE_MEM_SEC_CM_TZ: static chunk memory carved out for trustzone.
nit: s/trustzone/TrustZone/

Le 27/09/2023 à 15:46, Joakim Bech a écrit :
> On Mon, Sep 25, 2023 at 12:49:50PM +0000, Yong Wu (吴勇) wrote:
>> On Tue, 2023-09-12 at 11:32 +0200, AngeloGioacchino Del Regno wrote:
>>> Il 12/09/23 08:17, Yong Wu (吴勇) ha scritto:
>>>> On Mon, 2023-09-11 at 11:29 +0200, AngeloGioacchino Del Regno
>>>> wrote:
>>>>> Il 11/09/23 04:30, Yong Wu ha scritto:
>>>>>> The TEE probe later than dma-buf heap, and PROBE_DEDER doesn't
>>>>>> work
>>>>>> here since this is not a platform driver, therefore initialise
>>>>>> the
>>>>>> TEE
>>>>>> context/session while we allocate the first secure buffer.
>>>>>>
>>>>>> Signed-off-by: Yong Wu <yong.wu@mediatek.com>
>>>>>> ---
>>>>>>     drivers/dma-buf/heaps/mtk_secure_heap.c | 61
>>>>>> +++++++++++++++++++++++++
>>>>>>     1 file changed, 61 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/dma-buf/heaps/mtk_secure_heap.c
>>>>>> b/drivers/dma-
>>>>>> buf/heaps/mtk_secure_heap.c
>>>>>> index bbf1c8dce23e..e3da33a3d083 100644
>>>>>> --- a/drivers/dma-buf/heaps/mtk_secure_heap.c
>>>>>> +++ b/drivers/dma-buf/heaps/mtk_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		"4477588a-8476-11e2-ad15-
>>>>>> e41f1390d676"
>>>>>> +
>>>>> Is this UUID the same for all SoCs and all TZ versions?
>>>> Yes. It is the same for all SoCs and all TZ versions currently.
>>>>
>>> That's good news!
>>>
>>> Is this UUID used in any userspace component? (example: Android
>>> HALs?)
>> No. Userspace never use it. If userspace would like to allocate this
>> secure buffer, it can achieve through the existing dmabuf IOCTL via
>> /dev/dma_heap/mtk_svp node.
>>
> In general I think as mentioned elsewhere in comments, that there isn't
> that much here that seems to be unique for MediaTek in this patch
> series, so I think it worth to see whether this whole patch set can be
> made more generic. Having said that, the UUID is always unique for a
> certain Trusted Application. So, it's not entirely true saying that the
> UUID is the same for all SoCs and all TrustZone versions. It might be
> true for a family of MediaTek devices and the TEE in use, but not
> generically.
>
> So, if we need to differentiate between different TA implementations,
> then we need different UUIDs. If it would be possible to make this patch
> set generic, then it sounds like a single UUID would be sufficient, but
> that would imply that all TA's supporting such a generic UUID would be
> implemented the same from an API point of view. Which also means that
> for example Trusted Application function ID's needs to be the same etc.
> Not impossible to achieve, but still not easy (different TEE follows
> different specifications) and it's not typically something we've done in
> the past.
>
> Unfortunately there is no standardized database of TA's describing what
> they implement and support.
>
> As an alternative, we could implement a query call in the TEE answering,
> "What UUID does your TA have that implements secure unmapped heap?".
> I.e., something that reminds of a lookup table. Then we wouldn't have to
> carry this in UAPI, DT or anywhere else.

Joakim does a TA could offer a generic API and hide the hardware specific
details (like kernel uAPI does for drivers) ?

Aside that question I wonder what are the needs to perform a 'secure' playback.
I have in mind 2 requirements:
- secure memory regions, which means configure the hardware to ensure that only
dedicated hardware blocks and read or write into it.
- set hardware blocks in secure modes so they access to secure memory.
Do you see something else ?

Regards,
Benjamin

>

On Wed, Sep 27, 2023 at 6:46 AM Joakim Bech <joakim.bech@linaro.org> wrote:
>
> On Mon, Sep 25, 2023 at 12:49:50PM +0000, Yong Wu (吴勇) wrote:
> > On Tue, 2023-09-12 at 11:32 +0200, AngeloGioacchino Del Regno wrote:
> > > Il 12/09/23 08:17, Yong Wu (吴勇) ha scritto:
> > > > On Mon, 2023-09-11 at 11:29 +0200, AngeloGioacchino Del Regno
> > > > wrote:
> > > > > Il 11/09/23 04:30, Yong Wu ha scritto:
> > > > > > The TEE probe later than dma-buf heap, and PROBE_DEDER doesn't
> > > > > > work
> > > > > > here since this is not a platform driver, therefore initialise
> > > > > > the
> > > > > > TEE
> > > > > > context/session while we allocate the first secure buffer.
> > > > > >
> > > > > > Signed-off-by: Yong Wu <yong.wu@mediatek.com>
> > > > > > ---
> > > > > >    drivers/dma-buf/heaps/mtk_secure_heap.c | 61
> > > > > > +++++++++++++++++++++++++
> > > > > >    1 file changed, 61 insertions(+)
> > > > > >
> > > > > > diff --git a/drivers/dma-buf/heaps/mtk_secure_heap.c
> > > > > > b/drivers/dma-
> > > > > > buf/heaps/mtk_secure_heap.c
> > > > > > index bbf1c8dce23e..e3da33a3d083 100644
> > > > > > --- a/drivers/dma-buf/heaps/mtk_secure_heap.c
> > > > > > +++ b/drivers/dma-buf/heaps/mtk_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               "4477588a-8476-11e2-ad15-
> > > > > > e41f1390d676"
> > > > > > +
> > > > >
> > > > > Is this UUID the same for all SoCs and all TZ versions?
> > > >
> > > > Yes. It is the same for all SoCs and all TZ versions currently.
> > > >
> > >
> > > That's good news!
> > >
> > > Is this UUID used in any userspace component? (example: Android
> > > HALs?)
> >
> > No. Userspace never use it. If userspace would like to allocate this
> > secure buffer, it can achieve through the existing dmabuf IOCTL via
> > /dev/dma_heap/mtk_svp node.
> >
> In general I think as mentioned elsewhere in comments, that there isn't
> that much here that seems to be unique for MediaTek in this patch
> series, so I think it worth to see whether this whole patch set can be
> made more generic. Having said that, the UUID is always unique for a
> certain Trusted Application. So, it's not entirely true saying that the
> UUID is the same for all SoCs and all TrustZone versions. It might be
> true for a family of MediaTek devices and the TEE in use, but not
> generically.
>
> So, if we need to differentiate between different TA implementations,
> then we need different UUIDs. If it would be possible to make this patch
> set generic, then it sounds like a single UUID would be sufficient, but
> that would imply that all TA's supporting such a generic UUID would be
> implemented the same from an API point of view. Which also means that
> for example Trusted Application function ID's needs to be the same etc.
> Not impossible to achieve, but still not easy (different TEE follows
> different specifications) and it's not typically something we've done in
> the past.
>
> Unfortunately there is no standardized database of TA's describing what
> they implement and support.
>
> As an alternative, we could implement a query call in the TEE answering,
> "What UUID does your TA have that implements secure unmapped heap?".
> I.e., something that reminds of a lookup table. Then we wouldn't have to
> carry this in UAPI, DT or anywhere else.
>

I think that's a good idea. If we add kernel APIs to the tee for
opening a session for secure memory allocation and for performing the
allocation, then the UUID, TA commands and TA parameters can all be
decided upon in the TEE specific driver and the code in dma-heap
becomes generic.

> --
> // Regards
> Joakim
>
> >
> > > If it is (and I somehow expect that it is), then this definition
> > > should go
> > > to a UAPI header, as suggested by Christian.
> > >
> > > Cheers!
> > >
> > > > >
> > > > > Thanks,
> > > > > Angelo
> > > > >
> > > > >
> > > > > > +#define MTK_TEE_PARAM_NUM            4
> > > > > >
> > > > > >    /*
> > > > > >     * MediaTek secure (chunk) memory type
> > > > > > @@ -28,17 +34,72 @@ struct mtk_secure_heap_buffer {
> > > > > >    struct mtk_secure_heap {
> > > > > >       const char              *name;
> > > > > >       const enum kree_mem_type mem_type;
> > > > > > +     u32                      mem_session;
> > > > > > +     struct tee_context      *tee_ctx;
> > > > > >    };
> > > > > >
> > > > > > +static int mtk_optee_ctx_match(struct tee_ioctl_version_data
> > > > > > *ver,
> > > > > > const void *data)
> > > > > > +{
> > > > > > +     return ver->impl_id == TEE_IMPL_ID_OPTEE;
> > > > > > +}
> > > > > > +
> > > > > > +static int mtk_kree_secure_session_init(struct mtk_secure_heap
> > > > > > *sec_heap)
> > > > > > +{
> > > > > > +     struct tee_param t_param[MTK_TEE_PARAM_NUM] = {0};
> > > > > > +     struct tee_ioctl_open_session_arg arg = {0};
> > > > > > +     uuid_t ta_mem_uuid;
> > > > > > +     int ret;
> > > > > > +
> > > > > > +     sec_heap->tee_ctx = tee_client_open_context(NULL,
> > > > > > mtk_optee_ctx_match,
> > > > > > +                                                 NULL,
> > > > > > NULL);
> > > > > > +     if (IS_ERR(sec_heap->tee_ctx)) {
> > > > > > +             pr_err("%s: open context failed, ret=%ld\n",
> > > > > > sec_heap-
> > > > > > > name,
> > > > > >
> > > > > > +                    PTR_ERR(sec_heap->tee_ctx));
> > > > > > +             return -ENODEV;
> > > > > > +     }
> > > > > > +
> > > > > > +     arg.num_params = MTK_TEE_PARAM_NUM;
> > > > > > +     arg.clnt_login = TEE_IOCTL_LOGIN_PUBLIC;
> > > > > > +     ret = uuid_parse(TZ_TA_MEM_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("%s: open session failed, ret=%d:%d\n",
> > > > > > +                    sec_heap->name, ret, arg.ret);
> > > > > > +             ret = -EINVAL;
> > > > > > +             goto close_context;
> > > > > > +     }
> > > > > > +     sec_heap->mem_session = arg.session;
> > > > > > +     return 0;
> > > > > > +
> > > > > > +close_context:
> > > > > > +     tee_client_close_context(sec_heap->tee_ctx);
> > > > > > +     return ret;
> > > > > > +}
> > > > > > +
> > > > > >    static struct dma_buf *
> > > > > >    mtk_sec_heap_allocate(struct dma_heap *heap, size_t size,
> > > > > >                     unsigned long fd_flags, unsigned long
> > > > > > heap_flags)
> > > > > >    {
> > > > > > +     struct mtk_secure_heap *sec_heap =
> > > > > > dma_heap_get_drvdata(heap);
> > > > > >       struct mtk_secure_heap_buffer *sec_buf;
> > > > > >       DEFINE_DMA_BUF_EXPORT_INFO(exp_info);
> > > > > >       struct dma_buf *dmabuf;
> > > > > >       int ret;
> > > > > >
> > > > > > +     /*
> > > > > > +      * TEE probe may be late. Initialise the secure session
> > > > > > in the
> > > > > > first
> > > > > > +      * allocating secure buffer.
> > > > > > +      */
> > > > > > +     if (!sec_heap->mem_session) {
> > > > > > +             ret = mtk_kree_secure_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);
> > > > >
> > > > >
> > >
> > >

On Wed, Sep 27, 2023 at 8:18 AM Benjamin Gaignard
<benjamin.gaignard@collabora.com> wrote:
>
>
> Le 27/09/2023 à 15:46, Joakim Bech a écrit :
> > On Mon, Sep 25, 2023 at 12:49:50PM +0000, Yong Wu (吴勇) wrote:
> >> On Tue, 2023-09-12 at 11:32 +0200, AngeloGioacchino Del Regno wrote:
> >>> Il 12/09/23 08:17, Yong Wu (吴勇) ha scritto:
> >>>> On Mon, 2023-09-11 at 11:29 +0200, AngeloGioacchino Del Regno
> >>>> wrote:
> >>>>> Il 11/09/23 04:30, Yong Wu ha scritto:
> >>>>>> The TEE probe later than dma-buf heap, and PROBE_DEDER doesn't
> >>>>>> work
> >>>>>> here since this is not a platform driver, therefore initialise
> >>>>>> the
> >>>>>> TEE
> >>>>>> context/session while we allocate the first secure buffer.
> >>>>>>
> >>>>>> Signed-off-by: Yong Wu <yong.wu@mediatek.com>
> >>>>>> ---
> >>>>>>     drivers/dma-buf/heaps/mtk_secure_heap.c | 61
> >>>>>> +++++++++++++++++++++++++
> >>>>>>     1 file changed, 61 insertions(+)
> >>>>>>
> >>>>>> diff --git a/drivers/dma-buf/heaps/mtk_secure_heap.c
> >>>>>> b/drivers/dma-
> >>>>>> buf/heaps/mtk_secure_heap.c
> >>>>>> index bbf1c8dce23e..e3da33a3d083 100644
> >>>>>> --- a/drivers/dma-buf/heaps/mtk_secure_heap.c
> >>>>>> +++ b/drivers/dma-buf/heaps/mtk_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          "4477588a-8476-11e2-ad15-
> >>>>>> e41f1390d676"
> >>>>>> +
> >>>>> Is this UUID the same for all SoCs and all TZ versions?
> >>>> Yes. It is the same for all SoCs and all TZ versions currently.
> >>>>
> >>> That's good news!
> >>>
> >>> Is this UUID used in any userspace component? (example: Android
> >>> HALs?)
> >> No. Userspace never use it. If userspace would like to allocate this
> >> secure buffer, it can achieve through the existing dmabuf IOCTL via
> >> /dev/dma_heap/mtk_svp node.
> >>
> > In general I think as mentioned elsewhere in comments, that there isn't
> > that much here that seems to be unique for MediaTek in this patch
> > series, so I think it worth to see whether this whole patch set can be
> > made more generic. Having said that, the UUID is always unique for a
> > certain Trusted Application. So, it's not entirely true saying that the
> > UUID is the same for all SoCs and all TrustZone versions. It might be
> > true for a family of MediaTek devices and the TEE in use, but not
> > generically.
> >
> > So, if we need to differentiate between different TA implementations,
> > then we need different UUIDs. If it would be possible to make this patch
> > set generic, then it sounds like a single UUID would be sufficient, but
> > that would imply that all TA's supporting such a generic UUID would be
> > implemented the same from an API point of view. Which also means that
> > for example Trusted Application function ID's needs to be the same etc.
> > Not impossible to achieve, but still not easy (different TEE follows
> > different specifications) and it's not typically something we've done in
> > the past.
> >
> > Unfortunately there is no standardized database of TA's describing what
> > they implement and support.
> >
> > As an alternative, we could implement a query call in the TEE answering,
> > "What UUID does your TA have that implements secure unmapped heap?".
> > I.e., something that reminds of a lookup table. Then we wouldn't have to
> > carry this in UAPI, DT or anywhere else.
>
> Joakim does a TA could offer a generic API and hide the hardware specific
> details (like kernel uAPI does for drivers) ?
It would have to go through another layer (like the tee driver) to be
a generic API. The main issue with TAs is that they have UUIDs you
need to connect to and specific codes for each function; so we should
abstract at a layer above where those exist in the dma-heap code.
>
> Aside that question I wonder what are the needs to perform a 'secure' playback.
> I have in mind 2 requirements:
> - secure memory regions, which means configure the hardware to ensure that only
> dedicated hardware blocks and read or write into it.
> - set hardware blocks in secure modes so they access to secure memory.
> Do you see something else ?
This is more or less what is required, but this is out of scope for
the Linux kernel since it can't be trusted to do these things...this
is all done in firmware or the TEE itself.
>
> Regards,
> Benjamin
>
> >

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

Le 27/09/2023 à 20:56, Jeffrey Kardatzke a écrit :
> On Wed, Sep 27, 2023 at 8:18 AM Benjamin Gaignard
> <benjamin.gaignard@collabora.com> wrote:
>>
>> Le 27/09/2023 à 15:46, Joakim Bech a écrit :
>>> On Mon, Sep 25, 2023 at 12:49:50PM +0000, Yong Wu (吴勇) wrote:
>>>> On Tue, 2023-09-12 at 11:32 +0200, AngeloGioacchino Del Regno wrote:
>>>>> Il 12/09/23 08:17, Yong Wu (吴勇) ha scritto:
>>>>>> On Mon, 2023-09-11 at 11:29 +0200, AngeloGioacchino Del Regno
>>>>>> wrote:
>>>>>>> Il 11/09/23 04:30, Yong Wu ha scritto:
>>>>>>>> The TEE probe later than dma-buf heap, and PROBE_DEDER doesn't
>>>>>>>> work
>>>>>>>> here since this is not a platform driver, therefore initialise
>>>>>>>> the
>>>>>>>> TEE
>>>>>>>> context/session while we allocate the first secure buffer.
>>>>>>>>
>>>>>>>> Signed-off-by: Yong Wu <yong.wu@mediatek.com>
>>>>>>>> ---
>>>>>>>>      drivers/dma-buf/heaps/mtk_secure_heap.c | 61
>>>>>>>> +++++++++++++++++++++++++
>>>>>>>>      1 file changed, 61 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/drivers/dma-buf/heaps/mtk_secure_heap.c
>>>>>>>> b/drivers/dma-
>>>>>>>> buf/heaps/mtk_secure_heap.c
>>>>>>>> index bbf1c8dce23e..e3da33a3d083 100644
>>>>>>>> --- a/drivers/dma-buf/heaps/mtk_secure_heap.c
>>>>>>>> +++ b/drivers/dma-buf/heaps/mtk_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          "4477588a-8476-11e2-ad15-
>>>>>>>> e41f1390d676"
>>>>>>>> +
>>>>>>> Is this UUID the same for all SoCs and all TZ versions?
>>>>>> Yes. It is the same for all SoCs and all TZ versions currently.
>>>>>>
>>>>> That's good news!
>>>>>
>>>>> Is this UUID used in any userspace component? (example: Android
>>>>> HALs?)
>>>> No. Userspace never use it. If userspace would like to allocate this
>>>> secure buffer, it can achieve through the existing dmabuf IOCTL via
>>>> /dev/dma_heap/mtk_svp node.
>>>>
>>> In general I think as mentioned elsewhere in comments, that there isn't
>>> that much here that seems to be unique for MediaTek in this patch
>>> series, so I think it worth to see whether this whole patch set can be
>>> made more generic. Having said that, the UUID is always unique for a
>>> certain Trusted Application. So, it's not entirely true saying that the
>>> UUID is the same for all SoCs and all TrustZone versions. It might be
>>> true for a family of MediaTek devices and the TEE in use, but not
>>> generically.
>>>
>>> So, if we need to differentiate between different TA implementations,
>>> then we need different UUIDs. If it would be possible to make this patch
>>> set generic, then it sounds like a single UUID would be sufficient, but
>>> that would imply that all TA's supporting such a generic UUID would be
>>> implemented the same from an API point of view. Which also means that
>>> for example Trusted Application function ID's needs to be the same etc.
>>> Not impossible to achieve, but still not easy (different TEE follows
>>> different specifications) and it's not typically something we've done in
>>> the past.
>>>
>>> Unfortunately there is no standardized database of TA's describing what
>>> they implement and support.
>>>
>>> As an alternative, we could implement a query call in the TEE answering,
>>> "What UUID does your TA have that implements secure unmapped heap?".
>>> I.e., something that reminds of a lookup table. Then we wouldn't have to
>>> carry this in UAPI, DT or anywhere else.
>> Joakim does a TA could offer a generic API and hide the hardware specific
>> details (like kernel uAPI does for drivers) ?
> It would have to go through another layer (like the tee driver) to be
> a generic API. The main issue with TAs is that they have UUIDs you
> need to connect to and specific codes for each function; so we should
> abstract at a layer above where those exist in the dma-heap code.
>> Aside that question I wonder what are the needs to perform a 'secure' playback.
>> I have in mind 2 requirements:
>> - secure memory regions, which means configure the hardware to ensure that only
>> dedicated hardware blocks and read or write into it.
>> - set hardware blocks in secure modes so they access to secure memory.
>> Do you see something else ?
> This is more or less what is required, but this is out of scope for
> the Linux kernel since it can't be trusted to do these things...this
> is all done in firmware or the TEE itself.

Yes kernel can't be trusted to do these things but know what we need could help
to define a API for a generic TA.

Just to brainstorm on mailing list:
What about a TA API like
TA_secure_memory_region() and TA_unsecure_memory_region() with parameters like:
- device identifier (an ID or compatible string maybe)
- memory region (physical address, size, offset)
- requested access rights (read, write)

and on kernel side a IOMMU driver because it basically have all this information already
(device attachment, kernel map/unmap).

In my mind it sound like a solution to limit the impact (new controls, new memory type)
inside v4l2. Probably we won't need new heap either.
All hardware dedicated implementations could live inside the TA which can offer a generic
API.

>> Regards,
>> Benjamin
>>

On Thu, Sep 28, 2023 at 1:30 AM Benjamin Gaignard
<benjamin.gaignard@collabora.com> wrote:
>
>
> Le 27/09/2023 à 20:56, Jeffrey Kardatzke a écrit :
> > On Wed, Sep 27, 2023 at 8:18 AM Benjamin Gaignard
> > <benjamin.gaignard@collabora.com> wrote:
> >>
> >> Le 27/09/2023 à 15:46, Joakim Bech a écrit :
> >>> On Mon, Sep 25, 2023 at 12:49:50PM +0000, Yong Wu (吴勇) wrote:
> >>>> On Tue, 2023-09-12 at 11:32 +0200, AngeloGioacchino Del Regno wrote:
> >>>>> Il 12/09/23 08:17, Yong Wu (吴勇) ha scritto:
> >>>>>> On Mon, 2023-09-11 at 11:29 +0200, AngeloGioacchino Del Regno
> >>>>>> wrote:
> >>>>>>> Il 11/09/23 04:30, Yong Wu ha scritto:
> >>>>>>>> The TEE probe later than dma-buf heap, and PROBE_DEDER doesn't
> >>>>>>>> work
> >>>>>>>> here since this is not a platform driver, therefore initialise
> >>>>>>>> the
> >>>>>>>> TEE
> >>>>>>>> context/session while we allocate the first secure buffer.
> >>>>>>>>
> >>>>>>>> Signed-off-by: Yong Wu <yong.wu@mediatek.com>
> >>>>>>>> ---
> >>>>>>>>      drivers/dma-buf/heaps/mtk_secure_heap.c | 61
> >>>>>>>> +++++++++++++++++++++++++
> >>>>>>>>      1 file changed, 61 insertions(+)
> >>>>>>>>
> >>>>>>>> diff --git a/drivers/dma-buf/heaps/mtk_secure_heap.c
> >>>>>>>> b/drivers/dma-
> >>>>>>>> buf/heaps/mtk_secure_heap.c
> >>>>>>>> index bbf1c8dce23e..e3da33a3d083 100644
> >>>>>>>> --- a/drivers/dma-buf/heaps/mtk_secure_heap.c
> >>>>>>>> +++ b/drivers/dma-buf/heaps/mtk_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          "4477588a-8476-11e2-ad15-
> >>>>>>>> e41f1390d676"
> >>>>>>>> +
> >>>>>>> Is this UUID the same for all SoCs and all TZ versions?
> >>>>>> Yes. It is the same for all SoCs and all TZ versions currently.
> >>>>>>
> >>>>> That's good news!
> >>>>>
> >>>>> Is this UUID used in any userspace component? (example: Android
> >>>>> HALs?)
> >>>> No. Userspace never use it. If userspace would like to allocate this
> >>>> secure buffer, it can achieve through the existing dmabuf IOCTL via
> >>>> /dev/dma_heap/mtk_svp node.
> >>>>
> >>> In general I think as mentioned elsewhere in comments, that there isn't
> >>> that much here that seems to be unique for MediaTek in this patch
> >>> series, so I think it worth to see whether this whole patch set can be
> >>> made more generic. Having said that, the UUID is always unique for a
> >>> certain Trusted Application. So, it's not entirely true saying that the
> >>> UUID is the same for all SoCs and all TrustZone versions. It might be
> >>> true for a family of MediaTek devices and the TEE in use, but not
> >>> generically.
> >>>
> >>> So, if we need to differentiate between different TA implementations,
> >>> then we need different UUIDs. If it would be possible to make this patch
> >>> set generic, then it sounds like a single UUID would be sufficient, but
> >>> that would imply that all TA's supporting such a generic UUID would be
> >>> implemented the same from an API point of view. Which also means that
> >>> for example Trusted Application function ID's needs to be the same etc.
> >>> Not impossible to achieve, but still not easy (different TEE follows
> >>> different specifications) and it's not typically something we've done in
> >>> the past.
> >>>
> >>> Unfortunately there is no standardized database of TA's describing what
> >>> they implement and support.
> >>>
> >>> As an alternative, we could implement a query call in the TEE answering,
> >>> "What UUID does your TA have that implements secure unmapped heap?".
> >>> I.e., something that reminds of a lookup table. Then we wouldn't have to
> >>> carry this in UAPI, DT or anywhere else.
> >> Joakim does a TA could offer a generic API and hide the hardware specific
> >> details (like kernel uAPI does for drivers) ?
> > It would have to go through another layer (like the tee driver) to be
> > a generic API. The main issue with TAs is that they have UUIDs you
> > need to connect to and specific codes for each function; so we should
> > abstract at a layer above where those exist in the dma-heap code.
> >> Aside that question I wonder what are the needs to perform a 'secure' playback.
> >> I have in mind 2 requirements:
> >> - secure memory regions, which means configure the hardware to ensure that only
> >> dedicated hardware blocks and read or write into it.
> >> - set hardware blocks in secure modes so they access to secure memory.
> >> Do you see something else ?
> > This is more or less what is required, but this is out of scope for
> > the Linux kernel since it can't be trusted to do these things...this
> > is all done in firmware or the TEE itself.
>
> Yes kernel can't be trusted to do these things but know what we need could help
> to define a API for a generic TA.
>
> Just to brainstorm on mailing list:
> What about a TA API like
> TA_secure_memory_region() and TA_unsecure_memory_region() with parameters like:
> - device identifier (an ID or compatible string maybe)
> - memory region (physical address, size, offset)
> - requested access rights (read, write)
>
> and on kernel side a IOMMU driver because it basically have all this information already
> (device attachment, kernel map/unmap).
>
> In my mind it sound like a solution to limit the impact (new controls, new memory type)
> inside v4l2. Probably we won't need new heap either.
> All hardware dedicated implementations could live inside the TA which can offer a generic
> API.

The main problem with that type of design is the limitations of
TrustZone memory protection. Usually there is a limit to the number of
regions you can define for memory protection (and there is on
Mediatek). So you can't pass an arbitrary memory region and mark it
protected/unprotected at a given time. You need to establish these
regions in the firmware instead and then configure those regions for
protection in the firmware or the TEE.

>
> >> Regards,
> >> Benjamin
> >>

Le 28/09/2023 à 19:48, Jeffrey Kardatzke a écrit :
> On Thu, Sep 28, 2023 at 1:30 AM Benjamin Gaignard
> <benjamin.gaignard@collabora.com> wrote:
>>
>> Le 27/09/2023 à 20:56, Jeffrey Kardatzke a écrit :
>>> On Wed, Sep 27, 2023 at 8:18 AM Benjamin Gaignard
>>> <benjamin.gaignard@collabora.com> wrote:
>>>> Le 27/09/2023 à 15:46, Joakim Bech a écrit :
>>>>> On Mon, Sep 25, 2023 at 12:49:50PM +0000, Yong Wu (吴勇) wrote:
>>>>>> On Tue, 2023-09-12 at 11:32 +0200, AngeloGioacchino Del Regno wrote:
>>>>>>> Il 12/09/23 08:17, Yong Wu (吴勇) ha scritto:
>>>>>>>> On Mon, 2023-09-11 at 11:29 +0200, AngeloGioacchino Del Regno
>>>>>>>> wrote:
>>>>>>>>> Il 11/09/23 04:30, Yong Wu ha scritto:
>>>>>>>>>> The TEE probe later than dma-buf heap, and PROBE_DEDER doesn't
>>>>>>>>>> work
>>>>>>>>>> here since this is not a platform driver, therefore initialise
>>>>>>>>>> the
>>>>>>>>>> TEE
>>>>>>>>>> context/session while we allocate the first secure buffer.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Yong Wu <yong.wu@mediatek.com>
>>>>>>>>>> ---
>>>>>>>>>>       drivers/dma-buf/heaps/mtk_secure_heap.c | 61
>>>>>>>>>> +++++++++++++++++++++++++
>>>>>>>>>>       1 file changed, 61 insertions(+)
>>>>>>>>>>
>>>>>>>>>> diff --git a/drivers/dma-buf/heaps/mtk_secure_heap.c
>>>>>>>>>> b/drivers/dma-
>>>>>>>>>> buf/heaps/mtk_secure_heap.c
>>>>>>>>>> index bbf1c8dce23e..e3da33a3d083 100644
>>>>>>>>>> --- a/drivers/dma-buf/heaps/mtk_secure_heap.c
>>>>>>>>>> +++ b/drivers/dma-buf/heaps/mtk_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          "4477588a-8476-11e2-ad15-
>>>>>>>>>> e41f1390d676"
>>>>>>>>>> +
>>>>>>>>> Is this UUID the same for all SoCs and all TZ versions?
>>>>>>>> Yes. It is the same for all SoCs and all TZ versions currently.
>>>>>>>>
>>>>>>> That's good news!
>>>>>>>
>>>>>>> Is this UUID used in any userspace component? (example: Android
>>>>>>> HALs?)
>>>>>> No. Userspace never use it. If userspace would like to allocate this
>>>>>> secure buffer, it can achieve through the existing dmabuf IOCTL via
>>>>>> /dev/dma_heap/mtk_svp node.
>>>>>>
>>>>> In general I think as mentioned elsewhere in comments, that there isn't
>>>>> that much here that seems to be unique for MediaTek in this patch
>>>>> series, so I think it worth to see whether this whole patch set can be
>>>>> made more generic. Having said that, the UUID is always unique for a
>>>>> certain Trusted Application. So, it's not entirely true saying that the
>>>>> UUID is the same for all SoCs and all TrustZone versions. It might be
>>>>> true for a family of MediaTek devices and the TEE in use, but not
>>>>> generically.
>>>>>
>>>>> So, if we need to differentiate between different TA implementations,
>>>>> then we need different UUIDs. If it would be possible to make this patch
>>>>> set generic, then it sounds like a single UUID would be sufficient, but
>>>>> that would imply that all TA's supporting such a generic UUID would be
>>>>> implemented the same from an API point of view. Which also means that
>>>>> for example Trusted Application function ID's needs to be the same etc.
>>>>> Not impossible to achieve, but still not easy (different TEE follows
>>>>> different specifications) and it's not typically something we've done in
>>>>> the past.
>>>>>
>>>>> Unfortunately there is no standardized database of TA's describing what
>>>>> they implement and support.
>>>>>
>>>>> As an alternative, we could implement a query call in the TEE answering,
>>>>> "What UUID does your TA have that implements secure unmapped heap?".
>>>>> I.e., something that reminds of a lookup table. Then we wouldn't have to
>>>>> carry this in UAPI, DT or anywhere else.
>>>> Joakim does a TA could offer a generic API and hide the hardware specific
>>>> details (like kernel uAPI does for drivers) ?
>>> It would have to go through another layer (like the tee driver) to be
>>> a generic API. The main issue with TAs is that they have UUIDs you
>>> need to connect to and specific codes for each function; so we should
>>> abstract at a layer above where those exist in the dma-heap code.
>>>> Aside that question I wonder what are the needs to perform a 'secure' playback.
>>>> I have in mind 2 requirements:
>>>> - secure memory regions, which means configure the hardware to ensure that only
>>>> dedicated hardware blocks and read or write into it.
>>>> - set hardware blocks in secure modes so they access to secure memory.
>>>> Do you see something else ?
>>> This is more or less what is required, but this is out of scope for
>>> the Linux kernel since it can't be trusted to do these things...this
>>> is all done in firmware or the TEE itself.
>> Yes kernel can't be trusted to do these things but know what we need could help
>> to define a API for a generic TA.
>>
>> Just to brainstorm on mailing list:
>> What about a TA API like
>> TA_secure_memory_region() and TA_unsecure_memory_region() with parameters like:
>> - device identifier (an ID or compatible string maybe)
>> - memory region (physical address, size, offset)
>> - requested access rights (read, write)
>>
>> and on kernel side a IOMMU driver because it basically have all this information already
>> (device attachment, kernel map/unmap).
>>
>> In my mind it sound like a solution to limit the impact (new controls, new memory type)
>> inside v4l2. Probably we won't need new heap either.
>> All hardware dedicated implementations could live inside the TA which can offer a generic
>> API.
> The main problem with that type of design is the limitations of
> TrustZone memory protection. Usually there is a limit to the number of
> regions you can define for memory protection (and there is on
> Mediatek). So you can't pass an arbitrary memory region and mark it
> protected/unprotected at a given time. You need to establish these
> regions in the firmware instead and then configure those regions for
> protection in the firmware or the TEE.

The TEE iommu could be aware of these limitations and merge the regions when possible
plus we can define a CMA region for each device to limit the secured memory fragmentation.

>
>>>> Regards,
>>>> Benjamin
>>>>

Sorry for the delayed reply, needed to get some more info. This really
wouldn't be possible due to the limitation on the number of
regions...for example only 32 regions can be defined on some SoCs, and
you'd run out of regions really fast trying to do this. That's why
this is creating heaps for those regions and then allocations are
performed within the defined region is the preferred strategy.


On Thu, Sep 28, 2023 at 11:54 PM Benjamin Gaignard
<benjamin.gaignard@collabora.com> wrote:
>
>
> Le 28/09/2023 à 19:48, Jeffrey Kardatzke a écrit :
> > On Thu, Sep 28, 2023 at 1:30 AM Benjamin Gaignard
> > <benjamin.gaignard@collabora.com> wrote:
> >>
> >> Le 27/09/2023 à 20:56, Jeffrey Kardatzke a écrit :
> >>> On Wed, Sep 27, 2023 at 8:18 AM Benjamin Gaignard
> >>> <benjamin.gaignard@collabora.com> wrote:
> >>>> Le 27/09/2023 à 15:46, Joakim Bech a écrit :
> >>>>> On Mon, Sep 25, 2023 at 12:49:50PM +0000, Yong Wu (吴勇) wrote:
> >>>>>> On Tue, 2023-09-12 at 11:32 +0200, AngeloGioacchino Del Regno wrote:
> >>>>>>> Il 12/09/23 08:17, Yong Wu (吴勇) ha scritto:
> >>>>>>>> On Mon, 2023-09-11 at 11:29 +0200, AngeloGioacchino Del Regno
> >>>>>>>> wrote:
> >>>>>>>>> Il 11/09/23 04:30, Yong Wu ha scritto:
> >>>>>>>>>> The TEE probe later than dma-buf heap, and PROBE_DEDER doesn't
> >>>>>>>>>> work
> >>>>>>>>>> here since this is not a platform driver, therefore initialise
> >>>>>>>>>> the
> >>>>>>>>>> TEE
> >>>>>>>>>> context/session while we allocate the first secure buffer.
> >>>>>>>>>>
> >>>>>>>>>> Signed-off-by: Yong Wu <yong.wu@mediatek.com>
> >>>>>>>>>> ---
> >>>>>>>>>>       drivers/dma-buf/heaps/mtk_secure_heap.c | 61
> >>>>>>>>>> +++++++++++++++++++++++++
> >>>>>>>>>>       1 file changed, 61 insertions(+)
> >>>>>>>>>>
> >>>>>>>>>> diff --git a/drivers/dma-buf/heaps/mtk_secure_heap.c
> >>>>>>>>>> b/drivers/dma-
> >>>>>>>>>> buf/heaps/mtk_secure_heap.c
> >>>>>>>>>> index bbf1c8dce23e..e3da33a3d083 100644
> >>>>>>>>>> --- a/drivers/dma-buf/heaps/mtk_secure_heap.c
> >>>>>>>>>> +++ b/drivers/dma-buf/heaps/mtk_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          "4477588a-8476-11e2-ad15-
> >>>>>>>>>> e41f1390d676"
> >>>>>>>>>> +
> >>>>>>>>> Is this UUID the same for all SoCs and all TZ versions?
> >>>>>>>> Yes. It is the same for all SoCs and all TZ versions currently.
> >>>>>>>>
> >>>>>>> That's good news!
> >>>>>>>
> >>>>>>> Is this UUID used in any userspace component? (example: Android
> >>>>>>> HALs?)
> >>>>>> No. Userspace never use it. If userspace would like to allocate this
> >>>>>> secure buffer, it can achieve through the existing dmabuf IOCTL via
> >>>>>> /dev/dma_heap/mtk_svp node.
> >>>>>>
> >>>>> In general I think as mentioned elsewhere in comments, that there isn't
> >>>>> that much here that seems to be unique for MediaTek in this patch
> >>>>> series, so I think it worth to see whether this whole patch set can be
> >>>>> made more generic. Having said that, the UUID is always unique for a
> >>>>> certain Trusted Application. So, it's not entirely true saying that the
> >>>>> UUID is the same for all SoCs and all TrustZone versions. It might be
> >>>>> true for a family of MediaTek devices and the TEE in use, but not
> >>>>> generically.
> >>>>>
> >>>>> So, if we need to differentiate between different TA implementations,
> >>>>> then we need different UUIDs. If it would be possible to make this patch
> >>>>> set generic, then it sounds like a single UUID would be sufficient, but
> >>>>> that would imply that all TA's supporting such a generic UUID would be
> >>>>> implemented the same from an API point of view. Which also means that
> >>>>> for example Trusted Application function ID's needs to be the same etc.
> >>>>> Not impossible to achieve, but still not easy (different TEE follows
> >>>>> different specifications) and it's not typically something we've done in
> >>>>> the past.
> >>>>>
> >>>>> Unfortunately there is no standardized database of TA's describing what
> >>>>> they implement and support.
> >>>>>
> >>>>> As an alternative, we could implement a query call in the TEE answering,
> >>>>> "What UUID does your TA have that implements secure unmapped heap?".
> >>>>> I.e., something that reminds of a lookup table. Then we wouldn't have to
> >>>>> carry this in UAPI, DT or anywhere else.
> >>>> Joakim does a TA could offer a generic API and hide the hardware specific
> >>>> details (like kernel uAPI does for drivers) ?
> >>> It would have to go through another layer (like the tee driver) to be
> >>> a generic API. The main issue with TAs is that they have UUIDs you
> >>> need to connect to and specific codes for each function; so we should
> >>> abstract at a layer above where those exist in the dma-heap code.
> >>>> Aside that question I wonder what are the needs to perform a 'secure' playback.
> >>>> I have in mind 2 requirements:
> >>>> - secure memory regions, which means configure the hardware to ensure that only
> >>>> dedicated hardware blocks and read or write into it.
> >>>> - set hardware blocks in secure modes so they access to secure memory.
> >>>> Do you see something else ?
> >>> This is more or less what is required, but this is out of scope for
> >>> the Linux kernel since it can't be trusted to do these things...this
> >>> is all done in firmware or the TEE itself.
> >> Yes kernel can't be trusted to do these things but know what we need could help
> >> to define a API for a generic TA.
> >>
> >> Just to brainstorm on mailing list:
> >> What about a TA API like
> >> TA_secure_memory_region() and TA_unsecure_memory_region() with parameters like:
> >> - device identifier (an ID or compatible string maybe)
> >> - memory region (physical address, size, offset)
> >> - requested access rights (read, write)
> >>
> >> and on kernel side a IOMMU driver because it basically have all this information already
> >> (device attachment, kernel map/unmap).
> >>
> >> In my mind it sound like a solution to limit the impact (new controls, new memory type)
> >> inside v4l2. Probably we won't need new heap either.
> >> All hardware dedicated implementations could live inside the TA which can offer a generic
> >> API.
> > The main problem with that type of design is the limitations of
> > TrustZone memory protection. Usually there is a limit to the number of
> > regions you can define for memory protection (and there is on
> > Mediatek). So you can't pass an arbitrary memory region and mark it
> > protected/unprotected at a given time. You need to establish these
> > regions in the firmware instead and then configure those regions for
> > protection in the firmware or the TEE.
>
> The TEE iommu could be aware of these limitations and merge the regions when possible
> plus we can define a CMA region for each device to limit the secured memory fragmentation.
>
> >
> >>>> Regards,
> >>>> Benjamin
> >>>>

On 9/11/2023 8:00 AM, Yong Wu wrote:
> This patchset consists of two parts, the first is from John and TJ.
> It adds some heap interfaces, then our kernel users could allocate buffer
> from special heap. The second part is adding MTK secure heap for SVP
> (Secure Video Path). A total of two heaps are added, one is mtk_svp and
> the other is mtk_svp_cma. The mtk_svp buffer is reserved for the secure
> world after bootup and it is used for ES/working buffer, while the
> mtk_svp_cma buffer is dynamically reserved for the secure world and will
> be get ready when we start playing secure videos, this heap is used for the
> frame buffer. Once the security video playing is complete, the CMA will be
> released.
> 
> For easier viewing, I've split the new heap file into several patches.
> 
> The consumers of new heap and new interfaces are our codec and drm which
> will send upstream soon, probably this week.
> 
> Base on v6.6-rc1.
> 
> John Stultz (2):
>   dma-heap: Add proper kref handling on dma-buf heaps
>   dma-heap: Provide accessors so that in-kernel drivers can allocate
>     dmabufs from specific heaps
> 
> T.J. Mercier (1):
>   dma-buf: heaps: Deduplicate docs and adopt common format
> 
> Yong Wu (6):
>   dma-buf: heaps: Initialise MediaTek secure heap
>   dma-buf: heaps: mtk_sec_heap: Initialise tee session
>   dma-buf: heaps: mtk_sec_heap: Add tee service call for buffer
>     allocating/freeing
>   dma-buf: heaps: mtk_sec_heap: Add dma_ops
>   dt-bindings: reserved-memory: MediaTek: Add reserved memory for SVP
>   dma_buf: heaps: mtk_sec_heap: Add a new CMA heap
> 
>  .../mediatek,secure_cma_chunkmem.yaml         |  42 ++
>  drivers/dma-buf/dma-heap.c                    | 127 +++--
>  drivers/dma-buf/heaps/Kconfig                 |   8 +
>  drivers/dma-buf/heaps/Makefile                |   1 +
>  drivers/dma-buf/heaps/mtk_secure_heap.c       | 458 ++++++++++++++++++
>  include/linux/dma-heap.h                      |  42 +-
>  6 files changed, 630 insertions(+), 48 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/reserved-memory/mediatek,secure_cma_chunkmem.yaml
>  create mode 100644 drivers/dma-buf/heaps/mtk_secure_heap.c
> 

Thanks for this patch series.

In Qualcomm as well we have similar usecases which need secure heap. We are working on
posting them upstream, would share more details on usecases soon.

Have few comments on the current implementation.

1) I see most the implementation here is mtk specific, even file names ,heap names etc.
   But secure heap is a common requirement, can we keep naming as well generic may be secure_heap ?

2) secure heap has two parts, one is allocation and other one is securing the memory.
   Have few comments on making these interfaces generic, would post those on corresponding 
   patches.

Thanks,
Vijay

On 9/11/2023 8:00 AM, Yong Wu wrote:
> Initialise a mtk_svp 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           |  8 ++
>  drivers/dma-buf/heaps/Makefile          |  1 +
>  drivers/dma-buf/heaps/mtk_secure_heap.c | 99 +++++++++++++++++++++++++
>  3 files changed, 108 insertions(+)
>  create mode 100644 drivers/dma-buf/heaps/mtk_secure_heap.c
> 
> diff --git a/drivers/dma-buf/heaps/Kconfig b/drivers/dma-buf/heaps/Kconfig
> index a5eef06c4226..729c0cf3eb7c 100644
> --- a/drivers/dma-buf/heaps/Kconfig
> +++ b/drivers/dma-buf/heaps/Kconfig
> @@ -12,3 +12,11 @@ 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_MTK_SECURE
> +	bool "DMA-BUF MediaTek Secure Heap"
> +	depends on DMABUF_HEAPS && TEE
> +	help
> +	  Choose this option to enable dma-buf MediaTek secure heap for Secure
> +	  Video Path. This heap is backed by TEE client interfaces. If in
> +	  doubt, say N.
> diff --git a/drivers/dma-buf/heaps/Makefile b/drivers/dma-buf/heaps/Makefile
> index 974467791032..df559dbe33fe 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_SYSTEM)	+= system_heap.o
>  obj-$(CONFIG_DMABUF_HEAPS_CMA)		+= cma_heap.o
> +obj-$(CONFIG_DMABUF_HEAPS_MTK_SECURE)	+= mtk_secure_heap.o
> diff --git a/drivers/dma-buf/heaps/mtk_secure_heap.c b/drivers/dma-buf/heaps/mtk_secure_heap.c
> new file mode 100644
> index 000000000000..bbf1c8dce23e
> --- /dev/null
> +++ b/drivers/dma-buf/heaps/mtk_secure_heap.c
> @@ -0,0 +1,99 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * DMABUF mtk_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>
> +
> +/*
> + * MediaTek secure (chunk) memory type
> + *
> + * @KREE_MEM_SEC_CM_TZ: static chunk memory carved out for trustzone.
> + */
> +enum kree_mem_type {
> +	KREE_MEM_SEC_CM_TZ = 1,
> +};
> +
> +struct mtk_secure_heap_buffer {
> +	struct dma_heap		*heap;
> +	size_t			size;
> +};
> +
> +struct mtk_secure_heap {
> +	const char		*name;
> +	const enum kree_mem_type mem_type;
> +};
> +
> +static struct dma_buf *
> +mtk_sec_heap_allocate(struct dma_heap *heap, size_t size,
> +		      unsigned long fd_flags, unsigned long heap_flags)
> +{
> +	struct mtk_secure_heap_buffer *sec_buf;
> +	DEFINE_DMA_BUF_EXPORT_INFO(exp_info);
> +	struct dma_buf *dmabuf;
> +	int ret;
> +
> +	sec_buf = kzalloc(sizeof(*sec_buf), GFP_KERNEL);

As we know, kzalloc can only allocate 4MB at max. So, secure heap has this limitation.
can we have a way to allocate more memory in secure heap ? maybe similar to how system heap does?

Thanks,
Vijay

> +	if (!sec_buf)
> +		return ERR_PTR(-ENOMEM);
> +
> +	sec_buf->size = 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 mtk_sec_heap_ops = {
> +	.allocate	= mtk_sec_heap_allocate,
> +};
> +
> +static struct mtk_secure_heap mtk_sec_heap[] = {
> +	{
> +		.name		= "mtk_svp",
> +		.mem_type	= KREE_MEM_SEC_CM_TZ,
> +	},
> +};
> +
> +static int mtk_sec_heap_init(void)
> +{
> +	struct mtk_secure_heap *sec_heap = mtk_sec_heap;
> +	struct dma_heap_export_info exp_info;
> +	struct dma_heap *heap;
> +	unsigned int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(mtk_sec_heap); i++, sec_heap++) {
> +		exp_info.name = sec_heap->name;
> +		exp_info.ops = &mtk_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(mtk_sec_heap_init);
> +MODULE_DESCRIPTION("MediaTek Secure Heap Driver");
> +MODULE_LICENSE("GPL");

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);

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:
> > Initialise a mtk_svp 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           |  8 ++
> >  drivers/dma-buf/heaps/Makefile          |  1 +
> >  drivers/dma-buf/heaps/mtk_secure_heap.c | 99
> +++++++++++++++++++++++++
> >  3 files changed, 108 insertions(+)
> >  create mode 100644 drivers/dma-buf/heaps/mtk_secure_heap.c
> > 
> > diff --git a/drivers/dma-buf/heaps/Kconfig b/drivers/dma-
> buf/heaps/Kconfig
> > index a5eef06c4226..729c0cf3eb7c 100644
> > --- a/drivers/dma-buf/heaps/Kconfig
> > +++ b/drivers/dma-buf/heaps/Kconfig
> > @@ -12,3 +12,11 @@ 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_MTK_SECURE
> > +bool "DMA-BUF MediaTek Secure Heap"
> > +depends on DMABUF_HEAPS && TEE
> > +help
> > +  Choose this option to enable dma-buf MediaTek secure heap for
> Secure
> > +  Video Path. This heap is backed by TEE client interfaces. If in
> > +  doubt, say N.
> > diff --git a/drivers/dma-buf/heaps/Makefile b/drivers/dma-
> buf/heaps/Makefile
> > index 974467791032..df559dbe33fe 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_SYSTEM)+= system_heap.o
> >  obj-$(CONFIG_DMABUF_HEAPS_CMA)+= cma_heap.o
> > +obj-$(CONFIG_DMABUF_HEAPS_MTK_SECURE)+= mtk_secure_heap.o
> > diff --git a/drivers/dma-buf/heaps/mtk_secure_heap.c b/drivers/dma-
> buf/heaps/mtk_secure_heap.c
> > new file mode 100644
> > index 000000000000..bbf1c8dce23e
> > --- /dev/null
> > +++ b/drivers/dma-buf/heaps/mtk_secure_heap.c
> > @@ -0,0 +1,99 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * DMABUF mtk_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>
> > +
> > +/*
> > + * MediaTek secure (chunk) memory type
> > + *
> > + * @KREE_MEM_SEC_CM_TZ: static chunk memory carved out for
> trustzone.
> > + */
> > +enum kree_mem_type {
> > +KREE_MEM_SEC_CM_TZ = 1,
> > +};
> > +
> > +struct mtk_secure_heap_buffer {
> > +struct dma_heap*heap;
> > +size_tsize;
> > +};
> > +
> > +struct mtk_secure_heap {
> > +const char*name;
> > +const enum kree_mem_type mem_type;
> > +};
> > +
> > +static struct dma_buf *
> > +mtk_sec_heap_allocate(struct dma_heap *heap, size_t size,
> > +      unsigned long fd_flags, unsigned long heap_flags)
> > +{
> > +struct mtk_secure_heap_buffer *sec_buf;
> > +DEFINE_DMA_BUF_EXPORT_INFO(exp_info);
> > +struct dma_buf *dmabuf;
> > +int ret;
> > +
> > +sec_buf = kzalloc(sizeof(*sec_buf), GFP_KERNEL);
> 
> As we know, kzalloc can only allocate 4MB at max. So, secure heap has
> this limitation.
> can we have a way to allocate more memory in secure heap ? maybe
> similar to how system heap does?

This is just the size of a internal structure. I guess you mean the
secure memory size here. Regarding secure memory allocating flow, our
flow may be different with yours.

Let me explain our flow, we have two secure buffer types(heaps).
a) mtk_svp
b) mtk_svp_cma which requires the cma binding.

The memory management of both is inside the TEE. We only need to tell
the TEE which type and size of buffer we want, and then the TEE will
perform and return the memory handle to the kernel. The
kzalloc/alloc_pages is for the normal buffers.

Regarding the CMA buffer, we only call cma_alloc once, and its
management is also within the TEE.

> 
> Thanks,
> Vijay
>

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

On 10/20/2023 3:29 PM, Yong Wu (吴勇) wrote:
> 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:
>>> Initialise a mtk_svp 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           |  8 ++
>>>  drivers/dma-buf/heaps/Makefile          |  1 +
>>>  drivers/dma-buf/heaps/mtk_secure_heap.c | 99
>> +++++++++++++++++++++++++
>>>  3 files changed, 108 insertions(+)
>>>  create mode 100644 drivers/dma-buf/heaps/mtk_secure_heap.c
>>>
>>> diff --git a/drivers/dma-buf/heaps/Kconfig b/drivers/dma-
>> buf/heaps/Kconfig
>>> index a5eef06c4226..729c0cf3eb7c 100644
>>> --- a/drivers/dma-buf/heaps/Kconfig
>>> +++ b/drivers/dma-buf/heaps/Kconfig
>>> @@ -12,3 +12,11 @@ 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_MTK_SECURE
>>> +bool "DMA-BUF MediaTek Secure Heap"
>>> +depends on DMABUF_HEAPS && TEE
>>> +help
>>> +  Choose this option to enable dma-buf MediaTek secure heap for
>> Secure
>>> +  Video Path. This heap is backed by TEE client interfaces. If in
>>> +  doubt, say N.
>>> diff --git a/drivers/dma-buf/heaps/Makefile b/drivers/dma-
>> buf/heaps/Makefile
>>> index 974467791032..df559dbe33fe 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_SYSTEM)+= system_heap.o
>>>  obj-$(CONFIG_DMABUF_HEAPS_CMA)+= cma_heap.o
>>> +obj-$(CONFIG_DMABUF_HEAPS_MTK_SECURE)+= mtk_secure_heap.o
>>> diff --git a/drivers/dma-buf/heaps/mtk_secure_heap.c b/drivers/dma-
>> buf/heaps/mtk_secure_heap.c
>>> new file mode 100644
>>> index 000000000000..bbf1c8dce23e
>>> --- /dev/null
>>> +++ b/drivers/dma-buf/heaps/mtk_secure_heap.c
>>> @@ -0,0 +1,99 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/*
>>> + * DMABUF mtk_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>
>>> +
>>> +/*
>>> + * MediaTek secure (chunk) memory type
>>> + *
>>> + * @KREE_MEM_SEC_CM_TZ: static chunk memory carved out for
>> trustzone.
>>> + */
>>> +enum kree_mem_type {
>>> +KREE_MEM_SEC_CM_TZ = 1,
>>> +};
>>> +
>>> +struct mtk_secure_heap_buffer {
>>> +struct dma_heap*heap;
>>> +size_tsize;
>>> +};
>>> +
>>> +struct mtk_secure_heap {
>>> +const char*name;
>>> +const enum kree_mem_type mem_type;
>>> +};
>>> +
>>> +static struct dma_buf *
>>> +mtk_sec_heap_allocate(struct dma_heap *heap, size_t size,
>>> +      unsigned long fd_flags, unsigned long heap_flags)
>>> +{
>>> +struct mtk_secure_heap_buffer *sec_buf;
>>> +DEFINE_DMA_BUF_EXPORT_INFO(exp_info);
>>> +struct dma_buf *dmabuf;
>>> +int ret;
>>> +
>>> +sec_buf = kzalloc(sizeof(*sec_buf), GFP_KERNEL);
>>
>> As we know, kzalloc can only allocate 4MB at max. So, secure heap has
>> this limitation.
>> can we have a way to allocate more memory in secure heap ? maybe
>> similar to how system heap does?
> 
> This is just the size of a internal structure. I guess you mean the
> secure memory size here. Regarding secure memory allocating flow, our
> flow may be different with yours.
> 
> Let me explain our flow, we have two secure buffer types(heaps).
> a) mtk_svp
> b) mtk_svp_cma which requires the cma binding.
> 
> The memory management of both is inside the TEE. We only need to tell
> the TEE which type and size of buffer we want, and then the TEE will
> perform and return the memory handle to the kernel. The
> kzalloc/alloc_pages is for the normal buffers.
> 
> Regarding the CMA buffer, we only call cma_alloc once, and its
> management is also within the TEE.
> 

Thanks for the details.

I see for mvp_svp, allocation is also specific to TEE, as TEE takes
care of allocation as well. 

I was thinking if allocation path can also be made generic ? without having
dependency on TEE.
For eg : A case where we want to allocate from kernel and secure that memory,
the current secure heap design can't be used. 

Also i suppose TEE allocates contiguous memory for mtk_svp ? or does it support
scattered memory ?

>>
>> Thanks,
>> Vijay
>>

On Thu, 2023-10-26 at 10:18 +0530, Vijayanand Jitta wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  
> 
> On 10/20/2023 3:29 PM, Yong Wu (吴勇) wrote:
> > 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:
> >>> Initialise a mtk_svp 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           |  8 ++
> >>>  drivers/dma-buf/heaps/Makefile          |  1 +
> >>>  drivers/dma-buf/heaps/mtk_secure_heap.c | 99
> >> +++++++++++++++++++++++++

[...]

> >>> +
> >>> +static struct dma_buf *
> >>> +mtk_sec_heap_allocate(struct dma_heap *heap, size_t size,
> >>> +      unsigned long fd_flags, unsigned long heap_flags)
> >>> +{
> >>> +struct mtk_secure_heap_buffer *sec_buf;
> >>> +DEFINE_DMA_BUF_EXPORT_INFO(exp_info);
> >>> +struct dma_buf *dmabuf;
> >>> +int ret;
> >>> +
> >>> +sec_buf = kzalloc(sizeof(*sec_buf), GFP_KERNEL);
> >>
> >> As we know, kzalloc can only allocate 4MB at max. So, secure heap
> has
> >> this limitation.
> >> can we have a way to allocate more memory in secure heap ? maybe
> >> similar to how system heap does?
> > 
> > This is just the size of a internal structure. I guess you mean the
> > secure memory size here. Regarding secure memory allocating flow,
> our
> > flow may be different with yours.
> > 
> > Let me explain our flow, we have two secure buffer types(heaps).
> > a) mtk_svp
> > b) mtk_svp_cma which requires the cma binding.
> > 
> > The memory management of both is inside the TEE. We only need to
> tell
> > the TEE which type and size of buffer we want, and then the TEE
> will
> > perform and return the memory handle to the kernel. The
> > kzalloc/alloc_pages is for the normal buffers.
> > 
> > Regarding the CMA buffer, we only call cma_alloc once, and its
> > management is also within the TEE.
> > 
> 
> Thanks for the details.
> 
> I see for mvp_svp, allocation is also specific to TEE, as TEE takes
> care of allocation as well.

Yes. The allocation management of these two heaps is in the TEE.

> 
> I was thinking if allocation path can also be made generic ? without
> having
> dependency on TEE.
> For eg : A case where we want to allocate from kernel and secure that
> memory,
> the current secure heap design can't be used. 

Sorry, This may be because our HW is special. The HW could protect a
certain region, but it can only protect 32 regions. So we cannot
allocate them in the kernel arbitrarily and then enter TEE to protect
them.

> 
> Also i suppose TEE allocates contiguous memory for mtk_svp ? or does
> it support
> scattered memory ?

Yes. After the TEE runs for a period of time, the TEE memory will
become discontinuous, and a secure IOMMU exists in the TEE.

> >>
> >> Thanks,
> >> Vijay
> >>

On 10/27/2023 1:17 PM, Yong Wu (吴勇) wrote:
> On Thu, 2023-10-26 at 10:18 +0530, Vijayanand Jitta wrote:
>>  	 
>> External email : Please do not click links or open attachments until
>> you have verified the sender or the content.
>>  
>>
>> On 10/20/2023 3:29 PM, Yong Wu (吴勇) wrote:
>>> 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:
>>>>> Initialise a mtk_svp 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           |  8 ++
>>>>>  drivers/dma-buf/heaps/Makefile          |  1 +
>>>>>  drivers/dma-buf/heaps/mtk_secure_heap.c | 99
>>>> +++++++++++++++++++++++++
> 
> [...]
> 
>>>>> +
>>>>> +static struct dma_buf *
>>>>> +mtk_sec_heap_allocate(struct dma_heap *heap, size_t size,
>>>>> +      unsigned long fd_flags, unsigned long heap_flags)
>>>>> +{
>>>>> +struct mtk_secure_heap_buffer *sec_buf;
>>>>> +DEFINE_DMA_BUF_EXPORT_INFO(exp_info);
>>>>> +struct dma_buf *dmabuf;
>>>>> +int ret;
>>>>> +
>>>>> +sec_buf = kzalloc(sizeof(*sec_buf), GFP_KERNEL);
>>>>
>>>> As we know, kzalloc can only allocate 4MB at max. So, secure heap
>> has
>>>> this limitation.
>>>> can we have a way to allocate more memory in secure heap ? maybe
>>>> similar to how system heap does?
>>>
>>> This is just the size of a internal structure. I guess you mean the
>>> secure memory size here. Regarding secure memory allocating flow,
>> our
>>> flow may be different with yours.
>>>
>>> Let me explain our flow, we have two secure buffer types(heaps).
>>> a) mtk_svp
>>> b) mtk_svp_cma which requires the cma binding.
>>>
>>> The memory management of both is inside the TEE. We only need to
>> tell
>>> the TEE which type and size of buffer we want, and then the TEE
>> will
>>> perform and return the memory handle to the kernel. The
>>> kzalloc/alloc_pages is for the normal buffers.
>>>
>>> Regarding the CMA buffer, we only call cma_alloc once, and its
>>> management is also within the TEE.
>>>
>>
>> Thanks for the details.
>>
>> I see for mvp_svp, allocation is also specific to TEE, as TEE takes
>> care of allocation as well.
> 
> Yes. The allocation management of these two heaps is in the TEE.
> 
>>
>> I was thinking if allocation path can also be made generic ? without
>> having
>> dependency on TEE.
>> For eg : A case where we want to allocate from kernel and secure that
>> memory,
>> the current secure heap design can't be used. 
> 
> Sorry, This may be because our HW is special. The HW could protect a
> certain region, but it can only protect 32 regions. So we cannot
> allocate them in the kernel arbitrarily and then enter TEE to protect
> them.
> 

Got your point , I see for your case allocation must happen in TEE.
I was just saying if we want to make secure heap generic and remove
hard dependency on TEE, we must have a way to allocate irrespective
of what hypervisor/TZ being used. As current design for secure heap
assumes OPTEE.

We have a case where allocation happens in kernel and we secure it
using qcom_scm_assign_mem , this wouldn't be possible with current
design.

Probably some ops to allocate, similar to ops you pointed out to secure ?
in you case these ops would just allocate the internal structure.

Thanks,
Vijay

>>
>> Also i suppose TEE allocates contiguous memory for mtk_svp ? or does
>> it support
>> scattered memory ?
> 
> Yes. After the TEE runs for a period of time, the TEE memory will
> become discontinuous, and a secure IOMMU exists in the TEE.
> 
>>>>
>>>> Thanks,
>>>> Vijay
>>>>