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: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).

>>>> --- a/xen/drivers/passthrough/amd/iommu_intr.c
>>>> +++ b/xen/drivers/passthrough/amd/iommu_intr.c
>>>> @@ -551,6 +551,13 @@ int cf_check amd_iommu_msi_msg_update_ir
>>>>          for ( i = 1; i < nr; ++i )
>>>>              msi_desc[i].remap_index = msi_desc->remap_index + i;
>>>>          msg->data = data;
>>>> +        /*
>>>> +         * While the low address bits don't matter, "canonicalize" the 
>>>> address
>>>> +         * by zapping the bits that were transferred to the IRTE.  This 
>>>> way
>>>> +         * callers can check for there actually needing to be an update to
>>>> +         * wherever the address is put.
>>>> +         */
>>>> +        msg->address_lo &= ~(MSI_ADDR_DESTMODE_MASK | 
>>>> MSI_ADDR_DEST_ID_MASK);
>>>
>>> You might want to mention this change on the commit message also, as
>>> it could look unrelated to the rest of the code?
>>
>> I thought the comment here provided enough context and detail. I've added
>> "AMD interrupt remapping code so far didn't "return" a consistent MSI
>>  address when translating an MSI message. Clear respective fields there, to
>>  keep the respective assertion in set_channel_irq_affinity() from
>>  triggering."
> 
> LGTM, I would possibly remove the last "respective" for being
> repetitive given the previous one in the sentence.

Oh, indeed. Replaced it by "related" rather than dropping it completely.

Jan