xen/intr: adjust xen_intr_handle_upcall() to match interrupt filter

[ehem_freebsd_m5p.com]

Summary

xen_intr_handle_upcall() was originally implemented as a direct call
from an x86 assembly language interrupt hook. This works if you're
x86-only, but is problematic for other architectures.

The lapic_eoi() call is only needed on x86. Using a driver_filter_t
hook removes the need to call critical_enter()/critical_exit().
Additionally the interrupt counters get taken care of outside of the Xen
driver.

Since this allows reporting of stray interrupts (unlikely, but bugs
could exist), implement the functionality. Presently I don't believe
any Xen device drivers have the potential to report stray interrupts,
but this allows them to do so. Currently I believe this only works on
ARM64.

[ehem_freebsd_m5p.com]

The idea behind this is non-x86 really needs a function which can be passed to bus_setup_intr(). This results in two differences which I don't believe need any modifications what-so-ever to x86. First, instead of taking struct trap_frame * it takes void *.
Second, instead of being void, it returns int.

Of these the second is more substantial. On x86 this results in the value of %eax/%rax being changed. I'm wanting to say that is scratch for the callee on x86 and so has no effect. This though is done in sys/{amd64|i386}/{amd64|i386}/apic_vector.[sS] so caution is warranted. Doesn't appear to effect the assembly language, but note this isn't really tested on x86.

[royger]

You will also need to change the call to intr_execute_handlers as that's x86 specific also? And the lapic_eoi call.

Or maybe we could provide a wrapper for x86 or Arm around the function if the only issue is the prototype.

[ehem_freebsd_m5p.com]

Then I realize this really should be part of the work with the Xen interrupt work (which could still end up as a rewrite instead of being based off sys/x86/xen/xen_intr.c).

The reason xen_intr_handle_upcall() would be adjusted is since there isn't a great substitute. Reason intr_execute_handlers() and lapic_eoi() won't get this treatment is those simply aren't needed on other architectures.

Alas, this suggests both D29875 and D30006 need to be marked as on hold. Any idea whether an abandoned Phabricator differential can be brought back without a new differential?

[ehem_freebsd_m5p.com]

Revision Actions => Plan Changes

Hopefully this has the right effect. I now think D30006 was brought in too early. I was looking for commits which were in good enough shape to be submitted, but I now think this one wasn't ready.

[ehem_freebsd_m5p.com]

Placing this back in the queue. I dislike proposing this without D30236 in place, since otherwise this looks like a bizarre commit.

[ehem_freebsd_m5p.com]

Addressing @royger's comment. The removal of the lapic_eoi() call had been in a distinct commit, but it does make sense to move here.

Question now is the intr_execute_handlers() function. That is presently part of a creating sys/x86/xen/xen_arch_intr.c commit, but that one might be worth breaking up.

[ehem_freebsd_m5p.com]

Reverting making the function fully match driver_filter_t. I believe that was incorrect and simply need the return to be appropriate.

[ehem_freebsd_m5p.com]

I'm actually rather unsure of what to do with D30006. I dislike creating a near-trivial wrapper function around xen_intr_handle_upcall() as @julien_xen.org did in the original version of the device-tree code, yet getting the prototype for a new function right has proven tricky. Figure D30006 needs some flavor of change, but I'm unsure what precisely it should look like.

[ehem_freebsd_m5p.com]

Okay, I was getting worried about how despite everything appearing to work, some things didn't look right. Appears there may have been some longer term plans for the development of intrng, but those are still floating out there as vague ideas which haven't been implemented. I suspect the ARM64 (and theoretical RISC-V) implementation may want the prototype to change to match intr_irq_filter_t at some vague future point, but I've got no idea when that would be and appears that portion of intrng was last touched in 2015. For now this is what is needed.

[ehem_freebsd_m5p.com]

In D30006#673978, @royger wrote:

Or maybe we could provide a wrapper for x86 or Arm around the function if the only issue is the prototype.

That is what @julien_xen.org's original implementation did. Since the only real difference is the return (void versus int) it seems better to modify the return to match what ARM64 (and presumably RISC-V) needs. This reduces the need to a cast in the device-tree code as driver_filter_t passes the trap frame as a void *.

Longer term, sys/sys/intr.h has the suspicious intr_irq_filter_t type which I suspect was meant to subsume driver_filter_t. I've got no idea whether anyone plans to push that forward in the near or distant future though.

[ehem_freebsd_m5p.com]

I think I finally spotted the right direction to take this, due to perusing D30937 and D31064 in close proximity.

For x86 this is being directly called from assembly language and thus has to take care of the interrupt issues itself. Notably it needs to increment the counter and called critical_enter()/critical_exit() itself. For ARM64 those are handled by the common interrupt routines and thus all that matters is the return.

As such I think the right course is to push those portions to a wrapper in sys/x86/xen/xen_arch_intr.c. This pushes the final x86-specific portion of xen_intr.c out and the file is completely clean.

I'll need to try a test build before submission, but this finishes the last real wart. There is one remaining issue, but that is pure-ARM64 and not so much core Xen code.

[ehem_freebsd_m5p.com]

Update to the proposed approach. x86 now has a small wrapper around xen_intr_handle_upcall() which handles the extra bits (ARM64 has some of this in the kernel core). This appears to be a good solution, particularly since it takes care of the interrupt counters issue.

[ehem_freebsd_m5p.com]

Note to reviewers/testers. This was confirmed to compile for x86, but hasn't been further tested. I would expect the compiler to catch mistakes introduced during conversion, but this needs some checking for x86.

[ehem_freebsd_m5p.com]

Spot during review this was goofed in a non-compiler detectable way.

[ehem_freebsd_m5p.com]

Tiny tweak after checking information on these macros.

[ehem_freebsd_m5p.com]

Appears xenpci.c already had a wrapper similar to what xen-dt.c initially had. Replacing 2 trivial wrappers with a non-trivial function for x86 seems like a win. Switch to actual driver_filter_t as that IS what is being matched.

[ehem_freebsd_m5p.com]

Now with D32346 I've got an unfortunate suspicion there is something more needing adjustment here.

[ehem_freebsd_m5p.com]

Due to exploring elsewhere the issue of the struct trapframe has shown. I'm uncertain how to handle correctly. In its present form, this should behave identically to the previous version. Issue is there is a problem with the present form and I'm wondering whether the present form is correct.

The behavior of passing the trap frame as the argument is undocumented behavior for bus_setup_intr(). I've also encountered several mentions of peripheral interrupt controllers passing NULL trap frames.

In this light perhaps interrupts originating from the xenpci code should see a NULL curthread->td_intr_frame. On the flip side, since a valid one is passed in it could also be propagated.

With that, two approaches come to mind:

1. The existing code could be preserved. This would mean at the point when intr_event_handle()'s misfeature of passing the trap frame if the argument is NULL is removed, the xenpci code would start behaving as if having a peripheral interrupt controller.
2. xen_arch_intr_handle_upcall() should be modified to set curthread->td_intr_frame = trap_frame; before calling xen_intr_handle_upcall(NULL); and then set curthread->td_intr_frame = NULL; after. xen_intr_handle_upcall() should call xen_arch_intr_execute_handlers(isrc, curthread->td_intr_frame).

I don't know which option to choose. Note, I believe option 2 would be correct as xen_arch_intr_handle_upcall() is being called directly from the assembly-language interrupt handler (so losing the old value of curthread->td_intr_frame is correct behavior).

sys/dev/xen/xenpci/xenpci.c

74 ↗

(On Diff #96300)

Here we have one of the users of the "feature" brought up in D32346.

sys/xen/xen_intr.c

410 ↗

(On Diff #96300)

Here there is a big question. What should be being used for the trap_frame?

[ehem_freebsd_m5p.com]

One other note, I believe this causes the Xen interrupt counter to not include the Xen PCI emulated interrupts.

[mhorne]

In D30006#731339, @ehem_freebsd_m5p.com wrote:

Due to exploring elsewhere the issue of the struct trapframe has shown. I'm uncertain how to handle correctly. In its present form, this should behave identically to the previous version. Issue is there is a problem with the present form and I'm wondering whether the present form is correct.

The behavior of passing the trap frame as the argument is undocumented behavior for bus_setup_intr(). I've also encountered several mentions of peripheral interrupt controllers passing NULL trap frames.

Any pointers to one?

In this light perhaps interrupts originating from the xenpci code should see a NULL curthread->td_intr_frame. On the flip side, since a valid one is passed in it could also be propagated.

I don't think they would see NULL. Since xenpci handler is registered via BUS_SETUP_INTR(), it will be invoked by the x86 interrupt framework, and td_intr_frame will be updated in intr_event_handle(). Conversely, xen_arch_intr_handle_upcall() invoked directly from the asm IDT vector would have a NULL/outdated value for td_intr_frame.

With that, two approaches come to mind:

1. The existing code could be preserved. This would mean at the point when intr_event_handle()'s misfeature of passing the trap frame if the argument is NULL is removed, the xenpci code would start behaving as if having a peripheral interrupt controller.
2. xen_arch_intr_handle_upcall() should be modified to set curthread->td_intr_frame = trap_frame; before calling xen_intr_handle_upcall(NULL); and then set curthread->td_intr_frame = NULL; after. xen_intr_handle_upcall() should call xen_arch_intr_execute_handlers(isrc, curthread->td_intr_frame).

I don't know which option to choose. Note, I believe option 2 would be correct as xen_arch_intr_handle_upcall() is being called directly from the assembly-language interrupt handler (so losing the old value of curthread->td_intr_frame is correct behavior).

You are close with #2, but you need to save and restore the previous value in curthread->td_intr_frame, to properly return from a nested interrupt.

sys/dev/xen/xenpci/xenpci.c

62 ↗

(On Diff #96300)

Here is where you might obtain a trapframe from curthread->td_intr_frame, which could be passed to xen_intr_handle_upcall(). Since this handler is called by the interrupt framework (not direct from asm), we know that the value will be set and up to date. Please do this as a separate review from this one.

74 ↗

(On Diff #96300)

Indeed. Which bus does the xenpci device attach to, xenpv?

sys/xen/xen_intr.c

91 ↗

(On Diff #96300)

Why is this being removed?

410 ↗

(On Diff #96300)

It looks like it's doing the right thing by passing trap_frame.

[ehem_freebsd_m5p.com]

@mhorne I think you got mixed up on the direction this is going.

Presently there are two paths which lead to xen_intr_handle_upcall():

1. apic_vector.S (x86 assembly/raw interrupt vectors) => xen_intr.c/xen_intr_handle_upcall()
2. xenpci.c/BUS_SETUP_INTR() => xenpci.c/xenpci_intr_filter() => xen_intr.c/xen_intr_handle_upcall()

I want to rearrange this sequence to add a third caller:

1. apic_vector.S (interrupt) => x86/xen_arch_intr.c/xen_arch_intr_handle_upcall() => xen_intr.c/xen_intr_handle_upcall()
2. xenpci.c/BUS_SETUP_INTR() => xen_intr.c/xen_intr_handle_upcall()
3. arm64/xen_arch_intr.c/bus_setup_intr() (D29875) => xen_intr.c/xen_intr_handle_upcall()

There are two reasons for changing like this. First, some bits of x86-specific code are still in xen_intr_handle_upcall(); notably the interrupt counter increment, critical_enter()/critical_exit() calls and lapic_eoi() call. Second, in theory a device_filter_t function has the capability to report stray interrupts, but any such reporting is lost with the present signature of xen_intr_handle_upcall().

One difference which is observable is previously interrupts from xenpci were included in the Xen/x86 interrupt counts, after this they won't be.

In D30006#731640, @mhorne wrote:

In D30006#731339, @ehem_freebsd_m5p.com wrote:

Due to exploring elsewhere the issue of the struct trapframe has shown. I'm uncertain how to handle correctly. In its present form, this should behave identically to the previous version. Issue is there is a problem with the present form and I'm wondering whether the present form is correct.

The behavior of passing the trap frame as the argument is undocumented behavior for bus_setup_intr(). I've also encountered several mentions of peripheral interrupt controllers passing NULL trap frames.

Any pointers to one?

I've run across them in passing. I'll note the next time if I encounter one.

sys/dev/xen/xenpci/xenpci.c

62 ↗

(On Diff #96300)

Except I was trying to get rid of this function.

74 ↗

(On Diff #96300)

I'm not 100% certain as I haven't tried it. Looks like on x86 with HVM it is presented as a PCI device, I'm unsure how it shows in PVH mode. (partially I was taking a look since there are some patches to Xen to offer the feature on ARM, IOMMU will be needed)

sys/x86/xen/xen_arch_intr.c

85 ↗

(On Diff #96300)

Approach #2 is here set curthread->td_intr_frame = trap_frame; (which is passed from assembly language). Then after the call set curthread->td_intr_frame = NULL;. I think this is right, but I would need a review...

sys/xen/xen_intr.c

91 ↗

(On Diff #96300)

This is not being removed, it is moving to x86/xen/xen_arch_intr.c. This appears to be how the interrupt counters are handled on x86 (not ARM). As such this is moving to the x86 side.

[mhorne]

In D30006#731742, @ehem_freebsd_m5p.com wrote:

@mhorne I think you got mixed up on the direction this is going.

Presently there are two paths which lead to xen_intr_handle_upcall():

1. apic_vector.S (x86 assembly/raw interrupt vectors) => xen_intr.c/xen_intr_handle_upcall()
2. xenpci.c/BUS_SETUP_INTR() => xenpci.c/xenpci_intr_filter() => xen_intr.c/xen_intr_handle_upcall()

I want to rearrange this sequence to add a third caller:

1. apic_vector.S (interrupt) => x86/xen_arch_intr.c/xen_arch_intr_handle_upcall() => xen_intr.c/xen_intr_handle_upcall()
2. xenpci.c/BUS_SETUP_INTR() => xen_intr.c/xen_intr_handle_upcall()
3. arm64/xen_arch_intr.c/bus_setup_intr() (D29875) => xen_intr.c/xen_intr_handle_upcall()

There are two reasons for changing like this. First, some bits of x86-specific code are still in xen_intr_handle_upcall(); notably the interrupt counter increment, critical_enter()/critical_exit() calls and lapic_eoi() call. Second, in theory a device_filter_t function has the capability to report stray interrupts, but any such reporting is lost with the present signature of xen_intr_handle_upcall().

Yep, this is as I understood it.

sys/x86/xen/xen_arch_intr.c

85 ↗

(On Diff #96300)

Yep, agreed that this is where you ought to set it. But as I said, you need to save and restore the previous value of td_intr_frame, not set it to NULL.

[ehem_freebsd_m5p.com]

sys/x86/xen/xen_arch_intr.c

85 ↗

(On Diff #96300)

Okay, I can believe that. I cringe at how this means it will be saved both here and in intr_event_handle(), but that could well be correct. I was thinking setting it to NULL might be correct at this point the interrupt is returning and there shouldn't be further trap frames on the stack.

[ehem_freebsd_m5p.com]

Updating to match current believed to be correct situation. Adjusted to make use of curthread, instead of passing the trap frame as argument (except from asm).

[ehem_freebsd_m5p.com]

sys/x86/xen/xen_arch_intr.c

96–97 ↗

(On Diff #96776)

Note to reviewers (@royger) this is the really significant delta. Previously this would be done if xen_intr_handle_upcall() was invoked for the XenPCI code, but now this will only be invoked for the main Xen interrupt vector. This seems likely correct, but is worthy of reviewer attention.

sys/xen/xen_intr.h

41–42

This really describes how driver_filter_t functions are, I suspect this comment is unnecessary.

[ehem_freebsd_m5p.com]

Keeping synchronized. Reduced the one comment. I'm now suspecting modifying td_intr_nesting_level is incorrect, but I need a reviewer for that as I'm insufficiently familiar.

[ehem_freebsd_m5p.com]

To summarize the situation in a different way. xen_intr_handle_upcall() had been implemented as the assembly-language entry point (called from apic_vector.S). Both xenpci.c and xen-dt.c were creating wrappers around xen_intr_handle_upcall() in order to call it as a driver_filter_t as a device interrupt.

This had two problematic consequences. First, xen_intr_handle_upcall() needed to match the assembly-language interrupt interface. Second, in order to properly function for the interrupt interface some x86-specific calls were required.

By wrapping in the opposite direction xen_arch_intr_handle_upcall() becomes the assembly language interface. This allows moving the x86-specific calls there. xen_intr_handle_upcall() then matches driver_filter_t and the more common interface no longer needs extra wrappers.

sys/x86/xen/xen_arch_intr.c

87 ↗

(On Diff #99020)

I'm now guessing this (and the decrement 7 lines below) shouldn't be done. I'm not familiar enough with this to confidently make the call and need advice for whether this is correct.

[ehem_freebsd_m5p.com]

Updating to ensure this is consistent with the state of my tree. This has been moved earlier, before D30909.

[ehem_freebsd_m5p.com]

@cperciva could I persuade you to try testing D30006? I'm cautiously optimistic it does in fact work for x86, but the problem is I'm setup for testing ARM64, not x86.

[ehem_freebsd_m5p.com]

This needs moving earlier since it needs to alter xen_intr_handle_upcall()'s declaration.

One issue of concern, with this level of transformation how weak has the copyright chain become? What ends up in xen_intrcnt_init() is rather similar to zillions of other instances in FreeBSD. While xen_arch_intr_handle_upcall() is a collage of single-lines from xen_intr_handle_upcall().

[cperciva]

In D30006#841936, @ehem_freebsd_m5p.com wrote:

@cperciva could I persuade you to try testing D30006? I'm cautiously optimistic it does in fact work for x86, but the problem is I'm setup for testing ARM64, not x86.

Sorry, now that EC2 has moved on I don't have any Xen systems any more...