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

[Nicola Vetrini <nicola.vetrini@bugseng.com>]

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)