On Wed, Oct 9, 2024 at 10:35 AM Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
>
> 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.
I am fine with KVM_MMU_WARN_ON() here. Callers should check for the
value they provided and returned from this API and if it's important
to them in Production then decide on next steps accordingly.
