From mboxrd@z Thu Jan  1 00:00:00 1970
From: Gleb Natapov <gleb@redhat.com>
Subject: Re: [PATCH 1/8] KVM: MMU: Fix and clean up for_each_gfn_* macros
Date: Mon, 28 Jan 2013 14:24:25 +0200
Message-ID: <20130128122425.GC22871@redhat.com>
References: <20130123191231.d66489d2.yoshikawa_takuya_b1@lab.ntt.co.jp>
 <20130123191321.e280cec8.yoshikawa_takuya_b1@lab.ntt.co.jp>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: mtosatti@redhat.com, kvm@vger.kernel.org
To: Takuya Yoshikawa <yoshikawa_takuya_b1@lab.ntt.co.jp>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mx1.redhat.com ([209.132.183.28]:1103 "EHLO mx1.redhat.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1751895Ab3A1MY3 (ORCPT <rfc822;kvm@vger.kernel.org>);
	Mon, 28 Jan 2013 07:24:29 -0500
Content-Disposition: inline
In-Reply-To: <20130123191321.e280cec8.yoshikawa_takuya_b1@lab.ntt.co.jp>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

On Wed, Jan 23, 2013 at 07:13:21PM +0900, Takuya Yoshikawa wrote:
> The expression (sp)->gfn should not be expanded using @gfn.
> 
> Although no user of these macros passes a string other than gfn now,
> this should be fixed before anyone sees strange errors.
> 
> Also, the counter-intuitive conditions, which had been used before these
> macros were introduced to avoid extra indentations, should not be used.
> 
Not sure what do you mean here. This counter-intuitive conditions are
used so that if "else" follows the macro it will not be interpreted as
belonging to the hidden if(). Like in the following code:

 if (x)
    for_each_gfn_sp()
 else
   do_y();

You do not expect do_y() to be called for each sp->gfn != gfn.

> Note: ignored the following checkpatch report:
>   ERROR: Macros with complex values should be enclosed in parenthesis
> 
> Signed-off-by: Takuya Yoshikawa <yoshikawa_takuya_b1@lab.ntt.co.jp>
> ---
>  arch/x86/kvm/mmu.c |   18 ++++++++----------
>  1 files changed, 8 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
> index 9f628f7..376cec8 100644
> --- a/arch/x86/kvm/mmu.c
> +++ b/arch/x86/kvm/mmu.c
> @@ -1658,16 +1658,14 @@ static int kvm_mmu_prepare_zap_page(struct kvm *kvm, struct kvm_mmu_page *sp,
>  static void kvm_mmu_commit_zap_page(struct kvm *kvm,
>  				    struct list_head *invalid_list);
>  
> -#define for_each_gfn_sp(kvm, sp, gfn, pos)				\
> -  hlist_for_each_entry(sp, pos,						\
> -   &(kvm)->arch.mmu_page_hash[kvm_page_table_hashfn(gfn)], hash_link)	\
> -	if ((sp)->gfn != (gfn)) {} else
> -
> -#define for_each_gfn_indirect_valid_sp(kvm, sp, gfn, pos)		\
> -  hlist_for_each_entry(sp, pos,						\
> -   &(kvm)->arch.mmu_page_hash[kvm_page_table_hashfn(gfn)], hash_link)	\
> -		if ((sp)->gfn != (gfn) || (sp)->role.direct ||		\
> -			(sp)->role.invalid) {} else
> +#define for_each_gfn_sp(_kvm, _sp, _gfn, _pos)				\
> +	hlist_for_each_entry(_sp, _pos,					\
> +	  &(_kvm)->arch.mmu_page_hash[kvm_page_table_hashfn(_gfn)], hash_link) \
> +		if ((_sp)->gfn == (_gfn))
> +
> +#define for_each_gfn_indirect_valid_sp(_kvm, _sp, _gfn, _pos)		\
> +	for_each_gfn_sp(_kvm, _sp, _gfn, _pos)				\
> +		if (!(_sp)->role.direct && !(_sp)->role.invalid)
>  
>  /* @sp->gfn should be write-protected at the call site */
>  static int __kvm_sync_page(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp,
> -- 
> 1.7.5.4

--
			Gleb.