linux-kernel - Re: [PATCH 0/5] KVM: x86: Clean up rmap zap helpers

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 14 Jul 2022 17:27:27 +0000
From:   Sean Christopherson <seanjc@...gle.com>
To:     Paolo Bonzini <pbonzini@...hat.com>
Cc:     kvm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 0/5] KVM: x86: Clean up rmap zap helpers

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.