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
