Re: [RFC PATCH 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 5 potential issue(s) to consider:

New issues:
- [High] The global variable `amd_iommu_gappi` is not cleared to `false` when `enable_iommus_vapic()` returns early.
- [High] The `CONTROL_GAPPI_EN` feature is enabled but never disabled during IOMMU teardown or kexec initialization, leaking hardware state.
- [High] The GA log interrupt handler unconditionally re-enables `CONTROL_GALOG_EN` upon an overflow event, even when GAPPI is used and GA log memory is uninitialized.

Pre-existing issues:
- [High] `iommu_init_ga_log()` unconditionally allocates memory with `GFP_KERNEL` and reallocates pointers during suspend/resume cycles, causing a memory leak and a sleep-in-atomic bug.
- [High] `enable_iommus_vapic()` returns immediately on error without rolling back previously initialized IOMMUs, leading to an inconsistent initialization state.
--

commit a43ef908c557e1883a80f54d875c75488a6d0f52
Author: Sairaj Kodilkar <sarunkod@amd.com>

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

This commit introduces the amd_iommu=gappi command line parameter to enable
the GAPPI extended feature when supported by the IOMMU hardware.

> diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
> --- a/drivers/iommu/amd/init.c
> +++ b/drivers/iommu/amd/init.c
> @@ -3038,11 +3038,20 @@ static void enable_iommus_vapic(void)
>  		return;
>  	}
>  

[Severity: High]
If enable_iommus_vapic() returns early before this point because
FEATURE_GAM_VAPIC is missing or SNP is active, the newly added
amd_iommu_gappi flag is never cleared.

Could this cause issues later in amd_iommu_update_ga() when KVM expects
GAPPI behavior but Virtual APIC mode is logically disabled, triggering
warnings and failing interrupt delivery?

[Severity: High]
When recovering from a previous kernel in the kexec path earlier in this
function, the cleanup loop clears CONTROL_GALOG_EN and CONTROL_GAINT_EN.

Since this patch introduces CONTROL_GAPPI_EN, does the cleanup loop also
need to call iommu_feature_disable(iommu, CONTROL_GAPPI_EN) to prevent
hardware state corruption if the previous kernel left GAPPI active? A
similar omission appears to exist in iommu_disable().

> +	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))
> -			return;
> +		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]
When amd_iommu_gappi is true, iommu_init_ga_log() is intentionally bypassed,
meaning the GA log memory is never allocated.

If a spurious GA log overflow interrupt fires, amd_iommu_int_thread_galog()
processes it by calling amd_iommu_restart_ga_log(), which blindly re-enables
CONTROL_GALOG_EN.

Will the hardware then attempt to write log entries to an uninitialized
physical address, potentially causing kernel memory corruption or fatal
IOMMU target aborts?

[Severity: High]
This isn't a bug introduced by this patch, but returning immediately on
failure here leaves the system in an inconsistent state.

If a subsequent IOMMU fails initialization, the loop aborts without rolling
back the features (like CONTROL_GAM_EN or CONTROL_SNPAVIC_EN) enabled on
previously processed IOMMUs. Could this result in partially initialized
interrupt routing?

[Severity: High]
This isn't a bug introduced by this patch, but calling iommu_init_ga_log()
here unconditionally allocates memory with GFP_KERNEL.

During system resume, amd_iommu_syscore_ops.resume calls
enable_iommus_vapic() with hardware interrupts disabled. Does calling a
GFP_KERNEL allocator in this atomic context cause a sleep-in-atomic bug,
and does unconditionally overwriting iommu->ga_log leak the previously
allocated memory on every resume cycle?

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