x86 PICs as devices #1457

ehem · 2024-10-09T01:10:29Z

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

ehem · 2024-10-14T20:36:43Z

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 · 2024-10-16T23:17:21Z

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 · 2024-10-22T22:41:34Z

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 · 2024-10-24T00:49:24Z

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

sys/kern/subr_bus.c

sys/sys/bus.h

sys/kern/subr_bus.c

bsdimp · 2024-11-02T14:59:59Z

sys/kern/subr_bus.c

@@ -5108,7 +5109,6 @@ root_bus_module_handler(module_t mod, int what, void* arg)
 {
 	switch (what) {
 	case MOD_LOAD:
-		TAILQ_INIT(&bus_data_devices);


This change doesn't move the needle enough to take at this time. bus_data_devices is the first thing that does this, so that makes this a little inconsistent. It's fine how it is, and there's not a compelling reason to do this. Please drop this change from this commit series.

I had been considering squashing it into the move to => SI_SUB_INTR. Mainly I had looking at what could be pulled out of root_bus_module_handler().

sys/kern/subr_bus.c

As all architectures now use devices for PICs, it now makes sense to adapt kernel events to match. Simply replace the handler functions with a device_t, which then needs to include support for the interface. 3 ARM devices have been modifying the value of ->isrc_dev. Ensure these also modify ->ie_pic too.

In some cases it may be better to allocate the intr_event structure outside of kern_intr. Notably if an interrupt controller needed data attached to the event instead of the PIC. Differential Revision: https://reviews.freebsd.org/D35417

Callers of `intr_event_init()` are expected to convert to passing a structure surrounding the event structure. As a result the pointers alias and a pointer to the event can be converted back to the "source". This makes the relationship between machine interrupt core and interrupt events, similar to the relationship between PICs and machine interrupt core. As `intr_event_init()` does not sleep, it can be called while locks are held. This allows cleanup of `intr_register_source()` as the table pre-check is now worthless.

Due to now using devices, the x86 architectures can now use the new interface. Next is to start converting PICs to directly use hooks they're suited for. Adjust `intr_assign_cpu()`'s prototype to match new event interface. Using unsigned is likely better anyway, though it should be a while before that becomes an issue.

Rather than going through intr_machdep.c, it is better to go directly through kobj. This can be viewed as cleanup, but is also a mild performance improvement. Modify callers to use the INTR_EVENT_*() name, so the x86 equivalents can disappear.

…tures The intr_event structure is a baseline for what all architectures support. This isn't truly separate from the infrastructure and is always allocated. As such merging seems best. Since intr_event_init() can be called while locked, move the call inside the critical section. This also makes the preemptive table check worthless, so remove that too.

Since this is redundant with ->ie_irq, use the common location.

As all x86 PICs must set this value, it seems more consistent to do so inside intr_register_source(). Hopefully a trivial reduction in work.

x86 now passes a structure surrounding the event structure. As such, x86 can now disable the older interface and fully use inline events. This makes the relationship between machine interrupt core and interrupt events, similar to the relationship between PICs and machine interrupt core.

Now that the value is on the event, there is no need for x86 to duplicate the value on the intsrc structure.

These were needed for the struct->device transition. Now purge these as they are 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.

This reduces the memory consumed by the intr_event structures. The hooks associated with any given PIC don't vary.

…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. Cleanup a few uses of explicit "{ 0, 0 }" sentinal and switch to DEVMETHOD_END.

These were global, but now appear unused outside the file. As such now mark them static, mostly to document their current status.

These aren't flaged as unused due to `mv_gpio_setup_intrhandler()` being global. They don't appear to be used anywhere though, so completely disable them. I can only confirm `->gpio_events` being set by the above. As such the references to the variable would effectively be NOPs. I suspect this could be used by something outside of the FreeBSD tree, but with no references I'm unable to confirm this.

This driver was never fully adapted to work with INTRNG (current ARM interrupt core). Since the driver needs a full rework at this point, disconnect the driver from the FreeBSD kernel build. Hopefully this might be updated by someone, but it has become too problematic.

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.

INTRNG now passes a structure surrounding the event structure. As such INTRNG can now disable the older interface and fully use inline events. This makes the relationship between machine interrupt core and interrupt events, similar to the relationship between PICs and machine interrupt core.

Now that the value is on the event, there is no need for INTRNG to to duplicate the value on the intr_irqsrc structure.

Convert large numbers of instances of overt driver_t structures to using the DEFINE_CLASS_0() macro. Greater consistency should reduce future errors. Convert several uses of '{ 0, 0 }' driver method list ends to DEVMETHOD_END macro. Small amounts of similar cleanup to similar macros. The opal_sensor module had omitted the sentinal from the table. That had likely been functioning due to dumb luck for some time. Differential Revision: https://reviews.freebsd.org/D47546

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.

This reduces the memory consumed by the intr_event structures. The hooks associated with any given PIC don't vary.

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

PowerPC now passes a structure surrounding the event structure. As such PowerPC can now disable the older interface and fully use inline events. This makes the relationship between machine interrupt core and interrupt events, similar to the relationship between PICs and machine interrupt core.

Now that the value is on the event, there is no need for PowerPC to duplicate the value on the powerpc_intr structure.

As all FreeBSD architectures provide a PIC base class, move the declaration to the common header.

ehem · 2024-12-23T22:31:31Z

Another day, another bit of cleanup spotted and a goof with conversion spotted.

Now up to 6-ish cleanup commits at the start. Note the 5th and 6th still have those quirks leading into later commits. This is then followed by a 13.5 commit marathon of the actual conversion.

Then followed by 3 more refinements, then a huge aftershock.

ehem force-pushed the x86picdev branch 6 times, most recently from de5428d to 7ae23df Compare October 14, 2024 20:19

ehem closed this Oct 14, 2024

ehem deleted the x86picdev branch October 14, 2024 20:19

ehem restored the x86picdev branch October 14, 2024 20:19

ehem reopened this Oct 14, 2024

ehem marked this pull request as ready for review October 14, 2024 20:31

ehem requested review from bsdimp and bsdjhb as code owners October 14, 2024 20:31

ehem force-pushed the x86picdev branch 3 times, most recently from c233398 to b7da32e Compare October 31, 2024 05:13