Fix for the Morello Coprocess Switcher for Purecap Kernels #1791
+39
−33
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The least significant bit of the capability that points to the switcher needs to be set. Otherwise, we leave C64 mode after jumping into the switcher with BLRS. The switcher code of purecap kernels uses capability instructions and attempting to execute these outside C64 mode results in an tag-violation error. This commit also fixes two uses of non-capability instructions where the switcher must use capabilties.
Cocall and coaccept expect the in- and output-buffers to be aligned to 16 bytes. Asserts that check if inlen and outlen are also aligned fail in some test cases. However, these checks seem unnecessary because both are function parameters.
jrtc27
reviewed
Aug 11, 2023
sys/cheri/cheri_colocation.c
Outdated
| * by purecap kernels uses capabilities and so fails if it is not | ||
| * executed in C64 mode. | ||
| */ | ||
| codecap_addr = cheri_getaddress(codecap); |
There was a problem hiding this comment.
- Use cheri_capmode, it’s target-independent
- Shouldn’t switcher_code_cap do this?
- This breaks for the vm_cocall_codecap case, because you add rather than or (IIRC cheri_capmode is idempotent) and so will double capmode
- Why is this based on kernel ABI? Surely it should always be purecap code?
paulmetzger
changed the title
Set the cap mode bit in the code capability for the switcher code in switcher_code_cap() like Jess suggested. cheri_capmode() wasn't called in switcher_code_cap() when __aarch64__ is defined.
jrtc27
requested changes
Jan 10, 2024
| @@ -351,7 +351,11 @@ colocation_unborrow(struct thread *td) | |||
| if (!have_scb) | |||
| return; | |||
|
|
|||
| #ifdef __CHERI_PURE_CAPABILITY__ | |||
| peertd = scb.scb_borrower_td; | |||
| KASSERT(peertd != td, | ||
| ("%s: peertd %p == td %p\n", __func__, peertd, td)); | ||
| #endif | ||
| KASSERT(peertd != td, ("%s: peertd %p == td %p\n", __func__, peertd, td)); |
| @@ -375,10 +379,13 @@ colocation_unborrow(struct thread *td) | |||
| #error "what architecture is this?" | |||
| #endif | |||
|
|
|||
| #ifdef __CHERI_PURE_CAPABILITY__ | |||
| KASSERT(td == scb.scb_td, ("%s: td %p != scb_td %p\n", __func__, td, (__cheri_fromcap struct thread *)scb.scb_td)); | |||
| @@ -539,7 +546,6 @@ setup_scb(struct thread *td) | |||
| int | |||
| sys__cosetup(struct thread *td, struct _cosetup_args *uap) | |||
| { | |||
|
|
|||
| @@ -556,14 +562,13 @@ switcher_code_cap(struct thread *td, ptraddr_t base, size_t length) | |||
| */ | |||
| codecap = cheri_capability_build_user_rwx(CHERI_CAP_USER_CODE_PERMS, | |||
| base, length, 0); | |||
| #ifndef __aarch64__ | |||
| /* | |||
| * XXX: Somehow makes every attempt at ldr/str in the switcher | |||
| @@ -616,6 +621,7 @@ kern_cosetup(struct thread *td, int what, | |||
| td->td_proc->p_sysent->sv_cocall_base, | |||
| td->td_proc->p_sysent->sv_cocall_len); | |||
| } | |||
|
|
|||
| @@ -634,6 +640,7 @@ kern_cosetup(struct thread *td, int what, | |||
| td->td_proc->p_sysent->sv_coaccept_base, | |||
| td->td_proc->p_sysent->sv_coaccept_len); | |||
| } | |||
|
|
|||
| } | ||
|
|
||
| ATF_TC_WITHOUT_HEAD(cocall); | ||
| ATF_TC_BODY(cocall, tc) | ||
| { | ||
| char *name; | ||
| uint64_t buf; | ||
| uint64_t buf __attribute__((aligned(16))); |
| @@ -110,14 +110,14 @@ wait_for_coregister(void) | |||
| * | |||
| * XXX: You might have been expecting something more clever. | |||
| */ | |||
| usleep(200000); | |||
| usleep(600000); | |||
There was a problem hiding this comment.
Blegh... can we just do something actually correct rather than bodging it more?
| @@ -47,11 +47,11 @@ coaccept(void * __capability * __capability cookiep, | |||
|
|
|||
| if (outbuf != NULL) { | |||
| assert(__builtin_is_aligned(outbuf, sizeof(void * __capability))); | |||
| assert(__builtin_is_aligned(outlen, sizeof(void * __capability))); | |||
| //assert(__builtin_is_aligned(outlen, sizeof(void * __capability))); | |||
There was a problem hiding this comment.
These can't be committed as commented-out code (and certainly not with the style-violating C-style comments)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes the user space switcher for coprocesses that purecap kernels provide on Morello. The least significant bit of the capability that is used to jump to the switcher code wasn't set, which meant that we left C64 mode. Besides that, the switcher used at two points in its code normal registers were it must use capability registers.
I've tested these fixes with the Stevie and Clocks example applications on a Morello board and in Qemu. I also fixed two Kyua test cases.
The cocalls branch, from which this branch is forked, is missing this fix: https://github.com/CTSRD-CHERI/cheribsd/pull/1378/files
I applied manually a fix based on this PR to my local copy of the wip_fix_morello_coproc_switcher_for_purecap_kernels branch to compile a purecap kernel that boots.