From patchwork Thu Feb 2 22:43:50 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Daniel Kiper X-Patchwork-Id: 9553309 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id EE31A60236 for ; Thu, 2 Feb 2017 22:46:40 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id E7AA528491 for ; Thu, 2 Feb 2017 22:46:40 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id DC121284BC; Thu, 2 Feb 2017 22:46:40 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-4.2 required=2.0 tests=BAYES_00, RCVD_IN_DNSWL_MED, UNPARSEABLE_RELAY autolearn=ham version=3.3.1 Received: from lists.xenproject.org (lists.xenproject.org [192.237.175.120]) (using TLSv1.2 with cipher AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id E41AE28491 for ; Thu, 2 Feb 2017 22:46:39 +0000 (UTC) Received: from localhost ([127.0.0.1] helo=lists.xenproject.org) by lists.xenproject.org with esmtp (Exim 4.84_2) (envelope-from ) id 1cZQ71-0000Kb-Rh; Thu, 02 Feb 2017 22:44:19 +0000 Received: from mail6.bemta6.messagelabs.com ([193.109.254.103]) by lists.xenproject.org with esmtp (Exim 4.84_2) (envelope-from ) id 1cZQ70-0000KU-7c for xen-devel@lists.xenproject.org; Thu, 02 Feb 2017 22:44:18 +0000 Received: from [193.109.254.147] by server-5.bemta-6.messagelabs.com id C2/21-11476-146B3985; Thu, 02 Feb 2017 22:44:17 +0000 X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFrrLIsWRWlGSWpSXmKPExsUyZ7p8oK7Dtsk RBhfmqlp83zKZyYHR4/CHKywBjFGsmXlJ+RUJrBlHls9lKTgYX3Fx1hr2BsYJLl2MXBxCApOY JKb032GCcP4wSrz7fJoVwtnAKLHi1gooZyKjxP4lW5i7GDk5WARUJCbMeQZmswnoSFz88pAdx BYRUJK4t2oy2ChmgX1MEovW7GADSQgLhEjcPfmPBcTmFbCV+HKyhQ1iagujxNmuO8wQCUGJkz OfgBUxC6hL/Jl3CSjOAWRLSyz/xwERlpdo3jobrJxTwEOioe0b2HxRoIOmnNwGZksIGEu0v73 INoFRaBaSqbOQTJ2FMHUWkqkLGFlWMWoUpxaVpRbpGpnpJRVlpmeU5CZm5ugaGpjp5aYWFyem p+YkJhXrJefnbmIEBjsDEOxgPLMg8BCjJAeTkijvz4WTI4T4kvJTKjMSizPii0pzUosPMcpwc ChJ8O7fApQTLEpNT61Iy8wBxh1MWoKDR0mE12grUJq3uCAxtzgzHSJ1ilFRSpx3PUifAEgioz QPrg0W65cYZaWEeRmBDhHiKUgtys0sQZV/xSjOwagkzGsMMp4nM68EbvoroMVMQIt/Pp4Esrg kESEl1cDYv6r2r4PvOZ/M/GtTGtZtbf53aQJf6Ws1hoqjN1+prDrZ427WwfZzd7P2w/NPOcp8 V+4RXyV4trxiDrdPHvff94U6kWYScpLcMoH/z1+eXHXtxDQP7qYJt/OeBPfoymdsn5N8w2ILH wfbAr+V+3oq7K7++uw/yXFHJsPSRL5FFS88JnA5x11VYinOSDTUYi4qTgQAd9iz7PACAAA= X-Env-Sender: daniel.kiper@oracle.com X-Msg-Ref: server-14.tower-27.messagelabs.com!1486075454!72698506!1 X-Originating-IP: [156.151.31.81] X-SpamReason: No, hits=0.0 required=7.0 tests=sa_preprocessor: VHJ1c3RlZCBJUDogMTU2LjE1MS4zMS44MSA9PiAyODgzMzk=\n X-StarScan-Received: X-StarScan-Version: 9.1.1; banners=-,-,- X-VirusChecked: Checked Received: (qmail 17322 invoked from network); 2 Feb 2017 22:44:16 -0000 Received: from userp1040.oracle.com (HELO userp1040.oracle.com) (156.151.31.81) by server-14.tower-27.messagelabs.com with DHE-RSA-AES256-GCM-SHA384 encrypted SMTP; 2 Feb 2017 22:44:16 -0000 Received: from aserv0022.oracle.com (aserv0022.oracle.com [141.146.126.234]) by userp1040.oracle.com (Sentrion-MTA-4.3.2/Sentrion-MTA-4.3.2) with ESMTP id v12MhwMr024088 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Thu, 2 Feb 2017 22:43:58 GMT Received: from aserv0122.oracle.com (aserv0122.oracle.com [141.146.126.236]) by aserv0022.oracle.com (8.14.4/8.14.4) with ESMTP id v12MhvqK023576 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Thu, 2 Feb 2017 22:43:57 GMT Received: from abhmp0012.oracle.com (abhmp0012.oracle.com [141.146.116.18]) by aserv0122.oracle.com (8.14.4/8.14.4) with ESMTP id v12MhvJd028390; Thu, 2 Feb 2017 22:43:57 GMT Received: from olila.local.net-space.pl (/10.175.161.206) by default (Oracle Beehive Gateway v4.0) with ESMTP ; Thu, 02 Feb 2017 14:43:56 -0800 Date: Thu, 2 Feb 2017 23:43:50 +0100 From: Daniel Kiper To: xen-devel@lists.xenproject.org Message-ID: <20170202224350.GY16671@olila.local.net-space.pl> References: <1486072879-31637-1-git-send-email-daniel.kiper@oracle.com> <1486072879-31637-5-git-send-email-daniel.kiper@oracle.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <1486072879-31637-5-git-send-email-daniel.kiper@oracle.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-Source-IP: aserv0022.oracle.com [141.146.126.234] Cc: jgross@suse.com, sstabellini@kernel.org, andrew.cooper3@citrix.com, cardoe@cardoe.com, marcos.matsunaga@oracle.com, pgnet.dev@gmail.com, ning.sun@intel.com, julien.grall@arm.com, jbeulich@suse.com, qiaowei.ren@intel.com, gang.wei@intel.com, fu.wei@linaro.org Subject: Re: [Xen-devel] [PATCH v14 4/9] x86: add multiboot2 protocol support for EFI platforms X-BeenThere: xen-devel@lists.xen.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: Xen developer discussion List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xen.org Sender: "Xen-devel" X-Virus-Scanned: ClamAV using ClamSMTP On Thu, Feb 02, 2017 at 11:01:14PM +0100, Daniel Kiper wrote: > This way Xen can be loaded on EFI platforms using GRUB2 and > other boot loaders which support multiboot2 protocol. > > Signed-off-by: Daniel Kiper > --- > v14 - suggestions/fixes: > - mark .init.data section as writable; by the way we must change > similar definition in xen/arch/x86/boot/x86_64.S because otherwise > compiler complains about section types conflicts > (suggested by Jan Beulich), > - use %r15 instead of %r15d > (suggested by Jan Beulich), > - remove redundant add from UEFI stack alignment > (suggested by Jan Beulich), > - use "mov (%rsp),%rdi" instead of "pop %rdi/push %rdi" > (suggested by Jan Beulich), > - return void instead of paddr_t from efi_multiboot2() > and then simplify a bit trampoline setup assembly > (suggested by Jan Beulich), > - remove "(XEN)" from efi_multiboot2() stub error messages > (suggested by Jan Beulich), > - move err from inline assembly OutputOperands to InputOperands in > stub.c:efi_multiboot2(); this way we avoid following compile time error: > stub.c: In function ‘efi_multiboot2’: > stub.c:36:5: error: read-only variable ‘err’ used as ‘asm’ output > asm volatile( > ^~~ > issue was introduced by changing err type to "static const CHAR16 __initconst"; > potentially we can revert this change but move to InputOperands looks better IMO; > even if we are not able to specify %rdx in Clobbers; simply we do not care > because next instruction after call is hlt > (discovered by Konrad Rzeszutek Wilk and Marcos Matsunaga), > - take into account MBI_SPACE_MIN in ASSERT((trampoline_end - trampoline_start) < ...) > (suggested by Jan Beulich), > - improve comments > (suggested by Jan Beulich). Diff as Doug asked: diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S index 2ecdcb3..5147204 100644 --- a/xen/arch/x86/boot/head.S +++ b/xen/arch/x86/boot/head.S @@ -116,7 +116,7 @@ gdt_boot_descr: .Lbad_ldr_nih: .asciz "ERR: EFI ImageHandle is not provided by bootloader!" .Lbad_efi_msg: .asciz "ERR: EFI IA-32 platforms are not supported!" - .section .init.data, "a", @progbits + .section .init.data, "aw", @progbits .align 4 vga_text_buffer: @@ -173,9 +173,10 @@ not_multiboot: __efi64_mb2_start: /* - * Multiboot2 spec says that here CPU is in 64-bit mode. However, there - * is also guarantee that all code and data is always put by the bootloader - * below 4 GiB. Hence, we can safely use in most cases 32-bit addressing. + * Multiboot2 spec says that here CPU is in 64-bit mode. However, + * there is also guarantee that all code and data is always put + * by the bootloader below 4 GiB. Hence, we can safely truncate + * addresses to 32-bits in most cases below. */ cld @@ -188,7 +189,7 @@ __efi64_mb2_start: je .Lefi_multiboot2_proto /* Jump to not_multiboot after switching CPU to x86_32 mode. */ - lea not_multiboot(%rip),%r15d + lea not_multiboot(%rip),%r15 jmp x86_32_switch .Lefi_multiboot2_proto: @@ -248,28 +249,34 @@ __efi64_mb2_start: cmpb $0,efi_platform(%rip) /* Jump to .Lmb2_no_bs after switching CPU to x86_32 mode. */ - lea .Lmb2_no_bs(%rip),%r15d + lea .Lmb2_no_bs(%rip),%r15 jz x86_32_switch /* Is EFI SystemTable address provided by boot loader? */ test %rsi,%rsi /* Jump to .Lmb2_no_st after switching CPU to x86_32 mode. */ - lea .Lmb2_no_st(%rip),%r15d + lea .Lmb2_no_st(%rip),%r15 jz x86_32_switch /* Is EFI ImageHandle address provided by boot loader? */ test %rdi,%rdi /* Jump to .Lmb2_no_ih after switching CPU to x86_32 mode. */ - lea .Lmb2_no_ih(%rip),%r15d + lea .Lmb2_no_ih(%rip),%r15 jz x86_32_switch - /* Align the stack as UEFI spec requires. */ - add $15,%rsp + /* + * Align the stack as UEFI spec requires. Keep it aligned + * before efi_multiboot2() call by pushing/popping even + * numbers of items on it. + */ and $~15,%rsp + /* Save Multiboot2 magic on the stack. */ push %rax + + /* Save EFI ImageHandle on the stack. */ push %rdi /* @@ -284,34 +291,26 @@ __efi64_mb2_start: xor %eax,%eax rep stosq - pop %rdi - - /* Align the stack as UEFI spec requires. */ - push %rdi + /* Keep the stack aligned. Do not pop a single item off it. */ + mov (%rsp),%rdi /* * efi_multiboot2() is called according to System V AMD64 ABI: - * - IN: %rdi - EFI ImageHandle, %rsi - EFI SystemTable, - * - OUT: %rax - trampoline address. - * - * MULTIBOOT2_TAG_TYPE_BASIC_MEMINFO tag is not provided - * on EFI platforms. Hence, it could not be used like - * on legacy BIOS platforms. + * - IN: %rdi - EFI ImageHandle, %rsi - EFI SystemTable. */ call efi_multiboot2 - /* Convert memory address to bytes/16 and store it in safe place. */ - shr $4,%eax - mov %eax,%ecx + /* Just pop an item from the stack. */ + pop %rax - pop %rdi + /* Restore Multiboot2 magic. */ pop %rax /* Jump to trampoline_setup after switching CPU to x86_32 mode. */ - lea trampoline_setup(%rip),%r15d + lea trampoline_setup(%rip),%r15 x86_32_switch: - mov %r15d,%edi + mov %r15,%rdi cli @@ -428,46 +427,49 @@ trampoline_bios_setup: * multiboot structure (if available) and use the smallest. */ cmp $0x100,%edx /* is the multiboot value too small? */ - jb trampoline_setup /* if so, do not use it */ + jb 2f /* 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 memory for the trampoline and the low-memory stack. */ sub $((TRAMPOLINE_SPACE+TRAMPOLINE_STACK_SPACE)>>4),%ecx /* From arch/x86/smpboot.c: start_eip had better be page-aligned! */ xor %cl, %cl - -trampoline_setup: shl $4, %ecx mov %ecx,sym_phys(trampoline_phys) - /* Get topmost low-memory stack address. */ +trampoline_setup: + mov sym_phys(trampoline_phys),%ecx + + /* Get bottom-most low-memory stack address. */ add $TRAMPOLINE_SPACE,%ecx /* Save the Multiboot info struct (after relocation) for later use. */ mov $sym_phys(cpu0_stack)+1024,%esp - push %ecx /* Topmost low-memory stack address. */ + push %ecx /* Bottom-most low-memory stack address. */ push %ebx /* Multiboot information address. */ push %eax /* Multiboot magic. */ call reloc mov %eax,sym_phys(multiboot_ptr) /* - * Now trampoline_phys points to the following structure (lowest - * address is at the top): + * Now trampoline_phys points to the following structure (lowest address + * is at the bottom): * * +------------------------+ - * | TRAMPOLINE_SPACE | - * +- - - - - - - - - - - - + - * | mbi struct | - * +------------------------+ * | TRAMPOLINE_STACK_SPACE | * +------------------------+ + * | mbi data | + * +- - - - - - - - - - - - + + * | TRAMPOLINE_SPACE | + * +------------------------+ * - * mbi struct lives at the end of TRAMPOLINE_SPACE. The rest of - * TRAMPOLINE_SPACE is reserved for trampoline code and data. + * mbi data grows downwards from the highest address of TRAMPOLINE_SPACE + * region to the end of the trampoline. The rest of TRAMPOLINE_SPACE is + * reserved for trampoline code and data. */ /* diff --git a/xen/arch/x86/boot/x86_64.S b/xen/arch/x86/boot/x86_64.S index 139b2ca..4d507fb 100644 --- a/xen/arch/x86/boot/x86_64.S +++ b/xen/arch/x86/boot/x86_64.S @@ -186,7 +186,7 @@ GLOBAL(idle_pg_table) GLOBAL(__page_tables_end) /* Init pagetables. Enough page directories to map into the bottom 1GB. */ - .section .init.data, "a", @progbits + .section .init.data, "aw", @progbits .align PAGE_SIZE, 0 GLOBAL(l2_bootmap) diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h index d193847..94418bf 100644 --- a/xen/arch/x86/efi/efi-boot.h +++ b/xen/arch/x86/efi/efi-boot.h @@ -665,7 +665,7 @@ static bool_t __init efi_arch_use_config_file(EFI_SYSTEM_TABLE *SystemTable) static void efi_arch_flush_dcache_area(const void *vaddr, UINTN size) { } -paddr_t __init efi_multiboot2(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) +void __init efi_multiboot2(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) { EFI_GRAPHICS_OUTPUT_PROTOCOL *gop; UINTN cols, gop_mode = ~0, rows; @@ -698,9 +698,6 @@ paddr_t __init efi_multiboot2(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTa efi_set_gop_mode(gop, gop_mode); efi_exit_boot(ImageHandle, SystemTable); - - /* Return trampoline address. */ - return trampoline_phys; } /* diff --git a/xen/arch/x86/efi/stub.c b/xen/arch/x86/efi/stub.c index 8df9ba2..2127cce 100644 --- a/xen/arch/x86/efi/stub.c +++ b/xen/arch/x86/efi/stub.c @@ -17,11 +17,11 @@ * efi_multiboot2() is an exception. Please look below for more details. */ -paddr_t __init noreturn efi_multiboot2(EFI_HANDLE ImageHandle, - EFI_SYSTEM_TABLE *SystemTable) +void __init noreturn efi_multiboot2(EFI_HANDLE ImageHandle, + EFI_SYSTEM_TABLE *SystemTable) { static const CHAR16 __initconst err[] = - L"(XEN) Xen does not have EFI code build in!\r\n(XEN) System halted!\r\n"; + L"Xen does not have EFI code build in!\r\nSystem halted!\r\n"; SIMPLE_TEXT_OUTPUT_INTERFACE *StdErr; StdErr = SystemTable->StdErr ? SystemTable->StdErr : SystemTable->ConOut; @@ -37,7 +37,7 @@ paddr_t __init noreturn efi_multiboot2(EFI_HANDLE ImageHandle, " call *%2 \n" "0: hlt \n" " jmp 0b \n" - : "+c" (StdErr), "+d" (err) : "rm" (StdErr->OutputString) + : "+c" (StdErr) : "d" (err), "rm" (StdErr->OutputString) : "rax", "r8", "r9", "r10", "r11", "memory"); unreachable(); diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S index addf2ef..76e18ab 100644 --- a/xen/arch/x86/xen.lds.S +++ b/xen/arch/x86/xen.lds.S @@ -334,5 +334,5 @@ ASSERT(IS_ALIGNED(trampoline_end, 4), "trampoline_end misaligned") ASSERT(IS_ALIGNED(__bss_start, 8), "__bss_start misaligned") ASSERT(IS_ALIGNED(__bss_end, 8), "__bss_end misaligned") -ASSERT((trampoline_end - trampoline_start) < TRAMPOLINE_SPACE, - "not enough room for trampoline") +ASSERT((trampoline_end - trampoline_start) < TRAMPOLINE_SPACE - MBI_SPACE_MIN, + "not enough room for trampoline and mbi data") diff --git a/xen/include/asm-x86/config.h b/xen/include/asm-x86/config.h index 9cd05a2..b9a6d94 100644 --- a/xen/include/asm-x86/config.h +++ b/xen/include/asm-x86/config.h @@ -76,6 +76,8 @@ #define TRAMPOLINE_STACK_SPACE PAGE_SIZE #define TRAMPOLINE_SPACE (KB(64) - TRAMPOLINE_STACK_SPACE) +#define MBI_SPACE_MIN (2 * PAGE_SIZE) + /* Primary stack is restricted to 8kB by guard pages. */ #define PRIMARY_STACK_SIZE 8192