On Wed, Oct 21, 2020 at 02:39:20PM +0200, Vitaly Kuznetsov wrote: > Sean Christopherson <sean.j.christopherson@xxxxxxxxx> writes: > > > Drop the dedicated 'ept_pointers_match' field in favor of stuffing > > 'hv_tlb_eptp' with INVALID_PAGE to mark it as invalid, i.e. to denote > > that there is at least one EPTP mismatch. Use a local variable to > > track whether or not a mismatch is detected so that hv_tlb_eptp can be > > used to skip redundant flushes. > > > > No functional change intended. > > > > Signed-off-by: Sean Christopherson <sean.j.christopherson@xxxxxxxxx> > > --- > > arch/x86/kvm/vmx/vmx.c | 16 ++++++++-------- > > arch/x86/kvm/vmx/vmx.h | 7 ------- > > 2 files changed, 8 insertions(+), 15 deletions(-) > > > > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c > > index 52cb9eec1db3..4dfde8b64750 100644 > > --- a/arch/x86/kvm/vmx/vmx.c > > +++ b/arch/x86/kvm/vmx/vmx.c > > @@ -498,13 +498,13 @@ static int hv_remote_flush_tlb_with_range(struct kvm *kvm, > > struct kvm_vmx *kvm_vmx = to_kvm_vmx(kvm); > > struct kvm_vcpu *vcpu; > > int ret = 0, i; > > + bool mismatch; > > u64 tmp_eptp; > > > > spin_lock(&kvm_vmx->ept_pointer_lock); > > > > - if (kvm_vmx->ept_pointers_match != EPT_POINTERS_MATCH) { > > - kvm_vmx->ept_pointers_match = EPT_POINTERS_MATCH; > > - kvm_vmx->hv_tlb_eptp = INVALID_PAGE; > > + if (!VALID_PAGE(kvm_vmx->hv_tlb_eptp)) { > > + mismatch = false; > > > > kvm_for_each_vcpu(i, vcpu, kvm) { > > tmp_eptp = to_vmx(vcpu)->ept_pointer; > > @@ -515,12 +515,13 @@ static int hv_remote_flush_tlb_with_range(struct kvm *kvm, > > if (!VALID_PAGE(kvm_vmx->hv_tlb_eptp)) > > kvm_vmx->hv_tlb_eptp = tmp_eptp; > > else > > - kvm_vmx->ept_pointers_match > > - = EPT_POINTERS_MISMATCH; > > + mismatch = true; > > > > ret |= hv_remote_flush_eptp(tmp_eptp, range); > > } > > - } else if (VALID_PAGE(kvm_vmx->hv_tlb_eptp)) { > > + if (mismatch) > > + kvm_vmx->hv_tlb_eptp = INVALID_PAGE; > > + } else { > > ret = hv_remote_flush_eptp(kvm_vmx->hv_tlb_eptp, range); > > } > > Personally, I find double negations like 'mismatch = false' hard to read > :-). Paolo also dislikes double negatives (I just wasted a minute of my life trying to work a double negative into that sentence). > What if we write this all like > > if (!VALID_PAGE(kvm_vmx->hv_tlb_eptp)) { > kvm_vmx->hv_tlb_eptp = to_vmx(vcpu0)->ept_pointer; > kvm_for_each_vcpu() { > tmp_eptp = to_vmx(vcpu)->ept_pointer; > if (!VALID_PAGE(tmp_eptp) || tmp_eptp != kvm_vmx->hv_tlb_eptp) > kvm_vmx->hv_tlb_eptp = INVALID_PAGE; > if (VALID_PAGE(tmp_eptp)) > ret |= hv_remote_flush_eptp(tmp_eptp, range); > } > } else { > ret = hv_remote_flush_eptp(kvm_vmx->hv_tlb_eptp, range); > } > > (not tested and I've probably missed something) It works, but doesn't optimize the case where one or more vCPUs has an invalid EPTP. E.g. if vcpuN->ept_pointer is INVALID_PAGE, vcpuN+1..vcpuZ will flush, even if they all match. Now, whether or not it's worth optimizing that case... This is also why I named it "mismatch", i.e. it tracks whether or not there was a mismatch between valid EPTPs, not that all EPTPs matched. What about replacing "mismatch" with a counter that tracks the number of unique, valid PGDs that are encountered? if (!VALID_PAGE(kvm_vmx->hv_tlb_pgd)) { unique_valid_pgd_cnt = 0; kvm_for_each_vcpu(i, vcpu, kvm) { tmp_pgd = to_vmx(vcpu)->hv_tlb_pgd; if (!VALID_PAGE(tmp_pgd) || tmp_pgd == kvm_vmx->hv_tlb_pgd) continue; unique_valid_pgd_cnt++; if (!VALID_PAGE(kvm_vmx->hv_tlb_pgd)) kvm_vmx->hv_tlb_pgd = tmp_pgd; if (!ret) ret = hv_remote_flush_pgd(tmp_pgd, range); if (ret && unique_valid_pgd_cnt > 1) break; } if (unique_valid_pgd_cnt > 1) kvm_vmx->hv_tlb_pgd = INVALID_PAGE; } else { ret = hv_remote_flush_pgd(kvm_vmx->hv_tlb_pgd, range); } Alternatively, the pgd_cnt adjustment could be used to update hv_tlb_pgd, e.g. if (++unique_valid_pgd_cnt == 1) kvm_vmx->hv_tlb_pgd = tmp_pgd; I think I like this last one the most. It self-documents what we're tracking as well as the relationship between the number of valid PGDs and hv_tlb_pgd. I'll also add a few comments to explain how kvm_vmx->hv_tlb_pgd is used. Thoughts? > > @@ -3042,8 +3043,7 @@ static void vmx_load_mmu_pgd(struct kvm_vcpu *vcpu, unsigned long pgd, > > if (kvm_x86_ops.tlb_remote_flush) { > > spin_lock(&to_kvm_vmx(kvm)->ept_pointer_lock); > > to_vmx(vcpu)->ept_pointer = eptp; > > - to_kvm_vmx(kvm)->ept_pointers_match > > - = EPT_POINTERS_CHECK; > > + to_kvm_vmx(kvm)->hv_tlb_eptp = INVALID_PAGE; > > spin_unlock(&to_kvm_vmx(kvm)->ept_pointer_lock); > > } > > > > diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h > > index 3d557a065c01..e8d7d07b2020 100644 > > --- a/arch/x86/kvm/vmx/vmx.h > > +++ b/arch/x86/kvm/vmx/vmx.h > > @@ -288,12 +288,6 @@ struct vcpu_vmx { > > } shadow_msr_intercept; > > }; > > > > -enum ept_pointers_status { > > - EPT_POINTERS_CHECK = 0, > > - EPT_POINTERS_MATCH = 1, > > - EPT_POINTERS_MISMATCH = 2 > > -}; > > - > > struct kvm_vmx { > > struct kvm kvm; > > > > @@ -302,7 +296,6 @@ struct kvm_vmx { > > gpa_t ept_identity_map_addr; > > > > hpa_t hv_tlb_eptp; > > - enum ept_pointers_status ept_pointers_match; > > spinlock_t ept_pointer_lock; > > }; > > -- > Vitaly >