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

[Daniel Kiper <daniel.kiper@xxxxxxxxxx>]

On Mon, May 14, 2018 at 04:40:39AM -0600, Jan Beulich wrote:
> >>> On 08.05.18 at 14:47, <daniel.kiper@xxxxxxxxxx> wrote:
> > On Fri, May 04, 2018 at 09:38:03AM -0600, Jan Beulich wrote:
> >> >>> On 08.07.17 at 23:53, <daniel.kiper@xxxxxxxxxx> wrote:
> >> > --- 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.
>
> Just go through *.sys on any Windows, and I think you'll find several with
> smaller file alignment. Iirc anything down to 0x20 is fine with the Windows
> driver loader (and the EFI one as well). I don't see any reason to use
> larger values than needed here, as there's no demand paging involved,
> where the higher alignment indeed helps performance.

I saw this earlier in vmlinuz. However, it looks that even MS ignores
own specs. Ehhh... OK, will change this to 0x20.

> >> > --- 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.
>
> That last aspect is the important one, and the reason we try to morph
> assembly code over into C where possible.

OK, will do that then.

> >> > +        /*
> >> > +         * 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.
>
> Having this in .exe files is indeed helpful (or at least was back when DOS 
> still
> played some sort of a role). In .dll it is already highly questionable, and
> hence even more so in .efi. Let's not encode and carry cruft that's not
> needed for anything.

OK, but I think that we should leave at least one or two instructions here, e.g.
hlt and jmp back to it or something like that. Or int 0x21 with 0x4c00 in %ax.
Latter seems better for me.

> >> > @@ -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.
>
> My remark wasn't because I expect 1:1 matching output. However, core
> functionality needs to be the same, and iirc this being exactly 16Mb has a
> reason in today's xen.efi (you may want to fish out the commit).

OK, I will take a look.

Daniel

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