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 11:19, Nicola Vetrini wrote:
> 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.

There's also LOWEST_BIT_MASK as another possible name.

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