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

[Sairaj Kodilkar <sarunkod@amd.com>]

On 8/13/2026 3:29 AM, Sean Christopherson wrote:
> 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, sashiko-bot@kernel.org 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);
> +

I am not sure what you mean here, because pi_enable_wakeup_handler does
more than just manipulating the wakeup list, it also updates pi
descriptor which is Intel specific, We have following two options:

Option A:
Adding a new x86_ops

avic_update_iommu_vcpu_affinity
	kvm_pi_enable_wakeup_handler
		list handling
		kvm_x86_callback(configure_pi)
			IOMMU callbacks


The new x86_ops makes sure that irte and pi descriptor are configured
correctly.

Option B:
Common list handling

The svm and vmx will share function for list handling but IRTE/PI setup
code will remain same.

avic_update_iommu_vcpu_affinity/pi_enable_wakeup_handler
	kvm_add_to_wakeup_list
		list handling
	IOMMU callbacks


Thanks
Sairaj

>         /*
>          * 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);
>  }