Re: [PATCH for-4.21?] AMD/IOMMU: unshare IRQ .ack and .disable handlers

[Jan Beulich <jbeulich@xxxxxxxx>]

On 21.10.2025 11:48, Roger Pau Monné wrote:
> On Mon, Oct 20, 2025 at 04:16:13PM +0200, Jan Beulich wrote:
>> A .disable handler can't typically be re-used for .ack: The latter needs
>> to deal with IRQ migration, while the former shouldn't. Furthermore
>> invoking just irq_complete_move() isn't enough; one of
>> move_{native,masked}_irq() also need invoking.
>>
>> Fixes: 487a1cffd71a ("x86: Implement per-cpu vector for xen hypervisor")
>> Fixes: f821102450a1 ("x86: IRQ Migration logic enhancement")
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>>
>> --- a/xen/drivers/passthrough/amd/iommu_init.c
>> +++ b/xen/drivers/passthrough/amd/iommu_init.c
>> @@ -428,8 +428,6 @@ static void cf_check iommu_msi_mask(stru
>>      unsigned long flags;
>>      struct amd_iommu *iommu = desc->action->dev_id;
>>  
>> -    irq_complete_move(desc);
>> -
>>      spin_lock_irqsave(&iommu->lock, flags);
>>      amd_iommu_msi_enable(iommu, IOMMU_CONTROL_DISABLED);
>>      spin_unlock_irqrestore(&iommu->lock, flags);
>> @@ -442,6 +440,13 @@ static unsigned int cf_check iommu_msi_s
>>      return 0;
>>  }
>>  
>> +static void cf_check iommu_msi_ack(struct irq_desc *desc)
>> +{
>> +    irq_complete_move(desc);
>> +    iommu_msi_mask(desc);
>> +    move_masked_irq(desc);
> 
> Not sure it matters much, as I don't expect IOMMU interrupts to move
> around frequently, but do we really need to mask the source?  The
> update of the interrupt would be done atomically, as we know IOMMU is
> available.

First I wanted to keep things in sync with other, similar functions. Then
the masking here may be not only about the moving, but also about this
actually being the .ack handler. In fact, when taking into account the
combination of both aspects, don't we need to deal with the case of this
being the last IRQ on the "old" vector, with us wanting to prevent another
IRQ on the "new" vector until we actually handled the IRQ? The LAPIC ack
(only done in the .end handler) wouldn't guard against that, if the "new"
vector is a higher priority one than the "old" one.

>  Anyway this can be done later, so:
> 
> Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Thanks.

Jan