Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 11 May 2021 20:03:14 +0000
From:   Sean Christopherson <seanjc@google.com>
To:     Ben Gardon <bgardon@google.com>
Cc:     linux-kernel@vger.kernel.org, kvm@vger.kernel.org,
        Paolo Bonzini <pbonzini@redhat.com>,
        Peter Xu <peterx@redhat.com>, Peter Shier <pshier@google.com>,
        Yulei Zhang <yulei.kernel@gmail.com>,
        Wanpeng Li <kernellwp@gmail.com>,
        Xiao Guangrong <xiaoguangrong.eric@gmail.com>,
        Kai Huang <kai.huang@intel.com>,
        Keqian Zhu <zhukeqian1@huawei.com>,
        David Hildenbrand <david@redhat.com>
Subject: Re: [PATCH v4 7/7] KVM: x86/mmu: Lazily allocate memslot rmaps
Message-ID: <YJrjAt5eyCZQNSkM@google.com>
References: <20210511171610.170160-1-bgardon@google.com>
 <20210511171610.170160-8-bgardon@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210511171610.170160-8-bgardon@google.com>
Precedence: bulk

On Tue, May 11, 2021, Ben Gardon wrote:
> If the TDP MMU is in use, wait to allocate the rmaps until the shadow
> MMU is actually used. (i.e. a nested VM is launched.) This saves memory
> equal to 0.2% of guest memory in cases where the TDP MMU is used and
> there are no nested guests involved.
> 
> Signed-off-by: Ben Gardon <bgardon@google.com>
> ---
>  arch/x86/include/asm/kvm_host.h |  2 ++
>  arch/x86/kvm/mmu/mmu.c          | 53 +++++++++++++++++++++++----------
>  arch/x86/kvm/mmu/tdp_mmu.c      |  6 ++--
>  arch/x86/kvm/mmu/tdp_mmu.h      |  4 +--
>  arch/x86/kvm/x86.c              | 45 +++++++++++++++++++++++++++-
>  5 files changed, 89 insertions(+), 21 deletions(-)
> 
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index fc75ed49bfee..7b65f82ade1c 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -1868,4 +1868,6 @@ static inline int kvm_cpu_get_apicid(int mps_cpu)
>  
>  int kvm_cpu_dirty_log_size(void);
>  
> +int alloc_all_memslots_rmaps(struct kvm *kvm);
> +
>  #endif /* _ASM_X86_KVM_HOST_H */
> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> index b0bdb924d519..183afccd2944 100644
> --- a/arch/x86/kvm/mmu/mmu.c
> +++ b/arch/x86/kvm/mmu/mmu.c
> @@ -1190,7 +1190,8 @@ static void kvm_mmu_write_protect_pt_masked(struct kvm *kvm,
>  		kvm_tdp_mmu_clear_dirty_pt_masked(kvm, slot,
>  				slot->base_gfn + gfn_offset, mask, true);
>  
> -	if (!kvm->arch.memslots_have_rmaps)
> +	/* Read memslots_have_rmaps before the rmaps themselves */

IIRC, you open coded reading memslots_have_rmaps because of a circular
dependency, but you can solve that simply by defining the helper in mmu.h
instead of kvm_host.h.

And I think you could even make it static in mmu.c and omit the smp_load_acuquire
from the three users in x86.c, though that's probably not worth it.

Either way, reading the same comment over and over and over, just to make
checkpatch happy, gets more than a bit tedious.

That would also allow you to elaborate on why the smp_load_acquire() is
necessary, and preferably what it pairs with.

> +	if (!smp_load_acquire(&kvm->arch.memslots_have_rmaps))
>  		return;
>  
>  	while (mask) {