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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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.

> @@ -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?

> +        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.

> +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?

> @@ -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.

> @@ -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.

> @@ -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.

Jan

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