Skip to content
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

Merged
merged 4 commits into from
Mar 3, 2025

Conversation

HuangBorong
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

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.

Copy link
Contributor

@etienne-lms etienne-lms left a 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.

@HuangBorong
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor

@maroueneboubakri Do you have interest to review this PR?

Copy link
Contributor

@etienne-lms etienne-lms left a 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.

@gagachang
Copy link
Contributor

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
Copy link
Contributor

gagachang commented Feb 7, 2025

@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
Copy link
Contributor Author

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

@gagachang
Copy link
Contributor

@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
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

Thanks! I can receive UART interrupt now.

Great! It seems the APLIC direct mode works well.

@gagachang
Copy link
Contributor

@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 HuangBorong force-pushed the dev-aia-driver branch 2 times, most recently from a3b6f79 to ae8b182 Compare February 18, 2025 09:19
@HuangBorong
Copy link
Contributor Author

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
Copy link
Contributor

gagachang commented Feb 21, 2025

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor

@HuangBorong please fix the checkpatch errors too.

@HuangBorong
Copy link
Contributor Author

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

@HuangBorong
Copy link
Contributor Author

@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
Copy link
Contributor

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
Copy link
Contributor

@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
Copy link
Contributor Author

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
Copy link
Contributor

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!

return TEE_ERROR_ITEM_NOT_FOUND;
}

TEE_Result aplic_init_from_device_tree(struct aplic_data *aplic)
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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.

@HuangBorong HuangBorong force-pushed the dev-aia-driver branch 2 times, most recently from badd7b0 to 692b624 Compare February 27, 2025 08:29
@gagachang
Copy link
Contributor

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

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

Copy link
Contributor

@etienne-lms etienne-lms left a 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>
@HuangBorong
Copy link
Contributor Author

Fix related issues and add CI build in commit af2d7b8.

@jforissier jforissier merged commit 1717e0d into OP-TEE:master Mar 3, 2025
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants