Provision TEE threads for system invocations #5789

etienne-lms · 2023-01-24T11:20:11Z

Proposal to add an SMC function ID to OP-TEE entry for system operations invocation using provisioned thread contexts.
Related to ~~linaro-swg/linux#108~~ Linux [PATCH 1/2] and [PATCH 2/2].

(edited: updated Linux counterpart refs)

core/include/kernel/thread.h

mk/config.mk

core/arch/arm/kernel/thread.c

core/arch/arm/include/sm/optee_smc.h

etienne-lms · 2023-01-26T17:04:43Z

Comment addressed (in the right way, i hope).
I've added a tee_caps.c driver i tried to make generic. Tell me if it makes sense.

mk/config.mk

core/include/tee/tee_caps.h

jenswi-linaro · 2023-01-27T07:32:14Z

I've added a tee_caps.c driver i tried to make generic. Tell me if it makes sense.

I don't think it's needed. In principle, all the capabilities we announce to the normal world are new features that it doesn't matter if the normal world uses or not. I mean, if normal world doesn't use one particular feature there's nothing OP-TEE can do about that. We should try to keep things as simple as possible.

core/arch/arm/kernel/thread.c

etienne-lms · 2023-01-27T09:56:37Z

I've added a tee_caps.c driver i tried to make generic. Tell me if it makes sense.

I don't think it's needed. In principle, all the capabilities we announce to the normal world are new features that it doesn't matter if the normal world uses or not. I mean, if normal world doesn't use one particular feature there's nothing OP-TEE can do about that. We should try to keep things as simple as possible.

I agree with making things simple.... hmmm, okay. I'll drop that. That means updating device images with this feature needs linux kernel image to be updated first then TEE image to be updated unless what the provisioned TEE system threads will not be used by the old Linux which may affect the platform.

etienne-lms · 2023-01-27T09:58:46Z

Seen that the series makes changes that are reverted afterward, are you okay i squash fixup commits for the next proposal?

jenswi-linaro · 2023-01-27T10:54:14Z

Seen that the series makes changes that are reverted afterward, are you okay i squash fixup commits for the next proposal?

Please do.

etienne-lms · 2023-01-27T11:21:21Z

Udpdated.

core/arch/arm/include/sm/optee_smc.h

jenswi-linaro · 2023-01-27T12:11:27Z

With OPTEE_SMC_FUNCID_CALL_SYSTEM_WITH_REGD_ARG updated:
Reviewed-by: Jens Wiklander <[email protected]>

Please wait with merging this until we know how the counterpart in the Linux kernel driver is received.

jenswi-linaro · 2023-01-30T09:56:42Z

Feel free to squash and apply the tag.

Adds provisioned system threads among the OP-TEE provisioned threads. New config switch CFG_NUM_SYSTEM_THREADS defines a number of thread contexts reserved for system function invocations. The feature is reported by TEE during capabilities exchange. This is needed for platforms where specific OP-TEE yielded services are dedicated to system management that can be indirectly invoked from an RPC sequence and hence cannot wait clients complete their invocation to release their TEE thread context unless what system can deadlock. SCMI services for clocks, regulators and more, exposed by the SCMI PTA, are examples of such system services. Adds a new SMC function ID OPTEE_SMC_CALL_SYSTEM_WITH_REGD_ARG, defined for yielded system calls expecting using system provisioned resources. The implementation uses a best effort strategy when allocating a system thread. If all system thread contexts are already in use, allocate from the common pool. Reviewed-by: Jens Wiklander <[email protected]> Signed-off-by: Etienne Carriere <[email protected]>

Provision a system thread for SCMI support on platform STM32MP1 Acked-by: Jens Wiklander <[email protected]> Signed-off-by: Etienne Carriere <[email protected]>

etienne-lms · 2023-02-01T21:08:52Z

Tags applied (@jenswi-linaro, i changed the plat-stm32mp1 R-b into an A-b)

From Linux optee driver discussion, REE should call OP-TEE to reserve system contexts when needed. This change modifies how threads are allocated (check counters instead of finding a free cell). Adds 2 fastcall SMC functions for REE to reserve/release system contexts that is thread_counts.sys_reserved. The policy allows context reservation as far as there remains at least 1 generic purpose thread context. Signed-off-by: Etienne Carriere <[email protected]>

Enable CFG_RESERVED_SYSTEM_THREAD. Signed-off-by: Etienne Carriere <[email protected]>

etienne-lms · 2023-02-14T15:33:21Z

Updated to address comments at upstream.
~~Linux driver patches v2 can be found here~~
Linux driver patches v4.

jenswi-linaro

I'm also responding in the kernel thread for the kernel side of things.

jenswi-linaro · 2023-02-15T08:13:30Z

core/arch/arm/include/sm/optee_smc.h

+ */
+#define OPTEE_SMC_FUNCID_CALL_RESERVE_SYS_THREAD	U(21)
+#define OPTEE_SMC_CALL_RESERVE_SYS_THREAD \
+	OPTEE_SMC_STD_CALL_VAL(OPTEE_SMC_FUNCID_CALL_RESERVE_SYS_THREAD)


This should be a fast call, same below.

right, I messed this.

jenswi-linaro · 2023-02-15T08:28:58Z

core/arch/arm/kernel/thread.c

@@ -35,6 +35,22 @@
 #include <trace.h>
 #include <util.h>

+/*


This is more complicated than expected. I was expecting one new global variable which would replace the old CFG_NUM_SYSTEM_THREADS, leaving __thread_alloc_and_run() unchanged apart from that.

With the new scheme, we expect REE to call optee to reserve system threads. This could be done there are already TEE threads assigned, so we can have the thread context pool split in 2 parts (upper indices for system invocation, lower part for the rest). We must count how many threads are free hence to logic proposed here.

I don't see why you need to count. If threads are already in use it's likely they are staying off the higher ranges. Are you making all this trouble to guard against the pathological case where all threads have been used and the last of these happens to be one that runs forever?

With the implementation proposed here, there is no specific pre-provisioning of thread contextes on OP-TEE side. It's the REE that requests some system contexts reservation. With this scheme, when REE requests reservation of a system thread context, OP-TEE has to deal with already allocated threads. A pros is that running OP-TEE with a kernel not supporting TEE system context, will be able to use all available TEE thread context.
I'll try to simplify but I fear cases where OP-TEE accept to reserve a system context but fails to let REE use it because, as you said, the upper thread(s) are already in use.

jenswi-linaro · 2023-02-15T08:32:06Z

core/arch/arm/tee/entry_fast.c

+			args->a0 = OPTEE_SMC_RETURN_OK;
+
+	} else {
+		args->a0 = OPTEE_SMC_RETURN_OK;


Shouldn't this be an error?

jenswi-linaro · 2023-02-15T08:32:39Z

core/arch/arm/include/kernel/thread_private_arch.h

@@ -239,5 +239,10 @@ uint32_t thread_handle_std_smc(uint32_t a0, uint32_t a1, uint32_t a2,
 void thread_scall_handler(struct thread_scall_regs *regs);

 void thread_spmc_register_secondary_ep(vaddr_t ep);
+
+/* Reservation of system thread context */
+TEE_Result reserve_sys_thread(void);


Please use a thread_ prefix.

jenswi-linaro · 2023-02-15T10:13:10Z

core/arch/arm/include/sm/optee_smc.h

@@ -203,6 +209,8 @@
 	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 \


Sumit's suggestion with a bit to indicate if it's a system request made a lot of sense. That way we don't add arbitrary limitations.

I don't see where to store that bit. ABI for OPTEE_SMC_CALL_WITH*_ARG leave no room for an extra bit, we need new funcIDs, reversing an ABI register to carry that information (e.g. bit0 of input a4 argument).

Sumit suggested BIT(15) in the SMC ID if I remember correctly.

That would define few new funcIDs, from SMCCC view.
Using existing funcIDs and considering bit15 would be quite easy in Linux and OP-TEE impelmentation, but the description of this specific bit would look a bit hacky, no?

I didn't address this comment yet.

How about we start calling top 4 bits for funcIDs as flags in OP-TEE SMC ABI? Since they are already zeros and would give us enough flexibility to add features like system threads etc. Otherwise the function IDs list will keep on increasing given all the combinatorial involved and hard to keep track off. Also, still there will be 12 bits for real funcIDs which allows that list to go upto value: 8191.

I'd like to have both, combinations explicitly defined so:

#define OPTEE_SMC_CALL_SYSTEM_THREAD_FLAG BIT(15) #define OPTEE_SMC_CALL_SYSTEM_WITH_ARG \ OPTEE_SMC_STD_CALL_VAL(OPTEE_SMC_FUNCID_CALL_WITH_ARG | \ OPTEE_SMC_CALL_SYSTEM_THREAD_FLAG) #define OPTEE_SMC_CALL_SYSTEM_WITH_RPC_ARG \ OPTEE_SMC_STD_CALL_VAL(OPTEE_SMC_FUNCID_CALL_WITH_RPC_ARG | \ OPTEE_SMC_CALL_SYSTEM_THREAD_FLAG) #define OPTEE_SMC_CALL_SYSTEM_WITH_REGD_ARG \ OPTEE_SMC_STD_CALL_VAL(OPTEE_SMC_FUNCID_CALL_WITH_REGD_ARG | \ OPTEE_SMC_CALL_SYSTEM_THREAD_FLAG)

The OPTEE_SMC_CALL_SYSTEM_THREAD_FLAG doesn't make sense for all SMCs so we should still define all SMCs, but with the bit defined there's still some structure in how an SMC is constructed so masking, etc can still be done where that's convenient.

Okay that's fine with me.

Fixes SMC funcIDs RESERVE_SYS_THREAD/UNRESERVE_SYS_THREAD: fastcall IDs not standard + minor renaing (drop CALL_ in macros name). Signed-off-by: Etienne Carriere <[email protected]>

System invocation contexts are reserved from context pool high indices, simplifies lookup for a free thread. Signed-off-by: Etienne Carriere <[email protected]>

request_system_thread_context() and release_system_thread_context() to return an error code when CFG_RESERVED_SYSTEM_THREAD is disabled. Rename [un]eserve_sys_thread() to thread_[un]reserve_sys_ctx(). Signed-off-by: Etienne Carriere <[email protected]>

jenswi-linaro · 2023-02-17T08:07:22Z

At some point, you mentioned that you only wanted to reserve this thread if it was going to be used by normal world. And the reason for that is that you can't afford to waste an unused thread. With paging enabled an unused thread uses ~700 bytes if I remember correctly. Of these ~700 bytes are ~500 bytes VFP state which in theory could be saved on the stack instead. So we're basically doing this to save 200-700 bytes of nonpaged memory?

etienne-lms · 2023-02-17T09:11:02Z

Let me check my numbers 😕, i agree it's maybe not worth it.

etienne-lms · 2023-02-17T10:02:47Z

~~A thread costs 912 bytes of bss (ctx's and pgt entries) and 4kB of nozi (mmu tbl) on 32b.~~
~~On 64b, it costs 768 bytes of bss and 80B of nozi (1 mmu ul1 table).~~
~~Numbers from current post 3.20.0 revision.~~

etienne-lms · 2023-02-17T10:10:59Z

sorry, I messed up retrieving the numbers... I'll update.

etienne-lms · 2023-02-17T10:30:58Z

Cost of a thread (CFG_NUM_THREADS) in static mapping. Numbers from current post 3.20.0 revision.
64b.LPAE: 1872 bytes of bss + 4kbyte of nozi
32b/LPAE: 912 bytes of bss + 4kbyte of nozi
32b/32b-mmu: 768 bytes of bss + (1kB + 128 bytes) of nozi
bss stores ctx's and pgt entries. nozi store mmu tables.

jenswi-linaro · 2023-02-17T18:53:55Z

Thanks for the detailed figures. Are you using 32b/32b-mmu in your use case?

etienne-lms · 2023-02-17T19:31:18Z

No, the platform is using LPAE. But could be an argument for using 32b-mmu on that platform.

etienne-lms · 2023-03-13T13:56:41Z

From recent discussions, Linux should have all info needed to monitor TEE thread entry and provision system entry contexts for dedicated services. I've created linaro-swg/linux#109 on Linux kernel where no change is needed in OP-TEE OS.

etienne-lms · 2023-03-15T08:11:13Z

Closing this P-R in favor to a REE only system thread management.

etienne-lms mentioned this pull request Jan 24, 2023

[RFC] Provision TEE threads for system invocation linaro-swg/linux#108

Closed

jenswi-linaro reviewed Jan 25, 2023

View reviewed changes

core/include/kernel/thread.h Outdated Show resolved Hide resolved

mk/config.mk Outdated Show resolved Hide resolved

core/arch/arm/kernel/thread.c Outdated Show resolved Hide resolved

core/arch/arm/include/sm/optee_smc.h Outdated Show resolved Hide resolved

etienne-lms commented Jan 26, 2023

View reviewed changes

mk/config.mk Outdated Show resolved Hide resolved

core/include/tee/tee_caps.h Outdated Show resolved Hide resolved

jenswi-linaro reviewed Jan 27, 2023

View reviewed changes

core/arch/arm/kernel/thread.c Outdated Show resolved Hide resolved

etienne-lms force-pushed the system-thread branch 2 times, most recently from 12b6db1 to 0b0b9b1 Compare January 27, 2023 11:20

etienne-lms commented Jan 27, 2023

View reviewed changes

core/arch/arm/include/sm/optee_smc.h Outdated Show resolved Hide resolved

etienne-lms added 2 commits February 1, 2023 22:05

plat-stm32mp1: provision system thread for SCMI support

687e777

Provision a system thread for SCMI support on platform STM32MP1 Acked-by: Jens Wiklander <[email protected]> Signed-off-by: Etienne Carriere <[email protected]>

etienne-lms force-pushed the system-thread branch from 4abb03c to 687e777 Compare February 1, 2023 21:07

etienne-lms changed the title ~~[RFC] Provision TEE threads for system invocation~~ Provision TEE threads for system invocations Feb 1, 2023

etienne-lms mentioned this pull request Feb 8, 2023

[RFC] SCMI PTA with OCALL linaro-swg/linux#90

Closed

etienne-lms added 2 commits February 14, 2023 16:06

[review] plat-stm32mp1: provision system thread for SCMI support

b640912

Enable CFG_RESERVED_SYSTEM_THREAD. Signed-off-by: Etienne Carriere <[email protected]>

jenswi-linaro reviewed Feb 15, 2023

View reviewed changes

etienne-lms added 3 commits February 15, 2023 18:11

[review] core: arm: entry for system thread

41a4aa8

Fixes SMC funcIDs RESERVE_SYS_THREAD/UNRESERVE_SYS_THREAD: fastcall IDs not standard + minor renaing (drop CALL_ in macros name). Signed-off-by: Etienne Carriere <[email protected]>

[review] core: arm: entry for system thread

4fa2469

System invocation contexts are reserved from context pool high indices, simplifies lookup for a free thread. Signed-off-by: Etienne Carriere <[email protected]>

etienne-lms closed this Mar 15, 2023

etienne-lms deleted the system-thread branch May 24, 2023 14:06

Provision TEE threads for system invocations #5789

Provision TEE threads for system invocations #5789

Conversation

etienne-lms commented Jan 24, 2023 • edited Loading

etienne-lms commented Jan 26, 2023

jenswi-linaro commented Jan 27, 2023

etienne-lms commented Jan 27, 2023

etienne-lms commented Jan 27, 2023

jenswi-linaro commented Jan 27, 2023

etienne-lms commented Jan 27, 2023

jenswi-linaro commented Jan 27, 2023

jenswi-linaro commented Jan 30, 2023

etienne-lms commented Feb 1, 2023

etienne-lms commented Feb 14, 2023 • edited Loading

jenswi-linaro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jenswi-linaro commented Feb 17, 2023

etienne-lms commented Feb 17, 2023

etienne-lms commented Feb 17, 2023 • edited Loading

etienne-lms commented Feb 17, 2023

etienne-lms commented Feb 17, 2023 • edited Loading

jenswi-linaro commented Feb 17, 2023

etienne-lms commented Feb 17, 2023

etienne-lms commented Mar 13, 2023

etienne-lms commented Mar 15, 2023

etienne-lms commented Jan 24, 2023 •

edited

Loading

etienne-lms commented Feb 14, 2023 •

edited

Loading

etienne-lms commented Feb 17, 2023 •

edited

Loading

etienne-lms commented Feb 17, 2023 •

edited

Loading