Expose uVisor APIs via function table instead of SVC IDs #370
Conversation
api/src/api.c
Outdated
| /******************************** page heap ********************************/ | ||
| int uvisor_page_malloc(UvisorPageTable * const table) | ||
| { | ||
| return uvisor_api.page.malloc(table); |
There was a problem hiding this comment.
Why this level of indirection? There are 2 important design requirements which I'm not sure we can satisfy at the same time with this approach:
- All entry points in one file.
- The one file shows immediately which API will be run in P, NP, S, NS mode.
1. is satisfied here, but then the check on "where are you coming from" will be scattered throughout the other several files, compromising 2..
Also, we already discussed this: By using this level of indirection the boundary P/NP is shifted. It is more natural that the uvisor-lib runs the very last instruction that is executed in NP. The uvisor-core instead would run the very first instruction which is executed in P mode. In that sense the svc should be called from here (lib) and not from the core.
There was a problem hiding this comment.
A benefit of uVisor core doing the transition is that interface between uVisor core and all other software is just a simple table of functions that are callable via the ordinary AAPCS. uVisor core is then able to later decide how the transitions should work and when, which is of benefit when we are still trying out different models of transitioning.
Also note that not all entry points into uVisor core will perform a transition: pool queue code is shared between uVisor core and other software, but lives in the uVisor core binary because uVisor core also runs the code and we want all code that runs privileged to live in the uVisor core binary.
2 sounds like a "nice to have" and not a "must have", but I don't see the basic approach we are taking here as antithetical to 2; we could still do 2, but where an API runs is platform dependent (v7 vs v8) and awkward to annotate at this level of abstraction.
niklarm
force-pushed
the
feature/capi
branch
3 times, most recently
from
81519bc to
dabd5fa
Compare
core/system/src/api.c
Outdated
| } | ||
|
|
||
| /* Use this one weird trick to map functions to SVC IDs. */ | ||
| #define UVISOR_SVC_ID_debug_register_driver UVISOR_SVC_ID_DEBUG_REGISTER_BOX |
There was a problem hiding this comment.
Can we just define the IDs out of the function names? See my old commit:
- https://github.com/ARMmbed/uvisor-v8m/commit/083df8c6c791e64be9bcf5627559c97fffa75443 "SVC: Use function names as IDs"
There was a problem hiding this comment.
The reason why I didn't do that was that if you change the function name, you also have to change the SVC IDs in many places and that would spam this PR a little.
Would you like me to add this as a separate commit though?
core/system/src/api.c
Outdated
| "svc %[svc_id] \n" \ | ||
| "bx lr \n" \ | ||
| :: [svc_id] "I" (UVISOR_SVC_ID_ ## fn_name & 0xFF) \ | ||
| :: [svc_id] "I" UVISOR_SVC_ID_GET(fn_target) \ |
There was a problem hiding this comment.
Could you re-add the parentheses here?
|
The uvisor build and test jobs are misleading. The batch script that does uvisor build does not make Jenkins realise that make has failed. Will fix it soon. |
| LENGTH = UVISOR_SRAM_LENGTH_USED - STACK_SIZE | ||
| STACK (rw) : ORIGIN = (SRAM_ORIGIN + SRAM_OFFSET + UVISOR_SRAM_LENGTH_USED - STACK_SIZE), | ||
| LENGTH = STACK_SIZE | ||
| LENGTH = UVISOR_SRAM_LENGTH_USED - STACK_SIZE - STACK_GUARD_BAND |
There was a problem hiding this comment.
Does it make sense to group changes to the STACK_GUARD_BAND in this commit, which is otherwise focused on replacing export_table.c with api.c?
| LENGTH = STACK_SIZE | ||
| LENGTH = UVISOR_SRAM_LENGTH_USED - STACK_SIZE - STACK_GUARD_BAND | ||
| STACK (rw) : ORIGIN = ORIGIN(RAM) + LENGTH(RAM), | ||
| LENGTH = UVISOR_SRAM_LENGTH_PROTECTED - LENGTH(RAM) |
There was a problem hiding this comment.
These changes also seem to want grouping into a separate commit that has a commit description that explains these changes more explicitly.
The only changes to the linker script that makes sense to be grouped with the export_table.c to api.c transition commit are the ones involved in replacing the export_table with uvisor_api; otherwise, the diff is noisy.
The moving of the stack into its own section is certainly a logical change deserving of its own separate commit.
There was a problem hiding this comment.
Moved into separate commit.
niklarm
force-pushed
the
feature/capi
branch
2 times, most recently
from
f3405be to
6354ac9
Compare
For clarity, this moves the functions in export_table.c into three separate files.
Cleanup unused sections and use better section size computation.
Instead of calling SVCs in the application directly, the API function in the uvisor_api table is called, which jumps into uVisors binary and performs the SVC. This way no synchronization of SVC IDs between uVisor and the application needs to be performed. Note that this also speeds up calling the pool queue APIs, since the position of the export table needs not be manually computed anymore.
5 participants
This moves the export table at the beginning of the uVisor binary and allows the application to just call a uVisor function, which then executes a thunk inside uVisor binary which does the SVC with the right ID.
Benchmark API is also removed.
TODO: Porting guide needs to be updated to reflect an additional load:
cc @AlessandroA @Patater @meriac