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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 27.10.2025 12:53, Jan Beulich wrote:
> On 27.10.2025 12:33, Roger Pau Monné wrote:
>> On Mon, Oct 27, 2025 at 11:23:58AM +0100, Jan Beulich wrote:
>>> On 24.10.2025 15:24, Roger Pau Monné wrote:
>>>> On Thu, Oct 23, 2025 at 05:50:17PM +0200, Jan Beulich wrote:
>>>>> @@ -343,6 +347,12 @@ static int __init hpet_setup_msi_irq(str
>>>>>      u32 cfg = hpet_read32(HPET_Tn_CFG(ch->idx));
>>>>>      irq_desc_t *desc = irq_to_desc(ch->msi.irq);
>>>>>  
>>>>> +    clear_irq_vector(ch->msi.irq);
>>>>> +    ret = bind_irq_vector(ch->msi.irq, HPET_BROADCAST_VECTOR, 
>>>>> &cpu_online_map);
>>>>
>>>> By passing cpu_online_map here, it leads to _bind_irq_vector() doing:
>>>>
>>>> cpumask_copy(desc->arch.cpu_mask, &cpu_online_map);
>>>>
>>>> Which strictly speaking is wrong.  However this is just a cosmetic
>>>> issue until the irq is used for the first time, at which point it will
>>>> be assigned to a concrete CPU.
>>>>
>>>> You could do:
>>>>
>>>> cpumask_clear(desc->arch.cpu_mask);
>>>> cpumask_set_cpu(cpumask_any(&cpu_online_map), desc->arch.cpu_mask);
>>>>
>>>> (Or equivalent)
>>>>
>>>> To assign the interrupt to a concrete CPU and reflex it on the
>>>> cpu_mask after the bind_irq_vector() call, but I can live with it
>>>> being like this.  I have patches to adjust _bind_irq_vector() myself,
>>>> which I hope I will be able to post soon.
>>>
>>> Hmm, I wrongly memorized hpet_broadcast_init() as being pre-SMP-init only.
>>> It has three call sites:
>>> - mwait_idle_init(), called from cpuidle_presmp_init(),
>>> - amd_cpuidle_init(), calling in only when invoked the very first time,
>>>   which is again from cpuidle_presmp_init(),
>>> - _disable_pit_irq(), called from the regular initcall disable_pit_irq().
>>> I.e. for the latter you're right that the CPU mask is too broad (in only a
>>> cosmetic way though). Would be you okay if I used cpumask_of(0) in place
>>> of &cpu_online_map?
>>
>> Using cpumask_of(0) would be OK, as the per-cpu vector_irq array will
>> be updated ahead of assigning the interrupt to a CPU, and hence it
>> doesn't need to be done for all possible online CPUs in
>> _bind_irq_vector().
>>
>> In the context here it would be more accurate to provide an empty CPU
>> mask, as the interrupt is not yet targeting any CPU.  Using CPU 0
>> would be a placeholder, which seems fine for the purpose.
> 
> Putting an empty mask there, while indeed logically correct, would (I fear)
> again put us at risk with other code making various assumptions.

And indeed: _bind_irq_vector() would reject an empty mask.

Jan