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

[Michal Orzel <michal.orzel@xxxxxxx>]

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