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

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

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,

-- 
Anthony PERARD