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
