Re: [XEN PATCH v8 04/47] build: set XEN_BUILD_EFI earlier

[Jan Beulich <jbeulich@xxxxxxxx>]

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@xxxxxxxxxx>

Technically
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

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