Re: [PATCH v2 22/23] x86: make Xen early boot code relocatable

[Jan Beulich]

>>> On 20.07.15 at 16:29, <address@hidden> wrote:
>   - %fs register is filled with segment descriptor which describes memory 
> region
>     with Xen image (it could be relocated or not);

This is too fuzzy. Please be very precise which region it is that %fs
is supposed to point to (so that reviewers have a chance to validate
uses).

> --- a/xen/arch/x86/Makefile
> +++ b/xen/arch/x86/Makefile
> @@ -72,8 +72,10 @@ efi-$(x86_64) := $(shell if [ ! -r 
> $(BASEDIR)/include/xen/compile.h -o \
>                           echo '$(TARGET).efi'; fi)
>  
>  $(TARGET): $(TARGET)-syms $(efi-y) boot/mkelf32
> -     ./boot/mkelf32 $(TARGET)-syms $(TARGET) 0x100000 \
> -     `$(NM) -nr $(TARGET)-syms | head -n 1 | sed -e 's/^\([^ ]*\).*/0x\1/'`
> +#    THIS IS UGLY HACK! PLEASE DO NOT COMPLAIN. I WILL FIX IT IN NEXT 
> RELEASE.
> +     ./boot/mkelf32 $(TARGET)-syms $(TARGET) $(XEN_IMG_PHYS_START) 
> 0xffff82d081000000
> +#    ./boot/mkelf32 $(TARGET)-syms $(TARGET) 0x100000 \
> +#    `$(NM) -nr $(TARGET)-syms | head -n 1 | sed -e 's/^\([^ ]*\).*/0x\1/'`

I do complain nevertheless: Such a patch should be tagged RFC. And
with such a hack in place I'm not even sure it's worth reviewing.

> --- a/xen/arch/x86/boot/head.S
> +++ b/xen/arch/x86/boot/head.S
> @@ -12,13 +12,15 @@
>          .text
>          .code32
>  
> -#define sym_phys(sym)     ((sym) - __XEN_VIRT_START)
> +#define sym_phys(sym)     ((sym) - __XEN_VIRT_START + XEN_IMG_PHYS_START - 
> XEN_IMG_OFFSET)
> +#define sym_offset(sym)   ((sym) - __XEN_VIRT_START)

With XEN_IMG_PHYS_START == XEN_IMG_OFFSET - what's the point?
(If there is a point, then there's obviously a comment missing here
explaining things, including when to use which.)

> @@ -105,12 +107,13 @@ multiboot1_header_end:
>  
>          .word   0
>  gdt_boot_descr:
> -        .word   6*8-1
> -        .long   sym_phys(trampoline_gdt)
> +        .word   7*8-1
> +gdt_boot_descr_addr:

gdt_boot_base would seem a better name; with C in mind what
you use currently could be easily mistaken for a variable holding
&gdt_boot_descr.

> @@ -263,12 +271,8 @@ __start:
>          cld
>          cli
>  
> -        /* Initialise GDT and basic data segments. */
> -        lgdt    %cs:sym_phys(gdt_boot_descr)
> -        mov     $BOOT_DS,%ecx
> -        mov     %ecx,%ds
> -        mov     %ecx,%es
> -        mov     %ecx,%ss
> +        /* Load default Xen image base address. */
> +        mov     $sym_phys(__image_base__),%ebp

Isn't this always zero? I.e. wouldn't you better use "xor %ebp,%ebp"?

>          /* Save the Multiboot info struct (after relocation) for later use. 
> */
> -        mov     $sym_phys(cpu0_stack)+1024,%esp
> +        lea     (sym_offset(cpu0_stack)+1024)(%ebp),%esp

Please avoid obfuscating the code by adding pointless parentheses.

> @@ -390,72 +432,111 @@ 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,%fs:sym_offset(boot_tsc_stamp)
> +        mov     %edx,%fs:sym_offset(boot_tsc_stamp+4)
>  
> -        /* Initialise L2 boot-map page table entries (16MB). */
> -        mov     $sym_phys(l2_bootmap),%edx
> -        mov     $PAGE_HYPERVISOR|_PAGE_PSE,%eax
> -        mov     $8,%ecx
> +        /* Update frame addreses in page tables. */
> +        lea     sym_offset(__page_tables_start)(%ebp),%edx
> +        mov     $((__page_tables_end-__page_tables_start)/8),%ecx
> +1:      testl   $_PAGE_PRESENT,(%edx)
> +        jz      2f
> +        add     %ebp,(%edx)
> +2:      add     $8,%edx
> +        loop    1b
> +
> +        /* Initialise L2 boot-map page table entries (14MB). */
> +        lea     sym_offset(l2_bootmap)(%ebp),%edx
> +        lea     sym_offset(start)(%ebp),%eax
> +        and     $~((1<<L2_PAGETABLE_SHIFT)-1),%eax
> +        mov     %eax,%ebx
> +        shr     $(L2_PAGETABLE_SHIFT-3),%ebx
> +        and     $(L2_PAGETABLE_ENTRIES*4*8-1),%ebx
> +        add     %ebx,%edx
> +        add     $(PAGE_HYPERVISOR|_PAGE_PSE),%eax
> +        mov     $7,%ecx
>  1:      mov     %eax,(%edx)
>          add     $8,%edx
>          add     $(1<<L2_PAGETABLE_SHIFT),%eax
>          loop    1b
> +
>          /* Initialise L3 boot-map page directory entry. */
> -        mov     $sym_phys(l2_bootmap)+__PAGE_HYPERVISOR,%eax
> -        mov     %eax,sym_phys(l3_bootmap) + 0*8
> +        lea     (sym_offset(l2_bootmap)+__PAGE_HYPERVISOR)(%ebp),%eax
> +        lea     sym_offset(l3_bootmap)(%ebp),%ebx
> +        mov     $4,%ecx
> +1:      mov     %eax,(%ebx)
> +        add     $8,%ebx
> +        add     $(L2_PAGETABLE_ENTRIES*8),%eax
> +        loop    1b
> +
> +        /* Initialise L2 direct map page table entries (14MB). */
> +        lea     sym_offset(l2_identmap)(%ebp),%edx
> +        lea     sym_offset(start)(%ebp),%eax
> +        and     $~((1<<L2_PAGETABLE_SHIFT)-1),%eax
> +        mov     %eax,%ebx
> +        shr     $(L2_PAGETABLE_SHIFT-3),%ebx
> +        and     $(L2_PAGETABLE_ENTRIES*4*8-1),%ebx
> +        add     %ebx,%edx
> +        add     $(PAGE_HYPERVISOR|_PAGE_PSE),%eax
> +        mov     $7,%ecx
> +1:      mov     %eax,(%edx)
> +        add     $8,%edx
> +        add     $(1<<L2_PAGETABLE_SHIFT),%eax
> +        loop    1b
> +
>          /* Hook 4kB mappings of first 2MB of memory into L2. */
> -        mov     $sym_phys(l1_identmap)+__PAGE_HYPERVISOR,%edi
> -        mov     %edi,sym_phys(l2_xenmap)
> -        mov     %edi,sym_phys(l2_bootmap)
> +        lea     (sym_offset(l1_identmap)+__PAGE_HYPERVISOR)(%ebp),%edi
> +        mov     %edi,%fs:sym_offset(l2_bootmap)
>  
>          /* Apply relocations to bootstrap trampoline. */
> -        mov     sym_phys(trampoline_phys),%edx
> -        mov     $sym_phys(__trampoline_rel_start),%edi
> +        mov     %fs:sym_offset(trampoline_phys),%edx
> +        lea     sym_offset(__trampoline_rel_start)(%ebp),%edi
> +        lea     sym_offset(__trampoline_rel_stop)(%ebp),%esi
>  1:
>          mov     (%edi),%eax
>          add     %edx,(%edi,%eax)
>          add     $4,%edi
> -        cmp     $sym_phys(__trampoline_rel_stop),%edi
> +        cmp     %esi,%edi
>          jb      1b
>  
>          /* Patch in the trampoline segment. */
>          shr     $4,%edx
> -        mov     $sym_phys(__trampoline_seg_start),%edi
> +        lea     sym_offset(__trampoline_seg_start)(%ebp),%edi
> +        lea     sym_offset(__trampoline_seg_stop)(%ebp),%esi
>  1:
>          mov     (%edi),%eax
>          mov     %dx,(%edi,%eax)
>          add     $4,%edi
> -        cmp     $sym_phys(__trampoline_seg_stop),%edi
> +        cmp     %esi,%edi
>          jb      1b
>  
>          /* Do not parse command line on EFI platform here. */
> -        cmpb    $1,sym_phys(skip_realmode)
> +        cmpb    $1,%fs:sym_offset(skip_realmode)
>          je      1f
>  
>          /* Bail if there is no command line to parse. */
> -        mov     sym_phys(multiboot_ptr),%ebx
> +        mov     %fs:sym_offset(multiboot_ptr),%ebx
>          testl   $MBI_CMDLINE,MB_flags(%ebx)
>          jz      1f
>  
>          cmpl    $0,MB_cmdline(%ebx)
>          jz      1f
>  
> -        pushl   $sym_phys(early_boot_opts)
> +        lea     sym_offset(early_boot_opts)(%ebp),%eax
> +        push    %eax
>          pushl   MB_cmdline(%ebx)
>          call    cmdline_parse_early
>          add     $8,%esp             /* Remove cmdline_parse_early() args 
> from stack. */
>  
>  1:
>          /* Switch to low-memory stack.  */
> -        mov     sym_phys(trampoline_phys),%edi
> +        mov     %fs:sym_offset(trampoline_phys),%edi
>          lea     0x10000(%edi),%esp
>          lea     trampoline_boot_cpu_entry-trampoline_start(%edi),%eax
>          pushl   $BOOT_CS32
>          push    %eax
>  
>          /* Copy bootstrap trampoline to low memory, below 1MB. */
> -        mov     $sym_phys(trampoline_start),%esi
> +        lea     sym_offset(trampoline_start)(%ebp),%esi
>          mov     $trampoline_end - trampoline_start,%ecx
>          rep     movsb
>  

The latest at this final hunk I'm getting tired (and upset). I'd much
rather not touch all this code in these fragile ways, and instead
require Xen to be booted without grub2 on badly written firmware
like the one on the machine you quote in the description.

Jan