Details

Reviewers

julien_xen.org
royger
mhorne

Commits

rG5ca00e0c985f: xen/intr: use struct xenisrc * as xen_intr_handle_t

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

rG FreeBSD src repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

ehem_freebsd_m5p.com created this revision.Mar 30 2021, 6:54 PM

Herald added a subscriber: imp. · View Herald TranscriptMar 30 2021, 6:54 PM

Harbormaster completed remote builds in B38187: Diff 86567.Mar 30 2021, 6:54 PM

ehem_freebsd_m5p.com requested review of this revision.Mar 30 2021, 6:54 PM

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

Harbormaster completed remote builds in B38188: Diff 86568.Mar 30 2021, 6:58 PM

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

Harbormaster completed remote builds in B38189: Diff 86569.Mar 30 2021, 7:00 PM

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
181	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`.
197

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

Harbormaster completed remote builds in B38326: Diff 86849.Apr 5 2021, 5:20 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
1198	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 accepted this revision.Apr 5 2021, 11:48 PM

mhorne added inline comments.

sys/x86/xen/xen_intr.c
1198	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.