platform: set up syscall trap on SMP application processors#2370
platform: set up syscall trap on SMP application processors#2370uadhran wants to merge 2 commits into
Conversation
Pull syscall MSR setup into init_syscall_trap() and call it from both the BSP libc init path and revenant_main so AP cores get a handler instead of nothing. Related: includeos#2358
MSR setup for the syscall entry point lives next to __syscall_entry and syscall_entry.cpp. Platform code only calls x86::init_syscall_trap().
|
Moved |
| @@ -0,0 +1,5 @@ | |||
| #pragma once | |||
|
|
|||
| namespace x86 { | |||
There was a problem hiding this comment.
Do we want this under x86 only? I think syscalls can exist on any architecture, and thus it would make sense to declare this for all architectures, and require architectures for which it makes no sense to initialize syscalls to stub an empty implementation.
Maybe @elstr-512 has an opinion on this?
There was a problem hiding this comment.
Fair question. I kept this under x86:: for this PR because the implementation is x86_64-specific (IA32_STAR / IA32_LSTAR) and IncludeOS only runs the syscall path on x86 PC today.
A broader shape would be something like os::init_syscall_trap() in a neutral header with arch stubs (no-op on aarch64, real impl on x86_64). That's a nice cleanup but a separate refactor from "APs also get the MSRs."
I've left it as-is here to keep the diff small. Happy to follow up with an arch-neutral API if that's the direction you and @elstr-512 prefer — would you want that as a follow-up PR after this merges?
| #warning Classical syscall interface missing for 32-bit | ||
| #endif | ||
| KDEBUG("* Initialize syscall trap\n"); | ||
| x86::init_syscall_trap(); |
There was a problem hiding this comment.
I'm still wondering if this should be part of libc initialization. I wouldn't do such a change in this PR fwiw, but I would consider opening an issue to discuss it further, maybe.
There was a problem hiding this comment.
Agree — hooking syscall MSR setup off the libc init path is a bit odd, and I wouldn't want to reshuffle boot ordering inside this PR.
I'll open an issue to discuss when/where syscall trap init should live (e.g. earlier platform bring-up vs libc/runtime init, and how that interacts with AP startup). This PR only extracts the MSR programming and ensures BSP + APs both call the same helper; it doesn't change when the BSP first calls it.
Thanks for calling that out.
|
This is much cleaner than what it used to be, thanks! All integration tests are passing too. I kinda wish we could add some integration tests to verify syscalls fail to trap before they're initialized, and make sure they're trapped after initialization. Otherwise, see inlined comments. |
|
Good point — a test that exercises syscall trapping before vs after I'd rather not fold that into this PR since it's already scoped to AP MSR setup. I'll open a follow-up issue for an integration test (or small test service) that verifies:
Happy to take that on once this lands, unless you'd prefer it block merge. |
|
Veri clean. |
3 participants
Only CPU 0 had the syscall MSRs configured. This pulls that setup into
x86::init_syscall_trap()and calls it from both the BSP libc init path andrevenant_main, so application processors get a trap instead of nothing. The handler still panics — syscalls aren't implemented.Changes:
src/arch/x86_64/syscall_trap.cpp— programsIA32_STARandIA32_LSTARapi/arch/x86/syscall.hpp— declarationsrc/platform/x86_pc/init_libc.cpp— BSP callsinit_syscall_trap()src/platform/x86_pc/apic_revenant.cpp— each AP callsinit_syscall_trap()Tested:
nix-build unittests.nix— 85/85 passed (Nix 2.34.7, Linux).Split from #2367 per review feedback.
Closes: #2358