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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 23.10.2023 22:44, Stefano Stabellini wrote:
> On Mon, 23 Oct 2023, Jan Beulich wrote:
>> On 23.10.2023 15:19, Nicola Vetrini wrote:
>>> 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.
>>
>> While naming-wise okay to me, it has the same "too long" issue as
>> ISOLATE_LOW_BIT(). Considering x86, in the BMI ISA extension, has an
>> insn doing exactly that (BLSI), taking inspiration from its mnemonic
>> may also be an option.
> 
> I don't mean to make things difficult but I prefer LOWEST_BIT or
> MASK_LOWEST_BIT. It is clear at first glance. BLSI is not as clear,
> unless you work on the specific ISA with BLSI.
> 
> In general, I do appreciate shorter names, but if one option is much
> clearer than the other, I prefer clarity over shortness.

That's fine with me, but note that neither LOWEST_BIT nor MASK_LOWEST_BIT
really provide the aimed at clarity. The shortest that I could think of
that would be derived from that would be LOWEST_SET_BIT_MASK() (albeit
even that leaves a bit of ambiguity, thinking about it for a little
longer). The main point I'm trying to make that _if_ we need a wrapper
macro for this in the first place (note the other thread about macros
still requiring deviation comments at all use sites for Eclair), its name
needs to somehow express the precise operation it does (or, like would be
the case for BLSI, make people not recognizing the name go look rather
than guess).

Jan