xen/intr: introduce xen_arch_intr.c

[ehem_freebsd_m5p.com]

Summary

Break large portions of x86-specific code into a file separate from
xen_intr.c. A fair amount of the event channel code can be shared
among architectures, but the current lowest layer cannot.

The original implementation was done by Julien Grall in 2015, but this
has required very substantial updating.

Removal of PVHv1 meant substantial portions disappeared. The original
implementation took care of moving interrupt allocation to
xen_arch_int.c, but this has required massive rework and was broken off.

Submitted by: Elliott Mitchell <[e-mail]>
Original implementation: Julien Grall <[e-mail]>, 2015-10-20 09:14:56

[ehem_freebsd_m5p.com]

Updating to match with D30648 update (update to diff, though the result is the same). Reorder xen_intr_pic_enable_source()/xen_intr_pic_disable_source() to match xen_intr.c ordering.

[ehem_freebsd_m5p.com]

Reverting to previous version. Upon examination, that was not an improvement.

[ehem_freebsd_m5p.com]

Fixing corruption which occurred during work. Ensuring more comment blocks get preserved from portions completely removed from xen_intr.c.

[ehem_freebsd_m5p.com]

Update fixing logic inversion found during look. The return of xen_arch_intr_has_handlers() is inverted from how the KASSERT() was previously.

[ehem_freebsd_m5p.com]

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.

[ehem_freebsd_m5p.com]

sys/xen/arch-intr.h

71 ↗

(On Diff #98164)

I'm wondering about this. Neither architecture needs the struct xenisrc *isrc here, as both are ultimately forwarding the cookie to intr_event_remove_handler() which merely needs the cookie.

Likely @julien_xen.org was also confused about which side should own xi_cookie.

[ehem_freebsd_m5p.com]

sys/xen/xen_intr.c

1172–1173 ↗

(On Diff #98164)

One can argue against folding bugfixes into other commits, but this one might be appropriate. After the call to xen_arch_intr_remove_handler() should be isrc->xi_cookie = NULL;. This is to prevent a potential double-free issue.

[ehem_freebsd_m5p.com]

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.

[ehem_freebsd_m5p.com]

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

[ehem_freebsd_m5p.com]

sys/xen/arch-intr.h

66–78 ↗

(On Diff #98365)

I'm rather tempted to have these move to machine/xen/arch-intr.h. For both x86 and ARM64 these functions end up being small enough for inline. In particular xen_arch_intr_execute_handlers() is fairly hot (and is only called by one point in xen_intr.c).

77 ↗

(On Diff #98365)

To remove the first argument or not to remove the first argument? No architecture being worked on needs the first as they all end up being calls to intr_event_remove_handler() which only needs the cookie. With xi_cookie being architecture-independent, I'm inclined to remove the first.

[ehem_freebsd_m5p.com]

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

[ehem_freebsd_m5p.com]

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

[ehem_freebsd_m5p.com]

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

I found xen_intr.c doing #include <machine/xen/arch-intr.h> and <machine/xen/arch-intr.h> doing #include <xen/arch-intr.h> was better than the reverse. This allows many of the near-trivial wrappers in xen_arch_intr.c to become static inline.

At this point nothing outside of the Xen interrupt code is using enum evtchn_type, so move that to sys/xen/arch-intr.h.

One remaining question, how does everyone feel about casting pointers to functions? Having xen_intr_pic_vector() match the struct intsrc.pic_vector; and not declare its argument struct xenisrc *isrc suggests that is frowned on. Yet casting that way does have advantages...

[ehem_freebsd_m5p.com]

Single blank line adjustment, but keeping Phabricator synchronized.