linux-kernel - Re: [RFC PATCH 11/13] KVM: nSVM: Do not reset TLB

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 for Android: free password hash cracker in your pocket

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

Message-ID: <a70458e20e98da9cd6dd1d272cc16b71bfdd4842.camel@redhat.com>
Date: Fri, 28 Feb 2025 21:17:52 -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 11/13] KVM: nSVM: Do not reset TLB_CONTROL in VMCB02
 on nested entry

On Wed, 2025-02-05 at 18:24 +0000, Yosry Ahmed wrote:
> TLB_CONTROL is reset to TLB_CONTROL_DO_NOTHING on nested transitions to
> L2. This is unnecessary because it should always be
> TLB_CONTROL_DO_NOTHING at this point.
> 
> The flow for setting TLB_CONTROL is as follows:
> 1. In vcpu_enter_guest(), servicing a TLB flush request may set it to
> TLB_CONTROL_FLUSH_ASID in svm_flush_tlb_asid().
> 2. In svm_vcpu_run() -> pre_svm_run(), it may get upgraded to
> TLB_CONTROL_FLUSH_ALL_ASID when assigning a new ASID.
> 3. In svm_cpu_run(), it gets reset to TLB_CONTROL_DO_NOTHING after the
> guest is run.
> 
> Hence, TLB_CONTROL is reset after each run and there is no need to do it
> again on every nested transition to L2.
> 
> There is a TODO in nested_svm_transition_tlb_flush() about this reset
> crushing pending TLB flushes. Remove it, as the reset is not really
> crushing anything as explained above.

I am not sure that we don't crush a pending tlb request: 

svm_flush_tlb_asid can also be called by KVM_REQ_TLB_FLUSH
and set the flush request in both vmcbs, thus later the nested_svm_exit_tlb_flush
can crush this request.

But the patch itself does look OK to me, although I might be mistaken.

Reviewed-by: Maxim Levitsky <mlevitsk@...hat.com>


Best regards,
	Maxim Levitsky


> 
> Signed-off-by: Yosry Ahmed <yosry.ahmed@...ux.dev>
> ---
>  arch/x86/kvm/svm/nested.c | 12 ++----------
>  1 file changed, 2 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> index 12bb391884299..8e40ff21f7353 100644
> --- a/arch/x86/kvm/svm/nested.c
> +++ b/arch/x86/kvm/svm/nested.c
> @@ -512,12 +512,7 @@ static void nested_svm_entry_tlb_flush(struct kvm_vcpu *vcpu)
>  		svm->nested.last_asid = svm->nested.ctl.asid;
>  		kvm_make_request(KVM_REQ_TLB_FLUSH_GUEST, vcpu);
>  	}
> -	/*
> -	 * TODO: optimize unconditional TLB flush/MMU sync.  A partial list of
> -	 * things to fix before this can be conditional:
> -	 *
> -	 *  - Don't crush a pending TLB flush in vmcb02 on nested VMRUN
> -	 */
> +	/* TODO: optimize unconditional TLB flush/MMU sync */
>  	kvm_make_request(KVM_REQ_MMU_SYNC, vcpu);
>  	kvm_make_request(KVM_REQ_TLB_FLUSH_CURRENT, vcpu);
>  }
> @@ -536,7 +531,7 @@ static void nested_svm_exit_tlb_flush(struct kvm_vcpu *vcpu)
>  	if (svm->nested.ctl.tlb_ctl == TLB_CONTROL_FLUSH_ALL_ASID)
>  		kvm_make_request(KVM_REQ_TLB_FLUSH_GUEST, vcpu);
>  
> -	/* See nested_svm_entry_tlb_flush() */
> +	/* TODO: optimize unconditional TLB flush/MMU sync */
>  	kvm_make_request(KVM_REQ_MMU_SYNC, vcpu);
>  	kvm_make_request(KVM_REQ_TLB_FLUSH_CURRENT, vcpu);
>  }
> @@ -717,9 +712,6 @@ static void nested_vmcb02_prepare_control(struct vcpu_svm *svm,
>  
>  	/* Done at vmrun: asid.  */
>  
> -	/* Also overwritten later if necessary.  */
> -	svm_clear_tlb_ctl_flush(vmcb02);
> -
>  	/* nested_cr3.  */
>  	if (nested_npt_enabled(svm))
>  		nested_svm_init_mmu_context(vcpu);