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

On 7/13/2026 4:41 PM, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> 
> New issues:
> - [Critical] Unconditional enablement of GA log on overflow in kexec/kdump environments with GAPPI leads to critical DMA memory corruption.
> 
> Pre-existing issues:
> - [High] Lockless read-modify-write of the IOMMU MMIO control register causes a race condition that can permanently disable IOMMU interrupts or GAPPI.
> --
> 
> --- Patch [6]: [RFC PATCH v3 6/6] iommu/amd: Provide kernel command line option to enable GAPPI ---
> commit 38ebc4ddc54202efd2ae74234b7ee4169af3e39a
> Author: Sairaj Kodilkar <[email protected]>
> 
> iommu/amd: Provide kernel command line option to enable GAPPI
> 
> This commit adds a kernel parameter to enable GAPPI for AMD IOMMUs when
> the GAPPISup extended feature bit is set. It also modifies initialization
> code to handle GAPPI alongside the existing GALOG feature.
> 
>> diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
>> index 2e1889f8a9e42..7d2b1c24f6ecc 100644
>> --- a/drivers/iommu/amd/init.c
>> +++ b/drivers/iommu/amd/init.c
> 
> [ ... ]
> 
>> @@ -2999,6 +3002,13 @@ static void enable_iommus_vapic(void)
>>  	struct amd_iommu *iommu;
>>  
>>  	for_each_iommu(iommu) {
>> +		/*
>> +		 * Disable GAPPI, do not check amd_iommu_gappi as it may be
>> +		 * false in new kexec kernel even though previous kernel has
>> +		 * enabled it.
>> +		 */
>> +		iommu_feature_disable(iommu, CONTROL_GAPPI_EN);
> 
> [Severity: High]
> This is a pre-existing issue, but does this unlocked modification of the
> IOMMU MMIO control register cause a race condition that could disable
> interrupts or other features?
> 
> The iommu_feature_disable() function performs a read-modify-write without
> holding iommu->lock:
> 
> drivers/iommu/amd/init.c:iommu_feature_set() {
>     ctrl = readq(iommu->mmio_base +  MMIO_CONTROL_OFFSET);
>     ctrl &= ~((u64)mask << shift);
>     ctrl |= ((u64)val << shift) & ((u64)mask << shift);
>     writeq(ctrl, iommu->mmio_base +  MMIO_CONTROL_OFFSET);
> }
> 
> During initialization, the IRQ thread could be running concurrently. If a log
> overflow occurs, the IRQ thread calls amd_iommu_restart_log() which also
> invokes iommu_feature_disable().
> 
> Since this read-modify-write isn't synchronized, could concurrent
> modifications silently overwrite each other?
> 
>> +
>>  		/*
>>  		 * Disable GALog if already running. It could have been enabled
> 
> [ ... ]
> 
>> @@ -3038,10 +3048,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;
>> +	}
>> +
>>  	/* 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: Critical]
> If the system is booted into a kdump kernel with GAPPI enabled, could this
> lead to the IOMMU writing to stale physical memory from the previous kernel?
> 
> When GAPPI is used, iommu_init_ga_log() is skipped, meaning the ga_log buffer
> is not allocated. However, if the previous kernel crashed with a pending GA
> log overflow, the hardware's overflow status bit remains set.
> 
> Once shared IOMMU interrupts are enabled, the pending overflow status triggers
> the interrupt thread:
> 
> drivers/iommu/amd/iommu.c:amd_iommu_int_thread_galog() {
>     amd_iommu_handle_irq(data, "GA", MMIO_STATUS_GALOG_INT_MASK,
>                          MMIO_STATUS_GALOG_OVERFLOW_MASK,
>                          iommu_poll_ga_log, amd_iommu_restart_ga_log);
> }
> 
> This calls amd_iommu_restart_ga_log() which unconditionally enables
> CONTROL_GALOG_EN:
> 
> drivers/iommu/amd/init.c:amd_iommu_restart_ga_log() {
>     amd_iommu_restart_log(iommu, "GA", CONTROL_GAINT_EN,
>                           CONTROL_GALOG_EN, MMIO_STATUS_GALOG_RUN_MASK,
>                           MMIO_STATUS_GALOG_OVERFLOW_MASK);
> }
> 
> Since CONTROL_GALOG_EN is re-enabled without checking if GAPPI is active or
> if the log memory was allocated, will the IOMMU resume writing GA log
> entries to the uninitialized physical address left by the crashed kernel?
> 

The IOMMU driver does not evaluate GALOG overflow status bit unless
IOMMU delivers an GALOG interrupt, which is not possible when IOMMU
switches GAPPI mode.

Thanks
Sairaj

>>  		iommu_feature_enable(iommu, CONTROL_GAM_EN);
>>  		if (amd_iommu_snp_en)
>