Re: [PATCH 03/23] KVM: x86/mmu: Decompose kvm_mmu_get_page() into separate functions

[David Matlack <dmatlack@google.com>]

On Fri, Feb 18, 2022 at 5:25 PM Sean Christopherson <seanjc@google.com> wrote:
>
> On Thu, Feb 03, 2022, David Matlack wrote:
> > Decompose kvm_mmu_get_page() into separate helper functions to increase
> > readability and prepare for allocating shadow pages without a vcpu
> > pointer.
> >
> > Specifically, pull the guts of kvm_mmu_get_page() into 3 helper
> > functions:
> >
> > kvm_mmu_get_existing_sp_mabye_unsync() -
>
> Heh, this ain't Java.   Just add two underscores to whatever it's primary caller
> ends up being named; that succinctly documents the relationship _and_ suggests
> that there's some "danger" in using the inner helper.
>
> >   Walks the page hash checking for any existing mmu pages that match the
> >   given gfn and role. Does not attempt to synchronize the page if it is
> >   unsync.
> >
> > kvm_mmu_get_existing_sp() -
>
> Meh.  We should really be able to distill this down to something like
> kvm_mmu_find_sp().  I'm also tempted to say we go with shadow_page instead of
> "sp" for these helpers, so long as the line lengths don't get too brutal.  KVM
> uses "sp" and "spte" in lots of places, but I suspect it would be helpful to
> KVM newbies if the core routines actually spell out shadow_page, a la
> to_shadow_page().

s/get_existing/find/ sounds good to me.

I'll play around with s/sp/shadow_page/ but I suspect it will make the
line lengths quite long. But if I also replace "maybe_unsync" with
double-underscores it might work out.

>
> >   Gets an existing page from the page hash if it exists and guarantees
> >   the page, if one is returned, is synced.  Implemented as a thin wrapper
> >   around kvm_mmu_get_existing_page_mabye_unsync. Requres access to a vcpu
> >   pointer in order to sync the page.
> >
> > kvm_mmu_create_sp()
>
> Probably prefer s/create/alloc to match existing terminology for allocating roots.
> Though looking through the series, there's going to be a lot of juggling of names.
>
> It probably makes sense to figure out what names we want to end up with and then
> work back from there.  I'll be back next week for a proper bikeshed session. :-)

kvm_mmu_create_sp() is temporary anyway. It goes away after patch 6
and we just have kvm_mmu_alloc_sp() and kvm_mmu_init_sp().

I'll see what I can do about using kvm_mmu_alloc_sp() as the temporary
name, but the next patch renames kvm_mmu_alloc_page() to
kvm_mmu_alloc_sp() so it will take some juggling for sure.