[XEN PATCH 0/9] address violations of MISRA C:2012 Rule 10.1

[XEN PATCH 0/9] address violations of MISRA C:2012 Rule 10.1

[Nicola Vetrini]

This series aims to resolve or deviate various violations of Rule 10.1
(operands of inappropriate essential type).

To do so, 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.

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

Patches marked with [for-4.19] are not meant to be included in the current
staging, but they can be picked up for 4.18, if deemed risk-free.

Nicola Vetrini (9):
  xen/include: add macro LOWEST_POW2
  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_POW2
  x86/cpu-policy: address violations of MISRA C Rule 10.1
  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: address Rule 10.1 for macros CHECK_SIZE

 automation/eclair_analysis/ECLAIR/deviations.ecl |  6 ++++++
 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                         | 10 ++++++----
 xen/include/xen/iommu.h                          |  2 +-
 xen/include/xen/lib/x86/cpu-policy.h             | 13 +++++++------
 xen/include/xen/macros.h                         |  6 ++++--
 xen/include/xen/pdx.h                            |  2 +-
 xen/include/xen/types.h                          |  1 +
 12 files changed, 43 insertions(+), 29 deletions(-)

--
2.34.1

[XEN PATCH][for-4.19 1/9] xen/include: add macro LOWEST_POW2

[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>
---
 automation/eclair_analysis/ECLAIR/deviations.ecl | 6 ++++++
 xen/include/xen/macros.h                         | 6 ++++--
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl b/automation/eclair_analysis/ECLAIR/deviations.ecl
index d8170106b449..016164643105 100644
--- a/automation/eclair_analysis/ECLAIR/deviations.ecl
+++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
@@ -274,6 +274,12 @@ still non-negative."
 -config=MC3R1.R10.1,etypes+={safe, "stmt(operator(logical)||node(conditional_operator||binary_conditional_operator))", "dst_type(ebool||boolean)"}
 -doc_end
 
+-doc_begin="The macro LOWEST_POW2 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_POW2$))))"}
+-doc_end
+
 ### Set 3 ###
 
 #
diff --git a/xen/include/xen/macros.h b/xen/include/xen/macros.h
index d0caae7db298..bb9a1c9a53d0 100644
--- a/xen/include/xen/macros.h
+++ b/xen/include/xen/macros.h
@@ -8,8 +8,10 @@
 #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))
+#define LOWEST_POW2(x) ((x) & -(x))
+
+#define MASK_EXTR(v, m) (((v) & (m)) / LOWEST_POW2(m))
+#define MASK_INSR(v, m) (((v) * LOWEST_POW2(m)) & (m))
 
 #define count_args_(dot, a1, a2, a3, a4, a5, a6, a7, a8, x, ...) x
 #define count_args(args...) \
-- 
2.34.1

Re: [XEN PATCH][for-4.19 1/9] xen/include: add macro LOWEST_POW2

[Julien Grall]

Hi,

On 06/10/2023 09:26, Nicola Vetrini wrote:
> 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.

Can you explain why this is a deviation in ECLAIR rather than one with 
/* SAF-* */ (or whichever name we decide to rename to)? Is this because 
the code is correct from MISRA perspective but the tool is confused?

> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> ---
>   automation/eclair_analysis/ECLAIR/deviations.ecl | 6 ++++++
>   xen/include/xen/macros.h                         | 6 ++++--
>   2 files changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl b/automation/eclair_analysis/ECLAIR/deviations.ecl
> index d8170106b449..016164643105 100644
> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl
> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
> @@ -274,6 +274,12 @@ still non-negative."
>   -config=MC3R1.R10.1,etypes+={safe, "stmt(operator(logical)||node(conditional_operator||binary_conditional_operator))", "dst_type(ebool||boolean)"}
>   -doc_end
>   
> +-doc_begin="The macro LOWEST_POW2 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_POW2$))))"}
> +-doc_end
> +
>   ### Set 3 ###
>   
>   #
> diff --git a/xen/include/xen/macros.h b/xen/include/xen/macros.h
> index d0caae7db298..bb9a1c9a53d0 100644
> --- a/xen/include/xen/macros.h
> +++ b/xen/include/xen/macros.h
> @@ -8,8 +8,10 @@
>   #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))
> +#define LOWEST_POW2(x) ((x) & -(x))
> +
> +#define MASK_EXTR(v, m) (((v) & (m)) / LOWEST_POW2(m))
> +#define MASK_INSR(v, m) (((v) * LOWEST_POW2(m)) & (m))
>   
>   #define count_args_(dot, a1, a2, a3, a4, a5, a6, a7, a8, x, ...) x
>   #define count_args(args...) \

-- 
Julien Grall

Re: [XEN PATCH][for-4.19 1/9] xen/include: add macro LOWEST_POW2

[Nicola Vetrini]

On 06/10/2023 11:29, Julien Grall wrote:
> Hi,
> 
> On 06/10/2023 09:26, Nicola Vetrini wrote:
>> 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.
> 
> Can you explain why this is a deviation in ECLAIR rather than one with
> /* SAF-* */ (or whichever name we decide to rename to)? Is this
> because the code is correct from MISRA perspective but the tool is
> confused?
> 

The code does violate Rule 10.1 (a unary minus applied to an unsigned 
value is
deemed inappropriate by MISRA), but rather than changing a whole lot of 
places where this
construct is used (mainly in x86 code), the reasoning is that it makes 
more sense to isolate
it and justify its presence by the fact that on 2's complement 
architectures the result is
indeed correct.

>> 
>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>> ---
>>   automation/eclair_analysis/ECLAIR/deviations.ecl | 6 ++++++
>>   xen/include/xen/macros.h                         | 6 ++++--
>>   2 files changed, 10 insertions(+), 2 deletions(-)
>> 
>> diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl 
>> b/automation/eclair_analysis/ECLAIR/deviations.ecl
>> index d8170106b449..016164643105 100644
>> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl
>> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
>> @@ -274,6 +274,12 @@ still non-negative."
>>   -config=MC3R1.R10.1,etypes+={safe, 
>> "stmt(operator(logical)||node(conditional_operator||binary_conditional_operator))", 
>> "dst_type(ebool||boolean)"}
>>   -doc_end
>>   +-doc_begin="The macro LOWEST_POW2 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_POW2$))))"}
>> +-doc_end
>> +
>>   ### Set 3 ###
>>     #
>> diff --git a/xen/include/xen/macros.h b/xen/include/xen/macros.h
>> index d0caae7db298..bb9a1c9a53d0 100644
>> --- a/xen/include/xen/macros.h
>> +++ b/xen/include/xen/macros.h
>> @@ -8,8 +8,10 @@
>>   #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))
>> +#define LOWEST_POW2(x) ((x) & -(x))
>> +
>> +#define MASK_EXTR(v, m) (((v) & (m)) / LOWEST_POW2(m))
>> +#define MASK_INSR(v, m) (((v) * LOWEST_POW2(m)) & (m))
>>     #define count_args_(dot, a1, a2, a3, a4, a5, a6, a7, a8, x, ...) x
>>   #define count_args(args...) \

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

Re: [XEN PATCH][for-4.19 1/9] xen/include: add macro LOWEST_POW2

[Julien Grall]

On 06/10/2023 11:02, Nicola Vetrini wrote:
> On 06/10/2023 11:29, Julien Grall wrote:
>> Hi,
>>
>> On 06/10/2023 09:26, Nicola Vetrini wrote:
>>> 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.
>>
>> Can you explain why this is a deviation in ECLAIR rather than one with
>> /* SAF-* */ (or whichever name we decide to rename to)? Is this
>> because the code is correct from MISRA perspective but the tool is
>> confused?
>>
> 
> The code does violate Rule 10.1 (a unary minus applied to an unsigned 
> value is
> deemed inappropriate by MISRA), but rather than changing a whole lot of 
> places where this
> construct is used (mainly in x86 code), the reasoning is that it makes 
> more sense to isolate
> it and justify its presence by the fact that on 2's complement 
> architectures the result is
> indeed correct.

This is explaining to me why you are adding LOWEST_POW2(). But this 
doesn't explain why you are not using /* SAF-* */ on top of LOWEST_POW2().

To me, we should only use ECLAIR specific deviation when this is a 
shortcoming with the tool.

Cheers,

-- 
Julien Grall

Re: [XEN PATCH][for-4.19 1/9] xen/include: add macro LOWEST_POW2

[Nicola Vetrini]

On 06/10/2023 12:22, Julien Grall wrote:
> On 06/10/2023 11:02, Nicola Vetrini wrote:
>> On 06/10/2023 11:29, Julien Grall wrote:
>>> Hi,
>>> 
>>> On 06/10/2023 09:26, Nicola Vetrini wrote:
>>>> 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.
>>> 
>>> Can you explain why this is a deviation in ECLAIR rather than one 
>>> with
>>> /* SAF-* */ (or whichever name we decide to rename to)? Is this
>>> because the code is correct from MISRA perspective but the tool is
>>> confused?
>>> 
>> 
>> The code does violate Rule 10.1 (a unary minus applied to an unsigned 
>> value is
>> deemed inappropriate by MISRA), but rather than changing a whole lot 
>> of places where this
>> construct is used (mainly in x86 code), the reasoning is that it makes 
>> more sense to isolate
>> it and justify its presence by the fact that on 2's complement 
>> architectures the result is
>> indeed correct.
> 
> This is explaining to me why you are adding LOWEST_POW2(). But this
> doesn't explain why you are not using /* SAF-* */ on top of
> LOWEST_POW2().
> 
> To me, we should only use ECLAIR specific deviation when this is a
> shortcoming with the tool.
> 
> Cheers,

Because of the way ECLAIR deviation comments work implies that in most 
cases the real
place where to put the deviation is the usage site
(the so-called top expansion location of the macro). Now, for 
widely-used macros this is
cumbersome and would clutter the code unnecessarily. It's way cleaner 
imo to have a single
line in the configuration with a clear justification that is present in 
the textual output
of the tool.
But then there are tool interoperability considerations, that would call 
for standardized
deviation mechanisms, if they do detect this as a violation (which I 
don't know).

In the end, it could be done with a textual deviation, if that's 
preferred, but keep in mind
that those are more fragile w.r.t. code movement.

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

Re: [XEN PATCH][for-4.19 1/9] xen/include: add macro LOWEST_POW2

[Julien Grall]

Hi Nicola,

On 06/10/2023 11:34, Nicola Vetrini wrote:
> On 06/10/2023 12:22, Julien Grall wrote:
>> On 06/10/2023 11:02, Nicola Vetrini wrote:
>>> On 06/10/2023 11:29, Julien Grall wrote:
>>>> Hi,
>>>>
>>>> On 06/10/2023 09:26, Nicola Vetrini wrote:
>>>>> 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.
>>>>
>>>> Can you explain why this is a deviation in ECLAIR rather than one with
>>>> /* SAF-* */ (or whichever name we decide to rename to)? Is this
>>>> because the code is correct from MISRA perspective but the tool is
>>>> confused?
>>>>
>>>
>>> The code does violate Rule 10.1 (a unary minus applied to an unsigned 
>>> value is
>>> deemed inappropriate by MISRA), but rather than changing a whole lot 
>>> of places where this
>>> construct is used (mainly in x86 code), the reasoning is that it 
>>> makes more sense to isolate
>>> it and justify its presence by the fact that on 2's complement 
>>> architectures the result is
>>> indeed correct.
>>
>> This is explaining to me why you are adding LOWEST_POW2(). But this
>> doesn't explain why you are not using /* SAF-* */ on top of
>> LOWEST_POW2().
>>
>> To me, we should only use ECLAIR specific deviation when this is a
>> shortcoming with the tool.
>>
>> Cheers,
> 
> Because of the way ECLAIR deviation comments work implies that in most 
> cases the real
> place where to put the deviation is the usage site
> (the so-called top expansion location of the macro). Now, for 
> widely-used macros this is
> cumbersome and would clutter the code unnecessarily. It's way cleaner 
> imo to have a single
> line in the configuration with a clear justification that is present in 
> the textual output
> of the tool.

Just to clarify, you are saying that the following would not work for 
Eclair:

/* SAF-XXX */
#define LOWEST_POW2()

Instead you would need the following:

void foo()
{
	/* SAF-XXX */
	LOWEST()
}

Am I correct? If so, would something like below (untested) work?

#define LOWEST_POW2(...) ({ \
    /* SAFE-XXX */           \
    ...
    })

> But then there are tool interoperability considerations, that would call 
> for standardized
> deviation mechanisms, if they do detect this as a violation (which I 
> don't know).

I don't think we need to know whether a tool detects it. We only need to 
know whether this is  violation to MISRA. If this is one, then this is a 
call to have a marker in the code.

If this is a false positive, then adding the deviation in the tool 
configuration is best (unless there are multiple tools affected).

> 
> In the end, it could be done with a textual deviation, if that's 
> preferred, but keep in mind
> that those are more fragile w.r.t. code movement.

If the comment is around the macro there are limited chance that this 
will be missed. But if you are worried about code movement, you should 
be worried about macro renaming with your approach (one may not know 
Eclair has a deviation) and/or function with the same name.

I am curious to know what the other thinks.

Cheers,

-- 
Julien Grall

Re: [XEN PATCH][for-4.19 1/9] xen/include: add macro LOWEST_POW2

[Nicola Vetrini]

On 06/10/2023 16:35, Julien Grall wrote:
> Hi Nicola,
> 
> On 06/10/2023 11:34, Nicola Vetrini wrote:
>> On 06/10/2023 12:22, Julien Grall wrote:
>>> On 06/10/2023 11:02, Nicola Vetrini wrote:
>>>> On 06/10/2023 11:29, Julien Grall wrote:
>>>>> Hi,
>>>>> 
>>>>> On 06/10/2023 09:26, Nicola Vetrini wrote:
>>>>>> 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.
>>>>> 
>>>>> Can you explain why this is a deviation in ECLAIR rather than one 
>>>>> with
>>>>> /* SAF-* */ (or whichever name we decide to rename to)? Is this
>>>>> because the code is correct from MISRA perspective but the tool is
>>>>> confused?
>>>>> 
>>>> 
>>>> The code does violate Rule 10.1 (a unary minus applied to an 
>>>> unsigned value is
>>>> deemed inappropriate by MISRA), but rather than changing a whole lot 
>>>> of places where this
>>>> construct is used (mainly in x86 code), the reasoning is that it 
>>>> makes more sense to isolate
>>>> it and justify its presence by the fact that on 2's complement 
>>>> architectures the result is
>>>> indeed correct.
>>> 
>>> This is explaining to me why you are adding LOWEST_POW2(). But this
>>> doesn't explain why you are not using /* SAF-* */ on top of
>>> LOWEST_POW2().
>>> 
>>> To me, we should only use ECLAIR specific deviation when this is a
>>> shortcoming with the tool.
>>> 
>>> Cheers,
>> 
>> Because of the way ECLAIR deviation comments work implies that in most 
>> cases the real
>> place where to put the deviation is the usage site
>> (the so-called top expansion location of the macro). Now, for 
>> widely-used macros this is
>> cumbersome and would clutter the code unnecessarily. It's way cleaner 
>> imo to have a single
>> line in the configuration with a clear justification that is present 
>> in the textual output
>> of the tool.
> 
> Just to clarify, you are saying that the following would not work for 
> Eclair:
> 
> /* SAF-XXX */
> #define LOWEST_POW2()
> 
> Instead you would need the following:
> 
> void foo()
> {
> 	/* SAF-XXX */
> 	LOWEST()
> }
> 
> Am I correct? If so, would something like below (untested) work?
> 
> #define LOWEST_POW2(...) ({ \
>    /* SAFE-XXX */           \
>    ...
>    })
> 

Option (2) would work. I'm not sure about (3), I'll test it.

>> But then there are tool interoperability considerations, that would 
>> call for standardized
>> deviation mechanisms, if they do detect this as a violation (which I 
>> don't know).
> 
> I don't think we need to know whether a tool detects it. We only need
> to know whether this is  violation to MISRA. If this is one, then this
> is a call to have a marker in the code.
> 
> If this is a false positive, then adding the deviation in the tool
> configuration is best (unless there are multiple tools affected).
> 

This is definitely a MISRA violation.

>> 
>> In the end, it could be done with a textual deviation, if that's 
>> preferred, but keep in mind
>> that those are more fragile w.r.t. code movement.
> 
> If the comment is around the macro there are limited chance that this
> will be missed. But if you are worried about code movement, you should
> be worried about macro renaming with your approach (one may not know
> Eclair has a deviation) and/or function with the same name.
> 

True, but if you introduce a violation on a guideline that is supposed 
to be clean then
the analysis will fail and show what's wrong (not by making the pipeline 
fail right now, but
ideally that's the plan). Reused identifiers are addressed by separate 
rules
(mainly Series 5).

> I am curious to know what the other thinks.
> 
> Cheers,

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

Re: [XEN PATCH][for-4.19 1/9] xen/include: add macro LOWEST_POW2

[Stefano Stabellini]

On Fri, 6 Oct 2023, Julien Grall wrote:
> Hi Nicola,
> 
> On 06/10/2023 11:34, Nicola Vetrini wrote:
> > On 06/10/2023 12:22, Julien Grall wrote:
> > > On 06/10/2023 11:02, Nicola Vetrini wrote:
> > > > On 06/10/2023 11:29, Julien Grall wrote:
> > > > > Hi,
> > > > > 
> > > > > On 06/10/2023 09:26, Nicola Vetrini wrote:
> > > > > > 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.
> > > > > 
> > > > > Can you explain why this is a deviation in ECLAIR rather than one with
> > > > > /* SAF-* */ (or whichever name we decide to rename to)? Is this
> > > > > because the code is correct from MISRA perspective but the tool is
> > > > > confused?
> > > > > 
> > > > 
> > > > The code does violate Rule 10.1 (a unary minus applied to an unsigned
> > > > value is
> > > > deemed inappropriate by MISRA), but rather than changing a whole lot of
> > > > places where this
> > > > construct is used (mainly in x86 code), the reasoning is that it makes
> > > > more sense to isolate
> > > > it and justify its presence by the fact that on 2's complement
> > > > architectures the result is
> > > > indeed correct.
> > > 
> > > This is explaining to me why you are adding LOWEST_POW2(). But this
> > > doesn't explain why you are not using /* SAF-* */ on top of
> > > LOWEST_POW2().
> > > 
> > > To me, we should only use ECLAIR specific deviation when this is a
> > > shortcoming with the tool.
> > > 
> > > Cheers,
> > 
> > Because of the way ECLAIR deviation comments work implies that in most cases
> > the real
> > place where to put the deviation is the usage site
> > (the so-called top expansion location of the macro). Now, for widely-used
> > macros this is
> > cumbersome and would clutter the code unnecessarily. It's way cleaner imo to
> > have a single
> > line in the configuration with a clear justification that is present in the
> > textual output
> > of the tool.
> 
> Just to clarify, you are saying that the following would not work for Eclair:
> 
> /* SAF-XXX */
> #define LOWEST_POW2()
> 
> Instead you would need the following:
> 
> void foo()
> {
> 	/* SAF-XXX */
> 	LOWEST()
> }
> 
> Am I correct? If so, would something like below (untested) work?
> 
> #define LOWEST_POW2(...) ({ \
>    /* SAFE-XXX */           \
>    ...
>    })
> 
> > But then there are tool interoperability considerations, that would call for
> > standardized
> > deviation mechanisms, if they do detect this as a violation (which I don't
> > know).
> 
> I don't think we need to know whether a tool detects it. We only need to know
> whether this is  violation to MISRA. If this is one, then this is a call to
> have a marker in the code.
> 
> If this is a false positive, then adding the deviation in the tool
> configuration is best (unless there are multiple tools affected).
> 
> > 
> > In the end, it could be done with a textual deviation, if that's preferred,
> > but keep in mind
> > that those are more fragile w.r.t. code movement.
> 
> If the comment is around the macro there are limited chance that this will be
> missed. But if you are worried about code movement, you should be worried
> about macro renaming with your approach (one may not know Eclair has a
> deviation) and/or function with the same name.
> 
> I am curious to know what the other thinks.

I agree.

I think that we should use the SAF-x-safe framework as much as possible.
That is the most flexible and easier to maintain deviation system we
have. If we can make it work in this specific case with Julien's
suggestion above, then great.

But it is becoming clear that the SAF-x-safe framework has limitations,
for instance  https://marc.info/?l=xen-devel&m=169657904027210

There are going to be cases where SAF-x-safe won't work. In those cases,
we will probably end up using an ECLAIR specific configuration. Those
cases should be hopefully few and should be well documented, also
outside of the ECLAIR config file, which is very ECLAIR specific and
optimized to be machine readable. 

We need another RST document under docs/misra to document any deviations
that are not dealt by SAF-x-safe comments. Today we are basically using
the notes section in the docs/misra/rules.rst table but that doesn't
scale.

So I think we should:
- create new RST file like docs/misra/deviations.rst
- deviations.rst will list any deviation that is not a SAF-x-safe
  deviation
- all ECLAIR special deviations in the ECLAIR config file should be
  documented in deviations.rst
- in the future special ECLAIR deviations in the config file should
  come with a new documentation entry in deviations.rst


This doesn't entirely address Julien's valid concern but at least it
makes it easier to recognize the problem when it occurs.

Re: [XEN PATCH][for-4.19 1/9] xen/include: add macro LOWEST_POW2

[Stefano Stabellini]

On Fri, 6 Oct 2023, Stefano Stabellini wrote:
> On Fri, 6 Oct 2023, Julien Grall wrote:
> > Hi Nicola,
> > 
> > On 06/10/2023 11:34, Nicola Vetrini wrote:
> > > On 06/10/2023 12:22, Julien Grall wrote:
> > > > On 06/10/2023 11:02, Nicola Vetrini wrote:
> > > > > On 06/10/2023 11:29, Julien Grall wrote:
> > > > > > Hi,
> > > > > > 
> > > > > > On 06/10/2023 09:26, Nicola Vetrini wrote:
> > > > > > > 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.
> > > > > > 
> > > > > > Can you explain why this is a deviation in ECLAIR rather than one with
> > > > > > /* SAF-* */ (or whichever name we decide to rename to)? Is this
> > > > > > because the code is correct from MISRA perspective but the tool is
> > > > > > confused?
> > > > > > 
> > > > > 
> > > > > The code does violate Rule 10.1 (a unary minus applied to an unsigned
> > > > > value is
> > > > > deemed inappropriate by MISRA), but rather than changing a whole lot of
> > > > > places where this
> > > > > construct is used (mainly in x86 code), the reasoning is that it makes
> > > > > more sense to isolate
> > > > > it and justify its presence by the fact that on 2's complement
> > > > > architectures the result is
> > > > > indeed correct.
> > > > 
> > > > This is explaining to me why you are adding LOWEST_POW2(). But this
> > > > doesn't explain why you are not using /* SAF-* */ on top of
> > > > LOWEST_POW2().
> > > > 
> > > > To me, we should only use ECLAIR specific deviation when this is a
> > > > shortcoming with the tool.
> > > > 
> > > > Cheers,
> > > 
> > > Because of the way ECLAIR deviation comments work implies that in most cases
> > > the real
> > > place where to put the deviation is the usage site
> > > (the so-called top expansion location of the macro). Now, for widely-used
> > > macros this is
> > > cumbersome and would clutter the code unnecessarily. It's way cleaner imo to
> > > have a single
> > > line in the configuration with a clear justification that is present in the
> > > textual output
> > > of the tool.
> > 
> > Just to clarify, you are saying that the following would not work for Eclair:
> > 
> > /* SAF-XXX */
> > #define LOWEST_POW2()
> > 
> > Instead you would need the following:
> > 
> > void foo()
> > {
> > 	/* SAF-XXX */
> > 	LOWEST()
> > }
> > 
> > Am I correct? If so, would something like below (untested) work?
> > 
> > #define LOWEST_POW2(...) ({ \
> >    /* SAFE-XXX */           \
> >    ...
> >    })
> > 
> > > But then there are tool interoperability considerations, that would call for
> > > standardized
> > > deviation mechanisms, if they do detect this as a violation (which I don't
> > > know).
> > 
> > I don't think we need to know whether a tool detects it. We only need to know
> > whether this is  violation to MISRA. If this is one, then this is a call to
> > have a marker in the code.
> > 
> > If this is a false positive, then adding the deviation in the tool
> > configuration is best (unless there are multiple tools affected).
> > 
> > > 
> > > In the end, it could be done with a textual deviation, if that's preferred,
> > > but keep in mind
> > > that those are more fragile w.r.t. code movement.
> > 
> > If the comment is around the macro there are limited chance that this will be
> > missed. But if you are worried about code movement, you should be worried
> > about macro renaming with your approach (one may not know Eclair has a
> > deviation) and/or function with the same name.
> > 
> > I am curious to know what the other thinks.
> 
> I agree.
> 
> I think that we should use the SAF-x-safe framework as much as possible.
> That is the most flexible and easier to maintain deviation system we
> have. If we can make it work in this specific case with Julien's
> suggestion above, then great.
> 
> But it is becoming clear that the SAF-x-safe framework has limitations,
> for instance  https://marc.info/?l=xen-devel&m=169657904027210
> 
> There are going to be cases where SAF-x-safe won't work. In those cases,
> we will probably end up using an ECLAIR specific configuration. Those
> cases should be hopefully few and should be well documented, also
> outside of the ECLAIR config file, which is very ECLAIR specific and
> optimized to be machine readable. 
> 
> We need another RST document under docs/misra to document any deviations
> that are not dealt by SAF-x-safe comments. Today we are basically using
> the notes section in the docs/misra/rules.rst table but that doesn't
> scale.
> 
> So I think we should:
> - create new RST file like docs/misra/deviations.rst
> - deviations.rst will list any deviation that is not a SAF-x-safe
>   deviation
> - all ECLAIR special deviations in the ECLAIR config file should be
>   documented in deviations.rst
> - in the future special ECLAIR deviations in the config file should
>   come with a new documentation entry in deviations.rst
> 
> 
> This doesn't entirely address Julien's valid concern but at least it
> makes it easier to recognize the problem when it occurs.


We already have docs/misra/documenting-violations.rst, so maybe we could
have one more section at the end of the document with a list of
"special" deviations.

Re: [XEN PATCH][for-4.19 1/9] xen/include: add macro LOWEST_POW2

[Nicola Vetrini]

On 07/10/2023 02:29, Stefano Stabellini wrote:
> On Fri, 6 Oct 2023, Stefano Stabellini wrote:
>> On Fri, 6 Oct 2023, Julien Grall wrote:
>> > Hi Nicola,
>> >
>> > On 06/10/2023 11:34, Nicola Vetrini wrote:
>> > > On 06/10/2023 12:22, Julien Grall wrote:
>> > > > On 06/10/2023 11:02, Nicola Vetrini wrote:
>> > > > > On 06/10/2023 11:29, Julien Grall wrote:
>> > > > > > Hi,
>> > > > > >
>> > > > > > On 06/10/2023 09:26, Nicola Vetrini wrote:
>> > > > > > > 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.
>> > > > > >
>> > > > > > Can you explain why this is a deviation in ECLAIR rather than one with
>> > > > > > /* SAF-* */ (or whichever name we decide to rename to)? Is this
>> > > > > > because the code is correct from MISRA perspective but the tool is
>> > > > > > confused?
>> > > > > >
>> > > > >
>> > > > > The code does violate Rule 10.1 (a unary minus applied to an unsigned
>> > > > > value is
>> > > > > deemed inappropriate by MISRA), but rather than changing a whole lot of
>> > > > > places where this
>> > > > > construct is used (mainly in x86 code), the reasoning is that it makes
>> > > > > more sense to isolate
>> > > > > it and justify its presence by the fact that on 2's complement
>> > > > > architectures the result is
>> > > > > indeed correct.
>> > > >
>> > > > This is explaining to me why you are adding LOWEST_POW2(). But this
>> > > > doesn't explain why you are not using /* SAF-* */ on top of
>> > > > LOWEST_POW2().
>> > > >
>> > > > To me, we should only use ECLAIR specific deviation when this is a
>> > > > shortcoming with the tool.
>> > > >
>> > > > Cheers,
>> > >
>> > > Because of the way ECLAIR deviation comments work implies that in most cases
>> > > the real
>> > > place where to put the deviation is the usage site
>> > > (the so-called top expansion location of the macro). Now, for widely-used
>> > > macros this is
>> > > cumbersome and would clutter the code unnecessarily. It's way cleaner imo to
>> > > have a single
>> > > line in the configuration with a clear justification that is present in the
>> > > textual output
>> > > of the tool.
>> >
>> > Just to clarify, you are saying that the following would not work for Eclair:
>> >
>> > /* SAF-XXX */
>> > #define LOWEST_POW2()
>> >
>> > Instead you would need the following:
>> >
>> > void foo()
>> > {
>> > 	/* SAF-XXX */
>> > 	LOWEST()
>> > }
>> >
>> > Am I correct? If so, would something like below (untested) work?
>> >
>> > #define LOWEST_POW2(...) ({ \
>> >    /* SAFE-XXX */           \
>> >    ...
>> >    })
>> >
>> > > But then there are tool interoperability considerations, that would call for
>> > > standardized
>> > > deviation mechanisms, if they do detect this as a violation (which I don't
>> > > know).
>> >
>> > I don't think we need to know whether a tool detects it. We only need to know
>> > whether this is  violation to MISRA. If this is one, then this is a call to
>> > have a marker in the code.
>> >
>> > If this is a false positive, then adding the deviation in the tool
>> > configuration is best (unless there are multiple tools affected).
>> >
>> > >
>> > > In the end, it could be done with a textual deviation, if that's preferred,
>> > > but keep in mind
>> > > that those are more fragile w.r.t. code movement.
>> >
>> > If the comment is around the macro there are limited chance that this will be
>> > missed. But if you are worried about code movement, you should be worried
>> > about macro renaming with your approach (one may not know Eclair has a
>> > deviation) and/or function with the same name.
>> >
>> > I am curious to know what the other thinks.
>> 
>> I agree.
>> 
>> I think that we should use the SAF-x-safe framework as much as 
>> possible.
>> That is the most flexible and easier to maintain deviation system we
>> have. If we can make it work in this specific case with Julien's
>> suggestion above, then great.
>> 
>> But it is becoming clear that the SAF-x-safe framework has 
>> limitations,
>> for instance  https://marc.info/?l=xen-devel&m=169657904027210
>> 
>> There are going to be cases where SAF-x-safe won't work. In those 
>> cases,
>> we will probably end up using an ECLAIR specific configuration. Those
>> cases should be hopefully few and should be well documented, also
>> outside of the ECLAIR config file, which is very ECLAIR specific and
>> optimized to be machine readable.
>> 
>> We need another RST document under docs/misra to document any 
>> deviations
>> that are not dealt by SAF-x-safe comments. Today we are basically 
>> using
>> the notes section in the docs/misra/rules.rst table but that doesn't
>> scale.
>> 
>> So I think we should:
>> - create new RST file like docs/misra/deviations.rst
>> - deviations.rst will list any deviation that is not a SAF-x-safe
>>   deviation
>> - all ECLAIR special deviations in the ECLAIR config file should be
>>   documented in deviations.rst
>> - in the future special ECLAIR deviations in the config file should
>>   come with a new documentation entry in deviations.rst
>> 
>> 
>> This doesn't entirely address Julien's valid concern but at least it
>> makes it easier to recognize the problem when it occurs.
> 
> 
> We already have docs/misra/documenting-violations.rst, so maybe we 
> could
> have one more section at the end of the document with a list of
> "special" deviations.

Sounds good. I'll submit it as part of the ecl updates patch by Simone 
[1].
I think a separate file is better, There's a pointer in rules.rst to
documenting-violations.rst, and a reference to deviations.rst can be 
added here.

[1] https://marc.info/?l=xen-devel&m=169658766432087&w=2

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

Re: [XEN PATCH][for-4.19 1/9] xen/include: add macro LOWEST_POW2

[andrew.cooper3]

On 06/10/2023 9:26 am, Nicola Vetrini wrote:
> diff --git a/xen/include/xen/macros.h b/xen/include/xen/macros.h
> index d0caae7db298..bb9a1c9a53d0 100644
> --- a/xen/include/xen/macros.h
> +++ b/xen/include/xen/macros.h
> @@ -8,8 +8,10 @@
>  #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))
> +#define LOWEST_POW2(x) ((x) & -(x))

Naming wise, LOWEST_BIT() please.

The fact it's a power of two is incidental, and POW2 is ambiguous,
because it includes interpretations such as "calculate the lowest power
of two greater than x".

~Andrew

Re: [XEN PATCH][for-4.19 1/9] xen/include: add macro LOWEST_POW2

[Nicola Vetrini]

On 06/10/2023 18:35, andrew.cooper3@citrix.com wrote:
> On 06/10/2023 9:26 am, Nicola Vetrini wrote:
>> diff --git a/xen/include/xen/macros.h b/xen/include/xen/macros.h
>> index d0caae7db298..bb9a1c9a53d0 100644
>> --- a/xen/include/xen/macros.h
>> +++ b/xen/include/xen/macros.h
>> @@ -8,8 +8,10 @@
>>  #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))
>> +#define LOWEST_POW2(x) ((x) & -(x))
> 
> Naming wise, LOWEST_BIT() please.
> 
> The fact it's a power of two is incidental, and POW2 is ambiguous,
> because it includes interpretations such as "calculate the lowest power
> of two greater than x".
> 
> ~Andrew

You have a point. I'll change it.

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

[XEN PATCH][for-4.19 2/9] 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_POW2 macro.

No functional change.
---
 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..d119e1ccc952 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_POW2(__t)); })
+#define ffsl(x) ({ unsigned long __t = (x); flsl(LOWEST_POW2(__t)); })
 
 /**
  * find_first_set_bit - find the first set bit in @word
-- 
2.34.1

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

[Stefano Stabellini]

On Fri, 6 Oct 2023, Nicola Vetrini wrote:
> The definitions of ffs{l}? violate Rule 10.1, by using the well-known
> pattern (x & -x); its usage is wrapped by the LOWEST_POW2 macro.
> 
> No functional change.

Once we settle on a name for LOWEST_POW2 I can provided by reviewed-by
for this and the other patches in this series where LOWEST_POW2 is added


> ---
>  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..d119e1ccc952 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_POW2(__t)); })
> +#define ffsl(x) ({ unsigned long __t = (x); flsl(LOWEST_POW2(__t)); })
>  
>  /**
>   * find_first_set_bit - find the first set bit in @word
> -- 
> 2.34.1
> 

[XEN PATCH][for-4.19 3/9] 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_POW2 macro, which encapsulates the pattern.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
 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..5b88a4a2cd86 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_POW2(sizeof(*frame_table))))
 extern unsigned long pdx_group_valid[];
 
 /**
-- 
2.34.1

[XEN PATCH 4/9] x86_64/mm: express macro CNT using LOWEST_POW2

[Nicola Vetrini]

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

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
I do find somewhat odd the multiple identical #define-s and #undef-s of CNT in
this file, as well as the last
BUILD_BUG_ON((sizeof(*frame_table) & ~sizeof(*frame_table)) [...]
Perhaps here a cleanup is needed, to have a single definition of CNT and
identical BUILD_BUG_ON-s (wrapped in a macro, even)?
---
 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..8ae704f920a1 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_POW2(sizeof(*frame_table)) / \
              sizeof(*compat_machine_to_phys_mapping))
-    BUILD_BUG_ON((sizeof(*frame_table) & -sizeof(*frame_table)) % \
+    BUILD_BUG_ON(LOWEST_POW2(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_POW2(sizeof(*frame_table)) / \
              sizeof(*machine_to_phys_mapping))

-    BUILD_BUG_ON((sizeof(*frame_table) & -sizeof(*frame_table)) % \
+    BUILD_BUG_ON(LOWEST_POW2(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_POW2(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_POW2(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 5/9] x86/cpu-policy: address violations of MISRA C Rule 10.1

[Nicola Vetrini]

The COUNT_LEAVES macro is introduced to avoid using an essentially
boolean value in a subtraction.

No functional change.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
 xen/include/xen/lib/x86/cpu-policy.h | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/xen/include/xen/lib/x86/cpu-policy.h b/xen/include/xen/lib/x86/cpu-policy.h
index bab3eecda6c1..700993cc67e8 100644
--- a/xen/include/xen/lib/x86/cpu-policy.h
+++ b/xen/include/xen/lib/x86/cpu-policy.h
@@ -95,17 +95,18 @@ const char *x86_cpuid_vendor_to_str(unsigned int vendor);
 #define CPUID_GUEST_NR_EXTD       MAX(CPUID_GUEST_NR_EXTD_INTEL, \
                                       CPUID_GUEST_NR_EXTD_AMD)
 
+#define COUNT_LEAVES(X) ((X) - ((X) ? 1 : 0))
 /*
  * Maximum number of leaves a struct cpu_policy turns into when serialised for
  * interaction with the toolstack.  (Sum of all leaves in each union, less the
  * entries in basic which sub-unions hang off of.)
  */
-#define CPUID_MAX_SERIALISED_LEAVES                     \
-    (CPUID_GUEST_NR_BASIC +                             \
-     CPUID_GUEST_NR_FEAT   - !!CPUID_GUEST_NR_FEAT +    \
-     CPUID_GUEST_NR_CACHE  - !!CPUID_GUEST_NR_CACHE +   \
-     CPUID_GUEST_NR_TOPO   - !!CPUID_GUEST_NR_TOPO +    \
-     CPUID_GUEST_NR_XSTATE - !!CPUID_GUEST_NR_XSTATE +  \
+#define CPUID_MAX_SERIALISED_LEAVES         \
+    (CPUID_GUEST_NR_BASIC +                 \
+     COUNT_LEAVES(CPUID_GUEST_NR_FEAT) +    \
+     COUNT_LEAVES(CPUID_GUEST_NR_CACHE) +   \
+     COUNT_LEAVES(CPUID_GUEST_NR_TOPO) +    \
+     COUNT_LEAVES(CPUID_GUEST_NR_XSTATE) +  \
      CPUID_GUEST_NR_EXTD + 2 /* hv_limit and hv2_limit */ )
 
 /* Maximum number of MSRs written when serialising a cpu_policy. */
-- 
2.34.1

Re: [XEN PATCH 5/9] x86/cpu-policy: address violations of MISRA C Rule 10.1

[Andrew Cooper]

On 06/10/2023 9:26 am, Nicola Vetrini wrote:
> The COUNT_LEAVES macro is introduced to avoid using an essentially
> boolean value in a subtraction.
>
> No functional change.
>
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> ---
>  xen/include/xen/lib/x86/cpu-policy.h | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/xen/include/xen/lib/x86/cpu-policy.h b/xen/include/xen/lib/x86/cpu-policy.h
> index bab3eecda6c1..700993cc67e8 100644
> --- a/xen/include/xen/lib/x86/cpu-policy.h
> +++ b/xen/include/xen/lib/x86/cpu-policy.h
> @@ -95,17 +95,18 @@ const char *x86_cpuid_vendor_to_str(unsigned int vendor);
>  #define CPUID_GUEST_NR_EXTD       MAX(CPUID_GUEST_NR_EXTD_INTEL, \
>                                        CPUID_GUEST_NR_EXTD_AMD)
>  
> +#define COUNT_LEAVES(X) ((X) - ((X) ? 1 : 0))
>  /*
>   * Maximum number of leaves a struct cpu_policy turns into when serialised for
>   * interaction with the toolstack.  (Sum of all leaves in each union, less the
>   * entries in basic which sub-unions hang off of.)
>   */
> -#define CPUID_MAX_SERIALISED_LEAVES                     \
> -    (CPUID_GUEST_NR_BASIC +                             \
> -     CPUID_GUEST_NR_FEAT   - !!CPUID_GUEST_NR_FEAT +    \
> -     CPUID_GUEST_NR_CACHE  - !!CPUID_GUEST_NR_CACHE +   \
> -     CPUID_GUEST_NR_TOPO   - !!CPUID_GUEST_NR_TOPO +    \
> -     CPUID_GUEST_NR_XSTATE - !!CPUID_GUEST_NR_XSTATE +  \
> +#define CPUID_MAX_SERIALISED_LEAVES         \
> +    (CPUID_GUEST_NR_BASIC +                 \
> +     COUNT_LEAVES(CPUID_GUEST_NR_FEAT) +    \
> +     COUNT_LEAVES(CPUID_GUEST_NR_CACHE) +   \
> +     COUNT_LEAVES(CPUID_GUEST_NR_TOPO) +    \
> +     COUNT_LEAVES(CPUID_GUEST_NR_XSTATE) +  \
>       CPUID_GUEST_NR_EXTD + 2 /* hv_limit and hv2_limit */ )

This may not have been a MISRA-approved calculation, but encapsulating
it like this breaks any ability to follow what's going on.

CPUID data in x86 is mostly a sparse 1-D array (BASIC, EXTD, HV blocks),
but a couple of elements in the BASIC array have arrays themselves.

The struct is laid out for O(1) access, so you can't just say
sizeof(struct) / sizeof(element).  The BASIC array has elements (0x4,
0x7, 0xb, 0xd) which hold no data because there's actually an array
elsewhere containing all the data.

So logically, it's:

(BASIC + (FEAT - 1) + (CACHE - 1) + (TOPO - 1) + (XSTATE - 1)) + EXTD + 2

And in practice I'd far rather express it with a plain -1 than a -
!!NR_, if the latter isn't an option.

Presumably MISRA would be happy with that?

If so, I can submit a patch.  There's also a typo in that the comment
that wants fixing.

~Andrew

Re: [XEN PATCH 5/9] x86/cpu-policy: address violations of MISRA C Rule 10.1

[Nicola Vetrini]

On 06/10/2023 19:57, Andrew Cooper wrote:
> On 06/10/2023 9:26 am, Nicola Vetrini wrote:
>> The COUNT_LEAVES macro is introduced to avoid using an essentially
>> boolean value in a subtraction.
>> 
>> No functional change.
>> 
>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>> ---
>>  xen/include/xen/lib/x86/cpu-policy.h | 13 +++++++------
>>  1 file changed, 7 insertions(+), 6 deletions(-)
>> 
>> diff --git a/xen/include/xen/lib/x86/cpu-policy.h 
>> b/xen/include/xen/lib/x86/cpu-policy.h
>> index bab3eecda6c1..700993cc67e8 100644
>> --- a/xen/include/xen/lib/x86/cpu-policy.h
>> +++ b/xen/include/xen/lib/x86/cpu-policy.h
>> @@ -95,17 +95,18 @@ const char *x86_cpuid_vendor_to_str(unsigned int 
>> vendor);
>>  #define CPUID_GUEST_NR_EXTD       MAX(CPUID_GUEST_NR_EXTD_INTEL, \
>>                                        CPUID_GUEST_NR_EXTD_AMD)
>> 
>> +#define COUNT_LEAVES(X) ((X) - ((X) ? 1 : 0))
>>  /*
>>   * Maximum number of leaves a struct cpu_policy turns into when 
>> serialised for
>>   * interaction with the toolstack.  (Sum of all leaves in each union, 
>> less the
>>   * entries in basic which sub-unions hang off of.)
>>   */
>> -#define CPUID_MAX_SERIALISED_LEAVES                     \
>> -    (CPUID_GUEST_NR_BASIC +                             \
>> -     CPUID_GUEST_NR_FEAT   - !!CPUID_GUEST_NR_FEAT +    \
>> -     CPUID_GUEST_NR_CACHE  - !!CPUID_GUEST_NR_CACHE +   \
>> -     CPUID_GUEST_NR_TOPO   - !!CPUID_GUEST_NR_TOPO +    \
>> -     CPUID_GUEST_NR_XSTATE - !!CPUID_GUEST_NR_XSTATE +  \
>> +#define CPUID_MAX_SERIALISED_LEAVES         \
>> +    (CPUID_GUEST_NR_BASIC +                 \
>> +     COUNT_LEAVES(CPUID_GUEST_NR_FEAT) +    \
>> +     COUNT_LEAVES(CPUID_GUEST_NR_CACHE) +   \
>> +     COUNT_LEAVES(CPUID_GUEST_NR_TOPO) +    \
>> +     COUNT_LEAVES(CPUID_GUEST_NR_XSTATE) +  \
>>       CPUID_GUEST_NR_EXTD + 2 /* hv_limit and hv2_limit */ )
> 
> This may not have been a MISRA-approved calculation, but encapsulating
> it like this breaks any ability to follow what's going on.
> 
> CPUID data in x86 is mostly a sparse 1-D array (BASIC, EXTD, HV 
> blocks),
> but a couple of elements in the BASIC array have arrays themselves.
> 
> The struct is laid out for O(1) access, so you can't just say
> sizeof(struct) / sizeof(element).  The BASIC array has elements (0x4,
> 0x7, 0xb, 0xd) which hold no data because there's actually an array
> elsewhere containing all the data.
> 
> So logically, it's:
> 
> (BASIC + (FEAT - 1) + (CACHE - 1) + (TOPO - 1) + (XSTATE - 1)) + EXTD + 
> 2
> 
> And in practice I'd far rather express it with a plain -1 than a -
> !!NR_, if the latter isn't an option.
> 
> Presumably MISRA would be happy with that?
> 
> If so, I can submit a patch.  There's also a typo in that the comment
> that wants fixing.
> 
> ~Andrew

Yes, that should be fine. I'll be happy to test that.

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

[XEN PATCH 6/9] 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.
---
 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..a0fc50d601fe 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 6/9] x86/io_apic: address violation of MISRA C:2012 Rule 10.1

[Stefano Stabellini]

On Fri, 6 Oct 2023, 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.

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


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

[XEN PATCH 7/9] 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>
---
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

Re: [XEN PATCH 7/9] x86/mce: Move MC_NCLASSES into the enum mctelem_class

[andrew.cooper3]

On 06/10/2023 9:26 am, Nicola Vetrini wrote:
> 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>

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

I've started my normal for-next branch around releases, and included
this patch.

https://xenbits.xen.org/gitweb/?p=people/andrewcoop/xen.git;a=shortlog;h=refs/heads/for-next

I'll keep the rebased and add it to 4.19 when the tree re-opens.

~Andrew

Re: [XEN PATCH 7/9] x86/mce: Move MC_NCLASSES into the enum mctelem_class

[Nicola Vetrini]

On 06/10/2023 21:11, andrew.cooper3@citrix.com wrote:
> On 06/10/2023 9:26 am, Nicola Vetrini wrote:
>> 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>
> 
> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> I've started my normal for-next branch around releases, and included
> this patch.
> 
> https://xenbits.xen.org/gitweb/?p=people/andrewcoop/xen.git;a=shortlog;h=refs/heads/for-next
> 
> I'll keep the rebased and add it to 4.19 when the tree re-opens.
> 
> ~Andrew

Thanks, this helps a lot. I'll tag patches intended for this tree with 
[for-next].

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

[XEN PATCH][for-4.19 8/9] 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'.

A comment in BITS_TO_LONGS is added to make it clear that
values passed are meant to be positive.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
 xen/include/xen/iommu.h | 2 +-
 xen/include/xen/types.h | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
index 0e747b0bbc1c..34aa0b9b5b81 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -360,7 +360,7 @@ struct domain_iommu {
 #endif
 
     /* Features supported by the IOMMU */
-    DECLARE_BITMAP(features, IOMMU_FEAT_count);
+    DECLARE_BITMAP(features, (int)IOMMU_FEAT_count);
 
     /* Does the guest share HAP mapping with the IOMMU? */
     bool hap_pt_share;
diff --git a/xen/include/xen/types.h b/xen/include/xen/types.h
index aea259db1ef2..936e83d333a0 100644
--- a/xen/include/xen/types.h
+++ b/xen/include/xen/types.h
@@ -22,6 +22,7 @@ typedef signed long ssize_t;
 
 typedef __PTRDIFF_TYPE__ ptrdiff_t;
 
+/* Users of this macro are expected to pass a positive value */
 #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 8/9] xen/types: address Rule 10.1 for DECLARE_BITMAP use

[Julien Grall]

Hi Nicola,

On 06/10/2023 09:26, 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'.
> 
> A comment in BITS_TO_LONGS is added to make it clear that
> values passed are meant to be positive.

I am confused. If the value is meant to be positive. Then...

> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> ---
>   xen/include/xen/iommu.h | 2 +-
>   xen/include/xen/types.h | 1 +
>   2 files changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
> index 0e747b0bbc1c..34aa0b9b5b81 100644
> --- a/xen/include/xen/iommu.h
> +++ b/xen/include/xen/iommu.h
> @@ -360,7 +360,7 @@ struct domain_iommu {
>   #endif
>   
>       /* Features supported by the IOMMU */
> -    DECLARE_BITMAP(features, IOMMU_FEAT_count);
> +    DECLARE_BITMAP(features, (int)IOMMU_FEAT_count);

... why do we cast to (int) rather than (unsigned int)? Also, I think 
this cast deserve a comment on top because this is not a very obvious one.

>   
>       /* Does the guest share HAP mapping with the IOMMU? */
>       bool hap_pt_share;
> diff --git a/xen/include/xen/types.h b/xen/include/xen/types.h
> index aea259db1ef2..936e83d333a0 100644
> --- a/xen/include/xen/types.h
> +++ b/xen/include/xen/types.h
> @@ -22,6 +22,7 @@ typedef signed long ssize_t;
>   
>   typedef __PTRDIFF_TYPE__ ptrdiff_t;
>   
> +/* Users of this macro are expected to pass a positive value */
>   #define BITS_TO_LONGS(bits) \
>       (((bits)+BITS_PER_LONG-1)/BITS_PER_LONG)
>   #define DECLARE_BITMAP(name,bits) \

Cheers,

-- 
Julien Grall

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

[Nicola Vetrini]

On 06/10/2023 11:34, Julien Grall wrote:
> Hi Nicola,
> 
> On 06/10/2023 09:26, 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'.
>> 
>> A comment in BITS_TO_LONGS is added to make it clear that
>> values passed are meant to be positive.
> 
> I am confused. If the value is meant to be positive. Then...
> 
>> 
>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>> ---
>>   xen/include/xen/iommu.h | 2 +-
>>   xen/include/xen/types.h | 1 +
>>   2 files changed, 2 insertions(+), 1 deletion(-)
>> 
>> diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
>> index 0e747b0bbc1c..34aa0b9b5b81 100644
>> --- a/xen/include/xen/iommu.h
>> +++ b/xen/include/xen/iommu.h
>> @@ -360,7 +360,7 @@ struct domain_iommu {
>>   #endif
>>         /* Features supported by the IOMMU */
>> -    DECLARE_BITMAP(features, IOMMU_FEAT_count);
>> +    DECLARE_BITMAP(features, (int)IOMMU_FEAT_count);
> 
> ... why do we cast to (int) rather than (unsigned int)? Also, I think
> this cast deserve a comment on top because this is not a very obvious
> one.
> 
>>         /* Does the guest share HAP mapping with the IOMMU? */
>>       bool hap_pt_share;
>> diff --git a/xen/include/xen/types.h b/xen/include/xen/types.h
>> index aea259db1ef2..936e83d333a0 100644
>> --- a/xen/include/xen/types.h
>> +++ b/xen/include/xen/types.h
>> @@ -22,6 +22,7 @@ typedef signed long ssize_t;
>>     typedef __PTRDIFF_TYPE__ ptrdiff_t;
>>   +/* Users of this macro are expected to pass a positive value */
>>   #define BITS_TO_LONGS(bits) \
>>       (((bits)+BITS_PER_LONG-1)/BITS_PER_LONG)
>>   #define DECLARE_BITMAP(name,bits) \
> 
> Cheers,

See [1] for the reason why I did so. I should have mentioned that in the 
commit notes, sorry.
In short, making BITS_TO_LONGS essentially unsigned would cause a 
cascade of build errors and
possibly other essential type violations. If this is to be fixed that 
way, the effort required
is far greater. Either way, a comment on top of can be added, along the 
lines of:

Leaving this as an enum would violate MISRA C:2012 Rule 10.1

[1] 
https://lore.kernel.org/xen-devel/6495ba58bda01eae1f4baa46096424eb@bugseng.com/

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

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

[Julien Grall]

Hi Nicola,

On 06/10/2023 11:10, Nicola Vetrini wrote:
> On 06/10/2023 11:34, Julien Grall wrote:
>> Hi Nicola,
>>
>> On 06/10/2023 09:26, 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'.
>>>
>>> A comment in BITS_TO_LONGS is added to make it clear that
>>> values passed are meant to be positive.
>>
>> I am confused. If the value is meant to be positive. Then...
>>
>>>
>>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>>> ---
>>>   xen/include/xen/iommu.h | 2 +-
>>>   xen/include/xen/types.h | 1 +
>>>   2 files changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
>>> index 0e747b0bbc1c..34aa0b9b5b81 100644
>>> --- a/xen/include/xen/iommu.h
>>> +++ b/xen/include/xen/iommu.h
>>> @@ -360,7 +360,7 @@ struct domain_iommu {
>>>   #endif
>>>         /* Features supported by the IOMMU */
>>> -    DECLARE_BITMAP(features, IOMMU_FEAT_count);
>>> +    DECLARE_BITMAP(features, (int)IOMMU_FEAT_count);
>>
>> ... why do we cast to (int) rather than (unsigned int)? Also, I think
>> this cast deserve a comment on top because this is not a very obvious
>> one.
>>
>>>         /* Does the guest share HAP mapping with the IOMMU? */
>>>       bool hap_pt_share;
>>> diff --git a/xen/include/xen/types.h b/xen/include/xen/types.h
>>> index aea259db1ef2..936e83d333a0 100644
>>> --- a/xen/include/xen/types.h
>>> +++ b/xen/include/xen/types.h
>>> @@ -22,6 +22,7 @@ typedef signed long ssize_t;
>>>     typedef __PTRDIFF_TYPE__ ptrdiff_t;
>>>   +/* Users of this macro are expected to pass a positive value */
>>>   #define BITS_TO_LONGS(bits) \
>>>       (((bits)+BITS_PER_LONG-1)/BITS_PER_LONG)
>>>   #define DECLARE_BITMAP(name,bits) \
>>
>> Cheers,
> 
> See [1] for the reason why I did so. I should have mentioned that in the 
> commit notes, sorry.
> In short, making BITS_TO_LONGS essentially unsigned would cause a 
> cascade of build errors and
> possibly other essential type violations.
Can you share some of the errors?

> If this is to be fixed that 
> way, the effort required
> is far greater. Either way, a comment on top of can be added, along the 
> lines of:
> 
> Leaving this as an enum would violate MISRA C:2012 Rule 10.1

I read this as you are simply trying to silence your tool. I think this 
the wrong approach as you are now making the code confusing for the 
reader (you pass a signed int to a function that is supposed to take a 
positive number).

I appreciate that this will result to more violations at the beginning. 
But the whole point of MISRA is to make the code better.

If this is too complex to solve now, then a possible option is to 
deviate for the time being.

Cheers,

> 
> [1] 
> https://lore.kernel.org/xen-devel/6495ba58bda01eae1f4baa46096424eb@bugseng.com/
> 

-- 
Julien Grall

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

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 3904 bytes --]

On Fri, 6 Oct 2023, Julien Grall wrote:
> Hi Nicola,
> 
> On 06/10/2023 11:10, Nicola Vetrini wrote:
> > On 06/10/2023 11:34, Julien Grall wrote:
> > > Hi Nicola,
> > > 
> > > On 06/10/2023 09:26, 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'.
> > > > 
> > > > A comment in BITS_TO_LONGS is added to make it clear that
> > > > values passed are meant to be positive.
> > > 
> > > I am confused. If the value is meant to be positive. Then...
> > > 
> > > > 
> > > > Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> > > > ---
> > > >   xen/include/xen/iommu.h | 2 +-
> > > >   xen/include/xen/types.h | 1 +
> > > >   2 files changed, 2 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
> > > > index 0e747b0bbc1c..34aa0b9b5b81 100644
> > > > --- a/xen/include/xen/iommu.h
> > > > +++ b/xen/include/xen/iommu.h
> > > > @@ -360,7 +360,7 @@ struct domain_iommu {
> > > >   #endif
> > > >         /* Features supported by the IOMMU */
> > > > -    DECLARE_BITMAP(features, IOMMU_FEAT_count);
> > > > +    DECLARE_BITMAP(features, (int)IOMMU_FEAT_count);
> > > 
> > > ... why do we cast to (int) rather than (unsigned int)? Also, I think
> > > this cast deserve a comment on top because this is not a very obvious
> > > one.
> > > 
> > > >         /* Does the guest share HAP mapping with the IOMMU? */
> > > >       bool hap_pt_share;
> > > > diff --git a/xen/include/xen/types.h b/xen/include/xen/types.h
> > > > index aea259db1ef2..936e83d333a0 100644
> > > > --- a/xen/include/xen/types.h
> > > > +++ b/xen/include/xen/types.h
> > > > @@ -22,6 +22,7 @@ typedef signed long ssize_t;
> > > >     typedef __PTRDIFF_TYPE__ ptrdiff_t;
> > > >   +/* Users of this macro are expected to pass a positive value */
> > > >   #define BITS_TO_LONGS(bits) \
> > > >       (((bits)+BITS_PER_LONG-1)/BITS_PER_LONG)
> > > >   #define DECLARE_BITMAP(name,bits) \
> > > 
> > > Cheers,
> > 
> > See [1] for the reason why I did so. I should have mentioned that in the
> > commit notes, sorry.
> > In short, making BITS_TO_LONGS essentially unsigned would cause a cascade of
> > build errors and
> > possibly other essential type violations.
> Can you share some of the errors?
> 
> > If this is to be fixed that way, the effort required
> > is far greater. Either way, a comment on top of can be added, along the
> > lines of:
> > 
> > Leaving this as an enum would violate MISRA C:2012 Rule 10.1
> 
> I read this as you are simply trying to silence your tool. I think this the
> wrong approach as you are now making the code confusing for the reader (you
> pass a signed int to a function that is supposed to take a positive number).
> 
> I appreciate that this will result to more violations at the beginning. But
> the whole point of MISRA is to make the code better.
> 
> If this is too complex to solve now, then a possible option is to deviate for
> the time being.

I agree on everything Julien's wrote and I was about to suggest to use a
SAF comment to suppress the warning because it is clearer than a int
cast.

But then I realized that even if BITS_TO_LONGS was fixed, wouldn't still
we have a problem because IOMMU_FEAT_count is an enum?

Is it the case that IOMMU_FEAT_count would have to be cast regardless,
either to int or unsigned int or whatever to be used in DECLARE_BITMAP?


So we have 2 problems here: one problem is DECLARE_BITMAP taking int
instead of unsigned int, and another problem is IOMMU_FEAT_count being
an enum.

If I got it right, then I would go with the cast to int (like done in
this patch) with a decent comment on top of it.

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

[Nicola Vetrini]

On 07/10/2023 03:04, Stefano Stabellini wrote:
> On Fri, 6 Oct 2023, Julien Grall wrote:
>> Hi Nicola,
>> 
>> On 06/10/2023 11:10, Nicola Vetrini wrote:
>> > On 06/10/2023 11:34, Julien Grall wrote:
>> > > Hi Nicola,
>> > >
>> > > On 06/10/2023 09:26, 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'.
>> > > >
>> > > > A comment in BITS_TO_LONGS is added to make it clear that
>> > > > values passed are meant to be positive.
>> > >
>> > > I am confused. If the value is meant to be positive. Then...
>> > >
>> > > >
>> > > > Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>> > > > ---
>> > > >   xen/include/xen/iommu.h | 2 +-
>> > > >   xen/include/xen/types.h | 1 +
>> > > >   2 files changed, 2 insertions(+), 1 deletion(-)
>> > > >
>> > > > diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
>> > > > index 0e747b0bbc1c..34aa0b9b5b81 100644
>> > > > --- a/xen/include/xen/iommu.h
>> > > > +++ b/xen/include/xen/iommu.h
>> > > > @@ -360,7 +360,7 @@ struct domain_iommu {
>> > > >   #endif
>> > > >         /* Features supported by the IOMMU */
>> > > > -    DECLARE_BITMAP(features, IOMMU_FEAT_count);
>> > > > +    DECLARE_BITMAP(features, (int)IOMMU_FEAT_count);
>> > >
>> > > ... why do we cast to (int) rather than (unsigned int)? Also, I think
>> > > this cast deserve a comment on top because this is not a very obvious
>> > > one.
>> > >
>> > > >         /* Does the guest share HAP mapping with the IOMMU? */
>> > > >       bool hap_pt_share;
>> > > > diff --git a/xen/include/xen/types.h b/xen/include/xen/types.h
>> > > > index aea259db1ef2..936e83d333a0 100644
>> > > > --- a/xen/include/xen/types.h
>> > > > +++ b/xen/include/xen/types.h
>> > > > @@ -22,6 +22,7 @@ typedef signed long ssize_t;
>> > > >     typedef __PTRDIFF_TYPE__ ptrdiff_t;
>> > > >   +/* Users of this macro are expected to pass a positive value */
>> > > >   #define BITS_TO_LONGS(bits) \
>> > > >       (((bits)+BITS_PER_LONG-1)/BITS_PER_LONG)
>> > > >   #define DECLARE_BITMAP(name,bits) \
>> > >
>> > > Cheers,
>> >
>> > See [1] for the reason why I did so. I should have mentioned that in the
>> > commit notes, sorry.
>> > In short, making BITS_TO_LONGS essentially unsigned would cause a cascade of
>> > build errors and
>> > possibly other essential type violations.
>> Can you share some of the errors?
>> 
>> > If this is to be fixed that way, the effort required
>> > is far greater. Either way, a comment on top of can be added, along the
>> > lines of:
>> >
>> > Leaving this as an enum would violate MISRA C:2012 Rule 10.1
>> 
>> I read this as you are simply trying to silence your tool. I think 
>> this the
>> wrong approach as you are now making the code confusing for the reader 
>> (you
>> pass a signed int to a function that is supposed to take a positive 
>> number).
>> 
>> I appreciate that this will result to more violations at the 
>> beginning. But
>> the whole point of MISRA is to make the code better.
>> 
>> If this is too complex to solve now, then a possible option is to 
>> deviate for
>> the time being.
> 
> I agree on everything Julien's wrote and I was about to suggest to use 
> a
> SAF comment to suppress the warning because it is clearer than a int
> cast.
> 
> But then I realized that even if BITS_TO_LONGS was fixed, wouldn't 
> still
> we have a problem because IOMMU_FEAT_count is an enum?
> 

Indeed. MISRA interprets enums as unordered sets of constants.

> Is it the case that IOMMU_FEAT_count would have to be cast regardless,
> either to int or unsigned int or whatever to be used in DECLARE_BITMAP?
> 

Correct. See also my reply to Julien's message: right now 
value-preserving conversions
do not generate violations, but a non-value preserving conversion from 
unsigned to signed
will generate one.

> 
> So we have 2 problems here: one problem is DECLARE_BITMAP taking int
> instead of unsigned int, and another problem is IOMMU_FEAT_count being
> an enum.
> 
> If I got it right, then I would go with the cast to int (like done in
> this patch) with a decent comment on top of it.

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

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

[Julien Grall]

On 07/10/2023 02:04, Stefano Stabellini wrote:
> On Fri, 6 Oct 2023, Julien Grall wrote:
>> Hi Nicola,
>>
>> On 06/10/2023 11:10, Nicola Vetrini wrote:
>>> On 06/10/2023 11:34, Julien Grall wrote:
>>>> Hi Nicola,
>>>>
>>>> On 06/10/2023 09:26, 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'.
>>>>>
>>>>> A comment in BITS_TO_LONGS is added to make it clear that
>>>>> values passed are meant to be positive.
>>>>
>>>> I am confused. If the value is meant to be positive. Then...
>>>>
>>>>>
>>>>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>>>>> ---
>>>>>    xen/include/xen/iommu.h | 2 +-
>>>>>    xen/include/xen/types.h | 1 +
>>>>>    2 files changed, 2 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
>>>>> index 0e747b0bbc1c..34aa0b9b5b81 100644
>>>>> --- a/xen/include/xen/iommu.h
>>>>> +++ b/xen/include/xen/iommu.h
>>>>> @@ -360,7 +360,7 @@ struct domain_iommu {
>>>>>    #endif
>>>>>          /* Features supported by the IOMMU */
>>>>> -    DECLARE_BITMAP(features, IOMMU_FEAT_count);
>>>>> +    DECLARE_BITMAP(features, (int)IOMMU_FEAT_count);
>>>>
>>>> ... why do we cast to (int) rather than (unsigned int)? Also, I think
>>>> this cast deserve a comment on top because this is not a very obvious
>>>> one.
>>>>
>>>>>          /* Does the guest share HAP mapping with the IOMMU? */
>>>>>        bool hap_pt_share;
>>>>> diff --git a/xen/include/xen/types.h b/xen/include/xen/types.h
>>>>> index aea259db1ef2..936e83d333a0 100644
>>>>> --- a/xen/include/xen/types.h
>>>>> +++ b/xen/include/xen/types.h
>>>>> @@ -22,6 +22,7 @@ typedef signed long ssize_t;
>>>>>      typedef __PTRDIFF_TYPE__ ptrdiff_t;
>>>>>    +/* Users of this macro are expected to pass a positive value */
>>>>>    #define BITS_TO_LONGS(bits) \
>>>>>        (((bits)+BITS_PER_LONG-1)/BITS_PER_LONG)
>>>>>    #define DECLARE_BITMAP(name,bits) \
>>>>
>>>> Cheers,
>>>
>>> See [1] for the reason why I did so. I should have mentioned that in the
>>> commit notes, sorry.
>>> In short, making BITS_TO_LONGS essentially unsigned would cause a cascade of
>>> build errors and
>>> possibly other essential type violations.
>> Can you share some of the errors?
>>
>>> If this is to be fixed that way, the effort required
>>> is far greater. Either way, a comment on top of can be added, along the
>>> lines of:
>>>
>>> Leaving this as an enum would violate MISRA C:2012 Rule 10.1
>>
>> I read this as you are simply trying to silence your tool. I think this the
>> wrong approach as you are now making the code confusing for the reader (you
>> pass a signed int to a function that is supposed to take a positive number).
>>
>> I appreciate that this will result to more violations at the beginning. But
>> the whole point of MISRA is to make the code better.
>>
>> If this is too complex to solve now, then a possible option is to deviate for
>> the time being.
> 
> I agree on everything Julien's wrote and I was about to suggest to use a
> SAF comment to suppress the warning because it is clearer than a int
> cast.
> 
> But then I realized that even if BITS_TO_LONGS was fixed, wouldn't still
> we have a problem because IOMMU_FEAT_count is an enum?
> 
> Is it the case that IOMMU_FEAT_count would have to be cast regardless,
> either to int or unsigned int or whatever to be used in DECLARE_BITMAP?
> 
> 
> So we have 2 problems here: one problem is DECLARE_BITMAP taking int
> instead of unsigned int, and another problem is IOMMU_FEAT_count being
> an enum.
> 
> If I got it right, then I would go with the cast to int (like done in
> this patch) with a decent comment on top of it.

I might be missing something here. But why should we use cast rather 
than /* SAF-X */ just above? I would have expected we wanted to 
highlight the violation rather than hiding it.

Ultimately, the code is mantained by Jan. So this is his call.

Cheers,

-- 
Julien Grall

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

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 5524 bytes --]

On Mon, 9 Oct 2023, Julien Grall wrote:
> On 07/10/2023 02:04, Stefano Stabellini wrote:
> > On Fri, 6 Oct 2023, Julien Grall wrote:
> > > Hi Nicola,
> > > 
> > > On 06/10/2023 11:10, Nicola Vetrini wrote:
> > > > On 06/10/2023 11:34, Julien Grall wrote:
> > > > > Hi Nicola,
> > > > > 
> > > > > On 06/10/2023 09:26, 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'.
> > > > > > 
> > > > > > A comment in BITS_TO_LONGS is added to make it clear that
> > > > > > values passed are meant to be positive.
> > > > > 
> > > > > I am confused. If the value is meant to be positive. Then...
> > > > > 
> > > > > > 
> > > > > > Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> > > > > > ---
> > > > > >    xen/include/xen/iommu.h | 2 +-
> > > > > >    xen/include/xen/types.h | 1 +
> > > > > >    2 files changed, 2 insertions(+), 1 deletion(-)
> > > > > > 
> > > > > > diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
> > > > > > index 0e747b0bbc1c..34aa0b9b5b81 100644
> > > > > > --- a/xen/include/xen/iommu.h
> > > > > > +++ b/xen/include/xen/iommu.h
> > > > > > @@ -360,7 +360,7 @@ struct domain_iommu {
> > > > > >    #endif
> > > > > >          /* Features supported by the IOMMU */
> > > > > > -    DECLARE_BITMAP(features, IOMMU_FEAT_count);
> > > > > > +    DECLARE_BITMAP(features, (int)IOMMU_FEAT_count);
> > > > > 
> > > > > ... why do we cast to (int) rather than (unsigned int)? Also, I think
> > > > > this cast deserve a comment on top because this is not a very obvious
> > > > > one.
> > > > > 
> > > > > >          /* Does the guest share HAP mapping with the IOMMU? */
> > > > > >        bool hap_pt_share;
> > > > > > diff --git a/xen/include/xen/types.h b/xen/include/xen/types.h
> > > > > > index aea259db1ef2..936e83d333a0 100644
> > > > > > --- a/xen/include/xen/types.h
> > > > > > +++ b/xen/include/xen/types.h
> > > > > > @@ -22,6 +22,7 @@ typedef signed long ssize_t;
> > > > > >      typedef __PTRDIFF_TYPE__ ptrdiff_t;
> > > > > >    +/* Users of this macro are expected to pass a positive value */
> > > > > >    #define BITS_TO_LONGS(bits) \
> > > > > >        (((bits)+BITS_PER_LONG-1)/BITS_PER_LONG)
> > > > > >    #define DECLARE_BITMAP(name,bits) \
> > > > > 
> > > > > Cheers,
> > > > 
> > > > See [1] for the reason why I did so. I should have mentioned that in the
> > > > commit notes, sorry.
> > > > In short, making BITS_TO_LONGS essentially unsigned would cause a
> > > > cascade of
> > > > build errors and
> > > > possibly other essential type violations.
> > > Can you share some of the errors?
> > > 
> > > > If this is to be fixed that way, the effort required
> > > > is far greater. Either way, a comment on top of can be added, along the
> > > > lines of:
> > > > 
> > > > Leaving this as an enum would violate MISRA C:2012 Rule 10.1
> > > 
> > > I read this as you are simply trying to silence your tool. I think this
> > > the
> > > wrong approach as you are now making the code confusing for the reader
> > > (you
> > > pass a signed int to a function that is supposed to take a positive
> > > number).
> > > 
> > > I appreciate that this will result to more violations at the beginning.
> > > But
> > > the whole point of MISRA is to make the code better.
> > > 
> > > If this is too complex to solve now, then a possible option is to deviate
> > > for
> > > the time being.
> > 
> > I agree on everything Julien's wrote and I was about to suggest to use a
> > SAF comment to suppress the warning because it is clearer than a int
> > cast.
> > 
> > But then I realized that even if BITS_TO_LONGS was fixed, wouldn't still
> > we have a problem because IOMMU_FEAT_count is an enum?
> > 
> > Is it the case that IOMMU_FEAT_count would have to be cast regardless,
> > either to int or unsigned int or whatever to be used in DECLARE_BITMAP?
> > 
> > 
> > So we have 2 problems here: one problem is DECLARE_BITMAP taking int
> > instead of unsigned int, and another problem is IOMMU_FEAT_count being
> > an enum.
> > 
> > If I got it right, then I would go with the cast to int (like done in
> > this patch) with a decent comment on top of it.
> 
> I might be missing something here. But why should we use cast rather than /*
> SAF-X */ just above? I would have expected we wanted to highlight the
> violation rather than hiding it.

My understanding is that the cast is required when converting an enum
type to an integer type and vice versa. The idea is that we shouldn't do
implicit conversions as they are error prone, only explicit conversions
are allowed between enum and integers.

So we need either (int) or (unsigned int). The question is which one
between the two, and theoretically (unsigned int) is better but due to
the reasons above (int) is the simplest choice.

Yes, instead of the cast we can also add a SAF-x comment, which refers
to a deviation that says something along the lines "we could fix this
with a cast but we prefer a deviation because it makes the code easier
to read".

In general my personal preference would be to use a cast, because we
shouldn't implicitly convert enums to integers. However, in this
specific case where there is int vs. unsigned int discussion, I can see
that also SAF-x could be a way to go.

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

[Julien Grall]

On 10/10/2023 02:09, Stefano Stabellini wrote:
> On Mon, 9 Oct 2023, Julien Grall wrote:
>> On 07/10/2023 02:04, Stefano Stabellini wrote:
>>> On Fri, 6 Oct 2023, Julien Grall wrote:
>>>> Hi Nicola,
>>>>
>>>> On 06/10/2023 11:10, Nicola Vetrini wrote:
>>>>> On 06/10/2023 11:34, Julien Grall wrote:
>>>>>> Hi Nicola,
>>>>>>
>>>>>> On 06/10/2023 09:26, 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'.
>>>>>>>
>>>>>>> A comment in BITS_TO_LONGS is added to make it clear that
>>>>>>> values passed are meant to be positive.
>>>>>>
>>>>>> I am confused. If the value is meant to be positive. Then...
>>>>>>
>>>>>>>
>>>>>>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>>>>>>> ---
>>>>>>>     xen/include/xen/iommu.h | 2 +-
>>>>>>>     xen/include/xen/types.h | 1 +
>>>>>>>     2 files changed, 2 insertions(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
>>>>>>> index 0e747b0bbc1c..34aa0b9b5b81 100644
>>>>>>> --- a/xen/include/xen/iommu.h
>>>>>>> +++ b/xen/include/xen/iommu.h
>>>>>>> @@ -360,7 +360,7 @@ struct domain_iommu {
>>>>>>>     #endif
>>>>>>>           /* Features supported by the IOMMU */
>>>>>>> -    DECLARE_BITMAP(features, IOMMU_FEAT_count);
>>>>>>> +    DECLARE_BITMAP(features, (int)IOMMU_FEAT_count);
>>>>>>
>>>>>> ... why do we cast to (int) rather than (unsigned int)? Also, I think
>>>>>> this cast deserve a comment on top because this is not a very obvious
>>>>>> one.
>>>>>>
>>>>>>>           /* Does the guest share HAP mapping with the IOMMU? */
>>>>>>>         bool hap_pt_share;
>>>>>>> diff --git a/xen/include/xen/types.h b/xen/include/xen/types.h
>>>>>>> index aea259db1ef2..936e83d333a0 100644
>>>>>>> --- a/xen/include/xen/types.h
>>>>>>> +++ b/xen/include/xen/types.h
>>>>>>> @@ -22,6 +22,7 @@ typedef signed long ssize_t;
>>>>>>>       typedef __PTRDIFF_TYPE__ ptrdiff_t;
>>>>>>>     +/* Users of this macro are expected to pass a positive value */
>>>>>>>     #define BITS_TO_LONGS(bits) \
>>>>>>>         (((bits)+BITS_PER_LONG-1)/BITS_PER_LONG)
>>>>>>>     #define DECLARE_BITMAP(name,bits) \
>>>>>>
>>>>>> Cheers,
>>>>>
>>>>> See [1] for the reason why I did so. I should have mentioned that in the
>>>>> commit notes, sorry.
>>>>> In short, making BITS_TO_LONGS essentially unsigned would cause a
>>>>> cascade of
>>>>> build errors and
>>>>> possibly other essential type violations.
>>>> Can you share some of the errors?
>>>>
>>>>> If this is to be fixed that way, the effort required
>>>>> is far greater. Either way, a comment on top of can be added, along the
>>>>> lines of:
>>>>>
>>>>> Leaving this as an enum would violate MISRA C:2012 Rule 10.1
>>>>
>>>> I read this as you are simply trying to silence your tool. I think this
>>>> the
>>>> wrong approach as you are now making the code confusing for the reader
>>>> (you
>>>> pass a signed int to a function that is supposed to take a positive
>>>> number).
>>>>
>>>> I appreciate that this will result to more violations at the beginning.
>>>> But
>>>> the whole point of MISRA is to make the code better.
>>>>
>>>> If this is too complex to solve now, then a possible option is to deviate
>>>> for
>>>> the time being.
>>>
>>> I agree on everything Julien's wrote and I was about to suggest to use a
>>> SAF comment to suppress the warning because it is clearer than a int
>>> cast.
>>>
>>> But then I realized that even if BITS_TO_LONGS was fixed, wouldn't still
>>> we have a problem because IOMMU_FEAT_count is an enum?
>>>
>>> Is it the case that IOMMU_FEAT_count would have to be cast regardless,
>>> either to int or unsigned int or whatever to be used in DECLARE_BITMAP?
>>>
>>>
>>> So we have 2 problems here: one problem is DECLARE_BITMAP taking int
>>> instead of unsigned int, and another problem is IOMMU_FEAT_count being
>>> an enum.
>>>
>>> If I got it right, then I would go with the cast to int (like done in
>>> this patch) with a decent comment on top of it.
>>
>> I might be missing something here. But why should we use cast rather than /*
>> SAF-X */ just above? I would have expected we wanted to highlight the
>> violation rather than hiding it.
> 
> My understanding is that the cast is required when converting an enum
> type to an integer type and vice versa. The idea is that we shouldn't do
> implicit conversions as they are error prone, only explicit conversions
> are allowed between enum and integers.

We have a lot of places in Xen using implicit conversion from enum to 
integer (for instance in the P2M code for p2m_type_t). Does ECLAIR 
report all of them? If not, then why?

> 
> So we need either (int) or (unsigned int). The question is which one
> between the two, and theoretically (unsigned int) is better but due to
> the reasons above (int) is the simplest choice.
> 
> Yes, instead of the cast we can also add a SAF-x comment, which refers
> to a deviation that says something along the lines "we could fix this
> with a cast but we prefer a deviation because it makes the code easier
> to read".
> 
> In general my personal preference would be to use a cast, because we
> shouldn't implicitly convert enums to integers.

See above. I'd like to understand whether we are going to sprinkle the 
code with cast. If so, I am afraid I am against this solution.

Cheers,

-- 
Julien Grall

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

[Nicola Vetrini]

>>>> 
>>>> I agree on everything Julien's wrote and I was about to suggest to 
>>>> use a
>>>> SAF comment to suppress the warning because it is clearer than a int
>>>> cast.
>>>> 
>>>> But then I realized that even if BITS_TO_LONGS was fixed, wouldn't 
>>>> still
>>>> we have a problem because IOMMU_FEAT_count is an enum?
>>>> 
>>>> Is it the case that IOMMU_FEAT_count would have to be cast 
>>>> regardless,
>>>> either to int or unsigned int or whatever to be used in 
>>>> DECLARE_BITMAP?
>>>> 
>>>> 
>>>> So we have 2 problems here: one problem is DECLARE_BITMAP taking int
>>>> instead of unsigned int, and another problem is IOMMU_FEAT_count 
>>>> being
>>>> an enum.
>>>> 
>>>> If I got it right, then I would go with the cast to int (like done 
>>>> in
>>>> this patch) with a decent comment on top of it.
>>> 
>>> I might be missing something here. But why should we use cast rather 
>>> than /*
>>> SAF-X */ just above? I would have expected we wanted to highlight the
>>> violation rather than hiding it.
>> 
>> My understanding is that the cast is required when converting an enum
>> type to an integer type and vice versa. The idea is that we shouldn't 
>> do
>> implicit conversions as they are error prone, only explicit 
>> conversions
>> are allowed between enum and integers.
> 
> We have a lot of places in Xen using implicit conversion from enum to
> integer (for instance in the P2M code for p2m_type_t). Does ECLAIR
> report all of them? If not, then why?
> 

Can you give some pointers as to where this enum is used in arithmetic 
operations?
 From a cursory glace I can see equality comparisons and
as arguments to the array subscript operator, which are both compliant.

>> 
>> So we need either (int) or (unsigned int). The question is which one
>> between the two, and theoretically (unsigned int) is better but due to
>> the reasons above (int) is the simplest choice.
>> 
>> Yes, instead of the cast we can also add a SAF-x comment, which refers
>> to a deviation that says something along the lines "we could fix this
>> with a cast but we prefer a deviation because it makes the code easier
>> to read".
>> 
>> In general my personal preference would be to use a cast, because we
>> shouldn't implicitly convert enums to integers.
> 
> See above. I'd like to understand whether we are going to sprinkle the
> code with cast. If so, I am afraid I am against this solution.
> 
> Cheers,

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

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

[Julien Grall]

On 10/10/2023 13:07, Nicola Vetrini wrote:
> 
>>>>>
>>>>> I agree on everything Julien's wrote and I was about to suggest to 
>>>>> use a
>>>>> SAF comment to suppress the warning because it is clearer than a int
>>>>> cast.
>>>>>
>>>>> But then I realized that even if BITS_TO_LONGS was fixed, wouldn't 
>>>>> still
>>>>> we have a problem because IOMMU_FEAT_count is an enum?
>>>>>
>>>>> Is it the case that IOMMU_FEAT_count would have to be cast regardless,
>>>>> either to int or unsigned int or whatever to be used in 
>>>>> DECLARE_BITMAP?
>>>>>
>>>>>
>>>>> So we have 2 problems here: one problem is DECLARE_BITMAP taking int
>>>>> instead of unsigned int, and another problem is IOMMU_FEAT_count being
>>>>> an enum.
>>>>>
>>>>> If I got it right, then I would go with the cast to int (like done in
>>>>> this patch) with a decent comment on top of it.
>>>>
>>>> I might be missing something here. But why should we use cast rather 
>>>> than /*
>>>> SAF-X */ just above? I would have expected we wanted to highlight the
>>>> violation rather than hiding it.
>>>
>>> My understanding is that the cast is required when converting an enum
>>> type to an integer type and vice versa. The idea is that we shouldn't do
>>> implicit conversions as they are error prone, only explicit conversions
>>> are allowed between enum and integers.
>>
>> We have a lot of places in Xen using implicit conversion from enum to
>> integer (for instance in the P2M code for p2m_type_t). Does ECLAIR
>> report all of them? If not, then why?
>>
> 
> Can you give some pointers as to where this enum is used in arithmetic 
> operations?

I can't think of any right now. But reply from Stefano suggested that it 
was necessary anytime we were using 'enum' as an '(unsigned) int'.

>  From a cursory glace I can see equality comparisons and
> as arguments to the array subscript operator, which are both compliant.

How about assigning an enum to an '(unsigned) int : X'? (where X is the 
number of bits.

Cheers,

-- 
Julien Grall

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

[Julien Grall]

On 10/10/2023 13:13, Julien Grall wrote:
> 
> 
> On 10/10/2023 13:07, Nicola Vetrini wrote:
>>
>>>>>>
>>>>>> I agree on everything Julien's wrote and I was about to suggest to 
>>>>>> use a
>>>>>> SAF comment to suppress the warning because it is clearer than a int
>>>>>> cast.
>>>>>>
>>>>>> But then I realized that even if BITS_TO_LONGS was fixed, wouldn't 
>>>>>> still
>>>>>> we have a problem because IOMMU_FEAT_count is an enum?
>>>>>>
>>>>>> Is it the case that IOMMU_FEAT_count would have to be cast 
>>>>>> regardless,
>>>>>> either to int or unsigned int or whatever to be used in 
>>>>>> DECLARE_BITMAP?
>>>>>>
>>>>>>
>>>>>> So we have 2 problems here: one problem is DECLARE_BITMAP taking int
>>>>>> instead of unsigned int, and another problem is IOMMU_FEAT_count 
>>>>>> being
>>>>>> an enum.
>>>>>>
>>>>>> If I got it right, then I would go with the cast to int (like done in
>>>>>> this patch) with a decent comment on top of it.
>>>>>
>>>>> I might be missing something here. But why should we use cast 
>>>>> rather than /*
>>>>> SAF-X */ just above? I would have expected we wanted to highlight the
>>>>> violation rather than hiding it.
>>>>
>>>> My understanding is that the cast is required when converting an enum
>>>> type to an integer type and vice versa. The idea is that we 
>>>> shouldn't do
>>>> implicit conversions as they are error prone, only explicit conversions
>>>> are allowed between enum and integers.
>>>
>>> We have a lot of places in Xen using implicit conversion from enum to
>>> integer (for instance in the P2M code for p2m_type_t). Does ECLAIR
>>> report all of them? If not, then why?
>>>
>>
>> Can you give some pointers as to where this enum is used in arithmetic 
>> operations?
> 
> I can't think of any right now.

Obviously, right after I pressed send, I remember of one in 
__acpi_map_table() (x86 code).

FIX_ACPI_END is an enum, assigned to an 'int' and then used in arithmetics.

Cheers,

-- 
Julien Grall

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

[Nicola Vetrini]

On 10/10/2023 14:15, Julien Grall wrote:
> On 10/10/2023 13:13, Julien Grall wrote:
>> 
>> 
>> On 10/10/2023 13:07, Nicola Vetrini wrote:
>>> 
>>>>>>> 
>>>>>>> I agree on everything Julien's wrote and I was about to suggest 
>>>>>>> to use a
>>>>>>> SAF comment to suppress the warning because it is clearer than a 
>>>>>>> int
>>>>>>> cast.
>>>>>>> 
>>>>>>> But then I realized that even if BITS_TO_LONGS was fixed, 
>>>>>>> wouldn't still
>>>>>>> we have a problem because IOMMU_FEAT_count is an enum?
>>>>>>> 
>>>>>>> Is it the case that IOMMU_FEAT_count would have to be cast 
>>>>>>> regardless,
>>>>>>> either to int or unsigned int or whatever to be used in 
>>>>>>> DECLARE_BITMAP?
>>>>>>> 
>>>>>>> 
>>>>>>> So we have 2 problems here: one problem is DECLARE_BITMAP taking 
>>>>>>> int
>>>>>>> instead of unsigned int, and another problem is IOMMU_FEAT_count 
>>>>>>> being
>>>>>>> an enum.
>>>>>>> 
>>>>>>> If I got it right, then I would go with the cast to int (like 
>>>>>>> done in
>>>>>>> this patch) with a decent comment on top of it.
>>>>>> 
>>>>>> I might be missing something here. But why should we use cast 
>>>>>> rather than /*
>>>>>> SAF-X */ just above? I would have expected we wanted to highlight 
>>>>>> the
>>>>>> violation rather than hiding it.
>>>>> 
>>>>> My understanding is that the cast is required when converting an 
>>>>> enum
>>>>> type to an integer type and vice versa. The idea is that we 
>>>>> shouldn't do
>>>>> implicit conversions as they are error prone, only explicit 
>>>>> conversions
>>>>> are allowed between enum and integers.
>>>> 
>>>> We have a lot of places in Xen using implicit conversion from enum 
>>>> to
>>>> integer (for instance in the P2M code for p2m_type_t). Does ECLAIR
>>>> report all of them? If not, then why?
>>>> 
>>> 
>>> Can you give some pointers as to where this enum is used in 
>>> arithmetic operations?
>> 
>> I can't think of any right now.
> 
> Obviously, right after I pressed send, I remember of one in
> __acpi_map_table() (x86 code).
> 
> FIX_ACPI_END is an enum, assigned to an 'int' and then used in 
> arithmetics.
> 
> Cheers,

A couple of remarks:
- that file is part of the exclude-list.json, therefore it's not bound 
to be MISRA compliant
   right now;
- assignment to a different essential type category is dealt with by 
Rule 10.3
   (The value of an expression shall not be assigned to an object with a 
narrower
    essential type or of a different essential type category), so perhaps
   Luca's script does indeed capture it with gcc compilation flags.

Aside from this, if you feel that a deviation comment is a better 
choice, I'm ok with
dealing with it in this way.

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

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

[Nicola Vetrini]

On 10/10/2023 12:53, Julien Grall wrote:
> On 10/10/2023 02:09, Stefano Stabellini wrote:
>> On Mon, 9 Oct 2023, Julien Grall wrote:
>>> On 07/10/2023 02:04, Stefano Stabellini wrote:
>>>> On Fri, 6 Oct 2023, Julien Grall wrote:
>>>>> Hi Nicola,
>>>>> 
>>>>> On 06/10/2023 11:10, Nicola Vetrini wrote:
>>>>>> On 06/10/2023 11:34, Julien Grall wrote:
>>>>>>> Hi Nicola,
>>>>>>> 
>>>>>>> On 06/10/2023 09:26, 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'.
>>>>>>>> 
>>>>>>>> A comment in BITS_TO_LONGS is added to make it clear that
>>>>>>>> values passed are meant to be positive.
>>>>>>> 
>>>>>>> I am confused. If the value is meant to be positive. Then...
>>>>>>> 
>>>>>>>> 
>>>>>>>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>>>>>>>> ---
>>>>>>>>     xen/include/xen/iommu.h | 2 +-
>>>>>>>>     xen/include/xen/types.h | 1 +
>>>>>>>>     2 files changed, 2 insertions(+), 1 deletion(-)
>>>>>>>> 
>>>>>>>> diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
>>>>>>>> index 0e747b0bbc1c..34aa0b9b5b81 100644
>>>>>>>> --- a/xen/include/xen/iommu.h
>>>>>>>> +++ b/xen/include/xen/iommu.h
>>>>>>>> @@ -360,7 +360,7 @@ struct domain_iommu {
>>>>>>>>     #endif
>>>>>>>>           /* Features supported by the IOMMU */
>>>>>>>> -    DECLARE_BITMAP(features, IOMMU_FEAT_count);
>>>>>>>> +    DECLARE_BITMAP(features, (int)IOMMU_FEAT_count);
>>>>>>> 
>>>>>>> ... why do we cast to (int) rather than (unsigned int)? Also, I 
>>>>>>> think
>>>>>>> this cast deserve a comment on top because this is not a very 
>>>>>>> obvious
>>>>>>> one.
>>>>>>> 
>>>>>>>>           /* Does the guest share HAP mapping with the IOMMU? */
>>>>>>>>         bool hap_pt_share;
>>>>>>>> diff --git a/xen/include/xen/types.h b/xen/include/xen/types.h
>>>>>>>> index aea259db1ef2..936e83d333a0 100644
>>>>>>>> --- a/xen/include/xen/types.h
>>>>>>>> +++ b/xen/include/xen/types.h
>>>>>>>> @@ -22,6 +22,7 @@ typedef signed long ssize_t;
>>>>>>>>       typedef __PTRDIFF_TYPE__ ptrdiff_t;
>>>>>>>>     +/* Users of this macro are expected to pass a positive 
>>>>>>>> value */
>>>>>>>>     #define BITS_TO_LONGS(bits) \
>>>>>>>>         (((bits)+BITS_PER_LONG-1)/BITS_PER_LONG)
>>>>>>>>     #define DECLARE_BITMAP(name,bits) \
>>>>>>> 
>>>>>>> Cheers,
>>>>>> 
>>>>>> See [1] for the reason why I did so. I should have mentioned that 
>>>>>> in the
>>>>>> commit notes, sorry.
>>>>>> In short, making BITS_TO_LONGS essentially unsigned would cause a
>>>>>> cascade of
>>>>>> build errors and
>>>>>> possibly other essential type violations.
>>>>> Can you share some of the errors?
>>>>> 
>>>>>> If this is to be fixed that way, the effort required
>>>>>> is far greater. Either way, a comment on top of can be added, 
>>>>>> along the
>>>>>> lines of:
>>>>>> 
>>>>>> Leaving this as an enum would violate MISRA C:2012 Rule 10.1
>>>>> 
>>>>> I read this as you are simply trying to silence your tool. I think 
>>>>> this
>>>>> the
>>>>> wrong approach as you are now making the code confusing for the 
>>>>> reader
>>>>> (you
>>>>> pass a signed int to a function that is supposed to take a positive
>>>>> number).
>>>>> 
>>>>> I appreciate that this will result to more violations at the 
>>>>> beginning.
>>>>> But
>>>>> the whole point of MISRA is to make the code better.
>>>>> 
>>>>> If this is too complex to solve now, then a possible option is to 
>>>>> deviate
>>>>> for
>>>>> the time being.
>>>> 
>>>> I agree on everything Julien's wrote and I was about to suggest to 
>>>> use a
>>>> SAF comment to suppress the warning because it is clearer than a int
>>>> cast.
>>>> 
>>>> But then I realized that even if BITS_TO_LONGS was fixed, wouldn't 
>>>> still
>>>> we have a problem because IOMMU_FEAT_count is an enum?
>>>> 
>>>> Is it the case that IOMMU_FEAT_count would have to be cast 
>>>> regardless,
>>>> either to int or unsigned int or whatever to be used in 
>>>> DECLARE_BITMAP?
>>>> 
>>>> 
>>>> So we have 2 problems here: one problem is DECLARE_BITMAP taking int
>>>> instead of unsigned int, and another problem is IOMMU_FEAT_count 
>>>> being
>>>> an enum.
>>>> 
>>>> If I got it right, then I would go with the cast to int (like done 
>>>> in
>>>> this patch) with a decent comment on top of it.
>>> 
>>> I might be missing something here. But why should we use cast rather 
>>> than /*
>>> SAF-X */ just above? I would have expected we wanted to highlight the
>>> violation rather than hiding it.
>> 
>> My understanding is that the cast is required when converting an enum
>> type to an integer type and vice versa. The idea is that we shouldn't 
>> do
>> implicit conversions as they are error prone, only explicit 
>> conversions
>> are allowed between enum and integers.
> 
> We have a lot of places in Xen using implicit conversion from enum to
> integer (for instance in the P2M code for p2m_type_t). Does ECLAIR
> report all of them? If not, then why?
> 

Re-replying here, since in the other reply I didn't address your concern 
fully:
yes, there are more than a few places where this comes up for Rule 10.1, 
especially
in x86 code. In theory a cast is not the only option to bring the code 
into compliance,
but the specific solution should be checked on a case-by-case basis.

The main aim of the series on R10.1 is to deviate or fix the main 
offenders in terms
of violations with as little effort as possible, to have a more 
manageable analysis
result (in my branch, with some patches yet to be submitted I'm down to 
a few violations
on ARM and ~100 on x86).

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

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

[Nicola Vetrini]

On 06/10/2023 16:47, Julien Grall wrote:
> Hi Nicola,
> 
> On 06/10/2023 11:10, Nicola Vetrini wrote:
>> On 06/10/2023 11:34, Julien Grall wrote:
>>> Hi Nicola,
>>> 
>>> On 06/10/2023 09:26, 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'.
>>>> 
>>>> A comment in BITS_TO_LONGS is added to make it clear that
>>>> values passed are meant to be positive.
>>> 
>>> I am confused. If the value is meant to be positive. Then...
>>> 
>>>> 
>>>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>>>> ---
>>>>   xen/include/xen/iommu.h | 2 +-
>>>>   xen/include/xen/types.h | 1 +
>>>>   2 files changed, 2 insertions(+), 1 deletion(-)
>>>> 
>>>> diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
>>>> index 0e747b0bbc1c..34aa0b9b5b81 100644
>>>> --- a/xen/include/xen/iommu.h
>>>> +++ b/xen/include/xen/iommu.h
>>>> @@ -360,7 +360,7 @@ struct domain_iommu {
>>>>   #endif
>>>>         /* Features supported by the IOMMU */
>>>> -    DECLARE_BITMAP(features, IOMMU_FEAT_count);
>>>> +    DECLARE_BITMAP(features, (int)IOMMU_FEAT_count);
>>> 
>>> ... why do we cast to (int) rather than (unsigned int)? Also, I think
>>> this cast deserve a comment on top because this is not a very obvious
>>> one.
>>> 
>>>>         /* Does the guest share HAP mapping with the IOMMU? */
>>>>       bool hap_pt_share;
>>>> diff --git a/xen/include/xen/types.h b/xen/include/xen/types.h
>>>> index aea259db1ef2..936e83d333a0 100644
>>>> --- a/xen/include/xen/types.h
>>>> +++ b/xen/include/xen/types.h
>>>> @@ -22,6 +22,7 @@ typedef signed long ssize_t;
>>>>     typedef __PTRDIFF_TYPE__ ptrdiff_t;
>>>>   +/* Users of this macro are expected to pass a positive value */
>>>>   #define BITS_TO_LONGS(bits) \
>>>>       (((bits)+BITS_PER_LONG-1)/BITS_PER_LONG)
>>>>   #define DECLARE_BITMAP(name,bits) \
>>> 
>>> Cheers,
>> 
>> See [1] for the reason why I did so. I should have mentioned that in 
>> the commit notes, sorry.
>> In short, making BITS_TO_LONGS essentially unsigned would cause a 
>> cascade of build errors and
>> possibly other essential type violations.
> Can you share some of the errors?
> 

I don't have that build log at hand, but you can reproduce them by 
defining

#define BYTES_PER_LONG (_AC(1,U) << LONG_BYTEORDER)
#define BITS_PER_LONG (BYTES_PER_LONG << 3)

#define BITS_TO_LONGS(bits) \
   (((bits)+BITS_PER_LONG-1U)/BITS_PER_LONG)

and then fiddle a bit with the signedness of the constants in
xen/arch/x86/include/asm/page-bits.h.

They are essentially pointer comparison errors in the instances of 'min' 
and 'max'
that involve those macros or macros derived from those, which make the 
build fail because
they (rightfully) trigger a warning from gcc.

>> If this is to be fixed that way, the effort required
>> is far greater. Either way, a comment on top of can be added, along 
>> the lines of:
>> 
>> Leaving this as an enum would violate MISRA C:2012 Rule 10.1
> 
> I read this as you are simply trying to silence your tool. I think
> this the wrong approach as you are now making the code confusing for
> the reader (you pass a signed int to a function that is supposed to
> take a positive number).
> 
> I appreciate that this will result to more violations at the
> beginning. But the whole point of MISRA is to make the code better.
> 
> If this is too complex to solve now, then a possible option is to
> deviate for the time being.
> 
> Cheers,
> 

You have a point in that is sort of hides a deeper issue that is 
constituted by the
signedness of the macro, but I'd suggest adding to this patch a comment 
explaining what
needs to be done, rather than a deviation comment that hides the 
violations. The reason
is that in the former case, if you put an unsigned argument too big to 
fit in an integer
it will generate a new report, while if a negative argument is used, 
there is a warning
comment on the macro definition (not an ideal countermeasure, though).

>> 
>> [1] 
>> https://lore.kernel.org/xen-devel/6495ba58bda01eae1f4baa46096424eb@bugseng.com/
>> 

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

[XEN PATCH 9/9] xen/compat: address Rule 10.1 for macros CHECK_SIZE

[Nicola Vetrini]

The essential type of the result of an inequality operator is
essentially boolean, therefore it shouldn't be used as an argument of
the multiplication operator, which expects an integer.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
 xen/include/xen/compat.h | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/xen/include/xen/compat.h b/xen/include/xen/compat.h
index f2ce5bb3580a..5ffee6a9fed1 100644
--- a/xen/include/xen/compat.h
+++ b/xen/include/xen/compat.h
@@ -151,12 +151,14 @@ CHECK_NAME_(k, n, T)(k xen_ ## n *x, \
     return x == c; \
 }
 
+#define SIZE_NEQUAL(a, b) \
+    (sizeof(a) != sizeof(b) ? 1 : 0)
 #define CHECK_SIZE(name) \
-    typedef int CHECK_NAME(name, S)[1 - (sizeof(xen_ ## name ## _t) != \
-                                         sizeof(compat_ ## name ## _t)) * 2]
+    typedef int CHECK_NAME(name, S)[1 - (SIZE_NEQUAL(xen_ ## name ## _t, \
+                                                     compat_ ## name ## _t)) * 2]
 #define CHECK_SIZE_(k, n) \
-    typedef int CHECK_NAME_(k, n, S)[1 - (sizeof(k xen_ ## n) != \
-                                          sizeof(k compat_ ## n)) * 2]
+    typedef int CHECK_NAME_(k, n, S)[1 - (SIZE_NEQUAL(k xen_ ## n, \
+                                                      k compat_ ## n)) * 2]
 
 #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 9/9] xen/compat: address Rule 10.1 for macros CHECK_SIZE

[Stefano Stabellini]

On Fri, 6 Oct 2023, Nicola Vetrini wrote:
> The essential type of the result of an inequality operator is
> essentially boolean, therefore it shouldn't be used as an argument of
> the multiplication operator, which expects an integer.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> ---
>  xen/include/xen/compat.h | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/include/xen/compat.h b/xen/include/xen/compat.h
> index f2ce5bb3580a..5ffee6a9fed1 100644
> --- a/xen/include/xen/compat.h
> +++ b/xen/include/xen/compat.h
> @@ -151,12 +151,14 @@ CHECK_NAME_(k, n, T)(k xen_ ## n *x, \
>      return x == c; \
>  }
>  
> +#define SIZE_NEQUAL(a, b) \
> +    (sizeof(a) != sizeof(b) ? 1 : 0)
>  #define CHECK_SIZE(name) \
> -    typedef int CHECK_NAME(name, S)[1 - (sizeof(xen_ ## name ## _t) != \
> -                                         sizeof(compat_ ## name ## _t)) * 2]
> +    typedef int CHECK_NAME(name, S)[1 - (SIZE_NEQUAL(xen_ ## name ## _t, \
> +                                                     compat_ ## name ## _t)) * 2]
>  #define CHECK_SIZE_(k, n) \
> -    typedef int CHECK_NAME_(k, n, S)[1 - (sizeof(k xen_ ## n) != \
> -                                          sizeof(k compat_ ## n)) * 2]
> +    typedef int CHECK_NAME_(k, n, S)[1 - (SIZE_NEQUAL(k xen_ ## n, \
> +                                                      k compat_ ## n)) * 2]

I think this style is easier to read but I'll let the x86 maintainers
decide

    typedef int CHECK_NAME(name, S)[(sizeof(xen_ ## name ## _t) == \
                                     sizeof(compat_ ## name ## _t)) ? 1 : -1]

Also am I reading this correctly that we are using -1 as array index? I
must have made a calculation mistake?

Re: [XEN PATCH 9/9] xen/compat: address Rule 10.1 for macros CHECK_SIZE

[Andrew Cooper]

On 10/10/2023 9:02 am, Stefano Stabellini wrote:
> On Fri, 6 Oct 2023, Nicola Vetrini wrote:
>> The essential type of the result of an inequality operator is
>> essentially boolean, therefore it shouldn't be used as an argument of
>> the multiplication operator, which expects an integer.
>>
>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>> ---
>>  xen/include/xen/compat.h | 10 ++++++----
>>  1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/xen/include/xen/compat.h b/xen/include/xen/compat.h
>> index f2ce5bb3580a..5ffee6a9fed1 100644
>> --- a/xen/include/xen/compat.h
>> +++ b/xen/include/xen/compat.h
>> @@ -151,12 +151,14 @@ CHECK_NAME_(k, n, T)(k xen_ ## n *x, \
>>      return x == c; \
>>  }
>>  
>> +#define SIZE_NEQUAL(a, b) \
>> +    (sizeof(a) != sizeof(b) ? 1 : 0)
>>  #define CHECK_SIZE(name) \
>> -    typedef int CHECK_NAME(name, S)[1 - (sizeof(xen_ ## name ## _t) != \
>> -                                         sizeof(compat_ ## name ## _t)) * 2]
>> +    typedef int CHECK_NAME(name, S)[1 - (SIZE_NEQUAL(xen_ ## name ## _t, \
>> +                                                     compat_ ## name ## _t)) * 2]
>>  #define CHECK_SIZE_(k, n) \
>> -    typedef int CHECK_NAME_(k, n, S)[1 - (sizeof(k xen_ ## n) != \
>> -                                          sizeof(k compat_ ## n)) * 2]
>> +    typedef int CHECK_NAME_(k, n, S)[1 - (SIZE_NEQUAL(k xen_ ## n, \
>> +                                                      k compat_ ## n)) * 2]
> I think this style is easier to read but I'll let the x86 maintainers
> decide
>
>     typedef int CHECK_NAME(name, S)[(sizeof(xen_ ## name ## _t) == \
>                                      sizeof(compat_ ## name ## _t)) ? 1 : -1]
>
> Also am I reading this correctly that we are using -1 as array index? I
> must have made a calculation mistake?

This is a NIH BUILD_BUG_ON().  It should be rewritten as

BUILD_BUG_ON(sizeof(xen ...) != sizeof(compat ...));

which will use _Static_assert() in modern compilers.

~Andrew

Re: [XEN PATCH 9/9] xen/compat: address Rule 10.1 for macros CHECK_SIZE

[Nicola Vetrini]

On 10/10/2023 18:00, Andrew Cooper wrote:
> On 10/10/2023 9:02 am, Stefano Stabellini wrote:
>> On Fri, 6 Oct 2023, Nicola Vetrini wrote:
>>> The essential type of the result of an inequality operator is
>>> essentially boolean, therefore it shouldn't be used as an argument of
>>> the multiplication operator, which expects an integer.
>>> 
>>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>>> ---
>>>  xen/include/xen/compat.h | 10 ++++++----
>>>  1 file changed, 6 insertions(+), 4 deletions(-)
>>> 
>>> diff --git a/xen/include/xen/compat.h b/xen/include/xen/compat.h
>>> index f2ce5bb3580a..5ffee6a9fed1 100644
>>> --- a/xen/include/xen/compat.h
>>> +++ b/xen/include/xen/compat.h
>>> @@ -151,12 +151,14 @@ CHECK_NAME_(k, n, T)(k xen_ ## n *x, \
>>>      return x == c; \
>>>  }
>>> 
>>> +#define SIZE_NEQUAL(a, b) \
>>> +    (sizeof(a) != sizeof(b) ? 1 : 0)
>>>  #define CHECK_SIZE(name) \
>>> -    typedef int CHECK_NAME(name, S)[1 - (sizeof(xen_ ## name ## _t) 
>>> != \
>>> -                                         sizeof(compat_ ## name ## 
>>> _t)) * 2]
>>> +    typedef int CHECK_NAME(name, S)[1 - (SIZE_NEQUAL(xen_ ## name ## 
>>> _t, \
>>> +                                                     compat_ ## name 
>>> ## _t)) * 2]
>>>  #define CHECK_SIZE_(k, n) \
>>> -    typedef int CHECK_NAME_(k, n, S)[1 - (sizeof(k xen_ ## n) != \
>>> -                                          sizeof(k compat_ ## n)) * 
>>> 2]
>>> +    typedef int CHECK_NAME_(k, n, S)[1 - (SIZE_NEQUAL(k xen_ ## n, \
>>> +                                                      k compat_ ## 
>>> n)) * 2]
>> I think this style is easier to read but I'll let the x86 maintainers
>> decide
>> 
>>     typedef int CHECK_NAME(name, S)[(sizeof(xen_ ## name ## _t) == \
>>                                      sizeof(compat_ ## name ## _t)) ? 
>> 1 : -1]
>> 
>> Also am I reading this correctly that we are using -1 as array index? 
>> I
>> must have made a calculation mistake?
> 
> This is a NIH BUILD_BUG_ON().  It should be rewritten as
> 
> BUILD_BUG_ON(sizeof(xen ...) != sizeof(compat ...));
> 
> which will use _Static_assert() in modern compilers.
> 
> ~Andrew

Ok, thanks.

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

Re: [XEN PATCH 9/9] xen/compat: address Rule 10.1 for macros CHECK_SIZE

[Andrew Cooper]

On 11/10/2023 12:06 am, Nicola Vetrini wrote:
> On 10/10/2023 18:00, Andrew Cooper wrote:
>> On 10/10/2023 9:02 am, Stefano Stabellini wrote:
>>> On Fri, 6 Oct 2023, Nicola Vetrini wrote:
>>>> The essential type of the result of an inequality operator is
>>>> essentially boolean, therefore it shouldn't be used as an argument of
>>>> the multiplication operator, which expects an integer.
>>>>
>>>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>>>> ---
>>>>  xen/include/xen/compat.h | 10 ++++++----
>>>>  1 file changed, 6 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/xen/include/xen/compat.h b/xen/include/xen/compat.h
>>>> index f2ce5bb3580a..5ffee6a9fed1 100644
>>>> --- a/xen/include/xen/compat.h
>>>> +++ b/xen/include/xen/compat.h
>>>> @@ -151,12 +151,14 @@ CHECK_NAME_(k, n, T)(k xen_ ## n *x, \
>>>>      return x == c; \
>>>>  }
>>>>
>>>> +#define SIZE_NEQUAL(a, b) \
>>>> +    (sizeof(a) != sizeof(b) ? 1 : 0)
>>>>  #define CHECK_SIZE(name) \
>>>> -    typedef int CHECK_NAME(name, S)[1 - (sizeof(xen_ ## name ##
>>>> _t) != \
>>>> -                                         sizeof(compat_ ## name ##
>>>> _t)) * 2]
>>>> +    typedef int CHECK_NAME(name, S)[1 - (SIZE_NEQUAL(xen_ ## name
>>>> ## _t, \
>>>> +                                                     compat_ ##
>>>> name ## _t)) * 2]
>>>>  #define CHECK_SIZE_(k, n) \
>>>> -    typedef int CHECK_NAME_(k, n, S)[1 - (sizeof(k xen_ ## n) != \
>>>> -                                          sizeof(k compat_ ## n))
>>>> * 2]
>>>> +    typedef int CHECK_NAME_(k, n, S)[1 - (SIZE_NEQUAL(k xen_ ## n, \
>>>> +                                                      k compat_ ##
>>>> n)) * 2]
>>> I think this style is easier to read but I'll let the x86 maintainers
>>> decide
>>>
>>>     typedef int CHECK_NAME(name, S)[(sizeof(xen_ ## name ## _t) == \
>>>                                      sizeof(compat_ ## name ## _t))
>>> ? 1 : -1]
>>>
>>> Also am I reading this correctly that we are using -1 as array index? I
>>> must have made a calculation mistake?
>>
>> This is a NIH BUILD_BUG_ON().  It should be rewritten as
>>
>> BUILD_BUG_ON(sizeof(xen ...) != sizeof(compat ...));
>>
>> which will use _Static_assert() in modern compilers.
>>
>> ~Andrew
>
> Ok, thanks.
>

I'm going to go out on a limb and say that every other pattern that
looks like this probably wants converting to a BUILD_BUG_ON().

This code quite possibly predates the introduction of BUILD_BUG_ON(),
but we want to end up using BUILD_BUG_ON() everywhere because it's the
construct that uses _Static_assert() wherever possible.

~Andrew