Single blank line adjustment, but keeping Phabricator synchronized.

Tiny update, this line seems appropriate here, but also elsewhere.

Tiny update, pulling an extra line out of the locked section.

Updating to ensure consistency with what I've got. Should still apply, but updates on D30743 have made application unclean.

Overall this approach seems likely to be simpler in the end. Notably if the approach proposed by D31188/D32866 was brought in, isrc->xi_cpu would always be set and the CPU assignment would be done here in all cases.

Updating due to fallout from D30743 (which I expect to go in first).

Fallout from updates to D30743. The update makes the original patch no longer apply cleanly. Since I expect D30743 to go in first, ensure it works.

As threatened, dropping the preference for isrc->xi_port. Mostly ends up in D30598 anyway.

Tiny delta, removes one extra line from the lock/unlock in xen_intr_bind_isrc(). (also keeps Phabricator synchronized)

There have been a bunch of updates to this due to experimentation.

To summarize the situation in a different way. xen_intr_handle_upcall() had been implemented as the assembly-language entry point (called from apic_vector.S). Both xenpci.c and xen-dt.c were creating wrappers around xen_intr_handle_upcall() in order to call it as a driver_filter_t as a device interrupt.

Keeping synchronized. Reduced the one comment. I'm now suspecting modifying td_intr_nesting_level is incorrect, but I need a reviewer for that as I'm insufficiently familiar.

Upon examination, it was possible to split this into 3 separate commits somewhat reasonably. I'm doubtful the commits are worthy of individual review, so I'm leaving Phabricator alone.

Keeping things synchronized. Looks like a few stray bits snuck in last time. Need those approvals so I'm not dealing with a huge pile of local commits...

Turning a number of the ARM64 compatibility functions into inlines.

Switching to inlines for the paper-thin compatibility layer bits. There is only the tiniest conversion between architecture-independent and architecture-dependent for these.

Updating for MIPS removal.

Ensuring Phabricator stays up to date with my tree.

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

Updating given updates elsewhere. This is the set isrc->xi_cpu approach.

D30726 doesn't depend on anything. As originally submitted D31995 wants D30726 first. The big dependency is D30936 depends on D30726.

Sigh, simple goof fix.

Identifying where the need for <machine/intr_machdep.h> actually disappears.

Updating to present status. Difficult keeping everything up to date when you've got >60 diffs and ~100 commits in the queue.

Trying to keep Phabricator synchronized locally. Bit of a problem with the number of patches I'm playing with...

Breaking D30935 apart. Upon examination, this works as an alternative strategy. As I've looked at this, using merely this half has seemed more and more compelling. In fact this approach meshes very well with D32866/D31188.

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.

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

I think you've got the gist of it, but the borderline isn't quite where you're placing it and there is a bit more before breaking off.

Putting another way, there are 4 distinct locking domains:

1. interrupt_sources[]/x86 interrupts available for reuse
2. xen_intr_auto_vector_count/newly allocating x86 interrupts
3. xen_intr_port_to_isrc[]/isrc->xi_port
4. Acquire/release isrc->xi_refcount

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.

This appears to potentially benefit from D32876 as it means there is no longer a need to use xen_intr_unbind(). There is even more potential for merging things down, but this needs to be done with care. Alas, this doesn't appear to benefit my main goal.

I'm concerned this may work in my tree due to intermediate commits, but nothing is obvious so I suspect it was something long ago.

Trying to salvage some useful portions of this. I still like having xen_intr_release_isrc() do full cleanup, but clearly I goofed which side xi_cookie was best to remain on.

Better arrangement of enum evtchn_type. Nothing outside of the interrupt interface needs this, so reducing the visibility is good. I'm still wondering about the xen_arch_intr_remove_handler() interface. Simplest approach to clearing xi_cookie might be D31188, which ensures everything is cleared.

This isn't even the slightest bit of a priority. Worse, this does indeed make locking harrier.

Adjusting the comment before xen_intr_isrc_lock as per my comment.

In D30726#742595, @mhorne wrote:

In D30726#742593, @ehem_freebsd_m5p.com wrote:

Other issue is the current locking during allocation is really gnarly. Notice how before calling xen_intr_alloc_isrc() the lock must be acquired for access to the x86 allocation variables. Once inside xen_intr_alloc_isrc() after the x86 variables are modified (xen_intr_auto_vector_count is incremented); the lock is released in order to call malloc(); then the lock is reacquired for modifying xen_intr_port_to_isrc[].

You did not explain what is wrong with this pattern. Can something go wrong if the lock is acquired between these sections, or do you just not like it?

I cannot dispute the current implementation functions, but it is really awful. This badly breaks many rules of locking. Whereas the new implementation both acquires and releases xen_intr_x86_lock inside xen_intr_alloc_isrc().

I wish you would quantify at least one way in which it is awful before you call it that.

In D30726#742582, @ehem_freebsd_m5p.com wrote:

I doubt there are performance benefits from using two locks, just the resultant implementation will be of substantially lower quality.

In D30726#742559, @royger wrote:

In D30726#742538, @ehem_freebsd_m5p.com wrote:

See D30936. The goal is everything being protected by xen_intr_x86_lock is very x86-specific and destined to split away from this file. Whereas the portions which remain protected by xen_intr_isrc_lock are mostly architecture-independent and will remain in this file. Splitting the locks allows them to be static.

Would it be possible to just note that the _arch specific functions require external locking by the callers? (and protect them using the common lock) It might be simpler to just keep a single lock, and I don't think there are any performance benefits or otherwise from using two different locks.

At the point when I pushed this to Phabricator I was thinking xi_cookie should perhaps move to the architecture-dependent side. Problem is the way to go with this decision is quite unclear and I'm wondering whether to abandon D32876 since the situation is so unclear right now.

In D30726#742521, @royger wrote:

I'm unsure you actually need to split those apart. Wouldn't Arm also require some kind of locking of the architecture specific bits?

Updating for present tree status. Keeping Phabricator up to date is difficult due to the number of commits needed for this project...

Figured out the unpleasant feeling I was having. The cookie is the *handler* cookie. As such the Xen interrupt should really simply be forwarding it to client code. Given this, the cookie belongs to the architecture independent side and the architecture dependent side should simply forward.

Yes, this got into Phabricator before final testing. Indeed, it is broken. The fix is pretty obvious.

Kind of a "Hey, look what D32876 allows us to do!"

Good news is this did allow identifying places where curthread->tf_intr_frame was obtained from the argument, versus places where it was obtained from curthread. Also places which used driver_filter_t versus duplicating the prototype.

Now notable D32876 shows how isrc->xi_cookie can move to isrc->xi_arch.xai_cookie. Just an issue of getting that decision made.

This is the latest in the series of slowly getting the architecture-dependent and architecture-independent sides completely separated. If the reviewers (@royger) think this is acceptable, this would allow moving xi_cookie to xai_cookie. Thus the interrupt handler cookie would isolated to the architecture-dependent side.

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

Situation of exploring the effects of D31188. I rather like D31188's effect on the interface, but that needs a review before I'm going to start assuming things will head this direction.

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 match D31188 update.

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.

Another small update for the sign auditing.

Another signed/unsigned update, sorry.

Small tweak I had been pondering. Very minor difference.

Tiny update for signed/unsigned issue.

Finish full checking. The prototype for ffs*() seems *really* bad, it needs to match POSIX, but that is still impressively bad.

Update to match what I have been informed is correct.

Update to match comment.

Figuring mark these files too in order to avoid future issues like D30996.

Adjusting several functions to be u_int as that seems more appropriate here (potentially reducing warnings if they get enabled).

Adding suggested SPDX tag.

The Phabricator interface seems to suggest it is possible to remove the Core Team blocking review, but I suspect that is a no-no in cases like this. Due to this could @emaste or @imp give the Core Team okay? Appears the license was approved elsewhere and this isn't GPL (it is a permissive if separated from the Linux kernel).

I prefer pushing D31995 after D30726. That way the original (approved) form can be used and results in some later deltas being smaller. I don't like leaving mines lying around though.

With no guidance, simply do what I suspect was actually desired. Breaking the portion off into a separate diff. So now D32070 is purely making irq variables unsigned, while D32795 is making io_irq unsigned.

Splitting io_apic portion off into separate diff.

This needs someone knowledgeable to look and some testing. I imagine most, if not all, uses of bit sets act as if unsigned; if any do not that would be a problem.

Broken apart in my local repository. I believe under "Commits" the intermediate point I was thinking of should be visible. This might be a good idea.

Trying to clean these out and two more show up.

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.