linux-kernel - Re: [PATCH v3 2/6] KVM: SVM: Update "APICv in x2APIC without x2AVIC" in avic.c, not svm.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: <i4znbv2qka5nswuirlbm6ycjmeqmxtfflz6rbukzsdpfte7p3e@wez3k34xsrqa>
Date: Fri, 19 Sep 2025 15:12:19 +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 2/6] KVM: SVM: Update "APICv in x2APIC without x2AVIC"
 in avic.c, not svm.c

On Thu, Sep 18, 2025 at 05:21:32PM -0700, Sean Christopherson wrote:
> Set the "allow_apicv_in_x2apic_without_x2apic_virtualization" flag as part
> of avic_hardware_setup() instead of handling in svm_hardware_setup(), and
> make x2avic_enabled local to avic.c (setting the flag was the only use in
> svm.c).
> 
> Opportunistically tag avic_hardware_setup() with __init to make it clear
> that nothing untoward is happening with svm_x86_ops.
> 
> No functional change intended (aside from the side effects of tagging
> avic_hardware_setup() with __init).
> 
> Signed-off-by: Sean Christopherson <seanjc@...gle.com>
> ---
>  arch/x86/kvm/svm/avic.c | 6 ++++--
>  arch/x86/kvm/svm/svm.c  | 4 +---
>  arch/x86/kvm/svm/svm.h  | 3 +--
>  3 files changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
> index 478a18208a76..683411442476 100644
> --- a/arch/x86/kvm/svm/avic.c
> +++ b/arch/x86/kvm/svm/avic.c
> @@ -77,7 +77,7 @@ static DEFINE_HASHTABLE(svm_vm_data_hash, SVM_VM_DATA_HASH_BITS);
>  static u32 next_vm_id = 0;
>  static bool next_vm_id_wrapped = 0;
>  static DEFINE_SPINLOCK(svm_vm_data_hash_lock);
> -bool x2avic_enabled;
> +static bool x2avic_enabled;
>  
>  
>  static void avic_set_x2apic_msr_interception(struct vcpu_svm *svm,
> @@ -1147,7 +1147,7 @@ void avic_vcpu_unblocking(struct kvm_vcpu *vcpu)
>   * - Hypervisor can support both xAVIC and x2AVIC in the same guest.
>   * - The mode can be switched at run-time.
>   */
> -bool avic_hardware_setup(void)
> +bool __init avic_hardware_setup(struct kvm_x86_ops *svm_ops)
>  {
>  	if (!npt_enabled)
>  		return false;
> @@ -1182,6 +1182,8 @@ bool avic_hardware_setup(void)
>  	x2avic_enabled = boot_cpu_has(X86_FEATURE_X2AVIC);
>  	if (x2avic_enabled)
>  		pr_info("x2AVIC enabled\n");
> +	else
> +		svm_ops->allow_apicv_in_x2apic_without_x2apic_virtualization = true;

I'm not entirely convinced that this is better since svm_x86_ops fields 
are now being updated outside of svm.c. But, I do see your point about 
limiting x2avic_enabled to avic.c

Would it be better to name this field as svm_x86_ops here too, so it is 
at least easy to grep and find?

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


- Naveen