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

[Francesco Lavra <francescolavra.fl@gmail.com>]

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