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

[XEN PATCH 00/11] 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).

Patch 1 is taken, with adjustments, from [2]. Patches 2 and 3 are taken from [3]
without any modifications. All other patches are new in this series and are
pairwise indipendent.

[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/b93a64b93ef4210f5fe96368c2522e5e71e9c87c.1709896401.git.nicola.vetrini@bugseng.com/
[3] https://lore.kernel.org/xen-devel/cover.1710762555.git.nicola.vetrini@bugseng.com/

Nicola Vetrini (11):
  xen/list: address violations of MISRA C Rule 20.7
  xen/xsm: add parentheses to comply with MISRA C Rule 20.7
  xen/efi: efibind: address violations of MISRA C Rule 20.7
  xentrace: address violation of MISRA C Rule 20.7
  xen: address MISRA C Rule 20.7 violation in generated hypercall
  xen/efi: address violations of MISRA C Rule 20.7
  xen/page_alloc: address violations of MISRA C Rule 20.7
  x86/altcall: address violations of MISRA C Rule 20.7
  x86/msi: address violation of MISRA C Rule 20.7 and coding style
  x86/hvm: address violations of Rule 20.7
  x86/public: hvm: address violations of MISRA C Rule 20.7

 xen/arch/arm/include/asm/arm64/efibind.h  |   4 +-
 xen/arch/x86/hvm/hvm.c                    |   6 +-
 xen/arch/x86/include/asm/alternative.h    |  76 ++++++------
 xen/arch/x86/include/asm/hvm/save.h       |   4 +-
 xen/arch/x86/include/asm/msi.h            |  47 +++----
 xen/arch/x86/include/asm/x86_64/efibind.h |   4 +-
 xen/common/page_alloc.c                   |   2 +-
 xen/include/efi/efiapi.h                  |   3 +-
 xen/include/public/arch-x86/xen.h         |   2 +-
 xen/include/public/trace.h                |   2 +-
 xen/include/xen/list.h                    | 143 +++++++++++-----------
 xen/include/xsm/dummy.h                   |   4 +-
 xen/scripts/gen_hypercall.awk             |   4 +-
 13 files changed, 151 insertions(+), 150 deletions(-)

-- 
2.34.1

[XEN PATCH 01/11] xen/list: 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>
---
Changes in v2:
- changes to list.h are all in this patch;
- Parenthesized some instances of "pos" and "n" even when already
covered by the deviation on the lhs of an assingment for consistency.
Changes in v3:
- Added more parentheses that were missed.
---
 xen/include/xen/list.h | 143 ++++++++++++++++++++---------------------
 1 file changed, 71 insertions(+), 72 deletions(-)

diff --git a/xen/include/xen/list.h b/xen/include/xen/list.h
index b5eab3a1eb6c..6506ac40893b 100644
--- a/xen/include/xen/list.h
+++ b/xen/include/xen/list.h
@@ -452,7 +452,7 @@ static inline void list_splice_init(struct list_head *list,
  * @head:    the head for your list.
  */
 #define list_for_each(pos, head)                                        \
-    for (pos = (head)->next; pos != (head); pos = pos->next)
+    for ((pos) = (head)->next; (pos) != (head); (pos) = (pos)->next)
 
 /**
  * list_for_each_prev - iterate over a list backwards
@@ -460,7 +460,7 @@ static inline void list_splice_init(struct list_head *list,
  * @head:   the head for your list.
  */
 #define list_for_each_prev(pos, head)                                   \
-    for (pos = (head)->prev; pos != (head); pos = pos->prev)
+    for ((pos) = (head)->prev; (pos) != (head); (pos) = (pos)->prev)
 
 /**
  * list_for_each_safe - iterate over a list safe against removal of list entry
@@ -468,9 +468,9 @@ static inline void list_splice_init(struct list_head *list,
  * @n:      another &struct list_head to use as temporary storage
  * @head:   the head for your list.
  */
-#define list_for_each_safe(pos, n, head)                        \
-    for (pos = (head)->next, n = pos->next; pos != (head);      \
-         pos = n, n = pos->next)
+#define list_for_each_safe(pos, n, head)                                \
+    for ((pos) = (head)->next, (n) = (pos)->next; (pos) != (head);      \
+         (pos) = (n), (n) = (pos)->next)
 
 /**
  * list_for_each_backwards_safe    -    iterate backwards over a list safe
@@ -479,9 +479,9 @@ static inline void list_splice_init(struct list_head *list,
  * @n:      another &struct list_head to use as temporary storage
  * @head:   the head for your list.
  */
-#define list_for_each_backwards_safe(pos, n, head)              \
-    for ( pos = (head)->prev, n = pos->prev; pos != (head);     \
-          pos = n, n = pos->prev )
+#define list_for_each_backwards_safe(pos, n, head)                   \
+    for ( (pos) = (head)->prev, (n) = (pos)->prev; (pos) != (head);  \
+          (pos) = (n), (n) = (pos)->prev )
 
 /**
  * list_for_each_entry - iterate over list of given type
@@ -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
@@ -527,10 +527,10 @@ static inline void list_splice_init(struct list_head *list,
  * Continue to iterate over list of given type, continuing after
  * 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))
+#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))
 
 /**
  * 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
@@ -569,11 +569,11 @@ static inline void list_splice_init(struct list_head *list,
  * Iterate over list of given type, continuing after current point,
  * 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))
+#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))
 
 /**
  * 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
@@ -616,14 +616,14 @@ 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_rcu(pos, head)                            \
-    for (pos = (head)->next;                                    \
+    for ((pos) = (head)->next;                                  \
          rcu_dereference(pos) != (head);                        \
-         pos = pos->next)
+         (pos) = (pos)->next)
 
 #define __list_for_each_rcu(pos, head)          \
-    for (pos = (head)->next;                    \
+    for ((pos) = (head)->next;                  \
          rcu_dereference(pos) != (head);        \
-         pos = pos->next)
+         (pos) = (pos)->next)
 
 /**
  * list_for_each_safe_rcu
@@ -638,9 +638,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_safe_rcu(pos, n, head)            \
-    for (pos = (head)->next;                            \
-         n = rcu_dereference(pos)->next, pos != (head); \
-         pos = n)
+    for ((pos) = (head)->next;                          \
+         (n) = rcu_dereference(pos)->next, (pos) != (head); \
+         (pos) = (n))
 
 /**
  * list_for_each_entry_rcu - iterate over rcu list of given type
@@ -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
@@ -899,11 +899,11 @@ static inline void hlist_add_after_rcu(struct hlist_node *prev,
 #define hlist_entry(ptr, type, member) container_of(ptr,type,member)
 
 #define hlist_for_each(pos, head)                                       \
-    for (pos = (head)->first; pos; pos = pos->next)
+    for ((pos) = (head)->first; (pos); (pos) = (pos)->next)
 
-#define hlist_for_each_safe(pos, n, head)                       \
-    for (pos = (head)->first; pos && ({ n = pos->next; 1; });   \
-         pos = n)
+#define hlist_for_each_safe(pos, n, head)                               \
+    for ((pos) = (head)->first; (pos) && ({ (n) = (pos)->next; 1; });   \
+         (pos) = (n))
 
 /**
  * hlist_for_each_entry    - iterate over list of given type
@@ -913,10 +913,10 @@ static inline void hlist_add_after_rcu(struct hlist_node *prev,
  * @member:    the name of the hlist_node within the struct.
  */
 #define hlist_for_each_entry(tpos, pos, head, member)                   \
-    for (pos = (head)->first;                                           \
-         pos &&                                                         \
-         ({ tpos = hlist_entry(pos, typeof(*tpos), member); 1;});       \
-         pos = pos->next)
+    for ((pos) = (head)->first;                                         \
+         (pos) &&                                                       \
+         ({ tpos = hlist_entry(pos, typeof(*(tpos)), member); 1;});     \
+         (pos) = (pos)->next)
 
 /**
  * hlist_for_each_entry_continue - iterate over a hlist continuing
@@ -925,11 +925,11 @@ static inline void hlist_add_after_rcu(struct hlist_node *prev,
  * @pos:    the &struct hlist_node to use as a loop cursor.
  * @member:    the name of the hlist_node within the struct.
  */
-#define hlist_for_each_entry_continue(tpos, pos, member)                \
-    for (pos = (pos)->next;                                             \
-         pos &&                                                         \
-         ({ tpos = hlist_entry(pos, typeof(*tpos), member); 1;});       \
-         pos = pos->next)
+#define hlist_for_each_entry_continue(tpos, pos, member)                  \
+    for ((pos) = (pos)->next;                                             \
+         (pos) &&                                                         \
+         ({ tpos = hlist_entry(pos, typeof(*(tpos)), member); 1;});       \
+         (pos) = (pos)->next)
 
 /**
  * hlist_for_each_entry_from - iterate over a hlist continuing from
@@ -938,10 +938,10 @@ static inline void hlist_add_after_rcu(struct hlist_node *prev,
  * @pos:    the &struct hlist_node to use as a loop cursor.
  * @member:    the name of the hlist_node within the struct.
  */
-#define hlist_for_each_entry_from(tpos, pos, member)                    \
-    for (; pos &&                                                       \
-         ({ tpos = hlist_entry(pos, typeof(*tpos), member); 1;});       \
-         pos = pos->next)
+#define hlist_for_each_entry_from(tpos, pos, member)                      \
+    for (; (pos) &&                                                       \
+         ({ tpos = hlist_entry(pos, typeof(*(tpos)), member); 1;});       \
+         (pos) = (pos)->next)
 
 /**
  * hlist_for_each_entry_safe - iterate over list of given type safe
@@ -952,11 +952,11 @@ static inline void hlist_add_after_rcu(struct hlist_node *prev,
  * @head:    the head for your list.
  * @member:    the name of the hlist_node within the struct.
  */
-#define hlist_for_each_entry_safe(tpos, pos, n, head, member)           \
-    for (pos = (head)->first;                                           \
-         pos && ({ n = pos->next; 1; }) &&                              \
-         ({ tpos = hlist_entry(pos, typeof(*tpos), member); 1;});       \
-         pos = n)
+#define hlist_for_each_entry_safe(tpos, pos, n, head, member)             \
+    for ((pos) = (head)->first;                                           \
+         (pos) && ({ n = (pos)->next; 1; }) &&                            \
+         ({ tpos = hlist_entry(pos, typeof(*(tpos)), member); 1;});       \
+         (pos) = (n))
 
 
 /**
@@ -971,10 +971,9 @@ static inline void hlist_add_after_rcu(struct hlist_node *prev,
  * as long as the traversal is guarded by rcu_read_lock().
  */
 #define hlist_for_each_entry_rcu(tpos, pos, head, member)               \
-     for (pos = (head)->first;                                          \
+     for ((pos) = (head)->first;                                        \
           rcu_dereference(pos) &&                                       \
-          ({ tpos = hlist_entry(pos, typeof(*tpos), member); 1;});      \
-          pos = pos->next)
+          ({ tpos = hlist_entry(pos, typeof(*(tpos)), member); 1;});    \
+          (pos) = (pos)->next)
 
 #endif /* __XEN_LIST_H__ */
-
-- 
2.34.1

[XEN PATCH 02/11] xen/xsm: add parentheses to comply with 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>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
---
 xen/include/xsm/dummy.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
index 8671af1ba4d3..88039fdd227c 100644
--- a/xen/include/xsm/dummy.h
+++ b/xen/include/xsm/dummy.h
@@ -58,7 +58,7 @@ void __xsm_action_mismatch_detected(void);
 
 #define XSM_DEFAULT_ARG /* */
 #define XSM_DEFAULT_VOID void
-#define XSM_ASSERT_ACTION(def) xsm_default_t action = def; (void)action
+#define XSM_ASSERT_ACTION(def) xsm_default_t action = (def); (void)action
 
 #else /* CONFIG_XSM */
 
@@ -71,7 +71,7 @@ void __xsm_action_mismatch_detected(void);
 #define XSM_INLINE always_inline
 #define XSM_DEFAULT_ARG xsm_default_t action,
 #define XSM_DEFAULT_VOID xsm_default_t action
-#define XSM_ASSERT_ACTION(def) LINKER_BUG_ON(def != action)
+#define XSM_ASSERT_ACTION(def) LINKER_BUG_ON((def) != action)
 
 #endif /* CONFIG_XSM */
 
-- 
2.34.1

[XEN PATCH 03/11] xen/efi: efibind: 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>
Acked-by: Jan Beulich <jbeulich@suse.com>
---
This file is matched by exclude-list.json, but the fix is rather trivial
and impacts code that in under the scope of MISRA compliance.
---
 xen/arch/arm/include/asm/arm64/efibind.h  | 4 ++--
 xen/arch/x86/include/asm/x86_64/efibind.h | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/xen/arch/arm/include/asm/arm64/efibind.h b/xen/arch/arm/include/asm/arm64/efibind.h
index f13eadd4f0ab..a1323d452e2e 100644
--- a/xen/arch/arm/include/asm/arm64/efibind.h
+++ b/xen/arch/arm/include/asm/arm64/efibind.h
@@ -22,9 +22,9 @@ Revision History
 #pragma pack()
 #endif
 
-#define EFIERR(a)           (0x8000000000000000ULL | a)
+#define EFIERR(a)           (0x8000000000000000ULL | (a))
 #define EFI_ERROR_MASK      0x8000000000000000ULL
-#define EFIERR_OEM(a)       (0xc000000000000000ULL | a)
+#define EFIERR_OEM(a)       (0xc000000000000000ULL | (a))
 
 #define BAD_POINTER         0xFBFBFBFBFBFBFBFBULL
 #define MAX_ADDRESS         0xFFFFFFFFFFFFFFFFULL
diff --git a/xen/arch/x86/include/asm/x86_64/efibind.h b/xen/arch/x86/include/asm/x86_64/efibind.h
index e23cd16cb6a0..28bc18c24bb3 100644
--- a/xen/arch/x86/include/asm/x86_64/efibind.h
+++ b/xen/arch/x86/include/asm/x86_64/efibind.h
@@ -117,9 +117,9 @@ typedef uint64_t   UINTN;
     #endif
 #endif
 
-#define EFIERR(a)           (0x8000000000000000 | a)
+#define EFIERR(a)           (0x8000000000000000 | (a))
 #define EFI_ERROR_MASK      0x8000000000000000
-#define EFIERR_OEM(a)       (0xc000000000000000 | a)
+#define EFIERR_OEM(a)       (0xc000000000000000 | (a))
 
 
 #define BAD_POINTER         0xFBFBFBFBFBFBFBFB
-- 
2.34.1

[XEN PATCH 04/11] xentrace: address violation 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/public/trace.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/include/public/trace.h b/xen/include/public/trace.h
index 62a179971d2a..3c9f9c3c18b2 100644
--- a/xen/include/public/trace.h
+++ b/xen/include/public/trace.h
@@ -67,7 +67,7 @@
 #define TRC_SCHED_CLASS_EVT(_c, _e) \
   ( ( TRC_SCHED_CLASS | \
       ((TRC_SCHED_##_c << TRC_SCHED_ID_SHIFT) & TRC_SCHED_ID_MASK) ) + \
-    (_e & TRC_SCHED_EVT_MASK) )
+    ((_e) & TRC_SCHED_EVT_MASK) )
 
 /* Trace classes for DOM0 operations */
 #define TRC_DOM0_DOMOPS     0x00041000   /* Domains manipulations */
-- 
2.34.1

[XEN PATCH 05/11] xen: address MISRA C Rule 20.7 violation in generated hypercall

[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/scripts/gen_hypercall.awk | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/scripts/gen_hypercall.awk b/xen/scripts/gen_hypercall.awk
index 9f7cfa298a6d..1a7e051fde10 100644
--- a/xen/scripts/gen_hypercall.awk
+++ b/xen/scripts/gen_hypercall.awk
@@ -277,7 +277,7 @@ END {
                         if (call[i] == ca && call_prio[i] == p_list[pl]) {
                             fnd++;
                             if (fnd == 1)
-                                printf("        if ( num == __HYPERVISOR_%s ) \\\n", fn[call_fn[i]]);
+                                printf("        if ( (num) == __HYPERVISOR_%s ) \\\n", fn[call_fn[i]]);
                             else
                                 printf("        else \\\n");
                             do_call(call_fn[i], call_p[i]);
@@ -290,7 +290,7 @@ END {
             } else {
                 for (i = 1; i <= nc; i++)
                     if (call[i] == ca && call_prio[i] == p_list[pl]) {
-                        printf("if ( likely(num == __HYPERVISOR_%s) ) \\\n", fn[call_fn[i]]);
+                        printf("if ( likely((num) == __HYPERVISOR_%s) ) \\\n", fn[call_fn[i]]);
                         do_call(call_fn[i], call_p[i]);
                     }
             }
-- 
2.34.1

[XEN PATCH 06/11] xen/efi: 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/efi/efiapi.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/xen/include/efi/efiapi.h b/xen/include/efi/efiapi.h
index a616d1238aa4..6d4d4e340d9e 100644
--- a/xen/include/efi/efiapi.h
+++ b/xen/include/efi/efiapi.h
@@ -63,7 +63,8 @@ EFI_STATUS
     OUT UINT32                      *DescriptorVersion
     );
 
-#define NextMemoryDescriptor(Ptr,Size)  ((EFI_MEMORY_DESCRIPTOR *) (((UINT8 *) Ptr) + Size))
+#define NextMemoryDescriptor(Ptr,Size)  ((EFI_MEMORY_DESCRIPTOR *) \
+                                         ((UINT8 *)(Ptr) + (Size)))
 
 
 typedef
-- 
2.34.1

[XEN PATCH 07/11] xen/page_alloc: 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/page_alloc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index 28c510d6669b..82dc55829f78 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -150,7 +150,7 @@
 #include <asm/paging.h>
 #else
 #define p2m_pod_offline_or_broken_hit(pg) 0
-#define p2m_pod_offline_or_broken_replace(pg) BUG_ON(pg != NULL)
+#define p2m_pod_offline_or_broken_replace(pg) BUG_ON((pg) != NULL)
 #endif
 
 #ifndef PGC_static
-- 
2.34.1

[XEN PATCH 08/11] x86/altcall: 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/arch/x86/include/asm/alternative.h | 76 +++++++++++++-------------
 1 file changed, 38 insertions(+), 38 deletions(-)

diff --git a/xen/arch/x86/include/asm/alternative.h b/xen/arch/x86/include/asm/alternative.h
index 0d3697f1de49..a3b7cbab8740 100644
--- a/xen/arch/x86/include/asm/alternative.h
+++ b/xen/arch/x86/include/asm/alternative.h
@@ -243,28 +243,28 @@ extern void alternative_branches(void);
 
 #define alternative_vcall0(func) ({             \
     ALT_CALL_NO_ARG1;                           \
-    (void)sizeof(func());                       \
+    (void)sizeof((func)());                     \
     (void)alternative_callN(0, int, func);      \
 })
 
-#define alternative_call0(func) ({              \
-    ALT_CALL_NO_ARG1;                           \
-    alternative_callN(0, typeof(func()), func); \
+#define alternative_call0(func) ({                \
+    ALT_CALL_NO_ARG1;                             \
+    alternative_callN(0, typeof((func)()), func); \
 })
 
 #define alternative_vcall1(func, arg) ({           \
     typeof(arg) v1_ = (arg);                       \
     ALT_CALL_ARG(v1_, 1);                          \
     ALT_CALL_NO_ARG2;                              \
-    (void)sizeof(func(arg));                       \
+    (void)sizeof((func)(arg));                     \
     (void)alternative_callN(1, int, func);         \
 })
 
-#define alternative_call1(func, arg) ({            \
-    typeof(arg) v1_ = (arg);                       \
-    ALT_CALL_ARG(v1_, 1);                          \
-    ALT_CALL_NO_ARG2;                              \
-    alternative_callN(1, typeof(func(arg)), func); \
+#define alternative_call1(func, arg) ({              \
+    typeof(arg) v1_ = (arg);                         \
+    ALT_CALL_ARG(v1_, 1);                            \
+    ALT_CALL_NO_ARG2;                                \
+    alternative_callN(1, typeof((func)(arg)), func); \
 })
 
 #define alternative_vcall2(func, arg1, arg2) ({           \
@@ -273,17 +273,17 @@ extern void alternative_branches(void);
     ALT_CALL_ARG(v1_, 1);                                 \
     ALT_CALL_ARG(v2_, 2);                                 \
     ALT_CALL_NO_ARG3;                                     \
-    (void)sizeof(func(arg1, arg2));                       \
+    (void)sizeof((func)(arg1, arg2));                     \
     (void)alternative_callN(2, int, func);                \
 })
 
-#define alternative_call2(func, arg1, arg2) ({            \
-    typeof(arg1) v1_ = (arg1);                            \
-    typeof(arg2) v2_ = (arg2);                            \
-    ALT_CALL_ARG(v1_, 1);                                 \
-    ALT_CALL_ARG(v2_, 2);                                 \
-    ALT_CALL_NO_ARG3;                                     \
-    alternative_callN(2, typeof(func(arg1, arg2)), func); \
+#define alternative_call2(func, arg1, arg2) ({              \
+    typeof(arg1) v1_ = (arg1);                              \
+    typeof(arg2) v2_ = (arg2);                              \
+    ALT_CALL_ARG(v1_, 1);                                   \
+    ALT_CALL_ARG(v2_, 2);                                   \
+    ALT_CALL_NO_ARG3;                                       \
+    alternative_callN(2, typeof((func)(arg1, arg2)), func); \
 })
 
 #define alternative_vcall3(func, arg1, arg2, arg3) ({    \
@@ -294,20 +294,20 @@ extern void alternative_branches(void);
     ALT_CALL_ARG(v2_, 2);                                \
     ALT_CALL_ARG(v3_, 3);                                \
     ALT_CALL_NO_ARG4;                                    \
-    (void)sizeof(func(arg1, arg2, arg3));                \
+    (void)sizeof((func)(arg1, arg2, arg3));              \
     (void)alternative_callN(3, int, func);               \
 })
 
-#define alternative_call3(func, arg1, arg2, arg3) ({     \
-    typeof(arg1) v1_ = (arg1);                            \
-    typeof(arg2) v2_ = (arg2);                           \
-    typeof(arg3) v3_ = (arg3);                           \
-    ALT_CALL_ARG(v1_, 1);                                \
-    ALT_CALL_ARG(v2_, 2);                                \
-    ALT_CALL_ARG(v3_, 3);                                \
-    ALT_CALL_NO_ARG4;                                    \
-    alternative_callN(3, typeof(func(arg1, arg2, arg3)), \
-                      func);                             \
+#define alternative_call3(func, arg1, arg2, arg3) ({       \
+    typeof(arg1) v1_ = (arg1);                             \
+    typeof(arg2) v2_ = (arg2);                             \
+    typeof(arg3) v3_ = (arg3);                             \
+    ALT_CALL_ARG(v1_, 1);                                  \
+    ALT_CALL_ARG(v2_, 2);                                  \
+    ALT_CALL_ARG(v3_, 3);                                  \
+    ALT_CALL_NO_ARG4;                                      \
+    alternative_callN(3, typeof((func)(arg1, arg2, arg3)), \
+                      func);                               \
 })
 
 #define alternative_vcall4(func, arg1, arg2, arg3, arg4) ({ \
@@ -320,7 +320,7 @@ extern void alternative_branches(void);
     ALT_CALL_ARG(v3_, 3);                                   \
     ALT_CALL_ARG(v4_, 4);                                   \
     ALT_CALL_NO_ARG5;                                       \
-    (void)sizeof(func(arg1, arg2, arg3, arg4));             \
+    (void)sizeof((func)(arg1, arg2, arg3, arg4));           \
     (void)alternative_callN(4, int, func);                  \
 })
 
@@ -334,8 +334,8 @@ extern void alternative_branches(void);
     ALT_CALL_ARG(v3_, 3);                                   \
     ALT_CALL_ARG(v4_, 4);                                   \
     ALT_CALL_NO_ARG5;                                       \
-    alternative_callN(4, typeof(func(arg1, arg2,            \
-                                     arg3, arg4)),          \
+    alternative_callN(4, typeof((func)(arg1, arg2,          \
+                                       arg3, arg4)),        \
                       func);                                \
 })
 
@@ -351,7 +351,7 @@ extern void alternative_branches(void);
     ALT_CALL_ARG(v4_, 4);                                         \
     ALT_CALL_ARG(v5_, 5);                                         \
     ALT_CALL_NO_ARG6;                                             \
-    (void)sizeof(func(arg1, arg2, arg3, arg4, arg5));             \
+    (void)sizeof((func)(arg1, arg2, arg3, arg4, arg5));           \
     (void)alternative_callN(5, int, func);                        \
 })
 
@@ -367,8 +367,8 @@ extern void alternative_branches(void);
     ALT_CALL_ARG(v4_, 4);                                         \
     ALT_CALL_ARG(v5_, 5);                                         \
     ALT_CALL_NO_ARG6;                                             \
-    alternative_callN(5, typeof(func(arg1, arg2, arg3,            \
-                                     arg4, arg5)),                \
+    alternative_callN(5, typeof((func)(arg1, arg2, arg3,          \
+                                       arg4, arg5)),              \
                       func);                                      \
 })
 
@@ -385,7 +385,7 @@ extern void alternative_branches(void);
     ALT_CALL_ARG(v4_, 4);                                               \
     ALT_CALL_ARG(v5_, 5);                                               \
     ALT_CALL_ARG(v6_, 6);                                               \
-    (void)sizeof(func(arg1, arg2, arg3, arg4, arg5, arg6));             \
+    (void)sizeof((func)(arg1, arg2, arg3, arg4, arg5, arg6));           \
     (void)alternative_callN(6, int, func);                              \
 })
 
@@ -402,8 +402,8 @@ extern void alternative_branches(void);
     ALT_CALL_ARG(v4_, 4);                                               \
     ALT_CALL_ARG(v5_, 5);                                               \
     ALT_CALL_ARG(v6_, 6);                                               \
-    alternative_callN(6, typeof(func(arg1, arg2, arg3,                  \
-                                     arg4, arg5, arg6)),                \
+    alternative_callN(6, typeof((func)(arg1, arg2, arg3,                \
+                                       arg4, arg5, arg6)),              \
                       func);                                            \
 })
 
-- 
2.34.1

[XEN PATCH 09/11] x86/msi: address violation of MISRA C Rule 20.7 and coding style

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

While at it, the style of these macros has been somewhat uniformed.

No functional change.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
 xen/arch/x86/include/asm/msi.h | 47 +++++++++++++++++-----------------
 1 file changed, 24 insertions(+), 23 deletions(-)

diff --git a/xen/arch/x86/include/asm/msi.h b/xen/arch/x86/include/asm/msi.h
index 997ccb87be0c..e24d46d95a02 100644
--- a/xen/arch/x86/include/asm/msi.h
+++ b/xen/arch/x86/include/asm/msi.h
@@ -147,33 +147,34 @@ int msi_free_irq(struct msi_desc *entry);
  */
 #define NR_HP_RESERVED_VECTORS 	20
 
-#define msi_control_reg(base)		(base + PCI_MSI_FLAGS)
-#define msi_lower_address_reg(base)	(base + PCI_MSI_ADDRESS_LO)
-#define msi_upper_address_reg(base)	(base + PCI_MSI_ADDRESS_HI)
-#define msi_data_reg(base, is64bit)	\
-	( (is64bit == 1) ? base+PCI_MSI_DATA_64 : base+PCI_MSI_DATA_32 )
-#define msi_mask_bits_reg(base, is64bit) \
-	( (is64bit == 1) ? base+PCI_MSI_MASK_BIT : base+PCI_MSI_MASK_BIT-4)
+#define msi_control_reg(base)        ((base) + PCI_MSI_FLAGS)
+#define msi_lower_address_reg(base)  ((base) + PCI_MSI_ADDRESS_LO)
+#define msi_upper_address_reg(base)  ((base) + PCI_MSI_ADDRESS_HI)
+#define msi_data_reg(base, is64bit) \
+    (((is64bit) == 1) ? (base) + PCI_MSI_DATA_64 : (base) + PCI_MSI_DATA_32)
+#define msi_mask_bits_reg(base, is64bit)                \
+    (((is64bit) == 1) ? (base) + PCI_MSI_MASK_BIT       \
+                      : (base) + PCI_MSI_MASK_BIT - 4)
 #define msi_pending_bits_reg(base, is64bit) \
-	((base) + PCI_MSI_MASK_BIT + ((is64bit) ? 4 : 0))
-#define msi_disable(control)		control &= ~PCI_MSI_FLAGS_ENABLE
+    ((base) + PCI_MSI_MASK_BIT + ((is64bit) ? 4 : 0))
+#define msi_disable(control) (control) &= ~PCI_MSI_FLAGS_ENABLE
 #define multi_msi_capable(control) \
-	(1 << ((control & PCI_MSI_FLAGS_QMASK) >> 1))
+    (1 << (((control) & PCI_MSI_FLAGS_QMASK) >> 1))
 #define multi_msi_enable(control, num) \
-	control |= (((fls(num) - 1) << 4) & PCI_MSI_FLAGS_QSIZE);
-#define is_64bit_address(control)	(!!(control & PCI_MSI_FLAGS_64BIT))
-#define is_mask_bit_support(control)	(!!(control & PCI_MSI_FLAGS_MASKBIT))
+    (control) |= (((fls(num) - 1) << 4) & PCI_MSI_FLAGS_QSIZE);
+#define is_64bit_address(control) (!!((control) & PCI_MSI_FLAGS_64BIT))
+#define is_mask_bit_support(control) (!!((control) & PCI_MSI_FLAGS_MASKBIT))
 #define msi_enable(control, num) multi_msi_enable(control, num); \
-	control |= PCI_MSI_FLAGS_ENABLE
-
-#define msix_control_reg(base)		(base + PCI_MSIX_FLAGS)
-#define msix_table_offset_reg(base)	(base + PCI_MSIX_TABLE)
-#define msix_pba_offset_reg(base)	(base + PCI_MSIX_PBA)
-#define msix_enable(control)	 	control |= PCI_MSIX_FLAGS_ENABLE
-#define msix_disable(control)	 	control &= ~PCI_MSIX_FLAGS_ENABLE
-#define msix_table_size(control) 	((control & PCI_MSIX_FLAGS_QSIZE)+1)
-#define msix_unmask(address)	 	(address & ~PCI_MSIX_VECTOR_BITMASK)
-#define msix_mask(address)		(address | PCI_MSIX_VECTOR_BITMASK)
+                                 (control) |= PCI_MSI_FLAGS_ENABLE
+
+#define msix_control_reg(base)       ((base) + PCI_MSIX_FLAGS)
+#define msix_table_offset_reg(base)  ((base) + PCI_MSIX_TABLE)
+#define msix_pba_offset_reg(base)    ((base) + PCI_MSIX_PBA)
+#define msix_enable(control)         (control) |= PCI_MSIX_FLAGS_ENABLE
+#define msix_disable(control)        (control) &= ~PCI_MSIX_FLAGS_ENABLE
+#define msix_table_size(control)     (((control) & PCI_MSIX_FLAGS_QSIZE) + 1)
+#define msix_unmask(address)         ((address) & ~PCI_MSIX_VECTOR_BITMASK)
+#define msix_mask(address)           ((address) | PCI_MSIX_VECTOR_BITMASK)
 
 /*
  * MSI Defined Data Structures
-- 
2.34.1

[XEN PATCH 10/11] x86/hvm: address violations of 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/arch/x86/hvm/hvm.c              | 6 +++---
 xen/arch/x86/include/asm/hvm/save.h | 4 ++--
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index c75959588c0e..0ce45b177cf4 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -1066,9 +1066,9 @@ static int cf_check hvm_load_cpu_ctxt(struct domain *d, hvm_domain_context_t *h)
     /* Older Xen versions used to save the segment arbytes directly 
      * from the VMCS on Intel hosts.  Detect this and rearrange them
      * into the struct segment_register format. */
-#define UNFOLD_ARBYTES(_r)                          \
-    if ( (_r & 0xf000) && !(_r & 0x0f00) )          \
-        _r = ((_r & 0xff) | ((_r >> 4) & 0xf00))
+#define UNFOLD_ARBYTES(_r)                              \
+    if ( ((_r) & 0xf000) && !((_r) & 0x0f00) )          \
+        (_r) = (((_r) & 0xff) | (((_r) >> 4) & 0xf00))
     UNFOLD_ARBYTES(ctxt.cs_arbytes);
     UNFOLD_ARBYTES(ctxt.ds_arbytes);
     UNFOLD_ARBYTES(ctxt.es_arbytes);
diff --git a/xen/arch/x86/include/asm/hvm/save.h b/xen/arch/x86/include/asm/hvm/save.h
index 04a47ffcc40a..2d4b591aa3e2 100644
--- a/xen/arch/x86/include/asm/hvm/save.h
+++ b/xen/arch/x86/include/asm/hvm/save.h
@@ -128,9 +128,9 @@ static int __init cf_check __hvm_register_##_x##_save_and_restore(void)   \
 {                                                                         \
     hvm_register_savevm(HVM_SAVE_CODE(_x),                                \
                         #_x,                                              \
-                        &_save,                                           \
+                        &(_save),                                         \
                         check,                                            \
-                        &_load,                                           \
+                        &(_load),                                         \
                         (_num) * (HVM_SAVE_LENGTH(_x)                     \
                                   + sizeof (struct hvm_save_descriptor)), \
                         _k);                                              \
-- 
2.34.1

[XEN PATCH 11/11] x86/public: hvm: 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/public/arch-x86/xen.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/include/public/arch-x86/xen.h b/xen/include/public/arch-x86/xen.h
index c0f4551247f4..a9a87d9b50de 100644
--- a/xen/include/public/arch-x86/xen.h
+++ b/xen/include/public/arch-x86/xen.h
@@ -36,7 +36,7 @@
 #define __XEN_GUEST_HANDLE(name)        __guest_handle_ ## name
 #define XEN_GUEST_HANDLE(name)          __XEN_GUEST_HANDLE(name)
 #define XEN_GUEST_HANDLE_PARAM(name)    XEN_GUEST_HANDLE(name)
-#define set_xen_guest_handle_raw(hnd, val)  do { (hnd).p = val; } while (0)
+#define set_xen_guest_handle_raw(hnd, val)  do { (hnd).p = (val); } while (0)
 #define set_xen_guest_handle(hnd, val) set_xen_guest_handle_raw(hnd, val)
 
 #if defined(__i386__)
-- 
2.34.1

Re: [XEN PATCH 02/11] xen/xsm: add parentheses to comply with MISRA C Rule 20.7

[Daniel P. Smith]

On 3/22/24 12:01, 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/include/xsm/dummy.h | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
> index 8671af1ba4d3..88039fdd227c 100644
> --- a/xen/include/xsm/dummy.h
> +++ b/xen/include/xsm/dummy.h
> @@ -58,7 +58,7 @@ void __xsm_action_mismatch_detected(void);
>   
>   #define XSM_DEFAULT_ARG /* */
>   #define XSM_DEFAULT_VOID void
> -#define XSM_ASSERT_ACTION(def) xsm_default_t action = def; (void)action
> +#define XSM_ASSERT_ACTION(def) xsm_default_t action = (def); (void)action
>   
>   #else /* CONFIG_XSM */
>   
> @@ -71,7 +71,7 @@ void __xsm_action_mismatch_detected(void);
>   #define XSM_INLINE always_inline
>   #define XSM_DEFAULT_ARG xsm_default_t action,
>   #define XSM_DEFAULT_VOID xsm_default_t action
> -#define XSM_ASSERT_ACTION(def) LINKER_BUG_ON(def != action)
> +#define XSM_ASSERT_ACTION(def) LINKER_BUG_ON((def) != action)
>   
>   #endif /* CONFIG_XSM */
>   

Acked-by: Daniel P. Smith <dpsmith@apertussolutions.com>

Re: [XEN PATCH 00/11] address some violations of MISRA C Rule 20.7

[Jan Beulich]

On 22.03.2024 17:01, Nicola Vetrini wrote:
> 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).
> 
> Patch 1 is taken, with adjustments, from [2]. Patches 2 and 3 are taken from [3]
> without any modifications. All other patches are new in this series and are
> pairwise indipendent.
> 
> [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/b93a64b93ef4210f5fe96368c2522e5e71e9c87c.1709896401.git.nicola.vetrini@bugseng.com/
> [3] https://lore.kernel.org/xen-devel/cover.1710762555.git.nicola.vetrini@bugseng.com/
> 
> Nicola Vetrini (11):
>   xen/list: address violations of MISRA C Rule 20.7
>   xen/xsm: add parentheses to comply with MISRA C Rule 20.7
>   xen/efi: efibind: address violations of MISRA C Rule 20.7
>   xentrace: address violation of MISRA C Rule 20.7
>   xen: address MISRA C Rule 20.7 violation in generated hypercall
>   xen/efi: address violations of MISRA C Rule 20.7
>   xen/page_alloc: address violations of MISRA C Rule 20.7
>   x86/altcall: address violations of MISRA C Rule 20.7
>   x86/msi: address violation of MISRA C Rule 20.7 and coding style
>   x86/hvm: address violations of Rule 20.7
>   x86/public: hvm: address violations of MISRA C Rule 20.7

Just to clarify: While most of the patches here are new, two(?) I think
were submitted before. As such, to indicate that, the series as a whole
would want to be tagged v2.

Jan

Re: [XEN PATCH 00/11] address some violations of MISRA C Rule 20.7

[Nicola Vetrini]

On 2024-03-25 09:00, Jan Beulich wrote:
> On 22.03.2024 17:01, Nicola Vetrini wrote:
>> 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).
>> 
>> Patch 1 is taken, with adjustments, from [2]. Patches 2 and 3 are 
>> taken from [3]
>> without any modifications. All other patches are new in this series 
>> and are
>> pairwise indipendent.
>> 
>> [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/b93a64b93ef4210f5fe96368c2522e5e71e9c87c.1709896401.git.nicola.vetrini@bugseng.com/
>> [3] 
>> https://lore.kernel.org/xen-devel/cover.1710762555.git.nicola.vetrini@bugseng.com/
>> 
>> Nicola Vetrini (11):
>>   xen/list: address violations of MISRA C Rule 20.7
>>   xen/xsm: add parentheses to comply with MISRA C Rule 20.7
>>   xen/efi: efibind: address violations of MISRA C Rule 20.7
>>   xentrace: address violation of MISRA C Rule 20.7
>>   xen: address MISRA C Rule 20.7 violation in generated hypercall
>>   xen/efi: address violations of MISRA C Rule 20.7
>>   xen/page_alloc: address violations of MISRA C Rule 20.7
>>   x86/altcall: address violations of MISRA C Rule 20.7
>>   x86/msi: address violation of MISRA C Rule 20.7 and coding style
>>   x86/hvm: address violations of Rule 20.7
>>   x86/public: hvm: address violations of MISRA C Rule 20.7
> 
> Just to clarify: While most of the patches here are new, two(?) I think
> were submitted before. As such, to indicate that, the series as a whole
> would want to be tagged v2.
> 
> Jan

Ok, that was an oversight. Technically the list.h patch makes this a v3, 
then.

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

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

[Jan Beulich]

On 22.03.2024 17:01, 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: Jan Beulich <jbeulich@suse.com>

Re: [XEN PATCH 04/11] xentrace: address violation of MISRA C Rule 20.7

[Jan Beulich]

On 22.03.2024 17:01, 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>

Re: [XEN PATCH 05/11] xen: address MISRA C Rule 20.7 violation in generated hypercall

[Jan Beulich]

On 22.03.2024 17:01, 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: Jan Beulich <jbeulich@suse.com>

Re: [XEN PATCH 06/11] xen/efi: address violations of MISRA C Rule 20.7

[Jan Beulich]

On 22.03.2024 17:01, 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>

Seeing this is again touching an imported header, we finally need to settle
on where to draw the line. The way this is going is that you're proposing
many small changes which individually all look "small enough". Yet when
doing so for long enough, the entire header may end up different from its
original.

Jan

> --- a/xen/include/efi/efiapi.h
> +++ b/xen/include/efi/efiapi.h
> @@ -63,7 +63,8 @@ EFI_STATUS
>      OUT UINT32                      *DescriptorVersion
>      );
>  
> -#define NextMemoryDescriptor(Ptr,Size)  ((EFI_MEMORY_DESCRIPTOR *) (((UINT8 *) Ptr) + Size))
> +#define NextMemoryDescriptor(Ptr,Size)  ((EFI_MEMORY_DESCRIPTOR *) \
> +                                         ((UINT8 *)(Ptr) + (Size)))
>  
>  
>  typedef

Re: [XEN PATCH 07/11] xen/page_alloc: address violations of MISRA C Rule 20.7

[Jan Beulich]

On 22.03.2024 17:01, Nicola Vetrini wrote:
> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -150,7 +150,7 @@
>  #include <asm/paging.h>
>  #else
>  #define p2m_pod_offline_or_broken_hit(pg) 0

Seeing this in context: Does Misra also have a rule demanding evaluation
of macro arguments?

> -#define p2m_pod_offline_or_broken_replace(pg) BUG_ON(pg != NULL)
> +#define p2m_pod_offline_or_broken_replace(pg) BUG_ON((pg) != NULL)

Or easier

#define p2m_pod_offline_or_broken_replace(pg) BUG_ON(pg)

?

Jan

Re: [XEN PATCH 08/11] x86/altcall: address violations of MISRA C Rule 20.7

[Jan Beulich]

On 22.03.2024 17:01, 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>

Hmm. These macros are, at least in part, hard to read already. The added
parentheses, while necessary when following the rule to the letter, are
making things worse, even if just slightly. I therefore have a proposal /
question:

> --- a/xen/arch/x86/include/asm/alternative.h
> +++ b/xen/arch/x86/include/asm/alternative.h
> @@ -243,28 +243,28 @@ extern void alternative_branches(void);
>  
>  #define alternative_vcall0(func) ({             \
>      ALT_CALL_NO_ARG1;                           \
> -    (void)sizeof(func());                       \
> +    (void)sizeof((func)());                     \

Like this, all that's touched here are (syntactical, but not real) function
invocations. Function calls, like all postfix operators, are highest
precedence. Hence by omitting parentheses in that case no breakage can
happen as a result: If the passed expression is another postfix one, that'll
be evaluated first anyway. If any other expression is passed (aside primary
ones, of course), that'll be evaluated afterwards only due to being lower
precedence, irrespective of the presence/absence of parentheses.

Therefore, where harmful to readability, can we perhaps leave postfix
expressions alone?

Jan

Re: [XEN PATCH 06/11] xen/efi: address violations of MISRA C Rule 20.7

[Nicola Vetrini]

On 2024-03-25 10:25, Jan Beulich wrote:
> On 22.03.2024 17:01, 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>
> 
> Seeing this is again touching an imported header, we finally need to 
> settle
> on where to draw the line. The way this is going is that you're 
> proposing
> many small changes which individually all look "small enough". Yet when
> doing so for long enough, the entire header may end up different from 
> its
> original.
> 
> Jan
> 

Ok, if you feel that these (and possibly others) are too many, then I'll 
leave xen/include/efi/* alone and deviate those violations resulting 
from the use of these macros in files that are in scope. I don't think 
there are others from imported headers outside efi, but I'm not 100% 
sure.

>> --- a/xen/include/efi/efiapi.h
>> +++ b/xen/include/efi/efiapi.h
>> @@ -63,7 +63,8 @@ EFI_STATUS
>>      OUT UINT32                      *DescriptorVersion
>>      );
>> 
>> -#define NextMemoryDescriptor(Ptr,Size)  ((EFI_MEMORY_DESCRIPTOR *) 
>> (((UINT8 *) Ptr) + Size))
>> +#define NextMemoryDescriptor(Ptr,Size)  ((EFI_MEMORY_DESCRIPTOR *) \
>> +                                         ((UINT8 *)(Ptr) + (Size)))
>> 
>> 
>>  typedef

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

Re: [XEN PATCH 08/11] x86/altcall: address violations of MISRA C Rule 20.7

[Nicola Vetrini]

On 2024-03-25 10:38, Jan Beulich wrote:
> On 22.03.2024 17:01, 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>
> 
> Hmm. These macros are, at least in part, hard to read already. The 
> added
> parentheses, while necessary when following the rule to the letter, are
> making things worse, even if just slightly. I therefore have a proposal 
> /
> question:
> 
>> --- a/xen/arch/x86/include/asm/alternative.h
>> +++ b/xen/arch/x86/include/asm/alternative.h
>> @@ -243,28 +243,28 @@ extern void alternative_branches(void);
>> 
>>  #define alternative_vcall0(func) ({             \
>>      ALT_CALL_NO_ARG1;                           \
>> -    (void)sizeof(func());                       \
>> +    (void)sizeof((func)());                     \
> 
> Like this, all that's touched here are (syntactical, but not real) 
> function
> invocations. Function calls, like all postfix operators, are highest
> precedence. Hence by omitting parentheses in that case no breakage can
> happen as a result: If the passed expression is another postfix one, 
> that'll
> be evaluated first anyway. If any other expression is passed (aside 
> primary
> ones, of course), that'll be evaluated afterwards only due to being 
> lower
> precedence, irrespective of the presence/absence of parentheses.
> 
> Therefore, where harmful to readability, can we perhaps leave postfix
> expressions alone?
> 
> Jan

While I can understand the benefits of this, and the reasoning on 
postfix expressions, what about, for instance (modulo the actual 
invocation, this is just an example)

alternative_vcall0(2 + f) or similar scenarios?

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

Re: [XEN PATCH 08/11] x86/altcall: address violations of MISRA C Rule 20.7

[Jan Beulich]

On 25.03.2024 15:47, Nicola Vetrini wrote:
> On 2024-03-25 10:38, Jan Beulich wrote:
>> On 22.03.2024 17:01, 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>
>>
>> Hmm. These macros are, at least in part, hard to read already. The 
>> added
>> parentheses, while necessary when following the rule to the letter, are
>> making things worse, even if just slightly. I therefore have a proposal 
>> /
>> question:
>>
>>> --- a/xen/arch/x86/include/asm/alternative.h
>>> +++ b/xen/arch/x86/include/asm/alternative.h
>>> @@ -243,28 +243,28 @@ extern void alternative_branches(void);
>>>
>>>  #define alternative_vcall0(func) ({             \
>>>      ALT_CALL_NO_ARG1;                           \
>>> -    (void)sizeof(func());                       \
>>> +    (void)sizeof((func)());                     \
>>
>> Like this, all that's touched here are (syntactical, but not real) 
>> function
>> invocations. Function calls, like all postfix operators, are highest
>> precedence. Hence by omitting parentheses in that case no breakage can
>> happen as a result: If the passed expression is another postfix one, 
>> that'll
>> be evaluated first anyway. If any other expression is passed (aside 
>> primary
>> ones, of course), that'll be evaluated afterwards only due to being 
>> lower
>> precedence, irrespective of the presence/absence of parentheses.
>>
>> Therefore, where harmful to readability, can we perhaps leave postfix
>> expressions alone?
> 
> While I can understand the benefits of this, and the reasoning on 
> postfix expressions, what about, for instance (modulo the actual 
> invocation, this is just an example)
> 
> alternative_vcall0(2 + f) or similar scenarios?

Any kind of such expression will break with alternative_callN()'s
using of [addr] "i" (&(func)) as an asm() operand. Which clarifies
that even of the postfix expressions only some (in particular not
increment, decrement, and function call) could potentially be used
with the altcall machinery.

That said, you have a point in (indirectly) expressing that my
earlier description wasn't quite right (as in: too generic, when
it really needs tying to the specific case here).

Jan

Re: [XEN PATCH 09/11] x86/msi: address violation of MISRA C Rule 20.7 and coding style

[Jan Beulich]

On 22.03.2024 17:01, 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.
> 
> While at it, the style of these macros has been somewhat uniformed.

Hmm, yes, but they then still leave more to be desired. I guess I can see
though why you don't want to e.g. ...

> --- a/xen/arch/x86/include/asm/msi.h
> +++ b/xen/arch/x86/include/asm/msi.h
> @@ -147,33 +147,34 @@ int msi_free_irq(struct msi_desc *entry);
>   */
>  #define NR_HP_RESERVED_VECTORS 	20
>  
> -#define msi_control_reg(base)		(base + PCI_MSI_FLAGS)
> -#define msi_lower_address_reg(base)	(base + PCI_MSI_ADDRESS_LO)
> -#define msi_upper_address_reg(base)	(base + PCI_MSI_ADDRESS_HI)
> -#define msi_data_reg(base, is64bit)	\
> -	( (is64bit == 1) ? base+PCI_MSI_DATA_64 : base+PCI_MSI_DATA_32 )
> -#define msi_mask_bits_reg(base, is64bit) \
> -	( (is64bit == 1) ? base+PCI_MSI_MASK_BIT : base+PCI_MSI_MASK_BIT-4)
> +#define msi_control_reg(base)        ((base) + PCI_MSI_FLAGS)
> +#define msi_lower_address_reg(base)  ((base) + PCI_MSI_ADDRESS_LO)
> +#define msi_upper_address_reg(base)  ((base) + PCI_MSI_ADDRESS_HI)
> +#define msi_data_reg(base, is64bit) \
> +    (((is64bit) == 1) ? (base) + PCI_MSI_DATA_64 : (base) + PCI_MSI_DATA_32)
> +#define msi_mask_bits_reg(base, is64bit)                \
> +    (((is64bit) == 1) ? (base) + PCI_MSI_MASK_BIT       \
> +                      : (base) + PCI_MSI_MASK_BIT - 4)

... drop the bogus == 1 in these two, making them similar to ...

>  #define msi_pending_bits_reg(base, is64bit) \
> -	((base) + PCI_MSI_MASK_BIT + ((is64bit) ? 4 : 0))
> -#define msi_disable(control)		control &= ~PCI_MSI_FLAGS_ENABLE
> +    ((base) + PCI_MSI_MASK_BIT + ((is64bit) ? 4 : 0))

... this.

> +#define msi_disable(control) (control) &= ~PCI_MSI_FLAGS_ENABLE

Doesn't this need an outer pair of parentheses, too?

>  #define multi_msi_capable(control) \
> -	(1 << ((control & PCI_MSI_FLAGS_QMASK) >> 1))
> +    (1 << (((control) & PCI_MSI_FLAGS_QMASK) >> 1))
>  #define multi_msi_enable(control, num) \
> -	control |= (((fls(num) - 1) << 4) & PCI_MSI_FLAGS_QSIZE);
> -#define is_64bit_address(control)	(!!(control & PCI_MSI_FLAGS_64BIT))
> -#define is_mask_bit_support(control)	(!!(control & PCI_MSI_FLAGS_MASKBIT))
> +    (control) |= (((fls(num) - 1) << 4) & PCI_MSI_FLAGS_QSIZE);

And this, together with dropping the bogus semicolon?

There also look to be cases where MASK_EXTR() / MASK_INSR() would want using,
in favor of using open-coded numbers.

> +#define is_64bit_address(control) (!!((control) & PCI_MSI_FLAGS_64BIT))
> +#define is_mask_bit_support(control) (!!((control) & PCI_MSI_FLAGS_MASKBIT))
>  #define msi_enable(control, num) multi_msi_enable(control, num); \
> -	control |= PCI_MSI_FLAGS_ENABLE
> -
> -#define msix_control_reg(base)		(base + PCI_MSIX_FLAGS)
> -#define msix_table_offset_reg(base)	(base + PCI_MSIX_TABLE)
> -#define msix_pba_offset_reg(base)	(base + PCI_MSIX_PBA)
> -#define msix_enable(control)	 	control |= PCI_MSIX_FLAGS_ENABLE
> -#define msix_disable(control)	 	control &= ~PCI_MSIX_FLAGS_ENABLE
> -#define msix_table_size(control) 	((control & PCI_MSIX_FLAGS_QSIZE)+1)
> -#define msix_unmask(address)	 	(address & ~PCI_MSIX_VECTOR_BITMASK)
> -#define msix_mask(address)		(address | PCI_MSIX_VECTOR_BITMASK)
> +                                 (control) |= PCI_MSI_FLAGS_ENABLE

This again is suspiciously missing outer parentheses; really here, with
the earlier statement, it look like do { ... } while ( 0 ) or ({ ... })
are wanted.

> +#define msix_control_reg(base)       ((base) + PCI_MSIX_FLAGS)
> +#define msix_table_offset_reg(base)  ((base) + PCI_MSIX_TABLE)
> +#define msix_pba_offset_reg(base)    ((base) + PCI_MSIX_PBA)
> +#define msix_enable(control)         (control) |= PCI_MSIX_FLAGS_ENABLE
> +#define msix_disable(control)        (control) &= ~PCI_MSIX_FLAGS_ENABLE

Outer parentheses missing for these two again?

Jan

Re: [XEN PATCH 10/11] x86/hvm: address violations of Rule 20.7

[Jan Beulich]

On 22.03.2024 17:01, 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>
albeit preferably with ...

> --- a/xen/arch/x86/include/asm/hvm/save.h
> +++ b/xen/arch/x86/include/asm/hvm/save.h
> @@ -128,9 +128,9 @@ static int __init cf_check __hvm_register_##_x##_save_and_restore(void)   \
>  {                                                                         \
>      hvm_register_savevm(HVM_SAVE_CODE(_x),                                \
>                          #_x,                                              \
> -                        &_save,                                           \
> +                        &(_save),                                         \
>                          check,                                            \
> -                        &_load,                                           \
> +                        &(_load),                                         \

... the &s dropped rather than parentheses added, as we already have it
for (the recently added) "check".

Jan

Re: [XEN PATCH 11/11] x86/public: hvm: address violations of MISRA C Rule 20.7

[Jan Beulich]

On 22.03.2024 17:02, 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>

> --- a/xen/include/public/arch-x86/xen.h
> +++ b/xen/include/public/arch-x86/xen.h
> @@ -36,7 +36,7 @@
>  #define __XEN_GUEST_HANDLE(name)        __guest_handle_ ## name
>  #define XEN_GUEST_HANDLE(name)          __XEN_GUEST_HANDLE(name)
>  #define XEN_GUEST_HANDLE_PARAM(name)    XEN_GUEST_HANDLE(name)
> -#define set_xen_guest_handle_raw(hnd, val)  do { (hnd).p = val; } while (0)
> +#define set_xen_guest_handle_raw(hnd, val)  do { (hnd).p = (val); } while (0)
>  #define set_xen_guest_handle(hnd, val) set_xen_guest_handle_raw(hnd, val)
>  
>  #if defined(__i386__)

Would have been nice though to do the same thing for Arm at the same time.
PPC and RISC-V already have "val" parenthesized there.

Jan

Re: [XEN PATCH 08/11] x86/altcall: address violations of MISRA C Rule 20.7

[Nicola Vetrini]

On 2024-03-25 15:58, Jan Beulich wrote:
> On 25.03.2024 15:47, Nicola Vetrini wrote:
>> On 2024-03-25 10:38, Jan Beulich wrote:
>>> On 22.03.2024 17:01, 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>
>>> 
>>> Hmm. These macros are, at least in part, hard to read already. The
>>> added
>>> parentheses, while necessary when following the rule to the letter, 
>>> are
>>> making things worse, even if just slightly. I therefore have a 
>>> proposal
>>> /
>>> question:
>>> 
>>>> --- a/xen/arch/x86/include/asm/alternative.h
>>>> +++ b/xen/arch/x86/include/asm/alternative.h
>>>> @@ -243,28 +243,28 @@ extern void alternative_branches(void);
>>>> 
>>>>  #define alternative_vcall0(func) ({             \
>>>>      ALT_CALL_NO_ARG1;                           \
>>>> -    (void)sizeof(func());                       \
>>>> +    (void)sizeof((func)());                     \
>>> 
>>> Like this, all that's touched here are (syntactical, but not real)
>>> function
>>> invocations. Function calls, like all postfix operators, are highest
>>> precedence. Hence by omitting parentheses in that case no breakage 
>>> can
>>> happen as a result: If the passed expression is another postfix one,
>>> that'll
>>> be evaluated first anyway. If any other expression is passed (aside
>>> primary
>>> ones, of course), that'll be evaluated afterwards only due to being
>>> lower
>>> precedence, irrespective of the presence/absence of parentheses.
>>> 
>>> Therefore, where harmful to readability, can we perhaps leave postfix
>>> expressions alone?
>> 
>> While I can understand the benefits of this, and the reasoning on
>> postfix expressions, what about, for instance (modulo the actual
>> invocation, this is just an example)
>> 
>> alternative_vcall0(2 + f) or similar scenarios?
> 
> Any kind of such expression will break with alternative_callN()'s
> using of [addr] "i" (&(func)) as an asm() operand. Which clarifies
> that even of the postfix expressions only some (in particular not
> increment, decrement, and function call) could potentially be used
> with the altcall machinery.
> 
> That said, you have a point in (indirectly) expressing that my
> earlier description wasn't quite right (as in: too generic, when
> it really needs tying to the specific case here).
> 
> Jan

Ok, I see what you meant now. I'll deviate these two macros.

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

Re: [XEN PATCH 09/11] x86/msi: address violation of MISRA C Rule 20.7 and coding style

[Nicola Vetrini]

On 2024-03-26 11:05, Jan Beulich wrote:
> On 22.03.2024 17:01, 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.
>> 
>> While at it, the style of these macros has been somewhat uniformed.
> 
> Hmm, yes, but they then still leave more to be desired. I guess I can 
> see
> though why you don't want to e.g. ...
> 
>> --- a/xen/arch/x86/include/asm/msi.h
>> +++ b/xen/arch/x86/include/asm/msi.h
>> @@ -147,33 +147,34 @@ int msi_free_irq(struct msi_desc *entry);
>>   */
>>  #define NR_HP_RESERVED_VECTORS 	20
>> 
>> -#define msi_control_reg(base)		(base + PCI_MSI_FLAGS)
>> -#define msi_lower_address_reg(base)	(base + PCI_MSI_ADDRESS_LO)
>> -#define msi_upper_address_reg(base)	(base + PCI_MSI_ADDRESS_HI)
>> -#define msi_data_reg(base, is64bit)	\
>> -	( (is64bit == 1) ? base+PCI_MSI_DATA_64 : base+PCI_MSI_DATA_32 )
>> -#define msi_mask_bits_reg(base, is64bit) \
>> -	( (is64bit == 1) ? base+PCI_MSI_MASK_BIT : base+PCI_MSI_MASK_BIT-4)
>> +#define msi_control_reg(base)        ((base) + PCI_MSI_FLAGS)
>> +#define msi_lower_address_reg(base)  ((base) + PCI_MSI_ADDRESS_LO)
>> +#define msi_upper_address_reg(base)  ((base) + PCI_MSI_ADDRESS_HI)
>> +#define msi_data_reg(base, is64bit) \
>> +    (((is64bit) == 1) ? (base) + PCI_MSI_DATA_64 : (base) + 
>> PCI_MSI_DATA_32)
>> +#define msi_mask_bits_reg(base, is64bit)                \
>> +    (((is64bit) == 1) ? (base) + PCI_MSI_MASK_BIT       \
>> +                      : (base) + PCI_MSI_MASK_BIT - 4)
> 
> ... drop the bogus == 1 in these two, making them similar to ...
> 
>>  #define msi_pending_bits_reg(base, is64bit) \
>> -	((base) + PCI_MSI_MASK_BIT + ((is64bit) ? 4 : 0))
>> -#define msi_disable(control)		control &= ~PCI_MSI_FLAGS_ENABLE
>> +    ((base) + PCI_MSI_MASK_BIT + ((is64bit) ? 4 : 0))
> 
> ... this.
> 
>> +#define msi_disable(control) (control) &= ~PCI_MSI_FLAGS_ENABLE
> 
> Doesn't this need an outer pair of parentheses, too?
> 

Not necessarily. I'm in favour of a consistent style to be converted in. 
This also applies below.

>>  #define multi_msi_capable(control) \
>> -	(1 << ((control & PCI_MSI_FLAGS_QMASK) >> 1))
>> +    (1 << (((control) & PCI_MSI_FLAGS_QMASK) >> 1))
>>  #define multi_msi_enable(control, num) \
>> -	control |= (((fls(num) - 1) << 4) & PCI_MSI_FLAGS_QSIZE);
>> -#define is_64bit_address(control)	(!!(control & PCI_MSI_FLAGS_64BIT))
>> -#define is_mask_bit_support(control)	(!!(control & 
>> PCI_MSI_FLAGS_MASKBIT))
>> +    (control) |= (((fls(num) - 1) << 4) & PCI_MSI_FLAGS_QSIZE);
> 
> And this, together with dropping the bogus semicolon?
> 

I'll drop the semicolon.

> There also look to be cases where MASK_EXTR() / MASK_INSR() would want 
> using,
> in favor of using open-coded numbers.
> 

Yes, perhaps. However, the risk that I make some mistakes in doing so 
are quite high, though.

>> +#define is_64bit_address(control) (!!((control) & 
>> PCI_MSI_FLAGS_64BIT))
>> +#define is_mask_bit_support(control) (!!((control) & 
>> PCI_MSI_FLAGS_MASKBIT))
>>  #define msi_enable(control, num) multi_msi_enable(control, num); \
>> -	control |= PCI_MSI_FLAGS_ENABLE
>> -
>> -#define msix_control_reg(base)		(base + PCI_MSIX_FLAGS)
>> -#define msix_table_offset_reg(base)	(base + PCI_MSIX_TABLE)
>> -#define msix_pba_offset_reg(base)	(base + PCI_MSIX_PBA)
>> -#define msix_enable(control)	 	control |= PCI_MSIX_FLAGS_ENABLE
>> -#define msix_disable(control)	 	control &= ~PCI_MSIX_FLAGS_ENABLE
>> -#define msix_table_size(control) 	((control & 
>> PCI_MSIX_FLAGS_QSIZE)+1)
>> -#define msix_unmask(address)	 	(address & ~PCI_MSIX_VECTOR_BITMASK)
>> -#define msix_mask(address)		(address | PCI_MSIX_VECTOR_BITMASK)
>> +                                 (control) |= PCI_MSI_FLAGS_ENABLE
> 
> This again is suspiciously missing outer parentheses; really here, with
> the earlier statement, it look like do { ... } while ( 0 ) or ({ ... })
> are wanted.
> 
>> +#define msix_control_reg(base)       ((base) + PCI_MSIX_FLAGS)
>> +#define msix_table_offset_reg(base)  ((base) + PCI_MSIX_TABLE)
>> +#define msix_pba_offset_reg(base)    ((base) + PCI_MSIX_PBA)
>> +#define msix_enable(control)         (control) |= 
>> PCI_MSIX_FLAGS_ENABLE
>> +#define msix_disable(control)        (control) &= 
>> ~PCI_MSIX_FLAGS_ENABLE
> 
> Outer parentheses missing for these two again?
> 
> Jan

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

Re: [XEN PATCH 10/11] x86/hvm: address violations of Rule 20.7

[Nicola Vetrini]

On 2024-03-26 11:13, Jan Beulich wrote:
> On 22.03.2024 17:01, 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>
> albeit preferably with ...
> 
>> --- a/xen/arch/x86/include/asm/hvm/save.h
>> +++ b/xen/arch/x86/include/asm/hvm/save.h
>> @@ -128,9 +128,9 @@ static int __init cf_check 
>> __hvm_register_##_x##_save_and_restore(void)   \
>>  {                                                                     
>>     \
>>      hvm_register_savevm(HVM_SAVE_CODE(_x),                            
>>     \
>>                          #_x,                                          
>>     \
>> -                        &_save,                                       
>>     \
>> +                        &(_save),                                     
>>     \
>>                          check,                                        
>>     \
>> -                        &_load,                                       
>>     \
>> +                        &(_load),                                     
>>     \
> 
> ... the &s dropped rather than parentheses added, as we already have it
> for (the recently added) "check".
> 
> Jan

Sounds good

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

Re: [XEN PATCH 11/11] x86/public: hvm: address violations of MISRA C Rule 20.7

[Nicola Vetrini]

On 2024-03-26 11:15, Jan Beulich wrote:
> On 22.03.2024 17:02, 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>
> 
>> --- a/xen/include/public/arch-x86/xen.h
>> +++ b/xen/include/public/arch-x86/xen.h
>> @@ -36,7 +36,7 @@
>>  #define __XEN_GUEST_HANDLE(name)        __guest_handle_ ## name
>>  #define XEN_GUEST_HANDLE(name)          __XEN_GUEST_HANDLE(name)
>>  #define XEN_GUEST_HANDLE_PARAM(name)    XEN_GUEST_HANDLE(name)
>> -#define set_xen_guest_handle_raw(hnd, val)  do { (hnd).p = val; } 
>> while (0)
>> +#define set_xen_guest_handle_raw(hnd, val)  do { (hnd).p = (val); } 
>> while (0)
>>  #define set_xen_guest_handle(hnd, val) set_xen_guest_handle_raw(hnd, 
>> val)
>> 
>>  #if defined(__i386__)
> 
> Would have been nice though to do the same thing for Arm at the same 
> time.
> PPC and RISC-V already have "val" parenthesized there.
> 
> Jan

Ok, no problem.

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

Re: [XEN PATCH 09/11] x86/msi: address violation of MISRA C Rule 20.7 and coding style

[Jan Beulich]

On 26.03.2024 15:30, Nicola Vetrini wrote:
> On 2024-03-26 11:05, Jan Beulich wrote:
>> On 22.03.2024 17:01, 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.
>>>
>>> While at it, the style of these macros has been somewhat uniformed.
>>
>> Hmm, yes, but they then still leave more to be desired. I guess I can 
>> see
>> though why you don't want to e.g. ...
>>
>>> --- a/xen/arch/x86/include/asm/msi.h
>>> +++ b/xen/arch/x86/include/asm/msi.h
>>> @@ -147,33 +147,34 @@ int msi_free_irq(struct msi_desc *entry);
>>>   */
>>>  #define NR_HP_RESERVED_VECTORS 	20
>>>
>>> -#define msi_control_reg(base)		(base + PCI_MSI_FLAGS)
>>> -#define msi_lower_address_reg(base)	(base + PCI_MSI_ADDRESS_LO)
>>> -#define msi_upper_address_reg(base)	(base + PCI_MSI_ADDRESS_HI)
>>> -#define msi_data_reg(base, is64bit)	\
>>> -	( (is64bit == 1) ? base+PCI_MSI_DATA_64 : base+PCI_MSI_DATA_32 )
>>> -#define msi_mask_bits_reg(base, is64bit) \
>>> -	( (is64bit == 1) ? base+PCI_MSI_MASK_BIT : base+PCI_MSI_MASK_BIT-4)
>>> +#define msi_control_reg(base)        ((base) + PCI_MSI_FLAGS)
>>> +#define msi_lower_address_reg(base)  ((base) + PCI_MSI_ADDRESS_LO)
>>> +#define msi_upper_address_reg(base)  ((base) + PCI_MSI_ADDRESS_HI)
>>> +#define msi_data_reg(base, is64bit) \
>>> +    (((is64bit) == 1) ? (base) + PCI_MSI_DATA_64 : (base) + 
>>> PCI_MSI_DATA_32)
>>> +#define msi_mask_bits_reg(base, is64bit)                \
>>> +    (((is64bit) == 1) ? (base) + PCI_MSI_MASK_BIT       \
>>> +                      : (base) + PCI_MSI_MASK_BIT - 4)
>>
>> ... drop the bogus == 1 in these two, making them similar to ...
>>
>>>  #define msi_pending_bits_reg(base, is64bit) \
>>> -	((base) + PCI_MSI_MASK_BIT + ((is64bit) ? 4 : 0))
>>> -#define msi_disable(control)		control &= ~PCI_MSI_FLAGS_ENABLE
>>> +    ((base) + PCI_MSI_MASK_BIT + ((is64bit) ? 4 : 0))
>>
>> ... this.
>>
>>> +#define msi_disable(control) (control) &= ~PCI_MSI_FLAGS_ENABLE
>>
>> Doesn't this need an outer pair of parentheses, too?
>>
> 
> Not necessarily.

And use of msi_disable() in another expression would then likely not do
what's expected?

> I'm in favour of a consistent style to be converted in. 
> This also applies below.

I'm all for consistency; I just don't know what you want to be consistent
with, here.

>>>  #define multi_msi_capable(control) \
>>> -	(1 << ((control & PCI_MSI_FLAGS_QMASK) >> 1))
>>> +    (1 << (((control) & PCI_MSI_FLAGS_QMASK) >> 1))
>>>  #define multi_msi_enable(control, num) \
>>> -	control |= (((fls(num) - 1) << 4) & PCI_MSI_FLAGS_QSIZE);
>>> -#define is_64bit_address(control)	(!!(control & PCI_MSI_FLAGS_64BIT))
>>> -#define is_mask_bit_support(control)	(!!(control & 
>>> PCI_MSI_FLAGS_MASKBIT))
>>> +    (control) |= (((fls(num) - 1) << 4) & PCI_MSI_FLAGS_QSIZE);
>>
>> And this, together with dropping the bogus semicolon?
>>
> 
> I'll drop the semicolon.
> 
>> There also look to be cases where MASK_EXTR() / MASK_INSR() would want 
>> using,
>> in favor of using open-coded numbers.
> 
> Yes, perhaps. However, the risk that I make some mistakes in doing so 
> are quite high, though.

Right, hence how I started my earlier reply. Question is - do we want to
go just half the way here, or would we better tidy things all in one go?
In the latter case I could see about getting to that (whether to take
your patch as basis or instead do it from scratch isn't quite clear to
me at this point).

Jan

Re: [XEN PATCH 07/11] xen/page_alloc: address violations of MISRA C Rule 20.7

[Nicola Vetrini]

Hi Jan,

sorry, forgot to reply.

On 2024-03-25 10:27, Jan Beulich wrote:
> On 22.03.2024 17:01, Nicola Vetrini wrote:
>> --- a/xen/common/page_alloc.c
>> +++ b/xen/common/page_alloc.c
>> @@ -150,7 +150,7 @@
>>  #include <asm/paging.h>
>>  #else
>>  #define p2m_pod_offline_or_broken_hit(pg) 0
> 
> Seeing this in context: Does Misra also have a rule demanding 
> evaluation
> of macro arguments?
> 

No such rule. There is one for unused function parameters, though.

>> -#define p2m_pod_offline_or_broken_replace(pg) BUG_ON(pg != NULL)
>> +#define p2m_pod_offline_or_broken_replace(pg) BUG_ON((pg) != NULL)
> 
> Or easier
> 
> #define p2m_pod_offline_or_broken_replace(pg) BUG_ON(pg)
> 
> ?
> 

Good point. I'll modify it.

> Jan

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

Re: [XEN PATCH 07/11] xen/page_alloc: address violations of MISRA C Rule 20.7

[Jan Beulich]

On 26.03.2024 16:27, Nicola Vetrini wrote:
> On 2024-03-25 10:27, Jan Beulich wrote:
>> On 22.03.2024 17:01, Nicola Vetrini wrote:
>>> --- a/xen/common/page_alloc.c
>>> +++ b/xen/common/page_alloc.c
>>> @@ -150,7 +150,7 @@
>>>  #include <asm/paging.h>
>>>  #else
>>>  #define p2m_pod_offline_or_broken_hit(pg) 0
>>
>> Seeing this in context: Does Misra also have a rule demanding 
>> evaluation
>> of macro arguments?
> 
> No such rule. There is one for unused function parameters, though.

Interesting. Are there no concerns regarding side effects not taking
place, as one might expect when looking just at the call site?

>>> -#define p2m_pod_offline_or_broken_replace(pg) BUG_ON(pg != NULL)
>>> +#define p2m_pod_offline_or_broken_replace(pg) BUG_ON((pg) != NULL)
>>
>> Or easier
>>
>> #define p2m_pod_offline_or_broken_replace(pg) BUG_ON(pg)
>>
>> ?
>>
> 
> Good point. I'll modify it.

I can do that as well while committing. With that adjustment
Acked-by: Jan Beulich <jbeulich@suse.com>

Jan

Re: [XEN PATCH 09/11] x86/msi: address violation of MISRA C Rule 20.7 and coding style

[Nicola Vetrini]

On 2024-03-26 16:13, Jan Beulich wrote:
> On 26.03.2024 15:30, Nicola Vetrini wrote:
>> On 2024-03-26 11:05, Jan Beulich wrote:
>>> On 22.03.2024 17:01, 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.
>>>> 
>>>> While at it, the style of these macros has been somewhat uniformed.
>>> 
>>> Hmm, yes, but they then still leave more to be desired. I guess I can
>>> see
>>> though why you don't want to e.g. ...
>>> 
>>>> --- a/xen/arch/x86/include/asm/msi.h
>>>> +++ b/xen/arch/x86/include/asm/msi.h
>>>> @@ -147,33 +147,34 @@ int msi_free_irq(struct msi_desc *entry);
>>>>   */
>>>>  #define NR_HP_RESERVED_VECTORS 	20
>>>> 
>>>> -#define msi_control_reg(base)		(base + PCI_MSI_FLAGS)
>>>> -#define msi_lower_address_reg(base)	(base + PCI_MSI_ADDRESS_LO)
>>>> -#define msi_upper_address_reg(base)	(base + PCI_MSI_ADDRESS_HI)
>>>> -#define msi_data_reg(base, is64bit)	\
>>>> -	( (is64bit == 1) ? base+PCI_MSI_DATA_64 : base+PCI_MSI_DATA_32 )
>>>> -#define msi_mask_bits_reg(base, is64bit) \
>>>> -	( (is64bit == 1) ? base+PCI_MSI_MASK_BIT : 
>>>> base+PCI_MSI_MASK_BIT-4)
>>>> +#define msi_control_reg(base)        ((base) + PCI_MSI_FLAGS)
>>>> +#define msi_lower_address_reg(base)  ((base) + PCI_MSI_ADDRESS_LO)
>>>> +#define msi_upper_address_reg(base)  ((base) + PCI_MSI_ADDRESS_HI)
>>>> +#define msi_data_reg(base, is64bit) \
>>>> +    (((is64bit) == 1) ? (base) + PCI_MSI_DATA_64 : (base) +
>>>> PCI_MSI_DATA_32)
>>>> +#define msi_mask_bits_reg(base, is64bit)                \
>>>> +    (((is64bit) == 1) ? (base) + PCI_MSI_MASK_BIT       \
>>>> +                      : (base) + PCI_MSI_MASK_BIT - 4)
>>> 
>>> ... drop the bogus == 1 in these two, making them similar to ...
>>> 
>>>>  #define msi_pending_bits_reg(base, is64bit) \
>>>> -	((base) + PCI_MSI_MASK_BIT + ((is64bit) ? 4 : 0))
>>>> -#define msi_disable(control)		control &= ~PCI_MSI_FLAGS_ENABLE
>>>> +    ((base) + PCI_MSI_MASK_BIT + ((is64bit) ? 4 : 0))
>>> 
>>> ... this.
>>> 
>>>> +#define msi_disable(control) (control) &= ~PCI_MSI_FLAGS_ENABLE
>>> 
>>> Doesn't this need an outer pair of parentheses, too?
>>> 
>> 
>> Not necessarily.
> 
> And use of msi_disable() in another expression would then likely not do
> what's expected?
> 

Actually I just noticed that some of these macros are never used 
(msi_disable being one of them), as far as I can tell.

>> I'm in favour of a consistent style to be converted in.
>> This also applies below.
> 
> I'm all for consistency; I just don't know what you want to be 
> consistent
> with, here.
> 

I would propose adding parentheses around assignments to control, so 
that all macros are consistently parenthesized.

>>>>  #define multi_msi_capable(control) \
>>>> -	(1 << ((control & PCI_MSI_FLAGS_QMASK) >> 1))
>>>> +    (1 << (((control) & PCI_MSI_FLAGS_QMASK) >> 1))
>>>>  #define multi_msi_enable(control, num) \
>>>> -	control |= (((fls(num) - 1) << 4) & PCI_MSI_FLAGS_QSIZE);
>>>> -#define is_64bit_address(control)	(!!(control & 
>>>> PCI_MSI_FLAGS_64BIT))
>>>> -#define is_mask_bit_support(control)	(!!(control &
>>>> PCI_MSI_FLAGS_MASKBIT))
>>>> +    (control) |= (((fls(num) - 1) << 4) & PCI_MSI_FLAGS_QSIZE);
>>> 
>>> And this, together with dropping the bogus semicolon?
>>> 
>> 
>> I'll drop the semicolon.
>> 
>>> There also look to be cases where MASK_EXTR() / MASK_INSR() would 
>>> want
>>> using,
>>> in favor of using open-coded numbers.
>> 
>> Yes, perhaps. However, the risk that I make some mistakes in doing so
>> are quite high, though.
> 
> Right, hence how I started my earlier reply. Question is - do we want 
> to
> go just half the way here, or would we better tidy things all in one 
> go?
> In the latter case I could see about getting to that (whether to take
> your patch as basis or instead do it from scratch isn't quite clear to
> me at this point).
> 
> Jan

How about I revise this patch with parentheses added where needed, as 
suggested earlier, and then you can submit a further cleanup patch to 
remove e.g. the open coding?

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

Re: [XEN PATCH 07/11] xen/page_alloc: address violations of MISRA C Rule 20.7

[Nicola Vetrini]

On 2024-03-26 16:35, Jan Beulich wrote:
> On 26.03.2024 16:27, Nicola Vetrini wrote:
>> On 2024-03-25 10:27, Jan Beulich wrote:
>>> On 22.03.2024 17:01, Nicola Vetrini wrote:
>>>> --- a/xen/common/page_alloc.c
>>>> +++ b/xen/common/page_alloc.c
>>>> @@ -150,7 +150,7 @@
>>>>  #include <asm/paging.h>
>>>>  #else
>>>>  #define p2m_pod_offline_or_broken_hit(pg) 0
>>> 
>>> Seeing this in context: Does Misra also have a rule demanding
>>> evaluation
>>> of macro arguments?
>> 
>> No such rule. There is one for unused function parameters, though.
> 
> Interesting. Are there no concerns regarding side effects not taking
> place, as one might expect when looking just at the call site?
> 

I don't know. Either it was never discussed or it never made it to the 
final revisions, I guess.

>>>> -#define p2m_pod_offline_or_broken_replace(pg) BUG_ON(pg != NULL)
>>>> +#define p2m_pod_offline_or_broken_replace(pg) BUG_ON((pg) != NULL)
>>> 
>>> Or easier
>>> 
>>> #define p2m_pod_offline_or_broken_replace(pg) BUG_ON(pg)
>>> 
>>> ?
>>> 
>> 
>> Good point. I'll modify it.
> 
> I can do that as well while committing. With that adjustment
> Acked-by: Jan Beulich <jbeulich@suse.com>
> 
> Jan

Thanks

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

Re: [XEN PATCH 09/11] x86/msi: address violation of MISRA C Rule 20.7 and coding style

[Nicola Vetrini]

>>>>> +#define msi_disable(control) (control) &= ~PCI_MSI_FLAGS_ENABLE
>>>> 
>>>> Doesn't this need an outer pair of parentheses, too?
>>>> 
>>> 
>>> Not necessarily.
>> 
>> And use of msi_disable() in another expression would then likely not 
>> do
>> what's expected?
>> 
> 
> Actually I just noticed that some of these macros are never used 
> (msi_disable being one of them), as far as I can tell.
> 

I see why. The bodies of the macros are open-coded in multiple places in 
msi.c. I can't really tell whether that's intentional or not, but this 
is an opportunity to do a more general cleanup I guess, beyond the scope 
of this patch.

>>> I'm in favour of a consistent style to be converted in.
>>> This also applies below.
>> 
>> I'm all for consistency; I just don't know what you want to be 
>> consistent
>> with, here.
>> 
> 
> I would propose adding parentheses around assignments to control, so 
> that all macros are consistently parenthesized.
> 
>>>>>  #define multi_msi_capable(control) \
>>>>> -	(1 << ((control & PCI_MSI_FLAGS_QMASK) >> 1))
>>>>> +    (1 << (((control) & PCI_MSI_FLAGS_QMASK) >> 1))
>>>>>  #define multi_msi_enable(control, num) \
>>>>> -	control |= (((fls(num) - 1) << 4) & PCI_MSI_FLAGS_QSIZE);
>>>>> -#define is_64bit_address(control)	(!!(control & 
>>>>> PCI_MSI_FLAGS_64BIT))
>>>>> -#define is_mask_bit_support(control)	(!!(control &
>>>>> PCI_MSI_FLAGS_MASKBIT))
>>>>> +    (control) |= (((fls(num) - 1) << 4) & PCI_MSI_FLAGS_QSIZE);
>>>> 
>>>> And this, together with dropping the bogus semicolon?
>>>> 
>>> 
>>> I'll drop the semicolon.
>>> 
>>>> There also look to be cases where MASK_EXTR() / MASK_INSR() would 
>>>> want
>>>> using,
>>>> in favor of using open-coded numbers.
>>> 
>>> Yes, perhaps. However, the risk that I make some mistakes in doing so
>>> are quite high, though.
>> 
>> Right, hence how I started my earlier reply. Question is - do we want 
>> to
>> go just half the way here, or would we better tidy things all in one 
>> go?
>> In the latter case I could see about getting to that (whether to take
>> your patch as basis or instead do it from scratch isn't quite clear to
>> me at this point).
>> 
>> Jan
> 
> How about I revise this patch with parentheses added where needed, as 
> suggested earlier, and then you can submit a further cleanup patch to 
> remove e.g. the open coding?

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

Re: [XEN PATCH 11/11] x86/public: hvm: address violations of MISRA C Rule 20.7

[Jan Beulich]

On 22.03.2024 17:02, 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>

Btw, I've taken the liberty to drop the (secondary) hvm: prefix from the
subject; I can't ...

> --- a/xen/include/public/arch-x86/xen.h
> +++ b/xen/include/public/arch-x86/xen.h
> @@ -36,7 +36,7 @@
>  #define __XEN_GUEST_HANDLE(name)        __guest_handle_ ## name
>  #define XEN_GUEST_HANDLE(name)          __XEN_GUEST_HANDLE(name)
>  #define XEN_GUEST_HANDLE_PARAM(name)    XEN_GUEST_HANDLE(name)
> -#define set_xen_guest_handle_raw(hnd, val)  do { (hnd).p = val; } while (0)
> +#define set_xen_guest_handle_raw(hnd, val)  do { (hnd).p = (val); } while (0)
>  #define set_xen_guest_handle(hnd, val) set_xen_guest_handle_raw(hnd, val)

... spot anything HVM-specific in this change.

Jan