[XEN PATCH 00/13] address violations of MISRA C:2012 Directive 4.10

[XEN PATCH 00/13] address violations of MISRA C:2012 Directive 4.10

[Simone Ballarin]

Add or move inclusion guards to address violations of
MISRA C:2012 Directive 4.10 ("Precautions shall be taken in order
to prevent the contents of a header file being included more than
once").

Inclusion guards must appear at the beginning of the headers
(comments are permitted anywhere) and the #if directive cannot
be used for other checks.

C files, if included somewhere, need to comply with the guideline.

Simone Ballarin (13):
  misra: add deviation for headers that explicitly avoid guards
  automation/eclair: add text-based deviation for empty headers
  xen/arm: address violations of MISRA C:2012 Directive 4.10
  xen/x86: address violations of MISRA C:2012 Directive 4.10
  automation/eclair: add deviation for usercopy.c
  x86/EFI: address violations of MISRA C:2012 Directive 4.10
  x86/asm: address violations of MISRA C:2012 Directive 4.10
  x86/mm: address violations of MISRA C:2012 Directive 4.10
  xen/common: address violations of MISRA C:2012 Directive 4.10
  xen/efi: address violations of MISRA C:2012 Directive 4.10
  xen/sched: address violations of MISRA C:2012 Directive 4.10
  xen: address violations of MISRA C:2012 Directive 4.10
  x86/asm: address violations of MISRA C:2012 Directive 4.10

 automation/eclair_analysis/ECLAIR/deviations.ecl | 9 +++++++++
 docs/misra/rules.rst                             | 5 ++++-
 xen/arch/arm/efi/efi-boot.h                      | 6 ++++++
 xen/arch/arm/include/asm/hypercall.h             | 6 +++---
 xen/arch/arm/include/asm/iocap.h                 | 6 +++---
 xen/arch/x86/Makefile                            | 8 ++++----
 xen/arch/x86/cpu/cpu.h                           | 5 +++++
 xen/arch/x86/efi/efi-boot.h                      | 6 ++++++
 xen/arch/x86/efi/runtime.h                       | 5 +++++
 xen/arch/x86/include/asm/compat.h                | 5 +++++
 xen/arch/x86/include/asm/cpufeatures.h           | 4 +---
 xen/arch/x86/include/asm/efibind.h               | 5 +++++
 xen/arch/x86/include/asm/hypercall.h             | 6 +++---
 xen/arch/x86/mm/guest_walk.c                     | 5 +++++
 xen/arch/x86/mm/hap/guest_walk.c                 | 4 ++++
 xen/arch/x86/physdev.c                           | 4 ++++
 xen/arch/x86/platform_hypercall.c                | 5 +++++
 xen/arch/x86/x86_64/compat/mm.c                  | 5 +++++
 xen/arch/x86/x86_64/mmconfig.h                   | 5 +++++
 xen/arch/x86/x86_emulate/private.h               | 5 +++++
 xen/arch/x86/x86_emulate/x86_emulate.c           | 5 +++++
 xen/common/compat/grant_table.c                  | 7 +++++++
 xen/common/coverage/gcc_4_7.c                    | 5 +++++
 xen/common/decompress.h                          | 5 +++++
 xen/common/efi/efi.h                             | 5 +++++
 xen/common/efi/runtime.c                         | 6 ++++++
 xen/common/event_channel.h                       | 5 +++++
 xen/common/multicall.c                           | 5 +++++
 xen/common/sched/compat.c                        | 6 ++++++
 xen/include/xen/err.h                            | 4 +++-
 xen/include/xen/pci_ids.h                        | 5 +++++
 xen/include/xen/softirq.h                        | 4 +++-
 xen/include/xen/unaligned.h                      | 7 ++++---
 xen/include/xen/vmap.h                           | 4 +++-
 xen/tools/compat-xlat-header.py                  | 2 ++
 35 files changed, 161 insertions(+), 23 deletions(-)

-- 
2.34.1

[XEN PATCH 01/13] misra: add deviation for headers that explicitly avoid guards

[Simone Ballarin]

Some headers, under specific circumstances (documented in a comment at
the beginning of the file), explicitly avoid inclusion guards: the caller
is responsible for including them correctly.

These files are not supposed to comply with Directive 4.10:
"Precautions shall be taken in order to prevent the contents of a header
file being included more than once"

This patch adds a deviation for all headers that contain the following
in a comment text:
"In this case, no inclusion guards apply and the caller is responsible"

Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>
---
 automation/eclair_analysis/ECLAIR/deviations.ecl | 4 ++++
 docs/misra/rules.rst                             | 3 ++-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl b/automation/eclair_analysis/ECLAIR/deviations.ecl
index d8170106b4..5f068377fa 100644
--- a/automation/eclair_analysis/ECLAIR/deviations.ecl
+++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
@@ -91,6 +91,10 @@ conform to the directive."
 -config=MC3R1.D4.10,reports+={safe, "first_area(text(^/\\* Generated file, do not edit! \\*/$, begin-3))"}
 -doc_end
 
+-doc_begin="Some headers, under specific circumstances, explicitly avoid inclusion guards."
+-config=MC3R1.D4.10,reports+={safe, "first_area(text(^ \\* In this case, no inclusion guards apply and the caller is responsible.*\\*/$, begin-1))"}
+-doc_end
+
 #
 # Series 5.
 #
diff --git a/docs/misra/rules.rst b/docs/misra/rules.rst
index db30632b93..4b1a7b02b6 100644
--- a/docs/misra/rules.rst
+++ b/docs/misra/rules.rst
@@ -60,7 +60,8 @@ maintainers if you want to suggest a change.
      - Precautions shall be taken in order to prevent the contents of a
        header file being included more than once
      - Files that are intended to be included more than once do not need to
-       conform to the directive
+       conform to the directive. Files that explicitly avoid inclusion guards
+       under specific circumstances do not need to conform the directive.
 
    * - `Dir 4.11 <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/D_04_11.c>`_
      - Required
-- 
2.34.1

[XEN PATCH 02/13] automation/eclair: add text-based deviation for empty headers

[Simone Ballarin]

This patch adds a text-based deviation for Directive 4.10:
"Precautions shall be taken in order to prevent the contents of
a header file being included more than once"

Headers starting with the following comment are not supposed to
comply with the directive:
"/* empty */"

These headers should be empty, therefore they pose no risk if included
more than once.

Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>
---
 automation/eclair_analysis/ECLAIR/deviations.ecl | 1 +
 1 file changed, 1 insertion(+)

diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl b/automation/eclair_analysis/ECLAIR/deviations.ecl
index 5f068377fa..2681a4cff5 100644
--- a/automation/eclair_analysis/ECLAIR/deviations.ecl
+++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
@@ -80,6 +80,7 @@ inline functions."
 
 -doc_begin="This header file is autogenerated or empty, therefore it poses no
 risk if included more than once."
+-config=MC3R1.D4.10,reports+={safe, "first_area(text(^/\\* empty \\*/$, begin-1))"}
 -file_tag+={empty_header, "^xen/arch/arm/efi/runtime\\.h$"}
 -file_tag+={autogen_headers, "^xen/include/xen/compile\\.h$||^xen/include/generated/autoconf.h$||^xen/include/xen/hypercall-defs.h$"}
 -config=MC3R1.D4.10,reports+={safe, "all_area(all_loc(file(empty_header||autogen_headers)))"}
-- 
2.34.1

[XEN PATCH 03/13] xen/arm: address violations of MISRA C:2012 Directive 4.10

[Simone Ballarin]

Add or move inclusion guards to address violations of
MISRA C:2012 Directive 4.10 ("Precautions shall be taken in order
to prevent the contents of a header file being included more than
once").

Inclusion guards must appear at the beginning of the headers
(comments are permitted anywhere).

Mechanical change.

Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>
---
 xen/arch/arm/efi/efi-boot.h          | 6 ++++++
 xen/arch/arm/include/asm/hypercall.h | 6 +++---
 xen/arch/arm/include/asm/iocap.h     | 6 +++---
 3 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
index 1c3640bb65..aba522ead5 100644
--- a/xen/arch/arm/efi/efi-boot.h
+++ b/xen/arch/arm/efi/efi-boot.h
@@ -3,6 +3,10 @@
  * is intended to be included by common/efi/boot.c _only_, and
  * therefore can define arch specific global variables.
  */
+
+#ifndef __ARM_EFI_EFI_BOOT_H__
+#define __ARM_EFI_EFI_BOOT_H__
+
 #include <xen/device_tree.h>
 #include <xen/libfdt/libfdt.h>
 #include <asm/setup.h>
@@ -1003,6 +1007,8 @@ static void __init efi_arch_flush_dcache_area(const void *vaddr, UINTN size)
     __flush_dcache_area(vaddr, size);
 }
 
+#endif /* __ARM_EFI_EFI_BOOT_H__*/
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/arch/arm/include/asm/hypercall.h b/xen/arch/arm/include/asm/hypercall.h
index ccd26c5184..4f4d96f1f2 100644
--- a/xen/arch/arm/include/asm/hypercall.h
+++ b/xen/arch/arm/include/asm/hypercall.h
@@ -1,10 +1,10 @@
+#ifndef __ASM_ARM_HYPERCALL_H__
+#define __ASM_ARM_HYPERCALL_H__
+
 #ifndef __XEN_HYPERCALL_H__
 #error "asm/hypercall.h should not be included directly - include xen/hypercall.h instead"
 #endif
 
-#ifndef __ASM_ARM_HYPERCALL_H__
-#define __ASM_ARM_HYPERCALL_H__
-
 #include <public/domctl.h> /* for arch_do_domctl */
 
 long subarch_do_domctl(struct xen_domctl *domctl, struct domain *d,
diff --git a/xen/arch/arm/include/asm/iocap.h b/xen/arch/arm/include/asm/iocap.h
index 276fefbc59..4db1b16839 100644
--- a/xen/arch/arm/include/asm/iocap.h
+++ b/xen/arch/arm/include/asm/iocap.h
@@ -1,10 +1,10 @@
-#ifndef __X86_IOCAP_H__
-#define __X86_IOCAP_H__
+#ifndef __ASM_ARM_IOCAP_H__
+#define __ASM_ARM_IOCAP_H__
 
 #define cache_flush_permitted(d)                        \
     (!rangeset_is_empty((d)->iomem_caps))
 
-#endif
+#endif /* __ASM_ARM_IOCAP_H__ */
 
 /*
  * Local variables:
-- 
2.34.1

[XEN PATCH 04/13] xen/x86: address violations of MISRA C:2012 Directive 4.10

[Simone Ballarin]

Add or move inclusion guards to address violations of
MISRA C:2012 Directive 4.10 ("Precautions shall be taken in order
to prevent the contents of a header file being included more than
once").

Inclusion guards must appear at the beginning of the headers
(comments are permitted anywhere).

Also C files, if included somewhere, need to comply with the guideline.

Mechanical change.

Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>
---
 xen/arch/x86/Makefile                  | 8 ++++----
 xen/arch/x86/cpu/cpu.h                 | 5 +++++
 xen/arch/x86/physdev.c                 | 4 ++++
 xen/arch/x86/platform_hypercall.c      | 5 +++++
 xen/arch/x86/x86_64/compat/mm.c        | 5 +++++
 xen/arch/x86/x86_64/mmconfig.h         | 5 +++++
 xen/arch/x86/x86_emulate/private.h     | 5 +++++
 xen/arch/x86/x86_emulate/x86_emulate.c | 5 +++++
 8 files changed, 38 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
index e642ad6c55..f956b7f0cd 100644
--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -259,17 +259,17 @@ $(objtree)/arch/x86/include/asm/asm-macros.h: $(obj)/asm-macros.i $(src)/Makefil
 	$(call filechk,asm-macros.h)
 
 define filechk_asm-macros.h
+    echo '#ifndef __ASM_MACROS_H__'; \
+    echo '#define __ASM_MACROS_H__'; \
     echo '#if 0'; \
     echo '.if 0'; \
     echo '#endif'; \
-    echo '#ifndef __ASM_MACROS_H__'; \
-    echo '#define __ASM_MACROS_H__'; \
     echo 'asm ( ".include \"$@\"" );'; \
-    echo '#endif /* __ASM_MACROS_H__ */'; \
     echo '#if 0'; \
     echo '.endif'; \
     cat $<; \
-    echo '#endif'
+    echo '#endif'; \
+    echo '#endif /* __ASM_MACROS_H__ */'
 endef
 
 $(obj)/efi.lds: AFLAGS-y += -DEFI
diff --git a/xen/arch/x86/cpu/cpu.h b/xen/arch/x86/cpu/cpu.h
index e3d06278b3..95939c7fb6 100644
--- a/xen/arch/x86/cpu/cpu.h
+++ b/xen/arch/x86/cpu/cpu.h
@@ -1,3 +1,6 @@
+#ifndef __X86_CPU_CPU_H__
+#define __X86_CPU_CPU_H__
+
 /* attempt to consolidate cpu attributes */
 struct cpu_dev {
 	void		(*c_early_init)(struct cpuinfo_x86 *c);
@@ -24,3 +27,5 @@ void amd_init_lfence(struct cpuinfo_x86 *c);
 void amd_init_ssbd(const struct cpuinfo_x86 *c);
 void amd_init_spectral_chicken(void);
 void detect_zen2_null_seg_behaviour(void);
+
+#endif /* __X86_CPU_CPU_H__ */
diff --git a/xen/arch/x86/physdev.c b/xen/arch/x86/physdev.c
index 2f1d955a96..08b391d8f3 100644
--- a/xen/arch/x86/physdev.c
+++ b/xen/arch/x86/physdev.c
@@ -1,3 +1,5 @@
+#ifndef  __X86_PHYSDEV_C__
+#define  __X86_PHYSDEV_C__
 
 #include <xen/init.h>
 #include <xen/lib.h>
@@ -623,6 +625,8 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
     return ret;
 }
 
+#endif /* __X86_PHYSDEV_C__ */
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/arch/x86/platform_hypercall.c b/xen/arch/x86/platform_hypercall.c
index 9ff2da8fc3..11aa084887 100644
--- a/xen/arch/x86/platform_hypercall.c
+++ b/xen/arch/x86/platform_hypercall.c
@@ -6,6 +6,9 @@
  * Copyright (c) 2002-2006, K Fraser
  */
 
+#ifndef __X86_PLATFORM_HYPERCALL_C__
+#define __X86_PLATFORM_HYPERCALL_C__
+
 #include <xen/types.h>
 #include <xen/lib.h>
 #include <xen/mm.h>
@@ -899,6 +902,8 @@ ret_t do_platform_op(
     return ret;
 }
 
+#endif /* __X86_PLATFORM_HYPERCALL_C__ */
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/arch/x86/x86_64/compat/mm.c b/xen/arch/x86/x86_64/compat/mm.c
index d54efaad21..24f7eb8788 100644
--- a/xen/arch/x86/x86_64/compat/mm.c
+++ b/xen/arch/x86/x86_64/compat/mm.c
@@ -1,3 +1,6 @@
+#ifndef __X86_X86_64_COMPAT_MM_C__
+#define __X86_X86_64_COMPAT_MM_C__
+
 #include <xen/event.h>
 #include <xen/hypercall.h>
 #include <xen/mem_access.h>
@@ -326,6 +329,8 @@ int compat_mmuext_op(
 }
 #endif /* CONFIG_PV */
 
+#endif /* __X86_X86_64_COMPAT_MM_C__ */
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/arch/x86/x86_64/mmconfig.h b/xen/arch/x86/x86_64/mmconfig.h
index 2d49fc79a0..c562879c76 100644
--- a/xen/arch/x86/x86_64/mmconfig.h
+++ b/xen/arch/x86/x86_64/mmconfig.h
@@ -5,6 +5,9 @@
  * Author: Allen Kay <allen.m.kay@intel.com> - adapted from linux
  */
 
+#ifndef __X86_X86_64_MMCONFIG_H__
+#define __X86_X86_64_MMCONFIG_H__
+
 #define PCI_DEVICE_ID_INTEL_E7520_MCH    0x3590
 #define PCI_DEVICE_ID_INTEL_82945G_HB    0x2770
 
@@ -72,3 +75,5 @@ int pci_mmcfg_reserved(uint64_t address, unsigned int segment,
 int pci_mmcfg_arch_init(void);
 int pci_mmcfg_arch_enable(unsigned int);
 void pci_mmcfg_arch_disable(unsigned int);
+
+#endif /* __X86_X86_64_MMCONFIG_H__ */
diff --git a/xen/arch/x86/x86_emulate/private.h b/xen/arch/x86/x86_emulate/private.h
index 719dad59cd..ffa134f297 100644
--- a/xen/arch/x86/x86_emulate/private.h
+++ b/xen/arch/x86/x86_emulate/private.h
@@ -6,6 +6,9 @@
  * Copyright (c) 2005-2007 XenSource Inc.
  */
 
+#ifndef __X86_X86_EMULATE_PRIVATE_H__
+#define __X86_X86_EMULATE_PRIVATE_H__
+
 #ifdef __XEN__
 
 # include <xen/kernel.h>
@@ -831,3 +834,5 @@ static inline int read_ulong(enum x86_segment seg,
     *val = 0;
     return ops->read(seg, offset, val, bytes, ctxt);
 }
+
+#endif /* __X86_X86_EMULATE_PRIVATE_H__ */
diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c b/xen/arch/x86/x86_emulate/x86_emulate.c
index e88245eae9..8977a1b82e 100644
--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -8,6 +8,9 @@
  * Copyright (c) 2005-2007 XenSource Inc.
  */
 
+#ifndef __X86_X86_EMULATE_EMULATE_C__
+#define __X86_X86_EMULATE_EMULATE_C__
+
 #include "private.h"
 
 /*
@@ -8678,3 +8681,5 @@ int x86_emulate_wrapper(
     return rc;
 }
 #endif
+
+#endif /* __X86_X86_EMULATE_EMULATE_C__ */
-- 
2.34.1

[XEN PATCH 05/13] automation/eclair: add deviation for usercopy.c

[Simone Ballarin]

xen/arch/x86/usercopy.c includes itself, so it is not supposed to
comply with Directive 4.10:
"Precautions shall be taken in order to prevent the contents of a
header file being included more than once"

This patch adds a deviation for the file.

Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>
---
 automation/eclair_analysis/ECLAIR/deviations.ecl | 4 ++++
 docs/misra/rules.rst                             | 2 ++
 2 files changed, 6 insertions(+)

diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl b/automation/eclair_analysis/ECLAIR/deviations.ecl
index 2681a4cff5..a7d4f29b43 100644
--- a/automation/eclair_analysis/ECLAIR/deviations.ecl
+++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
@@ -96,6 +96,10 @@ conform to the directive."
 -config=MC3R1.D4.10,reports+={safe, "first_area(text(^ \\* In this case, no inclusion guards apply and the caller is responsible.*\\*/$, begin-1))"}
 -doc_end
 
+-doc_begin="xen/arch/x86/usercopy.c includes itself: it is not supposed to comply with the directive"
+-config=MC3R1.D4.10,reports+={deliberate, "all_area(all_loc(file("^xen/arch/x86/usercopy\\.c$")))"}
+-doc_end
+
 #
 # Series 5.
 #
diff --git a/docs/misra/rules.rst b/docs/misra/rules.rst
index 4b1a7b02b6..45e13d0302 100644
--- a/docs/misra/rules.rst
+++ b/docs/misra/rules.rst
@@ -62,6 +62,8 @@ maintainers if you want to suggest a change.
      - Files that are intended to be included more than once do not need to
        conform to the directive. Files that explicitly avoid inclusion guards
        under specific circumstances do not need to conform the directive.
+       xen/arch/x86/usercopy.c includes itself: it is not supposed to comply
+       with the directive.
 
    * - `Dir 4.11 <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/D_04_11.c>`_
      - Required
-- 
2.34.1

[XEN PATCH 06/13] x86/EFI: address violations of MISRA C:2012 Directive 4.10

[Simone Ballarin]

Add inclusion guards to address violations of
MISRA C:2012 Directive 4.10 ("Precautions shall be taken in order
to prevent the contents of a header file being included more than
once").

Mechanical change.

Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>
---
 xen/arch/x86/efi/efi-boot.h | 6 ++++++
 xen/arch/x86/efi/runtime.h  | 5 +++++
 2 files changed, 11 insertions(+)

diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
index 92f4cfe8bd..2c6be062cc 100644
--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -3,6 +3,10 @@
  * is intended to be included by common/efi/boot.c _only_, and
  * therefore can define arch specific global variables.
  */
+
+#ifndef __X86_EFI_EFI_BOOT_H__
+#define __X86_EFI_EFI_BOOT_H__
+
 #include <xen/vga.h>
 #include <asm/e820.h>
 #include <asm/edd.h>
@@ -913,6 +917,8 @@ void __init efi_multiboot2(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable
     efi_exit_boot(ImageHandle, SystemTable);
 }
 
+#endif /* __X86_EFI_EFI_BOOT_H__ */
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/arch/x86/efi/runtime.h b/xen/arch/x86/efi/runtime.h
index 77866c5f21..10b36bcb89 100644
--- a/xen/arch/x86/efi/runtime.h
+++ b/xen/arch/x86/efi/runtime.h
@@ -1,3 +1,6 @@
+#ifndef __X86_EFI_RUNTIME_H__
+#define __X86_EFI_RUNTIME_H__
+
 #include <xen/domain_page.h>
 #include <xen/mm.h>
 #include <asm/atomic.h>
@@ -17,3 +20,5 @@ void efi_update_l4_pgtable(unsigned int l4idx, l4_pgentry_t l4e)
     }
 }
 #endif
+
+#endif /* __X86_EFI_RUNTIME_H__ */
-- 
2.34.1

[XEN PATCH 07/13] x86/asm: address violations of MISRA C:2012 Directive 4.10

[Simone Ballarin]

Add or move inclusion guards to address violations of
MISRA C:2012 Directive 4.10 ("Precautions shall be taken in order
to prevent the contents of a header file being included more than
once").

Inclusion guards must appear at the beginning of the headers
(comments are permitted anywhere).

The text of the beggining comment of cpufeatures.h has been changed
to match the deviation in automation/eclair_analysis/ECLAIR/deviations.ecl,
moreover this new formulation is already used in other files.

Mechanical change.

Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>
---
 xen/arch/x86/include/asm/compat.h      | 5 +++++
 xen/arch/x86/include/asm/cpufeatures.h | 4 +---
 xen/arch/x86/include/asm/efibind.h     | 5 +++++
 xen/arch/x86/include/asm/hypercall.h   | 6 +++---
 4 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/xen/arch/x86/include/asm/compat.h b/xen/arch/x86/include/asm/compat.h
index 818cad87db..3d3891d061 100644
--- a/xen/arch/x86/include/asm/compat.h
+++ b/xen/arch/x86/include/asm/compat.h
@@ -2,6 +2,9 @@
  * compat.h
  */
 
+#ifndef __ASM_X86_COMPAT_H__
+#define __ASM_X86_COMPAT_H__
+
 #ifdef CONFIG_COMPAT
 
 #define COMPAT_BITS_PER_LONG 32
@@ -18,3 +21,5 @@ int switch_compat(struct domain *);
 #include <xen/errno.h>
 static inline int switch_compat(struct domain *d) { return -EOPNOTSUPP; }
 #endif
+
+#endif /* __ASM_X86_COMPAT_H__ */
diff --git a/xen/arch/x86/include/asm/cpufeatures.h b/xen/arch/x86/include/asm/cpufeatures.h
index da0593de85..1dfdd478ab 100644
--- a/xen/arch/x86/include/asm/cpufeatures.h
+++ b/xen/arch/x86/include/asm/cpufeatures.h
@@ -1,6 +1,4 @@
-/*
- * Explicitly intended for multiple inclusion.
- */
+/* This file is legitimately included multiple times */
 
 #include <xen/lib/x86/cpuid-autogen.h>
 
diff --git a/xen/arch/x86/include/asm/efibind.h b/xen/arch/x86/include/asm/efibind.h
index bce02f3707..f2eb8b5496 100644
--- a/xen/arch/x86/include/asm/efibind.h
+++ b/xen/arch/x86/include/asm/efibind.h
@@ -1,2 +1,7 @@
+#ifndef __ASM_X86_EFIBIND_H__
+#define __ASM_X86_EFIBIND_H__
+
 #include <xen/types.h>
 #include <asm/x86_64/efibind.h>
+
+#endif /* __ASM_X86_EFIBIND_H__ */
diff --git a/xen/arch/x86/include/asm/hypercall.h b/xen/arch/x86/include/asm/hypercall.h
index ec2edc771e..2ade5d71b8 100644
--- a/xen/arch/x86/include/asm/hypercall.h
+++ b/xen/arch/x86/include/asm/hypercall.h
@@ -2,13 +2,13 @@
  * asm-x86/hypercall.h
  */
 
+#ifndef __ASM_X86_HYPERCALL_H__
+#define __ASM_X86_HYPERCALL_H__
+
 #ifndef __XEN_HYPERCALL_H__
 #error "asm/hypercall.h should not be included directly - include xen/hypercall.h instead"
 #endif
 
-#ifndef __ASM_X86_HYPERCALL_H__
-#define __ASM_X86_HYPERCALL_H__
-
 #include <xen/types.h>
 #include <public/physdev.h>
 #include <public/event_channel.h>
-- 
2.34.1

[XEN PATCH 08/13] x86/mm: address violations of MISRA C:2012 Directive 4.10

[Simone Ballarin]

Add inclusion guards to address violations of
MISRA C:2012 Directive 4.10 ("Precautions shall be taken in order
to prevent the contents of a header file being included more than
once").

C files, if included somewhere, need to comply with the guideline.

Mechanical change.

Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>
---
 xen/arch/x86/mm/guest_walk.c     | 5 +++++
 xen/arch/x86/mm/hap/guest_walk.c | 4 ++++
 2 files changed, 9 insertions(+)

diff --git a/xen/arch/x86/mm/guest_walk.c b/xen/arch/x86/mm/guest_walk.c
index fe7393334f..66c127156d 100644
--- a/xen/arch/x86/mm/guest_walk.c
+++ b/xen/arch/x86/mm/guest_walk.c
@@ -9,6 +9,9 @@
  * Parts based on earlier work by Michael A Fetterman, Ian Pratt et al.
  */
 
+#ifndef __X86_MM_GUEST_WALK_C__
+#define __X86_MM_GUEST_WALK_C__
+
 #include <xen/types.h>
 #include <xen/mm.h>
 #include <xen/paging.h>
@@ -576,6 +579,8 @@ void *map_domain_gfn(struct p2m_domain *p2m, gfn_t gfn, mfn_t *mfn,
 }
 #endif
 
+#endif /* __X86_MM_GUEST_WALK_C__ */
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/arch/x86/mm/hap/guest_walk.c b/xen/arch/x86/mm/hap/guest_walk.c
index d1b7c5762c..d4ffa8141f 100644
--- a/xen/arch/x86/mm/hap/guest_walk.c
+++ b/xen/arch/x86/mm/hap/guest_walk.c
@@ -7,6 +7,9 @@
  * Copyright (c) 2007, XenSource Inc.
  */
 
+#ifndef __X86_MM_HAP_GUEST_WALK_C__
+#define __X86_MM_HAP_GUEST_WALK_C__
+
 #include <xen/domain_page.h>
 #include <xen/paging.h>
 #include <xen/sched.h>
@@ -124,6 +127,7 @@ unsigned long cf_check hap_p2m_ga_to_gfn(GUEST_PAGING_LEVELS)(
     return gfn_x(INVALID_GFN);
 }
 
+#endif /* __X86_MM_HAP_GUEST_WALK_C__ */
 
 /*
  * Local variables:
-- 
2.34.1

[XEN PATCH 09/13] xen/common: address violations of MISRA C:2012 Directive 4.10

[Simone Ballarin]

Add inclusion guards to address violations of
MISRA C:2012 Directive 4.10 ("Precautions shall be taken in order
to prevent the contents of a header file being included more than
once").

Also C files, if included somewhere, need to comply with the guideline.

Mechanical change.

Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>
---
 xen/common/compat/grant_table.c | 7 +++++++
 xen/common/coverage/gcc_4_7.c   | 5 +++++
 xen/common/decompress.h         | 5 +++++
 xen/common/event_channel.h      | 5 +++++
 xen/common/multicall.c          | 5 +++++
 5 files changed, 27 insertions(+)

diff --git a/xen/common/compat/grant_table.c b/xen/common/compat/grant_table.c
index f8177c84c0..614ad71a59 100644
--- a/xen/common/compat/grant_table.c
+++ b/xen/common/compat/grant_table.c
@@ -3,6 +3,10 @@
  *
  */
 
+
+#ifndef __COMMON_COMPAT_GRANT_TABLE_C__
+#define __COMMON_COMPAT_GRANT_TABLE_C__
+
 #include <xen/hypercall.h>
 #include <compat/grant_table.h>
 
@@ -331,6 +335,9 @@ int compat_grant_table_op(
     return rc;
 }
 
+
+#endif /* __COMMON_COMPAT_GRANT_TABLE_C__ */
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/common/coverage/gcc_4_7.c b/xen/common/coverage/gcc_4_7.c
index 25b4a8bcdc..12e4ec8cbb 100644
--- a/xen/common/coverage/gcc_4_7.c
+++ b/xen/common/coverage/gcc_4_7.c
@@ -14,6 +14,9 @@
  *    Wei Liu <wei.liu2@citrix.com>
  */
 
+#ifndef __COMMON_COVERAGE_GCC_4_7_C__
+#define __COMMON_COVERAGE_GCC_4_7_C__
+
 #include <xen/string.h>
 
 #include "gcov.h"
@@ -193,6 +196,8 @@ size_t gcov_info_to_gcda(char *buffer, const struct gcov_info *info)
     return pos;
 }
 
+#endif /* __COMMON_COVERAGE_GCC_4_7_C__ */
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/common/decompress.h b/xen/common/decompress.h
index e8195b353a..da3c3abb6a 100644
--- a/xen/common/decompress.h
+++ b/xen/common/decompress.h
@@ -1,3 +1,6 @@
+#ifndef __COMMON_DECOMPRESS_H__
+#define __COMMON_DECOMPRESS_H__
+
 #ifdef __XEN__
 
 #include <xen/cache.h>
@@ -23,3 +26,5 @@
 #define large_free free
 
 #endif
+
+#endif /* __COMMON_DECOMPRESS_H__ */
diff --git a/xen/common/event_channel.h b/xen/common/event_channel.h
index 45219ca67c..040bad77f9 100644
--- a/xen/common/event_channel.h
+++ b/xen/common/event_channel.h
@@ -1,5 +1,8 @@
 /* Event channel handling private header. */
 
+#ifndef __COMMON_EVENT_CHANNEL_H__
+#define __COMMON_EVENT_CHANNEL_H__
+
 #include <xen/event.h>
 
 static inline unsigned int max_evtchns(const struct domain *d)
@@ -52,6 +55,8 @@ int evtchn_fifo_init_control(struct evtchn_init_control *init_control);
 int evtchn_fifo_expand_array(const struct evtchn_expand_array *expand_array);
 void evtchn_fifo_destroy(struct domain *d);
 
+#endif /* __COMMON_EVENT_CHANNEL_H__ */
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/common/multicall.c b/xen/common/multicall.c
index 1f0cc4cb26..421bb25b70 100644
--- a/xen/common/multicall.c
+++ b/xen/common/multicall.c
@@ -2,6 +2,9 @@
  * multicall.c
  */
 
+#ifndef __COMMON_MULTICALL_C__
+#define __COMMON_MULTICALL_C__
+
 #include <xen/types.h>
 #include <xen/lib.h>
 #include <xen/mm.h>
@@ -124,6 +127,8 @@ ret_t do_multicall(
         __HYPERVISOR_multicall, "hi", call_list, nr_calls-i);
 }
 
+#endif /* __COMMON_MULTICALL_C__ */
+
 /*
  * Local variables:
  * mode: C
-- 
2.34.1

[XEN PATCH 10/13] xen/efi: address violations of MISRA C:2012 Directive 4.10

[Simone Ballarin]

Add inclusion guards to address violations of
MISRA C:2012 Directive 4.10 ("Precautions shall be taken in order
to prevent the contents of a header file being included more than
once").

Also C files, if included somewhere, need to comply with the guideline.

Mechanical change.

Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>
---
 xen/common/efi/efi.h     | 5 +++++
 xen/common/efi/runtime.c | 6 ++++++
 2 files changed, 11 insertions(+)

diff --git a/xen/common/efi/efi.h b/xen/common/efi/efi.h
index c02fbb7b69..cef9381d30 100644
--- a/xen/common/efi/efi.h
+++ b/xen/common/efi/efi.h
@@ -1,3 +1,6 @@
+#ifndef __COMMON_EFI_EFI_H__
+#define __COMMON_EFI_EFI_H__
+
 #include <asm/efibind.h>
 #include <efi/efidef.h>
 #include <efi/efierr.h>
@@ -51,3 +54,5 @@ void free_ebmalloc_unused_mem(void);
 
 const void *pe_find_section(const void *image, const UINTN image_size,
                             const CHAR16 *section_name, UINTN *size_out);
+
+#endif /* __COMMON_EFI_EFI_H__ */
diff --git a/xen/common/efi/runtime.c b/xen/common/efi/runtime.c
index 5cb7504c96..fb6fd17ba3 100644
--- a/xen/common/efi/runtime.c
+++ b/xen/common/efi/runtime.c
@@ -6,6 +6,10 @@
 #include <xen/irq.h>
 #include <xen/time.h>
 
+#ifndef __COMMON_EFI_RUNTIME_C__
+#define __COMMON_EFI_RUNTIME_C__
+
+
 DEFINE_XEN_GUEST_HANDLE(CHAR16);
 
 struct efi_rs_state {
@@ -704,3 +708,5 @@ int efi_runtime_call(struct xenpf_efi_runtime_call *op)
     return rc;
 }
 #endif
+
+#endif /* __COMMON_EFI_RUNTIME_C__ */
-- 
2.34.1

[XEN PATCH 11/13] xen/sched: address violations of MISRA C:2012 Directive 4.10

[Simone Ballarin]

Add inclusion guards to address violations of
MISRA C:2012 Directive 4.10 ("Precautions shall be taken in order
to prevent the contents of a header file being included more than
once").

Mechanical change.

Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>
---
 xen/common/sched/compat.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/xen/common/sched/compat.c b/xen/common/sched/compat.c
index a596e3a226..d718e450d4 100644
--- a/xen/common/sched/compat.c
+++ b/xen/common/sched/compat.c
@@ -3,6 +3,10 @@
  *
  */
 
+#ifndef __COMMON_SCHED_COMPAT_C__
+#define __COMMON_SCHED_COMPAT_C__
+
+
 #include <compat/sched.h>
 
 #define COMPAT
@@ -44,6 +48,8 @@ int compat_set_timer_op(uint32_t lo, int32_t hi)
     return do_set_timer_op(((s64)hi << 32) | lo);
 }
 
+#endif /* __COMMON_SCHED_COMPAT_C__ */
+
 /*
  * Local variables:
  * mode: C
-- 
2.34.1

[XEN PATCH 12/13] xen: address violations of MISRA C:2012 Directive 4.10

[Simone Ballarin]

Move or amended inclusion guards to address violations of
MISRA C:2012 Directive 4.10 ("Precautions shall be taken in order
to prevent the contents of a header file being included more than
once").

Inclusion guards must appear at the beginning of the headers
(comments are permitted anywhere) and the #if directive cannot
be used for other checks.

Mechanical change.

Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>
---
 xen/include/xen/err.h       | 4 +++-
 xen/include/xen/pci_ids.h   | 5 +++++
 xen/include/xen/softirq.h   | 4 +++-
 xen/include/xen/unaligned.h | 7 ++++---
 xen/include/xen/vmap.h      | 4 +++-
 5 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/xen/include/xen/err.h b/xen/include/xen/err.h
index 2f29b57d28..a6323d82d7 100644
--- a/xen/include/xen/err.h
+++ b/xen/include/xen/err.h
@@ -1,5 +1,6 @@
-#if !defined(__XEN_ERR_H__) && !defined(__ASSEMBLY__)
+#if !defined(__XEN_ERR_H__)
 #define __XEN_ERR_H__
+#if !defined(__ASSEMBLY__)
 
 #include <xen/compiler.h>
 #include <xen/errno.h>
@@ -54,4 +55,5 @@ static inline int __must_check PTR_RET(const void *ptr)
 	return IS_ERR(ptr) ? PTR_ERR(ptr) : 0;
 }
 
+#endif /* __ASSEMBLY__ */
 #endif /* __XEN_ERR_H__ */
diff --git a/xen/include/xen/pci_ids.h b/xen/include/xen/pci_ids.h
index e798477a7e..1a739d4c92 100644
--- a/xen/include/xen/pci_ids.h
+++ b/xen/include/xen/pci_ids.h
@@ -1,3 +1,6 @@
+#ifndef __XEN_PCI_IDS_H__
+#define __XEN_PCI_IDS_H__
+
 #define PCI_VENDOR_ID_AMD                0x1022
 
 #define PCI_VENDOR_ID_NVIDIA             0x10de
@@ -11,3 +14,5 @@
 #define PCI_VENDOR_ID_BROADCOM           0x14e4
 
 #define PCI_VENDOR_ID_INTEL              0x8086
+
+#endif /* __XEN_PCI_IDS_H__ */
diff --git a/xen/include/xen/softirq.h b/xen/include/xen/softirq.h
index 33d6f2ecd2..092ec733b7 100644
--- a/xen/include/xen/softirq.h
+++ b/xen/include/xen/softirq.h
@@ -1,5 +1,6 @@
-#if !defined(__XEN_SOFTIRQ_H__) && !defined(__ASSEMBLY__)
+#if !defined(__XEN_SOFTIRQ_H__)
 #define __XEN_SOFTIRQ_H__
+#if !defined(__ASSEMBLY__)
 
 /* Low-latency softirqs come first in the following list. */
 enum {
@@ -40,4 +41,5 @@ void cpu_raise_softirq_batch_finish(void);
  */
 void process_pending_softirqs(void);
 
+#endif /* __ASSEMBLY__ */
 #endif /* __XEN_SOFTIRQ_H__ */
diff --git a/xen/include/xen/unaligned.h b/xen/include/xen/unaligned.h
index 0a2b16d05d..45f03b3f1b 100644
--- a/xen/include/xen/unaligned.h
+++ b/xen/include/xen/unaligned.h
@@ -3,13 +3,14 @@
  * without faulting, and at least reasonably efficiently.  Other architectures
  * will need to have a custom asm/unaligned.h.
  */
-#ifndef __ASM_UNALIGNED_H__
-#error "xen/unaligned.h should not be included directly - include asm/unaligned.h instead"
-#endif
 
 #ifndef __XEN_UNALIGNED_H__
 #define __XEN_UNALIGNED_H__
 
+#ifndef __ASM_UNALIGNED_H__
+#error "xen/unaligned.h should not be included directly - include asm/unaligned.h instead"
+#endif
+
 #ifdef __XEN__
 #include <xen/types.h>
 #include <asm/byteorder.h>
diff --git a/xen/include/xen/vmap.h b/xen/include/xen/vmap.h
index b0f7632e89..7a61dea54a 100644
--- a/xen/include/xen/vmap.h
+++ b/xen/include/xen/vmap.h
@@ -1,5 +1,6 @@
-#if !defined(__XEN_VMAP_H__) && defined(VMAP_VIRT_START)
+#if !defined(__XEN_VMAP_H__)
 #define __XEN_VMAP_H__
+#if defined(VMAP_VIRT_START)
 
 #include <xen/mm-frame.h>
 #include <xen/page-size.h>
@@ -38,4 +39,5 @@ static inline void vm_init(void)
     vm_init_type(VMAP_DEFAULT, (void *)VMAP_VIRT_START, arch_vmap_virt_end());
 }
 
+#endif /* VMAP_VIRT_START */
 #endif /* __XEN_VMAP_H__ */
-- 
2.34.1

[XEN PATCH 13/13] x86/asm: address violations of MISRA C:2012 Directive 4.10

[Simone Ballarin]

Amend generation script to address a violation of
MISRA C:2012 Directive 4.10 ("Precautions shall be taken in order
to prevent the contents of a header file being included more than
once").

This patch adds a special comment to the beginning of the header
to make it explicit that the file is generated automatically.

The comment is recognized by ECLAIR and will cause the deviation of
the violation.

Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>
---
 xen/tools/compat-xlat-header.py | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/xen/tools/compat-xlat-header.py b/xen/tools/compat-xlat-header.py
index 2b805b23a8..9e336277ac 100644
--- a/xen/tools/compat-xlat-header.py
+++ b/xen/tools/compat-xlat-header.py
@@ -406,6 +406,8 @@ def main():
             line = line.strip()
             header_tokens += re_tokenazier.split(line)
 
+    print("/* Generated file, do not edit! */")
+
     with open(sys.argv[2]) as compat_list:
         for line in compat_list:
             words = re_tokenazier.split(line, maxsplit=1)
-- 
2.34.1

Re: [XEN PATCH 01/13] misra: add deviation for headers that explicitly avoid guards

[Stefano Stabellini]

On Mon, 28 Aug 2023, Simone Ballarin wrote:
> Some headers, under specific circumstances (documented in a comment at
> the beginning of the file), explicitly avoid inclusion guards: the caller
> is responsible for including them correctly.
> 
> These files are not supposed to comply with Directive 4.10:
> "Precautions shall be taken in order to prevent the contents of a header
> file being included more than once"
> 
> This patch adds a deviation for all headers that contain the following
> in a comment text:
> "In this case, no inclusion guards apply and the caller is responsible"
> 
> Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>

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


> ---
>  automation/eclair_analysis/ECLAIR/deviations.ecl | 4 ++++
>  docs/misra/rules.rst                             | 3 ++-
>  2 files changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl b/automation/eclair_analysis/ECLAIR/deviations.ecl
> index d8170106b4..5f068377fa 100644
> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl
> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
> @@ -91,6 +91,10 @@ conform to the directive."
>  -config=MC3R1.D4.10,reports+={safe, "first_area(text(^/\\* Generated file, do not edit! \\*/$, begin-3))"}
>  -doc_end
>  
> +-doc_begin="Some headers, under specific circumstances, explicitly avoid inclusion guards."
> +-config=MC3R1.D4.10,reports+={safe, "first_area(text(^ \\* In this case, no inclusion guards apply and the caller is responsible.*\\*/$, begin-1))"}
> +-doc_end
> +
>  #
>  # Series 5.
>  #
> diff --git a/docs/misra/rules.rst b/docs/misra/rules.rst
> index db30632b93..4b1a7b02b6 100644
> --- a/docs/misra/rules.rst
> +++ b/docs/misra/rules.rst
> @@ -60,7 +60,8 @@ maintainers if you want to suggest a change.
>       - Precautions shall be taken in order to prevent the contents of a
>         header file being included more than once
>       - Files that are intended to be included more than once do not need to
> -       conform to the directive
> +       conform to the directive. Files that explicitly avoid inclusion guards
> +       under specific circumstances do not need to conform the directive.
>  
>     * - `Dir 4.11 <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/D_04_11.c>`_
>       - Required
> -- 
> 2.34.1
> 

Re: [XEN PATCH 02/13] automation/eclair: add text-based deviation for empty headers

[Stefano Stabellini]

On Mon, 28 Aug 2023, Simone Ballarin wrote:
> This patch adds a text-based deviation for Directive 4.10:
> "Precautions shall be taken in order to prevent the contents of
> a header file being included more than once"
> 
> Headers starting with the following comment are not supposed to
> comply with the directive:
> "/* empty */"
> 
> These headers should be empty, therefore they pose no risk if included
> more than once.
> 
> Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>

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

However I think we should also update rules.rst and/or update
docs/misra/safe.json


> ---
>  automation/eclair_analysis/ECLAIR/deviations.ecl | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl b/automation/eclair_analysis/ECLAIR/deviations.ecl
> index 5f068377fa..2681a4cff5 100644
> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl
> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
> @@ -80,6 +80,7 @@ inline functions."
>  
>  -doc_begin="This header file is autogenerated or empty, therefore it poses no
>  risk if included more than once."
> +-config=MC3R1.D4.10,reports+={safe, "first_area(text(^/\\* empty \\*/$, begin-1))"}
>  -file_tag+={empty_header, "^xen/arch/arm/efi/runtime\\.h$"}
>  -file_tag+={autogen_headers, "^xen/include/xen/compile\\.h$||^xen/include/generated/autoconf.h$||^xen/include/xen/hypercall-defs.h$"}
>  -config=MC3R1.D4.10,reports+={safe, "all_area(all_loc(file(empty_header||autogen_headers)))"}
> -- 
> 2.34.1
> 

Re: [XEN PATCH 03/13] xen/arm: address violations of MISRA C:2012 Directive 4.10

[Stefano Stabellini]

On Mon, 28 Aug 2023, Simone Ballarin wrote:
> Add or move inclusion guards to address violations of
> MISRA C:2012 Directive 4.10 ("Precautions shall be taken in order
> to prevent the contents of a header file being included more than
> once").
> 
> Inclusion guards must appear at the beginning of the headers
> (comments are permitted anywhere).
> 
> Mechanical change.
> 
> Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>

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


> ---
>  xen/arch/arm/efi/efi-boot.h          | 6 ++++++
>  xen/arch/arm/include/asm/hypercall.h | 6 +++---
>  xen/arch/arm/include/asm/iocap.h     | 6 +++---
>  3 files changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
> index 1c3640bb65..aba522ead5 100644
> --- a/xen/arch/arm/efi/efi-boot.h
> +++ b/xen/arch/arm/efi/efi-boot.h
> @@ -3,6 +3,10 @@
>   * is intended to be included by common/efi/boot.c _only_, and
>   * therefore can define arch specific global variables.
>   */
> +
> +#ifndef __ARM_EFI_EFI_BOOT_H__
> +#define __ARM_EFI_EFI_BOOT_H__
> +
>  #include <xen/device_tree.h>
>  #include <xen/libfdt/libfdt.h>
>  #include <asm/setup.h>
> @@ -1003,6 +1007,8 @@ static void __init efi_arch_flush_dcache_area(const void *vaddr, UINTN size)
>      __flush_dcache_area(vaddr, size);
>  }
>  
> +#endif /* __ARM_EFI_EFI_BOOT_H__*/
> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/xen/arch/arm/include/asm/hypercall.h b/xen/arch/arm/include/asm/hypercall.h
> index ccd26c5184..4f4d96f1f2 100644
> --- a/xen/arch/arm/include/asm/hypercall.h
> +++ b/xen/arch/arm/include/asm/hypercall.h
> @@ -1,10 +1,10 @@
> +#ifndef __ASM_ARM_HYPERCALL_H__
> +#define __ASM_ARM_HYPERCALL_H__
> +
>  #ifndef __XEN_HYPERCALL_H__
>  #error "asm/hypercall.h should not be included directly - include xen/hypercall.h instead"
>  #endif
>  
> -#ifndef __ASM_ARM_HYPERCALL_H__
> -#define __ASM_ARM_HYPERCALL_H__
> -
>  #include <public/domctl.h> /* for arch_do_domctl */
>  
>  long subarch_do_domctl(struct xen_domctl *domctl, struct domain *d,
> diff --git a/xen/arch/arm/include/asm/iocap.h b/xen/arch/arm/include/asm/iocap.h
> index 276fefbc59..4db1b16839 100644
> --- a/xen/arch/arm/include/asm/iocap.h
> +++ b/xen/arch/arm/include/asm/iocap.h
> @@ -1,10 +1,10 @@
> -#ifndef __X86_IOCAP_H__
> -#define __X86_IOCAP_H__
> +#ifndef __ASM_ARM_IOCAP_H__
> +#define __ASM_ARM_IOCAP_H__
>  
>  #define cache_flush_permitted(d)                        \
>      (!rangeset_is_empty((d)->iomem_caps))
>  
> -#endif
> +#endif /* __ASM_ARM_IOCAP_H__ */
>  
>  /*
>   * Local variables:
> -- 
> 2.34.1
> 

Re: [XEN PATCH 03/13] xen/arm: address violations of MISRA C:2012 Directive 4.10

[Julien Grall]

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

Hi,

On Mon, 28 Aug 2023 at 09:20, Simone Ballarin <simone.ballarin@bugseng.com>
wrote:

> Add or move inclusion guards to address violations of
> MISRA C:2012 Directive 4.10 ("Precautions shall be taken in order
> to prevent the contents of a header file being included more than
> once").
>
> Inclusion guards must appear at the beginning of the headers
> (comments are permitted anywhere).
>
> Mechanical change.
>
> Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>
> ---
>  xen/arch/arm/efi/efi-boot.h          | 6 ++++++
>  xen/arch/arm/include/asm/hypercall.h | 6 +++---
>  xen/arch/arm/include/asm/iocap.h     | 6 +++---
>  3 files changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
> index 1c3640bb65..aba522ead5 100644
> --- a/xen/arch/arm/efi/efi-boot.h
> +++ b/xen/arch/arm/efi/efi-boot.h
> @@ -3,6 +3,10 @@
>   * is intended to be included by common/efi/boot.c _only_, and
>   * therefore can define arch specific global variables.
>   */
> +
> +#ifndef __ARM_EFI_EFI_BOOT_H__
> +#define __ARM_EFI_EFI_BOOT_H__
> +
>  #include <xen/device_tree.h>
>  #include <xen/libfdt/libfdt.h>
>  #include <asm/setup.h>
> @@ -1003,6 +1007,8 @@ static void __init efi_arch_flush_dcache_area(const
> void *vaddr, UINTN size)
>      __flush_dcache_area(vaddr, size);
>  }
>
> +#endif /* __ARM_EFI_EFI_BOOT_H__*/
> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/xen/arch/arm/include/asm/hypercall.h
> b/xen/arch/arm/include/asm/hypercall.h
> index ccd26c5184..4f4d96f1f2 100644
> --- a/xen/arch/arm/include/asm/hypercall.h
> +++ b/xen/arch/arm/include/asm/hypercall.h
> @@ -1,10 +1,10 @@
> +#ifndef __ASM_ARM_HYPERCALL_H__
> +#define __ASM_ARM_HYPERCALL_H__
> +
>  #ifndef __XEN_HYPERCALL_H__
>  #error "asm/hypercall.h should not be included directly - include
> xen/hypercall.h instead"
>  #endif
>
> -#ifndef __ASM_ARM_HYPERCALL_H__
> -#define __ASM_ARM_HYPERCALL_H__
> -


I understand that you are trying to fix a misra violation. However, this
feels like it was done on purpose.

With the new change, you would not always check that the file were included
at the correct place. I am not against this change but this ought to be
explained.


>  #include <public/domctl.h> /* for arch_do_domctl */
>
>  long subarch_do_domctl(struct xen_domctl *domctl, struct domain *d,
> diff --git a/xen/arch/arm/include/asm/iocap.h
> b/xen/arch/arm/include/asm/iocap.h
> index 276fefbc59..4db1b16839 100644
> --- a/xen/arch/arm/include/asm/iocap.h
> +++ b/xen/arch/arm/include/asm/iocap.h
> @@ -1,10 +1,10 @@
> -#ifndef __X86_IOCAP_H__
> -#define __X86_IOCAP_H__
> +#ifndef __ASM_ARM_IOCAP_H__
> +#define __ASM_ARM_IOCAP_H__
>
>  #define cache_flush_permitted(d)                        \
>      (!rangeset_is_empty((d)->iomem_caps))
>
> -#endif
> +#endif /* __ASM_ARM_IOCAP_H__ */


I don’t understand how this is related to the rest of the patch. You wrote
that inclusion must appear first and this is the case here.

However the name is technically not correct. Is this really related to
directive 4.10? If so, this should be clarified in the commit message. If
not, then I think this should be in a separate commit.

Cheers,


>
>  /*
>   * Local variables:
> --
> 2.34.1
>
>

[-- Attachment #2: Type: text/html, Size: 4496 bytes --]

Re: [XEN PATCH 04/13] xen/x86: address violations of MISRA C:2012 Directive 4.10

[Stefano Stabellini]

On Mon, 28 Aug 2023, Simone Ballarin wrote:
> Add or move inclusion guards to address violations of
> MISRA C:2012 Directive 4.10 ("Precautions shall be taken in order
> to prevent the contents of a header file being included more than
> once").
> 
> Inclusion guards must appear at the beginning of the headers
> (comments are permitted anywhere).
> 
> Also C files, if included somewhere, need to comply with the guideline.
> 
> Mechanical change.
> 
> Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>
> ---
>  xen/arch/x86/Makefile                  | 8 ++++----
>  xen/arch/x86/cpu/cpu.h                 | 5 +++++
>  xen/arch/x86/physdev.c                 | 4 ++++
>  xen/arch/x86/platform_hypercall.c      | 5 +++++
>  xen/arch/x86/x86_64/compat/mm.c        | 5 +++++
>  xen/arch/x86/x86_64/mmconfig.h         | 5 +++++
>  xen/arch/x86/x86_emulate/private.h     | 5 +++++
>  xen/arch/x86/x86_emulate/x86_emulate.c | 5 +++++
>  8 files changed, 38 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
> index e642ad6c55..f956b7f0cd 100644
> --- a/xen/arch/x86/Makefile
> +++ b/xen/arch/x86/Makefile
> @@ -259,17 +259,17 @@ $(objtree)/arch/x86/include/asm/asm-macros.h: $(obj)/asm-macros.i $(src)/Makefil
>  	$(call filechk,asm-macros.h)
>  
>  define filechk_asm-macros.h
> +    echo '#ifndef __ASM_MACROS_H__'; \
> +    echo '#define __ASM_MACROS_H__'; \
>      echo '#if 0'; \
>      echo '.if 0'; \
>      echo '#endif'; \
> -    echo '#ifndef __ASM_MACROS_H__'; \
> -    echo '#define __ASM_MACROS_H__'; \
>      echo 'asm ( ".include \"$@\"" );'; \
> -    echo '#endif /* __ASM_MACROS_H__ */'; \
>      echo '#if 0'; \
>      echo '.endif'; \
>      cat $<; \
> -    echo '#endif'
> +    echo '#endif'; \
> +    echo '#endif /* __ASM_MACROS_H__ */'
>  endef
  
This looks OK but it needs to be reviewed by an x86 maintainer


>  $(obj)/efi.lds: AFLAGS-y += -DEFI
> diff --git a/xen/arch/x86/cpu/cpu.h b/xen/arch/x86/cpu/cpu.h
> index e3d06278b3..95939c7fb6 100644
> --- a/xen/arch/x86/cpu/cpu.h
> +++ b/xen/arch/x86/cpu/cpu.h
> @@ -1,3 +1,6 @@
> +#ifndef __X86_CPU_CPU_H__
> +#define __X86_CPU_CPU_H__
> +
>  /* attempt to consolidate cpu attributes */
>  struct cpu_dev {
>  	void		(*c_early_init)(struct cpuinfo_x86 *c);
> @@ -24,3 +27,5 @@ void amd_init_lfence(struct cpuinfo_x86 *c);
>  void amd_init_ssbd(const struct cpuinfo_x86 *c);
>  void amd_init_spectral_chicken(void);
>  void detect_zen2_null_seg_behaviour(void);
> +
> +#endif /* __X86_CPU_CPU_H__ */
> diff --git a/xen/arch/x86/physdev.c b/xen/arch/x86/physdev.c
> index 2f1d955a96..08b391d8f3 100644
> --- a/xen/arch/x86/physdev.c
> +++ b/xen/arch/x86/physdev.c
> @@ -1,3 +1,5 @@
> +#ifndef  __X86_PHYSDEV_C__
> +#define  __X86_PHYSDEV_C__

NIT: double " "

everything else looks OK


>  #include <xen/init.h>
>  #include <xen/lib.h>
> @@ -623,6 +625,8 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>      return ret;
>  }
>  
> +#endif /* __X86_PHYSDEV_C__ */
> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/xen/arch/x86/platform_hypercall.c b/xen/arch/x86/platform_hypercall.c
> index 9ff2da8fc3..11aa084887 100644
> --- a/xen/arch/x86/platform_hypercall.c
> +++ b/xen/arch/x86/platform_hypercall.c
> @@ -6,6 +6,9 @@
>   * Copyright (c) 2002-2006, K Fraser
>   */
>  
> +#ifndef __X86_PLATFORM_HYPERCALL_C__
> +#define __X86_PLATFORM_HYPERCALL_C__
> +
>  #include <xen/types.h>
>  #include <xen/lib.h>
>  #include <xen/mm.h>
> @@ -899,6 +902,8 @@ ret_t do_platform_op(
>      return ret;
>  }
>  
> +#endif /* __X86_PLATFORM_HYPERCALL_C__ */
> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/xen/arch/x86/x86_64/compat/mm.c b/xen/arch/x86/x86_64/compat/mm.c
> index d54efaad21..24f7eb8788 100644
> --- a/xen/arch/x86/x86_64/compat/mm.c
> +++ b/xen/arch/x86/x86_64/compat/mm.c
> @@ -1,3 +1,6 @@
> +#ifndef __X86_X86_64_COMPAT_MM_C__
> +#define __X86_X86_64_COMPAT_MM_C__
> +
>  #include <xen/event.h>
>  #include <xen/hypercall.h>
>  #include <xen/mem_access.h>
> @@ -326,6 +329,8 @@ int compat_mmuext_op(
>  }
>  #endif /* CONFIG_PV */
>  
> +#endif /* __X86_X86_64_COMPAT_MM_C__ */
> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/xen/arch/x86/x86_64/mmconfig.h b/xen/arch/x86/x86_64/mmconfig.h
> index 2d49fc79a0..c562879c76 100644
> --- a/xen/arch/x86/x86_64/mmconfig.h
> +++ b/xen/arch/x86/x86_64/mmconfig.h
> @@ -5,6 +5,9 @@
>   * Author: Allen Kay <allen.m.kay@intel.com> - adapted from linux
>   */
>  
> +#ifndef __X86_X86_64_MMCONFIG_H__
> +#define __X86_X86_64_MMCONFIG_H__
> +
>  #define PCI_DEVICE_ID_INTEL_E7520_MCH    0x3590
>  #define PCI_DEVICE_ID_INTEL_82945G_HB    0x2770
>  
> @@ -72,3 +75,5 @@ int pci_mmcfg_reserved(uint64_t address, unsigned int segment,
>  int pci_mmcfg_arch_init(void);
>  int pci_mmcfg_arch_enable(unsigned int);
>  void pci_mmcfg_arch_disable(unsigned int);
> +
> +#endif /* __X86_X86_64_MMCONFIG_H__ */
> diff --git a/xen/arch/x86/x86_emulate/private.h b/xen/arch/x86/x86_emulate/private.h
> index 719dad59cd..ffa134f297 100644
> --- a/xen/arch/x86/x86_emulate/private.h
> +++ b/xen/arch/x86/x86_emulate/private.h
> @@ -6,6 +6,9 @@
>   * Copyright (c) 2005-2007 XenSource Inc.
>   */
>  
> +#ifndef __X86_X86_EMULATE_PRIVATE_H__
> +#define __X86_X86_EMULATE_PRIVATE_H__
> +
>  #ifdef __XEN__
>  
>  # include <xen/kernel.h>
> @@ -831,3 +834,5 @@ static inline int read_ulong(enum x86_segment seg,
>      *val = 0;
>      return ops->read(seg, offset, val, bytes, ctxt);
>  }
> +
> +#endif /* __X86_X86_EMULATE_PRIVATE_H__ */
> diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c b/xen/arch/x86/x86_emulate/x86_emulate.c
> index e88245eae9..8977a1b82e 100644
> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
> @@ -8,6 +8,9 @@
>   * Copyright (c) 2005-2007 XenSource Inc.
>   */
>  
> +#ifndef __X86_X86_EMULATE_EMULATE_C__
> +#define __X86_X86_EMULATE_EMULATE_C__
> +
>  #include "private.h"
>  
>  /*
> @@ -8678,3 +8681,5 @@ int x86_emulate_wrapper(
>      return rc;
>  }
>  #endif
> +
> +#endif /* __X86_X86_EMULATE_EMULATE_C__ */
> -- 
> 2.34.1
> 

Re: [XEN PATCH 05/13] automation/eclair: add deviation for usercopy.c

[Stefano Stabellini]

+Nicola, Luca

On Mon, 28 Aug 2023, Simone Ballarin wrote:
> xen/arch/x86/usercopy.c includes itself, so it is not supposed to
> comply with Directive 4.10:
> "Precautions shall be taken in order to prevent the contents of a
> header file being included more than once"
> 
> This patch adds a deviation for the file.
> 
> Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>
>
> ---
>  automation/eclair_analysis/ECLAIR/deviations.ecl | 4 ++++
>  docs/misra/rules.rst                             | 2 ++
>  2 files changed, 6 insertions(+)
> 
> diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl b/automation/eclair_analysis/ECLAIR/deviations.ecl
> index 2681a4cff5..a7d4f29b43 100644
> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl
> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
> @@ -96,6 +96,10 @@ conform to the directive."
>  -config=MC3R1.D4.10,reports+={safe, "first_area(text(^ \\* In this case, no inclusion guards apply and the caller is responsible.*\\*/$, begin-1))"}
>  -doc_end
>  
> +-doc_begin="xen/arch/x86/usercopy.c includes itself: it is not supposed to comply with the directive"
> +-config=MC3R1.D4.10,reports+={deliberate, "all_area(all_loc(file("^xen/arch/x86/usercopy\\.c$")))"}
> +-doc_end
> +
>  #
>  # Series 5.
>  #
> diff --git a/docs/misra/rules.rst b/docs/misra/rules.rst
> index 4b1a7b02b6..45e13d0302 100644
> --- a/docs/misra/rules.rst
> +++ b/docs/misra/rules.rst
> @@ -62,6 +62,8 @@ maintainers if you want to suggest a change.
>       - Files that are intended to be included more than once do not need to
>         conform to the directive. Files that explicitly avoid inclusion guards
>         under specific circumstances do not need to conform the directive.
> +       xen/arch/x86/usercopy.c includes itself: it is not supposed to comply
> +       with the directive.


We need to find a consistent way to document this kind of deviations in
a non-ECLAIR specific way, without adding the complete list of
deviations to rules.rst.

Can we use safe.json and add an in-code comment at the top of
usercopy.c? E.g.:

diff --git a/xen/arch/x86/usercopy.c b/xen/arch/x86/usercopy.c
index b8c2d1cc0b..8bb591f472 100644
--- a/xen/arch/x86/usercopy.c
+++ b/xen/arch/x86/usercopy.c
@@ -1,3 +1,4 @@
+/* SAF-1-safe */
 /* 
  * User address space access functions.
  *

Otherwise, maybe we should extend safe.json to also have an extra field
with a list of paths. For instance see "files" below:

{
    "version": "1.0",
    "content": [
        {
            "id": "SAF-0-safe",
            "analyser": {
                "eclair": "MC3R1.R8.6",
                "coverity": "misra_c_2012_rule_8_6_violation"
            },
            "name": "Rule 8.6: linker script defined symbols",
            "text": "It is safe to declare this symbol because it is defined in the linker script."
        },
        {
            "id": "SAF-1-safe",
            "analyser": {
                "eclair": "MC3R1.D4.10"
            },
            "name": "Dir 4.10: files that include themselves",
            "text": "Files purposely written to include themselves are not supposed to comply with D4.10.",
            "files": ["xen/arch/x86/usercopy.c"]
        },
        {
            "id": "SAF-2-safe",
            "analyser": {},
            "name": "Sentinel",
            "text": "Next ID to be used"
        }
    ]
}

Re: [XEN PATCH 06/13] x86/EFI: address violations of MISRA C:2012 Directive 4.10

[Stefano Stabellini]

On Mon, 28 Aug 2023, Simone Ballarin wrote:
> Add inclusion guards to address violations of
> MISRA C:2012 Directive 4.10 ("Precautions shall be taken in order
> to prevent the contents of a header file being included more than
> once").
> 
> Mechanical change.
> 
> Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>

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


> ---
>  xen/arch/x86/efi/efi-boot.h | 6 ++++++
>  xen/arch/x86/efi/runtime.h  | 5 +++++
>  2 files changed, 11 insertions(+)
> 
> diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
> index 92f4cfe8bd..2c6be062cc 100644
> --- a/xen/arch/x86/efi/efi-boot.h
> +++ b/xen/arch/x86/efi/efi-boot.h
> @@ -3,6 +3,10 @@
>   * is intended to be included by common/efi/boot.c _only_, and
>   * therefore can define arch specific global variables.
>   */
> +
> +#ifndef __X86_EFI_EFI_BOOT_H__
> +#define __X86_EFI_EFI_BOOT_H__
> +
>  #include <xen/vga.h>
>  #include <asm/e820.h>
>  #include <asm/edd.h>
> @@ -913,6 +917,8 @@ void __init efi_multiboot2(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable
>      efi_exit_boot(ImageHandle, SystemTable);
>  }
>  
> +#endif /* __X86_EFI_EFI_BOOT_H__ */
> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/xen/arch/x86/efi/runtime.h b/xen/arch/x86/efi/runtime.h
> index 77866c5f21..10b36bcb89 100644
> --- a/xen/arch/x86/efi/runtime.h
> +++ b/xen/arch/x86/efi/runtime.h
> @@ -1,3 +1,6 @@
> +#ifndef __X86_EFI_RUNTIME_H__
> +#define __X86_EFI_RUNTIME_H__
> +
>  #include <xen/domain_page.h>
>  #include <xen/mm.h>
>  #include <asm/atomic.h>
> @@ -17,3 +20,5 @@ void efi_update_l4_pgtable(unsigned int l4idx, l4_pgentry_t l4e)
>      }
>  }
>  #endif
> +
> +#endif /* __X86_EFI_RUNTIME_H__ */
> -- 
> 2.34.1
> 

Re: [XEN PATCH 07/13] x86/asm: address violations of MISRA C:2012 Directive 4.10

[Stefano Stabellini]

On Mon, 28 Aug 2023, Simone Ballarin wrote:
> Add or move inclusion guards to address violations of
> MISRA C:2012 Directive 4.10 ("Precautions shall be taken in order
> to prevent the contents of a header file being included more than
> once").
> 
> Inclusion guards must appear at the beginning of the headers
> (comments are permitted anywhere).
> 
> The text of the beggining comment of cpufeatures.h has been changed
> to match the deviation in automation/eclair_analysis/ECLAIR/deviations.ecl,
> moreover this new formulation is already used in other files.

I don't think it is a good idea to do this kind of textual matching.
Instead we should use the format introduced by safe.json, e.g.
SAF-1-safe


> Mechanical change.
> 
> Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>
> ---
>  xen/arch/x86/include/asm/compat.h      | 5 +++++
>  xen/arch/x86/include/asm/cpufeatures.h | 4 +---
>  xen/arch/x86/include/asm/efibind.h     | 5 +++++
>  xen/arch/x86/include/asm/hypercall.h   | 6 +++---
>  4 files changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/xen/arch/x86/include/asm/compat.h b/xen/arch/x86/include/asm/compat.h
> index 818cad87db..3d3891d061 100644
> --- a/xen/arch/x86/include/asm/compat.h
> +++ b/xen/arch/x86/include/asm/compat.h
> @@ -2,6 +2,9 @@
>   * compat.h
>   */
>  
> +#ifndef __ASM_X86_COMPAT_H__
> +#define __ASM_X86_COMPAT_H__
> +
>  #ifdef CONFIG_COMPAT
>  
>  #define COMPAT_BITS_PER_LONG 32
> @@ -18,3 +21,5 @@ int switch_compat(struct domain *);
>  #include <xen/errno.h>
>  static inline int switch_compat(struct domain *d) { return -EOPNOTSUPP; }
>  #endif
> +
> +#endif /* __ASM_X86_COMPAT_H__ */
> diff --git a/xen/arch/x86/include/asm/cpufeatures.h b/xen/arch/x86/include/asm/cpufeatures.h
> index da0593de85..1dfdd478ab 100644
> --- a/xen/arch/x86/include/asm/cpufeatures.h
> +++ b/xen/arch/x86/include/asm/cpufeatures.h
> @@ -1,6 +1,4 @@
> -/*
> - * Explicitly intended for multiple inclusion.
> - */
> +/* This file is legitimately included multiple times */
>  
>  #include <xen/lib/x86/cpuid-autogen.h>
>  
> diff --git a/xen/arch/x86/include/asm/efibind.h b/xen/arch/x86/include/asm/efibind.h
> index bce02f3707..f2eb8b5496 100644
> --- a/xen/arch/x86/include/asm/efibind.h
> +++ b/xen/arch/x86/include/asm/efibind.h
> @@ -1,2 +1,7 @@
> +#ifndef __ASM_X86_EFIBIND_H__
> +#define __ASM_X86_EFIBIND_H__
> +
>  #include <xen/types.h>
>  #include <asm/x86_64/efibind.h>
> +
> +#endif /* __ASM_X86_EFIBIND_H__ */
> diff --git a/xen/arch/x86/include/asm/hypercall.h b/xen/arch/x86/include/asm/hypercall.h
> index ec2edc771e..2ade5d71b8 100644
> --- a/xen/arch/x86/include/asm/hypercall.h
> +++ b/xen/arch/x86/include/asm/hypercall.h
> @@ -2,13 +2,13 @@
>   * asm-x86/hypercall.h
>   */
>  
> +#ifndef __ASM_X86_HYPERCALL_H__
> +#define __ASM_X86_HYPERCALL_H__
> +
>  #ifndef __XEN_HYPERCALL_H__
>  #error "asm/hypercall.h should not be included directly - include xen/hypercall.h instead"
>  #endif
>  
> -#ifndef __ASM_X86_HYPERCALL_H__
> -#define __ASM_X86_HYPERCALL_H__
> -
>  #include <xen/types.h>
>  #include <public/physdev.h>
>  #include <public/event_channel.h>
> -- 
> 2.34.1
> 

Re: [XEN PATCH 01/13] misra: add deviation for headers that explicitly avoid guards

[Stefano Stabellini]

On Mon, 28 Aug 2023, Stefano Stabellini wrote:
> On Mon, 28 Aug 2023, Simone Ballarin wrote:
> > Some headers, under specific circumstances (documented in a comment at
> > the beginning of the file), explicitly avoid inclusion guards: the caller
> > is responsible for including them correctly.
> > 
> > These files are not supposed to comply with Directive 4.10:
> > "Precautions shall be taken in order to prevent the contents of a header
> > file being included more than once"
> > 
> > This patch adds a deviation for all headers that contain the following
> > in a comment text:
> > "In this case, no inclusion guards apply and the caller is responsible"
> > 
> > Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>
> 
> Acked-by: Stefano Stabellini <sstabellini@kernel.org>

Actually one question


> > ---
> >  automation/eclair_analysis/ECLAIR/deviations.ecl | 4 ++++
> >  docs/misra/rules.rst                             | 3 ++-
> >  2 files changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl b/automation/eclair_analysis/ECLAIR/deviations.ecl
> > index d8170106b4..5f068377fa 100644
> > --- a/automation/eclair_analysis/ECLAIR/deviations.ecl
> > +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
> > @@ -91,6 +91,10 @@ conform to the directive."
> >  -config=MC3R1.D4.10,reports+={safe, "first_area(text(^/\\* Generated file, do not edit! \\*/$, begin-3))"}
> >  -doc_end
> >  
> > +-doc_begin="Some headers, under specific circumstances, explicitly avoid inclusion guards."
> > +-config=MC3R1.D4.10,reports+={safe, "first_area(text(^ \\* In this case, no inclusion guards apply and the caller is responsible.*\\*/$, begin-1))"}
> > +-doc_end

Is this supposed to match with any files starting with "In this case,
no inclusion..." ?

We should use the format introduced by safe.json instead

Re: [XEN PATCH 08/13] x86/mm: address violations of MISRA C:2012 Directive 4.10

[Stefano Stabellini]

On Mon, 28 Aug 2023, Simone Ballarin wrote:
> Add inclusion guards to address violations of
> MISRA C:2012 Directive 4.10 ("Precautions shall be taken in order
> to prevent the contents of a header file being included more than
> once").
> 
> C files, if included somewhere, need to comply with the guideline.
> 
> Mechanical change.
> 
> Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>

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


> ---
>  xen/arch/x86/mm/guest_walk.c     | 5 +++++
>  xen/arch/x86/mm/hap/guest_walk.c | 4 ++++
>  2 files changed, 9 insertions(+)
> 
> diff --git a/xen/arch/x86/mm/guest_walk.c b/xen/arch/x86/mm/guest_walk.c
> index fe7393334f..66c127156d 100644
> --- a/xen/arch/x86/mm/guest_walk.c
> +++ b/xen/arch/x86/mm/guest_walk.c
> @@ -9,6 +9,9 @@
>   * Parts based on earlier work by Michael A Fetterman, Ian Pratt et al.
>   */
>  
> +#ifndef __X86_MM_GUEST_WALK_C__
> +#define __X86_MM_GUEST_WALK_C__
> +
>  #include <xen/types.h>
>  #include <xen/mm.h>
>  #include <xen/paging.h>
> @@ -576,6 +579,8 @@ void *map_domain_gfn(struct p2m_domain *p2m, gfn_t gfn, mfn_t *mfn,
>  }
>  #endif
>  
> +#endif /* __X86_MM_GUEST_WALK_C__ */
> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/xen/arch/x86/mm/hap/guest_walk.c b/xen/arch/x86/mm/hap/guest_walk.c
> index d1b7c5762c..d4ffa8141f 100644
> --- a/xen/arch/x86/mm/hap/guest_walk.c
> +++ b/xen/arch/x86/mm/hap/guest_walk.c
> @@ -7,6 +7,9 @@
>   * Copyright (c) 2007, XenSource Inc.
>   */
>  
> +#ifndef __X86_MM_HAP_GUEST_WALK_C__
> +#define __X86_MM_HAP_GUEST_WALK_C__
> +
>  #include <xen/domain_page.h>
>  #include <xen/paging.h>
>  #include <xen/sched.h>
> @@ -124,6 +127,7 @@ unsigned long cf_check hap_p2m_ga_to_gfn(GUEST_PAGING_LEVELS)(
>      return gfn_x(INVALID_GFN);
>  }
>  
> +#endif /* __X86_MM_HAP_GUEST_WALK_C__ */
>  
>  /*
>   * Local variables:
> -- 
> 2.34.1
> 

Re: [XEN PATCH 09/13] xen/common: address violations of MISRA C:2012 Directive 4.10

[Stefano Stabellini]

On Mon, 28 Aug 2023, Simone Ballarin wrote:
> Add inclusion guards to address violations of
> MISRA C:2012 Directive 4.10 ("Precautions shall be taken in order
> to prevent the contents of a header file being included more than
> once").
> 
> Also C files, if included somewhere, need to comply with the guideline.
> 
> Mechanical change.
> 
> Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>

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


> ---
>  xen/common/compat/grant_table.c | 7 +++++++
>  xen/common/coverage/gcc_4_7.c   | 5 +++++
>  xen/common/decompress.h         | 5 +++++
>  xen/common/event_channel.h      | 5 +++++
>  xen/common/multicall.c          | 5 +++++
>  5 files changed, 27 insertions(+)
> 
> diff --git a/xen/common/compat/grant_table.c b/xen/common/compat/grant_table.c
> index f8177c84c0..614ad71a59 100644
> --- a/xen/common/compat/grant_table.c
> +++ b/xen/common/compat/grant_table.c
> @@ -3,6 +3,10 @@
>   *
>   */
>  
> +
> +#ifndef __COMMON_COMPAT_GRANT_TABLE_C__
> +#define __COMMON_COMPAT_GRANT_TABLE_C__
> +
>  #include <xen/hypercall.h>
>  #include <compat/grant_table.h>
>  
> @@ -331,6 +335,9 @@ int compat_grant_table_op(
>      return rc;
>  }
>  
> +
> +#endif /* __COMMON_COMPAT_GRANT_TABLE_C__ */
> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/xen/common/coverage/gcc_4_7.c b/xen/common/coverage/gcc_4_7.c
> index 25b4a8bcdc..12e4ec8cbb 100644
> --- a/xen/common/coverage/gcc_4_7.c
> +++ b/xen/common/coverage/gcc_4_7.c
> @@ -14,6 +14,9 @@
>   *    Wei Liu <wei.liu2@citrix.com>
>   */
>  
> +#ifndef __COMMON_COVERAGE_GCC_4_7_C__
> +#define __COMMON_COVERAGE_GCC_4_7_C__
> +
>  #include <xen/string.h>
>  
>  #include "gcov.h"
> @@ -193,6 +196,8 @@ size_t gcov_info_to_gcda(char *buffer, const struct gcov_info *info)
>      return pos;
>  }
>  
> +#endif /* __COMMON_COVERAGE_GCC_4_7_C__ */
> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/xen/common/decompress.h b/xen/common/decompress.h
> index e8195b353a..da3c3abb6a 100644
> --- a/xen/common/decompress.h
> +++ b/xen/common/decompress.h
> @@ -1,3 +1,6 @@
> +#ifndef __COMMON_DECOMPRESS_H__
> +#define __COMMON_DECOMPRESS_H__
> +
>  #ifdef __XEN__
>  
>  #include <xen/cache.h>
> @@ -23,3 +26,5 @@
>  #define large_free free
>  
>  #endif
> +
> +#endif /* __COMMON_DECOMPRESS_H__ */
> diff --git a/xen/common/event_channel.h b/xen/common/event_channel.h
> index 45219ca67c..040bad77f9 100644
> --- a/xen/common/event_channel.h
> +++ b/xen/common/event_channel.h
> @@ -1,5 +1,8 @@
>  /* Event channel handling private header. */
>  
> +#ifndef __COMMON_EVENT_CHANNEL_H__
> +#define __COMMON_EVENT_CHANNEL_H__
> +
>  #include <xen/event.h>
>  
>  static inline unsigned int max_evtchns(const struct domain *d)
> @@ -52,6 +55,8 @@ int evtchn_fifo_init_control(struct evtchn_init_control *init_control);
>  int evtchn_fifo_expand_array(const struct evtchn_expand_array *expand_array);
>  void evtchn_fifo_destroy(struct domain *d);
>  
> +#endif /* __COMMON_EVENT_CHANNEL_H__ */
> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/xen/common/multicall.c b/xen/common/multicall.c
> index 1f0cc4cb26..421bb25b70 100644
> --- a/xen/common/multicall.c
> +++ b/xen/common/multicall.c
> @@ -2,6 +2,9 @@
>   * multicall.c
>   */
>  
> +#ifndef __COMMON_MULTICALL_C__
> +#define __COMMON_MULTICALL_C__
> +
>  #include <xen/types.h>
>  #include <xen/lib.h>
>  #include <xen/mm.h>
> @@ -124,6 +127,8 @@ ret_t do_multicall(
>          __HYPERVISOR_multicall, "hi", call_list, nr_calls-i);
>  }
>  
> +#endif /* __COMMON_MULTICALL_C__ */
> +
>  /*
>   * Local variables:
>   * mode: C
> -- 
> 2.34.1
> 

Re: [XEN PATCH 10/13] xen/efi: address violations of MISRA C:2012 Directive 4.10

[Stefano Stabellini]

On Mon, 28 Aug 2023, Simone Ballarin wrote:
> Add inclusion guards to address violations of
> MISRA C:2012 Directive 4.10 ("Precautions shall be taken in order
> to prevent the contents of a header file being included more than
> once").
> 
> Also C files, if included somewhere, need to comply with the guideline.
> 
> Mechanical change.
> 
> Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>
> ---
>  xen/common/efi/efi.h     | 5 +++++
>  xen/common/efi/runtime.c | 6 ++++++
>  2 files changed, 11 insertions(+)
> 
> diff --git a/xen/common/efi/efi.h b/xen/common/efi/efi.h
> index c02fbb7b69..cef9381d30 100644
> --- a/xen/common/efi/efi.h
> +++ b/xen/common/efi/efi.h
> @@ -1,3 +1,6 @@
> +#ifndef __COMMON_EFI_EFI_H__
> +#define __COMMON_EFI_EFI_H__
> +
>  #include <asm/efibind.h>
>  #include <efi/efidef.h>
>  #include <efi/efierr.h>
> @@ -51,3 +54,5 @@ void free_ebmalloc_unused_mem(void);
>  
>  const void *pe_find_section(const void *image, const UINTN image_size,
>                              const CHAR16 *section_name, UINTN *size_out);
> +
> +#endif /* __COMMON_EFI_EFI_H__ */
> diff --git a/xen/common/efi/runtime.c b/xen/common/efi/runtime.c
> index 5cb7504c96..fb6fd17ba3 100644
> --- a/xen/common/efi/runtime.c
> +++ b/xen/common/efi/runtime.c
> @@ -6,6 +6,10 @@
>  #include <xen/irq.h>
>  #include <xen/time.h>
>  
> +#ifndef __COMMON_EFI_RUNTIME_C__
> +#define __COMMON_EFI_RUNTIME_C__

Shouldn't this be at the top of the file?


>  DEFINE_XEN_GUEST_HANDLE(CHAR16);
>  
>  struct efi_rs_state {
> @@ -704,3 +708,5 @@ int efi_runtime_call(struct xenpf_efi_runtime_call *op)
>      return rc;
>  }
>  #endif
> +
> +#endif /* __COMMON_EFI_RUNTIME_C__ */
> -- 
> 2.34.1
> 

Re: [XEN PATCH 11/13] xen/sched: address violations of MISRA C:2012 Directive 4.10

[Stefano Stabellini]

On Mon, 28 Aug 2023, Simone Ballarin wrote:
> Add inclusion guards to address violations of
> MISRA C:2012 Directive 4.10 ("Precautions shall be taken in order
> to prevent the contents of a header file being included more than
> once").
> 
> Mechanical change.
> 
> Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>

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


> ---
>  xen/common/sched/compat.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/xen/common/sched/compat.c b/xen/common/sched/compat.c
> index a596e3a226..d718e450d4 100644
> --- a/xen/common/sched/compat.c
> +++ b/xen/common/sched/compat.c
> @@ -3,6 +3,10 @@
>   *
>   */
>  
> +#ifndef __COMMON_SCHED_COMPAT_C__
> +#define __COMMON_SCHED_COMPAT_C__
> +
> +
>  #include <compat/sched.h>
>  
>  #define COMPAT
> @@ -44,6 +48,8 @@ int compat_set_timer_op(uint32_t lo, int32_t hi)
>      return do_set_timer_op(((s64)hi << 32) | lo);
>  }
>  
> +#endif /* __COMMON_SCHED_COMPAT_C__ */
> +
>  /*
>   * Local variables:
>   * mode: C
> -- 
> 2.34.1
> 

Re: [XEN PATCH 13/13] x86/asm: address violations of MISRA C:2012 Directive 4.10

[Stefano Stabellini]

On Mon, 28 Aug 2023, Simone Ballarin wrote:
> Amend generation script to address a violation of
> MISRA C:2012 Directive 4.10 ("Precautions shall be taken in order
> to prevent the contents of a header file being included more than
> once").
> 
> This patch adds a special comment to the beginning of the header
> to make it explicit that the file is generated automatically.
> 
> The comment is recognized by ECLAIR and will cause the deviation of
> the violation.
> 
> Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>
> ---
>  xen/tools/compat-xlat-header.py | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/xen/tools/compat-xlat-header.py b/xen/tools/compat-xlat-header.py
> index 2b805b23a8..9e336277ac 100644
> --- a/xen/tools/compat-xlat-header.py
> +++ b/xen/tools/compat-xlat-header.py
> @@ -406,6 +406,8 @@ def main():
>              line = line.strip()
>              header_tokens += re_tokenazier.split(line)
>  
> +    print("/* Generated file, do not edit! */")

I think it might be a good idea regardless of MISRA compliance to add
this comment.

However for MISRA compliance I think we should document somewhere other
than ECLAIR config file that "Generated file, do not edit!" is being
used as a MISRA C deviation marker.

I think we should add a special note to safe.json, or alternatively also
add the safe.json tag to the comment:

print("/* SAF-1-safe Generated file, do not edit! */")


>      with open(sys.argv[2]) as compat_list:
>          for line in compat_list:
>              words = re_tokenazier.split(line, maxsplit=1)

Re: [XEN PATCH 12/13] xen: address violations of MISRA C:2012 Directive 4.10

[Stefano Stabellini]

On Mon, 28 Aug 2023, Simone Ballarin wrote:
> Move or amended inclusion guards to address violations of
> MISRA C:2012 Directive 4.10 ("Precautions shall be taken in order
> to prevent the contents of a header file being included more than
> once").
> 
> Inclusion guards must appear at the beginning of the headers
> (comments are permitted anywhere) and the #if directive cannot
> be used for other checks.
> 
> Mechanical change.
> 
> Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>
> ---
>  xen/include/xen/err.h       | 4 +++-
>  xen/include/xen/pci_ids.h   | 5 +++++
>  xen/include/xen/softirq.h   | 4 +++-
>  xen/include/xen/unaligned.h | 7 ++++---
>  xen/include/xen/vmap.h      | 4 +++-
>  5 files changed, 18 insertions(+), 6 deletions(-)
> 
> diff --git a/xen/include/xen/err.h b/xen/include/xen/err.h
> index 2f29b57d28..a6323d82d7 100644
> --- a/xen/include/xen/err.h
> +++ b/xen/include/xen/err.h
> @@ -1,5 +1,6 @@
> -#if !defined(__XEN_ERR_H__) && !defined(__ASSEMBLY__)
> +#if !defined(__XEN_ERR_H__)
>  #define __XEN_ERR_H__
> +#if !defined(__ASSEMBLY__)

The original pattern was also guarding the header file sufficiently,
protecting it from double-inclusion. In fact, it is posing stricter
restrictions than usual (not laxer). This change is unnecessary?


>  #include <xen/compiler.h>
>  #include <xen/errno.h>
> @@ -54,4 +55,5 @@ static inline int __must_check PTR_RET(const void *ptr)
>  	return IS_ERR(ptr) ? PTR_ERR(ptr) : 0;
>  }
>  
> +#endif /* __ASSEMBLY__ */
>  #endif /* __XEN_ERR_H__ */
> diff --git a/xen/include/xen/pci_ids.h b/xen/include/xen/pci_ids.h
> index e798477a7e..1a739d4c92 100644
> --- a/xen/include/xen/pci_ids.h
> +++ b/xen/include/xen/pci_ids.h
> @@ -1,3 +1,6 @@
> +#ifndef __XEN_PCI_IDS_H__
> +#define __XEN_PCI_IDS_H__
> +
>  #define PCI_VENDOR_ID_AMD                0x1022
>  
>  #define PCI_VENDOR_ID_NVIDIA             0x10de
> @@ -11,3 +14,5 @@
>  #define PCI_VENDOR_ID_BROADCOM           0x14e4
>  
>  #define PCI_VENDOR_ID_INTEL              0x8086
> +
> +#endif /* __XEN_PCI_IDS_H__ */
> diff --git a/xen/include/xen/softirq.h b/xen/include/xen/softirq.h
> index 33d6f2ecd2..092ec733b7 100644
> --- a/xen/include/xen/softirq.h
> +++ b/xen/include/xen/softirq.h
> @@ -1,5 +1,6 @@
> -#if !defined(__XEN_SOFTIRQ_H__) && !defined(__ASSEMBLY__)
> +#if !defined(__XEN_SOFTIRQ_H__)
>  #define __XEN_SOFTIRQ_H__
> +#if !defined(__ASSEMBLY__)

same here


>  /* Low-latency softirqs come first in the following list. */
>  enum {
> @@ -40,4 +41,5 @@ void cpu_raise_softirq_batch_finish(void);
>   */
>  void process_pending_softirqs(void);
>  
> +#endif /* __ASSEMBLY__ */
>  #endif /* __XEN_SOFTIRQ_H__ */
> diff --git a/xen/include/xen/unaligned.h b/xen/include/xen/unaligned.h
> index 0a2b16d05d..45f03b3f1b 100644
> --- a/xen/include/xen/unaligned.h
> +++ b/xen/include/xen/unaligned.h
> @@ -3,13 +3,14 @@
>   * without faulting, and at least reasonably efficiently.  Other architectures
>   * will need to have a custom asm/unaligned.h.
>   */
> -#ifndef __ASM_UNALIGNED_H__
> -#error "xen/unaligned.h should not be included directly - include asm/unaligned.h instead"
> -#endif
>  
>  #ifndef __XEN_UNALIGNED_H__
>  #define __XEN_UNALIGNED_H__
>  
> +#ifndef __ASM_UNALIGNED_H__
> +#error "xen/unaligned.h should not be included directly - include asm/unaligned.h instead"
> +#endif
> +
>  #ifdef __XEN__
>  #include <xen/types.h>
>  #include <asm/byteorder.h>
> diff --git a/xen/include/xen/vmap.h b/xen/include/xen/vmap.h
> index b0f7632e89..7a61dea54a 100644
> --- a/xen/include/xen/vmap.h
> +++ b/xen/include/xen/vmap.h
> @@ -1,5 +1,6 @@
> -#if !defined(__XEN_VMAP_H__) && defined(VMAP_VIRT_START)
> +#if !defined(__XEN_VMAP_H__)
>  #define __XEN_VMAP_H__
> +#if defined(VMAP_VIRT_START)

same here


>  #include <xen/mm-frame.h>
>  #include <xen/page-size.h>
> @@ -38,4 +39,5 @@ static inline void vm_init(void)
>      vm_init_type(VMAP_DEFAULT, (void *)VMAP_VIRT_START, arch_vmap_virt_end());
>  }
>  
> +#endif /* VMAP_VIRT_START */
>  #endif /* __XEN_VMAP_H__ */
> -- 
> 2.34.1
> 

Re: [XEN PATCH 01/13] misra: add deviation for headers that explicitly avoid guards

[Jan Beulich]

On 28.08.2023 15:19, Simone Ballarin wrote:
> --- a/docs/misra/rules.rst
> +++ b/docs/misra/rules.rst
> @@ -60,7 +60,8 @@ maintainers if you want to suggest a change.
>       - Precautions shall be taken in order to prevent the contents of a
>         header file being included more than once
>       - Files that are intended to be included more than once do not need to
> -       conform to the directive
> +       conform to the directive. Files that explicitly avoid inclusion guards
> +       under specific circumstances do not need to conform the directive.

Nit: There's a "to" missing near the end of the added sentence. Can likely
be taken care of while committing, since Stefano has already ack-ed this,
but I'd like to still raise the question of the utility of this statement:
How is one to know whether omission of guards is intentional?

Jan

Re: [XEN PATCH 02/13] automation/eclair: add text-based deviation for empty headers

[Jan Beulich]

On 28.08.2023 15:19, Simone Ballarin wrote:
> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl
> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
> @@ -80,6 +80,7 @@ inline functions."
>  
>  -doc_begin="This header file is autogenerated or empty, therefore it poses no
>  risk if included more than once."

While unrelated to, the change at hand, I still have a question on this:
How come it is deemed universally safe to multi-include generated headers.
I would have said that whether that's safe depends on the nature of the
generated code in the header. Only truly empty ones are uniformly safe to
include any number of times.

Jan

> +-config=MC3R1.D4.10,reports+={safe, "first_area(text(^/\\* empty \\*/$, begin-1))"}
>  -file_tag+={empty_header, "^xen/arch/arm/efi/runtime\\.h$"}
>  -file_tag+={autogen_headers, "^xen/include/xen/compile\\.h$||^xen/include/generated/autoconf.h$||^xen/include/xen/hypercall-defs.h$"}
>  -config=MC3R1.D4.10,reports+={safe, "all_area(all_loc(file(empty_header||autogen_headers)))"}

Re: [XEN PATCH 05/13] automation/eclair: add deviation for usercopy.c

[Jan Beulich]

On 29.08.2023 00:27, Stefano Stabellini wrote:
> On Mon, 28 Aug 2023, Simone Ballarin wrote:
>> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl
>> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
>> @@ -96,6 +96,10 @@ conform to the directive."
>>  -config=MC3R1.D4.10,reports+={safe, "first_area(text(^ \\* In this case, no inclusion guards apply and the caller is responsible.*\\*/$, begin-1))"}
>>  -doc_end
>>  
>> +-doc_begin="xen/arch/x86/usercopy.c includes itself: it is not supposed to comply with the directive"
>> +-config=MC3R1.D4.10,reports+={deliberate, "all_area(all_loc(file("^xen/arch/x86/usercopy\\.c$")))"}
>> +-doc_end
>> +
>>  #
>>  # Series 5.
>>  #
>> diff --git a/docs/misra/rules.rst b/docs/misra/rules.rst
>> index 4b1a7b02b6..45e13d0302 100644
>> --- a/docs/misra/rules.rst
>> +++ b/docs/misra/rules.rst
>> @@ -62,6 +62,8 @@ maintainers if you want to suggest a change.
>>       - Files that are intended to be included more than once do not need to
>>         conform to the directive. Files that explicitly avoid inclusion guards
>>         under specific circumstances do not need to conform the directive.
>> +       xen/arch/x86/usercopy.c includes itself: it is not supposed to comply
>> +       with the directive.
> 
> 
> We need to find a consistent way to document this kind of deviations in
> a non-ECLAIR specific way, without adding the complete list of
> deviations to rules.rst.

+1

Especially rules.rst should not be modified to add mention of individual
exceptions. That's simply not the purpose of the file, at least the way
I understand it.

> Can we use safe.json and add an in-code comment at the top of
> usercopy.c? E.g.:

Right, this ought to be the was to go. Question is whether ...

> diff --git a/xen/arch/x86/usercopy.c b/xen/arch/x86/usercopy.c
> index b8c2d1cc0b..8bb591f472 100644
> --- a/xen/arch/x86/usercopy.c
> +++ b/xen/arch/x86/usercopy.c
> @@ -1,3 +1,4 @@
> +/* SAF-1-safe */

... this (or any other) placement of the comment will actually do (not
just for Eclair).

Jan

>  /* 
>   * User address space access functions.
>   *
> 
> Otherwise, maybe we should extend safe.json to also have an extra field
> with a list of paths. For instance see "files" below:
> 
> {
>     "version": "1.0",
>     "content": [
>         {
>             "id": "SAF-0-safe",
>             "analyser": {
>                 "eclair": "MC3R1.R8.6",
>                 "coverity": "misra_c_2012_rule_8_6_violation"
>             },
>             "name": "Rule 8.6: linker script defined symbols",
>             "text": "It is safe to declare this symbol because it is defined in the linker script."
>         },
>         {
>             "id": "SAF-1-safe",
>             "analyser": {
>                 "eclair": "MC3R1.D4.10"
>             },
>             "name": "Dir 4.10: files that include themselves",
>             "text": "Files purposely written to include themselves are not supposed to comply with D4.10.",
>             "files": ["xen/arch/x86/usercopy.c"]
>         },
>         {
>             "id": "SAF-2-safe",
>             "analyser": {},
>             "name": "Sentinel",
>             "text": "Next ID to be used"
>         }
>     ]
> }

Re: [XEN PATCH 07/13] x86/asm: address violations of MISRA C:2012 Directive 4.10

[Jan Beulich]

On 28.08.2023 15:20, Simone Ballarin wrote:
> --- a/xen/arch/x86/include/asm/hypercall.h
> +++ b/xen/arch/x86/include/asm/hypercall.h
> @@ -2,13 +2,13 @@
>   * asm-x86/hypercall.h
>   */
>  
> +#ifndef __ASM_X86_HYPERCALL_H__
> +#define __ASM_X86_HYPERCALL_H__
> +
>  #ifndef __XEN_HYPERCALL_H__
>  #error "asm/hypercall.h should not be included directly - include xen/hypercall.h instead"
>  #endif
>  
> -#ifndef __ASM_X86_HYPERCALL_H__
> -#define __ASM_X86_HYPERCALL_H__
> -
>  #include <xen/types.h>
>  #include <public/physdev.h>
>  #include <public/event_channel.h>

See Julien's comment on the similar Arm change.

Jan

Re: [XEN PATCH 10/13] xen/efi: address violations of MISRA C:2012 Directive 4.10

[Jan Beulich]

On 29.08.2023 00:42, Stefano Stabellini wrote:
> On Mon, 28 Aug 2023, Simone Ballarin wrote:
>> --- a/xen/common/efi/runtime.c
>> +++ b/xen/common/efi/runtime.c
>> @@ -6,6 +6,10 @@
>>  #include <xen/irq.h>
>>  #include <xen/time.h>
>>  
>> +#ifndef __COMMON_EFI_RUNTIME_C__
>> +#define __COMMON_EFI_RUNTIME_C__
> 
> Shouldn't this be at the top of the file?

Imo .c files shouldn't gain guards in the first place.

Jan

Re: [XEN PATCH 09/13] xen/common: address violations of MISRA C:2012 Directive 4.10

[Jan Beulich]

On 28.08.2023 15:20, Simone Ballarin wrote:
> Add inclusion guards to address violations of
> MISRA C:2012 Directive 4.10 ("Precautions shall be taken in order
> to prevent the contents of a header file being included more than
> once").
> 
> Also C files, if included somewhere, need to comply with the guideline.
> 
> Mechanical change.
> 
> Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>
> ---
>  xen/common/compat/grant_table.c | 7 +++++++
>  xen/common/coverage/gcc_4_7.c   | 5 +++++
>  xen/common/decompress.h         | 5 +++++
>  xen/common/event_channel.h      | 5 +++++
>  xen/common/multicall.c          | 5 +++++
>  5 files changed, 27 insertions(+)

As already said in reply to another patch, imo .c files shouldn't gain such
guards. These are commonly referred to as "header guards" for a reason.

> --- a/xen/common/compat/grant_table.c
> +++ b/xen/common/compat/grant_table.c
> @@ -3,6 +3,10 @@
>   *
>   */
>  
> +

Nit: No double blank lines please.

> +#ifndef __COMMON_COMPAT_GRANT_TABLE_C__
> +#define __COMMON_COMPAT_GRANT_TABLE_C__
> +
>  #include <xen/hypercall.h>
>  #include <compat/grant_table.h>
>  
> @@ -331,6 +335,9 @@ int compat_grant_table_op(
>      return rc;
>  }
>  
> +

Again here (at least).

Jan

Re: [XEN PATCH 12/13] xen: address violations of MISRA C:2012 Directive 4.10

[Jan Beulich]

On 28.08.2023 15:20, Simone Ballarin wrote:
> --- a/xen/include/xen/unaligned.h
> +++ b/xen/include/xen/unaligned.h
> @@ -3,13 +3,14 @@
>   * without faulting, and at least reasonably efficiently.  Other architectures
>   * will need to have a custom asm/unaligned.h.
>   */
> -#ifndef __ASM_UNALIGNED_H__
> -#error "xen/unaligned.h should not be included directly - include asm/unaligned.h instead"
> -#endif
>  
>  #ifndef __XEN_UNALIGNED_H__
>  #define __XEN_UNALIGNED_H__
>  
> +#ifndef __ASM_UNALIGNED_H__
> +#error "xen/unaligned.h should not be included directly - include asm/unaligned.h instead"
> +#endif

In addition to what Stefano said, this repositioning also is questionable
(as per comments elsewhere). Overall it looks like the entire patch wants
dropping? Or wait, no, the pci_ids.h change would remain.

Jan

Re: [XEN PATCH 04/13] xen/x86: address violations of MISRA C:2012 Directive 4.10

[Jan Beulich]

On 28.08.2023 15:20, Simone Ballarin wrote:
> Add or move inclusion guards to address violations of
> MISRA C:2012 Directive 4.10 ("Precautions shall be taken in order
> to prevent the contents of a header file being included more than
> once").
> 
> Inclusion guards must appear at the beginning of the headers
> (comments are permitted anywhere).
> 
> Also C files, if included somewhere, need to comply with the guideline.
> 
> Mechanical change.
> 
> Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>
> ---
>  xen/arch/x86/Makefile                  | 8 ++++----
>  xen/arch/x86/cpu/cpu.h                 | 5 +++++
>  xen/arch/x86/physdev.c                 | 4 ++++
>  xen/arch/x86/platform_hypercall.c      | 5 +++++
>  xen/arch/x86/x86_64/compat/mm.c        | 5 +++++
>  xen/arch/x86/x86_64/mmconfig.h         | 5 +++++
>  xen/arch/x86/x86_emulate/private.h     | 5 +++++
>  xen/arch/x86/x86_emulate/x86_emulate.c | 5 +++++
>  8 files changed, 38 insertions(+), 4 deletions(-)

Considering that the description talks of header files alone, there's a
lot of non-header-file churn here.

> --- a/xen/arch/x86/Makefile
> +++ b/xen/arch/x86/Makefile
> @@ -259,17 +259,17 @@ $(objtree)/arch/x86/include/asm/asm-macros.h: $(obj)/asm-macros.i $(src)/Makefil
>  	$(call filechk,asm-macros.h)
>  
>  define filechk_asm-macros.h
> +    echo '#ifndef __ASM_MACROS_H__'; \
> +    echo '#define __ASM_MACROS_H__'; \
>      echo '#if 0'; \
>      echo '.if 0'; \
>      echo '#endif'; \
> -    echo '#ifndef __ASM_MACROS_H__'; \
> -    echo '#define __ASM_MACROS_H__'; \
>      echo 'asm ( ".include \"$@\"" );'; \
> -    echo '#endif /* __ASM_MACROS_H__ */'; \
>      echo '#if 0'; \
>      echo '.endif'; \
>      cat $<; \
> -    echo '#endif'
> +    echo '#endif'; \
> +    echo '#endif /* __ASM_MACROS_H__ */'
>  endef

Can you please explain why this needs adjustment? While I think things
are going to be okay with the adjustment, this dual C and assembler
construct would imo better be left alone. Plus as per context found in
patch 2, aren't generated headers excluded anyway?

Jan

Re: [XEN PATCH 06/13] x86/EFI: address violations of MISRA C:2012 Directive 4.10

[Jan Beulich]

On 28.08.2023 15:20, Simone Ballarin wrote:
> Add inclusion guards to address violations of
> MISRA C:2012 Directive 4.10 ("Precautions shall be taken in order
> to prevent the contents of a header file being included more than
> once").
> 
> Mechanical change.
> 
> Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>
> ---
>  xen/arch/x86/efi/efi-boot.h | 6 ++++++
>  xen/arch/x86/efi/runtime.h  | 5 +++++
>  2 files changed, 11 insertions(+)
> 
> diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
> index 92f4cfe8bd..2c6be062cc 100644
> --- a/xen/arch/x86/efi/efi-boot.h
> +++ b/xen/arch/x86/efi/efi-boot.h
> @@ -3,6 +3,10 @@
>   * is intended to be included by common/efi/boot.c _only_, and
>   * therefore can define arch specific global variables.
>   */
> +
> +#ifndef __X86_EFI_EFI_BOOT_H__
> +#define __X86_EFI_EFI_BOOT_H__

Considering the comment, I find the need for a guard here quite
"interesting", to be honest.

Jan

Re: [XEN PATCH 01/13] misra: add deviation for headers that explicitly avoid guards

[Simone Ballarin]

On 29/08/23 08:33, Jan Beulich wrote:
> On 28.08.2023 15:19, Simone Ballarin wrote:
>> --- a/docs/misra/rules.rst
>> +++ b/docs/misra/rules.rst
>> @@ -60,7 +60,8 @@ maintainers if you want to suggest a change.
>>        - Precautions shall be taken in order to prevent the contents of a
>>          header file being included more than once
>>        - Files that are intended to be included more than once do not need to
>> -       conform to the directive
>> +       conform to the directive. Files that explicitly avoid inclusion guards
>> +       under specific circumstances do not need to conform the directive.
> 
> Nit: There's a "to" missing near the end of the added sentence. Can likely
> be taken care of while committing, since Stefano has already ack-ed this,
> but I'd like to still raise the question of the utility of this statement:
> How is one to know whether omission of guards is intentional?

As suggested by Stefano, this kind of deviation should be done using
the format specified in safe.json, and I will follow his suggestion in 
the next submission. The statement will disappear.

> 
> Jan
> 
> 
> 

-- 
Simone Ballarin, M.Sc.

Field Application Engineer, BUGSENG (https://bugseng.com)

Re: [XEN PATCH 01/13] misra: add deviation for headers that explicitly avoid guards

[Simone Ballarin]

On 29/08/23 00:32, Stefano Stabellini wrote:
> On Mon, 28 Aug 2023, Stefano Stabellini wrote:
>> On Mon, 28 Aug 2023, Simone Ballarin wrote:
>>> Some headers, under specific circumstances (documented in a comment at
>>> the beginning of the file), explicitly avoid inclusion guards: the caller
>>> is responsible for including them correctly.
>>>
>>> These files are not supposed to comply with Directive 4.10:
>>> "Precautions shall be taken in order to prevent the contents of a header
>>> file being included more than once"
>>>
>>> This patch adds a deviation for all headers that contain the following
>>> in a comment text:
>>> "In this case, no inclusion guards apply and the caller is responsible"
>>>
>>> Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>
>>
>> Acked-by: Stefano Stabellini <sstabellini@kernel.org>
> 
> Actually one question
> 
> 
>>> ---
>>>   automation/eclair_analysis/ECLAIR/deviations.ecl | 4 ++++
>>>   docs/misra/rules.rst                             | 3 ++-
>>>   2 files changed, 6 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl b/automation/eclair_analysis/ECLAIR/deviations.ecl
>>> index d8170106b4..5f068377fa 100644
>>> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl
>>> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
>>> @@ -91,6 +91,10 @@ conform to the directive."
>>>   -config=MC3R1.D4.10,reports+={safe, "first_area(text(^/\\* Generated file, do not edit! \\*/$, begin-3))"}
>>>   -doc_end
>>>   
>>> +-doc_begin="Some headers, under specific circumstances, explicitly avoid inclusion guards."
>>> +-config=MC3R1.D4.10,reports+={safe, "first_area(text(^ \\* In this case, no inclusion guards apply and the caller is responsible.*\\*/$, begin-1))"}
>>> +-doc_end
> 
> Is this supposed to match with any files starting with "In this case,
> no inclusion..." ?
> 
> We should use the format introduced by safe.json instead
> 

I agree, I will do it in the next submission.

-- 
Simone Ballarin, M.Sc.

Field Application Engineer, BUGSENG (https://bugseng.com)

Re: [XEN PATCH 02/13] automation/eclair: add text-based deviation for empty headers

[Simone Ballarin]

On 29/08/23 00:00, Stefano Stabellini wrote:
> On Mon, 28 Aug 2023, Simone Ballarin wrote:
>> This patch adds a text-based deviation for Directive 4.10:
>> "Precautions shall be taken in order to prevent the contents of
>> a header file being included more than once"
>>
>> Headers starting with the following comment are not supposed to
>> comply with the directive:
>> "/* empty */"
>>
>> These headers should be empty, therefore they pose no risk if included
>> more than once.
>>
>> Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>
> 
> Acked-by: Stefano Stabellini <sstabellini@kernel.org>
> 
> However I think we should also update rules.rst and/or update
> docs/misra/safe.json

I will do it in the next submission.
> 
> 
>> ---
>>   automation/eclair_analysis/ECLAIR/deviations.ecl | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl b/automation/eclair_analysis/ECLAIR/deviations.ecl
>> index 5f068377fa..2681a4cff5 100644
>> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl
>> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
>> @@ -80,6 +80,7 @@ inline functions."
>>   
>>   -doc_begin="This header file is autogenerated or empty, therefore it poses no
>>   risk if included more than once."
>> +-config=MC3R1.D4.10,reports+={safe, "first_area(text(^/\\* empty \\*/$, begin-1))"}
>>   -file_tag+={empty_header, "^xen/arch/arm/efi/runtime\\.h$"}
>>   -file_tag+={autogen_headers, "^xen/include/xen/compile\\.h$||^xen/include/generated/autoconf.h$||^xen/include/xen/hypercall-defs.h$"}
>>   -config=MC3R1.D4.10,reports+={safe, "all_area(all_loc(file(empty_header||autogen_headers)))"}
>> -- 
>> 2.34.1
>>
> 

-- 
Simone Ballarin, M.Sc.

Field Application Engineer, BUGSENG (https://bugseng.com)

Re: [XEN PATCH 02/13] automation/eclair: add text-based deviation for empty headers

[Simone Ballarin]

On 29/08/23 08:35, Jan Beulich wrote:
> On 28.08.2023 15:19, Simone Ballarin wrote:
>> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl
>> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
>> @@ -80,6 +80,7 @@ inline functions."
>>   
>>   -doc_begin="This header file is autogenerated or empty, therefore it poses no
>>   risk if included more than once."
> 
> While unrelated to, the change at hand, I still have a question on this:
> How come it is deemed universally safe to multi-include generated headers.
> I would have said that whether that's safe depends on the nature of the
> generated code in the header. Only truly empty ones are uniformly safe to
> include any number of times.

Yes, I agree with you. The mere fact that a file is auto-generated does 
not imply anything, moreover, this deviation is not even reported in 
rule.rst. In the next series, I'll drop it.

> 
> Jan
> 
>> +-config=MC3R1.D4.10,reports+={safe, "first_area(text(^/\\* empty \\*/$, begin-1))"}
>>   -file_tag+={empty_header, "^xen/arch/arm/efi/runtime\\.h$"}
>>   -file_tag+={autogen_headers, "^xen/include/xen/compile\\.h$||^xen/include/generated/autoconf.h$||^xen/include/xen/hypercall-defs.h$"}
>>   -config=MC3R1.D4.10,reports+={safe, "all_area(all_loc(file(empty_header||autogen_headers)))"}
> 

-- 
Simone Ballarin, M.Sc.

Field Application Engineer, BUGSENG (https://bugseng.com)

Re: [XEN PATCH 03/13] xen/arm: address violations of MISRA C:2012 Directive 4.10

[Simone Ballarin]

On 29/08/23 00:10, Julien Grall wrote:
> Hi,
> 
> On Mon, 28 Aug 2023 at 09:20, Simone Ballarin <simone.ballarin@bugseng.com>
> wrote:
> 
>> Add or move inclusion guards to address violations of
>> MISRA C:2012 Directive 4.10 ("Precautions shall be taken in order
>> to prevent the contents of a header file being included more than
>> once").
>>
>> Inclusion guards must appear at the beginning of the headers
>> (comments are permitted anywhere).
>>
>> Mechanical change.
>>
>> Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>
>> ---
>>   xen/arch/arm/efi/efi-boot.h          | 6 ++++++
>>   xen/arch/arm/include/asm/hypercall.h | 6 +++---
>>   xen/arch/arm/include/asm/iocap.h     | 6 +++---
>>   3 files changed, 12 insertions(+), 6 deletions(-)
>>
>> diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
>> index 1c3640bb65..aba522ead5 100644
>> --- a/xen/arch/arm/efi/efi-boot.h
>> +++ b/xen/arch/arm/efi/efi-boot.h
>> @@ -3,6 +3,10 @@
>>    * is intended to be included by common/efi/boot.c _only_, and
>>    * therefore can define arch specific global variables.
>>    */
>> +
>> +#ifndef __ARM_EFI_EFI_BOOT_H__
>> +#define __ARM_EFI_EFI_BOOT_H__
>> +
>>   #include <xen/device_tree.h>
>>   #include <xen/libfdt/libfdt.h>
>>   #include <asm/setup.h>
>> @@ -1003,6 +1007,8 @@ static void __init efi_arch_flush_dcache_area(const
>> void *vaddr, UINTN size)
>>       __flush_dcache_area(vaddr, size);
>>   }
>>
>> +#endif /* __ARM_EFI_EFI_BOOT_H__*/
>> +
>>   /*
>>    * Local variables:
>>    * mode: C
>> diff --git a/xen/arch/arm/include/asm/hypercall.h
>> b/xen/arch/arm/include/asm/hypercall.h
>> index ccd26c5184..4f4d96f1f2 100644
>> --- a/xen/arch/arm/include/asm/hypercall.h
>> +++ b/xen/arch/arm/include/asm/hypercall.h
>> @@ -1,10 +1,10 @@
>> +#ifndef __ASM_ARM_HYPERCALL_H__
>> +#define __ASM_ARM_HYPERCALL_H__
>> +
>>   #ifndef __XEN_HYPERCALL_H__
>>   #error "asm/hypercall.h should not be included directly - include
>> xen/hypercall.h instead"
>>   #endif
>>
>> -#ifndef __ASM_ARM_HYPERCALL_H__
>> -#define __ASM_ARM_HYPERCALL_H__
>> -
> 
> 
> I understand that you are trying to fix a misra violation. However, this
> feels like it was done on purpose.
> 
> With the new change, you would not always check that the file were included
> at the correct place. I am not against this change but this ought to be
> explained.
I don't think the semantics have changed. Please correct me if I'm wrong.

With this change, the only situation where the check is not performed is 
when __ASM_ARM_HYPERCALL_H__ is defined (i.e. the file has already been 
successfully included). This implies that if __ASM_ARM_HYPERCALL_H__ is 
defined, then __XEN_HYPERCALL_H__ is also defined, so the check would be 
useless.

The same thing happened with the code before the change: if I include 
the file after xen/hypercall.h, the check will always succeed.

> 
> 
>>   #include <public/domctl.h> /* for arch_do_domctl */
>>
>>   long subarch_do_domctl(struct xen_domctl *domctl, struct domain *d,
>> diff --git a/xen/arch/arm/include/asm/iocap.h
>> b/xen/arch/arm/include/asm/iocap.h
>> index 276fefbc59..4db1b16839 100644
>> --- a/xen/arch/arm/include/asm/iocap.h
>> +++ b/xen/arch/arm/include/asm/iocap.h
>> @@ -1,10 +1,10 @@
>> -#ifndef __X86_IOCAP_H__
>> -#define __X86_IOCAP_H__
>> +#ifndef __ASM_ARM_IOCAP_H__
>> +#define __ASM_ARM_IOCAP_H__
>>
>>   #define cache_flush_permitted(d)                        \
>>       (!rangeset_is_empty((d)->iomem_caps))
>>
>> -#endif
>> +#endif /* __ASM_ARM_IOCAP_H__ */
> 
> 
> I don’t understand how this is related to the rest of the patch. You wrote
> that inclusion must appear first and this is the case here.
> 
> However the name is technically not correct. Is this really related to
> directive 4.10? If so, this should be clarified in the commit message. If
> not, then I think this should be in a separate commit.
>

Yes, you are right. This is not correlated to this series. I will put it 
on a separate commit.

> Cheers,
> 
> 
>>
>>   /*
>>    * Local variables:
>> --
>> 2.34.1
>>
>>
> 

-- 
Simone Ballarin, M.Sc.

Field Application Engineer, BUGSENG (https://bugseng.com)

Re: [XEN PATCH 03/13] xen/arm: address violations of MISRA C:2012 Directive 4.10

[Jan Beulich]

On 30.08.2023 14:53, Simone Ballarin wrote:
> On 29/08/23 00:10, Julien Grall wrote:
>> On Mon, 28 Aug 2023 at 09:20, Simone Ballarin <simone.ballarin@bugseng.com>
>> wrote:
>>> --- a/xen/arch/arm/include/asm/hypercall.h
>>> +++ b/xen/arch/arm/include/asm/hypercall.h
>>> @@ -1,10 +1,10 @@
>>> +#ifndef __ASM_ARM_HYPERCALL_H__
>>> +#define __ASM_ARM_HYPERCALL_H__
>>> +
>>>   #ifndef __XEN_HYPERCALL_H__
>>>   #error "asm/hypercall.h should not be included directly - include
>>> xen/hypercall.h instead"
>>>   #endif
>>>
>>> -#ifndef __ASM_ARM_HYPERCALL_H__
>>> -#define __ASM_ARM_HYPERCALL_H__
>>> -
>>
>>
>> I understand that you are trying to fix a misra violation. However, this
>> feels like it was done on purpose.
>>
>> With the new change, you would not always check that the file were included
>> at the correct place. I am not against this change but this ought to be
>> explained.
> I don't think the semantics have changed. Please correct me if I'm wrong.
> 
> With this change, the only situation where the check is not performed is 
> when __ASM_ARM_HYPERCALL_H__ is defined (i.e. the file has already been 
> successfully included). This implies that if __ASM_ARM_HYPERCALL_H__ is 
> defined, then __XEN_HYPERCALL_H__ is also defined, so the check would be 
> useless.
> 
> The same thing happened with the code before the change: if I include 
> the file after xen/hypercall.h, the check will always succeed.

Hmm, I think you're right, but I draw a different conclusion: The check
fails to work as intended. And this can only be repaired without your
adjustment.

Jan

Re: [XEN PATCH 03/13] xen/arm: address violations of MISRA C:2012 Directive 4.10

[Simone Ballarin]

On 30/08/23 15:01, Jan Beulich wrote:
> On 30.08.2023 14:53, Simone Ballarin wrote:
>> On 29/08/23 00:10, Julien Grall wrote:
>>> On Mon, 28 Aug 2023 at 09:20, Simone Ballarin <simone.ballarin@bugseng.com>
>>> wrote:
>>>> --- a/xen/arch/arm/include/asm/hypercall.h
>>>> +++ b/xen/arch/arm/include/asm/hypercall.h
>>>> @@ -1,10 +1,10 @@
>>>> +#ifndef __ASM_ARM_HYPERCALL_H__
>>>> +#define __ASM_ARM_HYPERCALL_H__
>>>> +
>>>>    #ifndef __XEN_HYPERCALL_H__
>>>>    #error "asm/hypercall.h should not be included directly - include
>>>> xen/hypercall.h instead"
>>>>    #endif
>>>>
>>>> -#ifndef __ASM_ARM_HYPERCALL_H__
>>>> -#define __ASM_ARM_HYPERCALL_H__
>>>> -
>>>
>>>
>>> I understand that you are trying to fix a misra violation. However, this
>>> feels like it was done on purpose.
>>>
>>> With the new change, you would not always check that the file were included
>>> at the correct place. I am not against this change but this ought to be
>>> explained.
>> I don't think the semantics have changed. Please correct me if I'm wrong.
>>
>> With this change, the only situation where the check is not performed is
>> when __ASM_ARM_HYPERCALL_H__ is defined (i.e. the file has already been
>> successfully included). This implies that if __ASM_ARM_HYPERCALL_H__ is
>> defined, then __XEN_HYPERCALL_H__ is also defined, so the check would be
>> useless.
>>
>> The same thing happened with the code before the change: if I include
>> the file after xen/hypercall.h, the check will always succeed.
> 
> Hmm, I think you're right, but I draw a different conclusion: The check
> fails to work as intended. And this can only be repaired without your
> adjustment.
> 

Ok, I will just deviate these cases.

> Jan

-- 
Simone Ballarin, M.Sc.

Field Application Engineer, BUGSENG (https://bugseng.com)

Re: [XEN PATCH 05/13] automation/eclair: add deviation for usercopy.c

[Simone Ballarin]

On 29/08/23 00:27, Stefano Stabellini wrote:
> +Nicola, Luca
> 
> On Mon, 28 Aug 2023, Simone Ballarin wrote:
>> xen/arch/x86/usercopy.c includes itself, so it is not supposed to
>> comply with Directive 4.10:
>> "Precautions shall be taken in order to prevent the contents of a
>> header file being included more than once"
>>
>> This patch adds a deviation for the file.
>>
>> Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>
>>
>> ---
>>   automation/eclair_analysis/ECLAIR/deviations.ecl | 4 ++++
>>   docs/misra/rules.rst                             | 2 ++
>>   2 files changed, 6 insertions(+)
>>
>> diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl b/automation/eclair_analysis/ECLAIR/deviations.ecl
>> index 2681a4cff5..a7d4f29b43 100644
>> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl
>> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
>> @@ -96,6 +96,10 @@ conform to the directive."
>>   -config=MC3R1.D4.10,reports+={safe, "first_area(text(^ \\* In this case, no inclusion guards apply and the caller is responsible.*\\*/$, begin-1))"}
>>   -doc_end
>>   
>> +-doc_begin="xen/arch/x86/usercopy.c includes itself: it is not supposed to comply with the directive"
>> +-config=MC3R1.D4.10,reports+={deliberate, "all_area(all_loc(file("^xen/arch/x86/usercopy\\.c$")))"}
>> +-doc_end
>> +
>>   #
>>   # Series 5.
>>   #
>> diff --git a/docs/misra/rules.rst b/docs/misra/rules.rst
>> index 4b1a7b02b6..45e13d0302 100644
>> --- a/docs/misra/rules.rst
>> +++ b/docs/misra/rules.rst
>> @@ -62,6 +62,8 @@ maintainers if you want to suggest a change.
>>        - Files that are intended to be included more than once do not need to
>>          conform to the directive. Files that explicitly avoid inclusion guards
>>          under specific circumstances do not need to conform the directive.
>> +       xen/arch/x86/usercopy.c includes itself: it is not supposed to comply
>> +       with the directive.
> 
> 
> We need to find a consistent way to document this kind of deviations in
> a non-ECLAIR specific way, without adding the complete list of
> deviations to rules.rst.
> 
> Can we use safe.json and add an in-code comment at the top of
> usercopy.c? E.g.:
> 
> diff --git a/xen/arch/x86/usercopy.c b/xen/arch/x86/usercopy.c
> index b8c2d1cc0b..8bb591f472 100644
> --- a/xen/arch/x86/usercopy.c
> +++ b/xen/arch/x86/usercopy.c
> @@ -1,3 +1,4 @@
> +/* SAF-1-safe */
>   /*
>    * User address space access functions.
>    *
>  > Otherwise, maybe we should extend safe.json to also have an extra field
> with a list of paths. For instance see "files" below >
> {
>      "version": "1.0",
>      "content": [
>          {
>              "id": "SAF-0-safe",
>              "analyser": {
>                  "eclair": "MC3R1.R8.6",
>                  "coverity": "misra_c_2012_rule_8_6_violation"
>              },
>              "name": "Rule 8.6: linker script defined symbols",
>              "text": "It is safe to declare this symbol because it is defined in the linker script."
>          },
>          {
>              "id": "SAF-1-safe",
>              "analyser": {
>                  "eclair": "MC3R1.D4.10"
>              },
>              "name": "Dir 4.10: files that include themselves",
>              "text": "Files purposely written to include themselves are not supposed to comply with D4.10.",
>              "files": ["xen/arch/x86/usercopy.c"]
>          },
>          {
>              "id": "SAF-2-safe",
>              "analyser": {},
>              "name": "Sentinel",
>              "text": "Next ID to be used"
>          }
>      ]
> }
> 
In general, I prefer the first option for such ad hoc deviation (the 
comment at the beginning of the file): this way, anyone who touches the 
file will immediately see the comment and think as its changes will 
affect the deviation (is it still safe? is it still necessary?).

To help the developer more, I think it is better to also add the "name" 
in the comment, this is my proposal:

/* SAF-4-safe Dir 4.10: files that include themselves*/
/*
  * User address space access functions.
  *
  * Copyright 1997 Andi Kleen <ak@muc.de>
  * Copyright 1997 Linus Torvalds
  * Copyright 2002 Andi Kleen <ak@suse.de>
  */

-- 
Simone Ballarin, M.Sc.

Field Application Engineer, BUGSENG (https://bugseng.com)

Re: [XEN PATCH 11/13] xen/sched: address violations of MISRA C:2012 Directive 4.10

[George Dunlap]

On Mon, Aug 28, 2023 at 2:20 PM Simone Ballarin
<simone.ballarin@bugseng.com> wrote:
>
> Add inclusion guards to address violations of
> MISRA C:2012 Directive 4.10 ("Precautions shall be taken in order
> to prevent the contents of a header file being included more than
> once").
>
> Mechanical change.
>
> Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>

Reviewed-by: George Dunlap <george.dunlap@cloud.com>

Re: [XEN PATCH 06/13] x86/EFI: address violations of MISRA C:2012 Directive 4.10

[Simone Ballarin]

On 29/08/23 15:27, Jan Beulich wrote:
> On 28.08.2023 15:20, Simone Ballarin wrote:
>> Add inclusion guards to address violations of
>> MISRA C:2012 Directive 4.10 ("Precautions shall be taken in order
>> to prevent the contents of a header file being included more than
>> once").
>>
>> Mechanical change.
>>
>> Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>
>> ---
>>   xen/arch/x86/efi/efi-boot.h | 6 ++++++
>>   xen/arch/x86/efi/runtime.h  | 5 +++++
>>   2 files changed, 11 insertions(+)
>>
>> diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
>> index 92f4cfe8bd..2c6be062cc 100644
>> --- a/xen/arch/x86/efi/efi-boot.h
>> +++ b/xen/arch/x86/efi/efi-boot.h
>> @@ -3,6 +3,10 @@
>>    * is intended to be included by common/efi/boot.c _only_, and
>>    * therefore can define arch specific global variables.
>>    */
>> +
>> +#ifndef __X86_EFI_EFI_BOOT_H__
>> +#define __X86_EFI_EFI_BOOT_H__
> 
> Considering the comment, I find the need for a guard here quite
> "interesting", to be honest.
> 
> Jan
I don't think a simple comment is enough to say that "precautions have 
been taken". For the moment, I will drop this change to keep this patch 
going.

-- 
Simone Ballarin, M.Sc.

Field Application Engineer, BUGSENG (https://bugseng.com)

Re: [XEN PATCH 07/13] x86/asm: address violations of MISRA C:2012 Directive 4.10

[Simone Ballarin]

On 29/08/23 00:30, Stefano Stabellini wrote:
> On Mon, 28 Aug 2023, Simone Ballarin wrote:
>> Add or move inclusion guards to address violations of
>> MISRA C:2012 Directive 4.10 ("Precautions shall be taken in order
>> to prevent the contents of a header file being included more than
>> once").
>>
>> Inclusion guards must appear at the beginning of the headers
>> (comments are permitted anywhere).
>>
>> The text of the beggining comment of cpufeatures.h has been changed
>> to match the deviation in automation/eclair_analysis/ECLAIR/deviations.ecl,
>> moreover this new formulation is already used in other files.
> 
> I don't think it is a good idea to do this kind of textual matching.
> Instead we should use the format introduced by safe.json, e.g.
> SAF-1-safe
> 

I agree. I will use a comment-based deviation.

> 
>> Mechanical change.
>>
>> Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>
>> ---
>>   xen/arch/x86/include/asm/compat.h      | 5 +++++
>>   xen/arch/x86/include/asm/cpufeatures.h | 4 +---
>>   xen/arch/x86/include/asm/efibind.h     | 5 +++++
>>   xen/arch/x86/include/asm/hypercall.h   | 6 +++---
>>   4 files changed, 14 insertions(+), 6 deletions(-)
>>
>> diff --git a/xen/arch/x86/include/asm/compat.h b/xen/arch/x86/include/asm/compat.h
>> index 818cad87db..3d3891d061 100644
>> --- a/xen/arch/x86/include/asm/compat.h
>> +++ b/xen/arch/x86/include/asm/compat.h
>> @@ -2,6 +2,9 @@
>>    * compat.h
>>    */
>>   
>> +#ifndef __ASM_X86_COMPAT_H__
>> +#define __ASM_X86_COMPAT_H__
>> +
>>   #ifdef CONFIG_COMPAT
>>   
>>   #define COMPAT_BITS_PER_LONG 32
>> @@ -18,3 +21,5 @@ int switch_compat(struct domain *);
>>   #include <xen/errno.h>
>>   static inline int switch_compat(struct domain *d) { return -EOPNOTSUPP; }
>>   #endif
>> +
>> +#endif /* __ASM_X86_COMPAT_H__ */
>> diff --git a/xen/arch/x86/include/asm/cpufeatures.h b/xen/arch/x86/include/asm/cpufeatures.h
>> index da0593de85..1dfdd478ab 100644
>> --- a/xen/arch/x86/include/asm/cpufeatures.h
>> +++ b/xen/arch/x86/include/asm/cpufeatures.h
>> @@ -1,6 +1,4 @@
>> -/*
>> - * Explicitly intended for multiple inclusion.
>> - */
>> +/* This file is legitimately included multiple times */
>>   
>>   #include <xen/lib/x86/cpuid-autogen.h>
>>   
>> diff --git a/xen/arch/x86/include/asm/efibind.h b/xen/arch/x86/include/asm/efibind.h
>> index bce02f3707..f2eb8b5496 100644
>> --- a/xen/arch/x86/include/asm/efibind.h
>> +++ b/xen/arch/x86/include/asm/efibind.h
>> @@ -1,2 +1,7 @@
>> +#ifndef __ASM_X86_EFIBIND_H__
>> +#define __ASM_X86_EFIBIND_H__
>> +
>>   #include <xen/types.h>
>>   #include <asm/x86_64/efibind.h>
>> +
>> +#endif /* __ASM_X86_EFIBIND_H__ */
>> diff --git a/xen/arch/x86/include/asm/hypercall.h b/xen/arch/x86/include/asm/hypercall.h
>> index ec2edc771e..2ade5d71b8 100644
>> --- a/xen/arch/x86/include/asm/hypercall.h
>> +++ b/xen/arch/x86/include/asm/hypercall.h
>> @@ -2,13 +2,13 @@
>>    * asm-x86/hypercall.h
>>    */
>>   
>> +#ifndef __ASM_X86_HYPERCALL_H__
>> +#define __ASM_X86_HYPERCALL_H__
>> +
>>   #ifndef __XEN_HYPERCALL_H__
>>   #error "asm/hypercall.h should not be included directly - include xen/hypercall.h instead"
>>   #endif
>>   
>> -#ifndef __ASM_X86_HYPERCALL_H__
>> -#define __ASM_X86_HYPERCALL_H__
>> -
>>   #include <xen/types.h>
>>   #include <public/physdev.h>
>>   #include <public/event_channel.h>
>> -- 
>> 2.34.1
>>

-- 
Simone Ballarin, M.Sc.

Field Application Engineer, BUGSENG (https://bugseng.com)

Re: [XEN PATCH 05/13] automation/eclair: add deviation for usercopy.c

[Stefano Stabellini]

On Wed, 30 Aug 2023, Simone Ballarin wrote:
> On 29/08/23 00:27, Stefano Stabellini wrote:
> > +Nicola, Luca
> > 
> > On Mon, 28 Aug 2023, Simone Ballarin wrote:
> > > xen/arch/x86/usercopy.c includes itself, so it is not supposed to
> > > comply with Directive 4.10:
> > > "Precautions shall be taken in order to prevent the contents of a
> > > header file being included more than once"
> > > 
> > > This patch adds a deviation for the file.
> > > 
> > > Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>
> > > 
> > > ---
> > >   automation/eclair_analysis/ECLAIR/deviations.ecl | 4 ++++
> > >   docs/misra/rules.rst                             | 2 ++
> > >   2 files changed, 6 insertions(+)
> > > 
> > > diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl
> > > b/automation/eclair_analysis/ECLAIR/deviations.ecl
> > > index 2681a4cff5..a7d4f29b43 100644
> > > --- a/automation/eclair_analysis/ECLAIR/deviations.ecl
> > > +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
> > > @@ -96,6 +96,10 @@ conform to the directive."
> > >   -config=MC3R1.D4.10,reports+={safe, "first_area(text(^ \\* In this case,
> > > no inclusion guards apply and the caller is responsible.*\\*/$,
> > > begin-1))"}
> > >   -doc_end
> > >   +-doc_begin="xen/arch/x86/usercopy.c includes itself: it is not supposed
> > > to comply with the directive"
> > > +-config=MC3R1.D4.10,reports+={deliberate,
> > > "all_area(all_loc(file("^xen/arch/x86/usercopy\\.c$")))"}
> > > +-doc_end
> > > +
> > >   #
> > >   # Series 5.
> > >   #
> > > diff --git a/docs/misra/rules.rst b/docs/misra/rules.rst
> > > index 4b1a7b02b6..45e13d0302 100644
> > > --- a/docs/misra/rules.rst
> > > +++ b/docs/misra/rules.rst
> > > @@ -62,6 +62,8 @@ maintainers if you want to suggest a change.
> > >        - Files that are intended to be included more than once do not need
> > > to
> > >          conform to the directive. Files that explicitly avoid inclusion
> > > guards
> > >          under specific circumstances do not need to conform the
> > > directive.
> > > +       xen/arch/x86/usercopy.c includes itself: it is not supposed to
> > > comply
> > > +       with the directive.
> > 
> > 
> > We need to find a consistent way to document this kind of deviations in
> > a non-ECLAIR specific way, without adding the complete list of
> > deviations to rules.rst.
> > 
> > Can we use safe.json and add an in-code comment at the top of
> > usercopy.c? E.g.:
> > 
> > diff --git a/xen/arch/x86/usercopy.c b/xen/arch/x86/usercopy.c
> > index b8c2d1cc0b..8bb591f472 100644
> > --- a/xen/arch/x86/usercopy.c
> > +++ b/xen/arch/x86/usercopy.c
> > @@ -1,3 +1,4 @@
> > +/* SAF-1-safe */
> >   /*
> >    * User address space access functions.
> >    *
> >  > Otherwise, maybe we should extend safe.json to also have an extra field
> > with a list of paths. For instance see "files" below >
> > {
> >      "version": "1.0",
> >      "content": [
> >          {
> >              "id": "SAF-0-safe",
> >              "analyser": {
> >                  "eclair": "MC3R1.R8.6",
> >                  "coverity": "misra_c_2012_rule_8_6_violation"
> >              },
> >              "name": "Rule 8.6: linker script defined symbols",
> >              "text": "It is safe to declare this symbol because it is
> > defined in the linker script."
> >          },
> >          {
> >              "id": "SAF-1-safe",
> >              "analyser": {
> >                  "eclair": "MC3R1.D4.10"
> >              },
> >              "name": "Dir 4.10: files that include themselves",
> >              "text": "Files purposely written to include themselves are not
> > supposed to comply with D4.10.",
> >              "files": ["xen/arch/x86/usercopy.c"]
> >          },
> >          {
> >              "id": "SAF-2-safe",
> >              "analyser": {},
> >              "name": "Sentinel",
> >              "text": "Next ID to be used"
> >          }
> >      ]
> > }
> > 
> In general, I prefer the first option for such ad hoc deviation (the comment
> at the beginning of the file): this way, anyone who touches the file will
> immediately see the comment and think as its changes will affect the deviation
> (is it still safe? is it still necessary?).
> 
> To help the developer more, I think it is better to also add the "name" in the
> comment, this is my proposal:
> 
> /* SAF-4-safe Dir 4.10: files that include themselves*/

Yes, this is fine, it was always intended to be possible to add the
name of the deviation or a short comment in the in-code comment


> /*
>  * User address space access functions.
>  *
>  * Copyright 1997 Andi Kleen <ak@muc.de>
>  * Copyright 1997 Linus Torvalds
>  * Copyright 2002 Andi Kleen <ak@suse.de>
>  */
> 
> -- 
> Simone Ballarin, M.Sc.
> 
> Field Application Engineer, BUGSENG (https://bugseng.com)
> 

Re: [XEN PATCH 05/13] automation/eclair: add deviation for usercopy.c

[Jan Beulich]

On 31.08.2023 03:56, Stefano Stabellini wrote:
> On Wed, 30 Aug 2023, Simone Ballarin wrote:
>> On 29/08/23 00:27, Stefano Stabellini wrote:
>>> On Mon, 28 Aug 2023, Simone Ballarin wrote:
>>> --- a/xen/arch/x86/usercopy.c
>>> +++ b/xen/arch/x86/usercopy.c
>>> @@ -1,3 +1,4 @@
>>> +/* SAF-1-safe */
>>>   /*
>>>    * User address space access functions.
>>>    *
>>>  > Otherwise, maybe we should extend safe.json to also have an extra field
>>> with a list of paths. For instance see "files" below >
>>> {
>>>      "version": "1.0",
>>>      "content": [
>>>          {
>>>              "id": "SAF-0-safe",
>>>              "analyser": {
>>>                  "eclair": "MC3R1.R8.6",
>>>                  "coverity": "misra_c_2012_rule_8_6_violation"
>>>              },
>>>              "name": "Rule 8.6: linker script defined symbols",
>>>              "text": "It is safe to declare this symbol because it is
>>> defined in the linker script."
>>>          },
>>>          {
>>>              "id": "SAF-1-safe",
>>>              "analyser": {
>>>                  "eclair": "MC3R1.D4.10"
>>>              },
>>>              "name": "Dir 4.10: files that include themselves",
>>>              "text": "Files purposely written to include themselves are not
>>> supposed to comply with D4.10.",
>>>              "files": ["xen/arch/x86/usercopy.c"]
>>>          },
>>>          {
>>>              "id": "SAF-2-safe",
>>>              "analyser": {},
>>>              "name": "Sentinel",
>>>              "text": "Next ID to be used"
>>>          }
>>>      ]
>>> }
>>>
>> In general, I prefer the first option for such ad hoc deviation (the comment
>> at the beginning of the file): this way, anyone who touches the file will
>> immediately see the comment and think as its changes will affect the deviation
>> (is it still safe? is it still necessary?).
>>
>> To help the developer more, I think it is better to also add the "name" in the
>> comment, this is my proposal:
>>
>> /* SAF-4-safe Dir 4.10: files that include themselves*/
> 
> Yes, this is fine, it was always intended to be possible to add the
> name of the deviation or a short comment in the in-code comment

But then either the directive number wants omitting, or the Misra version
needs to also be stated.

Jan

Re: [XEN PATCH 09/13] xen/common: address violations of MISRA C:2012 Directive 4.10

[Simone Ballarin]

On 29/08/23 08:50, Jan Beulich wrote:
> On 28.08.2023 15:20, Simone Ballarin wrote:
>> Add inclusion guards to address violations of
>> MISRA C:2012 Directive 4.10 ("Precautions shall be taken in order
>> to prevent the contents of a header file being included more than
>> once").
>>
>> Also C files, if included somewhere, need to comply with the guideline.
>>
>> Mechanical change.
>>
>> Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>
>> ---
>>   xen/common/compat/grant_table.c | 7 +++++++
>>   xen/common/coverage/gcc_4_7.c   | 5 +++++
>>   xen/common/decompress.h         | 5 +++++
>>   xen/common/event_channel.h      | 5 +++++
>>   xen/common/multicall.c          | 5 +++++
>>   5 files changed, 27 insertions(+)
> 
> As already said in reply to another patch, imo .c files shouldn't gain such
> guards. These are commonly referred to as "header guards" for a reason.
> 

This is the MISRA's definition of "header file" (MISRA C:2012 Revision 
1, Appendix J):

   "A header file is any file that is the subject of a #include
    directive.
    Note: the filename extension is not significant."

So, the guards are required if we want to comply with the directive, 
otherwise we can raise a deviation.

The danger of multi-inclusion also exists for .c files, why do you want 
to avoid guards for them?

>> --- a/xen/common/compat/grant_table.c
>> +++ b/xen/common/compat/grant_table.c
>> @@ -3,6 +3,10 @@
>>    *
>>    */
>>   
>> +
> 
> Nit: No double blank lines please.
> 
>> +#ifndef __COMMON_COMPAT_GRANT_TABLE_C__
>> +#define __COMMON_COMPAT_GRANT_TABLE_C__
>> +
>>   #include <xen/hypercall.h>
>>   #include <compat/grant_table.h>
>>   
>> @@ -331,6 +335,9 @@ int compat_grant_table_op(
>>       return rc;
>>   }
>>   
>> +
> 
> Again here (at least).
> 
> Jan

-- 
Simone Ballarin, M.Sc.

Field Application Engineer, BUGSENG (https://bugseng.com)

Re: [XEN PATCH 09/13] xen/common: address violations of MISRA C:2012 Directive 4.10

[Jan Beulich]

On 31.08.2023 12:08, Simone Ballarin wrote:
> On 29/08/23 08:50, Jan Beulich wrote:
>> On 28.08.2023 15:20, Simone Ballarin wrote:
>>> Add inclusion guards to address violations of
>>> MISRA C:2012 Directive 4.10 ("Precautions shall be taken in order
>>> to prevent the contents of a header file being included more than
>>> once").
>>>
>>> Also C files, if included somewhere, need to comply with the guideline.
>>>
>>> Mechanical change.
>>>
>>> Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>
>>> ---
>>>   xen/common/compat/grant_table.c | 7 +++++++
>>>   xen/common/coverage/gcc_4_7.c   | 5 +++++
>>>   xen/common/decompress.h         | 5 +++++
>>>   xen/common/event_channel.h      | 5 +++++
>>>   xen/common/multicall.c          | 5 +++++
>>>   5 files changed, 27 insertions(+)
>>
>> As already said in reply to another patch, imo .c files shouldn't gain such
>> guards. These are commonly referred to as "header guards" for a reason.
>>
> 
> This is the MISRA's definition of "header file" (MISRA C:2012 Revision 
> 1, Appendix J):
> 
>    "A header file is any file that is the subject of a #include
>     directive.
>     Note: the filename extension is not significant."

That's completely misleading terminology then.

> So, the guards are required if we want to comply with the directive, 
> otherwise we can raise a deviation.
> 
> The danger of multi-inclusion also exists for .c files, why do you want 
> to avoid guards for them?

Counter question: Why only add guards to some of them? (My personal
answer is "Because it's extra clutter.")

Jan

Re: [XEN PATCH 12/13] xen: address violations of MISRA C:2012 Directive 4.10

[Simone Ballarin]

On 29/08/23 00:51, Stefano Stabellini wrote:
> On Mon, 28 Aug 2023, Simone Ballarin wrote:
>> Move or amended inclusion guards to address violations of
>> MISRA C:2012 Directive 4.10 ("Precautions shall be taken in order
>> to prevent the contents of a header file being included more than
>> once").
>>
>> Inclusion guards must appear at the beginning of the headers
>> (comments are permitted anywhere) and the #if directive cannot
>> be used for other checks.
>>
>> Mechanical change.
>>
>> Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>
>> ---
>>   xen/include/xen/err.h       | 4 +++-
>>   xen/include/xen/pci_ids.h   | 5 +++++
>>   xen/include/xen/softirq.h   | 4 +++-
>>   xen/include/xen/unaligned.h | 7 ++++---
>>   xen/include/xen/vmap.h      | 4 +++-
>>   5 files changed, 18 insertions(+), 6 deletions(-)
>>
>> diff --git a/xen/include/xen/err.h b/xen/include/xen/err.h
>> index 2f29b57d28..a6323d82d7 100644
>> --- a/xen/include/xen/err.h
>> +++ b/xen/include/xen/err.h
>> @@ -1,5 +1,6 @@
>> -#if !defined(__XEN_ERR_H__) && !defined(__ASSEMBLY__)
>> +#if !defined(__XEN_ERR_H__)
>>   #define __XEN_ERR_H__
>> +#if !defined(__ASSEMBLY__)
> 
> The original pattern was also guarding the header file sufficiently,
> protecting it from double-inclusion. In fact, it is posing stricter
> restrictions than usual (not laxer). This change is unnecessary?

The MISRA directive asks to use one of the two following forms:

<start-of-file>
#if !defined ( identifier )
#define identifier
/* Contents of file */
#endif
<end-of-file>

<start-of-file>
#ifndef identifier
#define identifier
/* Contents of file */
#endif
<end-of-file>

I do not see any reason for deviating, but if you ask that, I can do it.

> 
> 
>>   #include <xen/compiler.h>
>>   #include <xen/errno.h>
>> @@ -54,4 +55,5 @@ static inline int __must_check PTR_RET(const void *ptr)
>>   	return IS_ERR(ptr) ? PTR_ERR(ptr) : 0;
>>   }
>>   
>> +#endif /* __ASSEMBLY__ */
>>   #endif /* __XEN_ERR_H__ */
>> diff --git a/xen/include/xen/pci_ids.h b/xen/include/xen/pci_ids.h
>> index e798477a7e..1a739d4c92 100644
>> --- a/xen/include/xen/pci_ids.h
>> +++ b/xen/include/xen/pci_ids.h
>> @@ -1,3 +1,6 @@
>> +#ifndef __XEN_PCI_IDS_H__
>> +#define __XEN_PCI_IDS_H__
>> +
>>   #define PCI_VENDOR_ID_AMD                0x1022
>>   
>>   #define PCI_VENDOR_ID_NVIDIA             0x10de
>> @@ -11,3 +14,5 @@
>>   #define PCI_VENDOR_ID_BROADCOM           0x14e4
>>   
>>   #define PCI_VENDOR_ID_INTEL              0x8086
>> +
>> +#endif /* __XEN_PCI_IDS_H__ */
>> diff --git a/xen/include/xen/softirq.h b/xen/include/xen/softirq.h
>> index 33d6f2ecd2..092ec733b7 100644
>> --- a/xen/include/xen/softirq.h
>> +++ b/xen/include/xen/softirq.h
>> @@ -1,5 +1,6 @@
>> -#if !defined(__XEN_SOFTIRQ_H__) && !defined(__ASSEMBLY__)
>> +#if !defined(__XEN_SOFTIRQ_H__)
>>   #define __XEN_SOFTIRQ_H__
>> +#if !defined(__ASSEMBLY__)
> 
> same here
> 
> 
>>   /* Low-latency softirqs come first in the following list. */
>>   enum {
>> @@ -40,4 +41,5 @@ void cpu_raise_softirq_batch_finish(void);
>>    */
>>   void process_pending_softirqs(void);
>>   
>> +#endif /* __ASSEMBLY__ */
>>   #endif /* __XEN_SOFTIRQ_H__ */
>> diff --git a/xen/include/xen/unaligned.h b/xen/include/xen/unaligned.h
>> index 0a2b16d05d..45f03b3f1b 100644
>> --- a/xen/include/xen/unaligned.h
>> +++ b/xen/include/xen/unaligned.h
>> @@ -3,13 +3,14 @@
>>    * without faulting, and at least reasonably efficiently.  Other architectures
>>    * will need to have a custom asm/unaligned.h.
>>    */
>> -#ifndef __ASM_UNALIGNED_H__
>> -#error "xen/unaligned.h should not be included directly - include asm/unaligned.h instead"
>> -#endif
>>   
>>   #ifndef __XEN_UNALIGNED_H__
>>   #define __XEN_UNALIGNED_H__
>>   
>> +#ifndef __ASM_UNALIGNED_H__
>> +#error "xen/unaligned.h should not be included directly - include asm/unaligned.h instead"
>> +#endif
>> +
>>   #ifdef __XEN__
>>   #include <xen/types.h>
>>   #include <asm/byteorder.h>
>> diff --git a/xen/include/xen/vmap.h b/xen/include/xen/vmap.h
>> index b0f7632e89..7a61dea54a 100644
>> --- a/xen/include/xen/vmap.h
>> +++ b/xen/include/xen/vmap.h
>> @@ -1,5 +1,6 @@
>> -#if !defined(__XEN_VMAP_H__) && defined(VMAP_VIRT_START)
>> +#if !defined(__XEN_VMAP_H__)
>>   #define __XEN_VMAP_H__
>> +#if defined(VMAP_VIRT_START)
> 
> same here
> 
> 
>>   #include <xen/mm-frame.h>
>>   #include <xen/page-size.h>
>> @@ -38,4 +39,5 @@ static inline void vm_init(void)
>>       vm_init_type(VMAP_DEFAULT, (void *)VMAP_VIRT_START, arch_vmap_virt_end());
>>   }
>>   
>> +#endif /* VMAP_VIRT_START */
>>   #endif /* __XEN_VMAP_H__ */
>> -- 
>> 2.34.1
>>
> 

-- 
Simone Ballarin, M.Sc.

Field Application Engineer, BUGSENG (https://bugseng.com)

Re: [XEN PATCH 12/13] xen: address violations of MISRA C:2012 Directive 4.10

[Jan Beulich]

On 31.08.2023 14:18, Simone Ballarin wrote:
> On 29/08/23 00:51, Stefano Stabellini wrote:
>> On Mon, 28 Aug 2023, Simone Ballarin wrote:
>>> Move or amended inclusion guards to address violations of
>>> MISRA C:2012 Directive 4.10 ("Precautions shall be taken in order
>>> to prevent the contents of a header file being included more than
>>> once").
>>>
>>> Inclusion guards must appear at the beginning of the headers
>>> (comments are permitted anywhere) and the #if directive cannot
>>> be used for other checks.
>>>
>>> Mechanical change.
>>>
>>> Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>
>>> ---
>>>   xen/include/xen/err.h       | 4 +++-
>>>   xen/include/xen/pci_ids.h   | 5 +++++
>>>   xen/include/xen/softirq.h   | 4 +++-
>>>   xen/include/xen/unaligned.h | 7 ++++---
>>>   xen/include/xen/vmap.h      | 4 +++-
>>>   5 files changed, 18 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/xen/include/xen/err.h b/xen/include/xen/err.h
>>> index 2f29b57d28..a6323d82d7 100644
>>> --- a/xen/include/xen/err.h
>>> +++ b/xen/include/xen/err.h
>>> @@ -1,5 +1,6 @@
>>> -#if !defined(__XEN_ERR_H__) && !defined(__ASSEMBLY__)
>>> +#if !defined(__XEN_ERR_H__)
>>>   #define __XEN_ERR_H__
>>> +#if !defined(__ASSEMBLY__)
>>
>> The original pattern was also guarding the header file sufficiently,
>> protecting it from double-inclusion. In fact, it is posing stricter
>> restrictions than usual (not laxer). This change is unnecessary?
> 
> The MISRA directive asks to use one of the two following forms:
> 
> <start-of-file>
> #if !defined ( identifier )
> #define identifier
> /* Contents of file */
> #endif
> <end-of-file>
> 
> <start-of-file>
> #ifndef identifier
> #define identifier
> /* Contents of file */
> #endif
> <end-of-file>
> 
> I do not see any reason for deviating, but if you ask that, I can do it.

I do not see a reason why a deviation would be needed here. Misra shouldn't
be more pedantic / restrictive than necessary to achieve its goals. Looking
at the flood of changes we've already seen, pointless changes really
shouldn't be asked for.

Jan

Re: [XEN PATCH 09/13] xen/common: address violations of MISRA C:2012 Directive 4.10

[Simone Ballarin]

On 31/08/23 13:10, Jan Beulich wrote:
> On 31.08.2023 12:08, Simone Ballarin wrote:
>> On 29/08/23 08:50, Jan Beulich wrote:
>>> On 28.08.2023 15:20, Simone Ballarin wrote:
>>>> Add inclusion guards to address violations of
>>>> MISRA C:2012 Directive 4.10 ("Precautions shall be taken in order
>>>> to prevent the contents of a header file being included more than
>>>> once").
>>>>
>>>> Also C files, if included somewhere, need to comply with the guideline.
>>>>
>>>> Mechanical change.
>>>>
>>>> Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>
>>>> ---
>>>>    xen/common/compat/grant_table.c | 7 +++++++
>>>>    xen/common/coverage/gcc_4_7.c   | 5 +++++
>>>>    xen/common/decompress.h         | 5 +++++
>>>>    xen/common/event_channel.h      | 5 +++++
>>>>    xen/common/multicall.c          | 5 +++++
>>>>    5 files changed, 27 insertions(+)
>>>
>>> As already said in reply to another patch, imo .c files shouldn't gain such
>>> guards. These are commonly referred to as "header guards" for a reason.
>>>
>>
>> This is the MISRA's definition of "header file" (MISRA C:2012 Revision
>> 1, Appendix J):
>>
>>     "A header file is any file that is the subject of a #include
>>      directive.
>>      Note: the filename extension is not significant."
> 
> That's completely misleading terminology then.
> 

I might agree with you on this, but either way this is the definition to 
apply when reading the guideline. IMHO here, the best would be to use a 
separate extension for "C files intended to be included" (but that's not 
the point).

>> So, the guards are required if we want to comply with the directive,
>> otherwise we can raise a deviation.
>>
>> The danger of multi-inclusion also exists for .c files, why do you want
>> to avoid guards for them?
> 
> Counter question: Why only add guards to some of them? (My personal
> answer is "Because it's extra clutter.")
> 
> Jan
> 

It's not "some of them", it's exactly the ones used in an #include 
directive, so I'm not getting your objection.

By the way, if the community agrees, I can deviate all C files and
drop the changes.

-- 
Simone Ballarin, M.Sc.

Field Application Engineer, BUGSENG (https://bugseng.com)

Re: [XEN PATCH 09/13] xen/common: address violations of MISRA C:2012 Directive 4.10

[Jan Beulich]

On 31.08.2023 14:54, Simone Ballarin wrote:
> On 31/08/23 13:10, Jan Beulich wrote:
>> On 31.08.2023 12:08, Simone Ballarin wrote:
>>> The danger of multi-inclusion also exists for .c files, why do you want
>>> to avoid guards for them?
>>
>> Counter question: Why only add guards to some of them? (My personal
>> answer is "Because it's extra clutter.")
> 
> It's not "some of them", it's exactly the ones used in an #include 
> directive, so I'm not getting your objection.

My point is that by adding guards only for files we presently use in some
#include directive, we set us up for introducing new violations as soon
as another .c file becomes the subject of an #include. The more that it
is unusual to add guards in .c files, i.e. it is to be expected that
people wouldn't think about this extra Misra requirement.

Jan

Re: [XEN PATCH 09/13] xen/common: address violations of MISRA C:2012 Directive 4.10

[Simone Ballarin]

On 31/08/23 15:05, Jan Beulich wrote:
> On 31.08.2023 14:54, Simone Ballarin wrote:
>> On 31/08/23 13:10, Jan Beulich wrote:
>>> On 31.08.2023 12:08, Simone Ballarin wrote:
>>>> The danger of multi-inclusion also exists for .c files, why do you want
>>>> to avoid guards for them?
>>>
>>> Counter question: Why only add guards to some of them? (My personal
>>> answer is "Because it's extra clutter.")
>>
>> It's not "some of them", it's exactly the ones used in an #include
>> directive, so I'm not getting your objection.
> 
> My point is that by adding guards only for files we presently use in some
> #include directive, we set us up for introducing new violations as soon
> as another .c file becomes the subject of an #include.The more that it
> is unusual to add guards in .c files, i.e. it is to be expected that
> people wouldn't think about this extra Misra requirement.
> 
> Jan

I can agree to partially adopt the directive: I will add a deviation for 
C files in rules.txt.

-- 
Simone Ballarin, M.Sc.

Field Application Engineer, BUGSENG (https://bugseng.com)

Re: [XEN PATCH 05/13] automation/eclair: add deviation for usercopy.c

[Luca Fancellu]

> On 28 Aug 2023, at 23:27, Stefano Stabellini <sstabellini@kernel.org> wrote:
> 
> +Nicola, Luca
> 
> On Mon, 28 Aug 2023, Simone Ballarin wrote:
>> xen/arch/x86/usercopy.c includes itself, so it is not supposed to
>> comply with Directive 4.10:
>> "Precautions shall be taken in order to prevent the contents of a
>> header file being included more than once"
>> 
>> This patch adds a deviation for the file.
>> 
>> Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>
>> 
>> ---
>> automation/eclair_analysis/ECLAIR/deviations.ecl | 4 ++++
>> docs/misra/rules.rst                             | 2 ++
>> 2 files changed, 6 insertions(+)
>> 
>> diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl b/automation/eclair_analysis/ECLAIR/deviations.ecl
>> index 2681a4cff5..a7d4f29b43 100644
>> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl
>> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
>> @@ -96,6 +96,10 @@ conform to the directive."
>> -config=MC3R1.D4.10,reports+={safe, "first_area(text(^ \\* In this case, no inclusion guards apply and the caller is responsible.*\\*/$, begin-1))"}
>> -doc_end
>> 
>> +-doc_begin="xen/arch/x86/usercopy.c includes itself: it is not supposed to comply with the directive"
>> +-config=MC3R1.D4.10,reports+={deliberate, "all_area(all_loc(file("^xen/arch/x86/usercopy\\.c$")))"}
>> +-doc_end
>> +
>> #
>> # Series 5.
>> #
>> diff --git a/docs/misra/rules.rst b/docs/misra/rules.rst
>> index 4b1a7b02b6..45e13d0302 100644
>> --- a/docs/misra/rules.rst
>> +++ b/docs/misra/rules.rst
>> @@ -62,6 +62,8 @@ maintainers if you want to suggest a change.
>>      - Files that are intended to be included more than once do not need to
>>        conform to the directive. Files that explicitly avoid inclusion guards
>>        under specific circumstances do not need to conform the directive.
>> +       xen/arch/x86/usercopy.c includes itself: it is not supposed to comply
>> +       with the directive.
> 
> 
> We need to find a consistent way to document this kind of deviations in
> a non-ECLAIR specific way, without adding the complete list of
> deviations to rules.rst.
> 
> Can we use safe.json and add an in-code comment at the top of
> usercopy.c? E.g.:
> 
> diff --git a/xen/arch/x86/usercopy.c b/xen/arch/x86/usercopy.c
> index b8c2d1cc0b..8bb591f472 100644
> --- a/xen/arch/x86/usercopy.c
> +++ b/xen/arch/x86/usercopy.c
> @@ -1,3 +1,4 @@
> +/* SAF-1-safe */
> /* 
>  * User address space access functions.
>  *
> 
> Otherwise, maybe we should extend safe.json to also have an extra field
> with a list of paths. For instance see "files" below:
> 
> {
>    "version": "1.0",
>    "content": [
>        {
>            "id": "SAF-0-safe",
>            "analyser": {
>                "eclair": "MC3R1.R8.6",
>                "coverity": "misra_c_2012_rule_8_6_violation"
>            },
>            "name": "Rule 8.6: linker script defined symbols",
>            "text": "It is safe to declare this symbol because it is defined in the linker script."
>        },
>        {
>            "id": "SAF-1-safe",
>            "analyser": {
>                "eclair": "MC3R1.D4.10"
>            },
>            "name": "Dir 4.10: files that include themselves",
>            "text": "Files purposely written to include themselves are not supposed to comply with D4.10.",
>            "files": ["xen/arch/x86/usercopy.c"]

Why couldn’t we do it without the “files” field? The presence of the tag in the file and the justification (I think)
are enough. 

>        },
>        {
>            "id": "SAF-2-safe",
>            "analyser": {},
>            "name": "Sentinel",
>            "text": "Next ID to be used"
>        }
>    ]
> }

Re: [XEN PATCH 09/13] xen/common: address violations of MISRA C:2012 Directive 4.10

[Stefano Stabellini]

On Thu, 31 Aug 2023, Simone Ballarin wrote:
> On 31/08/23 15:05, Jan Beulich wrote:
> > On 31.08.2023 14:54, Simone Ballarin wrote:
> > > On 31/08/23 13:10, Jan Beulich wrote:
> > > > On 31.08.2023 12:08, Simone Ballarin wrote:
> > > > > The danger of multi-inclusion also exists for .c files, why do you
> > > > > want
> > > > > to avoid guards for them?
> > > > 
> > > > Counter question: Why only add guards to some of them? (My personal
> > > > answer is "Because it's extra clutter.")
> > > 
> > > It's not "some of them", it's exactly the ones used in an #include
> > > directive, so I'm not getting your objection.
> > 
> > My point is that by adding guards only for files we presently use in some
> > #include directive, we set us up for introducing new violations as soon
> > as another .c file becomes the subject of an #include.The more that it
> > is unusual to add guards in .c files, i.e. it is to be expected that
> > people wouldn't think about this extra Misra requirement.
> > 
> > Jan
> 
> I can agree to partially adopt the directive: I will add a deviation for C
> files in rules.txt.

Sorry for the late reply as I was OOO. Please hold on before adding a
deviation for C files.

In general, I think including .c files is not common behavior, and
should be restricted to special cases. We could say that exactly because
they are special, they follow different rules so we can skip the guards.
Or we could say that they are still at risk of double-inclusion, hence
we should be consistent and protect them too. I think we should discuss
the topic during the next MISRA C meeting.

Re: [XEN PATCH 12/13] xen: address violations of MISRA C:2012 Directive 4.10

[Stefano Stabellini]

On Thu, 31 Aug 2023, Simone Ballarin wrote:
> On 29/08/23 00:51, Stefano Stabellini wrote:
> > On Mon, 28 Aug 2023, Simone Ballarin wrote:
> > > Move or amended inclusion guards to address violations of
> > > MISRA C:2012 Directive 4.10 ("Precautions shall be taken in order
> > > to prevent the contents of a header file being included more than
> > > once").
> > > 
> > > Inclusion guards must appear at the beginning of the headers
> > > (comments are permitted anywhere) and the #if directive cannot
> > > be used for other checks.
> > > 
> > > Mechanical change.
> > > 
> > > Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>
> > > ---
> > >   xen/include/xen/err.h       | 4 +++-
> > >   xen/include/xen/pci_ids.h   | 5 +++++
> > >   xen/include/xen/softirq.h   | 4 +++-
> > >   xen/include/xen/unaligned.h | 7 ++++---
> > >   xen/include/xen/vmap.h      | 4 +++-
> > >   5 files changed, 18 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/xen/include/xen/err.h b/xen/include/xen/err.h
> > > index 2f29b57d28..a6323d82d7 100644
> > > --- a/xen/include/xen/err.h
> > > +++ b/xen/include/xen/err.h
> > > @@ -1,5 +1,6 @@
> > > -#if !defined(__XEN_ERR_H__) && !defined(__ASSEMBLY__)
> > > +#if !defined(__XEN_ERR_H__)
> > >   #define __XEN_ERR_H__
> > > +#if !defined(__ASSEMBLY__)
> > 
> > The original pattern was also guarding the header file sufficiently,
> > protecting it from double-inclusion. In fact, it is posing stricter
> > restrictions than usual (not laxer). This change is unnecessary?
> 
> The MISRA directive asks to use one of the two following forms:
> 
> <start-of-file>
> #if !defined ( identifier )
> #define identifier
> /* Contents of file */
> #endif
> <end-of-file>
> 
> <start-of-file>
> #ifndef identifier
> #define identifier
> /* Contents of file */
> #endif
> <end-of-file>
> 
> I do not see any reason for deviating, but if you ask that, I can do it.

Let's follow MISRA's form.

Re: [XEN PATCH 09/13] xen/common: address violations of MISRA C:2012 Directive 4.10

[Jan Beulich]

On 06.09.2023 00:18, Stefano Stabellini wrote:
> On Thu, 31 Aug 2023, Simone Ballarin wrote:
>> On 31/08/23 15:05, Jan Beulich wrote:
>>> On 31.08.2023 14:54, Simone Ballarin wrote:
>>>> On 31/08/23 13:10, Jan Beulich wrote:
>>>>> On 31.08.2023 12:08, Simone Ballarin wrote:
>>>>>> The danger of multi-inclusion also exists for .c files, why do you
>>>>>> want
>>>>>> to avoid guards for them?
>>>>>
>>>>> Counter question: Why only add guards to some of them? (My personal
>>>>> answer is "Because it's extra clutter.")
>>>>
>>>> It's not "some of them", it's exactly the ones used in an #include
>>>> directive, so I'm not getting your objection.
>>>
>>> My point is that by adding guards only for files we presently use in some
>>> #include directive, we set us up for introducing new violations as soon
>>> as another .c file becomes the subject of an #include.The more that it
>>> is unusual to add guards in .c files, i.e. it is to be expected that
>>> people wouldn't think about this extra Misra requirement.
>>
>> I can agree to partially adopt the directive: I will add a deviation for C
>> files in rules.txt.
> 
> Sorry for the late reply as I was OOO. Please hold on before adding a
> deviation for C files.
> 
> In general, I think including .c files is not common behavior, and
> should be restricted to special cases. We could say that exactly because
> they are special, they follow different rules so we can skip the guards.
> Or we could say that they are still at risk of double-inclusion, hence
> we should be consistent and protect them too. I think we should discuss
> the topic during the next MISRA C meeting.

Just one further remark right here: While including a header file a 2nd
time stands a fair chance of working (i.e. the compiler not spitting out
errors), that would be very unusual for a .c file: Every function or
data item it defines would (in the common case) be already defined.

Jan

Re: [XEN PATCH 12/13] xen: address violations of MISRA C:2012 Directive 4.10

[Jan Beulich]

On 06.09.2023 00:27, Stefano Stabellini wrote:
> On Thu, 31 Aug 2023, Simone Ballarin wrote:
>> On 29/08/23 00:51, Stefano Stabellini wrote:
>>> On Mon, 28 Aug 2023, Simone Ballarin wrote:
>>>> Move or amended inclusion guards to address violations of
>>>> MISRA C:2012 Directive 4.10 ("Precautions shall be taken in order
>>>> to prevent the contents of a header file being included more than
>>>> once").
>>>>
>>>> Inclusion guards must appear at the beginning of the headers
>>>> (comments are permitted anywhere) and the #if directive cannot
>>>> be used for other checks.
>>>>
>>>> Mechanical change.
>>>>
>>>> Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>
>>>> ---
>>>>   xen/include/xen/err.h       | 4 +++-
>>>>   xen/include/xen/pci_ids.h   | 5 +++++
>>>>   xen/include/xen/softirq.h   | 4 +++-
>>>>   xen/include/xen/unaligned.h | 7 ++++---
>>>>   xen/include/xen/vmap.h      | 4 +++-
>>>>   5 files changed, 18 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/xen/include/xen/err.h b/xen/include/xen/err.h
>>>> index 2f29b57d28..a6323d82d7 100644
>>>> --- a/xen/include/xen/err.h
>>>> +++ b/xen/include/xen/err.h
>>>> @@ -1,5 +1,6 @@
>>>> -#if !defined(__XEN_ERR_H__) && !defined(__ASSEMBLY__)
>>>> +#if !defined(__XEN_ERR_H__)
>>>>   #define __XEN_ERR_H__
>>>> +#if !defined(__ASSEMBLY__)
>>>
>>> The original pattern was also guarding the header file sufficiently,
>>> protecting it from double-inclusion. In fact, it is posing stricter
>>> restrictions than usual (not laxer). This change is unnecessary?
>>
>> The MISRA directive asks to use one of the two following forms:
>>
>> <start-of-file>
>> #if !defined ( identifier )
>> #define identifier
>> /* Contents of file */
>> #endif
>> <end-of-file>
>>
>> <start-of-file>
>> #ifndef identifier
>> #define identifier
>> /* Contents of file */
>> #endif
>> <end-of-file>
>>
>> I do not see any reason for deviating, but if you ask that, I can do it.
> 
> Let's follow MISRA's form.

This is what I strongly dislike: They could be less restrictive on the
exact patterns permitted without impacting the goal intended to be
reached. But it's all as simple as possible, not as flexible as possible.

In any event, if a transformation like what can still be seen in context
is to be made, then please #ifdef / #ifndef instead of defined(...)
whenever possible.

Jan

Re: [XEN PATCH 09/13] xen/common: address violations of MISRA C:2012 Directive 4.10

[Simone Ballarin]

On 06/09/23 00:18, Stefano Stabellini wrote:
> On Thu, 31 Aug 2023, Simone Ballarin wrote:
>> On 31/08/23 15:05, Jan Beulich wrote:
>>> On 31.08.2023 14:54, Simone Ballarin wrote:
>>>> On 31/08/23 13:10, Jan Beulich wrote:
>>>>> On 31.08.2023 12:08, Simone Ballarin wrote:
>>>>>> The danger of multi-inclusion also exists for .c files, why do you
>>>>>> want
>>>>>> to avoid guards for them?
>>>>>
>>>>> Counter question: Why only add guards to some of them? (My personal
>>>>> answer is "Because it's extra clutter.")
>>>>
>>>> It's not "some of them", it's exactly the ones used in an #include
>>>> directive, so I'm not getting your objection.
>>>
>>> My point is that by adding guards only for files we presently use in some
>>> #include directive, we set us up for introducing new violations as soon
>>> as another .c file becomes the subject of an #include.The more that it
>>> is unusual to add guards in .c files, i.e. it is to be expected that
>>> people wouldn't think about this extra Misra requirement.
>>>
>>> Jan
>>
>> I can agree to partially adopt the directive: I will add a deviation for C
>> files in rules.txt.
> 
> Sorry for the late reply as I was OOO. Please hold on before adding a
> deviation for C files.
> 
> In general, I think including .c files is not common behavior, and
> should be restricted to special cases. We could say that exactly because
> they are special, they follow different rules so we can skip the guards.
> Or we could say that they are still at risk of double-inclusion, hence
> we should be consistent and protect them too. I think we should discuss
> the topic during the next MISRA C meeting.
> 
Ok, I will drop changes in C files without adding the deviation.

-- 
Simone Ballarin, M.Sc.

Field Application Engineer, BUGSENG (https://bugseng.com)

Re: [XEN PATCH 12/13] xen: address violations of MISRA C:2012 Directive 4.10

[Stefano Stabellini]

On Wed, 6 Sep 2023, Jan Beulich wrote:
> On 06.09.2023 00:27, Stefano Stabellini wrote:
> > On Thu, 31 Aug 2023, Simone Ballarin wrote:
> >> On 29/08/23 00:51, Stefano Stabellini wrote:
> >>> On Mon, 28 Aug 2023, Simone Ballarin wrote:
> >>>> Move or amended inclusion guards to address violations of
> >>>> MISRA C:2012 Directive 4.10 ("Precautions shall be taken in order
> >>>> to prevent the contents of a header file being included more than
> >>>> once").
> >>>>
> >>>> Inclusion guards must appear at the beginning of the headers
> >>>> (comments are permitted anywhere) and the #if directive cannot
> >>>> be used for other checks.
> >>>>
> >>>> Mechanical change.
> >>>>
> >>>> Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>
> >>>> ---
> >>>>   xen/include/xen/err.h       | 4 +++-
> >>>>   xen/include/xen/pci_ids.h   | 5 +++++
> >>>>   xen/include/xen/softirq.h   | 4 +++-
> >>>>   xen/include/xen/unaligned.h | 7 ++++---
> >>>>   xen/include/xen/vmap.h      | 4 +++-
> >>>>   5 files changed, 18 insertions(+), 6 deletions(-)
> >>>>
> >>>> diff --git a/xen/include/xen/err.h b/xen/include/xen/err.h
> >>>> index 2f29b57d28..a6323d82d7 100644
> >>>> --- a/xen/include/xen/err.h
> >>>> +++ b/xen/include/xen/err.h
> >>>> @@ -1,5 +1,6 @@
> >>>> -#if !defined(__XEN_ERR_H__) && !defined(__ASSEMBLY__)
> >>>> +#if !defined(__XEN_ERR_H__)
> >>>>   #define __XEN_ERR_H__
> >>>> +#if !defined(__ASSEMBLY__)
> >>>
> >>> The original pattern was also guarding the header file sufficiently,
> >>> protecting it from double-inclusion. In fact, it is posing stricter
> >>> restrictions than usual (not laxer). This change is unnecessary?
> >>
> >> The MISRA directive asks to use one of the two following forms:
> >>
> >> <start-of-file>
> >> #if !defined ( identifier )
> >> #define identifier
> >> /* Contents of file */
> >> #endif
> >> <end-of-file>
> >>
> >> <start-of-file>
> >> #ifndef identifier
> >> #define identifier
> >> /* Contents of file */
> >> #endif
> >> <end-of-file>
> >>
> >> I do not see any reason for deviating, but if you ask that, I can do it.
> > 
> > Let's follow MISRA's form.
> 
> This is what I strongly dislike: They could be less restrictive on the
> exact patterns permitted without impacting the goal intended to be
> reached. But it's all as simple as possible, not as flexible as possible.
> 
> In any event, if a transformation like what can still be seen in context
> is to be made, then please #ifdef / #ifndef instead of defined(...)
> whenever possible.

In all fairness I dislike this too. However the rule is clear that to
make it easier to implement MISRA C checkers MISRA only supports 2
specific patterns. And I can see they have a point there in making it
easier to automatically check for correctness.

So I would go ahead with the change.