[PATCH] x86/efi: move the logic to detect PE build support

[PATCH] x86/efi: move the logic to detect PE build support

[Roger Pau Monne]

So that it can be used by other components apart from the efi specific
code.

This is required so that the conditional used to define the efi symbol
in the linker script can be removed and instead the definition of the
efi symbol can be guarded using the preprocessor.

The motivation behind this change is to be able to build Xen using lld
(the LLVM linker), that at least on version 6.0.0 doesn't work
properly with a DEFINED being used in a conditional expression:

ld    -melf_x86_64_fbsd  -T xen.lds -N prelink.o --build-id=sha1 \
    /root/src/xen/xen/common/symbols-dummy.o -o /root/src/xen/xen/.xen-syms.0
ld: error: xen.lds:233: symbol not found: efi

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Daniel Kiper <daniel.kiper@oracle.com>
---
 xen/arch/x86/Makefile     | 10 ++++++++++
 xen/arch/x86/efi/Makefile | 11 +++--------
 xen/arch/x86/xen.lds.S    |  4 +++-
 3 files changed, 16 insertions(+), 9 deletions(-)

diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
index 5563c813dd..59f96626aa 100644
--- 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
+
 ifneq ($(build_id_linker),)
 ifeq ($(call ld-ver-build-id,$(LD) $(filter -m%,$(EFI_LDFLAGS))),y)
 CFLAGS += -DBUILD_ID_EFI
diff --git a/xen/arch/x86/efi/Makefile b/xen/arch/x86/efi/Makefile
index 3be9661108..ff5e33fb25 100644
--- a/xen/arch/x86/efi/Makefile
+++ b/xen/arch/x86/efi/Makefile
@@ -1,16 +1,11 @@
 CFLAGS += -fshort-wchar
 
-efi := y$(shell rm -f disabled)
-efi := $(if $(efi),$(shell $(CC) $(filter-out $(CFLAGS-y) .%.d,$(CFLAGS)) -c check.c 2>disabled && echo y))
-efi := $(if $(efi),$(shell $(LD) -mi386pep --subsystem=10 -o check.efi check.o 2>disabled && echo y))
-efi := $(if $(efi),$(shell rm disabled)y)
-
 %.o: %.ihex
 	$(OBJCOPY) -I ihex -O binary $< $@
 
 boot.init.o: buildid.o
 
 obj-y := stub.o
-obj-$(efi) := boot.init.o compat.o relocs-dummy.o runtime.o
-extra-$(efi) += buildid.o
-nocov-$(efi) += stub.o
+obj-$(BUILD_PE) := boot.init.o compat.o relocs-dummy.o runtime.o
+extra-$(BUILD_PE) += buildid.o
+nocov-$(BUILD_PE) += stub.o
diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index 326e885402..f93d5d7e16 100644
--- 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
 
   /* Sections to be discarded */
   /DISCARD/ : {
-- 
2.17.1


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

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

[Jan Beulich]

>>> On 13.07.18 at 18:02, <roger.pau@citrix.com> 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.

Jan



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

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

[Roger Pau Monné]

On Mon, Jul 16, 2018 at 01:59:15AM -0600, Jan Beulich wrote:
> >>> On 13.07.18 at 18:02, <roger.pau@citrix.com> 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 the ELF binary will also contain efi support if the
build toolchain supports PE. Hence whether the efi symbol is defined
or not solely depends on the detection done above (and whether
runtime.c or stub.c is used).

Maybe I'm missing something, I have to admit the code is quite
convoluted.

Thanks, Roger.

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

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

[Jan Beulich]

>>> On 16.07.18 at 10:26, <roger.pau@citrix.com> wrote:
> On Mon, Jul 16, 2018 at 01:59:15AM -0600, Jan Beulich wrote:
>> >>> On 13.07.18 at 18:02, <roger.pau@citrix.com> 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.

Jan

> but the ELF binary will also contain efi support if the
> build toolchain supports PE. Hence whether the efi symbol is defined
> or not solely depends on the detection done above (and whether
> runtime.c or stub.c is used).
> 
> Maybe I'm missing something, I have to admit the code is quite
> convoluted.
> 
> Thanks, Roger.





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

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

[Roger Pau Monné]

On Mon, Jul 16, 2018 at 02:55:16AM -0600, Jan Beulich wrote:
> >>> On 16.07.18 at 10:26, <roger.pau@citrix.com> wrote:
> > On Mon, Jul 16, 2018 at 01:59:15AM -0600, Jan Beulich wrote:
> >> >>> On 13.07.18 at 18:02, <roger.pau@citrix.com> 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. It is my understanding
from the previous conversation with Daniel that for multiboot2 support
you need an EFL binary that contains runtime.c (and thus the efi
symbol is defined), and that you can build such a binary without
having EFI set.

Roger.

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

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

[Jan Beulich]

>>> On 16.07.18 at 11:12, <roger.pau@citrix.com> wrote:
> On Mon, Jul 16, 2018 at 02:55:16AM -0600, Jan Beulich wrote:
>> >>> On 16.07.18 at 10:26, <roger.pau@citrix.com> wrote:
>> > On Mon, Jul 16, 2018 at 01:59:15AM -0600, Jan Beulich wrote:
>> >> >>> On 13.07.18 at 18:02, <roger.pau@citrix.com> 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?

Jan



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

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

[Roger Pau Monné]

On Mon, Jul 16, 2018 at 03:18:13AM -0600, Jan Beulich wrote:
> >>> On 16.07.18 at 11:12, <roger.pau@citrix.com> wrote:
> > On Mon, Jul 16, 2018 at 02:55:16AM -0600, Jan Beulich wrote:
> >> >>> On 16.07.18 at 10:26, <roger.pau@citrix.com> wrote:
> >> > On Mon, Jul 16, 2018 at 01:59:15AM -0600, Jan Beulich wrote:
> >> >> >>> On 13.07.18 at 18:02, <roger.pau@citrix.com> 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

[Jan Beulich]

>>> On 16.07.18 at 11:25, <roger.pau@citrix.com> wrote:
> On Mon, Jul 16, 2018 at 03:18:13AM -0600, Jan Beulich wrote:
>> >>> On 16.07.18 at 11:12, <roger.pau@citrix.com> wrote:
>> > On Mon, Jul 16, 2018 at 02:55:16AM -0600, Jan Beulich wrote:
>> >> >>> On 16.07.18 at 10:26, <roger.pau@citrix.com> wrote:
>> >> > On Mon, Jul 16, 2018 at 01:59:15AM -0600, Jan Beulich wrote:
>> >> >> >>> On 13.07.18 at 18:02, <roger.pau@citrix.com> 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?

Indeed. But too strict checking is less of a problem than too lax one.
Otoh people are far more likely to have a suitable gcc but no suitably
configured binutils, so relaxing this would likely be worthwhile. Daniel?

> How about naming this BUILD_EFI_SERVICES or some such?

Perhaps just BUILD_EFI, but yes. EFI, as used in xen.lds.S, would
then perhaps better be renamed into BUILD_PE (or some such).

Jan



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

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

[Daniel Kiper]

On Mon, Jul 16, 2018 at 03:52:49AM -0600, Jan Beulich wrote:
> >>> On 16.07.18 at 11:25, <roger.pau@citrix.com> wrote:
> > On Mon, Jul 16, 2018 at 03:18:13AM -0600, Jan Beulich wrote:
> >> >>> On 16.07.18 at 11:12, <roger.pau@citrix.com> wrote:
> >> > On Mon, Jul 16, 2018 at 02:55:16AM -0600, Jan Beulich wrote:
> >> >> >>> On 16.07.18 at 10:26, <roger.pau@citrix.com> wrote:
> >> >> > On Mon, Jul 16, 2018 at 01:59:15AM -0600, Jan Beulich wrote:
> >> >> >> >>> On 13.07.18 at 18:02, <roger.pau@citrix.com> 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?
>
> Indeed. But too strict checking is less of a problem than too lax one.
> Otoh people are far more likely to have a suitable gcc but no suitably
> configured binutils, so relaxing this would likely be worthwhile. Daniel?

Yes, it is true and I think that it is worth relaxing the check.

> > How about naming this BUILD_EFI_SERVICES or some such?
>
> Perhaps just BUILD_EFI, but yes. EFI, as used in xen.lds.S, would

s/BUILD_EFI/XEN_BUILD_EFI/?

> then perhaps better be renamed into BUILD_PE (or some such).

XEN_BUILD_PE or XEN_LINK_PE?

Daniel

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