Re: [XEN PATCH][for-4.19 v3 1/8] xen/include: add macro LOWEST_BIT

[Nicola Vetrini <nicola.vetrini@bugseng.com>]

On 23/10/2023 09:48, Jan Beulich wrote:
> On 20.10.2023 17:28, Nicola Vetrini wrote:
>> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl
>> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
>> @@ -246,6 +246,12 @@ constant expressions are required.\""
>>    "any()"}
>>  -doc_end
>> 
>> +-doc_begin="The macro LOWEST_BIT encapsulates a well-known pattern to 
>> obtain the value
>> +2^ffs(x) for unsigned integers on two's complement architectures
>> +(all the architectures supported by Xen satisfy this requirement)."
>> +-config=MC3R1.R10.1,reports+={safe, 
>> "any_area(any_loc(any_exp(macro(^LOWEST_BIT$))))"}
>> +-doc_end
> 
> This being deviated this way (rather than by SAF-* comments) wants
> justifying in the description. You did reply to my respective
> comment on v2, but such then (imo) needs propagating into the actual
> patch as well.
> 

Sure, will do.

>> --- a/docs/misra/deviations.rst
>> +++ b/docs/misra/deviations.rst
>> @@ -192,6 +192,13 @@ Deviations related to MISRA C:2012 Rules:
>>         See automation/eclair_analysis/deviations.ecl for the full 
>> explanation.
>>       - Tagged as `safe` for ECLAIR.
>> 
>> +   * - R10.1
>> +     - The well-known pattern (x & -x) applied to unsigned integer 
>> values on 2's
>> +       complement architectures (i.e., all architectures supported by 
>> Xen), used
>> +       to obtain the value 2^ffs(x), where ffs(x) is the position of 
>> the first
>> +       bit set. If no bits are set, zero is returned.
>> +     - Tagged as `safe` for ECLAIR.
> 
> In such an explanation there shall not be any ambiguity. Here I see
> an issue with ffs() returning 1-based bit position numbers, which
> isn't in line with the use in 2^ffs(x). (Arguably use of ^ itself is
> also problematic, as that's XOR in C, not POW. I'd suggest 2^^ffs(x)
> or 2**ffs(x).)
> 

Makes sense, I think I'll use an plain english explanation to avoid any 
confusion
about notation.

>> --- a/xen/include/xen/macros.h
>> +++ b/xen/include/xen/macros.h
>> @@ -8,8 +8,11 @@
>>  #define DIV_ROUND(n, d) (((n) + (d) / 2) / (d))
>>  #define DIV_ROUND_UP(n, d) (((n) + (d) - 1) / (d))
>> 
>> -#define MASK_EXTR(v, m) (((v) & (m)) / ((m) & -(m)))
>> -#define MASK_INSR(v, m) (((v) * ((m) & -(m))) & (m))
>> +/* Returns the 2^ffs(x) or 0, where ffs(x) is the index of the lowest 
>> set bit */
>> +#define LOWEST_BIT(x) ((x) & -(x))
> 
> I'm afraid my concern regarding this new macro's name (voiced on v2) 
> hasn't
> been addressed (neither verbally nor by finding a more suitable name).
> 
> Jan

I didn't come up with much better names, so I left it as is. Here's a 
few ideas:

- LOWEST_SET
- MASK_LOWEST_SET
- MASK_LOWEST_BIT
- MASK_FFS_0
- LOWEST_ONE

and also yours, included here for convenience, even though you felt it 
was too long:

- ISOLATE_LOW_BIT()

All maintainers from THE REST are CC-ed, so we can see if anyone has any 
suggestion.

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)