Details

Reviewers

Commits

rG6ec8bf9f3d17: riscv: Convert local interrupt controller to a newbus PIC

Summary

Currently the local interrupt controller implementation is based on
pre-INTRNG arm/arm64 code, using hand-rolled event code rather than
INTRNG. This then interacts weirdly with the PLIC, and other future
interrupt controllers like the APLIC and IMSICs in the upcoming AIA
specification, since they become the root PIC despite not being the
logical root. Instead, use a real newbus device for it and register
it as the root PIC.

This also adapts the IPI code to make use of the newly-added INTRNG
generic IPI handling framework, adding a new sbi_ipi as the PIC. In
future there will be alternative devices for sending IPIs that will
register with higher priorities, such as the proposed AIA IMSIC and
ACLINT SSWI.

MFC after: 1 month

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

jrtc27 created this revision.Jul 24 2022, 9:58 PM

Herald added a subscriber: imp. · View Herald TranscriptJul 24 2022, 9:58 PM

jrtc27 requested review of this revision.Jul 24 2022, 9:58 PM

Harbormaster completed remote builds in B46539: Diff 108478.Jul 24 2022, 9:58 PM

jrtc27 added a parent revision: D35900: riscv: Create a newbus device for the SBI driver.Jul 24 2022, 9:58 PM

jrtc27 added a parent revision: D35899: intrng: Allow alternative IPI PICs to be registered and used.

jrtc27 added a parent revision: D35897: intrng: Remove irq_root_ipicount and corresponding intr_pic_claim_root arg.Jul 24 2022, 10:01 PM

Fix KTR build

Harbormaster completed remote builds in B46551: Diff 108500.Jul 25 2022, 5:15 PM

Hi, thanks for your patience while I've been away. This series improves a lot.

Can you explain a bit further the design/organization of components after all these changes? Such as the PIC and newbus hierarchies, and how the interrupt drivers we have now will be extended or replaced by the newer ACLINT, APLIC, AMSIC, etc. I do find it a little counter-intuitive that the intc becomes the root PIC, so I'd like to understand that more.

Also, we will get one intc device for the system, not $ncpu devices, yes?

Finally, I think it would be preferable to move the new IPI device to a separate new file (sbi_ipi.c or ipi.c or whatever), rather than adding it to sbi.c. Since, in the long term we will gain better mechanisms to inject IPIs, that code will become more and more legacy over time, and will be adequately sectioned off. Then, sbi.c can remain a simple toolbox of SBI wrapper functions, which we will (presumably) always need.

sys/riscv/include/smp.h
40–49	I would commit this hunk now 👍
47	lol
sys/riscv/riscv/sbi.c
348
371–373	Why do we call this now? You aren't really trying to attach the device, just set `sc->deferred_attach`, am I wrong?
396–400	May as well group this here.
441	Under what circumstances will it fail to attach?
sys/riscv/riscv/timer.c
232

In D35901#831453, @mhorne wrote:

Hi, thanks for your patience while I've been away. This series improves a lot.

Can you explain a bit further the design/organization of components after all these changes? Such as the PIC and newbus hierarchies, and how the interrupt drivers we have now will be extended or replaced by the newer ACLINT, APLIC, AMSIC, etc. I do find it a little counter-intuitive that the intc becomes the root PIC, so I'd like to understand that more.

I'll answer the shorter questions first and tack the answer for this one on the end.

Also, we will get one intc device for the system, not $ncpu devices, yes?

Yes; only one INTC will exist, and it will be associated with the boot hart's compatible=riscv,cpu-intc node, whichever one won the race to be the BSP.

Finally, I think it would be preferable to move the new IPI device to a separate new file (sbi_ipi.c or ipi.c or whatever), rather than adding it to sbi.c. Since, in the long term we will gain better mechanisms to inject IPIs, that code will become more and more legacy over time, and will be adequately sectioned off. Then, sbi.c can remain a simple toolbox of SBI wrapper functions, which we will (presumably) always need.

I know at one point that's what I intended to do, but for whatever reason I didn't. Probably I saw it as unnecessary complexity when sitting down to write it, but I suspect that was before I added all the bus_new_pass bits that you point out aren't great and would be cleaned up by that removal.

As for your first question, the INTC is a real interrupt controller, just one that's glued to CSRs rather than memory-mapped registers, providing a set of PPIs. In the device tree it is marked with interrupt-controller, and the PLIC logically and physically sends interrupts to it. There are other interrupt sources other than the PLIC, such as the timer (whether firmware-emulated or Sstimecmp), software interrupts (whether firmware-emulated or from an ACLINT SSWI) or local counter overflow interrupts (from Sscofpmf); the PLIC is only the root of the external interrupts. The current scheme of manually matching interrupts in intr_machdep.c requires a bunch of code to do what INTRNG can already do, but with less flexibility. It's already doing most of what a PIC does anyway, just not via the PIC interface. You can think of INTC as being the PPI part of the Arm GIC driver, which is fully modelled as a PIC.

As for the hierarchies (these should all be valid dot syntax if you shove digraph { ... } around them), you currently have (INTRNG-wise, and giving the full set of devices even though we don't support all of them yet):

"SBI Timer";
"S-mode Timer";
"SBI IPI";
"ACLINT SSWI";
HPM;

Device -> PLIC [label="wire"]

with a whole bunch of devices off on the side that need interrupts but don't go through INTRNG.

After this change you get the following for a "full" PLIC-based setup:

PLIC -> INTC [label="external"]
"SBI Timer" -> INTC [label="timer"]
"S-mode Timer" -> INTC [label="timer"]
"SBI IPI" -> INTC [label="software"]
"ACLINT SSWI" -> INTC [label="software"]
HPM -> INTC [label="local counter overflow"]

Device -> PLIC [label="wire"]

In a world where the PLIC becomes an APLIC you end up with a similar setup, just a somewhat more featureful controller:

APLIC -> INTC [label="external"]
"SBI Timer" -> INTC [label="timer"]
"S-mode Timer" -> INTC [label="timer"]
"SBI IPI" -> INTC [label="software"]
"ACLINT SSWI" -> INTC [label="software"]
HPM -> INTC [label="local counter overflow"]

Device -> APLIC [label="wire"]

In a world where you do away with wired interrupts and have MSIs, it looks somewhat similar again, though the "connection" to the IMSIC is via the memory subsystem:

IMSIC -> INTC [label="external"]
"SBI Timer" -> INTC [label="timer"]
"S-mode Timer" -> INTC [label="timer"]
"SBI IPI" -> INTC [label="software"]
"ACLINT SSWI" -> INTC [label="software"]
HPM -> INTC [label="local counter overflow"]

Device -> IMSIC [label="MSI"]

But there's also the world where you have both MSIs and wired interrupts, and in this world you have an APLIC configured to forward wired interrupts as MSIs:

IMSIC -> INTC [label="external"]
"SBI Timer" -> INTC [label="timer"]
"S-mode Timer" -> INTC [label="timer"]
"SBI IPI" -> INTC [label="software"]
"ACLINT SSWI" -> INTC [label="software"]
HPM -> INTC [label="local counter overflow"]

"Device 1" -> IMSIC [label="MSI"]
APLIC -> IMSIC [label="MSI"]

"Device 2" -> APLIC [label="wire"]

Without this reorganisation to make the INTC the real PIC root, you'd have to make the APLIC sometimes be the root (in the no-IMSIC case) and sometimes be a child PIC (in the IMSIC case). With this change the external interrupt controllers are always children and there's only ever one root, that's always the same root, the INTC, and life is less complicated.

As far as newbus is concerned, it's flat, they all live at the nexus, ofwbus (/) or simplebus (/soc) level. You get:

nexus0
+- sbi0 (and an sbi_ipi0 child if added)
+- timer0
+- ofwbus0
   +- intc0
   +- simplebus0
      +- plic0

The only weirdness is where intc0 lives; in the device tree there's a node under each /cpus/cpu@N, but this hangs it off ofwbus0 (can't go higher since we need it to be in the OFW world) rather than cpuN since we don't probe cpuN until much later during boot and this is where a saner device tree binding would have put it (Linux also doesn't appreciate the bindings and just has one instance of its driver so would work better with such a binding too).

For how things will change in our drivers as new hardware parts come along:

ACLINT - MTIMER and MSWI are M-mode things, SSWI is all that's left, and it just becomes a new sswi0 that gets used for injecting software interrupts in preference to sbi_ipi0 (but still MUXes just like it)
APLIC - until an IMSIC driver exists it's just another PLIC-like driver that gets used instead, and once we have IMSIC support it will also need to handle routing interrupts as MSIs to an IMSIC rather than just being a beefed-up PLIC like we have today (I don't know if that would be best implemented as one driver that changes behaviour or two drivers that implement the different behaviour)
IMSIC - this would implement the MSI interface instead of the PIC interface but otherwise take the same place as the PLIC today as its own driver

Hopefully that clears things up somewhat, though having not implemented APLIC or IMSIC support I can't say for certainty that there aren't wrinkles there (I have a basic SSWI implementation somewhere, though it doesn't handle the multi-socket case where each socket has its own SSWI for that set of harts but worked to prove the concept of this big refactoring and supporting priority-based IPI PIC selection).

sys/riscv/include/smp.h
40–49	I'd have to change the existing code to use `1 << IPI_FOO` rather than `IPI_FOO` for that only to then later change it in this patch. Currently the MUXing nature of IPIs is exposed outside the code to do the MUXing by using these bitmasks.
sys/riscv/riscv/sbi.c
371–373	The theory was this centralises the decision of when the IPI part needs to attach into sbi_ipi_attach, though the code doesn't quite achieve that as it won't work if sbi_attach runs at a high enough pass already for sbi_ipi_attach to not need to defer the attachment. Moving to a separate sbi_ipi would avoid that though.
396–400	I did it this way so the code that was just being moved stayed as untouched as possible, making it easier (with git diff -w --color-moved=zebra) to see what actually changed vs just got relocated.
441	All the error cases below, which are probably all "your system is on fire and not going to boot" but given we attempt to handle errors below I figured we should at least not spam the console with the same errors on every new bus pass. Hoisting this out to a child would avoid the issue though, as that could have its pass set to the right thing for IPIs and not bother with bus_new_pass.

Hi, I am finally circling back to this. Thanks a lot for the detailed reply, it definitely paints a clear picture.

Overall I think the series is in good shape, and a necessary improvement, so we should try to land it after stable/14 branches off. Most of my comments are advisory/trivial, so I'd say that when you have the chance just rebase and push what's here. The changes are more valuable in the tree, and whatever polish is necessary can be done after the fact.

I have some cycles I can dedicate to helping rebase/test/whatever, so let me know if that's something you would like.

This revision is now accepted and ready to land.Apr 26 2023, 6:03 PM

Herald added a subscriber: riscv. · View Herald TranscriptApr 26 2023, 6:03 PM

jrtc27 mentioned this in D43293: RISCV: Introduce support for APLIC interrupt controller.Jan 3 2024, 4:36 PM

Hi @mhorne @jrtc27,

I was trying to rebase and adapt my IMSIC patch to this one.
But the patch doesn't apply cleanly. I am using main branch of FreeBSD 15.0-CURRENT (at commit id 6caa19a08b8).

Do you have an updated patch?

Rebased (will split out sbi_ipi in a future update)

This revision now requires review to proceed.Jan 19 2024, 8:46 PM

Harbormaster completed remote builds in B55507: Diff 133070.Jan 19 2024, 8:46 PM

himanshu_thechauhan.dev added inline comments.Jan 20 2024, 8:55 AM

sys/riscv/riscv/plic.c
395	src->irq_res is not freed.
488	Since we are changing plic.c, changing this to DEFINE_CLASS_0 would make sense. New contributors get idea from what and how APIs are used.
sys/riscv/riscv/sbi.c
431	I general query I had. If the child interrupt controller is attached first and the parent later, what approach should be taken to register child with parent later on?
472	The previously allocated resources and inter_isrc_register are not freed/deregistered. Shouldn't they be freed?

Fix LINT (KTR) build

Harbormaster completed remote builds in B55531: Diff 133130.Jan 21 2024, 10:56 PM

jrtc27 added inline comments.Jan 21 2024, 11:05 PM

sys/riscv/riscv/plic.c
395	There's no detach support either. If your PLIC doesn't attach then you're not going to get very far and there's not much point cleaning up. Note that mem_res (previously intc_res) isn't freed in the original code. There's just not much point trying to make interrupt controllers clean up like that.
488	I'm not touching completely unrelated code in this commit, that's an entirely independent change, and there are many many cases of this pattern being used, so I'm not really sure what the benefit of changing just one driver over is...
sys/riscv/riscv/sbi.c
431	Normally you adjust the pass and order within that pass such that it doesn't happen
472	If this breaks you have bigger problems than a resource leak

Split out new sbi_ipi device

Harbormaster completed remote builds in B55534: Diff 133135.Jan 22 2024, 1:52 AM

jrtc27 edited the summary of this revision. (Show Details)Jan 22 2024, 1:52 AM

I am pretty sure this is good. But I don't understand the addition of the resource list management to sbi.c. Why shouldn't these requests just be passed to nexus?

sys/riscv/riscv/sbi_ipi.c
5	You can drop this.

In D35901#992938, @mhorne wrote:

I am pretty sure this is good. But I don't understand the addition of the resource list management to sbi.c. Why shouldn't these requests just be passed to nexus?

That was my initial thought, and that still sort of happens (in that, like most other buses, it eventually bubbles up to nexus). The problem is that the actual resources need to be stored in a struct resource_list somewhere, and in the case of nexus that's done in the child's ivars. But if you're a grandchild of nexus you won't have a struct nexus_device for your ivars, so you can't have nexus manage your resource list. See bus_set_resource, for example, which only works for a direct child.

Resource bits in sbi.c using a local resource_list in ivars and then using bus_generic_rl_* is all fine.

sys/riscv/riscv/mp_machdep.c
197–198

mhorne accepted this revision.Jan 24 2024, 8:05 PM

This revision is now accepted and ready to land.Jan 24 2024, 8:05 PM

Closed by commit rG6ec8bf9f3d17: riscv: Convert local interrupt controller to a newbus PIC (authored by jrtc27). · Explain WhyJan 24 2024, 11:51 PM

This revision was automatically updated to reflect the committed changes.

jrtc27 added a commit: rG6ec8bf9f3d17: riscv: Convert local interrupt controller to a newbus PIC.

riscv: Convert local interrupt controller to a newbus PIC
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 133338

sys/conf/files.riscv

sys/riscv/include/intr.h

sys/riscv/include/smp.h

sys/riscv/riscv/intc.c

sys/riscv/riscv/intr_machdep.c

sys/riscv/riscv/mp_machdep.c

sys/riscv/riscv/plic.c

sys/riscv/riscv/riscv_console.c

sys/riscv/riscv/sbi.c

sys/riscv/riscv/sbi_ipi.c

sys/riscv/riscv/timer.c

sys/riscv/riscv/trap.c

riscv: Convert local interrupt controller to a newbus PICClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 133338

sys/conf/files.riscv

sys/riscv/include/intr.h

sys/riscv/include/smp.h

sys/riscv/riscv/intc.c

sys/riscv/riscv/intr_machdep.c

sys/riscv/riscv/mp_machdep.c

sys/riscv/riscv/plic.c

sys/riscv/riscv/riscv_console.c

sys/riscv/riscv/sbi.c

sys/riscv/riscv/sbi_ipi.c

sys/riscv/riscv/timer.c

sys/riscv/riscv/trap.c

riscv: Convert local interrupt controller to a newbus PIC
ClosedPublic
Actions

Revision Contents
Changeset List