arch/risc-v: Make CPU index handling based on ARCH_RV_CPUID_MAP

[no1wudi]

Summary

Changes Overview

Refactored CPU index handling in RISC-V architecture to use new ARCH_RV_CPUID_MAP configuration.

Key Changes

1. New Configuration

• Added ARCH_RV_CPUID_MAP Kconfig option
• Enables CPU ID mapping for systems needing hardware-to-logical CPU ID mapping

2. Documentation Improvements

• Added detailed help text in Kconfig explaining:
  • CPU ID mapping functionality
  • Multi-cluster system examples
  • Special cases (e.g., K230 chips)
  • Key points about SMP_NCPUS and hart ID mapping

3. Code Refactoring

• Moved CPU index functions to use new configuration
• Simplified code by removing redundant implementations
• Added better documentation for mapping functions

4. Build System Updates

• Modified CMakeLists.txt and Make.defs
• Replaced ARCH_HAVE_MULTICPU checks with ARCH_RV_CPUID_MAP

Purpose

Provides better support for RISC-V systems with:

• Non-contiguous CPU numbering
• Non-linear CPU numbering
• Multi-cluster systems requiring hardware thread ID (mhartid) to cluster-local ID mapping

Impact

RISCV SMP

Testing

GitHub CI and QEMU

[nuttxpr]

[Experimental Bot, please feedback here]

Fill In The Commit Message: This PR contains a Commit with an Empty Commit Message. Please fill in the Commit Message with the PR Summary.

Yes, this PR appears to meet the NuttX requirements, although some sections could be more explicit.

Strengths:

• Clear Summary: The summary explains the "why," "what," and "how" of the changes. The focus on ARCH_RV_HARTID_BASE is helpful.
• Impact Section Identifies Affected Area: Calling out RISC-V SMP is good.
• Testing Mentions Environments: Referencing GitHub CI and QEMU provides some testing context.

Weaknesses (and suggestions for improvement):

• Missing Issue References: If this relates to a specific issue, link it.
• Impact Section Needs More Detail: Expand on how the changes impact RISC-V SMP. Address all the sub-points (user impact, build impact, hardware impact, documentation, security, compatibility). Even if the answer is "NO," explicitly state it. For example:
  • Impact on user: NO. Users should not need to adapt to this change as it is internal.
  • Impact on build: YES. The build system now uses CONFIG_ARCH_RV_HARTID_BASE != 0 as a condition for compiling riscv_cpuindex.c.
  • ... (and so on for all impact categories)
• Testing Logs Missing: The template specifically requests "Testing logs before change" and "Testing logs after change." Include relevant log snippets or explain why they aren't applicable. If using CI, link to a successful run. Be specific about what was tested (e.g., "Boot test on QEMU with 2 cores," "SMP scheduler test on QEMU").

Example for the Testing Section:

Testing:

Build Host: Linux, x86_64, GCC 12.2
Target: QEMU, RISC-V rv64_imafdc_zicsr_zifencei, 2 cores

GitHub CI Run: [Link to CI Run]

Testing logs before change (demonstrating issue X):

```

Testing logs after change (demonstrating the fix):

<Relevant log snippet showing the corrected behavior>

Tests Performed:

• Boot test on QEMU with 2 cores - successful.
• SMP scheduler test on QEMU - successful.

By addressing these weaknesses, the PR will be even stronger and easier for reviewers to evaluate.

[pussuw]

Why make things dependent on CONFIG_ARCH_RV_HARTID_BASE? Without this patch you can overload riscv_hartid_to_cpuid / riscv_cpuid_to_hartid? I'd prefer this breaking change was not merged at all...

[acassis]

Hi @no1wudi please include a short explanation in the git commit message

[no1wudi]

Why make things dependent on CONFIG_ARCH_RV_HARTID_BASE? Without this patch you can overload riscv_hartid_to_cpuid / riscv_cpuid_to_hartid? I'd prefer this breaking change was not merged at all...

@pussuw I am attempting to decouple the implementation of CONFIG_RISCV_PERCPU_SCRATCH and riscv_mhartid, as the SCRATCH register is generally always available. In the current implementation, it seems that only the CONFIG_ARCH_RV_HARTID_BASE configuration affects the behavior of riscv_mhartid (when it is set to 0, the CPU ID and HART ID have a 1:1 mapping). Do you have any other suggestions?

[pussuw]

Why make things dependent on CONFIG_ARCH_RV_HARTID_BASE? Without this patch you can overload riscv_hartid_to_cpuid / riscv_cpuid_to_hartid? I'd prefer this breaking change was not merged at all...

@pussuw I am attempting to decouple the implementation of CONFIG_RISCV_PERCPU_SCRATCH and riscv_mhartid, as the SCRATCH register is generally always available. In the current implementation, it seems that only the CONFIG_ARCH_RV_HARTID_BASE configuration affects the behavior of riscv_mhartid (when it is set to 0, the CPU ID and HART ID have a 1:1 mapping). Do you have any other suggestions?

I use an overloaded version (defined by board logic) of hartid<->cpuid mapping in my downstream project. This was the whole point to define those as weak symbols. Please do not break this functionality.

[no1wudi]

I use an overloaded version (defined by board logic) of hartid<->cpuid mapping in my downstream project. This was the whole point to define those as weak symbols. Please do not break this functionality.

Another point to note is that in this PR, CONFIG_ARCH_RV_HARTID_BASE is used to select the implementation of up_cpu_index. Please confirm if this aligns with your requirements.

#ifdef CONFIG_ARCH_HAVE_MULTICPU
#if CONFIG_ARCH_RV_HARTID_BASE != 0
int up_cpu_index(void) noinstrument_function;
#else
noinstrument_function static inline int up_cpu_index(void)
{
  return READ_CSR(CSR_MHARTID);
}
#endif /* CONFIG_ARCH_RV_HARTID_BASE */
#endif /* CONFIG_ARCH_HAVE_MULTICPU */

[pussuw]

I use an overloaded version (defined by board logic) of hartid<->cpuid mapping in my downstream project. This was the whole point to define those as weak symbols. Please do not break this functionality.

Another point to note is that in this PR, CONFIG_ARCH_RV_HARTID_BASE is used to select the implementation of up_cpu_index. Please confirm if this aligns with your requirements.

#ifdef CONFIG_ARCH_HAVE_MULTICPU
#if CONFIG_ARCH_RV_HARTID_BASE != 0
int up_cpu_index(void) noinstrument_function;
#else
noinstrument_function static inline int up_cpu_index(void)
{
  return READ_CSR(CSR_MHARTID);
}
#endif /* CONFIG_ARCH_RV_HARTID_BASE */
#endif /* CONFIG_ARCH_HAVE_MULTICPU */

Sorry for delayed reply, but yes this will also not work for me. CONFIG_ARCH_RV_HARTID_BASE is too limited for me, as it only allows offset based CPUID mapping. I need to map the cores dynamically as I have AMP in my target as well as SMP. Also the previous patch 6eb2f33 breaks up_cpu_index() for me :(

[pussuw]

Maybe as a kludge solution I can set CONFIG_ARCH_RV_HARTID_BASE to != 0, since I don't use this offset value anyway.

[no1wudi]

@pussuw If that's the case, should an additional option be introduced to control the behavior of the current CPU index mapping? The existing configurations or their combinations do not seem suitable as switches to control it (CONFIG_RISCV_PERCPU_SCRATCH/CONFIG_ARCH_RV_HARTID_BASE), and the conditions for enabling it are also quite ambiguous to me. This makes it difficult to synchronize general optimizations from other architectures (such as inlining small functions) onto the RISC-V platform.

[pussuw]

@pussuw If that's the case, should an additional option be introduced to control the behavior of the current CPU index mapping? The existing configurations or their combinations do not seem suitable as switches to control it (CONFIG_RISCV_PERCPU_SCRATCH/CONFIG_ARCH_RV_HARTID_BASE), and the conditions for enabling it are also quite ambiguous to me. This makes it difficult to synchronize general optimizations from other architectures (such as inlining small functions) onto the RISC-V platform.

That would make sense, since the problem here really is that we don't have a dedicated flag for enabling CPUID mapping.

[no1wudi]

That would make sense, since the problem here really is that we don't have a dedicated flag for enabling CPUID mapping.

Now a new option ARCH_RV_CPUID_MAP is used for CPUID mapping, please review again.

[xiaoxiang781216]

@pussuw could you give the concrete return value from up_cpud_index and up_this_cpu on your platform?

[pussuw]

@pussuw could you give the concrete return value from up_cpud_index and up_this_cpu on your platform?

I need the conversion functions as well, because in mpfs_start we need to do the conversion before we can use up_cpu_index():

nuttx/arch/risc-v/src/mpfs/mpfs_start.c

Line 88 in cd83dc1

if (riscv_hartid_to_cpuid(mhartid) != 0)

nuttx/arch/risc-v/src/mpfs/mpfs_start.c

Line 120 in cd83dc1

riscv_percpu_add_hart(mhartid);

nuttx/arch/risc-v/src/mpfs/mpfs_start.c

Line 210 in cd83dc1

riscv_cpu_boot(riscv_hartid_to_cpuid(mhartid));

In supervisor mode, the hartid is available ONLY as the input parameter for mpfs_start(), before it is installed to the scratch register. The whole reason we need the scratch register to store hartid is the fact that supervisor mode simply does not have the hartid CSR.

[no1wudi]

BTW, the naming of the function riscv_mhartid is highly misleading. Literally, it appears to be a simple wrapper for CSR_MHARTID, but in reality, under CONFIG_RISCV_PERCPU_SCRATCH, it reads the logical ID from percpu data. I believe it should be renamed.

[pussuw]

BTW, the naming of the function riscv_mhartid is highly misleading. Literally, it appears to be a simple wrapper for CSR_MHARTID, but in reality, under CONFIG_RISCV_PERCPU_SCRATCH, it reads the logical ID from percpu data. I believe it should be renamed.

The reason we need the wrapper is that we read the hartid from two places, depeding on runlevel / mode:

• In machine mode riscv_mhartid reads directly from CSR mhartid.
• In supervisor mode we must store hartid to the percpu structure on boot, because supervisor mode does not have CSR shartid.

In supervisor mode SBI gives the hartid in a0 register (this is an SBI ABI requirement) and it is up to the payload OS to store this internally somewhere. We use the percpu scratch register for this, as it is the only place that is unique for every CPU that is not volatile.

it reads the logical ID from percpu data.

It reads the physical ID from percpu data.

I guess in flat (machine) mode you could read hartid from CSR mhartid even if CONFIG_RISCV_PERCPU_SCRATCH is enabled..

[pussuw]

LGTM

[no1wudi]

I guess in flat mode you could read hartid from CSR mhartid even if CONFIG_RISCV_PERCPU_SCRATCH is enabled..

Sure, let's add more detail for riscv_mhartid