arch/riscv/qemu-rv: Add S-mode support #12178

inochisa · 2024-04-19T02:43:27Z

This patch series introduce some optimization for S-mode only nuttx:

abstract IPI send/clear function
add support for allocate stack dynamic
add SSTC support.

At last, it introduces S-mode only on qemu-rv platform, which can boot
with SBI provided by QEMU.

Logfiles:
nuttx-qemu32.log
nuttx-qemu64.log
nuttx-nsh32.log
nuttx-nsh64.log

lupyuen · 2024-04-20T16:08:03Z

Thanks @inochisa, I have some questions:

Have we tested on QEMU 32-bit and 64-bit? (Normally I attach NuttX Test Logs to my PRs)
There's a discussion here on IPI: Question about RISCV cpu_start #11930. Wonder if the PR will fix this issue?
Suggest we squash the commits into 1 commit, so it's easier for other devs to track the history. (Actually I wonder if we should have submitted as multiple smaller PRs)

Please gimme some time to read and understand the code, also to test on Ox64. Thanks!

inochisa · 2024-04-21T01:03:24Z

1. Have we tested on QEMU 32-bit and 64-bit? (Normally I attach [NuttX Test Logs](https://lupyuen.github.io/articles/pr#testing) to my PRs)

Thanks for reminding, I have attached test log file.

2. There's a discussion here on IPI: [Question about RISCV cpu_start #11930](https://github.com/apache/nuttx/issues/11930). Wonder if the PR will fix this issue?

No luck, this is another problem. The change on IPI is mainly for using different IPI code in different mode.

3. Suggest we [squash the commits](https://lupyuen.github.io/articles/pr#squash-the-commits) into 1 commit, so it's easier for other devs to track the history. (Actually I wonder if we should have submitted as multiple smaller PRs)

I struggled whether to break this PR into several ones, or just squash them as a whole. But finally, I decide to submmit this PR as is. IPI change allows to using another change for S mode. dynamic stack allocation make a progress on booting with non-zero hart (which is kind of necessary for booting with SBI). and SSTC give better performance in S-mode. This three are important for qemu-rv S-mode. For the last commit, it is a squash commit. Because I have no idea to separate them.

lupyuen · 2024-04-21T10:05:06Z

Thanks @inochisa. I'll write some Review Notes to explain the SSTC Extension, this is new to me and many folks too :-)

Hi @yf13 please let us know if you have comments on the K230 changes thanks!

lupyuen · 2024-04-21T11:34:17Z

Tested OK on Ox64 BL808 SBC, Kernel Mode. (See the Test Log)

@inochisa Thanks for the Test Logs for QEMU Kernel Mode: rv-virt:knsh32 and rv-virt:knsh64. As a Regression Test, could you also run QEMU Flat Mode: rv-virt:nsh and rv-virt:nsh64?

Thanks for the excellent work in refactoring QEMU Startup for Kernel Mode, I'm just making sure that QEMU Flat Mode is still OK after refactoring. Thanks!

inochisa · 2024-04-21T12:03:00Z

@lupyuen I have added new log files.

Thanks @inochisa. I'll write some Review Notes to explain the SSTC Extension, this is new to me and many folks too :-)

Thanks for this improvement.

masayuki2009 · 2024-04-22T01:45:10Z

@inochisa
I tried this PR with qemu-rv and k210 (maix-bit board).
It seems that qemu-rv works but maix-bit:nsh does not show any messages on the serial console.

inochisa · 2024-04-22T02:33:02Z

@masayuki2009 Thanks, it seems that I removed a branch jump by mistake. Could you test the new PR on the k210?

masayuki2009 · 2024-04-23T03:31:22Z

@masayuki2009 Thanks, it seems that I removed a branch jump by mistake. Could you test the new PR on the k210?

@inochisa
Thanks, it's working now.

lupyuen · 2024-04-23T07:19:50Z

@inochisa Could you change to the text below, so I can approve the PR? Thanks!

Documentation/platforms/risc-v/qemu-rv/boards/rv-virt/index.rst

If testing with kernel build, remove the ``-bios none`` option. Kernel build requires
SBI to function properly.

inochisa · 2024-04-23T07:23:57Z

@inochisa Could you change to the text below, so I can approve the PR? Thanks!

Documentation/platforms/risc-v/qemu-rv/boards/rv-virt/index.rst
If testing with kernel build, remove the `-bios none` option. Kernel build requires
SBI to function properly.

@lupyuen Do you have any suggestions? In fact, I am not sure how to explain this change. Maybe just add new command line for kernel mode will be better?

lupyuen · 2024-04-23T07:28:56Z

@inochisa Sorry I didn't understand your question? You inserted this line at Documentation/platforms/risc-v/qemu-rv/boards/rv-virt/index.rst:

If test with kernel build, remove `-bios none` options. Kernel build require
SBI to function properly.

The text has typos. Could you change the text so that it reads:

If testing with kernel build, remove the ``-bios none`` option. Kernel build requires
SBI to function properly.

There's no need to explain the change or add a command line. Thanks!

inochisa · 2024-04-23T07:30:19Z

@inochisa Sorry I didn't understand your question? You inserted this line at Documentation/platforms/risc-v/qemu-rv/boards/rv-virt/index.rst:
If test with kernel build, remove `-bios none` options. Kernel build require
SBI to function properly.
The text has typos. Could you change the text so that it reads:
If testing with kernel build, remove the ``-bios none`` option. Kernel build requires
SBI to function properly.
There's no need to explain the change or add a command line. Thanks!

Thanks, I will fix it.

lupyuen

LGTM Thank you so much :-)

arch/risc-v/src/qemu-rv/qemu_rv_start.c

arch/risc-v/Kconfig

arch/risc-v/src/common/riscv_macros.S

arch/risc-v/src/common/riscv_mtimer.c

arch/risc-v/src/qemu-rv/qemu_rv_exception_m.S

Documentation/platforms/risc-v/qemu-rv/boards/rv-virt/index.rst

acassis · 2024-04-25T12:50:29Z

@inochisa please rebase and fix the conflicts

masayuki2009 · 2024-04-26T03:45:37Z

@inochisa
Please rebase to fix the conflicts again.

arch/risc-v/src/common/riscv_internal.h

arch/risc-v/src/common/riscv_macros.S

Add ipi process abstract function support. Signed-off-by: Inochi Amaoto <[email protected]>

It is misleading to allocate stack from static array and heap, make all stack allocated from heap area. Signed-off-by: Inochi Amaoto <[email protected]>

As `up_get_intstackbase` supports per cpu stack base, fix the report value with the cpu specific one. Signed-off-by: Inochi Amaoto <[email protected]>

SSTC extension allows nuttx to implement S-mode timer directly, which is useful for starting at S-mode. Signed-off-by: Inochi Amaoto <[email protected]>

The qemu-rv use a small init code for M mode in kernel build. It is hard-coding and is difficult to change. Due to the fact, introduce a already mature SBI implement (e.g OpenSBI) to replace existing code is a better choice. This patch introduce some change for qemu-rv: 1. use SSTC to provide time interrupt in kernel build 2. remove uncessary M mode trap. For simplicity, this patch does not add support for booting nuttx for any core, but force boot core to start core 0 and let core 0 do the initialization. Signed-off-by: Inochi Amaoto <[email protected]>

xiaoxiang781216 · 2024-04-28T01:02:20Z

arch/risc-v/src/common/riscv_ipi.h

+
+static inline void riscv_ipi_send(int cpu)
+{
+#if defined(RISCV_IPI)


remove, it's better to generate the.compiler error instead runtime panic.

Right, I think it is better to add this when adding sbi_ipi_send function.

xiaoxiang781216 · 2024-04-28T01:03:33Z

arch/risc-v/src/common/riscv_ipi.h

+
+static inline void riscv_ipi_clear(int cpu)
+{
+#if defined(RISCV_IPI)


xiaoxiang781216 · 2024-04-28T01:19:16Z

arch/risc-v/src/common/riscv_internal.h

+
+ for (i = 0; i < CONFIG_SMP_NCPUS; i++)
+ {
+ g_cpux_idlestack[i] = (const uint8_t *)(base + size * i);


why not initialize the array at the definition place?

Initializing this array at the definition sounds like good, but it needs lots of ugly macro test like CONFIG_SMP_NCPUS < x. I don't think it is good.

but it still better to runtime initialize it in all chip specific file.

I agree, initializing this array statically does have benefits. The only problem is how to achieve it.The only way I find at now is just generating g_cpux_idlestack based on the CONFIG_SMP_NCPUS . Could you have some better idea?

I don't find a better method than this too.:(

xiaoxiang781216 · 2024-04-28T01:20:14Z

arch/risc-v/src/common/riscv_macros.S

+ * Parameter:
+ * base - Pointer to where the stack is allocated (e.g. _ebss)
+ * size - Stack size for pre cpu to allocate
+ * size - Hart id register of this hart (Usually a0)


size to hartid

xiaoxiang781216 · 2024-04-28T01:24:35Z

arch/risc-v/src/common/riscv_macros.S

+
+ /* ensure the last XCPTCONTEXT_SIZE is reserved for non boot CPU */
+
+ bnez \hartid, 998f


why not remove the reserve from up_initial_state to make the code mofe consistent? or skip calling riscv_set_inital_sp for boot cpu.

I have seen up_initial_state reserve something, It just return after setting the idle stack for the idle process 0. Do I miss somthing?
And skip calling riscv_set_inital_sp is not a good idea, this means chip always need to set it manually, which is weird.

xiaoxiang781216 · 2024-04-28T01:36:12Z

just send out before @acassis merge the patch, @inochisa please check my comment, thanks.

inochisa · 2024-04-28T01:41:54Z

just send out before @acassis merge the patch, @inochisa please check my comment, thanks.

@xiaoxiang781216 I see @acassis already merged this before your comment. Is it better to open a new PR to fix this?

xiaoxiang781216 · 2024-04-28T02:15:01Z

just send out before @acassis merge the patch, @inochisa please check my comment, thanks.

@xiaoxiang781216 I see @acassis already merged this before your comment. Is it better to open a new PR to fix this?

Yes.

acassis · 2024-04-28T11:02:51Z

Sorry @inochisa @xiaoxiang781216 I missed these comments, I just saw the other approvals and merged it

xiaoxiang781216 · 2024-04-28T14:42:24Z

Not your problem, the patch was merged before I submit the comment.

lupyuen · 2024-05-22T11:17:15Z

@inochisa There might be a problem with QEMU SMP and OpenSBI / SSTC:

When I tested with QEMU -smp 8: Out of 16 tries, 3 will boot OK, the rest will hang during NuttX Startup. It works fine without -smp.

-smp 8 comes from the NuttX Doc: https://nuttx.apache.org/docs/latest/platforms/risc-v/qemu-rv/boards/rv-virt/index.html#configurations

$ qemu-system-riscv64 -semihosting -M virt,aclint=on -cpu rv64 -smp 8 -bios none -kernel nuttx -nographic

So we might have to remove -smp 8 from the NuttX Doc. Or fix this issue in the NuttX Startup Code? (Due to Race Condition?)

Here's how I reproduced the problem. Thanks! https://gist.github.com/lupyuen/df2c9a552b1679257c9ff5d871ce0a4f

$ git clone https://github.com/inochisa/nuttx --branch qemu-s-mode
$ git clone https://github.com/XuNeo/incubator-nuttx-apps apps --branch fix-lvgl-distclean-warning

$ cat nuttx.hash
NuttX Source (26 Apr 2024): https://github.com/apache/nuttx/tree/02c1a95c96069ea59401255169961c082471f826
NuttX Apps (30 Apr 2024): https://github.com/apache/nuttx-apps/tree/0f0caee0d0367c4664c05c5c14807f5d70f2e1de

$ qemu-system-riscv64 --version
QEMU emulator version 8.2.2 (Debian 1:8.2.2+ds-0ubuntu1)
## OpenSBI v1.3
## Runtime SBI Version       : 1.0

$ qemu-system-riscv64 \
    -semihosting \
    -M virt,aclint=on \
    -cpu rv64 \
    -smp 8 \
    -kernel nuttx \
    -nographic
## Out of 16 tries, 3 will boot OK 

$ neofetch
            .-/+oossssoo+/-.               luppy@luppy-macbook-ubuntu 
        `:+ssssssssssssssssss+:`           -------------------------- 
      -+ssssssssssssssssssyyssss+-         OS: Ubuntu 24.04 LTS x86_64 
    .ossssssssssssssssssdMMMNysssso.       Host: MacBookPro10,1 1.0 
   /ssssssssssshdmmNNmmyNMMMMhssssss/      Kernel: 6.8.0-31-generic 
  +ssssssssshmydMMMMMMMNddddyssssssss+     Uptime: 1 hour, 29 mins 
 /sssssssshNMMMyhhyyyyhmNMMMNhssssssss/    Packages: 1911 (dpkg), 16 (snap 
.ssssssssdMMMNhsssssssssshNMMMdssssssss.   Shell: bash 5.2.21 
+sssshhhyNMMNyssssssssssssyNMMMysssssss+   Resolution: 1920x1080 
ossyNMMMNyMMhsssssssssssssshmmmhssssssso   DE: Unity 
ossyNMMMNyMMhsssssssssssssshmmmhssssssso   WM: Mutter 
+sssshhhyNMMNyssssssssssssyNMMMysssssss+   WM Theme: Adwaita 
.ssssssssdMMMNhsssssssssshNMMMdssssssss.   Theme: Yaru-dark [GTK2/3] 
 /sssssssshNMMMyhhyyyyhdNMMMNhssssssss/    Icons: Yaru [GTK2/3] 
  +sssssssssdmydMMMMMMMMddddyssssssss+     Terminal: vscode 
   /ssssssssssshdmNNNNmyNMMMMhssssss/      CPU: Intel i7-3820QM (8) @ 3.70 
    .ossssssssssssssssssdMMMNysssso.       GPU: NVIDIA GeForce GT 650M Mac 
      -+sssssssssssssssssyyyssss+-         GPU: Intel 3rd Gen Core process 
        `:+ssssssssssssssssss+:`           Memory: 3667MiB / 15898MiB 
            .-/+oossssoo+/-.

inochisa · 2024-05-22T11:35:40Z

@inochisa There might be a problem with QEMU SMP and OpenSBI / SSTC:

When I tested with QEMU -smp 8: Out of 16 tries, 3 will boot OK, the rest will hang during NuttX Startup. It works fine without -smp.

I think this is simple because SBI does not guarantee the boot hart is 0, which is required for nuttx (Although the firmware used by qemu support setting boot hart, qemu itself does not provide this interface IIRC). If you set the correct hart number for the build, everything works well.

-smp 8 comes from the NuttX Doc: https://nuttx.apache.org/docs/latest/platforms/risc-v/qemu-rv/boards/rv-virt/index.html#configurations
$ qemu-system-riscv64 -semihosting -M virt,aclint=on -cpu rv64 -smp 8 -bios none -kernel nuttx -nographic
So we might have to remove -smp 8 from the NuttX Doc. Or fix this issue in the NuttX Startup Code? (Due to Race Condition?)

At least for now, it seems we need to update the doc. I think setting the right number of CPU is always required.
Fix this issue needs the capability of CPU discovery, but I have no idea for implementing it. (risc-v does not guarantee the order of hartid).

lupyuen · 2024-05-22T11:57:09Z

@inochisa Thanks, does it mean that ksmp64 and knetnsh64_smp won't work with OpenSBI / SSTC?

https://nuttx.apache.org/docs/latest/platforms/risc-v/qemu-rv/boards/rv-virt/index.html#ksmp64

https://nuttx.apache.org/docs/latest/platforms/risc-v/qemu-rv/boards/rv-virt/index.html#knetnsh64-smp

inochisa · 2024-05-22T12:00:17Z

@inochisa Thanks, does it mean that ksmp64 and knetnsh64_smp won't work with OpenSBI / SSTC?

https://nuttx.apache.org/docs/latest/platforms/risc-v/qemu-rv/boards/rv-virt/index.html#ksmp64

https://nuttx.apache.org/docs/latest/platforms/risc-v/qemu-rv/boards/rv-virt/index.html#knetnsh64-smp

It works only when the cpu number will set to 4 (the CPU number in config).

I think the boot command for kernel mode may need to change as the following.

$ qemu-system-riscv64 -semihosting -M virt,aclint=on -cpu rv64 -smp <cpu number> -kernel nuttx -nographic

inochisa force-pushed the qemu-s-mode branch 3 times, most recently from b595e1b to 5e6e679 Compare April 19, 2024 10:02

acassis requested review from lupyuen and masayuki2009 April 20, 2024 14:59

inochisa force-pushed the qemu-s-mode branch from 5e6e679 to 60c0af4 Compare April 22, 2024 02:31

inochisa force-pushed the qemu-s-mode branch from 60c0af4 to 337e64d Compare April 23, 2024 07:34

lupyuen approved these changes Apr 23, 2024

View reviewed changes

inochisa force-pushed the qemu-s-mode branch from 337e64d to dacdf5c Compare April 26, 2024 02:34

inochisa force-pushed the qemu-s-mode branch 2 times, most recently from 67f1ff2 to 0831f43 Compare April 26, 2024 05:21

anchao reviewed Apr 26, 2024

View reviewed changes

arch/risc-v/src/common/riscv_internal.h Outdated Show resolved Hide resolved

arch/risc-v/src/common/riscv_macros.S Show resolved Hide resolved

inochisa force-pushed the qemu-s-mode branch from 0831f43 to 02c1a95 Compare April 26, 2024 09:43

inochisa added 3 commits April 26, 2024 17:44

arch/risc-v: unfiy IPI access

7c22573

Add ipi process abstract function support. Signed-off-by: Inochi Amaoto <[email protected]>

arch/risc-v: introduce dynamic stack allocation.

fec0879

It is misleading to allocate stack from static array and heap, make all stack allocated from heap area. Signed-off-by: Inochi Amaoto <[email protected]>

arch/risc-v: report correct interrupt stack base

e82327b

As `up_get_intstackbase` supports per cpu stack base, fix the report value with the cpu specific one. Signed-off-by: Inochi Amaoto <[email protected]>

inochisa added 2 commits April 26, 2024 17:44

arch/risc-v: add risc-v SSTC extension support

80f3b30

SSTC extension allows nuttx to implement S-mode timer directly, which is useful for starting at S-mode. Signed-off-by: Inochi Amaoto <[email protected]>

anchao approved these changes Apr 26, 2024

View reviewed changes

acassis approved these changes Apr 28, 2024

View reviewed changes

acassis merged commit 49b3f52 into apache:master Apr 28, 2024
26 checks passed

xiaoxiang781216 reviewed Apr 28, 2024

View reviewed changes

This was referenced Apr 28, 2024

arch/risc-v: remove macro test for ipi function #12242

Closed

arch/risc-v: Fix typo in riscv_set_inital_sp #12243

Merged

inochisa mentioned this pull request Apr 29, 2024

arch/risc-v: implement SBI ipi extension #12250

Merged

yf13 mentioned this pull request May 3, 2024

regression of rv-virt kernel builds #12275

Closed


		/* ensure the last XCPTCONTEXT_SIZE is reserved for non boot CPU */

		bnez \hartid, 998f

arch/riscv/qemu-rv: Add S-mode support #12178

arch/riscv/qemu-rv: Add S-mode support #12178

Conversation

inochisa commented Apr 19, 2024 • edited Loading

lupyuen commented Apr 20, 2024 • edited Loading

inochisa commented Apr 21, 2024

lupyuen commented Apr 21, 2024

lupyuen commented Apr 21, 2024

inochisa commented Apr 21, 2024

masayuki2009 commented Apr 22, 2024

inochisa commented Apr 22, 2024

masayuki2009 commented Apr 23, 2024

lupyuen commented Apr 23, 2024 • edited Loading

inochisa commented Apr 23, 2024

lupyuen commented Apr 23, 2024

inochisa commented Apr 23, 2024

lupyuen left a comment

Choose a reason for hiding this comment

acassis commented Apr 25, 2024

masayuki2009 commented Apr 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

inochisa Apr 28, 2024 • edited Loading

Choose a reason for hiding this comment

xiaoxiang781216 commented Apr 28, 2024

inochisa commented Apr 28, 2024

xiaoxiang781216 commented Apr 28, 2024

acassis commented Apr 28, 2024

xiaoxiang781216 commented Apr 28, 2024

lupyuen commented May 22, 2024

inochisa commented May 22, 2024 • edited Loading

lupyuen commented May 22, 2024

inochisa commented May 22, 2024 • edited Loading

inochisa commented Apr 19, 2024 •

edited

Loading

lupyuen commented Apr 20, 2024 •

edited

Loading

lupyuen commented Apr 23, 2024 •

edited

Loading

masayuki2009 commented Apr 26, 2024 •

edited

Loading

inochisa Apr 28, 2024 •

edited

Loading

inochisa commented May 22, 2024 •

edited

Loading

inochisa commented May 22, 2024 •

edited

Loading