Xen project Mailing List

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

©2013 Xen Project, A Linux Foundation Collaborative Project. All Rights Reserved.
Linux Foundation is a registered trademark of The Linux Foundation.
Xen Project is a trademark of The Linux Foundation.