tee_client_api: register user memory #72

lorc · 2016-12-13T19:10:46Z

This patch is the userspace part of dynamic shared memory in OP-TEE (refer to OP-TEE/optee_os#1232). This patch is authred by @jenswi-linaro, I just tidied it a bit and rebased on current master.

This patch utilizes new ioctl that allows client application to share any buffer with OP-TEE.

etienne-lms · 2016-12-14T09:29:46Z

why registering the shared memory to the optee secure world ?

lorc · 2016-12-14T12:55:20Z

@etienne-lms, Idea is not to use preshared pool. There are many reasons:

Current carveout have size of 1M on some platforms. This can be not enough in some use cases.
TEEC_RegisterSharedMemory() uses shadow buffer and copies data on every command invocation back and forth.
Preshared pool is not compatible with virtualization, because there are no easy way to split pool between VMs.

If client can use part of own memory for shared buffers, than all issues above can be solved easily.
I'm proposing this changes mainly because I want to use OP-TEE in virtualized environment.

etienne-lms · 2016-12-14T13:35:55Z

ok, i see your idea. And it is true that many buffer parameters can be provided to TAs without copies to the current single physically contiguous 'non secure shared memory area'.

I wonder why nonsecure should register shared buffers to the secure side.

If non-secure wishes to check if a buffer is ok to be used as shared mem, then it's fine, but this is not mandatory and waste cpu cycles. Non-secure could simply invoke the secure side with the memref parameters.

On secure side, we don't care if non-secure presents a shared memory area as "non-secure". Secure side will simply verify the shared memory area conformance against TA memref permissions. And maps it accordingly.

As far a nonsecure handle shared memory allocation, it is nonsecure world responsibility to track these memref, not the secure side.

jenswi-linaro · 2016-12-14T13:46:44Z

For a one shot invocation it's more efficient to do as you say @etienne-lms, but then for a single invocation performance isn't as critical as there's so much else that eats CPU time, checking the RSA signature of the TA for instance.

If we make multiple invocations it quickly pays off to have the shared memory registered in advance as it's represented by a single handle instead of a long list of physical pages.

Another advantage with always using registered memory is that we then can get rid of yet another way of describing a chunk of memory.

igoropaniuk · 2016-12-29T10:02:19Z

@etienne-lms
As I understand (based on @lorc comments and code) it's just some way of extending of existing preshared pool, which used for bypassing GP params, but with ability to work with this shared memory in more generic way (comparing wth GP params). Purposes are almost the same + we can avoid some restrictions of GP specification(amount of params, param size restrictions), and, as @lorc mentioned, unnesessary copying back & forth.

And, speaking about my benchmarking task it's kind of a life saver. I can easily, with minimum amount of code changes, move to this SHM API instead of using GP params :)

igoropaniuk · 2016-12-29T10:02:40Z

Reviewed-by: Igor Opaniuk <igor.opaniuk@linaro.org>

etienne-lms · 2017-01-11T13:43:03Z

late feedback... nice indeed.
Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org>

jbech-linaro · 2017-01-12T11:04:13Z

Same here, late reply, all good. But I'd be nice to have our https://github.com/OP-TEE/optee_os/blob/master/documentation/optee_design.md#7-shared-memory updated to reflect the changes (and the current implementation).

Reviewed-by: Joakim Bech <joakim.bech@linaro.org>

jforissier · 2017-02-02T17:24:03Z

Hi @lorc, would you please rebase this patch on top of the current master branch and add the R-b tags so that I can pick it up?

lorc · 2017-02-02T18:48:55Z

@jforissier I planed to wait till corresponding patches to OP-TEE kernel and OP-TEE driver will be ready for merge. But, actually you are right. This patch can be merged at any time. It is rebased on current master now. Thanks.

jforissier · 2017-02-03T08:53:56Z

@lorc thanks for rebasing. Actually, I'm not sure I want to merge this immediately since it depends on PRs that are still pending in optee_os and the linux driver. I'd rather wait until all 3 PRs are reviewed. Sorry I didn't realize this when I wrote the above ;)

lorc · 2017-03-07T20:39:51Z

Hello. I have rebased it again. Also I have added new patch that makes supplicant to register own memory instead of allocating it thru SHM pool.

etienne-lms

1 main potential issue to crosscheck: closing a fd on a

minor: typo in the commit message of the 2nd commit:
-If OP-TEE and kernel supports registereted shared memory
+If OP-TEE and kernel support registered shared memory

etienne-lms · 2017-03-08T07:51:20Z

tee-supplicant/src/tee_supplicant.c

+		if (munmap(shm->p, shm->size) != 0) {
+			EMSG("munmap(%p, %zu) failed - Error = %s",
+			     shm->p, shm->size, strerror(errno));
+			free(shm);


this should also close(shm->fd);

etienne-lms · 2017-03-08T07:53:19Z

tee-supplicant/src/tee_supplicant.c

@@ -99,6 +101,8 @@ static struct tee_shm *shm_head;

 static const char *ta_dir;

+static uint32_t gen_caps = 0;


minor: useless init

jenswi-linaro · 2017-03-09T16:30:07Z

tee-supplicant/src/tee_supplicant.c

+	if (num_params != 1 || get_value(num_params, params, 0, &val))
+		return TEEC_ERROR_BAD_PARAMETERS;
+
+	if (gen_caps & TEE_GEN_CAP_REG_MEM)


Wouldn't it be possible to rely on ctx->reg_mem instead?

@jenswi-linaro I'm not sure that I understood you. ctx->reg_mem is a kernel thing, isn't it?

Sorry is was confusing this with how we use TEEC_Context in libteec.

Using global variables should in general be avoided, this gen_caps should be stored together with the file descriptor as gen_caps is holds information specific for this file descriptor.

lorc · 2017-05-26T14:17:34Z

@jenswi-linaro, I want ask you about interface between optee client and kernel driver.
Currently we have TEE_IOC_SHM_ALLOC, then I have added TEE_IOC_SHM_REGISTER. Now I want to share tmem buffers. And there are two ways. I can add TEE_IOC_SHM_ADD_TMEM_BUF or:
I can rename TEE_IOC_SHM_REGISTER to TEE_IOC_SHM_ADD and use flags to select desired operation: either register shared buffer in OP-TEE core, or register it in tee framework as a temporary buffer. Which way do you prefer?

jenswi-linaro · 2017-05-29T08:58:40Z

So the difference between TEE_IOC_SHM_REGISTER and TEE_IOC_SHM_ADD_TMEM_BUF is that the latter is not registered in secure world and has to be represented as tmem when invoking for instance?

Why would we need TEE_IOC_SHM_ADD_TMEM_BUF? To me it seems like something user space shouldn't need to worry about.

lorc · 2017-05-29T11:31:02Z

So the difference between TEE_IOC_SHM_REGISTER and TEE_IOC_SHM_ADD_TMEM_BUF is that the latter is not registered in secure world and has to be represented as tmem when invoking for instance?

Exactly.

Why would we need TEE_IOC_SHM_ADD_TMEM_BUF? To me it seems like something user space shouldn't need to worry about.

Because current userspace<->kernelspace interface for TA parameter passing uses shm_id for memory references. My idea is to call TEE_IOC_SHM_ADD_TMEM_BUF to obtain shm_id. Another approach is to add new type for struct tee_ioctl_param.attr. Something like TEE_IOCTL_PARAM_ATTR_TYPE_TMPMEMREF_*

jenswi-linaro · 2017-05-29T11:41:46Z

Why do we need a way for userspace to control if buffers passed to secure world are registered in secure world or not?

lorc · 2017-05-29T11:51:58Z

A Shared Memory block can be registered or allocated once and then used in multiple Commands, and even in multiple Sessions, provided they exist within the scope of the TEE Context in which the Shared Memory was created. This pre-registration is typically more efficient than registering a block of memory using temporary registration if that memory buffer is used in more than one Command invocation.

I thought that TEEC_RegisterSharedMemory() and TEEC_AllocateSharedMemory() will register buffers in secure world (by using OPTEE_MSG_CMD_REGISTER_SHM SMC), while temporary memory buffers will not be registered and will be passed as list of pages every time TA function is invoked.

jenswi-linaro · 2017-05-29T12:05:45Z

OK, if I understand it correctly it's something that doesn't need to be done now.

It sounds like some optimization that could be useful but we need to know that it's actually is needed before we do something about it as it complicates things further a tiny bit. But we should right now make sure that if we choose to implement this later that the current ABI can be easily extended to deal with it. The flags field in struct tee_ioctl_shm_register_data seems suitable for this.

lorc · 2017-05-29T12:11:30Z

Ah, I see. Yes. You can think about this as an optimization. It needs extensions in tee core and in client library, but it is not necessary right now. So yes, we actually can postpone this for now.

I have tested ABI between NW and SW and it supports this use-case. I can share my patches for client library, kernel driver and xtest if you also want to test it.

jenswi-linaro · 2017-05-29T12:21:35Z

Right now I'd like to focus on getting the current stuff in place.
Once that's in place I'd be very interested in taking a look and test it.

etienne-lms

2 minor comments

etienne-lms · 2017-08-22T13:23:54Z

libteec/include/linux/tee.h

@@ -145,6 +146,36 @@ struct tee_ioctl_shm_register_fd_data {
 				     struct tee_ioctl_shm_register_fd_data)

 /**
+ * struct tee_shm_register_data - Shared memory register argument


s/tee_shm_register_data/tee_ioctl_shm_register_data/

etienne-lms · 2017-08-22T13:27:05Z

libteec/src/tee_client_api.c

@@ -34,6 +34,7 @@
 #include <fcntl.h>
 #include <limits.h>
 #include <pthread.h>
+#include <stdlib.h>


minor: move after stdbool.h. Can you fix order of these #include <stXXX.h>.

lorc · 2017-09-19T21:20:41Z

Hello.

I rebased this PR onto current master. There was conflicts in #include part of the files. So I have moved #include <stdbool.h> into public header during conflict resolving. You might want to review this. I can't make this change as [review] patch, because it is product of conflict resolution during rebase.

Also I have addressed @jenswi-linaro's comment regarding global gen_caps variable in supplicant code. It is in the separate patch.

jenswi-linaro

With or without my comment addressed
Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>

jenswi-linaro · 2017-09-20T09:13:42Z

libteec/src/tee_client_api.c

 		if (fd >= 0) {
 			ctx->fd = fd;
+			ctx->reg_mem = !!(gen_caps & TEE_GEN_CAP_REG_MEM);


No need to normalize the bool explicitly as the compiler already is required to do that.

lorc · 2017-09-20T14:29:38Z

Pushed fix for !!(), also added Jens's R-b tag.

etienne-lms

some more...
r-b applies anyway...

etienne-lms · 2017-09-20T14:43:10Z

libteec/include/linux/tee.h

@@ -49,6 +49,7 @@
 #define TEE_MAX_ARG_SIZE	1024

 #define TEE_GEN_CAP_GP		(1 << 0)/* GlobalPlatform compliant TEE */
+#define TEE_GEN_CAP_REG_MEM	(1 << 2)/* Supports registering shared memory */


as you are so patient, would you mind adding a tabulation before these comments.

I'll do this in a separate patch, okay?

as you like. (sorry to ask you to :)

etienne-lms · 2017-09-20T14:44:07Z

libteec/include/linux/tee.h

+};
+
+/**
+ * TEE_IOC_SHM_REGISTER - Register shared memory argument


minor: remove argument

etienne-lms · 2017-09-20T14:46:47Z

tee-supplicant/src/tee_supplicant.c

+		shm = alloc_shm(arg->fd, val->b);
+
+	if (!shm) {
+		return TEEC_ERROR_OUT_OF_MEMORY;


minor: remove {/} ?

lorc · 2017-09-20T15:05:38Z

Addressed @etienne-lms's comments. Added separate patch that fixes comments in tee.h

BTW, I'm sorry that my English is far from perfect. Feel free to correct me, especially if my mistakes are terrible :)

etienne-lms · 2017-09-20T15:20:44Z

Same with me, so do not hesitate to contradict/correct me.

etienne-lms · 2017-09-20T15:21:46Z

r-b on the whole.

Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org> Signed-off-by: Volodymyr Babchuk <vlad.babchuk@gmail.com> Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org> Reviewed-by: Igor Opaniuk <igor.opaniuk@linaro.org> Reviewed-by: Joakim Bech <joakim.bech@linaro.org> Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>

If OP-TEE and kernel support registereted shared memory, then supplicant will use this feature instead of using shared memory pool. Signed-off-by: Volodymyr Babchuk <vlad.babchuk@gmail.com> Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org> Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org>

lorc · 2017-09-20T15:40:44Z

Thanks, @etienne-lms. I have squashed all fixes and added your r-b tag.
@jenswi-linaro, there is a new patch. It changes only comments/formatting. Probably, you want take a look. Or I can put it in a separate PR, to finish this faster :)

jenswi-linaro · 2017-09-20T16:19:56Z

Later is fine with me a least.

lorc · 2017-09-20T16:26:29Z

Okay, so I dropped this patch for now. Will push it as a separate PR after this one will be merged.

lorc force-pushed the new_shm_rel branch from 7f3926a to 4167936 Compare February 2, 2017 18:45

lorc force-pushed the new_shm_rel branch from 4167936 to 445ae76 Compare March 7, 2017 20:38

etienne-lms reviewed Mar 8, 2017

View reviewed changes

jenswi-linaro reviewed Mar 9, 2017

View reviewed changes

lorc force-pushed the new_shm_rel branch 2 times, most recently from 75049dc to 190409b Compare March 10, 2017 19:13

etienne-lms mentioned this pull request Mar 13, 2017

Is there a way to pass fixed physical memory region to TA as a parameter (shm_mem)? OP-TEE/optee_os#1409

Closed

lorc mentioned this pull request Jul 6, 2017

Enable dynamic SHM support OP-TEE/optee_os#1631

Merged

etienne-lms reviewed Aug 24, 2017

View reviewed changes

jenswi-linaro mentioned this pull request Sep 19, 2017

New OP-TEE SHM design. linaro-swg/linux#29

Merged

lorc force-pushed the new_shm_rel branch from 190409b to 71dd6b4 Compare September 19, 2017 21:06

jenswi-linaro approved these changes Sep 20, 2017

View reviewed changes

lorc force-pushed the new_shm_rel branch 2 times, most recently from 289b1f7 to 6cd81d5 Compare September 20, 2017 14:28

etienne-lms reviewed Sep 20, 2017

View reviewed changes

jenswi-linaro and others added 2 commits September 20, 2017 18:36

lorc force-pushed the new_shm_rel branch from b38464b to 8f6c6e9 Compare September 20, 2017 15:37

lorc force-pushed the new_shm_rel branch from 8f6c6e9 to 2cc647a Compare September 20, 2017 16:25

jforissier merged commit 075c56e into OP-TEE:master Oct 5, 2017

		@@ -99,6 +101,8 @@ static struct tee_shm *shm_head;

		static const char *ta_dir;

		static uint32_t gen_caps = 0;

tee_client_api: register user memory #72

tee_client_api: register user memory #72

Conversation

lorc commented Dec 13, 2016

etienne-lms commented Dec 14, 2016

lorc commented Dec 14, 2016

etienne-lms commented Dec 14, 2016

jenswi-linaro commented Dec 14, 2016

igoropaniuk commented Dec 29, 2016

igoropaniuk commented Dec 29, 2016

etienne-lms commented Jan 11, 2017

jbech-linaro commented Jan 12, 2017

jforissier commented Feb 2, 2017

lorc commented Feb 2, 2017

jforissier commented Feb 3, 2017

lorc commented Mar 7, 2017

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

lorc commented May 26, 2017

jenswi-linaro commented May 29, 2017 • edited Loading

lorc commented May 29, 2017

jenswi-linaro commented May 29, 2017

lorc commented May 29, 2017

jenswi-linaro commented May 29, 2017

lorc commented May 29, 2017

jenswi-linaro commented May 29, 2017

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

lorc commented Sep 19, 2017

jenswi-linaro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lorc commented Sep 20, 2017

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

lorc commented Sep 20, 2017

etienne-lms commented Sep 20, 2017

etienne-lms commented Sep 20, 2017

lorc commented Sep 20, 2017

jenswi-linaro commented Sep 20, 2017

lorc commented Sep 20, 2017

jenswi-linaro commented May 29, 2017 •

edited

Loading