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

[Sairaj Kodilkar <sarunkod@amd.com>]

On 6/26/2026 4:55 PM, sashiko-bot@kernel.org wrote:
> [You don't often get email from sashiko-bot@kernel.org. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>
> 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?

No, this will not happen. If enable_iommus_vapic() returns early, AMD IOMMU
will not set IRQ_POSTING_CAP capability. kvm_x86_vendor_init() checks this
capability before enabling interrupt posting. Hence all vfio device
interrupts will take eventfd path, in case enable_iommus_vapic() returns
early.

>
> [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().

Yes, this is a valid concern. Will include this in the next version.

>> +     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?

It is not possible to have the spurious GALOG overflow interrupt without
enabling the GALOG in an "ideal" scenario. But to protect the kernel against
buggy hardware, I'll add necessary checks inside amd_iommu_restart_ga_log.
> [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?

You're right that the abort path can leave MMIO state inconsistent across
IOMMUs. For this series that is acceptable: we never set IRQ_POSTING_CAP,
so interrupt routing is unaffected. Proper cleanup belongs in a separate
series. I will keep this hunk as-is.

> [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?

Yes, This is a known issue in IOMMU driver, which leaks the previously
allocated memory for ga_log. Cleanup for this one also belongs to a
separate series. I will keep this hunk as is.

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