[PATCH 6/7] add ARM Linux loader

[PATCH 6/7] add ARM Linux loader

[Leif Lindholm]

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



[-- Attachment #2: 0006-linux-loader.patch --]
[-- Type: application/octet-stream, Size: 12985 bytes --]

=== modified file 'grub-core/Makefile.core.def'
--- grub-core/Makefile.core.def	2013-03-24 13:26:27 +0000
+++ grub-core/Makefile.core.def	2013-03-24 13:49:04 +0000
@@ -1472,9 +1472,12 @@
   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;
   common = loader/linux.c;
   common = lib/cmdline.c;
   enable = noemu;
+
+  fdt_cppflags = '$(CPPFLAGS_LIBFDT)';
 };
 
 module = {

=== added directory 'grub-core/loader/arm'
=== added file 'grub-core/loader/arm/linux.c'
--- grub-core/loader/arm/linux.c	1970-01-01 00:00:00 +0000
+++ grub-core/loader/arm/linux.c	2013-03-24 13:49:04 +0000
@@ -0,0 +1,405 @@
+/* linux.c - boot Linux */
+/*
+ *  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/dl.h>
+#include <grub/file.h>
+#include <grub/loader.h>
+#include <grub/mm.h>
+#include <grub/misc.h>
+#include <grub/command.h>
+#include <grub/cache.h>
+#include <grub/cpu/linux.h>
+#include <grub/lib/cmdline.h>
+
+#include <libfdt.h>
+
+GRUB_MOD_LICENSE ("GPLv3+");
+
+static grub_dl_t my_mod;
+
+static grub_addr_t initrd_start;
+static grub_size_t initrd_end;
+
+static grub_addr_t linux_addr;
+static grub_size_t linux_size;
+
+static char *linux_args;
+
+static grub_addr_t firmware_boot_data;
+static grub_addr_t boot_data;
+static grub_uint32_t machine_type;
+
+/*
+ * linux_prepare_fdt():
+ *   Prepares a loaded FDT for being passed to Linux.
+ *   Merges in command line parameters and sets up initrd addresses.
+ */
+static grub_err_t
+linux_prepare_fdt (void)
+{
+  int node;
+  int retval;
+  int tmp_size;
+  void *tmp_fdt;
+
+  tmp_size = fdt_totalsize ((void *) boot_data) + FDT_ADDITIONAL_ENTRIES_SIZE;
+  tmp_fdt = grub_malloc (tmp_size);
+  if (!tmp_fdt)
+    return GRUB_ERR_OUT_OF_MEMORY;
+
+  fdt_open_into ((void *) boot_data, tmp_fdt, tmp_size);
+
+  /* Find or create '/chosen' node */
+  node = fdt_subnode_offset (tmp_fdt, 0, "chosen");
+  if (node < 0)
+    {
+      grub_printf ("No 'chosen' node in FDT - creating.\n");
+      node = fdt_add_subnode (tmp_fdt, 0, "chosen");
+      if (node < 0)
+	goto failure;
+    }
+
+  grub_printf ("linux_args: '%s'\n", linux_args);
+
+  /* Generate and set command line */
+  retval = fdt_setprop (tmp_fdt, node, "bootargs", linux_args,
+			grub_strlen (linux_args) + 1);
+  if (retval)
+    goto failure;
+
+  if (initrd_start && initrd_end)
+    {
+      /*
+       * We're using physical addresses, so even if we have LPAE, we're
+       * restricted to a 32-bit address space.
+       */
+      grub_uint32_t fdt_initrd_start = cpu_to_fdt32 (initrd_start);
+      grub_uint32_t fdt_initrd_end = cpu_to_fdt32 (initrd_end);
+
+      grub_dprintf ("loader", "Initrd @ 0x%08x-0x%08x\n",
+		    initrd_start, initrd_end);
+
+      retval = fdt_setprop (tmp_fdt, node, "linux,initrd-start",
+			    &fdt_initrd_start, sizeof (fdt_initrd_start));
+      if (retval)
+	goto failure;
+      retval = fdt_setprop (tmp_fdt, node, "linux,initrd-end",
+			    &fdt_initrd_end, sizeof (fdt_initrd_end));
+      if (retval)
+	goto failure;
+    }
+
+  /* Copy updated FDT to its launch location */
+  fdt_move (tmp_fdt, (void *) boot_data, fdt_totalsize (tmp_fdt));
+  grub_free (tmp_fdt);
+  fdt_pack ((void *) boot_data);
+
+  grub_dprintf ("loader", "FDT updated for Linux boot\n");
+
+  return GRUB_ERR_NONE;
+
+failure:
+  grub_free (tmp_fdt);
+  return GRUB_ERR_BAD_ARGUMENT;
+}
+
+static grub_err_t
+linux_boot (void)
+{
+  kernel_entry_t linuxmain;
+  grub_err_t err = GRUB_ERR_NONE;
+
+  grub_arch_sync_caches ((void *) linux_addr, linux_size);
+
+  grub_dprintf ("loader", "Kernel at: 0x%x\n", linux_addr);
+
+  if (!boot_data)
+    {
+      if (firmware_boot_data)
+	{
+	  grub_printf ("Using firmware-supplied boot data @ 0x%08x\n",
+		       firmware_boot_data);
+	  boot_data = firmware_boot_data;
+	}
+      else
+	{
+	  return GRUB_ERR_FILE_NOT_FOUND;
+	}
+    }
+
+  grub_dprintf ("loader", "Boot data at: 0x%x\n", boot_data);
+
+  if (fdt32_to_cpu (*(grub_uint32_t *) (boot_data)) == FDT_MAGIC)
+    {
+      grub_dprintf ("loader", "FDT @ 0x%08x\n", (grub_addr_t) boot_data);
+      if (linux_prepare_fdt () != GRUB_ERR_NONE)
+	{
+	  grub_dprintf ("loader", "linux_prepare_fdt() failed\n");
+	  return GRUB_ERR_FILE_NOT_FOUND;
+	}
+    }
+
+  grub_dprintf ("loader", "Jumping to Linux...\n");
+
+  /* Boot the kernel.
+   *   Arguments to kernel:
+   *     r0 - 0
+   *     r1 - machine type (possibly passed from firmware)
+   *     r2 - address of DTB or ATAG list
+   */
+  linuxmain = (kernel_entry_t) linux_addr;
+
+#ifdef GRUB_MACHINE_EFI
+  err = grub_efi_prepare_platform();
+  if (err != GRUB_ERR_NONE)
+    return err;
+#endif
+
+  linuxmain (0, machine_type, (void *) boot_data);
+
+  return err;
+}
+
+/*
+ * Only support zImage, so no relocations necessary
+ */
+static grub_err_t
+linux_load (const char *filename)
+{
+  grub_file_t file;
+  int size;
+
+  file = grub_file_open (filename);
+  if (!file)
+    return GRUB_ERR_FILE_NOT_FOUND;
+
+  size = grub_file_size (file);
+  if (size == 0)
+    return GRUB_ERR_FILE_READ_ERROR;
+
+#ifdef GRUB_MACHINE_EFI
+  linux_addr = (grub_addr_t) grub_efi_allocate_loader_memory (LINUX_PHYS_OFFSET, size);
+#else
+  linux_addr = LINUX_ADDRESS;
+#endif
+  grub_dprintf ("loader", "Loading Linux to 0x%08x\n",
+		(grub_addr_t) linux_addr);
+
+  if (grub_file_read (file, (void *) linux_addr, size) != size)
+    {
+      grub_printf ("Kernel read failed!\n");
+      return GRUB_ERR_FILE_READ_ERROR;
+    }
+
+  if (*(grub_uint32_t *) (linux_addr + LINUX_ZIMAGE_OFFSET)
+      != LINUX_ZIMAGE_MAGIC)
+    {
+      return grub_error (GRUB_ERR_BAD_FILE_TYPE, N_("invalid zImage"));
+    }
+
+  linux_size = size;
+
+  return GRUB_ERR_NONE;
+}
+
+static grub_err_t
+linux_unload (void)
+{
+  grub_dl_unref (my_mod);
+
+  grub_free (linux_args);
+  linux_args = NULL;
+
+  initrd_start = initrd_end = 0;
+
+  return GRUB_ERR_NONE;
+}
+
+static grub_err_t
+grub_cmd_linux (grub_command_t cmd __attribute__ ((unused)),
+		int argc, char *argv[])
+{
+  int size, retval;
+  grub_file_t file;
+  grub_dl_ref (my_mod);
+
+  if (argc == 0)
+    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("filename expected"));
+
+  file = grub_file_open (argv[0]);
+  if (!file)
+    goto fail;
+
+  retval = linux_load (argv[0]);
+  grub_file_close (file);
+  if (retval != GRUB_ERR_NONE)
+    goto fail;
+
+  grub_loader_set (linux_boot, linux_unload, 1);
+
+  size = grub_loader_cmdline_size (argc, argv);
+  linux_args = grub_malloc (size + sizeof (LINUX_IMAGE));
+  if (!linux_args)
+    goto fail;
+
+  /* Create kernel command line.  */
+  grub_memcpy (linux_args, LINUX_IMAGE, sizeof (LINUX_IMAGE));
+  grub_create_loader_cmdline (argc, argv,
+			      linux_args + sizeof (LINUX_IMAGE) - 1, size);
+
+  return GRUB_ERR_NONE;
+
+fail:
+  grub_dl_unref (my_mod);
+  return grub_errno;
+}
+
+static grub_err_t
+grub_cmd_initrd (grub_command_t cmd __attribute__ ((unused)),
+		 int argc, char *argv[])
+{
+  grub_file_t file;
+  int size;
+
+  if (argc == 0)
+    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("filename expected"));
+
+  file = grub_file_open (argv[0]);
+  if (!file)
+    return grub_errno;
+
+  size = grub_file_size (file);
+  if (size == 0)
+    goto fail;
+
+#ifdef GRUB_MACHINE_EFI
+  initrd_start = (grub_addr_t) grub_efi_allocate_loader_memory (LINUX_INITRD_PHYS_OFFSET, size);
+#else
+  initrd_start = LINUX_INITRD_ADDRESS;
+#endif
+  grub_dprintf ("loader", "Loading initrd to 0x%08x\n",
+		(grub_addr_t) initrd_start);
+
+  if (grub_file_read (file, (void *) initrd_start, size) != size)
+    goto fail;
+
+  initrd_end = initrd_start + size;
+
+  return GRUB_ERR_NONE;
+
+fail:
+  grub_file_close (file);
+
+  return grub_errno;
+}
+
+static void *
+load_dtb (grub_file_t dtb, int size)
+{
+  void *fdt;
+
+  fdt = grub_malloc (size);
+  if (!fdt)
+    return NULL;
+
+  if (grub_file_read (dtb, fdt, size) != size)
+    {
+      grub_free (fdt);
+      return NULL;
+    }
+
+  if (fdt_check_header (fdt) != 0)
+    {
+      grub_free (fdt);
+      return NULL;
+    }
+
+  return fdt;
+}
+
+static grub_err_t
+grub_cmd_devicetree (grub_command_t cmd __attribute__ ((unused)),
+		     int argc, char *argv[])
+{
+  grub_file_t dtb;
+  void *blob;
+  int size;
+
+  if (argc != 1)
+    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("filename expected"));
+
+  dtb = grub_file_open (argv[0]);
+  if (!dtb)
+    return grub_error (GRUB_ERR_FILE_NOT_FOUND, N_("failed to open file"));
+
+  size = grub_file_size (dtb);
+  if (size == 0)
+    goto out;
+
+  blob = load_dtb (dtb, size);
+  if (!blob)
+    return GRUB_ERR_FILE_NOT_FOUND;
+
+#ifdef GRUB_MACHINE_EFI
+  boot_data = (grub_addr_t) grub_efi_allocate_loader_memory (LINUX_FDT_PHYS_OFFSET, size);
+#else
+  boot_data = LINUX_FDT_ADDRESS;
+#endif
+  grub_dprintf ("loader", "Loading device tree to 0x%08x\n",
+		(grub_addr_t) boot_data);
+  fdt_move (blob, (void *) boot_data, fdt_totalsize (blob));
+  grub_free (blob);
+
+  /* 
+   * We've successfully loaded an FDT, so any machine type passed
+   * from firmware is now obsolete.
+   */
+  machine_type = ARM_FDT_MACHINE_TYPE;
+
+  return GRUB_ERR_NONE;
+
+ out:
+  grub_file_close (dtb);
+
+  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;
+  firmware_boot_data = firmware_get_boot_data ();
+
+  boot_data = (grub_addr_t) NULL;
+  machine_type = firmware_get_machine_type ();
+}
+
+GRUB_MOD_FINI (linux)
+{
+  grub_unregister_command (cmd_linux);
+  grub_unregister_command (cmd_initrd);
+  grub_unregister_command (cmd_devicetree);
+}

=== added file 'include/grub/arm/linux.h'
--- include/grub/arm/linux.h	1970-01-01 00:00:00 +0000
+++ include/grub/arm/linux.h	2013-03-24 13:49:04 +0000
@@ -0,0 +1,59 @@
+/* linux.h - ARM linux specific definitions */
+/*
+ *  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
+
+#define LINUX_ZIMAGE_OFFSET 0x24
+#define LINUX_ZIMAGE_MAGIC  0x016f2818
+
+#define ARM_FDT_MACHINE_TYPE 0xFFFFFFFF
+
+#if defined GRUB_MACHINE_UBOOT
+# 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 firmware_get_boot_data uboot_get_boot_data
+# define firmware_get_machine_type 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)
+static inline grub_addr_t
+firmware_get_boot_data (void)
+{
+  return 0;
+}
+static inline grub_uint32_t
+firmware_get_machine_type (void)
+{
+  return ARM_FDT_MACHINE_TYPE;
+}
+#endif
+
+#define FDT_ADDITIONAL_ENTRIES_SIZE	0x300
+
+typedef void (*kernel_entry_t) (int, unsigned long, void *);
+
+#endif /* ! GRUB_LINUX_CPU_HEADER */

Re: [PATCH 6/7] add ARM Linux loader

[Francesco Lavra]

On 03/24/2013 06:01 PM, Leif Lindholm wrote:
> === added directory 'grub-core/loader/arm'
> === added file 'grub-core/loader/arm/linux.c'
> --- grub-core/loader/arm/linux.c	1970-01-01 00:00:00 +0000
> +++ grub-core/loader/arm/linux.c	2013-03-24 13:49:04 +0000
> @@ -0,0 +1,405 @@
> +/* linux.c - boot Linux */
> +/*
> + *  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/dl.h>
> +#include <grub/file.h>
> +#include <grub/loader.h>
> +#include <grub/mm.h>
> +#include <grub/misc.h>
> +#include <grub/command.h>
> +#include <grub/cache.h>
> +#include <grub/cpu/linux.h>
> +#include <grub/lib/cmdline.h>
> +
> +#include <libfdt.h>
> +
> +GRUB_MOD_LICENSE ("GPLv3+");
> +
> +static grub_dl_t my_mod;
> +
> +static grub_addr_t initrd_start;
> +static grub_size_t initrd_end;

initrd_end should be the same type as initrd_start.

> +
> +static grub_addr_t linux_addr;
> +static grub_size_t linux_size;
> +
> +static char *linux_args;
> +
> +static grub_addr_t firmware_boot_data;
> +static grub_addr_t boot_data;
> +static grub_uint32_t machine_type;
> +
> +/*
> + * linux_prepare_fdt():
> + *   Prepares a loaded FDT for being passed to Linux.
> + *   Merges in command line parameters and sets up initrd addresses.
> + */
> +static grub_err_t
> +linux_prepare_fdt (void)
> +{
> +  int node;
> +  int retval;
> +  int tmp_size;
> +  void *tmp_fdt;
> +
> +  tmp_size = fdt_totalsize ((void *) boot_data) + FDT_ADDITIONAL_ENTRIES_SIZE;

Having a fixed size limit (FDT_ADDITIONAL_ENTRIES_SIZE) to insert
variable-sized data (such as the linux command line) seems suboptimal,
as this may fail when a very long command line is passed. How about
removing the FDT_ADDITIONAL_ENTRIES_SIZE define from the header file
include/grub/arm/linux.h (after all, this is an implementation detail
and IMHO shouldn't go in a public header file) and defining it in this
function, say:

#define FDT_ADDITIONAL_ENTRIES_SIZE	(0x100 +	\
	grub_strlen (linux_args))

[...]
> +/*
> + * Only support zImage, so no relocations necessary
> + */
> +static grub_err_t
> +linux_load (const char *filename)
> +{
> +  grub_file_t file;
> +  int size;
> +
> +  file = grub_file_open (filename);
> +  if (!file)
> +    return GRUB_ERR_FILE_NOT_FOUND;
> +
> +  size = grub_file_size (file);
> +  if (size == 0)
> +    return GRUB_ERR_FILE_READ_ERROR;
> +
> +#ifdef GRUB_MACHINE_EFI
> +  linux_addr = (grub_addr_t) grub_efi_allocate_loader_memory (LINUX_PHYS_OFFSET, size);

There should be a check against allocation failure.

> +#else
> +  linux_addr = LINUX_ADDRESS;
> +#endif
> +  grub_dprintf ("loader", "Loading Linux to 0x%08x\n",
> +		(grub_addr_t) linux_addr);
> +
> +  if (grub_file_read (file, (void *) linux_addr, size) != size)
> +    {
> +      grub_printf ("Kernel read failed!\n");
> +      return GRUB_ERR_FILE_READ_ERROR;

In the EFI case, the allocated memory should be freed before returning.

> +    }
> +
> +  if (*(grub_uint32_t *) (linux_addr + LINUX_ZIMAGE_OFFSET)
> +      != LINUX_ZIMAGE_MAGIC)
> +    {
> +      return grub_error (GRUB_ERR_BAD_FILE_TYPE, N_("invalid zImage"));

As above, in the EFI case the allocated memory should be freed before
returning.

> +    }
> +
> +  linux_size = size;
> +
> +  return GRUB_ERR_NONE;
> +}
> +static grub_err_t
> +linux_unload (void)
> +{
> +  grub_dl_unref (my_mod);
> +
> +  grub_free (linux_args);
> +  linux_args = NULL;

linux_args might already be NULL, if memory allocation for it failed in
grub_cmd_linux().
In the EFI case, the memory allocated for the kernel image should be
freed as well.

> +
> +  initrd_start = initrd_end = 0;

Why should the initrd data be discarded here?

> +
> +  return GRUB_ERR_NONE;
> +}
> +static grub_err_t
> +grub_cmd_linux (grub_command_t cmd __attribute__ ((unused)),
> +		int argc, char *argv[])
> +{
> +  int size, retval;

The return type of linux_load() is grub_err_t, so retval should be the
same type.

> +  grub_file_t file;
> +  grub_dl_ref (my_mod);
> +
> +  if (argc == 0)
> +    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("filename expected"));
> +
> +  file = grub_file_open (argv[0]);
> +  if (!file)
> +    goto fail;
> +
> +  retval = linux_load (argv[0]);

Here the file is already open: why not just pass the file handle to
linux_load()?

> +  grub_file_close (file);
> +  if (retval != GRUB_ERR_NONE)
> +    goto fail;

In order to correctly report all error types, grub_errno should be set
to retval before entering the error path.

> +
> +  grub_loader_set (linux_boot, linux_unload, 1);

Since linux_boot() may return control to its caller, the third argument
of grub_loader_set() should be 0, otherwise linux_boot() returning
control to its caller results in a dysfunctional state.

> +
> +  size = grub_loader_cmdline_size (argc, argv);
> +  linux_args = grub_malloc (size + sizeof (LINUX_IMAGE));
> +  if (!linux_args)
> +    goto fail;

grub_loader_unset() should be called in this error path, otherwise if
one tries to boot in this state, linux_prepare_fdt() will access a NULL
pointer.

> +
> +  /* Create kernel command line.  */
> +  grub_memcpy (linux_args, LINUX_IMAGE, sizeof (LINUX_IMAGE));
> +  grub_create_loader_cmdline (argc, argv,
> +			      linux_args + sizeof (LINUX_IMAGE) - 1, size);
> +
> +  return GRUB_ERR_NONE;
> +
> +fail:
> +  grub_dl_unref (my_mod);
> +  return grub_errno;
> +}
> +
> +static grub_err_t
> +grub_cmd_initrd (grub_command_t cmd __attribute__ ((unused)),
> +		 int argc, char *argv[])
> +{
> +  grub_file_t file;
> +  int size;
> +
> +  if (argc == 0)
> +    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("filename expected"));
> +
> +  file = grub_file_open (argv[0]);
> +  if (!file)
> +    return grub_errno;
> +
> +  size = grub_file_size (file);
> +  if (size == 0)
> +    goto fail;
> +
> +#ifdef GRUB_MACHINE_EFI
> +  initrd_start = (grub_addr_t) grub_efi_allocate_loader_memory (LINUX_INITRD_PHYS_OFFSET, size);

If the initrd memory was already allocated as a result of the initrd
command being already called previously, it should be freed before
re-allocating memory. Also there should be a check against memory
allocation failure.

> +#else
> +  initrd_start = LINUX_INITRD_ADDRESS;
> +#endif
> +  grub_dprintf ("loader", "Loading initrd to 0x%08x\n",
> +		(grub_addr_t) initrd_start);
> +
> +  if (grub_file_read (file, (void *) initrd_start, size) != size)
> +    goto fail;

In the EFI case, the memory allocated for the initrd should be freed
here. And in both EFI and U-Boot cases, initrd_start should be zeroed.

> +
> +  initrd_end = initrd_start + size;
> +
> +  return GRUB_ERR_NONE;
> +
> +fail:
> +  grub_file_close (file);
> +
> +  return grub_errno;
> +}
> +
> +static void *
> +load_dtb (grub_file_t dtb, int size)
> +{
> +  void *fdt;
> +
> +  fdt = grub_malloc (size);
> +  if (!fdt)
> +    return NULL;
> +
> +  if (grub_file_read (dtb, fdt, size) != size)
> +    {
> +      grub_free (fdt);
> +      return NULL;
> +    }
> +
> +  if (fdt_check_header (fdt) != 0)
> +    {
> +      grub_free (fdt);
> +      return NULL;
> +    }
> +
> +  return fdt;
> +}
> +
> +static grub_err_t
> +grub_cmd_devicetree (grub_command_t cmd __attribute__ ((unused)),
> +		     int argc, char *argv[])
> +{
> +  grub_file_t dtb;
> +  void *blob;
> +  int size;
> +
> +  if (argc != 1)
> +    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("filename expected"));
> +
> +  dtb = grub_file_open (argv[0]);
> +  if (!dtb)
> +    return grub_error (GRUB_ERR_FILE_NOT_FOUND, N_("failed to open file"));
> +
> +  size = grub_file_size (dtb);
> +  if (size == 0)
> +    goto out;
> +
> +  blob = load_dtb (dtb, size);
> +  if (!blob)
> +    return GRUB_ERR_FILE_NOT_FOUND;
> +
> +#ifdef GRUB_MACHINE_EFI
> +  boot_data = (grub_addr_t) grub_efi_allocate_loader_memory (LINUX_FDT_PHYS_OFFSET, size);

As above, if the fdt memory was already allocated as a result of the
devicetree command being already called previously, it should be freed
before re-allocating memory. Also there should be a check against memory
allocation failure.

> +#else
> +  boot_data = LINUX_FDT_ADDRESS;
> +#endif
> +  grub_dprintf ("loader", "Loading device tree to 0x%08x\n",
> +		(grub_addr_t) boot_data);
> +  fdt_move (blob, (void *) boot_data, fdt_totalsize (blob));
> +  grub_free (blob);

I don't get the point of allocating memory for the FDT in load_dtb()
just to move the FDT data to boot_data and then freeing the temporary
memory. Isn't it easier if load_dtb() operates directly on boot_data?

> +
> +  /* 
> +   * We've successfully loaded an FDT, so any machine type passed
> +   * from firmware is now obsolete.
> +   */
> +  machine_type = ARM_FDT_MACHINE_TYPE;
> +
> +  return GRUB_ERR_NONE;

The file should be closed before returning.

> +
> + out:
> +  grub_file_close (dtb);
> +
> +  return grub_errno;
> +}

--
Francesco

Re: [PATCH 6/7] add ARM Linux loader

[Leif Lindholm]

On Mon, Apr 01, 2013 at 06:18:17PM +0200, Francesco Lavra wrote:
> On 03/24/2013 06:01 PM, Leif Lindholm wrote:
> > === added directory 'grub-core/loader/arm'
> > === added file 'grub-core/loader/arm/linux.c'
> > --- grub-core/loader/arm/linux.c	1970-01-01 00:00:00 +0000
> > +++ grub-core/loader/arm/linux.c	2013-03-24 13:49:04 +0000
[...]
> > +static grub_addr_t initrd_start;
> > +static grub_size_t initrd_end;
> 
> initrd_end should be the same type as initrd_start.
 
Clearly.

[...]
> > +static grub_err_t
> > +linux_prepare_fdt (void)
> > +{
> > +  int node;
> > +  int retval;
> > +  int tmp_size;
> > +  void *tmp_fdt;
> > +
> > +  tmp_size = fdt_totalsize ((void *) boot_data) + FDT_ADDITIONAL_ENTRIES_SIZE;
> 
> Having a fixed size limit (FDT_ADDITIONAL_ENTRIES_SIZE) to insert
> variable-sized data (such as the linux command line) seems suboptimal,
> as this may fail when a very long command line is passed. How about
> removing the FDT_ADDITIONAL_ENTRIES_SIZE define from the header file
> include/grub/arm/linux.h (after all, this is an implementation detail
> and IMHO shouldn't go in a public header file) and defining it in this
> function, say:
> 
> #define FDT_ADDITIONAL_ENTRIES_SIZE	(0x100 +	\
> 	grub_strlen (linux_args))
 
Mmm. It was only used in one location though, so I'll use your suggestion,
but just inline it.

> [...]
> > +#ifdef GRUB_MACHINE_EFI
> > +  linux_addr = (grub_addr_t) grub_efi_allocate_loader_memory (LINUX_PHYS_OFFSET, size);
> 
> There should be a check against allocation failure.
 
Yes, this is a global issue with this loader under EFI - dealing with memory
allocation/deallocation. Will be resolved in next version.

> > +static grub_err_t
> > +linux_unload (void)
> > +{
> > +  grub_dl_unref (my_mod);
> > +
> > +  grub_free (linux_args);
> > +  linux_args = NULL;
> 
> linux_args might already be NULL, if memory allocation for it failed in
> grub_cmd_linux().

Yes, but on the other hand grub_free (like libc free) checks for NULL.

> In the EFI case, the memory allocated for the kernel image should be
> freed as well.
 
Yes.

> > +
> > +  initrd_start = initrd_end = 0;
> 
> Why should the initrd data be discarded here?
 
It probably shouldn't.

> > +
> > +  return GRUB_ERR_NONE;
> > +}
> > +static grub_err_t
> > +grub_cmd_linux (grub_command_t cmd __attribute__ ((unused)),
> > +		int argc, char *argv[])
> > +{
> > +  int size, retval;
> 
> The return type of linux_load() is grub_err_t, so retval should be the
> same type.
> 
> > +  grub_file_t file;
> > +  grub_dl_ref (my_mod);
> > +
> > +  if (argc == 0)
> > +    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("filename expected"));
> > +
> > +  file = grub_file_open (argv[0]);
> > +  if (!file)
> > +    goto fail;
> > +
> > +  retval = linux_load (argv[0]);
> 
> Here the file is already open: why not just pass the file handle to
> linux_load()?
 
Unfortunate leftover from earlier version, thanks.

> > +  grub_file_close (file);
> > +  if (retval != GRUB_ERR_NONE)
> > +    goto fail;
> 
> In order to correctly report all error types, grub_errno should be set
> to retval before entering the error path.
 
OK, adding grub_error() to all failure paths in linux_load.

> > +
> > +  grub_loader_set (linux_boot, linux_unload, 1);
> 
> Since linux_boot() may return control to its caller, the third argument
> of grub_loader_set() should be 0, otherwise linux_boot() returning
> control to its caller results in a dysfunctional state.
 
OK.

> > +
> > +  size = grub_loader_cmdline_size (argc, argv);
> > +  linux_args = grub_malloc (size + sizeof (LINUX_IMAGE));
> > +  if (!linux_args)
> > +    goto fail;
> 
> grub_loader_unset() should be called in this error path, otherwise if
> one tries to boot in this state, linux_prepare_fdt() will access a NULL
> pointer.
 
Yes.

> > +  if (grub_file_read (file, (void *) initrd_start, size) != size)
> > +    goto fail;
> 
> In the EFI case, the memory allocated for the initrd should be freed
> here. And in both EFI and U-Boot cases, initrd_start should be zeroed.
 
Yes.

> > +#else
> > +  boot_data = LINUX_FDT_ADDRESS;
> > +#endif
> > +  grub_dprintf ("loader", "Loading device tree to 0x%08x\n",
> > +		(grub_addr_t) boot_data);
> > +  fdt_move (blob, (void *) boot_data, fdt_totalsize (blob));
> > +  grub_free (blob);
> 
> I don't get the point of allocating memory for the FDT in load_dtb()
> just to move the FDT data to boot_data and then freeing the temporary
> memory. Isn't it easier if load_dtb() operates directly on boot_data?
 
Maybe not the sanest of reasons, but the U-Boot port permits passing
through ATAGs if no FDT is available, so I prefer not corrupting
boot_data until a successful load has occurred.

> > +
> > +  /* 
> > +   * We've successfully loaded an FDT, so any machine type passed
> > +   * from firmware is now obsolete.
> > +   */
> > +  machine_type = ARM_FDT_MACHINE_TYPE;
> > +
> > +  return GRUB_ERR_NONE;
> 
> The file should be closed before returning.

Yes.

/
    Leif

Re: [PATCH 6/7] add ARM Linux loader

[Francesco Lavra]

Hi, I noticed another minor issue in this patch.

On 03/24/2013 06:01 PM, Leif Lindholm wrote:
> --- grub-core/loader/arm/linux.c	1970-01-01 00:00:00 +0000
> +++ grub-core/loader/arm/linux.c	2013-03-24 13:49:04 +0000
[...]
> +static grub_err_t
> +grub_cmd_initrd (grub_command_t cmd __attribute__ ((unused)),
> +		 int argc, char *argv[])
> +{
> +  grub_file_t file;
> +  int size;
> +
> +  if (argc == 0)
> +    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("filename expected"));
> +
> +  file = grub_file_open (argv[0]);
> +  if (!file)
> +    return grub_errno;
> +
> +  size = grub_file_size (file);
> +  if (size == 0)
> +    goto fail;
> +
> +#ifdef GRUB_MACHINE_EFI
> +  initrd_start = (grub_addr_t) grub_efi_allocate_loader_memory (LINUX_INITRD_PHYS_OFFSET, size);
> +#else
> +  initrd_start = LINUX_INITRD_ADDRESS;
> +#endif
> +  grub_dprintf ("loader", "Loading initrd to 0x%08x\n",
> +		(grub_addr_t) initrd_start);
> +
> +  if (grub_file_read (file, (void *) initrd_start, size) != size)
> +    goto fail;
> +
> +  initrd_end = initrd_start + size;
> +
> +  return GRUB_ERR_NONE;

The file should be closed also if the initrd is successfully loaded.

> +
> +fail:
> +  grub_file_close (file);
> +
> +  return grub_errno;
> +}

--
Francesco