[PATCH 0/4] Fix RCU warnings in TDP MMU

[PATCH 0/4] Fix RCU warnings in TDP MMU

[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

[PATCH 1/4] KVM: x86/mmu: Fix RCU usage in handle_removed_tdp_mmu_page

[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

[PATCH 2/4] KVM: x86/mmu: Fix RCU usage for tdp_iter_root_pt

[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

[PATCH 3/4] KVM: x86/mmu: Fix RCU usage when atomically zapping SPTEs

[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

[PATCH 4/4] KVM: x86/mmu: Factor out tdp_iter_return_to_root

[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

Re: [PATCH 1/4] KVM: x86/mmu: Fix RCU usage in handle_removed_tdp_mmu_page

[Sean Christopherson]

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>

Re: [PATCH 1/4] KVM: x86/mmu: Fix RCU usage in handle_removed_tdp_mmu_page

[Paolo Bonzini]

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>
> 

Re: [PATCH 2/4] KVM: x86/mmu: Fix RCU usage for tdp_iter_root_pt

[Sean Christopherson]

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);
 }
--

Re: [PATCH 3/4] KVM: x86/mmu: Fix RCU usage when atomically zapping SPTEs

[Sean Christopherson]

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>

Re: [PATCH 4/4] KVM: x86/mmu: Factor out tdp_iter_return_to_root

[Sean Christopherson]

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.

Re: [PATCH 4/4] KVM: x86/mmu: Factor out tdp_iter_return_to_root

[Paolo Bonzini]

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

Re: [PATCH 0/4] Fix RCU warnings in TDP MMU

[Paolo Bonzini]

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

Re: [PATCH 1/4] KVM: x86/mmu: Fix RCU usage in handle_removed_tdp_mmu_page

[Ben Gardon]

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>
> >
>

Re: [PATCH 2/4] KVM: x86/mmu: Fix RCU usage for tdp_iter_root_pt

[Ben Gardon]

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);
>  }
> --