| Message ID | 20210525223447.2724663-1-dmatlack@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: x86/mmu: Remove stale comment mentioning skip_4k | expand |
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 >
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 > >
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,
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(-)