Re: [PATCH v3 1/2] xen/arm: exclude xen,reg from direct-map domU extended regions

[Stewart Hildebrand <stewart.hildebrand@xxxxxxx>]

On 5/14/25 02:49, Jan Beulich wrote:
> On 13.05.2025 21:54, Stewart Hildebrand wrote:
>> --- a/xen/common/rangeset.c
>> +++ b/xen/common/rangeset.c
>> @@ -433,6 +433,20 @@ bool rangeset_is_empty(
>>      return ((r == NULL) || list_empty(&r->range_list));
>>  }
>>  
>> +int rangeset_count_ranges(const struct rangeset *r)
>> +{
>> +    int nr = 0;
> 
> Ehem - this and the function's return type want to be unsigned.
> 
>> +    struct list_head *list;
>> +
>> +    if ( r == NULL )
>> +        return 0;
>> +
>> +    list_for_each( list, &r->range_list )
> 
> Nit: Either you deem list_for_each a pseudo-keyword (then a blank is
> missing) or you don't (then there are excess blanks).
> 
> Further I don't think this is valid to do without holding the rangeset's
> lock in read mode (irrespective of the function return value potentially
> being stale by the time the caller gets to look at it, which is no
> different from other functions, i.e. falls in the caller's
> responsibilities).
> 
>> +        nr++;
> 
> And then, if already abstraction is wanted, wouldn't this loop better be
> yet another helper (macro?) in xen/list.h?
> 
>> +    return nr;
>> +}
> 
> Finally: If this is to be commonly used in several places, having such a
> helper is likely fine. As it stands, the sole caller is an __init
> function, and hence this is unreachable code post-init (which while not
> formally a Misra violation in my eyes effectively still is one). Aiui
> the same can be achieved using rangeset_report_ranges(), with a new
> (__init and static) callback function.

Moving rangeset_count_ranges() to rangeset.h might change the situation
slightly, but I'm not aware of any other potential callers so I'll just
use rangeset_report_ranges().