[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



On Fri, Jan 30, 2015 at 07:06:53PM +0000, Andrew Cooper wrote:
> On 30/01/15 17:54, Daniel Kiper wrote:
> > Signed-off-by: Daniel Kiper <daniel.kiper@xxxxxxxxxx>
> > ---
> >  xen/arch/x86/boot/head.S          |  174 
> > +++++++++++++++++++++++++++++++++++--
> >  xen/arch/x86/efi/efi-boot.h       |   29 +++++++
> >  xen/arch/x86/setup.c              |   23 ++---
> >  xen/arch/x86/x86_64/asm-offsets.c |    2 +
> >  xen/common/efi/boot.c             |   11 +++
> >  5 files changed, 222 insertions(+), 17 deletions(-)
> >
> > diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
> > index 7861057..89f5aa7 100644
> > --- a/xen/arch/x86/boot/head.S
> > +++ b/xen/arch/x86/boot/head.S
> > @@ -8,6 +8,7 @@
> >  #include <asm/page.h>
> >  #include <asm/msr.h>
> >  #include <asm/cpufeature.h>
> > +#include <asm/processor.h>
> >
> >          .text
> >          .code32
> > @@ -57,6 +58,9 @@ ENTRY(start)
> >          .long   .Lmultiboot2_info_req_end - .Lmultiboot2_info_req
> >          .long   MULTIBOOT2_TAG_TYPE_BASIC_MEMINFO
> >          .long   MULTIBOOT2_TAG_TYPE_MMAP
> > +        .long   MULTIBOOT2_TAG_TYPE_EFI_BS
> > +        .long   MULTIBOOT2_TAG_TYPE_EFI64
> > +        .long   MULTIBOOT2_TAG_TYPE_EFI64_IH
> >  .Lmultiboot2_info_req_end:
> >
> >          .align  MULTIBOOT2_TAG_ALIGN
> > @@ -80,6 +84,19 @@ ENTRY(start)
> >          .long   0 /* Number of the lines - no preference. */
> >          .long   0 /* Number of bits per pixel - no preference. */
> >
> > +        /* Do not disable EFI boot services. */
> > +        .align  MULTIBOOT2_TAG_ALIGN
> > +        .short  MULTIBOOT2_HEADER_TAG_EFI_BS
> > +        .short  MULTIBOOT2_HEADER_TAG_OPTIONAL
> > +        .long   8 /* Tag size. */
> > +
> > +        /* EFI64 entry point. */
> > +        .align  MULTIBOOT2_TAG_ALIGN
> > +        .short  MULTIBOOT2_HEADER_TAG_ENTRY_ADDRESS_EFI64
> > +        .short  MULTIBOOT2_HEADER_TAG_OPTIONAL
> > +        .long   12 /* Tag size. */
> > +        .long   sym_phys(__efi64_start)
> > +
> >          /* Multiboot2 header end tag. */
> >          .align  MULTIBOOT2_TAG_ALIGN
> >          .short  MULTIBOOT2_HEADER_TAG_END
> > @@ -94,6 +111,17 @@ ENTRY(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)
> > +        .long   BOOT_CS32
>
> .word
>
> ljmpl refers to an m32:16 not an m32:32
>
> > +
> > +efi_st:
> > +        .quad   0
> > +
> > +efi_ih:
> > +        .quad   0
> >
> >  .Lbad_cpu_msg: .asciz "ERR: Not a 64-bit CPU!"
> >  .Lbad_ldr_msg: .asciz "ERR: Not a Multiboot bootloader!"
> > @@ -124,6 +152,133 @@ print_err:
> >  .Lhalt: hlt
> >          jmp     .Lhalt
> >
> > +        .code64
> > +
> > +__efi64_start:
> > +        cld
> > +
> > +        /* Bootloaders may set multiboot[12].mem_lower to a nonzero value 
> > */
> > +        xor     %edx,%edx
> > +
> > +        /* Check for Multiboot2 bootloader */
> > +        cmp     $MULTIBOOT2_BOOTLOADER_MAGIC,%eax
> > +        je      efi_multiboot2_proto
> > +
> > +        jmp     not_multiboot
>
> not_multiboot is 32bit code.  You must drop out of 64bit before using
> it, or make a 64bit variant.
>
> > +
> > +efi_multiboot2_proto:
> > +        /* Skip Multiboot2 information fixed part */
> > +        lea     MB2_fixed_sizeof(%ebx),%ecx
> > +
> > +0:
> > +        /* Get mem_lower from Multiboot2 information */
> > +        cmpl    $MULTIBOOT2_TAG_TYPE_BASIC_MEMINFO,(%ecx)
> > +        jne     1f
> > +
> > +        mov     MB2_mem_lower(%ecx),%edx
> > +        jmp     4f
> > +
> > +1:
> > +        /* Get EFI SystemTable address from Multiboot2 information */
> > +        cmpl    $MULTIBOOT2_TAG_TYPE_EFI64,(%ecx)
> > +        jne     2f
> > +
> > +        lea     MB2_efi64_st(%ecx),%esi
> > +        lea     efi_st(%rip),%edi
> > +        movsq
>
> This is complete overkill for copying a 64bit variable out of the tag
> and into a local variable.  Just use a plain 64bit load and store.

I am not sure what do you mean by "64bit load and store" but I have
just realized that we do not need these variables. They are remnants
from early developments when I thought that we need ImageHandle
and SystemTable here and later somewhere else.

> > +        /* Do not go into real mode on EFI platform */
> > +        movb    $1,skip_realmode(%rip)
> > +
> > +        jmp     4f
> > +
> > +2:
> > +        /* Get EFI ImageHandle address from Multiboot2 information */
> > +        cmpl    $MULTIBOOT2_TAG_TYPE_EFI64_IH,(%ecx)
> > +        jne     3f
> > +
> > +        lea     MB2_efi64_ih(%ecx),%esi
> > +        lea     efi_ih(%rip),%edi
> > +        movsq
>
> And here.

Ditto.

> > +        jmp     4f
> > +
> > +3:
> > +        /* Is it the end of Multiboot2 information? */
> > +        cmpl    $MULTIBOOT2_TAG_TYPE_END,(%ecx)
> > +        je      run_bs
> > +
> > +4:
> > +        /* Go to next Multiboot2 information tag */
> > +        add     MB2_tag_size(%ecx),%ecx
> > +        add     $(MULTIBOOT2_TAG_ALIGN-1),%ecx
> > +        and     $~(MULTIBOOT2_TAG_ALIGN-1),%ecx
> > +        jmp     0b
> > +
> > +run_bs:
> > +        push    %rax
> > +        push    %rdx
>
> Does the EFI spec guarantee that we have a good stack to use at this point?

Unified Extensible Firmware Interface Specification, Version 2.4 Errata B,
section 2.3.4, x64 Platforms says: During boot services time the processor
is in the following execution mode: ..., 128 KiB, or more, of available
stack space. GRUB2 uses this stack too and do not move it to different
memory region. So, I think that here we are on safe side.

> > +        /* Initialize BSS (no nasty surprises!) */
> > +        lea     __bss_start(%rip),%rdi
> > +        lea     _end(%rip),%rcx
> > +        sub     %rdi,%rcx
> > +        xor     %rax,%rax
>
> xor %eax,%eax is shorter.
>
> > +        rep     stosb
>
> It would be more efficient to make sure that the linker aligns
> __bss_start and _end on 8 byte boundaries, and use stosq instead.

Right but just for this. Is it pays? We do this only once.
However, if you wish...

> > +        mov     efi_ih(%rip),%rdi   /* EFI ImageHandle */
> > +        mov     efi_st(%rip),%rsi   /* EFI SystemTable */
> > +        call    efi_multiboot2
> > +
> > +        pop     %rcx
> > +        pop     %rax
> > +
> > +        shl     $10-4,%rcx          /* Convert multiboot2.mem_lower to 
> > bytes/16 */
> > +
> > +        cli
>
> This looks suspiciously out of place.  Surely the EFI spec doesn't
> permit entry with interrupts enabled?

Unified Extensible Firmware Interface Specification, Version 2.4 Errata B,
section 2.3.4, x64 Platforms says: During boot services time the processor
is in the following execution mode: ..., Interrupts are enabledâthough no
interrupt services are supported other than the UEFI boot services timer
functions (All loaded device drivers are serviced synchronously by âpolling.â).
So, I think that we should use BS with interrupts enabled and disable
them after ExitBootServices(). Hmmm... Now I think that we should use
cli immediately after efi_multiboot2() call.

[...]

> > diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
> > index 6e98bc8..f50c10a 100644
> > --- a/xen/arch/x86/efi/efi-boot.h
> > +++ b/xen/arch/x86/efi/efi-boot.h
> > @@ -223,6 +223,9 @@ static void *__init efi_arch_allocate_mmap_buffer(UINTN 
> > *map_size)
> >
> >  static void __init efi_arch_pre_exit_boot(void)
> >  {
> > +    if ( !efi_loader )
> > +        return;
> > +
> >      if ( !trampoline_phys )
> >      {
> >          if ( !cfg.addr )
> > @@ -650,6 +653,32 @@ static bool_t __init 
> > efi_arch_use_config_file(EFI_SYSTEM_TABLE *SystemTable)
> >      return 1; /* x86 always uses a config file */
> >  }
> >
> > +void __init efi_multiboot2(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE 
> > *SystemTable)
> > +{
> > +    EFI_GRAPHICS_OUTPUT_PROTOCOL *gop;
> > +    UINTN cols, gop_mode = ~0, rows;
> > +
> > +    efi_platform = 1;
> > +    efi_loader = 0;
> > +
> > +    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);
> > +}
> > +
> >  /*
> >   * Local variables:
> >   * mode: C
> > diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> > index aebd010..8991b12 100644
> > --- a/xen/arch/x86/setup.c
> > +++ b/xen/arch/x86/setup.c
> > @@ -663,20 +663,23 @@ void __init noreturn __start_xen(unsigned long mbi_p)
> >      if ( ((unsigned long)cpu0_stack & (STACK_SIZE-1)) != 0 )
> >          panic("Misaligned CPU0 stack.");
> >
> > -    if ( efi_loader )
> > +    if ( efi_platform )
> >      {
> > -        set_pdx_range(xen_phys_start >> PAGE_SHIFT,
> > -                      (xen_phys_start + BOOTSTRAP_MAP_BASE) >> PAGE_SHIFT);
> > +        if ( efi_loader )
> > +        {
> > +            set_pdx_range(xen_phys_start >> PAGE_SHIFT,
> > +                          (xen_phys_start + BOOTSTRAP_MAP_BASE) >> 
> > PAGE_SHIFT);
> >
> > -        /* Clean up boot loader identity mappings. */
> > -        destroy_xen_mappings(xen_phys_start,
> > -                             xen_phys_start + BOOTSTRAP_MAP_BASE);
> > +            /* Clean up boot loader identity mappings. */
> > +            destroy_xen_mappings(xen_phys_start,
> > +                                 xen_phys_start + BOOTSTRAP_MAP_BASE);
> >
> > -        /* Make boot page tables match non-EFI boot. */
> > -        l3_bootmap[l3_table_offset(BOOTSTRAP_MAP_BASE)] =
> > -            l3e_from_paddr(__pa(l2_bootmap), __PAGE_HYPERVISOR);
> > +            /* Make boot page tables match non-EFI boot. */
> > +            l3_bootmap[l3_table_offset(BOOTSTRAP_MAP_BASE)] =
> > +                l3e_from_paddr(__pa(l2_bootmap), __PAGE_HYPERVISOR);
> > +        }
> >
> > -        memmap_type = loader;
> > +        memmap_type = "EFI";
> >      }
> >      else if ( e820_raw_nr != 0 )
> >      {
> > diff --git a/xen/arch/x86/x86_64/asm-offsets.c 
> > b/xen/arch/x86/x86_64/asm-offsets.c
> > index ca3712f..d27596b 100644
> > --- a/xen/arch/x86/x86_64/asm-offsets.c
> > +++ b/xen/arch/x86/x86_64/asm-offsets.c
> > @@ -172,4 +172,6 @@ void __dummy__(void)
> >      DEFINE(MB2_fixed_sizeof, sizeof(multiboot2_fixed_t));
> >      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/common/efi/boot.c b/xen/common/efi/boot.c
> > index f8be3dd..c5725ca 100644
> > --- a/xen/common/efi/boot.c
> > +++ b/xen/common/efi/boot.c
> > @@ -75,6 +75,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);
> > +
>
> If any of these forward declarations are needed, they should be

They are needed because efi-boot.h is included before above
mentioned functions definitions.

> introduced in the appropriate create efi_$FOO patch.  However, I can't

I thought about that during development. However, I stated that if I do what
you suggest then it is not clear who needs/uses these forward declarations.

> spot a need for any of them.

efi-boot.h:efi_multiboot2() uses them.

Daniel

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.