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

[Daniel Kiper]

On Tue, Mar 17, 2015 at 10:32:01AM +0000, Jan Beulich wrote:
> >>> On 30.01.15 at 18:54, <address@hidden> wrote:
> > @@ -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
> > +
> > +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
> > +
> > +efi_multiboot2_proto:
>
> jne not_multiboot (and efi_multiboot2_proto dropped altogether)
>
> > +        /* Skip Multiboot2 information fixed part */
> > +        lea     MB2_fixed_sizeof(%ebx),%ecx
>
> Let's please not add more assumptions than necessary about stuff
> being below 4G.

I am not sure what do you mean by that.

> > +
> > +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
>
> A simple pair of mov-s please, assuming all of this really needs to be
> done in assembly in the first place. Also please use .L<name> labels
> in this assembly coded switch statement to ease reading.
>
> > +
> > +        /* 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
> > +        jmp     4f
> > +
> > +3:
> > +        /* Is it the end of Multiboot2 information? */
> > +        cmpl    $MULTIBOOT2_TAG_TYPE_END,(%ecx)
> > +        je      run_bs
> > +
> > +4:
>
> Please switch the order so that 2 can fall through into 4 (and you
> then won't need the run_bs label, which otherwise should also
> becom .Lrun_bs).
>
> > +        /* 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
> > +
> > +        /* Initialize BSS (no nasty surprises!) */
> > +        lea     __bss_start(%rip),%rdi
> > +        lea     _end(%rip),%rcx
> > +        sub     %rdi,%rcx
> > +        xor     %rax,%rax
> > +        rep     stosb
>
> rep stosb
>
> > +
> > +        mov     efi_ih(%rip),%rdi   /* EFI ImageHandle */
> > +        mov     efi_st(%rip),%rsi   /* EFI SystemTable */
> > +        call    efi_multiboot2
>
> With efi_multiboot2 being a C function it really looks like much of the
> above doesn't need to be done in assembly.

Potentially make sense. I will try to do that.

Daniel