diff mbox series

[v8,05/33] x86/traps: add external_interrupt() to dispatch external interrupts

Message ID 20230410081438.1750-6-xin3.li@intel.com
State New
Headers
Series x86: enable FRED for x86-64 |

Commit Message

Li, Xin3 April 10, 2023, 8:14 a.m. UTC 
  From: "H. Peter Anvin (Intel)" <hpa@zytor.com>

Add external_interrupt() to dispatch external interrupts to their handlers.

If an external interrupt is a system interrupt, dipatch it through
system_interrupt_handlers table, otherwise to dispatch_common_interrupt().

Signed-off-by: H. Peter Anvin (Intel) <hpa@zytor.com>
Co-developed-by: Xin Li <xin3.li@intel.com>
Tested-by: Shan Kang <shan.kang@intel.com>
Signed-off-by: Xin Li <xin3.li@intel.com>
---

Changes since v5:
* Initialize system_interrupt_handlers with dispatch_table_spurious_interrupt()
  instead of NULL to get rid of a branch (Peter Zijlstra).
---
 arch/x86/kernel/traps.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

Comments

Thomas Gleixner June 5, 2023, 11:56 a.m. UTC | #1 
On Mon, Apr 10 2023 at 01:14, Xin Li wrote:
> Add external_interrupt() to dispatch external interrupts to their handlers.
>
> If an external interrupt is a system interrupt, dipatch it through
> system_interrupt_handlers table, otherwise to
> dispatch_common_interrupt().

This naming convention sucks. external interrupts which can be system
interrupts. Come on.

> +/*
> + * External interrupt dispatch function.
> + *
> + * Until/unless dispatch_common_interrupt() can be taught to deal with the
> + * special system vectors, split the dispatch.

More gibberish. It's not useful to add your sloppy personal notes which
are not understandable for anyone else. That comment might eventually
make some sense right in the code where the condition is.

> + * Note: dispatch_common_interrupt() already deals with IRQ_MOVE_CLEANUP_VECTOR.
> + */
> +int external_interrupt(struct pt_regs *regs)
> +{
> +	unsigned int vector = regs->vector;
> +	unsigned int sysvec = vector - FIRST_SYSTEM_VECTOR;
> +
> +	if (vector < FIRST_EXTERNAL_VECTOR) {

unlikely(...)

> +		pr_err("invalid external interrupt vector %d\n", vector);
> +		return -EINVAL;
> +	}
> +
> +	if (sysvec < NR_SYSTEM_VECTORS)
> +		system_interrupt_handlers[sysvec](regs);
> +	else
> +		dispatch_common_interrupt(regs, vector);

How is this supposed to work once the vector space gets extended in a
later version of FRED?

Can we please think about this _now_ and not rewrite all of this two
years down the road? 

Even if that's not fully specified yet, here is the obvious question:

 What are we going to do with the system vectors. Are they going to
 stay just in the middle of the expanded vector space?

That would be completely non-sensical as we'd end up with yet another
segmentation of the vector space.

So the obvious solution is to segment the vector space in the following
way:

  0  - 31   Exceptions/traps        - Cannot be moved
 32         IRQ_MOVE_CLEANUP_VECTOR
 33  - X    System vectors including APIC_SPURIOUS
 X+1 - MAX  External interrupts

This spares the IRQ_MOVE_CLEANUP_VECTOR hackery. It requires to move the
ISA vectors, but that's not rocket science.

That makes the external interrupt vector space trivially expandable, no?

Coming back to that comment:

> + * Until/unless dispatch_common_interrupt() can be taught to deal with the
> + * special system vectors, split the dispatch.

That's simply wishful thinking. Because all what this would achieve is
moving the condition and table lookup into dispatch_common_interrupt().

What's the win aside of convoluted code?

There are only two options to deal with that:

    1) Have the condition in external_interrupt()

       if (unlikely(vector < LEGACY_MAX_EXCEPTION_VECTORS))
            yell_and_bail();

       if (vector < FIRST_DEVICE_VECTOR)
            sysvec_handler[vector](regs)
       else
            fred_common_interrupt(regs, vector);

    2) Make the sysvec_handler[] fred wrapper functions take the vector
       argument and allocate the sysvec_handler[] array dynamically
       sized based on the maximum number of supported vectors in a
       particular [FRED]APIC implementation.

       Not sure whether that's worth it as FRED allows up to 64k vectors
       if I understand the reserved space layout in the spec correctly
       and that would cost 512k memory just for the table.

Why has all of the above not been thought through and addressed _before_
posting this pile?

<Kernel development educational template #11>

  1) Implementing a Prove of Concept for early validation is perfectly
     fine.

  2) Trying to sell that PoC upstream by polishing it a bit is doomed.

  3) The proper tools for upstream development are brain and taste, not
     duct tape and stapler.

</Kernel development educational template #11>

Thanks,

        tglx
Thomas Gleixner June 5, 2023, 5:52 p.m. UTC | #2 
On Mon, Jun 05 2023 at 13:56, Thomas Gleixner wrote:
> On Mon, Apr 10 2023 at 01:14, Xin Li wrote:
> How is this supposed to work once the vector space gets extended in a
> later version of FRED?
>
> Can we please think about this _now_ and not rewrite all of this two
> years down the road? 
>
> Even if that's not fully specified yet, here is the obvious question:
>
>  What are we going to do with the system vectors. Are they going to
>  stay just in the middle of the expanded vector space?
>
> That would be completely non-sensical as we'd end up with yet another
> segmentation of the vector space.
>
> So the obvious solution is to segment the vector space in the following
> way:
>
>   0  - 31   Exceptions/traps        - Cannot be moved
>  32         IRQ_MOVE_CLEANUP_VECTOR
>  33  - X    System vectors including APIC_SPURIOUS
>  X+1 - MAX  External interrupts
>
> This spares the IRQ_MOVE_CLEANUP_VECTOR hackery. It requires to move the
> ISA vectors, but that's not rocket science.

Which we just discussed completely away. :)

> That makes the external interrupt vector space trivially expandable, no?

So there is a theoretical problem here that device interrupts could
starve system vectors due to the priority scheme. Needs some thought.

That whole APIC priority muck is pretty useless as long as CR8 writes
are slower than sti/cli. When I tested that last (Broadwell) they were
significantly slower.

Also it's unclear how that expansion vector space is handled
vs. priorities.

Ideally event delivery would be FIFO because that's the only guarantee
for preventing starvation without having to configure priorities (which
is mostly a wrong guess anyway).

Thanks,

        tglx
Li, Xin3 June 19, 2023, 7:16 p.m. UTC | #3 
> > Add external_interrupt() to dispatch external interrupts to their handlers.
> >
> > If an external interrupt is a system interrupt, dipatch it through
> > system_interrupt_handlers table, otherwise to
> > dispatch_common_interrupt().
> 
> This naming convention sucks. external interrupts which can be system
> interrupts. Come on.

This name dispatch_common_interrupt() comes from arch/x86/kernel/irq.c:

/*
 * common_interrupt() handles all normal device IRQ's (the special SMP
 * cross-CPU interrupts have their own entry points).
 */
DEFINE_IDTENTRY_IRQ(common_interrupt)

Should we rename it to device_intertupt()?

Thanks!
  Xin
Thomas Gleixner June 19, 2023, 9:13 p.m. UTC | #4 
On Mon, Jun 19 2023 at 19:16, Li, Xin3 wrote:
>> > Add external_interrupt() to dispatch external interrupts to their handlers.
>> >
>> > If an external interrupt is a system interrupt, dipatch it through
>> > system_interrupt_handlers table, otherwise to
>> > dispatch_common_interrupt().
>> 
>> This naming convention sucks. external interrupts which can be system
>> interrupts. Come on.
>
> This name dispatch_common_interrupt() comes from arch/x86/kernel/irq.c:

That's not the point. Your changelog says:

  If an external interrupt is a system interrupt...

It's either an external interrupt which goes through common_interrupt()
or it is a system interrupt which goes through it's very own handler,
no?

Thanks,

        tglx
Li, Xin3 June 20, 2023, 12:16 a.m. UTC | #5 
> That's not the point. Your changelog says:
> 
>   If an external interrupt is a system interrupt...
> 
> It's either an external interrupt which goes through common_interrupt()
> or it is a system interrupt which goes through it's very own handler,
> no?

Ah, then it looks more of a problem in the way how I described it.

What I wanted to describe is the dispatch logic _inside_ the new function
external_interrupt(), what about:

external_interrupt() dispatches all external interrupts: it checks if an
external interrupt is a system interrupt, if yes it dipatches it through
the system_interrupt_handlers table, otherwise to dispatch_common_interrupt().

Thanks!
  Xin
diff mbox series

Patch

diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index 12072e2af4a6..f86cd233b00b 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -1511,6 +1511,32 @@  static system_interrupt_handler system_interrupt_handlers[NR_SYSTEM_VECTORS] = {
 
 #undef SYSV
 
+/*
+ * External interrupt dispatch function.
+ *
+ * Until/unless dispatch_common_interrupt() can be taught to deal with the
+ * special system vectors, split the dispatch.
+ *
+ * Note: dispatch_common_interrupt() already deals with IRQ_MOVE_CLEANUP_VECTOR.
+ */
+int external_interrupt(struct pt_regs *regs)
+{
+	unsigned int vector = regs->vector;
+	unsigned int sysvec = vector - FIRST_SYSTEM_VECTOR;
+
+	if (vector < FIRST_EXTERNAL_VECTOR) {
+		pr_err("invalid external interrupt vector %d\n", vector);
+		return -EINVAL;
+	}
+
+	if (sysvec < NR_SYSTEM_VECTORS)
+		system_interrupt_handlers[sysvec](regs);
+	else
+		dispatch_common_interrupt(regs, vector);
+
+	return 0;
+}
+
 #endif /* CONFIG_X86_64 */
 
 void __init install_system_interrupt_handler(unsigned int n, const void *asm_addr, const void *addr)