From: "Jan Beulich" <JBeulich@suse.com>
To: Daniel Kiper <daniel.kiper@oracle.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>,
xen-devel <xen-devel@lists.xenproject.org>
Subject: Re: [PATCH RFC 2/7] xen/x86: Manually build PE header
Date: Mon, 14 May 2018 04:40:39 -0600 [thread overview]
Message-ID: <5AF967A702000078001C26DA@prv1-mh.provo.novell.com> (raw)
In-Reply-To: <20180508124735.GH8391@olila.local.net-space.pl>
>>> On 08.05.18 at 14:47, <daniel.kiper@oracle.com> wrote:
> 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:
>> > --- 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
next prev parent reply other threads:[~2018-05-14 10:40 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-07-08 21:53 [PATCH RFC 0/7] Change xen.efi build and add SHIM_LOCK verification into efi_multiboot2() Daniel Kiper
2017-07-08 21:53 ` [PATCH RFC 1/7] xen: Introduce XEN_COMPILE_POSIX_TIME Daniel Kiper
2018-04-30 15:56 ` Jan Beulich
2018-05-08 12:18 ` Daniel Kiper
2018-05-14 10:30 ` Jan Beulich
2018-05-14 16:25 ` Daniel Kiper
2018-05-15 7:47 ` Jan Beulich
2017-07-08 21:53 ` [PATCH RFC 2/7] xen/x86: Manually build PE header Daniel Kiper
2018-05-04 15:38 ` Jan Beulich
2018-05-08 12:47 ` Daniel Kiper
2018-05-14 10:40 ` Jan Beulich [this message]
2018-05-14 16:52 ` Daniel Kiper
2018-05-15 8:01 ` Jan Beulich
2017-07-08 21:53 ` [PATCH RFC 3/7] xen/x86: Add some addresses to the Multiboot header Daniel Kiper
2018-05-04 15:40 ` Jan Beulich
2018-05-08 13:01 ` Daniel Kiper
2017-07-08 21:53 ` [PATCH RFC 4/7] xen/x86: Add some addresses to the Multiboot2 header Daniel Kiper
2017-07-08 21:53 ` [PATCH RFC 5/7] efi: split out efi_shim_lock() Daniel Kiper
2017-07-08 21:53 ` [PATCH RFC 6/7] xen/x86/efi: Verify dom0 kernel with SHIM_LOCK protocol in efi_multiboot2() Daniel Kiper
2018-05-04 15:46 ` Jan Beulich
2018-05-08 13:09 ` Daniel Kiper
2018-05-14 10:43 ` Jan Beulich
2018-05-14 16:56 ` Daniel Kiper
2018-05-15 8:06 ` Jan Beulich
2017-07-08 21:53 ` [PATCH RFC 7/7] xen/x86: Build xen.mb.efi directly from xen-syms Daniel Kiper
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5AF967A702000078001C26DA@prv1-mh.provo.novell.com \
--to=jbeulich@suse.com \
--cc=andrew.cooper3@citrix.com \
--cc=daniel.kiper@oracle.com \
--cc=xen-devel@lists.xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.