Re: [PATCH] KVM: VMX: Postpone IPIv setup after successful vCPU creation

[Sean Christopherson <seanjc@google.com>]

On Fri, Jul 17, 2026, Kai Huang wrote:
> On Fri, 2026-07-17 at 18:20 +0200, Dmytro Maluka wrote:
> > On Fri, Jul 17, 2026 at 12:59:38PM +0000, Huang, Kai wrote:
> > > > 
> > > > An easy fix would be to clear the pid_table entry in vmx_vcpu_free().
> > > > However that would be still problematic, for the following reason:
> > > > userspace may try to create a vCPU with the same vcpu_id as an existing
> > > > one; vmx_vcpu_create() will succeed, and only after that
> > > > kvm_vm_ioctl_create_vcpu() will check for the duplicate vcpu_id and
> > > > fail with -EEXIST and then free the vCPU in the failure path. So in this
> > > > failure path, vmx_vcpu_free() would clear the pid_table entry for that
> > > > already existing good vCPU, i.e. effectively disable IPIv for that vCPU.
> > > 
> > > IMHO this seems a bit fragile?  If something similar to pid_table coming up in
> > > the future, we could end up with a similar problem.
> > > 
> > > The "duplicated vcpu_id check" seems the ones that should happen as early as
> > > possible.  Is it better to move the "duplicated vcpu_id check" earlier before
> > > vmx_vcpu_create(), e.g., even before the kvm_arch_vcpu_precreate()?  In this
> > > case we can do the pid_table cleanup in vmx_vcpu_free() I think.
> > 
> > Unfortunately it is not that simple. As I understand, the reason why the
> > "duplicated vcpu_id check" is done later is that it needs to be done
> > atomically together with inserting the vCPU into kvm->vcpu_array after
> > it, i.e. both the check and the insertion need to be done with kvm->lock
> > held (rather than releasing kvm->lock and taking it again between the
> > two operations).
> > 
> > IOW, if we just move the "duplicated vcpu_id check" earlier (before the
> > first unlock of kvm->lock), we have a race:
> > 
> > 1. vCPU A is being created but not installed in kvm->vcpu_array yet.
> > 2. vCPU B with the same vcpu_id is being created. It passes the
> >    duplicated vcpu_id check, since the check doesn't find vCPU A in
> >    kvm->vcpu_array.
> > 3. vCPU A is installed in kvm->vcpu_array, vCPU creation succeeds.
> > 4. vCPU B with the same vcpu_id is installed in kvm->vcpu_array, vCPU
> >    creation succeeds.
> > 
> > And we cannot just move the kvm->vcpu_array insertion earlier (before
> > the first unlock of kvm->lock), at least because the vCPU is not even
> > allocated at that point.
> 
> Hmm right, and AFACIT vCPU A needs to actually bump up the kvm->online_vcpus in
> order for the vCPU B to detect the vCPU of same vcpu_id has been created because
> kvm_get_vcpu_by_id() will only scan online vCPUs.
> 
> Thanks for pointing out!
> 
> > I guess we could track the used vcpu_ids separately in another xarray
> > (or a bitmap) which could be checked and updated by the early
> > "duplicated vcpu_id check" before the first unlock of kvm->lock. But do
> > we want to pay the memory price for that?

I'm comfortable paying the cost for a bitmap.  If a system can actually support
thousands of vCPU IDs, then burning a few hundred bytes per VM is all but
guaranteed to be a non-issue.  And for smaller systems, e.g. on arm64 where a
VM can have at most 512 vCPU IDs, the bitmap costs a measely 64 bytes.

And if the wastefulness of a persistent bitmap is a concern, we could easly use
an xarray to track only "pending" vCPUs, so that the steady state cost is ~zero.
Actually, given how easy that is (famous last words), unless removing from an
xarray doesn't free memory soon-ish, I'd say go straight to a semi-permanent
xarray to avoid bikeshedding over the cost of the bitmap.

> I don't think we should do that, and your approach is much safer:

I disagree.  I actually arrived at the bitmap solution before reading this.  My
concern isn't so much about IPI virtualization, it's about the lurking danger of
an unverified vcpu_id.  E.g. see Naveen's suggestion in this thread of simply
letting vcpu_load() initialize the per-VM table, which doesn't work because x86
calls vcpu_load() as part of vCPU creation.  I wouldn't be all that surprised if
there's another bug or two in KVM where colliding 

Ha!  Case in point, s390 had what is effectively the *exact* same bug and fixed
it in the *exact* same way (hooking vcpu_postcreate()) over a decade ago in
commit 255088244929 ("KVM: s390: fix SCA related races and double use"), and that
obviously did nothing to help x86 from repeating the same mistake.

In other words, I want to fix this entire class of bugs, not play a game of
whack-a-mole with bugs that humans are all but guaranteed to overlook.

My other concern with the proposed change is that the vCPU becomes reachable
before long before kvm_arch_vcpu_postcreate().  Which should be fine" for IPI
virtualization, but sets a precedence I'd rather not exist, because initializing
vCPU state _after_ it's reachable is rarely correct.  E.g. msr_kvm_poll_control
is initialized in postcreate for some reason, and that's technically buggy
because it's possible, albeit extremely unlikely, that MSR_KVM_POLL_CONTROL could
be written by userspace before postcreate() runs.

E.g. for the xarray approacy (sketch only, completely untested):

diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index fdbd697d0337..e34ca5920393 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -791,6 +791,7 @@ struct kvm {
        /* The current active memslot set for each address space */
        struct kvm_memslots __rcu *memslots[KVM_MAX_NR_ADDRESS_SPACES];
        struct xarray vcpu_array;
+       struct xarray pending_vcpu_ids;
        /*
         * Protected by slots_lock, but can be read outside if an
         * incorrect answer is acceptable.
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 2df8ee9ecf6c..8bc4f7ffd76d 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -4179,6 +4179,12 @@ static int kvm_vm_ioctl_create_vcpu(struct kvm *kvm, unsigned long id)
                return r;
        }
 
+       r = xa_insert(&kvm->pending_vcpu_ids, id, xa_mk_value(id), GFP_KERNEL);
+       if (r) {
+               mutex_unlock(&kvm->lock);
+               return r;
+       }
+
        kvm->created_vcpus++;
        mutex_unlock(&kvm->lock);
 
@@ -4249,6 +4255,7 @@ static int kvm_vm_ioctl_create_vcpu(struct kvm *kvm, unsigned long id)
         */
        smp_wmb();
        atomic_inc(&kvm->online_vcpus);
+       xa_erase(&kvm->pending_vcpu_ids, id);
        mutex_unlock(&vcpu->mutex);
 
        mutex_unlock(&kvm->lock);
@@ -4273,6 +4280,7 @@ static int kvm_vm_ioctl_create_vcpu(struct kvm *kvm, unsigned long id)
 vcpu_decrement:
        mutex_lock(&kvm->lock);
        kvm->created_vcpus--;
+       xa_erase(&kvm->pending_vcpu_ids, id);
        mutex_unlock(&kvm->lock);
        return r;
 }


or if that doesn't work, the more naive bitmap approach:

diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index fdbd697d0337..eadde580e27f 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -791,6 +791,8 @@ struct kvm {
        /* The current active memslot set for each address space */
        struct kvm_memslots __rcu *memslots[KVM_MAX_NR_ADDRESS_SPACES];
        struct xarray vcpu_array;
+       DECLARE_BITMAP(vcpu_ids, KVM_MAX_VCPU_IDS);
+
        /*
         * Protected by slots_lock, but can be read outside if an
         * incorrect answer is acceptable.
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 2df8ee9ecf6c..c735698e76cf 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -4179,6 +4179,11 @@ static int kvm_vm_ioctl_create_vcpu(struct kvm *kvm, unsigned long id)
                return r;
        }
 
+       if (__test_and_set_bit(id, kvm->vcpu_ids)) {
+               mutex_unlock(&kvm->lock);
+               return -EEXIST;
+       }
+
        kvm->created_vcpus++;
        mutex_unlock(&kvm->lock);
 
@@ -4213,7 +4218,7 @@ static int kvm_vm_ioctl_create_vcpu(struct kvm *kvm, unsigned long id)
 
        mutex_lock(&kvm->lock);
 
-       if (kvm_get_vcpu_by_id(kvm, id)) {
+       if (WARN_ON_ONCE(kvm_get_vcpu_by_id(kvm, id))) {
                r = -EEXIST;
                goto unlock_vcpu_destroy;
        }
@@ -4273,6 +4278,7 @@ static int kvm_vm_ioctl_create_vcpu(struct kvm *kvm, unsigned long id)
 vcpu_decrement:
        mutex_lock(&kvm->lock);
        kvm->created_vcpus--;
+       __clear_bit(id, kvm->vcpu_ids);
        mutex_unlock(&kvm->lock);
        return r;
 }