sys: add trap frame compatibility macros

[ehem_freebsd_m5p.com]

Summary

Some portions of FreeBSD were written to pass the interrupt trap frame
as an argument. Some portions use the global curthread->td_intr_frame.
Create some macros to allow merging code to use only one approach.

[ehem_freebsd_m5p.com]

I really need some reviewers for this, but I'm unsure whom to ask for this one.

There is a rather gnarly issue here. There are two distinct APIs for interrupts and the trapframe structure. Luckily they don't conflict with each other, but the FreeBSD engineering team really should choose one direction to sort this out.

First way to get the current trap frame is via the global variable, curthread->td_intr_frame.

Second way to get the current trap frame is during interrupt setup pass NULL as the function context and intr_event_handle() will pass the trap frame as the argument.

To make the second really work, the driver_filter_t type definition should really change to typedef int driver_filter_t(void*, struct trapframe *); which would allow driver_filter_t functions to access both values. Then the context should always be passed as the argument by intr_event_handle(), even if NULL.

Both ways are viable, I'm just very unsure which one is the correct way forward. Really need to decide and start wiping the remains of the other approach away.

[mhorne]

Overall, I don't like the idea of making changes to working code for the benefit of a feature that is incomplete and has never been enabled. IMO any action on INTR_SOLO requires a more thorough set of changes + testing to make it production ready. It would probably be equally fine to remove it at this point.

You are right that the current usage of td_intr_frame is a bit of a mess. If you grep for it, you'll see that its more widespread than this file. I have some work in progress to eliminate this field and just make td_frame work for this, but it requires quite a lot more effort still.

The DISABLE_TD_INTR_FRAME bits are not committable, for one because I can see already that a kernel with this flag defined would fail to compile.

sys/kern/subr_intr.c

328–332

This block seems acceptable to me.

sys/sys/proc.h

371 ↗

(On Diff #96396)

We cannot have optional members of struct thread in the middle. Its layout needs to be the same regardless of kernel configuration options.

[ehem_freebsd_m5p.com]

Nudging D32346 in a slightly different direction. This hopefully works better for starting the untangling process.

[ehem_freebsd_m5p.com]

In D32346#730151, @mhorne wrote:

You are right that the current usage of td_intr_frame is a bit of a mess. If you grep for it, you'll see that its more widespread than this file. I have some work in progress to eliminate this field and just make td_frame work for this, but it requires quite a lot more effort still.

I think it is more than a bit of a mess. I've found most of the spots, but I needed a workable approach to cleaning things up and providing true compatibility.

In D32346#730151, @mhorne wrote:

The DISABLE_TD_INTR_FRAME bits are not committable, for one because I can see already that a kernel with this flag defined would fail to compile.

This was meant to be a variation of "notyet". Mostly aiming to mark places which needed to be modified to make the approach final.

What I just put in is hopefully a better step in the direction of cleaning this up. Mainly these macros should be sufficient to make the two approaches compatible. Then comes the step of modifying lots of functions/declarations.

[ehem_freebsd_m5p.com]

Then during updates find an extra macro is needed in a few places, plus simplies the ifdefs in the kernel code.

[mhorne]

Okay, let's take a step back here.

In D32346#729999, @ehem_freebsd_m5p.com wrote:

I really need some reviewers for this, but I'm unsure whom to ask for this one.

There is a rather gnarly issue here. There are two distinct APIs for interrupts and the trapframe structure. Luckily they don't conflict with each other, but the FreeBSD engineering team really should choose one direction to sort this out.

First way to get the current trap frame is via the global variable, curthread->td_intr_frame.

Second way to get the current trap frame is during interrupt setup pass NULL as the function context and intr_event_handle() will pass the trap frame as the argument.

I agree that having both of these methods to obtain the trapframe is a strange inconsistency. It is not a functional problem, but worth addressing if it is not too intrusive. This is the core of what you are hoping to solve with this change, right?

To make the second really work, the driver_filter_t type definition should really change to typedef int driver_filter_t(void*, struct trapframe *); which would allow driver_filter_t functions to access both values. Then the context should always be passed as the argument by intr_event_handle(), even if NULL.

Both ways are viable, I'm just very unsure which one is the correct way forward. Really need to decide and start wiping the remains of the other approach away.

The current proposed approach is not viable. Annotating every handler in the tree with these transition macros is just unnecessary churn, and following through with such a transition would break out of tree drivers -- after all, driver_filter_t is part of the established driver API, and we cannot change this lightly.

Instead, it would make sense to get an estimate on how many drivers rely on the second method (providing NULL as the argument to bus_setup_intr()), and we can take action based on that. It should be pretty easy to make them use td_intr_frame instead. I also note that this behaviour of passing NULL as arg to get a trapframe is not documented in bus_setup_intr(9).

[ehem_freebsd_m5p.com]

In D32346#730621, @mhorne wrote:

I agree that having both of these methods to obtain the trapframe is a strange inconsistency. It is not a functional problem, but worth addressing if it is not too intrusive. This is the core of what you are hoping to solve with this change, right?

Pretty much. Mostly I was looking at this situation and being puzzled at the way to do things (notably this has major impact on D30006).

In D32346#730621, @mhorne wrote:

Instead, it would make sense to get an estimate on how many drivers rely on the second method (providing NULL as the argument to bus_setup_intr()), and we can take action based on that. It should be pretty easy to make them use td_intr_frame instead. I also note that this behaviour of passing NULL as arg to get a trapframe is not documented in bus_setup_intr(9).

Good news is experimenting with this is valuable for tracking down the instances. It is possible my eyes glazed over at a crucial moment, but there aren't very many users of this. The Xen code though includes several (can you guess why I was getting confused?).

[ehem_freebsd_m5p.com]

Good news is this did allow identifying places where curthread->tf_intr_frame was obtained from the argument, versus places where it was obtained from curthread. Also places which used driver_filter_t versus duplicating the prototype.