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

[Daniel Kiper <daniel.kiper@xxxxxxxxxx>]

On Thu, Jul 05, 2018 at 02:18:03AM -0600, Jan Beulich wrote:
> >>> 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:

[...]

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

Line 667, 668 and 669.

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

Microsoft Portable Executable and Common Object File Format
Specification, Revision 11, says this:

  FileAlignment: The alignment factor (in bytes) that is used to align
  the raw data of sections in the image file. The value should be a power
  of 2 between 512 and 64 K, inclusive. The default is 512. If the
  SectionAlignment is less than the architecture’s page size, then
  FileAlignment must match SectionAlignment.

And e.g. at least sbsign is very picky and complains if sections are
not aligned correctly in the PE file.

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

The reason is that xen.mb.efi is build from xen-syms. So, we have to
have this symbol defined for ELF output. It does not hurt if we have
it defined for current xen.efi too.

Daniel

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