Re: [PATCH v6 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 1 potential issue(s) to consider:
- [Medium] iommu/amd: enable_iommus_vapic() leaves amd_iommu_gappi true on early return
--

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

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

This commit adds the amd_iommu=gappi kernel command-line parameter to enable
GAPPI when the hardware supports FEATURE_GAPPI and Virtual APIC mode is
active, along with documentation for the new option.

> diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
> index 78e3c14dc1c28..72394c92a3fa0 100644
> --- a/drivers/iommu/amd/init.c
> +++ b/drivers/iommu/amd/init.c

[ ... ]

> @@ -3027,10 +3037,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]
In drivers/iommu/amd/init.c, parse_amd_iommu_options() sets the exported
global amd_iommu_gappi to true during early boot when amd_iommu=gappi is
passed on the command line (with CONFIG_IRQ_REMAP enabled):

drivers/iommu/amd/init.c:parse_amd_iommu_options() {
	...
	} else if (strncmp(str, "gappi", 5) == 0) {
#ifdef CONFIG_IRQ_REMAP
		amd_iommu_gappi = true;
#else
		pr_warn("amd_iommu=gappi ignored, IRQ remapping is not enabled\n");
#endif
	}
	...
}

However, earlier in enable_iommus_vapic(), all paths where Virtual APIC mode
is not enabled or gets downgraded to legacy GA mode return before reaching
this check:

drivers/iommu/amd/init.c:enable_iommus_vapic() {
	...
	if (!AMD_IOMMU_GUEST_IR_VAPIC(amd_iommu_guest_ir))
		return;

	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;
	}
	...
}

Doesn't this mean AMD_IOMMU_GUEST_IR_VAPIC(amd_iommu_guest_ir) is always true
when this check is reached, leaving amd_iommu_gappi set to true whenever
enable_iommus_vapic() returns early (including on the
MMIO_STATUS_GALOG_RUN_MASK poll timeout) or is never called because the
IOMMU is disabled?

When amd_iommu_gappi remains true while IOMMU Virtual APIC and
CONTROL_GAPPI_EN are not enabled, avic_update_iommu_vcpu_affinity() and
avic_vcpu_free() in arch/x86/kvm/svm/avic.c still see amd_iommu_gappi set
before checking list_empty(&svm->ir_list):

arch/x86/kvm/svm/avic.c:avic_update_iommu_vcpu_affinity() {
	...
	if (amd_iommu_gappi && is_running) {
		kvm_pi_disable_wakeup_handler(vcpu, svm->gappi_cpu);
		svm->gappi_cpu = cpu; /* Store cpu number as target for GAPPI */
	} else if (amd_iommu_gappi && wakeup_intr) {
		apicid = cpu_physical_id(svm->gappi_cpu);
		kvm_pi_enable_wakeup_handler(vcpu, svm->gappi_cpu);
	}

	if (list_empty(&svm->ir_list))
		return 0;
	...
}

Wouldn't that cause KVM to unnecessarily call kvm_pi_enable_wakeup_handler()
and kvm_pi_disable_wakeup_handler() on every vCPU load, block, and free when
running guests with AVIC enabled?

[ ... ]

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