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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Tue, Oct 24, 2023 at 12:51:08PM +0200, Jan Beulich wrote:
> On 24.10.2023 12:14, Roger Pau Monné wrote:
> > On Tue, Oct 24, 2023 at 11:33:21AM +0200, Jan Beulich wrote:
> >> On 23.10.2023 14:46, Roger Pau Monne wrote:
> >>> --- a/xen/arch/x86/i8259.c
> >>> +++ b/xen/arch/x86/i8259.c
> >>> @@ -37,6 +37,15 @@ static bool _mask_and_ack_8259A_irq(unsigned int irq);
> >>>  
> >>>  bool bogus_8259A_irq(unsigned int irq)
> >>>  {
> >>> +    if ( smp_processor_id() &&
> >>> +         !(boot_cpu_data.x86_vendor & (X86_VENDOR_AMD | 
> >>> X86_VENDOR_HYGON)) )
> >>> +        /*
> >>> +         * 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.
> >>> +         */
> >>> +        return false;
> >>> +
> >>>      return !_mask_and_ack_8259A_irq(irq);
> >>>  }
> >>
> >> I don't think this function should be changed; imo the adjustment belongs
> >> at the call site.
> > 
> > It makes the call site much more complex to follow, in fact I was
> > considering pulling the PIC vector range checks into
> > bogus_8259A_irq().  Would that convince you into placing the check here
> > rather than in the caller context?
> 
> Passing a vector and moving the range check into the function is something
> that may make sense. But I'm afraid the same does not apply to the
> smp_processor_id() check, unless the function was also renamed to
> bogus_8259A_vector(). Which in turn doesn't make much sense, to me at
> least, as the logic would better be in terms of IRQs (which is what the
> chip deals with primarily), not vectors (which the chip deals with solely
> during the INTA cycle with the CPU).

The alternative is to use:

            if ( !(vector >= FIRST_LEGACY_VECTOR &&
                   vector <= LAST_LEGACY_VECTOR &&
                   (!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))) &&
                   bogus_8259A_irq(vector - FIRST_LEGACY_VECTOR)) )
            {

Which I find too complex to read, and prone to mistakes by future
modifications.

What is your reasoning for wanting the smp_processor_id() check in
the caller rather than bogus_8259A_irq()?  It does seem fine to me to
do such check in bogus_8259A_irq(), as whether the IRQ is bogus also
depends on whether it fired on the BSP or any of the APs.

Thanks, Roger.