CC @karels

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.

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.

Maybe the current code should just be moved under arm64.

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.

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.

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

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.

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?

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.

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.

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

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.

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.

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.

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.

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

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.

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.