Re: [Xen-devel] [PATCH v2 3/8] xen/x86: manually build xen.mb.efi binary

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 19.06.18 at 16:35, <daniel.kiper@xxxxxxxxxx> wrote:
> Not done:
>    - ASM PE header conversion to C; not feasible,

Hmm. As long as you can convince Andrew to give you an ack, I
won't veto it. But I continue to dislike it, and hence I don't
currently foresee myself acking it.

>    - DOS stub code reduction; experiments showed that DOS stub code size
>      cannot be changed due to some bugs in applications playing with PE
>      files, e.g. objdump (more about the issue can be found in the patch
>      itself); so, I think that if it is not possible to reduce the size
>      of code then it does make sens change the code itself; hence, it
>      pays to leave common DOS stub code as is.

Even more so here: I'm not sure I care about buggy tools. Did you
at least enter a bug report (which you would want to reference in the
code comment)? For all of my Win32 life I've been doing fine shrinking
DLL/EXE file sizes by moving the PE header to offset 0x40. No tool
has even complained. I've just tried objdump 2.25.0 on one of these
DLLs - no problem afaics. Did the tool perhaps choke on something
other than the non-"standard" offset of the PE header?

As to leaving the code as is - if there's anything to be left as is, then
the code live binutils would produce, i.e. I'd then ask you to obtain
the code at build time, rather than inserting a series of magic hex
values in the sources. But as said - even better would be to omit this
altogether.

> --- a/xen/Makefile
> +++ b/xen/Makefile
> @@ -61,6 +61,10 @@ _install: $(TARGET)$(CONFIG_XEN_INSTALL_SUFFIX)
>       ln -f -s $(T)-$(XEN_FULLVERSION)$(Z) 
> $(D)$(BOOT_DIR)/$(T)-$(XEN_VERSION).$(XEN_SUBVERSION)$(Z)
>       ln -f -s $(T)-$(XEN_FULLVERSION)$(Z) 
> $(D)$(BOOT_DIR)/$(T)-$(XEN_VERSION)$(Z)
>       ln -f -s $(T)-$(XEN_FULLVERSION)$(Z) $(D)$(BOOT_DIR)/$(T)$(Z)
> +     $(INSTALL_DATA) $(TARGET).mb.efi 
> $(D)$(BOOT_DIR)/$(T)-$(XEN_FULLVERSION).mb.efi
> +     ln -f -s $(T)-$(XEN_FULLVERSION).mb.efi 
> $(D)$(BOOT_DIR)/$(T)-$(XEN_VERSION).$(XEN_SUBVERSION).mb.efi
> +     ln -f -s $(T)-$(XEN_FULLVERSION).mb.efi 
> $(D)$(BOOT_DIR)/$(T)-$(XEN_VERSION).mb.efi
> +     ln -f -s $(T)-$(XEN_FULLVERSION).mb.efi $(D)$(BOOT_DIR)/$(T).mb.efi

This suggests something wants to be macro-ized here, I think.

> $(D)$(DEBUG_DIR)/$(T)-syms-$(XEN_FULLVERSION).map
> @@ -121,7 +125,7 @@ _clean: delete-unfresh-files
>       $(MAKE) -f $(BASEDIR)/Rules.mk -C test clean
>       $(MAKE) -f $(BASEDIR)/tools/kconfig/Makefile.kconfig ARCH=$(ARCH) 
> SRCARCH=$(SRCARCH) clean
>       find . \( -name "*.o" -o -name ".*.d" -o -name "*.gcno" \) -exec rm -f 
> {} \;
> -     rm -f include/asm $(TARGET) $(TARGET).gz $(TARGET).efi 
> $(TARGET).efi.map $(TARGET)-syms $(TARGET)-syms.map *~ core
> +     rm -f include/asm $(TARGET) $(TARGET).gz $(TARGET).efi $(TARGET).mb.efi 
> $(TARGET).efi.map $(TARGET)-syms $(TARGET)-syms.map *~ core

Perhaps simply $(TARGET)*.efi? I don't think we're at risk deleting something
precious that way.

> --- a/xen/arch/x86/Makefile
> +++ b/xen/arch/x86/Makefile
> @@ -101,6 +101,9 @@ 
> syms-warn-dup-$(CONFIG_SUPPRESS_DUPLICATE_SYMBOL_WARNINGS) :=
>  $(TARGET): $(TARGET)-syms $(efi-y) boot/mkelf32
>       ./boot/mkelf32 $(notes_phdrs) $(TARGET)-syms $(TARGET) 
> $(XEN_IMG_OFFSET) \
>                      `$(NM) $(TARGET)-syms | sed -ne 's/^\([^ ]*\) . 
> __2M_rwdata_end$$/0x\1/p'`
> +     $(OBJCOPY) -O binary -S --change-section-address \
> +             ".efi.pe.header-`$(NM) $(TARGET)-syms | sed -ne 's/^\([^ ]*\) . 
> __image_base__$$/0x\1/p'`" \
> +             $(TARGET)-syms $(TARGET).mb.efi

This wants to be a separate rule of a separate $(TARGET).mb.efi target.

> +GLOBAL(efi_pe_head)
> +        /*
> +         * Legacy EXE header.
> +         *
> +         * Most of it is copied from binutils package, version 2.30,
> +         * include/coff/pe.h:struct external_PEI_filehdr and
> +         * bfd/peXXigen.c:_bfd_XXi_only_swap_filehdr_out().
> +         *
> +         * Page is equal 512 bytes here.
> +         * Paragraph is equal 16 bytes here.
> +         */
> +        .short  0x5a4d                               /* EXE magic number. */
> +        .short  0x90                                 /* Bytes on last page 
> of file. */
> +        .short  0x3                                  /* Pages in file. */
> +        .short  0                                    /* Relocations. */
> +        .short  0x4                                  /* Size of header in 
> paragraphs. */
> +        .short  0                                    /* Minimum extra 
> paragraphs needed. */
> +        .short  0xffff                               /* Maximum extra 
> paragraphs needed. */
> +        .short  0                                    /* Initial (relative) 
> SS value. */
> +        .short  0xb8                                 /* Initial SP value. */
> +        .short  0                                    /* Checksum. */
> +        .short  0                                    /* Initial IP value. */
> +        .short  0                                    /* Initial (relative) 
> CS value. */
> +        .short  0x40                                 /* File address of 
> relocation table. */

This is just the most prominent example: Why is this a hard coded
number, while ...

> +        .short  0                                    /* Overlay number. */
> +        .fill   4, 2, 0                              /* Reserved words. */
> +        .short  0                                    /* OEM identifier. */
> +        .short  0                                    /* OEM information. */
> +        .fill   10, 2, 0                             /* Reserved words. */
> +        .long   pe_header - efi_pe_head              /* File address of the 
> PE header. */

... this isn't?


> +        /*
> +         * PE/COFF header.
> +         *
> +         * The PE/COFF format is defined by Microsoft, and is available from
> +         * 
> http://www.microsoft.com/whdc/system/platform/firmware/PECOFF.mspx 
> +         *
> +         * Some ideas are taken from Linux kernel and Xen ARM64.
> +         */
> +
> +pe_header:

Does this and ones further down really need to be a non-local labels?

> +        .ascii  "PE\0\0"                             /* PE signature. */
> +        .short  0x8664                               /* Machine: 
> IMAGE_FILE_MACHINE_AMD64 */
> +        .short  1                                    /* NumberOfSections. */
> +        .long   XEN_COMPILE_POSIX_TIME               /* TimeDateStamp. */
> +        .long   0                                    /* 
> PointerToSymbolTable. */
> +        .long   0                                    /* NumberOfSymbols. */
> +        .short  section_table - optional_header      /* 
> SizeOfOptionalHeader. */
> +        .short  0x226                                /* Characteristics:
> +                                                      *   
> IMAGE_FILE_EXECUTABLE_IMAGE |
> +                                                      *   
> IMAGE_FILE_LARGE_ADDRESS_AWARE |
> +                                                      *   
> IMAGE_FILE_DEBUG_STRIPPED |
> +                                                      *   
> IMAGE_FILE_LINE_NUMS_STRIPPED
> +                                                      */
> +
> +optional_header:
> +        .short  0x20b                                /* PE format: PE32+ */
> +        .byte   0x02                                 /* MajorLinkerVersion. 
> */
> +        .byte   0x1e                                 /* MinorLinkerVersion. 
> */

Let's not cheat more than needed: I'm pretty sure just zeros will do
fine here.

> +        .long   __2M_rwdata_end - efi_pe_head_end    /* SizeOfCode. */
> +        .long   0                                    /* 
> SizeOfInitializedData. */
> +        .long   0                                    /* 
> SizeOfUninitializedData. */
> +        .long   sym_offs(efi_mb_start)               /* AddressOfEntryPoint. 
> */
> +        .long   sym_offs(start)                      /* BaseOfCode. */
> +        .quad   sym_offs(__image_base__)             /* ImageBase. */

The sym_offs() here is certainly different from what xen.efi
currently has. With the plan being to have a drop-in replacement,
such differences need to be explained to be benign (which here
I doubt it is).

> +        .align XEN_FILE_ALIGN
> +GLOBAL(efi_pe_head_end)
> +
> +        .text
> +        .code32

Why the .code32 here? Perhaps this comes back to the question of
whether this whole header should really be lumped into this file.

> @@ -582,6 +587,12 @@ static void __init efi_arch_memory_setup(void)
>      if ( !efi_enabled(EFI_LOADER) )
>          return;
>  
> +    if ( efi_enabled(EFI_MB_LOADER) )
> +        for ( pte = __page_tables_start; pte < __page_tables_end;
> +              pte += ( pte != (intpte_t *)l2_identmap ) ? 1 : 4 * 
> L2_PAGETABLE_ENTRIES )

Please avoid explicit casts - &l2e_get_intpte(l2_identmap[0]) or
something along those lines ought to work here. Same for
4 * L2_PAGETABLE_ENTRIES - you mean ARRAY_SIZE() there.

Also this whole code block needs a comment, to explain what it
does and also why l2_identmap needs skipping.

Furthermore - isn't this off by one, and you process l2_identmap[0]
this way, skipping the rest _plus_ the first following entry? I think
you'd better use ++ here and have an if() inside the for() body.
Then you can also attach the related part of the comment there
instead of to the loop header. And I can avoid complaining about
the stray spaces inside the parentheses.

> @@ -674,6 +685,15 @@ static bool __init 
> efi_arch_use_config_file(EFI_SYSTEM_TABLE *SystemTable)
>  
>  static void __init efi_arch_flush_dcache_area(const void *vaddr, UINTN size) 
> { }
>  
> +void EFIAPI efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable);
> +
> +void EFIAPI __init noreturn
> +efi_mb_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
> +{
> +    __set_bit(EFI_MB_LOADER, &efi_flags);
> +    efi_start(ImageHandle, SystemTable);
> +}

Why yet another entry point? This again speaks against the image
being a drop-in replacement for xen.efi.

> --- a/xen/arch/x86/xen.lds.S
> +++ b/xen/arch/x86/xen.lds.S
> @@ -60,7 +60,20 @@ SECTIONS
>  
>    start_pa = ABSOLUTE(start - __XEN_VIRT_START);
>  
> +#ifdef EFI
>    . = __XEN_VIRT_START + XEN_IMG_OFFSET;
> +#else
> +  /*
> +   * The PE header must be followed by .text section which
> +   * starts at __XEN_VIRT_START + XEN_IMG_OFFSET address.
> +   */
> +  . = __XEN_VIRT_START + XEN_IMG_OFFSET - efi_pe_head_end + efi_pe_head;
> +
> +  DECL_SECTION(.efi.pe.header) {
> +       *(.efi.pe.header)
> +  } :NONE
> +#endif

The #ifdef wants a brief comment attached I think, since it's quite
odd to see the #else side deal with EFI as well.

> @@ -271,6 +284,9 @@ SECTIONS
>         *(.data.rel)
>         *(.data.rel.*)
>         CONSTRUCTORS
> +       /* PE file must end at XEN_FILE_ALIGN boundary. */
> +       . = ALIGN(XEN_FILE_ALIGN);
> +       __pe_text_raw_end = .;

Is this really a requirement on the file, or just on the label?

> @@ -292,6 +308,8 @@ SECTIONS
>    . = ALIGN(SECTION_ALIGN);
>    __2M_rwdata_end = .;
>  
> +  __pe_SizeOfImage = ALIGN(. - __image_base__, MB(16));
> +
>  #ifdef EFI
>    . = ALIGN(4);
>    .reloc : {

Considering the respective code and comment inside the #ifdef, does
your addition really belong ahead of it?

Jan


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