| Message ID | dd042e7d17e7833e12a5ff6f28dd560b5ff02cf7.1710145041.git.simone.ballarin@bugseng.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen: address violation of MISRA C:2012 Directive 4.10 | expand |
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@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
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.
On 25.06.2024 21:31, Nicola Vetrini wrote: > 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. Hmm, I'm puzzled: The example you point at presently has no guard at all, afaics. There'll need to be churn there anyway. If you picked an example, I would have expected that to be one where the guard already fully matches the proposed scheme. To be honest I'd be surprised if we had many files fulfilling this criteria. Jan
On 2024-06-26 10:20, Jan Beulich wrote: > On 25.06.2024 21:31, Nicola Vetrini wrote: >> 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. > > Hmm, I'm puzzled: The example you point at presently has no guard at > all, > afaics. There'll need to be churn there anyway. If you picked an > example, > I would have expected that to be one where the guard already fully > matches the proposed scheme. To be honest I'd be surprised if we had > many > files fulfilling this criteria. > Ah, yes. I was looking at the state of the tree after some patches applied. On staging, most examples are in asm directories, such as xen/arch/x86/include/asm/endbr.h, but that would be modified anyway by dropping rule #3: arch/<architecture>/include/asm/<subdir>/<filename>.h -> ASM_<architecture>_<subdir>_<filename>_H With this, unless I find some other obstacle or issue from other maintainers, the XEN_ prefix can be dropped.
On 25.06.2024 21:31, Nicola Vetrini wrote: > 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. If $(ARCHDIR) is going to be used elsewhere, then what you suggest is fine. My "whether" in the earlier reply specifically left open for clarification what the intentions with the variable are. The alternative I had described makes sense only when $(ARCHDIR) would only ever be used inside the filechk_asm-macros.h macro. Jan
On 2024-06-26 11:06, Jan Beulich wrote: > On 25.06.2024 21:31, Nicola Vetrini wrote: >> 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. > > If $(ARCHDIR) is going to be used elsewhere, then what you suggest is > fine. > My "whether" in the earlier reply specifically left open for > clarification > what the intentions with the variable are. The alternative I had > described > makes sense only when $(ARCHDIR) would only ever be used inside the > filechk_asm-macros.h macro. > Yes, the intention is to reuse $(ARCHDIR) in the formation of other places, as you can tell from the fact that subsequent patches replicate the same pattern. This is going to save some duplication. The only matter left then is whether xen/Makefile (around line 250, just after setting SRCARCH) would be better, or Kbuild.include. To me the former place seems more natural, but I'm not totally sure.
On 26.06.2024 11:20, Nicola Vetrini wrote: > On 2024-06-26 11:06, Jan Beulich wrote: >> On 25.06.2024 21:31, Nicola Vetrini wrote: >>> 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. >> >> If $(ARCHDIR) is going to be used elsewhere, then what you suggest is >> fine. >> My "whether" in the earlier reply specifically left open for >> clarification >> what the intentions with the variable are. The alternative I had >> described >> makes sense only when $(ARCHDIR) would only ever be used inside the >> filechk_asm-macros.h macro. > > Yes, the intention is to reuse $(ARCHDIR) in the formation of other > places, as you can tell from the fact that subsequent patches replicate > the same pattern. This is going to save some duplication. > The only matter left then is whether xen/Makefile (around line 250, just > after setting SRCARCH) would be better, or Kbuild.include. To me the > former place seems more natural, but I'm not totally sure. Depends on where all the intended uses are. If they're all in xen/Makefile, then having the macro just there is of course sufficient. Whereas when it's needed elsewhere, instead of exporting putting it in Kbuild.include would seem more natural / desirable to me. Jan
On 2024-06-26 11:26, Jan Beulich wrote: > On 26.06.2024 11:20, Nicola Vetrini wrote: >> On 2024-06-26 11:06, Jan Beulich wrote: >>> On 25.06.2024 21:31, Nicola Vetrini wrote: >>>> 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. >>> >>> If $(ARCHDIR) is going to be used elsewhere, then what you suggest is >>> fine. >>> My "whether" in the earlier reply specifically left open for >>> clarification >>> what the intentions with the variable are. The alternative I had >>> described >>> makes sense only when $(ARCHDIR) would only ever be used inside the >>> filechk_asm-macros.h macro. >> >> Yes, the intention is to reuse $(ARCHDIR) in the formation of other >> places, as you can tell from the fact that subsequent patches >> replicate >> the same pattern. This is going to save some duplication. >> The only matter left then is whether xen/Makefile (around line 250, >> just >> after setting SRCARCH) would be better, or Kbuild.include. To me the >> former place seems more natural, but I'm not totally sure. > > Depends on where all the intended uses are. If they're all in > xen/Makefile, > then having the macro just there is of course sufficient. Whereas when > it's > needed elsewhere, instead of exporting putting it in Kbuild.include > would > seem more natural / desirable to me. > The places where this would be used are these: file: target (or define) xen/build.mk: arch/$(SRCARCH)/include/asm/asm-offsets.h: asm-offsets.s xen/include/Makefile: define cmd_xlat_h xen/arch/x86/Makefile: define filechk_asm-macros.h The only issue that comes to my mind (it may not be one at all) is that SRCARCH is defined and exported in xen/Makefile after including Kbuild.include, so it would need to be defined after SRCARCH is assigned: include scripts/Kbuild.include # Don't break if the build process wasn't called from the top level # we need XEN_TARGET_ARCH to generate the proper config include $(XEN_ROOT)/Config.mk # Set ARCH/SRCARCH appropriately. ARCH := $(XEN_TARGET_ARCH) SRCARCH := $(shell echo $(ARCH) | \ sed -e 's/x86.*/x86/' -e 's/arm\(32\|64\)/arm/g' \ -e 's/riscv.*/riscv/g' -e 's/ppc.*/ppc/g') export ARCH SRCARCH Am I missing something?
On 26.06.2024 12:25, Nicola Vetrini wrote: > On 2024-06-26 11:26, Jan Beulich wrote: >> On 26.06.2024 11:20, Nicola Vetrini wrote: >>> On 2024-06-26 11:06, Jan Beulich wrote: >>>> On 25.06.2024 21:31, Nicola Vetrini wrote: >>>>> 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. >>>> >>>> If $(ARCHDIR) is going to be used elsewhere, then what you suggest is >>>> fine. >>>> My "whether" in the earlier reply specifically left open for >>>> clarification >>>> what the intentions with the variable are. The alternative I had >>>> described >>>> makes sense only when $(ARCHDIR) would only ever be used inside the >>>> filechk_asm-macros.h macro. >>> >>> Yes, the intention is to reuse $(ARCHDIR) in the formation of other >>> places, as you can tell from the fact that subsequent patches >>> replicate >>> the same pattern. This is going to save some duplication. >>> The only matter left then is whether xen/Makefile (around line 250, >>> just >>> after setting SRCARCH) would be better, or Kbuild.include. To me the >>> former place seems more natural, but I'm not totally sure. >> >> Depends on where all the intended uses are. If they're all in >> xen/Makefile, >> then having the macro just there is of course sufficient. Whereas when >> it's >> needed elsewhere, instead of exporting putting it in Kbuild.include >> would >> seem more natural / desirable to me. >> > > The places where this would be used are these: > file: target (or define) > xen/build.mk: arch/$(SRCARCH)/include/asm/asm-offsets.h: asm-offsets.s > xen/include/Makefile: define cmd_xlat_h > xen/arch/x86/Makefile: define filechk_asm-macros.h > > The only issue that comes to my mind (it may not be one at all) is that > SRCARCH is defined and exported in xen/Makefile after including > Kbuild.include, so it would need to be defined after SRCARCH is > assigned: > > include scripts/Kbuild.include > > # Don't break if the build process wasn't called from the top level > # we need XEN_TARGET_ARCH to generate the proper config > include $(XEN_ROOT)/Config.mk > > # Set ARCH/SRCARCH appropriately. > > ARCH := $(XEN_TARGET_ARCH) > SRCARCH := $(shell echo $(ARCH) | \ > sed -e 's/x86.*/x86/' -e 's/arm\(32\|64\)/arm/g' \ > -e 's/riscv.*/riscv/g' -e 's/ppc.*/ppc/g') > export ARCH SRCARCH > > Am I missing something? In that case the alternatives are exporting or using = rather than := in Kbuild.include, i.e. other than initially requested. Personally I dislike exporting to a fair degree, but I'm not sure which one's better in this case. Cc-ing Anthony for possible input. Jan
On Wed, Jun 26, 2024 at 12:31:42PM +0200, Jan Beulich wrote: > On 26.06.2024 12:25, Nicola Vetrini wrote: > > On 2024-06-26 11:26, Jan Beulich wrote: > >> On 26.06.2024 11:20, Nicola Vetrini wrote: > >>> On 2024-06-26 11:06, Jan Beulich wrote: > >>>> On 25.06.2024 21:31, Nicola Vetrini wrote: > >>>>> 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. > >>>> > >>>> If $(ARCHDIR) is going to be used elsewhere, then what you suggest is > >>>> fine. > >>>> My "whether" in the earlier reply specifically left open for > >>>> clarification > >>>> what the intentions with the variable are. The alternative I had > >>>> described > >>>> makes sense only when $(ARCHDIR) would only ever be used inside the > >>>> filechk_asm-macros.h macro. > >>> > >>> Yes, the intention is to reuse $(ARCHDIR) in the formation of other > >>> places, as you can tell from the fact that subsequent patches > >>> replicate > >>> the same pattern. This is going to save some duplication. > >>> The only matter left then is whether xen/Makefile (around line 250, > >>> just > >>> after setting SRCARCH) would be better, or Kbuild.include. To me the > >>> former place seems more natural, but I'm not totally sure. > >> > >> Depends on where all the intended uses are. If they're all in > >> xen/Makefile, > >> then having the macro just there is of course sufficient. Whereas when > >> it's > >> needed elsewhere, instead of exporting putting it in Kbuild.include > >> would > >> seem more natural / desirable to me. > >> > > > > The places where this would be used are these: > > file: target (or define) > > xen/build.mk: arch/$(SRCARCH)/include/asm/asm-offsets.h: asm-offsets.s > > xen/include/Makefile: define cmd_xlat_h > > xen/arch/x86/Makefile: define filechk_asm-macros.h > > > > The only issue that comes to my mind (it may not be one at all) is that > > SRCARCH is defined and exported in xen/Makefile after including > > Kbuild.include, so it would need to be defined after SRCARCH is > > assigned: > > > > include scripts/Kbuild.include > > > > # Don't break if the build process wasn't called from the top level > > # we need XEN_TARGET_ARCH to generate the proper config > > include $(XEN_ROOT)/Config.mk > > > > # Set ARCH/SRCARCH appropriately. > > > > ARCH := $(XEN_TARGET_ARCH) > > SRCARCH := $(shell echo $(ARCH) | \ > > sed -e 's/x86.*/x86/' -e 's/arm\(32\|64\)/arm/g' \ > > -e 's/riscv.*/riscv/g' -e 's/ppc.*/ppc/g') > > export ARCH SRCARCH > > > > Am I missing something? > > In that case the alternatives are exporting or using = rather than := in > Kbuild.include, i.e. other than initially requested. Personally I dislike > exporting to a fair degree, but I'm not sure which one's better in this > case. Cc-ing Anthony for possible input. None. The name is missleading anyway, it would suggest to me that it contain a directory, but that's wrong. Another thing that suboptimal, use make to call a shell to generate a string that is going to be later use in shell context. How about just doing the work in that later shell context? Something like: define filechk_asm-macros.h + guard=$$(echo ASM_${SRCARCH}_ASM_MACROS_H | tr a-z A-Z); \ + echo "#ifndef $$guard"; \ + echo "#define $$guard"; \ echo '#if 0'; \ echo '.if 0'; \ Or, instead of having to write the name of the file down, we could use a name that is already registered in a variable: define filechk_asm-macros.h + guard=$$(echo $@ | tr a-z/.- A-Z_); \ + echo "#ifndef $$guard"; \ + echo "#define $$guard"; \ echo '#if 0'; \ echo '.if 0'; \ This produces: #ifndef ARCH_X86_INCLUDE_ASM_ASM_MACROS_H #define ARCH_X86_INCLUDE_ASM_ASM_MACROS_H #if 0 .if 0 Cheers,
On 2024-06-26 15:38, Anthony PERARD wrote: > On Wed, Jun 26, 2024 at 12:31:42PM +0200, Jan Beulich wrote: >> On 26.06.2024 12:25, Nicola Vetrini wrote: >> > On 2024-06-26 11:26, Jan Beulich wrote: >> >> On 26.06.2024 11:20, Nicola Vetrini wrote: >> >>> On 2024-06-26 11:06, Jan Beulich wrote: >> >>>> On 25.06.2024 21:31, Nicola Vetrini wrote: >> >>>>> 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. >> >>>> >> >>>> If $(ARCHDIR) is going to be used elsewhere, then what you suggest is >> >>>> fine. >> >>>> My "whether" in the earlier reply specifically left open for >> >>>> clarification >> >>>> what the intentions with the variable are. The alternative I had >> >>>> described >> >>>> makes sense only when $(ARCHDIR) would only ever be used inside the >> >>>> filechk_asm-macros.h macro. >> >>> >> >>> Yes, the intention is to reuse $(ARCHDIR) in the formation of other >> >>> places, as you can tell from the fact that subsequent patches >> >>> replicate >> >>> the same pattern. This is going to save some duplication. >> >>> The only matter left then is whether xen/Makefile (around line 250, >> >>> just >> >>> after setting SRCARCH) would be better, or Kbuild.include. To me the >> >>> former place seems more natural, but I'm not totally sure. >> >> >> >> Depends on where all the intended uses are. If they're all in >> >> xen/Makefile, >> >> then having the macro just there is of course sufficient. Whereas when >> >> it's >> >> needed elsewhere, instead of exporting putting it in Kbuild.include >> >> would >> >> seem more natural / desirable to me. >> >> >> > >> > The places where this would be used are these: >> > file: target (or define) >> > xen/build.mk: arch/$(SRCARCH)/include/asm/asm-offsets.h: asm-offsets.s >> > xen/include/Makefile: define cmd_xlat_h >> > xen/arch/x86/Makefile: define filechk_asm-macros.h >> > >> > The only issue that comes to my mind (it may not be one at all) is that >> > SRCARCH is defined and exported in xen/Makefile after including >> > Kbuild.include, so it would need to be defined after SRCARCH is >> > assigned: >> > >> > include scripts/Kbuild.include >> > >> > # Don't break if the build process wasn't called from the top level >> > # we need XEN_TARGET_ARCH to generate the proper config >> > include $(XEN_ROOT)/Config.mk >> > >> > # Set ARCH/SRCARCH appropriately. >> > >> > ARCH := $(XEN_TARGET_ARCH) >> > SRCARCH := $(shell echo $(ARCH) | \ >> > sed -e 's/x86.*/x86/' -e 's/arm\(32\|64\)/arm/g' \ >> > -e 's/riscv.*/riscv/g' -e 's/ppc.*/ppc/g') >> > export ARCH SRCARCH >> > >> > Am I missing something? >> >> In that case the alternatives are exporting or using = rather than := >> in >> Kbuild.include, i.e. other than initially requested. Personally I >> dislike >> exporting to a fair degree, but I'm not sure which one's better in >> this >> case. Cc-ing Anthony for possible input. > > None. The name is missleading anyway, it would suggest to me that it > contain a directory, but that's wrong. > > Another thing that suboptimal, use make to call a shell to generate a > string that is going to be later use in shell context. How about just > doing the work in that later shell context? > > Something like: > > define filechk_asm-macros.h > + guard=$$(echo ASM_${SRCARCH}_ASM_MACROS_H | tr a-z A-Z); \ > + echo "#ifndef $$guard"; \ > + echo "#define $$guard"; \ > echo '#if 0'; \ > echo '.if 0'; \ > This approach looks ok to me. > Or, instead of having to write the name of the file down, we could > use a name that is already registered in a variable: > > define filechk_asm-macros.h > + guard=$$(echo $@ | tr a-z/.- A-Z_); \ > + echo "#ifndef $$guard"; \ > + echo "#define $$guard"; \ > echo '#if 0'; \ > echo '.if 0'; \ > > This produces: > #ifndef ARCH_X86_INCLUDE_ASM_ASM_MACROS_H > #define ARCH_X86_INCLUDE_ASM_ASM_MACROS_H > #if 0 > .if 0 > > Cheers, The issue I see here is that it would in some cases lead to long header guards, which is somewhat against the overall consensus, given that the naming convention should be followed by any file, so it was designed to generate shorter guards, rather than just the path.
diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile index 26d8740529..4548cdc53d 100644 --- 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) + 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 $(obj)/efi.lds: AFLAGS-y += -DEFI diff --git a/xen/arch/x86/cpu/cpu.h b/xen/arch/x86/cpu/cpu.h index e3d06278b3..b763cdf3da 100644 --- 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 */ diff --git a/xen/arch/x86/x86_64/mmconfig.h b/xen/arch/x86/x86_64/mmconfig.h index 3da4b21e9b..c8e67a67db 100644 --- 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 */ diff --git a/xen/arch/x86/x86_emulate/private.h b/xen/arch/x86/x86_emulate/private.h index 0fa26ba00a..835161cb53 100644 --- 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 */
Add or move inclusion guards to address violations of MISRA C:2012 Directive 4.10 ("Precautions shall be taken in order to prevent the contents of a header file being included more than once"). Inclusion guards must appear at the beginning of the headers (comments are permitted anywhere). Mechanical change. Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com> Signed-off-by: Maria Celeste Cesario <maria.celeste.cesario@bugseng.com> --- Changes in v3: - remove trailing underscores - change inclusion guard name to adhere to the new standard Changes in v2: - remove extra blanks - drop changes in C files Note: Changes in Makefile were not strictly necessary in v1 and a maintainer asked to remove them since there was a deviation for generated headers. Starting from v2, they are required since the deviation has been removed by another patch of this series. --- xen/arch/x86/Makefile | 10 ++++++---- xen/arch/x86/cpu/cpu.h | 5 +++++ xen/arch/x86/x86_64/mmconfig.h | 5 +++++ xen/arch/x86/x86_emulate/private.h | 5 +++++ 4 files changed, 21 insertions(+), 4 deletions(-)