arch/aarch64: fix VBAR_EL2 stale comment + hardcode HVC PSCI conduit
FABRIC-3.md §IV.3 item 7's code audit found two more findings; fixed both
per direct instruction naming them specifically.
Comments only, no behavior change:
interrupts.c's arch_interrupts_init() doc comment (and its matching
runtime EL2 console message) claimed VBAR_EL2 was "a known gap... not yet
wired." Checked against isr.S's aarch64_install_vectors() and found that
claim stale: the assembly already branches on aarch64_current_el() and
writes vbar_el2/vbar_el1 correctly, threading the same answer through
el2_mode_flag for the IRQ trampoline's ELR/SPSR selection too. There was
no gap to close -- only the comment was wrong. Also fixed a same-vintage
one-word staleness in arch.c's aarch64_install_vectors extern comment
("installs VBAR_EL1" -> EL-aware), caught while touching this.
Real behavior change, gated to preserve existing QEMU behavior:
arch_cold_reset() hardcoded the PSCI conduit to HVC, a documented
QEMU-specific workaround (QEMU's AAVMF has no genuine EL3 to answer SMC).
Real Pi 5 hardware's ATF means EL3 exists there; confirmed further this
pass that bcm2712.dtsi's own /psci node declares method="smc" directly,
not just inferred from ATF's presence.
New aarch64_psci_conduit_init(dtb) in arch.c: fdt_valid(dtb) ->
fdt_find_node_by_compatible(dtb, "arm,psci-1.0") ->
fdt_find_prop_in_node(..., "method", ...) -- sets a module-static
s_psci_use_smc flag to 1 only when method reads exactly "smc"; every
other outcome (no DTB, no PSCI node, method="hvc", property absent)
leaves it at its default 0/HVC, preserving this system's existing QEMU
behavior exactly. arch_cold_reset() now branches on that flag between
smc #0/hvc #0. Called once from apic_init() (arch/aarch64/apic.c), the
one point in boot with boot_info->dtb already in hand -- collocated with
its consumer in arch.c rather than defined in apic.c, since
arch_cold_reset() has no boot_info of its own by the time it runs (called
from deep in VM execution, via BYE).
Three-arch QEMU acceptance run exercised both new guard branches: the
aarch64 boot log shows "PSCI: no DTB -- using HVC (QEMU virt-machine
default)" immediately before the existing GIC discovery lines, then
boots clean to zuse)ok> -- confirming the QEMU/UEFI path is unchanged.
The SMC success branch itself stays unverified until real Pi 5 hardware
runs BYE.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019YcT3H2PQeyujrzjqS3Var
This commit is contained in:
co-authored by
Claude Sonnet 5
parent
dc9445abec
commit
350287850f
+46
-21
@@ -541,9 +541,12 @@ blocker for free.
|
||||
7. **Code audit pass — DONE, 2026-09-04.** Reviewed `arch/aarch64/apic.c`, `arch.c`,
|
||||
`timer.c`, `interrupts.c` for the same class of QEMU-virt-vs-real-hardware assumption
|
||||
§III item 6's amd64 audit looked for. Report-only by default, per this project's own
|
||||
"identify, don't fix unless asked" rule — three of the four findings below are report
|
||||
only, nothing changed for them. **The GIC base address finding was fixed in code**, per
|
||||
direct instruction naming it specifically after this audit landed — see that finding's
|
||||
"identify, don't fix unless asked" rule — **all three findings below were fixed in code
|
||||
across two follow-up rounds, each named specifically by direct instruction after this
|
||||
audit landed**: the GIC base address (severe), the stale VBAR_EL2 doc comment (doc-only),
|
||||
and the hardcoded HVC PSCI conduit. §V.3's own parallel audit found a fourth finding of
|
||||
this same class (riscv64's `satp`-clear reasoning, lowest severity of the whole set) that
|
||||
remains report-only, not asked for. See each finding's own
|
||||
own text for what changed.
|
||||
|
||||
**Finding — severe, confirmed live, blocked reaching `ok>` on real hardware as the code
|
||||
@@ -580,29 +583,51 @@ blocker for free.
|
||||
`"arm,gic-400"` node) — that needs a devicetree this build never has, so it stays
|
||||
unverified until real Pi 5 hardware, same caveat as every other native-path item in this
|
||||
list.
|
||||
**Finding — doc-only, not a functional gap**: `arch_interrupts_init()`'s own doc comment
|
||||
in `interrupts.c` claims "`VBAR_EL1` is written unconditionally regardless of the
|
||||
**Finding — doc-only, not a functional gap — FIXED (comment corrected) 2026-09-04, per
|
||||
direct instruction naming it specifically.** `arch_interrupts_init()`'s own doc comment
|
||||
in `interrupts.c` claimed "`VBAR_EL1` is written unconditionally regardless of the
|
||||
detected level... if EL2, this is a known gap" — checked against `isr.S`
|
||||
(`aarch64_install_vectors`, lines 124–146) and found **stale**: the actual implementation
|
||||
already branches on `aarch64_current_el()` and writes `vbar_el2`/`vbar_el1` correctly,
|
||||
with `el2_mode_flag` threading the same answer through the IRQ trampoline for
|
||||
`ELR_EL1`/`SPSR_EL1` vs `ELR_EL2`/`SPSR_EL2` selection. The comment describes a gap the
|
||||
code already closed; left as-is per report-only policy, but flagged so a future reader
|
||||
doesn't take the stale comment at face value. Which EL the Pi 5's ATF hands the kernel
|
||||
off at under the native path is itself unconfirmed (the `atf@0` reserved-memory region
|
||||
found during item 2's work only establishes that EL3 firmware exists, not which EL the
|
||||
kernel lands at) — moot for this specific gap now that both paths are actually wired, but
|
||||
worth knowing for other EL-dependent code.
|
||||
`ELR_EL1`/`SPSR_EL1` vs `ELR_EL2`/`SPSR_EL2` selection. Rewrote the doc comment and the
|
||||
matching runtime console message (which made the identical stale claim — "not yet
|
||||
wired, see item 0.5/0.7") to describe what `isr.S` actually does, and fixed a
|
||||
same-vintage one-word staleness in `arch.c`'s own `aarch64_install_vectors` extern
|
||||
comment ("installs `VBAR_EL1`" → EL-aware) caught while touching this. No behavior
|
||||
changed anywhere — this was purely three comments correcting themselves to match code
|
||||
that was already right. Which EL the Pi 5's ATF hands the kernel off at under the native
|
||||
path is itself still unconfirmed (the `atf@0` reserved-memory region found during item
|
||||
2's work only establishes that EL3 firmware exists, not which EL the kernel lands at) —
|
||||
moot for this specific fix now that both paths are correctly wired regardless of which EL
|
||||
is chosen, but worth knowing for other EL-dependent code.
|
||||
**Finding — same defect class as amd64's `arch_cold_reset()` finding, on the QEMU side of
|
||||
this arch specifically**: `arch/aarch64/arch.c`'s `arch_cold_reset()` issues PSCI
|
||||
`SYSTEM_RESET` via `HVC` — a fix already documented in-file as a QEMU-specific
|
||||
workaround, because QEMU's AAVMF firmware has no genuine EL3/TrustZone secure monitor to
|
||||
answer an `SMC`. The Pi 5's real ATF (confirmed present via the `atf@0` reservation) is
|
||||
exactly the kind of genuine EL3 firmware PSCI's `SMC` conduit assumes — the conventional
|
||||
real-hardware call, not `HVC`. No runtime conduit detection exists; this is hardcoded to
|
||||
the QEMU-only fix with no fallback. Riscv64's equivalent (`arch/riscv64/arch.c`, SBI SRST)
|
||||
is the portable pattern this and amd64's i8042-pulse version both lack — worth naming as
|
||||
the standard to bring the other two toward, not fixed here.
|
||||
this arch specifically — FIXED 2026-09-04, per direct instruction naming it
|
||||
specifically.** `arch/aarch64/arch.c`'s `arch_cold_reset()` issued PSCI `SYSTEM_RESET` via
|
||||
`HVC` unconditionally — a fix already documented in-file as a QEMU-specific workaround,
|
||||
because QEMU's AAVMF firmware has no genuine EL3/TrustZone secure monitor to answer an
|
||||
`SMC`. The Pi 5's real ATF (confirmed present via the `atf@0` reservation) is exactly the
|
||||
kind of genuine EL3 firmware PSCI's `SMC` conduit assumes — confirmed further this pass:
|
||||
`bcm2712.dtsi`'s own `/psci` node declares `method = "smc"` directly, not inferred from
|
||||
ATF's mere presence. **Fix**: new `aarch64_psci_conduit_init(dtb)` (`arch.c`) —
|
||||
`fdt_valid(dtb)` → `fdt_find_node_by_compatible(dtb, "arm,psci-1.0")` →
|
||||
`fdt_find_prop_in_node(..., "method", ...)` — sets a module-static `s_psci_use_smc` flag
|
||||
to 1 only when the DTB's own `method` property reads exactly `"smc"`; every other
|
||||
outcome (no DTB, no PSCI node, `method="hvc"`, property absent) leaves it at its default
|
||||
0, preserving the exact HVC behaviour this system's QEMU boot already relies on.
|
||||
`arch_cold_reset()` now branches on that flag between the `smc #0`/`hvc #0` trap
|
||||
instructions; the function ID and calling convention are identical either way, matching
|
||||
the file's own existing comment about that. Called once from `apic_init()`
|
||||
(`arch/aarch64/apic.c`), the one point in boot with `boot_info->dtb` already in hand —
|
||||
collocated in `arch.c`, not `apic.c`, because `arch_cold_reset()` is the actual consumer
|
||||
and has no `boot_info` of its own by the time it runs (called from deep in VM execution,
|
||||
via `BYE`). Riscv64's equivalent (`arch/riscv64/arch.c`, SBI SRST) remains the one cold-
|
||||
reset implementation across all three architectures that never needed this kind of fix —
|
||||
still worth naming as the standard the other two moved toward, one now fixed, amd64's
|
||||
i8042-pulse version still open. 3-arch acceptance exercised the guard (this system's QEMU
|
||||
AAVMF forwards no DTB, so `aarch64_psci_conduit_init()` hits its own "no DTB" branch and
|
||||
`arch_cold_reset()` keeps using HVC, unchanged) but not the `SMC` success branch itself —
|
||||
that stays unverified until real Pi 5 hardware runs `BYE`.
|
||||
|
||||
**Hardware-dependent, after 2026-09-17 (not started until then):**
|
||||
8. Build the boot media (SD card: `config.txt`, `bcm2712-rpi-5-b.dtb`, kernel image).
|
||||
|
||||
@@ -1,5 +1,5 @@
|
||||
# Capsule Block Manifest — Auto-generated
|
||||
<!-- Generated by mkcapsule --manifest 2026-09-05T03:52:12Z -->
|
||||
<!-- Generated by mkcapsule --manifest 2026-09-05T04:04:40Z -->
|
||||
<!-- DO NOT EDIT — re-run mkcapsule --manifest to refresh. -->
|
||||
<!-- Hand-written justifications and immutability notes live -->
|
||||
<!-- in MANIFEST.md alongside this auto-generated index. -->
|
||||
|
||||
Binary file not shown.
File diff suppressed because it is too large
Load Diff
File diff suppressed because it is too large
Load Diff
File diff suppressed because it is too large
Load Diff
@@ -53,6 +53,13 @@
|
||||
* interrupts.c: "exception level" has no cross-ISA meaning. */
|
||||
extern int aarch64_current_el(void);
|
||||
|
||||
/* Defined in arch.c (FABRIC-3.md §IV.3 item 7 fix, 2026-09-04) --
|
||||
* discovers the real PSCI conduit for arch_cold_reset() from the DTB.
|
||||
* Collocated with its consumer there, not defined here, even though this
|
||||
* is the one point in boot with boot_info->dtb already in hand for GIC
|
||||
* discovery -- see arch.c's own doc comment on this function for why. */
|
||||
extern void aarch64_psci_conduit_init(const void *dtb);
|
||||
|
||||
/* ---- QEMU virt machine constants (fallback when no DTB GIC node is
|
||||
* found -- see file header) ----------------------------------------------- */
|
||||
#define GICD_BASE_PA_QEMU_DEFAULT 0x08000000UL
|
||||
@@ -218,11 +225,17 @@ static int gic_bases_from_dtb(const void *dtb, uintptr_t *gicd_out, uintptr_t *g
|
||||
*
|
||||
* @param boot_info Kernel @c BootInfo; @c dtb is consulted for real-hardware
|
||||
* GIC base discovery as of the 2026-09-04 fix (previously
|
||||
* unused — see file header).
|
||||
* unused — see file header), and, as of the same fix, for
|
||||
* @c aarch64_psci_conduit_init()'s PSCI conduit discovery
|
||||
* (arch.c) — bundled into this call for the same reason
|
||||
* GIC discovery is: this is the one point in boot with
|
||||
* @c boot_info->dtb already in hand.
|
||||
* @return 0 always.
|
||||
*/
|
||||
int apic_init(BootInfo *boot_info)
|
||||
{
|
||||
aarch64_psci_conduit_init(boot_info ? boot_info->dtb : (void *) 0);
|
||||
|
||||
if (boot_info && gic_bases_from_dtb(boot_info->dtb, &s_gicd_base, &s_gicc_base)) {
|
||||
console_puts("GICv2: base addresses discovered from DTB (GICD=0x");
|
||||
for (int s = 60; s >= 0; s -= 4)
|
||||
|
||||
@@ -9,9 +9,15 @@
|
||||
*/
|
||||
|
||||
#include "arch.h"
|
||||
#include "console.h"
|
||||
#include "starkernel/fdt.h"
|
||||
#include <stdint.h>
|
||||
|
||||
/* Installed by isr.S — installs VBAR_EL1 */
|
||||
/* Installed by isr.S — installs VBAR_EL2 at EL2, VBAR_EL1 at EL1 (EL-aware
|
||||
* since before this comment was corrected, 2026-09-04 — see
|
||||
* interrupts.c's arch_interrupts_init() doc comment for the fuller
|
||||
* correction; this one-word staleness was caught in passing while fixing
|
||||
* that one). */
|
||||
extern void aarch64_install_vectors(void);
|
||||
|
||||
/* -1 = not yet read. Cached rather than re-read on every call: CurrentEL
|
||||
@@ -122,6 +128,70 @@ void arch_halt(void)
|
||||
__asm__ volatile ("wfi" ::: "memory");
|
||||
}
|
||||
|
||||
/* PSCI conduit selection (FABRIC-3.md §IV.3 item 7 fix, 2026-09-04).
|
||||
* Defaults to 0 (HVC) so a boot with no DTB -- this system's QEMU/UEFI
|
||||
* firmware, same `fdt_valid()` failure mode already documented in
|
||||
* apic.c's file header for GIC discovery -- keeps arch_cold_reset()'s
|
||||
* existing, working HVC behaviour unchanged. Set to 1 (SMC) by
|
||||
* aarch64_psci_conduit_init() below when the real DTB's own `/psci` node
|
||||
* says so. */
|
||||
static int s_psci_use_smc = 0;
|
||||
|
||||
/**
|
||||
* @brief Discover the real PSCI conduit ("smc" vs "hvc") from the DTB.
|
||||
*
|
||||
* Looks up the devicetree's PSCI firmware node and reads its `method`
|
||||
* property. BCM2712's own `/psci` node declares
|
||||
* `compatible = "arm,psci-1.0", "arm,psci-0.2", "arm,psci"` and
|
||||
* `method = "smc"` -- confirmed directly against `bcm2712.dtsi`, not
|
||||
* assumed. `fdt_find_node_by_compatible()` matches any entry in a node's
|
||||
* compatible list, so searching for just the first (most specific) string
|
||||
* is sufficient; the other two exist for older PSCI-spec-version
|
||||
* consumers, not because this node might be tagged differently.
|
||||
*
|
||||
* Leaves @c s_psci_use_smc at its default (0, HVC) if @p dtb is not a
|
||||
* valid FDT, no PSCI node is found, or `method` is anything other than
|
||||
* exactly `"smc"` -- including the explicit `"hvc"` case, and the
|
||||
* "property absent" case, both of which should keep the existing HVC
|
||||
* behaviour rather than guess.
|
||||
*
|
||||
* Called once from @c apic_init() (`arch/aarch64/apic.c`), the one point
|
||||
* in boot that already has @c boot_info->dtb in hand at the right time
|
||||
* for this kind of discovery (same rationale as that file's own GIC-base
|
||||
* lookup) -- collocated here in @c arch.c, not there, because
|
||||
* @c arch_cold_reset() is this function's actual consumer and has no
|
||||
* @c boot_info of its own to probe with by the time it runs (called from
|
||||
* deep in VM execution, via `BYE`). Same extern-in-place convention this
|
||||
* file and apic.c already use for @c aarch64_current_el().
|
||||
*
|
||||
* @param dtb Candidate devicetree blob (@c BootInfo->dtb); NULL-safe.
|
||||
*/
|
||||
void aarch64_psci_conduit_init(const void *dtb)
|
||||
{
|
||||
const void *node;
|
||||
const char *method;
|
||||
uint32_t len;
|
||||
|
||||
if (!fdt_valid(dtb)) {
|
||||
console_println("PSCI: no DTB -- using HVC (QEMU virt-machine default)");
|
||||
return;
|
||||
}
|
||||
|
||||
node = fdt_find_node_by_compatible(dtb, "arm,psci-1.0");
|
||||
if (!node) {
|
||||
console_println("PSCI: no DTB PSCI node -- using HVC (QEMU virt-machine default)");
|
||||
return;
|
||||
}
|
||||
|
||||
method = (const char *) fdt_find_prop_in_node(dtb, node, "method", &len);
|
||||
if (method && len >= 3 && method[0] == 's' && method[1] == 'm' && method[2] == 'c') {
|
||||
s_psci_use_smc = 1;
|
||||
console_println("PSCI: conduit discovered from DTB (method=\"smc\")");
|
||||
} else {
|
||||
console_println("PSCI: DTB method != \"smc\" -- using HVC");
|
||||
}
|
||||
}
|
||||
|
||||
void arch_cold_reset(void)
|
||||
{
|
||||
arch_disable_interrupts();
|
||||
@@ -129,21 +199,37 @@ void arch_cold_reset(void)
|
||||
* arguments and has no SMC64 variant defined by the PSCI spec -- only
|
||||
* the SMC32 encoding is valid (fixed 2026-08-18, was 0xC4000009).
|
||||
*
|
||||
* Conduit is @c s_psci_use_smc, set by @c aarch64_psci_conduit_init()
|
||||
* above from the real DTB when one is present (real Pi 5 hardware's
|
||||
* own `/psci` node declares `method = "smc"`, confirmed against
|
||||
* `bcm2712.dtsi`).
|
||||
*
|
||||
* FABRIC-1.md Section I, 2026-08-18: live gdb tracing (using the real
|
||||
* UEFI-relocated runtime address, not the standalone kernel.elf's
|
||||
* link-time address -- see that section for why those differ) proved
|
||||
* the SMC call itself traps: PC does not fall through to the wfi loop
|
||||
* below, it jumps straight into this kernel's own exception vector.
|
||||
* This QEMU aarch64 boot (AAVMF UEFI firmware, no genuine EL3/TrustZone
|
||||
* secure monitor) has nothing to answer an SMC -- PSCI here is served
|
||||
* via HVC (hypervisor call, EL2) instead. Switched conduits; the
|
||||
* function ID and calling convention are unchanged. */
|
||||
__asm__ volatile (
|
||||
"mov x0, #0x84000000\n"
|
||||
"movk x0, #0x0009\n"
|
||||
"hvc #0\n"
|
||||
::: "x0", "memory"
|
||||
);
|
||||
* the SMC call itself traps on *this system's* QEMU aarch64 boot
|
||||
* (AAVMF UEFI firmware, no genuine EL3/TrustZone secure monitor, no
|
||||
* DTB forwarded to the guest either): PC does not fall through to the
|
||||
* wfi loop below, it jumps straight into this kernel's own exception
|
||||
* vector. PSCI there is served via HVC (hypervisor call, EL2)
|
||||
* instead -- exactly the @c s_psci_use_smc == 0 default case. The
|
||||
* function ID and calling convention are unchanged between conduits,
|
||||
* only the trapping instruction differs. */
|
||||
if (s_psci_use_smc) {
|
||||
__asm__ volatile (
|
||||
"mov x0, #0x84000000\n"
|
||||
"movk x0, #0x0009\n"
|
||||
"smc #0\n"
|
||||
::: "x0", "memory"
|
||||
);
|
||||
} else {
|
||||
__asm__ volatile (
|
||||
"mov x0, #0x84000000\n"
|
||||
"movk x0, #0x0009\n"
|
||||
"hvc #0\n"
|
||||
::: "x0", "memory"
|
||||
);
|
||||
}
|
||||
for (;;) __asm__ volatile ("wfi" ::: "memory");
|
||||
}
|
||||
|
||||
|
||||
@@ -180,25 +180,35 @@ void aarch64_exception_handler(void)
|
||||
* this call site.
|
||||
*
|
||||
* Then calls @c aarch64_install_vectors() (defined in @c isr.S) which
|
||||
* writes the address of the vector table base into @c VBAR_EL1 (Vector
|
||||
* Base Address Register, EL1). After this instruction all EL1 exceptions
|
||||
* (synchronous, IRQ, FIQ, SError) dispatch through the 512-byte-aligned
|
||||
* vector table assembled in @c isr.S.
|
||||
* writes the address of the vector table base into @c VBAR_EL1 or
|
||||
* @c VBAR_EL2 (Vector Base Address Register), whichever matches the
|
||||
* exception level @c aarch64_current_el() just reported. After this
|
||||
* instruction all exceptions at that level (synchronous, IRQ, FIQ,
|
||||
* SError) dispatch through the 512-byte-aligned vector table assembled
|
||||
* in @c isr.S.
|
||||
*
|
||||
* **@c VBAR_EL1 is written unconditionally regardless of the detected
|
||||
* level.** If @c aarch64_current_el() reports EL2, this is a known gap:
|
||||
* exceptions taken at EL2 vector through @c VBAR_EL2, which nothing here
|
||||
* programs, and installing EL2-aware vectors is punch-list item 0.5's
|
||||
* scope, not this one's. Item 0.4 establishes the detection every later
|
||||
* EL-dependent item must consult; it does not yet make use of it here.
|
||||
* **Correction, 2026-09-04 (FABRIC-3.md §IV.3 item 7's code audit):**
|
||||
* this doc comment and the runtime console message below previously
|
||||
* claimed EL2 was "a known gap... @c VBAR_EL1 is written unconditionally
|
||||
* regardless of the detected level," citing punch-list item 0.5 as the
|
||||
* still-open work to fix it. Checked against @c isr.S's own
|
||||
* @c aarch64_install_vectors() (the actual implementation, not this
|
||||
* comment) and found that claim stale: the assembly already branches on
|
||||
* @c aarch64_current_el() and writes @c vbar_el2 at EL2 /
|
||||
* @c vbar_el1 at EL1, threading the same answer through
|
||||
* @c el2_mode_flag for the IRQ trampoline's @c ELR_ELx/@c SPSR_ELx
|
||||
* selection too (see @c isr.S's own "EL selection" header comment).
|
||||
* There never was a gap here to close — item 0.5's EL2 vector work was
|
||||
* already done by the time this comment was written describing it as
|
||||
* open; only the comment was wrong, not the code.
|
||||
*
|
||||
* On AArch64 there is no IDT and no PIC to disable; the GIC replaces
|
||||
* both. The @c arch_interrupts_init() name is kept identical across all
|
||||
* ISAs so that @c kernel_main() can call the same symbol regardless of
|
||||
* the target architecture.
|
||||
*
|
||||
* Must be called after the kernel stack is established (so that the EL1
|
||||
* SP_EL1 is valid) and before @c arch_enable_interrupts().
|
||||
* Must be called after the kernel stack is established (so that the
|
||||
* correct-EL SP is valid) and before @c arch_enable_interrupts().
|
||||
*/
|
||||
void arch_interrupts_init(void)
|
||||
{
|
||||
@@ -206,7 +216,7 @@ void arch_interrupts_init(void)
|
||||
console_puts("AArch64: running at EL");
|
||||
console_putc((char)('0' + el));
|
||||
console_println(el == 2
|
||||
? " (VBAR_EL2/CNTHP_*_EL2 required — not yet wired, see item 0.5/0.7)"
|
||||
? " (VBAR_EL2/CNTHP_*_EL2 -- both already EL-aware, see isr.S/apic.c)"
|
||||
: "");
|
||||
|
||||
aarch64_install_vectors();
|
||||
|
||||
Reference in New Issue
Block a user