You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2022/04/05 11:06:02 UTC
[GitHub] [incubator-nuttx] pussuw opened a new pull request, #5985: RISC-V: Combine 3 variables that depend on CPU amount into one
pussuw opened a new pull request, #5985:
URL: https://github.com/apache/incubator-nuttx/pull/5985
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pussuw commented on a diff in pull request #5985: RISC-V: Combine 3 variables that depend on CPU amount into one
Posted by GitBox <gi...@apache.org>.
pussuw commented on code in PR #5985:
URL: https://github.com/apache/incubator-nuttx/pull/5985#discussion_r844297957
##########
arch/risc-v/src/common/riscv_percpu.c:
##########
@@ -86,13 +92,39 @@ void riscv_percpu_init(void)
void riscv_percpu_add_hart(uintptr_t hartid)
{
- /* Hart IDs go from 0...4 */
+ riscv_percpu_t *percpu;
+ uintptr_t stack_top;
+
+ /* Get free entry for this hart, this must not fail */
+
+ percpu = (riscv_percpu_t *)sq_remfirst(&g_freelist);
Review Comment:
Because hartid != index.
For original code that had 5 indices this was true. `CONFIG_ARCH_CPU_COUNT` was needed to allocate a table of 5 entries. But we don't use 5 CPUs.
This decouples hartid entirely from the percpu table, and I think this is a better / more scalable solution.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pussuw commented on a diff in pull request #5985: RISC-V: Combine 3 variables that depend on CPU amount into one
Posted by GitBox <gi...@apache.org>.
pussuw commented on code in PR #5985:
URL: https://github.com/apache/incubator-nuttx/pull/5985#discussion_r844941552
##########
arch/risc-v/src/common/riscv_macros.S:
##########
@@ -128,3 +133,22 @@
REGLOAD x31, REG_X31(\out) /* t6 */
.endm
+
+#if CONFIG_ARCH_INTERRUPTSTACK > 15
+.macro setintstack tmp0, tmp1
+#ifndef CONFIG_ARCH_USE_SMODE
+#if CONFIG_SMP_NCPUS > 1
+ csrr \tmp0, mhartid
Review Comment:
I agree it should be in the chip specific files, but no risc-v target implements setintstack. This basically like how it was before.
Do you think I should add this code to every target or leave it like this for now ? Because if I implement this for mpfs only all other targets that use IRQ stack (every risc-v target in fact) will break ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a diff in pull request #5985: RISC-V: Combine 3 variables that depend on CPU amount into one
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #5985:
URL: https://github.com/apache/incubator-nuttx/pull/5985#discussion_r844938949
##########
arch/risc-v/src/common/riscv_macros.S:
##########
@@ -128,3 +133,22 @@
REGLOAD x31, REG_X31(\out) /* t6 */
.endm
+
+#if CONFIG_ARCH_INTERRUPTSTACK > 15
+.macro setintstack tmp0, tmp1
+#ifndef CONFIG_ARCH_USE_SMODE
+#if CONFIG_SMP_NCPUS > 1
+ csrr \tmp0, mhartid
Review Comment:
it's fine to put riscv_percpu.c in the common place, because user can simply ignore it by excluding it from compiling, but it's hard to replace setintstack if it define in riscv_macros.S, that's why I suggest riscv is better to follow other arch not define setintstack in the common place.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pussuw commented on a diff in pull request #5985: RISC-V: Combine 3 variables that depend on CPU amount into one
Posted by GitBox <gi...@apache.org>.
pussuw commented on code in PR #5985:
URL: https://github.com/apache/incubator-nuttx/pull/5985#discussion_r846105380
##########
arch/risc-v/src/qemu-rv/Make.defs:
##########
@@ -34,8 +34,7 @@ CMN_CSRCS += riscv_releasepending.c riscv_reprioritizertr.c
CMN_CSRCS += riscv_releasestack.c riscv_stackframe.c riscv_schedulesigaction.c
CMN_CSRCS += riscv_sigdeliver.c riscv_unblocktask.c riscv_usestack.c
CMN_CSRCS += riscv_idle.c riscv_tcbinfo.c riscv_cpuidlestack.c
-CMN_CSRCS += riscv_exception.c riscv_getnewintctx.c riscv_doirq.c
-CMN_CSRCS += riscv_cpuindex.c
+CMN_CSRCS += riscv_fault.c riscv_getnewintctx.c riscv_doirq.c
Review Comment:
Yes rebase error. riscv_fault does not exist anymore, need to fix
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pussuw commented on pull request #5985: RISC-V: Combine 3 variables that depend on CPU amount into one
Posted by GitBox <gi...@apache.org>.
pussuw commented on PR #5985:
URL: https://github.com/apache/incubator-nuttx/pull/5985#issuecomment-1088574503
@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 ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #5985: RISC-V: Combine 3 variables that depend on CPU amount into one
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on PR #5985:
URL: https://github.com/apache/incubator-nuttx/pull/5985#issuecomment-1089010676
> > 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pussuw commented on pull request #5985: RISC-V: Combine 3 variables that depend on CPU amount into one
Posted by GitBox <gi...@apache.org>.
pussuw commented on PR #5985:
URL: https://github.com/apache/incubator-nuttx/pull/5985#issuecomment-1089252057
> 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" is in the common code, 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
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
.endm
#endif /* CONFIG_ARCH_USE_SMODE */
#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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pussuw commented on a diff in pull request #5985: RISC-V: Combine 3 variables that depend on CPU amount into one
Posted by GitBox <gi...@apache.org>.
pussuw commented on code in PR #5985:
URL: https://github.com/apache/incubator-nuttx/pull/5985#discussion_r845758380
##########
arch/risc-v/src/mpfs/Kconfig:
##########
@@ -46,17 +46,12 @@ config MPFS_BOOTLOADER
---help---
This NuttX image is used as a bootloader, which will boot only on one hart, putting the others in WFI
-config IRQ_NSTACKS
- int
- depends on MPFS_BOOTLOADER
- default 5
-
config MPFS_OPENSBI
- bool "Use OpenSBI"
- depends on MPFS_BOOTLOADER && OPENSBI
- default n
- ---help---
- This uses a ld-envm-opensbi.script linker script and the mpfs_opensbi.c code to use external OpenSBI.
+ bool "Use OpenSBI"
+ depends on MPFS_BOOTLOADER && OPENSBI
+ default n
+ ---help---
+ This uses a ld-envm-opensbi.script linker script and the mpfs_opensbi.c code to use external OpenSBI.
Review Comment:
Of course
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pussuw commented on a diff in pull request #5985: RISC-V: Combine 3 variables that depend on CPU amount into one
Posted by GitBox <gi...@apache.org>.
pussuw commented on code in PR #5985:
URL: https://github.com/apache/incubator-nuttx/pull/5985#discussion_r845030881
##########
arch/risc-v/src/common/riscv_percpu.c:
##########
@@ -86,13 +92,39 @@ void riscv_percpu_init(void)
void riscv_percpu_add_hart(uintptr_t hartid)
{
- /* Hart IDs go from 0...4 */
+ riscv_percpu_t *percpu;
+ uintptr_t stack_top;
+
+ /* Get free entry for this hart, this must not fail */
+
+ percpu = (riscv_percpu_t *)sq_remfirst(&g_freelist);
Review Comment:
Hmm, I don't think the spinlock does quite what I intended. Maybe I need to provide a handle. Need to investigate the code a bit more.
EDIT: It seems to use some kind of global lock, so I think spin_lock_irqsave(NULL); should also act as a synchronization point, am I right ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pussuw commented on a diff in pull request #5985: RISC-V: Combine 3 variables that depend on CPU amount into one
Posted by GitBox <gi...@apache.org>.
pussuw commented on code in PR #5985:
URL: https://github.com/apache/incubator-nuttx/pull/5985#discussion_r845065191
##########
arch/risc-v/src/common/riscv_macros.S:
##########
@@ -128,3 +133,22 @@
REGLOAD x31, REG_X31(\out) /* t6 */
.endm
+
+#if CONFIG_ARCH_INTERRUPTSTACK > 15
+.macro setintstack tmp0, tmp1
+#ifndef CONFIG_ARCH_USE_SMODE
+#if CONFIG_SMP_NCPUS > 1
+ csrr \tmp0, mhartid
Review Comment:
I think this should now be solved. Old implementations use common macro which transforms into:
la sp, g_intstacktop
For MPFS there is a specific solution
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pussuw commented on a diff in pull request #5985: RISC-V: Combine 3 variables that depend on CPU amount into one
Posted by GitBox <gi...@apache.org>.
pussuw commented on code in PR #5985:
URL: https://github.com/apache/incubator-nuttx/pull/5985#discussion_r847026388
##########
arch/risc-v/src/mpfs/chip.h:
##########
@@ -29,4 +29,33 @@
#include "mpfs_memorymap.h"
+#include "riscv_percpu.h"
+
+/****************************************************************************
+ * Macro Definitions
+ ****************************************************************************/
+
+#ifdef __ASSEMBLY__
+
+/****************************************************************************
+ * Name: setintstack
+ *
+ * Description:
+ * Set the current stack pointer to the "top" the correct interrupt stack
+ * for the current CPU.
+ *
+ ****************************************************************************/
+
+#if CONFIG_ARCH_INTERRUPTSTACK > 15
+.macro setintstack tmp0, tmp1
+#ifndef CONFIG_ARCH_USE_S_MODE
Review Comment:
Done
##########
arch/risc-v/src/qemu-rv/Make.defs:
##########
@@ -34,8 +34,7 @@ CMN_CSRCS += riscv_releasepending.c riscv_reprioritizertr.c
CMN_CSRCS += riscv_releasestack.c riscv_stackframe.c riscv_schedulesigaction.c
CMN_CSRCS += riscv_sigdeliver.c riscv_unblocktask.c riscv_usestack.c
CMN_CSRCS += riscv_idle.c riscv_tcbinfo.c riscv_cpuidlestack.c
-CMN_CSRCS += riscv_exception.c riscv_getnewintctx.c riscv_doirq.c
-CMN_CSRCS += riscv_cpuindex.c
+CMN_CSRCS += riscv_fault.c riscv_getnewintctx.c riscv_doirq.c
Review Comment:
Done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pussuw commented on a diff in pull request #5985: RISC-V: Combine 3 variables that depend on CPU amount into one
Posted by GitBox <gi...@apache.org>.
pussuw commented on code in PR #5985:
URL: https://github.com/apache/incubator-nuttx/pull/5985#discussion_r844920820
##########
arch/risc-v/src/common/riscv_macros.S:
##########
@@ -128,3 +133,22 @@
REGLOAD x31, REG_X31(\out) /* t6 */
.endm
+
+#if CONFIG_ARCH_INTERRUPTSTACK > 15
+.macro setintstack tmp0, tmp1
+#ifndef CONFIG_ARCH_USE_SMODE
+#if CONFIG_SMP_NCPUS > 1
+ csrr \tmp0, mhartid
Review Comment:
Of course, just something to keep in mind here. Can easily produce strange results.
A good example is MPFS, which has 5 harts (SiFive cores).
Core 0 is u51 which does not have S-mode, FPU, etc.
Cores 1-4 are u54, which have S-mode / FPU.
So running SMP on e.g. core0&1 makes handling the code difficult, because the cores have different capabilities ! This is why I run NuttX on core 4. With SMP I would most likely use cores 3 and 4 :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pussuw commented on a diff in pull request #5985: RISC-V: Combine 3 variables that depend on CPU amount into one
Posted by GitBox <gi...@apache.org>.
pussuw commented on code in PR #5985:
URL: https://github.com/apache/incubator-nuttx/pull/5985#discussion_r845030881
##########
arch/risc-v/src/common/riscv_percpu.c:
##########
@@ -86,13 +92,39 @@ void riscv_percpu_init(void)
void riscv_percpu_add_hart(uintptr_t hartid)
{
- /* Hart IDs go from 0...4 */
+ riscv_percpu_t *percpu;
+ uintptr_t stack_top;
+
+ /* Get free entry for this hart, this must not fail */
+
+ percpu = (riscv_percpu_t *)sq_remfirst(&g_freelist);
Review Comment:
Hmm, I don't think the spinlock does quite what I intended. Need mutex or semaphore but those are not available yet.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pussuw commented on a diff in pull request #5985: RISC-V: Combine 3 variables that depend on CPU amount into one
Posted by GitBox <gi...@apache.org>.
pussuw commented on code in PR #5985:
URL: https://github.com/apache/incubator-nuttx/pull/5985#discussion_r845912848
##########
arch/risc-v/src/mpfs/Kconfig:
##########
@@ -46,17 +46,12 @@ config MPFS_BOOTLOADER
---help---
This NuttX image is used as a bootloader, which will boot only on one hart, putting the others in WFI
-config IRQ_NSTACKS
- int
- depends on MPFS_BOOTLOADER
- default 5
-
config MPFS_OPENSBI
- bool "Use OpenSBI"
- depends on MPFS_BOOTLOADER && OPENSBI
- default n
- ---help---
- This uses a ld-envm-opensbi.script linker script and the mpfs_opensbi.c code to use external OpenSBI.
+ bool "Use OpenSBI"
+ depends on MPFS_BOOTLOADER && OPENSBI
+ default n
+ ---help---
+ This uses a ld-envm-opensbi.script linker script and the mpfs_opensbi.c code to use external OpenSBI.
Review Comment:
Done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pussuw commented on a diff in pull request #5985: RISC-V: Combine 3 variables that depend on CPU amount into one
Posted by GitBox <gi...@apache.org>.
pussuw commented on code in PR #5985:
URL: https://github.com/apache/incubator-nuttx/pull/5985#discussion_r846106592
##########
arch/risc-v/src/mpfs/chip.h:
##########
@@ -29,4 +29,33 @@
#include "mpfs_memorymap.h"
+#include "riscv_percpu.h"
+
+/****************************************************************************
+ * Macro Definitions
+ ****************************************************************************/
+
+#ifdef __ASSEMBLY__
+
+/****************************************************************************
+ * Name: setintstack
+ *
+ * Description:
+ * Set the current stack pointer to the "top" the correct interrupt stack
+ * for the current CPU.
+ *
+ ****************************************************************************/
+
+#if CONFIG_ARCH_INTERRUPTSTACK > 15
+.macro setintstack tmp0, tmp1
+#ifndef CONFIG_ARCH_USE_S_MODE
Review Comment:
Yes this is wrong, I'll fix on monday
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pussuw commented on pull request #5985: RISC-V: Combine 3 variables that depend on CPU amount into one
Posted by GitBox <gi...@apache.org>.
pussuw commented on PR #5985:
URL: https://github.com/apache/incubator-nuttx/pull/5985#issuecomment-1088927143
> > @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 and the location is calculated there.
` .skip (((CONFIG_ARCH_INTERRUPTSTACK * IRQ_NSTACKS) + 8) & ~15)`
My idea was to pre-calculate the location of the IRQ stack per hart and use the riscv_percpu.c structure and [m/]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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a diff in pull request #5985: RISC-V: Combine 3 variables that depend on CPU amount into one
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #5985:
URL: https://github.com/apache/incubator-nuttx/pull/5985#discussion_r844266512
##########
arch/risc-v/src/common/riscv_percpu.c:
##########
@@ -120,3 +152,24 @@ uintptr_t riscv_percpu_get_hartid(void)
return ((struct riscv_percpu_s *)scratch)->hartid;
Review Comment:
replace all "struct riscv_percpu_s" with "riscv_percpu_t"
##########
arch/risc-v/src/common/riscv_percpu.c:
##########
@@ -86,13 +92,39 @@ void riscv_percpu_init(void)
void riscv_percpu_add_hart(uintptr_t hartid)
{
- /* Hart IDs go from 0...4 */
+ riscv_percpu_t *percpu;
+ uintptr_t stack_top;
+
+ /* Get free entry for this hart, this must not fail */
+
+ percpu = (riscv_percpu_t *)sq_remfirst(&g_freelist);
Review Comment:
why need allocate from g_freelist, we can simplify to:
1. Remove g_freelist and riscv_percpu_t::next
2. Remove riscv_percpu_init
3. Access the new percpu_t instance by g_percpu[hartid]
##########
arch/risc-v/src/common/riscv_percpu.c:
##########
@@ -86,13 +92,39 @@ void riscv_percpu_init(void)
void riscv_percpu_add_hart(uintptr_t hartid)
{
- /* Hart IDs go from 0...4 */
+ riscv_percpu_t *percpu;
+ uintptr_t stack_top;
+
+ /* Get free entry for this hart, this must not fail */
+
+ percpu = (riscv_percpu_t *)sq_remfirst(&g_freelist);
+ DEBUGASSERT(percpu);
+
+ /* Get interrupt stack (if any) */
+
+ stack_top = 0;
Review Comment:
remove
##########
arch/risc-v/src/common/riscv_percpu.c:
##########
@@ -86,13 +92,39 @@ void riscv_percpu_init(void)
void riscv_percpu_add_hart(uintptr_t hartid)
{
- /* Hart IDs go from 0...4 */
+ riscv_percpu_t *percpu;
+ uintptr_t stack_top;
+
+ /* Get free entry for this hart, this must not fail */
+
+ percpu = (riscv_percpu_t *)sq_remfirst(&g_freelist);
+ DEBUGASSERT(percpu);
+
+ /* Get interrupt stack (if any) */
+
+ stack_top = 0;
+
+#if CONFIG_ARCH_INTERRUPTSTACK > 15
+#if CONFIG_SMP_NCPUS > 1
+ /* Calculate offset first (stack size * hartid) */
- DEBUGASSERT(hartid < HART_CNT);
+ stack_top = (uintptr_t)&g_intstacktop - (uintptr_t)&g_intstackalloc
+ stack_top = stack_top * hartid;
+ stack_top = (uintptr_t)&g_intstacktop - stack_top;
Review Comment:
stack_top = (uintptr_t)&g_intstacktop - STACK_ALIGN_DOWN(CONFIG_ARCH_INTERRUPTSTACK) * hartid
##########
arch/risc-v/src/common/riscv_macros.S:
##########
@@ -29,6 +29,11 @@
#include <arch/arch.h>
#include <arch/irq.h>
+#include <sys/types.h>
Review Comment:
why need
##########
arch/risc-v/src/common/riscv_exception_common.S:
##########
@@ -28,8 +28,12 @@
#include <arch/irq.h>
#include <arch/mode.h>
+#include <sys/types.h>
Review Comment:
why need
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a diff in pull request #5985: RISC-V: Combine 3 variables that depend on CPU amount into one
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #5985:
URL: https://github.com/apache/incubator-nuttx/pull/5985#discussion_r844633149
##########
arch/risc-v/src/common/riscv_percpu.c:
##########
@@ -86,13 +92,39 @@ void riscv_percpu_init(void)
void riscv_percpu_add_hart(uintptr_t hartid)
{
- /* Hart IDs go from 0...4 */
+ riscv_percpu_t *percpu;
+ uintptr_t stack_top;
+
+ /* Get free entry for this hart, this must not fail */
+
+ percpu = (riscv_percpu_t *)sq_remfirst(&g_freelist);
+ DEBUGASSERT(percpu);
+
+ /* Get interrupt stack (if any) */
+
+ stack_top = 0;
Review Comment:
I mean remove line 105, your code at line 111 and 116 assign stack_top again. Why need assign stack_top to zero here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pussuw commented on a diff in pull request #5985: RISC-V: Combine 3 variables that depend on CPU amount into one
Posted by GitBox <gi...@apache.org>.
pussuw commented on code in PR #5985:
URL: https://github.com/apache/incubator-nuttx/pull/5985#discussion_r844953019
##########
arch/risc-v/src/common/riscv_percpu.c:
##########
@@ -86,13 +92,39 @@ void riscv_percpu_init(void)
void riscv_percpu_add_hart(uintptr_t hartid)
{
- /* Hart IDs go from 0...4 */
+ riscv_percpu_t *percpu;
+ uintptr_t stack_top;
+
+ /* Get free entry for this hart, this must not fail */
+
+ percpu = (riscv_percpu_t *)sq_remfirst(&g_freelist);
Review Comment:
Done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pussuw commented on a diff in pull request #5985: RISC-V: Combine 3 variables that depend on CPU amount into one
Posted by GitBox <gi...@apache.org>.
pussuw commented on code in PR #5985:
URL: https://github.com/apache/incubator-nuttx/pull/5985#discussion_r844745683
##########
arch/risc-v/src/common/riscv_macros.S:
##########
@@ -128,3 +133,22 @@
REGLOAD x31, REG_X31(\out) /* t6 */
.endm
+
+#if CONFIG_ARCH_INTERRUPTSTACK > 15
+.macro setintstack tmp0, tmp1
+#ifndef CONFIG_ARCH_USE_SMODE
+#if CONFIG_SMP_NCPUS > 1
+ csrr \tmp0, mhartid
Review Comment:
This code is also IMO wrong but the original code was not made by me. The problem is that SMP cpu index is not the same has the physical hart ID. And we do need both.
- CPU id is an incrementing number between 0...SMP_CPUs, needed e.g. for calculating the IRQ stack position
- hartid is the physical core number, needed for external interrupt handling (PLIC) at least
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pussuw commented on pull request #5985: RISC-V: Combine 3 variables that depend on CPU amount into one
Posted by GitBox <gi...@apache.org>.
pussuw commented on PR #5985:
URL: https://github.com/apache/incubator-nuttx/pull/5985#issuecomment-1091125762
Made a fixup error in rebase, restored now
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pussuw commented on a diff in pull request #5985: RISC-V: Combine 3 variables that depend on CPU amount into one
Posted by GitBox <gi...@apache.org>.
pussuw commented on code in PR #5985:
URL: https://github.com/apache/incubator-nuttx/pull/5985#discussion_r844899418
##########
arch/risc-v/src/common/riscv_percpu.c:
##########
@@ -86,13 +92,39 @@ void riscv_percpu_init(void)
void riscv_percpu_add_hart(uintptr_t hartid)
{
- /* Hart IDs go from 0...4 */
+ riscv_percpu_t *percpu;
+ uintptr_t stack_top;
+
+ /* Get free entry for this hart, this must not fail */
+
+ percpu = (riscv_percpu_t *)sq_remfirst(&g_freelist);
Review Comment:
Correct, the lock is needed and now added too
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a diff in pull request #5985: RISC-V: Combine 3 variables that depend on CPU amount into one
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #5985:
URL: https://github.com/apache/incubator-nuttx/pull/5985#discussion_r843034973
##########
arch/risc-v/src/mpfs/Kconfig:
##########
@@ -46,17 +46,12 @@ config MPFS_BOOTLOADER
---help---
This NuttX image is used as a bootloader, which will boot only on one hart, putting the others in WFI
-config IRQ_NSTACKS
- int
- depends on MPFS_BOOTLOADER
- default 5
-
config MPFS_OPENSBI
- bool "Use OpenSBI"
- depends on MPFS_BOOTLOADER && OPENSBI
- default n
- ---help---
- This uses a ld-envm-opensbi.script linker script and the mpfs_opensbi.c code to use external OpenSBI.
+ bool "Use OpenSBI"
Review Comment:
It's fine to keep it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pussuw commented on pull request #5985: RISC-V: Combine 3 variables that depend on CPU amount into one
Posted by GitBox <gi...@apache.org>.
pussuw commented on PR #5985:
URL: https://github.com/apache/incubator-nuttx/pull/5985#issuecomment-1092470678
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pussuw commented on a diff in pull request #5985: RISC-V: Combine 3 variables that depend on CPU amount into one
Posted by GitBox <gi...@apache.org>.
pussuw commented on code in PR #5985:
URL: https://github.com/apache/incubator-nuttx/pull/5985#discussion_r844301097
##########
arch/risc-v/src/common/riscv_percpu.c:
##########
@@ -86,13 +92,39 @@ void riscv_percpu_init(void)
void riscv_percpu_add_hart(uintptr_t hartid)
{
- /* Hart IDs go from 0...4 */
+ riscv_percpu_t *percpu;
+ uintptr_t stack_top;
+
+ /* Get free entry for this hart, this must not fail */
+
+ percpu = (riscv_percpu_t *)sq_remfirst(&g_freelist);
+ DEBUGASSERT(percpu);
+
+ /* Get interrupt stack (if any) */
+
+ stack_top = 0;
+
+#if CONFIG_ARCH_INTERRUPTSTACK > 15
+#if CONFIG_SMP_NCPUS > 1
+ /* Calculate offset first (stack size * hartid) */
- DEBUGASSERT(hartid < HART_CNT);
+ stack_top = (uintptr_t)&g_intstacktop - (uintptr_t)&g_intstackalloc
+ stack_top = stack_top * hartid;
+ stack_top = (uintptr_t)&g_intstacktop - stack_top;
Review Comment:
Yes this is wrong, will fix
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a diff in pull request #5985: RISC-V: Combine 3 variables that depend on CPU amount into one
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #5985:
URL: https://github.com/apache/incubator-nuttx/pull/5985#discussion_r844847862
##########
arch/risc-v/src/common/riscv_percpu.c:
##########
@@ -86,13 +92,39 @@ void riscv_percpu_init(void)
void riscv_percpu_add_hart(uintptr_t hartid)
{
- /* Hart IDs go from 0...4 */
+ riscv_percpu_t *percpu;
+ uintptr_t stack_top;
+
+ /* Get free entry for this hart, this must not fail */
+
+ percpu = (riscv_percpu_t *)sq_remfirst(&g_freelist);
Review Comment:
Here is my thought:
1. It's wrong to allocate Remove riscv_percpu from g_freelist without locking
2. Why not change riscv_percpu_add_hart prototype to:
```
void riscv_percpu_init_hart(int index, uintptr_t hartid);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a diff in pull request #5985: RISC-V: Combine 3 variables that depend on CPU amount into one
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #5985:
URL: https://github.com/apache/incubator-nuttx/pull/5985#discussion_r844897411
##########
arch/risc-v/src/common/riscv_percpu.c:
##########
@@ -86,13 +92,39 @@ void riscv_percpu_init(void)
void riscv_percpu_add_hart(uintptr_t hartid)
{
- /* Hart IDs go from 0...4 */
+ riscv_percpu_t *percpu;
+ uintptr_t stack_top;
+
+ /* Get free entry for this hart, this must not fail */
+
+ percpu = (riscv_percpu_t *)sq_remfirst(&g_freelist);
Review Comment:
Not about hotloading only, cpu1 and cpu2... will boot in parallel.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a diff in pull request #5985: RISC-V: Combine 3 variables that depend on CPU amount into one
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #5985:
URL: https://github.com/apache/incubator-nuttx/pull/5985#discussion_r844912065
##########
arch/risc-v/src/common/riscv_macros.S:
##########
@@ -128,3 +133,22 @@
REGLOAD x31, REG_X31(\out) /* t6 */
.endm
+
+#if CONFIG_ARCH_INTERRUPTSTACK > 15
+.macro setintstack tmp0, tmp1
+#ifndef CONFIG_ARCH_USE_SMODE
+#if CONFIG_SMP_NCPUS > 1
+ csrr \tmp0, mhartid
Review Comment:
It depends on how hardware/software design and config. In many simple design, hartid match index. If you want to support the index isn't equal to hartid. M mode also need the similar percpu struct. Since how to manage hartid is very hardware specific, that's why both riscv spec and NuttX left the detail to chip specific implementation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pussuw commented on a diff in pull request #5985: RISC-V: Combine 3 variables that depend on CPU amount into one
Posted by GitBox <gi...@apache.org>.
pussuw commented on code in PR #5985:
URL: https://github.com/apache/incubator-nuttx/pull/5985#discussion_r844915323
##########
arch/risc-v/src/common/riscv_percpu.c:
##########
@@ -86,13 +92,39 @@ void riscv_percpu_init(void)
void riscv_percpu_add_hart(uintptr_t hartid)
{
- /* Hart IDs go from 0...4 */
+ riscv_percpu_t *percpu;
+ uintptr_t stack_top;
+
+ /* Get free entry for this hart, this must not fail */
+
+ percpu = (riscv_percpu_t *)sq_remfirst(&g_freelist);
Review Comment:
_please use flags instead_ I don't know what you mean by flags. Can you give an example ?
I think having the init function and sl list is fine, as it also allows releasing a CPU. There are many other init functions that are called from the chip specific start routines, so I did not think this would be so different from those. And yes, the mpfs init must be fixed to work with SMP, all other init routines are executed on all harts too, so currently with SMP calling __mpfs_start() will fail miserably.
Using, the global variable then leaves the stack unallocated, and a similar mechanism would be needed for that, so it would converge into something like this:
uintptr_t g_init_index;
lock();
riscv_percpu_t *percpu = &g_percpu[g_init_index++];
unlock();
That implementation is something I tried to avoid by using sl list.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pussuw commented on a diff in pull request #5985: RISC-V: Combine 3 variables that depend on CPU amount into one
Posted by GitBox <gi...@apache.org>.
pussuw commented on code in PR #5985:
URL: https://github.com/apache/incubator-nuttx/pull/5985#discussion_r844920820
##########
arch/risc-v/src/common/riscv_macros.S:
##########
@@ -128,3 +133,22 @@
REGLOAD x31, REG_X31(\out) /* t6 */
.endm
+
+#if CONFIG_ARCH_INTERRUPTSTACK > 15
+.macro setintstack tmp0, tmp1
+#ifndef CONFIG_ARCH_USE_SMODE
+#if CONFIG_SMP_NCPUS > 1
+ csrr \tmp0, mhartid
Review Comment:
Of course, just something to keep in mind here. Can easily produce strange results.
A good example is MPFS, which has 5 harts (SiFive cores).
Core 0 is u51 which does not have S-mode, FPU, etc.
Cores 1-4 are u54, which have S-mode / FPU.
So running SMP on e.g. core0&1 makes handling the code difficult, because the cores have different capabilities ! This is why I run NuttX on core 4.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pussuw commented on a diff in pull request #5985: RISC-V: Combine 3 variables that depend on CPU amount into one
Posted by GitBox <gi...@apache.org>.
pussuw commented on code in PR #5985:
URL: https://github.com/apache/incubator-nuttx/pull/5985#discussion_r842968628
##########
arch/risc-v/src/mpfs/Kconfig:
##########
@@ -46,17 +46,12 @@ config MPFS_BOOTLOADER
---help---
This NuttX image is used as a bootloader, which will boot only on one hart, putting the others in WFI
-config IRQ_NSTACKS
Review Comment:
Yes, only SMP_NCPUS is needed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pussuw commented on a diff in pull request #5985: RISC-V: Combine 3 variables that depend on CPU amount into one
Posted by GitBox <gi...@apache.org>.
pussuw commented on code in PR #5985:
URL: https://github.com/apache/incubator-nuttx/pull/5985#discussion_r845079724
##########
arch/risc-v/src/common/riscv_percpu.c:
##########
@@ -86,13 +92,39 @@ void riscv_percpu_init(void)
void riscv_percpu_add_hart(uintptr_t hartid)
{
- /* Hart IDs go from 0...4 */
+ riscv_percpu_t *percpu;
+ uintptr_t stack_top;
+
+ /* Get free entry for this hart, this must not fail */
+
+ percpu = (riscv_percpu_t *)sq_remfirst(&g_freelist);
+ DEBUGASSERT(percpu);
+
+ /* Get interrupt stack (if any) */
+
+ stack_top = 0;
Review Comment:
Resolving as obsolete
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pussuw commented on a diff in pull request #5985: RISC-V: Combine 3 variables that depend on CPU amount into one
Posted by GitBox <gi...@apache.org>.
pussuw commented on code in PR #5985:
URL: https://github.com/apache/incubator-nuttx/pull/5985#discussion_r844298516
##########
arch/risc-v/src/common/riscv_percpu.c:
##########
@@ -86,13 +92,39 @@ void riscv_percpu_init(void)
void riscv_percpu_add_hart(uintptr_t hartid)
{
- /* Hart IDs go from 0...4 */
+ riscv_percpu_t *percpu;
+ uintptr_t stack_top;
+
+ /* Get free entry for this hart, this must not fail */
+
+ percpu = (riscv_percpu_t *)sq_remfirst(&g_freelist);
+ DEBUGASSERT(percpu);
+
+ /* Get interrupt stack (if any) */
+
+ stack_top = 0;
Review Comment:
stack_top = (uintptr_t)&g_intstacktop;
^- is this guaranteed to be 0/NULL if g_intstacktop is not defined, does the linker do relocation fix or something ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pussuw commented on a diff in pull request #5985: RISC-V: Combine 3 variables that depend on CPU amount into one
Posted by GitBox <gi...@apache.org>.
pussuw commented on code in PR #5985:
URL: https://github.com/apache/incubator-nuttx/pull/5985#discussion_r844738059
##########
arch/risc-v/src/common/riscv_percpu.c:
##########
@@ -86,13 +92,39 @@ void riscv_percpu_init(void)
void riscv_percpu_add_hart(uintptr_t hartid)
{
- /* Hart IDs go from 0...4 */
+ riscv_percpu_t *percpu;
+ uintptr_t stack_top;
+
+ /* Get free entry for this hart, this must not fail */
+
+ percpu = (riscv_percpu_t *)sq_remfirst(&g_freelist);
Review Comment:
Stack is now assigned to the list, removing dependency to hartid
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pussuw commented on a diff in pull request #5985: RISC-V: Combine 3 variables that depend on CPU amount into one
Posted by GitBox <gi...@apache.org>.
pussuw commented on code in PR #5985:
URL: https://github.com/apache/incubator-nuttx/pull/5985#discussion_r845030881
##########
arch/risc-v/src/common/riscv_percpu.c:
##########
@@ -86,13 +92,39 @@ void riscv_percpu_init(void)
void riscv_percpu_add_hart(uintptr_t hartid)
{
- /* Hart IDs go from 0...4 */
+ riscv_percpu_t *percpu;
+ uintptr_t stack_top;
+
+ /* Get free entry for this hart, this must not fail */
+
+ percpu = (riscv_percpu_t *)sq_remfirst(&g_freelist);
Review Comment:
Hmm, I don't think the spinlock does quite what I intended. Maybe I need to provide a handle. Need to investigate the code a bit more.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pussuw commented on a diff in pull request #5985: RISC-V: Combine 3 variables that depend on CPU amount into one
Posted by GitBox <gi...@apache.org>.
pussuw commented on code in PR #5985:
URL: https://github.com/apache/incubator-nuttx/pull/5985#discussion_r844725709
##########
arch/risc-v/src/common/riscv_percpu.c:
##########
@@ -86,13 +92,39 @@ void riscv_percpu_init(void)
void riscv_percpu_add_hart(uintptr_t hartid)
{
- /* Hart IDs go from 0...4 */
+ riscv_percpu_t *percpu;
+ uintptr_t stack_top;
+
+ /* Get free entry for this hart, this must not fail */
+
+ percpu = (riscv_percpu_t *)sq_remfirst(&g_freelist);
Review Comment:
Hmm yes, that was written based on the old NIRQSTACKS code but you're right, cannot use hartid for that.
We need two things:
- Hartid, the **real** hartid to claim and acknowledge PLIC interrupts
- CPU number, to calculate stack position
One option is to use sl_list for the stacks too, this way the CPU number is not needed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a diff in pull request #5985: RISC-V: Combine 3 variables that depend on CPU amount into one
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #5985:
URL: https://github.com/apache/incubator-nuttx/pull/5985#discussion_r844875955
##########
arch/risc-v/src/common/riscv_exception_common.S:
##########
@@ -28,8 +28,12 @@
#include <arch/irq.h>
#include <arch/mode.h>
+#include <sys/types.h>
Review Comment:
Ok, understand. It's because CONFIG_SMP_NCPUS isn't defined if CONFIG_SMP equal n.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a diff in pull request #5985: RISC-V: Combine 3 variables that depend on CPU amount into one
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #5985:
URL: https://github.com/apache/incubator-nuttx/pull/5985#discussion_r844897411
##########
arch/risc-v/src/common/riscv_percpu.c:
##########
@@ -86,13 +92,39 @@ void riscv_percpu_init(void)
void riscv_percpu_add_hart(uintptr_t hartid)
{
- /* Hart IDs go from 0...4 */
+ riscv_percpu_t *percpu;
+ uintptr_t stack_top;
+
+ /* Get free entry for this hart, this must not fail */
+
+ percpu = (riscv_percpu_t *)sq_remfirst(&g_freelist);
Review Comment:
> Answers:
>
> 1. Yes, you are right, need lock for sl list.
> 2. Then it gets hard to define index. Allocating from a free list does not require defining index.
>
If you really prefer the allocation, please use flags instead, so we don't need call riscv_percpu_init_hart. Or define a new global variable point to the next free slot:
```
static riscv_percpu_t g_percpu[HART_CNT];
static riscv_percpu_t *g_nextfree = g_percpu;
```
The final result is same, but without the additional initialization.
> For now the call is done only during init but I'll add the lock to allow hotloading CPUs.
Not about hotloading only, cpu1 and cpu2... will boot in parallel.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a diff in pull request #5985: RISC-V: Combine 3 variables that depend on CPU amount into one
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #5985:
URL: https://github.com/apache/incubator-nuttx/pull/5985#discussion_r844897411
##########
arch/risc-v/src/common/riscv_percpu.c:
##########
@@ -86,13 +92,39 @@ void riscv_percpu_init(void)
void riscv_percpu_add_hart(uintptr_t hartid)
{
- /* Hart IDs go from 0...4 */
+ riscv_percpu_t *percpu;
+ uintptr_t stack_top;
+
+ /* Get free entry for this hart, this must not fail */
+
+ percpu = (riscv_percpu_t *)sq_remfirst(&g_freelist);
Review Comment:
> Answers:
>
> 1. Yes, you are right, need lock for sl list.
> 2. Then it gets hard to define index. Allocating from a free list does not require defining index.
>
If you really prefer the allocation, please use flags instead, so we don't need call riscv_percpu_init_hart. Or define a new global variable point to the next free slot:
```
static riscv_percpu_t g_percpu[HART_CNT];
static riscv_percpu_t *g_nextfree = g_percpu;
```
> For now the call is done only during init but I'll add the lock to allow hotloading CPUs.
Not about hotloading only, cpu1 and cpu2... will boot in parallel.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pussuw commented on a diff in pull request #5985: RISC-V: Combine 3 variables that depend on CPU amount into one
Posted by GitBox <gi...@apache.org>.
pussuw commented on code in PR #5985:
URL: https://github.com/apache/incubator-nuttx/pull/5985#discussion_r842664179
##########
arch/risc-v/src/common/riscv_exception_common.S:
##########
@@ -91,24 +93,12 @@ exception_common:
mv a1, sp /* context = sp */
#if CONFIG_ARCH_INTERRUPTSTACK > 15
-
- /* Offset to hartid */
-
- mv s0, a0 /* save cause */
- jal x1, riscv_mhartid /* get hartid */
-
/* Switch to interrupt stack */
-#if IRQ_NSTACKS > 1
- li t0, (CONFIG_ARCH_INTERRUPTSTACK & ~15)
- mul t0, a0, t0
- la a0, g_intstacktop
- sub sp, a0, t0
-#else
- la sp, g_intstacktop
-#endif
-
- mv a0, s0 /* restore cause */
+ mv t0, a0 /* save cause */
+ jal x1, riscv_percpu_get_irqstack
Review Comment:
This could be wrapped so that the percpu module is not a strict requirement here
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pussuw commented on pull request #5985: RISC-V: Combine 3 variables that depend on CPU amount into one
Posted by GitBox <gi...@apache.org>.
pussuw commented on PR #5985:
URL: https://github.com/apache/incubator-nuttx/pull/5985#issuecomment-1089176386
> > > 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.
But maybe I can make uintptr_t riscv_irqstack(void) into an asm macro instead.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pussuw commented on a diff in pull request #5985: RISC-V: Combine 3 variables that depend on CPU amount into one
Posted by GitBox <gi...@apache.org>.
pussuw commented on code in PR #5985:
URL: https://github.com/apache/incubator-nuttx/pull/5985#discussion_r844296170
##########
arch/risc-v/src/common/riscv_exception_common.S:
##########
@@ -28,8 +28,12 @@
#include <arch/irq.h>
#include <arch/mode.h>
+#include <sys/types.h>
Review Comment:
CONFIG_SMP_NCPUS
##########
arch/risc-v/src/common/riscv_macros.S:
##########
@@ -29,6 +29,11 @@
#include <arch/arch.h>
#include <arch/irq.h>
+#include <sys/types.h>
Review Comment:
CONFIG_SMP_NCPUS
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pussuw commented on a diff in pull request #5985: RISC-V: Combine 3 variables that depend on CPU amount into one
Posted by GitBox <gi...@apache.org>.
pussuw commented on code in PR #5985:
URL: https://github.com/apache/incubator-nuttx/pull/5985#discussion_r844298516
##########
arch/risc-v/src/common/riscv_percpu.c:
##########
@@ -86,13 +92,39 @@ void riscv_percpu_init(void)
void riscv_percpu_add_hart(uintptr_t hartid)
{
- /* Hart IDs go from 0...4 */
+ riscv_percpu_t *percpu;
+ uintptr_t stack_top;
+
+ /* Get free entry for this hart, this must not fail */
+
+ percpu = (riscv_percpu_t *)sq_remfirst(&g_freelist);
+ DEBUGASSERT(percpu);
+
+ /* Get interrupt stack (if any) */
+
+ stack_top = 0;
Review Comment:
stack_top = (uintptr_t)&g_intstacktop;
^- is this guaranteed to be 0/NULL if g_intstacktop is not provided by the linker ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pussuw commented on a diff in pull request #5985: RISC-V: Combine 3 variables that depend on CPU amount into one
Posted by GitBox <gi...@apache.org>.
pussuw commented on code in PR #5985:
URL: https://github.com/apache/incubator-nuttx/pull/5985#discussion_r844745683
##########
arch/risc-v/src/common/riscv_macros.S:
##########
@@ -128,3 +133,22 @@
REGLOAD x31, REG_X31(\out) /* t6 */
.endm
+
+#if CONFIG_ARCH_INTERRUPTSTACK > 15
+.macro setintstack tmp0, tmp1
+#ifndef CONFIG_ARCH_USE_SMODE
+#if CONFIG_SMP_NCPUS > 1
+ csrr \tmp0, mhartid
Review Comment:
This code is also IMO wrong but the original code was not made by me. The problem is that SMP cpu index is not the same has the physical hart ID. And we do need both.
- CPU id is an incrementing number between 0...SMP_CPUS-1, needed e.g. for calculating the IRQ stack position
- hartid is the physical core number, needed for external interrupt handling (PLIC) at least
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pussuw commented on a diff in pull request #5985: RISC-V: Combine 3 variables that depend on CPU amount into one
Posted by GitBox <gi...@apache.org>.
pussuw commented on code in PR #5985:
URL: https://github.com/apache/incubator-nuttx/pull/5985#discussion_r844883591
##########
arch/risc-v/src/common/riscv_percpu.c:
##########
@@ -86,13 +92,39 @@ void riscv_percpu_init(void)
void riscv_percpu_add_hart(uintptr_t hartid)
{
- /* Hart IDs go from 0...4 */
+ riscv_percpu_t *percpu;
+ uintptr_t stack_top;
+
+ /* Get free entry for this hart, this must not fail */
+
+ percpu = (riscv_percpu_t *)sq_remfirst(&g_freelist);
Review Comment:
Answers:
1. Yes, you are right, need lock for sl list.
2. Then it gets hard to define index. Allocating from a free list does not require defining index.
For now the call is done only during init but I'll add the lock to allow hotloading CPUs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pussuw commented on a diff in pull request #5985: RISC-V: Combine 3 variables that depend on CPU amount into one
Posted by GitBox <gi...@apache.org>.
pussuw commented on code in PR #5985:
URL: https://github.com/apache/incubator-nuttx/pull/5985#discussion_r844952193
##########
arch/risc-v/src/common/riscv_macros.S:
##########
@@ -128,3 +133,22 @@
REGLOAD x31, REG_X31(\out) /* t6 */
.endm
+
+#if CONFIG_ARCH_INTERRUPTSTACK > 15
+.macro setintstack tmp0, tmp1
+#ifndef CONFIG_ARCH_USE_SMODE
+#if CONFIG_SMP_NCPUS > 1
+ csrr \tmp0, mhartid
Review Comment:
I think all (or at least many) risc-v targets use interrupt stack now even without SMP. This macro will work for them, because setting the interrupt stack was just:
exception_common:
` la sp, g_intstacktop`
This does not quite work either, because setinstack for mpfs is needed when 1 hart is used (to use the percpu structure):
exception_common:
```
#if CONFIG_SMP_NCPUS > 1
setintstack t0, t1
#else
la sp, g_intstacktop
#endif
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on a diff in pull request #5985: RISC-V: Combine 3 variables that depend on CPU amount into one
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on code in PR #5985:
URL: https://github.com/apache/incubator-nuttx/pull/5985#discussion_r846031942
##########
arch/risc-v/src/mpfs/chip.h:
##########
@@ -29,4 +29,33 @@
#include "mpfs_memorymap.h"
+#include "riscv_percpu.h"
+
+/****************************************************************************
+ * Macro Definitions
+ ****************************************************************************/
+
+#ifdef __ASSEMBLY__
+
+/****************************************************************************
+ * Name: setintstack
+ *
+ * Description:
+ * Set the current stack pointer to the "top" the correct interrupt stack
+ * for the current CPU.
+ *
+ ****************************************************************************/
+
+#if CONFIG_ARCH_INTERRUPTSTACK > 15
+.macro setintstack tmp0, tmp1
+#ifndef CONFIG_ARCH_USE_S_MODE
Review Comment:
Looks like the definition of the macro needs to be inside the second `#ifdef`, otherwise we are ending up with two definitions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pussuw commented on a diff in pull request #5985: RISC-V: Combine 3 variables that depend on CPU amount into one
Posted by GitBox <gi...@apache.org>.
pussuw commented on code in PR #5985:
URL: https://github.com/apache/incubator-nuttx/pull/5985#discussion_r842667519
##########
arch/risc-v/src/common/riscv_exception_common.S:
##########
@@ -91,24 +93,12 @@ exception_common:
mv a1, sp /* context = sp */
#if CONFIG_ARCH_INTERRUPTSTACK > 15
-
- /* Offset to hartid */
-
- mv s0, a0 /* save cause */
- jal x1, riscv_mhartid /* get hartid */
-
/* Switch to interrupt stack */
-#if IRQ_NSTACKS > 1
- li t0, (CONFIG_ARCH_INTERRUPTSTACK & ~15)
- mul t0, a0, t0
- la a0, g_intstacktop
- sub sp, a0, t0
-#else
- la sp, g_intstacktop
-#endif
-
- mv a0, s0 /* restore cause */
+ mv t0, a0 /* save cause */
+ jal x1, riscv_percpu_get_irqstack
+ mv sp, a0
+ csrr a0, t0 /* restore cause */
Review Comment:
Please ignore this, I did not even try to compile so I have missed many details
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #5985: RISC-V: Combine 3 variables that depend on CPU amount into one
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on PR #5985:
URL: https://github.com/apache/incubator-nuttx/pull/5985#issuecomment-1088752086
> @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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a diff in pull request #5985: RISC-V: Combine 3 variables that depend on CPU amount into one
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #5985:
URL: https://github.com/apache/incubator-nuttx/pull/5985#discussion_r844631656
##########
arch/risc-v/src/common/riscv_percpu.c:
##########
@@ -86,13 +92,39 @@ void riscv_percpu_init(void)
void riscv_percpu_add_hart(uintptr_t hartid)
{
- /* Hart IDs go from 0...4 */
+ riscv_percpu_t *percpu;
+ uintptr_t stack_top;
+
+ /* Get free entry for this hart, this must not fail */
+
+ percpu = (riscv_percpu_t *)sq_remfirst(&g_freelist);
Review Comment:
but your code use hartid as index to compute stack location.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a diff in pull request #5985: RISC-V: Combine 3 variables that depend on CPU amount into one
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #5985:
URL: https://github.com/apache/incubator-nuttx/pull/5985#discussion_r844630442
##########
arch/risc-v/src/common/riscv_exception_common.S:
##########
@@ -28,8 +28,12 @@
#include <arch/irq.h>
#include <arch/mode.h>
+#include <sys/types.h>
Review Comment:
But this macro isn't defined in sys/types.h
##########
arch/risc-v/src/common/riscv_macros.S:
##########
@@ -29,6 +29,11 @@
#include <arch/arch.h>
#include <arch/irq.h>
+#include <sys/types.h>
Review Comment:
ditto
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pussuw commented on a diff in pull request #5985: RISC-V: Combine 3 variables that depend on CPU amount into one
Posted by GitBox <gi...@apache.org>.
pussuw commented on code in PR #5985:
URL: https://github.com/apache/incubator-nuttx/pull/5985#discussion_r844726933
##########
arch/risc-v/src/common/riscv_exception_common.S:
##########
@@ -28,8 +28,12 @@
#include <arch/irq.h>
#include <arch/mode.h>
+#include <sys/types.h>
Review Comment:
From sys/types.h, lines 63-65
```
#ifndef CONFIG_SMP_NCPUS
# define CONFIG_SMP_NCPUS 1
#endif
```
I'm not saying I fully understand why this is here and not in config.h, but it is there now
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pussuw commented on a diff in pull request #5985: RISC-V: Combine 3 variables that depend on CPU amount into one
Posted by GitBox <gi...@apache.org>.
pussuw commented on code in PR #5985:
URL: https://github.com/apache/incubator-nuttx/pull/5985#discussion_r844915323
##########
arch/risc-v/src/common/riscv_percpu.c:
##########
@@ -86,13 +92,39 @@ void riscv_percpu_init(void)
void riscv_percpu_add_hart(uintptr_t hartid)
{
- /* Hart IDs go from 0...4 */
+ riscv_percpu_t *percpu;
+ uintptr_t stack_top;
+
+ /* Get free entry for this hart, this must not fail */
+
+ percpu = (riscv_percpu_t *)sq_remfirst(&g_freelist);
Review Comment:
_please use flags instead_ I don't know what you mean by flags. Can you give an example ?
I think having the init function and sl list is fine, as it also allows releasing a CPU. There are many other init functions that are called from the chip specific start routines, so I did not think this would be so different from those.
Using, the global variable then leaves the stack unallocated, and a similar mechanism would be needed for that, so it would converge into something like this:
uintptr_t g_init_index;
lock();
riscv_percpu_t *percpu = &g_percpu[g_init_index++];
unlock();
That implementation is something I tried to avoid by using sl list.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #5985: RISC-V: Combine 3 variables that depend on CPU amount into one
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #5985:
URL: https://github.com/apache/incubator-nuttx/pull/5985#discussion_r845538189
##########
arch/risc-v/src/mpfs/Kconfig:
##########
@@ -46,17 +46,12 @@ config MPFS_BOOTLOADER
---help---
This NuttX image is used as a bootloader, which will boot only on one hart, putting the others in WFI
-config IRQ_NSTACKS
- int
- depends on MPFS_BOOTLOADER
- default 5
-
config MPFS_OPENSBI
- bool "Use OpenSBI"
- depends on MPFS_BOOTLOADER && OPENSBI
- default n
- ---help---
- This uses a ld-envm-opensbi.script linker script and the mpfs_opensbi.c code to use external OpenSBI.
+ bool "Use OpenSBI"
+ depends on MPFS_BOOTLOADER && OPENSBI
+ default n
+ ---help---
+ This uses a ld-envm-opensbi.script linker script and the mpfs_opensbi.c code to use external OpenSBI.
Review Comment:
Please switch back to TABs here
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pussuw commented on pull request #5985: RISC-V: Combine 3 variables that depend on CPU amount into one
Posted by GitBox <gi...@apache.org>.
pussuw commented on PR #5985:
URL: https://github.com/apache/incubator-nuttx/pull/5985#issuecomment-1092503108
Still seems to have some issue with the setintstack macro, the wrong macro is used
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pussuw commented on a diff in pull request #5985: RISC-V: Combine 3 variables that depend on CPU amount into one
Posted by GitBox <gi...@apache.org>.
pussuw commented on code in PR #5985:
URL: https://github.com/apache/incubator-nuttx/pull/5985#discussion_r844737670
##########
arch/risc-v/src/common/riscv_percpu.c:
##########
@@ -120,3 +152,24 @@ uintptr_t riscv_percpu_get_hartid(void)
return ((struct riscv_percpu_s *)scratch)->hartid;
Review Comment:
Done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a diff in pull request #5985: RISC-V: Combine 3 variables that depend on CPU amount into one
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #5985:
URL: https://github.com/apache/incubator-nuttx/pull/5985#discussion_r844732822
##########
arch/risc-v/src/common/riscv_percpu.c:
##########
@@ -86,13 +92,39 @@ void riscv_percpu_init(void)
void riscv_percpu_add_hart(uintptr_t hartid)
{
- /* Hart IDs go from 0...4 */
+ riscv_percpu_t *percpu;
+ uintptr_t stack_top;
+
+ /* Get free entry for this hart, this must not fail */
+
+ percpu = (riscv_percpu_t *)sq_remfirst(&g_freelist);
+ DEBUGASSERT(percpu);
+
+ /* Get interrupt stack (if any) */
+
+ stack_top = 0;
Review Comment:
Sorry, I miss the CONFIG_ARCH_INTERRUPTSTACK check, but should we remove irq_stack field in this case?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pussuw commented on a diff in pull request #5985: RISC-V: Combine 3 variables that depend on CPU amount into one
Posted by GitBox <gi...@apache.org>.
pussuw commented on code in PR #5985:
URL: https://github.com/apache/incubator-nuttx/pull/5985#discussion_r845030881
##########
arch/risc-v/src/common/riscv_percpu.c:
##########
@@ -86,13 +92,39 @@ void riscv_percpu_init(void)
void riscv_percpu_add_hart(uintptr_t hartid)
{
- /* Hart IDs go from 0...4 */
+ riscv_percpu_t *percpu;
+ uintptr_t stack_top;
+
+ /* Get free entry for this hart, this must not fail */
+
+ percpu = (riscv_percpu_t *)sq_remfirst(&g_freelist);
Review Comment:
Hmm, I don't think the spinlock does quite what I intended. It does not synchronize, need to wait for init to complete. Still needs fixing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pussuw commented on a diff in pull request #5985: RISC-V: Combine 3 variables that depend on CPU amount into one
Posted by GitBox <gi...@apache.org>.
pussuw commented on code in PR #5985:
URL: https://github.com/apache/incubator-nuttx/pull/5985#discussion_r844915323
##########
arch/risc-v/src/common/riscv_percpu.c:
##########
@@ -86,13 +92,39 @@ void riscv_percpu_init(void)
void riscv_percpu_add_hart(uintptr_t hartid)
{
- /* Hart IDs go from 0...4 */
+ riscv_percpu_t *percpu;
+ uintptr_t stack_top;
+
+ /* Get free entry for this hart, this must not fail */
+
+ percpu = (riscv_percpu_t *)sq_remfirst(&g_freelist);
Review Comment:
_please use flags instead_ I don't know what you mean by flags. Can you give an example ?
I think having the init function and sl list is fine, as it also allows releasing a CPU. There are many other init functions that are called from the chip specific start routines, so I did not think this would be so different from those. And yes, `__mpfs_start()` must be fixed to work with SMP. All other init routines are also executed on all harts too, so currently with SMP calling `__mpfs_start()` will fail miserably.
Using, the global variable then leaves the stack unallocated, and a similar mechanism would be needed for that, so it would converge into something like this:
uintptr_t g_init_index;
lock();
riscv_percpu_t *percpu = &g_percpu[g_init_index++];
unlock();
That implementation is something I tried to avoid by using sl list.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a diff in pull request #5985: RISC-V: Combine 3 variables that depend on CPU amount into one
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #5985:
URL: https://github.com/apache/incubator-nuttx/pull/5985#discussion_r844946571
##########
arch/risc-v/src/common/riscv_macros.S:
##########
@@ -128,3 +133,22 @@
REGLOAD x31, REG_X31(\out) /* t6 */
.endm
+
+#if CONFIG_ARCH_INTERRUPTSTACK > 15
+.macro setintstack tmp0, tmp1
+#ifndef CONFIG_ARCH_USE_SMODE
+#if CONFIG_SMP_NCPUS > 1
+ csrr \tmp0, mhartid
Review Comment:
It's by design. since in the SMP case, the user must provide setintstack before they can enable interrupt stack.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pussuw commented on a diff in pull request #5985: RISC-V: Combine 3 variables that depend on CPU amount into one
Posted by GitBox <gi...@apache.org>.
pussuw commented on code in PR #5985:
URL: https://github.com/apache/incubator-nuttx/pull/5985#discussion_r844952193
##########
arch/risc-v/src/common/riscv_macros.S:
##########
@@ -128,3 +133,22 @@
REGLOAD x31, REG_X31(\out) /* t6 */
.endm
+
+#if CONFIG_ARCH_INTERRUPTSTACK > 15
+.macro setintstack tmp0, tmp1
+#ifndef CONFIG_ARCH_USE_SMODE
+#if CONFIG_SMP_NCPUS > 1
+ csrr \tmp0, mhartid
Review Comment:
I think all (or at least many) risc-v targets use interrupt stack now even without SMP. This macro will work for them, because before setting the interrupt stack was just:
exception_common:
` la sp, g_intstacktop`
This does not quite work either, because setinstack for mpfs is needed when 1 hart is used (to use the percpu structure):
exception_common:
```
#if CONFIG_SMP_NCPUS > 1
setintstack t0, t1
#else
la sp, g_intstacktop
#endif
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a diff in pull request #5985: RISC-V: Combine 3 variables that depend on CPU amount into one
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #5985:
URL: https://github.com/apache/incubator-nuttx/pull/5985#discussion_r842835933
##########
arch/risc-v/src/mpfs/Kconfig:
##########
@@ -46,17 +46,12 @@ config MPFS_BOOTLOADER
---help---
This NuttX image is used as a bootloader, which will boot only on one hart, putting the others in WFI
-config IRQ_NSTACKS
Review Comment:
look like you remove both IRQ_NSTACKS and ARCH_CPU_COUNT, is it enough to only use SMP_NCPUS for your case?
##########
arch/risc-v/src/mpfs/Kconfig:
##########
@@ -46,17 +46,12 @@ config MPFS_BOOTLOADER
---help---
This NuttX image is used as a bootloader, which will boot only on one hart, putting the others in WFI
-config IRQ_NSTACKS
- int
- depends on MPFS_BOOTLOADER
- default 5
-
config MPFS_OPENSBI
- bool "Use OpenSBI"
- depends on MPFS_BOOTLOADER && OPENSBI
- default n
- ---help---
- This uses a ld-envm-opensbi.script linker script and the mpfs_opensbi.c code to use external OpenSBI.
+ bool "Use OpenSBI"
Review Comment:
don't need change these lines
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pussuw commented on a diff in pull request #5985: RISC-V: Combine 3 variables that depend on CPU amount into one
Posted by GitBox <gi...@apache.org>.
pussuw commented on code in PR #5985:
URL: https://github.com/apache/incubator-nuttx/pull/5985#discussion_r842968008
##########
arch/risc-v/src/mpfs/Kconfig:
##########
@@ -46,17 +46,12 @@ config MPFS_BOOTLOADER
---help---
This NuttX image is used as a bootloader, which will boot only on one hart, putting the others in WFI
-config IRQ_NSTACKS
- int
- depends on MPFS_BOOTLOADER
- default 5
-
config MPFS_OPENSBI
- bool "Use OpenSBI"
- depends on MPFS_BOOTLOADER && OPENSBI
- default n
- ---help---
- This uses a ld-envm-opensbi.script linker script and the mpfs_opensbi.c code to use external OpenSBI.
+ bool "Use OpenSBI"
Review Comment:
Yeah but decided to beautify them to match indentation with the rest of the file since I'm touching this file anyway. I can of course remove the change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #5985: RISC-V: Combine 3 variables that depend on CPU amount into one
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on PR #5985:
URL: https://github.com/apache/incubator-nuttx/pull/5985#issuecomment-1089231107
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on a diff in pull request #5985: RISC-V: Combine 3 variables that depend on CPU amount into one
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on code in PR #5985:
URL: https://github.com/apache/incubator-nuttx/pull/5985#discussion_r846035317
##########
arch/risc-v/src/qemu-rv/Make.defs:
##########
@@ -34,8 +34,7 @@ CMN_CSRCS += riscv_releasepending.c riscv_reprioritizertr.c
CMN_CSRCS += riscv_releasestack.c riscv_stackframe.c riscv_schedulesigaction.c
CMN_CSRCS += riscv_sigdeliver.c riscv_unblocktask.c riscv_usestack.c
CMN_CSRCS += riscv_idle.c riscv_tcbinfo.c riscv_cpuidlestack.c
-CMN_CSRCS += riscv_exception.c riscv_getnewintctx.c riscv_doirq.c
-CMN_CSRCS += riscv_cpuindex.c
+CMN_CSRCS += riscv_fault.c riscv_getnewintctx.c riscv_doirq.c
Review Comment:
Is `riscv_fault.c` missing or that was supposed to be `riscv_exception`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a diff in pull request #5985: RISC-V: Combine 3 variables that depend on CPU amount into one
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #5985:
URL: https://github.com/apache/incubator-nuttx/pull/5985#discussion_r844897411
##########
arch/risc-v/src/common/riscv_percpu.c:
##########
@@ -86,13 +92,39 @@ void riscv_percpu_init(void)
void riscv_percpu_add_hart(uintptr_t hartid)
{
- /* Hart IDs go from 0...4 */
+ riscv_percpu_t *percpu;
+ uintptr_t stack_top;
+
+ /* Get free entry for this hart, this must not fail */
+
+ percpu = (riscv_percpu_t *)sq_remfirst(&g_freelist);
Review Comment:
> Answers:
>
> 1. Yes, you are right, need lock for sl list.
> 2. Then it gets hard to define index. Allocating from a free list does not require defining index.
>
If you really prefer the allocation, please use flags instead, so we don't need call riscv_percpu_init_hart.
> For now the call is done only during init but I'll add the lock to allow hotloading CPUs.
Not about hotloading only, cpu1 and cpu2... will boot in parallel.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a diff in pull request #5985: RISC-V: Combine 3 variables that depend on CPU amount into one
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #5985:
URL: https://github.com/apache/incubator-nuttx/pull/5985#discussion_r844925267
##########
arch/risc-v/src/common/riscv_percpu.c:
##########
@@ -86,13 +92,39 @@ void riscv_percpu_init(void)
void riscv_percpu_add_hart(uintptr_t hartid)
{
- /* Hart IDs go from 0...4 */
+ riscv_percpu_t *percpu;
+ uintptr_t stack_top;
+
+ /* Get free entry for this hart, this must not fail */
+
+ percpu = (riscv_percpu_t *)sq_remfirst(&g_freelist);
Review Comment:
> _please use flags instead_ I don't know what you mean by flags. Can you give an example ?
>
> I think having the init function and sl list is fine, as it also allows releasing a CPU. There are many other init functions that are called from the chip specific start routines, so I did not think this would be so different from those. And yes, `__mpfs_start()` must be fixed to work with SMP. All other init routines are also executed on all harts too, so currently with SMP calling `__mpfs_start()` will fail miserably.
>
> Using, the global variable then leaves the stack unallocated, and a similar mechanism would be needed for that, so it would converge into something like this:
>
> uintptr_t g_init_index; lock(); riscv_percpu_t *percpu = &g_percpu[g_init_index++]; unlock();
>
> That implementation is something I tried to avoid by using sl list.
Even using sl list, you can do the initialization when riscv_percpu_add_hart is called first time to simplify the use.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pussuw commented on a diff in pull request #5985: RISC-V: Combine 3 variables that depend on CPU amount into one
Posted by GitBox <gi...@apache.org>.
pussuw commented on code in PR #5985:
URL: https://github.com/apache/incubator-nuttx/pull/5985#discussion_r844926348
##########
arch/risc-v/src/common/riscv_percpu.c:
##########
@@ -86,13 +92,39 @@ void riscv_percpu_init(void)
void riscv_percpu_add_hart(uintptr_t hartid)
{
- /* Hart IDs go from 0...4 */
+ riscv_percpu_t *percpu;
+ uintptr_t stack_top;
+
+ /* Get free entry for this hart, this must not fail */
+
+ percpu = (riscv_percpu_t *)sq_remfirst(&g_freelist);
Review Comment:
> Even using sl list, you can do the initialization when riscv_percpu_add_hart is called first time to simplify the use.
Ok, that I can do. So the first who arrives does the init.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pussuw commented on a diff in pull request #5985: RISC-V: Combine 3 variables that depend on CPU amount into one
Posted by GitBox <gi...@apache.org>.
pussuw commented on code in PR #5985:
URL: https://github.com/apache/incubator-nuttx/pull/5985#discussion_r844318727
##########
arch/risc-v/src/common/riscv_percpu.c:
##########
@@ -86,13 +92,39 @@ void riscv_percpu_init(void)
void riscv_percpu_add_hart(uintptr_t hartid)
{
- /* Hart IDs go from 0...4 */
+ riscv_percpu_t *percpu;
+ uintptr_t stack_top;
+
+ /* Get free entry for this hart, this must not fail */
+
+ percpu = (riscv_percpu_t *)sq_remfirst(&g_freelist);
+ DEBUGASSERT(percpu);
+
+ /* Get interrupt stack (if any) */
+
+ stack_top = 0;
+
+#if CONFIG_ARCH_INTERRUPTSTACK > 15
+#if CONFIG_SMP_NCPUS > 1
+ /* Calculate offset first (stack size * hartid) */
- DEBUGASSERT(hartid < HART_CNT);
+ stack_top = (uintptr_t)&g_intstacktop - (uintptr_t)&g_intstackalloc
+ stack_top = stack_top * hartid;
+ stack_top = (uintptr_t)&g_intstacktop - stack_top;
+#else
+ UNUSED(hartid);
+ stack_top = (uintptr_t)&g_intstacktop;
+#endif
+#endif
+
+ /* Assign hartid and stack top */
+
+ percpu->hartid = hartid;
+ percpu->irq_stack = stack_top;
Review Comment:
If I remove initialization of stack_top from above, won't it put an uninitialized value here ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pussuw commented on a diff in pull request #5985: RISC-V: Combine 3 variables that depend on CPU amount into one
Posted by GitBox <gi...@apache.org>.
pussuw commented on code in PR #5985:
URL: https://github.com/apache/incubator-nuttx/pull/5985#discussion_r844301630
##########
arch/risc-v/src/common/riscv_percpu.c:
##########
@@ -120,3 +152,24 @@ uintptr_t riscv_percpu_get_hartid(void)
return ((struct riscv_percpu_s *)scratch)->hartid;
Review Comment:
Sure!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pussuw commented on a diff in pull request #5985: RISC-V: Combine 3 variables that depend on CPU amount into one
Posted by GitBox <gi...@apache.org>.
pussuw commented on code in PR #5985:
URL: https://github.com/apache/incubator-nuttx/pull/5985#discussion_r844739125
##########
arch/risc-v/src/common/riscv_percpu.c:
##########
@@ -86,13 +92,39 @@ void riscv_percpu_init(void)
void riscv_percpu_add_hart(uintptr_t hartid)
{
- /* Hart IDs go from 0...4 */
+ riscv_percpu_t *percpu;
+ uintptr_t stack_top;
+
+ /* Get free entry for this hart, this must not fail */
+
+ percpu = (riscv_percpu_t *)sq_remfirst(&g_freelist);
+ DEBUGASSERT(percpu);
+
+ /* Get interrupt stack (if any) */
+
+ stack_top = 0;
Review Comment:
Please, check the new patch. The old patch was just wrong. Using hartid does not work for calculating stack position.
It is a fully valid configuration to run NuttX with 2 cores and select harts 3 and 4. This will obviously fail.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pussuw commented on a diff in pull request #5985: RISC-V: Combine 3 variables that depend on CPU amount into one
Posted by GitBox <gi...@apache.org>.
pussuw commented on code in PR #5985:
URL: https://github.com/apache/incubator-nuttx/pull/5985#discussion_r844739401
##########
arch/risc-v/src/common/riscv_percpu.c:
##########
@@ -86,13 +92,39 @@ void riscv_percpu_init(void)
void riscv_percpu_add_hart(uintptr_t hartid)
{
- /* Hart IDs go from 0...4 */
+ riscv_percpu_t *percpu;
+ uintptr_t stack_top;
+
+ /* Get free entry for this hart, this must not fail */
+
+ percpu = (riscv_percpu_t *)sq_remfirst(&g_freelist);
+ DEBUGASSERT(percpu);
+
+ /* Get interrupt stack (if any) */
+
+ stack_top = 0;
+
+#if CONFIG_ARCH_INTERRUPTSTACK > 15
+#if CONFIG_SMP_NCPUS > 1
+ /* Calculate offset first (stack size * hartid) */
- DEBUGASSERT(hartid < HART_CNT);
+ stack_top = (uintptr_t)&g_intstacktop - (uintptr_t)&g_intstackalloc
+ stack_top = stack_top * hartid;
+ stack_top = (uintptr_t)&g_intstacktop - stack_top;
Review Comment:
Done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pussuw commented on a diff in pull request #5985: RISC-V: Combine 3 variables that depend on CPU amount into one
Posted by GitBox <gi...@apache.org>.
pussuw commented on code in PR #5985:
URL: https://github.com/apache/incubator-nuttx/pull/5985#discussion_r844722573
##########
arch/risc-v/src/common/riscv_percpu.c:
##########
@@ -86,13 +92,39 @@ void riscv_percpu_init(void)
void riscv_percpu_add_hart(uintptr_t hartid)
{
- /* Hart IDs go from 0...4 */
+ riscv_percpu_t *percpu;
+ uintptr_t stack_top;
+
+ /* Get free entry for this hart, this must not fail */
+
+ percpu = (riscv_percpu_t *)sq_remfirst(&g_freelist);
+ DEBUGASSERT(percpu);
+
+ /* Get interrupt stack (if any) */
+
+ stack_top = 0;
Review Comment:
Both assignments are dependent on this:
`#if CONFIG_ARCH_INTERRUPTSTACK > 15`
Which means this:
`percpu->irq_stack = stack_top;`
Would use an uninitialized value.
I can of course remove the temp variable altogether, but to me this current implementation is fine too (need to fix the bug below).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pussuw commented on a diff in pull request #5985: RISC-V: Combine 3 variables that depend on CPU amount into one
Posted by GitBox <gi...@apache.org>.
pussuw commented on code in PR #5985:
URL: https://github.com/apache/incubator-nuttx/pull/5985#discussion_r844883591
##########
arch/risc-v/src/common/riscv_percpu.c:
##########
@@ -86,13 +92,39 @@ void riscv_percpu_init(void)
void riscv_percpu_add_hart(uintptr_t hartid)
{
- /* Hart IDs go from 0...4 */
+ riscv_percpu_t *percpu;
+ uintptr_t stack_top;
+
+ /* Get free entry for this hart, this must not fail */
+
+ percpu = (riscv_percpu_t *)sq_remfirst(&g_freelist);
Review Comment:
Answers:
1. Yes, you are right, need lock for sl list.
2. The it gets hard to define index. Allocating from a free list does not require defining index.
For now the call is done only during init but I'll add the lock to allow hotloading CPUs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pussuw commented on a diff in pull request #5985: RISC-V: Combine 3 variables that depend on CPU amount into one
Posted by GitBox <gi...@apache.org>.
pussuw commented on code in PR #5985:
URL: https://github.com/apache/incubator-nuttx/pull/5985#discussion_r847225063
##########
arch/risc-v/src/common/riscv_percpu.c:
##########
@@ -86,13 +123,27 @@ void riscv_percpu_init(void)
void riscv_percpu_add_hart(uintptr_t hartid)
{
- /* Hart IDs go from 0...4 */
+ riscv_percpu_t *percpu;
+ irqstate_t flags;
+
+ /* Make sure we are initialized */
+
+ riscv_percpu_init();
- DEBUGASSERT(hartid < HART_CNT);
+ /* Get free entry for this hart, this must not fail */
+
+ flags = spin_lock_irqsave(NULL);
Review Comment:
Currently only called during init phase.
But using a local lock should be possible as the only thing that needs protecting are the module's internal, static global variables.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on a diff in pull request #5985: RISC-V: Combine 3 variables that depend on CPU amount into one
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on code in PR #5985:
URL: https://github.com/apache/incubator-nuttx/pull/5985#discussion_r847216610
##########
arch/risc-v/src/common/riscv_percpu.c:
##########
@@ -86,13 +123,27 @@ void riscv_percpu_init(void)
void riscv_percpu_add_hart(uintptr_t hartid)
{
- /* Hart IDs go from 0...4 */
+ riscv_percpu_t *percpu;
+ irqstate_t flags;
+
+ /* Make sure we are initialized */
+
+ riscv_percpu_init();
- DEBUGASSERT(hartid < HART_CNT);
+ /* Get free entry for this hart, this must not fail */
+
+ flags = spin_lock_irqsave(NULL);
Review Comment:
Do we need the global lock here (and in `riscv_percpu_init`)?
If these are called only during initialization then there won't be much to gain from a local lock, but I wanted to put this here in case you wanted to consider it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 merged pull request #5985: RISC-V: Combine 3 variables that depend on CPU amount into one
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 merged PR #5985:
URL: https://github.com/apache/incubator-nuttx/pull/5985
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org