Re: [PATCH for-4.21 03/10] x86/HPET: use single, global, low-priority vector for broadcast IRQ

[Jan Beulich <jbeulich@xxxxxxxx>]

On 17.10.2025 10:20, Roger Pau Monné wrote:
> On Fri, Oct 17, 2025 at 09:15:08AM +0200, Jan Beulich wrote:
>> On 16.10.2025 18:27, Roger Pau Monné wrote:
>>> On Thu, Oct 16, 2025 at 09:32:04AM +0200, Jan Beulich wrote:
>>>> @@ -497,6 +503,7 @@ static void set_channel_irq_affinity(str
>>>>      spin_lock(&desc->lock);
>>>>      hpet_msi_mask(desc);
>>>>      hpet_msi_set_affinity(desc, cpumask_of(ch->cpu));
>>>> +    per_cpu(vector_irq, ch->cpu)[HPET_BROADCAST_VECTOR] = ch->msi.irq;
>>>
>>> I would set the vector table ahead of setting the affinity, in case we
>>> can drop the mask calls around this block of code.
>>
>> Isn't there a problematic window either way round? I can make the change,
>> but I don't see that addressing anything. The new comparator value will
>> be written later anyway, and interrupts up to that point aren't of any
>> interest anyway. I.e. it doesn't matter which of the CPUs gets to handle
>> them.
> 
> It's preferable to get a silent stray interrupt (if the per-cpu vector
> table is correctly setup), rather than to get a message from Xen that
> an unknown vector has been received?
> 
> If a vector is injected ahead of vector_irq being set Xen would
> complain in do_IRQ() that that's no handler for such vector.

As of now, setup_vector_irq() makes sure the field isn't uninitialized
(i.e. left at INT_MIN). With that change dropped (see below), there
would indeed be such a risk (on the first instance on each CPU).

>>>> --- a/xen/arch/x86/include/asm/irq-vectors.h
>>>> +++ b/xen/arch/x86/include/asm/irq-vectors.h
>>>> @@ -18,6 +18,15 @@
>>>>  /* IRQ0 (timer) is statically allocated but must be high priority. */
>>>>  #define IRQ0_VECTOR             0xf0
>>>>  
>>>> +/*
>>>> + * Low-priority (for now statically allocated) vectors, sharing entry
>>>> + * points with exceptions in the 0x10 ... 0x1f range, as long as the
>>>> + * respective exception has an error code.
>>>> + */
>>>> +#define FIRST_LOPRIORITY_VECTOR 0x10
>>>> +#define HPET_BROADCAST_VECTOR   X86_EXC_AC
>>>> +#define LAST_LOPRIORITY_VECTOR  0x1f
>>>
>>> I wonder if it won't be clearer to simply reserve a vector if the HPET
>>> is used, instead of hijacking the AC one.  It's one vector less, but
>>> arguably now that we unconditionally use physical destination mode our
>>> pool of vectors has expanded considerably.
>>
>> Well, I'd really like to avoid consuming an otherwise usable vector, if
>> at all possible (as per Andrew's FRED plans, that won't be possible
>> there anymore then).
> 
> If re-using the AC vector is not possible with FRED we might want to
> do this uniformly and always consume a vector then?

Right now it saves us a vector. I'd leave the FRED side for when that's
going to be implemented. Which - aiui - isn't going to be straightforward
anyway, due to (at least) requirements around IRQ_MOVE_CLEANUP_VECTOR.

>>>> --- a/xen/arch/x86/irq.c
>>>> +++ b/xen/arch/x86/irq.c
>>>> @@ -755,8 +755,9 @@ 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 )
>>>> +        if ( vector <= (vector >= FIRST_HIPRIORITY_VECTOR
>>>> +                        ? LAST_HIPRIORITY_VECTOR
>>>> +                        : LAST_LOPRIORITY_VECTOR) )
>>>>              cpumask_set_cpu(cpu, desc->arch.cpu_mask);
>>>
>>> I think this is wrong.  The low priority vector used by the HPET will
>>> only target a single CPU at a time, and hence adding extra CPUs to
>>> that mask as part of AP bringup is not correct.
>>
>> I'm not sure about "wrong". It's not strictly necessary for the HPET one,
>> I expect, but it's generally what would be necessary. For the HPET one,
>> hpet_msi_set_affinity() replaces the value anyway. (I can add a sentence
>> to this effect to the description, if that helps.)
> 
> I do think it's wrong, it's just not harmful per-se apart from showing
> up in the output of dump_irqs().  The value in desc->arch.cpu_mask
> should be the CPU that's the destination of the interrupt.  In this
> case, the HPET interrupt does have a single destination at a give
> time, and adding another one will make the output of dump_irqs() show
> two destinations, when the interrupt will target a single interrupt.

Just that as soon as the interrupt is actually in use, what is done
here doesn't matter anymore.

I continue to think the change is correct for the general case: I'd
expect these special vectors to normally (just not here) be used as
"direct APIC vectors", in which case the IRQ does have multiple
destinations.

Problem is - if I don't make this change, I still expect I ought to
make _some_ change here, as the following "else if()" might be getting
in the way. Then again the vector_irq[] assignment also isn't strictly
needed this early, as set_channel_irq_affinity() deals with that
anyway.

Bottom line - I guess I'll drop this change, realizing that adding
something here later on may then be harder to understand.

> If anything you should add the CPU to the affinity set
> (desc->affinity), but that's not needed since you already init the
> affinity mask with cpumask_setall().

Indeed.

> FWIW, I'm working on tentatively getting rid of the
> desc->arch.{cpu,old_cpu,pending}_mask fields and converting them to
> plain unsigned ints after we have dropped logical interrupt delivery
> for external interrupts.

I'm aware, yes. And I realize this change - for the HPET case - would
be getting in the way (to some degree).

Jan