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".
