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.
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().
