On Tue, Jul 10, 2018 at 07:54:51AM -0600, Jan Beulich wrote: > >>> On 10.07.18 at 12:48, <daniel.ki...@oracle.com> wrote: > > On Fri, Jul 06, 2018 at 09:08:29AM -0600, Jan Beulich wrote: > >> >>> On 06.07.18 at 16:02, <daniel.ki...@oracle.com> wrote: > >> > On Thu, Jul 05, 2018 at 02:18:03AM -0600, Jan Beulich wrote: > >> >> >>> On 04.07.18 at 18:35, <daniel.ki...@oracle.com> wrote: > >> >> > On Wed, Jul 04, 2018 at 09:27:43AM -0600, Jan Beulich wrote: > >> >> >> >>> On 04.07.18 at 16:01, <daniel.ki...@oracle.com> wrote: > >> >> >> > On Mon, Jun 25, 2018 at 09:36:07AM -0600, Jan Beulich wrote: > >> >> >> >> >>> On 19.06.18 at 16:35, <daniel.ki...@oracle.com> 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? Daniel _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
