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

[Jan Beulich <jbeulich@xxxxxxxx>]

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