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.
