apic.c (aarch64): fix GIC base address for real Pi 5 hardware
FABRIC-3.md §IV.3 item 7's code audit found GICD_BASE_PA/GICC_BASE_PA hardcoded to QEMU virt-machine constants (0x08000000/0x08010000), self-documented as a deliberate exception because QEMU's aarch64 UEFI firmware never forwards a DTB. That premise doesn't hold on the native Pi 5 boot path (rpi5_native_boot.c), which does have a real DTB and calls this same, unmodified apic_init() -- which ignored boot_info entirely and always programmed the QEMU addresses. Real BCM2712 GIC-400 is at 0x10_7fff9000 (confirmed against bcm2712.dtsi's axi/gicv2 nodes), a completely different region of the address space. Fixed per direct instruction, naming this finding specifically. apic_init() now calls gic_bases_from_dtb() first: fdt_valid(dtb) -> fdt_find_node_by_compatible(dtb, "arm,gic-400") -> fdt_find_prop_in_node(..., "reg", ...), reading the first two 2-address-cell/2-size-cell entries (GICD, then GICC -- the standard arm,gic-400 binding order). Falls back to the QEMU constants on any failure (no DTB, no matching node), so the existing QEMU/UEFI path is unchanged. GICD_BASE_PA/GICC_BASE_PA became s_gicd_base/s_gicc_base (module-static uintptr_t, no longer compile-time constants on this path) -- every MMIO call site (apic_init, apic_spi_enable, apic_read_iar, apic_eoi_intid) now reads through them. Three-arch QEMU acceptance run exercised the guard itself, not just compiled it: the aarch64 boot log shows "GICv2: no DTB GIC node -- using QEMU virt-machine defaults" followed by the unchanged "distributor+CPU interface enabled, PPI 30" line, then boots clean to zuse)ok>. The success branch (a real DTB with a matching node) stays unverified until real Pi 5 hardware -- this build never has a devicetree to exercise it against. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019YcT3H2PQeyujrzjqS3Var
This commit is contained in:
Robert Allan James
co-authored by
Claude Sonnet 5
Claude Sonnet 5
parent
0f798256a0
commit
dc9445abec
@@ -14,36 +14,56 @@
|
||||
* not a general GIC driver -- no SGI support, no GICv3/ITS, no SPI beyond
|
||||
* the one 4.3.5e will actually enable.
|
||||
*
|
||||
* Base addresses and the PPI INTID are QEMU-virt-machine constants, not
|
||||
* device-tree-discovered, and that is a deliberate, recorded exception
|
||||
* rather than an oversight (FABRIC-0.md item 0.6, GAP-B1 follow-up):
|
||||
* `fdt_valid(boot_info->dtb)` fails on this system's aarch64 firmware
|
||||
* (qemu-efi-aarch64 2025.11-3ubuntu7 does not forward a devicetree to the
|
||||
* guest), confirmed live rather than assumed. The values below were not
|
||||
* recalled from memory either -- they were read out of QEMU 10.2.1's own
|
||||
* internal devicetree (`qemu-system-aarch64 -machine virt,dumpdtb=...`,
|
||||
* decoded with this tree's own fdt.c reader) and are therefore correct for
|
||||
* this exact QEMU version, though not guaranteed stable across others.
|
||||
* Register *offsets* within each block (GICD_CTLR, GICC_IAR, etc.) are
|
||||
* fixed by the GICv2 architecture, not the board, and were cross-checked
|
||||
* against Linux's own driver header (linux/irqchip/arm-gic.h, from the
|
||||
* linux-headers package installed on the build host) rather than recalled
|
||||
* either.
|
||||
* GICD/GICC base addresses -- DTB-discovered when a DTB is present, QEMU
|
||||
* virt-machine constants otherwise (FABRIC-3.md §IV.3 item 7, 2026-09-04
|
||||
* fix; originally recorded as a deliberate hardcode, FABRIC-0.md item 0.6
|
||||
* GAP-B1 follow-up, because `fdt_valid(boot_info->dtb)` fails on this
|
||||
* system's QEMU aarch64 firmware -- qemu-efi-aarch64 2025.11-3ubuntu7 does
|
||||
* not forward a devicetree to the guest, confirmed live rather than
|
||||
* assumed). That reasoning covered the QEMU/UEFI path correctly but never
|
||||
* covered the native Pi 5 boot path (`rpi5_native_boot.c`), which receives
|
||||
* a real DTB directly and calls this same, unmodified `apic_init()` --
|
||||
* audited 2026-09-04, found still hardcoding the QEMU addresses
|
||||
* unconditionally, which blocks reaching `ok>` on real BCM2712 hardware
|
||||
* (its real GIC-400 sits at `0x10_7fff9000`, nowhere near QEMU's
|
||||
* `0x08000000`). `gic_bases_from_dtb()` below closes that gap: tries
|
||||
* DTB discovery first, falls back to the QEMU constants only when no DTB
|
||||
* is present or the GIC node isn't found there -- so the QEMU/UEFI path's
|
||||
* existing, working behaviour is unchanged.
|
||||
*
|
||||
* QEMU's own values were not recalled from memory either -- they were read
|
||||
* out of QEMU 10.2.1's own internal devicetree
|
||||
* (`qemu-system-aarch64 -machine virt,dumpdtb=...`, decoded with this
|
||||
* tree's own fdt.c reader) and are therefore correct for this exact QEMU
|
||||
* version, though not guaranteed stable across others. Register *offsets*
|
||||
* within each block (GICD_CTLR, GICC_IAR, etc.) are fixed by the GICv2
|
||||
* architecture, not the board, and were cross-checked against Linux's own
|
||||
* driver header (linux/irqchip/arm-gic.h, from the linux-headers package
|
||||
* installed on the build host) rather than recalled either.
|
||||
*/
|
||||
|
||||
#include "apic.h"
|
||||
#include "uefi.h"
|
||||
#include "console.h"
|
||||
#include "timer.h"
|
||||
#include "starkernel/fdt.h"
|
||||
#include <stdint.h>
|
||||
|
||||
/* Defined in arch.c (item 0.4). Same extern-in-place convention as
|
||||
* interrupts.c: "exception level" has no cross-ISA meaning. */
|
||||
extern int aarch64_current_el(void);
|
||||
|
||||
/* ---- QEMU virt machine constants (see file header) --------------------- */
|
||||
#define GICD_BASE_PA 0x08000000UL
|
||||
#define GICC_BASE_PA 0x08010000UL
|
||||
/* ---- QEMU virt machine constants (fallback when no DTB GIC node is
|
||||
* found -- see file header) ----------------------------------------------- */
|
||||
#define GICD_BASE_PA_QEMU_DEFAULT 0x08000000UL
|
||||
#define GICC_BASE_PA_QEMU_DEFAULT 0x08010000UL
|
||||
|
||||
/* Runtime GIC base addresses -- set once by apic_init(), defaulting to the
|
||||
* QEMU constants above until/unless gic_bases_from_dtb() overrides them.
|
||||
* uintptr_t, not a #define, precisely because these are no longer
|
||||
* compile-time constants on this path. */
|
||||
static uintptr_t s_gicd_base = GICD_BASE_PA_QEMU_DEFAULT;
|
||||
static uintptr_t s_gicc_base = GICC_BASE_PA_QEMU_DEFAULT;
|
||||
|
||||
/* Non-secure EL1 physical timer PPI = 30 (INTID 16+14); EL2 hypervisor
|
||||
* timer PPI = 26 (INTID 16+10). Both read directly out of QEMU's own DT
|
||||
@@ -99,6 +119,78 @@ static uint32_t s_timer_ppi = TIMER_PPI_EL1;
|
||||
static uint64_t s_timer_period_tsc = 0;
|
||||
static uint64_t s_counter_hz_apic = 0; /* CNTFRQ_EL0 (item 0.8/§26) */
|
||||
|
||||
/**
|
||||
* @brief Read one big-endian 32-bit cell from a devicetree property blob.
|
||||
*
|
||||
* `fdt.c` keeps its own `be32()` helper file-local (freestanding, no shared
|
||||
* byte-swap utility to reuse) -- duplicated here rather than exposing it,
|
||||
* same "a few lines is simpler than a new shared dependency" precedent
|
||||
* `rpi5_dtb.c`'s own `reg_first_cell_be32()` already set for this exact
|
||||
* situation.
|
||||
*/
|
||||
static uint32_t be32_cell(const unsigned char *b)
|
||||
{
|
||||
return ((uint32_t) b[0] << 24) | ((uint32_t) b[1] << 16) |
|
||||
((uint32_t) b[2] << 8) | (uint32_t) b[3];
|
||||
}
|
||||
|
||||
/**
|
||||
* @brief Read one two-cell (64-bit) big-endian address from a `reg` entry.
|
||||
*
|
||||
* BCM2712's `gicv2` node (see file header) uses two address cells and two
|
||||
* size cells per `reg` entry -- confirmed directly against `bcm2712.dtsi`'s
|
||||
* `axi`/`gicv2` nodes (`#address-cells = <2>; #size-cells = <2>;`), unlike
|
||||
* `rpi5_dtb.c`'s UART/mailbox lookups which read a single 32-bit cell
|
||||
* because their `soc` parent node declares `#address-cells = <1>`. Reads
|
||||
* only the two address cells; the caller advances past the two size cells
|
||||
* itself via @c GIC_REG_ENTRY_BYTES to reach the next entry.
|
||||
*/
|
||||
static uint64_t be64_addr_cell_pair(const unsigned char *b)
|
||||
{
|
||||
return ((uint64_t) be32_cell(b) << 32) | (uint64_t) be32_cell(b + 4);
|
||||
}
|
||||
|
||||
/* One gicv2 `reg` entry is 4 cells (2 address + 2 size) * 4 bytes = 16
|
||||
* bytes. Entry 0 = GICD (distributor), entry 1 = GICC (CPU interface 0) --
|
||||
* confirmed against bcm2712.dtsi's own `reg` list order (GICD, GICC, GICH,
|
||||
* GICV, the standard arm,gic-400 binding order), not assumed. */
|
||||
#define GIC_REG_ENTRY_BYTES 16u
|
||||
|
||||
/**
|
||||
* @brief Try to discover the real GICD/GICC base addresses from the DTB.
|
||||
*
|
||||
* Looks up the `"arm,gic-400"` node and reads its `reg` property's first
|
||||
* two entries. Returns 0 (leaving @p gicd_out / @p gicc_out untouched) if
|
||||
* @p dtb is not a valid FDT, no matching node exists, or the `reg`
|
||||
* property is shorter than two entries -- callers must keep their own
|
||||
* QEMU-constant default in that case, exactly as before this function
|
||||
* existed (FABRIC-3.md §IV.3 item 7 fix, 2026-09-04; see file header for
|
||||
* why this is needed on the native Pi 5 path specifically).
|
||||
*
|
||||
* @param dtb Candidate devicetree blob (@c BootInfo->dtb); NULL-safe.
|
||||
* @param gicd_out Receives the distributor base address on success.
|
||||
* @param gicc_out Receives the CPU interface 0 base address on success.
|
||||
* @return 1 on success, 0 if discovery failed for any reason.
|
||||
*/
|
||||
static int gic_bases_from_dtb(const void *dtb, uintptr_t *gicd_out, uintptr_t *gicc_out)
|
||||
{
|
||||
const void *node;
|
||||
const unsigned char *reg;
|
||||
uint32_t len;
|
||||
|
||||
if (!fdt_valid(dtb)) return 0;
|
||||
|
||||
node = fdt_find_node_by_compatible(dtb, "arm,gic-400");
|
||||
if (!node) return 0;
|
||||
|
||||
reg = (const unsigned char *) fdt_find_prop_in_node(dtb, node, "reg", &len);
|
||||
if (!reg || len < 2u * GIC_REG_ENTRY_BYTES) return 0;
|
||||
|
||||
*gicd_out = (uintptr_t) be64_addr_cell_pair(reg);
|
||||
*gicc_out = (uintptr_t) be64_addr_cell_pair(reg + GIC_REG_ENTRY_BYTES);
|
||||
return 1;
|
||||
}
|
||||
|
||||
/**
|
||||
* @brief Initialise the GICv2 distributor and CPU interface (M4 milestone).
|
||||
*
|
||||
@@ -106,6 +198,11 @@ static uint64_t s_counter_hz_apic = 0; /* CNTFRQ_EL0 (item 0.8/§26) */
|
||||
* exception level detected in item 0.4 (30 at EL1, 26 at EL2 — see
|
||||
* @c TIMER_PPI_EL1 / @c TIMER_PPI_EL2 above). Sequence:
|
||||
*
|
||||
* 0. Try @c gic_bases_from_dtb() against @p boot_info->dtb; on success,
|
||||
* @c s_gicd_base / @c s_gicc_base are overridden from the real
|
||||
* hardware addresses. On failure (no DTB, e.g. this system's QEMU/UEFI
|
||||
* firmware — see file header) they keep the QEMU constant defaults,
|
||||
* exactly the behaviour this function had before this step existed.
|
||||
* 1. Set this interrupt's priority (@c GICD_IPRIORITYR) below the CPU
|
||||
* interface's priority mask, so it is never itself masked out.
|
||||
* 2. Enable it in @c GICD_ISENABLER0 (PPIs 16–31 live in the first word).
|
||||
@@ -119,13 +216,24 @@ static uint64_t s_counter_hz_apic = 0; /* CNTFRQ_EL0 (item 0.8/§26) */
|
||||
* a general GIC driver and item 0.6's own scope does not call for
|
||||
* reconfiguring it.
|
||||
*
|
||||
* @param boot_info Unused — see the file header for why this does not read
|
||||
* @c boot_info->dtb despite item 0.6 asking for it.
|
||||
* @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).
|
||||
* @return 0 always.
|
||||
*/
|
||||
int apic_init(BootInfo *boot_info)
|
||||
{
|
||||
(void)boot_info;
|
||||
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)
|
||||
console_putc("0123456789abcdef"[(s_gicd_base >> s) & 0xF]);
|
||||
console_puts(", GICC=0x");
|
||||
for (int s = 60; s >= 0; s -= 4)
|
||||
console_putc("0123456789abcdef"[(s_gicc_base >> s) & 0xF]);
|
||||
console_println(")");
|
||||
} else {
|
||||
console_println("GICv2: no DTB GIC node -- using QEMU virt-machine defaults");
|
||||
}
|
||||
|
||||
s_timer_ppi = (aarch64_current_el() == 2) ? TIMER_PPI_EL2 : TIMER_PPI_EL1;
|
||||
|
||||
@@ -133,16 +241,16 @@ int apic_init(BootInfo *boot_info)
|
||||
{
|
||||
uint32_t reg_off = GICD_IPRIORITYR + (s_timer_ppi & ~3u);
|
||||
uint32_t shift = (s_timer_ppi & 3u) * 8u;
|
||||
uint32_t val = mmio_read32(GICD_BASE_PA, reg_off);
|
||||
uint32_t val = mmio_read32(s_gicd_base, reg_off);
|
||||
val = (val & ~(0xFFu << shift)) | (TIMER_PRIORITY << shift);
|
||||
mmio_write32(GICD_BASE_PA, reg_off, val);
|
||||
mmio_write32(s_gicd_base, reg_off, val);
|
||||
}
|
||||
|
||||
mmio_write32(GICD_BASE_PA, GICD_ISENABLER0, 1u << s_timer_ppi);
|
||||
mmio_write32(GICD_BASE_PA, GICD_CTLR, GICD_CTLR_ENABLE_GRP0);
|
||||
mmio_write32(s_gicd_base, GICD_ISENABLER0, 1u << s_timer_ppi);
|
||||
mmio_write32(s_gicd_base, GICD_CTLR, GICD_CTLR_ENABLE_GRP0);
|
||||
|
||||
mmio_write32(GICC_BASE_PA, GICC_PMR, PMR_ALLOW_ALL);
|
||||
mmio_write32(GICC_BASE_PA, GICC_CTLR, GICC_CTLR_ENABLE_GRP0);
|
||||
mmio_write32(s_gicc_base, GICC_PMR, PMR_ALLOW_ALL);
|
||||
mmio_write32(s_gicc_base, GICC_CTLR, GICC_CTLR_ENABLE_GRP0);
|
||||
|
||||
console_puts("GICv2: distributor+CPU interface enabled, PPI ");
|
||||
console_putc((char)('0' + s_timer_ppi / 10));
|
||||
@@ -184,34 +292,34 @@ void apic_spi_enable(uint32_t intid)
|
||||
{
|
||||
uint32_t reg_off = GICD_IPRIORITYR + (intid & ~3u);
|
||||
uint32_t shift = (intid & 3u) * 8u;
|
||||
uint32_t val = mmio_read32(GICD_BASE_PA, reg_off);
|
||||
uint32_t val = mmio_read32(s_gicd_base, reg_off);
|
||||
val = (val & ~(0xFFu << shift)) | (TIMER_PRIORITY << shift);
|
||||
mmio_write32(GICD_BASE_PA, reg_off, val);
|
||||
mmio_write32(s_gicd_base, reg_off, val);
|
||||
}
|
||||
|
||||
{
|
||||
uint32_t reg_off = GICD_ISENABLER0 + 4u * (intid / 32u);
|
||||
uint32_t bit = intid % 32u;
|
||||
mmio_write32(GICD_BASE_PA, reg_off, 1u << bit);
|
||||
mmio_write32(s_gicd_base, reg_off, 1u << bit);
|
||||
}
|
||||
|
||||
{
|
||||
uint32_t reg_off = GICD_ITARGETSR + (intid & ~3u);
|
||||
uint32_t shift = (intid & 3u) * 8u;
|
||||
uint32_t val = mmio_read32(GICD_BASE_PA, reg_off);
|
||||
uint32_t val = mmio_read32(s_gicd_base, reg_off);
|
||||
val = (val & ~(0xFFu << shift)) | (0x01u << shift); /* CPU 0 only */
|
||||
mmio_write32(GICD_BASE_PA, reg_off, val);
|
||||
mmio_write32(s_gicd_base, reg_off, val);
|
||||
}
|
||||
|
||||
{
|
||||
uint32_t reg_off = GICD_ICFGR + 4u * (intid / 16u);
|
||||
uint32_t bitpos = (intid % 16u) * 2u;
|
||||
uint32_t val = mmio_read32(GICD_BASE_PA, reg_off);
|
||||
uint32_t val = mmio_read32(s_gicd_base, reg_off);
|
||||
uint32_t level_expected = 0u; /* level-triggered = the "trigger mode"
|
||||
* bit (bit 1 of the 2-bit field) clear */
|
||||
if (((val >> bitpos) & 0x2u) != level_expected) {
|
||||
val = (val & ~(0x3u << bitpos)) | (level_expected << bitpos);
|
||||
mmio_write32(GICD_BASE_PA, reg_off, val);
|
||||
mmio_write32(s_gicd_base, reg_off, val);
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -229,7 +337,7 @@ void apic_spi_enable(uint32_t intid)
|
||||
*/
|
||||
uint32_t apic_read_iar(void)
|
||||
{
|
||||
return mmio_read32(GICC_BASE_PA, GICC_IAR);
|
||||
return mmio_read32(s_gicc_base, GICC_IAR);
|
||||
}
|
||||
|
||||
/**
|
||||
@@ -256,7 +364,7 @@ uint32_t apic_timer_ppi(void)
|
||||
*/
|
||||
void apic_eoi_intid(uint32_t intid)
|
||||
{
|
||||
mmio_write32(GICC_BASE_PA, GICC_EOIR, intid);
|
||||
mmio_write32(s_gicc_base, GICC_EOIR, intid);
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
Reference in New Issue
Block a user