Updating, point to possible solution for what I need @mav to review (remove ->pic_enable_intr() call from clock.c/875b8844bec hack).

In D47846#1090926, @imp wrote:

yea, the only question I have is if we move all the assembly values into machine/intr.h, duplicating one or two across architectures and include machine/intr.h for the assembler, or if I leave it as I have it inthe review. @jhb

Seems the issue in sys/x86/isa/clock.c originates from rG:875b8844bec. I'm unsure how to work around the issue, rG:49ed68bbf3c makes the situation worse.

Yet the solution to the underlying issue is also quite simple (already have a commit on GH, I can upload to Phabricator if desired), so perhaps that should be considered? If nothing else that gives an additional option which may turn out rather superior for the long term. The underlying issue has likely been causing all sorts of (perhaps minor) trouble for some already.

Then when looking for something else discover sys/x86/isa/clock.c has the call i8254_intsrc->is_pic->pic_enable_intr(i8254_intsrc); without the ->pic_enable_source(). I lack hardware to test this, so I'm unsure of the effect.

In D47846#1090953, @jhb wrote:

I don't see a compelling reason to duplicate values across architectures. That would seem to defeat the point of the shared header file.

This seems reasonable, though I'm concerned with the present approach.

I dislike making the interrupt headers assembly-safe merely for this. If this is really desired, then have the all constants in machine/intr.h (which is better anyway).

Unless there is something I'm unaware of, INTR_IRQ_INVALID isn't ever used by assembly-language.

Closing in light of rG:700f7e793b3793606fbb9942eda03ac02991c126.

Updating to jhibbits@ preferred method of accomplishing the goal. This is only expected to be used during a transition period, so hopefully will disappear later.

Note to anyone else who happens to run across this. Indeed, the (mask_fn)isrc->is_pic->pic_disable_source could be substituted for the intr_disable_srcargument to intr_event_create() inside intr_register_source(). Issue is after this commit, #1457 first modifies intr_disable_src and then later completely removes it. At the point where this is the wrapper still needs to be retained.

Mostly looking for a review from @royger I'm unsure whether this additional call really needs to be retained, versus being unnecessary.

As proposed.

Reclaiming/reopening as per note in D47279. This could be a better solution to the issue pointed at.

In D47279#1078374, @jrtc27 wrote:

Says the person sending messages laced with hostile sarcasm.

Wow, so friendly to revert something immediately after the person gets a rebase done and without informing them about discussion which was occurring. Also odd how there is suddenly all this discussion now when there was no activity on the original source for more than a month. I tried to get everyone's attention who is on here.

This is part of a fascinating end point I reached in combination with other commits. Specifically, Github #1457. I think this is better to review via GitHub, since the full git support allows viewing as part of the full series. Phabicator isn't all that well suited to the role.

The problem which this was a part of solving looks like it will be dealt with via other means. As such dumping this.

Once the fixes to the multi-root support are in, using those will address my need. Unfortunately the problem brought up by the comment. "QQQ: Only root PIC is aware of other CPUs ???", still remains. This is no longer productive for me, so abandoning this one.

If someone is up for forward progress on this trivial issue, I recommend doing: (assuming your remote for FreeBSD-Github is "github")

I had thought that it might be better, if you were going to push this through quickly, to simply push past and deal with the aftermath. Issue is you've now taken long enough for me to start organizing my thoughts better.

Having two distinct names for the interrupt headers serves to interfere with most efforts to converge the interrupt frameworks. Simply rename all of them to "machine/a_bikeshed_string_for_sed_to_target.h". Another name could be substituted, but right now simply have a random one for this commit.

I was simply looking for a pause, so now removing the pause.

I guess I'll have to leave it there then.

In D47002#1071817, @imp wrote:

If it breaks the Gordian Knot, let's create machine/interrupt.h to contain the APIs that are common between INTRNG and !INTRNG, lets do it. Let's see where we wind up, and we'll discover if it's too trivial or not.

In D47002#1071806, @kevans wrote:

In D47002#1071777, @ehem_freebsd_m5p.com wrote:

Take a look at the first two commits of GitHub #1286. The first two create an interrupt_t type definition for things which only handle pointers to the architecture type, but don't directly examine the contents. If you have to #include sys/intr.h, then it is literally impossible to write truly machine-independent beyond having a void *.

Where did someone dictate that you have to include sys/intr.h, and for what? With the little details you've given it sounds like you're imposing restrictions on yourself, there's no hard requirement on INTRNG to include machine/intr.h. I don't see anything in your change offhand that's prevented by INTRNG consumers wanting sys/intr.h instead of machine/intr.h. You can include just the header you actually need.

In D47002#1071804, @imp wrote:

If you need a type that's really MI, define it in a MI header (say machine/types.h or invent machine/interrupt.h). End of problem. There's not even a need for #ifdef anything. Just include machine/interrupt.h which has a well defined interface (because you just defined it and it has no historical issues).

In D47002#1071782, @jrtc27 wrote:

You already need to #ifdef INTRNG for machine/intr.h, doing it for sys/intr.h is no worse in that regard. So kindly calm down.

In D47002#1071690, @imp wrote:

This is the more typical pattern.

See sys/endian.h including machine/endian.h for but one of many examples (though that example is a bit convoluted in spots due to slight differences between endian.h, sys/endian.h and machine/endian.h consumer expectations.

NO ABSOLUTELY NOT. The reason is this makes architecture-independent source impossible without a #ifdef INTRNG.

I suspect this needs to now be rebased onto main. I've got concerns, so I would like to take a look at what comes when the interaction with the FIQ stuff comes in. Simply asking looking for a temporary pause.

@jrtc27 my one concern was to alert everyone here that discussion has occurred elsewhere. I was thinking discussion might have ended here, but it is now clear it has not terminated. Now I know I need to be tracking both.

In D40161#954037, @kevans wrote:

I was trying to avoid enums because I didn't want to cement us into new KPI for other types when we can easily discriminate with an argument, and enums add room for error since they're a C construct.

This is a real problem with having both Phabricator and GitHub. This got copied to GitHub and now there is some discussion there. This was likely due to the long pause after September 7th, someone really wanted this feature.

The one comment can be done on commit.

This is basically okay, but there urgently needs to be a comment here. At a future point the versions of Xen which lack this hypercall will no longer have any support and the error should instead be returned. This will likely be at least 5 years before this can be done, but there should be a comment with specific information.

Seems fine, though as this is x86-land so I'm not doing testing. I do strongly prefer this being a separate commit from D46122 as it now is.

I had expected to simply handle D44251 as a squash review, though this works too. I do want to note what the output of devinfo -u looks like.

I doubt it is possible to converge the interrupt infrastructures without exposing this interface. This interface is the common point duplicated on every architecture, so things go through here. The way to discourage access to this interface for your alternative interface to provide valuable services which simplify consumers. For my case (Xen) where the drivers already exist, using the higher-level interface would be much harder.

I'm catching all Xen via a Herald rule. This is deep in the x86 side where I haven't done experimentation. Looks plausible, but I'm not your reviewer.

The interesting part is the combination of D39333 with D35559 and D39178. The result would be most of this low-level interface would be highly similar to x86. If the "BUS" interface was split into a separate file, it might be possible to share this portion between INTRNG and x86 with some work (INTRNG presently has no support for processor domains).

Update to current tree status (reflecting GitHub). Now retains INTRNG's argument order for intr_add_handler(). x86 was convinced to match INTRNG and thus the two prototypes are now quite similar.

In D35607#836545, @mmel wrote:

INTR_SOLO have nothing to do with shared interrupts. It's a fast path for cascading interrupts, and instead of removing it, we should extend it to support pass-through interrupts, polish the API for it, and put it in the regular build. Both of these are common in the arm/aarch64 world, and we lose performance significantly in these cases (and the big question is whether this should help us for LPI interrupts as well).

One other sort-of workable approach would be add a requirement for pic_init_secondary() implementations to be safe for being call multiple times. Then have intr_pic_init_secondary() loop through pic_list after calling PIC_INIT_SECONDARY(intr_irq_root_dev);.

Kind of overdue for action. I had already been moving discussion elsewhere.

Rounding up reviewers and getting reviews updated seems to get done more effectively via GitHub. I'm concerned about GitHub's handling of open source projects, but if that is the only way to make progress...

@royger under "Revision Contents" select the red tab "Stack (2 Open)". This is how Phabricator handles series and interrelated commits. The bits you were wondering about are there.

Does the silence mean it is now time to move this onto GitHub for landing? (may be better since this covers more than 1 commit)

The one to consider D45381 is @royger who handles the Xen interface for FreeBSD.

Hopefully commenting doesn't cause this to reopen.

Ping D40474. I suspect the chosen approach will be D43980 -> D40474, but it would also be viable to simply restrict intr_pic_init_secondary() to only call for FLAG_PIC entries.

If nothing shows up on D38599 soon I might try GitHub since there seems better throughput there.

I need a very good insecticide to get rid of the crickets inhabiting D40776.

Any word on D43980? Does seem appropriate to only create a single pic_list entry for combined PIC/MSI controllers.

I think one real issue is what question is the caller most interested in having answered? (since C functions have a single return, only 1 question can be answered) Another is how is the caller likely to react if intr_event_destroy() gives an error return?

Definitely want this, though still got some nitpicks.

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.

Right now mostly looks okay, but my initial test failed. Could be my implementation failed, but right now I'm hunting an issue.

This is acceptable to me, but I believe the desire is have an okay by @markj. I believe resigning should make this more visible to @markj, rather than accepting.

I think the usual is to let the differential owner do the update. Otherwise the steps end up being "Commandeer Revision", update, "Foist Upon".

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:

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.

So, most approaches to getting interrupt subsystems to converge are going to need something along the lines of D35559 at some point. Might it be possible to get a decision/activity on D35559?

I had been thinking @jrtc27 was supposed to remove her "changes requested" tag, yet I was advised she thought @mmel should instead review the replacement. So, still need a decision as to whether the approach is D43980 -> D40474, versus intr_pic_init_secondary() only calling for FLAG_PIC entries.

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.

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

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

In D40474#1001982, @jrtc27 wrote:

I'm really struggling to understand what the source of confusion here is.

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

@jrtc27 I was hoping for an answer to my question before doing anything.

Uploading the obvious SLIST->STAILQ adjustment.