Re: [PATCH 6/7] KVM: SVM: Use a single ASID per VM

[Jim Mattson]

On Mon, Mar 17, 2025 at 2:12 PM Yosry Ahmed <yosry.ahmed@xxxxxxxxx> wrote:
>
> On Thu, Mar 13, 2025 at 09:55:39PM +0000, Yosry Ahmed wrote:
> > The ASID generation and dynamic ASID allocation logic is now only used
> > by initialization the generation to 0 to trigger a new ASID allocation
> > per-vCPU on the first VMRUN, so the ASID is more-or-less static
> > per-vCPU.
> >
> > Moreover, having a unique ASID per-vCPU is not required. ASIDs are local
> > to each physical CPU, and the ASID is flushed when a vCPU is migrated to
> > a new physical CPU anyway. SEV VMs have been using a single ASID per VM
> > already for other reasons.
> >
> > Use a static ASID per VM and drop the dynamic ASID allocation logic. The
> > ASID is allocated during vCPU reset (SEV allocates its own ASID), and
> > the ASID is always flushed on first use in case it was used by another
> > VM previously.
> >
> > On VMRUN, WARN if the ASID in the VMCB does not match the VM's ASID, and
> > update it accordingly. Also, flush the ASID on every VMRUN if the VM
> > failed to allocate a unique ASID. This would probably wreck performance
> > if it happens, but it should be an edge case as most AMD CPUs have over
> > 32k ASIDs.
> >
> > Suggested-by: Sean Christopherson <seanjc@xxxxxxxxxx>
> > Signed-off-by: Yosry Ahmed <yosry.ahmed@xxxxxxxxx>
> > ---
> [..]
> > @@ -3622,7 +3613,7 @@ static int svm_handle_exit(struct kvm_vcpu *vcpu, fastpath_t exit_fastpath)
> >
> >  static int pre_svm_run(struct kvm_vcpu *vcpu)
> >  {
> > -     struct svm_cpu_data *sd = per_cpu_ptr(&svm_data, vcpu->cpu);
> > +     struct kvm_svm *kvm_svm = to_kvm_svm(vcpu->kvm);
> >       struct vcpu_svm *svm = to_svm(vcpu);
> >
> >       /*
> > @@ -3639,9 +3630,15 @@ static int pre_svm_run(struct kvm_vcpu *vcpu)
> >       if (sev_guest(vcpu->kvm))
> >               return pre_sev_run(svm, vcpu->cpu);
> >
> > -     /* FIXME: handle wraparound of asid_generation */
> > -     if (svm->current_vmcb->asid_generation != sd->asid_generation)
> > -             new_asid(svm, sd);
> > +     /* Flush the ASID on every VMRUN if kvm_svm->asid allocation failed */
> > +     if (unlikely(!kvm_svm->asid))
> > +             svm_vmcb_set_flush_asid(svm->vmcb);
>
> This is wrong. I thought we can handle ASID allocation failures by just
> reusing ASID=0 and flushing it on every VMRUN, but using ASID=0 is
> illegal according to the APM. Also, in this case we also need to flush
> the ASID on every VM-exit, which I failed to do here.
>
> There are two ways to handle running out of ASIDs:
>
> (a) Failing to create the VM. This will impose a virtual limit on the
> number of VMs that can be run concurrently. The number of ASIDs was
> quite high on the CPUs I checked (2^15 IIRC), so it's probably not
> an issue, but I am not sure if this is considered breaking userspace.

I'm pretty sure AMD had only 6 bits of ASID through at least Family
12H. At some point, VMCB ASID bits must have become decoupled from
physical TLB tag bits. 15 TLB tag bits is inconceivable!

> (b) Designating a specific ASID value as the "fallback ASID". This value
> would be used by any VMs created after running out of ASIDs, and we
> flush it on every VMRUN, similar to what I am trying to do here for
> ASID=0.
>
> Any thoughts on which way we should take? (a) is simpler if we can get
> away with it and all AMD CPUs have a sufficiently large number of ASIDs.
>