Re: [PATCH v3] x86/i8259: do not assume interrupts always target CPU0

[Jan Beulich <jbeulich@xxxxxxxx>]

On 24.10.2023 16:53, Roger Pau Monne wrote:
> Sporadically we have seen the following during AP bringup on AMD platforms
> only:
> 
> microcode: CPU59 updated from revision 0x830107a to 0x830107a, date = 
> 2023-05-17
> microcode: CPU60 updated from revision 0x830104d to 0x830107a, date = 
> 2023-05-17
> CPU60: No irq handler for vector 27 (IRQ -2147483648)
> microcode: CPU61 updated from revision 0x830107a to 0x830107a, date = 
> 2023-05-17
> 
> This is similar to the issue raised on Linux commit 36e9e1eab777e, where they
> observed i8259 (active) vectors getting delivered to CPUs different than 0.
> 
> On AMD or Hygon platforms adjust the target CPU mask of i8259 interrupt
> descriptors to contain all possible CPUs, so that APs will reserve the vector
> at startup if any legacy IRQ is still delivered through the i8259.  Note that
> if the IO-APIC takes over those interrupt descriptors the CPU mask will be
> reset.
> 
> Spurious i8259 interrupt vectors however (IRQ7 and IRQ15) can be injected even
> when all i8259 pins are masked, and hence would need to be handled on all 
> CPUs.
> 
> Continue to reserve PIC vectors on CPU0 only, but do check for such spurious
> interrupts on all CPUs if the vendor is AMD or Hygon.  Note that once the
> vectors get used by devices detecting PIC spurious interrupts will no longer 
> be
> possible, however the device driver should be able to cope with spurious
> interrupts.  Such PIC spurious interrupts occurring when the vector is in use
> by a local APIC routed source will lead to an extra EOI, which might
> unintentionally clear a different vector from ISR.  Note this is already the
> current behavior, so assume it's infrequent enough to not cause real issues.
> 
> Finally, adjust the printed message to display the CPU where the spurious
> interrupt has been received, so it looks like:
> 
> microcode: CPU1 updated from revision 0x830107a to 0x830107a, date = 
> 2023-05-17
> cpu1: spurious 8259A interrupt: IRQ7
> microcode: CPU2 updated from revision 0x830104d to 0x830107a, date = 
> 2023-05-17
> 
> Amends: 3fba06ba9f8b ('x86/IRQ: re-use legacy vector ranges on APs')
> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
with one nit (which I think can be taken care of when committing):

> --- a/xen/arch/x86/irq.c
> +++ b/xen/arch/x86/irq.c
> @@ -1920,7 +1920,16 @@ void do_IRQ(struct cpu_user_regs *regs)
>                  kind = "";
>              if ( !(vector >= FIRST_LEGACY_VECTOR &&
>                     vector <= LAST_LEGACY_VECTOR &&
> -                   !smp_processor_id() &&
> +                   (!smp_processor_id() ||
> +                    /*
> +                     * For AMD/Hygon do spurious PIC interrupt
> +                     * detection on all CPUs, as it has been observed
> +                     * that during unknown circumstances spurious PIC
> +                     * interrupts have been delivered to CPUs
> +                     * different than the BSP.
> +                     */
> +                   (boot_cpu_data.x86_vendor & (X86_VENDOR_AMD |
> +                                                X86_VENDOR_HYGON))) &&

Afaict these two lines need indenting by one more blank, to account
for the parentheses enclosing the || operands.

Jan

>                     bogus_8259A_irq(vector - FIRST_LEGACY_VECTOR)) )
>              {
>                  printk("CPU%u: No irq handler for vector %02x (IRQ %d%s)\n",