linux-kernel - Re: [PATCH v3 5/6] KVM: SVM: Move global "avic" variable to avic.c

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <73txiv6ycd3umvlptnqnepsc6hozuo4rfmyqj4rhtv7ahkm43k@37jbftataicw>
Date: Fri, 19 Sep 2025 16:01:55 +0530
From: Naveen N Rao <naveen@...nel.org>
To: Sean Christopherson <seanjc@...gle.com>
Cc: Paolo Bonzini <pbonzini@...hat.com>, kvm@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 5/6] KVM: SVM: Move global "avic" variable to avic.c

On Thu, Sep 18, 2025 at 05:21:35PM -0700, Sean Christopherson wrote:
> Move "avic" to avic.c so that it's colocated with the other AVIC specific
> globals and module params, and so that avic_hardware_setup() is a bit more
> self-contained, e.g. similar to sev_hardware_setup().
> 
> Deliberately set enable_apicv in svm.c as it's already globally visible
> (defined by kvm.ko, not by kvm-amd.ko), and to clearly capture the
> dependency on enable_apicv being initialized (svm_hardware_setup() clears
> several AVIC-specific hooks when enable_apicv is disabled).
> 
> Alternatively, clearing of the hooks (and enable_ipiv) could be moved to
> avic_hardware_setup(), but that's not obviously better, e.g. it's helpful
> to isolate the setting of enable_apicv when reading code from the generic
> x86 side of the world.
> 
> No functional change intended.
> 
> Cc: Naveen N Rao (AMD) <naveen@...nel.org>
> Signed-off-by: Sean Christopherson <seanjc@...gle.com>
> ---
>  arch/x86/kvm/svm/avic.c | 32 ++++++++++++++++++++++++--------
>  arch/x86/kvm/svm/svm.c  | 11 +----------
>  2 files changed, 25 insertions(+), 18 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
> index 497d755c206f..e059dcae6945 100644
> --- a/arch/x86/kvm/svm/avic.c
> +++ b/arch/x86/kvm/svm/avic.c
> @@ -64,6 +64,14 @@
>  
>  static_assert(__AVIC_GATAG(AVIC_VM_ID_MASK, AVIC_VCPU_IDX_MASK) == -1u);
>  
> +/*
> + * enable / disable AVIC.  Because the defaults differ for APICv
> + * support between VMX and SVM we cannot use module_param_named.
> + */
> +static bool avic;
> +module_param(avic, bool, 0444);
> +module_param(enable_ipiv, bool, 0444);
> +
>  static bool force_avic;
>  module_param_unsafe(force_avic, bool, 0444);
>  
> @@ -1141,15 +1149,9 @@ void avic_vcpu_unblocking(struct kvm_vcpu *vcpu)
>  	avic_vcpu_load(vcpu, vcpu->cpu);
>  }
>  
> -/*
> - * Note:
> - * - The module param avic enable both xAPIC and x2APIC mode.
> - * - Hypervisor can support both xAVIC and x2AVIC in the same guest.
> - * - The mode can be switched at run-time.
> - */
> -bool __init avic_hardware_setup(struct kvm_x86_ops *svm_ops)
> +static bool __init avic_want_avic_enable(void)

Maybe avic_can_enable()?

>  {
> -	if (!npt_enabled)
> +	if (!avic || !npt_enabled)
>  		return false;
>  
>  	/* AVIC is a prerequisite for x2AVIC. */
> @@ -1174,6 +1176,20 @@ bool __init avic_hardware_setup(struct kvm_x86_ops *svm_ops)
>  		pr_warn("AVIC unsupported in CPUID but force enabled, your system might crash and burn\n");
>  
>  	pr_info("AVIC enabled\n");

I think it would be good to keep this in avic_hardware_setup() alongside 
the message printing "x2AVIC enabled".

Otherwise:
Acked-by: Naveen N Rao (AMD) <naveen@...nel.org>


- Naveen