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

[Daniel Kiper <daniel.kiper@xxxxxxxxxx>]

On Thu, Aug 25, 2016 at 06:59:54AM -0600, Jan Beulich wrote:
> >>> 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

I hope that ".align 4" is sufficient here.

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

OK.

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

Yep.

[...]

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

efi_multiboot2() is called according to System V AMD64 ABI. So, according to it
return value is stored in %rax. Hence, from spec point of view comment is 
correct.
Use of %eax is a bit different thing (our need/choice) and if you wish it can be
documented accordingly.

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

I think that shr should be close to sub because both operations are related
to some extent. However, you are right that maybe this cleanup should be in
separate patch. Does it make sense for you?

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

You are right. This is a bug. However, the problem is more generic
and should be fixed for all boot cases (BIOS, EFI loader and EFI with
GRUB2). Additionally, in case of BIOS and EFI with GRUB2 we should
check that it is possible to put all data from grub loader in low
memory. So, it looks that there is a place for at least two (three?)
additional patches. Do you want them at the beginning of or the end
of this patch series.

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

I am not convinced but if you wish...

> that it open codes an MS-ABI function call. Which then makes me

OK.

> 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

Nope, MS-ABI says that %rsi and %rdi must be saved and restored by
function which uses them. So, we do not need list both registers
in clobbers here.

> need for an explicit "cc" clobber on x86.

Why?

Daniel

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