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?