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é <[email protected]>
>
> Reviewed-by: Jan Beulich <[email protected]>
> 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.
