[XEN PATCH 00/10] address some violations of MISRA C Rule 20.7

[XEN PATCH 00/10] address some violations of MISRA C Rule 20.7

[Nicola Vetrini]

Hi all,

this series aims to refactor some macros that cause violations of MISRA C Rule
20.7 ("Expressions resulting from the expansion of macro parameters shall be
enclosed in parentheses"). All the macros touched by these patches are in some
way involved in violations, and the strategy adopted to bring them into
compliance is to add parentheses around macro arguments where needed.

Given that the community has previously requested a deviation from the rule, as
stated in docs/misra/deviations.rst, and reported below for convenience [1],
some macro parameters do not need any adjusting (e.g., when used as lhs to
an assignment in statement expressions).

This series fixes a significant portion of the violations on Arm
(from ~14000 to ~2500). On x86, though there is one patch touching it, there are
still many more; they will be part of a later series.

[1] - Code violating Rule 20.7 is safe when macro parameters are used:
       (1) as function arguments;
       (2) as macro arguments;
       (3) as array indices;
       (4) as lhs in assignments.

Nicola Vetrini (10):
  xen/include: address violations of MISRA C Rule 20.7
  xen/arm: address some violations of MISRA C Rule 20.7
  x86: address some violations of MISRA C Rule 20.7
  xen/public: address violations of MISRA C Rule 20.7
  xen/perfc: address violations of MISRA C Rule 20.7
  arm/smmu: address some violations of MISRA C Rule 20.7
  xen/arm: smmuv3: address violations of MISRA C Rule 20.7
  xen/errno: address violations of MISRA C Rule 20.7
  xen/include: tasklet: address violations of MISRA C Rule 20.7
  xen/keyhandler: address violations of MISRA C Rule 20.7

 xen/arch/arm/arm64/cpufeature.c          | 14 +++---
 xen/arch/arm/cpuerrata.c                 |  4 +-
 xen/arch/arm/include/asm/arm64/sysregs.h |  2 +-
 xen/arch/arm/include/asm/guest_atomics.h |  4 +-
 xen/arch/arm/include/asm/mm.h            |  2 +-
 xen/arch/arm/include/asm/smccc.h         |  8 ++--
 xen/arch/arm/include/asm/vgic-emul.h     |  8 ++--
 xen/arch/arm/vcpreg.c                    |  5 +-
 xen/arch/x86/include/asm/irq.h           |  6 +--
 xen/arch/x86/usercopy.c                  |  2 +-
 xen/common/keyhandler.c                  |  4 +-
 xen/common/perfc.c                       |  8 ++--
 xen/drivers/passthrough/arm/smmu-v3.c    |  2 +-
 xen/drivers/passthrough/arm/smmu.c       |  4 +-
 xen/include/public/xen.h                 |  2 +-
 xen/include/xen/bug.h                    |  2 +-
 xen/include/xen/errno.h                  |  2 +-
 xen/include/xen/init.h                   |  4 +-
 xen/include/xen/kconfig.h                |  2 +-
 xen/include/xen/list.h                   | 59 ++++++++++++------------
 xen/include/xen/param.h                  | 22 ++++-----
 xen/include/xen/spinlock.h               |  2 +-
 xen/include/xen/tasklet.h                |  2 +-
 23 files changed, 85 insertions(+), 85 deletions(-)

-- 
2.34.1

[XEN PATCH 01/10] xen/include: address violations of MISRA C Rule 20.7

[Nicola Vetrini]

MISRA C Rule 20.7 states: "Expressions resulting from the expansion
of macro parameters shall be enclosed in parentheses". Therefore, some
macro definitions should gain additional parentheses to ensure that all
current and future users will be safe with respect to expansions that
can possibly alter the semantics of the passed-in macro parameter.

No functional change.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
 xen/include/xen/bug.h      |  2 +-
 xen/include/xen/init.h     |  4 +--
 xen/include/xen/kconfig.h  |  2 +-
 xen/include/xen/list.h     | 59 +++++++++++++++++++-------------------
 xen/include/xen/param.h    | 22 +++++++-------
 xen/include/xen/spinlock.h |  2 +-
 6 files changed, 45 insertions(+), 46 deletions(-)

diff --git a/xen/include/xen/bug.h b/xen/include/xen/bug.h
index 2c45c462fc63..77fe1e1ba840 100644
--- a/xen/include/xen/bug.h
+++ b/xen/include/xen/bug.h
@@ -80,7 +80,7 @@ struct bug_frame {
     [bf_type]    "i" (type),                                                 \
     [bf_ptr]     "i" (ptr),                                                  \
     [bf_msg]     "i" (msg),                                                  \
-    [bf_line_lo] "i" ((line & ((1 << BUG_LINE_LO_WIDTH) - 1))                \
+    [bf_line_lo] "i" (((line) & ((1 << BUG_LINE_LO_WIDTH) - 1))              \
                       << BUG_DISP_WIDTH),                                    \
     [bf_line_hi] "i" (((line) >> BUG_LINE_LO_WIDTH) << BUG_DISP_WIDTH)
 
diff --git a/xen/include/xen/init.h b/xen/include/xen/init.h
index 1d7c0216bc80..0a4223833755 100644
--- a/xen/include/xen/init.h
+++ b/xen/include/xen/init.h
@@ -63,9 +63,9 @@ typedef int (*initcall_t)(void);
 typedef void (*exitcall_t)(void);
 
 #define presmp_initcall(fn) \
-    const static initcall_t __initcall_##fn __init_call("presmp") = fn
+    const static initcall_t __initcall_##fn __init_call("presmp") = (fn)
 #define __initcall(fn) \
-    const static initcall_t __initcall_##fn __init_call("1") = fn
+    const static initcall_t __initcall_##fn __init_call("1") = (fn)
 #define __exitcall(fn) \
     static exitcall_t __exitcall_##fn __exit_call = fn
 
diff --git a/xen/include/xen/kconfig.h b/xen/include/xen/kconfig.h
index c25dc0f6c2a9..b7e70289737b 100644
--- a/xen/include/xen/kconfig.h
+++ b/xen/include/xen/kconfig.h
@@ -25,7 +25,7 @@
 #define __ARG_PLACEHOLDER_1 0,
 #define config_enabled(cfg) _config_enabled(cfg)
 #define _config_enabled(value) __config_enabled(__ARG_PLACEHOLDER_##value)
-#define __config_enabled(arg1_or_junk) ___config_enabled(arg1_or_junk 1, 0)
+#define __config_enabled(arg1_or_junk) ___config_enabled(arg1_or_junk (1), (0))
 #define ___config_enabled(__ignored, val, ...) val
 
 /*
diff --git a/xen/include/xen/list.h b/xen/include/xen/list.h
index b5eab3a1eb6c..d803e7848cad 100644
--- a/xen/include/xen/list.h
+++ b/xen/include/xen/list.h
@@ -490,9 +490,9 @@ static inline void list_splice_init(struct list_head *list,
  * @member: the name of the list_struct within the struct.
  */
 #define list_for_each_entry(pos, head, member)                          \
-    for (pos = list_entry((head)->next, typeof(*pos), member);          \
-         &pos->member != (head);                                        \
-         pos = list_entry(pos->member.next, typeof(*pos), member))
+    for (pos = list_entry((head)->next, typeof(*(pos)), member);          \
+         &(pos)->member != (head);                                      \
+         pos = list_entry((pos)->member.next, typeof(*(pos)), member))
 
 /**
  * list_for_each_entry_reverse - iterate backwards over list of given type.
@@ -501,9 +501,9 @@ static inline void list_splice_init(struct list_head *list,
  * @member: the name of the list_struct within the struct.
  */
 #define list_for_each_entry_reverse(pos, head, member)                  \
-    for (pos = list_entry((head)->prev, typeof(*pos), member);          \
-         &pos->member != (head);                                        \
-         pos = list_entry(pos->member.prev, typeof(*pos), member))
+    for (pos = list_entry((head)->prev, typeof(*(pos)), member);          \
+         &(pos)->member != (head);                                      \
+         pos = list_entry((pos)->member.prev, typeof(*(pos)), member))
 
 /**
  * list_prepare_entry - prepare a pos entry for use in
@@ -516,7 +516,7 @@ static inline void list_splice_init(struct list_head *list,
  * list_for_each_entry_continue.
  */
 #define list_prepare_entry(pos, head, member)           \
-    ((pos) ? : list_entry(head, typeof(*pos), member))
+    ((pos) ? : list_entry(head, typeof(*(pos)), member))
 
 /**
  * list_for_each_entry_continue - continue iteration over list of given type
@@ -528,9 +528,9 @@ static inline void list_splice_init(struct list_head *list,
  * the current position.
  */
 #define list_for_each_entry_continue(pos, head, member)                 \
-    for (pos = list_entry(pos->member.next, typeof(*pos), member);      \
-         &pos->member != (head);                                        \
-         pos = list_entry(pos->member.next, typeof(*pos), member))
+    for (pos = list_entry((pos)->member.next, typeof(*(pos)), member);  \
+         &(pos)->member != (head);                                      \
+         pos = list_entry((pos)->member.next, typeof(*(pos)), member))
 
 /**
  * list_for_each_entry_from - iterate over list of given type from the
@@ -542,8 +542,8 @@ static inline void list_splice_init(struct list_head *list,
  * Iterate over list of given type, continuing from current position.
  */
 #define list_for_each_entry_from(pos, head, member)                     \
-    for (; &pos->member != (head);                                      \
-         pos = list_entry(pos->member.next, typeof(*pos), member))
+    for (; &(pos)->member != (head);                                    \
+         pos = list_entry((pos)->member.next, typeof(*(pos)), member))
 
 /**
  * list_for_each_entry_safe - iterate over list of given type safe
@@ -554,10 +554,10 @@ static inline void list_splice_init(struct list_head *list,
  * @member: the name of the list_struct within the struct.
  */
 #define list_for_each_entry_safe(pos, n, head, member)                  \
-    for (pos = list_entry((head)->next, typeof(*pos), member),          \
-         n = list_entry(pos->member.next, typeof(*pos), member);        \
-         &pos->member != (head);                                        \
-         pos = n, n = list_entry(n->member.next, typeof(*n), member))
+    for (pos = list_entry((head)->next, typeof(*(pos)), member),        \
+         n = list_entry((pos)->member.next, typeof(*(pos)), member);    \
+         &(pos)->member != (head);                                      \
+         pos = (n), n = list_entry((n)->member.next, typeof(*(n)), member))
 
 /**
  * list_for_each_entry_safe_continue
@@ -570,10 +570,10 @@ static inline void list_splice_init(struct list_head *list,
  * safe against removal of list entry.
  */
 #define list_for_each_entry_safe_continue(pos, n, head, member)         \
-    for (pos = list_entry(pos->member.next, typeof(*pos), member),      \
-         n = list_entry(pos->member.next, typeof(*pos), member);        \
-         &pos->member != (head);                                        \
-         pos = n, n = list_entry(n->member.next, typeof(*n), member))
+    for (pos = list_entry((pos)->member.next, typeof(*(pos)), member),  \
+         n = list_entry((pos)->member.next, typeof(*(pos)), member);    \
+         &(pos)->member != (head);                                      \
+         pos = (n), n = list_entry((n)->member.next, typeof(*(n)), member))
 
 /**
  * list_for_each_entry_safe_from
@@ -586,9 +586,9 @@ static inline void list_splice_init(struct list_head *list,
  * removal of list entry.
  */
 #define list_for_each_entry_safe_from(pos, n, head, member)             \
-    for (n = list_entry(pos->member.next, typeof(*pos), member);        \
-         &pos->member != (head);                                        \
-         pos = n, n = list_entry(n->member.next, typeof(*n), member))
+    for (n = list_entry((pos)->member.next, typeof(*(pos)), member);    \
+         &(pos)->member != (head);                                      \
+         pos = (n), n = list_entry((n)->member.next, typeof(*(n)), member))
 
 /**
  * list_for_each_entry_safe_reverse
@@ -601,10 +601,10 @@ static inline void list_splice_init(struct list_head *list,
  * of list entry.
  */
 #define list_for_each_entry_safe_reverse(pos, n, head, member)          \
-    for (pos = list_entry((head)->prev, typeof(*pos), member),          \
-         n = list_entry(pos->member.prev, typeof(*pos), member);        \
-         &pos->member != (head);                                        \
-         pos = n, n = list_entry(n->member.prev, typeof(*n), member))
+    for (pos = list_entry((head)->prev, typeof(*(pos)), member),        \
+         n = list_entry((pos)->member.prev, typeof(*(pos)), member);    \
+         &(pos)->member != (head);                                      \
+         pos = (n), n = list_entry((n)->member.prev, typeof(*(n)), member))
 
 /**
  * list_for_each_rcu - iterate over an rcu-protected list
@@ -653,9 +653,9 @@ static inline void list_splice_init(struct list_head *list,
  * as long as the traversal is guarded by rcu_read_lock().
  */
 #define list_for_each_entry_rcu(pos, head, member)                      \
-    for (pos = list_entry((head)->next, typeof(*pos), member);          \
+    for (pos = list_entry((head)->next, typeof(*(pos)), member);        \
          &rcu_dereference(pos)->member != (head);                       \
-         pos = list_entry(pos->member.next, typeof(*pos), member))
+         pos = list_entry((pos)->member.next, typeof(*(pos)), member))
 
 /**
  * list_for_each_continue_rcu
@@ -977,4 +977,3 @@ static inline void hlist_add_after_rcu(struct hlist_node *prev,
           pos = pos->next)
 
 #endif /* __XEN_LIST_H__ */
-
diff --git a/xen/include/xen/param.h b/xen/include/xen/param.h
index 13607e0e50e0..1bdbab34ab1f 100644
--- a/xen/include/xen/param.h
+++ b/xen/include/xen/param.h
@@ -45,42 +45,42 @@ extern const struct kernel_param __setup_start[], __setup_end[];
 #define TEMP_NAME(base) _TEMP_NAME(base, __LINE__)
 
 #define custom_param(_name, _var) \
-    __setup_str __setup_str_##_var[] = _name; \
+    __setup_str __setup_str_##_var[] = (_name); \
     __kparam __setup_##_var = \
         { .name = __setup_str_##_var, \
           .type = OPT_CUSTOM, \
-          .par.func = _var }
+          .par.func = (_var) }
 #define boolean_param(_name, _var) \
-    __setup_str __setup_str_##_var[] = _name; \
+    __setup_str __setup_str_##_var[] = (_name); \
     __kparam __setup_##_var = \
         { .name = __setup_str_##_var, \
           .type = OPT_BOOL, \
           .len = sizeof(_var) + \
                  BUILD_BUG_ON_ZERO(sizeof(_var) != sizeof(bool)), \
-          .par.var = &_var }
+          .par.var = &(_var) }
 #define integer_param(_name, _var) \
-    __setup_str __setup_str_##_var[] = _name; \
+    __setup_str __setup_str_##_var[] = (_name); \
     __kparam __setup_##_var = \
         { .name = __setup_str_##_var, \
           .type = OPT_UINT, \
           .len = sizeof(_var), \
-          .par.var = &_var }
+          .par.var = &(_var) }
 #define size_param(_name, _var) \
-    __setup_str __setup_str_##_var[] = _name; \
+    __setup_str __setup_str_##_var[] = (_name); \
     __kparam __setup_##_var = \
         { .name = __setup_str_##_var, \
           .type = OPT_SIZE, \
           .len = sizeof(_var), \
-          .par.var = &_var }
+          .par.var = &(_var) }
 #define string_param(_name, _var) \
-    __setup_str __setup_str_##_var[] = _name; \
+    __setup_str __setup_str_##_var[] = (_name); \
     __kparam __setup_##_var = \
         { .name = __setup_str_##_var, \
           .type = OPT_STR, \
           .len = sizeof(_var), \
-          .par.var = &_var }
+          .par.var = &(_var) }
 #define ignore_param(_name)                 \
-    __setup_str TEMP_NAME(__setup_str_ign)[] = _name;    \
+    __setup_str TEMP_NAME(__setup_str_ign)[] = (_name);  \
     __kparam TEMP_NAME(__setup_ign) =                    \
         { .name = TEMP_NAME(__setup_str_ign),            \
           .type = OPT_IGNORE }
diff --git a/xen/include/xen/spinlock.h b/xen/include/xen/spinlock.h
index 1cd9120eac7a..0e6a083dfb9e 100644
--- a/xen/include/xen/spinlock.h
+++ b/xen/include/xen/spinlock.h
@@ -94,7 +94,7 @@ struct lock_profile_qhead {
     int32_t                   idx;     /* index for printout */
 };
 
-#define _LOCK_PROFILE(lockname) { .name = #lockname, .lock = &lockname, }
+#define _LOCK_PROFILE(lockname) { .name = #lockname, .lock = &(lockname), }
 #define _LOCK_PROFILE_PTR(name)                                               \
     static struct lock_profile * const __lock_profile_##name                  \
     __used_section(".lockprofile.data") =                                     \
-- 
2.34.1

Re: [XEN PATCH 01/10] xen/include: address violations of MISRA C Rule 20.7

[Andrew Cooper]

On 29/02/2024 3:27 pm, Nicola Vetrini wrote:
> diff --git a/xen/include/xen/kconfig.h b/xen/include/xen/kconfig.h
> index c25dc0f6c2a9..b7e70289737b 100644
> --- a/xen/include/xen/kconfig.h
> +++ b/xen/include/xen/kconfig.h
> @@ -25,7 +25,7 @@
>  #define __ARG_PLACEHOLDER_1 0,
>  #define config_enabled(cfg) _config_enabled(cfg)
>  #define _config_enabled(value) __config_enabled(__ARG_PLACEHOLDER_##value)
> -#define __config_enabled(arg1_or_junk) ___config_enabled(arg1_or_junk 1, 0)
> +#define __config_enabled(arg1_or_junk) ___config_enabled(arg1_or_junk (1), (0))
>  #define ___config_enabled(__ignored, val, ...) val

This one hunk I suggest we deviate rather than adjust.  You've subtly
broken it, and it's extreme preprocessor magic in the first place to
turn an absent symbol into a 0.

~Andrew

Re: [XEN PATCH 01/10] xen/include: address violations of MISRA C Rule 20.7

[Nicola Vetrini]

On 2024-02-29 17:10, Andrew Cooper wrote:
> On 29/02/2024 3:27 pm, Nicola Vetrini wrote:
>> diff --git a/xen/include/xen/kconfig.h b/xen/include/xen/kconfig.h
>> index c25dc0f6c2a9..b7e70289737b 100644
>> --- a/xen/include/xen/kconfig.h
>> +++ b/xen/include/xen/kconfig.h
>> @@ -25,7 +25,7 @@
>>  #define __ARG_PLACEHOLDER_1 0,
>>  #define config_enabled(cfg) _config_enabled(cfg)
>>  #define _config_enabled(value) 
>> __config_enabled(__ARG_PLACEHOLDER_##value)
>> -#define __config_enabled(arg1_or_junk) ___config_enabled(arg1_or_junk 
>> 1, 0)
>> +#define __config_enabled(arg1_or_junk) ___config_enabled(arg1_or_junk 
>> (1), (0))
>>  #define ___config_enabled(__ignored, val, ...) val
> 
> This one hunk I suggest we deviate rather than adjust.  You've subtly
> broken it, and it's extreme preprocessor magic in the first place to
> turn an absent symbol into a 0.
> 

How so? I did test this because I was very wary of it, but it seemed to 
expand fine (either if ((0)) or if ((1)) ). I may of course be wrong, 
and it could be deviated regardless.

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

Re: [XEN PATCH 01/10] xen/include: address violations of MISRA C Rule 20.7

[Andrew Cooper]

On 29/02/2024 4:21 pm, Nicola Vetrini wrote:
> On 2024-02-29 17:10, Andrew Cooper wrote:
>> On 29/02/2024 3:27 pm, Nicola Vetrini wrote:
>>> diff --git a/xen/include/xen/kconfig.h b/xen/include/xen/kconfig.h
>>> index c25dc0f6c2a9..b7e70289737b 100644
>>> --- a/xen/include/xen/kconfig.h
>>> +++ b/xen/include/xen/kconfig.h
>>> @@ -25,7 +25,7 @@
>>>  #define __ARG_PLACEHOLDER_1 0,
>>>  #define config_enabled(cfg) _config_enabled(cfg)
>>>  #define _config_enabled(value)
>>> __config_enabled(__ARG_PLACEHOLDER_##value)
>>> -#define __config_enabled(arg1_or_junk)
>>> ___config_enabled(arg1_or_junk 1, 0)
>>> +#define __config_enabled(arg1_or_junk)
>>> ___config_enabled(arg1_or_junk (1), (0))
>>>  #define ___config_enabled(__ignored, val, ...) val
>>
>> This one hunk I suggest we deviate rather than adjust.  You've subtly
>> broken it, and it's extreme preprocessor magic in the first place to
>> turn an absent symbol into a 0.
>>
>
> How so? I did test this because I was very wary of it, but it seemed
> to expand fine (either if ((0)) or if ((1)) ). I may of course be
> wrong, and it could be deviated regardless.
>

arg1_or_junk(1) can now be a function or macro expansion depending on
context, where previously it couldn't be.

~Andrew

Re: [XEN PATCH 01/10] xen/include: address violations of MISRA C Rule 20.7

[Nicola Vetrini]

On 2024-02-29 17:47, Andrew Cooper wrote:
> On 29/02/2024 4:21 pm, Nicola Vetrini wrote:
>> On 2024-02-29 17:10, Andrew Cooper wrote:
>>> On 29/02/2024 3:27 pm, Nicola Vetrini wrote:
>>>> diff --git a/xen/include/xen/kconfig.h b/xen/include/xen/kconfig.h
>>>> index c25dc0f6c2a9..b7e70289737b 100644
>>>> --- a/xen/include/xen/kconfig.h
>>>> +++ b/xen/include/xen/kconfig.h
>>>> @@ -25,7 +25,7 @@
>>>>  #define __ARG_PLACEHOLDER_1 0,
>>>>  #define config_enabled(cfg) _config_enabled(cfg)
>>>>  #define _config_enabled(value)
>>>> __config_enabled(__ARG_PLACEHOLDER_##value)
>>>> -#define __config_enabled(arg1_or_junk)
>>>> ___config_enabled(arg1_or_junk 1, 0)
>>>> +#define __config_enabled(arg1_or_junk)
>>>> ___config_enabled(arg1_or_junk (1), (0))
>>>>  #define ___config_enabled(__ignored, val, ...) val
>>> 
>>> This one hunk I suggest we deviate rather than adjust.  You've subtly
>>> broken it, and it's extreme preprocessor magic in the first place to
>>> turn an absent symbol into a 0.
>>> 
>> 
>> How so? I did test this because I was very wary of it, but it seemed
>> to expand fine (either if ((0)) or if ((1)) ). I may of course be
>> wrong, and it could be deviated regardless.
>> 
> 
> arg1_or_junk(1) can now be a function or macro expansion depending on
> context, where previously it couldn't be.
> 

I see, that would be a latent bug. I do agree on deviating then.

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

Re: [XEN PATCH 01/10] xen/include: address violations of MISRA C Rule 20.7

[Jan Beulich]

On 29.02.2024 16:27, Nicola Vetrini wrote:
> --- a/xen/include/xen/kconfig.h
> +++ b/xen/include/xen/kconfig.h
> @@ -25,7 +25,7 @@
>  #define __ARG_PLACEHOLDER_1 0,
>  #define config_enabled(cfg) _config_enabled(cfg)
>  #define _config_enabled(value) __config_enabled(__ARG_PLACEHOLDER_##value)
> -#define __config_enabled(arg1_or_junk) ___config_enabled(arg1_or_junk 1, 0)
> +#define __config_enabled(arg1_or_junk) ___config_enabled(arg1_or_junk (1), (0))
>  #define ___config_enabled(__ignored, val, ...) val

In addition to what Andrew said, would you mind clarifying what exactly the
violation is here? I find it questionable that numeric literals need
parenthesizing; they don't normally need to, aynwhere.

> --- a/xen/include/xen/list.h
> +++ b/xen/include/xen/list.h
> @@ -490,9 +490,9 @@ static inline void list_splice_init(struct list_head *list,
>   * @member: the name of the list_struct within the struct.
>   */
>  #define list_for_each_entry(pos, head, member)                          \
> -    for (pos = list_entry((head)->next, typeof(*pos), member);          \
> -         &pos->member != (head);                                        \
> -         pos = list_entry(pos->member.next, typeof(*pos), member))
> +    for (pos = list_entry((head)->next, typeof(*(pos)), member);          \
> +         &(pos)->member != (head);                                      \
> +         pos = list_entry((pos)->member.next, typeof(*(pos)), member))

this ends up inconsistent, which I think isn't nice: Some uses of "pos"
are now parenthesized, while others aren't. Applies further down as well.

You may also want to take this as a strong suggestion to split dissimilar
changes, so uncontroversial parts can go in.

> @@ -977,4 +977,3 @@ static inline void hlist_add_after_rcu(struct hlist_node *prev,
>            pos = pos->next)
>  
>  #endif /* __XEN_LIST_H__ */
> -

Unrelated change?

> --- a/xen/include/xen/spinlock.h
> +++ b/xen/include/xen/spinlock.h
> @@ -94,7 +94,7 @@ struct lock_profile_qhead {
>      int32_t                   idx;     /* index for printout */
>  };
>  
> -#define _LOCK_PROFILE(lockname) { .name = #lockname, .lock = &lockname, }
> +#define _LOCK_PROFILE(lockname) { .name = #lockname, .lock = &(lockname), }

This also may be viewed as falling in the same category, but is less
problematic because the other use is stringification, when in principle
some kind of expression would be passed in (albeit in practice I don't
expect anyone would do that).

Jan

Re: [XEN PATCH 01/10] xen/include: address violations of MISRA C Rule 20.7

[Nicola Vetrini]

On 2024-02-29 17:25, Jan Beulich wrote:
> On 29.02.2024 16:27, Nicola Vetrini wrote:
>> --- a/xen/include/xen/kconfig.h
>> +++ b/xen/include/xen/kconfig.h
>> @@ -25,7 +25,7 @@
>>  #define __ARG_PLACEHOLDER_1 0,
>>  #define config_enabled(cfg) _config_enabled(cfg)
>>  #define _config_enabled(value) 
>> __config_enabled(__ARG_PLACEHOLDER_##value)
>> -#define __config_enabled(arg1_or_junk) ___config_enabled(arg1_or_junk 
>> 1, 0)
>> +#define __config_enabled(arg1_or_junk) ___config_enabled(arg1_or_junk 
>> (1), (0))
>>  #define ___config_enabled(__ignored, val, ...) val
> 
> In addition to what Andrew said, would you mind clarifying what exactly 
> the
> violation is here? I find it questionable that numeric literals need
> parenthesizing; they don't normally need to, aynwhere.
> 

This one's a little special. I couldn't parenthesize val further down, 
because then it would break the build:

In file included from ././include/xen/config.h:14,
                  from <command-line>:
./include/xen/kconfig.h:29:52: error: expected identifier or ‘(’ before 
‘)’ token
    29 | #define ___config_enabled(__ignored, val, ...) (val)
       |                                                    ^
./include/xen/kconfig.h:39:34: note: in expansion of macro 
‘___config_enabled’
    39 | #define _static_if(arg1_or_junk) ___config_enabled(arg1_or_junk 
static,)
       |                                  ^~~~~~~~~~~~~~~~~
./include/xen/kconfig.h:38:26: note: in expansion of macro ‘_static_if’
    38 | #define static_if(value) _static_if(__ARG_PLACEHOLDER_##value)
       |                          ^~~~~~~~~~
./include/xen/kconfig.h:41:27: note: in expansion of macro ‘static_if’
    41 | #define STATIC_IF(option) static_if(option)
       |                           ^~~~~~~~~
common/page_alloc.c:260:1: note: in expansion of macro ‘STATIC_IF’
   260 | STATIC_IF(CONFIG_NUMA) mfn_t first_valid_mfn = 
INVALID_MFN_INITIALIZER;
       | ^~~~~~~~~
make[2]: *** [Rules.mk:249: common/page_alloc.o] Error 1


>> --- a/xen/include/xen/list.h
>> +++ b/xen/include/xen/list.h
>> @@ -490,9 +490,9 @@ static inline void list_splice_init(struct 
>> list_head *list,
>>   * @member: the name of the list_struct within the struct.
>>   */
>>  #define list_for_each_entry(pos, head, member)                        
>>   \
>> -    for (pos = list_entry((head)->next, typeof(*pos), member);        
>>   \
>> -         &pos->member != (head);                                      
>>   \
>> -         pos = list_entry(pos->member.next, typeof(*pos), member))
>> +    for (pos = list_entry((head)->next, typeof(*(pos)), member);      
>>     \
>> +         &(pos)->member != (head);                                    
>>   \
>> +         pos = list_entry((pos)->member.next, typeof(*(pos)), 
>> member))
> 
> this ends up inconsistent, which I think isn't nice: Some uses of "pos"
> are now parenthesized, while others aren't. Applies further down as 
> well.
> 

Yeah, the inconsistency is due to the fact that a non-parenthesized 
parameter as lhs is already deviated. To keep it consistent here I can 
add parentheses, but then the deviation would be kind of pointless, 
wouldn't it?

> You may also want to take this as a strong suggestion to split 
> dissimilar
> changes, so uncontroversial parts can go in.
> 

Ok, that was an oversight.

>> @@ -977,4 +977,3 @@ static inline void hlist_add_after_rcu(struct 
>> hlist_node *prev,
>>            pos = pos->next)
>> 
>>  #endif /* __XEN_LIST_H__ */
>> -
> 
> Unrelated change?
> 

Oh, yes. Will drop it.

>> --- a/xen/include/xen/spinlock.h
>> +++ b/xen/include/xen/spinlock.h
>> @@ -94,7 +94,7 @@ struct lock_profile_qhead {
>>      int32_t                   idx;     /* index for printout */
>>  };
>> 
>> -#define _LOCK_PROFILE(lockname) { .name = #lockname, .lock = 
>> &lockname, }
>> +#define _LOCK_PROFILE(lockname) { .name = #lockname, .lock = 
>> &(lockname), }
> 
> This also may be viewed as falling in the same category, but is less
> problematic because the other use is stringification, when in principle
> some kind of expression would be passed in (albeit in practice I don't
> expect anyone would do that).
> 

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

Re: [XEN PATCH 01/10] xen/include: address violations of MISRA C Rule 20.7

[Jan Beulich]

On 29.02.2024 17:40, Nicola Vetrini wrote:
> On 2024-02-29 17:25, Jan Beulich wrote:
>> On 29.02.2024 16:27, Nicola Vetrini wrote:
>>> --- a/xen/include/xen/kconfig.h
>>> +++ b/xen/include/xen/kconfig.h
>>> @@ -25,7 +25,7 @@
>>>  #define __ARG_PLACEHOLDER_1 0,
>>>  #define config_enabled(cfg) _config_enabled(cfg)
>>>  #define _config_enabled(value) 
>>> __config_enabled(__ARG_PLACEHOLDER_##value)
>>> -#define __config_enabled(arg1_or_junk) ___config_enabled(arg1_or_junk 
>>> 1, 0)
>>> +#define __config_enabled(arg1_or_junk) ___config_enabled(arg1_or_junk 
>>> (1), (0))
>>>  #define ___config_enabled(__ignored, val, ...) val
>>
>> In addition to what Andrew said, would you mind clarifying what exactly 
>> the
>> violation is here? I find it questionable that numeric literals need
>> parenthesizing; they don't normally need to, aynwhere.
>>
> 
> This one's a little special. I couldn't parenthesize val further down, 
> because then it would break the build:
> 
> In file included from ././include/xen/config.h:14,
>                   from <command-line>:
> ./include/xen/kconfig.h:29:52: error: expected identifier or ‘(’ before 
> ‘)’ token
>     29 | #define ___config_enabled(__ignored, val, ...) (val)
>        |                                                    ^
> ./include/xen/kconfig.h:39:34: note: in expansion of macro 
> ‘___config_enabled’
>     39 | #define _static_if(arg1_or_junk) ___config_enabled(arg1_or_junk 
> static,)
>        |                                  ^~~~~~~~~~~~~~~~~
> ./include/xen/kconfig.h:38:26: note: in expansion of macro ‘_static_if’
>     38 | #define static_if(value) _static_if(__ARG_PLACEHOLDER_##value)
>        |                          ^~~~~~~~~~
> ./include/xen/kconfig.h:41:27: note: in expansion of macro ‘static_if’
>     41 | #define STATIC_IF(option) static_if(option)
>        |                           ^~~~~~~~~
> common/page_alloc.c:260:1: note: in expansion of macro ‘STATIC_IF’
>    260 | STATIC_IF(CONFIG_NUMA) mfn_t first_valid_mfn = 
> INVALID_MFN_INITIALIZER;
>        | ^~~~~~~~~
> make[2]: *** [Rules.mk:249: common/page_alloc.o] Error 1

So if we're not gong to deviate the construct, then this change needs to
come entirely on its own, with a really good description.

>>> --- a/xen/include/xen/list.h
>>> +++ b/xen/include/xen/list.h
>>> @@ -490,9 +490,9 @@ static inline void list_splice_init(struct 
>>> list_head *list,
>>>   * @member: the name of the list_struct within the struct.
>>>   */
>>>  #define list_for_each_entry(pos, head, member)                        
>>>   \
>>> -    for (pos = list_entry((head)->next, typeof(*pos), member);        
>>>   \
>>> -         &pos->member != (head);                                      
>>>   \
>>> -         pos = list_entry(pos->member.next, typeof(*pos), member))
>>> +    for (pos = list_entry((head)->next, typeof(*(pos)), member);      
>>>     \
>>> +         &(pos)->member != (head);                                    
>>>   \
>>> +         pos = list_entry((pos)->member.next, typeof(*(pos)), 
>>> member))
>>
>> this ends up inconsistent, which I think isn't nice: Some uses of "pos"
>> are now parenthesized, while others aren't. Applies further down as 
>> well.
>>
> 
> Yeah, the inconsistency is due to the fact that a non-parenthesized 
> parameter as lhs is already deviated. To keep it consistent here I can 
> add parentheses, but then the deviation would be kind of pointless, 
> wouldn't it?

I don't think so: It would still be useful for cases where a macro
parameter is used solely as the lhs of some kind of assignment
operator. But yes, before making adjustments you will want to wait
for possible further comments, especially from e.g. Julien, who iirc
was primarily against this kind of parenthesization.

Jan

[XEN PATCH 02/10] xen/arm: address some violations of MISRA C Rule 20.7

[Nicola Vetrini]

MISRA C Rule 20.7 states: "Expressions resulting from the expansion
of macro parameters shall be enclosed in parentheses". Therefore, some
macro definitions should gain additional parentheses to ensure that all
current and future users will be safe with respect to expansions that
can possibly alter the semantics of the passed-in macro parameter.

No functional change.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
Style in arm64/cpufeature.c has not been amended, because this file seems
to be kept in sync with its Linux counterpart.
---
 xen/arch/arm/arm64/cpufeature.c          | 14 +++++++-------
 xen/arch/arm/cpuerrata.c                 |  4 ++--
 xen/arch/arm/include/asm/arm64/sysregs.h |  2 +-
 xen/arch/arm/include/asm/guest_atomics.h |  4 ++--
 xen/arch/arm/include/asm/mm.h            |  2 +-
 xen/arch/arm/include/asm/smccc.h         |  8 ++++----
 xen/arch/arm/include/asm/vgic-emul.h     |  8 ++++----
 xen/arch/arm/vcpreg.c                    |  5 +++--
 8 files changed, 24 insertions(+), 23 deletions(-)

diff --git a/xen/arch/arm/arm64/cpufeature.c b/xen/arch/arm/arm64/cpufeature.c
index 864413d9cc03..6fb8974ade7f 100644
--- a/xen/arch/arm/arm64/cpufeature.c
+++ b/xen/arch/arm/arm64/cpufeature.c
@@ -78,13 +78,13 @@
 
 #define __ARM64_FTR_BITS(SIGNED, VISIBLE, STRICT, TYPE, SHIFT, WIDTH, SAFE_VAL) \
 	{						\
-		.sign = SIGNED,				\
-		.visible = VISIBLE,			\
-		.strict = STRICT,			\
-		.type = TYPE,				\
-		.shift = SHIFT,				\
-		.width = WIDTH,				\
-		.safe_val = SAFE_VAL,			\
+		.sign = (SIGNED),				\
+		.visible = (VISIBLE),			\
+		.strict = (STRICT),			\
+		.type = (TYPE),				\
+		.shift = (SHIFT),				\
+		.width = (WIDTH),				\
+		.safe_val = (SAFE_VAL),			\
 	}
 
 /* Define a feature with unsigned values */
diff --git a/xen/arch/arm/cpuerrata.c b/xen/arch/arm/cpuerrata.c
index a28fa6ac78cc..c678a555910f 100644
--- a/xen/arch/arm/cpuerrata.c
+++ b/xen/arch/arm/cpuerrata.c
@@ -462,8 +462,8 @@ static bool has_ssbd_mitigation(const struct arm_cpu_capabilities *entry)
 #define MIDR_RANGE(model, min, max)     \
     .matches = is_affected_midr_range,  \
     .midr_model = model,                \
-    .midr_range_min = min,              \
-    .midr_range_max = max
+    .midr_range_min = (min),            \
+    .midr_range_max = (max)
 
 #define MIDR_ALL_VERSIONS(model)        \
     .matches = is_affected_midr_range,  \
diff --git a/xen/arch/arm/include/asm/arm64/sysregs.h b/xen/arch/arm/include/asm/arm64/sysregs.h
index 3fdeb9d8cdef..b593e4028b53 100644
--- a/xen/arch/arm/include/asm/arm64/sysregs.h
+++ b/xen/arch/arm/include/asm/arm64/sysregs.h
@@ -465,7 +465,7 @@
 /* Access to system registers */
 
 #define WRITE_SYSREG64(v, name) do {                    \
-    uint64_t _r = v;                                    \
+    uint64_t _r = (v);                                  \
     asm volatile("msr "__stringify(name)", %0" : : "r" (_r));       \
 } while (0)
 #define READ_SYSREG64(name) ({                          \
diff --git a/xen/arch/arm/include/asm/guest_atomics.h b/xen/arch/arm/include/asm/guest_atomics.h
index a1745f8613f6..8893eb9a55d7 100644
--- a/xen/arch/arm/include/asm/guest_atomics.h
+++ b/xen/arch/arm/include/asm/guest_atomics.h
@@ -32,7 +32,7 @@ static inline void guest_##name(struct domain *d, int nr, volatile void *p) \
     perfc_incr(atomics_guest_paused);                                       \
                                                                             \
     domain_pause_nosync(d);                                                 \
-    name(nr, p);                                                            \
+    (name)(nr, p);                                                          \
     domain_unpause(d);                                                      \
 }
 
@@ -52,7 +52,7 @@ static inline int guest_##name(struct domain *d, int nr, volatile void *p)  \
     perfc_incr(atomics_guest_paused);                                       \
                                                                             \
     domain_pause_nosync(d);                                                 \
-    oldbit = name(nr, p);                                                   \
+    oldbit = (name)(nr, p);                                                 \
     domain_unpause(d);                                                      \
                                                                             \
     return oldbit;                                                          \
diff --git a/xen/arch/arm/include/asm/mm.h b/xen/arch/arm/include/asm/mm.h
index cbcf3bf14767..48538b5337aa 100644
--- a/xen/arch/arm/include/asm/mm.h
+++ b/xen/arch/arm/include/asm/mm.h
@@ -250,7 +250,7 @@ static inline void __iomem *ioremap_wc(paddr_t start, size_t len)
 #define gaddr_to_gfn(ga)    _gfn(paddr_to_pfn(ga))
 #define mfn_to_maddr(mfn)   pfn_to_paddr(mfn_x(mfn))
 #define maddr_to_mfn(ma)    _mfn(paddr_to_pfn(ma))
-#define vmap_to_mfn(va)     maddr_to_mfn(virt_to_maddr((vaddr_t)va))
+#define vmap_to_mfn(va)     maddr_to_mfn(virt_to_maddr((vaddr_t)(va)))
 #define vmap_to_page(va)    mfn_to_page(vmap_to_mfn(va))
 
 /* Page-align address and convert to frame number format */
diff --git a/xen/arch/arm/include/asm/smccc.h b/xen/arch/arm/include/asm/smccc.h
index 1adcd37443c7..a1f309eea45a 100644
--- a/xen/arch/arm/include/asm/smccc.h
+++ b/xen/arch/arm/include/asm/smccc.h
@@ -122,7 +122,7 @@ struct arm_smccc_res {
 #define __constraint_read_7 __constraint_read_6, "r" (r7)
 
 #define __declare_arg_0(a0, res)                            \
-    struct arm_smccc_res    *___res = res;                  \
+    struct arm_smccc_res    *___res = (res);                \
     register unsigned long  r0 ASM_REG(0) = (uint32_t)a0;   \
     register unsigned long  r1 ASM_REG(1);                  \
     register unsigned long  r2 ASM_REG(2);                  \
@@ -130,7 +130,7 @@ struct arm_smccc_res {
 
 #define __declare_arg_1(a0, a1, res)                        \
     typeof(a1) __a1 = a1;                                   \
-    struct arm_smccc_res    *___res = res;                  \
+    struct arm_smccc_res    *___res = (res);                \
     register unsigned long  r0 ASM_REG(0) = (uint32_t)a0;   \
     register unsigned long  r1 ASM_REG(1) = __a1;           \
     register unsigned long  r2 ASM_REG(2);                  \
@@ -139,7 +139,7 @@ struct arm_smccc_res {
 #define __declare_arg_2(a0, a1, a2, res)                    \
     typeof(a1) __a1 = a1;                                   \
     typeof(a2) __a2 = a2;                                   \
-    struct arm_smccc_res    *___res = res;				    \
+    struct arm_smccc_res    *___res = (res);                \
     register unsigned long  r0 ASM_REG(0) = (uint32_t)a0;   \
     register unsigned long  r1 ASM_REG(1) = __a1;           \
     register unsigned long  r2 ASM_REG(2) = __a2;           \
@@ -149,7 +149,7 @@ struct arm_smccc_res {
     typeof(a1) __a1 = a1;                                   \
     typeof(a2) __a2 = a2;                                   \
     typeof(a3) __a3 = a3;                                   \
-    struct arm_smccc_res    *___res = res;                  \
+    struct arm_smccc_res    *___res = (res);                \
     register unsigned long  r0 ASM_REG(0) = (uint32_t)a0;   \
     register unsigned long  r1 ASM_REG(1) = __a1;           \
     register unsigned long  r2 ASM_REG(2) = __a2;           \
diff --git a/xen/arch/arm/include/asm/vgic-emul.h b/xen/arch/arm/include/asm/vgic-emul.h
index e52fbaa3ec04..e2afb52498a8 100644
--- a/xen/arch/arm/include/asm/vgic-emul.h
+++ b/xen/arch/arm/include/asm/vgic-emul.h
@@ -6,11 +6,11 @@
  * a range of registers
  */
 
-#define VREG32(reg) reg ... reg + 3
-#define VREG64(reg) reg ... reg + 7
+#define VREG32(reg) (reg) ... (reg) + 3
+#define VREG64(reg) (reg) ... (reg) + 7
 
-#define VRANGE32(start, end) start ... end + 3
-#define VRANGE64(start, end) start ... end + 7
+#define VRANGE32(start, end) (start) ... (end) + 3
+#define VRANGE64(start, end) (start) ... (end) + 7
 
 /*
  * 64 bits registers can be accessible using 32-bit and 64-bit unless
diff --git a/xen/arch/arm/vcpreg.c b/xen/arch/arm/vcpreg.c
index a2d050070473..019cf34f003a 100644
--- a/xen/arch/arm/vcpreg.c
+++ b/xen/arch/arm/vcpreg.c
@@ -39,7 +39,8 @@
  */
 
 #ifdef CONFIG_ARM_64
-#define WRITE_SYSREG_SZ(sz, val, sysreg) WRITE_SYSREG((uint##sz##_t)val, sysreg)
+#define WRITE_SYSREG_SZ(sz, val, sysreg) \
+    WRITE_SYSREG((uint##sz##_t)(val), sysreg)
 #else
 /*
  * WRITE_SYSREG{32/64} on arm32 is defined as variadic macro which imposes
@@ -64,7 +65,7 @@ static bool func(struct cpu_user_regs *regs, type##sz##_t *r, bool read)    \
     bool cache_enabled = vcpu_has_cache_enabled(v);                         \
                                                                             \
     GUEST_BUG_ON(read);                                                     \
-    WRITE_SYSREG_SZ(sz, *r, reg);                                           \
+    WRITE_SYSREG_SZ(sz, *(r), reg);                                         \
                                                                             \
     p2m_toggle_cache(v, cache_enabled);                                     \
                                                                             \
-- 
2.34.1

Re: [XEN PATCH 02/10] xen/arm: address some violations of MISRA C Rule 20.7

[Jan Beulich]

On 29.02.2024 16:27, Nicola Vetrini wrote:
> --- a/xen/arch/arm/cpuerrata.c
> +++ b/xen/arch/arm/cpuerrata.c
> @@ -462,8 +462,8 @@ static bool has_ssbd_mitigation(const struct arm_cpu_capabilities *entry)
>  #define MIDR_RANGE(model, min, max)     \
>      .matches = is_affected_midr_range,  \
>      .midr_model = model,                \
> -    .midr_range_min = min,              \
> -    .midr_range_max = max
> +    .midr_range_min = (min),            \
> +    .midr_range_max = (max)

Why min and max, but not model?

> --- a/xen/arch/arm/include/asm/smccc.h
> +++ b/xen/arch/arm/include/asm/smccc.h
> @@ -122,7 +122,7 @@ struct arm_smccc_res {
>  #define __constraint_read_7 __constraint_read_6, "r" (r7)
>  
>  #define __declare_arg_0(a0, res)                            \
> -    struct arm_smccc_res    *___res = res;                  \
> +    struct arm_smccc_res    *___res = (res);                \
>      register unsigned long  r0 ASM_REG(0) = (uint32_t)a0;   \

Why res but not a0?

> --- a/xen/arch/arm/include/asm/vgic-emul.h
> +++ b/xen/arch/arm/include/asm/vgic-emul.h
> @@ -6,11 +6,11 @@
>   * a range of registers
>   */
>  
> -#define VREG32(reg) reg ... reg + 3
> -#define VREG64(reg) reg ... reg + 7
> +#define VREG32(reg) (reg) ... (reg) + 3
> +#define VREG64(reg) (reg) ... (reg) + 7

#define VREG32(reg) (reg) ... ((reg) + 3)
#define VREG64(reg) (reg) ... ((reg) + 7)

?

Jan

Re: [XEN PATCH 02/10] xen/arm: address some violations of MISRA C Rule 20.7

[Nicola Vetrini]

On 2024-02-29 17:34, Jan Beulich wrote:
> On 29.02.2024 16:27, Nicola Vetrini wrote:
>> --- a/xen/arch/arm/cpuerrata.c
>> +++ b/xen/arch/arm/cpuerrata.c
>> @@ -462,8 +462,8 @@ static bool has_ssbd_mitigation(const struct 
>> arm_cpu_capabilities *entry)
>>  #define MIDR_RANGE(model, min, max)     \
>>      .matches = is_affected_midr_range,  \
>>      .midr_model = model,                \
>> -    .midr_range_min = min,              \
>> -    .midr_range_max = max
>> +    .midr_range_min = (min),            \
>> +    .midr_range_max = (max)
> 
> Why min and max, but not model?
> 

All the constants in the full expansions are parenthesized via 
MIDR_CPU_MODEL, so it doesn't trigger any violation right now, but for 
consistency I'd better put parentheses there as well.

>> --- a/xen/arch/arm/include/asm/smccc.h
>> +++ b/xen/arch/arm/include/asm/smccc.h
>> @@ -122,7 +122,7 @@ struct arm_smccc_res {
>>  #define __constraint_read_7 __constraint_read_6, "r" (r7)
>> 
>>  #define __declare_arg_0(a0, res)                            \
>> -    struct arm_smccc_res    *___res = res;                  \
>> +    struct arm_smccc_res    *___res = (res);                \
>>      register unsigned long  r0 ASM_REG(0) = (uint32_t)a0;   \
> 
> Why res but not a0?
> 

Seems like it's never used in a non-compliant way, but you do have a 
point. Here and also below, to keep it consistent. I didn't look at all 
the violations yet, so I may have missed some. I did want to show a few 
patches also to gather opinions on what may/may not be accepted.

>> --- a/xen/arch/arm/include/asm/vgic-emul.h
>> +++ b/xen/arch/arm/include/asm/vgic-emul.h
>> @@ -6,11 +6,11 @@
>>   * a range of registers
>>   */
>> 
>> -#define VREG32(reg) reg ... reg + 3
>> -#define VREG64(reg) reg ... reg + 7
>> +#define VREG32(reg) (reg) ... (reg) + 3
>> +#define VREG64(reg) (reg) ... (reg) + 7
> 
> #define VREG32(reg) (reg) ... ((reg) + 3)
> #define VREG64(reg) (reg) ... ((reg) + 7)
> 
> ?
> 

The outer parentheses are not required, but I can add them if the 
maintainers share your view.

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

Re: [XEN PATCH 02/10] xen/arm: address some violations of MISRA C Rule 20.7

[Nicola Vetrini]

Hi,

as the maintainers of this subsystem, would you prefer Jan's version or 
the one in the patch?
Both are fine w.r.t MISRA Rule 20.7 because the macro arguments 
themselves are parenthesized.

>>> --- a/xen/arch/arm/include/asm/vgic-emul.h
>>> +++ b/xen/arch/arm/include/asm/vgic-emul.h
>>> @@ -6,11 +6,11 @@
>>>   * a range of registers
>>>   */
>>> 
>>> -#define VREG32(reg) reg ... reg + 3
>>> -#define VREG64(reg) reg ... reg + 7
>>> +#define VREG32(reg) (reg) ... (reg) + 3
>>> +#define VREG64(reg) (reg) ... (reg) + 7
>> 
>> #define VREG32(reg) (reg) ... ((reg) + 3)
>> #define VREG64(reg) (reg) ... ((reg) + 7)
>> 
>> ?
>> 
> 
> The outer parentheses are not required, but I can add them if the 
> maintainers share your view.

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

Re: [XEN PATCH 02/10] xen/arm: address some violations of MISRA C Rule 20.7

[Stefano Stabellini]

On Mon, 4 Mar 2024, Nicola Vetrini wrote:
> Hi,
> 
> as the maintainers of this subsystem, would you prefer Jan's version or the
> one in the patch?
> Both are fine w.r.t MISRA Rule 20.7 because the macro arguments themselves are
> parenthesized.

I prefer Jan's version. Thanks for asking Nicola.


> > > > --- a/xen/arch/arm/include/asm/vgic-emul.h
> > > > +++ b/xen/arch/arm/include/asm/vgic-emul.h
> > > > @@ -6,11 +6,11 @@
> > > >   * a range of registers
> > > >   */
> > > > 
> > > > -#define VREG32(reg) reg ... reg + 3
> > > > -#define VREG64(reg) reg ... reg + 7
> > > > +#define VREG32(reg) (reg) ... (reg) + 3
> > > > +#define VREG64(reg) (reg) ... (reg) + 7
> > > 
> > > #define VREG32(reg) (reg) ... ((reg) + 3)
> > > #define VREG64(reg) (reg) ... ((reg) + 7)
> > > 
> > > ?
> > > 
> > 
> > The outer parentheses are not required, but I can add them if the
> > maintainers share your view.

Re: [XEN PATCH 02/10] xen/arm: address some violations of MISRA C Rule 20.7

[Nicola Vetrini]

On 2024-03-05 02:43, Stefano Stabellini wrote:
> On Mon, 4 Mar 2024, Nicola Vetrini wrote:
>> Hi,
>> 
>> as the maintainers of this subsystem, would you prefer Jan's version 
>> or the
>> one in the patch?
>> Both are fine w.r.t MISRA Rule 20.7 because the macro arguments 
>> themselves are
>> parenthesized.
> 
> I prefer Jan's version. Thanks for asking Nicola.
> 

Ok, thanks. I'll modify it and keep an eye for possibly additional 
opinions of other maintainers.

> 
>> > > > --- a/xen/arch/arm/include/asm/vgic-emul.h
>> > > > +++ b/xen/arch/arm/include/asm/vgic-emul.h
>> > > > @@ -6,11 +6,11 @@
>> > > >   * a range of registers
>> > > >   */
>> > > >
>> > > > -#define VREG32(reg) reg ... reg + 3
>> > > > -#define VREG64(reg) reg ... reg + 7
>> > > > +#define VREG32(reg) (reg) ... (reg) + 3
>> > > > +#define VREG64(reg) (reg) ... (reg) + 7
>> > >
>> > > #define VREG32(reg) (reg) ... ((reg) + 3)
>> > > #define VREG64(reg) (reg) ... ((reg) + 7)
>> > >
>> > > ?
>> > >
>> >
>> > The outer parentheses are not required, but I can add them if the
>> > maintainers share your view.

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

[XEN PATCH 03/10] x86: address some violations of MISRA C Rule 20.7

[Nicola Vetrini]

MISRA C Rule 20.7 states: "Expressions resulting from the expansion
of macro parameters shall be enclosed in parentheses". Therefore, some
macro definitions should gain additional parentheses to ensure that all
current and future users will be safe with respect to expansions that
can possibly alter the semantics of the passed-in macro parameter.

GUARD(1) is also amended to avoid modifying UA_KEEP or its definition.

No functional change.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
I wasn't very sure whether touching the definition of UA_KEEP would be a good
idea, so I added parentheses in the only user I've seen so far that causes a
violation.
---
 xen/arch/x86/include/asm/irq.h | 6 +++---
 xen/arch/x86/usercopy.c        | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/include/asm/irq.h b/xen/arch/x86/include/asm/irq.h
index 082a3d6bbc6a..5c722848e8ce 100644
--- a/xen/arch/x86/include/asm/irq.h
+++ b/xen/arch/x86/include/asm/irq.h
@@ -179,9 +179,9 @@ void cleanup_domain_irq_mapping(struct domain *d);
     void *__ret = radix_tree_lookup(&(d)->arch.hvm.emuirq_pirq, emuirq);\
     __ret ? radix_tree_ptr_to_int(__ret) : IRQ_UNBOUND;                 \
 })
-#define IRQ_UNBOUND -1
-#define IRQ_PT -2
-#define IRQ_MSI_EMU -3
+#define IRQ_UNBOUND (-1)
+#define IRQ_PT      (-2)
+#define IRQ_MSI_EMU (-3)
 
 bool cpu_has_pending_apic_eoi(void);
 
diff --git a/xen/arch/x86/usercopy.c b/xen/arch/x86/usercopy.c
index b8c2d1cc0bed..b0b55398e968 100644
--- a/xen/arch/x86/usercopy.c
+++ b/xen/arch/x86/usercopy.c
@@ -106,7 +106,7 @@ unsigned int copy_from_guest_ll(void *to, const void __user *from, unsigned int
     return n;
 }
 
-#if GUARD(1) + 0
+#if GUARD((1)) + 0
 
 /**
  * copy_to_guest_pv: - Copy a block of data into PV guest space.
-- 
2.34.1

Re: [XEN PATCH 03/10] x86: address some violations of MISRA C Rule 20.7

[Jan Beulich]

On 29.02.2024 16:27, Nicola Vetrini wrote:
> --- a/xen/arch/x86/include/asm/irq.h
> +++ b/xen/arch/x86/include/asm/irq.h
> @@ -179,9 +179,9 @@ void cleanup_domain_irq_mapping(struct domain *d);
>      void *__ret = radix_tree_lookup(&(d)->arch.hvm.emuirq_pirq, emuirq);\
>      __ret ? radix_tree_ptr_to_int(__ret) : IRQ_UNBOUND;                 \
>  })
> -#define IRQ_UNBOUND -1
> -#define IRQ_PT -2
> -#define IRQ_MSI_EMU -3
> +#define IRQ_UNBOUND (-1)
> +#define IRQ_PT      (-2)
> +#define IRQ_MSI_EMU (-3)
>  
>  bool cpu_has_pending_apic_eoi(void);
>  

I'd be happy to ack this change right away.

> --- a/xen/arch/x86/usercopy.c
> +++ b/xen/arch/x86/usercopy.c
> @@ -106,7 +106,7 @@ unsigned int copy_from_guest_ll(void *to, const void __user *from, unsigned int
>      return n;
>  }
>  
> -#if GUARD(1) + 0
> +#if GUARD((1)) + 0

I don't even understand the need for this one, and nothing is said in
the description in that regard. Generally I'm afraid I'm averse to
such (seemingly) redundant parentheses in macro invocations.

Jan

Re: [XEN PATCH 03/10] x86: address some violations of MISRA C Rule 20.7

[Nicola Vetrini]

On 2024-02-29 17:37, Jan Beulich wrote:
> On 29.02.2024 16:27, Nicola Vetrini wrote:
>> --- a/xen/arch/x86/include/asm/irq.h
>> +++ b/xen/arch/x86/include/asm/irq.h
>> @@ -179,9 +179,9 @@ void cleanup_domain_irq_mapping(struct domain *d);
>>      void *__ret = radix_tree_lookup(&(d)->arch.hvm.emuirq_pirq, 
>> emuirq);\
>>      __ret ? radix_tree_ptr_to_int(__ret) : IRQ_UNBOUND;               
>>   \
>>  })
>> -#define IRQ_UNBOUND -1
>> -#define IRQ_PT -2
>> -#define IRQ_MSI_EMU -3
>> +#define IRQ_UNBOUND (-1)
>> +#define IRQ_PT      (-2)
>> +#define IRQ_MSI_EMU (-3)
>> 
>>  bool cpu_has_pending_apic_eoi(void);
>> 
> 
> I'd be happy to ack this change right away.
> 
>> --- a/xen/arch/x86/usercopy.c
>> +++ b/xen/arch/x86/usercopy.c
>> @@ -106,7 +106,7 @@ unsigned int copy_from_guest_ll(void *to, const 
>> void __user *from, unsigned int
>>      return n;
>>  }
>> 
>> -#if GUARD(1) + 0
>> +#if GUARD((1)) + 0
> 
> I don't even understand the need for this one, and nothing is said in
> the description in that regard. Generally I'm afraid I'm averse to
> such (seemingly) redundant parentheses in macro invocations.
> 

It's because
#define UA_KEEP(args...) args
#define GUARD UA_KEEP

which would expand to #if 1 + 0, while the rule demands #if (1) + 0
I did note in the message after --- that I didn't wanna touch UA_KEEP so 
I did this instead, which I'm not particularly happy about either. I can 
remove this and deviate, there is no other issue with GUARD.
-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

Re: [XEN PATCH 03/10] x86: address some violations of MISRA C Rule 20.7

[Jan Beulich]

On 29.02.2024 17:45, Nicola Vetrini wrote:
> On 2024-02-29 17:37, Jan Beulich wrote:
>> On 29.02.2024 16:27, Nicola Vetrini wrote:
>>> --- a/xen/arch/x86/include/asm/irq.h
>>> +++ b/xen/arch/x86/include/asm/irq.h
>>> @@ -179,9 +179,9 @@ void cleanup_domain_irq_mapping(struct domain *d);
>>>      void *__ret = radix_tree_lookup(&(d)->arch.hvm.emuirq_pirq, 
>>> emuirq);\
>>>      __ret ? radix_tree_ptr_to_int(__ret) : IRQ_UNBOUND;               
>>>   \
>>>  })
>>> -#define IRQ_UNBOUND -1
>>> -#define IRQ_PT -2
>>> -#define IRQ_MSI_EMU -3
>>> +#define IRQ_UNBOUND (-1)
>>> +#define IRQ_PT      (-2)
>>> +#define IRQ_MSI_EMU (-3)
>>>
>>>  bool cpu_has_pending_apic_eoi(void);
>>>
>>
>> I'd be happy to ack this change right away.
>>
>>> --- a/xen/arch/x86/usercopy.c
>>> +++ b/xen/arch/x86/usercopy.c
>>> @@ -106,7 +106,7 @@ unsigned int copy_from_guest_ll(void *to, const 
>>> void __user *from, unsigned int
>>>      return n;
>>>  }
>>>
>>> -#if GUARD(1) + 0
>>> +#if GUARD((1)) + 0
>>
>> I don't even understand the need for this one, and nothing is said in
>> the description in that regard. Generally I'm afraid I'm averse to
>> such (seemingly) redundant parentheses in macro invocations.
>>
> 
> It's because
> #define UA_KEEP(args...) args
> #define GUARD UA_KEEP
> 
> which would expand to #if 1 + 0, while the rule demands #if (1) + 0
> I did note in the message after --- that I didn't wanna touch UA_KEEP so 
> I did this instead, which I'm not particularly happy about either. I can 
> remove this and deviate, there is no other issue with GUARD.

Or

#if (GUARD(1) + 0)

? To me at least that's quite a bit less odd. But I guess that still
wouldn't satisfy the rule. Perhaps even

#if (GUARD(1)) + 0

would be a little less odd, albeit there I'd already be on the edge.

Jan

Re: [XEN PATCH 03/10] x86: address some violations of MISRA C Rule 20.7

[Nicola Vetrini]

On 2024-02-29 18:05, Jan Beulich wrote:
> On 29.02.2024 17:45, Nicola Vetrini wrote:
>> On 2024-02-29 17:37, Jan Beulich wrote:
>>> On 29.02.2024 16:27, Nicola Vetrini wrote:
>>>> --- a/xen/arch/x86/include/asm/irq.h
>>>> +++ b/xen/arch/x86/include/asm/irq.h
>>>> @@ -179,9 +179,9 @@ void cleanup_domain_irq_mapping(struct domain 
>>>> *d);
>>>>      void *__ret = radix_tree_lookup(&(d)->arch.hvm.emuirq_pirq,
>>>> emuirq);\
>>>>      __ret ? radix_tree_ptr_to_int(__ret) : IRQ_UNBOUND;
>>>>   \
>>>>  })
>>>> -#define IRQ_UNBOUND -1
>>>> -#define IRQ_PT -2
>>>> -#define IRQ_MSI_EMU -3
>>>> +#define IRQ_UNBOUND (-1)
>>>> +#define IRQ_PT      (-2)
>>>> +#define IRQ_MSI_EMU (-3)
>>>> 
>>>>  bool cpu_has_pending_apic_eoi(void);
>>>> 
>>> 
>>> I'd be happy to ack this change right away.
>>> 
>>>> --- a/xen/arch/x86/usercopy.c
>>>> +++ b/xen/arch/x86/usercopy.c
>>>> @@ -106,7 +106,7 @@ unsigned int copy_from_guest_ll(void *to, const
>>>> void __user *from, unsigned int
>>>>      return n;
>>>>  }
>>>> 
>>>> -#if GUARD(1) + 0
>>>> +#if GUARD((1)) + 0
>>> 
>>> I don't even understand the need for this one, and nothing is said in
>>> the description in that regard. Generally I'm afraid I'm averse to
>>> such (seemingly) redundant parentheses in macro invocations.
>>> 
>> 
>> It's because
>> #define UA_KEEP(args...) args
>> #define GUARD UA_KEEP
>> 
>> which would expand to #if 1 + 0, while the rule demands #if (1) + 0
>> I did note in the message after --- that I didn't wanna touch UA_KEEP 
>> so
>> I did this instead, which I'm not particularly happy about either. I 
>> can
>> remove this and deviate, there is no other issue with GUARD.
> 
> Or
> 
> #if (GUARD(1) + 0)
> 
> ? To me at least that's quite a bit less odd. But I guess that still
> wouldn't satisfy the rule. Perhaps even
> 
> #if (GUARD(1)) + 0
> 
> would be a little less odd, albeit there I'd already be on the edge.
> 

Sorry for the late reply. I'll split this in v2. Solution #2 seems ok at 
first glance.

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

[XEN PATCH 04/10] xen/public: address violations of MISRA C Rule 20.7

[Nicola Vetrini]

MISRA C Rule 20.7 states: "Expressions resulting from the expansion
of macro parameters shall be enclosed in parentheses". Therefore
the macro XEN_DEFINE_UUID_ should wrap its parameters in parentheses.

No functional change.

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

diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h
index b47d48d0e2d6..fa23080bd7af 100644
--- a/xen/include/public/xen.h
+++ b/xen/include/public/xen.h
@@ -988,7 +988,7 @@ typedef struct {
       ((b) >>  8) & 0xFF, ((b) >>  0) & 0xFF,                           \
       ((c) >>  8) & 0xFF, ((c) >>  0) & 0xFF,                           \
       ((d) >>  8) & 0xFF, ((d) >>  0) & 0xFF,                           \
-                e1, e2, e3, e4, e5, e6}}
+                (e1), (e2), (e3), (e4), (e5), (e6)}}
 
 #if defined(__STDC_VERSION__) ? __STDC_VERSION__ >= 199901L : defined(__GNUC__)
 #define XEN_DEFINE_UUID(a, b, c, d, e1, e2, e3, e4, e5, e6)             \
-- 
2.34.1

Re: [XEN PATCH 04/10] xen/public: address violations of MISRA C Rule 20.7

[Jan Beulich]

On 29.02.2024 16:27, Nicola Vetrini wrote:
> --- a/xen/include/public/xen.h
> +++ b/xen/include/public/xen.h
> @@ -988,7 +988,7 @@ typedef struct {
>        ((b) >>  8) & 0xFF, ((b) >>  0) & 0xFF,                           \
>        ((c) >>  8) & 0xFF, ((c) >>  0) & 0xFF,                           \
>        ((d) >>  8) & 0xFF, ((d) >>  0) & 0xFF,                           \
> -                e1, e2, e3, e4, e5, e6}}
> +                (e1), (e2), (e3), (e4), (e5), (e6)}}

Why? Wasn't it agreed already that long macro arguments passed on
(no matter whether to a function, a macro, or like used here) don't
need parenthesizing?

Jan

Re: [XEN PATCH 04/10] xen/public: address violations of MISRA C Rule 20.7

[Nicola Vetrini]

On 2024-02-29 17:40, Jan Beulich wrote:
> On 29.02.2024 16:27, Nicola Vetrini wrote:
>> --- a/xen/include/public/xen.h
>> +++ b/xen/include/public/xen.h
>> @@ -988,7 +988,7 @@ typedef struct {
>>        ((b) >>  8) & 0xFF, ((b) >>  0) & 0xFF,                         
>>   \
>>        ((c) >>  8) & 0xFF, ((c) >>  0) & 0xFF,                         
>>   \
>>        ((d) >>  8) & 0xFF, ((d) >>  0) & 0xFF,                         
>>   \
>> -                e1, e2, e3, e4, e5, e6}}
>> +                (e1), (e2), (e3), (e4), (e5), (e6)}}
> 
> Why? Wasn't it agreed already that long macro arguments passed on
> (no matter whether to a function, a macro, or like used here) don't
> need parenthesizing?
> 

That applies to all outermost macro invocations, but not to the 
innermost one. If you want also aggregate initalizers to be deviated, 
that could be done (provided that the macro arg is not included in some 
expression, such as "{..., e1 + 1, ...}"

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

Re: [XEN PATCH 04/10] xen/public: address violations of MISRA C Rule 20.7

[Stefano Stabellini]

On Thu, 29 Feb 2024, Nicola Vetrini wrote:
> On 2024-02-29 17:40, Jan Beulich wrote:
> > On 29.02.2024 16:27, Nicola Vetrini wrote:
> > > --- a/xen/include/public/xen.h
> > > +++ b/xen/include/public/xen.h
> > > @@ -988,7 +988,7 @@ typedef struct {
> > >        ((b) >>  8) & 0xFF, ((b) >>  0) & 0xFF,                           \
> > >        ((c) >>  8) & 0xFF, ((c) >>  0) & 0xFF,                           \
> > >        ((d) >>  8) & 0xFF, ((d) >>  0) & 0xFF,                           \
> > > -                e1, e2, e3, e4, e5, e6}}
> > > +                (e1), (e2), (e3), (e4), (e5), (e6)}}
> > 
> > Why? Wasn't it agreed already that long macro arguments passed on
> > (no matter whether to a function, a macro, or like used here) don't
> > need parenthesizing?
> > 
> 
> That applies to all outermost macro invocations, but not to the innermost one.

I don't understand what you mean. Maybe a couple of trivial examples
would help.


> If you want also aggregate initalizers to be deviated, that could be done
> (provided that the macro arg is not included in some expression, such as
> "{..., e1 + 1, ...}"

My gut feeling tells me that probably this is what we want but I'd
rather first understand exactly what you meant above

Re: [XEN PATCH 04/10] xen/public: address violations of MISRA C Rule 20.7

[Nicola Vetrini]

On 2024-02-29 23:49, Stefano Stabellini wrote:
> On Thu, 29 Feb 2024, Nicola Vetrini wrote:
>> On 2024-02-29 17:40, Jan Beulich wrote:
>> > On 29.02.2024 16:27, Nicola Vetrini wrote:
>> > > --- a/xen/include/public/xen.h
>> > > +++ b/xen/include/public/xen.h
>> > > @@ -988,7 +988,7 @@ typedef struct {
>> > >        ((b) >>  8) & 0xFF, ((b) >>  0) & 0xFF,                           \
>> > >        ((c) >>  8) & 0xFF, ((c) >>  0) & 0xFF,                           \
>> > >        ((d) >>  8) & 0xFF, ((d) >>  0) & 0xFF,                           \
>> > > -                e1, e2, e3, e4, e5, e6}}
>> > > +                (e1), (e2), (e3), (e4), (e5), (e6)}}
>> >
>> > Why? Wasn't it agreed already that long macro arguments passed on
>> > (no matter whether to a function, a macro, or like used here) don't
>> > need parenthesizing?
>> >
>> 
>> That applies to all outermost macro invocations, but not to the 
>> innermost one.
> 
> I don't understand what you mean. Maybe a couple of trivial examples
> would help.
> 
> 
>> If you want also aggregate initalizers to be deviated, that could be 
>> done
>> (provided that the macro arg is not included in some expression, such 
>> as
>> "{..., e1 + 1, ...}"
> 

Sorry for the late reply. This is the current state:

#define N(x) somestruct var = {..., x, ...}; // <- not deviated, 
violation here
#define M(x) N(x) // <- deviated, no violation here

...

M(0xff);

The violation is resolved by {..., (x), ...} or by saying that when "x" 
is a whole expression in its fully expanded form, then we allow it not 
to be needlessly parenthesized, as Jan requested (unless I misunderstood 
his reply). In that case, the only this that would still give a 
violation in the above setting is questionable patterns such as

#define Q(x) x, x



> My gut feeling tells me that probably this is what we want but I'd
> rather first understand exactly what you meant above

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

Re: [XEN PATCH 04/10] xen/public: address violations of MISRA C Rule 20.7

[Jan Beulich]

On 05.03.2024 11:21, Nicola Vetrini wrote:
> On 2024-02-29 23:49, Stefano Stabellini wrote:
>> On Thu, 29 Feb 2024, Nicola Vetrini wrote:
>>> On 2024-02-29 17:40, Jan Beulich wrote:
>>>> On 29.02.2024 16:27, Nicola Vetrini wrote:
>>>>> --- a/xen/include/public/xen.h
>>>>> +++ b/xen/include/public/xen.h
>>>>> @@ -988,7 +988,7 @@ typedef struct {
>>>>>        ((b) >>  8) & 0xFF, ((b) >>  0) & 0xFF,                           \
>>>>>        ((c) >>  8) & 0xFF, ((c) >>  0) & 0xFF,                           \
>>>>>        ((d) >>  8) & 0xFF, ((d) >>  0) & 0xFF,                           \
>>>>> -                e1, e2, e3, e4, e5, e6}}
>>>>> +                (e1), (e2), (e3), (e4), (e5), (e6)}}
>>>>
>>>> Why? Wasn't it agreed already that long macro arguments passed on
>>>> (no matter whether to a function, a macro, or like used here) don't
>>>> need parenthesizing?
>>>>
>>>
>>> That applies to all outermost macro invocations, but not to the 
>>> innermost one.
>>
>> I don't understand what you mean. Maybe a couple of trivial examples
>> would help.
>>
>>
>>> If you want also aggregate initalizers to be deviated, that could be 
>>> done
>>> (provided that the macro arg is not included in some expression, such 
>>> as
>>> "{..., e1 + 1, ...}"
>>
> 
> Sorry for the late reply. This is the current state:
> 
> #define N(x) somestruct var = {..., x, ...}; // <- not deviated, 
> violation here
> #define M(x) N(x) // <- deviated, no violation here
> 
> ...
> 
> M(0xff);
> 
> The violation is resolved by {..., (x), ...} or by saying that when "x" 
> is a whole expression in its fully expanded form, then we allow it not 
> to be needlessly parenthesized, as Jan requested (unless I misunderstood 
> his reply).

Well, the thing I continue to have trouble with is "fully expanded form".
That's not the criteria I'd like to see applied. To me all depends on how
the macro is written, not what uses of the macro expand to.

> In that case, the only this that would still give a 
> violation in the above setting is questionable patterns such as
> 
> #define Q(x) x, x

Right.

#define Q(x) (x, x)

ought to be okay though, rule-wise, no matter that it's questionable too.

Jan

Re: [XEN PATCH 04/10] xen/public: address violations of MISRA C Rule 20.7

[Nicola Vetrini]

On 2024-03-05 11:26, Jan Beulich wrote:
> On 05.03.2024 11:21, Nicola Vetrini wrote:
>> On 2024-02-29 23:49, Stefano Stabellini wrote:
>>> On Thu, 29 Feb 2024, Nicola Vetrini wrote:
>>>> On 2024-02-29 17:40, Jan Beulich wrote:
>>>>> On 29.02.2024 16:27, Nicola Vetrini wrote:
>>>>>> --- a/xen/include/public/xen.h
>>>>>> +++ b/xen/include/public/xen.h
>>>>>> @@ -988,7 +988,7 @@ typedef struct {
>>>>>>        ((b) >>  8) & 0xFF, ((b) >>  0) & 0xFF,                     
>>>>>>       \
>>>>>>        ((c) >>  8) & 0xFF, ((c) >>  0) & 0xFF,                     
>>>>>>       \
>>>>>>        ((d) >>  8) & 0xFF, ((d) >>  0) & 0xFF,                     
>>>>>>       \
>>>>>> -                e1, e2, e3, e4, e5, e6}}
>>>>>> +                (e1), (e2), (e3), (e4), (e5), (e6)}}
>>>>> 
>>>>> Why? Wasn't it agreed already that long macro arguments passed on
>>>>> (no matter whether to a function, a macro, or like used here) don't
>>>>> need parenthesizing?
>>>>> 
>>>> 
>>>> That applies to all outermost macro invocations, but not to the
>>>> innermost one.
>>> 
>>> I don't understand what you mean. Maybe a couple of trivial examples
>>> would help.
>>> 
>>> 
>>>> If you want also aggregate initalizers to be deviated, that could be
>>>> done
>>>> (provided that the macro arg is not included in some expression, 
>>>> such
>>>> as
>>>> "{..., e1 + 1, ...}"
>>> 
>> 
>> Sorry for the late reply. This is the current state:
>> 
>> #define N(x) somestruct var = {..., x, ...}; // <- not deviated,
>> violation here
>> #define M(x) N(x) // <- deviated, no violation here
>> 
>> ...
>> 
>> M(0xff);
>> 
>> The violation is resolved by {..., (x), ...} or by saying that when 
>> "x"
>> is a whole expression in its fully expanded form, then we allow it not
>> to be needlessly parenthesized, as Jan requested (unless I 
>> misunderstood
>> his reply).
> 
> Well, the thing I continue to have trouble with is "fully expanded 
> form".
> That's not the criteria I'd like to see applied. To me all depends on 
> how
> the macro is written, not what uses of the macro expand to.
> 

Sure.

>> In that case, the only this that would still give a
>> violation in the above setting is questionable patterns such as
>> 
>> #define Q(x) x, x
> 
> Right.
> 
> #define Q(x) (x, x)
> 
> ought to be okay though, rule-wise, no matter that it's questionable 
> too.
> 
> Jan

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

Re: [XEN PATCH 04/10] xen/public: address violations of MISRA C Rule 20.7

[Jan Beulich]

On 29.02.2024 17:49, Nicola Vetrini wrote:
> On 2024-02-29 17:40, Jan Beulich wrote:
>> On 29.02.2024 16:27, Nicola Vetrini wrote:
>>> --- a/xen/include/public/xen.h
>>> +++ b/xen/include/public/xen.h
>>> @@ -988,7 +988,7 @@ typedef struct {
>>>        ((b) >>  8) & 0xFF, ((b) >>  0) & 0xFF,                         
>>>   \
>>>        ((c) >>  8) & 0xFF, ((c) >>  0) & 0xFF,                         
>>>   \
>>>        ((d) >>  8) & 0xFF, ((d) >>  0) & 0xFF,                         
>>>   \
>>> -                e1, e2, e3, e4, e5, e6}}
>>> +                (e1), (e2), (e3), (e4), (e5), (e6)}}
>>
>> Why? Wasn't it agreed already that long macro arguments passed on
>> (no matter whether to a function, a macro, or like used here) don't
>> need parenthesizing?
>>
> 
> That applies to all outermost macro invocations, but not to the 
> innermost one. If you want also aggregate initalizers to be deviated, 
> that could be done (provided that the macro arg is not included in some 
> expression, such as "{..., e1 + 1, ...}"

Sure, this obviously needs to be "{..., (e1) + 1, ...}" then. But yes,
the full scope of the underlying pattern ought to be excluded from
needing (pointless) parentheses added. Even in a plain comma expression
you don't need them - to pass in a comma expression the invocation site
of such a macro would then need to parenthesize the respective operand
(or else it would be two separate operands).

The one case we're leaving aside here anyway is use of odd things like

#define M a, b

and then passing M into another macro.

Jan

[XEN PATCH 05/10] xen/perfc: address violations of MISRA C Rule 20.7

[Nicola Vetrini]

MISRA C Rule 20.7 states: "Expressions resulting from the expansion
of macro parameters shall be enclosed in parentheses". Therefore, some
macro definitions should gain additional parentheses to ensure that all
current and future users will be safe with respect to expansions that
can possibly alter the semantics of the passed-in macro parameter.

No functional change.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
 xen/common/perfc.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/xen/common/perfc.c b/xen/common/perfc.c
index 7400667bf0c4..02f4fc8fe032 100644
--- a/xen/common/perfc.c
+++ b/xen/common/perfc.c
@@ -10,10 +10,10 @@
 #include <public/sysctl.h>
 #include <asm/perfc.h>
 
-#define PERFCOUNTER( var, name )              { name, TYPE_SINGLE, 0 },
-#define PERFCOUNTER_ARRAY( var, name, size )  { name, TYPE_ARRAY,  size },
-#define PERFSTATUS( var, name )               { name, TYPE_S_SINGLE, 0 },
-#define PERFSTATUS_ARRAY( var, name, size )   { name, TYPE_S_ARRAY,  size },
+#define PERFCOUNTER( var, name )              { (name), TYPE_SINGLE, 0 },
+#define PERFCOUNTER_ARRAY( var, name, size )  { (name), TYPE_ARRAY,  (size) },
+#define PERFSTATUS( var, name )               { (name), TYPE_S_SINGLE, 0 },
+#define PERFSTATUS_ARRAY( var, name, size )   { (name), TYPE_S_ARRAY,  (size) },
 static const struct {
     const char *name;
     enum { TYPE_SINGLE, TYPE_ARRAY,
-- 
2.34.1

Re: [XEN PATCH 05/10] xen/perfc: address violations of MISRA C Rule 20.7

[Jan Beulich]

On 29.02.2024 16:27, Nicola Vetrini wrote:
> --- a/xen/common/perfc.c
> +++ b/xen/common/perfc.c
> @@ -10,10 +10,10 @@
>  #include <public/sysctl.h>
>  #include <asm/perfc.h>
>  
> -#define PERFCOUNTER( var, name )              { name, TYPE_SINGLE, 0 },
> -#define PERFCOUNTER_ARRAY( var, name, size )  { name, TYPE_ARRAY,  size },
> -#define PERFSTATUS( var, name )               { name, TYPE_S_SINGLE, 0 },
> -#define PERFSTATUS_ARRAY( var, name, size )   { name, TYPE_S_ARRAY,  size },
> +#define PERFCOUNTER( var, name )              { (name), TYPE_SINGLE, 0 },
> +#define PERFCOUNTER_ARRAY( var, name, size )  { (name), TYPE_ARRAY,  (size) },
> +#define PERFSTATUS( var, name )               { (name), TYPE_S_SINGLE, 0 },
> +#define PERFSTATUS_ARRAY( var, name, size )   { (name), TYPE_S_ARRAY,  (size) },

Same question as for patch 4. Plus if this needed touching, then the stray
blanks immediately inside the parentheses would please be dropped as well.

Jan

Re: [XEN PATCH 05/10] xen/perfc: address violations of MISRA C Rule 20.7

[Nicola Vetrini]

On 2024-02-29 17:42, Jan Beulich wrote:
> On 29.02.2024 16:27, Nicola Vetrini wrote:
>> --- a/xen/common/perfc.c
>> +++ b/xen/common/perfc.c
>> @@ -10,10 +10,10 @@
>>  #include <public/sysctl.h>
>>  #include <asm/perfc.h>
>> 
>> -#define PERFCOUNTER( var, name )              { name, TYPE_SINGLE, 0 
>> },
>> -#define PERFCOUNTER_ARRAY( var, name, size )  { name, TYPE_ARRAY,  
>> size },
>> -#define PERFSTATUS( var, name )               { name, TYPE_S_SINGLE, 
>> 0 },
>> -#define PERFSTATUS_ARRAY( var, name, size )   { name, TYPE_S_ARRAY,  
>> size },
>> +#define PERFCOUNTER( var, name )              { (name), TYPE_SINGLE, 
>> 0 },
>> +#define PERFCOUNTER_ARRAY( var, name, size )  { (name), TYPE_ARRAY,  
>> (size) },
>> +#define PERFSTATUS( var, name )               { (name), 
>> TYPE_S_SINGLE, 0 },
>> +#define PERFSTATUS_ARRAY( var, name, size )   { (name), TYPE_S_ARRAY, 
>>  (size) },
> 
> Same question as for patch 4. Plus if this needed touching, then the 
> stray
> blanks immediately inside the parentheses would please be dropped as 
> well.
> 

Noted

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

[XEN PATCH 06/10] arm/smmu: address some violations of MISRA C Rule 20.7

[Nicola Vetrini]

MISRA C Rule 20.7 states: "Expressions resulting from the expansion
of macro parameters shall be enclosed in parentheses". Therefore, some
macro definitions should gain additional parentheses to ensure that all
current and future users will be safe with respect to expansions that
can possibly alter the semantics of the passed-in macro parameter.

No functional change.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
 xen/drivers/passthrough/arm/smmu.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
index 625ed0e41961..83196057a937 100644
--- a/xen/drivers/passthrough/arm/smmu.c
+++ b/xen/drivers/passthrough/arm/smmu.c
@@ -242,7 +242,7 @@ struct arm_smmu_xen_device {
 	struct iommu_group *group;
 };
 
-#define dev_archdata(dev) ((struct arm_smmu_xen_device *)dev->iommu)
+#define dev_archdata(dev) ((struct arm_smmu_xen_device *)(dev)->iommu)
 #define dev_iommu_domain(dev) (dev_archdata(dev)->domain)
 #define dev_iommu_group(dev) (dev_archdata(dev)->group)
 
@@ -627,7 +627,7 @@ struct arm_smmu_master_cfg {
 };
 #define INVALID_SMENDX			-1
 #define for_each_cfg_sme(cfg, i, idx, num) \
-	for (i = 0; idx = cfg->smendx[i], i < num; ++i)
+	for (i = 0; idx = (cfg)->smendx[i], (i) < (num); ++(i))
 
 struct arm_smmu_master {
 	struct device_node		*of_node;
-- 
2.34.1

Re: [XEN PATCH 06/10] arm/smmu: address some violations of MISRA C Rule 20.7

[Stefano Stabellini]

On Thu, 29 Feb 2024, Nicola Vetrini wrote:
> MISRA C Rule 20.7 states: "Expressions resulting from the expansion
> of macro parameters shall be enclosed in parentheses". Therefore, some
> macro definitions should gain additional parentheses to ensure that all
> current and future users will be safe with respect to expansions that
> can possibly alter the semantics of the passed-in macro parameter.
> 
> No functional change.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> ---
>  xen/drivers/passthrough/arm/smmu.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
> index 625ed0e41961..83196057a937 100644
> --- a/xen/drivers/passthrough/arm/smmu.c
> +++ b/xen/drivers/passthrough/arm/smmu.c
> @@ -242,7 +242,7 @@ struct arm_smmu_xen_device {
>  	struct iommu_group *group;
>  };
>  
> -#define dev_archdata(dev) ((struct arm_smmu_xen_device *)dev->iommu)
> +#define dev_archdata(dev) ((struct arm_smmu_xen_device *)(dev)->iommu)
>  #define dev_iommu_domain(dev) (dev_archdata(dev)->domain)
>  #define dev_iommu_group(dev) (dev_archdata(dev)->group)

this is OK


> @@ -627,7 +627,7 @@ struct arm_smmu_master_cfg {
>  };
>  #define INVALID_SMENDX			-1
>  #define for_each_cfg_sme(cfg, i, idx, num) \
> -	for (i = 0; idx = cfg->smendx[i], i < num; ++i)
> +	for (i = 0; idx = (cfg)->smendx[i], (i) < (num); ++(i))

The first i = 0 is missing parentheses?
Also idx misses parentheses?

Re: [XEN PATCH 06/10] arm/smmu: address some violations of MISRA C Rule 20.7

[Nicola Vetrini]

On 2024-02-29 23:53, Stefano Stabellini wrote:
> On Thu, 29 Feb 2024, Nicola Vetrini wrote:
>> MISRA C Rule 20.7 states: "Expressions resulting from the expansion
>> of macro parameters shall be enclosed in parentheses". Therefore, some
>> macro definitions should gain additional parentheses to ensure that 
>> all
>> current and future users will be safe with respect to expansions that
>> can possibly alter the semantics of the passed-in macro parameter.
>> 
>> No functional change.
>> 
>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>> ---
>>  xen/drivers/passthrough/arm/smmu.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>> 
>> diff --git a/xen/drivers/passthrough/arm/smmu.c 
>> b/xen/drivers/passthrough/arm/smmu.c
>> index 625ed0e41961..83196057a937 100644
>> --- a/xen/drivers/passthrough/arm/smmu.c
>> +++ b/xen/drivers/passthrough/arm/smmu.c
>> @@ -242,7 +242,7 @@ struct arm_smmu_xen_device {
>>  	struct iommu_group *group;
>>  };
>> 
>> -#define dev_archdata(dev) ((struct arm_smmu_xen_device *)dev->iommu)
>> +#define dev_archdata(dev) ((struct arm_smmu_xen_device 
>> *)(dev)->iommu)
>>  #define dev_iommu_domain(dev) (dev_archdata(dev)->domain)
>>  #define dev_iommu_group(dev) (dev_archdata(dev)->group)
> 
> this is OK
> 
> 
>> @@ -627,7 +627,7 @@ struct arm_smmu_master_cfg {
>>  };
>>  #define INVALID_SMENDX			-1
>>  #define for_each_cfg_sme(cfg, i, idx, num) \
>> -	for (i = 0; idx = cfg->smendx[i], i < num; ++i)
>> +	for (i = 0; idx = (cfg)->smendx[i], (i) < (num); ++(i))
> 
> The first i = 0 is missing parentheses?
> Also idx misses parentheses?

This is another case where the parentheses around the lhs are deviated 
currently. It's up to the maintainers to decide whether to add them 
regardless of that for consistency, or to keep it as is.

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

Re: [XEN PATCH 06/10] arm/smmu: address some violations of MISRA C Rule 20.7

[Stefano Stabellini]

On Thu, 29 Feb 2024, Nicola Vetrini wrote:
> MISRA C Rule 20.7 states: "Expressions resulting from the expansion
> of macro parameters shall be enclosed in parentheses". Therefore, some
> macro definitions should gain additional parentheses to ensure that all
> current and future users will be safe with respect to expansions that
> can possibly alter the semantics of the passed-in macro parameter.
> 
> No functional change.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>

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


> ---
>  xen/drivers/passthrough/arm/smmu.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
> index 625ed0e41961..83196057a937 100644
> --- a/xen/drivers/passthrough/arm/smmu.c
> +++ b/xen/drivers/passthrough/arm/smmu.c
> @@ -242,7 +242,7 @@ struct arm_smmu_xen_device {
>  	struct iommu_group *group;
>  };
>  
> -#define dev_archdata(dev) ((struct arm_smmu_xen_device *)dev->iommu)
> +#define dev_archdata(dev) ((struct arm_smmu_xen_device *)(dev)->iommu)
>  #define dev_iommu_domain(dev) (dev_archdata(dev)->domain)
>  #define dev_iommu_group(dev) (dev_archdata(dev)->group)
>  
> @@ -627,7 +627,7 @@ struct arm_smmu_master_cfg {
>  };
>  #define INVALID_SMENDX			-1
>  #define for_each_cfg_sme(cfg, i, idx, num) \
> -	for (i = 0; idx = cfg->smendx[i], i < num; ++i)
> +	for (i = 0; idx = (cfg)->smendx[i], (i) < (num); ++(i))
>  
>  struct arm_smmu_master {
>  	struct device_node		*of_node;
> -- 
> 2.34.1
> 

[XEN PATCH 07/10] xen/arm: smmuv3: address violations of MISRA C Rule 20.7

[Nicola Vetrini]

MISRA C Rule 20.7 states: "Expressions resulting from the expansion
of macro parameters shall be enclosed in parentheses". Therefore, some
macro definitions should gain additional parentheses to ensure that all
current and future users will be safe with respect to expansions that
can possibly alter the semantics of the passed-in macro parameter.

No functional change.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
 xen/drivers/passthrough/arm/smmu-v3.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/drivers/passthrough/arm/smmu-v3.c b/xen/drivers/passthrough/arm/smmu-v3.c
index c3ac6d17d1c8..b1c40c2c0ae7 100644
--- a/xen/drivers/passthrough/arm/smmu-v3.c
+++ b/xen/drivers/passthrough/arm/smmu-v3.c
@@ -111,7 +111,7 @@
 #define GFP_KERNEL		0
 
 /* Device logger functions */
-#define dev_name(dev)	dt_node_full_name(dev->of_node)
+#define dev_name(dev)	dt_node_full_name((dev)->of_node)
 #define dev_dbg(dev, fmt, ...)			\
 	printk(XENLOG_DEBUG "SMMUv3: %s: " fmt, dev_name(dev), ## __VA_ARGS__)
 #define dev_notice(dev, fmt, ...)		\
-- 
2.34.1

Re: [XEN PATCH 07/10] xen/arm: smmuv3: address violations of MISRA C Rule 20.7

[Stefano Stabellini]

On Thu, 29 Feb 2024, Nicola Vetrini wrote:
> MISRA C Rule 20.7 states: "Expressions resulting from the expansion
> of macro parameters shall be enclosed in parentheses". Therefore, some
> macro definitions should gain additional parentheses to ensure that all
> current and future users will be safe with respect to expansions that
> can possibly alter the semantics of the passed-in macro parameter.
> 
> No functional change.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>

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


> ---
>  xen/drivers/passthrough/arm/smmu-v3.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/drivers/passthrough/arm/smmu-v3.c b/xen/drivers/passthrough/arm/smmu-v3.c
> index c3ac6d17d1c8..b1c40c2c0ae7 100644
> --- a/xen/drivers/passthrough/arm/smmu-v3.c
> +++ b/xen/drivers/passthrough/arm/smmu-v3.c
> @@ -111,7 +111,7 @@
>  #define GFP_KERNEL		0
>  
>  /* Device logger functions */
> -#define dev_name(dev)	dt_node_full_name(dev->of_node)
> +#define dev_name(dev)	dt_node_full_name((dev)->of_node)
>  #define dev_dbg(dev, fmt, ...)			\
>  	printk(XENLOG_DEBUG "SMMUv3: %s: " fmt, dev_name(dev), ## __VA_ARGS__)
>  #define dev_notice(dev, fmt, ...)		\
> -- 
> 2.34.1
> 

[XEN PATCH 08/10] xen/errno: address violations of MISRA C Rule 20.7

[Nicola Vetrini]

MISRA C Rule 20.7 states: "Expressions resulting from the expansion
of macro parameters shall be enclosed in parentheses". Therefore, some
macro definitions should gain additional parentheses to ensure that all
current and future users will be safe with respect to expansions that
can possibly alter the semantics of the passed-in macro parameter.

No functional change.

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

diff --git a/xen/include/xen/errno.h b/xen/include/xen/errno.h
index 69b28dd3c6c5..506674701fae 100644
--- a/xen/include/xen/errno.h
+++ b/xen/include/xen/errno.h
@@ -3,7 +3,7 @@
 
 #ifndef __ASSEMBLY__
 
-#define XEN_ERRNO(name, value) name = value,
+#define XEN_ERRNO(name, value) name = (value),
 enum {
 #include <public/errno.h>
 };
-- 
2.34.1

Re: [XEN PATCH 08/10] xen/errno: address violations of MISRA C Rule 20.7

[Stefano Stabellini]

On Thu, 29 Feb 2024, Nicola Vetrini wrote:
> MISRA C Rule 20.7 states: "Expressions resulting from the expansion
> of macro parameters shall be enclosed in parentheses". Therefore, some
> macro definitions should gain additional parentheses to ensure that all
> current and future users will be safe with respect to expansions that
> can possibly alter the semantics of the passed-in macro parameter.
> 
> No functional change.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> ---
>  xen/include/xen/errno.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/include/xen/errno.h b/xen/include/xen/errno.h
> index 69b28dd3c6c5..506674701fae 100644
> --- a/xen/include/xen/errno.h
> +++ b/xen/include/xen/errno.h
> @@ -3,7 +3,7 @@
>  
>  #ifndef __ASSEMBLY__
>  
> -#define XEN_ERRNO(name, value) name = value,
> +#define XEN_ERRNO(name, value) name = (value),

I see this and the fact that "name" was not parenthesized and it would
deliberate right? So I guess the left side of an assignment doesn't need
parenthesis?

Re: [XEN PATCH 08/10] xen/errno: address violations of MISRA C Rule 20.7

[Nicola Vetrini]

On 2024-02-29 23:55, Stefano Stabellini wrote:
> On Thu, 29 Feb 2024, Nicola Vetrini wrote:
>> MISRA C Rule 20.7 states: "Expressions resulting from the expansion
>> of macro parameters shall be enclosed in parentheses". Therefore, some
>> macro definitions should gain additional parentheses to ensure that 
>> all
>> current and future users will be safe with respect to expansions that
>> can possibly alter the semantics of the passed-in macro parameter.
>> 
>> No functional change.
>> 
>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>> ---
>>  xen/include/xen/errno.h | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/xen/include/xen/errno.h b/xen/include/xen/errno.h
>> index 69b28dd3c6c5..506674701fae 100644
>> --- a/xen/include/xen/errno.h
>> +++ b/xen/include/xen/errno.h
>> @@ -3,7 +3,7 @@
>> 
>>  #ifndef __ASSEMBLY__
>> 
>> -#define XEN_ERRNO(name, value) name = value,
>> +#define XEN_ERRNO(name, value) name = (value),
> 
> I see this and the fact that "name" was not parenthesized and it would
> deliberate right? So I guess the left side of an assignment doesn't 
> need
> parenthesis?

Exactly. Quoting from rules.rst:
Extra parentheses are not required when macro parameters are used
as function arguments, as macro arguments, array indices, lhs in
assignments

so, as noted in earlier discussions in this series, it could either be 
said that the rhs consisting of a single expression is similarly ok, or 
have (name) = (value) for consistency.
Do note that a considerable percentage of violations are caused by these 
definitions (PERFCOUNTER, XEN_ERRNO and probably others in x86).

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

Re: [XEN PATCH 08/10] xen/errno: address violations of MISRA C Rule 20.7

[Jan Beulich]

On 29.02.2024 16:28, Nicola Vetrini wrote:
> MISRA C Rule 20.7 states: "Expressions resulting from the expansion
> of macro parameters shall be enclosed in parentheses". Therefore, some
> macro definitions should gain additional parentheses to ensure that all
> current and future users will be safe with respect to expansions that
> can possibly alter the semantics of the passed-in macro parameter.
> 
> No functional change.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>

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

[XEN PATCH 09/10] xen/include: tasklet: address violations of MISRA C Rule 20.7

[Nicola Vetrini]

MISRA C Rule 20.7 states: "Expressions resulting from the expansion
of macro parameters shall be enclosed in parentheses". Therefore, some
macro definitions should gain additional parentheses to ensure that all
current and future users will be safe with respect to expansions that
can possibly alter the semantics of the passed-in macro parameter.

No functional change.

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

diff --git a/xen/include/xen/tasklet.h b/xen/include/xen/tasklet.h
index 593d6a2400fb..78760b694a39 100644
--- a/xen/include/xen/tasklet.h
+++ b/xen/include/xen/tasklet.h
@@ -27,7 +27,7 @@ struct tasklet
 
 #define _DECLARE_TASKLET(name, func, data, softirq)                     \
     struct tasklet name = {                                             \
-        LIST_HEAD_INIT(name.list), -1, softirq, 0, 0, func, data }
+        LIST_HEAD_INIT((name).list), -1, softirq, 0, 0, func, data }
 #define DECLARE_TASKLET(name, func, data)               \
     _DECLARE_TASKLET(name, func, data, 0)
 #define DECLARE_SOFTIRQ_TASKLET(name, func, data)       \
-- 
2.34.1

Re: [XEN PATCH 09/10] xen/include: tasklet: address violations of MISRA C Rule 20.7

[Stefano Stabellini]

On Thu, 29 Feb 2024, Nicola Vetrini wrote:
> MISRA C Rule 20.7 states: "Expressions resulting from the expansion
> of macro parameters shall be enclosed in parentheses". Therefore, some
> macro definitions should gain additional parentheses to ensure that all
> current and future users will be safe with respect to expansions that
> can possibly alter the semantics of the passed-in macro parameter.
> 
> No functional change.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> ---
>  xen/include/xen/tasklet.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/include/xen/tasklet.h b/xen/include/xen/tasklet.h
> index 593d6a2400fb..78760b694a39 100644
> --- a/xen/include/xen/tasklet.h
> +++ b/xen/include/xen/tasklet.h
> @@ -27,7 +27,7 @@ struct tasklet
>  
>  #define _DECLARE_TASKLET(name, func, data, softirq)                     \
>      struct tasklet name = {                                             \
> -        LIST_HEAD_INIT(name.list), -1, softirq, 0, 0, func, data }
> +        LIST_HEAD_INIT((name).list), -1, softirq, 0, 0, func, data }
>  #define DECLARE_TASKLET(name, func, data)               \
>      _DECLARE_TASKLET(name, func, data, 0)
>  #define DECLARE_SOFTIRQ_TASKLET(name, func, data)       \

In reality this is not required due to "struct tasklet name", but for
uniformity:

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

[XEN PATCH 10/10] xen/keyhandler: address violations of MISRA C Rule 20.7

[Nicola Vetrini]

MISRA C Rule 20.7 states: "Expressions resulting from the expansion
of macro parameters shall be enclosed in parentheses". Therefore, some
macro definitions should gain additional parentheses to ensure that all
current and future users will be safe with respect to expansions that
can possibly alter the semantics of the passed-in macro parameter.

No functional change.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
 xen/common/keyhandler.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/common/keyhandler.c b/xen/common/keyhandler.c
index 127ca506965c..4c1ce007870f 100644
--- a/xen/common/keyhandler.c
+++ b/xen/common/keyhandler.c
@@ -42,10 +42,10 @@ static struct keyhandler {
 } key_table[128] __read_mostly =
 {
 #define KEYHANDLER(k, f, desc, diag)            \
-    [k] = { { .fn = (f) }, desc, 0, diag }
+    [k] = { { .fn = (f) }, (desc), 0, (diag) }
 
 #define IRQ_KEYHANDLER(k, f, desc, diag)        \
-    [k] = { { .irq_fn = (f) }, desc, 1, diag }
+    [k] = { { .irq_fn = (f) }, (desc), 1, (diag) }
 
     IRQ_KEYHANDLER('A', do_toggle_alt_key, "toggle alternative key handling", 0),
     IRQ_KEYHANDLER('d', dump_registers, "dump registers", 1),
-- 
2.34.1

Re: [XEN PATCH 10/10] xen/keyhandler: address violations of MISRA C Rule 20.7

[Stefano Stabellini]

On Thu, 29 Feb 2024, Nicola Vetrini wrote:
> MISRA C Rule 20.7 states: "Expressions resulting from the expansion
> of macro parameters shall be enclosed in parentheses". Therefore, some
> macro definitions should gain additional parentheses to ensure that all
> current and future users will be safe with respect to expansions that
> can possibly alter the semantics of the passed-in macro parameter.
> 
> No functional change.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>

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


> ---
>  xen/common/keyhandler.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/common/keyhandler.c b/xen/common/keyhandler.c
> index 127ca506965c..4c1ce007870f 100644
> --- a/xen/common/keyhandler.c
> +++ b/xen/common/keyhandler.c
> @@ -42,10 +42,10 @@ static struct keyhandler {
>  } key_table[128] __read_mostly =
>  {
>  #define KEYHANDLER(k, f, desc, diag)            \
> -    [k] = { { .fn = (f) }, desc, 0, diag }
> +    [k] = { { .fn = (f) }, (desc), 0, (diag) }
>  
>  #define IRQ_KEYHANDLER(k, f, desc, diag)        \
> -    [k] = { { .irq_fn = (f) }, desc, 1, diag }
> +    [k] = { { .irq_fn = (f) }, (desc), 1, (diag) }
>  
>      IRQ_KEYHANDLER('A', do_toggle_alt_key, "toggle alternative key handling", 0),
>      IRQ_KEYHANDLER('d', dump_registers, "dump registers", 1),
> -- 
> 2.34.1
> 

Re: [XEN PATCH 10/10] xen/keyhandler: address violations of MISRA C Rule 20.7

[Jan Beulich]

On 29.02.2024 23:57, Stefano Stabellini wrote:
> On Thu, 29 Feb 2024, Nicola Vetrini wrote:
>> MISRA C Rule 20.7 states: "Expressions resulting from the expansion
>> of macro parameters shall be enclosed in parentheses". Therefore, some
>> macro definitions should gain additional parentheses to ensure that all
>> current and future users will be safe with respect to expansions that
>> can possibly alter the semantics of the passed-in macro parameter.
>>
>> No functional change.
>>
>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> 
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

You did see the discussion on earlier patches, though? I don't think
any of the parentheses here are needed or wanted.

Jan

Re: [XEN PATCH 10/10] xen/keyhandler: address violations of MISRA C Rule 20.7

[Stefano Stabellini]

On Fri, 1 Mar 2024, Jan Beulich wrote:
> On 29.02.2024 23:57, Stefano Stabellini wrote:
> > On Thu, 29 Feb 2024, Nicola Vetrini wrote:
> >> MISRA C Rule 20.7 states: "Expressions resulting from the expansion
> >> of macro parameters shall be enclosed in parentheses". Therefore, some
> >> macro definitions should gain additional parentheses to ensure that all
> >> current and future users will be safe with respect to expansions that
> >> can possibly alter the semantics of the passed-in macro parameter.
> >>
> >> No functional change.
> >>
> >> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> > 
> > Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> 
> You did see the discussion on earlier patches, though? I don't think
> any of the parentheses here are needed or wanted.

We need to align on this. Currently if we go by what's written in
docs/misra/deviations.rst, then rhs should have parentheses.

Can we safely claim that rhs parentheses are never needed? If so, then
great, let's add it to deviations.rst and skip them here and other
places in this patch series (e.g. patch #8). When I say "never" I am
taking for granted that the caller is not doing something completely
unacceptably broken such as: 

     WRITE_SYSREG64(var +, TTBR0_EL1)

If we cannot generically claim that rhs parentheses are never needed,
then I don't think we should make any exceptions. We should add them here
and everywhere else. It should be easy to write a macro or review a
patch with a macro from someone else, and making special exception makes
it more difficult for everyone.

Re: [XEN PATCH 10/10] xen/keyhandler: address violations of MISRA C Rule 20.7

[Jan Beulich]

On 02.03.2024 02:37, Stefano Stabellini wrote:
> On Fri, 1 Mar 2024, Jan Beulich wrote:
>> On 29.02.2024 23:57, Stefano Stabellini wrote:
>>> On Thu, 29 Feb 2024, Nicola Vetrini wrote:
>>>> MISRA C Rule 20.7 states: "Expressions resulting from the expansion
>>>> of macro parameters shall be enclosed in parentheses". Therefore, some
>>>> macro definitions should gain additional parentheses to ensure that all
>>>> current and future users will be safe with respect to expansions that
>>>> can possibly alter the semantics of the passed-in macro parameter.
>>>>
>>>> No functional change.
>>>>
>>>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>>>
>>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
>>
>> You did see the discussion on earlier patches, though? I don't think
>> any of the parentheses here are needed or wanted.
> 
> We need to align on this. Currently if we go by what's written in
> docs/misra/deviations.rst, then rhs should have parentheses.

Quoting the actual patch again:

--- a/xen/common/keyhandler.c
+++ b/xen/common/keyhandler.c
@@ -42,10 +42,10 @@ static struct keyhandler {
 } key_table[128] __read_mostly =
 {
 #define KEYHANDLER(k, f, desc, diag)            \
-    [k] = { { .fn = (f) }, desc, 0, diag }
+    [k] = { { .fn = (f) }, (desc), 0, (diag) }
 
 #define IRQ_KEYHANDLER(k, f, desc, diag)        \
-    [k] = { { .irq_fn = (f) }, desc, 1, diag }
+    [k] = { { .irq_fn = (f) }, (desc), 1, (diag) }

What rhs are you talking about in light of this change? The only rhs I
can spot here is already parenthesized.

> Can we safely claim that rhs parentheses are never needed? If so, then
> great, let's add it to deviations.rst and skip them here and other
> places in this patch series (e.g. patch #8). When I say "never" I am
> taking for granted that the caller is not doing something completely
> unacceptably broken such as: 
> 
>      WRITE_SYSREG64(var +, TTBR0_EL1)

I'm afraid I can't associate this with the patch here either. Instead in
the context here a (respective) construct as you mention above would simply
fail to build.

Jan

> If we cannot generically claim that rhs parentheses are never needed,
> then I don't think we should make any exceptions. We should add them here
> and everywhere else. It should be easy to write a macro or review a
> patch with a macro from someone else, and making special exception makes
> it more difficult for everyone.

Re: [XEN PATCH 10/10] xen/keyhandler: address violations of MISRA C Rule 20.7

[Stefano Stabellini]

On Mon, 4 Mar 2024, Jan Beulich wrote:
> On 02.03.2024 02:37, Stefano Stabellini wrote:
> > On Fri, 1 Mar 2024, Jan Beulich wrote:
> >> On 29.02.2024 23:57, Stefano Stabellini wrote:
> >>> On Thu, 29 Feb 2024, Nicola Vetrini wrote:
> >>>> MISRA C Rule 20.7 states: "Expressions resulting from the expansion
> >>>> of macro parameters shall be enclosed in parentheses". Therefore, some
> >>>> macro definitions should gain additional parentheses to ensure that all
> >>>> current and future users will be safe with respect to expansions that
> >>>> can possibly alter the semantics of the passed-in macro parameter.
> >>>>
> >>>> No functional change.
> >>>>
> >>>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> >>>
> >>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> >>
> >> You did see the discussion on earlier patches, though? I don't think
> >> any of the parentheses here are needed or wanted.
> > 
> > We need to align on this. Currently if we go by what's written in
> > docs/misra/deviations.rst, then rhs should have parentheses.
> 
> Quoting the actual patch again:

[...]

> What rhs are you talking about in light of this change? The only rhs I
> can spot here is already parenthesized.

Yes you are right. I replied here as an overall comment about our
approach to 20.7, although this patch is not a good example. My reply
was meant in the context of https://marc.info/?l=xen-devel&m=170928051025701


> > Can we safely claim that rhs parentheses are never needed? If so, then
> > great, let's add it to deviations.rst and skip them here and other
> > places in this patch series (e.g. patch #8). When I say "never" I am
> > taking for granted that the caller is not doing something completely
> > unacceptably broken such as: 
> > 
> >      WRITE_SYSREG64(var +, TTBR0_EL1)
> 
> I'm afraid I can't associate this with the patch here either. Instead in
> the context here a (respective) construct as you mention above would simply
> fail to build.

Fair enough it will break the build. I was trying to clarify that when I
wrote "the rhs parentheses are never needed" I meant "never" within
reason. One can always find ways to break the system and I tried to make
an example of something that for sure would break rhs or lhs without
parentheses.

I meant to say, if we don't account for exceptionally broken cases, can
we safety say we don't need parentheses for rhs?


 
> > If we cannot generically claim that rhs parentheses are never needed,
> > then I don't think we should make any exceptions. We should add them here
> > and everywhere else. It should be easy to write a macro or review a
> > patch with a macro from someone else, and making special exception makes
> > it more difficult for everyone.

Re: [XEN PATCH 10/10] xen/keyhandler: address violations of MISRA C Rule 20.7

[Jan Beulich]

On 05.03.2024 03:03, Stefano Stabellini wrote:
> On Mon, 4 Mar 2024, Jan Beulich wrote:
>> On 02.03.2024 02:37, Stefano Stabellini wrote:
>>> On Fri, 1 Mar 2024, Jan Beulich wrote:
>>>> On 29.02.2024 23:57, Stefano Stabellini wrote:
>>>>> On Thu, 29 Feb 2024, Nicola Vetrini wrote:
>>>>>> MISRA C Rule 20.7 states: "Expressions resulting from the expansion
>>>>>> of macro parameters shall be enclosed in parentheses". Therefore, some
>>>>>> macro definitions should gain additional parentheses to ensure that all
>>>>>> current and future users will be safe with respect to expansions that
>>>>>> can possibly alter the semantics of the passed-in macro parameter.
>>>>>>
>>>>>> No functional change.
>>>>>>
>>>>>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>>>>>
>>>>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
>>>>
>>>> You did see the discussion on earlier patches, though? I don't think
>>>> any of the parentheses here are needed or wanted.
>>>
>>> We need to align on this. Currently if we go by what's written in
>>> docs/misra/deviations.rst, then rhs should have parentheses.
>>
>> Quoting the actual patch again:
> 
> [...]
> 
>> What rhs are you talking about in light of this change? The only rhs I
>> can spot here is already parenthesized.
> 
> Yes you are right. I replied here as an overall comment about our
> approach to 20.7, although this patch is not a good example. My reply
> was meant in the context of https://marc.info/?l=xen-devel&m=170928051025701

I'm still confused: The rhs is being parenthsized there. It's the _lhs_
which isn't and ...

>>> Can we safely claim that rhs parentheses are never needed? If so, then
>>> great, let's add it to deviations.rst and skip them here and other
>>> places in this patch series (e.g. patch #8). When I say "never" I am
>>> taking for granted that the caller is not doing something completely
>>> unacceptably broken such as: 
>>>
>>>      WRITE_SYSREG64(var +, TTBR0_EL1)
>>
>> I'm afraid I can't associate this with the patch here either. Instead in
>> the context here a (respective) construct as you mention above would simply
>> fail to build.
> 
> Fair enough it will break the build. I was trying to clarify that when I
> wrote "the rhs parentheses are never needed" I meant "never" within
> reason. One can always find ways to break the system and I tried to make
> an example of something that for sure would break rhs or lhs without
> parentheses.
> 
> I meant to say, if we don't account for exceptionally broken cases, can
> we safety say we don't need parentheses for rhs?

... doesn't need to, unless - as you say - one contrives examples. Yet to
clarify here as well: I assume you mean "we don't need parentheses for lhs".

And note that even if your example used the first parameter as lhs of an
assignment, the build would still break. The + there would not magically
combine with the = to a += operator. Tokenization occurs ahead of
preprocessing, so the expanded macro would still have a + token followed by
a = one. The only way to alter tokens is by using the ## operator. Which in
turn precludes using parentheses.

Jan

Re: [XEN PATCH 10/10] xen/keyhandler: address violations of MISRA C Rule 20.7

[Stefano Stabellini]

On Tue, 5 Mar 2024, Jan Beulich wrote:
> On 05.03.2024 03:03, Stefano Stabellini wrote:
> > On Mon, 4 Mar 2024, Jan Beulich wrote:
> >> On 02.03.2024 02:37, Stefano Stabellini wrote:
> >>> On Fri, 1 Mar 2024, Jan Beulich wrote:
> >>>> On 29.02.2024 23:57, Stefano Stabellini wrote:
> >>>>> On Thu, 29 Feb 2024, Nicola Vetrini wrote:
> >>>>>> MISRA C Rule 20.7 states: "Expressions resulting from the expansion
> >>>>>> of macro parameters shall be enclosed in parentheses". Therefore, some
> >>>>>> macro definitions should gain additional parentheses to ensure that all
> >>>>>> current and future users will be safe with respect to expansions that
> >>>>>> can possibly alter the semantics of the passed-in macro parameter.
> >>>>>>
> >>>>>> No functional change.
> >>>>>>
> >>>>>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> >>>>>
> >>>>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> >>>>
> >>>> You did see the discussion on earlier patches, though? I don't think
> >>>> any of the parentheses here are needed or wanted.
> >>>
> >>> We need to align on this. Currently if we go by what's written in
> >>> docs/misra/deviations.rst, then rhs should have parentheses.
> >>
> >> Quoting the actual patch again:
> > 
> > [...]
> > 
> >> What rhs are you talking about in light of this change? The only rhs I
> >> can spot here is already parenthesized.
> > 
> > Yes you are right. I replied here as an overall comment about our
> > approach to 20.7, although this patch is not a good example. My reply
> > was meant in the context of https://marc.info/?l=xen-devel&m=170928051025701
> 
> I'm still confused: The rhs is being parenthsized there. It's the _lhs_
> which isn't and ...
> 
> >>> Can we safely claim that rhs parentheses are never needed? If so, then
> >>> great, let's add it to deviations.rst and skip them here and other
> >>> places in this patch series (e.g. patch #8). When I say "never" I am
> >>> taking for granted that the caller is not doing something completely
> >>> unacceptably broken such as: 
> >>>
> >>>      WRITE_SYSREG64(var +, TTBR0_EL1)
> >>
> >> I'm afraid I can't associate this with the patch here either. Instead in
> >> the context here a (respective) construct as you mention above would simply
> >> fail to build.
> > 
> > Fair enough it will break the build. I was trying to clarify that when I
> > wrote "the rhs parentheses are never needed" I meant "never" within
> > reason. One can always find ways to break the system and I tried to make
> > an example of something that for sure would break rhs or lhs without
> > parentheses.
> > 
> > I meant to say, if we don't account for exceptionally broken cases, can
> > we safety say we don't need parentheses for rhs?
> 
> ... doesn't need to, unless - as you say - one contrives examples. Yet to
> clarify here as well: I assume you mean "we don't need parentheses for lhs".

Yes. Far clarity, we are all aligned that lhs does not need parentheses
and in fact it is even already deviated in docs/misra/deviations.rst.

Now back to this specific patch.

The problem is that the comma ',' doesn't need parenthesis for the
parameters. However, the way we wrote the deviation in
docs/misra/deviations.rst doesn't cover the case this patch is dealing
with. docs/misra/deviations.rst only speaks of functions and macros
arguments.

Should we rephrase docs/misra/deviations.rst in terms of ',' instead ?
Can ECLAR deal with it?



> And note that even if your example used the first parameter as lhs of an
> assignment, the build would still break. The + there would not magically
> combine with the = to a += operator. Tokenization occurs ahead of
> preprocessing, so the expanded macro would still have a + token followed by
> a = one. The only way to alter tokens is by using the ## operator. Which in
> turn precludes using parentheses.

OK

Re: [XEN PATCH 10/10] xen/keyhandler: address violations of MISRA C Rule 20.7

[Jan Beulich]

On 07.03.2024 02:39, Stefano Stabellini wrote:
> On Tue, 5 Mar 2024, Jan Beulich wrote:
>> On 05.03.2024 03:03, Stefano Stabellini wrote:
>>> On Mon, 4 Mar 2024, Jan Beulich wrote:
>>>> On 02.03.2024 02:37, Stefano Stabellini wrote:
>>>>> On Fri, 1 Mar 2024, Jan Beulich wrote:
>>>>>> On 29.02.2024 23:57, Stefano Stabellini wrote:
>>>>>>> On Thu, 29 Feb 2024, Nicola Vetrini wrote:
>>>>>>>> MISRA C Rule 20.7 states: "Expressions resulting from the expansion
>>>>>>>> of macro parameters shall be enclosed in parentheses". Therefore, some
>>>>>>>> macro definitions should gain additional parentheses to ensure that all
>>>>>>>> current and future users will be safe with respect to expansions that
>>>>>>>> can possibly alter the semantics of the passed-in macro parameter.
>>>>>>>>
>>>>>>>> No functional change.
>>>>>>>>
>>>>>>>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>>>>>>>
>>>>>>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
>>>>>>
>>>>>> You did see the discussion on earlier patches, though? I don't think
>>>>>> any of the parentheses here are needed or wanted.
>>>>>
>>>>> We need to align on this. Currently if we go by what's written in
>>>>> docs/misra/deviations.rst, then rhs should have parentheses.
>>>>
>>>> Quoting the actual patch again:
>>>
>>> [...]
>>>
>>>> What rhs are you talking about in light of this change? The only rhs I
>>>> can spot here is already parenthesized.
>>>
>>> Yes you are right. I replied here as an overall comment about our
>>> approach to 20.7, although this patch is not a good example. My reply
>>> was meant in the context of https://marc.info/?l=xen-devel&m=170928051025701
>>
>> I'm still confused: The rhs is being parenthsized there. It's the _lhs_
>> which isn't and ...
>>
>>>>> Can we safely claim that rhs parentheses are never needed? If so, then
>>>>> great, let's add it to deviations.rst and skip them here and other
>>>>> places in this patch series (e.g. patch #8). When I say "never" I am
>>>>> taking for granted that the caller is not doing something completely
>>>>> unacceptably broken such as: 
>>>>>
>>>>>      WRITE_SYSREG64(var +, TTBR0_EL1)
>>>>
>>>> I'm afraid I can't associate this with the patch here either. Instead in
>>>> the context here a (respective) construct as you mention above would simply
>>>> fail to build.
>>>
>>> Fair enough it will break the build. I was trying to clarify that when I
>>> wrote "the rhs parentheses are never needed" I meant "never" within
>>> reason. One can always find ways to break the system and I tried to make
>>> an example of something that for sure would break rhs or lhs without
>>> parentheses.
>>>
>>> I meant to say, if we don't account for exceptionally broken cases, can
>>> we safety say we don't need parentheses for rhs?
>>
>> ... doesn't need to, unless - as you say - one contrives examples. Yet to
>> clarify here as well: I assume you mean "we don't need parentheses for lhs".
> 
> Yes. Far clarity, we are all aligned that lhs does not need parentheses
> and in fact it is even already deviated in docs/misra/deviations.rst.
> 
> Now back to this specific patch.
> 
> The problem is that the comma ',' doesn't need parenthesis for the
> parameters. However, the way we wrote the deviation in
> docs/misra/deviations.rst doesn't cover the case this patch is dealing
> with. docs/misra/deviations.rst only speaks of functions and macros
> arguments.
> 
> Should we rephrase docs/misra/deviations.rst in terms of ',' instead ?

That's what I've requested elsewhere, yes.

> Can ECLAR deal with it?

I seem to vaguely recall Nicola saying it can, but if not then it surely
can be made to do so.

Jan

Re: [XEN PATCH 10/10] xen/keyhandler: address violations of MISRA C Rule 20.7

[Nicola Vetrini]

On 2024-03-07 08:42, Jan Beulich wrote:
> On 07.03.2024 02:39, Stefano Stabellini wrote:
>> On Tue, 5 Mar 2024, Jan Beulich wrote:
>>> On 05.03.2024 03:03, Stefano Stabellini wrote:
>>>> On Mon, 4 Mar 2024, Jan Beulich wrote:
>>>>> On 02.03.2024 02:37, Stefano Stabellini wrote:
>>>>>> On Fri, 1 Mar 2024, Jan Beulich wrote:
>>>>>>> On 29.02.2024 23:57, Stefano Stabellini wrote:
>>>>>>>> On Thu, 29 Feb 2024, Nicola Vetrini wrote:
>>>>>>>>> MISRA C Rule 20.7 states: "Expressions resulting from the 
>>>>>>>>> expansion
>>>>>>>>> of macro parameters shall be enclosed in parentheses". 
>>>>>>>>> Therefore, some
>>>>>>>>> macro definitions should gain additional parentheses to ensure 
>>>>>>>>> that all
>>>>>>>>> current and future users will be safe with respect to 
>>>>>>>>> expansions that
>>>>>>>>> can possibly alter the semantics of the passed-in macro 
>>>>>>>>> parameter.
>>>>>>>>> 
>>>>>>>>> No functional change.
>>>>>>>>> 
>>>>>>>>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>>>>>>>> 
>>>>>>>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
>>>>>>> 
>>>>>>> You did see the discussion on earlier patches, though? I don't 
>>>>>>> think
>>>>>>> any of the parentheses here are needed or wanted.
>>>>>> 
>>>>>> We need to align on this. Currently if we go by what's written in
>>>>>> docs/misra/deviations.rst, then rhs should have parentheses.
>>>>> 
>>>>> Quoting the actual patch again:
>>>> 
>>>> [...]
>>>> 
>>>>> What rhs are you talking about in light of this change? The only 
>>>>> rhs I
>>>>> can spot here is already parenthesized.
>>>> 
>>>> Yes you are right. I replied here as an overall comment about our
>>>> approach to 20.7, although this patch is not a good example. My 
>>>> reply
>>>> was meant in the context of 
>>>> https://marc.info/?l=xen-devel&m=170928051025701
>>> 
>>> I'm still confused: The rhs is being parenthsized there. It's the 
>>> _lhs_
>>> which isn't and ...
>>> 
>>>>>> Can we safely claim that rhs parentheses are never needed? If so, 
>>>>>> then
>>>>>> great, let's add it to deviations.rst and skip them here and other
>>>>>> places in this patch series (e.g. patch #8). When I say "never" I 
>>>>>> am
>>>>>> taking for granted that the caller is not doing something 
>>>>>> completely
>>>>>> unacceptably broken such as:
>>>>>> 
>>>>>>      WRITE_SYSREG64(var +, TTBR0_EL1)
>>>>> 
>>>>> I'm afraid I can't associate this with the patch here either. 
>>>>> Instead in
>>>>> the context here a (respective) construct as you mention above 
>>>>> would simply
>>>>> fail to build.
>>>> 
>>>> Fair enough it will break the build. I was trying to clarify that 
>>>> when I
>>>> wrote "the rhs parentheses are never needed" I meant "never" within
>>>> reason. One can always find ways to break the system and I tried to 
>>>> make
>>>> an example of something that for sure would break rhs or lhs without
>>>> parentheses.
>>>> 
>>>> I meant to say, if we don't account for exceptionally broken cases, 
>>>> can
>>>> we safety say we don't need parentheses for rhs?
>>> 
>>> ... doesn't need to, unless - as you say - one contrives examples. 
>>> Yet to
>>> clarify here as well: I assume you mean "we don't need parentheses 
>>> for lhs".
>> 
>> Yes. Far clarity, we are all aligned that lhs does not need 
>> parentheses
>> and in fact it is even already deviated in docs/misra/deviations.rst.
>> 
>> Now back to this specific patch.
>> 
>> The problem is that the comma ',' doesn't need parenthesis for the
>> parameters. However, the way we wrote the deviation in
>> docs/misra/deviations.rst doesn't cover the case this patch is dealing
>> with. docs/misra/deviations.rst only speaks of functions and macros
>> arguments.
>> 
>> Should we rephrase docs/misra/deviations.rst in terms of ',' instead ?
> 
> That's what I've requested elsewhere, yes.
> 
>> Can ECLAR deal with it?
> 
> I seem to vaguely recall Nicola saying it can, but if not then it 
> surely
> can be made to do so.
> 
> Jan

Yes. In v2 I'll write the deviation, so that this patch and other can be 
dropped, since they only deal with this subcase.

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

[XEN PATCH 00/10] address some violations of MISRA C Rule 20.7

[Nicola Vetrini]

Hi all,

this series aims to refactor some macros that cause violations of MISRA C Rule
20.7 ("Expressions resulting from the expansion of macro parameters shall be
enclosed in parentheses"). All the macros touched by these patches are in some
way involved in violations, and the strategy adopted to bring them into
compliance is to add parentheses around macro arguments where needed.

Given that the community has previously requested a deviation from the rule, as
stated in docs/misra/deviations.rst, and reported below for convenience [1],
some macro parameters do not need any adjusting (e.g., when used as lhs to
an assignment in statement expressions).

Note that the patch on list.h of the earlier series [2] has been intentionally
left out of this series, because it still needs some adjustments.

[1] - Code violating Rule 20.7 is safe when macro parameters are used:
       (1) as function arguments;
       (2) as macro arguments;
       (3) as array indices;
       (4) as lhs in assignments.
       
[2] https://lore.kernel.org/xen-devel/cover.1709896401.git.nicola.vetrini@bugseng.com/

Nicola Vetrini (10):
  x86/cpufeature: add parentheses to comply with Rule 20.7
  AMD/IOMMU: guest: address violations of MISRA C Rule 20.7
  xen/xsm: add parentheses to comply with MISRA C Rule 20.7
  xen/device_tree: address violations of MISRA C Rule 20.7
  EFI: address violations of MISRA C Rule 20.7
  xen/arm: smmu: address violations of MISRA C Rule 20.7
  xen/efi: efibind: address violations of MISRA C Rule 20.7
  xen/notifier: address violations of MISRA C Rule 20.7
  xen/wait: address violations of MISRA C Rule 20.7
  xen/sched: address violations of MISRA C Rule 20.7

 xen/arch/arm/include/asm/arm64/efibind.h  | 4 ++--
 xen/arch/x86/include/asm/cpufeatureset.h  | 2 +-
 xen/arch/x86/include/asm/x86_64/efibind.h | 4 ++--
 xen/common/sched/private.h                | 2 +-
 xen/drivers/passthrough/amd/iommu_guest.c | 2 +-
 xen/drivers/passthrough/arm/smmu.c        | 2 +-
 xen/include/efi/efierr.h                  | 2 +-
 xen/include/xen/device_tree.h             | 2 +-
 xen/include/xen/notifier.h                | 2 +-
 xen/include/xen/wait.h                    | 4 ++--
 xen/include/xsm/dummy.h                   | 4 ++--
 11 files changed, 15 insertions(+), 15 deletions(-)

-- 
2.34.1