Re: [Xen-devel] [PATCH] x86/efi: move the logic to detect PE build support

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Mon, Jul 16, 2018 at 03:18:13AM -0600, Jan Beulich wrote:
> >>> On 16.07.18 at 11:12, <roger.pau@xxxxxxxxxx> wrote:
> > On Mon, Jul 16, 2018 at 02:55:16AM -0600, Jan Beulich wrote:
> >> >>> On 16.07.18 at 10:26, <roger.pau@xxxxxxxxxx> wrote:
> >> > On Mon, Jul 16, 2018 at 01:59:15AM -0600, Jan Beulich wrote:
> >> >> >>> On 13.07.18 at 18:02, <roger.pau@xxxxxxxxxx> wrote:
> >> >> > --- a/xen/arch/x86/Makefile
> >> >> > +++ b/xen/arch/x86/Makefile
> >> >> > @@ -168,6 +168,16 @@ $(TARGET).efi: ALT_BASE = 0x$(shell $(NM) 
> >> >> > efi/relocs-dummy.o | sed -n 's, A ALT_
> >> >> >  # Don't use $(wildcard ...) here - at least make 3.80 expands this 
> >> >> > too 
> >> > early!
> >> >> >  $(TARGET).efi: guard = $(if $(shell echo efi/dis* | grep disabled),:)
> >> >> >  
> >> >> > +# Check if the build system supports PE.
> >> >> > +efi := y$(shell rm -f efi/disabled)
> >> >> > +efi := $(if $(efi),$(shell $(CC) $(filter-out $(CFLAGS-y) 
> >> >> > .%.d,$(CFLAGS)) -c 
> >> > efi/check.c -o efi/check.o 2>efi/disabled && echo y))
> >> >> > +efi := $(if $(efi),$(shell $(LD) -mi386pep --subsystem=10 -o 
> >> >> > efi/check.efi 
> >> > efi/check.o 2>efi/disabled && echo y))
> >> >> > +efi := $(if $(efi),$(shell rm efi/disabled)y)
> >> >> > +export BUILD_PE := $(efi)
> >> >> > +ifeq ($(efi),y)
> >> >> > +CFLAGS += -DBUILD_PE
> >> >> > +endif
> >> >> 
> >> >> For one I'm not really happy about this being moved here: I did place it
> >> >> in efi/Makefile for the simple reason of having as much as possible of
> >> >> the EFI specifics in that single file.
> >> >> 
> >> >> Additionally I think it would be better if setting propagated through 
> >> >> the
> >> >> environment had XEN_ prefixes.
> >> >> 
> >> >> > --- a/xen/arch/x86/xen.lds.S
> >> >> > +++ b/xen/arch/x86/xen.lds.S
> >> >> > @@ -304,7 +304,9 @@ SECTIONS
> >> >> >    } :text
> >> >> >  #endif
> >> >> >  
> >> >> > -  efi = DEFINED(efi) ? efi : .;
> >> >> > +#ifndef BUILD_PE
> >> >> > +  efi = .;
> >> >> > +#endif
> >> >> 
> >> >> And then I don't really understand how this is different from the
> >> >> original #ifndef EFI that Daniel had problems with.
> >> > 
> >> > As I understand it EFI only signals whether a PE binary will be
> >> > created,
> >> 
> >> But that is my point: BUILD_PE signals exactly that aiui.
> > 
> > No, BUILD_PE signals whether the binary will use runtime.c instead of
> > stub.c, and thus have the efi symbol defined.
> 
> But in that case - why did you chose this particular name?

Because that seems to be what the tests actually do. AFAICT it tests
that the compiler supports the MS ABI and that the linker is able to
generate PE binaries.

I think this is slightly wrong, because for multiboot2 support you
likely only need the compiler MS ABI support, but not the linker PE
support?

How about naming this BUILD_EFI_SERVICES or some such?

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel