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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 11.07.18 at 13:41, <daniel.kiper@xxxxxxxxxx> wrote:
> On Tue, Jul 10, 2018 at 07:54:51AM -0600, Jan Beulich wrote:
>> >>> On 10.07.18 at 12:48, <daniel.kiper@xxxxxxxxxx> wrote:
>> > 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.
>>
>> I don't think I am: The first entry of l2_identmap actually needs
>> processing afaics (and as the comment says), as that's the only one
>> with non-absolute contents. IOW - part of my original comment was
>> wrong, but the other half (you skipping one entry) still seems
>> applicable, as does the part concerning the missing comment.
> 
> It seems correct to me. l2_identmap[0] gets updated and then
> we move to l3_bootmap[0]. Am I missing something?

Hmm, yes, I think you're right. But the way you've coded it it's
less obvious than in the assembly variant, and typically it should
be the other way around. I'd really like you to make this a closer
match.

Jan



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