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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 20.08.16 at 00:43, <daniel.kiper@xxxxxxxxxx> wrote:
> @@ -100,19 +107,45 @@ 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
> +
> +vga_text_buffer:
> +        .long   0xb8000

This now ends up being misaligned. Not a big deal, but anyway.

>          .section .init.text, "ax", @progbits
>  
>  bad_cpu:
>          mov     $(sym_phys(.Lbad_cpu_msg)),%esi # Error message
> -        jmp     print_err
> +        jmp     0f
>  not_multiboot:
>          mov     $(sym_phys(.Lbad_ldr_msg)),%esi # Error message
> -print_err:
> -        mov     $0xB8000,%edi  # VGA framebuffer
> +        jmp     0f
> +mb2_no_st:
> +        mov     $(sym_phys(.Lbad_ldr_nst)),%esi # Error message
> +        jmp     0f
> +mb2_no_ih:
> +        mov     $(sym_phys(.Lbad_ldr_nih)),%esi # Error message
> +        jmp     0f
> +mb2_no_bs:
> +        mov     $(sym_phys(.Lbad_ldr_nbs)),%esi # Error message
> +        xor     %edi,%edi                       # No VGA text buffer
> +        jmp     1f
> +mb2_efi_ia_32:
> +        mov     $(sym_phys(.Lbad_efi_msg)),%esi # Error message
> +        xor     %edi,%edi                       # No VGA text buffer
> +        jmp     1f
> +0:      mov     sym_phys(vga_text_buffer),%edi
>  1:      mov     (%esi),%bl

All the labels you add should imo be .L ones. And the two numeric
labels, considering their use across other label boundaries, would
perhaps better become .L ones too.

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

Does the boot loader, btw, make any guarantees towards placing
the image below 4G?

> +.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)(%ebx),%ecx

As indicated before - please use %rbx here; there's no need for
having a pointless address size prefix.

> +        and     $~(MULTIBOOT2_TAG_ALIGN-1),%ecx
> +
> +0:
> +        /* 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     1f
> +
> +        /* Yes, skip real mode and do not do other unneeded things. */
> +        incb    skip_realmode(%rip)
> +        jmp     9f

I think this comment need extending: Why again is not skipping
real mode fine if there's no such tag, no matter that we got called
in 64-bit mode here? Actually, looking further down, wasn't it that
you simply abuse that label (you jump to an error label below when
it is still zero)? That needs to be said here if so, for someone
reading the code in order to understand that there's no actual use
of real mode anywhere on this path.

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

In assembly code please use .startof.(.bss) and .sizeof.(.bss), as
being propose for other uses by
https://lists.xenproject.org/archives/html/xen-devel/2016-08/msg02718.html

> +        /*
> +         * IN: %rdi - EFI ImageHandle, %rsi - EFI SystemTable.
> +         * OUT: %rax - Highest available memory address below 1 MiB.

Please be more precise, as "available" leaves open how much space
is actually available there, and hence what this can be used for. Also
according to ...

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

... the output really is in %eax.

> @@ -223,14 +425,22 @@ 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
> +
> +        /* Initialize BSS (no nasty surprises!). */
>          mov     $sym_phys(__bss_start),%edi
>          mov     $sym_phys(__bss_end),%ecx
>          sub     %edi,%ecx
> -        xor     %eax,%eax
>          shr     $2,%ecx
> +        xor     %eax,%eax
>          rep stosl

Please avoid pointless code movement like this.

> +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_BOOT, &efi_flags);
> +    __set_bit(EFI_RS, &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();
> +
> +    if ( gop )
> +        gop_mode = efi_find_gop_mode(gop, 0, 0, 0);
> +
> +    efi_arch_edd();
> +    efi_arch_cpu();
> +
> +    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;

Where is it being made certain that there are 64k of space available
right below this address, as is being assumed at trampoline_setup?

> --- a/xen/arch/x86/efi/stub.c
> +++ b/xen/arch/x86/efi/stub.c
> @@ -3,6 +3,30 @@
>  #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>
> +
> +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";
> +    SIMPLE_TEXT_OUTPUT_INTERFACE *StdErr;
> +
> +    StdErr = SystemTable->StdErr ? SystemTable->StdErr : SystemTable->ConOut;
> +
> +    /* Print error message and halt the system. */
> +    asm volatile(
> +    "    call %2                      \n"
> +    "0:  hlt                          \n"
> +    "    jmp  0b                      \n"
> +       : "+c" (StdErr), "+d" (err) : "g" (StdErr->OutputString)
> +       : "rax", "r8", "r9", "r10", "r11", "cc", "memory");

There are explanations missing here: First, a warning should be added
alongside the EFI header inclusions, making clear that no services
whatsoever can be called. And then the asm() here needs to explain
that it open codes an MS-ABI function call. Which then makes me
wonder (even if it doesn't matter much) - are the clobbers actually
correct? I think you also need to clobber rsi and rdi. Otoh there's no
need for an explicit "cc" clobber on x86.

Jan

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