| Message ID | 20210824105038.1257926-18-anthony.perard@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen: Build system improvements, now with out-of-tree build! | expand |
A general remark first: If I understand things correctly, a side effect of this change is to also address the issue that I'm trying to take care of in "x86/build: suppress EFI-related tool chain checks upon local $(MAKE) recursion". However, while that one's a reasonable backporting candidate, I don't think the one here is. Therefore I'd prefer my patch to go in ahead of this change of yours. Hence I wonder whether in return I couldn't ask you to review that one. On 24.08.2021 12:50, Anthony PERARD wrote: > We are going to need the variable XEN_BUILD_EFI earlier. > > But a side effect of calculating the value of $(XEN_BUILD_EFI) is to > also to generate "efi/check.o" which is used for further checks. > Thus the whole chain that check for EFI support is moved to > "arch.mk". > > Some other changes are made to avoid too much duplication: > - $(efi-check-o): Used to avoid repeating "efi/check.*". We don't > set it to the path to the source as it would be wrong as soon > as we support out-of-tree build. > - $(LD_PE_check_cmd): As it is called twice, with an updated > $(EFI_LDFLAGS). > > $(nr-fixups) is renamed to $(efi-check-relocs) as the former might be > a bit too generic. While I don't mind the prefix addition, may I please ask that the rest of the name remain as is, i.e. $(efi-nr-fixups)? "nr" because that's what the variable holds, and "fixups" to distinguish from full-fledged relocations as well as to match commentary there. > --- a/xen/arch/x86/Makefile > +++ b/xen/arch/x86/Makefile > @@ -123,41 +123,7 @@ $(TARGET): $(TARGET)-syms $(efi-y) boot/mkelf32 > mv $(TMP) $(TARGET) > > ifneq ($(efi-y),) > - > -# Check if the compiler supports the MS ABI. > -export XEN_BUILD_EFI := $(shell $(CC) $(XEN_CFLAGS) -c efi/check.c -o efi/check.o 2>/dev/null && echo y) > CFLAGS-$(XEN_BUILD_EFI) += -DXEN_BUILD_EFI > - > -# Check if the linker supports PE. > -EFI_LDFLAGS = $(patsubst -m%,-mi386pep,$(XEN_LDFLAGS)) --subsystem=10 > -XEN_BUILD_PE := $(if $(XEN_BUILD_EFI),$(call ld-option,$(EFI_LDFLAGS) --image-base=0x100000000 -o efi/check.efi efi/check.o)) > -# If the above failed, it may be merely because of the linker not dealing well > -# with debug info. Try again with stripping it. > -ifeq ($(CONFIG_DEBUG_INFO)-$(XEN_BUILD_PE),y-n) > -EFI_LDFLAGS += --strip-debug > -XEN_BUILD_PE := $(call ld-option,$(EFI_LDFLAGS) --image-base=0x100000000 -o efi/check.efi efi/check.o) > -endif > - > -ifeq ($(XEN_BUILD_PE),y) > - > -# Check if the linker produces fixups in PE by default > -nr-fixups := $(shell $(OBJDUMP) -p efi/check.efi | grep '^[[:blank:]]*reloc[[:blank:]]*[0-9][[:blank:]].*DIR64$$' | wc -l) > -ifeq ($(nr-fixups),2) > -MKRELOC := : > -relocs-dummy := > -else > -MKRELOC := efi/mkreloc > -relocs-dummy := efi/relocs-dummy.o > -# If the linker produced fixups but not precisely two of them, we need to > -# disable it doing so. But if it didn't produce any fixups, it also wouldn't > -# recognize the option. > -ifneq ($(nr-fixups),0) > -EFI_LDFLAGS += --disable-reloc-section > -endif > -endif > - > -endif # $(XEN_BUILD_PE) > - > endif # $(efi-y) Is the remaining if(,) block still warranted? I.e. can't the single line CFLAGS-$(XEN_BUILD_EFI) += -DXEN_BUILD_EFI live without the surrounding conditional? > --- a/xen/arch/x86/arch.mk > +++ b/xen/arch/x86/arch.mk > @@ -60,5 +60,47 @@ ifeq ($(CONFIG_UBSAN),y) > $(call cc-option-add,CFLAGS_UBSAN,CC,-fno-sanitize=alignment) > endif > > +ifneq ($(CONFIG_PV_SHIM_EXCLUSIVE),y) > + > +efi-check-o = arch/x86/efi/check.o Nit: Unless there's a reason not to, please prefer := here (and in general). Jan
On Thu, Oct 07, 2021 at 06:14:33PM +0200, Jan Beulich wrote: > On 24.08.2021 12:50, Anthony PERARD wrote: > > $(nr-fixups) is renamed to $(efi-check-relocs) as the former might be > > a bit too generic. > > While I don't mind the prefix addition, may I please ask that the rest > of the name remain as is, i.e. $(efi-nr-fixups)? "nr" because that's > what the variable holds, and "fixups" to distinguish from full-fledged > relocations as well as to match commentary there. Will change. > > --- a/xen/arch/x86/Makefile > > +++ b/xen/arch/x86/Makefile > > @@ -123,41 +123,7 @@ $(TARGET): $(TARGET)-syms $(efi-y) boot/mkelf32 > > mv $(TMP) $(TARGET) > > > > ifneq ($(efi-y),) > > - > > -# Check if the compiler supports the MS ABI. > > -export XEN_BUILD_EFI := $(shell $(CC) $(XEN_CFLAGS) -c efi/check.c -o efi/check.o 2>/dev/null && echo y) > > CFLAGS-$(XEN_BUILD_EFI) += -DXEN_BUILD_EFI > > - > > -# Check if the linker supports PE. > > -EFI_LDFLAGS = $(patsubst -m%,-mi386pep,$(XEN_LDFLAGS)) --subsystem=10 > > -XEN_BUILD_PE := $(if $(XEN_BUILD_EFI),$(call ld-option,$(EFI_LDFLAGS) --image-base=0x100000000 -o efi/check.efi efi/check.o)) > > -# If the above failed, it may be merely because of the linker not dealing well > > -# with debug info. Try again with stripping it. > > -ifeq ($(CONFIG_DEBUG_INFO)-$(XEN_BUILD_PE),y-n) > > -EFI_LDFLAGS += --strip-debug > > -XEN_BUILD_PE := $(call ld-option,$(EFI_LDFLAGS) --image-base=0x100000000 -o efi/check.efi efi/check.o) > > -endif > > - > > -ifeq ($(XEN_BUILD_PE),y) > > - > > -# Check if the linker produces fixups in PE by default > > -nr-fixups := $(shell $(OBJDUMP) -p efi/check.efi | grep '^[[:blank:]]*reloc[[:blank:]]*[0-9][[:blank:]].*DIR64$$' | wc -l) > > -ifeq ($(nr-fixups),2) > > -MKRELOC := : > > -relocs-dummy := > > -else > > -MKRELOC := efi/mkreloc > > -relocs-dummy := efi/relocs-dummy.o > > -# If the linker produced fixups but not precisely two of them, we need to > > -# disable it doing so. But if it didn't produce any fixups, it also wouldn't > > -# recognize the option. > > -ifneq ($(nr-fixups),0) > > -EFI_LDFLAGS += --disable-reloc-section > > -endif > > -endif > > - > > -endif # $(XEN_BUILD_PE) > > - > > endif # $(efi-y) > > Is the remaining if(,) block still warranted? I.e. can't the single line > > CFLAGS-$(XEN_BUILD_EFI) += -DXEN_BUILD_EFI > > live without the surrounding conditional? Let's see, $(efi-y) depends on CONFIG_PV_SHIM_EXCLUSIVE and `sudo make install`, but XEN_BUILD_EFI also depends on CONFIG_PV_SHIM_EXCLUSIVE and we don't want to build in `sudo make install` so CFLAGS shouldn't be used. So the single line without the if() block seems enough. I remove the surrounding conditional. Thanks,
diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile index 5e1d38d6cd5b..dfcbd01bb4ed 100644 --- a/xen/arch/x86/Makefile +++ b/xen/arch/x86/Makefile @@ -123,41 +123,7 @@ $(TARGET): $(TARGET)-syms $(efi-y) boot/mkelf32 mv $(TMP) $(TARGET) ifneq ($(efi-y),) - -# Check if the compiler supports the MS ABI. -export XEN_BUILD_EFI := $(shell $(CC) $(XEN_CFLAGS) -c efi/check.c -o efi/check.o 2>/dev/null && echo y) CFLAGS-$(XEN_BUILD_EFI) += -DXEN_BUILD_EFI - -# Check if the linker supports PE. -EFI_LDFLAGS = $(patsubst -m%,-mi386pep,$(XEN_LDFLAGS)) --subsystem=10 -XEN_BUILD_PE := $(if $(XEN_BUILD_EFI),$(call ld-option,$(EFI_LDFLAGS) --image-base=0x100000000 -o efi/check.efi efi/check.o)) -# If the above failed, it may be merely because of the linker not dealing well -# with debug info. Try again with stripping it. -ifeq ($(CONFIG_DEBUG_INFO)-$(XEN_BUILD_PE),y-n) -EFI_LDFLAGS += --strip-debug -XEN_BUILD_PE := $(call ld-option,$(EFI_LDFLAGS) --image-base=0x100000000 -o efi/check.efi efi/check.o) -endif - -ifeq ($(XEN_BUILD_PE),y) - -# Check if the linker produces fixups in PE by default -nr-fixups := $(shell $(OBJDUMP) -p efi/check.efi | grep '^[[:blank:]]*reloc[[:blank:]]*[0-9][[:blank:]].*DIR64$$' | wc -l) -ifeq ($(nr-fixups),2) -MKRELOC := : -relocs-dummy := -else -MKRELOC := efi/mkreloc -relocs-dummy := efi/relocs-dummy.o -# If the linker produced fixups but not precisely two of them, we need to -# disable it doing so. But if it didn't produce any fixups, it also wouldn't -# recognize the option. -ifneq ($(nr-fixups),0) -EFI_LDFLAGS += --disable-reloc-section -endif -endif - -endif # $(XEN_BUILD_PE) - endif # $(efi-y) ALL_OBJS := $(BASEDIR)/arch/x86/boot/built_in.o $(BASEDIR)/arch/x86/efi/built_in.o $(ALL_OBJS) @@ -218,8 +184,10 @@ endif $(TARGET).efi: VIRT_BASE = 0x$(shell $(NM) efi/relocs-dummy.o | sed -n 's, A VIRT_START$$,,p') ifeq ($(MKRELOC),:) +relocs-dummy := $(TARGET).efi: ALT_BASE := else +relocs-dummy := efi/relocs-dummy.o $(TARGET).efi: ALT_BASE = 0x$(shell $(NM) efi/relocs-dummy.o | sed -n 's, A ALT_START$$,,p') endif diff --git a/xen/arch/x86/arch.mk b/xen/arch/x86/arch.mk index eea320e618b9..98dd41d32118 100644 --- a/xen/arch/x86/arch.mk +++ b/xen/arch/x86/arch.mk @@ -60,5 +60,47 @@ ifeq ($(CONFIG_UBSAN),y) $(call cc-option-add,CFLAGS_UBSAN,CC,-fno-sanitize=alignment) endif +ifneq ($(CONFIG_PV_SHIM_EXCLUSIVE),y) + +efi-check-o = arch/x86/efi/check.o + +# Check if the compiler supports the MS ABI. +XEN_BUILD_EFI := $(call if-success,$(CC) $(CFLAGS) -c $(efi-check-o:.o=.c) -o $(efi-check-o),y) + +# Check if the linker supports PE. +EFI_LDFLAGS := $(patsubst -m%,-mi386pep,$(LDFLAGS)) --subsystem=10 +LD_PE_check_cmd = $(call ld-option,$(EFI_LDFLAGS) --image-base=0x100000000 -o $(efi-check-o:.o=.efi) $(efi-check-o)) +XEN_BUILD_PE := $(LD_PE_check_cmd) + +# If the above failed, it may be merely because of the linker not dealing well +# with debug info. Try again with stripping it. +ifeq ($(CONFIG_DEBUG_INFO)-$(XEN_BUILD_PE),y-n) +EFI_LDFLAGS += --strip-debug +XEN_BUILD_PE := $(LD_PE_check_cmd) +endif + +ifeq ($(XEN_BUILD_PE),y) + +# Check if the linker produces fixups in PE by default +efi-check-relocs := $(shell $(OBJDUMP) -p $(efi-check-o:.o=.efi) | grep '^[[:blank:]]*reloc[[:blank:]]*[0-9][[:blank:]].*DIR64$$' | wc -l) + +ifeq ($(efi-check-relocs),2) +MKRELOC := : +else +MKRELOC := efi/mkreloc +# If the linker produced fixups but not precisely two of them, we need to +# disable it doing so. But if it didn't produce any fixups, it also wouldn't +# recognize the option. +ifneq ($(efi-check-relocs),0) +EFI_LDFLAGS += --disable-reloc-section +endif +endif + +endif # $(XEN_BUILD_PE) + +export XEN_BUILD_EFI XEN_BUILD_PE MKRELOC +export EFI_LDFLAGS +endif + # Set up the assembler include path properly for older toolchains. CFLAGS += -Wa,-I$(BASEDIR)/include
We are going to need the variable XEN_BUILD_EFI earlier. But a side effect of calculating the value of $(XEN_BUILD_EFI) is to also to generate "efi/check.o" which is used for further checks. Thus the whole chain that check for EFI support is moved to "arch.mk". Some other changes are made to avoid too much duplication: - $(efi-check-o): Used to avoid repeating "efi/check.*". We don't set it to the path to the source as it would be wrong as soon as we support out-of-tree build. - $(LD_PE_check_cmd): As it is called twice, with an updated $(EFI_LDFLAGS). $(nr-fixups) is renamed to $(efi-check-relocs) as the former might be a bit too generic. Signed-off-by: Anthony PERARD <anthony.perard@citrix.com> --- Notes: v7: - Do the whole check for EFI support in arch.mk. So efi/check.o is produce there and used there, and produce efi/check.efi and use it there. Thus avoid the need to repeat the test done for XEN_BUILD_EFI. xen/arch/x86/Makefile | 36 ++---------------------------------- xen/arch/x86/arch.mk | 42 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 44 insertions(+), 34 deletions(-)