Re: [Xen-devel] [PATCH v7 12/14] x86: make Xen early boot code relocatable

[Daniel Kiper <daniel.kiper@xxxxxxxxxx>]

On Mon, Sep 26, 2016 at 09:03:30AM -0600, Jan Beulich wrote:
> >>> On 23.09.16 at 23:47, <daniel.kiper@xxxxxxxxxx> wrote:
> > @@ -426,32 +453,65 @@ trampoline_bios_setup:
> >          xor     %cl, %cl
> >
> >  trampoline_setup:
> > +        /*
> > +         * Called on legacy BIOS and EFI platforms.
> > +         *
> > +         * Initialize 0-15 bits of BOOT_FS segment descriptor base address.
> > +         */
> > +        mov     %si,BOOT_FS+2+sym_esi(trampoline_gdt)
> > +
> > +        /* Initialize 16-23 bits of BOOT_FS segment descriptor base 
> > address. */
> > +        mov     %esi,%edx
> > +        shr     $16,%edx
>
> I'd have liked it even better if you had done this with a single
> instruction, but anyway.

Do you think about "shld $16,%esi,%edx"?

> > @@ -474,23 +534,53 @@ trampoline_setup:
> >
> >          /* Stash TSC to calculate a good approximation of time-since-boot 
> > */
> >          rdtsc
> > -        mov     %eax,sym_phys(boot_tsc_stamp)
> > -        mov     %edx,sym_phys(boot_tsc_stamp+4)
> > +        mov     %eax,sym_fs(boot_tsc_stamp)
> > +        mov     %edx,sym_fs(boot_tsc_stamp)+4
> > +
> > +        /*
> > +         * Update frame addresses in page tables excluding l2_identmap
> > +         * without its first entry which points to l1_identmap.
> > +         */
> > +        mov     $((__page_tables_end-__page_tables_start)/8),%ecx
> > +        mov     $(((l2_identmap-__page_tables_start)/8)+2),%edx
>
> The +2 instead of +1 here is confusing. Why don't you do the natural
> thing here and ...
>
> > +1:      cmp     
> > $((l2_identmap+l2_identmap_sizeof-__page_tables_start)/8),%ecx
> > +        cmove   %edx,%ecx
> > +        je      2f
>
> ... simply drop this branch?

Make sense. I will do that.

> > +        testl   $_PAGE_PRESENT,sym_fs(__page_tables_start)-8(,%ecx,8)
> > +        jz      2f
> > +        add     %esi,sym_fs(__page_tables_start)-8(,%ecx,8)
> > +2:      loop    1b
> > +
> > +        /* Initialize L2 boot-map/direct map page table entries (14MB). */
> > +        lea     sym_esi(start),%ebx
> > +        lea     
> > (1<<L2_PAGETABLE_SHIFT)*7+(PAGE_HYPERVISOR|_PAGE_PSE)(%ebx),%eax
> > +        shr     $(L2_PAGETABLE_SHIFT-3),%ebx
> > +        mov     $8,%ecx
>
> The comment saying 14Mb kind of contradicts this being 8 here.

Ahhh... Right, comment is wrong.

> > +1:      mov     %eax,sym_fs(l2_bootmap)-8(%ebx,%ecx,8)
> > +        mov     %eax,sym_fs(l2_identmap)-8(%ebx,%ecx,8)
> > +        sub     $(1<<L2_PAGETABLE_SHIFT),%eax
> > +        loop    1b
> > +
> > +        /* Initialize L3 boot-map page directory entry. */
> > +        lea     
> > __PAGE_HYPERVISOR+(L2_PAGETABLE_ENTRIES*8)*3+sym_esi(l2_bootmap),%eax
> > +        mov     $4,%ecx
> > +1:      mov     %eax,sym_fs(l3_bootmap)-8(,%ecx,8)
> > +        sub     $(L2_PAGETABLE_ENTRIES*8),%eax
> > +        loop    1b
> >
> >          /*
> >           * During boot, hook 4kB mappings of first 2MB of memory into L2.
> > -         * This avoids mixing cachability for the legacy VGA region, and is
> > -         * corrected when Xen relocates itself.
> > +         * This avoids mixing cachability for the legacy VGA region.
> >           */
> > -        mov     $sym_phys(l1_identmap)+__PAGE_HYPERVISOR,%edi
> > -        mov     %edi,sym_phys(l2_xenmap)
> > +        lea     __PAGE_HYPERVISOR+sym_esi(l1_identmap),%edi
> > +        mov     %edi,sym_fs(l2_bootmap)
>
> Switching from l2_xenmap to l2_bootmap here?

Do we need first 2 MiB mapped in Xen image mapping? It looks that we do not.
Am I missing something?

I must initialize l2_bootmap first entry with l1_identmap here because
I removed relevant static initialization from x86_64.S.

> > @@ -121,8 +127,9 @@ GLOBAL(l2_identmap)
> >   * page.
> >   */
> >  GLOBAL(l2_xenmap)
> > -        idx = 0
> > -        .rept 8
> > +        .quad 0
> > +        idx = 1
> > +        .rept 7
> >          .quad sym_phys(__image_base__) + (idx << L2_PAGETABLE_SHIFT) + 
> > (PAGE_HYPERVISOR | _PAGE_PSE)
> >          idx = idx + 1
> >          .endr
>
> How come the first entry doesn't need filling anymore? I think such
> less obvious changes should really get briefly mentioned/explained
> in the commit message.

Ditto. I will add a few words about that to commit message.

> > @@ -674,6 +671,8 @@ void __init noreturn __start_xen(unsigned long mbi_p)
> >
> >      printk("Command line: %s\n", cmdline);
> >
> > +    printk("Xen image load base address: 0x%08lx\n", xen_phys_start);
>
> Please prefer %#lx in cases like this.

If I do that then 0 is displayed as 0 instead of 0x00000000. I prefer
latter. If you do not care I can use "%#lx" as you wish.

> > @@ -1018,6 +1018,8 @@ void __init noreturn __start_xen(unsigned long mbi_p)
> >                  : "memory" );
> >
> >              bootstrap_map(NULL);
> > +
> > +            printk("New Xen image base address: %#08lx\n", xen_phys_start);
>
> # and a minimum width generally don't fit together well.

Why?

Daniel

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel