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
