On Fri, Feb 18, 2022 at 5:25 PM Sean Christopherson <seanjc@xxxxxxxxxx> 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.
