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

[HuangBorong]

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:

1. 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.
2. 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:

1. 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.
2. It relies on OpenSBI to perform some initialization to APLIC and IMSIC in M-mode.
3. It does not support systems with multiple APLICs (NUMA).
4. It does not support multiple interrupt domains in APLIC; in other words, it only supports a single S-level interrupt domain.

[HuangBorong]

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]

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.

[HuangBorong]

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]

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]

@maroueneboubakri Do you have interest to review this PR?

[etienne-lms]

Some coding style and other minor comments.

[gagachang]

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]

@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

[HuangBorong]

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

[gagachang]

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

[HuangBorong]

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:

1. Added a simple timer in QEMU virt: It can be set to a specific duration and trigger an external interrupt.
2. Enabled M-mode interrupts after OpenSBI completes initialization and before returning to OP-TEE.
3. 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.
4. Added an itr_handler for the timer.
5. 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]

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]

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]

Thanks! I can receive UART interrupt now.

Great! It seems the APLIC direct mode works well.

[gagachang]

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

[HuangBorong]

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 test-aplic-msi branch.

I refactored the IMSIC interrupt handler in commit 99e2277. Here are the changes I made:

1. In the previous version, All external interrupts from APLIC shared share a single EIID, which was defined in the macro APLIC_DEFAULT_EIID. In this version, I have made the APLIC interrupt sources number be the EIID directly.
2. Added a pointer in imsic_data which points to aplic_data
3. When calling interrupt_configure(), the imsic_op_add() function now also calls aplic_op_add().

[gagachang]

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 test-aplic-msi branch.

I refactored the IMSIC interrupt handler in commit 99e2277. Here are the changes I made:

1. In the previous version, All external interrupts from APLIC shared share a single EIID, which was defined in the macro APLIC_DEFAULT_EIID. In this version, I have made the APLIC interrupt sources number be the EIID directly.
2. Added a pointer in imsic_data which points to aplic_data
3. When calling interrupt_configure(), the imsic_op_add() function now also calls aplic_op_add().

Amazing work! Thanks.
One quick problem is that the code assumes IMSIC and APLIC are always tied together.
A platform can adopt IMSIC independently. There is no APLIC behind the IMSIC in this case.

Besides, I think the relationship between APLIC and IMSIC in your code should be reversed.
If IMSIC and APLIC are tied together, IMSIC is parent of APLIC.
You can check APLIC device node msi-parent, and code in linux APLIC-MSI driver.

IMO, the itr_main_chip should be APLIC not IMSIC.
We can introduce parent member into struct itr_chip which points to parent struct itr_chip.
Call aplic_op_add() will check if it has parent (IMSIC in our case) and call imsic_op_add() .
What do you think?

[HuangBorong]

One quick problem is that the code assumes IMSIC and APLIC are always tied together.
A platform can adopt IMSIC independently. There is no APLIC behind the IMSIC in this case.

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

[HuangBorong]

Hi @gagachang

Besides, I think the relationship between APLIC and IMSIC in your code should be reversed.
If IMSIC and APLIC are tied together, IMSIC is parent of APLIC.
You can check APLIC device node msi-parent, and code in linux APLIC-MSI driver.

I believe you are correct. The IMSIC should be the parent of the APLIC.
In the Linux APLIC-MSI driver, when enabling(unmasking) an IRQ, it first enables it on the IMSIC and then on the APLIC. Conversely, when disabling(masking) an IRQ, it performs the reverse: it first disables the IRQ on the APLIC and then on the IMSIC.

IMO, the itr_main_chip should be APLIC not IMSIC.

I believe the itr_main_chip should be IMSIC instead, as it should point to the parent (root) interrupt controller, especially if we plan to add multiple APLIC interrupt domains in the future.

We can introduce parent member into struct itr_chip which points to parent struct itr_chip.

I believe the current struct itr_chip is acceptable. The itr_main_chip uses a pointer points to imsic_data and imsic_data uses a pointer points to aplic_data. Currently, it only supports a single S-level APLIC interrupt domain.

[gagachang]

I believe you are correct. The IMSIC should be the parent of the APLIC. In the Linux APLIC-MSI driver, when enabling(unmasking) an IRQ, it first enables it on the IMSIC and then on the APLIC. Conversely, when disabling(masking) an IRQ, it performs the reverse: it first disables the IRQ on the APLIC and then on the IMSIC.

That's correct.

I believe the itr_main_chip should be IMSIC instead, as it should point to the parent (root) interrupt controller, especially if we plan to add multiple APLIC interrupt domains in the future.

Can you provide use case to add multiple APLIC interrupt domains into OP-TEE ?

I believe the current struct itr_chip is acceptable. The itr_main_chip uses a pointer points to imsic_data and imsic_data uses a pointer points to aplic_data. Currently, it only supports a single S-level APLIC interrupt domain.

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.

[HuangBorong]

Can you provide use case to add multiple APLIC interrupt domains into OP-TEE ?

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 aplic_data entries, one for each APLIC. This will need to be managed by a data structure such as a linked list.

[gagachang]

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 aplic_data entries, one for each APLIC. This will need to be managed by a data structure such as a linked list.

OK. Let's put this use case aside and merge infrastructure with single APLIC domain first.

[jforissier]

@HuangBorong please fix the checkpatch errors too.

[HuangBorong]

Hi @gagachang , I fixed the issues we discussed earlier in commit 3530ddc.

[HuangBorong]

@HuangBorong please fix the checkpatch errors too.

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.

[gagachang]

Hi @gagachang , I fixed the issues we discussed earlier in commit 3530ddc.

LGTM. Please squash the commits and fix the CI/Code style errors. Otherwise the PR can't be accepted.

[gagachang]

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

Hi @HuangBorong , what's the status of Supervisor top interrupt CSR (stopi)?
Do you plan to add AIA version of thread_native_interrupt_handler() ?

[HuangBorong]

Hi @HuangBorong , what's the status of Supervisor top interrupt CSR (stopi)?
Do you plan to add AIA version of thread_native_interrupt_handler() ?

When I tested the APLIC and IMSIC drivers, I find it's OK to use the old thread_native_interrupt_handler(). These PR I focus on the APLIC and IMSIC drivers. I will add AIA version of thread_native_interrupt_handler() and other parts in AIA spec in the future.

[gagachang]

When I tested the APLIC and IMSIC drivers, I find it's OK to use the old thread_native_interrupt_handler(). These PR I focus on the APLIC and IMSIC drivers. I will add AIA version of thread_native_interrupt_handler() and other parts in AIA spec in the future.

Sure. Let's discuss them in future PR. This PR should focus on drivers. Thanks!

[gagachang]

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

[HuangBorong]

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

[gagachang]

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.

[HuangBorong]

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.

[gagachang]

@HuangBorong With the above comments are addressed, please add my tag:

Reviewed-by: Alvin Chang <alvinga@andestech.com>

[etienne-lms]

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

[HuangBorong]

Fix related issues and add CI build in commit af2d7b8.