Re: [PATCH 3/8] gnttab: Remove unused-but-set variable

[Michal Orzel <michal.orzel@xxxxxxx>]

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