[RFC PATCH v2 0/5] Add support for AMD IOMMU GAPPI

[RFC PATCH v2 0/5] Add support for AMD IOMMU GAPPI

[Sairaj Kodilkar]

Introduction
============
On newer generations of AMD processors, the IOMMU AVIC/x2AVIC feature can be
enabled using the Guest APIC Physical Processor Interrupt (GAPPI) mode, which is
an alternative for handling AVIC guest interrupts to non-running vCPUs
(i.e., IRTE[IsRun]=0).

With GAPPI enabled, the IOMMU delivers a posted interrupt to the physical CPU
described by the IRTE destination fields, with the wake-up vector in ga_tag.

See section 2.2.5.4, Guest APIC Physical Processor Interrupt, of the AMD I/O
Virtualization Technology (IOMMU) Specification [1] for more details on GAPPI.

Implementation Details
======================
GAPPI reuses the posted-interrupt wake-up path introduced for Intel VMX: the
IOMMU sets ga_tag to POSTED_INTR_WAKEUP_VECTOR, and SVM registers the handler
with kvm_set_posted_intr_wakeup_handler().

SVM maintains a per-CPU list of vCPUs that are scheduled out. When a CPU
receives a GAPPI interrupt from the IOMMU, the handler walks that list, finds
vCPUs with a pending IRR bit, and wakes them.

The IRTE destination is chosen as the last host CPU where the vCPU ran, to
reduce unnecessary VMEXITs from GAPPI deliveries.

The first patch refactors the SVM/IOMMU interface: apicid (formerly cpu)
denotes the running vCPU's host APIC ID or, when the vCPU is scheduled out,
the APIC ID of the pCPU that hosts it on its GAPPI wake-up list. Explicit
running and posted-interrupt flags replace the old ga_log_intr boolean, since
apicid no longer implies vCPU state on its own.

Advantages
==========
With GALOG, the IOMMU can generate only a single interrupt via MMIO offset
0x180h (XT IOMMU GA Log Interrupt Control Register) and appends vCPU
information to the GALOG buffer. The hypervisor must scan this list to wake
the vCPUs, which can introduce significant latency and even cause buffer
overflows at high interrupt rates.

GAPPI avoids this problem by distributing posted interrupts across multiple
CPUs.

[1] https://docs.amd.com/v/u/en-US/48882_3.11_IOMMU_PUB

-------

Changes since V1:
https://lore.kernel.org/all/20260626105906.14577-1-sarunkod@amd.com/

Patch4
    - Disable interrupts while holding wakeup list lock inside [sashiko]
      avic_add_vcpu_to_gappi_wakeup_list and avic_remove_vcpu_from_gappi_wakeup_list
    - Unregister posted_intr_wakeup_handler during module unload [sashiko]

Patch 5
    - Disable GAPPI feature during kexec and suspend path [sashiko]

-------

Sairaj Kodilkar (5):
  iommu/amd: kvm/svm: Improve API between SVM and AMD IOMMU
  iommu/amd: Configure IRTE to use the GAPPI for posted interrupts
  kvm/svm: Introduce per-CPU lock and wakeup queue
  kvm/svm: Update the per-CPU wakeup-list during vCPU load and unload
  iommu/amd: Provide kernel command line option to enable GAPPI

[RFC PATCH v2 1/5] iommu/amd: kvm/svm: Improve API between SVM and AMD IOMMU

[Sairaj Kodilkar]

The name "cpu" in the parameter is misleading as it represents the
physical apicid of the cpu. Hence rename it.

Introduce flags to determine the state of the vCPU (running or not) and
posted interrupts. This is useful as following patch overloads the
apicid (formerly cpu) field to determine GAPPI destination when vCPU is
not running and it can no longer be used to determine if vCPU is running
or not.

Signed-off-by: Sairaj Kodilkar <sarunkod@amd.com>
---
 arch/x86/include/asm/irq_remapping.h |  4 ++--
 arch/x86/kvm/svm/avic.c              | 22 ++++++++++++++--------
 drivers/iommu/amd/iommu.c            | 24 ++++++++++++------------
 include/linux/amd-iommu.h            | 14 ++++++++++----
 4 files changed, 38 insertions(+), 26 deletions(-)

diff --git a/arch/x86/include/asm/irq_remapping.h b/arch/x86/include/asm/irq_remapping.h
index 37b94f484ef3..40a2206a33c0 100644
--- a/arch/x86/include/asm/irq_remapping.h
+++ b/arch/x86/include/asm/irq_remapping.h
@@ -35,8 +35,8 @@ struct amd_iommu_pi_data {
 	u64 vapic_addr;		/* Physical address of the vCPU's vAPIC. */
 	u32 ga_tag;
 	u32 vector;		/* Guest vector of the interrupt */
-	int cpu;
-	bool ga_log_intr;
+	int apicid;
+	int flags;
 	bool is_guest_mode;
 	void *ir_data;
 };
diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
index cdd5a6dc646f..7862b13c5409 100644
--- a/arch/x86/kvm/svm/avic.c
+++ b/arch/x86/kvm/svm/avic.c
@@ -932,6 +932,7 @@ int avic_pi_update_irte(struct kvm_kernel_irqfd *irqfd, struct kvm *kvm,
 		struct vcpu_svm *svm = to_svm(vcpu);
 		u64 entry;
 		int ret;
+		int posted_intr;
 
 		/*
 		 * Prevent the vCPU from being scheduled out or migrated until
@@ -949,10 +950,11 @@ int avic_pi_update_irte(struct kvm_kernel_irqfd *irqfd, struct kvm *kvm,
 		 */
 		entry = svm->avic_physical_id_entry;
 		if (entry & AVIC_PHYSICAL_ID_ENTRY_IS_RUNNING_MASK) {
-			pi_data.cpu = entry & AVIC_PHYSICAL_ID_ENTRY_HOST_PHYSICAL_ID_MASK;
+			pi_data.apicid = entry & AVIC_PHYSICAL_ID_ENTRY_HOST_PHYSICAL_ID_MASK;
+			pi_data.flags = AMD_IOMMU_FLAG_VCPU_RUNNING;
 		} else {
-			pi_data.cpu = -1;
-			pi_data.ga_log_intr = entry & AVIC_PHYSICAL_ID_ENTRY_GA_LOG_INTR;
+			posted_intr = !!(entry & AVIC_PHYSICAL_ID_ENTRY_GA_LOG_INTR);
+			pi_data.flags = posted_intr << AMD_IOMMU_FLAG_POSTED_INTR_SHIFT;
 		}
 
 		ret = irq_set_vcpu_affinity(host_irq, &pi_data);
@@ -1004,10 +1006,14 @@ enum avic_vcpu_action {
 	AVIC_START_BLOCKING	= BIT(1),
 };
 
-static void avic_update_iommu_vcpu_affinity(struct kvm_vcpu *vcpu, int cpu,
+static void avic_update_iommu_vcpu_affinity(struct kvm_vcpu *vcpu, int apicid,
 					    enum avic_vcpu_action action)
 {
-	bool ga_log_intr = (action & AVIC_START_BLOCKING);
+	int posted_intr = !!(action & AVIC_START_BLOCKING) <<
+			  AMD_IOMMU_FLAG_POSTED_INTR_SHIFT;
+	int is_vcpu_running = (apicid >= 0) <<
+			      AMD_IOMMU_FLAG_VCPU_RUNNING_SHIFT;
+	int flags = posted_intr | is_vcpu_running;
 	struct vcpu_svm *svm = to_svm(vcpu);
 	struct kvm_kernel_irqfd *irqfd;
 
@@ -1024,9 +1030,9 @@ static void avic_update_iommu_vcpu_affinity(struct kvm_vcpu *vcpu, int cpu,
 		void *data = irqfd->irq_bypass_data;
 
 		if (!(action & AVIC_TOGGLE_ON_OFF))
-			WARN_ON_ONCE(amd_iommu_update_ga(data, cpu, ga_log_intr));
-		else if (cpu >= 0)
-			WARN_ON_ONCE(amd_iommu_activate_guest_mode(data, cpu, ga_log_intr));
+			WARN_ON_ONCE(amd_iommu_update_ga(data, apicid, flags));
+		else if (is_vcpu_running)
+			WARN_ON_ONCE(amd_iommu_activate_guest_mode(data, apicid, flags));
 		else
 			WARN_ON_ONCE(amd_iommu_deactivate_guest_mode(data));
 	}
diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 57dc8fabc7d9..76f0e469490e 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -3959,19 +3959,19 @@ static const struct irq_domain_ops amd_ir_domain_ops = {
 	.deactivate = irq_remapping_deactivate,
 };
 
-static void __amd_iommu_update_ga(struct irte_ga *entry, int cpu,
-				  bool ga_log_intr)
+static void __amd_iommu_update_ga(struct irte_ga *entry, int apicid, int flags)
 {
-	if (cpu >= 0) {
+	if (flags & AMD_IOMMU_FLAG_VCPU_RUNNING) {
 		entry->lo.fields_vapic.destination =
-					APICID_TO_IRTE_DEST_LO(cpu);
+					APICID_TO_IRTE_DEST_LO(apicid);
 		entry->hi.fields.destination =
-					APICID_TO_IRTE_DEST_HI(cpu);
+					APICID_TO_IRTE_DEST_HI(apicid);
 		entry->lo.fields_vapic.is_run = true;
 		entry->lo.fields_vapic.ga_log_intr = false;
 	} else {
 		entry->lo.fields_vapic.is_run = false;
-		entry->lo.fields_vapic.ga_log_intr = ga_log_intr;
+		entry->lo.fields_vapic.ga_log_intr = !!(flags &
+							AMD_IOMMU_FLAG_POSTED_INTR);
 	}
 }
 
@@ -3992,7 +3992,7 @@ static void __amd_iommu_update_ga(struct irte_ga *entry, int cpu,
  * and thus don't require an invalidation to ensure the IOMMU consumes fresh
  * information.
  */
-int amd_iommu_update_ga(void *data, int cpu, bool ga_log_intr)
+int amd_iommu_update_ga(void *data, int apicid, int flags)
 {
 	struct amd_ir_data *ir_data = (struct amd_ir_data *)data;
 	struct irte_ga *entry = (struct irte_ga *) ir_data->entry;
@@ -4006,14 +4006,14 @@ int amd_iommu_update_ga(void *data, int cpu, bool ga_log_intr)
 	if (!ir_data->iommu)
 		return -ENODEV;
 
-	__amd_iommu_update_ga(entry, cpu, ga_log_intr);
+	__amd_iommu_update_ga(entry, apicid, flags);
 
 	return __modify_irte_ga(ir_data->iommu, ir_data->irq_2_irte.devid,
 				ir_data->irq_2_irte.index, entry);
 }
 EXPORT_SYMBOL(amd_iommu_update_ga);
 
-int amd_iommu_activate_guest_mode(void *data, int cpu, bool ga_log_intr)
+int amd_iommu_activate_guest_mode(void *data, int apicid, int flags)
 {
 	struct amd_ir_data *ir_data = (struct amd_ir_data *)data;
 	struct irte_ga *entry = (struct irte_ga *) ir_data->entry;
@@ -4036,7 +4036,7 @@ int amd_iommu_activate_guest_mode(void *data, int cpu, bool ga_log_intr)
 	entry->hi.fields.vector            = ir_data->ga_vector;
 	entry->lo.fields_vapic.ga_tag      = ir_data->ga_tag;
 
-	__amd_iommu_update_ga(entry, cpu, ga_log_intr);
+	__amd_iommu_update_ga(entry, apicid, flags);
 
 	return modify_irte_ga(ir_data->iommu, ir_data->irq_2_irte.devid,
 			      ir_data->irq_2_irte.index, entry);
@@ -4107,8 +4107,8 @@ static int amd_ir_set_vcpu_affinity(struct irq_data *data, void *info)
 		ir_data->ga_vector = pi_data->vector;
 		ir_data->ga_tag = pi_data->ga_tag;
 		if (pi_data->is_guest_mode)
-			ret = amd_iommu_activate_guest_mode(ir_data, pi_data->cpu,
-							    pi_data->ga_log_intr);
+			ret = amd_iommu_activate_guest_mode(ir_data, pi_data->apicid,
+							    pi_data->flags);
 		else
 			ret = amd_iommu_deactivate_guest_mode(ir_data);
 	} else {
diff --git a/include/linux/amd-iommu.h b/include/linux/amd-iommu.h
index edcee9f5335a..3dd9074e5967 100644
--- a/include/linux/amd-iommu.h
+++ b/include/linux/amd-iommu.h
@@ -30,8 +30,8 @@ static inline void amd_iommu_detect(void) { }
 /* IOMMU AVIC Function */
 extern int amd_iommu_register_ga_log_notifier(int (*notifier)(u32));
 
-extern int amd_iommu_update_ga(void *data, int cpu, bool ga_log_intr);
-extern int amd_iommu_activate_guest_mode(void *data, int cpu, bool ga_log_intr);
+extern int amd_iommu_update_ga(void *data, int apicid, int flags);
+extern int amd_iommu_activate_guest_mode(void *data, int apicid, int flags);
 extern int amd_iommu_deactivate_guest_mode(void *data);
 
 #else /* defined(CONFIG_AMD_IOMMU) && defined(CONFIG_IRQ_REMAP) */
@@ -42,12 +42,12 @@ amd_iommu_register_ga_log_notifier(int (*notifier)(u32))
 	return 0;
 }
 
-static inline int amd_iommu_update_ga(void *data, int cpu, bool ga_log_intr)
+static inline int amd_iommu_update_ga(void *data, int apicid, int flags)
 {
 	return 0;
 }
 
-static inline int amd_iommu_activate_guest_mode(void *data, int cpu, bool ga_log_intr)
+static inline int amd_iommu_activate_guest_mode(void *data, int apicid, int flags)
 {
 	return 0;
 }
@@ -76,4 +76,10 @@ static inline int amd_iommu_snp_disable(void) { return 0; }
 static inline bool amd_iommu_sev_tio_supported(void) { return false; }
 #endif
 
+#define AMD_IOMMU_FLAG_VCPU_RUNNING_SHIFT 0
+#define AMD_IOMMU_FLAG_VCPU_RUNNING  BIT(0)
+
+#define AMD_IOMMU_FLAG_POSTED_INTR_SHIFT 1
+#define AMD_IOMMU_FLAG_POSTED_INTR  BIT(1)
+
 #endif /* _ASM_X86_AMD_IOMMU_H */
-- 
2.34.1

[RFC PATCH v2 2/5] iommu/amd: Configure IRTE to use the GAPPI for posted interrupts

[Sairaj Kodilkar]

When GAPPI is enabled, a guest-mode IRTE with is_run clear delivers a
GAPPI interrupt to the physical APIC ID in the IRTE destination fields,
with the wake vector carried in ga_tag. Program the guest-activated IRTE
accordingly.

Reuse the POSTED_INTR_WAKEUP_VECTOR for gappi which is already reserved
for posted interrupt wakeup handler on x86 and is handled by IDTE entry
sysvec_kvm_posted_intr_wakeup_ipi. Intel VMX already uses same vector
for the wakeup case.

Signed-off-by: Sairaj Kodilkar <sarunkod@amd.com>
---
 arch/x86/kvm/svm/avic.c             |  5 +++++
 drivers/iommu/amd/amd_iommu.h       |  1 +
 drivers/iommu/amd/amd_iommu_types.h |  4 +++-
 drivers/iommu/amd/init.c            |  3 +++
 drivers/iommu/amd/iommu.c           | 22 ++++++++++++++++++----
 include/linux/amd-iommu.h           |  1 +
 6 files changed, 31 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
index 7862b13c5409..b666efb5d91c 100644
--- a/arch/x86/kvm/svm/avic.c
+++ b/arch/x86/kvm/svm/avic.c
@@ -955,6 +955,11 @@ int avic_pi_update_irte(struct kvm_kernel_irqfd *irqfd, struct kvm *kvm,
 		} else {
 			posted_intr = !!(entry & AVIC_PHYSICAL_ID_ENTRY_GA_LOG_INTR);
 			pi_data.flags = posted_intr << AMD_IOMMU_FLAG_POSTED_INTR_SHIFT;
+			/* GAPPI is disabled at this point (amd_iommu_gappi is
+			 * enabled in the following patches) hence keep the
+			 * apicid as 0.
+			 */
+			pi_data.apicid = 0;
 		}
 
 		ret = irq_set_vcpu_affinity(host_irq, &pi_data);
diff --git a/drivers/iommu/amd/amd_iommu.h b/drivers/iommu/amd/amd_iommu.h
index 834d8fabfba3..044179cab12e 100644
--- a/drivers/iommu/amd/amd_iommu.h
+++ b/drivers/iommu/amd/amd_iommu.h
@@ -41,6 +41,7 @@ int amd_iommu_enable(void);
 void amd_iommu_disable(void);
 int amd_iommu_reenable(int mode);
 int amd_iommu_enable_faulting(unsigned int cpu);
+extern bool amd_iommu_gappi;
 extern int amd_iommu_guest_ir;
 extern enum protection_domain_mode amd_iommu_pgtable;
 extern int amd_iommu_gpt_level;
diff --git a/drivers/iommu/amd/amd_iommu_types.h b/drivers/iommu/amd/amd_iommu_types.h
index f9f718087893..26d7a9796e64 100644
--- a/drivers/iommu/amd/amd_iommu_types.h
+++ b/drivers/iommu/amd/amd_iommu_types.h
@@ -113,6 +113,7 @@
 /* Extended Feature 2 Bits */
 #define FEATURE_SEVSNPIO_SUP	BIT_ULL(1)
 #define FEATURE_GCR3TRPMODE	BIT_ULL(3)
+#define FEATURE_GAPPIDISSUP	BIT_ULL(4)
 #define FEATURE_SNPAVICSUP	GENMASK_ULL(7, 5)
 #define FEATURE_SNPAVICSUP_GAM(x) \
 	(FIELD_GET(FEATURE_SNPAVICSUP, x) == 0x1)
@@ -1004,7 +1005,8 @@ union irte_ga_lo {
 		    no_fault	: 1,
 		    /* ------ */
 		    ga_log_intr	: 1,
-		    rsvd1	: 3,
+		    rsvd1	: 2,
+		    gappi_dis	: 1,
 		    is_run	: 1,
 		    /* ------ */
 		    guest_mode	: 1,
diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
index 3bdb380d23e9..2e1889f8a9e4 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -160,6 +160,9 @@ u8 amd_iommu_hpt_level;
 /* Guest page table level */
 int amd_iommu_gpt_level = PAGE_MODE_4_LEVEL;
 
+bool amd_iommu_gappi;
+EXPORT_SYMBOL(amd_iommu_gappi);
+
 int amd_iommu_guest_ir = AMD_IOMMU_GUEST_IR_VAPIC;
 static int amd_iommu_xt_mode = IRQ_REMAP_XAPIC_MODE;
 
diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 76f0e469490e..b226d3941cb9 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -3969,9 +3969,19 @@ static void __amd_iommu_update_ga(struct irte_ga *entry, int apicid, int flags)
 		entry->lo.fields_vapic.is_run = true;
 		entry->lo.fields_vapic.ga_log_intr = false;
 	} else {
-		entry->lo.fields_vapic.is_run = false;
-		entry->lo.fields_vapic.ga_log_intr = !!(flags &
-							AMD_IOMMU_FLAG_POSTED_INTR);
+		bool posted_intr = !!(flags & AMD_IOMMU_FLAG_POSTED_INTR);
+
+		if (amd_iommu_gappi) {
+			entry->lo.fields_vapic.gappi_dis = !posted_intr &&
+							   check_feature2(FEATURE_GAPPIDISSUP);
+			entry->lo.fields_vapic.is_run = false;
+			entry->lo.fields_vapic.destination =
+						APICID_TO_IRTE_DEST_LO(apicid);
+			entry->hi.fields.destination = APICID_TO_IRTE_DEST_HI(apicid);
+		} else {
+			entry->lo.fields_vapic.is_run = false;
+			entry->lo.fields_vapic.ga_log_intr = posted_intr;
+		}
 	}
 }
 
@@ -4034,7 +4044,11 @@ int amd_iommu_activate_guest_mode(void *data, int apicid, int flags)
 	entry->lo.fields_vapic.guest_mode  = 1;
 	entry->hi.fields.ga_root_ptr       = ir_data->ga_root_ptr;
 	entry->hi.fields.vector            = ir_data->ga_vector;
-	entry->lo.fields_vapic.ga_tag      = ir_data->ga_tag;
+
+	if (amd_iommu_gappi)
+		entry->lo.fields_vapic.ga_tag = POSTED_INTR_WAKEUP_VECTOR;
+	else
+		entry->lo.fields_vapic.ga_tag = ir_data->ga_tag;
 
 	__amd_iommu_update_ga(entry, apicid, flags);
 
diff --git a/include/linux/amd-iommu.h b/include/linux/amd-iommu.h
index 3dd9074e5967..87e76f617ea1 100644
--- a/include/linux/amd-iommu.h
+++ b/include/linux/amd-iommu.h
@@ -82,4 +82,5 @@ static inline bool amd_iommu_sev_tio_supported(void) { return false; }
 #define AMD_IOMMU_FLAG_POSTED_INTR_SHIFT 1
 #define AMD_IOMMU_FLAG_POSTED_INTR  BIT(1)
 
+extern bool amd_iommu_gappi;
 #endif /* _ASM_X86_AMD_IOMMU_H */
-- 
2.34.1

[RFC PATCH v2 3/5] kvm/svm: Introduce per-CPU lock and wakeup queue

[Sairaj Kodilkar]

Add a per-CPU list and raw spinlock that SVM will use to track vCPUs
blocked and waiting for a wakeup.  The GAPPI interrupt handler will
scan the corresponding CPU’s list to decide which vCPU to run.
Initialize both in avic_hardware_setup() when AVIC is enabled.

Signed-off-by: Sairaj Kodilkar <sarunkod@amd.com>
---
 arch/x86/kvm/svm/avic.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
index b666efb5d91c..e7a4c0e90e7a 100644
--- a/arch/x86/kvm/svm/avic.c
+++ b/arch/x86/kvm/svm/avic.c
@@ -63,6 +63,9 @@
 	ga_tag;								\
 })
 
+static DEFINE_PER_CPU(struct list_head, gappi_vcpu_wakeup_list);
+static DEFINE_PER_CPU(raw_spinlock_t, gappi_vcpu_wakeup_list_lock);
+
 static_assert(__AVIC_GATAG(AVIC_VM_ID_MASK, AVIC_VCPU_IDX_MASK) == -1u);
 
 #define AVIC_AUTO_MODE -1
@@ -1315,12 +1318,18 @@ static bool __init avic_want_avic_enabled(void)
  */
 bool __init avic_hardware_setup(void)
 {
+	int cpu;
 	avic = avic_want_avic_enabled();
 	if (!avic)
 		return false;
 
 	pr_info("AVIC enabled\n");
 
+	for_each_possible_cpu(cpu) {
+		INIT_LIST_HEAD(&per_cpu(gappi_vcpu_wakeup_list, cpu));
+		raw_spin_lock_init(&per_cpu(gappi_vcpu_wakeup_list_lock, cpu));
+	}
+
 	/* AVIC is a prerequisite for x2AVIC. */
 	x2avic_enabled = boot_cpu_has(X86_FEATURE_X2AVIC);
 	if (x2avic_enabled) {
-- 
2.34.1

[RFC PATCH v2 4/5] kvm/svm: Update the per-CPU wakeup-list during vCPU load and unload

[Sairaj Kodilkar]

When a vCPU is unloaded from a physical CPU enqueue it on that CPU's
GAPPI wakeup list. Remove the vCPU from the wakeup list when it is loaded
on to a CPU again.

Also enqueue from avic_pi_update_irte() when vCPU is not running and
ir_list is still empty.  This handles the condition where vCPU load skips
the per-CPU wakeup-list update when ir_list is empty.

The GAPPI wakeup handler walks this CPU's list and wakes vCPUs that still
have a pending IRR.  Install it with kvm_set_posted_intr_wakeup_handler()
so deliveries on POSTED_INTR_WAKEUP_VECTOR invoke it.

Signed-off-by: Sairaj Kodilkar <sarunkod@amd.com>
---
 arch/x86/kvm/svm/avic.c | 115 ++++++++++++++++++++++++++++++++++------
 arch/x86/kvm/svm/svm.c  |   2 +
 arch/x86/kvm/svm/svm.h  |   5 ++
 3 files changed, 107 insertions(+), 15 deletions(-)

diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
index e7a4c0e90e7a..8185841d9b81 100644
--- a/arch/x86/kvm/svm/avic.c
+++ b/arch/x86/kvm/svm/avic.c
@@ -877,6 +877,9 @@ int avic_init_vcpu(struct vcpu_svm *svm)
 	INIT_LIST_HEAD(&svm->ir_list);
 	raw_spin_lock_init(&svm->ir_list_lock);
 
+	INIT_LIST_HEAD(&svm->gappi_vcpu_wakeup_list);
+	svm->gappi_cpu = -1;
+
 	if (!enable_apicv || !irqchip_in_kernel(vcpu->kvm))
 		return 0;
 
@@ -889,6 +892,42 @@ int avic_init_vcpu(struct vcpu_svm *svm)
 	return ret;
 }
 
+static void avic_add_vcpu_to_gappi_wakeup_list(struct vcpu_svm *svm, int cpu)
+{
+	struct list_head *wakeup_list;
+	raw_spinlock_t *spinlock;
+
+	if (WARN_ON(cpu < 0))
+		return;
+
+	wakeup_list = &per_cpu(gappi_vcpu_wakeup_list, cpu);
+	spinlock = &per_cpu(gappi_vcpu_wakeup_list_lock, cpu);
+	guard(raw_spinlock_irqsave)(spinlock);
+	if (list_empty(&svm->gappi_vcpu_wakeup_list))
+		list_add_tail(&svm->gappi_vcpu_wakeup_list, wakeup_list);
+}
+
+static void avic_remove_vcpu_from_gappi_wakeup_list(struct vcpu_svm *svm, int cpu)
+{
+	raw_spinlock_t *spinlock;
+
+	if (WARN_ON(cpu < 0))
+		return;
+
+	spinlock = &per_cpu(gappi_vcpu_wakeup_list_lock, cpu);
+	guard(raw_spinlock_irqsave)(spinlock);
+	if (!list_empty(&svm->gappi_vcpu_wakeup_list))
+		list_del_init(&svm->gappi_vcpu_wakeup_list);
+}
+
+void avic_destroy_vcpu(struct vcpu_svm *svm)
+{
+	if (svm->gappi_cpu != -1 && amd_iommu_gappi) {
+		avic_remove_vcpu_from_gappi_wakeup_list(svm, svm->gappi_cpu);
+		svm->gappi_cpu = -1;
+	}
+}
+
 void avic_apicv_post_state_restore(struct kvm_vcpu *vcpu)
 {
 	avic_handle_dfr_update(vcpu);
@@ -899,13 +938,18 @@ static void svm_ir_list_del(struct kvm_kernel_irqfd *irqfd)
 {
 	struct kvm_vcpu *vcpu = irqfd->irq_bypass_vcpu;
 	unsigned long flags;
+	struct vcpu_svm *svm;
 
 	if (!vcpu)
 		return;
 
-	raw_spin_lock_irqsave(&to_svm(vcpu)->ir_list_lock, flags);
+	svm = to_svm(vcpu);
+
+	raw_spin_lock_irqsave(&svm->ir_list_lock, flags);
 	list_del(&irqfd->vcpu_list);
-	raw_spin_unlock_irqrestore(&to_svm(vcpu)->ir_list_lock, flags);
+	if (list_empty(&svm->ir_list))
+		avic_remove_vcpu_from_gappi_wakeup_list(svm, svm->gappi_cpu);
+	raw_spin_unlock_irqrestore(&svm->ir_list_lock, flags);
 }
 
 int avic_pi_update_irte(struct kvm_kernel_irqfd *irqfd, struct kvm *kvm,
@@ -936,6 +980,7 @@ int avic_pi_update_irte(struct kvm_kernel_irqfd *irqfd, struct kvm *kvm,
 		u64 entry;
 		int ret;
 		int posted_intr;
+		bool is_vcpu_waiting = false;
 
 		/*
 		 * Prevent the vCPU from being scheduled out or migrated until
@@ -958,16 +1003,18 @@ int avic_pi_update_irte(struct kvm_kernel_irqfd *irqfd, struct kvm *kvm,
 		} else {
 			posted_intr = !!(entry & AVIC_PHYSICAL_ID_ENTRY_GA_LOG_INTR);
 			pi_data.flags = posted_intr << AMD_IOMMU_FLAG_POSTED_INTR_SHIFT;
-			/* GAPPI is disabled at this point (amd_iommu_gappi is
-			 * enabled in the following patches) hence keep the
-			 * apicid as 0.
-			 */
-			pi_data.apicid = 0;
+			if (amd_iommu_gappi) {
+				pi_data.apicid = kvm_cpu_get_apicid(svm->gappi_cpu);
+				if (list_empty(&svm->ir_list)) {
+					avic_add_vcpu_to_gappi_wakeup_list(svm, svm->gappi_cpu);
+					is_vcpu_waiting = true;
+				}
+			}
 		}
 
 		ret = irq_set_vcpu_affinity(host_irq, &pi_data);
 		if (ret)
-			return ret;
+			goto gappi_err_out;
 
 		/*
 		 * Revert to legacy mode if the IOMMU didn't provide metadata
@@ -976,12 +1023,17 @@ int avic_pi_update_irte(struct kvm_kernel_irqfd *irqfd, struct kvm *kvm,
 		 */
 		if (WARN_ON_ONCE(!pi_data.ir_data)) {
 			irq_set_vcpu_affinity(host_irq, NULL);
-			return -EIO;
+			ret = -EIO;
+			goto gappi_err_out;
 		}
 
 		irqfd->irq_bypass_data = pi_data.ir_data;
 		list_add(&irqfd->vcpu_list, &svm->ir_list);
 		return 0;
+gappi_err_out:
+		if (is_vcpu_waiting)
+			avic_remove_vcpu_from_gappi_wakeup_list(svm, svm->gappi_cpu);
+		return ret;
 	}
 	return irq_set_vcpu_affinity(host_irq, NULL);
 }
@@ -1015,7 +1067,7 @@ enum avic_vcpu_action {
 };
 
 static void avic_update_iommu_vcpu_affinity(struct kvm_vcpu *vcpu, int apicid,
-					    enum avic_vcpu_action action)
+					    int cpu, enum avic_vcpu_action action)
 {
 	int posted_intr = !!(action & AVIC_START_BLOCKING) <<
 			  AMD_IOMMU_FLAG_POSTED_INTR_SHIFT;
@@ -1031,8 +1083,22 @@ static void avic_update_iommu_vcpu_affinity(struct kvm_vcpu *vcpu, int apicid,
 	 * Here, we go through the per-vcpu ir_list to update all existing
 	 * interrupt remapping table entry targeting this vcpu.
 	 */
-	if (list_empty(&svm->ir_list))
+	if (list_empty(&svm->ir_list)) {
+		if (amd_iommu_gappi && cpu >= 0)
+			svm->gappi_cpu = cpu;
 		return;
+	}
+
+	if (is_vcpu_running && amd_iommu_gappi) {
+		/* IF condition handles the initial state */
+		if (svm->gappi_cpu != -1)
+			avic_remove_vcpu_from_gappi_wakeup_list(svm, svm->gappi_cpu);
+
+		svm->gappi_cpu = cpu; /* Store cpu no as target for GAPPI */
+	} else if (amd_iommu_gappi) {
+		apicid = kvm_cpu_get_apicid(svm->gappi_cpu);
+		avic_add_vcpu_to_gappi_wakeup_list(svm, svm->gappi_cpu);
+	}
 
 	list_for_each_entry(irqfd, &svm->ir_list, vcpu_list) {
 		void *data = irqfd->irq_bypass_data;
@@ -1094,7 +1160,7 @@ static void __avic_vcpu_load(struct kvm_vcpu *vcpu, int cpu,
 
 	WRITE_ONCE(kvm_svm->avic_physical_id_table[vcpu->vcpu_id], entry);
 
-	avic_update_iommu_vcpu_affinity(vcpu, h_physical_id, action);
+	avic_update_iommu_vcpu_affinity(vcpu, h_physical_id, cpu, action);
 
 	raw_spin_unlock_irqrestore(&svm->ir_list_lock, flags);
 }
@@ -1137,7 +1203,7 @@ static void __avic_vcpu_put(struct kvm_vcpu *vcpu, enum avic_vcpu_action action)
 	 */
 	raw_spin_lock_irqsave(&svm->ir_list_lock, flags);
 
-	avic_update_iommu_vcpu_affinity(vcpu, -1, action);
+	avic_update_iommu_vcpu_affinity(vcpu, -1, -1, action);
 
 	WARN_ON_ONCE(entry & AVIC_PHYSICAL_ID_ENTRY_GA_LOG_INTR);
 
@@ -1310,6 +1376,21 @@ static bool __init avic_want_avic_enabled(void)
 	return true;
 }
 
+static void avic_gappi_wakeup_handler(void)
+{
+	int cpu = smp_processor_id();
+	struct list_head *vcpu_wakeup_list = &per_cpu(gappi_vcpu_wakeup_list, cpu);
+	raw_spinlock_t *spinlock = &per_cpu(gappi_vcpu_wakeup_list_lock, cpu);
+	struct vcpu_svm *svm;
+
+	raw_spin_lock(spinlock);
+	list_for_each_entry(svm, vcpu_wakeup_list, gappi_vcpu_wakeup_list) {
+		if (kvm_lapic_find_highest_irr(&svm->vcpu) >= 0)
+			kvm_vcpu_wake_up(&svm->vcpu);
+	}
+	raw_spin_unlock(spinlock);
+}
+
 /*
  * Note:
  * - The module param avic enable both xAPIC and x2APIC mode.
@@ -1353,12 +1434,16 @@ bool __init avic_hardware_setup(void)
 		enable_ipiv = false;
 
 	amd_iommu_register_ga_log_notifier(&avic_ga_log_notifier);
+	kvm_set_posted_intr_wakeup_handler(&avic_gappi_wakeup_handler);
 
 	return true;
 }
 
 void avic_hardware_unsetup(void)
 {
-	if (avic)
-		amd_iommu_register_ga_log_notifier(NULL);
+	if (!avic)
+		return;
+
+	amd_iommu_register_ga_log_notifier(NULL);
+	kvm_set_posted_intr_wakeup_handler(NULL);
 }
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index e02a38da5296..b687133f8528 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -1356,6 +1356,8 @@ static void svm_vcpu_free(struct kvm_vcpu *vcpu)
 
 	WARN_ON_ONCE(!list_empty(&svm->ir_list));
 
+	avic_destroy_vcpu(svm);
+
 	svm_leave_nested(vcpu);
 	svm_free_nested(svm);
 
diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
index 5137416be593..47d5bb5d7103 100644
--- a/arch/x86/kvm/svm/svm.h
+++ b/arch/x86/kvm/svm/svm.h
@@ -362,6 +362,10 @@ struct vcpu_svm {
 
 	/* Guest GIF value, used when vGIF is not enabled */
 	bool guest_gif;
+
+	/* GAPPI related fields */
+	struct list_head gappi_vcpu_wakeup_list;
+	int gappi_cpu;
 };
 
 struct svm_cpu_data {
@@ -909,6 +913,7 @@ void avic_init_vmcb(struct vcpu_svm *svm, struct vmcb *vmcb);
 int avic_incomplete_ipi_interception(struct kvm_vcpu *vcpu);
 int avic_unaccelerated_access_interception(struct kvm_vcpu *vcpu);
 int avic_init_vcpu(struct vcpu_svm *svm);
+void avic_destroy_vcpu(struct vcpu_svm *svm);
 void avic_vcpu_load(struct kvm_vcpu *vcpu, int cpu);
 void avic_vcpu_put(struct kvm_vcpu *vcpu);
 void avic_apicv_post_state_restore(struct kvm_vcpu *vcpu);
-- 
2.34.1

[RFC PATCH v2 5/5] iommu/amd: Provide kernel command line option to enable GAPPI

[Sairaj Kodilkar]

Enable GAPPI when IOMMU GAPPISup extended feature bit is set and
the kernel is booted with "amd_iommu=gappi" command line parameter.

Co-developed-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Signed-off-by: Sairaj Kodilkar <sarunkod@amd.com>
---
 drivers/iommu/amd/amd_iommu_types.h |  2 ++
 drivers/iommu/amd/init.c            | 27 ++++++++++++++++++++++++---
 2 files changed, 26 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/amd/amd_iommu_types.h b/drivers/iommu/amd/amd_iommu_types.h
index 26d7a9796e64..de0b23306944 100644
--- a/drivers/iommu/amd/amd_iommu_types.h
+++ b/drivers/iommu/amd/amd_iommu_types.h
@@ -94,6 +94,7 @@
 #define FEATURE_X2APIC		BIT_ULL(2)
 #define FEATURE_NX		BIT_ULL(3)
 #define FEATURE_GT		BIT_ULL(4)
+#define FEATURE_GAPPI		BIT_ULL(5)
 #define FEATURE_IA		BIT_ULL(6)
 #define FEATURE_GA		BIT_ULL(7)
 #define FEATURE_HE		BIT_ULL(8)
@@ -191,6 +192,7 @@
 #define CONTROL_EPH_EN		45
 #define CONTROL_XT_EN		50
 #define CONTROL_INTCAPXT_EN	51
+#define CONTROL_GAPPI_EN	55
 #define CONTROL_GCR3TRPMODE	58
 #define CONTROL_IRTCACHEDIS	59
 #define CONTROL_SNPAVIC_EN	61
diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
index 2e1889f8a9e4..27ffb2184bf2 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -465,6 +465,9 @@ static void iommu_disable(struct amd_iommu *iommu)
 	iommu_feature_disable(iommu, CONTROL_GALOG_EN);
 	iommu_feature_disable(iommu, CONTROL_GAINT_EN);
 
+	/* Disable IOMMU GAPPI */
+	iommu_feature_disable(iommu, CONTROL_GAPPI_EN);
+
 	/* Disable IOMMU PPR logging */
 	iommu_feature_disable(iommu, CONTROL_PPRLOG_EN);
 	iommu_feature_disable(iommu, CONTROL_PPRINT_EN);
@@ -2999,6 +3002,12 @@ static void enable_iommus_vapic(void)
 	struct amd_iommu *iommu;
 
 	for_each_iommu(iommu) {
+		/* Disable GAPPI, do not check amd_iommu_gappi as it may be
+		 * false in new kexec kernel even though previous kernel has
+		 * enabled it.
+		 */
+		iommu_feature_disable(iommu, CONTROL_GAPPI_EN);
+
 		/*
 		 * Disable GALog if already running. It could have been enabled
 		 * in the previous boot before kdump.
@@ -3038,10 +3047,19 @@ static void enable_iommus_vapic(void)
 		return;
 	}
 
+	if (amd_iommu_gappi &&
+	    !(check_feature(FEATURE_GAPPI) &&
+	      AMD_IOMMU_GUEST_IR_VAPIC(amd_iommu_guest_ir))) {
+		pr_warn("GAPPI is not supported.\n");
+		amd_iommu_gappi = false;
+	}
+
 	/* Enabling GAM and SNPAVIC support */
 	for_each_iommu(iommu) {
-		if (iommu_init_ga_log(iommu) ||
-		    iommu_ga_log_enable(iommu))
+		if (amd_iommu_gappi)
+			iommu_feature_enable(iommu, CONTROL_GAPPI_EN);
+		else if (iommu_init_ga_log(iommu) ||
+			 iommu_ga_log_enable(iommu))
 			return;
 
 		iommu_feature_enable(iommu, CONTROL_GAM_EN);
@@ -3050,7 +3068,8 @@ static void enable_iommus_vapic(void)
 	}
 
 	amd_iommu_irq_ops.capability |= (1 << IRQ_POSTING_CAP);
-	pr_info("Virtual APIC enabled\n");
+	pr_info("Virtual APIC enabled with %s\n",
+		amd_iommu_gappi ? "GAPPI" : "GALOG");
 #endif
 }
 
@@ -3741,6 +3760,8 @@ static int __init parse_amd_iommu_options(char *str)
 		} else if (strncmp(str, "v2_pgsizes_only", 15) == 0) {
 			pr_info("Restricting V1 page-sizes to 4KiB/2MiB/1GiB");
 			amd_iommu_pgsize_bitmap = AMD_IOMMU_PGSIZES_V2;
+		} else if (strncmp(str, "gappi", 5) == 0) {
+			amd_iommu_gappi = true;
 		} else {
 			pr_notice("Unknown option - '%s'\n", str);
 		}
-- 
2.34.1

Re: [RFC PATCH v2 4/5] kvm/svm: Update the per-CPU wakeup-list during vCPU load and unload

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Missing CPU hotplug teardown logic for `gappi_vcpu_wakeup_list` causes indefinitely stalled vCPUs when a physical CPU is taken offline.
- [High] Host-userspace reachable `WARN_ON(cpu < 0)` via uninitialized `svm->gappi_cpu` when setting up posted interrupts for AVIC-inhibited VMs.
- [High] Unconditional call to `avic_remove_vcpu_from_gappi_wakeup_list()` triggers a spurious `WARN_ON` when GAPPI is disabled.
- [Medium] Mixing `goto`-based error handling with `guard()` scope-based cleanup in `avic_pi_update_irte()` violates subsystem guidelines.
--

--- Patch [4]: [RFC PATCH v2 4/5] kvm/svm: Update the per-CPU wakeup-list during vCPU load and unload ---
commit 943f44c8b99a66a1765989cd046e31b1a7a04469
Author: Sairaj Kodilkar <sarunkod@amd.com>

kvm/svm: Update the per-CPU wakeup-list during vCPU load and unload

When a vCPU is unloaded from a physical CPU enqueue it on that CPU's
GAPPI wakeup list. Remove the vCPU from the wakeup list when it is loaded
on to a CPU again.

Also enqueue from avic_pi_update_irte() when vCPU is not running and
ir_list is still empty.  This handles the condition where vCPU load skips
the per-CPU wakeup-list update when ir_list is empty.

The GAPPI wakeup handler walks this CPU's list and wakes vCPUs that still
have a pending IRR.  Install it with kvm_set_posted_intr_wakeup_handler()
so deliveries on POSTED_INTR_WAKEUP_VECTOR invoke it.

> diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
> index e7a4c0e90e7a7..8185841d9b816 100644
> --- a/arch/x86/kvm/svm/avic.c
> +++ b/arch/x86/kvm/svm/avic.c

[ ... ]

> @@ -899,13 +938,18 @@ static void svm_ir_list_del(struct kvm_kernel_irqfd *irqfd)
>  {
>  	struct kvm_vcpu *vcpu = irqfd->irq_bypass_vcpu;
>  	unsigned long flags;
> +	struct vcpu_svm *svm;
>  
>  	if (!vcpu)
>  		return;
>  
> -	raw_spin_lock_irqsave(&to_svm(vcpu)->ir_list_lock, flags);
> +	svm = to_svm(vcpu);
> +
> +	raw_spin_lock_irqsave(&svm->ir_list_lock, flags);
>  	list_del(&irqfd->vcpu_list);
> -	raw_spin_unlock_irqrestore(&to_svm(vcpu)->ir_list_lock, flags);
> +	if (list_empty(&svm->ir_list))
> +		avic_remove_vcpu_from_gappi_wakeup_list(svm, svm->gappi_cpu);

[Severity: High]
If GAPPI is globally disabled (amd_iommu_gappi == false), svm->gappi_cpu
remains at its initial value of -1 for the vCPU's entire lifetime.

When the last irqfd is deleted in svm_ir_list_del(), this calls
avic_remove_vcpu_from_gappi_wakeup_list() unconditionally without checking
if GAPPI is actually enabled.

Since cpu is -1, does this trigger the WARN_ON(cpu < 0) when host userspace
detaches an irqfd or shuts down a VM with assigned devices?

> +	raw_spin_unlock_irqrestore(&svm->ir_list_lock, flags);
>  }
>  
>  int avic_pi_update_irte(struct kvm_kernel_irqfd *irqfd, struct kvm *kvm,

[ ... ]

> @@ -958,16 +1003,18 @@ int avic_pi_update_irte(struct kvm_kernel_irqfd *irqfd, struct kvm *kvm,
>  		} else {
>  			posted_intr = !!(entry & AVIC_PHYSICAL_ID_ENTRY_GA_LOG_INTR);
>  			pi_data.flags = posted_intr << AMD_IOMMU_FLAG_POSTED_INTR_SHIFT;
> -			/* GAPPI is disabled at this point (amd_iommu_gappi is
> -			 * enabled in the following patches) hence keep the
> -			 * apicid as 0.
> -			 */
> -			pi_data.apicid = 0;
> +			if (amd_iommu_gappi) {
> +				pi_data.apicid = kvm_cpu_get_apicid(svm->gappi_cpu);
> +				if (list_empty(&svm->ir_list)) {
> +					avic_add_vcpu_to_gappi_wakeup_list(svm, svm->gappi_cpu);
> +					is_vcpu_waiting = true;
> +				}
> +			}
>  		}

[Severity: High]
svm->gappi_cpu is initialized to -1. If AVIC is disabled or inhibited for
the VM, kvm_vcpu_apicv_active() is false and avic_vcpu_load() is never
called, leaving svm->gappi_cpu as -1.

When userspace configures an IRQFD and the global amd_iommu_gappi flag is
true, this passes -1 to avic_add_vcpu_to_gappi_wakeup_list().

Could this trigger the host-userspace reachable WARN_ON(cpu < 0) in
avic_add_vcpu_to_gappi_wakeup_list()?

>  
>  		ret = irq_set_vcpu_affinity(host_irq, &pi_data);
>  		if (ret)
> -			return ret;
> +			goto gappi_err_out;

[Severity: Medium]
This introduces goto-based error handling in avic_pi_update_irte(), which
already uses guard(raw_spinlock_irqsave)(&svm->ir_list_lock) earlier in
the function.

Does mixing goto-based cleanup labels and guard() declarations violate the
subsystem guidelines for cleanup function compatibility?

>  
>  		/*
>  		 * Revert to legacy mode if the IOMMU didn't provide metadata

[ ... ]

> @@ -1310,6 +1376,21 @@ static bool __init avic_want_avic_enabled(void)
>  	return true;
>  }
>  
> +static void avic_gappi_wakeup_handler(void)
> +{
> +	int cpu = smp_processor_id();
> +	struct list_head *vcpu_wakeup_list = &per_cpu(gappi_vcpu_wakeup_list, cpu);
> +	raw_spinlock_t *spinlock = &per_cpu(gappi_vcpu_wakeup_list_lock, cpu);
> +	struct vcpu_svm *svm;
> +
> +	raw_spin_lock(spinlock);
> +	list_for_each_entry(svm, vcpu_wakeup_list, gappi_vcpu_wakeup_list) {
> +		if (kvm_lapic_find_highest_irr(&svm->vcpu) >= 0)
> +			kvm_vcpu_wake_up(&svm->vcpu);
> +	}
> +	raw_spin_unlock(spinlock);
> +}
> +

[Severity: High]
The per-CPU wakeup list is processed here in avic_gappi_wakeup_handler(),
but this handler will never run for an offline CPU.

If a physical CPU is hot-unplugged, the IOMMU's GAPPI hardware doorbell
interrupts sent to that CPU are dropped.

Since there is no CPU hotplug teardown logic registered (e.g., via
cpuhp_setup_state() in avic_hardware_setup()) to drain the list or migrate
the vCPUs to an online CPU, will any vCPUs left on its list miss their
wakeup events and stall indefinitely?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260708091408.12106-1-sarunkod@amd.com?part=4

Re: [RFC PATCH v2 2/5] iommu/amd: Configure IRTE to use the GAPPI for posted interrupts

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [Low] Duplicate declaration of `extern bool amd_iommu_gappi` in both public and private headers.

Pre-existing issues:
- [High] AMD IOMMU IRTE updates can be silently lost because `try_cmpxchg128` is not enclosed in a retry loop.
- [High] Concurrent lockless read-modify-write of `ir_data->entry` bitfields between KVM and the IRQ core causes data corruption.
--

--- Patch [2]: [RFC PATCH v2 2/5] iommu/amd: Configure IRTE to use the GAPPI for posted interrupts ---
commit 8796a3bab1c73d13bf17fdf71bd3ce4a5028e70d
Author: Sairaj Kodilkar <sarunkod@amd.com>

iommu/amd: Configure IRTE to use the GAPPI for posted interrupts

This commit configures the IRTE for guest-mode to use GAPPI for posted
interrupts when GAPPI is enabled. It uses the POSTED_INTR_WAKEUP_VECTOR
for the wake vector, which is carried in ga_tag. The patch modifies
the guest-activated IRTE programming to set the proper apicid and
gappi_dis fields.

> diff --git a/drivers/iommu/amd/amd_iommu.h b/drivers/iommu/amd/amd_iommu.h
> index a2fe804b038b6..d0dcaa4d5fe05 100644
> --- a/drivers/iommu/amd/amd_iommu.h
> +++ b/drivers/iommu/amd/amd_iommu.h
> @@ -41,6 +41,7 @@ int amd_iommu_enable(void);
>  void amd_iommu_disable(void);
>  int amd_iommu_reenable(int mode);
>  int amd_iommu_enable_faulting(unsigned int cpu);
> +extern bool amd_iommu_gappi;

[Severity: Low]
Is it necessary to declare this in the internal driver header?

I noticed extern bool amd_iommu_gappi; is also correctly added to the public 
header in include/linux/amd-iommu.h in this patch.

[ ... ]

> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
> index 51a2bd5435412..0040bced92ef9 100644
> --- a/drivers/iommu/amd/iommu.c
> +++ b/drivers/iommu/amd/iommu.c
[ ... ]
> @@ -4021,7 +4031,11 @@ int amd_iommu_activate_guest_mode(void *data, int apicid, int flags)
>  	entry->lo.fields_vapic.guest_mode  = 1;
>  	entry->hi.fields.ga_root_ptr       = ir_data->ga_root_ptr;
>  	entry->hi.fields.vector            = ir_data->ga_vector;
> -	entry->lo.fields_vapic.ga_tag      = ir_data->ga_tag;
> +
> +	if (amd_iommu_gappi)
> +		entry->lo.fields_vapic.ga_tag = POSTED_INTR_WAKEUP_VECTOR;
> +	else
> +		entry->lo.fields_vapic.ga_tag = ir_data->ga_tag;
>  
>  	__amd_iommu_update_ga(entry, apicid, flags);
>  

[Severity: High]
This isn't a bug introduced by this patch, but can AMD IOMMU IRTE updates
be silently lost here?

Looking at __modify_irte_ga():

drivers/iommu/amd/iommu.c:__modify_irte_ga() {
    ...
    old = entry->irte;
    WARN_ON(!try_cmpxchg128(&entry->irte, &old, irte->irte));
    ...
}

Does the single try_cmpxchg128() without a retry loop silently drop software
updates on failure?

Could it also clobber asynchronous hardware updates to the IRTE on success
because it writes back a stale software shadow copy?

[Severity: High]
This is a pre-existing issue, but can this concurrent lockless
read-modify-write of ir_data->entry bitfields cause data corruption?

KVM modifies the shadow copy bitfields using only svm->ir_list_lock:

drivers/iommu/amd/iommu.c:amd_iommu_deactivate_guest_mode() {
    ...
    entry->lo.val = 0;
    entry->hi.val = 0;

    entry->lo.fields_remap.valid       = valid;
    ...
}

While the IRQ core concurrently modifies the same 64-bit word bitfields under
desc->lock:

drivers/iommu/amd/iommu.c:irte_ga_set_affinity() {
    ...
    if (!irte->lo.fields_remap.guest_mode) {
        irte->hi.fields.vector = vector;
    ...
}

Could this race condition cause the hardware IRTE to be programmed with garbage,
leading to lost or misrouted interrupts?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260708091408.12106-1-sarunkod@amd.com?part=2

Re: [RFC PATCH v2 5/5] iommu/amd: Provide kernel command line option to enable GAPPI

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Global state `amd_iommu_gappi` remains incorrectly `true` when VAPIC is unsupported or disabled by SNP, causing KVM state corruption.

Pre-existing issues:
- [High] Memory leak and sleeping in atomic context during system resume on the GALog fallback path.
- [High] Resource leak and inconsistent VAPIC state if GALog initialization fails on a secondary IOMMU.
--

--- Patch [5]: [RFC PATCH v2 5/5] iommu/amd: Provide kernel command line option to enable GAPPI ---
commit eb6466c0ce487095bfa19ee53f280dedc6c60d0f
Author: Sairaj Kodilkar <sarunkod@amd.com>

iommu/amd: Provide kernel command line option to enable GAPPI

This commit adds a new kernel command line parameter amd_iommu=gappi. It
allows enabling the GAPPI feature if the corresponding extended feature bit
is set in the hardware. It modifies the IOMMU initialization sequence to
configure GAPPI when enabled.

> diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
> index e2da6d5a63c78..a1dd2e8840d6c 100644
> --- a/drivers/iommu/amd/init.c
> +++ b/drivers/iommu/amd/init.c

[ ... ]

> @@ -3016,10 +3025,19 @@ static void enable_iommus_vapic(void)
>  		return;
>  	}
>  
> +	if (amd_iommu_gappi &&
> +	    !(check_feature(FEATURE_GAPPI) &&
> +	      AMD_IOMMU_GUEST_IR_VAPIC(amd_iommu_guest_ir))) {
> +		pr_warn("GAPPI is not supported.\n");
> +		amd_iommu_gappi = false;
> +	}

[Severity: High]
Does this leave amd_iommu_gappi erroneously set to true if an early return
is taken?

If the kernel is booted with amd_iommu=gappi, the variable is initially
true. In enable_iommus_vapic(), if an early return is taken before reaching
this validation check (for instance, when check_feature(FEATURE_GAM_VAPIC)
returns false, or if forced to disable due to SNP), the function exits
without resetting amd_iommu_gappi to false.

Because the variable remains true, exported subsystems like KVM AVIC might
incorrectly assume hardware GAPPI is active and attempt to use it, which
could lead to state corruption or missed interrupts.

> +
>  	/* Enabling GAM and SNPAVIC support */
>  	for_each_iommu(iommu) {
> -		if (iommu_init_ga_log(iommu) ||
> -		    iommu_ga_log_enable(iommu))
> +		if (amd_iommu_gappi)
> +			iommu_feature_enable(iommu, CONTROL_GAPPI_EN);
> +		else if (iommu_init_ga_log(iommu) ||
> +			 iommu_ga_log_enable(iommu))
>  			return;

[Severity: High]
This is a pre-existing issue, but does this early return leave the VAPIC
configuration in an inconsistent state if it fails on a secondary IOMMU?

In enable_iommus_vapic(), if iommu_init_ga_log() or iommu_ga_log_enable()
fails for any IOMMU after the first one, the function returns immediately.
This leaves the global kernel state assuming VAPIC is active, but some
IOMMUs are left unconfigured. It also skips reverting amd_iommu_guest_ir
or cleaning up resources for the previously configured IOMMUs.

[Severity: High]
This isn't a bug introduced by this patch, but can calling
iommu_init_ga_log() here cause a memory leak and a sleeping in atomic
context issue during system resume?

During system resume, syscore_ops.resume runs with local interrupts
disabled. amd_iommu_resume() calls amd_iommu_enable_interrupts(), which
invokes enable_iommus_vapic().

On the GALog fallback path, it unconditionally calls iommu_init_ga_log().
This function then allocates memory using GFP_KERNEL:

drivers/iommu/amd/init.c:iommu_init_ga_log() {
    ...
    iommu->ga_log = iommu_alloc_pages_node_sz(nid, GFP_KERNEL, GA_LOG_SIZE);
    ...
}

Using GFP_KERNEL in atomic context triggers a sleeping in atomic context
bug. In addition, the existing ga_log pointers are overwritten without
being freed, which leaks the previous allocations on every resume cycle.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260708091408.12106-1-sarunkod@amd.com?part=5

Re: [RFC PATCH v2 1/5] iommu/amd: kvm/svm: Improve API between SVM and AMD IOMMU

[Sean Christopherson]

This should be two separate patches, one to rename the @cpu param/field, one to
replace @ga_log_intr with flags.  "Improve XYZ" is the equilavent of a do_work()
function.  And I'm having a hell of a time trying to review this because of the
overlapping churn.

Actually, if you insist on a "flags" field, three patches: rename, convert to
flags, add the dedicated RUNNING flag.

On Wed, Jul 08, 2026, Sairaj Kodilkar wrote:
> The name "cpu" in the parameter is misleading as it represents the
> physical apicid of the cpu. Hence rename it.

Hrm, I was going to suggest having KVM pass in the actual CPU instead of the APIC
ID, because the fact that the IRTE takes an APIC ID is an implementation detail
that would ideally not bleed into the API.  But unfortunately KVM uses the entry
in the physical ID table when filling amd_iommu_pi_data.cpu, so taking the APIC ID
probably is the least awful approach.  :-/

> Introduce flags to determine the state of the vCPU (running or not) and
> posted interrupts.

> This is useful as following patch overloads the apicid (formerly cpu) field
> to determine GAPPI destination when vCPU is not running and it can no longer
> be used to determine if vCPU is running or not.

Rewrite the GAPPI behavior with --verbose to explain *why*.  The only reason I
could suss out the why is because I already know how Intel posted interrupts work
and so could intuit why GAPPI would want to keep the APIC ID valid.  Without that
knowledge, this is unreviewable.

> Signed-off-by: Sairaj Kodilkar <sarunkod@amd.com>
> ---
>  arch/x86/include/asm/irq_remapping.h |  4 ++--
>  arch/x86/kvm/svm/avic.c              | 22 ++++++++++++++--------
>  drivers/iommu/amd/iommu.c            | 24 ++++++++++++------------
>  include/linux/amd-iommu.h            | 14 ++++++++++----
>  4 files changed, 38 insertions(+), 26 deletions(-)
> 
> diff --git a/arch/x86/include/asm/irq_remapping.h b/arch/x86/include/asm/irq_remapping.h
> index 37b94f484ef3..40a2206a33c0 100644
> --- a/arch/x86/include/asm/irq_remapping.h
> +++ b/arch/x86/include/asm/irq_remapping.h
> @@ -35,8 +35,8 @@ struct amd_iommu_pi_data {
>  	u64 vapic_addr;		/* Physical address of the vCPU's vAPIC. */
>  	u32 ga_tag;
>  	u32 vector;		/* Guest vector of the interrupt */
> -	int cpu;
> -	bool ga_log_intr;
> +	int apicid;
> +	int flags;

Why use flags?  This is on-stack data, packing booleans into flags adds no value
IMO, and makes the code way harder to read.

>  	bool is_guest_mode;
>  	void *ir_data;
>  };
> diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
> index cdd5a6dc646f..7862b13c5409 100644
> --- a/arch/x86/kvm/svm/avic.c
> +++ b/arch/x86/kvm/svm/avic.c
> @@ -932,6 +932,7 @@ int avic_pi_update_irte(struct kvm_kernel_irqfd *irqfd, struct kvm *kvm,
>  		struct vcpu_svm *svm = to_svm(vcpu);
>  		u64 entry;
>  		int ret;
> +		int posted_intr;
>  
>  		/*
>  		 * Prevent the vCPU from being scheduled out or migrated until
> @@ -949,10 +950,11 @@ int avic_pi_update_irte(struct kvm_kernel_irqfd *irqfd, struct kvm *kvm,
>  		 */
>  		entry = svm->avic_physical_id_entry;
>  		if (entry & AVIC_PHYSICAL_ID_ENTRY_IS_RUNNING_MASK) {
> -			pi_data.cpu = entry & AVIC_PHYSICAL_ID_ENTRY_HOST_PHYSICAL_ID_MASK;
> +			pi_data.apicid = entry & AVIC_PHYSICAL_ID_ENTRY_HOST_PHYSICAL_ID_MASK;
> +			pi_data.flags = AMD_IOMMU_FLAG_VCPU_RUNNING;
>  		} else {
> -			pi_data.cpu = -1;
> -			pi_data.ga_log_intr = entry & AVIC_PHYSICAL_ID_ENTRY_GA_LOG_INTR;
> +			posted_intr = !!(entry & AVIC_PHYSICAL_ID_ENTRY_GA_LOG_INTR);

"posted_intr" is misleading, and IMO flat out wrong.  Even when a GA Log Intr
and notification is logged/sent, the virtual interrupt is still posted to the
vCPU's virtual APIC.  I have no idea what you're trying to capture with this
code.  And that is yet another reason why this patch needs to be split up: so
that you can explain the reasoning and logic behind this refactoring.

> +			pi_data.flags = posted_intr << AMD_IOMMU_FLAG_POSTED_INTR_SHIFT;

Re: [RFC PATCH v2 3/5] kvm/svm: Introduce per-CPU lock and wakeup queue

[Sean Christopherson]

On Wed, Jul 08, 2026, Sairaj Kodilkar wrote:
> Add a per-CPU list and raw spinlock that SVM will use to track vCPUs
> blocked and waiting for a wakeup.  The GAPPI interrupt handler will
> scan the corresponding CPU’s list to decide which vCPU to run.
> Initialize both in avic_hardware_setup() when AVIC is enabled.

This needs to be squashed with patch 4.  It's useless and unreviewable as a
standalone patch.

Re: [RFC PATCH v2 4/5] kvm/svm: Update the per-CPU wakeup-list during vCPU load and unload

[Sean Christopherson]

KVM: SVM:

And this shortlog+changelog is woefully inadequate.  For all intents and purposes
this patch adds KVM support for GAPPI.  So, state that in the shortlog, explain
what GAPPI is (and spell out the acronym), how it works, what KVM's scheme is,
and so on and so forth.  I'm not even going to try to review the code without that.

On Wed, Jul 08, 2026, Sairaj Kodilkar wrote:
> When a vCPU is unloaded from a physical CPU enqueue it on that CPU's
> GAPPI wakeup list. Remove the vCPU from the wakeup list when it is loaded
> on to a CPU again.
> 
> Also enqueue from avic_pi_update_irte() when vCPU is not running and
> ir_list is still empty.  This handles the condition where vCPU load skips
> the per-CPU wakeup-list update when ir_list is empty.
> 
> The GAPPI wakeup handler walks this CPU's list and wakes vCPUs that still
> have a pending IRR.  Install it with kvm_set_posted_intr_wakeup_handler()
> so deliveries on POSTED_INTR_WAKEUP_VECTOR invoke it.

Re: [RFC PATCH v2 1/5] iommu/amd: kvm/svm: Improve API between SVM and AMD IOMMU

[Sairaj Kodilkar]

On 7/8/2026 7:06 PM, Sean Christopherson wrote:
> This should be two separate patches, one to rename the @cpu param/field, one to
> replace @ga_log_intr with flags.  "Improve XYZ" is the equilavent of a do_work()
> function.  And I'm having a hell of a time trying to review this because of the
> overlapping churn.
> 
> Actually, if you insist on a "flags" field, three patches: rename, convert to
> flags, add the dedicated RUNNING flag.
> 

Sorry for inconvenience, will split the patch and improve the verbosity
of all the commits.

> On Wed, Jul 08, 2026, Sairaj Kodilkar wrote:
>> The name "cpu" in the parameter is misleading as it represents the
>> physical apicid of the cpu. Hence rename it.
> 
> Hrm, I was going to suggest having KVM pass in the actual CPU instead of the APIC
> ID, because the fact that the IRTE takes an APIC ID is an implementation detail
> that would ideally not bleed into the API.  But unfortunately KVM uses the entry
> in the physical ID table when filling amd_iommu_pi_data.cpu, so taking the APIC ID
> probably is the least awful approach.  :-/
> 
>> Introduce flags to determine the state of the vCPU (running or not) and
>> posted interrupts.
> 
>> This is useful as following patch overloads the apicid (formerly cpu) field
>> to determine GAPPI destination when vCPU is not running and it can no longer
>> be used to determine if vCPU is running or not.
> 
> Rewrite the GAPPI behavior with --verbose to explain *why*.  The only reason I
> could suss out the why is because I already know how Intel posted interrupts work
> and so could intuit why GAPPI would want to keep the APIC ID valid.  Without that
> knowledge, this is unreviewable.
> 
>> Signed-off-by: Sairaj Kodilkar <sarunkod@amd.com>
>> ---
>>  arch/x86/include/asm/irq_remapping.h |  4 ++--
>>  arch/x86/kvm/svm/avic.c              | 22 ++++++++++++++--------
>>  drivers/iommu/amd/iommu.c            | 24 ++++++++++++------------
>>  include/linux/amd-iommu.h            | 14 ++++++++++----
>>  4 files changed, 38 insertions(+), 26 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/irq_remapping.h b/arch/x86/include/asm/irq_remapping.h
>> index 37b94f484ef3..40a2206a33c0 100644
>> --- a/arch/x86/include/asm/irq_remapping.h
>> +++ b/arch/x86/include/asm/irq_remapping.h
>> @@ -35,8 +35,8 @@ struct amd_iommu_pi_data {
>>  	u64 vapic_addr;		/* Physical address of the vCPU's vAPIC. */
>>  	u32 ga_tag;
>>  	u32 vector;		/* Guest vector of the interrupt */
>> -	int cpu;
>> -	bool ga_log_intr;
>> +	int apicid;
>> +	int flags;
> 
> Why use flags?  This is on-stack data, packing booleans into flags adds no value
> IMO, and makes the code way harder to read.
> 
>>  	bool is_guest_mode;
>>  	void *ir_data;
>>  };
>> diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
>> index cdd5a6dc646f..7862b13c5409 100644
>> --- a/arch/x86/kvm/svm/avic.c
>> +++ b/arch/x86/kvm/svm/avic.c
>> @@ -932,6 +932,7 @@ int avic_pi_update_irte(struct kvm_kernel_irqfd *irqfd, struct kvm *kvm,
>>  		struct vcpu_svm *svm = to_svm(vcpu);
>>  		u64 entry;
>>  		int ret;
>> +		int posted_intr;
>>  
>>  		/*
>>  		 * Prevent the vCPU from being scheduled out or migrated until
>> @@ -949,10 +950,11 @@ int avic_pi_update_irte(struct kvm_kernel_irqfd *irqfd, struct kvm *kvm,
>>  		 */
>>  		entry = svm->avic_physical_id_entry;
>>  		if (entry & AVIC_PHYSICAL_ID_ENTRY_IS_RUNNING_MASK) {
>> -			pi_data.cpu = entry & AVIC_PHYSICAL_ID_ENTRY_HOST_PHYSICAL_ID_MASK;
>> +			pi_data.apicid = entry & AVIC_PHYSICAL_ID_ENTRY_HOST_PHYSICAL_ID_MASK;
>> +			pi_data.flags = AMD_IOMMU_FLAG_VCPU_RUNNING;
>>  		} else {
>> -			pi_data.cpu = -1;
>> -			pi_data.ga_log_intr = entry & AVIC_PHYSICAL_ID_ENTRY_GA_LOG_INTR;
>> +			posted_intr = !!(entry & AVIC_PHYSICAL_ID_ENTRY_GA_LOG_INTR);
> 
> "posted_intr" is misleading, and IMO flat out wrong.  Even when a GA Log Intr
> and notification is logged/sent, the virtual interrupt is still posted to the
> vCPU's virtual APIC.  I have no idea what you're trying to capture with this
> code.  And that is yet another reason why this patch needs to be split up: so
> that you can explain the reasoning and logic behind this refactoring.
> 

I needed an generic name for boolean that is being used by both GAPPI
and GALOG, hence I replaced ga_log_intr with posted_intr. But I can
understand that it can lead to confusion. How about replacing
ga_log_intr with wakeup_intr and AVIC_PHYSICAL_ID_ENTRY_GA_LOG_INTR with
AVIC_PHYSICAL_ID_ENTRY_WAKEUP_INTR

Thanks
Sairaj

>> +			pi_data.flags = posted_intr << AMD_IOMMU_FLAG_POSTED_INTR_SHIFT;

Re: [RFC PATCH v2 1/5] iommu/amd: kvm/svm: Improve API between SVM and AMD IOMMU

[Sean Christopherson]

On Thu, Jul 09, 2026, Sairaj Kodilkar wrote:
> On 7/8/2026 7:06 PM, Sean Christopherson wrote:
> >> @@ -949,10 +950,11 @@ int avic_pi_update_irte(struct kvm_kernel_irqfd *irqfd, struct kvm *kvm,
> >>  		 */
> >>  		entry = svm->avic_physical_id_entry;
> >>  		if (entry & AVIC_PHYSICAL_ID_ENTRY_IS_RUNNING_MASK) {
> >> -			pi_data.cpu = entry & AVIC_PHYSICAL_ID_ENTRY_HOST_PHYSICAL_ID_MASK;
> >> +			pi_data.apicid = entry & AVIC_PHYSICAL_ID_ENTRY_HOST_PHYSICAL_ID_MASK;
> >> +			pi_data.flags = AMD_IOMMU_FLAG_VCPU_RUNNING;
> >>  		} else {
> >> -			pi_data.cpu = -1;
> >> -			pi_data.ga_log_intr = entry & AVIC_PHYSICAL_ID_ENTRY_GA_LOG_INTR;
> >> +			posted_intr = !!(entry & AVIC_PHYSICAL_ID_ENTRY_GA_LOG_INTR);
> > 
> > "posted_intr" is misleading, and IMO flat out wrong.  Even when a GA Log Intr
> > and notification is logged/sent, the virtual interrupt is still posted to the
> > vCPU's virtual APIC.  I have no idea what you're trying to capture with this
> > code.  And that is yet another reason why this patch needs to be split up: so
> > that you can explain the reasoning and logic behind this refactoring.
> > 
> 
> I needed an generic name for boolean that is being used by both GAPPI
> and GALOG, hence I replaced ga_log_intr with posted_intr. But I can
> understand that it can lead to confusion. How about replacing
> ga_log_intr with wakeup_intr and AVIC_PHYSICAL_ID_ENTRY_GA_LOG_INTR with
> AVIC_PHYSICAL_ID_ENTRY_WAKEUP_INTR

Yeah, wakeup_intr works for me.  I don't love bleeding KVM's usage into other
parts of the kernel, but the POSTED_INTR_WAKEUP_VECTOR ship sailed a looong time
ago.