From mboxrd@z Thu Jan 1 00:00:00 1970 From: Konrad Rzeszutek Wilk Subject: Re: EFI GetNextVariableName crashes when running under Xen, but not under Linux. efi-rs=0 works. No memmap issues Date: Tue, 27 Jan 2015 16:48:54 -0500 Message-ID: <20150127214854.GA2772@x230.dumpdata.com> References: <20150126162753.GA1812@l.oracle.com> <54C680C90200007800059907@mail.emea.novell.com> <20150127000247.GU3473@olila.local.net-space.pl> <54C6DCB7.3060206@citrix.com> <54C752460200007800059B8B@mail.emea.novell.com> <20150127201858.GY3473@olila.local.net-space.pl> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="45Z9DzgjV8m4Oswq" Return-path: Received: from mail6.bemta4.messagelabs.com ([85.158.143.247]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1YGE0V-00053q-0K for xen-devel@lists.xenproject.org; Tue, 27 Jan 2015 21:49:11 +0000 Content-Disposition: inline In-Reply-To: <20150127201858.GY3473@olila.local.net-space.pl> 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 , Marcos Matsunaga Cc: Andrew Cooper , Jan Beulich , xen-devel List-Id: xen-devel@lists.xenproject.org --45Z9DzgjV8m4Oswq Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Tue, Jan 27, 2015 at 09:18:58PM +0100, Daniel Kiper wrote: > On Tue, Jan 27, 2015 at 07:54:30AM +0000, Jan Beulich wrote: > > (re-adding xen-devel) > > > > >>> On 27.01.15 at 01:32, wrote: > > > On 27/01/2015 00:02, Daniel Kiper wrote: > > >> On Mon, Jan 26, 2015 at 05:00:41PM +0000, Jan Beulich wrote: > > >>>>>> On 26.01.15 at 17:27, wrote: > > >>>> Anyhow I am bit stuck: > > >>>> 1) It works with Linux, so what is it that Linux does that > > >>>> Xen does not? > > >>> They map more than just what is marked for runtime use. And they call SetVirtualAddressMap which we do not (and if I define USE_SET_VIRTUAL_ADDRESS_MAP Xen blows up during bootup). > > >> IIRC, Linux maps boot services unconditionally (and states in comment > > >> that this is not in line with spec). We do not have such mechanism. .. snip.. I've found that the issue is that the EFI firmware code assumes that if you have not called SetVirtualAddressMap then you must have not called ExitBootServices. But we do, and part of ExitBootServices job is to wipe out its system function table to zero. And since we did that - the system function table would point to zeros .. and the code would happily execute code at location 0 . The "fix" was to not call ExitBootServices. See patches - which are really just for diagnostic purposes. Daniel - thank you for suggesting that! P.S. Marcos, you might want to run with these patches (except the #4 patch) - and see how it works on your Dell machine (without the efi-rs=0 workaround). For those that are interested, here is the heavily annotated efi_rs->GetNextVariableName code on this Lenovo Thinkpad along with snippets from memory: The first [] is when running under Linux, the second is when running under Xen. The [S] means it had the same value when running w/ calling ExitBootServices under Xen. 0: 48 89 5c 24 08 mov %rbx,0x8(%rsp) 5: 48 89 6c 24 10 mov %rbp,0x10(%rsp) a: 48 89 74 24 18 mov %rsi,0x18(%rsp) f: 57 push %rdi 10: 41 54 push %r12 12: 41 55 push %r13 14: 48 83 ec 20 sub $0x20,%rsp 18: 45 33 ed xor %r13d,%r13d 1b: 48 85 c9 test %rcx,%rcx 1e: 4d 8b e0 mov %r8,%r12 21: 48 8b fa mov %rdx,%rdi 24: 48 8b e9 mov %rcx,%rbp 27: 0f 84 09 01 00 00 je 0x136 2d: 48 85 d2 test %rdx,%rdx 30: 0f 84 00 01 00 00 je 0x136 36: 4d 85 c0 test %r8,%r8 39: 0f 84 f7 00 00 00 je 0x136 3f: 48 8b 05 76 11 00 00 mov 0x1176(%rip),%rax # 0x11bc [20 53 c3 fa fe ff ff ff] [20 53 a3 d6 00 00 00 00][S] 46: 48 8d 15 af 11 00 00 lea 0x11af(%rip),%rdx # 0x11fc [00 54 f3 41 60 06 1c 8] [00 6d 15 d8 d6 db 40 8d][S] 4d: 48 8b c8 mov %rax,%rcx 50: ff 50 20 callq *0x20(%rax) 53: 80 3d a2 11 00 00 01 cmpb $0x1,0x11a2(%rip) # 0x11fc [00 54 f3 41 60 06 1c 8] [00 6d 15 d8 d6 db 40 8d][S] 5a: 75 1b jne 0x77 5c: 48 8b 05 81 11 00 00 mov 0x1181(%rip),%rax # 0x11e4 [80 62 2b db 00 00 00 00] [80 62 2b db 00 00 00 0][S] 63: 4d 8b c4 mov %r12,%r8 66: 48 8b d7 mov %rdi,%rdx 69: 48 8b cd mov %rbp,%rcx 6c: ff 50 08 callq *0x8(%rax) 6f: 48 8b d8 mov %rax,%rbx 72: e9 ba 00 00 00 jmpq 0x131 77: 48 8b cf mov %rdi,%rcx 7a: e8 bd 0f 00 00 callq 0x103c 7f: 48 3d 00 01 00 00 cmp $0x100,%rax 85: 0f 87 ab 00 00 00 ja 0x136 8b: 44 38 2d c2 10 00 00 cmp %r13b,0x10c2(%rip) # 0x1154 [01 01 00 00 00 00 00 00] [00 01 00 00 00 00 00 00][S] [Here we figure out whether to use BootServices. r13 is zero (see @18)] 92: 75 12 jne 0xa6 [Linux: 01 != 00, hence go to @a6, Xen keeps on going] 94: 48 8b 05 d1 10 00 00 mov 0x10d1(%rip),%rax # 0x116c [70 a2 db cf 00 00 00 00][70 a2 db cf 00 00 00 00][S] 9b: b9 1f 00 00 00 mov $0x1f,%ecx w/o ExitBootServices: [@cfdba270: 42 4f 4f 54 53 45 52 56] [@cfdba270+0x18: 48 30 dc cf 00 00 00 00] [@0: 68 02 00 f0 68 02 00 f0 6].. w/ ExitBootServices they [@cfdba270: 00 00 00 ...] [@cfdba270+0x18: 00 00 00 ..] [@0: 68 02 00 f0 68 02 00 f0 6].. a0: ff 50 18 callq *0x18(%rax) .. and the rest is unintersting - as right now Xen would crash when calling code at @0 which ends is full of garbage. If however we did not call ExitBootServices, we jump to cfdc3048 which is: 0: 48 89 5c 24 08 mov %rbx,0x8(%rsp) 5: 57 push %rdi 6: 48 83 ec 20 sub $0x20,%rsp a: 48 8b 1d 57 81 ff ff mov -0x7ea9(%rip),%rbx # 0xffffffffffff8168 11: 48 8b f9 mov %rcx,%rdi 14: 48 3b cb cmp %rbx,%rcx 17: 72 1a jb 0x33 19: 48 83 f9 1f cmp $0x1f,%rcx 1d: 72 0d jb 0x2c 1f: 48 83 fb 1f cmp $0x1f,%rbx 23: 73 07 jae 0x2c 25: 33 c9 xor %ecx,%ecx 27: e8 9c ff ff ff callq 0xffffffffffffffc8 2c: 48 89 3d 35 81 ff ff mov %rdi,-0x7ecb(%rip) # 0xffffffffffff8168 33: 48 8b c3 mov %rbx,%rax 36: 48 8b 5c 24 30 mov 0x30(%rsp),%rbx 3b: 48 83 c4 20 add $0x20,%rsp 3f: 5f pop %rdi 40: c3 retq a3: 4c 8b e8 mov %rax,%r13 a6: 48 8b 35 27 11 00 00 mov 0x1127(%rip),%rsi # 0x11d4 [10 d0 87 fa fe ff ff ff][10 d0 47 da 00 00 00 00][S] ad: 48 8b d7 mov %rdi,%rdx b0: c6 06 5a movb $0x5a,(%rsi) b3: c6 46 01 6b movb $0x6b,0x1(%rsi) b7: 48 8b 4d 00 mov 0x0(%rbp),%rcx bb: 48 89 4e 18 mov %rcx,0x18(%rsi) bf: 48 8d 4e 20 lea 0x20(%rsi),%rcx c3: e8 58 0f 00 00 callq 0x1020 c8: 48 8d 8e 20 02 00 00 lea 0x220(%rsi),%rcx cf: 41 b8 10 00 00 00 mov $0x10,%r8d d5: 49 8b d4 mov %r12,%rdx d8: e8 ff 0e 00 00 callq 0xfdc dd: e8 3a fb ff ff callq 0xfffffffffffffc1c e2: 44 8a 1e mov (%rsi),%r11b e5: 41 80 fb 5a cmp $0x5a,%r11b e9: 74 bb je 0xa6 eb: 48 8b 5e 08 mov 0x8(%rsi),%rbx ef: 48 8b 46 18 mov 0x18(%rsi),%rax f3: 48 85 db test %rbx,%rbx f6: 48 89 45 00 mov %rax,0x0(%rbp) fa: 75 1f jne 0x11b fc: 48 8d 56 20 lea 0x20(%rsi),%rdx 100: 48 8b cf mov %rdi,%rcx 103: e8 18 0f 00 00 callq 0x1020 108: 48 8d 96 20 02 00 00 lea 0x220(%rsi),%rdx 10f: 44 8d 43 10 lea 0x10(%rbx),%r8d 113: 49 8b cc mov %r12,%rcx 116: e8 c1 0e 00 00 callq 0xfdc 11b: 80 3d 32 10 00 00 00 cmpb $0x0,0x1032(%rip) # 0x1154 [70 a2 db cf 00 00 00 00][00 01 00 00 00 00 00 00][S] 122: 75 0d jne 0x131 124: 48 8b 05 41 10 00 00 mov 0x1041(%rip),%rax # 0x116c [70 a2 db cf 00 00 00 00][70 a2 db cf 00 00 00 00][S] 12b: 49 8b cd mov %r13,%rcx 12e: ff 50 20 callq *0x20(%rax) 131: 48 8b c3 mov %rbx,%rax 134: eb 0a jmp 0x140 136: 48 b8 02 00 00 00 00 movabs $0x8000000000000002,%rax 13d: 00 00 80 140: 48 8b 5c 24 40 mov 0x40(%rsp),%rbx 145: 48 8b 6c 24 48 mov 0x48(%rsp),%rbp 14a: 48 8b 74 24 50 mov 0x50(%rsp),%rsi 14f: 48 83 c4 20 add $0x20,%rsp 153: 41 5d pop %r13 155: 41 5c pop %r12 157: 5f pop %rdi 158: c3 retq 159: cc int3 15a: cc int3 15b: cc int3 --45Z9DzgjV8m4Oswq Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename="0001-EFI-Map-also-BootServicesData-and-BootServicesCode.patch" >>From 49aab3d6e623c907164475403ba495ede442e6e5 Mon Sep 17 00:00:00 2001 From: Konrad Rzeszutek Wilk Date: Tue, 27 Jan 2015 15:32:10 -0500 Subject: [PATCH 1/4] EFI: Map also BootServicesData and BootServicesCode Signed-off-by: Konrad Rzeszutek Wilk --- xen/arch/x86/efi/efi-boot.h | 2 -- xen/common/efi/boot.c | 27 ++++++++++++++++++++++++--- 2 files changed, 24 insertions(+), 5 deletions(-) diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h index 3a3b4fe..c3bdb8d 100644 --- a/xen/arch/x86/efi/efi-boot.h +++ b/xen/arch/x86/efi/efi-boot.h @@ -152,8 +152,6 @@ static void __init efi_arch_process_memory_map(EFI_SYSTEM_TABLE *SystemTable, type = E820_RESERVED; break; case EfiConventionalMemory: - case EfiBootServicesCode: - case EfiBootServicesData: if ( !trampoline_phys && desc->PhysicalStart + len <= 0x100000 && len >= cfg.size && desc->PhysicalStart + len > cfg.addr ) cfg.addr = (desc->PhysicalStart + len - cfg.size) & PAGE_MASK; diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c index c04e0a2..b0bbc4b 100644 --- a/xen/common/efi/boot.c +++ b/xen/common/efi/boot.c @@ -1156,13 +1156,23 @@ void __init efi_init_memory(void) u64 len = desc->NumberOfPages << EFI_PAGE_SHIFT; unsigned long smfn, emfn; unsigned int prot = PAGE_HYPERVISOR; + unsigned int skip = 1; printk(XENLOG_INFO " %013" PRIx64 "-%013" PRIx64 " type=%u attr=%016" PRIx64 "\n", desc->PhysicalStart, desc->PhysicalStart + len - 1, desc->Type, desc->Attribute); - if ( !efi_rs_enable || !(desc->Attribute & EFI_MEMORY_RUNTIME) ) + if ( desc->Attribute & EFI_MEMORY_RUNTIME ) + skip = 0; + + if ( desc->Type == 4 && desc->Attribute != 0 ) + skip = 0; + + if ( desc->Type == 3 && desc->Attribute != 0 ) + skip = 0; + + if ( !efi_rs_enable || skip ) { printk(XENLOG_INFO " .. skipped!\n"); continue; @@ -1246,18 +1256,29 @@ void __init efi_init_memory(void) copy_mapping(0, max_page, ram_range_valid); + printk(XENLOG_INFO "Copying..\n"); /* Insert non-RAM runtime mappings inside the direct map. */ for ( i = 0; i < efi_memmap_size; i += efi_mdesc_size ) { const EFI_MEMORY_DESCRIPTOR *desc = efi_memmap + i; - if ( (desc->Attribute & EFI_MEMORY_RUNTIME) && + if ( ((desc->Attribute & EFI_MEMORY_RUNTIME) || + (desc->Type == 3 && desc->Attribute != 0 ) || + (desc->Type == 4 && desc->Attribute != 0 )) && desc->VirtualStart != INVALID_VIRTUAL_ADDRESS && - desc->VirtualStart != desc->PhysicalStart ) + desc->VirtualStart != desc->PhysicalStart ) { + + printk(XENLOG_INFO " %013" PRIx64 "-%013" PRIx64 + " type=%u attr=%016" PRIx64 "\n", + PFN_DOWN(desc->PhysicalStart), + PFN_UP(desc->PhysicalStart + (desc->NumberOfPages << EFI_PAGE_SHIFT)), + desc->Type, desc->Attribute); + copy_mapping(PFN_DOWN(desc->PhysicalStart), PFN_UP(desc->PhysicalStart + (desc->NumberOfPages << EFI_PAGE_SHIFT)), rt_range_valid); + } } /* Insert non-RAM runtime mappings outside of the direct map. */ -- 2.1.0 --45Z9DzgjV8m4Oswq Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename="0002-EFI-early-Implement-GetNextVariableName-and-query-an.patch" >>From ffb09e4938021ed47bc7097585dd19a10f2e09de Mon Sep 17 00:00:00 2001 From: Konrad Rzeszutek Wilk Date: Tue, 27 Jan 2015 14:04:30 -0500 Subject: [PATCH 2/4] EFI/early: Implement GetNextVariableName and /query and /noexitboot parameters In the early EFI boot we will enumerate up to five EFI variables to make sure it works. The /query will just enumerate them and then quit. Helps in troubleshooting and redirecting the output to a file (xen.efi /query > q). The /noexitboot will inhibit Xen from calling ExitBootServices. This allows on Lenovo ThinkPad x230 to use GetNextVariableName in 1-1 mapping mode. Signed-off-by: Konrad Rzeszutek Wilk --- xen/common/efi/boot.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 82 insertions(+), 1 deletion(-) diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c index b0bbc4b..4ee8f68 100644 --- a/xen/common/efi/boot.c +++ b/xen/common/efi/boot.c @@ -673,6 +673,74 @@ static void __init setup_efi_pci(void) efi_bs->FreePool(handles); } +static int __init efi_getnextvariable(bool_t query_only) +{ + EFI_GUID guid; + UINTN i, size, idx; + CHAR16 *name; + EFI_STATUS status; + + + status = efi_bs->AllocatePool(EfiLoaderData, 1024, (void *)&name); + if ( EFI_ERROR(status) ) + return status; + + guid.Data1 = 0; + guid.Data2 = 0; + guid.Data3 = 0; + for ( i = 0; i < 8; i++ ) + guid.Data4[i] = 0; + + for ( i = 0 ; i < 1024 / sizeof (CHAR16); i++ ) + name[i] = 0; + + PrintStr(L"GetNextVariableName: "); + PrintStr(name); + PrintStr(newline); + idx = 0; + do { + size = 1024; + efi_rs->GetNextVariableName(&size, name, &guid); + if ( EFI_ERROR(status) ) + { + if ( query_only ) + { + efi_bs->FreePool(name); + PrintErrMesg(L"Failed to GetNextVariableName", status); + } + else + { + PrintStr(L"Warning: GetNextVariableName: "); + DisplayUint(status, 0); + PrintStr(newline); + } + } + else + { + DisplayUint(guid.Data1, 4); + DisplayUint(guid.Data2, 2); + DisplayUint(guid.Data3, 2); + for ( i = 0; i < 8; i++ ) + DisplayUint(guid.Data4[i], 1); + + PrintStr(L":"); + PrintStr(name); + PrintStr(L" "); + DisplayUint(status, 0); + PrintStr(newline); + } + } while ( !EFI_ERROR(status) && idx++ < 5 ); + + efi_bs->FreePool(name); + + if ( query_only ) + return -EINVAL; + + if ( EFI_ERROR(EFI_NOT_FOUND) ) + return 0; + + return status; +} static int __init __maybe_unused set_color(u32 mask, int bpp, u8 *pos, u8 *sz) { if ( bpp < 0 ) @@ -705,8 +773,10 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) EFI_GRAPHICS_OUTPUT_MODE_INFORMATION *mode_info; union string section = { NULL }, name; bool_t base_video = 0, retry; + bool_t query_only = 0; char *option_str; bool_t use_cfg_file; + bool_t exit_boot_services = 0; efi_ih = ImageHandle; efi_bs = SystemTable->BootServices; @@ -751,6 +821,10 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) { if ( wstrcmp(ptr + 1, L"basevideo") == 0 ) base_video = 1; + else if ( wstrcmp(ptr + 1, L"query") == 0 ) + query_only = 1; + else if ( wstrcmp(ptr + 1, L"noexitboot") == 0 ) + exit_boot_services = 0; else if ( wstrncmp(ptr + 1, L"cfg=", 4) == 0 ) cfg_file_name = ptr + 5; else if ( i + 1 < argc && wstrcmp(ptr + 1, L"cfg") == 0 ) @@ -1031,6 +1105,9 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) PrintStr(newline); } + if ( efi_getnextvariable(query_only) ) + blexit(L"Could not get next variable"); + efi_arch_memory_setup(); if ( gop ) @@ -1064,7 +1141,11 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) efi_arch_pre_exit_boot(); - status = efi_bs->ExitBootServices(ImageHandle, map_key); + if ( exit_boot_services ) + status = efi_bs->ExitBootServices(ImageHandle, map_key); + else + status = 0; + if ( status != EFI_INVALID_PARAMETER || retry ) break; } -- 2.1.0 --45Z9DzgjV8m4Oswq Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename="0003-EFI-early-Swap-noexitboot-to-exitboot-and-by-default.patch" >>From 76bee287621d22b2c9082eae6eacfea2446722c9 Mon Sep 17 00:00:00 2001 From: Konrad Rzeszutek Wilk Date: Tue, 27 Jan 2015 15:35:35 -0500 Subject: [PATCH 3/4] EFI/early: Swap noexitboot to exitboot and by default don't call ExitBootServices. Signed-off-by: Konrad Rzeszutek Wilk --- xen/common/efi/boot.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c index 4ee8f68..d9aabd3 100644 --- a/xen/common/efi/boot.c +++ b/xen/common/efi/boot.c @@ -823,8 +823,8 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) base_video = 1; else if ( wstrcmp(ptr + 1, L"query") == 0 ) query_only = 1; - else if ( wstrcmp(ptr + 1, L"noexitboot") == 0 ) - exit_boot_services = 0; + else if ( wstrcmp(ptr + 1, L"exitboot") == 0 ) + exit_boot_services = 1; else if ( wstrncmp(ptr + 1, L"cfg=", 4) == 0 ) cfg_file_name = ptr + 5; else if ( i + 1 < argc && wstrcmp(ptr + 1, L"cfg") == 0 ) @@ -834,6 +834,8 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) { PrintStr(L"Xen EFI Loader options:\r\n"); PrintStr(L"-basevideo retain current video mode\r\n"); + PrintStr(L"-exitboot call ExitBootServices\r\n"); + PrintStr(L"-query call GetNextVariableName for up to five times\r\n"); PrintStr(L"-cfg= specify configuration file\r\n"); PrintStr(L"-help, -? display this help\r\n"); blexit(NULL); -- 2.1.0 --45Z9DzgjV8m4Oswq Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename="0004-EFI-Dump-0xcfda270-and-the-other-address.patch" >>From 9f7469cd3b8a89594b16f2e39886948987bfa2ae Mon Sep 17 00:00:00 2001 From: Konrad Rzeszutek Wilk Date: Tue, 27 Jan 2015 16:09:51 -0500 Subject: [PATCH 4/4] EFI: Dump 0xcfda270 and the other address Signed-off-by: Konrad Rzeszutek Wilk --- xen/common/efi/runtime.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/xen/common/efi/runtime.c b/xen/common/efi/runtime.c index ef6b1df..79e8072 100644 --- a/xen/common/efi/runtime.c +++ b/xen/common/efi/runtime.c @@ -236,7 +236,12 @@ long __init efi_debug(void) printk(", GetNextVariableName: %lx\n", get); _dumpcode(get, get+1024); - + get = 0xcfdba270; + _dumpcode(get, get+8); + _dumpcode(get + 0x18, get + 0x18 + 8); + _dumpcode(get + 0x20, get + 0x20 + 8); + get = 0xcfdc3048; + _dumpcode(get, get+1024); idx = 1; do { printk("%4d:", idx++); -- 2.1.0 --45Z9DzgjV8m4Oswq Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel --45Z9DzgjV8m4Oswq--