linux-kernel - Re: [PATCH 1/2] KVM: SVM: Initialize AVIC VMCB fields if AVIC is enabled with in-kernel APIC

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] [day] [month] [year] [list]

Message-ID: <aYYwOTSIJsdafEvJ@google.com>
Date: Fri, 6 Feb 2026 10:17:29 -0800
From: Sean Christopherson <seanjc@...gle.com>
To: Naveen N Rao <naveen@...nel.org>
Cc: Paolo Bonzini <pbonzini@...hat.com>, kvm@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Jim Mattson <jmattson@...gle.com>, "Maciej S . Szmigiero" <maciej.szmigiero@...cle.com>
Subject: Re: [PATCH 1/2] KVM: SVM: Initialize AVIC VMCB fields if AVIC is
 enabled with in-kernel APIC

On Fri, Feb 06, 2026, Naveen N Rao wrote:
> On Tue, Feb 03, 2026 at 11:07:09AM -0800, Sean Christopherson wrote:
> > Initialize all per-vCPU AVIC control fields in the VMCB if AVIC is enabled
> > in KVM and the VM has an in-kernel local APIC, i.e. if it's _possible_ the
> > vCPU could activate AVIC at any point in its lifecycle.  Configuring the
> > VMCB if and only if AVIC is active "works" purely because of optimizations
> > in kvm_create_lapic() to speculatively set apicv_active if AVIC is enabled
> > *and* to defer updates until the first KVM_RUN.  In quotes because KVM
> 
> I think it will be good to clarify that two issues are being addressed 
> here (it wasn't clear to me to begin with):
> - One, described above, is about calling into avic_init_vmcb() 
>   regardless of the vCPU APICv status.
> - Two, described below is about using the vCPU APICv status for init and 
>   not consulting the VM-level APICv inhibit status.

Yeah, I was worried the changelog didn't capture the second one well, but I was
struggling to come up with wording.  How about this as a penultimate paragraph?

  Note!  Use the vCPU's current APICv status when initializing the VMCB,
  not the VM-level inhibit status.  The state of the VMCB *must* be kept
  consistent with the vCPU's APICv status at all times (KVM elides updates
  that are supposed be nops).  If the vCPU's APICv status isn't up-to-date
  with the VM-level status, then there is guaranteed to be a pending
  KVM_REQ_APICV_UPDATE, i.e. KVM will sync the vCPU with the VM before
  entering the guest.
 
> > likely won't do the right thing if kvm_apicv_activated() is false, i.e. if
> > a vCPU is created while APICv is inhibited at the VM level for whatever
> > reason.  E.g. if the inhibit is *removed* before KVM_REQ_APICV_UPDATE is
> > handled in KVM_RUN, then __kvm_vcpu_update_apicv() will elide calls to
> > vendor code due to seeing "apicv_active == activate".
> >
> > Cleaning up the initialization code will also allow fixing a bug where KVM
> > incorrectly leaves CR8 interception enabled when AVIC is activated without
> > creating a mess with respect to whether AVIC is activated or not.
> > 
> > Cc: stable@...r.kernel.org
> > Signed-off-by: Sean Christopherson <seanjc@...gle.com>
> 
> Any reason not to add a Fixes: tag?

Purely that I couldn't pin down exactly what commit(s) to blame.  Well, that's a
bit of a lie.  If I'm being 100% truthful, I got as far as commit 67034bb9dd5e
and decided I didn't care enough to spend the effort to figure out whether or not
that commit was truly to blame :-)

> It looks like the below commits are to blame, but those are really old so I
> understand if you don't think this is useful:
> Fixes: 67034bb9dd5e ("KVM: SVM: Add irqchip_split() checks before enabling AVIC")
> Fixes: 6c3e4422dd20 ("svm: Add support for dynamic APICv")

LGTM, I'll tack them on.

> Other than that:
> Reviewed-by: Naveen N Rao (AMD) <naveen@...nel.org>

Thanks!  (Seriously, I really appreciate the in-depth reviews)

> > ---
> >  arch/x86/kvm/svm/avic.c | 2 +-
> >  arch/x86/kvm/svm/svm.c  | 2 +-
> >  2 files changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
> > index f92214b1a938..44e07c27b190 100644
> > --- a/arch/x86/kvm/svm/avic.c
> > +++ b/arch/x86/kvm/svm/avic.c
> > @@ -368,7 +368,7 @@ void avic_init_vmcb(struct vcpu_svm *svm, struct vmcb *vmcb)
> >  	vmcb->control.avic_physical_id = __sme_set(__pa(kvm_svm->avic_physical_id_table));
> >  	vmcb->control.avic_vapic_bar = APIC_DEFAULT_PHYS_BASE;
> >  
> > -	if (kvm_apicv_activated(svm->vcpu.kvm))
> > +	if (kvm_vcpu_apicv_active(&svm->vcpu))
> >  		avic_activate_vmcb(svm);
> >  	else
> >  		avic_deactivate_vmcb(svm);
> > diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> > index 5f0136dbdde6..e8313fdc5465 100644
> > --- a/arch/x86/kvm/svm/svm.c
> > +++ b/arch/x86/kvm/svm/svm.c
> > @@ -1189,7 +1189,7 @@ static void init_vmcb(struct kvm_vcpu *vcpu, bool init_event)
> >  	if (guest_cpu_cap_has(vcpu, X86_FEATURE_ERAPS))
> >  		svm->vmcb->control.erap_ctl |= ERAP_CONTROL_ALLOW_LARGER_RAP;
> >  
> > -	if (kvm_vcpu_apicv_active(vcpu))
> > +	if (enable_apicv && irqchip_in_kernel(vcpu->kvm))
> >  		avic_init_vmcb(svm, vmcb);
> 
> Doesn't have to be done as part of this series, but I'm wondering if it 
> makes sense to turn this into a helper to clarify the intent and to make 
> it more obvious:

Hmm, yeah, though my only hesitation is the name.  For whatever reason, "possible"
makes me think "is APICv possible *right now*" (ignoring that I wrote exactly that
in the changelog).

What if we go with kvm_can_use_apicv()?  That would align with vmx_can_use_ipiv()
and vmx_can_use_vtd_pi(), which are pretty much identical in concept.