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

[Michal Orzel <michal.orzel@xxxxxxx>]

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