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

[Sean Christopherson <seanjc@google.com>]

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