mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
From: Sean Christopherson <seanjc@google.com>
To: Yosry Ahmed <yosry@kernel.org>
Cc: Paolo Bonzini <pbonzini@redhat.com>,
	kvm@vger.kernel.org, linux-kernel@vger.kernel.org,
	 stable@vger.kernel.org, Stefan Teodorescu <fane@google.com>
Subject: Re: [PATCH] KVM: SVM: Trigger new ASID allocation in both VMCBs on pCPU switch
Date: Thu, 3 Sep 2026 14:53:29 -0700	[thread overview]
Message-ID: <apnqonqMBZ2GeGEq@google.com> (raw)
In-Reply-To: <20260903175856.4065099-1-yosry@kernel.org>

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@linux.dev/
> [2]https://lore.kernel.org/kvm/20260728003557.1136583-1-yosry@kernel.org/
> 

Fixes: 193015adf40d ("KVM: nSVM: Track the ASID generation of the vmcb vmrun through the vmcb")

> Cc: stable@vger.kernel.org
> Reported-by: Stefan Teodorescu <fane@google.com>
> Signed-off-by: Yosry Ahmed <yosry@kernel.org>
> ---
> 
> 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.

 - 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.
 - 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.
 - vCPUx does VMRUN on vmcb02 with asid=2. 
 - 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.

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

  reply	other threads:[~2026-09-03 21:53 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-09-03 17:58 Yosry Ahmed
2026-09-03 21:53 ` Sean Christopherson [this message]
2026-09-03 22:00   ` Yosry Ahmed
2026-09-03 22:18     ` Sean Christopherson
2026-09-03 22:20       ` Yosry Ahmed
2026-09-03 22:26         ` Sean Christopherson

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=apnqonqMBZ2GeGEq@google.com \
    --to=seanjc@google.com \
    --cc=fane@google.com \
    --cc=kvm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=pbonzini@redhat.com \
    --cc=stable@vger.kernel.org \
    --cc=yosry@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®