Re: [Xen-devel] EFI GetNextVariableName crashes when running under Xen, but not under Linux. efi-rs=0 works. No memmap issues

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

On Tue, Jan 27, 2015 at 04:17:05PM +0000, Jan Beulich wrote:
> >>> On 27.01.15 at 15:26, <konrad.wilk@xxxxxxxxxx> 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, <andrew.cooper3@xxxxxxxxxx> 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, <konrad.wilk@xxxxxxxxxx> 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.
> >> >> IIRC, Linux maps boot services unconditionally (and states in comment
> >> >> that this is not in line with spec). We do not have such mechanism.
> >> >> Could we ease life of our users and add a boot option (e.g. map-efi-bs)
> >> >> which will enforce mapping of BS regions on platforms with buggy 
> >> >> EFI/UEFI
> >> >> implementations? We should not penalize owners of such hardware because
> >> >> they are not guilty of these crazy bugs. We should educate firmware 
> >> >> devs...
> >> >> Ehh... Please, do not curse at me. I remember discussion about EFI reset
> >> >> stuff which happened here a few days ago.
> >> > 
> >> > While, in principle, I would like to take a tough stand against buggy
> >> > firmware, the truth is that firmware is always going to be buggy, and
> >> > many users are going to be in a position where their buggy firmware is
> >> > not going to be fixed by their vendors.  Much as I would prefer not to,
> >> > I feel that the only rational course of action to take is to behave like
> >> > Linux in cases like this.
> >> > 
> >> > Therefore, I am a begrudgingly +1 "work around EFI firmware bugs",
> >> > despite it being the wrong pragmatic thing to do.
> >> 
> >> And I agree that we will need to accept in such workarounds. But
> >> two remarks to whoever is going to implement it: We already have
> >> the efi-rs workaround option - we should deprecate that one, and
> >> have a consolidated efi= one instead, covering the case here too.
> >> Plus the issue here is not just a matter of mapping BS memory, but
> >> also not making it available to the allocator. That in turn may yield
> >> problems with the conversion of the EFI memory map to E820 form,
> >> both because of the number of entries needed, and because that
> >> conversion happens _before_ the normal command line parsing.
> > 
> > Twisty maze. 
> > 
> > However even with my 'debug' patch and mapping the boot services
> > it still fails on this laptop. So I fear there is something more
> > to my woes with Lenovo's EFI firmware implementation.
> 
> Again - apart from mapping the range, did you also make sure it
> didn't get passed to the allocator (and hence couldn't have got
> overwritten)?

Yes, see patch:

Also see attached of the code with what Linux sees and what Xen sees
(Linux first). I am thinking that the firmware is under the assumption
that if SetVirtualAddressMap is not called then you MUST be still
before ExitBootServices has been called. Going to verify that by
implementing an GetNextVariableName before calling ExitBootServices)

diff --git a/xen/Rules.mk b/xen/Rules.mk
index b4315a5..6692242 100644
--- a/xen/Rules.mk
+++ b/xen/Rules.mk
@@ -7,10 +7,10 @@ verbose       ?= y
 perfc         ?= n
 perfc_arrays  ?= n
 lock_profile  ?= n
-crash_debug   ?= y
-frame_pointer ?= y
+crash_debug   ?= n
+frame_pointer ?= n
 lto           ?= n
-debug := y
+debug := n
 
 include $(XEN_ROOT)/Config.mk
 
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 c11b572..e7c939e 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -1159,17 +1221,27 @@ 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) )
-       {
-           printk(XENLOG_INFO " .. skipped!\n");
+        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;
-       }
+        }
         desc->VirtualStart = INVALID_VIRTUAL_ADDRESS;
 
         smfn = PFN_DOWN(desc->PhysicalStart);
@@ -1246,18 +1318,28 @@ 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. */
diff --git a/xen/common/efi/runtime.c b/xen/common/efi/runtime.c
index 0750436..15401a4 100644
--- a/xen/common/efi/runtime.c
+++ b/xen/common/efi/runtime.c
@@ -146,6 +146,44 @@ static void _delay(void)
     }
     printk("\n");
 }
+static void _dumpcode(char *code, unsigned long s, unsigned long e)
+{
+    unsigned long idx, e_idx;
+    unsigned long cr3;
+    unsigned int i;
+
+    if ( s > e )
+        return;
+
+    idx = s;
+
+    printk("%lx -> %lx\nCode: ", s, e);
+    do {
+        e_idx = idx + 4095;
+        if ( e_idx > e )
+            e_idx = e;
+
+        process_pending_softirqs();
+
+        memset(code, 0, 4096);
+
+        cr3 = efi_rs_enter();
+        memcpy(code, (void *)idx, e_idx - idx);
+        efi_rs_leave(cr3);
+
+        for ( i = 0; i < e_idx - idx ;i++)
+        {
+            if ( i & 0xFF )
+                process_pending_softirqs();
+            printk(" %02x", (unsigned short)code[i] & 0xFF);
+        }
+        printk("\n");
+        idx = e_idx + 1;
+    } while ( idx < e );
+
+    printk("\n");
+    _delay();
+}
 
 long efi_debug(void)
 {
@@ -162,12 +200,13 @@ long efi_debug(void)
     unsigned int rev;
     unsigned long getnext, get;
     char *code;
+    unsigned long val[13];
 
     if ( !cr3 )
         return -EOPNOTSUPP;
     efi_rs_leave(cr3);
 
-    code = xzalloc_bytes(size);
+    code = xzalloc_bytes(4096);
     if ( !code )
         return -ENOMEM;
 
@@ -193,18 +232,41 @@ long efi_debug(void)
 
     cr3 = efi_rs_enter();
     getnext = (unsigned long)efi_rs->GetNextVariableName;
-    memcpy(code, efi_rs->GetNextVariableName, 1024);
-    get = (unsigned long)efi_rs->GetVariable;
+    get = (unsigned long)efi_rs;
     efi_rs_leave(cr3);
 
-    printk(", GetNextVariableName: %lx, GetVariable: %lx\n", getnext, get);
-    printk(" Code: ");
-    for ( i = 0; i < 1024;i++)
-        printk(" %02x", (unsigned short)code[i] & 0xFF);
-    printk("\n");
+    printk(", GetNextVariableName: %lx, efi_rs: %lx\n", getnext, get);
+
+    val[0] = 0xcfdba230;       /* Saw it somewhere Boot Services?? */
+    val[1] = 0xcfdba270;
+    val[2] = val[0] + 0x18;
+    val[3] = val[1] + 0x18;
+    val[4] = 0xcfdc9cc0;
+       val[5] = getnext + + 0x11bc; /* 3f: */
+    val[6] = getnext + 0x11fc;
+    val[7] = getnext + 0x11e4;
+    val[8] = getnext + 0x1154;
+    val[9] = getnext + 0x116c;
+    val[10] = getnext + 0x11d4;
+    val[11] = getnext + 0x1154;
+    val[12] = getnext + 0x116c;
+
+    for ( i = 0; i < 13; i++)
+       {
+               printk("val[%d]:\n", i);
+        _dumpcode(code, val[i], val[i] + 8);
+       }
+#if 0
+    _dumpcode(code, get, get+4096);
+    _delay();
 
+    _dumpcode(code, 0x00000d6929000, 0x00000d6a4ffff);
     _delay();
 
+    _dumpcode(code, 0x00000cfdba000,0x00000cfdcffff);
+    _delay();
+#endif
+    _dumpcode(code, 0, 512);
     idx = 1;
     do {
         printk("%4d:", idx++);
> 
> Jan
>

Attachment: print.txt
Description: Text document

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