Re: [PATCH v2 3/4] KVM: x86/mmu: Document and enforce MMU-writable and Host-writable invariants

[David Matlack <dmatlack@google.com>]

On Fri, Jan 14, 2022 at 2:29 PM Sean Christopherson <seanjc@google.com> wrote:
>
> On Thu, Jan 13, 2022, David Matlack wrote:
> > +/*
> > + * *_SPTE_HOST_WRITEABLE (aka Host-writable) indicates whether the host permits
> > + * writes to the guest page mapped by the SPTE. This bit is cleared on SPTEs
> > + * that map guest pages in read-only memslots and read-only VMAs.
> > + *
> > + * Invariants:
> > + *  - If Host-writable is clear, PT_WRITABLE_MASK must be clear.
> > + *
> > + *
> > + * *_SPTE_MMU_WRITEABLE (aka MMU-writable) indicates whether the shadow MMU
> > + * allows writes to the guest page mapped by the SPTE. This bit is cleared when
> > + * the guest page mapped by the SPTE contains a page table that is being
> > + * monitored for shadow paging. In this case the SPTE can only be made writable
> > + * by unsyncing the shadow page under the mmu_lock.
> > + *
> > + * Invariants:
> > + *  - If MMU-writable is clear, PT_WRITABLE_MASK must be clear.
> > + *  - If MMU-writable is set, Host-writable must be set.
> > + *
> > + * If MMU-writable is set, PT_WRITABLE_MASK is normally set but can be cleared
> > + * to track writes for dirty logging. For such SPTEs, KVM will locklessly set
> > + * PT_WRITABLE_MASK upon the next write from the guest and record the write in
> > + * the dirty log (see fast_page_fault()).
> > + */
> > +
> > +/* Bits 9 and 10 are ignored by all non-EPT PTEs. */
> > +#define DEFAULT_SPTE_HOST_WRITEABLE  BIT_ULL(9)
> > +#define DEFAULT_SPTE_MMU_WRITEABLE   BIT_ULL(10)
>
> Ha, so there's a massive comment above is_writable_pte() that covers a lot of
> the same material.  More below.
>
> > +
> >  /*
> >   * Low ignored bits are at a premium for EPT, use high ignored bits, taking care
> >   * to not overlap the A/D type mask or the saved access bits of access-tracked
> > @@ -316,8 +341,13 @@ static __always_inline bool is_rsvd_spte(struct rsvd_bits_validate *rsvd_check,
> >
> >  static inline bool spte_can_locklessly_be_made_writable(u64 spte)
> >  {
> > -     return (spte & shadow_host_writable_mask) &&
> > -            (spte & shadow_mmu_writable_mask);
> > +     if (spte & shadow_mmu_writable_mask) {
> > +             WARN_ON_ONCE(!(spte & shadow_host_writable_mask));
> > +             return true;
> > +     }
> > +
> > +     WARN_ON_ONCE(spte & PT_WRITABLE_MASK);
>
> I don't like having the WARNs here.  This is a moderately hot path, there are a
> decent number of call sites, and the WARNs won't actually help detect the offender,
> i.e. whoever wrote the bad SPTE long since got away.

Re: hot path. The "return true" case (for fast_page_fault()) already
had to do 2 bitwise-ANDs and compares, so this patch shouldn't make
that any worse.

But that's a good point that it doesn't help with detecting the
offender. I agree these WARNs should move to where SPTEs are set.

>
> And for whatever reason, I had a hell of a time (correctly) reading the second WARN :-)
>
> Lastly, there's also an "overlapping" WARN in mark_spte_for_access_track().
>
> > +     return false;
>
> To kill a few birds with fewer stones, what if we:
>
>   a. Move is_writable_pte() into spte.h, somewhat close to the HOST/MMU_WRITABLE
>      definitions.
>
>   b. Add a new helper, spte_check_writable_invariants(), to enforce that a SPTE
>      is WRITABLE iff it's MMU-Writable, and that a SPTE is MMU-Writable iff it's
>      HOST-Writable.
>
>   c. Drop the WARN in mark_spte_for_access_track().
>
>   d. Call spte_check_writable_invariants() when setting SPTEs.
>
>   e. Document everything in a comment above spte_check_writable_invariants().

Sounds good. I'll send a follow-up series.