-
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
Support for *flat mapped* coherent secure RAM #1533
Conversation
core/arch/arm/kernel/kern.ld.S
Outdated
* Start coherent RAM with a platform specific structure. | ||
* Generic data to be place in coherent RAM can use attribute "coherent_ram". | ||
*/ | ||
#define DECLARE_COHERENT_RAM_SECTION \ |
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.
Why do we need a macro for this?
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.
To add some flexibility regarding the location of the coherent RAM, in case one wants it at a dedicated place. I proposed to possible location: before the load address or after the core reserved vaddr range (virt, no phys only, to fit with pager constraints). Since there were those 2 places, I preferred to use a single macro. You feel it bring complexity ?
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.
Aha, I didn't notice that. This is good then.
The \
at the end of each line seems to have ended up at column 81 which obviously is beyond 80.
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.
Yep, i've noticed that from travis feedback. Bad luck.
core/arch/arm/kernel/kern.ld.S
Outdated
|
||
#if defined(CFG_TEE_COHERENT_START) | ||
ASSERT((__coherent_start & (4096 - 1)) == 0, "Coherent RAM start is not 4Kbyte aligned") | ||
ASSERT((__coherent_end & (4096 - 1)) == 0, "Coherent RAM end is not 4Kbyte aligned") |
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.
Testing that . = ALIGN(4096);
works?
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.
This is extra checks. Actually __coherent_end
is already aligned, and static mapping will assert CFG_TEE_COHERENT_START
(hence __coherent_start
) is already 4kB page aligned.
Those checks aimed at detecting issues at build time.
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.
Note: i will remove those extra checks.
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.
The assert that checks that . = ALIGN(4096);
works is just silly and and other assert could be done inside the DECLARE_COHERENT_RAM_SECTION
macro instead.
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.
Adding several ASSERT()
inside the macro DECLARE_COHERENT_RAM_SECTION
combined with the 80 chars max line size make the macro not very easy to read. I will move all assertion at the end of the file. Nevertheless, fell free to comment back...
* the secure RAM. | ||
*/ | ||
#ifndef CFG_TEE_COHERENT_SIZE | ||
#define CFG_TEE_COHERENT_SIZE 0 |
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.
Why is this needed?
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.
Default: no coherent RAM => null sized.
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.
May I suggest CFG_TEE_COHERENT_SIZE ?= 0
in core/arch/arm/arm.mk
instead?
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.
Good idea. Thanks.
lib/libutils/ext/include/compiler.h
Outdated
@@ -54,6 +54,7 @@ | |||
#define __rodata_unpaged __section(".rodata.__unpaged") | |||
#define __early_bss __section(".early_bss") | |||
#define __noprof __attribute__((no_instrument_function)) | |||
#define __coherent __attribute__((coherent_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.
My google karma is failing me, do you have a link to that attribute?
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.
hmmm, will fix to __section(".coherent_ram")
. Thanks.
core/arch/arm/kernel/kern.ld.S
Outdated
. = CFG_TEE_COHERENT_START; \ | ||
.coherent (NOLOAD) : { \ | ||
__coherent_start = .; \ | ||
KEEP(*(coherent_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.
All the other sections starts with a "." can't we have the same here?
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.
my fault. i'll fix (see also comments on attribute __coherent_ram
).
core/arch/arm/kernel/kern.ld.S
Outdated
* Start coherent RAM with a platform specific structure. | ||
* Generic data to be place in coherent RAM can use attribute "coherent_ram". | ||
*/ | ||
#define DECLARE_COHERENT_RAM_SECTION \ |
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.
To add some flexibility regarding the location of the coherent RAM, in case one wants it at a dedicated place. I proposed to possible location: before the load address or after the core reserved vaddr range (virt, no phys only, to fit with pager constraints). Since there were those 2 places, I preferred to use a single macro. You feel it bring complexity ?
core/arch/arm/kernel/kern.ld.S
Outdated
. = CFG_TEE_COHERENT_START; \ | ||
.coherent (NOLOAD) : { \ | ||
__coherent_start = .; \ | ||
KEEP(*(coherent_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.
my fault. i'll fix (see also comments on attribute __coherent_ram
).
core/arch/arm/kernel/kern.ld.S
Outdated
@@ -53,9 +53,27 @@ | |||
OUTPUT_FORMAT(CFG_KERN_LINKER_FORMAT) | |||
OUTPUT_ARCH(CFG_KERN_LINKER_ARCH) | |||
|
|||
/* | |||
* Start coherent RAM with a platform specific structure. |
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.
Oups, this comment is related to the libpsci integ. I will remove it.
core/arch/arm/kernel/kern.ld.S
Outdated
|
||
#if defined(CFG_TEE_COHERENT_START) | ||
ASSERT((__coherent_start & (4096 - 1)) == 0, "Coherent RAM start is not 4Kbyte aligned") | ||
ASSERT((__coherent_end & (4096 - 1)) == 0, "Coherent RAM end is not 4Kbyte aligned") |
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.
This is extra checks. Actually __coherent_end
is already aligned, and static mapping will assert CFG_TEE_COHERENT_START
(hence __coherent_start
) is already 4kB page aligned.
Those checks aimed at detecting issues at build time.
* the secure RAM. | ||
*/ | ||
#ifndef CFG_TEE_COHERENT_SIZE | ||
#define CFG_TEE_COHERENT_SIZE 0 |
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.
Default: no coherent RAM => null sized.
lib/libutils/ext/include/compiler.h
Outdated
@@ -54,6 +54,7 @@ | |||
#define __rodata_unpaged __section(".rodata.__unpaged") | |||
#define __early_bss __section(".early_bss") | |||
#define __noprof __attribute__((no_instrument_function)) | |||
#define __coherent __attribute__((coherent_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.
hmmm, will fix to __section(".coherent_ram")
. Thanks.
core/arch/arm/kernel/kern.ld.S
Outdated
#if CFG_TEE_COHERENT_SIZE | ||
ASSERT(!(__coherent_start & (4096 - 1)), | ||
"Coherent memory start alignment"); | ||
ASSERT(!(CFG_TEE_COHERENT_SIZE & (4096 - 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.
This and the following assert are useless, they are just checking that . = ALIGN(4096)
works.
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.
The test above explicitly notifies developer that the coherent size follows the mapping constraint. Otherwise, this will be detected only a runtime. I found this explicit trace more convenient, but I agree it is redundant with the runtime assertions/panics.
Test below does not check the alignment but the size. In case one defines CFG_TEE_COHERENT_SIZE to a value below the effective coherent section computed at link stage, the coherent might the only partially mapped and only run time data abort could detect it.
core/arch/arm/kernel/kern.ld.S
Outdated
ASSERT((__coherent_start & (4096 - 1)) == 0, "Coherent RAM start is not 4Kbyte aligned") | ||
ASSERT((__coherent_end & (4096 - 1)) == 0, "Coherent RAM end is not 4Kbyte aligned") | ||
#if CFG_TEE_COHERENT_SIZE | ||
ASSERT(!(__coherent_start & (4096 - 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.
I don't see the problem with wrapping this inside the DECLARE_COHERENT_RAM_SECTION
macros as:
ASSERT(!(__coherent_start & (4096 - 1)), \
"Coherent memory start alignment"); \
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.
An assertion in the macro could be ok, but the 3 make the macro a bit ugly (in my opinion).
But ok, I will move them (at least those really required) inside the macro.
@@ -222,7 +222,6 @@ | |||
#define GICD_OFFSET 0 | |||
#define GICC_OFFSET 0x10000 | |||
|
|||
|
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 change, to be discarded.
Since the overall changes are quite small, I will rebase and squash the proposed commit to get a (hopefully) nice travis status and ease later reviews. |
thanks travis! i missed generic cases: must fix platform not defining any |
Rebased and adapted (on top of recently merged #1459). (edited) another info on updated content: coherent memory must be inside the physical TEE_RAM. This eases the flat mapping layout. |
a37b63f
to
07e539a
Compare
rebased. |
Looks like this needs some rework? |
Yes, this deserves a rebase. Latest optee_os already defines coherent memory to maps rwx ram. What's needs to be rebased:
|
07e539a
to
4e76f88
Compare
Rebased.
|
core/arch/arm/mm/core_mmu.c
Outdated
* To speed up scans of the memory_map table, it is sorted, moving | ||
* all small-page mapped area before the pgdir mapped areas. | ||
*/ | ||
qsort(memory_map, last, sizeof(struct tee_mmap_region), |
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 doubt this makes much difference, average performance of quicksort is O(n * log(n))
(worst case O(n * n)
) while doing without sorting here worst case is O(2 * n)
or O(3 * n)
.
I'd category this as doubtful micro optimization.
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 see your point. I will remove this sorting.
core/arch/arm/mm/core_mmu.c
Outdated
|
||
#ifdef CFG_WITH_LPAE | ||
/* LPAE: 1 pgdir for protection, user memory located above the core */ | ||
va = CORE_MMU_PGDIR_SIZE; |
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 start to map the rest at 2 MiB regardless of where flat mapped range is (it could even conflict with this)?
More efficient use of level 2 tables would be to keep in same level 2 table as used above, it's 1 GiB so there should be plenty of space.
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 start to map the rest at 2 MiB regardless of where flat mapped range is (it could even conflict with this)?
The loop right after this init skips the virtual range already used to assigned virtual locations. This virtual area is expected to be fully cover the current value of vaspace_start
/vaspace_size
.
More efficient use of level 2 tables would be to keep in same level 2 table as used above, it's 1 GiB so there should be plenty of space.
Agree. I will use the full 1GB around the flat map area to reuse the level2 xlat table.
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.
Comment addressed
- Change implementation comments (simplify)
- Found/fix issue (see commit "[fix] ...").
- Cleanup: remove unused functions
core/arch/arm/mm/core_mmu.c
Outdated
#ifdef CFG_TEE_COHERENT_START | ||
register_phys_mem(MEM_AREA_TEE_COHERENT, CFG_TEE_COHERENT_START, | ||
CFG_TEE_COHERENT_SIZE); | ||
#endif |
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.
This should not be in commit "core: map registered coherent memory".
It should be in "core: linkable coherent memory section from CFG_TEE_COHERENT_START" which introduces CFG_TEE_COHERENT_START
/_SIZE
.
core/arch/arm/mm/core_mmu.c
Outdated
init_smallpage_map(memory_map, vstart, coherent_start, false); | ||
vstart = coherent_start + coherent_size; | ||
} | ||
|
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.
Looks like the one and the one below could be merged...
56d5f2d
to
f361ac8
Compare
Rebased (conflict to fix). |
Please squash the commits as you'd like to have them merged and I'll take a final look. |
f361ac8
to
00f75ad
Compare
Thus we may rather call this |
It think the define is good as it is. |
@etienne-lms I did not follow this p-r. I'll give a check and test with #1729. |
Thanks |
@etienne-lms
|
Thanks for the test. Indeed, this p-r expects only 1 coherent memory. This is because the 'coherent' memory handler by this p-r is 'flat-mapped coherent memory' and each adds a constraints on the virtual mapping layout (discontinuous TEE_RAM and coherent memory location), hence only 1 coherent mem is supported. I think this p-r should not use COHERENT labelling, rather something like FLATMAP_COHERENT. Or maybe: we can rename the current COHERENT into UNCACHED_RWX; platforms needing RAM mapped uncached can use the IO_RAM type; and we define COHERENT as flat mapped uncached read/write memory. |
+1 for |
00f75ad
to
ed51c4a
Compare
With this change, the small-page mapped areas are mapped around the flat map areas to reuse the translation tables. If optee is memory constrained running with the pager enables, the small page tables use the map the flat map area will be use for other small page mapped entries. With this change, the virtual memory layout is design in 4 steps: - define virtual location of flat mapped areas. - define virtual location of small-page mapped areas around flat mapped areas (likely to already allocate xlat tables). - define virtual location of pgdir mapped areas from bottom range to top range, skipping the already assigned virtual locations. List of the constraints on virtual memory layout: - optee_os includes some flat mapped sections. - already leave the first pgdir entry unmapped for protection. - Lpae mappings will locate userland virtual memory above core virtual memory. - Non-lpae mappings must locate userland virtual memory below core virtual memory (ttbr0/ttbr1 referencing) This change prepares mapping support of a coherent memory which will add new constraints of the virtual memory layout. Function init_smallpage_map() is used to assign virtual location for small page regions inside a given virtual address range. This clarifies sequence building the virtual mapping layout. Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
Physical memory areas registered with register_phys_mem() with the type attribute MEM_AREA_TEE_COHERENT_FLAT are mapped by the generic code with virtual address set to the the physical address value. Generic code expects at most one such coherent flat mapped memory area. Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
Platform can define COHERENT_FLATMAP_BASE/SIZE have request generic code to define a flat mapped coherent memory area that gets linked in optee_os core. One may use the __coherent attribute to located data inside the coherent memory. CFG_TEE_COHERENT_START/_SIZE can be located before or after the optee_os reserved range that covers flat mapped areas and pager vaspace. The coherent memory defined by CFG_TEE_COHERENT_START/_SIZE is automatically to the core static mapping directive. Update plat-sunxi/kernel.ld.S accordingly. Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
Fix string size to match "COHERENT_FLAT" and "PAGER_VASPACE". Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
Closing deprecate p-r. |
This change enables secure coherent memory support and allow platform vexpress-qemu_virt to easily define a coherent memory at top of the secure RAM.