[PATCH RFC/RFT 0/3] Add grub loader support for RISC-V Linux

[PATCH RFC/RFT 0/3] Add grub loader support for RISC-V Linux

[Atish Patra]

This series adds grub loader support for RISC-V Linux. Thanks to the awesome
initial RISC-V support added by Alex, we just needed a loader for RISC-V to
load and execute Linux using UEFI protocol.

Fortunately, ARM64 Linux loader is written in an architecture agnostic manner
so thatgeneric RISC-V can easily reuse the loader code. Thus, the first patch
just moves the ARM64 code to common code. I have compile tested for
ARM64/ARM32. Even though it doesn't introduce any functional change
for ARM/ARM64, any real testing will be helpful.

I have tested this series for RISC-V on both Qemu and HiFive Unleashed.
Here are the dependencies of other opensource projects.

1. OpenSBI v0.7
2. U-boot master (Head: a5f9b8a8b592 Merge https://gitlab.denx.de/u-boot/custodians/u-boot-riscv)
3. Linux kernel (efi-next + top 4 RISC-V patches from riscv-efi-for-v5.8)
   Linux kernel tree can be found here as well.
   https://github.com/atishp04/linux/pull/new/uefi_riscv_pr
 

Atish Patra (3):
loader: Move arm64 linux loader to common code
RISC-V: Update image header
RISC-V: Use common linux loader

grub-core/Makefile.core.def             |  8 ++--
grub-core/loader/{arm64 => efi}/linux.c |  2 +-
grub-core/loader/riscv/linux.c          | 59 -------------------------
include/grub/arm/linux.h                |  2 +-
include/grub/arm64/linux.h              |  2 +-
include/grub/riscv32/linux.h            | 16 ++++---
include/grub/riscv64/linux.h            | 16 ++++---
7 files changed, 25 insertions(+), 80 deletions(-)
rename grub-core/loader/{arm64 => efi}/linux.c (99%)
delete mode 100644 grub-core/loader/riscv/linux.c

--
2.25.1

[PATCH RFC/RFT 1/3] loader: Move arm64 linux loader to common code

[Atish Patra]

ARM64 linux loader code is written in such a way that it can be reused
across different architectures without much change. Move it to common
code so that RISC-V doesn't have to define a separate loader.

Signed-off-by: Atish Patra <atish.patra@wdc.com>
---
 grub-core/Makefile.core.def             | 4 ++--
 grub-core/loader/{arm64 => efi}/linux.c | 2 +-
 include/grub/arm/linux.h                | 2 +-
 include/grub/arm64/linux.h              | 2 +-
 4 files changed, 5 insertions(+), 5 deletions(-)
 rename grub-core/loader/{arm64 => efi}/linux.c (99%)

diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
index 48b82e76322f..57bb70d73764 100644
--- a/grub-core/Makefile.core.def
+++ b/grub-core/Makefile.core.def
@@ -1813,9 +1813,9 @@ module = {
   sparc64_ieee1275 = loader/sparc64/ieee1275/linux.c;
   ia64_efi = loader/ia64/efi/linux.c;
   arm_coreboot = loader/arm/linux.c;
-  arm_efi = loader/arm64/linux.c;
+  arm_efi = loader/efi/linux.c;
   arm_uboot = loader/arm/linux.c;
-  arm64 = loader/arm64/linux.c;
+  arm64 = loader/efi/linux.c;
   riscv32 = loader/riscv/linux.c;
   riscv64 = loader/riscv/linux.c;
   common = loader/linux.c;
diff --git a/grub-core/loader/arm64/linux.c b/grub-core/loader/efi/linux.c
similarity index 99%
rename from grub-core/loader/arm64/linux.c
rename to grub-core/loader/efi/linux.c
index ef3e9f9444ca..8c7a4963f023 100644
--- a/grub-core/loader/arm64/linux.c
+++ b/grub-core/loader/efi/linux.c
@@ -51,7 +51,7 @@ static grub_addr_t initrd_end;
 grub_err_t
 grub_arch_efi_linux_check_image (struct linux_arch_kernel_header * lh)
 {
-  if (lh->magic != GRUB_LINUX_ARMXX_MAGIC_SIGNATURE)
+  if (lh->magic != GRUB_LINUX_ARCH_MAGIC_SIGNATURE)
     return grub_error(GRUB_ERR_BAD_OS, "invalid magic number");
 
   if ((lh->code0 & 0xffff) != GRUB_PE32_MAGIC)
diff --git a/include/grub/arm/linux.h b/include/grub/arm/linux.h
index 2e98a6689696..c924d15446af 100644
--- a/include/grub/arm/linux.h
+++ b/include/grub/arm/linux.h
@@ -35,7 +35,7 @@ struct linux_arm_kernel_header {
 };
 
 #if defined(__arm__)
-# define GRUB_LINUX_ARMXX_MAGIC_SIGNATURE GRUB_LINUX_ARM_MAGIC_SIGNATURE
+# define GRUB_LINUX_ARCH_MAGIC_SIGNATURE GRUB_LINUX_ARM_MAGIC_SIGNATURE
 # define linux_arch_kernel_header linux_arm_kernel_header
 #endif
 
diff --git a/include/grub/arm64/linux.h b/include/grub/arm64/linux.h
index 4269adc6dae5..96aa3bae01ee 100644
--- a/include/grub/arm64/linux.h
+++ b/include/grub/arm64/linux.h
@@ -37,7 +37,7 @@ struct linux_arm64_kernel_header
 };
 
 #if defined(__aarch64__)
-# define GRUB_LINUX_ARMXX_MAGIC_SIGNATURE GRUB_LINUX_ARM64_MAGIC_SIGNATURE
+# define GRUB_LINUX_ARCH_MAGIC_SIGNATURE GRUB_LINUX_ARM64_MAGIC_SIGNATURE
 # define linux_arch_kernel_header linux_arm64_kernel_header
 #endif
 
-- 
2.25.1

Re: [PATCH RFC/RFT 1/3] loader: Move arm64 linux loader to common code

[Heinrich Schuchardt]

On 4/26/20 9:40 PM, Atish Patra wrote:
> ARM64 linux loader code is written in such a way that it can be reused
> across different architectures without much change. Move it to common
> code so that RISC-V doesn't have to define a separate loader.
>
> Signed-off-by: Atish Patra <atish.patra@wdc.com>

Tested on Odroid-C2 (ARMv8) using GRUB master c543d678105037a plus this
patch series booting from U-Boot via GRUB to Debian Bullseye successfully.

Tested-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

> ---
>  grub-core/Makefile.core.def             | 4 ++--
>  grub-core/loader/{arm64 => efi}/linux.c | 2 +-
>  include/grub/arm/linux.h                | 2 +-
>  include/grub/arm64/linux.h              | 2 +-
>  4 files changed, 5 insertions(+), 5 deletions(-)
>  rename grub-core/loader/{arm64 => efi}/linux.c (99%)
>
> diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
> index 48b82e76322f..57bb70d73764 100644
> --- a/grub-core/Makefile.core.def
> +++ b/grub-core/Makefile.core.def
> @@ -1813,9 +1813,9 @@ module = {
>    sparc64_ieee1275 = loader/sparc64/ieee1275/linux.c;
>    ia64_efi = loader/ia64/efi/linux.c;
>    arm_coreboot = loader/arm/linux.c;
> -  arm_efi = loader/arm64/linux.c;
> +  arm_efi = loader/efi/linux.c;
>    arm_uboot = loader/arm/linux.c;
> -  arm64 = loader/arm64/linux.c;
> +  arm64 = loader/efi/linux.c;
>    riscv32 = loader/riscv/linux.c;
>    riscv64 = loader/riscv/linux.c;
>    common = loader/linux.c;
> diff --git a/grub-core/loader/arm64/linux.c b/grub-core/loader/efi/linux.c
> similarity index 99%
> rename from grub-core/loader/arm64/linux.c
> rename to grub-core/loader/efi/linux.c
> index ef3e9f9444ca..8c7a4963f023 100644
> --- a/grub-core/loader/arm64/linux.c
> +++ b/grub-core/loader/efi/linux.c
> @@ -51,7 +51,7 @@ static grub_addr_t initrd_end;
>  grub_err_t
>  grub_arch_efi_linux_check_image (struct linux_arch_kernel_header * lh)
>  {
> -  if (lh->magic != GRUB_LINUX_ARMXX_MAGIC_SIGNATURE)
> +  if (lh->magic != GRUB_LINUX_ARCH_MAGIC_SIGNATURE)
>      return grub_error(GRUB_ERR_BAD_OS, "invalid magic number");
>
>    if ((lh->code0 & 0xffff) != GRUB_PE32_MAGIC)
> diff --git a/include/grub/arm/linux.h b/include/grub/arm/linux.h
> index 2e98a6689696..c924d15446af 100644
> --- a/include/grub/arm/linux.h
> +++ b/include/grub/arm/linux.h
> @@ -35,7 +35,7 @@ struct linux_arm_kernel_header {
>  };
>
>  #if defined(__arm__)
> -# define GRUB_LINUX_ARMXX_MAGIC_SIGNATURE GRUB_LINUX_ARM_MAGIC_SIGNATURE
> +# define GRUB_LINUX_ARCH_MAGIC_SIGNATURE GRUB_LINUX_ARM_MAGIC_SIGNATURE
>  # define linux_arch_kernel_header linux_arm_kernel_header
>  #endif
>
> diff --git a/include/grub/arm64/linux.h b/include/grub/arm64/linux.h
> index 4269adc6dae5..96aa3bae01ee 100644
> --- a/include/grub/arm64/linux.h
> +++ b/include/grub/arm64/linux.h
> @@ -37,7 +37,7 @@ struct linux_arm64_kernel_header
>  };
>
>  #if defined(__aarch64__)
> -# define GRUB_LINUX_ARMXX_MAGIC_SIGNATURE GRUB_LINUX_ARM64_MAGIC_SIGNATURE
> +# define GRUB_LINUX_ARCH_MAGIC_SIGNATURE GRUB_LINUX_ARM64_MAGIC_SIGNATURE
>  # define linux_arch_kernel_header linux_arm64_kernel_header
>  #endif
>
>

[PATCH RFC/RFT 2/3] RISC-V: Update image header

[Atish Patra]

Update the RISC-V Linux kernel image headers as per the current header.

Reference:
<Linux kernel source>/Documentation/riscv/boot-image-header.rst

Signed-off-by: Atish Patra <atish.patra@wdc.com>
---
 include/grub/riscv32/linux.h | 15 ++++++++-------
 include/grub/riscv64/linux.h | 15 ++++++++-------
 2 files changed, 16 insertions(+), 14 deletions(-)

diff --git a/include/grub/riscv32/linux.h b/include/grub/riscv32/linux.h
index 512b777c8a41..de0dbdcd1be5 100644
--- a/include/grub/riscv32/linux.h
+++ b/include/grub/riscv32/linux.h
@@ -19,20 +19,21 @@
 #ifndef GRUB_RISCV32_LINUX_HEADER
 #define GRUB_RISCV32_LINUX_HEADER 1
 
-#define GRUB_LINUX_RISCV_MAGIC_SIGNATURE 0x52534356 /* 'RSCV' */
+#define GRUB_LINUX_RISCV_MAGIC_SIGNATURE 0x05435352 /* 'RSC\0x5' */
 
-/* From linux/Documentation/riscv/booting.txt */
+/* From linux/Documentation/riscv/boot-image-header.rst */
 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 text_offset;	/* Image load offset, little endian */
+  grub_uint64_t image_size;	/* Effective Image size, little endian */
+  grub_uint64_t flags;		/* kernel flags, little endian */
+  grub_uint32_t version;	/* Version of this header */
+  grub_uint32_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 magic;		/* Magic number, little endian, "RSC\x05" */
   grub_uint32_t hdr_offset;	/* Offset of PE/COFF header */
 };
 
diff --git a/include/grub/riscv64/linux.h b/include/grub/riscv64/linux.h
index 3630c30fbf1a..7c28bc92278e 100644
--- a/include/grub/riscv64/linux.h
+++ b/include/grub/riscv64/linux.h
@@ -19,22 +19,23 @@
 #ifndef GRUB_RISCV64_LINUX_HEADER
 #define GRUB_RISCV64_LINUX_HEADER 1
 
-#define GRUB_LINUX_RISCV_MAGIC_SIGNATURE 0x52534356 /* 'RSCV' */
+#define GRUB_LINUX_RISCV_MAGIC_SIGNATURE 0x05435352 /* 'RSC\0x5' */
 
 #define GRUB_EFI_PE_MAGIC	0x5A4D
 
-/* From linux/Documentation/riscv/booting.txt */
+/* From linux/Documentation/riscv/boot-image-header.rst */
 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 text_offset;	/* Image load offset, little endian */
+  grub_uint64_t image_size;	/* Effective Image size, little endian */
+  grub_uint64_t flags;		/* kernel flags, little endian */
+  grub_uint32_t version;	/* Version of this header */
+  grub_uint32_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 magic;		/* Magic number, little endian, "RSC\x05" */
   grub_uint32_t hdr_offset;	/* Offset of PE/COFF header */
 };
 
-- 
2.25.1

Re: [PATCH RFC/RFT 2/3] RISC-V: Update image header

[Heinrich Schuchardt]

On 4/26/20 9:40 PM, Atish Patra wrote:
> Update the RISC-V Linux kernel image headers as per the current header.
>
> Reference:
> <Linux kernel source>/Documentation/riscv/boot-image-header.rst
>
> Signed-off-by: Atish Patra <atish.patra@wdc.com>

Tested on Odroid-C2 (ARMv8) using GRUB master c543d678105037a plus this
patch series booting from U-Boot via GRUB to Debian Bullseye successfully.

Tested-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

> ---
>  include/grub/riscv32/linux.h | 15 ++++++++-------
>  include/grub/riscv64/linux.h | 15 ++++++++-------
>  2 files changed, 16 insertions(+), 14 deletions(-)
>
> diff --git a/include/grub/riscv32/linux.h b/include/grub/riscv32/linux.h
> index 512b777c8a41..de0dbdcd1be5 100644
> --- a/include/grub/riscv32/linux.h
> +++ b/include/grub/riscv32/linux.h
> @@ -19,20 +19,21 @@
>  #ifndef GRUB_RISCV32_LINUX_HEADER
>  #define GRUB_RISCV32_LINUX_HEADER 1
>
> -#define GRUB_LINUX_RISCV_MAGIC_SIGNATURE 0x52534356 /* 'RSCV' */
> +#define GRUB_LINUX_RISCV_MAGIC_SIGNATURE 0x05435352 /* 'RSC\0x5' */
>
> -/* From linux/Documentation/riscv/booting.txt */
> +/* From linux/Documentation/riscv/boot-image-header.rst */
>  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 text_offset;	/* Image load offset, little endian */
> +  grub_uint64_t image_size;	/* Effective Image size, little endian */
> +  grub_uint64_t flags;		/* kernel flags, little endian */
> +  grub_uint32_t version;	/* Version of this header */
> +  grub_uint32_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 magic;		/* Magic number, little endian, "RSC\x05" */
>    grub_uint32_t hdr_offset;	/* Offset of PE/COFF header */
>  };
>
> diff --git a/include/grub/riscv64/linux.h b/include/grub/riscv64/linux.h
> index 3630c30fbf1a..7c28bc92278e 100644
> --- a/include/grub/riscv64/linux.h
> +++ b/include/grub/riscv64/linux.h
> @@ -19,22 +19,23 @@
>  #ifndef GRUB_RISCV64_LINUX_HEADER
>  #define GRUB_RISCV64_LINUX_HEADER 1
>
> -#define GRUB_LINUX_RISCV_MAGIC_SIGNATURE 0x52534356 /* 'RSCV' */
> +#define GRUB_LINUX_RISCV_MAGIC_SIGNATURE 0x05435352 /* 'RSC\0x5' */
>
>  #define GRUB_EFI_PE_MAGIC	0x5A4D
>
> -/* From linux/Documentation/riscv/booting.txt */
> +/* From linux/Documentation/riscv/boot-image-header.rst */
>  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 text_offset;	/* Image load offset, little endian */
> +  grub_uint64_t image_size;	/* Effective Image size, little endian */
> +  grub_uint64_t flags;		/* kernel flags, little endian */
> +  grub_uint32_t version;	/* Version of this header */
> +  grub_uint32_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 magic;		/* Magic number, little endian, "RSC\x05" */
>    grub_uint32_t hdr_offset;	/* Offset of PE/COFF header */
>  };
>
>

[PATCH RFC/RFT 3/3] RISC-V: Use common linux loader

[Atish Patra]

RISC-V doesn't have to do anything very different from other architectures
to loader EFI stub linux kernel. As a result, just use the common linux
loader instead of defining a RISC-V specific linux loader.

Signed-off-by: Atish Patra <atish.patra@wdc.com>
---
 grub-core/Makefile.core.def    |  4 +--
 grub-core/loader/riscv/linux.c | 59 ----------------------------------
 include/grub/riscv32/linux.h   |  1 +
 include/grub/riscv64/linux.h   |  1 +
 4 files changed, 4 insertions(+), 61 deletions(-)
 delete mode 100644 grub-core/loader/riscv/linux.c

diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
index 57bb70d73764..8db90762af04 100644
--- a/grub-core/Makefile.core.def
+++ b/grub-core/Makefile.core.def
@@ -1816,8 +1816,8 @@ module = {
   arm_efi = loader/efi/linux.c;
   arm_uboot = loader/arm/linux.c;
   arm64 = loader/efi/linux.c;
-  riscv32 = loader/riscv/linux.c;
-  riscv64 = loader/riscv/linux.c;
+  riscv32 = loader/efi/linux.c;
+  riscv64 = loader/efi/linux.c;
   common = loader/linux.c;
   common = lib/cmdline.c;
   enable = noemu;
diff --git a/grub-core/loader/riscv/linux.c b/grub-core/loader/riscv/linux.c
deleted file mode 100644
index d17c488e118d..000000000000
--- a/grub-core/loader/riscv/linux.c
+++ /dev/null
@@ -1,59 +0,0 @@
-/*
- *  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/command.h>
-#include <grub/dl.h>
-#include <grub/lib/cmdline.h>
-
-GRUB_MOD_LICENSE ("GPLv3+");
-
-static grub_err_t
-grub_cmd_initrd (grub_command_t cmd __attribute__ ((unused)),
-		 int argc __attribute__ ((unused)),
-		 char *argv[] __attribute__ ((unused)))
-{
-  grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET, N_("Linux not supported yet"));
-
-  return grub_errno;
-}
-
-static grub_err_t
-grub_cmd_linux (grub_command_t cmd __attribute__ ((unused)),
-		int argc __attribute__ ((unused)),
-		char *argv[] __attribute__ ((unused)))
-{
-  grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET, N_("Linux not supported yet"));
-
-  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."));
-}
-
-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
index de0dbdcd1be5..706c69087546 100644
--- a/include/grub/riscv32/linux.h
+++ b/include/grub/riscv32/linux.h
@@ -38,5 +38,6 @@ struct linux_riscv_kernel_header
 };
 
 #define linux_arch_kernel_header linux_riscv_kernel_header
+# define GRUB_LINUX_ARCH_MAGIC_SIGNATURE GRUB_LINUX_RISCV_MAGIC_SIGNATURE
 
 #endif /* ! GRUB_RISCV32_LINUX_HEADER */
diff --git a/include/grub/riscv64/linux.h b/include/grub/riscv64/linux.h
index 7c28bc92278e..88d5df781899 100644
--- a/include/grub/riscv64/linux.h
+++ b/include/grub/riscv64/linux.h
@@ -40,5 +40,6 @@ struct linux_riscv_kernel_header
 };
 
 #define linux_arch_kernel_header linux_riscv_kernel_header
+# define GRUB_LINUX_ARCH_MAGIC_SIGNATURE GRUB_LINUX_RISCV_MAGIC_SIGNATURE
 
 #endif /* ! GRUB_RISCV64_LINUX_HEADER */
-- 
2.25.1

Re: [PATCH RFC/RFT 3/3] RISC-V: Use common linux loader

[Heinrich Schuchardt]

On 4/26/20 9:40 PM, Atish Patra wrote:
> RISC-V doesn't have to do anything very different from other architectures
> to loader EFI stub linux kernel. As a result, just use the common linux
> loader instead of defining a RISC-V specific linux loader.
>
> Signed-off-by: Atish Patra <atish.patra@wdc.com>

Tested on Odroid-C2 (ARMv8) using GRUB master c543d678105037a plus this
patch series booting from U-Boot via GRUB to Debian Bullseye successfully.

Tested-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

> ---
>  grub-core/Makefile.core.def    |  4 +--
>  grub-core/loader/riscv/linux.c | 59 ----------------------------------
>  include/grub/riscv32/linux.h   |  1 +
>  include/grub/riscv64/linux.h   |  1 +
>  4 files changed, 4 insertions(+), 61 deletions(-)
>  delete mode 100644 grub-core/loader/riscv/linux.c
>
> diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
> index 57bb70d73764..8db90762af04 100644
> --- a/grub-core/Makefile.core.def
> +++ b/grub-core/Makefile.core.def
> @@ -1816,8 +1816,8 @@ module = {
>    arm_efi = loader/efi/linux.c;
>    arm_uboot = loader/arm/linux.c;
>    arm64 = loader/efi/linux.c;
> -  riscv32 = loader/riscv/linux.c;
> -  riscv64 = loader/riscv/linux.c;
> +  riscv32 = loader/efi/linux.c;
> +  riscv64 = loader/efi/linux.c;
>    common = loader/linux.c;
>    common = lib/cmdline.c;
>    enable = noemu;
> diff --git a/grub-core/loader/riscv/linux.c b/grub-core/loader/riscv/linux.c
> deleted file mode 100644
> index d17c488e118d..000000000000
> --- a/grub-core/loader/riscv/linux.c
> +++ /dev/null
> @@ -1,59 +0,0 @@
> -/*
> - *  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/command.h>
> -#include <grub/dl.h>
> -#include <grub/lib/cmdline.h>
> -
> -GRUB_MOD_LICENSE ("GPLv3+");
> -
> -static grub_err_t
> -grub_cmd_initrd (grub_command_t cmd __attribute__ ((unused)),
> -		 int argc __attribute__ ((unused)),
> -		 char *argv[] __attribute__ ((unused)))
> -{
> -  grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET, N_("Linux not supported yet"));
> -
> -  return grub_errno;
> -}
> -
> -static grub_err_t
> -grub_cmd_linux (grub_command_t cmd __attribute__ ((unused)),
> -		int argc __attribute__ ((unused)),
> -		char *argv[] __attribute__ ((unused)))
> -{
> -  grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET, N_("Linux not supported yet"));
> -
> -  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."));
> -}
> -
> -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
> index de0dbdcd1be5..706c69087546 100644
> --- a/include/grub/riscv32/linux.h
> +++ b/include/grub/riscv32/linux.h
> @@ -38,5 +38,6 @@ struct linux_riscv_kernel_header
>  };
>
>  #define linux_arch_kernel_header linux_riscv_kernel_header
> +# define GRUB_LINUX_ARCH_MAGIC_SIGNATURE GRUB_LINUX_RISCV_MAGIC_SIGNATURE
>
>  #endif /* ! GRUB_RISCV32_LINUX_HEADER */
> diff --git a/include/grub/riscv64/linux.h b/include/grub/riscv64/linux.h
> index 7c28bc92278e..88d5df781899 100644
> --- a/include/grub/riscv64/linux.h
> +++ b/include/grub/riscv64/linux.h
> @@ -40,5 +40,6 @@ struct linux_riscv_kernel_header
>  };
>
>  #define linux_arch_kernel_header linux_riscv_kernel_header
> +# define GRUB_LINUX_ARCH_MAGIC_SIGNATURE GRUB_LINUX_RISCV_MAGIC_SIGNATURE
>
>  #endif /* ! GRUB_RISCV64_LINUX_HEADER */
>

Re: [PATCH RFC/RFT 0/3] Add grub loader support for RISC-V Linux

[Ard Biesheuvel]

On Sun, 26 Apr 2020 at 21:40, Atish Patra <atish.patra@wdc.com> wrote:
>
> This series adds grub loader support for RISC-V Linux. Thanks to the awesome
> initial RISC-V support added by Alex, we just needed a loader for RISC-V to
> load and execute Linux using UEFI protocol.
>
> Fortunately, ARM64 Linux loader is written in an architecture agnostic manner
> so thatgeneric RISC-V can easily reuse the loader code. Thus, the first patch
> just moves the ARM64 code to common code. I have compile tested for
> ARM64/ARM32. Even though it doesn't introduce any functional change
> for ARM/ARM64, any real testing will be helpful.
>

May I suggest that we not blindly adopt the ARM code here, but
instead, use the new initrd loading protocol that removes the need for
GRUB to modify or even know about the device tree at all?

The resulting code could serve as a legacy-free 'generic' EFI target,
that could work on all architectures, including x86, provided that the
kernel you are booting is recent enough (and that issue will solve
itself over time)


> I have tested this series for RISC-V on both Qemu and HiFive Unleashed.
> Here are the dependencies of other opensource projects.
>
> 1. OpenSBI v0.7
> 2. U-boot master (Head: a5f9b8a8b592 Merge https://gitlab.denx.de/u-boot/custodians/u-boot-riscv)
> 3. Linux kernel (efi-next + top 4 RISC-V patches from riscv-efi-for-v5.8)
>    Linux kernel tree can be found here as well.
>    https://github.com/atishp04/linux/pull/new/uefi_riscv_pr
>
>
> Atish Patra (3):
> loader: Move arm64 linux loader to common code
> RISC-V: Update image header
> RISC-V: Use common linux loader
>
> grub-core/Makefile.core.def             |  8 ++--
> grub-core/loader/{arm64 => efi}/linux.c |  2 +-
> grub-core/loader/riscv/linux.c          | 59 -------------------------
> include/grub/arm/linux.h                |  2 +-
> include/grub/arm64/linux.h              |  2 +-
> include/grub/riscv32/linux.h            | 16 ++++---
> include/grub/riscv64/linux.h            | 16 ++++---
> 7 files changed, 25 insertions(+), 80 deletions(-)
> rename grub-core/loader/{arm64 => efi}/linux.c (99%)
> delete mode 100644 grub-core/loader/riscv/linux.c
>
> --
> 2.25.1
>

Re: [PATCH RFC/RFT 0/3] Add grub loader support for RISC-V Linux

[Daniel Kiper]

On Mon, Apr 27, 2020 at 08:15:41AM +0200, Ard Biesheuvel wrote:
> On Sun, 26 Apr 2020 at 21:40, Atish Patra <atish.patra@wdc.com> wrote:
> >
> > This series adds grub loader support for RISC-V Linux. Thanks to the awesome
> > initial RISC-V support added by Alex, we just needed a loader for RISC-V to
> > load and execute Linux using UEFI protocol.
> >
> > Fortunately, ARM64 Linux loader is written in an architecture agnostic manner
> > so thatgeneric RISC-V can easily reuse the loader code. Thus, the first patch
> > just moves the ARM64 code to common code. I have compile tested for
> > ARM64/ARM32. Even though it doesn't introduce any functional change
> > for ARM/ARM64, any real testing will be helpful.
>
> May I suggest that we not blindly adopt the ARM code here, but
> instead, use the new initrd loading protocol that removes the need for
> GRUB to modify or even know about the device tree at all?
>
> The resulting code could serve as a legacy-free 'generic' EFI target,
> that could work on all architectures, including x86, provided that the
> kernel you are booting is recent enough (and that issue will solve
> itself over time)

I fully agree with Ard, this is way to go...

Daniel

Re: [PATCH RFC/RFT 0/3] Add grub loader support for RISC-V Linux

[Heinrich Schuchardt]

On 4/27/20 1:01 PM, Daniel Kiper wrote:
> On Mon, Apr 27, 2020 at 08:15:41AM +0200, Ard Biesheuvel wrote:
>> On Sun, 26 Apr 2020 at 21:40, Atish Patra <atish.patra@wdc.com> wrote:
>>>
>>> This series adds grub loader support for RISC-V Linux. Thanks to the awesome
>>> initial RISC-V support added by Alex, we just needed a loader for RISC-V to
>>> load and execute Linux using UEFI protocol.
>>>
>>> Fortunately, ARM64 Linux loader is written in an architecture agnostic manner
>>> so thatgeneric RISC-V can easily reuse the loader code. Thus, the first patch
>>> just moves the ARM64 code to common code. I have compile tested for
>>> ARM64/ARM32. Even though it doesn't introduce any functional change
>>> for ARM/ARM64, any real testing will be helpful.
>>
>> May I suggest that we not blindly adopt the ARM code here, but
>> instead, use the new initrd loading protocol that removes the need for
>> GRUB to modify or even know about the device tree at all?

Does this protocol exist in EDK2 by now?

In U-Boot there is a basic implementation which can provide a single
initrd image with a hardcoded file name. The file_path argument passed
to U-Boot is ignored due to Ilias' security concerns when he wrote the
patch.

GRUB is only needed if we have multiple kernels to choose from with
distinct initial ramdisks.

Please, describe what you expect the initrd loading protocol to do when
called from GRUB. How will the ramdisk fitting the kernel chosen in GRUB
be identified?

How do you deal with Ilias' security concerns expressed as follows in
U-boot commit ec80b4735a59 ("efi_loader: Implement FileLoad2 for
initramfs loading"):

"The file location is intentionally only supported as a config option
 argument(CONFIG_EFI_INITRD_FILESPEC), in an effort to enhance security.
Although U-boot is not responsible for verifying the integrity of the
initramfs, we can enhance the offered security by only accepting a
built-in option, which will be naturally verified by UEFI Secure Boot."

Best regards

Heinrich

>>
>> The resulting code could serve as a legacy-free 'generic' EFI target,
>> that could work on all architectures, including x86, provided that the
>> kernel you are booting is recent enough (and that issue will solve
>> itself over time)
>
> I fully agree with Ard, this is way to go...
>
> Daniel
>

Re: [PATCH RFC/RFT 0/3] Add grub loader support for RISC-V Linux

[Ard Biesheuvel]

On Mon, 27 Apr 2020 at 21:36, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 4/27/20 1:01 PM, Daniel Kiper wrote:
> > On Mon, Apr 27, 2020 at 08:15:41AM +0200, Ard Biesheuvel wrote:
> >> On Sun, 26 Apr 2020 at 21:40, Atish Patra <atish.patra@wdc.com> wrote:
> >>>
> >>> This series adds grub loader support for RISC-V Linux. Thanks to the awesome
> >>> initial RISC-V support added by Alex, we just needed a loader for RISC-V to
> >>> load and execute Linux using UEFI protocol.
> >>>
> >>> Fortunately, ARM64 Linux loader is written in an architecture agnostic manner
> >>> so thatgeneric RISC-V can easily reuse the loader code. Thus, the first patch
> >>> just moves the ARM64 code to common code. I have compile tested for
> >>> ARM64/ARM32. Even though it doesn't introduce any functional change
> >>> for ARM/ARM64, any real testing will be helpful.
> >>
> >> May I suggest that we not blindly adopt the ARM code here, but
> >> instead, use the new initrd loading protocol that removes the need for
> >> GRUB to modify or even know about the device tree at all?
>
> Does this protocol exist in EDK2 by now?
>

Yes. It exists as a shell command, and as a load option for OVMF.

> In U-Boot there is a basic implementation which can provide a single
> initrd image with a hardcoded file name. The file_path argument passed
> to U-Boot is ignored due to Ilias' security concerns when he wrote the
> patch.
>
> GRUB is only needed if we have multiple kernels to choose from with
> distinct initial ramdisks.
>
> Please, describe what you expect the initrd loading protocol to do when
> called from GRUB. How will the ramdisk fitting the kernel chosen in GRUB
> be identified?
>

The same what GRUB's 'initrd' command does. Whichever initrd you
select with it is the one that gets returned by the protocol.

> How do you deal with Ilias' security concerns expressed as follows in
> U-boot commit ec80b4735a59 ("efi_loader: Implement FileLoad2 for
> initramfs loading"):
>
> "The file location is intentionally only supported as a config option
>  argument(CONFIG_EFI_INITRD_FILESPEC), in an effort to enhance security.
> Although U-boot is not responsible for verifying the integrity of the
> initramfs, we can enhance the offered security by only accepting a
> built-in option, which will be naturally verified by UEFI Secure Boot."
>

That is an implementation detail of u-boot. This is one way to address
security concerns. Another way might be for U-Boot to check a
signature before it allows a file to be selected as the one to be
returned by the LoadFile2 protocol.

Re: [PATCH RFC/RFT 0/3] Add grub loader support for RISC-V Linux

[Heinrich Schuchardt]

Am April 27, 2020 7:39:38 PM UTC schrieb Ard Biesheuvel <ardb@kernel.org>:
>On Mon, 27 Apr 2020 at 21:36, Heinrich Schuchardt <xypron.glpk@gmx.de>
>wrote:
>>
>> On 4/27/20 1:01 PM, Daniel Kiper wrote:
>> > On Mon, Apr 27, 2020 at 08:15:41AM +0200, Ard Biesheuvel wrote:
>> >> On Sun, 26 Apr 2020 at 21:40, Atish Patra <atish.patra@wdc.com>
>wrote:
>> >>>
>> >>> This series adds grub loader support for RISC-V Linux. Thanks to
>the awesome
>> >>> initial RISC-V support added by Alex, we just needed a loader for
>RISC-V to
>> >>> load and execute Linux using UEFI protocol.
>> >>>
>> >>> Fortunately, ARM64 Linux loader is written in an architecture
>agnostic manner
>> >>> so thatgeneric RISC-V can easily reuse the loader code. Thus, the
>first patch
>> >>> just moves the ARM64 code to common code. I have compile tested
>for
>> >>> ARM64/ARM32. Even though it doesn't introduce any functional
>change
>> >>> for ARM/ARM64, any real testing will be helpful.
>> >>
>> >> May I suggest that we not blindly adopt the ARM code here, but
>> >> instead, use the new initrd loading protocol that removes the need
>for
>> >> GRUB to modify or even know about the device tree at all?
>>
>> Does this protocol exist in EDK2 by now?
>>
>
>Yes. It exists as a shell command, and as a load option for OVMF.
>
>> In U-Boot there is a basic implementation which can provide a single
>> initrd image with a hardcoded file name. The file_path argument
>passed
>> to U-Boot is ignored due to Ilias' security concerns when he wrote
>the
>> patch.
>>
>> GRUB is only needed if we have multiple kernels to choose from with
>> distinct initial ramdisks.
>>
>> Please, describe what you expect the initrd loading protocol to do
>when
>> called from GRUB. How will the ramdisk fitting the kernel chosen in
>GRUB
>> be identified?
>>
>
>The same what GRUB's 'initrd' command does. Whichever initrd you
>select with it is the one that gets returned by the protocol.

Will GRUB provide the absolute device path in parameter file_path?

>
>> How do you deal with Ilias' security concerns expressed as follows in
>> U-boot commit ec80b4735a59 ("efi_loader: Implement FileLoad2 for
>> initramfs loading"):
>>
>> "The file location is intentionally only supported as a config option
>>  argument(CONFIG_EFI_INITRD_FILESPEC), in an effort to enhance
>security.
>> Although U-boot is not responsible for verifying the integrity of the
>> initramfs, we can enhance the offered security by only accepting a
>> built-in option, which will be naturally verified by UEFI Secure
>Boot."
>>
>
>That is an implementation detail of u-boot. This is one way to address
>security concerns. Another way might be for U-Boot to check a
>signature before it allows a file to be selected as the one to be
>returned by the LoadFile2 protocol.

Re: [PATCH RFC/RFT 0/3] Add grub loader support for RISC-V Linux

[Ard Biesheuvel]

On Mon, 27 Apr 2020 at 22:47, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> Am April 27, 2020 7:39:38 PM UTC schrieb Ard Biesheuvel <ardb@kernel.org>:
> >On Mon, 27 Apr 2020 at 21:36, Heinrich Schuchardt <xypron.glpk@gmx.de>
> >wrote:
> >>
> >> On 4/27/20 1:01 PM, Daniel Kiper wrote:
> >> > On Mon, Apr 27, 2020 at 08:15:41AM +0200, Ard Biesheuvel wrote:
> >> >> On Sun, 26 Apr 2020 at 21:40, Atish Patra <atish.patra@wdc.com>
> >wrote:
> >> >>>
> >> >>> This series adds grub loader support for RISC-V Linux. Thanks to
> >the awesome
> >> >>> initial RISC-V support added by Alex, we just needed a loader for
> >RISC-V to
> >> >>> load and execute Linux using UEFI protocol.
> >> >>>
> >> >>> Fortunately, ARM64 Linux loader is written in an architecture
> >agnostic manner
> >> >>> so thatgeneric RISC-V can easily reuse the loader code. Thus, the
> >first patch
> >> >>> just moves the ARM64 code to common code. I have compile tested
> >for
> >> >>> ARM64/ARM32. Even though it doesn't introduce any functional
> >change
> >> >>> for ARM/ARM64, any real testing will be helpful.
> >> >>
> >> >> May I suggest that we not blindly adopt the ARM code here, but
> >> >> instead, use the new initrd loading protocol that removes the need
> >for
> >> >> GRUB to modify or even know about the device tree at all?
> >>
> >> Does this protocol exist in EDK2 by now?
> >>
> >
> >Yes. It exists as a shell command, and as a load option for OVMF.
> >
> >> In U-Boot there is a basic implementation which can provide a single
> >> initrd image with a hardcoded file name. The file_path argument
> >passed
> >> to U-Boot is ignored due to Ilias' security concerns when he wrote
> >the
> >> patch.
> >>
> >> GRUB is only needed if we have multiple kernels to choose from with
> >> distinct initial ramdisks.
> >>
> >> Please, describe what you expect the initrd loading protocol to do
> >when
> >> called from GRUB. How will the ramdisk fitting the kernel chosen in
> >GRUB
> >> be identified?
> >>
> >
> >The same what GRUB's 'initrd' command does. Whichever initrd you
> >select with it is the one that gets returned by the protocol.
>
> Will GRUB provide the absolute device path in parameter file_path?
>

Which parameter 'file_path" is that?

Re: [PATCH RFC/RFT 0/3] Add grub loader support for RISC-V Linux

[Ard Biesheuvel]

On Mon, 27 Apr 2020 at 22:47, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Mon, 27 Apr 2020 at 22:47, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >
> > Am April 27, 2020 7:39:38 PM UTC schrieb Ard Biesheuvel <ardb@kernel.org>:
> > >On Mon, 27 Apr 2020 at 21:36, Heinrich Schuchardt <xypron.glpk@gmx.de>
> > >wrote:
> > >>
> > >> On 4/27/20 1:01 PM, Daniel Kiper wrote:
> > >> > On Mon, Apr 27, 2020 at 08:15:41AM +0200, Ard Biesheuvel wrote:
> > >> >> On Sun, 26 Apr 2020 at 21:40, Atish Patra <atish.patra@wdc.com>
> > >wrote:
> > >> >>>
> > >> >>> This series adds grub loader support for RISC-V Linux. Thanks to
> > >the awesome
> > >> >>> initial RISC-V support added by Alex, we just needed a loader for
> > >RISC-V to
> > >> >>> load and execute Linux using UEFI protocol.
> > >> >>>
> > >> >>> Fortunately, ARM64 Linux loader is written in an architecture
> > >agnostic manner
> > >> >>> so thatgeneric RISC-V can easily reuse the loader code. Thus, the
> > >first patch
> > >> >>> just moves the ARM64 code to common code. I have compile tested
> > >for
> > >> >>> ARM64/ARM32. Even though it doesn't introduce any functional
> > >change
> > >> >>> for ARM/ARM64, any real testing will be helpful.
> > >> >>
> > >> >> May I suggest that we not blindly adopt the ARM code here, but
> > >> >> instead, use the new initrd loading protocol that removes the need
> > >for
> > >> >> GRUB to modify or even know about the device tree at all?
> > >>
> > >> Does this protocol exist in EDK2 by now?
> > >>
> > >
> > >Yes. It exists as a shell command, and as a load option for OVMF.
> > >
> > >> In U-Boot there is a basic implementation which can provide a single
> > >> initrd image with a hardcoded file name. The file_path argument
> > >passed
> > >> to U-Boot is ignored due to Ilias' security concerns when he wrote
> > >the
> > >> patch.
> > >>
> > >> GRUB is only needed if we have multiple kernels to choose from with
> > >> distinct initial ramdisks.
> > >>
> > >> Please, describe what you expect the initrd loading protocol to do
> > >when
> > >> called from GRUB. How will the ramdisk fitting the kernel chosen in
> > >GRUB
> > >> be identified?
> > >>
> > >
> > >The same what GRUB's 'initrd' command does. Whichever initrd you
> > >select with it is the one that gets returned by the protocol.
> >
> > Will GRUB provide the absolute device path in parameter file_path?
> >
>
> Which parameter 'file_path" is that?

Ah, I guess you mean the LoadFile2 argument? That is always
end-of-device-path in this case, since the initrd device path only
consists of the vendor media GUID.

The thing to keep in mind here is that the OS does not *choose* an
initrd, it simply loads the one that the bootloader has staged for it.

Re: [PATCH RFC/RFT 0/3] Add grub loader support for RISC-V Linux

[Heinrich Schuchardt]

Am April 27, 2020 8:52:43 PM UTC schrieb Ard Biesheuvel <ardb@kernel.org>:
>On Mon, 27 Apr 2020 at 22:47, Ard Biesheuvel <ardb@kernel.org> wrote:
>>
>> On Mon, 27 Apr 2020 at 22:47, Heinrich Schuchardt
><xypron.glpk@gmx.de> wrote:
>> >
>> > Am April 27, 2020 7:39:38 PM UTC schrieb Ard Biesheuvel
><ardb@kernel.org>:
>> > >On Mon, 27 Apr 2020 at 21:36, Heinrich Schuchardt
><xypron.glpk@gmx.de>
>> > >wrote:
>> > >>
>> > >> On 4/27/20 1:01 PM, Daniel Kiper wrote:
>> > >> > On Mon, Apr 27, 2020 at 08:15:41AM +0200, Ard Biesheuvel
>wrote:
>> > >> >> On Sun, 26 Apr 2020 at 21:40, Atish Patra
><atish.patra@wdc.com>
>> > >wrote:
>> > >> >>>
>> > >> >>> This series adds grub loader support for RISC-V Linux.
>Thanks to
>> > >the awesome
>> > >> >>> initial RISC-V support added by Alex, we just needed a
>loader for
>> > >RISC-V to
>> > >> >>> load and execute Linux using UEFI protocol.
>> > >> >>>
>> > >> >>> Fortunately, ARM64 Linux loader is written in an
>architecture
>> > >agnostic manner
>> > >> >>> so thatgeneric RISC-V can easily reuse the loader code.
>Thus, the
>> > >first patch
>> > >> >>> just moves the ARM64 code to common code. I have compile
>tested
>> > >for
>> > >> >>> ARM64/ARM32. Even though it doesn't introduce any functional
>> > >change
>> > >> >>> for ARM/ARM64, any real testing will be helpful.
>> > >> >>
>> > >> >> May I suggest that we not blindly adopt the ARM code here,
>but
>> > >> >> instead, use the new initrd loading protocol that removes the
>need
>> > >for
>> > >> >> GRUB to modify or even know about the device tree at all?
>> > >>
>> > >> Does this protocol exist in EDK2 by now?
>> > >>
>> > >
>> > >Yes. It exists as a shell command, and as a load option for OVMF.
>> > >
>> > >> In U-Boot there is a basic implementation which can provide a
>single
>> > >> initrd image with a hardcoded file name. The file_path argument
>> > >passed
>> > >> to U-Boot is ignored due to Ilias' security concerns when he
>wrote
>> > >the
>> > >> patch.
>> > >>
>> > >> GRUB is only needed if we have multiple kernels to choose from
>with
>> > >> distinct initial ramdisks.
>> > >>
>> > >> Please, describe what you expect the initrd loading protocol to
>do
>> > >when
>> > >> called from GRUB. How will the ramdisk fitting the kernel chosen
>in
>> > >GRUB
>> > >> be identified?
>> > >>
>> > >
>> > >The same what GRUB's 'initrd' command does. Whichever initrd you
>> > >select with it is the one that gets returned by the protocol.
>> >
>> > Will GRUB provide the absolute device path in parameter file_path?
>> >
>>
>> Which parameter 'file_path" is that?
>
>Ah, I guess you mean the LoadFile2 argument? That is always
>end-of-device-path in this case, since the initrd device path only
>consists of the vendor media GUID.
>
>The thing to keep in mind here is that the OS does not *choose* an
>initrd, it simply loads the one that the bootloader has staged for it.

How should U-Boot know which initrd fits the kernel chosen by the user in GRUB? That information exists in grub.cfg only.

If GRUB cannot provide this information, what is GRUB's added value in the boot process?

Re: [PATCH RFC/RFT 0/3] Add grub loader support for RISC-V Linux

[Heinrich Schuchardt]

Am April 27, 2020 8:58:57 PM UTC schrieb Heinrich Schuchardt <xypron.glpk@gmx.de>:
>Am April 27, 2020 8:52:43 PM UTC schrieb Ard Biesheuvel
><ardb@kernel.org>:
>>On Mon, 27 Apr 2020 at 22:47, Ard Biesheuvel <ardb@kernel.org> wrote:
>>>
>>> On Mon, 27 Apr 2020 at 22:47, Heinrich Schuchardt
>><xypron.glpk@gmx.de> wrote:
>>> >
>>> > Am April 27, 2020 7:39:38 PM UTC schrieb Ard Biesheuvel
>><ardb@kernel.org>:
>>> > >On Mon, 27 Apr 2020 at 21:36, Heinrich Schuchardt
>><xypron.glpk@gmx.de>
>>> > >wrote:
>>> > >>
>>> > >> On 4/27/20 1:01 PM, Daniel Kiper wrote:
>>> > >> > On Mon, Apr 27, 2020 at 08:15:41AM +0200, Ard Biesheuvel
>>wrote:
>>> > >> >> On Sun, 26 Apr 2020 at 21:40, Atish Patra
>><atish.patra@wdc.com>
>>> > >wrote:
>>> > >> >>>
>>> > >> >>> This series adds grub loader support for RISC-V Linux.
>>Thanks to
>>> > >the awesome
>>> > >> >>> initial RISC-V support added by Alex, we just needed a
>>loader for
>>> > >RISC-V to
>>> > >> >>> load and execute Linux using UEFI protocol.
>>> > >> >>>
>>> > >> >>> Fortunately, ARM64 Linux loader is written in an
>>architecture
>>> > >agnostic manner
>>> > >> >>> so thatgeneric RISC-V can easily reuse the loader code.
>>Thus, the
>>> > >first patch
>>> > >> >>> just moves the ARM64 code to common code. I have compile
>>tested
>>> > >for
>>> > >> >>> ARM64/ARM32. Even though it doesn't introduce any
>functional
>>> > >change
>>> > >> >>> for ARM/ARM64, any real testing will be helpful.
>>> > >> >>
>>> > >> >> May I suggest that we not blindly adopt the ARM code here,
>>but
>>> > >> >> instead, use the new initrd loading protocol that removes
>the
>>need
>>> > >for
>>> > >> >> GRUB to modify or even know about the device tree at all?
>>> > >>
>>> > >> Does this protocol exist in EDK2 by now?
>>> > >>
>>> > >
>>> > >Yes. It exists as a shell command, and as a load option for OVMF.
>>> > >
>>> > >> In U-Boot there is a basic implementation which can provide a
>>single
>>> > >> initrd image with a hardcoded file name. The file_path argument
>>> > >passed
>>> > >> to U-Boot is ignored due to Ilias' security concerns when he
>>wrote
>>> > >the
>>> > >> patch.
>>> > >>
>>> > >> GRUB is only needed if we have multiple kernels to choose from
>>with
>>> > >> distinct initial ramdisks.
>>> > >>
>>> > >> Please, describe what you expect the initrd loading protocol to
>>do
>>> > >when
>>> > >> called from GRUB. How will the ramdisk fitting the kernel
>chosen
>>in
>>> > >GRUB
>>> > >> be identified?
>>> > >>
>>> > >
>>> > >The same what GRUB's 'initrd' command does. Whichever initrd you
>>> > >select with it is the one that gets returned by the protocol.
>>> >
>>> > Will GRUB provide the absolute device path in parameter file_path?
>>> >
>>>
>>> Which parameter 'file_path" is that?
>>
>>Ah, I guess you mean the LoadFile2 argument? That is always
>>end-of-device-path in this case, since the initrd device path only
>>consists of the vendor media GUID.
>>
>>The thing to keep in mind here is that the OS does not *choose* an
>>initrd, it simply loads the one that the bootloader has staged for it.
>
>How should U-Boot know which initrd fits the kernel chosen by the user
>in GRUB? That information exists in grub.cfg only.
>
>If GRUB cannot provide this information, what is GRUB's added value in
>the boot process?

Hello Ard,

Did I misunderstand you and you want to provide a LOAD_FILE2 implementation in GRUB and not use the one in the firmware?

Regards

Heinrich

Re: [PATCH RFC/RFT 0/3] Add grub loader support for RISC-V Linux

[Ard Biesheuvel]

On Mon, 27 Apr 2020 at 23:16, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> Am April 27, 2020 8:58:57 PM UTC schrieb Heinrich Schuchardt <xypron.glpk@gmx.de>:
> >Am April 27, 2020 8:52:43 PM UTC schrieb Ard Biesheuvel
> ><ardb@kernel.org>:
> >>On Mon, 27 Apr 2020 at 22:47, Ard Biesheuvel <ardb@kernel.org> wrote:
> >>>
> >>> On Mon, 27 Apr 2020 at 22:47, Heinrich Schuchardt
> >><xypron.glpk@gmx.de> wrote:
> >>> >
> >>> > Am April 27, 2020 7:39:38 PM UTC schrieb Ard Biesheuvel
> >><ardb@kernel.org>:
> >>> > >On Mon, 27 Apr 2020 at 21:36, Heinrich Schuchardt
> >><xypron.glpk@gmx.de>
> >>> > >wrote:
> >>> > >>
> >>> > >> On 4/27/20 1:01 PM, Daniel Kiper wrote:
> >>> > >> > On Mon, Apr 27, 2020 at 08:15:41AM +0200, Ard Biesheuvel
> >>wrote:
> >>> > >> >> On Sun, 26 Apr 2020 at 21:40, Atish Patra
> >><atish.patra@wdc.com>
> >>> > >wrote:
> >>> > >> >>>
> >>> > >> >>> This series adds grub loader support for RISC-V Linux.
> >>Thanks to
> >>> > >the awesome
> >>> > >> >>> initial RISC-V support added by Alex, we just needed a
> >>loader for
> >>> > >RISC-V to
> >>> > >> >>> load and execute Linux using UEFI protocol.
> >>> > >> >>>
> >>> > >> >>> Fortunately, ARM64 Linux loader is written in an
> >>architecture
> >>> > >agnostic manner
> >>> > >> >>> so thatgeneric RISC-V can easily reuse the loader code.
> >>Thus, the
> >>> > >first patch
> >>> > >> >>> just moves the ARM64 code to common code. I have compile
> >>tested
> >>> > >for
> >>> > >> >>> ARM64/ARM32. Even though it doesn't introduce any
> >functional
> >>> > >change
> >>> > >> >>> for ARM/ARM64, any real testing will be helpful.
> >>> > >> >>
> >>> > >> >> May I suggest that we not blindly adopt the ARM code here,
> >>but
> >>> > >> >> instead, use the new initrd loading protocol that removes
> >the
> >>need
> >>> > >for
> >>> > >> >> GRUB to modify or even know about the device tree at all?
> >>> > >>
> >>> > >> Does this protocol exist in EDK2 by now?
> >>> > >>
> >>> > >
> >>> > >Yes. It exists as a shell command, and as a load option for OVMF.
> >>> > >
> >>> > >> In U-Boot there is a basic implementation which can provide a
> >>single
> >>> > >> initrd image with a hardcoded file name. The file_path argument
> >>> > >passed
> >>> > >> to U-Boot is ignored due to Ilias' security concerns when he
> >>wrote
> >>> > >the
> >>> > >> patch.
> >>> > >>
> >>> > >> GRUB is only needed if we have multiple kernels to choose from
> >>with
> >>> > >> distinct initial ramdisks.
> >>> > >>
> >>> > >> Please, describe what you expect the initrd loading protocol to
> >>do
> >>> > >when
> >>> > >> called from GRUB. How will the ramdisk fitting the kernel
> >chosen
> >>in
> >>> > >GRUB
> >>> > >> be identified?
> >>> > >>
> >>> > >
> >>> > >The same what GRUB's 'initrd' command does. Whichever initrd you
> >>> > >select with it is the one that gets returned by the protocol.
> >>> >
> >>> > Will GRUB provide the absolute device path in parameter file_path?
> >>> >
> >>>
> >>> Which parameter 'file_path" is that?
> >>
> >>Ah, I guess you mean the LoadFile2 argument? That is always
> >>end-of-device-path in this case, since the initrd device path only
> >>consists of the vendor media GUID.
> >>
> >>The thing to keep in mind here is that the OS does not *choose* an
> >>initrd, it simply loads the one that the bootloader has staged for it.
> >
> >How should U-Boot know which initrd fits the kernel chosen by the user
> >in GRUB? That information exists in grub.cfg only.
> >
> >If GRUB cannot provide this information, what is GRUB's added value in
> >the boot process?
>
> Hello Ard,
>
> Did I misunderstand you and you want to provide a LOAD_FILE2 implementation in GRUB and not use the one in the firmware?
>

Yes. If you use GRUB, it is provided by GRUB. Otherwise, it can be
provided by U-Boot or EDK2.

Re: [PATCH RFC/RFT 0/3] Add grub loader support for RISC-V Linux

[Atish Patra]

On Mon, Apr 27, 2020 at 3:11 PM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Mon, 27 Apr 2020 at 23:16, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >
> > Am April 27, 2020 8:58:57 PM UTC schrieb Heinrich Schuchardt <xypron.glpk@gmx.de>:
> > >Am April 27, 2020 8:52:43 PM UTC schrieb Ard Biesheuvel
> > ><ardb@kernel.org>:
> > >>On Mon, 27 Apr 2020 at 22:47, Ard Biesheuvel <ardb@kernel.org> wrote:
> > >>>
> > >>> On Mon, 27 Apr 2020 at 22:47, Heinrich Schuchardt
> > >><xypron.glpk@gmx.de> wrote:
> > >>> >
> > >>> > Am April 27, 2020 7:39:38 PM UTC schrieb Ard Biesheuvel
> > >><ardb@kernel.org>:
> > >>> > >On Mon, 27 Apr 2020 at 21:36, Heinrich Schuchardt
> > >><xypron.glpk@gmx.de>
> > >>> > >wrote:
> > >>> > >>
> > >>> > >> On 4/27/20 1:01 PM, Daniel Kiper wrote:
> > >>> > >> > On Mon, Apr 27, 2020 at 08:15:41AM +0200, Ard Biesheuvel
> > >>wrote:
> > >>> > >> >> On Sun, 26 Apr 2020 at 21:40, Atish Patra
> > >><atish.patra@wdc.com>
> > >>> > >wrote:
> > >>> > >> >>>
> > >>> > >> >>> This series adds grub loader support for RISC-V Linux.
> > >>Thanks to
> > >>> > >the awesome
> > >>> > >> >>> initial RISC-V support added by Alex, we just needed a
> > >>loader for
> > >>> > >RISC-V to
> > >>> > >> >>> load and execute Linux using UEFI protocol.
> > >>> > >> >>>
> > >>> > >> >>> Fortunately, ARM64 Linux loader is written in an
> > >>architecture
> > >>> > >agnostic manner
> > >>> > >> >>> so thatgeneric RISC-V can easily reuse the loader code.
> > >>Thus, the
> > >>> > >first patch
> > >>> > >> >>> just moves the ARM64 code to common code. I have compile
> > >>tested
> > >>> > >for
> > >>> > >> >>> ARM64/ARM32. Even though it doesn't introduce any
> > >functional
> > >>> > >change
> > >>> > >> >>> for ARM/ARM64, any real testing will be helpful.
> > >>> > >> >>
> > >>> > >> >> May I suggest that we not blindly adopt the ARM code here,
> > >>but
> > >>> > >> >> instead, use the new initrd loading protocol that removes
> > >the
> > >>need
> > >>> > >for
> > >>> > >> >> GRUB to modify or even know about the device tree at all?
> > >>> > >>
> > >>> > >> Does this protocol exist in EDK2 by now?
> > >>> > >>
> > >>> > >
> > >>> > >Yes. It exists as a shell command, and as a load option for OVMF.
> > >>> > >
> > >>> > >> In U-Boot there is a basic implementation which can provide a
> > >>single
> > >>> > >> initrd image with a hardcoded file name. The file_path argument
> > >>> > >passed
> > >>> > >> to U-Boot is ignored due to Ilias' security concerns when he
> > >>wrote
> > >>> > >the
> > >>> > >> patch.
> > >>> > >>
> > >>> > >> GRUB is only needed if we have multiple kernels to choose from
> > >>with
> > >>> > >> distinct initial ramdisks.
> > >>> > >>
> > >>> > >> Please, describe what you expect the initrd loading protocol to
> > >>do
> > >>> > >when
> > >>> > >> called from GRUB. How will the ramdisk fitting the kernel
> > >chosen
> > >>in
> > >>> > >GRUB
> > >>> > >> be identified?
> > >>> > >>
> > >>> > >
> > >>> > >The same what GRUB's 'initrd' command does. Whichever initrd you
> > >>> > >select with it is the one that gets returned by the protocol.
> > >>> >
> > >>> > Will GRUB provide the absolute device path in parameter file_path?
> > >>> >
> > >>>
> > >>> Which parameter 'file_path" is that?
> > >>
> > >>Ah, I guess you mean the LoadFile2 argument? That is always
> > >>end-of-device-path in this case, since the initrd device path only
> > >>consists of the vendor media GUID.
> > >>
> > >>The thing to keep in mind here is that the OS does not *choose* an
> > >>initrd, it simply loads the one that the bootloader has staged for it.
> > >
> > >How should U-Boot know which initrd fits the kernel chosen by the user
> > >in GRUB? That information exists in grub.cfg only.
> > >
> > >If GRUB cannot provide this information, what is GRUB's added value in
> > >the boot process?
> >
> > Hello Ard,
> >
> > Did I misunderstand you and you want to provide a LOAD_FILE2 implementation in GRUB and not use the one in the firmware?
> >
>
> Yes. If you use GRUB, it is provided by GRUB. Otherwise, it can be
> provided by U-Boot or EDK2.
>

Thanks for the discussion. I got clarification as well.
I will work on adding LOAD_FILE2 support in GRUB.

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



-- 
Regards,
Atish

Re: [PATCH RFC/RFT 0/3] Add grub loader support for RISC-V Linux

[Leif Lindholm]

Hi Atish,

On Tue, Apr 28, 2020 at 11:21:05 -0700, Atish Patra wrote:
> > > Hello Ard,
> > >
> > > Did I misunderstand you and you want to provide a LOAD_FILE2
> > > implementation in GRUB and not use the one in the firmware?
> >
> > Yes. If you use GRUB, it is provided by GRUB. Otherwise, it can be
> > provided by U-Boot or EDK2.
> 
> Thanks for the discussion. I got clarification as well.
> I will work on adding LOAD_FILE2 support in GRUB.

I had promised to look into that, but my paperwork with the FSF is
still pending. Feel free to contact me for bouncing ideas or whatever.

As for this set - the changes to the kernel header structs and naming
should be non-controversial, and will be required for a final unified
loader anyway. Could you possibly break those out as a separate set,
which could then be merged earlier?

Best Regards,

Leif

Re: [PATCH RFC/RFT 0/3] Add grub loader support for RISC-V Linux

[Atish Patra]

On Wed, Apr 29, 2020 at 4:22 AM Leif Lindholm <leif@nuviainc.com> wrote:
>
> Hi Atish,
>
> On Tue, Apr 28, 2020 at 11:21:05 -0700, Atish Patra wrote:
> > > > Hello Ard,
> > > >
> > > > Did I misunderstand you and you want to provide a LOAD_FILE2
> > > > implementation in GRUB and not use the one in the firmware?
> > >
> > > Yes. If you use GRUB, it is provided by GRUB. Otherwise, it can be
> > > provided by U-Boot or EDK2.
> >
> > Thanks for the discussion. I got clarification as well.
> > I will work on adding LOAD_FILE2 support in GRUB.
>
> I had promised to look into that, but my paperwork with the FSF is
> still pending. Feel free to contact me for bouncing ideas or whatever.
>

Sure.


> As for this set - the changes to the kernel header structs and naming
> should be non-controversial, and will be required for a final unified
> loader anyway. Could you possibly break those out as a separate set,
> which could then be merged earlier?
>

Ok. I will send out riscv header changes separately.

> Best Regards,
>
> Leif



--
Regards,
Atish

Re: [PATCH RFC/RFT 0/3] Add grub loader support for RISC-V Linux

[Heinrich Schuchardt]

Am April 27, 2020 8:47:51 PM UTC schrieb Ard Biesheuvel <ardb@kernel.org>:
>On Mon, 27 Apr 2020 at 22:47, Heinrich Schuchardt <xypron.glpk@gmx.de>
>wrote:
>>
>> Am April 27, 2020 7:39:38 PM UTC schrieb Ard Biesheuvel
><ardb@kernel.org>:
>> >On Mon, 27 Apr 2020 at 21:36, Heinrich Schuchardt
><xypron.glpk@gmx.de>
>> >wrote:
>> >>
>> >> On 4/27/20 1:01 PM, Daniel Kiper wrote:
>> >> > On Mon, Apr 27, 2020 at 08:15:41AM +0200, Ard Biesheuvel wrote:
>> >> >> On Sun, 26 Apr 2020 at 21:40, Atish Patra <atish.patra@wdc.com>
>> >wrote:
>> >> >>>
>> >> >>> This series adds grub loader support for RISC-V Linux. Thanks
>to
>> >the awesome
>> >> >>> initial RISC-V support added by Alex, we just needed a loader
>for
>> >RISC-V to
>> >> >>> load and execute Linux using UEFI protocol.
>> >> >>>
>> >> >>> Fortunately, ARM64 Linux loader is written in an architecture
>> >agnostic manner
>> >> >>> so thatgeneric RISC-V can easily reuse the loader code. Thus,
>the
>> >first patch
>> >> >>> just moves the ARM64 code to common code. I have compile
>tested
>> >for
>> >> >>> ARM64/ARM32. Even though it doesn't introduce any functional
>> >change
>> >> >>> for ARM/ARM64, any real testing will be helpful.
>> >> >>
>> >> >> May I suggest that we not blindly adopt the ARM code here, but
>> >> >> instead, use the new initrd loading protocol that removes the
>need
>> >for
>> >> >> GRUB to modify or even know about the device tree at all?
>> >>
>> >> Does this protocol exist in EDK2 by now?
>> >>
>> >
>> >Yes. It exists as a shell command, and as a load option for OVMF.
>> >
>> >> In U-Boot there is a basic implementation which can provide a
>single
>> >> initrd image with a hardcoded file name. The file_path argument
>> >passed
>> >> to U-Boot is ignored due to Ilias' security concerns when he wrote
>> >the
>> >> patch.
>> >>
>> >> GRUB is only needed if we have multiple kernels to choose from
>with
>> >> distinct initial ramdisks.
>> >>
>> >> Please, describe what you expect the initrd loading protocol to do
>> >when
>> >> called from GRUB. How will the ramdisk fitting the kernel chosen
>in
>> >GRUB
>> >> be identified?
>> >>
>> >
>> >The same what GRUB's 'initrd' command does. Whichever initrd you
>> >select with it is the one that gets returned by the protocol.
>>
>> Will GRUB provide the absolute device path in parameter file_path?
>>
>
>Which parameter 'file_path" is that?

See:

https://github.com/trini/u-boot/blob/master/lib/efi_loader/efi_load_initrd.c#L80

I hope we are talking about the same protocol.