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@xxxxxxxxxx> 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@xxxxxxxxxx> > > > > Reviewed-by: Ben Gardon <bgardon@xxxxxxxxxx> > > > > 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.
