[PATCH 0/3] OP-TEE RPC argument cache

[PATCH 0/3] OP-TEE RPC argument cache

[Jens Wiklander]

Hi all,

This patchset optimizes handling of the argument struction passed to
call_with_arg when doing a yielding call to OP-TEE.

Prior to this was this structure allocated before the yielding call and
then freed after it had returned. In case many calls are made in succession
this results in quite a bit of unncesary allocte/free and possibly also
switching back and forth to secure work in order to register if needed.

Another optimization handles the way the argument struct needed to do RPC
is passed. Please see the patch "optee: add OPTEE_SMC_CALL_WITH_RPC_ARG"
for details.

This patchset is based the next branch [1] in my kernel to avoid conflict
with other recent patches.

Thanks,
Jens

[1] https://git.linaro.org/people/jens.wiklander/linux-tee.git/log/?h=next

Jens Wiklander (3):
  optee: add OPTEE_SMC_CALL_WITH_RPC_ARG
  optee: add FF-A capability OPTEE_FFA_SEC_CAP_ARG_OFFSET
  optee: cache argument shared memory structs

 drivers/tee/optee/call.c          | 238 ++++++++++++++++++++++++------
 drivers/tee/optee/core.c          |   1 +
 drivers/tee/optee/ffa_abi.c       |  36 +++--
 drivers/tee/optee/optee_ffa.h     |  12 +-
 drivers/tee/optee/optee_private.h |  31 +++-
 drivers/tee/optee/optee_smc.h     |  47 +++++-
 drivers/tee/optee/smc_abi.c       | 151 +++++++++++++++----
 7 files changed, 419 insertions(+), 97 deletions(-)

-- 
2.31.1

[PATCH 1/3] optee: add OPTEE_SMC_CALL_WITH_RPC_ARG

[Jens Wiklander]

Adds OPTEE_SMC_CALL_WITH_RPC_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.

The presence OPTEE_SMC_CALL_WITH_RPC_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.

Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
---
 drivers/tee/optee/call.c          |  6 +--
 drivers/tee/optee/ffa_abi.c       | 10 ++--
 drivers/tee/optee/optee_private.h |  4 +-
 drivers/tee/optee/optee_smc.h     | 47 ++++++++++++++---
 drivers/tee/optee/smc_abi.c       | 88 +++++++++++++++++++++++--------
 5 files changed, 117 insertions(+), 38 deletions(-)

diff --git a/drivers/tee/optee/call.c b/drivers/tee/optee/call.c
index bd49ec934060..82d836df6922 100644
--- a/drivers/tee/optee/call.c
+++ b/drivers/tee/optee/call.c
@@ -117,8 +117,8 @@ struct tee_shm *optee_get_msg_arg(struct tee_context *ctx, size_t num_params,
 	 * the RPC argument struct if a second MSG arg struct is expected.
 	 * The second arg struct will then be used for RPC.
 	 */
-	if (optee->rpc_arg_count)
-		sz += OPTEE_MSG_GET_ARG_SIZE(optee->rpc_arg_count);
+	if (optee->rpc_param_count)
+		sz += OPTEE_MSG_GET_ARG_SIZE(optee->rpc_param_count);
 
 	shm = tee_shm_alloc_priv_buf(ctx, sz);
 	if (IS_ERR(shm))
@@ -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/ffa_abi.c b/drivers/tee/optee/ffa_abi.c
index a5eb4ef46971..7686f7020616 100644
--- a/drivers/tee/optee/ffa_abi.c
+++ b/drivers/tee/optee/ffa_abi.c
@@ -678,7 +678,7 @@ static bool optee_ffa_api_is_compatbile(struct ffa_device *ffa_dev,
 
 static bool optee_ffa_exchange_caps(struct ffa_device *ffa_dev,
 				    const struct ffa_dev_ops *ops,
-				    unsigned int *rpc_arg_count)
+				    unsigned int *rpc_param_count)
 {
 	struct ffa_send_direct_data data = { OPTEE_FFA_EXCHANGE_CAPABILITIES };
 	int rc;
@@ -693,7 +693,7 @@ static bool optee_ffa_exchange_caps(struct ffa_device *ffa_dev,
 		return false;
 	}
 
-	*rpc_arg_count = (u8)data.data1;
+	*rpc_param_count = (u8)data.data1;
 
 	return true;
 }
@@ -772,7 +772,7 @@ static void optee_ffa_remove(struct ffa_device *ffa_dev)
 static int optee_ffa_probe(struct ffa_device *ffa_dev)
 {
 	const struct ffa_dev_ops *ffa_ops;
-	unsigned int rpc_arg_count;
+	unsigned int rpc_param_count;
 	struct tee_shm_pool *pool;
 	struct tee_device *teedev;
 	struct tee_context *ctx;
@@ -788,7 +788,7 @@ static int optee_ffa_probe(struct ffa_device *ffa_dev)
 	if (!optee_ffa_api_is_compatbile(ffa_dev, ffa_ops))
 		return -EINVAL;
 
-	if (!optee_ffa_exchange_caps(ffa_dev, ffa_ops, &rpc_arg_count))
+	if (!optee_ffa_exchange_caps(ffa_dev, ffa_ops, &rpc_param_count))
 		return -EINVAL;
 
 	optee = kzalloc(sizeof(*optee), GFP_KERNEL);
@@ -805,7 +805,7 @@ static int optee_ffa_probe(struct ffa_device *ffa_dev)
 	optee->ops = &optee_ffa_ops;
 	optee->ffa.ffa_dev = ffa_dev;
 	optee->ffa.ffa_ops = ffa_ops;
-	optee->rpc_arg_count = rpc_arg_count;
+	optee->rpc_param_count = rpc_param_count;
 
 	teedev = tee_device_alloc(&optee_ffa_clnt_desc, NULL, optee->pool,
 				  optee);
diff --git a/drivers/tee/optee/optee_private.h b/drivers/tee/optee/optee_private.h
index e77765c78878..e80c5d9b62ec 100644
--- a/drivers/tee/optee/optee_private.h
+++ b/drivers/tee/optee/optee_private.h
@@ -143,7 +143,7 @@ struct optee_ops {
  * @notif:		notification synchronization struct
  * @supp:		supplicant synchronization struct for RPC to supplicant
  * @pool:		shared memory pool
- * @rpc_arg_count:	If > 0 number of RPC parameters to make room for
+ * @rpc_param_count:	If > 0 number of RPC parameters to make room for
  * @scan_bus_done	flag if device registation was already done.
  * @scan_bus_wq		workqueue to scan optee bus and register optee drivers
  * @scan_bus_work	workq to scan optee bus and register optee drivers
@@ -161,7 +161,7 @@ struct optee {
 	struct optee_notif notif;
 	struct optee_supp supp;
 	struct tee_shm_pool *pool;
-	unsigned int rpc_arg_count;
+	unsigned int rpc_param_count;
 	bool   scan_bus_done;
 	struct workqueue_struct *scan_bus_wq;
 	struct work_struct scan_bus_work;
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:
  * 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
  *
  * 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)
-
 /*
  * 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..c45cc9b7e5a7 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;
 
-	param.a0 = OPTEE_SMC_CALL_WITH_ARG;
-	reg_pair_from_64(&param.a1, &param.a2, parg);
+		arg = tee_shm_get_va(shm, 0);
+		if (IS_ERR(arg))
+			return PTR_ERR(arg);
+
+		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(&param.a1, &param.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(&param.a1, &param.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, &param, &call_ctx);
+			optee_handle_rpc(ctx, rpc_arg, &param, &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;
 }
@@ -1283,6 +1319,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 +1343,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 +1373,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 +1442,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");
-- 
2.31.1

Re: [PATCH 1/3] optee: add OPTEE_SMC_CALL_WITH_RPC_ARG

[Sumit Garg]

Hi Jens,

Firstly apologies for the late review as this patch-set was in my
queue but got distracted to other high priority activities.

On Wed, 2 Mar 2022 at 01:18, Jens Wiklander <jens.wiklander@linaro.org> wrote:
>
> Adds OPTEE_SMC_CALL_WITH_RPC_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.
>
> The presence OPTEE_SMC_CALL_WITH_RPC_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.
>
> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> ---
>  drivers/tee/optee/call.c          |  6 +--
>  drivers/tee/optee/ffa_abi.c       | 10 ++--
>  drivers/tee/optee/optee_private.h |  4 +-
>  drivers/tee/optee/optee_smc.h     | 47 ++++++++++++++---
>  drivers/tee/optee/smc_abi.c       | 88 +++++++++++++++++++++++--------
>  5 files changed, 117 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/tee/optee/call.c b/drivers/tee/optee/call.c
> index bd49ec934060..82d836df6922 100644
> --- a/drivers/tee/optee/call.c
> +++ b/drivers/tee/optee/call.c
> @@ -117,8 +117,8 @@ struct tee_shm *optee_get_msg_arg(struct tee_context *ctx, size_t num_params,
>          * the RPC argument struct if a second MSG arg struct is expected.
>          * The second arg struct will then be used for RPC.
>          */
> -       if (optee->rpc_arg_count)
> -               sz += OPTEE_MSG_GET_ARG_SIZE(optee->rpc_arg_count);
> +       if (optee->rpc_param_count)
> +               sz += OPTEE_MSG_GET_ARG_SIZE(optee->rpc_param_count);

nit: I see this renaming as part of this new feature patch-set. Can we
split it as a separate patch which would be helpful for review?

>
>         shm = tee_shm_alloc_priv_buf(ctx, sz);
>         if (IS_ERR(shm))
> @@ -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/ffa_abi.c b/drivers/tee/optee/ffa_abi.c
> index a5eb4ef46971..7686f7020616 100644
> --- a/drivers/tee/optee/ffa_abi.c
> +++ b/drivers/tee/optee/ffa_abi.c
> @@ -678,7 +678,7 @@ static bool optee_ffa_api_is_compatbile(struct ffa_device *ffa_dev,
>
>  static bool optee_ffa_exchange_caps(struct ffa_device *ffa_dev,
>                                     const struct ffa_dev_ops *ops,
> -                                   unsigned int *rpc_arg_count)
> +                                   unsigned int *rpc_param_count)
>  {
>         struct ffa_send_direct_data data = { OPTEE_FFA_EXCHANGE_CAPABILITIES };
>         int rc;
> @@ -693,7 +693,7 @@ static bool optee_ffa_exchange_caps(struct ffa_device *ffa_dev,
>                 return false;
>         }
>
> -       *rpc_arg_count = (u8)data.data1;
> +       *rpc_param_count = (u8)data.data1;
>
>         return true;
>  }
> @@ -772,7 +772,7 @@ static void optee_ffa_remove(struct ffa_device *ffa_dev)
>  static int optee_ffa_probe(struct ffa_device *ffa_dev)
>  {
>         const struct ffa_dev_ops *ffa_ops;
> -       unsigned int rpc_arg_count;
> +       unsigned int rpc_param_count;
>         struct tee_shm_pool *pool;
>         struct tee_device *teedev;
>         struct tee_context *ctx;
> @@ -788,7 +788,7 @@ static int optee_ffa_probe(struct ffa_device *ffa_dev)
>         if (!optee_ffa_api_is_compatbile(ffa_dev, ffa_ops))
>                 return -EINVAL;
>
> -       if (!optee_ffa_exchange_caps(ffa_dev, ffa_ops, &rpc_arg_count))
> +       if (!optee_ffa_exchange_caps(ffa_dev, ffa_ops, &rpc_param_count))
>                 return -EINVAL;
>
>         optee = kzalloc(sizeof(*optee), GFP_KERNEL);
> @@ -805,7 +805,7 @@ static int optee_ffa_probe(struct ffa_device *ffa_dev)
>         optee->ops = &optee_ffa_ops;
>         optee->ffa.ffa_dev = ffa_dev;
>         optee->ffa.ffa_ops = ffa_ops;
> -       optee->rpc_arg_count = rpc_arg_count;
> +       optee->rpc_param_count = rpc_param_count;
>
>         teedev = tee_device_alloc(&optee_ffa_clnt_desc, NULL, optee->pool,
>                                   optee);
> diff --git a/drivers/tee/optee/optee_private.h b/drivers/tee/optee/optee_private.h
> index e77765c78878..e80c5d9b62ec 100644
> --- a/drivers/tee/optee/optee_private.h
> +++ b/drivers/tee/optee/optee_private.h
> @@ -143,7 +143,7 @@ struct optee_ops {
>   * @notif:             notification synchronization struct
>   * @supp:              supplicant synchronization struct for RPC to supplicant
>   * @pool:              shared memory pool
> - * @rpc_arg_count:     If > 0 number of RPC parameters to make room for
> + * @rpc_param_count:   If > 0 number of RPC parameters to make room for
>   * @scan_bus_done      flag if device registation was already done.
>   * @scan_bus_wq                workqueue to scan optee bus and register optee drivers
>   * @scan_bus_work      workq to scan optee bus and register optee drivers
> @@ -161,7 +161,7 @@ struct optee {
>         struct optee_notif notif;
>         struct optee_supp supp;
>         struct tee_shm_pool *pool;
> -       unsigned int rpc_arg_count;
> +       unsigned int rpc_param_count;
>         bool   scan_bus_done;
>         struct workqueue_struct *scan_bus_wq;
>         struct work_struct scan_bus_work;
> 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:
>   * 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:

Although I didn't see any reference to OPTEE_SMC_CALL_WITH_REGD_ARG in
the commit message, but do we really need to introduce it? Wouldn't it
be possible to just pass additional "shared memory cookie" value as
part of "Not used" (a4-6) arguments?

> + * 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
>   *
>   * 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)
> -
>  /*
>   * 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..c45cc9b7e5a7 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;
>
> -       param.a0 = OPTEE_SMC_CALL_WITH_ARG;
> -       reg_pair_from_64(&param.a1, &param.a2, parg);
> +               arg = tee_shm_get_va(shm, 0);
> +               if (IS_ERR(arg))
> +                       return PTR_ERR(arg);
> +
> +               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(&param.a1, &param.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(&param.a1, &param.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, &param, &call_ctx);
> +                       optee_handle_rpc(ctx, rpc_arg, &param, &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;
>  }
> @@ -1283,6 +1319,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 +1343,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 +1373,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 +1442,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);
>

I think a similar check is required for redundant
optee_disable_shm_cache() and optee_disable_unmapped_shm_cache().

-Sumit

>         if (optee->smc.sec_caps & OPTEE_SMC_SEC_CAP_DYNAMIC_SHM)
>                 pr_info("dynamic shared memory is enabled\n");
> --
> 2.31.1
>

Re: [PATCH 1/3] optee: add OPTEE_SMC_CALL_WITH_RPC_ARG

[Jens Wiklander]

Hi Sumit,

On Mon, Mar 14, 2022 at 12:00:25PM +0530, Sumit Garg wrote:
> Hi Jens,
> 
> Firstly apologies for the late review as this patch-set was in my
> queue but got distracted to other high priority activities.

No worries, your comments are very welcome.

> 
> On Wed, 2 Mar 2022 at 01:18, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> >
> > Adds OPTEE_SMC_CALL_WITH_RPC_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.
> >
> > The presence OPTEE_SMC_CALL_WITH_RPC_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.
> >
> > Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> > ---
> >  drivers/tee/optee/call.c          |  6 +--
> >  drivers/tee/optee/ffa_abi.c       | 10 ++--
> >  drivers/tee/optee/optee_private.h |  4 +-
> >  drivers/tee/optee/optee_smc.h     | 47 ++++++++++++++---
> >  drivers/tee/optee/smc_abi.c       | 88 +++++++++++++++++++++++--------
> >  5 files changed, 117 insertions(+), 38 deletions(-)
> >
> > diff --git a/drivers/tee/optee/call.c b/drivers/tee/optee/call.c
> > index bd49ec934060..82d836df6922 100644
> > --- a/drivers/tee/optee/call.c
> > +++ b/drivers/tee/optee/call.c
> > @@ -117,8 +117,8 @@ struct tee_shm *optee_get_msg_arg(struct tee_context *ctx, size_t num_params,
> >          * the RPC argument struct if a second MSG arg struct is expected.
> >          * The second arg struct will then be used for RPC.
> >          */
> > -       if (optee->rpc_arg_count)
> > -               sz += OPTEE_MSG_GET_ARG_SIZE(optee->rpc_arg_count);
> > +       if (optee->rpc_param_count)
> > +               sz += OPTEE_MSG_GET_ARG_SIZE(optee->rpc_param_count);
> 
> nit: I see this renaming as part of this new feature patch-set. Can we
> split it as a separate patch which would be helpful for review?

OK, I'll separate that in the v2.

> 
> >
> >         shm = tee_shm_alloc_priv_buf(ctx, sz);
> >         if (IS_ERR(shm))
> > @@ -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/ffa_abi.c b/drivers/tee/optee/ffa_abi.c
> > index a5eb4ef46971..7686f7020616 100644
> > --- a/drivers/tee/optee/ffa_abi.c
> > +++ b/drivers/tee/optee/ffa_abi.c
> > @@ -678,7 +678,7 @@ static bool optee_ffa_api_is_compatbile(struct ffa_device *ffa_dev,
> >
> >  static bool optee_ffa_exchange_caps(struct ffa_device *ffa_dev,
> >                                     const struct ffa_dev_ops *ops,
> > -                                   unsigned int *rpc_arg_count)
> > +                                   unsigned int *rpc_param_count)
> >  {
> >         struct ffa_send_direct_data data = { OPTEE_FFA_EXCHANGE_CAPABILITIES };
> >         int rc;
> > @@ -693,7 +693,7 @@ static bool optee_ffa_exchange_caps(struct ffa_device *ffa_dev,
> >                 return false;
> >         }
> >
> > -       *rpc_arg_count = (u8)data.data1;
> > +       *rpc_param_count = (u8)data.data1;
> >
> >         return true;
> >  }
> > @@ -772,7 +772,7 @@ static void optee_ffa_remove(struct ffa_device *ffa_dev)
> >  static int optee_ffa_probe(struct ffa_device *ffa_dev)
> >  {
> >         const struct ffa_dev_ops *ffa_ops;
> > -       unsigned int rpc_arg_count;
> > +       unsigned int rpc_param_count;
> >         struct tee_shm_pool *pool;
> >         struct tee_device *teedev;
> >         struct tee_context *ctx;
> > @@ -788,7 +788,7 @@ static int optee_ffa_probe(struct ffa_device *ffa_dev)
> >         if (!optee_ffa_api_is_compatbile(ffa_dev, ffa_ops))
> >                 return -EINVAL;
> >
> > -       if (!optee_ffa_exchange_caps(ffa_dev, ffa_ops, &rpc_arg_count))
> > +       if (!optee_ffa_exchange_caps(ffa_dev, ffa_ops, &rpc_param_count))
> >                 return -EINVAL;
> >
> >         optee = kzalloc(sizeof(*optee), GFP_KERNEL);
> > @@ -805,7 +805,7 @@ static int optee_ffa_probe(struct ffa_device *ffa_dev)
> >         optee->ops = &optee_ffa_ops;
> >         optee->ffa.ffa_dev = ffa_dev;
> >         optee->ffa.ffa_ops = ffa_ops;
> > -       optee->rpc_arg_count = rpc_arg_count;
> > +       optee->rpc_param_count = rpc_param_count;
> >
> >         teedev = tee_device_alloc(&optee_ffa_clnt_desc, NULL, optee->pool,
> >                                   optee);
> > diff --git a/drivers/tee/optee/optee_private.h b/drivers/tee/optee/optee_private.h
> > index e77765c78878..e80c5d9b62ec 100644
> > --- a/drivers/tee/optee/optee_private.h
> > +++ b/drivers/tee/optee/optee_private.h
> > @@ -143,7 +143,7 @@ struct optee_ops {
> >   * @notif:             notification synchronization struct
> >   * @supp:              supplicant synchronization struct for RPC to supplicant
> >   * @pool:              shared memory pool
> > - * @rpc_arg_count:     If > 0 number of RPC parameters to make room for
> > + * @rpc_param_count:   If > 0 number of RPC parameters to make room for
> >   * @scan_bus_done      flag if device registation was already done.
> >   * @scan_bus_wq                workqueue to scan optee bus and register optee drivers
> >   * @scan_bus_work      workq to scan optee bus and register optee drivers
> > @@ -161,7 +161,7 @@ struct optee {
> >         struct optee_notif notif;
> >         struct optee_supp supp;
> >         struct tee_shm_pool *pool;
> > -       unsigned int rpc_arg_count;
> > +       unsigned int rpc_param_count;
> >         bool   scan_bus_done;
> >         struct workqueue_struct *scan_bus_wq;
> >         struct work_struct scan_bus_work;
> > 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:
> >   * 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:
> 
> Although I didn't see any reference to OPTEE_SMC_CALL_WITH_REGD_ARG in
> the commit message, but do we really need to introduce it? Wouldn't it
> be possible to just pass additional "shared memory cookie" value as
> part of "Not used" (a4-6) arguments?

I'll update the commit message to mention OPTEE_SMC_CALL_WITH_REGD_ARG
too. I think it's more clear with a separate ID for this, less risk of
confusion. How would the callee know if it's the cookie or the physical
address it should use?  Whatever we do, we're extenting the ABI.

> 
> > + * 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
> >   *
> >   * 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)
> > -
> >  /*
> >   * 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..c45cc9b7e5a7 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;
> >
> > -       param.a0 = OPTEE_SMC_CALL_WITH_ARG;
> > -       reg_pair_from_64(&param.a1, &param.a2, parg);
> > +               arg = tee_shm_get_va(shm, 0);
> > +               if (IS_ERR(arg))
> > +                       return PTR_ERR(arg);
> > +
> > +               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(&param.a1, &param.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(&param.a1, &param.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, &param, &call_ctx);
> > +                       optee_handle_rpc(ctx, rpc_arg, &param, &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;
> >  }
> > @@ -1283,6 +1319,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 +1343,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 +1373,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 +1442,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);
> >
> 
> I think a similar check is required for redundant
> optee_disable_shm_cache() and optee_disable_unmapped_shm_cache().

I believe optee_disable_unmapped_shm_cache() should be done
unconditionally as it cleans up from previous a instance of an OP-TEE
driver and who knows that state that was in.

The call to optee_disable_shm_cache() is harmless, but you're right it's
unnecesary so we should skip it if not needed. I'll fix.

Thanks,
Jens

> 
> -Sumit
> 
> >         if (optee->smc.sec_caps & OPTEE_SMC_SEC_CAP_DYNAMIC_SHM)
> >                 pr_info("dynamic shared memory is enabled\n");
> > --
> > 2.31.1
> >

Re: [PATCH 1/3] optee: add OPTEE_SMC_CALL_WITH_RPC_ARG

[Sumit Garg]

On Wed, 16 Mar 2022 at 13:47, Jens Wiklander <jens.wiklander@linaro.org> wrote:
>
> Hi Sumit,
>
> On Mon, Mar 14, 2022 at 12:00:25PM +0530, Sumit Garg wrote:
> > Hi Jens,
> >
> > Firstly apologies for the late review as this patch-set was in my
> > queue but got distracted to other high priority activities.
>
> No worries, your comments are very welcome.
>
> >
> > On Wed, 2 Mar 2022 at 01:18, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> > >
> > > Adds OPTEE_SMC_CALL_WITH_RPC_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.
> > >
> > > The presence OPTEE_SMC_CALL_WITH_RPC_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.
> > >
> > > Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> > > ---
> > >  drivers/tee/optee/call.c          |  6 +--
> > >  drivers/tee/optee/ffa_abi.c       | 10 ++--
> > >  drivers/tee/optee/optee_private.h |  4 +-
> > >  drivers/tee/optee/optee_smc.h     | 47 ++++++++++++++---
> > >  drivers/tee/optee/smc_abi.c       | 88 +++++++++++++++++++++++--------
> > >  5 files changed, 117 insertions(+), 38 deletions(-)
> > >
> > > diff --git a/drivers/tee/optee/call.c b/drivers/tee/optee/call.c
> > > index bd49ec934060..82d836df6922 100644
> > > --- a/drivers/tee/optee/call.c
> > > +++ b/drivers/tee/optee/call.c
> > > @@ -117,8 +117,8 @@ struct tee_shm *optee_get_msg_arg(struct tee_context *ctx, size_t num_params,
> > >          * the RPC argument struct if a second MSG arg struct is expected.
> > >          * The second arg struct will then be used for RPC.
> > >          */
> > > -       if (optee->rpc_arg_count)
> > > -               sz += OPTEE_MSG_GET_ARG_SIZE(optee->rpc_arg_count);
> > > +       if (optee->rpc_param_count)
> > > +               sz += OPTEE_MSG_GET_ARG_SIZE(optee->rpc_param_count);
> >
> > nit: I see this renaming as part of this new feature patch-set. Can we
> > split it as a separate patch which would be helpful for review?
>
> OK, I'll separate that in the v2.
>
> >
> > >
> > >         shm = tee_shm_alloc_priv_buf(ctx, sz);
> > >         if (IS_ERR(shm))
> > > @@ -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/ffa_abi.c b/drivers/tee/optee/ffa_abi.c
> > > index a5eb4ef46971..7686f7020616 100644
> > > --- a/drivers/tee/optee/ffa_abi.c
> > > +++ b/drivers/tee/optee/ffa_abi.c
> > > @@ -678,7 +678,7 @@ static bool optee_ffa_api_is_compatbile(struct ffa_device *ffa_dev,
> > >
> > >  static bool optee_ffa_exchange_caps(struct ffa_device *ffa_dev,
> > >                                     const struct ffa_dev_ops *ops,
> > > -                                   unsigned int *rpc_arg_count)
> > > +                                   unsigned int *rpc_param_count)
> > >  {
> > >         struct ffa_send_direct_data data = { OPTEE_FFA_EXCHANGE_CAPABILITIES };
> > >         int rc;
> > > @@ -693,7 +693,7 @@ static bool optee_ffa_exchange_caps(struct ffa_device *ffa_dev,
> > >                 return false;
> > >         }
> > >
> > > -       *rpc_arg_count = (u8)data.data1;
> > > +       *rpc_param_count = (u8)data.data1;
> > >
> > >         return true;
> > >  }
> > > @@ -772,7 +772,7 @@ static void optee_ffa_remove(struct ffa_device *ffa_dev)
> > >  static int optee_ffa_probe(struct ffa_device *ffa_dev)
> > >  {
> > >         const struct ffa_dev_ops *ffa_ops;
> > > -       unsigned int rpc_arg_count;
> > > +       unsigned int rpc_param_count;
> > >         struct tee_shm_pool *pool;
> > >         struct tee_device *teedev;
> > >         struct tee_context *ctx;
> > > @@ -788,7 +788,7 @@ static int optee_ffa_probe(struct ffa_device *ffa_dev)
> > >         if (!optee_ffa_api_is_compatbile(ffa_dev, ffa_ops))
> > >                 return -EINVAL;
> > >
> > > -       if (!optee_ffa_exchange_caps(ffa_dev, ffa_ops, &rpc_arg_count))
> > > +       if (!optee_ffa_exchange_caps(ffa_dev, ffa_ops, &rpc_param_count))
> > >                 return -EINVAL;
> > >
> > >         optee = kzalloc(sizeof(*optee), GFP_KERNEL);
> > > @@ -805,7 +805,7 @@ static int optee_ffa_probe(struct ffa_device *ffa_dev)
> > >         optee->ops = &optee_ffa_ops;
> > >         optee->ffa.ffa_dev = ffa_dev;
> > >         optee->ffa.ffa_ops = ffa_ops;
> > > -       optee->rpc_arg_count = rpc_arg_count;
> > > +       optee->rpc_param_count = rpc_param_count;
> > >
> > >         teedev = tee_device_alloc(&optee_ffa_clnt_desc, NULL, optee->pool,
> > >                                   optee);
> > > diff --git a/drivers/tee/optee/optee_private.h b/drivers/tee/optee/optee_private.h
> > > index e77765c78878..e80c5d9b62ec 100644
> > > --- a/drivers/tee/optee/optee_private.h
> > > +++ b/drivers/tee/optee/optee_private.h
> > > @@ -143,7 +143,7 @@ struct optee_ops {
> > >   * @notif:             notification synchronization struct
> > >   * @supp:              supplicant synchronization struct for RPC to supplicant
> > >   * @pool:              shared memory pool
> > > - * @rpc_arg_count:     If > 0 number of RPC parameters to make room for
> > > + * @rpc_param_count:   If > 0 number of RPC parameters to make room for
> > >   * @scan_bus_done      flag if device registation was already done.
> > >   * @scan_bus_wq                workqueue to scan optee bus and register optee drivers
> > >   * @scan_bus_work      workq to scan optee bus and register optee drivers
> > > @@ -161,7 +161,7 @@ struct optee {
> > >         struct optee_notif notif;
> > >         struct optee_supp supp;
> > >         struct tee_shm_pool *pool;
> > > -       unsigned int rpc_arg_count;
> > > +       unsigned int rpc_param_count;
> > >         bool   scan_bus_done;
> > >         struct workqueue_struct *scan_bus_wq;
> > >         struct work_struct scan_bus_work;
> > > 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:
> > >   * 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:
> >
> > Although I didn't see any reference to OPTEE_SMC_CALL_WITH_REGD_ARG in
> > the commit message, but do we really need to introduce it? Wouldn't it
> > be possible to just pass additional "shared memory cookie" value as
> > part of "Not used" (a4-6) arguments?
>
> I'll update the commit message to mention OPTEE_SMC_CALL_WITH_REGD_ARG
> too. I think it's more clear with a separate ID for this, less risk of
> confusion.

IMO, it would unnecessarily complicate and introduce ambiguity in the
ABI as after this patch we will have:

CALL_WITH_ARG: Standard arguments *without* pre-allocated RPC arguments buffer
CALL_WITH_RPC_ARG: Standard arguments *with* pre-allocated RPC arguments buffer
CALL_WITH_REGD_ARG: Registered arguments but *not* explicit whether
RPC arguments buffer is there or not.

If we keep the ABI simplified to say we only support two types of
invocation irrespective of whether the arguments are allocated from
statically shared memory or dynamically shared memory:

CALL_WITH_ARG: Standard arguments *without* pre-allocated RPC arguments buffer
CALL_WITH_RPC_ARG: Standard arguments *with* pre-allocated RPC arguments buffer

> How would the callee know if it's the cookie or the physical
> address it should use?  Whatever we do, we're extenting the ABI.
>

Isn't it possible for OP-TEE to determine if it's a valid cookie or
not which will be passed into currently unused arguments?

-Sumit

> >
> > > + * 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
> > >   *
> > >   * 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)
> > > -
> > >  /*
> > >   * 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..c45cc9b7e5a7 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;
> > >
> > > -       param.a0 = OPTEE_SMC_CALL_WITH_ARG;
> > > -       reg_pair_from_64(&param.a1, &param.a2, parg);
> > > +               arg = tee_shm_get_va(shm, 0);
> > > +               if (IS_ERR(arg))
> > > +                       return PTR_ERR(arg);
> > > +
> > > +               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(&param.a1, &param.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(&param.a1, &param.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, &param, &call_ctx);
> > > +                       optee_handle_rpc(ctx, rpc_arg, &param, &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;
> > >  }
> > > @@ -1283,6 +1319,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 +1343,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 +1373,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 +1442,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);
> > >
> >
> > I think a similar check is required for redundant
> > optee_disable_shm_cache() and optee_disable_unmapped_shm_cache().
>
> I believe optee_disable_unmapped_shm_cache() should be done
> unconditionally as it cleans up from previous a instance of an OP-TEE
> driver and who knows that state that was in.
>
> The call to optee_disable_shm_cache() is harmless, but you're right it's
> unnecesary so we should skip it if not needed. I'll fix.
>
> Thanks,
> Jens
>
> >
> > -Sumit
> >
> > >         if (optee->smc.sec_caps & OPTEE_SMC_SEC_CAP_DYNAMIC_SHM)
> > >                 pr_info("dynamic shared memory is enabled\n");
> > > --
> > > 2.31.1
> > >

Re: [PATCH 1/3] optee: add OPTEE_SMC_CALL_WITH_RPC_ARG

[Jens Wiklander]

On Thu, Mar 17, 2022 at 12:40 PM Sumit Garg <sumit.garg@linaro.org> wrote:
>
> On Wed, 16 Mar 2022 at 13:47, Jens Wiklander <jens.wiklander@linaro.org> wrote:
[snip]
> > > > 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:
> > > >   * 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:
> > >
> > > Although I didn't see any reference to OPTEE_SMC_CALL_WITH_REGD_ARG in
> > > the commit message, but do we really need to introduce it? Wouldn't it
> > > be possible to just pass additional "shared memory cookie" value as
> > > part of "Not used" (a4-6) arguments?
> >
> > I'll update the commit message to mention OPTEE_SMC_CALL_WITH_REGD_ARG
> > too. I think it's more clear with a separate ID for this, less risk of
> > confusion.
>
> IMO, it would unnecessarily complicate and introduce ambiguity in the
> ABI as after this patch we will have:
>
> CALL_WITH_ARG: Standard arguments *without* pre-allocated RPC arguments buffer
> CALL_WITH_RPC_ARG: Standard arguments *with* pre-allocated RPC arguments buffer
> CALL_WITH_REGD_ARG: Registered arguments but *not* explicit whether
> RPC arguments buffer is there or not.

CALL_WITH_REGD_ARG is quite explicit in the description above (in the patch):
 * 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.

I chose to use two new SMC IDs to have one clear purpose for each.

I preferred the name OPTEE_SMC_CALL_WITH_REGD_ARG instead of
OPTEE_SMC_CALL_WITH_REGD_RPC_ARG since I thought the former was long
enough.

>
> If we keep the ABI simplified to say we only support two types of
> invocation irrespective of whether the arguments are allocated from
> statically shared memory or dynamically shared memory:
>
> CALL_WITH_ARG: Standard arguments *without* pre-allocated RPC arguments buffer
> CALL_WITH_RPC_ARG: Standard arguments *with* pre-allocated RPC arguments buffer

That's only simple on the surface. When looking into the details of
CALL_WITH_RPC_ARG you'd need a more complicated register usage since
the function would be doing two different things.

>
> > How would the callee know if it's the cookie or the physical
> > address it should use?  Whatever we do, we're extenting the ABI.
> >
>
> Isn't it possible for OP-TEE to determine if it's a valid cookie or
> not which will be passed into currently unused arguments?

Actually, we need three registers, one to pass the offset in too.

Cheers,
Jens

>
> -Sumit
>
> > >
> > > > + * 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)
> > > >

Re: [PATCH 1/3] optee: add OPTEE_SMC_CALL_WITH_RPC_ARG

[Sumit Garg]

On Fri, 18 Mar 2022 at 12:59, Jens Wiklander <jens.wiklander@linaro.org> wrote:
>
> On Thu, Mar 17, 2022 at 12:40 PM Sumit Garg <sumit.garg@linaro.org> wrote:
> >
> > On Wed, 16 Mar 2022 at 13:47, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> [snip]
> > > > > 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:
> > > > >   * 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:
> > > >
> > > > Although I didn't see any reference to OPTEE_SMC_CALL_WITH_REGD_ARG in
> > > > the commit message, but do we really need to introduce it? Wouldn't it
> > > > be possible to just pass additional "shared memory cookie" value as
> > > > part of "Not used" (a4-6) arguments?
> > >
> > > I'll update the commit message to mention OPTEE_SMC_CALL_WITH_REGD_ARG
> > > too. I think it's more clear with a separate ID for this, less risk of
> > > confusion.
> >
> > IMO, it would unnecessarily complicate and introduce ambiguity in the
> > ABI as after this patch we will have:
> >
> > CALL_WITH_ARG: Standard arguments *without* pre-allocated RPC arguments buffer
> > CALL_WITH_RPC_ARG: Standard arguments *with* pre-allocated RPC arguments buffer
> > CALL_WITH_REGD_ARG: Registered arguments but *not* explicit whether
> > RPC arguments buffer is there or not.
>
> CALL_WITH_REGD_ARG is quite explicit in the description above (in the patch):
>  * 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.
>
> I chose to use two new SMC IDs to have one clear purpose for each.
>
> I preferred the name OPTEE_SMC_CALL_WITH_REGD_ARG instead of
> OPTEE_SMC_CALL_WITH_REGD_RPC_ARG since I thought the former was long
> enough.
>
> >
> > If we keep the ABI simplified to say we only support two types of
> > invocation irrespective of whether the arguments are allocated from
> > statically shared memory or dynamically shared memory:
> >
> > CALL_WITH_ARG: Standard arguments *without* pre-allocated RPC arguments buffer
> > CALL_WITH_RPC_ARG: Standard arguments *with* pre-allocated RPC arguments buffer
>
> That's only simple on the surface. When looking into the details of
> CALL_WITH_RPC_ARG you'd need a more complicated register usage since
> the function would be doing two different things.
>
> >
> > > How would the callee know if it's the cookie or the physical
> > > address it should use?  Whatever we do, we're extenting the ABI.
> > >
> >
> > Isn't it possible for OP-TEE to determine if it's a valid cookie or
> > not which will be passed into currently unused arguments?
>
> Actually, we need three registers, one to pass the offset in too.

Okay, fair enough I am fine with your preferred approach in this patch.

-Sumit

>
> Cheers,
> Jens
>
> >
> > -Sumit
> >
> > > >
> > > > > + * 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)
> > > > >

[PATCH 2/3] optee: add FF-A capability OPTEE_FFA_SEC_CAP_ARG_OFFSET

[Jens Wiklander]

Adds the secure capability OPTEE_FFA_SEC_CAP_ARG_OFFSET to indicate that
OP-TEE with FF-A can support an argument struct at a non-zero offset into
a passed shared memory object.

Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
---
 drivers/tee/optee/ffa_abi.c   | 17 +++++++++++++++--
 drivers/tee/optee/optee_ffa.h | 12 +++++++++++-
 2 files changed, 26 insertions(+), 3 deletions(-)

diff --git a/drivers/tee/optee/ffa_abi.c b/drivers/tee/optee/ffa_abi.c
index 7686f7020616..cc863aaefcd9 100644
--- a/drivers/tee/optee/ffa_abi.c
+++ b/drivers/tee/optee/ffa_abi.c
@@ -615,12 +615,21 @@ static int optee_ffa_do_call_with_arg(struct tee_context *ctx,
 		.data0 = OPTEE_FFA_YIELDING_CALL_WITH_ARG,
 		.data1 = (u32)shm->sec_world_id,
 		.data2 = (u32)(shm->sec_world_id >> 32),
-		.data3 = shm->offset,
+		.data3 = 0,
 	};
 	struct optee_msg_arg *arg;
 	unsigned int rpc_arg_offs;
 	struct optee_msg_arg *rpc_arg;
 
+	/*
+	 * The shared memory object has to start on a page when passed as
+	 * an argument struct. This is also what the shm pool allocator
+	 * returns, but check this before calling secure world to catch
+	 * eventual errors early in case something changes.
+	 */
+	if (shm->offset)
+		return -EINVAL;
+
 	arg = tee_shm_get_va(shm, 0);
 	if (IS_ERR(arg))
 		return PTR_ERR(arg);
@@ -678,6 +687,7 @@ static bool optee_ffa_api_is_compatbile(struct ffa_device *ffa_dev,
 
 static bool optee_ffa_exchange_caps(struct ffa_device *ffa_dev,
 				    const struct ffa_dev_ops *ops,
+				    u32 *sec_caps,
 				    unsigned int *rpc_param_count)
 {
 	struct ffa_send_direct_data data = { OPTEE_FFA_EXCHANGE_CAPABILITIES };
@@ -694,6 +704,7 @@ static bool optee_ffa_exchange_caps(struct ffa_device *ffa_dev,
 	}
 
 	*rpc_param_count = (u8)data.data1;
+	*sec_caps = data.data2;
 
 	return true;
 }
@@ -777,6 +788,7 @@ static int optee_ffa_probe(struct ffa_device *ffa_dev)
 	struct tee_device *teedev;
 	struct tee_context *ctx;
 	struct optee *optee;
+	u32 sec_caps;
 	int rc;
 
 	ffa_ops = ffa_dev_ops_get(ffa_dev);
@@ -788,7 +800,8 @@ static int optee_ffa_probe(struct ffa_device *ffa_dev)
 	if (!optee_ffa_api_is_compatbile(ffa_dev, ffa_ops))
 		return -EINVAL;
 
-	if (!optee_ffa_exchange_caps(ffa_dev, ffa_ops, &rpc_param_count))
+	if (!optee_ffa_exchange_caps(ffa_dev, ffa_ops, &sec_caps,
+				     &rpc_param_count))
 		return -EINVAL;
 
 	optee = kzalloc(sizeof(*optee), GFP_KERNEL);
diff --git a/drivers/tee/optee/optee_ffa.h b/drivers/tee/optee/optee_ffa.h
index ee3a03fc392c..97266243deaa 100644
--- a/drivers/tee/optee/optee_ffa.h
+++ b/drivers/tee/optee/optee_ffa.h
@@ -81,8 +81,16 @@
  *                   as the second MSG arg struct for
  *                   OPTEE_FFA_YIELDING_CALL_WITH_ARG.
  *        Bit[31:8]: Reserved (MBZ)
- * w5-w7: Note used (MBZ)
+ * w5:	  Bitfield of secure world capabilities OPTEE_FFA_SEC_CAP_* below,
+ *	  unused bits MBZ.
+ * w6-w7: Not used (MBZ)
+ */
+/*
+ * Secure world supports giving an offset into the argument shared memory
+ * object, see also OPTEE_FFA_YIELDING_CALL_WITH_ARG
  */
+#define OPTEE_FFA_SEC_CAP_ARG_OFFSET	BIT(0)
+
 #define OPTEE_FFA_EXCHANGE_CAPABILITIES OPTEE_FFA_BLOCKING_CALL(2)
 
 /*
@@ -112,6 +120,8 @@
  *	  OPTEE_MSG_GET_ARG_SIZE(num_params) follows a struct optee_msg_arg
  *	  for RPC, this struct has reserved space for the number of RPC
  *	  parameters as returned by OPTEE_FFA_EXCHANGE_CAPABILITIES.
+ *	  MBZ unless the bit OPTEE_FFA_SEC_CAP_ARG_OFFSET is received with
+ *	  OPTEE_FFA_EXCHANGE_CAPABILITIES.
  * w7:    Not used (MBZ)
  * Resume from RPC. Register usage:
  * w3:    Service ID, OPTEE_FFA_YIELDING_CALL_RESUME
-- 
2.31.1

Re: [PATCH 2/3] optee: add FF-A capability OPTEE_FFA_SEC_CAP_ARG_OFFSET

[Sumit Garg]

On Wed, 2 Mar 2022 at 01:18, Jens Wiklander <jens.wiklander@linaro.org> wrote:
>
> Adds the secure capability OPTEE_FFA_SEC_CAP_ARG_OFFSET to indicate that
> OP-TEE with FF-A can support an argument struct at a non-zero offset into
> a passed shared memory object.
>

It isn't clear to me why FF-A ABI requires this capability.
shm->offset should be honoured by default, if it isn't then it's a
bug. AFAIK, FF-A is currently still in its early stages so it
shouldn't be that hard to fix bugs in the ABI.

-Sumit

> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> ---
>  drivers/tee/optee/ffa_abi.c   | 17 +++++++++++++++--
>  drivers/tee/optee/optee_ffa.h | 12 +++++++++++-
>  2 files changed, 26 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/tee/optee/ffa_abi.c b/drivers/tee/optee/ffa_abi.c
> index 7686f7020616..cc863aaefcd9 100644
> --- a/drivers/tee/optee/ffa_abi.c
> +++ b/drivers/tee/optee/ffa_abi.c
> @@ -615,12 +615,21 @@ static int optee_ffa_do_call_with_arg(struct tee_context *ctx,
>                 .data0 = OPTEE_FFA_YIELDING_CALL_WITH_ARG,
>                 .data1 = (u32)shm->sec_world_id,
>                 .data2 = (u32)(shm->sec_world_id >> 32),
> -               .data3 = shm->offset,
> +               .data3 = 0,
>         };
>         struct optee_msg_arg *arg;
>         unsigned int rpc_arg_offs;
>         struct optee_msg_arg *rpc_arg;
>
> +       /*
> +        * The shared memory object has to start on a page when passed as
> +        * an argument struct. This is also what the shm pool allocator
> +        * returns, but check this before calling secure world to catch
> +        * eventual errors early in case something changes.
> +        */
> +       if (shm->offset)
> +               return -EINVAL;
> +
>         arg = tee_shm_get_va(shm, 0);
>         if (IS_ERR(arg))
>                 return PTR_ERR(arg);
> @@ -678,6 +687,7 @@ static bool optee_ffa_api_is_compatbile(struct ffa_device *ffa_dev,
>
>  static bool optee_ffa_exchange_caps(struct ffa_device *ffa_dev,
>                                     const struct ffa_dev_ops *ops,
> +                                   u32 *sec_caps,
>                                     unsigned int *rpc_param_count)
>  {
>         struct ffa_send_direct_data data = { OPTEE_FFA_EXCHANGE_CAPABILITIES };
> @@ -694,6 +704,7 @@ static bool optee_ffa_exchange_caps(struct ffa_device *ffa_dev,
>         }
>
>         *rpc_param_count = (u8)data.data1;
> +       *sec_caps = data.data2;
>
>         return true;
>  }
> @@ -777,6 +788,7 @@ static int optee_ffa_probe(struct ffa_device *ffa_dev)
>         struct tee_device *teedev;
>         struct tee_context *ctx;
>         struct optee *optee;
> +       u32 sec_caps;
>         int rc;
>
>         ffa_ops = ffa_dev_ops_get(ffa_dev);
> @@ -788,7 +800,8 @@ static int optee_ffa_probe(struct ffa_device *ffa_dev)
>         if (!optee_ffa_api_is_compatbile(ffa_dev, ffa_ops))
>                 return -EINVAL;
>
> -       if (!optee_ffa_exchange_caps(ffa_dev, ffa_ops, &rpc_param_count))
> +       if (!optee_ffa_exchange_caps(ffa_dev, ffa_ops, &sec_caps,
> +                                    &rpc_param_count))
>                 return -EINVAL;
>
>         optee = kzalloc(sizeof(*optee), GFP_KERNEL);
> diff --git a/drivers/tee/optee/optee_ffa.h b/drivers/tee/optee/optee_ffa.h
> index ee3a03fc392c..97266243deaa 100644
> --- a/drivers/tee/optee/optee_ffa.h
> +++ b/drivers/tee/optee/optee_ffa.h
> @@ -81,8 +81,16 @@
>   *                   as the second MSG arg struct for
>   *                   OPTEE_FFA_YIELDING_CALL_WITH_ARG.
>   *        Bit[31:8]: Reserved (MBZ)
> - * w5-w7: Note used (MBZ)
> + * w5:   Bitfield of secure world capabilities OPTEE_FFA_SEC_CAP_* below,
> + *       unused bits MBZ.
> + * w6-w7: Not used (MBZ)
> + */
> +/*
> + * Secure world supports giving an offset into the argument shared memory
> + * object, see also OPTEE_FFA_YIELDING_CALL_WITH_ARG
>   */
> +#define OPTEE_FFA_SEC_CAP_ARG_OFFSET   BIT(0)
> +
>  #define OPTEE_FFA_EXCHANGE_CAPABILITIES OPTEE_FFA_BLOCKING_CALL(2)
>
>  /*
> @@ -112,6 +120,8 @@
>   *       OPTEE_MSG_GET_ARG_SIZE(num_params) follows a struct optee_msg_arg
>   *       for RPC, this struct has reserved space for the number of RPC
>   *       parameters as returned by OPTEE_FFA_EXCHANGE_CAPABILITIES.
> + *       MBZ unless the bit OPTEE_FFA_SEC_CAP_ARG_OFFSET is received with
> + *       OPTEE_FFA_EXCHANGE_CAPABILITIES.
>   * w7:    Not used (MBZ)
>   * Resume from RPC. Register usage:
>   * w3:    Service ID, OPTEE_FFA_YIELDING_CALL_RESUME
> --
> 2.31.1
>

Re: [PATCH 2/3] optee: add FF-A capability OPTEE_FFA_SEC_CAP_ARG_OFFSET

[Jens Wiklander]

On Mon, Mar 14, 2022 at 02:33:26PM +0530, Sumit Garg wrote:
> On Wed, 2 Mar 2022 at 01:18, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> >
> > Adds the secure capability OPTEE_FFA_SEC_CAP_ARG_OFFSET to indicate that
> > OP-TEE with FF-A can support an argument struct at a non-zero offset into
> > a passed shared memory object.
> >
> 
> It isn't clear to me why FF-A ABI requires this capability.
> shm->offset should be honoured by default, if it isn't then it's a
> bug.

Yes, there was a bug in secure world when using a non-zero offset. So
far the driver has always used a zero offset that's why it hasn't been
caught earlier.

It's not a serious bug, but it might be quite hard to track down. This
is of course fixed now, but there is a window where it can be triggered.

So in order to spare FF-A developers this problem I'm making a non-zero
offset an extension guarded by a capability bit. Even though this is an
ABI change it's in practice not since it has been unused and not working
as expected.

The next commit will start using non-zero offsets if supported, so this
will change to become a problem (if left unchecked) in the window
mentioned above.

> AFAIK, FF-A is currently still in its early stages so it
> shouldn't be that hard to fix bugs in the ABI.

Starting from the kernel release (v5.16) where FF-A support in this
driver was merged the ABI is supposed to be stable.

Thanks,
Jens

> 
> -Sumit
> 
> > Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> > ---
> >  drivers/tee/optee/ffa_abi.c   | 17 +++++++++++++++--
> >  drivers/tee/optee/optee_ffa.h | 12 +++++++++++-
> >  2 files changed, 26 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/tee/optee/ffa_abi.c b/drivers/tee/optee/ffa_abi.c
> > index 7686f7020616..cc863aaefcd9 100644
> > --- a/drivers/tee/optee/ffa_abi.c
> > +++ b/drivers/tee/optee/ffa_abi.c
> > @@ -615,12 +615,21 @@ static int optee_ffa_do_call_with_arg(struct tee_context *ctx,
> >                 .data0 = OPTEE_FFA_YIELDING_CALL_WITH_ARG,
> >                 .data1 = (u32)shm->sec_world_id,
> >                 .data2 = (u32)(shm->sec_world_id >> 32),
> > -               .data3 = shm->offset,
> > +               .data3 = 0,
> >         };
> >         struct optee_msg_arg *arg;
> >         unsigned int rpc_arg_offs;
> >         struct optee_msg_arg *rpc_arg;
> >
> > +       /*
> > +        * The shared memory object has to start on a page when passed as
> > +        * an argument struct. This is also what the shm pool allocator
> > +        * returns, but check this before calling secure world to catch
> > +        * eventual errors early in case something changes.
> > +        */
> > +       if (shm->offset)
> > +               return -EINVAL;
> > +
> >         arg = tee_shm_get_va(shm, 0);
> >         if (IS_ERR(arg))
> >                 return PTR_ERR(arg);
> > @@ -678,6 +687,7 @@ static bool optee_ffa_api_is_compatbile(struct ffa_device *ffa_dev,
> >
> >  static bool optee_ffa_exchange_caps(struct ffa_device *ffa_dev,
> >                                     const struct ffa_dev_ops *ops,
> > +                                   u32 *sec_caps,
> >                                     unsigned int *rpc_param_count)
> >  {
> >         struct ffa_send_direct_data data = { OPTEE_FFA_EXCHANGE_CAPABILITIES };
> > @@ -694,6 +704,7 @@ static bool optee_ffa_exchange_caps(struct ffa_device *ffa_dev,
> >         }
> >
> >         *rpc_param_count = (u8)data.data1;
> > +       *sec_caps = data.data2;
> >
> >         return true;
> >  }
> > @@ -777,6 +788,7 @@ static int optee_ffa_probe(struct ffa_device *ffa_dev)
> >         struct tee_device *teedev;
> >         struct tee_context *ctx;
> >         struct optee *optee;
> > +       u32 sec_caps;
> >         int rc;
> >
> >         ffa_ops = ffa_dev_ops_get(ffa_dev);
> > @@ -788,7 +800,8 @@ static int optee_ffa_probe(struct ffa_device *ffa_dev)
> >         if (!optee_ffa_api_is_compatbile(ffa_dev, ffa_ops))
> >                 return -EINVAL;
> >
> > -       if (!optee_ffa_exchange_caps(ffa_dev, ffa_ops, &rpc_param_count))
> > +       if (!optee_ffa_exchange_caps(ffa_dev, ffa_ops, &sec_caps,
> > +                                    &rpc_param_count))
> >                 return -EINVAL;
> >
> >         optee = kzalloc(sizeof(*optee), GFP_KERNEL);
> > diff --git a/drivers/tee/optee/optee_ffa.h b/drivers/tee/optee/optee_ffa.h
> > index ee3a03fc392c..97266243deaa 100644
> > --- a/drivers/tee/optee/optee_ffa.h
> > +++ b/drivers/tee/optee/optee_ffa.h
> > @@ -81,8 +81,16 @@
> >   *                   as the second MSG arg struct for
> >   *                   OPTEE_FFA_YIELDING_CALL_WITH_ARG.
> >   *        Bit[31:8]: Reserved (MBZ)
> > - * w5-w7: Note used (MBZ)
> > + * w5:   Bitfield of secure world capabilities OPTEE_FFA_SEC_CAP_* below,
> > + *       unused bits MBZ.
> > + * w6-w7: Not used (MBZ)
> > + */
> > +/*
> > + * Secure world supports giving an offset into the argument shared memory
> > + * object, see also OPTEE_FFA_YIELDING_CALL_WITH_ARG
> >   */
> > +#define OPTEE_FFA_SEC_CAP_ARG_OFFSET   BIT(0)
> > +
> >  #define OPTEE_FFA_EXCHANGE_CAPABILITIES OPTEE_FFA_BLOCKING_CALL(2)
> >
> >  /*
> > @@ -112,6 +120,8 @@
> >   *       OPTEE_MSG_GET_ARG_SIZE(num_params) follows a struct optee_msg_arg
> >   *       for RPC, this struct has reserved space for the number of RPC
> >   *       parameters as returned by OPTEE_FFA_EXCHANGE_CAPABILITIES.
> > + *       MBZ unless the bit OPTEE_FFA_SEC_CAP_ARG_OFFSET is received with
> > + *       OPTEE_FFA_EXCHANGE_CAPABILITIES.
> >   * w7:    Not used (MBZ)
> >   * Resume from RPC. Register usage:
> >   * w3:    Service ID, OPTEE_FFA_YIELDING_CALL_RESUME
> > --
> > 2.31.1
> >

Re: [PATCH 2/3] optee: add FF-A capability OPTEE_FFA_SEC_CAP_ARG_OFFSET

[Sumit Garg]

On Wed, 16 Mar 2022 at 14:57, Jens Wiklander <jens.wiklander@linaro.org> wrote:
>
> On Mon, Mar 14, 2022 at 02:33:26PM +0530, Sumit Garg wrote:
> > On Wed, 2 Mar 2022 at 01:18, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> > >
> > > Adds the secure capability OPTEE_FFA_SEC_CAP_ARG_OFFSET to indicate that
> > > OP-TEE with FF-A can support an argument struct at a non-zero offset into
> > > a passed shared memory object.
> > >
> >
> > It isn't clear to me why FF-A ABI requires this capability.
> > shm->offset should be honoured by default, if it isn't then it's a
> > bug.
>
> Yes, there was a bug in secure world when using a non-zero offset. So
> far the driver has always used a zero offset that's why it hasn't been
> caught earlier.
>
> It's not a serious bug, but it might be quite hard to track down. This
> is of course fixed now, but there is a window where it can be triggered.

I am not able to find the fix in this [1] OP-TEE OS commit. Could you
help me to point it out?

[1] https://github.com/OP-TEE/optee_os/commit/89d991352352b80ae90f82228a014bb8cadfb80b

>
> So in order to spare FF-A developers this problem I'm making a non-zero
> offset an extension guarded by a capability bit. Even though this is an
> ABI change it's in practice not since it has been unused and not working
> as expected.
>
> The next commit will start using non-zero offsets if supported, so this
> will change to become a problem (if left unchecked) in the window
> mentioned above.
>
> > AFAIK, FF-A is currently still in its early stages so it
> > shouldn't be that hard to fix bugs in the ABI.
>
> Starting from the kernel release (v5.16) where FF-A support in this
> driver was merged the ABI is supposed to be stable.
>

From Linux kernel perspective, there are dedicated stable branches for
that purpose in case we want to push a fix for OP-TEE FF-A kernel
driver.

-Sumit

> Thanks,
> Jens
>
> >
> > -Sumit
> >
> > > Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> > > ---
> > >  drivers/tee/optee/ffa_abi.c   | 17 +++++++++++++++--
> > >  drivers/tee/optee/optee_ffa.h | 12 +++++++++++-
> > >  2 files changed, 26 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/tee/optee/ffa_abi.c b/drivers/tee/optee/ffa_abi.c
> > > index 7686f7020616..cc863aaefcd9 100644
> > > --- a/drivers/tee/optee/ffa_abi.c
> > > +++ b/drivers/tee/optee/ffa_abi.c
> > > @@ -615,12 +615,21 @@ static int optee_ffa_do_call_with_arg(struct tee_context *ctx,
> > >                 .data0 = OPTEE_FFA_YIELDING_CALL_WITH_ARG,
> > >                 .data1 = (u32)shm->sec_world_id,
> > >                 .data2 = (u32)(shm->sec_world_id >> 32),
> > > -               .data3 = shm->offset,
> > > +               .data3 = 0,
> > >         };
> > >         struct optee_msg_arg *arg;
> > >         unsigned int rpc_arg_offs;
> > >         struct optee_msg_arg *rpc_arg;
> > >
> > > +       /*
> > > +        * The shared memory object has to start on a page when passed as
> > > +        * an argument struct. This is also what the shm pool allocator
> > > +        * returns, but check this before calling secure world to catch
> > > +        * eventual errors early in case something changes.
> > > +        */
> > > +       if (shm->offset)
> > > +               return -EINVAL;
> > > +
> > >         arg = tee_shm_get_va(shm, 0);
> > >         if (IS_ERR(arg))
> > >                 return PTR_ERR(arg);
> > > @@ -678,6 +687,7 @@ static bool optee_ffa_api_is_compatbile(struct ffa_device *ffa_dev,
> > >
> > >  static bool optee_ffa_exchange_caps(struct ffa_device *ffa_dev,
> > >                                     const struct ffa_dev_ops *ops,
> > > +                                   u32 *sec_caps,
> > >                                     unsigned int *rpc_param_count)
> > >  {
> > >         struct ffa_send_direct_data data = { OPTEE_FFA_EXCHANGE_CAPABILITIES };
> > > @@ -694,6 +704,7 @@ static bool optee_ffa_exchange_caps(struct ffa_device *ffa_dev,
> > >         }
> > >
> > >         *rpc_param_count = (u8)data.data1;
> > > +       *sec_caps = data.data2;
> > >
> > >         return true;
> > >  }
> > > @@ -777,6 +788,7 @@ static int optee_ffa_probe(struct ffa_device *ffa_dev)
> > >         struct tee_device *teedev;
> > >         struct tee_context *ctx;
> > >         struct optee *optee;
> > > +       u32 sec_caps;
> > >         int rc;
> > >
> > >         ffa_ops = ffa_dev_ops_get(ffa_dev);
> > > @@ -788,7 +800,8 @@ static int optee_ffa_probe(struct ffa_device *ffa_dev)
> > >         if (!optee_ffa_api_is_compatbile(ffa_dev, ffa_ops))
> > >                 return -EINVAL;
> > >
> > > -       if (!optee_ffa_exchange_caps(ffa_dev, ffa_ops, &rpc_param_count))
> > > +       if (!optee_ffa_exchange_caps(ffa_dev, ffa_ops, &sec_caps,
> > > +                                    &rpc_param_count))
> > >                 return -EINVAL;
> > >
> > >         optee = kzalloc(sizeof(*optee), GFP_KERNEL);
> > > diff --git a/drivers/tee/optee/optee_ffa.h b/drivers/tee/optee/optee_ffa.h
> > > index ee3a03fc392c..97266243deaa 100644
> > > --- a/drivers/tee/optee/optee_ffa.h
> > > +++ b/drivers/tee/optee/optee_ffa.h
> > > @@ -81,8 +81,16 @@
> > >   *                   as the second MSG arg struct for
> > >   *                   OPTEE_FFA_YIELDING_CALL_WITH_ARG.
> > >   *        Bit[31:8]: Reserved (MBZ)
> > > - * w5-w7: Note used (MBZ)
> > > + * w5:   Bitfield of secure world capabilities OPTEE_FFA_SEC_CAP_* below,
> > > + *       unused bits MBZ.
> > > + * w6-w7: Not used (MBZ)
> > > + */
> > > +/*
> > > + * Secure world supports giving an offset into the argument shared memory
> > > + * object, see also OPTEE_FFA_YIELDING_CALL_WITH_ARG
> > >   */
> > > +#define OPTEE_FFA_SEC_CAP_ARG_OFFSET   BIT(0)
> > > +
> > >  #define OPTEE_FFA_EXCHANGE_CAPABILITIES OPTEE_FFA_BLOCKING_CALL(2)
> > >
> > >  /*
> > > @@ -112,6 +120,8 @@
> > >   *       OPTEE_MSG_GET_ARG_SIZE(num_params) follows a struct optee_msg_arg
> > >   *       for RPC, this struct has reserved space for the number of RPC
> > >   *       parameters as returned by OPTEE_FFA_EXCHANGE_CAPABILITIES.
> > > + *       MBZ unless the bit OPTEE_FFA_SEC_CAP_ARG_OFFSET is received with
> > > + *       OPTEE_FFA_EXCHANGE_CAPABILITIES.
> > >   * w7:    Not used (MBZ)
> > >   * Resume from RPC. Register usage:
> > >   * w3:    Service ID, OPTEE_FFA_YIELDING_CALL_RESUME
> > > --
> > > 2.31.1
> > >

Re: [PATCH 2/3] optee: add FF-A capability OPTEE_FFA_SEC_CAP_ARG_OFFSET

[Jens Wiklander]

On Thu, Mar 17, 2022 at 1:17 PM Sumit Garg <sumit.garg@linaro.org> wrote:
>
> On Wed, 16 Mar 2022 at 14:57, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> >
> > On Mon, Mar 14, 2022 at 02:33:26PM +0530, Sumit Garg wrote:
> > > On Wed, 2 Mar 2022 at 01:18, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> > > >
> > > > Adds the secure capability OPTEE_FFA_SEC_CAP_ARG_OFFSET to indicate that
> > > > OP-TEE with FF-A can support an argument struct at a non-zero offset into
> > > > a passed shared memory object.
> > > >
> > >
> > > It isn't clear to me why FF-A ABI requires this capability.
> > > shm->offset should be honoured by default, if it isn't then it's a
> > > bug.
> >
> > Yes, there was a bug in secure world when using a non-zero offset. So
> > far the driver has always used a zero offset that's why it hasn't been
> > caught earlier.
> >
> > It's not a serious bug, but it might be quite hard to track down. This
> > is of course fixed now, but there is a window where it can be triggered.
>
> I am not able to find the fix in this [1] OP-TEE OS commit. Could you
> help me to point it out?
>
> [1] https://github.com/OP-TEE/optee_os/commit/89d991352352b80ae90f82228a014bb8cadfb80b

I believe it's this one:
https://github.com/OP-TEE/optee_os/commit/7267624eef019476f20aee7dba11ff949d005670

>
> >
> > So in order to spare FF-A developers this problem I'm making a non-zero
> > offset an extension guarded by a capability bit. Even though this is an
> > ABI change it's in practice not since it has been unused and not working
> > as expected.
> >
> > The next commit will start using non-zero offsets if supported, so this
> > will change to become a problem (if left unchecked) in the window
> > mentioned above.
> >
> > > AFAIK, FF-A is currently still in its early stages so it
> > > shouldn't be that hard to fix bugs in the ABI.
> >
> > Starting from the kernel release (v5.16) where FF-A support in this
> > driver was merged the ABI is supposed to be stable.
> >
>
> From Linux kernel perspective, there are dedicated stable branches for
> that purpose in case we want to push a fix for OP-TEE FF-A kernel
> driver.

The problem isn't in the kernel driver. What I'm doing here is to
formalize the ABI that the kernel defacto was using. Since we
obviously didn't test the driver with a non-zero offset before, we
didn't notice that the offset wasn't used in the right way by the
secure world.

Cheers,
Jens

>
> -Sumit
>
> > Thanks,
> > Jens
> >
> > >
> > > -Sumit
> > >
> > > > Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> > > > ---
> > > >  drivers/tee/optee/ffa_abi.c   | 17 +++++++++++++++--
> > > >  drivers/tee/optee/optee_ffa.h | 12 +++++++++++-
> > > >  2 files changed, 26 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/drivers/tee/optee/ffa_abi.c b/drivers/tee/optee/ffa_abi.c
> > > > index 7686f7020616..cc863aaefcd9 100644
> > > > --- a/drivers/tee/optee/ffa_abi.c
> > > > +++ b/drivers/tee/optee/ffa_abi.c
> > > > @@ -615,12 +615,21 @@ static int optee_ffa_do_call_with_arg(struct tee_context *ctx,
> > > >                 .data0 = OPTEE_FFA_YIELDING_CALL_WITH_ARG,
> > > >                 .data1 = (u32)shm->sec_world_id,
> > > >                 .data2 = (u32)(shm->sec_world_id >> 32),
> > > > -               .data3 = shm->offset,
> > > > +               .data3 = 0,
> > > >         };
> > > >         struct optee_msg_arg *arg;
> > > >         unsigned int rpc_arg_offs;
> > > >         struct optee_msg_arg *rpc_arg;
> > > >
> > > > +       /*
> > > > +        * The shared memory object has to start on a page when passed as
> > > > +        * an argument struct. This is also what the shm pool allocator
> > > > +        * returns, but check this before calling secure world to catch
> > > > +        * eventual errors early in case something changes.
> > > > +        */
> > > > +       if (shm->offset)
> > > > +               return -EINVAL;
> > > > +
> > > >         arg = tee_shm_get_va(shm, 0);
> > > >         if (IS_ERR(arg))
> > > >                 return PTR_ERR(arg);
> > > > @@ -678,6 +687,7 @@ static bool optee_ffa_api_is_compatbile(struct ffa_device *ffa_dev,
> > > >
> > > >  static bool optee_ffa_exchange_caps(struct ffa_device *ffa_dev,
> > > >                                     const struct ffa_dev_ops *ops,
> > > > +                                   u32 *sec_caps,
> > > >                                     unsigned int *rpc_param_count)
> > > >  {
> > > >         struct ffa_send_direct_data data = { OPTEE_FFA_EXCHANGE_CAPABILITIES };
> > > > @@ -694,6 +704,7 @@ static bool optee_ffa_exchange_caps(struct ffa_device *ffa_dev,
> > > >         }
> > > >
> > > >         *rpc_param_count = (u8)data.data1;
> > > > +       *sec_caps = data.data2;
> > > >
> > > >         return true;
> > > >  }
> > > > @@ -777,6 +788,7 @@ static int optee_ffa_probe(struct ffa_device *ffa_dev)
> > > >         struct tee_device *teedev;
> > > >         struct tee_context *ctx;
> > > >         struct optee *optee;
> > > > +       u32 sec_caps;
> > > >         int rc;
> > > >
> > > >         ffa_ops = ffa_dev_ops_get(ffa_dev);
> > > > @@ -788,7 +800,8 @@ static int optee_ffa_probe(struct ffa_device *ffa_dev)
> > > >         if (!optee_ffa_api_is_compatbile(ffa_dev, ffa_ops))
> > > >                 return -EINVAL;
> > > >
> > > > -       if (!optee_ffa_exchange_caps(ffa_dev, ffa_ops, &rpc_param_count))
> > > > +       if (!optee_ffa_exchange_caps(ffa_dev, ffa_ops, &sec_caps,
> > > > +                                    &rpc_param_count))
> > > >                 return -EINVAL;
> > > >
> > > >         optee = kzalloc(sizeof(*optee), GFP_KERNEL);
> > > > diff --git a/drivers/tee/optee/optee_ffa.h b/drivers/tee/optee/optee_ffa.h
> > > > index ee3a03fc392c..97266243deaa 100644
> > > > --- a/drivers/tee/optee/optee_ffa.h
> > > > +++ b/drivers/tee/optee/optee_ffa.h
> > > > @@ -81,8 +81,16 @@
> > > >   *                   as the second MSG arg struct for
> > > >   *                   OPTEE_FFA_YIELDING_CALL_WITH_ARG.
> > > >   *        Bit[31:8]: Reserved (MBZ)
> > > > - * w5-w7: Note used (MBZ)
> > > > + * w5:   Bitfield of secure world capabilities OPTEE_FFA_SEC_CAP_* below,
> > > > + *       unused bits MBZ.
> > > > + * w6-w7: Not used (MBZ)
> > > > + */
> > > > +/*
> > > > + * Secure world supports giving an offset into the argument shared memory
> > > > + * object, see also OPTEE_FFA_YIELDING_CALL_WITH_ARG
> > > >   */
> > > > +#define OPTEE_FFA_SEC_CAP_ARG_OFFSET   BIT(0)
> > > > +
> > > >  #define OPTEE_FFA_EXCHANGE_CAPABILITIES OPTEE_FFA_BLOCKING_CALL(2)
> > > >
> > > >  /*
> > > > @@ -112,6 +120,8 @@
> > > >   *       OPTEE_MSG_GET_ARG_SIZE(num_params) follows a struct optee_msg_arg
> > > >   *       for RPC, this struct has reserved space for the number of RPC
> > > >   *       parameters as returned by OPTEE_FFA_EXCHANGE_CAPABILITIES.
> > > > + *       MBZ unless the bit OPTEE_FFA_SEC_CAP_ARG_OFFSET is received with
> > > > + *       OPTEE_FFA_EXCHANGE_CAPABILITIES.
> > > >   * w7:    Not used (MBZ)
> > > >   * Resume from RPC. Register usage:
> > > >   * w3:    Service ID, OPTEE_FFA_YIELDING_CALL_RESUME
> > > > --
> > > > 2.31.1
> > > >

Re: [PATCH 2/3] optee: add FF-A capability OPTEE_FFA_SEC_CAP_ARG_OFFSET

[Sumit Garg]

Hi Jens,

Apologies for the delay as I was on leave for the past 2 weeks.

On Fri, 18 Mar 2022 at 13:19, Jens Wiklander <jens.wiklander@linaro.org> wrote:
>
> On Thu, Mar 17, 2022 at 1:17 PM Sumit Garg <sumit.garg@linaro.org> wrote:
> >
> > On Wed, 16 Mar 2022 at 14:57, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> > >
> > > On Mon, Mar 14, 2022 at 02:33:26PM +0530, Sumit Garg wrote:
> > > > On Wed, 2 Mar 2022 at 01:18, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> > > > >
> > > > > Adds the secure capability OPTEE_FFA_SEC_CAP_ARG_OFFSET to indicate that
> > > > > OP-TEE with FF-A can support an argument struct at a non-zero offset into
> > > > > a passed shared memory object.
> > > > >
> > > >
> > > > It isn't clear to me why FF-A ABI requires this capability.
> > > > shm->offset should be honoured by default, if it isn't then it's a
> > > > bug.
> > >
> > > Yes, there was a bug in secure world when using a non-zero offset. So
> > > far the driver has always used a zero offset that's why it hasn't been
> > > caught earlier.
> > >
> > > It's not a serious bug, but it might be quite hard to track down. This
> > > is of course fixed now, but there is a window where it can be triggered.
> >
> > I am not able to find the fix in this [1] OP-TEE OS commit. Could you
> > help me to point it out?
> >
> > [1] https://github.com/OP-TEE/optee_os/commit/89d991352352b80ae90f82228a014bb8cadfb80b
>
> I believe it's this one:
> https://github.com/OP-TEE/optee_os/commit/7267624eef019476f20aee7dba11ff949d005670
>

Which OP-TEE release is affected by this where FF-A functionality is
fully supported?

> >
> > >
> > > So in order to spare FF-A developers this problem I'm making a non-zero
> > > offset an extension guarded by a capability bit. Even though this is an
> > > ABI change it's in practice not since it has been unused and not working
> > > as expected.
> > >
> > > The next commit will start using non-zero offsets if supported, so this
> > > will change to become a problem (if left unchecked) in the window
> > > mentioned above.
> > >
> > > > AFAIK, FF-A is currently still in its early stages so it
> > > > shouldn't be that hard to fix bugs in the ABI.
> > >
> > > Starting from the kernel release (v5.16) where FF-A support in this
> > > driver was merged the ABI is supposed to be stable.
> > >
> >
> > From Linux kernel perspective, there are dedicated stable branches for
> > that purpose in case we want to push a fix for OP-TEE FF-A kernel
> > driver.
>
> The problem isn't in the kernel driver. What I'm doing here is to
> formalize the ABI that the kernel defacto was using. Since we
> obviously didn't test the driver with a non-zero offset before, we
> didn't notice that the offset wasn't used in the right way by the
> secure world.

My understanding of capabilities is to denote new features rather than
bug fixes. So I am still not able to make any sense regarding why we
are abusing capabilities to fix bugs in the ABI. If we find more bugs
in the FF-A ABI in future then we will be stuck adding new
capabilities which aren't scalable and unnecessarily complicates
kernel driver.

IMO, the best way to maintain a stable ABI would be to create a stable
release for OP-TEE as well where ABI fixes can be backported. But
since we don't have that currently, we have to live with cherry
picking ABI fixes if we observe issues with earlier OP-TEE releases.

-Sumit

>
> Cheers,
> Jens
>
> >
> > -Sumit
> >
> > > Thanks,
> > > Jens
> > >
> > > >
> > > > -Sumit
> > > >
> > > > > Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> > > > > ---
> > > > >  drivers/tee/optee/ffa_abi.c   | 17 +++++++++++++++--
> > > > >  drivers/tee/optee/optee_ffa.h | 12 +++++++++++-
> > > > >  2 files changed, 26 insertions(+), 3 deletions(-)
> > > > >
> > > > > diff --git a/drivers/tee/optee/ffa_abi.c b/drivers/tee/optee/ffa_abi.c
> > > > > index 7686f7020616..cc863aaefcd9 100644
> > > > > --- a/drivers/tee/optee/ffa_abi.c
> > > > > +++ b/drivers/tee/optee/ffa_abi.c
> > > > > @@ -615,12 +615,21 @@ static int optee_ffa_do_call_with_arg(struct tee_context *ctx,
> > > > >                 .data0 = OPTEE_FFA_YIELDING_CALL_WITH_ARG,
> > > > >                 .data1 = (u32)shm->sec_world_id,
> > > > >                 .data2 = (u32)(shm->sec_world_id >> 32),
> > > > > -               .data3 = shm->offset,
> > > > > +               .data3 = 0,
> > > > >         };
> > > > >         struct optee_msg_arg *arg;
> > > > >         unsigned int rpc_arg_offs;
> > > > >         struct optee_msg_arg *rpc_arg;
> > > > >
> > > > > +       /*
> > > > > +        * The shared memory object has to start on a page when passed as
> > > > > +        * an argument struct. This is also what the shm pool allocator
> > > > > +        * returns, but check this before calling secure world to catch
> > > > > +        * eventual errors early in case something changes.
> > > > > +        */
> > > > > +       if (shm->offset)
> > > > > +               return -EINVAL;
> > > > > +
> > > > >         arg = tee_shm_get_va(shm, 0);
> > > > >         if (IS_ERR(arg))
> > > > >                 return PTR_ERR(arg);
> > > > > @@ -678,6 +687,7 @@ static bool optee_ffa_api_is_compatbile(struct ffa_device *ffa_dev,
> > > > >
> > > > >  static bool optee_ffa_exchange_caps(struct ffa_device *ffa_dev,
> > > > >                                     const struct ffa_dev_ops *ops,
> > > > > +                                   u32 *sec_caps,
> > > > >                                     unsigned int *rpc_param_count)
> > > > >  {
> > > > >         struct ffa_send_direct_data data = { OPTEE_FFA_EXCHANGE_CAPABILITIES };
> > > > > @@ -694,6 +704,7 @@ static bool optee_ffa_exchange_caps(struct ffa_device *ffa_dev,
> > > > >         }
> > > > >
> > > > >         *rpc_param_count = (u8)data.data1;
> > > > > +       *sec_caps = data.data2;
> > > > >
> > > > >         return true;
> > > > >  }
> > > > > @@ -777,6 +788,7 @@ static int optee_ffa_probe(struct ffa_device *ffa_dev)
> > > > >         struct tee_device *teedev;
> > > > >         struct tee_context *ctx;
> > > > >         struct optee *optee;
> > > > > +       u32 sec_caps;
> > > > >         int rc;
> > > > >
> > > > >         ffa_ops = ffa_dev_ops_get(ffa_dev);
> > > > > @@ -788,7 +800,8 @@ static int optee_ffa_probe(struct ffa_device *ffa_dev)
> > > > >         if (!optee_ffa_api_is_compatbile(ffa_dev, ffa_ops))
> > > > >                 return -EINVAL;
> > > > >
> > > > > -       if (!optee_ffa_exchange_caps(ffa_dev, ffa_ops, &rpc_param_count))
> > > > > +       if (!optee_ffa_exchange_caps(ffa_dev, ffa_ops, &sec_caps,
> > > > > +                                    &rpc_param_count))
> > > > >                 return -EINVAL;
> > > > >
> > > > >         optee = kzalloc(sizeof(*optee), GFP_KERNEL);
> > > > > diff --git a/drivers/tee/optee/optee_ffa.h b/drivers/tee/optee/optee_ffa.h
> > > > > index ee3a03fc392c..97266243deaa 100644
> > > > > --- a/drivers/tee/optee/optee_ffa.h
> > > > > +++ b/drivers/tee/optee/optee_ffa.h
> > > > > @@ -81,8 +81,16 @@
> > > > >   *                   as the second MSG arg struct for
> > > > >   *                   OPTEE_FFA_YIELDING_CALL_WITH_ARG.
> > > > >   *        Bit[31:8]: Reserved (MBZ)
> > > > > - * w5-w7: Note used (MBZ)
> > > > > + * w5:   Bitfield of secure world capabilities OPTEE_FFA_SEC_CAP_* below,
> > > > > + *       unused bits MBZ.
> > > > > + * w6-w7: Not used (MBZ)
> > > > > + */
> > > > > +/*
> > > > > + * Secure world supports giving an offset into the argument shared memory
> > > > > + * object, see also OPTEE_FFA_YIELDING_CALL_WITH_ARG
> > > > >   */
> > > > > +#define OPTEE_FFA_SEC_CAP_ARG_OFFSET   BIT(0)
> > > > > +
> > > > >  #define OPTEE_FFA_EXCHANGE_CAPABILITIES OPTEE_FFA_BLOCKING_CALL(2)
> > > > >
> > > > >  /*
> > > > > @@ -112,6 +120,8 @@
> > > > >   *       OPTEE_MSG_GET_ARG_SIZE(num_params) follows a struct optee_msg_arg
> > > > >   *       for RPC, this struct has reserved space for the number of RPC
> > > > >   *       parameters as returned by OPTEE_FFA_EXCHANGE_CAPABILITIES.
> > > > > + *       MBZ unless the bit OPTEE_FFA_SEC_CAP_ARG_OFFSET is received with
> > > > > + *       OPTEE_FFA_EXCHANGE_CAPABILITIES.
> > > > >   * w7:    Not used (MBZ)
> > > > >   * Resume from RPC. Register usage:
> > > > >   * w3:    Service ID, OPTEE_FFA_YIELDING_CALL_RESUME
> > > > > --
> > > > > 2.31.1
> > > > >

Re: [PATCH 2/3] optee: add FF-A capability OPTEE_FFA_SEC_CAP_ARG_OFFSET

[Jens Wiklander]

Hi Sumit,

On Tue, Apr 5, 2022 at 2:26 PM Sumit Garg <sumit.garg@linaro.org> wrote:
>
> Hi Jens,
>
> Apologies for the delay as I was on leave for the past 2 weeks.

No worries, quite understandable.

>
> On Fri, 18 Mar 2022 at 13:19, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> >
> > On Thu, Mar 17, 2022 at 1:17 PM Sumit Garg <sumit.garg@linaro.org> wrote:
> > >
> > > On Wed, 16 Mar 2022 at 14:57, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> > > >
> > > > On Mon, Mar 14, 2022 at 02:33:26PM +0530, Sumit Garg wrote:
> > > > > On Wed, 2 Mar 2022 at 01:18, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> > > > > >
> > > > > > Adds the secure capability OPTEE_FFA_SEC_CAP_ARG_OFFSET to indicate that
> > > > > > OP-TEE with FF-A can support an argument struct at a non-zero offset into
> > > > > > a passed shared memory object.
> > > > > >
> > > > >
> > > > > It isn't clear to me why FF-A ABI requires this capability.
> > > > > shm->offset should be honoured by default, if it isn't then it's a
> > > > > bug.
> > > >
> > > > Yes, there was a bug in secure world when using a non-zero offset. So
> > > > far the driver has always used a zero offset that's why it hasn't been
> > > > caught earlier.
> > > >
> > > > It's not a serious bug, but it might be quite hard to track down. This
> > > > is of course fixed now, but there is a window where it can be triggered.
> > >
> > > I am not able to find the fix in this [1] OP-TEE OS commit. Could you
> > > help me to point it out?
> > >
> > > [1] https://github.com/OP-TEE/optee_os/commit/89d991352352b80ae90f82228a014bb8cadfb80b
> >
> > I believe it's this one:
> > https://github.com/OP-TEE/optee_os/commit/7267624eef019476f20aee7dba11ff949d005670
> >
>
> Which OP-TEE release is affected by this where FF-A functionality is
> fully supported?

3.10.0 to 3.15.0, I'm note sure where to draw the line for fully
supported. But it would be a pity to break 3.14.0 and 3.15.0.

>
> > >
> > > >
> > > > So in order to spare FF-A developers this problem I'm making a non-zero
> > > > offset an extension guarded by a capability bit. Even though this is an
> > > > ABI change it's in practice not since it has been unused and not working
> > > > as expected.
> > > >
> > > > The next commit will start using non-zero offsets if supported, so this
> > > > will change to become a problem (if left unchecked) in the window
> > > > mentioned above.
> > > >
> > > > > AFAIK, FF-A is currently still in its early stages so it
> > > > > shouldn't be that hard to fix bugs in the ABI.
> > > >
> > > > Starting from the kernel release (v5.16) where FF-A support in this
> > > > driver was merged the ABI is supposed to be stable.
> > > >
> > >
> > > From Linux kernel perspective, there are dedicated stable branches for
> > > that purpose in case we want to push a fix for OP-TEE FF-A kernel
> > > driver.
> >
> > The problem isn't in the kernel driver. What I'm doing here is to
> > formalize the ABI that the kernel defacto was using. Since we
> > obviously didn't test the driver with a non-zero offset before, we
> > didn't notice that the offset wasn't used in the right way by the
> > secure world.
>
> My understanding of capabilities is to denote new features rather than
> bug fixes.

If you take the pragmatic approach to this it could be seen as a new
feature, this part of the ABI wasn't used before and had obviously not
been tested either. The only place where it did exist before was in a
comment.

> So I am still not able to make any sense regarding why we
> are abusing capabilities to fix bugs in the ABI. If we find more bugs
> in the FF-A ABI in future then we will be stuck adding new
> capabilities which aren't scalable and unnecessarily complicates
> kernel driver.

This is a balance act, in this case I believe it's worth it. It
doesn't leave any strange artefacts in the ABI.

Others are using OP-TEE for FF-A development without knowing too many
details of how this part works. So breakage here could cause a bit of
trouble for those developers and also give the impression that we
don't care if we break things.

>
> IMO, the best way to maintain a stable ABI would be to create a stable
> release for OP-TEE as well where ABI fixes can be backported.

I disagree, breakage is breakage.

> But
> since we don't have that currently, we have to live with cherry
> picking ABI fixes if we observe issues with earlier OP-TEE releases.

We have so far had zero requests to keep stable branches. I don't
think they would have helped in this case anyway.

Cheers,
Jens

>
> -Sumit
>
> >
> > Cheers,
> > Jens
> >
> > >
> > > -Sumit
> > >
> > > > Thanks,
> > > > Jens
> > > >
> > > > >
> > > > > -Sumit
> > > > >
> > > > > > Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> > > > > > ---
> > > > > >  drivers/tee/optee/ffa_abi.c   | 17 +++++++++++++++--
> > > > > >  drivers/tee/optee/optee_ffa.h | 12 +++++++++++-
> > > > > >  2 files changed, 26 insertions(+), 3 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/tee/optee/ffa_abi.c b/drivers/tee/optee/ffa_abi.c
> > > > > > index 7686f7020616..cc863aaefcd9 100644
> > > > > > --- a/drivers/tee/optee/ffa_abi.c
> > > > > > +++ b/drivers/tee/optee/ffa_abi.c
> > > > > > @@ -615,12 +615,21 @@ static int optee_ffa_do_call_with_arg(struct tee_context *ctx,
> > > > > >                 .data0 = OPTEE_FFA_YIELDING_CALL_WITH_ARG,
> > > > > >                 .data1 = (u32)shm->sec_world_id,
> > > > > >                 .data2 = (u32)(shm->sec_world_id >> 32),
> > > > > > -               .data3 = shm->offset,
> > > > > > +               .data3 = 0,
> > > > > >         };
> > > > > >         struct optee_msg_arg *arg;
> > > > > >         unsigned int rpc_arg_offs;
> > > > > >         struct optee_msg_arg *rpc_arg;
> > > > > >
> > > > > > +       /*
> > > > > > +        * The shared memory object has to start on a page when passed as
> > > > > > +        * an argument struct. This is also what the shm pool allocator
> > > > > > +        * returns, but check this before calling secure world to catch
> > > > > > +        * eventual errors early in case something changes.
> > > > > > +        */
> > > > > > +       if (shm->offset)
> > > > > > +               return -EINVAL;
> > > > > > +
> > > > > >         arg = tee_shm_get_va(shm, 0);
> > > > > >         if (IS_ERR(arg))
> > > > > >                 return PTR_ERR(arg);
> > > > > > @@ -678,6 +687,7 @@ static bool optee_ffa_api_is_compatbile(struct ffa_device *ffa_dev,
> > > > > >
> > > > > >  static bool optee_ffa_exchange_caps(struct ffa_device *ffa_dev,
> > > > > >                                     const struct ffa_dev_ops *ops,
> > > > > > +                                   u32 *sec_caps,
> > > > > >                                     unsigned int *rpc_param_count)
> > > > > >  {
> > > > > >         struct ffa_send_direct_data data = { OPTEE_FFA_EXCHANGE_CAPABILITIES };
> > > > > > @@ -694,6 +704,7 @@ static bool optee_ffa_exchange_caps(struct ffa_device *ffa_dev,
> > > > > >         }
> > > > > >
> > > > > >         *rpc_param_count = (u8)data.data1;
> > > > > > +       *sec_caps = data.data2;
> > > > > >
> > > > > >         return true;
> > > > > >  }
> > > > > > @@ -777,6 +788,7 @@ static int optee_ffa_probe(struct ffa_device *ffa_dev)
> > > > > >         struct tee_device *teedev;
> > > > > >         struct tee_context *ctx;
> > > > > >         struct optee *optee;
> > > > > > +       u32 sec_caps;
> > > > > >         int rc;
> > > > > >
> > > > > >         ffa_ops = ffa_dev_ops_get(ffa_dev);
> > > > > > @@ -788,7 +800,8 @@ static int optee_ffa_probe(struct ffa_device *ffa_dev)
> > > > > >         if (!optee_ffa_api_is_compatbile(ffa_dev, ffa_ops))
> > > > > >                 return -EINVAL;
> > > > > >
> > > > > > -       if (!optee_ffa_exchange_caps(ffa_dev, ffa_ops, &rpc_param_count))
> > > > > > +       if (!optee_ffa_exchange_caps(ffa_dev, ffa_ops, &sec_caps,
> > > > > > +                                    &rpc_param_count))
> > > > > >                 return -EINVAL;
> > > > > >
> > > > > >         optee = kzalloc(sizeof(*optee), GFP_KERNEL);
> > > > > > diff --git a/drivers/tee/optee/optee_ffa.h b/drivers/tee/optee/optee_ffa.h
> > > > > > index ee3a03fc392c..97266243deaa 100644
> > > > > > --- a/drivers/tee/optee/optee_ffa.h
> > > > > > +++ b/drivers/tee/optee/optee_ffa.h
> > > > > > @@ -81,8 +81,16 @@
> > > > > >   *                   as the second MSG arg struct for
> > > > > >   *                   OPTEE_FFA_YIELDING_CALL_WITH_ARG.
> > > > > >   *        Bit[31:8]: Reserved (MBZ)
> > > > > > - * w5-w7: Note used (MBZ)
> > > > > > + * w5:   Bitfield of secure world capabilities OPTEE_FFA_SEC_CAP_* below,
> > > > > > + *       unused bits MBZ.
> > > > > > + * w6-w7: Not used (MBZ)
> > > > > > + */
> > > > > > +/*
> > > > > > + * Secure world supports giving an offset into the argument shared memory
> > > > > > + * object, see also OPTEE_FFA_YIELDING_CALL_WITH_ARG
> > > > > >   */
> > > > > > +#define OPTEE_FFA_SEC_CAP_ARG_OFFSET   BIT(0)
> > > > > > +
> > > > > >  #define OPTEE_FFA_EXCHANGE_CAPABILITIES OPTEE_FFA_BLOCKING_CALL(2)
> > > > > >
> > > > > >  /*
> > > > > > @@ -112,6 +120,8 @@
> > > > > >   *       OPTEE_MSG_GET_ARG_SIZE(num_params) follows a struct optee_msg_arg
> > > > > >   *       for RPC, this struct has reserved space for the number of RPC
> > > > > >   *       parameters as returned by OPTEE_FFA_EXCHANGE_CAPABILITIES.
> > > > > > + *       MBZ unless the bit OPTEE_FFA_SEC_CAP_ARG_OFFSET is received with
> > > > > > + *       OPTEE_FFA_EXCHANGE_CAPABILITIES.
> > > > > >   * w7:    Not used (MBZ)
> > > > > >   * Resume from RPC. Register usage:
> > > > > >   * w3:    Service ID, OPTEE_FFA_YIELDING_CALL_RESUME
> > > > > > --
> > > > > > 2.31.1
> > > > > >

[PATCH 3/3] optee: cache argument shared memory structs

[Jens Wiklander]

Implements a cache to handle shared memory used to pass the argument
struct needed when doing a normal yielding call into secure world.

Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
---
 drivers/tee/optee/call.c          | 236 ++++++++++++++++++++++++------
 drivers/tee/optee/core.c          |   1 +
 drivers/tee/optee/ffa_abi.c       |  13 +-
 drivers/tee/optee/optee_private.h |  27 +++-
 drivers/tee/optee/smc_abi.c       |  73 +++++++--
 5 files changed, 285 insertions(+), 65 deletions(-)

diff --git a/drivers/tee/optee/call.c b/drivers/tee/optee/call.c
index 82d836df6922..608d5f4241de 100644
--- a/drivers/tee/optee/call.c
+++ b/drivers/tee/optee/call.c
@@ -11,6 +11,34 @@
 #include <linux/types.h>
 #include "optee_private.h"
 
+#define MAX_ARG_PARAM_COUNT	6
+
+/*
+ * How much memory we allocate for each entry. This doesn't have to be a
+ * single page, but it makes sense to keep at least keep it as multiples of
+ * the page size.
+ */
+#define SHM_ENTRY_SIZE		PAGE_SIZE
+
+/*
+ * We need to have a compile time constant to be able to determine the
+ * maximum needed size of the bit field.
+ */
+#define MIN_ARG_SIZE		OPTEE_MSG_GET_ARG_SIZE(MAX_ARG_PARAM_COUNT)
+#define MAX_ARG_COUNT_PER_ENTRY	(SHM_ENTRY_SIZE / MIN_ARG_SIZE)
+
+/*
+ * Shared memory for argument structs are cached here. The number of
+ * arguments structs that can fit is determined at runtime depending on the
+ * needed RPC parameter count reported by secure world
+ * (optee->rpc_param_count).
+ */
+struct optee_shm_arg_entry {
+	struct list_head list_node;
+	struct tee_shm *shm;
+	DECLARE_BITMAP(map, MAX_ARG_COUNT_PER_ENTRY);
+};
+
 void optee_cq_wait_init(struct optee_call_queue *cq,
 			struct optee_call_waiter *w)
 {
@@ -104,37 +132,149 @@ static struct optee_session *find_session(struct optee_context_data *ctxdata,
 	return NULL;
 }
 
-struct tee_shm *optee_get_msg_arg(struct tee_context *ctx, size_t num_params,
-				  struct optee_msg_arg **msg_arg)
+void optee_shm_arg_cache_init(struct optee *optee, u32 flags)
+{
+	INIT_LIST_HEAD(&optee->shm_arg_cache.shm_args);
+	mutex_init(&optee->shm_arg_cache.mutex);
+	optee->shm_arg_cache.flags = flags;
+}
+
+void optee_shm_arg_cache_uninit(struct optee *optee)
+{
+	struct list_head *head = &optee->shm_arg_cache.shm_args;
+	struct optee_shm_arg_entry *entry;
+
+	mutex_destroy(&optee->shm_arg_cache.mutex);
+	while (!list_empty(head)) {
+		entry = list_first_entry(head, struct optee_shm_arg_entry,
+					 list_node);
+		list_del(&entry->list_node);
+		if (find_first_bit(entry->map, MAX_ARG_COUNT_PER_ENTRY) !=
+		     MAX_ARG_COUNT_PER_ENTRY) {
+			pr_err("Freeing non-free entry\n");
+		}
+		tee_shm_free(entry->shm);
+		kfree(entry);
+	}
+}
+
+size_t optee_msg_arg_size(size_t rpc_param_count)
+{
+	size_t sz = OPTEE_MSG_GET_ARG_SIZE(MAX_ARG_PARAM_COUNT);
+
+	if (rpc_param_count)
+		sz += OPTEE_MSG_GET_ARG_SIZE(rpc_param_count);
+
+	return sz;
+}
+
+/**
+ * optee_get_msg_arg() - Provide shared memory for argument struct
+ * @ctx:	Caller TEE context
+ * @num_params:	Number of parameter to store
+ * @entry_ret:	Entry pointer, needed when freeing the buffer
+ * @shm_ret:	Shared memory buffer
+ * @offs_ret:	Offset of argument strut in shared memory buffer
+ *
+ * @returns a pointer to the argument struct in memory, else an ERR_PTR
+ */
+struct optee_msg_arg *optee_get_msg_arg(struct tee_context *ctx,
+					size_t num_params,
+					struct optee_shm_arg_entry **entry_ret,
+					struct tee_shm **shm_ret,
+					u_int *offs_ret)
 {
 	struct optee *optee = tee_get_drvdata(ctx->teedev);
-	size_t sz = OPTEE_MSG_GET_ARG_SIZE(num_params);
-	struct tee_shm *shm;
+	size_t sz = optee_msg_arg_size(optee->rpc_param_count);
+	struct optee_shm_arg_entry *entry;
 	struct optee_msg_arg *ma;
+	size_t args_per_entry;
+	u_long bit;
+	u_int offs;
+	void *res;
+
+	if (num_params > MAX_ARG_PARAM_COUNT)
+		return ERR_PTR(-EINVAL);
+
+	if (optee->shm_arg_cache.flags & OPTEE_SHM_ARG_SHARED)
+		args_per_entry = SHM_ENTRY_SIZE / sz;
+	else
+		args_per_entry = 1;
+
+	mutex_lock(&optee->shm_arg_cache.mutex);
+	list_for_each_entry(entry, &optee->shm_arg_cache.shm_args, list_node) {
+		bit = find_first_zero_bit(entry->map, MAX_ARG_COUNT_PER_ENTRY);
+		if (bit < args_per_entry)
+			goto have_entry;
+	}
 
 	/*
-	 * rpc_arg_count is set to the number of allocated parameters in
-	 * the RPC argument struct if a second MSG arg struct is expected.
-	 * The second arg struct will then be used for RPC.
+	 * No entry was found, let's allocate a new.
 	 */
-	if (optee->rpc_param_count)
-		sz += OPTEE_MSG_GET_ARG_SIZE(optee->rpc_param_count);
+	entry = kzalloc(sizeof(*entry), GFP_KERNEL);
+	if (!entry) {
+		res = ERR_PTR(-ENOMEM);
+		goto out;
+	}
 
-	shm = tee_shm_alloc_priv_buf(ctx, sz);
-	if (IS_ERR(shm))
-		return shm;
+	if (optee->shm_arg_cache.flags & OPTEE_SHM_ARG_ALLOC_PRIV)
+		res = tee_shm_alloc_priv_buf(ctx, SHM_ENTRY_SIZE);
+	else
+		res = tee_shm_alloc_kernel_buf(ctx, SHM_ENTRY_SIZE);
 
-	ma = tee_shm_get_va(shm, 0);
-	if (IS_ERR(ma)) {
-		tee_shm_free(shm);
-		return (void *)ma;
+	if (IS_ERR(res)) {
+		kfree(entry);
+		goto out;
 	}
-
+	entry->shm = res;
+	list_add(&entry->list_node, &optee->shm_arg_cache.shm_args);
+	bit = 0;
+
+have_entry:
+	offs = bit * sz;
+	res = tee_shm_get_va(entry->shm, offs);
+	if (IS_ERR(res))
+		goto out;
+	ma = res;
+	set_bit(bit, entry->map);
 	memset(ma, 0, sz);
 	ma->num_params = num_params;
-	*msg_arg = ma;
+	*entry_ret = entry;
+	*shm_ret = entry->shm;
+	*offs_ret = offs;
+out:
+	mutex_unlock(&optee->shm_arg_cache.mutex);
+	return res;
+}
+
+/**
+ * optee_free_msg_arg() - Free previsouly obtained shared memory
+ * @ctx:	Caller TEE context
+ * @entry:	Pointer returned when the shared memory was obtained
+ * @offs:	Offset of shared memory buffer to free
+ *
+ * This function frees the shared memory obtained with optee_get_msg_arg().
+ */
+void optee_free_msg_arg(struct tee_context *ctx,
+			struct optee_shm_arg_entry *entry, u_int offs)
+{
+	struct optee *optee = tee_get_drvdata(ctx->teedev);
+	size_t sz = optee_msg_arg_size(optee->rpc_param_count);
+	u_long bit;
 
-	return shm;
+	if (offs > SHM_ENTRY_SIZE || offs % sz) {
+		pr_err("Invalid offs %u\n", offs);
+		return;
+	}
+	bit = offs / sz;
+
+	mutex_lock(&optee->shm_arg_cache.mutex);
+
+	if (!test_bit(bit, entry->map))
+		pr_err("Bit pos %lu is already free\n", bit);
+	clear_bit(bit, entry->map);
+
+	mutex_unlock(&optee->shm_arg_cache.mutex);
 }
 
 int optee_open_session(struct tee_context *ctx,
@@ -143,16 +283,19 @@ int optee_open_session(struct tee_context *ctx,
 {
 	struct optee *optee = tee_get_drvdata(ctx->teedev);
 	struct optee_context_data *ctxdata = ctx->data;
-	int rc;
+	struct optee_shm_arg_entry *entry;
 	struct tee_shm *shm;
 	struct optee_msg_arg *msg_arg;
 	struct optee_session *sess = NULL;
 	uuid_t client_uuid;
+	u_int offs;
+	int rc;
 
 	/* +2 for the meta parameters added below */
-	shm = optee_get_msg_arg(ctx, arg->num_params + 2, &msg_arg);
-	if (IS_ERR(shm))
-		return PTR_ERR(shm);
+	msg_arg = optee_get_msg_arg(ctx, arg->num_params + 2,
+				    &entry, &shm, &offs);
+	if (IS_ERR(msg_arg))
+		return PTR_ERR(msg_arg);
 
 	msg_arg->cmd = OPTEE_MSG_CMD_OPEN_SESSION;
 	msg_arg->cancel_id = arg->cancel_id;
@@ -185,7 +328,7 @@ int optee_open_session(struct tee_context *ctx,
 		goto out;
 	}
 
-	if (optee->ops->do_call_with_arg(ctx, shm)) {
+	if (optee->ops->do_call_with_arg(ctx, shm, offs)) {
 		msg_arg->ret = TEEC_ERROR_COMMUNICATION;
 		msg_arg->ret_origin = TEEC_ORIGIN_COMMS;
 	}
@@ -212,26 +355,28 @@ int optee_open_session(struct tee_context *ctx,
 		arg->ret_origin = msg_arg->ret_origin;
 	}
 out:
-	tee_shm_free(shm);
+	optee_free_msg_arg(ctx, entry, offs);
 
 	return rc;
 }
 
 int optee_close_session_helper(struct tee_context *ctx, u32 session)
 {
-	struct tee_shm *shm;
 	struct optee *optee = tee_get_drvdata(ctx->teedev);
+	struct optee_shm_arg_entry *entry;
 	struct optee_msg_arg *msg_arg;
+	struct tee_shm *shm;
+	u_int offs;
 
-	shm = optee_get_msg_arg(ctx, 0, &msg_arg);
-	if (IS_ERR(shm))
-		return PTR_ERR(shm);
+	msg_arg = optee_get_msg_arg(ctx, 0, &entry, &shm, &offs);
+	if (IS_ERR(msg_arg))
+		return PTR_ERR(msg_arg);
 
 	msg_arg->cmd = OPTEE_MSG_CMD_CLOSE_SESSION;
 	msg_arg->session = session;
-	optee->ops->do_call_with_arg(ctx, shm);
+	optee->ops->do_call_with_arg(ctx, shm, offs);
 
-	tee_shm_free(shm);
+	optee_free_msg_arg(ctx, entry, offs);
 
 	return 0;
 }
@@ -259,9 +404,11 @@ int optee_invoke_func(struct tee_context *ctx, struct tee_ioctl_invoke_arg *arg,
 {
 	struct optee *optee = tee_get_drvdata(ctx->teedev);
 	struct optee_context_data *ctxdata = ctx->data;
-	struct tee_shm *shm;
+	struct optee_shm_arg_entry *entry;
 	struct optee_msg_arg *msg_arg;
 	struct optee_session *sess;
+	struct tee_shm *shm;
+	u_int offs;
 	int rc;
 
 	/* Check that the session is valid */
@@ -271,9 +418,10 @@ int optee_invoke_func(struct tee_context *ctx, struct tee_ioctl_invoke_arg *arg,
 	if (!sess)
 		return -EINVAL;
 
-	shm = optee_get_msg_arg(ctx, arg->num_params, &msg_arg);
-	if (IS_ERR(shm))
-		return PTR_ERR(shm);
+	msg_arg = optee_get_msg_arg(ctx, arg->num_params,
+				    &entry, &shm, &offs);
+	if (IS_ERR(msg_arg))
+		return PTR_ERR(msg_arg);
 	msg_arg->cmd = OPTEE_MSG_CMD_INVOKE_COMMAND;
 	msg_arg->func = arg->func;
 	msg_arg->session = arg->session;
@@ -284,7 +432,7 @@ int optee_invoke_func(struct tee_context *ctx, struct tee_ioctl_invoke_arg *arg,
 	if (rc)
 		goto out;
 
-	if (optee->ops->do_call_with_arg(ctx, shm)) {
+	if (optee->ops->do_call_with_arg(ctx, shm, offs)) {
 		msg_arg->ret = TEEC_ERROR_COMMUNICATION;
 		msg_arg->ret_origin = TEEC_ORIGIN_COMMS;
 	}
@@ -298,7 +446,7 @@ int optee_invoke_func(struct tee_context *ctx, struct tee_ioctl_invoke_arg *arg,
 	arg->ret = msg_arg->ret;
 	arg->ret_origin = msg_arg->ret_origin;
 out:
-	tee_shm_free(shm);
+	optee_free_msg_arg(ctx, entry, offs);
 	return rc;
 }
 
@@ -306,9 +454,11 @@ int optee_cancel_req(struct tee_context *ctx, u32 cancel_id, u32 session)
 {
 	struct optee *optee = tee_get_drvdata(ctx->teedev);
 	struct optee_context_data *ctxdata = ctx->data;
-	struct tee_shm *shm;
+	struct optee_shm_arg_entry *entry;
 	struct optee_msg_arg *msg_arg;
 	struct optee_session *sess;
+	struct tee_shm *shm;
+	u_int offs;
 
 	/* Check that the session is valid */
 	mutex_lock(&ctxdata->mutex);
@@ -317,16 +467,16 @@ int optee_cancel_req(struct tee_context *ctx, u32 cancel_id, u32 session)
 	if (!sess)
 		return -EINVAL;
 
-	shm = optee_get_msg_arg(ctx, 0, &msg_arg);
-	if (IS_ERR(shm))
-		return PTR_ERR(shm);
+	msg_arg = optee_get_msg_arg(ctx, 0, &entry, &shm, &offs);
+	if (IS_ERR(msg_arg))
+		return PTR_ERR(msg_arg);
 
 	msg_arg->cmd = OPTEE_MSG_CMD_CANCEL;
 	msg_arg->session = session;
 	msg_arg->cancel_id = cancel_id;
-	optee->ops->do_call_with_arg(ctx, shm);
+	optee->ops->do_call_with_arg(ctx, shm, offs);
 
-	tee_shm_free(shm);
+	optee_free_msg_arg(ctx, entry, offs);
 	return 0;
 }
 
diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c
index daf947e98d14..daf07737c4fd 100644
--- a/drivers/tee/optee/core.c
+++ b/drivers/tee/optee/core.c
@@ -171,6 +171,7 @@ void optee_remove_common(struct optee *optee)
 	optee_unregister_devices();
 
 	optee_notif_uninit(optee);
+	optee_shm_arg_cache_uninit(optee);
 	teedev_close_context(optee->ctx);
 	/*
 	 * The two devices have to be unregistered before we can free the
diff --git a/drivers/tee/optee/ffa_abi.c b/drivers/tee/optee/ffa_abi.c
index cc863aaefcd9..1552cd3f9d4e 100644
--- a/drivers/tee/optee/ffa_abi.c
+++ b/drivers/tee/optee/ffa_abi.c
@@ -601,6 +601,7 @@ static int optee_ffa_yielding_call(struct tee_context *ctx,
  * optee_ffa_do_call_with_arg() - Do a FF-A call to enter OP-TEE in secure world
  * @ctx:	calling context
  * @shm:	shared memory holding the message to pass to secure world
+ * @offs:	offset of the message in @shm
  *
  * Does a FF-A call to OP-TEE in secure world and handles eventual resulting
  * Remote Procedure Calls (RPC) from OP-TEE.
@@ -609,13 +610,13 @@ static int optee_ffa_yielding_call(struct tee_context *ctx,
  */
 
 static int optee_ffa_do_call_with_arg(struct tee_context *ctx,
-				      struct tee_shm *shm)
+				      struct tee_shm *shm, u_int offs)
 {
 	struct ffa_send_direct_data data = {
 		.data0 = OPTEE_FFA_YIELDING_CALL_WITH_ARG,
 		.data1 = (u32)shm->sec_world_id,
 		.data2 = (u32)(shm->sec_world_id >> 32),
-		.data3 = 0,
+		.data3 = offs,
 	};
 	struct optee_msg_arg *arg;
 	unsigned int rpc_arg_offs;
@@ -630,12 +631,12 @@ static int optee_ffa_do_call_with_arg(struct tee_context *ctx,
 	if (shm->offset)
 		return -EINVAL;
 
-	arg = tee_shm_get_va(shm, 0);
+	arg = tee_shm_get_va(shm, offs);
 	if (IS_ERR(arg))
 		return PTR_ERR(arg);
 
 	rpc_arg_offs = OPTEE_MSG_GET_ARG_SIZE(arg->num_params);
-	rpc_arg = tee_shm_get_va(shm, rpc_arg_offs);
+	rpc_arg = tee_shm_get_va(shm, offs + rpc_arg_offs);
 	if (IS_ERR(rpc_arg))
 		return PTR_ERR(rpc_arg);
 
@@ -787,6 +788,7 @@ static int optee_ffa_probe(struct ffa_device *ffa_dev)
 	struct tee_shm_pool *pool;
 	struct tee_device *teedev;
 	struct tee_context *ctx;
+	u32 arg_cache_flags = 0;
 	struct optee *optee;
 	u32 sec_caps;
 	int rc;
@@ -803,6 +805,8 @@ static int optee_ffa_probe(struct ffa_device *ffa_dev)
 	if (!optee_ffa_exchange_caps(ffa_dev, ffa_ops, &sec_caps,
 				     &rpc_param_count))
 		return -EINVAL;
+	if (sec_caps & OPTEE_FFA_SEC_CAP_ARG_OFFSET)
+		arg_cache_flags |= OPTEE_SHM_ARG_SHARED;
 
 	optee = kzalloc(sizeof(*optee), GFP_KERNEL);
 	if (!optee)
@@ -851,6 +855,7 @@ static int optee_ffa_probe(struct ffa_device *ffa_dev)
 	mutex_init(&optee->call_queue.mutex);
 	INIT_LIST_HEAD(&optee->call_queue.waiters);
 	optee_supp_init(&optee->supp);
+	optee_shm_arg_cache_init(optee, arg_cache_flags);
 	ffa_dev_set_drvdata(ffa_dev, optee);
 	ctx = teedev_open(optee->teedev);
 	if (IS_ERR(ctx)) {
diff --git a/drivers/tee/optee/optee_private.h b/drivers/tee/optee/optee_private.h
index e80c5d9b62ec..a33d98d17cfd 100644
--- a/drivers/tee/optee/optee_private.h
+++ b/drivers/tee/optee/optee_private.h
@@ -59,6 +59,16 @@ struct optee_notif {
 	u_long *bitmap;
 };
 
+#define OPTEE_SHM_ARG_ALLOC_PRIV	BIT(0)
+#define OPTEE_SHM_ARG_SHARED		BIT(1)
+struct optee_shm_arg_entry;
+struct optee_shm_arg_cache {
+	u32 flags;
+	/* Serializes access to this struct */
+	struct mutex mutex;
+	struct list_head shm_args;
+};
+
 /**
  * struct optee_supp - supplicant synchronization struct
  * @ctx			the context of current connected supplicant.
@@ -121,7 +131,7 @@ struct optee;
  */
 struct optee_ops {
 	int (*do_call_with_arg)(struct tee_context *ctx,
-				struct tee_shm *shm_arg);
+				struct tee_shm *shm_arg, u_int offs);
 	int (*to_msg_param)(struct optee *optee,
 			    struct optee_msg_param *msg_params,
 			    size_t num_params, const struct tee_param *params);
@@ -157,6 +167,7 @@ struct optee {
 		struct optee_smc smc;
 		struct optee_ffa ffa;
 	};
+	struct optee_shm_arg_cache shm_arg_cache;
 	struct optee_call_queue call_queue;
 	struct optee_notif notif;
 	struct optee_supp supp;
@@ -273,8 +284,18 @@ void optee_cq_wait_for_completion(struct optee_call_queue *cq,
 void optee_cq_wait_final(struct optee_call_queue *cq,
 			 struct optee_call_waiter *w);
 int optee_check_mem_type(unsigned long start, size_t num_pages);
-struct tee_shm *optee_get_msg_arg(struct tee_context *ctx, size_t num_params,
-				  struct optee_msg_arg **msg_arg);
+
+void optee_shm_arg_cache_init(struct optee *optee, u32 flags);
+void optee_shm_arg_cache_uninit(struct optee *optee);
+struct optee_msg_arg *optee_get_msg_arg(struct tee_context *ctx,
+					size_t num_params,
+					struct optee_shm_arg_entry **entry,
+					struct tee_shm **shm_ret,
+					u_int *offs);
+void optee_free_msg_arg(struct tee_context *ctx,
+			struct optee_shm_arg_entry *entry, u_int offs);
+size_t optee_msg_arg_size(size_t rpc_param_count);
+
 
 struct tee_shm *optee_rpc_cmd_alloc_suppl(struct tee_context *ctx, size_t sz);
 void optee_rpc_cmd_free_suppl(struct tee_context *ctx, struct tee_shm *shm);
diff --git a/drivers/tee/optee/smc_abi.c b/drivers/tee/optee/smc_abi.c
index c45cc9b7e5a7..211f34cdf55f 100644
--- a/drivers/tee/optee/smc_abi.c
+++ b/drivers/tee/optee/smc_abi.c
@@ -437,6 +437,7 @@ static int optee_shm_register(struct tee_context *ctx, struct tee_shm *shm,
 	struct optee_msg_arg *msg_arg;
 	struct tee_shm *shm_arg;
 	u64 *pages_list;
+	size_t sz;
 	int rc;
 
 	if (!num_pages)
@@ -450,15 +451,30 @@ static int optee_shm_register(struct tee_context *ctx, struct tee_shm *shm,
 	if (!pages_list)
 		return -ENOMEM;
 
-	shm_arg = optee_get_msg_arg(ctx, 1, &msg_arg);
+	/*
+	 * We're about to register shared memory we can't register shared
+	 * memory for this request or there's a catch-22.
+	 *
+	 * So in this we'll have to do the good old temporary private
+	 * allocation instead of using optee_get_msg_arg().
+	 */
+	sz = optee_msg_arg_size(optee->rpc_param_count);
+	shm_arg = tee_shm_alloc_priv_buf(ctx, sz);
 	if (IS_ERR(shm_arg)) {
 		rc = PTR_ERR(shm_arg);
 		goto out;
 	}
+	msg_arg = tee_shm_get_va(shm_arg, 0);
+	if (IS_ERR(msg_arg)) {
+		rc = PTR_ERR(msg_arg);
+		goto out;
+	}
 
 	optee_fill_pages_list(pages_list, pages, num_pages,
 			      tee_shm_get_page_offset(shm));
 
+	memset(msg_arg, 0, OPTEE_MSG_GET_ARG_SIZE(1));
+	msg_arg->num_params = 1;
 	msg_arg->cmd = OPTEE_MSG_CMD_REGISTER_SHM;
 	msg_arg->params->attr = OPTEE_MSG_ATTR_TYPE_TMEM_OUTPUT |
 				OPTEE_MSG_ATTR_NONCONTIG;
@@ -471,7 +487,7 @@ static int optee_shm_register(struct tee_context *ctx, struct tee_shm *shm,
 	msg_arg->params->u.tmem.buf_ptr = virt_to_phys(pages_list) |
 	  (tee_shm_get_page_offset(shm) & (OPTEE_MSG_NONCONTIG_PAGE_SIZE - 1));
 
-	if (optee->ops->do_call_with_arg(ctx, shm_arg) ||
+	if (optee->ops->do_call_with_arg(ctx, shm_arg, 0) ||
 	    msg_arg->ret != TEEC_SUCCESS)
 		rc = -EINVAL;
 
@@ -487,19 +503,37 @@ static int optee_shm_unregister(struct tee_context *ctx, struct tee_shm *shm)
 	struct optee_msg_arg *msg_arg;
 	struct tee_shm *shm_arg;
 	int rc = 0;
+	size_t sz;
 
-	shm_arg = optee_get_msg_arg(ctx, 1, &msg_arg);
+	/*
+	 * We're about to unregister shared memory and we may not be able
+	 * register shared memory for this request in case we're called
+	 * from optee_shm_arg_cache_uninit().
+	 *
+	 * So in order to keep things simple in this function just as in
+	 * optee_shm_register() we'll use temporary private allocation
+	 * instead of using optee_get_msg_arg().
+	 */
+	sz = optee_msg_arg_size(optee->rpc_param_count);
+	shm_arg = tee_shm_alloc_priv_buf(ctx, sz);
 	if (IS_ERR(shm_arg))
 		return PTR_ERR(shm_arg);
+	msg_arg = tee_shm_get_va(shm_arg, 0);
+	if (IS_ERR(msg_arg)) {
+		rc = PTR_ERR(msg_arg);
+		goto out;
+	}
 
+	memset(msg_arg, 0, sz);
+	msg_arg->num_params = 1;
 	msg_arg->cmd = OPTEE_MSG_CMD_UNREGISTER_SHM;
-
 	msg_arg->params[0].attr = OPTEE_MSG_ATTR_TYPE_RMEM_INPUT;
 	msg_arg->params[0].u.rmem.shm_ref = (unsigned long)shm;
 
-	if (optee->ops->do_call_with_arg(ctx, shm_arg) ||
+	if (optee->ops->do_call_with_arg(ctx, shm_arg, 0) ||
 	    msg_arg->ret != TEEC_SUCCESS)
 		rc = -EINVAL;
+out:
 	tee_shm_free(shm_arg);
 	return rc;
 }
@@ -823,6 +857,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
  * @shm:	shared memory holding the message to pass to secure world
+ * @offs:	offset of the message in @shm
  *
  * Does and SMC to OP-TEE in secure world and handles eventual resulting
  * Remote Procedure Calls (RPC) from OP-TEE.
@@ -830,7 +865,7 @@ 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 *shm)
+				      struct tee_shm *shm, u_int offs)
 {
 	struct optee *optee = tee_get_drvdata(ctx->teedev);
 	struct optee_call_waiter w;
@@ -843,12 +878,12 @@ static int optee_smc_do_call_with_arg(struct tee_context *ctx,
 		struct optee_msg_arg *arg;
 		unsigned int rpc_arg_offs;
 
-		arg = tee_shm_get_va(shm, 0);
+		arg = tee_shm_get_va(shm, offs);
 		if (IS_ERR(arg))
 			return PTR_ERR(arg);
 
 		rpc_arg_offs = OPTEE_MSG_GET_ARG_SIZE(arg->num_params);
-		rpc_arg = tee_shm_get_va(shm, rpc_arg_offs);
+		rpc_arg = tee_shm_get_va(shm, offs + rpc_arg_offs);
 		if (IS_ERR(arg))
 			return PTR_ERR(arg);
 	}
@@ -856,11 +891,11 @@ static int optee_smc_do_call_with_arg(struct tee_context *ctx,
 	if  (rpc_arg && tee_shm_is_dynamic(shm)) {
 		param.a0 = OPTEE_SMC_CALL_WITH_REGD_ARG;
 		reg_pair_from_64(&param.a1, &param.a2, (u_long)shm);
-		param.a3 = 0;
+		param.a3 = offs;
 	} else {
 		phys_addr_t parg;
 
-		rc = tee_shm_get_pa(shm, 0, &parg);
+		rc = tee_shm_get_pa(shm, offs, &parg);
 		if (rc)
 			return rc;
 
@@ -912,17 +947,19 @@ static int optee_smc_do_call_with_arg(struct tee_context *ctx,
 
 static int simple_call_with_arg(struct tee_context *ctx, u32 cmd)
 {
+	struct optee_shm_arg_entry *entry;
 	struct optee_msg_arg *msg_arg;
 	struct tee_shm *shm;
+	u_int offs;
 
-	shm = optee_get_msg_arg(ctx, 0, &msg_arg);
-	if (IS_ERR(shm))
-		return PTR_ERR(shm);
+	msg_arg = optee_get_msg_arg(ctx, 0, &entry, &shm, &offs);
+	if (IS_ERR(msg_arg))
+		return PTR_ERR(msg_arg);
 
 	msg_arg->cmd = cmd;
-	optee_smc_do_call_with_arg(ctx, shm);
+	optee_smc_do_call_with_arg(ctx, shm, offs);
 
-	tee_shm_free(shm);
+	optee_free_msg_arg(ctx, entry, offs);
 	return 0;
 }
 
@@ -1317,6 +1354,7 @@ static int optee_probe(struct platform_device *pdev)
 {
 	optee_invoke_fn *invoke_fn;
 	struct tee_shm_pool *pool = ERR_PTR(-EINVAL);
+	u32 arg_cache_flags = OPTEE_SHM_ARG_SHARED;
 	struct optee *optee = NULL;
 	void *memremaped_shm = NULL;
 	unsigned int rpc_param_count;
@@ -1349,6 +1387,9 @@ static int optee_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
+	if (!rpc_param_count)
+		arg_cache_flags |= OPTEE_SHM_ARG_ALLOC_PRIV;
+
 	/*
 	 * Try to use dynamic shared memory if possible
 	 */
@@ -1402,6 +1443,7 @@ static int optee_probe(struct platform_device *pdev)
 	optee_supp_init(&optee->supp);
 	optee->smc.memremaped_shm = memremaped_shm;
 	optee->pool = pool;
+	optee_shm_arg_cache_init(optee, arg_cache_flags);
 
 	platform_set_drvdata(pdev, optee);
 	ctx = teedev_open(optee->teedev);
@@ -1468,6 +1510,7 @@ static int optee_probe(struct platform_device *pdev)
 err_close_ctx:
 	teedev_close_context(ctx);
 err_supp_uninit:
+	optee_shm_arg_cache_uninit(optee);
 	optee_supp_uninit(&optee->supp);
 	mutex_destroy(&optee->call_queue.mutex);
 err_unreg_supp_teedev:
-- 
2.31.1