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, 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
>
