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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 12.07.18 at 12:52, <daniel.kiper@xxxxxxxxxx> wrote:
> On Wed, Jul 11, 2018 at 06:26:23AM -0600, Jan Beulich wrote:
>> >>> 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.
> 
> Do you suggest that we should do that backwards as we do in ASM?
> Or anything else?

The closer the match, the better imo. I wouldn't insist on it being
done backwards though, unless there's a particular reason for that.

Jan


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