Hi,
Here's a group of patches to the tee subsystem with cleanups and not so urgent fixes related to tee shared memory.
- Unused parts of the shared memory object (struct tee_shm) are removed. - Shared memory ids usable from user space are not assigned to driver private shared memory objects - The TEE_SHM_USER_MAPPED is used instead of TEE_SHM_REGISTER to accurately tell if a shared memory object originates from another user space mapping.
Only unused "features" should be removed with this patch set, there should be no change in behaviour or breakage with other code.
Thanks, Jens
Jens Wiklander (5): tee: remove linked list of struct tee_shm tee: remove unused tee_shm_priv_alloc() tee: don't assign shm id for private shms tee: remove redundant teedev in struct tee_shm tee: tee_shm_op_mmap(): use TEE_SHM_USER_MAPPED
drivers/tee/tee_core.c | 1 - drivers/tee/tee_private.h | 3 +- drivers/tee/tee_shm.c | 85 +++++++++++---------------------------- include/linux/tee_drv.h | 19 +-------- 4 files changed, 27 insertions(+), 81 deletions(-)
Removes list_shm from struct tee_context since the linked list isn't used any longer.
Signed-off-by: Jens Wiklander jens.wiklander@linaro.org --- drivers/tee/tee_core.c | 1 - drivers/tee/tee_shm.c | 12 +----------- include/linux/tee_drv.h | 3 --- 3 files changed, 1 insertion(+), 15 deletions(-)
diff --git a/drivers/tee/tee_core.c b/drivers/tee/tee_core.c index 37d22e39fd8d..6aec502c495c 100644 --- a/drivers/tee/tee_core.c +++ b/drivers/tee/tee_core.c @@ -44,7 +44,6 @@ static struct tee_context *teedev_open(struct tee_device *teedev)
kref_init(&ctx->refcount); ctx->teedev = teedev; - INIT_LIST_HEAD(&ctx->list_shm); rc = teedev->desc->ops->open(ctx); if (rc) goto err; diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c index 09ddcd06c715..6cabb834db7d 100644 --- a/drivers/tee/tee_shm.c +++ b/drivers/tee/tee_shm.c @@ -17,8 +17,6 @@ static void tee_shm_release(struct tee_shm *shm)
mutex_lock(&teedev->mutex); idr_remove(&teedev->idr, shm->id); - if (shm->ctx) - list_del(&shm->link); mutex_unlock(&teedev->mutex);
if (shm->flags & TEE_SHM_POOL) { @@ -174,12 +172,8 @@ static struct tee_shm *__tee_shm_alloc(struct tee_context *ctx, } }
- if (ctx) { + if (ctx) teedev_ctx_get(ctx); - mutex_lock(&teedev->mutex); - list_add_tail(&shm->link, &ctx->list_shm); - mutex_unlock(&teedev->mutex); - }
return shm; err_rem: @@ -307,10 +301,6 @@ struct tee_shm *tee_shm_register(struct tee_context *ctx, unsigned long addr, } }
- mutex_lock(&teedev->mutex); - list_add_tail(&shm->link, &ctx->list_shm); - mutex_unlock(&teedev->mutex); - return shm; err: if (shm) { diff --git a/include/linux/tee_drv.h b/include/linux/tee_drv.h index 7a03f68fb982..cbddb883a7f8 100644 --- a/include/linux/tee_drv.h +++ b/include/linux/tee_drv.h @@ -49,7 +49,6 @@ struct tee_shm_pool; */ struct tee_context { struct tee_device *teedev; - struct list_head list_shm; void *data; struct kref refcount; bool releasing; @@ -170,7 +169,6 @@ void tee_device_unregister(struct tee_device *teedev); * struct tee_shm - shared memory object * @teedev: device used to allocate the object * @ctx: context using the object, if NULL the context is gone - * @link link element * @paddr: physical address of the shared memory * @kaddr: virtual address of the shared memory * @size: size of shared memory @@ -187,7 +185,6 @@ void tee_device_unregister(struct tee_device *teedev); struct tee_shm { struct tee_device *teedev; struct tee_context *ctx; - struct list_head link; phys_addr_t paddr; void *kaddr; size_t size;
tee_shm_priv_alloc() isn't useful in the current state and it's also not not used so remove it.
Signed-off-by: Jens Wiklander jens.wiklander@linaro.org --- drivers/tee/tee_shm.c | 33 ++------------------------------- include/linux/tee_drv.h | 12 ------------ 2 files changed, 2 insertions(+), 43 deletions(-)
diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c index 6cabb834db7d..8afe08b23242 100644 --- a/drivers/tee/tee_shm.c +++ b/drivers/tee/tee_shm.c @@ -95,20 +95,14 @@ static const struct dma_buf_ops tee_shm_dma_buf_ops = { .mmap = tee_shm_op_mmap, };
-static struct tee_shm *__tee_shm_alloc(struct tee_context *ctx, - struct tee_device *teedev, - size_t size, u32 flags) +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; void *ret; int rc;
- if (ctx && ctx->teedev != teedev) { - dev_err(teedev->dev.parent, "ctx and teedev mismatch\n"); - return ERR_PTR(-EINVAL); - } - if (!(flags & TEE_SHM_MAPPED)) { dev_err(teedev->dev.parent, "only mapped allocations supported\n"); @@ -188,31 +182,8 @@ static struct tee_shm *__tee_shm_alloc(struct tee_context *ctx, tee_device_put(teedev); return ret; } - -/** - * 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_* in <linux/tee_drv.h>. 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. - */ -struct tee_shm *tee_shm_alloc(struct tee_context *ctx, size_t size, u32 flags) -{ - return __tee_shm_alloc(ctx, ctx->teedev, size, flags); -} EXPORT_SYMBOL_GPL(tee_shm_alloc);
-struct tee_shm *tee_shm_priv_alloc(struct tee_device *teedev, size_t size) -{ - return __tee_shm_alloc(NULL, teedev, size, TEE_SHM_MAPPED); -} -EXPORT_SYMBOL_GPL(tee_shm_priv_alloc); - 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 cbddb883a7f8..42687f6c546d 100644 --- a/include/linux/tee_drv.h +++ b/include/linux/tee_drv.h @@ -315,18 +315,6 @@ void *tee_get_drvdata(struct tee_device *teedev); */ struct tee_shm *tee_shm_alloc(struct tee_context *ctx, size_t size, u32 flags);
-/** - * tee_shm_priv_alloc() - Allocate shared memory privately - * @dev: Device that allocates the shared memory - * @size: Requested size of shared memory - * - * Allocates shared memory buffer that is not associated with any client - * context. Such buffers are owned by TEE driver and used for internal calls. - * - * @returns a pointer to 'struct tee_shm' - */ -struct tee_shm *tee_shm_priv_alloc(struct tee_device *teedev, size_t size); - /** * tee_shm_register() - Register shared memory buffer * @ctx: Context that registers the shared memory
Private shared memory object must not be referenced from user space. To guarantee that, don't assign an id to shared memory objects which are driver private.
Signed-off-by: Jens Wiklander jens.wiklander@linaro.org --- drivers/tee/tee_private.h | 3 ++- drivers/tee/tee_shm.c | 31 ++++++++++++++++++------------- 2 files changed, 20 insertions(+), 14 deletions(-)
diff --git a/drivers/tee/tee_private.h b/drivers/tee/tee_private.h index f797171f0434..e55204df31ce 100644 --- a/drivers/tee/tee_private.h +++ b/drivers/tee/tee_private.h @@ -37,7 +37,8 @@ struct tee_shm_pool { * @num_users: number of active users of this device * @c_no_user: completion used when unregistering the device * @mutex: mutex protecting @num_users and @idr - * @idr: register of shared memory object allocated on this device + * @idr: register of user space shared memory objects allocated or + * registered on this device * @pool: shared memory pool */ struct tee_device { diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c index 8afe08b23242..e636cf82acdb 100644 --- a/drivers/tee/tee_shm.c +++ b/drivers/tee/tee_shm.c @@ -15,9 +15,11 @@ static void tee_shm_release(struct tee_shm *shm) { struct tee_device *teedev = shm->teedev;
- mutex_lock(&teedev->mutex); - idr_remove(&teedev->idr, shm->id); - mutex_unlock(&teedev->mutex); + if (shm->flags & TEE_SHM_DMA_BUF) { + mutex_lock(&teedev->mutex); + idr_remove(&teedev->idr, shm->id); + mutex_unlock(&teedev->mutex); + }
if (shm->flags & TEE_SHM_POOL) { struct tee_shm_pool_mgr *poolm; @@ -143,17 +145,18 @@ struct tee_shm *tee_shm_alloc(struct tee_context *ctx, size_t size, u32 flags) goto err_kfree; }
- 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; - }
if (flags & TEE_SHM_DMA_BUF) { DEFINE_DMA_BUF_EXPORT_INFO(exp_info);
+ 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; + } + exp_info.ops = &tee_shm_dma_buf_ops; exp_info.size = shm->size; exp_info.flags = O_RDWR; @@ -171,9 +174,11 @@ struct tee_shm *tee_shm_alloc(struct tee_context *ctx, size_t size, u32 flags)
return shm; err_rem: - mutex_lock(&teedev->mutex); - idr_remove(&teedev->idr, shm->id); - mutex_unlock(&teedev->mutex); + if (flags & TEE_SHM_DMA_BUF) { + mutex_lock(&teedev->mutex); + idr_remove(&teedev->idr, shm->id); + mutex_unlock(&teedev->mutex); + } err_pool_free: poolm->ops->free(poolm, shm); err_kfree:
The ctx element in struct tee_shm is always valid. So remove the now redundant teedev element.
Signed-off-by: Jens Wiklander jens.wiklander@linaro.org --- drivers/tee/tee_shm.c | 7 ++----- include/linux/tee_drv.h | 4 +--- 2 files changed, 3 insertions(+), 8 deletions(-)
diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c index e636cf82acdb..c64ec5c5e464 100644 --- a/drivers/tee/tee_shm.c +++ b/drivers/tee/tee_shm.c @@ -13,7 +13,7 @@
static void tee_shm_release(struct tee_shm *shm) { - struct tee_device *teedev = shm->teedev; + struct tee_device *teedev = shm->ctx->teedev;
if (shm->flags & TEE_SHM_DMA_BUF) { mutex_lock(&teedev->mutex); @@ -44,8 +44,7 @@ static void tee_shm_release(struct tee_shm *shm) kfree(shm->pages); }
- if (shm->ctx) - teedev_ctx_put(shm->ctx); + teedev_ctx_put(shm->ctx);
kfree(shm);
@@ -132,7 +131,6 @@ struct tee_shm *tee_shm_alloc(struct tee_context *ctx, size_t size, u32 flags) }
shm->flags = flags | TEE_SHM_POOL; - shm->teedev = teedev; shm->ctx = ctx; if (flags & TEE_SHM_DMA_BUF) poolm = teedev->pool->dma_buf_mgr; @@ -221,7 +219,6 @@ struct tee_shm *tee_shm_register(struct tee_context *ctx, unsigned long addr, }
shm->flags = flags | TEE_SHM_REGISTER; - shm->teedev = teedev; shm->ctx = ctx; shm->id = -1; addr = untagged_addr(addr); diff --git a/include/linux/tee_drv.h b/include/linux/tee_drv.h index 42687f6c546d..1412e9cc79ce 100644 --- a/include/linux/tee_drv.h +++ b/include/linux/tee_drv.h @@ -167,8 +167,7 @@ void tee_device_unregister(struct tee_device *teedev);
/** * struct tee_shm - shared memory object - * @teedev: device used to allocate the object - * @ctx: context using the object, if NULL the context is gone + * @ctx: context using the object * @paddr: physical address of the shared memory * @kaddr: virtual address of the shared memory * @size: size of shared memory @@ -183,7 +182,6 @@ void tee_device_unregister(struct tee_device *teedev); * subsystem and from drivers that implements their own shm pool manager. */ struct tee_shm { - struct tee_device *teedev; struct tee_context *ctx; phys_addr_t paddr; void *kaddr;
tee_shm_op_mmap() uses the TEE_SHM_USER_MAPPED flag instead of the TEE_SHM_REGISTER flag to tell if a shared memory object is originating from registered user space memory.
Signed-off-by: Jens Wiklander jens.wiklander@linaro.org --- drivers/tee/tee_shm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c index c64ec5c5e464..deb22f877881 100644 --- a/drivers/tee/tee_shm.c +++ b/drivers/tee/tee_shm.c @@ -81,7 +81,7 @@ static int tee_shm_op_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma) size_t size = vma->vm_end - vma->vm_start;
/* Refuse sharing shared memory provided by application */ - if (shm->flags & TEE_SHM_REGISTER) + if (shm->flags & TEE_SHM_USER_MAPPED) return -EINVAL;
return remap_pfn_range(vma, vma->vm_start, shm->paddr >> PAGE_SHIFT,
Hi,
On Thu, Jan 9, 2020 at 1:37 PM Jens Wiklander jens.wiklander@linaro.org wrote:
Hi,
Here's a group of patches to the tee subsystem with cleanups and not so urgent fixes related to tee shared memory.
- Unused parts of the shared memory object (struct tee_shm) are removed.
- Shared memory ids usable from user space are not assigned to driver private shared memory objects
- The TEE_SHM_USER_MAPPED is used instead of TEE_SHM_REGISTER to accurately tell if a shared memory object originates from another user space mapping.
Only unused "features" should be removed with this patch set, there should be no change in behaviour or breakage with other code.
I'll pick up these as they are in a few days if there's no further comments.
Thanks, Jens
Thanks, Jens
Jens Wiklander (5): tee: remove linked list of struct tee_shm tee: remove unused tee_shm_priv_alloc() tee: don't assign shm id for private shms tee: remove redundant teedev in struct tee_shm tee: tee_shm_op_mmap(): use TEE_SHM_USER_MAPPED
drivers/tee/tee_core.c | 1 - drivers/tee/tee_private.h | 3 +- drivers/tee/tee_shm.c | 85 +++++++++++---------------------------- include/linux/tee_drv.h | 19 +-------- 4 files changed, 27 insertions(+), 81 deletions(-)
-- 2.17.1
-
Jens Wiklander