Re: [PATCH] KVM: SVM: Clear AVIC Physical ID table entry if vCPU creation fails

[Naveen N Rao <naveen@kernel.org>]

On Tue, Sep 22, 2026 at 12:36:15PM -0700, Sean Christopherson wrote:
> On Tue, Sep 22, 2026, Sean Christopherson wrote:
> > On Tue, Sep 22, 2026, Naveen N Rao wrote:
> > > On Fri, Sep 11, 2026 at 11:55:07AM -0700, Sean Christopherson wrote:
> > > > On Thu, Sep 03, 2026, Naveen N Rao (AMD) wrote:
> > > > > diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
> > > > > index 3b037e385523..bc2c699380b3 100644
> > > > > --- a/arch/x86/kvm/svm/avic.c
> > > > > +++ b/arch/x86/kvm/svm/avic.c
> > > > > @@ -885,6 +885,16 @@ int avic_init_vcpu(struct vcpu_svm *svm)
> > > > >  	return ret;
> > > > >  }
> > > > >  
> > > > > +void avic_vcpu_free(struct kvm_vcpu *vcpu)
> > > > > +{
> > > > > +	u32 max_id = x2avic_enabled ? x2avic_max_physical_id : AVIC_MAX_PHYSICAL_ID;
> > > > 
> > > > I don't love the duplicate (triplicate?) code, and looking at the usage in
> > > > avic_init_backing_page() with fresh eyes sketched me out.  It's "fine", because
> > > > KVM will reject vCPU creation if vcpu_id >= kvm->arch.max_vcpu_ids, i.e. checking
> > > > only the architectural max won't exceed this max:
> > > > 
> > > > 	return min(kvm->arch.max_vcpu_ids - 1, arch_max);
> > > > 
> > > > But it's hard to see that, and I can't think of any reason why being paranoid
> > > > during vCPU creation/destruction would be a bad thing.
> > > 
> > > Agreed.
> > > 
> > > > 
> > > > Assuming it actually works (haven't tested yet), I'll send a v2 with a prep patch
> > > > to add:
> > > > 
> > > >   static bool avic_is_addressable_vcpu(struct kvm_vcpu *vcpu)
> > > >   {
> > > > 	return (vcpu->vcpu_id * sizeof(u64)) <
> > > > 	       PAGE_SIZE << avic_get_physical_id_table_order(vcpu->kvm);
> > > >   }
> > > 
> > > Unless you are planning to replace similar checks in __avic_vcpu_load() 
> > > and __avic_vcpu_put(), 
> > 
> > Heh, I had coded up exactly that (and then completely forgot that I was going to
> > send a v2, *sigh*).
> > 
> > > I think it will be simpler to just check the id 
> > > itself and avoid dealing with the table size:
> > > 	return vcpu->vcpu_id <= __avic_get_max_physical_id(kvm, NULL);
> > > 
> > > This helper can then also be used in avic_init_backing_page().
> > 
> > Eww, I missed that wrinkle.   Keying off the table order could get a false negative
> > in avic_init_backing_page(), at least in theory.
> > 
> > What if we do both, sort of?  Convert load/put, and add a sanity check in
> > avic_init_vmcb() as well?
> 
> Uh, I think I had a brain fart and forgot that load()/put() are only called if
> AVIC is or was active.   So unless I'm having a brain fart *now*...
> 
> I like your code a lot more.  I don't see any reason to keep the check on
> avic_get_physical_id_table_order() in load()/put().  If we screw up that math
> *and* don't notice immediately, then we deserve the OOB explosions.

Ha, I think we replied at the same time.

> 
> So this?  Probably over two patches to isolate the changes to load()/put() from
> the change to avic_init_backing_page().

Yes, this LGTM, with a minor fix below.

> 
> diff --git arch/x86/kvm/svm/avic.c arch/x86/kvm/svm/avic.c
> index 3b037e385523..e767e43bfb40 100644
> --- arch/x86/kvm/svm/avic.c
> +++ arch/x86/kvm/svm/avic.c
> @@ -395,6 +395,11 @@ static phys_addr_t avic_get_backing_page_address(struct vcpu_svm *svm)
>         return __sme_set(__pa(svm->vcpu.arch.apic->regs));
>  }
>  
> +static bool avic_is_addressable_vcpu(struct kvm_vcpu *vcpu)
> +{
> +       return vcpu->vcpu_id <= __avic_get_max_physical_id(vcpu->kvm, NULL);
> +}
> +
>  void avic_init_vmcb(struct vcpu_svm *svm, struct vmcb *vmcb)
>  {
>         struct kvm_svm *kvm_svm = to_kvm_svm(svm->vcpu.kvm);
> @@ -412,7 +417,6 @@ void avic_init_vmcb(struct vcpu_svm *svm, struct vmcb *vmcb)
>  
>  static int avic_init_backing_page(struct kvm_vcpu *vcpu)
>  {
> -       u32 max_id = x2avic_enabled ? x2avic_max_physical_id : AVIC_MAX_PHYSICAL_ID;
>         struct kvm_svm *kvm_svm = to_kvm_svm(vcpu->kvm);
>         struct vcpu_svm *svm = to_svm(vcpu);
>         u32 id = vcpu->vcpu_id;
> @@ -425,7 +429,7 @@ static int avic_init_backing_page(struct kvm_vcpu *vcpu)
>          * avic_vcpu_load() expects to be called if and only if the vCPU has
>          * fully initialized AVIC.
>          */
> -       if (id > max_id) {
> +       if (avic_is_addressable_vcpu(vcpu)) {

Missing a negation there:
	if (!avic_is_addressable_vcpu(vcpu))

... which also means all three places are inverting the check, so you 
could optionally flip the helper instead.


Thanks!
- Naveen

>                 kvm_set_apicv_inhibit(vcpu->kvm, APICV_INHIBIT_REASON_PHYSICAL_ID_TOO_BIG);
>                 vcpu->arch.apic->apicv_active = false;
>                 return 0;
> @@ -1045,8 +1049,7 @@ static void __avic_vcpu_load(struct kvm_vcpu *vcpu, int cpu,
>         if (WARN_ON(h_physical_id & ~AVIC_PHYSICAL_ID_ENTRY_HOST_PHYSICAL_ID_MASK))
>                 return;
>  
> -       if (WARN_ON_ONCE(vcpu->vcpu_id * sizeof(entry) >=
> -                        PAGE_SIZE << avic_get_physical_id_table_order(vcpu->kvm)))
> +       if (WARN_ON_ONCE(!avic_is_addressable_vcpu(vcpu)))
>                 return;
>  
>         /*
> @@ -1108,8 +1111,7 @@ static void __avic_vcpu_put(struct kvm_vcpu *vcpu, enum avic_vcpu_action action)
>  
>         lockdep_assert_preemption_disabled();
>  
> -       if (WARN_ON_ONCE(vcpu->vcpu_id * sizeof(entry) >=
> -                        PAGE_SIZE << avic_get_physical_id_table_order(vcpu->kvm)))
> +       if (WARN_ON_ONCE(!avic_is_addressable_vcpu(vcpu)))
>                 return;
>  
>         /*
>