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

[sashiko-bot@kernel.org]

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