Page MenuHomeFreeBSD

xen/intr: rework xen_intr_resume() to use port mappings as implicit listing
Needs ReviewPublic

Authored by ehem_freebsd_m5p.com on Jun 1 2021, 3:44 PM.

Details

Summary

The prior implementation of xen_intr_resume() was (ab)using the x86
interrupt list as a table of in-use interrupts. Rework this to instead
use the xen_intr_port_to_isrc array for this purpose. This removes one
more x86-ism from xen_intr.c, a major benefit.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 40795
Build 37684: arc lint + arc unit

Event Timeline

This is the main goal of D30598/D30599. The variables xen_intr_auto_vector_count and first_evtchn_irq are x86-specific. The call intr_lookup_source() is also x86-specific which is being utilized to get access to interrupt_sources and use that as a table of allocated event channel interrupts. These two make it so xen_intr_port_to_isrc is used for this purpose instead.

xen_intr_auto_vector and first_evtchn_irq are attempting to head back into sys/x86/xen land, while xen_intr_port_to_isrc is definitely going to remain in sys/xen/xen_intr.c. Since the functionality of xen_intr_resume() requires access to xen_intr_port_to_isrc, removing references to xen_intr_auto_vector and first_evtchn_irq pushes the effort to get those out of sys/xen/xen_intr.c forward.

Actually, one question for the FreeBSD/Xen maintainer: Is the order of the xen_intr_port_to_isrc[cur->xi_port] = cur; and evtchn_unmask_port(cur->xi_port); lines important?

I suspect it is since the moment the event channel port is unmasked, events can occur. As such I moved the second call out of xen_intr_rebind_isrc(), but if it is acceptable to have those in the opposite order then having that remain in the function would be nice. I was guessing this would be a race condition and therefore opted for the safe choice.

Add a safety check. Protect against left-behind entries in xen_intr_port_to_isrc[] (shouldn't happen, but just in case).

Comment adjustment, adjustment of tests (consistently use xi_port >= NR_EVENT_CHANNELS).

The key point behind D30598/D30599 is xen_intr_port_to_isrc[] is part of the xen_intr.c file, whereas intr_lookup_source is using the x86-only interrupt_sources[] array for the purpose. As such this removes another x86-ism from the file. I could see these two being squashed in the end.

Tiny update, removing a single line. This line is redundant as the value gets wiped earlier.

Simply updating to match D30598 (just enough delta to confuse patch/git).

Due to this code, one could argue for adding a evtchn_port_t max_port;. The purpose being to keep track of the maximum used port. Before this commit xen_intr_auto_vector_count was being abused for this. Problem of course is at D30936 that variable becomes unavailable and this file needs a separate tracking variable.

Adjust condition to cur->xi_port == isrc_idx. This skips over correct entries, instead of ones with valid xi_port. This potentially steers more cases into the KASSERT().

Small adjustment. This makes the delta smaller, keeping more line in original order. This may simplify retesting. Additionally it is slightly shorter.

Updating patch to ensure successful application with updated D30598.