[v5,5/5] fix: add multiboot2 protocol support for EFI platforms

Commit Message

Douglas Goldstein Jan. 19, 2017, 3:52 a.m. UTC

This should be squashed into the 4/4 patch 'x86: add multiboot2 protocol
support for EFI platforms'.

- fix incorrect assembly (identified by Andrew Cooper)
- fix issue where the trampoline size was left as 0 and the
  way the memory is allocated for the trampolines we would go to
  the end of an available section and then subtract off the size
  to decide where to place it. The end result was that we would
  always copy the trampolines and the 32-bit stack into some
  form of reserved memory after the conventional region we
  wanted to put things into. On some systems this did not
  manifest as a crash while on others it did. Reworked the
  changes to always reserve 64kb for both the stack and the size
  of the trampolines. Added a build time assert to make sure we have
  enough room always.

Signed-off-by: Doug Goldstein <[e-mail]>
Reviewed-by: Jan Beulich <[e-mail]>
---
Doug v5 - change comment style in xen.lds.S as requested by Jan Beulich.
        - fix comment around ExitBootServices()
          (suggested by Andrew Cooper)
        - change multiboot2 efi entry point name to __mb2_efi64_start
          (suggested by Andrew Cooper)
Doug v4 - change wording around "stack base"
          (found by Jan Beulich)
        - added build time assert as suggested by Jan Beulich
        - added a KB() macro to make our sizes consistent with MB() and
          GB().
Doug v3 - drop ASSERTs since they are runtime only without any output.
          This should be completely mitigated by using max() and
          ensuring we have a sane value.
          (found by Jan Beulich)
        - removed extra_mem variable that was incorrectly left behind.
          (found by Jan Beulich)
        - fix comment around the "start of stack"
          (found by Jan Beulich)
Doug v2 - new in this version to help show what's changed
---
---
 xen/arch/x86/boot/head.S    | 7 ++++---
 xen/arch/x86/efi/efi-boot.h | 8 ++++----
 xen/arch/x86/efi/stub.c     | 2 +-
 xen/arch/x86/xen.lds.S      | 8 ++++++++
 xen/include/xen/config.h    | 1 +
 5 files changed, 18 insertions(+), 8 deletions(-)

Patch

diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
index ac93df0..f2e8cc9 100644
--- a/xen/arch/x86/boot/head.S
+++ b/xen/arch/x86/boot/head.S
@@ -89,12 +89,12 @@  multiboot2_header_start:
                    0, /* Number of the lines - no preference. */ \
                    0  /* Number of bits per pixel - no preference. */
 
-        /* Inhibit bootloader from calling ExitBootServices(). */
+        /* Request that ExitBootServices() not be called. */
         mb2ht_init MB2_HT(EFI_BS), MB2_HT(OPTIONAL)
 
         /* EFI64 entry point. */
         mb2ht_init MB2_HT(ENTRY_ADDRESS_EFI64), MB2_HT(OPTIONAL), \
-                   sym_phys(__efi64_start)
+                   sym_phys(__mb2_efi64_start)
 
         /* Multiboot2 header end tag. */
         mb2ht_init MB2_HT(END), MB2_HT(REQUIRED)
@@ -169,7 +169,7 @@  not_multiboot:
 
         .code64
 
-__efi64_start:
+__mb2_efi64_start:
         cld
 
         /* VGA is not available on EFI platforms. */
@@ -519,6 +519,7 @@  trampoline_setup:
 1:
         /* Switch to low-memory stack.  */
         mov     sym_phys(trampoline_phys),%edi
+        /* The stack ends 64kb after the location of trampoline_phys */
         lea     0x10000(%edi),%esp
         lea     trampoline_boot_cpu_entry-trampoline_start(%edi),%eax
         pushl   $BOOT_CS32
diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
index dc857d8..d2ebf21 100644
--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -146,8 +146,6 @@  static void __init efi_arch_process_memory_map(EFI_SYSTEM_TABLE *SystemTable,
 {
     struct e820entry *e;
     unsigned int i;
-    /* Check for extra mem for mbi data if Xen is loaded via multiboot2 protocol. */
-    UINTN extra_mem = efi_enabled(EFI_LOADER) ? 0 : (64 << 10);
 
     /* Populate E820 table and check trampoline area availability. */
     e = e820map - 1;
@@ -170,8 +168,7 @@  static void __init efi_arch_process_memory_map(EFI_SYSTEM_TABLE *SystemTable,
             /* fall through */
         case EfiConventionalMemory:
             if ( !trampoline_phys && desc->PhysicalStart + len <= 0x100000 &&
-                 len >= cfg.size + extra_mem &&
-                 desc->PhysicalStart + len > cfg.addr )
+                 len >= cfg.size && desc->PhysicalStart + len > cfg.addr )
                 cfg.addr = (desc->PhysicalStart + len - cfg.size) & PAGE_MASK;
             /* fall through */
         case EfiLoaderCode:
@@ -686,6 +683,9 @@  paddr_t __init efi_multiboot2(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTa
     setup_efi_pci();
     efi_variables();
 
+    /* This is the maximum size of our trampoline + our low memory stack */
+    cfg.size = KB(64);
+
     if ( gop )
         efi_set_gop_mode(gop, gop_mode);
 
diff --git a/xen/arch/x86/efi/stub.c b/xen/arch/x86/efi/stub.c
index 6ea6aa1..b81adc0 100644
--- a/xen/arch/x86/efi/stub.c
+++ b/xen/arch/x86/efi/stub.c
@@ -33,7 +33,7 @@  paddr_t __init noreturn efi_multiboot2(EFI_HANDLE ImageHandle,
      * not be directly supported by C compiler.
      */
     asm volatile(
-    "    call %2                      \n"
+    "    call *%2                     \n"
     "0:  hlt                          \n"
     "    jmp  0b                      \n"
        : "+c" (StdErr), "+d" (err) : "g" (StdErr->OutputString)
diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index e0e2529..025760f 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -333,3 +333,11 @@  ASSERT(IS_ALIGNED(trampoline_start, 4), "trampoline_start misaligned")
 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")
+
+/*
+ * The trampolines and the low memory stack must fit in 64kb. In testing
+ * the low memory stack never exceeded 1kb so just require that the
+ * trampolines fit in 63kb, leaving 1kb for the stack.
+ */
+ASSERT((trampoline_end - trampoline_start) < KB(63),
+    "not enough room for trampolines and low memory stack")
diff --git a/xen/include/xen/config.h b/xen/include/xen/config.h
index 473c5e8..04e4da5 100644
--- a/xen/include/xen/config.h
+++ b/xen/include/xen/config.h
@@ -70,6 +70,7 @@ 
 #define __force
 #define __bitwise
 
+#define KB(_kb)     (_AC(_kb, ULL) << 10)
 #define MB(_mb)     (_AC(_mb, ULL) << 20)
 #define GB(_gb)     (_AC(_gb, ULL) << 30)