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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 24.10.2023 13:36, Roger Pau Monné wrote:
> 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.

>From my pov the main badness with this is pre-existing: The wrapping of
a complex expression in !(...). The replacement of the prior plain
smp_processor_id() isn't much of a problem imo.

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

bogus_8259A_irq() shouldn't be concerned about the CPU it runs on; it
should solely deal with 8259A aspects.

Jan