| Message ID | 20220428094625.382970-1-michal.orzel@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] xen/arm: gnttab: cast unused macro arguments to void | expand |
On 28.04.2022 11:46, Michal Orzel wrote: > @@ -89,10 +90,12 @@ int replace_grant_host_mapping(unsigned long gpaddr, mfn_t mfn, > }) > > #define gnttab_shared_gfn(d, t, i) \ > - (((i) >= nr_grant_frames(t)) ? INVALID_GFN : (t)->arch.shared_gfn[i]) > + ((void)(d), \ > + ((i) >= nr_grant_frames(t)) ? INVALID_GFN : (t)->arch.shared_gfn[i]) > > -#define gnttab_status_gfn(d, t, i) \ > - (((i) >= nr_status_frames(t)) ? INVALID_GFN : (t)->arch.status_gfn[i]) > +#define gnttab_status_gfn(d, t, i) \ > + ((void)(d), \ > + ((i) >= nr_status_frames(t)) ? INVALID_GFN : (t)->arch.status_gfn[i]) Just as a note (I don't mind changing these too): If a macro cares to evaluate all its arguments, I think it should also care to evaluate all of them exactly once. Jan
Hi, On 28/04/2022 10:46, Michal Orzel wrote: > Function unmap_common_complete (common/grant_table.c) defines and sets > a variable ld that is later on passed to a macro: > gnttab_host_mapping_get_page_type(). > On Arm this macro does not make use of any arguments causing a compiler > to warn about unused-but-set variable (when -Wunused-but-set-variable > is enabled). Fix it by casting the arguments to void in macro's body. > > While there, take the opportunity to modify other macros in this file > that do not make use of all the arguments to prevent similar issues in > the future. > > Signed-off-by: Michal Orzel <michal.orzel@arm.com> > --- > Changes since v1: > -standalone patch carved out from a series (other patches already merged) > -v1 was ([3/8] gnttab: Remove unused-but-set variable) > -modify macro on Arm instead of removing ld variable > --- > xen/arch/arm/include/asm/grant_table.h | 13 ++++++++----- > 1 file changed, 8 insertions(+), 5 deletions(-) > > diff --git a/xen/arch/arm/include/asm/grant_table.h b/xen/arch/arm/include/asm/grant_table.h > index d31a4d6805..5bcd1ec528 100644 > --- a/xen/arch/arm/include/asm/grant_table.h > +++ b/xen/arch/arm/include/asm/grant_table.h > @@ -31,10 +31,11 @@ static inline void gnttab_mark_dirty(struct domain *d, mfn_t mfn) > > int create_grant_host_mapping(unsigned long gpaddr, mfn_t mfn, > unsigned int flags, unsigned int cache_flags); > -#define gnttab_host_mapping_get_page_type(ro, ld, rd) (0) > +#define gnttab_host_mapping_get_page_type(ro, ld, rd) \ > + ((void)(ro), (void)(ld), (void)(rd), 0) I would switch to a static inline helper: static inline bool gnttab_host_mapping_get_page_type(bool ro, struct domain *ld, struct domian *rd) { return false; } Note the switch from 0 to false as the function is technically returning a boolean (see the x86 implementation). > int replace_grant_host_mapping(unsigned long gpaddr, mfn_t mfn, > unsigned long new_gpaddr, unsigned int flags); > -#define gnttab_release_host_mappings(domain) 1 > +#define gnttab_release_host_mappings(domain) ((void)(domain), 1) Same here. > > /* > * The region used by Xen on the memory will never be mapped in DOM0 > @@ -89,10 +90,12 @@ int replace_grant_host_mapping(unsigned long gpaddr, mfn_t mfn, > }) > > #define gnttab_shared_gfn(d, t, i) \ > - (((i) >= nr_grant_frames(t)) ? INVALID_GFN : (t)->arch.shared_gfn[i]) > + ((void)(d), \ > + ((i) >= nr_grant_frames(t)) ? INVALID_GFN : (t)->arch.shared_gfn[i]) > > -#define gnttab_status_gfn(d, t, i) \ > - (((i) >= nr_status_frames(t)) ? INVALID_GFN : (t)->arch.status_gfn[i]) > +#define gnttab_status_gfn(d, t, i) \ > + ((void)(d), \ > + ((i) >= nr_status_frames(t)) ? INVALID_GFN : (t)->arch.status_gfn[i]) I share Jan's opinion here. If we want to evaluate d, then we should make sure t and i should be also evaluated once. However, IIRC, they can't be turned to static inline because the type of t (struct grant_table) is not fully defined yet. Cheers
Hi Julien, On 03.05.2022 19:44, Julien Grall wrote: > Hi, > > On 28/04/2022 10:46, Michal Orzel wrote: >> Function unmap_common_complete (common/grant_table.c) defines and sets >> a variable ld that is later on passed to a macro: >> gnttab_host_mapping_get_page_type(). >> On Arm this macro does not make use of any arguments causing a compiler >> to warn about unused-but-set variable (when -Wunused-but-set-variable >> is enabled). Fix it by casting the arguments to void in macro's body. >> >> While there, take the opportunity to modify other macros in this file >> that do not make use of all the arguments to prevent similar issues in >> the future. >> >> Signed-off-by: Michal Orzel <michal.orzel@arm.com> >> --- >> Changes since v1: >> -standalone patch carved out from a series (other patches already merged) >> -v1 was ([3/8] gnttab: Remove unused-but-set variable) >> -modify macro on Arm instead of removing ld variable >> --- >> xen/arch/arm/include/asm/grant_table.h | 13 ++++++++----- >> 1 file changed, 8 insertions(+), 5 deletions(-) >> >> diff --git a/xen/arch/arm/include/asm/grant_table.h b/xen/arch/arm/include/asm/grant_table.h >> index d31a4d6805..5bcd1ec528 100644 >> --- a/xen/arch/arm/include/asm/grant_table.h >> +++ b/xen/arch/arm/include/asm/grant_table.h >> @@ -31,10 +31,11 @@ static inline void gnttab_mark_dirty(struct domain *d, mfn_t mfn) >> int create_grant_host_mapping(unsigned long gpaddr, mfn_t mfn, >> unsigned int flags, unsigned int cache_flags); >> -#define gnttab_host_mapping_get_page_type(ro, ld, rd) (0) >> +#define gnttab_host_mapping_get_page_type(ro, ld, rd) \ >> + ((void)(ro), (void)(ld), (void)(rd), 0) > > I would switch to a static inline helper: > > static inline bool > gnttab_host_mapping_get_page_type(bool ro, struct domain *ld, > struct domian *rd) > { > return false; > } > > Note the switch from 0 to false as the function is technically returning a boolean (see the x86 implementation). > >> int replace_grant_host_mapping(unsigned long gpaddr, mfn_t mfn, >> unsigned long new_gpaddr, unsigned int flags); >> -#define gnttab_release_host_mappings(domain) 1 >> +#define gnttab_release_host_mappings(domain) ((void)(domain), 1) > > Same here. > Ok, sounds right. >> /* >> * The region used by Xen on the memory will never be mapped in DOM0 >> @@ -89,10 +90,12 @@ int replace_grant_host_mapping(unsigned long gpaddr, mfn_t mfn, >> }) >> #define gnttab_shared_gfn(d, t, i) \ >> - (((i) >= nr_grant_frames(t)) ? INVALID_GFN : (t)->arch.shared_gfn[i]) >> + ((void)(d), \ >> + ((i) >= nr_grant_frames(t)) ? INVALID_GFN : (t)->arch.shared_gfn[i]) >> -#define gnttab_status_gfn(d, t, i) \ >> - (((i) >= nr_status_frames(t)) ? INVALID_GFN : (t)->arch.status_gfn[i]) >> +#define gnttab_status_gfn(d, t, i) \ >> + ((void)(d), \ >> + ((i) >= nr_status_frames(t)) ? INVALID_GFN : (t)->arch.status_gfn[i]) > > I share Jan's opinion here. If we want to evaluate d, then we should make sure t and i should be also evaluated once. However, IIRC, they can't be turned to static inline because the type of t (struct grant_table) is not fully defined yet. > Then, we could do like this: #define gnttab_shared_gfn(d, t, i) \ ({ \ const unsigned int _i = (i); \ const struct grant_table *_t = (t); \ (void)(d); \ (_i >= nr_grant_frames(_t)) ? INVALID_GFN \ : _t->arch.shared_gfn[_i]; \ }) However, if we start modifying the macros to evaluate args only once, shouldn't we also take care of the following macros in this file?: gnttab_set_frame_gfn gnttab_init_arch I'm ok to do these changes but I'm afriad we are losing the origin of this patch as we are focusing on macros not related to the issue. > Cheers > Cheers, Michal
On 04.05.2022 08:41, Michal Orzel wrote: > On 03.05.2022 19:44, Julien Grall wrote: >> On 28/04/2022 10:46, Michal Orzel wrote: >>> @@ -89,10 +90,12 @@ int replace_grant_host_mapping(unsigned long gpaddr, mfn_t mfn, >>> }) >>> #define gnttab_shared_gfn(d, t, i) \ >>> - (((i) >= nr_grant_frames(t)) ? INVALID_GFN : (t)->arch.shared_gfn[i]) >>> + ((void)(d), \ >>> + ((i) >= nr_grant_frames(t)) ? INVALID_GFN : (t)->arch.shared_gfn[i]) >>> -#define gnttab_status_gfn(d, t, i) \ >>> - (((i) >= nr_status_frames(t)) ? INVALID_GFN : (t)->arch.status_gfn[i]) >>> +#define gnttab_status_gfn(d, t, i) \ >>> + ((void)(d), \ >>> + ((i) >= nr_status_frames(t)) ? INVALID_GFN : (t)->arch.status_gfn[i]) >> >> I share Jan's opinion here. If we want to evaluate d, then we should make sure t and i should be also evaluated once. However, IIRC, they can't be turned to static inline because the type of t (struct grant_table) is not fully defined yet. >> > Then, we could do like this: > > #define gnttab_shared_gfn(d, t, i) \ > ({ \ > const unsigned int _i = (i); \ > const struct grant_table *_t = (t); \ > (void)(d); \ > (_i >= nr_grant_frames(_t)) ? INVALID_GFN \ > : _t->arch.shared_gfn[_i]; \ > }) Please avoid underscore-prefixed names here; we've started to use underscore-suffixed names in a few macros. Additionally please consider using typeof() instead of spelling out types. This may help to avoid surprising behavior. Finally, instead of merely casting d to void, please consider using it in e.g. ASSERT((d)->grant_table == t_), which ought to also take care of the unused variable warning. After all the explicit passing of t is only an (attempted) optimization here. > However, if we start modifying the macros to evaluate args only once, shouldn't we also take care of the following macros in this file?: > gnttab_set_frame_gfn > gnttab_init_arch > > I'm ok to do these changes but I'm afriad we are losing the origin of this patch as we are focusing on macros not related to the issue. Indeed - I'd leave further ones to a subsequent patch, or make conversion of all of the macros a prereq patch to the one you're after. Jan
On 04.05.2022 10:13, Jan Beulich wrote: > On 04.05.2022 08:41, Michal Orzel wrote: >> On 03.05.2022 19:44, Julien Grall wrote: >>> On 28/04/2022 10:46, Michal Orzel wrote: >>>> @@ -89,10 +90,12 @@ int replace_grant_host_mapping(unsigned long gpaddr, mfn_t mfn, >>>> }) >>>> #define gnttab_shared_gfn(d, t, i) \ >>>> - (((i) >= nr_grant_frames(t)) ? INVALID_GFN : (t)->arch.shared_gfn[i]) >>>> + ((void)(d), \ >>>> + ((i) >= nr_grant_frames(t)) ? INVALID_GFN : (t)->arch.shared_gfn[i]) >>>> -#define gnttab_status_gfn(d, t, i) \ >>>> - (((i) >= nr_status_frames(t)) ? INVALID_GFN : (t)->arch.status_gfn[i]) >>>> +#define gnttab_status_gfn(d, t, i) \ >>>> + ((void)(d), \ >>>> + ((i) >= nr_status_frames(t)) ? INVALID_GFN : (t)->arch.status_gfn[i]) >>> >>> I share Jan's opinion here. If we want to evaluate d, then we should make sure t and i should be also evaluated once. However, IIRC, they can't be turned to static inline because the type of t (struct grant_table) is not fully defined yet. >>> >> Then, we could do like this: >> >> #define gnttab_shared_gfn(d, t, i) \ >> ({ \ >> const unsigned int _i = (i); \ >> const struct grant_table *_t = (t); \ >> (void)(d); \ >> (_i >= nr_grant_frames(_t)) ? INVALID_GFN \ >> : _t->arch.shared_gfn[_i]; \ >> }) > > Please avoid underscore-prefixed names here; we've started to use > underscore-suffixed names in a few macros. > > Additionally please consider using typeof() instead of spelling out > types. This may help to avoid surprising behavior. > > Finally, instead of merely casting d to void, please consider using it > in e.g. ASSERT((d)->grant_table == t_), which ought to also take care > of the unused variable warning. After all the explicit passing of t is > only an (attempted) optimization here. > >> However, if we start modifying the macros to evaluate args only once, shouldn't we also take care of the following macros in this file?: >> gnttab_set_frame_gfn >> gnttab_init_arch >> >> I'm ok to do these changes but I'm afriad we are losing the origin of this patch as we are focusing on macros not related to the issue. > > Indeed - I'd leave further ones to a subsequent patch, or make > conversion of all of the macros a prereq patch to the one you're after. > > Jan > Ok, so I will drop this patch and push a new series containing of 2 patches: 1. xen/arm: gnttab: use static inlines for gnttab_{release_}host_mapping* 2. xen/arm: gnttab: modify macros to evaluate all arguments and only once Cheers, Michal
diff --git a/xen/arch/arm/include/asm/grant_table.h b/xen/arch/arm/include/asm/grant_table.h index d31a4d6805..5bcd1ec528 100644 --- a/xen/arch/arm/include/asm/grant_table.h +++ b/xen/arch/arm/include/asm/grant_table.h @@ -31,10 +31,11 @@ static inline void gnttab_mark_dirty(struct domain *d, mfn_t mfn) int create_grant_host_mapping(unsigned long gpaddr, mfn_t mfn, unsigned int flags, unsigned int cache_flags); -#define gnttab_host_mapping_get_page_type(ro, ld, rd) (0) +#define gnttab_host_mapping_get_page_type(ro, ld, rd) \ + ((void)(ro), (void)(ld), (void)(rd), 0) int replace_grant_host_mapping(unsigned long gpaddr, mfn_t mfn, unsigned long new_gpaddr, unsigned int flags); -#define gnttab_release_host_mappings(domain) 1 +#define gnttab_release_host_mappings(domain) ((void)(domain), 1) /* * The region used by Xen on the memory will never be mapped in DOM0 @@ -89,10 +90,12 @@ int replace_grant_host_mapping(unsigned long gpaddr, mfn_t mfn, }) #define gnttab_shared_gfn(d, t, i) \ - (((i) >= nr_grant_frames(t)) ? INVALID_GFN : (t)->arch.shared_gfn[i]) + ((void)(d), \ + ((i) >= nr_grant_frames(t)) ? INVALID_GFN : (t)->arch.shared_gfn[i]) -#define gnttab_status_gfn(d, t, i) \ - (((i) >= nr_status_frames(t)) ? INVALID_GFN : (t)->arch.status_gfn[i]) +#define gnttab_status_gfn(d, t, i) \ + ((void)(d), \ + ((i) >= nr_status_frames(t)) ? INVALID_GFN : (t)->arch.status_gfn[i]) #define gnttab_need_iommu_mapping(d) \ (is_domain_direct_mapped(d) && is_iommu_enabled(d))
Function unmap_common_complete (common/grant_table.c) defines and sets a variable ld that is later on passed to a macro: gnttab_host_mapping_get_page_type(). On Arm this macro does not make use of any arguments causing a compiler to warn about unused-but-set variable (when -Wunused-but-set-variable is enabled). Fix it by casting the arguments to void in macro's body. While there, take the opportunity to modify other macros in this file that do not make use of all the arguments to prevent similar issues in the future. Signed-off-by: Michal Orzel <michal.orzel@arm.com> --- Changes since v1: -standalone patch carved out from a series (other patches already merged) -v1 was ([3/8] gnttab: Remove unused-but-set variable) -modify macro on Arm instead of removing ld variable --- xen/arch/arm/include/asm/grant_table.h | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-)