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

[Sean Christopherson <seanjc@google.com>]

On Thu, Sep 03, 2026, Yosry Ahmed wrote:
> On Thu, Sep 3, 2026 at 2:53 PM Sean Christopherson <seanjc@google.com> 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.