Conversation
ventZl
left a comment
There was a problem hiding this comment.
Thank you for your submission!
Except of the requirement to move RISC-V-related tests into riscv subtree, all remaining comments are just general non-binding stuff and can be resolved later.
|
|
||
| /* Syscall/trap entry is platform-specific. */ | ||
| #define __SYSCALL | ||
| #define __SVC(x) return 0; |
There was a problem hiding this comment.
It would be better to return E_NOTAVAIL here rather than 0. It will make whatever code that calls it fail early and serve as a reminder that there is something waiting to be implemented.
| uint32_t *new_saved_sp = NULL; | ||
| uint32_t *new_sp_after = NULL; | ||
|
|
||
| riscv_setup_switch(&old_sp, &new_saved_sp, &new_sp_after); |
There was a problem hiding this comment.
You can use CTEST_DATA / CTEST_SETUP / CTEST2 macros to automate this. Many kernel tests use this pattern.
CTEST_DATA serves purpose of defining per-group specific test data. In most cases this is not used, so you keep this empty.
CTEST_SETUP is executed automatically before every test starts, so you can reinitialize system into clean state there without having to call anything explicitly.
Use of CTEST2 macro rather than CTEST one is needed to trigger this semantics.
CTEST_DATA/CTEST_SETUP are grouped per test group (1st argument). You can have as many you need.
There was a problem hiding this comment.
Changed to use CTEST_DATA/CTEST_SETUP, not sure it was done right. Please recheck.
| * - mepc points to ecall instruction, must be incremented by 4 to continue | ||
| */ | ||
|
|
||
| #define FRAME_OFFSET_A0 16 |
There was a problem hiding this comment.
Couldn't this be replaced by something like type ExceptionFrame in arch/arm/cmsis/arch/cortex.h ?
There was a problem hiding this comment.
I created an exception frame adapter for rp2350 for now. SHould I try to make it generic now or we leave it for another PR?
There was a problem hiding this comment.
Don't bother with extensive cleanup while the port is not done. It may easily be found out things will have to be reworked while implementing later stages of support. Thus effort spent by making things generic may easily be thrown out.
| @@ -0,0 +1,34 @@ | |||
| # CMRX quirk for Pico SDK RISC-V (RP2350) | |||
There was a problem hiding this comment.
The rpi-sdk code was sent to exile here because it was modifying some of pico-sdk targets but I don't see anything similar here. All changes are to the CMRX's own os target.
Thus the code here seems to be pretty generic and not specific to RPI SDK. Of course, various SDKs can call these handlers various names which will cause us to fail to bind to the vectors, but this can be addressed later I guess.
Which is a good news because bunch of the code can be moved into arch/riscv rather than live here.
I hope that in long run, the we could get rid of the whole quirks subtree as portability will be recognized as an advantage in the world of embedded SW development.
There was a problem hiding this comment.
Sorry I didn't make the changes geenric. Do you want me to leave it for this first PR or should I try to move the generic bits to the os subdirectory still in this PR?
There was a problem hiding this comment.
Lets keep it as is for now, when some other RISC-V microcontroller is available, this can get revisited and we can find some way to make it generic.
09da34c to
6be04d0
Compare
| add_library(${NAME} STATIC EXCLUDE_FROM_ALL ${ARGN}) | ||
| set_property(TARGET ${NAME} PROPERTY CMRX_IS_APPLICATION 1) | ||
| target_compile_definitions(${NAME} PRIVATE -D APPLICATION_NAME=${NAME}) | ||
| target_include_directories(${NAME} PRIVATE |
There was a problem hiding this comment.
I am not sure this is actually needed.
There was a problem hiding this comment.
${CMRX_ROOT_DIR}/include should be injected by stdlib, latter shouldn't be needed at all I guess.
There was a problem hiding this comment.
What is functional state of this code?
I think that for initial stage of porting effort it is OK as long as it builds. Based on the code I see I'd expect it is capable of switching threads and maybe calling system calls?
If clang-tidy won't complain, it is probably good to go.
RISC-V specific unit tests opened question of organization of unit tests, maybe these need to be shuffled around. Testing is already messy anyway with unit-tests, testing platform, hil tests and clang-tidy builds all living in different parts of tree.
|
|
||
| int __futex_fast_lock(futex_t *futex, uint8_t thread_id, unsigned max_depth) | ||
| { | ||
| uint32_t saved = irq_save(); |
There was a problem hiding this comment.
Interesting. Can you disable interrupts from userspace on RISC-V? I'd expect you can't. This code runs in userspace (note the src/lib path). That's why CMRX is using load/store conditional to implement them.
Disabling interrupts from userspace is a big fat no.
There was a problem hiding this comment.
I don't remember why I let AI write this. So should I just remove this file ?
There was a problem hiding this comment.
REmoved interrupt handling, and implemented lock and unlock using riscv Atomic instructions (load reserved and store conditional)
| add_library(${NAME} STATIC EXCLUDE_FROM_ALL ${ARGN}) | ||
| set_property(TARGET ${NAME} PROPERTY CMRX_IS_APPLICATION 1) | ||
| target_compile_definitions(${NAME} PRIVATE -D APPLICATION_NAME=${NAME}) | ||
| target_include_directories(${NAME} PRIVATE |
There was a problem hiding this comment.
${CMRX_ROOT_DIR}/include should be injected by stdlib, latter shouldn't be needed at all I guess.
| @@ -0,0 +1,34 @@ | |||
| # CMRX quirk for Pico SDK RISC-V (RP2350) | |||
There was a problem hiding this comment.
Lets keep it as is for now, when some other RISC-V microcontroller is available, this can get revisited and we can find some way to make it generic.
| ) | ||
| target_sources(os PRIVATE ${cmrx_testing_SRCS}) | ||
| target_link_libraries(os PRIVATE ctest) | ||
|
|
There was a problem hiding this comment.
I would probably included this directory conditionally from riscv/hal/CMakeLists.txt.
Neither is totally clean but adding add_subdirectory() to direct parent directory at least makes things a bit more obvious.
edit: OK, now I understand. If unit testing is activated, arch directory won't be included at all.
When ARM platform support was developed there was no idea on writing tests for it so this case wasn't considered. Lets keep these tests here for now and we'll see what to do with it later.
Currently I have a small validation app that has 2 threads, one that turn off the led and another that turns on. Both threads print some variable count to uart. It looks like this the main c file : #define LED_PIN 25 static volatile uint32_t thread_a_count = 0; /*
/*
/*
/* Declare the application */ /* Auto-start two threads with priority 32 */ I also have a main source file that initializes the os like demonstrated in other cmrx examples : #include <cmrx/cmrx.h> #include "pico/stdlib.h" long timing_get_current_cpu_freq(void) int main(void) } With the latest code changes, it builds and when I try to debug the target I see that unfortunately it is not wiring my pico-sdk modifications in quirks/, but the pico-sdk default ones, so it is hanging : Thread 1 "rp2350.rv0" hit Temporary breakpoint 1, main () Thread 1 "rp2350.rv0" received signal SIGTRAP, Trace/breakpoint trap. do you think it is something wrong with my app or there might be something in cmrx code that I am missing? |
|
|
||
| /* No special handling yet; platform integration will define this as needed. */ | ||
| #define os_thread_initialize_arch(x) | ||
| #define os_thread_initialize_arch(...) |
There was a problem hiding this comment.
I would say this function here is wrong... ? I should have put it in riscv subdirecty under the sched.c with the actual implementation that calls os_thread_populate_stack that configures the thread state to be executed after the os boots .... ?
There was a problem hiding this comment.
Sorry for late reply, I somehow missed e-mail notification about reply.
Yes, I see you did it already. And your assumption is right. The purpose of this function is to make stack suitable for "returning" from interrupt handler into thread context. That's how threads are both launched and resumed. Upon resume, stack is in such state automatically due to work done on its suspension. Before first launch, this state has to be forged.
6be04d0 to
3ce1f74
Compare
Major problem CMRX has is that it is compiled as a static library. SDKs usually provide fallback weak no-op symbols for stuff like ISR handlers (even in case of ARM and other platforms) so that stuff will compile even if integrator doesn't provide / need their own version. This way you don't have to provide dozens of ISR handlers for peripherals you are not using. These symbols are weak so if you provide strong version of the symbol in any other object file entering linking, it will override the weak symbol. If you don't provide strong symbol, weak symbol will be used as fallback. The problem is that this changes in numerous ways, if your code isn't coming directly from object file, rather from static library. Static library is "just a bunch of object files" but linker treats it differently. Linker tries to use objects from libraries as least as possible. One of linker behavior changes is that weak symbols in main object are not overridable by strong symbols from library object. That's why pico-SDK quirk hacked pico-sdk to provide crt0.s which was missing few symbols which were then provided by quirk addition to OS library. This way this weak symbol in main object wasn't a problem anymore. PicoSDK "fixed" the issue by providing a small static library which hosts these files so this quirk is actually not needed anymore for RP2040 (and probably also RP2350 in ARM mode). |
639adea to
1b758f5
Compare
| * The context switch safe-point is handled by the exception dispatcher, | ||
| * not here. | ||
| * | ||
| * SPDX-License-Identifier: BSD-3-Clause |
There was a problem hiding this comment.
We're using MIT here, at least for now.
1b758f5 to
58d88d7
Compare
|
|
Close PR unintentionally, sorry about that |
ventZl
left a comment
There was a problem hiding this comment.
Looks good. There are several things that may turn out to be loose ends but dealing with them is not critical for now.
| * | ||
| * This file is derived from pico-sdk exception_table_riscv.S. | ||
| * Copyright (c) 2020 Raspberry Pi (Trading) Ltd. | ||
| * SPDX-License-Identifier: BSD-3-Clause |
There was a problem hiding this comment.
I see that in some files license identifier is already MIT, here BSD leaked
There was a problem hiding this comment.
I needed to add bsd license there because it was a derived file from the pico-sdk repo. Not sure I could simply put MIT.
There was a problem hiding this comment.
OK, it seems that there is no way around this. It would probably be best to make the whole pico-sdk-riscv component BSD-licensed, not just this file.
Then it should be compliant and clear as BSD-licensed component will link against MIT-licensed kernel with no dependencies from kernel back to the quirk component.
Sorry for inconvenience caused by shifting licenses back and forth. To comply with BSD license its copy probably has to be put into this folder into LICENSE.txt. This will also aid license inferring tools to figure out that the content of this directory is licensed under BSD while remainder is licensed as MIT.
For me, there will be a homework to add license identifiers into all files as this will become a multi-license project once this gets merged into master.
|
|
||
| #else | ||
|
|
||
| #error "RISC-V A extension (atomics) is required for the LR.W/SC.W-based mutex. \ |
There was a problem hiding this comment.
Good solution for now. In the future, maybe mutex implementation provided for Cortex-M0+ (which also doesn't support atomics) may be used, if sufficient large pool of MCUs with MPU but without atomics appears.
No action needed.
Add RISC-V architecture support for running on RP2350 with Pico SDK: - quirks/pico-sdk-riscv: Pico SDK RISC-V integration specifics - Custom CRT0 wrapper for IRQ exit context switch hook - ecall exception handler for CMRX syscalls - CMake integration for firmware builds - src/os/arch/riscv: Architecture-specific kernel components - sched.c: Thread stack setup, boot, and sleep primitives - mpu.c: Memory protection stubs (PMP not implemented) - static.c: Static thread/application table accessors - corelocal.h: Single-core lock/unlock via IRQ disable - sysenter.h: ecall-based syscall implementation - cmake/arch/riscv/hal: Build system support - CMRX.cmake: add_firmware() function - cmrx_sections.ld: Linker symbols for static init tables - src/lib/arch/riscv: Library support - mutex.c: Mutex implementation using HAL primitives
Add arch-level ExceptionFrame (32 words / 128 bytes) covering all GP registers, mepc, and mstatus. Simplify context switch to a pure SP swap; full register save/restore is now the trap handler's responsibility. Update os_thread_populate_stack to build an ExceptionFrame, os_boot_thread to use mret for consistent trap-return entry, and os_set_syscall_return_value to write a0 via the unified frame. Adjust unit tests accordingly.
Add SAVE_CONTEXT() and LOAD_CONTEXT() inline asm macros to exception_frame.h, mirroring ARM's cortex.h pattern. Rewrite the Pico SDK quirk handlers to use these macros, reducing each to a thin naked wrapper with only SDK-specific wiring (symbol override, mscratch protocol, exception table dispatch). Simplify the ecall handler to plain C receiving ExceptionFrame*. Delete the RP2350-specific exception frame type, superseded by the arch-level ExceptionFrame.
58d88d7 to
9e88ab2
Compare
|
Thanks for your contribution! |
Summary
This PR is my first attempt at scaffolding RISC-V support for CMRX thread switching, targeting to use RP2350 (Pico 2) with Pico SDK as the validation platform. The goal is to get a minimal context switcher + syscall/trap path wired enough to start doing on-target debugging and validation.
What’s included
ecall) and handler wiring so kernel syscalls can be dispatched.Current status
I’d appreciate your review/feedback