On Tue, Mar 15, 2022 at 2:04 AM Peter Xu <peterx@xxxxxxxxxx> wrote:
>
> On Fri, Mar 11, 2022 at 12:25:08AM +0000, David Matlack wrote:
> > Passing the memslot to kvm_mmu_new_shadow_page() avoids the need for the
> > vCPU pointer when write-protecting indirect 4k shadow pages. This moves
> > us closer to being able to create new shadow pages during VM ioctls for
> > eager page splitting, where there is not vCPU pointer.
> >
> > This change does not negatively impact "Populate memory time" for ept=Y
> > or ept=N configurations since kvm_vcpu_gfn_to_memslot() caches the last
> > use slot. So even though we now look up the slot more often, it is a
> > very cheap check.
> >
> > Opportunistically move the code to write-protect GFNs shadowed by
> > PG_LEVEL_4K shadow pages into account_shadowed() to reduce indentation
> > and consolidate the code. This also eliminates a memslot lookup.
> >
> > No functional change intended.
> >
> > Signed-off-by: David Matlack <dmatlack@xxxxxxxxxx>
> > ---
> > arch/x86/kvm/mmu/mmu.c | 23 ++++++++++++-----------
> > 1 file changed, 12 insertions(+), 11 deletions(-)
> >
> > diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> > index b6fb50e32291..519910938478 100644
> > --- a/arch/x86/kvm/mmu/mmu.c
> > +++ b/arch/x86/kvm/mmu/mmu.c
> > @@ -793,16 +793,14 @@ void kvm_mmu_gfn_allow_lpage(const struct kvm_memory_slot *slot, gfn_t gfn)
> > update_gfn_disallow_lpage_count(slot, gfn, -1);
> > }
> >
> > -static void account_shadowed(struct kvm *kvm, struct kvm_mmu_page *sp)
> > +static void account_shadowed(struct kvm *kvm,
> > + struct kvm_memory_slot *slot,
> > + struct kvm_mmu_page *sp)
> > {
> > - struct kvm_memslots *slots;
> > - struct kvm_memory_slot *slot;
> > gfn_t gfn;
> >
> > kvm->arch.indirect_shadow_pages++;
> > gfn = sp->gfn;
> > - slots = kvm_memslots_for_spte_role(kvm, sp->role);
> > - slot = __gfn_to_memslot(slots, gfn);
> >
> > /* the non-leaf shadow pages are keeping readonly. */
> > if (sp->role.level > PG_LEVEL_4K)
> > @@ -810,6 +808,9 @@ static void account_shadowed(struct kvm *kvm, struct kvm_mmu_page *sp)
> > KVM_PAGE_TRACK_WRITE);
> >
> > kvm_mmu_gfn_disallow_lpage(slot, gfn);
> > +
> > + if (kvm_mmu_slot_gfn_write_protect(kvm, slot, gfn, PG_LEVEL_4K))
> > + kvm_flush_remote_tlbs_with_address(kvm, gfn, 1);
>
> It's not immediately obvious in this diff, but when looking at the code
> yeah it looks right to just drop the 4K check..
Yeah it's a bit subtle but (as you probably noticed) account_shadowed()
returns early if the level is above PG_LEVEL_4K.
>
> I also never understood why we only write-track the >1 levels but only
> wr-protect the last level. It'll be great if there's quick answer from
> anyone.. even though it's probably unrelated to the patch.
>
> The change looks all correct:
>
> Reviewed-by: Peter Xu <peterx@xxxxxxxxxx>
>
> Thanks,
>
> --
> Peter Xu
>
