User Details
- User Since
- Feb 20 2021, 4:27 AM (167 w, 5 d)
Today
Yesterday
Got it working on arm64 and this does indeed seem a better approach to one issue.
I don't know what the commits look like behind the scenes. I think implementing the use of &unpopulated_mem should be a separate commit from adding the use of the e820 region.
Tue, May 7
Right now mostly looks okay, but my initial test failed. Could be my implementation failed, but right now I'm hunting an issue.
Thu, Apr 11
Apr 3 2024
I think the usual is to let the differential owner do the update. Otherwise the steps end up being "Commandeer Revision", update, "Foist Upon".
Mar 8 2024
Most of what is presently here is definitely wrong. xen_intr_handle_upcall() retrieving the trapframe from curthread->td_intr_frame is definitely right. Issue is xen_intr_handle_upcall() needs to behave as a proper driver_filter_t for the XenPCI support. ARM(64) and RISC-V also need this, likely PowerPC would need similar. I do see two ways forward though:
Mar 7 2024
I think so (suddenly worried about famous last words).
This is oddly similar to what Julien Grall did in 2015. For aarch64 the suggested range for the shared information page is available to the platform device. Perhaps the architecture should pass the struct resource * to xenpv?
If you check your archive of e-mail you will find I expressed uncertainty over whether curthread->td_intr_nesting_level should be incremented/decremented. Removing that is fine, but the rest is wrong.
Mar 5 2024
Feb 23 2024
Update to better reflect current state of FreeBSD tree. My repository had several patches, one of which had modified intr_describe(). As such this better reflects current state.
Feb 20 2024
Splitting D40166 back up. While I like the second part, it is lower value. This would serve to make all architectures noticably more similar.
Change the approach and use ->is_event->ie_irq for places besides intr_register_source(). Since D35386 is now a parent, the vector value can disappear from xen_arch_isrc_t. msi_irq can also disappear, but that is more involved.
Due to D36901 getting in first, intr_bind() disappeared. As threatened, I really like intr_add_handler()'s first two arguments being in the opposite order. Not only does this seem more consistent with the rest of the interface, but this also matches INTRNG (the domain argument is now the only difference).
Feb 19 2024
This is another approach to the issue pointed out in D40474, mainly using ->pic_flags as flags and allowing a pic_list entry to fill both roles.
Another variant of this strategy would be to add a pair of boolean arguments to intr_pic_register() so the caller would indicate which roles they can fill. This variant could be argued to be better as each layer of the implementation could get its own pic_list entry.
Depending on where xen_intr_disable_source() and xen_intr_disable_intr() were being called from, this is fixing both 1797ff962769 and some other commit. Certainly 1797ff962769 may have exposed this, but the is_valid_evtchn() call was needed earlier (could be rG76acc41fb7c7, or even before).
Feb 15 2024
@jrtc27 not being on intimate terms with the GICv3 implementation and lacking hardware to confirm how things work, I've got no idea what to do. What I do know is the extra PIC for this device never gets into sc->gic_children and therefore the loop in gic_v3_init_secondary() doesn't work for this device.
Feb 6 2024
@jrtc27 I was hoping for an answer to my question before doing anything.
Uploading the obvious SLIST->STAILQ adjustment.
Guess I had it easy as I had a system running Linux and arcanist was available as a package.
Jan 23 2024
Dec 9 2023
If in the "Accepted" state (someone selects "Accept Revision") the option "Close Revision" appears underneath "Revision Actions". I'm unsure whether there are other ways to get that to appear. Isn't available here since no reviewer ever marked it as such.
The only remaining usage of this is sys/arm/arm/pmu.c. If that can get removed then it become possible to use NULL as a handler argument. I can turn that removal into a review/differential if someone desires, but seems trivial enough to simply commit if someone has a chance.
Nov 7 2023
Nov 5 2023
I think I've found a better way to address this. Alas tis a bunch of commits and the throughput for non-sponsored patches is atrocious.
I mostly concur with D42343, I've noticed the behavior and found it suboptimal. Only trick is after a fair bit of time not looking at this I found a small issue which needs to be addressed.
Oct 23 2023
Oct 10 2023
Hmm, lost the mention of isrc_event_destroy(). I suppose if one wants to have that function, things could look like this instead.
Sep 9 2023
When it comes down to it, since I'm more concerned with getting rid of sys/kern/kern_intr.c:intr_lookup() it is acceptable to continue having intr_{get|set}affinity() using numeric interrupt numbers. This will require sys/kern/kern_intr.c to #include the architecture interrupt headers. I was reluctant to propose that due to how distinct they are, but this doesn't actually contradict my goals and is nominally acceptable to me.
Sep 5 2023
Tiny adjustment, split the NULL-check to a separate step. Just in case a use does the NULL-check itself and simply wants the appropriate field name. Hopefully common-subexpression elimination would take care of this, but it might not.
Sep 2 2023
All of these should be obvious from what INTRNG does. Failing that, an example was provided. So, a few call sequences:
Sep 1 2023
I'm unsure whether the call to child devices during gic_v3_init_secondary() should be removed. The only user of this is the GIVv3 ITS (550d01a2117f). Will most child devices need this called after the parent PIC is fully initialized? Do child devices merely need this called during processor bring-up?
Aug 24 2023
Looks like sys/arm/mv/gpio.c has a situation similar to what I'm looking at. The approach of doing local calls to intr_event_create() has been tested and functions, but this seemed unlikely to be accepted by a reviewer. Yet this makes me the second person to run into troubles with INTRNG due to assumptions in its interface.
This wasn't my goal, but it looks like this series, D38451 in particular fixes the intr_isrc_dispatch() call in sys/arm/mv/gpio.c. Since the counters get moved to the event, it ends up with functioning counters, plus no uninitialized counters get incremented (the function mv_gpio_intr_handler()).
This appears to partially fix the intr_isrc_dispatch() call in sys/arm/mv/gpio.c, since it removes the increment of the counters which have never been initialized.
Alas this came to my attention way to late to address on-time. Callers of intr_event_destroy() want to know whether everything has been appropriately released or not. If the event is NULL this likely something failed very early during setup. In such case the event doesn't need to be released since it was never allocated. As such 0 should be the return instead.
Aug 23 2023
Squash review, adding removal of ->isrc_irq. This is one reason why allocating the event earlier has value, ->ie_irq can then substitute everywhere. If reviewers want this split back off, that can be done.
Hmm, no that isn't really so elegant so keep to the smaller diff.
Hmm, spotted this a while ago, but didn't get Phabricator updated. The test in isrc_event_create() was wrong, this appears to be the correct action.
Having examined things more, an appropriate place to store the value already exists, but has been underutilized. Mainly isrc->is_event->ie_irq.
Aug 22 2023
Move the setting line to isrc_alloc_irq(). This would allow keeping the interrupt number strictly on the event and removing it from intr_irqsrc.
One extra may be needed for this. The file sys/arm/mv/gpio.c has a call to intr_event_create(). Unfortunately this isn't setup for INTRNG allocating events early. Interestingly it seems ad2be10ff043 is exactly the same issue I'm running into.
I've been concerned over whether the initial version of D40166 would pass. The use case was too specialized. As such an update, remove the lazy interrupt allocation.
Aug 18 2023
A handy example is D38599. It could test isrc->isrc_event before calling intr_event_destroy(), but if intr_event_destroy() will gracefully handle ie == NULL then it is easier not to bother. If you prefer to have a panic() sooner rather than later, the
if (ie == NULL) return (EINVAL);
should be taken out and hopefully a panic() will ensue on line 539. If intr_event_destroy() is going to check for ie == NULL then it should do something useful if that occurs. Returning EINVAL is almost certain not to be useful and thus returning 0 seems more sensible.
Errf, then find the goof after push.
Update to get things right. Though this is almost a fix on commit situation.
Aug 16 2023
Update as per comments.
Aug 15 2023
In light of D41319 and the issues of what was observed when similar was done there. Presently this is untested, but I'm reasonably sure this is correct.
I hadn't realized there was an actual sys/arm/arm/generic_timer.c which appears to be used for ARM64. As such I've identified 3 places on ARM where a single intr_event structure is used for interrupts which occur on multiple processors. I now fear this could be all over ARM and this may already have been endorsed by fiat.
Aug 14 2023
I'm unsure whether it works well as part of any existing differential, but it was suggested there should be a cpu_t type definition. That didn't fit as part of D36901, but does seem worthwhile. That would likely address the breakage which was found.
"My kingdom for a review"?
This looks plausible. Likely it mostly amounts to a debugging tool since you would basically never want this in anything approximating a real world situation.
Aug 11 2023
When it comes down to it, 9b33b154b53 was pretty much a simple hack to allow architectures without a proper interrupt table implementation to do simple interrupt number lookup. Thing is, MIPS was the last architecture lacking such a table, therefore this is now fully redundant with architecture functionality. As such this is effectively cleaning out yet another MIPS remainder.
Unfortunately see D38448 for most comments so far.
Updating D38448 to split off the counter addition. At this point I'm under the suspicion the core team needs to consider this, and say "yay" or "nay". Whether interrupts which occur on multiple processors should share an intr_event structure is a real question. So far several places appear to be saying "yes", but I would like a core team review.
This is splitting the counter add commit off the flag add commit. Originally these seemed like a single task, but seems further consideration is needed for the flags.
Aug 9 2023
Uhm, this would be kind of useful to have. Any chance this can get a review?
Missed it when looking for a fix in recent history. Most recent build succeeded, so looks like this was the fix.
Latest build attempt failed. This is my current suspect.
Aug 8 2023
Rebuild with 8920c5f2a11 reverted succeeded. Looks like D41320. The host is FreeBSD 13.2p2 with minimal extras.
I'm starting to think the flag addition to he lower layers should be broken into a separate review. Issue is that has some effects all over the place.
At the end of a full build which just terminated:
ld: error: unable to find library -lsoxstack clang: error: linker command failed with exit code 1 (use -v to see invocation) --- stack_dt_need_exec_test.full --- *** [stack_dt_need_exec_test.full] Error code 1
Looks like this broke the build.
Updating to re-add a commit which got dropped.
Updating, spreading the proposed flag further. Still need an answer for whether multi-processor interrupts should share an intr_event structure though.
Aug 5 2023
Appears INTRNG isn't the only thing tending to push multi-processor interrupts onto single intr_event structures. At D25754 a multi-processor soft interrupt was created. As such it would need to share this.
Jul 11 2023
Alerting everyone this is on the horizon. A check 3 weeks ago showed there are 3 remaining places where this is utilized. I've got patches out trying to get them nuked while bypassing Phabricator (they're trivial), but am presently waiting on news. Those are in the files sys/arm/arm/pmu.c and sys/x86/xen/xen_apic.c.
Jun 29 2023
Jun 28 2023
Jun 27 2023
Unfortunately there are several overlapping issues interwoven here and choices need to be made.
Jun 24 2023
Note on this. Should this perhaps be a separate file in sys/kern instead of mixed in with INTRNG/subr_intr.c?
Jun 23 2023
Jun 17 2023
With no activity, hopefully doing an update will provoke activity.