grub-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v4 06/10] RISC-V: Add Linux load logic


From: Daniel Kiper
Subject: Re: [PATCH v4 06/10] RISC-V: Add Linux load logic
Date: Thu, 17 Jan 2019 13:24:29 +0100
User-agent: NeoMutt/20170113 (1.7.2)

On Mon, Nov 26, 2018 at 12:38:11AM +0100, Alexander Graf wrote:
> We currently only support to run grub on RISC-V as UEFI payload. Ideally,
> we also only want to support running Linux underneath as UEFI payload.
>
> Prepare that with a Linux boot case that is not enabled in Linux yet. At
> least it will give people something to test against when they enable the
> Linux UEFI port.
>
> Signed-off-by: Alexander Graf <address@hidden>
> Reviewed-by: Alistair Francis <address@hidden>
>
> ---
>
> v1 -> v2:
>
>   - adapt to new grub_open_file() API
>   - adapt to new grub_create_loader_cmdline() API
>
> v3 -> v4:
>
>   - Change copyright from 2013 to 2018
>   - Coding style fixes
> ---
>  grub-core/loader/riscv/linux.c | 351 
> +++++++++++++++++++++++++++++++++++++++++
>  include/grub/riscv32/linux.h   |  41 +++++
>  include/grub/riscv64/linux.h   |  43 +++++
>  3 files changed, 435 insertions(+)
>  create mode 100644 grub-core/loader/riscv/linux.c
>  create mode 100644 include/grub/riscv32/linux.h
>  create mode 100644 include/grub/riscv64/linux.h
>
> diff --git a/grub-core/loader/riscv/linux.c b/grub-core/loader/riscv/linux.c
> new file mode 100644
> index 000000000..fc8c508c8
> --- /dev/null
> +++ b/grub-core/loader/riscv/linux.c
> @@ -0,0 +1,351 @@
> +/*
> + *  GRUB  --  GRand Unified Bootloader
> + *  Copyright (C) 2018  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/charset.h>
> +#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/fdtload.h>
> +#include <grub/efi/memory.h>
> +#include <grub/efi/pe32.h>
> +#include <grub/i18n.h>
> +#include <grub/lib/cmdline.h>
> +
> +GRUB_MOD_LICENSE ("GPLv3+");
> +
> +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;
> +
> +grub_err_t
> +grub_arch_efi_linux_check_image (struct linux_riscv_kernel_header * lh)
> +{
> +  if (lh->magic != GRUB_LINUX_RISCV_MAGIC_SIGNATURE)
> +    return grub_error(GRUB_ERR_BAD_OS, "invalid magic number");
> +
> +  if ((lh->code0 & 0xffff) != GRUB_PE32_MAGIC)
> +    return grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET,
> +                    N_("plain image kernel not supported - rebuild with 
> CONFIG_(U)EFI_STUB enabled"));
> +
> +  grub_dprintf ("linux", "UEFI stub kernel:\n");
> +  grub_dprintf ("linux", "PE/COFF header @ %08x\n", lh->hdr_offset);
> +
> +  return GRUB_ERR_NONE;
> +}
> +
> +static grub_err_t
> +finalize_params_linux (void)
> +{
> +  int node, retval;
> +

Please drop this empty line.

> +  void *fdt;
> +
> +  fdt = grub_fdt_load (0x400);

Why this number? Please define constant or add a comment here.
Whichever is better. And I can see the same value in ARM64. So,
maybe it is worth using the same constant here and there. Anyway,
please fix it somehow.

> +  if (!fdt)
> +    goto failure;
> +
> +  node = grub_fdt_find_subnode (fdt, 0, "chosen");
> +  if (node < 0)
> +    node = grub_fdt_add_subnode (fdt, 0, "chosen");
> +
> +  if (node < 1)
> +    goto failure;
> +
> +  /* Set initrd info */
> +  if (initrd_start && initrd_end > initrd_start)
> +    {
> +      grub_dprintf ("linux", "Initrd @ %p-%p\n",
> +                 (void *) initrd_start, (void *) initrd_end);
> +
> +      retval = grub_fdt_set_prop64 (fdt, node, "linux,initrd-start",
> +                                 initrd_start);
> +      if (retval)
> +     goto failure;
> +      retval = grub_fdt_set_prop64 (fdt, node, "linux,initrd-end",
> +                                 initrd_end);
> +      if (retval)
> +     goto failure;
> +    }
> +
> +  if (grub_fdt_install() != GRUB_ERR_NONE)
> +    goto failure;
> +
> +  return GRUB_ERR_NONE;
> +
> + failure:

s/failure/fail/?

> +  grub_fdt_unload();

s/grub_fdt_unload()/grub_fdt_unload ()/ May I ask you to fix similar
mistakes in the other patches too?

> +  return grub_error(GRUB_ERR_BAD_OS, "failed to install/update FDT");
> +}
> +
> +grub_err_t
> +grub_arch_efi_linux_boot_image (grub_addr_t addr, grub_size_t size, char 
> *args)
> +{
> +  grub_efi_memory_mapped_device_path_t *mempath;
> +  grub_efi_handle_t image_handle;
> +  grub_efi_boot_services_t *b;
> +  grub_efi_status_t status;
> +  grub_efi_loaded_image_t *loaded_image;
> +  int len;
> +
> +  mempath = grub_malloc (2 * sizeof (grub_efi_memory_mapped_device_path_t));
> +  if (!mempath)
> +    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 = addr;
> +  mempath[0].end_address = addr + 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 = sizeof (grub_efi_device_path_t);
> +
> +  b = grub_efi_system_table->boot_services;
> +  status = b->load_image (0, grub_efi_image_handle,
> +                       (grub_efi_device_path_t *) mempath,
> +                       (void *) addr, size, &image_handle);
> +  if (status != GRUB_EFI_SUCCESS)
> +    return grub_error (GRUB_ERR_BAD_OS, "cannot load image");
> +
> +  grub_dprintf ("linux", "linux command line: '%s'\n", args);
> +
> +  /* Convert command line to UCS-2 */
> +  loaded_image = grub_efi_get_loaded_image (image_handle);
> +  loaded_image->load_options_size = len =
> +    (grub_strlen (args) + 1) * sizeof (grub_efi_char16_t);
> +  loaded_image->load_options =
> +    grub_efi_allocate_any_pages (GRUB_EFI_BYTES_TO_PAGES 
> (loaded_image->load_options_size));
> +  if (!loaded_image->load_options)
> +    return grub_errno;

I am afraid that grub_errno may not be set by
grub_efi_allocate_any_pages() to correct value.

> +  loaded_image->load_options_size =
> +    2 * grub_utf8_to_utf16 (loaded_image->load_options, len,
> +                         (grub_uint8_t *) args, len, NULL);
> +
> +  grub_dprintf ("linux", "starting image %p\n", image_handle);
> +  status = b->start_image (image_handle, 0, NULL);
> +
> +  /* When successful, not reached */
> +  b->unload_image (image_handle);
> +  grub_efi_free_pages ((grub_addr_t) loaded_image->load_options,
> +                    GRUB_EFI_BYTES_TO_PAGES 
> (loaded_image->load_options_size));
> +
> +  return grub_errno;

This return value seems wrong too.

> +}
> +
> +static grub_err_t
> +grub_linux_boot (void)
> +{
> +  if (finalize_params_linux () != GRUB_ERR_NONE)
> +    return grub_errno;

Could you double check that grub_errno is set correctly by
finalize_params_linux()?

> +  return (grub_arch_efi_linux_boot_image((grub_addr_t)kernel_addr,
> +                                         kernel_size, linux_args));
> +}
> +
> +static grub_err_t
> +grub_linux_unload (void)
> +{
> +  grub_dl_unref (my_mod);

I think that would be safer if you call grub_dl_unref() just before return
at the end of this function.

> +  loaded = 0;
> +  if (initrd_start)
> +    grub_efi_free_pages ((grub_efi_physical_address_t) initrd_start,
> +                      GRUB_EFI_BYTES_TO_PAGES (initrd_end - initrd_start));
> +  initrd_start = initrd_end = 0;
> +  grub_free (linux_args);
> +  if (kernel_addr)
> +    grub_efi_free_pages ((grub_addr_t) kernel_addr,
> +                      GRUB_EFI_BYTES_TO_PAGES (kernel_size));
> +  grub_fdt_unload ();
> +  return GRUB_ERR_NONE;
> +}
> +
> +static grub_err_t
> +grub_cmd_initrd (grub_command_t cmd __attribute__ ((unused)),
> +              int argc, char *argv[])
> +{
> +  struct grub_linux_initrd_context initrd_ctx = { 0, 0, 0 };
> +  int initrd_size, initrd_pages;
> +  void *initrd_mem = NULL;
> +
> +  if (argc == 0)
> +    {
> +      grub_error (GRUB_ERR_BAD_ARGUMENT, N_("filename expected"));
> +      goto fail;
> +    }
> +
> +  if (!loaded)
> +    {
> +      grub_error (GRUB_ERR_BAD_ARGUMENT,
> +               N_("you need to load the kernel first"));
> +      goto fail;
> +    }
> +
> +  if (grub_initrd_init (argc, argv, &initrd_ctx))
> +    goto fail;
> +
> +  initrd_size = grub_get_initrd_size (&initrd_ctx);
> +  grub_dprintf ("linux", "Loading initrd\n");
> +
> +  initrd_pages = (GRUB_EFI_BYTES_TO_PAGES (initrd_size));
> +  initrd_mem = grub_efi_allocate_any_pages (initrd_pages);
> +  if (!initrd_mem)
> +    {
> +      grub_error (GRUB_ERR_OUT_OF_MEMORY, N_("out of memory"));
> +      goto fail;
> +    }
> +
> +  if (grub_initrd_load (&initrd_ctx, argv, initrd_mem))
> +    goto fail;
> +
> +  initrd_start = (grub_addr_t) initrd_mem;
> +  initrd_end = initrd_start + initrd_size;
> +  grub_dprintf ("linux", "[addr=%p, size=0x%x]\n",
> +             (void *) initrd_start, initrd_size);
> +
> + fail:
> +  grub_initrd_close (&initrd_ctx);
> +  if (initrd_mem && !initrd_start)
> +    grub_efi_free_pages ((grub_addr_t) initrd_mem, initrd_pages);
> +
> +  return grub_errno;

Again, is grub_errno value correct here?

> +}
> +
> +static grub_err_t
> +grub_cmd_linux (grub_command_t cmd __attribute__ ((unused)),
> +             int argc, char *argv[])
> +{
> +  grub_file_t file = 0;
> +  struct linux_riscv_kernel_header lh;
> +
> +  grub_dl_ref (my_mod);
> +
> +  if (argc == 0)
> +    {
> +      grub_error (GRUB_ERR_BAD_ARGUMENT, N_("filename expected"));
> +      goto fail;
> +    }
> +
> +  file = grub_file_open (argv[0], GRUB_FILE_TYPE_LINUX_KERNEL);
> +  if (!file)
> +    goto fail;
> +
> +  kernel_size = grub_file_size (file);
> +
> +  if (grub_file_read (file, &lh, sizeof (lh)) < (long) sizeof (lh))
> +    return grub_errno;
> +
> +  if (grub_arch_efi_linux_check_image (&lh) != GRUB_ERR_NONE)
> +    goto fail;
> +
> +  grub_loader_unset();
> +
> +  grub_dprintf ("linux", "kernel file size: %lld\n", (long long) 
> kernel_size);
> +  kernel_addr = grub_efi_allocate_any_pages (GRUB_EFI_BYTES_TO_PAGES 
> (kernel_size));
> +  grub_dprintf ("linux", "kernel numpages: %lld\n",
> +             (long long) GRUB_EFI_BYTES_TO_PAGES (kernel_size));
> +  if (!kernel_addr)
> +    {
> +      grub_error (GRUB_ERR_OUT_OF_MEMORY, N_("out of memory"));
> +      goto fail;
> +    }
> +
> +  grub_file_seek (file, 0);
> +  if (grub_file_read (file, kernel_addr, kernel_size)
> +      < (grub_int64_t) kernel_size)
> +    {
> +      if (!grub_errno)
> +     grub_error (GRUB_ERR_BAD_OS, N_("premature end of file %s"), argv[0]);
> +      goto fail;
> +    }
> +
> +  grub_dprintf ("linux", "kernel @ %p\n", kernel_addr);
> +
> +  cmdline_size = grub_loader_cmdline_size (argc, argv) + sizeof 
> (LINUX_IMAGE);
> +  linux_args = grub_malloc (cmdline_size);
> +  if (!linux_args)
> +    {
> +      grub_error (GRUB_ERR_OUT_OF_MEMORY, N_("out of memory"));
> +      goto fail;
> +    }
> +  grub_memcpy (linux_args, LINUX_IMAGE, sizeof (LINUX_IMAGE));
> +  grub_create_loader_cmdline (argc, argv,
> +                           linux_args + sizeof (LINUX_IMAGE) - 1,
> +                           cmdline_size,
> +                           GRUB_VERIFY_KERNEL_CMDLINE);
> +
> +  if (grub_errno == GRUB_ERR_NONE)
> +    {
> +      grub_loader_set (grub_linux_boot, grub_linux_unload, 0);
> +      loaded = 1;
> +    }
> +
> + fail:
> +  if (file)
> +    grub_file_close (file);
> +
> +  if (grub_errno != GRUB_ERR_NONE)
> +    {
> +      grub_dl_unref (my_mod);
> +      loaded = 0;
> +    }
> +
> +  if (linux_args && !loaded)
> +    grub_free (linux_args);
> +
> +  if (kernel_addr && !loaded)
> +    grub_efi_free_pages ((grub_addr_t) kernel_addr,
> +                      GRUB_EFI_BYTES_TO_PAGES (kernel_size));
> +
> +  return grub_errno;
> +}
> +
> +static grub_command_t cmd_linux, cmd_initrd;
> +
> +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."));
> +  my_mod = mod;
> +}
> +
> +GRUB_MOD_FINI (linux)
> +{
> +  grub_unregister_command (cmd_linux);
> +  grub_unregister_command (cmd_initrd);
> +}
> diff --git a/include/grub/riscv32/linux.h b/include/grub/riscv32/linux.h
> new file mode 100644
> index 000000000..9d15ff5c1
> --- /dev/null
> +++ b/include/grub/riscv32/linux.h
> @@ -0,0 +1,41 @@
> +/*
> + *  GRUB  --  GRand Unified Bootloader
> + *  Copyright (C) 2018  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_RISCV32_LINUX_HEADER
> +#define GRUB_RISCV32_LINUX_HEADER 1
> +
> +#define GRUB_LINUX_RISCV_MAGIC_SIGNATURE 0x52534356 /* 'RSCV' */
> +
> +/* From linux/Documentation/riscv/booting.txt */
> +struct linux_riscv_kernel_header
> +{
> +  grub_uint32_t code0;               /* Executable code */
> +  grub_uint32_t code1;               /* Executable code */
> +  grub_uint64_t text_offset;    /* Image load offset */

Tabs instead of spaces please.

> +  grub_uint64_t res0;                /* reserved */
> +  grub_uint64_t res1;                /* reserved */
> +  grub_uint64_t res2;                /* reserved */
> +  grub_uint64_t res3;                /* reserved */
> +  grub_uint64_t res4;                /* reserved */
> +  grub_uint32_t magic;               /* Magic number, little endian, "RSCV" 
> */
> +  grub_uint32_t hdr_offset;  /* Offset of PE/COFF header */

Wrong number of tabs?

> +};
> +
> +# define linux_arch_kernel_header linux_riscv_kernel_header
> +
> +#endif /* ! GRUB_RISCV32_LINUX_HEADER */
> diff --git a/include/grub/riscv64/linux.h b/include/grub/riscv64/linux.h
> new file mode 100644
> index 000000000..ef71742fb
> --- /dev/null
> +++ b/include/grub/riscv64/linux.h
> @@ -0,0 +1,43 @@
> +/*
> + *  GRUB  --  GRand Unified Bootloader
> + *  Copyright (C) 2018  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_RISCV64_LINUX_HEADER
> +#define GRUB_RISCV64_LINUX_HEADER 1
> +
> +#define GRUB_LINUX_RISCV_MAGIC_SIGNATURE 0x52534356 /* 'RSCV' */
> +
> +#define GRUB_EFI_PE_MAGIC    0x5A4D
> +
> +/* From linux/Documentation/riscv/booting.txt */
> +struct linux_riscv_kernel_header
> +{
> +  grub_uint32_t code0;               /* Executable code */
> +  grub_uint32_t code1;               /* Executable code */
> +  grub_uint64_t text_offset;    /* Image load offset */

Ditto.

> +  grub_uint64_t res0;                /* reserved */
> +  grub_uint64_t res1;                /* reserved */
> +  grub_uint64_t res2;                /* reserved */
> +  grub_uint64_t res3;                /* reserved */
> +  grub_uint64_t res4;                /* reserved */
> +  grub_uint32_t magic;               /* Magic number, little endian, "RSCV" 
> */
> +  grub_uint32_t hdr_offset;  /* Offset of PE/COFF header */

Ditto.

Daniel



reply via email to

[Prev in Thread] Current Thread [Next in Thread]