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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Thu, 5 May 2022, Jan Beulich wrote:
> 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.

I am fine with Jan's comments on both patches