Re: [XEN PATCH v7 17/51] build: set XEN_BUILD_EFI earlier

[Jan Beulich <jbeulich@xxxxxxxx>]

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