* [PATCH 1/7] tee: remove unused tee_shm_pool_alloc_res_mem()
2021-06-09 10:23 [PATCH 0/7] tee: shared memory updates Jens Wiklander
@ 2021-06-09 10:23 ` Jens Wiklander
2021-06-09 14:03 ` Tyler Hicks
2021-06-09 10:23 ` [PATCH 2/7] tee: simplify shm pool handling Jens Wiklander
` (5 subsequent siblings)
6 siblings, 1 reply; 19+ messages in thread
From: Jens Wiklander @ 2021-06-09 10:23 UTC (permalink / raw)
To: linux-kernel, linux-arm-kernel, op-tee
Cc: Sumit Garg, Herbert Xu, Sakkinen, Sasha Levin,
Thirupathaiah Annapureddy, Vikas Gupta, David S . Miller,
Tyler Hicks, Jens Wiklander
None of the drivers in the TEE subsystem uses
tee_shm_pool_alloc_res_mem() so remove the function.
Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
---
drivers/tee/tee_shm_pool.c | 56 --------------------------------------
include/linux/tee_drv.h | 30 --------------------
2 files changed, 86 deletions(-)
diff --git a/drivers/tee/tee_shm_pool.c b/drivers/tee/tee_shm_pool.c
index fcbb461fc59c..a9f9d50fd181 100644
--- a/drivers/tee/tee_shm_pool.c
+++ b/drivers/tee/tee_shm_pool.c
@@ -47,62 +47,6 @@ static const struct tee_shm_pool_mgr_ops pool_ops_generic = {
.destroy_poolmgr = pool_op_gen_destroy_poolmgr,
};
-/**
- * tee_shm_pool_alloc_res_mem() - Create a shared memory pool from reserved
- * memory range
- * @priv_info: Information for driver private shared memory pool
- * @dmabuf_info: Information for dma-buf shared memory pool
- *
- * Start and end of pools will must be page aligned.
- *
- * Allocation with the flag TEE_SHM_DMA_BUF set will use the range supplied
- * in @dmabuf, others will use the range provided by @priv.
- *
- * @returns pointer to a 'struct tee_shm_pool' or an ERR_PTR on failure.
- */
-struct tee_shm_pool *
-tee_shm_pool_alloc_res_mem(struct tee_shm_pool_mem_info *priv_info,
- struct tee_shm_pool_mem_info *dmabuf_info)
-{
- struct tee_shm_pool_mgr *priv_mgr;
- struct tee_shm_pool_mgr *dmabuf_mgr;
- void *rc;
-
- /*
- * Create the pool for driver private shared memory
- */
- rc = tee_shm_pool_mgr_alloc_res_mem(priv_info->vaddr, priv_info->paddr,
- priv_info->size,
- 3 /* 8 byte aligned */);
- if (IS_ERR(rc))
- return rc;
- priv_mgr = rc;
-
- /*
- * Create the pool for dma_buf shared memory
- */
- rc = tee_shm_pool_mgr_alloc_res_mem(dmabuf_info->vaddr,
- dmabuf_info->paddr,
- dmabuf_info->size, PAGE_SHIFT);
- if (IS_ERR(rc))
- goto err_free_priv_mgr;
- dmabuf_mgr = rc;
-
- rc = tee_shm_pool_alloc(priv_mgr, dmabuf_mgr);
- if (IS_ERR(rc))
- goto err_free_dmabuf_mgr;
-
- return rc;
-
-err_free_dmabuf_mgr:
- tee_shm_pool_mgr_destroy(dmabuf_mgr);
-err_free_priv_mgr:
- tee_shm_pool_mgr_destroy(priv_mgr);
-
- return rc;
-}
-EXPORT_SYMBOL_GPL(tee_shm_pool_alloc_res_mem);
-
struct tee_shm_pool_mgr *tee_shm_pool_mgr_alloc_res_mem(unsigned long vaddr,
phys_addr_t paddr,
size_t size,
diff --git a/include/linux/tee_drv.h b/include/linux/tee_drv.h
index 54269e47ac9a..53b9b2a13a87 100644
--- a/include/linux/tee_drv.h
+++ b/include/linux/tee_drv.h
@@ -272,36 +272,6 @@ static inline void tee_shm_pool_mgr_destroy(struct tee_shm_pool_mgr *poolm)
poolm->ops->destroy_poolmgr(poolm);
}
-/**
- * struct tee_shm_pool_mem_info - holds information needed to create a shared
- * memory pool
- * @vaddr: Virtual address of start of pool
- * @paddr: Physical address of start of pool
- * @size: Size in bytes of the pool
- */
-struct tee_shm_pool_mem_info {
- unsigned long vaddr;
- phys_addr_t paddr;
- size_t size;
-};
-
-/**
- * tee_shm_pool_alloc_res_mem() - Create a shared memory pool from reserved
- * memory range
- * @priv_info: Information for driver private shared memory pool
- * @dmabuf_info: Information for dma-buf shared memory pool
- *
- * Start and end of pools will must be page aligned.
- *
- * Allocation with the flag TEE_SHM_DMA_BUF set will use the range supplied
- * in @dmabuf, others will use the range provided by @priv.
- *
- * @returns pointer to a 'struct tee_shm_pool' or an ERR_PTR on failure.
- */
-struct tee_shm_pool *
-tee_shm_pool_alloc_res_mem(struct tee_shm_pool_mem_info *priv_info,
- struct tee_shm_pool_mem_info *dmabuf_info);
-
/**
* tee_shm_pool_free() - Free a shared memory pool
* @pool: The shared memory pool to free
--
2.31.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH 1/7] tee: remove unused tee_shm_pool_alloc_res_mem()
2021-06-09 10:23 ` [PATCH 1/7] tee: remove unused tee_shm_pool_alloc_res_mem() Jens Wiklander
@ 2021-06-09 14:03 ` Tyler Hicks
0 siblings, 0 replies; 19+ messages in thread
From: Tyler Hicks @ 2021-06-09 14:03 UTC (permalink / raw)
To: Jens Wiklander
Cc: linux-kernel, linux-arm-kernel, op-tee, Sumit Garg, Herbert Xu,
Sakkinen, Sasha Levin, Thirupathaiah Annapureddy, Vikas Gupta,
David S . Miller
On 2021-06-09 12:23:18, Jens Wiklander wrote:
> None of the drivers in the TEE subsystem uses
> tee_shm_pool_alloc_res_mem() so remove the function.
>
> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
Reviewed-by: Tyler Hicks <tyhicks@linux.microsoft.com>
Tyler
> ---
> drivers/tee/tee_shm_pool.c | 56 --------------------------------------
> include/linux/tee_drv.h | 30 --------------------
> 2 files changed, 86 deletions(-)
>
> diff --git a/drivers/tee/tee_shm_pool.c b/drivers/tee/tee_shm_pool.c
> index fcbb461fc59c..a9f9d50fd181 100644
> --- a/drivers/tee/tee_shm_pool.c
> +++ b/drivers/tee/tee_shm_pool.c
> @@ -47,62 +47,6 @@ static const struct tee_shm_pool_mgr_ops pool_ops_generic = {
> .destroy_poolmgr = pool_op_gen_destroy_poolmgr,
> };
>
> -/**
> - * tee_shm_pool_alloc_res_mem() - Create a shared memory pool from reserved
> - * memory range
> - * @priv_info: Information for driver private shared memory pool
> - * @dmabuf_info: Information for dma-buf shared memory pool
> - *
> - * Start and end of pools will must be page aligned.
> - *
> - * Allocation with the flag TEE_SHM_DMA_BUF set will use the range supplied
> - * in @dmabuf, others will use the range provided by @priv.
> - *
> - * @returns pointer to a 'struct tee_shm_pool' or an ERR_PTR on failure.
> - */
> -struct tee_shm_pool *
> -tee_shm_pool_alloc_res_mem(struct tee_shm_pool_mem_info *priv_info,
> - struct tee_shm_pool_mem_info *dmabuf_info)
> -{
> - struct tee_shm_pool_mgr *priv_mgr;
> - struct tee_shm_pool_mgr *dmabuf_mgr;
> - void *rc;
> -
> - /*
> - * Create the pool for driver private shared memory
> - */
> - rc = tee_shm_pool_mgr_alloc_res_mem(priv_info->vaddr, priv_info->paddr,
> - priv_info->size,
> - 3 /* 8 byte aligned */);
> - if (IS_ERR(rc))
> - return rc;
> - priv_mgr = rc;
> -
> - /*
> - * Create the pool for dma_buf shared memory
> - */
> - rc = tee_shm_pool_mgr_alloc_res_mem(dmabuf_info->vaddr,
> - dmabuf_info->paddr,
> - dmabuf_info->size, PAGE_SHIFT);
> - if (IS_ERR(rc))
> - goto err_free_priv_mgr;
> - dmabuf_mgr = rc;
> -
> - rc = tee_shm_pool_alloc(priv_mgr, dmabuf_mgr);
> - if (IS_ERR(rc))
> - goto err_free_dmabuf_mgr;
> -
> - return rc;
> -
> -err_free_dmabuf_mgr:
> - tee_shm_pool_mgr_destroy(dmabuf_mgr);
> -err_free_priv_mgr:
> - tee_shm_pool_mgr_destroy(priv_mgr);
> -
> - return rc;
> -}
> -EXPORT_SYMBOL_GPL(tee_shm_pool_alloc_res_mem);
> -
> struct tee_shm_pool_mgr *tee_shm_pool_mgr_alloc_res_mem(unsigned long vaddr,
> phys_addr_t paddr,
> size_t size,
> diff --git a/include/linux/tee_drv.h b/include/linux/tee_drv.h
> index 54269e47ac9a..53b9b2a13a87 100644
> --- a/include/linux/tee_drv.h
> +++ b/include/linux/tee_drv.h
> @@ -272,36 +272,6 @@ static inline void tee_shm_pool_mgr_destroy(struct tee_shm_pool_mgr *poolm)
> poolm->ops->destroy_poolmgr(poolm);
> }
>
> -/**
> - * struct tee_shm_pool_mem_info - holds information needed to create a shared
> - * memory pool
> - * @vaddr: Virtual address of start of pool
> - * @paddr: Physical address of start of pool
> - * @size: Size in bytes of the pool
> - */
> -struct tee_shm_pool_mem_info {
> - unsigned long vaddr;
> - phys_addr_t paddr;
> - size_t size;
> -};
> -
> -/**
> - * tee_shm_pool_alloc_res_mem() - Create a shared memory pool from reserved
> - * memory range
> - * @priv_info: Information for driver private shared memory pool
> - * @dmabuf_info: Information for dma-buf shared memory pool
> - *
> - * Start and end of pools will must be page aligned.
> - *
> - * Allocation with the flag TEE_SHM_DMA_BUF set will use the range supplied
> - * in @dmabuf, others will use the range provided by @priv.
> - *
> - * @returns pointer to a 'struct tee_shm_pool' or an ERR_PTR on failure.
> - */
> -struct tee_shm_pool *
> -tee_shm_pool_alloc_res_mem(struct tee_shm_pool_mem_info *priv_info,
> - struct tee_shm_pool_mem_info *dmabuf_info);
> -
> /**
> * tee_shm_pool_free() - Free a shared memory pool
> * @pool: The shared memory pool to free
> --
> 2.31.1
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 2/7] tee: simplify shm pool handling
2021-06-09 10:23 [PATCH 0/7] tee: shared memory updates Jens Wiklander
2021-06-09 10:23 ` [PATCH 1/7] tee: remove unused tee_shm_pool_alloc_res_mem() Jens Wiklander
@ 2021-06-09 10:23 ` Jens Wiklander
2021-06-09 14:50 ` Tyler Hicks
2021-06-09 10:23 ` [PATCH 3/7] tee: add tee_shm_alloc_kernel_buf() Jens Wiklander
` (4 subsequent siblings)
6 siblings, 1 reply; 19+ messages in thread
From: Jens Wiklander @ 2021-06-09 10:23 UTC (permalink / raw)
To: linux-kernel, linux-arm-kernel, op-tee
Cc: Sumit Garg, Herbert Xu, Sakkinen, Sasha Levin,
Thirupathaiah Annapureddy, Vikas Gupta, David S . Miller,
Tyler Hicks, Jens Wiklander
Replaces the shared memory pool based on two pools with a single pool.
The alloc() function pointer in struct tee_shm_pool_ops gets another
parameter, align. This makes it possible to make less than page aligned
allocations from the optional reserved shared memory pool while still
making user space allocations page aligned. With in practice unchanged
behaviour using only a single pool for bookkeeping.
The optee and amdtee drivers are updated as needed to work with this
changed pool handling.
This also removes OPTEE_SHM_NUM_PRIV_PAGES which becomes obsolete with
this change as the private pages can be mixed with the payload pages.
Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
---
drivers/tee/amdtee/shm_pool.c | 55 ++++++------------
drivers/tee/optee/Kconfig | 8 ---
drivers/tee/optee/core.c | 72 +++--------------------
drivers/tee/optee/shm_pool.c | 51 ++++++++++-------
drivers/tee/optee/shm_pool.h | 2 +-
drivers/tee/tee_private.h | 11 ----
drivers/tee/tee_shm.c | 29 +++++-----
drivers/tee/tee_shm_pool.c | 104 +++++++++++-----------------------
include/linux/tee_drv.h | 58 +++++++------------
9 files changed, 120 insertions(+), 270 deletions(-)
diff --git a/drivers/tee/amdtee/shm_pool.c b/drivers/tee/amdtee/shm_pool.c
index 065854e2db18..81c23f30b455 100644
--- a/drivers/tee/amdtee/shm_pool.c
+++ b/drivers/tee/amdtee/shm_pool.c
@@ -8,13 +8,17 @@
#include <linux/psp-sev.h>
#include "amdtee_private.h"
-static int pool_op_alloc(struct tee_shm_pool_mgr *poolm, struct tee_shm *shm,
- size_t size)
+static int pool_op_alloc(struct tee_shm_pool *pool, struct tee_shm *shm,
+ size_t size, u_int align)
{
unsigned int order = get_order(size);
unsigned long va;
int rc;
+ /*
+ * Ignore alignment since this is already going to be page aligned
+ * and there's no need for any larger alignment.
+ */
va = __get_free_pages(GFP_KERNEL | __GFP_ZERO, order);
if (!va)
return -ENOMEM;
@@ -34,7 +38,7 @@ static int pool_op_alloc(struct tee_shm_pool_mgr *poolm, struct tee_shm *shm,
return 0;
}
-static void pool_op_free(struct tee_shm_pool_mgr *poolm, struct tee_shm *shm)
+static void pool_op_free(struct tee_shm_pool *pool, struct tee_shm *shm)
{
/* Unmap the shared memory from TEE */
amdtee_unmap_shmem(shm);
@@ -42,52 +46,25 @@ static void pool_op_free(struct tee_shm_pool_mgr *poolm, struct tee_shm *shm)
shm->kaddr = NULL;
}
-static void pool_op_destroy_poolmgr(struct tee_shm_pool_mgr *poolm)
+static void pool_op_destroy_pool(struct tee_shm_pool *pool)
{
- kfree(poolm);
+ kfree(pool);
}
-static const struct tee_shm_pool_mgr_ops pool_ops = {
+static const struct tee_shm_pool_ops pool_ops = {
.alloc = pool_op_alloc,
.free = pool_op_free,
- .destroy_poolmgr = pool_op_destroy_poolmgr,
+ .destroy_pool = pool_op_destroy_pool,
};
-static struct tee_shm_pool_mgr *pool_mem_mgr_alloc(void)
-{
- struct tee_shm_pool_mgr *mgr = kzalloc(sizeof(*mgr), GFP_KERNEL);
-
- if (!mgr)
- return ERR_PTR(-ENOMEM);
-
- mgr->ops = &pool_ops;
-
- return mgr;
-}
-
struct tee_shm_pool *amdtee_config_shm(void)
{
- struct tee_shm_pool_mgr *priv_mgr;
- struct tee_shm_pool_mgr *dmabuf_mgr;
- void *rc;
+ struct tee_shm_pool *pool = kzalloc(sizeof(*pool), GFP_KERNEL);
- rc = pool_mem_mgr_alloc();
- if (IS_ERR(rc))
- return rc;
- priv_mgr = rc;
-
- rc = pool_mem_mgr_alloc();
- if (IS_ERR(rc)) {
- tee_shm_pool_mgr_destroy(priv_mgr);
- return rc;
- }
- dmabuf_mgr = rc;
+ if (!pool)
+ return ERR_PTR(-ENOMEM);
- rc = tee_shm_pool_alloc(priv_mgr, dmabuf_mgr);
- if (IS_ERR(rc)) {
- tee_shm_pool_mgr_destroy(priv_mgr);
- tee_shm_pool_mgr_destroy(dmabuf_mgr);
- }
+ pool->ops = &pool_ops;
- return rc;
+ return pool;
}
diff --git a/drivers/tee/optee/Kconfig b/drivers/tee/optee/Kconfig
index 3ca71e3812ed..f121c224e682 100644
--- a/drivers/tee/optee/Kconfig
+++ b/drivers/tee/optee/Kconfig
@@ -7,11 +7,3 @@ config OPTEE
help
This implements the OP-TEE Trusted Execution Environment (TEE)
driver.
-
-config OPTEE_SHM_NUM_PRIV_PAGES
- int "Private Shared Memory Pages"
- default 1
- depends on OPTEE
- help
- This sets the number of private shared memory pages to be
- used by OP-TEE TEE driver.
diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c
index ddb8f9ecf307..0c287345f9fe 100644
--- a/drivers/tee/optee/core.c
+++ b/drivers/tee/optee/core.c
@@ -428,33 +428,6 @@ static bool optee_msg_exchange_capabilities(optee_invoke_fn *invoke_fn,
return true;
}
-static struct tee_shm_pool *optee_config_dyn_shm(void)
-{
- struct tee_shm_pool_mgr *priv_mgr;
- struct tee_shm_pool_mgr *dmabuf_mgr;
- void *rc;
-
- rc = optee_shm_pool_alloc_pages();
- if (IS_ERR(rc))
- return rc;
- priv_mgr = rc;
-
- rc = optee_shm_pool_alloc_pages();
- if (IS_ERR(rc)) {
- tee_shm_pool_mgr_destroy(priv_mgr);
- return rc;
- }
- dmabuf_mgr = rc;
-
- rc = tee_shm_pool_alloc(priv_mgr, dmabuf_mgr);
- if (IS_ERR(rc)) {
- tee_shm_pool_mgr_destroy(priv_mgr);
- tee_shm_pool_mgr_destroy(dmabuf_mgr);
- }
-
- return rc;
-}
-
static struct tee_shm_pool *
optee_config_shm_memremap(optee_invoke_fn *invoke_fn, void **memremaped_shm)
{
@@ -468,10 +441,7 @@ optee_config_shm_memremap(optee_invoke_fn *invoke_fn, void **memremaped_shm)
phys_addr_t begin;
phys_addr_t end;
void *va;
- struct tee_shm_pool_mgr *priv_mgr;
- struct tee_shm_pool_mgr *dmabuf_mgr;
void *rc;
- const int sz = OPTEE_SHM_NUM_PRIV_PAGES * PAGE_SIZE;
invoke_fn(OPTEE_SMC_GET_SHM_CONFIG, 0, 0, 0, 0, 0, 0, 0, &res.smccc);
if (res.result.status != OPTEE_SMC_RETURN_OK) {
@@ -489,11 +459,6 @@ optee_config_shm_memremap(optee_invoke_fn *invoke_fn, void **memremaped_shm)
paddr = begin;
size = end - begin;
- if (size < 2 * OPTEE_SHM_NUM_PRIV_PAGES * PAGE_SIZE) {
- pr_err("too small shared memory area\n");
- return ERR_PTR(-EINVAL);
- }
-
va = memremap(paddr, size, MEMREMAP_WB);
if (!va) {
pr_err("shared memory ioremap failed\n");
@@ -501,35 +466,13 @@ optee_config_shm_memremap(optee_invoke_fn *invoke_fn, void **memremaped_shm)
}
vaddr = (unsigned long)va;
- rc = tee_shm_pool_mgr_alloc_res_mem(vaddr, paddr, sz,
- 3 /* 8 bytes aligned */);
- if (IS_ERR(rc))
- goto err_memunmap;
- priv_mgr = rc;
-
- vaddr += sz;
- paddr += sz;
- size -= sz;
-
- rc = tee_shm_pool_mgr_alloc_res_mem(vaddr, paddr, size, PAGE_SHIFT);
+ rc = tee_shm_pool_alloc_res_mem(vaddr, paddr, size,
+ 9 /* 512 bytes aligned */);
if (IS_ERR(rc))
- goto err_free_priv_mgr;
- dmabuf_mgr = rc;
-
- rc = tee_shm_pool_alloc(priv_mgr, dmabuf_mgr);
- if (IS_ERR(rc))
- goto err_free_dmabuf_mgr;
-
- *memremaped_shm = va;
-
- return rc;
+ memunmap(va);
+ else
+ *memremaped_shm = va;
-err_free_dmabuf_mgr:
- tee_shm_pool_mgr_destroy(dmabuf_mgr);
-err_free_priv_mgr:
- tee_shm_pool_mgr_destroy(priv_mgr);
-err_memunmap:
- memunmap(va);
return rc;
}
@@ -637,7 +580,7 @@ static int optee_probe(struct platform_device *pdev)
* Try to use dynamic shared memory if possible
*/
if (sec_caps & OPTEE_SMC_SEC_CAP_DYNAMIC_SHM)
- pool = optee_config_dyn_shm();
+ pool = optee_shm_pool_alloc_pages();
/*
* If dynamic shared memory is not available or failed - try static one
@@ -712,8 +655,7 @@ static int optee_probe(struct platform_device *pdev)
tee_device_unregister(optee->teedev);
kfree(optee);
}
- if (pool)
- tee_shm_pool_free(pool);
+ tee_shm_pool_free(pool);
if (memremaped_shm)
memunmap(memremaped_shm);
return rc;
diff --git a/drivers/tee/optee/shm_pool.c b/drivers/tee/optee/shm_pool.c
index d767eebf30bd..b036714845c1 100644
--- a/drivers/tee/optee/shm_pool.c
+++ b/drivers/tee/optee/shm_pool.c
@@ -12,13 +12,17 @@
#include "optee_smc.h"
#include "shm_pool.h"
-static int pool_op_alloc(struct tee_shm_pool_mgr *poolm,
- struct tee_shm *shm, size_t size)
+static int pool_op_alloc(struct tee_shm_pool *pool,
+ struct tee_shm *shm, size_t size, u_int align)
{
unsigned int order = get_order(size);
struct page *page;
int rc = 0;
+ /*
+ * Ignore alignment since this is already going to be page aligned
+ * and there's no need for any larger alignment.
+ */
page = alloc_pages(GFP_KERNEL | __GFP_ZERO, order);
if (!page)
return -ENOMEM;
@@ -27,47 +31,52 @@ static int pool_op_alloc(struct tee_shm_pool_mgr *poolm,
shm->paddr = page_to_phys(page);
shm->size = PAGE_SIZE << order;
- if (shm->flags & TEE_SHM_DMA_BUF) {
+ if (shm->flags & TEE_SHM_REGISTER) {
unsigned int nr_pages = 1 << order, i;
struct page **pages;
pages = kcalloc(nr_pages, sizeof(pages), GFP_KERNEL);
- if (!pages)
- return -ENOMEM;
-
- for (i = 0; i < nr_pages; i++) {
- pages[i] = page;
- page++;
+ if (!pages) {
+ rc = -ENOMEM;
+ goto err;
}
- shm->flags |= TEE_SHM_REGISTER;
+ for (i = 0; i < nr_pages; i++)
+ pages[i] = page + i;
+
rc = optee_shm_register(shm->ctx, shm, pages, nr_pages,
(unsigned long)shm->kaddr);
kfree(pages);
+ if (rc)
+ goto err;
}
+ return 0;
+
+err:
+ free_pages((unsigned long)shm->kaddr, order);
return rc;
}
-static void pool_op_free(struct tee_shm_pool_mgr *poolm,
+static void pool_op_free(struct tee_shm_pool *pool,
struct tee_shm *shm)
{
- if (shm->flags & TEE_SHM_DMA_BUF)
+ if (shm->flags & TEE_SHM_REGISTER)
optee_shm_unregister(shm->ctx, shm);
free_pages((unsigned long)shm->kaddr, get_order(shm->size));
shm->kaddr = NULL;
}
-static void pool_op_destroy_poolmgr(struct tee_shm_pool_mgr *poolm)
+static void pool_op_destroy_pool(struct tee_shm_pool *pool)
{
- kfree(poolm);
+ kfree(pool);
}
-static const struct tee_shm_pool_mgr_ops pool_ops = {
+static const struct tee_shm_pool_ops pool_ops = {
.alloc = pool_op_alloc,
.free = pool_op_free,
- .destroy_poolmgr = pool_op_destroy_poolmgr,
+ .destroy_pool = pool_op_destroy_pool,
};
/**
@@ -76,14 +85,14 @@ static const struct tee_shm_pool_mgr_ops pool_ops = {
* This pool is used when OP-TEE supports dymanic SHM. In this case
* command buffers and such are allocated from kernel's own memory.
*/
-struct tee_shm_pool_mgr *optee_shm_pool_alloc_pages(void)
+struct tee_shm_pool *optee_shm_pool_alloc_pages(void)
{
- struct tee_shm_pool_mgr *mgr = kzalloc(sizeof(*mgr), GFP_KERNEL);
+ struct tee_shm_pool *pool = kzalloc(sizeof(*pool), GFP_KERNEL);
- if (!mgr)
+ if (!pool)
return ERR_PTR(-ENOMEM);
- mgr->ops = &pool_ops;
+ pool->ops = &pool_ops;
- return mgr;
+ return pool;
}
diff --git a/drivers/tee/optee/shm_pool.h b/drivers/tee/optee/shm_pool.h
index 28109d991c4b..7024b9926ada 100644
--- a/drivers/tee/optee/shm_pool.h
+++ b/drivers/tee/optee/shm_pool.h
@@ -9,6 +9,6 @@
#include <linux/tee_drv.h>
-struct tee_shm_pool_mgr *optee_shm_pool_alloc_pages(void);
+struct tee_shm_pool *optee_shm_pool_alloc_pages(void);
#endif
diff --git a/drivers/tee/tee_private.h b/drivers/tee/tee_private.h
index e55204df31ce..72376cf38bc0 100644
--- a/drivers/tee/tee_private.h
+++ b/drivers/tee/tee_private.h
@@ -12,17 +12,6 @@
#include <linux/mutex.h>
#include <linux/types.h>
-/**
- * struct tee_shm_pool - shared memory pool
- * @private_mgr: pool manager for shared memory only between kernel
- * and secure world
- * @dma_buf_mgr: pool manager for shared memory exported to user space
- */
-struct tee_shm_pool {
- struct tee_shm_pool_mgr *private_mgr;
- struct tee_shm_pool_mgr *dma_buf_mgr;
-};
-
#define TEE_DEVICE_FLAG_REGISTERED 0x1
#define TEE_MAX_DEV_NAME_LEN 32
diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c
index 00472f5ce22e..b9dbf4bce149 100644
--- a/drivers/tee/tee_shm.c
+++ b/drivers/tee/tee_shm.c
@@ -39,14 +39,7 @@ static void tee_shm_release(struct tee_shm *shm)
}
if (shm->flags & TEE_SHM_POOL) {
- struct tee_shm_pool_mgr *poolm;
-
- if (shm->flags & TEE_SHM_DMA_BUF)
- poolm = teedev->pool->dma_buf_mgr;
- else
- poolm = teedev->pool->private_mgr;
-
- poolm->ops->free(poolm, shm);
+ teedev->pool->ops->free(teedev->pool, shm);
} else if (shm->flags & TEE_SHM_REGISTER) {
int rc = teedev->desc->ops->shm_unregister(shm->ctx, shm);
@@ -106,8 +99,8 @@ static const struct dma_buf_ops tee_shm_dma_buf_ops = {
struct tee_shm *tee_shm_alloc(struct tee_context *ctx, size_t size, u32 flags)
{
struct tee_device *teedev = ctx->teedev;
- struct tee_shm_pool_mgr *poolm = NULL;
struct tee_shm *shm;
+ size_t align;
void *ret;
int rc;
@@ -139,12 +132,18 @@ struct tee_shm *tee_shm_alloc(struct tee_context *ctx, size_t size, u32 flags)
shm->flags = flags | TEE_SHM_POOL;
shm->ctx = ctx;
- if (flags & TEE_SHM_DMA_BUF)
- poolm = teedev->pool->dma_buf_mgr;
- else
- poolm = teedev->pool->private_mgr;
+ if (flags & TEE_SHM_DMA_BUF) {
+ align = PAGE_SIZE;
+ /*
+ * Request to register the shm in the pool allocator below
+ * if supported.
+ */
+ shm->flags |= TEE_SHM_REGISTER;
+ } else {
+ align = 2 * sizeof(long);
+ }
- rc = poolm->ops->alloc(poolm, shm, size);
+ rc = teedev->pool->ops->alloc(teedev->pool, shm, size, align);
if (rc) {
ret = ERR_PTR(rc);
goto err_kfree;
@@ -184,7 +183,7 @@ struct tee_shm *tee_shm_alloc(struct tee_context *ctx, size_t size, u32 flags)
mutex_unlock(&teedev->mutex);
}
err_pool_free:
- poolm->ops->free(poolm, shm);
+ teedev->pool->ops->free(teedev->pool, shm);
err_kfree:
kfree(shm);
err_dev_put:
diff --git a/drivers/tee/tee_shm_pool.c b/drivers/tee/tee_shm_pool.c
index a9f9d50fd181..5ceab73ff1b5 100644
--- a/drivers/tee/tee_shm_pool.c
+++ b/drivers/tee/tee_shm_pool.c
@@ -9,14 +9,16 @@
#include <linux/tee_drv.h>
#include "tee_private.h"
-static int pool_op_gen_alloc(struct tee_shm_pool_mgr *poolm,
- struct tee_shm *shm, size_t size)
+static int pool_op_gen_alloc(struct tee_shm_pool *pool, struct tee_shm *shm,
+ size_t size, u_int align)
{
unsigned long va;
- struct gen_pool *genpool = poolm->private_data;
- size_t s = roundup(size, 1 << genpool->min_alloc_order);
+ struct gen_pool *genpool = pool->private_data;
+ size_t a = max(align, 1U << genpool->min_alloc_order);
+ struct genpool_data_align data = { .align = a };
+ size_t s = roundup(size, a);
- va = gen_pool_alloc(genpool, s);
+ va = gen_pool_alloc_algo(genpool, s, gen_pool_first_fit_align, &data);
if (!va)
return -ENOMEM;
@@ -24,107 +26,67 @@ static int pool_op_gen_alloc(struct tee_shm_pool_mgr *poolm,
shm->kaddr = (void *)va;
shm->paddr = gen_pool_virt_to_phys(genpool, va);
shm->size = s;
+ /*
+ * This is from a static shared memory pool so no need to register
+ * each chunk, and no need to unregister later either.
+ */
+ shm->flags &= ~TEE_SHM_REGISTER;
return 0;
}
-static void pool_op_gen_free(struct tee_shm_pool_mgr *poolm,
- struct tee_shm *shm)
+static void pool_op_gen_free(struct tee_shm_pool *pool, struct tee_shm *shm)
{
- gen_pool_free(poolm->private_data, (unsigned long)shm->kaddr,
+ gen_pool_free(pool->private_data, (unsigned long)shm->kaddr,
shm->size);
shm->kaddr = NULL;
}
-static void pool_op_gen_destroy_poolmgr(struct tee_shm_pool_mgr *poolm)
+static void pool_op_gen_destroy_pool(struct tee_shm_pool *pool)
{
- gen_pool_destroy(poolm->private_data);
- kfree(poolm);
+ gen_pool_destroy(pool->private_data);
+ kfree(pool);
}
-static const struct tee_shm_pool_mgr_ops pool_ops_generic = {
+static const struct tee_shm_pool_ops pool_ops_generic = {
.alloc = pool_op_gen_alloc,
.free = pool_op_gen_free,
- .destroy_poolmgr = pool_op_gen_destroy_poolmgr,
+ .destroy_pool = pool_op_gen_destroy_pool,
};
-struct tee_shm_pool_mgr *tee_shm_pool_mgr_alloc_res_mem(unsigned long vaddr,
- phys_addr_t paddr,
- size_t size,
- int min_alloc_order)
+struct tee_shm_pool *tee_shm_pool_alloc_res_mem(unsigned long vaddr,
+ phys_addr_t paddr, size_t size,
+ int min_alloc_order)
{
const size_t page_mask = PAGE_SIZE - 1;
- struct tee_shm_pool_mgr *mgr;
+ struct tee_shm_pool *pool;
int rc;
/* Start and end must be page aligned */
if (vaddr & page_mask || paddr & page_mask || size & page_mask)
return ERR_PTR(-EINVAL);
- mgr = kzalloc(sizeof(*mgr), GFP_KERNEL);
- if (!mgr)
+ pool = kzalloc(sizeof(*pool), GFP_KERNEL);
+ if (!pool)
return ERR_PTR(-ENOMEM);
- mgr->private_data = gen_pool_create(min_alloc_order, -1);
- if (!mgr->private_data) {
+ pool->private_data = gen_pool_create(min_alloc_order, -1);
+ if (!pool->private_data) {
rc = -ENOMEM;
goto err;
}
- gen_pool_set_algo(mgr->private_data, gen_pool_best_fit, NULL);
- rc = gen_pool_add_virt(mgr->private_data, vaddr, paddr, size, -1);
+ rc = gen_pool_add_virt(pool->private_data, vaddr, paddr, size, -1);
if (rc) {
- gen_pool_destroy(mgr->private_data);
+ gen_pool_destroy(pool->private_data);
goto err;
}
- mgr->ops = &pool_ops_generic;
+ pool->ops = &pool_ops_generic;
- return mgr;
+ return pool;
err:
- kfree(mgr);
+ kfree(pool);
return ERR_PTR(rc);
}
-EXPORT_SYMBOL_GPL(tee_shm_pool_mgr_alloc_res_mem);
-
-static bool check_mgr_ops(struct tee_shm_pool_mgr *mgr)
-{
- return mgr && mgr->ops && mgr->ops->alloc && mgr->ops->free &&
- mgr->ops->destroy_poolmgr;
-}
-
-struct tee_shm_pool *tee_shm_pool_alloc(struct tee_shm_pool_mgr *priv_mgr,
- struct tee_shm_pool_mgr *dmabuf_mgr)
-{
- struct tee_shm_pool *pool;
-
- if (!check_mgr_ops(priv_mgr) || !check_mgr_ops(dmabuf_mgr))
- return ERR_PTR(-EINVAL);
-
- pool = kzalloc(sizeof(*pool), GFP_KERNEL);
- if (!pool)
- return ERR_PTR(-ENOMEM);
-
- pool->private_mgr = priv_mgr;
- pool->dma_buf_mgr = dmabuf_mgr;
-
- return pool;
-}
-EXPORT_SYMBOL_GPL(tee_shm_pool_alloc);
-
-/**
- * tee_shm_pool_free() - Free a shared memory pool
- * @pool: The shared memory pool to free
- *
- * There must be no remaining shared memory allocated from this pool when
- * this function is called.
- */
-void tee_shm_pool_free(struct tee_shm_pool *pool)
-{
- if (pool->private_mgr)
- tee_shm_pool_mgr_destroy(pool->private_mgr);
- if (pool->dma_buf_mgr)
- tee_shm_pool_mgr_destroy(pool->dma_buf_mgr);
- kfree(pool);
-}
-EXPORT_SYMBOL_GPL(tee_shm_pool_free);
+EXPORT_SYMBOL_GPL(tee_shm_pool_alloc_res_mem);
diff --git a/include/linux/tee_drv.h b/include/linux/tee_drv.h
index 53b9b2a13a87..62b7c7a55743 100644
--- a/include/linux/tee_drv.h
+++ b/include/linux/tee_drv.h
@@ -215,62 +215,39 @@ struct tee_shm {
};
/**
- * struct tee_shm_pool_mgr - shared memory manager
+ * struct tee_shm_pool - shared memory pool
* @ops: operations
* @private_data: private data for the shared memory manager
*/
-struct tee_shm_pool_mgr {
- const struct tee_shm_pool_mgr_ops *ops;
+struct tee_shm_pool {
+ const struct tee_shm_pool_ops *ops;
void *private_data;
};
/**
- * struct tee_shm_pool_mgr_ops - shared memory pool manager operations
+ * struct tee_shm_pool_ops - shared memory pool operations
* @alloc: called when allocating shared memory
* @free: called when freeing shared memory
- * @destroy_poolmgr: called when destroying the pool manager
+ * @destroy_pool: called when destroying the pool
*/
-struct tee_shm_pool_mgr_ops {
- int (*alloc)(struct tee_shm_pool_mgr *poolmgr, struct tee_shm *shm,
- size_t size);
- void (*free)(struct tee_shm_pool_mgr *poolmgr, struct tee_shm *shm);
- void (*destroy_poolmgr)(struct tee_shm_pool_mgr *poolmgr);
+struct tee_shm_pool_ops {
+ int (*alloc)(struct tee_shm_pool *pool, struct tee_shm *shm,
+ size_t size, u_int align);
+ void (*free)(struct tee_shm_pool *pool, struct tee_shm *shm);
+ void (*destroy_pool)(struct tee_shm_pool *pool);
};
-/**
- * tee_shm_pool_alloc() - Create a shared memory pool from shm managers
- * @priv_mgr: manager for driver private shared memory allocations
- * @dmabuf_mgr: manager for dma-buf shared memory allocations
- *
- * Allocation with the flag TEE_SHM_DMA_BUF set will use the range supplied
- * in @dmabuf, others will use the range provided by @priv.
- *
- * @returns pointer to a 'struct tee_shm_pool' or an ERR_PTR on failure.
- */
-struct tee_shm_pool *tee_shm_pool_alloc(struct tee_shm_pool_mgr *priv_mgr,
- struct tee_shm_pool_mgr *dmabuf_mgr);
-
/*
- * tee_shm_pool_mgr_alloc_res_mem() - Create a shm manager for reserved
- * memory
+ * tee_shm_pool_alloc_res_mem() - Create a shm manager for reserved memory
* @vaddr: Virtual address of start of pool
* @paddr: Physical address of start of pool
* @size: Size in bytes of the pool
*
- * @returns pointer to a 'struct tee_shm_pool_mgr' or an ERR_PTR on failure.
- */
-struct tee_shm_pool_mgr *tee_shm_pool_mgr_alloc_res_mem(unsigned long vaddr,
- phys_addr_t paddr,
- size_t size,
- int min_alloc_order);
-
-/**
- * tee_shm_pool_mgr_destroy() - Free a shared memory manager
+ * @returns pointer to a 'struct tee_shm_pool' or an ERR_PTR on failure.
*/
-static inline void tee_shm_pool_mgr_destroy(struct tee_shm_pool_mgr *poolm)
-{
- poolm->ops->destroy_poolmgr(poolm);
-}
+struct tee_shm_pool *tee_shm_pool_alloc_res_mem(unsigned long vaddr,
+ phys_addr_t paddr, size_t size,
+ int min_alloc_order);
/**
* tee_shm_pool_free() - Free a shared memory pool
@@ -279,7 +256,10 @@ static inline void tee_shm_pool_mgr_destroy(struct tee_shm_pool_mgr *poolm)
* The must be no remaining shared memory allocated from this pool when
* this function is called.
*/
-void tee_shm_pool_free(struct tee_shm_pool *pool);
+static inline void tee_shm_pool_free(struct tee_shm_pool *pool)
+{
+ pool->ops->destroy_pool(pool);
+}
/**
* tee_get_drvdata() - Return driver_data pointer
--
2.31.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH 2/7] tee: simplify shm pool handling
2021-06-09 10:23 ` [PATCH 2/7] tee: simplify shm pool handling Jens Wiklander
@ 2021-06-09 14:50 ` Tyler Hicks
2021-06-10 9:01 ` Jens Wiklander
0 siblings, 1 reply; 19+ messages in thread
From: Tyler Hicks @ 2021-06-09 14:50 UTC (permalink / raw)
To: Jens Wiklander
Cc: linux-kernel, linux-arm-kernel, op-tee, Sumit Garg, Herbert Xu,
Sakkinen, Sasha Levin, Thirupathaiah Annapureddy, Vikas Gupta,
David S . Miller
On 2021-06-09 12:23:19, Jens Wiklander wrote:
> Replaces the shared memory pool based on two pools with a single pool.
> The alloc() function pointer in struct tee_shm_pool_ops gets another
> parameter, align. This makes it possible to make less than page aligned
> allocations from the optional reserved shared memory pool while still
> making user space allocations page aligned. With in practice unchanged
> behaviour using only a single pool for bookkeeping.
>
> The optee and amdtee drivers are updated as needed to work with this
> changed pool handling.
>
> This also removes OPTEE_SHM_NUM_PRIV_PAGES which becomes obsolete with
> this change as the private pages can be mixed with the payload pages.
>
> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> ---
> drivers/tee/amdtee/shm_pool.c | 55 ++++++------------
> drivers/tee/optee/Kconfig | 8 ---
> drivers/tee/optee/core.c | 72 +++--------------------
> drivers/tee/optee/shm_pool.c | 51 ++++++++++-------
> drivers/tee/optee/shm_pool.h | 2 +-
> drivers/tee/tee_private.h | 11 ----
> drivers/tee/tee_shm.c | 29 +++++-----
> drivers/tee/tee_shm_pool.c | 104 +++++++++++-----------------------
> include/linux/tee_drv.h | 58 +++++++------------
> 9 files changed, 120 insertions(+), 270 deletions(-)
>
> diff --git a/drivers/tee/amdtee/shm_pool.c b/drivers/tee/amdtee/shm_pool.c
> index 065854e2db18..81c23f30b455 100644
> --- a/drivers/tee/amdtee/shm_pool.c
> +++ b/drivers/tee/amdtee/shm_pool.c
> @@ -8,13 +8,17 @@
> #include <linux/psp-sev.h>
> #include "amdtee_private.h"
>
> -static int pool_op_alloc(struct tee_shm_pool_mgr *poolm, struct tee_shm *shm,
> - size_t size)
> +static int pool_op_alloc(struct tee_shm_pool *pool, struct tee_shm *shm,
> + size_t size, u_int align)
> {
> unsigned int order = get_order(size);
> unsigned long va;
> int rc;
>
> + /*
> + * Ignore alignment since this is already going to be page aligned
> + * and there's no need for any larger alignment.
> + */
> va = __get_free_pages(GFP_KERNEL | __GFP_ZERO, order);
> if (!va)
> return -ENOMEM;
> @@ -34,7 +38,7 @@ static int pool_op_alloc(struct tee_shm_pool_mgr *poolm, struct tee_shm *shm,
> return 0;
> }
>
> -static void pool_op_free(struct tee_shm_pool_mgr *poolm, struct tee_shm *shm)
> +static void pool_op_free(struct tee_shm_pool *pool, struct tee_shm *shm)
> {
> /* Unmap the shared memory from TEE */
> amdtee_unmap_shmem(shm);
> @@ -42,52 +46,25 @@ static void pool_op_free(struct tee_shm_pool_mgr *poolm, struct tee_shm *shm)
> shm->kaddr = NULL;
> }
>
> -static void pool_op_destroy_poolmgr(struct tee_shm_pool_mgr *poolm)
> +static void pool_op_destroy_pool(struct tee_shm_pool *pool)
> {
> - kfree(poolm);
> + kfree(pool);
> }
>
> -static const struct tee_shm_pool_mgr_ops pool_ops = {
> +static const struct tee_shm_pool_ops pool_ops = {
> .alloc = pool_op_alloc,
> .free = pool_op_free,
> - .destroy_poolmgr = pool_op_destroy_poolmgr,
> + .destroy_pool = pool_op_destroy_pool,
> };
>
> -static struct tee_shm_pool_mgr *pool_mem_mgr_alloc(void)
> -{
> - struct tee_shm_pool_mgr *mgr = kzalloc(sizeof(*mgr), GFP_KERNEL);
> -
> - if (!mgr)
> - return ERR_PTR(-ENOMEM);
> -
> - mgr->ops = &pool_ops;
> -
> - return mgr;
> -}
> -
> struct tee_shm_pool *amdtee_config_shm(void)
> {
> - struct tee_shm_pool_mgr *priv_mgr;
> - struct tee_shm_pool_mgr *dmabuf_mgr;
> - void *rc;
> + struct tee_shm_pool *pool = kzalloc(sizeof(*pool), GFP_KERNEL);
>
> - rc = pool_mem_mgr_alloc();
> - if (IS_ERR(rc))
> - return rc;
> - priv_mgr = rc;
> -
> - rc = pool_mem_mgr_alloc();
> - if (IS_ERR(rc)) {
> - tee_shm_pool_mgr_destroy(priv_mgr);
> - return rc;
> - }
> - dmabuf_mgr = rc;
> + if (!pool)
> + return ERR_PTR(-ENOMEM);
>
> - rc = tee_shm_pool_alloc(priv_mgr, dmabuf_mgr);
> - if (IS_ERR(rc)) {
> - tee_shm_pool_mgr_destroy(priv_mgr);
> - tee_shm_pool_mgr_destroy(dmabuf_mgr);
> - }
> + pool->ops = &pool_ops;
>
> - return rc;
> + return pool;
> }
> diff --git a/drivers/tee/optee/Kconfig b/drivers/tee/optee/Kconfig
> index 3ca71e3812ed..f121c224e682 100644
> --- a/drivers/tee/optee/Kconfig
> +++ b/drivers/tee/optee/Kconfig
> @@ -7,11 +7,3 @@ config OPTEE
> help
> This implements the OP-TEE Trusted Execution Environment (TEE)
> driver.
> -
> -config OPTEE_SHM_NUM_PRIV_PAGES
> - int "Private Shared Memory Pages"
> - default 1
> - depends on OPTEE
> - help
> - This sets the number of private shared memory pages to be
> - used by OP-TEE TEE driver.
> diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c
> index ddb8f9ecf307..0c287345f9fe 100644
> --- a/drivers/tee/optee/core.c
> +++ b/drivers/tee/optee/core.c
This patch should also remove the #define at the top of core.c:
#define OPTEE_SHM_NUM_PRIV_PAGES CONFIG_OPTEE_SHM_NUM_PRIV_PAGES
> @@ -428,33 +428,6 @@ static bool optee_msg_exchange_capabilities(optee_invoke_fn *invoke_fn,
> return true;
> }
>
> -static struct tee_shm_pool *optee_config_dyn_shm(void)
> -{
> - struct tee_shm_pool_mgr *priv_mgr;
> - struct tee_shm_pool_mgr *dmabuf_mgr;
> - void *rc;
> -
> - rc = optee_shm_pool_alloc_pages();
> - if (IS_ERR(rc))
> - return rc;
> - priv_mgr = rc;
> -
> - rc = optee_shm_pool_alloc_pages();
> - if (IS_ERR(rc)) {
> - tee_shm_pool_mgr_destroy(priv_mgr);
> - return rc;
> - }
> - dmabuf_mgr = rc;
> -
> - rc = tee_shm_pool_alloc(priv_mgr, dmabuf_mgr);
> - if (IS_ERR(rc)) {
> - tee_shm_pool_mgr_destroy(priv_mgr);
> - tee_shm_pool_mgr_destroy(dmabuf_mgr);
> - }
> -
> - return rc;
> -}
> -
> static struct tee_shm_pool *
> optee_config_shm_memremap(optee_invoke_fn *invoke_fn, void **memremaped_shm)
> {
> @@ -468,10 +441,7 @@ optee_config_shm_memremap(optee_invoke_fn *invoke_fn, void **memremaped_shm)
> phys_addr_t begin;
> phys_addr_t end;
> void *va;
> - struct tee_shm_pool_mgr *priv_mgr;
> - struct tee_shm_pool_mgr *dmabuf_mgr;
> void *rc;
> - const int sz = OPTEE_SHM_NUM_PRIV_PAGES * PAGE_SIZE;
>
> invoke_fn(OPTEE_SMC_GET_SHM_CONFIG, 0, 0, 0, 0, 0, 0, 0, &res.smccc);
> if (res.result.status != OPTEE_SMC_RETURN_OK) {
> @@ -489,11 +459,6 @@ optee_config_shm_memremap(optee_invoke_fn *invoke_fn, void **memremaped_shm)
> paddr = begin;
> size = end - begin;
>
> - if (size < 2 * OPTEE_SHM_NUM_PRIV_PAGES * PAGE_SIZE) {
> - pr_err("too small shared memory area\n");
> - return ERR_PTR(-EINVAL);
> - }
> -
> va = memremap(paddr, size, MEMREMAP_WB);
> if (!va) {
> pr_err("shared memory ioremap failed\n");
> @@ -501,35 +466,13 @@ optee_config_shm_memremap(optee_invoke_fn *invoke_fn, void **memremaped_shm)
> }
> vaddr = (unsigned long)va;
>
> - rc = tee_shm_pool_mgr_alloc_res_mem(vaddr, paddr, sz,
> - 3 /* 8 bytes aligned */);
> - if (IS_ERR(rc))
> - goto err_memunmap;
> - priv_mgr = rc;
> -
> - vaddr += sz;
> - paddr += sz;
> - size -= sz;
> -
> - rc = tee_shm_pool_mgr_alloc_res_mem(vaddr, paddr, size, PAGE_SHIFT);
> + rc = tee_shm_pool_alloc_res_mem(vaddr, paddr, size,
> + 9 /* 512 bytes aligned */);
> if (IS_ERR(rc))
> - goto err_free_priv_mgr;
> - dmabuf_mgr = rc;
> -
> - rc = tee_shm_pool_alloc(priv_mgr, dmabuf_mgr);
> - if (IS_ERR(rc))
> - goto err_free_dmabuf_mgr;
> -
> - *memremaped_shm = va;
> -
> - return rc;
> + memunmap(va);
> + else
> + *memremaped_shm = va;
>
> -err_free_dmabuf_mgr:
> - tee_shm_pool_mgr_destroy(dmabuf_mgr);
> -err_free_priv_mgr:
> - tee_shm_pool_mgr_destroy(priv_mgr);
> -err_memunmap:
> - memunmap(va);
> return rc;
> }
>
> @@ -637,7 +580,7 @@ static int optee_probe(struct platform_device *pdev)
> * Try to use dynamic shared memory if possible
> */
> if (sec_caps & OPTEE_SMC_SEC_CAP_DYNAMIC_SHM)
> - pool = optee_config_dyn_shm();
> + pool = optee_shm_pool_alloc_pages();
>
> /*
> * If dynamic shared memory is not available or failed - try static one
> @@ -712,8 +655,7 @@ static int optee_probe(struct platform_device *pdev)
> tee_device_unregister(optee->teedev);
> kfree(optee);
> }
> - if (pool)
> - tee_shm_pool_free(pool);
> + tee_shm_pool_free(pool);
> if (memremaped_shm)
> memunmap(memremaped_shm);
> return rc;
> diff --git a/drivers/tee/optee/shm_pool.c b/drivers/tee/optee/shm_pool.c
> index d767eebf30bd..b036714845c1 100644
> --- a/drivers/tee/optee/shm_pool.c
> +++ b/drivers/tee/optee/shm_pool.c
> @@ -12,13 +12,17 @@
> #include "optee_smc.h"
> #include "shm_pool.h"
>
> -static int pool_op_alloc(struct tee_shm_pool_mgr *poolm,
> - struct tee_shm *shm, size_t size)
> +static int pool_op_alloc(struct tee_shm_pool *pool,
> + struct tee_shm *shm, size_t size, u_int align)
> {
> unsigned int order = get_order(size);
> struct page *page;
> int rc = 0;
>
> + /*
> + * Ignore alignment since this is already going to be page aligned
> + * and there's no need for any larger alignment.
> + */
> page = alloc_pages(GFP_KERNEL | __GFP_ZERO, order);
> if (!page)
> return -ENOMEM;
> @@ -27,47 +31,52 @@ static int pool_op_alloc(struct tee_shm_pool_mgr *poolm,
> shm->paddr = page_to_phys(page);
> shm->size = PAGE_SIZE << order;
>
> - if (shm->flags & TEE_SHM_DMA_BUF) {
> + if (shm->flags & TEE_SHM_REGISTER) {
> unsigned int nr_pages = 1 << order, i;
> struct page **pages;
>
> pages = kcalloc(nr_pages, sizeof(pages), GFP_KERNEL);
> - if (!pages)
> - return -ENOMEM;
> -
> - for (i = 0; i < nr_pages; i++) {
> - pages[i] = page;
> - page++;
> + if (!pages) {
> + rc = -ENOMEM;
> + goto err;
> }
>
> - shm->flags |= TEE_SHM_REGISTER;
> + for (i = 0; i < nr_pages; i++)
> + pages[i] = page + i;
> +
> rc = optee_shm_register(shm->ctx, shm, pages, nr_pages,
> (unsigned long)shm->kaddr);
> kfree(pages);
> + if (rc)
> + goto err;
> }
>
> + return 0;
> +
> +err:
> + free_pages((unsigned long)shm->kaddr, order);
This memory leak fix should be split out and sent to stable. I've sent a
patch that does this:
https://lore.kernel.org/lkml/20210609002326.210024-2-tyhicks@linux.microsoft.com/
> return rc;
> }
>
> -static void pool_op_free(struct tee_shm_pool_mgr *poolm,
> +static void pool_op_free(struct tee_shm_pool *pool,
> struct tee_shm *shm)
> {
> - if (shm->flags & TEE_SHM_DMA_BUF)
> + if (shm->flags & TEE_SHM_REGISTER)
> optee_shm_unregister(shm->ctx, shm);
>
> free_pages((unsigned long)shm->kaddr, get_order(shm->size));
> shm->kaddr = NULL;
> }
>
> -static void pool_op_destroy_poolmgr(struct tee_shm_pool_mgr *poolm)
> +static void pool_op_destroy_pool(struct tee_shm_pool *pool)
> {
> - kfree(poolm);
> + kfree(pool);
> }
>
> -static const struct tee_shm_pool_mgr_ops pool_ops = {
> +static const struct tee_shm_pool_ops pool_ops = {
> .alloc = pool_op_alloc,
> .free = pool_op_free,
> - .destroy_poolmgr = pool_op_destroy_poolmgr,
> + .destroy_pool = pool_op_destroy_pool,
> };
>
> /**
> @@ -76,14 +85,14 @@ static const struct tee_shm_pool_mgr_ops pool_ops = {
> * This pool is used when OP-TEE supports dymanic SHM. In this case
> * command buffers and such are allocated from kernel's own memory.
> */
> -struct tee_shm_pool_mgr *optee_shm_pool_alloc_pages(void)
> +struct tee_shm_pool *optee_shm_pool_alloc_pages(void)
> {
> - struct tee_shm_pool_mgr *mgr = kzalloc(sizeof(*mgr), GFP_KERNEL);
> + struct tee_shm_pool *pool = kzalloc(sizeof(*pool), GFP_KERNEL);
>
> - if (!mgr)
> + if (!pool)
> return ERR_PTR(-ENOMEM);
>
> - mgr->ops = &pool_ops;
> + pool->ops = &pool_ops;
>
> - return mgr;
> + return pool;
> }
> diff --git a/drivers/tee/optee/shm_pool.h b/drivers/tee/optee/shm_pool.h
> index 28109d991c4b..7024b9926ada 100644
> --- a/drivers/tee/optee/shm_pool.h
> +++ b/drivers/tee/optee/shm_pool.h
> @@ -9,6 +9,6 @@
>
> #include <linux/tee_drv.h>
>
> -struct tee_shm_pool_mgr *optee_shm_pool_alloc_pages(void);
> +struct tee_shm_pool *optee_shm_pool_alloc_pages(void);
>
> #endif
> diff --git a/drivers/tee/tee_private.h b/drivers/tee/tee_private.h
> index e55204df31ce..72376cf38bc0 100644
> --- a/drivers/tee/tee_private.h
> +++ b/drivers/tee/tee_private.h
> @@ -12,17 +12,6 @@
> #include <linux/mutex.h>
> #include <linux/types.h>
>
> -/**
> - * struct tee_shm_pool - shared memory pool
> - * @private_mgr: pool manager for shared memory only between kernel
> - * and secure world
> - * @dma_buf_mgr: pool manager for shared memory exported to user space
> - */
> -struct tee_shm_pool {
> - struct tee_shm_pool_mgr *private_mgr;
> - struct tee_shm_pool_mgr *dma_buf_mgr;
> -};
> -
> #define TEE_DEVICE_FLAG_REGISTERED 0x1
> #define TEE_MAX_DEV_NAME_LEN 32
>
> diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c
> index 00472f5ce22e..b9dbf4bce149 100644
> --- a/drivers/tee/tee_shm.c
> +++ b/drivers/tee/tee_shm.c
> @@ -39,14 +39,7 @@ static void tee_shm_release(struct tee_shm *shm)
> }
>
> if (shm->flags & TEE_SHM_POOL) {
> - struct tee_shm_pool_mgr *poolm;
> -
> - if (shm->flags & TEE_SHM_DMA_BUF)
> - poolm = teedev->pool->dma_buf_mgr;
> - else
> - poolm = teedev->pool->private_mgr;
> -
> - poolm->ops->free(poolm, shm);
> + teedev->pool->ops->free(teedev->pool, shm);
> } else if (shm->flags & TEE_SHM_REGISTER) {
> int rc = teedev->desc->ops->shm_unregister(shm->ctx, shm);
>
> @@ -106,8 +99,8 @@ static const struct dma_buf_ops tee_shm_dma_buf_ops = {
> struct tee_shm *tee_shm_alloc(struct tee_context *ctx, size_t size, u32 flags)
> {
> struct tee_device *teedev = ctx->teedev;
> - struct tee_shm_pool_mgr *poolm = NULL;
> struct tee_shm *shm;
> + size_t align;
You use u_int for the align parameter type in the .alloc function
prototype. Perhaps switch it to size_t to match the argument that
you're passing in?
> void *ret;
> int rc;
>
> @@ -139,12 +132,18 @@ struct tee_shm *tee_shm_alloc(struct tee_context *ctx, size_t size, u32 flags)
>
> shm->flags = flags | TEE_SHM_POOL;
> shm->ctx = ctx;
> - if (flags & TEE_SHM_DMA_BUF)
> - poolm = teedev->pool->dma_buf_mgr;
> - else
> - poolm = teedev->pool->private_mgr;
> + if (flags & TEE_SHM_DMA_BUF) {
> + align = PAGE_SIZE;
> + /*
> + * Request to register the shm in the pool allocator below
> + * if supported.
> + */
> + shm->flags |= TEE_SHM_REGISTER;
> + } else {
> + align = 2 * sizeof(long);
Where does this alignment requirement come from?
> + }
>
> - rc = poolm->ops->alloc(poolm, shm, size);
> + rc = teedev->pool->ops->alloc(teedev->pool, shm, size, align);
> if (rc) {
> ret = ERR_PTR(rc);
> goto err_kfree;
> @@ -184,7 +183,7 @@ struct tee_shm *tee_shm_alloc(struct tee_context *ctx, size_t size, u32 flags)
> mutex_unlock(&teedev->mutex);
> }
> err_pool_free:
> - poolm->ops->free(poolm, shm);
> + teedev->pool->ops->free(teedev->pool, shm);
> err_kfree:
> kfree(shm);
> err_dev_put:
> diff --git a/drivers/tee/tee_shm_pool.c b/drivers/tee/tee_shm_pool.c
> index a9f9d50fd181..5ceab73ff1b5 100644
> --- a/drivers/tee/tee_shm_pool.c
> +++ b/drivers/tee/tee_shm_pool.c
> @@ -9,14 +9,16 @@
> #include <linux/tee_drv.h>
> #include "tee_private.h"
>
> -static int pool_op_gen_alloc(struct tee_shm_pool_mgr *poolm,
> - struct tee_shm *shm, size_t size)
> +static int pool_op_gen_alloc(struct tee_shm_pool *pool, struct tee_shm *shm,
> + size_t size, u_int align)
> {
> unsigned long va;
> - struct gen_pool *genpool = poolm->private_data;
> - size_t s = roundup(size, 1 << genpool->min_alloc_order);
> + struct gen_pool *genpool = pool->private_data;
> + size_t a = max(align, 1U << genpool->min_alloc_order);
> + struct genpool_data_align data = { .align = a };
> + size_t s = roundup(size, a);
>
> - va = gen_pool_alloc(genpool, s);
> + va = gen_pool_alloc_algo(genpool, s, gen_pool_first_fit_align, &data);
This is an algo change from the existing use of gen_pool_best_fit. Was
that intentional? If so, a mention of the reasoning for this change in
the commit message might be helpful in the future.
Tyler
> if (!va)
> return -ENOMEM;
>
> @@ -24,107 +26,67 @@ static int pool_op_gen_alloc(struct tee_shm_pool_mgr *poolm,
> shm->kaddr = (void *)va;
> shm->paddr = gen_pool_virt_to_phys(genpool, va);
> shm->size = s;
> + /*
> + * This is from a static shared memory pool so no need to register
> + * each chunk, and no need to unregister later either.
> + */
> + shm->flags &= ~TEE_SHM_REGISTER;
> return 0;
> }
>
> -static void pool_op_gen_free(struct tee_shm_pool_mgr *poolm,
> - struct tee_shm *shm)
> +static void pool_op_gen_free(struct tee_shm_pool *pool, struct tee_shm *shm)
> {
> - gen_pool_free(poolm->private_data, (unsigned long)shm->kaddr,
> + gen_pool_free(pool->private_data, (unsigned long)shm->kaddr,
> shm->size);
> shm->kaddr = NULL;
> }
>
> -static void pool_op_gen_destroy_poolmgr(struct tee_shm_pool_mgr *poolm)
> +static void pool_op_gen_destroy_pool(struct tee_shm_pool *pool)
> {
> - gen_pool_destroy(poolm->private_data);
> - kfree(poolm);
> + gen_pool_destroy(pool->private_data);
> + kfree(pool);
> }
>
> -static const struct tee_shm_pool_mgr_ops pool_ops_generic = {
> +static const struct tee_shm_pool_ops pool_ops_generic = {
> .alloc = pool_op_gen_alloc,
> .free = pool_op_gen_free,
> - .destroy_poolmgr = pool_op_gen_destroy_poolmgr,
> + .destroy_pool = pool_op_gen_destroy_pool,
> };
>
> -struct tee_shm_pool_mgr *tee_shm_pool_mgr_alloc_res_mem(unsigned long vaddr,
> - phys_addr_t paddr,
> - size_t size,
> - int min_alloc_order)
> +struct tee_shm_pool *tee_shm_pool_alloc_res_mem(unsigned long vaddr,
> + phys_addr_t paddr, size_t size,
> + int min_alloc_order)
> {
> const size_t page_mask = PAGE_SIZE - 1;
> - struct tee_shm_pool_mgr *mgr;
> + struct tee_shm_pool *pool;
> int rc;
>
> /* Start and end must be page aligned */
> if (vaddr & page_mask || paddr & page_mask || size & page_mask)
> return ERR_PTR(-EINVAL);
>
> - mgr = kzalloc(sizeof(*mgr), GFP_KERNEL);
> - if (!mgr)
> + pool = kzalloc(sizeof(*pool), GFP_KERNEL);
> + if (!pool)
> return ERR_PTR(-ENOMEM);
>
> - mgr->private_data = gen_pool_create(min_alloc_order, -1);
> - if (!mgr->private_data) {
> + pool->private_data = gen_pool_create(min_alloc_order, -1);
> + if (!pool->private_data) {
> rc = -ENOMEM;
> goto err;
> }
>
> - gen_pool_set_algo(mgr->private_data, gen_pool_best_fit, NULL);
> - rc = gen_pool_add_virt(mgr->private_data, vaddr, paddr, size, -1);
> + rc = gen_pool_add_virt(pool->private_data, vaddr, paddr, size, -1);
> if (rc) {
> - gen_pool_destroy(mgr->private_data);
> + gen_pool_destroy(pool->private_data);
> goto err;
> }
>
> - mgr->ops = &pool_ops_generic;
> + pool->ops = &pool_ops_generic;
>
> - return mgr;
> + return pool;
> err:
> - kfree(mgr);
> + kfree(pool);
>
> return ERR_PTR(rc);
> }
> -EXPORT_SYMBOL_GPL(tee_shm_pool_mgr_alloc_res_mem);
> -
> -static bool check_mgr_ops(struct tee_shm_pool_mgr *mgr)
> -{
> - return mgr && mgr->ops && mgr->ops->alloc && mgr->ops->free &&
> - mgr->ops->destroy_poolmgr;
> -}
> -
> -struct tee_shm_pool *tee_shm_pool_alloc(struct tee_shm_pool_mgr *priv_mgr,
> - struct tee_shm_pool_mgr *dmabuf_mgr)
> -{
> - struct tee_shm_pool *pool;
> -
> - if (!check_mgr_ops(priv_mgr) || !check_mgr_ops(dmabuf_mgr))
> - return ERR_PTR(-EINVAL);
> -
> - pool = kzalloc(sizeof(*pool), GFP_KERNEL);
> - if (!pool)
> - return ERR_PTR(-ENOMEM);
> -
> - pool->private_mgr = priv_mgr;
> - pool->dma_buf_mgr = dmabuf_mgr;
> -
> - return pool;
> -}
> -EXPORT_SYMBOL_GPL(tee_shm_pool_alloc);
> -
> -/**
> - * tee_shm_pool_free() - Free a shared memory pool
> - * @pool: The shared memory pool to free
> - *
> - * There must be no remaining shared memory allocated from this pool when
> - * this function is called.
> - */
> -void tee_shm_pool_free(struct tee_shm_pool *pool)
> -{
> - if (pool->private_mgr)
> - tee_shm_pool_mgr_destroy(pool->private_mgr);
> - if (pool->dma_buf_mgr)
> - tee_shm_pool_mgr_destroy(pool->dma_buf_mgr);
> - kfree(pool);
> -}
> -EXPORT_SYMBOL_GPL(tee_shm_pool_free);
> +EXPORT_SYMBOL_GPL(tee_shm_pool_alloc_res_mem);
> diff --git a/include/linux/tee_drv.h b/include/linux/tee_drv.h
> index 53b9b2a13a87..62b7c7a55743 100644
> --- a/include/linux/tee_drv.h
> +++ b/include/linux/tee_drv.h
> @@ -215,62 +215,39 @@ struct tee_shm {
> };
>
> /**
> - * struct tee_shm_pool_mgr - shared memory manager
> + * struct tee_shm_pool - shared memory pool
> * @ops: operations
> * @private_data: private data for the shared memory manager
> */
> -struct tee_shm_pool_mgr {
> - const struct tee_shm_pool_mgr_ops *ops;
> +struct tee_shm_pool {
> + const struct tee_shm_pool_ops *ops;
> void *private_data;
> };
>
> /**
> - * struct tee_shm_pool_mgr_ops - shared memory pool manager operations
> + * struct tee_shm_pool_ops - shared memory pool operations
> * @alloc: called when allocating shared memory
> * @free: called when freeing shared memory
> - * @destroy_poolmgr: called when destroying the pool manager
> + * @destroy_pool: called when destroying the pool
> */
> -struct tee_shm_pool_mgr_ops {
> - int (*alloc)(struct tee_shm_pool_mgr *poolmgr, struct tee_shm *shm,
> - size_t size);
> - void (*free)(struct tee_shm_pool_mgr *poolmgr, struct tee_shm *shm);
> - void (*destroy_poolmgr)(struct tee_shm_pool_mgr *poolmgr);
> +struct tee_shm_pool_ops {
> + int (*alloc)(struct tee_shm_pool *pool, struct tee_shm *shm,
> + size_t size, u_int align);
> + void (*free)(struct tee_shm_pool *pool, struct tee_shm *shm);
> + void (*destroy_pool)(struct tee_shm_pool *pool);
> };
>
> -/**
> - * tee_shm_pool_alloc() - Create a shared memory pool from shm managers
> - * @priv_mgr: manager for driver private shared memory allocations
> - * @dmabuf_mgr: manager for dma-buf shared memory allocations
> - *
> - * Allocation with the flag TEE_SHM_DMA_BUF set will use the range supplied
> - * in @dmabuf, others will use the range provided by @priv.
> - *
> - * @returns pointer to a 'struct tee_shm_pool' or an ERR_PTR on failure.
> - */
> -struct tee_shm_pool *tee_shm_pool_alloc(struct tee_shm_pool_mgr *priv_mgr,
> - struct tee_shm_pool_mgr *dmabuf_mgr);
> -
> /*
> - * tee_shm_pool_mgr_alloc_res_mem() - Create a shm manager for reserved
> - * memory
> + * tee_shm_pool_alloc_res_mem() - Create a shm manager for reserved memory
> * @vaddr: Virtual address of start of pool
> * @paddr: Physical address of start of pool
> * @size: Size in bytes of the pool
> *
> - * @returns pointer to a 'struct tee_shm_pool_mgr' or an ERR_PTR on failure.
> - */
> -struct tee_shm_pool_mgr *tee_shm_pool_mgr_alloc_res_mem(unsigned long vaddr,
> - phys_addr_t paddr,
> - size_t size,
> - int min_alloc_order);
> -
> -/**
> - * tee_shm_pool_mgr_destroy() - Free a shared memory manager
> + * @returns pointer to a 'struct tee_shm_pool' or an ERR_PTR on failure.
> */
> -static inline void tee_shm_pool_mgr_destroy(struct tee_shm_pool_mgr *poolm)
> -{
> - poolm->ops->destroy_poolmgr(poolm);
> -}
> +struct tee_shm_pool *tee_shm_pool_alloc_res_mem(unsigned long vaddr,
> + phys_addr_t paddr, size_t size,
> + int min_alloc_order);
>
> /**
> * tee_shm_pool_free() - Free a shared memory pool
> @@ -279,7 +256,10 @@ static inline void tee_shm_pool_mgr_destroy(struct tee_shm_pool_mgr *poolm)
> * The must be no remaining shared memory allocated from this pool when
> * this function is called.
> */
> -void tee_shm_pool_free(struct tee_shm_pool *pool);
> +static inline void tee_shm_pool_free(struct tee_shm_pool *pool)
> +{
> + pool->ops->destroy_pool(pool);
> +}
>
> /**
> * tee_get_drvdata() - Return driver_data pointer
> --
> 2.31.1
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 2/7] tee: simplify shm pool handling
2021-06-09 14:50 ` Tyler Hicks
@ 2021-06-10 9:01 ` Jens Wiklander
0 siblings, 0 replies; 19+ messages in thread
From: Jens Wiklander @ 2021-06-10 9:01 UTC (permalink / raw)
To: Tyler Hicks
Cc: linux-kernel, linux-arm-kernel, op-tee, Sumit Garg, Herbert Xu,
Sakkinen, Sasha Levin, Thirupathaiah Annapureddy, Vikas Gupta,
David S . Miller
On Wed, Jun 09, 2021 at 09:50:04AM -0500, Tyler Hicks wrote:
> On 2021-06-09 12:23:19, Jens Wiklander wrote:
> > Replaces the shared memory pool based on two pools with a single pool.
> > The alloc() function pointer in struct tee_shm_pool_ops gets another
> > parameter, align. This makes it possible to make less than page aligned
> > allocations from the optional reserved shared memory pool while still
> > making user space allocations page aligned. With in practice unchanged
> > behaviour using only a single pool for bookkeeping.
> >
> > The optee and amdtee drivers are updated as needed to work with this
> > changed pool handling.
> >
> > This also removes OPTEE_SHM_NUM_PRIV_PAGES which becomes obsolete with
> > this change as the private pages can be mixed with the payload pages.
> >
> > Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> > ---
> > drivers/tee/amdtee/shm_pool.c | 55 ++++++------------
> > drivers/tee/optee/Kconfig | 8 ---
> > drivers/tee/optee/core.c | 72 +++--------------------
> > drivers/tee/optee/shm_pool.c | 51 ++++++++++-------
> > drivers/tee/optee/shm_pool.h | 2 +-
> > drivers/tee/tee_private.h | 11 ----
> > drivers/tee/tee_shm.c | 29 +++++-----
> > drivers/tee/tee_shm_pool.c | 104 +++++++++++-----------------------
> > include/linux/tee_drv.h | 58 +++++++------------
> > 9 files changed, 120 insertions(+), 270 deletions(-)
> >
> > diff --git a/drivers/tee/amdtee/shm_pool.c b/drivers/tee/amdtee/shm_pool.c
> > index 065854e2db18..81c23f30b455 100644
> > --- a/drivers/tee/amdtee/shm_pool.c
> > +++ b/drivers/tee/amdtee/shm_pool.c
> > @@ -8,13 +8,17 @@
> > #include <linux/psp-sev.h>
> > #include "amdtee_private.h"
> >
> > -static int pool_op_alloc(struct tee_shm_pool_mgr *poolm, struct tee_shm *shm,
> > - size_t size)
> > +static int pool_op_alloc(struct tee_shm_pool *pool, struct tee_shm *shm,
> > + size_t size, u_int align)
> > {
> > unsigned int order = get_order(size);
> > unsigned long va;
> > int rc;
> >
> > + /*
> > + * Ignore alignment since this is already going to be page aligned
> > + * and there's no need for any larger alignment.
> > + */
> > va = __get_free_pages(GFP_KERNEL | __GFP_ZERO, order);
> > if (!va)
> > return -ENOMEM;
> > @@ -34,7 +38,7 @@ static int pool_op_alloc(struct tee_shm_pool_mgr *poolm, struct tee_shm *shm,
> > return 0;
> > }
> >
> > -static void pool_op_free(struct tee_shm_pool_mgr *poolm, struct tee_shm *shm)
> > +static void pool_op_free(struct tee_shm_pool *pool, struct tee_shm *shm)
> > {
> > /* Unmap the shared memory from TEE */
> > amdtee_unmap_shmem(shm);
> > @@ -42,52 +46,25 @@ static void pool_op_free(struct tee_shm_pool_mgr *poolm, struct tee_shm *shm)
> > shm->kaddr = NULL;
> > }
> >
> > -static void pool_op_destroy_poolmgr(struct tee_shm_pool_mgr *poolm)
> > +static void pool_op_destroy_pool(struct tee_shm_pool *pool)
> > {
> > - kfree(poolm);
> > + kfree(pool);
> > }
> >
> > -static const struct tee_shm_pool_mgr_ops pool_ops = {
> > +static const struct tee_shm_pool_ops pool_ops = {
> > .alloc = pool_op_alloc,
> > .free = pool_op_free,
> > - .destroy_poolmgr = pool_op_destroy_poolmgr,
> > + .destroy_pool = pool_op_destroy_pool,
> > };
> >
> > -static struct tee_shm_pool_mgr *pool_mem_mgr_alloc(void)
> > -{
> > - struct tee_shm_pool_mgr *mgr = kzalloc(sizeof(*mgr), GFP_KERNEL);
> > -
> > - if (!mgr)
> > - return ERR_PTR(-ENOMEM);
> > -
> > - mgr->ops = &pool_ops;
> > -
> > - return mgr;
> > -}
> > -
> > struct tee_shm_pool *amdtee_config_shm(void)
> > {
> > - struct tee_shm_pool_mgr *priv_mgr;
> > - struct tee_shm_pool_mgr *dmabuf_mgr;
> > - void *rc;
> > + struct tee_shm_pool *pool = kzalloc(sizeof(*pool), GFP_KERNEL);
> >
> > - rc = pool_mem_mgr_alloc();
> > - if (IS_ERR(rc))
> > - return rc;
> > - priv_mgr = rc;
> > -
> > - rc = pool_mem_mgr_alloc();
> > - if (IS_ERR(rc)) {
> > - tee_shm_pool_mgr_destroy(priv_mgr);
> > - return rc;
> > - }
> > - dmabuf_mgr = rc;
> > + if (!pool)
> > + return ERR_PTR(-ENOMEM);
> >
> > - rc = tee_shm_pool_alloc(priv_mgr, dmabuf_mgr);
> > - if (IS_ERR(rc)) {
> > - tee_shm_pool_mgr_destroy(priv_mgr);
> > - tee_shm_pool_mgr_destroy(dmabuf_mgr);
> > - }
> > + pool->ops = &pool_ops;
> >
> > - return rc;
> > + return pool;
> > }
> > diff --git a/drivers/tee/optee/Kconfig b/drivers/tee/optee/Kconfig
> > index 3ca71e3812ed..f121c224e682 100644
> > --- a/drivers/tee/optee/Kconfig
> > +++ b/drivers/tee/optee/Kconfig
> > @@ -7,11 +7,3 @@ config OPTEE
> > help
> > This implements the OP-TEE Trusted Execution Environment (TEE)
> > driver.
> > -
> > -config OPTEE_SHM_NUM_PRIV_PAGES
> > - int "Private Shared Memory Pages"
> > - default 1
> > - depends on OPTEE
> > - help
> > - This sets the number of private shared memory pages to be
> > - used by OP-TEE TEE driver.
> > diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c
> > index ddb8f9ecf307..0c287345f9fe 100644
> > --- a/drivers/tee/optee/core.c
> > +++ b/drivers/tee/optee/core.c
>
> This patch should also remove the #define at the top of core.c:
>
> #define OPTEE_SHM_NUM_PRIV_PAGES CONFIG_OPTEE_SHM_NUM_PRIV_PAGES
Good point, I'll fix.
>
> > @@ -428,33 +428,6 @@ static bool optee_msg_exchange_capabilities(optee_invoke_fn *invoke_fn,
> > return true;
> > }
> >
> > -static struct tee_shm_pool *optee_config_dyn_shm(void)
> > -{
> > - struct tee_shm_pool_mgr *priv_mgr;
> > - struct tee_shm_pool_mgr *dmabuf_mgr;
> > - void *rc;
> > -
> > - rc = optee_shm_pool_alloc_pages();
> > - if (IS_ERR(rc))
> > - return rc;
> > - priv_mgr = rc;
> > -
> > - rc = optee_shm_pool_alloc_pages();
> > - if (IS_ERR(rc)) {
> > - tee_shm_pool_mgr_destroy(priv_mgr);
> > - return rc;
> > - }
> > - dmabuf_mgr = rc;
> > -
> > - rc = tee_shm_pool_alloc(priv_mgr, dmabuf_mgr);
> > - if (IS_ERR(rc)) {
> > - tee_shm_pool_mgr_destroy(priv_mgr);
> > - tee_shm_pool_mgr_destroy(dmabuf_mgr);
> > - }
> > -
> > - return rc;
> > -}
> > -
> > static struct tee_shm_pool *
> > optee_config_shm_memremap(optee_invoke_fn *invoke_fn, void **memremaped_shm)
> > {
> > @@ -468,10 +441,7 @@ optee_config_shm_memremap(optee_invoke_fn *invoke_fn, void **memremaped_shm)
> > phys_addr_t begin;
> > phys_addr_t end;
> > void *va;
> > - struct tee_shm_pool_mgr *priv_mgr;
> > - struct tee_shm_pool_mgr *dmabuf_mgr;
> > void *rc;
> > - const int sz = OPTEE_SHM_NUM_PRIV_PAGES * PAGE_SIZE;
> >
> > invoke_fn(OPTEE_SMC_GET_SHM_CONFIG, 0, 0, 0, 0, 0, 0, 0, &res.smccc);
> > if (res.result.status != OPTEE_SMC_RETURN_OK) {
> > @@ -489,11 +459,6 @@ optee_config_shm_memremap(optee_invoke_fn *invoke_fn, void **memremaped_shm)
> > paddr = begin;
> > size = end - begin;
> >
> > - if (size < 2 * OPTEE_SHM_NUM_PRIV_PAGES * PAGE_SIZE) {
> > - pr_err("too small shared memory area\n");
> > - return ERR_PTR(-EINVAL);
> > - }
> > -
> > va = memremap(paddr, size, MEMREMAP_WB);
> > if (!va) {
> > pr_err("shared memory ioremap failed\n");
> > @@ -501,35 +466,13 @@ optee_config_shm_memremap(optee_invoke_fn *invoke_fn, void **memremaped_shm)
> > }
> > vaddr = (unsigned long)va;
> >
> > - rc = tee_shm_pool_mgr_alloc_res_mem(vaddr, paddr, sz,
> > - 3 /* 8 bytes aligned */);
> > - if (IS_ERR(rc))
> > - goto err_memunmap;
> > - priv_mgr = rc;
> > -
> > - vaddr += sz;
> > - paddr += sz;
> > - size -= sz;
> > -
> > - rc = tee_shm_pool_mgr_alloc_res_mem(vaddr, paddr, size, PAGE_SHIFT);
> > + rc = tee_shm_pool_alloc_res_mem(vaddr, paddr, size,
> > + 9 /* 512 bytes aligned */);
> > if (IS_ERR(rc))
> > - goto err_free_priv_mgr;
> > - dmabuf_mgr = rc;
> > -
> > - rc = tee_shm_pool_alloc(priv_mgr, dmabuf_mgr);
> > - if (IS_ERR(rc))
> > - goto err_free_dmabuf_mgr;
> > -
> > - *memremaped_shm = va;
> > -
> > - return rc;
> > + memunmap(va);
> > + else
> > + *memremaped_shm = va;
> >
> > -err_free_dmabuf_mgr:
> > - tee_shm_pool_mgr_destroy(dmabuf_mgr);
> > -err_free_priv_mgr:
> > - tee_shm_pool_mgr_destroy(priv_mgr);
> > -err_memunmap:
> > - memunmap(va);
> > return rc;
> > }
> >
> > @@ -637,7 +580,7 @@ static int optee_probe(struct platform_device *pdev)
> > * Try to use dynamic shared memory if possible
> > */
> > if (sec_caps & OPTEE_SMC_SEC_CAP_DYNAMIC_SHM)
> > - pool = optee_config_dyn_shm();
> > + pool = optee_shm_pool_alloc_pages();
> >
> > /*
> > * If dynamic shared memory is not available or failed - try static one
> > @@ -712,8 +655,7 @@ static int optee_probe(struct platform_device *pdev)
> > tee_device_unregister(optee->teedev);
> > kfree(optee);
> > }
> > - if (pool)
> > - tee_shm_pool_free(pool);
> > + tee_shm_pool_free(pool);
> > if (memremaped_shm)
> > memunmap(memremaped_shm);
> > return rc;
> > diff --git a/drivers/tee/optee/shm_pool.c b/drivers/tee/optee/shm_pool.c
> > index d767eebf30bd..b036714845c1 100644
> > --- a/drivers/tee/optee/shm_pool.c
> > +++ b/drivers/tee/optee/shm_pool.c
> > @@ -12,13 +12,17 @@
> > #include "optee_smc.h"
> > #include "shm_pool.h"
> >
> > -static int pool_op_alloc(struct tee_shm_pool_mgr *poolm,
> > - struct tee_shm *shm, size_t size)
> > +static int pool_op_alloc(struct tee_shm_pool *pool,
> > + struct tee_shm *shm, size_t size, u_int align)
> > {
> > unsigned int order = get_order(size);
> > struct page *page;
> > int rc = 0;
> >
> > + /*
> > + * Ignore alignment since this is already going to be page aligned
> > + * and there's no need for any larger alignment.
> > + */
> > page = alloc_pages(GFP_KERNEL | __GFP_ZERO, order);
> > if (!page)
> > return -ENOMEM;
> > @@ -27,47 +31,52 @@ static int pool_op_alloc(struct tee_shm_pool_mgr *poolm,
> > shm->paddr = page_to_phys(page);
> > shm->size = PAGE_SIZE << order;
> >
> > - if (shm->flags & TEE_SHM_DMA_BUF) {
> > + if (shm->flags & TEE_SHM_REGISTER) {
> > unsigned int nr_pages = 1 << order, i;
> > struct page **pages;
> >
> > pages = kcalloc(nr_pages, sizeof(pages), GFP_KERNEL);
> > - if (!pages)
> > - return -ENOMEM;
> > -
> > - for (i = 0; i < nr_pages; i++) {
> > - pages[i] = page;
> > - page++;
> > + if (!pages) {
> > + rc = -ENOMEM;
> > + goto err;
> > }
> >
> > - shm->flags |= TEE_SHM_REGISTER;
> > + for (i = 0; i < nr_pages; i++)
> > + pages[i] = page + i;
> > +
> > rc = optee_shm_register(shm->ctx, shm, pages, nr_pages,
> > (unsigned long)shm->kaddr);
> > kfree(pages);
> > + if (rc)
> > + goto err;
> > }
> >
> > + return 0;
> > +
> > +err:
> > + free_pages((unsigned long)shm->kaddr, order);
>
> This memory leak fix should be split out and sent to stable. I've sent a
> patch that does this:
>
> https://lore.kernel.org/lkml/20210609002326.210024-2-tyhicks@linux.microsoft.com/
Makes sense, I'll rebase on that in the next version.
>
> > return rc;
> > }
> >
> > -static void pool_op_free(struct tee_shm_pool_mgr *poolm,
> > +static void pool_op_free(struct tee_shm_pool *pool,
> > struct tee_shm *shm)
> > {
> > - if (shm->flags & TEE_SHM_DMA_BUF)
> > + if (shm->flags & TEE_SHM_REGISTER)
> > optee_shm_unregister(shm->ctx, shm);
> >
> > free_pages((unsigned long)shm->kaddr, get_order(shm->size));
> > shm->kaddr = NULL;
> > }
> >
> > -static void pool_op_destroy_poolmgr(struct tee_shm_pool_mgr *poolm)
> > +static void pool_op_destroy_pool(struct tee_shm_pool *pool)
> > {
> > - kfree(poolm);
> > + kfree(pool);
> > }
> >
> > -static const struct tee_shm_pool_mgr_ops pool_ops = {
> > +static const struct tee_shm_pool_ops pool_ops = {
> > .alloc = pool_op_alloc,
> > .free = pool_op_free,
> > - .destroy_poolmgr = pool_op_destroy_poolmgr,
> > + .destroy_pool = pool_op_destroy_pool,
> > };
> >
> > /**
> > @@ -76,14 +85,14 @@ static const struct tee_shm_pool_mgr_ops pool_ops = {
> > * This pool is used when OP-TEE supports dymanic SHM. In this case
> > * command buffers and such are allocated from kernel's own memory.
> > */
> > -struct tee_shm_pool_mgr *optee_shm_pool_alloc_pages(void)
> > +struct tee_shm_pool *optee_shm_pool_alloc_pages(void)
> > {
> > - struct tee_shm_pool_mgr *mgr = kzalloc(sizeof(*mgr), GFP_KERNEL);
> > + struct tee_shm_pool *pool = kzalloc(sizeof(*pool), GFP_KERNEL);
> >
> > - if (!mgr)
> > + if (!pool)
> > return ERR_PTR(-ENOMEM);
> >
> > - mgr->ops = &pool_ops;
> > + pool->ops = &pool_ops;
> >
> > - return mgr;
> > + return pool;
> > }
> > diff --git a/drivers/tee/optee/shm_pool.h b/drivers/tee/optee/shm_pool.h
> > index 28109d991c4b..7024b9926ada 100644
> > --- a/drivers/tee/optee/shm_pool.h
> > +++ b/drivers/tee/optee/shm_pool.h
> > @@ -9,6 +9,6 @@
> >
> > #include <linux/tee_drv.h>
> >
> > -struct tee_shm_pool_mgr *optee_shm_pool_alloc_pages(void);
> > +struct tee_shm_pool *optee_shm_pool_alloc_pages(void);
> >
> > #endif
> > diff --git a/drivers/tee/tee_private.h b/drivers/tee/tee_private.h
> > index e55204df31ce..72376cf38bc0 100644
> > --- a/drivers/tee/tee_private.h
> > +++ b/drivers/tee/tee_private.h
> > @@ -12,17 +12,6 @@
> > #include <linux/mutex.h>
> > #include <linux/types.h>
> >
> > -/**
> > - * struct tee_shm_pool - shared memory pool
> > - * @private_mgr: pool manager for shared memory only between kernel
> > - * and secure world
> > - * @dma_buf_mgr: pool manager for shared memory exported to user space
> > - */
> > -struct tee_shm_pool {
> > - struct tee_shm_pool_mgr *private_mgr;
> > - struct tee_shm_pool_mgr *dma_buf_mgr;
> > -};
> > -
> > #define TEE_DEVICE_FLAG_REGISTERED 0x1
> > #define TEE_MAX_DEV_NAME_LEN 32
> >
> > diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c
> > index 00472f5ce22e..b9dbf4bce149 100644
> > --- a/drivers/tee/tee_shm.c
> > +++ b/drivers/tee/tee_shm.c
> > @@ -39,14 +39,7 @@ static void tee_shm_release(struct tee_shm *shm)
> > }
> >
> > if (shm->flags & TEE_SHM_POOL) {
> > - struct tee_shm_pool_mgr *poolm;
> > -
> > - if (shm->flags & TEE_SHM_DMA_BUF)
> > - poolm = teedev->pool->dma_buf_mgr;
> > - else
> > - poolm = teedev->pool->private_mgr;
> > -
> > - poolm->ops->free(poolm, shm);
> > + teedev->pool->ops->free(teedev->pool, shm);
> > } else if (shm->flags & TEE_SHM_REGISTER) {
> > int rc = teedev->desc->ops->shm_unregister(shm->ctx, shm);
> >
> > @@ -106,8 +99,8 @@ static const struct dma_buf_ops tee_shm_dma_buf_ops = {
> > struct tee_shm *tee_shm_alloc(struct tee_context *ctx, size_t size, u32 flags)
> > {
> > struct tee_device *teedev = ctx->teedev;
> > - struct tee_shm_pool_mgr *poolm = NULL;
> > struct tee_shm *shm;
> > + size_t align;
>
> You use u_int for the align parameter type in the .alloc function
> prototype. Perhaps switch it to size_t to match the argument that
> you're passing in?
OK, I'll fix.
>
> > void *ret;
> > int rc;
> >
> > @@ -139,12 +132,18 @@ struct tee_shm *tee_shm_alloc(struct tee_context *ctx, size_t size, u32 flags)
> >
> > shm->flags = flags | TEE_SHM_POOL;
> > shm->ctx = ctx;
> > - if (flags & TEE_SHM_DMA_BUF)
> > - poolm = teedev->pool->dma_buf_mgr;
> > - else
> > - poolm = teedev->pool->private_mgr;
> > + if (flags & TEE_SHM_DMA_BUF) {
> > + align = PAGE_SIZE;
> > + /*
> > + * Request to register the shm in the pool allocator below
> > + * if supported.
> > + */
> > + shm->flags |= TEE_SHM_REGISTER;
> > + } else {
> > + align = 2 * sizeof(long);
>
> Where does this alignment requirement come from?
There are some Arm instructions that requires double word aligned pointers.
I believe it's good practice for a memory allocator to keep the returned
buffers at least double word aligned, unless there's a specific reason
to do otherwise.
>
> > + }
> >
> > - rc = poolm->ops->alloc(poolm, shm, size);
> > + rc = teedev->pool->ops->alloc(teedev->pool, shm, size, align);
> > if (rc) {
> > ret = ERR_PTR(rc);
> > goto err_kfree;
> > @@ -184,7 +183,7 @@ struct tee_shm *tee_shm_alloc(struct tee_context *ctx, size_t size, u32 flags)
> > mutex_unlock(&teedev->mutex);
> > }
> > err_pool_free:
> > - poolm->ops->free(poolm, shm);
> > + teedev->pool->ops->free(teedev->pool, shm);
> > err_kfree:
> > kfree(shm);
> > err_dev_put:
> > diff --git a/drivers/tee/tee_shm_pool.c b/drivers/tee/tee_shm_pool.c
> > index a9f9d50fd181..5ceab73ff1b5 100644
> > --- a/drivers/tee/tee_shm_pool.c
> > +++ b/drivers/tee/tee_shm_pool.c
> > @@ -9,14 +9,16 @@
> > #include <linux/tee_drv.h>
> > #include "tee_private.h"
> >
> > -static int pool_op_gen_alloc(struct tee_shm_pool_mgr *poolm,
> > - struct tee_shm *shm, size_t size)
> > +static int pool_op_gen_alloc(struct tee_shm_pool *pool, struct tee_shm *shm,
> > + size_t size, u_int align)
> > {
> > unsigned long va;
> > - struct gen_pool *genpool = poolm->private_data;
> > - size_t s = roundup(size, 1 << genpool->min_alloc_order);
> > + struct gen_pool *genpool = pool->private_data;
> > + size_t a = max(align, 1U << genpool->min_alloc_order);
> > + struct genpool_data_align data = { .align = a };
> > + size_t s = roundup(size, a);
> >
> > - va = gen_pool_alloc(genpool, s);
> > + va = gen_pool_alloc_algo(genpool, s, gen_pool_first_fit_align, &data);
>
> This is an algo change from the existing use of gen_pool_best_fit. Was
> that intentional? If so, a mention of the reasoning for this change in
> the commit message might be helpful in the future.
Best-fit was the default choice and not a conscious choice from the
beginning. We're doing first-fit now since that supports requesting an
alignment, while best-fit doesn't. I'll add something in the commit
message.
Cheers,
Jens
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 3/7] tee: add tee_shm_alloc_kernel_buf()
2021-06-09 10:23 [PATCH 0/7] tee: shared memory updates Jens Wiklander
2021-06-09 10:23 ` [PATCH 1/7] tee: remove unused tee_shm_pool_alloc_res_mem() Jens Wiklander
2021-06-09 10:23 ` [PATCH 2/7] tee: simplify shm pool handling Jens Wiklander
@ 2021-06-09 10:23 ` Jens Wiklander
2021-06-09 14:51 ` Tyler Hicks
2021-06-09 10:23 ` [PATCH 4/7] hwrng: optee-rng: use tee_shm_alloc_kernel_buf() Jens Wiklander
` (3 subsequent siblings)
6 siblings, 1 reply; 19+ messages in thread
From: Jens Wiklander @ 2021-06-09 10:23 UTC (permalink / raw)
To: linux-kernel, linux-arm-kernel, op-tee
Cc: Sumit Garg, Herbert Xu, Sakkinen, Sasha Levin,
Thirupathaiah Annapureddy, Vikas Gupta, David S . Miller,
Tyler Hicks, Jens Wiklander
Adds a new function tee_shm_alloc_kernel_buf() to allocate shared memory
from a kernel driver. This function can later be made more lightweight
by unnecessary dma-buf export.
Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
---
drivers/tee/tee_shm.c | 18 ++++++++++++++++++
include/linux/tee_drv.h | 1 +
2 files changed, 19 insertions(+)
diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c
index b9dbf4bce149..63fce8d39d8b 100644
--- a/drivers/tee/tee_shm.c
+++ b/drivers/tee/tee_shm.c
@@ -192,6 +192,24 @@ struct tee_shm *tee_shm_alloc(struct tee_context *ctx, size_t size, u32 flags)
}
EXPORT_SYMBOL_GPL(tee_shm_alloc);
+/**
+ * tee_shm_alloc_kernel_buf() - Allocate shared memory for kernel buffer
+ * @ctx: Context that allocates the shared memory
+ * @size: Requested size of shared memory
+ *
+ * The returned memory registered in secure world and is suitable to be
+ * passed as a memory buffer in parameter argument to
+ * tee_client_invoke_func(). The memory allocated is later freed with a
+ * call to tee_shm_free().
+ *
+ * @returns a pointer to 'struct tee_shm'
+ */
+struct tee_shm *tee_shm_alloc_kernel_buf(struct tee_context *ctx, size_t size)
+{
+ return tee_shm_alloc(ctx, size, TEE_SHM_MAPPED | TEE_SHM_DMA_BUF);
+}
+EXPORT_SYMBOL_GPL(tee_shm_alloc_kernel_buf);
+
struct tee_shm *tee_shm_register(struct tee_context *ctx, unsigned long addr,
size_t length, u32 flags)
{
diff --git a/include/linux/tee_drv.h b/include/linux/tee_drv.h
index 62b7c7a55743..58b319766f8e 100644
--- a/include/linux/tee_drv.h
+++ b/include/linux/tee_drv.h
@@ -282,6 +282,7 @@ void *tee_get_drvdata(struct tee_device *teedev);
* @returns a pointer to 'struct tee_shm'
*/
struct tee_shm *tee_shm_alloc(struct tee_context *ctx, size_t size, u32 flags);
+struct tee_shm *tee_shm_alloc_kernel_buf(struct tee_context *ctx, size_t size);
/**
* tee_shm_register() - Register shared memory buffer
--
2.31.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH 3/7] tee: add tee_shm_alloc_kernel_buf()
2021-06-09 10:23 ` [PATCH 3/7] tee: add tee_shm_alloc_kernel_buf() Jens Wiklander
@ 2021-06-09 14:51 ` Tyler Hicks
0 siblings, 0 replies; 19+ messages in thread
From: Tyler Hicks @ 2021-06-09 14:51 UTC (permalink / raw)
To: Jens Wiklander
Cc: linux-kernel, linux-arm-kernel, op-tee, Sumit Garg, Herbert Xu,
Sakkinen, Sasha Levin, Thirupathaiah Annapureddy, Vikas Gupta,
David S . Miller
On 2021-06-09 12:23:20, Jens Wiklander wrote:
> Adds a new function tee_shm_alloc_kernel_buf() to allocate shared memory
> from a kernel driver. This function can later be made more lightweight
> by unnecessary dma-buf export.
>
> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
Reviewed-by: Tyler Hicks <tyhicks@linux.microsoft.com>
Tyler
> ---
> drivers/tee/tee_shm.c | 18 ++++++++++++++++++
> include/linux/tee_drv.h | 1 +
> 2 files changed, 19 insertions(+)
>
> diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c
> index b9dbf4bce149..63fce8d39d8b 100644
> --- a/drivers/tee/tee_shm.c
> +++ b/drivers/tee/tee_shm.c
> @@ -192,6 +192,24 @@ struct tee_shm *tee_shm_alloc(struct tee_context *ctx, size_t size, u32 flags)
> }
> EXPORT_SYMBOL_GPL(tee_shm_alloc);
>
> +/**
> + * tee_shm_alloc_kernel_buf() - Allocate shared memory for kernel buffer
> + * @ctx: Context that allocates the shared memory
> + * @size: Requested size of shared memory
> + *
> + * The returned memory registered in secure world and is suitable to be
> + * passed as a memory buffer in parameter argument to
> + * tee_client_invoke_func(). The memory allocated is later freed with a
> + * call to tee_shm_free().
> + *
> + * @returns a pointer to 'struct tee_shm'
> + */
> +struct tee_shm *tee_shm_alloc_kernel_buf(struct tee_context *ctx, size_t size)
> +{
> + return tee_shm_alloc(ctx, size, TEE_SHM_MAPPED | TEE_SHM_DMA_BUF);
> +}
> +EXPORT_SYMBOL_GPL(tee_shm_alloc_kernel_buf);
> +
> struct tee_shm *tee_shm_register(struct tee_context *ctx, unsigned long addr,
> size_t length, u32 flags)
> {
> diff --git a/include/linux/tee_drv.h b/include/linux/tee_drv.h
> index 62b7c7a55743..58b319766f8e 100644
> --- a/include/linux/tee_drv.h
> +++ b/include/linux/tee_drv.h
> @@ -282,6 +282,7 @@ void *tee_get_drvdata(struct tee_device *teedev);
> * @returns a pointer to 'struct tee_shm'
> */
> struct tee_shm *tee_shm_alloc(struct tee_context *ctx, size_t size, u32 flags);
> +struct tee_shm *tee_shm_alloc_kernel_buf(struct tee_context *ctx, size_t size);
>
> /**
> * tee_shm_register() - Register shared memory buffer
> --
> 2.31.1
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 4/7] hwrng: optee-rng: use tee_shm_alloc_kernel_buf()
2021-06-09 10:23 [PATCH 0/7] tee: shared memory updates Jens Wiklander
` (2 preceding siblings ...)
2021-06-09 10:23 ` [PATCH 3/7] tee: add tee_shm_alloc_kernel_buf() Jens Wiklander
@ 2021-06-09 10:23 ` Jens Wiklander
2021-06-09 14:51 ` Tyler Hicks
2021-06-09 10:23 ` [PATCH 5/7] tpm_ftpm_tee: " Jens Wiklander
` (2 subsequent siblings)
6 siblings, 1 reply; 19+ messages in thread
From: Jens Wiklander @ 2021-06-09 10:23 UTC (permalink / raw)
To: linux-kernel, linux-arm-kernel, op-tee
Cc: Sumit Garg, Herbert Xu, Sakkinen, Sasha Levin,
Thirupathaiah Annapureddy, Vikas Gupta, David S . Miller,
Tyler Hicks, Jens Wiklander
Uses the new simplified tee_shm_alloc_kernel_buf() function instead of
the old deprecated tee_shm_alloc() function which required specific
TEE_SHM-flags.
Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
---
drivers/char/hw_random/optee-rng.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/char/hw_random/optee-rng.c b/drivers/char/hw_random/optee-rng.c
index 135a82590923..a948c0727b2b 100644
--- a/drivers/char/hw_random/optee-rng.c
+++ b/drivers/char/hw_random/optee-rng.c
@@ -145,10 +145,10 @@ static int optee_rng_init(struct hwrng *rng)
struct optee_rng_private *pvt_data = to_optee_rng_private(rng);
struct tee_shm *entropy_shm_pool = NULL;
- entropy_shm_pool = tee_shm_alloc(pvt_data->ctx, MAX_ENTROPY_REQ_SZ,
- TEE_SHM_MAPPED | TEE_SHM_DMA_BUF);
+ entropy_shm_pool = tee_shm_alloc_kernel_buf(pvt_data->ctx,
+ MAX_ENTROPY_REQ_SZ);
if (IS_ERR(entropy_shm_pool)) {
- dev_err(pvt_data->dev, "tee_shm_alloc failed\n");
+ dev_err(pvt_data->dev, "tee_shm_alloc_kernel_buf failed\n");
return PTR_ERR(entropy_shm_pool);
}
--
2.31.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH 4/7] hwrng: optee-rng: use tee_shm_alloc_kernel_buf()
2021-06-09 10:23 ` [PATCH 4/7] hwrng: optee-rng: use tee_shm_alloc_kernel_buf() Jens Wiklander
@ 2021-06-09 14:51 ` Tyler Hicks
0 siblings, 0 replies; 19+ messages in thread
From: Tyler Hicks @ 2021-06-09 14:51 UTC (permalink / raw)
To: Jens Wiklander
Cc: linux-kernel, linux-arm-kernel, op-tee, Sumit Garg, Herbert Xu,
Sakkinen, Sasha Levin, Thirupathaiah Annapureddy, Vikas Gupta,
David S . Miller
On 2021-06-09 12:23:21, Jens Wiklander wrote:
> Uses the new simplified tee_shm_alloc_kernel_buf() function instead of
> the old deprecated tee_shm_alloc() function which required specific
> TEE_SHM-flags.
>
> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
Reviewed-by: Tyler Hicks <tyhicks@linux.microsoft.com>
Tyler
> ---
> drivers/char/hw_random/optee-rng.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/char/hw_random/optee-rng.c b/drivers/char/hw_random/optee-rng.c
> index 135a82590923..a948c0727b2b 100644
> --- a/drivers/char/hw_random/optee-rng.c
> +++ b/drivers/char/hw_random/optee-rng.c
> @@ -145,10 +145,10 @@ static int optee_rng_init(struct hwrng *rng)
> struct optee_rng_private *pvt_data = to_optee_rng_private(rng);
> struct tee_shm *entropy_shm_pool = NULL;
>
> - entropy_shm_pool = tee_shm_alloc(pvt_data->ctx, MAX_ENTROPY_REQ_SZ,
> - TEE_SHM_MAPPED | TEE_SHM_DMA_BUF);
> + entropy_shm_pool = tee_shm_alloc_kernel_buf(pvt_data->ctx,
> + MAX_ENTROPY_REQ_SZ);
> if (IS_ERR(entropy_shm_pool)) {
> - dev_err(pvt_data->dev, "tee_shm_alloc failed\n");
> + dev_err(pvt_data->dev, "tee_shm_alloc_kernel_buf failed\n");
> return PTR_ERR(entropy_shm_pool);
> }
>
> --
> 2.31.1
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 5/7] tpm_ftpm_tee: use tee_shm_alloc_kernel_buf()
2021-06-09 10:23 [PATCH 0/7] tee: shared memory updates Jens Wiklander
` (3 preceding siblings ...)
2021-06-09 10:23 ` [PATCH 4/7] hwrng: optee-rng: use tee_shm_alloc_kernel_buf() Jens Wiklander
@ 2021-06-09 10:23 ` Jens Wiklander
2021-06-09 14:53 ` Tyler Hicks
2021-06-09 10:23 ` [PATCH 6/7] firmware: tee_bnxt: " Jens Wiklander
2021-06-09 10:23 ` [PATCH 7/7] tee: replace tee_shm_alloc() Jens Wiklander
6 siblings, 1 reply; 19+ messages in thread
From: Jens Wiklander @ 2021-06-09 10:23 UTC (permalink / raw)
To: linux-kernel, linux-arm-kernel, op-tee
Cc: Sumit Garg, Herbert Xu, Sakkinen, Sasha Levin,
Thirupathaiah Annapureddy, Vikas Gupta, David S . Miller,
Tyler Hicks, Jens Wiklander
Uses the new simplified tee_shm_alloc_kernel_buf() function instead of
the old deprecated tee_shm_alloc() function which required specific
TEE_SHM-flags.
Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
---
drivers/char/tpm/tpm_ftpm_tee.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/char/tpm/tpm_ftpm_tee.c b/drivers/char/tpm/tpm_ftpm_tee.c
index 2ccdf8ac6994..6e3235565a4d 100644
--- a/drivers/char/tpm/tpm_ftpm_tee.c
+++ b/drivers/char/tpm/tpm_ftpm_tee.c
@@ -254,11 +254,11 @@ static int ftpm_tee_probe(struct device *dev)
pvt_data->session = sess_arg.session;
/* Allocate dynamic shared memory with fTPM TA */
- pvt_data->shm = tee_shm_alloc(pvt_data->ctx,
- MAX_COMMAND_SIZE + MAX_RESPONSE_SIZE,
- TEE_SHM_MAPPED | TEE_SHM_DMA_BUF);
+ pvt_data->shm = tee_shm_alloc_kernel_buf(pvt_data->ctx,
+ MAX_COMMAND_SIZE +
+ MAX_RESPONSE_SIZE);
if (IS_ERR(pvt_data->shm)) {
- dev_err(dev, "%s: tee_shm_alloc failed\n", __func__);
+ dev_err(dev, "%s: tee_shm_alloc_kernel_buf failed\n", __func__);
rc = -ENOMEM;
goto out_shm_alloc;
}
--
2.31.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH 5/7] tpm_ftpm_tee: use tee_shm_alloc_kernel_buf()
2021-06-09 10:23 ` [PATCH 5/7] tpm_ftpm_tee: " Jens Wiklander
@ 2021-06-09 14:53 ` Tyler Hicks
0 siblings, 0 replies; 19+ messages in thread
From: Tyler Hicks @ 2021-06-09 14:53 UTC (permalink / raw)
To: Jens Wiklander
Cc: linux-kernel, linux-arm-kernel, op-tee, Sumit Garg, Herbert Xu,
Sakkinen, Sasha Levin, Thirupathaiah Annapureddy, Vikas Gupta,
David S . Miller
On 2021-06-09 12:23:22, Jens Wiklander wrote:
> Uses the new simplified tee_shm_alloc_kernel_buf() function instead of
> the old deprecated tee_shm_alloc() function which required specific
> TEE_SHM-flags.
>
> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
Reviewed-by: Tyler Hicks <tyhicks@linux.microsoft.com>
Tyler
> ---
> drivers/char/tpm/tpm_ftpm_tee.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm_ftpm_tee.c b/drivers/char/tpm/tpm_ftpm_tee.c
> index 2ccdf8ac6994..6e3235565a4d 100644
> --- a/drivers/char/tpm/tpm_ftpm_tee.c
> +++ b/drivers/char/tpm/tpm_ftpm_tee.c
> @@ -254,11 +254,11 @@ static int ftpm_tee_probe(struct device *dev)
> pvt_data->session = sess_arg.session;
>
> /* Allocate dynamic shared memory with fTPM TA */
> - pvt_data->shm = tee_shm_alloc(pvt_data->ctx,
> - MAX_COMMAND_SIZE + MAX_RESPONSE_SIZE,
> - TEE_SHM_MAPPED | TEE_SHM_DMA_BUF);
> + pvt_data->shm = tee_shm_alloc_kernel_buf(pvt_data->ctx,
> + MAX_COMMAND_SIZE +
> + MAX_RESPONSE_SIZE);
> if (IS_ERR(pvt_data->shm)) {
> - dev_err(dev, "%s: tee_shm_alloc failed\n", __func__);
> + dev_err(dev, "%s: tee_shm_alloc_kernel_buf failed\n", __func__);
> rc = -ENOMEM;
> goto out_shm_alloc;
> }
> --
> 2.31.1
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 6/7] firmware: tee_bnxt: use tee_shm_alloc_kernel_buf()
2021-06-09 10:23 [PATCH 0/7] tee: shared memory updates Jens Wiklander
` (4 preceding siblings ...)
2021-06-09 10:23 ` [PATCH 5/7] tpm_ftpm_tee: " Jens Wiklander
@ 2021-06-09 10:23 ` Jens Wiklander
2021-06-09 14:58 ` Tyler Hicks
2021-06-09 10:23 ` [PATCH 7/7] tee: replace tee_shm_alloc() Jens Wiklander
6 siblings, 1 reply; 19+ messages in thread
From: Jens Wiklander @ 2021-06-09 10:23 UTC (permalink / raw)
To: linux-kernel, linux-arm-kernel, op-tee
Cc: Sumit Garg, Herbert Xu, Sakkinen, Sasha Levin,
Thirupathaiah Annapureddy, Vikas Gupta, David S . Miller,
Tyler Hicks, Jens Wiklander
Uses the new simplified tee_shm_alloc_kernel_buf() function instead of
the old deprecated tee_shm_alloc() function which required specific
TEE_SHM-flags.
Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
---
drivers/firmware/broadcom/tee_bnxt_fw.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/firmware/broadcom/tee_bnxt_fw.c b/drivers/firmware/broadcom/tee_bnxt_fw.c
index ed10da5313e8..56d00ddd4357 100644
--- a/drivers/firmware/broadcom/tee_bnxt_fw.c
+++ b/drivers/firmware/broadcom/tee_bnxt_fw.c
@@ -212,10 +212,9 @@ static int tee_bnxt_fw_probe(struct device *dev)
pvt_data.dev = dev;
- fw_shm_pool = tee_shm_alloc(pvt_data.ctx, MAX_SHM_MEM_SZ,
- TEE_SHM_MAPPED | TEE_SHM_DMA_BUF);
+ fw_shm_pool = tee_shm_alloc_kernel_buf(pvt_data.ctx, MAX_SHM_MEM_SZ);
if (IS_ERR(fw_shm_pool)) {
- dev_err(pvt_data.dev, "tee_shm_alloc failed\n");
+ dev_err(pvt_data.dev, "tee_shm_alloc_kernel_buf failed\n");
err = PTR_ERR(fw_shm_pool);
goto out_sess;
}
--
2.31.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH 6/7] firmware: tee_bnxt: use tee_shm_alloc_kernel_buf()
2021-06-09 10:23 ` [PATCH 6/7] firmware: tee_bnxt: " Jens Wiklander
@ 2021-06-09 14:58 ` Tyler Hicks
2021-06-10 9:08 ` Jens Wiklander
0 siblings, 1 reply; 19+ messages in thread
From: Tyler Hicks @ 2021-06-09 14:58 UTC (permalink / raw)
To: Jens Wiklander
Cc: linux-kernel, linux-arm-kernel, op-tee, Sumit Garg, Herbert Xu,
Sakkinen, Sasha Levin, Thirupathaiah Annapureddy, Vikas Gupta,
David S . Miller
On 2021-06-09 12:23:23, Jens Wiklander wrote:
> Uses the new simplified tee_shm_alloc_kernel_buf() function instead of
> the old deprecated tee_shm_alloc() function which required specific
> TEE_SHM-flags.
>
> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
Since this series is essentially a rewrite of the shm allocation logic,
it is worth pointing out that the rewrite still uses contiguous
allocations (from alloc_pages()). The tee_bnxt_fw driver is performing
an order-10 allocation which is the max, by default. I've only tested
tee_bnxt_fw when it was built-in to the kernel and tee_bnxt_fw_probe()
was called early in boot but I suspect that it might not succeed when
built as a module and loaded later after memory is segmented. I think
this driver would benefit from being able to request a non-contiguous
allocation.
Is this rewrite a good time to offer drivers a way to perform a
non-contiguous allocation?
Tyler
> ---
> drivers/firmware/broadcom/tee_bnxt_fw.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/firmware/broadcom/tee_bnxt_fw.c b/drivers/firmware/broadcom/tee_bnxt_fw.c
> index ed10da5313e8..56d00ddd4357 100644
> --- a/drivers/firmware/broadcom/tee_bnxt_fw.c
> +++ b/drivers/firmware/broadcom/tee_bnxt_fw.c
> @@ -212,10 +212,9 @@ static int tee_bnxt_fw_probe(struct device *dev)
>
> pvt_data.dev = dev;
>
> - fw_shm_pool = tee_shm_alloc(pvt_data.ctx, MAX_SHM_MEM_SZ,
> - TEE_SHM_MAPPED | TEE_SHM_DMA_BUF);
> + fw_shm_pool = tee_shm_alloc_kernel_buf(pvt_data.ctx, MAX_SHM_MEM_SZ);
> if (IS_ERR(fw_shm_pool)) {
> - dev_err(pvt_data.dev, "tee_shm_alloc failed\n");
> + dev_err(pvt_data.dev, "tee_shm_alloc_kernel_buf failed\n");
> err = PTR_ERR(fw_shm_pool);
> goto out_sess;
> }
> --
> 2.31.1
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 6/7] firmware: tee_bnxt: use tee_shm_alloc_kernel_buf()
2021-06-09 14:58 ` Tyler Hicks
@ 2021-06-10 9:08 ` Jens Wiklander
0 siblings, 0 replies; 19+ messages in thread
From: Jens Wiklander @ 2021-06-10 9:08 UTC (permalink / raw)
To: Tyler Hicks
Cc: Linux Kernel Mailing List, Linux ARM, OP-TEE TrustedFirmware,
Sumit Garg, Herbert Xu, Sakkinen, Sasha Levin,
Thirupathaiah Annapureddy, Vikas Gupta, David S . Miller
On Wed, Jun 9, 2021 at 4:58 PM Tyler Hicks <tyhicks@linux.microsoft.com> wrote:
>
> On 2021-06-09 12:23:23, Jens Wiklander wrote:
> > Uses the new simplified tee_shm_alloc_kernel_buf() function instead of
> > the old deprecated tee_shm_alloc() function which required specific
> > TEE_SHM-flags.
> >
> > Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
>
> Since this series is essentially a rewrite of the shm allocation logic,
> it is worth pointing out that the rewrite still uses contiguous
> allocations (from alloc_pages()). The tee_bnxt_fw driver is performing
> an order-10 allocation which is the max, by default. I've only tested
> tee_bnxt_fw when it was built-in to the kernel and tee_bnxt_fw_probe()
> was called early in boot but I suspect that it might not succeed when
> built as a module and loaded later after memory is segmented. I think
> this driver would benefit from being able to request a non-contiguous
> allocation.
>
> Is this rewrite a good time to offer drivers a way to perform a
> non-contiguous allocation?
Good idea, I'll look into that. I'll add it as a separate patch if it works OK.
Cheers,
Jens
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 7/7] tee: replace tee_shm_alloc()
2021-06-09 10:23 [PATCH 0/7] tee: shared memory updates Jens Wiklander
` (5 preceding siblings ...)
2021-06-09 10:23 ` [PATCH 6/7] firmware: tee_bnxt: " Jens Wiklander
@ 2021-06-09 10:23 ` Jens Wiklander
2021-06-09 15:32 ` Tyler Hicks
6 siblings, 1 reply; 19+ messages in thread
From: Jens Wiklander @ 2021-06-09 10:23 UTC (permalink / raw)
To: linux-kernel, linux-arm-kernel, op-tee
Cc: Sumit Garg, Herbert Xu, Sakkinen, Sasha Levin,
Thirupathaiah Annapureddy, Vikas Gupta, David S . Miller,
Tyler Hicks, Jens Wiklander
tee_shm_alloc() is replaced by three new functions,
tee_shm_alloc_user_buf() - for user mode allocations, replacing passing
the flags TEE_SHM_MAPPED | TEE_SHM_DMA_BUF
tee_shm_alloc_kernel_buf() - for kernel mode allocations, slightly
optimized compared to using the flags TEE_SHM_MAPPED | TEE_SHM_DMA_BUF
since we now can avoid using the dma-buf registration.
tee_shm_alloc_anon_kernel_buf() - for TEE driver internal use only
allowing decoupling a shared memory object from its original
tee_context.
This also makes the interface easier to use as we can get rid of the
somewhat hard to use flags parameter.
The TEE subsystem and the TEE drivers are updated to use the new
functions instead.
Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
---
drivers/tee/optee/call.c | 16 ++--
drivers/tee/optee/core.c | 4 +-
drivers/tee/optee/device.c | 5 +-
drivers/tee/optee/rpc.c | 8 +-
drivers/tee/tee_core.c | 2 +-
drivers/tee/tee_shm.c | 186 +++++++++++++++++++++++++++----------
include/linux/tee_drv.h | 19 +---
7 files changed, 156 insertions(+), 84 deletions(-)
diff --git a/drivers/tee/optee/call.c b/drivers/tee/optee/call.c
index 6132cc8d014c..f31257649c0e 100644
--- a/drivers/tee/optee/call.c
+++ b/drivers/tee/optee/call.c
@@ -183,8 +183,8 @@ static struct tee_shm *get_msg_arg(struct tee_context *ctx, size_t num_params,
struct tee_shm *shm;
struct optee_msg_arg *ma;
- shm = tee_shm_alloc(ctx, OPTEE_MSG_GET_ARG_SIZE(num_params),
- TEE_SHM_MAPPED);
+ shm = tee_shm_alloc_anon_kernel_buf(ctx,
+ OPTEE_MSG_GET_ARG_SIZE(num_params));
if (IS_ERR(shm))
return shm;
@@ -281,7 +281,7 @@ int optee_open_session(struct tee_context *ctx,
arg->ret_origin = msg_arg->ret_origin;
}
out:
- tee_shm_free(shm);
+ tee_shm_free_anon_kernel_buf(ctx, shm);
return rc;
}
@@ -312,7 +312,7 @@ int optee_close_session(struct tee_context *ctx, u32 session)
msg_arg->session = session;
optee_do_call_with_arg(ctx, msg_parg);
- tee_shm_free(shm);
+ tee_shm_free_anon_kernel_buf(ctx, shm);
return 0;
}
@@ -358,7 +358,7 @@ int optee_invoke_func(struct tee_context *ctx, struct tee_ioctl_invoke_arg *arg,
arg->ret = msg_arg->ret;
arg->ret_origin = msg_arg->ret_origin;
out:
- tee_shm_free(shm);
+ tee_shm_free_anon_kernel_buf(ctx, shm);
return rc;
}
@@ -386,7 +386,7 @@ int optee_cancel_req(struct tee_context *ctx, u32 cancel_id, u32 session)
msg_arg->cancel_id = cancel_id;
optee_do_call_with_arg(ctx, msg_parg);
- tee_shm_free(shm);
+ tee_shm_free_anon_kernel_buf(ctx, shm);
return 0;
}
@@ -625,7 +625,7 @@ int optee_shm_register(struct tee_context *ctx, struct tee_shm *shm,
msg_arg->ret != TEEC_SUCCESS)
rc = -EINVAL;
- tee_shm_free(shm_arg);
+ tee_shm_free_anon_kernel_buf(ctx, shm_arg);
out:
optee_free_pages_list(pages_list, num_pages);
return rc;
@@ -650,7 +650,7 @@ int optee_shm_unregister(struct tee_context *ctx, struct tee_shm *shm)
if (optee_do_call_with_arg(ctx, msg_parg) ||
msg_arg->ret != TEEC_SUCCESS)
rc = -EINVAL;
- tee_shm_free(shm_arg);
+ tee_shm_free_anon_kernel_buf(ctx, shm_arg);
return rc;
}
diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c
index 0c287345f9fe..a15dc3881636 100644
--- a/drivers/tee/optee/core.c
+++ b/drivers/tee/optee/core.c
@@ -277,7 +277,7 @@ static void optee_release(struct tee_context *ctx)
if (!ctxdata)
return;
- shm = tee_shm_alloc(ctx, sizeof(struct optee_msg_arg), TEE_SHM_MAPPED);
+ shm = tee_shm_alloc_anon_kernel_buf(ctx, sizeof(struct optee_msg_arg));
if (!IS_ERR(shm)) {
arg = tee_shm_get_va(shm, 0);
/*
@@ -305,7 +305,7 @@ static void optee_release(struct tee_context *ctx)
kfree(ctxdata);
if (!IS_ERR(shm))
- tee_shm_free(shm);
+ tee_shm_free_anon_kernel_buf(ctx, shm);
ctx->data = NULL;
diff --git a/drivers/tee/optee/device.c b/drivers/tee/optee/device.c
index ec1d24693eba..5a5bf86b1b95 100644
--- a/drivers/tee/optee/device.c
+++ b/drivers/tee/optee/device.c
@@ -113,10 +113,9 @@ static int __optee_enumerate_devices(u32 func)
if (rc < 0 || !shm_size)
goto out_sess;
- device_shm = tee_shm_alloc(ctx, shm_size,
- TEE_SHM_MAPPED | TEE_SHM_DMA_BUF);
+ device_shm = tee_shm_alloc_kernel_buf(ctx, shm_size);
if (IS_ERR(device_shm)) {
- pr_err("tee_shm_alloc failed\n");
+ pr_err("tee_shm_alloc_kernel_buf failed\n");
rc = PTR_ERR(device_shm);
goto out_sess;
}
diff --git a/drivers/tee/optee/rpc.c b/drivers/tee/optee/rpc.c
index 1849180b0278..9108aedb3eee 100644
--- a/drivers/tee/optee/rpc.c
+++ b/drivers/tee/optee/rpc.c
@@ -314,7 +314,7 @@ static void handle_rpc_func_cmd_shm_alloc(struct tee_context *ctx,
shm = cmd_alloc_suppl(ctx, sz);
break;
case OPTEE_RPC_SHM_TYPE_KERNEL:
- shm = tee_shm_alloc(ctx, sz, TEE_SHM_MAPPED);
+ shm = tee_shm_alloc_anon_kernel_buf(ctx, sz);
break;
default:
arg->ret = TEEC_ERROR_BAD_PARAMETERS;
@@ -424,7 +424,7 @@ static void handle_rpc_func_cmd_shm_free(struct tee_context *ctx,
cmd_free_suppl(ctx, shm);
break;
case OPTEE_RPC_SHM_TYPE_KERNEL:
- tee_shm_free(shm);
+ tee_shm_free_anon_kernel_buf(ctx, shm);
break;
default:
arg->ret = TEEC_ERROR_BAD_PARAMETERS;
@@ -502,7 +502,7 @@ void optee_handle_rpc(struct tee_context *ctx, struct optee_rpc_param *param,
switch (OPTEE_SMC_RETURN_GET_RPC_FUNC(param->a0)) {
case OPTEE_SMC_RPC_FUNC_ALLOC:
- shm = tee_shm_alloc(ctx, param->a1, TEE_SHM_MAPPED);
+ shm = tee_shm_alloc_anon_kernel_buf(ctx, param->a1);
if (!IS_ERR(shm) && !tee_shm_get_pa(shm, 0, &pa)) {
reg_pair_from_64(¶m->a1, ¶m->a2, pa);
reg_pair_from_64(¶m->a4, ¶m->a5,
@@ -516,7 +516,7 @@ void optee_handle_rpc(struct tee_context *ctx, struct optee_rpc_param *param,
break;
case OPTEE_SMC_RPC_FUNC_FREE:
shm = reg_pair_to_ptr(param->a1, param->a2);
- tee_shm_free(shm);
+ tee_shm_free_anon_kernel_buf(ctx, shm);
break;
case OPTEE_SMC_RPC_FUNC_FOREIGN_INTR:
/*
diff --git a/drivers/tee/tee_core.c b/drivers/tee/tee_core.c
index 480d294a23ab..4f5c7c17a434 100644
--- a/drivers/tee/tee_core.c
+++ b/drivers/tee/tee_core.c
@@ -293,7 +293,7 @@ static int tee_ioctl_shm_alloc(struct tee_context *ctx,
if (data.flags)
return -EINVAL;
- shm = tee_shm_alloc(ctx, data.size, TEE_SHM_MAPPED | TEE_SHM_DMA_BUF);
+ shm = tee_shm_alloc_user_buf(ctx, data.size);
if (IS_ERR(shm))
return PTR_ERR(shm);
diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c
index 63fce8d39d8b..d134e2778a3a 100644
--- a/drivers/tee/tee_shm.c
+++ b/drivers/tee/tee_shm.c
@@ -96,25 +96,14 @@ static const struct dma_buf_ops tee_shm_dma_buf_ops = {
.mmap = tee_shm_op_mmap,
};
-struct tee_shm *tee_shm_alloc(struct tee_context *ctx, size_t size, u32 flags)
+static struct tee_shm *shm_alloc_helper(struct tee_context *ctx, size_t size,
+ size_t align, u32 flags)
{
struct tee_device *teedev = ctx->teedev;
struct tee_shm *shm;
- size_t align;
void *ret;
int rc;
- if (!(flags & TEE_SHM_MAPPED)) {
- dev_err(teedev->dev.parent,
- "only mapped allocations supported\n");
- return ERR_PTR(-EINVAL);
- }
-
- if ((flags & ~(TEE_SHM_MAPPED | TEE_SHM_DMA_BUF))) {
- dev_err(teedev->dev.parent, "invalid shm flags 0x%x", flags);
- return ERR_PTR(-EINVAL);
- }
-
if (!tee_device_get(teedev))
return ERR_PTR(-EINVAL);
@@ -131,17 +120,14 @@ struct tee_shm *tee_shm_alloc(struct tee_context *ctx, size_t size, u32 flags)
}
shm->flags = flags | TEE_SHM_POOL;
+
+ /*
+ * We're assigning this as it is needed if the shm is to be
+ * registered. If this function returns OK then the caller expected
+ * to call teedev_ctx_get() or clear shm->ctx in case it's not
+ * needed any longer.
+ */
shm->ctx = ctx;
- if (flags & TEE_SHM_DMA_BUF) {
- align = PAGE_SIZE;
- /*
- * Request to register the shm in the pool allocator below
- * if supported.
- */
- shm->flags |= TEE_SHM_REGISTER;
- } else {
- align = 2 * sizeof(long);
- }
rc = teedev->pool->ops->alloc(teedev->pool, shm, size, align);
if (rc) {
@@ -149,48 +135,71 @@ struct tee_shm *tee_shm_alloc(struct tee_context *ctx, size_t size, u32 flags)
goto err_kfree;
}
+ return shm;
+err_kfree:
+ kfree(shm);
+err_dev_put:
+ tee_device_put(teedev);
+ return ret;
+}
- if (flags & TEE_SHM_DMA_BUF) {
- DEFINE_DMA_BUF_EXPORT_INFO(exp_info);
+/**
+ * tee_shm_alloc_user_buf() - Allocate shared memory for user space
+ * @ctx: Context that allocates the shared memory
+ * @size: Requested size of shared memory
+ *
+ * Memory allocated as user space shared memory is automatically freed when
+ * the TEE file pointer is closed. The primary usage of this function is
+ * when the TEE driver doesn't support registering ordinary user space
+ * memory.
+ *
+ * @returns a pointer to 'struct tee_shm'
+ */
+struct tee_shm *tee_shm_alloc_user_buf(struct tee_context *ctx, size_t size)
+{
+ u32 flags = TEE_SHM_MAPPED | TEE_SHM_DMA_BUF | TEE_SHM_REGISTER;
+ struct tee_device *teedev = ctx->teedev;
+ DEFINE_DMA_BUF_EXPORT_INFO(exp_info);
+ struct tee_shm *shm;
+ void *ret;
- mutex_lock(&teedev->mutex);
- shm->id = idr_alloc(&teedev->idr, shm, 1, 0, GFP_KERNEL);
- mutex_unlock(&teedev->mutex);
- if (shm->id < 0) {
- ret = ERR_PTR(shm->id);
- goto err_pool_free;
- }
+ shm = shm_alloc_helper(ctx, size, PAGE_SIZE, flags);
+ if (IS_ERR(shm))
+ return shm;
- exp_info.ops = &tee_shm_dma_buf_ops;
- exp_info.size = shm->size;
- exp_info.flags = O_RDWR;
- exp_info.priv = shm;
+ mutex_lock(&teedev->mutex);
+ shm->id = idr_alloc(&teedev->idr, shm, 1, 0, GFP_KERNEL);
+ mutex_unlock(&teedev->mutex);
+ if (shm->id < 0) {
+ ret = ERR_PTR(shm->id);
+ goto err_pool_free;
+ }
- shm->dmabuf = dma_buf_export(&exp_info);
- if (IS_ERR(shm->dmabuf)) {
- ret = ERR_CAST(shm->dmabuf);
- goto err_rem;
- }
+ exp_info.ops = &tee_shm_dma_buf_ops;
+ exp_info.size = shm->size;
+ exp_info.flags = O_RDWR;
+ exp_info.priv = shm;
+
+ shm->dmabuf = dma_buf_export(&exp_info);
+ if (IS_ERR(shm->dmabuf)) {
+ ret = ERR_CAST(shm->dmabuf);
+ goto err_rem;
}
teedev_ctx_get(ctx);
-
return shm;
err_rem:
- if (flags & TEE_SHM_DMA_BUF) {
- mutex_lock(&teedev->mutex);
- idr_remove(&teedev->idr, shm->id);
- mutex_unlock(&teedev->mutex);
- }
+ mutex_lock(&teedev->mutex);
+ idr_remove(&teedev->idr, shm->id);
+ mutex_unlock(&teedev->mutex);
err_pool_free:
teedev->pool->ops->free(teedev->pool, shm);
-err_kfree:
kfree(shm);
-err_dev_put:
tee_device_put(teedev);
return ret;
+
}
-EXPORT_SYMBOL_GPL(tee_shm_alloc);
+EXPORT_SYMBOL_GPL(tee_shm_alloc_user_buf);
/**
* tee_shm_alloc_kernel_buf() - Allocate shared memory for kernel buffer
@@ -206,10 +215,85 @@ EXPORT_SYMBOL_GPL(tee_shm_alloc);
*/
struct tee_shm *tee_shm_alloc_kernel_buf(struct tee_context *ctx, size_t size)
{
- return tee_shm_alloc(ctx, size, TEE_SHM_MAPPED | TEE_SHM_DMA_BUF);
+ u32 flags = TEE_SHM_MAPPED | TEE_SHM_REGISTER;
+ struct tee_shm *shm;
+
+ shm = shm_alloc_helper(ctx, size, PAGE_SIZE, flags);
+ if (IS_ERR(shm))
+ return shm;
+
+ teedev_ctx_get(ctx);
+ return shm;
}
EXPORT_SYMBOL_GPL(tee_shm_alloc_kernel_buf);
+/**
+ * tee_shm_alloc_anon_kernel_buf() - Allocate shared memory for anonymous
+ * kernel buffer
+ * @ctx: Context that allocates the shared memory
+ * @size: Requested size of shared memory
+ *
+ * This function returns similar shared memory as tee_shm_alloc_kernel_buf(),
+ * but with two differences:
+ * 1. The memory might not be registered in secure world
+ * in case the driver supports passing memory not registered in advance.
+ * 2. The memory is not directly associated with the passed tee_context,
+ * rather the tee_device used by the context.
+ *
+ * This function should normally only be used internally in the TEE
+ * drivers. The memory must later only be freed using
+ * tee_shm_free_anon_kernel_buf() with a tee_contex with the same internal
+ * tee_device as when the memory was allocated.
+ *
+ * This allows allocating the shared memory using one context which is
+ * destroyed while the memory continues to live and finally freed using
+ * another context.
+ *
+ * @returns a pointer to 'struct tee_shm'
+ */
+struct tee_shm *tee_shm_alloc_anon_kernel_buf(struct tee_context *ctx,
+ size_t size)
+{
+ struct tee_shm *shm;
+
+ shm = shm_alloc_helper(ctx, size, sizeof(long) * 2, TEE_SHM_MAPPED);
+ if (IS_ERR(shm))
+ return shm;
+
+ shm->ctx = NULL;
+ return shm;
+}
+EXPORT_SYMBOL_GPL(tee_shm_alloc_anon_kernel_buf);
+
+/**
+ * tee_shm_free_anon_kernel_buf() - Free anonymous shared kernel memory
+ * @ctx: Borrowed context when freeing the shared memory
+ * @shm: Handle to shared memory to free
+ *
+ * This function must only be used to free a tee_shm allocated with
+ * tee_shm_alloc_anon_kernel_buf(). The passed @ctx has to have the same
+ * internal tee_device as was used by the tee_context passed when the
+ * memory was allocated.
+ */
+void tee_shm_free_anon_kernel_buf(struct tee_context *ctx, struct tee_shm *shm)
+{
+ struct tee_device *teedev = ctx->teedev;
+
+ /*
+ * The anonymous kernel buffer isn't attached to any tee_context
+ * we're instead assigning the current tee_context temporarily.
+ * This is needed because an eventual call to unregister the shared
+ * memory might need a context. As long as this context uses the
+ * same tee_device as in the ctx in the call in
+ * tee_shm_alloc_anon_kernel_buf() above we are OK.
+ */
+ shm->ctx = ctx;
+ teedev->pool->ops->free(teedev->pool, shm);
+ kfree(shm);
+ tee_device_put(teedev);
+}
+EXPORT_SYMBOL_GPL(tee_shm_free_anon_kernel_buf);
+
struct tee_shm *tee_shm_register(struct tee_context *ctx, unsigned long addr,
size_t length, u32 flags)
{
diff --git a/include/linux/tee_drv.h b/include/linux/tee_drv.h
index 58b319766f8e..11a4e556bdf9 100644
--- a/include/linux/tee_drv.h
+++ b/include/linux/tee_drv.h
@@ -267,22 +267,11 @@ static inline void tee_shm_pool_free(struct tee_shm_pool *pool)
*/
void *tee_get_drvdata(struct tee_device *teedev);
-/**
- * tee_shm_alloc() - Allocate shared memory
- * @ctx: Context that allocates the shared memory
- * @size: Requested size of shared memory
- * @flags: Flags setting properties for the requested shared memory.
- *
- * Memory allocated as global shared memory is automatically freed when the
- * TEE file pointer is closed. The @flags field uses the bits defined by
- * TEE_SHM_* above. TEE_SHM_MAPPED must currently always be set. If
- * TEE_SHM_DMA_BUF global shared memory will be allocated and associated
- * with a dma-buf handle, else driver private memory.
- *
- * @returns a pointer to 'struct tee_shm'
- */
-struct tee_shm *tee_shm_alloc(struct tee_context *ctx, size_t size, u32 flags);
+struct tee_shm *tee_shm_alloc_user_buf(struct tee_context *ctx, size_t size);
struct tee_shm *tee_shm_alloc_kernel_buf(struct tee_context *ctx, size_t size);
+struct tee_shm *tee_shm_alloc_anon_kernel_buf(struct tee_context *ctx,
+ size_t size);
+void tee_shm_free_anon_kernel_buf(struct tee_context *ctx, struct tee_shm *shm);
/**
* tee_shm_register() - Register shared memory buffer
--
2.31.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH 7/7] tee: replace tee_shm_alloc()
2021-06-09 10:23 ` [PATCH 7/7] tee: replace tee_shm_alloc() Jens Wiklander
@ 2021-06-09 15:32 ` Tyler Hicks
2021-06-09 15:38 ` Tyler Hicks
2021-06-11 7:42 ` Jens Wiklander
0 siblings, 2 replies; 19+ messages in thread
From: Tyler Hicks @ 2021-06-09 15:32 UTC (permalink / raw)
To: Jens Wiklander
Cc: linux-kernel, linux-arm-kernel, op-tee, Sumit Garg, Herbert Xu,
Sakkinen, Sasha Levin, Thirupathaiah Annapureddy, Vikas Gupta,
David S . Miller
On 2021-06-09 12:23:24, Jens Wiklander wrote:
> tee_shm_alloc() is replaced by three new functions,
>
> tee_shm_alloc_user_buf() - for user mode allocations, replacing passing
> the flags TEE_SHM_MAPPED | TEE_SHM_DMA_BUF
>
> tee_shm_alloc_kernel_buf() - for kernel mode allocations, slightly
> optimized compared to using the flags TEE_SHM_MAPPED | TEE_SHM_DMA_BUF
> since we now can avoid using the dma-buf registration.
>
> tee_shm_alloc_anon_kernel_buf() - for TEE driver internal use only
> allowing decoupling a shared memory object from its original
> tee_context.
>
> This also makes the interface easier to use as we can get rid of the
> somewhat hard to use flags parameter.
>
> The TEE subsystem and the TEE drivers are updated to use the new
> functions instead.
>
> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> ---
> drivers/tee/optee/call.c | 16 ++--
> drivers/tee/optee/core.c | 4 +-
> drivers/tee/optee/device.c | 5 +-
> drivers/tee/optee/rpc.c | 8 +-
> drivers/tee/tee_core.c | 2 +-
> drivers/tee/tee_shm.c | 186 +++++++++++++++++++++++++++----------
> include/linux/tee_drv.h | 19 +---
> 7 files changed, 156 insertions(+), 84 deletions(-)
>
> diff --git a/drivers/tee/optee/call.c b/drivers/tee/optee/call.c
> index 6132cc8d014c..f31257649c0e 100644
> --- a/drivers/tee/optee/call.c
> +++ b/drivers/tee/optee/call.c
> @@ -183,8 +183,8 @@ static struct tee_shm *get_msg_arg(struct tee_context *ctx, size_t num_params,
> struct tee_shm *shm;
> struct optee_msg_arg *ma;
>
> - shm = tee_shm_alloc(ctx, OPTEE_MSG_GET_ARG_SIZE(num_params),
> - TEE_SHM_MAPPED);
> + shm = tee_shm_alloc_anon_kernel_buf(ctx,
> + OPTEE_MSG_GET_ARG_SIZE(num_params));
The error handling in get_msg_arg() should be updated to call
tee_shm_free_anon_kernel_buf() instead of tee_shm_free().
> if (IS_ERR(shm))
> return shm;
>
> @@ -281,7 +281,7 @@ int optee_open_session(struct tee_context *ctx,
> arg->ret_origin = msg_arg->ret_origin;
> }
> out:
> - tee_shm_free(shm);
> + tee_shm_free_anon_kernel_buf(ctx, shm);
>
> return rc;
> }
> @@ -312,7 +312,7 @@ int optee_close_session(struct tee_context *ctx, u32 session)
> msg_arg->session = session;
> optee_do_call_with_arg(ctx, msg_parg);
>
> - tee_shm_free(shm);
> + tee_shm_free_anon_kernel_buf(ctx, shm);
> return 0;
> }
>
> @@ -358,7 +358,7 @@ int optee_invoke_func(struct tee_context *ctx, struct tee_ioctl_invoke_arg *arg,
> arg->ret = msg_arg->ret;
> arg->ret_origin = msg_arg->ret_origin;
> out:
> - tee_shm_free(shm);
> + tee_shm_free_anon_kernel_buf(ctx, shm);
> return rc;
> }
>
> @@ -386,7 +386,7 @@ int optee_cancel_req(struct tee_context *ctx, u32 cancel_id, u32 session)
> msg_arg->cancel_id = cancel_id;
> optee_do_call_with_arg(ctx, msg_parg);
>
> - tee_shm_free(shm);
> + tee_shm_free_anon_kernel_buf(ctx, shm);
> return 0;
> }
>
> @@ -625,7 +625,7 @@ int optee_shm_register(struct tee_context *ctx, struct tee_shm *shm,
> msg_arg->ret != TEEC_SUCCESS)
> rc = -EINVAL;
>
> - tee_shm_free(shm_arg);
> + tee_shm_free_anon_kernel_buf(ctx, shm_arg);
> out:
> optee_free_pages_list(pages_list, num_pages);
> return rc;
> @@ -650,7 +650,7 @@ int optee_shm_unregister(struct tee_context *ctx, struct tee_shm *shm)
> if (optee_do_call_with_arg(ctx, msg_parg) ||
> msg_arg->ret != TEEC_SUCCESS)
> rc = -EINVAL;
> - tee_shm_free(shm_arg);
> + tee_shm_free_anon_kernel_buf(ctx, shm_arg);
> return rc;
> }
>
> diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c
> index 0c287345f9fe..a15dc3881636 100644
> --- a/drivers/tee/optee/core.c
> +++ b/drivers/tee/optee/core.c
> @@ -277,7 +277,7 @@ static void optee_release(struct tee_context *ctx)
> if (!ctxdata)
> return;
>
> - shm = tee_shm_alloc(ctx, sizeof(struct optee_msg_arg), TEE_SHM_MAPPED);
> + shm = tee_shm_alloc_anon_kernel_buf(ctx, sizeof(struct optee_msg_arg));
> if (!IS_ERR(shm)) {
> arg = tee_shm_get_va(shm, 0);
> /*
> @@ -305,7 +305,7 @@ static void optee_release(struct tee_context *ctx)
> kfree(ctxdata);
>
> if (!IS_ERR(shm))
> - tee_shm_free(shm);
> + tee_shm_free_anon_kernel_buf(ctx, shm);
>
> ctx->data = NULL;
>
> diff --git a/drivers/tee/optee/device.c b/drivers/tee/optee/device.c
> index ec1d24693eba..5a5bf86b1b95 100644
> --- a/drivers/tee/optee/device.c
> +++ b/drivers/tee/optee/device.c
> @@ -113,10 +113,9 @@ static int __optee_enumerate_devices(u32 func)
> if (rc < 0 || !shm_size)
> goto out_sess;
>
> - device_shm = tee_shm_alloc(ctx, shm_size,
> - TEE_SHM_MAPPED | TEE_SHM_DMA_BUF);
> + device_shm = tee_shm_alloc_kernel_buf(ctx, shm_size);
The error handling in the 'err' label needs to use
tee_shm_free_anon_kernel_buf() instead of tee_shm_free().
> if (IS_ERR(device_shm)) {
> - pr_err("tee_shm_alloc failed\n");
> + pr_err("tee_shm_alloc_kernel_buf failed\n");
> rc = PTR_ERR(device_shm);
> goto out_sess;
> }
> diff --git a/drivers/tee/optee/rpc.c b/drivers/tee/optee/rpc.c
> index 1849180b0278..9108aedb3eee 100644
> --- a/drivers/tee/optee/rpc.c
> +++ b/drivers/tee/optee/rpc.c
> @@ -314,7 +314,7 @@ static void handle_rpc_func_cmd_shm_alloc(struct tee_context *ctx,
> shm = cmd_alloc_suppl(ctx, sz);
> break;
> case OPTEE_RPC_SHM_TYPE_KERNEL:
> - shm = tee_shm_alloc(ctx, sz, TEE_SHM_MAPPED);
> + shm = tee_shm_alloc_anon_kernel_buf(ctx, sz);
The error handling in the 'bad' label still uses tee_shm_free(). I guess
it needs to do something like handle_rpc_func_cmd_shm_free()?
> break;
> default:
> arg->ret = TEEC_ERROR_BAD_PARAMETERS;
> @@ -424,7 +424,7 @@ static void handle_rpc_func_cmd_shm_free(struct tee_context *ctx,
> cmd_free_suppl(ctx, shm);
> break;
> case OPTEE_RPC_SHM_TYPE_KERNEL:
> - tee_shm_free(shm);
> + tee_shm_free_anon_kernel_buf(ctx, shm);
> break;
> default:
> arg->ret = TEEC_ERROR_BAD_PARAMETERS;
> @@ -502,7 +502,7 @@ void optee_handle_rpc(struct tee_context *ctx, struct optee_rpc_param *param,
>
> switch (OPTEE_SMC_RETURN_GET_RPC_FUNC(param->a0)) {
> case OPTEE_SMC_RPC_FUNC_ALLOC:
> - shm = tee_shm_alloc(ctx, param->a1, TEE_SHM_MAPPED);
> + shm = tee_shm_alloc_anon_kernel_buf(ctx, param->a1);
> if (!IS_ERR(shm) && !tee_shm_get_pa(shm, 0, &pa)) {
> reg_pair_from_64(¶m->a1, ¶m->a2, pa);
> reg_pair_from_64(¶m->a4, ¶m->a5,
> @@ -516,7 +516,7 @@ void optee_handle_rpc(struct tee_context *ctx, struct optee_rpc_param *param,
> break;
> case OPTEE_SMC_RPC_FUNC_FREE:
> shm = reg_pair_to_ptr(param->a1, param->a2);
> - tee_shm_free(shm);
> + tee_shm_free_anon_kernel_buf(ctx, shm);
> break;
> case OPTEE_SMC_RPC_FUNC_FOREIGN_INTR:
> /*
> diff --git a/drivers/tee/tee_core.c b/drivers/tee/tee_core.c
> index 480d294a23ab..4f5c7c17a434 100644
> --- a/drivers/tee/tee_core.c
> +++ b/drivers/tee/tee_core.c
> @@ -293,7 +293,7 @@ static int tee_ioctl_shm_alloc(struct tee_context *ctx,
> if (data.flags)
> return -EINVAL;
>
> - shm = tee_shm_alloc(ctx, data.size, TEE_SHM_MAPPED | TEE_SHM_DMA_BUF);
> + shm = tee_shm_alloc_user_buf(ctx, data.size);
> if (IS_ERR(shm))
> return PTR_ERR(shm);
>
> diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c
> index 63fce8d39d8b..d134e2778a3a 100644
> --- a/drivers/tee/tee_shm.c
> +++ b/drivers/tee/tee_shm.c
> @@ -96,25 +96,14 @@ static const struct dma_buf_ops tee_shm_dma_buf_ops = {
> .mmap = tee_shm_op_mmap,
> };
>
> -struct tee_shm *tee_shm_alloc(struct tee_context *ctx, size_t size, u32 flags)
> +static struct tee_shm *shm_alloc_helper(struct tee_context *ctx, size_t size,
> + size_t align, u32 flags)
> {
> struct tee_device *teedev = ctx->teedev;
> struct tee_shm *shm;
> - size_t align;
> void *ret;
> int rc;
>
> - if (!(flags & TEE_SHM_MAPPED)) {
> - dev_err(teedev->dev.parent,
> - "only mapped allocations supported\n");
> - return ERR_PTR(-EINVAL);
> - }
> -
> - if ((flags & ~(TEE_SHM_MAPPED | TEE_SHM_DMA_BUF))) {
> - dev_err(teedev->dev.parent, "invalid shm flags 0x%x", flags);
> - return ERR_PTR(-EINVAL);
> - }
> -
> if (!tee_device_get(teedev))
> return ERR_PTR(-EINVAL);
>
> @@ -131,17 +120,14 @@ struct tee_shm *tee_shm_alloc(struct tee_context *ctx, size_t size, u32 flags)
> }
>
> shm->flags = flags | TEE_SHM_POOL;
> +
> + /*
> + * We're assigning this as it is needed if the shm is to be
> + * registered. If this function returns OK then the caller expected
> + * to call teedev_ctx_get() or clear shm->ctx in case it's not
> + * needed any longer.
> + */
> shm->ctx = ctx;
> - if (flags & TEE_SHM_DMA_BUF) {
> - align = PAGE_SIZE;
> - /*
> - * Request to register the shm in the pool allocator below
> - * if supported.
> - */
> - shm->flags |= TEE_SHM_REGISTER;
> - } else {
> - align = 2 * sizeof(long);
> - }
>
> rc = teedev->pool->ops->alloc(teedev->pool, shm, size, align);
> if (rc) {
> @@ -149,48 +135,71 @@ struct tee_shm *tee_shm_alloc(struct tee_context *ctx, size_t size, u32 flags)
> goto err_kfree;
> }
>
> + return shm;
> +err_kfree:
> + kfree(shm);
> +err_dev_put:
> + tee_device_put(teedev);
> + return ret;
> +}
>
> - if (flags & TEE_SHM_DMA_BUF) {
> - DEFINE_DMA_BUF_EXPORT_INFO(exp_info);
> +/**
> + * tee_shm_alloc_user_buf() - Allocate shared memory for user space
> + * @ctx: Context that allocates the shared memory
> + * @size: Requested size of shared memory
> + *
> + * Memory allocated as user space shared memory is automatically freed when
> + * the TEE file pointer is closed. The primary usage of this function is
> + * when the TEE driver doesn't support registering ordinary user space
> + * memory.
> + *
> + * @returns a pointer to 'struct tee_shm'
> + */
> +struct tee_shm *tee_shm_alloc_user_buf(struct tee_context *ctx, size_t size)
> +{
> + u32 flags = TEE_SHM_MAPPED | TEE_SHM_DMA_BUF | TEE_SHM_REGISTER;
Why not TEE_SHM_USER_MAPPED instead of TEE_SHM_MAPPED here?
> + struct tee_device *teedev = ctx->teedev;
> + DEFINE_DMA_BUF_EXPORT_INFO(exp_info);
> + struct tee_shm *shm;
> + void *ret;
>
> - mutex_lock(&teedev->mutex);
> - shm->id = idr_alloc(&teedev->idr, shm, 1, 0, GFP_KERNEL);
> - mutex_unlock(&teedev->mutex);
> - if (shm->id < 0) {
> - ret = ERR_PTR(shm->id);
> - goto err_pool_free;
> - }
> + shm = shm_alloc_helper(ctx, size, PAGE_SIZE, flags);
> + if (IS_ERR(shm))
> + return shm;
>
> - exp_info.ops = &tee_shm_dma_buf_ops;
> - exp_info.size = shm->size;
> - exp_info.flags = O_RDWR;
> - exp_info.priv = shm;
> + mutex_lock(&teedev->mutex);
> + shm->id = idr_alloc(&teedev->idr, shm, 1, 0, GFP_KERNEL);
> + mutex_unlock(&teedev->mutex);
> + if (shm->id < 0) {
> + ret = ERR_PTR(shm->id);
> + goto err_pool_free;
> + }
>
> - shm->dmabuf = dma_buf_export(&exp_info);
> - if (IS_ERR(shm->dmabuf)) {
> - ret = ERR_CAST(shm->dmabuf);
> - goto err_rem;
> - }
> + exp_info.ops = &tee_shm_dma_buf_ops;
> + exp_info.size = shm->size;
> + exp_info.flags = O_RDWR;
> + exp_info.priv = shm;
> +
> + shm->dmabuf = dma_buf_export(&exp_info);
> + if (IS_ERR(shm->dmabuf)) {
> + ret = ERR_CAST(shm->dmabuf);
> + goto err_rem;
> }
>
> teedev_ctx_get(ctx);
> -
> return shm;
> err_rem:
> - if (flags & TEE_SHM_DMA_BUF) {
> - mutex_lock(&teedev->mutex);
> - idr_remove(&teedev->idr, shm->id);
> - mutex_unlock(&teedev->mutex);
> - }
> + mutex_lock(&teedev->mutex);
> + idr_remove(&teedev->idr, shm->id);
> + mutex_unlock(&teedev->mutex);
> err_pool_free:
> teedev->pool->ops->free(teedev->pool, shm);
> -err_kfree:
> kfree(shm);
> -err_dev_put:
> tee_device_put(teedev);
> return ret;
> +
> }
> -EXPORT_SYMBOL_GPL(tee_shm_alloc);
> +EXPORT_SYMBOL_GPL(tee_shm_alloc_user_buf);
>
> /**
> * tee_shm_alloc_kernel_buf() - Allocate shared memory for kernel buffer
> @@ -206,10 +215,85 @@ EXPORT_SYMBOL_GPL(tee_shm_alloc);
> */
> struct tee_shm *tee_shm_alloc_kernel_buf(struct tee_context *ctx, size_t size)
> {
> - return tee_shm_alloc(ctx, size, TEE_SHM_MAPPED | TEE_SHM_DMA_BUF);
> + u32 flags = TEE_SHM_MAPPED | TEE_SHM_REGISTER;
Similar question as above... why not TEE_SHM_KERNEL_MAPPED instead of
TEE_SHM_MAPPED?
> + struct tee_shm *shm;
> +
> + shm = shm_alloc_helper(ctx, size, PAGE_SIZE, flags);
> + if (IS_ERR(shm))
> + return shm;
> +
> + teedev_ctx_get(ctx);
> + return shm;
> }
> EXPORT_SYMBOL_GPL(tee_shm_alloc_kernel_buf);
>
> +/**
> + * tee_shm_alloc_anon_kernel_buf() - Allocate shared memory for anonymous
> + * kernel buffer
> + * @ctx: Context that allocates the shared memory
> + * @size: Requested size of shared memory
> + *
> + * This function returns similar shared memory as tee_shm_alloc_kernel_buf(),
> + * but with two differences:
> + * 1. The memory might not be registered in secure world
> + * in case the driver supports passing memory not registered in advance.
> + * 2. The memory is not directly associated with the passed tee_context,
> + * rather the tee_device used by the context.
> + *
> + * This function should normally only be used internally in the TEE
> + * drivers. The memory must later only be freed using
> + * tee_shm_free_anon_kernel_buf() with a tee_contex with the same internal
> + * tee_device as when the memory was allocated.
> + *
> + * This allows allocating the shared memory using one context which is
> + * destroyed while the memory continues to live and finally freed using
> + * another context.
> + *
> + * @returns a pointer to 'struct tee_shm'
> + */
> +struct tee_shm *tee_shm_alloc_anon_kernel_buf(struct tee_context *ctx,
> + size_t size)
> +{
> + struct tee_shm *shm;
> +
> + shm = shm_alloc_helper(ctx, size, sizeof(long) * 2, TEE_SHM_MAPPED);
The generic TEE_SHM_MAPPED is used here, as well.
Tyler
> + if (IS_ERR(shm))
> + return shm;
> +
> + shm->ctx = NULL;
> + return shm;
> +}
> +EXPORT_SYMBOL_GPL(tee_shm_alloc_anon_kernel_buf);
> +
> +/**
> + * tee_shm_free_anon_kernel_buf() - Free anonymous shared kernel memory
> + * @ctx: Borrowed context when freeing the shared memory
> + * @shm: Handle to shared memory to free
> + *
> + * This function must only be used to free a tee_shm allocated with
> + * tee_shm_alloc_anon_kernel_buf(). The passed @ctx has to have the same
> + * internal tee_device as was used by the tee_context passed when the
> + * memory was allocated.
> + */
> +void tee_shm_free_anon_kernel_buf(struct tee_context *ctx, struct tee_shm *shm)
> +{
> + struct tee_device *teedev = ctx->teedev;
> +
> + /*
> + * The anonymous kernel buffer isn't attached to any tee_context
> + * we're instead assigning the current tee_context temporarily.
> + * This is needed because an eventual call to unregister the shared
> + * memory might need a context. As long as this context uses the
> + * same tee_device as in the ctx in the call in
> + * tee_shm_alloc_anon_kernel_buf() above we are OK.
> + */
> + shm->ctx = ctx;
> + teedev->pool->ops->free(teedev->pool, shm);
> + kfree(shm);
> + tee_device_put(teedev);
> +}
> +EXPORT_SYMBOL_GPL(tee_shm_free_anon_kernel_buf);
> +
> struct tee_shm *tee_shm_register(struct tee_context *ctx, unsigned long addr,
> size_t length, u32 flags)
> {
> diff --git a/include/linux/tee_drv.h b/include/linux/tee_drv.h
> index 58b319766f8e..11a4e556bdf9 100644
> --- a/include/linux/tee_drv.h
> +++ b/include/linux/tee_drv.h
> @@ -267,22 +267,11 @@ static inline void tee_shm_pool_free(struct tee_shm_pool *pool)
> */
> void *tee_get_drvdata(struct tee_device *teedev);
>
> -/**
> - * tee_shm_alloc() - Allocate shared memory
> - * @ctx: Context that allocates the shared memory
> - * @size: Requested size of shared memory
> - * @flags: Flags setting properties for the requested shared memory.
> - *
> - * Memory allocated as global shared memory is automatically freed when the
> - * TEE file pointer is closed. The @flags field uses the bits defined by
> - * TEE_SHM_* above. TEE_SHM_MAPPED must currently always be set. If
> - * TEE_SHM_DMA_BUF global shared memory will be allocated and associated
> - * with a dma-buf handle, else driver private memory.
> - *
> - * @returns a pointer to 'struct tee_shm'
> - */
> -struct tee_shm *tee_shm_alloc(struct tee_context *ctx, size_t size, u32 flags);
> +struct tee_shm *tee_shm_alloc_user_buf(struct tee_context *ctx, size_t size);
> struct tee_shm *tee_shm_alloc_kernel_buf(struct tee_context *ctx, size_t size);
> +struct tee_shm *tee_shm_alloc_anon_kernel_buf(struct tee_context *ctx,
> + size_t size);
> +void tee_shm_free_anon_kernel_buf(struct tee_context *ctx, struct tee_shm *shm);
>
> /**
> * tee_shm_register() - Register shared memory buffer
> --
> 2.31.1
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 7/7] tee: replace tee_shm_alloc()
2021-06-09 15:32 ` Tyler Hicks
@ 2021-06-09 15:38 ` Tyler Hicks
2021-06-11 7:42 ` Jens Wiklander
1 sibling, 0 replies; 19+ messages in thread
From: Tyler Hicks @ 2021-06-09 15:38 UTC (permalink / raw)
To: Jens Wiklander
Cc: linux-kernel, linux-arm-kernel, op-tee, Sumit Garg, Herbert Xu,
Sakkinen, Sasha Levin, Thirupathaiah Annapureddy, Vikas Gupta,
David S . Miller
On 2021-06-09 10:32:18, Tyler Hicks wrote:
> On 2021-06-09 12:23:24, Jens Wiklander wrote:
> > tee_shm_alloc() is replaced by three new functions,
> >
> > tee_shm_alloc_user_buf() - for user mode allocations, replacing passing
> > the flags TEE_SHM_MAPPED | TEE_SHM_DMA_BUF
> >
> > tee_shm_alloc_kernel_buf() - for kernel mode allocations, slightly
> > optimized compared to using the flags TEE_SHM_MAPPED | TEE_SHM_DMA_BUF
> > since we now can avoid using the dma-buf registration.
> >
> > tee_shm_alloc_anon_kernel_buf() - for TEE driver internal use only
> > allowing decoupling a shared memory object from its original
> > tee_context.
> >
> > This also makes the interface easier to use as we can get rid of the
> > somewhat hard to use flags parameter.
> >
> > The TEE subsystem and the TEE drivers are updated to use the new
> > functions instead.
> >
> > Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> > ---
> > drivers/tee/optee/call.c | 16 ++--
> > drivers/tee/optee/core.c | 4 +-
> > drivers/tee/optee/device.c | 5 +-
> > drivers/tee/optee/rpc.c | 8 +-
> > drivers/tee/tee_core.c | 2 +-
> > drivers/tee/tee_shm.c | 186 +++++++++++++++++++++++++++----------
> > include/linux/tee_drv.h | 19 +---
> > 7 files changed, 156 insertions(+), 84 deletions(-)
> >
> > diff --git a/drivers/tee/optee/call.c b/drivers/tee/optee/call.c
> > index 6132cc8d014c..f31257649c0e 100644
> > --- a/drivers/tee/optee/call.c
> > +++ b/drivers/tee/optee/call.c
> > @@ -183,8 +183,8 @@ static struct tee_shm *get_msg_arg(struct tee_context *ctx, size_t num_params,
> > struct tee_shm *shm;
> > struct optee_msg_arg *ma;
> >
> > - shm = tee_shm_alloc(ctx, OPTEE_MSG_GET_ARG_SIZE(num_params),
> > - TEE_SHM_MAPPED);
> > + shm = tee_shm_alloc_anon_kernel_buf(ctx,
> > + OPTEE_MSG_GET_ARG_SIZE(num_params));
>
> The error handling in get_msg_arg() should be updated to call
> tee_shm_free_anon_kernel_buf() instead of tee_shm_free().
>
> > if (IS_ERR(shm))
> > return shm;
> >
> > @@ -281,7 +281,7 @@ int optee_open_session(struct tee_context *ctx,
> > arg->ret_origin = msg_arg->ret_origin;
> > }
> > out:
> > - tee_shm_free(shm);
> > + tee_shm_free_anon_kernel_buf(ctx, shm);
> >
> > return rc;
> > }
> > @@ -312,7 +312,7 @@ int optee_close_session(struct tee_context *ctx, u32 session)
> > msg_arg->session = session;
> > optee_do_call_with_arg(ctx, msg_parg);
> >
> > - tee_shm_free(shm);
> > + tee_shm_free_anon_kernel_buf(ctx, shm);
> > return 0;
> > }
> >
> > @@ -358,7 +358,7 @@ int optee_invoke_func(struct tee_context *ctx, struct tee_ioctl_invoke_arg *arg,
> > arg->ret = msg_arg->ret;
> > arg->ret_origin = msg_arg->ret_origin;
> > out:
> > - tee_shm_free(shm);
> > + tee_shm_free_anon_kernel_buf(ctx, shm);
> > return rc;
> > }
> >
> > @@ -386,7 +386,7 @@ int optee_cancel_req(struct tee_context *ctx, u32 cancel_id, u32 session)
> > msg_arg->cancel_id = cancel_id;
> > optee_do_call_with_arg(ctx, msg_parg);
> >
> > - tee_shm_free(shm);
> > + tee_shm_free_anon_kernel_buf(ctx, shm);
> > return 0;
> > }
> >
> > @@ -625,7 +625,7 @@ int optee_shm_register(struct tee_context *ctx, struct tee_shm *shm,
> > msg_arg->ret != TEEC_SUCCESS)
> > rc = -EINVAL;
> >
> > - tee_shm_free(shm_arg);
> > + tee_shm_free_anon_kernel_buf(ctx, shm_arg);
> > out:
> > optee_free_pages_list(pages_list, num_pages);
> > return rc;
> > @@ -650,7 +650,7 @@ int optee_shm_unregister(struct tee_context *ctx, struct tee_shm *shm)
> > if (optee_do_call_with_arg(ctx, msg_parg) ||
> > msg_arg->ret != TEEC_SUCCESS)
> > rc = -EINVAL;
> > - tee_shm_free(shm_arg);
> > + tee_shm_free_anon_kernel_buf(ctx, shm_arg);
> > return rc;
> > }
> >
> > diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c
> > index 0c287345f9fe..a15dc3881636 100644
> > --- a/drivers/tee/optee/core.c
> > +++ b/drivers/tee/optee/core.c
> > @@ -277,7 +277,7 @@ static void optee_release(struct tee_context *ctx)
> > if (!ctxdata)
> > return;
> >
> > - shm = tee_shm_alloc(ctx, sizeof(struct optee_msg_arg), TEE_SHM_MAPPED);
> > + shm = tee_shm_alloc_anon_kernel_buf(ctx, sizeof(struct optee_msg_arg));
> > if (!IS_ERR(shm)) {
> > arg = tee_shm_get_va(shm, 0);
> > /*
> > @@ -305,7 +305,7 @@ static void optee_release(struct tee_context *ctx)
> > kfree(ctxdata);
> >
> > if (!IS_ERR(shm))
> > - tee_shm_free(shm);
> > + tee_shm_free_anon_kernel_buf(ctx, shm);
> >
> > ctx->data = NULL;
> >
> > diff --git a/drivers/tee/optee/device.c b/drivers/tee/optee/device.c
> > index ec1d24693eba..5a5bf86b1b95 100644
> > --- a/drivers/tee/optee/device.c
> > +++ b/drivers/tee/optee/device.c
> > @@ -113,10 +113,9 @@ static int __optee_enumerate_devices(u32 func)
> > if (rc < 0 || !shm_size)
> > goto out_sess;
> >
> > - device_shm = tee_shm_alloc(ctx, shm_size,
> > - TEE_SHM_MAPPED | TEE_SHM_DMA_BUF);
> > + device_shm = tee_shm_alloc_kernel_buf(ctx, shm_size);
>
> The error handling in the 'err' label needs to use
> tee_shm_free_anon_kernel_buf() instead of tee_shm_free().
Sorry, I got confused here. The error handling in this function should
continue to use tee_shm_free().
To avoid this confusion in the future, perhaps the following macro would
be a nice touch in include/linux/tee_drv.h?
#define tee_shm_free_kernel_buf tee_shm_free
If so, then all of the new users of tee_shm_alloc_kernel_buf() should be
updated to use tee_shm_free_kernel_buf().
Tyler
>
> > if (IS_ERR(device_shm)) {
> > - pr_err("tee_shm_alloc failed\n");
> > + pr_err("tee_shm_alloc_kernel_buf failed\n");
> > rc = PTR_ERR(device_shm);
> > goto out_sess;
> > }
> > diff --git a/drivers/tee/optee/rpc.c b/drivers/tee/optee/rpc.c
> > index 1849180b0278..9108aedb3eee 100644
> > --- a/drivers/tee/optee/rpc.c
> > +++ b/drivers/tee/optee/rpc.c
> > @@ -314,7 +314,7 @@ static void handle_rpc_func_cmd_shm_alloc(struct tee_context *ctx,
> > shm = cmd_alloc_suppl(ctx, sz);
> > break;
> > case OPTEE_RPC_SHM_TYPE_KERNEL:
> > - shm = tee_shm_alloc(ctx, sz, TEE_SHM_MAPPED);
> > + shm = tee_shm_alloc_anon_kernel_buf(ctx, sz);
>
> The error handling in the 'bad' label still uses tee_shm_free(). I guess
> it needs to do something like handle_rpc_func_cmd_shm_free()?
>
> > break;
> > default:
> > arg->ret = TEEC_ERROR_BAD_PARAMETERS;
> > @@ -424,7 +424,7 @@ static void handle_rpc_func_cmd_shm_free(struct tee_context *ctx,
> > cmd_free_suppl(ctx, shm);
> > break;
> > case OPTEE_RPC_SHM_TYPE_KERNEL:
> > - tee_shm_free(shm);
> > + tee_shm_free_anon_kernel_buf(ctx, shm);
> > break;
> > default:
> > arg->ret = TEEC_ERROR_BAD_PARAMETERS;
> > @@ -502,7 +502,7 @@ void optee_handle_rpc(struct tee_context *ctx, struct optee_rpc_param *param,
> >
> > switch (OPTEE_SMC_RETURN_GET_RPC_FUNC(param->a0)) {
> > case OPTEE_SMC_RPC_FUNC_ALLOC:
> > - shm = tee_shm_alloc(ctx, param->a1, TEE_SHM_MAPPED);
> > + shm = tee_shm_alloc_anon_kernel_buf(ctx, param->a1);
> > if (!IS_ERR(shm) && !tee_shm_get_pa(shm, 0, &pa)) {
> > reg_pair_from_64(¶m->a1, ¶m->a2, pa);
> > reg_pair_from_64(¶m->a4, ¶m->a5,
> > @@ -516,7 +516,7 @@ void optee_handle_rpc(struct tee_context *ctx, struct optee_rpc_param *param,
> > break;
> > case OPTEE_SMC_RPC_FUNC_FREE:
> > shm = reg_pair_to_ptr(param->a1, param->a2);
> > - tee_shm_free(shm);
> > + tee_shm_free_anon_kernel_buf(ctx, shm);
> > break;
> > case OPTEE_SMC_RPC_FUNC_FOREIGN_INTR:
> > /*
> > diff --git a/drivers/tee/tee_core.c b/drivers/tee/tee_core.c
> > index 480d294a23ab..4f5c7c17a434 100644
> > --- a/drivers/tee/tee_core.c
> > +++ b/drivers/tee/tee_core.c
> > @@ -293,7 +293,7 @@ static int tee_ioctl_shm_alloc(struct tee_context *ctx,
> > if (data.flags)
> > return -EINVAL;
> >
> > - shm = tee_shm_alloc(ctx, data.size, TEE_SHM_MAPPED | TEE_SHM_DMA_BUF);
> > + shm = tee_shm_alloc_user_buf(ctx, data.size);
> > if (IS_ERR(shm))
> > return PTR_ERR(shm);
> >
> > diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c
> > index 63fce8d39d8b..d134e2778a3a 100644
> > --- a/drivers/tee/tee_shm.c
> > +++ b/drivers/tee/tee_shm.c
> > @@ -96,25 +96,14 @@ static const struct dma_buf_ops tee_shm_dma_buf_ops = {
> > .mmap = tee_shm_op_mmap,
> > };
> >
> > -struct tee_shm *tee_shm_alloc(struct tee_context *ctx, size_t size, u32 flags)
> > +static struct tee_shm *shm_alloc_helper(struct tee_context *ctx, size_t size,
> > + size_t align, u32 flags)
> > {
> > struct tee_device *teedev = ctx->teedev;
> > struct tee_shm *shm;
> > - size_t align;
> > void *ret;
> > int rc;
> >
> > - if (!(flags & TEE_SHM_MAPPED)) {
> > - dev_err(teedev->dev.parent,
> > - "only mapped allocations supported\n");
> > - return ERR_PTR(-EINVAL);
> > - }
> > -
> > - if ((flags & ~(TEE_SHM_MAPPED | TEE_SHM_DMA_BUF))) {
> > - dev_err(teedev->dev.parent, "invalid shm flags 0x%x", flags);
> > - return ERR_PTR(-EINVAL);
> > - }
> > -
> > if (!tee_device_get(teedev))
> > return ERR_PTR(-EINVAL);
> >
> > @@ -131,17 +120,14 @@ struct tee_shm *tee_shm_alloc(struct tee_context *ctx, size_t size, u32 flags)
> > }
> >
> > shm->flags = flags | TEE_SHM_POOL;
> > +
> > + /*
> > + * We're assigning this as it is needed if the shm is to be
> > + * registered. If this function returns OK then the caller expected
> > + * to call teedev_ctx_get() or clear shm->ctx in case it's not
> > + * needed any longer.
> > + */
> > shm->ctx = ctx;
> > - if (flags & TEE_SHM_DMA_BUF) {
> > - align = PAGE_SIZE;
> > - /*
> > - * Request to register the shm in the pool allocator below
> > - * if supported.
> > - */
> > - shm->flags |= TEE_SHM_REGISTER;
> > - } else {
> > - align = 2 * sizeof(long);
> > - }
> >
> > rc = teedev->pool->ops->alloc(teedev->pool, shm, size, align);
> > if (rc) {
> > @@ -149,48 +135,71 @@ struct tee_shm *tee_shm_alloc(struct tee_context *ctx, size_t size, u32 flags)
> > goto err_kfree;
> > }
> >
> > + return shm;
> > +err_kfree:
> > + kfree(shm);
> > +err_dev_put:
> > + tee_device_put(teedev);
> > + return ret;
> > +}
> >
> > - if (flags & TEE_SHM_DMA_BUF) {
> > - DEFINE_DMA_BUF_EXPORT_INFO(exp_info);
> > +/**
> > + * tee_shm_alloc_user_buf() - Allocate shared memory for user space
> > + * @ctx: Context that allocates the shared memory
> > + * @size: Requested size of shared memory
> > + *
> > + * Memory allocated as user space shared memory is automatically freed when
> > + * the TEE file pointer is closed. The primary usage of this function is
> > + * when the TEE driver doesn't support registering ordinary user space
> > + * memory.
> > + *
> > + * @returns a pointer to 'struct tee_shm'
> > + */
> > +struct tee_shm *tee_shm_alloc_user_buf(struct tee_context *ctx, size_t size)
> > +{
> > + u32 flags = TEE_SHM_MAPPED | TEE_SHM_DMA_BUF | TEE_SHM_REGISTER;
>
> Why not TEE_SHM_USER_MAPPED instead of TEE_SHM_MAPPED here?
>
> > + struct tee_device *teedev = ctx->teedev;
> > + DEFINE_DMA_BUF_EXPORT_INFO(exp_info);
> > + struct tee_shm *shm;
> > + void *ret;
> >
> > - mutex_lock(&teedev->mutex);
> > - shm->id = idr_alloc(&teedev->idr, shm, 1, 0, GFP_KERNEL);
> > - mutex_unlock(&teedev->mutex);
> > - if (shm->id < 0) {
> > - ret = ERR_PTR(shm->id);
> > - goto err_pool_free;
> > - }
> > + shm = shm_alloc_helper(ctx, size, PAGE_SIZE, flags);
> > + if (IS_ERR(shm))
> > + return shm;
> >
> > - exp_info.ops = &tee_shm_dma_buf_ops;
> > - exp_info.size = shm->size;
> > - exp_info.flags = O_RDWR;
> > - exp_info.priv = shm;
> > + mutex_lock(&teedev->mutex);
> > + shm->id = idr_alloc(&teedev->idr, shm, 1, 0, GFP_KERNEL);
> > + mutex_unlock(&teedev->mutex);
> > + if (shm->id < 0) {
> > + ret = ERR_PTR(shm->id);
> > + goto err_pool_free;
> > + }
> >
> > - shm->dmabuf = dma_buf_export(&exp_info);
> > - if (IS_ERR(shm->dmabuf)) {
> > - ret = ERR_CAST(shm->dmabuf);
> > - goto err_rem;
> > - }
> > + exp_info.ops = &tee_shm_dma_buf_ops;
> > + exp_info.size = shm->size;
> > + exp_info.flags = O_RDWR;
> > + exp_info.priv = shm;
> > +
> > + shm->dmabuf = dma_buf_export(&exp_info);
> > + if (IS_ERR(shm->dmabuf)) {
> > + ret = ERR_CAST(shm->dmabuf);
> > + goto err_rem;
> > }
> >
> > teedev_ctx_get(ctx);
> > -
> > return shm;
> > err_rem:
> > - if (flags & TEE_SHM_DMA_BUF) {
> > - mutex_lock(&teedev->mutex);
> > - idr_remove(&teedev->idr, shm->id);
> > - mutex_unlock(&teedev->mutex);
> > - }
> > + mutex_lock(&teedev->mutex);
> > + idr_remove(&teedev->idr, shm->id);
> > + mutex_unlock(&teedev->mutex);
> > err_pool_free:
> > teedev->pool->ops->free(teedev->pool, shm);
> > -err_kfree:
> > kfree(shm);
> > -err_dev_put:
> > tee_device_put(teedev);
> > return ret;
> > +
> > }
> > -EXPORT_SYMBOL_GPL(tee_shm_alloc);
> > +EXPORT_SYMBOL_GPL(tee_shm_alloc_user_buf);
> >
> > /**
> > * tee_shm_alloc_kernel_buf() - Allocate shared memory for kernel buffer
> > @@ -206,10 +215,85 @@ EXPORT_SYMBOL_GPL(tee_shm_alloc);
> > */
> > struct tee_shm *tee_shm_alloc_kernel_buf(struct tee_context *ctx, size_t size)
> > {
> > - return tee_shm_alloc(ctx, size, TEE_SHM_MAPPED | TEE_SHM_DMA_BUF);
> > + u32 flags = TEE_SHM_MAPPED | TEE_SHM_REGISTER;
>
> Similar question as above... why not TEE_SHM_KERNEL_MAPPED instead of
> TEE_SHM_MAPPED?
>
> > + struct tee_shm *shm;
> > +
> > + shm = shm_alloc_helper(ctx, size, PAGE_SIZE, flags);
> > + if (IS_ERR(shm))
> > + return shm;
> > +
> > + teedev_ctx_get(ctx);
> > + return shm;
> > }
> > EXPORT_SYMBOL_GPL(tee_shm_alloc_kernel_buf);
> >
> > +/**
> > + * tee_shm_alloc_anon_kernel_buf() - Allocate shared memory for anonymous
> > + * kernel buffer
> > + * @ctx: Context that allocates the shared memory
> > + * @size: Requested size of shared memory
> > + *
> > + * This function returns similar shared memory as tee_shm_alloc_kernel_buf(),
> > + * but with two differences:
> > + * 1. The memory might not be registered in secure world
> > + * in case the driver supports passing memory not registered in advance.
> > + * 2. The memory is not directly associated with the passed tee_context,
> > + * rather the tee_device used by the context.
> > + *
> > + * This function should normally only be used internally in the TEE
> > + * drivers. The memory must later only be freed using
> > + * tee_shm_free_anon_kernel_buf() with a tee_contex with the same internal
> > + * tee_device as when the memory was allocated.
> > + *
> > + * This allows allocating the shared memory using one context which is
> > + * destroyed while the memory continues to live and finally freed using
> > + * another context.
> > + *
> > + * @returns a pointer to 'struct tee_shm'
> > + */
> > +struct tee_shm *tee_shm_alloc_anon_kernel_buf(struct tee_context *ctx,
> > + size_t size)
> > +{
> > + struct tee_shm *shm;
> > +
> > + shm = shm_alloc_helper(ctx, size, sizeof(long) * 2, TEE_SHM_MAPPED);
>
> The generic TEE_SHM_MAPPED is used here, as well.
>
> Tyler
>
> > + if (IS_ERR(shm))
> > + return shm;
> > +
> > + shm->ctx = NULL;
> > + return shm;
> > +}
> > +EXPORT_SYMBOL_GPL(tee_shm_alloc_anon_kernel_buf);
> > +
> > +/**
> > + * tee_shm_free_anon_kernel_buf() - Free anonymous shared kernel memory
> > + * @ctx: Borrowed context when freeing the shared memory
> > + * @shm: Handle to shared memory to free
> > + *
> > + * This function must only be used to free a tee_shm allocated with
> > + * tee_shm_alloc_anon_kernel_buf(). The passed @ctx has to have the same
> > + * internal tee_device as was used by the tee_context passed when the
> > + * memory was allocated.
> > + */
> > +void tee_shm_free_anon_kernel_buf(struct tee_context *ctx, struct tee_shm *shm)
> > +{
> > + struct tee_device *teedev = ctx->teedev;
> > +
> > + /*
> > + * The anonymous kernel buffer isn't attached to any tee_context
> > + * we're instead assigning the current tee_context temporarily.
> > + * This is needed because an eventual call to unregister the shared
> > + * memory might need a context. As long as this context uses the
> > + * same tee_device as in the ctx in the call in
> > + * tee_shm_alloc_anon_kernel_buf() above we are OK.
> > + */
> > + shm->ctx = ctx;
> > + teedev->pool->ops->free(teedev->pool, shm);
> > + kfree(shm);
> > + tee_device_put(teedev);
> > +}
> > +EXPORT_SYMBOL_GPL(tee_shm_free_anon_kernel_buf);
> > +
> > struct tee_shm *tee_shm_register(struct tee_context *ctx, unsigned long addr,
> > size_t length, u32 flags)
> > {
> > diff --git a/include/linux/tee_drv.h b/include/linux/tee_drv.h
> > index 58b319766f8e..11a4e556bdf9 100644
> > --- a/include/linux/tee_drv.h
> > +++ b/include/linux/tee_drv.h
> > @@ -267,22 +267,11 @@ static inline void tee_shm_pool_free(struct tee_shm_pool *pool)
> > */
> > void *tee_get_drvdata(struct tee_device *teedev);
> >
> > -/**
> > - * tee_shm_alloc() - Allocate shared memory
> > - * @ctx: Context that allocates the shared memory
> > - * @size: Requested size of shared memory
> > - * @flags: Flags setting properties for the requested shared memory.
> > - *
> > - * Memory allocated as global shared memory is automatically freed when the
> > - * TEE file pointer is closed. The @flags field uses the bits defined by
> > - * TEE_SHM_* above. TEE_SHM_MAPPED must currently always be set. If
> > - * TEE_SHM_DMA_BUF global shared memory will be allocated and associated
> > - * with a dma-buf handle, else driver private memory.
> > - *
> > - * @returns a pointer to 'struct tee_shm'
> > - */
> > -struct tee_shm *tee_shm_alloc(struct tee_context *ctx, size_t size, u32 flags);
> > +struct tee_shm *tee_shm_alloc_user_buf(struct tee_context *ctx, size_t size);
> > struct tee_shm *tee_shm_alloc_kernel_buf(struct tee_context *ctx, size_t size);
> > +struct tee_shm *tee_shm_alloc_anon_kernel_buf(struct tee_context *ctx,
> > + size_t size);
> > +void tee_shm_free_anon_kernel_buf(struct tee_context *ctx, struct tee_shm *shm);
> >
> > /**
> > * tee_shm_register() - Register shared memory buffer
> > --
> > 2.31.1
> >
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 7/7] tee: replace tee_shm_alloc()
2021-06-09 15:32 ` Tyler Hicks
2021-06-09 15:38 ` Tyler Hicks
@ 2021-06-11 7:42 ` Jens Wiklander
1 sibling, 0 replies; 19+ messages in thread
From: Jens Wiklander @ 2021-06-11 7:42 UTC (permalink / raw)
To: Tyler Hicks
Cc: linux-kernel, linux-arm-kernel, op-tee, Sumit Garg, Herbert Xu,
Sakkinen, Sasha Levin, Thirupathaiah Annapureddy, Vikas Gupta,
David S . Miller
On Wed, Jun 09, 2021 at 10:32:15AM -0500, Tyler Hicks wrote:
> On 2021-06-09 12:23:24, Jens Wiklander wrote:
> > tee_shm_alloc() is replaced by three new functions,
> >
> > tee_shm_alloc_user_buf() - for user mode allocations, replacing passing
> > the flags TEE_SHM_MAPPED | TEE_SHM_DMA_BUF
> >
> > tee_shm_alloc_kernel_buf() - for kernel mode allocations, slightly
> > optimized compared to using the flags TEE_SHM_MAPPED | TEE_SHM_DMA_BUF
> > since we now can avoid using the dma-buf registration.
> >
> > tee_shm_alloc_anon_kernel_buf() - for TEE driver internal use only
> > allowing decoupling a shared memory object from its original
> > tee_context.
> >
> > This also makes the interface easier to use as we can get rid of the
> > somewhat hard to use flags parameter.
> >
> > The TEE subsystem and the TEE drivers are updated to use the new
> > functions instead.
> >
> > Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> > ---
> > drivers/tee/optee/call.c | 16 ++--
> > drivers/tee/optee/core.c | 4 +-
> > drivers/tee/optee/device.c | 5 +-
> > drivers/tee/optee/rpc.c | 8 +-
> > drivers/tee/tee_core.c | 2 +-
> > drivers/tee/tee_shm.c | 186 +++++++++++++++++++++++++++----------
> > include/linux/tee_drv.h | 19 +---
> > 7 files changed, 156 insertions(+), 84 deletions(-)
> >
> > diff --git a/drivers/tee/optee/call.c b/drivers/tee/optee/call.c
> > index 6132cc8d014c..f31257649c0e 100644
> > --- a/drivers/tee/optee/call.c
> > +++ b/drivers/tee/optee/call.c
> > @@ -183,8 +183,8 @@ static struct tee_shm *get_msg_arg(struct tee_context *ctx, size_t num_params,
> > struct tee_shm *shm;
> > struct optee_msg_arg *ma;
> >
> > - shm = tee_shm_alloc(ctx, OPTEE_MSG_GET_ARG_SIZE(num_params),
> > - TEE_SHM_MAPPED);
> > + shm = tee_shm_alloc_anon_kernel_buf(ctx,
> > + OPTEE_MSG_GET_ARG_SIZE(num_params));
>
> The error handling in get_msg_arg() should be updated to call
> tee_shm_free_anon_kernel_buf() instead of tee_shm_free().
You're right, I'll fix.
>
> > if (IS_ERR(shm))
> > return shm;
> >
> > @@ -281,7 +281,7 @@ int optee_open_session(struct tee_context *ctx,
> > arg->ret_origin = msg_arg->ret_origin;
> > }
> > out:
> > - tee_shm_free(shm);
> > + tee_shm_free_anon_kernel_buf(ctx, shm);
> >
> > return rc;
> > }
> > @@ -312,7 +312,7 @@ int optee_close_session(struct tee_context *ctx, u32 session)
> > msg_arg->session = session;
> > optee_do_call_with_arg(ctx, msg_parg);
> >
> > - tee_shm_free(shm);
> > + tee_shm_free_anon_kernel_buf(ctx, shm);
> > return 0;
> > }
> >
> > @@ -358,7 +358,7 @@ int optee_invoke_func(struct tee_context *ctx, struct tee_ioctl_invoke_arg *arg,
> > arg->ret = msg_arg->ret;
> > arg->ret_origin = msg_arg->ret_origin;
> > out:
> > - tee_shm_free(shm);
> > + tee_shm_free_anon_kernel_buf(ctx, shm);
> > return rc;
> > }
> >
> > @@ -386,7 +386,7 @@ int optee_cancel_req(struct tee_context *ctx, u32 cancel_id, u32 session)
> > msg_arg->cancel_id = cancel_id;
> > optee_do_call_with_arg(ctx, msg_parg);
> >
> > - tee_shm_free(shm);
> > + tee_shm_free_anon_kernel_buf(ctx, shm);
> > return 0;
> > }
> >
> > @@ -625,7 +625,7 @@ int optee_shm_register(struct tee_context *ctx, struct tee_shm *shm,
> > msg_arg->ret != TEEC_SUCCESS)
> > rc = -EINVAL;
> >
> > - tee_shm_free(shm_arg);
> > + tee_shm_free_anon_kernel_buf(ctx, shm_arg);
> > out:
> > optee_free_pages_list(pages_list, num_pages);
> > return rc;
> > @@ -650,7 +650,7 @@ int optee_shm_unregister(struct tee_context *ctx, struct tee_shm *shm)
> > if (optee_do_call_with_arg(ctx, msg_parg) ||
> > msg_arg->ret != TEEC_SUCCESS)
> > rc = -EINVAL;
> > - tee_shm_free(shm_arg);
> > + tee_shm_free_anon_kernel_buf(ctx, shm_arg);
> > return rc;
> > }
> >
> > diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c
> > index 0c287345f9fe..a15dc3881636 100644
> > --- a/drivers/tee/optee/core.c
> > +++ b/drivers/tee/optee/core.c
> > @@ -277,7 +277,7 @@ static void optee_release(struct tee_context *ctx)
> > if (!ctxdata)
> > return;
> >
> > - shm = tee_shm_alloc(ctx, sizeof(struct optee_msg_arg), TEE_SHM_MAPPED);
> > + shm = tee_shm_alloc_anon_kernel_buf(ctx, sizeof(struct optee_msg_arg));
> > if (!IS_ERR(shm)) {
> > arg = tee_shm_get_va(shm, 0);
> > /*
> > @@ -305,7 +305,7 @@ static void optee_release(struct tee_context *ctx)
> > kfree(ctxdata);
> >
> > if (!IS_ERR(shm))
> > - tee_shm_free(shm);
> > + tee_shm_free_anon_kernel_buf(ctx, shm);
> >
> > ctx->data = NULL;
> >
> > diff --git a/drivers/tee/optee/device.c b/drivers/tee/optee/device.c
> > index ec1d24693eba..5a5bf86b1b95 100644
> > --- a/drivers/tee/optee/device.c
> > +++ b/drivers/tee/optee/device.c
> > @@ -113,10 +113,9 @@ static int __optee_enumerate_devices(u32 func)
> > if (rc < 0 || !shm_size)
> > goto out_sess;
> >
> > - device_shm = tee_shm_alloc(ctx, shm_size,
> > - TEE_SHM_MAPPED | TEE_SHM_DMA_BUF);
> > + device_shm = tee_shm_alloc_kernel_buf(ctx, shm_size);
>
> The error handling in the 'err' label needs to use
> tee_shm_free_anon_kernel_buf() instead of tee_shm_free().
I'll update with tee_shm_free_kernel_buf() as suggested in your next
mail.
>
> > if (IS_ERR(device_shm)) {
> > - pr_err("tee_shm_alloc failed\n");
> > + pr_err("tee_shm_alloc_kernel_buf failed\n");
> > rc = PTR_ERR(device_shm);
> > goto out_sess;
> > }
> > diff --git a/drivers/tee/optee/rpc.c b/drivers/tee/optee/rpc.c
> > index 1849180b0278..9108aedb3eee 100644
> > --- a/drivers/tee/optee/rpc.c
> > +++ b/drivers/tee/optee/rpc.c
> > @@ -314,7 +314,7 @@ static void handle_rpc_func_cmd_shm_alloc(struct tee_context *ctx,
> > shm = cmd_alloc_suppl(ctx, sz);
> > break;
> > case OPTEE_RPC_SHM_TYPE_KERNEL:
> > - shm = tee_shm_alloc(ctx, sz, TEE_SHM_MAPPED);
> > + shm = tee_shm_alloc_anon_kernel_buf(ctx, sz);
>
> The error handling in the 'bad' label still uses tee_shm_free(). I guess
> it needs to do something like handle_rpc_func_cmd_shm_free()?
Thanks, I'll fix.
>
> > break;
> > default:
> > arg->ret = TEEC_ERROR_BAD_PARAMETERS;
> > @@ -424,7 +424,7 @@ static void handle_rpc_func_cmd_shm_free(struct tee_context *ctx,
> > cmd_free_suppl(ctx, shm);
> > break;
> > case OPTEE_RPC_SHM_TYPE_KERNEL:
> > - tee_shm_free(shm);
> > + tee_shm_free_anon_kernel_buf(ctx, shm);
> > break;
> > default:
> > arg->ret = TEEC_ERROR_BAD_PARAMETERS;
> > @@ -502,7 +502,7 @@ void optee_handle_rpc(struct tee_context *ctx, struct optee_rpc_param *param,
> >
> > switch (OPTEE_SMC_RETURN_GET_RPC_FUNC(param->a0)) {
> > case OPTEE_SMC_RPC_FUNC_ALLOC:
> > - shm = tee_shm_alloc(ctx, param->a1, TEE_SHM_MAPPED);
> > + shm = tee_shm_alloc_anon_kernel_buf(ctx, param->a1);
> > if (!IS_ERR(shm) && !tee_shm_get_pa(shm, 0, &pa)) {
> > reg_pair_from_64(¶m->a1, ¶m->a2, pa);
> > reg_pair_from_64(¶m->a4, ¶m->a5,
> > @@ -516,7 +516,7 @@ void optee_handle_rpc(struct tee_context *ctx, struct optee_rpc_param *param,
> > break;
> > case OPTEE_SMC_RPC_FUNC_FREE:
> > shm = reg_pair_to_ptr(param->a1, param->a2);
> > - tee_shm_free(shm);
> > + tee_shm_free_anon_kernel_buf(ctx, shm);
> > break;
> > case OPTEE_SMC_RPC_FUNC_FOREIGN_INTR:
> > /*
> > diff --git a/drivers/tee/tee_core.c b/drivers/tee/tee_core.c
> > index 480d294a23ab..4f5c7c17a434 100644
> > --- a/drivers/tee/tee_core.c
> > +++ b/drivers/tee/tee_core.c
> > @@ -293,7 +293,7 @@ static int tee_ioctl_shm_alloc(struct tee_context *ctx,
> > if (data.flags)
> > return -EINVAL;
> >
> > - shm = tee_shm_alloc(ctx, data.size, TEE_SHM_MAPPED | TEE_SHM_DMA_BUF);
> > + shm = tee_shm_alloc_user_buf(ctx, data.size);
> > if (IS_ERR(shm))
> > return PTR_ERR(shm);
> >
> > diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c
> > index 63fce8d39d8b..d134e2778a3a 100644
> > --- a/drivers/tee/tee_shm.c
> > +++ b/drivers/tee/tee_shm.c
> > @@ -96,25 +96,14 @@ static const struct dma_buf_ops tee_shm_dma_buf_ops = {
> > .mmap = tee_shm_op_mmap,
> > };
> >
> > -struct tee_shm *tee_shm_alloc(struct tee_context *ctx, size_t size, u32 flags)
> > +static struct tee_shm *shm_alloc_helper(struct tee_context *ctx, size_t size,
> > + size_t align, u32 flags)
> > {
> > struct tee_device *teedev = ctx->teedev;
> > struct tee_shm *shm;
> > - size_t align;
> > void *ret;
> > int rc;
> >
> > - if (!(flags & TEE_SHM_MAPPED)) {
> > - dev_err(teedev->dev.parent,
> > - "only mapped allocations supported\n");
> > - return ERR_PTR(-EINVAL);
> > - }
> > -
> > - if ((flags & ~(TEE_SHM_MAPPED | TEE_SHM_DMA_BUF))) {
> > - dev_err(teedev->dev.parent, "invalid shm flags 0x%x", flags);
> > - return ERR_PTR(-EINVAL);
> > - }
> > -
> > if (!tee_device_get(teedev))
> > return ERR_PTR(-EINVAL);
> >
> > @@ -131,17 +120,14 @@ struct tee_shm *tee_shm_alloc(struct tee_context *ctx, size_t size, u32 flags)
> > }
> >
> > shm->flags = flags | TEE_SHM_POOL;
> > +
> > + /*
> > + * We're assigning this as it is needed if the shm is to be
> > + * registered. If this function returns OK then the caller expected
> > + * to call teedev_ctx_get() or clear shm->ctx in case it's not
> > + * needed any longer.
> > + */
> > shm->ctx = ctx;
> > - if (flags & TEE_SHM_DMA_BUF) {
> > - align = PAGE_SIZE;
> > - /*
> > - * Request to register the shm in the pool allocator below
> > - * if supported.
> > - */
> > - shm->flags |= TEE_SHM_REGISTER;
> > - } else {
> > - align = 2 * sizeof(long);
> > - }
> >
> > rc = teedev->pool->ops->alloc(teedev->pool, shm, size, align);
> > if (rc) {
> > @@ -149,48 +135,71 @@ struct tee_shm *tee_shm_alloc(struct tee_context *ctx, size_t size, u32 flags)
> > goto err_kfree;
> > }
> >
> > + return shm;
> > +err_kfree:
> > + kfree(shm);
> > +err_dev_put:
> > + tee_device_put(teedev);
> > + return ret;
> > +}
> >
> > - if (flags & TEE_SHM_DMA_BUF) {
> > - DEFINE_DMA_BUF_EXPORT_INFO(exp_info);
> > +/**
> > + * tee_shm_alloc_user_buf() - Allocate shared memory for user space
> > + * @ctx: Context that allocates the shared memory
> > + * @size: Requested size of shared memory
> > + *
> > + * Memory allocated as user space shared memory is automatically freed when
> > + * the TEE file pointer is closed. The primary usage of this function is
> > + * when the TEE driver doesn't support registering ordinary user space
> > + * memory.
> > + *
> > + * @returns a pointer to 'struct tee_shm'
> > + */
> > +struct tee_shm *tee_shm_alloc_user_buf(struct tee_context *ctx, size_t size)
> > +{
> > + u32 flags = TEE_SHM_MAPPED | TEE_SHM_DMA_BUF | TEE_SHM_REGISTER;
>
> Why not TEE_SHM_USER_MAPPED instead of TEE_SHM_MAPPED here?
The name TEE_SHM_USER_MAPPED is perhaps not the best, it means that it's
normal user space memory that we have pinned. We're allocating memory
which later can be mapped by user space (via dma-buf) here.
>
> > + struct tee_device *teedev = ctx->teedev;
> > + DEFINE_DMA_BUF_EXPORT_INFO(exp_info);
> > + struct tee_shm *shm;
> > + void *ret;
> >
> > - mutex_lock(&teedev->mutex);
> > - shm->id = idr_alloc(&teedev->idr, shm, 1, 0, GFP_KERNEL);
> > - mutex_unlock(&teedev->mutex);
> > - if (shm->id < 0) {
> > - ret = ERR_PTR(shm->id);
> > - goto err_pool_free;
> > - }
> > + shm = shm_alloc_helper(ctx, size, PAGE_SIZE, flags);
> > + if (IS_ERR(shm))
> > + return shm;
> >
> > - exp_info.ops = &tee_shm_dma_buf_ops;
> > - exp_info.size = shm->size;
> > - exp_info.flags = O_RDWR;
> > - exp_info.priv = shm;
> > + mutex_lock(&teedev->mutex);
> > + shm->id = idr_alloc(&teedev->idr, shm, 1, 0, GFP_KERNEL);
> > + mutex_unlock(&teedev->mutex);
> > + if (shm->id < 0) {
> > + ret = ERR_PTR(shm->id);
> > + goto err_pool_free;
> > + }
> >
> > - shm->dmabuf = dma_buf_export(&exp_info);
> > - if (IS_ERR(shm->dmabuf)) {
> > - ret = ERR_CAST(shm->dmabuf);
> > - goto err_rem;
> > - }
> > + exp_info.ops = &tee_shm_dma_buf_ops;
> > + exp_info.size = shm->size;
> > + exp_info.flags = O_RDWR;
> > + exp_info.priv = shm;
> > +
> > + shm->dmabuf = dma_buf_export(&exp_info);
> > + if (IS_ERR(shm->dmabuf)) {
> > + ret = ERR_CAST(shm->dmabuf);
> > + goto err_rem;
> > }
> >
> > teedev_ctx_get(ctx);
> > -
> > return shm;
> > err_rem:
> > - if (flags & TEE_SHM_DMA_BUF) {
> > - mutex_lock(&teedev->mutex);
> > - idr_remove(&teedev->idr, shm->id);
> > - mutex_unlock(&teedev->mutex);
> > - }
> > + mutex_lock(&teedev->mutex);
> > + idr_remove(&teedev->idr, shm->id);
> > + mutex_unlock(&teedev->mutex);
> > err_pool_free:
> > teedev->pool->ops->free(teedev->pool, shm);
> > -err_kfree:
> > kfree(shm);
> > -err_dev_put:
> > tee_device_put(teedev);
> > return ret;
> > +
> > }
> > -EXPORT_SYMBOL_GPL(tee_shm_alloc);
> > +EXPORT_SYMBOL_GPL(tee_shm_alloc_user_buf);
> >
> > /**
> > * tee_shm_alloc_kernel_buf() - Allocate shared memory for kernel buffer
> > @@ -206,10 +215,85 @@ EXPORT_SYMBOL_GPL(tee_shm_alloc);
> > */
> > struct tee_shm *tee_shm_alloc_kernel_buf(struct tee_context *ctx, size_t size)
> > {
> > - return tee_shm_alloc(ctx, size, TEE_SHM_MAPPED | TEE_SHM_DMA_BUF);
> > + u32 flags = TEE_SHM_MAPPED | TEE_SHM_REGISTER;
>
> Similar question as above... why not TEE_SHM_KERNEL_MAPPED instead of
> TEE_SHM_MAPPED?
Also here is the name a bit unfortunate, it means that the memory
originated as kernel owned memory where we've called get_kernel_pages().
>
> > + struct tee_shm *shm;
> > +
> > + shm = shm_alloc_helper(ctx, size, PAGE_SIZE, flags);
> > + if (IS_ERR(shm))
> > + return shm;
> > +
> > + teedev_ctx_get(ctx);
> > + return shm;
> > }
> > EXPORT_SYMBOL_GPL(tee_shm_alloc_kernel_buf);
> >
> > +/**
> > + * tee_shm_alloc_anon_kernel_buf() - Allocate shared memory for anonymous
> > + * kernel buffer
> > + * @ctx: Context that allocates the shared memory
> > + * @size: Requested size of shared memory
> > + *
> > + * This function returns similar shared memory as tee_shm_alloc_kernel_buf(),
> > + * but with two differences:
> > + * 1. The memory might not be registered in secure world
> > + * in case the driver supports passing memory not registered in advance.
> > + * 2. The memory is not directly associated with the passed tee_context,
> > + * rather the tee_device used by the context.
> > + *
> > + * This function should normally only be used internally in the TEE
> > + * drivers. The memory must later only be freed using
> > + * tee_shm_free_anon_kernel_buf() with a tee_contex with the same internal
> > + * tee_device as when the memory was allocated.
> > + *
> > + * This allows allocating the shared memory using one context which is
> > + * destroyed while the memory continues to live and finally freed using
> > + * another context.
> > + *
> > + * @returns a pointer to 'struct tee_shm'
> > + */
> > +struct tee_shm *tee_shm_alloc_anon_kernel_buf(struct tee_context *ctx,
> > + size_t size)
> > +{
> > + struct tee_shm *shm;
> > +
> > + shm = shm_alloc_helper(ctx, size, sizeof(long) * 2, TEE_SHM_MAPPED);
>
> The generic TEE_SHM_MAPPED is used here, as well.
Yes, it's needed by tee_shm_va2pa() and friends.
Thanks for the review,
Jens
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 19+ messages in thread