* [PATCH] iommu/amd: Explicitly bail from enable_iommus_vapic() when in legacy mode
@ 2025-03-15 3:09 Sean Christopherson
2026-04-06 23:05 ` Sean Christopherson
2026-05-05 9:55 ` Vasant Hegde
0 siblings, 2 replies; 6+ messages in thread
From: Sean Christopherson @ 2025-03-15 3:09 UTC (permalink / raw)
To: Joerg Roedel
Cc: iommu, linux-kernel, Suravee Suthikulpanit, Sean Christopherson
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;
+
+ if (!check_feature(FEATURE_GAM_VAPIC)) {
amd_iommu_guest_ir = AMD_IOMMU_GUEST_IR_LEGACY_GA;
return;
}
base-commit: ea9bd29a9c0d757b3384ae3e633e6bbaddf00725
--
2.49.0.rc1.451.g8f38331e32-goog
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] iommu/amd: Explicitly bail from enable_iommus_vapic() when in legacy mode
2025-03-15 3:09 [PATCH] iommu/amd: Explicitly bail from enable_iommus_vapic() when in legacy mode Sean Christopherson
@ 2026-04-06 23:05 ` Sean Christopherson
2026-05-04 8:24 ` Joerg Roedel
2026-05-05 9:55 ` Vasant Hegde
1 sibling, 1 reply; 6+ messages in thread
From: Sean Christopherson @ 2026-04-06 23:05 UTC (permalink / raw)
To: Joerg Roedel, iommu, linux-kernel, Suravee Suthikulpanit
On Fri, Mar 14, 2025, 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;
> +
> + if (!check_feature(FEATURE_GAM_VAPIC)) {
> amd_iommu_guest_ir = AMD_IOMMU_GUEST_IR_LEGACY_GA;
> return;
> }
>
> base-commit: ea9bd29a9c0d757b3384ae3e633e6bbaddf00725
> --
Another ping. This too still applies.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] iommu/amd: Explicitly bail from enable_iommus_vapic() when in legacy mode
2026-04-06 23:05 ` Sean Christopherson
@ 2026-05-04 8:24 ` Joerg Roedel
0 siblings, 0 replies; 6+ messages in thread
From: Joerg Roedel @ 2026-05-04 8:24 UTC (permalink / raw)
To: Sean Christopherson, Vasant Hegde
Cc: iommu, linux-kernel, Suravee Suthikulpanit
Vasant, could you please have a look? I'd prefer a review from you or Suravee
before applying.
Thanks,
Joerg
On Mon, Apr 06, 2026 at 04:05:09PM -0700, Sean Christopherson wrote:
> On Fri, Mar 14, 2025, 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;
> > +
> > + if (!check_feature(FEATURE_GAM_VAPIC)) {
> > amd_iommu_guest_ir = AMD_IOMMU_GUEST_IR_LEGACY_GA;
> > return;
> > }
> >
> > base-commit: ea9bd29a9c0d757b3384ae3e633e6bbaddf00725
> > --
>
> Another ping. This too still applies.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] iommu/amd: Explicitly bail from enable_iommus_vapic() when in legacy mode
2025-03-15 3:09 [PATCH] iommu/amd: Explicitly bail from enable_iommus_vapic() when in legacy mode Sean Christopherson
2026-04-06 23:05 ` Sean Christopherson
@ 2026-05-05 9:55 ` Vasant Hegde
2026-05-05 17:41 ` Sean Christopherson
1 sibling, 1 reply; 6+ messages in thread
From: Vasant Hegde @ 2026-05-05 9:55 UTC (permalink / raw)
To: Sean Christopherson, Joerg Roedel
Cc: iommu, linux-kernel, Suravee Suthikulpanit
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().
-Vasant
> + if (!check_feature(FEATURE_GAM_VAPIC)) {
> amd_iommu_guest_ir = AMD_IOMMU_GUEST_IR_LEGACY_GA;
> return;
> }
>
> base-commit: ea9bd29a9c0d757b3384ae3e633e6bbaddf00725
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] iommu/amd: Explicitly bail from enable_iommus_vapic() when in legacy mode
2026-05-05 9:55 ` Vasant Hegde
@ 2026-05-05 17:41 ` Sean Christopherson
2026-05-06 5:16 ` Vasant Hegde
0 siblings, 1 reply; 6+ messages in thread
From: Sean Christopherson @ 2026-05-05 17:41 UTC (permalink / raw)
To: Vasant Hegde; +Cc: Joerg Roedel, iommu, linux-kernel, Suravee Suthikulpanit
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;
}
/* 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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] iommu/amd: Explicitly bail from enable_iommus_vapic() when in legacy mode
2026-05-05 17:41 ` Sean Christopherson
@ 2026-05-06 5:16 ` Vasant Hegde
0 siblings, 0 replies; 6+ messages in thread
From: Vasant Hegde @ 2026-05-06 5:16 UTC (permalink / raw)
To: Sean Christopherson
Cc: Joerg Roedel, iommu, linux-kernel, Suravee Suthikulpanit
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2026-05-06 5:17 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-03-15 3:09 [PATCH] iommu/amd: Explicitly bail from enable_iommus_vapic() when in legacy mode Sean Christopherson
2026-04-06 23:05 ` Sean Christopherson
2026-05-04 8:24 ` Joerg Roedel
2026-05-05 9:55 ` Vasant Hegde
2026-05-05 17:41 ` Sean Christopherson
2026-05-06 5:16 ` Vasant Hegde
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox