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

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

[Jan Beulich]

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@suse.com>

--- 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);
+}
+
 static void cf_check iommu_msi_end(struct irq_desc *desc, u8 vector)
 {
     iommu_msi_unmask(desc);
@@ -455,7 +460,7 @@ static hw_irq_controller iommu_msi_type
     .shutdown = iommu_msi_mask,
     .enable = iommu_msi_unmask,
     .disable = iommu_msi_mask,
-    .ack = iommu_msi_mask,
+    .ack = iommu_msi_ack,
     .end = iommu_msi_end,
     .set_affinity = set_msi_affinity,
 };

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

[Andrew Cooper]

On 20/10/2025 3:16 pm, 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@suse.com>

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

[Roger Pau Monné]

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@suse.com>
> 
> --- 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.  Anyway this can be done later, so:

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.

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

[Jan Beulich]

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@suse.com>
>>
>> --- 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@citrix.com>

Thanks.

Jan

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

[Jan Beulich]

On 21.10.2025 11:59, Jan Beulich wrote:
> On 21.10.2025 11:48, Roger Pau Monné wrote:
>> On Mon, Oct 20, 2025 at 04:16:13PM +0200, Jan Beulich wrote:
>>> @@ -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.

... or on a different CPU.

Then again such nesting is avoided by generic logic, in particular via the
IRQ_INPROGRESS flag (in combination with the IRQ_PENDING one). So perhaps
you're right and the masking could be avoided (not just here).

Jan

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

[Oleksii Kurochko]

[-- Attachment #1: Type: text/plain, Size: 1683 bytes --]


On 10/20/25 4:16 PM, 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@suse.com>

Release-Acked-by: Oleksii Kurochko<oleksii.kurochko@gmail.com>

~ Oleksii

>
> --- 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);
> +}
> +
>   static void cf_check iommu_msi_end(struct irq_desc *desc, u8 vector)
>   {
>       iommu_msi_unmask(desc);
> @@ -455,7 +460,7 @@ static hw_irq_controller iommu_msi_type
>       .shutdown = iommu_msi_mask,
>       .enable = iommu_msi_unmask,
>       .disable = iommu_msi_mask,
> -    .ack = iommu_msi_mask,
> +    .ack = iommu_msi_ack,
>       .end = iommu_msi_end,
>       .set_affinity = set_msi_affinity,
>   };

[-- Attachment #2: Type: text/html, Size: 2329 bytes --]