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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 30.01.15 at 18:54, <daniel.kiper@xxxxxxxxxx> 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.

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

> +
> +        pop     %rcx
> +        pop     %rax
> +
> +        shl     $10-4,%rcx          /* Convert multiboot2.mem_lower to 
> bytes/16 */
> +
> +        cli
> +
> +        /* Initialise GDT */
> +        lgdt    gdt_boot_descr(%rip)
> +
> +        /* Reload code selector */
> +        ljmpl   *cs32_switch_addr(%rip)
> +
> +        .code32
> +
> +cs32_switch:
> +        /* Initialise basic data segments */
> +        mov     $BOOT_DS,%edx
> +        mov     %edx,%ds
> +        mov     %edx,%es
> +        mov     %edx,%fs
> +        mov     %edx,%gs
> +        mov     %edx,%ss
> +
> +        mov     $sym_phys(cpu0_stack)+1024,%esp
> +
> +        /* Disable paging */
> +        mov     %cr0,%edx
> +        and     $(~X86_CR0_PG),%edx
> +        mov     %edx,%cr0
> +
> +        push    %eax
> +        push    %ecx
> +
> +        /* Disable Long Mode */
> +        mov     $MSR_EFER,%ecx
> +        rdmsr
> +        and     $(~EFER_LME),%eax
> +        wrmsr

I don't think this is really needed.

> +
> +        pop     %ecx
> +        pop     %eax
> +
> +        /* Turn off PAE */
> +        mov     %cr4,%edx
> +        and     $(~X86_CR4_PAE),%edx
> +        mov     %edx,%cr4

Nor this.

> @@ -179,7 +334,7 @@ multiboot2_proto:
>          and     $~(MULTIBOOT2_TAG_ALIGN-1),%ecx
>          jmp     0b
>  
> -trampoline_setup:
> +bios_platform:
>          /* Set up trampoline segment 64k below EBDA */

This is still trampoline setup code, so it's not clear why you rename
the label. If you need another named label ...

> @@ -195,12 +350,13 @@ trampoline_setup:
>           * multiboot structure (if available) and use the smallest.
>           */
>          cmp     $0x100,%edx         /* is the multiboot value too small? */
> -        jb      2f                  /* if so, do not use it */
> +        jb      trampoline_setup    /* if so, do not use it */
>          shl     $10-4,%edx
>          cmp     %ecx,%edx           /* compare with BDA value */
>          cmovb   %edx,%ecx           /* and use the smaller */
>  
> -2:      /* Reserve 64kb for the trampoline */
> +trampoline_setup:
> +        /* Reserve 64kb for the trampoline */

... here, then give it a name descriptive to the specific purpose
you have.

> @@ -215,6 +371,9 @@ trampoline_setup:
>          call    reloc               /* %ecx contains trampoline address */
>          mov     %eax,sym_phys(multiboot_ptr)
>  
> +        cmpb    $1,sym_phys(skip_realmode)
> +        je      1f
> +
>          /* Initialize BSS (no nasty surprises!) */

As the checked variable is completely unrelated to the purpose of
the code skipped, the code being added needs a comment.

> --- 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";
>      }

Please try to limit the churn you cause on existing code: Afaict all
you really need is the insertion of another else if (along with the
switch from efi_loader to efi_platform, which is subject to change
due to comments on earlier patches anyway).

Jan

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