* [PATCH 0/4] Fix RCU warnings in TDP MMU
@ 2021-03-11 23:16 Ben Gardon
2021-03-11 23:16 ` [PATCH 1/4] KVM: x86/mmu: Fix RCU usage in handle_removed_tdp_mmu_page Ben Gardon
` (4 more replies)
0 siblings, 5 replies; 14+ messages in thread
From: Ben Gardon @ 2021-03-11 23:16 UTC (permalink / raw)
To: linux-kernel, kvm
Cc: Paolo Bonzini, Sean Christopherson, Peter Shier, Jim Mattson, Ben Gardon
The Linux Test Robot found a few RCU warnings in the TDP MMU:
https://www.spinics.net/lists/kernel/msg3845500.html
https://www.spinics.net/lists/kernel/msg3845521.html
Fix these warnings and cleanup a hack in tdp_mmu_iter_cond_resched.
Tested by compiling as suggested in the test robot report and confirmed that
the warnings go away with this series applied. Also ran kvm-unit-tests on an
Intel Skylake machine with the TDP MMU enabled and confirmed that the series
introduced no new failures.
Ben Gardon (4):
KVM: x86/mmu: Fix RCU usage in handle_removed_tdp_mmu_page
KVM: x86/mmu: Fix RCU usage for tdp_iter_root_pt
KVM: x86/mmu: Fix RCU usage when atomically zapping SPTEs
KVM: x86/mmu: Factor out tdp_iter_return_to_root
arch/x86/kvm/mmu/tdp_iter.c | 34 +++++++++++++++++++++++++---------
arch/x86/kvm/mmu/tdp_iter.h | 3 ++-
arch/x86/kvm/mmu/tdp_mmu.c | 19 +++++++++++--------
3 files changed, 38 insertions(+), 18 deletions(-)
--
2.31.0.rc2.261.g7f71774620-goog
^ permalink raw reply [flat|nested] 14+ messages in thread* [PATCH 1/4] KVM: x86/mmu: Fix RCU usage in handle_removed_tdp_mmu_page 2021-03-11 23:16 [PATCH 0/4] Fix RCU warnings in TDP MMU Ben Gardon @ 2021-03-11 23:16 ` Ben Gardon 2021-03-12 15:37 ` Sean Christopherson 2021-03-11 23:16 ` [PATCH 2/4] KVM: x86/mmu: Fix RCU usage for tdp_iter_root_pt Ben Gardon ` (3 subsequent siblings) 4 siblings, 1 reply; 14+ messages in thread From: Ben Gardon @ 2021-03-11 23:16 UTC (permalink / raw) To: linux-kernel, kvm Cc: Paolo Bonzini, Sean Christopherson, Peter Shier, Jim Mattson, Ben Gardon The pt passed into handle_removed_tdp_mmu_page does not need RCU protection, as it is not at any risk of being freed by another thread at that point. However, the implicit cast from tdp_sptep_t to u64 * dropped the __rcu annotation without a proper rcu_derefrence. Fix this by passing the pt as a tdp_ptep_t and then rcu_dereferencing it in the function. Suggested-by: Sean Christopherson <seanjc@google.com> Reported-by: kernel test robot <lkp@xxxxxxxxx> Signed-off-by: Ben Gardon <bgardon@google.com> --- arch/x86/kvm/mmu/tdp_mmu.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c index c926c6b899a1..5387ac040f66 100644 --- a/arch/x86/kvm/mmu/tdp_mmu.c +++ b/arch/x86/kvm/mmu/tdp_mmu.c @@ -301,11 +301,16 @@ static void tdp_mmu_unlink_page(struct kvm *kvm, struct kvm_mmu_page *sp, * * Given a page table that has been removed from the TDP paging structure, * iterates through the page table to clear SPTEs and free child page tables. + * + * Note that pt is passed in as a tdp_ptep_t, but it does not need RCU + * protection. Since this thread removed it from the paging structure, + * this thread will be responsible for ensuring the page is freed. Hence the + * early rcu_dereferences in the function. */ -static void handle_removed_tdp_mmu_page(struct kvm *kvm, u64 *pt, +static void handle_removed_tdp_mmu_page(struct kvm *kvm, tdp_ptep_t pt, bool shared) { - struct kvm_mmu_page *sp = sptep_to_sp(pt); + struct kvm_mmu_page *sp = sptep_to_sp(rcu_dereference(pt)); int level = sp->role.level; gfn_t base_gfn = sp->gfn; u64 old_child_spte; @@ -318,7 +323,7 @@ static void handle_removed_tdp_mmu_page(struct kvm *kvm, u64 *pt, tdp_mmu_unlink_page(kvm, sp, shared); for (i = 0; i < PT64_ENT_PER_PAGE; i++) { - sptep = pt + i; + sptep = rcu_dereference(pt) + i; gfn = base_gfn + (i * KVM_PAGES_PER_HPAGE(level - 1)); if (shared) { -- 2.31.0.rc2.261.g7f71774620-goog ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 1/4] KVM: x86/mmu: Fix RCU usage in handle_removed_tdp_mmu_page 2021-03-11 23:16 ` [PATCH 1/4] KVM: x86/mmu: Fix RCU usage in handle_removed_tdp_mmu_page Ben Gardon @ 2021-03-12 15:37 ` Sean Christopherson 2021-03-12 15:43 ` Paolo Bonzini 0 siblings, 1 reply; 14+ messages in thread From: Sean Christopherson @ 2021-03-12 15:37 UTC (permalink / raw) To: Ben Gardon; +Cc: linux-kernel, kvm, Paolo Bonzini, Peter Shier, Jim Mattson On Thu, Mar 11, 2021, Ben Gardon wrote: > The pt passed into handle_removed_tdp_mmu_page does not need RCU > protection, as it is not at any risk of being freed by another thread at > that point. However, the implicit cast from tdp_sptep_t to u64 * dropped > the __rcu annotation without a proper rcu_derefrence. Fix this by > passing the pt as a tdp_ptep_t and then rcu_dereferencing it in > the function. > > Suggested-by: Sean Christopherson <seanjc@google.com> > Reported-by: kernel test robot <lkp@xxxxxxxxx> Should be <lkp@intel.com>. Looks like you've been taking pointers from Paolo :-) https://lkml.org/lkml/2019/6/17/1210 Other than that, Reviewed-by: Sean Christopherson <seanjc@google.com> > Signed-off-by: Ben Gardon <bgardon@google.com> ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/4] KVM: x86/mmu: Fix RCU usage in handle_removed_tdp_mmu_page 2021-03-12 15:37 ` Sean Christopherson @ 2021-03-12 15:43 ` Paolo Bonzini 2021-03-15 17:08 ` Ben Gardon 0 siblings, 1 reply; 14+ messages in thread From: Paolo Bonzini @ 2021-03-12 15:43 UTC (permalink / raw) To: Sean Christopherson, Ben Gardon Cc: linux-kernel, kvm, Peter Shier, Jim Mattson On 12/03/21 16:37, Sean Christopherson wrote: > On Thu, Mar 11, 2021, Ben Gardon wrote: >> The pt passed into handle_removed_tdp_mmu_page does not need RCU >> protection, as it is not at any risk of being freed by another thread at >> that point. However, the implicit cast from tdp_sptep_t to u64 * dropped >> the __rcu annotation without a proper rcu_derefrence. Fix this by >> passing the pt as a tdp_ptep_t and then rcu_dereferencing it in >> the function. >> >> Suggested-by: Sean Christopherson <seanjc@google.com> >> Reported-by: kernel test robot <lkp@xxxxxxxxx> > > Should be <lkp@intel.com>. Looks like you've been taking pointers from Paolo :-) The day someone starts confusing employers in CCs you should tell them "I see you have constructed a new email sending alias. Your skills are now complete". Paolo > https://lkml.org/lkml/2019/6/17/1210 > > Other than that, > > Reviewed-by: Sean Christopherson <seanjc@google.com> > >> Signed-off-by: Ben Gardon <bgardon@google.com> > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/4] KVM: x86/mmu: Fix RCU usage in handle_removed_tdp_mmu_page 2021-03-12 15:43 ` Paolo Bonzini @ 2021-03-15 17:08 ` Ben Gardon 0 siblings, 0 replies; 14+ messages in thread From: Ben Gardon @ 2021-03-15 17:08 UTC (permalink / raw) To: Paolo Bonzini; +Cc: Sean Christopherson, LKML, kvm, Peter Shier, Jim Mattson On Fri, Mar 12, 2021 at 7:43 AM Paolo Bonzini <pbonzini@redhat.com> wrote: > > On 12/03/21 16:37, Sean Christopherson wrote: > > On Thu, Mar 11, 2021, Ben Gardon wrote: > >> The pt passed into handle_removed_tdp_mmu_page does not need RCU > >> protection, as it is not at any risk of being freed by another thread at > >> that point. However, the implicit cast from tdp_sptep_t to u64 * dropped > >> the __rcu annotation without a proper rcu_derefrence. Fix this by > >> passing the pt as a tdp_ptep_t and then rcu_dereferencing it in > >> the function. > >> > >> Suggested-by: Sean Christopherson <seanjc@google.com> > >> Reported-by: kernel test robot <lkp@xxxxxxxxx> > > > > Should be <lkp@intel.com>. Looks like you've been taking pointers from Paolo :-) I'll update that in v2. I was a little confused because I was looking at the report archived on Spinics, where all the domains are xxxxxxxx. Didn't notice that all the emails had been redacted like that. > > The day someone starts confusing employers in CCs you should tell them > "I see you have constructed a new email sending alias. Your skills are > now complete". > > Paolo > > > https://lkml.org/lkml/2019/6/17/1210 > > > > Other than that, > > > > Reviewed-by: Sean Christopherson <seanjc@google.com> > > > >> Signed-off-by: Ben Gardon <bgardon@google.com> > > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 2/4] KVM: x86/mmu: Fix RCU usage for tdp_iter_root_pt 2021-03-11 23:16 [PATCH 0/4] Fix RCU warnings in TDP MMU Ben Gardon 2021-03-11 23:16 ` [PATCH 1/4] KVM: x86/mmu: Fix RCU usage in handle_removed_tdp_mmu_page Ben Gardon @ 2021-03-11 23:16 ` Ben Gardon 2021-03-12 16:22 ` Sean Christopherson 2021-03-11 23:16 ` [PATCH 3/4] KVM: x86/mmu: Fix RCU usage when atomically zapping SPTEs Ben Gardon ` (2 subsequent siblings) 4 siblings, 1 reply; 14+ messages in thread From: Ben Gardon @ 2021-03-11 23:16 UTC (permalink / raw) To: linux-kernel, kvm Cc: Paolo Bonzini, Sean Christopherson, Peter Shier, Jim Mattson, Ben Gardon The root page table in the TDP MMU paging structure is not protected with RCU, but rather by the root_count in the associated SP. As a result it is safe for tdp_iter_root_pt to simply return a u64 *. This sidesteps the complexities assoicated with propagating the __rcu annotation around. Reported-by: kernel test robot <lkp@xxxxxxxxx> Signed-off-by: Ben Gardon <bgardon@google.com> --- arch/x86/kvm/mmu/tdp_iter.c | 10 ++++++++-- arch/x86/kvm/mmu/tdp_iter.h | 2 +- arch/x86/kvm/mmu/tdp_mmu.c | 4 ++-- 3 files changed, 11 insertions(+), 5 deletions(-) diff --git a/arch/x86/kvm/mmu/tdp_iter.c b/arch/x86/kvm/mmu/tdp_iter.c index e5f148106e20..8e2c053533b6 100644 --- a/arch/x86/kvm/mmu/tdp_iter.c +++ b/arch/x86/kvm/mmu/tdp_iter.c @@ -159,8 +159,14 @@ void tdp_iter_next(struct tdp_iter *iter) iter->valid = false; } -tdp_ptep_t tdp_iter_root_pt(struct tdp_iter *iter) +u64 *tdp_iter_root_pt(struct tdp_iter *iter) { - return iter->pt_path[iter->root_level - 1]; + /* + * Though it is stored in an array of tdp_ptep_t for convenience, + * the root PT is not actually protected by RCU, but by the root + * count on the associated struct kvm_mmu_page. As a result it's + * safe to rcu_dereference and return the value here. + */ + return rcu_dereference(iter->pt_path[iter->root_level - 1]); } diff --git a/arch/x86/kvm/mmu/tdp_iter.h b/arch/x86/kvm/mmu/tdp_iter.h index 4cc177d75c4a..5a47c57810ab 100644 --- a/arch/x86/kvm/mmu/tdp_iter.h +++ b/arch/x86/kvm/mmu/tdp_iter.h @@ -62,6 +62,6 @@ tdp_ptep_t spte_to_child_pt(u64 pte, int level); void tdp_iter_start(struct tdp_iter *iter, u64 *root_pt, int root_level, int min_level, gfn_t next_last_level_gfn); void tdp_iter_next(struct tdp_iter *iter); -tdp_ptep_t tdp_iter_root_pt(struct tdp_iter *iter); +u64 *tdp_iter_root_pt(struct tdp_iter *iter); #endif /* __KVM_X86_MMU_TDP_ITER_H */ diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c index 5387ac040f66..6c8824bcc2f2 100644 --- a/arch/x86/kvm/mmu/tdp_mmu.c +++ b/arch/x86/kvm/mmu/tdp_mmu.c @@ -558,7 +558,7 @@ static inline void __tdp_mmu_set_spte(struct kvm *kvm, struct tdp_iter *iter, u64 new_spte, bool record_acc_track, bool record_dirty_log) { - tdp_ptep_t root_pt = tdp_iter_root_pt(iter); + u64 *root_pt = tdp_iter_root_pt(iter); struct kvm_mmu_page *root = sptep_to_sp(root_pt); int as_id = kvm_mmu_page_as_id(root); @@ -653,7 +653,7 @@ static inline bool tdp_mmu_iter_cond_resched(struct kvm *kvm, WARN_ON(iter->gfn > iter->next_last_level_gfn); - tdp_iter_start(iter, iter->pt_path[iter->root_level - 1], + tdp_iter_start(iter, tdp_iter_root_pt(iter), iter->root_level, iter->min_level, iter->next_last_level_gfn); -- 2.31.0.rc2.261.g7f71774620-goog ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 2/4] KVM: x86/mmu: Fix RCU usage for tdp_iter_root_pt 2021-03-11 23:16 ` [PATCH 2/4] KVM: x86/mmu: Fix RCU usage for tdp_iter_root_pt Ben Gardon @ 2021-03-12 16:22 ` Sean Christopherson 2021-03-15 17:13 ` Ben Gardon 0 siblings, 1 reply; 14+ messages in thread From: Sean Christopherson @ 2021-03-12 16:22 UTC (permalink / raw) To: Ben Gardon; +Cc: linux-kernel, kvm, Paolo Bonzini, Peter Shier, Jim Mattson On Thu, Mar 11, 2021, Ben Gardon wrote: > The root page table in the TDP MMU paging structure is not protected > with RCU, but rather by the root_count in the associated SP. As a result > it is safe for tdp_iter_root_pt to simply return a u64 *. This sidesteps > the complexities assoicated with propagating the __rcu annotation > around. > > Reported-by: kernel test robot <lkp@xxxxxxxxx> > Signed-off-by: Ben Gardon <bgardon@google.com> > --- > arch/x86/kvm/mmu/tdp_iter.c | 10 ++++++++-- > arch/x86/kvm/mmu/tdp_iter.h | 2 +- > arch/x86/kvm/mmu/tdp_mmu.c | 4 ++-- > 3 files changed, 11 insertions(+), 5 deletions(-) > > diff --git a/arch/x86/kvm/mmu/tdp_iter.c b/arch/x86/kvm/mmu/tdp_iter.c > index e5f148106e20..8e2c053533b6 100644 > --- a/arch/x86/kvm/mmu/tdp_iter.c > +++ b/arch/x86/kvm/mmu/tdp_iter.c > @@ -159,8 +159,14 @@ void tdp_iter_next(struct tdp_iter *iter) > iter->valid = false; > } > > -tdp_ptep_t tdp_iter_root_pt(struct tdp_iter *iter) > +u64 *tdp_iter_root_pt(struct tdp_iter *iter) > { > - return iter->pt_path[iter->root_level - 1]; > + /* > + * Though it is stored in an array of tdp_ptep_t for convenience, > + * the root PT is not actually protected by RCU, but by the root > + * count on the associated struct kvm_mmu_page. As a result it's > + * safe to rcu_dereference and return the value here. I'm not a big fan of this comment. It implies that calling tdp_iter_root_pt() without RCU protection is completely ok, but that's not true, as rcu_dereferecne() will complain when CONFIG_PROVE_RCU=1. There's also a good opportunity to streamline the the helper here, since both callers use the root only to get to the associated shadow page, and that's only done to get the as_id. If we provide tdp_iter_as_id() then the need for a comment goes away and we shave a few lines of code. That being said, an even better option would be to store as_id in the TDP iter. The cost on the stack is negligible, and while the early sptep->as_id lookup will be unnecessary in some cases, it will be a net win when setting multiple sptes, e.g. in mmu_notifier callbacks. Compile tested only... From 02fb9cd2aa52d0afd318e93661d0212ccdb54218 Mon Sep 17 00:00:00 2001 From: Sean Christopherson <seanjc@google.com> Date: Fri, 12 Mar 2021 08:12:21 -0800 Subject: [PATCH] KVM: x86/mmu: Store the address space ID in the TDP iterator Store the address space ID in the TDP iterator so that it can be retrieved without having to bounce through the root shadow page. This streamlines the code and fixes a Sparse warning about not properly using rcu_dereference() when grabbing the ID from the root on the fly. Reported-by: kernel test robot <lkp@intel.com> Cc: Ben Gardon <bgardon@google.com> Signed-off-by: Sean Christopherson <seanjc@google.com> --- arch/x86/kvm/mmu/mmu_internal.h | 5 +++++ arch/x86/kvm/mmu/tdp_iter.c | 7 +------ arch/x86/kvm/mmu/tdp_iter.h | 3 ++- arch/x86/kvm/mmu/tdp_mmu.c | 23 +++++------------------ 4 files changed, 13 insertions(+), 25 deletions(-) diff --git a/arch/x86/kvm/mmu/mmu_internal.h b/arch/x86/kvm/mmu/mmu_internal.h index ec4fc28b325a..e844078d2374 100644 --- a/arch/x86/kvm/mmu/mmu_internal.h +++ b/arch/x86/kvm/mmu/mmu_internal.h @@ -119,6 +119,11 @@ static inline bool kvm_mmu_put_root(struct kvm *kvm, struct kvm_mmu_page *sp) return !sp->root_count; } +static inline int kvm_mmu_page_as_id(struct kvm_mmu_page *sp) +{ + return sp->role.smm ? 1 : 0; +} + /* * Return values of handle_mmio_page_fault, mmu.page_fault, and fast_page_fault(). * diff --git a/arch/x86/kvm/mmu/tdp_iter.c b/arch/x86/kvm/mmu/tdp_iter.c index e5f148106e20..55d0ce2185a5 100644 --- a/arch/x86/kvm/mmu/tdp_iter.c +++ b/arch/x86/kvm/mmu/tdp_iter.c @@ -40,6 +40,7 @@ void tdp_iter_start(struct tdp_iter *iter, u64 *root_pt, int root_level, iter->gfn = round_gfn_for_level(iter->next_last_level_gfn, iter->level); tdp_iter_refresh_sptep(iter); + iter->as_id = kvm_mmu_page_as_id(sptep_to_sp(root_pt)); iter->valid = true; } @@ -158,9 +159,3 @@ void tdp_iter_next(struct tdp_iter *iter) } while (try_step_up(iter)); iter->valid = false; } - -tdp_ptep_t tdp_iter_root_pt(struct tdp_iter *iter) -{ - return iter->pt_path[iter->root_level - 1]; -} - diff --git a/arch/x86/kvm/mmu/tdp_iter.h b/arch/x86/kvm/mmu/tdp_iter.h index 4cc177d75c4a..df9c84713f5b 100644 --- a/arch/x86/kvm/mmu/tdp_iter.h +++ b/arch/x86/kvm/mmu/tdp_iter.h @@ -36,6 +36,8 @@ struct tdp_iter { int min_level; /* The iterator's current level within the paging structure */ int level; + /* The address space ID, i.e. SMM vs. regular. */ + int as_id; /* A snapshot of the value at sptep */ u64 old_spte; /* @@ -62,6 +64,5 @@ tdp_ptep_t spte_to_child_pt(u64 pte, int level); void tdp_iter_start(struct tdp_iter *iter, u64 *root_pt, int root_level, int min_level, gfn_t next_last_level_gfn); void tdp_iter_next(struct tdp_iter *iter); -tdp_ptep_t tdp_iter_root_pt(struct tdp_iter *iter); #endif /* __KVM_X86_MMU_TDP_ITER_H */ diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c index 21cbbef0ee57..9f436aa14663 100644 --- a/arch/x86/kvm/mmu/tdp_mmu.c +++ b/arch/x86/kvm/mmu/tdp_mmu.c @@ -190,11 +190,6 @@ static void handle_changed_spte(struct kvm *kvm, int as_id, gfn_t gfn, u64 old_spte, u64 new_spte, int level, bool shared); -static int kvm_mmu_page_as_id(struct kvm_mmu_page *sp) -{ - return sp->role.smm ? 1 : 0; -} - static void handle_changed_spte_acc_track(u64 old_spte, u64 new_spte, int level) { if (!is_shadow_present_pte(old_spte) || !is_last_spte(old_spte, level)) @@ -472,10 +467,6 @@ static inline bool tdp_mmu_set_spte_atomic(struct kvm *kvm, struct tdp_iter *iter, u64 new_spte) { - u64 *root_pt = tdp_iter_root_pt(iter); - struct kvm_mmu_page *root = sptep_to_sp(root_pt); - int as_id = kvm_mmu_page_as_id(root); - lockdep_assert_held_read(&kvm->mmu_lock); /* @@ -489,8 +480,8 @@ static inline bool tdp_mmu_set_spte_atomic(struct kvm *kvm, new_spte) != iter->old_spte) return false; - handle_changed_spte(kvm, as_id, iter->gfn, iter->old_spte, new_spte, - iter->level, true); + handle_changed_spte(kvm, iter->as_id, iter->gfn, iter->old_spte, + new_spte, iter->level, true); return true; } @@ -544,10 +535,6 @@ static inline void __tdp_mmu_set_spte(struct kvm *kvm, struct tdp_iter *iter, u64 new_spte, bool record_acc_track, bool record_dirty_log) { - tdp_ptep_t root_pt = tdp_iter_root_pt(iter); - struct kvm_mmu_page *root = sptep_to_sp(root_pt); - int as_id = kvm_mmu_page_as_id(root); - lockdep_assert_held_write(&kvm->mmu_lock); /* @@ -561,13 +548,13 @@ static inline void __tdp_mmu_set_spte(struct kvm *kvm, struct tdp_iter *iter, WRITE_ONCE(*rcu_dereference(iter->sptep), new_spte); - __handle_changed_spte(kvm, as_id, iter->gfn, iter->old_spte, new_spte, - iter->level, false); + __handle_changed_spte(kvm, iter->as_id, iter->gfn, iter->old_spte, + new_spte, iter->level, false); if (record_acc_track) handle_changed_spte_acc_track(iter->old_spte, new_spte, iter->level); if (record_dirty_log) - handle_changed_spte_dirty_log(kvm, as_id, iter->gfn, + handle_changed_spte_dirty_log(kvm, iter->as_id, iter->gfn, iter->old_spte, new_spte, iter->level); } -- ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 2/4] KVM: x86/mmu: Fix RCU usage for tdp_iter_root_pt 2021-03-12 16:22 ` Sean Christopherson @ 2021-03-15 17:13 ` Ben Gardon 0 siblings, 0 replies; 14+ messages in thread From: Ben Gardon @ 2021-03-15 17:13 UTC (permalink / raw) To: Sean Christopherson; +Cc: LKML, kvm, Paolo Bonzini, Peter Shier, Jim Mattson On Fri, Mar 12, 2021 at 8:22 AM Sean Christopherson <seanjc@google.com> wrote: > > On Thu, Mar 11, 2021, Ben Gardon wrote: > > The root page table in the TDP MMU paging structure is not protected > > with RCU, but rather by the root_count in the associated SP. As a result > > it is safe for tdp_iter_root_pt to simply return a u64 *. This sidesteps > > the complexities assoicated with propagating the __rcu annotation > > around. > > > > Reported-by: kernel test robot <lkp@xxxxxxxxx> > > Signed-off-by: Ben Gardon <bgardon@google.com> > > --- > > arch/x86/kvm/mmu/tdp_iter.c | 10 ++++++++-- > > arch/x86/kvm/mmu/tdp_iter.h | 2 +- > > arch/x86/kvm/mmu/tdp_mmu.c | 4 ++-- > > 3 files changed, 11 insertions(+), 5 deletions(-) > > > > diff --git a/arch/x86/kvm/mmu/tdp_iter.c b/arch/x86/kvm/mmu/tdp_iter.c > > index e5f148106e20..8e2c053533b6 100644 > > --- a/arch/x86/kvm/mmu/tdp_iter.c > > +++ b/arch/x86/kvm/mmu/tdp_iter.c > > @@ -159,8 +159,14 @@ void tdp_iter_next(struct tdp_iter *iter) > > iter->valid = false; > > } > > > > -tdp_ptep_t tdp_iter_root_pt(struct tdp_iter *iter) > > +u64 *tdp_iter_root_pt(struct tdp_iter *iter) > > { > > - return iter->pt_path[iter->root_level - 1]; > > + /* > > + * Though it is stored in an array of tdp_ptep_t for convenience, > > + * the root PT is not actually protected by RCU, but by the root > > + * count on the associated struct kvm_mmu_page. As a result it's > > + * safe to rcu_dereference and return the value here. > > I'm not a big fan of this comment. It implies that calling tdp_iter_root_pt() > without RCU protection is completely ok, but that's not true, as rcu_dereferecne() > will complain when CONFIG_PROVE_RCU=1. > > There's also a good opportunity to streamline the the helper here, since both > callers use the root only to get to the associated shadow page, and that's only > done to get the as_id. If we provide tdp_iter_as_id() then the need for a > comment goes away and we shave a few lines of code. This is a good suggestion. I have a change to do this in another series I was preparing to send out, but your suggestion below is even better, so I'll add that to this series. > > That being said, an even better option would be to store as_id in the TDP iter. > The cost on the stack is negligible, and while the early sptep->as_id lookup > will be unnecessary in some cases, it will be a net win when setting multiple > sptes, e.g. in mmu_notifier callbacks. > > Compile tested only... > > From 02fb9cd2aa52d0afd318e93661d0212ccdb54218 Mon Sep 17 00:00:00 2001 > From: Sean Christopherson <seanjc@google.com> > Date: Fri, 12 Mar 2021 08:12:21 -0800 > Subject: [PATCH] KVM: x86/mmu: Store the address space ID in the TDP iterator > > Store the address space ID in the TDP iterator so that it can be > retrieved without having to bounce through the root shadow page. This > streamlines the code and fixes a Sparse warning about not properly using > rcu_dereference() when grabbing the ID from the root on the fly. > > Reported-by: kernel test robot <lkp@intel.com> > Cc: Ben Gardon <bgardon@google.com> > Signed-off-by: Sean Christopherson <seanjc@google.com> > --- > arch/x86/kvm/mmu/mmu_internal.h | 5 +++++ > arch/x86/kvm/mmu/tdp_iter.c | 7 +------ > arch/x86/kvm/mmu/tdp_iter.h | 3 ++- > arch/x86/kvm/mmu/tdp_mmu.c | 23 +++++------------------ > 4 files changed, 13 insertions(+), 25 deletions(-) > > diff --git a/arch/x86/kvm/mmu/mmu_internal.h b/arch/x86/kvm/mmu/mmu_internal.h > index ec4fc28b325a..e844078d2374 100644 > --- a/arch/x86/kvm/mmu/mmu_internal.h > +++ b/arch/x86/kvm/mmu/mmu_internal.h > @@ -119,6 +119,11 @@ static inline bool kvm_mmu_put_root(struct kvm *kvm, struct kvm_mmu_page *sp) > return !sp->root_count; > } > > +static inline int kvm_mmu_page_as_id(struct kvm_mmu_page *sp) > +{ > + return sp->role.smm ? 1 : 0; > +} > + > /* > * Return values of handle_mmio_page_fault, mmu.page_fault, and fast_page_fault(). > * > diff --git a/arch/x86/kvm/mmu/tdp_iter.c b/arch/x86/kvm/mmu/tdp_iter.c > index e5f148106e20..55d0ce2185a5 100644 > --- a/arch/x86/kvm/mmu/tdp_iter.c > +++ b/arch/x86/kvm/mmu/tdp_iter.c > @@ -40,6 +40,7 @@ void tdp_iter_start(struct tdp_iter *iter, u64 *root_pt, int root_level, > iter->gfn = round_gfn_for_level(iter->next_last_level_gfn, iter->level); > tdp_iter_refresh_sptep(iter); > > + iter->as_id = kvm_mmu_page_as_id(sptep_to_sp(root_pt)); > iter->valid = true; > } > > @@ -158,9 +159,3 @@ void tdp_iter_next(struct tdp_iter *iter) > } while (try_step_up(iter)); > iter->valid = false; > } > - > -tdp_ptep_t tdp_iter_root_pt(struct tdp_iter *iter) > -{ > - return iter->pt_path[iter->root_level - 1]; > -} > - > diff --git a/arch/x86/kvm/mmu/tdp_iter.h b/arch/x86/kvm/mmu/tdp_iter.h > index 4cc177d75c4a..df9c84713f5b 100644 > --- a/arch/x86/kvm/mmu/tdp_iter.h > +++ b/arch/x86/kvm/mmu/tdp_iter.h > @@ -36,6 +36,8 @@ struct tdp_iter { > int min_level; > /* The iterator's current level within the paging structure */ > int level; > + /* The address space ID, i.e. SMM vs. regular. */ > + int as_id; > /* A snapshot of the value at sptep */ > u64 old_spte; > /* > @@ -62,6 +64,5 @@ tdp_ptep_t spte_to_child_pt(u64 pte, int level); > void tdp_iter_start(struct tdp_iter *iter, u64 *root_pt, int root_level, > int min_level, gfn_t next_last_level_gfn); > void tdp_iter_next(struct tdp_iter *iter); > -tdp_ptep_t tdp_iter_root_pt(struct tdp_iter *iter); > > #endif /* __KVM_X86_MMU_TDP_ITER_H */ > diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c > index 21cbbef0ee57..9f436aa14663 100644 > --- a/arch/x86/kvm/mmu/tdp_mmu.c > +++ b/arch/x86/kvm/mmu/tdp_mmu.c > @@ -190,11 +190,6 @@ static void handle_changed_spte(struct kvm *kvm, int as_id, gfn_t gfn, > u64 old_spte, u64 new_spte, int level, > bool shared); > > -static int kvm_mmu_page_as_id(struct kvm_mmu_page *sp) > -{ > - return sp->role.smm ? 1 : 0; > -} > - > static void handle_changed_spte_acc_track(u64 old_spte, u64 new_spte, int level) > { > if (!is_shadow_present_pte(old_spte) || !is_last_spte(old_spte, level)) > @@ -472,10 +467,6 @@ static inline bool tdp_mmu_set_spte_atomic(struct kvm *kvm, > struct tdp_iter *iter, > u64 new_spte) > { > - u64 *root_pt = tdp_iter_root_pt(iter); > - struct kvm_mmu_page *root = sptep_to_sp(root_pt); > - int as_id = kvm_mmu_page_as_id(root); > - > lockdep_assert_held_read(&kvm->mmu_lock); > > /* > @@ -489,8 +480,8 @@ static inline bool tdp_mmu_set_spte_atomic(struct kvm *kvm, > new_spte) != iter->old_spte) > return false; > > - handle_changed_spte(kvm, as_id, iter->gfn, iter->old_spte, new_spte, > - iter->level, true); > + handle_changed_spte(kvm, iter->as_id, iter->gfn, iter->old_spte, > + new_spte, iter->level, true); > > return true; > } > @@ -544,10 +535,6 @@ static inline void __tdp_mmu_set_spte(struct kvm *kvm, struct tdp_iter *iter, > u64 new_spte, bool record_acc_track, > bool record_dirty_log) > { > - tdp_ptep_t root_pt = tdp_iter_root_pt(iter); > - struct kvm_mmu_page *root = sptep_to_sp(root_pt); > - int as_id = kvm_mmu_page_as_id(root); > - > lockdep_assert_held_write(&kvm->mmu_lock); > > /* > @@ -561,13 +548,13 @@ static inline void __tdp_mmu_set_spte(struct kvm *kvm, struct tdp_iter *iter, > > WRITE_ONCE(*rcu_dereference(iter->sptep), new_spte); > > - __handle_changed_spte(kvm, as_id, iter->gfn, iter->old_spte, new_spte, > - iter->level, false); > + __handle_changed_spte(kvm, iter->as_id, iter->gfn, iter->old_spte, > + new_spte, iter->level, false); > if (record_acc_track) > handle_changed_spte_acc_track(iter->old_spte, new_spte, > iter->level); > if (record_dirty_log) > - handle_changed_spte_dirty_log(kvm, as_id, iter->gfn, > + handle_changed_spte_dirty_log(kvm, iter->as_id, iter->gfn, > iter->old_spte, new_spte, > iter->level); > } > -- ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 3/4] KVM: x86/mmu: Fix RCU usage when atomically zapping SPTEs 2021-03-11 23:16 [PATCH 0/4] Fix RCU warnings in TDP MMU Ben Gardon 2021-03-11 23:16 ` [PATCH 1/4] KVM: x86/mmu: Fix RCU usage in handle_removed_tdp_mmu_page Ben Gardon 2021-03-11 23:16 ` [PATCH 2/4] KVM: x86/mmu: Fix RCU usage for tdp_iter_root_pt Ben Gardon @ 2021-03-11 23:16 ` Ben Gardon 2021-03-12 16:24 ` Sean Christopherson 2021-03-11 23:16 ` [PATCH 4/4] KVM: x86/mmu: Factor out tdp_iter_return_to_root Ben Gardon 2021-03-12 17:01 ` [PATCH 0/4] Fix RCU warnings in TDP MMU Paolo Bonzini 4 siblings, 1 reply; 14+ messages in thread From: Ben Gardon @ 2021-03-11 23:16 UTC (permalink / raw) To: linux-kernel, kvm Cc: Paolo Bonzini, Sean Christopherson, Peter Shier, Jim Mattson, Ben Gardon Fix a missing rcu_dereference in tdp_mmu_zap_spte_atomic. Reported-by: kernel test robot <lkp@xxxxxxxxx> Signed-off-by: Ben Gardon <bgardon@google.com> --- arch/x86/kvm/mmu/tdp_mmu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c index 6c8824bcc2f2..a8fdccf4fd06 100644 --- a/arch/x86/kvm/mmu/tdp_mmu.c +++ b/arch/x86/kvm/mmu/tdp_mmu.c @@ -532,7 +532,7 @@ static inline bool tdp_mmu_zap_spte_atomic(struct kvm *kvm, * here since the SPTE is going from non-present * to non-present. */ - WRITE_ONCE(*iter->sptep, 0); + WRITE_ONCE(*rcu_dereference(iter->sptep), 0); return true; } -- 2.31.0.rc2.261.g7f71774620-goog ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 3/4] KVM: x86/mmu: Fix RCU usage when atomically zapping SPTEs 2021-03-11 23:16 ` [PATCH 3/4] KVM: x86/mmu: Fix RCU usage when atomically zapping SPTEs Ben Gardon @ 2021-03-12 16:24 ` Sean Christopherson 0 siblings, 0 replies; 14+ messages in thread From: Sean Christopherson @ 2021-03-12 16:24 UTC (permalink / raw) To: Ben Gardon; +Cc: linux-kernel, kvm, Paolo Bonzini, Peter Shier, Jim Mattson On Thu, Mar 11, 2021, Ben Gardon wrote: > Fix a missing rcu_dereference in tdp_mmu_zap_spte_atomic. > > Reported-by: kernel test robot <lkp@xxxxxxxxx> s/xxxxxxxxx/intel.com Reviewed-by: Sean Christopherson <seanjc@google.com> > Signed-off-by: Ben Gardon <bgardon@google.com> ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 4/4] KVM: x86/mmu: Factor out tdp_iter_return_to_root 2021-03-11 23:16 [PATCH 0/4] Fix RCU warnings in TDP MMU Ben Gardon ` (2 preceding siblings ...) 2021-03-11 23:16 ` [PATCH 3/4] KVM: x86/mmu: Fix RCU usage when atomically zapping SPTEs Ben Gardon @ 2021-03-11 23:16 ` Ben Gardon 2021-03-12 16:35 ` Sean Christopherson 2021-03-12 17:01 ` [PATCH 0/4] Fix RCU warnings in TDP MMU Paolo Bonzini 4 siblings, 1 reply; 14+ messages in thread From: Ben Gardon @ 2021-03-11 23:16 UTC (permalink / raw) To: linux-kernel, kvm Cc: Paolo Bonzini, Sean Christopherson, Peter Shier, Jim Mattson, Ben Gardon In tdp_mmu_iter_cond_resched there is a call to tdp_iter_start which causes the iterator to continue its walk over the paging structure from the root. This is needed after a yield as paging structure could have been freed in the interim. The tdp_iter_start call is not very clear and something of a hack. It requires exposing tdp_iter fields not used elsewhere in tdp_mmu.c and the effect is not obvious from the function name. Factor a more aptly named function out of tdp_iter_start and call it from tdp_mmu_iter_cond_resched and tdp_iter_start. No functional change intended. Signed-off-by: Ben Gardon <bgardon@google.com> --- arch/x86/kvm/mmu/tdp_iter.c | 24 +++++++++++++++++------- arch/x86/kvm/mmu/tdp_iter.h | 1 + arch/x86/kvm/mmu/tdp_mmu.c | 4 +--- 3 files changed, 19 insertions(+), 10 deletions(-) diff --git a/arch/x86/kvm/mmu/tdp_iter.c b/arch/x86/kvm/mmu/tdp_iter.c index 8e2c053533b6..bbf53b98cc65 100644 --- a/arch/x86/kvm/mmu/tdp_iter.c +++ b/arch/x86/kvm/mmu/tdp_iter.c @@ -20,6 +20,21 @@ static gfn_t round_gfn_for_level(gfn_t gfn, int level) return gfn & -KVM_PAGES_PER_HPAGE(level); } +/* + * Return the TDP iterator to the root PT and allow it to continue its + * traversal over the paging structure from there. + */ +void tdp_iter_return_to_root(struct tdp_iter *iter) +{ + iter->yielded_gfn = iter->next_last_level_gfn; + iter->level = iter->root_level; + + iter->gfn = round_gfn_for_level(iter->next_last_level_gfn, iter->level); + tdp_iter_refresh_sptep(iter); + + iter->valid = true; +} + /* * Sets a TDP iterator to walk a pre-order traversal of the paging structure * rooted at root_pt, starting with the walk to translate next_last_level_gfn. @@ -31,16 +46,11 @@ void tdp_iter_start(struct tdp_iter *iter, u64 *root_pt, int root_level, WARN_ON(root_level > PT64_ROOT_MAX_LEVEL); iter->next_last_level_gfn = next_last_level_gfn; - iter->yielded_gfn = iter->next_last_level_gfn; iter->root_level = root_level; iter->min_level = min_level; - iter->level = root_level; - iter->pt_path[iter->level - 1] = (tdp_ptep_t)root_pt; + iter->pt_path[iter->root_level - 1] = (tdp_ptep_t)root_pt; - iter->gfn = round_gfn_for_level(iter->next_last_level_gfn, iter->level); - tdp_iter_refresh_sptep(iter); - - iter->valid = true; + tdp_iter_return_to_root(iter); } /* diff --git a/arch/x86/kvm/mmu/tdp_iter.h b/arch/x86/kvm/mmu/tdp_iter.h index 5a47c57810ab..2ecc48e78526 100644 --- a/arch/x86/kvm/mmu/tdp_iter.h +++ b/arch/x86/kvm/mmu/tdp_iter.h @@ -63,5 +63,6 @@ void tdp_iter_start(struct tdp_iter *iter, u64 *root_pt, int root_level, int min_level, gfn_t next_last_level_gfn); void tdp_iter_next(struct tdp_iter *iter); u64 *tdp_iter_root_pt(struct tdp_iter *iter); +void tdp_iter_return_to_root(struct tdp_iter *iter); #endif /* __KVM_X86_MMU_TDP_ITER_H */ diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c index a8fdccf4fd06..941e9d11c7ed 100644 --- a/arch/x86/kvm/mmu/tdp_mmu.c +++ b/arch/x86/kvm/mmu/tdp_mmu.c @@ -653,9 +653,7 @@ static inline bool tdp_mmu_iter_cond_resched(struct kvm *kvm, WARN_ON(iter->gfn > iter->next_last_level_gfn); - tdp_iter_start(iter, tdp_iter_root_pt(iter), - iter->root_level, iter->min_level, - iter->next_last_level_gfn); + tdp_iter_return_to_root(iter); return true; } -- 2.31.0.rc2.261.g7f71774620-goog ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 4/4] KVM: x86/mmu: Factor out tdp_iter_return_to_root 2021-03-11 23:16 ` [PATCH 4/4] KVM: x86/mmu: Factor out tdp_iter_return_to_root Ben Gardon @ 2021-03-12 16:35 ` Sean Christopherson 2021-03-12 17:00 ` Paolo Bonzini 0 siblings, 1 reply; 14+ messages in thread From: Sean Christopherson @ 2021-03-12 16:35 UTC (permalink / raw) To: Ben Gardon; +Cc: linux-kernel, kvm, Paolo Bonzini, Peter Shier, Jim Mattson On Thu, Mar 11, 2021, Ben Gardon wrote: > In tdp_mmu_iter_cond_resched there is a call to tdp_iter_start which > causes the iterator to continue its walk over the paging structure from > the root. This is needed after a yield as paging structure could have > been freed in the interim. > > The tdp_iter_start call is not very clear and something of a hack. It > requires exposing tdp_iter fields not used elsewhere in tdp_mmu.c and > the effect is not obvious from the function name. Factor a more aptly > named function out of tdp_iter_start and call it from > tdp_mmu_iter_cond_resched and tdp_iter_start. What about calling it tdp_iter_restart()? Or tdp_iter_resume()? Or something like tdp_iter_restart_at_next() if we want it to give a hint that the next_last thing is where it restarts. I think I like tdp_iter_restart() the best. It'd be easy enough to add a function comment clarifying from where it restarts, and IMO the resulting code in tdp_mmu_iter_cond_resched() is the most intutive, e.g. it makes it very clear that the walk is being restarted in some capacity after yielding. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 4/4] KVM: x86/mmu: Factor out tdp_iter_return_to_root 2021-03-12 16:35 ` Sean Christopherson @ 2021-03-12 17:00 ` Paolo Bonzini 0 siblings, 0 replies; 14+ messages in thread From: Paolo Bonzini @ 2021-03-12 17:00 UTC (permalink / raw) To: Sean Christopherson, Ben Gardon Cc: linux-kernel, kvm, Peter Shier, Jim Mattson On 12/03/21 17:35, Sean Christopherson wrote: > What about calling it tdp_iter_restart()? Or tdp_iter_resume()? Or something > like tdp_iter_restart_at_next() if we want it to give a hint that the next_last > thing is where it restarts. > > I think I like tdp_iter_restart() the best. It'd be easy enough to add a > function comment clarifying from where it restarts, and IMO the resulting code > in tdp_mmu_iter_cond_resched() is the most intutive, e.g. it makes it very clear > that the walk is being restarted in some capacity after yielding. I agree with tdp_iter_restart(), or tdp_iter_restart_from_root() too. Paolo ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 0/4] Fix RCU warnings in TDP MMU 2021-03-11 23:16 [PATCH 0/4] Fix RCU warnings in TDP MMU Ben Gardon ` (3 preceding siblings ...) 2021-03-11 23:16 ` [PATCH 4/4] KVM: x86/mmu: Factor out tdp_iter_return_to_root Ben Gardon @ 2021-03-12 17:01 ` Paolo Bonzini 4 siblings, 0 replies; 14+ messages in thread From: Paolo Bonzini @ 2021-03-12 17:01 UTC (permalink / raw) To: Ben Gardon, linux-kernel, kvm Cc: Sean Christopherson, Peter Shier, Jim Mattson On 12/03/21 00:16, Ben Gardon wrote: > The Linux Test Robot found a few RCU warnings in the TDP MMU: > https://www.spinics.net/lists/kernel/msg3845500.html > https://www.spinics.net/lists/kernel/msg3845521.html > > Fix these warnings and cleanup a hack in tdp_mmu_iter_cond_resched. > > Tested by compiling as suggested in the test robot report and confirmed that > the warnings go away with this series applied. Also ran kvm-unit-tests on an > Intel Skylake machine with the TDP MMU enabled and confirmed that the series > introduced no new failures. > > Ben Gardon (4): > KVM: x86/mmu: Fix RCU usage in handle_removed_tdp_mmu_page > KVM: x86/mmu: Fix RCU usage for tdp_iter_root_pt > KVM: x86/mmu: Fix RCU usage when atomically zapping SPTEs > KVM: x86/mmu: Factor out tdp_iter_return_to_root > > arch/x86/kvm/mmu/tdp_iter.c | 34 +++++++++++++++++++++++++--------- > arch/x86/kvm/mmu/tdp_iter.h | 3 ++- > arch/x86/kvm/mmu/tdp_mmu.c | 19 +++++++++++-------- > 3 files changed, 38 insertions(+), 18 deletions(-) > Mostly good but Sean's suggestion in patch 2 is indeed better. There's enough changes that I'll wait for a v2 from you. Thanks! Paolo ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2021-03-15 17:14 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-03-11 23:16 [PATCH 0/4] Fix RCU warnings in TDP MMU Ben Gardon 2021-03-11 23:16 ` [PATCH 1/4] KVM: x86/mmu: Fix RCU usage in handle_removed_tdp_mmu_page Ben Gardon 2021-03-12 15:37 ` Sean Christopherson 2021-03-12 15:43 ` Paolo Bonzini 2021-03-15 17:08 ` Ben Gardon 2021-03-11 23:16 ` [PATCH 2/4] KVM: x86/mmu: Fix RCU usage for tdp_iter_root_pt Ben Gardon 2021-03-12 16:22 ` Sean Christopherson 2021-03-15 17:13 ` Ben Gardon 2021-03-11 23:16 ` [PATCH 3/4] KVM: x86/mmu: Fix RCU usage when atomically zapping SPTEs Ben Gardon 2021-03-12 16:24 ` Sean Christopherson 2021-03-11 23:16 ` [PATCH 4/4] KVM: x86/mmu: Factor out tdp_iter_return_to_root Ben Gardon 2021-03-12 16:35 ` Sean Christopherson 2021-03-12 17:00 ` Paolo Bonzini 2021-03-12 17:01 ` [PATCH 0/4] Fix RCU warnings in TDP MMU Paolo Bonzini
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).