Re: [PATCH 04/12] x86/irq: set accurate cpu_mask for high priority vectors used by external interrupts

["Roger Pau Monné" <roger.pau@citrix.com>]

On Thu, Nov 20, 2025 at 02:06:39PM +0100, Jan Beulich wrote:
> On 20.11.2025 10:06, Roger Pau Monne wrote:
> > Setting the irq descriptor target CPU mask of high priority interrupts to
> > contain all online CPUs is not accurate.  External interrupts are
> > exclusively delivered using physical destination mode, and hence can only
> > target a single CPU.  Setting the descriptor CPU mask to contain all online
> > CPUs makes it impossible for Xen to figure out which CPU the interrupt is
> > really targeting.
> > 
> > Instead handle high priority vectors used by external interrupts similarly
> > to normal vectors, keeping the target CPU mask accurate.  Introduce
> > specific code in _assign_irq_vector() to deal with moving high priority
> > vectors across CPUs, this is needed at least for fixup_irqs() to be able to
> > evacuate those if the target CPU goes offline.
> > 
> > Fixes: fc0c3fa2ad5c ("x86/IO-APIC: fix setup of Xen internally used IRQs (take 2)")
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> with one further request:
> 
> > @@ -756,12 +770,10 @@ void setup_vector_irq(unsigned int cpu)
> >          if ( !irq_desc_initialized(desc) )
> >              continue;
> >          vector = irq_to_vector(irq);
> > -        if ( vector >= FIRST_HIPRIORITY_VECTOR &&
> > -             vector <= LAST_HIPRIORITY_VECTOR )
> > -            cpumask_set_cpu(cpu, desc->arch.cpu_mask);
> > -        else if ( !cpumask_test_cpu(cpu, desc->arch.cpu_mask) )
> > -            continue;
> > -        per_cpu(vector_irq, cpu)[vector] = irq;
> > +        if ( (vector >= FIRST_HIPRIORITY_VECTOR &&
> > +              vector <= LAST_HIPRIORITY_VECTOR) ||
> > +             cpumask_test_cpu(cpu, desc->arch.cpu_mask) )
> > +            per_cpu(vector_irq, cpu)[vector] = irq;
> 
> Going beyond desc->arch.cpu_mask for hiprio vectors may deserve a comment here. When
> the vector is global, this is necessary. But for e.g. the serial IRQ (which still
> moves, but isn't bound to multiple CPUs, the more normal way of respecting
> desc->arch.cpu_mask would be sufficient.

Note that hiprio vectors are handled specially in _assign_irq_vector()
and the logic to set per_cpu(vector_irq, cpu)[vector] is
short-circuited assuming that hiprio vectors are already setup on all
CPUs.

> It is merely (largely) benign if we set
> vector_irq[] also for other CPUs. "Largely" because strictly speaking if that vector
> triggered on the wrong CPU for whatever reason, we rather shouldn't treat it as a
> legitimate interrupt.

I see, yes, we could handle hiprio vectors more like normal ones and
do a bit more work in _assign_irq_vector() to also set the
vector_irq[] array at bind time, but then we would need the cleanup
logic as the interrupt migrates, which is a bit of overhead when we
know the vector won't be re-used for anything else.

What about I add the following comment:

        if ( cpumask_test_cpu(cpu, desc->arch.cpu_mask) ||
             /*
              * High-priority vectors are reserved on all CPUs.  Set
              * the vector to IRQ assignment on all CPUs, even if the
              * interrupt is not targeting this CPU.  That makes
              * shuffling those interrupts around CPUs easier.
              */
             (vector >= FIRST_HIPRIORITY_VECTOR &&
              vector <= LAST_HIPRIORITY_VECTOR) )
            per_cpu(vector_irq, cpu)[vector] = irq;

Thanks, Roger.