Message-ID: <20210610090134.GD2753553@jade>
Date:   Thu, 10 Jun 2021 11:01:34 +0200
From:   Jens Wiklander <jens.wiklander@...aro.org>
To:     Tyler Hicks <tyhicks@...ux.microsoft.com>
Cc:     linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        op-tee@...ts.trustedfirmware.org,
        Sumit Garg <sumit.garg@...aro.org>,
        Herbert Xu <herbert@...dor.apana.org.au>,
        Sakkinen <jarkko.sakkinen@...ux.intel.com>,
        Sasha Levin <sashal@...nel.org>,
        Thirupathaiah Annapureddy <thiruan@...rosoft.com>,
        Vikas Gupta <vikas.gupta@...adcom.com>,
        "David S . Miller" <davem@...emloft.net>
Subject: 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@...aro.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

Powered by blists - more mailing lists