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

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

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

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

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

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

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

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

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

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

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

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

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

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

Hi Rob,

Thanks for your review.

On Mon, 2023-09-11 at 10:44 -0500, Rob Herring 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:37AM +0800, Yong Wu wrote:

...

This adds the binding for describing a CMA memory for MediaTek

SVP(Secure

...

Video Path).

CMA is a Linux thing. How is this related to CMA?

...

Signed-off-by: Yong Wu yong.wu@mediatek.com

.../mediatek,secure_cma_chunkmem.yaml | 42

+++++++++++++++++++

...

1 file changed, 42 insertions(+) create mode 100644 Documentation/devicetree/bindings/reserved-

memory/mediatek,secure_cma_chunkmem.yaml

...

diff --git a/Documentation/devicetree/bindings/reserved-

memory/mediatek,secure_cma_chunkmem.yaml b/Documentation/devicetree/bindings/reserved- memory/mediatek,secure_cma_chunkmem.yaml

...

new file mode 100644 index 000000000000..cc10e00d35c4 --- /dev/null +++ b/Documentation/devicetree/bindings/reserved-

memory/mediatek,secure_cma_chunkmem.yaml

...

@@ -0,0 +1,42 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id:

http://devicetree.org/schemas/reserved-memory/mediatek,secure_cma_chunkmem.y...

...

+$schema: http://devicetree.org/meta-schemas/core.yaml#

+title: MediaTek Secure Video Path Reserved Memory

What makes this specific to Mediatek? Secure video path is fairly common, right?

Here we just reserve a buffer and would like to create a dma-buf secure heap for SVP, then the secure engines(Vcodec and DRM) could prepare secure buffer through it.

But the heap driver is pure SW driver, it is not platform device and we don't have a corresponding HW unit for it. Thus I don't think I could create a platform dtsi node and use "memory-region" pointer to the region. I used RESERVEDMEM_OF_DECLARE currently(The code is in [9/9]). Sorry if this is not right.

Then in our usage case, is there some similar method to do this? or any other suggestion?

Appreciate in advance.

...

+description:

• This binding describes the reserved memory for secure video

path.

...

+maintainers:

• • Yong Wu yong.wu@mediatek.com

+allOf:

• • $ref: reserved-memory.yaml

+properties:

• compatible:
• const: mediatek,secure_cma_chunkmem

+required:

• • compatible
• • reg
• • reusable

+unevaluatedProperties: false

+examples:

• • |
• reserved-memory {

•    #address-cells = <1>;
  

•    #size-cells = <1>;
  

•    ranges;
  

•    reserved-memory@80000000 {
  

•        compatible = "mediatek,secure_cma_chunkmem";
  

•        reusable;
  

•        reg = <0x80000000 0x18000000>;
  

•    };
  

• };

-- 2.25.1

On 12/09/2023 9:28 am, Krzysztof Kozlowski wrote:

On 12/09/2023 08:16, Yong Wu (吴勇) wrote:

...

Hi Rob,

Thanks for your review.

On Mon, 2023-09-11 at 10:44 -0500, Rob Herring 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:37AM +0800, Yong Wu wrote:

...

This adds the binding for describing a CMA memory for MediaTek

SVP(Secure

...

Video Path).

CMA is a Linux thing. How is this related to CMA?

...

...

Signed-off-by: Yong Wu yong.wu@mediatek.com

.../mediatek,secure_cma_chunkmem.yaml | 42

+++++++++++++++++++

...

1 file changed, 42 insertions(+) create mode 100644 Documentation/devicetree/bindings/reserved-

memory/mediatek,secure_cma_chunkmem.yaml

...

diff --git a/Documentation/devicetree/bindings/reserved-

memory/mediatek,secure_cma_chunkmem.yaml b/Documentation/devicetree/bindings/reserved- memory/mediatek,secure_cma_chunkmem.yaml

...

new file mode 100644 index 000000000000..cc10e00d35c4 --- /dev/null +++ b/Documentation/devicetree/bindings/reserved-

memory/mediatek,secure_cma_chunkmem.yaml

...

@@ -0,0 +1,42 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id:

http://devicetree.org/schemas/reserved-memory/mediatek,secure_cma_chunkmem.y...

...

+$schema: http://devicetree.org/meta-schemas/core.yaml#

+title: MediaTek Secure Video Path Reserved Memory

What makes this specific to Mediatek? Secure video path is fairly common, right?

Here we just reserve a buffer and would like to create a dma-buf secure heap for SVP, then the secure engines(Vcodec and DRM) could prepare secure buffer through it. But the heap driver is pure SW driver, it is not platform device and

All drivers are pure SW.

...

we don't have a corresponding HW unit for it. Thus I don't think I could create a platform dtsi node and use "memory-region" pointer to the region. I used RESERVEDMEM_OF_DECLARE currently(The code is in [9/9]). Sorry if this is not right.

If this is not for any hardware and you already understand this (since you cannot use other bindings) then you cannot have custom bindings for it either.

...

Then in our usage case, is there some similar method to do this? or any other suggestion?

Don't stuff software into DTS.

Aren't most reserved-memory bindings just software policy if you look at it that way, though? IIUC this is a pool of memory that is visible and available to the Non-Secure OS, but is fundamentally owned by the Secure TEE, and pages that the TEE allocates from it will become physically inaccessible to the OS. Thus the platform does impose constraints on how the Non-Secure OS may use it, and per the rest of the reserved-memory bindings, describing it as a "reusable" reservation seems entirely appropriate. If anything that's *more* platform-related and so DT-relevant than typical arbitrary reservations which just represent "save some memory to dedicate to a particular driver" and don't actually bear any relationship to firmware or hardware at all.

However, the fact that Linux's implementation of how to reuse reserved memory areas is called CMA is indeed still irrelevant and has no place in the binding itself.

Thanks, Robin.

On 12/09/2023 4:53 pm, Rob Herring wrote:

On Tue, Sep 12, 2023 at 11:13:50AM +0100, Robin Murphy wrote:

...

On 12/09/2023 9:28 am, Krzysztof Kozlowski wrote:

...

On 12/09/2023 08:16, Yong Wu (吴勇) wrote:

...

Hi Rob,

Thanks for your review.

On Mon, 2023-09-11 at 10:44 -0500, Rob Herring 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:37AM +0800, Yong Wu wrote:

...

This adds the binding for describing a CMA memory for MediaTek

SVP(Secure

...

Video Path).

CMA is a Linux thing. How is this related to CMA?

...

...

Signed-off-by: Yong Wu yong.wu@mediatek.com

.../mediatek,secure_cma_chunkmem.yaml | 42

+++++++++++++++++++

...

1 file changed, 42 insertions(+) create mode 100644 Documentation/devicetree/bindings/reserved-

memory/mediatek,secure_cma_chunkmem.yaml

...

diff --git a/Documentation/devicetree/bindings/reserved-

memory/mediatek,secure_cma_chunkmem.yaml b/Documentation/devicetree/bindings/reserved- memory/mediatek,secure_cma_chunkmem.yaml

...

new file mode 100644 index 000000000000..cc10e00d35c4 --- /dev/null +++ b/Documentation/devicetree/bindings/reserved-

memory/mediatek,secure_cma_chunkmem.yaml

...

@@ -0,0 +1,42 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id:

http://devicetree.org/schemas/reserved-memory/mediatek,secure_cma_chunkmem.y...

...

+$schema: http://devicetree.org/meta-schemas/core.yaml#

+title: MediaTek Secure Video Path Reserved Memory

What makes this specific to Mediatek? Secure video path is fairly common, right?

Here we just reserve a buffer and would like to create a dma-buf secure heap for SVP, then the secure engines(Vcodec and DRM) could prepare secure buffer through it. But the heap driver is pure SW driver, it is not platform device and

All drivers are pure SW.

...

we don't have a corresponding HW unit for it. Thus I don't think I could create a platform dtsi node and use "memory-region" pointer to the region. I used RESERVEDMEM_OF_DECLARE currently(The code is in [9/9]). Sorry if this is not right.

If this is not for any hardware and you already understand this (since you cannot use other bindings) then you cannot have custom bindings for it either.

...

Then in our usage case, is there some similar method to do this? or any other suggestion?

Don't stuff software into DTS.

Aren't most reserved-memory bindings just software policy if you look at it that way, though? IIUC this is a pool of memory that is visible and available to the Non-Secure OS, but is fundamentally owned by the Secure TEE, and pages that the TEE allocates from it will become physically inaccessible to the OS. Thus the platform does impose constraints on how the Non-Secure OS may use it, and per the rest of the reserved-memory bindings, describing it as a "reusable" reservation seems entirely appropriate. If anything that's *more* platform-related and so DT-relevant than typical arbitrary reservations which just represent "save some memory to dedicate to a particular driver" and don't actually bear any relationship to firmware or hardware at all.

Yes, a memory range defined by hardware or firmware is within scope of DT. (CMA at aribitrary address was questionable.)

My issue here is more that 'secure video memory' is not any way Mediatek specific. AIUI, it's a requirement from certain content providers for video playback to work. So why the Mediatek specific binding?