From: Lai Jiangshan <[email protected]>
mmu_unsync_walk() is designed to be workable in a pagetable which has
unsync child bits set in the shadow pages in the pagetable but without
any unsync shadow pages.
This can be resulted when the unsync shadow pages of a pagetable
can be walked from other pagetables and have been synced or zapped
when other pagetables are synced or zapped.
So mmu_pages_clear_parents() is not required even when the callers of
mmu_unsync_walk() zap or sync the pagetable.
So remove mmu_pages_clear_parents() and the child bits can be cleared in
the next call of mmu_unsync_walk() in one go.
Removing mmu_pages_clear_parents() allows for further simplifying
mmu_unsync_walk() including removing the struct mmu_page_path since
the function is the only user of it.
Signed-off-by: Lai Jiangshan <[email protected]>
---
arch/x86/kvm/mmu/mmu.c | 19 -------------------
1 file changed, 19 deletions(-)
diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
index cc0207e26f6e..f35fd5c59c38 100644
--- a/arch/x86/kvm/mmu/mmu.c
+++ b/arch/x86/kvm/mmu/mmu.c
@@ -1948,23 +1948,6 @@ static int mmu_pages_first(struct kvm_mmu_pages *pvec,
return mmu_pages_next(pvec, parents, 0);
}
-static void mmu_pages_clear_parents(struct mmu_page_path *parents)
-{
- struct kvm_mmu_page *sp;
- unsigned int level = 0;
-
- do {
- unsigned int idx = parents->idx[level];
- sp = parents->parent[level];
- if (!sp)
- return;
-
- WARN_ON(idx == INVALID_INDEX);
- clear_unsync_child_bit(sp, idx);
- level++;
- } while (!sp->unsync_children);
-}
-
static int mmu_sync_children(struct kvm_vcpu *vcpu,
struct kvm_mmu_page *parent, bool can_yield)
{
@@ -1989,7 +1972,6 @@ static int mmu_sync_children(struct kvm_vcpu *vcpu,
for_each_sp(pages, sp, parents, i) {
kvm_mmu_page_clear_unsync(vcpu->kvm, sp);
flush |= kvm_sync_page(vcpu, sp, &invalid_list) > 0;
- mmu_pages_clear_parents(&parents);
}
if (need_resched() || rwlock_needbreak(&vcpu->kvm->mmu_lock)) {
kvm_mmu_remote_flush_or_zap(vcpu->kvm, &invalid_list, flush);
@@ -2298,7 +2280,6 @@ static int mmu_zap_unsync_children(struct kvm *kvm,
for_each_sp(pages, sp, parents, i) {
kvm_mmu_prepare_zap_page(kvm, sp, invalid_list);
- mmu_pages_clear_parents(&parents);
zapped++;
}
}
--
2.19.1.6.gb485710b
For the shortlog, I really want to capture the net effect. It took me a lot of
staring and reading (and hopefully not misreading) to figure out that this is a
glorified nop.
KVM: x86/mmu: Update unsync children metadata via recursion, not bottom-up walk
On Sun, Jun 05, 2022, Lai Jiangshan wrote:
> From: Lai Jiangshan <[email protected]>
>
> mmu_unsync_walk() is designed to be workable in a pagetable which has
> unsync child bits set in the shadow pages in the pagetable but without
> any unsync shadow pages.
>
> This can be resulted when the unsync shadow pages of a pagetable
> can be walked from other pagetables and have been synced or zapped
> when other pagetables are synced or zapped.
>
> So mmu_pages_clear_parents() is not required even when the callers of
> mmu_unsync_walk() zap or sync the pagetable.
There's one other critical piece that it took me a quite some time to suss out
from the code: the @parent passed to mmu_sync_children() _is_ updated because
mmu_sync_children() loops on mmu_unsync_walk(). It's only the parents of @parent
that are not updated, but they weren't updated anyways because mmu_pages_clear_parents()
doesn't operate on the parents of @parent.
> So remove mmu_pages_clear_parents() and the child bits can be cleared in
> the next call of mmu_unsync_walk() in one go.
Ah, I missed (over and over) that the "next call" is the one right mmu_sync_children()
and mmu_unsync_walk(), not a future call.
Because I kept losing track of which pagetable was which, how about this for
a changelog?
When syncing a shadow page with unsync children, do not update the
"unsync children" metadata from the bottom up, and instead defer the
update to the next "iteration" of mmu_unsync_walk() (all users of
mmu_unsync_walk() loop until it returns "no unsync children").
mmu_unsync_walk() is designed to handle the scenario where a shadow page
has a false positive on having unsync children, i.e. unsync_children can
be elevated without any child shadow pages actually being unsync.
Such a scenario already occurs when a child is synced or zapped by a
different walk of the page tables, i.e. with a different set of parents,
as unmarking parents is done only for the current walk.
Note, mmu_pages_clear_parents() doesn't update parents of @parent, so
there's no change in functionality from that perspective.
Removing mmu_pages_clear_parents() allows for further simplifying
mmu_unsync_walk(), including removing the struct mmu_page_path since
mmu_pages_clear_parents() was the only the function is the only user of it.
With a cleaned up shortlog+changelog, and assuming I didn't misread everything...
Reviewed-by: Sean Christopherson <[email protected]>
>
> Removing mmu_pages_clear_parents() allows for further simplifying
> mmu_unsync_walk() including removing the struct mmu_page_path since
> the function is the only user of it.
>
> Signed-off-by: Lai Jiangshan <[email protected]>
> ---