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

[Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>]

On 24/10/2023 08:14, Jan Beulich wrote:
> 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
Ok, I'll send a revised version using MASK_LOWEST_BIT, taking into 
account also the
other comments about the explanation on the macro definition
(which some IDEs even show when hovering on its usage, which could 
partially address
the latter concern).

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