linux-kernel - Re: [PATCH v8 06/11] KVM: x86/mmu: Only check gfn age in shadow MMU if indirect_shadow

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: <Z4GnvHqZqxW7sRjs@google.com>
Date: Fri, 10 Jan 2025 15:05:32 -0800
From: Sean Christopherson <seanjc@...gle.com>
To: James Houghton <jthoughton@...gle.com>
Cc: Paolo Bonzini <pbonzini@...hat.com>, David Matlack <dmatlack@...gle.com>, 
	David Rientjes <rientjes@...gle.com>, Marc Zyngier <maz@...nel.org>, 
	Oliver Upton <oliver.upton@...ux.dev>, Wei Xu <weixugc@...gle.com>, Yu Zhao <yuzhao@...gle.com>, 
	Axel Rasmussen <axelrasmussen@...gle.com>, kvm@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v8 06/11] KVM: x86/mmu: Only check gfn age in shadow MMU
 if indirect_shadow_pages > 0

On Tue, Nov 05, 2024, James Houghton wrote:
> Optimize both kvm_age_gfn and kvm_test_age_gfn's interaction with the
> shadow MMU by, rather than checking if our memslot has rmaps, check if

No "our" (pronouns bad).

> there are any indirect_shadow_pages at all.

Again, use wording that is more conversational.  I also think it's worthwhile to
call out when this optimization is helpful.  E.g.

When aging SPTEs and the TDP MMU is enabled, process the shadow MMU if and
only if the VM has at least one shadow page, as opposed to checking if the
VM has rmaps.  Checking for rmaps will effectively yield a false positive
if the VM ran nested TDP VMs in the past, but is not currently doing so.

> Signed-off-by: James Houghton <jthoughton@...gle.com>
> ---
>  arch/x86/kvm/mmu/mmu.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> index 793565a3a573..125d4c3ccceb 100644
> --- a/arch/x86/kvm/mmu/mmu.c
> +++ b/arch/x86/kvm/mmu/mmu.c
> @@ -1582,6 +1582,11 @@ static bool kvm_rmap_age_gfn_range(struct kvm *kvm,
>  	return young;
>  }
>  
> +static bool kvm_has_shadow_mmu_sptes(struct kvm *kvm)

I think this should be kvm_may_have_shadow_mmu_sptes(), or something along those
lines that makes it clear the check is imprecise.  E.g. to avoid someone thinking
that KVM is guaranteed to have shadow MMU SPTEs if it returns true.

> +{
> +	return !tdp_mmu_enabled || READ_ONCE(kvm->arch.indirect_shadow_pages);
> +}
> +
>  bool kvm_age_gfn(struct kvm *kvm, struct kvm_gfn_range *range)
>  {
>  	bool young = false;
> @@ -1589,7 +1594,7 @@ bool kvm_age_gfn(struct kvm *kvm, struct kvm_gfn_range *range)
>  	if (tdp_mmu_enabled)
>  		young = kvm_tdp_mmu_age_gfn_range(kvm, range);
>  
> -	if (kvm_memslots_have_rmaps(kvm)) {
> +	if (kvm_has_shadow_mmu_sptes(kvm)) {
>  		write_lock(&kvm->mmu_lock);
>  		young |= kvm_rmap_age_gfn_range(kvm, range, false);
>  		write_unlock(&kvm->mmu_lock);
> @@ -1605,7 +1610,7 @@ bool kvm_test_age_gfn(struct kvm *kvm, struct kvm_gfn_range *range)
>  	if (tdp_mmu_enabled)
>  		young = kvm_tdp_mmu_test_age_gfn(kvm, range);
>  
> -	if (!young && kvm_memslots_have_rmaps(kvm)) {
> +	if (!young && kvm_has_shadow_mmu_sptes(kvm)) {
>  		write_lock(&kvm->mmu_lock);
>  		young |= kvm_rmap_age_gfn_range(kvm, range, true);
>  		write_unlock(&kvm->mmu_lock);
> -- 
> 2.47.0.199.ga7371fff76-goog
>