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

[Jan Beulich <jbeulich@xxxxxxxx>]

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).

Jan