linux-kernel - Re: [PATCH v3 3/3] KVM: x86/mmu: Defer allocation of shadow MMU's hashed page list

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: <fb0580d9-103f-4aa1-94ae-c67938460d71@redhat.com>
Date: Sat, 17 May 2025 14:43:28 +0200
From: Paolo Bonzini <pbonzini@...hat.com>
To: Sean Christopherson <seanjc@...gle.com>
Cc: kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
 Vipin Sharma <vipinsh@...gle.com>
Subject: Re: [PATCH v3 3/3] KVM: x86/mmu: Defer allocation of shadow MMU's
 hashed page list

On 5/16/25 23:54, Sean Christopherson wrote:
> When the TDP MMU is enabled, i.e. when the shadow MMU isn't used until a
> nested TDP VM is run, defer allocation of the array of hashed lists used
> to track shadow MMU pages until the first shadow root is allocated.
> 
> Setting the list outside of mmu_lock is safe, as concurrent readers must
> hold mmu_lock in some capacity, shadow pages can only be added (or removed)
> from the list when mmu_lock is held for write, and tasks that are creating
> a shadow root are serialized by slots_arch_lock.  I.e. it's impossible for
> the list to become non-empty until all readers go away, and so readers are
> guaranteed to see an empty list even if they make multiple calls to
> kvm_get_mmu_page_hash() in a single mmu_lock critical section.
> 
> Use {WRITE/READ}_ONCE to set/get the list when mmu_lock isn't held for
> write, out of an abundance of paranoia; no sane compiler should tear the
> store or load, but it's technically possible.
> 
> Signed-off-by: Sean Christopherson <seanjc@...gle.com>
> ---
>   arch/x86/kvm/mmu/mmu.c | 60 +++++++++++++++++++++++++++++++++++-------
>   1 file changed, 50 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> index 41da2cb1e3f1..edb4ecff9917 100644
> --- a/arch/x86/kvm/mmu/mmu.c
> +++ b/arch/x86/kvm/mmu/mmu.c
> @@ -1983,14 +1983,33 @@ static bool sp_has_gptes(struct kvm_mmu_page *sp)
>   	return true;
>   }
>   
> +static __ro_after_init HLIST_HEAD(empty_page_hash);
> +
> +static struct hlist_head *kvm_get_mmu_page_hash(struct kvm *kvm, gfn_t gfn)
> +{
> +	/*
> +	 * Load mmu_page_hash from memory exactly once, as it's set at runtime
> +	 * outside of mmu_lock when the TDP MMU is enabled, i.e. when the hash
> +	 * table of shadow pages isn't needed unless KVM needs to shadow L1's
> +	 * TDP for an L2 guest.

Avoid double negations, for example "i.e. when shadow paging and the 
mmu_page_hash is only needed for a L1 hypervisor's TDP".

> +	 */
> +	struct hlist_head *page_hash = READ_ONCE(kvm->arch.mmu_page_hash);

This READ_ONCE is more for the (now implicit) smp_read_barrier_depends() 
than for "loading exactly once".  Might as well use smp_load_acquire() 
since it's free on x86.

> +	lockdep_assert_held(&kvm->mmu_lock);

Move this comment here from kvm_mmu_alloc_page_hash():

	/* mmu_lock must be held for write to add (or remove) shadow
	 * pages, and so readers are guaranteed to see an empty list for
	 * their current mmu_lock critical section.
	 */
> +	if (!page_hash)
> +		return &empty_page_hash;
> +
> +	return &page_hash[kvm_page_table_hashfn(gfn)];
> +}
> +
>   #define for_each_valid_sp(_kvm, _sp, _list)				\
>   	hlist_for_each_entry(_sp, _list, hash_link)			\
>   		if (is_obsolete_sp((_kvm), (_sp))) {			\
>   		} else
>   
>   #define for_each_gfn_valid_sp_with_gptes(_kvm, _sp, _gfn)		\
> -	for_each_valid_sp(_kvm, _sp,					\
> -	  &(_kvm)->arch.mmu_page_hash[kvm_page_table_hashfn(_gfn)])	\
> +	for_each_valid_sp(_kvm, _sp, kvm_get_mmu_page_hash(_kvm, _gfn))	\
>   		if ((_sp)->gfn != (_gfn) || !sp_has_gptes(_sp)) {} else
>   
>   static bool kvm_sync_page_check(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp)
> @@ -2358,6 +2377,12 @@ static struct kvm_mmu_page *__kvm_mmu_get_shadow_page(struct kvm *kvm,
>   	struct kvm_mmu_page *sp;
>   	bool created = false;
>   
> +	/*
> +	 * No need for READ_ONCE(), unlike in kvm_get_mmu_page_hash(), because
> +	 * mmu_page_hash must be set prior to creating the first shadow root,
> +	 * i.e. reaching this point is fully serialized by slots_arch_lock.
> +	 */
> +	BUG_ON(!kvm->arch.mmu_page_hash);
>   	sp_list = &kvm->arch.mmu_page_hash[kvm_page_table_hashfn(gfn)];
>   
>   	sp = kvm_mmu_find_shadow_page(kvm, vcpu, gfn, sp_list, role);
> @@ -3886,11 +3911,21 @@ static int kvm_mmu_alloc_page_hash(struct kvm *kvm)
>   {
>   	typeof(kvm->arch.mmu_page_hash) h;
>   
> +	if (kvm->arch.mmu_page_hash)
> +		return 0;
> +
>   	h = kvcalloc(KVM_NUM_MMU_PAGES, sizeof(*h), GFP_KERNEL_ACCOUNT);
>   	if (!h)
>   		return -ENOMEM;
>   
> -	kvm->arch.mmu_page_hash = h;
> +	/*
> +	 * Write mmu_page_hash exactly once as there may be concurrent readers,
> +	 * e.g. to check for shadowed PTEs in mmu_try_to_unsync_pages().  Note,
> +	 * mmu_lock must be held for write to add (or remove) shadow pages, and
> +	 * so readers are guaranteed to see an empty list for their current
> +	 * mmu_lock critical section.
> +	 */
> +	WRITE_ONCE(kvm->arch.mmu_page_hash, h);

Use smp_store_release here (unlike READ_ONCE(), it's technically 
incorrect to use WRITE_ONCE() here!), with a remark that it pairs with 
kvm_get_mmu_page_hash().  That's both more accurate and leads to a 
better comment than "write exactly once".

Paolo