diff mbox series

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

Message ID 20181125233815.56392-7-agraf@suse.de
State Superseded
Headers show
Series Add RISC-V support | expand

Commit Message

Alexander Graf Nov. 25, 2018, 11:38 p.m. UTC
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 <agraf@suse.de>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>


---

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

-- 
2.12.3


_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Comments

Daniel Kiper Jan. 17, 2019, 12:24 p.m. UTC | #1
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 <agraf@suse.de>

> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

>

> ---

>

> 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

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Leif Lindholm Jan. 17, 2019, 2:50 p.m. UTC | #2
On Thu, Jan 17, 2019 at 01:24:29PM +0100, Daniel Kiper wrote:
> > +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.


So, this one is my fault.
It effectively comes down to "the space made available for the chosen
node" - meaning space for the initrd start/end address nodes (and
their associated strings), and now the address-cells node as well.
So we need "some extra space".
(The parameter is the "extra space" that will be dynamically added to
the DT - the static struct is separately accounted for if we're
creating an empty one.)

Since at the point this memory gets allocated we're already very close
to jumping into the kernel, it didn't feel worth trying to calculate
the exact number of bytes needed.

I agree it's ugly. Would you be OK with a centralised
GRUB_EFI_LINUX_FDT_EXTRA_SPACE #define?

/
    Leif

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Daniel Kiper Jan. 18, 2019, 11:45 a.m. UTC | #3
On Thu, Jan 17, 2019 at 02:50:19PM +0000, Leif Lindholm wrote:
> On Thu, Jan 17, 2019 at 01:24:29PM +0100, Daniel Kiper wrote:

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

>

> So, this one is my fault.

> It effectively comes down to "the space made available for the chosen

> node" - meaning space for the initrd start/end address nodes (and

> their associated strings), and now the address-cells node as well.

> So we need "some extra space".

> (The parameter is the "extra space" that will be dynamically added to

> the DT - the static struct is separately accounted for if we're

> creating an empty one.)

>

> Since at the point this memory gets allocated we're already very close

> to jumping into the kernel, it didn't feel worth trying to calculate

> the exact number of bytes needed.

>

> I agree it's ugly. Would you be OK with a centralised

> GRUB_EFI_LINUX_FDT_EXTRA_SPACE #define?


Yes, go ahead with that.

Daniel

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Alexander Graf Jan. 22, 2019, 4:09 p.m. UTC | #4
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 <agraf@suse.de>

>> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

>>

>> ---

>>

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

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Daniel Kiper Jan. 23, 2019, 10:41 a.m. UTC | #5
On Tue, Jan 22, 2019 at 05:09:18PM +0100, Alexander Graf wrote:
> 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 <agraf@suse.de>

> >> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

> >>

> >> ---

> >>

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


ARM copy is fixed in the master right now. So, you can fix it here too.

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


Yeah, this is rather cosmetic. However, AFAICT "fail" is more common in
the GRUB code. Or "err" would be even better if possible.

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


Sadly I am too busy right now to take a look at it. If somebody is willing
to work on it then I can review the patches. Anyway, I am adding this
to TODO list.

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


Yep, I agree. Sadly I am afraid that we have to live with that. Maybe
checkpatch.pl will relieve some pain in the future.

[...]

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


If yes then never mind.

Daniel

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Leif Lindholm Jan. 23, 2019, 11:47 a.m. UTC | #6
On Tue, Jan 22, 2019 at 05:09:18PM +0100, Alexander Graf wrote:
> 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 <agraf@suse.de>

> >> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

> >>

> >> ---

> >>

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


$ git grep "fail:" | wc -l
180
$ git grep "failure:" | wc -l
5

Err, yeah, fair enough. And the only perpetrators in C code (that
aren't part of an imported project) were added by me.

Daniel: would you take a single patch for
loader/arm/linux.c and loader/arm64/linux.c?

> > 

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


I mean, that would superficially seem like the right thing to do, but
I'd really be happy with either. I haven't really managed to find a
natural pattern to where grub_errno is supposed to be used/set and not.

> > 

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


Same answer.

> > 

> >> +}

> >> +

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


Well, anyone exiting through the failure path will hit
  return grub_error(GRUB_ERR_BAD_OS, "failed to install/update FDT");
which does set grub_errno.

What finalize_params_linux () doesn't do is set grub_errno to
GRUB_ERR_NONE at the start. But then, you should only look at the
errno if the call returned failure, right?

> > 

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


Now this bit I know _I_ cargo-culted :)

> > 

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


Likewise.

/
    Leif

> > 

> >> +}

> >> +

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


_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Daniel Kiper Jan. 23, 2019, 4:53 p.m. UTC | #7
On Wed, Jan 23, 2019 at 11:47:25AM +0000, Leif Lindholm wrote:
> On Tue, Jan 22, 2019 at 05:09:18PM +0100, Alexander Graf wrote:

> > On 17.01.19 13:24, Daniel Kiper wrote:

> > > On Mon, Nov 26, 2018 at 12:38:11AM +0100, Alexander Graf wrote:


[...]

> > >> +  return GRUB_ERR_NONE;

> > >> +

> > >> + failure:

> > >

> > > s/failure/fail/?

> >

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

>

> $ git grep "fail:" | wc -l

> 180

> $ git grep "failure:" | wc -l

> 5

>

> Err, yeah, fair enough. And the only perpetrators in C code (that

> aren't part of an imported project) were added by me.

>

> Daniel: would you take a single patch for

> loader/arm/linux.c and loader/arm64/linux.c?


If you change label name only then I am OK with that.

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

>

> I mean, that would superficially seem like the right thing to do, but

> I'd really be happy with either. I haven't really managed to find a

> natural pattern to where grub_errno is supposed to be used/set and not.


Could you check what is the rule among several/all EFI GRUB functions?
If they do not set grub_errno then you should set it here. If more EFI
GRUB functions set grub_errno then these ones called here should be
updated accordingly.

[...]

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

>

> Now this bit I know _I_ cargo-culted :)


Well, there is a chance that I do not get it because I am not native speaker.
Could you enlighten me what does it mean?

Daniel

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Leif Lindholm Jan. 23, 2019, 5:15 p.m. UTC | #8
On Wed, Jan 23, 2019 at 05:53:00PM +0100, Daniel Kiper wrote:
> On Wed, Jan 23, 2019 at 11:47:25AM +0000, Leif Lindholm wrote:

> > On Tue, Jan 22, 2019 at 05:09:18PM +0100, Alexander Graf wrote:

> > > On 17.01.19 13:24, Daniel Kiper wrote:

> > > > On Mon, Nov 26, 2018 at 12:38:11AM +0100, Alexander Graf wrote:

> 

> [...]

> 

> > > >> +  return GRUB_ERR_NONE;

> > > >> +

> > > >> + failure:

> > > >

> > > > s/failure/fail/?

> > >

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

> >

> > $ git grep "fail:" | wc -l

> > 180

> > $ git grep "failure:" | wc -l

> > 5

> >

> > Err, yeah, fair enough. And the only perpetrators in C code (that

> > aren't part of an imported project) were added by me.

> >

> > Daniel: would you take a single patch for

> > loader/arm/linux.c and loader/arm64/linux.c?

> 

> If you change label name only then I am OK with that.


Yeah, that's what I meant. Coming up.

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

> >

> > I mean, that would superficially seem like the right thing to do, but

> > I'd really be happy with either. I haven't really managed to find a

> > natural pattern to where grub_errno is supposed to be used/set and not.

> 

> Could you check what is the rule among several/all EFI GRUB functions?

> If they do not set grub_errno then you should set it here. If more EFI

> GRUB functions set grub_errno then these ones called here should be

> updated accordingly.


I'm afraid it doesn't look terribly consistent - some very core
functions, like grub_efi_set_virtual_address_map...

"£$%$^!

Why?
Why???
Why do we have a grub_efi_set_virtual_address_map()?

Requesting permission to nuke.


Anyway, back on topic.
grub_efi_set_variable() sets it.
grub_efi_finish_boot_services() sets it (not that we make use of that
for arm*). To be honest, most calls to grub_error() are in this
function.

> [...]

> 

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

> >

> > Now this bit I know _I_ cargo-culted :)

> 

> Well, there is a chance that I do not get it because I am not native speaker.

> Could you enlighten me what does it mean?


I copied existing code without particularly paying any attention to
what it was doing.
Based on https://en.wikipedia.org/wiki/Cargo_cult.

I probably copied i386/pc/linux.c.

/
    Leif

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Colin Watson Jan. 23, 2019, 10:51 p.m. UTC | #9
On Wed, Jan 23, 2019 at 11:47:25AM +0000, Leif Lindholm wrote:
> I mean, that would superficially seem like the right thing to do, but

> I'd really be happy with either. I haven't really managed to find a

> natural pattern to where grub_errno is supposed to be used/set and not.


There's an "Error Handling" section in docs/grub-dev.texi.  Does
anything there help?

-- 
Colin Watson                                       [cjwatson@ubuntu.com]

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Daniel Kiper Jan. 24, 2019, 1:54 p.m. UTC | #10
On Wed, Jan 23, 2019 at 05:15:58PM +0000, Leif Lindholm wrote:
> On Wed, Jan 23, 2019 at 05:53:00PM +0100, Daniel Kiper wrote:

> > On Wed, Jan 23, 2019 at 11:47:25AM +0000, Leif Lindholm wrote:

> > > On Tue, Jan 22, 2019 at 05:09:18PM +0100, Alexander Graf wrote:

> > > > On 17.01.19 13:24, Daniel Kiper wrote:

> > > > > On Mon, Nov 26, 2018 at 12:38:11AM +0100, Alexander Graf wrote:


[...]

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

> > >

> > > I mean, that would superficially seem like the right thing to do, but

> > > I'd really be happy with either. I haven't really managed to find a

> > > natural pattern to where grub_errno is supposed to be used/set and not.

> >

> > Could you check what is the rule among several/all EFI GRUB functions?

> > If they do not set grub_errno then you should set it here. If more EFI

> > GRUB functions set grub_errno then these ones called here should be

> > updated accordingly.

>

> I'm afraid it doesn't look terribly consistent - some very core


Well, this does not help...

> functions, like grub_efi_set_virtual_address_map...

>

> "£$%$^!

>

> Why?

> Why???

> Why do we have a grub_efi_set_virtual_address_map()?

>

> Requesting permission to nuke.


I think that it can be safely dropped. So, go ahead.

> Anyway, back on topic.

> grub_efi_set_variable() sets it.

> grub_efi_finish_boot_services() sets it (not that we make use of that

> for arm*). To be honest, most calls to grub_error() are in this

> function.


Or let's go different way. If any called function sets grub_errno then
use it. If no then set it properly in your own function. And please if
possible take into account Colin's comment too.

> > [...]

> >

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

> > >

> > > Now this bit I know _I_ cargo-culted :)

> >

> > Well, there is a chance that I do not get it because I am not native speaker.

> > Could you enlighten me what does it mean?

>

> I copied existing code without particularly paying any attention to

> what it was doing.

> Based on https://en.wikipedia.org/wiki/Cargo_cult.

>

> I probably copied i386/pc/linux.c.


Ahhh... OK. I should be a bit smarter and look for it myself...
Anyway, thanks for the explanation.

Daniel

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
diff mbox series

Patch

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;
+
+  void *fdt;
+
+  fdt = grub_fdt_load (0x400);
+
+  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:
+  grub_fdt_unload();
+  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;
+
+  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;
+}
+
+static grub_err_t
+grub_linux_boot (void)
+{
+  if (finalize_params_linux () != GRUB_ERR_NONE)
+    return grub_errno;
+
+  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);
+  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;
+}
+
+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 */
+  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 */
+};
+
+# 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 */
+  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 */
+};
+
+# define linux_arch_kernel_header linux_riscv_kernel_header
+
+#endif /* ! GRUB_RISCV64_LINUX_HEADER */