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

[Vasant Hegde <vasant.hegde@amd.com>]

On 5/5/2026 11:11 PM, Sean Christopherson wrote:
> 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@amd.com>
>>> Signed-off-by: Sean Christopherson <seanjc@google.com>
>>> ---
>>>  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;
> 	}


Right. I missed the return part. Thanks!


Reviewed-by: Vasant Hegde <vasant.hegde@amd.com>





> 
> 	/* 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.

Right. I will add fix to move #2 to iommu_init_pci() and #3 to iommu_snp_enable() .

-Vasant