RISC-V: Combine 3 variables that depend on CPU amount into one

[pussuw]

IRQ_NSTACKS, ARCH_CPU_COUNT, CONFIG_SMP_NCPUS all relate to each
other. However, a bit of clean up can be done and everything can
be merged into SMP_NCPUS.

The MPFS bootloader case works also as it requires only 1 IRQ stack
for the hart that executes as bootloader.

Summary

Merges three related Kconfig parameters into one

Impact

Reduces configuration complexity by reducing related parameters

Testing

MPFS icicle:knsh + CI passed

[pussuw]

@xiaoxiang781216 can you take a look when you have time. I marked this as draft as it does not compile yet / needs fixing but the basic structure should be in place.

The question I have:
Do you think it's appropriate for me to force other RISC-V targets that use an IRQ stack to use the percpu structure, or should I wrap the IRQ stack getter, so that it can e.g.:

uintptr_t riscv_irqstack(void)
{
#if PERCPU_COMPILED
  return riscv_percpu_get_irqstack
#else
  return (uintptr_t) &g_intstacktop;
#endif
}

Or something along those lines ?

[xiaoxiang781216]

@xiaoxiang781216 can you take a look when you have time. I marked this as draft as it does not compile yet / needs fixing but the basic structure should be in place.

The question I have: Do you think it's appropriate for me to force other RISC-V targets that use an IRQ stack to use the percpu structure, or should I wrap the IRQ stack getter, so that it can e.g.:

uintptr_t riscv_irqstack(void)
{
#if PERCPU_COMPILED
  return riscv_percpu_get_irqstack
#else
  return (uintptr_t) &g_intstacktop;
#endif
}

Or something along those lines ?

but all arch call the chip specific setintstack macro setup the interrupt stack. It work very well, do you have any problem with it?

[pussuw]

@xiaoxiang781216 can you take a look when you have time. I marked this as draft as it does not compile yet / needs fixing but the basic structure should be in place.
The question I have: Do you think it's appropriate for me to force other RISC-V targets that use an IRQ stack to use the percpu structure, or should I wrap the IRQ stack getter, so that it can e.g.:

uintptr_t riscv_irqstack(void)
{
#if PERCPU_COMPILED
  return riscv_percpu_get_irqstack
#else
  return (uintptr_t) &g_intstacktop;
#endif
}

Or something along those lines ?

but all arch call the chip specific setintstack macro setup the interrupt stack. It work very well, do you have any problem with it?

I'm not sure what this "setintstack" macro does, can you provide and example of what you mean ?

For RISC-V, riscv_exception_common.S defines 1 interrupt stack per CPU:
.skip (((CONFIG_ARCH_INTERRUPTSTACK * IRQ_NSTACKS) + 8) & ~15)

And every exception its location is calculated again:

#if CONFIG_ARCH_INTERRUPTSTACK > 15
  /* Load mhartid (cpuid) */

  csrr s0, mhartid

  /* Switch to interrupt stack */
#if IRQ_NSTACKS > 1
  li t0, (CONFIG_ARCH_INTERRUPTSTACK & ~15)
  mul t0, s0, t0
  la s0, g_intstacktop
  sub sp, s0, t0
#else
  la sp, g_intstacktop
#endif

#endif

My idea was to pre-calculate the location of the IRQ stack per hart and use the riscv_percpu.c structure and [m/s]scratch register for this. However, for it to work, you need to call riscv_percpu_add_hart to register the hart there. Only MPFS does this now.

My question was, is it OK to force registering for other RISC-V targets too or should I do the wrapper.

[xiaoxiang781216]

but all arch call the chip specific setintstack macro setup the interrupt stack. It work very well, do you have any problem with it?

I'm not sure what this "setintstack" macro does, can you provide and example of what you mean ?

Here is an example:
https://github.com/apache/incubator-nuttx/blob/master/arch/arm/src/cxd56xx/chip.h#L52-L69

For RISC-V, riscv_exception_common.S defines 1 interrupt stack per CPU: .skip (((CONFIG_ARCH_INTERRUPTSTACK * IRQ_NSTACKS) + 8) & ~15)

And every exception its location is calculated again:

#if CONFIG_ARCH_INTERRUPTSTACK > 15
  /* Load mhartid (cpuid) */

  csrr s0, mhartid

  /* Switch to interrupt stack */
#if IRQ_NSTACKS > 1
  li t0, (CONFIG_ARCH_INTERRUPTSTACK & ~15)
  mul t0, s0, t0
  la s0, g_intstacktop
  sub sp, s0, t0
#else
  la sp, g_intstacktop
#endif

#endif

My idea was to pre-calculate the location of the IRQ stack per hart and use the riscv_percpu.c structure and [m/s]scratch register for this. However, for it to work, you need to call riscv_percpu_add_hart to register the hart there. Only MPFS does this now.

My question was, is it OK to force registering for other RISC-V targets too or should I do the wrapper.

Basically, you can provide setintstack to do what you describe above.

[pussuw]

but all arch call the chip specific setintstack macro setup the interrupt stack. It work very well, do you have any problem with it?

I'm not sure what this "setintstack" macro does, can you provide and example of what you mean ?

Here is an example: https://github.com/apache/incubator-nuttx/blob/master/arch/arm/src/cxd56xx/chip.h#L52-L69

For RISC-V, riscv_exception_common.S defines 1 interrupt stack per CPU: .skip (((CONFIG_ARCH_INTERRUPTSTACK * IRQ_NSTACKS) + 8) & ~15)
And every exception its location is calculated again:

#if CONFIG_ARCH_INTERRUPTSTACK > 15
  /* Load mhartid (cpuid) */

  csrr s0, mhartid

  /* Switch to interrupt stack */
#if IRQ_NSTACKS > 1
  li t0, (CONFIG_ARCH_INTERRUPTSTACK & ~15)
  mul t0, s0, t0
  la s0, g_intstacktop
  sub sp, s0, t0
#else
  la sp, g_intstacktop
#endif

#endif

My idea was to pre-calculate the location of the IRQ stack per hart and use the riscv_percpu.c structure and [m/s]scratch register for this. However, for it to work, you need to call riscv_percpu_add_hart to register the hart there. Only MPFS does this now.
My question was, is it OK to force registering for other RISC-V targets too or should I do the wrapper.

Basically, you can provide setintstack to do what you describe above.

I see, I wanted to store access to the interrupt stack directly to the scratch register, but for this someone would have to set the scratch register. With flat mode the macro should work, with S-mode reading mhartid fails, so I don't see a way to calculate the index for g_cpu_intstack_top. So the macro would work as is for M-mode, but for S-mode someone will have to set the percpu / sscratch anyway (to get hart id).

But maybe I can make uintptr_t riscv_irqstack(void) into an asm macro instead.

[xiaoxiang781216]

setintstack is implemented by chip, so you can define setintstack in mpfs to fetch the interrupt stack pointer from scratch register without breaking other chip's implementation. Basically, you can:

1. Add the interrupt stack pointer to percpu struct
2. Define setintstack in mpfs to return it

Other chip could reuse this, or the developer can provide setintstack in other way if they have a better solution.

[pussuw]

setintstack is implemented by chip, so you can define setintstack in mpfs to fetch the interrupt stack pointer from scratch register without breaking other chip's implementation. Basically, you can:

1. Add the interrupt stack pointer to percpu struct

2. Define setintstack in mpfs to return it

Other chip could reuse this, or the developer can provide setintstack in other way if they have a better solution.

Yes this is workable.

I was worried because now the equivalent of "setintstack" for RISC-V is now in the common code (riscv_exception_common.S), I think I can provide a solution for the common code too. Something like:

#if CONFIG_ARCH_INTERRUPTSTACK > 15
.macro  setintstack, tmp0, tmp1
#ifndef CONFIG_ARCH_USE_SMODE
#if SMP_NCPUS > 1
  csrr \tmp0, mhartid # is the mapping hartid <-> SMP_CPU number ? Don't know
  li     \tmp1, (CONFIG_ARCH_INTERRUPTSTACK & ~15)
  mul \tmp1, \tmp0, \tmp1
  la    \tmp0, g_intstacktop
  sub sp, \tmp0, \tmp1
#else
  la sp, g_intstacktop
#endif
#else
  csrr \tmp0, CSR_SCRATCH
  REGLOAD \tmp1, RISCV_PERCPU_IRQSTACK(\tmp0)
  mv  sp, \tmp1
#endif /* CONFIG_ARCH_USE_SMODE */
.endm
#endif /* CONFIG_ARCH_INTERRUPTSTACK > 15 */

Maybe needs a bit of fixing / tidying up but the basic idea would be like that. You need the percpu structure if the kernel is in S-mode. That cannot be avoided.

[pussuw]

Made a fixup error in rebase, restored now

[pussuw]

There was a CI issue for k210 and qemu with SMP configuration, I added setintstack for those platforms because they did not have one. I think the SMP targets shared 1 IRQ stack for multiple CPUs or maybe the #ifdef stuff in riscv_internal.h saved it.

[pussuw]

Still seems to have some issue with the setintstack macro, the wrong macro is used.
EDIT: Fixed

[Ouss4]

LGTM. I just had one curious comment.