Repository navigation
userlib: Introduce Arch trait - #2705
Conversation
|
For reviewers, I've added some "big picture" notes in #2706 on how this PR fits into the bigger picture. |
ec52c70 to
a7136fd
Compare
6ba5f69 to
83c6c83
Compare
|
About to do the renames, but I repeated the analysis I did in #2704 (comment), but between james@fool:~/oxide ls -al *.bin
-rw-r--r-- 1 james staff 748288 9 Oct 16:26 arch-trait.bin
-rw-r--r-- 1 james staff 748288 9 Oct 16:24 master.bin
-rw-r--r-- 1 james staff 748288 9 Oct 17:58 userlib-trait.bin
james@fool:~/oxide xxd ./arch-trait.bin > ./arch-trait.hex
james@fool:~/oxide xxd ./userlib-trait.bin > ./userlib-trait.hex
james@fool:~/oxide diff -u ./arch-trait.hex ./userlib-trait.hex
--- ./arch-trait.hex 2026-10-09 17:59:59
+++ ./userlib-trait.hex 2026-10-09 18:00:12
@@ -46751,9 +46751,9 @@
000b69e0: ffff ffff ffff ffff ffff ffff ffff ffff ................
000b69f0: ffff ffff ffff ffff ffff ffff ffff ffff ................
000b6a00: 5e00 b0ca 4749 5443 2800 0000 b8d8 0c0c ^...GITC(.......
-000b6a10: 3732 3338 3235 6262 3230 6365 3830 3537 723825bb20ce8057
-000b6a20: 3035 3261 3137 3432 3034 6135 6132 6534 052a174204a5a2e4
-000b6a30: 3365 3933 3565 6238 b302 eb84 424f 5244 3e935eb8....BORD
+000b6a10: 3833 6336 6338 3334 3264 3563 3964 3065 83c6c8342d5c9d0e
+000b6a20: 3239 3538 6432 3830 3131 3630 3862 6635 2958d28011608bf5
+000b6a30: 3961 3565 6237 3534 5ea8 00f2 424f 5244 9a5eb754^...BORD
000b6a40: 0700 0000 e67f 1fca 636f 736d 6f2d 6200 ........cosmo-b.
000b6a50: dc94 eabb 4e41 4d45 0700 0000 fac4 843c ....NAME.......<
000b6a60: 636f 736d 6f2d 6200 dc94 eabb ffff ffff cosmo-b.........
james@fool:~/oxide edit, and post-renames: james@fool:~/oxide xxd ./userlib-trait-2.bin > ./userlib-trait-2.hex
james@fool:~/oxide diff -u ./arch-trait.hex ./userlib-trait-2.hex
--- ./arch-trait.hex 2026-10-09 17:59:59
+++ ./userlib-trait-2.hex 2026-10-09 18:28:00
@@ -46751,9 +46751,9 @@
000b69e0: ffff ffff ffff ffff ffff ffff ffff ffff ................
000b69f0: ffff ffff ffff ffff ffff ffff ffff ffff ................
000b6a00: 5e00 b0ca 4749 5443 2800 0000 b8d8 0c0c ^...GITC(.......
-000b6a10: 3732 3338 3235 6262 3230 6365 3830 3537 723825bb20ce8057
-000b6a20: 3035 3261 3137 3432 3034 6135 6132 6534 052a174204a5a2e4
-000b6a30: 3365 3933 3565 6238 b302 eb84 424f 5244 3e935eb8....BORD
+000b6a10: 3864 3164 3836 3035 3638 3364 3838 6265 8d1d8605683d88be
+000b6a20: 3331 6663 6432 3564 3062 6264 3563 3939 31fcd25d0bbd5c99
+000b6a30: 3964 6335 3363 6538 35b8 864b 424f 5244 9dc53ce85..KBORD
000b6a40: 0700 0000 e67f 1fca 636f 736d 6f2d 6200 ........cosmo-b.
000b6a50: dc94 eabb 4e41 4d45 0700 0000 fac4 843c ....NAME.......<
000b6a60: 636f 736d 6f2d 6200 dc94 eabb ffff ffff cosmo-b......... |
8d1d860 to
55ce3b1
Compare
55ce3b1 to
11b6acb
Compare
|
One more check post-rebase: james@fool:~/oxide/hubris git:(james/userlib-trait) cp target/cosmo-b/dist/default/final.bin ../userlib-trait-3.bin
james@fool:~/oxide/hubris git:(james/userlib-trait) cd ..
james@fool:~/oxide xxd ./userlib-trait-3.bin > ./userlib-trait-3.hex
james@fool:~/oxide diff -u ./arch-trait.hex ./userlib-trait-3.hex
--- ./arch-trait.hex 2026-10-09 17:59:59
+++ ./userlib-trait-3.hex 2026-10-09 21:56:34
@@ -46751,9 +46751,9 @@
000b69e0: ffff ffff ffff ffff ffff ffff ffff ffff ................
000b69f0: ffff ffff ffff ffff ffff ffff ffff ffff ................
000b6a00: 5e00 b0ca 4749 5443 2800 0000 b8d8 0c0c ^...GITC(.......
-000b6a10: 3732 3338 3235 6262 3230 6365 3830 3537 723825bb20ce8057
-000b6a20: 3035 3261 3137 3432 3034 6135 6132 6534 052a174204a5a2e4
-000b6a30: 3365 3933 3565 6238 b302 eb84 424f 5244 3e935eb8....BORD
+000b6a10: 3131 6236 6163 6230 6433 3966 6264 3035 11b6acb0d39fbd05
+000b6a20: 3264 3666 6335 3239 6139 3434 3939 6633 2d6fc529a94499f3
+000b6a30: 3965 3463 3539 6362 d491 3ee1 424f 5244 9e4c59cb..>.BORD
000b6a40: 0700 0000 e67f 1fca 636f 736d 6f2d 6200 ........cosmo-b.
000b6a50: dc94 eabb 4e41 4d45 0700 0000 fac4 843c ....NAME.......<
000b6a60: 636f 736d 6f2d 6200 dc94 eabb ffff ffff cosmo-b......... |
| cfg_if::cfg_if! { | ||
| if #[cfg(armv6m)] { |
There was a problem hiding this comment.
part of me now wonders if it might make sense to just have separate Arch impls for v6m and v7m/v8m, and select between them based on cfgs, so that we don't have to duplicate the cfg_if! in every single stub...but, on the other hand, this way we can just copy the previous code more or less unchanged, so it may not be worth it, I dunno...
There was a problem hiding this comment.
Oh, that's actually a pretty good idea. I might want to do that in a follow-up PR, just to keep the diff reasonable? But I think overall it's much more of the "vibe", moving towards something like:
- arch/
- arm_m/
- mod.rs (common parts)
- armv6m.rs
- armv7m.rs
- armv8m.rs
There was a problem hiding this comment.
Yeah, I think doing it in a follow up PR is entirely reasonable, just figured it was worth mentioning. AFAICT it's just the syscall stubs that currently differ here, and
v7m and v8m use the same assembly. I'd kind of prefer to have one version of the code for v[78]m, rather than duplicating it, until we actually need to add code that differs between those versions... I'm always a bit leery about introducing opportunities for identical code to drift apart unless the drift is on purpose.
| let msg = unsafe { pw.buf.get_unchecked(..pw.pos) }; | ||
|
|
||
| // Pass it to kernel. | ||
| ArmM::panic(msg) |
There was a problem hiding this comment.
most of the code here seems to not actually be arch-specific...i wonder if there's some way to have the panic handler not be defined in the arch specific module and just call into the architecture-specific panic code via the ArchImpl renaming import in lib.rs?
There was a problem hiding this comment.
So, yes and no. I think the reason this ended up like this is mostly that on std/native-hosted we may not want a custom panic handler at all, so it made more sense to make this at least no_std+cortex-m specific.
That being said, if we end up supporting rv32 or something else in the future, I think we could definitely refactor this out.
There was a problem hiding this comment.
Mm, yeah, that's fair. I guess seeing this kinda makes me question why we even have a separate function on the Arch trait that is distinct from the rest of the panic handling machinery, but I suppose that allows you to have code that calls the panic syscall directly without going through the actual panic handler (though I don't know if that exists or if it's even something we would want). I think it's fine to leave this as-is today and factor out the non-arch-specific parts if and when it becomes necessary to add another architecture.
Co-authored-by: Eliza Weisman <eliza@elizas.website>
hawkw
left a comment
There was a problem hiding this comment.
Okay, this seems good to me, though perhaps you'll also want to wait for an ack from Laura?
|
Talked to Laura, she's fine with this going through and took a pass over this earlier. Thank you both for the reviews! |
This is the third PR in a series to be able to potentially run the kernal and tasks on a 64-bit host for testing purposes. This is currently based on #2704 (and #2703).
This PR introduces an
Archtrait foruserlib, similar to theArchtrait introduced for the kernel in #2704.This follows the pattern we've started using for BSP impls lately: We define a trait, each BSP (or Arch in this case) implements it, and we expose the One Selected Impl using cfgs as a commonly named type (here: Current) which the rest of the crate uses.
Like #2703, this one might be worth review commit-by-commit, as the total diff is a bit large, but the individual commits are each pretty straightforward.