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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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.

Maybe we need a third opinion here to make progress a bit faster.
Julien?