Re: [RFC PATCH v3 5/6] KVM: SVM: Add support for AMD IOMMU Guest APIC Physical Processor Interrupt (GAPPI)

On Wed, Aug 12, 2026, Sairaj Kodilkar wrote:
> 
> 
> On 8/11/2026 6:13 AM, Sean Christopherson wrote:
> > On Mon, Aug 10, 2026, Sairaj Kodilkar wrote:
> >> On 7/13/2026 4:41 PM, [email protected] wrote:
> >>> [Severity: High]
> >>> Can an unprivileged user-space process trigger this WARN_ON?
> >>>
> >>> When a vCPU is created, svm->gappi_cpu is initialized to -1 in
> >>> avic_init_vcpu(). It is only set to a valid CPU ID later during
> >>> __avic_vcpu_load().
> >>>
> >>> If host user-space configures device interrupt routing via the KVM_IRQFD ioctl
> >>> before the vCPU runs, avic_pi_update_irte() is invoked. This will pass the
> >>> uninitialized svm->gappi_cpu (-1) down to this function, hitting the WARN_ON.
> >>> If the host has panic_on_warn enabled, this allows host userspace to trigger
> >>> a kernel panic.
> >>
> >> This is a valid concern.
> >>
> >> If host userspace attaches a bypass IRQ targeting a vCPU that has never
> >> been loaded. Functionally, there is nothing to do in that window. A vCPU
> >> that has never been loaded cannot be blocking, so no GAPPI wakeup is
> >> required. The IOMMU still posts the interrupt into the vAPIC backing
> >> page, and the pending IRR is evaluated at the first VMRUN after
> >> avic_vcpu_load(), which is also where the IRTE gets a valid Destination
> >> and IsRun = 1.
> >>
> >> This can be resolved by assigning a arbitrary gappi destination, without
> >> actually updating the gappi wakeup list of that CPU.
> > 
> > With the disclaimer that I haven't look super closely at this series, and haven't
> > thought too deeply about the feature itself either...
> > 
> > Why are we doing anything different than what VMX does?  vCPUs on the wakeup
> > list when they block, and come off the list when they wakeup.  It's literally
> > one flow that's guarantee to pair put()+load(), and the logic for manipulating
> > the list is quite simple as a result.
> 
> I was trying to manipulate the vCPU list after is_empty(ir_list) check
> in put() and load() path. Which complicated the things, since
> pi_update_irte() will have to add the vCPU to the list if it was a first
> interrupt assigned to given vCPU.

No, there are other options.  Specifically, the solution VMX uses is to put the
vCPU on the wakeup list if it *might* get a posted IRQ.  Again, why reinvent the
wheel?
 
> I think its better to keep the list operations before is_empty(ir_list)
> check in order to simplify things a little bit.Note that it may increase
> the list size and potentially increasing time for gappi_wakeup_handler.
>
> > Going a step further, why is GAPPI not sharing code with VMX Posted Interupts?
> > At a glance, the only meaningful difference in the wakeup flow is the "should
> > this particular vCPU be awakened".
> 
> On Intel there is a level of indirection: KVM hands the IOMMU the
> physical address of the PI descriptor once, at IRQ affinity setup time
> (vmx_pi_update_irte()). After that the descriptor is the only thing that
> needs updating, so vcpu_load()/vcpu_put() just write NDST/NV/SN in
> memory and never call into the IOMMU driver again.
> 
> AMD has no such indirection. The destination APIC ID, IsRun and GATag
> live directly in the IRTE, and there is no per-vCPU structure that the
> IOMMU dereferences. So every vcpu_load()/vcpu_put() has to call into the
> AMD IOMMU driver to update each IRTE targeting the vCPU, via
> avic_update_iommu_vcpu_affinity() -> amd_iommu_update_ga().
> 
> That difference leaks into the wakeup list handling. On Intel, putting a
> vCPU on the per-pCPU list is self-contained. On AMD it has to happen in
> the same ir_list_lock critical section as the IRTE update, and it is
> conditional on the vCPU actually having posted IRQs (ir_list being non
> empty), because the pCPU that the vCPU is enqueued on is also what gets
> programmed into IRTE[Destination].

And?  That's literally calling one function from a slightly difference location
on Intel vs. AMD.  E.g. unless I'm missing something, the handling should be
something like this, sans actual code movement:

diff --git arch/x86/kvm/svm/avic.c arch/x86/kvm/svm/avic.c
index c5b1d294b15a..c9a6da646c8e 100644
--- arch/x86/kvm/svm/avic.c
+++ arch/x86/kvm/svm/avic.c
@@ -1020,6 +1020,9 @@ static void avic_update_iommu_vcpu_affinity(struct kvm_vcpu *vcpu, int cpu,
 
        lockdep_assert_held(&svm->ir_list_lock);
 
+       if (ga_log_intr && gappi && kvm_can_use_device_posted_irqs(vcpu->kvm))
+               kvm_pi_enable_wakeup_handler(vcpu);
+
        /*
         * Here, we go through the per-vcpu ir_list to update all existing
         * interrupt remapping table entry targeting this vcpu.
diff --git arch/x86/kvm/vmx/posted_intr.c arch/x86/kvm/vmx/posted_intr.c
index 4a6d9a17da23..f6e3fea11baa 100644
--- arch/x86/kvm/vmx/posted_intr.c
+++ arch/x86/kvm/vmx/posted_intr.c
@@ -144,7 +144,7 @@ void vmx_vcpu_pi_load(struct kvm_vcpu *vcpu, int cpu)
                pi_set_on(pi_desc);
 }
 
-static bool vmx_can_use_vtd_pi(struct kvm *kvm)
+static bool kvm_can_use_device_posted_irqs(struct kvm *kvm)
 {
        /*
         * Note, reading the number of possible bypass IRQs can race with a
@@ -217,7 +217,7 @@ static bool vmx_needs_pi_wakeup(struct kvm_vcpu *vcpu)
         * back to the pi_wakeup_handler() function.
         */
        return (vmx_can_use_ipiv(vcpu) && !is_td_vcpu(vcpu)) ||
-               vmx_can_use_vtd_pi(vcpu->kvm);
+               kvm_can_use_device_posted_irqs(vcpu->kvm);
 }
 
 void vmx_vcpu_pi_put(struct kvm_vcpu *vcpu)
@@ -242,7 +242,7 @@ void vmx_vcpu_pi_put(struct kvm_vcpu *vcpu)
        if (!vcpu->preempted && kvm_vcpu_is_blocking(vcpu) &&
            ((is_td_vcpu(vcpu) && tdx_interrupt_allowed(vcpu)) ||
             (!is_td_vcpu(vcpu) && !vmx_interrupt_blocked(vcpu))))
-               pi_enable_wakeup_handler(vcpu);
+               kvm_pi_enable_wakeup_handler(vcpu);
        else
                pi_set_sn(pi_desc);
 }