Re: [PATCH 2/7] tee: simplify shm pool handling

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

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