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.

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.

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?

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.

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.

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

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).

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

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.

@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.

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.

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.

@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.

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().

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...