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

[sashiko-bot@kernel.org]

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