| Message ID | 20211125134006.1076646-5-anthony.perard@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen: Build system improvements, now with out-of-tree build! | expand |
On 25.11.2021 14:39, 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-nr-fixups) as the former might be > a bit too generic. > > Signed-off-by: Anthony PERARD <anthony.perard@citrix.com> Technically Reviewed-by: Jan Beulich <jbeulich@suse.com> Nevertheless a suggestion and a remark: > --- 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 How about making this efi-check := arch/x86/efi/check That way you wouldn't need to replace the extension in a number of places, but simply append the respective one in every place using this. > +# 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-nr-fixups := $(shell $(OBJDUMP) -p $(efi-check-o:.o=.efi) | grep '^[[:blank:]]*reloc[[:blank:]]*[0-9][[:blank:]].*DIR64$$' | wc -l) > + > +ifeq ($(efi-nr-fixups),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-nr-fixups),0) > +EFI_LDFLAGS += --disable-reloc-section > +endif > +endif > + > +endif # $(XEN_BUILD_PE) > + > +export XEN_BUILD_EFI XEN_BUILD_PE MKRELOC > +export EFI_LDFLAGS > +endif Exporting MKRELOC in particular isn't very nice. I wonder whether there wouldn't be a way to keep it local to xen/Makefile. Jan
On Thu, Dec 02, 2021 at 03:06:54PM +0100, Jan Beulich wrote: > On 25.11.2021 14:39, Anthony PERARD wrote: > > +efi-check-o := arch/x86/efi/check.o > > How about making this > > efi-check := arch/x86/efi/check > > That way you wouldn't need to replace the extension in a number of places, > but simply append the respective one in every place using this. This change sound fine. I guess it will make reading the code a bit easier. > > +export XEN_BUILD_EFI XEN_BUILD_PE MKRELOC > > +export EFI_LDFLAGS > > +endif > > Exporting MKRELOC in particular isn't very nice. I wonder whether there > wouldn't be a way to keep it local to xen/Makefile. I don't think that's possible. The value of MKRELOC depends on a call with OBJDUMP which depends on call with LD which depends on a call with CC. And the call with CC is the one I'm trying to move. Unless there is a better way to build *.efi, we need to know whether to use `mkreloc` or not. I could rename it XEN_MKRELOC. Or if there is another name that could make sense, that would be fine too, like XEN_BUILD_EFI_NEED_RELOC which could be a boolean. Thanks,
On 07.12.2021 12:04, Anthony PERARD wrote: > On Thu, Dec 02, 2021 at 03:06:54PM +0100, Jan Beulich wrote: >> On 25.11.2021 14:39, Anthony PERARD wrote: >>> +export XEN_BUILD_EFI XEN_BUILD_PE MKRELOC >>> +export EFI_LDFLAGS >>> +endif >> >> Exporting MKRELOC in particular isn't very nice. I wonder whether there >> wouldn't be a way to keep it local to xen/Makefile. > > I don't think that's possible. The value of MKRELOC depends on a call > with OBJDUMP which depends on call with LD which depends on a call with > CC. And the call with CC is the one I'm trying to move. Like suggested for another variable elsewhere, besides moving the definition (which I agree looks difficult to achieve) there's also the option of passing it on the command line to (presumably) just the single sub-make which actually means to consume it. It's only the final linking step where it's needed afaict. Jan
diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile index 8db4cb98edbb..d7d2adc1881e 100644 --- a/xen/arch/x86/Makefile +++ b/xen/arch/x86/Makefile @@ -121,44 +121,8 @@ $(TARGET): $(TARGET)-syms $(efi-y) boot/mkelf32 { echo "No Multiboot2 header found" >&2; false; } 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) ifeq ($(CONFIG_LTO),y) @@ -217,8 +181,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..19c9cd206ed0 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-nr-fixups := $(shell $(OBJDUMP) -p $(efi-check-o:.o=.efi) | grep '^[[:blank:]]*reloc[[:blank:]]*[0-9][[:blank:]].*DIR64$$' | wc -l) + +ifeq ($(efi-nr-fixups),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-nr-fixups),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-nr-fixups) as the former might be a bit too generic. Signed-off-by: Anthony PERARD <anthony.perard@citrix.com> --- Notes: v8: - rename to efi-nr-fixups rather than efi-check-relocs - use := when assigning variable in makefile when recursive expansion isn't needed. - no more check of $(efi-y) value for "CFLAGS-$(XEN_BUILD_EFI) += -DXEN_BUILD_EFI". 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 | 38 ++------------------------------------ xen/arch/x86/arch.mk | 42 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 44 insertions(+), 36 deletions(-)