|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v2 3/8] xen/x86: manually build xen.mb.efi binary
On Mon, Jun 25, 2018 at 09:36:07AM -0600, Jan Beulich wrote:
> >>> On 19.06.18 at 16:35, <daniel.kiper@xxxxxxxxxx> wrote:
> > Not done:
> > - ASM PE header conversion to C; not feasible,
>
> Hmm. As long as you can convince Andrew to give you an ack, I
> won't veto it. But I continue to dislike it, and hence I don't
> currently foresee myself acking it.
Well, I am not sure why are insisting on C here. Andrew and I have tried
to rewrite two different Xen headers to C and we failed. We have discussed
that on IRC. If you give us a hint how to proceed further with that then
I am happy to try once again. Otherwise I think that it is not fair to
require anything which is not technically feasible. Even if I understand
that you do not like currently proposed solution.
> > - 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. 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.
> rather than inserting a series of magic hex
IIRC this magic hex is a standard DOS stub used here in there.
However, if you do not like it I can open code the stub.
> values in the sources. But as said - even better would be to omit this
> altogether.
>
> > --- 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.
> > $(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.
> > --- a/xen/arch/x86/Makefile
> > +++ b/xen/arch/x86/Makefile
> > @@ -101,6 +101,9 @@
> > syms-warn-dup-$(CONFIG_SUPPRESS_DUPLICATE_SYMBOL_WARNINGS) :=
> > $(TARGET): $(TARGET)-syms $(efi-y) boot/mkelf32
> > ./boot/mkelf32 $(notes_phdrs) $(TARGET)-syms $(TARGET)
> > $(XEN_IMG_OFFSET) \
> > `$(NM) $(TARGET)-syms | sed -ne 's/^\([^ ]*\) .
> > __2M_rwdata_end$$/0x\1/p'`
> > + $(OBJCOPY) -O binary -S --change-section-address \
> > + ".efi.pe.header-`$(NM) $(TARGET)-syms | sed -ne 's/^\([^ ]*\) .
> > __image_base__$$/0x\1/p'`" \
> > + $(TARGET)-syms $(TARGET).mb.efi
>
> This wants to be a separate rule of a separate $(TARGET).mb.efi target.
OK.
> > +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?
> 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?
> > + .short 0 /* Overlay number. */
> > + .fill 4, 2, 0 /* Reserved words. */
> > + .short 0 /* OEM identifier. */
> > + .short 0 /* OEM information. */
> > + .fill 10, 2, 0 /* Reserved words. */
> > + .long pe_header - efi_pe_head /* File address of
> > the PE header. */
>
> ... this isn't?
>
>
> > + /*
> > + * PE/COFF header.
> > + *
> > + * The PE/COFF format is defined by Microsoft, and is available
> > from
> > + *
> > http://www.microsoft.com/whdc/system/platform/firmware/PECOFF.mspx
> > + *
> > + * Some ideas are taken from Linux kernel and Xen ARM64.
> > + */
> > +
> > +pe_header:
>
> Does this and ones further down really need to be a non-local labels?
No, they do not. I will change all of them.
> > + .ascii "PE\0\0" /* PE signature. */
> > + .short 0x8664 /* Machine:
> > IMAGE_FILE_MACHINE_AMD64 */
> > + .short 1 /* NumberOfSections.
> > */
> > + .long XEN_COMPILE_POSIX_TIME /* TimeDateStamp. */
> > + .long 0 /*
> > PointerToSymbolTable. */
> > + .long 0 /* NumberOfSymbols. */
> > + .short section_table - optional_header /*
> > SizeOfOptionalHeader. */
> > + .short 0x226 /* Characteristics:
> > + *
> > IMAGE_FILE_EXECUTABLE_IMAGE |
> > + *
> > IMAGE_FILE_LARGE_ADDRESS_AWARE |
> > + *
> > IMAGE_FILE_DEBUG_STRIPPED |
> > + *
> > IMAGE_FILE_LINE_NUMS_STRIPPED
> > + */
> > +
> > +optional_header:
> > + .short 0x20b /* PE format: PE32+ */
> > + .byte 0x02 /*
> > MajorLinkerVersion. */
> > + .byte 0x1e /*
> > MinorLinkerVersion. */
>
> Let's not cheat more than needed: I'm pretty sure just zeros will do
> fine here.
OK.
> > + .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.
> > + .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.
> > @@ -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.
> you'd better use ++ here and have an if() inside the for() body.
> Then you can also attach the related part of the comment there
> instead of to the loop header. And I can avoid complaining about
> the stray spaces inside the parentheses.
If you wish why not...
> > @@ -674,6 +685,15 @@ static bool __init
> > efi_arch_use_config_file(EFI_SYSTEM_TABLE *SystemTable)
> >
> > static void __init efi_arch_flush_dcache_area(const void *vaddr, UINTN
> > size) { }
> >
> > +void EFIAPI efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE
> > *SystemTable);
> > +
> > +void EFIAPI __init noreturn
> > +efi_mb_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
> > +{
> > + __set_bit(EFI_MB_LOADER, &efi_flags);
> > + efi_start(ImageHandle, SystemTable);
> > +}
>
> Why yet another entry point? This again speaks against the image
> being a drop-in replacement for xen.efi.
At this point I have to differentiate xen.efi and xen.mb.efi. Both
require different entry points because the code is a little different
in each one. However, this is dropped by last patch which changes
xen.mb.efi to xen.efi and removes old xen.efi code.
> > --- a/xen/arch/x86/xen.lds.S
> > +++ b/xen/arch/x86/xen.lds.S
> > @@ -60,7 +60,20 @@ SECTIONS
> >
> > start_pa = ABSOLUTE(start - __XEN_VIRT_START);
> >
> > +#ifdef EFI
> > . = __XEN_VIRT_START + XEN_IMG_OFFSET;
> > +#else
> > + /*
> > + * The PE header must be followed by .text section which
> > + * starts at __XEN_VIRT_START + XEN_IMG_OFFSET address.
> > + */
> > + . = __XEN_VIRT_START + XEN_IMG_OFFSET - efi_pe_head_end + efi_pe_head;
> > +
> > + DECL_SECTION(.efi.pe.header) {
> > + *(.efi.pe.header)
> > + } :NONE
> > +#endif
>
> The #ifdef wants a brief comment attached I think, since it's quite
> odd to see the #else side deal with EFI as well.
OK.
> > @@ -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.
Do we care?
> > @@ -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.
Daniel
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |