1. Patchew
  2. Xen
  3. View series

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

Jan Beulich posted 1 patch 1 week, 2 days ago
Failed in applying to current master (apply log)
[PATCH for-4.21?] AMD/IOMMU: unshare IRQ .ack and .disable handlers
Posted by Jan Beulich 1 week, 2 days ago 
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
Posted by Oleksii Kurochko 1 week, 1 day ago 
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,
>   };
Re: [PATCH for-4.21?] AMD/IOMMU: unshare IRQ .ack and .disable handlers
Posted by Roger Pau Monné 1 week, 1 day ago 
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
Posted by Jan Beulich 1 week, 1 day ago 
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
Posted by Jan Beulich 1 week, 1 day ago 
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
Posted by Andrew Cooper 1 week, 2 days ago 
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>

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.