[PATCH] KVM: SVM: Fix an off-by-one typo in the comment for enabling AVIC by default

Sean Christopherson posted 1 patch 1 month ago
There is a newer version of this series
arch/x86/kvm/svm/avic.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
[PATCH] KVM: SVM: Fix an off-by-one typo in the comment for enabling AVIC by default
Posted by Sean Christopherson 1 month ago
Fix a goof in the comment that documents KVM's logic for enabling AVIC by
default to reference Zen5+ as family 0x1A (Zen5), not family 0x19 (Zen4).
The code is correct (checks for _greater_ than 0x19), only the comment is
flawed.

Fixes: ca2967de5a5b ("KVM: SVM: Enable AVIC by default for Zen4+ if x2AVIC is support")
Cc: Naveen N Rao (AMD) <naveen@kernel.org>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/svm/avic.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
index 6b77b2033208..7e62d05c2136 100644
--- a/arch/x86/kvm/svm/avic.c
+++ b/arch/x86/kvm/svm/avic.c
@@ -1224,7 +1224,7 @@ static bool __init avic_want_avic_enabled(void)
 	 * In "auto" mode, enable AVIC by default for Zen4+ if x2AVIC is
 	 * supported (to avoid enabling partial support by default, and because
 	 * x2AVIC should be supported by all Zen4+ CPUs).  Explicitly check for
-	 * family 0x19 and later (Zen5+), as the kernel's synthetic ZenX flags
+	 * family 0x1A and later (Zen5+), as the kernel's synthetic ZenX flags
 	 * aren't inclusive of previous generations, i.e. the kernel will set
 	 * at most one ZenX feature flag.
 	 */

base-commit: 9448598b22c50c8a5bb77a9103e2d49f134c9578
-- 
2.52.0.351.gbe84eed79e-goog
Re: [PATCH] KVM: SVM: Fix an off-by-one typo in the comment for enabling AVIC by default
Posted by Naveen N Rao 1 month ago
On Wed, Jan 07, 2026 at 12:45:46PM -0800, Sean Christopherson wrote:
> Fix a goof in the comment that documents KVM's logic for enabling AVIC by
> default to reference Zen5+ as family 0x1A (Zen5), not family 0x19 (Zen4).
> The code is correct (checks for _greater_ than 0x19), only the comment is
> flawed.

I had thought that the comment was correct and that you wanted to 
reference Zen4 there. That is:
	family 0x19 (Zen4) and later (Zen5+),

Though family 0x19 also includes Zen3 :/

I think it would be better to update the code as well, just so it is 
easier to correlate the comment and the code?

        if (avic == AVIC_AUTO_MODE)
                avic = boot_cpu_has(X86_FEATURE_X2AVIC) &&
-                      (boot_cpu_data.x86 > 0x19 || cpu_feature_enabled(X86_FEATURE_ZEN4));
+                      (cpu_feature_enabled(X86_FEATURE_ZEN4) || boot_cpu_data.x86 >= 0x1A);
 
        if (!avic || !npt_enabled)
		return false;

> 
> Fixes: ca2967de5a5b ("KVM: SVM: Enable AVIC by default for Zen4+ if x2AVIC is support")
> Cc: Naveen N Rao (AMD) <naveen@kernel.org>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  arch/x86/kvm/svm/avic.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Regardless of that, this change is accurate:
Acked-by: Naveen N Rao (AMD) <naveen@kernel.org>

> 
> diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
> index 6b77b2033208..7e62d05c2136 100644
> --- a/arch/x86/kvm/svm/avic.c
> +++ b/arch/x86/kvm/svm/avic.c
> @@ -1224,7 +1224,7 @@ static bool __init avic_want_avic_enabled(void)
>  	 * In "auto" mode, enable AVIC by default for Zen4+ if x2AVIC is
>  	 * supported (to avoid enabling partial support by default, and because
>  	 * x2AVIC should be supported by all Zen4+ CPUs).  Explicitly check for
> -	 * family 0x19 and later (Zen5+), as the kernel's synthetic ZenX flags
> +	 * family 0x1A and later (Zen5+), as the kernel's synthetic ZenX flags
>  	 * aren't inclusive of previous generations, i.e. the kernel will set
>  	 * at most one ZenX feature flag.
>  	 */
> 
> base-commit: 9448598b22c50c8a5bb77a9103e2d49f134c9578
> -- 
> 2.52.0.351.gbe84eed79e-goog
> 

- Naveen
Re: [PATCH] KVM: SVM: Fix an off-by-one typo in the comment for enabling AVIC by default
Posted by Sean Christopherson 1 month ago
On Thu, Jan 08, 2026, Naveen N Rao wrote:
> On Wed, Jan 07, 2026 at 12:45:46PM -0800, Sean Christopherson wrote:
> > Fix a goof in the comment that documents KVM's logic for enabling AVIC by
> > default to reference Zen5+ as family 0x1A (Zen5), not family 0x19 (Zen4).
> > The code is correct (checks for _greater_ than 0x19), only the comment is
> > flawed.
> 
> I had thought that the comment was correct and that you wanted to 
> reference Zen4 there. That is:
> 	family 0x19 (Zen4) and later (Zen5+),
> 
> Though family 0x19 also includes Zen3 :/
> 
> I think it would be better to update the code as well, just so it is 
> easier to correlate the comment and the code?
> 
>         if (avic == AVIC_AUTO_MODE)
>                 avic = boot_cpu_has(X86_FEATURE_X2AVIC) &&
> -                      (boot_cpu_data.x86 > 0x19 || cpu_feature_enabled(X86_FEATURE_ZEN4));
> +                      (cpu_feature_enabled(X86_FEATURE_ZEN4) || boot_cpu_data.x86 >= 0x1A);

This thought crossed my mind as well.  I'll send a v2 with this, I especially
like the idea of swapping the ordering so that the checks are "ascending".