[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 18/18] x86: add multiboot2 protocol support for EFI platforms
From: |
Jan Beulich |
Subject: |
Re: [PATCH 18/18] x86: add multiboot2 protocol support for EFI platforms |
Date: |
Tue, 17 Mar 2015 10:32:01 +0000 |
>>> 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.
> +
> +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
- Re: [PATCH 18/18] x86: add multiboot2 protocol support for EFI platforms,
Jan Beulich <=
- Re: [PATCH 18/18] x86: add multiboot2 protocol support for EFI platforms, Daniel Kiper, 2015/03/17
- Re: [PATCH 18/18] x86: add multiboot2 protocol support for EFI platforms, Daniel Kiper, 2015/03/27
- Re: [PATCH 18/18] x86: add multiboot2 protocol support for EFI platforms, Jan Beulich, 2015/03/27
- Re: [PATCH 18/18] x86: add multiboot2 protocol support for EFI platforms, Daniel Kiper, 2015/03/27
- Re: [PATCH 18/18] x86: add multiboot2 protocol support for EFI platforms, Jan Beulich, 2015/03/27
- Re: [PATCH 18/18] x86: add multiboot2 protocol support for EFI platforms, Daniel Kiper, 2015/03/27
- Re: [PATCH 18/18] x86: add multiboot2 protocol support for EFI platforms, Jan Beulich, 2015/03/27
- Re: [PATCH 18/18] x86: add multiboot2 protocol support for EFI platforms, Daniel Kiper, 2015/03/27