Re: [PATCH 10/24] kvm: x86/mmu: Factor out handle disconnected pt

Paolo Bonzini
2021-01-26T14:22:42Z

On 12/01/21 19:10, Ben Gardon wrote:
> Factor out the code to handle a disconnected subtree of the TDP paging
> structure from the code to handle the change to an individual SPTE.
> Future commits will build on this to allow asynchronous page freeing.
>
> No functional change intended.
>
> Reviewed-by: Peter Feiner <pfeiner@google.com>
>
> Signed-off-by: Ben Gardon <bgardon@google.com>
> ---
> arch/x86/kvm/mmu/tdp_mmu.c | 75 +++++++++++++++++++++++---------------
> 1 file changed, 46 insertions(+), 29 deletions(-)
>
> diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c
> index 55df596696c7..e8f35cd46b4c 100644
> --- a/arch/x86/kvm/mmu/tdp_mmu.c
> +++ b/arch/x86/kvm/mmu/tdp_mmu.c
> @@ -234,6 +234,49 @@ static void handle_changed_spte_dirty_log(struct kvm *kvm, int as_id, gfn_t gfn,
> }
> }
>
> +/**
> + * handle_disconnected_tdp_mmu_page - handle a pt removed from the TDP structure
> + *
> + * @kvm: kvm instance
> + * @pt: the page removed from the paging structure
> + *
> + * Given a page table that has been removed from the TDP paging structure,
> + * iterates through the page table to clear SPTEs and free child page tables.
> + */
> +static void handle_disconnected_tdp_mmu_page(struct kvm *kvm, u64 *pt)
> +{
> + struct kvm_mmu_page *sp;
> + gfn_t gfn;
> + int level;
> + u64 old_child_spte;
> + int i;
> +
> + sp = sptep_to_sp(pt);
> + gfn = sp->gfn;
> + level = sp->role.level;
> +
> + trace_kvm_mmu_prepare_zap_page(sp);
> +
> + list_del(&sp->link);
> +
> + if (sp->lpage_disallowed)
> + unaccount_huge_nx_page(kvm, sp);
> +
> + for (i = 0; i < PT64_ENT_PER_PAGE; i++) {
> + old_child_spte = READ_ONCE(*(pt + i));
> + WRITE_ONCE(*(pt + i), 0);
> + handle_changed_spte(kvm, kvm_mmu_page_as_id(sp),
> + gfn + (i * KVM_PAGES_PER_HPAGE(level - 1)),
> + old_child_spte, 0, level - 1);
> + }
> +
> + kvm_flush_remote_tlbs_with_address(kvm, gfn,
> + KVM_PAGES_PER_HPAGE(level));
> +
> + free_page((unsigned long)pt);
> + kmem_cache_free(mmu_page_header_cache, sp);
> +}
> +
> /**
> * handle_changed_spte - handle bookkeeping associated with an SPTE change
> * @kvm: kvm instance
> @@ -254,10 +297,6 @@ static void __handle_changed_spte(struct kvm *kvm, int as_id, gfn_t gfn,
> bool was_leaf = was_present && is_last_spte(old_spte, level);
> bool is_leaf = is_present && is_last_spte(new_spte, level);
> bool pfn_changed = spte_to_pfn(old_spte) != spte_to_pfn(new_spte);
> - u64 *pt;
> - struct kvm_mmu_page *sp;
> - u64 old_child_spte;
> - int i;
>
> WARN_ON(level > PT64_ROOT_MAX_LEVEL);
> WARN_ON(level < PG_LEVEL_4K);
> @@ -321,31 +360,9 @@ static void __handle_changed_spte(struct kvm *kvm, int as_id, gfn_t gfn,
> * Recursively handle child PTs if the change removed a subtree from
> * the paging structure.
> */
> - if (was_present && !was_leaf && (pfn_changed || !is_present)) {
> - pt = spte_to_child_pt(old_spte, level);
> - sp = sptep_to_sp(pt);
> -
> - trace_kvm_mmu_prepare_zap_page(sp);
> -
> - list_del(&sp->link);
> -
> - if (sp->lpage_disallowed)
> - unaccount_huge_nx_page(kvm, sp);
> -
> - for (i = 0; i < PT64_ENT_PER_PAGE; i++) {
> - old_child_spte = READ_ONCE(*(pt + i));
> - WRITE_ONCE(*(pt + i), 0);
> - handle_changed_spte(kvm, as_id,
> - gfn + (i * KVM_PAGES_PER_HPAGE(level - 1)),
> - old_child_spte, 0, level - 1);
> - }
> -
> - kvm_flush_remote_tlbs_with_address(kvm, gfn,
> - KVM_PAGES_PER_HPAGE(level));
> -
> - free_page((unsigned long)pt);
> - kmem_cache_free(mmu_page_header_cache, sp);
> - }
> + if (was_present && !was_leaf && (pfn_changed || !is_present))
> + handle_disconnected_tdp_mmu_page(kvm,
> + spte_to_child_pt(old_spte, level));
> }
>
> static void handle_changed_spte(struct kvm *kvm, int as_id, gfn_t gfn,
>

Queued, thanks.

Paolo