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
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
