[2/7] KVM: x86/MMU: Move rmap_iterator to rmap.h
Commit Message
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@google.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(-)
Comments
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@google.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) {
...
}
On Fri, Dec 9, 2022 at 3:04 PM David Matlack <dmatlack@google.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@google.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.
On Tue, Dec 13, 2022, Ben Gardon wrote:
> On Fri, Dec 9, 2022 at 3:04 PM David Matlack <dmatlack@google.com> wrote:
> >
> > > +/*
> > > + * 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
Heh, you definitely aren't the only one.
> > 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()
I would strongly prefer to keep "spte" in this one regardless of what other naming
changes we do (see below). Maybe just for_each_spte()? IMO, "pte_list_entry"
unnecessarily obfuscates that it's a list of SPTEs.
> > 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.
I generally like this idea too, but tying into my above comment, before jumping
in I think we should figure out what end state we want, i.e. get the bikeshedding
out of the way now to hopefully avoid dragging out a series while various things
get nitpicked.
E.g. if we if we just rename the structs and their macros, then we'll end up with
things like
static bool slot_rmap_write_protect(struct kvm *kvm,
struct pte_list_head *rmap_head,
const struct kvm_memory_slot *slot)
{
return rmap_write_protect(rmap_head, false);
}
which isn't terrible, but there's still opportunity for cleanup, e.g.
rmap_write_protect() could easily be sptes_write_protect() or write_protect_sptes().
That will generate a naming conflict of sorts with pte_list_head if we don't also
rename that to spte_list_head. And I think capturing that it's a list of SPTEs and
not guest PTEs will be helpful in general.
And if we rename pte_list_head, then we might as well commit 100% and use consisnent
nomenclature across the board, e.g. end up with
static bool sptes_clear_dirty(struct kvm *kvm, struct sptes_list_head *head,
const struct kvm_memory_slot *slot)
{
u64 *sptep;
struct spte_list_iterator iter;
bool flush = false;
for_each_spte(head, &iter, sptep) {
if (spte_ad_need_write_protect(*sptep))
flush |= spte_wrprot_for_clear_dirty(sptep);
else
flush |= spte_clear_dirty(sptep);
}
return flush;
}
versus the current
static bool __rmap_clear_dirty(struct kvm *kvm, struct kvm_rmap_head *rmap_head,
const struct kvm_memory_slot *slot)
{
u64 *sptep;
struct rmap_iterator iter;
bool flush = false;
for_each_rmap_spte(rmap_head, &iter, sptep)
if (spte_ad_need_write_protect(*sptep))
flush |= spte_wrprot_for_clear_dirty(sptep);
else
flush |= spte_clear_dirty(sptep);
return flush;
}
On Tue, Dec 13, 2022 at 4:59 PM Sean Christopherson <seanjc@google.com> wrote:
>
> On Tue, Dec 13, 2022, Ben Gardon wrote:
> > On Fri, Dec 9, 2022 at 3:04 PM David Matlack <dmatlack@google.com> wrote:
> > >
> > > > +/*
> > > > + * 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
>
> Heh, you definitely aren't the only one.
>
> > > 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()
>
> I would strongly prefer to keep "spte" in this one regardless of what other naming
> changes we do (see below). Maybe just for_each_spte()? IMO, "pte_list_entry"
> unnecessarily obfuscates that it's a list of SPTEs.
>
> > > 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.
>
> I generally like this idea too, but tying into my above comment, before jumping
> in I think we should figure out what end state we want, i.e. get the bikeshedding
> out of the way now to hopefully avoid dragging out a series while various things
> get nitpicked.
>
> E.g. if we if we just rename the structs and their macros, then we'll end up with
> things like
>
> static bool slot_rmap_write_protect(struct kvm *kvm,
> struct pte_list_head *rmap_head,
> const struct kvm_memory_slot *slot)
> {
> return rmap_write_protect(rmap_head, false);
> }
>
> which isn't terrible, but there's still opportunity for cleanup, e.g.
> rmap_write_protect() could easily be sptes_write_protect() or write_protect_sptes().
>
> That will generate a naming conflict of sorts with pte_list_head if we don't also
> rename that to spte_list_head. And I think capturing that it's a list of SPTEs and
> not guest PTEs will be helpful in general.
>
> And if we rename pte_list_head, then we might as well commit 100% and use consisnent
> nomenclature across the board, e.g. end up with
>
> static bool sptes_clear_dirty(struct kvm *kvm, struct sptes_list_head *head,
> const struct kvm_memory_slot *slot)
> {
> u64 *sptep;
> struct spte_list_iterator iter;
> bool flush = false;
>
> for_each_spte(head, &iter, sptep) {
> if (spte_ad_need_write_protect(*sptep))
> flush |= spte_wrprot_for_clear_dirty(sptep);
> else
> flush |= spte_clear_dirty(sptep);
> }
>
> return flush;
> }
>
> versus the current
>
> static bool __rmap_clear_dirty(struct kvm *kvm, struct kvm_rmap_head *rmap_head,
> const struct kvm_memory_slot *slot)
> {
> u64 *sptep;
> struct rmap_iterator iter;
> bool flush = false;
>
> for_each_rmap_spte(rmap_head, &iter, sptep)
> if (spte_ad_need_write_protect(*sptep))
> flush |= spte_wrprot_for_clear_dirty(sptep);
> else
> flush |= spte_clear_dirty(sptep);
>
> return flush;
> }
I'd be happy to see some consistent SPTE-based naming in the Shadow
MMU and more or less get rid of the rmap naming scheme. Once you
change to spte_list_head or whatever, the use of the actual rmap (an
array of spte_list_heads) becomes super narrow.
Given the potential for enormous scope creep on what's already going
to be a long series, I'm inclined to split this work into two parts:
1. Move code from mmu.c to shadow_mmu.c with minimal cleanups /
refactors / renames; just move the code
2. Clean up naming conventions: make the functions exported in
shadow_mmu.h consistent, get rid of the whole rmap naming scheme, etc.
That way git-blame will preserve context around the renames /
refactors which would be obfuscated if we did 2 before 1, and we can
reduce merge conflicts.
On Wed, Dec 14, 2022, Ben Gardon wrote:
> On Tue, Dec 13, 2022 at 4:59 PM Sean Christopherson <seanjc@google.com> wrote:
> > And if we rename pte_list_head, then we might as well commit 100% and use consisnent
> > nomenclature across the board, e.g. end up with
...
> I'd be happy to see some consistent SPTE-based naming in the Shadow
> MMU and more or less get rid of the rmap naming scheme. Once you
> change to spte_list_head or whatever, the use of the actual rmap (an
> array of spte_list_heads) becomes super narrow.
Yeah. And at least for me, the more literal "walk a list of SPTEs" is much
easier for me to wrap my head around than "walk rmaps".
> Given the potential for enormous scope creep on what's already going
> to be a long series, I'm inclined to split this work into two parts:
> 1. Move code from mmu.c to shadow_mmu.c with minimal cleanups /
> refactors / renames; just move the code
> 2. Clean up naming conventions: make the functions exported in
> shadow_mmu.h consistent, get rid of the whole rmap naming scheme, etc.
>
> That way git-blame will preserve context around the renames /
> refactors which would be obfuscated if we did 2 before 1,
+1
> and we can reduce merge conflicts.
That might be wishful thinking ;-)
One thought for the rename would be to gather all the reviews and feedback, and
then wait to send the final version until shortly before the merge window, i.e.
wait for everything else to land so that only future development gets affected.
That would give Paolo and I a bit of extra motiviation to get the x86 queue
solidified sooner than later too...
@@ -932,82 +932,6 @@ static void rmap_remove(struct kvm *kvm, u64 *spte)
pte_list_remove(spte, 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 */
-};
-
-/*
- * Iteration must be started by this function. This should also be used after
- * removing/dropping sptes from the rmap link because in such cases the
- * information in the iterator may not be valid.
- *
- * Returns sptep if found, NULL otherwise.
- */
-static u64 *rmap_get_first(struct kvm_rmap_head *rmap_head,
- struct rmap_iterator *iter)
-{
- u64 *sptep;
-
- if (!rmap_head->val)
- return NULL;
-
- if (!(rmap_head->val & 1)) {
- iter->desc = NULL;
- sptep = (u64 *)rmap_head->val;
- goto out;
- }
-
- iter->desc = (struct pte_list_desc *)(rmap_head->val & ~1ul);
- iter->pos = 0;
- sptep = iter->desc->sptes[iter->pos];
-out:
- BUG_ON(!is_shadow_present_pte(*sptep));
- return sptep;
-}
-
-/*
- * Must be used with a valid iterator: e.g. after rmap_get_first().
- *
- * Returns sptep if found, NULL otherwise.
- */
-static u64 *rmap_get_next(struct rmap_iterator *iter)
-{
- u64 *sptep;
-
- if (iter->desc) {
- if (iter->pos < PTE_LIST_EXT - 1) {
- ++iter->pos;
- sptep = iter->desc->sptes[iter->pos];
- if (sptep)
- goto out;
- }
-
- iter->desc = iter->desc->more;
-
- if (iter->desc) {
- iter->pos = 0;
- /* desc->sptes[0] cannot be NULL */
- sptep = iter->desc->sptes[iter->pos];
- goto out;
- }
- }
-
- return NULL;
-out:
- BUG_ON(!is_shadow_present_pte(*sptep));
- return sptep;
-}
-
-#define for_each_rmap_spte(_rmap_head_, _iter_, _spte_) \
- for (_spte_ = rmap_get_first(_rmap_head_, _iter_); \
- _spte_; _spte_ = rmap_get_next(_iter_))
-
static void drop_spte(struct kvm *kvm, u64 *sptep)
{
u64 old_spte = mmu_spte_clear_track_bits(kvm, sptep);
@@ -139,3 +139,64 @@ unsigned int pte_list_count(struct kvm_rmap_head *rmap_head)
return count;
}
+/*
+ * Iteration must be started by this function. This should also be used after
+ * removing/dropping sptes from the rmap link because in such cases the
+ * information in the iterator may not be valid.
+ *
+ * Returns sptep if found, NULL otherwise.
+ */
+u64 *rmap_get_first(struct kvm_rmap_head *rmap_head, struct rmap_iterator *iter)
+{
+ u64 *sptep;
+
+ if (!rmap_head->val)
+ return NULL;
+
+ if (!(rmap_head->val & 1)) {
+ iter->desc = NULL;
+ sptep = (u64 *)rmap_head->val;
+ goto out;
+ }
+
+ iter->desc = (struct pte_list_desc *)(rmap_head->val & ~1ul);
+ iter->pos = 0;
+ sptep = iter->desc->sptes[iter->pos];
+out:
+ BUG_ON(!is_shadow_present_pte(*sptep));
+ return sptep;
+}
+
+/*
+ * Must be used with a valid iterator: e.g. after rmap_get_first().
+ *
+ * Returns sptep if found, NULL otherwise.
+ */
+u64 *rmap_get_next(struct rmap_iterator *iter)
+{
+ u64 *sptep;
+
+ if (iter->desc) {
+ if (iter->pos < PTE_LIST_EXT - 1) {
+ ++iter->pos;
+ sptep = iter->desc->sptes[iter->pos];
+ if (sptep)
+ goto out;
+ }
+
+ iter->desc = iter->desc->more;
+
+ if (iter->desc) {
+ iter->pos = 0;
+ /* desc->sptes[0] cannot be NULL */
+ sptep = iter->desc->sptes[iter->pos];
+ goto out;
+ }
+ }
+
+ return NULL;
+out:
+ BUG_ON(!is_shadow_present_pte(*sptep));
+ return sptep;
+}
+
@@ -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_))
+
#endif /* __KVM_X86_MMU_RMAP_H */