| Message ID | 20220427094941.291554-4-michal.orzel@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen: Remove unused-but-set variables | expand |
On 27.04.2022 11:49, Michal Orzel wrote: > Function unmap_common_complete 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 this by removing ld and directly passing current->domain > to gnttab_host_mapping_get_page_type. I think we want to retain the ld / rd notation. Therefore I think it's rather the Arm macro which wants adjusting to not leave this argument unused. Jan
Hi Jan, On 27.04.2022 11:59, Jan Beulich wrote: > On 27.04.2022 11:49, Michal Orzel wrote: >> Function unmap_common_complete 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 this by removing ld and directly passing current->domain >> to gnttab_host_mapping_get_page_type. > > I think we want to retain the ld / rd notation. Therefore I think it's > rather the Arm macro which wants adjusting to not leave this argument > unused. > I would agree provided that the ld variable was used in more than one place. As it is not, it does not seem very beneficial to keep a variable that is used just in one place and stores the macro value. When it comes to gnttab_host_mapping_get_page_type macro, on Arm it is defined as (0) so modifying it seems to be a quite big overhead. > Jan > Cheers, Michal
On 27/04/2022 12:06, Michal Orzel wrote: > Hi Jan, > > On 27.04.2022 11:59, Jan Beulich wrote: >> On 27.04.2022 11:49, Michal Orzel wrote: >>> Function unmap_common_complete 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 this by removing ld and directly passing current->domain >>> to gnttab_host_mapping_get_page_type. >> I think we want to retain the ld / rd notation. Therefore I think it's >> rather the Arm macro which wants adjusting to not leave this argument >> unused. >> > I would agree provided that the ld variable was used in more than one place. > As it is not, it does not seem very beneficial to keep a variable that is used > just in one place and stores the macro value. > > When it comes to gnttab_host_mapping_get_page_type macro, on Arm it is defined as (0) > so modifying it seems to be a quite big overhead. diff --git a/xen/arch/arm/include/asm/grant_table.h b/xen/arch/arm/include/asm/grant_table.h index d31a4d6805d6..9f68c2a37eb6 100644 --- a/xen/arch/arm/include/asm/grant_table.h +++ b/xen/arch/arm/include/asm/grant_table.h @@ -31,10 +31,10 @@ 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) (ro, ld, 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) (domain, 1) /* * The region used by Xen on the memory will never be mapped in DOM0 It's about parameter evaluation, not about adding extra code when compiled. ~Andrew
Hi Andrew, On 27.04.2022 14:33, Andrew Cooper wrote: > On 27/04/2022 12:06, Michal Orzel wrote: >> Hi Jan, >> >> On 27.04.2022 11:59, Jan Beulich wrote: >>> On 27.04.2022 11:49, Michal Orzel wrote: >>>> Function unmap_common_complete 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 this by removing ld and directly passing current->domain >>>> to gnttab_host_mapping_get_page_type. >>> I think we want to retain the ld / rd notation. Therefore I think it's >>> rather the Arm macro which wants adjusting to not leave this argument >>> unused. >>> >> I would agree provided that the ld variable was used in more than one place. >> As it is not, it does not seem very beneficial to keep a variable that is used >> just in one place and stores the macro value. >> >> When it comes to gnttab_host_mapping_get_page_type macro, on Arm it is defined as (0) >> so modifying it seems to be a quite big overhead. > > diff --git a/xen/arch/arm/include/asm/grant_table.h > b/xen/arch/arm/include/asm/grant_table.h > index d31a4d6805d6..9f68c2a37eb6 100644 > --- a/xen/arch/arm/include/asm/grant_table.h > +++ b/xen/arch/arm/include/asm/grant_table.h > @@ -31,10 +31,10 @@ 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) (ro, ld, 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) (domain, 1) > > /* > * The region used by Xen on the memory will never be mapped in DOM0 > > It's about parameter evaluation, not about adding extra code when compiled. > You're right, thanks. I will do it your way in v2. > ~Andrew Cheers, Michal
Hi Andrew, Jan On 27.04.2022 14:33, Andrew Cooper wrote: > On 27/04/2022 12:06, Michal Orzel wrote: >> Hi Jan, >> >> On 27.04.2022 11:59, Jan Beulich wrote: >>> On 27.04.2022 11:49, Michal Orzel wrote: >>>> Function unmap_common_complete 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 this by removing ld and directly passing current->domain >>>> to gnttab_host_mapping_get_page_type. >>> I think we want to retain the ld / rd notation. Therefore I think it's >>> rather the Arm macro which wants adjusting to not leave this argument >>> unused. >>> >> I would agree provided that the ld variable was used in more than one place. >> As it is not, it does not seem very beneficial to keep a variable that is used >> just in one place and stores the macro value. >> >> When it comes to gnttab_host_mapping_get_page_type macro, on Arm it is defined as (0) >> so modifying it seems to be a quite big overhead. > > diff --git a/xen/arch/arm/include/asm/grant_table.h > b/xen/arch/arm/include/asm/grant_table.h > index d31a4d6805d6..9f68c2a37eb6 100644 > --- a/xen/arch/arm/include/asm/grant_table.h > +++ b/xen/arch/arm/include/asm/grant_table.h > @@ -31,10 +31,10 @@ 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) (ro, ld, 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) (domain, 1) > > /* > * The region used by Xen on the memory will never be mapped in DOM0 > > It's about parameter evaluation, not about adding extra code when compiled. > Unfortunately, solution presented by Andrew does not work. We will get the following error due to -Werror=unused-value: "left-hand operand of comma expression has no effect" If we want to keep this variable, how about using unused attribute? struct domain *__maybe_unused ld Cheers, Michal
On 28.04.2022 09:16, Michal Orzel wrote: > Hi Andrew, Jan > > On 27.04.2022 14:33, Andrew Cooper wrote: >> On 27/04/2022 12:06, Michal Orzel wrote: >>> Hi Jan, >>> >>> On 27.04.2022 11:59, Jan Beulich wrote: >>>> On 27.04.2022 11:49, Michal Orzel wrote: >>>>> Function unmap_common_complete 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 this by removing ld and directly passing current->domain >>>>> to gnttab_host_mapping_get_page_type. >>>> I think we want to retain the ld / rd notation. Therefore I think it's >>>> rather the Arm macro which wants adjusting to not leave this argument >>>> unused. >>>> >>> I would agree provided that the ld variable was used in more than one place. >>> As it is not, it does not seem very beneficial to keep a variable that is used >>> just in one place and stores the macro value. >>> >>> When it comes to gnttab_host_mapping_get_page_type macro, on Arm it is defined as (0) >>> so modifying it seems to be a quite big overhead. >> >> diff --git a/xen/arch/arm/include/asm/grant_table.h >> b/xen/arch/arm/include/asm/grant_table.h >> index d31a4d6805d6..9f68c2a37eb6 100644 >> --- a/xen/arch/arm/include/asm/grant_table.h >> +++ b/xen/arch/arm/include/asm/grant_table.h >> @@ -31,10 +31,10 @@ 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) (ro, ld, 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) (domain, 1) >> >> /* >> * The region used by Xen on the memory will never be mapped in DOM0 >> >> It's about parameter evaluation, not about adding extra code when compiled. >> > > Unfortunately, solution presented by Andrew does not work. > We will get the following error due to -Werror=unused-value: > "left-hand operand of comma expression has no effect" Perhaps #define gnttab_host_mapping_get_page_type(ro, ld, rd) \ ((void)(ro), (void)(ld), (void)(rd), 0) ? Jan
On 28.04.2022 09:27, Jan Beulich wrote: >>> diff --git a/xen/arch/arm/include/asm/grant_table.h >>> b/xen/arch/arm/include/asm/grant_table.h >>> index d31a4d6805d6..9f68c2a37eb6 100644 >>> --- a/xen/arch/arm/include/asm/grant_table.h >>> +++ b/xen/arch/arm/include/asm/grant_table.h >>> @@ -31,10 +31,10 @@ 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) (ro, ld, 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) (domain, 1) >>> >>> /* >>> * The region used by Xen on the memory will never be mapped in DOM0 >>> >>> It's about parameter evaluation, not about adding extra code when compiled. >>> >> >> Unfortunately, solution presented by Andrew does not work. >> We will get the following error due to -Werror=unused-value: >> "left-hand operand of comma expression has no effect" > > Perhaps > > #define gnttab_host_mapping_get_page_type(ro, ld, rd) \ > ((void)(ro), (void)(ld), (void)(rd), 0) > > ? I already tried that and it won't work producing the following: "error: void value not ignored as it ought to be" Michal
On 28.04.2022 09:32, Michal Orzel wrote: > > > On 28.04.2022 09:27, Jan Beulich wrote: >>>> diff --git a/xen/arch/arm/include/asm/grant_table.h >>>> b/xen/arch/arm/include/asm/grant_table.h >>>> index d31a4d6805d6..9f68c2a37eb6 100644 >>>> --- a/xen/arch/arm/include/asm/grant_table.h >>>> +++ b/xen/arch/arm/include/asm/grant_table.h >>>> @@ -31,10 +31,10 @@ 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) (ro, ld, 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) (domain, 1) >>>> >>>> /* >>>> * The region used by Xen on the memory will never be mapped in DOM0 >>>> >>>> It's about parameter evaluation, not about adding extra code when compiled. >>>> >>> >>> Unfortunately, solution presented by Andrew does not work. >>> We will get the following error due to -Werror=unused-value: >>> "left-hand operand of comma expression has no effect" >> >> Perhaps >> >> #define gnttab_host_mapping_get_page_type(ro, ld, rd) \ >> ((void)(ro), (void)(ld), (void)(rd), 0) >> >> ? > I already tried that and it won't work producing the following: > "error: void value not ignored as it ought to be" > > Michal Sorry about that but I was wrong. Your solution does work. I did not enclose the params in parentheses. Michal
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c index febbe12eab..71b1107999 100644 --- a/xen/common/grant_table.c +++ b/xen/common/grant_table.c @@ -1556,7 +1556,7 @@ unmap_common( static void unmap_common_complete(struct gnttab_unmap_common *op) { - struct domain *ld, *rd = op->rd; + struct domain *rd = op->rd; struct grant_table *rgt; struct active_grant_entry *act; grant_entry_header_t *sha; @@ -1569,8 +1569,6 @@ unmap_common_complete(struct gnttab_unmap_common *op) return; } - ld = current->domain; - rcu_lock_domain(rd); rgt = rd->grant_table; @@ -1608,7 +1606,7 @@ unmap_common_complete(struct gnttab_unmap_common *op) if ( pg ) { if ( gnttab_host_mapping_get_page_type(op->done & GNTMAP_readonly, - ld, rd) ) + current->domain, rd) ) put_page_type(pg); put_page(pg); }
Function unmap_common_complete 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 this by removing ld and directly passing current->domain to gnttab_host_mapping_get_page_type. Signed-off-by: Michal Orzel <michal.orzel@arm.com> --- xen/common/grant_table.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)