From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.71) id 1YHLEa-0003Vr-LU for mharc-grub-devel@gnu.org; Fri, 30 Jan 2015 18:44:20 -0500 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58730) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YHLEW-0003V5-6z for grub-devel@gnu.org; Fri, 30 Jan 2015 18:44:18 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YHLER-0001Zl-3v for grub-devel@gnu.org; Fri, 30 Jan 2015 18:44:16 -0500 Received: from userp1040.oracle.com ([156.151.31.81]:32219) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YHLEQ-0001Ze-Rd for grub-devel@gnu.org; Fri, 30 Jan 2015 18:44:11 -0500 Received: from acsinet21.oracle.com (acsinet21.oracle.com [141.146.126.237]) by userp1040.oracle.com (Sentrion-MTA-4.3.2/Sentrion-MTA-4.3.2) with ESMTP id t0UNhvCG009764 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Fri, 30 Jan 2015 23:43:58 GMT Received: from userz7022.oracle.com (userz7022.oracle.com [156.151.31.86]) by acsinet21.oracle.com (8.14.4+Sun/8.14.4) with ESMTP id t0UNhtVq008433 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Fri, 30 Jan 2015 23:43:56 GMT Received: from abhmp0009.oracle.com (abhmp0009.oracle.com [141.146.116.15]) by userz7022.oracle.com (8.14.5+Sun/8.14.4) with ESMTP id t0UNhssD009173; Fri, 30 Jan 2015 23:43:54 GMT Received: from olila.local.net-space.pl (/10.175.253.219) by default (Oracle Beehive Gateway v4.0) with ESMTP ; Fri, 30 Jan 2015 15:43:53 -0800 Date: Sat, 31 Jan 2015 00:43:46 +0100 From: Daniel Kiper To: Andrew Cooper Subject: Re: [PATCH 18/18] x86: add multiboot2 protocol support for EFI platforms Message-ID: <20150130234346.GG29167@olila.local.net-space.pl> References: <1422640462-28103-1-git-send-email-daniel.kiper@oracle.com> <1422640462-28103-19-git-send-email-daniel.kiper@oracle.com> <54CBD64D.60605@citrix.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <54CBD64D.60605@citrix.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-Source-IP: acsinet21.oracle.com [141.146.126.237] Content-Transfer-Encoding: quoted-printable X-MIME-Autoconverted: from 8bit to quoted-printable by userp1040.oracle.com id t0UNhvCG009764 X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.4.x-2.6.x [generic] X-Received-From: 156.151.31.81 Cc: jgross@suse.com, grub-devel@gnu.org, keir@xen.org, ian.campbell@citrix.com, phcoder@gmail.com, stefano.stabellini@eu.citrix.com, roy.franz@linaro.org, ning.sun@intel.com, david.vrabel@citrix.com, jbeulich@suse.com, xen-devel@lists.xenproject.org, qiaowei.ren@intel.com, richard.l.maliszewski@intel.com, gang.wei@intel.com, fu.wei@linaro.org X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.14 Precedence: list Reply-To: The development of GNU GRUB List-Id: The development of GNU GRUB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 30 Jan 2015 23:44:18 -0000 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 > > --- > > 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 > > #include > > #include > > +#include > > > > .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 po= int? Unified Extensible Firmware Interface Specification, Version 2.4 Errata B= , section 2.3.4, x64 Platforms says: During boot services time the processo= r 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 processo= r is in the following execution mode: ..., Interrupts are enabled=E2=80=93t= hough no interrupt services are supported other than the UEFI boot services timer functions (All loaded device drivers are serviced synchronously by =E2=80= =9Cpolling.=E2=80=9D). 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(EF= I_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 =3D ~0, rows; > > + > > + efi_platform =3D 1; > > + efi_loader =3D 0; > > + > > + efi_init(ImageHandle, SystemTable); > > + > > + efi_console_set_mode(); > > + > > + if ( StdOut->QueryMode(StdOut, StdOut->Mode->Mode, > > + &cols, &rows) =3D=3D EFI_SUCCESS ) > > + efi_arch_console_init(cols, rows); > > + > > + gop =3D efi_get_gop(); > > + gop_mode =3D 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)) !=3D 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) >> P= AGE_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)] =3D > > - l3e_from_paddr(__pa(l2_bootmap), __PAGE_HYPERVISOR); > > + /* Make boot page tables match non-EFI boot. */ > > + l3_bootmap[l3_table_offset(BOOTSTRAP_MAP_BASE)] =3D > > + l3e_from_paddr(__pa(l2_bootmap), __PAGE_HYPERVISOR); > > + } > > > > - memmap_type =3D loader; > > + memmap_type =3D "EFI"; > > } > > else if ( e820_raw_nr !=3D 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 *guid= 2); > > > > +static void efi_init(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *Syste= mTable); > > +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, UINT= N 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 w= hat you suggest then it is not clear who needs/uses these forward declaration= s. > spot a need for any of them. efi-boot.h:efi_multiboot2() uses them. Daniel