Received-SPF: pass (google.com: domain of linux-kernel+bounces-214017-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Thu, 13 Jun 2024 14:15:17 -0700
From: David Matlack <dmatlack@google.com>
To: Sean Christopherson <seanjc@google.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>, kvm@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH] KVM: x86/mmu: Clean up function comments for dirty
 logging APIs
Message-ID: <ZmthZVGmgcM5NQEm@google.com>
References: <20240611215805.340664-1-seanjc@google.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240611215805.340664-1-seanjc@google.com>

On 2024-06-11 02:58 PM, Sean Christopherson wrote:
> Rework the function comment for kvm_arch_mmu_enable_log_dirty_pt_masked(),
> as it has gotten a bit stale, and is the last source of warnings for W=1
> builds in KVM x86 due to using a kernel-doc comment without documenting
> all parameters.
> 
> Opportunistically subsume the functions comments for
> kvm_mmu_write_protect_pt_masked() and kvm_mmu_clear_dirty_pt_masked(), as
> there is no value in regurgitating the same parameter information, and
> capturing the differences between write-protection and PML-based dirty
> logging is best done in a common location.
> 
> No functional change intended.
> 
> Cc: David Matlack <dmatlack@google.com>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
> 
> I don't actually care too much about the comment itself, I really just want to
> get rid of the annoying warnings (I was *very* tempted to just delete the extra
> asterisk), so if anyone has any opinion whatsoever...

I vote to drop it and document the nuance around PML in the function
body itself. The initially-all-set / huge page stuff is already mostly
documented in the function body. e.g.

diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
index dfea48bdd285..d56fa757ee81 100644
--- a/arch/x86/kvm/mmu/mmu.c
+++ b/arch/x86/kvm/mmu/mmu.c
@@ -1372,24 +1372,16 @@ static void kvm_mmu_clear_dirty_pt_masked(struct kvm *kvm,
 	}
 }

-/**
- * kvm_arch_mmu_enable_log_dirty_pt_masked - enable dirty logging for selected
- * PT level pages.
- *
- * It calls kvm_mmu_write_protect_pt_masked to write protect selected pages to
- * enable dirty logging for them.
- *
- * We need to care about huge page mappings: e.g. during dirty logging we may
- * have such mappings.
- */
 void kvm_arch_mmu_enable_log_dirty_pt_masked(struct kvm *kvm,
 				struct kvm_memory_slot *slot,
 				gfn_t gfn_offset, unsigned long mask)
 {
 	/*
-	 * Huge pages are NOT write protected when we start dirty logging in
-	 * initially-all-set mode; must write protect them here so that they
-	 * are split to 4K on the first write.
+	 * If the slot was assumed to be "initially all dirty", write-protect
+	 * huge pages to ensure they are split to 4KiB on the first write (KVM
+	 * dirty logs at 4KiB granularity). If eager page splitting is enabled,
+	 * try to split pages, e.g. so that vCPUs don't get saddled with the
+	 * cost of splitting.
 	 *
 	 * The gfn_offset is guaranteed to be aligned to 64, but the base_gfn
 	 * of memslot has no such restriction, so the range can cross two large
@@ -1411,7 +1403,16 @@ void kvm_arch_mmu_enable_log_dirty_pt_masked(struct kvm *kvm,
 						       PG_LEVEL_2M);
 	}

-	/* Now handle 4K PTEs.  */
+	/*
+	 * Re(enable) dirty logging for all 4KiB SPTEs that map the GFNs in
+	 * mask. If PML is enabled and the and the GFN doesn't need to be
+	 * write-protected for other reasons, e.g. shadow paging, clear the
+	 * Dirty bit. Otherwise clear the Writable bit.
+	 *
+	 * Note that kvm_mmu_clear_dirty_pt_masked() is called whenever PML is
+	 * enabled but it chooses between clearing the Dirty bit and Writeable
+	 * bit based on the context.
+	 */
 	if (kvm_x86_ops.cpu_dirty_log_size)
 		kvm_mmu_clear_dirty_pt_masked(kvm, slot, gfn_offset, mask);
 	else

> 
>  arch/x86/kvm/mmu/mmu.c | 43 ++++++++++++++++++------------------------
>  1 file changed, 18 insertions(+), 25 deletions(-)
> 
> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> index f2c9580d9588..7eb87d473223 100644
> --- a/arch/x86/kvm/mmu/mmu.c
> +++ b/arch/x86/kvm/mmu/mmu.c
> @@ -1307,15 +1307,6 @@ static bool __rmap_clear_dirty(struct kvm *kvm, struct kvm_rmap_head *rmap_head,
>  	return flush;
>  }
>  
> -/**
> - * kvm_mmu_write_protect_pt_masked - write protect selected PT level pages
> - * @kvm: kvm instance
> - * @slot: slot to protect
> - * @gfn_offset: start of the BITS_PER_LONG pages we care about
> - * @mask: indicates which pages we should protect
> - *
> - * Used when we do not need to care about huge page mappings.
> - */
>  static void kvm_mmu_write_protect_pt_masked(struct kvm *kvm,
>  				     struct kvm_memory_slot *slot,
>  				     gfn_t gfn_offset, unsigned long mask)
> @@ -1339,16 +1330,6 @@ static void kvm_mmu_write_protect_pt_masked(struct kvm *kvm,
>  	}
>  }
>  
> -/**
> - * kvm_mmu_clear_dirty_pt_masked - clear MMU D-bit for PT level pages, or write
> - * protect the page if the D-bit isn't supported.
> - * @kvm: kvm instance
> - * @slot: slot to clear D-bit
> - * @gfn_offset: start of the BITS_PER_LONG pages we care about
> - * @mask: indicates which pages we should clear D-bit
> - *
> - * Used for PML to re-log the dirty GPAs after userspace querying dirty_bitmap.
> - */
>  static void kvm_mmu_clear_dirty_pt_masked(struct kvm *kvm,
>  					 struct kvm_memory_slot *slot,
>  					 gfn_t gfn_offset, unsigned long mask)
> @@ -1373,14 +1354,26 @@ static void kvm_mmu_clear_dirty_pt_masked(struct kvm *kvm,
>  }
>  
>  /**
> - * kvm_arch_mmu_enable_log_dirty_pt_masked - enable dirty logging for selected
> - * PT level pages.
> + * kvm_arch_mmu_enable_log_dirty_pt_masked - (Re)Enable dirty logging for a set
> + * of GFNs
>   *
> - * It calls kvm_mmu_write_protect_pt_masked to write protect selected pages to
> - * enable dirty logging for them.
> + * @kvm: kvm instance
> + * @slot: slot to containing the gfns to dirty log
> + * @gfn_offset: start of the BITS_PER_LONG pages we care about

Someone once told me to avoid using "we" in comments :)

> + * @mask: indicates which gfns to dirty log (1 == enable)
>   *
> - * We need to care about huge page mappings: e.g. during dirty logging we may
> - * have such mappings.
> + * (Re)Enable dirty logging for the set of GFNs indicated by the slot,
> + * gfn_offset, and mask, e.g. after userspace has harvested dirty information
> + * and wants to re-log dirty GFNs for the next round of migration.
> + *
> + * If the slot was assumed to be "initially all dirty", write-protect hugepages
> + * to ensure they are split to 4KiB on the first write (KVM dirty logs at 4KiB
> + * granularity).  If eager page splitting is enabled, immediately try to split
> + * hugepages, e.g. so that vCPUs don't get saddled with the cost of the split.
> + *
> + * If Page-Modification Logging (PML) is enabled and the GFN doesn't need to be
> + * write-protected for other reasons, e.g. shadow paging, clear the Dirty Bit.
> + * Otherwise write-protect the GFN, i.e. clear the Writable Bit.
>   */
>  void kvm_arch_mmu_enable_log_dirty_pt_masked(struct kvm *kvm,
>  				struct kvm_memory_slot *slot,
> 
> base-commit: f99b052256f16224687e5947772f0942bff73fc1
> -- 
> 2.45.2.505.gda0bf45e8d-goog
>