[PATCH 06/17] KVM: x86/tdp_mmu: Morph the !is_frozen_spte() check into a KVM_MMU_WARN_ON()

Rick Edgecombe posted 17 patches 5 days, 17 hours ago
[PATCH 06/17] KVM: x86/tdp_mmu: Morph the !is_frozen_spte() check into a KVM_MMU_WARN_ON()
Posted by Rick Edgecombe 5 days, 17 hours ago
Remove the conditional logic for handling the setting of mirror EPTs to
frozen in __tdp_mmu_set_spte_atomic() and add it as a warning instead.

Mirror TDP needs propagate PTE changes to the to the external TDP. This
presents a problem for atomic updates which can't update both at once. So
a special value, FROZEN_SPTE, is used as a temporary state during these
updates to prevent concurrent operations to the PTE. If the TDP MMU tried
to install this as a long term value, it would confuse these updates.
Despite this __tdp_mmu_set_spte_atomic() includes a check to handle it
being set. Remove this check and turn it into a warning.

Suggested-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
---
 arch/x86/kvm/mmu/tdp_mmu.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c
index 0809fe8e8737..338957bc5109 100644
--- a/arch/x86/kvm/mmu/tdp_mmu.c
+++ b/arch/x86/kvm/mmu/tdp_mmu.c
@@ -656,7 +656,13 @@ static inline int __must_check __tdp_mmu_set_spte_atomic(struct kvm *kvm,
 	 */
 	WARN_ON_ONCE(iter->yielded || is_frozen_spte(iter->old_spte));
 
-	if (is_mirror_sptep(iter->sptep) && !is_frozen_spte(new_spte)) {
+	/*
+	 * FROZEN_SPTE is a temporary state and should never be set via higher
+	 * level helpers.
+	 */
+	KVM_MMU_WARN_ON(is_frozen_spte(new_spte));
+
+	if (is_mirror_sptep(iter->sptep)) {
 		int ret;
 
 		ret = set_external_spte_present(kvm, iter->sptep, iter->gfn,
-- 
2.53.0
Re: [PATCH 06/17] KVM: x86/tdp_mmu: Morph the !is_frozen_spte() check into a KVM_MMU_WARN_ON()
Posted by Yan Zhao 3 days, 9 hours ago
On Fri, Mar 27, 2026 at 01:14:10PM -0700, Rick Edgecombe wrote:
> Remove the conditional logic for handling the setting of mirror EPTs to
Should we unify the terms "mirror EPTs," "mirror TDP," and "mirror page tables"
in this series?

> frozen in __tdp_mmu_set_spte_atomic() and add it as a warning instead.
> 
> Mirror TDP needs propagate PTE changes to the to the external TDP. This
Two "to the".

> presents a problem for atomic updates which can't update both at once. So
> a special value, FROZEN_SPTE, is used as a temporary state during these
> updates to prevent concurrent operations to the PTE. If the TDP MMU tried
> to install this as a long term value, it would confuse these updates.
> Despite this __tdp_mmu_set_spte_atomic() includes a check to handle it
> being set. Remove this check and turn it into a warning.
> 
> Suggested-by: Sean Christopherson <seanjc@google.com>
> Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
> ---
>  arch/x86/kvm/mmu/tdp_mmu.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c
> index 0809fe8e8737..338957bc5109 100644
> --- a/arch/x86/kvm/mmu/tdp_mmu.c
> +++ b/arch/x86/kvm/mmu/tdp_mmu.c
> @@ -656,7 +656,13 @@ static inline int __must_check __tdp_mmu_set_spte_atomic(struct kvm *kvm,
>  	 */
>  	WARN_ON_ONCE(iter->yielded || is_frozen_spte(iter->old_spte));
>  
> -	if (is_mirror_sptep(iter->sptep) && !is_frozen_spte(new_spte)) {
> +	/*
> +	 * FROZEN_SPTE is a temporary state and should never be set via higher
> +	 * level helpers.
> +	 */
> +	KVM_MMU_WARN_ON(is_frozen_spte(new_spte));
Why is KVM_MMU_WARN_ON() used here for new_spte while WARN_ON_ONCE() is used
above for old_spte?

> +	if (is_mirror_sptep(iter->sptep)) {
>  		int ret;
>  
>  		ret = set_external_spte_present(kvm, iter->sptep, iter->gfn,
> -- 
> 2.53.0
>
Re: [PATCH 06/17] KVM: x86/tdp_mmu: Morph the !is_frozen_spte() check into a KVM_MMU_WARN_ON()
Posted by Edgecombe, Rick P 1 day, 21 hours ago
On Mon, 2026-03-30 at 13:00 +0800, Yan Zhao wrote:

Yep on the typos. 

> > --- a/arch/x86/kvm/mmu/tdp_mmu.c
> > +++ b/arch/x86/kvm/mmu/tdp_mmu.c
> > @@ -656,7 +656,13 @@ static inline int __must_check
> > __tdp_mmu_set_spte_atomic(struct kvm *kvm,
> >   	 */
> >   	WARN_ON_ONCE(iter->yielded || is_frozen_spte(iter-
> > >old_spte));
> >   
> > -	if (is_mirror_sptep(iter->sptep) &&
> > !is_frozen_spte(new_spte)) {
> > +	/*
> > +	 * FROZEN_SPTE is a temporary state and should never be
> > set via higher
> > +	 * level helpers.
> > +	 */
> > +	KVM_MMU_WARN_ON(is_frozen_spte(new_spte));
> Why is KVM_MMU_WARN_ON() used here for new_spte while WARN_ON_ONCE()
> is used
> above for old_spte?

For the KVM_MMU_WARN_ON() it was Sean's suggestion.

https://lore.kernel.org/lkml/aYYn0nf2cayYu8e7@google.com/

It allows for compiling it out, so probably a better choice. So I see
the options are leave them different or opportunistically convert the
other one to KVM_MMU_WARN_ON(). Thoughts?
Re: [PATCH 06/17] KVM: x86/tdp_mmu: Morph the !is_frozen_spte() check into a KVM_MMU_WARN_ON()
Posted by Yan Zhao 13 hours ago
On Wed, Apr 01, 2026 at 12:37:51AM +0800, Edgecombe, Rick P wrote:
> On Mon, 2026-03-30 at 13:00 +0800, Yan Zhao wrote:
> 
> Yep on the typos. 
> 
> > > --- a/arch/x86/kvm/mmu/tdp_mmu.c
> > > +++ b/arch/x86/kvm/mmu/tdp_mmu.c
> > > @@ -656,7 +656,13 @@ static inline int __must_check
> > > __tdp_mmu_set_spte_atomic(struct kvm *kvm,
> > >   	 */
> > >   	WARN_ON_ONCE(iter->yielded || is_frozen_spte(iter-
> > > >old_spte));
> > >   
> > > -	if (is_mirror_sptep(iter->sptep) &&
> > > !is_frozen_spte(new_spte)) {
> > > +	/*
> > > +	 * FROZEN_SPTE is a temporary state and should never be
> > > set via higher
> > > +	 * level helpers.
> > > +	 */
> > > +	KVM_MMU_WARN_ON(is_frozen_spte(new_spte));
> > Why is KVM_MMU_WARN_ON() used here for new_spte while WARN_ON_ONCE()
> > is used
> > above for old_spte?
> 
> For the KVM_MMU_WARN_ON() it was Sean's suggestion.
> 
> https://lore.kernel.org/lkml/aYYn0nf2cayYu8e7@google.com/
> 
> It allows for compiling it out, so probably a better choice. So I see
> the options are leave them different or opportunistically convert the
> other one to KVM_MMU_WARN_ON(). Thoughts?
I see there are mixed WARN_ON_ONCE() and KVM_MMU_WARN_ON() calls in mmu.c and
tdp_mmu.c. I'm not sure if there's a rule for which one to use.
Is it necessary to evaluate them all and have a separate patch to convert
WARN_ON_ONCE() to KVM_MMU_WARN_ON()?