From mboxrd@z Thu Jan 1 00:00:00 1970 From: Konrad Rzeszutek Wilk Subject: Re: [PATCH v2 20/23] x86: add multiboot2 protocol support for EFI platforms Date: Mon, 10 Aug 2015 16:07:15 -0400 Message-ID: <20150810200715.GQ13576__46455.7713632987$1439237353$gmane$org@l.oracle.com> References: <1437402558-7313-1-git-send-email-daniel.kiper@oracle.com> <1437402558-7313-21-git-send-email-daniel.kiper@oracle.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta14.messagelabs.com ([193.109.254.103]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1ZOtM8-0000jf-Tv for xen-devel@lists.xenproject.org; Mon, 10 Aug 2015 20:07:37 +0000 Content-Disposition: inline In-Reply-To: <1437402558-7313-21-git-send-email-daniel.kiper@oracle.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Daniel Kiper Cc: jgross@suse.com, grub-devel@gnu.org, keir@xen.org, ian.campbell@citrix.com, stefano.stabellini@eu.citrix.com, andrew.cooper3@citrix.com, gang.wei@intel.com, roy.franz@linaro.org, ning.sun@intel.com, david.vrabel@citrix.com, jbeulich@suse.com, phcoder@gmail.com, xen-devel@lists.xenproject.org, wei.liu2@citrix.com, richard.l.maliszewski@intel.com, qiaowei.ren@intel.com, fu.wei@linaro.org List-Id: xen-devel@lists.xenproject.org On Mon, Jul 20, 2015 at 04:29:15PM +0200, Daniel Kiper wrote: > Signed-off-by: Daniel Kiper > --- > v2 - suggestions/fixes: > - generate multiboot2 header using macros > (suggested by Jan Beulich), > - switch CPU to x86_32 mode before > jumping to 32-bit code > (suggested by Andrew Cooper), > - reduce code changes to increase patch readability > (suggested by Jan Beulich), > - improve comments > (suggested by Jan Beulich), > - ignore MULTIBOOT2_TAG_TYPE_BASIC_MEMINFO tag on EFI platform > and find on my own multiboot2.mem_lower value, > - stop execution if EFI platform is detected > in legacy BIOS path. > --- > xen/arch/x86/boot/head.S | 157 +++++++++++++++++++++++++++++++++++-- > xen/arch/x86/efi/efi-boot.h | 30 +++++++ > xen/arch/x86/efi/stub.c | 5 ++ > xen/arch/x86/setup.c | 10 ++- > xen/arch/x86/x86_64/asm-offsets.c | 2 + > xen/arch/x86/xen.lds.S | 4 +- > xen/common/efi/boot.c | 12 +++ > xen/include/xen/efi.h | 1 + > 8 files changed, 210 insertions(+), 11 deletions(-) > > diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S > index 57197db..056047f 100644 > --- a/xen/arch/x86/boot/head.S > +++ b/xen/arch/x86/boot/head.S > @@ -89,6 +89,13 @@ multiboot1_header_end: > 0, /* Number of the lines - no preference. */ \ > 0 /* Number of bits per pixel - no preference. */ > > + /* Do not disable EFI boot services. */ s/disable/exit/ ? Or perhaps: /* Inhibit GRUB2 from calling ExitBootServices. */ ? > + mb2ht_init MB2_HT(EFI_BS), MB2_HT(OPTIONAL) > + > + /* EFI64 entry point. */ > + mb2ht_init MB2_HT(ENTRY_ADDRESS_EFI64), MB2_HT(OPTIONAL), \ > + sym_phys(__efi64_start) > + > /* Multiboot2 header end tag. */ > mb2ht_init MB2_HT(END), MB2_HT(REQUIRED) > .Lmultiboot2_header_end: > @@ -100,9 +107,15 @@ multiboot1_header_end: > 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) > + .word BOOT_CS32 > > .Lbad_cpu_msg: .asciz "ERR: Not a 64-bit CPU!" > .Lbad_ldr_msg: .asciz "ERR: Not a Multiboot bootloader!" > +.Lbad_mb2_ldr: .asciz "ERR: Use latest Multiboot2 compatible bootloader!" Multiboot2+EFI compatible..? > > .section .init.text, "ax", @progbits > > @@ -111,6 +124,9 @@ bad_cpu: > jmp print_err > not_multiboot: > mov $(sym_phys(.Lbad_ldr_msg)),%esi # Error message > + jmp print_err > +mb2_too_old: > + mov $(sym_phys(.Lbad_mb2_ldr)),%esi # Error message > print_err: > mov $0xB8000,%edi # VGA framebuffer > 1: mov (%esi),%bl > @@ -130,6 +146,119 @@ print_err: > .Lhalt: hlt > jmp .Lhalt > > + .code64 > + > +__efi64_start: > + cld > + > + /* Check for Multiboot2 bootloader. */ > + cmp $MULTIBOOT2_BOOTLOADER_MAGIC,%eax > + je efi_multiboot2_proto > + > + /* Jump to not_multiboot after switching CPU to x86_32 mode. */ > + lea not_multiboot(%rip),%rdi > + jmp x86_32_switch > + > +efi_multiboot2_proto: > + /* > + * Multiboot2 information address is 32-bit, > + * so, zero higher half of %rbx. > + */ > + mov %ebx,%ebx > + > + /* Skip Multiboot2 information fixed part. */ > + lea MB2_fixed_sizeof(%rbx),%rcx Don't want to check that the address is aligned properly? > + > +0: > + /* Get EFI SystemTable address from Multiboot2 information. */ > + cmpl $MULTIBOOT2_TAG_TYPE_EFI64,MB2_tag_type(%rcx) > + jne 1f > + > + mov MB2_efi64_st(%rcx),%rsi > + > + /* Do not go into real mode on EFI platform. */ Add please: /* And also inhibit clearing of BSS later on. */ > + movb $1,skip_realmode(%rip) > + jmp 3f > + > +1: > + /* Get EFI ImageHandle address from Multiboot2 information. */ > + cmpl $MULTIBOOT2_TAG_TYPE_EFI64_IH,MB2_tag_type(%rcx) > + jne 2f > + > + mov MB2_efi64_ih(%rcx),%rdi > + jmp 3f > + > +2: > + /* Is it the end of Multiboot2 information? */ > + cmpl $MULTIBOOT2_TAG_TYPE_END,MB2_tag_type(%rcx) > + je run_bs > + > +3: > + /* Go to next Multiboot2 information tag. */ > + add MB2_tag_size(%rcx),%ecx > + add $(MULTIBOOT2_TAG_ALIGN-1),%rcx > + and $~(MULTIBOOT2_TAG_ALIGN-1),%rcx > + jmp 0b > + > +run_bs: > + push %rax > + push %rdi > + > + /* Initialize BSS (no nasty surprises!). */ > + lea __bss_start(%rip),%rdi > + lea __bss_end(%rip),%rcx > + sub %rdi,%rcx > + shr $3,%rcx > + xor %eax,%eax > + rep stosq > + > + pop %rdi > + > + /* > + * IN: %rdi - EFI ImageHandle, %rsi - EFI SystemTable. > + * OUT: %rax - multiboot2.mem_lower. Do not get this value from > + * MULTIBOOT2_TAG_TYPE_BASIC_MEMINFO tag. It could be bogus on > + * EFI platforms. > + */ > + call efi_multiboot2 > + > + /* Convert multiboot2.mem_lower to bytes/16. */ > + mov %rax,%rcx > + shr $4,%rcx > + > + pop %rax > + > + /* Jump to trampoline_setup after switching CPU to x86_32 mode. */ > + lea trampoline_setup(%rip),%rdi > + > +x86_32_switch: > + 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 > + > + /* Disable paging. */ > + mov %cr0,%edx > + and $(~X86_CR0_PG),%edx > + mov %edx,%cr0 > + > + /* Jump to earlier loaded address. */ > + jmp *%edi > + > __start: > cld > cli > @@ -157,7 +286,7 @@ __start: > > /* Not available? BDA value will be fine. */ > cmovnz MB_mem_lower(%ebx),%edx > - jmp trampoline_setup > + jmp trampoline_bios_setup > > multiboot2_proto: > /* Skip Multiboot2 information fixed part. */ > @@ -169,12 +298,19 @@ multiboot2_proto: > jne 1f > > mov MB2_mem_lower(%ecx),%edx > - jmp trampoline_setup > + jmp trampoline_bios_setup > > 1: > + /* EFI mode is not supported via legacy BIOS path. */ > + cmpl $MULTIBOOT2_TAG_TYPE_EFI32,MB2_tag_type(%ecx) > + je mb2_too_old > + > + cmpl $MULTIBOOT2_TAG_TYPE_EFI64,MB2_tag_type(%ecx) > + je mb2_too_old > + > /* Is it the end of Multiboot2 information? */ > cmpl $MULTIBOOT2_TAG_TYPE_END,MB2_tag_type(%ecx) > - je trampoline_setup > + je trampoline_bios_setup > > /* Go to next Multiboot2 information tag. */ > add MB2_tag_size(%ecx),%ecx > @@ -182,7 +318,7 @@ multiboot2_proto: > and $~(MULTIBOOT2_TAG_ALIGN-1),%ecx > jmp 0b > > -trampoline_setup: > +trampoline_bios_setup: > /* Set up trampoline segment 64k below EBDA */ > movzwl 0x40e,%ecx /* EBDA segment */ > cmp $0xa000,%ecx /* sanity check (high) */ > @@ -198,12 +334,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. */ > sub $0x1000,%ecx > > /* From arch/x86/smpboot.c: start_eip had better be page-aligned! */ > @@ -220,6 +357,13 @@ trampoline_setup: > add $12,%esp /* Remove reloc() args from stack. */ > mov %eax,sym_phys(multiboot_ptr) > > + /* > + * Do not zero BSS on EFI platform here. > + * It was initialized earlier. > + */ > + cmpb $1,sym_phys(skip_realmode) > + je 1f > + > /* Initialize BSS (no nasty surprises!). */ > mov $sym_phys(__bss_start),%edi > mov $sym_phys(__bss_end),%ecx > @@ -228,6 +372,7 @@ trampoline_setup: > xor %eax,%eax > rep stosl > > +1: > /* Interrogate CPU extended features via CPUID. */ > mov $0x80000000,%eax > cpuid > diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h > index 3d25c48..1b25a2d 100644 > --- a/xen/arch/x86/efi/efi-boot.h > +++ b/xen/arch/x86/efi/efi-boot.h > @@ -228,6 +228,9 @@ static void *__init efi_arch_allocate_mmap_buffer(UINTN map_size) > > static void __init efi_arch_pre_exit_boot(void) > { > + if ( !efi_enabled(EFI_LOADER) ) > + return; > + > if ( !trampoline_phys ) > { > if ( !cfg.addr ) > @@ -662,6 +665,33 @@ static bool_t __init efi_arch_use_config_file(EFI_SYSTEM_TABLE *SystemTable) > return 1; /* x86 always uses a config file */ > } > > +paddr_t __init efi_multiboot2(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) > +{ > + EFI_GRAPHICS_OUTPUT_PROTOCOL *gop; > + UINTN cols, gop_mode = ~0, rows; > + > + set_bit(EFI_PLATFORM, &efi.flags); > + > + 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); > + > + return cfg.addr; > +} > + > /* > * Local variables: > * mode: C > diff --git a/xen/arch/x86/efi/stub.c b/xen/arch/x86/efi/stub.c > index c5ae369..d30fe89 100644 > --- a/xen/arch/x86/efi/stub.c > +++ b/xen/arch/x86/efi/stub.c > @@ -13,6 +13,11 @@ struct efi __read_mostly efi = { > .smbios3 = EFI_INVALID_TABLE_ADDR > }; > > +void __init efi_multiboot2(void) > +{ > + /* TODO: Fail if entered! */ Heheh. Could we add the 'ud2' instruction here? > +} > + > void __init efi_init_memory(void) { } > > void efi_update_l4_pgtable(unsigned int l4idx, l4_pgentry_t l4e) { } > diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c > index a59fc4e..8bec67f 100644 > --- a/xen/arch/x86/setup.c > +++ b/xen/arch/x86/setup.c > @@ -695,7 +695,7 @@ void __init noreturn __start_xen(unsigned long mbi_p) > if ( !(mbi->flags & MBI_MODULES) || (mbi->mods_count == 0) ) > panic("dom0 kernel not specified. Check bootloader configuration."); > > - if ( efi_enabled(EFI_PLATFORM) ) > + if ( efi_enabled(EFI_LOADER) ) > { > set_pdx_range(xen_phys_start >> PAGE_SHIFT, > (xen_phys_start + BOOTSTRAP_MAP_BASE) >> PAGE_SHIFT); > @@ -708,7 +708,11 @@ void __init noreturn __start_xen(unsigned long mbi_p) > l3_bootmap[l3_table_offset(BOOTSTRAP_MAP_BASE)] = > l3e_from_paddr(__pa(l2_bootmap), __PAGE_HYPERVISOR); > > - memmap_type = loader; > + memmap_type = "EFI"; > + } > + else if ( efi_enabled(EFI_PLATFORM) ) > + { > + memmap_type = "EFI"; > } > else if ( e820_raw_nr != 0 ) > { > @@ -806,7 +810,7 @@ void __init noreturn __start_xen(unsigned long mbi_p) > * we can relocate the dom0 kernel and other multiboot modules. Also, on > * x86/64, we relocate Xen to higher memory. > */ > - for ( i = 0; !efi_enabled(EFI_PLATFORM) && i < mbi->mods_count; i++ ) > + for ( i = 0; !efi_enabled(EFI_LOADER) && i < mbi->mods_count; i++ ) > { > if ( mod[i].mod_start & (PAGE_SIZE - 1) ) > panic("Bootloader didn't honor module alignment request."); > diff --git a/xen/arch/x86/x86_64/asm-offsets.c b/xen/arch/x86/x86_64/asm-offsets.c > index b926082..b7aed49 100644 > --- a/xen/arch/x86/x86_64/asm-offsets.c > +++ b/xen/arch/x86/x86_64/asm-offsets.c > @@ -173,4 +173,6 @@ void __dummy__(void) > OFFSET(MB2_tag_type, multiboot2_tag_t, type); > 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/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S > index 87f3e83..a399615 100644 > --- a/xen/arch/x86/xen.lds.S > +++ b/xen/arch/x86/xen.lds.S > @@ -162,7 +162,7 @@ SECTIONS > . = ALIGN(STACK_SIZE); > __init_end = .; > > - . = ALIGN(4); > + . = ALIGN(8); > .bss : { /* BSS */ > __bss_start = .; > *(.bss.stack_aligned) > @@ -176,7 +176,7 @@ SECTIONS > *(.bss.percpu.read_mostly) > . = ALIGN(SMP_CACHE_BYTES); > __per_cpu_data_end = .; > - . = ALIGN(4); > + . = ALIGN(8); This aligment check I presume is due to 'rep stosq'? As such could you add a comment here please? > __bss_end = .; > } :text > _end = . ; > diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c > index bf2f198..5a02e71 100644 > --- a/xen/common/efi/boot.c > +++ b/xen/common/efi/boot.c > @@ -79,6 +79,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); > + > static const EFI_BOOT_SERVICES *__initdata efi_bs; > static UINT32 __initdata efi_bs_revision; > static EFI_HANDLE __initdata efi_ih; > @@ -960,6 +971,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) > > #ifndef CONFIG_ARM /* Disabled until runtime services implemented. */ > set_bit(EFI_PLATFORM, &efi.flags); > + set_bit(EFI_LOADER, &efi.flags); > #endif > > efi_init(ImageHandle, SystemTable); > diff --git a/xen/include/xen/efi.h b/xen/include/xen/efi.h > index 318bbec..6b952df 100644 > --- a/xen/include/xen/efi.h > +++ b/xen/include/xen/efi.h > @@ -9,6 +9,7 @@ > #define EFI_INVALID_TABLE_ADDR (~0UL) > > #define EFI_PLATFORM 0 > +#define EFI_LOADER 1 > > /* Add fields here only if they need to be referenced from non-EFI code. */ > struct efi { With the changes above: Reviewed-by: Konrad Rzeszutek Wilk > -- > 1.7.10.4 > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel