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
| ||
|
Date: Tue, 13 Dec 2022 16:12:17 -0800 From: Ben Gardon <bgardon@...gle.com> To: David Matlack <dmatlack@...gle.com> Cc: linux-kernel@...r.kernel.org, kvm@...r.kernel.org, Paolo Bonzini <pbonzini@...hat.com>, Peter Xu <peterx@...hat.com>, Sean Christopherson <seanjc@...gle.com>, Vipin Sharma <vipinsh@...gle.com> Subject: Re: [PATCH 2/7] KVM: x86/MMU: Move rmap_iterator to rmap.h On Fri, Dec 9, 2022 at 3:04 PM David Matlack <dmatlack@...gle.com> wrote: > > On Tue, Dec 06, 2022 at 05:35:56PM +0000, Ben Gardon wrote: > > In continuing to factor the rmap out of mmu.c, move the rmap_iterator > > and associated functions and macros into rmap.(c|h). > > > > No functional change intended. > > > > Signed-off-by: Ben Gardon <bgardon@...gle.com> > > --- > > arch/x86/kvm/mmu/mmu.c | 76 ----------------------------------------- > > arch/x86/kvm/mmu/rmap.c | 61 +++++++++++++++++++++++++++++++++ > > arch/x86/kvm/mmu/rmap.h | 18 ++++++++++ > > 3 files changed, 79 insertions(+), 76 deletions(-) > > > [...] > > diff --git a/arch/x86/kvm/mmu/rmap.h b/arch/x86/kvm/mmu/rmap.h > > index 059765b6e066..13b265f3a95e 100644 > > --- a/arch/x86/kvm/mmu/rmap.h > > +++ b/arch/x86/kvm/mmu/rmap.h > > @@ -31,4 +31,22 @@ void free_pte_list_desc(struct pte_list_desc *pte_list_desc); > > void pte_list_remove(u64 *spte, struct kvm_rmap_head *rmap_head); > > unsigned int pte_list_count(struct kvm_rmap_head *rmap_head); > > > > +/* > > + * Used by the following functions to iterate through the sptes linked by a > > + * rmap. All fields are private and not assumed to be used outside. > > + */ > > +struct rmap_iterator { > > + /* private fields */ > > + struct pte_list_desc *desc; /* holds the sptep if not NULL */ > > + int pos; /* index of the sptep */ > > +}; > > + > > +u64 *rmap_get_first(struct kvm_rmap_head *rmap_head, > > + struct rmap_iterator *iter); > > +u64 *rmap_get_next(struct rmap_iterator *iter); > > + > > +#define for_each_rmap_spte(_rmap_head_, _iter_, _spte_) \ > > + for (_spte_ = rmap_get_first(_rmap_head_, _iter_); \ > > + _spte_; _spte_ = rmap_get_next(_iter_)) > > + > > I always found these function names and kvm_rmap_head confusing since > they are about iterating through the pte_list_desc data structure. The > rmap (gfn -> list of sptes) is a specific application of the > pte_list_desc structure, but not the only application. There's also > parent_ptes in struct kvm_mmu_page, which is not an rmap, just a plain > old list of ptes. > > While you are refactoring this code, what do you think about doing the > following renames? > > struct kvm_rmap_head -> struct pte_list_head > struct rmap_iterator -> struct pte_list_iterator > rmap_get_first() -> pte_list_get_first() > rmap_get_next() -> pte_list_get_next() > for_each_rmap_spte() -> for_each_pte_list_entry() > > Then we can reserve the term "rmap" just for the actual rmap > (slot->arch.rmap), and code that deals with sp->parent_ptes will become > a lot more clear IMO (because it will not longer mention rmap). > > e.g. We go from this: > > struct rmap_iterator iter; > u64 *sptep; > > for_each_rmap_spte(&sp->parent_ptes, &iter, sptep) { > ... > } > > To this: > > struct pte_list_iterator iter; > u64 *sptep; > > for_each_pte_list_entry(&sp->parent_ptes, &iter, sptep) { > ... > } I like this suggestion, and I do think it'll make things more readable. It's going to be a huge patch to rename all the instances of kvm_rmap_head, but it's probably worth it.
Powered by blists - more mailing lists