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

[Daniel Kiper <daniel.kiper@oracle.com>]

On Fri, May 04, 2018 at 09:38:03AM -0600, Jan Beulich wrote:
> >>> On 08.07.17 at 23:53, <daniel.kiper@oracle.com> 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,

I generate both xen.gz and xen.efi from xen-syms. Anyway, as I can see
we currently depend more on ELF output than earlier. So, I do not expect
that we would be able to drop xen.gz in the near future.

> 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.)

OK, I will improve the commit message. And maybe s/XEN_FILE_ALIGN/XEN_EFI_FILE_ALIGN/.
And s/PAGE_SIZE/512/. This is minimal value required by PE spec. I have used
PAGE_SIZE earlier just to be on safe side and in line with the output from ld.

> > --- 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?

Really? I thought that it makes sense to have all bootloader headers in
one place. Additionally, C requires struct definition in advance and later
it have to be filled somehow. So, it will be twice as large. Hence, I do not
see much benefit in using C here. OK, maybe it will be a bit more readable.

> > +        /*
> > +         * 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.

Potentially we can drop this. However, ld from binutils put this into
EFI binary. And IIRC this is exactly what is embedded by other linkers
into PE/COFF compatible files, e.g. *.efi, *.exe, *.dll, etc. So,
I would leave this just for the sake of compatibility.

> > @@ -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).

I am not going to build manually exact copy of current xen.efi. It does not
make sense. I would like to provide something minimalistic which works. No
more no less. However, if you wish I can provide relevant comment here.

Daniel

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