-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enable dynamic SHM support #1631
Conversation
core/arch/arm/include/sm/optee_smc.h
Outdated
@@ -290,6 +290,12 @@ | |||
#define OPTEE_SMC_SEC_CAP_HAVE_RESERVED_SHM (1 << 0) | |||
/* Secure world can communicate via previously unregistered shared memory */ | |||
#define OPTEE_SMC_SEC_CAP_UNREGISTERED_SHM (1 << 1) | |||
/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on the mailing list discussion, we don't need both OPTEE_SMC_SEC_CAP_UNREGISTERED_SHM and OPTEE_SMC_SEC_CAP_DYNAMIC_SHM, right?
Jens had proposed changing the existing "UNREGISTERED_SHM" capability to this:
/*
- Secure world supports shared memory objects to represent
- physically non-contiguous memory and the commands
- "register/unregister shared memory object" for more efficient
- communication via physically non-contiguous memory.
*/
#define OPTEE_SMC_SEC_CAP_NONCONTIG_SHM (1 << 1)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, @stuyoder. I did exactly this. I decided not to touch existing OPTEE_SMC_SEC_CAP_UNREGISTERED_SHM
. Instead I added new OPTEE_SMC_SEC_CAP_DYNAMIC_SHM
. I think, that this name better describes this capability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually the UNREGISTERED_SHM relates the the so-called default shm (NSEC_SHM
in the core). This shm is "negotiated" at core inits with nsec world (some SMCs allow to get/set the nsec_shm location, but core_mmu.c expects it from static config CFG_SHMEM_START/_SIZE
). It happens to be physically contiguous and as a single memory chunk. That's its real attributes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@etienne-lms, What do you mean with respect to "negotiated"? UNREGISTERED_SHM is unused as of now in both OP-TEE and Linux.
This is how the capabilities are defined today:
/* Secure world has reserved shared memory for normal world to use /
#define OPTEE_SMC_SEC_CAP_HAVE_RESERVED_SHM (1 << 0)
/ Secure world can communicate via previously unregistered shared memory */
#define OPTEE_SMC_SEC_CAP_UNREGISTERED_SHM (1 << 1)
It is HAVE_RESERVED_SHM that is referring to the physically contiguous static CFG_SHMEM_START/SIZE.
The question is what capabilities we need to refer to the new ability for OP-TEE to map arbitrary, non-contiguous physical addresses.
Do we need:
A). One capability that refers to the capability of OP-TEE to handle non-contiguous shared memory and the capability to handle the register/unregister core APIs?
or
B) . Two capabilities-- 1) one for the capability of OP-TEE to handle non-contiguous shared memory, and 2) one for the capability to handle the register/unregister core APIs.
Jens proposal on the mailing list was for option #A.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lorc, what do the capabilities OPTEE_SMC_SEC_CAP_UNREGISTERED_SHM and OPTEE_SMC_SEC_CAP_DYNAMIC_SHM mean in your view? How are they different?
As I stated above, the first question is whether we need one capability or two capabilities. Once that is decided, we can decide what the best name is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer only on capability as those two are so intertwined.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stuyoder, I'm not using OPTEE_SMC_SEC_CAP_UNREGISTERED_SHM
at all. I have leaved it as is. I use only OPTEE_SMC_SEC_CAP_DYNAMIC_SHM
in my patch series.
I decided not to rename OPTEE_SMC_SEC_CAP_UNREGISTERED_SHM
to OPTEE_SMC_SEC_CAP_DYNAMIC_SHM
because that can add some confusion.
At this moment there are at least 30 free bits for capability flags, so I don't see why we need to reuse existing values.
@lorc Do you have any plan to support user TA mapping for the dynamic SHM? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I seem to have missed this. Here's my comments and then there's the comment by @stuyoder also.
core/arch/arm/kernel/thread.c
Outdated
*cookie = arg->params[0].u.tmem.shm_ref; | ||
mobj = mobj_shm_alloc(arg->params[0].u.tmem.buf_ptr, | ||
arg->params[0].u.tmem.size); | ||
} else if (arg->params[0].attr == |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would have formatted these two lines as:
} else if (arg->params[0].attr == (OPTEE_MSG_ATTR_TYPE_TMEM_OUTPUT |
OPTEE_MSG_ATTR_NONCONTIG)) {
but that's a matter of taste.
core/arch/arm/mm/core_mmu.c
Outdated
@@ -1052,6 +1052,9 @@ static void set_pg_region(struct core_mmu_table_info *dir_info, | |||
|
|||
r.size = MIN(CORE_MMU_PGDIR_SIZE - (r.va - pg_info->va_base), | |||
end - r.va); | |||
r.size = MIN(r.size, mobj_get_phys_granule(region->mobj)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two lines are not relevant for a paged mobj so they should be moved into the if
below.
core/arch/arm/tee/entry_std.c
Outdated
@@ -94,8 +114,26 @@ static TEE_Result assign_mobj_to_param_mem(const paddr_t pa, const size_t sz, | |||
return TEE_ERROR_BAD_PARAMETERS; | |||
} | |||
|
|||
static TEE_Result set_rmem_param(const struct optee_msg_param *param, | |||
struct param_mem *mem) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Align with (
on the line above.
core/arch/arm/tee/entry_std.c
Outdated
static TEE_Result copy_in_params(const struct optee_msg_param *params, | ||
uint32_t num_params, struct tee_ta_param *ta_param) | ||
uint32_t num_params, struct tee_ta_param *ta_param, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Align with (
on the line above.
core/arch/arm/tee/entry_std.c
Outdated
mobj_free(mobj); | ||
arg->ret = TEE_SUCCESS; | ||
} else { | ||
EMSG("Can't find mapping with given cookie\n"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
\n
not needed.
Addressed comments. @prime-zeng, I'm sorry. I didn't get what are you asking. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i need to review this again, seems but still unclear to me.
core/arch/arm/include/sm/optee_smc.h
Outdated
@@ -290,6 +290,12 @@ | |||
#define OPTEE_SMC_SEC_CAP_HAVE_RESERVED_SHM (1 << 0) | |||
/* Secure world can communicate via previously unregistered shared memory */ | |||
#define OPTEE_SMC_SEC_CAP_UNREGISTERED_SHM (1 << 1) | |||
/* | |||
* Secure world supporst commands "register/unregister shared memory", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo /supporst/supports/
add empty line before comment.
core/arch/arm/tee/entry_fast.c
Outdated
IMSG("NS DDR configured. Enabling dynamic SHM"); | ||
args->a1 |= OPTEE_SMC_SEC_CAP_DYNAMIC_SHM; | ||
} else { | ||
EMSG("No NS DDR configured for the platform. Disabling dynamic SHM"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
coudl you log on info level, it is not a error.
core/arch/arm/include/sm/optee_smc.h
Outdated
@@ -290,6 +290,12 @@ | |||
#define OPTEE_SMC_SEC_CAP_HAVE_RESERVED_SHM (1 << 0) | |||
/* Secure world can communicate via previously unregistered shared memory */ | |||
#define OPTEE_SMC_SEC_CAP_UNREGISTERED_SHM (1 << 1) | |||
/* | |||
* Secure world supporst commands "register/unregister shared memory", | |||
* secure world accepts command buffers located in any parts of unsecure RAM |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/unsecure/non secure/
return; | ||
} | ||
mobj = map_cmd_buffer(parg, &num_params); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
get_cmd_buffer()
could handle both case. It would make tee_entry_std()
more readable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer to leave it in current state. get_cmd_buffer()
is responsible for buffers in MEM_AREA_NSEC_SHM
which is already mapped in OP-TEE address space. map_cmd_buffer()
maps buffers from MEM_AREA_RAM_NSEC
. One task for one function.
As alternative I can create a third function that will choose to call get_cmd_buffer()
or map_cmd_buffer()
.
core/arch/arm/include/sm/optee_smc.h
Outdated
@@ -290,6 +290,12 @@ | |||
#define OPTEE_SMC_SEC_CAP_HAVE_RESERVED_SHM (1 << 0) | |||
/* Secure world can communicate via previously unregistered shared memory */ | |||
#define OPTEE_SMC_SEC_CAP_UNREGISTERED_SHM (1 << 1) | |||
/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually the UNREGISTERED_SHM relates the the so-called default shm (NSEC_SHM
in the core). This shm is "negotiated" at core inits with nsec world (some SMCs allow to get/set the nsec_shm location, but core_mmu.c expects it from static config CFG_SHMEM_START/_SIZE
). It happens to be physically contiguous and as a single memory chunk. That's its real attributes.
return TEE_SUCCESS; | ||
} | ||
|
||
/* Non-contigous buffer from non sec DDR? */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
apology: could you add a space before the ?
for consistency with below. (which could be rephrased some day).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No space before ?
in English...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we'll clean the other comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added separate patch that does this.
core/arch/arm/tee/entry_std.c
Outdated
false); | ||
if (!mem->mobj) | ||
return TEE_ERROR_BAD_PARAMETERS; | ||
mem->offs = (pa & SMALL_PAGE_MASK); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor: useless parenthesis.
I agree with @lorc. Since his work on the shm, the memref parameters of TA invocations are now handled through memory object |
return TEE_ERROR_BAD_PARAMETERS; | ||
if (params[n].attr & OPTEE_MSG_ATTR_NONCONTIG) | ||
return TEE_ERROR_BAD_PARAMETERS; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should reject noncontig attributes.
Non-contig reference are used only from register_[un]shm()
std entries to register a reference related to the parameter type OPTEE_MSG_ATTR_RMEM_*
.
Current function copy_in_params()
is called from open/close_session()
and and invoke_command()
. It does not expect non-contig references, only value (with.. value), tmem (with its phys addr) and rmem (with its registered ref) types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean we should disable noncontig bufs as parameters to TA unless it's via already registered buffers? Why?
I think it could be useful for one-shot invokes etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it could be useful for one-shot invokes etc.
You are right. It could be used.
So it would be allowed only for OPTEE_MSG_ATTR_TMEM_*
types, as used in assign_mobj_to_param_mem()
.
But TEE Client API expects client to either use TEEC_AllocateSharedMemory()
or TEE_RegisterSharedMemory(),
do we really need to support here un-registered shm references ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's the TEEC_MEMREF_TEMP_*
also.
@etienne-lms @lorc I mean to ask whether the User TAs can use the registered (not physically contiguous )memory. And i see the final User TAs mapping are created in the function |
As of current implementation, yes, the function |
@etienne-lms i got it, thank you, so i think maybe the problem now is how to fix the |
@prime-zeng , actually, one of the changes makes @etienne-lms, I have addressed your comments. [1] https://github.com/OP-TEE/optee_os/pull/1631/files#diff-21c5a372f38c092a429fd2b7e8f71ef3R1060 |
I'd like to run some tests with this, which driver and client branch did you use? |
Hi @jenswi-linaro, you will need kernel patches from this PR: linaro-swg/linux#29 |
This works very well. The ABI provided to normal world is flexible enough to handle all use cases and I'm pretty sure it will survive a review on the kernel mailing lists without incompatible changes. I'd like to wrap this up and merge it as soon as possible. We should do the same for linaro-swg/linux#29 and OP-TEE/optee_client#72. When it comes to upstreaming the kernel changes I'd be grateful I you posted the patches @lorc, in order to drive the process on the kernel mailing lists. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some late minor comments.
Looks ok to me: Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org>
core/arch/arm/tee/entry_fast.c
Outdated
@@ -103,6 +103,13 @@ static void tee_entry_exchange_capabilities(struct thread_smc_args *args) | |||
|
|||
args->a0 = OPTEE_SMC_RETURN_OK; | |||
args->a1 = OPTEE_SMC_SEC_CAP_HAVE_RESERVED_SHM; | |||
|
|||
if (core_mmu_nsec_ddr_is_defined()) { | |||
IMSG("NS DDR configured. Enabling dynamic SHM"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor: s/configured/defined/. This code does not really configure anything.
Same below.
core/arch/arm/tee/entry_std.c
Outdated
return TEE_SUCCESS; | ||
} | ||
|
||
/* belongs to nonsecure shared memory? */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor: s/belongs/Belongs/
core/arch/arm/tee/entry_std.c
Outdated
if (param_mem_from_mobj(mem, shm_mobj, pa, sz)) | ||
return TEE_SUCCESS; | ||
|
||
#ifdef CFG_SECURE_DATA_PATH | ||
/* belongs to SDP memories ? */ | ||
/* belongs to SDP memories? */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor: s/belongs/Belongs/
|
@lorc could you please squash/rebase the patches and apply the Reviewed-by: tags? Can this safely be merged without the linux and optee_client patches? I'm concerned by the "ABI change:" text. Assuming the changes are compatible, I'd suggest changing the subject of two commits as follows:
Did I get this right? |
ae9d888
to
0f0cff2
Compare
Hello. I have squashed, rebased and applied tags to this patch series. |
You can add: |
Thank you, @jenswi-linaro , I added your R-b tag and repushed patches. |
Sorry to bother, but I'm still not happy with the commit subject: "core: add shared memory support". OP-TEE already supports shared memory so it doesn't make sense. Say waht you are really adding. Same goes with "core: enable TMEM parameters with non-contiguous shm". What is TMEM? This acronym appears absolutely nowhere, not even in that patch. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- still late comments....
Commit " entry_std.c: comment fixes: "
- can you change 1st line to "core: fix comments in entry_std.c"
Comment of commit "entry_std: save parameters attributes into local memory"
- s/poses/pose/
- s/parameter attribute/parameter attributes/.
core/arch/arm/tee/entry_std.c
Outdated
@@ -292,6 +325,9 @@ static void entry_open_session(struct thread_smc_args *smc_args, | |||
plat_prng_add_jitter_entropy(); | |||
|
|||
out: | |||
cleanup_params(arg->params + num_meta, saved_attr, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you cross check that there is no issue when this cleanup from get_open_session_meta()
. num_meta
may not be inited ?
e6091b5
to
126bf59
Compare
Yeah, I thought about this. But, it does not eliminate unused code and also, strictly speaking, defined nsec region is not equal to enabled SHM. Yes, defined nsec memory region is prerequisite for dynamic SHM, but It can have other uses... |
@lorc I think your @etienne-lms relying on That being said... for simplicity, I would recommend With that, commit "build: disable..." is: |
Actually, the last change (introduces CFG_...) does not remove dynamic shm support from the optee kernel, it only drop the capability flag. Hence even disabled, the REE can still register and use dyn-shm. It is the expected behaviour? |
Yep. I have pointed this in comments to
|
:) i should have read further. Well, so this is the expected behaviour. |
@etienne-lms yes and it's enough to exercise the usual shm pool on a platform that otherwise supports the dyn shm. |
@jforissier I did as you asked. That was trivial change, so I did it in place, if you don't mind. Also I added your |
@lorc thanks but you forgot to update the commit subject ;-) |
@jforissier oops :) Should be fine now. I also elaborated commit message a bit. |
Shippable fails due to checkpatch warnings that can easily be fixed. |
Together with #1874 |
@jforissier, should I fix this one?
Kernel coding style recommends leave long text constants as is even if they are longer than 80 characters to ease up grepping:
|
What about this (we want the message even when CFG_DYN_SHM_CAP is disabled): bool dshm_en = false;
#ifdef CFG_DYN_SHM_CAP
dhsm_en = core_mmu_nsec_ddr_is_defined();
if (dhsm_en)
args->a1 |= OPTEE_SMC_SEC_CAP_DYNAMIC_SHM;
#endif
IMSG("Dynamic shared memory is %sabled", dhsm_en ? "en" : "dis"); |
Okay, will do in this way. |
Normal world now can call OPTEE_MSG_CMD_REGISTER_SHM and OPTEE_MSG_CMD_UNREGISTER_SHM functions to register/unregister shared memory. After that, it can use OPTEE_MSG_ATTR_TYPE_RMEM_* to reference to that registered shared buffers. 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>
Now, when we can pass list of pages between REE and TEE it is possible to use temporary memory references that are not located in a preallocated shared memory area. By employing OPTEE_MSG_ATTR_NONCONTIG parameter attribute, REE can provide own buffer as a temporary memory reference. Actually, such parameters are indistinguishable from registered shared memory references. So, when OP-TEE spots temporary memory reference with OPTEE_MSG_ATTR_NONCONTIG attribute, it will create `mobj_reg_shm` for it. After call was handled, it will free that mobj. Signed-off-by: Volodymyr Babchuk <vlad.babchuk@gmail.com> Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org> Tested-by: Jerome Forissier <jerome.forissier@linaro.org> (HiKey) Tested-by: Jens Wiklander <jens.wiklander@linaro.org> (FVP, QEMU v7/v8) Tested-by: Jens Wiklander <jens.wiklander@linaro.org> (Juno with and without pager) Tested-by: Volodymyr Babchuk <vlad.babchuk@gmail.com> (Rcar M3)
- removed spaces before "?" in comments - Capitalized first letter in first words 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>
This variable can disable reported capability OPTEE_SMC_SEC_CAP_DYNAMIC_SHM. But dynamic SHM remains fully operational, though. This can be used for testing and debugging to emulate system, where dynamic SHM is not supported. Signed-off-by: Volodymyr Babchuk <vlad.babchuk@gmail.com> Acked-by: Jerome Forissier <jerome.forissier@linaro.org>
Reworked |
Thanks to all the contributors in this thread, @lorc good job! 👍 |
Wow, I can't believe that it is done! Thank you to all of you guys for your support and help. I learned much while working on this feature. |
congratulations @lorc (and all contribs) for this. took a while, thanks for your patience. |
Hello.
There are last patches for dynamic SHM.
I'm not sure, if I did right thing, when created two patches with ABI changes, because 4e5d00d ("ABI change: add shared memory support") enables new capability and 30260ae ("ABI change: enable TMEM parameters with non-contiguous shm") extends it.
Probably I should squash those two patches into one, or create one additional patch for
entry_fast.c
, that enablesOPTEE_SMC_SEC_CAP_DYNAMIC_SHM
. What do you think?