Page MenuHomeFreeBSD

xen/intr: switch to passing initial parameters as temporary isrc
Needs ReviewPublic

Authored by ehem_freebsd_m5p.com on Jul 15 2021, 5:04 AM.

Details

Reviewers
royger
mhorne
Summary

Rather than joining the paramters together inside xen_intr_bind_isrc(),
there is some sense in using a temporary on-stack structure. While low
value at this point, this aids later commits.

Diff Detail

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

Event Timeline

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.

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

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

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.

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.

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.

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

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.

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

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.

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.

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

Ensuring Phabricator stays up to date with my tree.

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.

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.

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