[XEN PATCH v3 00/16] xen: address violation of MISRA C:2012 Directive 4.10

[XEN PATCH v3 00/16] xen: address violation of MISRA C:2012 Directive 4.10

[Simone Ballarin]

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

As stated in v2, the following naming convention has been estabilished:
- arch/.../include/asm/ headers -> ASM_<filename>_H
- private headers -> <dir>_<filename>_H
- asm-generic headers -> ASM_GENERIC_<filename>_H

Since there would have been conflicting guards between architectures (which is a violation
of the directive), there has been a need for ASM headers to specify  if the inclusion guard
referred to ARM or X86. Hence it has been decided to adopt instead:
- arch/<architecture>/include/asm/<subdir>/<filename>.h -> ASM_<architecture>_<subdir>_<filename>_H

The subdir used is the smallest possible to avoid collisions. For example, it has been
observed that in "xen/arch/arm/include/asm/arm32" and "xen/arch/arm/include/asm/arm64" there
are plenty of header files with the same name, hence  _ARMxx_ was added as subdirectory.

There has been a need to define a standard for generated headers too:
 - include/generated/<subdir>/<filename>.h-> GENERATED_<subdir>_<filename>_H
 - arch/<architecture>/include/generated/asm/<filename>.h-> <arch>_GENERATED_ASM_<name>_H

To summarize, 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

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

Maria Celeste Cesario (6):
  xen/arm: address violations of MISRA C:2012 Directive 4.10
  xen: address violations of MISRA C:2012 Directive 4.10
  xen: add deviations for MISRA C.2012 Directive 4.10
  xen/x86: address violations of MISRA C:2012 Directive 4.10
  x86/mtrr: address violations of MISRA C:2012 Directive 4.10
  xen/lz4: address violations of MISRA C:2012 Directive 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

 .../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                         | 10 +++--
 xen/arch/x86/cpu/cpu.h                        |  5 +++
 xen/arch/x86/cpu/mtrr/mtrr.h                  |  4 ++
 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                                  |  6 ++-
 xen/common/decompress.h                       |  5 +++
 xen/common/efi/efi.h                          |  5 +++
 xen/common/event_channel.h                    |  5 +++
 xen/common/lz4/defs.h                         |  5 +++
 xen/include/Makefile                          | 12 ++++--
 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              | 16 +++++++-
 48 files changed, 233 insertions(+), 78 deletions(-)

-- 
2.34.1

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

[Simone Ballarin]

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

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

This patch adds deviation cooments for headers that avoid guards.

Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>

---
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                        | 9 +++++++++
 xen/include/public/arch-x86/cpufeatureset.h | 1 +
 xen/include/public/errno.h                  | 1 +
 3 files changed, 11 insertions(+)

diff --git a/docs/misra/safe.json b/docs/misra/safe.json
index 952324f85c..e98956d604 100644
--- a/docs/misra/safe.json
+++ b/docs/misra/safe.json
@@ -28,6 +28,15 @@
         },
         {
             "id": "SAF-3-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-4-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 0374cec3a2..f78a461d93 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-3-safe omitted inclusion guard */
 #ifndef XEN_CPUFEATURE
 
 /*
diff --git a/xen/include/public/errno.h b/xen/include/public/errno.h
index 5a78a7607c..28e064b67f 100644
--- a/xen/include/public/errno.h
+++ b/xen/include/public/errno.h
@@ -17,6 +17,7 @@
  * will unilaterally #undef XEN_ERRNO().
  */
 
+/* SAF-3-safe omitted inclusion guard */
 #ifndef XEN_ERRNO
 
 /*
-- 
2.34.1

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

[Jan Beulich]

On 11.03.2024 09:59, Simone Ballarin wrote:
> Some headers, under specific circumstances (documented in a comment at
> the beginning of the file), explicitly avoid inclusion guards: the caller
> is responsible for including them correctly.
> 
> These files are not supposed to comply with Directive 4.10:
> "Precautions shall be taken in order to prevent the contents of a header
> file being included more than once"
> 
> This patch adds deviation cooments for headers that avoid guards.
> 
> Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>
> 
> ---
> 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                        | 9 +++++++++
>  xen/include/public/arch-x86/cpufeatureset.h | 1 +
>  xen/include/public/errno.h                  | 1 +
>  3 files changed, 11 insertions(+)

I understand something wants doing, but having such comments appear in public
headers feels wrong to me. I'm afraid I have no good alternative suggestion.

Jan

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

[Stefano Stabellini]

On Mon, 11 Mar 2024, Jan Beulich wrote:
> On 11.03.2024 09:59, Simone Ballarin wrote:
> > Some headers, under specific circumstances (documented in a comment at
> > the beginning of the file), explicitly avoid inclusion guards: the caller
> > is responsible for including them correctly.
> > 
> > These files are not supposed to comply with Directive 4.10:
> > "Precautions shall be taken in order to prevent the contents of a header
> > file being included more than once"
> > 
> > This patch adds deviation cooments for headers that avoid guards.
> > 
> > Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>
> > 
> > ---
> > 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                        | 9 +++++++++
> >  xen/include/public/arch-x86/cpufeatureset.h | 1 +
> >  xen/include/public/errno.h                  | 1 +
> >  3 files changed, 11 insertions(+)
> 
> I understand something wants doing, but having such comments appear in public
> headers feels wrong to me. I'm afraid I have no good alternative suggestion.

Given that in both cases there is very nice explanation on how to use
the headers as an in-code comment just above, could we embed the
SAF-3-safe marker in the existing comment?

If not, I think we should go with this patch as is (I don't think it is
worth my, your, and Simone's time to look for alternatives).

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

[Jan Beulich]

On 14.03.2024 23:55, Stefano Stabellini wrote:
> On Mon, 11 Mar 2024, Jan Beulich wrote:
>> On 11.03.2024 09:59, Simone Ballarin wrote:
>>> Some headers, under specific circumstances (documented in a comment at
>>> the beginning of the file), explicitly avoid inclusion guards: the caller
>>> is responsible for including them correctly.
>>>
>>> These files are not supposed to comply with Directive 4.10:
>>> "Precautions shall be taken in order to prevent the contents of a header
>>> file being included more than once"
>>>
>>> This patch adds deviation cooments for headers that avoid guards.
>>>
>>> Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>
>>>
>>> ---
>>> 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                        | 9 +++++++++
>>>  xen/include/public/arch-x86/cpufeatureset.h | 1 +
>>>  xen/include/public/errno.h                  | 1 +
>>>  3 files changed, 11 insertions(+)
>>
>> I understand something wants doing, but having such comments appear in public
>> headers feels wrong to me. I'm afraid I have no good alternative suggestion.
> 
> Given that in both cases there is very nice explanation on how to use
> the headers as an in-code comment just above, could we embed the
> SAF-3-safe marker in the existing comment?

I'm afraid that won't address my remark, and I'm further afraid this would
then render the SAF part of the comment ineffectual.

> If not, I think we should go with this patch as is (I don't think it is
> worth my, your, and Simone's time to look for alternatives).

Easy alternative: Simply leave public headers alone.

Jan

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

[Stefano Stabellini]

On Fri, 15 Mar 2024, Jan Beulich wrote:
> On 14.03.2024 23:55, Stefano Stabellini wrote:
> > On Mon, 11 Mar 2024, Jan Beulich wrote:
> >> On 11.03.2024 09:59, Simone Ballarin wrote:
> >>> Some headers, under specific circumstances (documented in a comment at
> >>> the beginning of the file), explicitly avoid inclusion guards: the caller
> >>> is responsible for including them correctly.
> >>>
> >>> These files are not supposed to comply with Directive 4.10:
> >>> "Precautions shall be taken in order to prevent the contents of a header
> >>> file being included more than once"
> >>>
> >>> This patch adds deviation cooments for headers that avoid guards.
> >>>
> >>> Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>
> >>>
> >>> ---
> >>> 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                        | 9 +++++++++
> >>>  xen/include/public/arch-x86/cpufeatureset.h | 1 +
> >>>  xen/include/public/errno.h                  | 1 +
> >>>  3 files changed, 11 insertions(+)
> >>
> >> I understand something wants doing, but having such comments appear in public
> >> headers feels wrong to me. I'm afraid I have no good alternative suggestion.
> > 
> > Given that in both cases there is very nice explanation on how to use
> > the headers as an in-code comment just above, could we embed the
> > SAF-3-safe marker in the existing comment?
> 
> I'm afraid that won't address my remark, and I'm further afraid this would
> then render the SAF part of the comment ineffectual.
> 
> > If not, I think we should go with this patch as is (I don't think it is
> > worth my, your, and Simone's time to look for alternatives).
> 
> Easy alternative: Simply leave public headers alone.

I don't think it is a good idea to skip MISRA on public headers as they
are used as part of the Xen build. I think adding the one line SAF
comment is better.

We still have strange tags like the following in public headers:

 * `incontents 150 evtchn Event Channels

What does `incontents 150 even mean? If I grep for "incontents" I cannot
find any explanation or definition. The SAF comment is easily greppable
and with a clear definition. I am in favor of adding them (and removing
"incontents" but that's a discussion for another day.) 

[XEN PATCH v3 02/16] misra: modify deviations for empty and generated headers

[Simone Ballarin]

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>

---
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                             | 13 ++++++++++---
 xen/arch/arm/efi/runtime.h                       |  1 +
 xen/include/Makefile                             |  2 +-
 4 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl b/automation/eclair_analysis/ECLAIR/deviations.ecl
index 9ac3ee4dfd..039ffaf52a 100644
--- a/automation/eclair_analysis/ECLAIR/deviations.ecl
+++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
@@ -63,13 +63,6 @@ they are not instances of commented-out code."
 -config=MC3R1.D4.3,reports+={disapplied,"!(any_area(any_loc(file(^xen/arch/arm/arm64/.*$))))"}
 -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 e98956d604..d2489379a7 100644
--- a/docs/misra/safe.json
+++ b/docs/misra/safe.json
@@ -33,10 +33,17 @@
             },
             "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-4-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-5-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..732bf4a18c 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-4-safe empty header */
diff --git a/xen/include/Makefile b/xen/include/Makefile
index 2e61b50139..a77c9ffb7e 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-4-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

Re: [XEN PATCH v3 02/16] misra: modify deviations for empty and generated headers

[Stefano Stabellini]

On Mon, 11 Mar 2024, Simone Ballarin wrote:
> 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>

Nice!

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

[XEN PATCH v3 03/16] misra: add deviations for direct inclusion guards

[Simone Ballarin]

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.

Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>

---
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 d2489379a7..363c9e21b0 100644
--- a/docs/misra/safe.json
+++ b/docs/misra/safe.json
@@ -44,6 +44,14 @@
         },
         {
             "id": "SAF-5-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-6-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..b754475cd0 100644
--- a/xen/arch/arm/include/asm/hypercall.h
+++ b/xen/arch/arm/include/asm/hypercall.h
@@ -1,3 +1,4 @@
+/* SAF-5-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..1a51ae02bf 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-5-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: [XEN PATCH v3 03/16] misra: add deviations for direct inclusion guards

[Jan Beulich]

On 11.03.2024 09:59, Simone Ballarin wrote:
> 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.
> 
> Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>
> 
> ---
> 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(+)

What about asm-generic/hypercall.h?

> --- a/xen/arch/arm/include/asm/hypercall.h
> +++ b/xen/arch/arm/include/asm/hypercall.h
> @@ -1,3 +1,4 @@
> +/* SAF-5-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-5-safe direct inclusion guard before */
>  #ifndef __XEN_HYPERCALL_H__
>  #error "asm/hypercall.h should not be included directly - include xen/hypercall.h instead"
>  #endif

Iirc it was said that this way checking for correct guards is suppressed
altogether in Eclair, which is not what we want. Can you clarify this,
please?

Jan

Re: [XEN PATCH v3 03/16] misra: add deviations for direct inclusion guards

[Simone Ballarin]

On 11/03/24 11:08, Jan Beulich wrote:
> On 11.03.2024 09:59, Simone Ballarin wrote:
>> 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.
>>
>> Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>
>>
>> ---
>> 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(+)
> 
> What about asm-generic/hypercall.h?

Apparently the file is not part of the analysed build.

> 
>> --- a/xen/arch/arm/include/asm/hypercall.h
>> +++ b/xen/arch/arm/include/asm/hypercall.h
>> @@ -1,3 +1,4 @@
>> +/* SAF-5-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-5-safe direct inclusion guard before */
>>   #ifndef __XEN_HYPERCALL_H__
>>   #error "asm/hypercall.h should not be included directly - include xen/hypercall.h instead"
>>   #endif
> 
> Iirc it was said that this way checking for correct guards is suppressed
> altogether in Eclair, which is not what we want. Can you clarify this,
> please?
> 

My first change was moving this check inside the guard.
You commented my patch saying that this would be an error because someone can
include it directly if it has already been included indirectly.
I replied telling that this was the case also before the change.
You agreed with me, and we decided that the correct thing would be fixing the
check and not apply my temporary change to address the finding.

Considering that the code should be amended, a SAF deviation seems to me
the most appropriate way for suppressing these findings.

> Jan
> 

-- 
Simone Ballarin, M.Sc.

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

Re: [XEN PATCH v3 03/16] misra: add deviations for direct inclusion guards

[Jan Beulich]

On 11.03.2024 13:00, Simone Ballarin wrote:
> On 11/03/24 11:08, Jan Beulich wrote:
>> On 11.03.2024 09:59, Simone Ballarin wrote:
>>> --- a/xen/arch/arm/include/asm/hypercall.h
>>> +++ b/xen/arch/arm/include/asm/hypercall.h
>>> @@ -1,3 +1,4 @@
>>> +/* SAF-5-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-5-safe direct inclusion guard before */
>>>   #ifndef __XEN_HYPERCALL_H__
>>>   #error "asm/hypercall.h should not be included directly - include xen/hypercall.h instead"
>>>   #endif
>>
>> Iirc it was said that this way checking for correct guards is suppressed
>> altogether in Eclair, which is not what we want. Can you clarify this,
>> please?
>>
> 
> My first change was moving this check inside the guard.
> You commented my patch saying that this would be an error because someone can
> include it directly if it has already been included indirectly.
> I replied telling that this was the case also before the change.
> You agreed with me, and we decided that the correct thing would be fixing the
> check and not apply my temporary change to address the finding.
> 
> Considering that the code should be amended, a SAF deviation seems to me
> the most appropriate way for suppressing these findings.

Since I don't feel your reply addresses my question, asking differently: With
your change in place, will failure to have proper guards (later) in these
headers still be reported by Eclair?

Jan

Re: [XEN PATCH v3 03/16] misra: add deviations for direct inclusion guards

[Simone Ballarin]

On 11/03/24 14:56, Jan Beulich wrote:
> On 11.03.2024 13:00, Simone Ballarin wrote:
>> On 11/03/24 11:08, Jan Beulich wrote:
>>> On 11.03.2024 09:59, Simone Ballarin wrote:
>>>> --- a/xen/arch/arm/include/asm/hypercall.h
>>>> +++ b/xen/arch/arm/include/asm/hypercall.h
>>>> @@ -1,3 +1,4 @@
>>>> +/* SAF-5-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-5-safe direct inclusion guard before */
>>>>    #ifndef __XEN_HYPERCALL_H__
>>>>    #error "asm/hypercall.h should not be included directly - include xen/hypercall.h instead"
>>>>    #endif
>>>
>>> Iirc it was said that this way checking for correct guards is suppressed
>>> altogether in Eclair, which is not what we want. Can you clarify this,
>>> please?
>>>
>>
>> My first change was moving this check inside the guard.
>> You commented my patch saying that this would be an error because someone can
>> include it directly if it has already been included indirectly.
>> I replied telling that this was the case also before the change.
>> You agreed with me, and we decided that the correct thing would be fixing the
>> check and not apply my temporary change to address the finding.
>>
>> Considering that the code should be amended, a SAF deviation seems to me
>> the most appropriate way for suppressing these findings.
> 
> Since I don't feel your reply addresses my question, asking differently: With
> your change in place, will failure to have proper guards (later) in these
> headers still be reported by Eclair?
> 
> Jan
> 

No, if you put something between the check and the guard,
no violation will be reported.


-- 
Simone Ballarin, M.Sc.

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

Re: [XEN PATCH v3 03/16] misra: add deviations for direct inclusion guards

[Stefano Stabellini]

On Mon, 11 Mar 2024, Simone Ballarin wrote:
> On 11/03/24 14:56, Jan Beulich wrote:
> > On 11.03.2024 13:00, Simone Ballarin wrote:
> > > On 11/03/24 11:08, Jan Beulich wrote:
> > > > On 11.03.2024 09:59, Simone Ballarin wrote:
> > > > > --- a/xen/arch/arm/include/asm/hypercall.h
> > > > > +++ b/xen/arch/arm/include/asm/hypercall.h
> > > > > @@ -1,3 +1,4 @@
> > > > > +/* SAF-5-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-5-safe direct inclusion guard before */
> > > > >    #ifndef __XEN_HYPERCALL_H__
> > > > >    #error "asm/hypercall.h should not be included directly - include
> > > > > xen/hypercall.h instead"
> > > > >    #endif
> > > > 
> > > > Iirc it was said that this way checking for correct guards is suppressed
> > > > altogether in Eclair, which is not what we want. Can you clarify this,
> > > > please?
> > > > 
> > > 
> > > My first change was moving this check inside the guard.
> > > You commented my patch saying that this would be an error because someone
> > > can
> > > include it directly if it has already been included indirectly.
> > > I replied telling that this was the case also before the change.
> > > You agreed with me, and we decided that the correct thing would be fixing
> > > the
> > > check and not apply my temporary change to address the finding.
> > > 
> > > Considering that the code should be amended, a SAF deviation seems to me
> > > the most appropriate way for suppressing these findings.
> > 
> > Since I don't feel your reply addresses my question, asking differently:
> > With
> > your change in place, will failure to have proper guards (later) in these
> > headers still be reported by Eclair?
> > 
> > Jan
> > 
> 
> No, if you put something between the check and the guard,
> no violation will be reported.

From this email exchange I cannot under if Jan is OK with this patch or
not.

Re: [XEN PATCH v3 03/16] misra: add deviations for direct inclusion guards

[Jan Beulich]

On 14.03.2024 23:59, Stefano Stabellini wrote:
> On Mon, 11 Mar 2024, Simone Ballarin wrote:
>> On 11/03/24 14:56, Jan Beulich wrote:
>>> On 11.03.2024 13:00, Simone Ballarin wrote:
>>>> On 11/03/24 11:08, Jan Beulich wrote:
>>>>> On 11.03.2024 09:59, Simone Ballarin wrote:
>>>>>> --- a/xen/arch/arm/include/asm/hypercall.h
>>>>>> +++ b/xen/arch/arm/include/asm/hypercall.h
>>>>>> @@ -1,3 +1,4 @@
>>>>>> +/* SAF-5-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-5-safe direct inclusion guard before */
>>>>>>    #ifndef __XEN_HYPERCALL_H__
>>>>>>    #error "asm/hypercall.h should not be included directly - include
>>>>>> xen/hypercall.h instead"
>>>>>>    #endif
>>>>>
>>>>> Iirc it was said that this way checking for correct guards is suppressed
>>>>> altogether in Eclair, which is not what we want. Can you clarify this,
>>>>> please?
>>>>>
>>>>
>>>> My first change was moving this check inside the guard.
>>>> You commented my patch saying that this would be an error because someone
>>>> can
>>>> include it directly if it has already been included indirectly.
>>>> I replied telling that this was the case also before the change.
>>>> You agreed with me, and we decided that the correct thing would be fixing
>>>> the
>>>> check and not apply my temporary change to address the finding.
>>>>
>>>> Considering that the code should be amended, a SAF deviation seems to me
>>>> the most appropriate way for suppressing these findings.
>>>
>>> Since I don't feel your reply addresses my question, asking differently:
>>> With
>>> your change in place, will failure to have proper guards (later) in these
>>> headers still be reported by Eclair?
>>
>> No, if you put something between the check and the guard,
>> no violation will be reported.
> 
> From this email exchange I cannot under if Jan is OK with this patch or
> not.

Whether I'm okay(ish) with the patch here depends on our position towards
the lost checking in Eclair mentioned above. To me it still looks relevant
that checking for a guard occurs, even if that isn't first in a file for
some specific reason.

Jan

Re: [XEN PATCH v3 03/16] misra: add deviations for direct inclusion guards

[Stefano Stabellini]

On Fri, 15 Mar 2024, Jan Beulich wrote:
> On 14.03.2024 23:59, Stefano Stabellini wrote:
> > On Mon, 11 Mar 2024, Simone Ballarin wrote:
> >> On 11/03/24 14:56, Jan Beulich wrote:
> >>> On 11.03.2024 13:00, Simone Ballarin wrote:
> >>>> On 11/03/24 11:08, Jan Beulich wrote:
> >>>>> On 11.03.2024 09:59, Simone Ballarin wrote:
> >>>>>> --- a/xen/arch/arm/include/asm/hypercall.h
> >>>>>> +++ b/xen/arch/arm/include/asm/hypercall.h
> >>>>>> @@ -1,3 +1,4 @@
> >>>>>> +/* SAF-5-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-5-safe direct inclusion guard before */
> >>>>>>    #ifndef __XEN_HYPERCALL_H__
> >>>>>>    #error "asm/hypercall.h should not be included directly - include
> >>>>>> xen/hypercall.h instead"
> >>>>>>    #endif
> >>>>>
> >>>>> Iirc it was said that this way checking for correct guards is suppressed
> >>>>> altogether in Eclair, which is not what we want. Can you clarify this,
> >>>>> please?
> >>>>>
> >>>>
> >>>> My first change was moving this check inside the guard.
> >>>> You commented my patch saying that this would be an error because someone
> >>>> can
> >>>> include it directly if it has already been included indirectly.
> >>>> I replied telling that this was the case also before the change.
> >>>> You agreed with me, and we decided that the correct thing would be fixing
> >>>> the
> >>>> check and not apply my temporary change to address the finding.
> >>>>
> >>>> Considering that the code should be amended, a SAF deviation seems to me
> >>>> the most appropriate way for suppressing these findings.
> >>>
> >>> Since I don't feel your reply addresses my question, asking differently:
> >>> With
> >>> your change in place, will failure to have proper guards (later) in these
> >>> headers still be reported by Eclair?
> >>
> >> No, if you put something between the check and the guard,
> >> no violation will be reported.
> > 
> > From this email exchange I cannot under if Jan is OK with this patch or
> > not.
> 
> Whether I'm okay(ish) with the patch here depends on our position towards
> the lost checking in Eclair mentioned above. To me it still looks relevant
> that checking for a guard occurs, even if that isn't first in a file for
> some specific reason.

More checking is better than less checking, but if we cannot find a
simple and actionable way forward on this violation, deviating it is
still a big improvement because it allows us to enable the ECLAIR Dir
4.10 checks in xen.git overall (which again goes back to more checking
is better than less checking). 

Do you have a simple alternative suggestion? If not, this is still an
improvement.

Re: [XEN PATCH v3 03/16] misra: add deviations for direct inclusion guards

[Jan Beulich]

On 16.03.2024 01:43, Stefano Stabellini wrote:
> On Fri, 15 Mar 2024, Jan Beulich wrote:
>> On 14.03.2024 23:59, Stefano Stabellini wrote:
>>> On Mon, 11 Mar 2024, Simone Ballarin wrote:
>>>> On 11/03/24 14:56, Jan Beulich wrote:
>>>>> On 11.03.2024 13:00, Simone Ballarin wrote:
>>>>>> On 11/03/24 11:08, Jan Beulich wrote:
>>>>>>> On 11.03.2024 09:59, Simone Ballarin wrote:
>>>>>>>> --- a/xen/arch/arm/include/asm/hypercall.h
>>>>>>>> +++ b/xen/arch/arm/include/asm/hypercall.h
>>>>>>>> @@ -1,3 +1,4 @@
>>>>>>>> +/* SAF-5-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-5-safe direct inclusion guard before */
>>>>>>>>    #ifndef __XEN_HYPERCALL_H__
>>>>>>>>    #error "asm/hypercall.h should not be included directly - include
>>>>>>>> xen/hypercall.h instead"
>>>>>>>>    #endif
>>>>>>>
>>>>>>> Iirc it was said that this way checking for correct guards is suppressed
>>>>>>> altogether in Eclair, which is not what we want. Can you clarify this,
>>>>>>> please?
>>>>>>>
>>>>>>
>>>>>> My first change was moving this check inside the guard.
>>>>>> You commented my patch saying that this would be an error because someone
>>>>>> can
>>>>>> include it directly if it has already been included indirectly.
>>>>>> I replied telling that this was the case also before the change.
>>>>>> You agreed with me, and we decided that the correct thing would be fixing
>>>>>> the
>>>>>> check and not apply my temporary change to address the finding.
>>>>>>
>>>>>> Considering that the code should be amended, a SAF deviation seems to me
>>>>>> the most appropriate way for suppressing these findings.
>>>>>
>>>>> Since I don't feel your reply addresses my question, asking differently:
>>>>> With
>>>>> your change in place, will failure to have proper guards (later) in these
>>>>> headers still be reported by Eclair?
>>>>
>>>> No, if you put something between the check and the guard,
>>>> no violation will be reported.
>>>
>>> From this email exchange I cannot under if Jan is OK with this patch or
>>> not.
>>
>> Whether I'm okay(ish) with the patch here depends on our position towards
>> the lost checking in Eclair mentioned above. To me it still looks relevant
>> that checking for a guard occurs, even if that isn't first in a file for
>> some specific reason.
> 
> More checking is better than less checking, but if we cannot find a
> simple and actionable way forward on this violation, deviating it is
> still a big improvement because it allows us to enable the ECLAIR Dir
> 4.10 checks in xen.git overall (which again goes back to more checking
> is better than less checking). 

You have a point here. I think though that at the very least the lost
checking opportunity wants calling out quite explicitly.

> Do you have a simple alternative suggestion? If not, this is still an
> improvement.

I don't know the inner workings of Eclair. Without that I'm afraid I'm not
in a position to make alternative suggestions.

Jan

Re: [XEN PATCH v3 03/16] misra: add deviations for direct inclusion guards

[Stefano Stabellini]

On Mon, 18 Mar 2024, Jan Beulich wrote:
> On 16.03.2024 01:43, Stefano Stabellini wrote:
> > On Fri, 15 Mar 2024, Jan Beulich wrote:
> >> On 14.03.2024 23:59, Stefano Stabellini wrote:
> >>> On Mon, 11 Mar 2024, Simone Ballarin wrote:
> >>>> On 11/03/24 14:56, Jan Beulich wrote:
> >>>>> On 11.03.2024 13:00, Simone Ballarin wrote:
> >>>>>> On 11/03/24 11:08, Jan Beulich wrote:
> >>>>>>> On 11.03.2024 09:59, Simone Ballarin wrote:
> >>>>>>>> --- a/xen/arch/arm/include/asm/hypercall.h
> >>>>>>>> +++ b/xen/arch/arm/include/asm/hypercall.h
> >>>>>>>> @@ -1,3 +1,4 @@
> >>>>>>>> +/* SAF-5-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-5-safe direct inclusion guard before */
> >>>>>>>>    #ifndef __XEN_HYPERCALL_H__
> >>>>>>>>    #error "asm/hypercall.h should not be included directly - include
> >>>>>>>> xen/hypercall.h instead"
> >>>>>>>>    #endif
> >>>>>>>
> >>>>>>> Iirc it was said that this way checking for correct guards is suppressed
> >>>>>>> altogether in Eclair, which is not what we want. Can you clarify this,
> >>>>>>> please?
> >>>>>>>
> >>>>>>
> >>>>>> My first change was moving this check inside the guard.
> >>>>>> You commented my patch saying that this would be an error because someone
> >>>>>> can
> >>>>>> include it directly if it has already been included indirectly.
> >>>>>> I replied telling that this was the case also before the change.
> >>>>>> You agreed with me, and we decided that the correct thing would be fixing
> >>>>>> the
> >>>>>> check and not apply my temporary change to address the finding.
> >>>>>>
> >>>>>> Considering that the code should be amended, a SAF deviation seems to me
> >>>>>> the most appropriate way for suppressing these findings.
> >>>>>
> >>>>> Since I don't feel your reply addresses my question, asking differently:
> >>>>> With
> >>>>> your change in place, will failure to have proper guards (later) in these
> >>>>> headers still be reported by Eclair?
> >>>>
> >>>> No, if you put something between the check and the guard,
> >>>> no violation will be reported.
> >>>
> >>> From this email exchange I cannot under if Jan is OK with this patch or
> >>> not.
> >>
> >> Whether I'm okay(ish) with the patch here depends on our position towards
> >> the lost checking in Eclair mentioned above. To me it still looks relevant
> >> that checking for a guard occurs, even if that isn't first in a file for
> >> some specific reason.
> > 
> > More checking is better than less checking, but if we cannot find a
> > simple and actionable way forward on this violation, deviating it is
> > still a big improvement because it allows us to enable the ECLAIR Dir
> > 4.10 checks in xen.git overall (which again goes back to more checking
> > is better than less checking). 
> 
> You have a point here. I think though that at the very least the lost
> checking opportunity wants calling out quite explicitly.

All right, then maybe this patch can go in with a clarification in the
commit message?

Something like:

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


> > Do you have a simple alternative suggestion? If not, this is still an
> > improvement.
> 
> I don't know the inner workings of Eclair. Without that I'm afraid I'm not
> in a position to make alternative suggestions.
 

Re: [XEN PATCH v3 03/16] misra: add deviations for direct inclusion guards

[Jan Beulich]

On 19.03.2024 04:34, Stefano Stabellini wrote:
> On Mon, 18 Mar 2024, Jan Beulich wrote:
>> On 16.03.2024 01:43, Stefano Stabellini wrote:
>>> On Fri, 15 Mar 2024, Jan Beulich wrote:
>>>> On 14.03.2024 23:59, Stefano Stabellini wrote:
>>>>> On Mon, 11 Mar 2024, Simone Ballarin wrote:
>>>>>> On 11/03/24 14:56, Jan Beulich wrote:
>>>>>>> On 11.03.2024 13:00, Simone Ballarin wrote:
>>>>>>>> On 11/03/24 11:08, Jan Beulich wrote:
>>>>>>>>> On 11.03.2024 09:59, Simone Ballarin wrote:
>>>>>>>>>> --- a/xen/arch/arm/include/asm/hypercall.h
>>>>>>>>>> +++ b/xen/arch/arm/include/asm/hypercall.h
>>>>>>>>>> @@ -1,3 +1,4 @@
>>>>>>>>>> +/* SAF-5-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-5-safe direct inclusion guard before */
>>>>>>>>>>    #ifndef __XEN_HYPERCALL_H__
>>>>>>>>>>    #error "asm/hypercall.h should not be included directly - include
>>>>>>>>>> xen/hypercall.h instead"
>>>>>>>>>>    #endif
>>>>>>>>>
>>>>>>>>> Iirc it was said that this way checking for correct guards is suppressed
>>>>>>>>> altogether in Eclair, which is not what we want. Can you clarify this,
>>>>>>>>> please?
>>>>>>>>>
>>>>>>>>
>>>>>>>> My first change was moving this check inside the guard.
>>>>>>>> You commented my patch saying that this would be an error because someone
>>>>>>>> can
>>>>>>>> include it directly if it has already been included indirectly.
>>>>>>>> I replied telling that this was the case also before the change.
>>>>>>>> You agreed with me, and we decided that the correct thing would be fixing
>>>>>>>> the
>>>>>>>> check and not apply my temporary change to address the finding.
>>>>>>>>
>>>>>>>> Considering that the code should be amended, a SAF deviation seems to me
>>>>>>>> the most appropriate way for suppressing these findings.
>>>>>>>
>>>>>>> Since I don't feel your reply addresses my question, asking differently:
>>>>>>> With
>>>>>>> your change in place, will failure to have proper guards (later) in these
>>>>>>> headers still be reported by Eclair?
>>>>>>
>>>>>> No, if you put something between the check and the guard,
>>>>>> no violation will be reported.
>>>>>
>>>>> From this email exchange I cannot under if Jan is OK with this patch or
>>>>> not.
>>>>
>>>> Whether I'm okay(ish) with the patch here depends on our position towards
>>>> the lost checking in Eclair mentioned above. To me it still looks relevant
>>>> that checking for a guard occurs, even if that isn't first in a file for
>>>> some specific reason.
>>>
>>> More checking is better than less checking, but if we cannot find a
>>> simple and actionable way forward on this violation, deviating it is
>>> still a big improvement because it allows us to enable the ECLAIR Dir
>>> 4.10 checks in xen.git overall (which again goes back to more checking
>>> is better than less checking). 
>>
>> You have a point here. I think though that at the very least the lost
>> checking opportunity wants calling out quite explicitly.
> 
> All right, then maybe this patch can go in with a clarification in the
> commit message?
> 
> Something like:
> 
> Note that with SAF-5-safe in place, failures to have proper guards later
> in the header files will not be reported

That would be okay with me.

Jan

Re: [XEN PATCH v3 03/16] misra: add deviations for direct inclusion guards

[Nicola Vetrini]

On 2024-03-19 08:45, Jan Beulich wrote:
> On 19.03.2024 04:34, Stefano Stabellini wrote:
>> On Mon, 18 Mar 2024, Jan Beulich wrote:
>>> On 16.03.2024 01:43, Stefano Stabellini wrote:
>>>> On Fri, 15 Mar 2024, Jan Beulich wrote:
>>>>> On 14.03.2024 23:59, Stefano Stabellini wrote:
>>>>>> On Mon, 11 Mar 2024, Simone Ballarin wrote:
>>>>>>> On 11/03/24 14:56, Jan Beulich wrote:
>>>>>>>> On 11.03.2024 13:00, Simone Ballarin wrote:
>>>>>>>>> On 11/03/24 11:08, Jan Beulich wrote:
>>>>>>>>>> On 11.03.2024 09:59, Simone Ballarin wrote:
>>>>>>>>>>> --- a/xen/arch/arm/include/asm/hypercall.h
>>>>>>>>>>> +++ b/xen/arch/arm/include/asm/hypercall.h
>>>>>>>>>>> @@ -1,3 +1,4 @@
>>>>>>>>>>> +/* SAF-5-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-5-safe direct inclusion guard before */
>>>>>>>>>>>    #ifndef __XEN_HYPERCALL_H__
>>>>>>>>>>>    #error "asm/hypercall.h should not be included directly - 
>>>>>>>>>>> include
>>>>>>>>>>> xen/hypercall.h instead"
>>>>>>>>>>>    #endif
>>>>>>>>>> 
>>>>>>>>>> Iirc it was said that this way checking for correct guards is 
>>>>>>>>>> suppressed
>>>>>>>>>> altogether in Eclair, which is not what we want. Can you 
>>>>>>>>>> clarify this,
>>>>>>>>>> please?
>>>>>>>>>> 
>>>>>>>>> 
>>>>>>>>> My first change was moving this check inside the guard.
>>>>>>>>> You commented my patch saying that this would be an error 
>>>>>>>>> because someone
>>>>>>>>> can
>>>>>>>>> include it directly if it has already been included indirectly.
>>>>>>>>> I replied telling that this was the case also before the 
>>>>>>>>> change.
>>>>>>>>> You agreed with me, and we decided that the correct thing would 
>>>>>>>>> be fixing
>>>>>>>>> the
>>>>>>>>> check and not apply my temporary change to address the finding.
>>>>>>>>> 
>>>>>>>>> Considering that the code should be amended, a SAF deviation 
>>>>>>>>> seems to me
>>>>>>>>> the most appropriate way for suppressing these findings.
>>>>>>>> 
>>>>>>>> Since I don't feel your reply addresses my question, asking 
>>>>>>>> differently:
>>>>>>>> With
>>>>>>>> your change in place, will failure to have proper guards (later) 
>>>>>>>> in these
>>>>>>>> headers still be reported by Eclair?
>>>>>>> 
>>>>>>> No, if you put something between the check and the guard,
>>>>>>> no violation will be reported.
>>>>>> 
>>>>>> From this email exchange I cannot under if Jan is OK with this 
>>>>>> patch or
>>>>>> not.
>>>>> 
>>>>> Whether I'm okay(ish) with the patch here depends on our position 
>>>>> towards
>>>>> the lost checking in Eclair mentioned above. To me it still looks 
>>>>> relevant
>>>>> that checking for a guard occurs, even if that isn't first in a 
>>>>> file for
>>>>> some specific reason.
>>>> 
>>>> More checking is better than less checking, but if we cannot find a
>>>> simple and actionable way forward on this violation, deviating it is
>>>> still a big improvement because it allows us to enable the ECLAIR 
>>>> Dir
>>>> 4.10 checks in xen.git overall (which again goes back to more 
>>>> checking
>>>> is better than less checking).
>>> 
>>> You have a point here. I think though that at the very least the lost
>>> checking opportunity wants calling out quite explicitly.
>> 
>> All right, then maybe this patch can go in with a clarification in the
>> commit message?
>> 
>> Something like:
>> 
>> Note that with SAF-5-safe in place, failures to have proper guards 
>> later
>> in the header files will not be reported
> 
> That would be okay with me.
> 

Coming back to this thread. Yes, I'll update the message to reflect this 
change.

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

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

[Simone Ballarin]

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>

---
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 0cb29f90a0..580859fecd 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 XEN_ARCH_ARM_EFI_EFI_BOOT_H
+#define XEN_ARCH_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 /* XEN_ARCH_ARM_EFI_EFI_BOOT_H */
+
 /*
  * Local variables:
  * mode: C
-- 
2.34.1

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

[Jan Beulich]

On 11.03.2024 09:59, Simone Ballarin wrote:
> --- 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 XEN_ARCH_ARM_EFI_EFI_BOOT_H
> +#define XEN_ARCH_ARM_EFI_EFI_BOOT_H

Related to my question raised against the cover letter, what does the
XEN_ prefix gain us here? All building of the hypervisor binaries
happens inside the xen/ subtree.

Jan

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

[Simone Ballarin]

On 11/03/24 11:10, Jan Beulich wrote:
> On 11.03.2024 09:59, Simone Ballarin wrote:
>> --- 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 XEN_ARCH_ARM_EFI_EFI_BOOT_H
>> +#define XEN_ARCH_ARM_EFI_EFI_BOOT_H
> 
> Related to my question raised against the cover letter, what does the
> XEN_ prefix gain us here? All building of the hypervisor binaries
> happens inside the xen/ subtree.
> 
> Jan

what do you thing about adding this rule:
arch/<arch>/<subdir>/<filename> -> <ARCH>_<subdir>_<filename>_H
?

-- 
Simone Ballarin, M.Sc.

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

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

[Jan Beulich]

On 11.03.2024 13:07, Simone Ballarin wrote:
> On 11/03/24 11:10, Jan Beulich wrote:
>> On 11.03.2024 09:59, Simone Ballarin wrote:
>>> --- 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 XEN_ARCH_ARM_EFI_EFI_BOOT_H
>>> +#define XEN_ARCH_ARM_EFI_EFI_BOOT_H
>>
>> Related to my question raised against the cover letter, what does the
>> XEN_ prefix gain us here? All building of the hypervisor binaries
>> happens inside the xen/ subtree.
> 
> what do you thing about adding this rule:
> arch/<arch>/<subdir>/<filename> -> <ARCH>_<subdir>_<filename>_H
> ?

Yet better - even the ARCH_ is gone then. Of course we want to then be
reasonably sure no arch appears with a name matching some of the other
top-level (under xen/) subdirs.

Jan

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

[Simone Ballarin]

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

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

Mechanical change.

Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>
Signed-off-by: Maria Celeste Cesario  <maria.celeste.cesario@bugseng.com>

---
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 remove them since there was a deviation for generated headers.
Starting from v2, they are required since the deviation has been removed by
another patch of this series.
---
 xen/arch/x86/Makefile              | 10 ++++++----
 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, 21 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
index 26d8740529..4548cdc53d 100644
--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -258,18 +258,20 @@ $(obj)/asm-macros.i: CFLAGS-y += -P
 $(objtree)/arch/x86/include/asm/asm-macros.h: $(obj)/asm-macros.i $(src)/Makefile
 	$(call filechk,asm-macros.h)
 
+ARCHDIR = $(shell echo $(SRCARCH) | tr a-z A-Z)
+
 define filechk_asm-macros.h
+    echo '#ifndef ASM_$(ARCHDIR)_ASM_MACROS_H'; \
+    echo '#define ASM_$(ARCHDIR)_ASM_MACROS_H'; \
     echo '#if 0'; \
     echo '.if 0'; \
     echo '#endif'; \
-    echo '#ifndef __ASM_MACROS_H__'; \
-    echo '#define __ASM_MACROS_H__'; \
     echo 'asm ( ".include \"$@\"" );'; \
-    echo '#endif /* __ASM_MACROS_H__ */'; \
     echo '#if 0'; \
     echo '.endif'; \
     cat $<; \
-    echo '#endif'
+    echo '#endif'; \
+    echo '#endif /* ASM_$(ARCHDIR)_ASM_MACROS_H */'
 endef
 
 $(obj)/efi.lds: AFLAGS-y += -DEFI
diff --git a/xen/arch/x86/cpu/cpu.h b/xen/arch/x86/cpu/cpu.h
index e3d06278b3..b763cdf3da 100644
--- a/xen/arch/x86/cpu/cpu.h
+++ b/xen/arch/x86/cpu/cpu.h
@@ -1,3 +1,6 @@
+#ifndef XEN_ARCH_X86_CPU_CPU_H
+#define XEN_ARCH_X86_CPU_CPU_H
+
 /* attempt to consolidate cpu attributes */
 struct cpu_dev {
 	void		(*c_early_init)(struct cpuinfo_x86 *c);
@@ -24,3 +27,5 @@ void amd_init_lfence(struct cpuinfo_x86 *c);
 void amd_init_ssbd(const struct cpuinfo_x86 *c);
 void amd_init_spectral_chicken(void);
 void detect_zen2_null_seg_behaviour(void);
+
+#endif /* XEN_ARCH_X86_CPU_CPU_H */
diff --git a/xen/arch/x86/x86_64/mmconfig.h b/xen/arch/x86/x86_64/mmconfig.h
index 3da4b21e9b..c8e67a67db 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 XEN_ARCH_X86_X86_64_MMCONFIG_H
+#define XEN_ARCH_X86_X86_64_MMCONFIG_H
+
 #define PCI_DEVICE_ID_INTEL_E7520_MCH    0x3590
 #define PCI_DEVICE_ID_INTEL_82945G_HB    0x2770
 
@@ -72,3 +75,5 @@ int pci_mmcfg_reserved(uint64_t address, unsigned int segment,
 int pci_mmcfg_arch_init(void);
 int pci_mmcfg_arch_enable(unsigned int idx);
 void pci_mmcfg_arch_disable(unsigned int idx);
+
+#endif /* XEN_ARCH_X86_X86_64_MMCONFIG_H */
diff --git a/xen/arch/x86/x86_emulate/private.h b/xen/arch/x86/x86_emulate/private.h
index 0fa26ba00a..835161cb53 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 XEN_ARCH_X86_X86_EMULATE_PRIVATE_H
+#define XEN_ARCH_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 /* XEN_ARCH_X86_X86_EMULATE_PRIVATE_H */
-- 
2.34.1

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

[Jan Beulich]

On 11.03.2024 09:59, Simone Ballarin wrote:
> --- a/xen/arch/x86/Makefile
> +++ b/xen/arch/x86/Makefile
> @@ -258,18 +258,20 @@ $(obj)/asm-macros.i: CFLAGS-y += -P
>  $(objtree)/arch/x86/include/asm/asm-macros.h: $(obj)/asm-macros.i $(src)/Makefile
>  	$(call filechk,asm-macros.h)
>  
> +ARCHDIR = $(shell echo $(SRCARCH) | tr a-z A-Z)

This wants to use :=, I think - there's no reason to invoke the shell ...

>  define filechk_asm-macros.h
> +    echo '#ifndef ASM_$(ARCHDIR)_ASM_MACROS_H'; \
> +    echo '#define ASM_$(ARCHDIR)_ASM_MACROS_H'; \
>      echo '#if 0'; \
>      echo '.if 0'; \
>      echo '#endif'; \
> -    echo '#ifndef __ASM_MACROS_H__'; \
> -    echo '#define __ASM_MACROS_H__'; \
>      echo 'asm ( ".include \"$@\"" );'; \
> -    echo '#endif /* __ASM_MACROS_H__ */'; \
>      echo '#if 0'; \
>      echo '.endif'; \
>      cat $<; \
> -    echo '#endif'
> +    echo '#endif'; \
> +    echo '#endif /* ASM_$(ARCHDIR)_ASM_MACROS_H */'
>  endef

... three times while expanding this macro. Alternatively (to avoid
an unnecessary shell invocation when this macro is never expanded at
all) a shell variable inside the "define" above would want introducing.
Whether this 2nd approach is better depends on whether we anticipate
further uses of ARCHDIR.

> --- a/xen/arch/x86/cpu/cpu.h
> +++ b/xen/arch/x86/cpu/cpu.h
> @@ -1,3 +1,6 @@
> +#ifndef XEN_ARCH_X86_CPU_CPU_H
> +#define XEN_ARCH_X86_CPU_CPU_H
> +
>  /* attempt to consolidate cpu attributes */
>  struct cpu_dev {
>  	void		(*c_early_init)(struct cpuinfo_x86 *c);
> @@ -24,3 +27,5 @@ void amd_init_lfence(struct cpuinfo_x86 *c);
>  void amd_init_ssbd(const struct cpuinfo_x86 *c);
>  void amd_init_spectral_chicken(void);
>  void detect_zen2_null_seg_behaviour(void);
> +
> +#endif /* XEN_ARCH_X86_CPU_CPU_H */

Leaving aside the earlier voiced request to get rid of the XEN_ prefixes
here, ...

> --- 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 XEN_ARCH_X86_X86_64_MMCONFIG_H
> +#define XEN_ARCH_X86_X86_64_MMCONFIG_H
> +
>  #define PCI_DEVICE_ID_INTEL_E7520_MCH    0x3590
>  #define PCI_DEVICE_ID_INTEL_82945G_HB    0x2770
>  
> @@ -72,3 +75,5 @@ int pci_mmcfg_reserved(uint64_t address, unsigned int segment,
>  int pci_mmcfg_arch_init(void);
>  int pci_mmcfg_arch_enable(unsigned int idx);
>  void pci_mmcfg_arch_disable(unsigned int idx);
> +
> +#endif /* XEN_ARCH_X86_X86_64_MMCONFIG_H */

... in a case like this and maybe even ...

> --- 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 XEN_ARCH_X86_X86_EMULATE_PRIVATE_H
> +#define XEN_ARCH_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 /* XEN_ARCH_X86_X86_EMULATE_PRIVATE_H */

... this I wonder whether they are too strictly sticking to the base
scheme (or whether the base scheme itself isn't flexible enough): I'm
not overly happy with the "_X86_X86_" in there. Especially in the
former case, where it's the sub-arch path, like for arm/arm<NN> I'd
like to see that folded to just "_X86_64_" here as well.

Jan

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

[Nicola Vetrini]

On 2024-03-12 09:16, Jan Beulich wrote:
> On 11.03.2024 09:59, Simone Ballarin wrote:
>> --- a/xen/arch/x86/Makefile
>> +++ b/xen/arch/x86/Makefile
>> @@ -258,18 +258,20 @@ $(obj)/asm-macros.i: CFLAGS-y += -P
>>  $(objtree)/arch/x86/include/asm/asm-macros.h: $(obj)/asm-macros.i 
>> $(src)/Makefile
>>  	$(call filechk,asm-macros.h)
>> 
>> +ARCHDIR = $(shell echo $(SRCARCH) | tr a-z A-Z)
> 
> This wants to use :=, I think - there's no reason to invoke the shell 
> ...

I agree on this

> 
>>  define filechk_asm-macros.h
>> +    echo '#ifndef ASM_$(ARCHDIR)_ASM_MACROS_H'; \
>> +    echo '#define ASM_$(ARCHDIR)_ASM_MACROS_H'; \
>>      echo '#if 0'; \
>>      echo '.if 0'; \
>>      echo '#endif'; \
>> -    echo '#ifndef __ASM_MACROS_H__'; \
>> -    echo '#define __ASM_MACROS_H__'; \
>>      echo 'asm ( ".include \"$@\"" );'; \
>> -    echo '#endif /* __ASM_MACROS_H__ */'; \
>>      echo '#if 0'; \
>>      echo '.endif'; \
>>      cat $<; \
>> -    echo '#endif'
>> +    echo '#endif'; \
>> +    echo '#endif /* ASM_$(ARCHDIR)_ASM_MACROS_H */'
>>  endef
> 
> ... three times while expanding this macro. Alternatively (to avoid
> an unnecessary shell invocation when this macro is never expanded at
> all) a shell variable inside the "define" above would want introducing.
> Whether this 2nd approach is better depends on whether we anticipate
> further uses of ARCHDIR.

However here I'm not entirely sure about the meaning of this latter 
proposal.
My proposal is the following:

ARCHDIR := $(shell echo $(SRCARCH) | tr a-z A-Z)

in a suitably generic place (such as Kbuild.include or maybe 
xen/Makefile) as you suggested in subsequent patches that reused this 
pattern.

> 
>> --- a/xen/arch/x86/cpu/cpu.h
>> +++ b/xen/arch/x86/cpu/cpu.h
>> @@ -1,3 +1,6 @@
>> +#ifndef XEN_ARCH_X86_CPU_CPU_H
>> +#define XEN_ARCH_X86_CPU_CPU_H
>> +
>>  /* attempt to consolidate cpu attributes */
>>  struct cpu_dev {
>>  	void		(*c_early_init)(struct cpuinfo_x86 *c);
>> @@ -24,3 +27,5 @@ void amd_init_lfence(struct cpuinfo_x86 *c);
>>  void amd_init_ssbd(const struct cpuinfo_x86 *c);
>>  void amd_init_spectral_chicken(void);
>>  void detect_zen2_null_seg_behaviour(void);
>> +
>> +#endif /* XEN_ARCH_X86_CPU_CPU_H */
> 
> Leaving aside the earlier voiced request to get rid of the XEN_ 
> prefixes
> here, ...
> 
>> --- 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 XEN_ARCH_X86_X86_64_MMCONFIG_H
>> +#define XEN_ARCH_X86_X86_64_MMCONFIG_H
>> +
>>  #define PCI_DEVICE_ID_INTEL_E7520_MCH    0x3590
>>  #define PCI_DEVICE_ID_INTEL_82945G_HB    0x2770
>> 
>> @@ -72,3 +75,5 @@ int pci_mmcfg_reserved(uint64_t address, unsigned 
>> int segment,
>>  int pci_mmcfg_arch_init(void);
>>  int pci_mmcfg_arch_enable(unsigned int idx);
>>  void pci_mmcfg_arch_disable(unsigned int idx);
>> +
>> +#endif /* XEN_ARCH_X86_X86_64_MMCONFIG_H */
> 
> ... in a case like this and maybe even ...
> 
>> --- 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 XEN_ARCH_X86_X86_EMULATE_PRIVATE_H
>> +#define XEN_ARCH_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 /* XEN_ARCH_X86_X86_EMULATE_PRIVATE_H */
> 
> ... this I wonder whether they are too strictly sticking to the base
> scheme (or whether the base scheme itself isn't flexible enough): I'm
> not overly happy with the "_X86_X86_" in there. Especially in the
> former case, where it's the sub-arch path, like for arm/arm<NN> I'd
> like to see that folded to just "_X86_64_" here as well.
> 

I do agree we should make an exception here: e.g. 
XEN_X86_64_EMULATE_PRIVATE_H

I'm ambivalent about the XEN_ prefix: I can't immediately see an issue 
with dropping it, but on the other hand there are several headers that 
already use it (either it or the __XEN prefix) as far as I can tell 
(e.g. x86/cpu/cpu.h), so dropping it from the naming convention would 
imply that a fair amount of additional churn may be needed to have an 
uniform naming scheme in all the xen/ directory. I'll leave the decision 
to the maintainers.

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

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

[Jan Beulich]

On 25.06.2024 21:31, Nicola Vetrini wrote:
> On 2024-03-12 09:16, Jan Beulich wrote:
>> On 11.03.2024 09:59, Simone Ballarin wrote:
>>> --- a/xen/arch/x86/Makefile
>>> +++ b/xen/arch/x86/Makefile
>>> @@ -258,18 +258,20 @@ $(obj)/asm-macros.i: CFLAGS-y += -P
>>>  $(objtree)/arch/x86/include/asm/asm-macros.h: $(obj)/asm-macros.i 
>>> $(src)/Makefile
>>>  	$(call filechk,asm-macros.h)
>>>
>>> +ARCHDIR = $(shell echo $(SRCARCH) | tr a-z A-Z)
>>
>> This wants to use :=, I think - there's no reason to invoke the shell 
>> ...
> 
> I agree on this
> 
>>
>>>  define filechk_asm-macros.h
>>> +    echo '#ifndef ASM_$(ARCHDIR)_ASM_MACROS_H'; \
>>> +    echo '#define ASM_$(ARCHDIR)_ASM_MACROS_H'; \
>>>      echo '#if 0'; \
>>>      echo '.if 0'; \
>>>      echo '#endif'; \
>>> -    echo '#ifndef __ASM_MACROS_H__'; \
>>> -    echo '#define __ASM_MACROS_H__'; \
>>>      echo 'asm ( ".include \"$@\"" );'; \
>>> -    echo '#endif /* __ASM_MACROS_H__ */'; \
>>>      echo '#if 0'; \
>>>      echo '.endif'; \
>>>      cat $<; \
>>> -    echo '#endif'
>>> +    echo '#endif'; \
>>> +    echo '#endif /* ASM_$(ARCHDIR)_ASM_MACROS_H */'
>>>  endef
>>
>> ... three times while expanding this macro. Alternatively (to avoid
>> an unnecessary shell invocation when this macro is never expanded at
>> all) a shell variable inside the "define" above would want introducing.
>> Whether this 2nd approach is better depends on whether we anticipate
>> further uses of ARCHDIR.
> 
> However here I'm not entirely sure about the meaning of this latter 
> proposal.
> My proposal is the following:
> 
> ARCHDIR := $(shell echo $(SRCARCH) | tr a-z A-Z)
> 
> in a suitably generic place (such as Kbuild.include or maybe 
> xen/Makefile) as you suggested in subsequent patches that reused this 
> pattern.
> 
>>
>>> --- a/xen/arch/x86/cpu/cpu.h
>>> +++ b/xen/arch/x86/cpu/cpu.h
>>> @@ -1,3 +1,6 @@
>>> +#ifndef XEN_ARCH_X86_CPU_CPU_H
>>> +#define XEN_ARCH_X86_CPU_CPU_H
>>> +
>>>  /* attempt to consolidate cpu attributes */
>>>  struct cpu_dev {
>>>  	void		(*c_early_init)(struct cpuinfo_x86 *c);
>>> @@ -24,3 +27,5 @@ void amd_init_lfence(struct cpuinfo_x86 *c);
>>>  void amd_init_ssbd(const struct cpuinfo_x86 *c);
>>>  void amd_init_spectral_chicken(void);
>>>  void detect_zen2_null_seg_behaviour(void);
>>> +
>>> +#endif /* XEN_ARCH_X86_CPU_CPU_H */
>>
>> Leaving aside the earlier voiced request to get rid of the XEN_ 
>> prefixes
>> here, ...
>>
>>> --- 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 XEN_ARCH_X86_X86_64_MMCONFIG_H
>>> +#define XEN_ARCH_X86_X86_64_MMCONFIG_H
>>> +
>>>  #define PCI_DEVICE_ID_INTEL_E7520_MCH    0x3590
>>>  #define PCI_DEVICE_ID_INTEL_82945G_HB    0x2770
>>>
>>> @@ -72,3 +75,5 @@ int pci_mmcfg_reserved(uint64_t address, unsigned 
>>> int segment,
>>>  int pci_mmcfg_arch_init(void);
>>>  int pci_mmcfg_arch_enable(unsigned int idx);
>>>  void pci_mmcfg_arch_disable(unsigned int idx);
>>> +
>>> +#endif /* XEN_ARCH_X86_X86_64_MMCONFIG_H */
>>
>> ... in a case like this and maybe even ...
>>
>>> --- 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 XEN_ARCH_X86_X86_EMULATE_PRIVATE_H
>>> +#define XEN_ARCH_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 /* XEN_ARCH_X86_X86_EMULATE_PRIVATE_H */
>>
>> ... this I wonder whether they are too strictly sticking to the base
>> scheme (or whether the base scheme itself isn't flexible enough): I'm
>> not overly happy with the "_X86_X86_" in there. Especially in the
>> former case, where it's the sub-arch path, like for arm/arm<NN> I'd
>> like to see that folded to just "_X86_64_" here as well.
>>
> 
> I do agree we should make an exception here: e.g. 
> XEN_X86_64_EMULATE_PRIVATE_H
> 
> I'm ambivalent about the XEN_ prefix: I can't immediately see an issue 
> with dropping it, but on the other hand there are several headers that 
> already use it (either it or the __XEN prefix) as far as I can tell 
> (e.g. x86/cpu/cpu.h), so dropping it from the naming convention would 
> imply that a fair amount of additional churn may be needed to have an 
> uniform naming scheme in all the xen/ directory. I'll leave the decision 
> to the maintainers.

Hmm, I'm puzzled: The example you point at presently has no guard at all,
afaics. There'll need to be churn there anyway. If you picked an example,
I would have expected that to be one where the guard already fully
matches the proposed scheme. To be honest I'd be surprised if we had many
files fulfilling this criteria.

Jan

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

[Nicola Vetrini]

On 2024-06-26 10:20, Jan Beulich wrote:
> On 25.06.2024 21:31, Nicola Vetrini wrote:
>> On 2024-03-12 09:16, Jan Beulich wrote:
>>> On 11.03.2024 09:59, Simone Ballarin wrote:
>>>> --- a/xen/arch/x86/Makefile
>>>> +++ b/xen/arch/x86/Makefile
>>>> @@ -258,18 +258,20 @@ $(obj)/asm-macros.i: CFLAGS-y += -P
>>>>  $(objtree)/arch/x86/include/asm/asm-macros.h: $(obj)/asm-macros.i
>>>> $(src)/Makefile
>>>>  	$(call filechk,asm-macros.h)
>>>> 
>>>> +ARCHDIR = $(shell echo $(SRCARCH) | tr a-z A-Z)
>>> 
>>> This wants to use :=, I think - there's no reason to invoke the shell
>>> ...
>> 
>> I agree on this
>> 
>>> 
>>>>  define filechk_asm-macros.h
>>>> +    echo '#ifndef ASM_$(ARCHDIR)_ASM_MACROS_H'; \
>>>> +    echo '#define ASM_$(ARCHDIR)_ASM_MACROS_H'; \
>>>>      echo '#if 0'; \
>>>>      echo '.if 0'; \
>>>>      echo '#endif'; \
>>>> -    echo '#ifndef __ASM_MACROS_H__'; \
>>>> -    echo '#define __ASM_MACROS_H__'; \
>>>>      echo 'asm ( ".include \"$@\"" );'; \
>>>> -    echo '#endif /* __ASM_MACROS_H__ */'; \
>>>>      echo '#if 0'; \
>>>>      echo '.endif'; \
>>>>      cat $<; \
>>>> -    echo '#endif'
>>>> +    echo '#endif'; \
>>>> +    echo '#endif /* ASM_$(ARCHDIR)_ASM_MACROS_H */'
>>>>  endef
>>> 
>>> ... three times while expanding this macro. Alternatively (to avoid
>>> an unnecessary shell invocation when this macro is never expanded at
>>> all) a shell variable inside the "define" above would want 
>>> introducing.
>>> Whether this 2nd approach is better depends on whether we anticipate
>>> further uses of ARCHDIR.
>> 
>> However here I'm not entirely sure about the meaning of this latter
>> proposal.
>> My proposal is the following:
>> 
>> ARCHDIR := $(shell echo $(SRCARCH) | tr a-z A-Z)
>> 
>> in a suitably generic place (such as Kbuild.include or maybe
>> xen/Makefile) as you suggested in subsequent patches that reused this
>> pattern.
>> 
>>> 
>>>> --- a/xen/arch/x86/cpu/cpu.h
>>>> +++ b/xen/arch/x86/cpu/cpu.h
>>>> @@ -1,3 +1,6 @@
>>>> +#ifndef XEN_ARCH_X86_CPU_CPU_H
>>>> +#define XEN_ARCH_X86_CPU_CPU_H
>>>> +
>>>>  /* attempt to consolidate cpu attributes */
>>>>  struct cpu_dev {
>>>>  	void		(*c_early_init)(struct cpuinfo_x86 *c);
>>>> @@ -24,3 +27,5 @@ void amd_init_lfence(struct cpuinfo_x86 *c);
>>>>  void amd_init_ssbd(const struct cpuinfo_x86 *c);
>>>>  void amd_init_spectral_chicken(void);
>>>>  void detect_zen2_null_seg_behaviour(void);
>>>> +
>>>> +#endif /* XEN_ARCH_X86_CPU_CPU_H */
>>> 
>>> Leaving aside the earlier voiced request to get rid of the XEN_
>>> prefixes
>>> here, ...
>>> 
>>>> --- 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 XEN_ARCH_X86_X86_64_MMCONFIG_H
>>>> +#define XEN_ARCH_X86_X86_64_MMCONFIG_H
>>>> +
>>>>  #define PCI_DEVICE_ID_INTEL_E7520_MCH    0x3590
>>>>  #define PCI_DEVICE_ID_INTEL_82945G_HB    0x2770
>>>> 
>>>> @@ -72,3 +75,5 @@ int pci_mmcfg_reserved(uint64_t address, unsigned
>>>> int segment,
>>>>  int pci_mmcfg_arch_init(void);
>>>>  int pci_mmcfg_arch_enable(unsigned int idx);
>>>>  void pci_mmcfg_arch_disable(unsigned int idx);
>>>> +
>>>> +#endif /* XEN_ARCH_X86_X86_64_MMCONFIG_H */
>>> 
>>> ... in a case like this and maybe even ...
>>> 
>>>> --- 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 XEN_ARCH_X86_X86_EMULATE_PRIVATE_H
>>>> +#define XEN_ARCH_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 /* XEN_ARCH_X86_X86_EMULATE_PRIVATE_H */
>>> 
>>> ... this I wonder whether they are too strictly sticking to the base
>>> scheme (or whether the base scheme itself isn't flexible enough): I'm
>>> not overly happy with the "_X86_X86_" in there. Especially in the
>>> former case, where it's the sub-arch path, like for arm/arm<NN> I'd
>>> like to see that folded to just "_X86_64_" here as well.
>>> 
>> 
>> I do agree we should make an exception here: e.g.
>> XEN_X86_64_EMULATE_PRIVATE_H
>> 
>> I'm ambivalent about the XEN_ prefix: I can't immediately see an issue
>> with dropping it, but on the other hand there are several headers that
>> already use it (either it or the __XEN prefix) as far as I can tell
>> (e.g. x86/cpu/cpu.h), so dropping it from the naming convention would
>> imply that a fair amount of additional churn may be needed to have an
>> uniform naming scheme in all the xen/ directory. I'll leave the 
>> decision
>> to the maintainers.
> 
> Hmm, I'm puzzled: The example you point at presently has no guard at 
> all,
> afaics. There'll need to be churn there anyway. If you picked an 
> example,
> I would have expected that to be one where the guard already fully
> matches the proposed scheme. To be honest I'd be surprised if we had 
> many
> files fulfilling this criteria.
> 

Ah, yes. I was looking at the state of the tree after some patches 
applied. On staging, most examples are in asm directories, such as 
xen/arch/x86/include/asm/endbr.h, but that would be modified anyway by 
dropping rule #3:
arch/<architecture>/include/asm/<subdir>/<filename>.h -> 
ASM_<architecture>_<subdir>_<filename>_H

With this, unless I find some other obstacle or issue from other 
maintainers, the XEN_ prefix can be dropped.

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

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

[Jan Beulich]

On 25.06.2024 21:31, Nicola Vetrini wrote:
> On 2024-03-12 09:16, Jan Beulich wrote:
>> On 11.03.2024 09:59, Simone Ballarin wrote:
>>> --- a/xen/arch/x86/Makefile
>>> +++ b/xen/arch/x86/Makefile
>>> @@ -258,18 +258,20 @@ $(obj)/asm-macros.i: CFLAGS-y += -P
>>>  $(objtree)/arch/x86/include/asm/asm-macros.h: $(obj)/asm-macros.i 
>>> $(src)/Makefile
>>>  	$(call filechk,asm-macros.h)
>>>
>>> +ARCHDIR = $(shell echo $(SRCARCH) | tr a-z A-Z)
>>
>> This wants to use :=, I think - there's no reason to invoke the shell 
>> ...
> 
> I agree on this
> 
>>
>>>  define filechk_asm-macros.h
>>> +    echo '#ifndef ASM_$(ARCHDIR)_ASM_MACROS_H'; \
>>> +    echo '#define ASM_$(ARCHDIR)_ASM_MACROS_H'; \
>>>      echo '#if 0'; \
>>>      echo '.if 0'; \
>>>      echo '#endif'; \
>>> -    echo '#ifndef __ASM_MACROS_H__'; \
>>> -    echo '#define __ASM_MACROS_H__'; \
>>>      echo 'asm ( ".include \"$@\"" );'; \
>>> -    echo '#endif /* __ASM_MACROS_H__ */'; \
>>>      echo '#if 0'; \
>>>      echo '.endif'; \
>>>      cat $<; \
>>> -    echo '#endif'
>>> +    echo '#endif'; \
>>> +    echo '#endif /* ASM_$(ARCHDIR)_ASM_MACROS_H */'
>>>  endef
>>
>> ... three times while expanding this macro. Alternatively (to avoid
>> an unnecessary shell invocation when this macro is never expanded at
>> all) a shell variable inside the "define" above would want introducing.
>> Whether this 2nd approach is better depends on whether we anticipate
>> further uses of ARCHDIR.
> 
> However here I'm not entirely sure about the meaning of this latter 
> proposal.
> My proposal is the following:
> 
> ARCHDIR := $(shell echo $(SRCARCH) | tr a-z A-Z)
> 
> in a suitably generic place (such as Kbuild.include or maybe 
> xen/Makefile) as you suggested in subsequent patches that reused this 
> pattern.

If $(ARCHDIR) is going to be used elsewhere, then what you suggest is fine.
My "whether" in the earlier reply specifically left open for clarification
what the intentions with the variable are. The alternative I had described
makes sense only when $(ARCHDIR) would only ever be used inside the
filechk_asm-macros.h macro.

Jan

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

[Nicola Vetrini]

On 2024-06-26 11:06, Jan Beulich wrote:
> On 25.06.2024 21:31, Nicola Vetrini wrote:
>> On 2024-03-12 09:16, Jan Beulich wrote:
>>> On 11.03.2024 09:59, Simone Ballarin wrote:
>>>> --- a/xen/arch/x86/Makefile
>>>> +++ b/xen/arch/x86/Makefile
>>>> @@ -258,18 +258,20 @@ $(obj)/asm-macros.i: CFLAGS-y += -P
>>>>  $(objtree)/arch/x86/include/asm/asm-macros.h: $(obj)/asm-macros.i
>>>> $(src)/Makefile
>>>>  	$(call filechk,asm-macros.h)
>>>> 
>>>> +ARCHDIR = $(shell echo $(SRCARCH) | tr a-z A-Z)
>>> 
>>> This wants to use :=, I think - there's no reason to invoke the shell
>>> ...
>> 
>> I agree on this
>> 
>>> 
>>>>  define filechk_asm-macros.h
>>>> +    echo '#ifndef ASM_$(ARCHDIR)_ASM_MACROS_H'; \
>>>> +    echo '#define ASM_$(ARCHDIR)_ASM_MACROS_H'; \
>>>>      echo '#if 0'; \
>>>>      echo '.if 0'; \
>>>>      echo '#endif'; \
>>>> -    echo '#ifndef __ASM_MACROS_H__'; \
>>>> -    echo '#define __ASM_MACROS_H__'; \
>>>>      echo 'asm ( ".include \"$@\"" );'; \
>>>> -    echo '#endif /* __ASM_MACROS_H__ */'; \
>>>>      echo '#if 0'; \
>>>>      echo '.endif'; \
>>>>      cat $<; \
>>>> -    echo '#endif'
>>>> +    echo '#endif'; \
>>>> +    echo '#endif /* ASM_$(ARCHDIR)_ASM_MACROS_H */'
>>>>  endef
>>> 
>>> ... three times while expanding this macro. Alternatively (to avoid
>>> an unnecessary shell invocation when this macro is never expanded at
>>> all) a shell variable inside the "define" above would want 
>>> introducing.
>>> Whether this 2nd approach is better depends on whether we anticipate
>>> further uses of ARCHDIR.
>> 
>> However here I'm not entirely sure about the meaning of this latter
>> proposal.
>> My proposal is the following:
>> 
>> ARCHDIR := $(shell echo $(SRCARCH) | tr a-z A-Z)
>> 
>> in a suitably generic place (such as Kbuild.include or maybe
>> xen/Makefile) as you suggested in subsequent patches that reused this
>> pattern.
> 
> If $(ARCHDIR) is going to be used elsewhere, then what you suggest is 
> fine.
> My "whether" in the earlier reply specifically left open for 
> clarification
> what the intentions with the variable are. The alternative I had 
> described
> makes sense only when $(ARCHDIR) would only ever be used inside the
> filechk_asm-macros.h macro.
> 

Yes, the intention is to reuse $(ARCHDIR) in the formation of other 
places, as you can tell from the fact that subsequent patches replicate 
the same pattern. This is going to save some duplication.
The only matter left then is whether xen/Makefile (around line 250, just 
after setting SRCARCH) would be better, or Kbuild.include. To me the 
former place seems more natural, but I'm not totally sure.

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

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

[Jan Beulich]

On 26.06.2024 11:20, Nicola Vetrini wrote:
> On 2024-06-26 11:06, Jan Beulich wrote:
>> On 25.06.2024 21:31, Nicola Vetrini wrote:
>>> On 2024-03-12 09:16, Jan Beulich wrote:
>>>> On 11.03.2024 09:59, Simone Ballarin wrote:
>>>>> --- a/xen/arch/x86/Makefile
>>>>> +++ b/xen/arch/x86/Makefile
>>>>> @@ -258,18 +258,20 @@ $(obj)/asm-macros.i: CFLAGS-y += -P
>>>>>  $(objtree)/arch/x86/include/asm/asm-macros.h: $(obj)/asm-macros.i
>>>>> $(src)/Makefile
>>>>>  	$(call filechk,asm-macros.h)
>>>>>
>>>>> +ARCHDIR = $(shell echo $(SRCARCH) | tr a-z A-Z)
>>>>
>>>> This wants to use :=, I think - there's no reason to invoke the shell
>>>> ...
>>>
>>> I agree on this
>>>
>>>>
>>>>>  define filechk_asm-macros.h
>>>>> +    echo '#ifndef ASM_$(ARCHDIR)_ASM_MACROS_H'; \
>>>>> +    echo '#define ASM_$(ARCHDIR)_ASM_MACROS_H'; \
>>>>>      echo '#if 0'; \
>>>>>      echo '.if 0'; \
>>>>>      echo '#endif'; \
>>>>> -    echo '#ifndef __ASM_MACROS_H__'; \
>>>>> -    echo '#define __ASM_MACROS_H__'; \
>>>>>      echo 'asm ( ".include \"$@\"" );'; \
>>>>> -    echo '#endif /* __ASM_MACROS_H__ */'; \
>>>>>      echo '#if 0'; \
>>>>>      echo '.endif'; \
>>>>>      cat $<; \
>>>>> -    echo '#endif'
>>>>> +    echo '#endif'; \
>>>>> +    echo '#endif /* ASM_$(ARCHDIR)_ASM_MACROS_H */'
>>>>>  endef
>>>>
>>>> ... three times while expanding this macro. Alternatively (to avoid
>>>> an unnecessary shell invocation when this macro is never expanded at
>>>> all) a shell variable inside the "define" above would want 
>>>> introducing.
>>>> Whether this 2nd approach is better depends on whether we anticipate
>>>> further uses of ARCHDIR.
>>>
>>> However here I'm not entirely sure about the meaning of this latter
>>> proposal.
>>> My proposal is the following:
>>>
>>> ARCHDIR := $(shell echo $(SRCARCH) | tr a-z A-Z)
>>>
>>> in a suitably generic place (such as Kbuild.include or maybe
>>> xen/Makefile) as you suggested in subsequent patches that reused this
>>> pattern.
>>
>> If $(ARCHDIR) is going to be used elsewhere, then what you suggest is 
>> fine.
>> My "whether" in the earlier reply specifically left open for 
>> clarification
>> what the intentions with the variable are. The alternative I had 
>> described
>> makes sense only when $(ARCHDIR) would only ever be used inside the
>> filechk_asm-macros.h macro.
> 
> Yes, the intention is to reuse $(ARCHDIR) in the formation of other 
> places, as you can tell from the fact that subsequent patches replicate 
> the same pattern. This is going to save some duplication.
> The only matter left then is whether xen/Makefile (around line 250, just 
> after setting SRCARCH) would be better, or Kbuild.include. To me the 
> former place seems more natural, but I'm not totally sure.

Depends on where all the intended uses are. If they're all in xen/Makefile,
then having the macro just there is of course sufficient. Whereas when it's
needed elsewhere, instead of exporting putting it in Kbuild.include would
seem more natural / desirable to me.

Jan

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

[Nicola Vetrini]

On 2024-06-26 11:26, Jan Beulich wrote:
> On 26.06.2024 11:20, Nicola Vetrini wrote:
>> On 2024-06-26 11:06, Jan Beulich wrote:
>>> On 25.06.2024 21:31, Nicola Vetrini wrote:
>>>> On 2024-03-12 09:16, Jan Beulich wrote:
>>>>> On 11.03.2024 09:59, Simone Ballarin wrote:
>>>>>> --- a/xen/arch/x86/Makefile
>>>>>> +++ b/xen/arch/x86/Makefile
>>>>>> @@ -258,18 +258,20 @@ $(obj)/asm-macros.i: CFLAGS-y += -P
>>>>>>  $(objtree)/arch/x86/include/asm/asm-macros.h: $(obj)/asm-macros.i
>>>>>> $(src)/Makefile
>>>>>>  	$(call filechk,asm-macros.h)
>>>>>> 
>>>>>> +ARCHDIR = $(shell echo $(SRCARCH) | tr a-z A-Z)
>>>>> 
>>>>> This wants to use :=, I think - there's no reason to invoke the 
>>>>> shell
>>>>> ...
>>>> 
>>>> I agree on this
>>>> 
>>>>> 
>>>>>>  define filechk_asm-macros.h
>>>>>> +    echo '#ifndef ASM_$(ARCHDIR)_ASM_MACROS_H'; \
>>>>>> +    echo '#define ASM_$(ARCHDIR)_ASM_MACROS_H'; \
>>>>>>      echo '#if 0'; \
>>>>>>      echo '.if 0'; \
>>>>>>      echo '#endif'; \
>>>>>> -    echo '#ifndef __ASM_MACROS_H__'; \
>>>>>> -    echo '#define __ASM_MACROS_H__'; \
>>>>>>      echo 'asm ( ".include \"$@\"" );'; \
>>>>>> -    echo '#endif /* __ASM_MACROS_H__ */'; \
>>>>>>      echo '#if 0'; \
>>>>>>      echo '.endif'; \
>>>>>>      cat $<; \
>>>>>> -    echo '#endif'
>>>>>> +    echo '#endif'; \
>>>>>> +    echo '#endif /* ASM_$(ARCHDIR)_ASM_MACROS_H */'
>>>>>>  endef
>>>>> 
>>>>> ... three times while expanding this macro. Alternatively (to avoid
>>>>> an unnecessary shell invocation when this macro is never expanded 
>>>>> at
>>>>> all) a shell variable inside the "define" above would want
>>>>> introducing.
>>>>> Whether this 2nd approach is better depends on whether we 
>>>>> anticipate
>>>>> further uses of ARCHDIR.
>>>> 
>>>> However here I'm not entirely sure about the meaning of this latter
>>>> proposal.
>>>> My proposal is the following:
>>>> 
>>>> ARCHDIR := $(shell echo $(SRCARCH) | tr a-z A-Z)
>>>> 
>>>> in a suitably generic place (such as Kbuild.include or maybe
>>>> xen/Makefile) as you suggested in subsequent patches that reused 
>>>> this
>>>> pattern.
>>> 
>>> If $(ARCHDIR) is going to be used elsewhere, then what you suggest is
>>> fine.
>>> My "whether" in the earlier reply specifically left open for
>>> clarification
>>> what the intentions with the variable are. The alternative I had
>>> described
>>> makes sense only when $(ARCHDIR) would only ever be used inside the
>>> filechk_asm-macros.h macro.
>> 
>> Yes, the intention is to reuse $(ARCHDIR) in the formation of other
>> places, as you can tell from the fact that subsequent patches 
>> replicate
>> the same pattern. This is going to save some duplication.
>> The only matter left then is whether xen/Makefile (around line 250, 
>> just
>> after setting SRCARCH) would be better, or Kbuild.include. To me the
>> former place seems more natural, but I'm not totally sure.
> 
> Depends on where all the intended uses are. If they're all in 
> xen/Makefile,
> then having the macro just there is of course sufficient. Whereas when 
> it's
> needed elsewhere, instead of exporting putting it in Kbuild.include 
> would
> seem more natural / desirable to me.
> 

The places where this would be used are these:
file: target (or define)
xen/build.mk: arch/$(SRCARCH)/include/asm/asm-offsets.h: asm-offsets.s
xen/include/Makefile: define cmd_xlat_h
xen/arch/x86/Makefile: define filechk_asm-macros.h

The only issue that comes to my mind (it may not be one at all) is that 
SRCARCH is defined and exported in xen/Makefile after including 
Kbuild.include, so it would need to be defined after SRCARCH is 
assigned:

include scripts/Kbuild.include

# Don't break if the build process wasn't called from the top level
# we need XEN_TARGET_ARCH to generate the proper config
include $(XEN_ROOT)/Config.mk

# Set ARCH/SRCARCH appropriately.

ARCH := $(XEN_TARGET_ARCH)
SRCARCH := $(shell echo $(ARCH) | \
     sed -e 's/x86.*/x86/' -e 's/arm\(32\|64\)/arm/g' \
         -e 's/riscv.*/riscv/g' -e 's/ppc.*/ppc/g')
export ARCH SRCARCH

Am I missing something?

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

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

[Jan Beulich]

On 26.06.2024 12:25, Nicola Vetrini wrote:
> On 2024-06-26 11:26, Jan Beulich wrote:
>> On 26.06.2024 11:20, Nicola Vetrini wrote:
>>> On 2024-06-26 11:06, Jan Beulich wrote:
>>>> On 25.06.2024 21:31, Nicola Vetrini wrote:
>>>>> On 2024-03-12 09:16, Jan Beulich wrote:
>>>>>> On 11.03.2024 09:59, Simone Ballarin wrote:
>>>>>>> --- a/xen/arch/x86/Makefile
>>>>>>> +++ b/xen/arch/x86/Makefile
>>>>>>> @@ -258,18 +258,20 @@ $(obj)/asm-macros.i: CFLAGS-y += -P
>>>>>>>  $(objtree)/arch/x86/include/asm/asm-macros.h: $(obj)/asm-macros.i
>>>>>>> $(src)/Makefile
>>>>>>>  	$(call filechk,asm-macros.h)
>>>>>>>
>>>>>>> +ARCHDIR = $(shell echo $(SRCARCH) | tr a-z A-Z)
>>>>>>
>>>>>> This wants to use :=, I think - there's no reason to invoke the 
>>>>>> shell
>>>>>> ...
>>>>>
>>>>> I agree on this
>>>>>
>>>>>>
>>>>>>>  define filechk_asm-macros.h
>>>>>>> +    echo '#ifndef ASM_$(ARCHDIR)_ASM_MACROS_H'; \
>>>>>>> +    echo '#define ASM_$(ARCHDIR)_ASM_MACROS_H'; \
>>>>>>>      echo '#if 0'; \
>>>>>>>      echo '.if 0'; \
>>>>>>>      echo '#endif'; \
>>>>>>> -    echo '#ifndef __ASM_MACROS_H__'; \
>>>>>>> -    echo '#define __ASM_MACROS_H__'; \
>>>>>>>      echo 'asm ( ".include \"$@\"" );'; \
>>>>>>> -    echo '#endif /* __ASM_MACROS_H__ */'; \
>>>>>>>      echo '#if 0'; \
>>>>>>>      echo '.endif'; \
>>>>>>>      cat $<; \
>>>>>>> -    echo '#endif'
>>>>>>> +    echo '#endif'; \
>>>>>>> +    echo '#endif /* ASM_$(ARCHDIR)_ASM_MACROS_H */'
>>>>>>>  endef
>>>>>>
>>>>>> ... three times while expanding this macro. Alternatively (to avoid
>>>>>> an unnecessary shell invocation when this macro is never expanded 
>>>>>> at
>>>>>> all) a shell variable inside the "define" above would want
>>>>>> introducing.
>>>>>> Whether this 2nd approach is better depends on whether we 
>>>>>> anticipate
>>>>>> further uses of ARCHDIR.
>>>>>
>>>>> However here I'm not entirely sure about the meaning of this latter
>>>>> proposal.
>>>>> My proposal is the following:
>>>>>
>>>>> ARCHDIR := $(shell echo $(SRCARCH) | tr a-z A-Z)
>>>>>
>>>>> in a suitably generic place (such as Kbuild.include or maybe
>>>>> xen/Makefile) as you suggested in subsequent patches that reused 
>>>>> this
>>>>> pattern.
>>>>
>>>> If $(ARCHDIR) is going to be used elsewhere, then what you suggest is
>>>> fine.
>>>> My "whether" in the earlier reply specifically left open for
>>>> clarification
>>>> what the intentions with the variable are. The alternative I had
>>>> described
>>>> makes sense only when $(ARCHDIR) would only ever be used inside the
>>>> filechk_asm-macros.h macro.
>>>
>>> Yes, the intention is to reuse $(ARCHDIR) in the formation of other
>>> places, as you can tell from the fact that subsequent patches 
>>> replicate
>>> the same pattern. This is going to save some duplication.
>>> The only matter left then is whether xen/Makefile (around line 250, 
>>> just
>>> after setting SRCARCH) would be better, or Kbuild.include. To me the
>>> former place seems more natural, but I'm not totally sure.
>>
>> Depends on where all the intended uses are. If they're all in 
>> xen/Makefile,
>> then having the macro just there is of course sufficient. Whereas when 
>> it's
>> needed elsewhere, instead of exporting putting it in Kbuild.include 
>> would
>> seem more natural / desirable to me.
>>
> 
> The places where this would be used are these:
> file: target (or define)
> xen/build.mk: arch/$(SRCARCH)/include/asm/asm-offsets.h: asm-offsets.s
> xen/include/Makefile: define cmd_xlat_h
> xen/arch/x86/Makefile: define filechk_asm-macros.h
> 
> The only issue that comes to my mind (it may not be one at all) is that 
> SRCARCH is defined and exported in xen/Makefile after including 
> Kbuild.include, so it would need to be defined after SRCARCH is 
> assigned:
> 
> include scripts/Kbuild.include
> 
> # Don't break if the build process wasn't called from the top level
> # we need XEN_TARGET_ARCH to generate the proper config
> include $(XEN_ROOT)/Config.mk
> 
> # Set ARCH/SRCARCH appropriately.
> 
> ARCH := $(XEN_TARGET_ARCH)
> SRCARCH := $(shell echo $(ARCH) | \
>      sed -e 's/x86.*/x86/' -e 's/arm\(32\|64\)/arm/g' \
>          -e 's/riscv.*/riscv/g' -e 's/ppc.*/ppc/g')
> export ARCH SRCARCH
> 
> Am I missing something?

In that case the alternatives are exporting or using = rather than := in
Kbuild.include, i.e. other than initially requested. Personally I dislike
exporting to a fair degree, but I'm not sure which one's better in this
case. Cc-ing Anthony for possible input.

Jan

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

[Anthony PERARD]

On Wed, Jun 26, 2024 at 12:31:42PM +0200, Jan Beulich wrote:
> On 26.06.2024 12:25, Nicola Vetrini wrote:
> > On 2024-06-26 11:26, Jan Beulich wrote:
> >> On 26.06.2024 11:20, Nicola Vetrini wrote:
> >>> On 2024-06-26 11:06, Jan Beulich wrote:
> >>>> On 25.06.2024 21:31, Nicola Vetrini wrote:
> >>>>> On 2024-03-12 09:16, Jan Beulich wrote:
> >>>>>> On 11.03.2024 09:59, Simone Ballarin wrote:
> >>>>>>> --- a/xen/arch/x86/Makefile
> >>>>>>> +++ b/xen/arch/x86/Makefile
> >>>>>>> @@ -258,18 +258,20 @@ $(obj)/asm-macros.i: CFLAGS-y += -P
> >>>>>>>  $(objtree)/arch/x86/include/asm/asm-macros.h: $(obj)/asm-macros.i
> >>>>>>> $(src)/Makefile
> >>>>>>>  	$(call filechk,asm-macros.h)
> >>>>>>>
> >>>>>>> +ARCHDIR = $(shell echo $(SRCARCH) | tr a-z A-Z)
> >>>>>>
> >>>>>> This wants to use :=, I think - there's no reason to invoke the 
> >>>>>> shell
> >>>>>> ...
> >>>>>
> >>>>> I agree on this
> >>>>>
> >>>>>>
> >>>>>>>  define filechk_asm-macros.h
> >>>>>>> +    echo '#ifndef ASM_$(ARCHDIR)_ASM_MACROS_H'; \
> >>>>>>> +    echo '#define ASM_$(ARCHDIR)_ASM_MACROS_H'; \
> >>>>>>>      echo '#if 0'; \
> >>>>>>>      echo '.if 0'; \
> >>>>>>>      echo '#endif'; \
> >>>>>>> -    echo '#ifndef __ASM_MACROS_H__'; \
> >>>>>>> -    echo '#define __ASM_MACROS_H__'; \
> >>>>>>>      echo 'asm ( ".include \"$@\"" );'; \
> >>>>>>> -    echo '#endif /* __ASM_MACROS_H__ */'; \
> >>>>>>>      echo '#if 0'; \
> >>>>>>>      echo '.endif'; \
> >>>>>>>      cat $<; \
> >>>>>>> -    echo '#endif'
> >>>>>>> +    echo '#endif'; \
> >>>>>>> +    echo '#endif /* ASM_$(ARCHDIR)_ASM_MACROS_H */'
> >>>>>>>  endef
> >>>>>>
> >>>>>> ... three times while expanding this macro. Alternatively (to avoid
> >>>>>> an unnecessary shell invocation when this macro is never expanded 
> >>>>>> at
> >>>>>> all) a shell variable inside the "define" above would want
> >>>>>> introducing.
> >>>>>> Whether this 2nd approach is better depends on whether we 
> >>>>>> anticipate
> >>>>>> further uses of ARCHDIR.
> >>>>>
> >>>>> However here I'm not entirely sure about the meaning of this latter
> >>>>> proposal.
> >>>>> My proposal is the following:
> >>>>>
> >>>>> ARCHDIR := $(shell echo $(SRCARCH) | tr a-z A-Z)
> >>>>>
> >>>>> in a suitably generic place (such as Kbuild.include or maybe
> >>>>> xen/Makefile) as you suggested in subsequent patches that reused 
> >>>>> this
> >>>>> pattern.
> >>>>
> >>>> If $(ARCHDIR) is going to be used elsewhere, then what you suggest is
> >>>> fine.
> >>>> My "whether" in the earlier reply specifically left open for
> >>>> clarification
> >>>> what the intentions with the variable are. The alternative I had
> >>>> described
> >>>> makes sense only when $(ARCHDIR) would only ever be used inside the
> >>>> filechk_asm-macros.h macro.
> >>>
> >>> Yes, the intention is to reuse $(ARCHDIR) in the formation of other
> >>> places, as you can tell from the fact that subsequent patches 
> >>> replicate
> >>> the same pattern. This is going to save some duplication.
> >>> The only matter left then is whether xen/Makefile (around line 250, 
> >>> just
> >>> after setting SRCARCH) would be better, or Kbuild.include. To me the
> >>> former place seems more natural, but I'm not totally sure.
> >>
> >> Depends on where all the intended uses are. If they're all in 
> >> xen/Makefile,
> >> then having the macro just there is of course sufficient. Whereas when 
> >> it's
> >> needed elsewhere, instead of exporting putting it in Kbuild.include 
> >> would
> >> seem more natural / desirable to me.
> >>
> > 
> > The places where this would be used are these:
> > file: target (or define)
> > xen/build.mk: arch/$(SRCARCH)/include/asm/asm-offsets.h: asm-offsets.s
> > xen/include/Makefile: define cmd_xlat_h
> > xen/arch/x86/Makefile: define filechk_asm-macros.h
> > 
> > The only issue that comes to my mind (it may not be one at all) is that 
> > SRCARCH is defined and exported in xen/Makefile after including 
> > Kbuild.include, so it would need to be defined after SRCARCH is 
> > assigned:
> > 
> > include scripts/Kbuild.include
> > 
> > # Don't break if the build process wasn't called from the top level
> > # we need XEN_TARGET_ARCH to generate the proper config
> > include $(XEN_ROOT)/Config.mk
> > 
> > # Set ARCH/SRCARCH appropriately.
> > 
> > ARCH := $(XEN_TARGET_ARCH)
> > SRCARCH := $(shell echo $(ARCH) | \
> >      sed -e 's/x86.*/x86/' -e 's/arm\(32\|64\)/arm/g' \
> >          -e 's/riscv.*/riscv/g' -e 's/ppc.*/ppc/g')
> > export ARCH SRCARCH
> > 
> > Am I missing something?
> 
> In that case the alternatives are exporting or using = rather than := in
> Kbuild.include, i.e. other than initially requested. Personally I dislike
> exporting to a fair degree, but I'm not sure which one's better in this
> case. Cc-ing Anthony for possible input.

None. The name is missleading anyway, it would suggest to me that it
contain a directory, but that's wrong.

Another thing that suboptimal, use make to call a shell to generate a
string that is going to be later use in shell context. How about just
doing the work in that later shell context?

Something like:

 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'; \

Or, instead of having to write the name of the file down, we could
use a name that is already registered in a variable:

 define filechk_asm-macros.h
+    guard=$$(echo $@ | tr a-z/.- A-Z_); \
+    echo "#ifndef $$guard"; \
+    echo "#define $$guard"; \
     echo '#if 0'; \
     echo '.if 0'; \

This produces:
    #ifndef ARCH_X86_INCLUDE_ASM_ASM_MACROS_H
    #define ARCH_X86_INCLUDE_ASM_ASM_MACROS_H
    #if 0
    .if 0

Cheers,

-- 

Anthony Perard | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

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

[Nicola Vetrini]

On 2024-06-26 15:38, Anthony PERARD wrote:
> On Wed, Jun 26, 2024 at 12:31:42PM +0200, Jan Beulich wrote:
>> On 26.06.2024 12:25, Nicola Vetrini wrote:
>> > On 2024-06-26 11:26, Jan Beulich wrote:
>> >> On 26.06.2024 11:20, Nicola Vetrini wrote:
>> >>> On 2024-06-26 11:06, Jan Beulich wrote:
>> >>>> On 25.06.2024 21:31, Nicola Vetrini wrote:
>> >>>>> On 2024-03-12 09:16, Jan Beulich wrote:
>> >>>>>> On 11.03.2024 09:59, Simone Ballarin wrote:
>> >>>>>>> --- a/xen/arch/x86/Makefile
>> >>>>>>> +++ b/xen/arch/x86/Makefile
>> >>>>>>> @@ -258,18 +258,20 @@ $(obj)/asm-macros.i: CFLAGS-y += -P
>> >>>>>>>  $(objtree)/arch/x86/include/asm/asm-macros.h: $(obj)/asm-macros.i
>> >>>>>>> $(src)/Makefile
>> >>>>>>>  	$(call filechk,asm-macros.h)
>> >>>>>>>
>> >>>>>>> +ARCHDIR = $(shell echo $(SRCARCH) | tr a-z A-Z)
>> >>>>>>
>> >>>>>> This wants to use :=, I think - there's no reason to invoke the
>> >>>>>> shell
>> >>>>>> ...
>> >>>>>
>> >>>>> I agree on this
>> >>>>>
>> >>>>>>
>> >>>>>>>  define filechk_asm-macros.h
>> >>>>>>> +    echo '#ifndef ASM_$(ARCHDIR)_ASM_MACROS_H'; \
>> >>>>>>> +    echo '#define ASM_$(ARCHDIR)_ASM_MACROS_H'; \
>> >>>>>>>      echo '#if 0'; \
>> >>>>>>>      echo '.if 0'; \
>> >>>>>>>      echo '#endif'; \
>> >>>>>>> -    echo '#ifndef __ASM_MACROS_H__'; \
>> >>>>>>> -    echo '#define __ASM_MACROS_H__'; \
>> >>>>>>>      echo 'asm ( ".include \"$@\"" );'; \
>> >>>>>>> -    echo '#endif /* __ASM_MACROS_H__ */'; \
>> >>>>>>>      echo '#if 0'; \
>> >>>>>>>      echo '.endif'; \
>> >>>>>>>      cat $<; \
>> >>>>>>> -    echo '#endif'
>> >>>>>>> +    echo '#endif'; \
>> >>>>>>> +    echo '#endif /* ASM_$(ARCHDIR)_ASM_MACROS_H */'
>> >>>>>>>  endef
>> >>>>>>
>> >>>>>> ... three times while expanding this macro. Alternatively (to avoid
>> >>>>>> an unnecessary shell invocation when this macro is never expanded
>> >>>>>> at
>> >>>>>> all) a shell variable inside the "define" above would want
>> >>>>>> introducing.
>> >>>>>> Whether this 2nd approach is better depends on whether we
>> >>>>>> anticipate
>> >>>>>> further uses of ARCHDIR.
>> >>>>>
>> >>>>> However here I'm not entirely sure about the meaning of this latter
>> >>>>> proposal.
>> >>>>> My proposal is the following:
>> >>>>>
>> >>>>> ARCHDIR := $(shell echo $(SRCARCH) | tr a-z A-Z)
>> >>>>>
>> >>>>> in a suitably generic place (such as Kbuild.include or maybe
>> >>>>> xen/Makefile) as you suggested in subsequent patches that reused
>> >>>>> this
>> >>>>> pattern.
>> >>>>
>> >>>> If $(ARCHDIR) is going to be used elsewhere, then what you suggest is
>> >>>> fine.
>> >>>> My "whether" in the earlier reply specifically left open for
>> >>>> clarification
>> >>>> what the intentions with the variable are. The alternative I had
>> >>>> described
>> >>>> makes sense only when $(ARCHDIR) would only ever be used inside the
>> >>>> filechk_asm-macros.h macro.
>> >>>
>> >>> Yes, the intention is to reuse $(ARCHDIR) in the formation of other
>> >>> places, as you can tell from the fact that subsequent patches
>> >>> replicate
>> >>> the same pattern. This is going to save some duplication.
>> >>> The only matter left then is whether xen/Makefile (around line 250,
>> >>> just
>> >>> after setting SRCARCH) would be better, or Kbuild.include. To me the
>> >>> former place seems more natural, but I'm not totally sure.
>> >>
>> >> Depends on where all the intended uses are. If they're all in
>> >> xen/Makefile,
>> >> then having the macro just there is of course sufficient. Whereas when
>> >> it's
>> >> needed elsewhere, instead of exporting putting it in Kbuild.include
>> >> would
>> >> seem more natural / desirable to me.
>> >>
>> >
>> > The places where this would be used are these:
>> > file: target (or define)
>> > xen/build.mk: arch/$(SRCARCH)/include/asm/asm-offsets.h: asm-offsets.s
>> > xen/include/Makefile: define cmd_xlat_h
>> > xen/arch/x86/Makefile: define filechk_asm-macros.h
>> >
>> > The only issue that comes to my mind (it may not be one at all) is that
>> > SRCARCH is defined and exported in xen/Makefile after including
>> > Kbuild.include, so it would need to be defined after SRCARCH is
>> > assigned:
>> >
>> > include scripts/Kbuild.include
>> >
>> > # Don't break if the build process wasn't called from the top level
>> > # we need XEN_TARGET_ARCH to generate the proper config
>> > include $(XEN_ROOT)/Config.mk
>> >
>> > # Set ARCH/SRCARCH appropriately.
>> >
>> > ARCH := $(XEN_TARGET_ARCH)
>> > SRCARCH := $(shell echo $(ARCH) | \
>> >      sed -e 's/x86.*/x86/' -e 's/arm\(32\|64\)/arm/g' \
>> >          -e 's/riscv.*/riscv/g' -e 's/ppc.*/ppc/g')
>> > export ARCH SRCARCH
>> >
>> > Am I missing something?
>> 
>> In that case the alternatives are exporting or using = rather than := 
>> in
>> Kbuild.include, i.e. other than initially requested. Personally I 
>> dislike
>> exporting to a fair degree, but I'm not sure which one's better in 
>> this
>> case. Cc-ing Anthony for possible input.
> 
> None. The name is missleading anyway, it would suggest to me that it
> contain a directory, but that's wrong.
> 
> Another thing that suboptimal, use make to call a shell to generate a
> string that is going to be later use in shell context. How about just
> doing the work in that later shell context?
> 
> Something like:
> 
>  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'; \
> 

This approach looks ok to me.

> Or, instead of having to write the name of the file down, we could
> use a name that is already registered in a variable:
> 
>  define filechk_asm-macros.h
> +    guard=$$(echo $@ | tr a-z/.- A-Z_); \
> +    echo "#ifndef $$guard"; \
> +    echo "#define $$guard"; \
>      echo '#if 0'; \
>      echo '.if 0'; \
> 
> This produces:
>     #ifndef ARCH_X86_INCLUDE_ASM_ASM_MACROS_H
>     #define ARCH_X86_INCLUDE_ASM_ASM_MACROS_H
>     #if 0
>     .if 0
> 
> Cheers,

The issue I see here is that it would in some cases lead to long header 
guards, which is somewhat against the overall consensus, given that the 
naming convention should be followed by any file, so it was designed to 
generate shorter guards, rather than just the path.

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

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

[Simone Ballarin]

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>

---
Changes in v3:
- remove trailing underscores
- change inclusion guard name to adhere to the new standard
---
 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 8ea64e31cd..df6eaa9e9e 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 XEN_ARCH_X86_EFI_EFI_BOOT_H
+#define XEN_ARCH_X86_EFI_EFI_BOOT_H
+
+
 #include <xen/vga.h>
 #include <asm/e820.h>
 #include <asm/edd.h>
@@ -905,6 +910,8 @@ void asmlinkage __init efi_multiboot2(EFI_HANDLE ImageHandle,
     efi_exit_boot(ImageHandle, SystemTable);
 }
 
+#endif /* XEN_ARCH_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..13d4ba66b6 100644
--- a/xen/arch/x86/efi/runtime.h
+++ b/xen/arch/x86/efi/runtime.h
@@ -1,3 +1,6 @@
+#ifndef XEN_ARCH_X86_EFI_RUNTIME_H
+#define XEN_ARCH_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 /* XEN_ARCH_X86_EFI_RUNTIME_H */
-- 
2.34.1

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

[Stefano Stabellini]

On Mon, 11 Mar 2024, Simone Ballarin wrote:
> 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>

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

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

[Simone Ballarin]

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

Mechanical change.

Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>
Signed-off-by: Maria Celeste Cesario  <maria.celeste.cesario@bugseng.com>

---
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..41848274ab 100644
--- a/xen/common/decompress.h
+++ b/xen/common/decompress.h
@@ -1,3 +1,6 @@
+#ifndef XEN_COMMON_DECOMPRESS_H
+#define XEN_COMMON_DECOMPRESS_H
+
 #ifdef __XEN__
 
 #include <xen/cache.h>
@@ -23,3 +26,5 @@
 #define large_free free
 
 #endif
+
+#endif /* XEN_COMMON_DECOMPRESS_H */
diff --git a/xen/common/event_channel.h b/xen/common/event_channel.h
index 45219ca67c..a23e39a713 100644
--- a/xen/common/event_channel.h
+++ b/xen/common/event_channel.h
@@ -1,5 +1,8 @@
 /* Event channel handling private header. */
 
+#ifndef XEN_COMMON_EVENT_CHANNEL_H
+#define XEN_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 /* XEN_COMMON_EVENT_CHANNEL_H */
+
 /*
  * Local variables:
  * mode: C
-- 
2.34.1

Re: [XEN PATCH v3 07/16] xen/common: address violations of MISRA C:2012 Directive 4.10

[Stefano Stabellini]

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

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

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

[Simone Ballarin]

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

Mechanical change.

Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>
Signed-off-by: Maria Celeste Cesario  <maria.celeste.cesario@bugseng.com>

---
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..ab604ebd59 100644
--- a/xen/common/efi/efi.h
+++ b/xen/common/efi/efi.h
@@ -1,3 +1,6 @@
+#ifndef XEN_COMMON_EFI_EFI_H
+#define XEN_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 /* XEN_COMMON_EFI_EFI_H */
-- 
2.34.1

Re: [XEN PATCH v3 08/16] xen/efi: address violations of MISRA C:2012 Directive 4.10

[Stefano Stabellini]

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

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

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

[Simone Ballarin]

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>

---
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..360307c5da 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 XEN_INCLUDE_XEN_ERR_H
+#define XEN_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 /* XEN_INCLUDE_XEN_ERR_H */
diff --git a/xen/include/xen/pci_ids.h b/xen/include/xen/pci_ids.h
index e798477a7e..8f2318bccd 100644
--- a/xen/include/xen/pci_ids.h
+++ b/xen/include/xen/pci_ids.h
@@ -1,3 +1,6 @@
+#ifndef XEN_INCLUDE_XEN_PCI_IDS_H
+#define XEN_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 /* XEN_INCLUDE_XEN_PCI_IDS_H */
diff --git a/xen/include/xen/softirq.h b/xen/include/xen/softirq.h
index 33d6f2ecd2..d108cdd4bc 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 XEN_INCLUDE_XEN_SOFTIRQ_H
+#define XEN_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 /* XEN_INCLUDE_XEN_SOFTIRQ_H */
diff --git a/xen/include/xen/vmap.h b/xen/include/xen/vmap.h
index 0c16baa85f..bb83f5f64a 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 XEN_INCLUDE_XEN_VMAP_H
+#define XEN_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 /* XEN_INCLUDE_XEN_VMAP_H */
-- 
2.34.1

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

[Jan Beulich]

On 11.03.2024 09:59, Simone Ballarin wrote:
> 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.

This latter restriction, even if just slightly, hampers readability

> --- 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 XEN_INCLUDE_XEN_ERR_H
> +#define XEN_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 /* XEN_INCLUDE_XEN_ERR_H */

... here, ...

> --- 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 XEN_INCLUDE_XEN_SOFTIRQ_H
> +#define XEN_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 /* XEN_INCLUDE_XEN_SOFTIRQ_H */

... here, and ...

> --- 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 XEN_INCLUDE_XEN_VMAP_H
> +#define XEN_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 /* XEN_INCLUDE_XEN_VMAP_H */

... here. Wasn't a goal of Misra to also not have negative effects on code
readability?

Jan

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

[Julien Grall]

Hi Simone,

On 11/03/2024 08:59, Simone Ballarin wrote:
> 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>

Regarding Jan's comment, I don't have any strong opinion on whether this 
patch impair readability in this case. So:

Acked-by: Julien Grall <jgrall@amazon.com>

Cheers,

-- 
Julien Grall

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

[Stefano Stabellini]

On Mon, 11 Mar 2024, Simone Ballarin wrote:
> 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>

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

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

[Simone Ballarin]

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>

---
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                   | 10 ++++++++--
 5 files changed, 27 insertions(+), 6 deletions(-)

diff --git a/docs/misra/safe.json b/docs/misra/safe.json
index 363c9e21b0..13208d18ec 100644
--- a/docs/misra/safe.json
+++ b/docs/misra/safe.json
@@ -52,6 +52,14 @@
         },
         {
             "id": "SAF-6-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-7-safe",
             "analyser": {},
             "name": "Sentinel",
             "text": "Next ID to be used"
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 c3aad21c3b..a65754d553 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-6-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 a77c9ffb7e..e777921eb1 100644
--- a/xen/include/Makefile
+++ b/xen/include/Makefile
@@ -104,10 +104,16 @@ $(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
+	echo "#ifndef ASM_$(ARCHDIR)_COMPAT_XLAT_H" > $@.new; \
+	echo "#define ASM_$(ARCHDIR)_COMPAT_XLAT_H" >> $@.new; \
+	cat $(filter %.h,$^) >> $@.new; \
+	echo "#endif /* ASM_$(ARCHDIR)_COMPAT_XLAT_H */" >> $@.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: [XEN PATCH v3 10/16] x86/asm: address violations of MISRA C:2012 Directive 4.10

[Jan Beulich]

On 11.03.2024 09:59, Simone Ballarin wrote:
> --- a/docs/misra/safe.json
> +++ b/docs/misra/safe.json
> @@ -52,6 +52,14 @@
>          },
>          {
>              "id": "SAF-6-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."
> +        },

There's an overlap with SAF-3-safe as added by patch 01. What's the reason
separate entries are needed?

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

I'd be happy to ack this change; once again an indication that dissimilar
changes would better not be munged in a single patch. Such a patch, btw,
also shouldn't come with "x86/asm:" as a subject prefix - there's nothing
assembly-ish here; asm/ as a directory name is Linux heritage, without
that being a "component" in any way.

> --- 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-6-safe file intended for multiple inclusion */
>  #include <xen/lib/x86/cpuid-autogen.h>

With the blank line removed the comment, by convention of how these
comments are placed, applies to the #include directive, which is wrong.

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

This could go with the compat.h change above.

> --- a/xen/include/Makefile
> +++ b/xen/include/Makefile
> @@ -104,10 +104,16 @@ $(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)

Didn't the earlier patch introduce ARCHDIR already, just elsewhere? This
would better be done once in a central place then, maybe in
scripts/Kbuild.include.

>  quiet_cmd_xlat_h = GEN     $@
> -cmd_xlat_h = \
> -	cat $(filter %.h,$^) >$@.new; \
> +define cmd_xlat_h
> +	echo "#ifndef ASM_$(ARCHDIR)_COMPAT_XLAT_H" > $@.new; \
> +	echo "#define ASM_$(ARCHDIR)_COMPAT_XLAT_H" >> $@.new; \
> +	cat $(filter %.h,$^) >> $@.new; \
> +	echo "#endif /* ASM_$(ARCHDIR)_COMPAT_XLAT_H */" >> $@.new; \
>  	mv -f $@.new $@
> +endef

I'm unclear about the move from "=" to "define". Readability isn't really
helped, is it?

Jan

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

[Simone Ballarin]

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.
---
Commit introduced in v3

Signed-off-by: Maria Celeste Cesario  <maria.celeste.cesario@bugseng.com>
Signed-off-by: Simone Ballarin  <simone.ballarin@bugseng.com>
---
 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 8218afb862..b7dc023a25 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>
@@ -312,7 +312,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..689bf63232 100644
--- a/xen/arch/arm/include/asm/efibind.h
+++ b/xen/arch/arm/include/asm/efibind.h
@@ -1,2 +1,7 @@
+#ifndef ASM_ARM_EFI_BIND_H
+#define ASM_ARM_EFI_BIND_H
+
 #include <xen/types.h>
 #include <asm/arm64/efibind.h>
+
+#endif /* ASM_ARM_EFI_BIND_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 c8044b0371..48b46de51f 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>
@@ -99,7 +99,7 @@ void irq_set_affinity(struct irq_desc *desc, const cpumask_t *mask);
  */
 bool irq_type_set_by_domain(const struct domain *d);
 
-#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: [XEN PATCH v3 11/16] xen/arm: address violations of MISRA C:2012 Directive 4.10

[Stefano Stabellini]

On Mon, 11 Mar 2024, Simone Ballarin 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.
> ---
> Commit introduced in v3
> 
> Signed-off-by: Maria Celeste Cesario  <maria.celeste.cesario@bugseng.com>
> Signed-off-by: Simone Ballarin  <simone.ballarin@bugseng.com>
> ---
>  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 8218afb862..b7dc023a25 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>
> @@ -312,7 +312,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..689bf63232 100644
> --- a/xen/arch/arm/include/asm/efibind.h
> +++ b/xen/arch/arm/include/asm/efibind.h
> @@ -1,2 +1,7 @@
> +#ifndef ASM_ARM_EFI_BIND_H

This should be ASM_ARM_EFIBIND_H ?

Everything else looks fine to me

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

[Simone Ballarin]

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.
---
Commit introduced in v3

Signed-off-by: Maria Celeste Cesario  <maria.celeste.cesario@bugseng.com>
Signed-off-by: Simone Ballarin  <simone.ballarin@bugseng.com>
---
 xen/build.mk                     |  6 ++++--
 xen/scripts/Makefile.asm-generic | 16 +++++++++++++++-
 2 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/xen/build.mk b/xen/build.mk
index 0f490ca71b..bd8b93e2ae 100644
--- a/xen/build.mk
+++ b/xen/build.mk
@@ -45,6 +45,8 @@ asm-offsets.s: arch/$(SRCARCH)/$(ARCH)/asm-offsets.c
 	$(CC) $(call cpp_flags,$(c_flags)) -S -g0 -o $@.new -MQ $@ $<
 	$(call move-if-changed,$@.new,$@)
 
+ARCHDIR = $(shell echo $(SRCARCH) | tr a-z A-Z)
+
 arch/$(SRCARCH)/include/asm/asm-offsets.h: asm-offsets.s
 	@(set -e; \
 	  echo "/*"; \
@@ -54,8 +56,8 @@ 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 ASM_$(ARCHDIR)_ASM_OFFSETS_H"; \
+	  echo "#define ASM_$(ARCHDIR)_ASM_OFFSETS_H"; \
 	  echo ""; \
 	  sed -rne "/^[^#].*==>/{s:.*==>(.*)<==.*:\1:; s: [\$$#]: :; p;}"; \
 	  echo ""; \
diff --git a/xen/scripts/Makefile.asm-generic b/xen/scripts/Makefile.asm-generic
index b0d356bfa3..3b538bc7a4 100644
--- a/xen/scripts/Makefile.asm-generic
+++ b/xen/scripts/Makefile.asm-generic
@@ -31,8 +31,22 @@ generated-y := $(addprefix $(obj)/, $(generated-y))
 old-headers := $(wildcard $(obj)/*.h)
 unwanted    := $(filter-out $(generic-y) $(generated-y),$(old-headers))
 
+define header_stub
+#ifndef ASM_GENERIC_$(3)_$(2)_H
+#define ASM_GENERIC_$(3)_$(2)_H
+
+#include <asm-generic/$(1).h>
+
+#endif /* ASM_GENERIC_$(3)_$(2)_H */
+endef
+
+arch = $(shell echo $(SRCARCH) | tr a-z A-Z)
+
+header_body = $(call header_stub,$*,$(shell echo "$*" | tr a-z A-Z),$(arch))
+export header_body
+
 quiet_cmd_wrap = WRAP    $@
-      cmd_wrap = echo "\#include <asm-generic/$*.h>" > $@
+      cmd_wrap = echo "$$header_body" > $@
 
 quiet_cmd_remove = REMOVE  $(unwanted)
       cmd_remove = rm -f $(unwanted)
-- 
2.34.1

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

[Jan Beulich]

On 11.03.2024 09:59, Simone Ballarin wrote:
> --- a/xen/build.mk
> +++ b/xen/build.mk
> @@ -45,6 +45,8 @@ asm-offsets.s: arch/$(SRCARCH)/$(ARCH)/asm-offsets.c
>  	$(CC) $(call cpp_flags,$(c_flags)) -S -g0 -o $@.new -MQ $@ $<
>  	$(call move-if-changed,$@.new,$@)
>  
> +ARCHDIR = $(shell echo $(SRCARCH) | tr a-z A-Z)

Yet another instance?

> --- a/xen/scripts/Makefile.asm-generic
> +++ b/xen/scripts/Makefile.asm-generic
> @@ -31,8 +31,22 @@ generated-y := $(addprefix $(obj)/, $(generated-y))
>  old-headers := $(wildcard $(obj)/*.h)
>  unwanted    := $(filter-out $(generic-y) $(generated-y),$(old-headers))
>  
> +define header_stub
> +#ifndef ASM_GENERIC_$(3)_$(2)_H
> +#define ASM_GENERIC_$(3)_$(2)_H
> +
> +#include <asm-generic/$(1).h>
> +
> +#endif /* ASM_GENERIC_$(3)_$(2)_H */
> +endef
> +
> +arch = $(shell echo $(SRCARCH) | tr a-z A-Z)

And one more, in disguise - why not ARCHDIR here?

> +header_body = $(call header_stub,$*,$(shell echo "$*" | tr a-z A-Z),$(arch))
> +export header_body

This ought to be doable without involving a shell variable, I'd hope.
But - this is a lot of effort just to ...

>  quiet_cmd_wrap = WRAP    $@
> -      cmd_wrap = echo "\#include <asm-generic/$*.h>" > $@
> +      cmd_wrap = echo "$$header_body" > $@

... deal with an innocent header consisting of just a single #include.

Jan

[XEN PATCH v3 13/16] xen: add deviations for MISRA C.2012 Directive 4.10

[Simone Ballarin]

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

Add safe deviation for *.c files, as estabilished in past discussion.
Add SAF deviation for files that need an #include directive before guard.

Signed-off-by: Maria Celeste Cesario  <maria.celeste.cesario@bugseng.com>
Signed-off-by: Simone Ballarin  <simone.ballarin@bugseng.com>
---
Commit introduced in v3

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 +++++++
 docs/misra/safe.json                             | 8 ++++++++
 xen/include/public/arch-x86/xen.h                | 1 +
 4 files changed, 21 insertions(+)

diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl b/automation/eclair_analysis/ECLAIR/deviations.ecl
index 039ffaf52a..8082239ccc 100644
--- a/automation/eclair_analysis/ECLAIR/deviations.ecl
+++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
@@ -69,6 +69,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 ce855ddae6..7b32dbd23f 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:
 -----------------------------------------
 
diff --git a/docs/misra/safe.json b/docs/misra/safe.json
index 13208d18ec..dd61b47194 100644
--- a/docs/misra/safe.json
+++ b/docs/misra/safe.json
@@ -60,6 +60,14 @@
         },
         {
             "id": "SAF-7-safe",
+            "analyser": {
+                "eclair": "MC3R1.D4.10"
+            },
+            "name": "Dir 4.10: include needed before guard",
+            "text": "These files need to start with an include directive to generate preprocessed code in the correct order."
+        },
+        {
+            "id": "SAF-8-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 c0f4551247..0b2e9271f8 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-7-safe include before guard needed for correct code generation */
 #include "../xen.h"
 
 #ifndef __XEN_PUBLIC_ARCH_X86_XEN_H__
-- 
2.34.1

Re: [XEN PATCH v3 13/16] xen: add deviations for MISRA C.2012 Directive 4.10

[Jan Beulich]

On 11.03.2024 09:59, Simone Ballarin wrote:
> From: Maria Celeste Cesario <maria.celeste.cesario@bugseng.com>
> 
> Add safe deviation for *.c files, as estabilished in past discussion.
> Add SAF deviation for files that need an #include directive before guard.

While similar topics, the two are technically entirely different, and
hence would likely again better be split. In fact I think I might ack
the former as is, while ...

> --- 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-7-safe include before guard needed for correct code generation */
>  #include "../xen.h"
>  
>  #ifndef __XEN_PUBLIC_ARCH_X86_XEN_H__

... I fear I lack details about the need for this, not even taking into
account my earlier remark regarding the insertion of such comments in
public headers. Why is the #include needed for our own purposes (we
can't easily (re)move it entirely, as external consumers may rely on
it)? The common case is for this header to be included from ../xen.h.
In that case, aiui, the #include above has no effect at all, due to the
guard in ../xen.h. Which leaves direct inclusions of the header here.
There I'd expect a change like the one here to be accompanied by
clarification of why those few cases can't be switched to including
public/xen.h instead. Because if that was possible, doing so would
allow us to get away here without any deviation.

Jan

[XEN PATCH v3 14/16] xen/x86: address violations of MISRA C:2012 Directive 4.10

[Simone Ballarin]

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

Edit 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.
 ---
 Commit introduced in v3

 Signed-off-by: Maria Celeste Cesario  <maria.celeste.cesario@bugseng.com>
 Signed-off-by: Simone Ballarin  <simone.ballarin@bugseng.com>
---
 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 622d22bef2..cb7b01f7ef 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>
@@ -780,7 +780,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 9b19d2d389..9371784584 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 082a3d6bbc..f8139842b4 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 */
 
@@ -192,4 +192,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: [XEN PATCH v3 14/16] xen/x86: address violations of MISRA C:2012 Directive 4.10

[Jan Beulich]

On 11.03.2024 09:59, Simone Ballarin wrote:
> From: Maria Celeste Cesario <maria.celeste.cesario@bugseng.com>
> 
> Edit 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.

The changes all follow a single pattern, yet I'm afraid I can't bring
this pattern in line with this description. To take ...

> --- 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>
> @@ -780,7 +780,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 */

... this as example - what's the violation here? The existing symbol
provides the intended effect, doesn't it? What it does not is adhere
to the new naming scheme, but there's no mention of that in the
description.

Jan

Re: [XEN PATCH v3 14/16] xen/x86: address violations of MISRA C:2012 Directive 4.10

[Stefano Stabellini]

On Tue, 12 Mar 2024, Jan Beulich wrote:
> On 11.03.2024 09:59, Simone Ballarin wrote:
> > From: Maria Celeste Cesario <maria.celeste.cesario@bugseng.com>
> > 
> > Edit 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.
> 
> The changes all follow a single pattern, yet I'm afraid I can't bring
> this pattern in line with this description. To take ...
> 
> > --- 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>
> > @@ -780,7 +780,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 */
> 
> ... this as example - what's the violation here? The existing symbol
> provides the intended effect, doesn't it? What it does not is adhere
> to the new naming scheme, but there's no mention of that in the
> description.

I think the commit message is incorrect. As you pointed out there is no
underlying MISRA issue. However, it does bring the headers inline with
the new naming scheme, which I think it is a great improvement.

With the commit message improved:

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

[XEN PATCH v3 15/16] x86/mtrr: address violations of MISRA C:2012 Directive 4.10

[Simone Ballarin]

From: Maria Celeste Cesario <maria.celeste.cesario@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.
---
Commit introduced in v3

Signed-off-by: Maria Celeste Cesario  <maria.celeste.cesario@bugseng.com>
Signed-off-by: Simone Ballarin  <simone.ballarin@bugseng.com>
---
 xen/arch/x86/cpu/mtrr/mtrr.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/xen/arch/x86/cpu/mtrr/mtrr.h b/xen/arch/x86/cpu/mtrr/mtrr.h
index a9741e0cb0..632bf658be 100644
--- a/xen/arch/x86/cpu/mtrr/mtrr.h
+++ b/xen/arch/x86/cpu/mtrr/mtrr.h
@@ -1,6 +1,8 @@
 /*
  * local mtrr defines.
  */
+#ifndef X86_CPU_MTRR_MTRR_H
+#define X86_CPU_MTRR_MTRR_H
 
 #define MTRR_CHANGE_MASK_FIXED     0x01
 #define MTRR_CHANGE_MASK_VARIABLE  0x02
@@ -25,3 +27,5 @@ extern u64 size_or_mask, size_and_mask;
 extern unsigned int num_var_ranges;
 
 void mtrr_state_warn(void);
+
+#endif /* X86_CPU_MTRR_MTRR_H */
-- 
2.34.1

Re: [XEN PATCH v3 15/16] x86/mtrr: address violations of MISRA C:2012 Directive 4.10

[Jan Beulich]

On 11.03.2024 09:59, Simone Ballarin wrote:
> From: Maria Celeste Cesario <maria.celeste.cesario@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.
> ---
> Commit introduced in v3
> 
> Signed-off-by: Maria Celeste Cesario  <maria.celeste.cesario@bugseng.com>
> Signed-off-by: Simone Ballarin  <simone.ballarin@bugseng.com>

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

Jan

Re: [XEN PATCH v3 15/16] x86/mtrr: address violations of MISRA C:2012 Directive 4.10

[Jan Beulich]

On 11.03.2024 09:59, Simone Ballarin wrote:
> From: Maria Celeste Cesario <maria.celeste.cesario@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.
> ---
> Commit introduced in v3

I almost hadn't noticed this misplaced revlog (also in patch 16); with
it kept in place I expect git would have zapped ...

> Signed-off-by: Maria Celeste Cesario  <maria.celeste.cesario@bugseng.com>
> Signed-off-by: Simone Ballarin  <simone.ballarin@bugseng.com>

... all tags.

Jan

[XEN PATCH v3 16/16] xen/lz4: address violations of MISRA C:2012 Directive 4.10

[Simone Ballarin]

From: Maria Celeste Cesario <maria.celeste.cesario@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.
---
Commit introduced in v3

Signed-off-by: Maria Celeste Cesario  <maria.celeste.cesario@bugseng.com>
Signed-off-by: Simone Ballarin  <simone.ballarin@bugseng.com>
---
 xen/common/lz4/defs.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/xen/common/lz4/defs.h b/xen/common/lz4/defs.h
index 6d81113266..ecfbf07f83 100644
--- a/xen/common/lz4/defs.h
+++ b/xen/common/lz4/defs.h
@@ -8,6 +8,9 @@
  * published by the Free Software Foundation.
  */
 
+#ifndef COMMON_LZ4_DEFS_H
+#define COMMON_LZ4_DEFS_H
+
 #ifdef __XEN__
 #include <asm/byteorder.h>
 #include <xen/unaligned.h>
@@ -166,3 +169,5 @@ typedef struct _U64_S { u64 v; } U64_S;
 		LZ4_WILDCOPY(s, d, e);	\
 		d = e;	\
 	} while (0)
+
+#endif /* COMMON_LZ4_DEFS_H */
-- 
2.34.1

Re: [XEN PATCH v3 16/16] xen/lz4: address violations of MISRA C:2012 Directive 4.10

[Jan Beulich]

On 11.03.2024 09:59, Simone Ballarin wrote:
> From: Maria Celeste Cesario <maria.celeste.cesario@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.
> ---
> Commit introduced in v3
> 
> Signed-off-by: Maria Celeste Cesario  <maria.celeste.cesario@bugseng.com>
> Signed-off-by: Simone Ballarin  <simone.ballarin@bugseng.com>

Once again somewhat reluctantly
Acked-by: Jan Beulich <jbeulich@suse.com>

Jan

Re: [XEN PATCH v3 00/16] xen: address violation of MISRA C:2012 Directive 4.10

[Jan Beulich]

On 11.03.2024 09:59, Simone Ballarin wrote:
> 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".
> 
> As stated in v2, the following naming convention has been estabilished:
> - arch/.../include/asm/ headers -> ASM_<filename>_H
> - private headers -> <dir>_<filename>_H
> - asm-generic headers -> ASM_GENERIC_<filename>_H
> 
> Since there would have been conflicting guards between architectures (which is a violation
> of the directive),

Why would this be a violation? The two sets of headers aren't use together,
and any scan should only point out collisions in what's actively used, I'd
hope.

> there has been a need for ASM headers to specify  if the inclusion guard
> referred to ARM or X86. Hence it has been decided to adopt instead:
> - arch/<architecture>/include/asm/<subdir>/<filename>.h -> ASM_<architecture>_<subdir>_<filename>_H
> 
> The subdir used is the smallest possible to avoid collisions. For example, it has been
> observed that in "xen/arch/arm/include/asm/arm32" and "xen/arch/arm/include/asm/arm64" there
> are plenty of header files with the same name, hence  _ARMxx_ was added as subdirectory.

Why couldn't <arch>_<subdir> together be ARMxx there, allowing us to get
away with one less name component.

> There has been a need to define a standard for generated headers too:
>  - include/generated/<subdir>/<filename>.h-> GENERATED_<subdir>_<filename>_H
>  - arch/<architecture>/include/generated/asm/<filename>.h-> <arch>_GENERATED_ASM_<name>_H
> 
> To summarize, here are all the rules that have been applied:
> - private headers -> <dir>_<filename>_H

And <dir> here is the full path? I thought I had indicated before that
this is going to lead to excessively long identifiers.

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

And _ASM_ is merely a precaution for stuff appearing outside of asm/ (which
doesn't seem very likely)?

Jan

Re: [XEN PATCH v3 00/16] xen: address violation of MISRA C:2012 Directive 4.10

[Simone Ballarin]

On 11/03/24 10:59, Jan Beulich wrote:
> On 11.03.2024 09:59, Simone Ballarin wrote:
>> 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".
>>
>> As stated in v2, the following naming convention has been estabilished:
>> - arch/.../include/asm/ headers -> ASM_<filename>_H
>> - private headers -> <dir>_<filename>_H
>> - asm-generic headers -> ASM_GENERIC_<filename>_H
>>
>> Since there would have been conflicting guards between architectures (which is a violation
>> of the directive),
> 
> Why would this be a violation? The two sets of headers aren't use together,
> and any scan should only point out collisions in what's actively used, I'd
> hope.
> 

A header guard cannot be used multiple times in the same project, so it's a question
of whether or not we want to consider each arch variant part of the same project.
In case, we can define a new rule for these files.

>> there has been a need for ASM headers to specify  if the inclusion guard
>> referred to ARM or X86. Hence it has been decided to adopt instead:
>> - arch/<architecture>/include/asm/<subdir>/<filename>.h -> ASM_<architecture>_<subdir>_<filename>_H
>>
>> The subdir used is the smallest possible to avoid collisions. For example, it has been
>> observed that in "xen/arch/arm/include/asm/arm32" and "xen/arch/arm/include/asm/arm64" there
>> are plenty of header files with the same name, hence  _ARMxx_ was added as subdirectory.
> 
> Why couldn't <arch>_<subdir> together be ARMxx there, allowing us to get
> away with one less name component.
> 

I agree.

>> There has been a need to define a standard for generated headers too:
>>   - include/generated/<subdir>/<filename>.h-> GENERATED_<subdir>_<filename>_H
>>   - arch/<architecture>/include/generated/asm/<filename>.h-> <arch>_GENERATED_ASM_<name>_H
>>
>> To summarize, here are all the rules that have been applied:
>> - private headers -> <dir>_<filename>_H
> 
> And <dir> here is the full path? I thought I had indicated before that
> this is going to lead to excessively long identifiers.
> 

Yes, dir is the full path. This general fallback rule to use when more specific rules do not apply.
If this generates excessively long guards, we can simply add more rules.
>> - 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
> 
> And _ASM_ is merely a precaution for stuff appearing outside of asm/ (which
> doesn't seem very likely)?
> 

Yes, it is. Do you prefer dropping the _ARM_?

> Jan

-- 
Simone Ballarin, M.Sc.

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

Re: [XEN PATCH v3 00/16] xen: address violation of MISRA C:2012 Directive 4.10

[Jan Beulich]

On 11.03.2024 12:41, Simone Ballarin wrote:
> On 11/03/24 10:59, Jan Beulich wrote:
>> On 11.03.2024 09:59, Simone Ballarin wrote:
>>> 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".
>>>
>>> As stated in v2, the following naming convention has been estabilished:
>>> - arch/.../include/asm/ headers -> ASM_<filename>_H
>>> - private headers -> <dir>_<filename>_H
>>> - asm-generic headers -> ASM_GENERIC_<filename>_H
>>>
>>> Since there would have been conflicting guards between architectures (which is a violation
>>> of the directive),
>>
>> Why would this be a violation? The two sets of headers aren't use together,
>> and any scan should only point out collisions in what's actively used, I'd
>> hope.
>>
> 
> A header guard cannot be used multiple times in the same project, so it's a question
> of whether or not we want to consider each arch variant part of the same project.
> In case, we can define a new rule for these files.
> 
>>> there has been a need for ASM headers to specify  if the inclusion guard
>>> referred to ARM or X86. Hence it has been decided to adopt instead:
>>> - arch/<architecture>/include/asm/<subdir>/<filename>.h -> ASM_<architecture>_<subdir>_<filename>_H
>>>
>>> The subdir used is the smallest possible to avoid collisions. For example, it has been
>>> observed that in "xen/arch/arm/include/asm/arm32" and "xen/arch/arm/include/asm/arm64" there
>>> are plenty of header files with the same name, hence  _ARMxx_ was added as subdirectory.
>>
>> Why couldn't <arch>_<subdir> together be ARMxx there, allowing us to get
>> away with one less name component.
>>
> 
> I agree.
> 
>>> There has been a need to define a standard for generated headers too:
>>>   - include/generated/<subdir>/<filename>.h-> GENERATED_<subdir>_<filename>_H
>>>   - arch/<architecture>/include/generated/asm/<filename>.h-> <arch>_GENERATED_ASM_<name>_H
>>>
>>> To summarize, here are all the rules that have been applied:
>>> - private headers -> <dir>_<filename>_H
>>
>> And <dir> here is the full path? I thought I had indicated before that
>> this is going to lead to excessively long identifiers.
>>
> 
> Yes, dir is the full path. This general fallback rule to use when more specific rules do not apply.
> If this generates excessively long guards, we can simply add more rules.
>>> - 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
>>
>> And _ASM_ is merely a precaution for stuff appearing outside of asm/ (which
>> doesn't seem very likely)?
> 
> Yes, it is. Do you prefer dropping the _ARM_?

Well, I prefer any measure keeping down the length of these identifiers.

Jan