-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
[RFC] riscv: drivers: Implement APLIC and IMSIC drivers #7236
Conversation
By the way, I used some of the code related to APLIC and IMSIC from Linux and OpenSBI. Should I inform the original authors or take any specific actions? |
If you copy code, you must do so under the license provided by the copyright holder and preserve the copyright. However, we don't accept GPL licenses (copyleft) in OP-TEE. The copyright holder may agree to change the license, but it can get tricky if there are many copyright holders. |
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.
Comments to core/drivers/aplic_direct.c may apply to other source files.
Please fix all indention issue. Can use scripts/checkpatch.sh
to fund them.
Thank you for your insights. I need to consider removing the part of code copied from Linux and rewrite the relevant parts. Additionally, since OpenSBI https://github.com/riscv-software-src/opensbi under the BSD-2-Clause license, I can use their code in my PR by preserve the copyright, right? |
Correct. |
dabed47
to
d0e5ed0
Compare
@maroueneboubakri Do you have interest to review this PR? |
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.
Some coding style and other minor comments.
Hi @HuangBorong , An year ago I implemented a simple UART RX interrupt handler to test secure interrupt. static void read_console(void)
{
struct serial_chip *cons = &console_data.chip;
if (!cons->ops->getchar || !cons->ops->have_rx_data)
return;
while (cons->ops->have_rx_data(cons)) {
int ch __maybe_unused = cons->ops->getchar(cons);
DMSG("got 0x%x", ch);
}
}
static enum itr_return console_itr_cb(struct itr_handler *h __maybe_unused)
{
read_console();
return ITRR_HANDLED;
}
static struct itr_handler console_itr = {
.it = UART1_IRQ,
.flags = ITRF_TRIGGER_LEVEL,
.handler = console_itr_cb,
};
DECLARE_KEEP_PAGER(console_itr);
static TEE_Result init_console_itr(void)
{
TEE_Result res = TEE_ERROR_GENERIC;
console_itr.chip = interrupt_get_main_chip();
res = interrupt_add_configure_handler(&console_itr, IRQ_TYPE_LEVEL_HIGH,
1);
if (res)
return res;
interrupt_enable(console_itr.chip, console_itr.it);
return TEE_SUCCESS;
}
driver_init(init_console_itr); |
@HuangBorong , About the Supervisor top interrupt CSR (stopi), yes we definitely need it. #if defined(CFG_RISCV_APLIC_MSI) || defined(CFG_RISCV_IMSIC)
void thread_native_interrupt_handler(struct thread_ctx_regs *regs,
unsigned long cause)
{
unsigned long topi;
while ((topi = csr_read(CSR_XTOPI))) {
topi = topi >> TOPI_IID_SHIFT;
switch (topi) {
case IRQ_XTIMER:
clear_csr(CSR_XIE, CSR_XIE_TIE);
break;
case IRQ_XSOFT:
thread_unhandled_trap(regs, cause);
break;
case IRQ_XEXT:
thread_irq_handler();
break;
default:
thread_unhandled_trap(regs, cause);
}
}
}
#endif |
@gagachang Thank you for your review! I have fixed the issues you mentioned above in commit 5d901b7. |
You're welcome, did you test secure interrupt ? |
Hi @gagachang, So far, I’ve tested secure interrupts in APLIC direct mode. You can find my code in my repository https://github.com/HuangBorong/riscv-optee-dev-env/tree/test-aplic-direct under the branch To test the secure interrupt, I’ve done the following:
However, I’ve encountered an issue: While OP-TEE successfully receives interrupts from the timer, it fails to receive interrupts from UART RX. However, the console is able to capture the characters I enter. I’m unsure whether this is due to a misconfiguration or related to other underlying problems. |
Sorry I forgot to give you some configurations. You need to enable |
Thanks! I can receive UART interrupt now. |
Great! It seems the APLIC direct mode works well. |
@HuangBorong You can hide the resolved reviews so that the reviewers can know which reviews were resolved. |
a3b6f79
to
ae8b182
Compare
Hi @gagachang, I have tested the secure interrupt in APLIC MSI mode. You can find my code in my repository https://github.com/HuangBorong/riscv-optee-dev-env/tree/test-aplic-msi under I refactored the IMSIC interrupt handler in commit 99e2277. Here are the changes I made:
|
Amazing work! Thanks. Besides, I think the relationship between APLIC and IMSIC in your code should be reversed. IMO, the |
I have considered this scenario before. However, the QEMU virt only supports two AIA types: APLIC only or APLIC + IMSIC. Currently, if a platform only has the IMSIC, we can not emulate it on QEMU virt. Furthermore, the IMSIC only receives MSI writes as interrupt signals. If the platform can adopt IMSIC independently, all devices on this platform should be PCI devices or support MSI writes. However, some simple devices, like UART, only support wired interrupts (using a wire to connect to the interrupt controller and possibly utilizing a positive edge as the interrupt signal). |
Hi @gagachang
I believe you are correct. The IMSIC should be the parent of the APLIC.
I believe the
I believe the current |
That's correct.
Can you provide use case to add multiple APLIC interrupt domains into OP-TEE ?
It's OK to me. At least please add some sanity checks: - interrupt_configure(&imsic->aplic->chip, it, type, prio);
+ if (imsic->aplic)
+ interrupt_configure(&imsic->aplic->chip, it, type, prio);
imsic_it_disable(it);
imsic_it_clear_pending(it); So that the driver also works for IMSIC systems without APLIC. |
To be honest, I don't know. The AIA specification states that the APLIC can have multiple interrupt domains, so I guess it may be used in the future. However, if we want to support multiple APLICs (NUMA), there will be multiple |
OK. Let's put this use case aside and merge infrastructure with single APLIC domain first. |
@HuangBorong please fix the checkpatch errors too. |
dc74094
to
3530ddc
Compare
Hi @gagachang , I fixed the issues we discussed earlier in commit 3530ddc. |
The checkpatch errors are mainly due to a missing 'Signed-off-by' and some misspellings. I will squash the commit and fix these issues after the code review. |
LGTM. Please squash the commits and fix the |
Hi @HuangBorong , what's the status of Supervisor top interrupt CSR (stopi)? |
When I tested the APLIC and IMSIC drivers, I find it's OK to use the old |
Sure. Let's discuss them in future PR. This PR should focus on drivers. Thanks! |
return TEE_ERROR_ITEM_NOT_FOUND; | ||
} | ||
|
||
TEE_Result aplic_init_from_device_tree(struct aplic_data *aplic) |
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.
@HuangBorong, Another topic is how to differentiate secure/non-secure APLIC S-level interrupt domains in one device tree.
In general, OP-TEE is assigned a dedicated APLIC S-level interrupt domain, while Linux is assigned another APLIC S-level interrupt domain.
If OP-TEE and Linux share one DTB, they must probe its own interrupt domain only.
ARM has "secure-status", see https://www.kernel.org/doc/Documentation/devicetree/bindings/arm/secure.txt.
RISC-V needs similar way. Do you have any idea?
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.
@HuangBorong, Another topic is how to differentiate secure/non-secure APLIC S-level interrupt domains in one device tree. In general, OP-TEE is assigned a dedicated APLIC S-level interrupt domain, while Linux is assigned another APLIC S-level interrupt domain. If OP-TEE and Linux share one DTB, they must probe its own interrupt domain only.
ARM has "secure-status", see https://www.kernel.org/doc/Documentation/devicetree/bindings/arm/secure.txt. RISC-V needs similar way. Do you have any idea?
I think the simplest solution is to use a dedicated DTB for OP-TEE and another DTB for Linux and OpenSBI. However, if OP-TEE and Linux share a single DTB, the current APLIC DTS documentation (Linux/Documentation/devicetree/bindings/interrupt-controller/riscv,aplic.yaml) does not provide a way to distinguish between secure and non-secure APLIC S-level interrupt domains. Perhaps we could propose adding a property in the APLIC DTS, similar to ARM, to indicate whether the APLIC interrupt domains are secure or non-secure.
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 think the simplest solution is to use a dedicated DTB for OP-TEE and another DTB for Linux and OpenSBI.
Yeah, I have the same thought as you. OP-TEE has CFG_EMBED_DTB
and CFG_EMBED_DTB_SOURCE_FILE
to embed a dedicated DTB into OP-TEE image. This might be simplest solution.
Perhaps we could propose adding a property in the APLIC DTS, similar to ARM, to indicate whether the APLIC interrupt domains are secure or non-secure.
Since RISC-V's security model is "multiple supervisor domains" not just two secure/non-secure worlds, I don't think we want to use "secure-status". We need a more general property.
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.
Since RISC-V's security model is "multiple supervisor domains" not just two secure/non-secure worlds, I don't think we want to use "secure-status". We need a more general property.
Maybe we can propose adding a property in the APLIC DTS called supervisor-domain-id
. Linux or OP-TEE can read this property and compare it with its own domain ID. If they match, then the APLIC interrupt domain can be used; otherwise, it will be skipped.
Since this requires modifying the current APLIC DTS, I think it will need further discussion.
badd7b0
to
692b624
Compare
@HuangBorong With the above comments are addressed, please add my tag:
|
692b624
to
6469b49
Compare
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.
Acked-by: Etienne Carriere <etienne.carriere@foss.st.com>
with below comments addressed or not.
Maybe shorten these 2 commit message header lines:
-drivers: implement Advanced Platform-Level Interrupt Controller (APLIC) driver
+drivers: add RiscV APLIC interrupt driver
-drivers: implement Incoming MSI Controller (IMSIC) driver
+drivers: add RiscV IMSIC interrupt driver
It may be worth adding 2 CI build make commands:
_make PLATFORM=virt CFG_RISCV_PLIC=n CFG_RISCV_APLIC=y
_make PLATFORM=virt CFG_RISCV_PLIC=n CFG_RISCV_APLIC_MSI=y CFG_RISCV_IMSIC=y
The RISC-V Advanced Interrupt Architecture (AIA) specification introduces the IMSIC as a new external interrupt controller. An IMSIC receives and records incoming message-signaled interrupts (MSIs). This commit enables the initialization of the IMSIC based on the device tree and adds control and status registers (CSRs) for indirect access to the IMSIC as well as for reading interrupt identities. Use the `CFG_RISCV_IMSIC` flag to control whether to build this driver. For more details, see: https://github.com/riscv/riscv-aia Signed-off-by: Huang Borong <huangborong@bosc.ac.cn> Reviewed-by: Alvin Chang <alvinga@andestech.com> Acked-by: Etienne Carriere <etienne.carriere@foss.st.com>
The RISC-V Advanced Interrupt Architecture (AIA) specification introduces the APLIC, which can serve as a new external interrupt controller to replace the original Platform-Level Interrupt Controller (PLIC) or as a device to convert wired interrupts into message-signaled interrupts (MSIs) and forward them to the Incoming MSI Controller (IMSIC). The APLIC driver supports both "direct delivery mode" and "MSI delivery mode." Use the `CFG_RISCV_APLIC` flag to enable the APLIC driver in "direct delivery mode," and use the `CFG_RISCV_APLIC_MSI` flag to enable the APLIC driver in "MSI delivery mode" when selecting `CFG_RISCV_IMSIC`. APLIC initialization can be done through the device tree. For more details, see: https://github.com/riscv/riscv-aia Signed-off-by: Huang Borong <huangborong@bosc.ac.cn> Reviewed-by: Alvin Chang <alvinga@andestech.com> Acked-by: Etienne Carriere <etienne.carriere@foss.st.com>
- Add APLIC and IMSIC configurations for the QEMU virt platform. - Override the interrupt controller initialization and interrupt handler functions when using APLIC or IMSIC. Signed-off-by: Huang Borong <huangborong@bosc.ac.cn> Reviewed-by: Alvin Chang <alvinga@andestech.com> Acked-by: Etienne Carriere <etienne.carriere@foss.st.com>
- Add build job for QEMU virt platform with APLIC only. - Add build job for QEMU virt platform with both APLIC and IMSIC. Signed-off-by: Huang Borong <huangborong@bosc.ac.cn> Acked-by: Etienne Carriere <etienne.carriere@foss.st.com>
6469b49
to
af2d7b8
Compare
Fix related issues and add CI build in commit af2d7b8. |
Implement the APLIC and IMSIC drivers which introduced by the "RISC-V Advanced Interrupt Architecture" specification: https://github.com/riscv/riscv-aia
The drivers support two modes:
Some systems without IMSIC only have APLIC, which replaces the original PLIC as the Advanced PLIC. The role of APLIC is to serve as interrupt controllers.
In these systems, both APLIC and IMSIC are present. The harts receive external interrupts only in the form of MSIs. Therefore, the role of APLIC is to convert wired interrupts into MSIs for harts, while the interrupt controllers in these systems are IMSICs.
There are some limitations in the drivers: