Re: [PATCH v3 03/27] KVM: VMX: Generalize VPID allocation to be vendor-neutral

[Yosry Ahmed]

On Wed, Oct 14, 2026 at 11:23 AM Jim Mattson <jmattson@xxxxxxxxxx> wrote:
>
> On Mon, Oct 12, 2026 at 4:36 PM Yosry Ahmed <yosry@xxxxxxxxxx> wrote:
> >
> > In preparation for sharing with SVM, generalize the VMX VPID allocation
> > code and move it to common code as a TLB tags allocator. Parameterize
> > the TLB tags allocator by the number of tags, and allocate the bitmap
> > dynamically. Opportunisitcally use guards to acquire the lock instead of
>
> s/Opportunisitcally/Opportunistically/
>
> > spin_{lock/unlock}().
> >
> > Keep the number of allowed tags capped at VMX's hardware cap, to avoid
> > allocating a huge bitmap if L0 advertises a huge number of ASIDs to an
> > L1 KVM. Realistically, the number of actual hardware ASIDs wouldn't be
> > that large so there is no benefit.
> >
> > Initialize the TLB tags allocator during hardware setup/unsetup, and
> > reserve tag=0 during initialziation, similar to how VPID=0 is currently
>
> s/initialziation/initialization/
>
> > reserved in the VMX-specific bitmap during hardware setup.
> >
> > Allow nr=0 to allow allocating all ASIDs to SEV on AMD without failing
> > hardware setup, if at all possible, in which case any tag allocation
> > fails (SEV won't use the tag allocator).  The number of tags includes
> > tag=0, which is not usable. The interface is a little confusing in that
> > regard, but this will be changed soon when reserved tags are explicitly
> > introduced.
> >
> > Keep allocate_vpid() and free_vpid() as wrapper that check enable_vpid
> > to avoid checking at all callsites, and add init_vpids() and
> > destroy_vpids() to wrap init/destroy calls as well.
> >
> > No functional change intended.
>
> I don't think that's really true. If it is, then you failed. For
> example, the new -ENOMEM when the bitmap can't be dynamically
> allocated.

Point taken, will remove the claim.

>
> > Signed-off-by: Yosry Ahmed <yosry@xxxxxxxxxx>
> > ---
> >  arch/x86/kvm/mmu.h     |  8 +++++
> >  arch/x86/kvm/mmu/mmu.c | 78 ++++++++++++++++++++++++++++++++++++++++++
> >  arch/x86/kvm/vmx/vmx.c | 40 +++++-----------------
> >  arch/x86/kvm/vmx/vmx.h | 28 ++++++++++++---
> >  4 files changed, 119 insertions(+), 35 deletions(-)
> >
> > diff --git a/arch/x86/kvm/mmu.h b/arch/x86/kvm/mmu.h
> > index 2ae7f9ed4cf86..de79e002edf8f 100644
> > --- a/arch/x86/kvm/mmu.h
> > +++ b/arch/x86/kvm/mmu.h
> > @@ -410,4 +410,12 @@ static inline bool kvm_is_gfn_alias(struct kvm *kvm, gfn_t gfn)
> >  {
> >         return gfn & kvm_gfn_direct_bits(kvm);
> >  }
> > +
> > +typedef unsigned int kvm_tlb_tag_t;
> > +
> > +int kvm_init_tlb_tags(unsigned int nr);
> > +void kvm_destroy_tlb_tags(void);
> > +kvm_tlb_tag_t kvm_alloc_tlb_tag(void);
> > +void kvm_free_tlb_tag(kvm_tlb_tag_t tag);
> > +
> >  #endif
> > diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> > index aadd5bdc2c97a..97d49247d9d82 100644
> > --- a/arch/x86/kvm/mmu/mmu.c
> > +++ b/arch/x86/kvm/mmu/mmu.c
> > @@ -8178,6 +8178,84 @@ void kvm_mmu_pre_destroy_vm(struct kvm *kvm)
> >                 vhost_task_stop(kvm->arch.nx_huge_page_recovery_thread);
> >  }
> >
> > +static struct {
> > +       spinlock_t      lock;
> > +       unsigned long   *bitmap;
> > +       unsigned int    nr;
> > +} tlb_tags;
>
> Can we statically initialize this to avoid the call to spin_lock_init()?
>
> ... tlb_tags = {
>         .lock = __SPIN_LOCK_UNLOCKED(tlb_tags.lock),
> };

The call to spin_lock_init() is less code, but I can make the change.
I don't have a preference.

>
> > +
> > +int kvm_init_tlb_tags(unsigned int nr)
> > +{
> > +       /*
> > +        * Limit the number of TLB tags to VMX's hardcoded maximum of 0x10000
> > +        * to avoid wasting memory for the bitmap in the unlikely scenario the
> > +        * CPU supports an inordinate number of ASIDs (on AMD).  If userspace
> > +        * wants to concurrently run tens of thousands of vCPUs, they'll likely
> > +        * need a solution that works for both Intel and AMD.
> > +        */
> > +       const unsigned int MAX_NR_TLB_TAGS = VMX_NR_VPIDS;
> > +
> > +       if (!nr)
> > +               return 0;
> > +
> > +       if (nr > MAX_NR_TLB_TAGS) {
> > +               pr_warn_once("Number of TLB tags capped (%u instead of %u)\n",
> > +                            MAX_NR_TLB_TAGS, nr);
> > +               nr = MAX_NR_TLB_TAGS;
> > +       }
> > +
> > +       tlb_tags.bitmap = bitmap_zalloc(nr, GFP_KERNEL);
> > +       if (!tlb_tags.bitmap)
> > +               return -ENOMEM;
> > +
> > +       /*
> > +        * 0 is the host's TLB tag for both VMX's VPID and SVM's ASID, and is
> > +        * returned on failed allocations (e.g. no more tags left).
> > +        */
> > +       __set_bit(0, tlb_tags.bitmap);
> > +
> > +       tlb_tags.nr = nr;
> > +       spin_lock_init(&tlb_tags.lock);
> > +       return 0;
> > +}
> > +EXPORT_SYMBOL_FOR_KVM_INTERNAL(kvm_init_tlb_tags);
> > +
> > +void kvm_destroy_tlb_tags(void)
> > +{
> > +       bitmap_free(tlb_tags.bitmap);
> > +       tlb_tags.bitmap = NULL;
>
> For consistency and future-proofing, it might be wise to clear tlb_tags.nr here.

Can do.

>
> > +}
> > +EXPORT_SYMBOL_FOR_KVM_INTERNAL(kvm_destroy_tlb_tags);