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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Z9iQEV9SQYjtLT8V@google.com>
Date: Mon, 17 Mar 2025 21:11:45 +0000
From: Yosry Ahmed <yosry.ahmed@...ux.dev>
To: Sean Christopherson <seanjc@...gle.com>
Cc: Paolo Bonzini <pbonzini@...hat.com>,
	Maxim Levitsky <mlevitsk@...hat.com>, x86@...nel.org,
	kvm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 6/7] KVM: SVM: Use a single ASID per VM

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@...gle.com>
> Signed-off-by: Yosry Ahmed <yosry.ahmed@...ux.dev>
> ---
[..]
> @@ -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.

(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.