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

[Yosry Ahmed]

On Thu, Sep 3, 2026 at 3:18 PM Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
>
> On Thu, Sep 03, 2026, Yosry Ahmed wrote:
> > On Thu, Sep 3, 2026 at 2:53 PM Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
> > > >       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)?
>
> Definitely keep it as hardening.
>
> > >  - 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?
>
> Oh, right.  Yeesh, that's subtle.  All the more reason to burn with fire.
>
> > >  - 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?
>
> Keep the change in svm_allocate_nested().  Or just do nothing, I'll happily drop
> the svm->nested.initialized check when applying.

You'll need to update the changelog too.

Instead of:

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.

Maybe:

Additionally, initialize the tracked pCPU for vmcb02 to -1 on nested
enablement as hardening, so that a new ASID allocation is always
triggered when nested is disabled and re-enabled.

Let me know if you still wanna fixup when applying or a v2 :)