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: Alexander Graf
Subject: Re: [PATCH v4 06/10] RISC-V: Add Linux load logic
Date: Tue, 22 Jan 2019 17:09:18 +0100
User-agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:60.0) Gecko/20100101 Thunderbird/60.4.0


On 17.01.19 13:24, Daniel Kiper wrote:
> 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.

This file is a 1:1 copy from the arm version. Ideally, they should get
merged eventually. But any issues you found here apply similarly to the
arm copy.

> 
>> +  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/?

Why? I mean, sure, I can change it. But why?

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

Can you please write a checkpatch.pl or similar to catch them? At this
point, all those coding style issues just add to frustration on both
sides I think. To me, the GNU style comes very close in uglyness to the
TianoCore one - and my fingers just simply refuse to naturally adhere to it.

That said, same as above. This is a copy from the arm64 linux.c.

> 
>> +  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.

True. What is the intended fix? Have the efi helpers set errno or set
errno here? Leif?

> 
>> +  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.

Same question as above, yes.

> 
>> +}
>> +
>> +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()?

That one too I would defer to Leif.

> 
>> +  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.

Same.

> 
>> +  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?

Same.

> 
>> +}
>> +
>> +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?

Looks correct in my editor?

> 
>> +};
>> +
>> +# 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.

Sure, happy to fix whitespace :)


Alex



reply via email to

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