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

["Roger Pau Monné" <roger.pau@citrix.com>]

On Mon, Oct 27, 2025 at 12:53:34PM +0100, 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. I'll go
> with cpumask_of(0).

Yeah, that's what I fear also.  It's in principle possible for
the cpu_mask to be empty, but since this targets 4.21 I think it's too
risky.  Using cpumask_of(0) is fine.  Please keep my RB.

Thanks, Roger.