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