Skip to content

arch/risc-v: assign per-cpu data by logical cpu - #20012

Open
imeghar2408-max wants to merge 1 commit into
apache:masterfrom
imeghar2408-max:fix/riscv-percpu-hart-mapping
Open

arch/risc-v: assign per-cpu data by logical cpu#20012
imeghar2408-max wants to merge 1 commit into
apache:masterfrom
imeghar2408-max:fix/riscv-percpu-hart-mapping

Conversation

@imeghar2408-max

@imeghar2408-max imeghar2408-max commented Aug 30, 2026

Copy link
Copy Markdown

Summary

Fix the assignment of RISC-V per-CPU data so that each HART is assigned
the g_percpu entry corresponding to its logical CPU ID.

Previously, riscv_percpu_init() initialized interrupt stacks according
to the g_percpu[] array index and placed the entries in a FIFO freelist.
riscv_percpu_add_hart() then assigned entries according to the order in
which HARTs registered. When HART boot order differs from logical CPU
order, a HART can therefore be associated with the wrong per-CPU
interrupt stack.

This change uses riscv_hartid_to_cpuid() to select the corresponding
g_percpu[] entry directly and removes the now-unused freelist.

Impact

  • Impact on user: NO, no user-facing API changes.
  • Impact on build: NO, no build-system changes are introduced.
  • Impact on hardware: YES, this affects RISC-V SMP platforms using
    scratch-based per-CPU storage by making per-CPU interrupt-stack
    assignment deterministic with respect to logical CPU ID.
  • Impact on API: NO.
  • Impact on security: NO.
  • Impact on compatibility: NO.

Testing

Built and boot-tested with rv-virt:ksmp64 using
riscv-none-elf-gcc 13.2.0 on Linux with QEMU 10.2.1.

Build Log

Memory region         Used Size  Region Size  %age Used
          kflash:      125604 B         2 MB      5.99%
           ksram:       47232 B         2 MB      2.25%
           pgram:           0 GB         4 MB      0.00%
CP: nuttx.hex
BUILD EXIT CODE: 0

Runtime Log:

CPU0  ... 0x80408800  2048
CPU1  ... 0x80408000  2048
CPU2  ... 0x80407800  2048
CPU3  ... 0x80407000  2048

CPU0 IDLE
CPU1 IDLE
CPU2 IDLE
CPU3 IDLE

All four CPUs booted successfully, and the interrupt-stack bases were
spaced by 2048 bytes in the expected CPU-index order.

The rv-virt:ksmp64 SMP exception entry does not read
RISCV_PERCPU_IRQSTACK, so this runtime test verifies normal SMP boot
and the expected per-CPU stack layout but does not exercise the specific
interrupt-stack path affected by this change.

MPFS uses the RISCV_PERCPU_IRQSTACK path, but no in-tree MPFS SMP
configuration is available for runtime validation in this environment.

Fixes: #19875

@github-actions github-actions Bot added Arch: risc-v Issues related to the RISC-V (32-bit or 64-bit) architecture Size: S The size of the change in this PR is small labels Aug 30, 2026

@acassis acassis left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@imeghar2408-max please fix the CI issue: "Blank line precedes right brace at line"

@github-actions

github-actions Bot commented Aug 30, 2026

Copy link
Copy Markdown

MemBrowse Memory Report

No memory changes detected for:

@imeghar2408-max

Copy link
Copy Markdown
Author

@acassis Fixed the CI formatting issue and pushed the change in 393d724. Thanks!

@simbit18

Copy link
Copy Markdown
Contributor

Hi @imeghar2408-max please rebase

@imeghar2408-max
imeghar2408-max force-pushed the fix/riscv-percpu-hart-mapping branch from 393d724 to 5191fb5 Compare August 30, 2026 14:10
@imeghar2408-max

Copy link
Copy Markdown
Author

@simbit18 Rebased the branch onto the latest upstream/master and pushed the updated history.

@simbit18

Copy link
Copy Markdown
Contributor

@imeghar2408-max please fix

../nuttx/tools/checkpatch.sh -c -u -m -g  5e92a05dc437225e1d139e57b60d4127851db645..HEAD
❌ Missing git commit message

@imeghar2408-max
imeghar2408-max force-pushed the fix/riscv-percpu-hart-mapping branch from 5191fb5 to d61aebb Compare August 30, 2026 14:22
@acassis

acassis commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

@imeghar2408-max if you used some AI LLM to help you to fix this issue, please add to your commit message:
Assisted-by: Name of AI vendor and model

I asked Claude Code to analyze these changes and it reported:

Problems with the PR itself

1. The test doesn't exercise the bug. On rv-virt with CONFIG_SMP, setintstack in qemu-rv/chip.h
computes g_intstacktop - up_cpu_index*INT_STACK_SIZE and never reads irq_stack from the
scratch area. Same for jh7110, k230, bl808, sg2000, eic7700x. The only chip whose SMP exception
entry reads RISCV_PERCPU_IRQSTACK is MPFS - and there's no in-tree MPFS SMP defconfig. So the
runtime log proves it boots, nothing more. It needs validation on MPFS SMP; @pussuw is the code
owner and the right reviewer for that.

2. spin_lock_irqsave() around percpu = &g_percpu[cpu] is now protecting an address computation.
Drop it. Arguably riscv_percpu_init()/g_initialized could go too (compute irq_stack inline for cpu).

3. Incomplete cleanup: union riscv_percpu_s in riscv_percpu.h still has the next member for "sl list
linkage". Should become a plain struct.

4. Correctness now depends on riscv_hartid_to_cpuid() being right. Without
CONFIG_ARCH_RV_CPUID_MAP it's the identity macro (hart), so any hart with hartid >=
CONFIG_SMP_NCPUS writes past g_percpu[] in a release build — the DEBUGASSERT only catches it
with debug asserts on. I checked the in-tree callers and they're fine (jh7110 rebases the hart ID in
head.S, the other SoCs only register hart 0 directly and secondaries go through the
riscv_cpuid_to_hartid(cpu) round-trip, MPFS has the map), but the freelist tolerated arbitrary hart
IDs and this doesn't.

5. The issue (#19875) gives no platform and no observed failure, and the description conflates two
things. The likely real-world symptom is a crash dump showing the wrong CPU's IRQ stack on an
MPFS-style port, not a runtime fault.

@imeghar2408-max

Copy link
Copy Markdown
Author

@acassis Regarding point #4, I understand the concern about riscv_hartid_to_cpuid() being used as an array index. Since the in-tree callers currently provide valid CPU mappings, would you recommend keeping the DEBUGASSERT as an invariant check, or is there a preferred way in NuttX to handle an invalid mapping here?

Comment thread arch/risc-v/src/common/riscv_percpu.c Outdated
@imeghar2408-max
imeghar2408-max force-pushed the fix/riscv-percpu-hart-mapping branch from d61aebb to f3607ee Compare August 30, 2026 16:25
@acassis

acassis commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

@acassis Regarding point #4, I understand the concern about riscv_hartid_to_cpuid() being used as an array index. Since the in-tree callers currently provide valid CPU mappings, would you recommend keeping the DEBUGASSERT as an invariant check, or is there a preferred way in NuttX to handle an invalid mapping here?

I think it is better to prevent it in the release version too, not only on development/testing build

Assign the per-CPU area using the HART to CPU mapping instead of the
order in which HARTs register. This keeps interrupt stack assignment
consistent with the logical CPU and avoids incorrect per-CPU IRQ stack
selection when HART boot order differs.

Signed-off-by: Megha Rajput <i.meghar.2408@gmail.com>
@imeghar2408-max

Copy link
Copy Markdown
Author

@acassis Addressed point 4 by adding a release-safe bounds check before indexing g_percpu.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Arch: risc-v Issues related to the RISC-V (32-bit or 64-bit) architecture Size: S The size of the change in this PR is small

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] CONFIG_RISCV_PERCPU_SCRATCH mismatch stack pointer

4 participants