| Message ID | 20130123191321.e280cec8.yoshikawa_takuya_b1@lab.ntt.co.jp (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 28 Jan 2013 14:24:25 +0200 Gleb Natapov <gleb@redhat.com> wrote: > 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. I could not think of this case. Will fix not to change the current conditions. Thanks, Takuya -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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,
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. 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(-)