Re: [Xen-devel] [PATCH v9 07/13] x86: add multiboot2 protocol support for EFI platforms

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 29/09/16 22:42, Daniel Kiper wrote:
> This way Xen can be loaded on EFI platforms using GRUB2 and
> other boot loaders which support multiboot2 protocol.
>
> Signed-off-by: Daniel Kiper <daniel.kiper@xxxxxxxxxx>
> ---
> v9 - suggestions/fixes:
>    - use .L labels instead of numeric ones in multiboot2 data scanning loops
>      (suggested by Jan Beulich).
>
> v8 - suggestions/fixes:
>    - use __bss_start(%rip)/__bss_end(%rip) instead of
>      of .startof.(.bss)(%rip)/$.sizeof.(.bss) because
>      latter is not tested extensively in different
>      built environments yet
>      (suggested by Andrew Cooper),
>    - fix multiboot2 data scanning loop in x86_32 code
>      (suggested by Jan Beulich),
>    - add check for extra mem for mbi data if Xen is loaded
>      via multiboot2 protocol on EFI platform
>      (suggested by Jan Beulich),
>    - improve comments
>      (suggested by Jan Beulich).
>
> v7 - suggestions/fixes:
>    - do not allocate twice memory for trampoline if we were
>      loaded via multiboot2 protocol on EFI platform,
>    - wrap long line
>      (suggested by Jan Beulich),
>    - improve comments
>      (suggested by Jan Beulich).
>
> v6 - suggestions/fixes:
>    - improve label names in assembly
>      error printing code
>      (suggested by Jan Beulich),
>    - improve comments
>      (suggested by Jan Beulich),
>    - various minor cleanups and fixes
>      (suggested by Jan Beulich).
>
> v4 - suggestions/fixes:
>    - remove redundant BSS alignment,
>    - update BSS alignment check,
>    - use __set_bit() instead of set_bit() if possible
>      (suggested by Jan Beulich),
>    - call efi_arch_cpu() from efi_multiboot2()
>      even if the same work is done later in
>      other place right now
>      (suggested by Jan Beulich),
>    - xen/arch/x86/efi/stub.c:efi_multiboot2()
>      fail properly on EFI platforms,
>    - do not read data beyond the end of multiboot2
>      information in xen/arch/x86/boot/head.S
>      (suggested by Jan Beulich),
>    - use 32-bit registers in x86_64 code if possible
>      (suggested by Jan Beulich),
>    - multiboot2 information address is 64-bit
>      in x86_64 code, so, treat it is as is
>      (suggested by Jan Beulich),
>    - use cmovcc if possible,
>    - leave only one space between rep and stosq
>      (suggested by Jan Beulich),
>    - improve error handling,
>    - improve early error messages,
>      (suggested by Jan Beulich),
>    - improve early error messages printing code,
>    - improve label names
>      (suggested by Jan Beulich),
>    - improve comments
>      (suggested by Jan Beulich),
>    - various minor cleanups.
>
> v3 - suggestions/fixes:
>    - take into account alignment when skipping multiboot2 fixed part
>      (suggested by Konrad Rzeszutek Wilk),
>    - improve segment registers initialization
>      (suggested by Jan Beulich),
>    - improve comments
>      (suggested by Jan Beulich and Konrad Rzeszutek Wilk),
>    - improve commit message
>      (suggested by Jan Beulich).
>
> v2 - suggestions/fixes:
>    - generate multiboot2 header using macros
>      (suggested by Jan Beulich),
>    - switch CPU to x86_32 mode before
>      jumping to 32-bit code
>      (suggested by Andrew Cooper),
>    - reduce code changes to increase patch readability
>      (suggested by Jan Beulich),
>    - improve comments
>      (suggested by Jan Beulich),
>    - ignore MULTIBOOT2_TAG_TYPE_BASIC_MEMINFO tag on EFI platform
>      and find on my own multiboot2.mem_lower value,
>    - stop execution if EFI platform is detected
>      in legacy BIOS path.
> ---
>  xen/arch/x86/boot/head.S          |  260 
> ++++++++++++++++++++++++++++++++++---
>  xen/arch/x86/efi/efi-boot.h       |   54 +++++++-
>  xen/arch/x86/efi/stub.c           |   38 ++++++
>  xen/arch/x86/x86_64/asm-offsets.c |    2 +
>  xen/arch/x86/xen.lds.S            |    4 +-
>  xen/common/efi/boot.c             |   11 ++
>  6 files changed, 346 insertions(+), 23 deletions(-)
>
> diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
> index d423fd8..0155b32 100644
> --- a/xen/arch/x86/boot/head.S
> +++ b/xen/arch/x86/boot/head.S
> @@ -89,6 +89,13 @@ multiboot2_header_start:
>                     0, /* Number of the lines - no preference. */ \
>                     0  /* Number of bits per pixel - no preference. */
>  
> +        /* Inhibit bootloader from calling ExitBootServices(). */
> +        mb2ht_init MB2_HT(EFI_BS), MB2_HT(OPTIONAL)
> +
> +        /* EFI64 entry point. */
> +        mb2ht_init MB2_HT(ENTRY_ADDRESS_EFI64), MB2_HT(OPTIONAL), \
> +                   sym_phys(__efi64_start)
> +
>          /* Multiboot2 header end tag. */
>          mb2ht_init MB2_HT(END), MB2_HT(REQUIRED)
>  .Lmultiboot2_header_end:
> @@ -100,20 +107,49 @@ multiboot2_header_start:
>  gdt_boot_descr:
>          .word   6*8-1
>          .long   sym_phys(trampoline_gdt)
> +        .long   0 /* Needed for 64-bit lgdt */
> +
> +cs32_switch_addr:
> +        .long   sym_phys(cs32_switch)
> +        .word   BOOT_CS32
> +
> +        .align 4
> +vga_text_buffer:
> +        .long   0xb8000

Why is this turned into a variable?

>  
>  .Lbad_cpu_msg: .asciz "ERR: Not a 64-bit CPU!"
>  .Lbad_ldr_msg: .asciz "ERR: Not a Multiboot bootloader!"
> +.Lbad_ldr_nbs: .asciz "ERR: Bootloader shutdown EFI x64 boot services!"
> +.Lbad_ldr_nst: .asciz "ERR: EFI SystemTable is not provided by bootloader!"
> +.Lbad_ldr_nih: .asciz "ERR: EFI ImageHandle is not provided by bootloader!"
> +.Lbad_efi_msg: .asciz "ERR: EFI IA-32 platforms are not supported!"
>  
>          .section .init.text, "ax", @progbits
>  
>  bad_cpu:
>          mov     $(sym_phys(.Lbad_cpu_msg)),%esi # Error message
> -        jmp     print_err
> +        jmp     .Lget_vtb
>  not_multiboot:
>          mov     $(sym_phys(.Lbad_ldr_msg)),%esi # Error message
> -print_err:
> -        mov     $0xB8000,%edi  # VGA framebuffer
> -1:      mov     (%esi),%bl
> +        jmp     .Lget_vtb
> +.Lmb2_no_st:
> +        mov     $(sym_phys(.Lbad_ldr_nst)),%esi # Error message
> +        jmp     .Lget_vtb
> +.Lmb2_no_ih:
> +        mov     $(sym_phys(.Lbad_ldr_nih)),%esi # Error message
> +        jmp     .Lget_vtb
> +.Lmb2_no_bs:
> +        mov     $(sym_phys(.Lbad_ldr_nbs)),%esi # Error message
> +        xor     %edi,%edi                       # No VGA text buffer
> +        jmp     .Lsend_chr
> +.Lmb2_efi_ia_32:
> +        mov     $(sym_phys(.Lbad_efi_msg)),%esi # Error message
> +        xor     %edi,%edi                       # No VGA text buffer
> +        jmp     .Lsend_chr
> +.Lget_vtb:
> +        mov     sym_phys(vga_text_buffer),%edi
> +.Lsend_chr:
> +        mov     (%esi),%bl
>          test    %bl,%bl        # Terminate on '\0' sentinel
>          je      .Lhalt
>          mov     $0x3f8+5,%dx   # UART Line Status Register
> @@ -123,13 +159,182 @@ print_err:
>          mov     $0x3f8+0,%dx   # UART Transmit Holding Register
>          mov     %bl,%al
>          out     %al,%dx        # Send a character over the serial line
> -        movsb                  # Write a character to the VGA framebuffer
> +        test    %edi,%edi      # Is the VGA text buffer available?
> +        jz      .Lsend_chr
> +        movsb                  # Write a character to the VGA text buffer
>          mov     $7,%al
> -        stosb                  # Write an attribute to the VGA framebuffer
> -        jmp     1b
> +        stosb                  # Write an attribute to the VGA text buffer
> +        jmp     .Lsend_chr
>  .Lhalt: hlt
>          jmp     .Lhalt
>  
> +        .code64
> +
> +__efi64_start:
> +        cld
> +
> +        /* VGA is not available on EFI platforms. */
> +        movl   $0,vga_text_buffer(%rip)
> +
> +        /* Check for Multiboot2 bootloader. */
> +        cmp     $MULTIBOOT2_BOOTLOADER_MAGIC,%eax
> +        je      .Lefi_multiboot2_proto
> +
> +        /* Jump to not_multiboot after switching CPU to x86_32 mode. */
> +        lea     not_multiboot(%rip),%edi
> +        jmp     x86_32_switch
> +
> +.Lefi_multiboot2_proto:
> +        /* Zero EFI SystemTable and EFI ImageHandle addresses. */
> +        xor     %esi,%esi
> +        xor     %edi,%edi
> +
> +        /* Skip Multiboot2 information fixed part. */
> +        lea     (MB2_fixed_sizeof+MULTIBOOT2_TAG_ALIGN-1)(%rbx),%ecx
> +        and     $~(MULTIBOOT2_TAG_ALIGN-1),%ecx
> +
> +.Lefi_mb2_tsize:
> +        /* Check Multiboot2 information total size. */
> +        mov     %ecx,%r8d
> +        sub     %ebx,%r8d
> +        cmp     %r8d,MB2_fixed_total_size(%rbx)
> +        jbe     run_bs
> +
> +        /* Are EFI boot services available? */
> +        cmpl    $MULTIBOOT2_TAG_TYPE_EFI_BS,MB2_tag_type(%rcx)
> +        jne     .Lefi_mb2_st
> +
> +        /*
> +         * Yes, store that info in skip_realmode variable. I agree that
> +         * its name is a bit unfortunate in this context, however, by the
> +         * way we disable real mode and other legacy stuff which should
> +         * not be run on EFI platforms.
> +         */
> +        incb    skip_realmode(%rip)

Always use add/sub 1 in preference to inc and dec.  They are the same
length to encode in 64bit, and avoids a pipeline stall from a merge of
the eflags register.

> +        jmp     .Lefi_mb2_next_tag
> +
> +.Lefi_mb2_st:
> +        /* Get EFI SystemTable address from Multiboot2 information. */
> +        cmpl    $MULTIBOOT2_TAG_TYPE_EFI64,MB2_tag_type(%rcx)
> +        cmove   MB2_efi64_st(%rcx),%rsi
> +        je      .Lefi_mb2_next_tag
> +
> +        /* Get EFI ImageHandle address from Multiboot2 information. */
> +        cmpl    $MULTIBOOT2_TAG_TYPE_EFI64_IH,MB2_tag_type(%rcx)
> +        cmove   MB2_efi64_ih(%rcx),%rdi
> +        je      .Lefi_mb2_next_tag
> +
> +        /* Is it the end of Multiboot2 information? */
> +        cmpl    $MULTIBOOT2_TAG_TYPE_END,MB2_tag_type(%rcx)
> +        je      run_bs
> +
> +.Lefi_mb2_next_tag:
> +        /* Go to next Multiboot2 information tag. */
> +        add     MB2_tag_size(%rcx),%ecx
> +        add     $(MULTIBOOT2_TAG_ALIGN-1),%ecx
> +        and     $~(MULTIBOOT2_TAG_ALIGN-1),%ecx
> +        jmp     .Lefi_mb2_tsize
> +
> +run_bs:
> +        /* Are EFI boot services available? */
> +        cmpb    $0,skip_realmode(%rip)
> +        jnz     0f
> +
> +        /* Jump to .Lmb2_no_bs after switching CPU to x86_32 mode. */
> +        lea     .Lmb2_no_bs(%rip),%edi
> +        jmp     x86_32_switch
> +
> +0:
> +        /* Is EFI SystemTable address provided by boot loader? */
> +        test    %rsi,%rsi
> +        jnz     1f
> +
> +        /* Jump to .Lmb2_no_st after switching CPU to x86_32 mode. */
> +        lea     .Lmb2_no_st(%rip),%edi
> +        jmp     x86_32_switch
> +
> +1:
> +        /* Is EFI ImageHandle address provided by boot loader? */
> +        test    %rdi,%rdi
> +        jnz     2f
> +
> +        /* Jump to .Lmb2_no_ih after switching CPU to x86_32 mode. */
> +        lea     .Lmb2_no_ih(%rip),%edi
> +        jmp     x86_32_switch
> +
> +2:
> +        push    %rax

What is %rax being preserved for?

> +        push    %rdi
> +
> +        /*
> +         * Initialize BSS (no nasty surprises!).
> +         * It must be done earlier than in BIOS case
> +         * because efi_multiboot2() touches it.
> +         */
> +        lea     __bss_start(%rip),%edi
> +        lea     __bss_end(%rip),%ecx
> +        sub     %edi,%ecx
> +        shr     $3,%ecx
> +        xor     %eax,%eax
> +        rep stosq
> +
> +        pop     %rdi
> +
> +        /*
> +         * efi_multiboot2() is called according to System V AMD64 ABI:
> +         *   - IN: %rdi - EFI ImageHandle, %rsi - EFI SystemTable,
> +         *   - OUT: %rax - highest usable memory address below 1 MiB;
> +         *                 memory above this address is reserved for 
> trampoline;
> +         *                 memory below this address is used as a storage for
> +         *                 mbi struct created in reloc().
> +         *
> +         * MULTIBOOT2_TAG_TYPE_BASIC_MEMINFO tag is not provided
> +         * on EFI platforms. Hence, it could not be used like
> +         * on legacy BIOS platforms.
> +         */
> +        call    efi_multiboot2
> +
> +        /* Convert memory address to bytes/16 and store it in safe place. */
> +        shr     $4,%eax
> +        mov     %eax,%ecx
> +
> +        pop     %rax
> +
> +        /* Jump to trampoline_setup after switching CPU to x86_32 mode. */
> +        lea     trampoline_setup(%rip),%edi
> +
> +x86_32_switch:
> +        cli
> +
> +        /* Initialize GDTR. */
> +        lgdt    gdt_boot_descr(%rip)
> +
> +        /* Reload code selector. */
> +        ljmpl   *cs32_switch_addr(%rip)

This would be cleaner and shorter as

push $BOOT_CS32
push $cs32_switch
lretq

> +
> +        .code32
> +
> +cs32_switch:
> +        /* Initialize basic data segments. */
> +        mov     $BOOT_DS,%edx
> +        mov     %edx,%ds
> +        mov     %edx,%es
> +        mov     %edx,%ss
> +        /* %esp is initialized later. */
> +
> +        /* Load null descriptor to unused segment registers. */
> +        xor     %edx,%edx
> +        mov     %edx,%fs
> +        mov     %edx,%gs
> +
> +        /* Disable paging. */
> +        mov     %cr0,%edx
> +        and     $(~X86_CR0_PG),%edx
> +        mov     %edx,%cr0
> +
> +        /* Jump to earlier loaded address. */
> +        jmp     *%edi
> +
>  __start:
>          cld
>          cli
> @@ -157,7 +362,7 @@ __start:
>  
>          /* Not available? BDA value will be fine. */
>          cmovnz  MB_mem_lower(%ebx),%edx
> -        jmp     trampoline_setup
> +        jmp     trampoline_bios_setup
>  
>  .Lmultiboot2_proto:
>          /* Skip Multiboot2 information fixed part. */
> @@ -169,24 +374,33 @@ __start:
>          mov     %ecx,%edi
>          sub     %ebx,%edi
>          cmp     %edi,MB2_fixed_total_size(%ebx)
> -        jbe     trampoline_setup
> +        jbe     trampoline_bios_setup
>  
>          /* Get mem_lower from Multiboot2 information. */
>          cmpl    $MULTIBOOT2_TAG_TYPE_BASIC_MEMINFO,MB2_tag_type(%ecx)
>          cmove   MB2_mem_lower(%ecx),%edx
> -        je      trampoline_setup
> +        je      .Lmb2_next_tag
> +
> +        /* EFI IA-32 platforms are not supported. */
> +        cmpl    $MULTIBOOT2_TAG_TYPE_EFI32,MB2_tag_type(%ecx)
> +        je      .Lmb2_efi_ia_32
> +
> +        /* Bootloader shutdown EFI x64 boot services. */
> +        cmpl    $MULTIBOOT2_TAG_TYPE_EFI64,MB2_tag_type(%ecx)
> +        je      .Lmb2_no_bs
>  
>          /* Is it the end of Multiboot2 information? */
>          cmpl    $MULTIBOOT2_TAG_TYPE_END,MB2_tag_type(%ecx)
> -        je      trampoline_setup
> +        je      trampoline_bios_setup
>  
> +.Lmb2_next_tag:
>          /* Go to next Multiboot2 information tag. */
>          add     MB2_tag_size(%ecx),%ecx
>          add     $(MULTIBOOT2_TAG_ALIGN-1),%ecx
>          and     $~(MULTIBOOT2_TAG_ALIGN-1),%ecx
>          jmp     .Lmb2_tsize
>  
> -trampoline_setup:
> +trampoline_bios_setup:
>          /* Set up trampoline segment 64k below EBDA */
>          movzwl  0x40e,%ecx          /* EBDA segment */
>          cmp     $0xa000,%ecx        /* sanity check (high) */
> @@ -202,16 +416,19 @@ trampoline_setup:
>           * multiboot structure (if available) and use the smallest.
>           */
>          cmp     $0x100,%edx         /* is the multiboot value too small? */
> -        jb      2f                  /* if so, do not use it */
> +        jb      trampoline_setup    /* if so, do not use it */
>          shl     $10-4,%edx
>          cmp     %ecx,%edx           /* compare with BDA value */
>          cmovb   %edx,%ecx           /* and use the smaller */
>  
> -2:      /* Reserve 64kb for the trampoline */
> +        /* Reserve 64kb for the trampoline. */
>          sub     $0x1000,%ecx
>  
>          /* From arch/x86/smpboot.c: start_eip had better be page-aligned! */
>          xor     %cl, %cl
> +
> +trampoline_setup:
> +        /* Save trampoline address for later use. */
>          shl     $4, %ecx
>          mov     %ecx,sym_phys(trampoline_phys)
>  
> @@ -223,7 +440,14 @@ trampoline_setup:
>          call    reloc
>          mov     %eax,sym_phys(multiboot_ptr)
>  
> -        /* Initialize BSS (no nasty surprises!) */
> +        /*
> +         * Do not zero BSS on EFI platform here.
> +         * It was initialized earlier.
> +         */
> +        cmpb    $0,sym_phys(skip_realmode)
> +        jnz     1f

This should be moved earlier in the BIOS case, rather than inserting
jump.  It can go ahead of even the multiboot check, immediately after
loading the gdt.

> +
> +        /* Initialize BSS (no nasty surprises!). */
>          mov     $sym_phys(__bss_start),%edi
>          mov     $sym_phys(__bss_end),%ecx
>          sub     %edi,%ecx
> @@ -231,6 +455,7 @@ trampoline_setup:
>          shr     $2,%ecx
>          rep stosl
>  
> +1:
>          /* Interrogate CPU extended features via CPUID. */
>          mov     $0x80000000,%eax
>          cpuid
> @@ -282,8 +507,13 @@ trampoline_setup:
>          cmp     $sym_phys(__trampoline_seg_stop),%edi
>          jb      1b
>  
> +        /* Do not parse command line on EFI platform here. */
> +        cmpb    $0,sym_phys(skip_realmode)

Please don't reuse skip_realmode for something unrelated.  For the sake
of one extra byte of data, its not worth the confusion when reading the
code.

~Andrew

> +        jnz     1f
> +
>          call    cmdline_parse_early
>  
> +1:
>          /* Switch to low-memory stack.  */
>          mov     sym_phys(trampoline_phys),%edi
>          lea     0x10000(%edi),%esp
>


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