Messages in this thread | | | From | Sumit Garg <> | Date | Mon, 11 Apr 2022 18:48:46 +0530 | Subject | Re: [PATCH v2 2/4] optee: add OPTEE_SMC_CALL_WITH_RPC_ARG and OPTEE_SMC_CALL_WITH_REGD_ARG |
| |
On Mon, 21 Mar 2022 at 18:33, Jens Wiklander <jens.wiklander@linaro.org> wrote: > > Adds OPTEE_SMC_CALL_WITH_RPC_ARG and OPTEE_SMC_CALL_WITH_REGD_ARG where > the struct optee_msg_arg to be used for RPC is appended in the memory > following the normal argument struct optee_msg_arg. This is an > optimization to avoid caching the RPC argument struct while still > maintaining similar performance as if it was cached. > > OPTEE_SMC_CALL_WITH_REGD_ARG optimized one step further by using a > registered shared memory object instead. It's in other aspects identical > to OPTEE_SMC_CALL_WITH_RPC_ARG. > > The presence of OPTEE_SMC_CALL_WITH_RPC_ARG and > OPTEE_SMC_CALL_WITH_REGD_ARG is indicated by the new > OPTEE_SMC_SEC_CAP_RPC_ARG bit returned by > OPTEE_SMC_EXCHANGE_CAPABILITIES. OPTEE_SMC_EXCHANGE_CAPABILITIES also > reports the number of arguments that the RPC argument struct must have > room for. > > OPTEE_SMC_CALL_WITH_RPC_ARG and OPTEE_SMC_CALL_WITH_ARG can be used > interleaved with difference that when OPTEE_SMC_CALL_WITH_RPC_ARG is > used the RPC argument struct to be used is the one appended to the > normal argument struct. The same is true for > OPTEE_SMC_CALL_WITH_REGD_ARG. > > Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org> > --- > drivers/tee/optee/call.c | 2 +- > drivers/tee/optee/optee_smc.h | 47 ++++++++++++++--- > drivers/tee/optee/smc_abi.c | 99 ++++++++++++++++++++++++++--------- > 3 files changed, 116 insertions(+), 32 deletions(-) >
Apart from minor nits below, feel free to add:
Reviewed-by: Sumit Garg <sumit.garg@linaro.org>
> diff --git a/drivers/tee/optee/call.c b/drivers/tee/optee/call.c > index a9a237d20c61..58ac15c02818 100644 > --- a/drivers/tee/optee/call.c > +++ b/drivers/tee/optee/call.c > @@ -130,7 +130,7 @@ struct tee_shm *optee_get_msg_arg(struct tee_context *ctx, size_t num_params, > return (void *)ma; > } > > - memset(ma, 0, OPTEE_MSG_GET_ARG_SIZE(num_params)); > + memset(ma, 0, sz); > ma->num_params = num_params; > *msg_arg = ma; > > diff --git a/drivers/tee/optee/optee_smc.h b/drivers/tee/optee/optee_smc.h > index d44a6ae994f8..378741a459b6 100644 > --- a/drivers/tee/optee/optee_smc.h > +++ b/drivers/tee/optee/optee_smc.h > @@ -107,14 +107,22 @@ struct optee_smc_call_get_os_revision_result { > /* > * Call with struct optee_msg_arg as argument > * > - * When calling this function normal world has a few responsibilities: > + * When called with OPTEE_SMC_CALL_WITH_RPC_ARG or > + * OPTEE_SMC_CALL_WITH_REGD_ARG in a0 there is one RPC struct optee_msg_arg > + * following after the first struct optee_msg_arg. The RPC struct > + * optee_msg_arg has reserved space for the number of RPC parameters as > + * returned by OPTEE_SMC_EXCHANGE_CAPABILITIES. > + * > + * When calling these functions normal world has a few responsibilities:
s/functions normal/functions, the normal/
> * 1. It must be able to handle eventual RPCs > * 2. Non-secure interrupts should not be masked > * 3. If asynchronous notifications has been negotiated successfully, then > - * asynchronous notifications should be unmasked during this call. > + * the interrupt for asynchronous notifications should be unmasked > + * during this call. > * > - * Call register usage: > - * a0 SMC Function ID, OPTEE_SMC*CALL_WITH_ARG > + * Call register usage, OPTEE_SMC_CALL_WITH_ARG and > + * OPTEE_SMC_CALL_WITH_RPC_ARG: > + * a0 SMC Function ID, OPTEE_SMC_CALL_WITH_ARG or OPTEE_SMC_CALL_WITH_RPC_ARG > * a1 Upper 32 bits of a 64-bit physical pointer to a struct optee_msg_arg > * a2 Lower 32 bits of a 64-bit physical pointer to a struct optee_msg_arg > * a3 Cache settings, not used if physical pointer is in a predefined shared > @@ -122,6 +130,15 @@ struct optee_smc_call_get_os_revision_result { > * a4-6 Not used > * a7 Hypervisor Client ID register > * > + * Call register usage, OPTEE_SMC_CALL_WITH_REGD_ARG: > + * a0 SMC Function ID, OPTEE_SMC_CALL_WITH_REGD_ARG > + * a1 Upper 32 bits of a 64-bit shared memory cookie > + * a2 Lower 32 bits of a 64-bit shared memory cookie > + * a3 Offset of the struct optee_msg_arg in the shared memory with the > + * supplied cookie > + * a4-6 Not used > + * a7 Hypervisor Client ID register > + * > * Normal return register usage: > * a0 Return value, OPTEE_SMC_RETURN_* > * a1-3 Not used > @@ -154,6 +171,10 @@ struct optee_smc_call_get_os_revision_result { > #define OPTEE_SMC_FUNCID_CALL_WITH_ARG OPTEE_MSG_FUNCID_CALL_WITH_ARG > #define OPTEE_SMC_CALL_WITH_ARG \ > OPTEE_SMC_STD_CALL_VAL(OPTEE_SMC_FUNCID_CALL_WITH_ARG) > +#define OPTEE_SMC_CALL_WITH_RPC_ARG \ > + OPTEE_SMC_STD_CALL_VAL(OPTEE_SMC_FUNCID_CALL_WITH_RPC_ARG) > +#define OPTEE_SMC_CALL_WITH_REGD_ARG \ > + OPTEE_SMC_STD_CALL_VAL(OPTEE_SMC_FUNCID_CALL_WITH_REGD_ARG) > > /* > * Get Shared Memory Config > @@ -202,6 +223,10 @@ struct optee_smc_get_shm_config_result { > * a0 OPTEE_SMC_RETURN_OK > * a1 bitfield of secure world capabilities OPTEE_SMC_SEC_CAP_* > * a2 The maximum secure world notification number > + * a3 Bit[7:0]: Number of parameters needed for RPC to be supplied > + * as the second MSG arg struct for > + * OPTEE_SMC_CALL_WITH_ARG > + * Bit[31:8]: Reserved (MBZ) > * a3-7 Preserved
s/a3-7/a4-7/
> * > * Error return register usage: > @@ -215,7 +240,6 @@ struct optee_smc_get_shm_config_result { > #define OPTEE_SMC_SEC_CAP_HAVE_RESERVED_SHM BIT(0) > /* Secure world can communicate via previously unregistered shared memory */ > #define OPTEE_SMC_SEC_CAP_UNREGISTERED_SHM BIT(1) > -
nit: blank line shouldn't be removed.
> /* > * Secure world supports commands "register/unregister shared memory", > * secure world accepts command buffers located in any parts of non-secure RAM > @@ -227,6 +251,8 @@ struct optee_smc_get_shm_config_result { > #define OPTEE_SMC_SEC_CAP_MEMREF_NULL BIT(4) > /* Secure world supports asynchronous notification of normal world */ > #define OPTEE_SMC_SEC_CAP_ASYNC_NOTIF BIT(5) > +/* Secure world supports pre-allocating RPC arg struct */ > +#define OPTEE_SMC_SEC_CAP_RPC_ARG BIT(6) > > #define OPTEE_SMC_FUNCID_EXCHANGE_CAPABILITIES 9 > #define OPTEE_SMC_EXCHANGE_CAPABILITIES \ > @@ -236,7 +262,7 @@ struct optee_smc_exchange_capabilities_result { > unsigned long status; > unsigned long capabilities; > unsigned long max_notif_value; > - unsigned long reserved0; > + unsigned long data; > }; > > /* > @@ -358,6 +384,9 @@ struct optee_smc_disable_shm_cache_result { > * should be called until all pended values have been retrieved. When a > * value is retrieved, it's cleared from the record in secure world. > * > + * It is expected that this function is called from an interrupt handler > + * in normal world. > + * > * Call requests usage: > * a0 SMC Function ID, OPTEE_SMC_GET_ASYNC_NOTIF_VALUE > * a1-6 Not used > @@ -390,6 +419,12 @@ struct optee_smc_disable_shm_cache_result { > #define OPTEE_SMC_GET_ASYNC_NOTIF_VALUE \ > OPTEE_SMC_FAST_CALL_VAL(OPTEE_SMC_FUNCID_GET_ASYNC_NOTIF_VALUE) > > +/* See OPTEE_SMC_CALL_WITH_RPC_ARG above */ > +#define OPTEE_SMC_FUNCID_CALL_WITH_RPC_ARG 18 > + > +/* See OPTEE_SMC_CALL_WITH_REGD_ARG above */ > +#define OPTEE_SMC_FUNCID_CALL_WITH_REGD_ARG 19 > + > /* > * Resume from RPC (for example after processing a foreign interrupt) > * > diff --git a/drivers/tee/optee/smc_abi.c b/drivers/tee/optee/smc_abi.c > index 67b7f7d2ff27..b258d7306042 100644 > --- a/drivers/tee/optee/smc_abi.c > +++ b/drivers/tee/optee/smc_abi.c > @@ -732,16 +732,9 @@ static void optee_rpc_finalize_call(struct optee_call_ctx *call_ctx) > } > > static void handle_rpc_func_cmd(struct tee_context *ctx, struct optee *optee, > - struct tee_shm *shm, > + struct optee_msg_arg *arg, > struct optee_call_ctx *call_ctx) > { > - struct optee_msg_arg *arg; > - > - arg = tee_shm_get_va(shm, 0); > - if (IS_ERR(arg)) { > - pr_err("%s: tee_shm_get_va %p failed\n", __func__, shm); > - return; > - } > > switch (arg->cmd) { > case OPTEE_RPC_CMD_SHM_ALLOC: > @@ -765,11 +758,13 @@ static void handle_rpc_func_cmd(struct tee_context *ctx, struct optee *optee, > * Result of RPC is written back into @param. > */ > static void optee_handle_rpc(struct tee_context *ctx, > + struct optee_msg_arg *rpc_arg, > struct optee_rpc_param *param, > struct optee_call_ctx *call_ctx) > { > struct tee_device *teedev = ctx->teedev; > struct optee *optee = tee_get_drvdata(teedev); > + struct optee_msg_arg *arg; > struct tee_shm *shm; > phys_addr_t pa; > > @@ -801,8 +796,19 @@ static void optee_handle_rpc(struct tee_context *ctx, > */ > break; > case OPTEE_SMC_RPC_FUNC_CMD: > - shm = reg_pair_to_ptr(param->a1, param->a2); > - handle_rpc_func_cmd(ctx, optee, shm, call_ctx); > + if (rpc_arg) { > + arg = rpc_arg; > + } else { > + shm = reg_pair_to_ptr(param->a1, param->a2); > + arg = tee_shm_get_va(shm, 0); > + if (IS_ERR(arg)) { > + pr_err("%s: tee_shm_get_va %p failed\n", > + __func__, shm); > + break; > + } > + } > + > + handle_rpc_func_cmd(ctx, optee, arg, call_ctx); > break; > default: > pr_warn("Unknown RPC func 0x%x\n", > @@ -816,7 +822,7 @@ static void optee_handle_rpc(struct tee_context *ctx, > /** > * optee_smc_do_call_with_arg() - Do an SMC to OP-TEE in secure world > * @ctx: calling context > - * @arg: shared memory holding the message to pass to secure world > + * @shm: shared memory holding the message to pass to secure world > * > * Does and SMC to OP-TEE in secure world and handles eventual resulting > * Remote Procedure Calls (RPC) from OP-TEE. > @@ -824,21 +830,46 @@ static void optee_handle_rpc(struct tee_context *ctx, > * Returns return code from secure world, 0 is OK > */ > static int optee_smc_do_call_with_arg(struct tee_context *ctx, > - struct tee_shm *arg) > + struct tee_shm *shm) > { > struct optee *optee = tee_get_drvdata(ctx->teedev); > struct optee_call_waiter w; > struct optee_rpc_param param = { }; > struct optee_call_ctx call_ctx = { }; > - phys_addr_t parg; > + struct optee_msg_arg *rpc_arg = NULL; > int rc; > > - rc = tee_shm_get_pa(arg, 0, &parg); > - if (rc) > - return rc; > + if (optee->rpc_param_count) { > + struct optee_msg_arg *arg; > + unsigned int rpc_arg_offs; > + > + arg = tee_shm_get_va(shm, 0); > + if (IS_ERR(arg)) > + return PTR_ERR(arg); > > - param.a0 = OPTEE_SMC_CALL_WITH_ARG; > - reg_pair_from_64(¶m.a1, ¶m.a2, parg); > + rpc_arg_offs = OPTEE_MSG_GET_ARG_SIZE(arg->num_params); > + rpc_arg = tee_shm_get_va(shm, rpc_arg_offs); > + if (IS_ERR(arg)) > + return PTR_ERR(arg); > + } > + > + if (rpc_arg && tee_shm_is_dynamic(shm)) { > + param.a0 = OPTEE_SMC_CALL_WITH_REGD_ARG; > + reg_pair_from_64(¶m.a1, ¶m.a2, (u_long)shm); > + param.a3 = 0; > + } else { > + phys_addr_t parg; > + > + rc = tee_shm_get_pa(shm, 0, &parg); > + if (rc) > + return rc; > + > + if (rpc_arg) > + param.a0 = OPTEE_SMC_CALL_WITH_RPC_ARG; > + else > + param.a0 = OPTEE_SMC_CALL_WITH_ARG; > + reg_pair_from_64(¶m.a1, ¶m.a2, parg); > + } > /* Initialize waiter */ > optee_cq_wait_init(&optee->call_queue, &w); > while (true) { > @@ -862,7 +893,7 @@ static int optee_smc_do_call_with_arg(struct tee_context *ctx, > param.a1 = res.a1; > param.a2 = res.a2; > param.a3 = res.a3; > - optee_handle_rpc(ctx, ¶m, &call_ctx); > + optee_handle_rpc(ctx, rpc_arg, ¶m, &call_ctx); > } else { > rc = res.a0; > break; > @@ -1118,7 +1149,8 @@ static bool optee_msg_api_revision_is_compatible(optee_invoke_fn *invoke_fn) > } > > static bool optee_msg_exchange_capabilities(optee_invoke_fn *invoke_fn, > - u32 *sec_caps, u32 *max_notif_value) > + u32 *sec_caps, u32 *max_notif_value, > + unsigned int *rpc_param_count) > { > union { > struct arm_smccc_res smccc; > @@ -1145,6 +1177,10 @@ static bool optee_msg_exchange_capabilities(optee_invoke_fn *invoke_fn, > *max_notif_value = res.result.max_notif_value; > else > *max_notif_value = OPTEE_DEFAULT_MAX_NOTIF_VALUE; > + if (*sec_caps & OPTEE_SMC_SEC_CAP_RPC_ARG) > + *rpc_param_count = (u8)res.result.data; > + else > + *rpc_param_count = 0; > > return true; > } > @@ -1251,7 +1287,8 @@ static int optee_smc_remove(struct platform_device *pdev) > * reference counters and also avoid wild pointers in secure world > * into the old shared memory range. > */ > - optee_disable_shm_cache(optee); > + if (!optee->rpc_param_count) > + optee_disable_shm_cache(optee); > > optee_smc_notif_uninit_irq(optee); > > @@ -1274,7 +1311,10 @@ static int optee_smc_remove(struct platform_device *pdev) > */ > static void optee_shutdown(struct platform_device *pdev) > { > - optee_disable_shm_cache(platform_get_drvdata(pdev)); > + struct optee *optee = platform_get_drvdata(pdev); > + > + if (!optee->rpc_param_count) > + optee_disable_shm_cache(optee); > } > > static int optee_probe(struct platform_device *pdev) > @@ -1283,6 +1323,7 @@ static int optee_probe(struct platform_device *pdev) > struct tee_shm_pool *pool = ERR_PTR(-EINVAL); > struct optee *optee = NULL; > void *memremaped_shm = NULL; > + unsigned int rpc_param_count; > struct tee_device *teedev; > struct tee_context *ctx; > u32 max_notif_value; > @@ -1306,7 +1347,8 @@ static int optee_probe(struct platform_device *pdev) > } > > if (!optee_msg_exchange_capabilities(invoke_fn, &sec_caps, > - &max_notif_value)) { > + &max_notif_value, > + &rpc_param_count)) { > pr_warn("capabilities mismatch\n"); > return -EINVAL; > } > @@ -1335,6 +1377,7 @@ static int optee_probe(struct platform_device *pdev) > optee->ops = &optee_ops; > optee->smc.invoke_fn = invoke_fn; > optee->smc.sec_caps = sec_caps; > + optee->rpc_param_count = rpc_param_count; > > teedev = tee_device_alloc(&optee_clnt_desc, NULL, pool, optee); > if (IS_ERR(teedev)) { > @@ -1403,7 +1446,12 @@ static int optee_probe(struct platform_device *pdev) > */ > optee_disable_unmapped_shm_cache(optee); > > - optee_enable_shm_cache(optee); > + /* > + * Only enable the shm cache in case we're not able to pass the RPC > + * arg struct right after the normal arg struct. > + */ > + if (!optee->rpc_param_count) > + optee_enable_shm_cache(optee); > > if (optee->smc.sec_caps & OPTEE_SMC_SEC_CAP_DYNAMIC_SHM) > pr_info("dynamic shared memory is enabled\n"); > @@ -1416,7 +1464,8 @@ static int optee_probe(struct platform_device *pdev) > return 0; > > err_disable_shm_cache: > - optee_disable_shm_cache(optee); > + if (!optee->rpc_param_count) > + optee_disable_shm_cache(optee); > optee_smc_notif_uninit_irq(optee); > optee_unregister_devices(); > err_notif_uninit: > -- > 2.31.1 >
| |