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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Tue, 12 Sep 2023, Simone Ballarin wrote:
> 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@xxxxxxxxxxx>
> 
> ---
> 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                   | 8 ++++++--
>  5 files changed, 25 insertions(+), 6 deletions(-)
> 
> diff --git a/docs/misra/safe.json b/docs/misra/safe.json
> index 0ec594f6bf..82c636ee94 100644
> --- a/docs/misra/safe.json
> +++ b/docs/misra/safe.json
> @@ -44,6 +44,14 @@
>          },
>          {
>              "id": "SAF-5-safe",

You might want to double-check the SAF id here as well


> +            "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-6-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..3d3891d061 100644
> --- a/xen/arch/x86/include/asm/compat.h
> +++ b/xen/arch/x86/include/asm/compat.h
> @@ -2,6 +2,9 @@
>   * compat.h
>   */
>  
> +#ifndef __ASM_X86_COMPAT_H__
> +#define __ASM_X86_COMPAT_H__
> +
>  #ifdef CONFIG_COMPAT
>  
>  #define COMPAT_BITS_PER_LONG 32
> @@ -18,3 +21,5 @@ int switch_compat(struct domain *);
>  #include <xen/errno.h>
>  static inline int switch_compat(struct domain *d) { return -EOPNOTSUPP; }
>  #endif
> +
> +#endif /* __ASM_X86_COMPAT_H__ */
> diff --git a/xen/arch/x86/include/asm/cpufeatures.h 
> b/xen/arch/x86/include/asm/cpufeatures.h
> index da0593de85..39b15e463a 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-4-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..f2eb8b5496 100644
> --- a/xen/arch/x86/include/asm/efibind.h
> +++ b/xen/arch/x86/include/asm/efibind.h
> @@ -1,2 +1,7 @@
> +#ifndef __ASM_X86_EFIBIND_H__
> +#define __ASM_X86_EFIBIND_H__
> +
>  #include <xen/types.h>
>  #include <asm/x86_64/efibind.h>
> +
> +#endif /* __ASM_X86_EFIBIND_H__ */
> diff --git a/xen/include/Makefile b/xen/include/Makefile
> index 31782fb177..b2f9576362 100644
> --- a/xen/include/Makefile
> +++ b/xen/include/Makefile
> @@ -105,9 +105,13 @@ xlat-y := $(shell sed -ne 's,@arch@,$(compat-arch-y),g' 
> -re 's,^[?!][[:blank:]]+
>  xlat-y := $(filter $(patsubst compat/%,%,$(headers-y)),$(xlat-y))
>  
>  quiet_cmd_xlat_h = GEN     $@
> -cmd_xlat_h = \
> -     cat $(filter %.h,$^) >$@.new; \
> +define cmd_xlat_h
> +     echo "#ifndef _COMPAT_XLAT_H" > $@.new; \
> +     echo "#define _COMPAT_XLAT_H" >> $@.new; \
> +     cat $(filter %.h,$^) >> $@.new; \
> +     echo "#endif /* _COMPAT_XLAT_H */" >> $@.new; \
>       mv -f $@.new $@
> +endef
>  
>  $(obj)/compat/xlat.h: $(addprefix $(obj)/compat/.xlat/,$(xlat-y)) FORCE
>       $(call if_changed,xlat_h)

I checked that everything works as expected with this change and it does. I am 
not
sure about the choice of name "_COMPAT_XLAT_H" but anyway:

Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>