On Wed, Jan 15, 2025 at 9:27 PM Jim Mattson <jmattson@xxxxxxxxxx> wrote: > > On Wed, Jan 15, 2025 at 7:50 PM Yosry Ahmed <yosryahmed@xxxxxxxxxx> wrote: > > > > nested_vmx_transition_tlb_flush() uses KVM_REQ_TLB_FLUSH_CURRENT to > > flush the TLB if VPID is enabled for both L1 and L2, but they still > > share the TLB tag. This happens if EPT is disabled and KVM fails to > > allocate a VPID for L2, so both the EPTP and VPID are shared between L1 > > and L2. > > Nit: Combined and guest-physical TLB tags are based on EPTRTA (the new > acronym for EP4TA), not EPTP. But, in any case, with EPT disabled, > there are no combined or guest-physical mappings. There are only > linear mappings. Interestingly, I did initially write EPTRTA, but I changed it to EPTP because that is the terminology used in nested_has_guest_tlb_tag(). Anyway, I definitely don't mind changing it to EPTRTA. > > > Interestingly, nested_vmx_transition_tlb_flush() uses > > KVM_REQ_TLB_FLUSH_GUEST to flush the TLB for all other cases where a > > flush is required. > > > > Taking a close look at vmx_flush_tlb_guest() and > > vmx_flush_tlb_current(), the main differences are: > > (a) vmx_flush_tlb_current() is a noop if the KVM MMU is invalid. > > (b) vmx_flush_tlb_current() uses INVEPT if EPT is enabled (instead of > > INVVPID) to flush the guest-physical mappings as well as combined > > mappings. > > > > The check in (a) is seemingly an optimization, and there should not be > > any TLB entries for L1 anyway if the KVM MMU is invalid. Not having this > > check in vmx_flush_tlb_guest() is not a fundamental difference, and it > > can be added there separately if needed. > > > > The difference in (b) is irrelevant in this case, because EPT being > > enabled for L1 means that its TLB tags are tagged with EPTP and cannot > > be used by L2 (regardless of whether or not EPT is enabled for L2). > > The difference is also irrelevant because, as you concluded in the > first paragraph, EPT is disabled in the final block of > nested_vmx_transition_tlb_flush(). I was trying to explain that even if EPT is enabled, sharing guest-physical translations between L1 and L2 should never be possible (and hence we should never worry about flushing these translations in nested_vmx_transition_tlb_flush()). Now that I read it again it is not as clear as I had hoped. > > > Use KVM_REQ_TLB_FLUSH_GUEST in this case in > > nested_vmx_transition_tlb_flush() for consistency. This arguably makes > > more sense conceptually too -- L1 and L2 cannot share the TLB tag for > > guest-physical translations, so only flushing linear and combined > > translations (i.e. guest-generated translations) is needed. > > And, as I mentioned above, with EPT disabled, there are no combined or > guest-physical mappings. > > > Signed-off-by: Yosry Ahmed <yosryahmed@xxxxxxxxxx> > > I think the reasoning in the commit message can be cleared up a bit, but... Agreed :) I am sure Sean will also want changes in the commit message anyway. > > Reviewed-by: Jim Mattson <mattson@xxxxxxxxxx> Thanks for the quick review!
