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@xxxxxxxxxx> > > Thanks! > > > > > Best regards, > > Maxim Levitsky
