arm64/intrng: add support for FIQs

[kevans]

Summary

aarch64 has support for some number of FIQs, fast interrupt requests,
that will be used in the impending Apple Interrupt Controller for, e.g.,
clock interrupts. Let's add in the basic infrastructure now so that we
can ensure it does no harm (e.g. unexpected FIQs) on other platforms.

(Author set to andrew@)

[kevans]

Missed header

[kevans]

CC @karels

[• karels]

sys/kern/subr_intr.c

99

It seems wrong to add a bunch of architecture-specific ifdefs to kern. If there are other architectures with fiq, maybe use a feature-test macro that is defined, e.g. in <machine/intr.h>, and test that here? Or even if this is aarch64-specific?

sys/sys/intr.h

133

If a new feature-test macro is added, this should be conditional on it too instead of aarch64.

[kevans]

sys/kern/subr_intr.c

99

I believe there is no analog for FIQs in !arm64 (some remnants in 32-bit arm's implementation is about to be ripped out, iirc), but I really can't say for sure for RISC-V. CC @jrtc27 @mhorne

[jrtc27]

Why does INTRNG need to know what kind of interrupt this is? Can the root PIC not just figure it out? Or can we pass an opaque flag through the thread or trapframe somewhere (or as an MD flag through INTRNG)? I don't think you want to be teaching INTRNG about something so MD as FIQs.

[kevans]

In D40161#915102, @jrtc27 wrote:

Why does INTRNG need to know what kind of interrupt this is? Can the root PIC not just figure it out? Or can we pass an opaque flag through the thread or trapframe somewhere (or as an MD flag through INTRNG)? I don't think you want to be teaching INTRNG about something so MD as FIQs.

At least in apple silicon case, the root filter /could/ just check all of the possible causes every time, but that's a bit dirty feeling. I'm not so sure it's worth adding an entire field to the trapframe, but adding an md flag field to intr_irq_handler/irq_root_filter seems similarly low risk on other platforms while still fulfilling the need.

[• karels]

Maybe the current code should just be moved under arm64.

[mhorne]

I am having difficulty trying to rationalize the acronym "FIQ" 🤔 😆

In D40161#915103, @kevans wrote:

In D40161#915102, @jrtc27 wrote:

Why does INTRNG need to know what kind of interrupt this is? Can the root PIC not just figure it out? Or can we pass an opaque flag through the thread or trapframe somewhere (or as an MD flag through INTRNG)? I don't think you want to be teaching INTRNG about something so MD as FIQs.

At least in apple silicon case, the root filter /could/ just check all of the possible causes every time, but that's a bit dirty feeling. I'm not so sure it's worth adding an entire field to the trapframe, but adding an md flag field to intr_irq_handler/irq_root_filter seems similarly low risk on other platforms while still fulfilling the need.

You certainly should have a separate filter, the question is whether it has any business (advantage) of being placed in INTRNG, versus keeping it as a machine-dependent hook.

sys/kern/subr_intr.c

99

Agreed with @karels here, having a HAVE_FAST_IRQ or similarly named feature flag defined in machine/intr.h makes it a more stomach-able addition to this code.

I do not think RISC-V has such an analogue today.

385

395–399

I wonder if this should become an actual AST. This is a note to self really.

958

To me this check is an indicator that the proposed structure of the change is not quite right.

For one thing, you could just as well add filter and filter_arg as new parameters to intr_pic_claim_root().

[kevans]

sys/kern/subr_intr.c

958

I'm still munching on this a bit; I agree that this could be seen as a red flag, but I'm less certain about adding filter and filter_arg parameters to intr_pic_claim_root, making it either different on FIQ-having platforms (read as "arm64") or always NULL on every other platform. My concern with the latter is that it's a potential source of confusion in itself.

[kevans]

sys/kern/subr_intr.c

958

I had considered a setup where intr_pic_claim_root takes an array of {filter, arg} pairs; pair[0] is reserved for irq filter and required, every other index is MD. We'd then offer a generic function to invoke the filter at a certain index, along with the existing intr_irq_handler that just calls that with INTR_FILTER_IDX_IRQ (0).

The upside is that it allows this to be arbitrarily extended without having to add some arch-specific terminology to this file. The downside is that it's probably a really weird interface for it.

[kevans]

Another proposal for FIQ support

This is one of the cleaner ideas I came up with that was easy enough to whip up,
so let's start here and continue the discussion.

This version extends INTRNG a little bit more to have the concept of an
interrupt type. I chopped off the high bit to use for the basic IRQ type that's
common to all systems to make it a little harder to mess up platform additions;
they just have to start at 0x0001.

The basic intr_* KPI is extended with an intr_pic_claim_root_type() to claim the
root device and declare the types of interrupts it can route, and an
intr_irq_handler_type() is added for us to indicate a non-IRQ type that we have
fired off. The existing KPI with the same names modulo _type just call into the
new _type variants with INTR_TYPE_IRQ to keep a good chunk of existing code
working.

The biggest incompatibility is that intr_irq_filter_t is no longer compatible
with driver_filter_t, and they can't be used interchangably anymore. In
the few cases where this is done today (in-tree), I just added a _handler for
bus_setup_intr()'s use and called that from the irq filter. I couldn't come up
with a way to avoid some kind of incompatibility here; I had considered making
the opaque arg a struct that wraps the opaque arg and other data, but the
filter callback still needs to know how to differentiate and at that point it
doesn't seem any cleaner.

I included the arm64 specific changes (now limited to sys/arm64) since they're
fairly minimal and demonstrate how I think this would work out; if this idea
isn't outright rejected, I'll split them out in the next iteration.

I also note that this isn't really run-tested at all yet, though it will be
soon.

[andrew]

Could we create a intr_irq_type_filter_t type that is used with intr_pic_claim_root_type? We would then need either root filter pointers, or a union of both with a flag to decide which to use, but would keep the KBI of intr_pic_claim_root and keep intr_irq_filter_t compatible with driver_filter_t.

sys/arm64/include/intr.h

64 ↗

(On Diff #127099)

We should reserve a space for machdep types

[kevans]

Address review feedback

Preserve existing KBI so that we don't require any driver changes, we just shim
out the untyped filters so that they can cleanly keep their driver_intr_t
compatible signatures and avoid getting any interrupt types they're not
expecting from this extension.

Be a little less sloppy with language; set out proper reservations for MD and
platform types. MD and platform each get 15 and we leave an additional bit for
INTRNG usage in case, for instance, risc-v grows a fast interrupt type and it
begins to make sense to just support that as a common type rather than forcing
individual architectures to use up part of their allocation for it.

[andrew]

(Author set to andrew@)

How much of the current change is code I wrote?

sys/kern/subr_intr.c

358

powerof2(type)?

[kevans]

In D40161#953475, @andrew wrote:

(Author set to andrew@)

How much of the current change is code I wrote?

Hmm, arc didn't seem to update the commit message. Everything remaining from this set that you wrote was split off into the arm64 specific bits

[kevans]

sys/kern/subr_intr.c

358

D'oh; switched to KASSERT(powerof2(type), ... locally queued up to go along with any other changes requested.

[mmel]

The problem is that the FIQ tree may have a different topology than a normal interrupts. So instead of passing flags to the root handler, we should create a new root for the FIQ tree (or for any other type of interrupt), including a new intr_pic_claim_root_<foo>() (or we can convert intr_irq_root_dev and irq_root_* to arrays or so...).

It should be the interrupt driver's responsibility to register the interrupt source in the right parent/root. In addition, we could also use D35899, which would prevent the IPI from being hardwired to the root IRQ controller.

[kevans]

In D40161#953575, @mmel wrote:

The problem is that the FIQ tree may have a different topology than a normal interrupts. So instead of passing flags to the root handler, we should create a new root for the FIQ tree (or for any other type of interrupt), including a new intr_pic_claim_root_<foo>() (or we can convert intr_irq_root_dev and irq_root_* to arrays or so...).

I had a sort of vision for this, which I'll explain in a minute -- I note that the initial version of this did exactly that, but that was a little less palatable since only one of the three architectures currently using INTRNG will actually use FIQs (no analogous concept on RISC-V when I asked about it, and we're decommissioning it on 32-bit ARM since it's presumably not used there anyways).

My vision here was that extending this for different topologies is still relatively easy using what I think you were starting to allude to later on in your message -- turning irq_root_* into arrays and allowing multiple roots, perhaps with the caveat at first that each root can only support one interrupt type. It becomes valid, then, to call intr_pic_claim_root() multiple times as long as an existing filter doesn't claim the type that you're wanting to register.

None of this is implemented at the moment because my immediate concern, Apple Silicon, will simply be using the same root for both anyways.

It should be the interrupt driver's responsibility to register the interrupt source in the right parent/root. In addition, we could also use D35899, which would prevent the IPI from being hardwired to the root IRQ controller.

I think that the above doesn't shift that responsibility any more or less in one direction?

[mmel]

I see. So if I assume that the full implementation should have multiple roots (per type). My main objection is about using bitfield for interrupt type. Is misleading, not much compatible with multiple root implementation and it allows only single bit value almost everywhere.

The only place it allows multibit value is intr_pic_claim_root_type(). Not in all other usages. You cannot allow multiple bits to be set in intr_irq_handler_type() in any case. Inside the handler, you must only handle interrupt sources that are associated with the actual signaled interrupt line (to the CPU complex). For example, you cannot handle IRQs inside the FIQ interrupt service -> you cannot know if another interrupt type is enabled, asserting and then clearing another interrupt line before its service is called leads to a stray interrupt.

IMHO, we should change the interrupt type from bitfield to standard enumeration.

irq_root_type_filter, irq_root_typemask and the dance around intr_pic_claim_root() are victims of incomplete implementation. The interrupt driver(GIC) should register its own unique root with its own arguments for each interrupt type. imho nothing else is needed.

So again, a minimal version with 3 interrupt roots for IRQ, FIQ and NMI ( it already exists in arm64 and riscv) roots leads to less changes and better code I think.

[kevans]

In D40161#953962, @mmel wrote:

I see. So if I assume that the full implementation should have multiple roots (per type). My main objection is about using bitfield for interrupt type. Is misleading, not much compatible with multiple root implementation and it allows only single bit value almost everywhere.

The only place it allows multibit value is intr_pic_claim_root_type(). Not in all other usages. You cannot allow multiple bits to be set in intr_irq_handler_type() in any case. Inside the handler, you must only handle interrupt sources that are associated with the actual signaled interrupt line (to the CPU complex). For example, you cannot handle IRQs inside the FIQ interrupt service -> you cannot know if another interrupt type is enabled, asserting and then clearing another interrupt line before its service is called leads to a stray interrupt.

IMHO, we should change the interrupt type from bitfield to standard enumeration.

I was trying to avoid enums because I didn't want to cement us into new KPI for other types when we can easily discriminate with an argument, and enums add room for error since they're a C construct.

irq_root_type_filter, irq_root_typemask and the dance around intr_pic_claim_root() are victims of incomplete implementation. The interrupt driver(GIC) should register its own unique root with its own arguments for each interrupt type. imho nothing else is needed.

So again, a minimal version with 3 interrupt roots for IRQ, FIQ and NMI ( it already exists in arm64 and riscv) roots leads to less changes and better code I think.

I'll have to think on it... this kind of needlessly complicates the one user I have for any of this, but I'm not sure I care that much. I really just want to make forward progress on this.

[mmel]

I certainly don't want to block your work. I just don't want to introduce new KPIs that are problematic ( at least for me) for further expansion , nothing else.

Since I'm not sure I could articulate my objections in more detail / cleaner , I tried to do a proof of concept. It's just compiled and booted, without KASSERT and other sanity checks. The modifications to md_machdep.c are only temporary until D35899 is committed. Intr_pic_init_secondary() needs a little more love, but so far it doesn't seem like a hard problem.
The change in gic_v3 is just a sample 'use case', I don't have time to try the actual implementation.

Please take a look at it:
https://github.com/strejda/freebsd/commit/396c0b26c33bf99bf660760d68c68c65215b6b3b
Do you think this concept meets your requirements?

Sorry to keep you...

[kevans]

In D40161#954119, @mmel wrote:

I certainly don't want to block your work. I just don't want to introduce new KPIs that are problematic ( at least for me) for further expansion , nothing else.

Since I'm not sure I could articulate my objections in more detail / cleaner , I tried to do a proof of concept. It's just compiled and booted, without KASSERT and other sanity checks. The modifications to md_machdep.c are only temporary until D35899 is committed. Intr_pic_init_secondary() needs a little more love, but so far it doesn't seem like a hard problem.
The change in gic_v3 is just a sample 'use case', I don't have time to try the actual implementation.

Please take a look at it:
https://github.com/strejda/freebsd/commit/396c0b26c33bf99bf660760d68c68c65215b6b3b
Do you think this concept meets your requirements?

On the surface this looks quite reasonable, I'll take a closer look tonight. Thanks!

Sorry to keep you...

Sorry for the apparent frustration in my last message; I'm happy enough that we're not at a complete standstill, and I do appreciate the time you're putting in to working with me here.

[ehem_freebsd_m5p.com]

This is a real problem with having both Phabricator and GitHub. This got copied to GitHub and now there is some discussion there. This was likely due to the long pause after September 7th, someone really wanted this feature.

To my view Phabricator has two advantages: It also works with Mercurial and Subversion; it is under FreeBSD's control.

With everything now in Git, Phabricator is suboptimal since it is limited to things all version control software has. Whereas GitHub is rather superior for Git.

Anyway alert this is now being discussed in multiple places.

[jrtc27]

People put up new versions of patches rather than commandeering an existing one regularly. Whether that new version is on GitHub or Phabricator is irrelevant. Please stop complaining all the time about GitHub and Phabricator, your views are well known and it just leads to a bunch of spam in everyone's inboxes and IRC clients.

[ehem_freebsd_m5p.com]

In D40161#954037, @kevans wrote:

I was trying to avoid enums because I didn't want to cement us into new KPI for other types when we can easily discriminate with an argument, and enums add room for error since they're a C construct.

Yet due to being a C construct, they work well in interfacing with C.

With an enum, the following could be used in sys/arm64/include/intr.h:

#define __INTR_TYPE_IRQ 0
#ifdef FIQ
#define __INTR_TYPE_FIQ 1
#endif

#ifdef LOCORE
enum intr_type {
        INTR_TYPE_IRQ = __INTR_TYPE_IRQ,
#ifdef FIQ
        INTR_TYPE_FIQ = __INTR_TYPE_FIQ,
#endif
#if defined(XENHVM) // || defined(OTHER_HYPERVISOR)
        INTR_TYPE_HYPERVISOR,
#endif
        INTR_TYPE_COUNT
};

Whereas in sys/riscv/include/intr.h you might have:

enum intr_type {
        INTR_TYPE_IRQ,
#ifdef SMP
        INTR_TYPE_IPI,
#endif
#if defined(XENHVM) // || defined(OTHER_HYPERVISOR)
        INTR_TYPE_HYPERVISOR,
#endif
        INTR_TYPE_COUNT
};

(then use INTR_TYPE_COUNT as the number of roots)

There wouldn't be any extra roots and adding more pseudo-roots would be simple. List me as very strongly in favor of enum right now (until something with similar features shows up).

I understand the desire to keep architecture closer together, but at this phase it seems better to let them run wild. Then when once we know the actual needs the common pieces could be merged.

sys/kern/subr_intr.c

343

enum might not work for intr_irq_handler(), but u_register_t seems a better choice.

sys/sys/intr.h

76–89

Why does INTRNG need to constrain the values like this? Using an enum would allow the architecture to use values appropriate to that architecture and simplify the interface.

[ehem_freebsd_m5p.com]

@jrtc27 my one concern was to alert everyone here that discussion has occurred elsewhere. I was thinking discussion might have ended here, but it is now clear it has not terminated. Now I know I need to be tracking both.

[jrtc27]

In D40161#1063583, @ehem_freebsd_m5p.com wrote:

@jrtc27 my one concern was to alert everyone here that discussion has occurred elsewhere. I was thinking discussion might have ended here, but it is now clear it has not terminated. Now I know I need to be tracking both.

And that's fine. A short message with the link is all that's needed. Not a rant about FreeBSD's review systems alongside it.

[ehem_freebsd_m5p.com]

sys/kern/subr_intr.c

1620–1627

As mentioned previously, intr_pic_init_secondary() needs an update. Likely want to pass the root identifier in case a root handles multiple types.

[ehem_freebsd_m5p.com]

I suspect this needs to now be rebased onto main. I've got concerns, so I would like to take a look at what comes when the interaction with the FIQ stuff comes in. Simply asking looking for a temporary pause.

[kevans]

In D40161#1065823, @ehem_freebsd_m5p.com wrote:

I suspect this needs to now be rebased onto main. I've got concerns, so I would like to take a look at what comes when the interaction with the FIQ stuff comes in. Simply asking looking for a temporary pause.

I'm really not interested in more bikeshedding. Based on what's landed, the arm64 part of this will be pretty straightforward.

[ehem_freebsd_m5p.com]

I was simply looking for a pause, so now removing the pause.