On Mon, Feb 28, 2022 at 12:47 PM Ben Gardon <bgardon@xxxxxxxxxx> wrote:
>
> On Wed, Feb 2, 2022 at 5:02 PM David Matlack <dmatlack@xxxxxxxxxx> wrote:
> >
> > drop_large_spte() drops a large SPTE if it exists and then flushes TLBs.
> > Its helper function, __drop_large_spte(), does the drop without the
> > flush. This difference is not obvious from the name.
> >
> > To make the code more readable, pass an explicit flush parameter. Also
> > replace the vCPU pointer with a KVM pointer so we can get rid of the
> > double-underscore helper function.
> >
> > This is also in preparation for a future commit that will conditionally
> > flush after dropping a large SPTE.
> >
> > No functional change intended.
> >
> > Signed-off-by: David Matlack <dmatlack@xxxxxxxxxx>
> > ---
> > arch/x86/kvm/mmu/mmu.c | 25 +++++++++++--------------
> > arch/x86/kvm/mmu/paging_tmpl.h | 4 ++--
> > 2 files changed, 13 insertions(+), 16 deletions(-)
> >
> > diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> > index 99ad7cc8683f..2d47a54e62a5 100644
> > --- a/arch/x86/kvm/mmu/mmu.c
> > +++ b/arch/x86/kvm/mmu/mmu.c
> > @@ -1162,23 +1162,20 @@ static void drop_spte(struct kvm *kvm, u64 *sptep)
> > }
> >
> >
> > -static bool __drop_large_spte(struct kvm *kvm, u64 *sptep)
> > +static void drop_large_spte(struct kvm *kvm, u64 *sptep, bool flush)
>
> Since there are no callers of __drop_large_spte, I'd be inclined to
> hold off on adding the flush parameter in this commit and just add it
> when it's needed,
The same argument about waiting until there's a user could be said
about "KVM: x86/mmu: Pass access information to
make_huge_page_split_spte()". I agree with this advice when the future
user is entirely theoretical or some future series. But when the
future user is literally the next commit in the series, I think it's
ok to do things this way since it distributes the net diff more evenly
among patches, which eases reviewing.
But, you've got me thinking and I think I want to change this commit
slightly: I'll keep __drop_larg_spte() but push all the implementation
into it and add a bool flush parameter there. That way we don't have
to change all the call sites of drop_large_spte() in this commit. The
implementation of drop_large_spte() will just be
__drop_large_spte(..., true). And the next commit can call
__drop_large_spte(..., false) with a comment.
> or better yet after you add the new user with the
> conditional flush so that there's a commit explaining why it's safe to
> not always flush in that case.
>
> > {
> > - if (is_large_pte(*sptep)) {
> > - WARN_ON(sptep_to_sp(sptep)->role.level == PG_LEVEL_4K);
> > - drop_spte(kvm, sptep);
> > - return true;
> > - }
> > + struct kvm_mmu_page *sp;
> >
> > - return false;
> > -}
> > + if (!is_large_pte(*sptep))
> > + return;
> >
> > -static void drop_large_spte(struct kvm_vcpu *vcpu, u64 *sptep)
> > -{
> > - if (__drop_large_spte(vcpu->kvm, sptep)) {
> > - struct kvm_mmu_page *sp = sptep_to_sp(sptep);
> > + sp = sptep_to_sp(sptep);
> > + WARN_ON(sp->role.level == PG_LEVEL_4K);
> > +
> > + drop_spte(kvm, sptep);
> >
> > - kvm_flush_remote_tlbs_with_address(vcpu->kvm, sp->gfn,
> > + if (flush) {
> > + kvm_flush_remote_tlbs_with_address(kvm, sp->gfn,
> > KVM_PAGES_PER_HPAGE(sp->role.level));
> > }
> > }
> > @@ -3051,7 +3048,7 @@ static int __direct_map(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault)
> > if (it.level == fault->goal_level)
> > break;
> >
> > - drop_large_spte(vcpu, it.sptep);
> > + drop_large_spte(vcpu->kvm, it.sptep, true);
> > if (is_shadow_present_pte(*it.sptep))
> > continue;
> >
> > diff --git a/arch/x86/kvm/mmu/paging_tmpl.h b/arch/x86/kvm/mmu/paging_tmpl.h
> > index 703dfb062bf0..ba61de29f2e5 100644
> > --- a/arch/x86/kvm/mmu/paging_tmpl.h
> > +++ b/arch/x86/kvm/mmu/paging_tmpl.h
> > @@ -677,7 +677,7 @@ static int FNAME(fetch)(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault,
> > gfn_t table_gfn;
> >
> > clear_sp_write_flooding_count(it.sptep);
> > - drop_large_spte(vcpu, it.sptep);
> > + drop_large_spte(vcpu->kvm, it.sptep, true);
> >
> > sp = NULL;
> > if (!is_shadow_present_pte(*it.sptep)) {
> > @@ -739,7 +739,7 @@ static int FNAME(fetch)(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault,
> >
> > validate_direct_spte(vcpu, it.sptep, direct_access);
> >
> > - drop_large_spte(vcpu, it.sptep);
> > + drop_large_spte(vcpu->kvm, it.sptep, true);
> >
> > if (!is_shadow_present_pte(*it.sptep)) {
> > sp = kvm_mmu_get_child_sp(vcpu, it.sptep, base_gfn,
> > --
> > 2.35.0.rc2.247.g8bbb082509-goog
> >
