intr/x86: merge pic_{dis,en}able_source() call into pic_{dis,en}able_intr()
Needs ReviewPublic
Actions

Authored by ehem_freebsd_m5p.com on Nov 25 2024, 8:28 PM.

Details

Reviewers

royger
mav
jhibbits

Summary

When pic_{dis,en}able_intr() are called, they are paired with
pic_{dis,en}able_source() calls. Let the PIC implementations handle
calling of the second function.

Significantly, the pic_disable_source() calls paired with the
pic_disable_intr() calls are the only time PIC_NO_EOI was used. This
means the enum can be removed and pic_disable_source()'s prototype can
be simplified. Crucially, this means pic_disable_source() matches the
(*pre_ithread)() function of intr_event_create().

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 60769
Build 57653: arc lint + arc unit

Event Timeline

ehem_freebsd_m5p.com created this revision.Nov 25 2024, 8:28 PM

Herald added a subscriber: imp. · View Herald TranscriptNov 25 2024, 8:28 PM

ehem_freebsd_m5p.com requested review of this revision.Nov 25 2024, 8:28 PM

Harbormaster completed remote builds in B60769: Diff 146932.Nov 25 2024, 8:28 PM

Mostly looking for a review from @royger I'm unsure whether this additional call really needs to be retained, versus being unnecessary.

For for information on where this is headed: https://github.com/freebsd/freebsd-src/pull/1457

sys/x86/xen/xen_arch_intr.c
191	I'm unsure of this. `intr_remove_handler()` would previously end up calling `xen_intr_pic_disable_source()` after the `xen_intr_pic_disable_intr()` call, but I'm unsure whether this is actually needed. Does the source really need to be masked before the port is?

Note to anyone else who happens to run across this. Indeed, (mask_fn)isrc->is_pic->pic_disable_source could be substituted for the intr_disable_srcargument to intr_event_create() inside intr_register_source(). Issue is after this commit, #1457 first modifies intr_disable_src and then later completely removes it. At the point where this is the wrapper still needs to be retained.

Then when looking for something else discover sys/x86/isa/clock.c has the call i8254_intsrc->is_pic->pic_enable_intr(i8254_intsrc); without the ->pic_enable_source(). I lack hardware to test this, so I'm unsure of the effect.

Seems the issue in sys/x86/isa/clock.c originates from rG:875b8844bec. I'm unsure how to work around the issue, rG:49ed68bbf3c makes the situation worse.

Is my understanding is correct the ISA clock is always paired with the AT PIC? If so then it is notable the AT PIC pic_enable_intr() function is a NOP before D47745. In which case the call to ->pic_enable_intr() in attimer_attach() can simply be removed. Then the call to ->pic_enable_source() is the only operational one and it handles everything.

@mav any insight into the effect of rG:875b8844bec, rG:49ed68bbf3c, and D47745?

Updating, point to possible solution for what I need @mav to review (remove ->pic_enable_intr() call from clock.c/875b8844bec hack).

Harbormaster completed remote builds in B60937: Diff 147427.Dec 3 2024, 9:04 PM

ehem_freebsd_m5p.com added inline comments.Dec 4 2024, 1:56 AM

sys/x86/isa/clock.c
608–609 ↗	(On Diff #147427)	@mav is the one to look at this (since @mav created it several years ago). I note if the ISA clock is paired with the AT PIC, the AT PIC's `atpic_enable_intr()` function is a NOP. My concern is, was the ISA clock ever paired with a different PIC driver? I do note most `->pic_enable_source()` implementations do a superset of what `->pic_enable_intr()` does (not just the AT PIC). In which case removing this call and only doing `->pic_enable_source()` will function correctly anyway.

mav added inline comments.Dec 4 2024, 11:44 AM

sys/x86/isa/clock.c
608–609 ↗	(On Diff #147427)	14 years is a bit more than several. ;) I am not sure I can review this without much deeper look, other than sharing some memories. What I can recall is that this timer is 16-bit and has very short wraparound time. It means to use it as a timecounter we must track its interrupts to handle overflows. On the other side we might or might not need the timer interrupts themselves to do anything, since we might have more functional eventtimers to run `hardclock()`. You may see `clkintr_pending = 1` hack in `intr_execute_handlers()`, which I think might explain these manipulations with enabling interrupts vs sources -- we wants interrupts enabled, but we want them as cheap and invisible as possible unless we really need them to play as eventtimer. I don't remember this driver worked with different interrupt controller. Mentioned HPET can just steal the IRQ0 from this timer, but after that this driver can not function and should be disabled, so they don't really interoperate. There was also such thing as PC98 platform at that time that had its own oddities that could requite some hacks, but it was almost dead even back then and I don't remember much now.

ehem_freebsd_m5p.com abandoned this revision.Dec 13 2024, 3:27 AM

This is part of a fascinating end point I reached in combination with other commits. Specifically, Github #1457. I think this is better to review via GitHub, since the full git support allows viewing as part of the full series. Phabicator isn't all that well suited to the role.

This has a problematic interaction with D51623. It could be possible to instead move the EOI flag on ->pic_disable_intr() which keeps the performance benefit.

ehem_freebsd_m5p.com added a reviewer: jhibbits.Oct 28 2025, 12:24 AM

I have a question for @jhibbits/@kib or whomever might know.

Merging pic_enable_intr/pic_enable_source has mild value. Originally I had noticed the calls next to each other and thought this was cleanup (build AMD64 non-LINT, the ISA clock driver doesn't get built and they look ripe for merge). Yet removing the ISA clock driver isn't a high priority for me.

Merging the pic_disable_source call into the pic_disable_intr call is quite valuable. In particular this causes pic_disable_source's signature to become rather similar to intr_disable_src()/pre_ithread.

Problem now is D51623 interacts with this. D47745 was created prior to D51623, where the single call to pic_disable_intr was paired with pic_disable_source with PIC_NO_EOI. While D51623 added a call to pic_disable_intr paired with pic_disable_source with PIC_EOI.

I've two solutions for this conflict. First, PIC_EOI/PIC_NO_EOI could be moved to the pic_disable_intr call (visible as
0791fde65f5). Second using PIC_NO_EOI during intr_remove_handler() appears to be a low-value optimization and I wonder weather it is appropriate to do EOI during intr_remove_handler() (visible as a48ac909b1a).

I strongly prefer the second if it won't break anything. In particular this would cause the functionality and signature of x86's pic_disable_intr to be identical to INTRNG's pic_disable_intr.