From: Ben Gardon <bgardon@google.com>
To: Sean Christopherson <sean.j.christopherson@intel.com>
Cc: LKML <linux-kernel@vger.kernel.org>, kvm <kvm@vger.kernel.org>,
Cannon Matthews <cannonmatthews@google.com>,
Paolo Bonzini <pbonzini@redhat.com>, Peter Xu <peterx@redhat.com>,
Peter Shier <pshier@google.com>,
Peter Feiner <pfeiner@google.com>,
Junaid Shahid <junaids@google.com>,
Jim Mattson <jmattson@google.com>,
Yulei Zhang <yulei.kernel@gmail.com>,
Wanpeng Li <kernellwp@gmail.com>,
Vitaly Kuznetsov <vkuznets@redhat.com>,
Xiao Guangrong <xiaoguangrong.eric@gmail.com>
Subject: Re: [PATCH 10/22] kvm: mmu: Add TDP MMU PF handler
Date: Tue, 6 Oct 2020 15:33:21 -0700 [thread overview]
Message-ID: <CANgfPd_A6Bbv+ehRvMVi_NK2C_Jb=bBmXJR89fj=JSFSga0avg@mail.gmail.com> (raw)
In-Reply-To: <20200930163740.GD32672@linux.intel.com>
On Wed, Sep 30, 2020 at 9:37 AM Sean Christopherson
<sean.j.christopherson@intel.com> wrote:
>
> On Fri, Sep 25, 2020 at 02:22:50PM -0700, Ben Gardon wrote:
> > @@ -4113,8 +4088,9 @@ static int direct_page_fault(struct kvm_vcpu *vcpu, gpa_t gpa, u32 error_code,
> > if (page_fault_handle_page_track(vcpu, error_code, gfn))
> > return RET_PF_EMULATE;
> >
> > - if (fast_page_fault(vcpu, gpa, error_code))
> > - return RET_PF_RETRY;
> > + if (!is_tdp_mmu_root(vcpu->kvm, vcpu->arch.mmu->root_hpa))
> > + if (fast_page_fault(vcpu, gpa, error_code))
> > + return RET_PF_RETRY;
>
> It'll probably be easier to handle is_tdp_mmu() in fast_page_fault().
I'd prefer to keep this check here because then in the fast page fault
path, we can just handle the case where we do have a tdp mmu root with
the tdp mmu fast pf handler and it'll mirror the split below with
__direct_map and the TDP MMU PF handler.
>
> >
> > r = mmu_topup_memory_caches(vcpu, false);
> > if (r)
> > @@ -4139,8 +4115,14 @@ static int direct_page_fault(struct kvm_vcpu *vcpu, gpa_t gpa, u32 error_code,
> > r = make_mmu_pages_available(vcpu);
> > if (r)
> > goto out_unlock;
> > - r = __direct_map(vcpu, gpa, write, map_writable, max_level, pfn,
> > - prefault, is_tdp && lpage_disallowed);
> > +
> > + if (is_tdp_mmu_root(vcpu->kvm, vcpu->arch.mmu->root_hpa))
> > + r = kvm_tdp_mmu_page_fault(vcpu, write, map_writable, max_level,
> > + gpa, pfn, prefault,
> > + is_tdp && lpage_disallowed);
> > + else
> > + r = __direct_map(vcpu, gpa, write, map_writable, max_level, pfn,
> > + prefault, is_tdp && lpage_disallowed);
> >
> > out_unlock:
> > spin_unlock(&vcpu->kvm->mmu_lock);
>
> ...
>
> > +/*
> > + * Handle a TDP page fault (NPT/EPT violation/misconfiguration) by installing
> > + * page tables and SPTEs to translate the faulting guest physical address.
> > + */
> > +int kvm_tdp_mmu_page_fault(struct kvm_vcpu *vcpu, int write, int map_writable,
> > + int max_level, gpa_t gpa, kvm_pfn_t pfn,
> > + bool prefault, bool account_disallowed_nx_lpage)
> > +{
> > + struct tdp_iter iter;
> > + struct kvm_mmu_memory_cache *pf_pt_cache =
> > + &vcpu->arch.mmu_shadow_page_cache;
> > + u64 *child_pt;
> > + u64 new_spte;
> > + int ret;
> > + int as_id = kvm_arch_vcpu_memslots_id(vcpu);
> > + gfn_t gfn = gpa >> PAGE_SHIFT;
> > + int level;
> > +
> > + if (WARN_ON(!VALID_PAGE(vcpu->arch.mmu->root_hpa)))
> > + return RET_PF_RETRY;
>
> I feel like we should kill off these silly WARNs in the existing code instead
> of adding more. If they actually fired, I'm pretty sure that they would
> continue firing and spamming the kernel log until the VM is killed as I don't
> see how restarting the guest will magically fix anything.
>
> > +
> > + if (WARN_ON(!is_tdp_mmu_root(vcpu->kvm, vcpu->arch.mmu->root_hpa)))
> > + return RET_PF_RETRY;
>
> This seems especially gratuitous, this has exactly one caller that explicitly
> checks is_tdp_mmu_root(). Again, if this fires it will spam the kernel log
> into submission.
>
> > +
> > + level = kvm_mmu_hugepage_adjust(vcpu, gfn, max_level, &pfn);
> > +
> > + for_each_tdp_pte_vcpu(iter, vcpu, gfn, gfn + 1) {
> > + disallowed_hugepage_adjust(iter.old_spte, gfn, iter.level,
> > + &pfn, &level);
> > +
> > + if (iter.level == level)
> > + break;
> > +
> > + /*
> > + * If there is an SPTE mapping a large page at a higher level
> > + * than the target, that SPTE must be cleared and replaced
> > + * with a non-leaf SPTE.
> > + */
> > + if (is_shadow_present_pte(iter.old_spte) &&
> > + is_large_pte(iter.old_spte)) {
> > + *iter.sptep = 0;
> > + handle_changed_spte(vcpu->kvm, as_id, iter.gfn,
> > + iter.old_spte, 0, iter.level);
> > + kvm_flush_remote_tlbs_with_address(vcpu->kvm, iter.gfn,
> > + KVM_PAGES_PER_HPAGE(iter.level));
> > +
> > + /*
> > + * The iter must explicitly re-read the spte here
> > + * because the new is needed before the next iteration
> > + * of the loop.
> > + */
>
> I think it'd be better to explicitly, and simply, call out that iter.old_spte
> is consumed below. It's subtle enough to warrant a comment, but the comment
> didn't actually help me. Maybe something like:
>
> /*
> * Refresh iter.old_spte, it will trigger the !present
> * path below.
> */
>
That's a good point and calling out the relation to the present check
below is much clearer.
> > + iter.old_spte = READ_ONCE(*iter.sptep);
> > + }
> > +
> > + if (!is_shadow_present_pte(iter.old_spte)) {
> > + child_pt = kvm_mmu_memory_cache_alloc(pf_pt_cache);
> > + clear_page(child_pt);
> > + new_spte = make_nonleaf_spte(child_pt,
> > + !shadow_accessed_mask);
> > +
> > + *iter.sptep = new_spte;
> > + handle_changed_spte(vcpu->kvm, as_id, iter.gfn,
> > + iter.old_spte, new_spte,
> > + iter.level);
> > + }
> > + }
> > +
> > + if (WARN_ON(iter.level != level))
> > + return RET_PF_RETRY;
>
> This also seems unnecessary. Or maybe these are all good candiates for
> KVM_BUG_ON...
>
I've replaced all these warnings with KVM_BUG_ONs.
> > +
> > + ret = page_fault_handle_target_level(vcpu, write, map_writable,
> > + as_id, &iter, pfn, prefault);
> > +
> > + /* If emulating, flush this vcpu's TLB. */
>
> Why? It's obvious _what_ the code is doing, the comment should explain _why_.
>
> > + if (ret == RET_PF_EMULATE)
> > + kvm_make_request(KVM_REQ_TLB_FLUSH, vcpu);
> > +
> > + return ret;
> > +}
> > diff --git a/arch/x86/kvm/mmu/tdp_mmu.h b/arch/x86/kvm/mmu/tdp_mmu.h
> > index cb86f9fe69017..abf23dc0ab7ad 100644
> > --- a/arch/x86/kvm/mmu/tdp_mmu.h
> > +++ b/arch/x86/kvm/mmu/tdp_mmu.h
> > @@ -14,4 +14,8 @@ void kvm_tdp_mmu_put_root_hpa(struct kvm *kvm, hpa_t root_hpa);
> >
> > bool kvm_tdp_mmu_zap_gfn_range(struct kvm *kvm, gfn_t start, gfn_t end);
> > void kvm_tdp_mmu_zap_all(struct kvm *kvm);
> > +
> > +int kvm_tdp_mmu_page_fault(struct kvm_vcpu *vcpu, int write, int map_writable,
> > + int level, gpa_t gpa, kvm_pfn_t pfn, bool prefault,
> > + bool lpage_disallowed);
> > #endif /* __KVM_X86_MMU_TDP_MMU_H */
> > --
> > 2.28.0.709.gb0816b6eb0-goog
> >
next prev parent reply other threads:[~2020-10-06 22:33 UTC|newest]
Thread overview: 105+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-09-25 21:22 [PATCH 00/22] Introduce the TDP MMU Ben Gardon
2020-09-25 21:22 ` [PATCH 01/22] kvm: mmu: Separate making SPTEs from set_spte Ben Gardon
2020-09-30 4:55 ` Sean Christopherson
2020-09-30 23:03 ` Ben Gardon
2020-09-25 21:22 ` [PATCH 02/22] kvm: mmu: Introduce tdp_iter Ben Gardon
2020-09-26 0:04 ` Paolo Bonzini
2020-09-30 5:06 ` Sean Christopherson
2020-09-26 0:54 ` Paolo Bonzini
2020-09-30 5:08 ` Sean Christopherson
2020-09-30 5:24 ` Sean Christopherson
2020-09-30 6:24 ` Paolo Bonzini
2020-09-30 23:20 ` Eric van Tassell
2020-09-30 23:34 ` Paolo Bonzini
2020-10-01 0:07 ` Sean Christopherson
2020-09-25 21:22 ` [PATCH 03/22] kvm: mmu: Init / Uninit the TDP MMU Ben Gardon
2020-09-26 0:06 ` Paolo Bonzini
2020-09-30 5:34 ` Sean Christopherson
2020-09-30 18:36 ` Ben Gardon
2020-09-30 16:57 ` Sean Christopherson
2020-09-30 17:39 ` Paolo Bonzini
2020-09-30 18:42 ` Ben Gardon
2020-09-25 21:22 ` [PATCH 04/22] kvm: mmu: Allocate and free TDP MMU roots Ben Gardon
2020-09-30 6:06 ` Sean Christopherson
2020-09-30 6:26 ` Paolo Bonzini
2020-09-30 15:38 ` Sean Christopherson
2020-10-12 22:59 ` Ben Gardon
2020-10-12 23:59 ` Sean Christopherson
2020-09-25 21:22 ` [PATCH 05/22] kvm: mmu: Add functions to handle changed TDP SPTEs Ben Gardon
2020-09-26 0:39 ` Paolo Bonzini
2020-09-28 17:23 ` Paolo Bonzini
2020-09-25 21:22 ` [PATCH 06/22] kvm: mmu: Make address space ID a property of memslots Ben Gardon
2020-09-30 6:10 ` Sean Christopherson
2020-09-30 23:11 ` Ben Gardon
2020-09-25 21:22 ` [PATCH 07/22] kvm: mmu: Support zapping SPTEs in the TDP MMU Ben Gardon
2020-09-26 0:14 ` Paolo Bonzini
2020-09-30 6:15 ` Sean Christopherson
2020-09-30 6:28 ` Paolo Bonzini
2020-09-25 21:22 ` [PATCH 08/22] kvm: mmu: Separate making non-leaf sptes from link_shadow_page Ben Gardon
2020-09-25 21:22 ` [PATCH 09/22] kvm: mmu: Remove disallowed_hugepage_adjust shadow_walk_iterator arg Ben Gardon
2020-09-30 16:19 ` Sean Christopherson
2020-09-25 21:22 ` [PATCH 10/22] kvm: mmu: Add TDP MMU PF handler Ben Gardon
2020-09-26 0:24 ` Paolo Bonzini
2020-09-30 16:37 ` Sean Christopherson
2020-09-30 16:55 ` Paolo Bonzini
2020-09-30 17:37 ` Paolo Bonzini
2020-10-06 22:35 ` Ben Gardon
2020-10-06 22:33 ` Ben Gardon [this message]
2020-10-07 20:55 ` Sean Christopherson
2020-09-25 21:22 ` [PATCH 11/22] kvm: mmu: Factor out allocating a new tdp_mmu_page Ben Gardon
2020-09-26 0:22 ` Paolo Bonzini
2020-09-30 18:53 ` Ben Gardon
2020-09-25 21:22 ` [PATCH 12/22] kvm: mmu: Allocate struct kvm_mmu_pages for all pages in TDP MMU Ben Gardon
2020-09-25 21:22 ` [PATCH 13/22] kvm: mmu: Support invalidate range MMU notifier for " Ben Gardon
2020-09-30 17:03 ` Sean Christopherson
2020-09-30 23:15 ` Ben Gardon
2020-09-30 23:24 ` Sean Christopherson
2020-09-30 23:27 ` Ben Gardon
2020-09-25 21:22 ` [PATCH 14/22] kvm: mmu: Add access tracking for tdp_mmu Ben Gardon
2020-09-26 0:32 ` Paolo Bonzini
2020-09-30 17:48 ` Sean Christopherson
2020-10-06 23:38 ` Ben Gardon
2020-09-25 21:22 ` [PATCH 15/22] kvm: mmu: Support changed pte notifier in tdp MMU Ben Gardon
2020-09-26 0:33 ` Paolo Bonzini
2020-09-28 15:11 ` Paolo Bonzini
2020-10-07 16:53 ` Ben Gardon
2020-10-07 17:18 ` Paolo Bonzini
2020-10-07 17:30 ` Ben Gardon
2020-10-07 17:54 ` Paolo Bonzini
2020-09-25 21:22 ` [PATCH 16/22] kvm: mmu: Add dirty logging handler for changed sptes Ben Gardon
2020-09-26 0:45 ` Paolo Bonzini
2020-09-25 21:22 ` [PATCH 17/22] kvm: mmu: Support dirty logging for the TDP MMU Ben Gardon
2020-09-26 1:04 ` Paolo Bonzini
2020-10-08 18:27 ` Ben Gardon
2020-09-29 15:07 ` Paolo Bonzini
2020-09-30 18:04 ` Sean Christopherson
2020-09-30 18:08 ` Paolo Bonzini
2020-09-25 21:22 ` [PATCH 18/22] kvm: mmu: Support disabling dirty logging for the tdp MMU Ben Gardon
2020-09-26 1:09 ` Paolo Bonzini
2020-10-07 16:30 ` Ben Gardon
2020-10-07 17:21 ` Paolo Bonzini
2020-10-07 17:28 ` Ben Gardon
2020-10-07 17:53 ` Paolo Bonzini
2020-09-25 21:22 ` [PATCH 19/22] kvm: mmu: Support write protection for nesting in " Ben Gardon
2020-09-30 18:06 ` Sean Christopherson
2020-09-25 21:23 ` [PATCH 20/22] kvm: mmu: NX largepage recovery for TDP MMU Ben Gardon
2020-09-26 1:14 ` Paolo Bonzini
2020-09-30 22:23 ` Ben Gardon
2020-09-29 18:24 ` Paolo Bonzini
2020-09-30 18:15 ` Sean Christopherson
2020-09-30 19:56 ` Paolo Bonzini
2020-09-30 22:33 ` Ben Gardon
2020-09-30 22:27 ` Ben Gardon
2020-09-25 21:23 ` [PATCH 21/22] kvm: mmu: Support MMIO in the " Ben Gardon
2020-09-30 18:19 ` Sean Christopherson
2020-09-25 21:23 ` [PATCH 22/22] kvm: mmu: Don't clear write flooding count for direct roots Ben Gardon
2020-09-26 1:25 ` Paolo Bonzini
2020-10-05 22:48 ` Ben Gardon
2020-10-05 23:44 ` Sean Christopherson
2020-10-06 16:19 ` Ben Gardon
2020-09-26 1:14 ` [PATCH 00/22] Introduce the TDP MMU Paolo Bonzini
2020-09-28 17:31 ` Paolo Bonzini
2020-09-29 17:40 ` Ben Gardon
2020-09-29 18:10 ` Paolo Bonzini
2020-09-30 6:19 ` Sean Christopherson
2020-09-30 6:30 ` Paolo Bonzini
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CANgfPd_A6Bbv+ehRvMVi_NK2C_Jb=bBmXJR89fj=JSFSga0avg@mail.gmail.com' \
--to=bgardon@google.com \
--cc=cannonmatthews@google.com \
--cc=jmattson@google.com \
--cc=junaids@google.com \
--cc=kernellwp@gmail.com \
--cc=kvm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=pbonzini@redhat.com \
--cc=peterx@redhat.com \
--cc=pfeiner@google.com \
--cc=pshier@google.com \
--cc=sean.j.christopherson@intel.com \
--cc=vkuznets@redhat.com \
--cc=xiaoguangrong.eric@gmail.com \
--cc=yulei.kernel@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).