[PATCH 1/2] tee: system invocation

[PATCH 1/2] tee: system invocation

[Etienne Carriere]

Adds TEE context flag sys_service to be enabled for invocation contexts
that should used TEE provisioned system resources. OP-TEE SMC ABI entry
rely this information to use a dedicated entry function to request
allocation of a system thread from a dedicated system context pool.

This feature is needed when a TEE invocation cannot afford to wait for
a free TEE thread when all TEE threads context are used and suspended
as these may be suspended waiting for a system service, as an SCMI clock
or voltage regulator, to be enabled. An example is when OP-TEE invokes
a Linux OS remove service (RPC) to access an eMMC RPMB partition and
the eMMC device is supplied by an OP-TEE SCMI regulator.

Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
---
 drivers/tee/optee/optee_smc.h | 14 +++++++++++---
 drivers/tee/optee/smc_abi.c   |  6 +++++-
 include/linux/tee_drv.h       |  4 ++++
 3 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/drivers/tee/optee/optee_smc.h b/drivers/tee/optee/optee_smc.h
index 73b5e7760d10..7c7eedf183c5 100644
--- a/drivers/tee/optee/optee_smc.h
+++ b/drivers/tee/optee/optee_smc.h
@@ -108,7 +108,8 @@ struct optee_smc_call_get_os_revision_result {
  * Call with struct optee_msg_arg as argument
  *
  * When called with OPTEE_SMC_CALL_WITH_RPC_ARG or
- * OPTEE_SMC_CALL_WITH_REGD_ARG in a0 there is one RPC struct optee_msg_arg
+ * OPTEE_SMC_CALL_WITH_REGD_ARG or OPTEE_SMC_FUNCID_CALL_SYSTEM_WITH_REGD_ARG
+ * in a0 there is one RPC struct optee_msg_arg
  * following after the first struct optee_msg_arg. The RPC struct
  * optee_msg_arg has reserved space for the number of RPC parameters as
  * returned by OPTEE_SMC_EXCHANGE_CAPABILITIES.
@@ -130,8 +131,8 @@ struct optee_smc_call_get_os_revision_result {
  * a4-6	Not used
  * a7	Hypervisor Client ID register
  *
- * Call register usage, OPTEE_SMC_CALL_WITH_REGD_ARG:
- * a0	SMC Function ID, OPTEE_SMC_CALL_WITH_REGD_ARG
+ * Call register usage, OPTEE_SMC_CALL_WITH_REGD_ARG and OPTEE_SMC_FUNCID_CALL_SYSTEM_WITH_REGD_ARG:
+ * a0	SMC Function ID, OPTEE_SMC_CALL_WITH_REGD_ARG or OPTEE_SMC_FUNCID_CALL_SYSTEM_WITH_REGD_ARG
  * a1	Upper 32 bits of a 64-bit shared memory cookie
  * a2	Lower 32 bits of a 64-bit shared memory cookie
  * a3	Offset of the struct optee_msg_arg in the shared memory with the
@@ -175,6 +176,8 @@ struct optee_smc_call_get_os_revision_result {
 	OPTEE_SMC_STD_CALL_VAL(OPTEE_SMC_FUNCID_CALL_WITH_RPC_ARG)
 #define OPTEE_SMC_CALL_WITH_REGD_ARG \
 	OPTEE_SMC_STD_CALL_VAL(OPTEE_SMC_FUNCID_CALL_WITH_REGD_ARG)
+#define OPTEE_SMC_CALL_SYSTEM_WITH_REGD_ARG \
+	OPTEE_SMC_STD_CALL_VAL(OPTEE_SMC_FUNCID_CALL_SYSTEM_WITH_REGD_ARG)
 
 /*
  * Get Shared Memory Config
@@ -254,6 +257,8 @@ struct optee_smc_get_shm_config_result {
 #define OPTEE_SMC_SEC_CAP_ASYNC_NOTIF		BIT(5)
 /* Secure world supports pre-allocating RPC arg struct */
 #define OPTEE_SMC_SEC_CAP_RPC_ARG		BIT(6)
+/* Secure world provisions thread for system service invocation */
+#define OPTEE_SMC_SEC_CAP_SYSTEM_THREAD		BIT(7)
 
 #define OPTEE_SMC_FUNCID_EXCHANGE_CAPABILITIES	9
 #define OPTEE_SMC_EXCHANGE_CAPABILITIES \
@@ -426,6 +431,9 @@ struct optee_smc_disable_shm_cache_result {
 /* See OPTEE_SMC_CALL_WITH_REGD_ARG above */
 #define OPTEE_SMC_FUNCID_CALL_WITH_REGD_ARG	19
 
+/* See OPTEE_SMC_CALL_SYSTEM_WITH_REGD_ARG above */
+#define OPTEE_SMC_FUNCID_CALL_SYSTEM_WITH_REGD_ARG	20
+
 /*
  * Resume from RPC (for example after processing a foreign interrupt)
  *
diff --git a/drivers/tee/optee/smc_abi.c b/drivers/tee/optee/smc_abi.c
index a1c1fa1a9c28..513038a138f6 100644
--- a/drivers/tee/optee/smc_abi.c
+++ b/drivers/tee/optee/smc_abi.c
@@ -889,7 +889,11 @@ static int optee_smc_do_call_with_arg(struct tee_context *ctx,
 	}
 
 	if  (rpc_arg && tee_shm_is_dynamic(shm)) {
-		param.a0 = OPTEE_SMC_CALL_WITH_REGD_ARG;
+		if (ctx->sys_service &&
+		    (optee->smc.sec_caps & OPTEE_SMC_SEC_CAP_SYSTEM_THREAD))
+			param.a0 = OPTEE_SMC_CALL_SYSTEM_WITH_REGD_ARG;
+		else
+			param.a0 = OPTEE_SMC_CALL_WITH_REGD_ARG;
 		reg_pair_from_64(&param.a1, &param.a2, (u_long)shm);
 		param.a3 = offs;
 	} else {
diff --git a/include/linux/tee_drv.h b/include/linux/tee_drv.h
index 17eb1c5205d3..1ff292ba7679 100644
--- a/include/linux/tee_drv.h
+++ b/include/linux/tee_drv.h
@@ -47,6 +47,9 @@ struct tee_shm_pool;
  *              non-blocking in nature.
  * @cap_memref_null: flag indicating if the TEE Client support shared
  *                   memory buffer with a NULL pointer.
+ * @sys_service: flag set by the TEE Client to indicate that it is part of
+ *		 a system service and that the TEE may use resources reserved
+ *		 for this.
  */
 struct tee_context {
 	struct tee_device *teedev;
@@ -55,6 +58,7 @@ struct tee_context {
 	bool releasing;
 	bool supp_nowait;
 	bool cap_memref_null;
+	bool sys_service;
 };
 
 struct tee_param_memref {
-- 
2.25.1

[PATCH 1/2] tee: system invocation

[Etienne Carriere]

Adds TEE context flag sys_service to be enabled for invocation contexts
that should used TEE provisioned system resources. OP-TEE SMC ABI entry
rely this information to use a dedicated entry function to request
allocation of a system thread from a dedicated system context pool.

This feature is needed when a TEE invocation cannot afford to wait for
a free TEE thread when all TEE threads context are used and suspended
as these may be suspended waiting for a system service, as an SCMI clock
or voltage regulator, to be enabled. An example is when OP-TEE invokes
a Linux OS remove service (RPC) to access an eMMC RPMB partition and
the eMMC device is supplied by an OP-TEE SCMI regulator.

Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
---
 drivers/tee/optee/optee_smc.h | 14 +++++++++++---
 drivers/tee/optee/smc_abi.c   |  6 +++++-
 include/linux/tee_drv.h       |  4 ++++
 3 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/drivers/tee/optee/optee_smc.h b/drivers/tee/optee/optee_smc.h
index 73b5e7760d10..7c7eedf183c5 100644
--- a/drivers/tee/optee/optee_smc.h
+++ b/drivers/tee/optee/optee_smc.h
@@ -108,7 +108,8 @@ struct optee_smc_call_get_os_revision_result {
  * Call with struct optee_msg_arg as argument
  *
  * When called with OPTEE_SMC_CALL_WITH_RPC_ARG or
- * OPTEE_SMC_CALL_WITH_REGD_ARG in a0 there is one RPC struct optee_msg_arg
+ * OPTEE_SMC_CALL_WITH_REGD_ARG or OPTEE_SMC_FUNCID_CALL_SYSTEM_WITH_REGD_ARG
+ * in a0 there is one RPC struct optee_msg_arg
  * following after the first struct optee_msg_arg. The RPC struct
  * optee_msg_arg has reserved space for the number of RPC parameters as
  * returned by OPTEE_SMC_EXCHANGE_CAPABILITIES.
@@ -130,8 +131,8 @@ struct optee_smc_call_get_os_revision_result {
  * a4-6	Not used
  * a7	Hypervisor Client ID register
  *
- * Call register usage, OPTEE_SMC_CALL_WITH_REGD_ARG:
- * a0	SMC Function ID, OPTEE_SMC_CALL_WITH_REGD_ARG
+ * Call register usage, OPTEE_SMC_CALL_WITH_REGD_ARG and OPTEE_SMC_FUNCID_CALL_SYSTEM_WITH_REGD_ARG:
+ * a0	SMC Function ID, OPTEE_SMC_CALL_WITH_REGD_ARG or OPTEE_SMC_FUNCID_CALL_SYSTEM_WITH_REGD_ARG
  * a1	Upper 32 bits of a 64-bit shared memory cookie
  * a2	Lower 32 bits of a 64-bit shared memory cookie
  * a3	Offset of the struct optee_msg_arg in the shared memory with the
@@ -175,6 +176,8 @@ struct optee_smc_call_get_os_revision_result {
 	OPTEE_SMC_STD_CALL_VAL(OPTEE_SMC_FUNCID_CALL_WITH_RPC_ARG)
 #define OPTEE_SMC_CALL_WITH_REGD_ARG \
 	OPTEE_SMC_STD_CALL_VAL(OPTEE_SMC_FUNCID_CALL_WITH_REGD_ARG)
+#define OPTEE_SMC_CALL_SYSTEM_WITH_REGD_ARG \
+	OPTEE_SMC_STD_CALL_VAL(OPTEE_SMC_FUNCID_CALL_SYSTEM_WITH_REGD_ARG)
 
 /*
  * Get Shared Memory Config
@@ -254,6 +257,8 @@ struct optee_smc_get_shm_config_result {
 #define OPTEE_SMC_SEC_CAP_ASYNC_NOTIF		BIT(5)
 /* Secure world supports pre-allocating RPC arg struct */
 #define OPTEE_SMC_SEC_CAP_RPC_ARG		BIT(6)
+/* Secure world provisions thread for system service invocation */
+#define OPTEE_SMC_SEC_CAP_SYSTEM_THREAD		BIT(7)
 
 #define OPTEE_SMC_FUNCID_EXCHANGE_CAPABILITIES	9
 #define OPTEE_SMC_EXCHANGE_CAPABILITIES \
@@ -426,6 +431,9 @@ struct optee_smc_disable_shm_cache_result {
 /* See OPTEE_SMC_CALL_WITH_REGD_ARG above */
 #define OPTEE_SMC_FUNCID_CALL_WITH_REGD_ARG	19
 
+/* See OPTEE_SMC_CALL_SYSTEM_WITH_REGD_ARG above */
+#define OPTEE_SMC_FUNCID_CALL_SYSTEM_WITH_REGD_ARG	20
+
 /*
  * Resume from RPC (for example after processing a foreign interrupt)
  *
diff --git a/drivers/tee/optee/smc_abi.c b/drivers/tee/optee/smc_abi.c
index a1c1fa1a9c28..513038a138f6 100644
--- a/drivers/tee/optee/smc_abi.c
+++ b/drivers/tee/optee/smc_abi.c
@@ -889,7 +889,11 @@ static int optee_smc_do_call_with_arg(struct tee_context *ctx,
 	}
 
 	if  (rpc_arg && tee_shm_is_dynamic(shm)) {
-		param.a0 = OPTEE_SMC_CALL_WITH_REGD_ARG;
+		if (ctx->sys_service &&
+		    (optee->smc.sec_caps & OPTEE_SMC_SEC_CAP_SYSTEM_THREAD))
+			param.a0 = OPTEE_SMC_CALL_SYSTEM_WITH_REGD_ARG;
+		else
+			param.a0 = OPTEE_SMC_CALL_WITH_REGD_ARG;
 		reg_pair_from_64(&param.a1, &param.a2, (u_long)shm);
 		param.a3 = offs;
 	} else {
diff --git a/include/linux/tee_drv.h b/include/linux/tee_drv.h
index 17eb1c5205d3..1ff292ba7679 100644
--- a/include/linux/tee_drv.h
+++ b/include/linux/tee_drv.h
@@ -47,6 +47,9 @@ struct tee_shm_pool;
  *              non-blocking in nature.
  * @cap_memref_null: flag indicating if the TEE Client support shared
  *                   memory buffer with a NULL pointer.
+ * @sys_service: flag set by the TEE Client to indicate that it is part of
+ *		 a system service and that the TEE may use resources reserved
+ *		 for this.
  */
 struct tee_context {
 	struct tee_device *teedev;
@@ -55,6 +58,7 @@ struct tee_context {
 	bool releasing;
 	bool supp_nowait;
 	bool cap_memref_null;
+	bool sys_service;
 };
 
 struct tee_param_memref {
-- 
2.25.1


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

[PATCH 2/2] firmware: arm_scmi: optee: use optee system invocation

[Etienne Carriere]

Changes SCMI optee transport to enable sys_service capability of
its tee context to leverage provisioned system resources in OP-TEE
preventing possible deadlock.

Such deadlock could happen when many Linux clients invoke OP-TEE are
are all suspended waiting for an OP-TEE RPC request access an SCMI
resource through the SCMI OP-TEE PTA service.

Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
---
 drivers/firmware/arm_scmi/optee.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/firmware/arm_scmi/optee.c b/drivers/firmware/arm_scmi/optee.c
index 2a7aeab40e54..91840345e946 100644
--- a/drivers/firmware/arm_scmi/optee.c
+++ b/drivers/firmware/arm_scmi/optee.c
@@ -559,6 +559,9 @@ static int scmi_optee_service_probe(struct device *dev)
 	if (IS_ERR(tee_ctx))
 		return -ENODEV;
 
+	/* SCMI agent can used TEE system service resources */
+	tee_ctx->sys_service = true;
+
 	agent = devm_kzalloc(dev, sizeof(*agent), GFP_KERNEL);
 	if (!agent) {
 		ret = -ENOMEM;
-- 
2.25.1

[PATCH 2/2] firmware: arm_scmi: optee: use optee system invocation

[Etienne Carriere]

Changes SCMI optee transport to enable sys_service capability of
its tee context to leverage provisioned system resources in OP-TEE
preventing possible deadlock.

Such deadlock could happen when many Linux clients invoke OP-TEE are
are all suspended waiting for an OP-TEE RPC request access an SCMI
resource through the SCMI OP-TEE PTA service.

Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
---
 drivers/firmware/arm_scmi/optee.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/firmware/arm_scmi/optee.c b/drivers/firmware/arm_scmi/optee.c
index 2a7aeab40e54..91840345e946 100644
--- a/drivers/firmware/arm_scmi/optee.c
+++ b/drivers/firmware/arm_scmi/optee.c
@@ -559,6 +559,9 @@ static int scmi_optee_service_probe(struct device *dev)
 	if (IS_ERR(tee_ctx))
 		return -ENODEV;
 
+	/* SCMI agent can used TEE system service resources */
+	tee_ctx->sys_service = true;
+
 	agent = devm_kzalloc(dev, sizeof(*agent), GFP_KERNEL);
 	if (!agent) {
 		ret = -ENOMEM;
-- 
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 1/2] tee: system invocation

[Jens Wiklander]

On Mon, Jan 30, 2023 at 10:42 AM Etienne Carriere
<etienne.carriere@linaro.org> wrote:
>
> Adds TEE context flag sys_service to be enabled for invocation contexts
> that should used TEE provisioned system resources. OP-TEE SMC ABI entry
> rely this information to use a dedicated entry function to request
> allocation of a system thread from a dedicated system context pool.
>
> This feature is needed when a TEE invocation cannot afford to wait for
> a free TEE thread when all TEE threads context are used and suspended
> as these may be suspended waiting for a system service, as an SCMI clock
> or voltage regulator, to be enabled. An example is when OP-TEE invokes
> a Linux OS remove service (RPC) to access an eMMC RPMB partition and
> the eMMC device is supplied by an OP-TEE SCMI regulator.
>
> Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
> ---
>  drivers/tee/optee/optee_smc.h | 14 +++++++++++---
>  drivers/tee/optee/smc_abi.c   |  6 +++++-
>  include/linux/tee_drv.h       |  4 ++++
>  3 files changed, 20 insertions(+), 4 deletions(-)

Looks good to me.

Thanks,
Jens

>
> diff --git a/drivers/tee/optee/optee_smc.h b/drivers/tee/optee/optee_smc.h
> index 73b5e7760d10..7c7eedf183c5 100644
> --- a/drivers/tee/optee/optee_smc.h
> +++ b/drivers/tee/optee/optee_smc.h
> @@ -108,7 +108,8 @@ struct optee_smc_call_get_os_revision_result {
>   * Call with struct optee_msg_arg as argument
>   *
>   * When called with OPTEE_SMC_CALL_WITH_RPC_ARG or
> - * OPTEE_SMC_CALL_WITH_REGD_ARG in a0 there is one RPC struct optee_msg_arg
> + * OPTEE_SMC_CALL_WITH_REGD_ARG or OPTEE_SMC_FUNCID_CALL_SYSTEM_WITH_REGD_ARG
> + * in a0 there is one RPC struct optee_msg_arg
>   * following after the first struct optee_msg_arg. The RPC struct
>   * optee_msg_arg has reserved space for the number of RPC parameters as
>   * returned by OPTEE_SMC_EXCHANGE_CAPABILITIES.
> @@ -130,8 +131,8 @@ struct optee_smc_call_get_os_revision_result {
>   * a4-6        Not used
>   * a7  Hypervisor Client ID register
>   *
> - * Call register usage, OPTEE_SMC_CALL_WITH_REGD_ARG:
> - * a0  SMC Function ID, OPTEE_SMC_CALL_WITH_REGD_ARG
> + * Call register usage, OPTEE_SMC_CALL_WITH_REGD_ARG and OPTEE_SMC_FUNCID_CALL_SYSTEM_WITH_REGD_ARG:
> + * a0  SMC Function ID, OPTEE_SMC_CALL_WITH_REGD_ARG or OPTEE_SMC_FUNCID_CALL_SYSTEM_WITH_REGD_ARG
>   * a1  Upper 32 bits of a 64-bit shared memory cookie
>   * a2  Lower 32 bits of a 64-bit shared memory cookie
>   * a3  Offset of the struct optee_msg_arg in the shared memory with the
> @@ -175,6 +176,8 @@ struct optee_smc_call_get_os_revision_result {
>         OPTEE_SMC_STD_CALL_VAL(OPTEE_SMC_FUNCID_CALL_WITH_RPC_ARG)
>  #define OPTEE_SMC_CALL_WITH_REGD_ARG \
>         OPTEE_SMC_STD_CALL_VAL(OPTEE_SMC_FUNCID_CALL_WITH_REGD_ARG)
> +#define OPTEE_SMC_CALL_SYSTEM_WITH_REGD_ARG \
> +       OPTEE_SMC_STD_CALL_VAL(OPTEE_SMC_FUNCID_CALL_SYSTEM_WITH_REGD_ARG)
>
>  /*
>   * Get Shared Memory Config
> @@ -254,6 +257,8 @@ struct optee_smc_get_shm_config_result {
>  #define OPTEE_SMC_SEC_CAP_ASYNC_NOTIF          BIT(5)
>  /* Secure world supports pre-allocating RPC arg struct */
>  #define OPTEE_SMC_SEC_CAP_RPC_ARG              BIT(6)
> +/* Secure world provisions thread for system service invocation */
> +#define OPTEE_SMC_SEC_CAP_SYSTEM_THREAD                BIT(7)
>
>  #define OPTEE_SMC_FUNCID_EXCHANGE_CAPABILITIES 9
>  #define OPTEE_SMC_EXCHANGE_CAPABILITIES \
> @@ -426,6 +431,9 @@ struct optee_smc_disable_shm_cache_result {
>  /* See OPTEE_SMC_CALL_WITH_REGD_ARG above */
>  #define OPTEE_SMC_FUNCID_CALL_WITH_REGD_ARG    19
>
> +/* See OPTEE_SMC_CALL_SYSTEM_WITH_REGD_ARG above */
> +#define OPTEE_SMC_FUNCID_CALL_SYSTEM_WITH_REGD_ARG     20
> +
>  /*
>   * Resume from RPC (for example after processing a foreign interrupt)
>   *
> diff --git a/drivers/tee/optee/smc_abi.c b/drivers/tee/optee/smc_abi.c
> index a1c1fa1a9c28..513038a138f6 100644
> --- a/drivers/tee/optee/smc_abi.c
> +++ b/drivers/tee/optee/smc_abi.c
> @@ -889,7 +889,11 @@ static int optee_smc_do_call_with_arg(struct tee_context *ctx,
>         }
>
>         if  (rpc_arg && tee_shm_is_dynamic(shm)) {
> -               param.a0 = OPTEE_SMC_CALL_WITH_REGD_ARG;
> +               if (ctx->sys_service &&
> +                   (optee->smc.sec_caps & OPTEE_SMC_SEC_CAP_SYSTEM_THREAD))
> +                       param.a0 = OPTEE_SMC_CALL_SYSTEM_WITH_REGD_ARG;
> +               else
> +                       param.a0 = OPTEE_SMC_CALL_WITH_REGD_ARG;
>                 reg_pair_from_64(&param.a1, &param.a2, (u_long)shm);
>                 param.a3 = offs;
>         } else {
> diff --git a/include/linux/tee_drv.h b/include/linux/tee_drv.h
> index 17eb1c5205d3..1ff292ba7679 100644
> --- a/include/linux/tee_drv.h
> +++ b/include/linux/tee_drv.h
> @@ -47,6 +47,9 @@ struct tee_shm_pool;
>   *              non-blocking in nature.
>   * @cap_memref_null: flag indicating if the TEE Client support shared
>   *                   memory buffer with a NULL pointer.
> + * @sys_service: flag set by the TEE Client to indicate that it is part of
> + *              a system service and that the TEE may use resources reserved
> + *              for this.
>   */
>  struct tee_context {
>         struct tee_device *teedev;
> @@ -55,6 +58,7 @@ struct tee_context {
>         bool releasing;
>         bool supp_nowait;
>         bool cap_memref_null;
> +       bool sys_service;
>  };
>
>  struct tee_param_memref {
> --
> 2.25.1
>

Re: [PATCH 1/2] tee: system invocation

[Jens Wiklander]

On Mon, Jan 30, 2023 at 10:42 AM Etienne Carriere
<etienne.carriere@linaro.org> wrote:
>
> Adds TEE context flag sys_service to be enabled for invocation contexts
> that should used TEE provisioned system resources. OP-TEE SMC ABI entry
> rely this information to use a dedicated entry function to request
> allocation of a system thread from a dedicated system context pool.
>
> This feature is needed when a TEE invocation cannot afford to wait for
> a free TEE thread when all TEE threads context are used and suspended
> as these may be suspended waiting for a system service, as an SCMI clock
> or voltage regulator, to be enabled. An example is when OP-TEE invokes
> a Linux OS remove service (RPC) to access an eMMC RPMB partition and
> the eMMC device is supplied by an OP-TEE SCMI regulator.
>
> Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
> ---
>  drivers/tee/optee/optee_smc.h | 14 +++++++++++---
>  drivers/tee/optee/smc_abi.c   |  6 +++++-
>  include/linux/tee_drv.h       |  4 ++++
>  3 files changed, 20 insertions(+), 4 deletions(-)

Looks good to me.

Thanks,
Jens

>
> diff --git a/drivers/tee/optee/optee_smc.h b/drivers/tee/optee/optee_smc.h
> index 73b5e7760d10..7c7eedf183c5 100644
> --- a/drivers/tee/optee/optee_smc.h
> +++ b/drivers/tee/optee/optee_smc.h
> @@ -108,7 +108,8 @@ struct optee_smc_call_get_os_revision_result {
>   * Call with struct optee_msg_arg as argument
>   *
>   * When called with OPTEE_SMC_CALL_WITH_RPC_ARG or
> - * OPTEE_SMC_CALL_WITH_REGD_ARG in a0 there is one RPC struct optee_msg_arg
> + * OPTEE_SMC_CALL_WITH_REGD_ARG or OPTEE_SMC_FUNCID_CALL_SYSTEM_WITH_REGD_ARG
> + * in a0 there is one RPC struct optee_msg_arg
>   * following after the first struct optee_msg_arg. The RPC struct
>   * optee_msg_arg has reserved space for the number of RPC parameters as
>   * returned by OPTEE_SMC_EXCHANGE_CAPABILITIES.
> @@ -130,8 +131,8 @@ struct optee_smc_call_get_os_revision_result {
>   * a4-6        Not used
>   * a7  Hypervisor Client ID register
>   *
> - * Call register usage, OPTEE_SMC_CALL_WITH_REGD_ARG:
> - * a0  SMC Function ID, OPTEE_SMC_CALL_WITH_REGD_ARG
> + * Call register usage, OPTEE_SMC_CALL_WITH_REGD_ARG and OPTEE_SMC_FUNCID_CALL_SYSTEM_WITH_REGD_ARG:
> + * a0  SMC Function ID, OPTEE_SMC_CALL_WITH_REGD_ARG or OPTEE_SMC_FUNCID_CALL_SYSTEM_WITH_REGD_ARG
>   * a1  Upper 32 bits of a 64-bit shared memory cookie
>   * a2  Lower 32 bits of a 64-bit shared memory cookie
>   * a3  Offset of the struct optee_msg_arg in the shared memory with the
> @@ -175,6 +176,8 @@ struct optee_smc_call_get_os_revision_result {
>         OPTEE_SMC_STD_CALL_VAL(OPTEE_SMC_FUNCID_CALL_WITH_RPC_ARG)
>  #define OPTEE_SMC_CALL_WITH_REGD_ARG \
>         OPTEE_SMC_STD_CALL_VAL(OPTEE_SMC_FUNCID_CALL_WITH_REGD_ARG)
> +#define OPTEE_SMC_CALL_SYSTEM_WITH_REGD_ARG \
> +       OPTEE_SMC_STD_CALL_VAL(OPTEE_SMC_FUNCID_CALL_SYSTEM_WITH_REGD_ARG)
>
>  /*
>   * Get Shared Memory Config
> @@ -254,6 +257,8 @@ struct optee_smc_get_shm_config_result {
>  #define OPTEE_SMC_SEC_CAP_ASYNC_NOTIF          BIT(5)
>  /* Secure world supports pre-allocating RPC arg struct */
>  #define OPTEE_SMC_SEC_CAP_RPC_ARG              BIT(6)
> +/* Secure world provisions thread for system service invocation */
> +#define OPTEE_SMC_SEC_CAP_SYSTEM_THREAD                BIT(7)
>
>  #define OPTEE_SMC_FUNCID_EXCHANGE_CAPABILITIES 9
>  #define OPTEE_SMC_EXCHANGE_CAPABILITIES \
> @@ -426,6 +431,9 @@ struct optee_smc_disable_shm_cache_result {
>  /* See OPTEE_SMC_CALL_WITH_REGD_ARG above */
>  #define OPTEE_SMC_FUNCID_CALL_WITH_REGD_ARG    19
>
> +/* See OPTEE_SMC_CALL_SYSTEM_WITH_REGD_ARG above */
> +#define OPTEE_SMC_FUNCID_CALL_SYSTEM_WITH_REGD_ARG     20
> +
>  /*
>   * Resume from RPC (for example after processing a foreign interrupt)
>   *
> diff --git a/drivers/tee/optee/smc_abi.c b/drivers/tee/optee/smc_abi.c
> index a1c1fa1a9c28..513038a138f6 100644
> --- a/drivers/tee/optee/smc_abi.c
> +++ b/drivers/tee/optee/smc_abi.c
> @@ -889,7 +889,11 @@ static int optee_smc_do_call_with_arg(struct tee_context *ctx,
>         }
>
>         if  (rpc_arg && tee_shm_is_dynamic(shm)) {
> -               param.a0 = OPTEE_SMC_CALL_WITH_REGD_ARG;
> +               if (ctx->sys_service &&
> +                   (optee->smc.sec_caps & OPTEE_SMC_SEC_CAP_SYSTEM_THREAD))
> +                       param.a0 = OPTEE_SMC_CALL_SYSTEM_WITH_REGD_ARG;
> +               else
> +                       param.a0 = OPTEE_SMC_CALL_WITH_REGD_ARG;
>                 reg_pair_from_64(&param.a1, &param.a2, (u_long)shm);
>                 param.a3 = offs;
>         } else {
> diff --git a/include/linux/tee_drv.h b/include/linux/tee_drv.h
> index 17eb1c5205d3..1ff292ba7679 100644
> --- a/include/linux/tee_drv.h
> +++ b/include/linux/tee_drv.h
> @@ -47,6 +47,9 @@ struct tee_shm_pool;
>   *              non-blocking in nature.
>   * @cap_memref_null: flag indicating if the TEE Client support shared
>   *                   memory buffer with a NULL pointer.
> + * @sys_service: flag set by the TEE Client to indicate that it is part of
> + *              a system service and that the TEE may use resources reserved
> + *              for this.
>   */
>  struct tee_context {
>         struct tee_device *teedev;
> @@ -55,6 +58,7 @@ struct tee_context {
>         bool releasing;
>         bool supp_nowait;
>         bool cap_memref_null;
> +       bool sys_service;
>  };
>
>  struct tee_param_memref {
> --
> 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 2/2] firmware: arm_scmi: optee: use optee system invocation

[Cristian Marussi]

On Mon, Jan 30, 2023 at 10:41:57AM +0100, Etienne Carriere wrote:
> Changes SCMI optee transport to enable sys_service capability of
> its tee context to leverage provisioned system resources in OP-TEE
> preventing possible deadlock.
> 
> Such deadlock could happen when many Linux clients invoke OP-TEE are
> are all suspended waiting for an OP-TEE RPC request access an SCMI
> resource through the SCMI OP-TEE PTA service.
> 
> Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
> ---
>  drivers/firmware/arm_scmi/optee.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/firmware/arm_scmi/optee.c b/drivers/firmware/arm_scmi/optee.c
> index 2a7aeab40e54..91840345e946 100644
> --- a/drivers/firmware/arm_scmi/optee.c
> +++ b/drivers/firmware/arm_scmi/optee.c
> @@ -559,6 +559,9 @@ static int scmi_optee_service_probe(struct device *dev)
>  	if (IS_ERR(tee_ctx))
>  		return -ENODEV;
>  
> +	/* SCMI agent can used TEE system service resources */
> +	tee_ctx->sys_service = true;
> +
>  	agent = devm_kzalloc(dev, sizeof(*agent), GFP_KERNEL);
>  	if (!agent) {
>  		ret = -ENOMEM;

LGTM.

I suppose you'll sync-up with Sudeep for how to queue this whole series.

Thanks,
Cristian

Re: [PATCH 2/2] firmware: arm_scmi: optee: use optee system invocation

[Cristian Marussi]

On Mon, Jan 30, 2023 at 10:41:57AM +0100, Etienne Carriere wrote:
> Changes SCMI optee transport to enable sys_service capability of
> its tee context to leverage provisioned system resources in OP-TEE
> preventing possible deadlock.
> 
> Such deadlock could happen when many Linux clients invoke OP-TEE are
> are all suspended waiting for an OP-TEE RPC request access an SCMI
> resource through the SCMI OP-TEE PTA service.
> 
> Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
> ---
>  drivers/firmware/arm_scmi/optee.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/firmware/arm_scmi/optee.c b/drivers/firmware/arm_scmi/optee.c
> index 2a7aeab40e54..91840345e946 100644
> --- a/drivers/firmware/arm_scmi/optee.c
> +++ b/drivers/firmware/arm_scmi/optee.c
> @@ -559,6 +559,9 @@ static int scmi_optee_service_probe(struct device *dev)
>  	if (IS_ERR(tee_ctx))
>  		return -ENODEV;
>  
> +	/* SCMI agent can used TEE system service resources */
> +	tee_ctx->sys_service = true;
> +
>  	agent = devm_kzalloc(dev, sizeof(*agent), GFP_KERNEL);
>  	if (!agent) {
>  		ret = -ENOMEM;

LGTM.

I suppose you'll sync-up with Sudeep for how to queue this whole series.

Thanks,
Cristian

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

Re: [PATCH 2/2] firmware: arm_scmi: optee: use optee system invocation

[Sudeep Holla]

On Fri, Feb 03, 2023 at 02:36:26PM +0000, Cristian Marussi wrote:
> On Mon, Jan 30, 2023 at 10:41:57AM +0100, Etienne Carriere wrote:
> > Changes SCMI optee transport to enable sys_service capability of
> > its tee context to leverage provisioned system resources in OP-TEE
> > preventing possible deadlock.
> > 
> > Such deadlock could happen when many Linux clients invoke OP-TEE are
> > are all suspended waiting for an OP-TEE RPC request access an SCMI
> > resource through the SCMI OP-TEE PTA service.
> > 
> > Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
> > ---
> >  drivers/firmware/arm_scmi/optee.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/firmware/arm_scmi/optee.c b/drivers/firmware/arm_scmi/optee.c
> > index 2a7aeab40e54..91840345e946 100644
> > --- a/drivers/firmware/arm_scmi/optee.c
> > +++ b/drivers/firmware/arm_scmi/optee.c
> > @@ -559,6 +559,9 @@ static int scmi_optee_service_probe(struct device *dev)
> >  	if (IS_ERR(tee_ctx))
> >  		return -ENODEV;
> >  
> > +	/* SCMI agent can used TEE system service resources */
> > +	tee_ctx->sys_service = true;
> > +
> >  	agent = devm_kzalloc(dev, sizeof(*agent), GFP_KERNEL);
> >  	if (!agent) {
> >  		ret = -ENOMEM;
> 
> LGTM.
> 
> I suppose you'll sync-up with Sudeep for how to queue this whole series.
> 

I thought I had responded to this but not. Anyways since TEE changes are
significant than SCMI, you can route it via TEE tree. In that case,

Acked-by: Sudeep Holla <sudeep.holla@arm.com>

Let me know if that was not your plan.

-- 
Regards,
Sudeep

Re: [PATCH 2/2] firmware: arm_scmi: optee: use optee system invocation

[Sudeep Holla]

On Fri, Feb 03, 2023 at 02:36:26PM +0000, Cristian Marussi wrote:
> On Mon, Jan 30, 2023 at 10:41:57AM +0100, Etienne Carriere wrote:
> > Changes SCMI optee transport to enable sys_service capability of
> > its tee context to leverage provisioned system resources in OP-TEE
> > preventing possible deadlock.
> > 
> > Such deadlock could happen when many Linux clients invoke OP-TEE are
> > are all suspended waiting for an OP-TEE RPC request access an SCMI
> > resource through the SCMI OP-TEE PTA service.
> > 
> > Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
> > ---
> >  drivers/firmware/arm_scmi/optee.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/firmware/arm_scmi/optee.c b/drivers/firmware/arm_scmi/optee.c
> > index 2a7aeab40e54..91840345e946 100644
> > --- a/drivers/firmware/arm_scmi/optee.c
> > +++ b/drivers/firmware/arm_scmi/optee.c
> > @@ -559,6 +559,9 @@ static int scmi_optee_service_probe(struct device *dev)
> >  	if (IS_ERR(tee_ctx))
> >  		return -ENODEV;
> >  
> > +	/* SCMI agent can used TEE system service resources */
> > +	tee_ctx->sys_service = true;
> > +
> >  	agent = devm_kzalloc(dev, sizeof(*agent), GFP_KERNEL);
> >  	if (!agent) {
> >  		ret = -ENOMEM;
> 
> LGTM.
> 
> I suppose you'll sync-up with Sudeep for how to queue this whole series.
> 

I thought I had responded to this but not. Anyways since TEE changes are
significant than SCMI, you can route it via TEE tree. In that case,

Acked-by: Sudeep Holla <sudeep.holla@arm.com>

Let me know if that was not your plan.

-- 
Regards,
Sudeep

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

Re: [PATCH 2/2] firmware: arm_scmi: optee: use optee system invocation

[Etienne Carriere]

Hello Sudeep,

On Fri, 3 Feb 2023 at 18:04, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Fri, Feb 03, 2023 at 02:36:26PM +0000, Cristian Marussi wrote:
> > On Mon, Jan 30, 2023 at 10:41:57AM +0100, Etienne Carriere wrote:
> > > Changes SCMI optee transport to enable sys_service capability of
> > > its tee context to leverage provisioned system resources in OP-TEE
> > > preventing possible deadlock.
> > >
> > > Such deadlock could happen when many Linux clients invoke OP-TEE are
> > > are all suspended waiting for an OP-TEE RPC request access an SCMI
> > > resource through the SCMI OP-TEE PTA service.
> > >
> > > Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
> > > ---
> > >  drivers/firmware/arm_scmi/optee.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > >
> > > diff --git a/drivers/firmware/arm_scmi/optee.c b/drivers/firmware/arm_scmi/optee.c
> > > index 2a7aeab40e54..91840345e946 100644
> > > --- a/drivers/firmware/arm_scmi/optee.c
> > > +++ b/drivers/firmware/arm_scmi/optee.c
> > > @@ -559,6 +559,9 @@ static int scmi_optee_service_probe(struct device *dev)
> > >     if (IS_ERR(tee_ctx))
> > >             return -ENODEV;
> > >
> > > +   /* SCMI agent can used TEE system service resources */
> > > +   tee_ctx->sys_service = true;
> > > +
> > >     agent = devm_kzalloc(dev, sizeof(*agent), GFP_KERNEL);
> > >     if (!agent) {
> > >             ret = -ENOMEM;
> >
> > LGTM.
> >
> > I suppose you'll sync-up with Sudeep for how to queue this whole series.
> >
>
> I thought I had responded to this but not. Anyways since TEE changes are
> significant than SCMI, you can route it via TEE tree. In that case,
>
> Acked-by: Sudeep Holla <sudeep.holla@arm.com>
>
> Let me know if that was not your plan.

That's fine. Thanks. I'll ask Jens to apply it next to the optee commit.

Regards,
Etienne

>
> --
> Regards,
> Sudeep

Re: [PATCH 2/2] firmware: arm_scmi: optee: use optee system invocation

[Etienne Carriere]

Hello Sudeep,

On Fri, 3 Feb 2023 at 18:04, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Fri, Feb 03, 2023 at 02:36:26PM +0000, Cristian Marussi wrote:
> > On Mon, Jan 30, 2023 at 10:41:57AM +0100, Etienne Carriere wrote:
> > > Changes SCMI optee transport to enable sys_service capability of
> > > its tee context to leverage provisioned system resources in OP-TEE
> > > preventing possible deadlock.
> > >
> > > Such deadlock could happen when many Linux clients invoke OP-TEE are
> > > are all suspended waiting for an OP-TEE RPC request access an SCMI
> > > resource through the SCMI OP-TEE PTA service.
> > >
> > > Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
> > > ---
> > >  drivers/firmware/arm_scmi/optee.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > >
> > > diff --git a/drivers/firmware/arm_scmi/optee.c b/drivers/firmware/arm_scmi/optee.c
> > > index 2a7aeab40e54..91840345e946 100644
> > > --- a/drivers/firmware/arm_scmi/optee.c
> > > +++ b/drivers/firmware/arm_scmi/optee.c
> > > @@ -559,6 +559,9 @@ static int scmi_optee_service_probe(struct device *dev)
> > >     if (IS_ERR(tee_ctx))
> > >             return -ENODEV;
> > >
> > > +   /* SCMI agent can used TEE system service resources */
> > > +   tee_ctx->sys_service = true;
> > > +
> > >     agent = devm_kzalloc(dev, sizeof(*agent), GFP_KERNEL);
> > >     if (!agent) {
> > >             ret = -ENOMEM;
> >
> > LGTM.
> >
> > I suppose you'll sync-up with Sudeep for how to queue this whole series.
> >
>
> I thought I had responded to this but not. Anyways since TEE changes are
> significant than SCMI, you can route it via TEE tree. In that case,
>
> Acked-by: Sudeep Holla <sudeep.holla@arm.com>
>
> Let me know if that was not your plan.

That's fine. Thanks. I'll ask Jens to apply it next to the optee commit.

Regards,
Etienne

>
> --
> Regards,
> Sudeep

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

Re: [PATCH 1/2] tee: system invocation

[Sumit Garg]

Hi Etienne,

On Mon, 30 Jan 2023 at 15:12, Etienne Carriere
<etienne.carriere@linaro.org> wrote:
>
> Adds TEE context flag sys_service to be enabled for invocation contexts
> that should used TEE provisioned system resources. OP-TEE SMC ABI entry

s/used/use/

> rely this information to use a dedicated entry function to request
> allocation of a system thread from a dedicated system context pool.
>
> This feature is needed when a TEE invocation cannot afford to wait for
> a free TEE thread when all TEE threads context are used and suspended
> as these may be suspended waiting for a system service, as an SCMI clock
> or voltage regulator, to be enabled. An example is when OP-TEE invokes
> a Linux OS remove service (RPC) to access an eMMC RPMB partition and

s/remove/remote/

> the eMMC device is supplied by an OP-TEE SCMI regulator.
>
> Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
> ---
>  drivers/tee/optee/optee_smc.h | 14 +++++++++++---
>  drivers/tee/optee/smc_abi.c   |  6 +++++-
>  include/linux/tee_drv.h       |  4 ++++
>  3 files changed, 20 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/tee/optee/optee_smc.h b/drivers/tee/optee/optee_smc.h
> index 73b5e7760d10..7c7eedf183c5 100644
> --- a/drivers/tee/optee/optee_smc.h
> +++ b/drivers/tee/optee/optee_smc.h
> @@ -108,7 +108,8 @@ struct optee_smc_call_get_os_revision_result {
>   * Call with struct optee_msg_arg as argument
>   *
>   * When called with OPTEE_SMC_CALL_WITH_RPC_ARG or
> - * OPTEE_SMC_CALL_WITH_REGD_ARG in a0 there is one RPC struct optee_msg_arg
> + * OPTEE_SMC_CALL_WITH_REGD_ARG or OPTEE_SMC_FUNCID_CALL_SYSTEM_WITH_REGD_ARG
> + * in a0 there is one RPC struct optee_msg_arg
>   * following after the first struct optee_msg_arg. The RPC struct
>   * optee_msg_arg has reserved space for the number of RPC parameters as
>   * returned by OPTEE_SMC_EXCHANGE_CAPABILITIES.
> @@ -130,8 +131,8 @@ struct optee_smc_call_get_os_revision_result {
>   * a4-6        Not used
>   * a7  Hypervisor Client ID register
>   *
> - * Call register usage, OPTEE_SMC_CALL_WITH_REGD_ARG:
> - * a0  SMC Function ID, OPTEE_SMC_CALL_WITH_REGD_ARG
> + * Call register usage, OPTEE_SMC_CALL_WITH_REGD_ARG and OPTEE_SMC_FUNCID_CALL_SYSTEM_WITH_REGD_ARG:
> + * a0  SMC Function ID, OPTEE_SMC_CALL_WITH_REGD_ARG or OPTEE_SMC_FUNCID_CALL_SYSTEM_WITH_REGD_ARG
>   * a1  Upper 32 bits of a 64-bit shared memory cookie
>   * a2  Lower 32 bits of a 64-bit shared memory cookie
>   * a3  Offset of the struct optee_msg_arg in the shared memory with the
> @@ -175,6 +176,8 @@ struct optee_smc_call_get_os_revision_result {
>         OPTEE_SMC_STD_CALL_VAL(OPTEE_SMC_FUNCID_CALL_WITH_RPC_ARG)
>  #define OPTEE_SMC_CALL_WITH_REGD_ARG \
>         OPTEE_SMC_STD_CALL_VAL(OPTEE_SMC_FUNCID_CALL_WITH_REGD_ARG)
> +#define OPTEE_SMC_CALL_SYSTEM_WITH_REGD_ARG \
> +       OPTEE_SMC_STD_CALL_VAL(OPTEE_SMC_FUNCID_CALL_SYSTEM_WITH_REGD_ARG)
>
>  /*
>   * Get Shared Memory Config
> @@ -254,6 +257,8 @@ struct optee_smc_get_shm_config_result {
>  #define OPTEE_SMC_SEC_CAP_ASYNC_NOTIF          BIT(5)
>  /* Secure world supports pre-allocating RPC arg struct */
>  #define OPTEE_SMC_SEC_CAP_RPC_ARG              BIT(6)
> +/* Secure world provisions thread for system service invocation */
> +#define OPTEE_SMC_SEC_CAP_SYSTEM_THREAD                BIT(7)
>
>  #define OPTEE_SMC_FUNCID_EXCHANGE_CAPABILITIES 9
>  #define OPTEE_SMC_EXCHANGE_CAPABILITIES \
> @@ -426,6 +431,9 @@ struct optee_smc_disable_shm_cache_result {
>  /* See OPTEE_SMC_CALL_WITH_REGD_ARG above */
>  #define OPTEE_SMC_FUNCID_CALL_WITH_REGD_ARG    19
>
> +/* See OPTEE_SMC_CALL_SYSTEM_WITH_REGD_ARG above */
> +#define OPTEE_SMC_FUNCID_CALL_SYSTEM_WITH_REGD_ARG     20
> +
>  /*
>   * Resume from RPC (for example after processing a foreign interrupt)
>   *
> diff --git a/drivers/tee/optee/smc_abi.c b/drivers/tee/optee/smc_abi.c
> index a1c1fa1a9c28..513038a138f6 100644
> --- a/drivers/tee/optee/smc_abi.c
> +++ b/drivers/tee/optee/smc_abi.c
> @@ -889,7 +889,11 @@ static int optee_smc_do_call_with_arg(struct tee_context *ctx,
>         }
>
>         if  (rpc_arg && tee_shm_is_dynamic(shm)) {
> -               param.a0 = OPTEE_SMC_CALL_WITH_REGD_ARG;
> +               if (ctx->sys_service &&
> +                   (optee->smc.sec_caps & OPTEE_SMC_SEC_CAP_SYSTEM_THREAD))
> +                       param.a0 = OPTEE_SMC_CALL_SYSTEM_WITH_REGD_ARG;
> +               else
> +                       param.a0 = OPTEE_SMC_CALL_WITH_REGD_ARG;

This system thread flag should also be applicable to platforms without
registered arguments support. IOW, we need similar equivalents for
OPTEE_SMC_FUNCID_CALL_WITH_ARG and OPTEE_SMC_FUNCID_CALL_WITH_RPC_ARG
too. So I would rather suggest that we add following flag to all 3
call types:

#define OPTEE_SMC_CALL_SYSTEM_THREAD_FLAG    0x8000

-Sumit

>                 reg_pair_from_64(&param.a1, &param.a2, (u_long)shm);
>                 param.a3 = offs;
>         } else {
> diff --git a/include/linux/tee_drv.h b/include/linux/tee_drv.h
> index 17eb1c5205d3..1ff292ba7679 100644
> --- a/include/linux/tee_drv.h
> +++ b/include/linux/tee_drv.h
> @@ -47,6 +47,9 @@ struct tee_shm_pool;
>   *              non-blocking in nature.
>   * @cap_memref_null: flag indicating if the TEE Client support shared
>   *                   memory buffer with a NULL pointer.
> + * @sys_service: flag set by the TEE Client to indicate that it is part of
> + *              a system service and that the TEE may use resources reserved
> + *              for this.
>   */
>  struct tee_context {
>         struct tee_device *teedev;
> @@ -55,6 +58,7 @@ struct tee_context {
>         bool releasing;
>         bool supp_nowait;
>         bool cap_memref_null;
> +       bool sys_service;
>  };
>
>  struct tee_param_memref {
> --
> 2.25.1
>

Re: [PATCH 1/2] tee: system invocation

[Sumit Garg]

Hi Etienne,

On Mon, 30 Jan 2023 at 15:12, Etienne Carriere
<etienne.carriere@linaro.org> wrote:
>
> Adds TEE context flag sys_service to be enabled for invocation contexts
> that should used TEE provisioned system resources. OP-TEE SMC ABI entry

s/used/use/

> rely this information to use a dedicated entry function to request
> allocation of a system thread from a dedicated system context pool.
>
> This feature is needed when a TEE invocation cannot afford to wait for
> a free TEE thread when all TEE threads context are used and suspended
> as these may be suspended waiting for a system service, as an SCMI clock
> or voltage regulator, to be enabled. An example is when OP-TEE invokes
> a Linux OS remove service (RPC) to access an eMMC RPMB partition and

s/remove/remote/

> the eMMC device is supplied by an OP-TEE SCMI regulator.
>
> Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
> ---
>  drivers/tee/optee/optee_smc.h | 14 +++++++++++---
>  drivers/tee/optee/smc_abi.c   |  6 +++++-
>  include/linux/tee_drv.h       |  4 ++++
>  3 files changed, 20 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/tee/optee/optee_smc.h b/drivers/tee/optee/optee_smc.h
> index 73b5e7760d10..7c7eedf183c5 100644
> --- a/drivers/tee/optee/optee_smc.h
> +++ b/drivers/tee/optee/optee_smc.h
> @@ -108,7 +108,8 @@ struct optee_smc_call_get_os_revision_result {
>   * Call with struct optee_msg_arg as argument
>   *
>   * When called with OPTEE_SMC_CALL_WITH_RPC_ARG or
> - * OPTEE_SMC_CALL_WITH_REGD_ARG in a0 there is one RPC struct optee_msg_arg
> + * OPTEE_SMC_CALL_WITH_REGD_ARG or OPTEE_SMC_FUNCID_CALL_SYSTEM_WITH_REGD_ARG
> + * in a0 there is one RPC struct optee_msg_arg
>   * following after the first struct optee_msg_arg. The RPC struct
>   * optee_msg_arg has reserved space for the number of RPC parameters as
>   * returned by OPTEE_SMC_EXCHANGE_CAPABILITIES.
> @@ -130,8 +131,8 @@ struct optee_smc_call_get_os_revision_result {
>   * a4-6        Not used
>   * a7  Hypervisor Client ID register
>   *
> - * Call register usage, OPTEE_SMC_CALL_WITH_REGD_ARG:
> - * a0  SMC Function ID, OPTEE_SMC_CALL_WITH_REGD_ARG
> + * Call register usage, OPTEE_SMC_CALL_WITH_REGD_ARG and OPTEE_SMC_FUNCID_CALL_SYSTEM_WITH_REGD_ARG:
> + * a0  SMC Function ID, OPTEE_SMC_CALL_WITH_REGD_ARG or OPTEE_SMC_FUNCID_CALL_SYSTEM_WITH_REGD_ARG
>   * a1  Upper 32 bits of a 64-bit shared memory cookie
>   * a2  Lower 32 bits of a 64-bit shared memory cookie
>   * a3  Offset of the struct optee_msg_arg in the shared memory with the
> @@ -175,6 +176,8 @@ struct optee_smc_call_get_os_revision_result {
>         OPTEE_SMC_STD_CALL_VAL(OPTEE_SMC_FUNCID_CALL_WITH_RPC_ARG)
>  #define OPTEE_SMC_CALL_WITH_REGD_ARG \
>         OPTEE_SMC_STD_CALL_VAL(OPTEE_SMC_FUNCID_CALL_WITH_REGD_ARG)
> +#define OPTEE_SMC_CALL_SYSTEM_WITH_REGD_ARG \
> +       OPTEE_SMC_STD_CALL_VAL(OPTEE_SMC_FUNCID_CALL_SYSTEM_WITH_REGD_ARG)
>
>  /*
>   * Get Shared Memory Config
> @@ -254,6 +257,8 @@ struct optee_smc_get_shm_config_result {
>  #define OPTEE_SMC_SEC_CAP_ASYNC_NOTIF          BIT(5)
>  /* Secure world supports pre-allocating RPC arg struct */
>  #define OPTEE_SMC_SEC_CAP_RPC_ARG              BIT(6)
> +/* Secure world provisions thread for system service invocation */
> +#define OPTEE_SMC_SEC_CAP_SYSTEM_THREAD                BIT(7)
>
>  #define OPTEE_SMC_FUNCID_EXCHANGE_CAPABILITIES 9
>  #define OPTEE_SMC_EXCHANGE_CAPABILITIES \
> @@ -426,6 +431,9 @@ struct optee_smc_disable_shm_cache_result {
>  /* See OPTEE_SMC_CALL_WITH_REGD_ARG above */
>  #define OPTEE_SMC_FUNCID_CALL_WITH_REGD_ARG    19
>
> +/* See OPTEE_SMC_CALL_SYSTEM_WITH_REGD_ARG above */
> +#define OPTEE_SMC_FUNCID_CALL_SYSTEM_WITH_REGD_ARG     20
> +
>  /*
>   * Resume from RPC (for example after processing a foreign interrupt)
>   *
> diff --git a/drivers/tee/optee/smc_abi.c b/drivers/tee/optee/smc_abi.c
> index a1c1fa1a9c28..513038a138f6 100644
> --- a/drivers/tee/optee/smc_abi.c
> +++ b/drivers/tee/optee/smc_abi.c
> @@ -889,7 +889,11 @@ static int optee_smc_do_call_with_arg(struct tee_context *ctx,
>         }
>
>         if  (rpc_arg && tee_shm_is_dynamic(shm)) {
> -               param.a0 = OPTEE_SMC_CALL_WITH_REGD_ARG;
> +               if (ctx->sys_service &&
> +                   (optee->smc.sec_caps & OPTEE_SMC_SEC_CAP_SYSTEM_THREAD))
> +                       param.a0 = OPTEE_SMC_CALL_SYSTEM_WITH_REGD_ARG;
> +               else
> +                       param.a0 = OPTEE_SMC_CALL_WITH_REGD_ARG;

This system thread flag should also be applicable to platforms without
registered arguments support. IOW, we need similar equivalents for
OPTEE_SMC_FUNCID_CALL_WITH_ARG and OPTEE_SMC_FUNCID_CALL_WITH_RPC_ARG
too. So I would rather suggest that we add following flag to all 3
call types:

#define OPTEE_SMC_CALL_SYSTEM_THREAD_FLAG    0x8000

-Sumit

>                 reg_pair_from_64(&param.a1, &param.a2, (u_long)shm);
>                 param.a3 = offs;
>         } else {
> diff --git a/include/linux/tee_drv.h b/include/linux/tee_drv.h
> index 17eb1c5205d3..1ff292ba7679 100644
> --- a/include/linux/tee_drv.h
> +++ b/include/linux/tee_drv.h
> @@ -47,6 +47,9 @@ struct tee_shm_pool;
>   *              non-blocking in nature.
>   * @cap_memref_null: flag indicating if the TEE Client support shared
>   *                   memory buffer with a NULL pointer.
> + * @sys_service: flag set by the TEE Client to indicate that it is part of
> + *              a system service and that the TEE may use resources reserved
> + *              for this.
>   */
>  struct tee_context {
>         struct tee_device *teedev;
> @@ -55,6 +58,7 @@ struct tee_context {
>         bool releasing;
>         bool supp_nowait;
>         bool cap_memref_null;
> +       bool sys_service;
>  };
>
>  struct tee_param_memref {
> --
> 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 1/2] tee: system invocation

[Jens Wiklander]

Hi,

On Tue, Feb 7, 2023 at 8:27 AM Sumit Garg <sumit.garg@linaro.org> wrote:
>
> Hi Etienne,
>
> On Mon, 30 Jan 2023 at 15:12, Etienne Carriere
> <etienne.carriere@linaro.org> wrote:
> >
> > Adds TEE context flag sys_service to be enabled for invocation contexts
> > that should used TEE provisioned system resources. OP-TEE SMC ABI entry
>
> s/used/use/
>
> > rely this information to use a dedicated entry function to request
> > allocation of a system thread from a dedicated system context pool.
> >
> > This feature is needed when a TEE invocation cannot afford to wait for
> > a free TEE thread when all TEE threads context are used and suspended
> > as these may be suspended waiting for a system service, as an SCMI clock
> > or voltage regulator, to be enabled. An example is when OP-TEE invokes
> > a Linux OS remove service (RPC) to access an eMMC RPMB partition and
>
> s/remove/remote/
>
> > the eMMC device is supplied by an OP-TEE SCMI regulator.
> >
> > Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
> > ---
> >  drivers/tee/optee/optee_smc.h | 14 +++++++++++---
> >  drivers/tee/optee/smc_abi.c   |  6 +++++-
> >  include/linux/tee_drv.h       |  4 ++++
> >  3 files changed, 20 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/tee/optee/optee_smc.h b/drivers/tee/optee/optee_smc.h
> > index 73b5e7760d10..7c7eedf183c5 100644
> > --- a/drivers/tee/optee/optee_smc.h
> > +++ b/drivers/tee/optee/optee_smc.h
> > @@ -108,7 +108,8 @@ struct optee_smc_call_get_os_revision_result {
> >   * Call with struct optee_msg_arg as argument
> >   *
> >   * When called with OPTEE_SMC_CALL_WITH_RPC_ARG or
> > - * OPTEE_SMC_CALL_WITH_REGD_ARG in a0 there is one RPC struct optee_msg_arg
> > + * OPTEE_SMC_CALL_WITH_REGD_ARG or OPTEE_SMC_FUNCID_CALL_SYSTEM_WITH_REGD_ARG
> > + * in a0 there is one RPC struct optee_msg_arg
> >   * following after the first struct optee_msg_arg. The RPC struct
> >   * optee_msg_arg has reserved space for the number of RPC parameters as
> >   * returned by OPTEE_SMC_EXCHANGE_CAPABILITIES.
> > @@ -130,8 +131,8 @@ struct optee_smc_call_get_os_revision_result {
> >   * a4-6        Not used
> >   * a7  Hypervisor Client ID register
> >   *
> > - * Call register usage, OPTEE_SMC_CALL_WITH_REGD_ARG:
> > - * a0  SMC Function ID, OPTEE_SMC_CALL_WITH_REGD_ARG
> > + * Call register usage, OPTEE_SMC_CALL_WITH_REGD_ARG and OPTEE_SMC_FUNCID_CALL_SYSTEM_WITH_REGD_ARG:
> > + * a0  SMC Function ID, OPTEE_SMC_CALL_WITH_REGD_ARG or OPTEE_SMC_FUNCID_CALL_SYSTEM_WITH_REGD_ARG
> >   * a1  Upper 32 bits of a 64-bit shared memory cookie
> >   * a2  Lower 32 bits of a 64-bit shared memory cookie
> >   * a3  Offset of the struct optee_msg_arg in the shared memory with the
> > @@ -175,6 +176,8 @@ struct optee_smc_call_get_os_revision_result {
> >         OPTEE_SMC_STD_CALL_VAL(OPTEE_SMC_FUNCID_CALL_WITH_RPC_ARG)
> >  #define OPTEE_SMC_CALL_WITH_REGD_ARG \
> >         OPTEE_SMC_STD_CALL_VAL(OPTEE_SMC_FUNCID_CALL_WITH_REGD_ARG)
> > +#define OPTEE_SMC_CALL_SYSTEM_WITH_REGD_ARG \
> > +       OPTEE_SMC_STD_CALL_VAL(OPTEE_SMC_FUNCID_CALL_SYSTEM_WITH_REGD_ARG)
> >
> >  /*
> >   * Get Shared Memory Config
> > @@ -254,6 +257,8 @@ struct optee_smc_get_shm_config_result {
> >  #define OPTEE_SMC_SEC_CAP_ASYNC_NOTIF          BIT(5)
> >  /* Secure world supports pre-allocating RPC arg struct */
> >  #define OPTEE_SMC_SEC_CAP_RPC_ARG              BIT(6)
> > +/* Secure world provisions thread for system service invocation */
> > +#define OPTEE_SMC_SEC_CAP_SYSTEM_THREAD                BIT(7)
> >
> >  #define OPTEE_SMC_FUNCID_EXCHANGE_CAPABILITIES 9
> >  #define OPTEE_SMC_EXCHANGE_CAPABILITIES \
> > @@ -426,6 +431,9 @@ struct optee_smc_disable_shm_cache_result {
> >  /* See OPTEE_SMC_CALL_WITH_REGD_ARG above */
> >  #define OPTEE_SMC_FUNCID_CALL_WITH_REGD_ARG    19
> >
> > +/* See OPTEE_SMC_CALL_SYSTEM_WITH_REGD_ARG above */
> > +#define OPTEE_SMC_FUNCID_CALL_SYSTEM_WITH_REGD_ARG     20
> > +
> >  /*
> >   * Resume from RPC (for example after processing a foreign interrupt)
> >   *
> > diff --git a/drivers/tee/optee/smc_abi.c b/drivers/tee/optee/smc_abi.c
> > index a1c1fa1a9c28..513038a138f6 100644
> > --- a/drivers/tee/optee/smc_abi.c
> > +++ b/drivers/tee/optee/smc_abi.c
> > @@ -889,7 +889,11 @@ static int optee_smc_do_call_with_arg(struct tee_context *ctx,
> >         }
> >
> >         if  (rpc_arg && tee_shm_is_dynamic(shm)) {
> > -               param.a0 = OPTEE_SMC_CALL_WITH_REGD_ARG;
> > +               if (ctx->sys_service &&
> > +                   (optee->smc.sec_caps & OPTEE_SMC_SEC_CAP_SYSTEM_THREAD))
> > +                       param.a0 = OPTEE_SMC_CALL_SYSTEM_WITH_REGD_ARG;
> > +               else
> > +                       param.a0 = OPTEE_SMC_CALL_WITH_REGD_ARG;
>
> This system thread flag should also be applicable to platforms without
> registered arguments support. IOW, we need similar equivalents for
> OPTEE_SMC_FUNCID_CALL_WITH_ARG and OPTEE_SMC_FUNCID_CALL_WITH_RPC_ARG
> too. So I would rather suggest that we add following flag to all 3
> call types:
>
> #define OPTEE_SMC_CALL_SYSTEM_THREAD_FLAG    0x8000

The main reason platforms don't support registered arguments is that
they haven't been updated since this was introduced. So if a platform
needs system threads it could update to use registered arguments too.
The Linux kernel already supports registered arguments. An advantage
with the current approach is that the ABI is easier to implement
since we have distinct SMC IDs for each function.

Cheers,
Jens

>
> -Sumit
>
> >                 reg_pair_from_64(&param.a1, &param.a2, (u_long)shm);
> >                 param.a3 = offs;
> >         } else {
> > diff --git a/include/linux/tee_drv.h b/include/linux/tee_drv.h
> > index 17eb1c5205d3..1ff292ba7679 100644
> > --- a/include/linux/tee_drv.h
> > +++ b/include/linux/tee_drv.h
> > @@ -47,6 +47,9 @@ struct tee_shm_pool;
> >   *              non-blocking in nature.
> >   * @cap_memref_null: flag indicating if the TEE Client support shared
> >   *                   memory buffer with a NULL pointer.
> > + * @sys_service: flag set by the TEE Client to indicate that it is part of
> > + *              a system service and that the TEE may use resources reserved
> > + *              for this.
> >   */
> >  struct tee_context {
> >         struct tee_device *teedev;
> > @@ -55,6 +58,7 @@ struct tee_context {
> >         bool releasing;
> >         bool supp_nowait;
> >         bool cap_memref_null;
> > +       bool sys_service;
> >  };
> >
> >  struct tee_param_memref {
> > --
> > 2.25.1
> >

Re: [PATCH 1/2] tee: system invocation

[Jens Wiklander]

Hi,

On Tue, Feb 7, 2023 at 8:27 AM Sumit Garg <sumit.garg@linaro.org> wrote:
>
> Hi Etienne,
>
> On Mon, 30 Jan 2023 at 15:12, Etienne Carriere
> <etienne.carriere@linaro.org> wrote:
> >
> > Adds TEE context flag sys_service to be enabled for invocation contexts
> > that should used TEE provisioned system resources. OP-TEE SMC ABI entry
>
> s/used/use/
>
> > rely this information to use a dedicated entry function to request
> > allocation of a system thread from a dedicated system context pool.
> >
> > This feature is needed when a TEE invocation cannot afford to wait for
> > a free TEE thread when all TEE threads context are used and suspended
> > as these may be suspended waiting for a system service, as an SCMI clock
> > or voltage regulator, to be enabled. An example is when OP-TEE invokes
> > a Linux OS remove service (RPC) to access an eMMC RPMB partition and
>
> s/remove/remote/
>
> > the eMMC device is supplied by an OP-TEE SCMI regulator.
> >
> > Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
> > ---
> >  drivers/tee/optee/optee_smc.h | 14 +++++++++++---
> >  drivers/tee/optee/smc_abi.c   |  6 +++++-
> >  include/linux/tee_drv.h       |  4 ++++
> >  3 files changed, 20 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/tee/optee/optee_smc.h b/drivers/tee/optee/optee_smc.h
> > index 73b5e7760d10..7c7eedf183c5 100644
> > --- a/drivers/tee/optee/optee_smc.h
> > +++ b/drivers/tee/optee/optee_smc.h
> > @@ -108,7 +108,8 @@ struct optee_smc_call_get_os_revision_result {
> >   * Call with struct optee_msg_arg as argument
> >   *
> >   * When called with OPTEE_SMC_CALL_WITH_RPC_ARG or
> > - * OPTEE_SMC_CALL_WITH_REGD_ARG in a0 there is one RPC struct optee_msg_arg
> > + * OPTEE_SMC_CALL_WITH_REGD_ARG or OPTEE_SMC_FUNCID_CALL_SYSTEM_WITH_REGD_ARG
> > + * in a0 there is one RPC struct optee_msg_arg
> >   * following after the first struct optee_msg_arg. The RPC struct
> >   * optee_msg_arg has reserved space for the number of RPC parameters as
> >   * returned by OPTEE_SMC_EXCHANGE_CAPABILITIES.
> > @@ -130,8 +131,8 @@ struct optee_smc_call_get_os_revision_result {
> >   * a4-6        Not used
> >   * a7  Hypervisor Client ID register
> >   *
> > - * Call register usage, OPTEE_SMC_CALL_WITH_REGD_ARG:
> > - * a0  SMC Function ID, OPTEE_SMC_CALL_WITH_REGD_ARG
> > + * Call register usage, OPTEE_SMC_CALL_WITH_REGD_ARG and OPTEE_SMC_FUNCID_CALL_SYSTEM_WITH_REGD_ARG:
> > + * a0  SMC Function ID, OPTEE_SMC_CALL_WITH_REGD_ARG or OPTEE_SMC_FUNCID_CALL_SYSTEM_WITH_REGD_ARG
> >   * a1  Upper 32 bits of a 64-bit shared memory cookie
> >   * a2  Lower 32 bits of a 64-bit shared memory cookie
> >   * a3  Offset of the struct optee_msg_arg in the shared memory with the
> > @@ -175,6 +176,8 @@ struct optee_smc_call_get_os_revision_result {
> >         OPTEE_SMC_STD_CALL_VAL(OPTEE_SMC_FUNCID_CALL_WITH_RPC_ARG)
> >  #define OPTEE_SMC_CALL_WITH_REGD_ARG \
> >         OPTEE_SMC_STD_CALL_VAL(OPTEE_SMC_FUNCID_CALL_WITH_REGD_ARG)
> > +#define OPTEE_SMC_CALL_SYSTEM_WITH_REGD_ARG \
> > +       OPTEE_SMC_STD_CALL_VAL(OPTEE_SMC_FUNCID_CALL_SYSTEM_WITH_REGD_ARG)
> >
> >  /*
> >   * Get Shared Memory Config
> > @@ -254,6 +257,8 @@ struct optee_smc_get_shm_config_result {
> >  #define OPTEE_SMC_SEC_CAP_ASYNC_NOTIF          BIT(5)
> >  /* Secure world supports pre-allocating RPC arg struct */
> >  #define OPTEE_SMC_SEC_CAP_RPC_ARG              BIT(6)
> > +/* Secure world provisions thread for system service invocation */
> > +#define OPTEE_SMC_SEC_CAP_SYSTEM_THREAD                BIT(7)
> >
> >  #define OPTEE_SMC_FUNCID_EXCHANGE_CAPABILITIES 9
> >  #define OPTEE_SMC_EXCHANGE_CAPABILITIES \
> > @@ -426,6 +431,9 @@ struct optee_smc_disable_shm_cache_result {
> >  /* See OPTEE_SMC_CALL_WITH_REGD_ARG above */
> >  #define OPTEE_SMC_FUNCID_CALL_WITH_REGD_ARG    19
> >
> > +/* See OPTEE_SMC_CALL_SYSTEM_WITH_REGD_ARG above */
> > +#define OPTEE_SMC_FUNCID_CALL_SYSTEM_WITH_REGD_ARG     20
> > +
> >  /*
> >   * Resume from RPC (for example after processing a foreign interrupt)
> >   *
> > diff --git a/drivers/tee/optee/smc_abi.c b/drivers/tee/optee/smc_abi.c
> > index a1c1fa1a9c28..513038a138f6 100644
> > --- a/drivers/tee/optee/smc_abi.c
> > +++ b/drivers/tee/optee/smc_abi.c
> > @@ -889,7 +889,11 @@ static int optee_smc_do_call_with_arg(struct tee_context *ctx,
> >         }
> >
> >         if  (rpc_arg && tee_shm_is_dynamic(shm)) {
> > -               param.a0 = OPTEE_SMC_CALL_WITH_REGD_ARG;
> > +               if (ctx->sys_service &&
> > +                   (optee->smc.sec_caps & OPTEE_SMC_SEC_CAP_SYSTEM_THREAD))
> > +                       param.a0 = OPTEE_SMC_CALL_SYSTEM_WITH_REGD_ARG;
> > +               else
> > +                       param.a0 = OPTEE_SMC_CALL_WITH_REGD_ARG;
>
> This system thread flag should also be applicable to platforms without
> registered arguments support. IOW, we need similar equivalents for
> OPTEE_SMC_FUNCID_CALL_WITH_ARG and OPTEE_SMC_FUNCID_CALL_WITH_RPC_ARG
> too. So I would rather suggest that we add following flag to all 3
> call types:
>
> #define OPTEE_SMC_CALL_SYSTEM_THREAD_FLAG    0x8000

The main reason platforms don't support registered arguments is that
they haven't been updated since this was introduced. So if a platform
needs system threads it could update to use registered arguments too.
The Linux kernel already supports registered arguments. An advantage
with the current approach is that the ABI is easier to implement
since we have distinct SMC IDs for each function.

Cheers,
Jens

>
> -Sumit
>
> >                 reg_pair_from_64(&param.a1, &param.a2, (u_long)shm);
> >                 param.a3 = offs;
> >         } else {
> > diff --git a/include/linux/tee_drv.h b/include/linux/tee_drv.h
> > index 17eb1c5205d3..1ff292ba7679 100644
> > --- a/include/linux/tee_drv.h
> > +++ b/include/linux/tee_drv.h
> > @@ -47,6 +47,9 @@ struct tee_shm_pool;
> >   *              non-blocking in nature.
> >   * @cap_memref_null: flag indicating if the TEE Client support shared
> >   *                   memory buffer with a NULL pointer.
> > + * @sys_service: flag set by the TEE Client to indicate that it is part of
> > + *              a system service and that the TEE may use resources reserved
> > + *              for this.
> >   */
> >  struct tee_context {
> >         struct tee_device *teedev;
> > @@ -55,6 +58,7 @@ struct tee_context {
> >         bool releasing;
> >         bool supp_nowait;
> >         bool cap_memref_null;
> > +       bool sys_service;
> >  };
> >
> >  struct tee_param_memref {
> > --
> > 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 1/2] tee: system invocation

[Sumit Garg]

On Tue, 7 Feb 2023 at 14:38, Jens Wiklander <jens.wiklander@linaro.org> wrote:
>
> Hi,
>
> On Tue, Feb 7, 2023 at 8:27 AM Sumit Garg <sumit.garg@linaro.org> wrote:
> >
> > Hi Etienne,
> >
> > On Mon, 30 Jan 2023 at 15:12, Etienne Carriere
> > <etienne.carriere@linaro.org> wrote:
> > >
> > > Adds TEE context flag sys_service to be enabled for invocation contexts
> > > that should used TEE provisioned system resources. OP-TEE SMC ABI entry
> >
> > s/used/use/
> >
> > > rely this information to use a dedicated entry function to request
> > > allocation of a system thread from a dedicated system context pool.
> > >
> > > This feature is needed when a TEE invocation cannot afford to wait for
> > > a free TEE thread when all TEE threads context are used and suspended
> > > as these may be suspended waiting for a system service, as an SCMI clock
> > > or voltage regulator, to be enabled. An example is when OP-TEE invokes
> > > a Linux OS remove service (RPC) to access an eMMC RPMB partition and
> >
> > s/remove/remote/
> >
> > > the eMMC device is supplied by an OP-TEE SCMI regulator.
> > >
> > > Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
> > > ---
> > >  drivers/tee/optee/optee_smc.h | 14 +++++++++++---
> > >  drivers/tee/optee/smc_abi.c   |  6 +++++-
> > >  include/linux/tee_drv.h       |  4 ++++
> > >  3 files changed, 20 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/tee/optee/optee_smc.h b/drivers/tee/optee/optee_smc.h
> > > index 73b5e7760d10..7c7eedf183c5 100644
> > > --- a/drivers/tee/optee/optee_smc.h
> > > +++ b/drivers/tee/optee/optee_smc.h
> > > @@ -108,7 +108,8 @@ struct optee_smc_call_get_os_revision_result {
> > >   * Call with struct optee_msg_arg as argument
> > >   *
> > >   * When called with OPTEE_SMC_CALL_WITH_RPC_ARG or
> > > - * OPTEE_SMC_CALL_WITH_REGD_ARG in a0 there is one RPC struct optee_msg_arg
> > > + * OPTEE_SMC_CALL_WITH_REGD_ARG or OPTEE_SMC_FUNCID_CALL_SYSTEM_WITH_REGD_ARG
> > > + * in a0 there is one RPC struct optee_msg_arg
> > >   * following after the first struct optee_msg_arg. The RPC struct
> > >   * optee_msg_arg has reserved space for the number of RPC parameters as
> > >   * returned by OPTEE_SMC_EXCHANGE_CAPABILITIES.
> > > @@ -130,8 +131,8 @@ struct optee_smc_call_get_os_revision_result {
> > >   * a4-6        Not used
> > >   * a7  Hypervisor Client ID register
> > >   *
> > > - * Call register usage, OPTEE_SMC_CALL_WITH_REGD_ARG:
> > > - * a0  SMC Function ID, OPTEE_SMC_CALL_WITH_REGD_ARG
> > > + * Call register usage, OPTEE_SMC_CALL_WITH_REGD_ARG and OPTEE_SMC_FUNCID_CALL_SYSTEM_WITH_REGD_ARG:
> > > + * a0  SMC Function ID, OPTEE_SMC_CALL_WITH_REGD_ARG or OPTEE_SMC_FUNCID_CALL_SYSTEM_WITH_REGD_ARG
> > >   * a1  Upper 32 bits of a 64-bit shared memory cookie
> > >   * a2  Lower 32 bits of a 64-bit shared memory cookie
> > >   * a3  Offset of the struct optee_msg_arg in the shared memory with the
> > > @@ -175,6 +176,8 @@ struct optee_smc_call_get_os_revision_result {
> > >         OPTEE_SMC_STD_CALL_VAL(OPTEE_SMC_FUNCID_CALL_WITH_RPC_ARG)
> > >  #define OPTEE_SMC_CALL_WITH_REGD_ARG \
> > >         OPTEE_SMC_STD_CALL_VAL(OPTEE_SMC_FUNCID_CALL_WITH_REGD_ARG)
> > > +#define OPTEE_SMC_CALL_SYSTEM_WITH_REGD_ARG \
> > > +       OPTEE_SMC_STD_CALL_VAL(OPTEE_SMC_FUNCID_CALL_SYSTEM_WITH_REGD_ARG)
> > >
> > >  /*
> > >   * Get Shared Memory Config
> > > @@ -254,6 +257,8 @@ struct optee_smc_get_shm_config_result {
> > >  #define OPTEE_SMC_SEC_CAP_ASYNC_NOTIF          BIT(5)
> > >  /* Secure world supports pre-allocating RPC arg struct */
> > >  #define OPTEE_SMC_SEC_CAP_RPC_ARG              BIT(6)
> > > +/* Secure world provisions thread for system service invocation */
> > > +#define OPTEE_SMC_SEC_CAP_SYSTEM_THREAD                BIT(7)
> > >
> > >  #define OPTEE_SMC_FUNCID_EXCHANGE_CAPABILITIES 9
> > >  #define OPTEE_SMC_EXCHANGE_CAPABILITIES \
> > > @@ -426,6 +431,9 @@ struct optee_smc_disable_shm_cache_result {
> > >  /* See OPTEE_SMC_CALL_WITH_REGD_ARG above */
> > >  #define OPTEE_SMC_FUNCID_CALL_WITH_REGD_ARG    19
> > >
> > > +/* See OPTEE_SMC_CALL_SYSTEM_WITH_REGD_ARG above */
> > > +#define OPTEE_SMC_FUNCID_CALL_SYSTEM_WITH_REGD_ARG     20
> > > +
> > >  /*
> > >   * Resume from RPC (for example after processing a foreign interrupt)
> > >   *
> > > diff --git a/drivers/tee/optee/smc_abi.c b/drivers/tee/optee/smc_abi.c
> > > index a1c1fa1a9c28..513038a138f6 100644
> > > --- a/drivers/tee/optee/smc_abi.c
> > > +++ b/drivers/tee/optee/smc_abi.c
> > > @@ -889,7 +889,11 @@ static int optee_smc_do_call_with_arg(struct tee_context *ctx,
> > >         }
> > >
> > >         if  (rpc_arg && tee_shm_is_dynamic(shm)) {
> > > -               param.a0 = OPTEE_SMC_CALL_WITH_REGD_ARG;
> > > +               if (ctx->sys_service &&
> > > +                   (optee->smc.sec_caps & OPTEE_SMC_SEC_CAP_SYSTEM_THREAD))
> > > +                       param.a0 = OPTEE_SMC_CALL_SYSTEM_WITH_REGD_ARG;
> > > +               else
> > > +                       param.a0 = OPTEE_SMC_CALL_WITH_REGD_ARG;
> >
> > This system thread flag should also be applicable to platforms without
> > registered arguments support. IOW, we need similar equivalents for
> > OPTEE_SMC_FUNCID_CALL_WITH_ARG and OPTEE_SMC_FUNCID_CALL_WITH_RPC_ARG
> > too. So I would rather suggest that we add following flag to all 3
> > call types:
> >
> > #define OPTEE_SMC_CALL_SYSTEM_THREAD_FLAG    0x8000
>
> The main reason platforms don't support registered arguments is that
> they haven't been updated since this was introduced. So if a platform
> needs system threads it could update to use registered arguments too.

Are we hinting at deprecating reserved shared memory support? If yes,
wouldn't it be better to be explicit about it with a boot time warning
message about its deprecation?

Otherwise it will be difficult to debug for the end user to find out
why system thread support isn't activated.

> The Linux kernel already supports registered arguments. An advantage
> with the current approach is that the ABI is easier to implement
> since we have distinct SMC IDs for each function.

I see your point but my initial thought was that we don't end up
making that list too large that it becomes cumbersome to maintain,
involving all the combinatorial.

-Sumit

>
> Cheers,
> Jens
>
> >
> > -Sumit
> >
> > >                 reg_pair_from_64(&param.a1, &param.a2, (u_long)shm);
> > >                 param.a3 = offs;
> > >         } else {
> > > diff --git a/include/linux/tee_drv.h b/include/linux/tee_drv.h
> > > index 17eb1c5205d3..1ff292ba7679 100644
> > > --- a/include/linux/tee_drv.h
> > > +++ b/include/linux/tee_drv.h
> > > @@ -47,6 +47,9 @@ struct tee_shm_pool;
> > >   *              non-blocking in nature.
> > >   * @cap_memref_null: flag indicating if the TEE Client support shared
> > >   *                   memory buffer with a NULL pointer.
> > > + * @sys_service: flag set by the TEE Client to indicate that it is part of
> > > + *              a system service and that the TEE may use resources reserved
> > > + *              for this.
> > >   */
> > >  struct tee_context {
> > >         struct tee_device *teedev;
> > > @@ -55,6 +58,7 @@ struct tee_context {
> > >         bool releasing;
> > >         bool supp_nowait;
> > >         bool cap_memref_null;
> > > +       bool sys_service;
> > >  };
> > >
> > >  struct tee_param_memref {
> > > --
> > > 2.25.1
> > >

Re: [PATCH 1/2] tee: system invocation

[Sumit Garg]

On Tue, 7 Feb 2023 at 14:38, Jens Wiklander <jens.wiklander@linaro.org> wrote:
>
> Hi,
>
> On Tue, Feb 7, 2023 at 8:27 AM Sumit Garg <sumit.garg@linaro.org> wrote:
> >
> > Hi Etienne,
> >
> > On Mon, 30 Jan 2023 at 15:12, Etienne Carriere
> > <etienne.carriere@linaro.org> wrote:
> > >
> > > Adds TEE context flag sys_service to be enabled for invocation contexts
> > > that should used TEE provisioned system resources. OP-TEE SMC ABI entry
> >
> > s/used/use/
> >
> > > rely this information to use a dedicated entry function to request
> > > allocation of a system thread from a dedicated system context pool.
> > >
> > > This feature is needed when a TEE invocation cannot afford to wait for
> > > a free TEE thread when all TEE threads context are used and suspended
> > > as these may be suspended waiting for a system service, as an SCMI clock
> > > or voltage regulator, to be enabled. An example is when OP-TEE invokes
> > > a Linux OS remove service (RPC) to access an eMMC RPMB partition and
> >
> > s/remove/remote/
> >
> > > the eMMC device is supplied by an OP-TEE SCMI regulator.
> > >
> > > Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
> > > ---
> > >  drivers/tee/optee/optee_smc.h | 14 +++++++++++---
> > >  drivers/tee/optee/smc_abi.c   |  6 +++++-
> > >  include/linux/tee_drv.h       |  4 ++++
> > >  3 files changed, 20 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/tee/optee/optee_smc.h b/drivers/tee/optee/optee_smc.h
> > > index 73b5e7760d10..7c7eedf183c5 100644
> > > --- a/drivers/tee/optee/optee_smc.h
> > > +++ b/drivers/tee/optee/optee_smc.h
> > > @@ -108,7 +108,8 @@ struct optee_smc_call_get_os_revision_result {
> > >   * Call with struct optee_msg_arg as argument
> > >   *
> > >   * When called with OPTEE_SMC_CALL_WITH_RPC_ARG or
> > > - * OPTEE_SMC_CALL_WITH_REGD_ARG in a0 there is one RPC struct optee_msg_arg
> > > + * OPTEE_SMC_CALL_WITH_REGD_ARG or OPTEE_SMC_FUNCID_CALL_SYSTEM_WITH_REGD_ARG
> > > + * in a0 there is one RPC struct optee_msg_arg
> > >   * following after the first struct optee_msg_arg. The RPC struct
> > >   * optee_msg_arg has reserved space for the number of RPC parameters as
> > >   * returned by OPTEE_SMC_EXCHANGE_CAPABILITIES.
> > > @@ -130,8 +131,8 @@ struct optee_smc_call_get_os_revision_result {
> > >   * a4-6        Not used
> > >   * a7  Hypervisor Client ID register
> > >   *
> > > - * Call register usage, OPTEE_SMC_CALL_WITH_REGD_ARG:
> > > - * a0  SMC Function ID, OPTEE_SMC_CALL_WITH_REGD_ARG
> > > + * Call register usage, OPTEE_SMC_CALL_WITH_REGD_ARG and OPTEE_SMC_FUNCID_CALL_SYSTEM_WITH_REGD_ARG:
> > > + * a0  SMC Function ID, OPTEE_SMC_CALL_WITH_REGD_ARG or OPTEE_SMC_FUNCID_CALL_SYSTEM_WITH_REGD_ARG
> > >   * a1  Upper 32 bits of a 64-bit shared memory cookie
> > >   * a2  Lower 32 bits of a 64-bit shared memory cookie
> > >   * a3  Offset of the struct optee_msg_arg in the shared memory with the
> > > @@ -175,6 +176,8 @@ struct optee_smc_call_get_os_revision_result {
> > >         OPTEE_SMC_STD_CALL_VAL(OPTEE_SMC_FUNCID_CALL_WITH_RPC_ARG)
> > >  #define OPTEE_SMC_CALL_WITH_REGD_ARG \
> > >         OPTEE_SMC_STD_CALL_VAL(OPTEE_SMC_FUNCID_CALL_WITH_REGD_ARG)
> > > +#define OPTEE_SMC_CALL_SYSTEM_WITH_REGD_ARG \
> > > +       OPTEE_SMC_STD_CALL_VAL(OPTEE_SMC_FUNCID_CALL_SYSTEM_WITH_REGD_ARG)
> > >
> > >  /*
> > >   * Get Shared Memory Config
> > > @@ -254,6 +257,8 @@ struct optee_smc_get_shm_config_result {
> > >  #define OPTEE_SMC_SEC_CAP_ASYNC_NOTIF          BIT(5)
> > >  /* Secure world supports pre-allocating RPC arg struct */
> > >  #define OPTEE_SMC_SEC_CAP_RPC_ARG              BIT(6)
> > > +/* Secure world provisions thread for system service invocation */
> > > +#define OPTEE_SMC_SEC_CAP_SYSTEM_THREAD                BIT(7)
> > >
> > >  #define OPTEE_SMC_FUNCID_EXCHANGE_CAPABILITIES 9
> > >  #define OPTEE_SMC_EXCHANGE_CAPABILITIES \
> > > @@ -426,6 +431,9 @@ struct optee_smc_disable_shm_cache_result {
> > >  /* See OPTEE_SMC_CALL_WITH_REGD_ARG above */
> > >  #define OPTEE_SMC_FUNCID_CALL_WITH_REGD_ARG    19
> > >
> > > +/* See OPTEE_SMC_CALL_SYSTEM_WITH_REGD_ARG above */
> > > +#define OPTEE_SMC_FUNCID_CALL_SYSTEM_WITH_REGD_ARG     20
> > > +
> > >  /*
> > >   * Resume from RPC (for example after processing a foreign interrupt)
> > >   *
> > > diff --git a/drivers/tee/optee/smc_abi.c b/drivers/tee/optee/smc_abi.c
> > > index a1c1fa1a9c28..513038a138f6 100644
> > > --- a/drivers/tee/optee/smc_abi.c
> > > +++ b/drivers/tee/optee/smc_abi.c
> > > @@ -889,7 +889,11 @@ static int optee_smc_do_call_with_arg(struct tee_context *ctx,
> > >         }
> > >
> > >         if  (rpc_arg && tee_shm_is_dynamic(shm)) {
> > > -               param.a0 = OPTEE_SMC_CALL_WITH_REGD_ARG;
> > > +               if (ctx->sys_service &&
> > > +                   (optee->smc.sec_caps & OPTEE_SMC_SEC_CAP_SYSTEM_THREAD))
> > > +                       param.a0 = OPTEE_SMC_CALL_SYSTEM_WITH_REGD_ARG;
> > > +               else
> > > +                       param.a0 = OPTEE_SMC_CALL_WITH_REGD_ARG;
> >
> > This system thread flag should also be applicable to platforms without
> > registered arguments support. IOW, we need similar equivalents for
> > OPTEE_SMC_FUNCID_CALL_WITH_ARG and OPTEE_SMC_FUNCID_CALL_WITH_RPC_ARG
> > too. So I would rather suggest that we add following flag to all 3
> > call types:
> >
> > #define OPTEE_SMC_CALL_SYSTEM_THREAD_FLAG    0x8000
>
> The main reason platforms don't support registered arguments is that
> they haven't been updated since this was introduced. So if a platform
> needs system threads it could update to use registered arguments too.

Are we hinting at deprecating reserved shared memory support? If yes,
wouldn't it be better to be explicit about it with a boot time warning
message about its deprecation?

Otherwise it will be difficult to debug for the end user to find out
why system thread support isn't activated.

> The Linux kernel already supports registered arguments. An advantage
> with the current approach is that the ABI is easier to implement
> since we have distinct SMC IDs for each function.

I see your point but my initial thought was that we don't end up
making that list too large that it becomes cumbersome to maintain,
involving all the combinatorial.

-Sumit

>
> Cheers,
> Jens
>
> >
> > -Sumit
> >
> > >                 reg_pair_from_64(&param.a1, &param.a2, (u_long)shm);
> > >                 param.a3 = offs;
> > >         } else {
> > > diff --git a/include/linux/tee_drv.h b/include/linux/tee_drv.h
> > > index 17eb1c5205d3..1ff292ba7679 100644
> > > --- a/include/linux/tee_drv.h
> > > +++ b/include/linux/tee_drv.h
> > > @@ -47,6 +47,9 @@ struct tee_shm_pool;
> > >   *              non-blocking in nature.
> > >   * @cap_memref_null: flag indicating if the TEE Client support shared
> > >   *                   memory buffer with a NULL pointer.
> > > + * @sys_service: flag set by the TEE Client to indicate that it is part of
> > > + *              a system service and that the TEE may use resources reserved
> > > + *              for this.
> > >   */
> > >  struct tee_context {
> > >         struct tee_device *teedev;
> > > @@ -55,6 +58,7 @@ struct tee_context {
> > >         bool releasing;
> > >         bool supp_nowait;
> > >         bool cap_memref_null;
> > > +       bool sys_service;
> > >  };
> > >
> > >  struct tee_param_memref {
> > > --
> > > 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 2/2] firmware: arm_scmi: optee: use optee system invocation

[Sumit Garg]

On Mon, 30 Jan 2023 at 15:12, Etienne Carriere
<etienne.carriere@linaro.org> wrote:
>
> Changes SCMI optee transport to enable sys_service capability of
> its tee context to leverage provisioned system resources in OP-TEE
> preventing possible deadlock.
>
> Such deadlock could happen when many Linux clients invoke OP-TEE are
> are all suspended waiting for an OP-TEE RPC request access an SCMI
> resource through the SCMI OP-TEE PTA service.
>
> Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
> ---
>  drivers/firmware/arm_scmi/optee.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/firmware/arm_scmi/optee.c b/drivers/firmware/arm_scmi/optee.c
> index 2a7aeab40e54..91840345e946 100644
> --- a/drivers/firmware/arm_scmi/optee.c
> +++ b/drivers/firmware/arm_scmi/optee.c
> @@ -559,6 +559,9 @@ static int scmi_optee_service_probe(struct device *dev)
>         if (IS_ERR(tee_ctx))
>                 return -ENODEV;
>
> +       /* SCMI agent can used TEE system service resources */
> +       tee_ctx->sys_service = true;
> +

As per the other thread for patch #1, this feature will only be
available with OP-TEE supporting TEE_GEN_CAP_REG_MEM. Can we add a
corresponding conditional check here?

-Sumit

>         agent = devm_kzalloc(dev, sizeof(*agent), GFP_KERNEL);
>         if (!agent) {
>                 ret = -ENOMEM;
> --
> 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 2/2] firmware: arm_scmi: optee: use optee system invocation

[Sumit Garg]

On Mon, 30 Jan 2023 at 15:12, Etienne Carriere
<etienne.carriere@linaro.org> wrote:
>
> Changes SCMI optee transport to enable sys_service capability of
> its tee context to leverage provisioned system resources in OP-TEE
> preventing possible deadlock.
>
> Such deadlock could happen when many Linux clients invoke OP-TEE are
> are all suspended waiting for an OP-TEE RPC request access an SCMI
> resource through the SCMI OP-TEE PTA service.
>
> Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
> ---
>  drivers/firmware/arm_scmi/optee.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/firmware/arm_scmi/optee.c b/drivers/firmware/arm_scmi/optee.c
> index 2a7aeab40e54..91840345e946 100644
> --- a/drivers/firmware/arm_scmi/optee.c
> +++ b/drivers/firmware/arm_scmi/optee.c
> @@ -559,6 +559,9 @@ static int scmi_optee_service_probe(struct device *dev)
>         if (IS_ERR(tee_ctx))
>                 return -ENODEV;
>
> +       /* SCMI agent can used TEE system service resources */
> +       tee_ctx->sys_service = true;
> +

As per the other thread for patch #1, this feature will only be
available with OP-TEE supporting TEE_GEN_CAP_REG_MEM. Can we add a
corresponding conditional check here?

-Sumit

>         agent = devm_kzalloc(dev, sizeof(*agent), GFP_KERNEL);
>         if (!agent) {
>                 ret = -ENOMEM;
> --
> 2.25.1
>

Re: [PATCH 1/2] tee: system invocation

[Jens Wiklander]

On Tue, Feb 7, 2023 at 10:52 AM Sumit Garg <sumit.garg@linaro.org> wrote:
>
> On Tue, 7 Feb 2023 at 14:38, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> >
> > Hi,
> >
> > On Tue, Feb 7, 2023 at 8:27 AM Sumit Garg <sumit.garg@linaro.org> wrote:
> > >
> > > Hi Etienne,
> > >
> > > On Mon, 30 Jan 2023 at 15:12, Etienne Carriere
> > > <etienne.carriere@linaro.org> wrote:
> > > >
> > > > Adds TEE context flag sys_service to be enabled for invocation contexts
> > > > that should used TEE provisioned system resources. OP-TEE SMC ABI entry
> > >
> > > s/used/use/
> > >
> > > > rely this information to use a dedicated entry function to request
> > > > allocation of a system thread from a dedicated system context pool.
> > > >
> > > > This feature is needed when a TEE invocation cannot afford to wait for
> > > > a free TEE thread when all TEE threads context are used and suspended
> > > > as these may be suspended waiting for a system service, as an SCMI clock
> > > > or voltage regulator, to be enabled. An example is when OP-TEE invokes
> > > > a Linux OS remove service (RPC) to access an eMMC RPMB partition and
> > >
> > > s/remove/remote/
> > >
> > > > the eMMC device is supplied by an OP-TEE SCMI regulator.
> > > >
> > > > Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
> > > > ---
> > > >  drivers/tee/optee/optee_smc.h | 14 +++++++++++---
> > > >  drivers/tee/optee/smc_abi.c   |  6 +++++-
> > > >  include/linux/tee_drv.h       |  4 ++++
> > > >  3 files changed, 20 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/drivers/tee/optee/optee_smc.h b/drivers/tee/optee/optee_smc.h
> > > > index 73b5e7760d10..7c7eedf183c5 100644
> > > > --- a/drivers/tee/optee/optee_smc.h
> > > > +++ b/drivers/tee/optee/optee_smc.h
> > > > @@ -108,7 +108,8 @@ struct optee_smc_call_get_os_revision_result {
> > > >   * Call with struct optee_msg_arg as argument
> > > >   *
> > > >   * When called with OPTEE_SMC_CALL_WITH_RPC_ARG or
> > > > - * OPTEE_SMC_CALL_WITH_REGD_ARG in a0 there is one RPC struct optee_msg_arg
> > > > + * OPTEE_SMC_CALL_WITH_REGD_ARG or OPTEE_SMC_FUNCID_CALL_SYSTEM_WITH_REGD_ARG
> > > > + * in a0 there is one RPC struct optee_msg_arg
> > > >   * following after the first struct optee_msg_arg. The RPC struct
> > > >   * optee_msg_arg has reserved space for the number of RPC parameters as
> > > >   * returned by OPTEE_SMC_EXCHANGE_CAPABILITIES.
> > > > @@ -130,8 +131,8 @@ struct optee_smc_call_get_os_revision_result {
> > > >   * a4-6        Not used
> > > >   * a7  Hypervisor Client ID register
> > > >   *
> > > > - * Call register usage, OPTEE_SMC_CALL_WITH_REGD_ARG:
> > > > - * a0  SMC Function ID, OPTEE_SMC_CALL_WITH_REGD_ARG
> > > > + * Call register usage, OPTEE_SMC_CALL_WITH_REGD_ARG and OPTEE_SMC_FUNCID_CALL_SYSTEM_WITH_REGD_ARG:
> > > > + * a0  SMC Function ID, OPTEE_SMC_CALL_WITH_REGD_ARG or OPTEE_SMC_FUNCID_CALL_SYSTEM_WITH_REGD_ARG
> > > >   * a1  Upper 32 bits of a 64-bit shared memory cookie
> > > >   * a2  Lower 32 bits of a 64-bit shared memory cookie
> > > >   * a3  Offset of the struct optee_msg_arg in the shared memory with the
> > > > @@ -175,6 +176,8 @@ struct optee_smc_call_get_os_revision_result {
> > > >         OPTEE_SMC_STD_CALL_VAL(OPTEE_SMC_FUNCID_CALL_WITH_RPC_ARG)
> > > >  #define OPTEE_SMC_CALL_WITH_REGD_ARG \
> > > >         OPTEE_SMC_STD_CALL_VAL(OPTEE_SMC_FUNCID_CALL_WITH_REGD_ARG)
> > > > +#define OPTEE_SMC_CALL_SYSTEM_WITH_REGD_ARG \
> > > > +       OPTEE_SMC_STD_CALL_VAL(OPTEE_SMC_FUNCID_CALL_SYSTEM_WITH_REGD_ARG)
> > > >
> > > >  /*
> > > >   * Get Shared Memory Config
> > > > @@ -254,6 +257,8 @@ struct optee_smc_get_shm_config_result {
> > > >  #define OPTEE_SMC_SEC_CAP_ASYNC_NOTIF          BIT(5)
> > > >  /* Secure world supports pre-allocating RPC arg struct */
> > > >  #define OPTEE_SMC_SEC_CAP_RPC_ARG              BIT(6)
> > > > +/* Secure world provisions thread for system service invocation */
> > > > +#define OPTEE_SMC_SEC_CAP_SYSTEM_THREAD                BIT(7)
> > > >
> > > >  #define OPTEE_SMC_FUNCID_EXCHANGE_CAPABILITIES 9
> > > >  #define OPTEE_SMC_EXCHANGE_CAPABILITIES \
> > > > @@ -426,6 +431,9 @@ struct optee_smc_disable_shm_cache_result {
> > > >  /* See OPTEE_SMC_CALL_WITH_REGD_ARG above */
> > > >  #define OPTEE_SMC_FUNCID_CALL_WITH_REGD_ARG    19
> > > >
> > > > +/* See OPTEE_SMC_CALL_SYSTEM_WITH_REGD_ARG above */
> > > > +#define OPTEE_SMC_FUNCID_CALL_SYSTEM_WITH_REGD_ARG     20
> > > > +
> > > >  /*
> > > >   * Resume from RPC (for example after processing a foreign interrupt)
> > > >   *
> > > > diff --git a/drivers/tee/optee/smc_abi.c b/drivers/tee/optee/smc_abi.c
> > > > index a1c1fa1a9c28..513038a138f6 100644
> > > > --- a/drivers/tee/optee/smc_abi.c
> > > > +++ b/drivers/tee/optee/smc_abi.c
> > > > @@ -889,7 +889,11 @@ static int optee_smc_do_call_with_arg(struct tee_context *ctx,
> > > >         }
> > > >
> > > >         if  (rpc_arg && tee_shm_is_dynamic(shm)) {
> > > > -               param.a0 = OPTEE_SMC_CALL_WITH_REGD_ARG;
> > > > +               if (ctx->sys_service &&
> > > > +                   (optee->smc.sec_caps & OPTEE_SMC_SEC_CAP_SYSTEM_THREAD))
> > > > +                       param.a0 = OPTEE_SMC_CALL_SYSTEM_WITH_REGD_ARG;
> > > > +               else
> > > > +                       param.a0 = OPTEE_SMC_CALL_WITH_REGD_ARG;
> > >
> > > This system thread flag should also be applicable to platforms without
> > > registered arguments support. IOW, we need similar equivalents for
> > > OPTEE_SMC_FUNCID_CALL_WITH_ARG and OPTEE_SMC_FUNCID_CALL_WITH_RPC_ARG
> > > too. So I would rather suggest that we add following flag to all 3
> > > call types:
> > >
> > > #define OPTEE_SMC_CALL_SYSTEM_THREAD_FLAG    0x8000
> >
> > The main reason platforms don't support registered arguments is that
> > they haven't been updated since this was introduced. So if a platform
> > needs system threads it could update to use registered arguments too.
>
> Are we hinting at deprecating reserved shared memory support? If yes,
> wouldn't it be better to be explicit about it with a boot time warning
> message about its deprecation?
>
> Otherwise it will be difficult to debug for the end user to find out
> why system thread support isn't activated.
>
> > The Linux kernel already supports registered arguments. An advantage
> > with the current approach is that the ABI is easier to implement
> > since we have distinct SMC IDs for each function.
>
> I see your point but my initial thought was that we don't end up
> making that list too large that it becomes cumbersome to maintain,
> involving all the combinatorial.

You have a point. Etienne, do you think we could give it a try at
https://github.com/OP-TEE/optee_os/pull/5789 to better see how this
would play out?

Cheers,
Jens

Re: [PATCH 1/2] tee: system invocation

[Jens Wiklander]

On Tue, Feb 7, 2023 at 10:52 AM Sumit Garg <sumit.garg@linaro.org> wrote:
>
> On Tue, 7 Feb 2023 at 14:38, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> >
> > Hi,
> >
> > On Tue, Feb 7, 2023 at 8:27 AM Sumit Garg <sumit.garg@linaro.org> wrote:
> > >
> > > Hi Etienne,
> > >
> > > On Mon, 30 Jan 2023 at 15:12, Etienne Carriere
> > > <etienne.carriere@linaro.org> wrote:
> > > >
> > > > Adds TEE context flag sys_service to be enabled for invocation contexts
> > > > that should used TEE provisioned system resources. OP-TEE SMC ABI entry
> > >
> > > s/used/use/
> > >
> > > > rely this information to use a dedicated entry function to request
> > > > allocation of a system thread from a dedicated system context pool.
> > > >
> > > > This feature is needed when a TEE invocation cannot afford to wait for
> > > > a free TEE thread when all TEE threads context are used and suspended
> > > > as these may be suspended waiting for a system service, as an SCMI clock
> > > > or voltage regulator, to be enabled. An example is when OP-TEE invokes
> > > > a Linux OS remove service (RPC) to access an eMMC RPMB partition and
> > >
> > > s/remove/remote/
> > >
> > > > the eMMC device is supplied by an OP-TEE SCMI regulator.
> > > >
> > > > Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
> > > > ---
> > > >  drivers/tee/optee/optee_smc.h | 14 +++++++++++---
> > > >  drivers/tee/optee/smc_abi.c   |  6 +++++-
> > > >  include/linux/tee_drv.h       |  4 ++++
> > > >  3 files changed, 20 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/drivers/tee/optee/optee_smc.h b/drivers/tee/optee/optee_smc.h
> > > > index 73b5e7760d10..7c7eedf183c5 100644
> > > > --- a/drivers/tee/optee/optee_smc.h
> > > > +++ b/drivers/tee/optee/optee_smc.h
> > > > @@ -108,7 +108,8 @@ struct optee_smc_call_get_os_revision_result {
> > > >   * Call with struct optee_msg_arg as argument
> > > >   *
> > > >   * When called with OPTEE_SMC_CALL_WITH_RPC_ARG or
> > > > - * OPTEE_SMC_CALL_WITH_REGD_ARG in a0 there is one RPC struct optee_msg_arg
> > > > + * OPTEE_SMC_CALL_WITH_REGD_ARG or OPTEE_SMC_FUNCID_CALL_SYSTEM_WITH_REGD_ARG
> > > > + * in a0 there is one RPC struct optee_msg_arg
> > > >   * following after the first struct optee_msg_arg. The RPC struct
> > > >   * optee_msg_arg has reserved space for the number of RPC parameters as
> > > >   * returned by OPTEE_SMC_EXCHANGE_CAPABILITIES.
> > > > @@ -130,8 +131,8 @@ struct optee_smc_call_get_os_revision_result {
> > > >   * a4-6        Not used
> > > >   * a7  Hypervisor Client ID register
> > > >   *
> > > > - * Call register usage, OPTEE_SMC_CALL_WITH_REGD_ARG:
> > > > - * a0  SMC Function ID, OPTEE_SMC_CALL_WITH_REGD_ARG
> > > > + * Call register usage, OPTEE_SMC_CALL_WITH_REGD_ARG and OPTEE_SMC_FUNCID_CALL_SYSTEM_WITH_REGD_ARG:
> > > > + * a0  SMC Function ID, OPTEE_SMC_CALL_WITH_REGD_ARG or OPTEE_SMC_FUNCID_CALL_SYSTEM_WITH_REGD_ARG
> > > >   * a1  Upper 32 bits of a 64-bit shared memory cookie
> > > >   * a2  Lower 32 bits of a 64-bit shared memory cookie
> > > >   * a3  Offset of the struct optee_msg_arg in the shared memory with the
> > > > @@ -175,6 +176,8 @@ struct optee_smc_call_get_os_revision_result {
> > > >         OPTEE_SMC_STD_CALL_VAL(OPTEE_SMC_FUNCID_CALL_WITH_RPC_ARG)
> > > >  #define OPTEE_SMC_CALL_WITH_REGD_ARG \
> > > >         OPTEE_SMC_STD_CALL_VAL(OPTEE_SMC_FUNCID_CALL_WITH_REGD_ARG)
> > > > +#define OPTEE_SMC_CALL_SYSTEM_WITH_REGD_ARG \
> > > > +       OPTEE_SMC_STD_CALL_VAL(OPTEE_SMC_FUNCID_CALL_SYSTEM_WITH_REGD_ARG)
> > > >
> > > >  /*
> > > >   * Get Shared Memory Config
> > > > @@ -254,6 +257,8 @@ struct optee_smc_get_shm_config_result {
> > > >  #define OPTEE_SMC_SEC_CAP_ASYNC_NOTIF          BIT(5)
> > > >  /* Secure world supports pre-allocating RPC arg struct */
> > > >  #define OPTEE_SMC_SEC_CAP_RPC_ARG              BIT(6)
> > > > +/* Secure world provisions thread for system service invocation */
> > > > +#define OPTEE_SMC_SEC_CAP_SYSTEM_THREAD                BIT(7)
> > > >
> > > >  #define OPTEE_SMC_FUNCID_EXCHANGE_CAPABILITIES 9
> > > >  #define OPTEE_SMC_EXCHANGE_CAPABILITIES \
> > > > @@ -426,6 +431,9 @@ struct optee_smc_disable_shm_cache_result {
> > > >  /* See OPTEE_SMC_CALL_WITH_REGD_ARG above */
> > > >  #define OPTEE_SMC_FUNCID_CALL_WITH_REGD_ARG    19
> > > >
> > > > +/* See OPTEE_SMC_CALL_SYSTEM_WITH_REGD_ARG above */
> > > > +#define OPTEE_SMC_FUNCID_CALL_SYSTEM_WITH_REGD_ARG     20
> > > > +
> > > >  /*
> > > >   * Resume from RPC (for example after processing a foreign interrupt)
> > > >   *
> > > > diff --git a/drivers/tee/optee/smc_abi.c b/drivers/tee/optee/smc_abi.c
> > > > index a1c1fa1a9c28..513038a138f6 100644
> > > > --- a/drivers/tee/optee/smc_abi.c
> > > > +++ b/drivers/tee/optee/smc_abi.c
> > > > @@ -889,7 +889,11 @@ static int optee_smc_do_call_with_arg(struct tee_context *ctx,
> > > >         }
> > > >
> > > >         if  (rpc_arg && tee_shm_is_dynamic(shm)) {
> > > > -               param.a0 = OPTEE_SMC_CALL_WITH_REGD_ARG;
> > > > +               if (ctx->sys_service &&
> > > > +                   (optee->smc.sec_caps & OPTEE_SMC_SEC_CAP_SYSTEM_THREAD))
> > > > +                       param.a0 = OPTEE_SMC_CALL_SYSTEM_WITH_REGD_ARG;
> > > > +               else
> > > > +                       param.a0 = OPTEE_SMC_CALL_WITH_REGD_ARG;
> > >
> > > This system thread flag should also be applicable to platforms without
> > > registered arguments support. IOW, we need similar equivalents for
> > > OPTEE_SMC_FUNCID_CALL_WITH_ARG and OPTEE_SMC_FUNCID_CALL_WITH_RPC_ARG
> > > too. So I would rather suggest that we add following flag to all 3
> > > call types:
> > >
> > > #define OPTEE_SMC_CALL_SYSTEM_THREAD_FLAG    0x8000
> >
> > The main reason platforms don't support registered arguments is that
> > they haven't been updated since this was introduced. So if a platform
> > needs system threads it could update to use registered arguments too.
>
> Are we hinting at deprecating reserved shared memory support? If yes,
> wouldn't it be better to be explicit about it with a boot time warning
> message about its deprecation?
>
> Otherwise it will be difficult to debug for the end user to find out
> why system thread support isn't activated.
>
> > The Linux kernel already supports registered arguments. An advantage
> > with the current approach is that the ABI is easier to implement
> > since we have distinct SMC IDs for each function.
>
> I see your point but my initial thought was that we don't end up
> making that list too large that it becomes cumbersome to maintain,
> involving all the combinatorial.

You have a point. Etienne, do you think we could give it a try at
https://github.com/OP-TEE/optee_os/pull/5789 to better see how this
would play out?

Cheers,
Jens

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

Re: [PATCH 2/2] firmware: arm_scmi: optee: use optee system invocation

[Jens Wiklander]

On Tue, Feb 7, 2023 at 10:59 AM Sumit Garg <sumit.garg@linaro.org> wrote:
>
> On Mon, 30 Jan 2023 at 15:12, Etienne Carriere
> <etienne.carriere@linaro.org> wrote:
> >
> > Changes SCMI optee transport to enable sys_service capability of
> > its tee context to leverage provisioned system resources in OP-TEE
> > preventing possible deadlock.
> >
> > Such deadlock could happen when many Linux clients invoke OP-TEE are
> > are all suspended waiting for an OP-TEE RPC request access an SCMI
> > resource through the SCMI OP-TEE PTA service.
> >
> > Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
> > ---
> >  drivers/firmware/arm_scmi/optee.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/firmware/arm_scmi/optee.c b/drivers/firmware/arm_scmi/optee.c
> > index 2a7aeab40e54..91840345e946 100644
> > --- a/drivers/firmware/arm_scmi/optee.c
> > +++ b/drivers/firmware/arm_scmi/optee.c
> > @@ -559,6 +559,9 @@ static int scmi_optee_service_probe(struct device *dev)
> >         if (IS_ERR(tee_ctx))
> >                 return -ENODEV;
> >
> > +       /* SCMI agent can used TEE system service resources */
> > +       tee_ctx->sys_service = true;
> > +
>
> As per the other thread for patch #1, this feature will only be
> available with OP-TEE supporting TEE_GEN_CAP_REG_MEM. Can we add a
> corresponding conditional check here?

What would be gained by that? If a system thread isn't available or
already is busy we're supposed to fall back to normal threads.

Cheers,
Jens

>
> -Sumit
>
> >         agent = devm_kzalloc(dev, sizeof(*agent), GFP_KERNEL);
> >         if (!agent) {
> >                 ret = -ENOMEM;
> > --
> > 2.25.1
> >

Re: [PATCH 2/2] firmware: arm_scmi: optee: use optee system invocation

[Jens Wiklander]

On Tue, Feb 7, 2023 at 10:59 AM Sumit Garg <sumit.garg@linaro.org> wrote:
>
> On Mon, 30 Jan 2023 at 15:12, Etienne Carriere
> <etienne.carriere@linaro.org> wrote:
> >
> > Changes SCMI optee transport to enable sys_service capability of
> > its tee context to leverage provisioned system resources in OP-TEE
> > preventing possible deadlock.
> >
> > Such deadlock could happen when many Linux clients invoke OP-TEE are
> > are all suspended waiting for an OP-TEE RPC request access an SCMI
> > resource through the SCMI OP-TEE PTA service.
> >
> > Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
> > ---
> >  drivers/firmware/arm_scmi/optee.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/firmware/arm_scmi/optee.c b/drivers/firmware/arm_scmi/optee.c
> > index 2a7aeab40e54..91840345e946 100644
> > --- a/drivers/firmware/arm_scmi/optee.c
> > +++ b/drivers/firmware/arm_scmi/optee.c
> > @@ -559,6 +559,9 @@ static int scmi_optee_service_probe(struct device *dev)
> >         if (IS_ERR(tee_ctx))
> >                 return -ENODEV;
> >
> > +       /* SCMI agent can used TEE system service resources */
> > +       tee_ctx->sys_service = true;
> > +
>
> As per the other thread for patch #1, this feature will only be
> available with OP-TEE supporting TEE_GEN_CAP_REG_MEM. Can we add a
> corresponding conditional check here?

What would be gained by that? If a system thread isn't available or
already is busy we're supposed to fall back to normal threads.

Cheers,
Jens

>
> -Sumit
>
> >         agent = devm_kzalloc(dev, sizeof(*agent), GFP_KERNEL);
> >         if (!agent) {
> >                 ret = -ENOMEM;
> > --
> > 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 2/2] firmware: arm_scmi: optee: use optee system invocation

[Sumit Garg]

On Tue, 7 Feb 2023 at 16:09, Jens Wiklander <jens.wiklander@linaro.org> wrote:
>
> On Tue, Feb 7, 2023 at 10:59 AM Sumit Garg <sumit.garg@linaro.org> wrote:
> >
> > On Mon, 30 Jan 2023 at 15:12, Etienne Carriere
> > <etienne.carriere@linaro.org> wrote:
> > >
> > > Changes SCMI optee transport to enable sys_service capability of
> > > its tee context to leverage provisioned system resources in OP-TEE
> > > preventing possible deadlock.
> > >
> > > Such deadlock could happen when many Linux clients invoke OP-TEE are
> > > are all suspended waiting for an OP-TEE RPC request access an SCMI
> > > resource through the SCMI OP-TEE PTA service.
> > >
> > > Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
> > > ---
> > >  drivers/firmware/arm_scmi/optee.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > >
> > > diff --git a/drivers/firmware/arm_scmi/optee.c b/drivers/firmware/arm_scmi/optee.c
> > > index 2a7aeab40e54..91840345e946 100644
> > > --- a/drivers/firmware/arm_scmi/optee.c
> > > +++ b/drivers/firmware/arm_scmi/optee.c
> > > @@ -559,6 +559,9 @@ static int scmi_optee_service_probe(struct device *dev)
> > >         if (IS_ERR(tee_ctx))
> > >                 return -ENODEV;
> > >
> > > +       /* SCMI agent can used TEE system service resources */
> > > +       tee_ctx->sys_service = true;
> > > +
> >
> > As per the other thread for patch #1, this feature will only be
> > available with OP-TEE supporting TEE_GEN_CAP_REG_MEM. Can we add a
> > corresponding conditional check here?
>
> What would be gained by that? If a system thread isn't available or
> already is busy we're supposed to fall back to normal threads.
>

Just to make the dependency explicit and probably warn users that the
system is missing a required capability. Earlier I went through a
similar dependency issue report for trusted keys driver. I ended with
a dependency fix [1] to make it easier while debugging when something
doesn't work as intended.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/security/keys/trusted-keys/trusted_tee.c#n223

-Sumit

> Cheers,
> Jens
>
> >
> > -Sumit
> >
> > >         agent = devm_kzalloc(dev, sizeof(*agent), GFP_KERNEL);
> > >         if (!agent) {
> > >                 ret = -ENOMEM;
> > > --
> > > 2.25.1
> > >

Re: [PATCH 2/2] firmware: arm_scmi: optee: use optee system invocation

[Sumit Garg]

On Tue, 7 Feb 2023 at 16:09, Jens Wiklander <jens.wiklander@linaro.org> wrote:
>
> On Tue, Feb 7, 2023 at 10:59 AM Sumit Garg <sumit.garg@linaro.org> wrote:
> >
> > On Mon, 30 Jan 2023 at 15:12, Etienne Carriere
> > <etienne.carriere@linaro.org> wrote:
> > >
> > > Changes SCMI optee transport to enable sys_service capability of
> > > its tee context to leverage provisioned system resources in OP-TEE
> > > preventing possible deadlock.
> > >
> > > Such deadlock could happen when many Linux clients invoke OP-TEE are
> > > are all suspended waiting for an OP-TEE RPC request access an SCMI
> > > resource through the SCMI OP-TEE PTA service.
> > >
> > > Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
> > > ---
> > >  drivers/firmware/arm_scmi/optee.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > >
> > > diff --git a/drivers/firmware/arm_scmi/optee.c b/drivers/firmware/arm_scmi/optee.c
> > > index 2a7aeab40e54..91840345e946 100644
> > > --- a/drivers/firmware/arm_scmi/optee.c
> > > +++ b/drivers/firmware/arm_scmi/optee.c
> > > @@ -559,6 +559,9 @@ static int scmi_optee_service_probe(struct device *dev)
> > >         if (IS_ERR(tee_ctx))
> > >                 return -ENODEV;
> > >
> > > +       /* SCMI agent can used TEE system service resources */
> > > +       tee_ctx->sys_service = true;
> > > +
> >
> > As per the other thread for patch #1, this feature will only be
> > available with OP-TEE supporting TEE_GEN_CAP_REG_MEM. Can we add a
> > corresponding conditional check here?
>
> What would be gained by that? If a system thread isn't available or
> already is busy we're supposed to fall back to normal threads.
>

Just to make the dependency explicit and probably warn users that the
system is missing a required capability. Earlier I went through a
similar dependency issue report for trusted keys driver. I ended with
a dependency fix [1] to make it easier while debugging when something
doesn't work as intended.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/security/keys/trusted-keys/trusted_tee.c#n223

-Sumit

> Cheers,
> Jens
>
> >
> > -Sumit
> >
> > >         agent = devm_kzalloc(dev, sizeof(*agent), GFP_KERNEL);
> > >         if (!agent) {
> > >                 ret = -ENOMEM;
> > > --
> > > 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 1/2] tee: system invocation

[Etienne Carriere]

Hello Sumit, Jens,

On Tue, 7 Feb 2023 at 11:36, Jens Wiklander <jens.wiklander@linaro.org> wrote:
>
> On Tue, Feb 7, 2023 at 10:52 AM Sumit Garg <sumit.garg@linaro.org> wrote:
> >
> > On Tue, 7 Feb 2023 at 14:38, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> > >
> > > Hi,
> > >
> > > On Tue, Feb 7, 2023 at 8:27 AM Sumit Garg <sumit.garg@linaro.org> wrote:
> > > >
> > > > Hi Etienne,
> > > >
> > > > On Mon, 30 Jan 2023 at 15:12, Etienne Carriere
> > > > <etienne.carriere@linaro.org> wrote:
> > > > >
> > > > > Adds TEE context flag sys_service to be enabled for invocation contexts
> > > > > that should used TEE provisioned system resources. OP-TEE SMC ABI entry
> > > >
> > > > s/used/use/
> > > >
> > > > > rely this information to use a dedicated entry function to request
> > > > > allocation of a system thread from a dedicated system context pool.
> > > > >
> > > > > This feature is needed when a TEE invocation cannot afford to wait for
> > > > > a free TEE thread when all TEE threads context are used and suspended
> > > > > as these may be suspended waiting for a system service, as an SCMI clock
> > > > > or voltage regulator, to be enabled. An example is when OP-TEE invokes
> > > > > a Linux OS remove service (RPC) to access an eMMC RPMB partition and
> > > >
> > > > s/remove/remote/
> > > >
> > > > > the eMMC device is supplied by an OP-TEE SCMI regulator.
> > > > >
> > > > > Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
> > > > > ---
> > > > >  drivers/tee/optee/optee_smc.h | 14 +++++++++++---
> > > > >  drivers/tee/optee/smc_abi.c   |  6 +++++-
> > > > >  include/linux/tee_drv.h       |  4 ++++
> > > > >  3 files changed, 20 insertions(+), 4 deletions(-)
> > > > >
> > > > > diff --git a/drivers/tee/optee/optee_smc.h b/drivers/tee/optee/optee_smc.h
> > > > > index 73b5e7760d10..7c7eedf183c5 100644
> > > > > --- a/drivers/tee/optee/optee_smc.h
> > > > > +++ b/drivers/tee/optee/optee_smc.h
> > > > > @@ -108,7 +108,8 @@ struct optee_smc_call_get_os_revision_result {
> > > > >   * Call with struct optee_msg_arg as argument
> > > > >   *
> > > > >   * When called with OPTEE_SMC_CALL_WITH_RPC_ARG or
> > > > > - * OPTEE_SMC_CALL_WITH_REGD_ARG in a0 there is one RPC struct optee_msg_arg
> > > > > + * OPTEE_SMC_CALL_WITH_REGD_ARG or OPTEE_SMC_FUNCID_CALL_SYSTEM_WITH_REGD_ARG
> > > > > + * in a0 there is one RPC struct optee_msg_arg
> > > > >   * following after the first struct optee_msg_arg. The RPC struct
> > > > >   * optee_msg_arg has reserved space for the number of RPC parameters as
> > > > >   * returned by OPTEE_SMC_EXCHANGE_CAPABILITIES.
> > > > > @@ -130,8 +131,8 @@ struct optee_smc_call_get_os_revision_result {
> > > > >   * a4-6        Not used
> > > > >   * a7  Hypervisor Client ID register
> > > > >   *
> > > > > - * Call register usage, OPTEE_SMC_CALL_WITH_REGD_ARG:
> > > > > - * a0  SMC Function ID, OPTEE_SMC_CALL_WITH_REGD_ARG
> > > > > + * Call register usage, OPTEE_SMC_CALL_WITH_REGD_ARG and OPTEE_SMC_FUNCID_CALL_SYSTEM_WITH_REGD_ARG:
> > > > > + * a0  SMC Function ID, OPTEE_SMC_CALL_WITH_REGD_ARG or OPTEE_SMC_FUNCID_CALL_SYSTEM_WITH_REGD_ARG
> > > > >   * a1  Upper 32 bits of a 64-bit shared memory cookie
> > > > >   * a2  Lower 32 bits of a 64-bit shared memory cookie
> > > > >   * a3  Offset of the struct optee_msg_arg in the shared memory with the
> > > > > @@ -175,6 +176,8 @@ struct optee_smc_call_get_os_revision_result {
> > > > >         OPTEE_SMC_STD_CALL_VAL(OPTEE_SMC_FUNCID_CALL_WITH_RPC_ARG)
> > > > >  #define OPTEE_SMC_CALL_WITH_REGD_ARG \
> > > > >         OPTEE_SMC_STD_CALL_VAL(OPTEE_SMC_FUNCID_CALL_WITH_REGD_ARG)
> > > > > +#define OPTEE_SMC_CALL_SYSTEM_WITH_REGD_ARG \
> > > > > +       OPTEE_SMC_STD_CALL_VAL(OPTEE_SMC_FUNCID_CALL_SYSTEM_WITH_REGD_ARG)
> > > > >
> > > > >  /*
> > > > >   * Get Shared Memory Config
> > > > > @@ -254,6 +257,8 @@ struct optee_smc_get_shm_config_result {
> > > > >  #define OPTEE_SMC_SEC_CAP_ASYNC_NOTIF          BIT(5)
> > > > >  /* Secure world supports pre-allocating RPC arg struct */
> > > > >  #define OPTEE_SMC_SEC_CAP_RPC_ARG              BIT(6)
> > > > > +/* Secure world provisions thread for system service invocation */
> > > > > +#define OPTEE_SMC_SEC_CAP_SYSTEM_THREAD                BIT(7)
> > > > >
> > > > >  #define OPTEE_SMC_FUNCID_EXCHANGE_CAPABILITIES 9
> > > > >  #define OPTEE_SMC_EXCHANGE_CAPABILITIES \
> > > > > @@ -426,6 +431,9 @@ struct optee_smc_disable_shm_cache_result {
> > > > >  /* See OPTEE_SMC_CALL_WITH_REGD_ARG above */
> > > > >  #define OPTEE_SMC_FUNCID_CALL_WITH_REGD_ARG    19
> > > > >
> > > > > +/* See OPTEE_SMC_CALL_SYSTEM_WITH_REGD_ARG above */
> > > > > +#define OPTEE_SMC_FUNCID_CALL_SYSTEM_WITH_REGD_ARG     20
> > > > > +
> > > > >  /*
> > > > >   * Resume from RPC (for example after processing a foreign interrupt)
> > > > >   *
> > > > > diff --git a/drivers/tee/optee/smc_abi.c b/drivers/tee/optee/smc_abi.c
> > > > > index a1c1fa1a9c28..513038a138f6 100644
> > > > > --- a/drivers/tee/optee/smc_abi.c
> > > > > +++ b/drivers/tee/optee/smc_abi.c
> > > > > @@ -889,7 +889,11 @@ static int optee_smc_do_call_with_arg(struct tee_context *ctx,
> > > > >         }
> > > > >
> > > > >         if  (rpc_arg && tee_shm_is_dynamic(shm)) {
> > > > > -               param.a0 = OPTEE_SMC_CALL_WITH_REGD_ARG;
> > > > > +               if (ctx->sys_service &&
> > > > > +                   (optee->smc.sec_caps & OPTEE_SMC_SEC_CAP_SYSTEM_THREAD))
> > > > > +                       param.a0 = OPTEE_SMC_CALL_SYSTEM_WITH_REGD_ARG;
> > > > > +               else
> > > > > +                       param.a0 = OPTEE_SMC_CALL_WITH_REGD_ARG;
> > > >
> > > > This system thread flag should also be applicable to platforms without
> > > > registered arguments support. IOW, we need similar equivalents for
> > > > OPTEE_SMC_FUNCID_CALL_WITH_ARG and OPTEE_SMC_FUNCID_CALL_WITH_RPC_ARG
> > > > too. So I would rather suggest that we add following flag to all 3
> > > > call types:
> > > >
> > > > #define OPTEE_SMC_CALL_SYSTEM_THREAD_FLAG    0x8000
> > >
> > > The main reason platforms don't support registered arguments is that
> > > they haven't been updated since this was introduced. So if a platform
> > > needs system threads it could update to use registered arguments too.
> >
> > Are we hinting at deprecating reserved shared memory support? If yes,
> > wouldn't it be better to be explicit about it with a boot time warning
> > message about its deprecation?
> >
> > Otherwise it will be difficult to debug for the end user to find out
> > why system thread support isn't activated.
> >
> > > The Linux kernel already supports registered arguments. An advantage
> > > with the current approach is that the ABI is easier to implement
> > > since we have distinct SMC IDs for each function.
> >
> > I see your point but my initial thought was that we don't end up
> > making that list too large that it becomes cumbersome to maintain,
> > involving all the combinatorial.
>
> You have a point. Etienne, do you think we could give it a try at
> https://github.com/OP-TEE/optee_os/pull/5789 to better see how this
> would play out?
>

Indeed I miss that...
With the patch proposed here, indeed if OP-TEE does not support
dynamic shared memory then Linux will never use the provisioned TEE
thread. This is weird as in such a case OP-TEE provisions resources
that will never be used, which is the exact opposite goal of this
feature. Verified on our qemu-arm setup.

For simplicity, I think this system invocation should require OP-TEE
supports dyn shm.

If OP-TEE could know when Linux does not support TEE system
invocation, then OP-TEE could let any invocation use these provisioned
resources so that they are not wasted.
I think a good way would be Linux to expose if it supports this
capability, during capabilities exchange.
Would you agree with this approach?

Etienne



> Cheers,
> Jens

Re: [PATCH 1/2] tee: system invocation

[Etienne Carriere]

Hello Sumit, Jens,

On Tue, 7 Feb 2023 at 11:36, Jens Wiklander <jens.wiklander@linaro.org> wrote:
>
> On Tue, Feb 7, 2023 at 10:52 AM Sumit Garg <sumit.garg@linaro.org> wrote:
> >
> > On Tue, 7 Feb 2023 at 14:38, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> > >
> > > Hi,
> > >
> > > On Tue, Feb 7, 2023 at 8:27 AM Sumit Garg <sumit.garg@linaro.org> wrote:
> > > >
> > > > Hi Etienne,
> > > >
> > > > On Mon, 30 Jan 2023 at 15:12, Etienne Carriere
> > > > <etienne.carriere@linaro.org> wrote:
> > > > >
> > > > > Adds TEE context flag sys_service to be enabled for invocation contexts
> > > > > that should used TEE provisioned system resources. OP-TEE SMC ABI entry
> > > >
> > > > s/used/use/
> > > >
> > > > > rely this information to use a dedicated entry function to request
> > > > > allocation of a system thread from a dedicated system context pool.
> > > > >
> > > > > This feature is needed when a TEE invocation cannot afford to wait for
> > > > > a free TEE thread when all TEE threads context are used and suspended
> > > > > as these may be suspended waiting for a system service, as an SCMI clock
> > > > > or voltage regulator, to be enabled. An example is when OP-TEE invokes
> > > > > a Linux OS remove service (RPC) to access an eMMC RPMB partition and
> > > >
> > > > s/remove/remote/
> > > >
> > > > > the eMMC device is supplied by an OP-TEE SCMI regulator.
> > > > >
> > > > > Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
> > > > > ---
> > > > >  drivers/tee/optee/optee_smc.h | 14 +++++++++++---
> > > > >  drivers/tee/optee/smc_abi.c   |  6 +++++-
> > > > >  include/linux/tee_drv.h       |  4 ++++
> > > > >  3 files changed, 20 insertions(+), 4 deletions(-)
> > > > >
> > > > > diff --git a/drivers/tee/optee/optee_smc.h b/drivers/tee/optee/optee_smc.h
> > > > > index 73b5e7760d10..7c7eedf183c5 100644
> > > > > --- a/drivers/tee/optee/optee_smc.h
> > > > > +++ b/drivers/tee/optee/optee_smc.h
> > > > > @@ -108,7 +108,8 @@ struct optee_smc_call_get_os_revision_result {
> > > > >   * Call with struct optee_msg_arg as argument
> > > > >   *
> > > > >   * When called with OPTEE_SMC_CALL_WITH_RPC_ARG or
> > > > > - * OPTEE_SMC_CALL_WITH_REGD_ARG in a0 there is one RPC struct optee_msg_arg
> > > > > + * OPTEE_SMC_CALL_WITH_REGD_ARG or OPTEE_SMC_FUNCID_CALL_SYSTEM_WITH_REGD_ARG
> > > > > + * in a0 there is one RPC struct optee_msg_arg
> > > > >   * following after the first struct optee_msg_arg. The RPC struct
> > > > >   * optee_msg_arg has reserved space for the number of RPC parameters as
> > > > >   * returned by OPTEE_SMC_EXCHANGE_CAPABILITIES.
> > > > > @@ -130,8 +131,8 @@ struct optee_smc_call_get_os_revision_result {
> > > > >   * a4-6        Not used
> > > > >   * a7  Hypervisor Client ID register
> > > > >   *
> > > > > - * Call register usage, OPTEE_SMC_CALL_WITH_REGD_ARG:
> > > > > - * a0  SMC Function ID, OPTEE_SMC_CALL_WITH_REGD_ARG
> > > > > + * Call register usage, OPTEE_SMC_CALL_WITH_REGD_ARG and OPTEE_SMC_FUNCID_CALL_SYSTEM_WITH_REGD_ARG:
> > > > > + * a0  SMC Function ID, OPTEE_SMC_CALL_WITH_REGD_ARG or OPTEE_SMC_FUNCID_CALL_SYSTEM_WITH_REGD_ARG
> > > > >   * a1  Upper 32 bits of a 64-bit shared memory cookie
> > > > >   * a2  Lower 32 bits of a 64-bit shared memory cookie
> > > > >   * a3  Offset of the struct optee_msg_arg in the shared memory with the
> > > > > @@ -175,6 +176,8 @@ struct optee_smc_call_get_os_revision_result {
> > > > >         OPTEE_SMC_STD_CALL_VAL(OPTEE_SMC_FUNCID_CALL_WITH_RPC_ARG)
> > > > >  #define OPTEE_SMC_CALL_WITH_REGD_ARG \
> > > > >         OPTEE_SMC_STD_CALL_VAL(OPTEE_SMC_FUNCID_CALL_WITH_REGD_ARG)
> > > > > +#define OPTEE_SMC_CALL_SYSTEM_WITH_REGD_ARG \
> > > > > +       OPTEE_SMC_STD_CALL_VAL(OPTEE_SMC_FUNCID_CALL_SYSTEM_WITH_REGD_ARG)
> > > > >
> > > > >  /*
> > > > >   * Get Shared Memory Config
> > > > > @@ -254,6 +257,8 @@ struct optee_smc_get_shm_config_result {
> > > > >  #define OPTEE_SMC_SEC_CAP_ASYNC_NOTIF          BIT(5)
> > > > >  /* Secure world supports pre-allocating RPC arg struct */
> > > > >  #define OPTEE_SMC_SEC_CAP_RPC_ARG              BIT(6)
> > > > > +/* Secure world provisions thread for system service invocation */
> > > > > +#define OPTEE_SMC_SEC_CAP_SYSTEM_THREAD                BIT(7)
> > > > >
> > > > >  #define OPTEE_SMC_FUNCID_EXCHANGE_CAPABILITIES 9
> > > > >  #define OPTEE_SMC_EXCHANGE_CAPABILITIES \
> > > > > @@ -426,6 +431,9 @@ struct optee_smc_disable_shm_cache_result {
> > > > >  /* See OPTEE_SMC_CALL_WITH_REGD_ARG above */
> > > > >  #define OPTEE_SMC_FUNCID_CALL_WITH_REGD_ARG    19
> > > > >
> > > > > +/* See OPTEE_SMC_CALL_SYSTEM_WITH_REGD_ARG above */
> > > > > +#define OPTEE_SMC_FUNCID_CALL_SYSTEM_WITH_REGD_ARG     20
> > > > > +
> > > > >  /*
> > > > >   * Resume from RPC (for example after processing a foreign interrupt)
> > > > >   *
> > > > > diff --git a/drivers/tee/optee/smc_abi.c b/drivers/tee/optee/smc_abi.c
> > > > > index a1c1fa1a9c28..513038a138f6 100644
> > > > > --- a/drivers/tee/optee/smc_abi.c
> > > > > +++ b/drivers/tee/optee/smc_abi.c
> > > > > @@ -889,7 +889,11 @@ static int optee_smc_do_call_with_arg(struct tee_context *ctx,
> > > > >         }
> > > > >
> > > > >         if  (rpc_arg && tee_shm_is_dynamic(shm)) {
> > > > > -               param.a0 = OPTEE_SMC_CALL_WITH_REGD_ARG;
> > > > > +               if (ctx->sys_service &&
> > > > > +                   (optee->smc.sec_caps & OPTEE_SMC_SEC_CAP_SYSTEM_THREAD))
> > > > > +                       param.a0 = OPTEE_SMC_CALL_SYSTEM_WITH_REGD_ARG;
> > > > > +               else
> > > > > +                       param.a0 = OPTEE_SMC_CALL_WITH_REGD_ARG;
> > > >
> > > > This system thread flag should also be applicable to platforms without
> > > > registered arguments support. IOW, we need similar equivalents for
> > > > OPTEE_SMC_FUNCID_CALL_WITH_ARG and OPTEE_SMC_FUNCID_CALL_WITH_RPC_ARG
> > > > too. So I would rather suggest that we add following flag to all 3
> > > > call types:
> > > >
> > > > #define OPTEE_SMC_CALL_SYSTEM_THREAD_FLAG    0x8000
> > >
> > > The main reason platforms don't support registered arguments is that
> > > they haven't been updated since this was introduced. So if a platform
> > > needs system threads it could update to use registered arguments too.
> >
> > Are we hinting at deprecating reserved shared memory support? If yes,
> > wouldn't it be better to be explicit about it with a boot time warning
> > message about its deprecation?
> >
> > Otherwise it will be difficult to debug for the end user to find out
> > why system thread support isn't activated.
> >
> > > The Linux kernel already supports registered arguments. An advantage
> > > with the current approach is that the ABI is easier to implement
> > > since we have distinct SMC IDs for each function.
> >
> > I see your point but my initial thought was that we don't end up
> > making that list too large that it becomes cumbersome to maintain,
> > involving all the combinatorial.
>
> You have a point. Etienne, do you think we could give it a try at
> https://github.com/OP-TEE/optee_os/pull/5789 to better see how this
> would play out?
>

Indeed I miss that...
With the patch proposed here, indeed if OP-TEE does not support
dynamic shared memory then Linux will never use the provisioned TEE
thread. This is weird as in such a case OP-TEE provisions resources
that will never be used, which is the exact opposite goal of this
feature. Verified on our qemu-arm setup.

For simplicity, I think this system invocation should require OP-TEE
supports dyn shm.

If OP-TEE could know when Linux does not support TEE system
invocation, then OP-TEE could let any invocation use these provisioned
resources so that they are not wasted.
I think a good way would be Linux to expose if it supports this
capability, during capabilities exchange.
Would you agree with this approach?

Etienne



> Cheers,
> Jens

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

Re: [PATCH 1/2] tee: system invocation

[Jens Wiklander]

Hi Etienne,

On Wed, Feb 08, 2023 at 06:09:17PM +0100, Etienne Carriere wrote:
> Hello Sumit, Jens,
> 
[snip]
> > > > > >
> > > > > >         if  (rpc_arg && tee_shm_is_dynamic(shm)) {
> > > > > > -               param.a0 = OPTEE_SMC_CALL_WITH_REGD_ARG;
> > > > > > +               if (ctx->sys_service &&
> > > > > > +                   (optee->smc.sec_caps & OPTEE_SMC_SEC_CAP_SYSTEM_THREAD))
> > > > > > +                       param.a0 = OPTEE_SMC_CALL_SYSTEM_WITH_REGD_ARG;
> > > > > > +               else
> > > > > > +                       param.a0 = OPTEE_SMC_CALL_WITH_REGD_ARG;
> > > > >
> > > > > This system thread flag should also be applicable to platforms without
> > > > > registered arguments support. IOW, we need similar equivalents for
> > > > > OPTEE_SMC_FUNCID_CALL_WITH_ARG and OPTEE_SMC_FUNCID_CALL_WITH_RPC_ARG
> > > > > too. So I would rather suggest that we add following flag to all 3
> > > > > call types:
> > > > >
> > > > > #define OPTEE_SMC_CALL_SYSTEM_THREAD_FLAG    0x8000
> > > >
> > > > The main reason platforms don't support registered arguments is that
> > > > they haven't been updated since this was introduced. So if a platform
> > > > needs system threads it could update to use registered arguments too.
> > >
> > > Are we hinting at deprecating reserved shared memory support? If yes,
> > > wouldn't it be better to be explicit about it with a boot time warning
> > > message about its deprecation?
> > >
> > > Otherwise it will be difficult to debug for the end user to find out
> > > why system thread support isn't activated.
> > >
> > > > The Linux kernel already supports registered arguments. An advantage
> > > > with the current approach is that the ABI is easier to implement
> > > > since we have distinct SMC IDs for each function.
> > >
> > > I see your point but my initial thought was that we don't end up
> > > making that list too large that it becomes cumbersome to maintain,
> > > involving all the combinatorial.
> >
> > You have a point. Etienne, do you think we could give it a try at
> > https://github.com/OP-TEE/optee_os/pull/5789 to better see how this
> > would play out?
> >
> 
> Indeed I miss that...
> With the patch proposed here, indeed if OP-TEE does not support
> dynamic shared memory then Linux will never use the provisioned TEE
> thread. This is weird as in such a case OP-TEE provisions resources
> that will never be used, which is the exact opposite goal of this
> feature. Verified on our qemu-arm setup.
> 
> For simplicity, I think this system invocation should require OP-TEE
> supports dyn shm.

It's not obvious to me that this will easier to implement and maintain.
Looking at the code in optee_os it looks like using a flag bit as
proposed by Sumit would be quite easy to handle.

> 
> If OP-TEE could know when Linux does not support TEE system
> invocation, then OP-TEE could let any invocation use these provisioned
> resources so that they are not wasted.
> I think a good way would be Linux to expose if it supports this
> capability, during capabilities exchange.
> Would you agree with this approach?

No, I'm not so keen on adding that side effect to
OPTEE_SMC_EXCHANGE_CAPABILITIES.

The way you're describing the problem it sounds like it's a normal world
problem to know how many system threads are needed. How about adding a
fast call where normal world can request how many system threads should
be reserved?  If none are requested, none will be reserved.

Cheers,
Jens

Re: [PATCH 1/2] tee: system invocation

[Jens Wiklander]

Hi Etienne,

On Wed, Feb 08, 2023 at 06:09:17PM +0100, Etienne Carriere wrote:
> Hello Sumit, Jens,
> 
[snip]
> > > > > >
> > > > > >         if  (rpc_arg && tee_shm_is_dynamic(shm)) {
> > > > > > -               param.a0 = OPTEE_SMC_CALL_WITH_REGD_ARG;
> > > > > > +               if (ctx->sys_service &&
> > > > > > +                   (optee->smc.sec_caps & OPTEE_SMC_SEC_CAP_SYSTEM_THREAD))
> > > > > > +                       param.a0 = OPTEE_SMC_CALL_SYSTEM_WITH_REGD_ARG;
> > > > > > +               else
> > > > > > +                       param.a0 = OPTEE_SMC_CALL_WITH_REGD_ARG;
> > > > >
> > > > > This system thread flag should also be applicable to platforms without
> > > > > registered arguments support. IOW, we need similar equivalents for
> > > > > OPTEE_SMC_FUNCID_CALL_WITH_ARG and OPTEE_SMC_FUNCID_CALL_WITH_RPC_ARG
> > > > > too. So I would rather suggest that we add following flag to all 3
> > > > > call types:
> > > > >
> > > > > #define OPTEE_SMC_CALL_SYSTEM_THREAD_FLAG    0x8000
> > > >
> > > > The main reason platforms don't support registered arguments is that
> > > > they haven't been updated since this was introduced. So if a platform
> > > > needs system threads it could update to use registered arguments too.
> > >
> > > Are we hinting at deprecating reserved shared memory support? If yes,
> > > wouldn't it be better to be explicit about it with a boot time warning
> > > message about its deprecation?
> > >
> > > Otherwise it will be difficult to debug for the end user to find out
> > > why system thread support isn't activated.
> > >
> > > > The Linux kernel already supports registered arguments. An advantage
> > > > with the current approach is that the ABI is easier to implement
> > > > since we have distinct SMC IDs for each function.
> > >
> > > I see your point but my initial thought was that we don't end up
> > > making that list too large that it becomes cumbersome to maintain,
> > > involving all the combinatorial.
> >
> > You have a point. Etienne, do you think we could give it a try at
> > https://github.com/OP-TEE/optee_os/pull/5789 to better see how this
> > would play out?
> >
> 
> Indeed I miss that...
> With the patch proposed here, indeed if OP-TEE does not support
> dynamic shared memory then Linux will never use the provisioned TEE
> thread. This is weird as in such a case OP-TEE provisions resources
> that will never be used, which is the exact opposite goal of this
> feature. Verified on our qemu-arm setup.
> 
> For simplicity, I think this system invocation should require OP-TEE
> supports dyn shm.

It's not obvious to me that this will easier to implement and maintain.
Looking at the code in optee_os it looks like using a flag bit as
proposed by Sumit would be quite easy to handle.

> 
> If OP-TEE could know when Linux does not support TEE system
> invocation, then OP-TEE could let any invocation use these provisioned
> resources so that they are not wasted.
> I think a good way would be Linux to expose if it supports this
> capability, during capabilities exchange.
> Would you agree with this approach?

No, I'm not so keen on adding that side effect to
OPTEE_SMC_EXCHANGE_CAPABILITIES.

The way you're describing the problem it sounds like it's a normal world
problem to know how many system threads are needed. How about adding a
fast call where normal world can request how many system threads should
be reserved?  If none are requested, none will be reserved.

Cheers,
Jens

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

Re: [PATCH 1/2] tee: system invocation

[Etienne Carriere]

Hi Jens,


On Thu, 9 Feb 2023 at 08:14, Jens Wiklander <jens.wiklander@linaro.org> wrote:
>
> Hi Etienne,
>
> On Wed, Feb 08, 2023 at 06:09:17PM +0100, Etienne Carriere wrote:
> > Hello Sumit, Jens,
> >
> [snip]
> > > > > > >
> > > > > > >         if  (rpc_arg && tee_shm_is_dynamic(shm)) {
> > > > > > > -               param.a0 = OPTEE_SMC_CALL_WITH_REGD_ARG;
> > > > > > > +               if (ctx->sys_service &&
> > > > > > > +                   (optee->smc.sec_caps & OPTEE_SMC_SEC_CAP_SYSTEM_THREAD))
> > > > > > > +                       param.a0 = OPTEE_SMC_CALL_SYSTEM_WITH_REGD_ARG;
> > > > > > > +               else
> > > > > > > +                       param.a0 = OPTEE_SMC_CALL_WITH_REGD_ARG;
> > > > > >
> > > > > > This system thread flag should also be applicable to platforms without
> > > > > > registered arguments support. IOW, we need similar equivalents for
> > > > > > OPTEE_SMC_FUNCID_CALL_WITH_ARG and OPTEE_SMC_FUNCID_CALL_WITH_RPC_ARG
> > > > > > too. So I would rather suggest that we add following flag to all 3
> > > > > > call types:
> > > > > >
> > > > > > #define OPTEE_SMC_CALL_SYSTEM_THREAD_FLAG    0x8000
> > > > >
> > > > > The main reason platforms don't support registered arguments is that
> > > > > they haven't been updated since this was introduced. So if a platform
> > > > > needs system threads it could update to use registered arguments too.
> > > >
> > > > Are we hinting at deprecating reserved shared memory support? If yes,
> > > > wouldn't it be better to be explicit about it with a boot time warning
> > > > message about its deprecation?
> > > >
> > > > Otherwise it will be difficult to debug for the end user to find out
> > > > why system thread support isn't activated.
> > > >
> > > > > The Linux kernel already supports registered arguments. An advantage
> > > > > with the current approach is that the ABI is easier to implement
> > > > > since we have distinct SMC IDs for each function.
> > > >
> > > > I see your point but my initial thought was that we don't end up
> > > > making that list too large that it becomes cumbersome to maintain,
> > > > involving all the combinatorial.
> > >
> > > You have a point. Etienne, do you think we could give it a try at
> > > https://github.com/OP-TEE/optee_os/pull/5789 to better see how this
> > > would play out?
> > >
> >
> > Indeed I miss that...
> > With the patch proposed here, indeed if OP-TEE does not support
> > dynamic shared memory then Linux will never use the provisioned TEE
> > thread. This is weird as in such a case OP-TEE provisions resources
> > that will never be used, which is the exact opposite goal of this
> > feature. Verified on our qemu-arm setup.
> >
> > For simplicity, I think this system invocation should require OP-TEE
> > supports dyn shm.
>
> It's not obvious to me that this will easier to implement and maintain.
> Looking at the code in optee_os it looks like using a flag bit as
> proposed by Sumit would be quite easy to handle.

OP-TEE could auto disable thread provis when dyn shm is disabled, right.
Will it be sufficient? We will still face cases where an OP-TEE
provisions thread but Linux kernel is not aware (older vanilla kernel
used with a recent OP-TEE OS). Not much platforms are really affected
I guess but those executing with pager in small RAMs where a 4kB
thread context costs.


>
> >
> > If OP-TEE could know when Linux does not support TEE system
> > invocation, then OP-TEE could let any invocation use these provisioned
> > resources so that they are not wasted.
> > I think a good way would be Linux to expose if it supports this
> > capability, during capabilities exchange.
> > Would you agree with this approach?
>
> No, I'm not so keen on adding that side effect to
> OPTEE_SMC_EXCHANGE_CAPABILITIES.

It is a capability REE would exchanges with TEE.
What kind of side effects do you fear?

>
> The way you're describing the problem it sounds like it's a normal world
> problem to know how many system threads are needed. How about adding a
> fast call where normal world can request how many system threads should
> be reserved?  If none are requested, none will be reserved.

Well, could be. With caps exchange, we have an SMC funcID to REE to
say to TEE: "reserved the default configured number of sys thread". I
think it is simpler.

With REE calling TEE to provision thread, we would need another call
to release the reservation. Whe caps exchange, we have a single SMC to
reconfig the negotiated caps.

>
> Cheers,
> Jens

Re: [PATCH 1/2] tee: system invocation

[Etienne Carriere]

Hi Jens,


On Thu, 9 Feb 2023 at 08:14, Jens Wiklander <jens.wiklander@linaro.org> wrote:
>
> Hi Etienne,
>
> On Wed, Feb 08, 2023 at 06:09:17PM +0100, Etienne Carriere wrote:
> > Hello Sumit, Jens,
> >
> [snip]
> > > > > > >
> > > > > > >         if  (rpc_arg && tee_shm_is_dynamic(shm)) {
> > > > > > > -               param.a0 = OPTEE_SMC_CALL_WITH_REGD_ARG;
> > > > > > > +               if (ctx->sys_service &&
> > > > > > > +                   (optee->smc.sec_caps & OPTEE_SMC_SEC_CAP_SYSTEM_THREAD))
> > > > > > > +                       param.a0 = OPTEE_SMC_CALL_SYSTEM_WITH_REGD_ARG;
> > > > > > > +               else
> > > > > > > +                       param.a0 = OPTEE_SMC_CALL_WITH_REGD_ARG;
> > > > > >
> > > > > > This system thread flag should also be applicable to platforms without
> > > > > > registered arguments support. IOW, we need similar equivalents for
> > > > > > OPTEE_SMC_FUNCID_CALL_WITH_ARG and OPTEE_SMC_FUNCID_CALL_WITH_RPC_ARG
> > > > > > too. So I would rather suggest that we add following flag to all 3
> > > > > > call types:
> > > > > >
> > > > > > #define OPTEE_SMC_CALL_SYSTEM_THREAD_FLAG    0x8000
> > > > >
> > > > > The main reason platforms don't support registered arguments is that
> > > > > they haven't been updated since this was introduced. So if a platform
> > > > > needs system threads it could update to use registered arguments too.
> > > >
> > > > Are we hinting at deprecating reserved shared memory support? If yes,
> > > > wouldn't it be better to be explicit about it with a boot time warning
> > > > message about its deprecation?
> > > >
> > > > Otherwise it will be difficult to debug for the end user to find out
> > > > why system thread support isn't activated.
> > > >
> > > > > The Linux kernel already supports registered arguments. An advantage
> > > > > with the current approach is that the ABI is easier to implement
> > > > > since we have distinct SMC IDs for each function.
> > > >
> > > > I see your point but my initial thought was that we don't end up
> > > > making that list too large that it becomes cumbersome to maintain,
> > > > involving all the combinatorial.
> > >
> > > You have a point. Etienne, do you think we could give it a try at
> > > https://github.com/OP-TEE/optee_os/pull/5789 to better see how this
> > > would play out?
> > >
> >
> > Indeed I miss that...
> > With the patch proposed here, indeed if OP-TEE does not support
> > dynamic shared memory then Linux will never use the provisioned TEE
> > thread. This is weird as in such a case OP-TEE provisions resources
> > that will never be used, which is the exact opposite goal of this
> > feature. Verified on our qemu-arm setup.
> >
> > For simplicity, I think this system invocation should require OP-TEE
> > supports dyn shm.
>
> It's not obvious to me that this will easier to implement and maintain.
> Looking at the code in optee_os it looks like using a flag bit as
> proposed by Sumit would be quite easy to handle.

OP-TEE could auto disable thread provis when dyn shm is disabled, right.
Will it be sufficient? We will still face cases where an OP-TEE
provisions thread but Linux kernel is not aware (older vanilla kernel
used with a recent OP-TEE OS). Not much platforms are really affected
I guess but those executing with pager in small RAMs where a 4kB
thread context costs.


>
> >
> > If OP-TEE could know when Linux does not support TEE system
> > invocation, then OP-TEE could let any invocation use these provisioned
> > resources so that they are not wasted.
> > I think a good way would be Linux to expose if it supports this
> > capability, during capabilities exchange.
> > Would you agree with this approach?
>
> No, I'm not so keen on adding that side effect to
> OPTEE_SMC_EXCHANGE_CAPABILITIES.

It is a capability REE would exchanges with TEE.
What kind of side effects do you fear?

>
> The way you're describing the problem it sounds like it's a normal world
> problem to know how many system threads are needed. How about adding a
> fast call where normal world can request how many system threads should
> be reserved?  If none are requested, none will be reserved.

Well, could be. With caps exchange, we have an SMC funcID to REE to
say to TEE: "reserved the default configured number of sys thread". I
think it is simpler.

With REE calling TEE to provision thread, we would need another call
to release the reservation. Whe caps exchange, we have a single SMC to
reconfig the negotiated caps.

>
> Cheers,
> Jens

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

Re: [PATCH 1/2] tee: system invocation

[Etienne Carriere]

On Thu, 9 Feb 2023 at 09:11, Etienne Carriere
<etienne.carriere@linaro.org> wrote:
>
> Hi Jens,
>
>
> On Thu, 9 Feb 2023 at 08:14, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> >
> > Hi Etienne,
> >
> > On Wed, Feb 08, 2023 at 06:09:17PM +0100, Etienne Carriere wrote:
> > > Hello Sumit, Jens,
> > >
> > [snip]
> > > > > > > >
> > > > > > > >         if  (rpc_arg && tee_shm_is_dynamic(shm)) {
> > > > > > > > -               param.a0 = OPTEE_SMC_CALL_WITH_REGD_ARG;
> > > > > > > > +               if (ctx->sys_service &&
> > > > > > > > +                   (optee->smc.sec_caps & OPTEE_SMC_SEC_CAP_SYSTEM_THREAD))
> > > > > > > > +                       param.a0 = OPTEE_SMC_CALL_SYSTEM_WITH_REGD_ARG;
> > > > > > > > +               else
> > > > > > > > +                       param.a0 = OPTEE_SMC_CALL_WITH_REGD_ARG;
> > > > > > >
> > > > > > > This system thread flag should also be applicable to platforms without
> > > > > > > registered arguments support. IOW, we need similar equivalents for
> > > > > > > OPTEE_SMC_FUNCID_CALL_WITH_ARG and OPTEE_SMC_FUNCID_CALL_WITH_RPC_ARG
> > > > > > > too. So I would rather suggest that we add following flag to all 3
> > > > > > > call types:
> > > > > > >
> > > > > > > #define OPTEE_SMC_CALL_SYSTEM_THREAD_FLAG    0x8000
> > > > > >
> > > > > > The main reason platforms don't support registered arguments is that
> > > > > > they haven't been updated since this was introduced. So if a platform
> > > > > > needs system threads it could update to use registered arguments too.
> > > > >
> > > > > Are we hinting at deprecating reserved shared memory support? If yes,
> > > > > wouldn't it be better to be explicit about it with a boot time warning
> > > > > message about its deprecation?
> > > > >
> > > > > Otherwise it will be difficult to debug for the end user to find out
> > > > > why system thread support isn't activated.
> > > > >
> > > > > > The Linux kernel already supports registered arguments. An advantage
> > > > > > with the current approach is that the ABI is easier to implement
> > > > > > since we have distinct SMC IDs for each function.
> > > > >
> > > > > I see your point but my initial thought was that we don't end up
> > > > > making that list too large that it becomes cumbersome to maintain,
> > > > > involving all the combinatorial.
> > > >
> > > > You have a point. Etienne, do you think we could give it a try at
> > > > https://github.com/OP-TEE/optee_os/pull/5789 to better see how this
> > > > would play out?
> > > >
> > >
> > > Indeed I miss that...
> > > With the patch proposed here, indeed if OP-TEE does not support
> > > dynamic shared memory then Linux will never use the provisioned TEE
> > > thread. This is weird as in such a case OP-TEE provisions resources
> > > that will never be used, which is the exact opposite goal of this
> > > feature. Verified on our qemu-arm setup.
> > >
> > > For simplicity, I think this system invocation should require OP-TEE
> > > supports dyn shm.
> >
> > It's not obvious to me that this will easier to implement and maintain.
> > Looking at the code in optee_os it looks like using a flag bit as
> > proposed by Sumit would be quite easy to handle.
>
> OP-TEE could auto disable thread provis when dyn shm is disabled, right.

By the way, from OP-TEE OS, we have config switches for dyn-shm and
system context provisioning.
The latter could depend on the former so it's clear at build time when
TEE can embed the capability.

Br,
etienne

> Will it be sufficient? We will still face cases where an OP-TEE
> provisions thread but Linux kernel is not aware (older vanilla kernel
> used with a recent OP-TEE OS). Not much platforms are really affected
> I guess but those executing with pager in small RAMs where a 4kB
> thread context costs.
>
> (snip)

Re: [PATCH 1/2] tee: system invocation

[Etienne Carriere]

On Thu, 9 Feb 2023 at 09:11, Etienne Carriere
<etienne.carriere@linaro.org> wrote:
>
> Hi Jens,
>
>
> On Thu, 9 Feb 2023 at 08:14, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> >
> > Hi Etienne,
> >
> > On Wed, Feb 08, 2023 at 06:09:17PM +0100, Etienne Carriere wrote:
> > > Hello Sumit, Jens,
> > >
> > [snip]
> > > > > > > >
> > > > > > > >         if  (rpc_arg && tee_shm_is_dynamic(shm)) {
> > > > > > > > -               param.a0 = OPTEE_SMC_CALL_WITH_REGD_ARG;
> > > > > > > > +               if (ctx->sys_service &&
> > > > > > > > +                   (optee->smc.sec_caps & OPTEE_SMC_SEC_CAP_SYSTEM_THREAD))
> > > > > > > > +                       param.a0 = OPTEE_SMC_CALL_SYSTEM_WITH_REGD_ARG;
> > > > > > > > +               else
> > > > > > > > +                       param.a0 = OPTEE_SMC_CALL_WITH_REGD_ARG;
> > > > > > >
> > > > > > > This system thread flag should also be applicable to platforms without
> > > > > > > registered arguments support. IOW, we need similar equivalents for
> > > > > > > OPTEE_SMC_FUNCID_CALL_WITH_ARG and OPTEE_SMC_FUNCID_CALL_WITH_RPC_ARG
> > > > > > > too. So I would rather suggest that we add following flag to all 3
> > > > > > > call types:
> > > > > > >
> > > > > > > #define OPTEE_SMC_CALL_SYSTEM_THREAD_FLAG    0x8000
> > > > > >
> > > > > > The main reason platforms don't support registered arguments is that
> > > > > > they haven't been updated since this was introduced. So if a platform
> > > > > > needs system threads it could update to use registered arguments too.
> > > > >
> > > > > Are we hinting at deprecating reserved shared memory support? If yes,
> > > > > wouldn't it be better to be explicit about it with a boot time warning
> > > > > message about its deprecation?
> > > > >
> > > > > Otherwise it will be difficult to debug for the end user to find out
> > > > > why system thread support isn't activated.
> > > > >
> > > > > > The Linux kernel already supports registered arguments. An advantage
> > > > > > with the current approach is that the ABI is easier to implement
> > > > > > since we have distinct SMC IDs for each function.
> > > > >
> > > > > I see your point but my initial thought was that we don't end up
> > > > > making that list too large that it becomes cumbersome to maintain,
> > > > > involving all the combinatorial.
> > > >
> > > > You have a point. Etienne, do you think we could give it a try at
> > > > https://github.com/OP-TEE/optee_os/pull/5789 to better see how this
> > > > would play out?
> > > >
> > >
> > > Indeed I miss that...
> > > With the patch proposed here, indeed if OP-TEE does not support
> > > dynamic shared memory then Linux will never use the provisioned TEE
> > > thread. This is weird as in such a case OP-TEE provisions resources
> > > that will never be used, which is the exact opposite goal of this
> > > feature. Verified on our qemu-arm setup.
> > >
> > > For simplicity, I think this system invocation should require OP-TEE
> > > supports dyn shm.
> >
> > It's not obvious to me that this will easier to implement and maintain.
> > Looking at the code in optee_os it looks like using a flag bit as
> > proposed by Sumit would be quite easy to handle.
>
> OP-TEE could auto disable thread provis when dyn shm is disabled, right.

By the way, from OP-TEE OS, we have config switches for dyn-shm and
system context provisioning.
The latter could depend on the former so it's clear at build time when
TEE can embed the capability.

Br,
etienne

> Will it be sufficient? We will still face cases where an OP-TEE
> provisions thread but Linux kernel is not aware (older vanilla kernel
> used with a recent OP-TEE OS). Not much platforms are really affected
> I guess but those executing with pager in small RAMs where a 4kB
> thread context costs.
>
> (snip)

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

Re: [PATCH 1/2] tee: system invocation

[Jens Wiklander]

Hi,

On Thu, Feb 09, 2023 at 09:11:53AM +0100, Etienne Carriere wrote:
> Hi Jens,
> 
> 
> On Thu, 9 Feb 2023 at 08:14, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> >
> > Hi Etienne,
> >
> > On Wed, Feb 08, 2023 at 06:09:17PM +0100, Etienne Carriere wrote:
> > > Hello Sumit, Jens,
> > >
> > [snip]
> > > > > > > >
> > > > > > > >         if  (rpc_arg && tee_shm_is_dynamic(shm)) {
> > > > > > > > -               param.a0 = OPTEE_SMC_CALL_WITH_REGD_ARG;
> > > > > > > > +               if (ctx->sys_service &&
> > > > > > > > +                   (optee->smc.sec_caps & OPTEE_SMC_SEC_CAP_SYSTEM_THREAD))
> > > > > > > > +                       param.a0 = OPTEE_SMC_CALL_SYSTEM_WITH_REGD_ARG;
> > > > > > > > +               else
> > > > > > > > +                       param.a0 = OPTEE_SMC_CALL_WITH_REGD_ARG;
> > > > > > >
> > > > > > > This system thread flag should also be applicable to platforms without
> > > > > > > registered arguments support. IOW, we need similar equivalents for
> > > > > > > OPTEE_SMC_FUNCID_CALL_WITH_ARG and OPTEE_SMC_FUNCID_CALL_WITH_RPC_ARG
> > > > > > > too. So I would rather suggest that we add following flag to all 3
> > > > > > > call types:
> > > > > > >
> > > > > > > #define OPTEE_SMC_CALL_SYSTEM_THREAD_FLAG    0x8000
> > > > > >
> > > > > > The main reason platforms don't support registered arguments is that
> > > > > > they haven't been updated since this was introduced. So if a platform
> > > > > > needs system threads it could update to use registered arguments too.
> > > > >
> > > > > Are we hinting at deprecating reserved shared memory support? If yes,
> > > > > wouldn't it be better to be explicit about it with a boot time warning
> > > > > message about its deprecation?
> > > > >
> > > > > Otherwise it will be difficult to debug for the end user to find out
> > > > > why system thread support isn't activated.
> > > > >
> > > > > > The Linux kernel already supports registered arguments. An advantage
> > > > > > with the current approach is that the ABI is easier to implement
> > > > > > since we have distinct SMC IDs for each function.
> > > > >
> > > > > I see your point but my initial thought was that we don't end up
> > > > > making that list too large that it becomes cumbersome to maintain,
> > > > > involving all the combinatorial.
> > > >
> > > > You have a point. Etienne, do you think we could give it a try at
> > > > https://github.com/OP-TEE/optee_os/pull/5789 to better see how this
> > > > would play out?
> > > >
> > >
> > > Indeed I miss that...
> > > With the patch proposed here, indeed if OP-TEE does not support
> > > dynamic shared memory then Linux will never use the provisioned TEE
> > > thread. This is weird as in such a case OP-TEE provisions resources
> > > that will never be used, which is the exact opposite goal of this
> > > feature. Verified on our qemu-arm setup.
> > >
> > > For simplicity, I think this system invocation should require OP-TEE
> > > supports dyn shm.
> >
> > It's not obvious to me that this will easier to implement and maintain.
> > Looking at the code in optee_os it looks like using a flag bit as
> > proposed by Sumit would be quite easy to handle.
> 
> OP-TEE could auto disable thread provis when dyn shm is disabled, right.
> Will it be sufficient? We will still face cases where an OP-TEE
> provisions thread but Linux kernel is not aware (older vanilla kernel
> used with a recent OP-TEE OS). Not much platforms are really affected
> I guess but those executing with pager in small RAMs where a 4kB
> thread context costs.

When you add exceptions you make it more complicated. Now we must
remember to always use dyn shm if we are to succeed in configuring with
system threads. What if both dyn shm and static shm is configured in
OP-TEE, but the kernel only uses static shm?

> > > If OP-TEE could know when Linux does not support TEE system
> > > invocation, then OP-TEE could let any invocation use these provisioned
> > > resources so that they are not wasted.
> > > I think a good way would be Linux to expose if it supports this
> > > capability, during capabilities exchange.
> > > Would you agree with this approach?
> >
> > No, I'm not so keen on adding that side effect to
> > OPTEE_SMC_EXCHANGE_CAPABILITIES.
> 
> It is a capability REE would exchanges with TEE.
> What kind of side effects do you fear?

I was hoping to keep it stateless. One thing less to keep track of when
handing over from a boot stage to the kernel.

> > The way you're describing the problem it sounds like it's a normal world
> > problem to know how many system threads are needed. How about adding a
> > fast call where normal world can request how many system threads should
> > be reserved?  If none are requested, none will be reserved.
> 
> Well, could be. With caps exchange, we have an SMC funcID to REE to
> say to TEE: "reserved the default configured number of sys thread". I
> think it is simpler.

Until you realize the that the default number of system threads doesn't
match what you need.

> 
> With REE calling TEE to provision thread, we would need another call
> to release the reservation. Whe caps exchange, we have a single SMC to
> reconfig the negotiated caps.

A single SMC with growing complexity in its arguments.

Cheers,
Jens

Re: [PATCH 1/2] tee: system invocation

[Jens Wiklander]

Hi,

On Thu, Feb 09, 2023 at 09:11:53AM +0100, Etienne Carriere wrote:
> Hi Jens,
> 
> 
> On Thu, 9 Feb 2023 at 08:14, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> >
> > Hi Etienne,
> >
> > On Wed, Feb 08, 2023 at 06:09:17PM +0100, Etienne Carriere wrote:
> > > Hello Sumit, Jens,
> > >
> > [snip]
> > > > > > > >
> > > > > > > >         if  (rpc_arg && tee_shm_is_dynamic(shm)) {
> > > > > > > > -               param.a0 = OPTEE_SMC_CALL_WITH_REGD_ARG;
> > > > > > > > +               if (ctx->sys_service &&
> > > > > > > > +                   (optee->smc.sec_caps & OPTEE_SMC_SEC_CAP_SYSTEM_THREAD))
> > > > > > > > +                       param.a0 = OPTEE_SMC_CALL_SYSTEM_WITH_REGD_ARG;
> > > > > > > > +               else
> > > > > > > > +                       param.a0 = OPTEE_SMC_CALL_WITH_REGD_ARG;
> > > > > > >
> > > > > > > This system thread flag should also be applicable to platforms without
> > > > > > > registered arguments support. IOW, we need similar equivalents for
> > > > > > > OPTEE_SMC_FUNCID_CALL_WITH_ARG and OPTEE_SMC_FUNCID_CALL_WITH_RPC_ARG
> > > > > > > too. So I would rather suggest that we add following flag to all 3
> > > > > > > call types:
> > > > > > >
> > > > > > > #define OPTEE_SMC_CALL_SYSTEM_THREAD_FLAG    0x8000
> > > > > >
> > > > > > The main reason platforms don't support registered arguments is that
> > > > > > they haven't been updated since this was introduced. So if a platform
> > > > > > needs system threads it could update to use registered arguments too.
> > > > >
> > > > > Are we hinting at deprecating reserved shared memory support? If yes,
> > > > > wouldn't it be better to be explicit about it with a boot time warning
> > > > > message about its deprecation?
> > > > >
> > > > > Otherwise it will be difficult to debug for the end user to find out
> > > > > why system thread support isn't activated.
> > > > >
> > > > > > The Linux kernel already supports registered arguments. An advantage
> > > > > > with the current approach is that the ABI is easier to implement
> > > > > > since we have distinct SMC IDs for each function.
> > > > >
> > > > > I see your point but my initial thought was that we don't end up
> > > > > making that list too large that it becomes cumbersome to maintain,
> > > > > involving all the combinatorial.
> > > >
> > > > You have a point. Etienne, do you think we could give it a try at
> > > > https://github.com/OP-TEE/optee_os/pull/5789 to better see how this
> > > > would play out?
> > > >
> > >
> > > Indeed I miss that...
> > > With the patch proposed here, indeed if OP-TEE does not support
> > > dynamic shared memory then Linux will never use the provisioned TEE
> > > thread. This is weird as in such a case OP-TEE provisions resources
> > > that will never be used, which is the exact opposite goal of this
> > > feature. Verified on our qemu-arm setup.
> > >
> > > For simplicity, I think this system invocation should require OP-TEE
> > > supports dyn shm.
> >
> > It's not obvious to me that this will easier to implement and maintain.
> > Looking at the code in optee_os it looks like using a flag bit as
> > proposed by Sumit would be quite easy to handle.
> 
> OP-TEE could auto disable thread provis when dyn shm is disabled, right.
> Will it be sufficient? We will still face cases where an OP-TEE
> provisions thread but Linux kernel is not aware (older vanilla kernel
> used with a recent OP-TEE OS). Not much platforms are really affected
> I guess but those executing with pager in small RAMs where a 4kB
> thread context costs.

When you add exceptions you make it more complicated. Now we must
remember to always use dyn shm if we are to succeed in configuring with
system threads. What if both dyn shm and static shm is configured in
OP-TEE, but the kernel only uses static shm?

> > > If OP-TEE could know when Linux does not support TEE system
> > > invocation, then OP-TEE could let any invocation use these provisioned
> > > resources so that they are not wasted.
> > > I think a good way would be Linux to expose if it supports this
> > > capability, during capabilities exchange.
> > > Would you agree with this approach?
> >
> > No, I'm not so keen on adding that side effect to
> > OPTEE_SMC_EXCHANGE_CAPABILITIES.
> 
> It is a capability REE would exchanges with TEE.
> What kind of side effects do you fear?

I was hoping to keep it stateless. One thing less to keep track of when
handing over from a boot stage to the kernel.

> > The way you're describing the problem it sounds like it's a normal world
> > problem to know how many system threads are needed. How about adding a
> > fast call where normal world can request how many system threads should
> > be reserved?  If none are requested, none will be reserved.
> 
> Well, could be. With caps exchange, we have an SMC funcID to REE to
> say to TEE: "reserved the default configured number of sys thread". I
> think it is simpler.

Until you realize the that the default number of system threads doesn't
match what you need.

> 
> With REE calling TEE to provision thread, we would need another call
> to release the reservation. Whe caps exchange, we have a single SMC to
> reconfig the negotiated caps.

A single SMC with growing complexity in its arguments.

Cheers,
Jens

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

Re: [PATCH 1/2] tee: system invocation

[Etienne Carriere]

On Thu, 9 Feb 2023 at 10:19, Jens Wiklander <jens.wiklander@linaro.org> wrote:
>
> Hi,
>
> On Thu, Feb 09, 2023 at 09:11:53AM +0100, Etienne Carriere wrote:
> > Hi Jens,
> >
> >
> > On Thu, 9 Feb 2023 at 08:14, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> > >
> > > Hi Etienne,
> > >
> > > On Wed, Feb 08, 2023 at 06:09:17PM +0100, Etienne Carriere wrote:
> > > > Hello Sumit, Jens,
> > > >
> > > [snip]
> > > > > > > > >
> > > > > > > > >         if  (rpc_arg && tee_shm_is_dynamic(shm)) {
> > > > > > > > > -               param.a0 = OPTEE_SMC_CALL_WITH_REGD_ARG;
> > > > > > > > > +               if (ctx->sys_service &&
> > > > > > > > > +                   (optee->smc.sec_caps & OPTEE_SMC_SEC_CAP_SYSTEM_THREAD))
> > > > > > > > > +                       param.a0 = OPTEE_SMC_CALL_SYSTEM_WITH_REGD_ARG;
> > > > > > > > > +               else
> > > > > > > > > +                       param.a0 = OPTEE_SMC_CALL_WITH_REGD_ARG;
> > > > > > > >
> > > > > > > > This system thread flag should also be applicable to platforms without
> > > > > > > > registered arguments support. IOW, we need similar equivalents for
> > > > > > > > OPTEE_SMC_FUNCID_CALL_WITH_ARG and OPTEE_SMC_FUNCID_CALL_WITH_RPC_ARG
> > > > > > > > too. So I would rather suggest that we add following flag to all 3
> > > > > > > > call types:
> > > > > > > >
> > > > > > > > #define OPTEE_SMC_CALL_SYSTEM_THREAD_FLAG    0x8000
> > > > > > >
> > > > > > > The main reason platforms don't support registered arguments is that
> > > > > > > they haven't been updated since this was introduced. So if a platform
> > > > > > > needs system threads it could update to use registered arguments too.
> > > > > >
> > > > > > Are we hinting at deprecating reserved shared memory support? If yes,
> > > > > > wouldn't it be better to be explicit about it with a boot time warning
> > > > > > message about its deprecation?
> > > > > >
> > > > > > Otherwise it will be difficult to debug for the end user to find out
> > > > > > why system thread support isn't activated.
> > > > > >
> > > > > > > The Linux kernel already supports registered arguments. An advantage
> > > > > > > with the current approach is that the ABI is easier to implement
> > > > > > > since we have distinct SMC IDs for each function.
> > > > > >
> > > > > > I see your point but my initial thought was that we don't end up
> > > > > > making that list too large that it becomes cumbersome to maintain,
> > > > > > involving all the combinatorial.
> > > > >
> > > > > You have a point. Etienne, do you think we could give it a try at
> > > > > https://github.com/OP-TEE/optee_os/pull/5789 to better see how this
> > > > > would play out?
> > > > >
> > > >
> > > > Indeed I miss that...
> > > > With the patch proposed here, indeed if OP-TEE does not support
> > > > dynamic shared memory then Linux will never use the provisioned TEE
> > > > thread. This is weird as in such a case OP-TEE provisions resources
> > > > that will never be used, which is the exact opposite goal of this
> > > > feature. Verified on our qemu-arm setup.
> > > >
> > > > For simplicity, I think this system invocation should require OP-TEE
> > > > supports dyn shm.
> > >
> > > It's not obvious to me that this will easier to implement and maintain.
> > > Looking at the code in optee_os it looks like using a flag bit as
> > > proposed by Sumit would be quite easy to handle.
> >
> > OP-TEE could auto disable thread provis when dyn shm is disabled, right.
> > Will it be sufficient? We will still face cases where an OP-TEE
> > provisions thread but Linux kernel is not aware (older vanilla kernel
> > used with a recent OP-TEE OS). Not much platforms are really affected
> > I guess but those executing with pager in small RAMs where a 4kB
> > thread context costs.
>
> When you add exceptions you make it more complicated. Now we must
> remember to always use dyn shm if we are to succeed in configuring with
> system threads. What if both dyn shm and static shm is configured in
> OP-TEE, but the kernel only uses static shm?
>
> > > > If OP-TEE could know when Linux does not support TEE system
> > > > invocation, then OP-TEE could let any invocation use these provisioned
> > > > resources so that they are not wasted.
> > > > I think a good way would be Linux to expose if it supports this
> > > > capability, during capabilities exchange.
> > > > Would you agree with this approach?
> > >
> > > No, I'm not so keen on adding that side effect to
> > > OPTEE_SMC_EXCHANGE_CAPABILITIES.
> >
> > It is a capability REE would exchanges with TEE.
> > What kind of side effects do you fear?
>
> I was hoping to keep it stateless. One thing less to keep track of when
> handing over from a boot stage to the kernel.

Or from a kernel VM unload/reload.

>
> > > The way you're describing the problem it sounds like it's a normal world
> > > problem to know how many system threads are needed. How about adding a
> > > fast call where normal world can request how many system threads should
> > > be reserved?  If none are requested, none will be reserved.
> >
> > Well, could be. With caps exchange, we have an SMC funcID to REE to
> > say to TEE: "reserved the default configured number of sys thread". I
> > think it is simpler.
>
> Until you realize the that the default number of system threads doesn't
> match what you need.

Ok, I see your point. Indeed, Linux drivers requiring system context
could issue a fastcall SMC to request dynamic provisioning of TEE
context resources, and release their request upon driver unload. I
agree it would better scale in the long term. I'll propose something
in a v2.

>
> >
> > With REE calling TEE to provision thread, we would need another call
> > to release the reservation. Whe caps exchange, we have a single SMC to
> > reconfig the negotiated caps.
>
> A single SMC with growing complexity in its arguments.

:)  fair.

>
> Cheers,
> Jens

Re: [PATCH 1/2] tee: system invocation

[Etienne Carriere]

On Thu, 9 Feb 2023 at 10:19, Jens Wiklander <jens.wiklander@linaro.org> wrote:
>
> Hi,
>
> On Thu, Feb 09, 2023 at 09:11:53AM +0100, Etienne Carriere wrote:
> > Hi Jens,
> >
> >
> > On Thu, 9 Feb 2023 at 08:14, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> > >
> > > Hi Etienne,
> > >
> > > On Wed, Feb 08, 2023 at 06:09:17PM +0100, Etienne Carriere wrote:
> > > > Hello Sumit, Jens,
> > > >
> > > [snip]
> > > > > > > > >
> > > > > > > > >         if  (rpc_arg && tee_shm_is_dynamic(shm)) {
> > > > > > > > > -               param.a0 = OPTEE_SMC_CALL_WITH_REGD_ARG;
> > > > > > > > > +               if (ctx->sys_service &&
> > > > > > > > > +                   (optee->smc.sec_caps & OPTEE_SMC_SEC_CAP_SYSTEM_THREAD))
> > > > > > > > > +                       param.a0 = OPTEE_SMC_CALL_SYSTEM_WITH_REGD_ARG;
> > > > > > > > > +               else
> > > > > > > > > +                       param.a0 = OPTEE_SMC_CALL_WITH_REGD_ARG;
> > > > > > > >
> > > > > > > > This system thread flag should also be applicable to platforms without
> > > > > > > > registered arguments support. IOW, we need similar equivalents for
> > > > > > > > OPTEE_SMC_FUNCID_CALL_WITH_ARG and OPTEE_SMC_FUNCID_CALL_WITH_RPC_ARG
> > > > > > > > too. So I would rather suggest that we add following flag to all 3
> > > > > > > > call types:
> > > > > > > >
> > > > > > > > #define OPTEE_SMC_CALL_SYSTEM_THREAD_FLAG    0x8000
> > > > > > >
> > > > > > > The main reason platforms don't support registered arguments is that
> > > > > > > they haven't been updated since this was introduced. So if a platform
> > > > > > > needs system threads it could update to use registered arguments too.
> > > > > >
> > > > > > Are we hinting at deprecating reserved shared memory support? If yes,
> > > > > > wouldn't it be better to be explicit about it with a boot time warning
> > > > > > message about its deprecation?
> > > > > >
> > > > > > Otherwise it will be difficult to debug for the end user to find out
> > > > > > why system thread support isn't activated.
> > > > > >
> > > > > > > The Linux kernel already supports registered arguments. An advantage
> > > > > > > with the current approach is that the ABI is easier to implement
> > > > > > > since we have distinct SMC IDs for each function.
> > > > > >
> > > > > > I see your point but my initial thought was that we don't end up
> > > > > > making that list too large that it becomes cumbersome to maintain,
> > > > > > involving all the combinatorial.
> > > > >
> > > > > You have a point. Etienne, do you think we could give it a try at
> > > > > https://github.com/OP-TEE/optee_os/pull/5789 to better see how this
> > > > > would play out?
> > > > >
> > > >
> > > > Indeed I miss that...
> > > > With the patch proposed here, indeed if OP-TEE does not support
> > > > dynamic shared memory then Linux will never use the provisioned TEE
> > > > thread. This is weird as in such a case OP-TEE provisions resources
> > > > that will never be used, which is the exact opposite goal of this
> > > > feature. Verified on our qemu-arm setup.
> > > >
> > > > For simplicity, I think this system invocation should require OP-TEE
> > > > supports dyn shm.
> > >
> > > It's not obvious to me that this will easier to implement and maintain.
> > > Looking at the code in optee_os it looks like using a flag bit as
> > > proposed by Sumit would be quite easy to handle.
> >
> > OP-TEE could auto disable thread provis when dyn shm is disabled, right.
> > Will it be sufficient? We will still face cases where an OP-TEE
> > provisions thread but Linux kernel is not aware (older vanilla kernel
> > used with a recent OP-TEE OS). Not much platforms are really affected
> > I guess but those executing with pager in small RAMs where a 4kB
> > thread context costs.
>
> When you add exceptions you make it more complicated. Now we must
> remember to always use dyn shm if we are to succeed in configuring with
> system threads. What if both dyn shm and static shm is configured in
> OP-TEE, but the kernel only uses static shm?
>
> > > > If OP-TEE could know when Linux does not support TEE system
> > > > invocation, then OP-TEE could let any invocation use these provisioned
> > > > resources so that they are not wasted.
> > > > I think a good way would be Linux to expose if it supports this
> > > > capability, during capabilities exchange.
> > > > Would you agree with this approach?
> > >
> > > No, I'm not so keen on adding that side effect to
> > > OPTEE_SMC_EXCHANGE_CAPABILITIES.
> >
> > It is a capability REE would exchanges with TEE.
> > What kind of side effects do you fear?
>
> I was hoping to keep it stateless. One thing less to keep track of when
> handing over from a boot stage to the kernel.

Or from a kernel VM unload/reload.

>
> > > The way you're describing the problem it sounds like it's a normal world
> > > problem to know how many system threads are needed. How about adding a
> > > fast call where normal world can request how many system threads should
> > > be reserved?  If none are requested, none will be reserved.
> >
> > Well, could be. With caps exchange, we have an SMC funcID to REE to
> > say to TEE: "reserved the default configured number of sys thread". I
> > think it is simpler.
>
> Until you realize the that the default number of system threads doesn't
> match what you need.

Ok, I see your point. Indeed, Linux drivers requiring system context
could issue a fastcall SMC to request dynamic provisioning of TEE
context resources, and release their request upon driver unload. I
agree it would better scale in the long term. I'll propose something
in a v2.

>
> >
> > With REE calling TEE to provision thread, we would need another call
> > to release the reservation. Whe caps exchange, we have a single SMC to
> > reconfig the negotiated caps.
>
> A single SMC with growing complexity in its arguments.

:)  fair.

>
> Cheers,
> Jens

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