xen/intr: move isrc allocation out of xen_intr_bind_isrc()

[ehem_freebsd_m5p.com]

Summary

This allows the calling function to merge several of
xen_intr_bind_isrc()'s arguments together. Instead of having 6
arguments, it can instead merely have 3. This distinctly simplifies
xen_intr_bind_isrc() calling.

[ehem_freebsd_m5p.com]

This is what I was suggesting in D30936. Doesn't add much by itself, but seems plausible as a potential API. This has a distinct advantage for aarch64 as it could use "xen-virq%u", isrc->xi_virq for the intr_event_create() call in case of VIRQ. This seems better for VIRQs since they could get reassigned to different ports.

[ehem_freebsd_m5p.com]

This removes one hunk which leaked in from a potential later commit.

[ehem_freebsd_m5p.com]

Small update arising from a quick look. This increases consistency a bit, hopefully aiding pieces being merged at a later point.

[ehem_freebsd_m5p.com]

I distinctly like this approach in some ways. Makes the xen_intr_alloc_isrc() prototype simpler and ensures everything potentially needed gets in. This could also simplify a number of error cases as it becomes valid to use xen_intr_release_isrc() for cleanup sooner. Downside I see right now is the temporary struct xenisrc could use a substantial amount of stack.

[ehem_freebsd_m5p.com]

Update due to examining elsewhere and noticing a crucial missing piece. I really do find this approach elegant and I suspect it could simplify things. Do need to choose the approach for the issue.

[ehem_freebsd_m5p.com]

Reverting to previous arrangement. The extra bits didn't seem to go with this delta that well. Trying to grab the full additions would have been too confusing. Do include the reflowing though.

[ehem_freebsd_m5p.com]

Updating to keep synchronized with my local repository. Minimal delta, simply a bit of formatting adjustment.

[ehem_freebsd_m5p.com]

In case comments elsewhere didn't make it obvious, I really like this interface. Yet since it isn't required I continue to keep options open. Getting a decision sooner is highly valuable since this choice effects many follow-on commits substantially.

[ehem_freebsd_m5p.com]

Minor adjustment. Slightly shorter, but also works better with D30935.

[ehem_freebsd_m5p.com]

Testing revealed unexpect behavior by Clang. Didn't like the _Static_assert() directly after the label. Looks like a Clang bug at first glance. Adding a bit of cleanup while at it.

[ehem_freebsd_m5p.com]

sys/xen/xen_intr.c

310–311 ↗

(On Diff #98342)

Clang gave an "expected expression" error at the _Static_assert() without the semicolon. This seems like a compiler bug, as it doesn't complain with the exact same CTASSERT() directly after a bracket. If someone has a better explanation I'd like to learn of it.

[ehem_freebsd_m5p.com]

Realizing the last revision ended up with a bug added. Adjust to take care of issue. I still like this approach, but I'm rather lacking feedback...

[ehem_freebsd_m5p.com]

Ensuring Phabricator stays up to date with my tree.

[mhorne]

I have looked through the series (D31188, D33622, D33623, D33624 and D31355), but what's missing is the "why" of all these changes. Does this refactoring enable something specific? IMO the new code is not obviously better than what was present before, just different.

Since none of them apply against main, they are not immediately actionable either.

[ehem_freebsd_m5p.com]

They may not directly on HEAD since this is a bit into the chain.

The Real question I've been aiming to get answered here is: What should the MI <-> MD interface for isrc allocation look like? D30936 has been tried and it works acceptably. Could this be improved? Likely, issue is what does a better interface look like?

What I've been trying to get feedback on for 6 months is, might a prototype of struct xenisrc *xen_arch_intr_alloc(const struct xenisrc *const params); be better? The advantage is this provides the MD side with more data for setting interrupt sources up.

Meanwhile the rest of the series is pretty much exploring what passing struct xenisrc *const params to xen_intr_bind_isrc() allows. The extras are merging functionality duplicated in several places together.

Many of xen_intr_bind_isrc()'s callers close the event channel in case of failure. The additional data passed in (xi_close) allows that to be handled as common code.

D32866 is yet more functionality with 3 duplicate implementations. Worse, they've drifted apart as at least one piece of functionality was lost at ca7af67ac958 (xen_intr_alloc_and_bind_ipi() used to call intr_event_bind()).

The reduction in lines of code is small, but should have some value.

[ehem_freebsd_m5p.com]

Updating to match present status. Some interesting refinement has been done.

[ehem_freebsd_m5p.com]

Updating to current tree status.

[ehem_freebsd_m5p.com]

The main issue here is, what should the allocation portion of the MI<->MD interface look like?

xen_intr_alloc_isrc() is where the interface needs to run through. intr_register_source() and intr_lookup_source() are innately architecture-dependent, whereas xen_intr_bind_isrc() is mostly clean of architecture features (some bits need wrappers, but those are simple).

The benefits include being able to share quite a bit more in xen_intr_bind_isrc(). The architecture then has more data for interfaces it needs to interface with. Notably struct xenisrc * provides a proper superset of the data needed for the original PVHv1 implementation.

I like the consequences of this interface, but I need an initial okay before I spend time to rebase everything on top of this.

[ehem_freebsd_m5p.com]

Updating. New approach to this situation which may be more likely to be accepted.