Clean up the rmap helpers (mostly renames) to yield a more coherent set of
APIs, and to purge the irritating and inconsistent "rmapp" (p is for pointer)
nomenclature.

Patch 1 is a tangentially related fix for a benign bug.

Sean Christopherson (5):
  KVM: x86/mmu: Return a u64 (the old SPTE) from
    mmu_spte_clear_track_bits()
  KVM: x86/mmu: Rename rmap zap helpers to better show relationships
  KVM: x86/mmu: Remove underscores from __pte_list_remove()
  KVM: x86/mmu: Use innermost rmap zap helper when recycling rmaps
  KVM: x86/mmu: Drop the "p is for pointer" from rmap helpers

 arch/x86/kvm/mmu/mmu.c | 73 +++++++++++++++++++++---------------------
 1 file changed, 36 insertions(+), 37 deletions(-)


base-commit: b9b71f43683ae9d76b0989249607bbe8c9eb6c5c
-- 
2.37.0.144.g8ac04bfd2-goog

On 7/12/22 03:55, Sean Christopherson wrote:
> Clean up the rmap helpers (mostly renames) to yield a more coherent set of
> APIs, and to purge the irritating and inconsistent "rmapp" (p is for pointer)
> nomenclature.
> 
> Patch 1 is a tangentially related fix for a benign bug.
> 
> Sean Christopherson (5):
>    KVM: x86/mmu: Return a u64 (the old SPTE) from
>      mmu_spte_clear_track_bits()
>    KVM: x86/mmu: Rename rmap zap helpers to better show relationships
>    KVM: x86/mmu: Remove underscores from __pte_list_remove()
>    KVM: x86/mmu: Use innermost rmap zap helper when recycling rmaps
>    KVM: x86/mmu: Drop the "p is for pointer" from rmap helpers
> 
>   arch/x86/kvm/mmu/mmu.c | 73 +++++++++++++++++++++---------------------
>   1 file changed, 36 insertions(+), 37 deletions(-)
> 
> 
> base-commit: b9b71f43683ae9d76b0989249607bbe8c9eb6c5c

I'm not sure I dig the ____, I'll take a closer look tomorrow or next 
week since it's dinner time here.

I'm pushing what you and I collected over the past 3 weeks, for now I 
only checked that it compiles.

Paolo

On Thu, Jul 14, 2022, Paolo Bonzini wrote:
> On 7/12/22 03:55, Sean Christopherson wrote:
> > Clean up the rmap helpers (mostly renames) to yield a more coherent set of
> > APIs, and to purge the irritating and inconsistent "rmapp" (p is for pointer)
> > nomenclature.
> > 
> > Patch 1 is a tangentially related fix for a benign bug.
> > 
> > Sean Christopherson (5):
> >    KVM: x86/mmu: Return a u64 (the old SPTE) from
> >      mmu_spte_clear_track_bits()
> >    KVM: x86/mmu: Rename rmap zap helpers to better show relationships
> >    KVM: x86/mmu: Remove underscores from __pte_list_remove()
> >    KVM: x86/mmu: Use innermost rmap zap helper when recycling rmaps
> >    KVM: x86/mmu: Drop the "p is for pointer" from rmap helpers
> > 
> >   arch/x86/kvm/mmu/mmu.c | 73 +++++++++++++++++++++---------------------
> >   1 file changed, 36 insertions(+), 37 deletions(-)
> > 
> > 
> > base-commit: b9b71f43683ae9d76b0989249607bbe8c9eb6c5c
> 
> I'm not sure I dig the ____, I'll take a closer look tomorrow or next week
> since it's dinner time here.

Yeah, I'm not a fan of it either.  And rereading things, my proposed names also
create an inconsistency; the zap path is the only user of kvm_handle_gfn_range()
that uses a plural "rmaps".

  $ git grep kvm_handle_gfn_range
  arch/x86/kvm/mmu/mmu.c:static __always_inline bool kvm_handle_gfn_range(struct kvm *kvm,
  arch/x86/kvm/mmu/mmu.c:         flush = kvm_handle_gfn_range(kvm, range, kvm_zap_rmaps);
  arch/x86/kvm/mmu/mmu.c:         flush = kvm_handle_gfn_range(kvm, range, kvm_set_pte_rmap);
  arch/x86/kvm/mmu/mmu.c:         young = kvm_handle_gfn_range(kvm, range, kvm_age_rmap);
  arch/x86/kvm/mmu/mmu.c:         young = kvm_handle_gfn_range(kvm, range, kvm_test_age_rmap);

Make "rmaps" plural is probably a mistake.  The helper zaps multiple SPTEs for a
given rmap list, but from a certain point of view it's just a single "rmap".

What about:

  kvm_zap_rmapp => kvm_zap_rmap    // to align with kvm_handle_gfn_range() usage
  kvm_zap_rmap  => __kvm_zap_rmap  // to pair with kvm_zap_rmap()
  
and

  pte_list_remove  => kvm_zap_one_rmap_spte  
  pte_list_destroy => kvm_zap_all_rmap_sptes

That will yield a better series too, as I can move patch 5 to be patch 2, then
split what was patch 2 (the rename) into separate patches to first align kvm_zap_rmap()
and __kvm_zap_rmap(), and then rename the pte_list_remove/destroy helpers.

On Thu, Jul 14, 2022, Sean Christopherson wrote:
> On Thu, Jul 14, 2022, Paolo Bonzini wrote:
> > On 7/12/22 03:55, Sean Christopherson wrote:
> > > Clean up the rmap helpers (mostly renames) to yield a more coherent set of
> > > APIs, and to purge the irritating and inconsistent "rmapp" (p is for pointer)
> > > nomenclature.
> > > 
> > > Patch 1 is a tangentially related fix for a benign bug.
> > > 
> > > Sean Christopherson (5):
> > >    KVM: x86/mmu: Return a u64 (the old SPTE) from
> > >      mmu_spte_clear_track_bits()
> > >    KVM: x86/mmu: Rename rmap zap helpers to better show relationships
> > >    KVM: x86/mmu: Remove underscores from __pte_list_remove()
> > >    KVM: x86/mmu: Use innermost rmap zap helper when recycling rmaps
> > >    KVM: x86/mmu: Drop the "p is for pointer" from rmap helpers
> > > 
> > >   arch/x86/kvm/mmu/mmu.c | 73 +++++++++++++++++++++---------------------
> > >   1 file changed, 36 insertions(+), 37 deletions(-)
> > > 
> > > 
> > > base-commit: b9b71f43683ae9d76b0989249607bbe8c9eb6c5c
> > 
> > I'm not sure I dig the ____, I'll take a closer look tomorrow or next week
> > since it's dinner time here.
> 
> Yeah, I'm not a fan of it either.  And rereading things, my proposed names also
> create an inconsistency; the zap path is the only user of kvm_handle_gfn_range()
> that uses a plural "rmaps".
> 
>   $ git grep kvm_handle_gfn_range
>   arch/x86/kvm/mmu/mmu.c:static __always_inline bool kvm_handle_gfn_range(struct kvm *kvm,
>   arch/x86/kvm/mmu/mmu.c:         flush = kvm_handle_gfn_range(kvm, range, kvm_zap_rmaps);
>   arch/x86/kvm/mmu/mmu.c:         flush = kvm_handle_gfn_range(kvm, range, kvm_set_pte_rmap);
>   arch/x86/kvm/mmu/mmu.c:         young = kvm_handle_gfn_range(kvm, range, kvm_age_rmap);
>   arch/x86/kvm/mmu/mmu.c:         young = kvm_handle_gfn_range(kvm, range, kvm_test_age_rmap);
> 
> Make "rmaps" plural is probably a mistake.  The helper zaps multiple SPTEs for a
> given rmap list, but from a certain point of view it's just a single "rmap".
> 
> What about:
> 
>   kvm_zap_rmapp => kvm_zap_rmap    // to align with kvm_handle_gfn_range() usage
>   kvm_zap_rmap  => __kvm_zap_rmap  // to pair with kvm_zap_rmap()
>   
> and
> 
>   pte_list_remove  => kvm_zap_one_rmap_spte  
>   pte_list_destroy => kvm_zap_all_rmap_sptes
> 
> That will yield a better series too, as I can move patch 5 to be patch 2, then
> split what was patch 2 (the rename) into separate patches to first align kvm_zap_rmap()
> and __kvm_zap_rmap(), and then rename the pte_list_remove/destroy helpers.

And also:

  __kvm_zap_rmaps => kvm_rmap_zap_gfn_range

instead of renaming it to __kvm_zap_gfn_range() to make it clear that it zaps only
rmap-based MMUs, to align with kvm_rmap_zap_collapsible_sptes(), and to avoid the
plural "rmaps".

On 7/14/22 19:27, Sean Christopherson wrote:
> 
>    pte_list_remove  => kvm_zap_one_rmap_spte
>    pte_list_destroy => kvm_zap_all_rmap_sptes
> 
> That will yield a better series too, as I can move patch 5 to be patch 2, then
> split what was patch 2 (the rename) into separate patches to first align kvm_zap_rmap()
> and __kvm_zap_rmap(), and then rename the pte_list_remove/destroy helpers.

Yeah, sounds good (I also was looking into moving patch 5 and possibly 
even patch 4 more towards the beginning).

Paolo