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

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

[Naveen N Rao (AMD)]

If vCPU creation fails after kvm_arch_vcpu_create(), the AVIC Physical
ID table entry corresponding to that vCPU continues to point to the
freed APIC backing page which can result in UAF. Address this by
clearing out the corresponding AVIC Physical ID table entry in the
vcpu_free() callback, similar to the VMX commit b41f2ca6c060 ("KVM: VMX:
Fix stale PID-pointer table entry left after vCPU free").

Though unlikely, it is also possible that svm_vcpu_create() itself fails
after the AVIC Physical ID table entry has been setup if memory
allocation fails in svm_vcpu_alloc_msrpm(). Clear the entry in this path
as well.

Note: this change depends on commit 97d65b544f48 ("KVM: Check for
duplicate vcpu_id as early as possible"), which ensures that a vCPU with
a duplicate ID is never created. Otherwise, a valid AVIC Physical ID
table entry for an existing vCPU will be cleared.

Fixes: 44a95dae1d22 ("KVM: x86: Detect and Initialize AVIC support")
Signed-off-by: Naveen N Rao (AMD) <naveen@kernel.org>
---
 arch/x86/kvm/svm/svm.h  |  1 +
 arch/x86/kvm/svm/avic.c | 10 ++++++++++
 arch/x86/kvm/svm/svm.c  |  6 +++++-
 3 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
index e958943b8162..790bd96a9791 100644
--- a/arch/x86/kvm/svm/svm.h
+++ b/arch/x86/kvm/svm/svm.h
@@ -954,6 +954,7 @@ void avic_init_vmcb(struct vcpu_svm *svm, struct vmcb *vmcb);
 int avic_incomplete_ipi_interception(struct kvm_vcpu *vcpu);
 int avic_unaccelerated_access_interception(struct kvm_vcpu *vcpu);
 int avic_init_vcpu(struct vcpu_svm *svm);
+void avic_vcpu_free(struct kvm_vcpu *vcpu);
 void avic_vcpu_load(struct kvm_vcpu *vcpu, int cpu);
 void avic_vcpu_put(struct kvm_vcpu *vcpu);
 void avic_apicv_post_state_restore(struct kvm_vcpu *vcpu);
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;
+	struct kvm_svm *kvm_svm = to_kvm_svm(vcpu->kvm);
+	u32 id = vcpu->vcpu_id;
+
+	if (kvm_svm->avic_physical_id_table && id <= max_id)
+		WRITE_ONCE(kvm_svm->avic_physical_id_table[id], 0);
+}
+
 void avic_apicv_post_state_restore(struct kvm_vcpu *vcpu)
 {
 	avic_handle_dfr_update(vcpu);
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index ea647938a2a6..c2a2ae7d8ec3 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -1337,7 +1337,7 @@ static int svm_vcpu_create(struct kvm_vcpu *vcpu)
 	svm->msrpm = svm_vcpu_alloc_msrpm();
 	if (!svm->msrpm) {
 		err = -ENOMEM;
-		goto error_free_sev;
+		goto error_free_avic;
 	}
 
 	svm->x2avic_msrs_intercepted = true;
@@ -1351,6 +1351,8 @@ static int svm_vcpu_create(struct kvm_vcpu *vcpu)
 
 	return 0;
 
+error_free_avic:
+	avic_vcpu_free(vcpu);
 error_free_sev:
 	sev_free_vcpu(vcpu);
 error_free_vmcb_page:
@@ -1365,6 +1367,8 @@ static void svm_vcpu_free(struct kvm_vcpu *vcpu)
 
 	WARN_ON_ONCE(!list_empty(&svm->ir_list));
 
+	avic_vcpu_free(vcpu);
+
 	svm_leave_nested(vcpu);
 	svm_free_nested(svm);
 

base-commit: 76671054f9a1ff6abb976583cd8da37650acdc97
-- 
2.55.0

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

[Atish Patra]

On 9/2/26 11:28 PM, Naveen N Rao (AMD) wrote:
> If vCPU creation fails after kvm_arch_vcpu_create(), the AVIC Physical
> ID table entry corresponding to that vCPU continues to point to the
> freed APIC backing page which can result in UAF. Address this by
> clearing out the corresponding AVIC Physical ID table entry in the
> vcpu_free() callback, similar to the VMX commit b41f2ca6c060 ("KVM: VMX:
> Fix stale PID-pointer table entry left after vCPU free").
>
> Though unlikely, it is also possible that svm_vcpu_create() itself fails
> after the AVIC Physical ID table entry has been setup if memory
> allocation fails in svm_vcpu_alloc_msrpm(). Clear the entry in this path
> as well.
>
> Note: this change depends on commit 97d65b544f48 ("KVM: Check for
> duplicate vcpu_id as early as possible"), which ensures that a vCPU with
> a duplicate ID is never created. Otherwise, a valid AVIC Physical ID
> table entry for an existing vCPU will be cleared.
>
> Fixes: 44a95dae1d22 ("KVM: x86: Detect and Initialize AVIC support")
> Signed-off-by: Naveen N Rao (AMD) <naveen@kernel.org>
> ---
>   arch/x86/kvm/svm/svm.h  |  1 +
>   arch/x86/kvm/svm/avic.c | 10 ++++++++++
>   arch/x86/kvm/svm/svm.c  |  6 +++++-
>   3 files changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
> index e958943b8162..790bd96a9791 100644
> --- a/arch/x86/kvm/svm/svm.h
> +++ b/arch/x86/kvm/svm/svm.h
> @@ -954,6 +954,7 @@ void avic_init_vmcb(struct vcpu_svm *svm, struct vmcb *vmcb);
>   int avic_incomplete_ipi_interception(struct kvm_vcpu *vcpu);
>   int avic_unaccelerated_access_interception(struct kvm_vcpu *vcpu);
>   int avic_init_vcpu(struct vcpu_svm *svm);
> +void avic_vcpu_free(struct kvm_vcpu *vcpu);
>   void avic_vcpu_load(struct kvm_vcpu *vcpu, int cpu);
>   void avic_vcpu_put(struct kvm_vcpu *vcpu);
>   void avic_apicv_post_state_restore(struct kvm_vcpu *vcpu);
> 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;
> +	struct kvm_svm *kvm_svm = to_kvm_svm(vcpu->kvm);
> +	u32 id = vcpu->vcpu_id;
> +
> +	if (kvm_svm->avic_physical_id_table && id <= max_id)
> +		WRITE_ONCE(kvm_svm->avic_physical_id_table[id], 0);
> +}
> +
>   void avic_apicv_post_state_restore(struct kvm_vcpu *vcpu)
>   {
>   	avic_handle_dfr_update(vcpu);
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index ea647938a2a6..c2a2ae7d8ec3 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -1337,7 +1337,7 @@ static int svm_vcpu_create(struct kvm_vcpu *vcpu)
>   	svm->msrpm = svm_vcpu_alloc_msrpm();
>   	if (!svm->msrpm) {
>   		err = -ENOMEM;
> -		goto error_free_sev;
> +		goto error_free_avic;
>   	}
>   
>   	svm->x2avic_msrs_intercepted = true;
> @@ -1351,6 +1351,8 @@ static int svm_vcpu_create(struct kvm_vcpu *vcpu)
>   
>   	return 0;
>   
> +error_free_avic:
> +	avic_vcpu_free(vcpu);
>   error_free_sev:
>   	sev_free_vcpu(vcpu);
>   error_free_vmcb_page:
> @@ -1365,6 +1367,8 @@ static void svm_vcpu_free(struct kvm_vcpu *vcpu)
>   
>   	WARN_ON_ONCE(!list_empty(&svm->ir_list));
>   
> +	avic_vcpu_free(vcpu);
> +
>   	svm_leave_nested(vcpu);
>   	svm_free_nested(svm);
>   
>
> base-commit: 76671054f9a1ff6abb976583cd8da37650acdc97

FWIW, we had the same bug report from kres. This patch fixes that.
Verified on AMD beragmo with x2AVIC enabled and a reproducer that makes 
KVM_CREATE_VCPU fail with EMFILE after the entry is published.


Tested-by: Atish Patra <atishp@meta.com>

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

[Sean Christopherson]

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.

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

and then this code becomes:

  void avic_vcpu_free(struct kvm_vcpu *vcpu)
  {
	struct kvm_svm *kvm_svm = to_kvm_svm(vcpu->kvm);

	if (kvm_svm->avic_physical_id_table && avic_is_addressable_vcpu(vcpu))
		WRITE_ONCE(kvm_svm->avic_physical_id_table[vcpu->vcpu_id], 0);
  }

> +	struct kvm_svm *kvm_svm = to_kvm_svm(vcpu->kvm);
> +	u32 id = vcpu->vcpu_id;
> +
> +	if (kvm_svm->avic_physical_id_table && id <= max_id)
> +		WRITE_ONCE(kvm_svm->avic_physical_id_table[id], 0);
> +}

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

[Naveen N Rao]

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(), 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().

> 
> and then this code becomes:
> 
>   void avic_vcpu_free(struct kvm_vcpu *vcpu)
>   {
> 	struct kvm_svm *kvm_svm = to_kvm_svm(vcpu->kvm);
> 
> 	if (kvm_svm->avic_physical_id_table && avic_is_addressable_vcpu(vcpu))
> 		WRITE_ONCE(kvm_svm->avic_physical_id_table[vcpu->vcpu_id], 0);
>   }
> 

Thanks,
Naveen

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

[Sean Christopherson]

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?

diff --git arch/x86/kvm/svm/avic.c arch/x86/kvm/svm/avic.c
index 3b037e385523..3725c033f8e9 100644
--- arch/x86/kvm/svm/avic.c
+++ arch/x86/kvm/svm/avic.c
@@ -395,6 +395,12 @@ 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 * sizeof(u64)) <
+              PAGE_SIZE << avic_get_physical_id_table_order(vcpu->kvm);
+}
+
 void avic_init_vmcb(struct vcpu_svm *svm, struct vmcb *vmcb)
 {
        struct kvm_svm *kvm_svm = to_kvm_svm(svm->vcpu.kvm);
@@ -425,7 +431,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 (id > max_id || WARN_ON_ONCE(!avic_is_addressable_vcpu(vcpu))) {
                kvm_set_apicv_inhibit(vcpu->kvm, APICV_INHIBIT_REASON_PHYSICAL_ID_TOO_BIG);
                vcpu->arch.apic->apicv_active = false;
                return 0;
@@ -1045,8 +1051,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 +1113,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;
 
        /*

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

[Sean Christopherson]

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.

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

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)) {
                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;
 
        /*

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

[Naveen N Rao]

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;
>  
>         /*
> 

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

[Sean Christopherson]

On Wed, Sep 23, 2026, Naveen N Rao wrote:
> On Tue, Sep 22, 2026 at 12:36:15PM -0700, Sean Christopherson wrote:
> 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))

Heh, I discovered this literally ~10 seconds before reading your mail.  Hooray
for tests!

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

I'll keep it as is, Paolo generally prefers "positive" predicates, and I tend to
agree with him, e.g. it makes it the error paths more obvious (the above bug
notwithstanding).

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

[Naveen N Rao]

On Tue, Sep 22, 2026 at 12:08:16PM -0700, 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?

Shouldn't hurt. Though to be entirely honest, it looks a bit redundant 
in avic_init_vmcb(). And we will still have max_id key'ed off the AVIC 
max APIC ID there, which I thought was one of your original issues.

> 
> diff --git arch/x86/kvm/svm/avic.c arch/x86/kvm/svm/avic.c
> index 3b037e385523..3725c033f8e9 100644
> --- arch/x86/kvm/svm/avic.c
> +++ arch/x86/kvm/svm/avic.c
> @@ -395,6 +395,12 @@ 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 * sizeof(u64)) <
> +              PAGE_SIZE << avic_get_physical_id_table_order(vcpu->kvm);
> +}
> +
>  void avic_init_vmcb(struct vcpu_svm *svm, struct vmcb *vmcb)
>  {
>         struct kvm_svm *kvm_svm = to_kvm_svm(svm->vcpu.kvm);
> @@ -425,7 +431,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 (id > max_id || WARN_ON_ONCE(!avic_is_addressable_vcpu(vcpu))) {

I still think it is reasonable to use __avic_get_max_physical_id() here, 
which makes the intent clear:
	if (id > __avic_get_max_physical_id(vcpu->kvm, NULL))

If the __ name is a concern, perhaps a new wrapper can help, seeing as 
this would now be used in two places.


- Naveen