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

work towards unifying interrupt interfaces #1286

Draft
wants to merge 22 commits into
base: main
Choose a base branch
from
Draft

Conversation

ehem
Copy link
Contributor

@ehem ehem commented Jun 12, 2024

This is some work towards unifying the architectural interrupt interfaces. Simply tries to identify an identical portion and take advantage of what having a common interface provides.

The interrupt core "irq" is meant to be an index into a dense table of
interrupts.  This better maps to the PowerPC interrupt vector, than the
PowerPC interrupt identifier.  As such switch to ->vector for the
intr_event_create() call.

Fixes: 9b33b15 ("Add the interrupt vector number to intr_event_create")
@ehem
Copy link
Contributor Author

ehem commented Jun 12, 2024

This is marked "draft" for a reason.

The first issue is the PowerPC team needs to okay 03da9e8. I'm pretty sure it is correct, but it hasn't been verified. I'm taking a look at @chmeeedalf's suggested emulation environment, but haven't gotten it working yet.

The second issue is machine/intr_machdep.h versus machine/intr.h. The former was the traditional name, but ARM/INTRNG chose the latter. Unfortunately this now serves to inhibit further attempts at unification.

The last 4 commits (b9522c7..78ef9cb) are special. Most of other interfaces are tending towards using pointers to the appropriate structures. As such I feel using struct intr_event * is better than int. I also have some dislike of #include <machine/intr_machdep.h> into sys/kern/kern_intr.c. Yet others want to stay with the integer. I'm okay with b9522c7 being dropped, but I'm including it as I prefer that interface.

I've looked into the possibility of removing the lazy allocation of struct intr_event from INTRNG and PowerPC. What those need is to set ->ie_irq later than x86 does. In light of this ab0fd36 is meant to modify the interface to match what is actually needed.

03ab13d..03ab13d are simply fallout associated with this. I can't help thinking 9b33b15 was really a WIP. It took a very small step forward, but the process was never finished. Originally D32504 was closer to a revert of 9b33b15, but it is actually reasonable to leave ->ie_irq behind.

@ehem
Copy link
Contributor Author

ehem commented Jun 12, 2024

I now have a better feel for the complaints about Hyper-V for arm64.

ehem added 10 commits June 12, 2024 16:09
While the INTRNG framework aspires to becomming the common FreeBSD
interrupt framework, right now it is not.  The "machine/intr.h" headers
were exactly as machine-specific as the "machine/intr_machdep.h"
headers.  Rename in the hope of increasing commonality.

Differential Revision: https://reviews.freebsd.org/D35559
For code which doesn't care about the underlying structure and simply
needs to have a common name.

Differential Revision: https://reviews.freebsd.org/D39178
Interrupt numbering belongs to the interrupt controller, not the core
event code.  As such, this layer should be providing a function to
resolve the interrupt number to the source/event.

Differential Revision: https://reviews.freebsd.org/D39178
Most other systems have this via INTRNG or other means.  Now add the
functionality to PowerPC.

Differential Revision: https://reviews.freebsd.org/D39178
Different name for the main function, but then a simple macro on top.

Differential Revision: https://reviews.freebsd.org/D39178
A standardized interface for the architecture interrupt lookup functions
is now available.  As such this is now a better approach for
intr_{get|set}affinity() and _intr_drain().  At worst the architecture
functions are faster, often they are much faster.

Differential Revision: https://reviews.freebsd.org/D32504
A standardized interface for the architecture interrupt lookup functions
is now available.  As such this is now a better approach for
intr_{get|set}affinity() and _intr_drain().  At worst the architecture
functions are faster, often they are much faster.

Differential Revision: https://reviews.freebsd.org/D32504
Having the "irq" as an argument to intr_event_create() requires knowing
the value before calling.  This matches the approach of traditional x86
interrupts where the value was always known early.  Other architectures
and designs may not know the value until later.  As such it is better
not to be an argument and simply be set later.

Differential Revision: https://reviews.freebsd.org/D32504
Retrieving the interrupt number from isrcs via a ->pic_vector() function
made sense when the current x86 interrupt framework was implemented.
Since the kernel event structure has ->ie_irq readily available,
->pic_vector() now serves to increase complexity.  All callers know
the vector at time of interrupt registration.

Remove vector from xen_arch_isrc_t as that is now completely unused.

Differential Revision: https://reviews.freebsd.org/D35406
Since this is redundant with ->ie_irq, use the common location.
#include <arm64/include/intr.h>
#include <machine/intr_machdep.h>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow. Just wow. The qualities of this are just...

Alas, I'm quite unsure why the original author did things this way. On further consideration, perhaps this should simply be #include <arm64/include/intr_machdep.h> to fulfill the minimum. Let someone involved with this do the proper job.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comes from 6e5b082. Looks like they wanted ac29dda (or the inverse of intr_machdep.h => intr.h) as well. Alas the qualities of that implementation are poor.

@ehem
Copy link
Contributor Author

ehem commented Jun 13, 2024

Another note to reviewers. I think intrtab_lookup() is a better name for the function due to that being the minimum guarantee behind it. Many MIPS devices would have used typedef struct intr_event interrupt_t;, so the thing being returned is something interrupt and not necessarily something which could be called "source".

Hmm, I was planning to remove since I'm unsure. I'm not quite not 100% sure whether the intrtab_lookup() functions should be marked __pure. There are no side-effects and their returns tend to be stable, yet behind the scenes they are relying on global tables which can be modified by calling other functions.

ehem added 11 commits June 12, 2024 22:16
These are accessed so often the macros result in notable savings.  This
could also aid later changes to the basic implementation.

Fix two spots which should have been using the GIC_INTR_ISRC() macro.
These are accessed so often the macros result in notable savings.  This
could also aid later changes to the basic implementation.

Fix two spots which should have been using the GIC_INTR_ISRC() macro.
Work in progress.

Having a table of interrupts is common functionality among all the
interrupt subsystems.  Taking this and making it common code reduces the
delta between interrupt subsystems.

Presently the interface mostly looks okay, but do need details for
implementing that hash table.
Work in progress.

For devices which need to allocate ranges of interrupt numbers, this is
quite valuable.  Instead of the x86 mess of everyone playing with
num_io_irqs, devices can allocate ranges.
The common table allows reserving of blocks of interrupt numbers.  This
allows INTRNG PICs to reserve an interrupt range and specify numbering
within that.  PICs must first allocate ranges though.

In order to work with the common functionality, the core now allocates
starting from high interrupt numbers.  If additional numbers are needed
these grow down.  The expectation is many PICs will prefer this
interface (since some control of numbering is valuable).
Work in progress.

Switch to this alternative, which should work for all architectures.
The common interrupt table was designed with PowerPC's situation in
mind, as such it fits appropriately.  Enable the distinctly faster
implementation.
Rather than having this in architectural configuration, move to
common.
Since sys/intrtab.h depends on machine/intr_machdep.h, these files
should really be #including sys/intrtab.h, rather than
machine/intr_machdep.h.
Rather than allocating a huge array, now we can instead allocate
interrupt structures individually and spread them out.  Next step is to
do lazy allocation and only allocate interrupt structures for in-use
interrupts.
Rather than allocating a huge array, now we can instead allocate
interrupt structures individually and spread them out.  Next step is to
do lazy allocation and only allocate interrupt structures for in-use
interrupts.
@ehem
Copy link
Contributor Author

ehem commented Jun 15, 2024

This is roughly where I figure to end #1286. Everything up to 06660ff is pretty much acceptable. Everything after 06660ff is near-certain not to work. Problem is the documentation for rman doesn't really give me an appropriate feel for some of its workings. Ideally a userspace librman.so which could be used to explore its capabilities and interface would be really handy.

Getting a ruling on 83653a7 and the issue of machine/intr_machdep.h versus machine/intr.h sooner would be nice though. That really doesn't need to wait and is an albatross for any effort to unify the interrupt interfaces.

Even though this is a draft, comments are invited.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
1 participant