Message-ID: <aYmvWLjNaJ5fbOcO@blrnaveerao1>
Date: Mon, 9 Feb 2026 15:53:01 +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, 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 at 10:17:29AM -0800, Sean Christopherson wrote:
> 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.

LGTM.

>  
> > > 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)

Glad to hear that!

> 
> > > ---
> > >  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.

Yes, that's better. I'll use that and post it as a subsequent cleanup, 
unless you want to pick it up rightaway.


Thanks!
- Naveen

Powered by blists - more mailing lists