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

[Sairaj Kodilkar <sarunkod@amd.com>]

On 10/15/2026 12:38 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [High] iommu/amd: NULL pointer dereference in iommu_poll_ga_log() with GAPPI
> - [Medium] iommu/amd: amd_iommu_gappi state inconsistent on VAPIC fallback
> - [Low] iommu/amd: missing GAPPI in feat_str feature name array
> --
> 
> commit f3f34d41f6c88bbaa635da8109c52cf22d09f580
> Author: Sairaj Kodilkar <sarunkod@amd.com>
> 
> iommu/amd: Provide kernel command line option to enable GAPPI
> 
> Enable GAPPI when the IOMMU GAPPISup extended feature bit is set and
> the kernel is booted with the amd_iommu=gappi kernel parameter.
> 
>> diff --git a/drivers/iommu/amd/amd_iommu_types.h b/drivers/iommu/amd/amd_iommu_types.h
>> index b89a2b3df0a49..51edd617ddca2 100644
>> --- a/drivers/iommu/amd/amd_iommu_types.h
>> +++ b/drivers/iommu/amd/amd_iommu_types.h
>> @@ -78,6 +78,7 @@
>>  #define FEATURE_X2APIC		BIT_ULL(2)
>>  #define FEATURE_NX		BIT_ULL(3)
>>  #define FEATURE_GT		BIT_ULL(4)
>> +#define FEATURE_GAPPI		BIT_ULL(5)
> 
> [Severity: Low]
> This isn't a functional bug, but FEATURE_GAPPI is defined as bit 5. 
> 
> In print_iommu_info(), bit 5 is still hardcoded as "[5]" in the feature
> string array:
> 
> drivers/iommu/amd/init.c:print_iommu_info() {
> ...
> 	static const char * const feat_str[] = {
> 		"PreF", "PPR", "X2APIC", "NX", "GT", "[5]",
> 		"IA", "GA", "HE", "PC"
> 	};
> ...
> }
> 
> Should "[5]" be updated to "GAPPI" so the kernel boot log displays the
> correct feature name?
> 
> [ ... ]
>> diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
>> index ef23e0755c32d..c536e0b3c4ff7 100644
>> --- a/drivers/iommu/amd/init.c
>> +++ b/drivers/iommu/amd/init.c
> [ ... ]
>> @@ -3025,10 +3035,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: Medium]
> If VAPIC is unsupported or disabled due to SNP, the code returns early just
> above this diff and falls back to legacy mode:
> 
> drivers/iommu/amd/init.c:enable_iommus_vapic() {
> ...
> 	if (!check_feature(FEATURE_GAM_VAPIC)) {
> 		amd_iommu_guest_ir = AMD_IOMMU_GUEST_IR_LEGACY_GA;
> 		return;
> 	}
> 
> 	if (amd_iommu_snp_en &&
> 	    !FEATURE_SNPAVICSUP_GAM(amd_iommu_efr2)) {
> 		pr_warn("Force to disable Virtual APIC due to SNP\n");
> 		amd_iommu_guest_ir = AMD_IOMMU_GUEST_IR_LEGACY_GA;
> 		return;
> 	}
> ...
> }
> 
> These early returns bypass the new fallback check added here that sets
> amd_iommu_gappi = false.
> 
> If KVM subsequently reads the stale amd_iommu_gappi flag and attempts to set
> up IRTE entries using GAPPI parameters, it calls irq_set_vcpu_affinity()
> which triggers this check:
> 
> drivers/iommu/amd/iommu.c:amd_ir_set_vcpu_affinity() {
> ...
> 	if (WARN_ON_ONCE(!AMD_IOMMU_GUEST_IR_VAPIC(amd_iommu_guest_ir)))
> 		return -EINVAL;
> ...
> }
> 
> Could this result in a kernel warning and failure to configure the affinity
> since the IOMMU driver actually fell back to legacy mode?
> 
>> +
>>  	/* 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]
> If GAPPI is enabled, we skip allocating the GA log, so iommu->ga_log
> remains NULL.
> 
> If the previous kernel used the legacy GA log and left an interrupt pending,
> or if an unmasked interrupt triggers the handler:
> 
> drivers/iommu/amd/iommu.c:iommu_poll_ga_log() {
> ...
> 	head = readl(iommu->mmio_base + MMIO_GA_HEAD_OFFSET);
> 	tail = readl(iommu->mmio_base + MMIO_GA_TAIL_OFFSET);
> 
> 	while (head != tail) {
> 		volatile u64 *raw;
> ...
> 		raw = (u64 *)(iommu->ga_log + head);
> ...
> 		/* Avoid memcpy function-call overhead */
> 		log_entry = *raw;
> ...
> }
> 
> Won't the while loop unconditionally dereference iommu->ga_log which is now
> NULL, crashing the kernel?
> 

This is a valid concern. This is a pre-existing bug and can arise if
second kernel has IRQ_REMAP disabled or fails to enable ga_log. Hence it
is better to address this in a separate series.

Thanks
Sairaj