On Fri, Jan 14, 2022 at 2:29 PM Sean Christopherson <seanjc@xxxxxxxxxx> 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.
