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

[Daniel Kiper <daniel.kiper@xxxxxxxxxx>]

On Wed, Jul 04, 2018 at 09:27:43AM -0600, Jan Beulich wrote:
> >>> 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?

It is not needed by the build process. However, if somebody builds Xen with old
binutils then he/she will not be able to do PE analysis with objdump on the
same machine. Nothing scary but a bit annoying...

[...]

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

Last patch touches this line and drops "$(TARGET).mb.efi $(TARGET).efi.map".

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

OK, will do.

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

OK. Do you wish this particular thing in the code or in the commit message?

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

OK.

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

 662         /*
 663          * Update frame addresses in page tables excluding l2_identmap
 664          * without its first entry which points to l1_identmap.
 665          */
 666         mov     $((__page_tables_end-__page_tables_start)/8),%ecx
 667         mov     $(((l2_identmap-__page_tables_start)/8)+1),%edx
 668 1:      cmp     
$((l2_identmap+l2_identmap_sizeof-__page_tables_start)/8),%ecx
 669         cmove   %edx,%ecx
 670         testl   $_PAGE_PRESENT,sym_fs(__page_tables_start)-8(,%ecx,8)
 671         jz      2f
 672         add     %esi,sym_fs(__page_tables_start)-8(,%ecx,8)
 673 2:      loop    1b

Anyway, I will add the comment in both places that they should be kept in sync.

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

I am not sure what kind of entity you think about.

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

Yes, __pe_SizeOfImage ... code belongs ahead of #ifdef EFI. Looking at your
question it seems to me that I should put a few words of comment here to
clarify why I do that thing.

Daniel

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