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

[Jan Beulich <jbeulich@xxxxxxxx>]

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