Re: [PATCH] iommu/amd: Explicitly bail from enable_iommus_vapic() when in legacy mode

[Sean Christopherson]

On Tue, May 05, 2026, Vasant Hegde wrote:
> Sean,
> 
> On 3/15/2025 8:39 AM, Sean Christopherson wrote:
> > Bail early from enable_iommus_vapic() if IOMMUs are configured for either
> > of the legacy modes, as it's absurdly difficult to see that
> > iommu_ga_log_enable() is guaranteed to fail because iommu_init_ga_log()
> > skips allocating the ga_log.
> > 
> > Opportunistically have iommu_ga_log_enable() WARN if it's called without
> > IOMMUs being configured to support AVIC/vAPIC.
> 
> 
> > 
> > Cc: Suravee Suthikulpanit <suravee.suthikulpanit@xxxxxxx>
> > Signed-off-by: Sean Christopherson <seanjc@xxxxxxxxxx>
> > ---
> >  drivers/iommu/amd/init.c | 10 ++++++----
> >  1 file changed, 6 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
> > index cb536d372b12..05c568da589a 100644
> > --- a/drivers/iommu/amd/init.c
> > +++ b/drivers/iommu/amd/init.c
> > @@ -931,8 +931,8 @@ static int iommu_ga_log_enable(struct amd_iommu *iommu)
> >  
> >  static int iommu_init_ga_log(struct amd_iommu *iommu)
> >  {
> > -	if (!AMD_IOMMU_GUEST_IR_VAPIC(amd_iommu_guest_ir))
> > -		return 0;
> > +	if (WARN_ON_ONCE(!AMD_IOMMU_GUEST_IR_VAPIC(amd_iommu_guest_ir)))
>  +		return -EINVAL;
> >  
> >  	iommu->ga_log = iommu_alloc_pages(GFP_KERNEL, get_order(GA_LOG_SIZE));
> >  	if (!iommu->ga_log)
> > @@ -2863,8 +2863,10 @@ static void enable_iommus_vapic(void)
> >  			return;
> >  	}
> >  
> > -	if (AMD_IOMMU_GUEST_IR_VAPIC(amd_iommu_guest_ir) &&
> > -	    !check_feature(FEATURE_GAM_VAPIC)) {
> > +	if (!AMD_IOMMU_GUEST_IR_VAPIC(amd_iommu_guest_ir))
> > +		return;
> > +
> 
> We should move this down after SNP check. Otherwise we may hit WARN_ON_ONCE in
> iommu_init_ga_log().

No?  enable_iommus_vapic() ends up with:

1.	if (!AMD_IOMMU_GUEST_IR_VAPIC(amd_iommu_guest_ir))
		return;

2.	if (!check_feature(FEATURE_GAM_VAPIC)) {
		amd_iommu_guest_ir = AMD_IOMMU_GUEST_IR_LEGACY_GA;
		return;
	}

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

	/* Enabling GAM and SNPAVIC support */
	for_each_iommu(iommu) {
		if (iommu_init_ga_log(iommu) ||
		    iommu_ga_log_enable(iommu))
			return;

		iommu_feature_enable(iommu, CONTROL_GAM_EN);
		if (amd_iommu_snp_en)
			iommu_feature_enable(iommu, CONTROL_SNPAVIC_EN);
	}

And that's the only call site for iommu_init_ga_log().  Getting past #1 guarantees
"amd_iommu_guest_ir == AMD_IOMMU_GUEST_IR_VAPIC", and if #2 and #3 return early
if they modify amd_iommu_guest_ir, so it should be impossible for iommu_init_ga_log()
to be called with amd_iommu_guest_ir anything other than AMD_IOMMU_GUEST_IR_VAPIC.

Shifting the initial check down isn't functionally correct, as it wouldn't do the
right thing if amd_iommu_guest_ir==AMD_IOMMU_GUEST_IR_LEGACY.  AFAICT, nothing
prevents calling into enable_iommus_vapic() in that case.