[PATCH] iommu/amd: Restore three-stage vector cleanup for IR affinity migration

[PATCH] iommu/amd: Restore three-stage vector cleanup for IR affinity migration

[Guanghui Feng]

From: Guanghui Feng <guanghui.fgh@linux.alibaba.com>

AMD IOMMU specification (Rev 3.10) does not define interrupt message
draining semantics for INVALIDATE_INTERRUPT_TABLE + COMPLETION_WAIT.
This creates a race between vector_schedule_cleanup() and in-flight MSI
messages still transiting the Data Fabric after IRTE modification.

Intel VT-d is architecturally safe due to explicit interrupt draining
guarantees.  VT-d spec Section 6.5.5 states:

  "An interrupt request is considered drained when the interrupt is
   accepted by the processor Local xAPIC (for fixed and lowest priority
   delivery mode interrupts this means the interrupt is at least
   recorded in the Local xAPIC Interrupt Request Register (IRR))."

  "Interrupt draining is performed by remapping hardware after Interrupt
   Entry Cache (IEC) invalidation requests.  For IEC invalidations
   submitted through the queued invalidation interface, interrupt
   draining must be completed before the next Invalidation Wait
   Descriptor is completed by hardware."

  "For index-selective IEC invalidation requests, hardware only
   guarantees draining of non-committed interrupt requests referencing
   interrupt indexes specified in the invalidation request."

Additionally, Section 6.5.2.8 (IEC Invalidate Descriptor) mandates:

  "As part of IEC invalidation, hardware must drain interrupt requests
   that are already processed by the remapping hardware, but queued
   within the Root-Complex to be delivered to the processor."

Therefore when qi_flush_iec() + qi_submit_sync() returns (Wait Descriptor
reports QI_DONE), all non-committed interrupt messages referencing the
invalidated IRTE index are guaranteed to have been delivered to the
target LAPIC IRR.  The subsequent vector_schedule_cleanup() is safe
because is_vector_pending(prev_vector) will always observe the drained
message in IRR, causing the cleanup timer to rearm until the interrupt
is properly serviced.

In contrast, AMD IOMMU spec Section 2.4.5 defines
INVALIDATE_INTERRUPT_TABLE only as:

  "instructs the IOMMU to invalidate all cached interrupt information
   for the device"

with no completion condition, no drain semantics, and no ordering
guarantee for previously-remapped interrupt messages.  Section 2.4.11.2
("Invalidation Commands Interaction Requirements") limits the completion
guarantee exclusively to DMA transactions:

  "Invalidation commands are considered complete only when the IOMMU can
   ensure there are no DMA transactions in flight anywhere in the system
   fabric that relied on translation cache contents prior to
   invalidation."

The words "drain", "drained", or "draining" appear zero times in the
entire AMD IOMMU specification (Rev 3.10, 48882-PUB, Feb 2025) in
connection with interrupt messages.

Consequently, iommu_flush_irt_and_complete() returning does NOT guarantee
that old (vector, destination) interrupt messages have been delivered to
the previous CPU's LAPIC IRR.  The immediate vector_schedule_cleanup()
after IRTE modification races with in-flight messages on the Data Fabric:
the cleanup timer reads IRR clear (message still in transit), calls
free_moved_vector(), then the message arrives at the old CPU hitting
VECTOR_UNUSED -- producing "No irq handler for vector" and permanently
losing an edge-triggered MSI.

Fix by restoring three-stage cleanup semantics for the normal
(guest_mode=0) path:

 1. Change amd_ir_chip.irq_ack from apic_ack_irq to apic_ack_edge,
    which calls irq_complete_move().  This ensures cleanup is only
    scheduled when the new vector first arrives at the new CPU --
    providing a natural delay (device interrupt period + PCIe transit +
    IOMMU translation + Data Fabric delivery) that far exceeds any
    residual in-flight message transit time.

 2. Remove the unconditional vector_schedule_cleanup() from
    amd_ir_set_affinity() for the IRTE-modified path.

 3. For guest_mode=1 (AVIC/guest interrupt virtualization), the IRTE
    is not modified and no host-vector interrupt will arrive (interrupts
    are delivered to the vAPIC backing page instead).  Retain direct
    vector_schedule_cleanup() in this case to prevent move_in_progress
    from permanently blocking future affinity changes.

The combination of stage-2 natural delay and stage-3 IRR gating
(is_vector_pending + timer rearm) provides robust protection against
the AMD-specific lack of interrupt drain guarantee.

Intel VT-d, Hyper-V, and UV platforms are not modified as Intel is
architecturally safe per the spec guarantees cited above.

Fixes: 5478844b12d3 ("iommu/amd: Move interrupt remapping to its own file")
Signed-off-by: Guanghui Feng <guanghui.fgh@linux.alibaba.com>
---
 drivers/iommu/amd/amd_iommu_types.h |  2 +-
 drivers/iommu/amd/iommu.c           | 46 +++++++++++++++++++++--------
 2 files changed, 34 insertions(+), 14 deletions(-)

diff --git a/drivers/iommu/amd/amd_iommu_types.h b/drivers/iommu/amd/amd_iommu_types.h
index 8241ef922519..35024e3f5b90 100644
--- a/drivers/iommu/amd/amd_iommu_types.h
+++ b/drivers/iommu/amd/amd_iommu_types.h
@@ -1037,7 +1037,7 @@ struct amd_irte_ops {
 	void (*prepare)(void *, u32, bool, u8, u32, int);
 	void (*activate)(struct amd_iommu *iommu, void *, u16, u16);
 	void (*deactivate)(struct amd_iommu *iommu, void *, u16, u16);
-	void (*set_affinity)(struct amd_iommu *iommu, void *, u16, u16, u8, u32);
+	bool (*set_affinity)(struct amd_iommu *iommu, void *, u16, u16, u8, u32);
 	void *(*get)(struct irq_remap_table *, int);
 	void (*set_allocated)(struct irq_remap_table *, int);
 	bool (*is_allocated)(struct irq_remap_table *, int);
diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 56262f6b1f70..60e769384e8b 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -3634,7 +3634,7 @@ static void irte_ga_deactivate(struct amd_iommu *iommu, void *entry, u16 devid,
 	modify_irte_ga(iommu, devid, index, irte);
 }
 
-static void irte_set_affinity(struct amd_iommu *iommu, void *entry, u16 devid, u16 index,
+static bool irte_set_affinity(struct amd_iommu *iommu, void *entry, u16 devid, u16 index,
 			      u8 vector, u32 dest_apicid)
 {
 	union irte *irte = (union irte *) entry;
@@ -3642,9 +3642,11 @@ static void irte_set_affinity(struct amd_iommu *iommu, void *entry, u16 devid, u
 	irte->fields.vector = vector;
 	irte->fields.destination = dest_apicid;
 	modify_irte(iommu, devid, index, irte);
+
+	return true;
 }
 
-static void irte_ga_set_affinity(struct amd_iommu *iommu, void *entry, u16 devid, u16 index,
+static bool irte_ga_set_affinity(struct amd_iommu *iommu, void *entry, u16 devid, u16 index,
 				 u8 vector, u32 dest_apicid)
 {
 	struct irte_ga *irte = (struct irte_ga *) entry;
@@ -3656,7 +3658,11 @@ static void irte_ga_set_affinity(struct amd_iommu *iommu, void *entry, u16 devid
 		irte->hi.fields.destination =
 					APICID_TO_IRTE_DEST_HI(dest_apicid);
 		modify_irte_ga(iommu, devid, index, irte);
+
+		return true;
 	}
+
+	return false;
 }
 
 #define IRTE_ALLOCATED (~1U)
@@ -3933,7 +3939,7 @@ static void irq_remapping_free(struct irq_domain *domain, unsigned int virq,
 	irq_domain_free_irqs_common(domain, virq, nr_irqs);
 }
 
-static void amd_ir_update_irte(struct irq_data *irqd, struct amd_iommu *iommu,
+static bool amd_ir_update_irte(struct irq_data *irqd, struct amd_iommu *iommu,
 			       struct amd_ir_data *ir_data,
 			       struct irq_2_irte *irte_info,
 			       struct irq_cfg *cfg);
@@ -4153,7 +4159,7 @@ static int amd_ir_set_vcpu_affinity(struct irq_data *data, void *info)
 }
 
 
-static void amd_ir_update_irte(struct irq_data *irqd, struct amd_iommu *iommu,
+static bool amd_ir_update_irte(struct irq_data *irqd, struct amd_iommu *iommu,
 			       struct amd_ir_data *ir_data,
 			       struct irq_2_irte *irte_info,
 			       struct irq_cfg *cfg)
@@ -4163,9 +4169,9 @@ static void amd_ir_update_irte(struct irq_data *irqd, struct amd_iommu *iommu,
 	 * Atomically updates the IRTE with the new destination, vector
 	 * and flushes the interrupt entry cache.
 	 */
-	iommu->irte_ops->set_affinity(iommu, ir_data->entry, irte_info->devid,
-				      irte_info->index, cfg->vector,
-				      cfg->dest_apicid);
+	return iommu->irte_ops->set_affinity(iommu, ir_data->entry, irte_info->devid,
+					     irte_info->index, cfg->vector,
+					     cfg->dest_apicid);
 }
 
 static int amd_ir_set_affinity(struct irq_data *data,
@@ -4176,6 +4182,7 @@ static int amd_ir_set_affinity(struct irq_data *data,
 	struct irq_cfg *cfg = irqd_cfg(data);
 	struct irq_data *parent = data->parent_data;
 	struct amd_iommu *iommu = ir_data->iommu;
+	bool irte_modified;
 	int ret;
 
 	if (!iommu)
@@ -4185,13 +4192,26 @@ static int amd_ir_set_affinity(struct irq_data *data,
 	if (ret < 0 || ret == IRQ_SET_MASK_OK_DONE)
 		return ret;
 
-	amd_ir_update_irte(data, iommu, ir_data, irte_info, cfg);
+	irte_modified = amd_ir_update_irte(data, iommu, ir_data, irte_info, cfg);
+
 	/*
-	 * After this point, all the interrupts will start arriving
-	 * at the new destination. So, time to cleanup the previous
-	 * vector allocation.
+	 * If the IRTE was actually modified (guest_mode == 0), rely on the
+	 * three-stage cleanup: apic_ack_edge() calls irq_complete_move() when
+	 * the new vector first arrives at the new CPU, which then schedules
+	 * the cleanup of the previous vector allocation.  This avoids a race
+	 * where in-flight MSI messages, still on the Data Fabric after the
+	 * IRT invalidation because the AMD-Vi specification provides no
+	 * interrupt drain guarantee, arrive after the old vector has been
+	 * freed.
+	 *
+	 * If the IRTE was NOT modified (guest_mode == 1), no in-flight
+	 * messages exist for the host vector and the new host vector will
+	 * never receive an interrupt (those go to the vAPIC backing page
+	 * instead).  Schedule the cleanup directly to prevent
+	 * move_in_progress from getting permanently stuck.
 	 */
-	vector_schedule_cleanup(cfg);
+	if (!irte_modified)
+		vector_schedule_cleanup(cfg);
 
 	return IRQ_SET_MASK_OK_DONE;
 }
@@ -4205,7 +4225,7 @@ static void ir_compose_msi_msg(struct irq_data *irq_data, struct msi_msg *msg)
 
 static struct irq_chip amd_ir_chip = {
 	.name			= "AMD-IR",
-	.irq_ack		= apic_ack_irq,
+	.irq_ack		= apic_ack_edge,
 	.irq_set_affinity	= amd_ir_set_affinity,
 	.irq_set_vcpu_affinity	= amd_ir_set_vcpu_affinity,
 	.irq_compose_msi_msg	= ir_compose_msi_msg,
-- 
2.43.7

Re: [PATCH] iommu/amd: Restore three-stage vector cleanup for IR affinity migration

[Suthikulpanit, Suravee]

Hi Guanghui,

On 10/15/2026 11:23 AM, Guanghui Feng wrote:
> From: Guanghui Feng <guanghui.fgh@linux.alibaba.com>
> 
> AMD IOMMU specification (Rev 3.10) does not define interrupt message
> draining semantics for INVALIDATE_INTERRUPT_TABLE + COMPLETION_WAIT.
> This creates a race between vector_schedule_cleanup() and in-flight MSI
> messages still transiting the Data Fabric after IRTE modification.
> 
> Intel VT-d is architecturally safe due to explicit interrupt draining
> guarantees.  VT-d spec Section 6.5.5 states:
> 
>    "An interrupt request is considered drained when the interrupt is
>     accepted by the processor Local xAPIC (for fixed and lowest priority
>     delivery mode interrupts this means the interrupt is at least
>     recorded in the Local xAPIC Interrupt Request Register (IRR))."
> 
>    "Interrupt draining is performed by remapping hardware after Interrupt
>     Entry Cache (IEC) invalidation requests.  For IEC invalidations
>     submitted through the queued invalidation interface, interrupt
>     draining must be completed before the next Invalidation Wait
>     Descriptor is completed by hardware."
> 
>    "For index-selective IEC invalidation requests, hardware only
>     guarantees draining of non-committed interrupt requests referencing
>     interrupt indexes specified in the invalidation request."
> 
> Additionally, Section 6.5.2.8 (IEC Invalidate Descriptor) mandates:
> 
>    "As part of IEC invalidation, hardware must drain interrupt requests
>     that are already processed by the remapping hardware, but queued
>     within the Root-Complex to be delivered to the processor."
> 
> Therefore when qi_flush_iec() + qi_submit_sync() returns (Wait Descriptor
> reports QI_DONE), all non-committed interrupt messages referencing the
> invalidated IRTE index are guaranteed to have been delivered to the
> target LAPIC IRR.  The subsequent vector_schedule_cleanup() is safe
> because is_vector_pending(prev_vector) will always observe the drained
> message in IRR, causing the cleanup timer to rearm until the interrupt
> is properly serviced.
> 
> In contrast, AMD IOMMU spec Section 2.4.5 defines
> INVALIDATE_INTERRUPT_TABLE only as:
> 
>    "instructs the IOMMU to invalidate all cached interrupt information
>     for the device"
> 
> with no completion condition, no drain semantics, and no ordering
> guarantee for previously-remapped interrupt messages.  Section 2.4.11.2
> ("Invalidation Commands Interaction Requirements") limits the completion
> guarantee exclusively to DMA transactions:
> 
>    "Invalidation commands are considered complete only when the IOMMU can
>     ensure there are no DMA transactions in flight anywhere in the system
>     fabric that relied on translation cache contents prior to
>     invalidation."
> 
> The words "drain", "drained", or "draining" appear zero times in the
> entire AMD IOMMU specification (Rev 3.10, 48882-PUB, Feb 2025) in
> connection with interrupt messages.
> 
> Consequently, iommu_flush_irt_and_complete() returning does NOT guarantee
> that old (vector, destination) interrupt messages have been delivered to
> the previous CPU's LAPIC IRR.  The immediate vector_schedule_cleanup()
> after IRTE modification races with in-flight messages on the Data Fabric:
> the cleanup timer reads IRR clear (message still in transit), calls
> free_moved_vector(), then the message arrives at the old CPU hitting
> VECTOR_UNUSED -- producing "No irq handler for vector" and permanently
> losing an edge-triggered MSI.

Please describe the scenario you encountered to reproduce this issue.

> Fix by restoring three-stage cleanup semantics for the normal
> (guest_mode=0) path:
> 
>   1. Change amd_ir_chip.irq_ack from apic_ack_irq to apic_ack_edge,
>      which calls irq_complete_move().  This ensures cleanup is only
>      scheduled when the new vector first arrives at the new CPU --
>      providing a natural delay (device interrupt period + PCIe transit +
>      IOMMU translation + Data Fabric delivery) that far exceeds any
>      residual in-flight message transit time.
> 
>   2. Remove the unconditional vector_schedule_cleanup() from
>      amd_ir_set_affinity() for the IRTE-modified path.
> 
>   3. For guest_mode=1 (AVIC/guest interrupt virtualization), the IRTE
>      is not modified and no host-vector interrupt will arrive (interrupts
>      are delivered to the vAPIC backing page instead).  Retain direct
>      vector_schedule_cleanup() in this case to prevent move_in_progress
>      from permanently blocking future affinity changes.

Please describe the permanently blocking scenario.

> The combination of stage-2 natural delay and stage-3 IRR gating
> (is_vector_pending + timer rearm) provides robust protection against
> the AMD-specific lack of interrupt drain guarantee.

Adding some arbitary delay does not necessary guarantee the delivery 
before clearing the old vector.

> Intel VT-d, Hyper-V, and UV platforms are not modified as Intel is
> architecturally safe per the spec guarantees cited above.
> 
> Fixes: 5478844b12d3 ("iommu/amd: Move interrupt remapping to its own file")

Is this the correct commit? I could not find this commit. Could you 
please provide link to the commit?

> Signed-off-by: Guanghui Feng <guanghui.fgh@linux.alibaba.com>
> ---
>   drivers/iommu/amd/amd_iommu_types.h |  2 +-
>   drivers/iommu/amd/iommu.c           | 46 +++++++++++++++++++++--------
>   2 files changed, 34 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/iommu/amd/amd_iommu_types.h b/drivers/iommu/amd/amd_iommu_types.h
> index 8241ef922519..35024e3f5b90 100644
> --- a/drivers/iommu/amd/amd_iommu_types.h
> +++ b/drivers/iommu/amd/amd_iommu_types.h
> @@ -1037,7 +1037,7 @@ struct amd_irte_ops {
>   	void (*prepare)(void *, u32, bool, u8, u32, int);
>   	void (*activate)(struct amd_iommu *iommu, void *, u16, u16);
>   	void (*deactivate)(struct amd_iommu *iommu, void *, u16, u16);
> -	void (*set_affinity)(struct amd_iommu *iommu, void *, u16, u16, u8, u32);
> +	bool (*set_affinity)(struct amd_iommu *iommu, void *, u16, u16, u8, u32);
>   	void *(*get)(struct irq_remap_table *, int);
>   	void (*set_allocated)(struct irq_remap_table *, int);
>   	bool (*is_allocated)(struct irq_remap_table *, int);
> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
> index 56262f6b1f70..60e769384e8b 100644
> --- a/drivers/iommu/amd/iommu.c
> +++ b/drivers/iommu/amd/iommu.c
> @@ -3634,7 +3634,7 @@ static void irte_ga_deactivate(struct amd_iommu *iommu, void *entry, u16 devid,
>   	modify_irte_ga(iommu, devid, index, irte);
>   }
>   
> -static void irte_set_affinity(struct amd_iommu *iommu, void *entry, u16 devid, u16 index,
> +static bool irte_set_affinity(struct amd_iommu *iommu, void *entry, u16 devid, u16 index,
>   			      u8 vector, u32 dest_apicid)
>   {
>   	union irte *irte = (union irte *) entry;
> @@ -3642,9 +3642,11 @@ static void irte_set_affinity(struct amd_iommu *iommu, void *entry, u16 devid, u
>   	irte->fields.vector = vector;
>   	irte->fields.destination = dest_apicid;
>   	modify_irte(iommu, devid, index, irte);
> +
> +	return true;

This blindly return true regardless of the return value from modify_irte_ga.

>   }
>   
> -static void irte_ga_set_affinity(struct amd_iommu *iommu, void *entry, u16 devid, u16 index,
> +static bool irte_ga_set_affinity(struct amd_iommu *iommu, void *entry, u16 devid, u16 index,
>   				 u8 vector, u32 dest_apicid)
>   {
>   	struct irte_ga *irte = (struct irte_ga *) entry;
> @@ -3656,7 +3658,11 @@ static void irte_ga_set_affinity(struct amd_iommu *iommu, void *entry, u16 devid
>   		irte->hi.fields.destination =
>   					APICID_TO_IRTE_DEST_HI(dest_apicid);
>   		modify_irte_ga(iommu, devid, index, irte);
> +
> +		return true;
>   	}
> +
> +	return false;

This blindly return true regardless of the return value from modify_irte_ga.

>   }
>   
>   #define IRTE_ALLOCATED (~1U)
> @@ -3933,7 +3939,7 @@ static void irq_remapping_free(struct irq_domain *domain, unsigned int virq,
>   	irq_domain_free_irqs_common(domain, virq, nr_irqs);
>   }
>   
> -static void amd_ir_update_irte(struct irq_data *irqd, struct amd_iommu *iommu,
> +static bool amd_ir_update_irte(struct irq_data *irqd, struct amd_iommu *iommu,
>   			       struct amd_ir_data *ir_data,
>   			       struct irq_2_irte *irte_info,
>   			       struct irq_cfg *cfg);
> @@ -4153,7 +4159,7 @@ static int amd_ir_set_vcpu_affinity(struct irq_data *data, void *info)
>   }
>   
>   
> -static void amd_ir_update_irte(struct irq_data *irqd, struct amd_iommu *iommu,
> +static bool amd_ir_update_irte(struct irq_data *irqd, struct amd_iommu *iommu,
>   			       struct amd_ir_data *ir_data,
>   			       struct irq_2_irte *irte_info,
>   			       struct irq_cfg *cfg)
> @@ -4163,9 +4169,9 @@ static void amd_ir_update_irte(struct irq_data *irqd, struct amd_iommu *iommu,
>   	 * Atomically updates the IRTE with the new destination, vector
>   	 * and flushes the interrupt entry cache.
>   	 */
> -	iommu->irte_ops->set_affinity(iommu, ir_data->entry, irte_info->devid,
> -				      irte_info->index, cfg->vector,
> -				      cfg->dest_apicid);
> +	return iommu->irte_ops->set_affinity(iommu, ir_data->entry, irte_info->devid,
> +					     irte_info->index, cfg->vector,
> +					     cfg->dest_apicid);
>   }
>   
>   static int amd_ir_set_affinity(struct irq_data *data,
> @@ -4176,6 +4182,7 @@ static int amd_ir_set_affinity(struct irq_data *data,
>   	struct irq_cfg *cfg = irqd_cfg(data);
>   	struct irq_data *parent = data->parent_data;
>   	struct amd_iommu *iommu = ir_data->iommu;
> +	bool irte_modified;
>   	int ret;
>   
>   	if (!iommu)
> @@ -4185,13 +4192,26 @@ static int amd_ir_set_affinity(struct irq_data *data,
>   	if (ret < 0 || ret == IRQ_SET_MASK_OK_DONE)
>   		return ret;
>   
> -	amd_ir_update_irte(data, iommu, ir_data, irte_info, cfg);
> +	irte_modified = amd_ir_update_irte(data, iommu, ir_data, irte_info, cfg);
> +
>   	/*
> -	 * After this point, all the interrupts will start arriving
> -	 * at the new destination. So, time to cleanup the previous
> -	 * vector allocation.
> +	 * If the IRTE was actually modified (guest_mode == 0), rely on the
> +	 * three-stage cleanup: apic_ack_edge() calls irq_complete_move() when
> +	 * the new vector first arrives at the new CPU, which then schedules
> +	 * the cleanup of the previous vector allocation.  This avoids a race
> +	 * where in-flight MSI messages, still on the Data Fabric after the
> +	 * IRT invalidation because the AMD-Vi specification provides no
> +	 * interrupt drain guarantee, arrive after the old vector has been
> +	 * freed.
> +	 *
> +	 * If the IRTE was NOT modified (guest_mode == 1), no in-flight
> +	 * messages exist for the host vector and the new host vector will
> +	 * never receive an interrupt (those go to the vAPIC backing page
> +	 * instead).  Schedule the cleanup directly to prevent
> +	 * move_in_progress from getting permanently stuck.
>   	 */
> -	vector_schedule_cleanup(cfg);
> +	if (!irte_modified)
> +		vector_schedule_cleanup(cfg);

The !irte_modified is really checking guest_mode == 0 before calling 
vector_schedule_cleanup(). The name is misleading. I also do not see how 
this resolves the issue instead of calling vector_schedule_cleanup() 
unconditionally.

>   
>   	return IRQ_SET_MASK_OK_DONE;
>   }
> @@ -4205,7 +4225,7 @@ static void ir_compose_msi_msg(struct irq_data *irq_data, struct msi_msg *msg)
>   
>   static struct irq_chip amd_ir_chip = {
>   	.name			= "AMD-IR",
> -	.irq_ack		= apic_ack_irq,
> +	.irq_ack		= apic_ack_edge,

The apic_ack_edge() basically calls __vector_schedule_cleanup() before 
apic_ack_irq() on the new cpu, which should have already been called 
during irq_set_affinity() and clears the move_in_progress. So, the 
apic_ack_edge() would not be calling __vector_schedule_cleanup(). I do 
not see how this would make a different.

 From AMD IOMMU hardware standpoint,

1. The IOMMU considers the interrupt "done" at the point the write 
completes at the fabric level (i.e. when the upstream posted write is 
accepted). There is no round-trip back from the CPU APIC.

2. When SW issues an interrupt table invalidation followed by 
COMPLETION_WAIT, the IOMMU must drain all in-flight interrupt before 
acknowledging the COMPLETION_WAIT. This ensures any stale IRTE data has 
been flushed before the invalidation is considered complete from the 
software's perspective.

3. The time to drainthe data fabric should be much less than the 
1-jiffie during the vector_schedule_cleanup().

Regards,

Suravee

>   	.irq_set_affinity	= amd_ir_set_affinity,
>   	.irq_set_vcpu_affinity	= amd_ir_set_vcpu_affinity,
>   	.irq_compose_msi_msg	= ir_compose_msi_msg,

Re: [PATCH] iommu/amd: Restore three-stage vector cleanup for IR affinity migration

[guanghuifeng]

Thank you for the detailed review and the hardware clarification.

在 2026/10/20 10:36, Suthikulpanit, Suravee 写道:
> Hi Guanghui,
> 
> On 10/15/2026 11:23 AM, Guanghui Feng wrote:
>> From: Guanghui Feng <guanghui.fgh@linux.alibaba.com>
>>
>> AMD IOMMU specification (Rev 3.10) does not define interrupt message
>> draining semantics for INVALIDATE_INTERRUPT_TABLE + COMPLETION_WAIT.
>> This creates a race between vector_schedule_cleanup() and in-flight MSI
>> messages still transiting the Data Fabric after IRTE modification.
>>
>> Intel VT-d is architecturally safe due to explicit interrupt draining
>> guarantees.  VT-d spec Section 6.5.5 states:
>>
>>    "An interrupt request is considered drained when the interrupt is
>>     accepted by the processor Local xAPIC (for fixed and lowest priority
>>     delivery mode interrupts this means the interrupt is at least
>>     recorded in the Local xAPIC Interrupt Request Register (IRR))."
>>
>>    "Interrupt draining is performed by remapping hardware after Interrupt
>>     Entry Cache (IEC) invalidation requests.  For IEC invalidations
>>     submitted through the queued invalidation interface, interrupt
>>     draining must be completed before the next Invalidation Wait
>>     Descriptor is completed by hardware."
>>
>>    "For index-selective IEC invalidation requests, hardware only
>>     guarantees draining of non-committed interrupt requests referencing
>>     interrupt indexes specified in the invalidation request."
>>
>> Additionally, Section 6.5.2.8 (IEC Invalidate Descriptor) mandates:
>>
>>    "As part of IEC invalidation, hardware must drain interrupt requests
>>     that are already processed by the remapping hardware, but queued
>>     within the Root-Complex to be delivered to the processor."
>>
>> Therefore when qi_flush_iec() + qi_submit_sync() returns (Wait Descriptor
>> reports QI_DONE), all non-committed interrupt messages referencing the
>> invalidated IRTE index are guaranteed to have been delivered to the
>> target LAPIC IRR.  The subsequent vector_schedule_cleanup() is safe
>> because is_vector_pending(prev_vector) will always observe the drained
>> message in IRR, causing the cleanup timer to rearm until the interrupt
>> is properly serviced.
>>
>> In contrast, AMD IOMMU spec Section 2.4.5 defines
>> INVALIDATE_INTERRUPT_TABLE only as:
>>
>>    "instructs the IOMMU to invalidate all cached interrupt information
>>     for the device"
>>
>> with no completion condition, no drain semantics, and no ordering
>> guarantee for previously-remapped interrupt messages.  Section 2.4.11.2
>> ("Invalidation Commands Interaction Requirements") limits the completion
>> guarantee exclusively to DMA transactions:
>>
>>    "Invalidation commands are considered complete only when the IOMMU can
>>     ensure there are no DMA transactions in flight anywhere in the system
>>     fabric that relied on translation cache contents prior to
>>     invalidation."
>>
>> The words "drain", "drained", or "draining" appear zero times in the
>> entire AMD IOMMU specification (Rev 3.10, 48882-PUB, Feb 2025) in
>> connection with interrupt messages.
>>
>> Consequently, iommu_flush_irt_and_complete() returning does NOT guarantee
>> that old (vector, destination) interrupt messages have been delivered to
>> the previous CPU's LAPIC IRR.  The immediate vector_schedule_cleanup()
>> after IRTE modification races with in-flight messages on the Data Fabric:
>> the cleanup timer reads IRR clear (message still in transit), calls
>> free_moved_vector(), then the message arrives at the old CPU hitting
>> VECTOR_UNUSED -- producing "No irq handler for vector" and permanently
>> losing an edge-triggered MSI.
> 
> Please describe the scenario you encountered to reproduce this issue.

We observed this on kernel 5.10 in a virtualization shared-instance
deployment:

- Guest uses VFIO passthrough devices (NVMe, NIC) with IOMMU interrupt
   remapping enabled (non-posted mode, guest_mode=0).
- Interrupts are delivered via KVM irqfd/eventfd software injection
   path, not hardware posted interrupts.
- When a vCPU is migrated to a different physical CPU (scheduler
   balancing or NUMA-aware placement), may adjusts the associated
   device interrupt affinity to follow the vCPU's new pCPU via
   irq_set_affinity -> amd_ir_set_affinity.
- During this affinity migration, the device continues generating MSI
   at high rate.  We observe intermittent "do_IRQ: X.Y No irq handler
   for vector" messages

Note: kernel 5.10 does not contain commit bdc1dad299bb ("x86/vector:
Replace IRQ_MOVE_CLEANUP_VECTOR with a timer callback").  In 5.10,
the cleanup path uses send_cleanup_vector() which sends an IPI
(IRQ_MOVE_CLEANUP_VECTOR) directly to the old CPU.  The IPI arrives
and runs sysvec_irq_move_cleanup() almost immediately -- providing
even less delay than the timer-based approach in newer kernels.
This makes the race window significantly wider on 5.10.

Commit bdc1dad299bb already recognized and partially mitigated this
class of issue by:
   - Replacing the immediate IPI with a per-CPU timer (jiffies+1)
   - Adding is_vector_pending() IRR check before freeing


> 
>> Fix by restoring three-stage cleanup semantics for the normal
>> (guest_mode=0) path:
>>
>>   1. Change amd_ir_chip.irq_ack from apic_ack_irq to apic_ack_edge,
>>      which calls irq_complete_move().  This ensures cleanup is only
>>      scheduled when the new vector first arrives at the new CPU --
>>      providing a natural delay (device interrupt period + PCIe transit +
>>      IOMMU translation + Data Fabric delivery) that far exceeds any
>>      residual in-flight message transit time.
>>
>>   2. Remove the unconditional vector_schedule_cleanup() from
>>      amd_ir_set_affinity() for the IRTE-modified path.
>>
>>   3. For guest_mode=1 (AVIC/guest interrupt virtualization), the IRTE
>>      is not modified and no host-vector interrupt will arrive (interrupts
>>      are delivered to the vAPIC backing page instead).  Retain direct
>>      vector_schedule_cleanup() in this case to prevent move_in_progress
>>      from permanently blocking future affinity changes.
> 
> Please describe the permanently blocking scenario.

When guest_mode=1 (AVIC), irte_ga_set_affinity() does not modify the
IRTE (guarded by "if (!irte->lo.fields_remap.guest_mode)").  However,
the parent domain (apic_set_affinity) has already allocated a new host
vector and set move_in_progress=1.  Since the IRTE still points to
the guest vAPIC backing page, no interrupt will ever arrive at the
host with the new vector, so irq_complete_move() will never fire.

Without vector_schedule_cleanup() in this path, move_in_progress stays
set permanently.  Subsequent calls to assign_vector_locked() check:

     if (apicd->move_in_progress || !hlist_unhashed(&apicd->clist))
         return -EBUSY;

This causes all future affinity changes for this IRQ to fail with
-EBUSY, which irq_move_masked_irq() handles by re-setting
IRQD_SETAFFINITY_PENDING -- but since no interrupt arrives on the
host to trigger __irq_move_irq(), the pending request is never
processed.  The IRQ's affinity becomes permanently stuck.


> 
>> The combination of stage-2 natural delay and stage-3 IRR gating
>> (is_vector_pending + timer rearm) provides robust protection against
>> the AMD-specific lack of interrupt drain guarantee.
> 
> Adding some arbitary delay does not necessary guarantee the delivery 
> before clearing the old vector.
> 
>> Intel VT-d, Hyper-V, and UV platforms are not modified as Intel is
>> architecturally safe per the spec guarantees cited above.
>>
>> Fixes: 5478844b12d3 ("iommu/amd: Move interrupt remapping to its own 
>> file")
> 
> Is this the correct commit? I could not find this commit. Could you 
> please provide link to the commit?
> 
>> Signed-off-by: Guanghui Feng <guanghui.fgh@linux.alibaba.com>
>> ---
>>   drivers/iommu/amd/amd_iommu_types.h |  2 +-
>>   drivers/iommu/amd/iommu.c           | 46 +++++++++++++++++++++--------
>>   2 files changed, 34 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/iommu/amd/amd_iommu_types.h b/drivers/iommu/amd/ 
>> amd_iommu_types.h
>> index 8241ef922519..35024e3f5b90 100644
>> --- a/drivers/iommu/amd/amd_iommu_types.h
>> +++ b/drivers/iommu/amd/amd_iommu_types.h
>> @@ -1037,7 +1037,7 @@ struct amd_irte_ops {
>>       void (*prepare)(void *, u32, bool, u8, u32, int);
>>       void (*activate)(struct amd_iommu *iommu, void *, u16, u16);
>>       void (*deactivate)(struct amd_iommu *iommu, void *, u16, u16);
>> -    void (*set_affinity)(struct amd_iommu *iommu, void *, u16, u16, 
>> u8, u32);
>> +    bool (*set_affinity)(struct amd_iommu *iommu, void *, u16, u16, 
>> u8, u32);
>>       void *(*get)(struct irq_remap_table *, int);
>>       void (*set_allocated)(struct irq_remap_table *, int);
>>       bool (*is_allocated)(struct irq_remap_table *, int);
>> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
>> index 56262f6b1f70..60e769384e8b 100644
>> --- a/drivers/iommu/amd/iommu.c
>> +++ b/drivers/iommu/amd/iommu.c
>> @@ -3634,7 +3634,7 @@ static void irte_ga_deactivate(struct amd_iommu 
>> *iommu, void *entry, u16 devid,
>>       modify_irte_ga(iommu, devid, index, irte);
>>   }
>> -static void irte_set_affinity(struct amd_iommu *iommu, void *entry, 
>> u16 devid, u16 index,
>> +static bool irte_set_affinity(struct amd_iommu *iommu, void *entry, 
>> u16 devid, u16 index,
>>                     u8 vector, u32 dest_apicid)
>>   {
>>       union irte *irte = (union irte *) entry;
>> @@ -3642,9 +3642,11 @@ static void irte_set_affinity(struct amd_iommu 
>> *iommu, void *entry, u16 devid, u
>>       irte->fields.vector = vector;
>>       irte->fields.destination = dest_apicid;
>>       modify_irte(iommu, devid, index, irte);
>> +
>> +    return true;
> 
> This blindly return true regardless of the return value from 
> modify_irte_ga.
> 
>>   }
>> -static void irte_ga_set_affinity(struct amd_iommu *iommu, void 
>> *entry, u16 devid, u16 index,
>> +static bool irte_ga_set_affinity(struct amd_iommu *iommu, void 
>> *entry, u16 devid, u16 index,
>>                    u8 vector, u32 dest_apicid)
>>   {
>>       struct irte_ga *irte = (struct irte_ga *) entry;
>> @@ -3656,7 +3658,11 @@ static void irte_ga_set_affinity(struct 
>> amd_iommu *iommu, void *entry, u16 devid
>>           irte->hi.fields.destination =
>>                       APICID_TO_IRTE_DEST_HI(dest_apicid);
>>           modify_irte_ga(iommu, devid, index, irte);
>> +
>> +        return true;
>>       }
>> +
>> +    return false;
> 
> This blindly return true regardless of the return value from 
> modify_irte_ga.
> 
>>   }
>>   #define IRTE_ALLOCATED (~1U)
>> @@ -3933,7 +3939,7 @@ static void irq_remapping_free(struct irq_domain 
>> *domain, unsigned int virq,
>>       irq_domain_free_irqs_common(domain, virq, nr_irqs);
>>   }
>> -static void amd_ir_update_irte(struct irq_data *irqd, struct 
>> amd_iommu *iommu,
>> +static bool amd_ir_update_irte(struct irq_data *irqd, struct 
>> amd_iommu *iommu,
>>                      struct amd_ir_data *ir_data,
>>                      struct irq_2_irte *irte_info,
>>                      struct irq_cfg *cfg);
>> @@ -4153,7 +4159,7 @@ static int amd_ir_set_vcpu_affinity(struct 
>> irq_data *data, void *info)
>>   }
>> -static void amd_ir_update_irte(struct irq_data *irqd, struct 
>> amd_iommu *iommu,
>> +static bool amd_ir_update_irte(struct irq_data *irqd, struct 
>> amd_iommu *iommu,
>>                      struct amd_ir_data *ir_data,
>>                      struct irq_2_irte *irte_info,
>>                      struct irq_cfg *cfg)
>> @@ -4163,9 +4169,9 @@ static void amd_ir_update_irte(struct irq_data 
>> *irqd, struct amd_iommu *iommu,
>>        * Atomically updates the IRTE with the new destination, vector
>>        * and flushes the interrupt entry cache.
>>        */
>> -    iommu->irte_ops->set_affinity(iommu, ir_data->entry, irte_info- 
>> >devid,
>> -                      irte_info->index, cfg->vector,
>> -                      cfg->dest_apicid);
>> +    return iommu->irte_ops->set_affinity(iommu, ir_data->entry, 
>> irte_info->devid,
>> +                         irte_info->index, cfg->vector,
>> +                         cfg->dest_apicid);
>>   }
>>   static int amd_ir_set_affinity(struct irq_data *data,
>> @@ -4176,6 +4182,7 @@ static int amd_ir_set_affinity(struct irq_data 
>> *data,
>>       struct irq_cfg *cfg = irqd_cfg(data);
>>       struct irq_data *parent = data->parent_data;
>>       struct amd_iommu *iommu = ir_data->iommu;
>> +    bool irte_modified;
>>       int ret;
>>       if (!iommu)
>> @@ -4185,13 +4192,26 @@ static int amd_ir_set_affinity(struct irq_data 
>> *data,
>>       if (ret < 0 || ret == IRQ_SET_MASK_OK_DONE)
>>           return ret;
>> -    amd_ir_update_irte(data, iommu, ir_data, irte_info, cfg);
>> +    irte_modified = amd_ir_update_irte(data, iommu, ir_data, 
>> irte_info, cfg);
>> +
>>       /*
>> -     * After this point, all the interrupts will start arriving
>> -     * at the new destination. So, time to cleanup the previous
>> -     * vector allocation.
>> +     * If the IRTE was actually modified (guest_mode == 0), rely on the
>> +     * three-stage cleanup: apic_ack_edge() calls irq_complete_move() 
>> when
>> +     * the new vector first arrives at the new CPU, which then schedules
>> +     * the cleanup of the previous vector allocation.  This avoids a 
>> race
>> +     * where in-flight MSI messages, still on the Data Fabric after the
>> +     * IRT invalidation because the AMD-Vi specification provides no
>> +     * interrupt drain guarantee, arrive after the old vector has been
>> +     * freed.
>> +     *
>> +     * If the IRTE was NOT modified (guest_mode == 1), no in-flight
>> +     * messages exist for the host vector and the new host vector will
>> +     * never receive an interrupt (those go to the vAPIC backing page
>> +     * instead).  Schedule the cleanup directly to prevent
>> +     * move_in_progress from getting permanently stuck.
>>        */
>> -    vector_schedule_cleanup(cfg);
>> +    if (!irte_modified)
>> +        vector_schedule_cleanup(cfg);
> 
> The !irte_modified is really checking guest_mode == 0 before calling 
> vector_schedule_cleanup(). The name is misleading. I also do not see how 
> this resolves the issue instead of calling vector_schedule_cleanup() 
> unconditionally.
> 
>>       return IRQ_SET_MASK_OK_DONE;
>>   }
>> @@ -4205,7 +4225,7 @@ static void ir_compose_msi_msg(struct irq_data 
>> *irq_data, struct msi_msg *msg)
>>   static struct irq_chip amd_ir_chip = {
>>       .name            = "AMD-IR",
>> -    .irq_ack        = apic_ack_irq,
>> +    .irq_ack        = apic_ack_edge,
> 
> The apic_ack_edge() basically calls __vector_schedule_cleanup() before 
> apic_ack_irq() on the new cpu, which should have already been called 
> during irq_set_affinity() and clears the move_in_progress. So, the 
> apic_ack_edge() would not be calling __vector_schedule_cleanup(). I do 
> not see how this would make a different.
> 

I believe there may be a misunderstanding.  The patch has two parts
that work together as a pair:

   Part (1): Change .irq_ack to apic_ack_edge (adds irq_complete_move)
   Part (2): REMOVE the unconditional vector_schedule_cleanup() from
             amd_ir_set_affinity() for the guest_mode=0 path

With part (2), move_in_progress is NOT cleared during
irq_set_affinity() anymore.  It remains set until the new vector
first arrives at the new CPU and apic_ack_edge() -> irq_complete_move()
fires. both changes are required together.

>  From AMD IOMMU hardware standpoint,
> 
> 1. The IOMMU considers the interrupt "done" at the point the write 
> completes at the fabric level (i.e. when the upstream posted write is 
> accepted). There is no round-trip back from the CPU APIC.
> 
> 2. When SW issues an interrupt table invalidation followed by 
> COMPLETION_WAIT, the IOMMU must drain all in-flight interrupt before 
> acknowledging the COMPLETION_WAIT. This ensures any stale IRTE data has 
> been flushed before the invalidation is considered complete from the 
> software's perspective.
> 
> 3. The time to drainthe data fabric should be much less than the 1- 
> jiffie during the vector_schedule_cleanup().
> 
> Regards,
> 
> Suravee

- COMPLETION_WAIT guarantees all in-flight interrupts are drained
   before software proceeds (point 2).
- The drain time is well within 1 jiffy (point 3).
- The timer + is_vector_pending() IRR check from bdc1dad299bb
   provides additional safety margin.

With this understanding, we agree that mainline kernels (containing
bdc1dad299bb) are safe without this patch.

> 
>>       .irq_set_affinity    = amd_ir_set_affinity,
>>       .irq_set_vcpu_affinity    = amd_ir_set_vcpu_affinity,
>>       .irq_compose_msi_msg    = ir_compose_msi_msg,