Re: [PATCH v2 4/6] KVM: x86/mmu: Recover TDP MMU huge page mappings in-place instead of zapping

[Sean Christopherson <seanjc@google.com>]

On Wed, Oct 09, 2024, Vipin Sharma wrote:
> On Fri, Aug 23, 2024 at 4:57 PM David Matlack <dmatlack@google.com> wrote:
> > +static u64 modify_spte_protections(u64 spte, u64 set, u64 clear)
> >  {
> >         bool is_access_track = is_access_track_spte(spte);
> >
> >         if (is_access_track)
> >                 spte = restore_acc_track_spte(spte);
> >
> > -       spte &= ~shadow_nx_mask;
> > -       spte |= shadow_x_mask;
> > +       spte = (spte | set) & ~clear;
> 
> We should add a check here WARN_ON_ONCE(set & clear) because if both
> have a common bit set to 1 then the result  will be different between:
> 1. spte = (spt | set) & ~clear
> 2. spte = (spt | ~clear) & set
> 
> In the current form, 'clear' has more authority in the final value of spte.

KVM_MMU_WARN_ON(), overlapping @set and @clear is definitely something that should
be caught during development, i.e. we don't need to carry the WARN_ON_ONCE() in
production kernels

> > +u64 make_huge_spte(struct kvm *kvm, u64 small_spte, int level)
> > +{
> > +       u64 huge_spte;
> > +
> > +       if (KVM_BUG_ON(!is_shadow_present_pte(small_spte), kvm))
> > +               return SHADOW_NONPRESENT_VALUE;
> > +
> > +       if (KVM_BUG_ON(level == PG_LEVEL_4K, kvm))
> > +               return SHADOW_NONPRESENT_VALUE;
> > +
> 
> KVM_BUG_ON() is very aggressive. We should replace it with WARN_ON_ONCE()

I'm tempted to say KVM_MMU_WARN_ON() here too.