lkml.org 
[lkml]   [2022]   [Jul]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 04/12] KVM: X86/MMU: Remove mmu_pages_clear_parents()
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 <jiangshan.ljs@antgroup.com>
>
> 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 <seanjc@google.com>

>
> 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 <jiangshan.ljs@antgroup.com>
> ---

\
 
 \ /
  Last update: 2022-07-15 01:17    [W:0.658 / U:0.336 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site