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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 22.10.2025 11:21, Jan Beulich wrote:
> On 21.10.2025 15:49, Roger Pau Monné wrote:
>> On Tue, Oct 21, 2025 at 08:42:13AM +0200, Jan Beulich wrote:
>>> On 20.10.2025 18:22, Roger Pau Monné wrote:
>>>> On Mon, Oct 20, 2025 at 01:18:34PM +0200, Jan Beulich wrote:
>>>>> @@ -476,19 +486,50 @@ static struct hpet_event_channel *hpet_g
>>>>>  static void set_channel_irq_affinity(struct hpet_event_channel *ch)
>>>>>  {
>>>>>      struct irq_desc *desc = irq_to_desc(ch->msi.irq);
>>>>> +    struct msi_msg msg = ch->msi.msg;
>>>>>  
>>>>>      ASSERT(!local_irq_is_enabled());
>>>>>      spin_lock(&desc->lock);
>>>>> -    hpet_msi_mask(desc);
>>>>> -    hpet_msi_set_affinity(desc, cpumask_of(ch->cpu));
>>>>> -    hpet_msi_unmask(desc);
>>>>> +
>>>>> +    per_cpu(vector_irq, ch->cpu)[HPET_BROADCAST_VECTOR] = ch->msi.irq;
>>>>> +
>>>>> +    /*
>>>>> +     * Open-coding a reduced form of hpet_msi_set_affinity() here.  With 
>>>>> the
>>>>> +     * actual update below (either of the IRTE or of [just] message 
>>>>> address;
>>>>> +     * with interrupt remapping message address/data don't change) now 
>>>>> being
>>>>> +     * atomic, we can avoid masking the IRQ around the update.  As a 
>>>>> result
>>>>> +     * we're no longer at risk of missing IRQs (provided 
>>>>> hpet_broadcast_enter()
>>>>> +     * keeps setting the new deadline only afterwards).
>>>>> +     */
>>>>> +    cpumask_copy(desc->arch.cpu_mask, cpumask_of(ch->cpu));
>>>>> +
>>>>>      spin_unlock(&desc->lock);
>>>>>  
>>>>> -    spin_unlock(&ch->lock);
>>>>> +    msg.dest32 = cpu_physical_id(ch->cpu);
>>>>> +    msg.address_lo &= ~MSI_ADDR_DEST_ID_MASK;
>>>>> +    msg.address_lo |= MSI_ADDR_DEST_ID(msg.dest32);
>>>>> +    if ( msg.dest32 != ch->msi.msg.dest32 )
>>>>> +    {
>>>>> +        ch->msi.msg = msg;
>>>>> +
>>>>> +        if ( iommu_intremap != iommu_intremap_off )
>>>>> +        {
>>>>> +            int rc = iommu_update_ire_from_msi(&ch->msi, &msg);
>>>>>  
>>>>> -    /* We may have missed an interrupt due to the temporary masking. */
>>>>> -    if ( ch->event_handler && ch->next_event < NOW() )
>>>>> -        ch->event_handler(ch);
>>>>> +            ASSERT(rc <= 0);
>>>>> +            if ( rc > 0 )
>>>>> +            {
>>>>> +                ASSERT(msg.data == hpet_read32(HPET_Tn_ROUTE(ch->idx)));
>>>>> +                ASSERT(msg.address_lo ==
>>>>> +                       hpet_read32(HPET_Tn_ROUTE(ch->idx) + 4));
>>>>> +            }
>>>>
>>>> The sequence of asserts seem wrong here, the asserts inside of the rc
>>>>> 0 check will never trigger, because there's an ASSERT(rc <= 0)
>>>> ahead of them?
>>>
>>> Hmm. My way of thinking was that if we get back 1 (which we shouldn't),
>>> we ought to check (and presumably fail on) data or address having changed.
>>
>> Right, but the ASSERT(rc <= 0) will prevent reaching any of the
>> followup ASSERTs if rc == 1?
> 
> Which is no problem, as we'd be dead already anyway if the first assertion
> triggered. Nevertheless I've switched the if() to >= 0 (which then pointed
> out a necessary change in AMD IOMMU code).
> 
>>  IOW, we possibly want:
>>
>>             if ( rc > 0 )
>>             {
>>                 dprintk(XENLOG_ERR,
>>                         "Unexpected HPET MSI setup returned: data: %#x 
>> address: %#lx expected data %#x address %#lx\n",
>>                         msg.data, msg.address,
>>                         ch->msi.msg.data, ch->msi.msg.address);
>>                 ASSERT_UNREACHABLE();
>>                 hpet_msi_mask(desc);
>>                 hpet_write32(msg.data, HPET_Tn_ROUTE(ch->idx));
>>                 hpet_write32(msg.address_lo, HPET_Tn_ROUTE(ch->idx) + 4);
>>                 hpet_msi_unmask(desc);
>>             }
>>             ASSERT(!rc);
> 
> To be honest, for my taste this goes too far as to what follows an
> ASSERT_UNREACHABLE().

And it's insufficient: If we suspected the need for a non-atomic update,
the channel would need disabling before the IOMMU update.

Jan