On 20.11.2025 10:58, Roger Pau Monne wrote: > Attempt to clarify the purpose of setup_ioapic_dest(), as the comment ahead > of it is outdated, and looks to be a verbatim copy from Linux from one of > the code imports. > > The function serves two purposes: shuffling the interrupts across CPUs > after SMP bringup or re-assigning all interrupts to CPU#0 if no IRQ > balancing is set at run time. However the function won't perform any of > those functions correctly, as it was unconditionally using > desc->arch.cpu_mask as the target CPU mask for interrupts (which is the > current target anyway). > > Fix by adding a new `shuffle` parameter that's used to signal whether the > function is intended to balance interrupts across CPUs, or to re-assign all > interrupts to the BSP. > > Signed-off-by: Roger Pau Monné <[email protected]>
Reviewed-by: Jan Beulich <[email protected]> > --- > I couldn't find a specific Fixes tag to use here, I think this has been > broken all along. Perhaps dddd88c891af ("Auto-disable IRQ balancing/affinity on buggy chipsets"), which is where the 2nd use of the function was introduced? > --- a/xen/arch/x86/io_apic.c > +++ b/xen/arch/x86/io_apic.c > @@ -717,12 +717,14 @@ static int __init find_isa_irq_apic(int irq, int type) > static int pin_2_irq(int idx, int apic, int pin); > > /* > - * This function currently is only a helper for the i386 smp boot process > where > - * we need to reprogram the ioredtbls to cater for the cpus which have come > online > - * so mask in all cases should simply be TARGET_CPUS > + * This function serves two different purposes: shuffling the IO-APIC > + * interrupts across CPUs after SMP bringup, or re-assigning all interrupts > to > + * the BSP if IRQ balancing is disabled at runtime. Such functional > + * distinction is signaled by the `shuffle` parameter. > */ > -void /*__init*/ setup_ioapic_dest(void) > +void setup_ioapic_dest(bool shuffle) > { > + const cpumask_t *mask = shuffle ? TARGET_CPUS : cpumask_of(0); Don't we want to aim at getting rid of TARGET_CPUS, which is now only hiding something invariant? IOW should we perhaps avoid introducing new uses? Jan
