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

[Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>]

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