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

[Daniel Kiper <daniel.kiper@xxxxxxxxxx>]

On Fri, Jul 06, 2018 at 09:08:29AM -0600, Jan Beulich wrote:
> >>> On 06.07.18 at 16:02, <daniel.kiper@xxxxxxxxxx> wrote:
> > 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.
>
> I don't think so, no. Note the -8 in lines 670 and 672.

However, you are missing +1 in line 667.

> >> >> >> > @@ -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.
>
> Well, as said before - there's a variety of Windows drivers with much
> smaller alignment (often 32).

The most important is first sentence here. Others I have put just
for completeness. We have discussed that earlier.

> > And e.g. at least sbsign is very picky and complains if sections are
> > not aligned correctly in the PE file.
>
> That's possibly relevant, and would make up for a good reason here.
> In which case, with the comment minimally extended, I'm fine with the
> change as still visible above.

Sure thing!

Daniel

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