Re: [PATCH 2/2] xen/arm: gnttab: modify macros to evaluate all arguments and only once

[Jan Beulich <jbeulich@xxxxxxxx>]

On 05.05.2022 13:25, Michal Orzel wrote:
> On 05.05.2022 13:20, Jan Beulich wrote:
>> On 05.05.2022 12:36, Michal Orzel wrote:
>>> Modify macros to evaluate all the arguments and make sure the arguments
>>> are evaluated only once. While doing so, use typeof for basic types
>>> and use const qualifier when applicable.
>>
>> Why only for basic types? To take an example, passing void * into
>> gnttab_need_iommu_mapping() would imo also better not work.
>>
> Just by looking at the majority of macros in Xen, typeof is used mostly for 
> basic data types.
> Also I think it is better to explictly use a struct type for better 
> readability.
> Otherwise one need to search in other files, to what type does typeof 
> evaluates.
> 
>>> @@ -98,13 +104,36 @@ 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])
>>> +    ({                                                                   \
>>> +        const struct domain *d_ = (d);                                   \
>>> +        const struct grant_table *t_ = (t);                              \
>>> +        const typeof(i) i_ = (i);                                        \
>>> +                                                                         \
>>> +        if ( d_ != NULL )                                                \
>>> +            ASSERT(d_->grant_table == t_);                               \
>>
>> I'm puzzled by this NULL check (and the similar instance further down):
>> Are you suggesting NULL can legitimately come into here? If not, maybe
>> better ASSERT(d_ && d_->grant_table == t_)?
>>
> Example:
> NULL is coming explictly from macro gnttab_get_frame_gfn right above 
> gnttab_shared_gfn.

Hmm, that's pretty odd (and Arm specific). Just like with the other remark
above, it'll be the Arm maintainers to judge, but here I think the NULLs
would better be done away with, by introducing intermediate macros, e.g.

#define gnttab_shared_gfn_(t, i) ...

#define gnttab_shared_gfn(d, t, i) ({                                  \
    const typeof(t) t_ = (t);                                          \
    ASSERT((d)->grant_table == t_);                                    \
    gnttab_shared_gfn_(t_, i);                                         \
})

#define gnttab_get_frame_gfn(gt, st, idx) ({                           \
   (st) ? gnttab_status_gfn_(gt, idx)                                  \
        : gnttab_shared_gfn_(gt, idx);                                 \
})

Jan