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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

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

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

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

Jan


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