linux-kernel - Re: [RFC PATCH 01/13] KVM: nSVM: Track the ASID per-VMCB

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: <7addde721e3f67bfa8ec5c9671f51d131f84bc6b.camel@redhat.com>
Date: Fri, 28 Feb 2025 20:23:48 -0500
From: Maxim Levitsky <mlevitsk@...hat.com>
To: Yosry Ahmed <yosry.ahmed@...ux.dev>, Sean Christopherson
 <seanjc@...gle.com>,  Paolo Bonzini <pbonzini@...hat.com>
Cc: kvm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH 01/13] KVM: nSVM: Track the ASID per-VMCB

On Wed, 2025-02-05 at 18:23 +0000, Yosry Ahmed wrote:
> The ASID is currently tracked per-vCPU, because the same ASID is used by
> L1 and L2. That ASID is flushed on every transition between L1 and L2.
> 
> Track the ASID separately for each VMCB (similar to the
> asid_generation), giving L2 a separate ASID. This is in preparation for
> doing fine-grained TLB flushes on nested transitions instead of
> unconditional full flushes.
> 
> The ASIDs are still not fully maintained (e.g. a remote flush will only
> flush the current ASID), so keep the TLB flush on every transition until
> this is sorted out.
> 
> L1's ASID will be flushed on KVM_REQ_TLB_FLUSH_GUEST if it is the
> active context, so remove the TODO in nested_svm_transition_tlb_flush()
> about it.
> 
> Signed-off-by: Yosry Ahmed <yosry.ahmed@...ux.dev>
> ---
>  arch/x86/kvm/svm/nested.c |  1 -
>  arch/x86/kvm/svm/sev.c    |  2 +-
>  arch/x86/kvm/svm/svm.c    | 12 +++++++-----
>  arch/x86/kvm/svm/svm.h    |  2 +-
>  4 files changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> index 04c375bf1ac2a..bbe4f3ac9f250 100644
> --- a/arch/x86/kvm/svm/nested.c
> +++ b/arch/x86/kvm/svm/nested.c
> @@ -495,7 +495,6 @@ static void nested_svm_transition_tlb_flush(struct kvm_vcpu *vcpu)
>  	 *  - Honor L1's request to flush an ASID on nested VMRUN
>  	 *  - Sync nested NPT MMU on VMRUN that flushes L2's ASID[*]
>  	 *  - Don't crush a pending TLB flush in vmcb02 on nested VMRUN
> -	 *  - Flush L1's ASID on KVM_REQ_TLB_FLUSH_GUEST
>  	 *
>  	 * [*] Unlike nested EPT, SVM's ASID management can invalidate nested
>  	 *     NPT guest-physical mappings on VMRUN.
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 799f8494b599c..b0adfd0537d00 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -3468,7 +3468,7 @@ void pre_sev_run(struct vcpu_svm *svm, int cpu)
>  	unsigned int asid = sev_get_asid(svm->vcpu.kvm);
>  
>  	/* Assign the asid allocated with this SEV guest */
> -	svm->asid = asid;
> +	svm->current_vmcb->asid = asid;
>  
>  	/*
>  	 * Flush guest TLB:
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 7640a84e554a6..08340ae57777b 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -1335,8 +1335,10 @@ static void init_vmcb(struct kvm_vcpu *vcpu)
>  		save->g_pat = vcpu->arch.pat;
>  		save->cr3 = 0;
>  	}
> -	svm->current_vmcb->asid_generation = 0;
> -	svm->asid = 0;
> +	svm->vmcb01.asid_generation = 0;
> +	svm->vmcb01.asid = 0;
> +	svm->nested.vmcb02.asid_generation = 0;
> +	svm->nested.vmcb02.asid = 0;
>  
>  	svm->nested.vmcb12_gpa = INVALID_GPA;
>  	svm->nested.last_vmcb12_gpa = INVALID_GPA;
> @@ -1988,7 +1990,7 @@ static void new_asid(struct vcpu_svm *svm, struct svm_cpu_data *sd)
>  	}
>  
>  	svm->current_vmcb->asid_generation = sd->asid_generation;
> -	svm->asid = sd->next_asid++;
> +	svm->current_vmcb->asid = sd->next_asid++;
>  }
>  
>  static void svm_set_dr6(struct vcpu_svm *svm, unsigned long value)
> @@ -4235,8 +4237,8 @@ static __no_kcsan fastpath_t svm_vcpu_run(struct kvm_vcpu *vcpu,
>  
>  	sync_lapic_to_cr8(vcpu);
>  
> -	if (unlikely(svm->asid != svm->vmcb->control.asid)) {
> -		svm->vmcb->control.asid = svm->asid;
> +	if (unlikely(svm->current_vmcb->asid != svm->vmcb->control.asid)) {
> +		svm->vmcb->control.asid = svm->current_vmcb->asid;
>  		vmcb_mark_dirty(svm->vmcb, VMCB_ASID);
>  	}
>  	svm->vmcb->save.cr2 = vcpu->arch.cr2;
> diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
> index 9d7cdb8fbf872..ebbb0b1a64676 100644
> --- a/arch/x86/kvm/svm/svm.h
> +++ b/arch/x86/kvm/svm/svm.h
> @@ -133,6 +133,7 @@ struct kvm_vmcb_info {
>  	unsigned long pa;
>  	int cpu;
>  	uint64_t asid_generation;
> +	u32 asid;
>  };
>  
>  struct vmcb_save_area_cached {
> @@ -247,7 +248,6 @@ struct vcpu_svm {
>  	struct vmcb *vmcb;
>  	struct kvm_vmcb_info vmcb01;
>  	struct kvm_vmcb_info *current_vmcb;
> -	u32 asid;
>  	u32 sysenter_esp_hi;
>  	u32 sysenter_eip_hi;
>  	uint64_t tsc_aux;

Hi,


I think it should be possible to eliminate separate ASID field (current_vmcb->asid/svm->asid)
completely and instead just use the value stored in the vmcb.

When there is a need to update it, KVM can also set the corresponding dirty bit
as done in svm_vcpu_run (new_asid also already does this when the asid generation increases)

Also KVM already sets the tlb_ctl directly in the vmcb.

What do you think?

Best regards,
	Maxim Levitsky