Re: [PATCH v3 13/16 - RFC] x86: add multiboot2 protocol support for EFI platforms

["Jan Beulich" <JBeulich@suse.com>]

>>> On 15.04.16 at 14:33, <daniel.kiper@oracle.com> wrote:
> @@ -100,19 +107,29 @@ multiboot2_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_ldr_mb2: .asciz "ERR: On EFI platform use latest Multiboot2 compatible bootloader!"

What is "latest" going to mean 5 years from now?

>          .section .init.text, "ax", @progbits
>  
>  bad_cpu:
>          mov     $(sym_phys(.Lbad_cpu_msg)),%esi # Error message
> -        jmp     print_err
> +        mov     $0xB8000,%edi                   # VGA framebuffer
> +        jmp     1f
>  not_multiboot:
>          mov     $(sym_phys(.Lbad_ldr_msg)),%esi # Error message
> -print_err:
> -        mov     $0xB8000,%edi  # VGA framebuffer
> +        mov     $0xB8000,%edi                   # VGA framebuffer
> +        jmp     1f
> +mb2_too_old:
> +        mov     $(sym_phys(.Lbad_ldr_mb2)),%esi # Error message
> +        xor     %edi,%edi                       # No VGA framebuffer

Leaving aside that "framebuffer" really is a bad term here (we're
talking of text mode output after all), limiting the output to serial
isn't going to be very helpful in the field, I'm afraid. Even more so
that there's no guarantee for a UART to be at port 0x3f8. That's
not much of a problem for the other two messages as people are
unlikely to try to boot Xen on an unsuitable system, but I view it
as quite possible for Xen to be tried to get booted with an
unsuitable grub2.

IOW - this needs a better solution, presumably using EFI boot
service output functions.

> @@ -130,6 +149,130 @@ print_err:
>  .Lhalt: hlt
>          jmp     .Lhalt
>  
> +        .code64
> +
> +__efi64_start:

As long as we have split files under boot/, I think I'd prefer 64-bit
code to only go into x86_64.S.

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

What I've said above would also eliminate the need to switch to
32-bit mode just for emitting an error message and halting the
system.

> +efi_multiboot2_proto:

.Lefi_multiboot2_proto

> +        /*
> +         * Multiboot2 information address is 32-bit,
> +         * so, zero higher half of %rbx.
> +         */
> +        mov     %ebx,%ebx

Wait, no - that's a protocol bug then. We're being entered in 64-bit
mode here, so registers should be in 64-bit clean state.

> +        /* Skip Multiboot2 information fixed part. */
> +        lea     (MB2_fixed_sizeof+MULTIBOOT2_TAG_ALIGN-1)(%rbx),%rcx
> +        and     $~(MULTIBOOT2_TAG_ALIGN-1),%rcx

Or if there really was a reason to do the above, and if there is a
reason not to assume this data is located below 4Gb, then
calculations like this could avoid the REX64 prefix by using %ecx.

> +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 clear BSS twice and do not go into real mode. */
> +        movb    $1,skip_realmode(%rip)

How is the setting of skip_realmode related to the clearing of BSS?
Oh, I've found the connection below (albeit see there for its
validity), but I think mentioning this here is more confusing than
clarifying.

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

At least down to here it looks like this would better live in a C helper
function. Did you consider this?

> +run_bs:
> +        push    %rax
> +        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),%rdi
> +        lea     __bss_end(%rip),%rcx
> +        sub     %rdi,%rcx
> +        shr     $3,%rcx
> +        xor     %eax,%eax
> +        rep     stosq

Please let's not repeat pre-existing mistakes: REP is not an
instruction, and STOSB is not an operand. IOW there should be
just a single space between the two.

> +        pop     %rdi
> +
> +        /*
> +         * IN: %rdi - EFI ImageHandle, %rsi - EFI SystemTable.
> +         * OUT: %rax - Highest available memory address below 1 MiB.
> +         *
> +         * 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,%rax
> +        mov     %rax,%rcx

Again, considering that the starting value in %rax here is an
address below 1Mb, no need to do the calculations on 64-bit
registers.

> +        pop     %rax
> +
> +        /* Jump to trampoline_setup after switching CPU to x86_32 mode. */
> +        lea     trampoline_setup(%rip),%rdi

Same here - the branch below uses %edi only anyway.

> @@ -170,12 +313,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

According to the comment we're on the legacy BIOS boot path
here, yet at mb2_too_old you assume no text mode VGA. I.e. I'm
now even confused about the output handling there.

> @@ -221,6 +372,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

So what if skip_realmode is set on a legacy BIOS system because
of the command line option or the use of TBOOT?

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

Please connect the two if()-s - afaiu you really only care about the
trampoline handling to not be done. Otherwise the new conditional
would probably rather belong on the (arch-independent) call site.

> +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);

__set_bit()

> +    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();
> +
> +    if ( gop )
> +        gop_mode = efi_find_gop_mode(gop, 0, 0, 0);
> +
> +    efi_arch_edd();
> +
> +    /*
> +     * efi_arch_cpu() is not needed here. boot_cpu_data
> +     * is set later in xen/arch/x86/boot/head.S.
> +     */

That's not a good explanation. Is there any harm in calling it? If not,
I'd suggest calling it here just to avoid missing a dependency on what
it does in any of the functions called subsequently. If there is, the
precise details of that is what you should say here.

> +    efi_tables();
> +    setup_efi_pci();
> +    efi_variables();
> +
> +    if ( gop )
> +        efi_set_gop_mode(gop, gop_mode);
> +
> +    efi_exit_boot(ImageHandle, SystemTable);
> +
> +    /* Return highest available memory address below 1 MiB. */
> +    return cfg.addr;

Didn't you say on some systems all of the memory below 640k is in
use by boot/loader code/data? If so, what meaning has the value
you return here (I don't recall the consumer side special casing any
error value)?

> --- 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! */
> +}

Why not just BUG()? What exactly you do here doesn't seem to
matter, as the symbol is unreachable in this case anyway (you
only need it to please the linker).

> @@ -728,7 +728,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";
>      }

Stray braces.

> --- a/xen/arch/x86/xen.lds.S
> +++ b/xen/arch/x86/xen.lds.S
> @@ -192,7 +192,7 @@ SECTIONS
>    } :text
>  
>    /* Align BSS to speedup its initialization. */
> -  . = ALIGN(4);
> +  . = ALIGN(8);
>    .bss : {                     /* BSS */
>         . = ALIGN(STACK_SIZE);
>         __bss_start = .;
> @@ -207,7 +207,7 @@ SECTIONS
>         *(.bss.percpu.read_mostly)
>         . = ALIGN(SMP_CACHE_BYTES);
>         __per_cpu_data_end = .;
> -       . = ALIGN(4);
> +       . = ALIGN(8);
>         __bss_end = .;

Is that really worth it? I.e. is going from STOSD to STOSQ really a
meaningful win?

> @@ -936,6 +947,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

So _neither_ of the two bits get set for ARM? I'm even more puzzled
now, and hence think even more that this can't be fine grained enough.

Jan

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