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

[Yan Zhao]

On Thu, Apr 02, 2026 at 12:21:13PM -0700, Sean Christopherson wrote:
> On Thu, Apr 02, 2026, Yan Zhao wrote:
> > 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@xxxxxxxxxx/
> > > 
> > > 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.
> 
> KVM_MMU_WARN_ON() is intended to be used only when having a WARN_ON_ONCE() enabled
> in production environments isn't worth the cost of the code.  E.g. if the code in
> question is a low level helper that used "everywhere" and in super hot paths, or
> in extremely paranoid sanity checks where the platform is beyond hosed if the
> sanity check fails.
> 
> The other thing with KVM_MMU_WARN_ON() is that it doesn't evaluate the expression
> when CONFIG_KVM_PROVE_MMU=n, i.e. can't be used in if-statements (the only case
> where it's used in an if-statement is wrapped with a CONFIG_KVM_PROVE_MMU #ifdef).
Ah, I missed this one.

> For infrequently used and/or slow paths, and for cases where KVM needs to take
> action no matter what, WARN_ON_ONCE() is preferred because if something goes
> sideways, we'll get a helpful splat even in production.
Got it. Thanks for the detailed explanation!

> > Is it necessary to evaluate them all and have a separate patch to convert
> > WARN_ON_ONCE() to KVM_MMU_WARN_ON()?
> 
> Nope.  We could probably convert a few select ones if there's good reason to do
> so, but we definitely don't want to do a bulk conversion.
Makes sense.