Re: [Xen-devel] [PATCH v12 05/10] x86: add multiboot2 protocol support for EFI platforms

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 20.01.17 at 02:34, <daniel.kiper@xxxxxxxxxx> wrote:
> @@ -100,20 +107,48 @@ multiboot2_header_start:
>  gdt_boot_descr:
>          .word   6*8-1
>          .long   sym_phys(trampoline_gdt)
> +        .long   0 /* Needed for 64-bit lgdt */
> +
> +        .align 4
> +vga_text_buffer:
> +        .long   0xb8000
> +
> +efi_platform:
> +        .byte   0

You mean to modify these fields, but you add them to a r/o section.

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

In an earlier reply to Andrew's inquiry regarding the possible
truncation here you said that grub can be made obey to such a
load restriction. None of the tags added here or in patch 2
appear to have such an effect, so would you please clarify how
the address restriction is being enforced?

> +        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
> +
> +        /* We are on EFI platform and EFI boot services are available. */
> +        incb    efi_platform(%rip)
> +
> +        /*
> +         * Disable real mode and other legacy stuff which should not
> +         * be run on EFI platforms.
> +         */
> +        incb    skip_realmode(%rip)
> +        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:

.Lrun_bs:

> +        /* Are EFI boot services available? */
> +        cmpb    $0,efi_platform(%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:

I realize you need to avoid clobbering %rdi in the non-error case,
but the register choice seems sub-optimal: If you used a register
which you can clobber here (e.g. %edx as it seems, using %edi in
its place at x86_32_switch and cs32_switch), you could simplify
this to

    cmpb ...
    lea ...
    je x86_32_switch

at once avoiding all the numeric labels here.

> +2:
> +        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),%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 allocated memory address below 1 MiB;
> +         *                 memory below this address is used for trampoline
> +         *                 stack, trampoline itself and 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

Now that we had a need for commit f6b7fedc89 ("x86/EFI: meet
further spec requirements for runtime calls") I'm worried about stack
alignment here. Does GrUB call or jmp to our entry point (and is that
firmly specified to be the case)? Perhaps it would be a good idea to
align the stack earlier on in any case. Or if not (and if alignment at
this call is ABI conforming), a comment should be left here to warn
people of future modifications to the amount of items pushed onto /
popped off the stack.

> +trampoline_setup:
> +        /* Save trampoline address for later use. */
>          shl     $4, %ecx
>          mov     %ecx,sym_phys(trampoline_phys)

I don't think this comment is very useful.

> --- a/xen/arch/x86/efi/efi-boot.h
> +++ b/xen/arch/x86/efi/efi-boot.h
> @@ -100,6 +100,9 @@ static void __init relocate_trampoline(unsigned long phys)
>  {
>      const s32 *trampoline_ptr;
>  
> +    if ( !efi_enabled(EFI_LOADER) || trampoline_phys )
> +        return;
> +
>      trampoline_phys = phys;
>      /* Apply relocations to trampoline. */
>      for ( trampoline_ptr = __trampoline_rel_start;
> @@ -210,12 +213,10 @@ static void *__init efi_arch_allocate_mmap_buffer(UINTN 
> map_size)
>  
>  static void __init efi_arch_pre_exit_boot(void)
>  {
> -    if ( !trampoline_phys )
> -    {
> -        if ( !cfg.addr )
> -            blexit(L"No memory for trampoline");
> -        relocate_trampoline(cfg.addr);
> -    }
> +    if ( !cfg.addr )
> +        blexit(L"No memory for trampoline");
> +
> +    relocate_trampoline(cfg.addr);
>  }

Why can't this function be left untouched, and the change to
relocate_trampoline() above also be reduced or even be removed
altogether?

> --- a/xen/arch/x86/efi/stub.c
> +++ b/xen/arch/x86/efi/stub.c
> @@ -3,6 +3,44 @@
>  #include <xen/init.h>
>  #include <xen/lib.h>
>  #include <asm/page.h>
> +#include <asm/efibind.h>
> +#include <efi/efidef.h>
> +#include <efi/eficapsule.h>
> +#include <efi/eficon.h>
> +#include <efi/efidevp.h>
> +#include <efi/efiapi.h>
> +
> +/*
> + * Here we are in EFI stub. EFI calls are not supported due to lack
> + * of relevant functionality in compiler and/or linker.
> + *
> + * efi_multiboot2() is an exception. Please look below for more details.
> + */
> +
> +paddr_t __init noreturn efi_multiboot2(EFI_HANDLE ImageHandle,
> +                                       EFI_SYSTEM_TABLE *SystemTable)
> +{
> +    CHAR16 *err = L"Xen does not have EFI code build in!!!\r\nSystem 
> halted!!!\r\n";

static const CHAR16 __initconst err[]

And please reduce the number of exclamation marks.

> +    SIMPLE_TEXT_OUTPUT_INTERFACE *StdErr;
> +
> +    StdErr = SystemTable->StdErr ? SystemTable->StdErr : SystemTable->ConOut;
> +
> +    /*
> +     * Print error message and halt the system.
> +     *
> +     * We have to open code MS x64 calling convention
> +     * in assembly because here this convention may
> +     * not be directly supported by C compiler.
> +     */
> +    asm volatile(
> +    "    call *%2                     \n"
> +    "0:  hlt                          \n"
> +    "    jmp  0b                      \n"
> +       : "+c" (StdErr), "+d" (err) : "g" (StdErr->OutputString)

The "g" really wants to be "rm": While the kind of expression doesn't
really allow for an immediate, you still shouldn't give wrong examples
of constraints (with the * now properly added to the call operand, it
doesn't allow for an immediate anymore).

> --- a/xen/include/asm-x86/config.h
> +++ b/xen/include/asm-x86/config.h
> @@ -73,6 +73,11 @@
>  #define STACK_ORDER 3
>  #define STACK_SIZE  (PAGE_SIZE << STACK_ORDER)
>  
> +#define MB_TRAMPOLINE_STACK_SIZE        PAGE_SIZE
> +#define MB_TRAMPOLINE_SIZE              (KB(64) - MB_TRAMPOLINE_STACK_SIZE)

What's the reason for the MB_ prefixes here? The trampoline is
always the same size, isn't it? Nor am I convinced we really need
two defines - except in the link time assertion you always use
the sum of the two.

> +#define MBI_SIZE                        (2 * PAGE_SIZE)

On top of Doug's question - if it is needed at all, what does this
match up with, i.e. how come this is 2 pages (and not 1 or 3)?

Jan

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