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>
---
xen/arch/x86/irq.c | 24 ++++++++++++++++++------
xen/arch/x86/smpboot.c | 3 ++-
2 files changed, 20 insertions(+), 7 deletions(-)
diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
index 7009a3c6d0dd..5cd934ea2a32 100644
--- a/xen/arch/x86/irq.c
+++ b/xen/arch/x86/irq.c
@@ -547,6 +547,20 @@ static int _assign_irq_vector(struct irq_desc *desc, const cpumask_t *mask)
cpumask_t tmp_mask;
cpumask_and(&tmp_mask, mask, &cpu_online_map);
+
+ /*
+ * High priority vectors are reserved on all CPUs, hence moving them
+ * just requires changing the target CPU. There's no need for vector
+ * allocation on the destination.
+ */
+ if ( old_vector >= FIRST_HIPRIORITY_VECTOR &&
+ old_vector <= LAST_HIPRIORITY_VECTOR )
+ {
+ cpumask_copy(desc->arch.cpu_mask,
+ cpumask_of(cpumask_any(&tmp_mask)));
+ return 0;
+ }
+
if (cpumask_intersects(&tmp_mask, desc->arch.cpu_mask)) {
desc->arch.vector = old_vector;
return 0;
@@ -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;
}
}
diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c
index 7fab5552335b..69cc9bbc6e2c 100644
--- a/xen/arch/x86/smpboot.c
+++ b/xen/arch/x86/smpboot.c
@@ -1464,7 +1464,8 @@ void __init smp_intr_init(void)
desc = irq_to_desc(irq);
desc->arch.vector = vector;
- cpumask_copy(desc->arch.cpu_mask, &cpu_online_map);
+ cpumask_copy(desc->arch.cpu_mask, cpumask_of(cpu));
+ cpumask_setall(desc->affinity);
}
/* Direct IPI vectors. */
--
2.51.0
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. 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.
Jan
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.
On 21.11.2025 09:29, Roger Pau Monné wrote:
> 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, that's fine with me.
Jan
On 20/11/2025 9:06 am, 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>
Which external interrupts do we have like this?
Looking at Jan's series, the VMX Posted Interrupt vector is like this,
but I can't see a case of getting a high priority vector, and
fixup_irqs() being a legitimate thing to do.
~Andrew
On 20.11.2025 13:45, Andrew Cooper wrote:
> On 20/11/2025 9:06 am, 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>
>
> Which external interrupts do we have like this?
>
> Looking at Jan's series, the VMX Posted Interrupt vector is like this,
> but I can't see a case of getting a high priority vector, and
> fixup_irqs() being a legitimate thing to do.
The serial IRQ is hiprio and wants to be subject to moving around.
Jan
© 2016 - 2025 Red Hat, Inc.