Re: [PATCH v2] xen/arm: gnttab: cast unused macro arguments to void

[Julien Grall <julien@xxxxxxx>]

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@xxxxxxx>
> ---
> 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

--
Julien Grall