-
Notifications
You must be signed in to change notification settings - Fork 49
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Migrate to ACLINT #66
Conversation
reg = <0x4300000 0x10000>; | ||
sswi0: sswi@4500000 {{ | ||
#interrupt-cells = <0>; | ||
interrupt-controller; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can add address-cells = <0>
to suppress "Missing #address-cells" warning provided by DTC v1.6.1
Reference: #37
|
||
mswi0: mswi@4400000 {{ | ||
#interrupt-cells = <0>; | ||
interrupt-controller; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto.
@@ -53,7 +53,7 @@ OBJS := \ | |||
plic.o \ | |||
uart.o \ | |||
main.o \ | |||
clint.o \ | |||
aclint.o \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm uncertain if the CLINT-related implementation will still exist once ACLINT is fully implemented. If it does, add an option here to use CLINT when ACLINT is unavailable.
In the latest commit, |
I don't think we should maintain both CLINT and ACLINT implementation. The RISC-V Advanced Core Local Interruptor (ACLINT) enhances the existing SiFive CLINT design through several key improvements:
The recent Linux kernel now provides support for ACLINT, and QEMU has also integrated this feature, allowing users to enable it through the |
The latest commit removed all implementations of CLINT, including the previously introduced |
Due to two minor issues in the first commit of this PR—specifically, the absence of To address this, the first commit message has been rewritten in accordance with the commit message rules, and the two minor issues have also been fixed in the process. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't use backticks for the sake of terminal compatibility. Use single quotation marks instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Improve git commit messages by:
- Clearly articulating the rationale behind transitioning from CLINT to ACLINT, highlighting the motivations and potential benefits of this migration.
- Detailing the methodological approach for verifying and validating the functionalities of the proposed ACLINT support, including specific testing strategies or validation criteria.
- Summarize the Linux kernel support on ACLINT along with git revisions.
- Outlining potential future development paths to enhance and complete the SMP support, identifying specific areas for improvement or expansion.
By the way, it is not necessary to mention "preliminary" if the proposed change covers the functionalities of CLINT. Instead, the major consideration is to migrate.
The latest commit includes the following updates:
|
Don't attempt to summarize your changes. Instead, we always care about the changesets. Let's consolidate each git commit without unnecessary report. |
device.h
Outdated
* | ||
* For more details, please refer to the register map at: | ||
* https://github.com/riscv/riscv-aclint/blob/main/riscv-aclint.adoc#21-register-map | ||
*/ | ||
uint64_t mtimecmp[4095]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The pre-allocated mtimecmp
causes unexpected memory waste since we don't really need all of its members. Consider allocating it on-demand.
device.h
Outdated
#ifndef NUM_HARTS | ||
#define NUM_HARTS 1 | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto. Remove the uncertain code.
device.h
Outdated
@@ -190,7 +196,7 @@ typedef struct { | |||
* For more details, please refer to the register map at: | |||
* https://github.com/riscv/riscv-aclint/blob/main/riscv-aclint.adoc#21-register-map | |||
*/ | |||
uint64_t mtimecmp[4095]; | |||
uint64_t mtimecmp[NUM_HARTS]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Allocate via runtime instead of build-time.
To implement timer interrupts and inter-processor interrupts (IPIs) on RISC-V, ACLINT and CLINT are the commonly used hardware components. The key difference between ACLINT and CLINT lies in ACLINT’s ability to support both Supervisor software interrupts (SSWI) and Machine software interrupts (MSWI). Additionally, ACLINT modularizes its hardware functionalities, such as timers and IPI controllers, making the design and implementation more flexible than CLINT. According to the Linux kernel documentation: https://www.kernel.org/doc/html/next/riscv/boot.html#kernel-entry, there are two methods for entering the Linux kernel on SMP systems: - RISCV_BOOT_SPINWAIT: Boots all harts simultaneously, mainly used for older firmwares without SBI HSM extension and M-mode RISC-V kernels. - Ordered booting: Utilizes the SBI HSM extension to boot only one hart during the initial boot phase. The Linux kernel introduced ordered booting (commit 'cfafe26') to simplify multi-stage SMP boot management. The commit explains that the previous method complicated the multi-stage boot process, requiring management of all harts at each stage. The SBI HSM extension simplifies this by booting only one hart initially, which can then bring up the remaining harts sequentially. To fully support the HSM extension, ACLINT is necessary. particularly for supervisor-level interrupt management. This commit transitions from CLINT to ACLINT, aligning with modern RISC-V specifications and providing support for 'mtimer', 'mswi', and 'sswi'. The existing CLINT implementation has been removed entirely as ACLINT covers its functionalities. Testing instructions: - Run the following command to test the implementation: 'make check SMP=n', where 'n' is the number of harts to simulate. - After booting the emulator: - Verify multi-core operation and HSM implementation with '/proc/cpuinfo'. - Check timer interrupts via '/proc/interrupts'. - Confirm ACLINT is correctly recognized using '/proc/device-tree'. Future work: Currently, due to the lack of implementation, the introduced ACLINT uses only supervisor-level IPI. Therefore, although the logic for mswi is implemented, it is not being used at the moment. Also, SMP support remains incomplete. For example, the current semu implementation sequentially simulates multi-core execution, causing a slowdown as the number of cores increases. This leads to a time desynchronization issue across cores. To achieve multi-threaded system emulation, RFENCE extension implementation is required. However, it is currently incomplete. After completing ACLINT, the next step is to implement the RFENCE extension to fully support multi-threaded system emulation.
Refine the wording in both comments and the previous git commit message. And use |
Thank @Mes0903 for contributing! |
In this commit, based on the implementation of CLINT, an preliminary ACLINT is implemented, including the basic logic for operating
mtimer
,mswi
, andsswi
. And this commit replaced CLINT with ACLINT, the old CLINT implementation was removed.Currently, due to the lack of implementation, the introduced ACLINT uses only supervisor-level IPI. Therefore, although the logic for mswi is implemented, it is not being used at the moment.
It can be tested by
make check SMP=n
, where n is the number of harts you want to simulate.