lkml.org 
[lkml]   [2020]   [Oct]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH v2 05/10] KVM: VMX: Invalidate hv_tlb_eptp to denote an EPTP mismatch
    On Wed, Oct 21, 2020 at 02:39:20PM +0200, Vitaly Kuznetsov wrote:
    > Sean Christopherson <sean.j.christopherson@intel.com> 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@intel.com>
    > > ---
    > > 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
    >

    \
     
     \ /
      Last update: 2020-10-21 18:38    [W:2.592 / U:0.332 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site