[PATCH 0/4] x86,efi: prerequisites for ARM* cleanup series

[PATCH 0/4] x86,efi: prerequisites for ARM* cleanup series

[Leif Lindholm]

Breaking this up into a separate prerequisite series to reduce the
messiness of the series - this set deals with items that do not
change any ARM behaviour (but touches some of the files).

First is a new patch to insert an i386 namespace in the x86 linux
header definitions, to prevent clashes with other architectures.

The remaining three are taken from the previous round of the EFI
cleanup series, with feedback addressed (mostly the addition of
comments).

Leif Lindholm (4):
  i386/x86_64: make linux header definitions arch-specific
  arm64/efi: move EFI_PAGE definitions to efi/memory.h
  efi: add central copy of grub_efi_find_mmap_size
  loader: drop local implementations of find_efi_mmap_size

 grub-core/commands/file.c         |  6 ++---
 grub-core/kern/efi/mm.c           | 47 +++++++++++++++++++++++++++++++++
 grub-core/loader/arm64/linux.c    |  1 +
 grub-core/loader/arm64/xen_boot.c |  1 +
 grub-core/loader/efi/fdt.c        |  1 +
 grub-core/loader/i386/linux.c     | 55 +++------------------------------------
 grub-core/loader/i386/pc/linux.c  | 12 ++++-----
 grub-core/loader/i386/xen_file.c  |  4 +--
 grub-core/loader/multiboot_mbi2.c | 38 +--------------------------
 include/grub/efi/efi.h            |  1 +
 include/grub/efi/fdtload.h        |  3 ---
 include/grub/efi/memory.h         |  7 +++++
 include/grub/i386/linux.h         | 10 +++----
 13 files changed, 78 insertions(+), 108 deletions(-)

-- 
2.11.0

[PATCH 1/4] i386/x86_64: make linux header definitions arch-specific

[Leif Lindholm]

Rename GRUB_LINUX_MAGIC_SIGNATURE -> GRUB_LINUX_I386_MAGIC_SIGNATURE
and linux_kernel_header -> linux_i386_kernel_header.

Also change the include guard on grub/i386/linux.h to prevent clashes
when including multiple linux.h headers (like in commands/file.c).

Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
---

OK, so one question here: in order to follow existing style, I have kept
to the existing linux_ format. Is that the appropriate prefix, or should
this be grub_linux_ like I originally used in the ARM series?
Regardless, I intend to align the ARM patches to whatever format ends up
being used for this patch.

 grub-core/commands/file.c        |  6 +++---
 grub-core/loader/i386/linux.c    |  4 ++--
 grub-core/loader/i386/pc/linux.c | 12 ++++++------
 grub-core/loader/i386/xen_file.c |  4 ++--
 include/grub/i386/linux.h        | 10 +++++-----
 5 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/grub-core/commands/file.c b/grub-core/commands/file.c
index 12fba99e0..63c84499b 100644
--- a/grub-core/commands/file.c
+++ b/grub-core/commands/file.c
@@ -497,7 +497,7 @@ grub_cmd_file (grub_extcmd_context_t ctxt, int argc, char **args)
     case IS_X86_LINUX32:
     case IS_X86_LINUX:
       {
-	struct linux_kernel_header lh;
+	struct linux_i386_kernel_header lh;
 	if (grub_file_read (file, &lh, sizeof (lh)) != sizeof (lh))
 	  break;
 	if (lh.boot_flag != grub_cpu_to_le16_compile_time (0xaa55))
@@ -508,7 +508,7 @@ grub_cmd_file (grub_extcmd_context_t ctxt, int argc, char **args)
 
 	/* FIXME: some really old kernels (< 1.3.73) will fail this.  */
 	if (lh.header !=
-	    grub_cpu_to_le32_compile_time (GRUB_LINUX_MAGIC_SIGNATURE)
+	    grub_cpu_to_le32_compile_time (GRUB_LINUX_I386_MAGIC_SIGNATURE)
 	    || grub_le_to_cpu16 (lh.version) < 0x0200)
 	  break;
 
@@ -521,7 +521,7 @@ grub_cmd_file (grub_extcmd_context_t ctxt, int argc, char **args)
 	/* FIXME: 2.03 is not always good enough (Linux 2.4 can be 2.03 and
 	   still not support 32-bit boot.  */
 	if (lh.header !=
-	    grub_cpu_to_le32_compile_time (GRUB_LINUX_MAGIC_SIGNATURE)
+	    grub_cpu_to_le32_compile_time (GRUB_LINUX_I386_MAGIC_SIGNATURE)
 	    || grub_le_to_cpu16 (lh.version) < 0x0203)
 	  break;
 
diff --git a/grub-core/loader/i386/linux.c b/grub-core/loader/i386/linux.c
index 083f9417c..44301e126 100644
--- a/grub-core/loader/i386/linux.c
+++ b/grub-core/loader/i386/linux.c
@@ -678,7 +678,7 @@ grub_cmd_linux (grub_command_t cmd __attribute__ ((unused)),
 		int argc, char *argv[])
 {
   grub_file_t file = 0;
-  struct linux_kernel_header lh;
+  struct linux_i386_kernel_header lh;
   grub_uint8_t setup_sects;
   grub_size_t real_size, prot_size, prot_file_size;
   grub_ssize_t len;
@@ -721,7 +721,7 @@ grub_cmd_linux (grub_command_t cmd __attribute__ ((unused)),
 
   /* FIXME: 2.03 is not always good enough (Linux 2.4 can be 2.03 and
      still not support 32-bit boot.  */
-  if (lh.header != grub_cpu_to_le32_compile_time (GRUB_LINUX_MAGIC_SIGNATURE)
+  if (lh.header != grub_cpu_to_le32_compile_time (GRUB_LINUX_I386_MAGIC_SIGNATURE)
       || grub_le_to_cpu16 (lh.version) < 0x0203)
     {
       grub_error (GRUB_ERR_BAD_OS, "version too old for 32-bit boot"
diff --git a/grub-core/loader/i386/pc/linux.c b/grub-core/loader/i386/pc/linux.c
index a293b17aa..b69cb7a3a 100644
--- a/grub-core/loader/i386/pc/linux.c
+++ b/grub-core/loader/i386/pc/linux.c
@@ -121,7 +121,7 @@ grub_cmd_linux (grub_command_t cmd __attribute__ ((unused)),
 		int argc, char *argv[])
 {
   grub_file_t file = 0;
-  struct linux_kernel_header lh;
+  struct linux_i386_kernel_header lh;
   grub_uint8_t setup_sects;
   grub_size_t real_size;
   grub_ssize_t len;
@@ -169,7 +169,7 @@ grub_cmd_linux (grub_command_t cmd __attribute__ ((unused)),
 
   maximal_cmdline_size = 256;
 
-  if (lh.header == grub_cpu_to_le32_compile_time (GRUB_LINUX_MAGIC_SIGNATURE)
+  if (lh.header == grub_cpu_to_le32_compile_time (GRUB_LINUX_I386_MAGIC_SIGNATURE)
       && grub_le_to_cpu16 (lh.version) >= 0x0200)
     {
       grub_linux_is_bzimage = (lh.loadflags & GRUB_LINUX_FLAG_BIG_KERNEL);
@@ -322,7 +322,7 @@ grub_cmd_linux (grub_command_t cmd __attribute__ ((unused)),
       goto fail;
     }
 
-  if (lh.header != grub_cpu_to_le32_compile_time (GRUB_LINUX_MAGIC_SIGNATURE)
+  if (lh.header != grub_cpu_to_le32_compile_time (GRUB_LINUX_I386_MAGIC_SIGNATURE)
       || grub_le_to_cpu16 (lh.version) < 0x0200)
     /* Clear the heap space.  */
     grub_memset (grub_linux_real_chunk
@@ -387,7 +387,7 @@ grub_cmd_initrd (grub_command_t cmd __attribute__ ((unused)),
 {
   grub_size_t size = 0;
   grub_addr_t addr_max, addr_min;
-  struct linux_kernel_header *lh;
+  struct linux_i386_kernel_header *lh;
   grub_uint8_t *initrd_chunk;
   grub_addr_t initrd_addr;
   grub_err_t err;
@@ -405,9 +405,9 @@ grub_cmd_initrd (grub_command_t cmd __attribute__ ((unused)),
       goto fail;
     }
 
-  lh = (struct linux_kernel_header *) grub_linux_real_chunk;
+  lh = (struct linux_i386_kernel_header *) grub_linux_real_chunk;
 
-  if (!(lh->header == grub_cpu_to_le32_compile_time (GRUB_LINUX_MAGIC_SIGNATURE)
+  if (!(lh->header == grub_cpu_to_le32_compile_time (GRUB_LINUX_I386_MAGIC_SIGNATURE)
 	&& grub_le_to_cpu16 (lh->version) >= 0x0200))
     {
       grub_error (GRUB_ERR_BAD_OS, "the kernel is too old for initrd");
diff --git a/grub-core/loader/i386/xen_file.c b/grub-core/loader/i386/xen_file.c
index 99fad4cad..77a93e7b2 100644
--- a/grub-core/loader/i386/xen_file.c
+++ b/grub-core/loader/i386/xen_file.c
@@ -26,7 +26,7 @@ grub_elf_t
 grub_xen_file (grub_file_t file)
 {
   grub_elf_t elf;
-  struct linux_kernel_header lh;
+  struct linux_i386_kernel_header lh;
   grub_file_t off_file;
   grub_uint32_t payload_offset, payload_length;
   grub_uint8_t magic[6];
@@ -43,7 +43,7 @@ grub_xen_file (grub_file_t file)
     goto fail;
 
   if (lh.boot_flag != grub_cpu_to_le16_compile_time (0xaa55)
-      || lh.header != grub_cpu_to_le32_compile_time (GRUB_LINUX_MAGIC_SIGNATURE)
+      || lh.header != grub_cpu_to_le32_compile_time (GRUB_LINUX_I386_MAGIC_SIGNATURE)
       || grub_le_to_cpu16 (lh.version) < 0x0208)
     {
       grub_error (GRUB_ERR_BAD_OS, "version too old for xen boot");
diff --git a/include/grub/i386/linux.h b/include/grub/i386/linux.h
index da0ca3b83..854e3e687 100644
--- a/include/grub/i386/linux.h
+++ b/include/grub/i386/linux.h
@@ -16,10 +16,10 @@
  *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
  */
 
-#ifndef GRUB_LINUX_MACHINE_HEADER
-#define GRUB_LINUX_MACHINE_HEADER	1
+#ifndef GRUB_LINUX_I386_HEADER
+#define GRUB_LINUX_I386_HEADER	1
 
-#define GRUB_LINUX_MAGIC_SIGNATURE	0x53726448      /* "HdrS" */
+#define GRUB_LINUX_I386_MAGIC_SIGNATURE	0x53726448      /* "HdrS" */
 #define GRUB_LINUX_DEFAULT_SETUP_SECTS	4
 #define GRUB_LINUX_INITRD_MAX_ADDRESS	0x37FFFFFF
 #define GRUB_LINUX_MAX_SETUP_SECTS	64
@@ -85,7 +85,7 @@ enum
   };
 
 /* For the Linux/i386 boot protocol version 2.10.  */
-struct linux_kernel_header
+struct linux_i386_kernel_header
 {
   grub_uint8_t code1[0x0020];
   grub_uint16_t cl_magic;		/* Magic number 0xA33F */
@@ -312,4 +312,4 @@ struct linux_kernel_params
 } GRUB_PACKED;
 #endif /* ! ASM_FILE */
 
-#endif /* ! GRUB_LINUX_MACHINE_HEADER */
+#endif /* ! GRUB_LINUX_I386_HEADER */
-- 
2.11.0

[PATCH 2/4] arm64/efi: move EFI_PAGE definitions to efi/memory.h

[Leif Lindholm]

The EFI page definitions and macros are generic and should not be confined
to arm64 headers - so move to efi/memory.h.
Also add EFI_PAGE_SIZE macro.

Update loader sources to reflect new header location.

Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
---
 grub-core/loader/arm64/linux.c    | 1 +
 grub-core/loader/arm64/xen_boot.c | 1 +
 grub-core/loader/efi/fdt.c        | 1 +
 include/grub/efi/fdtload.h        | 3 ---
 include/grub/efi/memory.h         | 7 +++++++
 5 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/grub-core/loader/arm64/linux.c b/grub-core/loader/arm64/linux.c
index 746edd104..ca01a2349 100644
--- a/grub-core/loader/arm64/linux.c
+++ b/grub-core/loader/arm64/linux.c
@@ -28,6 +28,7 @@
 #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>
 #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 c95d6c5a8..67ada136b 100644
--- a/grub-core/loader/arm64/xen_boot.c
+++ b/grub-core/loader/arm64/xen_boot.c
@@ -30,6 +30,7 @@
 #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/efi/fdt.c b/grub-core/loader/efi/fdt.c
index 17212c38d..c0c6800f7 100644
--- a/grub-core/loader/efi/fdt.c
+++ b/grub-core/loader/efi/fdt.c
@@ -24,6 +24,7 @@
 #include <grub/file.h>
 #include <grub/efi/efi.h>
 #include <grub/efi/fdtload.h>
+#include <grub/efi/memory.h>
 
 static void *loaded_fdt;
 static void *fdt;
diff --git a/include/grub/efi/fdtload.h b/include/grub/efi/fdtload.h
index 7b9ddba91..713c9424d 100644
--- a/include/grub/efi/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..08fe62277 100644
--- a/include/grub/efi/memory.h
+++ b/include/grub/efi/memory.h
@@ -22,6 +22,13 @@
 #include <grub/err.h>
 #include <grub/types.h>
 
+/* The term "page" in UEFI refers only to a 4 KiB-aligned 4 KiB size region of
+   memory. It is not concerned with underlying translation management concepts,
+   but only used as the granule for memory allocations. */
+#define GRUB_EFI_PAGE_SHIFT             12
+#define GRUB_EFI_PAGE_SIZE              (1 << GRUB_EFI_PAGE_SHIFT)
+#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 3/4] efi: add central copy of grub_efi_find_mmap_size

[Leif Lindholm]

There are several implementations of this function in the tree.
Add a central version in grub-core/efi/mm.c.

Taken from grub-core/loader/i386/linux.c, changing some hard-coded constants
to use macros from efi/memory.h.

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

diff --git a/grub-core/kern/efi/mm.c b/grub-core/kern/efi/mm.c
index ac2a4c556..8795aa1e0 100644
--- a/grub-core/kern/efi/mm.c
+++ b/grub-core/kern/efi/mm.c
@@ -218,6 +218,53 @@ grub_efi_finish_boot_services (grub_efi_uintn_t *outbuf_size, void *outbuf,
   return GRUB_ERR_NONE;
 }
 
+/* To obtain the UEFI memory map, we must pass a buffer of sufficient size
+   to hold the entire map. This function returns a sane start value for
+   buffer size.  */
+grub_efi_uintn_t
+grub_efi_find_mmap_size (void)
+{
+  static grub_efi_uintn_t mmap_size = 0;
+
+  if (mmap_size != 0)
+    return mmap_size;
+
+  mmap_size = 1 * GRUB_EFI_PAGE_SIZE;
+  while (1)
+    {
+      int ret;
+      grub_efi_memory_descriptor_t *mmap;
+      grub_efi_uintn_t desc_size;
+      grub_efi_uintn_t cur_mmap_size = mmap_size;
+
+      mmap = grub_malloc (cur_mmap_size);
+      if (! mmap)
+	return 0;
+
+      ret = grub_efi_get_memory_map (&cur_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;
+
+      if (mmap_size < cur_mmap_size)
+	mmap_size = cur_mmap_size;
+      mmap_size += GRUB_EFI_PAGE_SIZE;
+    }
+
+  /* Increase the size a bit for safety, because GRUB allocates more on
+     later, and EFI itself may allocate more.  */
+  mmap_size += 3 * GRUB_EFI_PAGE_SIZE;
+
+  mmap_size = ALIGN_UP (mmap_size, GRUB_EFI_PAGE_SIZE);
+  return mmap_size;
+}
+
 /* Get the memory map as defined in the EFI spec. Return 1 if successful,
    return 0 if partial, or return -1 if an error occurs.  */
 int
diff --git a/include/grub/efi/efi.h b/include/grub/efi/efi.h
index 3fa082816..8f1f36c8c 100644
--- a/include/grub/efi/efi.h
+++ b/include/grub/efi/efi.h
@@ -49,6 +49,7 @@ void *
 EXPORT_FUNC(grub_efi_allocate_any_pages) (grub_efi_uintn_t pages);
 void EXPORT_FUNC(grub_efi_free_pages) (grub_efi_physical_address_t address,
 				       grub_efi_uintn_t pages);
+grub_efi_uintn_t EXPORT_FUNC(grub_efi_find_mmap_size) (void);
 int
 EXPORT_FUNC(grub_efi_get_memory_map) (grub_efi_uintn_t *memory_map_size,
 				      grub_efi_memory_descriptor_t *memory_map,
-- 
2.11.0

[PATCH 4/4] loader: drop local implementations of find_efi_mmap_size

[Leif Lindholm]

Now we have a grub_efi_find_mmap_size, replace near-identical implementations
in i386/linux and mbi2 loaders with calls to that one.

Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
---
 grub-core/loader/i386/linux.c     | 51 +--------------------------------------
 grub-core/loader/multiboot_mbi2.c | 38 +----------------------------
 2 files changed, 2 insertions(+), 87 deletions(-)

diff --git a/grub-core/loader/i386/linux.c b/grub-core/loader/i386/linux.c
index 44301e126..ab02add53 100644
--- a/grub-core/loader/i386/linux.c
+++ b/grub-core/loader/i386/linux.c
@@ -101,55 +101,6 @@ page_align (grub_size_t size)
   return (size + (1 << 12) - 1) & (~((1 << 12) - 1));
 }
 
-#ifdef GRUB_MACHINE_EFI
-/* 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_efi_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;
-      grub_efi_uintn_t cur_mmap_size = mmap_size;
-
-      mmap = grub_malloc (cur_mmap_size);
-      if (! mmap)
-	return 0;
-
-      ret = grub_efi_get_memory_map (&cur_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;
-
-      if (mmap_size < cur_mmap_size)
-	mmap_size = cur_mmap_size;
-      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 += (3 << 12);
-
-  mmap_size = page_align (mmap_size);
-  return mmap_size;
-}
-
-#endif
-
 /* Helper for find_mmap_size.  */
 static int
 count_hook (grub_uint64_t addr __attribute__ ((unused)),
@@ -560,7 +511,7 @@ grub_linux_boot (void)
   ctx.real_size = ALIGN_UP (cl_offset + maximal_cmdline_size, 4096);
 
 #ifdef GRUB_MACHINE_EFI
-  efi_mmap_size = find_efi_mmap_size ();
+  efi_mmap_size = grub_efi_find_mmap_size ();
   if (efi_mmap_size == 0)
     return grub_errno;
 #endif
diff --git a/grub-core/loader/multiboot_mbi2.c b/grub-core/loader/multiboot_mbi2.c
index b0679a9f6..b5f961494 100644
--- a/grub-core/loader/multiboot_mbi2.c
+++ b/grub-core/loader/multiboot_mbi2.c
@@ -407,42 +407,6 @@ acpiv2_size (void)
 
 static grub_efi_uintn_t efi_mmap_size = 0;
 
-/* Find the optimal number of pages for the memory map. Is it better to
-   move this code to efi/mm.c?  */
-static void
-find_efi_mmap_size (void)
-{
-  efi_mmap_size = (1 << 12);
-  while (1)
-    {
-      int ret;
-      grub_efi_memory_descriptor_t *mmap;
-      grub_efi_uintn_t desc_size;
-      grub_efi_uintn_t cur_mmap_size = efi_mmap_size;
-
-      mmap = grub_malloc (cur_mmap_size);
-      if (! mmap)
-	return;
-
-      ret = grub_efi_get_memory_map (&cur_mmap_size, mmap, 0, &desc_size, 0);
-      grub_free (mmap);
-
-      if (ret < 0)
-	return;
-      else if (ret > 0)
-	break;
-
-      if (efi_mmap_size < cur_mmap_size)
-	efi_mmap_size = cur_mmap_size;
-      efi_mmap_size += (1 << 12);
-    }
-
-  /* Increase the size a bit for safety, because GRUB allocates more on
-     later, and EFI itself may allocate more.  */
-  efi_mmap_size += (3 << 12);
-
-  efi_mmap_size = ALIGN_UP (efi_mmap_size, 4096);
-}
 #endif
 
 static grub_size_t
@@ -463,7 +427,7 @@ grub_multiboot_get_mbi_size (void)
 {
 #ifdef GRUB_MACHINE_EFI
   if (!keep_bs && !efi_mmap_size)
-    find_efi_mmap_size ();    
+    efi_mmap_size = grub_efi_find_mmap_size ();
 #endif
   return 2 * sizeof (grub_uint32_t) + sizeof (struct multiboot_tag)
     + sizeof (struct multiboot_tag)
-- 
2.11.0

Re: [PATCH 1/4] i386/x86_64: make linux header definitions arch-specific

[Daniel Kiper]

Hi Leif,

Sorry for late reply but I am traveling. I should be
more responsive starting from next week.

On Tue, Sep 05, 2017 at 09:41:11PM +0100, Leif Lindholm wrote:
> Rename GRUB_LINUX_MAGIC_SIGNATURE -> GRUB_LINUX_I386_MAGIC_SIGNATURE
> and linux_kernel_header -> linux_i386_kernel_header.

I think that this begs for 2 separate patches.

> Also change the include guard on grub/i386/linux.h to prevent clashes
> when including multiple linux.h headers (like in commands/file.c).

Third patch.

> Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
> ---
>
> OK, so one question here: in order to follow existing style, I have kept
> to the existing linux_ format. Is that the appropriate prefix, or should
> this be grub_linux_ like I originally used in the ARM series?

I prefer linux_.

> Regardless, I intend to align the ARM patches to whatever format ends up
> being used for this patch.

Please do.

Daniel

Re: [PATCH 2/4] arm64/efi: move EFI_PAGE definitions to efi/memory.h

[Daniel Kiper]

On Tue, Sep 05, 2017 at 09:41:12PM +0100, Leif Lindholm wrote:
> The EFI page definitions and macros are generic and should not be confined
> to arm64 headers - so move to efi/memory.h.

I am OK with this.

> Also add EFI_PAGE_SIZE macro.

However, this should be added in 3rd patch.

> Update loader sources to reflect new header location.
>
> Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
> ---
>  grub-core/loader/arm64/linux.c    | 1 +
>  grub-core/loader/arm64/xen_boot.c | 1 +
>  grub-core/loader/efi/fdt.c        | 1 +
>  include/grub/efi/fdtload.h        | 3 ---
>  include/grub/efi/memory.h         | 7 +++++++
>  5 files changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/grub-core/loader/arm64/linux.c b/grub-core/loader/arm64/linux.c
> index 746edd104..ca01a2349 100644
> --- a/grub-core/loader/arm64/linux.c
> +++ b/grub-core/loader/arm64/linux.c
> @@ -28,6 +28,7 @@
>  #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>
>  #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 c95d6c5a8..67ada136b 100644
> --- a/grub-core/loader/arm64/xen_boot.c
> +++ b/grub-core/loader/arm64/xen_boot.c
> @@ -30,6 +30,7 @@
>  #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/efi/fdt.c b/grub-core/loader/efi/fdt.c
> index 17212c38d..c0c6800f7 100644
> --- a/grub-core/loader/efi/fdt.c
> +++ b/grub-core/loader/efi/fdt.c
> @@ -24,6 +24,7 @@
>  #include <grub/file.h>
>  #include <grub/efi/efi.h>
>  #include <grub/efi/fdtload.h>
> +#include <grub/efi/memory.h>
>
>  static void *loaded_fdt;
>  static void *fdt;
> diff --git a/include/grub/efi/fdtload.h b/include/grub/efi/fdtload.h
> index 7b9ddba91..713c9424d 100644
> --- a/include/grub/efi/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..08fe62277 100644
> --- a/include/grub/efi/memory.h
> +++ b/include/grub/efi/memory.h
> @@ -22,6 +22,13 @@
>  #include <grub/err.h>
>  #include <grub/types.h>
>
> +/* The term "page" in UEFI refers only to a 4 KiB-aligned 4 KiB size region of
> +   memory. It is not concerned with underlying translation management concepts,
> +   but only used as the granule for memory allocations. */
> +#define GRUB_EFI_PAGE_SHIFT             12
> +#define GRUB_EFI_PAGE_SIZE              (1 << GRUB_EFI_PAGE_SHIFT)
> +#define GRUB_EFI_BYTES_TO_PAGES(bytes)  (((bytes) + 0xfff) >> GRUB_EFI_PAGE_SHIFT)

If you add GRUB_EFI_PAGE_SIZE why not change GRUB_EFI_BYTES_TO_PAGES() macro to

#define GRUB_EFI_BYTES_TO_PAGES(bytes) ALIGN_UP (bytes, GRUB_EFI_PAGE_SIZE)?

Of course in separate patch.

Daniel

Re: [PATCH 3/4] efi: add central copy of grub_efi_find_mmap_size

[Daniel Kiper]

On Tue, Sep 05, 2017 at 09:41:13PM +0100, Leif Lindholm wrote:
> There are several implementations of this function in the tree.
> Add a central version in grub-core/efi/mm.c.
>
> Taken from grub-core/loader/i386/linux.c, changing some hard-coded constants
> to use macros from efi/memory.h.

I am OK with the idea but...

> Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
> ---
>  grub-core/kern/efi/mm.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
>  include/grub/efi/efi.h  |  1 +
>  2 files changed, 48 insertions(+)
>
> diff --git a/grub-core/kern/efi/mm.c b/grub-core/kern/efi/mm.c
> index ac2a4c556..8795aa1e0 100644
> --- a/grub-core/kern/efi/mm.c
> +++ b/grub-core/kern/efi/mm.c
> @@ -218,6 +218,53 @@ grub_efi_finish_boot_services (grub_efi_uintn_t *outbuf_size, void *outbuf,
>    return GRUB_ERR_NONE;
>  }
>
> +/* To obtain the UEFI memory map, we must pass a buffer of sufficient size
> +   to hold the entire map. This function returns a sane start value for
> +   buffer size.  */
> +grub_efi_uintn_t
> +grub_efi_find_mmap_size (void)
> +{
> +  static grub_efi_uintn_t mmap_size = 0;
> +
> +  if (mmap_size != 0)
> +    return mmap_size;

What will happen if memory will be fragmented further after this call
and number of memory map entries increases above mmap_size?

> +  mmap_size = 1 * GRUB_EFI_PAGE_SIZE;
> +  while (1)
> +    {
> +      int ret;
> +      grub_efi_memory_descriptor_t *mmap;
> +      grub_efi_uintn_t desc_size;
> +      grub_efi_uintn_t cur_mmap_size = mmap_size;
> +
> +      mmap = grub_malloc (cur_mmap_size);
> +      if (! mmap)
> +	return 0;
> +
> +      ret = grub_efi_get_memory_map (&cur_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;
> +
> +      if (mmap_size < cur_mmap_size)
> +	mmap_size = cur_mmap_size;
> +      mmap_size += GRUB_EFI_PAGE_SIZE;
> +    }
> +
> +  /* Increase the size a bit for safety, because GRUB allocates more on
> +     later, and EFI itself may allocate more.  */
> +  mmap_size += 3 * GRUB_EFI_PAGE_SIZE;
> +
> +  mmap_size = ALIGN_UP (mmap_size, GRUB_EFI_PAGE_SIZE);

I prefer if you do something like that:

map_size = 0;

if (grub_efi_get_memory_map (&map_size, NULL, NULL, &desc_size, 0) < 0)
{
  grub_error (GRUB_ERR_IO, "cannot get EFI memory map size");
  return 0;
}

return ALIGN_UP (map_size + GRUB_EFI_PAGE_SIZE, GRUB_EFI_PAGE_SIZE);

Daniel

Re: [PATCH 4/4] loader: drop local implementations of find_efi_mmap_size

[Daniel Kiper]

On Tue, Sep 05, 2017 at 09:41:14PM +0100, Leif Lindholm wrote:
> Now we have a grub_efi_find_mmap_size, replace near-identical implementations
> in i386/linux and mbi2 loaders with calls to that one.

Please do it in separate patches. And I think that
grub-core/commands/efi/lsefimmap.c:grub_cmd_lsefimmap()
begs for similar change too. In general please look for

grub_efi_get_memory_map (&map_size, NULL, NULL, &desc_size, 0)

calls or something like that.

Daniel

Re: [PATCH 3/4] efi: add central copy of grub_efi_find_mmap_size

[Leif Lindholm]

On Wed, Sep 13, 2017 at 06:39:11AM +0200, Daniel Kiper wrote:
> On Tue, Sep 05, 2017 at 09:41:13PM +0100, Leif Lindholm wrote:
> > There are several implementations of this function in the tree.
> > Add a central version in grub-core/efi/mm.c.
> >
> > Taken from grub-core/loader/i386/linux.c, changing some hard-coded constants
> > to use macros from efi/memory.h.
> 
> I am OK with the idea but...
> 
> > Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
> > ---
> >  grub-core/kern/efi/mm.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
> >  include/grub/efi/efi.h  |  1 +
> >  2 files changed, 48 insertions(+)
> >
> > diff --git a/grub-core/kern/efi/mm.c b/grub-core/kern/efi/mm.c
> > index ac2a4c556..8795aa1e0 100644
> > --- a/grub-core/kern/efi/mm.c
> > +++ b/grub-core/kern/efi/mm.c
> > @@ -218,6 +218,53 @@ grub_efi_finish_boot_services (grub_efi_uintn_t *outbuf_size, void *outbuf,
> >    return GRUB_ERR_NONE;
> >  }
> >
> > +/* To obtain the UEFI memory map, we must pass a buffer of sufficient size
> > +   to hold the entire map. This function returns a sane start value for
> > +   buffer size.  */
> > +grub_efi_uintn_t
> > +grub_efi_find_mmap_size (void)
> > +{
> > +  static grub_efi_uintn_t mmap_size = 0;
> > +
> > +  if (mmap_size != 0)
> > +    return mmap_size;
> 
> What will happen if memory will be fragmented further after this call
> and number of memory map entries increases above mmap_size?

Oh, that is completely broken, but there is no functional change
introduced by this patch.

I was just hoping there was some point at which I could stop fixing
the world in order to get to the point where upstream grub is able to
boot on arm64 systems with large amounts of memory, or holes in the
memory map.

A journey I commenced on February 28th this year:
http://lists.gnu.org/archive/html/grub-devel/2017-02/msg00156.html

I note that this patch was in itself triggered by your feedback on
"efi: add grub_efi_get_dram_base() function for arm*" on 27 July.

/
    Leif

> > +  mmap_size = 1 * GRUB_EFI_PAGE_SIZE;
> > +  while (1)
> > +    {
> > +      int ret;
> > +      grub_efi_memory_descriptor_t *mmap;
> > +      grub_efi_uintn_t desc_size;
> > +      grub_efi_uintn_t cur_mmap_size = mmap_size;
> > +
> > +      mmap = grub_malloc (cur_mmap_size);
> > +      if (! mmap)
> > +	return 0;
> > +
> > +      ret = grub_efi_get_memory_map (&cur_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;
> > +
> > +      if (mmap_size < cur_mmap_size)
> > +	mmap_size = cur_mmap_size;
> > +      mmap_size += GRUB_EFI_PAGE_SIZE;
> > +    }
> > +
> > +  /* Increase the size a bit for safety, because GRUB allocates more on
> > +     later, and EFI itself may allocate more.  */
> > +  mmap_size += 3 * GRUB_EFI_PAGE_SIZE;
> > +
> > +  mmap_size = ALIGN_UP (mmap_size, GRUB_EFI_PAGE_SIZE);
> 
> I prefer if you do something like that:
> 
> map_size = 0;
> 
> if (grub_efi_get_memory_map (&map_size, NULL, NULL, &desc_size, 0) < 0)
> {
>   grub_error (GRUB_ERR_IO, "cannot get EFI memory map size");
>   return 0;
> }
> 
> return ALIGN_UP (map_size + GRUB_EFI_PAGE_SIZE, GRUB_EFI_PAGE_SIZE);
> 
> Daniel

Re: [PATCH 1/4] i386/x86_64: make linux header definitions arch-specific

[Leif Lindholm]

On Wed, Sep 13, 2017 at 05:45:58AM +0200, Daniel Kiper wrote:
> Hi Leif,
> 
> Sorry for late reply but I am traveling. I should be
> more responsive starting from next week.
> 
> On Tue, Sep 05, 2017 at 09:41:11PM +0100, Leif Lindholm wrote:
> > Rename GRUB_LINUX_MAGIC_SIGNATURE -> GRUB_LINUX_I386_MAGIC_SIGNATURE
> > and linux_kernel_header -> linux_i386_kernel_header.
> 
> I think that this begs for 2 separate patches.

Sure.

> > Also change the include guard on grub/i386/linux.h to prevent clashes
> > when including multiple linux.h headers (like in commands/file.c).
> 
> Third patch.

Sure.

> > Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
> > ---
> >
> > OK, so one question here: in order to follow existing style, I have kept
> > to the existing linux_ format. Is that the appropriate prefix, or should
> > this be grub_linux_ like I originally used in the ARM series?
> 
> I prefer linux_.

Sure.

> > Regardless, I intend to align the ARM patches to whatever format ends up
> > being used for this patch.
> 
> Please do.
> 
> Daniel

Re: [PATCH 2/4] arm64/efi: move EFI_PAGE definitions to efi/memory.h

[Leif Lindholm]

On Wed, Sep 13, 2017 at 06:00:21AM +0200, Daniel Kiper wrote:
> On Tue, Sep 05, 2017 at 09:41:12PM +0100, Leif Lindholm wrote:
> > The EFI page definitions and macros are generic and should not be confined
> > to arm64 headers - so move to efi/memory.h.
> 
> I am OK with this.
> 
> > Also add EFI_PAGE_SIZE macro.
> 
> However, this should be added in 3rd patch.

Sure.

> > Update loader sources to reflect new header location.
> >
> > Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
> > ---
> >  grub-core/loader/arm64/linux.c    | 1 +
> >  grub-core/loader/arm64/xen_boot.c | 1 +
> >  grub-core/loader/efi/fdt.c        | 1 +
> >  include/grub/efi/fdtload.h        | 3 ---
> >  include/grub/efi/memory.h         | 7 +++++++
> >  5 files changed, 10 insertions(+), 3 deletions(-)
> >
> > diff --git a/grub-core/loader/arm64/linux.c b/grub-core/loader/arm64/linux.c
> > index 746edd104..ca01a2349 100644
> > --- a/grub-core/loader/arm64/linux.c
> > +++ b/grub-core/loader/arm64/linux.c
> > @@ -28,6 +28,7 @@
> >  #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>
> >  #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 c95d6c5a8..67ada136b 100644
> > --- a/grub-core/loader/arm64/xen_boot.c
> > +++ b/grub-core/loader/arm64/xen_boot.c
> > @@ -30,6 +30,7 @@
> >  #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/efi/fdt.c b/grub-core/loader/efi/fdt.c
> > index 17212c38d..c0c6800f7 100644
> > --- a/grub-core/loader/efi/fdt.c
> > +++ b/grub-core/loader/efi/fdt.c
> > @@ -24,6 +24,7 @@
> >  #include <grub/file.h>
> >  #include <grub/efi/efi.h>
> >  #include <grub/efi/fdtload.h>
> > +#include <grub/efi/memory.h>
> >
> >  static void *loaded_fdt;
> >  static void *fdt;
> > diff --git a/include/grub/efi/fdtload.h b/include/grub/efi/fdtload.h
> > index 7b9ddba91..713c9424d 100644
> > --- a/include/grub/efi/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..08fe62277 100644
> > --- a/include/grub/efi/memory.h
> > +++ b/include/grub/efi/memory.h
> > @@ -22,6 +22,13 @@
> >  #include <grub/err.h>
> >  #include <grub/types.h>
> >
> > +/* The term "page" in UEFI refers only to a 4 KiB-aligned 4 KiB size region of
> > +   memory. It is not concerned with underlying translation management concepts,
> > +   but only used as the granule for memory allocations. */
> > +#define GRUB_EFI_PAGE_SHIFT             12
> > +#define GRUB_EFI_PAGE_SIZE              (1 << GRUB_EFI_PAGE_SHIFT)
> > +#define GRUB_EFI_BYTES_TO_PAGES(bytes)  (((bytes) + 0xfff) >> GRUB_EFI_PAGE_SHIFT)
> 
> If you add GRUB_EFI_PAGE_SIZE why not change GRUB_EFI_BYTES_TO_PAGES() macro to
> 
> #define GRUB_EFI_BYTES_TO_PAGES(bytes) ALIGN_UP (bytes, GRUB_EFI_PAGE_SIZE)?
> 
> Of course in separate patch.

Sure.