Re: [PATCH v2 06/12] optee: add driver private tee_context

On Fri, Jan 21, 2022 at 1:48 PM Sumit Garg sumit.garg@linaro.org wrote:

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

...

Adds a driver private tee_context by moving the tee_context in struct optee_notif to struct optee. This tee_context is used when doing internal calls to secure world to deliver notification and later also when sharing driver private memory with secure world.

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

drivers/tee/optee/core.c | 1 + drivers/tee/optee/ffa_abi.c | 61 ++++++++++++++++++------------- drivers/tee/optee/optee_private.h | 5 ++- drivers/tee/optee/smc_abi.c | 40 ++++++-------------- 4 files changed, 51 insertions(+), 56 deletions(-)

After looking at this patch, it looks like there is a need for some more refactoring to pick out common probe functionality from both ABIs and add optee_probe_common(). If it was there earlier, this patch would mostly be modifying the OP-TEE core file apart from notifications stuff.

I would leave it upto you to have a refactoring patch before and then changes in this patch or vice-versa.

I'll do the refactoring after this patch set.

Thanks, Jens

-Sumit

...

diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c index 2a369e346b85..f4bccb5f0e93 100644 --- a/drivers/tee/optee/core.c +++ b/drivers/tee/optee/core.c @@ -161,6 +161,7 @@ void optee_remove_common(struct optee *optee) optee_unregister_devices();

    optee_notif_uninit(optee);

•   teedev_close_context(optee->ctx);
    /*
     * The two devices have to be unregistered before we can free the
     * other resources.
  

diff --git a/drivers/tee/optee/ffa_abi.c b/drivers/tee/optee/ffa_abi.c index 18963f7e4d48..88a028d4fb7b 100644 --- a/drivers/tee/optee/ffa_abi.c +++ b/drivers/tee/optee/ffa_abi.c @@ -766,7 +766,9 @@ static int optee_ffa_probe(struct ffa_device *ffa_dev) { const struct ffa_dev_ops *ffa_ops; unsigned int rpc_arg_count;

•   struct tee_shm_pool *pool;
    struct tee_device *teedev;
  

•   struct tee_context *ctx;
    struct optee *optee;
    int rc;
  
  

@@ -786,12 +788,12 @@ static int optee_ffa_probe(struct ffa_device *ffa_dev) if (!optee) return -ENOMEM;

•   optee->pool = optee_ffa_shm_pool_alloc_pages();
  

•   if (IS_ERR(optee->pool)) {
  

•           rc = PTR_ERR(optee->pool);
  

•           optee->pool = NULL;
  

•           goto err;
  

•   pool = optee_ffa_shm_pool_alloc_pages();
  

•   if (IS_ERR(pool)) {
  

•           rc = PTR_ERR(pool);
  

•           goto err_free_optee;
    }
  

•   optee->pool = pool;
  
    optee->ops = &optee_ffa_ops;
    optee->ffa.ffa_dev = ffa_dev;
  

@@ -802,7 +804,7 @@ static int optee_ffa_probe(struct ffa_device *ffa_dev) optee); if (IS_ERR(teedev)) { rc = PTR_ERR(teedev);

•           goto err;
  

•           goto err_free_pool;
    }
    optee->teedev = teedev;
  
  

@@ -810,50 +812,57 @@ static int optee_ffa_probe(struct ffa_device *ffa_dev) optee); if (IS_ERR(teedev)) { rc = PTR_ERR(teedev);

•           goto err;
  

•           goto err_unreg_teedev;
    }
    optee->supp_teedev = teedev;
  
    rc = tee_device_register(optee->teedev);
    if (rc)
  

•           goto err;
  

•           goto err_unreg_supp_teedev;
  
    rc = tee_device_register(optee->supp_teedev);
    if (rc)
  

•           goto err;
  

•           goto err_unreg_supp_teedev;
  
    rc = rhashtable_init(&optee->ffa.global_ids, &shm_rhash_params);
    if (rc)
  

•           goto err;
  

•           goto err_unreg_supp_teedev;
    mutex_init(&optee->ffa.mutex);
    mutex_init(&optee->call_queue.mutex);
    INIT_LIST_HEAD(&optee->call_queue.waiters);
    optee_supp_init(&optee->supp);
    ffa_dev_set_drvdata(ffa_dev, optee);
  

•   ctx = teedev_open(optee->teedev);
  

•   if (IS_ERR(ctx))
  

•           goto err_rhashtable_free;
  

•   optee->ctx = ctx;
    rc = optee_notif_init(optee, OPTEE_DEFAULT_MAX_NOTIF_VALUE);
  

•   if (rc) {
  

•           optee_ffa_remove(ffa_dev);
  

•           return rc;
  

•   }
  

•   if (rc)
  

•           goto err_close_ctx;
  
    rc = optee_enumerate_devices(PTA_CMD_GET_DEVICES);
  

•   if (rc) {
  

•           optee_ffa_remove(ffa_dev);
  

•           return rc;
  

•   }
  

•   if (rc)
  

•           goto err_unregister_devices;
  
    pr_info("initialized driver\n");
    return 0;
  

-err:

•   /*
  

•    * tee_device_unregister() is safe to call even if the
  

•    * devices hasn't been registered with
  

•    * tee_device_register() yet.
  

•    */
  

+err_unregister_devices:

•   optee_unregister_devices();
  

•   optee_notif_uninit(optee);
  

+err_close_ctx:

•   teedev_close_context(ctx);
  

+err_rhashtable_free:

•   rhashtable_free_and_destroy(&optee->ffa.global_ids, rh_free_fn, NULL);
  

•   optee_supp_uninit(&optee->supp);
  

•   mutex_destroy(&optee->call_queue.mutex);
  

+err_unreg_supp_teedev: tee_device_unregister(optee->supp_teedev); +err_unreg_teedev: tee_device_unregister(optee->teedev);

•   if (optee->pool)
  

•           tee_shm_pool_free(optee->pool);
  

+err_free_pool:

•   tee_shm_pool_free(pool);
  

+err_free_optee: kfree(optee); return rc; } diff --git a/drivers/tee/optee/optee_private.h b/drivers/tee/optee/optee_private.h index df2450921464..df3a483bbf46 100644 --- a/drivers/tee/optee/optee_private.h +++ b/drivers/tee/optee/optee_private.h @@ -53,7 +53,6 @@ struct optee_call_queue {

struct optee_notif { u_int max_key;

•   struct tee_context *ctx;
    /* Serializes access to the elements below in this struct */
    spinlock_t lock;
    struct list_head db;
  

@@ -134,9 +133,10 @@ struct optee_ops { /**

• struct optee - main service struct
• @supp_teedev: supplicant device

• • @teedev: client device
  • @ops: internal callbacks for different ways to reach secure

  •                 world
    

• • @teedev: client device

• • @ctx: driver internal TEE context
  • @smc: specific to SMC ABI
  • @ffa: specific to FF-A ABI
  • @call_queue: queue of threads waiting to call @invoke_fn

@@ -152,6 +152,7 @@ struct optee { struct tee_device *supp_teedev; struct tee_device *teedev; const struct optee_ops *ops;

•   struct tee_context *ctx;
    union {
            struct optee_smc smc;
            struct optee_ffa ffa;
  

diff --git a/drivers/tee/optee/smc_abi.c b/drivers/tee/optee/smc_abi.c index 196cd4316d7d..1dbb13b08381 100644 --- a/drivers/tee/optee/smc_abi.c +++ b/drivers/tee/optee/smc_abi.c @@ -952,57 +952,34 @@ static irqreturn_t notif_irq_thread_fn(int irq, void *dev_id) { struct optee *optee = dev_id;

•   optee_smc_do_bottom_half(optee->notif.ctx);
  

•   optee_smc_do_bottom_half(optee->ctx);
  
    return IRQ_HANDLED;
  

}

static int optee_smc_notif_init_irq(struct optee *optee, u_int irq) {

•   struct tee_context *ctx;
    int rc;
  
  

•   ctx = teedev_open(optee->teedev);
  

•   if (IS_ERR(ctx))
  

•           return PTR_ERR(ctx);
  

•   optee->notif.ctx = ctx;
    rc = request_threaded_irq(irq, notif_irq_handler,
                              notif_irq_thread_fn,
                              0, "optee_notification", optee);
    if (rc)
  

•           goto err_close_ctx;
  

•           return rc;
  
    optee->smc.notif_irq = irq;
  
    return 0;
  

-err_close_ctx:

•   teedev_close_context(optee->notif.ctx);
  

•   optee->notif.ctx = NULL;
  

•   return rc;
  

}

static void optee_smc_notif_uninit_irq(struct optee *optee) {

•   if (optee->notif.ctx) {
  

•           optee_smc_stop_async_notif(optee->notif.ctx);
  

•   if (optee->smc.sec_caps & OPTEE_SMC_SEC_CAP_ASYNC_NOTIF) {
  

•           optee_smc_stop_async_notif(optee->ctx);
            if (optee->smc.notif_irq) {
                    free_irq(optee->smc.notif_irq, optee);
                    irq_dispose_mapping(optee->smc.notif_irq);
            }
  

•           /*
  

•            * The thread normally working with optee->notif.ctx was
  

•            * stopped with free_irq() above.
  

•            *
  

•            * Note we're not using teedev_close_context() or
  

•            * tee_client_close_context() since we have already called
  

•            * tee_device_put() while initializing to avoid a circular
  

•            * reference counting.
  

•            */
  

•           teedev_close_context(optee->notif.ctx);
    }
  

}

@@ -1307,6 +1284,7 @@ static int optee_probe(struct platform_device *pdev) struct optee *optee = NULL; void *memremaped_shm = NULL; struct tee_device *teedev;

•   struct tee_context *ctx;
    u32 max_notif_value;
    u32 sec_caps;
    int rc;
  

@@ -1387,9 +1365,13 @@ static int optee_probe(struct platform_device *pdev) optee->pool = pool;

    platform_set_drvdata(pdev, optee);

•   ctx = teedev_open(optee->teedev);
  

•   if (IS_ERR(ctx))
  

•           goto err_supp_uninit;
  

•   optee->ctx = ctx;
    rc = optee_notif_init(optee, max_notif_value);
    if (rc)
  

•           goto err_supp_uninit;
  

•           goto err_close_ctx;
  
    if (sec_caps & OPTEE_SMC_SEC_CAP_ASYNC_NOTIF) {
            unsigned int irq;
  

@@ -1437,6 +1419,8 @@ static int optee_probe(struct platform_device *pdev) optee_unregister_devices(); err_notif_uninit: optee_notif_uninit(optee); +err_close_ctx:

•   teedev_close_context(ctx);
  

err_supp_uninit: optee_supp_uninit(&optee->supp); mutex_destroy(&optee->call_queue.mutex); -- 2.31.1