- User Since
- Feb 20 2021, 4:27 AM (41 w, 4 d)
Sun, Dec 5
Single blank line adjustment, but keeping Phabricator synchronized.
Sat, Dec 4
Tiny update, this line seems appropriate here, but also elsewhere.
Tiny update, pulling an extra line out of the locked section.
Sun, Nov 28
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).
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)
Fri, Nov 26
Thu, Nov 25
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.
Wed, Nov 24
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...
Mon, Nov 22
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.
Sun, Nov 21
Updating for MIPS removal.
Ensuring Phabricator stays up to date with my tree.
Sat, Nov 20
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...
Fri, Nov 19
Wed, Nov 17
Updating given updates elsewhere. This is the set isrc->xi_cpu approach.
Tue, Nov 16
Sat, Nov 13
Fri, Nov 12
Sigh, simple goof fix.
Thu, Nov 11
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.
Wed, Nov 10
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:
- interrupt_sources/x86 interrupts available for reuse
- xen_intr_auto_vector_count/newly allocating x86 interrupts
- 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.
Tue, Nov 9
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.
Mon, Nov 8
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.
Nov 8 2021
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.
Nov 7 2021
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.
Nov 6 2021
Updating to keep synchronized with my local repository. Minimal delta, simply a bit of formatting adjustment.
Nov 5 2021
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.
Nov 4 2021
Another signed/unsigned update, sorry.
Small tweak I had been pondering. Very minor difference.
Nov 3 2021
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.
Nov 2 2021
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).
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.
Nov 1 2021
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.