Re: [PATCH v3 2/2] arm64: add EFI Linux loader

[Vladimir 'φ-coder/phcoder' Serbinenko]

On 20.12.2013 17:44, Leif Lindholm wrote:
> Bugfix of v2 loaded_fdt handling and removal of redundant typedefs.
> 
> ---
>  grub-core/Makefile.core.def    |    3 +-
>  grub-core/loader/arm64/linux.c |  479 
> ++++++++++++++++++++++++++++++++++++++++
>  include/grub/arm64/linux.h     |   54 +++++
>  include/grub/efi/api.h         |    4 +
>  4 files changed, 539 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 e5e558c..c916246 100644
> --- a/grub-core/Makefile.core.def
> +++ b/grub-core/Makefile.core.def
> @@ -1672,7 +1672,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..52c6160
> --- /dev/null
> +++ b/grub-core/loader/arm64/linux.c
> @@ -0,0 +1,479 @@
> +/*
> + *  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/linux.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)
> +#define GRUB_EFI_PE_MAGIC    0x5A4D
> +
> +static grub_efi_guid_t fdt_guid = GRUB_EFI_DEVICE_TREE_GUID;
> +
> +static grub_dl_t my_mod;
> +static int loaded;
> +
> +static void *kernel_addr;
> +static grub_uint64_t kernel_size;
> +
> +static char *linux_args;
> +static grub_uint32_t cmdline_size;
> +
> +static grub_addr_t initrd_start;
> +static grub_addr_t initrd_end;
> +
> +static void *loaded_fdt;
> +static void *fdt;
> +
> +static void *
> +get_firmware_fdt (void)
> +{
> +  grub_efi_configuration_table_t *tables;
> +  void *firmware_fdt = NULL;
> +  unsigned int i;
> +
> +  /* 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)
> +      {
> +     firmware_fdt = tables[i].vendor_table;
> +     grub_dprintf ("linux", "found registered FDT @ 0x%p\n", firmware_fdt);
> +     break;
> +      }
> +
> +  return firmware_fdt;
> +}
> +
> +static void
> +get_fdt (void)
> +{
> +  void *raw_fdt;
> +  int size;
> +
int doesn't sound like right type here. Did you mean grub_size_t ?
> +  if (fdt)
> +    {
> +      size = BYTES_TO_PAGES (grub_fdt_get_totalsize (fdt));
> +      grub_efi_free_pages ((grub_efi_physical_address_t) fdt, size);
> +    }
> +
> +  if (loaded_fdt)
> +    raw_fdt = loaded_fdt;
> +  else
> +    raw_fdt = get_firmware_fdt();
> +
> +  size =
> +    raw_fdt ? grub_fdt_get_totalsize (raw_fdt) : GRUB_FDT_EMPTY_TREE_SZ;
> +  size += 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 (raw_fdt)
> +    {
> +      grub_memmove (fdt, raw_fdt, size);
> +      grub_fdt_set_totalsize (fdt, size);
> +    }
> +  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_error(GRUB_ERR_BAD_OS, N_("Invalid kernel image"));
> +
Please reuse the strings from other platforms. "invalid magic number" in
this case
> +  if ((lh->code0 & 0xffff) != GRUB_EFI_PE_MAGIC)
> +    return grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET,
> +                    N_("Plain Image kernel not supported - rebuild with 
> CONFIG_UEFI_STUB enabled.\n"));
> +
Error messgaes startwith lowercase and have neither newline nor dot at
the end. They're used in error: %s.\n template.

> +  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_efi_free_pages ((grub_efi_physical_address_t) fdt,
> +                    BYTES_TO_PAGES (grub_fdt_get_totalsize (fdt)));
> +  fdt = NULL;
> +  return GRUB_ERR_BAD_OS;
grub_error is missing.
> +  size = grub_file_size (dtb);
> +  blob = grub_malloc (size);
> +  if (!blob)
> +    goto out;
> +
> +  if (grub_file_read (dtb, blob, size) < size)
> +    {
> +      if (!grub_errno)
> +     grub_error (GRUB_ERR_BAD_OS, N_("premature end of file %s"), argv[0]);
> +      goto out;
> +    }
> +
> +  if (grub_fdt_check_header (blob, size) != 0)
> +    {
> +      grub_error (GRUB_ERR_BAD_OS, N_("Invalid FDT"));
Please reuse the strings: N_("invalid device tree")
Every translatable string is work for translators.
> +
> +  mempath = grub_malloc (2 * sizeof (grub_efi_memory_mapped_device_path_t));
> +  if (!mempath)
> +    return GRUB_ERR_OUT_OF_MEMORY;
> +
return grub_errno;
> +  mempath[0].header.type = GRUB_EFI_HARDWARE_DEVICE_PATH_TYPE;
> +  mempath[0].header.subtype = GRUB_EFI_MEMORY_MAPPED_DEVICE_PATH_SUBTYPE;
> +  mempath[0].header.length = grub_cpu_to_le16_compile_time (sizeof 
> (*mempath));
> +  mempath[0].memory_type = GRUB_EFI_LOADER_DATA;
> +  mempath[0].start_address = (grub_addr_t) kernel_addr;
> +  mempath[0].end_address = (grub_addr_t) kernel_addr + kernel_size;
> +
> +  mempath[1].header.type = GRUB_EFI_END_DEVICE_PATH_TYPE;
> +  mempath[1].header.subtype = GRUB_EFI_END_ENTIRE_DEVICE_PATH_SUBTYPE;
> +  mempath[1].header.length = 0;
> +
> +  b = grub_efi_system_table->boot_services;
> +  status = b->load_image (0, grub_efi_image_handle,
> +                       (grub_efi_device_path_t *) mempath,
> +                          kernel_addr, kernel_size, &image_handle);
> +  if (status != GRUB_EFI_SUCCESS)
> +    return GRUB_ERR_BAD_OS;
> +
grub_error is missing
> +  grub_dprintf ("linux", "linux command line: '%s'\n", linux_args);
> +
> +  /* Convert command line to UCS-2 */
> +  loaded_image = grub_efi_get_loaded_image (image_handle);
> +  loaded_image->load_options_size =
> +    grub_strlen (linux_args) * sizeof (grub_efi_char16_t);
> +  loaded_image->load_options = p16 =
> +    grub_efi_allocate_pages (0,
> +                          BYTES_TO_PAGES (loaded_image->load_options_size));
> +  if (!loaded_image->load_options)
> +    return GRUB_ERR_OUT_OF_MEMORY;
> +
grub_error
> +  p8 = linux_args;
> +  do
> +    *p16++ = *p8++;
> +  while (*p8);
> +
Please use utf8_to_utf16. MAke sure that you allocate enough memory.
> +
> +  if (grub_file_read (file, &lh, sizeof (lh)) < (long) sizeof (lh))
> +    return GRUB_ERR_FILE_READ_ERROR;
> +
return grub_errno;
> +      grub_error (GRUB_ERR_OUT_OF_MEMORY, N_("can't allocate kernel"));
N_("out of memory")
> +  linux_args = grub_malloc (cmdline_size);
> +  if (!linux_args)
> +    grub_error (GRUB_ERR_OUT_OF_MEMORY, "cannot allocate command line");
goto fail. Otherwise you try to copy to NULL.

Also I'd recommend calling grub_loader_unset after checking file format
but before starting all the loading. It's not strictily necessarry but
avoids possible problems if loading fails midway with half of variables
referring to old and half to new load attempt.

signature.asc
Description: OpenPGP digital signature