Re: [Xen-devel] [PATCH RFC 2/7] xen/x86: Manually build PE header

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 08.07.17 at 23:53, <daniel.kiper@xxxxxxxxxx> wrote:
> This is the first step to get:
>   - one binary which can be loaded by the EFI loader,
>     Multiboot and Multiboot2 protocols,
>   - if we wish, in the future we can drop xen/xen.gz
>     and build xen.efi only,

If anything, generate xen.gz from xen.efi - I see value in the compression,
but the EFI loader requires an uncompressed binary. And of course we'd have
to raise the minimal gcc version requirement.

> --- a/xen/arch/x86/Rules.mk
> +++ b/xen/arch/x86/Rules.mk
> @@ -7,6 +7,8 @@ CFLAGS += -I$(BASEDIR)/include
>  CFLAGS += -I$(BASEDIR)/include/asm-x86/mach-generic
>  CFLAGS += -I$(BASEDIR)/include/asm-x86/mach-default
>  CFLAGS += -DXEN_IMG_OFFSET=$(XEN_IMG_OFFSET)
> +CFLAGS += -DXEN_LOAD_ALIGN=XEN_IMG_OFFSET
> +CFLAGS += -DXEN_FILE_ALIGN=PAGE_SIZE

??? (Sadly your description talks about benefits only, not about what the
patch actually does.)

> --- a/xen/arch/x86/boot/head.S
> +++ b/xen/arch/x86/boot/head.S
> @@ -1,3 +1,4 @@
> +#include <xen/compile.h>
>  #include <xen/multiboot.h>
>  #include <xen/multiboot2.h>
>  #include <public/xen.h>
> @@ -44,6 +45,150 @@
>  .Lmb2ht_init_end\@:
>          .endm
>  
> +        .section .efi.pe.header, "a", @progbits
> +
> +ENTRY(efi_pe_head)

Since you put this in a separate section anyway, why don't you place it in
a C file (perhaps even of its own) with suitably declared structures?

> +        /*
> +         * Legacy EXE header.
> +         *
> +         * Most of it is copied from binutils package, version 2.28,
> +         * 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. */
> +        .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. */
> +
> +        /*
> +         * DOS message.
> +         *
> +         * It is copied from binutils package, version 2.28,
> +         * include/coff/pe.h:struct external_PEI_filehdr and
> +         * bfd/peXXigen.c:_bfd_XXi_only_swap_filehdr_out().
> +         */
> +        .long   0x0eba1f0e
> +        .long   0xcd09b400
> +        .long   0x4c01b821
> +        .long   0x685421cd
> +        .long   0x70207369
> +        .long   0x72676f72
> +        .long   0x63206d61
> +        .long   0x6f6e6e61
> +        .long   0x65622074
> +        .long   0x6e757220
> +        .long   0x206e6920
> +        .long   0x20534f44
> +        .long   0x65646f6d
> +        .long   0x0a0d0d2e
> +        .long   0x24
> +        .long   0

Other than what the comment says, this isn't just a message (or else you
could have used .asciz for the whole thing). I'm not convinced we need
any of this.

> @@ -259,6 +266,8 @@ SECTIONS
>  #endif
>    __2M_rwdata_end = .;
>  
> +  __pe_SizeOfImage = ALIGN(. - __image_base__, XEN_LOAD_ALIGN);

I don't think this is in line with what xen.efi currently has. Any difference
needs explaining (I think there are further fields in this category).

Jan


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