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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 04.07.18 at 18:35, <daniel.kiper@xxxxxxxxxx> wrote:
> 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:
>> >> > $(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".

Oh, sorry - I meant to say "Not exactly - you can simply leave this line
alone in the later patch then."

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

Commit message: You want to explain why the _change_ is correct.

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

Well - this is the code in question, but you fail to point out where
the off-by-1 is.

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

Taking your comment, there must be (a) something said in the spec
and (b) its "violation" leading to problems. I guess if I dug carefully
enough I might be able to find (a), so it is (b) that I'm asking 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.

Okay; it would have helped if you also gave the reasons here (rather
than just saying "yes").

Jan


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