Re: [PATCH 07/12] x86/io-apic: fix usage of setup_ioapic_dest()

[Jan Beulich <jbeulich@xxxxxxxx>]

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é <roger.pau@xxxxxxxxxx>

Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

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