Re: crash in efi_runtime_call

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

[-- Attachment #1: Type: text/plain, Size: 2681 bytes --]

On Fri, Apr 17, 2015 at 01:54:28PM +0100, Andrew Cooper wrote:
> On 17/04/15 13:39, Jan Beulich wrote:
> >>>> On 17.04.15 at 13:59, <andrew.cooper3@citrix.com> wrote:
> >> On 17/04/15 12:17, Olaf Hering wrote:
> >>> Since booting xen fails on my ProBook unless I specify "maxcpus=1" I
> >>> tried the EFI firmware today. To my surprise it boots and finds all
> >>> cpus. But once some efi driver in dom0 is loaded xen crashes. The same
> >>> happens with xen-4.4 as included in SLE12.
> >>>
> >>> ...
> >>> (XEN) Xen call trace:
> >>> (XEN)    [<00000000aec1e8e1>] 00000000aec1e8e1
> >>> (XEN)    [<ffff82d080222600>] efi_runtime_call+0x7f0/0x890
> >>> (XEN)    [<ffff82d0801641a9>] do_platform_op+0x679/0x1670
> >>> (XEN)    [<ffff82d08021dfb9>] syscall_enter+0xa9/0xae
> >>> ....
> >>>
> >>> Can I do anything about it, or is this a firmware bug? I will move the
> >>> offending efi driver away and try again.
> >>>
> >>> Olaf
> >> This is a firmware bug.
> > +1 (and I'm surprised how common this is)
> 
> The bug is present in the reference implementation code, which means it
> is present in a lot of real firmware.  We have kit from 3 different
> vendors which are affected, including latest available firmware.
> 
> >
> >>> (XEN)  0000100000000-000023fffffff type=7 attr=000000000000000
> >>> (XEN)  00000fec10000-00000fec10fff type=11 attr=8000000000000001
> >>> (XEN)  00000fff40000-00000fff46fff type=11 attr=8000000000000000
> >>> (XEN) Unknown cachability for MFNs 0xfff40-0xfff46
> >> This unknown cacheability causes Xen not to make pagetables for the region.
> >>
> >> There is a patch or two floating around the list, but currently no
> >> resolution on the argument it created.
> >>
> >> https://github.com/xenserver/xen-4.5.pg/blob/master/master/unknown-cacheabilit 
> >> y.patch
> >> is the XenServer fix.
> > Now that's surely wrong
> 
> Right or wrong, this is (apparently; I have not checked) what Linux does.
> 
> >  - if anything, unknown should be treated as
> > UC (and quite likely specifically in a case like the one Olaf reports here,
> > as the offending memory range pretty likely is other than normal RAM).
> > What I'd accept as a patch would be the addition of a command line
> > option enforcing the mapping of such unknown cacheability areas with
> > a certain caching type (default then being UC).
> 
> If I can find some copious free time, I will see about making this happen.

I actually did cobble a patch like this, but it is based on Daniel's Multibootv2
so it won't apply cleany. See attached patchset with various 'work-arounds'.

Jan if you are OK with them (well the 'idea' behind them) I can refresh
it against staging and post them?

[-- Attachment #2: 0001-EFI-early-Implement-noexit-to-not-ExitBootServices.patch --]
[-- Type: text/plain, Size: 4177 bytes --]

>From 33badf8e314251e9d9c3b768c0b7a34b225aa45c Mon Sep 17 00:00:00 2001
From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Date: Tue, 3 Feb 2015 11:56:33 -0500
Subject: [PATCH 1/3] EFI/early: Implement /noexit to not ExitBootServices

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 <konrad.wilk@oracle.com>
---
 xen/arch/x86/efi/efi-boot.h |  2 +-
 xen/common/efi/boot.c       | 15 +++++++++++----
 2 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
index f50c10a..0fbc4de 100644
--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -676,7 +676,7 @@ void __init efi_multiboot2(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable
     setup_efi_pci();
     efi_variables();
     efi_set_gop_mode(gop, gop_mode);
-    efi_exit_boot(ImageHandle, SystemTable);
+    efi_exit_boot(ImageHandle, SystemTable, 0);
 }
 
 /*
diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index d1d06d7..2389a1a 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -86,7 +86,7 @@ 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 void efi_exit_boot(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable, int exit_boot_services);
 
 static const EFI_BOOT_SERVICES *__initdata efi_bs;
 static EFI_HANDLE __initdata efi_ih;
@@ -882,7 +882,7 @@ static void __init efi_set_gop_mode(EFI_GRAPHICS_OUTPUT_PROTOCOL *gop, UINTN gop
         efi_arch_video_init(gop, info_size, mode_info);
 }
 
-static void __init efi_exit_boot(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
+static void __init efi_exit_boot(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable, int exit_boot_services)
 {
     EFI_STATUS status;
     UINTN map_key;
@@ -906,7 +906,10 @@ static void __init efi_exit_boot(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *Syste
 
         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;
     }
@@ -1023,6 +1026,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
     char *option_str;
     bool_t use_cfg_file;
     bool_t query_only = 0;
+    bool_t exit_boot_services = 1;
 
 #ifndef CONFIG_ARM /* TODO - disabled until implemented on ARM */
     efi_platform = 1;
@@ -1065,6 +1069,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"noexit") == 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 )
@@ -1076,6 +1082,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
                     PrintStr(L"-basevideo   retain current video mode\r\n");
                     PrintStr(L"-cfg=<file>  specify configuration file\r\n");
                     PrintStr(L"-query       GetNextVariableName up to five\r\n");
+                    PrintStr(L"-noexit      Don't call ExitBootServices\r\n");
                     PrintStr(L"-help, -?    display this help\r\n");
                     blexit(NULL);
                 }
@@ -1242,7 +1249,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
 
     efi_set_gop_mode(gop, gop_mode);
 
-    efi_exit_boot(ImageHandle, SystemTable);
+    efi_exit_boot(ImageHandle, SystemTable, exit_boot_services);
 
     efi_arch_post_exit_boot();
     for( ; ; ); /* not reached */
-- 
2.1.0


[-- Attachment #3: 0002-EFI-early-Implement-map-to-map-EfiBootServicesData-a.patch --]
[-- Type: text/plain, Size: 9408 bytes --]

>From e3048d6500ace943c8ce6ee52e00ee832615b6ab Mon Sep 17 00:00:00 2001
From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Date: Tue, 3 Feb 2015 15:55:13 -0500
Subject: [PATCH 2/3] EFI/early: Implement /map to map EfiBootServicesData and
 Code

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 xen/arch/x86/efi/efi-boot.h | 12 +++++++++---
 xen/common/efi/boot.c       | 43 ++++++++++++++++++++++++++++++++++---------
 xen/common/efi/efi.h        |  2 +-
 xen/common/efi/runtime.c    |  1 +
 4 files changed, 45 insertions(+), 13 deletions(-)

diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
index 0fbc4de..346f273 100644
--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -159,7 +159,8 @@ static void __init efi_arch_process_memory_map(EFI_SYSTEM_TABLE *SystemTable,
                                                void *map,
                                                UINTN map_size,
                                                UINTN desc_size,
-                                               UINT32 desc_ver)
+                                               UINT32 desc_ver,
+                                               UINT32 map_bootservices)
 {
     struct e820entry *e;
     unsigned int i;
@@ -177,9 +178,14 @@ static void __init efi_arch_process_memory_map(EFI_SYSTEM_TABLE *SystemTable,
         default:
             type = E820_RESERVED;
             break;
-        case EfiConventionalMemory:
         case EfiBootServicesCode:
         case EfiBootServicesData:
+            if ( map_bootservices )
+            {
+                type = E820_RESERVED;
+                break;
+            }
+        case EfiConventionalMemory:
             if ( !trampoline_phys && desc->PhysicalStart + len <= 0x100000 &&
                  len >= cfg.size && desc->PhysicalStart + len > cfg.addr )
                 cfg.addr = (desc->PhysicalStart + len - cfg.size) & PAGE_MASK;
@@ -676,7 +682,7 @@ void __init efi_multiboot2(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable
     setup_efi_pci();
     efi_variables();
     efi_set_gop_mode(gop, gop_mode);
-    efi_exit_boot(ImageHandle, SystemTable, 0);
+    efi_exit_boot(ImageHandle, SystemTable, 0, 0);
 }
 
 /*
diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index 2389a1a..ec98914 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -86,7 +86,7 @@ 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, int exit_boot_services);
+static void efi_exit_boot(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable, int exit_boot_services, int map_bs);
 
 static const EFI_BOOT_SERVICES *__initdata efi_bs;
 static EFI_HANDLE __initdata efi_ih;
@@ -882,7 +882,7 @@ static void __init efi_set_gop_mode(EFI_GRAPHICS_OUTPUT_PROTOCOL *gop, UINTN gop
         efi_arch_video_init(gop, info_size, mode_info);
 }
 
-static void __init efi_exit_boot(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable, int exit_boot_services)
+static void __init efi_exit_boot(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable, int exit_boot_services, int map_bs)
 {
     EFI_STATUS status;
     UINTN map_key;
@@ -902,7 +902,7 @@ static void __init efi_exit_boot(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *Syste
             PrintErrMesg(L"Cannot obtain memory map", status);
 
         efi_arch_process_memory_map(SystemTable, efi_memmap, efi_memmap_size,
-                                    efi_mdesc_size, mdesc_ver);
+                                    efi_mdesc_size, mdesc_ver, map_bs);
 
         efi_arch_pre_exit_boot();
 
@@ -924,6 +924,7 @@ static void __init efi_exit_boot(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *Syste
 #endif
     efi_memmap = (void *)efi_memmap + DIRECTMAP_VIRT_START;
     efi_fw_vendor = (void *)efi_fw_vendor + DIRECTMAP_VIRT_START;
+    efi_map = map_bs;
 }
 
 static int __init efi_getnextvariable(bool_t query_only)
@@ -1027,7 +1028,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
     bool_t use_cfg_file;
     bool_t query_only = 0;
     bool_t exit_boot_services = 1;
-
+    bool_t map_bs = 0;
 #ifndef CONFIG_ARM /* TODO - disabled until implemented on ARM */
     efi_platform = 1;
     efi_loader = 1;
@@ -1071,6 +1072,8 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
                     query_only = 1;
                 else if ( wstrcmp(ptr + 1, L"noexit") == 0 )
                     exit_boot_services = 0;
+                else if ( wstrcmp(ptr + 1, L"map") == 0 )
+                    map_bs = 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 )
@@ -1082,6 +1085,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
                     PrintStr(L"-basevideo   retain current video mode\r\n");
                     PrintStr(L"-cfg=<file>  specify configuration file\r\n");
                     PrintStr(L"-query       GetNextVariableName up to five\r\n");
+                    PrintStr(L"-map         map EfiBootServices Code and Data\r\n");
                     PrintStr(L"-noexit      Don't call ExitBootServices\r\n");
                     PrintStr(L"-help, -?    display this help\r\n");
                     blexit(NULL);
@@ -1249,7 +1253,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
 
     efi_set_gop_mode(gop, gop_mode);
 
-    efi_exit_boot(ImageHandle, SystemTable, exit_boot_services);
+    efi_exit_boot(ImageHandle, SystemTable, exit_boot_services, map_bs);
 
     efi_arch_post_exit_boot();
     for( ; ; ); /* not reached */
@@ -1325,20 +1329,30 @@ void __init efi_init_memory(void)
         return;
 #endif
 
-    printk(XENLOG_INFO "EFI memory map:\n");
+    printk(XENLOG_INFO "EFI memory map: %s\n", efi_map ? "(mapping BootServices)" : "");
     for ( i = 0; i < efi_memmap_size; i += efi_mdesc_size )
     {
         EFI_MEMORY_DESCRIPTOR *desc = efi_memmap + i;
         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 && efi_map )
+            skip = 0;
+
+        if ( desc->Type == 3 && desc->Attribute != 0 && efi_map )
+            skip = 0;
+
+        if ( !efi_rs_enable || skip )
         {
             printk(XENLOG_INFO " .. skipped!\n");
             continue;
@@ -1422,18 +1436,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 && efi_map ) ||
+             (desc->Type == 4 && desc->Attribute != 0 && efi_map ))) &&
              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. */
diff --git a/xen/common/efi/efi.h b/xen/common/efi/efi.h
index c557104..6267020 100644
--- a/xen/common/efi/efi.h
+++ b/xen/common/efi/efi.h
@@ -20,7 +20,7 @@ struct efi_pci_rom {
 extern unsigned int efi_num_ct;
 extern const EFI_CONFIGURATION_TABLE *efi_ct;
 
-extern unsigned int efi_version, efi_fw_revision;
+extern unsigned int efi_version, efi_fw_revision, efi_map;
 extern const CHAR16 *efi_fw_vendor;
 
 extern const EFI_RUNTIME_SERVICES *efi_rs;
diff --git a/xen/common/efi/runtime.c b/xen/common/efi/runtime.c
index 4ca25c2..e1709fd 100644
--- a/xen/common/efi/runtime.c
+++ b/xen/common/efi/runtime.c
@@ -27,6 +27,7 @@ const EFI_CONFIGURATION_TABLE *__read_mostly efi_ct;
 
 unsigned int __read_mostly efi_version;
 unsigned int __read_mostly efi_fw_revision;
+unsigned int __read_mostly efi_map;
 const CHAR16 *__read_mostly efi_fw_vendor;
 
 const EFI_RUNTIME_SERVICES *__read_mostly efi_rs;
-- 
2.1.0


[-- Attachment #4: 0003-efi-Implement-efi-attr-uc-1-Xen-command-line-to-map-.patch --]
[-- Type: text/plain, Size: 1877 bytes --]

>From 9e7d26786c006f7fa5eb8165b4fad182f75f988e Mon Sep 17 00:00:00 2001
From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Date: Tue, 3 Feb 2015 15:57:54 -0500
Subject: [PATCH 3/3] efi: Implement 'efi-attr-uc=1' Xen command line to map
 Runtime services with no attribute.

For example on Dell machines we see:

(XEN)  00000fed18000-00000fed19fff type=11 attr=8000000000000000
(XEN) Unknown cachability for MFNs 0xfed18-0xfed19

Lets allow them to be mapped as UC with said attribute.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 xen/common/efi/boot.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index ec98914..724f75c 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -1264,6 +1264,9 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
 static bool_t __initdata efi_rs_enable = 1;
 boolean_param("efi-rs", efi_rs_enable);
 
+static bool_t __initdata efi_attr_uc = 1;
+boolean_param("efi-attr-uc", efi_attr_uc);
+
 #ifndef USE_SET_VIRTUAL_ADDRESS_MAP
 static __init void copy_mapping(unsigned long mfn, unsigned long end,
                                 bool_t (*is_valid)(unsigned long smfn,
@@ -1373,9 +1376,12 @@ void __init efi_init_memory(void)
             prot |= _PAGE_PWT | _PAGE_PCD | MAP_SMALL_PAGES;
         else
         {
-            printk(XENLOG_ERR "Unknown cachability for MFNs %#lx-%#lx\n",
-                   smfn, emfn - 1);
-            continue;
+            printk(XENLOG_ERR "Unknown cachability for MFNs %#lx-%#lx %s\n",
+                   smfn, emfn - 1, efi_attr_uc ? "assuming UC" : "");
+            if ( efi_attr_uc )
+                prot |= _PAGE_PWT | _PAGE_PCD | MAP_SMALL_PAGES;
+            else
+                continue;
         }
 
         if ( desc->Attribute & EFI_MEMORY_WP )
-- 
2.1.0


[-- Attachment #5: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel