[XEN PATCH][for-4.19 v3 0/8] address violations of MISRA C:2012 Rule 10.1

[XEN PATCH][for-4.19 v3 0/8] address violations of MISRA C:2012 Rule 10.1

[Nicola Vetrini]

The widely-used construct
(x & -x), where x is an unsigned integer quantity represented in 2's complement,
does yield the expected result. Since all architectures that are targets for
compliance do fulfill such requirements, the construct is deemed safe and
deviated.

The use of 'DECLARE_BITMAP(features, IOMMU_FEAT_count);' is deviated, to avoid
harming code readability.

On the contrary, other uses of inappropriate types are changed.

Changes in v3:
- Patch [6/8] has already been picked up in Andrew's for-next tree

Nicola Vetrini (8):
  xen/include: add macro LOWEST_BIT
  arm/bitops: encapsulate violation of MISRA C:2012 Rule 10.1
  xen/pdx: amend definition of PDX_GROUP_COUNT
  x86_64/mm: express macro CNT using LOWEST_BIT
  x86/io_apic: address violation of MISRA C:2012 Rule 10.1
  x86/mce: Move MC_NCLASSES into the enum mctelem_class
  xen/types: address Rule 10.1 for DECLARE_BITMAP use
  xen/compat: use BUILD_BUG_ON in CHECK_SIZE macros

 automation/eclair_analysis/ECLAIR/deviations.ecl |  6 ++++++
 docs/misra/deviations.rst                        |  7 +++++++
 docs/misra/safe.json                             |  8 ++++++++
 xen/arch/arm/include/asm/bitops.h                |  6 ++++--
 xen/arch/x86/cpu/mcheck/mctelem.c                |  2 --
 xen/arch/x86/cpu/mcheck/mctelem.h                |  5 +++--
 xen/arch/x86/include/asm/io_apic.h               |  7 ++++---
 xen/arch/x86/x86_64/mm.c                         | 12 ++++++------
 xen/include/xen/compat.h                         | 16 +++++++++++-----
 xen/include/xen/iommu.h                          |  1 +
 xen/include/xen/macros.h                         |  7 +++++--
 xen/include/xen/pdx.h                            |  2 +-
 xen/include/xen/types.h                          | 15 ++++++++++-----
 13 files changed, 66 insertions(+), 28 deletions(-)

--
2.34.1

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

[Nicola Vetrini]

The purpose of this macro is to encapsulate the well-known expression
'x & -x', that in 2's complement architectures on unsigned integers will
give 2^ffs(x), where ffs(x) is the position of the lowest set bit in x.

A deviation for ECLAIR is also introduced.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
Changes in v2:
- rename to LOWEST_BIT
Changes in v3:
- entry for deviations.rst
- comment on the macro defn
---
 automation/eclair_analysis/ECLAIR/deviations.ecl |  6 ++++++
 docs/misra/deviations.rst                        |  7 +++++++
 xen/include/xen/macros.h                         |  7 +++++--
 xen/include/xen/types.h                          | 10 +++++-----
 4 files changed, 23 insertions(+), 7 deletions(-)

diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl b/automation/eclair_analysis/ECLAIR/deviations.ecl
index fa56e5c00a27..9390f6d839ff 100644
--- 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
+
 #
 # Series 13
 #
diff --git a/docs/misra/deviations.rst b/docs/misra/deviations.rst
index 8511a189253b..c3c65f4a9454 100644
--- 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.
+
    * - R13.5
      - All developers and reviewers can be safely assumed to be well aware of
        the short-circuit evaluation strategy for logical operators.
diff --git a/xen/include/xen/macros.h b/xen/include/xen/macros.h
index d0caae7db298..49f3ebf848e9 100644
--- 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))
+
+#define MASK_EXTR(v, m) (((v) & (m)) / LOWEST_BIT(m))
+#define MASK_INSR(v, m) (((v) * LOWEST_BIT(m)) & (m))
 
 #define count_args_(dot, a1, a2, a3, a4, a5, a6, a7, a8, x, ...) x
 #define count_args(args...) \
diff --git a/xen/include/xen/types.h b/xen/include/xen/types.h
index aea259db1ef2..23cad71c8a47 100644
--- a/xen/include/xen/types.h
+++ b/xen/include/xen/types.h
@@ -31,9 +31,9 @@ typedef __PTRDIFF_TYPE__ ptrdiff_t;
 #define NULL ((void*)0)
 #endif
 
-#define INT8_MIN        (-127-1)
-#define INT16_MIN       (-32767-1)
-#define INT32_MIN       (-2147483647-1)
+#define INT8_MIN        (-127 - 1)
+#define INT16_MIN       (-32767 - 1)
+#define INT32_MIN       (-2147483647 - 1)
 
 #define INT8_MAX        (127)
 #define INT16_MAX       (32767)
@@ -43,10 +43,10 @@ typedef __PTRDIFF_TYPE__ ptrdiff_t;
 #define UINT16_MAX      (65535)
 #define UINT32_MAX      (4294967295U)
 
-#define INT_MAX         ((int)(~0U>>1))
+#define INT_MAX         ((int)(~0U >> 1))
 #define INT_MIN         (-INT_MAX - 1)
 #define UINT_MAX        (~0U)
-#define LONG_MAX        ((long)(~0UL>>1))
+#define LONG_MAX        ((long)(~0UL >> 1))
 #define LONG_MIN        (-LONG_MAX - 1)
 #define ULONG_MAX       (~0UL)
 
-- 
2.34.1

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

[Julien Grall]

Hi Nicola,

On 20/10/2023 16:28, Nicola Vetrini wrote:
> diff --git a/xen/include/xen/macros.h b/xen/include/xen/macros.h
> index d0caae7db298..49f3ebf848e9 100644
> --- 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))
> +
> +#define MASK_EXTR(v, m) (((v) & (m)) / LOWEST_BIT(m))
> +#define MASK_INSR(v, m) (((v) * LOWEST_BIT(m)) & (m))
>   
>   #define count_args_(dot, a1, a2, a3, a4, a5, a6, a7, a8, x, ...) x
>   #define count_args(args...) \
> diff --git a/xen/include/xen/types.h b/xen/include/xen/types.h
> index aea259db1ef2..23cad71c8a47 100644
> --- a/xen/include/xen/types.h
> +++ b/xen/include/xen/types.h

I don't understand how the changes in this file are related to the 
commit. Did you intend to create a separate commit?

The rest LGTM.

> @@ -31,9 +31,9 @@ typedef __PTRDIFF_TYPE__ ptrdiff_t;
>   #define NULL ((void*)0)
>   #endif
>   
> -#define INT8_MIN        (-127-1)
> -#define INT16_MIN       (-32767-1)
> -#define INT32_MIN       (-2147483647-1)
> +#define INT8_MIN        (-127 - 1)
> +#define INT16_MIN       (-32767 - 1)
> +#define INT32_MIN       (-2147483647 - 1)
>   
>   #define INT8_MAX        (127)
>   #define INT16_MAX       (32767)
> @@ -43,10 +43,10 @@ typedef __PTRDIFF_TYPE__ ptrdiff_t;
>   #define UINT16_MAX      (65535)
>   #define UINT32_MAX      (4294967295U)
>   
> -#define INT_MAX         ((int)(~0U>>1))
> +#define INT_MAX         ((int)(~0U >> 1))
>   #define INT_MIN         (-INT_MAX - 1)
>   #define UINT_MAX        (~0U)
> -#define LONG_MAX        ((long)(~0UL>>1))
> +#define LONG_MAX        ((long)(~0UL >> 1))
>   #define LONG_MIN        (-LONG_MAX - 1)
>   #define ULONG_MAX       (~0UL)
>   

Cheers,

-- 
Julien Grall

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

[Nicola Vetrini]

On 20/10/2023 19:03, Julien Grall wrote:
> Hi Nicola,
> 
> On 20/10/2023 16:28, Nicola Vetrini wrote:
>> diff --git a/xen/include/xen/macros.h b/xen/include/xen/macros.h
>> index d0caae7db298..49f3ebf848e9 100644
>> --- 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))
>> +
>> +#define MASK_EXTR(v, m) (((v) & (m)) / LOWEST_BIT(m))
>> +#define MASK_INSR(v, m) (((v) * LOWEST_BIT(m)) & (m))
>>     #define count_args_(dot, a1, a2, a3, a4, a5, a6, a7, a8, x, ...) x
>>   #define count_args(args...) \
>> diff --git a/xen/include/xen/types.h b/xen/include/xen/types.h
>> index aea259db1ef2..23cad71c8a47 100644
>> --- a/xen/include/xen/types.h
>> +++ b/xen/include/xen/types.h
> 
> I don't understand how the changes in this file are related to the
> commit. Did you intend to create a separate commit?
> 
> The rest LGTM.
> 

Oh, I made a mistake. This hunk should have been part of patch 7 as a 
cleanup.
I can resubmit with this removed, or it could be ignored altogether.

>> @@ -31,9 +31,9 @@ typedef __PTRDIFF_TYPE__ ptrdiff_t;
>>   #define NULL ((void*)0)
>>   #endif
>>   -#define INT8_MIN        (-127-1)
>> -#define INT16_MIN       (-32767-1)
>> -#define INT32_MIN       (-2147483647-1)
>> +#define INT8_MIN        (-127 - 1)
>> +#define INT16_MIN       (-32767 - 1)
>> +#define INT32_MIN       (-2147483647 - 1)
>>     #define INT8_MAX        (127)
>>   #define INT16_MAX       (32767)
>> @@ -43,10 +43,10 @@ typedef __PTRDIFF_TYPE__ ptrdiff_t;
>>   #define UINT16_MAX      (65535)
>>   #define UINT32_MAX      (4294967295U)
>>   -#define INT_MAX         ((int)(~0U>>1))
>> +#define INT_MAX         ((int)(~0U >> 1))
>>   #define INT_MIN         (-INT_MAX - 1)
>>   #define UINT_MAX        (~0U)
>> -#define LONG_MAX        ((long)(~0UL>>1))
>> +#define LONG_MAX        ((long)(~0UL >> 1))
>>   #define LONG_MIN        (-LONG_MAX - 1)
>>   #define ULONG_MAX       (~0UL)
>> 
> 
> Cheers,

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

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

[Jan Beulich]

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.

> --- 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).)

> --- 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

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

[Nicola Vetrini]

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.

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

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

[Nicola Vetrini]

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)

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

[Jan Beulich]

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.

Jan

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

[Stefano Stabellini]

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?

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

[Jan Beulich]

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

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

[Nicola Vetrini]

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)

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

[Jan Beulich]

On 25.10.2023 16:50, Nicola Vetrini wrote:
> 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).

You're of course free to do so, but since - as indicated before -
MASK_LOWEST_BIT() imo isn't a better name than LOWEST_BIT(), I'll
continue to object.

Jan

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

[Stefano Stabellini]

On Wed, 25 Oct 2023, Jan Beulich wrote:
> On 25.10.2023 16:50, Nicola Vetrini wrote:
> > 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).
> 
> You're of course free to do so, but since - as indicated before -
> MASK_LOWEST_BIT() imo isn't a better name than LOWEST_BIT(), I'll
> continue to object.

Jan if you are OK with that I'll ask Julien to break the tie and pick
the name to use. Julien can you please help us move forward?

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

[Jan Beulich]

On 26.10.2023 00:38, Stefano Stabellini wrote:
> On Wed, 25 Oct 2023, Jan Beulich wrote:
>> On 25.10.2023 16:50, Nicola Vetrini wrote:
>>> 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).
>>
>> You're of course free to do so, but since - as indicated before -
>> MASK_LOWEST_BIT() imo isn't a better name than LOWEST_BIT(), I'll
>> continue to object.
> 
> Jan if you are OK with that I'll ask Julien to break the tie and pick
> the name to use. Julien can you please help us move forward?

Hmm, I'm having trouble seeing us at the point of breaking ties yet.
First we need naming suggestions which actually unambiguously
describe what's being done by the macro. I gave one suggestion which
I think fulfills this property, but is a little too long for my
taste. I gave another suggestion with a far-off but shorter name,
which I can appreciate isn't liked. I've not seen other suggestions
fulfilling this base criteria.

Jan

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

[Nicola Vetrini]

On 26/10/2023 08:52, Jan Beulich wrote:
> On 26.10.2023 00:38, Stefano Stabellini wrote:
>> On Wed, 25 Oct 2023, Jan Beulich wrote:
>>> On 25.10.2023 16:50, Nicola Vetrini wrote:
>>>> 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).
>>> 
>>> You're of course free to do so, but since - as indicated before -
>>> MASK_LOWEST_BIT() imo isn't a better name than LOWEST_BIT(), I'll
>>> continue to object.
>> 
>> Jan if you are OK with that I'll ask Julien to break the tie and pick
>> the name to use. Julien can you please help us move forward?
> 
> Hmm, I'm having trouble seeing us at the point of breaking ties yet.
> First we need naming suggestions which actually unambiguously
> describe what's being done by the macro. I gave one suggestion which
> I think fulfills this property, but is a little too long for my
> taste. I gave another suggestion with a far-off but shorter name,
> which I can appreciate isn't liked. I've not seen other suggestions
> fulfilling this base criteria.
> 
> Jan

Any name is fine with me. ISOLATE_LOW_BIT may be longish, but the macro 
would be used
in just a few places for a specific reason, so the loss in readability 
is probably not
that high.

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

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

[Julien Grall]

Hi,

On 26/10/2023 11:32, Nicola Vetrini wrote:
> On 26/10/2023 08:52, Jan Beulich wrote:
>> On 26.10.2023 00:38, Stefano Stabellini wrote:
>>> On Wed, 25 Oct 2023, Jan Beulich wrote:
>>>> On 25.10.2023 16:50, Nicola Vetrini wrote:
>>>>> 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).
>>>>
>>>> You're of course free to do so, but since - as indicated before -
>>>> MASK_LOWEST_BIT() imo isn't a better name than LOWEST_BIT(), I'll
>>>> continue to object.
>>>
>>> Jan if you are OK with that I'll ask Julien to break the tie and pick
>>> the name to use. Julien can you please help us move forward?
>>
>> Hmm, I'm having trouble seeing us at the point of breaking ties yet.
>> First we need naming suggestions which actually unambiguously
>> describe what's being done by the macro. I gave one suggestion which
>> I think fulfills this property, but is a little too long for my
>> taste. I gave another suggestion with a far-off but shorter name,
>> which I can appreciate isn't liked. I've not seen other suggestions
>> fulfilling this base criteria.
>>
>> Jan
> 
> Any name is fine with me. ISOLATE_LOW_BIT may be longish, but the macro 
> would be used
> in just a few places for a specific reason, so the loss in readability 
> is probably not
> that high.

+1. It doesn't seem we will be able to find a name that 100% fit all the 
criteria. So of all the choice, my preference would be ISOLATE_LOW_BIT().

Cheers,

-- 
Julien Grall

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

[Stefano Stabellini]

On Thu, 26 Oct 2023, Julien Grall wrote:
> On 26/10/2023 11:32, Nicola Vetrini wrote:
> > On 26/10/2023 08:52, Jan Beulich wrote:
> > > On 26.10.2023 00:38, Stefano Stabellini wrote:
> > > > On Wed, 25 Oct 2023, Jan Beulich wrote:
> > > > > On 25.10.2023 16:50, Nicola Vetrini wrote:
> > > > > > 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).
> > > > > 
> > > > > You're of course free to do so, but since - as indicated before -
> > > > > MASK_LOWEST_BIT() imo isn't a better name than LOWEST_BIT(), I'll
> > > > > continue to object.
> > > > 
> > > > Jan if you are OK with that I'll ask Julien to break the tie and pick
> > > > the name to use. Julien can you please help us move forward?
> > > 
> > > Hmm, I'm having trouble seeing us at the point of breaking ties yet.
> > > First we need naming suggestions which actually unambiguously
> > > describe what's being done by the macro. I gave one suggestion which
> > > I think fulfills this property, but is a little too long for my
> > > taste. I gave another suggestion with a far-off but shorter name,
> > > which I can appreciate isn't liked. I've not seen other suggestions
> > > fulfilling this base criteria.
> > > 
> > > Jan
> > 
> > Any name is fine with me. ISOLATE_LOW_BIT may be longish, but the macro
> > would be used
> > in just a few places for a specific reason, so the loss in readability is
> > probably not
> > that high.
> 
> +1. It doesn't seem we will be able to find a name that 100% fit all the
> criteria. So of all the choice, my preference would be ISOLATE_LOW_BIT().

It is clear enough, so +1

[XEN PATCH][for-4.19 v3 2/8] arm/bitops: encapsulate violation of MISRA C:2012 Rule 10.1

[Nicola Vetrini]

The definitions of ffs{l}? violate Rule 10.1, by using the well-known
pattern (x & -x); its usage is wrapped by the LOWEST_BIT macro.

No functional change.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
---
 xen/arch/arm/include/asm/bitops.h | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/include/asm/bitops.h b/xen/arch/arm/include/asm/bitops.h
index 71ae14cab355..8b5d61545e19 100644
--- a/xen/arch/arm/include/asm/bitops.h
+++ b/xen/arch/arm/include/asm/bitops.h
@@ -9,6 +9,8 @@
 #ifndef _ARM_BITOPS_H
 #define _ARM_BITOPS_H
 
+#include <xen/macros.h>
+
 #include <asm/asm_defns.h>
 
 /*
@@ -155,8 +157,8 @@ static inline int fls(unsigned int x)
 }
 
 
-#define ffs(x) ({ unsigned int __t = (x); fls(__t & -__t); })
-#define ffsl(x) ({ unsigned long __t = (x); flsl(__t & -__t); })
+#define ffs(x) ({ unsigned int __t = (x); fls(LOWEST_BIT(__t)); })
+#define ffsl(x) ({ unsigned long __t = (x); flsl(LOWEST_BIT(__t)); })
 
 /**
  * find_first_set_bit - find the first set bit in @word
-- 
2.34.1

[XEN PATCH][for-4.19 v3 3/8] xen/pdx: amend definition of PDX_GROUP_COUNT

[Nicola Vetrini]

The definition of PDX_GROUP_COUNT causes violations of
MISRA C:2012 Rule 10.1, therefore the problematic part now uses
the LOWEST_BIT macro, which encapsulates the pattern.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
---
 xen/include/xen/pdx.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/include/xen/pdx.h b/xen/include/xen/pdx.h
index f3fbc4273aa4..36d618a8ba7d 100644
--- a/xen/include/xen/pdx.h
+++ b/xen/include/xen/pdx.h
@@ -72,7 +72,7 @@
 extern unsigned long max_pdx;
 
 #define PDX_GROUP_COUNT ((1 << PDX_GROUP_SHIFT) / \
-                         (sizeof(*frame_table) & -sizeof(*frame_table)))
+                         (LOWEST_BIT(sizeof(*frame_table))))
 extern unsigned long pdx_group_valid[];
 
 /**
-- 
2.34.1

[XEN PATCH][for-4.19 v3 4/8] x86_64/mm: express macro CNT using LOWEST_BIT

[Nicola Vetrini]

The various definitions of macro CNT (and the related BUILD_BUG_ON)
can be rewritten using LOWEST_BIT, encapsulating a violation of
MISRA C:2012 Rule 10.1.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
---
 xen/arch/x86/x86_64/mm.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/xen/arch/x86/x86_64/mm.c b/xen/arch/x86/x86_64/mm.c
index c3ebb777144a..0eb7b71124f5 100644
--- a/xen/arch/x86/x86_64/mm.c
+++ b/xen/arch/x86/x86_64/mm.c
@@ -351,9 +351,9 @@ static int setup_compat_m2p_table(struct mem_hotadd_info *info)
                 ~((1UL << (L2_PAGETABLE_SHIFT - 2)) - 1) );
 
 #define MFN(x) (((x) << L2_PAGETABLE_SHIFT) / sizeof(unsigned int))
-#define CNT ((sizeof(*frame_table) & -sizeof(*frame_table)) / \
+#define CNT (LOWEST_BIT(sizeof(*frame_table)) / \
              sizeof(*compat_machine_to_phys_mapping))
-    BUILD_BUG_ON((sizeof(*frame_table) & -sizeof(*frame_table)) % \
+    BUILD_BUG_ON(LOWEST_BIT(sizeof(*frame_table)) % \
                  sizeof(*compat_machine_to_phys_mapping));
 
     for ( i = smap; i < emap; i += (1UL << (L2_PAGETABLE_SHIFT - 2)) )
@@ -410,10 +410,10 @@ static int setup_m2p_table(struct mem_hotadd_info *info)
     va = RO_MPT_VIRT_START + smap * sizeof(*machine_to_phys_mapping);
 
 #define MFN(x) (((x) << L2_PAGETABLE_SHIFT) / sizeof(unsigned long))
-#define CNT ((sizeof(*frame_table) & -sizeof(*frame_table)) / \
+#define CNT (LOWEST_BIT(sizeof(*frame_table)) / \
              sizeof(*machine_to_phys_mapping))
 
-    BUILD_BUG_ON((sizeof(*frame_table) & -sizeof(*frame_table)) % \
+    BUILD_BUG_ON(LOWEST_BIT(sizeof(*frame_table)) % \
                  sizeof(*machine_to_phys_mapping));
 
     i = smap;
@@ -539,7 +539,7 @@ void __init paging_init(void)
     mpt_size  = (max_page * BYTES_PER_LONG) + (1UL << L2_PAGETABLE_SHIFT) - 1;
     mpt_size &= ~((1UL << L2_PAGETABLE_SHIFT) - 1UL);
 #define MFN(x) (((x) << L2_PAGETABLE_SHIFT) / sizeof(unsigned long))
-#define CNT ((sizeof(*frame_table) & -sizeof(*frame_table)) / \
+#define CNT (LOWEST_BIT(sizeof(*frame_table)) / \
              sizeof(*machine_to_phys_mapping))
     BUILD_BUG_ON((sizeof(*frame_table) & ~sizeof(*frame_table)) % \
                  sizeof(*machine_to_phys_mapping));
@@ -666,7 +666,7 @@ void __init paging_init(void)
         mpt_size = 0;
 
 #define MFN(x) (((x) << L2_PAGETABLE_SHIFT) / sizeof(unsigned int))
-#define CNT ((sizeof(*frame_table) & -sizeof(*frame_table)) / \
+#define CNT (LOWEST_BIT(sizeof(*frame_table)) / \
              sizeof(*compat_machine_to_phys_mapping))
     BUILD_BUG_ON((sizeof(*frame_table) & ~sizeof(*frame_table)) % \
                  sizeof(*compat_machine_to_phys_mapping));
-- 
2.34.1

[XEN PATCH][for-4.19 v3 5/8] x86/io_apic: address violation of MISRA C:2012 Rule 10.1

[Nicola Vetrini]

The definition of IO_APIC_BASE contains a sum of an essentially enum
value (FIX_IO_APIC_BASE_0) that is positive with an index that, in all
instances, is unsigned, therefore the former is cast to unsigned, so that
the operands are of the same essential type.

No functional change.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
Eventually __fix_to_virt may become an inline function; in that case,
it should retain unsigned int as its parameter type.

Changes in v3:
- style fix
- Add missing S-o-b
---
 xen/arch/x86/include/asm/io_apic.h | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/include/asm/io_apic.h b/xen/arch/x86/include/asm/io_apic.h
index a7e4c9e146de..206bb961c005 100644
--- a/xen/arch/x86/include/asm/io_apic.h
+++ b/xen/arch/x86/include/asm/io_apic.h
@@ -14,9 +14,10 @@
  * Copyright (C) 1997, 1998, 1999, 2000 Ingo Molnar
  */
 
-#define IO_APIC_BASE(idx)                                               \
-    ((volatile uint32_t *)(__fix_to_virt(FIX_IO_APIC_BASE_0 + (idx))    \
-                           + (mp_ioapics[idx].mpc_apicaddr & ~PAGE_MASK)))
+#define IO_APIC_BASE(idx)                                         \
+    ((volatile uint32_t *)                                        \
+     (__fix_to_virt((unsigned int)FIX_IO_APIC_BASE_0 + (idx)) +   \
+                    (mp_ioapics[idx].mpc_apicaddr & ~PAGE_MASK)))
 
 #define IO_APIC_ID(idx) (mp_ioapics[idx].mpc_apicid)
 
-- 
2.34.1

Re: [XEN PATCH][for-4.19 v3 5/8] x86/io_apic: address violation of MISRA C:2012 Rule 10.1

[Jan Beulich]

On 20.10.2023 17:28, Nicola Vetrini wrote:
> The definition of IO_APIC_BASE contains a sum of an essentially enum
> value (FIX_IO_APIC_BASE_0) that is positive with an index that, in all
> instances, is unsigned, therefore the former is cast to unsigned, so that
> the operands are of the same essential type.
> 
> No functional change.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>

Acked-by: Jan Beulich <jbeulich@suse.com>

[XEN PATCH][for-4.19 v3 6/8] x86/mce: Move MC_NCLASSES into the enum mctelem_class

[Nicola Vetrini]

The definition of MC_NCLASSES contained a violation of MISRA C:2012
Rule 10.1, therefore by moving it as an enumeration constant resolves the
violation and makes it more resilient to possible additions to that enum.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
This patch has already been picked up in Andrew's for-next tree.

Note that the use of an enum constant as operand to [ ] and != is allowed
by the Rule.
---
 xen/arch/x86/cpu/mcheck/mctelem.c | 2 --
 xen/arch/x86/cpu/mcheck/mctelem.h | 5 +++--
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/cpu/mcheck/mctelem.c b/xen/arch/x86/cpu/mcheck/mctelem.c
index 329ac20faf96..77a4d1d5ff48 100644
--- a/xen/arch/x86/cpu/mcheck/mctelem.c
+++ b/xen/arch/x86/cpu/mcheck/mctelem.c
@@ -64,8 +64,6 @@ struct mctelem_ent {
 
 #define MC_NENT (MC_URGENT_NENT + MC_NONURGENT_NENT)
 
-#define	MC_NCLASSES		(MC_NONURGENT + 1)
-
 #define	COOKIE2MCTE(c)		((struct mctelem_ent *)(c))
 #define	MCTE2COOKIE(tep)	((mctelem_cookie_t)(tep))
 
diff --git a/xen/arch/x86/cpu/mcheck/mctelem.h b/xen/arch/x86/cpu/mcheck/mctelem.h
index d4eba53ae0e5..21b251847bc0 100644
--- a/xen/arch/x86/cpu/mcheck/mctelem.h
+++ b/xen/arch/x86/cpu/mcheck/mctelem.h
@@ -55,8 +55,9 @@
 typedef struct mctelem_cookie *mctelem_cookie_t;
 
 typedef enum mctelem_class {
-	MC_URGENT,
-	MC_NONURGENT
+    MC_URGENT,
+    MC_NONURGENT,
+    MC_NCLASSES
 } mctelem_class_t;
 
 extern void mctelem_init(unsigned int);
-- 
2.34.1

[XEN PATCH][for-4.19 v3 7/8] xen/types: address Rule 10.1 for DECLARE_BITMAP use

[Nicola Vetrini]

Given its use in the declaration
'DECLARE_BITMAP(features, IOMMU_FEAT_count)' the argument
'bits' has essential type 'enum iommu_feature', which is not
allowed by the Rule as an operand to the addition operator
in macro 'BITS_TO_LONGS'.

This construct is deviated with a deviation comment.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
Changes in v3:
- edited comment
---
 docs/misra/safe.json    | 8 ++++++++
 xen/include/xen/iommu.h | 1 +
 xen/include/xen/types.h | 5 +++++
 3 files changed, 14 insertions(+)

diff --git a/docs/misra/safe.json b/docs/misra/safe.json
index 39c5c056c7d4..952324f85cf9 100644
--- a/docs/misra/safe.json
+++ b/docs/misra/safe.json
@@ -20,6 +20,14 @@
         },
         {
             "id": "SAF-2-safe",
+            "analyser": {
+                "eclair": "MC3R1.R10.1"
+            },
+            "name": "MC3R1.R10.1: use of an enumeration constant in an arithmetic operation",
+            "text": "This violation can be fixed with a cast to (int) of the enumeration constant, but a deviation was chosen due to code readability (see also the comment in BITS_TO_LONGS)."
+        },
+        {
+            "id": "SAF-3-safe",
             "analyser": {},
             "name": "Sentinel",
             "text": "Next ID to be used"
diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
index 0e747b0bbc1c..d5c25770915b 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -360,6 +360,7 @@ struct domain_iommu {
 #endif
 
     /* Features supported by the IOMMU */
+    /* SAF-2-safe enum constant in arithmetic operation */
     DECLARE_BITMAP(features, IOMMU_FEAT_count);
 
     /* Does the guest share HAP mapping with the IOMMU? */
diff --git a/xen/include/xen/types.h b/xen/include/xen/types.h
index 23cad71c8a47..9603ab70b54d 100644
--- a/xen/include/xen/types.h
+++ b/xen/include/xen/types.h
@@ -22,6 +22,11 @@ typedef signed long ssize_t;
 
 typedef __PTRDIFF_TYPE__ ptrdiff_t;
 
+/*
+ * Users of this macro are expected to pass a positive value.
+ *
+ * XXX: should become an unsigned quantity
+ */
 #define BITS_TO_LONGS(bits) \
     (((bits)+BITS_PER_LONG-1)/BITS_PER_LONG)
 #define DECLARE_BITMAP(name,bits) \
-- 
2.34.1

Re: [XEN PATCH][for-4.19 v3 7/8] xen/types: address Rule 10.1 for DECLARE_BITMAP use

[Stefano Stabellini]

On Fri, 20 Oct 2023, Nicola Vetrini wrote:
> Given its use in the declaration
> 'DECLARE_BITMAP(features, IOMMU_FEAT_count)' the argument
> 'bits' has essential type 'enum iommu_feature', which is not
> allowed by the Rule as an operand to the addition operator
> in macro 'BITS_TO_LONGS'.
> 
> This construct is deviated with a deviation comment.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

[XEN PATCH][for-4.19 v3 8/8] xen/compat: use BUILD_BUG_ON in CHECK_SIZE macros

[Nicola Vetrini]

BUILD_BUG_ON is the preferred way to induce a build error
upon statically determined incorrect conditions.

This also fixes a MISRA C:2012 Rule 10.1 violation in the
previous formulation.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
Changes in v2:
- replace the construct with a BUILD_BUG_ON.
Changes in v3:
- drop unused typedef.
---
 xen/include/xen/compat.h | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/xen/include/xen/compat.h b/xen/include/xen/compat.h
index f2ce5bb3580a..41a5d61eef98 100644
--- a/xen/include/xen/compat.h
+++ b/xen/include/xen/compat.h
@@ -151,12 +151,18 @@ CHECK_NAME_(k, n, T)(k xen_ ## n *x, \
     return x == c; \
 }
 
-#define CHECK_SIZE(name) \
-    typedef int CHECK_NAME(name, S)[1 - (sizeof(xen_ ## name ## _t) != \
-                                         sizeof(compat_ ## name ## _t)) * 2]
+#define CHECK_SIZE(name)                                  \
+static inline void __maybe_unused CHECK_SIZE_##name(void) \
+{                                                         \
+    BUILD_BUG_ON(sizeof(xen_ ## name ## _t) !=            \
+                 sizeof(compat_ ## name ## _t));          \
+}
 #define CHECK_SIZE_(k, n) \
-    typedef int CHECK_NAME_(k, n, S)[1 - (sizeof(k xen_ ## n) != \
-                                          sizeof(k compat_ ## n)) * 2]
+static inline void __maybe_unused CHECK_SIZE_##k_##n(void) \
+{                                                          \
+    BUILD_BUG_ON(sizeof(k xen_ ## n) !=                    \
+                 sizeof(k compat_ ## n));                  \
+}
 
 #define CHECK_FIELD_COMMON(name, t, f) \
 static inline int __maybe_unused name(xen_ ## t ## _t *x, compat_ ## t ## _t *c) \
-- 
2.34.1

Re: [XEN PATCH][for-4.19 v3 8/8] xen/compat: use BUILD_BUG_ON in CHECK_SIZE macros

[Jan Beulich]

On 20.10.2023 17:28, Nicola Vetrini wrote:
> BUILD_BUG_ON is the preferred way to induce a build error
> upon statically determined incorrect conditions.
> 
> This also fixes a MISRA C:2012 Rule 10.1 violation in the
> previous formulation.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>

Acked-by: Jan Beulich <jbeulich@suse.com>