Subject: Re: [PATCH 4/4] KVM: MMU: skip obsolete sp in for_each_gfn_*()
To: Xiao Guangrong <guangrong.xiao@linux.intel.com>
References: <1458911978-19430-1-git-send-email-guangrong.xiao@linux.intel.com>
 <1458911978-19430-4-git-send-email-guangrong.xiao@linux.intel.com>
Cc: gleb@kernel.org, mtosatti@redhat.com, kvm@vger.kernel.org,
        linux-kernel@vger.kernel.org
From: Paolo Bonzini <pbonzini@redhat.com>
Message-ID: <56FA4E88.9040906@redhat.com>
Date: Tue, 29 Mar 2016 11:44:40 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101
 Thunderbird/38.6.0
MIME-Version: 1.0
In-Reply-To: <1458911978-19430-4-git-send-email-guangrong.xiao@linux.intel.com>
Content-Type: text/plain; charset=windows-1252
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3108
Lines: 84

On 25/03/2016 14:19, Xiao Guangrong wrote:
> The obsolete sp should not be used on current vCPUs and should not hurt
> vCPU's running, so skip it from for_each_gfn_sp() and
> for_each_gfn_indirect_valid_sp()
> 
> The side effort is we will double check role.invalid in kvm_mmu_get_page()
> but i think it is okay as role is well cached
> 
> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>

Queued for 4.7.

Paolo

> ---
>  arch/x86/kvm/mmu.c | 28 ++++++++++++----------------
>  1 file changed, 12 insertions(+), 16 deletions(-)
> 
> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
> index c396e8b..4d66a9e 100644
> --- a/arch/x86/kvm/mmu.c
> +++ b/arch/x86/kvm/mmu.c
> @@ -1906,18 +1906,17 @@ static void kvm_mmu_commit_zap_page(struct kvm *kvm,
>   * since it has been deleted from active_mmu_pages but still can be found
>   * at hast list.
>   *
> - * for_each_gfn_indirect_valid_sp has skipped that kind of page and
> - * kvm_mmu_get_page(), the only user of for_each_gfn_sp(), has skipped
> - * all the obsolete pages.
> + * for_each_gfn_valid_sp() has skipped that kind of pages.
>   */
> -#define for_each_gfn_sp(_kvm, _sp, _gfn)				\
> +#define for_each_gfn_valid_sp(_kvm, _sp, _gfn)				\
>  	hlist_for_each_entry(_sp,					\
>  	  &(_kvm)->arch.mmu_page_hash[kvm_page_table_hashfn(_gfn)], hash_link) \
> -		if ((_sp)->gfn != (_gfn)) {} else
> +		if ((_sp)->gfn != (_gfn) || is_obsolete_sp((_kvm), (_sp)) \
> +			|| (_sp)->role.invalid) {} else
>  
>  #define for_each_gfn_indirect_valid_sp(_kvm, _sp, _gfn)			\
> -	for_each_gfn_sp(_kvm, _sp, _gfn)				\
> -		if ((_sp)->role.direct || (_sp)->role.invalid) {} else
> +	for_each_gfn_valid_sp(_kvm, _sp, _gfn)				\
> +		if ((_sp)->role.direct) {} else
>  
>  /* @sp->gfn should be write-protected at the call site */
>  static bool __kvm_sync_page(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp,
> @@ -1958,6 +1957,11 @@ static void kvm_mmu_audit(struct kvm_vcpu *vcpu, int point) { }
>  static void mmu_audit_disable(void) { }
>  #endif
>  
> +static bool is_obsolete_sp(struct kvm *kvm, struct kvm_mmu_page *sp)
> +{
> +	return unlikely(sp->mmu_valid_gen != kvm->arch.mmu_valid_gen);
> +}
> +
>  static bool kvm_sync_page(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp,
>  			 struct list_head *invalid_list)
>  {
> @@ -2092,11 +2096,6 @@ static void clear_sp_write_flooding_count(u64 *spte)
>  	__clear_sp_write_flooding_count(sp);
>  }
>  
> -static bool is_obsolete_sp(struct kvm *kvm, struct kvm_mmu_page *sp)
> -{
> -	return unlikely(sp->mmu_valid_gen != kvm->arch.mmu_valid_gen);
> -}
> -
>  static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu,
>  					     gfn_t gfn,
>  					     gva_t gaddr,
> @@ -2123,10 +2122,7 @@ static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu,
>  		quadrant &= (1 << ((PT32_PT_BITS - PT64_PT_BITS) * level)) - 1;
>  		role.quadrant = quadrant;
>  	}
> -	for_each_gfn_sp(vcpu->kvm, sp, gfn) {
> -		if (is_obsolete_sp(vcpu->kvm, sp))
> -			continue;
> -
> +	for_each_gfn_valid_sp(vcpu->kvm, sp, gfn) {
>  		if (!need_sync && sp->unsync)
>  			need_sync = true;
>  
>