Re: [Xen-devel] [PATCH v2 3/8] xen/x86: manually build xen.mb.efi binary

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 04.07.18 at 16:01, <daniel.kiper@xxxxxxxxxx> wrote:
> On Mon, Jun 25, 2018 at 09:36:07AM -0600, Jan Beulich wrote:
>> >>> On 19.06.18 at 16:35, <daniel.kiper@xxxxxxxxxx> wrote:
>> >    - DOS stub code reduction; experiments showed that DOS stub code size
>> >      cannot be changed due to some bugs in applications playing with PE
>> >      files, e.g. objdump (more about the issue can be found in the patch
>> >      itself); so, I think that if it is not possible to reduce the size
>> >      of code then it does make sens change the code itself; hence, it
>> >      pays to leave common DOS stub code as is.
>>
>> Even more so here: I'm not sure I care about buggy tools. Did you
>> at least enter a bug report (which you would want to reference in the
>> code comment)? For all of my Win32 life I've been doing fine shrinking
>> DLL/EXE file sizes by moving the PE header to offset 0x40. No tool
>> has even complained. I've just tried objdump 2.25.0 on one of these
>> DLLs - no problem afaics. Did the tool perhaps choke on something
>> other than the non-"standard" offset of the PE header?
> 
> I have tried objdump from binutils 2.22. It fails. OK, this is fixed
> in later versions but Xen README says that we support at least binutils
> 2.16.91.0.5.

And objdump is needed again in which step of the build process?

> Of course this is not very big issue but...
> 
>> As to leaving the code as is - if there's anything to be left as is, then
>> the code live binutils would produce, i.e. I'd then ask you to obtain
>> the code at build time,
> 
> This probably will reintroduce dependency on i386ep emulation which
> we are trying to avoid.

Hmm, true.

>> > --- a/xen/Makefile
>> > +++ b/xen/Makefile
>> > @@ -61,6 +61,10 @@ _install: $(TARGET)$(CONFIG_XEN_INSTALL_SUFFIX)
>> >    ln -f -s $(T)-$(XEN_FULLVERSION)$(Z)
>> > $(D)$(BOOT_DIR)/$(T)-$(XEN_VERSION).$(XEN_SUBVERSION)$(Z)
>> >    ln -f -s $(T)-$(XEN_FULLVERSION)$(Z) 
>> > $(D)$(BOOT_DIR)/$(T)-$(XEN_VERSION)$(Z)
>> >    ln -f -s $(T)-$(XEN_FULLVERSION)$(Z) $(D)$(BOOT_DIR)/$(T)$(Z)
>> > +  $(INSTALL_DATA) $(TARGET).mb.efi 
>> > $(D)$(BOOT_DIR)/$(T)-$(XEN_FULLVERSION).mb.efi
>> > +  ln -f -s $(T)-$(XEN_FULLVERSION).mb.efi 
>> > $(D)$(BOOT_DIR)/$(T)-$(XEN_VERSION).$(XEN_SUBVERSION).mb.efi
>> > +  ln -f -s $(T)-$(XEN_FULLVERSION).mb.efi 
>> > $(D)$(BOOT_DIR)/$(T)-$(XEN_VERSION).mb.efi
>> > +  ln -f -s $(T)-$(XEN_FULLVERSION).mb.efi $(D)$(BOOT_DIR)/$(T).mb.efi
>>
>> This suggests something wants to be macro-ized here, I think.
> 
> I do not think it pays. Last patch renames xen.mb.efi to xen.efi
> and drops this code.

Right - I hadn't seen the later patches yet. So if everything is to go in
together, then fine with me.

>> > $(D)$(DEBUG_DIR)/$(T)-syms-$(XEN_FULLVERSION).map
>> > @@ -121,7 +125,7 @@ _clean: delete-unfresh-files
>> >    $(MAKE) -f $(BASEDIR)/Rules.mk -C test clean
>> >    $(MAKE) -f $(BASEDIR)/tools/kconfig/Makefile.kconfig ARCH=$(ARCH)
>> > SRCARCH=$(SRCARCH) clean
>> >    find . \( -name "*.o" -o -name ".*.d" -o -name "*.gcno" \) -exec rm -f 
>> > {} \;
>> > -  rm -f include/asm $(TARGET) $(TARGET).gz $(TARGET).efi 
>> > $(TARGET).efi.map $(TARGET)-syms $(TARGET)-syms.map *~ core
>> > +  rm -f include/asm $(TARGET) $(TARGET).gz $(TARGET).efi $(TARGET).mb.efi 
>> > $(TARGET).efi.map $(TARGET)-syms $(TARGET)-syms.map *~ core
>>
>> Perhaps simply $(TARGET)*.efi? I don't think we're at risk deleting 
> something
>> precious that way.
> 
> Ditto.

No exactly - you leave this line alone in the later patch then.

>> > +GLOBAL(efi_pe_head)
>> > +        /*
>> > +         * Legacy EXE header.
>> > +         *
>> > +         * Most of it is copied from binutils package, version 2.30,
>> > +         * 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. */
>>
>> This is just the most prominent example: Why is this a hard coded
> 
> Example of what?

Example of questionable uses of plain numbers.

>> number, while ...
> 
> This is standard DOS stub. Additionally, this is not real relocation
> table. Just fake one. The pointer points to the DOS stub code. Of
> course we can change that but does it pays?

Put yourself in the position of a reader not knowing all the details.
The first question on any of these plain numbers will be: Why is it
this number, and not some arbitrary other one. Something like
"number of sections" can still be derived if necessary, but I'd
prefer if you used calculations instead of raw numbers wherever
possible.

>> > +        .long   __2M_rwdata_end - efi_pe_head_end    /* SizeOfCode. */
>> > +        .long   0                                    /* 
>> > SizeOfInitializedData. */
>> > +        .long   0                                    /* 
>> > SizeOfUninitializedData. */
>> > +        .long   sym_offs(efi_mb_start)               /* 
>> > AddressOfEntryPoint. */
>> > +        .long   sym_offs(start)                      /* BaseOfCode. */
>> > +        .quad   sym_offs(__image_base__)             /* ImageBase. */
>>
>> The sym_offs() here is certainly different from what xen.efi
>> currently has. With the plan being to have a drop-in replacement,
>> such differences need to be explained to be benign (which here
>> I doubt it is).
> 
> We share the code with ELF file, so, both have the same __image_base__ 
> address.

But the question wasn't about __image_base__, but the use of sym_offs()
on it. Again - for this new binary to be a drop-in replacement for the
current xen.efi, all such differences need to be explained.

>> > +        .align XEN_FILE_ALIGN
>> > +GLOBAL(efi_pe_head_end)
>> > +
>> > +        .text
>> > +        .code32
>>
>> Why the .code32 here? Perhaps this comes back to the question of
> 
> It looks that I have just copied this from the beginning of the file
> during initial work on this patch. However, there is a chance that it
> can be dropped.
> 
>> whether this whole header should really be lumped into this file.
> 
> I can move it to separate S file if you wish.

This would help readability quite a bit, I think.

>> > @@ -582,6 +587,12 @@ static void __init efi_arch_memory_setup(void)
>> >      if ( !efi_enabled(EFI_LOADER) )
>> >          return;
>> >
>> > +    if ( efi_enabled(EFI_MB_LOADER) )
>> > +        for ( pte = __page_tables_start; pte < __page_tables_end;
>> > +              pte += ( pte != (intpte_t *)l2_identmap ) ? 1 : 4 * 
>> > L2_PAGETABLE_ENTRIES )
>>
>> Please avoid explicit casts - &l2e_get_intpte(l2_identmap[0]) or
>> something along those lines ought to work here. Same for
>> 4 * L2_PAGETABLE_ENTRIES - you mean ARRAY_SIZE() there.
> 
> OK.
> 
>> Also this whole code block needs a comment, to explain what it
>> does and also why l2_identmap needs skipping.
>>
>> Furthermore - isn't this off by one, and you process l2_identmap[0]
>> this way, skipping the rest _plus_ the first following entry? I think
> 
> The code mimics similar code in head.S.

I can't see a similar off-by-1 in head.S.

>> > @@ -271,6 +284,9 @@ SECTIONS
>> >         *(.data.rel)
>> >         *(.data.rel.*)
>> >         CONSTRUCTORS
>> > +       /* PE file must end at XEN_FILE_ALIGN boundary. */
>> > +       . = ALIGN(XEN_FILE_ALIGN);
>> > +       __pe_text_raw_end = .;
>>
>> Is this really a requirement on the file, or just on the label?
> 
> File, so, probably it can be moved behind the label. Though it means
> that __pe_text_raw_end will not point to the real end of .text section.

This is an answer contradicting itself: If the requirement indeed
is on the file, then things need to remain as is. I'm wondering
though what entity would enforce this requirement (if such
exists in the first place).

>> > @@ -292,6 +308,8 @@ SECTIONS
>> >    . = ALIGN(SECTION_ALIGN);
>> >    __2M_rwdata_end = .;
>> >
>> > +  __pe_SizeOfImage = ALIGN(. - __image_base__, MB(16));
>> > +
>> >  #ifdef EFI
>> >    . = ALIGN(4);
>> >    .reloc : {
>>
>> Considering the respective code and comment inside the #ifdef, does
>> your addition really belong ahead of it?
> 
> Yes, so, it looks that it requires some comment as code above.

I'm afraid I don't understand.

Jan


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