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

[Thomas Gleixner <tglx@linutronix.de>]

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