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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <47e0b040-f9de-376d-80a3-75b5680a7b85@redhat.com>
Date:   Tue, 26 Jan 2021 15:14:16 +0100
From:   Paolo Bonzini <pbonzini@...hat.com>
To:     Ben Gardon <bgardon@...gle.com>, linux-kernel@...r.kernel.org,
        kvm@...r.kernel.org
Cc:     Peter Xu <peterx@...hat.com>,
        Sean Christopherson <seanjc@...gle.com>,
        Peter Shier <pshier@...gle.com>,
        Peter Feiner <pfeiner@...gle.com>,
        Junaid Shahid <junaids@...gle.com>,
        Jim Mattson <jmattson@...gle.com>,
        Yulei Zhang <yulei.kernel@...il.com>,
        Wanpeng Li <kernellwp@...il.com>,
        Vitaly Kuznetsov <vkuznets@...hat.com>,
        Xiao Guangrong <xiaoguangrong.eric@...il.com>
Subject: Re: [PATCH 10/24] kvm: x86/mmu: Factor out handle disconnected pt

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@...gle.com>
> 
> Signed-off-by: Ben Gardon <bgardon@...gle.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