[PATCH 0/7] efi: improved correctness, arm unification, and cleanup

[PATCH 0/7] efi: improved correctness, arm unification, and cleanup

[Leif Lindholm]

This patch series is really three different ones, but they unite around
the need for (and the implementation) of more flexible control of memory
allocation on UEFI systems.

1: Adding new interfaces
- A function for detecting the start address of RAM
  Since ARM platforms have no standardised memory map, implement a
  function that picks the lowest-address region supporting the
  write-back cache atribute from the UEFI memory map.
- Implement and expose a new memory allocation function giving access to
  the allocation type and memory type arguments to the AllocatePages
  boot service.

2: ARM unification
- The fdt helper library for arm64-efi is really an efi/fdt function.
  So move it to neutral ground in loader/efi.
- The arm64 efi linux loader was written for arm64 only, so clean it up a
  bit with regards to 32/64-bit portability, and abstract out some
  definitions and function prototypes.
- Move the arm efi port to use the arm64 linux loader instead of the one
  shared with the u-boot port. Clean up the u-boot loder by deleting the
  efi-specific bits and other code only used by that.

3: Correctness improvements
- There are some restrictions on the placement of initrd images in
  relation to the runtime kernel images, for both arm and arm64 - but the
  arm64 one did not use to be explicitly documented (and only triggerable
  on systems with > 40GB of RAM), and the u-boot loader always placed the
  images nearly adjacent. Use the new interfaces to place the initrd as
  approrpiate for each architecture.
- The allocation of memory for the grub heap is done of memory type
  GRUB_EFI_LOADER_DATA. Since UEFI can return memory with non-executable
  mappings for this request, and modules are loaded onto the heap, change
  this allocation to GRUB_EFI_LOADER_CODE instead.

I first sent this series out on 28 February this year.
Changes since previous submission:
- Rebased (to deal with arm coreboot upstream changes).
- Moved EFI page size definitions to common header.
- Moved a few stray 32/64-bit fixes from 3/7 to 4/7.

Leif Lindholm (7):
  efi: add grub_efi_get_dram_base() function for arm*
  efi: refactor grub_efi_allocate_pages
  efi: move fdt helper library
  arm64: make efi linux loader more generic
  arm: reuse arm64 linux loader on efi systems
  efi: restrict arm/arm64 linux loader initrd placement
  efi: change heap allocation type to GRUB_EFI_LOADER_CODE

 grub-core/Makefile.am                 |   1 -
 grub-core/Makefile.core.def           |   6 +-
 grub-core/kern/arm/efi/misc.c         | 202 ----------------------------------
 grub-core/kern/efi/mm.c               |  92 ++++++++++++----
 grub-core/loader/arm/linux.c          |  39 +------
 grub-core/loader/arm64/linux.c        |  73 ++++++++----
 grub-core/loader/arm64/xen_boot.c     |  15 +--
 grub-core/loader/{arm64 => efi}/fdt.c |  11 +-
 include/grub/arm/efi/loader.h         |  26 -----
 include/grub/arm/efi/memory.h         |   3 +
 include/grub/arm/linux.h              |  30 ++---
 include/grub/arm64/linux.h            |  13 +--
 include/grub/efi/efi.h                |  10 ++
 include/grub/{arm64 => efi}/fdtload.h |   3 -
 include/grub/efi/memory.h             |   3 +
 include/grub/efi/pe32.h               |   2 +
 16 files changed, 176 insertions(+), 353 deletions(-)
 delete mode 100644 grub-core/kern/arm/efi/misc.c
 rename grub-core/loader/{arm64 => efi}/fdt.c (93%)
 delete mode 100644 include/grub/arm/efi/loader.h
 rename include/grub/{arm64 => efi}/fdtload.h (89%)

-- 
2.11.0

[PATCH 1/7] efi: add grub_efi_get_dram_base() function for arm*

[Leif Lindholm]

Since ARM platforms do not have a common memory map, add a helper
function that finds the lowest address region with the EFI_MEMORY_WB
attribute set in the UEFI memory map.

Required for the arm/arm64 linux loader to restrict the initrd
location to where it will be accessible by the kernel at runtime.

Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
---
 grub-core/kern/efi/mm.c | 42 ++++++++++++++++++++++++++++++++++++++++++
 include/grub/efi/efi.h  |  1 +
 2 files changed, 43 insertions(+)

diff --git a/grub-core/kern/efi/mm.c b/grub-core/kern/efi/mm.c
index 20a47aaf5..460a4b763 100644
--- a/grub-core/kern/efi/mm.c
+++ b/grub-core/kern/efi/mm.c
@@ -525,3 +525,45 @@ grub_efi_mm_init (void)
   grub_efi_free_pages ((grub_addr_t) memory_map,
 		       2 * BYTES_TO_PAGES (MEMORY_MAP_SIZE));
 }
+
+#if defined (__arm__) || defined (__aarch64__)
+grub_err_t
+grub_efi_get_dram_base(grub_addr_t *base_addr)
+{
+  grub_efi_memory_descriptor_t *memory_map;
+  grub_efi_memory_descriptor_t *desc;
+  grub_efi_uintn_t mmap_size;
+  grub_efi_uintn_t desc_size;
+
+  mmap_size = (1 << GRUB_EFI_PAGE_SHIFT);
+  while (1)
+    {
+      int ret;
+
+      memory_map = grub_malloc (mmap_size);
+      if (! memory_map)
+        return GRUB_ERR_OUT_OF_MEMORY;
+      ret = grub_efi_get_memory_map (&mmap_size, memory_map, NULL,
+				     &desc_size, NULL);
+      if (ret > 0)
+	break;
+
+      grub_free (memory_map);
+      if (ret == 0)
+	return GRUB_ERR_BUG;
+
+      mmap_size += (1 << GRUB_EFI_PAGE_SHIFT);
+    }
+
+  for (desc = memory_map, *base_addr = GRUB_UINT_MAX;
+       (grub_addr_t) desc < ((grub_addr_t) memory_map + mmap_size);
+       desc = NEXT_MEMORY_DESCRIPTOR (desc, desc_size))
+    {
+      if (desc->attribute & GRUB_EFI_MEMORY_WB)
+	if (desc->physical_start < *base_addr)
+	  *base_addr = desc->physical_start;
+    }
+
+  return GRUB_ERR_NONE;
+}
+#endif
diff --git a/include/grub/efi/efi.h b/include/grub/efi/efi.h
index e9c601f34..845fc2438 100644
--- a/include/grub/efi/efi.h
+++ b/include/grub/efi/efi.h
@@ -83,6 +83,7 @@ extern void (*EXPORT_VAR(grub_efi_net_config)) (grub_efi_handle_t hnd,
 
 #if defined(__arm__) || defined(__aarch64__)
 void *EXPORT_FUNC(grub_efi_get_firmware_fdt)(void);
+grub_err_t EXPORT_FUNC(grub_efi_get_dram_base)(grub_addr_t *);
 #endif
 
 grub_addr_t grub_efi_modules_addr (void);
-- 
2.11.0

[PATCH 2/7] efi: refactor grub_efi_allocate_pages

[Leif Lindholm]

Expose a new function, grub_efi_allocate_pages_real(), making it possible
to specify allocation type and memory type as supported by the UEFI
AllocatePages boot service.

Make grub_efi_allocate_pages() a consumer of the new function,
maintaining its old functionality.

Also delete some left-around #if 1/#else blocks in the affected
functions.

Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
---
 grub-core/kern/efi/mm.c | 46 ++++++++++++++++++++++++----------------------
 include/grub/efi/efi.h  |  5 +++++
 2 files changed, 29 insertions(+), 22 deletions(-)

diff --git a/grub-core/kern/efi/mm.c b/grub-core/kern/efi/mm.c
index 460a4b763..7b1763bc5 100644
--- a/grub-core/kern/efi/mm.c
+++ b/grub-core/kern/efi/mm.c
@@ -51,36 +51,20 @@ int grub_efi_is_finished = 0;
 
 /* Allocate pages. Return the pointer to the first of allocated pages.  */
 void *
-grub_efi_allocate_pages (grub_efi_physical_address_t address,
-			 grub_efi_uintn_t pages)
+grub_efi_allocate_pages_real (grub_efi_physical_address_t address,
+			      grub_efi_uintn_t pages,
+			      grub_efi_allocate_type_t alloctype,
+			      grub_efi_memory_type_t memtype)
 {
-  grub_efi_allocate_type_t type;
   grub_efi_status_t status;
   grub_efi_boot_services_t *b;
 
-#if 1
   /* Limit the memory access to less than 4GB for 32-bit platforms.  */
   if (address > GRUB_EFI_MAX_USABLE_ADDRESS)
     return 0;
-#endif
-
-#if 1
-  if (address == 0)
-    {
-      type = GRUB_EFI_ALLOCATE_MAX_ADDRESS;
-      address = GRUB_EFI_MAX_USABLE_ADDRESS;
-    }
-  else
-    type = GRUB_EFI_ALLOCATE_ADDRESS;
-#else
-  if (address == 0)
-    type = GRUB_EFI_ALLOCATE_ANY_PAGES;
-  else
-    type = GRUB_EFI_ALLOCATE_ADDRESS;
-#endif
 
   b = grub_efi_system_table->boot_services;
-  status = efi_call_4 (b->allocate_pages, type, GRUB_EFI_LOADER_DATA, pages, &address);
+  status = efi_call_4 (b->allocate_pages, alloctype, memtype, pages, &address);
   if (status != GRUB_EFI_SUCCESS)
     return 0;
 
@@ -89,7 +73,7 @@ grub_efi_allocate_pages (grub_efi_physical_address_t address,
       /* Uggh, the address 0 was allocated... This is too annoying,
 	 so reallocate another one.  */
       address = GRUB_EFI_MAX_USABLE_ADDRESS;
-      status = efi_call_4 (b->allocate_pages, type, GRUB_EFI_LOADER_DATA, pages, &address);
+      status = efi_call_4 (b->allocate_pages, alloctype, memtype, pages, &address);
       grub_efi_free_pages (0, pages);
       if (status != GRUB_EFI_SUCCESS)
 	return 0;
@@ -98,6 +82,24 @@ grub_efi_allocate_pages (grub_efi_physical_address_t address,
   return (void *) ((grub_addr_t) address);
 }
 
+void *
+grub_efi_allocate_pages (grub_efi_physical_address_t address,
+			 grub_efi_uintn_t pages)
+{
+  grub_efi_allocate_type_t alloctype;
+
+  if (address == 0)
+    {
+      alloctype = GRUB_EFI_ALLOCATE_MAX_ADDRESS;
+      address = GRUB_EFI_MAX_USABLE_ADDRESS;
+    }
+  else
+    alloctype = GRUB_EFI_ALLOCATE_ADDRESS;
+
+  return grub_efi_allocate_pages_real (address, pages, alloctype,
+				       GRUB_EFI_LOADER_DATA);
+}
+
 /* Free pages starting from ADDRESS.  */
 void
 grub_efi_free_pages (grub_efi_physical_address_t address,
diff --git a/include/grub/efi/efi.h b/include/grub/efi/efi.h
index 845fc2438..904722174 100644
--- a/include/grub/efi/efi.h
+++ b/include/grub/efi/efi.h
@@ -38,6 +38,11 @@ void *EXPORT_FUNC(grub_efi_open_protocol) (grub_efi_handle_t handle,
 int EXPORT_FUNC(grub_efi_set_text_mode) (int on);
 void EXPORT_FUNC(grub_efi_stall) (grub_efi_uintn_t microseconds);
 void *
+EXPORT_FUNC(grub_efi_allocate_pages_real) (grub_efi_physical_address_t address,
+				           grub_efi_uintn_t pages,
+					   grub_efi_allocate_type_t alloctype,
+					   grub_efi_memory_type_t memtype);
+void *
 EXPORT_FUNC(grub_efi_allocate_pages) (grub_efi_physical_address_t address,
 				      grub_efi_uintn_t pages);
 void EXPORT_FUNC(grub_efi_free_pages) (grub_efi_physical_address_t address,
-- 
2.11.0

[PATCH 3/7] efi: move fdt helper library

[Leif Lindholm]

There is nothing ARM64 (or even ARM) specific about the efi fdt helper
library, which is used for locating or overriding a firmware-provided
devicetree in a UEFI system - so move it to loader/efi for reuse.

Move the fdtload.h include file to grub/efi and move the EFI page size
definitions to grub/efi/memory.h. (These definitions refer strictly to
allocation operations, as opposed to translation granules.)
---
 grub-core/Makefile.core.def           | 2 +-
 grub-core/loader/arm64/linux.c        | 3 ++-
 grub-core/loader/arm64/xen_boot.c     | 3 ++-
 grub-core/loader/{arm64 => efi}/fdt.c | 3 ++-
 include/grub/{arm64 => efi}/fdtload.h | 3 ---
 include/grub/efi/memory.h             | 3 +++
 6 files changed, 10 insertions(+), 7 deletions(-)
 rename grub-core/loader/{arm64 => efi}/fdt.c (98%)
 rename include/grub/{arm64 => efi}/fdtload.h (89%)

diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
index 1d86bd22e..a65c27f7f 100644
--- a/grub-core/Makefile.core.def
+++ b/grub-core/Makefile.core.def
@@ -1707,7 +1707,7 @@ module = {
 
 module = {
   name = fdt;
-  arm64 = loader/arm64/fdt.c;
+  arm64 = loader/efi/fdt.c;
   common = lib/fdt.c;
   enable = fdt;
 };
diff --git a/grub-core/loader/arm64/linux.c b/grub-core/loader/arm64/linux.c
index 9519d2e4d..cac94d53d 100644
--- a/grub-core/loader/arm64/linux.c
+++ b/grub-core/loader/arm64/linux.c
@@ -26,8 +26,9 @@
 #include <grub/mm.h>
 #include <grub/types.h>
 #include <grub/cpu/linux.h>
-#include <grub/cpu/fdtload.h>
 #include <grub/efi/efi.h>
+#include <grub/efi/fdtload.h>
+#include <grub/efi/memory.h>
 #include <grub/efi/pe32.h>
 #include <grub/i18n.h>
 #include <grub/lib/cmdline.h>
diff --git a/grub-core/loader/arm64/xen_boot.c b/grub-core/loader/arm64/xen_boot.c
index 27ede46ca..d092a53ed 100644
--- a/grub-core/loader/arm64/xen_boot.c
+++ b/grub-core/loader/arm64/xen_boot.c
@@ -27,9 +27,10 @@
 #include <grub/misc.h>
 #include <grub/mm.h>
 #include <grub/types.h>
-#include <grub/cpu/fdtload.h>
 #include <grub/cpu/linux.h>
 #include <grub/efi/efi.h>
+#include <grub/efi/fdtload.h>
+#include <grub/efi/memory.h>
 #include <grub/efi/pe32.h>	/* required by struct xen_hypervisor_header */
 #include <grub/i18n.h>
 #include <grub/lib/cmdline.h>
diff --git a/grub-core/loader/arm64/fdt.c b/grub-core/loader/efi/fdt.c
similarity index 98%
rename from grub-core/loader/arm64/fdt.c
rename to grub-core/loader/efi/fdt.c
index db49cf649..be369fd9d 100644
--- a/grub-core/loader/arm64/fdt.c
+++ b/grub-core/loader/efi/fdt.c
@@ -18,12 +18,13 @@
 
 #include <grub/fdt.h>
 #include <grub/mm.h>
-#include <grub/cpu/fdtload.h>
 #include <grub/err.h>
 #include <grub/dl.h>
 #include <grub/command.h>
 #include <grub/file.h>
 #include <grub/efi/efi.h>
+#include <grub/efi/fdtload.h>
+#include <grub/machine/memory.h>
 
 static void *loaded_fdt;
 static void *fdt;
diff --git a/include/grub/arm64/fdtload.h b/include/grub/efi/fdtload.h
similarity index 89%
rename from include/grub/arm64/fdtload.h
rename to include/grub/efi/fdtload.h
index 7b9ddba91..713c9424d 100644
--- a/include/grub/arm64/fdtload.h
+++ b/include/grub/efi/fdtload.h
@@ -29,7 +29,4 @@ grub_fdt_unload (void);
 grub_err_t
 grub_fdt_install (void);
 
-#define GRUB_EFI_PAGE_SHIFT	12
-#define GRUB_EFI_BYTES_TO_PAGES(bytes)   (((bytes) + 0xfff) >> GRUB_EFI_PAGE_SHIFT)
-
 #endif
diff --git a/include/grub/efi/memory.h b/include/grub/efi/memory.h
index 20526b146..0eb0b70b6 100644
--- a/include/grub/efi/memory.h
+++ b/include/grub/efi/memory.h
@@ -22,6 +22,9 @@
 #include <grub/err.h>
 #include <grub/types.h>
 
+#define GRUB_EFI_PAGE_SHIFT	12
+#define GRUB_EFI_BYTES_TO_PAGES(bytes)   (((bytes) + 0xfff) >> GRUB_EFI_PAGE_SHIFT)
+
 #define GRUB_MMAP_REGISTER_BY_FIRMWARE  1
 
 grub_err_t grub_machine_mmap_register (grub_uint64_t start, grub_uint64_t size,
-- 
2.11.0

[PATCH 4/7] arm64: make efi linux loader more generic

[Leif Lindholm]

In order to enable reuse of the arm64 efi linux loader for arm,
change a few function names and macros. Add a global definition
of GRUB_PE32_MAGIC in grub/efi/pe32.h.

Make the arm64 efi loader (and fdt helpers) 32/64-bit safe.

Also update the arm64 xen loader, since it depends on some of the
functions in the linux loader.

Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
---
 grub-core/loader/arm64/linux.c    | 31 ++++++++++++++-----------------
 grub-core/loader/arm64/xen_boot.c | 12 ++++++------
 grub-core/loader/efi/fdt.c        |  8 ++++----
 include/grub/arm64/linux.h        | 13 ++-----------
 include/grub/efi/efi.h            |  4 ++++
 include/grub/efi/pe32.h           |  2 ++
 6 files changed, 32 insertions(+), 38 deletions(-)

diff --git a/grub-core/loader/arm64/linux.c b/grub-core/loader/arm64/linux.c
index cac94d53d..8cd44230d 100644
--- a/grub-core/loader/arm64/linux.c
+++ b/grub-core/loader/arm64/linux.c
@@ -48,18 +48,16 @@ static grub_addr_t initrd_start;
 static grub_addr_t initrd_end;
 
 grub_err_t
-grub_arm64_uefi_check_image (struct grub_arm64_linux_kernel_header * lh)
+grub_efi_linux_check_image (struct grub_linux_kernel_header * lh)
 {
-  if (lh->magic != GRUB_ARM64_LINUX_MAGIC)
+  if (lh->magic != GRUB_LINUX_MAGIC_SIGNATURE)
     return grub_error(GRUB_ERR_BAD_OS, "invalid magic number");
 
-  if ((lh->code0 & 0xffff) != GRUB_EFI_PE_MAGIC)
+  if ((lh->code0 & 0xffff) != GRUB_PE32_MAGIC)
     return grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET,
 		       N_("plain image kernel not supported - rebuild with CONFIG_(U)EFI_STUB enabled"));
 
   grub_dprintf ("linux", "UEFI stub kernel:\n");
-  grub_dprintf ("linux", "text_offset = 0x%012llx\n",
-		(long long unsigned) lh->text_offset);
   grub_dprintf ("linux", "PE/COFF header @ %08x\n", lh->hdr_offset);
 
   return GRUB_ERR_NONE;
@@ -87,8 +85,8 @@ finalize_params_linux (void)
   /* Set initrd info */
   if (initrd_start && initrd_end > initrd_start)
     {
-      grub_dprintf ("linux", "Initrd @ 0x%012lx-0x%012lx\n",
-		    initrd_start, initrd_end);
+      grub_dprintf ("linux", "Initrd @ %p-%p\n",
+		    (void *) initrd_start, (void *) initrd_end);
 
       retval = grub_fdt_set_prop64 (fdt, node, "linux,initrd-start",
 				    initrd_start);
@@ -111,7 +109,7 @@ failure:
 }
 
 grub_err_t
-grub_arm64_uefi_boot_image (grub_addr_t addr, grub_size_t size, char *args)
+grub_efi_linux_boot_image (grub_addr_t addr, grub_size_t size, char *args)
 {
   grub_efi_memory_mapped_device_path_t *mempath;
   grub_efi_handle_t image_handle;
@@ -163,7 +161,7 @@ grub_arm64_uefi_boot_image (grub_addr_t addr, grub_size_t size, char *args)
 
   /* When successful, not reached */
   b->unload_image (image_handle);
-  grub_efi_free_pages ((grub_efi_physical_address_t) loaded_image->load_options,
+  grub_efi_free_pages ((grub_addr_t) loaded_image->load_options,
 		       GRUB_EFI_BYTES_TO_PAGES (loaded_image->load_options_size));
 
   return grub_errno;
@@ -175,8 +173,8 @@ grub_linux_boot (void)
   if (finalize_params_linux () != GRUB_ERR_NONE)
     return grub_errno;
 
-  return (grub_arm64_uefi_boot_image((grub_addr_t)kernel_addr,
-                                     kernel_size, linux_args));
+  return (grub_efi_linux_boot_image((grub_addr_t)kernel_addr,
+                                    kernel_size, linux_args));
 }
 
 static grub_err_t
@@ -190,7 +188,7 @@ grub_linux_unload (void)
   initrd_start = initrd_end = 0;
   grub_free (linux_args);
   if (kernel_addr)
-    grub_efi_free_pages ((grub_efi_physical_address_t) kernel_addr,
+    grub_efi_free_pages ((grub_addr_t) kernel_addr,
 			 GRUB_EFI_BYTES_TO_PAGES (kernel_size));
   grub_fdt_unload ();
   return GRUB_ERR_NONE;
@@ -242,8 +240,7 @@ grub_cmd_initrd (grub_command_t cmd __attribute__ ((unused)),
  fail:
   grub_initrd_close (&initrd_ctx);
   if (initrd_mem && !initrd_start)
-    grub_efi_free_pages ((grub_efi_physical_address_t) initrd_mem,
-			 initrd_pages);
+    grub_efi_free_pages ((grub_addr_t) initrd_mem, initrd_pages);
 
   return grub_errno;
 }
@@ -253,7 +250,7 @@ grub_cmd_linux (grub_command_t cmd __attribute__ ((unused)),
 		int argc, char *argv[])
 {
   grub_file_t file = 0;
-  struct grub_arm64_linux_kernel_header lh;
+  struct grub_linux_kernel_header lh;
 
   grub_dl_ref (my_mod);
 
@@ -272,7 +269,7 @@ grub_cmd_linux (grub_command_t cmd __attribute__ ((unused)),
   if (grub_file_read (file, &lh, sizeof (lh)) < (long) sizeof (lh))
     return grub_errno;
 
-  if (grub_arm64_uefi_check_image (&lh) != GRUB_ERR_NONE)
+  if (grub_efi_linux_check_image (&lh) != GRUB_ERR_NONE)
     goto fail;
 
   grub_loader_unset();
@@ -330,7 +327,7 @@ fail:
     grub_free (linux_args);
 
   if (kernel_addr && !loaded)
-    grub_efi_free_pages ((grub_efi_physical_address_t) kernel_addr,
+    grub_efi_free_pages ((grub_addr_t) kernel_addr,
 			 GRUB_EFI_BYTES_TO_PAGES (kernel_size));
 
   return grub_errno;
diff --git a/grub-core/loader/arm64/xen_boot.c b/grub-core/loader/arm64/xen_boot.c
index d092a53ed..e8720584b 100644
--- a/grub-core/loader/arm64/xen_boot.c
+++ b/grub-core/loader/arm64/xen_boot.c
@@ -67,7 +67,7 @@ typedef enum module_type module_type_t;
 
 struct xen_hypervisor_header
 {
-  struct grub_arm64_linux_kernel_header efi_head;
+  struct grub_linux_kernel_header efi_head;
 
   /* This is always PE\0\0.  */
   grub_uint8_t signature[GRUB_PE32_SIGNATURE_SIZE];
@@ -254,9 +254,9 @@ xen_boot (void)
   if (err)
     return err;
 
-  return grub_arm64_uefi_boot_image (xen_hypervisor->start,
-				     xen_hypervisor->size,
-				     xen_hypervisor->cmdline);
+  return grub_efi_linux_boot_image (xen_hypervisor->start,
+				    xen_hypervisor->size,
+				    xen_hypervisor->cmdline);
 }
 
 static void
@@ -458,8 +458,8 @@ grub_cmd_xen_hypervisor (grub_command_t cmd __attribute__ ((unused)),
 
   if (grub_file_read (file, &sh, sizeof (sh)) != (long) sizeof (sh))
     goto fail;
-  if (grub_arm64_uefi_check_image
-      ((struct grub_arm64_linux_kernel_header *) &sh) != GRUB_ERR_NONE)
+  if (grub_efi_linux_check_image
+      ((struct grub_linux_kernel_header *) &sh) != GRUB_ERR_NONE)
     goto fail;
   grub_file_seek (file, 0);
 
diff --git a/grub-core/loader/efi/fdt.c b/grub-core/loader/efi/fdt.c
index be369fd9d..e2899c47b 100644
--- a/grub-core/loader/efi/fdt.c
+++ b/grub-core/loader/efi/fdt.c
@@ -33,12 +33,12 @@ void *
 grub_fdt_load (grub_size_t additional_size)
 {
   void *raw_fdt;
-  grub_size_t size;
+  unsigned int size;
 
   if (fdt)
     {
       size = GRUB_EFI_BYTES_TO_PAGES (grub_fdt_get_totalsize (fdt));
-      grub_efi_free_pages ((grub_efi_physical_address_t) fdt, size);
+      grub_efi_free_pages ((grub_addr_t) fdt, size);
     }
 
   if (loaded_fdt)
@@ -50,7 +50,7 @@ grub_fdt_load (grub_size_t additional_size)
     raw_fdt ? grub_fdt_get_totalsize (raw_fdt) : GRUB_FDT_EMPTY_TREE_SZ;
   size += additional_size;
 
-  grub_dprintf ("linux", "allocating %ld bytes for fdt\n", size);
+  grub_dprintf ("linux", "allocating %d bytes for fdt\n", size);
   fdt = grub_efi_allocate_pages (0, GRUB_EFI_BYTES_TO_PAGES (size));
   if (!fdt)
     return NULL;
@@ -89,7 +89,7 @@ grub_fdt_unload (void) {
   if (!fdt) {
     return;
   }
-  grub_efi_free_pages ((grub_efi_physical_address_t) fdt,
+  grub_efi_free_pages ((grub_addr_t) fdt,
 		       GRUB_EFI_BYTES_TO_PAGES (grub_fdt_get_totalsize (fdt)));
   fdt = NULL;
 }
diff --git a/include/grub/arm64/linux.h b/include/grub/arm64/linux.h
index 1ea23696e..e53be83b1 100644
--- a/include/grub/arm64/linux.h
+++ b/include/grub/arm64/linux.h
@@ -19,14 +19,10 @@
 #ifndef GRUB_LINUX_CPU_HEADER
 #define GRUB_LINUX_CPU_HEADER 1
 
-#include <grub/efi/efi.h>
-
-#define GRUB_ARM64_LINUX_MAGIC 0x644d5241 /* 'ARM\x64' */
-
-#define GRUB_EFI_PE_MAGIC	0x5A4D
+#define GRUB_LINUX_MAGIC_SIGNATURE 0x644d5241 /* 'ARM\x64' */
 
 /* From linux/Documentation/arm64/booting.txt */
-struct grub_arm64_linux_kernel_header
+struct grub_linux_kernel_header
 {
   grub_uint32_t code0;		/* Executable code */
   grub_uint32_t code1;		/* Executable code */
@@ -40,9 +36,4 @@ struct grub_arm64_linux_kernel_header
   grub_uint32_t hdr_offset;	/* Offset of PE/COFF header */
 };
 
-grub_err_t grub_arm64_uefi_check_image (struct grub_arm64_linux_kernel_header
-                                        *lh);
-grub_err_t grub_arm64_uefi_boot_image (grub_addr_t addr, grub_size_t size,
-                                       char *args);
-
 #endif /* ! GRUB_LINUX_CPU_HEADER */
diff --git a/include/grub/efi/efi.h b/include/grub/efi/efi.h
index 904722174..395daeae3 100644
--- a/include/grub/efi/efi.h
+++ b/include/grub/efi/efi.h
@@ -89,6 +89,10 @@ extern void (*EXPORT_VAR(grub_efi_net_config)) (grub_efi_handle_t hnd,
 #if defined(__arm__) || defined(__aarch64__)
 void *EXPORT_FUNC(grub_efi_get_firmware_fdt)(void);
 grub_err_t EXPORT_FUNC(grub_efi_get_dram_base)(grub_addr_t *);
+#include <grub/cpu/linux.h>
+grub_err_t grub_efi_linux_check_image(struct grub_linux_kernel_header *lh);
+grub_err_t grub_efi_linux_boot_image(grub_addr_t addr, grub_size_t size,
+				     char *args);
 #endif
 
 grub_addr_t grub_efi_modules_addr (void);
diff --git a/include/grub/efi/pe32.h b/include/grub/efi/pe32.h
index f79c36c02..7d44732d2 100644
--- a/include/grub/efi/pe32.h
+++ b/include/grub/efi/pe32.h
@@ -45,6 +45,8 @@
 
 #define GRUB_PE32_MSDOS_STUB_SIZE	0x80
 
+#define GRUB_PE32_MAGIC			0x5a4d
+
 /* According to the spec, the minimal alignment is 512 bytes...
    But some examples (such as EFI drivers in the Intel
    Sample Implementation) use 32 bytes (0x20) instead, and it seems
-- 
2.11.0

[PATCH 5/7] arm: reuse arm64 linux loader on efi systems

[Leif Lindholm]

The original 32-bit arm EFI Linux loader reused the 32-bit Linux
loader for U-Boot. However, this meant it was acting in an
entirely not UEFI-compliant fashion.

Since EFI stub loader support for arm went into upstream Linux
for 4.5, we can now reuse the same loader as is used on arm64.

This results in some now-redundant code being dropped from the U-Boot
linux loader, and the arm efi port in general. This also drops the
ability to boot kernels without the efi stub support on 32-bit arm efi
grub.

Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
---
 grub-core/Makefile.am         |   1 -
 grub-core/Makefile.core.def   |   6 +-
 grub-core/kern/arm/efi/misc.c | 202 ------------------------------------------
 grub-core/loader/arm/linux.c  |  39 ++------
 include/grub/arm/efi/loader.h |  26 ------
 include/grub/arm/efi/memory.h |   3 +
 include/grub/arm/linux.h      |  30 +++----
 7 files changed, 22 insertions(+), 285 deletions(-)
 delete mode 100644 grub-core/kern/arm/efi/misc.c
 delete mode 100644 include/grub/arm/efi/loader.h

diff --git a/grub-core/Makefile.am b/grub-core/Makefile.am
index 622f946f5..cd3424c07 100644
--- a/grub-core/Makefile.am
+++ b/grub-core/Makefile.am
@@ -256,7 +256,6 @@ KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/fdtbus.h
 endif
 
 if COND_arm_efi
-KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/arm/efi/loader.h
 KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/efi/efi.h
 KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/efi/disk.h
 KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/arm/system.h
diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
index a65c27f7f..87f80d316 100644
--- a/grub-core/Makefile.core.def
+++ b/grub-core/Makefile.core.def
@@ -229,7 +229,6 @@ kernel = {
   ia64_efi = kern/ia64/cache.c;
 
   arm_efi = kern/arm/efi/init.c;
-  arm_efi = kern/arm/efi/misc.c;
   arm_efi = kern/efi/fdt.c;
 
   arm64_efi = kern/arm64/efi/init.c;
@@ -1698,7 +1697,8 @@ module = {
   powerpc_ieee1275 = loader/powerpc/ieee1275/linux.c;
   sparc64_ieee1275 = loader/sparc64/ieee1275/linux.c;
   ia64_efi = loader/ia64/efi/linux.c;
-  arm = loader/arm/linux.c;
+  arm_uboot = loader/arm/linux.c;
+  arm_efi = loader/arm64/linux.c;
   arm64 = loader/arm64/linux.c;
   common = loader/linux.c;
   common = lib/cmdline.c;
@@ -1707,7 +1707,7 @@ module = {
 
 module = {
   name = fdt;
-  arm64 = loader/efi/fdt.c;
+  efi = loader/efi/fdt.c;
   common = lib/fdt.c;
   enable = fdt;
 };
diff --git a/grub-core/kern/arm/efi/misc.c b/grub-core/kern/arm/efi/misc.c
deleted file mode 100644
index 7cd41842a..000000000
--- a/grub-core/kern/arm/efi/misc.c
+++ /dev/null
@@ -1,202 +0,0 @@
-/* misc.c - various system functions for an arm-based EFI system */
-/*
- *  GRUB  --  GRand Unified Bootloader
- *  Copyright (C) 2013 Free Software Foundation, Inc.
- *
- *  GRUB is free software: you can redistribute it and/or modify
- *  it under the terms of the GNU General Public License as published by
- *  the Free Software Foundation, either version 3 of the License, or
- *  (at your option) any later version.
- *
- *  GRUB is distributed in the hope that it will be useful,
- *  but WITHOUT ANY WARRANTY; without even the implied warranty of
- *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- *  GNU General Public License for more details.
- *
- *  You should have received a copy of the GNU General Public License
- *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
- */
-
-#include <grub/misc.h>
-#include <grub/mm.h>
-#include <grub/cpu/linux.h>
-#include <grub/cpu/system.h>
-#include <grub/efi/efi.h>
-#include <grub/machine/loader.h>
-
-static inline grub_size_t
-page_align (grub_size_t size)
-{
-  return (size + (1 << 12) - 1) & (~((1 << 12) - 1));
-}
-
-/* Find the optimal number of pages for the memory map. Is it better to
-   move this code to efi/mm.c?  */
-static grub_efi_uintn_t
-find_mmap_size (void)
-{
-  static grub_efi_uintn_t mmap_size = 0;
-
-  if (mmap_size != 0)
-    return mmap_size;
-  
-  mmap_size = (1 << 12);
-  while (1)
-    {
-      int ret;
-      grub_efi_memory_descriptor_t *mmap;
-      grub_efi_uintn_t desc_size;
-      
-      mmap = grub_malloc (mmap_size);
-      if (! mmap)
-	return 0;
-
-      ret = grub_efi_get_memory_map (&mmap_size, mmap, 0, &desc_size, 0);
-      grub_free (mmap);
-      
-      if (ret < 0)
-	{
-	  grub_error (GRUB_ERR_IO, "cannot get memory map");
-	  return 0;
-	}
-      else if (ret > 0)
-	break;
-
-      mmap_size += (1 << 12);
-    }
-
-  /* Increase the size a bit for safety, because GRUB allocates more on
-     later, and EFI itself may allocate more.  */
-  mmap_size += (1 << 12);
-
-  return page_align (mmap_size);
-}
-
-#define NEXT_MEMORY_DESCRIPTOR(desc, size)      \
-  ((grub_efi_memory_descriptor_t *) ((char *) (desc) + (size)))
-#define PAGE_SHIFT 12
-
-void *
-grub_efi_allocate_loader_memory (grub_uint32_t min_offset, grub_uint32_t size)
-{
-  grub_efi_uintn_t desc_size;
-  grub_efi_memory_descriptor_t *mmap, *mmap_end;
-  grub_efi_uintn_t mmap_size, tmp_mmap_size;
-  grub_efi_memory_descriptor_t *desc;
-  void *mem = NULL;
-  grub_addr_t min_start = 0;
-
-  mmap_size = find_mmap_size();
-  if (!mmap_size)
-    return NULL;
-
-  mmap = grub_malloc(mmap_size);
-  if (!mmap)
-    return NULL;
-
-  tmp_mmap_size = mmap_size;
-  if (grub_efi_get_memory_map (&tmp_mmap_size, mmap, 0, &desc_size, 0) <= 0)
-    {
-      grub_error (GRUB_ERR_IO, "cannot get memory map");
-      goto fail;
-    }
-
-  mmap_end = NEXT_MEMORY_DESCRIPTOR (mmap, tmp_mmap_size);
-  /* Find lowest accessible RAM location */
-  {
-    int found = 0;
-    for (desc = mmap ; !found && (desc < mmap_end) ;
-	 desc = NEXT_MEMORY_DESCRIPTOR(desc, desc_size))
-      {
-	switch (desc->type)
-	  {
-	  case GRUB_EFI_CONVENTIONAL_MEMORY:
-	  case GRUB_EFI_LOADER_CODE:
-	  case GRUB_EFI_LOADER_DATA:
-	    min_start = desc->physical_start + min_offset;
-	    found = 1;
-	    break;
-	  default:
-	    break;
-	  }
-      }
-  }
-
-  /* First, find free pages for the real mode code
-     and the memory map buffer.  */
-  for (desc = mmap ; desc < mmap_end ;
-       desc = NEXT_MEMORY_DESCRIPTOR(desc, desc_size))
-    {
-      grub_uint64_t start, end;
-
-      grub_dprintf("mm", "%s: 0x%08x bytes @ 0x%08x\n",
-		   __FUNCTION__,
-		   (grub_uint32_t) (desc->num_pages << PAGE_SHIFT),
-		   (grub_uint32_t) (desc->physical_start));
-
-      if (desc->type != GRUB_EFI_CONVENTIONAL_MEMORY)
-	continue;
-
-      start = desc->physical_start;
-      end = start + (desc->num_pages << PAGE_SHIFT);
-      grub_dprintf("mm", "%s: start=0x%016llx, end=0x%016llx\n",
-		  __FUNCTION__, start, end);
-      start = start < min_start ? min_start : start;
-      if (start + size > end)
-	continue;
-      grub_dprintf("mm", "%s: let's allocate some (0x%x) pages @ 0x%08x...\n",
-		  __FUNCTION__, (size >> PAGE_SHIFT), (grub_addr_t) start);
-      mem = grub_efi_allocate_pages (start, (size >> PAGE_SHIFT) + 1);
-      grub_dprintf("mm", "%s: retval=0x%08x\n",
-		   __FUNCTION__, (grub_addr_t) mem);
-      if (! mem)
-	{
-	  grub_error (GRUB_ERR_OUT_OF_MEMORY, "cannot allocate memory");
-	  goto fail;
-	}
-      break;
-    }
-
-  if (! mem)
-    {
-      grub_error (GRUB_ERR_OUT_OF_MEMORY, "cannot allocate memory");
-      goto fail;
-    }
-
-  grub_free (mmap);
-  return mem;
-
- fail:
-  grub_free (mmap);
-  return NULL;
-}
-
-grub_err_t
-grub_efi_prepare_platform (void)
-{
-  grub_efi_uintn_t mmap_size;
-  grub_efi_uintn_t map_key;
-  grub_efi_uintn_t desc_size;
-  grub_efi_uint32_t desc_version;
-  grub_efi_memory_descriptor_t *mmap_buf;
-  grub_err_t err;
-
-  /*
-   * Cloned from IA64
-   * Must be done after grub_machine_fini because map_key is used by
-   *exit_boot_services.
-   */
-  mmap_size = find_mmap_size ();
-  if (! mmap_size)
-    return GRUB_ERR_OUT_OF_MEMORY;
-  mmap_buf = grub_efi_allocate_pages (0, page_align (mmap_size) >> 12);
-  if (! mmap_buf)
-    return GRUB_ERR_OUT_OF_MEMORY;
-
-  err = grub_efi_finish_boot_services (&mmap_size, mmap_buf, &map_key,
-				       &desc_size, &desc_version);
-  if (err != GRUB_ERR_NONE)
-    return err;
-
-  return GRUB_ERR_NONE;
-}
diff --git a/grub-core/loader/arm/linux.c b/grub-core/loader/arm/linux.c
index 260cbf068..8d50f77e9 100644
--- a/grub-core/loader/arm/linux.c
+++ b/grub-core/loader/arm/linux.c
@@ -46,9 +46,6 @@ static const void *current_fdt;
 
 typedef void (*kernel_entry_t) (int, unsigned long, void *);
 
-#define LINUX_ZIMAGE_OFFSET	0x24
-#define LINUX_ZIMAGE_MAGIC	0x016f2818
-
 #define LINUX_PHYS_OFFSET        (0x00008000)
 #define LINUX_INITRD_PHYS_OFFSET (LINUX_PHYS_OFFSET + 0x02000000)
 #define LINUX_FDT_PHYS_OFFSET    (LINUX_INITRD_PHYS_OFFSET - 0x10000)
@@ -293,15 +290,6 @@ linux_boot (void)
    */
   linuxmain = (kernel_entry_t) linux_addr;
 
-#ifdef GRUB_MACHINE_EFI
-  {
-    grub_err_t err;
-    err = grub_efi_prepare_platform();
-    if (err != GRUB_ERR_NONE)
-      return err;
-  }
-#endif
-
   grub_arm_disable_caches_mmu ();
 
   linuxmain (0, machine_type, target_fdt);
@@ -315,17 +303,12 @@ linux_boot (void)
 static grub_err_t
 linux_load (const char *filename, grub_file_t file)
 {
+  struct grub_linux_kernel_header *lh;
   int size;
 
   size = grub_file_size (file);
 
-#ifdef GRUB_MACHINE_EFI
-  linux_addr = (grub_addr_t) grub_efi_allocate_loader_memory (LINUX_PHYS_OFFSET, size);
-  if (!linux_addr)
-    return grub_errno;
-#else
   linux_addr = LINUX_ADDRESS;
-#endif
   grub_dprintf ("loader", "Loading Linux to 0x%08x\n",
 		(grub_addr_t) linux_addr);
 
@@ -337,9 +320,10 @@ linux_load (const char *filename, grub_file_t file)
       return grub_errno;
     }
 
-  if (size > LINUX_ZIMAGE_OFFSET + 4
-      && *(grub_uint32_t *) (linux_addr + LINUX_ZIMAGE_OFFSET)
-      == LINUX_ZIMAGE_MAGIC)
+  lh = (void *) linux_addr;
+
+  if ((grub_size_t) size > sizeof (*lh) &&
+      lh->magic == GRUB_LINUX_MAGIC_SIGNATURE)
     ;
   else if (size > 0x8000 && *(grub_uint32_t *) (linux_addr) == 0xea000006
 	   && machine_type == GRUB_ARM_MACHINE_TYPE_RASPBERRY_PI)
@@ -429,20 +413,7 @@ grub_cmd_initrd (grub_command_t cmd __attribute__ ((unused)),
 
   size = grub_get_initrd_size (&initrd_ctx);
 
-#ifdef GRUB_MACHINE_EFI
-  if (initrd_start)
-    grub_efi_free_pages (initrd_start,
-			 (initrd_end - initrd_start + 0xfff) >> 12);
-  initrd_start = (grub_addr_t) grub_efi_allocate_loader_memory (LINUX_INITRD_PHYS_OFFSET, size);
-
-  if (!initrd_start)
-    {
-      grub_error (GRUB_ERR_OUT_OF_MEMORY, N_("out of memory"));
-      goto fail;
-    }
-#else
   initrd_start = LINUX_INITRD_ADDRESS;
-#endif
 
   grub_dprintf ("loader", "Loading initrd to 0x%08x\n",
 		(grub_addr_t) initrd_start);
diff --git a/include/grub/arm/efi/loader.h b/include/grub/arm/efi/loader.h
deleted file mode 100644
index 4bab18e83..000000000
--- a/include/grub/arm/efi/loader.h
+++ /dev/null
@@ -1,26 +0,0 @@
-/*
- *  GRUB  --  GRand Unified Bootloader
- *  Copyright (C) 2013  Free Software Foundation, Inc.
- *
- *  GRUB is free software: you can redistribute it and/or modify
- *  it under the terms of the GNU General Public License as published by
- *  the Free Software Foundation, either version 3 of the License, or
- *  (at your option) any later version.
- *
- *  GRUB is distributed in the hope that it will be useful,
- *  but WITHOUT ANY WARRANTY; without even the implied warranty of
- *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- *  GNU General Public License for more details.
- *
- *  You should have received a copy of the GNU General Public License
- *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
- */
-
-#ifndef GRUB_LOADER_MACHINE_HEADER
-#define GRUB_LOADER_MACHINE_HEADER	1
-
-grub_err_t EXPORT_FUNC (grub_efi_prepare_platform) (void);
-void * EXPORT_FUNC (grub_efi_allocate_loader_memory) (grub_uint32_t min_offset,
-						      grub_uint32_t size);
-
-#endif /* ! GRUB_LOADER_MACHINE_HEADER */
diff --git a/include/grub/arm/efi/memory.h b/include/grub/arm/efi/memory.h
index 2c64918e3..986f656d6 100644
--- a/include/grub/arm/efi/memory.h
+++ b/include/grub/arm/efi/memory.h
@@ -3,4 +3,7 @@
 
 #define GRUB_EFI_MAX_USABLE_ADDRESS 0xffffffff
 
+#define GRUB_EFI_PAGE_SHIFT	12
+#define GRUB_EFI_BYTES_TO_PAGES(bytes)   (((bytes) + 0xfff) >> GRUB_EFI_PAGE_SHIFT)
+
 #endif /* ! GRUB_MEMORY_CPU_HEADER */
diff --git a/include/grub/arm/linux.h b/include/grub/arm/linux.h
index f217f8281..98ff4c004 100644
--- a/include/grub/arm/linux.h
+++ b/include/grub/arm/linux.h
@@ -20,32 +20,26 @@
 #ifndef GRUB_LINUX_CPU_HEADER
 #define GRUB_LINUX_CPU_HEADER 1
 
-#define LINUX_ZIMAGE_OFFSET 0x24
-#define LINUX_ZIMAGE_MAGIC  0x016f2818
+#define GRUB_LINUX_MAGIC_SIGNATURE 0x016f2818
 
-#include "system.h"
+struct grub_linux_kernel_header {
+  grub_uint32_t code0;
+  grub_uint32_t reserved1[8];
+  grub_uint32_t magic;
+  grub_uint32_t start; /* _start */
+  grub_uint32_t end;   /* _edata */
+  grub_uint32_t reserved2[4];
+  grub_uint32_t hdr_offset;
+};
 
 #if defined GRUB_MACHINE_UBOOT
+# include "system.h"
 # include <grub/uboot/uboot.h>
 # define LINUX_ADDRESS        (start_of_ram + 0x8000)
 # define LINUX_INITRD_ADDRESS (start_of_ram + 0x02000000)
 # define LINUX_FDT_ADDRESS    (LINUX_INITRD_ADDRESS - 0x10000)
 # define grub_arm_firmware_get_boot_data grub_uboot_get_boot_data
 # define grub_arm_firmware_get_machine_type grub_uboot_get_machine_type
-#elif defined GRUB_MACHINE_EFI
-# include <grub/efi/efi.h>
-# include <grub/machine/loader.h>
-/* On UEFI platforms - load the images at the lowest available address not
-   less than *_PHYS_OFFSET from the first available memory location. */
-# define LINUX_PHYS_OFFSET        (0x00008000)
-# define LINUX_INITRD_PHYS_OFFSET (LINUX_PHYS_OFFSET + 0x02000000)
-# define LINUX_FDT_PHYS_OFFSET    (LINUX_INITRD_PHYS_OFFSET - 0x10000)
-# define grub_arm_firmware_get_boot_data (grub_addr_t)grub_efi_get_firmware_fdt
-static inline grub_uint32_t
-grub_arm_firmware_get_machine_type (void)
-{
-  return GRUB_ARM_MACHINE_TYPE_FDT;
-}
 #elif defined (GRUB_MACHINE_COREBOOT)
 #include <grub/fdtbus.h>
 #include <grub/machine/kernel.h>
@@ -64,6 +58,4 @@ grub_arm_firmware_get_machine_type (void)
 }
 #endif
 
-#define FDT_ADDITIONAL_ENTRIES_SIZE	0x300
-
 #endif /* ! GRUB_LINUX_CPU_HEADER */
-- 
2.11.0

[PATCH 6/7] efi: restrict arm/arm64 linux loader initrd placement

[Leif Lindholm]

The 32-bit arm Linux kernel is built as a zImage, which self-decompresses
down to near start of RAM. In order for an initrd/initramfs to be
accessible, it needs to be placed within the first ~768MB of RAM.
The initrd loader built into the kernel EFI stub restricts this down to
512MB for simplicity - so enable the same restriction in grub.

For arm64, the requirement is within a 1GB aligned 32GB window also
covering the (runtime) kernel image. Since the EFI stub loader itself
will attempt to relocate to near start of RAM, force initrd to be loaded
completely within the first 32GB of RAM.

Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
---
 grub-core/loader/arm64/linux.c | 39 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 38 insertions(+), 1 deletion(-)

diff --git a/grub-core/loader/arm64/linux.c b/grub-core/loader/arm64/linux.c
index 8cd44230d..7e989c2b9 100644
--- a/grub-core/loader/arm64/linux.c
+++ b/grub-core/loader/arm64/linux.c
@@ -35,6 +35,23 @@
 
 GRUB_MOD_LICENSE ("GPLv3+");
 
+/*
+ * As per linux/Documentation/arm/Booting
+ * ARM initrd needs to be covered by kernel linear mapping,
+ * so place it in the first 512MB of DRAM.
+ *
+ * As per linux/Documentation/arm64/booting.txt
+ * ARM64 initrd needs to be contained entirely within a 1GB aligned window
+ * of up to 32GB of size that covers the kernel image as well.
+ * Since the EFI stub loader will attempt to load the kernel near start of
+ * RAM, place the buffer in the first 32GB of RAM.
+ */
+#ifdef __arm__
+#define INITRD_MAX_ADDRESS_OFFSET (512U * 1024 * 1024)
+#else /* __aarch64__ */
+#define INITRD_MAX_ADDRESS_OFFSET (32ULL * 1024 * 1024 * 1024)
+#endif
+
 static grub_dl_t my_mod;
 static int loaded;
 
@@ -194,6 +211,25 @@ grub_linux_unload (void)
   return GRUB_ERR_NONE;
 }
 
+/*
+ * This function returns a pointer to a legally allocated initrd buffer,
+ * or NULL if unsuccessful
+ */
+static void *
+allocate_initrd_mem (int initrd_pages)
+{
+  grub_addr_t max_addr;
+
+  if (grub_efi_get_dram_base (&max_addr) != GRUB_ERR_NONE)
+    return NULL;
+
+  max_addr += INITRD_MAX_ADDRESS_OFFSET - 1;
+
+  return grub_efi_allocate_pages_real (max_addr, initrd_pages,
+				       GRUB_EFI_ALLOCATE_MAX_ADDRESS,
+				       GRUB_EFI_LOADER_DATA);
+}
+
 static grub_err_t
 grub_cmd_initrd (grub_command_t cmd __attribute__ ((unused)),
 		 int argc, char *argv[])
@@ -222,7 +258,8 @@ grub_cmd_initrd (grub_command_t cmd __attribute__ ((unused)),
   grub_dprintf ("linux", "Loading initrd\n");
 
   initrd_pages = (GRUB_EFI_BYTES_TO_PAGES (initrd_size));
-  initrd_mem = grub_efi_allocate_pages (0, initrd_pages);
+  initrd_mem = allocate_initrd_mem (initrd_pages);
+
   if (!initrd_mem)
     {
       grub_error (GRUB_ERR_OUT_OF_MEMORY, N_("out of memory"));
-- 
2.11.0

[PATCH 7/7] efi: change heap allocation type to GRUB_EFI_LOADER_CODE

[Leif Lindholm]

With upcoming changes to EDK2, allocations of type EFI_LOADER_DATA may
not return regions with execute ability. Since modules are loaded onto
the heap, change the heap allocation type to GRUB_EFI_LOADER_CODE in
order to permit execution on systems with this feature enabled.

Closes: 50420

Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
---
 grub-core/kern/efi/mm.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/grub-core/kern/efi/mm.c b/grub-core/kern/efi/mm.c
index 7b1763bc5..f27a48e68 100644
--- a/grub-core/kern/efi/mm.c
+++ b/grub-core/kern/efi/mm.c
@@ -404,7 +404,9 @@ add_memory_regions (grub_efi_memory_descriptor_t *memory_map,
 	  pages = required_pages;
 	}
 
-      addr = grub_efi_allocate_pages (start, pages);
+      addr = grub_efi_allocate_pages_real (start, pages,
+					   GRUB_EFI_ALLOCATE_ADDRESS,
+					   GRUB_EFI_LOADER_CODE);
       if (! addr)
 	grub_fatal ("cannot allocate conventional memory %p with %u pages",
 		    (void *) ((grub_addr_t) start),
-- 
2.11.0

Re: [PATCH 0/7] efi: improved correctness, arm unification, and cleanup

[Daniel Kiper]

On Mon, Jun 12, 2017 at 03:53:34PM +0100, Leif Lindholm wrote:
> This patch series is really three different ones, but they unite around
> the need for (and the implementation) of more flexible control of memory
> allocation on UEFI systems.

Thanks for posting this. If nobody takes care then I will take a look in
2-4 weeks. I am busy with other stuff right now. Sorry for delays.

Daniel

PS Next time please CC me.

Re: [PATCH 1/7] efi: add grub_efi_get_dram_base() function for arm*

[Daniel Kiper]

On Mon, Jun 12, 2017 at 03:53:35PM +0100, Leif Lindholm wrote:
> Since ARM platforms do not have a common memory map, add a helper
> function that finds the lowest address region with the EFI_MEMORY_WB
> attribute set in the UEFI memory map.
>
> Required for the arm/arm64 linux loader to restrict the initrd
> location to where it will be accessible by the kernel at runtime.
>
> Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
> ---
>  grub-core/kern/efi/mm.c | 42 ++++++++++++++++++++++++++++++++++++++++++
>  include/grub/efi/efi.h  |  1 +
>  2 files changed, 43 insertions(+)
>
> diff --git a/grub-core/kern/efi/mm.c b/grub-core/kern/efi/mm.c
> index 20a47aaf5..460a4b763 100644
> --- a/grub-core/kern/efi/mm.c
> +++ b/grub-core/kern/efi/mm.c
> @@ -525,3 +525,45 @@ grub_efi_mm_init (void)
>    grub_efi_free_pages ((grub_addr_t) memory_map,
>  		       2 * BYTES_TO_PAGES (MEMORY_MAP_SIZE));
>  }
> +
> +#if defined (__arm__) || defined (__aarch64__)
> +grub_err_t
> +grub_efi_get_dram_base(grub_addr_t *base_addr)

Please make this function more generic and get
attribute as an argument.

> +{
> +  grub_efi_memory_descriptor_t *memory_map;
> +  grub_efi_memory_descriptor_t *desc;
> +  grub_efi_uintn_t mmap_size;
> +  grub_efi_uintn_t desc_size;
> +
> +  mmap_size = (1 << GRUB_EFI_PAGE_SHIFT);

Could not you define GRUB_EFI_PAGE and use it here?
And why GRUB_EFI_PAGE_SHIFT and other stuff is defined
in include/grub/arm64/fdtload.h? It looks like generic
thing and should be in genric place too. Please fix it
if possible.

> +  while (1)
> +    {
> +      int ret;
> +
> +      memory_map = grub_malloc (mmap_size);
> +      if (! memory_map)
> +        return GRUB_ERR_OUT_OF_MEMORY;
> +      ret = grub_efi_get_memory_map (&mmap_size, memory_map, NULL,
> +				     &desc_size, NULL);
> +      if (ret > 0)
> +	break;
> +
> +      grub_free (memory_map);
> +      if (ret == 0)
> +	return GRUB_ERR_BUG;
> +
> +      mmap_size += (1 << GRUB_EFI_PAGE_SHIFT);
> +    }

Hmmm... This is not optimal. Please take a look at e.g.
grub_efi_finish_boot_services() how buffer for memory
map should be allocated.

And going further... Could you take a look at
grub_relocator_alloc_chunk_align() and
grub_relocator_alloc_chunk_addr(). Good example
is in grub-core/loader/multiboot_mbi2.c and
grub-core/loader/multiboot_elfxx.c. If you
use this machinery then there is a chance
that you do not duplicate code so much.

Daniel

Re: [PATCH 2/7] efi: refactor grub_efi_allocate_pages

[Daniel Kiper]

On Mon, Jun 12, 2017 at 03:53:36PM +0100, Leif Lindholm wrote:
> Expose a new function, grub_efi_allocate_pages_real(), making it possible
> to specify allocation type and memory type as supported by the UEFI
> AllocatePages boot service.
>
> Make grub_efi_allocate_pages() a consumer of the new function,
> maintaining its old functionality.
>
> Also delete some left-around #if 1/#else blocks in the affected
> functions.
>
> Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
> ---
>  grub-core/kern/efi/mm.c | 46 ++++++++++++++++++++++++----------------------
>  include/grub/efi/efi.h  |  5 +++++
>  2 files changed, 29 insertions(+), 22 deletions(-)
>
> diff --git a/grub-core/kern/efi/mm.c b/grub-core/kern/efi/mm.c
> index 460a4b763..7b1763bc5 100644
> --- a/grub-core/kern/efi/mm.c
> +++ b/grub-core/kern/efi/mm.c
> @@ -51,36 +51,20 @@ int grub_efi_is_finished = 0;
>
>  /* Allocate pages. Return the pointer to the first of allocated pages.  */
>  void *
> -grub_efi_allocate_pages (grub_efi_physical_address_t address,
> -			 grub_efi_uintn_t pages)
> +grub_efi_allocate_pages_real (grub_efi_physical_address_t address,

Could not you just add an extra argument to grub_efi_allocate_pages()?
It is not called in many places. Do you really need separate
grub_efi_allocate_pages_real()?

Daniel

Re: [PATCH 3/7] efi: move fdt helper library

[Daniel Kiper]

On Mon, Jun 12, 2017 at 03:53:37PM +0100, Leif Lindholm wrote:
> There is nothing ARM64 (or even ARM) specific about the efi fdt helper
> library, which is used for locating or overriding a firmware-provided
> devicetree in a UEFI system - so move it to loader/efi for reuse.

OK.

> Move the fdtload.h include file to grub/efi

This begs one patch...

> and move the EFI page size
> definitions to grub/efi/memory.h. (These definitions refer strictly to
> allocation operations, as opposed to translation granules.)

Great! This is what I requested earlier. However, please put this in separate patch.

> ---
>  grub-core/Makefile.core.def           | 2 +-
>  grub-core/loader/arm64/linux.c        | 3 ++-
>  grub-core/loader/arm64/xen_boot.c     | 3 ++-
>  grub-core/loader/{arm64 => efi}/fdt.c | 3 ++-
>  include/grub/{arm64 => efi}/fdtload.h | 3 ---
>  include/grub/efi/memory.h             | 3 +++
>  6 files changed, 10 insertions(+), 7 deletions(-)
>  rename grub-core/loader/{arm64 => efi}/fdt.c (98%)
>  rename include/grub/{arm64 => efi}/fdtload.h (89%)
>
> diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
> index 1d86bd22e..a65c27f7f 100644
> --- a/grub-core/Makefile.core.def
> +++ b/grub-core/Makefile.core.def
> @@ -1707,7 +1707,7 @@ module = {
>
>  module = {
>    name = fdt;
> -  arm64 = loader/arm64/fdt.c;
> +  arm64 = loader/efi/fdt.c;
>    common = lib/fdt.c;
>    enable = fdt;
>  };
> diff --git a/grub-core/loader/arm64/linux.c b/grub-core/loader/arm64/linux.c
> index 9519d2e4d..cac94d53d 100644
> --- a/grub-core/loader/arm64/linux.c
> +++ b/grub-core/loader/arm64/linux.c
> @@ -26,8 +26,9 @@
>  #include <grub/mm.h>
>  #include <grub/types.h>
>  #include <grub/cpu/linux.h>
> -#include <grub/cpu/fdtload.h>
>  #include <grub/efi/efi.h>
> +#include <grub/efi/fdtload.h>
> +#include <grub/efi/memory.h>

Why do you shuffle and add headers?

>  #include <grub/efi/pe32.h>
>  #include <grub/i18n.h>
>  #include <grub/lib/cmdline.h>
> diff --git a/grub-core/loader/arm64/xen_boot.c b/grub-core/loader/arm64/xen_boot.c
> index 27ede46ca..d092a53ed 100644
> --- a/grub-core/loader/arm64/xen_boot.c
> +++ b/grub-core/loader/arm64/xen_boot.c
> @@ -27,9 +27,10 @@
>  #include <grub/misc.h>
>  #include <grub/mm.h>
>  #include <grub/types.h>
> -#include <grub/cpu/fdtload.h>
>  #include <grub/cpu/linux.h>
>  #include <grub/efi/efi.h>
> +#include <grub/efi/fdtload.h>
> +#include <grub/efi/memory.h>

Ditto?

>  #include <grub/efi/pe32.h>	/* required by struct xen_hypervisor_header */
>  #include <grub/i18n.h>
>  #include <grub/lib/cmdline.h>
> diff --git a/grub-core/loader/arm64/fdt.c b/grub-core/loader/efi/fdt.c
> similarity index 98%
> rename from grub-core/loader/arm64/fdt.c
> rename to grub-core/loader/efi/fdt.c
> index db49cf649..be369fd9d 100644
> --- a/grub-core/loader/arm64/fdt.c
> +++ b/grub-core/loader/efi/fdt.c
> @@ -18,12 +18,13 @@
>
>  #include <grub/fdt.h>
>  #include <grub/mm.h>
> -#include <grub/cpu/fdtload.h>
>  #include <grub/err.h>
>  #include <grub/dl.h>
>  #include <grub/command.h>
>  #include <grub/file.h>
>  #include <grub/efi/efi.h>
> +#include <grub/efi/fdtload.h>
> +#include <grub/machine/memory.h>

Ditto?

>  static void *loaded_fdt;
>  static void *fdt;
> diff --git a/include/grub/arm64/fdtload.h b/include/grub/efi/fdtload.h
> similarity index 89%
> rename from include/grub/arm64/fdtload.h
> rename to include/grub/efi/fdtload.h
> index 7b9ddba91..713c9424d 100644
> --- a/include/grub/arm64/fdtload.h
> +++ b/include/grub/efi/fdtload.h
> @@ -29,7 +29,4 @@ grub_fdt_unload (void);
>  grub_err_t
>  grub_fdt_install (void);
>
> -#define GRUB_EFI_PAGE_SHIFT	12
> -#define GRUB_EFI_BYTES_TO_PAGES(bytes)   (((bytes) + 0xfff) >> GRUB_EFI_PAGE_SHIFT)
> -
>  #endif
> diff --git a/include/grub/efi/memory.h b/include/grub/efi/memory.h
> index 20526b146..0eb0b70b6 100644
> --- a/include/grub/efi/memory.h
> +++ b/include/grub/efi/memory.h
> @@ -22,6 +22,9 @@
>  #include <grub/err.h>
>  #include <grub/types.h>
>
> +#define GRUB_EFI_PAGE_SHIFT	12
> +#define GRUB_EFI_BYTES_TO_PAGES(bytes)   (((bytes) + 0xfff) >> GRUB_EFI_PAGE_SHIFT)

If you move this then fix the aligment. I mean more or less this:

#define GRUB_EFI_PAGE_SHIFT              12
#define GRUB_EFI_BYTES_TO_PAGES(bytes)   (((bytes) + 0xfff) >> GRUB_EFI_PAGE_SHIFT)

#define GRUB_MMAP_REGISTER_BY_FIRMWARE   1

Daniel

Re: [PATCH 4/7] arm64: make efi linux loader more generic

[Daniel Kiper]

On Mon, Jun 12, 2017 at 03:53:38PM +0100, Leif Lindholm wrote:
> In order to enable reuse of the arm64 efi linux loader for arm,
> change a few function names and macros. Add a global definition
> of GRUB_PE32_MAGIC in grub/efi/pe32.h.
>
> Make the arm64 efi loader (and fdt helpers) 32/64-bit safe.
>
> Also update the arm64 xen loader, since it depends on some of the
> functions in the linux loader.

Please, please, please one logical change in the patch. Otherwise
this does not parse.

Daniel

Re: [PATCH 5/7] arm: reuse arm64 linux loader on efi systems

[Daniel Kiper]

On Mon, Jun 12, 2017 at 03:53:39PM +0100, Leif Lindholm wrote:
> The original 32-bit arm EFI Linux loader reused the 32-bit Linux
> loader for U-Boot. However, this meant it was acting in an
> entirely not UEFI-compliant fashion.
>
> Since EFI stub loader support for arm went into upstream Linux
> for 4.5, we can now reuse the same loader as is used on arm64.
>
> This results in some now-redundant code being dropped from the U-Boot
> linux loader, and the arm efi port in general. This also drops the
> ability to boot kernels without the efi stub support on 32-bit arm efi
> grub.

I have a feeling that this patch does more than one thing. Please
split it if possible.

> Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
> ---
>  grub-core/Makefile.am         |   1 -
>  grub-core/Makefile.core.def   |   6 +-
>  grub-core/kern/arm/efi/misc.c | 202 ------------------------------------------
>  grub-core/loader/arm/linux.c  |  39 ++------
>  include/grub/arm/efi/loader.h |  26 ------
>  include/grub/arm/efi/memory.h |   3 +
>  include/grub/arm/linux.h      |  30 +++----
>  7 files changed, 22 insertions(+), 285 deletions(-)
>  delete mode 100644 grub-core/kern/arm/efi/misc.c
>  delete mode 100644 include/grub/arm/efi/loader.h

[...]

> diff --git a/include/grub/arm/efi/memory.h b/include/grub/arm/efi/memory.h
> index 2c64918e3..986f656d6 100644
> --- a/include/grub/arm/efi/memory.h
> +++ b/include/grub/arm/efi/memory.h
> @@ -3,4 +3,7 @@
>
>  #define GRUB_EFI_MAX_USABLE_ADDRESS 0xffffffff
>
> +#define GRUB_EFI_PAGE_SHIFT	12
> +#define GRUB_EFI_BYTES_TO_PAGES(bytes)   (((bytes) + 0xfff) >> GRUB_EFI_PAGE_SHIFT)

Hmmm... It looks familiar. Why do you duplicate this?

> +
>  #endif /* ! GRUB_MEMORY_CPU_HEADER */
> diff --git a/include/grub/arm/linux.h b/include/grub/arm/linux.h
> index f217f8281..98ff4c004 100644
> --- a/include/grub/arm/linux.h
> +++ b/include/grub/arm/linux.h
> @@ -20,32 +20,26 @@
>  #ifndef GRUB_LINUX_CPU_HEADER
>  #define GRUB_LINUX_CPU_HEADER 1
>
> -#define LINUX_ZIMAGE_OFFSET 0x24
> -#define LINUX_ZIMAGE_MAGIC  0x016f2818
> +#define GRUB_LINUX_MAGIC_SIGNATURE 0x016f2818
>
> -#include "system.h"
> +struct grub_linux_kernel_header {
> +  grub_uint32_t code0;
> +  grub_uint32_t reserved1[8];
> +  grub_uint32_t magic;
> +  grub_uint32_t start; /* _start */
> +  grub_uint32_t end;   /* _edata */
> +  grub_uint32_t reserved2[4];
> +  grub_uint32_t hdr_offset;
> +};

???

Daniel

Re: [PATCH 6/7] efi: restrict arm/arm64 linux loader initrd placement

[Daniel Kiper]

On Mon, Jun 12, 2017 at 03:53:40PM +0100, Leif Lindholm wrote:
> The 32-bit arm Linux kernel is built as a zImage, which self-decompresses
> down to near start of RAM. In order for an initrd/initramfs to be
> accessible, it needs to be placed within the first ~768MB of RAM.
> The initrd loader built into the kernel EFI stub restricts this down to
> 512MB for simplicity - so enable the same restriction in grub.
>
> For arm64, the requirement is within a 1GB aligned 32GB window also
> covering the (runtime) kernel image. Since the EFI stub loader itself
> will attempt to relocate to near start of RAM, force initrd to be loaded
> completely within the first 32GB of RAM.
>
> Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
> ---
>  grub-core/loader/arm64/linux.c | 39 ++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 38 insertions(+), 1 deletion(-)
>
> diff --git a/grub-core/loader/arm64/linux.c b/grub-core/loader/arm64/linux.c
> index 8cd44230d..7e989c2b9 100644
> --- a/grub-core/loader/arm64/linux.c
> +++ b/grub-core/loader/arm64/linux.c
> @@ -35,6 +35,23 @@
>
>  GRUB_MOD_LICENSE ("GPLv3+");
>
> +/*
> + * As per linux/Documentation/arm/Booting
> + * ARM initrd needs to be covered by kernel linear mapping,
> + * so place it in the first 512MB of DRAM.
> + *
> + * As per linux/Documentation/arm64/booting.txt
> + * ARM64 initrd needs to be contained entirely within a 1GB aligned window
> + * of up to 32GB of size that covers the kernel image as well.
> + * Since the EFI stub loader will attempt to load the kernel near start of
> + * RAM, place the buffer in the first 32GB of RAM.
> + */
> +#ifdef __arm__
> +#define INITRD_MAX_ADDRESS_OFFSET (512U * 1024 * 1024)
> +#else /* __aarch64__ */
> +#define INITRD_MAX_ADDRESS_OFFSET (32ULL * 1024 * 1024 * 1024)
> +#endif
> +
>  static grub_dl_t my_mod;
>  static int loaded;
>
> @@ -194,6 +211,25 @@ grub_linux_unload (void)
>    return GRUB_ERR_NONE;
>  }
>
> +/*
> + * This function returns a pointer to a legally allocated initrd buffer,
> + * or NULL if unsuccessful
> + */
> +static void *
> +allocate_initrd_mem (int initrd_pages)
> +{
> +  grub_addr_t max_addr;
> +
> +  if (grub_efi_get_dram_base (&max_addr) != GRUB_ERR_NONE)
> +    return NULL;
> +
> +  max_addr += INITRD_MAX_ADDRESS_OFFSET - 1;

I do not understand this. Why do not pass simply INITRD_MAX_ADDRESS_OFFSET
instead of max_addr to grub_efi_allocate_pages_real()?

> +  return grub_efi_allocate_pages_real (max_addr, initrd_pages,
> +				       GRUB_EFI_ALLOCATE_MAX_ADDRESS,
> +				       GRUB_EFI_LOADER_DATA);
> +}

Daniel

Re: [PATCH 6/7] efi: restrict arm/arm64 linux loader initrd placement

[Vladimir 'phcoder' Serbinenko]

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

On Thu, Jul 27, 2017, 17:16 Daniel Kiper <dkiper@net-space.pl> wrote:

> On Mon, Jun 12, 2017 at 03:53:40PM +0100, Leif Lindholm wrote:
> > The 32-bit arm Linux kernel is built as a zImage, which self-decompresses
> > down to near start of RAM. In order for an initrd/initramfs to be
> > accessible, it needs to be placed within the first ~768MB of RAM.
> > The initrd loader built into the kernel EFI stub restricts this down to
> > 512MB for simplicity - so enable the same restriction in grub.
> >
> > For arm64, the requirement is within a 1GB aligned 32GB window also
> > covering the (runtime) kernel image. Since the EFI stub loader itself
> > will attempt to relocate to near start of RAM, force initrd to be loaded
> > completely within the first 32GB of RAM.
> >
> > Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
> > ---
> >  grub-core/loader/arm64/linux.c | 39
> ++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 38 insertions(+), 1 deletion(-)
> >
> > diff --git a/grub-core/loader/arm64/linux.c
> b/grub-core/loader/arm64/linux.c
> > index 8cd44230d..7e989c2b9 100644
> > --- a/grub-core/loader/arm64/linux.c
> > +++ b/grub-core/loader/arm64/linux.c
> > @@ -35,6 +35,23 @@
> >
> >  GRUB_MOD_LICENSE ("GPLv3+");
> >
> > +/*
> > + * As per linux/Documentation/arm/Booting
> > + * ARM initrd needs to be covered by kernel linear mapping,
> > + * so place it in the first 512MB of DRAM.
> > + *
> > + * As per linux/Documentation/arm64/booting.txt
> > + * ARM64 initrd needs to be contained entirely within a 1GB aligned
> window
> > + * of up to 32GB of size that covers the kernel image as well.
> > + * Since the EFI stub loader will attempt to load the kernel near start
> of
> > + * RAM, place the buffer in the first 32GB of RAM.
> > + */
> > +#ifdef __arm__
> > +#define INITRD_MAX_ADDRESS_OFFSET (512U * 1024 * 1024)
> > +#else /* __aarch64__ */
> > +#define INITRD_MAX_ADDRESS_OFFSET (32ULL * 1024 * 1024 * 1024)
> > +#endif
> > +
> >  static grub_dl_t my_mod;
> >  static int loaded;
> >
> > @@ -194,6 +211,25 @@ grub_linux_unload (void)
> >    return GRUB_ERR_NONE;
> >  }
> >
> > +/*
> > + * This function returns a pointer to a legally allocated initrd buffer,
> > + * or NULL if unsuccessful
> > + */
> > +static void *
> > +allocate_initrd_mem (int initrd_pages)
> > +{
> > +  grub_addr_t max_addr;
> > +
> > +  if (grub_efi_get_dram_base (&max_addr) != GRUB_ERR_NONE)
> > +    return NULL;
> > +
> > +  max_addr += INITRD_MAX_ADDRESS_OFFSET - 1;
>
> I do not understand this. Why do not pass simply INITRD_MAX_ADDRESS_OFFSET
> instead of max_addr to grub_efi_allocate_pages_real()?
>
On ARM it's common for RAM to start at address different from 0

>
> > +  return grub_efi_allocate_pages_real (max_addr, initrd_pages,
> > +                                    GRUB_EFI_ALLOCATE_MAX_ADDRESS,
> > +                                    GRUB_EFI_LOADER_DATA);
> > +}
>
> Daniel
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
>

[-- Attachment #2: Type: text/html, Size: 4160 bytes --]

Re: [PATCH 7/7] efi: change heap allocation type to GRUB_EFI_LOADER_CODE

[Daniel Kiper]

On Mon, Jun 12, 2017 at 03:53:41PM +0100, Leif Lindholm wrote:
> With upcoming changes to EDK2, allocations of type EFI_LOADER_DATA may
> not return regions with execute ability. Since modules are loaded onto
> the heap, change the heap allocation type to GRUB_EFI_LOADER_CODE in
> order to permit execution on systems with this feature enabled.
>
> Closes: 50420
>
> Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
> ---
>  grub-core/kern/efi/mm.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/grub-core/kern/efi/mm.c b/grub-core/kern/efi/mm.c
> index 7b1763bc5..f27a48e68 100644
> --- a/grub-core/kern/efi/mm.c
> +++ b/grub-core/kern/efi/mm.c
> @@ -404,7 +404,9 @@ add_memory_regions (grub_efi_memory_descriptor_t *memory_map,
>  	  pages = required_pages;
>  	}
>
> -      addr = grub_efi_allocate_pages (start, pages);
> +      addr = grub_efi_allocate_pages_real (start, pages,
> +					   GRUB_EFI_ALLOCATE_ADDRESS,
> +					   GRUB_EFI_LOADER_CODE);

Is it really needed? Is any module exectued in place or does it contain
code ready to run out of the box?

Daniel

Re: [PATCH 7/7] efi: change heap allocation type to GRUB_EFI_LOADER_CODE

[Vladimir 'phcoder' Serbinenko]

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

On Thu, Jul 27, 2017, 17:30 Daniel Kiper <dkiper@net-space.pl> wrote:

> On Mon, Jun 12, 2017 at 03:53:41PM +0100, Leif Lindholm wrote:
> > With upcoming changes to EDK2, allocations of type EFI_LOADER_DATA may
> > not return regions with execute ability. Since modules are loaded onto
> > the heap, change the heap allocation type to GRUB_EFI_LOADER_CODE in
> > order to permit execution on systems with this feature enabled.
> >
> > Closes: 50420
> >
> > Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
> > ---
> >  grub-core/kern/efi/mm.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/grub-core/kern/efi/mm.c b/grub-core/kern/efi/mm.c
> > index 7b1763bc5..f27a48e68 100644
> > --- a/grub-core/kern/efi/mm.c
> > +++ b/grub-core/kern/efi/mm.c
> > @@ -404,7 +404,9 @@ add_memory_regions (grub_efi_memory_descriptor_t
> *memory_map,
> >         pages = required_pages;
> >       }
> >
> > -      addr = grub_efi_allocate_pages (start, pages);
> > +      addr = grub_efi_allocate_pages_real (start, pages,
> > +                                        GRUB_EFI_ALLOCATE_ADDRESS,
> > +                                        GRUB_EFI_LOADER_CODE);
>
> Is it really needed? Is any module exectued in place or does it contain
> code ready to run out of the box?
>
All the modules are loaded to heap

>
> Daniel
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
>

[-- Attachment #2: Type: text/html, Size: 2335 bytes --]

Re: [PATCH 6/7] efi: restrict arm/arm64 linux loader initrd placement

[Daniel Kiper]

On Thu, Jul 27, 2017 at 03:18:16PM +0000, Vladimir 'phcoder' Serbinenko wrote:
> On Thu, Jul 27, 2017, 17:16 Daniel Kiper <dkiper@net-space.pl> wrote:
> > On Mon, Jun 12, 2017 at 03:53:40PM +0100, Leif Lindholm wrote:
> > > The 32-bit arm Linux kernel is built as a zImage, which self-decompresses
> > > down to near start of RAM. In order for an initrd/initramfs to be
> > > accessible, it needs to be placed within the first ~768MB of RAM.
> > > The initrd loader built into the kernel EFI stub restricts this down to
> > > 512MB for simplicity - so enable the same restriction in grub.
> > >
> > > For arm64, the requirement is within a 1GB aligned 32GB window also
> > > covering the (runtime) kernel image. Since the EFI stub loader itself
> > > will attempt to relocate to near start of RAM, force initrd to be loaded
> > > completely within the first 32GB of RAM.
> > >
> > > Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
> > > ---
> > >  grub-core/loader/arm64/linux.c | 39
> > ++++++++++++++++++++++++++++++++++++++-
> > >  1 file changed, 38 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/grub-core/loader/arm64/linux.c
> > b/grub-core/loader/arm64/linux.c
> > > index 8cd44230d..7e989c2b9 100644
> > > --- a/grub-core/loader/arm64/linux.c
> > > +++ b/grub-core/loader/arm64/linux.c
> > > @@ -35,6 +35,23 @@
> > >
> > >  GRUB_MOD_LICENSE ("GPLv3+");
> > >
> > > +/*
> > > + * As per linux/Documentation/arm/Booting
> > > + * ARM initrd needs to be covered by kernel linear mapping,
> > > + * so place it in the first 512MB of DRAM.
> > > + *
> > > + * As per linux/Documentation/arm64/booting.txt
> > > + * ARM64 initrd needs to be contained entirely within a 1GB aligned
> > window
> > > + * of up to 32GB of size that covers the kernel image as well.
> > > + * Since the EFI stub loader will attempt to load the kernel near start
> > of
> > > + * RAM, place the buffer in the first 32GB of RAM.
> > > + */
> > > +#ifdef __arm__
> > > +#define INITRD_MAX_ADDRESS_OFFSET (512U * 1024 * 1024)
> > > +#else /* __aarch64__ */
> > > +#define INITRD_MAX_ADDRESS_OFFSET (32ULL * 1024 * 1024 * 1024)
> > > +#endif
> > > +
> > >  static grub_dl_t my_mod;
> > >  static int loaded;
> > >
> > > @@ -194,6 +211,25 @@ grub_linux_unload (void)
> > >    return GRUB_ERR_NONE;
> > >  }
> > >
> > > +/*
> > > + * This function returns a pointer to a legally allocated initrd buffer,
> > > + * or NULL if unsuccessful
> > > + */
> > > +static void *
> > > +allocate_initrd_mem (int initrd_pages)
> > > +{
> > > +  grub_addr_t max_addr;
> > > +
> > > +  if (grub_efi_get_dram_base (&max_addr) != GRUB_ERR_NONE)
> > > +    return NULL;
> > > +
> > > +  max_addr += INITRD_MAX_ADDRESS_OFFSET - 1;
> >
> > I do not understand this. Why do not pass simply INITRD_MAX_ADDRESS_OFFSET
> > instead of max_addr to grub_efi_allocate_pages_real()?
> >
> On ARM it's common for RAM to start at address different from 0

OK, but, AIUI we have to load initrd no higher than INITRD_MAX_ADDRESS_OFFSET.
So, we do not care where the region starts. We care where region ends.
Am I missing something?

Daniel

Re: [PATCH 6/7] efi: restrict arm/arm64 linux loader initrd placement

[Vladimir 'phcoder' Serbinenko]

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

On Thu, Jul 27, 2017, 17:40 Daniel Kiper <dkiper@net-space.pl> wrote:

> On Thu, Jul 27, 2017 at 03:18:16PM +0000, Vladimir 'phcoder' Serbinenko
> wrote:
> > On Thu, Jul 27, 2017, 17:16 Daniel Kiper <dkiper@net-space.pl> wrote:
> > > On Mon, Jun 12, 2017 at 03:53:40PM +0100, Leif Lindholm wrote:
> > > > The 32-bit arm Linux kernel is built as a zImage, which
> self-decompresses
> > > > down to near start of RAM. In order for an initrd/initramfs to be
> > > > accessible, it needs to be placed within the first ~768MB of RAM.
> > > > The initrd loader built into the kernel EFI stub restricts this down
> to
> > > > 512MB for simplicity - so enable the same restriction in grub.
> > > >
> > > > For arm64, the requirement is within a 1GB aligned 32GB window also
> > > > covering the (runtime) kernel image. Since the EFI stub loader itself
> > > > will attempt to relocate to near start of RAM, force initrd to be
> loaded
> > > > completely within the first 32GB of RAM.
> > > >
> > > > Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
> > > > ---
> > > >  grub-core/loader/arm64/linux.c | 39
> > > ++++++++++++++++++++++++++++++++++++++-
> > > >  1 file changed, 38 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/grub-core/loader/arm64/linux.c
> > > b/grub-core/loader/arm64/linux.c
> > > > index 8cd44230d..7e989c2b9 100644
> > > > --- a/grub-core/loader/arm64/linux.c
> > > > +++ b/grub-core/loader/arm64/linux.c
> > > > @@ -35,6 +35,23 @@
> > > >
> > > >  GRUB_MOD_LICENSE ("GPLv3+");
> > > >
> > > > +/*
> > > > + * As per linux/Documentation/arm/Booting
> > > > + * ARM initrd needs to be covered by kernel linear mapping,
> > > > + * so place it in the first 512MB of DRAM.
> > > > + *
> > > > + * As per linux/Documentation/arm64/booting.txt
> > > > + * ARM64 initrd needs to be contained entirely within a 1GB aligned
> > > window
> > > > + * of up to 32GB of size that covers the kernel image as well.
> > > > + * Since the EFI stub loader will attempt to load the kernel near
> start
> > > of
> > > > + * RAM, place the buffer in the first 32GB of RAM.
> > > > + */
> > > > +#ifdef __arm__
> > > > +#define INITRD_MAX_ADDRESS_OFFSET (512U * 1024 * 1024)
> > > > +#else /* __aarch64__ */
> > > > +#define INITRD_MAX_ADDRESS_OFFSET (32ULL * 1024 * 1024 * 1024)
> > > > +#endif
> > > > +
> > > >  static grub_dl_t my_mod;
> > > >  static int loaded;
> > > >
> > > > @@ -194,6 +211,25 @@ grub_linux_unload (void)
> > > >    return GRUB_ERR_NONE;
> > > >  }
> > > >
> > > > +/*
> > > > + * This function returns a pointer to a legally allocated initrd
> buffer,
> > > > + * or NULL if unsuccessful
> > > > + */
> > > > +static void *
> > > > +allocate_initrd_mem (int initrd_pages)
> > > > +{
> > > > +  grub_addr_t max_addr;
> > > > +
> > > > +  if (grub_efi_get_dram_base (&max_addr) != GRUB_ERR_NONE)
> > > > +    return NULL;
> > > > +
> > > > +  max_addr += INITRD_MAX_ADDRESS_OFFSET - 1;
> > >
> > > I do not understand this. Why do not pass simply
> INITRD_MAX_ADDRESS_OFFSET
> > > instead of max_addr to grub_efi_allocate_pages_real()?
> > >
> > On ARM it's common for RAM to start at address different from 0
>
> OK, but, AIUI we have to load initrd no higher than
> INITRD_MAX_ADDRESS_OFFSET.
> So, we do not care where the region starts. We care where region ends.
> Am I missing something?
>
The limit is relative to the start of RAM.

>
> Daniel
>

[-- Attachment #2: Type: text/html, Size: 4769 bytes --]

Re: [PATCH 7/7] efi: change heap allocation type to GRUB_EFI_LOADER_CODE

[Daniel Kiper]

On Thu, Jul 27, 2017 at 03:33:06PM +0000, Vladimir 'phcoder' Serbinenko wrote:
> On Thu, Jul 27, 2017, 17:30 Daniel Kiper <dkiper@net-space.pl> wrote:
> > On Mon, Jun 12, 2017 at 03:53:41PM +0100, Leif Lindholm wrote:
> > > With upcoming changes to EDK2, allocations of type EFI_LOADER_DATA may
> > > not return regions with execute ability. Since modules are loaded onto
> > > the heap, change the heap allocation type to GRUB_EFI_LOADER_CODE in
> > > order to permit execution on systems with this feature enabled.
> > >
> > > Closes: 50420
> > >
> > > Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
> > > ---
> > >  grub-core/kern/efi/mm.c | 4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/grub-core/kern/efi/mm.c b/grub-core/kern/efi/mm.c
> > > index 7b1763bc5..f27a48e68 100644
> > > --- a/grub-core/kern/efi/mm.c
> > > +++ b/grub-core/kern/efi/mm.c
> > > @@ -404,7 +404,9 @@ add_memory_regions (grub_efi_memory_descriptor_t
> > *memory_map,
> > >         pages = required_pages;
> > >       }
> > >
> > > -      addr = grub_efi_allocate_pages (start, pages);
> > > +      addr = grub_efi_allocate_pages_real (start, pages,
> > > +                                        GRUB_EFI_ALLOCATE_ADDRESS,
> > > +                                        GRUB_EFI_LOADER_CODE);
> >
> > Is it really needed? Is any module exectued in place or does it contain
> > code ready to run out of the box?
> >
> All the modules are loaded to heap

I do not see why modules have to be loaded into memory region with
GRUB_EFI_LOADER_CODE type. Kernels are different things and I can
agree that they should be loaded into GRUB_EFI_LOADER_CODE.

Daniel

Re: [PATCH 7/7] efi: change heap allocation type to GRUB_EFI_LOADER_CODE

[Vladimir 'phcoder' Serbinenko]

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

On Thu, Jul 27, 2017, 17:44 Daniel Kiper <dkiper@net-space.pl> wrote:

> On Thu, Jul 27, 2017 at 03:33:06PM +0000, Vladimir 'phcoder' Serbinenko
> wrote:
> > On Thu, Jul 27, 2017, 17:30 Daniel Kiper <dkiper@net-space.pl> wrote:
> > > On Mon, Jun 12, 2017 at 03:53:41PM +0100, Leif Lindholm wrote:
> > > > With upcoming changes to EDK2, allocations of type EFI_LOADER_DATA
> may
> > > > not return regions with execute ability. Since modules are loaded
> onto
> > > > the heap, change the heap allocation type to GRUB_EFI_LOADER_CODE in
> > > > order to permit execution on systems with this feature enabled.
> > > >
> > > > Closes: 50420
> > > >
> > > > Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
> > > > ---
> > > >  grub-core/kern/efi/mm.c | 4 +++-
> > > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/grub-core/kern/efi/mm.c b/grub-core/kern/efi/mm.c
> > > > index 7b1763bc5..f27a48e68 100644
> > > > --- a/grub-core/kern/efi/mm.c
> > > > +++ b/grub-core/kern/efi/mm.c
> > > > @@ -404,7 +404,9 @@ add_memory_regions (grub_efi_memory_descriptor_t
> > > *memory_map,
> > > >         pages = required_pages;
> > > >       }
> > > >
> > > > -      addr = grub_efi_allocate_pages (start, pages);
> > > > +      addr = grub_efi_allocate_pages_real (start, pages,
> > > > +                                        GRUB_EFI_ALLOCATE_ADDRESS,
> > > > +                                        GRUB_EFI_LOADER_CODE);
> > >
> > > Is it really needed? Is any module exectued in place or does it contain
> > > code ready to run out of the box?
> > >
> > All the modules are loaded to heap
>
> I do not see why modules have to be loaded into memory region with
> GRUB_EFI_LOADER_CODE type.

He means grub modules not initrd or multiboot modules

> > Kernels

>  are different things and I can
> agree that they should be loaded into GRUB_EFI_LOADER_CODE.
>
> Daniel
>

[-- Attachment #2: Type: text/html, Size: 3000 bytes --]

Re: [PATCH 7/7] efi: change heap allocation type to GRUB_EFI_LOADER_CODE

[Daniel Kiper]

On Thu, Jul 27, 2017 at 03:45:48PM +0000, Vladimir 'phcoder' Serbinenko wrote:
> On Thu, Jul 27, 2017, 17:44 Daniel Kiper <dkiper@net-space.pl> wrote:
> > On Thu, Jul 27, 2017 at 03:33:06PM +0000, Vladimir 'phcoder' Serbinenko
> > wrote:
> > > On Thu, Jul 27, 2017, 17:30 Daniel Kiper <dkiper@net-space.pl> wrote:
> > > > On Mon, Jun 12, 2017 at 03:53:41PM +0100, Leif Lindholm wrote:
> > > > > With upcoming changes to EDK2, allocations of type EFI_LOADER_DATA
> > may
> > > > > not return regions with execute ability. Since modules are loaded
> > onto
> > > > > the heap, change the heap allocation type to GRUB_EFI_LOADER_CODE in
> > > > > order to permit execution on systems with this feature enabled.
> > > > >
> > > > > Closes: 50420
> > > > >
> > > > > Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
> > > > > ---
> > > > >  grub-core/kern/efi/mm.c | 4 +++-
> > > > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/grub-core/kern/efi/mm.c b/grub-core/kern/efi/mm.c
> > > > > index 7b1763bc5..f27a48e68 100644
> > > > > --- a/grub-core/kern/efi/mm.c
> > > > > +++ b/grub-core/kern/efi/mm.c
> > > > > @@ -404,7 +404,9 @@ add_memory_regions (grub_efi_memory_descriptor_t
> > > > *memory_map,
> > > > >         pages = required_pages;
> > > > >       }
> > > > >
> > > > > -      addr = grub_efi_allocate_pages (start, pages);
> > > > > +      addr = grub_efi_allocate_pages_real (start, pages,
> > > > > +                                        GRUB_EFI_ALLOCATE_ADDRESS,
> > > > > +                                        GRUB_EFI_LOADER_CODE);
> > > >
> > > > Is it really needed? Is any module exectued in place or does it contain
> > > > code ready to run out of the box?
> > > >
> > > All the modules are loaded to heap
> >
> > I do not see why modules have to be loaded into memory region with
> > GRUB_EFI_LOADER_CODE type.
>
> He means grub modules not initrd or multiboot modules

Ahhh... Right, then it should be correct. Though I would double
check it applies to GRUB2 modules only.

Daniel

Re: [PATCH 6/7] efi: restrict arm/arm64 linux loader initrd placement

[Daniel Kiper]

On Thu, Jul 27, 2017 at 03:41:25PM +0000, Vladimir 'phcoder' Serbinenko wrote:
> On Thu, Jul 27, 2017, 17:40 Daniel Kiper <dkiper@net-space.pl> wrote:
> > On Thu, Jul 27, 2017 at 03:18:16PM +0000, Vladimir 'phcoder' Serbinenko
> > wrote:
> > > On Thu, Jul 27, 2017, 17:16 Daniel Kiper <dkiper@net-space.pl> wrote:
> > > > On Mon, Jun 12, 2017 at 03:53:40PM +0100, Leif Lindholm wrote:
> > > > > The 32-bit arm Linux kernel is built as a zImage, which
> > self-decompresses
> > > > > down to near start of RAM. In order for an initrd/initramfs to be
> > > > > accessible, it needs to be placed within the first ~768MB of RAM.
> > > > > The initrd loader built into the kernel EFI stub restricts this down
> > to
> > > > > 512MB for simplicity - so enable the same restriction in grub.
> > > > >
> > > > > For arm64, the requirement is within a 1GB aligned 32GB window also
> > > > > covering the (runtime) kernel image. Since the EFI stub loader itself
> > > > > will attempt to relocate to near start of RAM, force initrd to be
> > loaded
> > > > > completely within the first 32GB of RAM.
> > > > >
> > > > > Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
> > > > > ---
> > > > >  grub-core/loader/arm64/linux.c | 39
> > > > ++++++++++++++++++++++++++++++++++++++-
> > > > >  1 file changed, 38 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/grub-core/loader/arm64/linux.c
> > > > b/grub-core/loader/arm64/linux.c
> > > > > index 8cd44230d..7e989c2b9 100644
> > > > > --- a/grub-core/loader/arm64/linux.c
> > > > > +++ b/grub-core/loader/arm64/linux.c
> > > > > @@ -35,6 +35,23 @@
> > > > >
> > > > >  GRUB_MOD_LICENSE ("GPLv3+");
> > > > >
> > > > > +/*
> > > > > + * As per linux/Documentation/arm/Booting
> > > > > + * ARM initrd needs to be covered by kernel linear mapping,
> > > > > + * so place it in the first 512MB of DRAM.
> > > > > + *
> > > > > + * As per linux/Documentation/arm64/booting.txt
> > > > > + * ARM64 initrd needs to be contained entirely within a 1GB aligned
> > > > window
> > > > > + * of up to 32GB of size that covers the kernel image as well.
> > > > > + * Since the EFI stub loader will attempt to load the kernel near
> > start
> > > > of
> > > > > + * RAM, place the buffer in the first 32GB of RAM.
> > > > > + */
> > > > > +#ifdef __arm__
> > > > > +#define INITRD_MAX_ADDRESS_OFFSET (512U * 1024 * 1024)
> > > > > +#else /* __aarch64__ */
> > > > > +#define INITRD_MAX_ADDRESS_OFFSET (32ULL * 1024 * 1024 * 1024)
> > > > > +#endif
> > > > > +
> > > > >  static grub_dl_t my_mod;
> > > > >  static int loaded;
> > > > >
> > > > > @@ -194,6 +211,25 @@ grub_linux_unload (void)
> > > > >    return GRUB_ERR_NONE;
> > > > >  }
> > > > >
> > > > > +/*
> > > > > + * This function returns a pointer to a legally allocated initrd
> > buffer,
> > > > > + * or NULL if unsuccessful
> > > > > + */
> > > > > +static void *
> > > > > +allocate_initrd_mem (int initrd_pages)
> > > > > +{
> > > > > +  grub_addr_t max_addr;
> > > > > +
> > > > > +  if (grub_efi_get_dram_base (&max_addr) != GRUB_ERR_NONE)
> > > > > +    return NULL;
> > > > > +
> > > > > +  max_addr += INITRD_MAX_ADDRESS_OFFSET - 1;
> > > >
> > > > I do not understand this. Why do not pass simply
> > INITRD_MAX_ADDRESS_OFFSET
> > > > instead of max_addr to grub_efi_allocate_pages_real()?
> > > >
> > > On ARM it's common for RAM to start at address different from 0
> >
> > OK, but, AIUI we have to load initrd no higher than
> > INITRD_MAX_ADDRESS_OFFSET.
> > So, we do not care where the region starts. We care where region ends.
> > Am I missing something?
> >
> The limit is relative to the start of RAM.

OK, then it make sense. Thanks for clarification. I would just
ask for a comment to avoid reader confusion.

Daniel

Re: [PATCH 1/7] efi: add grub_efi_get_dram_base() function for arm*

[Leif Lindholm]

On Thu, Jul 27, 2017 at 04:27:26PM +0200, Daniel Kiper wrote:
> On Mon, Jun 12, 2017 at 03:53:35PM +0100, Leif Lindholm wrote:
> > Since ARM platforms do not have a common memory map, add a helper
> > function that finds the lowest address region with the EFI_MEMORY_WB
> > attribute set in the UEFI memory map.
> >
> > Required for the arm/arm64 linux loader to restrict the initrd
> > location to where it will be accessible by the kernel at runtime.
> >
> > Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
> > ---
> >  grub-core/kern/efi/mm.c | 42 ++++++++++++++++++++++++++++++++++++++++++
> >  include/grub/efi/efi.h  |  1 +
> >  2 files changed, 43 insertions(+)
> >
> > diff --git a/grub-core/kern/efi/mm.c b/grub-core/kern/efi/mm.c
> > index 20a47aaf5..460a4b763 100644
> > --- a/grub-core/kern/efi/mm.c
> > +++ b/grub-core/kern/efi/mm.c
> > @@ -525,3 +525,45 @@ grub_efi_mm_init (void)
> >    grub_efi_free_pages ((grub_addr_t) memory_map,
> >  		       2 * BYTES_TO_PAGES (MEMORY_MAP_SIZE));
> >  }
> > +
> > +#if defined (__arm__) || defined (__aarch64__)
> > +grub_err_t
> > +grub_efi_get_dram_base(grub_addr_t *base_addr)
> 
> Please make this function more generic and get
> attribute as an argument.

While I am normally a huge fan of making everything as generic as
possible, this really is a very special case - and as far as I know,
it is really only relevant to ARM/AArch64. I would expect other
architectures to just have a static inline, return 0 in efi/memory.h,
if we ended up merging even more Linux EFI stub loaders together.

Or are you looking for some more multidimensional memory map lookup
thing? If so, could you give me an example invocation to work
against?

> > +{
> > +  grub_efi_memory_descriptor_t *memory_map;
> > +  grub_efi_memory_descriptor_t *desc;
> > +  grub_efi_uintn_t mmap_size;
> > +  grub_efi_uintn_t desc_size;
> > +
> > +  mmap_size = (1 << GRUB_EFI_PAGE_SHIFT);
> 
> Could not you define GRUB_EFI_PAGE and use it here?

Sure.

> And why GRUB_EFI_PAGE_SHIFT and other stuff is defined
> in include/grub/arm64/fdtload.h? It looks like generic
> thing and should be in genric place too. Please fix it
> if possible.

Sure, I'll reorder that hunk (as you spotted in a later patch).

> > +  while (1)
> > +    {
> > +      int ret;
> > +
> > +      memory_map = grub_malloc (mmap_size);
> > +      if (! memory_map)
> > +        return GRUB_ERR_OUT_OF_MEMORY;
> > +      ret = grub_efi_get_memory_map (&mmap_size, memory_map, NULL,
> > +				     &desc_size, NULL);
> > +      if (ret > 0)
> > +	break;
> > +
> > +      grub_free (memory_map);
> > +      if (ret == 0)
> > +	return GRUB_ERR_BUG;
> > +
> > +      mmap_size += (1 << GRUB_EFI_PAGE_SHIFT);
> > +    }
> 
> Hmmm... This is not optimal. Please take a look at e.g.
> grub_efi_finish_boot_services() how buffer for memory
> map should be allocated.
> 
> And going further... Could you take a look at
> grub_relocator_alloc_chunk_align() and
> grub_relocator_alloc_chunk_addr(). Good example
> is in grub-core/loader/multiboot_mbi2.c and
> grub-core/loader/multiboot_elfxx.c. If you
> use this machinery then there is a chance
> that you do not duplicate code so much.

Yeah, I can move the relevant bits from there to kern/efi/mm.c to
start getting rid of the existing duplication.

/
    Leif

Re: [PATCH 2/7] efi: refactor grub_efi_allocate_pages

[Leif Lindholm]

On Thu, Jul 27, 2017 at 04:46:14PM +0200, Daniel Kiper wrote:
> On Mon, Jun 12, 2017 at 03:53:36PM +0100, Leif Lindholm wrote:
> > Expose a new function, grub_efi_allocate_pages_real(), making it possible
> > to specify allocation type and memory type as supported by the UEFI
> > AllocatePages boot service.
> >
> > Make grub_efi_allocate_pages() a consumer of the new function,
> > maintaining its old functionality.
> >
> > Also delete some left-around #if 1/#else blocks in the affected
> > functions.
> >
> > Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
> > ---
> >  grub-core/kern/efi/mm.c | 46 ++++++++++++++++++++++++----------------------
> >  include/grub/efi/efi.h  |  5 +++++
> >  2 files changed, 29 insertions(+), 22 deletions(-)
> >
> > diff --git a/grub-core/kern/efi/mm.c b/grub-core/kern/efi/mm.c
> > index 460a4b763..7b1763bc5 100644
> > --- a/grub-core/kern/efi/mm.c
> > +++ b/grub-core/kern/efi/mm.c
> > @@ -51,36 +51,20 @@ int grub_efi_is_finished = 0;
> >
> >  /* Allocate pages. Return the pointer to the first of allocated pages.  */
> >  void *
> > -grub_efi_allocate_pages (grub_efi_physical_address_t address,
> > -			 grub_efi_uintn_t pages)
> > +grub_efi_allocate_pages_real (grub_efi_physical_address_t address,
> 
> Could not you just add an extra argument to grub_efi_allocate_pages()?
> It is not called in many places. Do you really need separate
> grub_efi_allocate_pages_real()?

Need? No.

It gave an excuse to break some of the implicit semantics of the
existing function out separately from the bit that actually allocates
memory.

I also instinctively dislike functions with > 4 arguments due to
32-bit ARM ABI :)

/
    Leif

Re: [PATCH 3/7] efi: move fdt helper library

[Leif Lindholm]

On Thu, Jul 27, 2017 at 04:54:19PM +0200, Daniel Kiper wrote:
> On Mon, Jun 12, 2017 at 03:53:37PM +0100, Leif Lindholm wrote:
> > There is nothing ARM64 (or even ARM) specific about the efi fdt helper
> > library, which is used for locating or overriding a firmware-provided
> > devicetree in a UEFI system - so move it to loader/efi for reuse.
> 
> OK.
> 
> > Move the fdtload.h include file to grub/efi
> 
> This begs one patch...

Sure.

> > and move the EFI page size
> > definitions to grub/efi/memory.h. (These definitions refer strictly to
> > allocation operations, as opposed to translation granules.)
> 
> Great! This is what I requested earlier. However, please put this in separate patch.

Sure.

> > ---
> >  grub-core/Makefile.core.def           | 2 +-
> >  grub-core/loader/arm64/linux.c        | 3 ++-
> >  grub-core/loader/arm64/xen_boot.c     | 3 ++-
> >  grub-core/loader/{arm64 => efi}/fdt.c | 3 ++-
> >  include/grub/{arm64 => efi}/fdtload.h | 3 ---
> >  include/grub/efi/memory.h             | 3 +++
> >  6 files changed, 10 insertions(+), 7 deletions(-)
> >  rename grub-core/loader/{arm64 => efi}/fdt.c (98%)
> >  rename include/grub/{arm64 => efi}/fdtload.h (89%)
> >
> > diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
> > index 1d86bd22e..a65c27f7f 100644
> > --- a/grub-core/Makefile.core.def
> > +++ b/grub-core/Makefile.core.def
> > @@ -1707,7 +1707,7 @@ module = {
> >
> >  module = {
> >    name = fdt;
> > -  arm64 = loader/arm64/fdt.c;
> > +  arm64 = loader/efi/fdt.c;
> >    common = lib/fdt.c;
> >    enable = fdt;
> >  };
> > diff --git a/grub-core/loader/arm64/linux.c b/grub-core/loader/arm64/linux.c
> > index 9519d2e4d..cac94d53d 100644
> > --- a/grub-core/loader/arm64/linux.c
> > +++ b/grub-core/loader/arm64/linux.c
> > @@ -26,8 +26,9 @@
> >  #include <grub/mm.h>
> >  #include <grub/types.h>
> >  #include <grub/cpu/linux.h>
> > -#include <grub/cpu/fdtload.h>
> >  #include <grub/efi/efi.h>
> > +#include <grub/efi/fdtload.h>
> > +#include <grub/efi/memory.h>
> 
> Why do you shuffle and add headers?

The shuffling is to fit alphabetically sorted when moving from
cpu/fdtload.h to efi/fdtload.h.

The addition is because the EFI_PAGE_SHIFT & co moves from
cpu/fdtload.h into efi/memory.h.

> >  #include <grub/efi/pe32.h>
> >  #include <grub/i18n.h>
> >  #include <grub/lib/cmdline.h>
> > diff --git a/grub-core/loader/arm64/xen_boot.c b/grub-core/loader/arm64/xen_boot.c
> > index 27ede46ca..d092a53ed 100644
> > --- a/grub-core/loader/arm64/xen_boot.c
> > +++ b/grub-core/loader/arm64/xen_boot.c
> > @@ -27,9 +27,10 @@
> >  #include <grub/misc.h>
> >  #include <grub/mm.h>
> >  #include <grub/types.h>
> > -#include <grub/cpu/fdtload.h>
> >  #include <grub/cpu/linux.h>
> >  #include <grub/efi/efi.h>
> > +#include <grub/efi/fdtload.h>
> > +#include <grub/efi/memory.h>
> 
> Ditto?

Ditto.

> >  #include <grub/efi/pe32.h>	/* required by struct xen_hypervisor_header */
> >  #include <grub/i18n.h>
> >  #include <grub/lib/cmdline.h>
> > diff --git a/grub-core/loader/arm64/fdt.c b/grub-core/loader/efi/fdt.c
> > similarity index 98%
> > rename from grub-core/loader/arm64/fdt.c
> > rename to grub-core/loader/efi/fdt.c
> > index db49cf649..be369fd9d 100644
> > --- a/grub-core/loader/arm64/fdt.c
> > +++ b/grub-core/loader/efi/fdt.c
> > @@ -18,12 +18,13 @@
> >
> >  #include <grub/fdt.h>
> >  #include <grub/mm.h>
> > -#include <grub/cpu/fdtload.h>
> >  #include <grub/err.h>
> >  #include <grub/dl.h>
> >  #include <grub/command.h>
> >  #include <grub/file.h>
> >  #include <grub/efi/efi.h>
> > +#include <grub/efi/fdtload.h>
> > +#include <grub/machine/memory.h>
> 
> Ditto?

Ditto.

> >  static void *loaded_fdt;
> >  static void *fdt;
> > diff --git a/include/grub/arm64/fdtload.h b/include/grub/efi/fdtload.h
> > similarity index 89%
> > rename from include/grub/arm64/fdtload.h
> > rename to include/grub/efi/fdtload.h
> > index 7b9ddba91..713c9424d 100644
> > --- a/include/grub/arm64/fdtload.h
> > +++ b/include/grub/efi/fdtload.h
> > @@ -29,7 +29,4 @@ grub_fdt_unload (void);
> >  grub_err_t
> >  grub_fdt_install (void);
> >
> > -#define GRUB_EFI_PAGE_SHIFT	12
> > -#define GRUB_EFI_BYTES_TO_PAGES(bytes)   (((bytes) + 0xfff) >> GRUB_EFI_PAGE_SHIFT)
> > -
> >  #endif
> > diff --git a/include/grub/efi/memory.h b/include/grub/efi/memory.h
> > index 20526b146..0eb0b70b6 100644
> > --- a/include/grub/efi/memory.h
> > +++ b/include/grub/efi/memory.h
> > @@ -22,6 +22,9 @@
> >  #include <grub/err.h>
> >  #include <grub/types.h>
> >
> > +#define GRUB_EFI_PAGE_SHIFT	12
> > +#define GRUB_EFI_BYTES_TO_PAGES(bytes)   (((bytes) + 0xfff) >> GRUB_EFI_PAGE_SHIFT)
> 
> If you move this then fix the aligment. I mean more or less this:
> 
> #define GRUB_EFI_PAGE_SHIFT              12
> #define GRUB_EFI_BYTES_TO_PAGES(bytes)   (((bytes) + 0xfff) >> GRUB_EFI_PAGE_SHIFT)
> 
> #define GRUB_MMAP_REGISTER_BY_FIRMWARE   1

Will do.

/
    Leif

Re: [PATCH 4/7] arm64: make efi linux loader more generic

[Leif Lindholm]

On Thu, Jul 27, 2017 at 04:57:22PM +0200, Daniel Kiper wrote:
> On Mon, Jun 12, 2017 at 03:53:38PM +0100, Leif Lindholm wrote:
> > In order to enable reuse of the arm64 efi linux loader for arm,
> > change a few function names and macros. Add a global definition
> > of GRUB_PE32_MAGIC in grub/efi/pe32.h.
> >
> > Make the arm64 efi loader (and fdt helpers) 32/64-bit safe.
> >
> > Also update the arm64 xen loader, since it depends on some of the
> > functions in the linux loader.
> 
> Please, please, please one logical change in the patch. Otherwise
> this does not parse.

Will do.
Wasn't sure a bunch of tiny patches would be appreciated.

/
    Leif

Re: [PATCH 5/7] arm: reuse arm64 linux loader on efi systems

[Leif Lindholm]

On Thu, Jul 27, 2017 at 05:06:25PM +0200, Daniel Kiper wrote:
> On Mon, Jun 12, 2017 at 03:53:39PM +0100, Leif Lindholm wrote:
> > The original 32-bit arm EFI Linux loader reused the 32-bit Linux
> > loader for U-Boot. However, this meant it was acting in an
> > entirely not UEFI-compliant fashion.
> >
> > Since EFI stub loader support for arm went into upstream Linux
> > for 4.5, we can now reuse the same loader as is used on arm64.
> >
> > This results in some now-redundant code being dropped from the U-Boot
> > linux loader, and the arm efi port in general. This also drops the
> > ability to boot kernels without the efi stub support on 32-bit arm efi
> > grub.
> 
> I have a feeling that this patch does more than one thing. Please
> split it if possible.

If you're happy with keeping dead code temporarily, I can split into
at least two.

> > Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
> > ---
> >  grub-core/Makefile.am         |   1 -
> >  grub-core/Makefile.core.def   |   6 +-
> >  grub-core/kern/arm/efi/misc.c | 202 ------------------------------------------
> >  grub-core/loader/arm/linux.c  |  39 ++------
> >  include/grub/arm/efi/loader.h |  26 ------
> >  include/grub/arm/efi/memory.h |   3 +
> >  include/grub/arm/linux.h      |  30 +++----
> >  7 files changed, 22 insertions(+), 285 deletions(-)
> >  delete mode 100644 grub-core/kern/arm/efi/misc.c
> >  delete mode 100644 include/grub/arm/efi/loader.h
> 
> [...]
> 
> > diff --git a/include/grub/arm/efi/memory.h b/include/grub/arm/efi/memory.h
> > index 2c64918e3..986f656d6 100644
> > --- a/include/grub/arm/efi/memory.h
> > +++ b/include/grub/arm/efi/memory.h
> > @@ -3,4 +3,7 @@
> >
> >  #define GRUB_EFI_MAX_USABLE_ADDRESS 0xffffffff
> >
> > +#define GRUB_EFI_PAGE_SHIFT	12
> > +#define GRUB_EFI_BYTES_TO_PAGES(bytes)   (((bytes) + 0xfff) >> GRUB_EFI_PAGE_SHIFT)
> 
> Hmmm... It looks familiar. Why do you duplicate this?

Sorry, rebase mess-up.

> > +
> >  #endif /* ! GRUB_MEMORY_CPU_HEADER */
> > diff --git a/include/grub/arm/linux.h b/include/grub/arm/linux.h
> > index f217f8281..98ff4c004 100644
> > --- a/include/grub/arm/linux.h
> > +++ b/include/grub/arm/linux.h
> > @@ -20,32 +20,26 @@
> >  #ifndef GRUB_LINUX_CPU_HEADER
> >  #define GRUB_LINUX_CPU_HEADER 1
> >
> > -#define LINUX_ZIMAGE_OFFSET 0x24
> > -#define LINUX_ZIMAGE_MAGIC  0x016f2818
> > +#define GRUB_LINUX_MAGIC_SIGNATURE 0x016f2818
> >
> > -#include "system.h"
> > +struct grub_linux_kernel_header {
> > +  grub_uint32_t code0;
> > +  grub_uint32_t reserved1[8];
> > +  grub_uint32_t magic;
> > +  grub_uint32_t start; /* _start */
> > +  grub_uint32_t end;   /* _edata */
> > +  grub_uint32_t reserved2[4];
> > +  grub_uint32_t hdr_offset;
> > +};
> 
> ???

A struct describing the start of the 32-bit ARM zImage for the sanity
check in grub_efi_linux_check_image.

/
    Leif

Re: [PATCH 6/7] efi: restrict arm/arm64 linux loader initrd placement

[Leif Lindholm]

On Thu, Jul 27, 2017 at 06:12:42PM +0200, Daniel Kiper wrote:
> On Thu, Jul 27, 2017 at 03:41:25PM +0000, Vladimir 'phcoder' Serbinenko wrote:
> > On Thu, Jul 27, 2017, 17:40 Daniel Kiper <dkiper@net-space.pl> wrote:
> > > On Thu, Jul 27, 2017 at 03:18:16PM +0000, Vladimir 'phcoder' Serbinenko
> > > wrote:
> > > > On Thu, Jul 27, 2017, 17:16 Daniel Kiper <dkiper@net-space.pl> wrote:
> > > > > On Mon, Jun 12, 2017 at 03:53:40PM +0100, Leif Lindholm wrote:
> > > > > > The 32-bit arm Linux kernel is built as a zImage, which
> > > self-decompresses
> > > > > > down to near start of RAM. In order for an initrd/initramfs to be
> > > > > > accessible, it needs to be placed within the first ~768MB of RAM.
> > > > > > The initrd loader built into the kernel EFI stub restricts this down
> > > to
> > > > > > 512MB for simplicity - so enable the same restriction in grub.
> > > > > >
> > > > > > For arm64, the requirement is within a 1GB aligned 32GB window also
> > > > > > covering the (runtime) kernel image. Since the EFI stub loader itself
> > > > > > will attempt to relocate to near start of RAM, force initrd to be
> > > loaded
> > > > > > completely within the first 32GB of RAM.
> > > > > >
> > > > > > Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
> > > > > > ---
> > > > > >  grub-core/loader/arm64/linux.c | 39
> > > > > ++++++++++++++++++++++++++++++++++++++-
> > > > > >  1 file changed, 38 insertions(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/grub-core/loader/arm64/linux.c
> > > > > b/grub-core/loader/arm64/linux.c
> > > > > > index 8cd44230d..7e989c2b9 100644
> > > > > > --- a/grub-core/loader/arm64/linux.c
> > > > > > +++ b/grub-core/loader/arm64/linux.c
> > > > > > @@ -35,6 +35,23 @@
> > > > > >
> > > > > >  GRUB_MOD_LICENSE ("GPLv3+");
> > > > > >
> > > > > > +/*
> > > > > > + * As per linux/Documentation/arm/Booting
> > > > > > + * ARM initrd needs to be covered by kernel linear mapping,
> > > > > > + * so place it in the first 512MB of DRAM.
> > > > > > + *
> > > > > > + * As per linux/Documentation/arm64/booting.txt
> > > > > > + * ARM64 initrd needs to be contained entirely within a 1GB aligned
> > > > > window
> > > > > > + * of up to 32GB of size that covers the kernel image as well.
> > > > > > + * Since the EFI stub loader will attempt to load the kernel near
> > > start
> > > > > of
> > > > > > + * RAM, place the buffer in the first 32GB of RAM.
> > > > > > + */
> > > > > > +#ifdef __arm__
> > > > > > +#define INITRD_MAX_ADDRESS_OFFSET (512U * 1024 * 1024)
> > > > > > +#else /* __aarch64__ */
> > > > > > +#define INITRD_MAX_ADDRESS_OFFSET (32ULL * 1024 * 1024 * 1024)
> > > > > > +#endif
> > > > > > +
> > > > > >  static grub_dl_t my_mod;
> > > > > >  static int loaded;
> > > > > >
> > > > > > @@ -194,6 +211,25 @@ grub_linux_unload (void)
> > > > > >    return GRUB_ERR_NONE;
> > > > > >  }
> > > > > >
> > > > > > +/*
> > > > > > + * This function returns a pointer to a legally allocated initrd
> > > buffer,
> > > > > > + * or NULL if unsuccessful
> > > > > > + */
> > > > > > +static void *
> > > > > > +allocate_initrd_mem (int initrd_pages)
> > > > > > +{
> > > > > > +  grub_addr_t max_addr;
> > > > > > +
> > > > > > +  if (grub_efi_get_dram_base (&max_addr) != GRUB_ERR_NONE)
> > > > > > +    return NULL;
> > > > > > +
> > > > > > +  max_addr += INITRD_MAX_ADDRESS_OFFSET - 1;
> > > > >
> > > > > I do not understand this. Why do not pass simply
> > > INITRD_MAX_ADDRESS_OFFSET
> > > > > instead of max_addr to grub_efi_allocate_pages_real()?
> > > > >
> > > > On ARM it's common for RAM to start at address different from 0
> > >
> > > OK, but, AIUI we have to load initrd no higher than
> > > INITRD_MAX_ADDRESS_OFFSET.
> > > So, we do not care where the region starts. We care where region ends.
> > > Am I missing something?
> > >
> > The limit is relative to the start of RAM.
> 
> OK, then it make sense. Thanks for clarification. I would just
> ask for a comment to avoid reader confusion.

Would moving this function next to the comment block this patch adds
to the top of the file be sufficient?

/
    Leif

Re: [PATCH 7/7] efi: change heap allocation type to GRUB_EFI_LOADER_CODE

[Leif Lindholm]

On Thu, Jul 27, 2017 at 06:06:18PM +0200, Daniel Kiper wrote:
> > > > > > diff --git a/grub-core/kern/efi/mm.c b/grub-core/kern/efi/mm.c
> > > > > > index 7b1763bc5..f27a48e68 100644
> > > > > > --- a/grub-core/kern/efi/mm.c
> > > > > > +++ b/grub-core/kern/efi/mm.c
> > > > > > @@ -404,7 +404,9 @@ add_memory_regions (grub_efi_memory_descriptor_t
> > > > > *memory_map,
> > > > > >         pages = required_pages;
> > > > > >       }
> > > > > >
> > > > > > -      addr = grub_efi_allocate_pages (start, pages);
> > > > > > +      addr = grub_efi_allocate_pages_real (start, pages,
> > > > > > +                                        GRUB_EFI_ALLOCATE_ADDRESS,
> > > > > > +                                        GRUB_EFI_LOADER_CODE);
> > > > >
> > > > > Is it really needed? Is any module exectued in place or does it contain
> > > > > code ready to run out of the box?
> > > > >
> > > > All the modules are loaded to heap
> > >
> > > I do not see why modules have to be loaded into memory region with
> > > GRUB_EFI_LOADER_CODE type.
> >
> > He means grub modules not initrd or multiboot modules
> 
> Ahhh... Right, then it should be correct. Though I would double
> check it applies to GRUB2 modules only.

It applies to any executable code running until an operating system
takes over.

Don't get me wrong - this is more of a workaround than a fix for the
grub module loading. But properly separating code/data and ro/rw
regions is a much more invasive change which deserves more discussion.

/
    Leif

Re: [PATCH 1/7] efi: add grub_efi_get_dram_base() function for arm*

[Daniel Kiper]

On Fri, Jul 28, 2017 at 06:38:22PM +0100, Leif Lindholm wrote:
> On Thu, Jul 27, 2017 at 04:27:26PM +0200, Daniel Kiper wrote:
> > On Mon, Jun 12, 2017 at 03:53:35PM +0100, Leif Lindholm wrote:
> > > Since ARM platforms do not have a common memory map, add a helper
> > > function that finds the lowest address region with the EFI_MEMORY_WB
> > > attribute set in the UEFI memory map.
> > >
> > > Required for the arm/arm64 linux loader to restrict the initrd
> > > location to where it will be accessible by the kernel at runtime.
> > >
> > > Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
> > > ---
> > >  grub-core/kern/efi/mm.c | 42 ++++++++++++++++++++++++++++++++++++++++++
> > >  include/grub/efi/efi.h  |  1 +
> > >  2 files changed, 43 insertions(+)
> > >
> > > diff --git a/grub-core/kern/efi/mm.c b/grub-core/kern/efi/mm.c
> > > index 20a47aaf5..460a4b763 100644
> > > --- a/grub-core/kern/efi/mm.c
> > > +++ b/grub-core/kern/efi/mm.c
> > > @@ -525,3 +525,45 @@ grub_efi_mm_init (void)
> > >    grub_efi_free_pages ((grub_addr_t) memory_map,
> > >  		       2 * BYTES_TO_PAGES (MEMORY_MAP_SIZE));
> > >  }
> > > +
> > > +#if defined (__arm__) || defined (__aarch64__)
> > > +grub_err_t
> > > +grub_efi_get_dram_base(grub_addr_t *base_addr)
> >
> > Please make this function more generic and get
> > attribute as an argument.
>
> While I am normally a huge fan of making everything as generic as
> possible, this really is a very special case - and as far as I know,
> it is really only relevant to ARM/AArch64. I would expect other
> architectures to just have a static inline, return 0 in efi/memory.h,
> if we ended up merging even more Linux EFI stub loaders together.
>
> Or are you looking for some more multidimensional memory map lookup
> thing? If so, could you give me an example invocation to work
> against?

OK, let's leave it as is. Just change function name to grub_efi_get_ram_base().

Daniel

Re: [PATCH 2/7] efi: refactor grub_efi_allocate_pages

[Daniel Kiper]

On Fri, Jul 28, 2017 at 06:57:36PM +0100, Leif Lindholm wrote:
> On Thu, Jul 27, 2017 at 04:46:14PM +0200, Daniel Kiper wrote:
> > On Mon, Jun 12, 2017 at 03:53:36PM +0100, Leif Lindholm wrote:
> > > Expose a new function, grub_efi_allocate_pages_real(), making it possible
> > > to specify allocation type and memory type as supported by the UEFI
> > > AllocatePages boot service.
> > >
> > > Make grub_efi_allocate_pages() a consumer of the new function,
> > > maintaining its old functionality.
> > >
> > > Also delete some left-around #if 1/#else blocks in the affected
> > > functions.
> > >
> > > Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
> > > ---
> > >  grub-core/kern/efi/mm.c | 46 ++++++++++++++++++++++++----------------------
> > >  include/grub/efi/efi.h  |  5 +++++
> > >  2 files changed, 29 insertions(+), 22 deletions(-)
> > >
> > > diff --git a/grub-core/kern/efi/mm.c b/grub-core/kern/efi/mm.c
> > > index 460a4b763..7b1763bc5 100644
> > > --- a/grub-core/kern/efi/mm.c
> > > +++ b/grub-core/kern/efi/mm.c
> > > @@ -51,36 +51,20 @@ int grub_efi_is_finished = 0;
> > >
> > >  /* Allocate pages. Return the pointer to the first of allocated pages.  */
> > >  void *
> > > -grub_efi_allocate_pages (grub_efi_physical_address_t address,
> > > -			 grub_efi_uintn_t pages)
> > > +grub_efi_allocate_pages_real (grub_efi_physical_address_t address,
> >
> > Could not you just add an extra argument to grub_efi_allocate_pages()?
> > It is not called in many places. Do you really need separate
> > grub_efi_allocate_pages_real()?
>
> Need? No.
>
> It gave an excuse to break some of the implicit semantics of the
> existing function out separately from the bit that actually allocates
> memory.

OK.

> I also instinctively dislike functions with > 4 arguments due to
> 32-bit ARM ABI :)

But after the suggested change you will have exactly 4 args. So?

Daniel

Re: [PATCH 3/7] efi: move fdt helper library

[Daniel Kiper]

On Fri, Jul 28, 2017 at 07:08:42PM +0100, Leif Lindholm wrote:
> On Thu, Jul 27, 2017 at 04:54:19PM +0200, Daniel Kiper wrote:
> > On Mon, Jun 12, 2017 at 03:53:37PM +0100, Leif Lindholm wrote:
> > > There is nothing ARM64 (or even ARM) specific about the efi fdt helper
> > > library, which is used for locating or overriding a firmware-provided
> > > devicetree in a UEFI system - so move it to loader/efi for reuse.
> >
> > OK.
> >
> > > Move the fdtload.h include file to grub/efi
> >
> > This begs one patch...
>
> Sure.
>
> > > and move the EFI page size
> > > definitions to grub/efi/memory.h. (These definitions refer strictly to
> > > allocation operations, as opposed to translation granules.)
> >
> > Great! This is what I requested earlier. However, please put this in separate patch.
>
> Sure.
>
> > > ---
> > >  grub-core/Makefile.core.def           | 2 +-
> > >  grub-core/loader/arm64/linux.c        | 3 ++-
> > >  grub-core/loader/arm64/xen_boot.c     | 3 ++-
> > >  grub-core/loader/{arm64 => efi}/fdt.c | 3 ++-
> > >  include/grub/{arm64 => efi}/fdtload.h | 3 ---
> > >  include/grub/efi/memory.h             | 3 +++
> > >  6 files changed, 10 insertions(+), 7 deletions(-)
> > >  rename grub-core/loader/{arm64 => efi}/fdt.c (98%)
> > >  rename include/grub/{arm64 => efi}/fdtload.h (89%)
> > >
> > > diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
> > > index 1d86bd22e..a65c27f7f 100644
> > > --- a/grub-core/Makefile.core.def
> > > +++ b/grub-core/Makefile.core.def
> > > @@ -1707,7 +1707,7 @@ module = {
> > >
> > >  module = {
> > >    name = fdt;
> > > -  arm64 = loader/arm64/fdt.c;
> > > +  arm64 = loader/efi/fdt.c;
> > >    common = lib/fdt.c;
> > >    enable = fdt;
> > >  };
> > > diff --git a/grub-core/loader/arm64/linux.c b/grub-core/loader/arm64/linux.c
> > > index 9519d2e4d..cac94d53d 100644
> > > --- a/grub-core/loader/arm64/linux.c
> > > +++ b/grub-core/loader/arm64/linux.c
> > > @@ -26,8 +26,9 @@
> > >  #include <grub/mm.h>
> > >  #include <grub/types.h>
> > >  #include <grub/cpu/linux.h>
> > > -#include <grub/cpu/fdtload.h>
> > >  #include <grub/efi/efi.h>
> > > +#include <grub/efi/fdtload.h>
> > > +#include <grub/efi/memory.h>
> >
> > Why do you shuffle and add headers?
>
> The shuffling is to fit alphabetically sorted when moving from
> cpu/fdtload.h to efi/fdtload.h.
>
> The addition is because the EFI_PAGE_SHIFT & co moves from
> cpu/fdtload.h into efi/memory.h.

OK, but say about this in commit message please.

Daniel

Re: [PATCH 6/7] efi: restrict arm/arm64 linux loader initrd placement

[Daniel Kiper]

On Fri, Jul 28, 2017 at 07:44:15PM +0100, Leif Lindholm wrote:
> On Thu, Jul 27, 2017 at 06:12:42PM +0200, Daniel Kiper wrote:
> > On Thu, Jul 27, 2017 at 03:41:25PM +0000, Vladimir 'phcoder' Serbinenko wrote:
> > > On Thu, Jul 27, 2017, 17:40 Daniel Kiper <dkiper@net-space.pl> wrote:
> > > > On Thu, Jul 27, 2017 at 03:18:16PM +0000, Vladimir 'phcoder' Serbinenko
> > > > wrote:
> > > > > On Thu, Jul 27, 2017, 17:16 Daniel Kiper <dkiper@net-space.pl> wrote:
> > > > > > On Mon, Jun 12, 2017 at 03:53:40PM +0100, Leif Lindholm wrote:
> > > > > > > The 32-bit arm Linux kernel is built as a zImage, which
> > > > self-decompresses
> > > > > > > down to near start of RAM. In order for an initrd/initramfs to be
> > > > > > > accessible, it needs to be placed within the first ~768MB of RAM.
> > > > > > > The initrd loader built into the kernel EFI stub restricts this down
> > > > to
> > > > > > > 512MB for simplicity - so enable the same restriction in grub.
> > > > > > >
> > > > > > > For arm64, the requirement is within a 1GB aligned 32GB window also
> > > > > > > covering the (runtime) kernel image. Since the EFI stub loader itself
> > > > > > > will attempt to relocate to near start of RAM, force initrd to be
> > > > loaded
> > > > > > > completely within the first 32GB of RAM.
> > > > > > >
> > > > > > > Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
> > > > > > > ---
> > > > > > >  grub-core/loader/arm64/linux.c | 39
> > > > > > ++++++++++++++++++++++++++++++++++++++-
> > > > > > >  1 file changed, 38 insertions(+), 1 deletion(-)
> > > > > > >
> > > > > > > diff --git a/grub-core/loader/arm64/linux.c
> > > > > > b/grub-core/loader/arm64/linux.c
> > > > > > > index 8cd44230d..7e989c2b9 100644
> > > > > > > --- a/grub-core/loader/arm64/linux.c
> > > > > > > +++ b/grub-core/loader/arm64/linux.c
> > > > > > > @@ -35,6 +35,23 @@
> > > > > > >
> > > > > > >  GRUB_MOD_LICENSE ("GPLv3+");
> > > > > > >
> > > > > > > +/*
> > > > > > > + * As per linux/Documentation/arm/Booting
> > > > > > > + * ARM initrd needs to be covered by kernel linear mapping,
> > > > > > > + * so place it in the first 512MB of DRAM.
> > > > > > > + *
> > > > > > > + * As per linux/Documentation/arm64/booting.txt
> > > > > > > + * ARM64 initrd needs to be contained entirely within a 1GB aligned
> > > > > > window
> > > > > > > + * of up to 32GB of size that covers the kernel image as well.
> > > > > > > + * Since the EFI stub loader will attempt to load the kernel near
> > > > start
> > > > > > of
> > > > > > > + * RAM, place the buffer in the first 32GB of RAM.
> > > > > > > + */
> > > > > > > +#ifdef __arm__
> > > > > > > +#define INITRD_MAX_ADDRESS_OFFSET (512U * 1024 * 1024)
> > > > > > > +#else /* __aarch64__ */
> > > > > > > +#define INITRD_MAX_ADDRESS_OFFSET (32ULL * 1024 * 1024 * 1024)
> > > > > > > +#endif
> > > > > > > +
> > > > > > >  static grub_dl_t my_mod;
> > > > > > >  static int loaded;
> > > > > > >
> > > > > > > @@ -194,6 +211,25 @@ grub_linux_unload (void)
> > > > > > >    return GRUB_ERR_NONE;
> > > > > > >  }
> > > > > > >
> > > > > > > +/*
> > > > > > > + * This function returns a pointer to a legally allocated initrd
> > > > buffer,
> > > > > > > + * or NULL if unsuccessful
> > > > > > > + */
> > > > > > > +static void *
> > > > > > > +allocate_initrd_mem (int initrd_pages)
> > > > > > > +{
> > > > > > > +  grub_addr_t max_addr;
> > > > > > > +
> > > > > > > +  if (grub_efi_get_dram_base (&max_addr) != GRUB_ERR_NONE)
> > > > > > > +    return NULL;
> > > > > > > +
> > > > > > > +  max_addr += INITRD_MAX_ADDRESS_OFFSET - 1;
> > > > > >
> > > > > > I do not understand this. Why do not pass simply
> > > > INITRD_MAX_ADDRESS_OFFSET
> > > > > > instead of max_addr to grub_efi_allocate_pages_real()?
> > > > > >
> > > > > On ARM it's common for RAM to start at address different from 0
> > > >
> > > > OK, but, AIUI we have to load initrd no higher than
> > > > INITRD_MAX_ADDRESS_OFFSET.
> > > > So, we do not care where the region starts. We care where region ends.
> > > > Am I missing something?
> > > >
> > > The limit is relative to the start of RAM.
> >
> > OK, then it make sense. Thanks for clarification. I would just
> > ask for a comment to avoid reader confusion.
>
> Would moving this function next to the comment block this patch adds
> to the top of the file be sufficient?

What if somebody adds another function before that one? So, please
add a comment into the function body.

Daniel

Re: [PATCH 7/7] efi: change heap allocation type to GRUB_EFI_LOADER_CODE

[Daniel Kiper]

On Fri, Jul 28, 2017 at 10:35:38PM +0100, Leif Lindholm wrote:
> On Thu, Jul 27, 2017 at 06:06:18PM +0200, Daniel Kiper wrote:
> > > > > > > diff --git a/grub-core/kern/efi/mm.c b/grub-core/kern/efi/mm.c
> > > > > > > index 7b1763bc5..f27a48e68 100644
> > > > > > > --- a/grub-core/kern/efi/mm.c
> > > > > > > +++ b/grub-core/kern/efi/mm.c
> > > > > > > @@ -404,7 +404,9 @@ add_memory_regions (grub_efi_memory_descriptor_t
> > > > > > *memory_map,
> > > > > > >         pages = required_pages;
> > > > > > >       }
> > > > > > >
> > > > > > > -      addr = grub_efi_allocate_pages (start, pages);
> > > > > > > +      addr = grub_efi_allocate_pages_real (start, pages,
> > > > > > > +                                        GRUB_EFI_ALLOCATE_ADDRESS,
> > > > > > > +                                        GRUB_EFI_LOADER_CODE);
> > > > > >
> > > > > > Is it really needed? Is any module exectued in place or does it contain
> > > > > > code ready to run out of the box?
> > > > > >
> > > > > All the modules are loaded to heap
> > > >
> > > > I do not see why modules have to be loaded into memory region with
> > > > GRUB_EFI_LOADER_CODE type.
> > >
> > > He means grub modules not initrd or multiboot modules
> >
> > Ahhh... Right, then it should be correct. Though I would double
> > check it applies to GRUB2 modules only.
>
> It applies to any executable code running until an operating system
> takes over.

It looks that I was misunderstood. I hope that this change only influence
allocations for GRUB2 modules and does not change allocation behavior for
others, i.e. OS kernels, Multiboot proto, Multiboo2 proto, etc.

> Don't get me wrong - this is more of a workaround than a fix for the
> grub module loading. But properly separating code/data and ro/rw
> regions is a much more invasive change which deserves more discussion.

Yep, you are right. Added to TODO list.

Daniel

[PATCH 3/7] efi: move fdt helper library

[Leif Lindholm]

There is nothing ARM64 (or even ARM) specific about the efi fdt
helper library, which is used for locating or overriding a
firmware-provided devicetree in a UEFI system - so move it to
loader/efi for reuse.

Move the fdtload.h include file to grub/efi and move the (at least
theoretically) machine dependent page size definitions to
grub/machine/memory.h.
---
 grub-core/Makefile.core.def           |  2 +-
 grub-core/loader/arm64/linux.c        |  3 ++-
 grub-core/loader/arm64/xen_boot.c     |  3 ++-
 grub-core/loader/{arm64 => efi}/fdt.c | 11 ++++++-----
 include/grub/arm64/efi/memory.h       |  3 +++
 include/grub/{arm64 => efi}/fdtload.h |  3 ---
 6 files changed, 14 insertions(+), 11 deletions(-)
 rename grub-core/loader/{arm64 => efi}/fdt.c (93%)
 rename include/grub/{arm64 => efi}/fdtload.h (89%)

diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
index 2dfa22a92..2d8d8618d 100644
--- a/grub-core/Makefile.core.def
+++ b/grub-core/Makefile.core.def
@@ -1677,7 +1677,7 @@ module = {
 
 module = {
   name = fdt;
-  arm64 = loader/arm64/fdt.c;
+  arm64 = loader/efi/fdt.c;
   common = lib/fdt.c;
   enable = fdt;
 };
diff --git a/grub-core/loader/arm64/linux.c b/grub-core/loader/arm64/linux.c
index 9519d2e4d..02e405460 100644
--- a/grub-core/loader/arm64/linux.c
+++ b/grub-core/loader/arm64/linux.c
@@ -26,11 +26,12 @@
 #include <grub/mm.h>
 #include <grub/types.h>
 #include <grub/cpu/linux.h>
-#include <grub/cpu/fdtload.h>
 #include <grub/efi/efi.h>
+#include <grub/efi/fdtload.h>
 #include <grub/efi/pe32.h>
 #include <grub/i18n.h>
 #include <grub/lib/cmdline.h>
+#include <grub/machine/memory.h>
 
 GRUB_MOD_LICENSE ("GPLv3+");
 
diff --git a/grub-core/loader/arm64/xen_boot.c b/grub-core/loader/arm64/xen_boot.c
index a914eb8e2..341805c50 100644
--- a/grub-core/loader/arm64/xen_boot.c
+++ b/grub-core/loader/arm64/xen_boot.c
@@ -27,12 +27,13 @@
 #include <grub/misc.h>
 #include <grub/mm.h>
 #include <grub/types.h>
-#include <grub/cpu/fdtload.h>
 #include <grub/cpu/linux.h>
 #include <grub/efi/efi.h>
+#include <grub/efi/fdtload.h>
 #include <grub/efi/pe32.h>	/* required by struct xen_hypervisor_header */
 #include <grub/i18n.h>
 #include <grub/lib/cmdline.h>
+#include <grub/machine/memory.h>
 
 GRUB_MOD_LICENSE ("GPLv3+");
 
diff --git a/grub-core/loader/arm64/fdt.c b/grub-core/loader/efi/fdt.c
similarity index 93%
rename from grub-core/loader/arm64/fdt.c
rename to grub-core/loader/efi/fdt.c
index db49cf649..e2899c47b 100644
--- a/grub-core/loader/arm64/fdt.c
+++ b/grub-core/loader/efi/fdt.c
@@ -18,12 +18,13 @@
 
 #include <grub/fdt.h>
 #include <grub/mm.h>
-#include <grub/cpu/fdtload.h>
 #include <grub/err.h>
 #include <grub/dl.h>
 #include <grub/command.h>
 #include <grub/file.h>
 #include <grub/efi/efi.h>
+#include <grub/efi/fdtload.h>
+#include <grub/machine/memory.h>
 
 static void *loaded_fdt;
 static void *fdt;
@@ -32,12 +33,12 @@ void *
 grub_fdt_load (grub_size_t additional_size)
 {
   void *raw_fdt;
-  grub_size_t size;
+  unsigned int size;
 
   if (fdt)
     {
       size = GRUB_EFI_BYTES_TO_PAGES (grub_fdt_get_totalsize (fdt));
-      grub_efi_free_pages ((grub_efi_physical_address_t) fdt, size);
+      grub_efi_free_pages ((grub_addr_t) fdt, size);
     }
 
   if (loaded_fdt)
@@ -49,7 +50,7 @@ grub_fdt_load (grub_size_t additional_size)
     raw_fdt ? grub_fdt_get_totalsize (raw_fdt) : GRUB_FDT_EMPTY_TREE_SZ;
   size += additional_size;
 
-  grub_dprintf ("linux", "allocating %ld bytes for fdt\n", size);
+  grub_dprintf ("linux", "allocating %d bytes for fdt\n", size);
   fdt = grub_efi_allocate_pages (0, GRUB_EFI_BYTES_TO_PAGES (size));
   if (!fdt)
     return NULL;
@@ -88,7 +89,7 @@ grub_fdt_unload (void) {
   if (!fdt) {
     return;
   }
-  grub_efi_free_pages ((grub_efi_physical_address_t) fdt,
+  grub_efi_free_pages ((grub_addr_t) fdt,
 		       GRUB_EFI_BYTES_TO_PAGES (grub_fdt_get_totalsize (fdt)));
   fdt = NULL;
 }
diff --git a/include/grub/arm64/efi/memory.h b/include/grub/arm64/efi/memory.h
index c6cb32417..9414b69e0 100644
--- a/include/grub/arm64/efi/memory.h
+++ b/include/grub/arm64/efi/memory.h
@@ -3,4 +3,7 @@
 
 #define GRUB_EFI_MAX_USABLE_ADDRESS 0xffffffffffffULL
 
+#define GRUB_EFI_PAGE_SHIFT	12
+#define GRUB_EFI_BYTES_TO_PAGES(bytes)   (((bytes) + 0xfff) >> GRUB_EFI_PAGE_SHIFT)
+
 #endif /* ! GRUB_MEMORY_CPU_HEADER */
diff --git a/include/grub/arm64/fdtload.h b/include/grub/efi/fdtload.h
similarity index 89%
rename from include/grub/arm64/fdtload.h
rename to include/grub/efi/fdtload.h
index 7b9ddba91..713c9424d 100644
--- a/include/grub/arm64/fdtload.h
+++ b/include/grub/efi/fdtload.h
@@ -29,7 +29,4 @@ grub_fdt_unload (void);
 grub_err_t
 grub_fdt_install (void);
 
-#define GRUB_EFI_PAGE_SHIFT	12
-#define GRUB_EFI_BYTES_TO_PAGES(bytes)   (((bytes) + 0xfff) >> GRUB_EFI_PAGE_SHIFT)
-
 #endif
-- 
2.11.0