Re: [PATCH] KVM: SVM: Trigger new ASID allocation in both VMCBs on pCPU switch

[Yosry Ahmed]

On Thu, Sep 3, 2026 at 2:53 PM Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
>
> On Thu, Sep 03, 2026, Yosry Ahmed wrote:
> > When the pCPU where the VMCB was mostly recently used is switched, reset
> > the ASID generation in both VMCBs, triggering new ASID allocation for
> > the immediate VMRUN as well as the next VMRUN on the other VMCB.
> >
> > The ASID is shared between vmcb01 and vmcb02, and gets flushed on every
> > nested transition. However, since pCPU tracking is done per VMCB, it is
> > possible for one VMCB to allocate a new ASID when migrated to a new
> > pCPU, and then the other VMCB reuses that ASID on the old pCPU. This can
> > result in the same ASID being used by multiple vCPUs on the old pCPU.
> >
> > Example scenario:
> > - vCPU runs on pCPU A, vmcb01 is active, asid=1.
> > - vCPU migrates to pCPU B, vmcb01 pCPU changes, new asid=2.
> > - Another vCPU runs on pCPU A and allocates asid=2 as well.
> > - vCPU migrates back to pCPU A, and then switches to vmcb02 before it
> >   runs again with vmcb01.
> > - No pCPU switch is detected for vmcb02, so VMRUN is done with asid=2.
> > - Two vCPUs end up using asid=2 on the same pCPU.
> >
> > Keep the VMCB dirtying to the active VMCB only. Clean bits are tracked
> > by a pCPU for each VMCB, so do not unnecessarily dirty a VMCB if its
> > pCPU does not change.
> >
> > Additionally, initialize the tracker pCPU for vmcb02 to -1 on nested
> > enablement, so that the new ASID allocation in the scenario above
> > happens even if EFER.SVME is disabled in L1 before migrating to the new
> > pCPU (so asid_generation in vmcb02 is not reset), but enabled before
> > returning to the old pCPU.
> >
> > No performance regression was noticed when overcommitting L1 vCPUs in L0
> > (to force rescheduling), pinning L1 <-> L2 vCPUs, and running CPUID in a
> > tight loop bouncing between 2 vCPUs in L2.
> >
> > An alternative (and perhaps more proper) fix would be tracking the ASID
> > per-VMCB instead (e.g. [1]). However, that's a more involved change, and
> > it would result in having different ASIDs for L1 and L2 without actually
> > properly maintaining them. It would probably work because all TLB
> > flushes target the current VMCB, and the other VMCB is always flushed on
> > nested transitions, but the code ends up in an arguably more fragile
> > state. Punt a proper clean fix to an incoming (and overdue) overhaul of
> > SVM's ASID usage [2].
> >
> > [1]https://lore.kernel.org/lkml/20250205182402.2147495-2-yosry.ahmed@xxxxxxxxx/
> > [2]https://lore.kernel.org/kvm/20260728003557.1136583-1-yosry@xxxxxxxxxx/
> >
>
> Fixes: 193015adf40d ("KVM: nSVM: Track the ASID generation of the vmcb vmrun through the vmcb")
>
> > Cc: stable@xxxxxxxxxxxxxxx
> > Reported-by: Stefan Teodorescu <fane@xxxxxxxxxx>
> > Signed-off-by: Yosry Ahmed <yosry@xxxxxxxxxx>
> > ---
> >
> > I wasn't sure if the last paragraph (or parts of it) fit in the
> > changelog or below ---, so I just put it all in the changelog, but feel
> > free to move things around.
>
> I like having the alternative(s) listed in the changelog, it saves having to
> find the alternative when digging through git (if the reader is even aware there
> was/is an alternative).
>
> I agree with your assessment, tracking per-VMCB is absolutely the right approach
> given that the asid_generation is tracked per-VMCB.  But I hate how SVM manages
> ASIDs and want to burn it with fire.  Taking a quick-and-dirty approach will be
> good motivation for landing the overhaul of ASIDs.
>
> I _was_ going to propose an alternative solution, but it subtly doesn't work.
> More below.
>
> > ---
> >  arch/x86/kvm/svm/nested.c |  1 +
> >  arch/x86/kvm/svm/svm.c    | 20 ++++++++++++++++----
> >  2 files changed, 17 insertions(+), 4 deletions(-)
> >
> > diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> > index 73f37b050d0a0..0c55c71fc6010 100644
> > --- a/arch/x86/kvm/svm/nested.c
> > +++ b/arch/x86/kvm/svm/nested.c
> > @@ -1494,6 +1494,7 @@ int svm_allocate_nested(struct vcpu_svm *svm)
> >       if (!svm->nested.msrpm)
> >               goto err_free_vmcb02;
> >
> > +     svm->nested.vmcb02.cpu = -1;
> >       svm->nested.initialized = true;
> >       return 0;
> >
> > diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> > index ea647938a2a65..c388c6598463e 100644
> > --- a/arch/x86/kvm/svm/svm.c
> > +++ b/arch/x86/kvm/svm/svm.c
> > @@ -3765,14 +3765,26 @@ static int pre_svm_run(struct kvm_vcpu *vcpu)
> >       struct vcpu_svm *svm = to_svm(vcpu);
> >
> >       /*
> > -      * If the previous vmrun of the vmcb occurred on a different physical
> > -      * cpu, then mark the vmcb dirty and assign a new asid.  Hardware's
> > -      * vmcb clean bits are per logical CPU, as are KVM's asid assignments.
> > +      * If the previous VMRUN of the VMCB occurred on a different physical
> > +      * cpu, then mark the VMCB dirty as hardware's clean bits are per pCPU.
> > +      *
> > +      * Reset the ASID generation in both VMCBs. This will lead to assigning
> > +      * a new ASID now, and then again when switching to the other VMCB.
> > +      * However, this is needed as the ASID is shared between the VMCBs, and
> > +      * otherwise it would be possible to use an ASID allocated on one pCPU
> > +      * on another, for example:
> > +      * - vCPU migrates from pCPU A to pCPU B, allocates a new ASID.
> > +      * - vCPU migrates back to pCPU A, and then switches the VMCB.
> > +      * - The new VMCB does not detect a pCPU change and runs on pCPU A with
> > +      *   the new ASID allocated on pCPU B, which is potentially used by
> > +      *   another vCPU/VM.
> >        */
> >       if (unlikely(svm->current_vmcb->cpu != vcpu->cpu)) {
> > -             svm->current_vmcb->asid_generation = 0;
> >               vmcb_mark_all_dirty(svm->vmcb);
> >               svm->current_vmcb->cpu = vcpu->cpu;
> > +             svm->vmcb01.asid_generation = 0;
> > +             if (svm->nested.initialized)
>
> Isn't conditioning the clear on nested.initialized wrong?  It's stupidly contrived,
> but I think it can happen?  Even if it can't, I don't see any reason to conditionally
> zero vmcb02.asid_generation.  Either it's buggy or it's a wash in terms of performance.

Oh yeah it's not needed. I thought the generation is only allocated
when svm->nested.initialized, but that's not true it's always there.
So yeah we can just always zero it, and then we can drop the change in
svm_allocate_nested() too, or keep it as hardening (it seems like a
good idea in general)?

>
>  - vCPUx runs on pCPU A, vmcb02 is active, asid=1.
>  - vCPUx migrates to pCPU B, vmcb02 pCPU changes, new asid=2.
>  - vCPUz runs on pCPU A and allocates asid=2 as well.
>  - vCPUx switches to vmbc01 and disabled nested, but doesn't run, e.g. because
>    userspace stuffs EFER.

IIUC, here userspace wrote EFER.SVME=0..

>  - vCPUx migrates back to pCPU A, and zeroes vmcb01.asid_generation, but not
>    vmcb02.asid_generation.
>  - vCPUx switches to vmcb02, without running vmcb01, again thanks to userspace.

..and here it wrote EFER.SVME=1, in which case svm_allocate_nested()
should set svm->nested.vmcb02.cpu = -1..

>  - vCPUx does VMRUN on vmcb02 with asid=2.

..and we allocate a new ASID here?

>  - Two vCPUs end up using asid=2 on the same pCPU.
>
> The "svm->current_vmcb->cpu != vcpu->cpu" check is also sketchy, but I don't think
> it's outright wrong?
>
> Ugh.  Jumping back a bit, I _was_ going to say that we could revert 193015adf40d
> and then do:
>
> diff --git arch/x86/kvm/svm/svm.c arch/x86/kvm/svm/svm.c
> index b4845e452e69..4c8e2fcd378e 100644
> --- arch/x86/kvm/svm/svm.c
> +++ arch/x86/kvm/svm/svm.c
> @@ -1310,12 +1310,6 @@ void svm_switch_vmcb(struct vcpu_svm *svm, struct kvm_vmcb_info *target_vmcb)
>  {
>         svm->current_vmcb = target_vmcb;
>         svm->vmcb = target_vmcb->ptr;
> -
> -       /*
> -       * Workaround: we don't yet track the ASID generation
> -       * that was active the last time target_vmcb was run.
> -       */
> -       svm->asid_generation = 0;
>  }
>
>  static int svm_vcpu_create(struct kvm_vcpu *vcpu)
> @@ -4531,6 +4525,9 @@ static __no_kcsan fastpath_t svm_vcpu_run(struct kvm_vcpu *vcpu, u64 run_flags)
>         sync_lapic_to_cr8(vcpu);
>
>         if (unlikely(svm->asid != svm->vmcb->control.asid)) {
> +               if (svm->vmcb->control.tlb_ctl != TLB_CONTROL_FLUSH_ALL_ASID)
> +                       svm->vmcb->control.tlb_ctl = TLB_CONTROL_FLUSH_ASID;
> +
>                 svm->vmcb->control.asid = svm->asid;
>                 vmcb_mark_dirty(svm->vmcb, VMCB_ASID);
>         }
>
>
> But after reading the cover letter[*], I can't tell if 193015adf40d was a bug
> fix for a dirty/clean bits bug, a bug fix for ASID reuse, or an optimization (I
> thought it was an optimization until reading the cover letter and looking more
> at commit af18fa775d07 ("KVM: nSVM: Track the physical cpu of the vmcb vmrun
> through the vmcb").
>
> So yeah, hit this with a hammer and defer the proper fix to your cleanup series,
> because I have low confidence that doing a proper fix is the safest approach for
> LTS kernels.

I can send a new version dropping conditioning the reset on
svm->nested.initialized and dropping the change in
svm_allocate_nested(), is this what you had in mind?

>
> [*] https://lore.kernel.org/all/20210112164313.4204-1-cavery@xxxxxxxxxx