On Wed, Oct 09, 2024, Vipin Sharma wrote:
> On Fri, Aug 23, 2024 at 4:57 PM David Matlack <dmatlack@xxxxxxxxxx> 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.
