Re: [Xen-devel] [PATCH v2 20/23] x86: add multiboot2 protocol support for EFI platforms

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Mon, Jul 20, 2015 at 04:29:15PM +0200, Daniel Kiper wrote:
> Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
> ---
> 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          |  157 +++++++++++++++++++++++++++++++++++--
>  xen/arch/x86/efi/efi-boot.h       |   30 +++++++
>  xen/arch/x86/efi/stub.c           |    5 ++
>  xen/arch/x86/setup.c              |   10 ++-
>  xen/arch/x86/x86_64/asm-offsets.c |    2 +
>  xen/arch/x86/xen.lds.S            |    4 +-
>  xen/common/efi/boot.c             |   12 +++
>  xen/include/xen/efi.h             |    1 +
>  8 files changed, 210 insertions(+), 11 deletions(-)
> 
> diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
> index 57197db..056047f 100644
> --- a/xen/arch/x86/boot/head.S
> +++ b/xen/arch/x86/boot/head.S
> @@ -89,6 +89,13 @@ multiboot1_header_end:
>                     0, /* Number of the lines - no preference. */ \
>                     0  /* Number of bits per pixel - no preference. */
>  
> +        /* Do not disable EFI boot services. */

s/disable/exit/ ?

Or perhaps:

/* Inhibit GRUB2 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,9 +107,15 @@ multiboot1_header_end:
>  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
>  
>  .Lbad_cpu_msg: .asciz "ERR: Not a 64-bit CPU!"
>  .Lbad_ldr_msg: .asciz "ERR: Not a Multiboot bootloader!"
> +.Lbad_mb2_ldr: .asciz "ERR: Use latest Multiboot2 compatible bootloader!"

Multiboot2+EFI compatible..?
>  
>          .section .init.text, "ax", @progbits
>  
> @@ -111,6 +124,9 @@ bad_cpu:
>          jmp     print_err
>  not_multiboot:
>          mov     $(sym_phys(.Lbad_ldr_msg)),%esi # Error message
> +        jmp     print_err
> +mb2_too_old:
> +        mov     $(sym_phys(.Lbad_mb2_ldr)),%esi # Error message
>  print_err:
>          mov     $0xB8000,%edi  # VGA framebuffer
>  1:      mov     (%esi),%bl
> @@ -130,6 +146,119 @@ print_err:
>  .Lhalt: hlt
>          jmp     .Lhalt
>  
> +        .code64
> +
> +__efi64_start:
> +        cld
> +
> +        /* Check for Multiboot2 bootloader. */
> +        cmp     $MULTIBOOT2_BOOTLOADER_MAGIC,%eax
> +        je      efi_multiboot2_proto
> +
> +        /* Jump to not_multiboot after switching CPU to x86_32 mode. */
> +        lea     not_multiboot(%rip),%rdi
> +        jmp     x86_32_switch
> +
> +efi_multiboot2_proto:
> +        /*
> +         * Multiboot2 information address is 32-bit,
> +         * so, zero higher half of %rbx.
> +         */
> +        mov     %ebx,%ebx
> +
> +        /* Skip Multiboot2 information fixed part. */
> +        lea     MB2_fixed_sizeof(%rbx),%rcx

Don't want to check that the address is aligned properly?

> +
> +0:
> +        /* Get EFI SystemTable address from Multiboot2 information. */
> +        cmpl    $MULTIBOOT2_TAG_TYPE_EFI64,MB2_tag_type(%rcx)
> +        jne     1f
> +
> +        mov     MB2_efi64_st(%rcx),%rsi
> +
> +        /* Do not go into real mode on EFI platform. */

Add please:

/* And also inhibit clearing of BSS later on. */

> +        movb    $1,skip_realmode(%rip)
> +        jmp     3f
> +
> +1:
> +        /* Get EFI ImageHandle address from Multiboot2 information. */
> +        cmpl    $MULTIBOOT2_TAG_TYPE_EFI64_IH,MB2_tag_type(%rcx)
> +        jne     2f
> +
> +        mov     MB2_efi64_ih(%rcx),%rdi
> +        jmp     3f
> +
> +2:
> +        /* Is it the end of Multiboot2 information? */
> +        cmpl    $MULTIBOOT2_TAG_TYPE_END,MB2_tag_type(%rcx)
> +        je      run_bs
> +
> +3:
> +        /* Go to next Multiboot2 information tag. */
> +        add     MB2_tag_size(%rcx),%ecx
> +        add     $(MULTIBOOT2_TAG_ALIGN-1),%rcx
> +        and     $~(MULTIBOOT2_TAG_ALIGN-1),%rcx
> +        jmp     0b
> +
> +run_bs:
> +        push    %rax
> +        push    %rdi
> +
> +        /* Initialize BSS (no nasty surprises!). */
> +        lea     __bss_start(%rip),%rdi
> +        lea     __bss_end(%rip),%rcx
> +        sub     %rdi,%rcx
> +        shr     $3,%rcx
> +        xor     %eax,%eax
> +        rep     stosq
> +
> +        pop     %rdi
> +
> +        /*
> +         * IN: %rdi - EFI ImageHandle, %rsi - EFI SystemTable.
> +         * OUT: %rax - multiboot2.mem_lower. Do not get this value from
> +         * MULTIBOOT2_TAG_TYPE_BASIC_MEMINFO tag. It could be bogus on
> +         * EFI platforms.
> +         */
> +        call    efi_multiboot2
> +
> +        /* Convert multiboot2.mem_lower to bytes/16. */
> +        mov     %rax,%rcx
> +        shr     $4,%rcx
> +
> +        pop     %rax
> +
> +        /* Jump to trampoline_setup after switching CPU to x86_32 mode. */
> +        lea     trampoline_setup(%rip),%rdi
> +
> +x86_32_switch:
> +        cli
> +
> +        /* Initialise GDT. */
> +        lgdt    gdt_boot_descr(%rip)
> +
> +        /* Reload code selector. */
> +        ljmpl   *cs32_switch_addr(%rip)
> +
> +        .code32
> +
> +cs32_switch:
> +        /* Initialise basic data segments. */
> +        mov     $BOOT_DS,%edx
> +        mov     %edx,%ds
> +        mov     %edx,%es
> +        mov     %edx,%fs
> +        mov     %edx,%gs
> +        mov     %edx,%ss
> +
> +        /* 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 +286,7 @@ __start:
>  
>          /* Not available? BDA value will be fine. */
>          cmovnz  MB_mem_lower(%ebx),%edx
> -        jmp     trampoline_setup
> +        jmp     trampoline_bios_setup
>  
>  multiboot2_proto:
>          /* Skip Multiboot2 information fixed part. */
> @@ -169,12 +298,19 @@ multiboot2_proto:
>          jne     1f
>  
>          mov     MB2_mem_lower(%ecx),%edx
> -        jmp     trampoline_setup
> +        jmp     trampoline_bios_setup
>  
>  1:
> +        /* EFI mode is not supported via legacy BIOS path. */
> +        cmpl    $MULTIBOOT2_TAG_TYPE_EFI32,MB2_tag_type(%ecx)
> +        je      mb2_too_old
> +
> +        cmpl    $MULTIBOOT2_TAG_TYPE_EFI64,MB2_tag_type(%ecx)
> +        je      mb2_too_old
> +
>          /* Is it the end of Multiboot2 information? */
>          cmpl    $MULTIBOOT2_TAG_TYPE_END,MB2_tag_type(%ecx)
> -        je      trampoline_setup
> +        je      trampoline_bios_setup
>  
>          /* Go to next Multiboot2 information tag. */
>          add     MB2_tag_size(%ecx),%ecx
> @@ -182,7 +318,7 @@ multiboot2_proto:
>          and     $~(MULTIBOOT2_TAG_ALIGN-1),%ecx
>          jmp     0b
>  
> -trampoline_setup:
> +trampoline_bios_setup:
>          /* Set up trampoline segment 64k below EBDA */
>          movzwl  0x40e,%ecx          /* EBDA segment */
>          cmp     $0xa000,%ecx        /* sanity check (high) */
> @@ -198,12 +334,13 @@ 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 */
> +trampoline_setup:
> +        /* Reserve 64kb for the trampoline. */
>          sub     $0x1000,%ecx
>  
>          /* From arch/x86/smpboot.c: start_eip had better be page-aligned! */
> @@ -220,6 +357,13 @@ trampoline_setup:
>          add     $12,%esp            /* Remove reloc() args from stack. */
>          mov     %eax,sym_phys(multiboot_ptr)
>  
> +        /*
> +         * Do not zero BSS on EFI platform here.
> +         * It was initialized earlier.
> +         */
> +        cmpb    $1,sym_phys(skip_realmode)
> +        je      1f
> +
>          /* Initialize BSS (no nasty surprises!). */
>          mov     $sym_phys(__bss_start),%edi
>          mov     $sym_phys(__bss_end),%ecx
> @@ -228,6 +372,7 @@ trampoline_setup:
>          xor     %eax,%eax
>          rep     stosl
>  
> +1:
>          /* Interrogate CPU extended features via CPUID. */
>          mov     $0x80000000,%eax
>          cpuid
> diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
> index 3d25c48..1b25a2d 100644
> --- a/xen/arch/x86/efi/efi-boot.h
> +++ b/xen/arch/x86/efi/efi-boot.h
> @@ -228,6 +228,9 @@ static void *__init efi_arch_allocate_mmap_buffer(UINTN map_size)
>  
>  static void __init efi_arch_pre_exit_boot(void)
>  {
> +    if ( !efi_enabled(EFI_LOADER) )
> +        return;
> +
>      if ( !trampoline_phys )
>      {
>          if ( !cfg.addr )
> @@ -662,6 +665,33 @@ static bool_t __init efi_arch_use_config_file(EFI_SYSTEM_TABLE *SystemTable)
>      return 1; /* x86 always uses a config file */
>  }
>  
> +paddr_t __init efi_multiboot2(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
> +{
> +    EFI_GRAPHICS_OUTPUT_PROTOCOL *gop;
> +    UINTN cols, gop_mode = ~0, rows;
> +
> +    set_bit(EFI_PLATFORM, &efi.flags);
> +
> +    efi_init(ImageHandle, SystemTable);
> +
> +    efi_console_set_mode();
> +
> +    if ( StdOut->QueryMode(StdOut, StdOut->Mode->Mode,
> +                           &cols, &rows) == EFI_SUCCESS )
> +        efi_arch_console_init(cols, rows);
> +
> +    gop = efi_get_gop();
> +    gop_mode = efi_find_gop_mode(gop, 0, 0, 0);
> +    efi_arch_edd();
> +    efi_tables();
> +    setup_efi_pci();
> +    efi_variables();
> +    efi_set_gop_mode(gop, gop_mode);
> +    efi_exit_boot(ImageHandle, SystemTable);
> +
> +    return cfg.addr;
> +}
> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/xen/arch/x86/efi/stub.c b/xen/arch/x86/efi/stub.c
> index c5ae369..d30fe89 100644
> --- a/xen/arch/x86/efi/stub.c
> +++ b/xen/arch/x86/efi/stub.c
> @@ -13,6 +13,11 @@ struct efi __read_mostly efi = {
>  	.smbios3 = EFI_INVALID_TABLE_ADDR
>  };
>  
> +void __init efi_multiboot2(void)
> +{
> +    /* TODO: Fail if entered! */

Heheh. Could we add the 'ud2' instruction here?

> +}
> +
>  void __init efi_init_memory(void) { }
>  
>  void efi_update_l4_pgtable(unsigned int l4idx, l4_pgentry_t l4e) { }
> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index a59fc4e..8bec67f 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -695,7 +695,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>      if ( !(mbi->flags & MBI_MODULES) || (mbi->mods_count == 0) )
>          panic("dom0 kernel not specified. Check bootloader configuration.");
>  
> -    if ( efi_enabled(EFI_PLATFORM) )
> +    if ( efi_enabled(EFI_LOADER) )
>      {
>          set_pdx_range(xen_phys_start >> PAGE_SHIFT,
>                        (xen_phys_start + BOOTSTRAP_MAP_BASE) >> PAGE_SHIFT);
> @@ -708,7 +708,11 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>          l3_bootmap[l3_table_offset(BOOTSTRAP_MAP_BASE)] =
>              l3e_from_paddr(__pa(l2_bootmap), __PAGE_HYPERVISOR);
>  
> -        memmap_type = loader;
> +        memmap_type = "EFI";
> +    }
> +    else if ( efi_enabled(EFI_PLATFORM) )
> +    {
> +        memmap_type = "EFI";
>      }
>      else if ( e820_raw_nr != 0 )
>      {
> @@ -806,7 +810,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>       * we can relocate the dom0 kernel and other multiboot modules. Also, on
>       * x86/64, we relocate Xen to higher memory.
>       */
> -    for ( i = 0; !efi_enabled(EFI_PLATFORM) && i < mbi->mods_count; i++ )
> +    for ( i = 0; !efi_enabled(EFI_LOADER) && i < mbi->mods_count; i++ )
>      {
>          if ( mod[i].mod_start & (PAGE_SIZE - 1) )
>              panic("Bootloader didn't honor module alignment request.");
> diff --git a/xen/arch/x86/x86_64/asm-offsets.c b/xen/arch/x86/x86_64/asm-offsets.c
> index b926082..b7aed49 100644
> --- a/xen/arch/x86/x86_64/asm-offsets.c
> +++ b/xen/arch/x86/x86_64/asm-offsets.c
> @@ -173,4 +173,6 @@ void __dummy__(void)
>      OFFSET(MB2_tag_type, multiboot2_tag_t, type);
>      OFFSET(MB2_tag_size, multiboot2_tag_t, size);
>      OFFSET(MB2_mem_lower, multiboot2_tag_basic_meminfo_t, mem_lower);
> +    OFFSET(MB2_efi64_st, multiboot2_tag_efi64_t, pointer);
> +    OFFSET(MB2_efi64_ih, multiboot2_tag_efi64_ih_t, pointer);
>  }
> diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
> index 87f3e83..a399615 100644
> --- a/xen/arch/x86/xen.lds.S
> +++ b/xen/arch/x86/xen.lds.S
> @@ -162,7 +162,7 @@ SECTIONS
>    . = ALIGN(STACK_SIZE);
>    __init_end = .;
>  
> -  . = ALIGN(4);
> +  . = ALIGN(8);
>    .bss : {                     /* BSS */
>         __bss_start = .;
>         *(.bss.stack_aligned)
> @@ -176,7 +176,7 @@ SECTIONS
>         *(.bss.percpu.read_mostly)
>         . = ALIGN(SMP_CACHE_BYTES);
>         __per_cpu_data_end = .;
> -       . = ALIGN(4);
> +       . = ALIGN(8);

This aligment check I presume is due to 'rep stosq'?

As such could you add a comment here please?


>         __bss_end = .;
>    } :text
>    _end = . ;
> diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
> index bf2f198..5a02e71 100644
> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -79,6 +79,17 @@ static size_t wstrlen(const CHAR16 * s);
>  static int set_color(u32 mask, int bpp, u8 *pos, u8 *sz);
>  static bool_t match_guid(const EFI_GUID *guid1, const EFI_GUID *guid2);
>  
> +static void efi_init(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable);
> +static void efi_console_set_mode(void);
> +static EFI_GRAPHICS_OUTPUT_PROTOCOL *efi_get_gop(void);
> +static UINTN efi_find_gop_mode(EFI_GRAPHICS_OUTPUT_PROTOCOL *gop,
> +                               UINTN cols, UINTN rows, UINTN depth);
> +static void efi_tables(void);
> +static void setup_efi_pci(void);
> +static void efi_variables(void);
> +static void efi_set_gop_mode(EFI_GRAPHICS_OUTPUT_PROTOCOL *gop, UINTN gop_mode);
> +static void efi_exit_boot(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable);
> +
>  static const EFI_BOOT_SERVICES *__initdata efi_bs;
>  static UINT32 __initdata efi_bs_revision;
>  static EFI_HANDLE __initdata efi_ih;
> @@ -960,6 +971,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
>  
>  #ifndef CONFIG_ARM /* Disabled until runtime services implemented. */
>      set_bit(EFI_PLATFORM, &efi.flags);
> +    set_bit(EFI_LOADER, &efi.flags);
>  #endif
>  
>      efi_init(ImageHandle, SystemTable);
> diff --git a/xen/include/xen/efi.h b/xen/include/xen/efi.h
> index 318bbec..6b952df 100644
> --- a/xen/include/xen/efi.h
> +++ b/xen/include/xen/efi.h
> @@ -9,6 +9,7 @@
>  #define EFI_INVALID_TABLE_ADDR (~0UL)
>  
>  #define EFI_PLATFORM	0
> +#define EFI_LOADER	1
>  
>  /* Add fields here only if they need to be referenced from non-EFI code. */
>  struct efi {


With the changes above:

Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> -- 
> 1.7.10.4
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel