intrng: Extract arm/arm64 IPI->PIC glue code

[jrtc27]

Summary

The arm and arm64 implementations of dispatching IPIs via PIC_IPI_SEND
are almost identical, and entirely MI with the lone exception of a
single store barrier on arm64 (that is likely either redundant or needed
on arm too). Thus, de-duplicate this code by moving it to INTRNG as a
generic IPI glue framework. The ipi_* functions remain declared in MD
smp.h headers and implemented in MD code, but are trivial wrappers
around intr_ipi_send that could be made MI, at least for INTRNG ports,
at a later date.

Note that, whilst both arm and arm64 had an ii_send member in intr_ipi
to abstract over how to send interrupts,, they were always ultimately
using PIC_IPI_SEND, and so this complexity has been removed. A follow-up
commit will re-introduce the same flexibility by instead allowing a
device other than the root PIC to be registered as the IPI sender.

MFC after: 1 month

[jrtc27]

Fix tinderbox build (missing headers for arm)

[ehem_freebsd_m5p.com]

Even though intr_ipi_dispatch() duplicates how the existing implementations act, I must object to having such in INTRNG. Likely intr_ipi_dispatch() should handle the trapframe similar to how intr_event_handle() handles them. Everything else seems fairly minor and could be handled afterwords, but I find intr_ipi_dispatch() unacceptable.

In other news I was able to rebase D36610 on top and successfully build. After the build succeeded, it booted successfully and appeared to operate normally. As such this does appear a good idea, but there is the one spot I object to.

sys/arm/arm/mp_machdep.c

290–292

This needs to be fixed.

310–312

Another problem in need of fixing.

sys/kern/subr_intr.c

139

May not be a good fit for this patch, but find | xargs grep was unable to find any uses of ii_name. This should likely be purged.

1767

Perhaps shows the type of person I am. I would tend towards intrcnt + ipi. Though this falls under cleanup which isn't the intent of this patch.

1778

This seems to deviate from the style of the functions in INTRNG. All other functions will give error indications and not panic().

This could explain why this has remained on the architectures, it was never cleaned up appropriately for moving to the core. I think all of the added panic()s should be considered for removal.

1780

Another instance where pointer arithmetic is appropriate. Though again cleanup isn't the intent of this patch.

1811

This is the only use of ii_name I was able to locate.

Since the size can be obtained by sizeof(ii->ii_name) I'm inclined to suggest INTR_IPI_NAMELEN should be nuked.

1826

Another panic() which should likely have an error return instead.

1853

Remove this variable.

1858

Last one, "me too!"

1866–1867

I'm going to have to object to this. This might be suggest how the situation with intr_event_handle()'s handling of the trapframe evolved. The argument should be the passed-in argument, period. If the called function needs the trapframe, curthread->td_intr_frame should be getting set appropriately.

This is how the existing instances handle this, but this needs to be fixed before coming into INTRNG.

[ehem_freebsd_m5p.com]

I'm extremely suspicious how ipi_hardclock()/ipi_preempt() modify td_intr_frame and td_nesting_level on ARM, but not ARM64. Is this actually needed on ARM?

Also note, my strong objection is to 1866-1867 and conditionally passing trapframe or the argument. Everything else could be handled by later commits, but that one is unacceptable.

[ehem_freebsd_m5p.com]

I've tried this and it unsurprisingly passed. Still have the objection, but noting it does appear to work.

[ehem_freebsd_m5p.com]

In some situations, source code review systems are about as precise as using individual nuclear weapons to target individual insects. I'm unsure I got this into the right state, so perhaps getting it out of this state will push things forward.

My notes on this are at https://gitlab.com/ehem/freebsd-src/-/commits/D35898/

First thought when looking at ipi_preempt() and ipi_hardclock(), was they really wanted intr_ipi_dispatch() to emulate intr_event_handle() and do the curthread->td_intr_frame push for them. This got me here.

Further reading history makes me suspect intr_ipi_dispatch() was originally intended to be called from assembly-language or a device-specific interrupt function. As such ipi_preempt() and ipi_hardclock() didn't expect to already be in a critical section. Yet now all callers of intr_ipi_dispatch() have already gone through INTRNG and are already critical.

Yet while the ARM64 versions of ipi_preempt() and ipi_hardclock() were updated, the ARM versions were not. As such I suspect the correct course of action is to match the ARM64 versions, then stop passing the trap frame to intr_ipi_dispatch(). This would suggest squashing 2fcd147b9fd3 over 567e6d465e1f.

In fact I notice none of the ipi-handling functions touch their argument at all. I'm not presently going to advocate removing the handler argument, but I note doing so shouldn't harm anything.

sys/kern/subr_intr.c

1778

Perhaps this should instead be a KASSERT()? This isn't supposed to happen so that may be good enough.

1826

Same case, perhaps this can be a KASSERT() since it isn't supposed to ever occur in non-debug environments?

1844–1849

Both the ARM and ARM64 versions of this function used to have this comment. History suggests to me at the time it was removed from the ARM version, it should also have been removed from the ARM64 version, alas they drifted apart. I suspect this no longer applies and should be removed.

1858

Same thought came up.

[jrtc27]

I would rather leave them as panics; it avoids changing more than is necessary to move the code around and having the panics hasn't so far been deemed a performance issue on arm64. If it later turns out to be then people can convert them, but I doubt it. As for returning error codes instead, you could, but again that's more invasive a change for little gain as all that'll happen is you'll have every caller panic if it gets an error instead, since the callers can't forward on an error anyway. So you'd just end up with multiple copies of the checks, one per architecture, which doesn't really achieve anything.

sys/kern/subr_intr.c

1767

When I want a pointer to a specific element I use &a[i]. When I want a pointer that starts at a specific element I use a + i. They're defined to be semantically equivalent by C (unfortunately for sanitizers and other related things...) but I feel that better conveys intent. So no.

[ehem_freebsd_m5p.com]

sys/kern/subr_intr.c

133

Notable INTR_IPI_NAMELEN has a purpose which is awfully similar to INTR_ISRC_NAMELEN. I'm not certain they should be merged, but they're certainly candidates for doing so.

[ehem_freebsd_m5p.com]

I've got this visible based on top of D37938 (GitLab, includes 2 cleanup commits). I would mark this approved in that state, since the functionality is reasonable to share.

sys/kern/subr_intr.c

139

If the functionality of D36610 gets in, in a form similar to how it presently is, this might allow fully removing intrnames. Not what I was expecting.

[mhorne]

LGTM. For this step let's just do the code move/deduplication and possible refinements to the interface can follow.

sys/kern/subr_intr.c

8

[ehem_freebsd_m5p.com]

Now that D37938 has gone in.

[ehem_freebsd_m5p.com]

Note on this. Should this perhaps be a separate file in sys/kern instead of mixed in with INTRNG/subr_intr.c?

While it is valuable to share this between multiple architectures, I'm unsure this really overlaps with normal interrupts too much. The main overlap is the use of the counters, but I have proposed moving the counters to intr_event at which point the tables of counters would only be for IPI interrupts.

I'm finding myself wanting to see PPI interrupts merged into IPI interrupts and then split off from INTRNG. Perhaps this move would be an intermediate step in that process.

[jrtc27]

Rebased

[jrtc27]

Fix arm build

[mhorne]

sys/kern/subr_intr.c

1749

This should be more correct based on how we allocate the counters intr_irq_init()/intr_ipi_setup_counters().

[jrtc27]

sys/kern/subr_intr.c

1749

I just moved this from sys/sys/intr.h since it's no longer needed outside this file, which I guess got missed by 2f0b059eeafc ("intrng: switch from MAXCPU to mp_ncpus") (which, incidentally, has an inaccurate commit message...). But can correct this as part of the move.