| Message ID | 20210701141011.785641-22-anthony.perard@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen: Build system improvements | expand |
On 01.07.2021 16:10, Anthony PERARD wrote: > We are going to need the variable XEN_BUILD_EFI earlier. > > This early check is using "try-run" to allow to have a temporary > output file in case it is needed for $(CC) to build the *.c file. > > The "efi/check.o" file is still needed in "arch/x86/Makefile" so the > check is currently duplicated. Why is this? Can't you ... > --- a/xen/arch/x86/Makefile > +++ b/xen/arch/x86/Makefile > @@ -126,7 +126,7 @@ $(TARGET): $(TARGET)-syms $(efi-y) boot/mkelf32 > 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) > +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 ... use here what you ... > --- a/xen/arch/x86/arch.mk > +++ b/xen/arch/x86/arch.mk > @@ -60,5 +60,10 @@ ifeq ($(CONFIG_UBSAN),y) > $(call cc-option-add,CFLAGS_UBSAN,CC,-fno-sanitize=alignment) > endif > > +ifneq ($(CONFIG_PV_SHIM_EXCLUSIVE),y) > +# Check if the compiler supports the MS ABI. > +export XEN_BUILD_EFI := $(call try-run,$(CC) $(CFLAGS) -c arch/x86/efi/check.c -o "$$TMPO",y) > +endif ... export here? Jan
On Thu, Aug 05, 2021 at 09:27:18AM +0200, Jan Beulich wrote: > On 01.07.2021 16:10, Anthony PERARD wrote: > > We are going to need the variable XEN_BUILD_EFI earlier. > > > > This early check is using "try-run" to allow to have a temporary > > output file in case it is needed for $(CC) to build the *.c file. > > > > The "efi/check.o" file is still needed in "arch/x86/Makefile" so the > > check is currently duplicated. > > Why is this? Can't you ... > > > --- a/xen/arch/x86/Makefile > > +++ b/xen/arch/x86/Makefile > > @@ -126,7 +126,7 @@ $(TARGET): $(TARGET)-syms $(efi-y) boot/mkelf32 > > 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) > > +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 > > ... use here what you ... > > > --- a/xen/arch/x86/arch.mk > > +++ b/xen/arch/x86/arch.mk > > @@ -60,5 +60,10 @@ ifeq ($(CONFIG_UBSAN),y) > > $(call cc-option-add,CFLAGS_UBSAN,CC,-fno-sanitize=alignment) > > endif > > > > +ifneq ($(CONFIG_PV_SHIM_EXCLUSIVE),y) > > +# Check if the compiler supports the MS ABI. > > +export XEN_BUILD_EFI := $(call try-run,$(CC) $(CFLAGS) -c arch/x86/efi/check.c -o "$$TMPO",y) > > +endif > > ... export here? The problem with the check for EFI support is that there several step, with a step depending on the binary produced by the previous one. XEN_BUILD_EFI In addition to check "__ms_abi__" attribute is supported by $CC, the file "efi/check.o" is produced. XEN_BUILD_PE It is using "efi/check.o" to check for PE support and produce "efi/check.efi". "efi/check.efi" is also used by the Makefile for additional checks (mkreloc). So, if I let the duplicated check for $(XEN_BUILD_EFI) is that it felt wrong to produce "efi/check.o" in "arch/x86/arch.mk" and then later use it in "arch/x86/Makefile". I could maybe move the command that create efi/check.o in the $(XEN_BUILD_PE) check, or I could try to move most of the checks done for EFI into x86/arch.mk. Or maybe just creating the "efi/check.o" file in x86/arch.mk and use it in x86/Makefile, with a comment. What do you think? Thanks,
On 09.08.2021 17:59, Anthony PERARD wrote: > On Thu, Aug 05, 2021 at 09:27:18AM +0200, Jan Beulich wrote: >> On 01.07.2021 16:10, Anthony PERARD wrote: >>> We are going to need the variable XEN_BUILD_EFI earlier. >>> >>> This early check is using "try-run" to allow to have a temporary >>> output file in case it is needed for $(CC) to build the *.c file. >>> >>> The "efi/check.o" file is still needed in "arch/x86/Makefile" so the >>> check is currently duplicated. >> >> Why is this? Can't you ... >> >>> --- a/xen/arch/x86/Makefile >>> +++ b/xen/arch/x86/Makefile >>> @@ -126,7 +126,7 @@ $(TARGET): $(TARGET)-syms $(efi-y) boot/mkelf32 >>> 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) >>> +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 >> >> ... use here what you ... >> >>> --- a/xen/arch/x86/arch.mk >>> +++ b/xen/arch/x86/arch.mk >>> @@ -60,5 +60,10 @@ ifeq ($(CONFIG_UBSAN),y) >>> $(call cc-option-add,CFLAGS_UBSAN,CC,-fno-sanitize=alignment) >>> endif >>> >>> +ifneq ($(CONFIG_PV_SHIM_EXCLUSIVE),y) >>> +# Check if the compiler supports the MS ABI. >>> +export XEN_BUILD_EFI := $(call try-run,$(CC) $(CFLAGS) -c arch/x86/efi/check.c -o "$$TMPO",y) >>> +endif >> >> ... export here? > > The problem with the check for EFI support is that there several step, > with a step depending on the binary produced by the previous one. > > XEN_BUILD_EFI > In addition to check "__ms_abi__" attribute is supported by $CC, the > file "efi/check.o" is produced. > XEN_BUILD_PE > It is using "efi/check.o" to check for PE support and produce > "efi/check.efi". > "efi/check.efi" is also used by the Makefile for additional checks > (mkreloc). > > > So, if I let the duplicated check for $(XEN_BUILD_EFI) is that it felt > wrong to produce "efi/check.o" in "arch/x86/arch.mk" and then later use > it in "arch/x86/Makefile". I could maybe move the command that create > efi/check.o in the $(XEN_BUILD_PE) check, or I could try to move most of > the checks done for EFI into x86/arch.mk. Or maybe just creating the > "efi/check.o" file in x86/arch.mk and use it in x86/Makefile, with a > comment. > > What do you think? The last option looks to promise the least code churn while still eliminating the duplication. So that's one option I'd be fine with, the other being to do all of this together in a single place. Jan
diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile index bb446a1b928d..d3e38e4e9f02 100644 --- a/xen/arch/x86/Makefile +++ b/xen/arch/x86/Makefile @@ -126,7 +126,7 @@ $(TARGET): $(TARGET)-syms $(efi-y) boot/mkelf32 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) +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. diff --git a/xen/arch/x86/arch.mk b/xen/arch/x86/arch.mk index 9f5fade39e91..5a4a1704636f 100644 --- a/xen/arch/x86/arch.mk +++ b/xen/arch/x86/arch.mk @@ -60,5 +60,10 @@ ifeq ($(CONFIG_UBSAN),y) $(call cc-option-add,CFLAGS_UBSAN,CC,-fno-sanitize=alignment) endif +ifneq ($(CONFIG_PV_SHIM_EXCLUSIVE),y) +# Check if the compiler supports the MS ABI. +export XEN_BUILD_EFI := $(call try-run,$(CC) $(CFLAGS) -c arch/x86/efi/check.c -o "$$TMPO",y) +endif + # Set up the assembler include path properly for older toolchains. CFLAGS += -Wa,-I$(BASEDIR)/include diff --git a/xen/scripts/Kbuild.include b/xen/scripts/Kbuild.include index 838c9440f35e..5fe13a7c5abd 100644 --- a/xen/scripts/Kbuild.include +++ b/xen/scripts/Kbuild.include @@ -57,6 +57,23 @@ define filechk fi endef +# output directory for tests below +TMPOUT = .tmp_$$$$ + +# try-run +# Usage: option = $(call try-run, $(CC)...-o "$$TMP",option-ok,otherwise) +# Exit code chooses option. "$$TMP" serves as a temporary file and is +# automatically cleaned up. +try-run = $(shell set -e; \ + TMP=$(TMPOUT)/tmp; \ + TMPO=$(TMPOUT)/tmp.o; \ + mkdir -p $(TMPOUT); \ + trap "rm -rf $(TMPOUT)" EXIT; \ + if ($(1)) >/dev/null 2>&1; \ + then echo "$(2)"; \ + else echo "$(3)"; \ + fi) + # as-insn: Check whether assembler supports an instruction. # Usage: cflags-y += $(call as-insn,CC FLAGS,"insn",option-yes,option-no) as-insn = $(if $(shell echo 'void _(void) { asm volatile ( $(2) ); }' \
We are going to need the variable XEN_BUILD_EFI earlier. This early check is using "try-run" to allow to have a temporary output file in case it is needed for $(CC) to build the *.c file. The "efi/check.o" file is still needed in "arch/x86/Makefile" so the check is currently duplicated. This patch imports the macro "try-run" from Linux v5.12. Signed-off-by: Anthony PERARD <anthony.perard@citrix.com> --- xen/arch/x86/Makefile | 2 +- xen/arch/x86/arch.mk | 5 +++++ xen/scripts/Kbuild.include | 17 +++++++++++++++++ 3 files changed, 23 insertions(+), 1 deletion(-)