Page MenuHomeFreeBSD

xen/intr: move interrupt allocation/release to architecture
ClosedPublic

Authored by ehem_freebsd_m5p.com on Jun 28 2021, 9:55 PM.
Tags
None
Referenced Files
F84765366: D30936.id91464.diff
Tue, May 28, 7:27 AM
F84716775: D30936.id93431.diff
Mon, May 27, 2:48 PM
Unknown Object (File)
Sun, May 19, 2:52 AM
Unknown Object (File)
Mon, May 13, 5:54 PM
Unknown Object (File)
Fri, May 10, 12:28 PM
Unknown Object (File)
Fri, May 10, 12:28 PM
Unknown Object (File)
Fri, May 10, 12:28 PM
Unknown Object (File)
Fri, May 10, 12:28 PM
Subscribers

Details

Summary

Inspired by the work of Julien Grall <julien@xen.org>,
2015-10-20 09:14:56, but heavily adjusted.

Simply moving the interrupt allocation and release functions into files
which belong to the architecture. Since x86 interrupt handling is quite
distinct from other architectures, this is a crucial necessary step.

Identifying the border between x86 and architecture-independent is
actually quite tricky. Similarly, getting the prototypes for the
border right is also quite tricky.

Since interrupt structure allocation/release is now in architecture
code, the malloc structure needs to be passed. Some architectures also
need the device when allocating interrupts.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Lots of action in D30936 as it is near the completion of the architecture-independent API for FreeBSD/Xen. I ended up splitting this off of D30909 due to the shear complexity involved in this portion. Unfortunately this has the effect of requiring xen_intr_pic being global/declared in sys/x86/include/xen/arch-intr.c between D30909 and D30936. Yet isolating this portion of the complexity seems worthwhile.

As you can see, not one, but 3 functions are fairly tightly interlinked and are best to move together. Similarly 3 static variables also need to stick together.

sys/xen/arch-intr.h
63–65 ↗(On Diff #91464)

I'm not 100% certain I got these right. The device name is useful for the intr_create_event() call on arm64, though I don't believe that is absolutely required.

The bigger question is whether to have M_XENINTR remain in xen_intr.c, versus moving with xen_arch_intr_alloc() to the architecture-specific file. In earlier versions the call to malloc() had remained in xen_intr.c, but that became untenable when it was clear xen_intr_find_unused_isrc() should move to architecture (since identifying reusable interrupts relies on highly architecture-specific structures).

I think I had also initially been seeing hints part of the memory handling might remain in xen_intr.c, but that may have disappeared with PVHv1.

sys/xen/arch-intr.h
63–65 ↗(On Diff #91464)

So this really does look like a good question of what these should look like. The minimum arguments are:

struct xenisrc *xen_arch_intr_alloc(enum evtchn_type type, evtchn_port_t port);
void    xen_arch_intr_release(struct xenisrc *isrc);

Testing confirmed passing the name is actually unnecessary. Meanwhile, if the architecture file takes ownership of M_XENINTR then that can be static to architecture and need not be passed around. Checking found the other use of M_XENINTR disappeared from with D30228.

Thoughts? (somehow I'm guessing those are going to be "do it")

Another possible prototype might be struct xenisrc *xen_arch_intr_alloc(const struct xenisrc *params);. This has the advantage of xen_intr_bind_isrc() could merge its type and port parameters together. Better yet, if xen_intr_bind_virq() set xi_virq then arm64 could use these in the intr_event_create() call (for virqs I would tend towards "xen-virq-%u" as the format).

Then the architecture functions simply do memcpy(&isrc->xi_arch + 1, &params->xi_arch + 1, sizeof(struct xenisrc) - sizeof(xen_arch_isrc_t));, to copy everything from the passed-in temporary structure to the final structure.

Updating diff to match other diffs. Deltas which overlap can be trouble.

Update reflecting logic fix to D30909. The return of xen_arch_intr_has_handlers() is inverted from what had previously been used.

sys/x86/xen/xen_arch_intr.c
406–408

Mentioned on D30726, should this segment be dropped? These values must be set during the allocation process, while the architecture code only looks at ->xi_type (== EVTCHN_TYPE_UNBOUND, presently unallocated, other values indicate in use). Though looks like xen_intr_bind_isrc() may need to start clearing ->xi_cookie (which it should have been doing already, D31188 implements this implicitly).

sys/xen/arch-intr.h
63–65 ↗(On Diff #91464)

Though by doing less in D30935 it is possible to remove the requirement for passing the port (leaving the xen_intr_assign_cpu() in xen_intr_bind_isrc()). Alternatively D31188 passes the port (and everything else; this in fact was the inspiration for D31188).

Updating to present status. Difficult keeping everything up to date when you've got >60 diffs and ~100 commits in the queue.

This revision was not accepted when it landed; it landed in state Needs Review.Apr 14 2023, 2:02 PM
This revision was automatically updated to reflect the committed changes.