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

[Jan Beulich <jbeulich@xxxxxxxx>]

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@xxxxxxxxxx>
>>
>> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
>> 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