Re: [PATCH v2 11/12] tee: replace tee_shm_register()

On Thu, 20 Jan 2022 at 19:42, Jens Wiklander jens.wiklander@linaro.org wrote:

On Thu, Jan 20, 2022 at 12:51 PM Sumit Garg sumit.garg@linaro.org wrote:

...

On Fri, 14 Jan 2022 at 20:38, Jens Wiklander jens.wiklander@linaro.org wrote:

...

tee_shm_register() is replaced by the previously introduced functions tee_shm_register_user_buf() and tee_shm_register_kernel_buf().

Since there are not external callers left we can remove tee_shm_register() and refactor the remains.

Signed-off-by: Jens Wiklander jens.wiklander@linaro.org

drivers/tee/tee_shm.c | 157 +++++++++++++++++++++++----------------- include/linux/tee_drv.h | 11 --- 2 files changed, 91 insertions(+), 77 deletions(-)

diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c index 6a1dbce75616..fe3e5977dd7c 100644 --- a/drivers/tee/tee_shm.c +++ b/drivers/tee/tee_shm.c @@ -12,17 +12,43 @@ #include <linux/uio.h> #include "tee_private.h"

+static void shm_put_kernel_pages(struct page **pages, unsigned long npages) +{

•   unsigned long n;
  

nit: Can we have a consistent prototype for the number of pages? size_t as used by struct tee_shm or unsigned long? Same holds true for variables below as well: npages, page_count, num_pages, n.

Fair point, I'll fix what's changed here.

...

...

•   for (n = 0; n < npages; n++)
  

•           put_page(pages[n]);
  

+}

+static int shm_get_kernel_pages(unsigned long start, int page_count,

•                           struct page **pages)
  

+{

•   struct kvec *kiov;
  

•   unsigned int n;
  

•   int rc;
  

•   kiov = kcalloc(page_count, sizeof(*kiov), GFP_KERNEL);
  

•   if (!kiov)
  

•           return -ENOMEM;
  

•   for (n = 0; n < page_count; n++) {
  

•           kiov[n].iov_base = (void *)(start + n * PAGE_SIZE);
  

•           kiov[n].iov_len = PAGE_SIZE;
  

•   }
  

•   rc = get_kernel_pages(kiov, page_count, 0, pages);
  

•   kfree(kiov);
  

•   return rc;
  

+}

static void release_registered_pages(struct tee_shm *shm) { if (shm->pages) {

•           if (shm->flags & TEE_SHM_USER_MAPPED) {
  

•           if (shm->flags & TEE_SHM_USER_MAPPED)
                    unpin_user_pages(shm->pages, shm->num_pages);
  

•           } else {
  

•                   size_t n;
  

•                   for (n = 0; n < shm->num_pages; n++)
  

•                           put_page(shm->pages[n]);
  

•           }
  

•           else
  

•                   shm_put_kernel_pages(shm->pages, shm->num_pages);
  
            kfree(shm->pages);
    }
  

@@ -191,28 +217,24 @@ struct tee_shm *tee_shm_alloc_priv_kernel_buf(struct tee_context *ctx, } EXPORT_SYMBOL_GPL(tee_shm_alloc_priv_kernel_buf);

-struct tee_shm *tee_shm_register(struct tee_context *ctx, unsigned long addr,

•                            size_t length, u32 flags)
  

+static struct tee_shm * +register_shm_helper(struct tee_context *ctx, unsigned long addr,

•               size_t length, u32 flags, int id)
  

{ struct tee_device *teedev = ctx->teedev;

•   const u32 req_user_flags = TEE_SHM_DMA_BUF | TEE_SHM_USER_MAPPED;
  

•   const u32 req_kernel_flags = TEE_SHM_DMA_BUF | TEE_SHM_KERNEL_MAPPED;
    struct tee_shm *shm;
  

•   unsigned long start;
  

•   int num_pages;
    void *ret;
    int rc;
  

•   int num_pages;
  

•   unsigned long start;
  

•   if (flags != req_user_flags && flags != req_kernel_flags)
  

•           return ERR_PTR(-ENOTSUPP);
  
    if (!tee_device_get(teedev))
            return ERR_PTR(-EINVAL);
  
    if (!teedev->desc->ops->shm_register ||
        !teedev->desc->ops->shm_unregister) {
  

•           tee_device_put(teedev);
  

•           return ERR_PTR(-ENOTSUPP);
  

•           ret = ERR_PTR(-ENOTSUPP);
  

Checkpatch warns about this error code:

WARNING: ENOTSUPP is not a SUSV4 error code, prefer EOPNOTSUPP #104: FILE: drivers/tee/tee_shm.c:236:

• ret = ERR_PTR(-ENOTSUPP);

I noticed that too. I'd rather keep it as it is for now as changing would a bit of a ABI change even if I doubt that anyone is depending on this particular errno value.

Okay, I am fine with this.

-Sumit

...

...

•           goto err_dev_put;
    }
  
    teedev_ctx_get(ctx);
  

@@ -220,13 +242,13 @@ struct tee_shm *tee_shm_register(struct tee_context *ctx, unsigned long addr, shm = kzalloc(sizeof(*shm), GFP_KERNEL); if (!shm) { ret = ERR_PTR(-ENOMEM);

•           goto err;
  

•           goto err_ctx_put;
    }
  
    refcount_set(&shm->refcount, 1);
  

•   shm->flags = flags | TEE_SHM_REGISTER;
  

•   shm->flags = flags;
    shm->ctx = ctx;
  

•   shm->id = -1;
  

•   shm->id = id;
    addr = untagged_addr(addr);
    start = rounddown(addr, PAGE_SIZE);
    shm->offset = addr - start;
  

@@ -235,71 +257,45 @@ struct tee_shm *tee_shm_register(struct tee_context *ctx, unsigned long addr, shm->pages = kcalloc(num_pages, sizeof(*shm->pages), GFP_KERNEL); if (!shm->pages) { ret = ERR_PTR(-ENOMEM);

•           goto err;
  

•           goto err_free_shm;
    }
  
  

•   if (flags & TEE_SHM_USER_MAPPED) {
  

•   if (flags & TEE_SHM_USER_MAPPED)
            rc = pin_user_pages_fast(start, num_pages, FOLL_WRITE,
                                     shm->pages);
  

•   } else {
  

•           struct kvec *kiov;
  

•           int i;
  

•           kiov = kcalloc(num_pages, sizeof(*kiov), GFP_KERNEL);
  

•           if (!kiov) {
  

•                   ret = ERR_PTR(-ENOMEM);
  

•                   goto err;
  

•           }
  

•           for (i = 0; i < num_pages; i++) {
  

•                   kiov[i].iov_base = (void *)(start + i * PAGE_SIZE);
  

•                   kiov[i].iov_len = PAGE_SIZE;
  

•           }
  

•           rc = get_kernel_pages(kiov, num_pages, 0, shm->pages);
  

•           kfree(kiov);
  

•   }
  

•   else
  

•           rc = shm_get_kernel_pages(start, num_pages, shm->pages);
    if (rc > 0)
            shm->num_pages = rc;
    if (rc != num_pages) {
            if (rc >= 0)
                    rc = -ENOMEM;
            ret = ERR_PTR(rc);
  

•           goto err;
  

•   }
  

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

•           goto err_put_shm_pages;
    }
  
    rc = teedev->desc->ops->shm_register(ctx, shm, shm->pages,
                                         shm->num_pages, start);
    if (rc) {
            ret = ERR_PTR(rc);
  

•           goto err;
  

•           goto err_put_shm_pages;
    }
  
    return shm;
  

-err:

•   if (shm) {
  

•           if (shm->id >= 0) {
  

•                   mutex_lock(&teedev->mutex);
  

•                   idr_remove(&teedev->idr, shm->id);
  

•                   mutex_unlock(&teedev->mutex);
  

•           }
  

•           release_registered_pages(shm);
  

•   }
  

+err_put_shm_pages:

•   if (flags & TEE_SHM_USER_MAPPED)
  

•           unpin_user_pages(shm->pages, shm->num_pages);
  

•   else
  

•           shm_put_kernel_pages(shm->pages, shm->num_pages);
  

•   kfree(shm->pages);
  

+err_free_shm: kfree(shm); +err_ctx_put: teedev_ctx_put(ctx); +err_dev_put: tee_device_put(teedev); return ret; } -EXPORT_SYMBOL_GPL(tee_shm_register);

/**

• tee_shm_register_user_buf() - Register a userspace shared memory buffer

@@ -312,8 +308,36 @@ EXPORT_SYMBOL_GPL(tee_shm_register); struct tee_shm *tee_shm_register_user_buf(struct tee_context *ctx, unsigned long addr, size_t length) {

•   return tee_shm_register(ctx, addr, length,
  

•                           TEE_SHM_DMA_BUF | TEE_SHM_USER_MAPPED);
  

•   u32 f = TEE_SHM_DMA_BUF | TEE_SHM_USER_MAPPED | TEE_SHM_REGISTER;
  

nit: I would prefer to write variable name as "flags" for clarity.

...

•   struct tee_device *teedev = ctx->teedev;
  

•   struct tee_shm *shm;
  

•   void *ret;
  

•   int id;
  

•   mutex_lock(&teedev->mutex);
  

•   id = idr_alloc(&teedev->idr, NULL, 1, 0, GFP_KERNEL);
  

•   mutex_unlock(&teedev->mutex);
  

nit: redundant extra line.

...

•   if (id < 0)
  

•           return ERR_PTR(id);
  

•   shm = register_shm_helper(ctx, addr, length, f, id);
  

•   if (IS_ERR(shm)) {
  

•           mutex_lock(&teedev->mutex);
  

•           idr_remove(&teedev->idr, id);
  

•           mutex_unlock(&teedev->mutex);
  

•           return shm;
  

•   }
  

•   mutex_lock(&teedev->mutex);
  

•   ret = idr_replace(&teedev->idr, shm, id);
  

•   mutex_unlock(&teedev->mutex);
  

•   if (IS_ERR(ret)) {
  

•           tee_shm_free(shm);
  

•           return ret;
  

•   }
  

•   return shm;
  

} EXPORT_SYMBOL_GPL(tee_shm_register_user_buf);

@@ -330,8 +354,9 @@ EXPORT_SYMBOL_GPL(tee_shm_register_user_buf); struct tee_shm *tee_shm_register_kernel_buf(struct tee_context *ctx, void *addr, size_t length) {

•   return tee_shm_register(ctx, (unsigned long)addr, length,
  

•                           TEE_SHM_DMA_BUF | TEE_SHM_KERNEL_MAPPED);
  

•   u32 f = TEE_SHM_REGISTER | TEE_SHM_KERNEL_MAPPED;
  

Ditto.

Thanks, Jens

...

-Sumit

...

•   return register_shm_helper(ctx, (unsigned long)addr, length, f, -1);
  

} EXPORT_SYMBOL_GPL(tee_shm_register_kernel_buf);

diff --git a/include/linux/tee_drv.h b/include/linux/tee_drv.h index 029c9a0590cc..e4f32885e273 100644 --- a/include/linux/tee_drv.h +++ b/include/linux/tee_drv.h @@ -278,17 +278,6 @@ struct tee_shm *tee_shm_alloc_priv_kernel_buf(struct tee_context *ctx, size_t size); struct tee_shm *tee_shm_alloc_kernel_buf(struct tee_context *ctx, size_t size);

-/**

• • tee_shm_register() - Register shared memory buffer
• • @ctx: Context that registers the shared memory
• • @addr: Address is userspace of the shared buffer
• • @length: Length of the shared buffer
• • @flags: Flags setting properties for the requested shared memory.
• • @returns a pointer to 'struct tee_shm'
• */

-struct tee_shm *tee_shm_register(struct tee_context *ctx, unsigned long addr,

•                            size_t length, u32 flags);
  

struct tee_shm *tee_shm_register_user_buf(struct tee_context *ctx, unsigned long addr, size_t length); struct tee_shm *tee_shm_register_kernel_buf(struct tee_context *ctx, -- 2.31.1