[PATCH 00/17] xen: address violation of MISRA C:2012 Directive 4.10

[PATCH 00/17] xen: address violation of MISRA C:2012 Directive 4.10

[Alessandro Zucchelli]

The Xen sources contain violations of MISRA C:2012 Directive 4.10 whose headline states:
"Precautions shall be taken in order to prevent the contents of a header file
being included more than once".

Following V2 and V3, here are all the rules that have been applied:
- private headers -> <dir>_<filename>_H
- asm-generic headers -> ASM_GENERIC_<filename>_H
- arch/<architecture>/include/asm/<subdir>/<filename>.h -> ASM_<architecture>_<subdir>_<filename>_H
- include/generated/<subdir>/<filename>.h-> GENERATED_<subdir>_<filename>_H
- arch/<architecture>/include/generated/asm/<filename>.h-> <arch>_GENERATED_ASM_<name>_H

Links to the discussions:
https://lists.xenproject.org/archives/html/xen-devel/2023-09/msg01928.html
https://lists.xenproject.org/archives/html/xen-devel/2023-10/msg01784.html
https://lists.xenproject.org/archives/html/xen-devel/2023-10/msg02073.html

Furthermore, the violations arising from the autogenerated header files
include/xen/compile.h and xen/hypercall-defs.h are addressed.

Patch 17/17 adds a new section for CODING_STYLE with the aforementioned naming
conventions. 
this is just a draft, please give us a feedback on whether something similar may
be appreciated. 

Changes in v4:
 add/amend inclusion guards to address violations of the Directive and the new naming convention.
 drop teh XEN_ prefix when needed, according to the feedback received.
 add inclusion guard naming convention section in CODING_STYLE

Changes in v3:
Add/amend inclusion guards to address violations of the Directive and the new naming convention.
Remove trailing underscores.
Modify creation rule for asm-offsets.h to conform to the new standard and to not generate conflicting
guards between architectures (which is a violation of the Directive).

Alessandro Zucchelli (2):
  xen/build: address violation of MISRA C Directive 4.10
  CODING_STYLE: Add a section on header guards naming conventions

Maria Celeste Cesario (3):
  xen/arm: address violations of MISRA C:2012 Directive 4.10
  xen: address violations of MISRA C:2012 Directive 4.10
  xen/x86: rename inclusion guards for consistency

Nicola Vetrini (2):
  xen: add deviations for MISRA C 2012 Dir D4.10
  xen: add SAF deviation for MISRA C Dir 4.10

Simone Ballarin (10):
  misra: add deviation for headers that explicitly avoid guards
  misra: modify deviations for empty and generated headers
  misra: add deviations for direct inclusion guards
  xen/arm: address violations of MISRA C:2012 Directive 4.10
  xen/x86: address violations of MISRA C:2012 Directive 4.10
  x86/EFI: 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: address violations of MISRA C:2012 Directive 4.10
  x86/asm: address violations of MISRA C:2012 Directive 4.10

 CODING_STYLE                                  | 19 +++++++++
 .../eclair_analysis/ECLAIR/deviations.ecl     | 12 +++---
 docs/misra/deviations.rst                     |  7 ++++
 docs/misra/safe.json                          | 40 +++++++++++++++++++
 xen/arch/arm/efi/efi-boot.h                   |  6 +++
 xen/arch/arm/efi/runtime.h                    |  1 +
 xen/arch/arm/include/asm/domain.h             |  6 +--
 xen/arch/arm/include/asm/efibind.h            |  5 +++
 xen/arch/arm/include/asm/event.h              |  6 +--
 xen/arch/arm/include/asm/grant_table.h        |  6 +--
 xen/arch/arm/include/asm/hypercall.h          |  1 +
 xen/arch/arm/include/asm/io.h                 |  6 +--
 xen/arch/arm/include/asm/irq.h                |  6 +--
 xen/arch/arm/include/asm/smp.h                |  6 +--
 xen/arch/arm/include/asm/spinlock.h           |  6 +--
 xen/arch/arm/include/asm/system.h             |  6 +--
 xen/arch/x86/Makefile                         |  9 +++--
 xen/arch/x86/cpu/cpu.h                        |  5 +++
 xen/arch/x86/efi/efi-boot.h                   |  7 ++++
 xen/arch/x86/efi/runtime.h                    |  5 +++
 xen/arch/x86/include/asm/compat.h             |  5 +++
 xen/arch/x86/include/asm/cpufeatures.h        |  5 +--
 xen/arch/x86/include/asm/domain.h             |  6 +--
 xen/arch/x86/include/asm/efibind.h            |  5 +++
 xen/arch/x86/include/asm/event.h              |  6 +--
 xen/arch/x86/include/asm/grant_table.h        |  6 +--
 xen/arch/x86/include/asm/hypercall.h          |  1 +
 xen/arch/x86/include/asm/io.h                 |  6 +--
 xen/arch/x86/include/asm/irq.h                |  6 +--
 xen/arch/x86/include/asm/smp.h                |  6 +--
 xen/arch/x86/include/asm/spinlock.h           |  6 +--
 xen/arch/x86/include/asm/system.h             |  6 +--
 xen/arch/x86/x86_64/mmconfig.h                |  5 +++
 xen/arch/x86/x86_emulate/private.h            |  5 +++
 xen/build.mk                                  | 13 ++++--
 xen/common/decompress.h                       |  5 +++
 xen/common/efi/efi.h                          |  5 +++
 xen/common/event_channel.h                    |  5 +++
 xen/include/Makefile                          | 18 +++++++--
 xen/include/public/arch-x86/cpufeatureset.h   |  1 +
 xen/include/public/arch-x86/xen.h             |  1 +
 xen/include/public/errno.h                    |  1 +
 xen/include/xen/err.h                         |  8 ++--
 xen/include/xen/pci_ids.h                     |  5 +++
 xen/include/xen/softirq.h                     |  8 ++--
 xen/include/xen/vmap.h                        |  8 ++--
 xen/scripts/Makefile.asm-generic              |  8 +++-
 47 files changed, 244 insertions(+), 81 deletions(-)

-- 
2.34.1

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

[Alessandro Zucchelli]

From: Simone Ballarin <simone.ballarin@bugseng.com>

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 deviation cooments for headers that avoid guards.

Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>
Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
Signed-off-by: Alessandro Zucchelli <alessandro.zucchelli@bugseng.com>

---
Changes in v4:
- rebased against current staging tree
Changes in v3:
- fix inconsistent deviation ID
- change comment-based deviation text
Changes in v2:
- use the format introduced with doc/misra/safe.json instead of
  a generic text-based deviation
---
 docs/misra/safe.json                        | 8 ++++++++
 xen/include/public/arch-x86/cpufeatureset.h | 1 +
 xen/include/public/errno.h                  | 1 +
 3 files changed, 10 insertions(+)

diff --git a/docs/misra/safe.json b/docs/misra/safe.json
index 3f18ef401c..b865caac73 100644
--- a/docs/misra/safe.json
+++ b/docs/misra/safe.json
@@ -68,6 +68,14 @@
         },
         {
             "id": "SAF-8-safe",
+            "analyser": {
+                "eclair": "MC3R1.D4.10"
+            },
+            "name": "Dir 4.10: headers that leave it up to the caller to include them correctly",
+            "text": "Headers that deliberatively avoid inclusion guards explicitly leaving responsibility to the caller are allowed."
+        },
+        {
+            "id": "SAF-9-safe",
             "analyser": {},
             "name": "Sentinel",
             "text": "Next ID to be used"
diff --git a/xen/include/public/arch-x86/cpufeatureset.h b/xen/include/public/arch-x86/cpufeatureset.h
index d9eba5e9a7..0d2adfdc3a 100644
--- a/xen/include/public/arch-x86/cpufeatureset.h
+++ b/xen/include/public/arch-x86/cpufeatureset.h
@@ -23,6 +23,7 @@
  * their XEN_CPUFEATURE() being appropriate in the included context.
  */
 
+/* SAF-8-safe omitted inclusion guard */
 #ifndef XEN_CPUFEATURE
 
 /*
diff --git a/xen/include/public/errno.h b/xen/include/public/errno.h
index 5a78a7607c..ccd5023c3a 100644
--- a/xen/include/public/errno.h
+++ b/xen/include/public/errno.h
@@ -17,6 +17,7 @@
  * will unilaterally #undef XEN_ERRNO().
  */
 
+/* SAF-8-safe omitted inclusion guard */
 #ifndef XEN_ERRNO
 
 /*
-- 
2.34.1

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

[Jan Beulich]

On 01.07.2024 13:10, Alessandro Zucchelli wrote:
> --- a/docs/misra/safe.json
> +++ b/docs/misra/safe.json
> @@ -68,6 +68,14 @@
>          },
>          {
>              "id": "SAF-8-safe",
> +            "analyser": {
> +                "eclair": "MC3R1.D4.10"
> +            },
> +            "name": "Dir 4.10: headers that leave it up to the caller to include them correctly",
> +            "text": "Headers that deliberatively avoid inclusion guards explicitly leaving responsibility to the caller are allowed."
> +        },
> +        {
> +            "id": "SAF-9-safe",
>              "analyser": {},
>              "name": "Sentinel",
>              "text": "Next ID to be used"

Patch 10 adds another entry here, targeted at headers which are intended to be
possible to include more than once. Both headers here also fit that criteria,
even if right now they aren't used that way (iirc). Do we really need two
SAF-* markers for effectively all the same kind of headers?

> --- a/xen/include/public/arch-x86/cpufeatureset.h
> +++ b/xen/include/public/arch-x86/cpufeatureset.h
> @@ -23,6 +23,7 @@
>   * their XEN_CPUFEATURE() being appropriate in the included context.
>   */
>  
> +/* SAF-8-safe omitted inclusion guard */
>  #ifndef XEN_CPUFEATURE
>  
>  /*
> diff --git a/xen/include/public/errno.h b/xen/include/public/errno.h
> index 5a78a7607c..ccd5023c3a 100644
> --- a/xen/include/public/errno.h
> +++ b/xen/include/public/errno.h
> @@ -17,6 +17,7 @@
>   * will unilaterally #undef XEN_ERRNO().
>   */
>  
> +/* SAF-8-safe omitted inclusion guard */
>  #ifndef XEN_ERRNO
>  
>  /*

Further both of these headers have guards (to cover the default case), so
"omitted" certainly isn't correct. Much like the "name" line in the SAF
entry also isn't quite correct, as in the common case it's not left to
the use sites.

Jan

[PATCH 02/17] misra: modify deviations for empty and generated headers

[Alessandro Zucchelli]

From: Simone Ballarin <simone.ballarin@bugseng.com>

This patch modifies deviations for Directive 4.10:
"Precautions shall be taken in order to prevent the contents of
a header file being included more than once"

This patch avoids the file-based deviation for empty headers, and
replaces it with a comment-based one using the format specified in
docs/misra/safe.json.

Generated headers are not generally safe against multi-inclusions,
whether a header is safe depends on the nature of the generated code
in the header. For that reason, this patch drops the deviation for
generated headers.

Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
Signed-off-by: Alessandro Zucchelli <alessandro.zucchelli@bugseng.com>

---
Changes in v4:
- rebased against current staging tree

Changes in v2:
- use the format introduced with doc/misra/safe.json instead of
  a file-based deviation for empty headers
- remove deviation for generated headers

The reason of moving the comment-based deviation in "runtime.h" is that
it should appear immediatly before the violation and, for files with no
tokens,	the location is the the last line of the file.
---
 automation/eclair_analysis/ECLAIR/deviations.ecl | 7 -------
 docs/misra/safe.json                             | 8 ++++++++
 xen/arch/arm/efi/runtime.h                       | 1 +
 xen/include/Makefile                             | 2 +-
 4 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl b/automation/eclair_analysis/ECLAIR/deviations.ecl
index 0af1cb93d1..1c39a9a16d 100644
--- a/automation/eclair_analysis/ECLAIR/deviations.ecl
+++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
@@ -72,13 +72,6 @@ they are not instances of commented-out code."
 -config=MC3R1.D4.3,reports+={deliberate, "any_area(any_loc(file(arm64_bitops))&&context(name(int_clear_mask16)))"}
 -doc_end
 
--doc_begin="This header file is autogenerated or empty, therefore it poses no
-risk if included more than once."
--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)))"}
--doc_end
-
 -doc_begin="Files that are intended to be included more than once do not need to
 conform to the directive."
 -config=MC3R1.D4.10,reports+={safe, "first_area(text(^/\\* This file is legitimately included multiple times\\. \\*/$, begin-4))"}
diff --git a/docs/misra/safe.json b/docs/misra/safe.json
index b865caac73..282013aa4e 100644
--- a/docs/misra/safe.json
+++ b/docs/misra/safe.json
@@ -76,6 +76,14 @@
         },
         {
             "id": "SAF-9-safe",
+            "analyser": {
+                "eclair": "MC3R1.D4.10"
+            },
+            "name": "Dir 4.10: empty headers",
+            "text": "Empty headers pose no risk if included more than once."
+        },
+        {
+            "id": "SAF-10-safe",
             "analyser": {},
             "name": "Sentinel",
             "text": "Next ID to be used"
diff --git a/xen/arch/arm/efi/runtime.h b/xen/arch/arm/efi/runtime.h
index 25afcebed1..4d2d40bf3c 100644
--- a/xen/arch/arm/efi/runtime.h
+++ b/xen/arch/arm/efi/runtime.h
@@ -1 +1,2 @@
 /* Placeholder for ARM-specific runtime include/declarations */
+/* SAF-9-safe empty header */
diff --git a/xen/include/Makefile b/xen/include/Makefile
index 2e61b50139..058b0a566b 100644
--- a/xen/include/Makefile
+++ b/xen/include/Makefile
@@ -53,7 +53,7 @@ cmd_compat_h = \
     mv -f $@.new $@
 
 quiet_cmd_stub_h = GEN     $@
-cmd_stub_h = echo '/* empty */' >$@
+cmd_stub_h = echo '/* SAF-9-safe empty header */' >$@
 
 quiet_cmd_compat_i = CPP     $@
 cmd_compat_i = $(CPP) $(filter-out -Wa$(comma)% -include %/include/xen/config.h,$(XEN_CFLAGS)) $(cppflags-y) -o $@ $<
-- 
2.34.1

[PATCH 03/17] misra: add deviations for direct inclusion guards

[Alessandro Zucchelli]

From: Simone Ballarin <simone.ballarin@bugseng.com>

Add deviation comments 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).

This patch adds deviation comments using the format specified
in docs/misra/safe.json for headers with just the direct
inclusion guard before the inclusion guard since they are
safe and not supposed to comply with the directive.

Note that with SAF-9-safe in place, failures to have proper guards later
in the header files will not be reported

Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>
Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
Signed-off-by: Alessandro Zucchelli <alessandro.zucchelli@bugseng.com>

---
Changes in v4:
- Added comment clarifying that no further checks will be performed
on a file that has a SAF-9-safe deviation against missing inclusion
guards.
- rebased against the current staging tree

Changes in v3:
- fix inconsistent deviation ID
The patch has been introduced in v2.
---
 docs/misra/safe.json                 | 8 ++++++++
 xen/arch/arm/include/asm/hypercall.h | 1 +
 xen/arch/x86/include/asm/hypercall.h | 1 +
 3 files changed, 10 insertions(+)

diff --git a/docs/misra/safe.json b/docs/misra/safe.json
index 282013aa4e..3e9ebff8fe 100644
--- a/docs/misra/safe.json
+++ b/docs/misra/safe.json
@@ -84,6 +84,14 @@
         },
         {
             "id": "SAF-10-safe",
+            "analyser": {
+                "eclair": "MC3R1.D4.10"
+            },
+            "name": "Dir 4.10: direct inclusion guard before",
+            "text": "Headers with just the direct inclusion guard before the inclusion guard are safe."
+        },
+        {
+            "id": "SAF-11-safe",
             "analyser": {},
             "name": "Sentinel",
             "text": "Next ID to be used"
diff --git a/xen/arch/arm/include/asm/hypercall.h b/xen/arch/arm/include/asm/hypercall.h
index ccd26c5184..07e231f8b5 100644
--- a/xen/arch/arm/include/asm/hypercall.h
+++ b/xen/arch/arm/include/asm/hypercall.h
@@ -1,3 +1,4 @@
+/* SAF-10-safe direct inclusion guard before */
 #ifndef __XEN_HYPERCALL_H__
 #error "asm/hypercall.h should not be included directly - include xen/hypercall.h instead"
 #endif
diff --git a/xen/arch/x86/include/asm/hypercall.h b/xen/arch/x86/include/asm/hypercall.h
index ec2edc771e..389fa62af2 100644
--- a/xen/arch/x86/include/asm/hypercall.h
+++ b/xen/arch/x86/include/asm/hypercall.h
@@ -2,6 +2,7 @@
  * asm-x86/hypercall.h
  */
 
+/* SAF-10-safe direct inclusion guard before */
 #ifndef __XEN_HYPERCALL_H__
 #error "asm/hypercall.h should not be included directly - include xen/hypercall.h instead"
 #endif
-- 
2.34.1

Re: [PATCH 03/17] misra: add deviations for direct inclusion guards

[Jan Beulich]

On 01.07.2024 13:10, Alessandro Zucchelli wrote:
> From: Simone Ballarin <simone.ballarin@bugseng.com>
> 
> Add deviation comments 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).
> 
> This patch adds deviation comments using the format specified
> in docs/misra/safe.json for headers with just the direct
> inclusion guard before the inclusion guard since they are
> safe and not supposed to comply with the directive.
> 
> Note that with SAF-9-safe in place, failures to have proper guards later
> in the header files will not be reported

Rebasing mistake, seeing that it's ...

> --- a/docs/misra/safe.json
> +++ b/docs/misra/safe.json
> @@ -84,6 +84,14 @@
>          },
>          {
>              "id": "SAF-10-safe",
> +            "analyser": {
> +                "eclair": "MC3R1.D4.10"
> +            },
> +            "name": "Dir 4.10: direct inclusion guard before",
> +            "text": "Headers with just the direct inclusion guard before the inclusion guard are safe."
> +        },
> +        {
> +            "id": "SAF-11-safe",
>              "analyser": {},
>              "name": "Sentinel",
>              "text": "Next ID to be used"

... SFA-10-safe that's being added and ...

> --- a/xen/arch/arm/include/asm/hypercall.h
> +++ b/xen/arch/arm/include/asm/hypercall.h
> @@ -1,3 +1,4 @@
> +/* SAF-10-safe direct inclusion guard before */
>  #ifndef __XEN_HYPERCALL_H__
>  #error "asm/hypercall.h should not be included directly - include xen/hypercall.h instead"
>  #endif
> --- a/xen/arch/x86/include/asm/hypercall.h
> +++ b/xen/arch/x86/include/asm/hypercall.h
> @@ -2,6 +2,7 @@
>   * asm-x86/hypercall.h
>   */
>  
> +/* SAF-10-safe direct inclusion guard before */
>  #ifndef __XEN_HYPERCALL_H__
>  #error "asm/hypercall.h should not be included directly - include xen/hypercall.h instead"
>  #endif

... used here?

Jan

Re: [PATCH 03/17] misra: add deviations for direct inclusion guards

[Stefano Stabellini]

On Mon, 1 Jul 2024, Jan Beulich wrote:
> On 01.07.2024 13:10, Alessandro Zucchelli wrote:
> > From: Simone Ballarin <simone.ballarin@bugseng.com>
> > 
> > Add deviation comments 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).
> > 
> > This patch adds deviation comments using the format specified
> > in docs/misra/safe.json for headers with just the direct
> > inclusion guard before the inclusion guard since they are
> > safe and not supposed to comply with the directive.
> > 
> > Note that with SAF-9-safe in place, failures to have proper guards later
> > in the header files will not be reported
> 
> Rebasing mistake, seeing that it's ...

Yes with "SAF-9-safe" changed to SAF-10-safe in the commit message:

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



> > --- a/docs/misra/safe.json
> > +++ b/docs/misra/safe.json
> > @@ -84,6 +84,14 @@
> >          },
> >          {
> >              "id": "SAF-10-safe",
> > +            "analyser": {
> > +                "eclair": "MC3R1.D4.10"
> > +            },
> > +            "name": "Dir 4.10: direct inclusion guard before",
> > +            "text": "Headers with just the direct inclusion guard before the inclusion guard are safe."
> > +        },
> > +        {
> > +            "id": "SAF-11-safe",
> >              "analyser": {},
> >              "name": "Sentinel",
> >              "text": "Next ID to be used"
> 
> ... SFA-10-safe that's being added and ...
> 
> > --- a/xen/arch/arm/include/asm/hypercall.h
> > +++ b/xen/arch/arm/include/asm/hypercall.h
> > @@ -1,3 +1,4 @@
> > +/* SAF-10-safe direct inclusion guard before */
> >  #ifndef __XEN_HYPERCALL_H__
> >  #error "asm/hypercall.h should not be included directly - include xen/hypercall.h instead"
> >  #endif
> > --- a/xen/arch/x86/include/asm/hypercall.h
> > +++ b/xen/arch/x86/include/asm/hypercall.h
> > @@ -2,6 +2,7 @@
> >   * asm-x86/hypercall.h
> >   */
> >  
> > +/* SAF-10-safe direct inclusion guard before */
> >  #ifndef __XEN_HYPERCALL_H__
> >  #error "asm/hypercall.h should not be included directly - include xen/hypercall.h instead"
> >  #endif
> 
> ... used here?
> 
> Jan
> 

[PATCH 04/17] xen/arm: address violations of MISRA C:2012 Directive 4.10

[Alessandro Zucchelli]

From: Simone Ballarin <simone.ballarin@bugseng.com>

Add inclusion guard 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>
Signed-off-by: Maria Celeste Cesario <maria.celeste.cesario@bugseng.com>
Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
Signed-off-by: Alessandro Zucchelli <alessandro.zucchelli@bugseng.com>

---
Changes in v4:
- Drop ARCH_ string from the guard, according to the feedback received
Changes in v3:
- remove trailing underscores
- change inclusion guard name to adhere to the new standard
Changes in v2:
- drop changes in xen/arch/arm/include/asm/hypercall.h
- drop changes in xen/arch/arm/include/asm/iocap.h since they are
  not related with the directive
---
 xen/arch/arm/efi/efi-boot.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
index 199f526022..947166905f 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>
@@ -992,6 +996,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
-- 
2.34.1

Re: [PATCH 04/17] xen/arm: address violations of MISRA C:2012 Directive 4.10

[Stefano Stabellini]

On Mon, 1 Jul 2024, Alessandro Zucchelli wrote:
> From: Simone Ballarin <simone.ballarin@bugseng.com>
> 
> Add inclusion guard 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>
> Signed-off-by: Maria Celeste Cesario <maria.celeste.cesario@bugseng.com>
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> Signed-off-by: Alessandro Zucchelli <alessandro.zucchelli@bugseng.com>

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

[PATCH 05/17] xen/x86: address violations of MISRA C:2012 Directive 4.10

[Alessandro Zucchelli]

From: Simone Ballarin <simone.ballarin@bugseng.com>

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

Note that in x86_64/mmconfig.h we slightly deviate from the naming
convention in place: instead of having the inclusion guard as
X86_X86_64_MMCONFIG_H we shortened the directory prefix as X86_64 for
the sake of readability.

Mechanical change.

Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>
Signed-off-by: Maria Celeste Cesario <maria.celeste.cesario@bugseng.com>
Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
Signed-off-by: Alessandro Zucchelli <alessandro.zucchelli@bugseng.com>

---
Changes in v4:
- modified inclusion guards and makefile.
Changes in v3:
- remove trailing underscores
- change inclusion guard name to adhere to the new standard
Changes in v2:
- remove extra blanks
- drop changes in C files

Note:
Changes in Makefile were not strictly necessary in v1 and a maintainer
asked to removing them since there was a deviation for generated headers.
Now, in v2, they are required since the deviation has been removed by
another patch of this series.
---
 xen/arch/x86/Makefile              | 9 +++++----
 xen/arch/x86/cpu/cpu.h             | 5 +++++
 xen/arch/x86/x86_64/mmconfig.h     | 5 +++++
 xen/arch/x86/x86_emulate/private.h | 5 +++++
 4 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
index d902fb7acc..06d1bab43c 100644
--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -260,17 +260,18 @@ $(objtree)/arch/x86/include/asm/asm-macros.h: $(obj)/asm-macros.i $(src)/Makefil
 	$(call filechk,asm-macros.h)
 
 define filechk_asm-macros.h
+	guard=$$(echo ASM_${SRCARCH}_ASM_MACROS_H | tr a-z A-Z);  \
+    echo '#ifndef $$guard'; \
+    echo '#define $$guard'; \
     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 /* $$guard */'
 endef
 
 $(obj)/efi.lds: AFLAGS-y += -DEFI
diff --git a/xen/arch/x86/cpu/cpu.h b/xen/arch/x86/cpu/cpu.h
index 8be65e975a..ee1c176ca4 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);
@@ -26,3 +29,5 @@ void amd_init_spectral_chicken(void);
 void detect_zen2_null_seg_behaviour(void);
 
 void intel_unlock_cpuid_leaves(struct cpuinfo_x86 *c);
+
+#endif /* X86_CPU_CPU_H */
diff --git a/xen/arch/x86/x86_64/mmconfig.h b/xen/arch/x86/x86_64/mmconfig.h
index 3da4b21e9b..722bf67975 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_64_MMCONFIG_H
+#define 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 idx);
 void pci_mmcfg_arch_disable(unsigned int idx);
+
+#endif /* X86_64_MMCONFIG_H */
diff --git a/xen/arch/x86/x86_emulate/private.h b/xen/arch/x86/x86_emulate/private.h
index 0fa26ba00a..8429b30b5e 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/bug.h>
@@ -836,3 +839,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 */
-- 
2.34.1

Re: [PATCH 05/17] xen/x86: address violations of MISRA C:2012 Directive 4.10

[Jan Beulich]

On 01.07.2024 15:36, Alessandro Zucchelli wrote:
> --- a/xen/arch/x86/Makefile
> +++ b/xen/arch/x86/Makefile
> @@ -260,17 +260,18 @@ $(objtree)/arch/x86/include/asm/asm-macros.h: $(obj)/asm-macros.i $(src)/Makefil
>  	$(call filechk,asm-macros.h)
>  
>  define filechk_asm-macros.h
> +	guard=$$(echo ASM_${SRCARCH}_ASM_MACROS_H | tr a-z A-Z);  \

Nit: Hard tab slipped in.

> +    echo '#ifndef $$guard'; \
> +    echo '#define $$guard'; \
>      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 /* $$guard */'
>  endef
>  
>  $(obj)/efi.lds: AFLAGS-y += -DEFI
> --- 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

This, ...

> --- 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_64_MMCONFIG_H
> +#define X86_64_MMCONFIG_H

... this, and ...

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

... this guard can't possibly all follow the same proposed naming scheme
(wherever the final version of that is being recorded; I don't recall it
having gone in, and I didn't spot anything earlier in the series); at
least one must be wrong.

Jan

Re: [PATCH 05/17] xen/x86: address violations of MISRA C:2012 Directive 4.10

[Alessandro Zucchelli]

On 2024-07-01 16:21, Jan Beulich wrote:
> On 01.07.2024 15:36, Alessandro Zucchelli wrote:
>> --- a/xen/arch/x86/Makefile
>> +++ b/xen/arch/x86/Makefile
>> @@ -260,17 +260,18 @@ $(objtree)/arch/x86/include/asm/asm-macros.h: 
>> $(obj)/asm-macros.i $(src)/Makefil
>>  	$(call filechk,asm-macros.h)
>> 
>>  define filechk_asm-macros.h
>> +	guard=$$(echo ASM_${SRCARCH}_ASM_MACROS_H | tr a-z A-Z);  \
> 
> Nit: Hard tab slipped in.
> 
>> +    echo '#ifndef $$guard'; \
>> +    echo '#define $$guard'; \
>>      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 /* $$guard */'
>>  endef
>> 
>>  $(obj)/efi.lds: AFLAGS-y += -DEFI
>> --- 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
> 
> This, ...
> 
>> --- 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_64_MMCONFIG_H
>> +#define X86_64_MMCONFIG_H
> 
> ... this, and ...
> 
>> --- 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
> 
> ... this guard can't possibly all follow the same proposed naming 
> scheme
> (wherever the final version of that is being recorded; I don't recall 
> it
> having gone in, and I didn't spot anything earlier in the series); at
> least one must be wrong.

For x86/x86_64/mmconfig.h has been made an exception as stated in the 
commit
message:
Note that in x86_64/mmconfig.h we slightly deviate from the naming
convention in place: instead of having the inclusion guard as
X86_X86_64_MMCONFIG_H we shortened the directory prefix as X86_64 for
the sake of readability.

If you do not agree with this exception and you prefer to keep the 
additional
X86_ prefix let me know so as I prepare the patch series V5 I may 
reintroduce it.

Best regards,
-- 
Alessandro Zucchelli, B.Sc.

Software Engineer, BUGSENG (https://bugseng.com)

Re: [PATCH 05/17] xen/x86: address violations of MISRA C:2012 Directive 4.10

[Jan Beulich]

On 09.07.2024 09:38, Alessandro Zucchelli wrote:
> On 2024-07-01 16:21, Jan Beulich wrote:
>> On 01.07.2024 15:36, Alessandro Zucchelli wrote:
>>> --- a/xen/arch/x86/Makefile
>>> +++ b/xen/arch/x86/Makefile
>>> @@ -260,17 +260,18 @@ $(objtree)/arch/x86/include/asm/asm-macros.h: 
>>> $(obj)/asm-macros.i $(src)/Makefil
>>>  	$(call filechk,asm-macros.h)
>>>
>>>  define filechk_asm-macros.h
>>> +	guard=$$(echo ASM_${SRCARCH}_ASM_MACROS_H | tr a-z A-Z);  \
>>
>> Nit: Hard tab slipped in.
>>
>>> +    echo '#ifndef $$guard'; \
>>> +    echo '#define $$guard'; \
>>>      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 /* $$guard */'
>>>  endef
>>>
>>>  $(obj)/efi.lds: AFLAGS-y += -DEFI
>>> --- 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
>>
>> This, ...
>>
>>> --- 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_64_MMCONFIG_H
>>> +#define X86_64_MMCONFIG_H
>>
>> ... this, and ...
>>
>>> --- 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
>>
>> ... this guard can't possibly all follow the same proposed naming 
>> scheme
>> (wherever the final version of that is being recorded; I don't recall 
>> it
>> having gone in, and I didn't spot anything earlier in the series); at
>> least one must be wrong.
> 
> For x86/x86_64/mmconfig.h has been made an exception as stated in the 
> commit
> message:
> Note that in x86_64/mmconfig.h we slightly deviate from the naming
> convention in place: instead of having the inclusion guard as
> X86_X86_64_MMCONFIG_H we shortened the directory prefix as X86_64 for
> the sake of readability.
> 
> If you do not agree with this exception and you prefer to keep the 
> additional
> X86_ prefix let me know so as I prepare the patch series V5 I may 
> reintroduce it.

What I have an issue with is making an exception in one place when quite
clearly would as much (or as little) benefit from one. Before there's
any further back and forth, I'd like to suggest that you wait with
adjustments here until the base scheme has really been agreed upon,
including situations where we think we'd like to make exceptions (after
all we might decide that there simply shouldn't be exceptions, and that
in order to eliminate such redundancy in guard identifiers we'd rather
like to rename some of the files).

Jan

Re: [PATCH 05/17] xen/x86: address violations of MISRA C:2012 Directive 4.10

[Stefano Stabellini]

On Tue, 9 Jul 2024, Jan Beulich wrote:
> On 09.07.2024 09:38, Alessandro Zucchelli wrote:
> > On 2024-07-01 16:21, Jan Beulich wrote:
> >> On 01.07.2024 15:36, Alessandro Zucchelli wrote:
> >>> --- a/xen/arch/x86/Makefile
> >>> +++ b/xen/arch/x86/Makefile
> >>> @@ -260,17 +260,18 @@ $(objtree)/arch/x86/include/asm/asm-macros.h: 
> >>> $(obj)/asm-macros.i $(src)/Makefil
> >>>  	$(call filechk,asm-macros.h)
> >>>
> >>>  define filechk_asm-macros.h
> >>> +	guard=$$(echo ASM_${SRCARCH}_ASM_MACROS_H | tr a-z A-Z);  \
> >>
> >> Nit: Hard tab slipped in.
> >>
> >>> +    echo '#ifndef $$guard'; \
> >>> +    echo '#define $$guard'; \
> >>>      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 /* $$guard */'
> >>>  endef
> >>>
> >>>  $(obj)/efi.lds: AFLAGS-y += -DEFI
> >>> --- 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
> >>
> >> This, ...
> >>
> >>> --- 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_64_MMCONFIG_H
> >>> +#define X86_64_MMCONFIG_H
> >>
> >> ... this, and ...
> >>
> >>> --- 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
> >>
> >> ... this guard can't possibly all follow the same proposed naming 
> >> scheme
> >> (wherever the final version of that is being recorded; I don't recall 
> >> it
> >> having gone in, and I didn't spot anything earlier in the series); at
> >> least one must be wrong.
> > 
> > For x86/x86_64/mmconfig.h has been made an exception as stated in the 
> > commit
> > message:
> > Note that in x86_64/mmconfig.h we slightly deviate from the naming
> > convention in place: instead of having the inclusion guard as
> > X86_X86_64_MMCONFIG_H we shortened the directory prefix as X86_64 for
> > the sake of readability.
> > 
> > If you do not agree with this exception and you prefer to keep the 
> > additional
> > X86_ prefix let me know so as I prepare the patch series V5 I may 
> > reintroduce it.
> 
> What I have an issue with is making an exception in one place when quite
> clearly would as much (or as little) benefit from one. Before there's
> any further back and forth, I'd like to suggest that you wait with
> adjustments here until the base scheme has really been agreed upon,
> including situations where we think we'd like to make exceptions (after
> all we might decide that there simply shouldn't be exceptions, and that
> in order to eliminate such redundancy in guard identifiers we'd rather
> like to rename some of the files).

I don't think it is a good idea to introduce a naming scheme and
immediately add exceptions. I would stick to the naming scheme even if
it doesn't always lead to the best possible name. Also these are header
guards, they are not critical variables for which clarity and shortness
are paramount.

The naming scheme was written in the 0 email and copy/pasted here for
convenience:

- private headers -> <dir>_<filename>_H
- asm-generic headers -> ASM_GENERIC_<filename>_H
- arch/<architecture>/include/asm/<subdir>/<filename>.h -> ASM_<architecture>_<subdir>_<filename>_H
- include/generated/<subdir>/<filename>.h-> GENERATED_<subdir>_<filename>_H
- arch/<architecture>/include/generated/asm/<filename>.h-> <arch>_GENERATED_ASM_<name>_H

So in this patch I would sticked to the naming scheme and used:

X86_CPU_CPU_H
X86_X86_64_MMCONFIG_H
X86_X86_EMULATE_PRIVATE_H

Re: [PATCH 05/17] xen/x86: address violations of MISRA C:2012 Directive 4.10

[Jan Beulich]

On 13.07.2024 00:09, Stefano Stabellini wrote:
> On Tue, 9 Jul 2024, Jan Beulich wrote:
>> On 09.07.2024 09:38, Alessandro Zucchelli wrote:
>>> On 2024-07-01 16:21, Jan Beulich wrote:
>>>> On 01.07.2024 15:36, Alessandro Zucchelli wrote:
>>>>> --- a/xen/arch/x86/Makefile
>>>>> +++ b/xen/arch/x86/Makefile
>>>>> @@ -260,17 +260,18 @@ $(objtree)/arch/x86/include/asm/asm-macros.h: 
>>>>> $(obj)/asm-macros.i $(src)/Makefil
>>>>>  	$(call filechk,asm-macros.h)
>>>>>
>>>>>  define filechk_asm-macros.h
>>>>> +	guard=$$(echo ASM_${SRCARCH}_ASM_MACROS_H | tr a-z A-Z);  \
>>>>
>>>> Nit: Hard tab slipped in.
>>>>
>>>>> +    echo '#ifndef $$guard'; \
>>>>> +    echo '#define $$guard'; \
>>>>>      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 /* $$guard */'
>>>>>  endef
>>>>>
>>>>>  $(obj)/efi.lds: AFLAGS-y += -DEFI
>>>>> --- 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
>>>>
>>>> This, ...
>>>>
>>>>> --- 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_64_MMCONFIG_H
>>>>> +#define X86_64_MMCONFIG_H
>>>>
>>>> ... this, and ...
>>>>
>>>>> --- 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
>>>>
>>>> ... this guard can't possibly all follow the same proposed naming 
>>>> scheme
>>>> (wherever the final version of that is being recorded; I don't recall 
>>>> it
>>>> having gone in, and I didn't spot anything earlier in the series); at
>>>> least one must be wrong.
>>>
>>> For x86/x86_64/mmconfig.h has been made an exception as stated in the 
>>> commit
>>> message:
>>> Note that in x86_64/mmconfig.h we slightly deviate from the naming
>>> convention in place: instead of having the inclusion guard as
>>> X86_X86_64_MMCONFIG_H we shortened the directory prefix as X86_64 for
>>> the sake of readability.
>>>
>>> If you do not agree with this exception and you prefer to keep the 
>>> additional
>>> X86_ prefix let me know so as I prepare the patch series V5 I may 
>>> reintroduce it.
>>
>> What I have an issue with is making an exception in one place when quite
>> clearly would as much (or as little) benefit from one. Before there's
>> any further back and forth, I'd like to suggest that you wait with
>> adjustments here until the base scheme has really been agreed upon,
>> including situations where we think we'd like to make exceptions (after
>> all we might decide that there simply shouldn't be exceptions, and that
>> in order to eliminate such redundancy in guard identifiers we'd rather
>> like to rename some of the files).
> 
> I don't think it is a good idea to introduce a naming scheme and
> immediately add exceptions. I would stick to the naming scheme even if
> it doesn't always lead to the best possible name. Also these are header
> guards, they are not critical variables for which clarity and shortness
> are paramount.
> 
> The naming scheme was written in the 0 email and copy/pasted here for
> convenience:
> 
> - private headers -> <dir>_<filename>_H
> - asm-generic headers -> ASM_GENERIC_<filename>_H
> - arch/<architecture>/include/asm/<subdir>/<filename>.h -> ASM_<architecture>_<subdir>_<filename>_H
> - include/generated/<subdir>/<filename>.h-> GENERATED_<subdir>_<filename>_H
> - arch/<architecture>/include/generated/asm/<filename>.h-> <arch>_GENERATED_ASM_<name>_H
> 
> So in this patch I would sticked to the naming scheme and used:
> 
> X86_CPU_CPU_H
> X86_X86_64_MMCONFIG_H
> X86_X86_EMULATE_PRIVATE_H

Uniformly sticking to the scheme is definitely an option. Provided we're
happy with excessively long identifiers (think of, sooner or later, there
going to be style violations wrt line length) and ones having overly
redundant name components. Yet as said elsewhere, first and foremost I
think we need a scheme where collisions cannot occur.

Jan

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

[Alessandro Zucchelli]

From: Simone Ballarin <simone.ballarin@bugseng.com>

Add inclusion guard 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>
Signed-off-by: Maria Celeste Cesario <maria.celeste.cesario@bugseng.com>
Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
Signed-off-by: Alessandro Zucchelli <alessandro.zucchelli@bugseng.com>

---
Changes in v4:
- Modified inclusion guard.
Changes in v3:
- remove trailing underscores
- change inclusion guard name to adhere to the new standard
Changes in v2:
- remove changes in "xen/arch/x86/efi/efi-boot.h"

Note:
Changes in efi-boot.h have been removed since the file is
intenteded to be included by common/efi/boot.c only. This motivation
is not enough to raise a deviation record, so the violation is
still present.
---
 xen/arch/x86/efi/efi-boot.h | 7 +++++++
 xen/arch/x86/efi/runtime.h  | 5 +++++
 2 files changed, 12 insertions(+)

diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
index f282358435..c6be744f2b 100644
--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -3,6 +3,11 @@
  * 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>
@@ -912,6 +917,8 @@ void asmlinkage __init efi_multiboot2(EFI_HANDLE ImageHandle,
     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..88ab5651e9 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: [PATCH 06/17] x86/EFI: address violations of MISRA C:2012 Directive 4.10

[Marek Marczykowski-Górecki]

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

On Mon, Jul 01, 2024 at 03:36:01PM +0200, Alessandro Zucchelli wrote:
> From: Simone Ballarin <simone.ballarin@bugseng.com>
> 
> Add inclusion guard 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>
> Signed-off-by: Maria Celeste Cesario <maria.celeste.cesario@bugseng.com>
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> Signed-off-by: Alessandro Zucchelli <alessandro.zucchelli@bugseng.com>
> 
> ---
> Changes in v4:
> - Modified inclusion guard.
> Changes in v3:
> - remove trailing underscores
> - change inclusion guard name to adhere to the new standard
> Changes in v2:
> - remove changes in "xen/arch/x86/efi/efi-boot.h"
> 
> Note:
> Changes in efi-boot.h have been removed since the file is
> intenteded to be included by common/efi/boot.c only. This motivation
> is not enough to raise a deviation record, so the violation is
> still present.

I'm confused by this comment. It says changes in efi-boot.h have been
removed, yet the patch does include them.

> ---
>  xen/arch/x86/efi/efi-boot.h | 7 +++++++
>  xen/arch/x86/efi/runtime.h  | 5 +++++
>  2 files changed, 12 insertions(+)
> 
> diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
> index f282358435..c6be744f2b 100644
> --- a/xen/arch/x86/efi/efi-boot.h
> +++ b/xen/arch/x86/efi/efi-boot.h
> @@ -3,6 +3,11 @@
>   * 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>
> @@ -912,6 +917,8 @@ void asmlinkage __init efi_multiboot2(EFI_HANDLE ImageHandle,
>      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..88ab5651e9 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
> 

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Alessandro Zucchelli]

On 2024-07-01 16:09, Marek Marczykowski-Górecki wrote:
> On Mon, Jul 01, 2024 at 03:36:01PM +0200, Alessandro Zucchelli wrote:
>> From: Simone Ballarin <simone.ballarin@bugseng.com>
>> 
>> Add inclusion guard 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>
>> Signed-off-by: Maria Celeste Cesario 
>> <maria.celeste.cesario@bugseng.com>
>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>> Signed-off-by: Alessandro Zucchelli <alessandro.zucchelli@bugseng.com>
>> 
>> ---
>> Changes in v4:
>> - Modified inclusion guard.
>> Changes in v3:
>> - remove trailing underscores
>> - change inclusion guard name to adhere to the new standard
>> Changes in v2:
>> - remove changes in "xen/arch/x86/efi/efi-boot.h"
>> 
>> Note:
>> Changes in efi-boot.h have been removed since the file is
>> intenteded to be included by common/efi/boot.c only. This motivation
>> is not enough to raise a deviation record, so the violation is
>> still present.
> 
> I'm confused by this comment. It says changes in efi-boot.h have been
> removed, yet the patch does include them.

The note referes to the "Changes in v2".
You can ignore it.

> 
>> ---
>>  xen/arch/x86/efi/efi-boot.h | 7 +++++++
>>  xen/arch/x86/efi/runtime.h  | 5 +++++
>>  2 files changed, 12 insertions(+)
>> 
>> diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
>> index f282358435..c6be744f2b 100644
>> --- a/xen/arch/x86/efi/efi-boot.h
>> +++ b/xen/arch/x86/efi/efi-boot.h
>> @@ -3,6 +3,11 @@
>>   * 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>
>> @@ -912,6 +917,8 @@ void asmlinkage __init efi_multiboot2(EFI_HANDLE 
>> ImageHandle,
>>      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..88ab5651e9 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
>> 

-- 
Alessandro Zucchelli, B.Sc.

Software Engineer, BUGSENG (https://bugseng.com)

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

[Jan Beulich]

On 01.07.2024 15:36, Alessandro Zucchelli wrote:
> --- a/xen/arch/x86/efi/efi-boot.h
> +++ b/xen/arch/x86/efi/efi-boot.h
> @@ -3,6 +3,11 @@
>   * 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>

Nit: No double blank lines please, anywhere.

Jan

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

[Stefano Stabellini]

On Mon, 1 Jul 2024, Jan Beulich wrote:
> On 01.07.2024 15:36, Alessandro Zucchelli wrote:
> > --- a/xen/arch/x86/efi/efi-boot.h
> > +++ b/xen/arch/x86/efi/efi-boot.h
> > @@ -3,6 +3,11 @@
> >   * 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>
> 
> Nit: No double blank lines please, anywhere.

With this addressed:

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

[PATCH 07/17] xen/common: address violations of MISRA C:2012 Directive 4.10

[Alessandro Zucchelli]

From: Simone Ballarin <simone.ballarin@bugseng.com>

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>
Signed-off-by: Maria Celeste Cesario  <maria.celeste.cesario@bugseng.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
Signed-off-by: Alessandro Zucchelli <alessandro.zucchelli@bugseng.com>

---
Changes in v4:
- drop XEN_ prefix from inclusion guard, according to the feedback received
Changes in v3:
- remove trailing underscores
- change inclusion guard name to adhere to the new standard
Changes in v2:
- drop changes in C files
---
 xen/common/decompress.h    | 5 +++++
 xen/common/event_channel.h | 5 +++++
 2 files changed, 10 insertions(+)

diff --git a/xen/common/decompress.h b/xen/common/decompress.h
index e8195b353a..9bf20e6a2d 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..502ac39a48 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
-- 
2.34.1

[PATCH 08/17] xen/efi: address violations of MISRA C:2012 Directive 4.10

[Alessandro Zucchelli]

From: Simone Ballarin <simone.ballarin@bugseng.com>

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>
Signed-off-by: Maria Celeste Cesario  <maria.celeste.cesario@bugseng.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
Signed-off-by: Alessandro Zucchelli <alessandro.zucchelli@bugseng.com>

---
Changes in v4:
- drop XEN_ prefix from inclusion guard, according to the feedback received
Changes in v3:
- remove trailing underscores
- change inclusion guard name to adhere to the new standard
Changes in v2:
- drop changes in C files
---
 xen/common/efi/efi.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/xen/common/efi/efi.h b/xen/common/efi/efi.h
index c02fbb7b69..e5eae8f78a 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 */
-- 
2.34.1

[PATCH 09/17] xen: address violations of MISRA C:2012 Directive 4.10

[Alessandro Zucchelli]

From: Simone Ballarin <simone.ballarin@bugseng.com>

Amend 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>
Signed-off-by: Maria Celeste Cesario <maria.celeste.cesario@bugseng.com>
Acked-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
Signed-off-by: Alessandro Zucchelli <alessandro.zucchelli@bugseng.com>

---
Changes in v4:
- drop XEN_ prefix from inclusion guard, according to the feedback received
Changes in v3:
- remove trailing underscores
- change inclusion guard name to adhere to the new standard
Changes in v2:
- drop changes in xen/include/xen/unaligned.h since this second
  series adds a comment-based deviation in a separate patch
- use #ifndef instead of #if !defined()
---
 xen/include/xen/err.h     | 8 +++++---
 xen/include/xen/pci_ids.h | 5 +++++
 xen/include/xen/softirq.h | 8 +++++---
 xen/include/xen/vmap.h    | 8 +++++---
 4 files changed, 20 insertions(+), 9 deletions(-)

diff --git a/xen/include/xen/err.h b/xen/include/xen/err.h
index cbdd1bf7f8..65549caac2 100644
--- a/xen/include/xen/err.h
+++ b/xen/include/xen/err.h
@@ -1,5 +1,6 @@
-#if !defined(__XEN_ERR_H__) && !defined(__ASSEMBLY__)
-#define __XEN_ERR_H__
+#ifndef INCLUDE_XEN_ERR_H
+#define INCLUDE_XEN_ERR_H
+#ifndef __ASSEMBLY__
 
 #include <xen/compiler.h>
 #include <xen/errno.h>
@@ -41,4 +42,5 @@ static inline int __must_check PTR_RET(const void *ptr)
 	return IS_ERR(ptr) ? PTR_ERR(ptr) : 0;
 }
 
-#endif /* __XEN_ERR_H__ */
+#endif /* __ASSEMBLY__ */
+#endif /* INCLUDE_XEN_ERR_H */
diff --git a/xen/include/xen/pci_ids.h b/xen/include/xen/pci_ids.h
index e798477a7e..8e40c78db7 100644
--- a/xen/include/xen/pci_ids.h
+++ b/xen/include/xen/pci_ids.h
@@ -1,3 +1,6 @@
+#ifndef INCLUDE_XEN_PCI_IDS_H
+#define INCLUDE_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 /* INCLUDE_XEN_PCI_IDS_H */
diff --git a/xen/include/xen/softirq.h b/xen/include/xen/softirq.h
index 33d6f2ecd2..90d4875df7 100644
--- a/xen/include/xen/softirq.h
+++ b/xen/include/xen/softirq.h
@@ -1,5 +1,6 @@
-#if !defined(__XEN_SOFTIRQ_H__) && !defined(__ASSEMBLY__)
-#define __XEN_SOFTIRQ_H__
+#ifndef INCLUDE_XEN_SOFTIRQ_H
+#define INCLUDE_XEN_SOFTIRQ_H
+#ifndef __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 /* __XEN_SOFTIRQ_H__ */
+#endif /* __ASSEMBLY__ */
+#endif /* INCLUDE_XEN_SOFTIRQ_H */
diff --git a/xen/include/xen/vmap.h b/xen/include/xen/vmap.h
index 0c16baa85f..ec1b6b05e9 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)
-#define __XEN_VMAP_H__
+#ifndef INCLUDE_XEN_VMAP_H
+#define INCLUDE_XEN_VMAP_H
+#ifdef VMAP_VIRT_START
 
 #include <xen/mm-frame.h>
 #include <xen/page-size.h>
@@ -42,4 +43,5 @@ static inline void vm_init(void)
     vm_init_type(VMAP_DEFAULT, (void *)VMAP_VIRT_START, arch_vmap_virt_end());
 }
 
-#endif /* __XEN_VMAP_H__ */
+#endif /* VMAP_VIRT_START */
+#endif /* INCLUDE_XEN_VMAP_H */
-- 
2.34.1

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

[Jan Beulich]

On 01.07.2024 15:36, Alessandro Zucchelli wrote:
> --- a/xen/include/xen/err.h
> +++ b/xen/include/xen/err.h
> @@ -1,5 +1,6 @@
> -#if !defined(__XEN_ERR_H__) && !defined(__ASSEMBLY__)
> -#define __XEN_ERR_H__
> +#ifndef INCLUDE_XEN_ERR_H
> +#define INCLUDE_XEN_ERR_H

There once was a document (or was it a patch description) describing the
naming system for these guards. Where did that go? With include files
typically living under include/, seeing INCLUDE_ as a prefix is, well,
odd and unnecessary baggage. I also don't recall there having been
agreement to use names like the ones presented here.

Jan

> +#ifndef __ASSEMBLY__
>  
>  #include <xen/compiler.h>
>  #include <xen/errno.h>
> @@ -41,4 +42,5 @@ static inline int __must_check PTR_RET(const void *ptr)
>  	return IS_ERR(ptr) ? PTR_ERR(ptr) : 0;
>  }
>  
> -#endif /* __XEN_ERR_H__ */
> +#endif /* __ASSEMBLY__ */
> +#endif /* INCLUDE_XEN_ERR_H */
> diff --git a/xen/include/xen/pci_ids.h b/xen/include/xen/pci_ids.h
> index e798477a7e..8e40c78db7 100644
> --- a/xen/include/xen/pci_ids.h
> +++ b/xen/include/xen/pci_ids.h
> @@ -1,3 +1,6 @@
> +#ifndef INCLUDE_XEN_PCI_IDS_H
> +#define INCLUDE_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 /* INCLUDE_XEN_PCI_IDS_H */
> diff --git a/xen/include/xen/softirq.h b/xen/include/xen/softirq.h
> index 33d6f2ecd2..90d4875df7 100644
> --- a/xen/include/xen/softirq.h
> +++ b/xen/include/xen/softirq.h
> @@ -1,5 +1,6 @@
> -#if !defined(__XEN_SOFTIRQ_H__) && !defined(__ASSEMBLY__)
> -#define __XEN_SOFTIRQ_H__
> +#ifndef INCLUDE_XEN_SOFTIRQ_H
> +#define INCLUDE_XEN_SOFTIRQ_H
> +#ifndef __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 /* __XEN_SOFTIRQ_H__ */
> +#endif /* __ASSEMBLY__ */
> +#endif /* INCLUDE_XEN_SOFTIRQ_H */
> diff --git a/xen/include/xen/vmap.h b/xen/include/xen/vmap.h
> index 0c16baa85f..ec1b6b05e9 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)
> -#define __XEN_VMAP_H__
> +#ifndef INCLUDE_XEN_VMAP_H
> +#define INCLUDE_XEN_VMAP_H
> +#ifdef VMAP_VIRT_START
>  
>  #include <xen/mm-frame.h>
>  #include <xen/page-size.h>
> @@ -42,4 +43,5 @@ static inline void vm_init(void)
>      vm_init_type(VMAP_DEFAULT, (void *)VMAP_VIRT_START, arch_vmap_virt_end());
>  }
>  
> -#endif /* __XEN_VMAP_H__ */
> +#endif /* VMAP_VIRT_START */
> +#endif /* INCLUDE_XEN_VMAP_H */

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

[Stefano Stabellini]

On Wed, 3 Jul 2024, Jan Beulich wrote:
> On 01.07.2024 15:36, Alessandro Zucchelli wrote:
> > --- a/xen/include/xen/err.h
> > +++ b/xen/include/xen/err.h
> > @@ -1,5 +1,6 @@
> > -#if !defined(__XEN_ERR_H__) && !defined(__ASSEMBLY__)
> > -#define __XEN_ERR_H__
> > +#ifndef INCLUDE_XEN_ERR_H
> > +#define INCLUDE_XEN_ERR_H
> 
> There once was a document (or was it a patch description) describing the
> naming system for these guards. Where did that go? With include files
> typically living under include/, seeing INCLUDE_ as a prefix is, well,
> odd and unnecessary baggage. I also don't recall there having been
> agreement to use names like the ones presented here.

I agree with Jan that INCLUDE_ is unnecessary and looking at patch #0
and our previous discussions it would seem that this case was not
covered.

I think we should just use XEN_ERR_H

The rule could be (to be added to the others in patch #0):
- xen/include/xen -> XEN_<filename>_H


> > +#ifndef __ASSEMBLY__
> >  
> >  #include <xen/compiler.h>
> >  #include <xen/errno.h>
> > @@ -41,4 +42,5 @@ static inline int __must_check PTR_RET(const void *ptr)
> >  	return IS_ERR(ptr) ? PTR_ERR(ptr) : 0;
> >  }
> >  
> > -#endif /* __XEN_ERR_H__ */
> > +#endif /* __ASSEMBLY__ */
> > +#endif /* INCLUDE_XEN_ERR_H */
> > diff --git a/xen/include/xen/pci_ids.h b/xen/include/xen/pci_ids.h
> > index e798477a7e..8e40c78db7 100644
> > --- a/xen/include/xen/pci_ids.h
> > +++ b/xen/include/xen/pci_ids.h
> > @@ -1,3 +1,6 @@
> > +#ifndef INCLUDE_XEN_PCI_IDS_H
> > +#define INCLUDE_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 /* INCLUDE_XEN_PCI_IDS_H */
> > diff --git a/xen/include/xen/softirq.h b/xen/include/xen/softirq.h
> > index 33d6f2ecd2..90d4875df7 100644
> > --- a/xen/include/xen/softirq.h
> > +++ b/xen/include/xen/softirq.h
> > @@ -1,5 +1,6 @@
> > -#if !defined(__XEN_SOFTIRQ_H__) && !defined(__ASSEMBLY__)
> > -#define __XEN_SOFTIRQ_H__
> > +#ifndef INCLUDE_XEN_SOFTIRQ_H
> > +#define INCLUDE_XEN_SOFTIRQ_H
> > +#ifndef __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 /* __XEN_SOFTIRQ_H__ */
> > +#endif /* __ASSEMBLY__ */
> > +#endif /* INCLUDE_XEN_SOFTIRQ_H */
> > diff --git a/xen/include/xen/vmap.h b/xen/include/xen/vmap.h
> > index 0c16baa85f..ec1b6b05e9 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)
> > -#define __XEN_VMAP_H__
> > +#ifndef INCLUDE_XEN_VMAP_H
> > +#define INCLUDE_XEN_VMAP_H
> > +#ifdef VMAP_VIRT_START
> >  
> >  #include <xen/mm-frame.h>
> >  #include <xen/page-size.h>
> > @@ -42,4 +43,5 @@ static inline void vm_init(void)
> >      vm_init_type(VMAP_DEFAULT, (void *)VMAP_VIRT_START, arch_vmap_virt_end());
> >  }
> >  
> > -#endif /* __XEN_VMAP_H__ */
> > +#endif /* VMAP_VIRT_START */
> > +#endif /* INCLUDE_XEN_VMAP_H */
> 
> 

[PATCH 10/17] x86/asm: address violations of MISRA C:2012 Directive 4.10

[Alessandro Zucchelli]

From: Simone Ballarin <simone.ballarin@bugseng.com>

Amend generation script, 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").

This patch amends the Makefile adding the required inclusion guards
for xlat.h.

Add deviation comment for files intended for multiple inclusion.

Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>
Signed-off-by: Maria Celeste Cesario <maria.celeste.cesario@bugseng.com>
Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
Signed-off-by: Alessandro Zucchelli <alessandro.zucchelli@bugseng.com>

---
Changes in v4:
- changed guard creation for autogenerated headers in Makefile
Changes in v3:
- fix inconsistent deviation ID
- remove trailing underscores
Changes in v2:
- merge patches 7/13 and 13/13 of v1 as they had the same
  commit message
- amend the Makefile to produce the required inclusion guard
- use the format introduced with doc/misra/safe.json instead of
  a generic text-based deviation
---
 docs/misra/safe.json                   |  8 ++++++++
 xen/arch/x86/include/asm/compat.h      |  5 +++++
 xen/arch/x86/include/asm/cpufeatures.h |  5 +----
 xen/arch/x86/include/asm/efibind.h     |  5 +++++
 xen/include/Makefile                   | 11 +++++++++--
 5 files changed, 28 insertions(+), 6 deletions(-)

diff --git a/docs/misra/safe.json b/docs/misra/safe.json
index 3e9ebff8fe..0739eac806 100644
--- a/docs/misra/safe.json
+++ b/docs/misra/safe.json
@@ -90,6 +90,14 @@
             "name": "Dir 4.10: direct inclusion guard before",
             "text": "Headers with just the direct inclusion guard before the inclusion guard are safe."
         },
+        {
+            "id": "SAF-11-safe",
+            "analyser": {
+                "eclair": "MC3R1.D4.10"
+            },
+            "name": "Dir 4.10: file intended for multiple inclusion",
+            "text": "Files intended for multiple inclusion are not supposed to comply with D4.10."
+        },
         {
             "id": "SAF-11-safe",
             "analyser": {},
diff --git a/xen/arch/x86/include/asm/compat.h b/xen/arch/x86/include/asm/compat.h
index 818cad87db..36e7750843 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 ba3df174b7..d590fe31ea 100644
--- a/xen/arch/x86/include/asm/cpufeatures.h
+++ b/xen/arch/x86/include/asm/cpufeatures.h
@@ -1,7 +1,4 @@
-/*
- * Explicitly intended for multiple inclusion.
- */
-
+/* SAF-11-safe file intended for multiple inclusion */
 #include <xen/lib/x86/cpuid-autogen.h>
 
 /* Number of capability words covered by the featureset words. */
diff --git a/xen/arch/x86/include/asm/efibind.h b/xen/arch/x86/include/asm/efibind.h
index bce02f3707..75684742be 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/include/Makefile b/xen/include/Makefile
index 058b0a566b..eccc2e017a 100644
--- a/xen/include/Makefile
+++ b/xen/include/Makefile
@@ -104,10 +104,17 @@ $(obj)/compat/.xlat/%.lst: $(srcdir)/xlat.lst FORCE
 xlat-y := $(shell sed -ne 's,@arch@,$(compat-arch-y),g' -re 's,^[?!][[:blank:]]+[^[:blank:]]+[[:blank:]]+,,p' $(srcdir)/xlat.lst | uniq)
 xlat-y := $(filter $(patsubst compat/%,%,$(headers-y)),$(xlat-y))
 
+ARCHDIR = $(shell echo $(SRCARCH) | tr a-z A-Z)
+
 quiet_cmd_xlat_h = GEN     $@
-cmd_xlat_h = \
-	cat $(filter %.h,$^) >$@.new; \
+define cmd_xlat_h
+	guard=$$(echo ASM_${SRCARCH}_COMPAT_XLAT_H | tr a-z A-Z); \
+	echo "#ifndef $$guard" > $@.new; \
+	echo "#define $$guard" >> $@.new; \
+	cat $(filter %.h,$^) >> $@.new; \
+	echo "#endif /* $$guard */" >> $@.new; \
 	mv -f $@.new $@
+endef
 
 $(obj)/compat/xlat.h: $(addprefix $(obj)/compat/.xlat/,$(xlat-y)) FORCE
 	$(call if_changed,xlat_h)
-- 
2.34.1

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

[Jan Beulich]

On 01.07.2024 15:36, Alessandro Zucchelli wrote:
> --- a/docs/misra/safe.json
> +++ b/docs/misra/safe.json
> @@ -90,6 +90,14 @@
>              "name": "Dir 4.10: direct inclusion guard before",
>              "text": "Headers with just the direct inclusion guard before the inclusion guard are safe."
>          },
> +        {
> +            "id": "SAF-11-safe",
> +            "analyser": {
> +                "eclair": "MC3R1.D4.10"
> +            },
> +            "name": "Dir 4.10: file intended for multiple inclusion",
> +            "text": "Files intended for multiple inclusion are not supposed to comply with D4.10."
> +        },
>          {
>              "id": "SAF-11-safe",

This can't be right; the sentinel must have its number changed.

> --- a/xen/include/Makefile
> +++ b/xen/include/Makefile
> @@ -104,10 +104,17 @@ $(obj)/compat/.xlat/%.lst: $(srcdir)/xlat.lst FORCE
>  xlat-y := $(shell sed -ne 's,@arch@,$(compat-arch-y),g' -re 's,^[?!][[:blank:]]+[^[:blank:]]+[[:blank:]]+,,p' $(srcdir)/xlat.lst | uniq)
>  xlat-y := $(filter $(patsubst compat/%,%,$(headers-y)),$(xlat-y))
>  
> +ARCHDIR = $(shell echo $(SRCARCH) | tr a-z A-Z)

Why is this being added here? It's not used ...

>  quiet_cmd_xlat_h = GEN     $@
> -cmd_xlat_h = \
> -	cat $(filter %.h,$^) >$@.new; \
> +define cmd_xlat_h
> +	guard=$$(echo ASM_${SRCARCH}_COMPAT_XLAT_H | tr a-z A-Z); \
> +	echo "#ifndef $$guard" > $@.new; \
> +	echo "#define $$guard" >> $@.new; \
> +	cat $(filter %.h,$^) >> $@.new; \
> +	echo "#endif /* $$guard */" >> $@.new; \
>  	mv -f $@.new $@
> +endef
>  
>  $(obj)/compat/xlat.h: $(addprefix $(obj)/compat/.xlat/,$(xlat-y)) FORCE
>  	$(call if_changed,xlat_h)

... anywhere. Did you mean to use it in place of ${SRCARCH}?

Jan

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

[Alessandro Zucchelli]

On 2024-07-03 14:49, Jan Beulich wrote:
> On 01.07.2024 15:36, Alessandro Zucchelli wrote:
>> --- a/docs/misra/safe.json
>> +++ b/docs/misra/safe.json
>> @@ -90,6 +90,14 @@
>>              "name": "Dir 4.10: direct inclusion guard before",
>>              "text": "Headers with just the direct inclusion guard 
>> before the inclusion guard are safe."
>>          },
>> +        {
>> +            "id": "SAF-11-safe",
>> +            "analyser": {
>> +                "eclair": "MC3R1.D4.10"
>> +            },
>> +            "name": "Dir 4.10: file intended for multiple inclusion",
>> +            "text": "Files intended for multiple inclusion are not 
>> supposed to comply with D4.10."
>> +        },
>>          {
>>              "id": "SAF-11-safe",
> 
> This can't be right; the sentinel must have its number changed.

Yes, this must have been overlooked during the rebasing of the previous 
patch version's commits.

> 
>> --- a/xen/include/Makefile
>> +++ b/xen/include/Makefile
>> @@ -104,10 +104,17 @@ $(obj)/compat/.xlat/%.lst: $(srcdir)/xlat.lst 
>> FORCE
>>  xlat-y := $(shell sed -ne 's,@arch@,$(compat-arch-y),g' -re 
>> 's,^[?!][[:blank:]]+[^[:blank:]]+[[:blank:]]+,,p' $(srcdir)/xlat.lst | 
>> uniq)
>>  xlat-y := $(filter $(patsubst compat/%,%,$(headers-y)),$(xlat-y))
>> 
>> +ARCHDIR = $(shell echo $(SRCARCH) | tr a-z A-Z)
> 
> Why is this being added here? It's not used ...

It was mistakenly left here from the previous version of the patch 
series.

> 
>>  quiet_cmd_xlat_h = GEN     $@
>> -cmd_xlat_h = \
>> -	cat $(filter %.h,$^) >$@.new; \
>> +define cmd_xlat_h
>> +	guard=$$(echo ASM_${SRCARCH}_COMPAT_XLAT_H | tr a-z A-Z); \
>> +	echo "#ifndef $$guard" > $@.new; \
>> +	echo "#define $$guard" >> $@.new; \
>> +	cat $(filter %.h,$^) >> $@.new; \
>> +	echo "#endif /* $$guard */" >> $@.new; \
>>  	mv -f $@.new $@
>> +endef
>> 
>>  $(obj)/compat/xlat.h: $(addprefix $(obj)/compat/.xlat/,$(xlat-y)) 
>> FORCE
>>  	$(call if_changed,xlat_h)
> 
> ... anywhere. Did you mean to use it in place of ${SRCARCH}?

No, SRCARCH is correct, as ARCHDIR was supposed to be removed.

-- 
Alessandro Zucchelli, B.Sc.

Software Engineer, BUGSENG (https://bugseng.com)

[PATCH 11/17] xen/arm: address violations of MISRA C:2012 Directive 4.10

[Alessandro Zucchelli]

From: Maria Celeste Cesario <maria.celeste.cesario@bugseng.com>

Add or modify 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: Maria Celeste Cesario  <maria.celeste.cesario@bugseng.com>
Signed-off-by: Simone Ballarin  <simone.ballarin@bugseng.com>
Signed-off-by: Nicola Vetrini  <nicola.vetrini@bugseng.com>
Signed-off-by: Alessandro Zucchelli <alessandro.zucchelli@bugseng.com>

--
Changes in v4:
- fix typo in include guard for arm efibind.

Commit introduced in v3
---
 xen/arch/arm/include/asm/domain.h      | 6 +++---
 xen/arch/arm/include/asm/efibind.h     | 5 +++++
 xen/arch/arm/include/asm/event.h       | 6 +++---
 xen/arch/arm/include/asm/grant_table.h | 6 +++---
 xen/arch/arm/include/asm/io.h          | 6 +++---
 xen/arch/arm/include/asm/irq.h         | 6 +++---
 xen/arch/arm/include/asm/smp.h         | 6 +++---
 xen/arch/arm/include/asm/spinlock.h    | 6 +++---
 xen/arch/arm/include/asm/system.h      | 6 +++---
 9 files changed, 29 insertions(+), 24 deletions(-)

diff --git a/xen/arch/arm/include/asm/domain.h b/xen/arch/arm/include/asm/domain.h
index f1d72c6e48..e5f011c2fa 100644
--- a/xen/arch/arm/include/asm/domain.h
+++ b/xen/arch/arm/include/asm/domain.h
@@ -1,5 +1,5 @@
-#ifndef __ASM_DOMAIN_H__
-#define __ASM_DOMAIN_H__
+#ifndef ASM_ARM_DOMAIN_H
+#define ASM_ARM_DOMAIN_H
 
 #include <xen/cache.h>
 #include <xen/timer.h>
@@ -310,7 +310,7 @@ static inline void update_guest_memory_policy(struct vcpu *v,
                                               struct guest_memory_policy *gmp)
 {}
 
-#endif /* __ASM_DOMAIN_H__ */
+#endif /* ASM_ARM_DOMAIN_H */
 
 /*
  * Local variables:
diff --git a/xen/arch/arm/include/asm/efibind.h b/xen/arch/arm/include/asm/efibind.h
index 09dca7a8c9..61e23c6510 100644
--- a/xen/arch/arm/include/asm/efibind.h
+++ b/xen/arch/arm/include/asm/efibind.h
@@ -1,2 +1,7 @@
+#ifndef ASM_ARM_EFIBIND_H
+#define ASM_ARM_EFIBIND_H
+
 #include <xen/types.h>
 #include <asm/arm64/efibind.h>
+
+#endif /* ASM_ARM_EFIBIND_H */
diff --git a/xen/arch/arm/include/asm/event.h b/xen/arch/arm/include/asm/event.h
index b14c166ad6..db5d5ea37a 100644
--- a/xen/arch/arm/include/asm/event.h
+++ b/xen/arch/arm/include/asm/event.h
@@ -1,5 +1,5 @@
-#ifndef __ASM_EVENT_H__
-#define __ASM_EVENT_H__
+#ifndef ASM_ARM_EVENT_H
+#define ASM_ARM_EVENT_H
 
 #include <asm/domain.h>
 
@@ -52,7 +52,7 @@ static inline bool arch_virq_is_global(unsigned int virq)
     return true;
 }
 
-#endif
+#endif /* ASM_ARM_EVENT_H */
 /*
  * Local variables:
  * mode: C
diff --git a/xen/arch/arm/include/asm/grant_table.h b/xen/arch/arm/include/asm/grant_table.h
index d3c518a926..0363c6aa1e 100644
--- a/xen/arch/arm/include/asm/grant_table.h
+++ b/xen/arch/arm/include/asm/grant_table.h
@@ -1,5 +1,5 @@
-#ifndef __ASM_GRANT_TABLE_H__
-#define __ASM_GRANT_TABLE_H__
+#ifndef ASM_ARM_GRANT_TABLE_H
+#define ASM_ARM_GRANT_TABLE_H
 
 #include <xen/grant_table.h>
 #include <xen/kernel.h>
@@ -76,7 +76,7 @@ int replace_grant_host_mapping(uint64_t gpaddr, mfn_t frame,
 #define gnttab_need_iommu_mapping(d)                    \
     (is_domain_direct_mapped(d) && is_iommu_enabled(d))
 
-#endif /* __ASM_GRANT_TABLE_H__ */
+#endif /* ASM_ARM_GRANT_TABLE_H */
 /*
  * Local variables:
  * mode: C
diff --git a/xen/arch/arm/include/asm/io.h b/xen/arch/arm/include/asm/io.h
index e426804424..8b5f811ec8 100644
--- a/xen/arch/arm/include/asm/io.h
+++ b/xen/arch/arm/include/asm/io.h
@@ -1,5 +1,5 @@
-#ifndef _ASM_IO_H
-#define _ASM_IO_H
+#ifndef ASM_ARM_IO_H
+#define ASM_ARM_IO_H
 
 #if defined(CONFIG_ARM_32)
 # include <asm/arm32/io.h>
@@ -9,7 +9,7 @@
 # error "unknown ARM variant"
 #endif
 
-#endif
+#endif /* ASM_ARM_IO_H */
 /*
  * Local variables:
  * mode: C
diff --git a/xen/arch/arm/include/asm/irq.h b/xen/arch/arm/include/asm/irq.h
index ec437add09..5a1b2cd5ba 100644
--- a/xen/arch/arm/include/asm/irq.h
+++ b/xen/arch/arm/include/asm/irq.h
@@ -1,5 +1,5 @@
-#ifndef _ASM_HW_IRQ_H
-#define _ASM_HW_IRQ_H
+#ifndef ASM_ARM_IRQ_H
+#define ASM_ARM_IRQ_H
 
 #include <xen/device_tree.h>
 #include <public/device_tree_defs.h>
@@ -101,7 +101,7 @@ bool irq_type_set_by_domain(const struct domain *d);
 void irq_end_none(struct irq_desc *irq);
 #define irq_end_none irq_end_none
 
-#endif /* _ASM_HW_IRQ_H */
+#endif /* ASM_ARM_IRQ_H */
 /*
  * Local variables:
  * mode: C
diff --git a/xen/arch/arm/include/asm/smp.h b/xen/arch/arm/include/asm/smp.h
index e99a3a3f53..44e4183470 100644
--- a/xen/arch/arm/include/asm/smp.h
+++ b/xen/arch/arm/include/asm/smp.h
@@ -1,5 +1,5 @@
-#ifndef __ASM_SMP_H
-#define __ASM_SMP_H
+#ifndef ASM_ARM_SMP_H
+#define ASM_ARM_SMP_H
 
 #ifndef __ASSEMBLY__
 #include <xen/cpumask.h>
@@ -39,7 +39,7 @@ extern unsigned int smp_get_max_cpus(void);
 
 #define cpu_physical_id(cpu) cpu_logical_map(cpu)
 
-#endif
+#endif /* ASM_ARM_SMP_H */
 
 /*
  * Local variables:
diff --git a/xen/arch/arm/include/asm/spinlock.h b/xen/arch/arm/include/asm/spinlock.h
index 42b0f584fe..326005c39a 100644
--- a/xen/arch/arm/include/asm/spinlock.h
+++ b/xen/arch/arm/include/asm/spinlock.h
@@ -1,5 +1,5 @@
-#ifndef __ASM_SPINLOCK_H
-#define __ASM_SPINLOCK_H
+#ifndef ASM_ARM_SPINLOCK_H
+#define ASM_ARM_SPINLOCK_H
 
 #define arch_lock_acquire_barrier() smp_mb()
 #define arch_lock_release_barrier() smp_mb()
@@ -12,4 +12,4 @@
 
 #define arch_lock_signal_wmb()  arch_lock_signal()
 
-#endif /* __ASM_SPINLOCK_H */
+#endif /* ASM_ARM_SPINLOCK_H */
diff --git a/xen/arch/arm/include/asm/system.h b/xen/arch/arm/include/asm/system.h
index 65d5c8e423..faf9227f53 100644
--- a/xen/arch/arm/include/asm/system.h
+++ b/xen/arch/arm/include/asm/system.h
@@ -1,6 +1,6 @@
 /* Portions taken from Linux arch arm */
-#ifndef __ASM_SYSTEM_H
-#define __ASM_SYSTEM_H
+#ifndef ASM_ARM_SYSTEM_H
+#define ASM_ARM_SYSTEM_H
 
 #include <xen/lib.h>
 #include <public/arch-arm.h>
@@ -62,7 +62,7 @@ static inline int local_abort_is_enabled(void)
 
 extern struct vcpu *__context_switch(struct vcpu *prev, struct vcpu *next);
 
-#endif
+#endif /* ASM_ARM_SYSTEM_H */
 /*
  * Local variables:
  * mode: C
-- 
2.34.1

Re: [PATCH 11/17] xen/arm: address violations of MISRA C:2012 Directive 4.10

[Stefano Stabellini]

On Mon, 1 Jul 2024, Alessandro Zucchelli wrote:
> From: Maria Celeste Cesario <maria.celeste.cesario@bugseng.com>
> 
> Add or modify 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: Maria Celeste Cesario  <maria.celeste.cesario@bugseng.com>
> Signed-off-by: Simone Ballarin  <simone.ballarin@bugseng.com>
> Signed-off-by: Nicola Vetrini  <nicola.vetrini@bugseng.com>
> Signed-off-by: Alessandro Zucchelli <alessandro.zucchelli@bugseng.com>

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

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

[Alessandro Zucchelli]

From: Maria Celeste Cesario <maria.celeste.cesario@bugseng.com>

Modify creation rule for asm-offsets.h to conform to
the new standard and to not generate conflicting guards
between architectures (which is a violation of the directive).
Modify generic-y creation rule to generate code without violations
and to conform to the new standard.

Mechanical change.
---
Changes in v4:
- improve inclusion guard generation

Signed-off-by: Maria Celeste Cesario  <maria.celeste.cesario@bugseng.com>
Signed-off-by: Simone Ballarin  <simone.ballarin@bugseng.com>
Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
Signed-off-by: Alessandro Zucchelli <alessandro.zucchelli@bugseng.com>

---
 xen/build.mk                     | 7 ++++---
 xen/scripts/Makefile.asm-generic | 8 +++++++-
 2 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/xen/build.mk b/xen/build.mk
index 0f490ca71b..bbd4c2970f 100644
--- a/xen/build.mk
+++ b/xen/build.mk
@@ -47,6 +47,7 @@ asm-offsets.s: arch/$(SRCARCH)/$(ARCH)/asm-offsets.c
 
 arch/$(SRCARCH)/include/asm/asm-offsets.h: asm-offsets.s
 	@(set -e; \
+	  guard=$$(echo ASM_${SRCARCH}_ASM_OFFSETS_H | tr a-z A-Z); \
 	  echo "/*"; \
 	  echo " * DO NOT MODIFY."; \
 	  echo " *"; \
@@ -54,12 +55,12 @@ arch/$(SRCARCH)/include/asm/asm-offsets.h: asm-offsets.s
 	  echo " *"; \
 	  echo " */"; \
 	  echo ""; \
-	  echo "#ifndef __ASM_OFFSETS_H__"; \
-	  echo "#define __ASM_OFFSETS_H__"; \
+	  echo "#ifndef $$guard"; \
+	  echo "#define $$guard"; \
 	  echo ""; \
 	  sed -rne "/^[^#].*==>/{s:.*==>(.*)<==.*:\1:; s: [\$$#]: :; p;}"; \
 	  echo ""; \
-	  echo "#endif") <$< >$@
+	  echo "#endif /* $$guard */") <$< >$@
 
 build-dirs := $(patsubst %/built_in.o,%,$(filter %/built_in.o,$(ALL_OBJS) $(ALL_LIBS)))
 
diff --git a/xen/scripts/Makefile.asm-generic b/xen/scripts/Makefile.asm-generic
index b0d356bfa3..5775acf44b 100644
--- a/xen/scripts/Makefile.asm-generic
+++ b/xen/scripts/Makefile.asm-generic
@@ -32,7 +32,13 @@ old-headers := $(wildcard $(obj)/*.h)
 unwanted    := $(filter-out $(generic-y) $(generated-y),$(old-headers))
 
 quiet_cmd_wrap = WRAP    $@
-      cmd_wrap = echo "\#include <asm-generic/$*.h>" > $@
+cmd_wrap = \
+    guard=$$(echo ASM_GENERIC_${SRCARCH}_$*_H | tr a-z A-Z); \
+    echo "\#ifndef $$guard" >$@.new; \
+    echo "\#define $$guard" >>$@.new; \
+    echo "\#include <asm-generic/$*.h>" >>$@.new; \
+    echo "\#endif /* $$guard */" >>$@.new; \
+    mv -f $@.new $@
 
 quiet_cmd_remove = REMOVE  $(unwanted)
       cmd_remove = rm -f $(unwanted)
-- 
2.34.1

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

[Jan Beulich]

On 01.07.2024 15:43, Alessandro Zucchelli wrote:
> From: Maria Celeste Cesario <maria.celeste.cesario@bugseng.com>
> 
> Modify creation rule for asm-offsets.h to conform to
> the new standard and to not generate conflicting guards
> between architectures (which is a violation of the directive).
> Modify generic-y creation rule to generate code without violations
> and to conform to the new standard.
> 
> Mechanical change.
> ---
> Changes in v4:
> - improve inclusion guard generation
> 
> Signed-off-by: Maria Celeste Cesario  <maria.celeste.cesario@bugseng.com>
> Signed-off-by: Simone Ballarin  <simone.ballarin@bugseng.com>
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> Signed-off-by: Alessandro Zucchelli <alessandro.zucchelli@bugseng.com>
> 
> ---
>  xen/build.mk                     | 7 ++++---
>  xen/scripts/Makefile.asm-generic | 8 +++++++-
>  2 files changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/build.mk b/xen/build.mk
> index 0f490ca71b..bbd4c2970f 100644
> --- a/xen/build.mk
> +++ b/xen/build.mk
> @@ -47,6 +47,7 @@ asm-offsets.s: arch/$(SRCARCH)/$(ARCH)/asm-offsets.c
>  
>  arch/$(SRCARCH)/include/asm/asm-offsets.h: asm-offsets.s
>  	@(set -e; \
> +	  guard=$$(echo ASM_${SRCARCH}_ASM_OFFSETS_H | tr a-z A-Z); \

Was this and ...

> --- a/xen/scripts/Makefile.asm-generic
> +++ b/xen/scripts/Makefile.asm-generic
> @@ -32,7 +32,13 @@ old-headers := $(wildcard $(obj)/*.h)
>  unwanted    := $(filter-out $(generic-y) $(generated-y),$(old-headers))
>  
>  quiet_cmd_wrap = WRAP    $@
> -      cmd_wrap = echo "\#include <asm-generic/$*.h>" > $@
> +cmd_wrap = \
> +    guard=$$(echo ASM_GENERIC_${SRCARCH}_$*_H | tr a-z A-Z); \

... this mean to be using $(ARCHDIR) as well then?

Jan

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

[Alessandro Zucchelli]

On 2024-07-03 14:51, Jan Beulich wrote:
> On 01.07.2024 15:43, Alessandro Zucchelli wrote:
>> From: Maria Celeste Cesario <maria.celeste.cesario@bugseng.com>
>> 
>> Modify creation rule for asm-offsets.h to conform to
>> the new standard and to not generate conflicting guards
>> between architectures (which is a violation of the directive).
>> Modify generic-y creation rule to generate code without violations
>> and to conform to the new standard.
>> 
>> Mechanical change.
>> ---
>> Changes in v4:
>> - improve inclusion guard generation
>> 
>> Signed-off-by: Maria Celeste Cesario  
>> <maria.celeste.cesario@bugseng.com>
>> Signed-off-by: Simone Ballarin  <simone.ballarin@bugseng.com>
>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>> Signed-off-by: Alessandro Zucchelli <alessandro.zucchelli@bugseng.com>
>> 
>> ---
>>  xen/build.mk                     | 7 ++++---
>>  xen/scripts/Makefile.asm-generic | 8 +++++++-
>>  2 files changed, 11 insertions(+), 4 deletions(-)
>> 
>> diff --git a/xen/build.mk b/xen/build.mk
>> index 0f490ca71b..bbd4c2970f 100644
>> --- a/xen/build.mk
>> +++ b/xen/build.mk
>> @@ -47,6 +47,7 @@ asm-offsets.s: arch/$(SRCARCH)/$(ARCH)/asm-offsets.c
>> 
>>  arch/$(SRCARCH)/include/asm/asm-offsets.h: asm-offsets.s
>>  	@(set -e; \
>> +	  guard=$$(echo ASM_${SRCARCH}_ASM_OFFSETS_H | tr a-z A-Z); \
> 
> Was this and ...
> 
>> --- a/xen/scripts/Makefile.asm-generic
>> +++ b/xen/scripts/Makefile.asm-generic
>> @@ -32,7 +32,13 @@ old-headers := $(wildcard $(obj)/*.h)
>>  unwanted    := $(filter-out $(generic-y) 
>> $(generated-y),$(old-headers))
>> 
>>  quiet_cmd_wrap = WRAP    $@
>> -      cmd_wrap = echo "\#include <asm-generic/$*.h>" > $@
>> +cmd_wrap = \
>> +    guard=$$(echo ASM_GENERIC_${SRCARCH}_$*_H | tr a-z A-Z); \
> 
> ... this mean to be using $(ARCHDIR) as well then?

The introduction of ARCHDIR is a slip-up from amending the previous 
version of the patch series.
This script is correct as it is.

-- 
Alessandro Zucchelli, B.Sc.

Software Engineer, BUGSENG (https://bugseng.com)

[PATCH 13/17] xen: add deviations for MISRA C 2012 Dir D4.10

[Alessandro Zucchelli]

From: Nicola Vetrini <nicola.vetrini@bugseng.com>

Add safe deviation for *.c files, as estabilished in past discussion.

Signed-off-by: Maria Celeste Cesario  <maria.celeste.cesario@bugseng.com>
Signed-off-by: Simone Ballarin  <simone.ballarin@bugseng.com>
Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
Signed-off-by: Alessandro Zucchelli <alessandro.zucchelli@bugseng.com>

---
Changes in v4:
- split the commit from the unrelated SAF deviation

Link to the discussion thread:
https://lists.xenproject.org/archives/html/xen-devel/2023-09/msg00239.html
---
 automation/eclair_analysis/ECLAIR/deviations.ecl | 5 +++++
 docs/misra/deviations.rst                        | 7 +++++++
 2 files changed, 12 insertions(+)

diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl b/automation/eclair_analysis/ECLAIR/deviations.ecl
index 1c39a9a16d..c6b1a10bcf 100644
--- a/automation/eclair_analysis/ECLAIR/deviations.ecl
+++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
@@ -78,6 +78,11 @@ conform to the directive."
 -config=MC3R1.D4.10,reports+={safe, "first_area(text(^/\\* Generated file, do not edit! \\*/$, begin-3))"}
 -doc_end
 
+-doc_begin="Including multiple times a .c file is safe because every function or data item
+it defines would (in the common case) be already defined. Peer reviewed by the community."
+-config=MC3R1.D4.10,reports+={safe, "all_area(all_loc(^.*\\.c$))"}
+-doc_end
+
 #
 # Series 5.
 #
diff --git a/docs/misra/deviations.rst b/docs/misra/deviations.rst
index 1ecce1469a..a5dac38bb3 100644
--- a/docs/misra/deviations.rst
+++ b/docs/misra/deviations.rst
@@ -30,6 +30,13 @@ Deviations related to MISRA C:2012 Directives:
        not to add an additional encapsulation layer.
      - Tagged as `deliberate` for ECLAIR.
 
+   * - D4.10
+     - Including multiple times a .c file is safe because every function or data item
+       it defines would in (the common case) be already defined.
+       Peer reviewed by the community.
+     - Tagged as `safe` for ECLAIR.
+
+
 Deviations related to MISRA C:2012 Rules:
 -----------------------------------------
 
-- 
2.34.1

Re: [PATCH 13/17] xen: add deviations for MISRA C 2012 Dir D4.10

[Stefano Stabellini]

On Mon, 1 Jul 2024, Alessandro Zucchelli wrote:
> From: Nicola Vetrini <nicola.vetrini@bugseng.com>
> 
> Add safe deviation for *.c files, as estabilished in past discussion.
> 
> Signed-off-by: Maria Celeste Cesario  <maria.celeste.cesario@bugseng.com>
> Signed-off-by: Simone Ballarin  <simone.ballarin@bugseng.com>
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> Signed-off-by: Alessandro Zucchelli <alessandro.zucchelli@bugseng.com>
> 

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

[PATCH 14/17] xen: add SAF deviation for MISRA C Dir 4.10

[Alessandro Zucchelli]

From: Nicola Vetrini <nicola.vetrini@bugseng.com>

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
Signed-off-by: Alessandro Zucchelli <alessandro.zucchelli@bugseng.com>

---
 docs/misra/safe.json              | 10 +++++++++-
 xen/include/public/arch-x86/xen.h |  1 +
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/docs/misra/safe.json b/docs/misra/safe.json
index 0739eac806..a1cd821aea 100644
--- a/docs/misra/safe.json
+++ b/docs/misra/safe.json
@@ -99,7 +99,15 @@
             "text": "Files intended for multiple inclusion are not supposed to comply with D4.10."
         },
         {
-            "id": "SAF-11-safe",
+            "id": "SAF-12-safe",
+            "analyser": {
+                "eclair": "MC3R1.D4.10"
+            },
+            "name": "Dir 4.10: arch-x86/xen.h include before guard",
+            "text": "This file needs to start with #include ../xen.h to generate preprocessed code in the correct order."
+        },
+        {
+            "id": "SAF-13-safe",
             "analyser": {},
             "name": "Sentinel",
             "text": "Next ID to be used"
diff --git a/xen/include/public/arch-x86/xen.h b/xen/include/public/arch-x86/xen.h
index a9a87d9b50..d8ad935af3 100644
--- a/xen/include/public/arch-x86/xen.h
+++ b/xen/include/public/arch-x86/xen.h
@@ -7,6 +7,7 @@
  * Copyright (c) 2004-2006, K A Fraser
  */
 
+/* SAF-12-safe include before guard needed for correct code generation */
 #include "../xen.h"
 
 #ifndef __XEN_PUBLIC_ARCH_X86_XEN_H__
-- 
2.34.1

Re: [PATCH 14/17] xen: add SAF deviation for MISRA C Dir 4.10

[Jan Beulich]

On 01.07.2024 15:45, Alessandro Zucchelli wrote:
> From: Nicola Vetrini <nicola.vetrini@bugseng.com>
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> Signed-off-by: Alessandro Zucchelli <alessandro.zucchelli@bugseng.com>

So no description at all for a somewhat unobvious issue with, I think,
a pretty obvious (but entirely different) solution? And that (obvious)
alternative not even being mentioned, together with why it was not
possible to use? Neither ...

> --- a/docs/misra/safe.json
> +++ b/docs/misra/safe.json
> @@ -99,7 +99,15 @@
>              "text": "Files intended for multiple inclusion are not supposed to comply with D4.10."
>          },
>          {
> -            "id": "SAF-11-safe",
> +            "id": "SAF-12-safe",
> +            "analyser": {
> +                "eclair": "MC3R1.D4.10"
> +            },
> +            "name": "Dir 4.10: arch-x86/xen.h include before guard",
> +            "text": "This file needs to start with #include ../xen.h to generate preprocessed code in the correct order."

... here nor ...

> +        },
> +        {
> +            "id": "SAF-13-safe",
>              "analyser": {},
>              "name": "Sentinel",
>              "text": "Next ID to be used"
> --- a/xen/include/public/arch-x86/xen.h
> +++ b/xen/include/public/arch-x86/xen.h
> @@ -7,6 +7,7 @@
>   * Copyright (c) 2004-2006, K A Fraser
>   */
>  
> +/* SAF-12-safe include before guard needed for correct code generation */
>  #include "../xen.h"
>  
>  #ifndef __XEN_PUBLIC_ARCH_X86_XEN_H__

... here is really becomes clear what "correct" is, or what breaks if this
was moved. However, thinking of moving as the first obvious alternative I
checked other arch-specific headers. None includes ../xen.h (really just
xen.h, as the others all live right in public/) like this. Which made me
conclude that maybe there's something wrong with the x86 header doing so.
And indeed, according to my build testing the #include can simple be
dropped, with just one further change elsewhere; see below.

Jan

public/x86: don't include common xen.h from arch-specific one

No other arch-*.h does so, and arch-x86/xen.h really just takes the role
of arch-x86_32.h and arch-x86_64.h (by those two forwarding there). With
xen.h itself including the per-arch headers, doing so is also kind of
backwards anyway, and just calling for problems. There's exactly one
place where arch-x86/xen.h is included when really xen.h is meant (for
wanting XEN_GUEST_HANDLE_64() to be made available, the default
definition of which lives in the common xen.h).

This then addresses 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").

Reported-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/include/public/arch-x86/xen.h
+++ b/xen/include/public/arch-x86/xen.h
@@ -7,8 +7,6 @@
  * Copyright (c) 2004-2006, K A Fraser
  */
 
-#include "../xen.h"
-
 #ifndef __XEN_PUBLIC_ARCH_X86_XEN_H__
 #define __XEN_PUBLIC_ARCH_X86_XEN_H__
 
--- a/xen/include/xen/lib/x86/cpu-policy.h
+++ b/xen/include/xen/lib/x86/cpu-policy.h
@@ -525,7 +525,7 @@ void x86_cpu_policy_bound_max_leaves(str
 void x86_cpu_policy_shrink_max_leaves(struct cpu_policy *p);
 
 #ifdef __XEN__
-#include <public/arch-x86/xen.h>
+#include <public/xen.h>
 typedef XEN_GUEST_HANDLE_64(xen_cpuid_leaf_t) cpuid_leaf_buffer_t;
 typedef XEN_GUEST_HANDLE_64(xen_msr_entry_t) msr_entry_buffer_t;
 #else

Re: [PATCH 14/17] xen: add SAF deviation for MISRA C Dir 4.10

[Stefano Stabellini]

On Wed, 3 Jul 2024, Jan Beulich wrote:
> public/x86: don't include common xen.h from arch-specific one
> 
> No other arch-*.h does so, and arch-x86/xen.h really just takes the role
> of arch-x86_32.h and arch-x86_64.h (by those two forwarding there). With
> xen.h itself including the per-arch headers, doing so is also kind of
> backwards anyway, and just calling for problems. There's exactly one
> place where arch-x86/xen.h is included when really xen.h is meant (for
> wanting XEN_GUEST_HANDLE_64() to be made available, the default
> definition of which lives in the common xen.h).
> 
> This then addresses 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").
> 
> Reported-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

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


> --- a/xen/include/public/arch-x86/xen.h
> +++ b/xen/include/public/arch-x86/xen.h
> @@ -7,8 +7,6 @@
>   * Copyright (c) 2004-2006, K A Fraser
>   */
>  
> -#include "../xen.h"
> -
>  #ifndef __XEN_PUBLIC_ARCH_X86_XEN_H__
>  #define __XEN_PUBLIC_ARCH_X86_XEN_H__
>  
> --- a/xen/include/xen/lib/x86/cpu-policy.h
> +++ b/xen/include/xen/lib/x86/cpu-policy.h
> @@ -525,7 +525,7 @@ void x86_cpu_policy_bound_max_leaves(str
>  void x86_cpu_policy_shrink_max_leaves(struct cpu_policy *p);
>  
>  #ifdef __XEN__
> -#include <public/arch-x86/xen.h>
> +#include <public/xen.h>
>  typedef XEN_GUEST_HANDLE_64(xen_cpuid_leaf_t) cpuid_leaf_buffer_t;
>  typedef XEN_GUEST_HANDLE_64(xen_msr_entry_t) msr_entry_buffer_t;
>  #else
> 
> 

Re: [PATCH 14/17] xen: add SAF deviation for MISRA C Dir 4.10

[Alessandro Zucchelli]

On 2024-07-13 00:28, Stefano Stabellini wrote:
> On Wed, 3 Jul 2024, Jan Beulich wrote:
>> public/x86: don't include common xen.h from arch-specific one
>> 
>> No other arch-*.h does so, and arch-x86/xen.h really just takes the 
>> role
>> of arch-x86_32.h and arch-x86_64.h (by those two forwarding there). 
>> With
>> xen.h itself including the per-arch headers, doing so is also kind of
>> backwards anyway, and just calling for problems. There's exactly one
>> place where arch-x86/xen.h is included when really xen.h is meant (for
>> wanting XEN_GUEST_HANDLE_64() to be made available, the default
>> definition of which lives in the common xen.h).
>> 
>> This then addresses 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").
>> 
>> Reported-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

One question: when making the new version of the patch series should I
revert this commit as Jan made the patch for it himself, or should Jan's 
fixes
be integrated in the patch series?

Many thanks in advance,
Alessandro Zucchelli

>> --- a/xen/include/public/arch-x86/xen.h
>> +++ b/xen/include/public/arch-x86/xen.h
>> @@ -7,8 +7,6 @@
>>   * Copyright (c) 2004-2006, K A Fraser
>>   */
>> 
>> -#include "../xen.h"
>> -
>>  #ifndef __XEN_PUBLIC_ARCH_X86_XEN_H__
>>  #define __XEN_PUBLIC_ARCH_X86_XEN_H__
>> 
>> --- a/xen/include/xen/lib/x86/cpu-policy.h
>> +++ b/xen/include/xen/lib/x86/cpu-policy.h
>> @@ -525,7 +525,7 @@ void x86_cpu_policy_bound_max_leaves(str
>>  void x86_cpu_policy_shrink_max_leaves(struct cpu_policy *p);
>> 
>>  #ifdef __XEN__
>> -#include <public/arch-x86/xen.h>
>> +#include <public/xen.h>
>>  typedef XEN_GUEST_HANDLE_64(xen_cpuid_leaf_t) cpuid_leaf_buffer_t;
>>  typedef XEN_GUEST_HANDLE_64(xen_msr_entry_t) msr_entry_buffer_t;
>>  #else
>> 
>> 

-- 
Alessandro Zucchelli, B.Sc.

Software Engineer, BUGSENG (https://bugseng.com)

Re: [PATCH 14/17] xen: add SAF deviation for MISRA C Dir 4.10

[Jan Beulich]

On 22.07.2024 10:54, Alessandro Zucchelli wrote:
> On 2024-07-13 00:28, Stefano Stabellini wrote:
>> On Wed, 3 Jul 2024, Jan Beulich wrote:
>>> public/x86: don't include common xen.h from arch-specific one
>>>
>>> No other arch-*.h does so, and arch-x86/xen.h really just takes the 
>>> role
>>> of arch-x86_32.h and arch-x86_64.h (by those two forwarding there). 
>>> With
>>> xen.h itself including the per-arch headers, doing so is also kind of
>>> backwards anyway, and just calling for problems. There's exactly one
>>> place where arch-x86/xen.h is included when really xen.h is meant (for
>>> wanting XEN_GUEST_HANDLE_64() to be made available, the default
>>> definition of which lives in the common xen.h).
>>>
>>> This then addresses 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").
>>>
>>> Reported-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> 
> One question: when making the new version of the patch series should I
> revert this commit as Jan made the patch for it himself, or should Jan's 
> fixes
> be integrated in the patch series?

You certainly want to leave out this patch. Whether you want to add my
alternative into the series is really up to you. I intend to commit it
relatively soon anyway.

Jan

[PATCH 15/17] xen/x86: rename inclusion guards for consistency

[Alessandro Zucchelli]

From: Maria Celeste Cesario <maria.celeste.cesario@bugseng.com>

Edit inclusion guards in order to make them consistent with the
estabilished naming conventions.

No functional change.

Signed-off-by: Maria Celeste Cesario  <maria.celeste.cesario@bugseng.com>
Signed-off-by: Simone Ballarin  <simone.ballarin@bugseng.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
Signed-off-by: Alessandro Zucchelli <alessandro.zucchelli@bugseng.com>

---
Changes in v4:
- improved commit message
- change inclusion guards names
Commit introduced in v3
---
 xen/arch/x86/include/asm/domain.h      | 6 +++---
 xen/arch/x86/include/asm/event.h       | 6 +++---
 xen/arch/x86/include/asm/grant_table.h | 6 +++---
 xen/arch/x86/include/asm/io.h          | 6 +++---
 xen/arch/x86/include/asm/irq.h         | 6 +++---
 xen/arch/x86/include/asm/smp.h         | 6 +++---
 xen/arch/x86/include/asm/spinlock.h    | 6 +++---
 xen/arch/x86/include/asm/system.h      | 6 +++---
 8 files changed, 24 insertions(+), 24 deletions(-)

diff --git a/xen/arch/x86/include/asm/domain.h b/xen/arch/x86/include/asm/domain.h
index f5daeb182b..ddf0a28f48 100644
--- a/xen/arch/x86/include/asm/domain.h
+++ b/xen/arch/x86/include/asm/domain.h
@@ -1,5 +1,5 @@
-#ifndef __ASM_DOMAIN_H__
-#define __ASM_DOMAIN_H__
+#ifndef ASM_X86_DOMAIN_H
+#define ASM_X86_DOMAIN_H
 
 #include <xen/mm.h>
 #include <xen/radix-tree.h>
@@ -779,7 +779,7 @@ struct arch_vcpu_io {
 /* Maxphysaddr supportable by the paging infrastructure. */
 unsigned int domain_max_paddr_bits(const struct domain *d);
 
-#endif /* __ASM_DOMAIN_H__ */
+#endif /* ASM_X86_DOMAIN_H */
 
 /*
  * Local variables:
diff --git a/xen/arch/x86/include/asm/event.h b/xen/arch/x86/include/asm/event.h
index 5e09ede6d7..c0b6088c91 100644
--- a/xen/arch/x86/include/asm/event.h
+++ b/xen/arch/x86/include/asm/event.h
@@ -6,8 +6,8 @@
  *
  */
 
-#ifndef __ASM_EVENT_H__
-#define __ASM_EVENT_H__
+#ifndef ASM_X86_EVENT_H
+#define ASM_X86_EVENT_H
 
 #include <xen/shared.h>
 
@@ -53,4 +53,4 @@ static inline bool arch_virq_is_global(unsigned int virq)
              (pv_shim && (chn)->port && (chn)->state == ECS_RESERVED)
 #endif
 
-#endif
+#endif /* ASM_X86_EVENT_H */
diff --git a/xen/arch/x86/include/asm/grant_table.h b/xen/arch/x86/include/asm/grant_table.h
index 5c23cec90c..2468f21ad6 100644
--- a/xen/arch/x86/include/asm/grant_table.h
+++ b/xen/arch/x86/include/asm/grant_table.h
@@ -4,8 +4,8 @@
  * Copyright (c) 2004-2005 K A Fraser
  */
 
-#ifndef __ASM_GRANT_TABLE_H__
-#define __ASM_GRANT_TABLE_H__
+#ifndef ASM_X86_GRANT_TABLE_H
+#define ASM_X86_GRANT_TABLE_H
 
 #include <asm/paging.h>
 
@@ -72,4 +72,4 @@ static inline void gnttab_clear_flags(struct domain *d,
 #define gnttab_need_iommu_mapping(d)                \
     (!paging_mode_translate(d) && need_iommu_pt_sync(d))
 
-#endif /* __ASM_GRANT_TABLE_H__ */
+#endif /* ASM_X86_GRANT_TABLE_H */
diff --git a/xen/arch/x86/include/asm/io.h b/xen/arch/x86/include/asm/io.h
index 1cb4217cff..afa1a9e69d 100644
--- a/xen/arch/x86/include/asm/io.h
+++ b/xen/arch/x86/include/asm/io.h
@@ -1,5 +1,5 @@
-#ifndef _ASM_IO_H
-#define _ASM_IO_H
+#ifndef ASM_X86_IO_H
+#define ASM_X86_IO_H
 
 #include <xen/vmap.h>
 #include <xen/types.h>
@@ -57,4 +57,4 @@ struct cpu_user_regs;
 unsigned int ioemul_handle_proliant_quirk(
     uint8_t opcode, char *io_emul_stub, const struct cpu_user_regs *regs);
 
-#endif
+#endif /* ASM_X86_IO_H */
diff --git a/xen/arch/x86/include/asm/irq.h b/xen/arch/x86/include/asm/irq.h
index 5f445299be..aa8e4f3900 100644
--- a/xen/arch/x86/include/asm/irq.h
+++ b/xen/arch/x86/include/asm/irq.h
@@ -1,5 +1,5 @@
-#ifndef _ASM_HW_IRQ_H
-#define _ASM_HW_IRQ_H
+#ifndef ASM_X86_IRQ_H
+#define ASM_X86_IRQ_H
 
 /* (C) 1992, 1993 Linus Torvalds, (C) 1997 Ingo Molnar */
 
@@ -231,4 +231,4 @@ int allocate_and_map_gsi_pirq(struct domain *d, int index, int *pirq_p);
 int allocate_and_map_msi_pirq(struct domain *d, int index, int *pirq_p,
                               int type, struct msi_info *msi);
 
-#endif /* _ASM_HW_IRQ_H */
+#endif /* ASM_X86_IRQ_H */
diff --git a/xen/arch/x86/include/asm/smp.h b/xen/arch/x86/include/asm/smp.h
index c8c7960134..cdfe9ea96f 100644
--- a/xen/arch/x86/include/asm/smp.h
+++ b/xen/arch/x86/include/asm/smp.h
@@ -1,5 +1,5 @@
-#ifndef __ASM_SMP_H
-#define __ASM_SMP_H
+#ifndef ASM_X86_SMP_H
+#define ASM_X86_SMP_H
 
 /*
  * We need the APIC definitions automatically as part of 'smp.h'
@@ -81,4 +81,4 @@ void *cpu_alloc_stack(unsigned int cpu);
 
 #endif /* !__ASSEMBLY__ */
 
-#endif
+#endif /* ASM_X86_SMP_H */
diff --git a/xen/arch/x86/include/asm/spinlock.h b/xen/arch/x86/include/asm/spinlock.h
index 56f6095752..2a86560343 100644
--- a/xen/arch/x86/include/asm/spinlock.h
+++ b/xen/arch/x86/include/asm/spinlock.h
@@ -1,5 +1,5 @@
-#ifndef __ASM_SPINLOCK_H
-#define __ASM_SPINLOCK_H
+#ifndef ASM_X86_SPINLOCK_H
+#define ASM_X86_SPINLOCK_H
 
 #define _raw_read_unlock(l) \
     BUILD_BUG_ON(sizeof((l)->lock) != 4); /* Clang doesn't support %z in asm. */ \
@@ -24,4 +24,4 @@
     arch_lock_signal();             \
 })
 
-#endif /* __ASM_SPINLOCK_H */
+#endif /* ASM_X86_SPINLOCK_H */
diff --git a/xen/arch/x86/include/asm/system.h b/xen/arch/x86/include/asm/system.h
index 73cb16ca68..12e8b974b1 100644
--- a/xen/arch/x86/include/asm/system.h
+++ b/xen/arch/x86/include/asm/system.h
@@ -1,5 +1,5 @@
-#ifndef __ASM_SYSTEM_H
-#define __ASM_SYSTEM_H
+#ifndef ASM_X86_SYSTEM_H
+#define ASM_X86_SYSTEM_H
 
 #include <xen/bitops.h>
 #include <xen/bug.h>
@@ -269,4 +269,4 @@ void load_system_tables(void);
 void percpu_traps_init(void);
 void subarch_percpu_traps_init(void);
 
-#endif
+#endif /* ASM_X86_SYSTEM_H */
-- 
2.34.1

Re: [PATCH 15/17] xen/x86: rename inclusion guards for consistency

[Jan Beulich]

On 01.07.2024 15:46, Alessandro Zucchelli wrote:
> From: Maria Celeste Cesario <maria.celeste.cesario@bugseng.com>
> 
> Edit inclusion guards in order to make them consistent with the
> estabilished naming conventions.

The code changes all look okay to me, but as before: Where did those
"established naming conventions" go? Grep-ing for "guard" under
docs/misra/ yields only a single hit, which is unrelated.

Jan

[PATCH 16/17] xen/build: address violation of MISRA C Directive 4.10

[Alessandro Zucchelli]

This addresses violations of MISRA C:2012 Rule 4.10 which states as
following: Precautions shall be taken in order to prevent the contents
of a header file being included more than once.

Added inclusion guards for autogenerated header files: include/xen/compile.h
and include/xen/hypercall-defs.h.

No functional change.

Signed-off-by: Alessandro Zucchelli <alessandro.zucchelli@bugseng.com>
---
 xen/build.mk         | 6 +++++-
 xen/include/Makefile | 5 ++++-
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/xen/build.mk b/xen/build.mk
index bbd4c2970f..c83882c949 100644
--- a/xen/build.mk
+++ b/xen/build.mk
@@ -18,6 +18,8 @@ quiet_cmd_compile.h = UPD     $@
 define cmd_compile.h
     if [ ! -r $@ -o -O $@ ]; then \
 	cat .banner; \
+	echo '#ifndef INCLUDE_XEN_COMPILE_H' >> $(dot-target).tmp; \
+	echo '#define INCLUDE_XEN_COMPILE_H' >> $(dot-target).tmp; \
 	sed -e 's/@@date@@/$(XEN_BUILD_DATE)/g' \
 	    -e 's/@@time@@/$(XEN_BUILD_TIME)/g' \
 	    -e 's/@@whoami@@/$(XEN_WHOAMI)/g' \
@@ -28,8 +30,9 @@ define cmd_compile.h
 	    -e 's/@@subversion@@/$(XEN_SUBVERSION)/g' \
 	    -e 's/@@extraversion@@/$(XEN_EXTRAVERSION)/g' \
 	    -e 's!@@changeset@@!$(shell $(srctree)/tools/scmversion $(XEN_ROOT) || echo "unavailable")!g' \
-	    < $< > $(dot-target).tmp; \
+	    < $< >> $(dot-target).tmp; \
 	sed -rf $(srctree)/tools/process-banner.sed < .banner >> $(dot-target).tmp; \
+	echo '#endif /* INCLUDE_XEN_COMPILE_H */' >> $(dot-target).tmp; \
 	mv -f $(dot-target).tmp $@; \
     fi
 endef
@@ -40,6 +43,7 @@ include/xen/compile.h: include/xen/compile.h.in .banner FORCE
 
 targets += include/xen/compile.h
 
+
 -include $(wildcard .asm-offsets.s.d)
 asm-offsets.s: arch/$(SRCARCH)/$(ARCH)/asm-offsets.c
 	$(CC) $(call cpp_flags,$(c_flags)) -S -g0 -o $@.new -MQ $@ $<
diff --git a/xen/include/Makefile b/xen/include/Makefile
index eccc2e017a..34e133949c 100644
--- a/xen/include/Makefile
+++ b/xen/include/Makefile
@@ -121,7 +121,10 @@ $(obj)/compat/xlat.h: $(addprefix $(obj)/compat/.xlat/,$(xlat-y)) FORCE
 
 quiet_cmd_genhyp = GEN     $@
 define cmd_genhyp
-    awk -f $(srctree)/scripts/gen_hypercall.awk <$< >$@
+    echo "#ifndef XEN_HYPERCALL_DEFS_H" >$@; \
+    echo "#define XEN_HYPERCALL_DEFS_H" >>$@; \
+    awk -f $(srctree)/scripts/gen_hypercall.awk <$< >>$@; \
+    echo "#endif /* XEN_HYPERCALL_DEFS_H */" >>$@
 endef
 
 all: $(obj)/xen/hypercall-defs.h
-- 
2.34.1

Re: [PATCH 16/17] xen/build: address violation of MISRA C Directive 4.10

[Jan Beulich]

On 01.07.2024 15:46, Alessandro Zucchelli wrote:
> --- a/xen/build.mk
> +++ b/xen/build.mk
> @@ -18,6 +18,8 @@ quiet_cmd_compile.h = UPD     $@
>  define cmd_compile.h
>      if [ ! -r $@ -o -O $@ ]; then \
>  	cat .banner; \
> +	echo '#ifndef INCLUDE_XEN_COMPILE_H' >> $(dot-target).tmp; \

Leaving aside the question on the INCLUDE_ prefix (see earlier comments
on another patch in this series), I wonder what good a guard does here
in the first place. But anyway, I expect this again gets us into "we
need to mechanically and slavishly follow the rules" territory.

However, shouldn't this first "echo" use > in place of >>, to prevent
surprises when e.g. an earlier build was interrupted at exactly the
"right" point?

Jan

[RFC PATCH 17/17] CODING_STYLE: Add a section on header guards naming conventions

[Alessandro Zucchelli]

This section explains which format should be followed by header
inclusion guards via a drop-down list of rules.

No functional change.

Signed-off-by: Alessandro Zucchelli <alessandro.zucchelli@bugseng.com>
---
 CODING_STYLE | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/CODING_STYLE b/CODING_STYLE
index 7f6e9ad065..87836c97d4 100644
--- a/CODING_STYLE
+++ b/CODING_STYLE
@@ -167,3 +167,22 @@ the end of files.  It should be:
  * indent-tabs-mode: nil
  * End:
  */
+
+Header inclusion guards
+-----------------------
+
+Unless differently specified all header files should have proper inclusion
+guards in order to avoid being included multiple times.
+The following naming conventions have been devised:
+- private headers -> <dir>_<filename>_H
+- asm-generic headers -> ASM_GENERIC_<filename>_H
+    - #ifndef ASM_GENERIC_X86_PERCPU_H
+      #define ASM_GENERIC_X86_PERCPU_H
+      //...
+      #endif /* ASM_GENERIC_X86_PERCPU_H */
+- arch/<architecture>/include/asm/<subdir>/<filename>.h -> ASM_<architecture>_<subdir>_<filename>_H
+    - #ifndef ASM_X86_DOMAIN_H
+      #define ASM_X86_DOMAIN_H
+      //...
+      #endif /* ASM_X86_DOMAIN_H */
+
-- 
2.34.1

Re: [RFC PATCH 17/17] CODING_STYLE: Add a section on header guards naming conventions

[Jan Beulich]

On 01.07.2024 15:46, Alessandro Zucchelli wrote:
> This section explains which format should be followed by header
> inclusion guards via a drop-down list of rules.

Ah, so this answers my earlier question regarding where the naming
rules are spelled out. Yet why is this not much earlier in the series,
/before/ anything trying to follow these rules?

> --- a/CODING_STYLE
> +++ b/CODING_STYLE
> @@ -167,3 +167,22 @@ the end of files.  It should be:
>   * indent-tabs-mode: nil
>   * End:
>   */

This footer is not just an example; it also serves that function here.
While not strictly needed in this file, I think it should still remain
last.

> +
> +Header inclusion guards
> +-----------------------
> +
> +Unless differently specified all header files should have proper inclusion
> +guards in order to avoid being included multiple times.
> +The following naming conventions have been devised:
> +- private headers -> <dir>_<filename>_H
> +- asm-generic headers -> ASM_GENERIC_<filename>_H
> +    - #ifndef ASM_GENERIC_X86_PERCPU_H
> +      #define ASM_GENERIC_X86_PERCPU_H
> +      //...
> +      #endif /* ASM_GENERIC_X86_PERCPU_H */

GENERIC contradicts X86. Please try to avoid giving confusing / possibly
misleading examples.

> +- arch/<architecture>/include/asm/<subdir>/<filename>.h -> ASM_<architecture>_<subdir>_<filename>_H
> +    - #ifndef ASM_X86_DOMAIN_H
> +      #define ASM_X86_DOMAIN_H
> +      //...
> +      #endif /* ASM_X86_DOMAIN_H */

I'm afraid I can't connect the example to the filename pattern given:
The example has 4 components separated by 3 underscores, when the
pattern suggests 5 and 4 respectively.

> +

Please avoid empty lines at the bottom of files.

Having reached the end, I don't see common headers (the ones under
xen/include/ in the tree) covered. I can only conclude that the odd
INCLUDE_ prefixes I had asked about were derived from the "private
headers" rule. Yet what's in xen/include/ aren't private headers.

I further have to note that, as indicated during the earlier discussion,
I still cannot see how occasional ambiguity is going to be dealt with.
IOW from the rules above two different headers could still end up with
the same guard identifier.

Finally, it shouldn't be silently assumed that all name components are
to be converted to upper case; everything wants spelling out imo.

Jan

Re: [RFC PATCH 17/17] CODING_STYLE: Add a section on header guards naming conventions

[Stefano Stabellini]

On Wed, 3 Jul 2024, Jan Beulich wrote:
> On 01.07.2024 15:46, Alessandro Zucchelli wrote:
> > This section explains which format should be followed by header
> > inclusion guards via a drop-down list of rules.
> 
> Ah, so this answers my earlier question regarding where the naming
> rules are spelled out. Yet why is this not much earlier in the series,
> /before/ anything trying to follow these rules?
> 
> > --- a/CODING_STYLE
> > +++ b/CODING_STYLE
> > @@ -167,3 +167,22 @@ the end of files.  It should be:
> >   * indent-tabs-mode: nil
> >   * End:
> >   */
> 
> This footer is not just an example; it also serves that function here.
> While not strictly needed in this file, I think it should still remain
> last.

+1


> > +Header inclusion guards
> > +-----------------------
> > +
> > +Unless differently specified all header files should have proper inclusion
> > +guards in order to avoid being included multiple times.
> > +The following naming conventions have been devised:
> > +- private headers -> <dir>_<filename>_H
> > +- asm-generic headers -> ASM_GENERIC_<filename>_H
> > +    - #ifndef ASM_GENERIC_X86_PERCPU_H
> > +      #define ASM_GENERIC_X86_PERCPU_H
> > +      //...
> > +      #endif /* ASM_GENERIC_X86_PERCPU_H */
> 
> GENERIC contradicts X86. Please try to avoid giving confusing / possibly
> misleading examples.

For clarity, Jan means that GENERIC by definition is not arch-specific
so GENERIC_X86 or GENERIC_ARM is a contradiction and it would be better
not to use it as reference example for this rule


> > +- arch/<architecture>/include/asm/<subdir>/<filename>.h -> ASM_<architecture>_<subdir>_<filename>_H
> > +    - #ifndef ASM_X86_DOMAIN_H
> > +      #define ASM_X86_DOMAIN_H
> > +      //...
> > +      #endif /* ASM_X86_DOMAIN_H */
> 
> I'm afraid I can't connect the example to the filename pattern given:
> The example has 4 components separated by 3 underscores, when the
> pattern suggests 5 and 4 respectively.

I read the above with <subdir> being optional. But yes it is unclear
because the example should have both the header guard but also the
related file path. In this case the file path corresponding to
ASM_X86_DOMAIN_H would be arch/x86/include/asm/domain.h



> Please avoid empty lines at the bottom of files.
> 
> Having reached the end, I don't see common headers (the ones under
> xen/include/ in the tree) covered. I can only conclude that the odd
> INCLUDE_ prefixes I had asked about were derived from the "private
> headers" rule. Yet what's in xen/include/ aren't private headers.

Yeah. I proposed in a previous email to use:

- xen/include/xen/<filename>.h -> XEN_<filename>_H
- xen/include/xen/<subdir>/<filename>.h -> XEN_<subdir>_<filename>_H

with <subdir> being optional.


> I further have to note that, as indicated during the earlier discussion,
> I still cannot see how occasional ambiguity is going to be dealt with.
> IOW from the rules above two different headers could still end up with
> the same guard identifier.

Maybe something like this?

"In the event of naming collisions, exceptions to the coding style may
be made at the discretion of the contributor and maintainers."


> Finally, it shouldn't be silently assumed that all name components are
> to be converted to upper case; everything wants spelling out imo.
 
+1

Re: [RFC PATCH 17/17] CODING_STYLE: Add a section on header guards naming conventions

[Jan Beulich]

On 13.07.2024 00:38, Stefano Stabellini wrote:
> On Wed, 3 Jul 2024, Jan Beulich wrote:
>> I further have to note that, as indicated during the earlier discussion,
>> I still cannot see how occasional ambiguity is going to be dealt with.
>> IOW from the rules above two different headers could still end up with
>> the same guard identifier.
> 
> Maybe something like this?
> 
> "In the event of naming collisions, exceptions to the coding style may
> be made at the discretion of the contributor and maintainers."

Hmm, maybe I wasn't clear enough then. My take is that the scheme should
simply not allow for possible collisions. Neither the contributor nor the
reviewer may spot such a collision, and it may therefore take until the
first full scan that one is actually noticed. Which I consider too late
in the process, even if we already were at the point where commits were
checked pre-push.

Jan

Re: [RFC PATCH 17/17] CODING_STYLE: Add a section on header guards naming conventions

[Alessandro Zucchelli]

On 2024-07-15 09:23, Jan Beulich wrote:
> On 13.07.2024 00:38, Stefano Stabellini wrote:
>> On Wed, 3 Jul 2024, Jan Beulich wrote:
>>> I further have to note that, as indicated during the earlier 
>>> discussion,
>>> I still cannot see how occasional ambiguity is going to be dealt 
>>> with.
>>> IOW from the rules above two different headers could still end up 
>>> with
>>> the same guard identifier.
>> 
>> Maybe something like this?
>> 
>> "In the event of naming collisions, exceptions to the coding style may
>> be made at the discretion of the contributor and maintainers."
> 
> Hmm, maybe I wasn't clear enough then. My take is that the scheme 
> should
> simply not allow for possible collisions. Neither the contributor nor 
> the
> reviewer may spot such a collision, and it may therefore take until the
> first full scan that one is actually noticed. Which I consider too late
> in the process, even if we already were at the point where commits were
> checked pre-push.
> 

If we could come to an agreement, I will make the new version of the 
patch
series with all the needed changes.
-- 
Alessandro Zucchelli, B.Sc.

Software Engineer, BUGSENG (https://bugseng.com)

Re: [RFC PATCH 17/17] CODING_STYLE: Add a section on header guards naming conventions

[Stefano Stabellini]

On Mon, 15 Jul 2024, Jan Beulich wrote:
> On 13.07.2024 00:38, Stefano Stabellini wrote:
> > On Wed, 3 Jul 2024, Jan Beulich wrote:
> >> I further have to note that, as indicated during the earlier discussion,
> >> I still cannot see how occasional ambiguity is going to be dealt with.
> >> IOW from the rules above two different headers could still end up with
> >> the same guard identifier.
> > 
> > Maybe something like this?
> > 
> > "In the event of naming collisions, exceptions to the coding style may
> > be made at the discretion of the contributor and maintainers."
> 
> Hmm, maybe I wasn't clear enough then. My take is that the scheme should
> simply not allow for possible collisions. Neither the contributor nor the
> reviewer may spot such a collision, and it may therefore take until the
> first full scan that one is actually noticed. Which I consider too late
> in the process, even if we already were at the point where commits were
> checked pre-push.

Looking at the proposal, copy/pasted here for convenience:

- private headers -> <dir>_<filename>_H
- asm-generic headers -> ASM_GENERIC_<filename>_H
    - #ifndef ASM_GENERIC_X86_PERCPU_H
      #define ASM_GENERIC_X86_PERCPU_H
      //...
      #endif /* ASM_GENERIC_X86_PERCPU_H */
- arch/<architecture>/include/asm/<subdir>/<filename>.h -> ASM_<architecture>_<subdir>_<filename>_H
    - #ifndef ASM_X86_DOMAIN_H
      #define ASM_X86_DOMAIN_H
      //...
      #endif /* ASM_X86_DOMAIN_H */
- xen/include/xen/<filename>.h -> XEN_<filename>_H
- xen/include/xen/<subdir>/<filename>.h -> XEN_<subdir>_<filename>_H


The only possibility for collision that I can see is from the first
point:

- private headers -> <dir>_<filename>_H


two directories like this could collide:

- arch/arm/arm64/lib/something.h -> LIB_SOMETHING_H
- arch/arm/arm32/lib/something.h -> LIB_SOMETHING_H
- arch/x86/lib/something.h -> LIB_SOMETHING_H

(Leaving aside that in this example it would not be an issue because the
three headers are not meant to be all included in the same file.)

Can we specify that <dir> should go all the way back to the arch/ or or
common or drivers directory?

- arch/arm/arm64/lib/something.h -> ARM_ARM64_LIB_SOMETHING_H
- arch/arm/arm32/lib/something.h -> ARM_ARM32_LIB_SOMETHING_H
- arch/x86/lib/something.h -> X86_LIB_SOMETHING_H


We can rely on the filesystem paths to make sure to avoid collisions.

Re: [RFC PATCH 17/17] CODING_STYLE: Add a section on header guards naming conventions

[Jan Beulich]

On 16.07.2024 02:43, Stefano Stabellini wrote:
> On Mon, 15 Jul 2024, Jan Beulich wrote:
>> On 13.07.2024 00:38, Stefano Stabellini wrote:
>>> On Wed, 3 Jul 2024, Jan Beulich wrote:
>>>> I further have to note that, as indicated during the earlier discussion,
>>>> I still cannot see how occasional ambiguity is going to be dealt with.
>>>> IOW from the rules above two different headers could still end up with
>>>> the same guard identifier.
>>>
>>> Maybe something like this?
>>>
>>> "In the event of naming collisions, exceptions to the coding style may
>>> be made at the discretion of the contributor and maintainers."
>>
>> Hmm, maybe I wasn't clear enough then. My take is that the scheme should
>> simply not allow for possible collisions. Neither the contributor nor the
>> reviewer may spot such a collision, and it may therefore take until the
>> first full scan that one is actually noticed. Which I consider too late
>> in the process, even if we already were at the point where commits were
>> checked pre-push.
> 
> Looking at the proposal, copy/pasted here for convenience:
> 
> - private headers -> <dir>_<filename>_H
> - asm-generic headers -> ASM_GENERIC_<filename>_H
>     - #ifndef ASM_GENERIC_X86_PERCPU_H
>       #define ASM_GENERIC_X86_PERCPU_H
>       //...
>       #endif /* ASM_GENERIC_X86_PERCPU_H */
> - arch/<architecture>/include/asm/<subdir>/<filename>.h -> ASM_<architecture>_<subdir>_<filename>_H
>     - #ifndef ASM_X86_DOMAIN_H
>       #define ASM_X86_DOMAIN_H
>       //...
>       #endif /* ASM_X86_DOMAIN_H */
> - xen/include/xen/<filename>.h -> XEN_<filename>_H
> - xen/include/xen/<subdir>/<filename>.h -> XEN_<subdir>_<filename>_H
> 
> 
> The only possibility for collision that I can see is from the first
> point:
> 
> - private headers -> <dir>_<filename>_H

I don't think this is the only possibility of collisions. The <subdir>_<filename>
parts can similarly cause problems if either of the two involved names contains
e.g. a dash (which would need converting to an underscore) or an underscore. To
avoid this, the name separators (slashes in the actual file names) there may need
representing by double underscores.

> two directories like this could collide:
> 
> - arch/arm/arm64/lib/something.h -> LIB_SOMETHING_H
> - arch/arm/arm32/lib/something.h -> LIB_SOMETHING_H
> - arch/x86/lib/something.h -> LIB_SOMETHING_H
> 
> (Leaving aside that in this example it would not be an issue because the
> three headers are not meant to be all included in the same file.)
> 
> Can we specify that <dir> should go all the way back to the arch/ or or
> common or drivers directory?
> 
> - arch/arm/arm64/lib/something.h -> ARM_ARM64_LIB_SOMETHING_H
> - arch/arm/arm32/lib/something.h -> ARM_ARM32_LIB_SOMETHING_H
> - arch/x86/lib/something.h -> X86_LIB_SOMETHING_H

We can of course, so long as we're okay(ish) with the length and redundancy. As
already indicated before, there are downsides to this. Yet a firm scheme with
few rules has the benefit that it might even be possible to use a script to do
all the guard adjustments.

Jan

> We can rely on the filesystem paths to make sure to avoid collisions.

Re: [RFC PATCH 17/17] CODING_STYLE: Add a section on header guards naming conventions

[Stefano Stabellini]

On Tue, 16 Jul 2024, Jan Beulich wrote:
> On 16.07.2024 02:43, Stefano Stabellini wrote:
> > On Mon, 15 Jul 2024, Jan Beulich wrote:
> >> On 13.07.2024 00:38, Stefano Stabellini wrote:
> >>> On Wed, 3 Jul 2024, Jan Beulich wrote:
> >>>> I further have to note that, as indicated during the earlier discussion,
> >>>> I still cannot see how occasional ambiguity is going to be dealt with.
> >>>> IOW from the rules above two different headers could still end up with
> >>>> the same guard identifier.
> >>>
> >>> Maybe something like this?
> >>>
> >>> "In the event of naming collisions, exceptions to the coding style may
> >>> be made at the discretion of the contributor and maintainers."
> >>
> >> Hmm, maybe I wasn't clear enough then. My take is that the scheme should
> >> simply not allow for possible collisions. Neither the contributor nor the
> >> reviewer may spot such a collision, and it may therefore take until the
> >> first full scan that one is actually noticed. Which I consider too late
> >> in the process, even if we already were at the point where commits were
> >> checked pre-push.
> > 
> > Looking at the proposal, copy/pasted here for convenience:
> > 
> > - private headers -> <dir>_<filename>_H
> > - asm-generic headers -> ASM_GENERIC_<filename>_H
> >     - #ifndef ASM_GENERIC_X86_PERCPU_H
> >       #define ASM_GENERIC_X86_PERCPU_H
> >       //...
> >       #endif /* ASM_GENERIC_X86_PERCPU_H */
> > - arch/<architecture>/include/asm/<subdir>/<filename>.h -> ASM_<architecture>_<subdir>_<filename>_H
> >     - #ifndef ASM_X86_DOMAIN_H
> >       #define ASM_X86_DOMAIN_H
> >       //...
> >       #endif /* ASM_X86_DOMAIN_H */
> > - xen/include/xen/<filename>.h -> XEN_<filename>_H
> > - xen/include/xen/<subdir>/<filename>.h -> XEN_<subdir>_<filename>_H
> > 
> > 
> > The only possibility for collision that I can see is from the first
> > point:
> > 
> > - private headers -> <dir>_<filename>_H
> 
> I don't think this is the only possibility of collisions. The <subdir>_<filename>
> parts can similarly cause problems if either of the two involved names contains
> e.g. a dash (which would need converting to an underscore) or an underscore. To
> avoid this, the name separators (slashes in the actual file names) there may need
> representing by double underscores.

I am OK with you two underscores as name separator (slashes in the
actual file names). Would you do it for all levels like this?

- arch/arm/arm64/lib/something.h -> ARM__ARM64__LIB__SOMETHING_H
- arch/arm/arm32/lib/something.h -> ARM__ARM32__LIB__SOMETHING_H
- arch/x86/lib/something.h -> X86__LIB__SOMETHING_H


I think it is better than the below:

- arch/arm/arm64/lib/something.h -> ARM_ARM64__LIB__SOMETHING_H
- arch/arm/arm32/lib/something.h -> ARM_ARM32__LIB__SOMETHING_H
- arch/x86/lib/something.h -> X86_LIB__SOMETHING_H


> > two directories like this could collide:
> > 
> > - arch/arm/arm64/lib/something.h -> LIB_SOMETHING_H
> > - arch/arm/arm32/lib/something.h -> LIB_SOMETHING_H
> > - arch/x86/lib/something.h -> LIB_SOMETHING_H
> > 
> > (Leaving aside that in this example it would not be an issue because the
> > three headers are not meant to be all included in the same file.)
> > 
> > Can we specify that <dir> should go all the way back to the arch/ or or
> > common or drivers directory?
> > 
> > - arch/arm/arm64/lib/something.h -> ARM_ARM64_LIB_SOMETHING_H
> > - arch/arm/arm32/lib/something.h -> ARM_ARM32_LIB_SOMETHING_H
> > - arch/x86/lib/something.h -> X86_LIB_SOMETHING_H
> 
> We can of course, so long as we're okay(ish) with the length and redundancy. As
> already indicated before, there are downsides to this. Yet a firm scheme with
> few rules has the benefit that it might even be possible to use a script to do
> all the guard adjustments.

I also like the firm scheme with few rules

Re: [RFC PATCH 17/17] CODING_STYLE: Add a section on header guards naming conventions

[Jan Beulich]

On 17.07.2024 02:20, Stefano Stabellini wrote:
> On Tue, 16 Jul 2024, Jan Beulich wrote:
>> On 16.07.2024 02:43, Stefano Stabellini wrote:
>>> On Mon, 15 Jul 2024, Jan Beulich wrote:
>>>> On 13.07.2024 00:38, Stefano Stabellini wrote:
>>>>> On Wed, 3 Jul 2024, Jan Beulich wrote:
>>>>>> I further have to note that, as indicated during the earlier discussion,
>>>>>> I still cannot see how occasional ambiguity is going to be dealt with.
>>>>>> IOW from the rules above two different headers could still end up with
>>>>>> the same guard identifier.
>>>>>
>>>>> Maybe something like this?
>>>>>
>>>>> "In the event of naming collisions, exceptions to the coding style may
>>>>> be made at the discretion of the contributor and maintainers."
>>>>
>>>> Hmm, maybe I wasn't clear enough then. My take is that the scheme should
>>>> simply not allow for possible collisions. Neither the contributor nor the
>>>> reviewer may spot such a collision, and it may therefore take until the
>>>> first full scan that one is actually noticed. Which I consider too late
>>>> in the process, even if we already were at the point where commits were
>>>> checked pre-push.
>>>
>>> Looking at the proposal, copy/pasted here for convenience:
>>>
>>> - private headers -> <dir>_<filename>_H
>>> - asm-generic headers -> ASM_GENERIC_<filename>_H
>>>     - #ifndef ASM_GENERIC_X86_PERCPU_H
>>>       #define ASM_GENERIC_X86_PERCPU_H
>>>       //...
>>>       #endif /* ASM_GENERIC_X86_PERCPU_H */
>>> - arch/<architecture>/include/asm/<subdir>/<filename>.h -> ASM_<architecture>_<subdir>_<filename>_H
>>>     - #ifndef ASM_X86_DOMAIN_H
>>>       #define ASM_X86_DOMAIN_H
>>>       //...
>>>       #endif /* ASM_X86_DOMAIN_H */
>>> - xen/include/xen/<filename>.h -> XEN_<filename>_H
>>> - xen/include/xen/<subdir>/<filename>.h -> XEN_<subdir>_<filename>_H
>>>
>>>
>>> The only possibility for collision that I can see is from the first
>>> point:
>>>
>>> - private headers -> <dir>_<filename>_H
>>
>> I don't think this is the only possibility of collisions. The <subdir>_<filename>
>> parts can similarly cause problems if either of the two involved names contains
>> e.g. a dash (which would need converting to an underscore) or an underscore. To
>> avoid this, the name separators (slashes in the actual file names) there may need
>> representing by double underscores.
> 
> I am OK with you two underscores as name separator (slashes in the
> actual file names). Would you do it for all levels like this?
> 
> - arch/arm/arm64/lib/something.h -> ARM__ARM64__LIB__SOMETHING_H
> - arch/arm/arm32/lib/something.h -> ARM__ARM32__LIB__SOMETHING_H
> - arch/x86/lib/something.h -> X86__LIB__SOMETHING_H
> 
> 
> I think it is better than the below:
> 
> - arch/arm/arm64/lib/something.h -> ARM_ARM64__LIB__SOMETHING_H
> - arch/arm/arm32/lib/something.h -> ARM_ARM32__LIB__SOMETHING_H
> - arch/x86/lib/something.h -> X86_LIB__SOMETHING_H

Hmm, maybe it's indeed better to do it entirely uniformly then.

Jan

Re: [RFC PATCH 17/17] CODING_STYLE: Add a section on header guards naming conventions

[Stefano Stabellini]

On Wed, 17 Jul 2024, Jan Beulich wrote:
> On 17.07.2024 02:20, Stefano Stabellini wrote:
> > On Tue, 16 Jul 2024, Jan Beulich wrote:
> >> On 16.07.2024 02:43, Stefano Stabellini wrote:
> >>> On Mon, 15 Jul 2024, Jan Beulich wrote:
> >>>> On 13.07.2024 00:38, Stefano Stabellini wrote:
> >>>>> On Wed, 3 Jul 2024, Jan Beulich wrote:
> >>>>>> I further have to note that, as indicated during the earlier discussion,
> >>>>>> I still cannot see how occasional ambiguity is going to be dealt with.
> >>>>>> IOW from the rules above two different headers could still end up with
> >>>>>> the same guard identifier.
> >>>>>
> >>>>> Maybe something like this?
> >>>>>
> >>>>> "In the event of naming collisions, exceptions to the coding style may
> >>>>> be made at the discretion of the contributor and maintainers."
> >>>>
> >>>> Hmm, maybe I wasn't clear enough then. My take is that the scheme should
> >>>> simply not allow for possible collisions. Neither the contributor nor the
> >>>> reviewer may spot such a collision, and it may therefore take until the
> >>>> first full scan that one is actually noticed. Which I consider too late
> >>>> in the process, even if we already were at the point where commits were
> >>>> checked pre-push.
> >>>
> >>> Looking at the proposal, copy/pasted here for convenience:
> >>>
> >>> - private headers -> <dir>_<filename>_H
> >>> - asm-generic headers -> ASM_GENERIC_<filename>_H
> >>>     - #ifndef ASM_GENERIC_X86_PERCPU_H
> >>>       #define ASM_GENERIC_X86_PERCPU_H
> >>>       //...
> >>>       #endif /* ASM_GENERIC_X86_PERCPU_H */
> >>> - arch/<architecture>/include/asm/<subdir>/<filename>.h -> ASM_<architecture>_<subdir>_<filename>_H
> >>>     - #ifndef ASM_X86_DOMAIN_H
> >>>       #define ASM_X86_DOMAIN_H
> >>>       //...
> >>>       #endif /* ASM_X86_DOMAIN_H */
> >>> - xen/include/xen/<filename>.h -> XEN_<filename>_H
> >>> - xen/include/xen/<subdir>/<filename>.h -> XEN_<subdir>_<filename>_H
> >>>
> >>>
> >>> The only possibility for collision that I can see is from the first
> >>> point:
> >>>
> >>> - private headers -> <dir>_<filename>_H
> >>
> >> I don't think this is the only possibility of collisions. The <subdir>_<filename>
> >> parts can similarly cause problems if either of the two involved names contains
> >> e.g. a dash (which would need converting to an underscore) or an underscore. To
> >> avoid this, the name separators (slashes in the actual file names) there may need
> >> representing by double underscores.
> > 
> > I am OK with you two underscores as name separator (slashes in the
> > actual file names). Would you do it for all levels like this?
> > 
> > - arch/arm/arm64/lib/something.h -> ARM__ARM64__LIB__SOMETHING_H
> > - arch/arm/arm32/lib/something.h -> ARM__ARM32__LIB__SOMETHING_H
> > - arch/x86/lib/something.h -> X86__LIB__SOMETHING_H
> > 
> > 
> > I think it is better than the below:
> > 
> > - arch/arm/arm64/lib/something.h -> ARM_ARM64__LIB__SOMETHING_H
> > - arch/arm/arm32/lib/something.h -> ARM_ARM32__LIB__SOMETHING_H
> > - arch/x86/lib/something.h -> X86_LIB__SOMETHING_H
> 
> Hmm, maybe it's indeed better to do it entirely uniformly then.


Do we have agreement on the naming convention then? 


- private headers -> <dir>__<filename>__H
    - arch/arm/arm64/lib/something.h -> ARM__ARM64__LIB__SOMETHING_H
    - arch/arm/arm32/lib/something.h -> ARM__ARM32__LIB__SOMETHING_H
    - arch/x86/lib/something.h -> X86__LIB__SOMETHING_H

- asm-generic headers -> ASM_GENERIC_<filename>_H
    - include/asm-generic/percpu.h -> ASM_GENERIC_X86_PERCPU_H

- arch/<architecture>/include/asm/<subdir>/<filename>.h -> ASM_<architecture>_<subdir>_<filename>_H
    - arch/x86/include/asm/domain.h -> ASM_X86_DOMAIN_H

- include/xen -> XEN_<filename>_H
    - include/xen/percpu.h -> XEN_PERCPU_H


Or do you prefer the double underscore __  in all cases?

Re: [RFC PATCH 17/17] CODING_STYLE: Add a section on header guards naming conventions

[Jan Beulich]

On 18.07.2024 01:02, Stefano Stabellini wrote:
> On Wed, 17 Jul 2024, Jan Beulich wrote:
>> On 17.07.2024 02:20, Stefano Stabellini wrote:
>>> On Tue, 16 Jul 2024, Jan Beulich wrote:
>>>> On 16.07.2024 02:43, Stefano Stabellini wrote:
>>>>> On Mon, 15 Jul 2024, Jan Beulich wrote:
>>>>>> On 13.07.2024 00:38, Stefano Stabellini wrote:
>>>>>>> On Wed, 3 Jul 2024, Jan Beulich wrote:
>>>>>>>> I further have to note that, as indicated during the earlier discussion,
>>>>>>>> I still cannot see how occasional ambiguity is going to be dealt with.
>>>>>>>> IOW from the rules above two different headers could still end up with
>>>>>>>> the same guard identifier.
>>>>>>>
>>>>>>> Maybe something like this?
>>>>>>>
>>>>>>> "In the event of naming collisions, exceptions to the coding style may
>>>>>>> be made at the discretion of the contributor and maintainers."
>>>>>>
>>>>>> Hmm, maybe I wasn't clear enough then. My take is that the scheme should
>>>>>> simply not allow for possible collisions. Neither the contributor nor the
>>>>>> reviewer may spot such a collision, and it may therefore take until the
>>>>>> first full scan that one is actually noticed. Which I consider too late
>>>>>> in the process, even if we already were at the point where commits were
>>>>>> checked pre-push.
>>>>>
>>>>> Looking at the proposal, copy/pasted here for convenience:
>>>>>
>>>>> - private headers -> <dir>_<filename>_H
>>>>> - asm-generic headers -> ASM_GENERIC_<filename>_H
>>>>>     - #ifndef ASM_GENERIC_X86_PERCPU_H
>>>>>       #define ASM_GENERIC_X86_PERCPU_H
>>>>>       //...
>>>>>       #endif /* ASM_GENERIC_X86_PERCPU_H */
>>>>> - arch/<architecture>/include/asm/<subdir>/<filename>.h -> ASM_<architecture>_<subdir>_<filename>_H
>>>>>     - #ifndef ASM_X86_DOMAIN_H
>>>>>       #define ASM_X86_DOMAIN_H
>>>>>       //...
>>>>>       #endif /* ASM_X86_DOMAIN_H */
>>>>> - xen/include/xen/<filename>.h -> XEN_<filename>_H
>>>>> - xen/include/xen/<subdir>/<filename>.h -> XEN_<subdir>_<filename>_H
>>>>>
>>>>>
>>>>> The only possibility for collision that I can see is from the first
>>>>> point:
>>>>>
>>>>> - private headers -> <dir>_<filename>_H
>>>>
>>>> I don't think this is the only possibility of collisions. The <subdir>_<filename>
>>>> parts can similarly cause problems if either of the two involved names contains
>>>> e.g. a dash (which would need converting to an underscore) or an underscore. To
>>>> avoid this, the name separators (slashes in the actual file names) there may need
>>>> representing by double underscores.
>>>
>>> I am OK with you two underscores as name separator (slashes in the
>>> actual file names). Would you do it for all levels like this?
>>>
>>> - arch/arm/arm64/lib/something.h -> ARM__ARM64__LIB__SOMETHING_H
>>> - arch/arm/arm32/lib/something.h -> ARM__ARM32__LIB__SOMETHING_H
>>> - arch/x86/lib/something.h -> X86__LIB__SOMETHING_H
>>>
>>>
>>> I think it is better than the below:
>>>
>>> - arch/arm/arm64/lib/something.h -> ARM_ARM64__LIB__SOMETHING_H
>>> - arch/arm/arm32/lib/something.h -> ARM_ARM32__LIB__SOMETHING_H
>>> - arch/x86/lib/something.h -> X86_LIB__SOMETHING_H
>>
>> Hmm, maybe it's indeed better to do it entirely uniformly then.
> 
> 
> Do we have agreement on the naming convention then? 
> 
> 
> - private headers -> <dir>__<filename>__H
>     - arch/arm/arm64/lib/something.h -> ARM__ARM64__LIB__SOMETHING_H
>     - arch/arm/arm32/lib/something.h -> ARM__ARM32__LIB__SOMETHING_H
>     - arch/x86/lib/something.h -> X86__LIB__SOMETHING_H
> 
> - asm-generic headers -> ASM_GENERIC_<filename>_H
>     - include/asm-generic/percpu.h -> ASM_GENERIC_X86_PERCPU_H
> 
> - arch/<architecture>/include/asm/<subdir>/<filename>.h -> ASM_<architecture>_<subdir>_<filename>_H
>     - arch/x86/include/asm/domain.h -> ASM_X86_DOMAIN_H
> 
> - include/xen -> XEN_<filename>_H
>     - include/xen/percpu.h -> XEN_PERCPU_H
> 
> 
> Or do you prefer the double underscore __  in all cases?

It's not so much prefer, but a requirement if we want to be future proof.
Even for ASM_GENERIC_* that'll be needed, as your outline above simply
doesn't mention the (future) case of there being subdir-s there (see how
Linux already has some). Imo the question doesn't even arise for XEN_*,
as xen/ has subdir-s already.

Jan

Re: [RFC PATCH 17/17] CODING_STYLE: Add a section on header guards naming conventions

[Stefano Stabellini]

On Thu, 18 Jul 2024, Jan Beulich wrote:
> On 18.07.2024 01:02, Stefano Stabellini wrote:
> > On Wed, 17 Jul 2024, Jan Beulich wrote:
> >> On 17.07.2024 02:20, Stefano Stabellini wrote:
> >>> On Tue, 16 Jul 2024, Jan Beulich wrote:
> >>>> On 16.07.2024 02:43, Stefano Stabellini wrote:
> >>>>> On Mon, 15 Jul 2024, Jan Beulich wrote:
> >>>>>> On 13.07.2024 00:38, Stefano Stabellini wrote:
> >>>>>>> On Wed, 3 Jul 2024, Jan Beulich wrote:
> >>>>>>>> I further have to note that, as indicated during the earlier discussion,
> >>>>>>>> I still cannot see how occasional ambiguity is going to be dealt with.
> >>>>>>>> IOW from the rules above two different headers could still end up with
> >>>>>>>> the same guard identifier.
> >>>>>>>
> >>>>>>> Maybe something like this?
> >>>>>>>
> >>>>>>> "In the event of naming collisions, exceptions to the coding style may
> >>>>>>> be made at the discretion of the contributor and maintainers."
> >>>>>>
> >>>>>> Hmm, maybe I wasn't clear enough then. My take is that the scheme should
> >>>>>> simply not allow for possible collisions. Neither the contributor nor the
> >>>>>> reviewer may spot such a collision, and it may therefore take until the
> >>>>>> first full scan that one is actually noticed. Which I consider too late
> >>>>>> in the process, even if we already were at the point where commits were
> >>>>>> checked pre-push.
> >>>>>
> >>>>> Looking at the proposal, copy/pasted here for convenience:
> >>>>>
> >>>>> - private headers -> <dir>_<filename>_H
> >>>>> - asm-generic headers -> ASM_GENERIC_<filename>_H
> >>>>>     - #ifndef ASM_GENERIC_X86_PERCPU_H
> >>>>>       #define ASM_GENERIC_X86_PERCPU_H
> >>>>>       //...
> >>>>>       #endif /* ASM_GENERIC_X86_PERCPU_H */
> >>>>> - arch/<architecture>/include/asm/<subdir>/<filename>.h -> ASM_<architecture>_<subdir>_<filename>_H
> >>>>>     - #ifndef ASM_X86_DOMAIN_H
> >>>>>       #define ASM_X86_DOMAIN_H
> >>>>>       //...
> >>>>>       #endif /* ASM_X86_DOMAIN_H */
> >>>>> - xen/include/xen/<filename>.h -> XEN_<filename>_H
> >>>>> - xen/include/xen/<subdir>/<filename>.h -> XEN_<subdir>_<filename>_H
> >>>>>
> >>>>>
> >>>>> The only possibility for collision that I can see is from the first
> >>>>> point:
> >>>>>
> >>>>> - private headers -> <dir>_<filename>_H
> >>>>
> >>>> I don't think this is the only possibility of collisions. The <subdir>_<filename>
> >>>> parts can similarly cause problems if either of the two involved names contains
> >>>> e.g. a dash (which would need converting to an underscore) or an underscore. To
> >>>> avoid this, the name separators (slashes in the actual file names) there may need
> >>>> representing by double underscores.
> >>>
> >>> I am OK with you two underscores as name separator (slashes in the
> >>> actual file names). Would you do it for all levels like this?
> >>>
> >>> - arch/arm/arm64/lib/something.h -> ARM__ARM64__LIB__SOMETHING_H
> >>> - arch/arm/arm32/lib/something.h -> ARM__ARM32__LIB__SOMETHING_H
> >>> - arch/x86/lib/something.h -> X86__LIB__SOMETHING_H
> >>>
> >>>
> >>> I think it is better than the below:
> >>>
> >>> - arch/arm/arm64/lib/something.h -> ARM_ARM64__LIB__SOMETHING_H
> >>> - arch/arm/arm32/lib/something.h -> ARM_ARM32__LIB__SOMETHING_H
> >>> - arch/x86/lib/something.h -> X86_LIB__SOMETHING_H
> >>
> >> Hmm, maybe it's indeed better to do it entirely uniformly then.
> > 
> > 
> > Do we have agreement on the naming convention then? 
> > 
> > 
> > - private headers -> <dir>__<filename>__H
> >     - arch/arm/arm64/lib/something.h -> ARM__ARM64__LIB__SOMETHING_H
> >     - arch/arm/arm32/lib/something.h -> ARM__ARM32__LIB__SOMETHING_H
> >     - arch/x86/lib/something.h -> X86__LIB__SOMETHING_H
> > 
> > - asm-generic headers -> ASM_GENERIC_<filename>_H
> >     - include/asm-generic/percpu.h -> ASM_GENERIC_X86_PERCPU_H
> > 
> > - arch/<architecture>/include/asm/<subdir>/<filename>.h -> ASM_<architecture>_<subdir>_<filename>_H
> >     - arch/x86/include/asm/domain.h -> ASM_X86_DOMAIN_H
> > 
> > - include/xen -> XEN_<filename>_H
> >     - include/xen/percpu.h -> XEN_PERCPU_H
> > 
> > 
> > Or do you prefer the double underscore __  in all cases?
> 
> It's not so much prefer, but a requirement if we want to be future proof.
> Even for ASM_GENERIC_* that'll be needed, as your outline above simply
> doesn't mention the (future) case of there being subdir-s there (see how
> Linux already has some). Imo the question doesn't even arise for XEN_*,
> as xen/ has subdir-s already.

OK. So it becomes:

- private headers -> <dir>__<filename>_H
    - arch/arm/arm64/lib/something.h -> ARM__ARM64__LIB__SOMETHING_H
    - arch/arm/arm32/lib/something.h -> ARM__ARM32__LIB__SOMETHING_H
    - arch/x86/lib/something.h -> X86__LIB__SOMETHING_H

- asm-generic headers -> ASM_GENERIC__<filename>_H
    - include/asm-generic/percpu.h -> ASM_GENERIC__X86__PERCPU_H

- arch/<architecture>/include/asm/<subdir>/<filename>.h -> ASM__<architecture>__<subdir>__<filename>_H
    - arch/x86/include/asm/domain.h -> ASM__X86__DOMAIN_H

- include/xen -> XEN__<filename>_H
    - include/xen/percpu.h -> XEN__PERCPU_H

If we have found agreement then Alessandro could send an update

Re: [RFC PATCH 17/17] CODING_STYLE: Add a section on header guards naming conventions

[Jan Beulich]

On 19.07.2024 00:01, Stefano Stabellini wrote:
> On Thu, 18 Jul 2024, Jan Beulich wrote:
>> On 18.07.2024 01:02, Stefano Stabellini wrote:
>>> On Wed, 17 Jul 2024, Jan Beulich wrote:
>>>> On 17.07.2024 02:20, Stefano Stabellini wrote:
>>>>> On Tue, 16 Jul 2024, Jan Beulich wrote:
>>>>>> On 16.07.2024 02:43, Stefano Stabellini wrote:
>>>>>>> On Mon, 15 Jul 2024, Jan Beulich wrote:
>>>>>>>> On 13.07.2024 00:38, Stefano Stabellini wrote:
>>>>>>>>> On Wed, 3 Jul 2024, Jan Beulich wrote:
>>>>>>>>>> I further have to note that, as indicated during the earlier discussion,
>>>>>>>>>> I still cannot see how occasional ambiguity is going to be dealt with.
>>>>>>>>>> IOW from the rules above two different headers could still end up with
>>>>>>>>>> the same guard identifier.
>>>>>>>>>
>>>>>>>>> Maybe something like this?
>>>>>>>>>
>>>>>>>>> "In the event of naming collisions, exceptions to the coding style may
>>>>>>>>> be made at the discretion of the contributor and maintainers."
>>>>>>>>
>>>>>>>> Hmm, maybe I wasn't clear enough then. My take is that the scheme should
>>>>>>>> simply not allow for possible collisions. Neither the contributor nor the
>>>>>>>> reviewer may spot such a collision, and it may therefore take until the
>>>>>>>> first full scan that one is actually noticed. Which I consider too late
>>>>>>>> in the process, even if we already were at the point where commits were
>>>>>>>> checked pre-push.
>>>>>>>
>>>>>>> Looking at the proposal, copy/pasted here for convenience:
>>>>>>>
>>>>>>> - private headers -> <dir>_<filename>_H
>>>>>>> - asm-generic headers -> ASM_GENERIC_<filename>_H
>>>>>>>     - #ifndef ASM_GENERIC_X86_PERCPU_H
>>>>>>>       #define ASM_GENERIC_X86_PERCPU_H
>>>>>>>       //...
>>>>>>>       #endif /* ASM_GENERIC_X86_PERCPU_H */
>>>>>>> - arch/<architecture>/include/asm/<subdir>/<filename>.h -> ASM_<architecture>_<subdir>_<filename>_H
>>>>>>>     - #ifndef ASM_X86_DOMAIN_H
>>>>>>>       #define ASM_X86_DOMAIN_H
>>>>>>>       //...
>>>>>>>       #endif /* ASM_X86_DOMAIN_H */
>>>>>>> - xen/include/xen/<filename>.h -> XEN_<filename>_H
>>>>>>> - xen/include/xen/<subdir>/<filename>.h -> XEN_<subdir>_<filename>_H
>>>>>>>
>>>>>>>
>>>>>>> The only possibility for collision that I can see is from the first
>>>>>>> point:
>>>>>>>
>>>>>>> - private headers -> <dir>_<filename>_H
>>>>>>
>>>>>> I don't think this is the only possibility of collisions. The <subdir>_<filename>
>>>>>> parts can similarly cause problems if either of the two involved names contains
>>>>>> e.g. a dash (which would need converting to an underscore) or an underscore. To
>>>>>> avoid this, the name separators (slashes in the actual file names) there may need
>>>>>> representing by double underscores.
>>>>>
>>>>> I am OK with you two underscores as name separator (slashes in the
>>>>> actual file names). Would you do it for all levels like this?
>>>>>
>>>>> - arch/arm/arm64/lib/something.h -> ARM__ARM64__LIB__SOMETHING_H
>>>>> - arch/arm/arm32/lib/something.h -> ARM__ARM32__LIB__SOMETHING_H
>>>>> - arch/x86/lib/something.h -> X86__LIB__SOMETHING_H
>>>>>
>>>>>
>>>>> I think it is better than the below:
>>>>>
>>>>> - arch/arm/arm64/lib/something.h -> ARM_ARM64__LIB__SOMETHING_H
>>>>> - arch/arm/arm32/lib/something.h -> ARM_ARM32__LIB__SOMETHING_H
>>>>> - arch/x86/lib/something.h -> X86_LIB__SOMETHING_H
>>>>
>>>> Hmm, maybe it's indeed better to do it entirely uniformly then.
>>>
>>>
>>> Do we have agreement on the naming convention then? 
>>>
>>>
>>> - private headers -> <dir>__<filename>__H
>>>     - arch/arm/arm64/lib/something.h -> ARM__ARM64__LIB__SOMETHING_H
>>>     - arch/arm/arm32/lib/something.h -> ARM__ARM32__LIB__SOMETHING_H
>>>     - arch/x86/lib/something.h -> X86__LIB__SOMETHING_H
>>>
>>> - asm-generic headers -> ASM_GENERIC_<filename>_H
>>>     - include/asm-generic/percpu.h -> ASM_GENERIC_X86_PERCPU_H
>>>
>>> - arch/<architecture>/include/asm/<subdir>/<filename>.h -> ASM_<architecture>_<subdir>_<filename>_H
>>>     - arch/x86/include/asm/domain.h -> ASM_X86_DOMAIN_H
>>>
>>> - include/xen -> XEN_<filename>_H
>>>     - include/xen/percpu.h -> XEN_PERCPU_H
>>>
>>>
>>> Or do you prefer the double underscore __  in all cases?
>>
>> It's not so much prefer, but a requirement if we want to be future proof.
>> Even for ASM_GENERIC_* that'll be needed, as your outline above simply
>> doesn't mention the (future) case of there being subdir-s there (see how
>> Linux already has some). Imo the question doesn't even arise for XEN_*,
>> as xen/ has subdir-s already.
> 
> OK. So it becomes:
> 
> - private headers -> <dir>__<filename>_H
>     - arch/arm/arm64/lib/something.h -> ARM__ARM64__LIB__SOMETHING_H
>     - arch/arm/arm32/lib/something.h -> ARM__ARM32__LIB__SOMETHING_H
>     - arch/x86/lib/something.h -> X86__LIB__SOMETHING_H
> 
> - asm-generic headers -> ASM_GENERIC__<filename>_H
>     - include/asm-generic/percpu.h -> ASM_GENERIC__X86__PERCPU_H

Nit: There's still a stray _X86_ in here.

Jan

> - arch/<architecture>/include/asm/<subdir>/<filename>.h -> ASM__<architecture>__<subdir>__<filename>_H
>     - arch/x86/include/asm/domain.h -> ASM__X86__DOMAIN_H
> 
> - include/xen -> XEN__<filename>_H
>     - include/xen/percpu.h -> XEN__PERCPU_H
> 
> If we have found agreement then Alessandro could send an update

Re: [RFC PATCH 17/17] CODING_STYLE: Add a section on header guards naming conventions

[Stefano Stabellini]

On Fri, 19 Jul 2024, Jan Beulich wrote:
> On 19.07.2024 00:01, Stefano Stabellini wrote:
> > On Thu, 18 Jul 2024, Jan Beulich wrote:
> >> On 18.07.2024 01:02, Stefano Stabellini wrote:
> >>> On Wed, 17 Jul 2024, Jan Beulich wrote:
> >>>> On 17.07.2024 02:20, Stefano Stabellini wrote:
> >>>>> On Tue, 16 Jul 2024, Jan Beulich wrote:
> >>>>>> On 16.07.2024 02:43, Stefano Stabellini wrote:
> >>>>>>> On Mon, 15 Jul 2024, Jan Beulich wrote:
> >>>>>>>> On 13.07.2024 00:38, Stefano Stabellini wrote:
> >>>>>>>>> On Wed, 3 Jul 2024, Jan Beulich wrote:
> >>>>>>>>>> I further have to note that, as indicated during the earlier discussion,
> >>>>>>>>>> I still cannot see how occasional ambiguity is going to be dealt with.
> >>>>>>>>>> IOW from the rules above two different headers could still end up with
> >>>>>>>>>> the same guard identifier.
> >>>>>>>>>
> >>>>>>>>> Maybe something like this?
> >>>>>>>>>
> >>>>>>>>> "In the event of naming collisions, exceptions to the coding style may
> >>>>>>>>> be made at the discretion of the contributor and maintainers."
> >>>>>>>>
> >>>>>>>> Hmm, maybe I wasn't clear enough then. My take is that the scheme should
> >>>>>>>> simply not allow for possible collisions. Neither the contributor nor the
> >>>>>>>> reviewer may spot such a collision, and it may therefore take until the
> >>>>>>>> first full scan that one is actually noticed. Which I consider too late
> >>>>>>>> in the process, even if we already were at the point where commits were
> >>>>>>>> checked pre-push.
> >>>>>>>
> >>>>>>> Looking at the proposal, copy/pasted here for convenience:
> >>>>>>>
> >>>>>>> - private headers -> <dir>_<filename>_H
> >>>>>>> - asm-generic headers -> ASM_GENERIC_<filename>_H
> >>>>>>>     - #ifndef ASM_GENERIC_X86_PERCPU_H
> >>>>>>>       #define ASM_GENERIC_X86_PERCPU_H
> >>>>>>>       //...
> >>>>>>>       #endif /* ASM_GENERIC_X86_PERCPU_H */
> >>>>>>> - arch/<architecture>/include/asm/<subdir>/<filename>.h -> ASM_<architecture>_<subdir>_<filename>_H
> >>>>>>>     - #ifndef ASM_X86_DOMAIN_H
> >>>>>>>       #define ASM_X86_DOMAIN_H
> >>>>>>>       //...
> >>>>>>>       #endif /* ASM_X86_DOMAIN_H */
> >>>>>>> - xen/include/xen/<filename>.h -> XEN_<filename>_H
> >>>>>>> - xen/include/xen/<subdir>/<filename>.h -> XEN_<subdir>_<filename>_H
> >>>>>>>
> >>>>>>>
> >>>>>>> The only possibility for collision that I can see is from the first
> >>>>>>> point:
> >>>>>>>
> >>>>>>> - private headers -> <dir>_<filename>_H
> >>>>>>
> >>>>>> I don't think this is the only possibility of collisions. The <subdir>_<filename>
> >>>>>> parts can similarly cause problems if either of the two involved names contains
> >>>>>> e.g. a dash (which would need converting to an underscore) or an underscore. To
> >>>>>> avoid this, the name separators (slashes in the actual file names) there may need
> >>>>>> representing by double underscores.
> >>>>>
> >>>>> I am OK with you two underscores as name separator (slashes in the
> >>>>> actual file names). Would you do it for all levels like this?
> >>>>>
> >>>>> - arch/arm/arm64/lib/something.h -> ARM__ARM64__LIB__SOMETHING_H
> >>>>> - arch/arm/arm32/lib/something.h -> ARM__ARM32__LIB__SOMETHING_H
> >>>>> - arch/x86/lib/something.h -> X86__LIB__SOMETHING_H
> >>>>>
> >>>>>
> >>>>> I think it is better than the below:
> >>>>>
> >>>>> - arch/arm/arm64/lib/something.h -> ARM_ARM64__LIB__SOMETHING_H
> >>>>> - arch/arm/arm32/lib/something.h -> ARM_ARM32__LIB__SOMETHING_H
> >>>>> - arch/x86/lib/something.h -> X86_LIB__SOMETHING_H
> >>>>
> >>>> Hmm, maybe it's indeed better to do it entirely uniformly then.
> >>>
> >>>
> >>> Do we have agreement on the naming convention then? 
> >>>
> >>>
> >>> - private headers -> <dir>__<filename>__H
> >>>     - arch/arm/arm64/lib/something.h -> ARM__ARM64__LIB__SOMETHING_H
> >>>     - arch/arm/arm32/lib/something.h -> ARM__ARM32__LIB__SOMETHING_H
> >>>     - arch/x86/lib/something.h -> X86__LIB__SOMETHING_H
> >>>
> >>> - asm-generic headers -> ASM_GENERIC_<filename>_H
> >>>     - include/asm-generic/percpu.h -> ASM_GENERIC_X86_PERCPU_H
> >>>
> >>> - arch/<architecture>/include/asm/<subdir>/<filename>.h -> ASM_<architecture>_<subdir>_<filename>_H
> >>>     - arch/x86/include/asm/domain.h -> ASM_X86_DOMAIN_H
> >>>
> >>> - include/xen -> XEN_<filename>_H
> >>>     - include/xen/percpu.h -> XEN_PERCPU_H
> >>>
> >>>
> >>> Or do you prefer the double underscore __  in all cases?
> >>
> >> It's not so much prefer, but a requirement if we want to be future proof.
> >> Even for ASM_GENERIC_* that'll be needed, as your outline above simply
> >> doesn't mention the (future) case of there being subdir-s there (see how
> >> Linux already has some). Imo the question doesn't even arise for XEN_*,
> >> as xen/ has subdir-s already.
> > 
> > OK. So it becomes:
> > 
> > - private headers -> <dir>__<filename>_H
> >     - arch/arm/arm64/lib/something.h -> ARM__ARM64__LIB__SOMETHING_H
> >     - arch/arm/arm32/lib/something.h -> ARM__ARM32__LIB__SOMETHING_H
> >     - arch/x86/lib/something.h -> X86__LIB__SOMETHING_H
> > 
> > - asm-generic headers -> ASM_GENERIC__<filename>_H
> >     - include/asm-generic/percpu.h -> ASM_GENERIC__X86__PERCPU_H
> 
> Nit: There's still a stray _X86_ in here.
 
yes, good point.

Alessandro, let us know if we are good to go ahead or if we are missing
anything.


> > - arch/<architecture>/include/asm/<subdir>/<filename>.h -> ASM__<architecture>__<subdir>__<filename>_H
> >     - arch/x86/include/asm/domain.h -> ASM__X86__DOMAIN_H
> > 
> > - include/xen -> XEN__<filename>_H
> >     - include/xen/percpu.h -> XEN__PERCPU_H
> > 
> > If we have found agreement then Alessandro could send an update
> 

Re: [RFC PATCH 17/17] CODING_STYLE: Add a section on header guards naming conventions

[Alessandro Zucchelli]

On 2024-07-19 17:21, Stefano Stabellini wrote:
> On Fri, 19 Jul 2024, Jan Beulich wrote:
>> On 19.07.2024 00:01, Stefano Stabellini wrote:
>> > On Thu, 18 Jul 2024, Jan Beulich wrote:
>> >> On 18.07.2024 01:02, Stefano Stabellini wrote:
>> >>> On Wed, 17 Jul 2024, Jan Beulich wrote:
>> >>>> On 17.07.2024 02:20, Stefano Stabellini wrote:
>> >>>>> On Tue, 16 Jul 2024, Jan Beulich wrote:
>> >>>>>> On 16.07.2024 02:43, Stefano Stabellini wrote:
>> >>>>>>> On Mon, 15 Jul 2024, Jan Beulich wrote:
>> >>>>>>>> On 13.07.2024 00:38, Stefano Stabellini wrote:
>> >>>>>>>>> On Wed, 3 Jul 2024, Jan Beulich wrote:
>> >>>>>>>>>> I further have to note that, as indicated during the earlier discussion,
>> >>>>>>>>>> I still cannot see how occasional ambiguity is going to be dealt with.
>> >>>>>>>>>> IOW from the rules above two different headers could still end up with
>> >>>>>>>>>> the same guard identifier.
>> >>>>>>>>>
>> >>>>>>>>> Maybe something like this?
>> >>>>>>>>>
>> >>>>>>>>> "In the event of naming collisions, exceptions to the coding style may
>> >>>>>>>>> be made at the discretion of the contributor and maintainers."
>> >>>>>>>>
>> >>>>>>>> Hmm, maybe I wasn't clear enough then. My take is that the scheme should
>> >>>>>>>> simply not allow for possible collisions. Neither the contributor nor the
>> >>>>>>>> reviewer may spot such a collision, and it may therefore take until the
>> >>>>>>>> first full scan that one is actually noticed. Which I consider too late
>> >>>>>>>> in the process, even if we already were at the point where commits were
>> >>>>>>>> checked pre-push.
>> >>>>>>>
>> >>>>>>> Looking at the proposal, copy/pasted here for convenience:
>> >>>>>>>
>> >>>>>>> - private headers -> <dir>_<filename>_H
>> >>>>>>> - asm-generic headers -> ASM_GENERIC_<filename>_H
>> >>>>>>>     - #ifndef ASM_GENERIC_X86_PERCPU_H
>> >>>>>>>       #define ASM_GENERIC_X86_PERCPU_H
>> >>>>>>>       //...
>> >>>>>>>       #endif /* ASM_GENERIC_X86_PERCPU_H */
>> >>>>>>> - arch/<architecture>/include/asm/<subdir>/<filename>.h -> ASM_<architecture>_<subdir>_<filename>_H
>> >>>>>>>     - #ifndef ASM_X86_DOMAIN_H
>> >>>>>>>       #define ASM_X86_DOMAIN_H
>> >>>>>>>       //...
>> >>>>>>>       #endif /* ASM_X86_DOMAIN_H */
>> >>>>>>> - xen/include/xen/<filename>.h -> XEN_<filename>_H
>> >>>>>>> - xen/include/xen/<subdir>/<filename>.h -> XEN_<subdir>_<filename>_H
>> >>>>>>>
>> >>>>>>>
>> >>>>>>> The only possibility for collision that I can see is from the first
>> >>>>>>> point:
>> >>>>>>>
>> >>>>>>> - private headers -> <dir>_<filename>_H
>> >>>>>>
>> >>>>>> I don't think this is the only possibility of collisions. The <subdir>_<filename>
>> >>>>>> parts can similarly cause problems if either of the two involved names contains
>> >>>>>> e.g. a dash (which would need converting to an underscore) or an underscore. To
>> >>>>>> avoid this, the name separators (slashes in the actual file names) there may need
>> >>>>>> representing by double underscores.
>> >>>>>
>> >>>>> I am OK with you two underscores as name separator (slashes in the
>> >>>>> actual file names). Would you do it for all levels like this?
>> >>>>>
>> >>>>> - arch/arm/arm64/lib/something.h -> ARM__ARM64__LIB__SOMETHING_H
>> >>>>> - arch/arm/arm32/lib/something.h -> ARM__ARM32__LIB__SOMETHING_H
>> >>>>> - arch/x86/lib/something.h -> X86__LIB__SOMETHING_H
>> >>>>>
>> >>>>>
>> >>>>> I think it is better than the below:
>> >>>>>
>> >>>>> - arch/arm/arm64/lib/something.h -> ARM_ARM64__LIB__SOMETHING_H
>> >>>>> - arch/arm/arm32/lib/something.h -> ARM_ARM32__LIB__SOMETHING_H
>> >>>>> - arch/x86/lib/something.h -> X86_LIB__SOMETHING_H
>> >>>>
>> >>>> Hmm, maybe it's indeed better to do it entirely uniformly then.
>> >>>
>> >>>
>> >>> Do we have agreement on the naming convention then?
>> >>>
>> >>>
>> >>> - private headers -> <dir>__<filename>__H
>> >>>     - arch/arm/arm64/lib/something.h -> ARM__ARM64__LIB__SOMETHING_H
>> >>>     - arch/arm/arm32/lib/something.h -> ARM__ARM32__LIB__SOMETHING_H
>> >>>     - arch/x86/lib/something.h -> X86__LIB__SOMETHING_H
>> >>>
>> >>> - asm-generic headers -> ASM_GENERIC_<filename>_H
>> >>>     - include/asm-generic/percpu.h -> ASM_GENERIC_X86_PERCPU_H
>> >>>
>> >>> - arch/<architecture>/include/asm/<subdir>/<filename>.h -> ASM_<architecture>_<subdir>_<filename>_H
>> >>>     - arch/x86/include/asm/domain.h -> ASM_X86_DOMAIN_H
>> >>>
>> >>> - include/xen -> XEN_<filename>_H
>> >>>     - include/xen/percpu.h -> XEN_PERCPU_H
>> >>>
>> >>>
>> >>> Or do you prefer the double underscore __  in all cases?
>> >>
>> >> It's not so much prefer, but a requirement if we want to be future proof.
>> >> Even for ASM_GENERIC_* that'll be needed, as your outline above simply
>> >> doesn't mention the (future) case of there being subdir-s there (see how
>> >> Linux already has some). Imo the question doesn't even arise for XEN_*,
>> >> as xen/ has subdir-s already.
>> >
>> > OK. So it becomes:
>> >
>> > - private headers -> <dir>__<filename>_H
>> >     - arch/arm/arm64/lib/something.h -> ARM__ARM64__LIB__SOMETHING_H
>> >     - arch/arm/arm32/lib/something.h -> ARM__ARM32__LIB__SOMETHING_H
>> >     - arch/x86/lib/something.h -> X86__LIB__SOMETHING_H
>> >
>> > - asm-generic headers -> ASM_GENERIC__<filename>_H
>> >     - include/asm-generic/percpu.h -> ASM_GENERIC__X86__PERCPU_H
>> 
>> Nit: There's still a stray _X86_ in here.
> 
> yes, good point.
> 
> Alessandro, let us know if we are good to go ahead or if we are missing
> anything.

I think we are good right now, I will provide the patch series v5 with 
all the
fixes and inclusion guards renamings soon.

>> > - arch/<architecture>/include/asm/<subdir>/<filename>.h -> ASM__<architecture>__<subdir>__<filename>_H
>> >     - arch/x86/include/asm/domain.h -> ASM__X86__DOMAIN_H
>> >
>> > - include/xen -> XEN__<filename>_H
>> >     - include/xen/percpu.h -> XEN__PERCPU_H
>> >
>> > If we have found agreement then Alessandro could send an update
>> 

-- 
Alessandro Zucchelli, B.Sc.

Software Engineer, BUGSENG (https://bugseng.com)