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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 16.07.18 at 11:25, <roger.pau@xxxxxxxxxx> wrote:
> 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?

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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel