Re: [PATCH v2 3/6] KVM: x86/mmu: Make walk_shadow_page_lockless_{begin,end} interoperate with the TDP MMU

[Sean Christopherson <seanjc@google.com>]

On Mon, Jul 12, 2021, David Matlack wrote:
> On Mon, Jul 12, 2021 at 10:02:31AM -0700, Ben Gardon wrote:
> > On Wed, Jun 30, 2021 at 2:48 PM David Matlack <dmatlack@google.com> wrote:
> > >
> > > Acquire the RCU read lock in walk_shadow_page_lockless_begin and release
> > > it in walk_shadow_page_lockless_end when the TDP MMU is enabled.  This
> > > should not introduce any functional changes but is used in the following
> > > commit to make fast_page_fault interoperate with the TDP MMU.
> > >
> > > Signed-off-by: David Matlack <dmatlack@google.com>
> > 
> > Reviewed-by: Ben Gardon <bgardon@google.com>
> > 
> > This as I understand this, we're just lifting the rcu_lock/unlock out
> > of kvm_tdp_mmu_get_walk, and then putting all the TDP MMU specific
> > stuff down a level under walk_shadow_page_lockless_begin/end and
> > get_walk.
> > 
> > Instead of moving kvm_tdp_mmu_get_walk_lockless into get_walk, it
> > could also be open-coded as:
> > 
> > walk_shadow_page_lockless_begin
> >  if (is_tdp_mmu(vcpu->arch.mmu))
> >                leaf = kvm_tdp_mmu_get_walk(vcpu, addr, sptes, &root);
> >  else
> >                leaf = get_walk(vcpu, addr, sptes, &root);
> > walk_shadow_page_lockless_end
> > 
> > in get_mmio_spte, since get_walk isn't used anywhere else. Then
> > walk_shadow_page_lockless_begin/end could also be moved up out of
> > get_walk instead of having to add a goto to that function.
> > I don't have a strong preference either way, but the above feels like
> > a slightly simpler refactor.
> 
> I don't have a strong preference either way as well. I'd be happy to
> switch to your suggestion in v3.

I vote for Ben's suggestion.  As is, I think it would be too easy to overlook the
TDP MMU path in get_walk() and focus only on the for-loop.