[RFC] core: add a new pta to support any ree service for TA #3270

divneil · 2019-09-12T07:55:05Z

o Following new APIs have been added for TA to access REE service

TEE_OpenREESession()
TEE_CloseREESession()
TEE_InvokeREECommand()
These APIs use the same infrastructure as the Global Platform APIs,
however, the first param of array TEE_Param[0] is reserved for
internal use

o 4 commands are reserved and should not be used explicity by TA

OPTEE_MRC_GENERIC_OPEN : Internal usage
OPTEE_MRC_GENERIC_CLOSE : Internal usage
OPTEE_MRC_GENERIC_SERVICE_START: Need to be implemented by REE
service. Sent as part of session
open.
OPTEE_MRC_GENERIC_SERVICE_STOP : Need to be implemented by REE
service. Sent as part of session
close.

Signed-off-by: Divneil Rai Wadhawan [email protected]

divneil · 2019-09-12T08:40:55Z

I am planning to fix CI issues in the end. The review may require code changes which may again make CI to complain for something.

I would like to have a view about the new API signatures. For time being, I thought of using Global Platform APIs signatures for demonstrating this feature.

Ticket: #3266

jenswi-linaro

All commands seem to be routed to tee-supplicant. I've heard of people needing to do RPC to the calling (normal world) client too, I think it would be nice leave room for that with at least some rough idea of how it could be done.

jenswi-linaro · 2019-09-13T06:11:47Z

lib/libutee/include/tee_api_defines.h

@@ -445,6 +445,7 @@

 /* Not specified in the standard */
 #define TEE_NUM_PARAMS  4
+#define REE_NUM_PARAMS  TEE_NUM_PARAMS


This define isn't needed, you should rather use TEE_NUM_PARAMS instead.

jenswi-linaro · 2019-09-13T06:12:12Z

lib/libutee/include/tee_api_types.h

@@ -26,6 +26,8 @@ typedef struct {
 	uint8_t clockSeqAndNode[8];
 } TEE_UUID;

+typedef TEE_UUID REE_UUID;


TEE_UUID is good enough for this purpose, please remove the typedef.

I had posted a question in optee_client for similar comment. TEE_UUID gives an indication it is of TA, so, I did a typedef. If you confirm, it's okay to retain TEE_UUID for reverse UUID, I will modify it.

Yes. In normal world we're using TEEC_UUID, though.

Yes. In normal world we're using TEEC_UUID, though.

Good catch. So our argument "use a single type for any UUID" doesn't really hold :-/
I'd say stick with TEEC_UUID for normal world and TEE_UUID for secure world maybe?

jenswi-linaro · 2019-09-13T06:12:37Z

lib/libutee/include/tee_api_types.h

@@ -57,12 +59,20 @@ typedef union {
 	} value;
 } TEE_Param;

+typedef TEE_Param REE_Param;


TEE_Param is good enough for this purpose, please remove the typedef.

jenswi-linaro · 2019-09-13T06:13:43Z

core/include/optee_rpc_cmd.h

+/*
+ * The first parameter is reserved and filled as
+ *
+ * [in]     param[0].u.value.a	cmd


What is this? Please elaborate. It would also make it easier to understand how things work while looking at the code.

As we know we are only sending out 4 RPC params to REE, and, I need 1 REE param to identify REE service. So, it leaves me only with 3 params, so, as first step (I still need signature discussion of InvokeCommand), I reserved 1.

jenswi-linaro · 2019-09-13T06:16:27Z

core/include/optee_rpc_cmd.h

+ * [in]     param[0].u.value.b	TA instance id
+ * [out]    param[0].u.value.c	REE service handle
+ */
+#define OPTEE_RPC_CMD_GENERIC		30


I'd prefer something more specific, like OPTEE_RPC_CMD_REE_SERVICE

jenswi-linaro · 2019-09-13T07:15:13Z

lib/libutee/tee_api_ree_service.c

+
+	return TEE_InvokeTACommand(ree_session->session,
+				cancellationRequestTimeout,
+				commandID, paramTypes,


I don't think we should mix command IDs passed by the TA with command IDs needed to drive the communication. What if the TA unknowingly happens to reuse the value of OPTEE_MRC_GENERIC_CLOSE?

It is not documented, but it will be wrong usage of commands. Do you think, if I document it well, it is good enough to close this point.

If we could save a parameter I think it would be justified to reserve 0xfffffff0-0xffffffff, with a check in TEE_InvokeREECommand() too.

jenswi-linaro · 2019-09-13T07:18:01Z

lib/libutee/include/pta_generic.h

+
+#define OPTEE_MRC_GENERIC_OPEN			1
+#define OPTEE_MRC_GENERIC_CLOSE			2
+#define OPTEE_MRC_GENERIC_SERVICE_START		3


Why are start and stop needed? Where's invoke?

InvokeCommand is to be negotiated between TA and CA, similar to hello_world/ta/include/hello_world_ta.h. Since, this is custom service, optee_os will not be knowing what commands will be there. PTA will just pass the commands to REE service using tee-supplicant and will get error for non-existent commands.

Why cannot start be implicit by open and stop be implicit by close?

Somewhere I posted with a typo. Correcting here. Apologies for creating a bit of confusion.

The following are to be written by a custom service.

OPTEE_MRC_GENERIC_SERVICE_START OPTEE_MRC_GENERIC_SERVICE_STOP

The following is handled by tee-supplicant

OPTEE_MRC_GENERIC_OPEN (sends in the UUID to find the custom REE service)

find the relevant REE service (based on Message Queue/Dynamic Library) and corresponding OPTEE_MRC_GENERIC_CLOSE closes message queue/Dynamic Library

I'm sorry, I can't see the answer to my question.

I will rephrase it. Let me know where I missed to address the point.

OPTEE_MRC_REE_SERVICE_OPEN/CLOSE is used by tee-supplicant to open/close a message queue or to get/put a handle to DLL.

OPTEE_MRC_REE_SERVICE_START/STOP is to be implemented by REE service. REE service can setup itself before it starts receiving the next command.

InvokeCommand() uses the above defines in commandID.

So why cannot the REE Service automatically do START when it has been received a connection from tee-supplicant and do STOP when the connection is teared down. I don't see why this has to be driven by secure world.

Let's start with the first command sent OPTEE_MRC_REE_SERVICE_OPEN. It sends following parameters to Normal World.

o param[0] = OPTEE_MRC_REE_SERVICE_OPEN
o param[1]= uuid
o param[2] = handle to be filled by tee-supplicant.

There is no space for passing any TA specific parameters. So, a new command is sent internally which is START and it is intended to pass any parameters which TA requested.

jenswi-linaro · 2019-09-13T07:26:39Z

lib/libutee/tee_api_ree_service.c

+TEE_Result TEE_InvokeREECommand(TEE_REESessionHandle ree_session,
+		uint32_t cancellationRequestTimeout,
+		uint32_t commandID, uint32_t paramTypes,
+		TEE_Param params[TEE_NUM_PARAMS],


I don't think that params[0] should be reserved from the TA point of view. We should find another way of passing the required session information.

I maintained the Global Platform API signature. I wanted to save some effort because API signatures are tricky. Do you think if we say 3 params are supported, it is okay to proceed. Internally, I will map to 4 while calling the service.

If we're using this signature of the function we should support 4 parameters from the TA point of view.

As we know that, RPC doesn't allow more than 4, so, unfortunately, we can't support 4 for TA. However, in the next release, I can reduce it to 3.

If its okay to proceed with marking current feature as EXPERIMENTAL, it will help me in starting the next patch. It will be bit of heavy lifting to carry these for a longer time.

I am hoping that it will be agreeable to you as well.

jenswi-linaro · 2019-09-13T07:28:56Z

core/arch/arm/tee/pta_generic.c

+	void *va = NULL;
+	size_t size = param->memref.size;
+
+	if (cached)


How about letting the TA pre-allocate shared memory buffers via another new TA function instead? It would simplify this PTA and also enable zero copy.

Hmm. I think it will complicate the calling mechanism. Currently, it is similar to calling another UTA (internally it calls PTA).

Anyways, it's interesting to have that feature, may be using another PTA, called a zero-copy REE Service. It will also take some good time to implement, so, may be as a next step please.

Yes, and that mess (when a UTA is calling another UTA and temporary buffers must be allocated inside OP-TEE Core) is something i regret now since it's hard to get rid of the code while maintaining backwards compatibility.

I can add a function to allocate non-secure shared memory from a TA.

Currently, from normal world, we can create a Shared memory using Global Platform APIs. I have not explored that interface for now, but it could be another way. However, the programming model will involve interleaving the calls to REE service with buffer fill. It's just that as a first step, it will be a bit of complicated flow to present. Also, I hope that users are careful enough not to start using the shared buffer for TA internal buffers, thinking that I am not exhausting secure RAM ;).

jenswi-linaro · 2019-09-13T07:31:53Z

lib/libutee/tee_api_ree_service.c

+		MSG("Failed to find the ree service\n");
+		goto err;
+	}
+	rsess->handle = init_params[1].value.a;


We could let the PTA save this information if we have each PTA session only hold one REE session. I don't expect many enough REE session to make that a problem.

My thinking process was to attach the REE context to UTA, since, it initiated the call.

Who cleans up when the TA panics or crashes?

I have not seen the code where TA clean up is done. If handling session at PTA does the job, can you please guide me how to do that.

It's something you can do in pta_ree_service_close_session()

I need to explore that. If it can be taken in next patchset, please let me know. That will be really helpful.

jforissier

Hi @divneil, thanks for the RFC. This is an interesting feature that's been requested several times in the past but never implemented (or more precisely: not shared publicly).
Some comments below.

lib/libutee/include/tee_api.h

lib/libutee/include/tee_api_defines.h

lib/libutee/include/tee_api_types.h

core/arch/arm/tee/pta_generic.c

etienne-lms

I think this is pretty nice and clear. (yet, need to consolidate with OP-TEE/optee_client#170 review).

See the few stylish comments. Consider also local variable default initialization, 80 chars max per line, #include alpha sorting. Also prevent new CamelCase typedefs but the strict TEE API extensions. I guess these comments apply to OP-TEE/optee_client#170 also.

etienne-lms · 2019-09-13T07:34:44Z

core/include/optee_rpc_cmd.h

+ * [in]     param[0].u.value.b	TA instance id
+ * [out]    param[0].u.value.c	REE service handle
+ */
+#define OPTEE_RPC_CMD_GENERIC		30


As this is a REE service, suggest OPTEE_RPC_CMD_SERVICE (or REE_SERVICE)

etienne-lms · 2019-09-13T07:35:35Z

lib/libutee/include/tee_api.h

@@ -70,6 +70,21 @@ TEE_Result TEE_InvokeTACommand(TEE_TASessionHandle session,
 				TEE_Param params[TEE_NUM_PARAMS],
 				uint32_t *returnOrigin);

+TEE_Result TEE_OpenREESession(REE_UUID *destination,


Prefer add these in tee_api_extensions.h as these are not in the GPD TEE API which is file targets.

etienne-lms · 2019-09-13T07:40:00Z

lib/libutee/include/tee_api_types.h

@@ -57,12 +59,20 @@ typedef union {
 	} value;
 } TEE_Param;

+typedef TEE_Param REE_Param;


Better simply use only TEE_Param rather than introducing a new type.

Waiting for reply on the same comment by @jbech-linaro and @jforissier

etienne-lms · 2019-09-13T07:40:42Z

lib/libutee/include/tee_api_types.h

@@ -26,6 +26,8 @@ typedef struct {
 	uint8_t clockSeqAndNode[8];
 } TEE_UUID;

+typedef TEE_UUID REE_UUID;


Should keep type TEE_UUID even for REE IDs.

Waiting for reply on the same comment by @jbech-linaro and @jforissier

etienne-lms · 2019-09-13T07:42:36Z

lib/libutee/tee_api_ree_service.c

+#include <string.h>
+
+
+struct __TEE_REESessionHandle {


suggest lower case struct ree_session_handle

I used the existing conventions. I will modify it.

etienne-lms · 2019-09-13T07:44:04Z

core/arch/arm/tee/pta_generic.c

+{
+	uint32_t ptype = TEE_PARAM_TYPE_GET(param_types, idx);
+
+	if ((ptype == TEE_PARAM_TYPE_MEMREF_INPUT) ||


style: recommend a switch/case here and in the 2 next functions.

etienne-lms · 2019-09-13T08:56:03Z

core/arch/arm/tee/pta_generic.c

+
+static inline bool is_param_none(uint32_t param_types, uint32_t idx)
+{
+	if (TEE_PARAM_TYPE_GET(param_types, idx) == TEE_PARAM_TYPE_NONE)


minor: return TEE_PARAM...() = ...;

I wanted to retain the essence of bool as true and false ;). Will change it

etienne-lms · 2019-09-13T08:57:49Z

core/arch/arm/tee/pta_generic.c

+	return TEE_SUCCESS;
+}
+
+static void pta_generic_close_session(void *sess_ctx __unused)


minor: if nothi ngto do, can simply not register a handler function: remove line 286.

divneil · 2019-09-13T10:24:06Z

All commands seem to be routed to tee-supplicant. I've heard of people needing to do RPC to the calling (normal world) client too, I think it would be nice leave room for that with at least some rough idea of how it could be done.

The idea is to route everything to tee-supplicant, and tee-supplicant then passes the control to either dynamic library or message queue. My idea of using these 2 mechanisms are:

Message queue

If the client application wants to use a state machine, it can do so, by synchronizing it's processing with message queue processing

Dynamic library

As an example, I use to fill in the IP addresses from the DNS query. Since, I don't need to maintain a state in this case, so, this interface fits perfectly in that. It easy service to write also ;)

HernanGatta · 2019-09-13T18:14:42Z

@divneil: This is great to see. We also have an implementation for allowing TA's to invoke services from their host application, which we use for the Open Enclave SDK. The support for it is on the SDK's master branch and in our OP-TEE fork.

We've been meaning to upstream that for a while, though it needs a little cleanup before we can do so. Our version does not go through the TEE supplicant, however; we route out calls (OCALLs) directly to the host program.

I haven't read through all the details of this PR yet, but we should definitely compare notes.

divneil · 2019-09-16T07:03:42Z

@divneil: This is great to see. We also have an implementation for allowing TA's to invoke services from their host application, which we use for the [Open Enclave SDK]

:)

however; we route out calls (OCALLs) directly to the host program.

I suppose you are then giving APIs for the host application to wait on teepriv or equivalent, thus completing bypassing the need to have tee-supplicant.

I haven't read through all the details of this PR yet, but we should definitely compare notes.

Sure. How do we connect?

jenswi-linaro · 2019-09-16T08:13:45Z

One thing I noticed earlier but didn't mention is that REE services has a bit in common with socket PTA. In fact it would be possible to implement REE services using a new socket type or vice versa if desired. It would require the function to allocate non-secure shared memory from a user space TA which I mentioned I'll add above.

divneil · 2019-09-16T12:12:28Z

Can you please share which indent tool you are using? I will anyways push the indent changes in the end, when everything is closed.

There are few opens left from the 1st commit. I fixed some quick ones. I am hoping that once you resolve those, it will clear up the screen :) and I will continue the discussions on remaining points.

There will be a delay in pushing the patches for next 2 days also, as I will be in conference for next 2 days.

jenswi-linaro · 2019-09-16T12:16:31Z

Can you please share which indent tool you are using?

In my case it's vim with no fancy settings, just the default 8 spaces per tab and disciplined editing.

divneil · 2019-09-19T09:13:56Z

Can you please have a go through the code once. I think the only open point remaining is reserving the first parameter of TEE_Param when UTA calls for a REE service.

I am of the view that as a first release of this feature (tagging it as experimental), we can go ahead with it. I can then work on hiding this parameter. Currently, hiding this parameter will be another feature activity, as the REE service (msg queue based or .so based) is using the first parameter to find out the command.

If I have to hide this first parameter, then another API needs to be written for REE service, which will register the commands and call them.

jenswi-linaro · 2019-09-19T12:24:52Z

Note that we're trying to be backwards compatible towards both normal world and TAs. So even if an interface is replaced with a more advanced we still have to maintain the old. So I'd rather that we have the full picture of the solution before we start to commit to an ABI.

divneil · 2019-09-19T12:36:29Z

we still have to maintain the old. So I'd rather that we have the full picture of the solution before we start to commit to an ABI.

As I understand, you are looking for next APIs which I am thinking to replace the current ones with. Please confirm.

o Following new APIs have been added for TA to access REE service - TEE_OpenREESession() - TEE_CloseREESession() - TEE_InvokeREECommand() These APIs use the same infrastructure as the Global Platform APIs, however, the first param of array TEE_Param[0] is reserved for internal use o 4 commands are reserved and should not be used explicity by TA - OPTEE_MRC_GENERIC_OPEN : Internal usage - OPTEE_MRC_GENERIC_CLOSE : Internal usage - OPTEE_MRC_GENERIC_SERVICE_START: Need to be implemented by REE service. Sent as part of session open. - OPTEE_MRC_GENERIC_SERVICE_STOP : Need to be implemented by REE service. Sent as part of session close. Signed-off-by: Divneil Rai Wadhawan <[email protected]>

o renamed pta_generic.* to pta_ree_service.* o Replace _GENERIC with _REE_SERVICE o Moved REE function definitons to - tee_api_extensions.h o Moved REE declarations to - tee_api_types_extensions.h Signed-off-by: Divneil Rai Wadhawan <[email protected]>

o Added more comments o Removed _GENERIC_ in missed places o Some generic cleanup Signed-off-by: Divneil Rai Wadhawan <[email protected]>

HernanGatta · 2019-09-19T20:02:32Z

@divneil: I'm preparing a set of PR's with our implementation of the same feature. I'd like for us to compare the two versions and see if there is anything we can learn from each other. The way ours works is very different from what you have done yet allows developers to do fundamentally the same. So perhaps we can arrive at an even better solution by picking what's best from each.

The biggest difference between our two implementations is that the way we did it, the supplicant is not involved at all, so there's no additional hop or bottleneck for RPC's; RPC requests are added to a per-session queue that the host application pumps and processes. Effectively, the host becomes a mini-supplicant. This also helps in that in Windows, the supplicant is part of the OP-TEE driver as opposed to a user-mode service.

I'll have the PR's out by Monday.

divneil · 2019-09-20T05:02:21Z

a user-mode service.

I'll have the PR's out by Monday.

Please don't forget to tag me in PR. Looking forward to it.

jenswi-linaro · 2019-09-20T07:06:12Z

@HernanGatta, I'm looking forward to the PR.

HernanGatta · 2019-09-23T08:59:32Z

@divneil, @jenswi-linaro: Here they are:

OP-TEE OS
OP-TEE Client
OP-TEE Examples

We also have changes to Linux, though I'm uncertain as to where to submit them (besides the official tree):

Linux

divneil · 2019-09-23T11:53:44Z

@divneil, @jenswi-linaro: Here they are:

OP-TEE OS
OP-TEE Client
OP-TEE Examples

I am not well today, so, will be going through it in some time for understanding. I was working on another example last week, so, was barely able to push it out today.

divneil mentioned this pull request Sep 12, 2019

TEE -> REE custom service #3266

Closed

This was referenced Sep 12, 2019

[RFC] tee-supplicant: add support for accessing custom REE service OP-TEE/optee_client#170

Closed

[RFC] hello_world: add demo code for using custom ree service linaro-swg/optee_examples#63

Closed

jenswi-linaro reviewed Sep 13, 2019

View reviewed changes

jforissier reviewed Sep 13, 2019

View reviewed changes

etienne-lms reviewed Sep 13, 2019

View reviewed changes

divneil force-pushed the RFC-generic-pta-for-custom-service branch from 9894f2f to 062d460 Compare September 19, 2019 08:54

divneil-intel added 3 commits September 19, 2019 22:02

core: refactored the custom REE service infrastructure

062d460

o Added more comments o Removed _GENERIC_ in missed places o Some generic cleanup Signed-off-by: Divneil Rai Wadhawan <[email protected]>

jenswi-linaro mentioned this pull request Sep 26, 2019

Custom RPC from user TA #3303

Closed

divneil closed this Oct 23, 2019

[RFC] core: add a new pta to support any ree service for TA #3270

[RFC] core: add a new pta to support any ree service for TA #3270

Conversation

divneil commented Sep 12, 2019

divneil commented Sep 12, 2019 • 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

jenswi-linaro Sep 19, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

divneil Sep 19, 2019 • edited Loading

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

jforissier left a comment

Choose a reason for hiding this comment

etienne-lms 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

divneil commented Sep 13, 2019

Message queue

Dynamic library

HernanGatta commented Sep 13, 2019 • edited Loading

divneil commented Sep 16, 2019 • edited Loading

jenswi-linaro commented Sep 16, 2019

divneil commented Sep 16, 2019

jenswi-linaro commented Sep 16, 2019

divneil commented Sep 19, 2019

jenswi-linaro commented Sep 19, 2019

divneil commented Sep 19, 2019

HernanGatta commented Sep 19, 2019 • edited Loading

divneil commented Sep 20, 2019

jenswi-linaro commented Sep 20, 2019

HernanGatta commented Sep 23, 2019

divneil commented Sep 23, 2019

divneil commented Sep 12, 2019 •

edited

Loading

jenswi-linaro Sep 19, 2019 •

edited

Loading

divneil Sep 19, 2019 •

edited

Loading

HernanGatta commented Sep 13, 2019 •

edited

Loading

divneil commented Sep 16, 2019 •

edited

Loading

HernanGatta commented Sep 19, 2019 •

edited

Loading