Skip to content
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

Large tee ram va size #1669

Merged
merged 10 commits into from
Aug 29, 2017
Merged

Conversation

jenswi-linaro
Copy link
Contributor

Introduces support for tee ram va size larger than the page directory size.

Copy link
Contributor

@etienne-lms etienne-lms left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the split in tiny changes.
Reviewed, looks ok. Only minor comments.
Successfully tested on b2260 (dual c-a9) with few configurations spreading vcore range over 2 mmu sections.

* Most addresses are mapped lineary, try that first if possible.
*/
if (!tee_pager_get_table_info(pa, &ti))
return NULL; /* impossible pa */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor: move this test before the comment which is related to the sequence below.
Also, arg#1 of tee_pager_get_table_info() is a vaddr_t whereas we pass here a paddr_t. This would deserve a cast.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually the comment applies to the call of tee_pager_get_table_info() too.
I'll skip the cast as frivolous casting causes more problems than they solve.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right. ok.

idx = core_mmu_va2idx(&pt->tbl_info, smem);
while (pt <= (pager_tables + ARRAY_SIZE(pager_tables) - 1)) {
while (idx < TBL_NUM_ENTRIES) {
v = core_mmu_idx2va(&pt->tbl_info, idx);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor: v is local to this instructions block.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a matter of taste. In this case I think the code is easier to read with v declared above together with the other local variables.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

idx = core_mmu_va2idx(&pager_tables[n].tbl_info, CFG_TEE_RAM_START);
while (true) {
while (idx < TBL_NUM_ENTRIES) {
v = core_mmu_idx2va(&pager_tables[n].tbl_info, idx);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor: v is local to this instructions block.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a matter of taste. In this case I think the code is easier to read with v declared above together with the other local variables.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

while (true) {
while (idx < TBL_NUM_ENTRIES) {
v = core_mmu_idx2va(&pager_tables[n].tbl_info, idx);
if (v > (CFG_TEE_RAM_START + CFG_TEE_RAM_VA_SIZE))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor: s/>/>=/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I'll fix.

struct pager_table *pt = find_pager_table_may_fail(va);

if (!pt)
panic();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor: i think an assert is enough. Functions that call find_pager_table() will simply page fault when using a null pager table pointer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree


if (smem & SMALL_PAGE_MASK || nbytes & SMALL_PAGE_MASK)
panic("invalid area alignment");

pager_alias_area = mm;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor: as set_alias_area() shall be called only once, at init, maybe we can restore the test on pager_alias_area == NULL, eventually through an assert().

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK

base, size, ti->va_base, tbl_va_size);
panic();
while (s) {
s2 = MIN(CORE_MMU_PGDIR_SIZE - (b & CORE_MMU_PGDIR_MASK), s);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor: s2 is local to this instruction block.

desc |= base_va - mm->va + mm->pa;
}

if (desc)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor: if (desc == UNSET_DESC) (here and above) for consistency ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, that would be something different as UNSET_DESC is defined to -1

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, you are right. Ok for the one above.
For the if (desc) here, I wonder case where desc==UNSET_DESC and attr<0. Minor but still might print a "mapped" trace instead of a "not-mapped" trace. Maybe if (desc != UNSET_DESC && desc).

Copy link
Contributor Author

@jenswi-linaro jenswi-linaro Aug 16, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, it should probably be if (attr >= 0 && desc).
Edit: your test is better, I'll use that instead.

@jenswi-linaro
Copy link
Contributor Author

Update

1 similar comment
@jenswi-linaro
Copy link
Contributor Author

Update

Copy link
Contributor

@etienne-lms etienne-lms left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found 1 remaining case that must be fixed and a potential weakness.

Otherwise, successfully tested on non lpae (using b2260) and lpae (using qemu_armv8) with various secure RAM locations/sizes (with pager enabled of course).

@@ -824,7 +864,8 @@ static void init_mem_map(struct tee_mmap_region *memory_map, size_t num_elems)
/* Map non-flat mapped addresses above flat mapped addresses */
va = ROUNDUP(va + CFG_TEE_RAM_VA_SIZE, CORE_MMU_PGDIR_SIZE);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not very clean how va here is set to the end of the pager va space. Both "pager vaspace" end address and va here are computed using the same sequence, so that's fine now but not clean in the long term.
Maybe add_pager_vaspace() should update end so it can be reused here to define the virtual base address for the non-flat-mapped areas.

@jenswi-linaro
Copy link
Contributor Author

Update, did the add_pager_vaspace() changes slightly different.

@@ -837,12 +843,14 @@ static void init_mem_map(struct tee_mmap_region *memory_map, size_t num_elems)
assert(va >= CFG_TEE_RAM_START);
assert(end <= CFG_TEE_RAM_START + CFG_TEE_RAM_VA_SIZE);

add_pager_vaspace(memory_map, num_elems, va, end, &last);
add_pager_vaspace(memory_map, num_elems, va, end, &last,
&next_pgdir_va);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think here, if pager is enabled, it is mandatory that the pager vmem space is fully reserved to pager. Hence, we should maybe add va = ROUNDDOWN(va, CORE_MMU_PGDIR_SIZE);, the same way we now have the pager vmem end provided through next_pgdir_va.

Maybe more simple to have add_pager_vaspace(memory_map, num_elems, &va, &end, &last); and the function to update va and end content if the virtual memory area reserved to TEE_RAM management must be enlarged due to pager constraint.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, that's not needed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why ? What insures the first area mapped right below TEE_RAM (case mapping setup with "tee ram at top") is pgdir aligned ? At this stage, va may not be pgdir aligned.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After few reading and test, I see the issue i'm talking about is not specific to this P-R. So maybe it should be treated outside this P-R.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason I don't want to enforce pgdir alignment of va is primary for LPAE. It seems wasteful to forbid usage of otherwise unused entries.

#ifdef CFG_WITH_PAGER
size_t size = CFG_TEE_RAM_VA_SIZE - (end - begin);
size_t size = CFG_TEE_RAM_VA_SIZE - (end - begin_pgdir);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not ok. size is expected to cover from begin, not begin_pgdir, and next_pgdir_va should be end + size eventually rounded up to the pgdir size. With such changes, we would end to the original code (the one before this [review] patch). I even think the final roundup to pgdir size is useless.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please propose what you have in mind and we'll take it from there.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please look at jenswi-linaro#2.


if (core_mmu_place_tee_ram_at_top(va)) {
/* Map non-flat mapped addresses below flat mapped addresses */
for (map = memory_map; !core_mmap_is_end_of_table(map); map++) {
if (map_is_flat_mapped(map))
if (map_is_flat_mapped(map) ||
map->type == MEM_AREA_PAGER_VASPACE)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we could replace this with if (map->va) continue; so that only the areas without an assigned virtual address get one assigned.

mmap[pos].region_size = SMALL_PAGE_SIZE;
mmap[pos].attr = core_mmu_type_to_attr(MEM_AREA_PAGER_VASPACE);

*end += size;
Copy link
Contributor Author

@jenswi-linaro jenswi-linaro Aug 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be updated regardless if pager is enabled or not.

Edit: Maybe not strictly necessary as no code depends on that part of the pgdir being unused.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with the edited comment: when pager is disable, CFG_TEE_RAM_VA_SIZE is meaningless.
Hmm... maybe usefull also for the kasan ?

However, some platform do define CFG_TEE_RAM_VA_SIZE and some piece of code use it. Ok to not condition the sequence here and postponed cleaning of CFG_TEE_RAM_VA_SIZE to later if needed.

@jenswi-linaro
Copy link
Contributor Author

What's next step here, squash and rebase for a final review?

@etienne-lms
Copy link
Contributor

I would say remove CFG_WITH_PAGER condition from add_pager_vaspace and sqash rebase.
Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org> for the current content.
If have successfully tested a rebased version of it on b2260 and qemu_armv8 (with various vcore ranges).

@@ -120,7 +120,7 @@

#define CFG_TEE_CORE_NB_CORE 8

#define CFG_TEE_RAM_VA_SIZE (1024 * 1024)
#define CFG_TEE_RAM_VA_SIZE (2 * 1024 * 1024)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: the memory map above (comment block) needs fixing.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, i don't think this change is required. 1MB is fine for current. Increase to 2MB should not hurt (but the layout description pointed by @jforissier) and allows to test the change.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/0x3F10_0000/0x3F20_0000/

I think we all agree 2M is a better default value if only for exercising the new code.
Acked-by: Jerome Forissier <jerome.forissier@linaro.org>

@jforissier
Copy link
Contributor

@jenswi-linaro yes I think so, but I'll let @etienne-lms speak up since he has been doing all the review work.

Prior to this patch was tee_mm_vcore initialized to cover the complete
page directories covering TEE_RAM. With this patch tee_mm_vcore will
only cover TEE_RAM in order to avoid returning unexpected addresses when
allocating.

Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org>
Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
Adds the define CORE_MMU_PGDIR_LEVEL which indicates the level used for
page directories.

Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org>
Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
Hides tee_pager_tbl_info and provides new needed functions.

Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org>
Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
All failures in tee_pager_add_core_area() are fatal. Replaces return
code with void and panics on errors instead.

Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org>
Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
Adds MEM_AREA_PAGER_VASPACE which is used to create empty translation
tables as needed for the pager.

Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org>
Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
Moves some internal functions inside the pager code to prepare for a
future commit.

Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org>
Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
Changes tee_pager_release_phys() to handle freeing a range of pages spanning
multiple areas.

Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org>
Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
Deals with CFG_TEE_RAM_VA_SIZE > CORE_MMU_PGDIR_SIZE. This is a special
problem as the pages managed by the pager then spans several translation
tables.

Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org>
Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
Increases CFG_TEE_RAM_VA_SIZE to 2 MiB for the plat-vexpress platforms.

Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org>
Tested-by: Jens Wiklander <jens.wiklander@linaro.org> (QEMU v7)
Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
@jenswi-linaro
Copy link
Contributor Author

Rebased and squashed. Tag applied to all commits but the last (plat-hikey) as @etienne-lms had some comment on that.

@jenswi-linaro
Copy link
Contributor Author

@jforissier, did I get that right?

@jforissier
Copy link
Contributor

Yes ;)

@jenswi-linaro
Copy link
Contributor Author

Update

@jforissier
Copy link
Contributor

For "core: plat-hikey: increase CFG_TEE_RAM_VA_SIZE":
Acked-by: Jerome Forissier <jerome.forissier@linaro.org>

Increases CFG_TEE_RAM_VA_SIZE to 2 MiB for the plat-hikey platform.

Acked-by: Jerome Forissier <jerome.forissier@linaro.org>
Tested-by: Jens Wiklander <jens.wiklander@linaro.org> (Hikey)
Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
@jenswi-linaro
Copy link
Contributor Author

Tag applied

@jforissier jforissier merged commit 8446c47 into OP-TEE:master Aug 29, 2017
@jenswi-linaro jenswi-linaro deleted the large_tee_ram_va_size branch August 29, 2017 13:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants