Page MenuHomeFreeBSD

intr/x86: remove ->pic_vector() from x86 interrupt framework
Needs ReviewPublic

Authored by ehem_freebsd_m5p.com on Jun 5 2022, 7:15 PM.
Tags
None
Referenced Files
Unknown Object (File)
Feb 13 2024, 7:34 PM
Unknown Object (File)
Dec 19 2023, 11:51 PM
Unknown Object (File)
Dec 17 2023, 8:05 AM
Unknown Object (File)
Dec 10 2023, 6:50 PM
Unknown Object (File)
Sep 10 2023, 4:57 AM
Unknown Object (File)
Aug 14 2023, 10:40 AM
Unknown Object (File)
Aug 11 2023, 11:48 PM
Unknown Object (File)
Jun 24 2023, 7:57 AM
Subscribers

Details

Reviewers
kib
markj
jhb
Summary

Retrieving the interrupt number from isrcs via a ->pic_vector() function
made sense when the current x86 interrupt framework was implemented.
Now having ->pic_vector() serves to unnecessarily increase complexity,
rather than adding useful functionality as all callers have the valua
readily available.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 45856
Build 42744: arc lint + arc unit

Event Timeline

This revision is now accepted and ready to land.Jun 6 2022, 1:01 PM

Taking a look due while considering something else. I'm wondering about the argument order for intr_execute_handlers(). Should the vector argument be last instead of first? The thought is to mimic the argument order of intr_event_handle(). Issue is whether this reduces confusion or increases confusion?

sys/x86/include/intr_machdep.h
156

I would add a new field to struct intsrc in order to store the passed vector in intr_register_source(), so that the caller doesn't need to pass it again in intr_execute_handlers().

Having considered things in various places I'm no longer so sure of this being a good idea. There is merit, but due to a certain case I could see this being reused for major gain. As such marking this as on hold.

Having examined things more, an appropriate place to store the value already exists, but has been underutilized. Mainly isrc->is_event->ie_irq.

I had been pushing trying to remove the interrupt number from intr_event (D32504) since it doesn't work for all architectures. Thing is, it doesn't work for the INTRNG architectures because INTRNG is lazy about allocating the event structure. If INTRNG followed PowerPC and x86, and allocated the event structure during interrupt registration (D40166), I would be in favor of using it for all platforms.

Problem is I'm pretty sure someone (mmel) would be against this since it would conflict with "INTR_SOLO". Issue there is "INTR_SOLO" is stillborn (D35607), yet mmel still wants to keep this future-less blob.

So a decision needs to be made, push the interrupt number towards a common location (D40166) or towards the architecture (D32504). Right now I would tend to favor D40166, but in order to make progress a decision is needed and then potentially pushed over objections.

(note if the D40166 approach wins, then D32504 still has the value of removing the redundant interrupt number lookup)

ehem_freebsd_m5p.com marked an inline comment as done.

Change the approach and use ->is_event->ie_irq for places besides intr_register_source(). Since D35386 is now a parent, the vector value can disappear from xen_arch_isrc_t. msi_irq can also disappear, but that is more involved.