Page MenuHomeFreeBSD

xen/intr: use struct xenisrc * as xen_intr_handle_t
ClosedPublic

Authored by ehem_freebsd_m5p.com on Mar 30 2021, 6:54 PM.

Details

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.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 38326
Build 35215: arc lint + arc unit

Event Timeline

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

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

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.

Looks sensible to me.

sys/x86/xen/xen_intr.c
212

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.

228
This revision is now accepted and ready to land.Mar 31 2021, 2:00 PM

Adding the proposed __inline. No objection to this.

This revision now requires review to proceed.Apr 5 2021, 5:19 PM

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 added inline comments.
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.

This revision is now accepted and ready to land.Apr 5 2021, 11:48 PM

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
212

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.

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.