[PATCH] KVM: x86/mmu: Remove stale comment mentioning skip_4k

[PATCH] KVM: x86/mmu: Remove stale comment mentioning skip_4k

[David Matlack]

This comment was left over from a previous version of the patch that
introduced wrprot_gfn_range, when skip_4k was passed in instead of
min_level.

Remove the comment (instead of fixing it) since wrprot_gfn_range has
only one caller and min_level is documented there.

Signed-off-by: David Matlack <dmatlack@google.com>
---
 arch/x86/kvm/mmu/tdp_mmu.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c
index 95eeb5ac6a8a..97f273912764 100644
--- a/arch/x86/kvm/mmu/tdp_mmu.c
+++ b/arch/x86/kvm/mmu/tdp_mmu.c
@@ -1192,8 +1192,7 @@ bool kvm_tdp_mmu_set_spte_gfn(struct kvm *kvm, struct kvm_gfn_range *range)
 }
 
 /*
- * Remove write access from all the SPTEs mapping GFNs [start, end). If
- * skip_4k is set, SPTEs that map 4k pages, will not be write-protected.
+ * Remove write access from all the SPTEs mapping GFNs [start, end).
  * Returns true if an SPTE has been changed and the TLBs need to be flushed.
  */
 static bool wrprot_gfn_range(struct kvm *kvm, struct kvm_mmu_page *root,
-- 
2.32.0.rc0.204.g9fa02ecfa5-goog

Re: [PATCH] KVM: x86/mmu: Remove stale comment mentioning skip_4k

[Sean Christopherson]

On Tue, May 25, 2021, David Matlack wrote:
> This comment was left over from a previous version of the patch that
> introduced wrprot_gfn_range, when skip_4k was passed in instead of
> min_level.
> 
> Remove the comment (instead of fixing it) since wrprot_gfn_range has
> only one caller and min_level is documented there.

That would be ok-ish if there were no comment whatsoever, but the function comment
is now flat out wrong, which is bad.

> Signed-off-by: David Matlack <dmatlack@google.com>
> ---
>  arch/x86/kvm/mmu/tdp_mmu.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c
> index 95eeb5ac6a8a..97f273912764 100644
> --- a/arch/x86/kvm/mmu/tdp_mmu.c
> +++ b/arch/x86/kvm/mmu/tdp_mmu.c
> @@ -1192,8 +1192,7 @@ bool kvm_tdp_mmu_set_spte_gfn(struct kvm *kvm, struct kvm_gfn_range *range)
>  }
>  
>  /*
> - * Remove write access from all the SPTEs mapping GFNs [start, end). If
> - * skip_4k is set, SPTEs that map 4k pages, will not be write-protected.
> + * Remove write access from all the SPTEs mapping GFNs [start, end).

If the goal is to avoid churn on the last sentence, what about:

    * Write protect SPTEs mapping GFNs [start, end) at or above min_level.

>   * Returns true if an SPTE has been changed and the TLBs need to be flushed.
>   */
>  static bool wrprot_gfn_range(struct kvm *kvm, struct kvm_mmu_page *root,
> -- 
> 2.32.0.rc0.204.g9fa02ecfa5-goog
> 

Re: [PATCH] KVM: x86/mmu: Remove stale comment mentioning skip_4k

[David Matlack]

On Wed, May 26, 2021 at 8:12 AM Sean Christopherson <seanjc@google.com> wrote:
>
> On Tue, May 25, 2021, David Matlack wrote:
> > This comment was left over from a previous version of the patch that
> > introduced wrprot_gfn_range, when skip_4k was passed in instead of
> > min_level.
> >
> > Remove the comment (instead of fixing it) since wrprot_gfn_range has
> > only one caller and min_level is documented there.
>
> That would be ok-ish if there were no comment whatsoever, but the function comment
> is now flat out wrong, which is bad.

You're right!

>
> > Signed-off-by: David Matlack <dmatlack@google.com>
> > ---
> >  arch/x86/kvm/mmu/tdp_mmu.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c
> > index 95eeb5ac6a8a..97f273912764 100644
> > --- a/arch/x86/kvm/mmu/tdp_mmu.c
> > +++ b/arch/x86/kvm/mmu/tdp_mmu.c
> > @@ -1192,8 +1192,7 @@ bool kvm_tdp_mmu_set_spte_gfn(struct kvm *kvm, struct kvm_gfn_range *range)
> >  }
> >
> >  /*
> > - * Remove write access from all the SPTEs mapping GFNs [start, end). If
> > - * skip_4k is set, SPTEs that map 4k pages, will not be write-protected.
> > + * Remove write access from all the SPTEs mapping GFNs [start, end).
>
> If the goal is to avoid churn on the last sentence, what about:
>
>     * Write protect SPTEs mapping GFNs [start, end) at or above min_level.

Thanks. Will send another patch fixing the comment.

>
> >   * Returns true if an SPTE has been changed and the TLBs need to be flushed.
> >   */
> >  static bool wrprot_gfn_range(struct kvm *kvm, struct kvm_mmu_page *root,
> > --
> > 2.32.0.rc0.204.g9fa02ecfa5-goog
> >