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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <6345e31c7973a2ec32b11ed54cede142a901044e.camel@redhat.com>
Date: Tue, 04 Mar 2025 22:01:25 -0500
From: Maxim Levitsky <mlevitsk@...hat.com>
To: Yosry Ahmed <yosry.ahmed@...ux.dev>
Cc: Sean Christopherson <seanjc@...gle.com>, Paolo Bonzini
 <pbonzini@...hat.com>,  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 Mon, 2025-03-03 at 22:14 +0000, Yosry Ahmed wrote:
> On Fri, Feb 28, 2025 at 09:17:52PM -0500, Maxim Levitsky wrote:
> > 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.
> 
> How so?
> 
> nested_svm_exit_tlb_flush() makes a KVM_REQ_TLB_FLUSH_GUEST request.
> svm_flush_tlb_asid() is called when servicing KVM_REQ_TLB_* requests.

I am probably missing something but:

Suppose KVM_REQ_TLB_FLUSH is raised and then processed while ordinary L1 entry is happening,
but nested state is allocated.

(KVM_REQ_TLB_FLUSH can be raised anytime MMU wants a 'big hammer flush everything')

In this case svm_flush_tlb_all will call svm_flush_tlb_asid on both vmcbs (see patch 8),
and that will set TLB_CONTROL_FLUSH_ASID in both vmcbs.
In particular it will be set in vmcb02.

Later, maybe even hours later in theory, L1 issues VMRUN, we reach nested_vmcb02_prepare_control,
and crush the value (TLB_CONTROL_FLUSH_ASID) set in vmcb02.

I think that this is what the removed comment referred to.


Best regards,
	Maxim Levitsky

> 
> So svm_flush_tlb_asid() does not make a request in the sense of
> KVM_REQ_*, it sets TLB_CONTROL or invalidates the ASID, which is can
> more-or-less be described as "requesting" a TLB flush on VM-enter, but
> is not the same thing as KVM_REQ_TLB_FLUSH.
> 
> So I am not sure there are any requests being crushed here.
> 
> > But the patch itself does look OK to me, although I might be mistaken.
> > 
> > Reviewed-by: Maxim Levitsky <mlevitsk@...hat.com>
> 
> Thanks!
> 
> > 
> > Best regards,
> > 	Maxim Levitsky