Re: [PATCH v5 7/7] 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:
- [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?

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