Skip to content

Comments

x86 PICs as devices#1457

Open
ehem wants to merge 166 commits intofreebsd:mainfrom
ehem:x86picdev
Open

x86 PICs as devices#1457
ehem wants to merge 166 commits intofreebsd:mainfrom
ehem:x86picdev

Conversation

@ehem
Copy link
Contributor

@ehem ehem commented Oct 9, 2024

A major reason behind non-x86 PICs being devices is to utilize the module and newbus infrastructure. This does make x86 the odd one out, so turning the x86 PICs into full devices will likely aid converging the interrupt systems.

Having PICs for all architectures be proper devices allows the interrupt event core use kobj methods for interrupt callbacks. This shrinks the interrupt event data structures as everything can be done via a single pointer, rather than a pointer for each callback.

If interrupt events could be embedded inside the architecture interrupt structures, struct intr_event could be further shrunk by removing another pointer. The result would make the architecture core <=> event core relationship similar to the PIC <=> architecture core relationship.

Note there aren't nearly as many commits as the raw count suggests. With some massaging, the x86 PICs => devices and interrupt event => kobj methods steps can be reordered. I've got both visible here since both work. Likely around half of the commits will disappear in the end as they're duplicates of each other.

@ehem ehem force-pushed the x86picdev branch 6 times, most recently from de5428d to 7ae23df Compare October 14, 2024 20:19
@ehem ehem closed this Oct 14, 2024
@ehem ehem deleted the x86picdev branch October 14, 2024 20:19
@ehem ehem restored the x86picdev branch October 14, 2024 20:19
@ehem ehem reopened this Oct 14, 2024
@ehem ehem marked this pull request as ready for review October 14, 2024 20:31
@ehem ehem requested review from bsdimp and bsdjhb as code owners October 14, 2024 20:31
@ehem
Copy link
Contributor Author

ehem commented Oct 14, 2024

This has ended up going rather further than I had expected before terminating.

The old Marvell GPIO driver has needed a severe update for some time. It has become so rotten I can no longer work around the problems.

The x86 local-APIC and IO-APIC drivers have been tested. The AT PIC driver may work, but I haven't yet gotten a good recipe for qemu. The impact on x86/Xen and x86/MSI drivers is small enough for them to likely still be working, but they're not yet tested.

@ehem
Copy link
Contributor Author

ehem commented Oct 16, 2024

This is certainly going to need some adjustment, but I need advice as to which direction to go with this. There are 3 places I see where this series could be reasonably terminated.

First, it could be terminated after converting the x86 PICs to devices. This was the main goal. This fails to pick up most of the benefits, but does make the interrupt infrastructures consistent in PICs being devices.

Second, it could terminate after the conversion to int intr_event_create_device(struct intr_event **event, device_t pic, interrupt_t *source, u_int irq, int flags, const char *fmt, ...). This shrinks the structures for hardware interrupts by 24 bytes (3 fewer pointers) and enlarges software interrupts by 16 bytes. Many systems will have >100 hardware interrupts and with 3-7 software interrupts being the maximum, this is a notable benefit. This requires conversion to #include <machine/interrupt.h> in many places.

Third, it could terminate after converting to int intr_event_init(struct intr_event *ie, device_t pic, u_int irq, int flags, const char *fmt, ...). I feel having the PIC <=> arch core interface being similar to the arch core <=> shared interface is a good thing. I've found it rather bizarre how the PIC structure wraps the architecture core, but the architecture core then has a pointer to the shared core. Not having memory allocation in intr_event_init() allows the architectures to initialize their intr_event structures sooner as they can be setup while the appropriate mutex is held. This also reduces memory usage for hardware interrupts by 48 bytes and enlarges software interrupts by 8 bytes.

Overall this seems a rather good idea. Yet now I'm waiting on reviewers...

@bsdimp
Copy link
Member

bsdimp commented Oct 22, 2024

This change is too large. It has a lot of gratuitous changes that aren't strictly necessary. 49 changes and 244 files is simply too much to review. A quick survey of the changes shows lots of header file renames (which aren't needed) and kobj function name renaming (also need needed). In it's current form, it's not possible to review this.

@ehem
Copy link
Contributor Author

ehem commented Oct 24, 2024

That is what happens if you look at a squash of 49 commits. Take almost any sequence of 49 commits in FreeBSD's repository, look at the delta and it will be confusing. There is also some infrastructure which has been waiting 2.5 years and had that been brought in in a timely fashion it wouldn't need to be included here.

For some overview of the earlier ones, 5a4854b ended up getting in via other means. It is here as I was expecting it to be brought in with this, but then unexpectedly it came in via other means.

The next 3 are 2 bits of cleanup and one thing I've got an unpleasant suspicion will explode in the not too distant future.

Commit #5 is a key commit. Without that the approach is impossible. Notably x86 presently initializes PICs at SI_SUB_INTR while newbus was doing all initialization at SI_SUB_DRIVERS. Initializing the root bus earlier works fine, but I'm unsure whether that will be accepted.

Advice on the question asked immediately above might allow abandoning some pieces. Might be clearer to point to the primary commit for intr_event_create_device(). The point is the arguments to intr_event_create() are rarely independent. For any given PIC, the functions will always be the same. As such if the interface moves to intr_event there is substantial savings on hardware interrupts since only a single pointer is used.

There isn't a good commit to point to for intr_event_init(), though perhaps f2abf73 can give some idea. I've long been bothered by how INTRNG and x86 PICs wrap their data around the architecture interrupt type, then cast back and forth. Whereas both INTRNG and x86 include a struct intr_event *<prefix>_event; inside their respective structures.

Why is one boundary embedding the structure inside the other, while the other is handled via pointer? These two relationships are similar, so shouldn't their interfaces be similar? Embedding the structure results in 16 bytes of savings since two pointers are removed (pointer to intr_event and there is no longer any need for ->ie_source).

@ehem ehem force-pushed the x86picdev branch 3 times, most recently from c233398 to b7da32e Compare October 31, 2024 05:13
@ehem ehem force-pushed the x86picdev branch 2 times, most recently from ac8444c to 8398309 Compare November 1, 2024 17:48
ehem added 29 commits February 20, 2026 17:57
As these are now formal devices, use the appropriate printf() flavor.
device_printf() takes care of all instances outside ioapic_create(), so
there is no longer any need to explicitly store this value.
Attempt to further constain use of APIC-Ids to the APIC core, rather
than spreading them everywhere.  Should allow further convergence of the
interrupt infrastructures.
…c_id

Using APIC identifiers in this interface drags APIC-specific detail into
the x86 interrupt interface.  Changing this portion of the interrupt
interface to instead use cpu_ids furthers convergence.
mv_gpio_setup_intrhandler() was removed from mvvar.h at ad2be10.
The function and others removed from the header were in fact last used
before db5ef4f.  Leave these all marked static as future fixes would
likely make them so.

An effect is ->gpio_events[] is never non-NULL.  The means
mv_gpio_intr_handler() is always a NOP.  This isn't entirely bad as both
functions were blatantly violating INTRNG's interface.

In light of how broken the interrupt portion of the file presently is,
add #ifdef MV_GPIO_INTERRUPTS_BROKEN around all relevant functions to
disable them.  These are presently too broken to do anything else.
This allows the interrupt framework to introduce hooks into device
function tables.  Some functionality may be common and the framework
may provide standard functionality.  Certain PIC tasks may also go
through the function tables.
isrc_alloc_irq() seems to implement 2 tasks.  First being to choose an
interrupt number, second setup the interrupt table entry.  Splitting
these seems to make things a bit cleaner.
The intr_event structure is a baseline for what all architectures
support.  This isn't truly separate from the infrastructures and is
always allocated.  As such merging seems best.

Adjust allocation and release as needed for full functionality.
…ndling

As the interrupt event interface is friendly to all INTRNG drivers,
switch to that.  Remove the INTRNG version of the interface as it is now
redundant.  Yet to be introduced drivers are likely to need updates.

Leave base class functions behind for errors with debug kernels.
Now that the value is on the event, there is no need for INTRNG to
to duplicate the value on the intr_irqsrc structure.
Always having ->isrc_event means .ie_name fulfills the purpose of
->isrc_name.  Therefore switch to the standard location.

While touching `isrc_update_name()` update the sizes to use sizeof()
instead of the fixed known size.
Always allocating `intr_event` means we have a better place to store
the interrupt number.  Implement using this in all cases.
isrc_free_irq() is called from error and release paths which do not want
to know about issues with the table.  They may panic() due to inability
to recover from double-errors.  They may leak the isrc due to
consequences of EBUSY from intr_event_destroy().  They may provide a
less precise printf().  Since isrc_free_irq() can very easily recover
from these conditions, do so and there is no longer any need for a
return.

Report as this should not occur.

Differential Revision: https://reviews.freebsd.org/D38599
Previously `isrc_release_counters()` hadn't been invalidating the
fields when releasing the counters.  Now set them as invalid and
warn if this inconsistent situation occurs.
INTRNG makes absolutely no guarantees about the "irq" value.  Anything
depending on the value is almost certainly broken.  At best it might
work by luck, but relying on the present values is unsafe.

Modifying the value of the PIC pointer is suspicious.  While this
doesn't suggest breakage it is quite suspicious.  Likely these are
remnants of the past and no longer needed,
This allows the interrupt framework to introduce hooks into device
function tables.  Some functionality may be common and the framework
may provide standard functionality.  Certain PIC tasks may also go
through the function tables.
Create an IPI device for handling IPI interrupts.  While modifying the
internals of an interrupt event is concerning, this does avoid the
present troubles from 0c50edf.  ->ie_pic may well replace the
architecture counterparts on machine interrupt source structures, at
which point the value would be non-removable anyway.
All of the interrupt infrastructures want this information.  As commonly
use information, add a common query function.  Hopefully this also
reduces instances of looking inside interrupt events and discourages
infrastructures from duplicating the information.
…tructures

The intr_event structure is a baseline for what all architectures
support.  This isn't truly separate from the infrastructures and is
always allocated.  As such merging seems best.

As `intr_event_init()` does not sleep, it can be called while locks are
held.  This allows cleanup of `intr_lookup()` as the event can
be initialized while the lock is held.  The removes the lazy event
allocation, pushing architectures closer together.
Now that the value is on the event, there is no need for PowerPC to
duplicate the value on the powerpc_intr structure.
This finishes removing OpenPIC functions from the global namespace.
Instead PICs based on OpenPIC should use `openpic_class` for explicitly
calling OpenPIC functions.
The newer macros allow marking classes with a parent private (static).
Minor cleanup as OpenPIC class creation got done out of order.
This matches other interrupt infrastructures.  This potentially allows
non-hardware PICs to allocate only the lower-level structure, instead
of the full HW structure.
These reduce the number of kernel object methods used when handling
interrupts.  While reasonably fast they are slower than indirect
function calls.  Having these on the drivers potentially allows the
compiler to inline the functions.  More significantly, this distinctly
reduces the odds of collisions in the object method cache.
The event compatibility implementation is targeted for compatibility
for source not converted to devices.  Notably architectures being
maintained outside the main tree.  SWIs can have lower overhead by
removing the full compatibility.
Each architecture can convert to kobj interrupt interface independently.
This is the final result of all architectures fully implementing use of
the kobj interrupt interface.
As all FreeBSD architectures provide a PIC base class, move the
declaration to the common header.
…rrupt.h>

Referencing anything other than the fully compatible header is risky.
Many of these will be expected to break due to lacking later
compatibility adjustments.  Avoid direct #include of headers besides
<machine/interrupt.h>.

Add #include <sys/types.h> to <sys/interrupt.h>.  All current
architectures need this header, this is very likely to apply to any
architecture added to FreeBSD.

Differential Revision: https://reviews.freebsd.org/D35559
Some headers include poisons to ensure they're not used in improper
situations.  Add a LINTING_HEADERS #define to allow the header test to
check these poisoned headers.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants