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

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

[Nicola Vetrini]

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

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

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

Changes in v2:
- the patch 'x86/cpu-policy' has been dropped, in favour of the patch submitted
  by Andrew Cooper.

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

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

--
2.34.1

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

[Nicola Vetrini]

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

A deviation for ECLAIR is also introduced.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
Changes in v2:
- rename to LOWEST_BIT
---
 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..b8e1155ee49d 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_BIT encapsulates a well-known pattern to obtain the value
+2^ffs(x) for unsigned integers on two's complement architectures
+(all the architectures supported by Xen satisfy this requirement)."
+-config=MC3R1.R10.1,reports+={safe, "any_area(any_loc(any_exp(macro(^LOWEST_BIT$))))"}
+-doc_end
+
 ### Set 3 ###

 #
diff --git a/xen/include/xen/macros.h b/xen/include/xen/macros.h
index d0caae7db298..af47179d1056 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_BIT(x) ((x) & -(x))
+
+#define MASK_EXTR(v, m) (((v) & (m)) / LOWEST_BIT(m))
+#define MASK_INSR(v, m) (((v) * LOWEST_BIT(m)) & (m))

 #define count_args_(dot, a1, a2, a3, a4, a5, a6, a7, a8, x, ...) x
 #define count_args(args...) \
--
2.34.1

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

[Nicola Vetrini]

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

No functional change.

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

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

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

[Nicola Vetrini]

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

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

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

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

[Nicola Vetrini]

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

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

diff --git a/xen/arch/x86/x86_64/mm.c b/xen/arch/x86/x86_64/mm.c
index c3ebb777144a..0eb7b71124f5 100644
--- a/xen/arch/x86/x86_64/mm.c
+++ b/xen/arch/x86/x86_64/mm.c
@@ -351,9 +351,9 @@ static int setup_compat_m2p_table(struct mem_hotadd_info *info)
                 ~((1UL << (L2_PAGETABLE_SHIFT - 2)) - 1) );

 #define MFN(x) (((x) << L2_PAGETABLE_SHIFT) / sizeof(unsigned int))
-#define CNT ((sizeof(*frame_table) & -sizeof(*frame_table)) / \
+#define CNT (LOWEST_BIT(sizeof(*frame_table)) / \
              sizeof(*compat_machine_to_phys_mapping))
-    BUILD_BUG_ON((sizeof(*frame_table) & -sizeof(*frame_table)) % \
+    BUILD_BUG_ON(LOWEST_BIT(sizeof(*frame_table)) % \
                  sizeof(*compat_machine_to_phys_mapping));

     for ( i = smap; i < emap; i += (1UL << (L2_PAGETABLE_SHIFT - 2)) )
@@ -410,10 +410,10 @@ static int setup_m2p_table(struct mem_hotadd_info *info)
     va = RO_MPT_VIRT_START + smap * sizeof(*machine_to_phys_mapping);

 #define MFN(x) (((x) << L2_PAGETABLE_SHIFT) / sizeof(unsigned long))
-#define CNT ((sizeof(*frame_table) & -sizeof(*frame_table)) / \
+#define CNT (LOWEST_BIT(sizeof(*frame_table)) / \
              sizeof(*machine_to_phys_mapping))

-    BUILD_BUG_ON((sizeof(*frame_table) & -sizeof(*frame_table)) % \
+    BUILD_BUG_ON(LOWEST_BIT(sizeof(*frame_table)) % \
                  sizeof(*machine_to_phys_mapping));

     i = smap;
@@ -539,7 +539,7 @@ void __init paging_init(void)
     mpt_size  = (max_page * BYTES_PER_LONG) + (1UL << L2_PAGETABLE_SHIFT) - 1;
     mpt_size &= ~((1UL << L2_PAGETABLE_SHIFT) - 1UL);
 #define MFN(x) (((x) << L2_PAGETABLE_SHIFT) / sizeof(unsigned long))
-#define CNT ((sizeof(*frame_table) & -sizeof(*frame_table)) / \
+#define CNT (LOWEST_BIT(sizeof(*frame_table)) / \
              sizeof(*machine_to_phys_mapping))
     BUILD_BUG_ON((sizeof(*frame_table) & ~sizeof(*frame_table)) % \
                  sizeof(*machine_to_phys_mapping));
@@ -666,7 +666,7 @@ void __init paging_init(void)
         mpt_size = 0;

 #define MFN(x) (((x) << L2_PAGETABLE_SHIFT) / sizeof(unsigned int))
-#define CNT ((sizeof(*frame_table) & -sizeof(*frame_table)) / \
+#define CNT (LOWEST_BIT(sizeof(*frame_table)) / \
              sizeof(*compat_machine_to_phys_mapping))
     BUILD_BUG_ON((sizeof(*frame_table) & ~sizeof(*frame_table)) % \
                  sizeof(*compat_machine_to_phys_mapping));
--
2.34.1

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

[Nicola Vetrini]

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

No functional change.
---
 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][for-next v2 6/8] x86/mce: Move MC_NCLASSES into the enum mctelem_class

[Nicola Vetrini]

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

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
Note that the use of an enum constant as operand to [ ] and != is allowed
by the Rule.
---
 xen/arch/x86/cpu/mcheck/mctelem.c | 2 --
 xen/arch/x86/cpu/mcheck/mctelem.h | 5 +++--
 2 files changed, 3 insertions(+), 4 deletions(-)

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

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

[Nicola Vetrini]

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

This construct is deviated with a deviation comment.

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

diff --git a/docs/misra/safe.json b/docs/misra/safe.json
index 39c5c056c7d4..952324f85cf9 100644
--- a/docs/misra/safe.json
+++ b/docs/misra/safe.json
@@ -20,6 +20,14 @@
         },
         {
             "id": "SAF-2-safe",
+            "analyser": {
+                "eclair": "MC3R1.R10.1"
+            },
+            "name": "MC3R1.R10.1: use of an enumeration constant in an arithmetic operation",
+            "text": "This violation can be fixed with a cast to (int) of the enumeration constant, but a deviation was chosen due to code readability (see also the comment in BITS_TO_LONGS)."
+        },
+        {
+            "id": "SAF-3-safe",
             "analyser": {},
             "name": "Sentinel",
             "text": "Next ID to be used"
diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
index 0e747b0bbc1c..d5c25770915b 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -360,6 +360,7 @@ struct domain_iommu {
 #endif
 
     /* Features supported by the IOMMU */
+    /* SAF-2-safe enum constant in arithmetic operation */
     DECLARE_BITMAP(features, IOMMU_FEAT_count);
 
     /* Does the guest share HAP mapping with the IOMMU? */
diff --git a/xen/include/xen/types.h b/xen/include/xen/types.h
index aea259db1ef2..50171ea1ad28 100644
--- a/xen/include/xen/types.h
+++ b/xen/include/xen/types.h
@@ -22,6 +22,14 @@ typedef signed long ssize_t;
 
 typedef __PTRDIFF_TYPE__ ptrdiff_t;
 
+/*
+ * Users of this macro are expected to pass a positive value.
+ *
+ * Eventually, this should become an unsigned quantity, but this
+ * requires fixing various uses of this macro and BITS_PER_LONG in signed
+ * contexts, such as type-safe 'min' macro uses, which give rise to build errors
+ * when the arguments have differing signedness, due to the build flags used.
+ */
 #define BITS_TO_LONGS(bits) \
     (((bits)+BITS_PER_LONG-1)/BITS_PER_LONG)
 #define DECLARE_BITMAP(name,bits) \
-- 
2.34.1

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

[Nicola Vetrini]

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

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

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

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

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

[Stefano Stabellini]

On Thu, 12 Oct 2023, 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.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> ---
> Changes in v2:
> - rename to LOWEST_BIT
> ---
>  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..b8e1155ee49d 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_BIT encapsulates a well-known pattern to obtain the value
> +2^ffs(x) for unsigned integers on two's complement architectures
> +(all the architectures supported by Xen satisfy this requirement)."
> +-config=MC3R1.R10.1,reports+={safe, "any_area(any_loc(any_exp(macro(^LOWEST_BIT$))))"}
> +-doc_end

Please also add the same deviation and explanation to
docs/misra/deviations.rst. Other than that, this looks fine.


>  ### Set 3 ###
> 
>  #
> diff --git a/xen/include/xen/macros.h b/xen/include/xen/macros.h
> index d0caae7db298..af47179d1056 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_BIT(x) ((x) & -(x))
> +
> +#define MASK_EXTR(v, m) (((v) & (m)) / LOWEST_BIT(m))
> +#define MASK_INSR(v, m) (((v) * LOWEST_BIT(m)) & (m))
> 
>  #define count_args_(dot, a1, a2, a3, a4, a5, a6, a7, a8, x, ...) x
>  #define count_args(args...) \
> --
> 2.34.1
> 

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

[Stefano Stabellini]

On Thu, 12 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_BIT macro.
> 
> No functional change.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>

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

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

[Stefano Stabellini]

On Thu, 12 Oct 2023, Nicola Vetrini wrote:
> The definition of PDX_GROUP_COUNT causes violations of
> MISRA C:2012 Rule 10.1, therefore the problematic part now uses
> the LOWEST_BIT macro, which encapsulates the pattern.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>

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

Re: [XEN PATCH][for-next v2 4/8] x86_64/mm: express macro CNT using LOWEST_BIT

[Stefano Stabellini]

On Thu, 12 Oct 2023, Nicola Vetrini wrote:
> The various definitions of macro CNT (and the related BUILD_BUG_ON)
> can be rewritten using LOWEST_BIT, encapsulating a violation of
> MISRA C:2012 Rule 10.1.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>

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

Re: [XEN PATCH][for-next v2 6/8] x86/mce: Move MC_NCLASSES into the enum mctelem_class

[Stefano Stabellini]

On Thu, 12 Oct 2023, 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>

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

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

[Julien Grall]

Hi Nicola,

On 12/10/2023 16:28, 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.

In the commit message it is clear that the macro will return the lowest 
set bit. But...

> 
> A deviation for ECLAIR is also introduced.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> ---
> Changes in v2:
> - rename to LOWEST_BIT
> ---
>   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..b8e1155ee49d 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_BIT encapsulates a well-known pattern to obtain the value
> +2^ffs(x) for unsigned integers on two's complement architectures
> +(all the architectures supported by Xen satisfy this requirement)."
> +-config=MC3R1.R10.1,reports+={safe, "any_area(any_loc(any_exp(macro(^LOWEST_BIT$))))"}
> +-doc_end
> +
>   ### Set 3 ###
> 
>   #
> diff --git a/xen/include/xen/macros.h b/xen/include/xen/macros.h
> index d0caae7db298..af47179d1056 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_BIT(x) ((x) & -(x))

... this is not reflected in the name of the macro. So it is not obvious 
if it will return the lowest bit set or clear.

Can you at least add a comment on top explaining what it returns? 
Something like:

/* Return the lowest bit set */

Cheers,

-- 
Julien Grall

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

[Nicola Vetrini]

On 13/10/2023 01:31, Stefano Stabellini wrote:
> On Thu, 12 Oct 2023, 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.
>> 
>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>> ---
>> Changes in v2:
>> - rename to LOWEST_BIT
>> ---
>>  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..b8e1155ee49d 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_BIT encapsulates a well-known pattern to 
>> obtain the value
>> +2^ffs(x) for unsigned integers on two's complement architectures
>> +(all the architectures supported by Xen satisfy this requirement)."
>> +-config=MC3R1.R10.1,reports+={safe, 
>> "any_area(any_loc(any_exp(macro(^LOWEST_BIT$))))"}
>> +-doc_end
> 
> Please also add the same deviation and explanation to
> docs/misra/deviations.rst. Other than that, this looks fine.
> 
> 

Ok.

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

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

[Nicola Vetrini]

On 13/10/2023 10:25, Julien Grall wrote:
> Hi Nicola,
> 
> On 12/10/2023 16:28, 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.
> 
> In the commit message it is clear that the macro will return the
> lowest set bit. But...
> 
>> 
>> A deviation for ECLAIR is also introduced.
>> 
>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>> ---
>> Changes in v2:
>> - rename to LOWEST_BIT
>> ---
>>   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..b8e1155ee49d 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_BIT encapsulates a well-known pattern to 
>> obtain the value
>> +2^ffs(x) for unsigned integers on two's complement architectures
>> +(all the architectures supported by Xen satisfy this requirement)."
>> +-config=MC3R1.R10.1,reports+={safe, 
>> "any_area(any_loc(any_exp(macro(^LOWEST_BIT$))))"}
>> +-doc_end
>> +
>>   ### Set 3 ###
>> 
>>   #
>> diff --git a/xen/include/xen/macros.h b/xen/include/xen/macros.h
>> index d0caae7db298..af47179d1056 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_BIT(x) ((x) & -(x))
> 
> ... this is not reflected in the name of the macro. So it is not
> obvious if it will return the lowest bit set or clear.
> 
> Can you at least add a comment on top explaining what it returns?
> Something like:
> 
> /* Return the lowest bit set */
> 
> Cheers,

No problem

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

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

[Jan Beulich]

On 12.10.2023 17:28, Nicola Vetrini wrote:
> --- 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_BIT encapsulates a well-known pattern to obtain the value
> +2^ffs(x) for unsigned integers on two's complement architectures
> +(all the architectures supported by Xen satisfy this requirement)."
> +-config=MC3R1.R10.1,reports+={safe, "any_area(any_loc(any_exp(macro(^LOWEST_BIT$))))"}
> +-doc_end

Why is this added here rather than by ...

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

a SAF-<n>-safe comment here?

> +#define LOWEST_BIT(x) ((x) & -(x))

Personally I consider the name misleading: I'd expect a macro of this
name to return a bit number, not a mask with a single bit set. No
good, reasonably short name comes to mind - ISOLATE_LOW_BIT() is too
long for my taste.

Jan

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

[Jan Beulich]

On 12.10.2023 17:28, Nicola Vetrini wrote:
> The definition of IO_APIC_BASE contains a sum of an essentially enum
> value (FIX_IO_APIC_BASE_0) that is positive with an index that, in all
> instances, is unsigned, therefore the former is cast to unsigned, so that
> the operands are of the same essential type.
> 
> No functional change.
> ---
>  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)))

Let's assume that down the road we want to make __fix_to_virt() an inline
function (which perhaps it really ought to be already): Won't this trade
one violation for another then? I wonder whether the better course of
action wouldn't be to switch the two enums to be "anonymous", even if that
means adjusting __set_fixmap() and __set_fixmap_x().

As an aside, since you touch the entire construct, it would be nice if the
+ was also moved to the end of the earlier line.

Jan

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

[Jan Beulich]

On 12.10.2023 17:28, Nicola Vetrini wrote:
> The definition of IO_APIC_BASE contains a sum of an essentially enum
> value (FIX_IO_APIC_BASE_0) that is positive with an index that, in all
> instances, is unsigned, therefore the former is cast to unsigned, so that
> the operands are of the same essential type.
> 
> No functional change.
> ---
>  xen/arch/x86/include/asm/io_apic.h | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)

Oh, also - there's no S-o-b here.

Jan

Re: [XEN PATCH][for-next v2 6/8] x86/mce: Move MC_NCLASSES into the enum mctelem_class

[Jan Beulich]

On 12.10.2023 17:28, 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.

And using an enumerator as array dimension specifier is okay for Misra?
That would be odd when elsewhere named enums are treated specially.

Jan

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

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

[Jan Beulich]

On 12.10.2023 17:28, Nicola Vetrini wrote:
> --- a/xen/include/xen/types.h
> +++ b/xen/include/xen/types.h
> @@ -22,6 +22,14 @@ typedef signed long ssize_t;
>  
>  typedef __PTRDIFF_TYPE__ ptrdiff_t;
>  
> +/*
> + * Users of this macro are expected to pass a positive value.

Is passing 0 going to cause any issues?

> + * Eventually, this should become an unsigned quantity, but this
> + * requires fixing various uses of this macro and BITS_PER_LONG in signed
> + * contexts, such as type-safe 'min' macro uses, which give rise to build errors
> + * when the arguments have differing signedness, due to the build flags used.
> + */

I'm not convinced of the usefulness of this part of the comment.

Jan

>  #define BITS_TO_LONGS(bits) \
>      (((bits)+BITS_PER_LONG-1)/BITS_PER_LONG)
>  #define DECLARE_BITMAP(name,bits) \

Re: [XEN PATCH][for-next v2 6/8] x86/mce: Move MC_NCLASSES into the enum mctelem_class

[Nicola Vetrini]

On 16/10/2023 17:45, Jan Beulich wrote:
> On 12.10.2023 17:28, 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.
> 
> And using an enumerator as array dimension specifier is okay for Misra?
> That would be odd when elsewhere named enums are treated specially.
> 
> Jan
> 

Yes, the array subscript operator is one of the few places where an enum 
can be used as
an operand (also because negative values wouldn't compile), as opposed 
to mixing them
with ordinary integers.

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

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

[Nicola Vetrini]

On 16/10/2023 17:33, Jan Beulich wrote:
> On 12.10.2023 17:28, Nicola Vetrini wrote:
>> --- 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_BIT encapsulates a well-known pattern to 
>> obtain the value
>> +2^ffs(x) for unsigned integers on two's complement architectures
>> +(all the architectures supported by Xen satisfy this requirement)."
>> +-config=MC3R1.R10.1,reports+={safe, 
>> "any_area(any_loc(any_exp(macro(^LOWEST_BIT$))))"}
>> +-doc_end
> 
> Why is this added here rather than by ...
> 
>> --- 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))
> 
> a SAF-<n>-safe comment here?
> 

One reason is that now that violations only belonging to tool 
configurations
and similar are documented in docs/misra/deviations.rst (committed in 
Stefano's
branch for-4.19 [1]). Also, there were disagreements on the SAF naming 
scheme, and
patches like those would not be accepted at the moment.

>> +#define LOWEST_BIT(x) ((x) & -(x))
> 
> Personally I consider the name misleading: I'd expect a macro of this
> name to return a bit number, not a mask with a single bit set. No
> good, reasonably short name comes to mind - ISOLATE_LOW_BIT() is too
> long for my taste.
> 
> Jan

[1] 
https://gitlab.com/xen-project/people/sstabellini/xen/-/commits/for-4.19

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

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

[Jan Beulich]

On 16.10.2023 18:17, Nicola Vetrini wrote:
> On 16/10/2023 17:33, Jan Beulich wrote:
>> On 12.10.2023 17:28, Nicola Vetrini wrote:
>>> --- 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_BIT encapsulates a well-known pattern to 
>>> obtain the value
>>> +2^ffs(x) for unsigned integers on two's complement architectures
>>> +(all the architectures supported by Xen satisfy this requirement)."
>>> +-config=MC3R1.R10.1,reports+={safe, 
>>> "any_area(any_loc(any_exp(macro(^LOWEST_BIT$))))"}
>>> +-doc_end
>>
>> Why is this added here rather than by ...
>>
>>> --- 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))
>>
>> a SAF-<n>-safe comment here?
>>
> 
> One reason is that now that violations only belonging to tool 
> configurations
> and similar are documented in docs/misra/deviations.rst (committed in 
> Stefano's
> branch for-4.19 [1]).

But tool configuration means every analysis tool needs configuring
separately. That's why the comment tagging scheme was decided to be
preferred, iirc.

> Also, there were disagreements on the SAF naming 
> scheme, and
> patches like those would not be accepted at the moment.

Well, that needs resolving. The naming there shouldn't lead to patches
being accepted that later may need redoing.

Jan

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

[Nicola Vetrini]

On 16/10/2023 17:42, Jan Beulich wrote:
> On 12.10.2023 17:28, Nicola Vetrini wrote:
>> The definition of IO_APIC_BASE contains a sum of an essentially enum
>> value (FIX_IO_APIC_BASE_0) that is positive with an index that, in all
>> instances, is unsigned, therefore the former is cast to unsigned, so 
>> that
>> the operands are of the same essential type.
>> 
>> No functional change.
>> ---
>>  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)))
> 
> Let's assume that down the road we want to make __fix_to_virt() an 
> inline
> function (which perhaps it really ought to be already): Won't this 
> trade
> one violation for another then?

I don't think so: the violation is in the argument to the macro (i.e., 
the fact that
idx is unsigned and FIX_IO_APIC_BASE_0 is a constant from a named enum). 
Do you see a way in
which a MISRA rule is violated if __fix_to_virt becomes a function with 
an unsigned int argument?

> I wonder whether the better course of
> action wouldn't be to switch the two enums to be "anonymous", even if 
> that
> means adjusting __set_fixmap() and __set_fixmap_x().
> 

I don't know. It certainly would help from a violation count 
perspective, though that's more of a code
style decision in my opinion.

> As an aside, since you touch the entire construct, it would be nice if 
> the
> + was also moved to the end of the earlier line.
> 

Sure

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

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

[Nicola Vetrini]

On 16/10/2023 17:42, Jan Beulich wrote:
> On 12.10.2023 17:28, Nicola Vetrini wrote:
>> The definition of IO_APIC_BASE contains a sum of an essentially enum
>> value (FIX_IO_APIC_BASE_0) that is positive with an index that, in all
>> instances, is unsigned, therefore the former is cast to unsigned, so 
>> that
>> the operands are of the same essential type.
>> 
>> No functional change.
>> ---
>>  xen/arch/x86/include/asm/io_apic.h | 7 ++++---
>>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> Oh, also - there's no S-o-b here.
> 
> Jan

Ah, good catch.

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

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

[Stefano Stabellini]

On Mon, 16 Oct 2023, Jan Beulich wrote:
> On 16.10.2023 18:17, Nicola Vetrini wrote:
> > On 16/10/2023 17:33, Jan Beulich wrote:
> >> On 12.10.2023 17:28, Nicola Vetrini wrote:
> >>> --- 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_BIT encapsulates a well-known pattern to 
> >>> obtain the value
> >>> +2^ffs(x) for unsigned integers on two's complement architectures
> >>> +(all the architectures supported by Xen satisfy this requirement)."
> >>> +-config=MC3R1.R10.1,reports+={safe, 
> >>> "any_area(any_loc(any_exp(macro(^LOWEST_BIT$))))"}
> >>> +-doc_end
> >>
> >> Why is this added here rather than by ...
> >>
> >>> --- 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))
> >>
> >> a SAF-<n>-safe comment here?
> >>
> > 
> > One reason is that now that violations only belonging to tool 
> > configurations
> > and similar are documented in docs/misra/deviations.rst (committed in 
> > Stefano's
> > branch for-4.19 [1]).
> 
> But tool configuration means every analysis tool needs configuring
> separately. That's why the comment tagging scheme was decided to be
> preferred, iirc.
> 
> > Also, there were disagreements on the SAF naming 
> > scheme, and
> > patches like those would not be accepted at the moment.
> 
> Well, that needs resolving. The naming there shouldn't lead to patches
> being accepted that later may need redoing.

I agree

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

[Jan Beulich]

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

Hmm, looking back it's indeed not clear why I didn't use BUILD_BUG_ON() right
away. Perhaps just to avoid inline functions when things can be done without.
And/or because originally the macros were intended to be usable in function
bodies, not (just) at file scope. However, ...

> --- a/xen/include/xen/compat.h
> +++ b/xen/include/xen/compat.h
> @@ -151,12 +151,20 @@ CHECK_NAME_(k, n, T)(k xen_ ## n *x, \
>      return x == c; \
>  }
>  
> -#define CHECK_SIZE(name) \
> -    typedef int CHECK_NAME(name, S)[1 - (sizeof(xen_ ## name ## _t) != \
> -                                         sizeof(compat_ ## name ## _t)) * 2]
> +#define CHECK_SIZE(name)                                  \
> +static inline void __maybe_unused CHECK_SIZE_##name(void) \
> +{                                                         \
> +    typedef int CHECK_NAME(name, S)[1];                   \

... what's this and ...

> +    BUILD_BUG_ON(sizeof(xen_ ## name ## _t) !=            \
> +                 sizeof(compat_ ## name ## _t));          \
> +}
>  #define CHECK_SIZE_(k, n) \
> -    typedef int CHECK_NAME_(k, n, S)[1 - (sizeof(k xen_ ## n) != \
> -                                          sizeof(k compat_ ## n)) * 2]
> +static inline void __maybe_unused CHECK_SIZE_##k_##n(void) \
> +{                                                          \
> +    typedef int CHECK_NAME_(k, n, S)[1];                   \

... this needed for? The types aren't used anywhere afaict.

Jan

> +    BUILD_BUG_ON(sizeof(k xen_ ## n) !=                    \
> +                 sizeof(k compat_ ## n));                  \
> +}
>  
>  #define CHECK_FIELD_COMMON(name, t, f) \
>  static inline int __maybe_unused name(xen_ ## t ## _t *x, compat_ ## t ## _t *c) \

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

[Jan Beulich]

On 16.10.2023 18:36, Nicola Vetrini wrote:
> On 16/10/2023 17:42, Jan Beulich wrote:
>> On 12.10.2023 17:28, Nicola Vetrini wrote:
>>> --- 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)))
>>
>> Let's assume that down the road we want to make __fix_to_virt() an 
>> inline
>> function (which perhaps it really ought to be already): Won't this 
>> trade
>> one violation for another then?
> 
> I don't think so: the violation is in the argument to the macro (i.e., 
> the fact that
> idx is unsigned and FIX_IO_APIC_BASE_0 is a constant from a named enum). 
> Do you see a way in
> which a MISRA rule is violated if __fix_to_virt becomes a function with 
> an unsigned int argument?

No. But I assumed such a function would take an enum parameter.

Jan

Re: [XEN PATCH][for-next v2 6/8] x86/mce: Move MC_NCLASSES into the enum mctelem_class

[Jan Beulich]

On 16.10.2023 18:05, Nicola Vetrini wrote:
> On 16/10/2023 17:45, Jan Beulich wrote:
>> On 12.10.2023 17:28, 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.
>>
>> And using an enumerator as array dimension specifier is okay for Misra?
>> That would be odd when elsewhere named enums are treated specially.
> 
> Yes, the array subscript operator is one of the few places where an enum 
> can be used as
> an operand (also because negative values wouldn't compile), as opposed 
> to mixing them
> with ordinary integers.

When saying "odd" I didn't even think of negative values. May I therefore
ask for the reasoning of why this specific case is deemed non-risky? To
me there looks to be a fair risk of creating undersized arrays, leading
to out-of-bounds accesses.

Jan

Re: [XEN PATCH][for-next v2 6/8] x86/mce: Move MC_NCLASSES into the enum mctelem_class

[Nicola Vetrini]

On 17/10/2023 09:02, Jan Beulich wrote:
> On 16.10.2023 18:05, Nicola Vetrini wrote:
>> On 16/10/2023 17:45, Jan Beulich wrote:
>>> On 12.10.2023 17:28, 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.
>>> 
>>> And using an enumerator as array dimension specifier is okay for 
>>> Misra?
>>> That would be odd when elsewhere named enums are treated specially.
>> 
>> Yes, the array subscript operator is one of the few places where an 
>> enum
>> can be used as
>> an operand (also because negative values wouldn't compile), as opposed
>> to mixing them
>> with ordinary integers.
> 
> When saying "odd" I didn't even think of negative values. May I 
> therefore
> ask for the reasoning of why this specific case is deemed non-risky? To
> me there looks to be a fair risk of creating undersized arrays, leading
> to out-of-bounds accesses.
> 
> Jan

Two reasons: MC_NCLASSES is the last value of the enum, and a pattern 
I've spot in various
other places in Xen, so I assumed it was a fairly common pattern for the 
community.
The other reason is that since the value of an enum constant can be 
derived statically, there
is little risk of it being the wrong value, because no arithmetic is 
done on it in its use
as an array's size (and besides, the enum changed the last time ~15 
years ago, so I think
it's unlikely to change much in the near future).

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

Re: [XEN PATCH][for-next v2 6/8] x86/mce: Move MC_NCLASSES into the enum mctelem_class

[Jan Beulich]

On 17.10.2023 10:12, Nicola Vetrini wrote:
> On 17/10/2023 09:02, Jan Beulich wrote:
>> On 16.10.2023 18:05, Nicola Vetrini wrote:
>>> On 16/10/2023 17:45, Jan Beulich wrote:
>>>> On 12.10.2023 17:28, 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.
>>>>
>>>> And using an enumerator as array dimension specifier is okay for 
>>>> Misra?
>>>> That would be odd when elsewhere named enums are treated specially.
>>>
>>> Yes, the array subscript operator is one of the few places where an 
>>> enum
>>> can be used as
>>> an operand (also because negative values wouldn't compile), as opposed
>>> to mixing them
>>> with ordinary integers.
>>
>> When saying "odd" I didn't even think of negative values. May I 
>> therefore
>> ask for the reasoning of why this specific case is deemed non-risky? To
>> me there looks to be a fair risk of creating undersized arrays, leading
>> to out-of-bounds accesses.
> 
> Two reasons: MC_NCLASSES is the last value of the enum, and a pattern 
> I've spot in various
> other places in Xen, so I assumed it was a fairly common pattern for the 
> community.
> The other reason is that since the value of an enum constant can be 
> derived statically, there
> is little risk of it being the wrong value, because no arithmetic is 
> done on it in its use
> as an array's size (and besides, the enum changed the last time ~15 
> years ago, so I think
> it's unlikely to change much in the near future).

You focus on the specific instance, yet my question was on the general
principle.

Jan

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

[Nicola Vetrini]

On 16/10/2023 17:49, Jan Beulich wrote:
> On 12.10.2023 17:28, Nicola Vetrini wrote:
>> --- a/xen/include/xen/types.h
>> +++ b/xen/include/xen/types.h
>> @@ -22,6 +22,14 @@ typedef signed long ssize_t;
>> 
>>  typedef __PTRDIFF_TYPE__ ptrdiff_t;
>> 
>> +/*
>> + * Users of this macro are expected to pass a positive value.
> 
> Is passing 0 going to cause any issues?
> 

I don't think so, even if that wouldn't make much sense. Given that the 
usage of the
zero lenght array extension is documented, that shouldn't be a concern 
either.

>> + * Eventually, this should become an unsigned quantity, but this
>> + * requires fixing various uses of this macro and BITS_PER_LONG in 
>> signed
>> + * contexts, such as type-safe 'min' macro uses, which give rise to 
>> build errors
>> + * when the arguments have differing signedness, due to the build 
>> flags used.
>> + */
> 
> I'm not convinced of the usefulness of this part of the comment.
> 
> Jan
> 

Isn't it useful to record why it was left as-is, and what should be done 
about it?
If it's not, this can be dropped on commit, in my opinion.

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

Re: [XEN PATCH][for-next v2 6/8] x86/mce: Move MC_NCLASSES into the enum mctelem_class

[Nicola Vetrini]

On 17/10/2023 10:26, Jan Beulich wrote:
> On 17.10.2023 10:12, Nicola Vetrini wrote:
>> On 17/10/2023 09:02, Jan Beulich wrote:
>>> On 16.10.2023 18:05, Nicola Vetrini wrote:
>>>> On 16/10/2023 17:45, Jan Beulich wrote:
>>>>> On 12.10.2023 17:28, 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.
>>>>> 
>>>>> And using an enumerator as array dimension specifier is okay for
>>>>> Misra?
>>>>> That would be odd when elsewhere named enums are treated specially.
>>>> 
>>>> Yes, the array subscript operator is one of the few places where an
>>>> enum
>>>> can be used as
>>>> an operand (also because negative values wouldn't compile), as 
>>>> opposed
>>>> to mixing them
>>>> with ordinary integers.
>>> 
>>> When saying "odd" I didn't even think of negative values. May I
>>> therefore
>>> ask for the reasoning of why this specific case is deemed non-risky? 
>>> To
>>> me there looks to be a fair risk of creating undersized arrays, 
>>> leading
>>> to out-of-bounds accesses.
>> 
>> Two reasons: MC_NCLASSES is the last value of the enum, and a pattern
>> I've spot in various
>> other places in Xen, so I assumed it was a fairly common pattern for 
>> the
>> community.
>> The other reason is that since the value of an enum constant can be
>> derived statically, there
>> is little risk of it being the wrong value, because no arithmetic is
>> done on it in its use
>> as an array's size (and besides, the enum changed the last time ~15
>> years ago, so I think
>> it's unlikely to change much in the near future).
> 
> You focus on the specific instance, yet my question was on the general
> principle.
> 
> Jan

A couple of reasons why this is allowed:
- associating values to set of symbols is typical and makes sense in 
some contexts
- out-of-bounds operations on arrays are dealt with by a host of other 
guidelines
   (Series 18, mainly)
- this rule is about which kinds of operands makes sense to use with 
certain operators.
   It was deemed unlikely by MISRA that risky behaviour may arise by 
using symbolic indices
   as subscripts, given the rest of the other guidelines and the 
unspecified and undefined
   associated with Rule 10.1. It's not impossible that problems will 
arise, but far less
   likely than using enums with no restrictions at all (such as those 
caused by an enum of
   and implementation-defined type used in an arithmetic operation, that 
could give
   unexpected results).

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

Re: [XEN PATCH][for-next v2 6/8] x86/mce: Move MC_NCLASSES into the enum mctelem_class

[Jan Beulich]

On 17.10.2023 11:43, Nicola Vetrini wrote:
> On 17/10/2023 10:26, Jan Beulich wrote:
>> On 17.10.2023 10:12, Nicola Vetrini wrote:
>>> On 17/10/2023 09:02, Jan Beulich wrote:
>>>> On 16.10.2023 18:05, Nicola Vetrini wrote:
>>>>> On 16/10/2023 17:45, Jan Beulich wrote:
>>>>>> On 12.10.2023 17:28, 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.
>>>>>>
>>>>>> And using an enumerator as array dimension specifier is okay for
>>>>>> Misra?
>>>>>> That would be odd when elsewhere named enums are treated specially.
>>>>>
>>>>> Yes, the array subscript operator is one of the few places where an
>>>>> enum
>>>>> can be used as
>>>>> an operand (also because negative values wouldn't compile), as 
>>>>> opposed
>>>>> to mixing them
>>>>> with ordinary integers.
>>>>
>>>> When saying "odd" I didn't even think of negative values. May I
>>>> therefore
>>>> ask for the reasoning of why this specific case is deemed non-risky? 
>>>> To
>>>> me there looks to be a fair risk of creating undersized arrays, 
>>>> leading
>>>> to out-of-bounds accesses.
>>>
>>> Two reasons: MC_NCLASSES is the last value of the enum, and a pattern
>>> I've spot in various
>>> other places in Xen, so I assumed it was a fairly common pattern for 
>>> the
>>> community.
>>> The other reason is that since the value of an enum constant can be
>>> derived statically, there
>>> is little risk of it being the wrong value, because no arithmetic is
>>> done on it in its use
>>> as an array's size (and besides, the enum changed the last time ~15
>>> years ago, so I think
>>> it's unlikely to change much in the near future).
>>
>> You focus on the specific instance, yet my question was on the general
>> principle.
> 
> A couple of reasons why this is allowed:
> - associating values to set of symbols is typical and makes sense in 
> some contexts
> - out-of-bounds operations on arrays are dealt with by a host of other 
> guidelines
>    (Series 18, mainly)
> - this rule is about which kinds of operands makes sense to use with 
> certain operators.
>    It was deemed unlikely by MISRA that risky behaviour may arise by 
> using symbolic indices
>    as subscripts, given the rest of the other guidelines and the 
> unspecified and undefined
>    associated with Rule 10.1. It's not impossible that problems will 
> arise, but far less
>    likely than using enums with no restrictions at all (such as those 
> caused by an enum of
>    and implementation-defined type used in an arithmetic operation, that 
> could give
>    unexpected results).

Now you appear to focus on uses of arrays, not their definition. Yet even
there I wonder: array[idx] is nothing else than *(array + idx). Adding
integer types and enums is disallowed. Nothing is said about arithmetic
on pointers throughout the description of the type model and its rules.
So despite the restriction on integer types, adding enums to pointers is
permitted?

Jan

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

[Nicola Vetrini]

On 16/10/2023 18:30, Jan Beulich wrote:
> On 16.10.2023 18:17, Nicola Vetrini wrote:
>> On 16/10/2023 17:33, Jan Beulich wrote:
>>> On 12.10.2023 17:28, Nicola Vetrini wrote:
>>>> --- 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_BIT encapsulates a well-known pattern 
>>>> to
>>>> obtain the value
>>>> +2^ffs(x) for unsigned integers on two's complement architectures
>>>> +(all the architectures supported by Xen satisfy this requirement)."
>>>> +-config=MC3R1.R10.1,reports+={safe,
>>>> "any_area(any_loc(any_exp(macro(^LOWEST_BIT$))))"}
>>>> +-doc_end
>>> 
>>> Why is this added here rather than by ...
>>> 
>>>> --- 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))
>>> 
>>> a SAF-<n>-safe comment here?
>>> 
>> 
>> One reason is that now that violations only belonging to tool
>> configurations
>> and similar are documented in docs/misra/deviations.rst (committed in
>> Stefano's
>> branch for-4.19 [1]).
> 
> But tool configuration means every analysis tool needs configuring
> separately. That's why the comment tagging scheme was decided to be
> preferred, iirc.
> 
>> Also, there were disagreements on the SAF naming
>> scheme, and
>> patches like those would not be accepted at the moment.
> 
> Well, that needs resolving. The naming there shouldn't lead to patches
> being accepted that later may need redoing.
> 
> Jan

While this is true, in this case I'm not willing to deviate with a SAF, 
given that
some ECLAIR-specific configuration would be needed anyways, given that 
I'm deviating a macro definition, rather than the line where it's 
actually used (and maybe other tools would need
that as well).

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

Re: [XEN PATCH][for-next v2 6/8] x86/mce: Move MC_NCLASSES into the enum mctelem_class

[Nicola Vetrini]

On 17/10/2023 11:54, Jan Beulich wrote:
> On 17.10.2023 11:43, Nicola Vetrini wrote:
>> On 17/10/2023 10:26, Jan Beulich wrote:
>>> On 17.10.2023 10:12, Nicola Vetrini wrote:
>>>> On 17/10/2023 09:02, Jan Beulich wrote:
>>>>> On 16.10.2023 18:05, Nicola Vetrini wrote:
>>>>>> On 16/10/2023 17:45, Jan Beulich wrote:
>>>>>>> On 12.10.2023 17:28, 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.
>>>>>>> 
>>>>>>> And using an enumerator as array dimension specifier is okay for
>>>>>>> Misra?
>>>>>>> That would be odd when elsewhere named enums are treated 
>>>>>>> specially.
>>>>>> 
>>>>>> Yes, the array subscript operator is one of the few places where 
>>>>>> an
>>>>>> enum
>>>>>> can be used as
>>>>>> an operand (also because negative values wouldn't compile), as
>>>>>> opposed
>>>>>> to mixing them
>>>>>> with ordinary integers.
>>>>> 
>>>>> When saying "odd" I didn't even think of negative values. May I
>>>>> therefore
>>>>> ask for the reasoning of why this specific case is deemed 
>>>>> non-risky?
>>>>> To
>>>>> me there looks to be a fair risk of creating undersized arrays,
>>>>> leading
>>>>> to out-of-bounds accesses.
>>>> 
>>>> Two reasons: MC_NCLASSES is the last value of the enum, and a 
>>>> pattern
>>>> I've spot in various
>>>> other places in Xen, so I assumed it was a fairly common pattern for
>>>> the
>>>> community.
>>>> The other reason is that since the value of an enum constant can be
>>>> derived statically, there
>>>> is little risk of it being the wrong value, because no arithmetic is
>>>> done on it in its use
>>>> as an array's size (and besides, the enum changed the last time ~15
>>>> years ago, so I think
>>>> it's unlikely to change much in the near future).
>>> 
>>> You focus on the specific instance, yet my question was on the 
>>> general
>>> principle.
>> 
>> A couple of reasons why this is allowed:
>> - associating values to set of symbols is typical and makes sense in
>> some contexts
>> - out-of-bounds operations on arrays are dealt with by a host of other
>> guidelines
>>    (Series 18, mainly)
>> - this rule is about which kinds of operands makes sense to use with
>> certain operators.
>>    It was deemed unlikely by MISRA that risky behaviour may arise by
>> using symbolic indices
>>    as subscripts, given the rest of the other guidelines and the
>> unspecified and undefined
>>    associated with Rule 10.1. It's not impossible that problems will
>> arise, but far less
>>    likely than using enums with no restrictions at all (such as those
>> caused by an enum of
>>    and implementation-defined type used in an arithmetic operation, 
>> that
>> could give
>>    unexpected results).
> 
> Now you appear to focus on uses of arrays, not their definition. Yet 
> even
> there I wonder: array[idx] is nothing else than *(array + idx). Adding
> integer types and enums is disallowed. Nothing is said about arithmetic
> on pointers throughout the description of the type model and its rules.
> So despite the restriction on integer types, adding enums to pointers 
> is
> permitted?
> 
> Jan

It's not disallowed just to add enums to integers, but the operation of 
addition on
(named) enums. The MISRA C coding standard deemed arr[IDX] an 
appropriate (not to be
interpreted as safe, though what is unsafe is inappropriate as a 
consequence of the
essential type model) use of enums.

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

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

[Nicola Vetrini]

On 17/10/2023 08:09, Jan Beulich wrote:
> On 12.10.2023 17:28, Nicola Vetrini wrote:
>> BUILD_BUG_ON is the preferred way to induce a build error
>> upon statically determined incorrect conditions.
>> 
>> This also fixes a MISRA C:2012 Rule 10.1 violation in the
>> previous formulation.
>> 
>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> 
> Hmm, looking back it's indeed not clear why I didn't use BUILD_BUG_ON() 
> right
> away. Perhaps just to avoid inline functions when things can be done 
> without.
> And/or because originally the macros were intended to be usable in 
> function
> bodies, not (just) at file scope. However, ...
> 
>> --- a/xen/include/xen/compat.h
>> +++ b/xen/include/xen/compat.h
>> @@ -151,12 +151,20 @@ CHECK_NAME_(k, n, T)(k xen_ ## n *x, \
>>      return x == c; \
>>  }
>> 
>> -#define CHECK_SIZE(name) \
>> -    typedef int CHECK_NAME(name, S)[1 - (sizeof(xen_ ## name ## _t) 
>> != \
>> -                                         sizeof(compat_ ## name ## 
>> _t)) * 2]
>> +#define CHECK_SIZE(name)                                  \
>> +static inline void __maybe_unused CHECK_SIZE_##name(void) \
>> +{                                                         \
>> +    typedef int CHECK_NAME(name, S)[1];                   \
> 
> ... what's this and ...
> 
>> +    BUILD_BUG_ON(sizeof(xen_ ## name ## _t) !=            \
>> +                 sizeof(compat_ ## name ## _t));          \
>> +}
>>  #define CHECK_SIZE_(k, n) \
>> -    typedef int CHECK_NAME_(k, n, S)[1 - (sizeof(k xen_ ## n) != \
>> -                                          sizeof(k compat_ ## n)) * 
>> 2]
>> +static inline void __maybe_unused CHECK_SIZE_##k_##n(void) \
>> +{                                                          \
>> +    typedef int CHECK_NAME_(k, n, S)[1];                   \
> 
> ... this needed for? The types aren't used anywhere afaict.
> 
> Jan
> 
>> +    BUILD_BUG_ON(sizeof(k xen_ ## n) !=                    \
>> +                 sizeof(k compat_ ## n));                  \
>> +}
>> 
>>  #define CHECK_FIELD_COMMON(name, t, f) \
>>  static inline int __maybe_unused name(xen_ ## t ## _t *x, compat_ ## 
>> t ## _t *c) \

You're probably right. I was wondering the same thing when replacing the 
code with
BUILD_BUG_ON.

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

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

[Stefano Stabellini]

+Luca

> > > > > --- 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))
> > > > 
> > > > a SAF-<n>-safe comment here?
> > > > 
> > > 
> > > One reason is that now that violations only belonging to tool
> > > configurations
> > > and similar are documented in docs/misra/deviations.rst (committed in
> > > Stefano's
> > > branch for-4.19 [1]).
> > 
> > But tool configuration means every analysis tool needs configuring
> > separately. That's why the comment tagging scheme was decided to be
> > preferred, iirc.
> > 
> > > Also, there were disagreements on the SAF naming
> > > scheme, and
> > > patches like those would not be accepted at the moment.
> > 
> > Well, that needs resolving. The naming there shouldn't lead to patches
> > being accepted that later may need redoing.
> > 
> > Jan
> 
> While this is true, in this case I'm not willing to deviate with a SAF, given
> that
> some ECLAIR-specific configuration would be needed anyways, given that I'm
> deviating a macro definition, rather than the line where it's actually used
> (and maybe other tools would need
> that as well).

Did I get it right that the problem with using SAF in this case is that
it wouldn't be sufficient to add a SAF comment on top of the MACRO
definition, but we would need a SAF comment on top of every MACRO
invocation?

If so, then not just for this MACRO but in general basically we have to
use deviations.rst.

Luca, do you know what would be the behavior for cppcheck and/or
Coverity? I imagine it will be the same and they would also need a
deviation at every MACRO invocation, not just the definition?

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

[Jan Beulich]

On 19.10.2023 21:58, Stefano Stabellini wrote:
>>>>>> --- 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))
>>>>>
>>>>> a SAF-<n>-safe comment here?
>>>>>
>>>>
>>>> One reason is that now that violations only belonging to tool
>>>> configurations
>>>> and similar are documented in docs/misra/deviations.rst (committed in
>>>> Stefano's
>>>> branch for-4.19 [1]).
>>>
>>> But tool configuration means every analysis tool needs configuring
>>> separately. That's why the comment tagging scheme was decided to be
>>> preferred, iirc.
>>>
>>>> Also, there were disagreements on the SAF naming
>>>> scheme, and
>>>> patches like those would not be accepted at the moment.
>>>
>>> Well, that needs resolving. The naming there shouldn't lead to patches
>>> being accepted that later may need redoing.
>>>
>>> Jan
>>
>> While this is true, in this case I'm not willing to deviate with a SAF, given
>> that
>> some ECLAIR-specific configuration would be needed anyways, given that I'm
>> deviating a macro definition, rather than the line where it's actually used
>> (and maybe other tools would need
>> that as well).
> 
> Did I get it right that the problem with using SAF in this case is that
> it wouldn't be sufficient to add a SAF comment on top of the MACRO
> definition, but we would need a SAF comment on top of every MACRO
> invocation?
> 
> If so, then not just for this MACRO but in general basically we have to
> use deviations.rst.

That would be pretty sad.

Jan

> Luca, do you know what would be the behavior for cppcheck and/or
> Coverity? I imagine it will be the same and they would also need a
> deviation at every MACRO invocation, not just the definition?

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

[Nicola Vetrini]

On 20/10/2023 08:00, Jan Beulich wrote:
> On 19.10.2023 21:58, Stefano Stabellini wrote:
>>>>>>> --- 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))
>>>>>> 
>>>>>> a SAF-<n>-safe comment here?
>>>>>> 
>>>>> 
>>>>> One reason is that now that violations only belonging to tool
>>>>> configurations
>>>>> and similar are documented in docs/misra/deviations.rst (committed 
>>>>> in
>>>>> Stefano's
>>>>> branch for-4.19 [1]).
>>>> 
>>>> But tool configuration means every analysis tool needs configuring
>>>> separately. That's why the comment tagging scheme was decided to be
>>>> preferred, iirc.
>>>> 
>>>>> Also, there were disagreements on the SAF naming
>>>>> scheme, and
>>>>> patches like those would not be accepted at the moment.
>>>> 
>>>> Well, that needs resolving. The naming there shouldn't lead to 
>>>> patches
>>>> being accepted that later may need redoing.
>>>> 
>>>> Jan
>>> 
>>> While this is true, in this case I'm not willing to deviate with a 
>>> SAF, given
>>> that
>>> some ECLAIR-specific configuration would be needed anyways, given 
>>> that I'm
>>> deviating a macro definition, rather than the line where it's 
>>> actually used
>>> (and maybe other tools would need
>>> that as well).
>> 
>> Did I get it right that the problem with using SAF in this case is 
>> that
>> it wouldn't be sufficient to add a SAF comment on top of the MACRO
>> definition, but we would need a SAF comment on top of every MACRO
>> invocation?
>> 
>> If so, then not just for this MACRO but in general basically we have 
>> to
>> use deviations.rst.
> 
> That would be pretty sad.
> 
> Jan
> 

Local deviation comments are for local deviations; deviating patterns is 
a tool configuration.

>> Luca, do you know what would be the behavior for cppcheck and/or
>> Coverity? I imagine it will be the same and they would also need a
>> deviation at every MACRO invocation, not just the definition?

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

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

[Luca Fancellu]

> On 19 Oct 2023, at 20:58, Stefano Stabellini <sstabellini@kernel.org> wrote:
> 
> +Luca
> 
>>>>>> --- 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))
>>>>> 
>>>>> a SAF-<n>-safe comment here?
>>>>> 
>>>> 
>>>> One reason is that now that violations only belonging to tool
>>>> configurations
>>>> and similar are documented in docs/misra/deviations.rst (committed in
>>>> Stefano's
>>>> branch for-4.19 [1]).
>>> 
>>> But tool configuration means every analysis tool needs configuring
>>> separately. That's why the comment tagging scheme was decided to be
>>> preferred, iirc.
>>> 
>>>> Also, there were disagreements on the SAF naming
>>>> scheme, and
>>>> patches like those would not be accepted at the moment.
>>> 
>>> Well, that needs resolving. The naming there shouldn't lead to patches
>>> being accepted that later may need redoing.
>>> 
>>> Jan
>> 
>> While this is true, in this case I'm not willing to deviate with a SAF, given
>> that
>> some ECLAIR-specific configuration would be needed anyways, given that I'm
>> deviating a macro definition, rather than the line where it's actually used
>> (and maybe other tools would need
>> that as well).
> 
> Did I get it right that the problem with using SAF in this case is that
> it wouldn't be sufficient to add a SAF comment on top of the MACRO
> definition, but we would need a SAF comment on top of every MACRO
> invocation?
> 
> If so, then not just for this MACRO but in general basically we have to
> use deviations.rst.
> 
> Luca, do you know what would be the behavior for cppcheck and/or
> Coverity? I imagine it will be the same and they would also need a
> deviation at every MACRO invocation, not just the definition?

Seems that cppcheck reports at the macro definition, instead Coverity reports
at the macro invocation.

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

[Jan Beulich]

On 20.10.2023 12:40, Nicola Vetrini wrote:
> On 20/10/2023 08:00, Jan Beulich wrote:
>> On 19.10.2023 21:58, Stefano Stabellini wrote:
>>>>>>>> --- 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))
>>>>>>>
>>>>>>> a SAF-<n>-safe comment here?
>>>>>>>
>>>>>>
>>>>>> One reason is that now that violations only belonging to tool
>>>>>> configurations
>>>>>> and similar are documented in docs/misra/deviations.rst (committed 
>>>>>> in
>>>>>> Stefano's
>>>>>> branch for-4.19 [1]).
>>>>>
>>>>> But tool configuration means every analysis tool needs configuring
>>>>> separately. That's why the comment tagging scheme was decided to be
>>>>> preferred, iirc.
>>>>>
>>>>>> Also, there were disagreements on the SAF naming
>>>>>> scheme, and
>>>>>> patches like those would not be accepted at the moment.
>>>>>
>>>>> Well, that needs resolving. The naming there shouldn't lead to 
>>>>> patches
>>>>> being accepted that later may need redoing.
>>>>>
>>>>> Jan
>>>>
>>>> While this is true, in this case I'm not willing to deviate with a 
>>>> SAF, given
>>>> that
>>>> some ECLAIR-specific configuration would be needed anyways, given 
>>>> that I'm
>>>> deviating a macro definition, rather than the line where it's 
>>>> actually used
>>>> (and maybe other tools would need
>>>> that as well).
>>>
>>> Did I get it right that the problem with using SAF in this case is 
>>> that
>>> it wouldn't be sufficient to add a SAF comment on top of the MACRO
>>> definition, but we would need a SAF comment on top of every MACRO
>>> invocation?
>>>
>>> If so, then not just for this MACRO but in general basically we have 
>>> to
>>> use deviations.rst.
>>
>> That would be pretty sad.
> 
> Local deviation comments are for local deviations; deviating patterns is 
> a tool configuration.

That's orthogonal. A deviating comment on a macro definition, when it is
about an aspect that's meaningful only after the macro is expanded (i.e.
not violating some rule concerning macro definitions only), would be
quite helpful to limit the number of such comments that need sprinkling
across the code base.

Jan

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

[Stefano Stabellini]

On Fri, 20 Oct 2023, Luca Fancellu wrote:
> > On 19 Oct 2023, at 20:58, Stefano Stabellini <sstabellini@kernel.org> wrote:
> > +Luca
> > 
> >>>>>> --- 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))
> >>>>> 
> >>>>> a SAF-<n>-safe comment here?
> >>>>> 
> >>>> 
> >>>> One reason is that now that violations only belonging to tool
> >>>> configurations
> >>>> and similar are documented in docs/misra/deviations.rst (committed in
> >>>> Stefano's
> >>>> branch for-4.19 [1]).
> >>> 
> >>> But tool configuration means every analysis tool needs configuring
> >>> separately. That's why the comment tagging scheme was decided to be
> >>> preferred, iirc.
> >>> 
> >>>> Also, there were disagreements on the SAF naming
> >>>> scheme, and
> >>>> patches like those would not be accepted at the moment.
> >>> 
> >>> Well, that needs resolving. The naming there shouldn't lead to patches
> >>> being accepted that later may need redoing.
> >>> 
> >>> Jan
> >> 
> >> While this is true, in this case I'm not willing to deviate with a SAF, given
> >> that
> >> some ECLAIR-specific configuration would be needed anyways, given that I'm
> >> deviating a macro definition, rather than the line where it's actually used
> >> (and maybe other tools would need
> >> that as well).
> > 
> > Did I get it right that the problem with using SAF in this case is that
> > it wouldn't be sufficient to add a SAF comment on top of the MACRO
> > definition, but we would need a SAF comment on top of every MACRO
> > invocation?
> > 
> > If so, then not just for this MACRO but in general basically we have to
> > use deviations.rst.
> > 
> > Luca, do you know what would be the behavior for cppcheck and/or
> > Coverity? I imagine it will be the same and they would also need a
> > deviation at every MACRO invocation, not just the definition?
> 
> Seems that cppcheck reports at the macro definition, instead Coverity reports
> at the macro invocation.

Does that mean that a /* SAF-xx-safe */ comment would work at the MACRO
definition for cppcheck but not for Coverity?

If so, then I wonder if cppcheck's behavior is what we would like to
have, but from a code compliance point of view it is the least reliable,
so that's the reason why both Coverity and ECLAIR don't implement it. In
terms of correctness of the implementation we know cppcheck is lagging a
bit behind...

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

[Luca Fancellu]

> On 20 Oct 2023, at 19:07, Stefano Stabellini <sstabellini@kernel.org> wrote:
> 
> On Fri, 20 Oct 2023, Luca Fancellu wrote:
>>> On 19 Oct 2023, at 20:58, Stefano Stabellini <sstabellini@kernel.org> wrote:
>>> +Luca
>>> 
>>>>>>>> --- 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))
>>>>>>> 
>>>>>>> a SAF-<n>-safe comment here?
>>>>>>> 
>>>>>> 
>>>>>> One reason is that now that violations only belonging to tool
>>>>>> configurations
>>>>>> and similar are documented in docs/misra/deviations.rst (committed in
>>>>>> Stefano's
>>>>>> branch for-4.19 [1]).
>>>>> 
>>>>> But tool configuration means every analysis tool needs configuring
>>>>> separately. That's why the comment tagging scheme was decided to be
>>>>> preferred, iirc.
>>>>> 
>>>>>> Also, there were disagreements on the SAF naming
>>>>>> scheme, and
>>>>>> patches like those would not be accepted at the moment.
>>>>> 
>>>>> Well, that needs resolving. The naming there shouldn't lead to patches
>>>>> being accepted that later may need redoing.
>>>>> 
>>>>> Jan
>>>> 
>>>> While this is true, in this case I'm not willing to deviate with a SAF, given
>>>> that
>>>> some ECLAIR-specific configuration would be needed anyways, given that I'm
>>>> deviating a macro definition, rather than the line where it's actually used
>>>> (and maybe other tools would need
>>>> that as well).
>>> 
>>> Did I get it right that the problem with using SAF in this case is that
>>> it wouldn't be sufficient to add a SAF comment on top of the MACRO
>>> definition, but we would need a SAF comment on top of every MACRO
>>> invocation?
>>> 
>>> If so, then not just for this MACRO but in general basically we have to
>>> use deviations.rst.
>>> 
>>> Luca, do you know what would be the behavior for cppcheck and/or
>>> Coverity? I imagine it will be the same and they would also need a
>>> deviation at every MACRO invocation, not just the definition?
>> 
>> Seems that cppcheck reports at the macro definition, instead Coverity reports
>> at the macro invocation.
> 
> Does that mean that a /* SAF-xx-safe */ comment would work at the MACRO
> definition for cppcheck but not for Coverity?
> 
> If so, then I wonder if cppcheck's behavior is what we would like to
> have, but from a code compliance point of view it is the least reliable,
> so that's the reason why both Coverity and ECLAIR don't implement it. In
> terms of correctness of the implementation we know cppcheck is lagging a
> bit behind...

Yes, I’m starting to think that if we want to deviate a large number of macro usage,
We should come up with something like declaring the macro somewhere and adding
the in-code comment automatically by the script before the analysis...
But we need to see how feasible it is.