* Re: [PATCH] KVM: x86/mmu: Clean up function comments for dirty logging APIs
2024-06-11 21:58 [PATCH] KVM: x86/mmu: Clean up function comments for dirty logging APIs Sean Christopherson
@ 2024-06-13 21:15 ` David Matlack
2024-06-13 23:33 ` Sean Christopherson
0 siblings, 1 reply; 3+ messages in thread
From: David Matlack @ 2024-06-13 21:15 UTC (permalink / raw)
To: Sean Christopherson; +Cc: Paolo Bonzini, kvm, linux-kernel
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
>
^ permalink raw reply related [flat|nested] 3+ messages in thread