[PATCH] add arm64 UEFI Linux loader

[PATCH] add arm64 UEFI Linux loader

[Leif Lindholm]

This loader supports booting using the UEFI stub method only.

Currently debugging an issue with using the load_image method,
so usurping the current GRUB image context instead.

Re: [PATCH] add arm64 UEFI Linux loader

[Andrey Borzenkov]

В Mon, 16 Dec 2013 14:55:51 +0100
Leif Lindholm <leif.lindholm@linaro.org> пишет:

> This loader supports booting using the UEFI stub method only.
> 

And where is patch? :)

Re: [PATCH] add arm64 UEFI Linux loader

[Vladimir 'φ-coder/phcoder' Serbinenko]

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

On 16.12.2013 14:55, Leif Lindholm wrote:
> This loader supports booting using the UEFI stub method only.
> 
I don't see any loader attached to mail at all.
> Currently debugging an issue with using the load_image method,
> so usurping the current GRUB image context instead.
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
> .
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 291 bytes --]

Re: [PATCH] add arm64 UEFI Linux loader

[Leif Lindholm]

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

On Mon, Dec 16, 2013 at 08:13:01PM +0400, Andrey Borzenkov wrote:
> В Mon, 16 Dec 2013 14:55:51 +0100
> Leif Lindholm <leif.lindholm@linaro.org> пишет:
> 
> > This loader supports booting using the UEFI stub method only.
> 
> And where is patch? :)

Doh!
Here.

[-- Attachment #2: 0002-arm64-add-EFI-Linux-loader.patch --]
[-- Type: text/x-diff, Size: 16233 bytes --]

From 03ebc609f33df1ddcdf10668cacf69f1861a5acf Mon Sep 17 00:00:00 2001
From: Leif Lindholm <leif.lindholm@linaro.org>
Date: Wed, 4 Dec 2013 15:21:16 +0000
Subject: [PATCH 2/2] arm64: add EFI Linux loader

Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
---
 grub-core/Makefile.core.def    |    3 +-
 grub-core/loader/arm64/linux.c |  486 ++++++++++++++++++++++++++++++++++++++++
 include/grub/arm64/linux.h     |   54 +++++
 include/grub/efi/api.h         |    4 +
 4 files changed, 546 insertions(+), 1 deletion(-)
 create mode 100644 grub-core/loader/arm64/linux.c
 create mode 100644 include/grub/arm64/linux.h

diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
index 4102e73..ffa4f4b 100644
--- a/grub-core/Makefile.core.def
+++ b/grub-core/Makefile.core.def
@@ -1667,7 +1667,8 @@ module = {
   sparc64_ieee1275 = loader/sparc64/ieee1275/linux.c;
   ia64_efi = loader/ia64/efi/linux.c;
   arm = loader/arm/linux.c;
-  arm = lib/fdt.c;
+  arm64 = loader/arm64/linux.c;
+  fdt = lib/fdt.c;
   common = loader/linux.c;
   common = lib/cmdline.c;
   enable = noemu;
diff --git a/grub-core/loader/arm64/linux.c b/grub-core/loader/arm64/linux.c
new file mode 100644
index 0000000..e94a76a
--- /dev/null
+++ b/grub-core/loader/arm64/linux.c
@@ -0,0 +1,486 @@
+/*
+ *  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/command.h>
+#include <grub/err.h>
+#include <grub/file.h>
+#include <grub/fdt.h>
+#include <grub/loader.h>
+#include <grub/mm.h>
+#include <grub/types.h>
+#include <grub/cpu/linux.h>
+#include <grub/efi/efi.h>
+#include <grub/efi/pe32.h>
+#include <grub/i18n.h>
+#include <grub/lib/cmdline.h>
+
+GRUB_MOD_LICENSE ("GPLv3+");
+
+#define GRUB_EFI_PAGE_SHIFT	12
+#define BYTES_TO_PAGES(bytes)   (((bytes) + 0xfff) >> GRUB_EFI_PAGE_SHIFT)
+
+static grub_efi_guid_t fdt_guid = GRUB_EFI_DEVICE_TREE_GUID;
+
+typedef void (*kernel_stub) (grub_efi_handle_t, grub_efi_system_table_t *);
+typedef void (*kernel_image) (void *);
+
+static grub_dl_t my_mod;
+static int loaded;
+static void *kernel_mem;
+static grub_uint64_t kernel_size;
+static void *initrd_mem;
+static grub_uint32_t initrd_size;
+
+static grub_uint32_t cmdline_size;
+
+static grub_addr_t initrd_start;
+static grub_addr_t initrd_end;
+
+static void *orig_fdt;
+static void *fdt;
+
+static char *linux_args;
+
+static void
+get_fdt (void)
+{
+  grub_efi_configuration_table_t *tables;
+  unsigned int i;
+  int fdt_loaded;
+  int size;
+
+  if (!orig_fdt)
+    {
+      fdt_loaded = 0;
+      /* Look for FDT in UEFI config tables. */
+      tables = grub_efi_system_table->configuration_table;
+
+      for (i = 0; i < grub_efi_system_table->num_table_entries; i++)
+	if (grub_memcmp (&tables[i].vendor_guid, &fdt_guid, sizeof (fdt_guid))
+	    == 0)
+	  {
+	    orig_fdt = tables[i].vendor_table;
+	    grub_dprintf ("linux", "found registered FDT @ 0x%p\n", orig_fdt);
+	    break;
+	  }
+    }
+  else
+    fdt_loaded = 1;
+
+  size =
+    orig_fdt ? grub_fdt_get_totalsize (orig_fdt) : GRUB_FDT_EMPTY_TREE_SZ;
+  size += grub_strlen (linux_args) + 0x400;
+
+  grub_dprintf ("linux", "allocating %d bytes for fdt\n", size);
+  fdt = grub_efi_allocate_pages (0, BYTES_TO_PAGES (size));
+  if (!fdt)
+    return;
+
+  if (orig_fdt)
+    {
+      grub_memmove (fdt, orig_fdt, size);
+      grub_fdt_set_totalsize (fdt, size);
+      if (fdt_loaded)
+	grub_free (orig_fdt);
+    }
+  else
+    {
+      grub_fdt_create_empty_tree (fdt, size);
+    }
+}
+
+static grub_err_t
+check_kernel (struct linux_kernel_header *lh)
+{
+  if (lh->magic != GRUB_LINUX_MAGIC)
+    return GRUB_ERR_BAD_OS;
+
+  if ((lh->code0 & 0xffff) != 0x5A4D)
+    {
+      grub_error (GRUB_ERR_BAD_OS,
+		  N_
+		  ("Plain Image kernel not supported - rebuild with CONFIG_UEFI_STUB enabled.\n"));
+      return GRUB_ERR_BAD_OS;
+    }
+
+  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;
+}
+
+static grub_err_t
+finalize_params (void)
+{
+  grub_efi_boot_services_t *b;
+  grub_efi_status_t status;
+  int node, retval;
+
+  get_fdt ();
+  if (!fdt)
+    goto failure;
+
+  node = grub_fdt_find_subnode (fdt, 0, "chosen");
+  if (node < 0)
+    node = grub_fdt_add_subnode (fdt, 0, "chosen");
+
+  if (node < 1)
+    goto failure;
+
+  retval = grub_fdt_set_prop (fdt, node, "bootargs", linux_args,
+			      grub_strlen (linux_args) + 1);
+  if (retval)
+    {
+      grub_error (GRUB_ERR_BAD_OS, N_("failed to set command line\n"));
+      goto failure;
+    }
+
+  grub_dprintf ("linux", "linux command line: '%s'\n", linux_args);
+
+  /* Set initrd info */
+  if (initrd_start && initrd_end > initrd_start)
+    {
+      grub_dprintf ("linux", "Initrd @ 0x%012lx-0x%012lx\n",
+		    initrd_start, initrd_end);
+
+      retval = grub_fdt_set_prop64 (fdt, node, "linux,initrd-start",
+				    initrd_start);
+      if (retval)
+	goto failure;
+      retval = grub_fdt_set_prop64 (fdt, node, "linux,initrd-end",
+				    initrd_end);
+      if (retval)
+	goto failure;
+    }
+
+  b = grub_efi_system_table->boot_services;
+  status = b->install_configuration_table (&fdt_guid, fdt);
+  if (status != GRUB_EFI_SUCCESS)
+    goto failure;
+
+  grub_dprintf ("linux", "Installed/updated FDT configuration table @ %p\n",
+		fdt);
+
+  return GRUB_ERR_NONE;
+
+failure:
+  grub_free (fdt);
+  fdt = NULL;
+  return GRUB_ERR_BAD_OS;
+}
+
+static void *
+load_dtb (const char *filename)
+{
+  grub_file_t dtb;
+  void *tmp_fdt;
+  int size;
+
+  tmp_fdt = NULL;
+  dtb = grub_file_open (filename);
+  if (!dtb)
+    {
+      grub_error (GRUB_ERR_FILE_NOT_FOUND, N_("failed to open file"));
+      return NULL;
+    }
+
+  size = grub_file_size (dtb);
+  if (size == 0)
+    goto out;
+
+  tmp_fdt = grub_malloc (size);
+  if (!tmp_fdt)
+    goto out;
+
+  if (grub_file_read (dtb, tmp_fdt, size) != size)
+    {
+      grub_free (tmp_fdt);
+      return NULL;
+    }
+
+  if (grub_fdt_check_header (tmp_fdt, size) != 0)
+    {
+      grub_free (tmp_fdt);
+      return NULL;
+    }
+
+out:
+  grub_file_close (dtb);
+  return tmp_fdt;
+}
+
+static grub_err_t
+grub_cmd_devicetree (grub_command_t cmd __attribute__ ((unused)),
+		     int argc, char *argv[])
+{
+  void *blob;
+
+  if (!loaded)
+    {
+      grub_error (GRUB_ERR_BAD_ARGUMENT,
+		  N_("you need to load the kernel first"));
+      return GRUB_ERR_BAD_OS;
+    }
+
+  if (argc != 1)
+    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("filename expected"));
+
+  blob = load_dtb (argv[0]);
+  if (!blob)
+    return GRUB_ERR_FILE_NOT_FOUND;
+
+  orig_fdt = blob;
+
+  return GRUB_ERR_NONE;
+}
+
+static grub_err_t
+grub_linux_boot (void)
+{
+  grub_efi_loaded_image_t *image;
+  struct linux_kernel_header *lh = kernel_mem;
+  struct grub_pe32_optional_header *oh;
+  kernel_stub entry;
+  grub_err_t retval;
+
+  retval = finalize_params ();
+  if (retval != GRUB_ERR_NONE)
+    return retval;
+
+  /*
+   * Terminate command line of usurped image, to inform
+   * stub loader to get command line out of DT.
+   */
+  image = grub_efi_get_loaded_image (grub_efi_image_handle);
+  image->load_options = NULL;
+  image->load_options_size = 0;
+
+  oh = (void *) ((grub_addr_t) kernel_mem + sizeof ("PE\0") + lh->hdr_offset
+		 + sizeof (struct grub_pe32_coff_header));
+
+  entry = (void *) ((grub_addr_t) kernel_mem + oh->entry_addr);
+  grub_dprintf ("linux", "Entry point: %p\n", (void *) entry);
+  entry (grub_efi_image_handle, grub_efi_system_table);
+
+  /* When successful, not reached */
+  return GRUB_ERR_NONE;
+}
+
+static grub_err_t
+grub_linux_unload (void)
+{
+  grub_dl_unref (my_mod);
+  loaded = 0;
+  if (initrd_mem)
+    grub_efi_free_pages ((grub_efi_physical_address_t) initrd_mem,
+			 BYTES_TO_PAGES (initrd_size));
+  if (kernel_mem)
+    grub_efi_free_pages ((grub_efi_physical_address_t) kernel_mem,
+			 BYTES_TO_PAGES (kernel_size));
+  if (fdt)
+    grub_efi_free_pages ((grub_efi_physical_address_t) fdt,
+			 BYTES_TO_PAGES (grub_fdt_get_totalsize (fdt)));
+
+  return GRUB_ERR_NONE;
+}
+
+static grub_err_t
+grub_cmd_initrd (grub_command_t cmd __attribute__ ((unused)),
+		 int argc, char *argv[])
+{
+  grub_file_t *files = 0;
+  int i, nfiles = 0;
+  grub_size_t size = 0;
+  grub_uint8_t *ptr;
+
+  if (argc == 0)
+    {
+      grub_error (GRUB_ERR_BAD_ARGUMENT, N_("filename expected"));
+      goto fail;
+    }
+
+  if (!loaded)
+    {
+      grub_error (GRUB_ERR_BAD_ARGUMENT,
+		  N_("you need to load the kernel first"));
+      goto fail;
+    }
+
+  files = grub_zalloc (argc * sizeof (files[0]));
+  if (!files)
+    goto fail;
+
+  for (i = 0; i < argc; i++)
+    {
+      grub_file_filter_disable_compression ();
+      files[i] = grub_file_open (argv[i]);
+      if (!files[i])
+	goto fail;
+      nfiles++;
+      size += ALIGN_UP (grub_file_size (files[i]), 4);
+    }
+
+  initrd_mem = grub_efi_allocate_pages (0, BYTES_TO_PAGES (size));
+
+  if (!initrd_mem)
+    {
+      grub_error (GRUB_ERR_OUT_OF_MEMORY, N_("can't allocate initrd"));
+      goto fail;
+    }
+
+  initrd_start = (grub_addr_t) initrd_mem;
+  initrd_end = initrd_start + size;
+
+  ptr = initrd_mem;
+
+  for (i = 0; i < nfiles; i++)
+    {
+      grub_ssize_t cursize = grub_file_size (files[i]);
+      if (grub_file_read (files[i], ptr, cursize) != cursize)
+	{
+	  if (!grub_errno)
+	    grub_error (GRUB_ERR_FILE_READ_ERROR,
+			N_("premature end of file %s"), argv[i]);
+	  goto fail;
+	}
+      ptr += cursize;
+      grub_memset (ptr, 0, ALIGN_UP_OVERHEAD (cursize, 4));
+      ptr += ALIGN_UP_OVERHEAD (cursize, 4);
+    }
+
+fail:
+  for (i = 0; i < nfiles; i++)
+    grub_file_close (files[i]);
+  grub_free (files);
+
+  if (initrd_mem && grub_errno)
+    grub_efi_free_pages ((grub_efi_physical_address_t) initrd_mem,
+			 BYTES_TO_PAGES (size));
+
+  return grub_errno;
+}
+
+static grub_err_t
+grub_cmd_linux (grub_command_t cmd __attribute__ ((unused)),
+		int argc, char *argv[])
+{
+  grub_file_t file = 0;
+  struct linux_kernel_header lh;
+
+  grub_dl_ref (my_mod);
+
+  if (argc == 0)
+    {
+      grub_error (GRUB_ERR_BAD_ARGUMENT, N_("filename expected"));
+      goto fail;
+    }
+
+  file = grub_file_open (argv[0]);
+  if (!file)
+    goto fail;
+
+  kernel_size = grub_file_size (file);
+
+  if (grub_file_read (file, &lh, sizeof (lh)) < (long) sizeof (lh))
+    return GRUB_ERR_FILE_READ_ERROR;
+
+  if (check_kernel (&lh) != GRUB_ERR_NONE)
+    goto fail;
+
+  grub_dprintf ("linux", "kernel file size: %lld\n", (long long) kernel_size);
+  kernel_mem = grub_efi_allocate_pages (0, BYTES_TO_PAGES (kernel_size));
+  grub_dprintf ("linux", "kernel numpages: %lld\n",
+		(long long) BYTES_TO_PAGES (kernel_size));
+  if (!kernel_mem)
+    {
+      grub_error (GRUB_ERR_OUT_OF_MEMORY, N_("can't allocate kernel"));
+      goto fail;
+    }
+
+  grub_file_seek (file, 0);
+  if (grub_file_read (file, kernel_mem, kernel_size)
+      != (grub_int64_t) kernel_size)
+    {
+      grub_error (GRUB_ERR_FILE_READ_ERROR, N_("Can't read kernel %s"),
+		  argv[0]);
+      goto fail;
+    }
+
+  grub_dprintf ("linux", "kernel_mem @ %p\n", kernel_mem);
+
+  cmdline_size = grub_loader_cmdline_size (argc, argv);
+
+  linux_args = grub_malloc (cmdline_size + sizeof (LINUX_IMAGE));
+  if (!linux_args)
+    grub_error (GRUB_ERR_OUT_OF_MEMORY, "cannot allocate command line");
+  grub_memcpy (linux_args, LINUX_IMAGE, sizeof (LINUX_IMAGE));
+  grub_create_loader_cmdline (argc, argv,
+			      linux_args + sizeof (LINUX_IMAGE) - 1,
+			      cmdline_size);
+
+  if (grub_errno == GRUB_ERR_NONE)
+    {
+      grub_loader_set (grub_linux_boot, grub_linux_unload, 0);
+      loaded = 1;
+    }
+
+fail:
+
+  if (file)
+    grub_file_close (file);
+
+  if (grub_errno != GRUB_ERR_NONE)
+    {
+      grub_dl_unref (my_mod);
+      loaded = 0;
+    }
+
+  if (linux_args && !loaded)
+    grub_efi_free_pages ((grub_efi_physical_address_t) linux_args,
+			 BYTES_TO_PAGES (cmdline_size));
+
+  if (kernel_mem && !loaded)
+    grub_efi_free_pages ((grub_efi_physical_address_t) kernel_mem,
+			 BYTES_TO_PAGES (kernel_size));
+
+  return grub_errno;
+}
+
+
+static grub_command_t cmd_linux, cmd_initrd, cmd_devicetree;
+
+GRUB_MOD_INIT (linux)
+{
+  cmd_linux = grub_register_command ("linux", grub_cmd_linux, 0,
+				     N_("Load Linux."));
+  cmd_initrd = grub_register_command ("initrd", grub_cmd_initrd, 0,
+				      N_("Load initrd."));
+  cmd_devicetree =
+    grub_register_command ("devicetree", grub_cmd_devicetree, 0,
+			   N_("Load DTB file."));
+  my_mod = mod;
+}
+
+GRUB_MOD_FINI (linux)
+{
+  grub_unregister_command (cmd_linux);
+  grub_unregister_command (cmd_initrd);
+  grub_unregister_command (cmd_devicetree);
+}
diff --git a/include/grub/arm64/linux.h b/include/grub/arm64/linux.h
new file mode 100644
index 0000000..53e60c6
--- /dev/null
+++ b/include/grub/arm64/linux.h
@@ -0,0 +1,54 @@
+/*
+ *  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_LINUX_CPU_HEADER
+#define GRUB_LINUX_CPU_HEADER 1
+
+#include <grub/efi/efi.h>
+
+#define GRUB_EFI_KERNEL_STUB_ENTRY_OFFSET 0
+#define GRUB_LINUX_MAX_LOAD_ADDR 0xffffffffffffULL
+
+#define GRUB_LINUX_MAGIC 0x644d5241 /* 'ARM\x64' */
+
+/* From linux/Documentation/arm64/booting.txt */
+struct linux_kernel_header
+{
+  grub_uint32_t code0;		/* Executable code */
+  grub_uint32_t code1;		/* Executable code */
+  grub_uint64_t text_offset;    /* Image load offset */
+  grub_uint64_t res0;		/* reserved */
+  grub_uint64_t res1;		/* reserved */
+  grub_uint64_t res2;		/* reserved */
+  grub_uint64_t res3;		/* reserved */
+  grub_uint64_t res4;		/* reserved */
+  grub_uint32_t magic;		/* Magic number, little endian, "ARM\x64" */
+  grub_uint32_t hdr_offset;	/* Offset of PE/COFF header */
+};
+
+#define grub_linux_get_params() NULL
+extern grub_err_t grub_linux_init_params (void);
+extern grub_err_t grub_linux_finalize_params (void);
+extern grub_err_t grub_linux_register_kernel (struct linux_kernel_header *lh);
+extern grub_err_t grub_linux_register_cmdline (void * addr);
+extern grub_err_t grub_linux_register_initrd (void * addr, grub_size_t size);
+
+extern void grub_efi_linux_arch_register_commands (void);
+extern void grub_efi_linux_arch_unregister_commands (void);
+
+#endif /* ! GRUB_LINUX_CPU_HEADER */
diff --git a/include/grub/efi/api.h b/include/grub/efi/api.h
index 68fc90f..c72126e 100644
--- a/include/grub/efi/api.h
+++ b/include/grub/efi/api.h
@@ -276,6 +276,10 @@
       { 0x82, 0x79, 0xa8, 0x4b, 0x79, 0x61, 0x78, 0x98 } \
   }
 
+#define GRUB_EFI_DEVICE_TREE_GUID \
+  { 0xb1b621d5, 0xf19c, 0x41a5, \
+      { 0x83, 0x0b, 0xd9, 0x15, 0x2c, 0x69, 0xaa, 0xe0 } \
+  }
 struct grub_efi_sal_system_table
 {
   grub_uint32_t signature;
-- 
1.7.10.4

Re: [PATCH] add arm64 UEFI Linux loader

[Vladimir 'φ-coder/phcoder' Serbinenko]

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

On 16.12.2013 17:24, Leif Lindholm wrote:
> On Mon, Dec 16, 2013 at 08:13:01PM +0400, Andrey Borzenkov wrote:
>> > В Mon, 16 Dec 2013 14:55:51 +0100
>> > Leif Lindholm <leif.lindholm@linaro.org> пишет:
>> > 
>>> > > This loader supports booting using the UEFI stub method only.
>> > 
>> > And where is patch? :)
> Doh!
> Here.
> 
> 
> 0002-arm64-add-EFI-Linux-loader.patch
> 
> 
>>From 03ebc609f33df1ddcdf10668cacf69f1861a5acf Mon Sep 17 00:00:00 2001
> From: Leif Lindholm <leif.lindholm@linaro.org>
> Date: Wed, 4 Dec 2013 15:21:16 +0000
> Subject: [PATCH 2/2] arm64: add EFI Linux loader
> 
> Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
> ---
>  grub-core/Makefile.core.def    |    3 +-
>  grub-core/loader/arm64/linux.c |  486 ++++++++++++++++++++++++++++++++++++++++
>  include/grub/arm64/linux.h     |   54 +++++
>  include/grub/efi/api.h         |    4 +
>  4 files changed, 546 insertions(+), 1 deletion(-)
>  create mode 100644 grub-core/loader/arm64/linux.c
>  create mode 100644 include/grub/arm64/linux.h
> 
> diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
> index 4102e73..ffa4f4b 100644
> --- a/grub-core/Makefile.core.def
> +++ b/grub-core/Makefile.core.def
> @@ -1667,7 +1667,8 @@ module = {
>    sparc64_ieee1275 = loader/sparc64/ieee1275/linux.c;
>    ia64_efi = loader/ia64/efi/linux.c;
>    arm = loader/arm/linux.c;
> -  arm = lib/fdt.c;
> +  arm64 = loader/arm64/linux.c;
> +  fdt = lib/fdt.c;
>    common = loader/linux.c;
>    common = lib/cmdline.c;
>    enable = noemu;
> diff --git a/grub-core/loader/arm64/linux.c b/grub-core/loader/arm64/linux.c
> new file mode 100644
> index 0000000..e94a76a
> --- /dev/null
> +++ b/grub-core/loader/arm64/linux.c
> @@ -0,0 +1,486 @@
> +/*
> + *  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/command.h>
> +#include <grub/err.h>
> +#include <grub/file.h>
> +#include <grub/fdt.h>
> +#include <grub/loader.h>
> +#include <grub/mm.h>
> +#include <grub/types.h>
> +#include <grub/cpu/linux.h>
> +#include <grub/efi/efi.h>
> +#include <grub/efi/pe32.h>
> +#include <grub/i18n.h>
> +#include <grub/lib/cmdline.h>
> +
> +GRUB_MOD_LICENSE ("GPLv3+");
> +
> +#define GRUB_EFI_PAGE_SHIFT	12
> +#define BYTES_TO_PAGES(bytes)   (((bytes) + 0xfff) >> GRUB_EFI_PAGE_SHIFT)
> +
> +static grub_efi_guid_t fdt_guid = GRUB_EFI_DEVICE_TREE_GUID;
> +
> +typedef void (*kernel_stub) (grub_efi_handle_t, grub_efi_system_table_t *);
> +typedef void (*kernel_image) (void *);
> +
> +static grub_dl_t my_mod;
> +static int loaded;
> +static void *kernel_mem;
> +static grub_uint64_t kernel_size;
> +static void *initrd_mem;
> +static grub_uint32_t initrd_size;
> +
> +static grub_uint32_t cmdline_size;
> +
> +static grub_addr_t initrd_start;
> +static grub_addr_t initrd_end;
> +
> +static void *orig_fdt;
> +static void *fdt;
> +
> +static char *linux_args;
> +
> +static void
> +get_fdt (void)
> +{
> +  grub_efi_configuration_table_t *tables;
> +  unsigned int i;
> +  int fdt_loaded;
> +  int size;
> +
> +  if (!orig_fdt)
> +    {
> +      fdt_loaded = 0;
> +      /* Look for FDT in UEFI config tables. */
> +      tables = grub_efi_system_table->configuration_table;
> +
> +      for (i = 0; i < grub_efi_system_table->num_table_entries; i++)
> +	if (grub_memcmp (&tables[i].vendor_guid, &fdt_guid, sizeof (fdt_guid))
> +	    == 0)
> +	  {
> +	    orig_fdt = tables[i].vendor_table;
> +	    grub_dprintf ("linux", "found registered FDT @ 0x%p\n", orig_fdt);
> +	    break;
> +	  }
> +    }
> +  else
> +    fdt_loaded = 1;
> +
> +  size =
> +    orig_fdt ? grub_fdt_get_totalsize (orig_fdt) : GRUB_FDT_EMPTY_TREE_SZ;
> +  size += grub_strlen (linux_args) + 0x400;
> +
> +  grub_dprintf ("linux", "allocating %d bytes for fdt\n", size);
> +  fdt = grub_efi_allocate_pages (0, BYTES_TO_PAGES (size));
> +  if (!fdt)
> +    return;
> +
> +  if (orig_fdt)
> +    {
> +      grub_memmove (fdt, orig_fdt, size);
> +      grub_fdt_set_totalsize (fdt, size);
> +      if (fdt_loaded)
> +	grub_free (orig_fdt);
There is a problem with this: in case of failure orig_fdt isn't
available for next try anymore.
> +    }
> +  else
> +    {
> +      grub_fdt_create_empty_tree (fdt, size);
> +    }
> +}
> +
> +static grub_err_t
> +check_kernel (struct linux_kernel_header *lh)
> +{
> +  if (lh->magic != GRUB_LINUX_MAGIC)
> +    return GRUB_ERR_BAD_OS;
You need grub_error here
> +
> +  if ((lh->code0 & 0xffff) != 0x5A4D)
> +    {
Need a comment that it's MZ/exe header
> +      grub_error (GRUB_ERR_BAD_OS,
Sounds like NOT_IMPLEMENTED_YET
> +		  N_
> +		  ("Plain Image kernel not supported - rebuild with CONFIG_UEFI_STUB enabled.\n"));

> +      return GRUB_ERR_BAD_OS;
You can return grub_error (....);
> +  dtb = grub_file_open (filename);
> +  if (!dtb)
> +    {
> +      grub_error (GRUB_ERR_FILE_NOT_FOUND, N_("failed to open file"));
grub_file_open already does that.
> +  size = grub_file_size (dtb);
> +  if (size == 0)
> +    goto out;
> +
You propbably need some error
> +  tmp_fdt = grub_malloc (size);
> +  if (!tmp_fdt)
> +    goto out;
> +
> +  if (grub_file_read (dtb, tmp_fdt, size) != size)
> +    {
> +      grub_free (tmp_fdt);
Where is file closed?
> +      return NULL;
> +    }
> +
> +  if (grub_fdt_check_header (tmp_fdt, size) != 0)
> +    {
> +      grub_free (tmp_fdt);
ditto
> +      return NULL;
> +    }
> +
> +out:
> +  grub_file_close (dtb);
> +  return tmp_fdt;
> +}
> +
> +static grub_err_t
> +grub_cmd_devicetree (grub_command_t cmd __attribute__ ((unused)),
> +		     int argc, char *argv[])
> +{
> +  void *blob;
> +
> +  if (!loaded)
> +    {
> +      grub_error (GRUB_ERR_BAD_ARGUMENT,
> +		  N_("you need to load the kernel first"));
> +      return GRUB_ERR_BAD_OS;
> +    }
> +
> +  if (argc != 1)
> +    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("filename expected"));
> +
> +  blob = load_dtb (argv[0]);
> +  if (!blob)
> +    return GRUB_ERR_FILE_NOT_FOUND;
> +
> +  orig_fdt = blob;
> +
> +  return GRUB_ERR_NONE;
> +}
> +
> +static grub_err_t
> +grub_linux_boot (void)
> +{
> +  grub_efi_loaded_image_t *image;
> +  struct linux_kernel_header *lh = kernel_mem;
> +  struct grub_pe32_optional_header *oh;
pe32? Not pe64?
> +  kernel_stub entry;
> +  grub_err_t retval;
> +
> +  retval = finalize_params ();
> +  if (retval != GRUB_ERR_NONE)
> +    return retval;
> +
> +  /*
> +   * Terminate command line of usurped image, to inform
> +   * stub loader to get command line out of DT.
> +   */
> +  image = grub_efi_get_loaded_image (grub_efi_image_handle);
> +  image->load_options = NULL;
> +  image->load_options_size = 0;
> +
> +  oh = (void *) ((grub_addr_t) kernel_mem + sizeof ("PE\0") + lh->hdr_offset
> +		 + sizeof (struct grub_pe32_coff_header));
> +
> +  entry = (void *) ((grub_addr_t) kernel_mem + oh->entry_addr);
> +  grub_dprintf ("linux", "Entry point: %p\n", (void *) entry);
> +  entry (grub_efi_image_handle, grub_efi_system_table);
> +
> +  /* When successful, not reached */
> +  return GRUB_ERR_NONE;
Sounds like return grub_errno; then
> +}
> +
> +static grub_err_t
> +grub_linux_unload (void)
> +{
> +  grub_dl_unref (my_mod);
> +  loaded = 0;
> +  if (initrd_mem)
> +    grub_efi_free_pages ((grub_efi_physical_address_t) initrd_mem,
> +			 BYTES_TO_PAGES (initrd_size));
> +  if (kernel_mem)
> +    grub_efi_free_pages ((grub_efi_physical_address_t) kernel_mem,
> +			 BYTES_TO_PAGES (kernel_size));
> +  if (fdt)
> +    grub_efi_free_pages ((grub_efi_physical_address_t) fdt,
> +			 BYTES_TO_PAGES (grub_fdt_get_totalsize (fdt)));
is fdt allocate with malloc or allocate_pages?
> +  if (!loaded)
> +    {
> +      grub_error (GRUB_ERR_BAD_ARGUMENT,
> +		  N_("you need to load the kernel first"));
> +      goto fail;
> +    }
> +
> +  files = grub_zalloc (argc * sizeof (files[0]));
> +  if (!files)
> +    goto fail;
> +
> +  for (i = 0; i < argc; i++)
> +    {
> +      grub_file_filter_disable_compression ();
> +      files[i] = grub_file_open (argv[i]);
> +      if (!files[i])
> +	goto fail;
> +      nfiles++;
> +      size += ALIGN_UP (grub_file_size (files[i]), 4);
> +    }
> +
Why don't you use methods from loader/linux.c ?
> +  if (grub_file_read (file, kernel_mem, kernel_size)
> +      != (grub_int64_t) kernel_size)
> +    {
> +      grub_error (GRUB_ERR_FILE_READ_ERROR, N_("Can't read kernel %s"),
> +		  argv[0]);
Please look at similar place in other architectures.



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 291 bytes --]

Re: [PATCH] add arm64 UEFI Linux loader

[Leif Lindholm]

On Mon, Dec 16, 2013 at 10:34:51PM +0100, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
> > +static void
> > +get_fdt (void)
> > +{
> > +  grub_efi_configuration_table_t *tables;
> > +  unsigned int i;
> > +  int fdt_loaded;
> > +  int size;
> > +
> > +  if (!orig_fdt)
> > +    {
> > +      fdt_loaded = 0;
> > +      /* Look for FDT in UEFI config tables. */
> > +      tables = grub_efi_system_table->configuration_table;
> > +
> > +      for (i = 0; i < grub_efi_system_table->num_table_entries; i++)
> > +	if (grub_memcmp (&tables[i].vendor_guid, &fdt_guid, sizeof (fdt_guid))
> > +	    == 0)
> > +	  {
> > +	    orig_fdt = tables[i].vendor_table;
> > +	    grub_dprintf ("linux", "found registered FDT @ 0x%p\n", orig_fdt);
> > +	    break;
> > +	  }
> > +    }
> > +  else
> > +    fdt_loaded = 1;
> > +
> > +  size =
> > +    orig_fdt ? grub_fdt_get_totalsize (orig_fdt) : GRUB_FDT_EMPTY_TREE_SZ;
> > +  size += grub_strlen (linux_args) + 0x400;
> > +
> > +  grub_dprintf ("linux", "allocating %d bytes for fdt\n", size);
> > +  fdt = grub_efi_allocate_pages (0, BYTES_TO_PAGES (size));
> > +  if (!fdt)
> > +    return;
> > +
> > +  if (orig_fdt)
> > +    {
> > +      grub_memmove (fdt, orig_fdt, size);
> > +      grub_fdt_set_totalsize (fdt, size);
> > +      if (fdt_loaded)
> > +	grub_free (orig_fdt);
>
> There is a problem with this: in case of failure orig_fdt isn't
> available for next try anymore.

Right. I need to also NULL orig_fdt, will do.

> > +static grub_err_t
> > +check_kernel (struct linux_kernel_header *lh)
> > +{
> > +  if (lh->magic != GRUB_LINUX_MAGIC)
> > +    return GRUB_ERR_BAD_OS;
>
> You need grub_error here

Yes.

> > +  if ((lh->code0 & 0xffff) != 0x5A4D)
> > +    {
>
> Need a comment that it's MZ/exe header

Yes.

> > +      grub_error (GRUB_ERR_BAD_OS,
>
> Sounds like NOT_IMPLEMENTED_YET

Yes.

> > +		  N_
> > +		  ("Plain Image kernel not supported - rebuild with CONFIG_UEFI_STUB enabled.\n"));
> 
> > +      return GRUB_ERR_BAD_OS;
> You can return grub_error (....);

Yes.

> > +  dtb = grub_file_open (filename);
> > +  if (!dtb)
> > +    {
> > +      grub_error (GRUB_ERR_FILE_NOT_FOUND, N_("failed to open file"));
>
> grub_file_open already does that.

OK.

> > +  size = grub_file_size (dtb);
> > +  if (size == 0)
> > +    goto out;
> > +
>
> You propbably need some error

Actually, I'll delete that test - it's a bit arbitrary anyway.

> > +  tmp_fdt = grub_malloc (size);
> > +  if (!tmp_fdt)
> > +    goto out;
> > +
> > +  if (grub_file_read (dtb, tmp_fdt, size) != size)
> > +    {
> > +      grub_free (tmp_fdt);
>
> Where is file closed?

Added.

> > +      return NULL;
> > +    }
> > +
> > +  if (grub_fdt_check_header (tmp_fdt, size) != 0)
> > +    {
> > +      grub_free (tmp_fdt);
> ditto

Ditto.

> > +static grub_err_t
> > +grub_linux_boot (void)
> > +{
> > +  grub_efi_loaded_image_t *image;
> > +  struct linux_kernel_header *lh = kernel_mem;
> > +  struct grub_pe32_optional_header *oh;
> pe32? Not pe64?

Mmm, that's a bit unpretty.
The fields that matter for the loader do not differ, but clearly
should be pe64. Changed.

> > +  kernel_stub entry;
> > +  grub_err_t retval;
> > +
> > +  retval = finalize_params ();
> > +  if (retval != GRUB_ERR_NONE)
> > +    return retval;
> > +
> > +  /*
> > +   * Terminate command line of usurped image, to inform
> > +   * stub loader to get command line out of DT.
> > +   */
> > +  image = grub_efi_get_loaded_image (grub_efi_image_handle);
> > +  image->load_options = NULL;
> > +  image->load_options_size = 0;
> > +
> > +  oh = (void *) ((grub_addr_t) kernel_mem + sizeof ("PE\0") + lh->hdr_offset
> > +		 + sizeof (struct grub_pe32_coff_header));
> > +
> > +  entry = (void *) ((grub_addr_t) kernel_mem + oh->entry_addr);
> > +  grub_dprintf ("linux", "Entry point: %p\n", (void *) entry);
> > +  entry (grub_efi_image_handle, grub_efi_system_table);
> > +
> > +  /* When successful, not reached */
> > +  return GRUB_ERR_NONE;
> Sounds like return grub_errno; then

OK.

> > +}
> > +
> > +static grub_err_t
> > +grub_linux_unload (void)
> > +{
> > +  grub_dl_unref (my_mod);
> > +  loaded = 0;
> > +  if (initrd_mem)
> > +    grub_efi_free_pages ((grub_efi_physical_address_t) initrd_mem,
> > +			 BYTES_TO_PAGES (initrd_size));
> > +  if (kernel_mem)
> > +    grub_efi_free_pages ((grub_efi_physical_address_t) kernel_mem,
> > +			 BYTES_TO_PAGES (kernel_size));
> > +  if (fdt)
> > +    grub_efi_free_pages ((grub_efi_physical_address_t) fdt,
> > +			 BYTES_TO_PAGES (grub_fdt_get_totalsize (fdt)));
> is fdt allocate with malloc or allocate_pages?

grub_efi_allocate_pages().
Fixed in finalize_params(), thanks.

> > +  if (!loaded)
> > +    {
> > +      grub_error (GRUB_ERR_BAD_ARGUMENT,
> > +		  N_("you need to load the kernel first"));
> > +      goto fail;
> > +    }
> > +
> > +  files = grub_zalloc (argc * sizeof (files[0]));
> > +  if (!files)
> > +    goto fail;
> > +
> > +  for (i = 0; i < argc; i++)
> > +    {
> > +      grub_file_filter_disable_compression ();
> > +      files[i] = grub_file_open (argv[i]);
> > +      if (!files[i])
> > +	goto fail;
> > +      nfiles++;
> > +      size += ALIGN_UP (grub_file_size (files[i]), 4);
> > +    }
> > +
> Why don't you use methods from loader/linux.c ?

Umm, this entire function is quite embarassing - it is left around from
when I included Matthew Garrett's "linuxefi" code for understanding the
process better..
Updated set contains the much simpler one which I meant to include.
ARM* do not even support multiple initrds.

> > +  if (grub_file_read (file, kernel_mem, kernel_size)
> > +      != (grub_int64_t) kernel_size)
> > +    {
> > +      grub_error (GRUB_ERR_FILE_READ_ERROR, N_("Can't read kernel %s"),
> > +		  argv[0]);
> Please look at similar place in other architectures.

i386 looks near-identical, apart from the fact that their bzImage has
a size field which the ARM64 image does not. If you want me to change
something here, I'm afraid you will have to rub my nose in it.

/
    Leif

Re: [PATCH] add arm64 UEFI Linux loader

[Andrey Borzenkov]

В Wed, 18 Dec 2013 17:54:39 +0100
Leif Lindholm <leif.lindholm@linaro.org> пишет:

> 
> > > +  if (!loaded)
> > > +    {
> > > +      grub_error (GRUB_ERR_BAD_ARGUMENT,
> > > +		  N_("you need to load the kernel first"));
> > > +      goto fail;
> > > +    }
> > > +
> > > +  files = grub_zalloc (argc * sizeof (files[0]));
> > > +  if (!files)
> > > +    goto fail;
> > > +
> > > +  for (i = 0; i < argc; i++)
> > > +    {
> > > +      grub_file_filter_disable_compression ();
> > > +      files[i] = grub_file_open (argv[i]);
> > > +      if (!files[i])
> > > +	goto fail;
> > > +      nfiles++;
> > > +      size += ALIGN_UP (grub_file_size (files[i]), 4);
> > > +    }
> > > +
> > Why don't you use methods from loader/linux.c ?
> 
[...]
> ARM* do not even support multiple initrds.
> 

Is this arch dependant? I mean, kernel gets buffer that holds initramfs
and just tries to cpio extract it; if there are multiple concatenated
archives it will simply process them all. I thought this was pretty
much arch-independent.

Re: [PATCH] add arm64 UEFI Linux loader

[Vladimir 'φ-coder/phcoder' Serbinenko]

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

On 18.12.2013 17:54, Leif Lindholm wrote:
> On Mon, Dec 16, 2013 at 10:34:51PM +0100, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
>>> +static void
>>> +get_fdt (void)
>>> +{
>>> +  grub_efi_configuration_table_t *tables;
>>> +  unsigned int i;
>>> +  int fdt_loaded;
>>> +  int size;
>>> +
>>> +  if (!orig_fdt)
>>> +    {
>>> +      fdt_loaded = 0;
>>> +      /* Look for FDT in UEFI config tables. */
>>> +      tables = grub_efi_system_table->configuration_table;
>>> +
>>> +      for (i = 0; i < grub_efi_system_table->num_table_entries; i++)
>>> +	if (grub_memcmp (&tables[i].vendor_guid, &fdt_guid, sizeof (fdt_guid))
>>> +	    == 0)
>>> +	  {
>>> +	    orig_fdt = tables[i].vendor_table;
>>> +	    grub_dprintf ("linux", "found registered FDT @ 0x%p\n", orig_fdt);
>>> +	    break;
>>> +	  }
>>> +    }
>>> +  else
>>> +    fdt_loaded = 1;
>>> +
>>> +  size =
>>> +    orig_fdt ? grub_fdt_get_totalsize (orig_fdt) : GRUB_FDT_EMPTY_TREE_SZ;
>>> +  size += grub_strlen (linux_args) + 0x400;
>>> +
>>> +  grub_dprintf ("linux", "allocating %d bytes for fdt\n", size);
>>> +  fdt = grub_efi_allocate_pages (0, BYTES_TO_PAGES (size));
>>> +  if (!fdt)
>>> +    return;
>>> +
>>> +  if (orig_fdt)
>>> +    {
>>> +      grub_memmove (fdt, orig_fdt, size);
>>> +      grub_fdt_set_totalsize (fdt, size);
>>> +      if (fdt_loaded)
>>> +	grub_free (orig_fdt);
>>
>> There is a problem with this: in case of failure orig_fdt isn't
>> available for next try anymore.
> 
> Right. I need to also NULL orig_fdt, will do.
> 
I think that you have to keep orig_fdt as otherwise only first attempt
to boot will use FDT supplied by system. Second one won't, likely
resulting in another failure
>>> +  if (!loaded)
>>> +    {
>>> +      grub_error (GRUB_ERR_BAD_ARGUMENT,
>>> +		  N_("you need to load the kernel first"));
>>> +      goto fail;
>>> +    }
>>> +
>>> +  files = grub_zalloc (argc * sizeof (files[0]));
>>> +  if (!files)
>>> +    goto fail;
>>> +
>>> +  for (i = 0; i < argc; i++)
>>> +    {
>>> +      grub_file_filter_disable_compression ();
>>> +      files[i] = grub_file_open (argv[i]);
>>> +      if (!files[i])
>>> +	goto fail;
>>> +      nfiles++;
>>> +      size += ALIGN_UP (grub_file_size (files[i]), 4);
>>> +    }
>>> +
>> Why don't you use methods from loader/linux.c ?
> 
> Umm, this entire function is quite embarassing - it is left around from
> when I included Matthew Garrett's "linuxefi" code for understanding the
> process better..
> Updated set contains the much simpler one which I meant to include.
> ARM* do not even support multiple initrds.
They're concatenated in memory if you use common functions and results
in valid cpio which will be decompressed/parsed by Linux.
> 

>>> +  if (grub_file_read (file, kernel_mem, kernel_size)
>>> +      != (grub_int64_t) kernel_size)
>>> +    {
>>> +      grub_error (GRUB_ERR_FILE_READ_ERROR, N_("Can't read kernel %s"),
>>> +		  argv[0]);
>> Please look at similar place in other architectures.
> 
> i386 looks near-identical, apart from the fact that their bzImage has
> a size field which the ARM64 image does not. If you want me to change
> something here, I'm afraid you will have to rub my nose in it.
> 
if grub_errno is set you shouldn't override it. And please use the same
message verbatim (unexpected end of file): it decreases work for translators
> /
>     Leif
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 291 bytes --]

Re: [PATCH] add arm64 UEFI Linux loader

[Leif Lindholm]

On Wed, Dec 18, 2013 at 06:23:06PM +0100, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
> >>> +  if (!orig_fdt)
> >>> +    {
> >>> +      fdt_loaded = 0;
> >>> +      /* Look for FDT in UEFI config tables. */
> >>> +      tables = grub_efi_system_table->configuration_table;
> >>> +
> >>> +      for (i = 0; i < grub_efi_system_table->num_table_entries; i++)
> >>> +	if (grub_memcmp (&tables[i].vendor_guid, &fdt_guid, sizeof (fdt_guid))
> >>> +	    == 0)
> >>> +	  {
> >>> +	    orig_fdt = tables[i].vendor_table;
> >>> +	    grub_dprintf ("linux", "found registered FDT @ 0x%p\n", orig_fdt);
> >>> +	    break;
> >>> +	  }
> >>> +    }
> >>> +  else
> >>> +    fdt_loaded = 1;
> >>> +
> >>> +  size =
> >>> +    orig_fdt ? grub_fdt_get_totalsize (orig_fdt) : GRUB_FDT_EMPTY_TREE_SZ;
> >>> +  size += grub_strlen (linux_args) + 0x400;
> >>> +
> >>> +  grub_dprintf ("linux", "allocating %d bytes for fdt\n", size);
> >>> +  fdt = grub_efi_allocate_pages (0, BYTES_TO_PAGES (size));
> >>> +  if (!fdt)
> >>> +    return;
> >>> +
> >>> +  if (orig_fdt)
> >>> +    {
> >>> +      grub_memmove (fdt, orig_fdt, size);
> >>> +      grub_fdt_set_totalsize (fdt, size);
> >>> +      if (fdt_loaded)
> >>> +	grub_free (orig_fdt);
> >>
> >> There is a problem with this: in case of failure orig_fdt isn't
> >> available for next try anymore.
> > 
> > Right. I need to also NULL orig_fdt, will do.
> > 
> I think that you have to keep orig_fdt as otherwise only first attempt
> to boot will use FDT supplied by system. Second one won't, likely
> resulting in another failure

No, I null/free FDT loaded with "devicetree" command.
Anyway, I have refactored this handling a bit in my updated set,
also improving readability.

> >>> +  if (!loaded)
> >>> +    {
> >>> +      grub_error (GRUB_ERR_BAD_ARGUMENT,
> >>> +		  N_("you need to load the kernel first"));
> >>> +      goto fail;
> >>> +    }
> >>> +
> >>> +  files = grub_zalloc (argc * sizeof (files[0]));
> >>> +  if (!files)
> >>> +    goto fail;
> >>> +
> >>> +  for (i = 0; i < argc; i++)
> >>> +    {
> >>> +      grub_file_filter_disable_compression ();
> >>> +      files[i] = grub_file_open (argv[i]);
> >>> +      if (!files[i])
> >>> +	goto fail;
> >>> +      nfiles++;
> >>> +      size += ALIGN_UP (grub_file_size (files[i]), 4);
> >>> +    }
> >>> +
> >> Why don't you use methods from loader/linux.c ?
> > 
> > Umm, this entire function is quite embarassing - it is left around from
> > when I included Matthew Garrett's "linuxefi" code for understanding the
> > process better..
> > Updated set contains the much simpler one which I meant to include.
> > ARM* do not even support multiple initrds.
>
> They're concatenated in memory if you use common functions and results
> in valid cpio which will be decompressed/parsed by Linux.

Done. Cleaning up patch for submitting v2.

> >>> +  if (grub_file_read (file, kernel_mem, kernel_size)
> >>> +      != (grub_int64_t) kernel_size)
> >>> +    {
> >>> +      grub_error (GRUB_ERR_FILE_READ_ERROR, N_("Can't read kernel %s"),
> >>> +		  argv[0]);
> >> Please look at similar place in other architectures.
> > 
> > i386 looks near-identical, apart from the fact that their bzImage has
> > a size field which the ARM64 image does not. If you want me to change
> > something here, I'm afraid you will have to rub my nose in it.
> > 
> if grub_errno is set you shouldn't override it. And please use the same
> message verbatim (unexpected end of file): it decreases work for translators

Well, since I have already checked the file size before this, any
failure will be an i/o error - so I'll just goto fail instead.

/
    Leif

Re: [PATCH] add arm64 UEFI Linux loader

[Vladimir 'φ-coder/phcoder' Serbinenko]

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

>> if grub_errno is set you shouldn't override it. And please use the same
>> message verbatim (unexpected end of file): it decreases work for translators
> 
> Well, since I have already checked the file size before this, any
> failure will be an i/o error - so I'll just goto fail instead.
> 
If the file is read from network it may change size between when you
check it and when you read it.
> /
>     Leif
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 291 bytes --]

Re: [PATCH] add arm64 UEFI Linux loader

[Leif Lindholm]

On Thu, Dec 19, 2013 at 08:06:03PM +0100, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
> >> if grub_errno is set you shouldn't override it. And please use the same
> >> message verbatim (unexpected end of file): it decreases work for translators
> > 
> > Well, since I have already checked the file size before this, any
> > failure will be an i/o error - so I'll just goto fail instead.
> > 
> If the file is read from network it may change size between when you
> check it and when you read it.

Ah, ok.
Fixed and resubmitting now.

/
    Leif