| Message ID | 20210526163227.3113557-1-dmatlack@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: x86/mmu: Fix comment mentioning skip_4k | expand |
Put version information in the subject, otherwise it's not always obvious which patch you want to be accepted, e.g. [PATCH v2] KVM: x86/mmu: Fix comment mentioning skip_4k On Wed, May 26, 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. > > Signed-off-by: David Matlack <dmatlack@google.com> Reviewed-by: Sean Christopherson <seanjc@google.com>
On Wed, May 26, 2021 at 10:29 AM Sean Christopherson <seanjc@google.com> wrote: > > Put version information in the subject, otherwise it's not always obvious which > patch you want to be accepted, e.g. > > [PATCH v2] KVM: x86/mmu: Fix comment mentioning skip_4k Got it. My thinking was that I changed the title of the patch so should omit the v2, but that doesn't really make sense. > > On Wed, May 26, 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. > > > > Signed-off-by: David Matlack <dmatlack@google.com> > > Reviewed-by: Sean Christopherson <seanjc@google.com>
On Wed, May 26, 2021, David Matlack wrote: > On Wed, May 26, 2021 at 10:29 AM Sean Christopherson <seanjc@google.com> wrote: > > > > Put version information in the subject, otherwise it's not always obvious which > > patch you want to be accepted, e.g. > > > > [PATCH v2] KVM: x86/mmu: Fix comment mentioning skip_4k > > Got it. My thinking was that I changed the title of the patch so > should omit the v2, but that doesn't really make sense. Ha, yeah, the version should get bumped even if a patch/series gets heavily rewritten. There are exceptions (though I'm struggling to think of a good example), but even then it's helpful to describe the relationship to any previous series. It's also customery to describe the changes between versions in the cover letter, or in the case of a one-off patch, in the part of the patch that git ignores. And my own personal preference is to also include lore links to previous versions, e.g. in this case I would do something like: v2: Reword comment to document min_level. [sean] v1: https://lkml.kernel.org/r/20210526163227.3113557-1-dmatlack@google.com Providing the explicit link in addition to the delta summaray makes it easy for reviewers to see the history and understand the context of _why_ changes were made. That's especially helpful for reviewers that didn't read/review earlier versions.
On Wed, May 26, 2021 at 11:51 AM Sean Christopherson <seanjc@google.com> wrote: > > On Wed, May 26, 2021, David Matlack wrote: > > On Wed, May 26, 2021 at 10:29 AM Sean Christopherson <seanjc@google.com> wrote: > > > > > > Put version information in the subject, otherwise it's not always obvious which > > > patch you want to be accepted, e.g. > > > > > > [PATCH v2] KVM: x86/mmu: Fix comment mentioning skip_4k > > > > Got it. My thinking was that I changed the title of the patch so > > should omit the v2, but that doesn't really make sense. > > Ha, yeah, the version should get bumped even if a patch/series gets heavily > rewritten. There are exceptions (though I'm struggling to think of a good > example), but even then it's helpful to describe the relationship to any > previous series. > > It's also customery to describe the changes between versions in the cover letter, > or in the case of a one-off patch, in the part of the patch that git ignores. > > And my own personal preference is to also include lore links to previous versions, > e.g. in this case I would do something like: > > v2: Reword comment to document min_level. [sean] > > v1: https://lkml.kernel.org/r/20210526163227.3113557-1-dmatlack@google.com > > Providing the explicit link in addition to the delta summaray makes it easy for > reviewers to see the history and understand the context of _why_ changes were > made. That's especially helpful for reviewers that didn't read/review earlier > versions. Great advice. Thanks Sean!
On 26/05/21 18:32, 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. > > Signed-off-by: David Matlack <dmatlack@google.com> > --- > arch/x86/kvm/mmu/tdp_mmu.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c > index 95eeb5ac6a8a..237317b1eddd 100644 > --- a/arch/x86/kvm/mmu/tdp_mmu.c > +++ b/arch/x86/kvm/mmu/tdp_mmu.c > @@ -1192,9 +1192,9 @@ 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. > - * Returns true if an SPTE has been changed and the TLBs need to be flushed. > + * Remove write access from all SPTEs at or above min_level that map 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, > gfn_t start, gfn_t end, int min_level) > Queued, thanks. Paolo
diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c index 95eeb5ac6a8a..237317b1eddd 100644 --- a/arch/x86/kvm/mmu/tdp_mmu.c +++ b/arch/x86/kvm/mmu/tdp_mmu.c @@ -1192,9 +1192,9 @@ 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. - * Returns true if an SPTE has been changed and the TLBs need to be flushed. + * Remove write access from all SPTEs at or above min_level that map 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, gfn_t start, gfn_t end, int min_level)
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. Signed-off-by: David Matlack <dmatlack@google.com> --- arch/x86/kvm/mmu/tdp_mmu.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)