Re: [PATCH v8 06/11] KVM: x86/mmu: Only check gfn age in shadow MMU if indirect_shadow_pages > 0

[Sean Christopherson <seanjc@google.com>]

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@google.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
>