Re: [PATCH] add arm64 UEFI Linux loader

[Vladimir 'φ-coder/phcoder' Serbinenko]

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 <address@hidden> пишет:
>> > 
>>> > > 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 <address@hidden>
> Date: Wed, 4 Dec 2013 15:21:16 +0000
> Subject: [PATCH 2/2] arm64: add EFI Linux loader
> 
> Signed-off-by: Leif Lindholm <address@hidden>
> ---
>  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.

signature.asc
Description: OpenPGP digital signature