[PATCH v10 0/4] tee: introduce TEE system sssion

[PATCH v10 0/4] tee: introduce TEE system sssion

[Etienne Carriere]

This series introduces TEE system sessions for TEE service sessions that
require TEE to provision resources to prevent deadlock when clients call
the TEE.

This deadlock situation can happen when a TEE service is used by low
level system resources as for example when Linux kernel uses SCMI
service embedded in TEE for clock, reset, regulator, etc... controls.
This case is detailled in patch 2/4:

> This feature is needed to prevent a system deadlock when several TEE
> client applications invoke TEE, consuming all TEE thread contexts
> available in the secure world. The deadlock can happen in the OP-TEE
> driver for example if all these TEE threads issue an RPC call from TEE
> to Linux OS to access an eMMC RPMB partition (TEE secure storage) which
> device clock or regulator controller is accessed through an OP-TEE SCMI
> services. In that case, Linux SCMI driver must reach OP-TEE SCMI
> service without waiting until one of the consumed TEE threads is freed.

Etienne Carriere (4):
  tee: optee: system call property
  tee: system session
  tee: optee: support tracking system threads
  firmware: arm_scmi: optee: use optee system invocation

 drivers/firmware/arm_scmi/optee.c |   4 +
 drivers/tee/optee/call.c          | 152 +++++++++++++++++++++++++++---
 drivers/tee/optee/core.c          |   5 +-
 drivers/tee/optee/ffa_abi.c       |  13 +--
 drivers/tee/optee/optee_private.h |  33 ++++++-
 drivers/tee/optee/smc_abi.c       |  31 ++++--
 drivers/tee/tee_core.c            |   8 ++
 include/linux/tee_drv.h           |  16 ++++
 8 files changed, 227 insertions(+), 35 deletions(-)

-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v10 1/4] tee: optee: system call property

[Etienne Carriere]

Adds an argument to do_call_with_arg() handler to tell whether the call
is a system call or nor. This change always sets this info to false
hence no functional change.

This change prepares management of system invocation proposed in a later
change.

Reviewed-by: Sumit Garg <sumit.garg@linaro.org>
Co-developed-by: Jens Wiklander <jens.wiklander@linaro.org>
Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
Signed-off-by: Etienne Carriere <etienne.carriere@foss.st.com>
---
Changes since v9:
- Applied Sumit R-b tag:
  https://lore.kernel.org/lkml/CAFA6WYMwWFRUo719wHHsjaAUOSdo4cMa8gdHBMWidP4vC4z31g@mail.gmail.com/
- Updated my e-mail address.

No change since v8
No change since v7

Changes since v6:
- Squashed a part of patch v6 3/4 changes into this patch v7 1/4
  related to adding boolean system thread attribute into optee
  driver call queue and SMC/FF-A ABIs API functions.
- Removed local variable sys_thread set to constant false value
  and use false straight as function argument instead.
- Comment on struct optee_session::use_sys_thread being read with
  optee mutex locked is not addressed as still under discussion.

No changes since v5

Changes since v4:
- New change, extracted from PATCH v4 1/2 (tee: system invocation") and
  revised to cover preparatory changes in optee driver for system session
  support with contribution from Jens.
---
 drivers/tee/optee/call.c          | 24 +++++++++++++++++-------
 drivers/tee/optee/core.c          |  5 +++--
 drivers/tee/optee/ffa_abi.c       | 10 ++++++----
 drivers/tee/optee/optee_private.h |  9 ++++++---
 drivers/tee/optee/smc_abi.c       | 15 ++++++++-------
 5 files changed, 40 insertions(+), 23 deletions(-)

diff --git a/drivers/tee/optee/call.c b/drivers/tee/optee/call.c
index df5fb5410b72..152ae9bb1785 100644
--- a/drivers/tee/optee/call.c
+++ b/drivers/tee/optee/call.c
@@ -40,7 +40,7 @@ struct optee_shm_arg_entry {
 };
 
 void optee_cq_wait_init(struct optee_call_queue *cq,
-			struct optee_call_waiter *w)
+			struct optee_call_waiter *w, bool sys_thread)
 {
 	/*
 	 * We're preparing to make a call to secure world. In case we can't
@@ -328,7 +328,8 @@ int optee_open_session(struct tee_context *ctx,
 		goto out;
 	}
 
-	if (optee->ops->do_call_with_arg(ctx, shm, offs)) {
+	if (optee->ops->do_call_with_arg(ctx, shm, offs,
+					 sess->use_sys_thread)) {
 		msg_arg->ret = TEEC_ERROR_COMMUNICATION;
 		msg_arg->ret_origin = TEEC_ORIGIN_COMMS;
 	}
@@ -360,7 +361,8 @@ int optee_open_session(struct tee_context *ctx,
 	return rc;
 }
 
-int optee_close_session_helper(struct tee_context *ctx, u32 session)
+int optee_close_session_helper(struct tee_context *ctx, u32 session,
+			       bool system_thread)
 {
 	struct optee *optee = tee_get_drvdata(ctx->teedev);
 	struct optee_shm_arg_entry *entry;
@@ -374,7 +376,7 @@ int optee_close_session_helper(struct tee_context *ctx, u32 session)
 
 	msg_arg->cmd = OPTEE_MSG_CMD_CLOSE_SESSION;
 	msg_arg->session = session;
-	optee->ops->do_call_with_arg(ctx, shm, offs);
+	optee->ops->do_call_with_arg(ctx, shm, offs, system_thread);
 
 	optee_free_msg_arg(ctx, entry, offs);
 
@@ -385,6 +387,7 @@ int optee_close_session(struct tee_context *ctx, u32 session)
 {
 	struct optee_context_data *ctxdata = ctx->data;
 	struct optee_session *sess;
+	bool system_thread;
 
 	/* Check that the session is valid and remove it from the list */
 	mutex_lock(&ctxdata->mutex);
@@ -394,9 +397,10 @@ int optee_close_session(struct tee_context *ctx, u32 session)
 	mutex_unlock(&ctxdata->mutex);
 	if (!sess)
 		return -EINVAL;
+	system_thread = sess->use_sys_thread;
 	kfree(sess);
 
-	return optee_close_session_helper(ctx, session);
+	return optee_close_session_helper(ctx, session, system_thread);
 }
 
 int optee_invoke_func(struct tee_context *ctx, struct tee_ioctl_invoke_arg *arg,
@@ -408,12 +412,15 @@ int optee_invoke_func(struct tee_context *ctx, struct tee_ioctl_invoke_arg *arg,
 	struct optee_msg_arg *msg_arg;
 	struct optee_session *sess;
 	struct tee_shm *shm;
+	bool system_thread;
 	u_int offs;
 	int rc;
 
 	/* Check that the session is valid */
 	mutex_lock(&ctxdata->mutex);
 	sess = find_session(ctxdata, arg->session);
+	if (sess)
+		system_thread = sess->use_sys_thread;
 	mutex_unlock(&ctxdata->mutex);
 	if (!sess)
 		return -EINVAL;
@@ -432,7 +439,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, offs)) {
+	if (optee->ops->do_call_with_arg(ctx, shm, offs, system_thread)) {
 		msg_arg->ret = TEEC_ERROR_COMMUNICATION;
 		msg_arg->ret_origin = TEEC_ORIGIN_COMMS;
 	}
@@ -457,12 +464,15 @@ int optee_cancel_req(struct tee_context *ctx, u32 cancel_id, u32 session)
 	struct optee_shm_arg_entry *entry;
 	struct optee_msg_arg *msg_arg;
 	struct optee_session *sess;
+	bool system_thread;
 	struct tee_shm *shm;
 	u_int offs;
 
 	/* Check that the session is valid */
 	mutex_lock(&ctxdata->mutex);
 	sess = find_session(ctxdata, session);
+	if (sess)
+		system_thread = sess->use_sys_thread;
 	mutex_unlock(&ctxdata->mutex);
 	if (!sess)
 		return -EINVAL;
@@ -474,7 +484,7 @@ int optee_cancel_req(struct tee_context *ctx, u32 cancel_id, u32 session)
 	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, offs);
+	optee->ops->do_call_with_arg(ctx, shm, offs, system_thread);
 
 	optee_free_msg_arg(ctx, entry, offs);
 	return 0;
diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c
index 2a258bd3b6b5..d01ca47f7bde 100644
--- a/drivers/tee/optee/core.c
+++ b/drivers/tee/optee/core.c
@@ -129,7 +129,8 @@ int optee_open(struct tee_context *ctx, bool cap_memref_null)
 
 static void optee_release_helper(struct tee_context *ctx,
 				 int (*close_session)(struct tee_context *ctx,
-						      u32 session))
+						      u32 session,
+						      bool system_thread))
 {
 	struct optee_context_data *ctxdata = ctx->data;
 	struct optee_session *sess;
@@ -141,7 +142,7 @@ static void optee_release_helper(struct tee_context *ctx,
 	list_for_each_entry_safe(sess, sess_tmp, &ctxdata->sess_list,
 				 list_node) {
 		list_del(&sess->list_node);
-		close_session(ctx, sess->session_id);
+		close_session(ctx, sess->session_id, sess->use_sys_thread);
 		kfree(sess);
 	}
 	kfree(ctxdata);
diff --git a/drivers/tee/optee/ffa_abi.c b/drivers/tee/optee/ffa_abi.c
index 0828240f27e6..5fde9d4100e3 100644
--- a/drivers/tee/optee/ffa_abi.c
+++ b/drivers/tee/optee/ffa_abi.c
@@ -528,7 +528,8 @@ static void optee_handle_ffa_rpc(struct tee_context *ctx, struct optee *optee,
 
 static int optee_ffa_yielding_call(struct tee_context *ctx,
 				   struct ffa_send_direct_data *data,
-				   struct optee_msg_arg *rpc_arg)
+				   struct optee_msg_arg *rpc_arg,
+				   bool system_thread)
 {
 	struct optee *optee = tee_get_drvdata(ctx->teedev);
 	struct ffa_device *ffa_dev = optee->ffa.ffa_dev;
@@ -541,7 +542,7 @@ static int optee_ffa_yielding_call(struct tee_context *ctx,
 	int rc;
 
 	/* Initialize waiter */
-	optee_cq_wait_init(&optee->call_queue, &w);
+	optee_cq_wait_init(&optee->call_queue, &w, system_thread);
 	while (true) {
 		rc = msg_ops->sync_send_receive(ffa_dev, data);
 		if (rc)
@@ -612,7 +613,8 @@ 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, u_int offs)
+				      struct tee_shm *shm, u_int offs,
+				      bool system_thread)
 {
 	struct ffa_send_direct_data data = {
 		.data0 = OPTEE_FFA_YIELDING_CALL_WITH_ARG,
@@ -642,7 +644,7 @@ static int optee_ffa_do_call_with_arg(struct tee_context *ctx,
 	if (IS_ERR(rpc_arg))
 		return PTR_ERR(rpc_arg);
 
-	return optee_ffa_yielding_call(ctx, &data, rpc_arg);
+	return optee_ffa_yielding_call(ctx, &data, rpc_arg, system_thread);
 }
 
 /*
diff --git a/drivers/tee/optee/optee_private.h b/drivers/tee/optee/optee_private.h
index 72685ee0d53f..b68273051454 100644
--- a/drivers/tee/optee/optee_private.h
+++ b/drivers/tee/optee/optee_private.h
@@ -154,7 +154,8 @@ struct optee;
  */
 struct optee_ops {
 	int (*do_call_with_arg)(struct tee_context *ctx,
-				struct tee_shm *shm_arg, u_int offs);
+				struct tee_shm *shm_arg, u_int offs,
+				bool system_thread);
 	int (*to_msg_param)(struct optee *optee,
 			    struct optee_msg_param *msg_params,
 			    size_t num_params, const struct tee_param *params);
@@ -204,6 +205,7 @@ struct optee {
 struct optee_session {
 	struct list_head list_node;
 	u32 session_id;
+	bool use_sys_thread;
 };
 
 struct optee_context_data {
@@ -252,7 +254,8 @@ int optee_supp_send(struct tee_context *ctx, u32 ret, u32 num_params,
 int optee_open_session(struct tee_context *ctx,
 		       struct tee_ioctl_open_session_arg *arg,
 		       struct tee_param *param);
-int optee_close_session_helper(struct tee_context *ctx, u32 session);
+int optee_close_session_helper(struct tee_context *ctx, u32 session,
+			       bool system_thread);
 int optee_close_session(struct tee_context *ctx, u32 session);
 int optee_invoke_func(struct tee_context *ctx, struct tee_ioctl_invoke_arg *arg,
 		      struct tee_param *param);
@@ -301,7 +304,7 @@ static inline void optee_to_msg_param_value(struct optee_msg_param *mp,
 }
 
 void optee_cq_wait_init(struct optee_call_queue *cq,
-			struct optee_call_waiter *w);
+			struct optee_call_waiter *w, bool sys_thread);
 void optee_cq_wait_for_completion(struct optee_call_queue *cq,
 				  struct optee_call_waiter *w);
 void optee_cq_wait_final(struct optee_call_queue *cq,
diff --git a/drivers/tee/optee/smc_abi.c b/drivers/tee/optee/smc_abi.c
index d5b28fd35d66..1033d7da03ea 100644
--- a/drivers/tee/optee/smc_abi.c
+++ b/drivers/tee/optee/smc_abi.c
@@ -283,7 +283,7 @@ static void optee_enable_shm_cache(struct optee *optee)
 	struct optee_call_waiter w;
 
 	/* We need to retry until secure world isn't busy. */
-	optee_cq_wait_init(&optee->call_queue, &w);
+	optee_cq_wait_init(&optee->call_queue, &w, false);
 	while (true) {
 		struct arm_smccc_res res;
 
@@ -308,7 +308,7 @@ static void __optee_disable_shm_cache(struct optee *optee, bool is_mapped)
 	struct optee_call_waiter w;
 
 	/* We need to retry until secure world isn't busy. */
-	optee_cq_wait_init(&optee->call_queue, &w);
+	optee_cq_wait_init(&optee->call_queue, &w, false);
 	while (true) {
 		union {
 			struct arm_smccc_res smccc;
@@ -507,7 +507,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, 0) ||
+	if (optee->ops->do_call_with_arg(ctx, shm_arg, 0, false) ||
 	    msg_arg->ret != TEEC_SUCCESS)
 		rc = -EINVAL;
 
@@ -550,7 +550,7 @@ static int optee_shm_unregister(struct tee_context *ctx, struct tee_shm *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, 0) ||
+	if (optee->ops->do_call_with_arg(ctx, shm_arg, 0, false) ||
 	    msg_arg->ret != TEEC_SUCCESS)
 		rc = -EINVAL;
 out:
@@ -885,7 +885,8 @@ 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, u_int offs)
+				      struct tee_shm *shm, u_int offs,
+				      bool system_thread)
 {
 	struct optee *optee = tee_get_drvdata(ctx->teedev);
 	struct optee_call_waiter w;
@@ -926,7 +927,7 @@ static int optee_smc_do_call_with_arg(struct tee_context *ctx,
 		reg_pair_from_64(&param.a1, &param.a2, parg);
 	}
 	/* Initialize waiter */
-	optee_cq_wait_init(&optee->call_queue, &w);
+	optee_cq_wait_init(&optee->call_queue, &w, system_thread);
 	while (true) {
 		struct arm_smccc_res res;
 
@@ -977,7 +978,7 @@ static int simple_call_with_arg(struct tee_context *ctx, u32 cmd)
 		return PTR_ERR(msg_arg);
 
 	msg_arg->cmd = cmd;
-	optee_smc_do_call_with_arg(ctx, shm, offs);
+	optee_smc_do_call_with_arg(ctx, shm, offs, false);
 
 	optee_free_msg_arg(ctx, entry, offs);
 	return 0;
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v10 1/4] tee: optee: system call property

[kernel test robot]

Hi Etienne,

kernel test robot noticed the following build warnings:

[auto build test WARNING on soc/for-next]
[also build test WARNING on linus/master v6.6-rc4 next-20231003]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Etienne-Carriere/tee-optee-system-call-property/20231003-220916
base:   https://git.kernel.org/pub/scm/linux/kernel/git/soc/soc.git for-next
patch link:    https://lore.kernel.org/r/20231003140637.31346-2-etienne.carriere%40foss.st.com
patch subject: [PATCH v10 1/4] tee: optee: system call property
config: arm-allyesconfig (https://download.01.org/0day-ci/archive/20231004/202310040951.IEVtCbbL-lkp@intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231004/202310040951.IEVtCbbL-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202310040951.IEVtCbbL-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/tee/optee/ffa_abi.c:618: warning: Function parameter or member 'system_thread' not described in 'optee_ffa_do_call_with_arg'
--
   drivers/tee/optee/smc_abi.c:818: warning: Function parameter or member 'rpc_arg' not described in 'optee_handle_rpc'
>> drivers/tee/optee/smc_abi.c:890: warning: Function parameter or member 'system_thread' not described in 'optee_smc_do_call_with_arg'


vim +618 drivers/tee/optee/ffa_abi.c

4615e5a34b95e0 Jens Wiklander   2021-07-21  602  
4615e5a34b95e0 Jens Wiklander   2021-07-21  603  /**
4615e5a34b95e0 Jens Wiklander   2021-07-21  604   * optee_ffa_do_call_with_arg() - Do a FF-A call to enter OP-TEE in secure world
4615e5a34b95e0 Jens Wiklander   2021-07-21  605   * @ctx:	calling context
4615e5a34b95e0 Jens Wiklander   2021-07-21  606   * @shm:	shared memory holding the message to pass to secure world
5b4018b959149e Jens Wiklander   2022-01-25  607   * @offs:	offset of the message in @shm
4615e5a34b95e0 Jens Wiklander   2021-07-21  608   *
4615e5a34b95e0 Jens Wiklander   2021-07-21  609   * Does a FF-A call to OP-TEE in secure world and handles eventual resulting
4615e5a34b95e0 Jens Wiklander   2021-07-21  610   * Remote Procedure Calls (RPC) from OP-TEE.
4615e5a34b95e0 Jens Wiklander   2021-07-21  611   *
4615e5a34b95e0 Jens Wiklander   2021-07-21  612   * Returns return code from FF-A, 0 is OK
4615e5a34b95e0 Jens Wiklander   2021-07-21  613   */
4615e5a34b95e0 Jens Wiklander   2021-07-21  614  
4615e5a34b95e0 Jens Wiklander   2021-07-21  615  static int optee_ffa_do_call_with_arg(struct tee_context *ctx,
8f8e909e5204c3 Etienne Carriere 2023-10-03  616  				      struct tee_shm *shm, u_int offs,
8f8e909e5204c3 Etienne Carriere 2023-10-03  617  				      bool system_thread)
4615e5a34b95e0 Jens Wiklander   2021-07-21 @618  {
4615e5a34b95e0 Jens Wiklander   2021-07-21  619  	struct ffa_send_direct_data data = {
4615e5a34b95e0 Jens Wiklander   2021-07-21  620  		.data0 = OPTEE_FFA_YIELDING_CALL_WITH_ARG,
4615e5a34b95e0 Jens Wiklander   2021-07-21  621  		.data1 = (u32)shm->sec_world_id,
4615e5a34b95e0 Jens Wiklander   2021-07-21  622  		.data2 = (u32)(shm->sec_world_id >> 32),
5b4018b959149e Jens Wiklander   2022-01-25  623  		.data3 = offs,
4615e5a34b95e0 Jens Wiklander   2021-07-21  624  	};
4064c461148ab1 Jens Wiklander   2021-12-28  625  	struct optee_msg_arg *arg;
4064c461148ab1 Jens Wiklander   2021-12-28  626  	unsigned int rpc_arg_offs;
4064c461148ab1 Jens Wiklander   2021-12-28  627  	struct optee_msg_arg *rpc_arg;
4064c461148ab1 Jens Wiklander   2021-12-28  628  
a639b2b18a240d Jens Wiklander   2022-01-12  629  	/*
a639b2b18a240d Jens Wiklander   2022-01-12  630  	 * The shared memory object has to start on a page when passed as
a639b2b18a240d Jens Wiklander   2022-01-12  631  	 * an argument struct. This is also what the shm pool allocator
a639b2b18a240d Jens Wiklander   2022-01-12  632  	 * returns, but check this before calling secure world to catch
a639b2b18a240d Jens Wiklander   2022-01-12  633  	 * eventual errors early in case something changes.
a639b2b18a240d Jens Wiklander   2022-01-12  634  	 */
a639b2b18a240d Jens Wiklander   2022-01-12  635  	if (shm->offset)
a639b2b18a240d Jens Wiklander   2022-01-12  636  		return -EINVAL;
a639b2b18a240d Jens Wiklander   2022-01-12  637  
5b4018b959149e Jens Wiklander   2022-01-25  638  	arg = tee_shm_get_va(shm, offs);
4064c461148ab1 Jens Wiklander   2021-12-28  639  	if (IS_ERR(arg))
4064c461148ab1 Jens Wiklander   2021-12-28  640  		return PTR_ERR(arg);
4064c461148ab1 Jens Wiklander   2021-12-28  641  
4064c461148ab1 Jens Wiklander   2021-12-28  642  	rpc_arg_offs = OPTEE_MSG_GET_ARG_SIZE(arg->num_params);
5b4018b959149e Jens Wiklander   2022-01-25  643  	rpc_arg = tee_shm_get_va(shm, offs + rpc_arg_offs);
4064c461148ab1 Jens Wiklander   2021-12-28  644  	if (IS_ERR(rpc_arg))
4064c461148ab1 Jens Wiklander   2021-12-28  645  		return PTR_ERR(rpc_arg);
4615e5a34b95e0 Jens Wiklander   2021-07-21  646  
8f8e909e5204c3 Etienne Carriere 2023-10-03  647  	return optee_ffa_yielding_call(ctx, &data, rpc_arg, system_thread);
4615e5a34b95e0 Jens Wiklander   2021-07-21  648  }
4615e5a34b95e0 Jens Wiklander   2021-07-21  649  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v10 2/4] tee: system session

[Etienne Carriere]

Adds kernel client API function tee_client_system_session() for a client
to request a system service entry in TEE context.

This feature is needed to prevent a system deadlock when several TEE
client applications invoke TEE, consuming all TEE thread contexts
available in the secure world. The deadlock can happen in the OP-TEE
driver for example if all these TEE threads issue an RPC call from TEE
to Linux OS to access an eMMC RPMB partition (TEE secure storage) which
device clock or regulator controller is accessed through an OP-TEE SCMI
services. In that case, Linux SCMI driver must reach OP-TEE SCMI service
without waiting until one of the consumed TEE threads is freed.

Reviewed-by: Sumit Garg <sumit.garg@linaro.org>
Co-developed-by: Jens Wiklander <jens.wiklander@linaro.org>
Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
Signed-off-by: Etienne Carriere <etienne.carriere@foss.st.com>
---
Changes since v9:
- Updated my e-mail address.

No change since v8
No change since v7

Changes since v6:
- Fixed commit message from review comments.
- Applied Sumit's R-b tag.

No change since v5

Changes since v4:
- Changes extracted from "[PATCH v4 1/2] tee: system invocation" and
  revised with Jens contribution to cover only definition of tee driver
  new API function tee_client_system_session() for kernel clients to
  register their session as a system session.
- Commit message rephrased, including header line changed from
  "tee: system invocation" to "tee: system session" has the feature
  relates to system attributes of tee sessions.

Changes since v3:
- Fixed new SMC funcIDs to reserved/unreserve OP-TEE thread contexts:
  minor renaming + define as fastcall funcIDs.
- Moved system_ctx_count from generic struct tee_context to optee's
  private struct optee_context_data. This changes optee smc_abi.c
  to release reserved thread contexts when the optee device is released.
- Fixed inline description comments.

No change since v2

Change since v1
- Addressed comment on Linux client to claim reservation on TEE context.
  This brings 2 new operations from client to TEE to request and release
  system thread contexts: 2 new tee_drv.h API functions, 2 new ops
  functions in struct tee_driver_ops. The OP-TEE implement shall implement
  2 new fastcall SMC funcIDs.
- Fixed typos in commit message.
---
 drivers/tee/tee_core.c  |  8 ++++++++
 include/linux/tee_drv.h | 16 ++++++++++++++++
 2 files changed, 24 insertions(+)

diff --git a/drivers/tee/tee_core.c b/drivers/tee/tee_core.c
index 0eb342de0b00..91932835d0f7 100644
--- a/drivers/tee/tee_core.c
+++ b/drivers/tee/tee_core.c
@@ -1170,6 +1170,14 @@ int tee_client_close_session(struct tee_context *ctx, u32 session)
 }
 EXPORT_SYMBOL_GPL(tee_client_close_session);
 
+int tee_client_system_session(struct tee_context *ctx, u32 session)
+{
+	if (!ctx->teedev->desc->ops->system_session)
+		return -EINVAL;
+	return ctx->teedev->desc->ops->system_session(ctx, session);
+}
+EXPORT_SYMBOL_GPL(tee_client_system_session);
+
 int tee_client_invoke_func(struct tee_context *ctx,
 			   struct tee_ioctl_invoke_arg *arg,
 			   struct tee_param *param)
diff --git a/include/linux/tee_drv.h b/include/linux/tee_drv.h
index 17eb1c5205d3..911ddf92dcee 100644
--- a/include/linux/tee_drv.h
+++ b/include/linux/tee_drv.h
@@ -84,6 +84,7 @@ struct tee_param {
  * @release:		release this open file
  * @open_session:	open a new session
  * @close_session:	close a session
+ * @system_session:	declare session as a system session
  * @invoke_func:	invoke a trusted function
  * @cancel_req:		request cancel of an ongoing invoke or open
  * @supp_recv:		called for supplicant to get a command
@@ -100,6 +101,7 @@ struct tee_driver_ops {
 			    struct tee_ioctl_open_session_arg *arg,
 			    struct tee_param *param);
 	int (*close_session)(struct tee_context *ctx, u32 session);
+	int (*system_session)(struct tee_context *ctx, u32 session);
 	int (*invoke_func)(struct tee_context *ctx,
 			   struct tee_ioctl_invoke_arg *arg,
 			   struct tee_param *param);
@@ -429,6 +431,20 @@ int tee_client_open_session(struct tee_context *ctx,
  */
 int tee_client_close_session(struct tee_context *ctx, u32 session);
 
+/**
+ * tee_client_system_session() - Declare session as a system session
+ * @ctx:	TEE Context
+ * @session:	Session id
+ *
+ * This function requests TEE to provision an entry context ready to use for
+ * that session only. The provisioned entry context is used for command
+ * invocation and session closure, not for command cancelling requests.
+ * TEE releases the provisioned context upon session closure.
+ *
+ * Return < 0 on error else 0 if an entry context has been provisioned.
+ */
+int tee_client_system_session(struct tee_context *ctx, u32 session);
+
 /**
  * tee_client_invoke_func() - Invoke a function in a Trusted Application
  * @ctx:	TEE Context
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v10 3/4] tee: optee: support tracking system threads

[Etienne Carriere]

Adds support in the OP-TEE driver to keep track of reserved system
threads. The logic allows one OP-TEE thread to be reserved to TEE system
sessions.

The optee_cq_*() functions are updated to handle this if enabled,
that is when TEE describes how many thread context it supports
and when at least 1 session has registered as a system session
(using tee_client_system_session()).

For sake of simplicity, initialization of call queue management
is factorized into new helper function optee_cq_init().

The SMC ABI part of the driver enables this tracking, but the
FF-A ABI part does not.


Co-developed-by: Jens Wiklander <jens.wiklander@linaro.org>
Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
Co-developed-by: Sumit Garg <sumit.garg@linaro.org>
Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
Signed-off-by: Etienne Carriere <etienne.carriere@foss.st.com>
---
Changes since v9:
- Add a reference counter for TEE system thread provisioning. We reserve
  a TEE thread context for system session only when there is at least
  1 opened system session.
- Use 2 wait queue lists, normal_waiters and sys_waiter, as proposed in
  patch v8. Using a single list can prevent a waiting system thread from
  being resumed if the executing system thread wakes a normal waiter in
  the list.
- Updated my e-mail address.
- Rephrased a bit the commit message.

Changes since patch v8
- Patch v9 (reference below) attempted to simplify the implementation
  https://lore.kernel.org/lkml/20230517143311.585080-1-sumit.garg@linaro.org/#t

Changes since v7:
- Changes the logic to reserve at most 1 call entry for system sessions
  as per patches v6 and v7 discussion threads (the 2 below bullets)
  and updates commit message accordingly.
- Field optee_call_queue::res_sys_thread_count is replaced with 2 fields:
  sys_thread_req_count and boolean sys_thread_in_use.
- Field optee_call_waiter::sys_thread is replaced with 2 fields:
  sys_thread_req and sys_thread_used.
- Adds inline description comments for struct optee_call_queue and
  struct optee_call_waiter.

Changes since v6:
- Moved out changes related to adding boolean system thread attribute
  into optee driver call queue and SMC/FF-A ABIs API functions. These
  changes were squashed into patch 1/4 of this patch v7 series.
- Comment about adding a specific commit for call queue refactoring
  was not addressed such a patch would only introduce function
  optee_cq_init()  with very little content in (mutex & list init).
- Added Co-developed-by tag for Jens contribution as he's not responsible
  for the changes I made in this patch v7.

No change since v5

Changes since v4:
- New change that supersedes implementation proposed in PATCH v4
  (tee: system invocation"). Thanks to Jens implementation we don't need
  the new OP-TEE services that my previous patch versions introduced to
  monitor system threads entry. Now, Linux optee SMC ABI driver gets TEE
  provisioned thread contexts count once and monitors thread entries in
  OP-TEE on that basis and the system thread capability of the related
  tee session. By the way, I dropped the WARN_ONCE() call I suggested
  on tee thread exhaustion as it does not provides useful information.
---
 drivers/tee/optee/call.c          | 128 ++++++++++++++++++++++++++++--
 drivers/tee/optee/ffa_abi.c       |   3 +-
 drivers/tee/optee/optee_private.h |  24 +++++-
 drivers/tee/optee/smc_abi.c       |  16 +++-
 4 files changed, 159 insertions(+), 12 deletions(-)

diff --git a/drivers/tee/optee/call.c b/drivers/tee/optee/call.c
index 152ae9bb1785..38543538d77b 100644
--- a/drivers/tee/optee/call.c
+++ b/drivers/tee/optee/call.c
@@ -39,9 +39,31 @@ struct optee_shm_arg_entry {
 	DECLARE_BITMAP(map, MAX_ARG_COUNT_PER_ENTRY);
 };
 
+void optee_cq_init(struct optee_call_queue *cq, int thread_count)
+{
+	mutex_init(&cq->mutex);
+	INIT_LIST_HEAD(&cq->sys_waiters);
+	INIT_LIST_HEAD(&cq->normal_waiters);
+
+	/*
+	 * If cq->total_thread_count is 0 then we're not trying to keep
+	 * track of how many free threads we have, instead we're relying on
+	 * the secure world to tell us when we're out of thread and have to
+	 * wait for another thread to become available.
+	 */
+	cq->total_thread_count = thread_count;
+	cq->free_thread_count = thread_count;
+}
+
 void optee_cq_wait_init(struct optee_call_queue *cq,
 			struct optee_call_waiter *w, bool sys_thread)
 {
+	unsigned int free_thread_threshold;
+	bool need_wait = false;
+
+	memset(w, 0, sizeof(*w));
+	w->sys_thread = sys_thread;
+
 	/*
 	 * We're preparing to make a call to secure world. In case we can't
 	 * allocate a thread in secure world we'll end up waiting in
@@ -53,15 +75,47 @@ void optee_cq_wait_init(struct optee_call_queue *cq,
 	mutex_lock(&cq->mutex);
 
 	/*
-	 * We add ourselves to the queue, but we don't wait. This
-	 * guarantees that we don't lose a completion if secure world
-	 * returns busy and another thread just exited and try to complete
-	 * someone.
+	 * We add ourselves to a queue, but we don't wait. This guarantees
+	 * that we don't lose a completion if secure world returns busy and
+	 * another thread just exited and try to complete someone.
 	 */
 	init_completion(&w->c);
-	list_add_tail(&w->list_node, &cq->waiters);
+
+	if (sys_thread)
+		list_add_tail(&w->list_node, &cq->sys_waiters);
+	else
+		list_add_tail(&w->list_node, &cq->normal_waiters);
+
+	if (cq->total_thread_count) {
+		if (sys_thread || !cq->sys_thread_req_count)
+			free_thread_threshold = 0;
+		else
+			free_thread_threshold = 1;
+
+		if (cq->free_thread_count > free_thread_threshold)
+			cq->free_thread_count--;
+		else
+			need_wait = true;
+	}
 
 	mutex_unlock(&cq->mutex);
+
+	while (need_wait) {
+		optee_cq_wait_for_completion(cq, w);
+		mutex_lock(&cq->mutex);
+
+		if (sys_thread || !cq->sys_thread_req_count)
+			free_thread_threshold = 0;
+		else
+			free_thread_threshold = 1;
+
+		if (cq->free_thread_count > free_thread_threshold) {
+			cq->free_thread_count--;
+			need_wait = false;
+		}
+
+		mutex_unlock(&cq->mutex);
+	}
 }
 
 void optee_cq_wait_for_completion(struct optee_call_queue *cq,
@@ -74,7 +128,11 @@ void optee_cq_wait_for_completion(struct optee_call_queue *cq,
 	/* Move to end of list to get out of the way for other waiters */
 	list_del(&w->list_node);
 	reinit_completion(&w->c);
-	list_add_tail(&w->list_node, &cq->waiters);
+
+	if (w->sys_thread)
+		list_add_tail(&w->list_node, &cq->sys_waiters);
+	else
+		list_add_tail(&w->list_node, &cq->normal_waiters);
 
 	mutex_unlock(&cq->mutex);
 }
@@ -83,7 +141,15 @@ static void optee_cq_complete_one(struct optee_call_queue *cq)
 {
 	struct optee_call_waiter *w;
 
-	list_for_each_entry(w, &cq->waiters, list_node) {
+	/* Wake waiting system session first */
+	list_for_each_entry(w, &cq->sys_waiters, list_node) {
+		if (!completion_done(&w->c)) {
+			complete(&w->c);
+			break;
+		}
+	}
+
+	list_for_each_entry(w, &cq->normal_waiters, list_node) {
 		if (!completion_done(&w->c)) {
 			complete(&w->c);
 			break;
@@ -104,6 +170,8 @@ void optee_cq_wait_final(struct optee_call_queue *cq,
 	/* Get out of the list */
 	list_del(&w->list_node);
 
+	cq->free_thread_count++;
+
 	/* Wake up one eventual waiting task */
 	optee_cq_complete_one(cq);
 
@@ -119,6 +187,28 @@ void optee_cq_wait_final(struct optee_call_queue *cq,
 	mutex_unlock(&cq->mutex);
 }
 
+/* Count registered system sessions to reserved a system thread or not */
+static bool optee_cq_incr_sys_thread_count(struct optee_call_queue *cq)
+{
+	if (cq->total_thread_count <= 1)
+		return false;
+
+	mutex_lock(&cq->mutex);
+	cq->sys_thread_req_count++;
+	mutex_unlock(&cq->mutex);
+
+	return true;
+}
+
+static void optee_cq_decr_sys_thread_count(struct optee_call_queue *cq)
+{
+	mutex_lock(&cq->mutex);
+	cq->sys_thread_req_count--;
+	/* If there's someone waiting, let it resume */
+	optee_cq_complete_one(cq);
+	mutex_unlock(&cq->mutex);
+}
+
 /* Requires the filpstate mutex to be held */
 static struct optee_session *find_session(struct optee_context_data *ctxdata,
 					  u32 session_id)
@@ -361,6 +451,27 @@ int optee_open_session(struct tee_context *ctx,
 	return rc;
 }
 
+int optee_system_session(struct tee_context *ctx, u32 session)
+{
+	struct optee *optee = tee_get_drvdata(ctx->teedev);
+	struct optee_context_data *ctxdata = ctx->data;
+	struct optee_session *sess;
+	int rc = -EINVAL;
+
+	mutex_lock(&ctxdata->mutex);
+
+	sess = find_session(ctxdata, session);
+	if (sess && (sess->use_sys_thread ||
+		     optee_cq_incr_sys_thread_count(&optee->call_queue))) {
+		sess->use_sys_thread = true;
+		rc = 0;
+	}
+
+	mutex_unlock(&ctxdata->mutex);
+
+	return rc;
+}
+
 int optee_close_session_helper(struct tee_context *ctx, u32 session,
 			       bool system_thread)
 {
@@ -380,6 +491,9 @@ int optee_close_session_helper(struct tee_context *ctx, u32 session,
 
 	optee_free_msg_arg(ctx, entry, offs);
 
+	if (system_thread)
+		optee_cq_decr_sys_thread_count(&optee->call_queue);
+
 	return 0;
 }
 
diff --git a/drivers/tee/optee/ffa_abi.c b/drivers/tee/optee/ffa_abi.c
index 5fde9d4100e3..0c9055691343 100644
--- a/drivers/tee/optee/ffa_abi.c
+++ b/drivers/tee/optee/ffa_abi.c
@@ -852,8 +852,7 @@ static int optee_ffa_probe(struct ffa_device *ffa_dev)
 	if (rc)
 		goto err_unreg_supp_teedev;
 	mutex_init(&optee->ffa.mutex);
-	mutex_init(&optee->call_queue.mutex);
-	INIT_LIST_HEAD(&optee->call_queue.waiters);
+	optee_cq_init(&optee->call_queue, 0);
 	optee_supp_init(&optee->supp);
 	optee_shm_arg_cache_init(optee, arg_cache_flags);
 	ffa_dev_set_drvdata(ffa_dev, optee);
diff --git a/drivers/tee/optee/optee_private.h b/drivers/tee/optee/optee_private.h
index b68273051454..69f6397c3646 100644
--- a/drivers/tee/optee/optee_private.h
+++ b/drivers/tee/optee/optee_private.h
@@ -40,15 +40,35 @@ typedef void (optee_invoke_fn)(unsigned long, unsigned long, unsigned long,
 				unsigned long, unsigned long,
 				struct arm_smccc_res *);
 
+/*
+ * struct optee_call_waiter - TEE entry may need to wait for a free TEE thread
+ * @list_node		Reference in waiters list
+ * @c			Waiting completion reference
+ * @sys_thread_req	True if waiter belongs to a system thread
+ */
 struct optee_call_waiter {
 	struct list_head list_node;
 	struct completion c;
+	bool sys_thread;
 };
 
+/*
+ * struct optee_call_queue - OP-TEE call queue management
+ * @mutex			Serializes access to this struct
+ * @sys_waiters			List of system threads waiting to enter OP-TEE
+ * @normal_waiters		List of normal threads waiting to enter OP-TEE
+ * @total_thread_count		Overall number of thread context in OP-TEE or 0
+ * @free_thread_count		Number of threads context free in OP-TEE
+ * @sys_thread_req_count	Number of registered system thread sessions
+ */
 struct optee_call_queue {
 	/* Serializes access to this struct */
 	struct mutex mutex;
-	struct list_head waiters;
+	struct list_head sys_waiters;
+	struct list_head normal_waiters;
+	int total_thread_count;
+	int free_thread_count;
+	int sys_thread_req_count;
 };
 
 struct optee_notif {
@@ -254,6 +274,7 @@ int optee_supp_send(struct tee_context *ctx, u32 ret, u32 num_params,
 int optee_open_session(struct tee_context *ctx,
 		       struct tee_ioctl_open_session_arg *arg,
 		       struct tee_param *param);
+int optee_system_session(struct tee_context *ctx, u32 session);
 int optee_close_session_helper(struct tee_context *ctx, u32 session,
 			       bool system_thread);
 int optee_close_session(struct tee_context *ctx, u32 session);
@@ -303,6 +324,7 @@ static inline void optee_to_msg_param_value(struct optee_msg_param *mp,
 	mp->u.value.c = p->u.value.c;
 }
 
+void optee_cq_init(struct optee_call_queue *cq, int thread_count);
 void optee_cq_wait_init(struct optee_call_queue *cq,
 			struct optee_call_waiter *w, bool sys_thread);
 void optee_cq_wait_for_completion(struct optee_call_queue *cq,
diff --git a/drivers/tee/optee/smc_abi.c b/drivers/tee/optee/smc_abi.c
index 1033d7da03ea..5595028d6dae 100644
--- a/drivers/tee/optee/smc_abi.c
+++ b/drivers/tee/optee/smc_abi.c
@@ -1211,6 +1211,7 @@ static const struct tee_driver_ops optee_clnt_ops = {
 	.release = optee_release,
 	.open_session = optee_open_session,
 	.close_session = optee_close_session,
+	.system_session = optee_system_session,
 	.invoke_func = optee_invoke_func,
 	.cancel_req = optee_cancel_req,
 	.shm_register = optee_shm_register,
@@ -1358,6 +1359,16 @@ static bool optee_msg_exchange_capabilities(optee_invoke_fn *invoke_fn,
 	return true;
 }
 
+static unsigned int optee_msg_get_thread_count(optee_invoke_fn *invoke_fn)
+{
+	struct arm_smccc_res res;
+
+	invoke_fn(OPTEE_SMC_GET_THREAD_COUNT, 0, 0, 0, 0, 0, 0, 0, &res);
+	if (res.a0)
+		return 0;
+	return res.a1;
+}
+
 static struct tee_shm_pool *
 optee_config_shm_memremap(optee_invoke_fn *invoke_fn, void **memremaped_shm)
 {
@@ -1610,6 +1621,7 @@ static int optee_probe(struct platform_device *pdev)
 	struct optee *optee = NULL;
 	void *memremaped_shm = NULL;
 	unsigned int rpc_param_count;
+	unsigned int thread_count;
 	struct tee_device *teedev;
 	struct tee_context *ctx;
 	u32 max_notif_value;
@@ -1637,6 +1649,7 @@ static int optee_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
+	thread_count = optee_msg_get_thread_count(invoke_fn);
 	if (!optee_msg_exchange_capabilities(invoke_fn, &sec_caps,
 					     &max_notif_value,
 					     &rpc_param_count)) {
@@ -1726,8 +1739,7 @@ static int optee_probe(struct platform_device *pdev)
 	if (rc)
 		goto err_unreg_supp_teedev;
 
-	mutex_init(&optee->call_queue.mutex);
-	INIT_LIST_HEAD(&optee->call_queue.waiters);
+	optee_cq_init(&optee->call_queue, thread_count);
 	optee_supp_init(&optee->supp);
 	optee->smc.memremaped_shm = memremaped_shm;
 	optee->pool = pool;
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v10 3/4] tee: optee: support tracking system threads

[Sumit Garg]

On Tue, 3 Oct 2023 at 19:36, Etienne Carriere
<etienne.carriere@foss.st.com> wrote:
>
> Adds support in the OP-TEE driver to keep track of reserved system
> threads. The logic allows one OP-TEE thread to be reserved to TEE system
> sessions.
>
> The optee_cq_*() functions are updated to handle this if enabled,
> that is when TEE describes how many thread context it supports
> and when at least 1 session has registered as a system session
> (using tee_client_system_session()).
>
> For sake of simplicity, initialization of call queue management
> is factorized into new helper function optee_cq_init().
>
> The SMC ABI part of the driver enables this tracking, but the
> FF-A ABI part does not.
>
>
> Co-developed-by: Jens Wiklander <jens.wiklander@linaro.org>
> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> Co-developed-by: Sumit Garg <sumit.garg@linaro.org>
> Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> Signed-off-by: Etienne Carriere <etienne.carriere@foss.st.com>
> ---
> Changes since v9:
> - Add a reference counter for TEE system thread provisioning. We reserve
>   a TEE thread context for system session only when there is at least
>   1 opened system session.
> - Use 2 wait queue lists, normal_waiters and sys_waiter, as proposed in
>   patch v8. Using a single list can prevent a waiting system thread from
>   being resumed if the executing system thread wakes a normal waiter in
>   the list.

How would that be possible? The system thread wakeup
(free_thread_threshold = 0) is given priority over normal thread
wakeup (free_thread_threshold = 1). I think a single queue list would
be sufficient as demonstrated in v9.

-Sumit

> - Updated my e-mail address.
> - Rephrased a bit the commit message.
>
> Changes since patch v8
> - Patch v9 (reference below) attempted to simplify the implementation
>   https://lore.kernel.org/lkml/20230517143311.585080-1-sumit.garg@linaro.org/#t
>
> Changes since v7:
> - Changes the logic to reserve at most 1 call entry for system sessions
>   as per patches v6 and v7 discussion threads (the 2 below bullets)
>   and updates commit message accordingly.
> - Field optee_call_queue::res_sys_thread_count is replaced with 2 fields:
>   sys_thread_req_count and boolean sys_thread_in_use.
> - Field optee_call_waiter::sys_thread is replaced with 2 fields:
>   sys_thread_req and sys_thread_used.
> - Adds inline description comments for struct optee_call_queue and
>   struct optee_call_waiter.
>
> Changes since v6:
> - Moved out changes related to adding boolean system thread attribute
>   into optee driver call queue and SMC/FF-A ABIs API functions. These
>   changes were squashed into patch 1/4 of this patch v7 series.
> - Comment about adding a specific commit for call queue refactoring
>   was not addressed such a patch would only introduce function
>   optee_cq_init()  with very little content in (mutex & list init).
> - Added Co-developed-by tag for Jens contribution as he's not responsible
>   for the changes I made in this patch v7.
>
> No change since v5
>
> Changes since v4:
> - New change that supersedes implementation proposed in PATCH v4
>   (tee: system invocation"). Thanks to Jens implementation we don't need
>   the new OP-TEE services that my previous patch versions introduced to
>   monitor system threads entry. Now, Linux optee SMC ABI driver gets TEE
>   provisioned thread contexts count once and monitors thread entries in
>   OP-TEE on that basis and the system thread capability of the related
>   tee session. By the way, I dropped the WARN_ONCE() call I suggested
>   on tee thread exhaustion as it does not provides useful information.
> ---
>  drivers/tee/optee/call.c          | 128 ++++++++++++++++++++++++++++--
>  drivers/tee/optee/ffa_abi.c       |   3 +-
>  drivers/tee/optee/optee_private.h |  24 +++++-
>  drivers/tee/optee/smc_abi.c       |  16 +++-
>  4 files changed, 159 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/tee/optee/call.c b/drivers/tee/optee/call.c
> index 152ae9bb1785..38543538d77b 100644
> --- a/drivers/tee/optee/call.c
> +++ b/drivers/tee/optee/call.c
> @@ -39,9 +39,31 @@ struct optee_shm_arg_entry {
>         DECLARE_BITMAP(map, MAX_ARG_COUNT_PER_ENTRY);
>  };
>
> +void optee_cq_init(struct optee_call_queue *cq, int thread_count)
> +{
> +       mutex_init(&cq->mutex);
> +       INIT_LIST_HEAD(&cq->sys_waiters);
> +       INIT_LIST_HEAD(&cq->normal_waiters);
> +
> +       /*
> +        * If cq->total_thread_count is 0 then we're not trying to keep
> +        * track of how many free threads we have, instead we're relying on
> +        * the secure world to tell us when we're out of thread and have to
> +        * wait for another thread to become available.
> +        */
> +       cq->total_thread_count = thread_count;
> +       cq->free_thread_count = thread_count;
> +}
> +
>  void optee_cq_wait_init(struct optee_call_queue *cq,
>                         struct optee_call_waiter *w, bool sys_thread)
>  {
> +       unsigned int free_thread_threshold;
> +       bool need_wait = false;
> +
> +       memset(w, 0, sizeof(*w));
> +       w->sys_thread = sys_thread;
> +
>         /*
>          * We're preparing to make a call to secure world. In case we can't
>          * allocate a thread in secure world we'll end up waiting in
> @@ -53,15 +75,47 @@ void optee_cq_wait_init(struct optee_call_queue *cq,
>         mutex_lock(&cq->mutex);
>
>         /*
> -        * We add ourselves to the queue, but we don't wait. This
> -        * guarantees that we don't lose a completion if secure world
> -        * returns busy and another thread just exited and try to complete
> -        * someone.
> +        * We add ourselves to a queue, but we don't wait. This guarantees
> +        * that we don't lose a completion if secure world returns busy and
> +        * another thread just exited and try to complete someone.
>          */
>         init_completion(&w->c);
> -       list_add_tail(&w->list_node, &cq->waiters);
> +
> +       if (sys_thread)
> +               list_add_tail(&w->list_node, &cq->sys_waiters);
> +       else
> +               list_add_tail(&w->list_node, &cq->normal_waiters);
> +
> +       if (cq->total_thread_count) {
> +               if (sys_thread || !cq->sys_thread_req_count)
> +                       free_thread_threshold = 0;
> +               else
> +                       free_thread_threshold = 1;
> +
> +               if (cq->free_thread_count > free_thread_threshold)
> +                       cq->free_thread_count--;
> +               else
> +                       need_wait = true;
> +       }
>
>         mutex_unlock(&cq->mutex);
> +
> +       while (need_wait) {
> +               optee_cq_wait_for_completion(cq, w);
> +               mutex_lock(&cq->mutex);
> +
> +               if (sys_thread || !cq->sys_thread_req_count)
> +                       free_thread_threshold = 0;
> +               else
> +                       free_thread_threshold = 1;
> +
> +               if (cq->free_thread_count > free_thread_threshold) {
> +                       cq->free_thread_count--;
> +                       need_wait = false;
> +               }
> +
> +               mutex_unlock(&cq->mutex);
> +       }
>  }
>
>  void optee_cq_wait_for_completion(struct optee_call_queue *cq,
> @@ -74,7 +128,11 @@ void optee_cq_wait_for_completion(struct optee_call_queue *cq,
>         /* Move to end of list to get out of the way for other waiters */
>         list_del(&w->list_node);
>         reinit_completion(&w->c);
> -       list_add_tail(&w->list_node, &cq->waiters);
> +
> +       if (w->sys_thread)
> +               list_add_tail(&w->list_node, &cq->sys_waiters);
> +       else
> +               list_add_tail(&w->list_node, &cq->normal_waiters);
>
>         mutex_unlock(&cq->mutex);
>  }
> @@ -83,7 +141,15 @@ static void optee_cq_complete_one(struct optee_call_queue *cq)
>  {
>         struct optee_call_waiter *w;
>
> -       list_for_each_entry(w, &cq->waiters, list_node) {
> +       /* Wake waiting system session first */
> +       list_for_each_entry(w, &cq->sys_waiters, list_node) {
> +               if (!completion_done(&w->c)) {
> +                       complete(&w->c);
> +                       break;
> +               }
> +       }
> +
> +       list_for_each_entry(w, &cq->normal_waiters, list_node) {
>                 if (!completion_done(&w->c)) {
>                         complete(&w->c);
>                         break;
> @@ -104,6 +170,8 @@ void optee_cq_wait_final(struct optee_call_queue *cq,
>         /* Get out of the list */
>         list_del(&w->list_node);
>
> +       cq->free_thread_count++;
> +
>         /* Wake up one eventual waiting task */
>         optee_cq_complete_one(cq);
>
> @@ -119,6 +187,28 @@ void optee_cq_wait_final(struct optee_call_queue *cq,
>         mutex_unlock(&cq->mutex);
>  }
>
> +/* Count registered system sessions to reserved a system thread or not */
> +static bool optee_cq_incr_sys_thread_count(struct optee_call_queue *cq)
> +{
> +       if (cq->total_thread_count <= 1)
> +               return false;
> +
> +       mutex_lock(&cq->mutex);
> +       cq->sys_thread_req_count++;
> +       mutex_unlock(&cq->mutex);
> +
> +       return true;
> +}
> +
> +static void optee_cq_decr_sys_thread_count(struct optee_call_queue *cq)
> +{
> +       mutex_lock(&cq->mutex);
> +       cq->sys_thread_req_count--;
> +       /* If there's someone waiting, let it resume */
> +       optee_cq_complete_one(cq);
> +       mutex_unlock(&cq->mutex);
> +}
> +
>  /* Requires the filpstate mutex to be held */
>  static struct optee_session *find_session(struct optee_context_data *ctxdata,
>                                           u32 session_id)
> @@ -361,6 +451,27 @@ int optee_open_session(struct tee_context *ctx,
>         return rc;
>  }
>
> +int optee_system_session(struct tee_context *ctx, u32 session)
> +{
> +       struct optee *optee = tee_get_drvdata(ctx->teedev);
> +       struct optee_context_data *ctxdata = ctx->data;
> +       struct optee_session *sess;
> +       int rc = -EINVAL;
> +
> +       mutex_lock(&ctxdata->mutex);
> +
> +       sess = find_session(ctxdata, session);
> +       if (sess && (sess->use_sys_thread ||
> +                    optee_cq_incr_sys_thread_count(&optee->call_queue))) {
> +               sess->use_sys_thread = true;
> +               rc = 0;
> +       }
> +
> +       mutex_unlock(&ctxdata->mutex);
> +
> +       return rc;
> +}
> +
>  int optee_close_session_helper(struct tee_context *ctx, u32 session,
>                                bool system_thread)
>  {
> @@ -380,6 +491,9 @@ int optee_close_session_helper(struct tee_context *ctx, u32 session,
>
>         optee_free_msg_arg(ctx, entry, offs);
>
> +       if (system_thread)
> +               optee_cq_decr_sys_thread_count(&optee->call_queue);
> +
>         return 0;
>  }
>
> diff --git a/drivers/tee/optee/ffa_abi.c b/drivers/tee/optee/ffa_abi.c
> index 5fde9d4100e3..0c9055691343 100644
> --- a/drivers/tee/optee/ffa_abi.c
> +++ b/drivers/tee/optee/ffa_abi.c
> @@ -852,8 +852,7 @@ static int optee_ffa_probe(struct ffa_device *ffa_dev)
>         if (rc)
>                 goto err_unreg_supp_teedev;
>         mutex_init(&optee->ffa.mutex);
> -       mutex_init(&optee->call_queue.mutex);
> -       INIT_LIST_HEAD(&optee->call_queue.waiters);
> +       optee_cq_init(&optee->call_queue, 0);
>         optee_supp_init(&optee->supp);
>         optee_shm_arg_cache_init(optee, arg_cache_flags);
>         ffa_dev_set_drvdata(ffa_dev, optee);
> diff --git a/drivers/tee/optee/optee_private.h b/drivers/tee/optee/optee_private.h
> index b68273051454..69f6397c3646 100644
> --- a/drivers/tee/optee/optee_private.h
> +++ b/drivers/tee/optee/optee_private.h
> @@ -40,15 +40,35 @@ typedef void (optee_invoke_fn)(unsigned long, unsigned long, unsigned long,
>                                 unsigned long, unsigned long,
>                                 struct arm_smccc_res *);
>
> +/*
> + * struct optee_call_waiter - TEE entry may need to wait for a free TEE thread
> + * @list_node          Reference in waiters list
> + * @c                  Waiting completion reference
> + * @sys_thread_req     True if waiter belongs to a system thread
> + */
>  struct optee_call_waiter {
>         struct list_head list_node;
>         struct completion c;
> +       bool sys_thread;
>  };
>
> +/*
> + * struct optee_call_queue - OP-TEE call queue management
> + * @mutex                      Serializes access to this struct
> + * @sys_waiters                        List of system threads waiting to enter OP-TEE
> + * @normal_waiters             List of normal threads waiting to enter OP-TEE
> + * @total_thread_count         Overall number of thread context in OP-TEE or 0
> + * @free_thread_count          Number of threads context free in OP-TEE
> + * @sys_thread_req_count       Number of registered system thread sessions
> + */
>  struct optee_call_queue {
>         /* Serializes access to this struct */
>         struct mutex mutex;
> -       struct list_head waiters;
> +       struct list_head sys_waiters;
> +       struct list_head normal_waiters;
> +       int total_thread_count;
> +       int free_thread_count;
> +       int sys_thread_req_count;
>  };
>
>  struct optee_notif {
> @@ -254,6 +274,7 @@ int optee_supp_send(struct tee_context *ctx, u32 ret, u32 num_params,
>  int optee_open_session(struct tee_context *ctx,
>                        struct tee_ioctl_open_session_arg *arg,
>                        struct tee_param *param);
> +int optee_system_session(struct tee_context *ctx, u32 session);
>  int optee_close_session_helper(struct tee_context *ctx, u32 session,
>                                bool system_thread);
>  int optee_close_session(struct tee_context *ctx, u32 session);
> @@ -303,6 +324,7 @@ static inline void optee_to_msg_param_value(struct optee_msg_param *mp,
>         mp->u.value.c = p->u.value.c;
>  }
>
> +void optee_cq_init(struct optee_call_queue *cq, int thread_count);
>  void optee_cq_wait_init(struct optee_call_queue *cq,
>                         struct optee_call_waiter *w, bool sys_thread);
>  void optee_cq_wait_for_completion(struct optee_call_queue *cq,
> diff --git a/drivers/tee/optee/smc_abi.c b/drivers/tee/optee/smc_abi.c
> index 1033d7da03ea..5595028d6dae 100644
> --- a/drivers/tee/optee/smc_abi.c
> +++ b/drivers/tee/optee/smc_abi.c
> @@ -1211,6 +1211,7 @@ static const struct tee_driver_ops optee_clnt_ops = {
>         .release = optee_release,
>         .open_session = optee_open_session,
>         .close_session = optee_close_session,
> +       .system_session = optee_system_session,
>         .invoke_func = optee_invoke_func,
>         .cancel_req = optee_cancel_req,
>         .shm_register = optee_shm_register,
> @@ -1358,6 +1359,16 @@ static bool optee_msg_exchange_capabilities(optee_invoke_fn *invoke_fn,
>         return true;
>  }
>
> +static unsigned int optee_msg_get_thread_count(optee_invoke_fn *invoke_fn)
> +{
> +       struct arm_smccc_res res;
> +
> +       invoke_fn(OPTEE_SMC_GET_THREAD_COUNT, 0, 0, 0, 0, 0, 0, 0, &res);
> +       if (res.a0)
> +               return 0;
> +       return res.a1;
> +}
> +
>  static struct tee_shm_pool *
>  optee_config_shm_memremap(optee_invoke_fn *invoke_fn, void **memremaped_shm)
>  {
> @@ -1610,6 +1621,7 @@ static int optee_probe(struct platform_device *pdev)
>         struct optee *optee = NULL;
>         void *memremaped_shm = NULL;
>         unsigned int rpc_param_count;
> +       unsigned int thread_count;
>         struct tee_device *teedev;
>         struct tee_context *ctx;
>         u32 max_notif_value;
> @@ -1637,6 +1649,7 @@ static int optee_probe(struct platform_device *pdev)
>                 return -EINVAL;
>         }
>
> +       thread_count = optee_msg_get_thread_count(invoke_fn);
>         if (!optee_msg_exchange_capabilities(invoke_fn, &sec_caps,
>                                              &max_notif_value,
>                                              &rpc_param_count)) {
> @@ -1726,8 +1739,7 @@ static int optee_probe(struct platform_device *pdev)
>         if (rc)
>                 goto err_unreg_supp_teedev;
>
> -       mutex_init(&optee->call_queue.mutex);
> -       INIT_LIST_HEAD(&optee->call_queue.waiters);
> +       optee_cq_init(&optee->call_queue, thread_count);
>         optee_supp_init(&optee->supp);
>         optee->smc.memremaped_shm = memremaped_shm;
>         optee->pool = pool;
> --
> 2.25.1
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v10 3/4] tee: optee: support tracking system threads

[Etienne CARRIERE - foss]

> From: Sumit Garg <sumit.garg@linaro.org>
> Sent: Friday, October 6, 2023 11:33 AM
> 
> On Tue, 3 Oct 2023 at 19:36, Etienne Carriere
> <etienne.carriere@foss.st.com> wrote:
> >
> > Adds support in the OP-TEE driver to keep track of reserved system
> > threads. The logic allows one OP-TEE thread to be reserved to TEE system
> > sessions.
> >
> > The optee_cq_*() functions are updated to handle this if enabled,
> > that is when TEE describes how many thread context it supports
> > and when at least 1 session has registered as a system session
> > (using tee_client_system_session()).
> >
> > For sake of simplicity, initialization of call queue management
> > is factorized into new helper function optee_cq_init().
> >
> > The SMC ABI part of the driver enables this tracking, but the
> > FF-A ABI part does not.
> >
> >
> > Co-developed-by: Jens Wiklander <jens.wiklander@linaro.org>
> > Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> > Co-developed-by: Sumit Garg <sumit.garg@linaro.org>
> > Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> > Signed-off-by: Etienne Carriere <etienne.carriere@foss.st.com>
> > ---
> > Changes since v9:
> > - Add a reference counter for TEE system thread provisioning. We reserve
> >   a TEE thread context for system session only when there is at least
> >   1 opened system session.
> > - Use 2 wait queue lists, normal_waiters and sys_waiter, as proposed in
> >   patch v8. Using a single list can prevent a waiting system thread from
> >   being resumed if the executing system thread wakes a normal waiter in
> >   the list.
> 
> How would that be possible? The system thread wakeup
> (free_thread_threshold = 0) is given priority over normal thread
> wakeup (free_thread_threshold = 1). I think a single queue list would
> be sufficient as demonstrated in v9.
> 

Hello Sumit,

I think a system session can be trapped waiting when using a single queue list.
To have a chance to reach the TEE, a waiting thread must wait that a TEE thread comes out of the TEE and calls complete() on the waitqueue to wake next waiter.

To illustrate, consider a 10 TEE threads configuration on TEE side (::total_thread_count=10 at init),
and several TEE clients in Linux OS, including 2 system sessions, from 2 consumer drivers (::sys_thread_req_count=2).

Imagine the 9 normal threads and the 1 system thread are in use. (::free_thread_count=0),
Now comes the other system session: it goes to the waitqueue list.
Now comes a normal session invocation: it goes to the waitqueue list, 1st position.

Now, TEE system thread returns to Linux:
It increments the counter, ::free_thread_count=1, and calls complete() for the waitequeue.
The 1st element in the waitqueue list is the last entered normal session invocation.
However, that waiter won't switch local boolean 'need_wait'  to false because ::free_thread_count=1 and ::sys_thread_req_count!=0.
So no attempt to reach TEE and wake another waiter on return.
At that point there is a system session in the waitqueue list that could enter TEE (::free_thread_count=1) but is waiting someone returns from the TEE.

With 2 lists, we first treat system sessions to overcome that.
Am I missing something?

Best regards,
Etienne

> -Sumit
> 
> > - Updated my e-mail address.
> > - Rephrased a bit the commit message.
> >
> > Changes since patch v8
> > - Patch v9 (reference below) attempted to simplify the implementation
> >   https://lore.kernel.org/lkml/20230517143311.585080-1-sumit.garg@linaro.org/#t
> >
> > Changes since v7:
> > - Changes the logic to reserve at most 1 call entry for system sessions
> >   as per patches v6 and v7 discussion threads (the 2 below bullets)
> >   and updates commit message accordingly.
> > - Field optee_call_queue::res_sys_thread_count is replaced with 2 fields:
> >   sys_thread_req_count and boolean sys_thread_in_use.
> > - Field optee_call_waiter::sys_thread is replaced with 2 fields:
> >   sys_thread_req and sys_thread_used.
> > - Adds inline description comments for struct optee_call_queue and
> >   struct optee_call_waiter.
> >
> > Changes since v6:
> > - Moved out changes related to adding boolean system thread attribute
> >   into optee driver call queue and SMC/FF-A ABIs API functions. These
> >   changes were squashed into patch 1/4 of this patch v7 series.
> > - Comment about adding a specific commit for call queue refactoring
> >   was not addressed such a patch would only introduce function
> >   optee_cq_init()  with very little content in (mutex & list init).
> > - Added Co-developed-by tag for Jens contribution as he's not responsible
> >   for the changes I made in this patch v7.
> >
> > No change since v5
> >
> > Changes since v4:
> > - New change that supersedes implementation proposed in PATCH v4
> >   (tee: system invocation"). Thanks to Jens implementation we don't need
> >   the new OP-TEE services that my previous patch versions introduced to
> >   monitor system threads entry. Now, Linux optee SMC ABI driver gets TEE
> >   provisioned thread contexts count once and monitors thread entries in
> >   OP-TEE on that basis and the system thread capability of the related
> >   tee session. By the way, I dropped the WARN_ONCE() call I suggested
> >   on tee thread exhaustion as it does not provides useful information.
> > ---
> >  drivers/tee/optee/call.c          | 128 ++++++++++++++++++++++++++++--
> >  drivers/tee/optee/ffa_abi.c       |   3 +-
> >  drivers/tee/optee/optee_private.h |  24 +++++-
> >  drivers/tee/optee/smc_abi.c       |  16 +++-
> >  4 files changed, 159 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/tee/optee/call.c b/drivers/tee/optee/call.c
> > index 152ae9bb1785..38543538d77b 100644
> > --- a/drivers/tee/optee/call.c
> > +++ b/drivers/tee/optee/call.c
> > @@ -39,9 +39,31 @@ struct optee_shm_arg_entry {
> >         DECLARE_BITMAP(map, MAX_ARG_COUNT_PER_ENTRY);
> >  };
> >
> > +void optee_cq_init(struct optee_call_queue *cq, int thread_count)
> > +{
> > +       mutex_init(&cq->mutex);
> > +       INIT_LIST_HEAD(&cq->sys_waiters);
> > +       INIT_LIST_HEAD(&cq->normal_waiters);
> > +
> > +       /*
> > +        * If cq->total_thread_count is 0 then we're not trying to keep
> > +        * track of how many free threads we have, instead we're relying on
> > +        * the secure world to tell us when we're out of thread and have to
> > +        * wait for another thread to become available.
> > +        */
> > +       cq->total_thread_count = thread_count;
> > +       cq->free_thread_count = thread_count;
> > +}
> > +
> >  void optee_cq_wait_init(struct optee_call_queue *cq,
> >                         struct optee_call_waiter *w, bool sys_thread)
> >  {
> > +       unsigned int free_thread_threshold;
> > +       bool need_wait = false;
> > +
> > +       memset(w, 0, sizeof(*w));
> > +       w->sys_thread = sys_thread;
> > +
> >         /*
> >          * We're preparing to make a call to secure world. In case we can't
> >          * allocate a thread in secure world we'll end up waiting in
> > @@ -53,15 +75,47 @@ void optee_cq_wait_init(struct optee_call_queue *cq,
> >         mutex_lock(&cq->mutex);
> >
> >         /*
> > -        * We add ourselves to the queue, but we don't wait. This
> > -        * guarantees that we don't lose a completion if secure world
> > -        * returns busy and another thread just exited and try to complete
> > -        * someone.
> > +        * We add ourselves to a queue, but we don't wait. This guarantees
> > +        * that we don't lose a completion if secure world returns busy and
> > +        * another thread just exited and try to complete someone.
> >          */
> >         init_completion(&w->c);
> > -       list_add_tail(&w->list_node, &cq->waiters);
> > +
> > +       if (sys_thread)
> > +               list_add_tail(&w->list_node, &cq->sys_waiters);
> > +       else
> > +               list_add_tail(&w->list_node, &cq->normal_waiters);
> > +
> > +       if (cq->total_thread_count) {
> > +               if (sys_thread || !cq->sys_thread_req_count)
> > +                       free_thread_threshold = 0;
> > +               else
> > +                       free_thread_threshold = 1;
> > +
> > +               if (cq->free_thread_count > free_thread_threshold)
> > +                       cq->free_thread_count--;
> > +               else
> > +                       need_wait = true;
> > +       }
> >
> >         mutex_unlock(&cq->mutex);
> > +
> > +       while (need_wait) {
> > +               optee_cq_wait_for_completion(cq, w);
> > +               mutex_lock(&cq->mutex);
> > +
> > +               if (sys_thread || !cq->sys_thread_req_count)
> > +                       free_thread_threshold = 0;
> > +               else
> > +                       free_thread_threshold = 1;
> > +
> > +               if (cq->free_thread_count > free_thread_threshold) {
> > +                       cq->free_thread_count--;
> > +                       need_wait = false;
> > +               }
> > +
> > +               mutex_unlock(&cq->mutex);
> > +       }
> >  }
> >
> >  void optee_cq_wait_for_completion(struct optee_call_queue *cq,
> > @@ -74,7 +128,11 @@ void optee_cq_wait_for_completion(struct optee_call_queue *cq,
> >         /* Move to end of list to get out of the way for other waiters */
> >         list_del(&w->list_node);
> >         reinit_completion(&w->c);
> > -       list_add_tail(&w->list_node, &cq->waiters);
> > +
> > +       if (w->sys_thread)
> > +               list_add_tail(&w->list_node, &cq->sys_waiters);
> > +       else
> > +               list_add_tail(&w->list_node, &cq->normal_waiters);
> >
> >         mutex_unlock(&cq->mutex);
> >  }
> > @@ -83,7 +141,15 @@ static void optee_cq_complete_one(struct optee_call_queue *cq)
> >  {
> >         struct optee_call_waiter *w;
> >
> > -       list_for_each_entry(w, &cq->waiters, list_node) {
> > +       /* Wake waiting system session first */
> > +       list_for_each_entry(w, &cq->sys_waiters, list_node) {
> > +               if (!completion_done(&w->c)) {
> > +                       complete(&w->c);
> > +                       break;
> > +               }
> > +       }
> > +
> > +       list_for_each_entry(w, &cq->normal_waiters, list_node) {
> >                 if (!completion_done(&w->c)) {
> >                         complete(&w->c);
> >                         break;
> > @@ -104,6 +170,8 @@ void optee_cq_wait_final(struct optee_call_queue *cq,
> >         /* Get out of the list */
> >         list_del(&w->list_node);
> >
> > +       cq->free_thread_count++;
> > +
> >         /* Wake up one eventual waiting task */
> >         optee_cq_complete_one(cq);
> >
> > @@ -119,6 +187,28 @@ void optee_cq_wait_final(struct optee_call_queue *cq,
> >         mutex_unlock(&cq->mutex);
> >  }
> >
> > +/* Count registered system sessions to reserved a system thread or not */
> > +static bool optee_cq_incr_sys_thread_count(struct optee_call_queue *cq)
> > +{
> > +       if (cq->total_thread_count <= 1)
> > +               return false;
> > +
> > +       mutex_lock(&cq->mutex);
> > +       cq->sys_thread_req_count++;
> > +       mutex_unlock(&cq->mutex);
> > +
> > +       return true;
> > +}
> > +
> > +static void optee_cq_decr_sys_thread_count(struct optee_call_queue *cq)
> > +{
> > +       mutex_lock(&cq->mutex);
> > +       cq->sys_thread_req_count--;
> > +       /* If there's someone waiting, let it resume */
> > +       optee_cq_complete_one(cq);
> > +       mutex_unlock(&cq->mutex);
> > +}
> > +
> >  /* Requires the filpstate mutex to be held */
> >  static struct optee_session *find_session(struct optee_context_data *ctxdata,
> >                                           u32 session_id)
> > @@ -361,6 +451,27 @@ int optee_open_session(struct tee_context *ctx,
> >         return rc;
> >  }
> >
> > +int optee_system_session(struct tee_context *ctx, u32 session)
> > +{
> > +       struct optee *optee = tee_get_drvdata(ctx->teedev);
> > +       struct optee_context_data *ctxdata = ctx->data;
> > +       struct optee_session *sess;
> > +       int rc = -EINVAL;
> > +
> > +       mutex_lock(&ctxdata->mutex);
> > +
> > +       sess = find_session(ctxdata, session);
> > +       if (sess && (sess->use_sys_thread ||
> > +                    optee_cq_incr_sys_thread_count(&optee->call_queue))) {
> > +               sess->use_sys_thread = true;
> > +               rc = 0;
> > +       }
> > +
> > +       mutex_unlock(&ctxdata->mutex);
> > +
> > +       return rc;
> > +}
> > +
> >  int optee_close_session_helper(struct tee_context *ctx, u32 session,
> >                                bool system_thread)
> >  {
> > @@ -380,6 +491,9 @@ int optee_close_session_helper(struct tee_context *ctx, u32 session,
> >
> >         optee_free_msg_arg(ctx, entry, offs);
> >
> > +       if (system_thread)
> > +               optee_cq_decr_sys_thread_count(&optee->call_queue);
> > +
> >         return 0;
> >  }
> >
> > diff --git a/drivers/tee/optee/ffa_abi.c b/drivers/tee/optee/ffa_abi.c
> > index 5fde9d4100e3..0c9055691343 100644
> > --- a/drivers/tee/optee/ffa_abi.c
> > +++ b/drivers/tee/optee/ffa_abi.c
> > @@ -852,8 +852,7 @@ static int optee_ffa_probe(struct ffa_device *ffa_dev)
> >         if (rc)
> >                 goto err_unreg_supp_teedev;
> >         mutex_init(&optee->ffa.mutex);
> > -       mutex_init(&optee->call_queue.mutex);
> > -       INIT_LIST_HEAD(&optee->call_queue.waiters);
> > +       optee_cq_init(&optee->call_queue, 0);
> >         optee_supp_init(&optee->supp);
> >         optee_shm_arg_cache_init(optee, arg_cache_flags);
> >         ffa_dev_set_drvdata(ffa_dev, optee);
> > diff --git a/drivers/tee/optee/optee_private.h b/drivers/tee/optee/optee_private.h
> > index b68273051454..69f6397c3646 100644
> > --- a/drivers/tee/optee/optee_private.h
> > +++ b/drivers/tee/optee/optee_private.h
> > @@ -40,15 +40,35 @@ typedef void (optee_invoke_fn)(unsigned long, unsigned long, unsigned long,
> >                                 unsigned long, unsigned long,
> >                                 struct arm_smccc_res *);
> >
> > +/*
> > + * struct optee_call_waiter - TEE entry may need to wait for a free TEE thread
> > + * @list_node          Reference in waiters list
> > + * @c                  Waiting completion reference
> > + * @sys_thread_req     True if waiter belongs to a system thread
> > + */
> >  struct optee_call_waiter {
> >         struct list_head list_node;
> >         struct completion c;
> > +       bool sys_thread;
> >  };
> >
> > +/*
> > + * struct optee_call_queue - OP-TEE call queue management
> > + * @mutex                      Serializes access to this struct
> > + * @sys_waiters                        List of system threads waiting to enter OP-TEE
> > + * @normal_waiters             List of normal threads waiting to enter OP-TEE
> > + * @total_thread_count         Overall number of thread context in OP-TEE or 0
> > + * @free_thread_count          Number of threads context free in OP-TEE
> > + * @sys_thread_req_count       Number of registered system thread sessions
> > + */
> >  struct optee_call_queue {
> >         /* Serializes access to this struct */
> >         struct mutex mutex;
> > -       struct list_head waiters;
> > +       struct list_head sys_waiters;
> > +       struct list_head normal_waiters;
> > +       int total_thread_count;
> > +       int free_thread_count;
> > +       int sys_thread_req_count;
> >  };
> >
> >  struct optee_notif {
> > @@ -254,6 +274,7 @@ int optee_supp_send(struct tee_context *ctx, u32 ret, u32 num_params,
> >  int optee_open_session(struct tee_context *ctx,
> >                        struct tee_ioctl_open_session_arg *arg,
> >                        struct tee_param *param);
> > +int optee_system_session(struct tee_context *ctx, u32 session);
> >  int optee_close_session_helper(struct tee_context *ctx, u32 session,
> >                                bool system_thread);
> >  int optee_close_session(struct tee_context *ctx, u32 session);
> > @@ -303,6 +324,7 @@ static inline void optee_to_msg_param_value(struct optee_msg_param *mp,
> >         mp->u.value.c = p->u.value.c;
> >  }
> >
> > +void optee_cq_init(struct optee_call_queue *cq, int thread_count);
> >  void optee_cq_wait_init(struct optee_call_queue *cq,
> >                         struct optee_call_waiter *w, bool sys_thread);
> >  void optee_cq_wait_for_completion(struct optee_call_queue *cq,
> > diff --git a/drivers/tee/optee/smc_abi.c b/drivers/tee/optee/smc_abi.c
> > index 1033d7da03ea..5595028d6dae 100644
> > --- a/drivers/tee/optee/smc_abi.c
> > +++ b/drivers/tee/optee/smc_abi.c
> > @@ -1211,6 +1211,7 @@ static const struct tee_driver_ops optee_clnt_ops = {
> >         .release = optee_release,
> >         .open_session = optee_open_session,
> >         .close_session = optee_close_session,
> > +       .system_session = optee_system_session,
> >         .invoke_func = optee_invoke_func,
> >         .cancel_req = optee_cancel_req,
> >         .shm_register = optee_shm_register,
> > @@ -1358,6 +1359,16 @@ static bool optee_msg_exchange_capabilities(optee_invoke_fn *invoke_fn,
> >         return true;
> >  }
> >
> > +static unsigned int optee_msg_get_thread_count(optee_invoke_fn *invoke_fn)
> > +{
> > +       struct arm_smccc_res res;
> > +
> > +       invoke_fn(OPTEE_SMC_GET_THREAD_COUNT, 0, 0, 0, 0, 0, 0, 0, &res);
> > +       if (res.a0)
> > +               return 0;
> > +       return res.a1;
> > +}
> > +
> >  static struct tee_shm_pool *
> >  optee_config_shm_memremap(optee_invoke_fn *invoke_fn, void **memremaped_shm)
> >  {
> > @@ -1610,6 +1621,7 @@ static int optee_probe(struct platform_device *pdev)
> >         struct optee *optee = NULL;
> >         void *memremaped_shm = NULL;
> >         unsigned int rpc_param_count;
> > +       unsigned int thread_count;
> >         struct tee_device *teedev;
> >         struct tee_context *ctx;
> >         u32 max_notif_value;
> > @@ -1637,6 +1649,7 @@ static int optee_probe(struct platform_device *pdev)
> >                 return -EINVAL;
> >         }
> >
> > +       thread_count = optee_msg_get_thread_count(invoke_fn);
> >         if (!optee_msg_exchange_capabilities(invoke_fn, &sec_caps,
> >                                              &max_notif_value,
> >                                              &rpc_param_count)) {
> > @@ -1726,8 +1739,7 @@ static int optee_probe(struct platform_device *pdev)
> >         if (rc)
> >                 goto err_unreg_supp_teedev;
> >
> > -       mutex_init(&optee->call_queue.mutex);
> > -       INIT_LIST_HEAD(&optee->call_queue.waiters);
> > +       optee_cq_init(&optee->call_queue, thread_count);
> >         optee_supp_init(&optee->supp);
> >         optee->smc.memremaped_shm = memremaped_shm;
> >         optee->pool = pool;
> > --
> > 2.25.1
> >
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v10 3/4] tee: optee: support tracking system threads

[Sumit Garg]

On Wed, 11 Oct 2023 at 12:41, Etienne CARRIERE - foss
<etienne.carriere@foss.st.com> wrote:
>
> > From: Sumit Garg <sumit.garg@linaro.org>
> > Sent: Friday, October 6, 2023 11:33 AM
> >
> > On Tue, 3 Oct 2023 at 19:36, Etienne Carriere
> > <etienne.carriere@foss.st.com> wrote:
> > >
> > > Adds support in the OP-TEE driver to keep track of reserved system
> > > threads. The logic allows one OP-TEE thread to be reserved to TEE system
> > > sessions.
> > >
> > > The optee_cq_*() functions are updated to handle this if enabled,
> > > that is when TEE describes how many thread context it supports
> > > and when at least 1 session has registered as a system session
> > > (using tee_client_system_session()).
> > >
> > > For sake of simplicity, initialization of call queue management
> > > is factorized into new helper function optee_cq_init().
> > >
> > > The SMC ABI part of the driver enables this tracking, but the
> > > FF-A ABI part does not.
> > >
> > >
> > > Co-developed-by: Jens Wiklander <jens.wiklander@linaro.org>
> > > Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> > > Co-developed-by: Sumit Garg <sumit.garg@linaro.org>
> > > Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> > > Signed-off-by: Etienne Carriere <etienne.carriere@foss.st.com>
> > > ---
> > > Changes since v9:
> > > - Add a reference counter for TEE system thread provisioning. We reserve
> > >   a TEE thread context for system session only when there is at least
> > >   1 opened system session.
> > > - Use 2 wait queue lists, normal_waiters and sys_waiter, as proposed in
> > >   patch v8. Using a single list can prevent a waiting system thread from
> > >   being resumed if the executing system thread wakes a normal waiter in
> > >   the list.
> >
> > How would that be possible? The system thread wakeup
> > (free_thread_threshold = 0) is given priority over normal thread
> > wakeup (free_thread_threshold = 1). I think a single queue list would
> > be sufficient as demonstrated in v9.
> >
>
> Hello Sumit,
>
> I think a system session can be trapped waiting when using a single queue list.
> To have a chance to reach the TEE, a waiting thread must wait that a TEE thread comes out of the TEE and calls complete() on the waitqueue to wake next waiter.
>
> To illustrate, consider a 10 TEE threads configuration on TEE side (::total_thread_count=10 at init),
> and several TEE clients in Linux OS, including 2 system sessions, from 2 consumer drivers (::sys_thread_req_count=2).
>
> Imagine the 9 normal threads and the 1 system thread are in use. (::free_thread_count=0),
> Now comes the other system session: it goes to the waitqueue list.
> Now comes a normal session invocation: it goes to the waitqueue list, 1st position.
>
> Now, TEE system thread returns to Linux:
> It increments the counter, ::free_thread_count=1, and calls complete() for the waitequeue.
> The 1st element in the waitqueue list is the last entered normal session invocation.
> However, that waiter won't switch local boolean 'need_wait'  to false because ::free_thread_count=1 and ::sys_thread_req_count!=0.
> So no attempt to reach TEE and wake another waiter on return.
> At that point there is a system session in the waitqueue list that could enter TEE (::free_thread_count=1) but is waiting someone returns from the TEE.

I suppose the following loop tries to wake-up every waiter to give
them a chance to enter OP-TEE. So with that system session would
always be prefered over normal session, right?

static void optee_cq_complete_one(struct optee_call_queue *cq)
{
        struct optee_call_waiter *w;

        list_for_each_entry(w, &cq->waiters, list_node) {
                if (!completion_done(&w->c)) {
                        complete(&w->c);
                        break;
                }
        }
}

-Sumit

>
> With 2 lists, we first treat system sessions to overcome that.
> Am I missing something?
>
> Best regards,
> Etienne
>
> > -Sumit
> >
> > > - Updated my e-mail address.
> > > - Rephrased a bit the commit message.
> > >
> > > Changes since patch v8
> > > - Patch v9 (reference below) attempted to simplify the implementation
> > >   https://lore.kernel.org/lkml/20230517143311.585080-1-sumit.garg@linaro.org/#t
> > >
> > > Changes since v7:
> > > - Changes the logic to reserve at most 1 call entry for system sessions
> > >   as per patches v6 and v7 discussion threads (the 2 below bullets)
> > >   and updates commit message accordingly.
> > > - Field optee_call_queue::res_sys_thread_count is replaced with 2 fields:
> > >   sys_thread_req_count and boolean sys_thread_in_use.
> > > - Field optee_call_waiter::sys_thread is replaced with 2 fields:
> > >   sys_thread_req and sys_thread_used.
> > > - Adds inline description comments for struct optee_call_queue and
> > >   struct optee_call_waiter.
> > >
> > > Changes since v6:
> > > - Moved out changes related to adding boolean system thread attribute
> > >   into optee driver call queue and SMC/FF-A ABIs API functions. These
> > >   changes were squashed into patch 1/4 of this patch v7 series.
> > > - Comment about adding a specific commit for call queue refactoring
> > >   was not addressed such a patch would only introduce function
> > >   optee_cq_init()  with very little content in (mutex & list init).
> > > - Added Co-developed-by tag for Jens contribution as he's not responsible
> > >   for the changes I made in this patch v7.
> > >
> > > No change since v5
> > >
> > > Changes since v4:
> > > - New change that supersedes implementation proposed in PATCH v4
> > >   (tee: system invocation"). Thanks to Jens implementation we don't need
> > >   the new OP-TEE services that my previous patch versions introduced to
> > >   monitor system threads entry. Now, Linux optee SMC ABI driver gets TEE
> > >   provisioned thread contexts count once and monitors thread entries in
> > >   OP-TEE on that basis and the system thread capability of the related
> > >   tee session. By the way, I dropped the WARN_ONCE() call I suggested
> > >   on tee thread exhaustion as it does not provides useful information.
> > > ---
> > >  drivers/tee/optee/call.c          | 128 ++++++++++++++++++++++++++++--
> > >  drivers/tee/optee/ffa_abi.c       |   3 +-
> > >  drivers/tee/optee/optee_private.h |  24 +++++-
> > >  drivers/tee/optee/smc_abi.c       |  16 +++-
> > >  4 files changed, 159 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/drivers/tee/optee/call.c b/drivers/tee/optee/call.c
> > > index 152ae9bb1785..38543538d77b 100644
> > > --- a/drivers/tee/optee/call.c
> > > +++ b/drivers/tee/optee/call.c
> > > @@ -39,9 +39,31 @@ struct optee_shm_arg_entry {
> > >         DECLARE_BITMAP(map, MAX_ARG_COUNT_PER_ENTRY);
> > >  };
> > >
> > > +void optee_cq_init(struct optee_call_queue *cq, int thread_count)
> > > +{
> > > +       mutex_init(&cq->mutex);
> > > +       INIT_LIST_HEAD(&cq->sys_waiters);
> > > +       INIT_LIST_HEAD(&cq->normal_waiters);
> > > +
> > > +       /*
> > > +        * If cq->total_thread_count is 0 then we're not trying to keep
> > > +        * track of how many free threads we have, instead we're relying on
> > > +        * the secure world to tell us when we're out of thread and have to
> > > +        * wait for another thread to become available.
> > > +        */
> > > +       cq->total_thread_count = thread_count;
> > > +       cq->free_thread_count = thread_count;
> > > +}
> > > +
> > >  void optee_cq_wait_init(struct optee_call_queue *cq,
> > >                         struct optee_call_waiter *w, bool sys_thread)
> > >  {
> > > +       unsigned int free_thread_threshold;
> > > +       bool need_wait = false;
> > > +
> > > +       memset(w, 0, sizeof(*w));
> > > +       w->sys_thread = sys_thread;
> > > +
> > >         /*
> > >          * We're preparing to make a call to secure world. In case we can't
> > >          * allocate a thread in secure world we'll end up waiting in
> > > @@ -53,15 +75,47 @@ void optee_cq_wait_init(struct optee_call_queue *cq,
> > >         mutex_lock(&cq->mutex);
> > >
> > >         /*
> > > -        * We add ourselves to the queue, but we don't wait. This
> > > -        * guarantees that we don't lose a completion if secure world
> > > -        * returns busy and another thread just exited and try to complete
> > > -        * someone.
> > > +        * We add ourselves to a queue, but we don't wait. This guarantees
> > > +        * that we don't lose a completion if secure world returns busy and
> > > +        * another thread just exited and try to complete someone.
> > >          */
> > >         init_completion(&w->c);
> > > -       list_add_tail(&w->list_node, &cq->waiters);
> > > +
> > > +       if (sys_thread)
> > > +               list_add_tail(&w->list_node, &cq->sys_waiters);
> > > +       else
> > > +               list_add_tail(&w->list_node, &cq->normal_waiters);
> > > +
> > > +       if (cq->total_thread_count) {
> > > +               if (sys_thread || !cq->sys_thread_req_count)
> > > +                       free_thread_threshold = 0;
> > > +               else
> > > +                       free_thread_threshold = 1;
> > > +
> > > +               if (cq->free_thread_count > free_thread_threshold)
> > > +                       cq->free_thread_count--;
> > > +               else
> > > +                       need_wait = true;
> > > +       }
> > >
> > >         mutex_unlock(&cq->mutex);
> > > +
> > > +       while (need_wait) {
> > > +               optee_cq_wait_for_completion(cq, w);
> > > +               mutex_lock(&cq->mutex);
> > > +
> > > +               if (sys_thread || !cq->sys_thread_req_count)
> > > +                       free_thread_threshold = 0;
> > > +               else
> > > +                       free_thread_threshold = 1;
> > > +
> > > +               if (cq->free_thread_count > free_thread_threshold) {
> > > +                       cq->free_thread_count--;
> > > +                       need_wait = false;
> > > +               }
> > > +
> > > +               mutex_unlock(&cq->mutex);
> > > +       }
> > >  }
> > >
> > >  void optee_cq_wait_for_completion(struct optee_call_queue *cq,
> > > @@ -74,7 +128,11 @@ void optee_cq_wait_for_completion(struct optee_call_queue *cq,
> > >         /* Move to end of list to get out of the way for other waiters */
> > >         list_del(&w->list_node);
> > >         reinit_completion(&w->c);
> > > -       list_add_tail(&w->list_node, &cq->waiters);
> > > +
> > > +       if (w->sys_thread)
> > > +               list_add_tail(&w->list_node, &cq->sys_waiters);
> > > +       else
> > > +               list_add_tail(&w->list_node, &cq->normal_waiters);
> > >
> > >         mutex_unlock(&cq->mutex);
> > >  }
> > > @@ -83,7 +141,15 @@ static void optee_cq_complete_one(struct optee_call_queue *cq)
> > >  {
> > >         struct optee_call_waiter *w;
> > >
> > > -       list_for_each_entry(w, &cq->waiters, list_node) {
> > > +       /* Wake waiting system session first */
> > > +       list_for_each_entry(w, &cq->sys_waiters, list_node) {
> > > +               if (!completion_done(&w->c)) {
> > > +                       complete(&w->c);
> > > +                       break;
> > > +               }
> > > +       }
> > > +
> > > +       list_for_each_entry(w, &cq->normal_waiters, list_node) {
> > >                 if (!completion_done(&w->c)) {
> > >                         complete(&w->c);
> > >                         break;
> > > @@ -104,6 +170,8 @@ void optee_cq_wait_final(struct optee_call_queue *cq,
> > >         /* Get out of the list */
> > >         list_del(&w->list_node);
> > >
> > > +       cq->free_thread_count++;
> > > +
> > >         /* Wake up one eventual waiting task */
> > >         optee_cq_complete_one(cq);
> > >
> > > @@ -119,6 +187,28 @@ void optee_cq_wait_final(struct optee_call_queue *cq,
> > >         mutex_unlock(&cq->mutex);
> > >  }
> > >
> > > +/* Count registered system sessions to reserved a system thread or not */
> > > +static bool optee_cq_incr_sys_thread_count(struct optee_call_queue *cq)
> > > +{
> > > +       if (cq->total_thread_count <= 1)
> > > +               return false;
> > > +
> > > +       mutex_lock(&cq->mutex);
> > > +       cq->sys_thread_req_count++;
> > > +       mutex_unlock(&cq->mutex);
> > > +
> > > +       return true;
> > > +}
> > > +
> > > +static void optee_cq_decr_sys_thread_count(struct optee_call_queue *cq)
> > > +{
> > > +       mutex_lock(&cq->mutex);
> > > +       cq->sys_thread_req_count--;
> > > +       /* If there's someone waiting, let it resume */
> > > +       optee_cq_complete_one(cq);
> > > +       mutex_unlock(&cq->mutex);
> > > +}
> > > +
> > >  /* Requires the filpstate mutex to be held */
> > >  static struct optee_session *find_session(struct optee_context_data *ctxdata,
> > >                                           u32 session_id)
> > > @@ -361,6 +451,27 @@ int optee_open_session(struct tee_context *ctx,
> > >         return rc;
> > >  }
> > >
> > > +int optee_system_session(struct tee_context *ctx, u32 session)
> > > +{
> > > +       struct optee *optee = tee_get_drvdata(ctx->teedev);
> > > +       struct optee_context_data *ctxdata = ctx->data;
> > > +       struct optee_session *sess;
> > > +       int rc = -EINVAL;
> > > +
> > > +       mutex_lock(&ctxdata->mutex);
> > > +
> > > +       sess = find_session(ctxdata, session);
> > > +       if (sess && (sess->use_sys_thread ||
> > > +                    optee_cq_incr_sys_thread_count(&optee->call_queue))) {
> > > +               sess->use_sys_thread = true;
> > > +               rc = 0;
> > > +       }
> > > +
> > > +       mutex_unlock(&ctxdata->mutex);
> > > +
> > > +       return rc;
> > > +}
> > > +
> > >  int optee_close_session_helper(struct tee_context *ctx, u32 session,
> > >                                bool system_thread)
> > >  {
> > > @@ -380,6 +491,9 @@ int optee_close_session_helper(struct tee_context *ctx, u32 session,
> > >
> > >         optee_free_msg_arg(ctx, entry, offs);
> > >
> > > +       if (system_thread)
> > > +               optee_cq_decr_sys_thread_count(&optee->call_queue);
> > > +
> > >         return 0;
> > >  }
> > >
> > > diff --git a/drivers/tee/optee/ffa_abi.c b/drivers/tee/optee/ffa_abi.c
> > > index 5fde9d4100e3..0c9055691343 100644
> > > --- a/drivers/tee/optee/ffa_abi.c
> > > +++ b/drivers/tee/optee/ffa_abi.c
> > > @@ -852,8 +852,7 @@ static int optee_ffa_probe(struct ffa_device *ffa_dev)
> > >         if (rc)
> > >                 goto err_unreg_supp_teedev;
> > >         mutex_init(&optee->ffa.mutex);
> > > -       mutex_init(&optee->call_queue.mutex);
> > > -       INIT_LIST_HEAD(&optee->call_queue.waiters);
> > > +       optee_cq_init(&optee->call_queue, 0);
> > >         optee_supp_init(&optee->supp);
> > >         optee_shm_arg_cache_init(optee, arg_cache_flags);
> > >         ffa_dev_set_drvdata(ffa_dev, optee);
> > > diff --git a/drivers/tee/optee/optee_private.h b/drivers/tee/optee/optee_private.h
> > > index b68273051454..69f6397c3646 100644
> > > --- a/drivers/tee/optee/optee_private.h
> > > +++ b/drivers/tee/optee/optee_private.h
> > > @@ -40,15 +40,35 @@ typedef void (optee_invoke_fn)(unsigned long, unsigned long, unsigned long,
> > >                                 unsigned long, unsigned long,
> > >                                 struct arm_smccc_res *);
> > >
> > > +/*
> > > + * struct optee_call_waiter - TEE entry may need to wait for a free TEE thread
> > > + * @list_node          Reference in waiters list
> > > + * @c                  Waiting completion reference
> > > + * @sys_thread_req     True if waiter belongs to a system thread
> > > + */
> > >  struct optee_call_waiter {
> > >         struct list_head list_node;
> > >         struct completion c;
> > > +       bool sys_thread;
> > >  };
> > >
> > > +/*
> > > + * struct optee_call_queue - OP-TEE call queue management
> > > + * @mutex                      Serializes access to this struct
> > > + * @sys_waiters                        List of system threads waiting to enter OP-TEE
> > > + * @normal_waiters             List of normal threads waiting to enter OP-TEE
> > > + * @total_thread_count         Overall number of thread context in OP-TEE or 0
> > > + * @free_thread_count          Number of threads context free in OP-TEE
> > > + * @sys_thread_req_count       Number of registered system thread sessions
> > > + */
> > >  struct optee_call_queue {
> > >         /* Serializes access to this struct */
> > >         struct mutex mutex;
> > > -       struct list_head waiters;
> > > +       struct list_head sys_waiters;
> > > +       struct list_head normal_waiters;
> > > +       int total_thread_count;
> > > +       int free_thread_count;
> > > +       int sys_thread_req_count;
> > >  };
> > >
> > >  struct optee_notif {
> > > @@ -254,6 +274,7 @@ int optee_supp_send(struct tee_context *ctx, u32 ret, u32 num_params,
> > >  int optee_open_session(struct tee_context *ctx,
> > >                        struct tee_ioctl_open_session_arg *arg,
> > >                        struct tee_param *param);
> > > +int optee_system_session(struct tee_context *ctx, u32 session);
> > >  int optee_close_session_helper(struct tee_context *ctx, u32 session,
> > >                                bool system_thread);
> > >  int optee_close_session(struct tee_context *ctx, u32 session);
> > > @@ -303,6 +324,7 @@ static inline void optee_to_msg_param_value(struct optee_msg_param *mp,
> > >         mp->u.value.c = p->u.value.c;
> > >  }
> > >
> > > +void optee_cq_init(struct optee_call_queue *cq, int thread_count);
> > >  void optee_cq_wait_init(struct optee_call_queue *cq,
> > >                         struct optee_call_waiter *w, bool sys_thread);
> > >  void optee_cq_wait_for_completion(struct optee_call_queue *cq,
> > > diff --git a/drivers/tee/optee/smc_abi.c b/drivers/tee/optee/smc_abi.c
> > > index 1033d7da03ea..5595028d6dae 100644
> > > --- a/drivers/tee/optee/smc_abi.c
> > > +++ b/drivers/tee/optee/smc_abi.c
> > > @@ -1211,6 +1211,7 @@ static const struct tee_driver_ops optee_clnt_ops = {
> > >         .release = optee_release,
> > >         .open_session = optee_open_session,
> > >         .close_session = optee_close_session,
> > > +       .system_session = optee_system_session,
> > >         .invoke_func = optee_invoke_func,
> > >         .cancel_req = optee_cancel_req,
> > >         .shm_register = optee_shm_register,
> > > @@ -1358,6 +1359,16 @@ static bool optee_msg_exchange_capabilities(optee_invoke_fn *invoke_fn,
> > >         return true;
> > >  }
> > >
> > > +static unsigned int optee_msg_get_thread_count(optee_invoke_fn *invoke_fn)
> > > +{
> > > +       struct arm_smccc_res res;
> > > +
> > > +       invoke_fn(OPTEE_SMC_GET_THREAD_COUNT, 0, 0, 0, 0, 0, 0, 0, &res);
> > > +       if (res.a0)
> > > +               return 0;
> > > +       return res.a1;
> > > +}
> > > +
> > >  static struct tee_shm_pool *
> > >  optee_config_shm_memremap(optee_invoke_fn *invoke_fn, void **memremaped_shm)
> > >  {
> > > @@ -1610,6 +1621,7 @@ static int optee_probe(struct platform_device *pdev)
> > >         struct optee *optee = NULL;
> > >         void *memremaped_shm = NULL;
> > >         unsigned int rpc_param_count;
> > > +       unsigned int thread_count;
> > >         struct tee_device *teedev;
> > >         struct tee_context *ctx;
> > >         u32 max_notif_value;
> > > @@ -1637,6 +1649,7 @@ static int optee_probe(struct platform_device *pdev)
> > >                 return -EINVAL;
> > >         }
> > >
> > > +       thread_count = optee_msg_get_thread_count(invoke_fn);
> > >         if (!optee_msg_exchange_capabilities(invoke_fn, &sec_caps,
> > >                                              &max_notif_value,
> > >                                              &rpc_param_count)) {
> > > @@ -1726,8 +1739,7 @@ static int optee_probe(struct platform_device *pdev)
> > >         if (rc)
> > >                 goto err_unreg_supp_teedev;
> > >
> > > -       mutex_init(&optee->call_queue.mutex);
> > > -       INIT_LIST_HEAD(&optee->call_queue.waiters);
> > > +       optee_cq_init(&optee->call_queue, thread_count);
> > >         optee_supp_init(&optee->supp);
> > >         optee->smc.memremaped_shm = memremaped_shm;
> > >         optee->pool = pool;
> > > --
> > > 2.25.1
> > >
> >

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v10 3/4] tee: optee: support tracking system threads

[Etienne CARRIERE - foss]

> From: Sumit Garg <sumit.garg@linaro.org>
> Sent: Friday, October 13, 2023 9:21 AM
> 
> On Wed, 11 Oct 2023 at 12:41, Etienne CARRIERE - foss
> <etienne.carriere@foss.st.com> wrote:
> >
> > > From: Sumit Garg <sumit.garg@linaro.org>
> > > Sent: Friday, October 6, 2023 11:33 AM
> > >
> > > On Tue, 3 Oct 2023 at 19:36, Etienne Carriere
> > > <etienne.carriere@foss.st.com> wrote:
> > > >
> > > > Adds support in the OP-TEE driver to keep track of reserved system
> > > > threads. The logic allows one OP-TEE thread to be reserved to TEE system
> > > > sessions.
> > > >
> > > > The optee_cq_*() functions are updated to handle this if enabled,
> > > > that is when TEE describes how many thread context it supports
> > > > and when at least 1 session has registered as a system session
> > > > (using tee_client_system_session()).
> > > >
> > > > For sake of simplicity, initialization of call queue management
> > > > is factorized into new helper function optee_cq_init().
> > > >
> > > > The SMC ABI part of the driver enables this tracking, but the
> > > > FF-A ABI part does not.
> > > >
> > > >
> > > > Co-developed-by: Jens Wiklander <jens.wiklander@linaro.org>
> > > > Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> > > > Co-developed-by: Sumit Garg <sumit.garg@linaro.org>
> > > > Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> > > > Signed-off-by: Etienne Carriere <etienne.carriere@foss.st.com>
> > > > ---
> > > > Changes since v9:
> > > > - Add a reference counter for TEE system thread provisioning. We reserve
> > > >   a TEE thread context for system session only when there is at least
> > > >   1 opened system session.
> > > > - Use 2 wait queue lists, normal_waiters and sys_waiter, as proposed in
> > > >   patch v8. Using a single list can prevent a waiting system thread from
> > > >   being resumed if the executing system thread wakes a normal waiter in
> > > >   the list.
> > >
> > > How would that be possible? The system thread wakeup
> > > (free_thread_threshold = 0) is given priority over normal thread
> > > wakeup (free_thread_threshold = 1). I think a single queue list would
> > > be sufficient as demonstrated in v9.
> > >
> >
> > Hello Sumit,
> >
> > I think a system session can be trapped waiting when using a single queue list.
> > To have a chance to reach the TEE, a waiting thread must wait that a TEE thread comes out of the TEE and calls complete() on the waitqueue to wake next waiter.
> >
> > To illustrate, consider a 10 TEE threads configuration on TEE side (::total_thread_count=10 at init),
> > and several TEE clients in Linux OS, including 2 system sessions, from 2 consumer drivers (::sys_thread_req_count=2).
> >
> > Imagine the 9 normal threads and the 1 system thread are in use. (::free_thread_count=0),
> > Now comes the other system session: it goes to the waitqueue list.
> > Now comes a normal session invocation: it goes to the waitqueue list, 1st position.
> >
> > Now, TEE system thread returns to Linux:
> > It increments the counter, ::free_thread_count=1, and calls complete() for the waitequeue.
> > The 1st element in the waitqueue list is the last entered normal session invocation.
> > However, that waiter won't switch local boolean 'need_wait'  to false because ::free_thread_count=1 and ::sys_thread_req_count!=0.
> > So no attempt to reach TEE and wake another waiter on return.
> > At that point there is a system session in the waitqueue list that could enter TEE (::free_thread_count=1) but is waiting someone returns from the TEE.
> 
> I suppose the following loop tries to wake-up every waiter to give
> them a chance to enter OP-TEE. So with that system session would
> always be prefered over normal session, right?

No, the below loop will wake only the 1st waiter it finds in the list that is
current waiting (completion_done() returns false). So if it finds a normal
session, it will only wake this one which, in turn, will not try to reach the
TEE from the while(need_wiat) loop in optee_cq_wait_init(), because there is
not enough free threads. Because it does not reach the TEE, it will not
it wake another waiter.

> 
> static void optee_cq_complete_one(struct optee_call_queue *cq)
> {
>         struct optee_call_waiter *w;
> 
>         list_for_each_entry(w, &cq->waiters, list_node) {
>                 if (!completion_done(&w->c)) {
>                         complete(&w->c);
>                         break;
>                 }
>         }
> }
> 
> -Sumit
> 

Note I've found a error in this patch v10, see below.

BR,
Etienne


> >
> > With 2 lists, we first treat system sessions to overcome that.
> > Am I missing something?
> >
> > Best regards,
> > Etienne
> >
> > > -Sumit
> > >
> > > > - Updated my e-mail address.
> > > > - Rephrased a bit the commit message.
> > > >
> > > > Changes since patch v8
> > > > - Patch v9 (reference below) attempted to simplify the implementation
> > > >   https://lore.kernel.org/lkml/20230517143311.585080-1-sumit.garg@linaro.org/#t
> > > >
> > > > Changes since v7:
> > > > - Changes the logic to reserve at most 1 call entry for system sessions
> > > >   as per patches v6 and v7 discussion threads (the 2 below bullets)
> > > >   and updates commit message accordingly.
> > > > - Field optee_call_queue::res_sys_thread_count is replaced with 2 fields:
> > > >   sys_thread_req_count and boolean sys_thread_in_use.
> > > > - Field optee_call_waiter::sys_thread is replaced with 2 fields:
> > > >   sys_thread_req and sys_thread_used.
> > > > - Adds inline description comments for struct optee_call_queue and
> > > >   struct optee_call_waiter.
> > > >
> > > > Changes since v6:
> > > > - Moved out changes related to adding boolean system thread attribute
> > > >   into optee driver call queue and SMC/FF-A ABIs API functions. These
> > > >   changes were squashed into patch 1/4 of this patch v7 series.
> > > > - Comment about adding a specific commit for call queue refactoring
> > > >   was not addressed such a patch would only introduce function
> > > >   optee_cq_init()  with very little content in (mutex & list init).
> > > > - Added Co-developed-by tag for Jens contribution as he's not responsible
> > > >   for the changes I made in this patch v7.
> > > >
> > > > No change since v5
> > > >
> > > > Changes since v4:
> > > > - New change that supersedes implementation proposed in PATCH v4
> > > >   (tee: system invocation"). Thanks to Jens implementation we don't need
> > > >   the new OP-TEE services that my previous patch versions introduced to
> > > >   monitor system threads entry. Now, Linux optee SMC ABI driver gets TEE
> > > >   provisioned thread contexts count once and monitors thread entries in
> > > >   OP-TEE on that basis and the system thread capability of the related
> > > >   tee session. By the way, I dropped the WARN_ONCE() call I suggested
> > > >   on tee thread exhaustion as it does not provides useful information.
> > > > ---
> > > >  drivers/tee/optee/call.c          | 128 ++++++++++++++++++++++++++++--
> > > >  drivers/tee/optee/ffa_abi.c       |   3 +-
> > > >  drivers/tee/optee/optee_private.h |  24 +++++-
> > > >  drivers/tee/optee/smc_abi.c       |  16 +++-
> > > >  4 files changed, 159 insertions(+), 12 deletions(-)
> > > >
> > > > diff --git a/drivers/tee/optee/call.c b/drivers/tee/optee/call.c
> > > > index 152ae9bb1785..38543538d77b 100644
> > > > --- a/drivers/tee/optee/call.c
> > > > +++ b/drivers/tee/optee/call.c
> > > > @@ -39,9 +39,31 @@ struct optee_shm_arg_entry {
> > > >         DECLARE_BITMAP(map, MAX_ARG_COUNT_PER_ENTRY);
> > > >  };
> > > >
> > > > +void optee_cq_init(struct optee_call_queue *cq, int thread_count)
> > > > +{
> > > > +       mutex_init(&cq->mutex);
> > > > +       INIT_LIST_HEAD(&cq->sys_waiters);
> > > > +       INIT_LIST_HEAD(&cq->normal_waiters);
> > > > +
> > > > +       /*
> > > > +        * If cq->total_thread_count is 0 then we're not trying to keep
> > > > +        * track of how many free threads we have, instead we're relying on
> > > > +        * the secure world to tell us when we're out of thread and have to
> > > > +        * wait for another thread to become available.
> > > > +        */
> > > > +       cq->total_thread_count = thread_count;
> > > > +       cq->free_thread_count = thread_count;
> > > > +}
> > > > +
> > > >  void optee_cq_wait_init(struct optee_call_queue *cq,
> > > >                         struct optee_call_waiter *w, bool sys_thread)
> > > >  {
> > > > +       unsigned int free_thread_threshold;
> > > > +       bool need_wait = false;
> > > > +
> > > > +       memset(w, 0, sizeof(*w));
> > > > +       w->sys_thread = sys_thread;
> > > > +
> > > >         /*
> > > >          * We're preparing to make a call to secure world. In case we can't
> > > >          * allocate a thread in secure world we'll end up waiting in
> > > > @@ -53,15 +75,47 @@ void optee_cq_wait_init(struct optee_call_queue *cq,
> > > >         mutex_lock(&cq->mutex);
> > > >
> > > >         /*
> > > > -        * We add ourselves to the queue, but we don't wait. This
> > > > -        * guarantees that we don't lose a completion if secure world
> > > > -        * returns busy and another thread just exited and try to complete
> > > > -        * someone.
> > > > +        * We add ourselves to a queue, but we don't wait. This guarantees
> > > > +        * that we don't lose a completion if secure world returns busy and
> > > > +        * another thread just exited and try to complete someone.
> > > >          */
> > > >         init_completion(&w->c);
> > > > -       list_add_tail(&w->list_node, &cq->waiters);
> > > > +
> > > > +       if (sys_thread)
> > > > +               list_add_tail(&w->list_node, &cq->sys_waiters);
> > > > +       else
> > > > +               list_add_tail(&w->list_node, &cq->normal_waiters);
> > > > +
> > > > +       if (cq->total_thread_count) {
> > > > +               if (sys_thread || !cq->sys_thread_req_count)
> > > > +                       free_thread_threshold = 0;
> > > > +               else
> > > > +                       free_thread_threshold = 1;
> > > > +
> > > > +               if (cq->free_thread_count > free_thread_threshold)
> > > > +                       cq->free_thread_count--;
> > > > +               else
> > > > +                       need_wait = true;
> > > > +       }
> > > >
> > > >         mutex_unlock(&cq->mutex);
> > > > +
> > > > +       while (need_wait) {
> > > > +               optee_cq_wait_for_completion(cq, w);
> > > > +               mutex_lock(&cq->mutex);
> > > > +
> > > > +               if (sys_thread || !cq->sys_thread_req_count)
> > > > +                       free_thread_threshold = 0;
> > > > +               else
> > > > +                       free_thread_threshold = 1;
> > > > +
> > > > +               if (cq->free_thread_count > free_thread_threshold) {
> > > > +                       cq->free_thread_count--;
> > > > +                       need_wait = false;
> > > > +               }
> > > > +
> > > > +               mutex_unlock(&cq->mutex);
> > > > +       }
> > > >  }
> > > >
> > > >  void optee_cq_wait_for_completion(struct optee_call_queue *cq,
> > > > @@ -74,7 +128,11 @@ void optee_cq_wait_for_completion(struct optee_call_queue *cq,
> > > >         /* Move to end of list to get out of the way for other waiters */
> > > >         list_del(&w->list_node);
> > > >         reinit_completion(&w->c);
> > > > -       list_add_tail(&w->list_node, &cq->waiters);
> > > > +
> > > > +       if (w->sys_thread)
> > > > +               list_add_tail(&w->list_node, &cq->sys_waiters);
> > > > +       else
> > > > +               list_add_tail(&w->list_node, &cq->normal_waiters);
> > > >
> > > >         mutex_unlock(&cq->mutex);
> > > >  }
> > > > @@ -83,7 +141,15 @@ static void optee_cq_complete_one(struct optee_call_queue *cq)
> > > >  {
> > > >         struct optee_call_waiter *w;
> > > >
> > > > -       list_for_each_entry(w, &cq->waiters, list_node) {
> > > > +       /* Wake waiting system session first */
> > > > +       list_for_each_entry(w, &cq->sys_waiters, list_node) {
> > > > +               if (!completion_done(&w->c)) {
> > > > +                       complete(&w->c);
> > > > +                       break;

I see a bug here. The loop should return straight (s/break/return/)
without attempting to wake a normal waiter.

> > > > +               }
> > > > +       }
> > > > +
> > > > +       list_for_each_entry(w, &cq->normal_waiters, list_node) {
> > > >                 if (!completion_done(&w->c)) {
> > > >                         complete(&w->c);
> > > >                         break;
> > > > @@ -104,6 +170,8 @@ void optee_cq_wait_final(struct optee_call_queue *cq,
> > > >         /* Get out of the list */
> > > >         list_del(&w->list_node);
> > > >
> > > > +       cq->free_thread_count++;
> > > > +
> > > >         /* Wake up one eventual waiting task */
> > > >         optee_cq_complete_one(cq);
> > > >
> > > > @@ -119,6 +187,28 @@ void optee_cq_wait_final(struct optee_call_queue *cq,
> > > >         mutex_unlock(&cq->mutex);
> > > >  }
> > > >
> > > > +/* Count registered system sessions to reserved a system thread or not */
> > > > +static bool optee_cq_incr_sys_thread_count(struct optee_call_queue *cq)
> > > > +{
> > > > +       if (cq->total_thread_count <= 1)
> > > > +               return false;
> > > > +
> > > > +       mutex_lock(&cq->mutex);
> > > > +       cq->sys_thread_req_count++;
> > > > +       mutex_unlock(&cq->mutex);
> > > > +
> > > > +       return true;
> > > > +}
> > > > +
> > > > +static void optee_cq_decr_sys_thread_count(struct optee_call_queue *cq)
> > > > +{
> > > > +       mutex_lock(&cq->mutex);
> > > > +       cq->sys_thread_req_count--;
> > > > +       /* If there's someone waiting, let it resume */
> > > > +       optee_cq_complete_one(cq);
> > > > +       mutex_unlock(&cq->mutex);
> > > > +}
> > > > +
> > > >  /* Requires the filpstate mutex to be held */
> > > >  static struct optee_session *find_session(struct optee_context_data *ctxdata,
> > > >                                           u32 session_id)
> > > > @@ -361,6 +451,27 @@ int optee_open_session(struct tee_context *ctx,
> > > >         return rc;
> > > >  }
> > > >
> > > > +int optee_system_session(struct tee_context *ctx, u32 session)
> > > > +{
> > > > +       struct optee *optee = tee_get_drvdata(ctx->teedev);
> > > > +       struct optee_context_data *ctxdata = ctx->data;
> > > > +       struct optee_session *sess;
> > > > +       int rc = -EINVAL;
> > > > +
> > > > +       mutex_lock(&ctxdata->mutex);
> > > > +
> > > > +       sess = find_session(ctxdata, session);
> > > > +       if (sess && (sess->use_sys_thread ||
> > > > +                    optee_cq_incr_sys_thread_count(&optee->call_queue))) {
> > > > +               sess->use_sys_thread = true;
> > > > +               rc = 0;
> > > > +       }
> > > > +
> > > > +       mutex_unlock(&ctxdata->mutex);
> > > > +
> > > > +       return rc;
> > > > +}
> > > > +
> > > >  int optee_close_session_helper(struct tee_context *ctx, u32 session,
> > > >                                bool system_thread)
> > > >  {
> > > > @@ -380,6 +491,9 @@ int optee_close_session_helper(struct tee_context *ctx, u32 session,
> > > >
> > > >         optee_free_msg_arg(ctx, entry, offs);
> > > >
> > > > +       if (system_thread)
> > > > +               optee_cq_decr_sys_thread_count(&optee->call_queue);
> > > > +
> > > >         return 0;
> > > >  }
> > > >
> > > > diff --git a/drivers/tee/optee/ffa_abi.c b/drivers/tee/optee/ffa_abi.c
> > > > index 5fde9d4100e3..0c9055691343 100644
> > > > --- a/drivers/tee/optee/ffa_abi.c
> > > > +++ b/drivers/tee/optee/ffa_abi.c
> > > > @@ -852,8 +852,7 @@ static int optee_ffa_probe(struct ffa_device *ffa_dev)
> > > >         if (rc)
> > > >                 goto err_unreg_supp_teedev;
> > > >         mutex_init(&optee->ffa.mutex);
> > > > -       mutex_init(&optee->call_queue.mutex);
> > > > -       INIT_LIST_HEAD(&optee->call_queue.waiters);
> > > > +       optee_cq_init(&optee->call_queue, 0);
> > > >         optee_supp_init(&optee->supp);
> > > >         optee_shm_arg_cache_init(optee, arg_cache_flags);
> > > >         ffa_dev_set_drvdata(ffa_dev, optee);
> > > > diff --git a/drivers/tee/optee/optee_private.h b/drivers/tee/optee/optee_private.h
> > > > index b68273051454..69f6397c3646 100644
> > > > --- a/drivers/tee/optee/optee_private.h
> > > > +++ b/drivers/tee/optee/optee_private.h
> > > > @@ -40,15 +40,35 @@ typedef void (optee_invoke_fn)(unsigned long, unsigned long, unsigned long,
> > > >                                 unsigned long, unsigned long,
> > > >                                 struct arm_smccc_res *);
> > > >
> > > > +/*
> > > > + * struct optee_call_waiter - TEE entry may need to wait for a free TEE thread
> > > > + * @list_node          Reference in waiters list
> > > > + * @c                  Waiting completion reference
> > > > + * @sys_thread_req     True if waiter belongs to a system thread
> > > > + */
> > > >  struct optee_call_waiter {
> > > >         struct list_head list_node;
> > > >         struct completion c;
> > > > +       bool sys_thread;
> > > >  };
> > > >
> > > > +/*
> > > > + * struct optee_call_queue - OP-TEE call queue management
> > > > + * @mutex                      Serializes access to this struct
> > > > + * @sys_waiters                        List of system threads waiting to enter OP-TEE
> > > > + * @normal_waiters             List of normal threads waiting to enter OP-TEE
> > > > + * @total_thread_count         Overall number of thread context in OP-TEE or 0
> > > > + * @free_thread_count          Number of threads context free in OP-TEE
> > > > + * @sys_thread_req_count       Number of registered system thread sessions
> > > > + */
> > > >  struct optee_call_queue {
> > > >         /* Serializes access to this struct */
> > > >         struct mutex mutex;
> > > > -       struct list_head waiters;
> > > > +       struct list_head sys_waiters;
> > > > +       struct list_head normal_waiters;
> > > > +       int total_thread_count;
> > > > +       int free_thread_count;
> > > > +       int sys_thread_req_count;
> > > >  };
> > > >
> > > >  struct optee_notif {
> > > > @@ -254,6 +274,7 @@ int optee_supp_send(struct tee_context *ctx, u32 ret, u32 num_params,
> > > >  int optee_open_session(struct tee_context *ctx,
> > > >                        struct tee_ioctl_open_session_arg *arg,
> > > >                        struct tee_param *param);
> > > > +int optee_system_session(struct tee_context *ctx, u32 session);
> > > >  int optee_close_session_helper(struct tee_context *ctx, u32 session,
> > > >                                bool system_thread);
> > > >  int optee_close_session(struct tee_context *ctx, u32 session);
> > > > @@ -303,6 +324,7 @@ static inline void optee_to_msg_param_value(struct optee_msg_param *mp,
> > > >         mp->u.value.c = p->u.value.c;
> > > >  }
> > > >
> > > > +void optee_cq_init(struct optee_call_queue *cq, int thread_count);
> > > >  void optee_cq_wait_init(struct optee_call_queue *cq,
> > > >                         struct optee_call_waiter *w, bool sys_thread);
> > > >  void optee_cq_wait_for_completion(struct optee_call_queue *cq,
> > > > diff --git a/drivers/tee/optee/smc_abi.c b/drivers/tee/optee/smc_abi.c
> > > > index 1033d7da03ea..5595028d6dae 100644
> > > > --- a/drivers/tee/optee/smc_abi.c
> > > > +++ b/drivers/tee/optee/smc_abi.c
> > > > @@ -1211,6 +1211,7 @@ static const struct tee_driver_ops optee_clnt_ops = {
> > > >         .release = optee_release,
> > > >         .open_session = optee_open_session,
> > > >         .close_session = optee_close_session,
> > > > +       .system_session = optee_system_session,
> > > >         .invoke_func = optee_invoke_func,
> > > >         .cancel_req = optee_cancel_req,
> > > >         .shm_register = optee_shm_register,
> > > > @@ -1358,6 +1359,16 @@ static bool optee_msg_exchange_capabilities(optee_invoke_fn *invoke_fn,
> > > >         return true;
> > > >  }
> > > >
> > > > +static unsigned int optee_msg_get_thread_count(optee_invoke_fn *invoke_fn)
> > > > +{
> > > > +       struct arm_smccc_res res;
> > > > +
> > > > +       invoke_fn(OPTEE_SMC_GET_THREAD_COUNT, 0, 0, 0, 0, 0, 0, 0, &res);
> > > > +       if (res.a0)
> > > > +               return 0;
> > > > +       return res.a1;
> > > > +}
> > > > +
> > > >  static struct tee_shm_pool *
> > > >  optee_config_shm_memremap(optee_invoke_fn *invoke_fn, void **memremaped_shm)
> > > >  {
> > > > @@ -1610,6 +1621,7 @@ static int optee_probe(struct platform_device *pdev)
> > > >         struct optee *optee = NULL;
> > > >         void *memremaped_shm = NULL;
> > > >         unsigned int rpc_param_count;
> > > > +       unsigned int thread_count;
> > > >         struct tee_device *teedev;
> > > >         struct tee_context *ctx;
> > > >         u32 max_notif_value;
> > > > @@ -1637,6 +1649,7 @@ static int optee_probe(struct platform_device *pdev)
> > > >                 return -EINVAL;
> > > >         }
> > > >
> > > > +       thread_count = optee_msg_get_thread_count(invoke_fn);
> > > >         if (!optee_msg_exchange_capabilities(invoke_fn, &sec_caps,
> > > >                                              &max_notif_value,
> > > >                                              &rpc_param_count)) {
> > > > @@ -1726,8 +1739,7 @@ static int optee_probe(struct platform_device *pdev)
> > > >         if (rc)
> > > >                 goto err_unreg_supp_teedev;
> > > >
> > > > -       mutex_init(&optee->call_queue.mutex);
> > > > -       INIT_LIST_HEAD(&optee->call_queue.waiters);
> > > > +       optee_cq_init(&optee->call_queue, thread_count);
> > > >         optee_supp_init(&optee->supp);
> > > >         optee->smc.memremaped_shm = memremaped_shm;
> > > >         optee->pool = pool;
> > > > --
> > > > 2.25.1
> > > >
> > >
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v10 3/4] tee: optee: support tracking system threads

[Sumit Garg]

On Fri, 13 Oct 2023 at 14:09, Etienne CARRIERE - foss
<etienne.carriere@foss.st.com> wrote:
>
> > From: Sumit Garg <sumit.garg@linaro.org>
> > Sent: Friday, October 13, 2023 9:21 AM
> >
> > On Wed, 11 Oct 2023 at 12:41, Etienne CARRIERE - foss
> > <etienne.carriere@foss.st.com> wrote:
> > >
> > > > From: Sumit Garg <sumit.garg@linaro.org>
> > > > Sent: Friday, October 6, 2023 11:33 AM
> > > >
> > > > On Tue, 3 Oct 2023 at 19:36, Etienne Carriere
> > > > <etienne.carriere@foss.st.com> wrote:
> > > > >
> > > > > Adds support in the OP-TEE driver to keep track of reserved system
> > > > > threads. The logic allows one OP-TEE thread to be reserved to TEE system
> > > > > sessions.
> > > > >
> > > > > The optee_cq_*() functions are updated to handle this if enabled,
> > > > > that is when TEE describes how many thread context it supports
> > > > > and when at least 1 session has registered as a system session
> > > > > (using tee_client_system_session()).
> > > > >
> > > > > For sake of simplicity, initialization of call queue management
> > > > > is factorized into new helper function optee_cq_init().
> > > > >
> > > > > The SMC ABI part of the driver enables this tracking, but the
> > > > > FF-A ABI part does not.
> > > > >
> > > > >
> > > > > Co-developed-by: Jens Wiklander <jens.wiklander@linaro.org>
> > > > > Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> > > > > Co-developed-by: Sumit Garg <sumit.garg@linaro.org>
> > > > > Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> > > > > Signed-off-by: Etienne Carriere <etienne.carriere@foss.st.com>
> > > > > ---
> > > > > Changes since v9:
> > > > > - Add a reference counter for TEE system thread provisioning. We reserve
> > > > >   a TEE thread context for system session only when there is at least
> > > > >   1 opened system session.
> > > > > - Use 2 wait queue lists, normal_waiters and sys_waiter, as proposed in
> > > > >   patch v8. Using a single list can prevent a waiting system thread from
> > > > >   being resumed if the executing system thread wakes a normal waiter in
> > > > >   the list.
> > > >
> > > > How would that be possible? The system thread wakeup
> > > > (free_thread_threshold = 0) is given priority over normal thread
> > > > wakeup (free_thread_threshold = 1). I think a single queue list would
> > > > be sufficient as demonstrated in v9.
> > > >
> > >
> > > Hello Sumit,
> > >
> > > I think a system session can be trapped waiting when using a single queue list.
> > > To have a chance to reach the TEE, a waiting thread must wait that a TEE thread comes out of the TEE and calls complete() on the waitqueue to wake next waiter.
> > >
> > > To illustrate, consider a 10 TEE threads configuration on TEE side (::total_thread_count=10 at init),
> > > and several TEE clients in Linux OS, including 2 system sessions, from 2 consumer drivers (::sys_thread_req_count=2).
> > >
> > > Imagine the 9 normal threads and the 1 system thread are in use. (::free_thread_count=0),
> > > Now comes the other system session: it goes to the waitqueue list.
> > > Now comes a normal session invocation: it goes to the waitqueue list, 1st position.
> > >
> > > Now, TEE system thread returns to Linux:
> > > It increments the counter, ::free_thread_count=1, and calls complete() for the waitequeue.
> > > The 1st element in the waitqueue list is the last entered normal session invocation.
> > > However, that waiter won't switch local boolean 'need_wait'  to false because ::free_thread_count=1 and ::sys_thread_req_count!=0.
> > > So no attempt to reach TEE and wake another waiter on return.
> > > At that point there is a system session in the waitqueue list that could enter TEE (::free_thread_count=1) but is waiting someone returns from the TEE.
> >
> > I suppose the following loop tries to wake-up every waiter to give
> > them a chance to enter OP-TEE. So with that system session would
> > always be prefered over normal session, right?
>
> No, the below loop will wake only the 1st waiter it finds in the list that is
> current waiting (completion_done() returns false). So if it finds a normal
> session, it will only wake this one which, in turn, will not try to reach the
> TEE from the while(need_wiat) loop in optee_cq_wait_init(), because there is
> not enough free threads. Because it does not reach the TEE, it will not
> it wake another waiter.
>

Okay I see your point, so how about the following change on top of v9.
I still think having 2 queues is an overkill here.

diff --git a/drivers/tee/optee/call.c b/drivers/tee/optee/call.c
index df5fb5410b72..47f57054d9b7 100644
--- a/drivers/tee/optee/call.c
+++ b/drivers/tee/optee/call.c
@@ -60,6 +60,7 @@ void optee_cq_wait_init(struct optee_call_queue *cq,
         */
        init_completion(&w->c);
        list_add_tail(&w->list_node, &cq->waiters);
+       w->sys_thread = sys_thread;

...

@@ -83,6 +84,14 @@ static void optee_cq_complete_one(struct
optee_call_queue *cq)
 {
        struct optee_call_waiter *w;

+       /* Try to wakeup system session capable threads first */
+       list_for_each_entry(w, &cq->waiters, list_node) {
+               if (!completion_done(&w->c) && w->sys_thread) {
+                       complete(&w->c);
+                       return;
+               }
+       }
+
        list_for_each_entry(w, &cq->waiters, list_node) {
                if (!completion_done(&w->c)) {
                        complete(&w->c);
diff --git a/drivers/tee/optee/optee_private.h
b/drivers/tee/optee/optee_private.h
index 6bb5cae09688..a7817ce9f90f 100644
--- a/drivers/tee/optee/optee_private.h
+++ b/drivers/tee/optee/optee_private.h
@@ -43,6 +43,7 @@ typedef void (optee_invoke_fn)(unsigned long,
unsigned long, unsigned long,
 struct optee_call_waiter {
        struct list_head list_node;
        struct completion c;
+       bool sys_thread;
 };

 struct optee_call_queue {

-Sumit

> >
> > static void optee_cq_complete_one(struct optee_call_queue *cq)
> > {
> >         struct optee_call_waiter *w;
> >
> >         list_for_each_entry(w, &cq->waiters, list_node) {
> >                 if (!completion_done(&w->c)) {
> >                         complete(&w->c);
> >                         break;
> >                 }
> >         }
> > }
> >
> > -Sumit
> >
>
> Note I've found a error in this patch v10, see below.
>
> BR,
> Etienne
>
>
> > >
> > > With 2 lists, we first treat system sessions to overcome that.
> > > Am I missing something?
> > >
> > > Best regards,
> > > Etienne
> > >
> > > > -Sumit
> > > >
> > > > > - Updated my e-mail address.
> > > > > - Rephrased a bit the commit message.
> > > > >
> > > > > Changes since patch v8
> > > > > - Patch v9 (reference below) attempted to simplify the implementation
> > > > >   https://lore.kernel.org/lkml/20230517143311.585080-1-sumit.garg@linaro.org/#t
> > > > >
> > > > > Changes since v7:
> > > > > - Changes the logic to reserve at most 1 call entry for system sessions
> > > > >   as per patches v6 and v7 discussion threads (the 2 below bullets)
> > > > >   and updates commit message accordingly.
> > > > > - Field optee_call_queue::res_sys_thread_count is replaced with 2 fields:
> > > > >   sys_thread_req_count and boolean sys_thread_in_use.
> > > > > - Field optee_call_waiter::sys_thread is replaced with 2 fields:
> > > > >   sys_thread_req and sys_thread_used.
> > > > > - Adds inline description comments for struct optee_call_queue and
> > > > >   struct optee_call_waiter.
> > > > >
> > > > > Changes since v6:
> > > > > - Moved out changes related to adding boolean system thread attribute
> > > > >   into optee driver call queue and SMC/FF-A ABIs API functions. These
> > > > >   changes were squashed into patch 1/4 of this patch v7 series.
> > > > > - Comment about adding a specific commit for call queue refactoring
> > > > >   was not addressed such a patch would only introduce function
> > > > >   optee_cq_init()  with very little content in (mutex & list init).
> > > > > - Added Co-developed-by tag for Jens contribution as he's not responsible
> > > > >   for the changes I made in this patch v7.
> > > > >
> > > > > No change since v5
> > > > >
> > > > > Changes since v4:
> > > > > - New change that supersedes implementation proposed in PATCH v4
> > > > >   (tee: system invocation"). Thanks to Jens implementation we don't need
> > > > >   the new OP-TEE services that my previous patch versions introduced to
> > > > >   monitor system threads entry. Now, Linux optee SMC ABI driver gets TEE
> > > > >   provisioned thread contexts count once and monitors thread entries in
> > > > >   OP-TEE on that basis and the system thread capability of the related
> > > > >   tee session. By the way, I dropped the WARN_ONCE() call I suggested
> > > > >   on tee thread exhaustion as it does not provides useful information.
> > > > > ---
> > > > >  drivers/tee/optee/call.c          | 128 ++++++++++++++++++++++++++++--
> > > > >  drivers/tee/optee/ffa_abi.c       |   3 +-
> > > > >  drivers/tee/optee/optee_private.h |  24 +++++-
> > > > >  drivers/tee/optee/smc_abi.c       |  16 +++-
> > > > >  4 files changed, 159 insertions(+), 12 deletions(-)
> > > > >
> > > > > diff --git a/drivers/tee/optee/call.c b/drivers/tee/optee/call.c
> > > > > index 152ae9bb1785..38543538d77b 100644
> > > > > --- a/drivers/tee/optee/call.c
> > > > > +++ b/drivers/tee/optee/call.c
> > > > > @@ -39,9 +39,31 @@ struct optee_shm_arg_entry {
> > > > >         DECLARE_BITMAP(map, MAX_ARG_COUNT_PER_ENTRY);
> > > > >  };
> > > > >
> > > > > +void optee_cq_init(struct optee_call_queue *cq, int thread_count)
> > > > > +{
> > > > > +       mutex_init(&cq->mutex);
> > > > > +       INIT_LIST_HEAD(&cq->sys_waiters);
> > > > > +       INIT_LIST_HEAD(&cq->normal_waiters);
> > > > > +
> > > > > +       /*
> > > > > +        * If cq->total_thread_count is 0 then we're not trying to keep
> > > > > +        * track of how many free threads we have, instead we're relying on
> > > > > +        * the secure world to tell us when we're out of thread and have to
> > > > > +        * wait for another thread to become available.
> > > > > +        */
> > > > > +       cq->total_thread_count = thread_count;
> > > > > +       cq->free_thread_count = thread_count;
> > > > > +}
> > > > > +
> > > > >  void optee_cq_wait_init(struct optee_call_queue *cq,
> > > > >                         struct optee_call_waiter *w, bool sys_thread)
> > > > >  {
> > > > > +       unsigned int free_thread_threshold;
> > > > > +       bool need_wait = false;
> > > > > +
> > > > > +       memset(w, 0, sizeof(*w));
> > > > > +       w->sys_thread = sys_thread;
> > > > > +
> > > > >         /*
> > > > >          * We're preparing to make a call to secure world. In case we can't
> > > > >          * allocate a thread in secure world we'll end up waiting in
> > > > > @@ -53,15 +75,47 @@ void optee_cq_wait_init(struct optee_call_queue *cq,
> > > > >         mutex_lock(&cq->mutex);
> > > > >
> > > > >         /*
> > > > > -        * We add ourselves to the queue, but we don't wait. This
> > > > > -        * guarantees that we don't lose a completion if secure world
> > > > > -        * returns busy and another thread just exited and try to complete
> > > > > -        * someone.
> > > > > +        * We add ourselves to a queue, but we don't wait. This guarantees
> > > > > +        * that we don't lose a completion if secure world returns busy and
> > > > > +        * another thread just exited and try to complete someone.
> > > > >          */
> > > > >         init_completion(&w->c);
> > > > > -       list_add_tail(&w->list_node, &cq->waiters);
> > > > > +
> > > > > +       if (sys_thread)
> > > > > +               list_add_tail(&w->list_node, &cq->sys_waiters);
> > > > > +       else
> > > > > +               list_add_tail(&w->list_node, &cq->normal_waiters);
> > > > > +
> > > > > +       if (cq->total_thread_count) {
> > > > > +               if (sys_thread || !cq->sys_thread_req_count)
> > > > > +                       free_thread_threshold = 0;
> > > > > +               else
> > > > > +                       free_thread_threshold = 1;
> > > > > +
> > > > > +               if (cq->free_thread_count > free_thread_threshold)
> > > > > +                       cq->free_thread_count--;
> > > > > +               else
> > > > > +                       need_wait = true;
> > > > > +       }
> > > > >
> > > > >         mutex_unlock(&cq->mutex);
> > > > > +
> > > > > +       while (need_wait) {
> > > > > +               optee_cq_wait_for_completion(cq, w);
> > > > > +               mutex_lock(&cq->mutex);
> > > > > +
> > > > > +               if (sys_thread || !cq->sys_thread_req_count)
> > > > > +                       free_thread_threshold = 0;
> > > > > +               else
> > > > > +                       free_thread_threshold = 1;
> > > > > +
> > > > > +               if (cq->free_thread_count > free_thread_threshold) {
> > > > > +                       cq->free_thread_count--;
> > > > > +                       need_wait = false;
> > > > > +               }
> > > > > +
> > > > > +               mutex_unlock(&cq->mutex);
> > > > > +       }
> > > > >  }
> > > > >
> > > > >  void optee_cq_wait_for_completion(struct optee_call_queue *cq,
> > > > > @@ -74,7 +128,11 @@ void optee_cq_wait_for_completion(struct optee_call_queue *cq,
> > > > >         /* Move to end of list to get out of the way for other waiters */
> > > > >         list_del(&w->list_node);
> > > > >         reinit_completion(&w->c);
> > > > > -       list_add_tail(&w->list_node, &cq->waiters);
> > > > > +
> > > > > +       if (w->sys_thread)
> > > > > +               list_add_tail(&w->list_node, &cq->sys_waiters);
> > > > > +       else
> > > > > +               list_add_tail(&w->list_node, &cq->normal_waiters);
> > > > >
> > > > >         mutex_unlock(&cq->mutex);
> > > > >  }
> > > > > @@ -83,7 +141,15 @@ static void optee_cq_complete_one(struct optee_call_queue *cq)
> > > > >  {
> > > > >         struct optee_call_waiter *w;
> > > > >
> > > > > -       list_for_each_entry(w, &cq->waiters, list_node) {
> > > > > +       /* Wake waiting system session first */
> > > > > +       list_for_each_entry(w, &cq->sys_waiters, list_node) {
> > > > > +               if (!completion_done(&w->c)) {
> > > > > +                       complete(&w->c);
> > > > > +                       break;
>
> I see a bug here. The loop should return straight (s/break/return/)
> without attempting to wake a normal waiter.
>
> > > > > +               }
> > > > > +       }
> > > > > +
> > > > > +       list_for_each_entry(w, &cq->normal_waiters, list_node) {
> > > > >                 if (!completion_done(&w->c)) {
> > > > >                         complete(&w->c);
> > > > >                         break;
> > > > > @@ -104,6 +170,8 @@ void optee_cq_wait_final(struct optee_call_queue *cq,
> > > > >         /* Get out of the list */
> > > > >         list_del(&w->list_node);
> > > > >
> > > > > +       cq->free_thread_count++;
> > > > > +
> > > > >         /* Wake up one eventual waiting task */
> > > > >         optee_cq_complete_one(cq);
> > > > >
> > > > > @@ -119,6 +187,28 @@ void optee_cq_wait_final(struct optee_call_queue *cq,
> > > > >         mutex_unlock(&cq->mutex);
> > > > >  }
> > > > >
> > > > > +/* Count registered system sessions to reserved a system thread or not */
> > > > > +static bool optee_cq_incr_sys_thread_count(struct optee_call_queue *cq)
> > > > > +{
> > > > > +       if (cq->total_thread_count <= 1)
> > > > > +               return false;
> > > > > +
> > > > > +       mutex_lock(&cq->mutex);
> > > > > +       cq->sys_thread_req_count++;
> > > > > +       mutex_unlock(&cq->mutex);
> > > > > +
> > > > > +       return true;
> > > > > +}
> > > > > +
> > > > > +static void optee_cq_decr_sys_thread_count(struct optee_call_queue *cq)
> > > > > +{
> > > > > +       mutex_lock(&cq->mutex);
> > > > > +       cq->sys_thread_req_count--;
> > > > > +       /* If there's someone waiting, let it resume */
> > > > > +       optee_cq_complete_one(cq);
> > > > > +       mutex_unlock(&cq->mutex);
> > > > > +}
> > > > > +
> > > > >  /* Requires the filpstate mutex to be held */
> > > > >  static struct optee_session *find_session(struct optee_context_data *ctxdata,
> > > > >                                           u32 session_id)
> > > > > @@ -361,6 +451,27 @@ int optee_open_session(struct tee_context *ctx,
> > > > >         return rc;
> > > > >  }
> > > > >
> > > > > +int optee_system_session(struct tee_context *ctx, u32 session)
> > > > > +{
> > > > > +       struct optee *optee = tee_get_drvdata(ctx->teedev);
> > > > > +       struct optee_context_data *ctxdata = ctx->data;
> > > > > +       struct optee_session *sess;
> > > > > +       int rc = -EINVAL;
> > > > > +
> > > > > +       mutex_lock(&ctxdata->mutex);
> > > > > +
> > > > > +       sess = find_session(ctxdata, session);
> > > > > +       if (sess && (sess->use_sys_thread ||
> > > > > +                    optee_cq_incr_sys_thread_count(&optee->call_queue))) {
> > > > > +               sess->use_sys_thread = true;
> > > > > +               rc = 0;
> > > > > +       }
> > > > > +
> > > > > +       mutex_unlock(&ctxdata->mutex);
> > > > > +
> > > > > +       return rc;
> > > > > +}
> > > > > +
> > > > >  int optee_close_session_helper(struct tee_context *ctx, u32 session,
> > > > >                                bool system_thread)
> > > > >  {
> > > > > @@ -380,6 +491,9 @@ int optee_close_session_helper(struct tee_context *ctx, u32 session,
> > > > >
> > > > >         optee_free_msg_arg(ctx, entry, offs);
> > > > >
> > > > > +       if (system_thread)
> > > > > +               optee_cq_decr_sys_thread_count(&optee->call_queue);
> > > > > +
> > > > >         return 0;
> > > > >  }
> > > > >
> > > > > diff --git a/drivers/tee/optee/ffa_abi.c b/drivers/tee/optee/ffa_abi.c
> > > > > index 5fde9d4100e3..0c9055691343 100644
> > > > > --- a/drivers/tee/optee/ffa_abi.c
> > > > > +++ b/drivers/tee/optee/ffa_abi.c
> > > > > @@ -852,8 +852,7 @@ static int optee_ffa_probe(struct ffa_device *ffa_dev)
> > > > >         if (rc)
> > > > >                 goto err_unreg_supp_teedev;
> > > > >         mutex_init(&optee->ffa.mutex);
> > > > > -       mutex_init(&optee->call_queue.mutex);
> > > > > -       INIT_LIST_HEAD(&optee->call_queue.waiters);
> > > > > +       optee_cq_init(&optee->call_queue, 0);
> > > > >         optee_supp_init(&optee->supp);
> > > > >         optee_shm_arg_cache_init(optee, arg_cache_flags);
> > > > >         ffa_dev_set_drvdata(ffa_dev, optee);
> > > > > diff --git a/drivers/tee/optee/optee_private.h b/drivers/tee/optee/optee_private.h
> > > > > index b68273051454..69f6397c3646 100644
> > > > > --- a/drivers/tee/optee/optee_private.h
> > > > > +++ b/drivers/tee/optee/optee_private.h
> > > > > @@ -40,15 +40,35 @@ typedef void (optee_invoke_fn)(unsigned long, unsigned long, unsigned long,
> > > > >                                 unsigned long, unsigned long,
> > > > >                                 struct arm_smccc_res *);
> > > > >
> > > > > +/*
> > > > > + * struct optee_call_waiter - TEE entry may need to wait for a free TEE thread
> > > > > + * @list_node          Reference in waiters list
> > > > > + * @c                  Waiting completion reference
> > > > > + * @sys_thread_req     True if waiter belongs to a system thread
> > > > > + */
> > > > >  struct optee_call_waiter {
> > > > >         struct list_head list_node;
> > > > >         struct completion c;
> > > > > +       bool sys_thread;
> > > > >  };
> > > > >
> > > > > +/*
> > > > > + * struct optee_call_queue - OP-TEE call queue management
> > > > > + * @mutex                      Serializes access to this struct
> > > > > + * @sys_waiters                        List of system threads waiting to enter OP-TEE
> > > > > + * @normal_waiters             List of normal threads waiting to enter OP-TEE
> > > > > + * @total_thread_count         Overall number of thread context in OP-TEE or 0
> > > > > + * @free_thread_count          Number of threads context free in OP-TEE
> > > > > + * @sys_thread_req_count       Number of registered system thread sessions
> > > > > + */
> > > > >  struct optee_call_queue {
> > > > >         /* Serializes access to this struct */
> > > > >         struct mutex mutex;
> > > > > -       struct list_head waiters;
> > > > > +       struct list_head sys_waiters;
> > > > > +       struct list_head normal_waiters;
> > > > > +       int total_thread_count;
> > > > > +       int free_thread_count;
> > > > > +       int sys_thread_req_count;
> > > > >  };
> > > > >
> > > > >  struct optee_notif {
> > > > > @@ -254,6 +274,7 @@ int optee_supp_send(struct tee_context *ctx, u32 ret, u32 num_params,
> > > > >  int optee_open_session(struct tee_context *ctx,
> > > > >                        struct tee_ioctl_open_session_arg *arg,
> > > > >                        struct tee_param *param);
> > > > > +int optee_system_session(struct tee_context *ctx, u32 session);
> > > > >  int optee_close_session_helper(struct tee_context *ctx, u32 session,
> > > > >                                bool system_thread);
> > > > >  int optee_close_session(struct tee_context *ctx, u32 session);
> > > > > @@ -303,6 +324,7 @@ static inline void optee_to_msg_param_value(struct optee_msg_param *mp,
> > > > >         mp->u.value.c = p->u.value.c;
> > > > >  }
> > > > >
> > > > > +void optee_cq_init(struct optee_call_queue *cq, int thread_count);
> > > > >  void optee_cq_wait_init(struct optee_call_queue *cq,
> > > > >                         struct optee_call_waiter *w, bool sys_thread);
> > > > >  void optee_cq_wait_for_completion(struct optee_call_queue *cq,
> > > > > diff --git a/drivers/tee/optee/smc_abi.c b/drivers/tee/optee/smc_abi.c
> > > > > index 1033d7da03ea..5595028d6dae 100644
> > > > > --- a/drivers/tee/optee/smc_abi.c
> > > > > +++ b/drivers/tee/optee/smc_abi.c
> > > > > @@ -1211,6 +1211,7 @@ static const struct tee_driver_ops optee_clnt_ops = {
> > > > >         .release = optee_release,
> > > > >         .open_session = optee_open_session,
> > > > >         .close_session = optee_close_session,
> > > > > +       .system_session = optee_system_session,
> > > > >         .invoke_func = optee_invoke_func,
> > > > >         .cancel_req = optee_cancel_req,
> > > > >         .shm_register = optee_shm_register,
> > > > > @@ -1358,6 +1359,16 @@ static bool optee_msg_exchange_capabilities(optee_invoke_fn *invoke_fn,
> > > > >         return true;
> > > > >  }
> > > > >
> > > > > +static unsigned int optee_msg_get_thread_count(optee_invoke_fn *invoke_fn)
> > > > > +{
> > > > > +       struct arm_smccc_res res;
> > > > > +
> > > > > +       invoke_fn(OPTEE_SMC_GET_THREAD_COUNT, 0, 0, 0, 0, 0, 0, 0, &res);
> > > > > +       if (res.a0)
> > > > > +               return 0;
> > > > > +       return res.a1;
> > > > > +}
> > > > > +
> > > > >  static struct tee_shm_pool *
> > > > >  optee_config_shm_memremap(optee_invoke_fn *invoke_fn, void **memremaped_shm)
> > > > >  {
> > > > > @@ -1610,6 +1621,7 @@ static int optee_probe(struct platform_device *pdev)
> > > > >         struct optee *optee = NULL;
> > > > >         void *memremaped_shm = NULL;
> > > > >         unsigned int rpc_param_count;
> > > > > +       unsigned int thread_count;
> > > > >         struct tee_device *teedev;
> > > > >         struct tee_context *ctx;
> > > > >         u32 max_notif_value;
> > > > > @@ -1637,6 +1649,7 @@ static int optee_probe(struct platform_device *pdev)
> > > > >                 return -EINVAL;
> > > > >         }
> > > > >
> > > > > +       thread_count = optee_msg_get_thread_count(invoke_fn);
> > > > >         if (!optee_msg_exchange_capabilities(invoke_fn, &sec_caps,
> > > > >                                              &max_notif_value,
> > > > >                                              &rpc_param_count)) {
> > > > > @@ -1726,8 +1739,7 @@ static int optee_probe(struct platform_device *pdev)
> > > > >         if (rc)
> > > > >                 goto err_unreg_supp_teedev;
> > > > >
> > > > > -       mutex_init(&optee->call_queue.mutex);
> > > > > -       INIT_LIST_HEAD(&optee->call_queue.waiters);
> > > > > +       optee_cq_init(&optee->call_queue, thread_count);
> > > > >         optee_supp_init(&optee->supp);
> > > > >         optee->smc.memremaped_shm = memremaped_shm;
> > > > >         optee->pool = pool;
> > > > > --
> > > > > 2.25.1
> > > > >
> > > >
> >

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v10 3/4] tee: optee: support tracking system threads

[Etienne CARRIERE - foss]

> From: Sumit Garg <sumit.garg@linaro.org>
> Sent: Friday, October 13, 2023 11:13 AM
> 
> On Fri, 13 Oct 2023 at 14:09, Etienne CARRIERE - foss
> <etienne.carriere@foss.st.com> wrote:
> >
> > > From: Sumit Garg <sumit.garg@linaro.org>
> > > Sent: Friday, October 13, 2023 9:21 AM
> > >
> > > On Wed, 11 Oct 2023 at 12:41, Etienne CARRIERE - foss
> > > <etienne.carriere@foss.st.com> wrote:
> > > >
> > > > > From: Sumit Garg <sumit.garg@linaro.org>
> > > > > Sent: Friday, October 6, 2023 11:33 AM
> > > > >
> > > > > On Tue, 3 Oct 2023 at 19:36, Etienne Carriere
> > > > > <etienne.carriere@foss.st.com> wrote:
> > > > > >
> > > > > > Adds support in the OP-TEE driver to keep track of reserved system
> > > > > > threads. The logic allows one OP-TEE thread to be reserved to TEE system
> > > > > > sessions.
> > > > > >
> > > > > > The optee_cq_*() functions are updated to handle this if enabled,
> > > > > > that is when TEE describes how many thread context it supports
> > > > > > and when at least 1 session has registered as a system session
> > > > > > (using tee_client_system_session()).
> > > > > >
> > > > > > For sake of simplicity, initialization of call queue management
> > > > > > is factorized into new helper function optee_cq_init().
> > > > > >
> > > > > > The SMC ABI part of the driver enables this tracking, but the
> > > > > > FF-A ABI part does not.
> > > > > >
> > > > > >
> > > > > > Co-developed-by: Jens Wiklander <jens.wiklander@linaro.org>
> > > > > > Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> > > > > > Co-developed-by: Sumit Garg <sumit.garg@linaro.org>
> > > > > > Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> > > > > > Signed-off-by: Etienne Carriere <etienne.carriere@foss.st.com>
> > > > > > ---
> > > > > > Changes since v9:
> > > > > > - Add a reference counter for TEE system thread provisioning. We reserve
> > > > > >   a TEE thread context for system session only when there is at least
> > > > > >   1 opened system session.
> > > > > > - Use 2 wait queue lists, normal_waiters and sys_waiter, as proposed in
> > > > > >   patch v8. Using a single list can prevent a waiting system thread from
> > > > > >   being resumed if the executing system thread wakes a normal waiter in
> > > > > >   the list.
> > > > >
> > > > > How would that be possible? The system thread wakeup
> > > > > (free_thread_threshold = 0) is given priority over normal thread
> > > > > wakeup (free_thread_threshold = 1). I think a single queue list would
> > > > > be sufficient as demonstrated in v9.
> > > > >
> > > >
> > > > Hello Sumit,
> > > >
> > > > I think a system session can be trapped waiting when using a single queue list.
> > > > To have a chance to reach the TEE, a waiting thread must wait that a TEE thread comes out of the TEE and calls complete() on the waitqueue to wake next waiter.
> > > >
> > > > To illustrate, consider a 10 TEE threads configuration on TEE side (::total_thread_count=10 at init),
> > > > and several TEE clients in Linux OS, including 2 system sessions, from 2 consumer drivers (::sys_thread_req_count=2).
> > > >
> > > > Imagine the 9 normal threads and the 1 system thread are in use. (::free_thread_count=0),
> > > > Now comes the other system session: it goes to the waitqueue list.
> > > > Now comes a normal session invocation: it goes to the waitqueue list, 1st position.
> > > >
> > > > Now, TEE system thread returns to Linux:
> > > > It increments the counter, ::free_thread_count=1, and calls complete() for the waitequeue.
> > > > The 1st element in the waitqueue list is the last entered normal session invocation.
> > > > However, that waiter won't switch local boolean 'need_wait'  to false because ::free_thread_count=1 and ::sys_thread_req_count!=0.
> > > > So no attempt to reach TEE and wake another waiter on return.
> > > > At that point there is a system session in the waitqueue list that could enter TEE (::free_thread_count=1) but is waiting someone returns from the TEE.
> > >
> > > I suppose the following loop tries to wake-up every waiter to give
> > > them a chance to enter OP-TEE. So with that system session would
> > > always be prefered over normal session, right?
> >
> > No, the below loop will wake only the 1st waiter it finds in the list that is
> > current waiting (completion_done() returns false). So if it finds a normal
> > session, it will only wake this one which, in turn, will not try to reach the
> > TEE from the while(need_wiat) loop in optee_cq_wait_init(), because there is
> > not enough free threads. Because it does not reach the TEE, it will not
> > it wake another waiter.
> >
> 
> Okay I see your point, so how about the following change on top of v9.
> I still think having 2 queues is an overkill here.
> 
> diff --git a/drivers/tee/optee/call.c b/drivers/tee/optee/call.c
> index df5fb5410b72..47f57054d9b7 100644
> --- a/drivers/tee/optee/call.c
> +++ b/drivers/tee/optee/call.c
> @@ -60,6 +60,7 @@ void optee_cq_wait_init(struct optee_call_queue *cq,
>          */
>         init_completion(&w->c);
>         list_add_tail(&w->list_node, &cq->waiters);
> +       w->sys_thread = sys_thread;
> 
> ...
> 
> @@ -83,6 +84,14 @@ static void optee_cq_complete_one(struct
> optee_call_queue *cq)
>  {
>         struct optee_call_waiter *w;
> 
> +       /* Try to wakeup system session capable threads first */
> +       list_for_each_entry(w, &cq->waiters, list_node) {
> +               if (!completion_done(&w->c) && w->sys_thread) {
> +                       complete(&w->c);
> +                       return;
> +               }
> +       }
> +

Indeed, looking for system sessions first in the list would address the issue.
I would test sys_thread firs: if (w->sys_thread && !completion_done(&w->c))

That said, is it better to have 2 lists or to have 1 list possibly scanned twice?
I'm fine with both ways.

etienne


>         list_for_each_entry(w, &cq->waiters, list_node) {
>                 if (!completion_done(&w->c)) {
>                         complete(&w->c);
> diff --git a/drivers/tee/optee/optee_private.h
> b/drivers/tee/optee/optee_private.h
> index 6bb5cae09688..a7817ce9f90f 100644
> --- a/drivers/tee/optee/optee_private.h
> +++ b/drivers/tee/optee/optee_private.h
> @@ -43,6 +43,7 @@ typedef void (optee_invoke_fn)(unsigned long,
> unsigned long, unsigned long,
>  struct optee_call_waiter {
>         struct list_head list_node;
>         struct completion c;
> +       bool sys_thread;
>  };
> 
>  struct optee_call_queue {
> 
> -Sumit
> 
> > >
> > > static void optee_cq_complete_one(struct optee_call_queue *cq)
> > > {
> > >         struct optee_call_waiter *w;
> > >
> > >         list_for_each_entry(w, &cq->waiters, list_node) {
> > >                 if (!completion_done(&w->c)) {
> > >                         complete(&w->c);
> > >                         break;
> > >                 }
> > >         }
> > > }
> > >
> > > -Sumit
> > >
> >
> > Note I've found a error in this patch v10, see below.
> >
> > BR,
> > Etienne
> >
> >
> > > >
> > > > With 2 lists, we first treat system sessions to overcome that.
> > > > Am I missing something?
> > > >
> > > > Best regards,
> > > > Etienne
> > > >
> > > > > -Sumit
> > > > >
(snip)
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v10 3/4] tee: optee: support tracking system threads

[Sumit Garg]

On Fri, 13 Oct 2023 at 14:53, Etienne CARRIERE - foss
<etienne.carriere@foss.st.com> wrote:
>
> > From: Sumit Garg <sumit.garg@linaro.org>
> > Sent: Friday, October 13, 2023 11:13 AM
> >
> > On Fri, 13 Oct 2023 at 14:09, Etienne CARRIERE - foss
> > <etienne.carriere@foss.st.com> wrote:
> > >
> > > > From: Sumit Garg <sumit.garg@linaro.org>
> > > > Sent: Friday, October 13, 2023 9:21 AM
> > > >
> > > > On Wed, 11 Oct 2023 at 12:41, Etienne CARRIERE - foss
> > > > <etienne.carriere@foss.st.com> wrote:
> > > > >
> > > > > > From: Sumit Garg <sumit.garg@linaro.org>
> > > > > > Sent: Friday, October 6, 2023 11:33 AM
> > > > > >
> > > > > > On Tue, 3 Oct 2023 at 19:36, Etienne Carriere
> > > > > > <etienne.carriere@foss.st.com> wrote:
> > > > > > >
> > > > > > > Adds support in the OP-TEE driver to keep track of reserved system
> > > > > > > threads. The logic allows one OP-TEE thread to be reserved to TEE system
> > > > > > > sessions.
> > > > > > >
> > > > > > > The optee_cq_*() functions are updated to handle this if enabled,
> > > > > > > that is when TEE describes how many thread context it supports
> > > > > > > and when at least 1 session has registered as a system session
> > > > > > > (using tee_client_system_session()).
> > > > > > >
> > > > > > > For sake of simplicity, initialization of call queue management
> > > > > > > is factorized into new helper function optee_cq_init().
> > > > > > >
> > > > > > > The SMC ABI part of the driver enables this tracking, but the
> > > > > > > FF-A ABI part does not.
> > > > > > >
> > > > > > >
> > > > > > > Co-developed-by: Jens Wiklander <jens.wiklander@linaro.org>
> > > > > > > Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> > > > > > > Co-developed-by: Sumit Garg <sumit.garg@linaro.org>
> > > > > > > Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> > > > > > > Signed-off-by: Etienne Carriere <etienne.carriere@foss.st.com>
> > > > > > > ---
> > > > > > > Changes since v9:
> > > > > > > - Add a reference counter for TEE system thread provisioning. We reserve
> > > > > > >   a TEE thread context for system session only when there is at least
> > > > > > >   1 opened system session.
> > > > > > > - Use 2 wait queue lists, normal_waiters and sys_waiter, as proposed in
> > > > > > >   patch v8. Using a single list can prevent a waiting system thread from
> > > > > > >   being resumed if the executing system thread wakes a normal waiter in
> > > > > > >   the list.
> > > > > >
> > > > > > How would that be possible? The system thread wakeup
> > > > > > (free_thread_threshold = 0) is given priority over normal thread
> > > > > > wakeup (free_thread_threshold = 1). I think a single queue list would
> > > > > > be sufficient as demonstrated in v9.
> > > > > >
> > > > >
> > > > > Hello Sumit,
> > > > >
> > > > > I think a system session can be trapped waiting when using a single queue list.
> > > > > To have a chance to reach the TEE, a waiting thread must wait that a TEE thread comes out of the TEE and calls complete() on the waitqueue to wake next waiter.
> > > > >
> > > > > To illustrate, consider a 10 TEE threads configuration on TEE side (::total_thread_count=10 at init),
> > > > > and several TEE clients in Linux OS, including 2 system sessions, from 2 consumer drivers (::sys_thread_req_count=2).
> > > > >
> > > > > Imagine the 9 normal threads and the 1 system thread are in use. (::free_thread_count=0),
> > > > > Now comes the other system session: it goes to the waitqueue list.
> > > > > Now comes a normal session invocation: it goes to the waitqueue list, 1st position.
> > > > >
> > > > > Now, TEE system thread returns to Linux:
> > > > > It increments the counter, ::free_thread_count=1, and calls complete() for the waitequeue.
> > > > > The 1st element in the waitqueue list is the last entered normal session invocation.
> > > > > However, that waiter won't switch local boolean 'need_wait'  to false because ::free_thread_count=1 and ::sys_thread_req_count!=0.
> > > > > So no attempt to reach TEE and wake another waiter on return.
> > > > > At that point there is a system session in the waitqueue list that could enter TEE (::free_thread_count=1) but is waiting someone returns from the TEE.
> > > >
> > > > I suppose the following loop tries to wake-up every waiter to give
> > > > them a chance to enter OP-TEE. So with that system session would
> > > > always be prefered over normal session, right?
> > >
> > > No, the below loop will wake only the 1st waiter it finds in the list that is
> > > current waiting (completion_done() returns false). So if it finds a normal
> > > session, it will only wake this one which, in turn, will not try to reach the
> > > TEE from the while(need_wiat) loop in optee_cq_wait_init(), because there is
> > > not enough free threads. Because it does not reach the TEE, it will not
> > > it wake another waiter.
> > >
> >
> > Okay I see your point, so how about the following change on top of v9.
> > I still think having 2 queues is an overkill here.
> >
> > diff --git a/drivers/tee/optee/call.c b/drivers/tee/optee/call.c
> > index df5fb5410b72..47f57054d9b7 100644
> > --- a/drivers/tee/optee/call.c
> > +++ b/drivers/tee/optee/call.c
> > @@ -60,6 +60,7 @@ void optee_cq_wait_init(struct optee_call_queue *cq,
> >          */
> >         init_completion(&w->c);
> >         list_add_tail(&w->list_node, &cq->waiters);
> > +       w->sys_thread = sys_thread;
> >
> > ...
> >
> > @@ -83,6 +84,14 @@ static void optee_cq_complete_one(struct
> > optee_call_queue *cq)
> >  {
> >         struct optee_call_waiter *w;
> >
> > +       /* Try to wakeup system session capable threads first */
> > +       list_for_each_entry(w, &cq->waiters, list_node) {
> > +               if (!completion_done(&w->c) && w->sys_thread) {
> > +                       complete(&w->c);
> > +                       return;
> > +               }
> > +       }
> > +
>
> Indeed, looking for system sessions first in the list would address the issue.
> I would test sys_thread firs: if (w->sys_thread && !completion_done(&w->c))

Ack.

>
> That said, is it better to have 2 lists or to have 1 list possibly scanned twice?

I would prefer to reuse the existing queue.

-Sumit

> I'm fine with both ways.
>
> etienne
>
>
> >         list_for_each_entry(w, &cq->waiters, list_node) {
> >                 if (!completion_done(&w->c)) {
> >                         complete(&w->c);
> > diff --git a/drivers/tee/optee/optee_private.h
> > b/drivers/tee/optee/optee_private.h
> > index 6bb5cae09688..a7817ce9f90f 100644
> > --- a/drivers/tee/optee/optee_private.h
> > +++ b/drivers/tee/optee/optee_private.h
> > @@ -43,6 +43,7 @@ typedef void (optee_invoke_fn)(unsigned long,
> > unsigned long, unsigned long,
> >  struct optee_call_waiter {
> >         struct list_head list_node;
> >         struct completion c;
> > +       bool sys_thread;
> >  };
> >
> >  struct optee_call_queue {
> >
> > -Sumit
> >
> > > >
> > > > static void optee_cq_complete_one(struct optee_call_queue *cq)
> > > > {
> > > >         struct optee_call_waiter *w;
> > > >
> > > >         list_for_each_entry(w, &cq->waiters, list_node) {
> > > >                 if (!completion_done(&w->c)) {
> > > >                         complete(&w->c);
> > > >                         break;
> > > >                 }
> > > >         }
> > > > }
> > > >
> > > > -Sumit
> > > >
> > >
> > > Note I've found a error in this patch v10, see below.
> > >
> > > BR,
> > > Etienne
> > >
> > >
> > > > >
> > > > > With 2 lists, we first treat system sessions to overcome that.
> > > > > Am I missing something?
> > > > >
> > > > > Best regards,
> > > > > Etienne
> > > > >
> > > > > > -Sumit
> > > > > >
> (snip)

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v10 3/4] tee: optee: support tracking system threads

[Etienne CARRIERE - foss]

> From: Sumit Garg <sumit.garg@linaro.org>
> Sent: Friday, October 13, 2023 11:36 AM
> 
> On Fri, 13 Oct 2023 at 14:53, Etienne CARRIERE - foss
> <etienne.carriere@foss.st.com> wrote:
> >
> > > From: Sumit Garg <sumit.garg@linaro.org>
> > > Sent: Friday, October 13, 2023 11:13 AM
> > >
> > > On Fri, 13 Oct 2023 at 14:09, Etienne CARRIERE - foss
> > > <etienne.carriere@foss.st.com> wrote:
> > > >
> > > > > From: Sumit Garg <sumit.garg@linaro.org>
> > > > > Sent: Friday, October 13, 2023 9:21 AM
> > > > >
> > > > > On Wed, 11 Oct 2023 at 12:41, Etienne CARRIERE - foss
> > > > > <etienne.carriere@foss.st.com> wrote:
> > > > > >
> > > > > > > From: Sumit Garg <sumit.garg@linaro.org>
> > > > > > > Sent: Friday, October 6, 2023 11:33 AM
> > > > > > >
> > > > > > > On Tue, 3 Oct 2023 at 19:36, Etienne Carriere
> > > > > > > <etienne.carriere@foss.st.com> wrote:
> > > > > > > >
> > > > > > > > Adds support in the OP-TEE driver to keep track of reserved system
> > > > > > > > threads. The logic allows one OP-TEE thread to be reserved to TEE system
> > > > > > > > sessions.
> > > > > > > >
> > > > > > > > The optee_cq_*() functions are updated to handle this if enabled,
> > > > > > > > that is when TEE describes how many thread context it supports
> > > > > > > > and when at least 1 session has registered as a system session
> > > > > > > > (using tee_client_system_session()).
> > > > > > > >
> > > > > > > > For sake of simplicity, initialization of call queue management
> > > > > > > > is factorized into new helper function optee_cq_init().
> > > > > > > >
> > > > > > > > The SMC ABI part of the driver enables this tracking, but the
> > > > > > > > FF-A ABI part does not.
> > > > > > > >
> > > > > > > >
> > > > > > > > Co-developed-by: Jens Wiklander <jens.wiklander@linaro.org>
> > > > > > > > Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> > > > > > > > Co-developed-by: Sumit Garg <sumit.garg@linaro.org>
> > > > > > > > Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> > > > > > > > Signed-off-by: Etienne Carriere <etienne.carriere@foss.st.com>
> > > > > > > > ---
> > > > > > > > Changes since v9:
> > > > > > > > - Add a reference counter for TEE system thread provisioning. We reserve
> > > > > > > >   a TEE thread context for system session only when there is at least
> > > > > > > >   1 opened system session.
> > > > > > > > - Use 2 wait queue lists, normal_waiters and sys_waiter, as proposed in
> > > > > > > >   patch v8. Using a single list can prevent a waiting system thread from
> > > > > > > >   being resumed if the executing system thread wakes a normal waiter in
> > > > > > > >   the list.
> > > > > > >
> > > > > > > How would that be possible? The system thread wakeup
> > > > > > > (free_thread_threshold = 0) is given priority over normal thread
> > > > > > > wakeup (free_thread_threshold = 1). I think a single queue list would
> > > > > > > be sufficient as demonstrated in v9.
> > > > > > >
> > > > > >
> > > > > > Hello Sumit,
> > > > > >
> > > > > > I think a system session can be trapped waiting when using a single queue list.
> > > > > > To have a chance to reach the TEE, a waiting thread must wait that a TEE thread comes out of the TEE and calls complete() on the waitqueue to wake next waiter.
> > > > > >
> > > > > > To illustrate, consider a 10 TEE threads configuration on TEE side (::total_thread_count=10 at init),
> > > > > > and several TEE clients in Linux OS, including 2 system sessions, from 2 consumer drivers (::sys_thread_req_count=2).
> > > > > >
> > > > > > Imagine the 9 normal threads and the 1 system thread are in use. (::free_thread_count=0),
> > > > > > Now comes the other system session: it goes to the waitqueue list.
> > > > > > Now comes a normal session invocation: it goes to the waitqueue list, 1st position.
> > > > > >
> > > > > > Now, TEE system thread returns to Linux:
> > > > > > It increments the counter, ::free_thread_count=1, and calls complete() for the waitequeue.
> > > > > > The 1st element in the waitqueue list is the last entered normal session invocation.
> > > > > > However, that waiter won't switch local boolean 'need_wait'  to false because ::free_thread_count=1 and ::sys_thread_req_count!=0.
> > > > > > So no attempt to reach TEE and wake another waiter on return.
> > > > > > At that point there is a system session in the waitqueue list that could enter TEE (::free_thread_count=1) but is waiting someone returns from the TEE.
> > > > >
> > > > > I suppose the following loop tries to wake-up every waiter to give
> > > > > them a chance to enter OP-TEE. So with that system session would
> > > > > always be prefered over normal session, right?
> > > >
> > > > No, the below loop will wake only the 1st waiter it finds in the list that is
> > > > current waiting (completion_done() returns false). So if it finds a normal
> > > > session, it will only wake this one which, in turn, will not try to reach the
> > > > TEE from the while(need_wiat) loop in optee_cq_wait_init(), because there is
> > > > not enough free threads. Because it does not reach the TEE, it will not
> > > > it wake another waiter.
> > > >
> > >
> > > Okay I see your point, so how about the following change on top of v9.
> > > I still think having 2 queues is an overkill here.
> > >
> > > diff --git a/drivers/tee/optee/call.c b/drivers/tee/optee/call.c
> > > index df5fb5410b72..47f57054d9b7 100644
> > > --- a/drivers/tee/optee/call.c
> > > +++ b/drivers/tee/optee/call.c
> > > @@ -60,6 +60,7 @@ void optee_cq_wait_init(struct optee_call_queue *cq,
> > >          */
> > >         init_completion(&w->c);
> > >         list_add_tail(&w->list_node, &cq->waiters);
> > > +       w->sys_thread = sys_thread;
> > >
> > > ...
> > >
> > > @@ -83,6 +84,14 @@ static void optee_cq_complete_one(struct
> > > optee_call_queue *cq)
> > >  {
> > >         struct optee_call_waiter *w;
> > >
> > > +       /* Try to wakeup system session capable threads first */
> > > +       list_for_each_entry(w, &cq->waiters, list_node) {
> > > +               if (!completion_done(&w->c) && w->sys_thread) {
> > > +                       complete(&w->c);
> > > +                       return;
> > > +               }
> > > +       }
> > > +
> >
> > Indeed, looking for system sessions first in the list would address the issue.
> > I would test sys_thread firs: if (w->sys_thread && !completion_done(&w->c))
> 
> Ack.
> 
> >
> > That said, is it better to have 2 lists or to have 1 list possibly scanned twice?
> 
> I would prefer to reuse the existing queue.

Ok, I'll do.

BR,
Etienne

> 
> -Sumit
> 
> > I'm fine with both ways.
> >
> > etienne
> >
> >
> > >         list_for_each_entry(w, &cq->waiters, list_node) {
> > >                 if (!completion_done(&w->c)) {
> > >                         complete(&w->c);
> > > diff --git a/drivers/tee/optee/optee_private.h
> > > b/drivers/tee/optee/optee_private.h
> > > index 6bb5cae09688..a7817ce9f90f 100644
> > > --- a/drivers/tee/optee/optee_private.h
> > > +++ b/drivers/tee/optee/optee_private.h
> > > @@ -43,6 +43,7 @@ typedef void (optee_invoke_fn)(unsigned long,
> > > unsigned long, unsigned long,
> > >  struct optee_call_waiter {
> > >         struct list_head list_node;
> > >         struct completion c;
> > > +       bool sys_thread;
> > >  };
> > >
> > >  struct optee_call_queue {
> > >
> > > -Sumit
> > >
> > > > >
> > > > > static void optee_cq_complete_one(struct optee_call_queue *cq)
> > > > > {
> > > > >         struct optee_call_waiter *w;
> > > > >
> > > > >         list_for_each_entry(w, &cq->waiters, list_node) {
> > > > >                 if (!completion_done(&w->c)) {
> > > > >                         complete(&w->c);
> > > > >                         break;
> > > > >                 }
> > > > >         }
> > > > > }
> > > > >
> > > > > -Sumit
> > > > >
> > > >
> > > > Note I've found a error in this patch v10, see below.
> > > >
> > > > BR,
> > > > Etienne
> > > >
> > > >
> > > > > >
> > > > > > With 2 lists, we first treat system sessions to overcome that.
> > > > > > Am I missing something?
> > > > > >
> > > > > > Best regards,
> > > > > > Etienne
> > > > > >
> > > > > > > -Sumit
> > > > > > >
> > (snip)
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v10 4/4] firmware: arm_scmi: optee: use optee system invocation

[Etienne Carriere]

Changes SCMI optee transport to call tee_client_system_session()
to request optee driver to provision an entry context in OP-TEE
for processing OP-TEE messages. This prevents possible deadlock
in case OP-TEE threads are all consumed while these may be waiting
for a clock or regulator to be enable which SCMI OP-TEE service which
requires a free thread context to execute.

Cc: Sudeep Holla <sudeep.holla@arm.com>
Cc: Cristian Marussi <cristian.marussi@arm.com>
Acked-by: Sudeep Holla <sudeep.holla@arm.com>
Reviewed-by: Sumit Garg <sumit.garg@linaro.org>
Signed-off-by: Etienne Carriere <etienne.carriere@foss.st.com>
---
Changes since v9:
- Applied Sumit R-b tag:
  https://lore.kernel.org/lkml/CAFA6WYMyJrW25sdZRkQHDje72+tLDw4T+bjB6tmVf8XH0De1RQ@mail.gmail.com/
- Added Cc: tags and updated my e-mail address.

No change since v8
No change since v7
No change since v6

Changes since v5:
- Applied Sudeep's review tag

Changes since v4:
- Updated to new API function tee_client_system_session() introduced
  in patch v5 2/3.

No change since v3

Changes since v2:
- Fixed syntax issues (missing ';' chars), reported by kernel test robot.

Changes since v1:
- Updated to use new tee API functions tee_client_request_system_context()
  and tee_client_release_system_context().
---
 drivers/firmware/arm_scmi/optee.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/firmware/arm_scmi/optee.c b/drivers/firmware/arm_scmi/optee.c
index e123de6e8c67..25bfb465484d 100644
--- a/drivers/firmware/arm_scmi/optee.c
+++ b/drivers/firmware/arm_scmi/optee.c
@@ -440,6 +440,10 @@ static int scmi_optee_chan_setup(struct scmi_chan_info *cinfo, struct device *de
 	if (ret)
 		goto err_free_shm;
 
+	ret = tee_client_system_session(scmi_optee_private->tee_ctx, channel->tee_session);
+	if (ret)
+		dev_warn(dev, "Could not switch to system session, do best effort\n");
+
 	ret = get_channel(channel);
 	if (ret)
 		goto err_close_sess;
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel