xen/intr: use struct xenisrc * as xen_intr_handle_t

[ehem_freebsd_m5p.com]

Summary

Since xen_intr_handle_t is meant to be an opaque handle and the only use
is retrieving the associated struct xenisrc *, cut the Gordian Knot and
simply use a struct xenisrc *.

I'm puzzled at the reason for previously using an interrupt vector
number as the value. Also some of the other values. Why not simply the
thing actually needed?

Also add a wrapper function for converting the other direction. If some
other value becomes appropriate in the future, these two functions will
be the only spots needing modification.

[ehem_freebsd_m5p.com]

Removing the "has" from the function name as requested. Seems quite
appropriate.

[ehem_freebsd_m5p.com]

Erk, reverting the state of the review system. In the wrong place in
history when running arc diff. Sorry about the noise.

[ehem_freebsd_m5p.com]

In Julien's original commit series the function an architecture-specific xen_arch_intr_loookup_source() function was created and used by xen_intr_isrc().

During work with that two issues showed up. First it made no sense for the lookup side (from handle) to transform a handle to isrc without also being able to choose which value was used as the handle. Second during my experimentation it became clear xen_arch_intr_lookup_source() needed the full pointer value.

Once done it was clear the struct xenisrc * itself was pretty well the ideal value to use as the handle. This also has the nice property of being architecture-independent. I'm wondering why that value hadn't been used as the handle.

[mhorne]

Looks sensible to me.

sys/x86/xen/xen_intr.c

213

Since this is just a type cast, the helpers can be marked __inline or converted to macros.

Optionally you can shorten the name too, since they are private to this file, e.g. handle_from_xenisrc/xenisrc_from_handle.

229

[ehem_freebsd_m5p.com]

Adding the proposed __inline. No objection to this.

[ehem_freebsd_m5p.com]

Okay, got the inline marking in. I'm unsure of using short names. The origin of this was Julien's patches for FreeBSD on Xen/ARM made xen_intr_isrc() call an architecture-specific function xen_arch_isrc_handle() which was in a separate architecture-specific file.

Since turning xen_intr_handle_t into an alias of struct xenisrc * fixes the issue for ARM, but also works quite well for x86, my adjustments nuke the proposed xen_arch_isrc_handle() function. Yet I see a possible future where xen_intr_handle_from_isrc() and xen_intr_isrc_from_handle() were turned back into architecture-specific functions. As such I think non-local names should be used.

sys/x86/xen/xen_intr.c

1517

Actually, I'm halfway tempted to rename "port_handle" to "handle" while I'm at it. Everywhere else it is "handle" for being passed in or "port_handlep" for a pointer being modified. Calling it "port_handle" here seems cause for confusion.

[mhorne]

sys/x86/xen/xen_intr.c

1517

Up to you, but I'm not very concerned here. The number of uses of port_handle[p] and handle are roughly equal in the current version of this file.

[royger]

LGTM, I'm however tempted to s/__inline/inline/ at commit unless there's a reason to explicitly use __inline.

sys/x86/xen/xen_intr.c

213

Any reason to use __inline instead of the C99 keyword inline?

I can fix while committing if there's no specific reason to use __inline.

[mhorne]

In D29500#663586, @royger wrote:

LGTM, I'm however tempted to s/__inline/inline/ at commit unless there's a reason to explicitly use __inline.

That's fine. I believe __inline should be preferred for user-visible headers, but is not at all required here. Just muscle memory on my part.