Page MenuHomeFreeBSD

intrng: Allow alternative IPI PICs to be registered and used
ClosedPublic

Authored by jrtc27 on Jul 24 2022, 9:56 PM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Apr 9, 12:24 PM
Unknown Object (File)
Mar 17 2024, 10:15 AM
Unknown Object (File)
Mar 17 2024, 10:15 AM
Unknown Object (File)
Mar 17 2024, 10:15 AM
Unknown Object (File)
Mar 17 2024, 10:14 AM
Unknown Object (File)
Mar 17 2024, 10:14 AM
Unknown Object (File)
Mar 14 2024, 5:39 PM
Unknown Object (File)
Jan 25 2024, 1:36 AM

Details

Summary

On RISC-V, the root PIC (whether the PLIC or, as will be the case in
future, the local interrupt controller) cannot send IPIs, relying on
another means to trigger the necessary software interrupts (firmware
calls), but there are upcoming standard devices that will be able to
inject them, so we can't just put the firmware calls in the root PIC
driver.

Thus, split out a new intr_ipi_dev from intr_irq_root_dev to use for
sending IPIs. New devices can be registered with a given priority up
until the first IPI is set up, when the best device seen so far gets
frozen as the IPI device to use.

MFC after: 1 month

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

I don't have a single problem with intr_irq_root_dev partitioning. I feel that this implementation (priority based selection) is probably not viable in the long term and it seems to be RISC-V specific (i.e. not suitable for a platform independent framework). But decoupling the IPI controller from the root controller will allow us to do it all in a platform-specific "virtual" layer, so I don't went block you. But I have a slight problem with keeping the fallback IPI functionality in the root interrupt controller. We actually only have a few root controllers in the codebase, so adding a one-line intr_ipi_pic_register() function to all of them is not a big deal, which an reduce future bloating of spaghettis like functionally. What do you think?

sys/kern/subr_intr.c
387–388

How does this new condition relate to this review and why do you even need it ?

In D35899#817312, @mmel wrote:

I don't have a single problem with intr_irq_root_dev partitioning. I feel that this implementation (priority based selection) is probably not viable in the long term and it seems to be RISC-V specific (i.e. not suitable for a platform independent framework). But decoupling the IPI controller from the root controller will allow us to do it all in a platform-specific "virtual" layer, so I don't went block you.

Using a priority was done to mirror how newbus picks device drivers. It's true that RISC-V will be the only real user of this but I'm not sure what a better approach is whilst maintaining a single INTRNG IPI framework.

But I have a slight problem with keeping the fallback IPI functionality in the root interrupt controller. We actually only have a few root controllers in the codebase, so adding a one-line intr_ipi_pic_register() function to all of them is not a big deal, which an reduce future bloating of spaghettis like functionally. What do you think?

I have no problem doing that, I just thought not changing the existing API's semantics, just extending them, would be preferred.

sys/kern/subr_intr.c
387–388

Cirrently IPIs are counted separately in the intr_ipi's ii_count, with the count for the underlying interrupt not tracked (presumably for performance?). intr_ipi_dispatch is currently called directly from the root GIC/bcm2836 drivers and so don't currently hit this path. However, if you want to have something other than the root controller handling IPI injection, intr_isrc_dispatch will be called instead from the root controller and it will chain through to the IPI PIC in the end. The RISC-V patch has the root controller mark its sole software interrupt ISRC as an IPI in order to preserve this non-counting behaviour, which is distinct from the IPI ISRC created by the IPI PIC and used for PIC_IPI_SEND.

The alternative would be to say they should be marked as PPIs in everything except the IPI PIC, but that would make the counters behave differently.

Either way, intr_isrc_dispatch is currently broken if given an IPI ISRC since isrc_setup_counters isn't called for those, so you either need to stub out the counters for them like we already do elsewhere or forbid that case one way or another.

Rebased (will remove fallback in future update)

Drop root PIC fallback as suggested

mhorne added subscribers: kevans, mhorne.

To me, this is quite sane, and achieves what we need in the immediate present for the purposes of the RISC-V interrupt controller stack.

However, comparing with the ongoing discussion in D40161, and @mmel's proof-of-concept presented there, it seems this functionality can/should be subsumed by the more general support for different types of interrupts/root devices (regular, IPI, FIQ).

I think we should proceed with what is here and ready for commit today, as it is essential for Jess's larger series reworking interrupt organization on riscv, which is also ready for commit. This series is in turn a blocker for newer interrupt controller drivers (D43452). But, we proceed knowing that this code will be reshaped in the near future by the result of D40161.

Tagging @kevans as a relevant party in the discussion.

This revision is now accepted and ready to land.Jan 22 2024, 4:51 PM

To me, this is quite sane, and achieves what we need in the immediate present for the purposes of the RISC-V interrupt controller stack.

However, comparing with the ongoing discussion in D40161, and @mmel's proof-of-concept presented there, it seems this functionality can/should be subsumed by the more general support for different types of interrupts/root devices (regular, IPI, FIQ).

I think we should proceed with what is here and ready for commit today, as it is essential for Jess's larger series reworking interrupt organization on riscv, which is also ready for commit. This series is in turn a blocker for newer interrupt controller drivers (D43452). But, we proceed knowing that this code will be reshaped in the near future by the result of D40161.

Tagging @kevans as a relevant party in the discussion.

I kind of envision IPIs going the other way and becoming divorced from a PIC. They're part of the PIC today because most sane interrupt controllers for MP systems have a way to generate IPIs in the controller itself, but with RISC-V's PLIC that's not true, so we end up having to create dummy PICs for things that aren't interrupt controllers, just a device that can inject interrupts (which really is no different to any other device that generates an external interrupt, other than what triggers them).

However, these new IPIs still generate interrupts/exceptions - so some form of more or less abstract PIC is still needed. Of course, if these instructions have evolved into a fixed part of the CPU (that is, the IPI will be generated by the exact instruction and raise an exact exception), we can hardcode it and omit it from INTRNQ...

I would prefer the opposite approach, i.e. implement even in a simplified form the multiroot concept and then implement RISCV and/or FIQ extensions. The only bottleneck that requires a bit of work is the initialization of secondary PICs, the correct semantics of which is a bit problematic. IMHO one should initialize the local-CPU related part of the current PIC, then pass the initialization to the child PICs, and finally re-initialize the current PIC to the active state. All other methods are open to possible glitches in my opinion. We cannot assume that all these slave PICs are initialized to idle state by reset or by firmware.