On Fri, Sep 25, 2020 at 02:22:42PM -0700, Ben Gardon wrote: > diff --git a/arch/x86/kvm/mmu/tdp_iter.c b/arch/x86/kvm/mmu/tdp_iter.c > new file mode 100644 > index 0000000000000..ee90d62d2a9b1 > --- /dev/null > +++ b/arch/x86/kvm/mmu/tdp_iter.c > @@ -0,0 +1,163 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > + > +#include "mmu_internal.h" > +#include "tdp_iter.h" > + > +/* > + * Recalculates the pointer to the SPTE for the current GFN and level and > + * reread the SPTE. > + */ > +static void tdp_iter_refresh_sptep(struct tdp_iter *iter) > +{ > + iter->sptep = iter->pt_path[iter->level - 1] + > + SHADOW_PT_INDEX(iter->gfn << PAGE_SHIFT, iter->level); > + iter->old_spte = READ_ONCE(*iter->sptep); > +} > + > +/* > + * Sets a TDP iterator to walk a pre-order traversal of the paging structure > + * rooted at root_pt, starting with the walk to translate goal_gfn. > + */ > +void tdp_iter_start(struct tdp_iter *iter, u64 *root_pt, int root_level, > + gfn_t goal_gfn) > +{ > + WARN_ON(root_level < 1); > + WARN_ON(root_level > PT64_ROOT_MAX_LEVEL); > + > + iter->goal_gfn = goal_gfn; > + iter->root_level = root_level; > + iter->level = root_level; > + iter->pt_path[iter->level - 1] = root_pt; > + > + iter->gfn = iter->goal_gfn - > + (iter->goal_gfn % KVM_PAGES_PER_HPAGE(iter->level)); Maybe use the params, if only to avoid the line wrap? iter->gfn = goal_gfn - (goal_gfn % KVM_PAGES_PER_HPAGE(root_level)); Actually, peeking further into the file, this calculation is repeated in both try_step_up and try_step_down, probably worth adding a helper of some form. > + tdp_iter_refresh_sptep(iter); > + > + iter->valid = true; > +} > + > +/* > + * Given an SPTE and its level, returns a pointer containing the host virtual > + * address of the child page table referenced by the SPTE. Returns null if > + * there is no such entry. > + */ > +u64 *spte_to_child_pt(u64 spte, int level) > +{ > + u64 *pt; > + /* There's no child entry if this entry isn't present */ > + if (!is_shadow_present_pte(spte)) > + return NULL; > + > + /* There is no child page table if this is a leaf entry. */ > + if (is_last_spte(spte, level)) > + return NULL; I'd collapse the checks and their comments. > + > + pt = (u64 *)__va(spte_to_pfn(spte) << PAGE_SHIFT); > + return pt; No need for the local variable or the explicit cast. /* There's no child if this entry is non-present or a leaf entry. */ if (!is_shadow_present_pte(spte) || is_last_spte(spte, level)) return NULL; return __va(spte_to_pfn(spte) << PAGE_SHIFT); > +} ... > +void tdp_iter_next(struct tdp_iter *iter) > +{ > + bool done; > + > + done = try_step_down(iter); > + if (done) > + return; > + > + done = try_step_side(iter); > + while (!done) { > + if (!try_step_up(iter)) { > + iter->valid = false; > + break; > + } > + done = try_step_side(iter); > + } At the risk of being too clever: bool done; if (try_step_down(iter)) return; do { done = try_step_side(iter); } while (!done && try_step_up(iter)); iter->valid = done; > +}