[RFC] riscv: drivers: Implement APLIC and IMSIC drivers #7236

HuangBorong · 2025-01-24T03:10:28Z

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:

APLIC only:
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.
APLIC + IMSIC:
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:

It is assumed that the OP-TEE OS runs in S-mode. Consequently, the APLIC driver only interacts with the supervisor-level interrupt domain, while IMSIC only interacts with supervisor-level interrupt files.
It relies on OpenSBI to perform some initialization to APLIC and IMSIC in M-mode.
It does not support systems with multiple APLICs (NUMA).
It does not support multiple interrupt domains in APLIC; in other words, it only supports a single S-level interrupt domain.

HuangBorong · 2025-01-24T03:19:18Z

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?

jenswi-linaro · 2025-01-24T07:00:30Z

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.

etienne-lms

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.

core/drivers/aplic_msi.c

core/drivers/aplic_direct.c

core/drivers/aplic_priv.c

core/include/drivers/aplic_priv.h

HuangBorong · 2025-01-24T09:03:13Z

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.

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?

jenswi-linaro · 2025-01-24T10:29:32Z

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.

gagachang · 2025-02-07T05:56:54Z

@maroueneboubakri Do you have interest to review this PR?

etienne-lms

Some coding style and other minor comments.

core/arch/riscv/include/riscv.h

core/drivers/imsic.c

core/drivers/aplic_priv.c

gagachang · 2025-02-07T09:51:41Z

Hi @HuangBorong , An year ago I implemented a simple UART RX interrupt handler to test secure interrupt.
You can add the following code into plat-virt/main.c to test it.
Just boot to OP-TEE, enable interrupts, and enter some keys in OP-TEE console to see if something is printed.
The code is old, you may need to modify the code to be compatible with current OP-TEE.

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);

gagachang · 2025-02-07T11:28:59Z

@HuangBorong , About the Supervisor top interrupt CSR (stopi), yes we definitely need it.
A simple solution is to implement AIA version of thread_native_interrupt_handler().
For example:

#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

core/drivers/aplic_direct.c

core/drivers/aplic_msi.c

core/drivers/aplic_priv.c

core/drivers/imsic.c

core/drivers/aplic_priv.c

HuangBorong · 2025-02-14T02:40:33Z

@gagachang Thank you for your review! I have fixed the issues you mentioned above in commit 5d901b7.

gagachang · 2025-02-14T02:44:35Z

@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 ?

core/drivers/imsic.c

HuangBorong · 2025-02-14T04:16:47Z

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 test-aplic-direct, which is dedicated to testing APLIC direct mode in OP-TEE.

To test the secure interrupt, I’ve done the following:

Added a simple timer in QEMU virt: It can be set to a specific duration and trigger an external interrupt.
Enabled M-mode interrupts after OpenSBI completes initialization and before returning to OP-TEE.
Since there is no Linux, S-mode interrupts are enabled when OP-TEE completes its initialization, and OP-TEE enters an infinite loop after initialization.
Added an itr_handler for the timer.
Added the UART RX you provided earlier.

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.

gagachang · 2025-02-16T15:05:42Z

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 CFG_16550_UART.
And the UART IP interrupt must be enabled. You need to add code to configure UART_IER register bit 0 in ns16550_init().

HuangBorong · 2025-02-17T03:08:30Z

Sorry I forgot to give you some configurations. You need to enable CFG_16550_UART.
And the UART IP interrupt must be enabled. You need to add code to configure UART_IER register bit 0 in ns16550_init().

Thanks! I can receive UART interrupt now.

gagachang · 2025-02-17T03:13:59Z

Thanks! I can receive UART interrupt now.

Great! It seems the APLIC direct mode works well.

core/arch/riscv/plat-virt/conf.mk

core/arch/riscv/plat-virt/main.c

core/arch/riscv/plat-virt/platform_config.h

Implement the IMSIC driver introduced by the AIA specification, enabling initialization based on the device tree. Add CSRs for indirect access to the IMSIC and for reading interrupt identity. Signed-off-by: Huang Borong <[email protected]>

…C) driver Implement the APLIC driver introduced by the AIA specification, supporting direct delivery mode and MSI delivery mode. Initialization can be done through the device tree. Signed-off-by: Huang Borong <[email protected]>

Signed-off-by: Huang Borong <[email protected]>

…mation 1. Fix indentation issues 2. Fix variable initialization problems 3. Reorder header files alphabetically 4. Remov redundant comments and add new ones for clarity 5. Add open-source license information as part of secondary development based on OpenSBI 6. Refactor portions of the code to improve readability

- Change condition to check for NULL address - Prevent potential NULL pointer dereference

- Fix indentation issues - Add error handling for device tree parsing - Refactor IMSIC CSR access functions - Update platform configuration for IMSIC - Fix some code style issues

- Bug Fixes: Resolved identified issues to ensure proper functionality - Code Refactoring: Removed redundant and unnecessary code segments - Code Style Enhancements: Improved coding style

gagachang · 2025-02-18T09:10:21Z

@HuangBorong You can hide the resolved reviews so that the reviewers can know which reviews were resolved.
BTW, do you have test environment or idea for IMSIC/MSI?
PCIe devices are most common devices inject MSI, but we don't have PCIe drivers in OP-TEE.
Maybe the easiest way is to triggering IPI via MSI ? E.g., OP-TEE hart0 writes OP-TEE hart1 IMSIC.

- Update interrupt controller configuration macros - Fix coding sytle issues

HuangBorong · 2025-02-18T09:40:04Z

BTW, do you have test environment or idea for IMSIC/MSI?

I haven't tested the IMSIC/MSI yet.

PCIe devices are most common devices inject MSI, but we don't have PCIe drivers in OP-TEE.
Maybe the easiest way is to triggering IPI via MSI ? E.g., OP-TEE hart0 writes OP-TEE hart1 IMSIC.

Could APLIC MSI delivery mode test the IMSIC? The APLIC can convert the wire interrupt into an MSI write and send it to IMSIC.
In this version of the code, I have not implemented the IPI handler for the IMSIC.

gagachang · 2025-02-18T10:04:47Z

Could APLIC MSI delivery mode test the IMSIC? The APLIC can convert the wire interrupt into an MSI write and send it to IMSIC. In this version of the code, I have not implemented the IPI handler for the IMSIC.

Oh! You are right. We can configure secure timer or UART to use APLIC MSI delivery mode.
IPI is unnecessary, please ignore my comment.

gagachang · 2025-02-18T11:15:50Z

core/arch/riscv/plat-virt/conf.mk

@@ -28,6 +28,9 @@ $(call force,CFG_BOOT_SYNC_CPU,n)

 # Interrupt controller
 CFG_RISCV_PLIC ?= y
+CFG_RISCV_APLIC ?= n
+CFG_RISCV_APLIC_MSI ?= n
+CFG_RISCV_IMSIC ?= n


CFG_RISCV_PLIC is incompatible with the other three configurations.
I think they must not be enabled simultaneously.
Recommend to add check on this case:

ifeq ($(CFG_RISCV_PLIC)-$(call cfg-one-enabled,CFG_RISCV_APLIC CFG_RISCV_APLIC_MSI CFG_RISCV_IMSIC),y-y) $(error PLIC is incompatible with AIA) endif

The check can be added into core/arch/riscv/riscv.mk.

gagachang · 2025-02-18T11:22:16Z

core/drivers/imsic.c

+	if (id == IMSIC_IPI_ID)
+		DMSG("Interprocessor interrupt");
+
+	if (id > 1 && id <= imsic->num_ids)


Prefer using macro instead of hardcode.

if (IMSIC_IPI_ID < id && id <= imsic->num_ids)

gagachang · 2025-02-18T11:25:47Z

core/drivers/imsic.c

+		imsic->hart_index_bits = fdt32_to_cpu(*val);
+	else
+		imsic->hart_index_bits = 32 -
+			__builtin_clz(CFG_TEE_CORE_NB_CORE - 1);


Suggest to put calculation in one line:

imsic->hart_index_bits = 32 - __builtin_clz(CFG_TEE_CORE_NB_CORE - 1);

etienne-lms reviewed Jan 24, 2025

View reviewed changes

HuangBorong force-pushed the dev-aia-driver branch from dabed47 to d0e5ed0 Compare February 6, 2025 04:06

etienne-lms reviewed Feb 7, 2025

View reviewed changes