efi: disentangle the generic EFI stub from FDT

Expose the EFI boot time memory map to the kernel via a configuration table. This is arch agnostic and enables future changes that remove the dependency on DT on architectures that don't otherwise rely on it.

Signed-off-by Ard Biesheuvel <[email protected]>

---
 drivers/firmware/efi/libstub/arm64-stub.c      |  2 +-
 drivers/firmware/efi/libstub/efi-stub-helper.c |  2 +-
 drivers/firmware/efi/libstub/efistub.h         |  3 ++-
 drivers/firmware/efi/libstub/mem.c             | 26 ++++++++++++++++++--
 drivers/firmware/efi/libstub/randomalloc.c     |  2 +-
 drivers/firmware/efi/libstub/relocate.c        |  2 +-
 include/linux/efi.h                            |  1 +
 7 files changed, 31 insertions(+), 7 deletions(-)

drivers/firmware/efi/libstub/mem.c +20 -2

index 40721573e494..ed4c145afe11 100644
@@ -9,14 +9,20 @@
  * efi_get_memory_map() - get memory map
  * @map:		pointer to memory map pointer to which to assign the
  *			newly allocated memory map
+ * @install_cfg_tbl:	whether or not to install the boot memory map as a
+ *			configuration table
  *
  * Retrieve the UEFI memory map. The allocated memory leaves room for
  * up to EFI_MMAP_NR_SLACK_SLOTS additional memory map entries.
  *
  * Return:	status code
  */
-efi_status_t efi_get_memory_map(struct efi_boot_memmap **map)
+efi_status_t efi_get_memory_map(struct efi_boot_memmap **map,
+				bool install_cfg_tbl)
 {
+	int memtype = install_cfg_tbl ? EFI_ACPI_RECLAIM_MEMORY
+				      : EFI_LOADER_DATA;
+	efi_guid_t tbl_guid = LINUX_EFI_BOOT_MEMMAP_GUID;
 	struct efi_boot_memmap *m, tmp;
 	efi_status_t status;
 	unsigned long size;

@@ -28,11 +34,23 @@ efi_status_t efi_get_memory_map(struct efi_boot_memmap **map)
 		return EFI_LOAD_ERROR;
 
 	size = tmp.map_size + tmp.desc_size * EFI_MMAP_NR_SLACK_SLOTS;
-	status = efi_bs_call(allocate_pool, EFI_LOADER_DATA, sizeof(*m) + size,
+	status = efi_bs_call(allocate_pool, memtype, sizeof(*m) + size,
 			     (void **)&m);
 	if (status != EFI_SUCCESS)
 		return status;
 
+	if (install_cfg_tbl) {
+		/*
+		 * Installing a configuration table might allocate memory, and
+		 * this may modify the memory map. This means we should install
+		 * the configuration table first, and re-install or delete it
+		 * as needed.
+		 */
+		status = efi_bs_call(install_configuration_table, &tbl_guid, m);
+		if (status != EFI_SUCCESS)
+			goto free_map;
+	}
+
 	m->buff_size = m->map_size = size;
 	status = efi_bs_call(get_memory_map, &m->map_size, m->map, &m->map_key,
 			     &m->desc_size, &m->desc_ver);

Hi Ard,

Quoted 15 lines · Ard Biesheuvel

Expose the EFI boot time memory map to the kernel via a configuration table. This is arch agnostic and enables future changes that remove the dependency on DT on architectures that don't otherwise rely on it.

Signed-off-by Ard Biesheuvel <[email protected]>

---
 drivers/firmware/efi/libstub/arm64-stub.c      |  2 +-
 drivers/firmware/efi/libstub/efi-stub-helper.c |  2 +-
 drivers/firmware/efi/libstub/efistub.h         |  3 ++-
 drivers/firmware/efi/libstub/mem.c             | 26 ++++++++++++++++++--
 drivers/firmware/efi/libstub/randomalloc.c     |  2 +-
 drivers/firmware/efi/libstub/relocate.c        |  2 +-
 include/linux/efi.h                            |  1 +
 7 files changed, 31 insertions(+), 7 deletions(-)

[..]

Quoted 52 lines

drivers/firmware/efi/libstub/mem.c +20 -2

index 40721573e494..ed4c145afe11 100644
@@ -9,14 +9,20 @@
  * efi_get_memory_map() - get memory map
  * @map:             pointer to memory map pointer to which to assign the
  *                   newly allocated memory map
+ * @install_cfg_tbl: whether or not to install the boot memory map as a
+ *                   configuration table
  *
  * Retrieve the UEFI memory map. The allocated memory leaves room for
  * up to EFI_MMAP_NR_SLACK_SLOTS additional memory map entries.
  *
  * Return:   status code
  */
-efi_status_t efi_get_memory_map(struct efi_boot_memmap **map)
+efi_status_t efi_get_memory_map(struct efi_boot_memmap **map,
+                             bool install_cfg_tbl)
 {
+     int memtype = install_cfg_tbl ? EFI_ACPI_RECLAIM_MEMORY
+                                   : EFI_LOADER_DATA;
+     efi_guid_t tbl_guid = LINUX_EFI_BOOT_MEMMAP_GUID;
      struct efi_boot_memmap *m, tmp;
      efi_status_t status;
      unsigned long size;

@@ -28,11 +34,23 @@ efi_status_t efi_get_memory_map(struct efi_boot_memmap **map)
              return EFI_LOAD_ERROR;
 
      size = tmp.map_size + tmp.desc_size * EFI_MMAP_NR_SLACK_SLOTS;
-     status = efi_bs_call(allocate_pool, EFI_LOADER_DATA, sizeof(*m) + size,
+     status = efi_bs_call(allocate_pool, memtype, sizeof(*m) + size,
                           (void **)&m);
      if (status != EFI_SUCCESS)
              return status;
 
+     if (install_cfg_tbl) {
+             /*
+              * Installing a configuration table might allocate memory, and
+              * this may modify the memory map. This means we should install
+              * the configuration table first, and re-install or delete it
+              * as needed.
+              */
+             status = efi_bs_call(install_configuration_table, &tbl_guid, m);
+             if (status != EFI_SUCCESS)
+                     goto free_map;
+     }
+
      m->buff_size = m->map_size = size;
      status = efi_bs_call(get_memory_map, &m->map_size, m->map, &m->map_key,
                           &m->desc_size, &m->desc_ver);

drivers/firmware/efi/libstub/mem.c efi_status_t efi_get_memory_map(struct efi_boot_memmap **map) · line 66

Quoted 9 lines

drivers/firmware/efi/libstub/mem.c +4 -0

@@ -40,6 +58,10 @@ efi_status_t efi_get_memory_map(struct efi_boot_memmap **map)
      if (status == EFI_SUCCESS) {
              *map = m;
      } else {
+             if (install_cfg_tbl)
+                     efi_bs_call(install_configuration_table, &tbl_guid,
+                                 NULL);
+free_map:
              efi_bs_call(free_pool, m);
      }

You have another commit about removing goto kludges, so maybe write this like the following, rather than a goto into an 'else' statement?

drivers/firmware/efi/libstub/mem.c +0 -1

index feef8d4be113..2f22ef7c5232 100644
@@ -64,10 +64,12 @@ efi_status_t efi_get_memory_map(struct efi_boot_memmap *map)
                        *map->key_ptr = key;
                if (map->desc_ver)
                        *map->desc_ver = desc_version;
-       } else {

LoongArch does not use FDT or DT natively [yet], and the only reason it currently uses it is so that it can reuse the existing EFI stub code.

Overloading the DT with data passed between the EFI stub and the core kernel has been a source of problems: there is the overlap between information provided by EFI which DT can also provide (initrd base/size, command line, memory descriptions), requiring us to reason about which is which and what to prioritize. It has also resulted in ABI leaks, i.e., internal ABI being promoted to external ABI inadvertently because the bootloader can set the EFI stub's DT properties as well (e.g., "kaslr-seed"). This has become especially problematic with boot environments that want to pretend that EFI boot is being done (to access ACPI and SMBIOS tables, for instance) but have no ability to execute the EFI stub, and so the environment that the EFI stub creates is emulated [poorly, in some cases].

Another downside of treating DT like this is that the DT binary that the kernel receives is different from the one created by the firmware, which is undesirable in the context of secure and measured boot.

Given that LoongArch support in Linux is brand new, we can avoid these pitfalls, and treat the DT strictly as a hardware description, and use a separate handover method between the EFI stub and the kernel. Now that initrd loading and passing the EFI memory map have been refactored into pure EFI routines that use EFI configuration tables, the only thing we need to pass directly is the kernel command line (even if we could pass this via a config table as well, it is used extremely early, so passing it directly is preferred in this case.)

Signed-off-by Ard Biesheuvel <[email protected]>

---
 arch/loongarch/Kconfig                        |  3 --
 arch/loongarch/include/asm/bootinfo.h         |  2 +-
 arch/loongarch/kernel/efi.c                   | 30 ++++++++++-
 arch/loongarch/kernel/env.c                   | 22 ++++----
 arch/loongarch/kernel/head.S                  |  2 +
 arch/loongarch/kernel/setup.c                 |  4 +-
 drivers/firmware/efi/libstub/Makefile         | 13 +++--
 drivers/firmware/efi/libstub/loongarch-stub.c | 56 +++++++++++++++++---
 8 files changed, 100 insertions(+), 32 deletions(-)

arch/loongarch/Kconfig +0 -3

index fca106a8b8af..14a2a1ec8561 100644
@@ -104,8 +104,6 @@ config LOONGARCH
        select MODULES_USE_ELF_RELA if MODULES
        select NEED_PER_CPU_EMBED_FIRST_CHUNK
        select NEED_PER_CPU_PAGE_FIRST_CHUNK
-       select OF
-       select OF_EARLY_FLATTREE
        select PCI
        select PCI_DOMAINS_GENERIC
        select PCI_ECAM if ACPI

@@ -311,7 +309,6 @@ config DMI
 config EFI
        bool "EFI runtime service support"
        select UCS2_STRING
-       select EFI_PARAMS_FROM_FDT
        select EFI_RUNTIME_WRAPPERS
        help
          This enables the kernel to use EFI runtime services that are

arch/loongarch/include/asm/bootinfo.h +1 -1

index e02ac4af7f6e..8e5881bc5ad1 100644
@@ -36,7 +36,7 @@ struct loongson_system_configuration {
 };
 
 extern u64 efi_system_table;
-extern unsigned long fw_arg0, fw_arg1;
+extern unsigned long fw_arg0, fw_arg1, fw_arg2;
 extern struct loongson_board_info b_info;
 extern struct loongson_system_configuration loongson_sysconf;
 

arch/loongarch/kernel/efi.c +29 -1

index 1f1f755fb425..3b80675726ec 100644
@@ -27,8 +27,13 @@
 static unsigned long efi_nr_tables;
 static unsigned long efi_config_table;
 
+static unsigned long __initdata boot_memmap = EFI_INVALID_TABLE_ADDR;
+
 static efi_system_table_t *efi_systab;
-static efi_config_table_type_t arch_tables[] __initdata = {{},};
+static efi_config_table_type_t arch_tables[] __initdata = {
+       {LINUX_EFI_BOOT_MEMMAP_GUID,    &boot_memmap,   "MEMMAP" },
+       {},
+};
 
 void __init efi_runtime_init(void)
 {

@@ -61,6 +66,8 @@ void __init efi_init(void)
                return;
        }
 
+       efi_systab_report_header(&efi_systab->hdr, efi_systab->fw_vendor);
+
        set_bit(EFI_64BIT, &efi.flags);
        efi_nr_tables    = efi_systab->nr_tables;
        efi_config_table = (unsigned long)efi_systab->tables;

@@ -72,4 +79,25 @@ void __init efi_init(void)
 
        if (screen_info.orig_video_isVGA == VIDEO_TYPE_EFI)
                memblock_reserve(screen_info.lfb_base, screen_info.lfb_size);
+
+       if (boot_memmap != EFI_INVALID_TABLE_ADDR) {
+               struct efi_memory_map_data data;
+               struct efi_boot_memmap *tbl;
+
+               tbl = early_memremap_ro(boot_memmap, sizeof(*tbl));
+               if (tbl) {
+                       data.phys_map           = boot_memmap + sizeof(*tbl);
+                       data.size               = tbl->map_size;
+                       data.desc_size          = tbl->desc_size;
+                       data.desc_version       = tbl->desc_ver;
+
+                       if (efi_memmap_init_early(&data) < 0)
+                               panic("Unable to map EFI memory map.\n");
+
+                       memblock_reserve(data.phys_map & PAGE_MASK,
+                                        PAGE_ALIGN(data.size + (data.phys_map & ~PAGE_MASK)));
+
+                       early_memunmap(tbl, sizeof(*tbl));
+               }
+       }
 }

arch/loongarch/kernel/env.c +9 -13

index 82b478a5c665..05c38d28476e 100644
@@ -8,7 +8,6 @@
 #include <linux/efi.h>
 #include <linux/export.h>
 #include <linux/memblock.h>
-#include <linux/of_fdt.h>
 #include <asm/early_ioremap.h>
 #include <asm/bootinfo.h>
 #include <asm/loongson.h>

@@ -20,21 +19,18 @@ EXPORT_SYMBOL(loongson_sysconf);
 void __init init_environ(void)
 {
        int efi_boot = fw_arg0;
-       struct efi_memory_map_data data;
-       void *fdt_ptr = early_memremap_ro(fw_arg1, SZ_64K);
 
-       if (efi_boot)
-               set_bit(EFI_BOOT, &efi.flags);
-       else
-               clear_bit(EFI_BOOT, &efi.flags);
+       if (efi_boot) {
+               char *cmdline = early_memremap_ro(fw_arg2, COMMAND_LINE_SIZE);
 
-       early_init_dt_scan(fdt_ptr);
-       early_init_fdt_reserve_self();
-       efi_system_table = efi_get_fdt_params(&data);
+               strscpy(boot_command_line, cmdline, COMMAND_LINE_SIZE);
+               early_memunmap(cmdline, COMMAND_LINE_SIZE);
 
-       efi_memmap_init_early(&data);
-       memblock_reserve(data.phys_map & PAGE_MASK,
-                        PAGE_ALIGN(data.size + (data.phys_map & ~PAGE_MASK)));
+               efi_system_table = fw_arg1;
+               set_bit(EFI_BOOT, &efi.flags);
+       } else {
+               clear_bit(EFI_BOOT, &efi.flags);
+       }
 }
 
 static int __init init_cpu_fullname(void)

arch/loongarch/kernel/head.S +2 -0

index 01bac62a6442..8f89f39fd31b 100644
@@ -67,6 +67,8 @@ SYM_CODE_START(kernel_entry)                  # kernel entry point
        st.d            a0, t0, 0               # firmware arguments
        la              t0, fw_arg1
        st.d            a1, t0, 0
+       la              t0, fw_arg2
+       st.d            a2, t0, 0
 
        /* KSave3 used for percpu base, initialized as 0 */
        csrwr           zero, PERCPU_BASE_KS

arch/loongarch/kernel/setup.c +2 -2

index e8714b1d94c8..7fabf2306e80 100644
@@ -51,7 +51,7 @@
 
 struct screen_info screen_info __section(".data");
 
-unsigned long fw_arg0, fw_arg1;
+unsigned long fw_arg0, fw_arg1, fw_arg2;
 DEFINE_PER_CPU(unsigned long, kernelsp);
 struct cpuinfo_loongarch cpu_data[NR_CPUS] __read_mostly;
 

@@ -187,7 +187,6 @@ early_param("mem", early_parse_mem);
 
 void __init platform_init(void)
 {
-       efi_init();
 #ifdef CONFIG_ACPI_TABLE_UPGRADE
        acpi_table_upgrade();
 #endif

@@ -347,6 +346,7 @@ void __init setup_arch(char **cmdline_p)
        *cmdline_p = boot_command_line;
 
        init_environ();
+       efi_init();
        memblock_init();
        parse_early_param();
 

drivers/firmware/efi/libstub/Makefile +8 -5

index 0dbc6d93f2e6..d8d6657e6277 100644
@@ -29,7 +29,7 @@ cflags-$(CONFIG_RISCV)                := $(subst $(CC_FLAGS_FTRACE),,$(KBUILD_CFLAGS)) \
 cflags-$(CONFIG_LOONGARCH)     := $(subst $(CC_FLAGS_FTRACE),,$(KBUILD_CFLAGS)) \
                                   -fpie
 
-cflags-$(CONFIG_EFI_GENERIC_STUB) += -I$(srctree)/scripts/dtc/libfdt
+cflags-$(CONFIG_EFI_PARAMS_FROM_FDT)   += -I$(srctree)/scripts/dtc/libfdt
 
 KBUILD_CFLAGS                  := $(cflags-y) -Os -DDISABLE_BRANCH_PROFILING \
                                   -include $(srctree)/include/linux/hidden.h \

@@ -60,14 +60,17 @@ lib-y                               := efi-stub-helper.o gop.o secureboot.o tpm.o \
                                   alignedmem.o relocate.o vsprintf.o \
                                   systable.o
 
-# include the stub's generic dependencies from lib/ when building for ARM/arm64
-efi-deps-y := fdt_rw.c fdt_ro.c fdt_wip.c fdt.c fdt_empty_tree.c fdt_sw.c
+# include the stub's libfdt dependencies from lib/ when needed
+libfdt-deps                    := fdt_rw.c fdt_ro.c fdt_wip.c fdt.c \
+                                  fdt_empty_tree.c fdt_sw.c
+
+lib-$(CONFIG_EFI_PARAMS_FROM_FDT) += fdt.o \
+                                    $(patsubst %.c,lib-%.o,$(libfdt-deps))
 
 $(obj)/lib-%.o: $(srctree)/lib/%.c FORCE
        $(call if_changed_rule,cc_o_c)
 
-lib-$(CONFIG_EFI_GENERIC_STUB) += efi-stub.o fdt.o string.o intrinsics.o \
-                                  $(patsubst %.c,lib-%.o,$(efi-deps-y))
+lib-$(CONFIG_EFI_GENERIC_STUB) += efi-stub.o string.o intrinsics.o
 
 lib-$(CONFIG_ARM)              += arm32-stub.o
 lib-$(CONFIG_ARM64)            += arm64-stub.o

drivers/firmware/efi/libstub/loongarch-stub.c +49 -8

index b7ef8d2df59e..7c684d10f936 100644
@@ -9,7 +9,8 @@
 #include <asm/addrspace.h>
 #include "efistub.h"
 
-typedef void __noreturn (*kernel_entry_t)(bool efi, unsigned long fdt);
+typedef void __noreturn (*kernel_entry_t)(bool efi, unsigned long systab,
+                                         unsigned long cmdline);
 
 extern int kernel_asize;
 extern int kernel_fsize;

@@ -42,19 +43,60 @@ efi_status_t handle_kernel_image(unsigned long *image_addr,
        return status;
 }
 
-void __noreturn efi_enter_kernel(unsigned long entrypoint, unsigned long fdt, unsigned long fdt_size)
+struct exit_boot_struct {
+       efi_memory_desc_t       *runtime_map;
+       int                     runtime_entry_count;
+};
+
+static efi_status_t exit_boot_func(struct efi_boot_memmap *map, void *priv)
+{
+       struct exit_boot_struct *p = priv;
+
+       /*
+        * Update the memory map with virtual addresses. The function will also
+        * populate @runtime_map with copies of just the EFI_MEMORY_RUNTIME
+        * entries so that we can pass it straight to SetVirtualAddressMap()
+        */
+       efi_get_virtmap(map->map, map->map_size, map->desc_size,
+                       p->runtime_map, &p->runtime_entry_count);
+
+       return EFI_SUCCESS;
+}
+
+efi_status_t efi_boot_kernel(void *handle, efi_loaded_image_t *image,
+                            unsigned long image_addr, char *cmdline_ptr)
 {
        kernel_entry_t real_kernel_entry;
+       struct exit_boot_struct priv;
+       unsigned long desc_size;
+       efi_status_t status;
+       u32 desc_ver;
+
+       status = efi_alloc_virtmap(&priv.runtime_map, &desc_size, &desc_ver);
+       if (status != EFI_SUCCESS) {
+               efi_err("Unable to retrieve UEFI memory map.\n");
+               return status;
+       }
+
+       efi_info("Exiting boot services\n");
+
+       efi_novamap = false;
+       status = efi_exit_boot_services(handle, &priv, exit_boot_func);
+       if (status != EFI_SUCCESS)
+               return status;
+
+       /* Install the new virtual address map */
+       efi_rt_call(set_virtual_address_map,
+                   priv.runtime_entry_count * desc_size, desc_size,
+                   desc_ver, priv.runtime_map);
 
        /* Config Direct Mapping */
        csr_write64(CSR_DMW0_INIT, LOONGARCH_CSR_DMWIN0);
        csr_write64(CSR_DMW1_INIT, LOONGARCH_CSR_DMWIN1);
 
        real_kernel_entry = (kernel_entry_t)
-               ((unsigned long)&kernel_entry - entrypoint + VMLINUX_LOAD_ADDRESS);
+               ((unsigned long)&kernel_entry - image_addr + VMLINUX_LOAD_ADDRESS);
 
-       if (!efi_novamap)
-               real_kernel_entry(true, fdt);
-       else
-               real_kernel_entry(false, fdt);
+       real_kernel_entry(true, (unsigned long)efi_system_table,
+                         (unsigned long)cmdline_ptr);
 }
--

2.35.1

Hi, Ard,

I think the parameters passed to the core kernel need to be discussed. The old way (so-called old world):
a0=argc, a1=argv, a1=bpi

The current way (so-called new world):
a0=efi boot flag, a1=fdt pointer

The new way (in this patchset):
a0=efi boot flag, a1=systemtable, a2=cmdline

I prefer to use the current way, there are 3 reasons:
1, both acpi system and dt system can use the same parameters passing method;

2, arm64, riscv and loongarch can use similar logics (light FDT);

3, out-of-tree patches can make compatibility with the old world easier by just judging whether a2 is zero.

Quoted 335 lines

LoongArch does not use FDT or DT natively [yet], and the only reason it currently uses it is so that it can reuse the existing EFI stub code.

Overloading the DT with data passed between the EFI stub and the core kernel has been a source of problems: there is the overlap between information provided by EFI which DT can also provide (initrd base/size, command line, memory descriptions), requiring us to reason about which is which and what to prioritize. It has also resulted in ABI leaks, i.e., internal ABI being promoted to external ABI inadvertently because the bootloader can set the EFI stub's DT properties as well (e.g., "kaslr-seed"). This has become especially problematic with boot environments that want to pretend that EFI boot is being done (to access ACPI and SMBIOS tables, for instance) but have no ability to execute the EFI stub, and so the environment that the EFI stub creates is emulated [poorly, in some cases].

Another downside of treating DT like this is that the DT binary that the kernel receives is different from the one created by the firmware, which is undesirable in the context of secure and measured boot.

Given that LoongArch support in Linux is brand new, we can avoid these pitfalls, and treat the DT strictly as a hardware description, and use a separate handover method between the EFI stub and the kernel. Now that initrd loading and passing the EFI memory map have been refactored into pure EFI routines that use EFI configuration tables, the only thing we need to pass directly is the kernel command line (even if we could pass this via a config table as well, it is used extremely early, so passing it directly is preferred in this case.)

Signed-off-by Ard Biesheuvel <[email protected]>

---
 arch/loongarch/Kconfig                        |  3 --
 arch/loongarch/include/asm/bootinfo.h         |  2 +-
 arch/loongarch/kernel/efi.c                   | 30 ++++++++++-
 arch/loongarch/kernel/env.c                   | 22 ++++----
 arch/loongarch/kernel/head.S                  |  2 +
 arch/loongarch/kernel/setup.c                 |  4 +-
 drivers/firmware/efi/libstub/Makefile         | 13 +++--
 drivers/firmware/efi/libstub/loongarch-stub.c | 56 +++++++++++++++++---
 8 files changed, 100 insertions(+), 32 deletions(-)

arch/loongarch/Kconfig +0 -3

index fca106a8b8af..14a2a1ec8561 100644
@@ -104,8 +104,6 @@ config LOONGARCH
        select MODULES_USE_ELF_RELA if MODULES
        select NEED_PER_CPU_EMBED_FIRST_CHUNK
        select NEED_PER_CPU_PAGE_FIRST_CHUNK
-       select OF
-       select OF_EARLY_FLATTREE
        select PCI
        select PCI_DOMAINS_GENERIC
        select PCI_ECAM if ACPI

@@ -311,7 +309,6 @@ config DMI
 config EFI
        bool "EFI runtime service support"
        select UCS2_STRING
-       select EFI_PARAMS_FROM_FDT
        select EFI_RUNTIME_WRAPPERS
        help
          This enables the kernel to use EFI runtime services that are

arch/loongarch/include/asm/bootinfo.h +1 -1

index e02ac4af7f6e..8e5881bc5ad1 100644
@@ -36,7 +36,7 @@ struct loongson_system_configuration {
 };
 
 extern u64 efi_system_table;
-extern unsigned long fw_arg0, fw_arg1;
+extern unsigned long fw_arg0, fw_arg1, fw_arg2;
 extern struct loongson_board_info b_info;
 extern struct loongson_system_configuration loongson_sysconf;
 

arch/loongarch/kernel/efi.c +29 -1

index 1f1f755fb425..3b80675726ec 100644
@@ -27,8 +27,13 @@
 static unsigned long efi_nr_tables;
 static unsigned long efi_config_table;
 
+static unsigned long __initdata boot_memmap = EFI_INVALID_TABLE_ADDR;
+
 static efi_system_table_t *efi_systab;
-static efi_config_table_type_t arch_tables[] __initdata = {{},};
+static efi_config_table_type_t arch_tables[] __initdata = {
+       {LINUX_EFI_BOOT_MEMMAP_GUID,    &boot_memmap,   "MEMMAP" },
+       {},
+};
 
 void __init efi_runtime_init(void)
 {

@@ -61,6 +66,8 @@ void __init efi_init(void)
                return;
        }
 
+       efi_systab_report_header(&efi_systab->hdr, efi_systab->fw_vendor);
+
        set_bit(EFI_64BIT, &efi.flags);
        efi_nr_tables    = efi_systab->nr_tables;
        efi_config_table = (unsigned long)efi_systab->tables;

@@ -72,4 +79,25 @@ void __init efi_init(void)
 
        if (screen_info.orig_video_isVGA == VIDEO_TYPE_EFI)
                memblock_reserve(screen_info.lfb_base, screen_info.lfb_size);
+
+       if (boot_memmap != EFI_INVALID_TABLE_ADDR) {
+               struct efi_memory_map_data data;
+               struct efi_boot_memmap *tbl;
+
+               tbl = early_memremap_ro(boot_memmap, sizeof(*tbl));
+               if (tbl) {
+                       data.phys_map           = boot_memmap + sizeof(*tbl);
+                       data.size               = tbl->map_size;
+                       data.desc_size          = tbl->desc_size;
+                       data.desc_version       = tbl->desc_ver;
+
+                       if (efi_memmap_init_early(&data) < 0)
+                               panic("Unable to map EFI memory map.\n");
+
+                       memblock_reserve(data.phys_map & PAGE_MASK,
+                                        PAGE_ALIGN(data.size + (data.phys_map & ~PAGE_MASK)));
+
+                       early_memunmap(tbl, sizeof(*tbl));
+               }
+       }
 }

arch/loongarch/kernel/env.c +9 -13

index 82b478a5c665..05c38d28476e 100644
@@ -8,7 +8,6 @@
 #include <linux/efi.h>
 #include <linux/export.h>
 #include <linux/memblock.h>
-#include <linux/of_fdt.h>
 #include <asm/early_ioremap.h>
 #include <asm/bootinfo.h>
 #include <asm/loongson.h>

@@ -20,21 +19,18 @@ EXPORT_SYMBOL(loongson_sysconf);
 void __init init_environ(void)
 {
        int efi_boot = fw_arg0;
-       struct efi_memory_map_data data;
-       void *fdt_ptr = early_memremap_ro(fw_arg1, SZ_64K);
 
-       if (efi_boot)
-               set_bit(EFI_BOOT, &efi.flags);
-       else
-               clear_bit(EFI_BOOT, &efi.flags);
+       if (efi_boot) {
+               char *cmdline = early_memremap_ro(fw_arg2, COMMAND_LINE_SIZE);
 
-       early_init_dt_scan(fdt_ptr);
-       early_init_fdt_reserve_self();
-       efi_system_table = efi_get_fdt_params(&data);
+               strscpy(boot_command_line, cmdline, COMMAND_LINE_SIZE);
+               early_memunmap(cmdline, COMMAND_LINE_SIZE);
 
-       efi_memmap_init_early(&data);
-       memblock_reserve(data.phys_map & PAGE_MASK,
-                        PAGE_ALIGN(data.size + (data.phys_map & ~PAGE_MASK)));
+               efi_system_table = fw_arg1;
+               set_bit(EFI_BOOT, &efi.flags);
+       } else {
+               clear_bit(EFI_BOOT, &efi.flags);
+       }
 }
 
 static int __init init_cpu_fullname(void)

arch/loongarch/kernel/head.S +2 -0

index 01bac62a6442..8f89f39fd31b 100644
@@ -67,6 +67,8 @@ SYM_CODE_START(kernel_entry)                  # kernel entry point
        st.d            a0, t0, 0               # firmware arguments
        la              t0, fw_arg1
        st.d            a1, t0, 0
+       la              t0, fw_arg2
+       st.d            a2, t0, 0
 
        /* KSave3 used for percpu base, initialized as 0 */
        csrwr           zero, PERCPU_BASE_KS

arch/loongarch/kernel/setup.c +2 -2

index e8714b1d94c8..7fabf2306e80 100644
@@ -51,7 +51,7 @@
 
 struct screen_info screen_info __section(".data");
 
-unsigned long fw_arg0, fw_arg1;
+unsigned long fw_arg0, fw_arg1, fw_arg2;
 DEFINE_PER_CPU(unsigned long, kernelsp);
 struct cpuinfo_loongarch cpu_data[NR_CPUS] __read_mostly;
 

@@ -187,7 +187,6 @@ early_param("mem", early_parse_mem);
 
 void __init platform_init(void)
 {
-       efi_init();
 #ifdef CONFIG_ACPI_TABLE_UPGRADE
        acpi_table_upgrade();
 #endif

@@ -347,6 +346,7 @@ void __init setup_arch(char **cmdline_p)
        *cmdline_p = boot_command_line;
 
        init_environ();
+       efi_init();
        memblock_init();
        parse_early_param();
 

drivers/firmware/efi/libstub/Makefile +8 -5

index 0dbc6d93f2e6..d8d6657e6277 100644
@@ -29,7 +29,7 @@ cflags-$(CONFIG_RISCV)                := $(subst $(CC_FLAGS_FTRACE),,$(KBUILD_CFLAGS)) \
 cflags-$(CONFIG_LOONGARCH)     := $(subst $(CC_FLAGS_FTRACE),,$(KBUILD_CFLAGS)) \
                                   -fpie
 
-cflags-$(CONFIG_EFI_GENERIC_STUB) += -I$(srctree)/scripts/dtc/libfdt
+cflags-$(CONFIG_EFI_PARAMS_FROM_FDT)   += -I$(srctree)/scripts/dtc/libfdt
 
 KBUILD_CFLAGS                  := $(cflags-y) -Os -DDISABLE_BRANCH_PROFILING \
                                   -include $(srctree)/include/linux/hidden.h \

@@ -60,14 +60,17 @@ lib-y                               := efi-stub-helper.o gop.o secureboot.o tpm.o \
                                   alignedmem.o relocate.o vsprintf.o \
                                   systable.o
 
-# include the stub's generic dependencies from lib/ when building for ARM/arm64
-efi-deps-y := fdt_rw.c fdt_ro.c fdt_wip.c fdt.c fdt_empty_tree.c fdt_sw.c
+# include the stub's libfdt dependencies from lib/ when needed
+libfdt-deps                    := fdt_rw.c fdt_ro.c fdt_wip.c fdt.c \
+                                  fdt_empty_tree.c fdt_sw.c
+
+lib-$(CONFIG_EFI_PARAMS_FROM_FDT) += fdt.o \
+                                    $(patsubst %.c,lib-%.o,$(libfdt-deps))
 
 $(obj)/lib-%.o: $(srctree)/lib/%.c FORCE
        $(call if_changed_rule,cc_o_c)
 
-lib-$(CONFIG_EFI_GENERIC_STUB) += efi-stub.o fdt.o string.o intrinsics.o \
-                                  $(patsubst %.c,lib-%.o,$(efi-deps-y))
+lib-$(CONFIG_EFI_GENERIC_STUB) += efi-stub.o string.o intrinsics.o
 
 lib-$(CONFIG_ARM)              += arm32-stub.o
 lib-$(CONFIG_ARM64)            += arm64-stub.o

drivers/firmware/efi/libstub/loongarch-stub.c +49 -8

index b7ef8d2df59e..7c684d10f936 100644
@@ -9,7 +9,8 @@
 #include <asm/addrspace.h>
 #include "efistub.h"
 
-typedef void __noreturn (*kernel_entry_t)(bool efi, unsigned long fdt);
+typedef void __noreturn (*kernel_entry_t)(bool efi, unsigned long systab,
+                                         unsigned long cmdline);
 
 extern int kernel_asize;
 extern int kernel_fsize;

@@ -42,19 +43,60 @@ efi_status_t handle_kernel_image(unsigned long *image_addr,
        return status;
 }
 
-void __noreturn efi_enter_kernel(unsigned long entrypoint, unsigned long fdt, unsigned long fdt_size)
+struct exit_boot_struct {
+       efi_memory_desc_t       *runtime_map;
+       int                     runtime_entry_count;
+};
+
+static efi_status_t exit_boot_func(struct efi_boot_memmap *map, void *priv)
+{
+       struct exit_boot_struct *p = priv;
+
+       /*
+        * Update the memory map with virtual addresses. The function will also
+        * populate @runtime_map with copies of just the EFI_MEMORY_RUNTIME
+        * entries so that we can pass it straight to SetVirtualAddressMap()
+        */
+       efi_get_virtmap(map->map, map->map_size, map->desc_size,
+                       p->runtime_map, &p->runtime_entry_count);
+
+       return EFI_SUCCESS;
+}
+
+efi_status_t efi_boot_kernel(void *handle, efi_loaded_image_t *image,
+                            unsigned long image_addr, char *cmdline_ptr)
 {
        kernel_entry_t real_kernel_entry;
+       struct exit_boot_struct priv;
+       unsigned long desc_size;
+       efi_status_t status;
+       u32 desc_ver;
+
+       status = efi_alloc_virtmap(&priv.runtime_map, &desc_size, &desc_ver);
+       if (status != EFI_SUCCESS) {
+               efi_err("Unable to retrieve UEFI memory map.\n");
+               return status;
+       }
+
+       efi_info("Exiting boot services\n");
+
+       efi_novamap = false;
+       status = efi_exit_boot_services(handle, &priv, exit_boot_func);
+       if (status != EFI_SUCCESS)
+               return status;
+
+       /* Install the new virtual address map */
+       efi_rt_call(set_virtual_address_map,
+                   priv.runtime_entry_count * desc_size, desc_size,
+                   desc_ver, priv.runtime_map);
 
        /* Config Direct Mapping */
        csr_write64(CSR_DMW0_INIT, LOONGARCH_CSR_DMWIN0);
        csr_write64(CSR_DMW1_INIT, LOONGARCH_CSR_DMWIN1);
 
        real_kernel_entry = (kernel_entry_t)
-               ((unsigned long)&kernel_entry - entrypoint + VMLINUX_LOAD_ADDRESS);
+               ((unsigned long)&kernel_entry - image_addr + VMLINUX_LOAD_ADDRESS);
 
-       if (!efi_novamap)
-               real_kernel_entry(true, fdt);
-       else
-               real_kernel_entry(false, fdt);
+       real_kernel_entry(true, (unsigned long)efi_system_table,
+                         (unsigned long)cmdline_ptr);
 }
--

2.35.1

Quoted 13 lines · Huacai Chen

Hi, Ard,

I think the parameters passed to the core kernel need to be discussed. The old way (so-called old world):
a0=argc, a1=argv, a1=bpi

The current way (so-called new world):
a0=efi boot flag, a1=fdt pointer

The new way (in this patchset):
a0=efi boot flag, a1=systemtable, a2=cmdline

Quoted 2 lines

I prefer to use the current way, there are 3 reasons:
1, both acpi system and dt system can use the same parameters passing method;

DT systems will use this too. The distinction is between EFI boot and non-EFI boot. We *really* should keep these separate, given the experience on ARM, where other projects invent ways to pass those values to the kernel without going through the stub.

Quoted 2 lines

2, arm64, riscv and loongarch can use similar logics (light FDT);

No need to repeat a mistake. I intend to fix RISC-V next.

Quoted 3 lines

3, out-of-tree patches can make compatibility with the old world easier by just judging whether a2 is zero.

The whole point of this series is that the EFI stub should not be touching the DT at all. In other words, there is no DT pointer, so the current method needs to be revised.

What we might do is

a0=systemtable, a1=cmdline

as any non-zero value is treated as logic true. That way, your logic to test a2 is zero will still work.

Quoted 339 lines · Ard Biesheuvel

Quoted 335 lines

LoongArch does not use FDT or DT natively [yet], and the only reason it currently uses it is so that it can reuse the existing EFI stub code.

Overloading the DT with data passed between the EFI stub and the core kernel has been a source of problems: there is the overlap between information provided by EFI which DT can also provide (initrd base/size, command line, memory descriptions), requiring us to reason about which is which and what to prioritize. It has also resulted in ABI leaks, i.e., internal ABI being promoted to external ABI inadvertently because the bootloader can set the EFI stub's DT properties as well (e.g., "kaslr-seed"). This has become especially problematic with boot environments that want to pretend that EFI boot is being done (to access ACPI and SMBIOS tables, for instance) but have no ability to execute the EFI stub, and so the environment that the EFI stub creates is emulated [poorly, in some cases].

Another downside of treating DT like this is that the DT binary that the kernel receives is different from the one created by the firmware, which is undesirable in the context of secure and measured boot.

Given that LoongArch support in Linux is brand new, we can avoid these pitfalls, and treat the DT strictly as a hardware description, and use a separate handover method between the EFI stub and the kernel. Now that initrd loading and passing the EFI memory map have been refactored into pure EFI routines that use EFI configuration tables, the only thing we need to pass directly is the kernel command line (even if we could pass this via a config table as well, it is used extremely early, so passing it directly is preferred in this case.)

Signed-off-by Ard Biesheuvel <[email protected]>

---
 arch/loongarch/Kconfig                        |  3 --
 arch/loongarch/include/asm/bootinfo.h         |  2 +-
 arch/loongarch/kernel/efi.c                   | 30 ++++++++++-
 arch/loongarch/kernel/env.c                   | 22 ++++----
 arch/loongarch/kernel/head.S                  |  2 +
 arch/loongarch/kernel/setup.c                 |  4 +-
 drivers/firmware/efi/libstub/Makefile         | 13 +++--
 drivers/firmware/efi/libstub/loongarch-stub.c | 56 +++++++++++++++++---
 8 files changed, 100 insertions(+), 32 deletions(-)

arch/loongarch/Kconfig +0 -3

index fca106a8b8af..14a2a1ec8561 100644
@@ -104,8 +104,6 @@ config LOONGARCH
        select MODULES_USE_ELF_RELA if MODULES
        select NEED_PER_CPU_EMBED_FIRST_CHUNK
        select NEED_PER_CPU_PAGE_FIRST_CHUNK
-       select OF
-       select OF_EARLY_FLATTREE
        select PCI
        select PCI_DOMAINS_GENERIC
        select PCI_ECAM if ACPI

@@ -311,7 +309,6 @@ config DMI
 config EFI
        bool "EFI runtime service support"
        select UCS2_STRING
-       select EFI_PARAMS_FROM_FDT
        select EFI_RUNTIME_WRAPPERS
        help
          This enables the kernel to use EFI runtime services that are

arch/loongarch/include/asm/bootinfo.h +1 -1

index e02ac4af7f6e..8e5881bc5ad1 100644
@@ -36,7 +36,7 @@ struct loongson_system_configuration {
 };
 
 extern u64 efi_system_table;
-extern unsigned long fw_arg0, fw_arg1;
+extern unsigned long fw_arg0, fw_arg1, fw_arg2;
 extern struct loongson_board_info b_info;
 extern struct loongson_system_configuration loongson_sysconf;
 

arch/loongarch/kernel/efi.c +29 -1

index 1f1f755fb425..3b80675726ec 100644
@@ -27,8 +27,13 @@
 static unsigned long efi_nr_tables;
 static unsigned long efi_config_table;
 
+static unsigned long __initdata boot_memmap = EFI_INVALID_TABLE_ADDR;
+
 static efi_system_table_t *efi_systab;
-static efi_config_table_type_t arch_tables[] __initdata = {{},};
+static efi_config_table_type_t arch_tables[] __initdata = {
+       {LINUX_EFI_BOOT_MEMMAP_GUID,    &boot_memmap,   "MEMMAP" },
+       {},
+};
 
 void __init efi_runtime_init(void)
 {

@@ -61,6 +66,8 @@ void __init efi_init(void)
                return;
        }
 
+       efi_systab_report_header(&efi_systab->hdr, efi_systab->fw_vendor);
+
        set_bit(EFI_64BIT, &efi.flags);
        efi_nr_tables    = efi_systab->nr_tables;
        efi_config_table = (unsigned long)efi_systab->tables;

@@ -72,4 +79,25 @@ void __init efi_init(void)
 
        if (screen_info.orig_video_isVGA == VIDEO_TYPE_EFI)
                memblock_reserve(screen_info.lfb_base, screen_info.lfb_size);
+
+       if (boot_memmap != EFI_INVALID_TABLE_ADDR) {
+               struct efi_memory_map_data data;
+               struct efi_boot_memmap *tbl;
+
+               tbl = early_memremap_ro(boot_memmap, sizeof(*tbl));
+               if (tbl) {
+                       data.phys_map           = boot_memmap + sizeof(*tbl);
+                       data.size               = tbl->map_size;
+                       data.desc_size          = tbl->desc_size;
+                       data.desc_version       = tbl->desc_ver;
+
+                       if (efi_memmap_init_early(&data) < 0)
+                               panic("Unable to map EFI memory map.\n");
+
+                       memblock_reserve(data.phys_map & PAGE_MASK,
+                                        PAGE_ALIGN(data.size + (data.phys_map & ~PAGE_MASK)));
+
+                       early_memunmap(tbl, sizeof(*tbl));
+               }
+       }
 }

arch/loongarch/kernel/env.c +9 -13

index 82b478a5c665..05c38d28476e 100644
@@ -8,7 +8,6 @@
 #include <linux/efi.h>
 #include <linux/export.h>
 #include <linux/memblock.h>
-#include <linux/of_fdt.h>
 #include <asm/early_ioremap.h>
 #include <asm/bootinfo.h>
 #include <asm/loongson.h>

@@ -20,21 +19,18 @@ EXPORT_SYMBOL(loongson_sysconf);
 void __init init_environ(void)
 {
        int efi_boot = fw_arg0;
-       struct efi_memory_map_data data;
-       void *fdt_ptr = early_memremap_ro(fw_arg1, SZ_64K);
 
-       if (efi_boot)
-               set_bit(EFI_BOOT, &efi.flags);
-       else
-               clear_bit(EFI_BOOT, &efi.flags);
+       if (efi_boot) {
+               char *cmdline = early_memremap_ro(fw_arg2, COMMAND_LINE_SIZE);
 
-       early_init_dt_scan(fdt_ptr);
-       early_init_fdt_reserve_self();
-       efi_system_table = efi_get_fdt_params(&data);
+               strscpy(boot_command_line, cmdline, COMMAND_LINE_SIZE);
+               early_memunmap(cmdline, COMMAND_LINE_SIZE);
 
-       efi_memmap_init_early(&data);
-       memblock_reserve(data.phys_map & PAGE_MASK,
-                        PAGE_ALIGN(data.size + (data.phys_map & ~PAGE_MASK)));
+               efi_system_table = fw_arg1;
+               set_bit(EFI_BOOT, &efi.flags);
+       } else {
+               clear_bit(EFI_BOOT, &efi.flags);
+       }
 }
 
 static int __init init_cpu_fullname(void)

arch/loongarch/kernel/head.S +2 -0

index 01bac62a6442..8f89f39fd31b 100644
@@ -67,6 +67,8 @@ SYM_CODE_START(kernel_entry)                  # kernel entry point
        st.d            a0, t0, 0               # firmware arguments
        la              t0, fw_arg1
        st.d            a1, t0, 0
+       la              t0, fw_arg2
+       st.d            a2, t0, 0
 
        /* KSave3 used for percpu base, initialized as 0 */
        csrwr           zero, PERCPU_BASE_KS

arch/loongarch/kernel/setup.c +2 -2

index e8714b1d94c8..7fabf2306e80 100644
@@ -51,7 +51,7 @@
 
 struct screen_info screen_info __section(".data");
 
-unsigned long fw_arg0, fw_arg1;
+unsigned long fw_arg0, fw_arg1, fw_arg2;
 DEFINE_PER_CPU(unsigned long, kernelsp);
 struct cpuinfo_loongarch cpu_data[NR_CPUS] __read_mostly;
 

@@ -187,7 +187,6 @@ early_param("mem", early_parse_mem);
 
 void __init platform_init(void)
 {
-       efi_init();
 #ifdef CONFIG_ACPI_TABLE_UPGRADE
        acpi_table_upgrade();
 #endif

@@ -347,6 +346,7 @@ void __init setup_arch(char **cmdline_p)
        *cmdline_p = boot_command_line;
 
        init_environ();
+       efi_init();
        memblock_init();
        parse_early_param();
 

drivers/firmware/efi/libstub/Makefile +8 -5

index 0dbc6d93f2e6..d8d6657e6277 100644
@@ -29,7 +29,7 @@ cflags-$(CONFIG_RISCV)                := $(subst $(CC_FLAGS_FTRACE),,$(KBUILD_CFLAGS)) \
 cflags-$(CONFIG_LOONGARCH)     := $(subst $(CC_FLAGS_FTRACE),,$(KBUILD_CFLAGS)) \
                                   -fpie
 
-cflags-$(CONFIG_EFI_GENERIC_STUB) += -I$(srctree)/scripts/dtc/libfdt
+cflags-$(CONFIG_EFI_PARAMS_FROM_FDT)   += -I$(srctree)/scripts/dtc/libfdt
 
 KBUILD_CFLAGS                  := $(cflags-y) -Os -DDISABLE_BRANCH_PROFILING \
                                   -include $(srctree)/include/linux/hidden.h \

@@ -60,14 +60,17 @@ lib-y                               := efi-stub-helper.o gop.o secureboot.o tpm.o \
                                   alignedmem.o relocate.o vsprintf.o \
                                   systable.o
 
-# include the stub's generic dependencies from lib/ when building for ARM/arm64
-efi-deps-y := fdt_rw.c fdt_ro.c fdt_wip.c fdt.c fdt_empty_tree.c fdt_sw.c
+# include the stub's libfdt dependencies from lib/ when needed
+libfdt-deps                    := fdt_rw.c fdt_ro.c fdt_wip.c fdt.c \
+                                  fdt_empty_tree.c fdt_sw.c
+
+lib-$(CONFIG_EFI_PARAMS_FROM_FDT) += fdt.o \
+                                    $(patsubst %.c,lib-%.o,$(libfdt-deps))
 
 $(obj)/lib-%.o: $(srctree)/lib/%.c FORCE
        $(call if_changed_rule,cc_o_c)
 
-lib-$(CONFIG_EFI_GENERIC_STUB) += efi-stub.o fdt.o string.o intrinsics.o \
-                                  $(patsubst %.c,lib-%.o,$(efi-deps-y))
+lib-$(CONFIG_EFI_GENERIC_STUB) += efi-stub.o string.o intrinsics.o
 
 lib-$(CONFIG_ARM)              += arm32-stub.o
 lib-$(CONFIG_ARM64)            += arm64-stub.o

drivers/firmware/efi/libstub/loongarch-stub.c +49 -8

index b7ef8d2df59e..7c684d10f936 100644
@@ -9,7 +9,8 @@
 #include <asm/addrspace.h>
 #include "efistub.h"
 
-typedef void __noreturn (*kernel_entry_t)(bool efi, unsigned long fdt);
+typedef void __noreturn (*kernel_entry_t)(bool efi, unsigned long systab,
+                                         unsigned long cmdline);
 
 extern int kernel_asize;
 extern int kernel_fsize;

@@ -42,19 +43,60 @@ efi_status_t handle_kernel_image(unsigned long *image_addr,
        return status;
 }
 
-void __noreturn efi_enter_kernel(unsigned long entrypoint, unsigned long fdt, unsigned long fdt_size)
+struct exit_boot_struct {
+       efi_memory_desc_t       *runtime_map;
+       int                     runtime_entry_count;
+};
+
+static efi_status_t exit_boot_func(struct efi_boot_memmap *map, void *priv)
+{
+       struct exit_boot_struct *p = priv;
+
+       /*
+        * Update the memory map with virtual addresses. The function will also
+        * populate @runtime_map with copies of just the EFI_MEMORY_RUNTIME
+        * entries so that we can pass it straight to SetVirtualAddressMap()
+        */
+       efi_get_virtmap(map->map, map->map_size, map->desc_size,
+                       p->runtime_map, &p->runtime_entry_count);
+
+       return EFI_SUCCESS;
+}
+
+efi_status_t efi_boot_kernel(void *handle, efi_loaded_image_t *image,
+                            unsigned long image_addr, char *cmdline_ptr)
 {
        kernel_entry_t real_kernel_entry;
+       struct exit_boot_struct priv;
+       unsigned long desc_size;
+       efi_status_t status;
+       u32 desc_ver;
+
+       status = efi_alloc_virtmap(&priv.runtime_map, &desc_size, &desc_ver);
+       if (status != EFI_SUCCESS) {
+               efi_err("Unable to retrieve UEFI memory map.\n");
+               return status;
+       }
+
+       efi_info("Exiting boot services\n");
+
+       efi_novamap = false;
+       status = efi_exit_boot_services(handle, &priv, exit_boot_func);
+       if (status != EFI_SUCCESS)
+               return status;
+
+       /* Install the new virtual address map */
+       efi_rt_call(set_virtual_address_map,
+                   priv.runtime_entry_count * desc_size, desc_size,
+                   desc_ver, priv.runtime_map);
 
        /* Config Direct Mapping */
        csr_write64(CSR_DMW0_INIT, LOONGARCH_CSR_DMWIN0);
        csr_write64(CSR_DMW1_INIT, LOONGARCH_CSR_DMWIN1);
 
        real_kernel_entry = (kernel_entry_t)
-               ((unsigned long)&kernel_entry - entrypoint + VMLINUX_LOAD_ADDRESS);
+               ((unsigned long)&kernel_entry - image_addr + VMLINUX_LOAD_ADDRESS);
 
-       if (!efi_novamap)
-               real_kernel_entry(true, fdt);
-       else
-               real_kernel_entry(false, fdt);
+       real_kernel_entry(true, (unsigned long)efi_system_table,
+                         (unsigned long)cmdline_ptr);
 }
--

2.35.1

Hi, Ard,

Quoted 15 lines · Ard Biesheuvel

Quoted 13 lines · Huacai Chen

Hi, Ard,

I think the parameters passed to the core kernel need to be discussed. The old way (so-called old world):
a0=argc, a1=argv, a1=bpi

The current way (so-called new world):
a0=efi boot flag, a1=fdt pointer

The new way (in this patchset):
a0=efi boot flag, a1=systemtable, a2=cmdline

Quoted 7 lines

Quoted 2 lines

I prefer to use the current way, there are 3 reasons:
1, both acpi system and dt system can use the same parameters passing method;

DT systems will use this too. The distinction is between EFI boot and non-EFI boot. We *really* should keep these separate, given the experience on ARM, where other projects invent ways to pass those values to the kernel without going through the stub.

In the last patch I see:

(quoted diff) +5 -0

+               void *fdt_ptr = early_memremap_ro(fw_arg1, SZ_64K);
+
+               early_init_dt_scan(fdt_ptr);
+               early_init_fdt_reserve_self();
+
                clear_bit(EFI_BOOT, &efi.flags);

So I suppose for the DT system that means a0=efi boot flag, a1=fdt pointer, a2=cmdline? Then it is not exactly the same as the ACPI system, but similar.

Quoted 15 lines

Quoted 2 lines

2, arm64, riscv and loongarch can use similar logics (light FDT);

No need to repeat a mistake. I intend to fix RISC-V next.

Quoted 3 lines

3, out-of-tree patches can make compatibility with the old world easier by just judging whether a2 is zero.

The whole point of this series is that the EFI stub should not be touching the DT at all. In other words, there is no DT pointer, so the current method needs to be revised.

What we might do is

Quoted 4 lines

a0=systemtable, a1=cmdline

as any non-zero value is treated as logic true. That way, your logic to test a2 is zero will still work.

I think the efi boot flag is still needed, even boot from efistub. Because if boot with "efi=novamap", the efi runtime should be disabled. Then we need efi_enabled(EFI_BOOT) to be false in efi_init().

Quoted 33 lines · Huacai Chen

Hi, Ard,

Quoted 15 lines · Ard Biesheuvel

Quoted 13 lines · Huacai Chen

Hi, Ard,

I think the parameters passed to the core kernel need to be discussed. The old way (so-called old world):
a0=argc, a1=argv, a1=bpi

The current way (so-called new world):
a0=efi boot flag, a1=fdt pointer

The new way (in this patchset):
a0=efi boot flag, a1=systemtable, a2=cmdline

Quoted 7 lines

Quoted 2 lines

I prefer to use the current way, there are 3 reasons:
1, both acpi system and dt system can use the same parameters passing method;

DT systems will use this too. The distinction is between EFI boot and non-EFI boot. We *really* should keep these separate, given the experience on ARM, where other projects invent ways to pass those values to the kernel without going through the stub.

In the last patch I see:

(quoted diff) +5 -0

+               void *fdt_ptr = early_memremap_ro(fw_arg1, SZ_64K);
+
+               early_init_dt_scan(fdt_ptr);
+               early_init_fdt_reserve_self();
+
                clear_bit(EFI_BOOT, &efi.flags);

Quoted 4 lines

So I suppose for the DT system that means a0=efi boot flag, a1=fdt pointer, a2=cmdline? Then it is not exactly the same as the ACPI system, but similar.

No, for non-EFI DT boot, the command line is passed via the DT, so a0=0x0 (non-efi), a1=DT, a2=0x0

Do you intend to support non-EFI DT boot by the way?

So

a2 != 0x0 means old world
a0 != 0x0 means EFI boot, a1 is the command line
a0 == 0x0 means !EFI boot, a1 is the DT

Quoted 33 lines · Huacai Chen

Hi, Ard,

Quoted 15 lines · Ard Biesheuvel

Quoted 13 lines · Huacai Chen

Hi, Ard,

I think the parameters passed to the core kernel need to be discussed. The old way (so-called old world):
a0=argc, a1=argv, a1=bpi

The current way (so-called new world):
a0=efi boot flag, a1=fdt pointer

The new way (in this patchset):
a0=efi boot flag, a1=systemtable, a2=cmdline

Quoted 7 lines

Quoted 2 lines

I prefer to use the current way, there are 3 reasons:
1, both acpi system and dt system can use the same parameters passing method;

DT systems will use this too. The distinction is between EFI boot and non-EFI boot. We *really* should keep these separate, given the experience on ARM, where other projects invent ways to pass those values to the kernel without going through the stub.

In the last patch I see:

(quoted diff) +5 -0

+               void *fdt_ptr = early_memremap_ro(fw_arg1, SZ_64K);
+
+               early_init_dt_scan(fdt_ptr);
+               early_init_fdt_reserve_self();
+
                clear_bit(EFI_BOOT, &efi.flags);

Quoted 4 lines

So I suppose for the DT system that means a0=efi boot flag, a1=fdt pointer, a2=cmdline? Then it is not exactly the same as the ACPI system, but similar.

No, for non-EFI DT boot, the command line is passed via the DT, so a0=0x0 (non-efi), a1=DT, a2=0x0

Do you intend to support non-EFI DT boot by the way?

So

a2 != 0x0 means old world
a0 != 0x0 means EFI boot, a1 is the command line
a0 == 0x0 means !EFI boot, a1 is the DT

Quoted 15 lines

Quoted 15 lines

Quoted 2 lines

2, arm64, riscv and loongarch can use similar logics (light FDT);

No need to repeat a mistake. I intend to fix RISC-V next.

Quoted 3 lines

3, out-of-tree patches can make compatibility with the old world easier by just judging whether a2 is zero.

The whole point of this series is that the EFI stub should not be touching the DT at all. In other words, there is no DT pointer, so the current method needs to be revised.

What we might do is

Quoted 9 lines

Quoted 4 lines

a0=systemtable, a1=cmdline

as any non-zero value is treated as logic true. That way, your logic to test a2 is zero will still work.

I think the efi boot flag is still needed, even boot from efistub. Because if boot with "efi=novamap", the efi runtime should be disabled. Then we need efi_enabled(EFI_BOOT) to be false in efi_init().

I don't think it makes sense to allow efi=novamap on LoongArch, given that we cannot make use of the runtime services that way. So in my code, efi_novamap is set to false unconditionally.

Hi, Ard,

Quoted 43 lines · Ard Biesheuvel

Quoted 33 lines · Huacai Chen

Hi, Ard,

Quoted 15 lines · Ard Biesheuvel

Quoted 13 lines · Huacai Chen

Hi, Ard,

I think the parameters passed to the core kernel need to be discussed. The old way (so-called old world):
a0=argc, a1=argv, a1=bpi

The current way (so-called new world):
a0=efi boot flag, a1=fdt pointer

The new way (in this patchset):
a0=efi boot flag, a1=systemtable, a2=cmdline

Quoted 7 lines

Quoted 2 lines

I prefer to use the current way, there are 3 reasons:
1, both acpi system and dt system can use the same parameters passing method;

DT systems will use this too. The distinction is between EFI boot and non-EFI boot. We *really* should keep these separate, given the experience on ARM, where other projects invent ways to pass those values to the kernel without going through the stub.

In the last patch I see:

(quoted diff) +5 -0

+               void *fdt_ptr = early_memremap_ro(fw_arg1, SZ_64K);
+
+               early_init_dt_scan(fdt_ptr);
+               early_init_fdt_reserve_self();
+
                clear_bit(EFI_BOOT, &efi.flags);

Quoted 4 lines

So I suppose for the DT system that means a0=efi boot flag, a1=fdt pointer, a2=cmdline? Then it is not exactly the same as the ACPI system, but similar.

No, for non-EFI DT boot, the command line is passed via the DT, so a0=0x0 (non-efi), a1=DT, a2=0x0

Quoted 1 line

Do you intend to support non-EFI DT boot by the way?

I think we needn't support non-EFI DT boot, so a0=efi boot flag, a1=systemtable, a2=cmdline is just OK (or maybe we can exchange a1/a2, which looks similar to the old-world).

But I have another question: is
it early enough to get DT from systemtable for DT boot (in the current way DT is the earliest thing)?

Quoted 32 lines

So

a2 != 0x0 means old world
a0 != 0x0 means EFI boot, a1 is the command line
a0 == 0x0 means !EFI boot, a1 is the DT

Quoted 24 lines

Quoted 13 lines

Quoted 2 lines

2, arm64, riscv and loongarch can use similar logics (light FDT);

No need to repeat a mistake. I intend to fix RISC-V next.

Quoted 3 lines

3, out-of-tree patches can make compatibility with the old world easier by just judging whether a2 is zero.

The whole point of this series is that the EFI stub should not be touching the DT at all. In other words, there is no DT pointer, so the current method needs to be revised.

Quoted 6 lines

What we might do is

a0=systemtable, a1=cmdline

as any non-zero value is treated as logic true. That way, your logic to test a2 is zero will still work.

I think the efi boot flag is still needed, even boot from efistub. Because if boot with "efi=novamap", the efi runtime should be disabled. Then we need efi_enabled(EFI_BOOT) to be false in efi_init().

Quoted 3 lines

I don't think it makes sense to allow efi=novamap on LoongArch, given that we cannot make use of the runtime services that way. So in my code, efi_novamap is set to false unconditionally.

Emm, I prefer to support "efi=novamap", the core kernel has already supported "noefi" to disable runtime, I don't want to hack efi_novamap. So please keep the efi boot flag, thanks.

Quoted 47 lines · Huacai Chen

Hi, Ard,

Quoted 43 lines · Ard Biesheuvel

Quoted 33 lines · Huacai Chen

Hi, Ard,

Quoted 15 lines · Ard Biesheuvel

Quoted 13 lines · Huacai Chen

Hi, Ard,

I think the parameters passed to the core kernel need to be discussed. The old way (so-called old world):
a0=argc, a1=argv, a1=bpi

The current way (so-called new world):
a0=efi boot flag, a1=fdt pointer

The new way (in this patchset):
a0=efi boot flag, a1=systemtable, a2=cmdline

Quoted 7 lines

Quoted 2 lines

I prefer to use the current way, there are 3 reasons:
1, both acpi system and dt system can use the same parameters passing method;

DT systems will use this too. The distinction is between EFI boot and non-EFI boot. We *really* should keep these separate, given the experience on ARM, where other projects invent ways to pass those values to the kernel without going through the stub.

In the last patch I see:

(quoted diff) +5 -0

+               void *fdt_ptr = early_memremap_ro(fw_arg1, SZ_64K);
+
+               early_init_dt_scan(fdt_ptr);
+               early_init_fdt_reserve_self();
+
                clear_bit(EFI_BOOT, &efi.flags);

Quoted 4 lines

So I suppose for the DT system that means a0=efi boot flag, a1=fdt pointer, a2=cmdline? Then it is not exactly the same as the ACPI system, but similar.

No, for non-EFI DT boot, the command line is passed via the DT, so a0=0x0 (non-efi), a1=DT, a2=0x0

Quoted 4 lines

Quoted 1 line

Do you intend to support non-EFI DT boot by the way?

I think we needn't support non-EFI DT boot, so a0=efi boot flag, a1=systemtable, a2=cmdline is just OK (or maybe we can exchange a1/a2, which looks similar to the old-world).

Excellent. If non-EFI boot is not supported, we can drop the code that deals with that.

Quoted 4 lines

But I have another question: is
it early enough to get DT from systemtable for DT boot (in the current way DT is the earliest thing)?

If you rely on DT only for the hardware description, then loading it from efi_init() should be fine.

Quoted 36 lines

Quoted 36 lines

So

a2 != 0x0 means old world
a0 != 0x0 means EFI boot, a1 is the command line
a0 == 0x0 means !EFI boot, a1 is the DT

Quoted 15 lines

Quoted 15 lines

Quoted 2 lines

2, arm64, riscv and loongarch can use similar logics (light FDT);

No need to repeat a mistake. I intend to fix RISC-V next.

Quoted 3 lines

3, out-of-tree patches can make compatibility with the old world easier by just judging whether a2 is zero.

The whole point of this series is that the EFI stub should not be touching the DT at all. In other words, there is no DT pointer, so the current method needs to be revised.

What we might do is

Quoted 9 lines

Quoted 4 lines

a0=systemtable, a1=cmdline

as any non-zero value is treated as logic true. That way, your logic to test a2 is zero will still work.

I think the efi boot flag is still needed, even boot from efistub. Because if boot with "efi=novamap", the efi runtime should be disabled. Then we need efi_enabled(EFI_BOOT) to be false in efi_init().

I don't think it makes sense to allow efi=novamap on LoongArch, given that we cannot make use of the runtime services that way. So in my code, efi_novamap is set to false unconditionally.

Quoted 4 lines

Emm, I prefer to support "efi=novamap", the core kernel has already supported "noefi" to disable runtime, I don't want to hack efi_novamap. So please keep the efi boot flag, thanks.

Fair enough. Do you have any uses for efi_novamap in mind?

Quoted 62 lines · Ard Biesheuvel

Quoted 47 lines · Huacai Chen

Hi, Ard,

Quoted 43 lines · Ard Biesheuvel

Quoted 33 lines · Huacai Chen

Hi, Ard,

Quoted 15 lines · Ard Biesheuvel

Quoted 13 lines · Huacai Chen

Hi, Ard,

I think the parameters passed to the core kernel need to be discussed. The old way (so-called old world):
a0=argc, a1=argv, a1=bpi

The current way (so-called new world):
a0=efi boot flag, a1=fdt pointer

The new way (in this patchset):
a0=efi boot flag, a1=systemtable, a2=cmdline

Quoted 7 lines

Quoted 2 lines

I prefer to use the current way, there are 3 reasons:
1, both acpi system and dt system can use the same parameters passing method;

DT systems will use this too. The distinction is between EFI boot and non-EFI boot. We *really* should keep these separate, given the experience on ARM, where other projects invent ways to pass those values to the kernel without going through the stub.

In the last patch I see:

(quoted diff) +5 -0

+               void *fdt_ptr = early_memremap_ro(fw_arg1, SZ_64K);
+
+               early_init_dt_scan(fdt_ptr);
+               early_init_fdt_reserve_self();
+
                clear_bit(EFI_BOOT, &efi.flags);

Quoted 4 lines

So I suppose for the DT system that means a0=efi boot flag, a1=fdt pointer, a2=cmdline? Then it is not exactly the same as the ACPI system, but similar.

No, for non-EFI DT boot, the command line is passed via the DT, so a0=0x0 (non-efi), a1=DT, a2=0x0

Quoted 4 lines

Quoted 1 line

Do you intend to support non-EFI DT boot by the way?

I think we needn't support non-EFI DT boot, so a0=efi boot flag, a1=systemtable, a2=cmdline is just OK (or maybe we can exchange a1/a2, which looks similar to the old-world).

Excellent. If non-EFI boot is not supported, we can drop the code that deals with that.

Quoted 4 lines

But I have another question: is
it early enough to get DT from systemtable for DT boot (in the current way DT is the earliest thing)?

Quoted 2 lines

If you rely on DT only for the hardware description, then loading it from efi_init() should be fine.

OK, then please drop patch #12 at this time. It can be added when we add Loongson-2K support.

Quoted 8 lines

Quoted 7 lines

Quoted 3 lines

I don't think it makes sense to allow efi=novamap on LoongArch, given that we cannot make use of the runtime services that way. So in my code, efi_novamap is set to false unconditionally.

Emm, I prefer to support "efi=novamap", the core kernel has already supported "noefi" to disable runtime, I don't want to hack efi_novamap. So please keep the efi boot flag, thanks.

Quoted 1 line

Fair enough. Do you have any uses for efi_novamap in mind?

I usually use "efi=novamap" in EFI shell to see whether it can work well without runtime. And I think I will modify this patch these days because in another thread you said that this series cannot boot.

Quoted 64 lines · Huacai Chen

Quoted 62 lines · Ard Biesheuvel

Quoted 47 lines · Huacai Chen

Hi, Ard,

Quoted 43 lines · Ard Biesheuvel

Quoted 33 lines · Huacai Chen

Hi, Ard,

Quoted 15 lines · Ard Biesheuvel

Quoted 13 lines · Huacai Chen

Hi, Ard,

I think the parameters passed to the core kernel need to be discussed. The old way (so-called old world):
a0=argc, a1=argv, a1=bpi

The current way (so-called new world):
a0=efi boot flag, a1=fdt pointer

The new way (in this patchset):
a0=efi boot flag, a1=systemtable, a2=cmdline

Quoted 7 lines

Quoted 2 lines

I prefer to use the current way, there are 3 reasons:
1, both acpi system and dt system can use the same parameters passing method;

DT systems will use this too. The distinction is between EFI boot and non-EFI boot. We *really* should keep these separate, given the experience on ARM, where other projects invent ways to pass those values to the kernel without going through the stub.

In the last patch I see:

(quoted diff) +5 -0

+               void *fdt_ptr = early_memremap_ro(fw_arg1, SZ_64K);
+
+               early_init_dt_scan(fdt_ptr);
+               early_init_fdt_reserve_self();
+
                clear_bit(EFI_BOOT, &efi.flags);

Quoted 4 lines

So I suppose for the DT system that means a0=efi boot flag, a1=fdt pointer, a2=cmdline? Then it is not exactly the same as the ACPI system, but similar.

No, for non-EFI DT boot, the command line is passed via the DT, so a0=0x0 (non-efi), a1=DT, a2=0x0

Quoted 4 lines

Quoted 1 line

Do you intend to support non-EFI DT boot by the way?

I think we needn't support non-EFI DT boot, so a0=efi boot flag, a1=systemtable, a2=cmdline is just OK (or maybe we can exchange a1/a2, which looks similar to the old-world).

Excellent. If non-EFI boot is not supported, we can drop the code that deals with that.

Quoted 4 lines

But I have another question: is
it early enough to get DT from systemtable for DT boot (in the current way DT is the earliest thing)?

Quoted 5 lines

Quoted 2 lines

If you rely on DT only for the hardware description, then loading it from efi_init() should be fine.

OK, then please drop patch #12 at this time. It can be added when we add Loongson-2K support.

OK

Quoted 8 lines

Quoted 8 lines

Quoted 7 lines

Quoted 3 lines

I don't think it makes sense to allow efi=novamap on LoongArch, given that we cannot make use of the runtime services that way. So in my code, efi_novamap is set to false unconditionally.

Emm, I prefer to support "efi=novamap", the core kernel has already supported "noefi" to disable runtime, I don't want to hack efi_novamap. So please keep the efi boot flag, thanks.

Quoted 4 lines

Quoted 1 line

Fair enough. Do you have any uses for efi_novamap in mind?

I usually use "efi=novamap" in EFI shell to see whether it can work well without runtime. And I think I will modify this patch these days because in another thread you said that this series cannot boot.

It works fine now.

However, clearing the EFI_BOOT flag is not the correct way to disable runtime services only. And note that we also have efi=noruntime - does that currently work on LoongArch?

Quoted 87 lines · Ard Biesheuvel

Quoted 64 lines · Huacai Chen

Quoted 62 lines · Ard Biesheuvel

Quoted 47 lines · Huacai Chen

Hi, Ard,

Quoted 43 lines · Ard Biesheuvel

Quoted 33 lines · Huacai Chen

Hi, Ard,

Quoted 15 lines · Ard Biesheuvel

Quoted 13 lines · Huacai Chen

Hi, Ard,

I think the parameters passed to the core kernel need to be discussed. The old way (so-called old world):
a0=argc, a1=argv, a1=bpi

The current way (so-called new world):
a0=efi boot flag, a1=fdt pointer

The new way (in this patchset):
a0=efi boot flag, a1=systemtable, a2=cmdline

Quoted 7 lines

Quoted 2 lines

I prefer to use the current way, there are 3 reasons:
1, both acpi system and dt system can use the same parameters passing method;

DT systems will use this too. The distinction is between EFI boot and non-EFI boot. We *really* should keep these separate, given the experience on ARM, where other projects invent ways to pass those values to the kernel without going through the stub.

In the last patch I see:

(quoted diff) +5 -0

+               void *fdt_ptr = early_memremap_ro(fw_arg1, SZ_64K);
+
+               early_init_dt_scan(fdt_ptr);
+               early_init_fdt_reserve_self();
+
                clear_bit(EFI_BOOT, &efi.flags);

Quoted 4 lines

So I suppose for the DT system that means a0=efi boot flag, a1=fdt pointer, a2=cmdline? Then it is not exactly the same as the ACPI system, but similar.

No, for non-EFI DT boot, the command line is passed via the DT, so a0=0x0 (non-efi), a1=DT, a2=0x0

Quoted 4 lines

Quoted 1 line

Do you intend to support non-EFI DT boot by the way?

I think we needn't support non-EFI DT boot, so a0=efi boot flag, a1=systemtable, a2=cmdline is just OK (or maybe we can exchange a1/a2, which looks similar to the old-world).

Excellent. If non-EFI boot is not supported, we can drop the code that deals with that.

Quoted 4 lines

But I have another question: is
it early enough to get DT from systemtable for DT boot (in the current way DT is the earliest thing)?

Quoted 5 lines

Quoted 2 lines

If you rely on DT only for the hardware description, then loading it from efi_init() should be fine.

OK, then please drop patch #12 at this time. It can be added when we add Loongson-2K support.

OK

Quoted 12 lines

Quoted 9 lines

Quoted 7 lines

Quoted 3 lines

I don't think it makes sense to allow efi=novamap on LoongArch, given that we cannot make use of the runtime services that way. So in my code, efi_novamap is set to false unconditionally.

Emm, I prefer to support "efi=novamap", the core kernel has already supported "noefi" to disable runtime, I don't want to hack efi_novamap. So please keep the efi boot flag, thanks.

Fair enough. Do you have any uses for efi_novamap in mind?

I usually use "efi=novamap" in EFI shell to see whether it can work well without runtime. And I think I will modify this patch these days because in another thread you said that this series cannot boot.

Quoted 5 lines

It works fine now.

However, clearing the EFI_BOOT flag is not the correct way to disable runtime services only. And note that we also have efi=noruntime - does that currently work on LoongArch?

OK, but we don't need to add a new "efi=noruntime" parameter, I can use "noefi" instead.

But I think support "efi=novamap" is not a bad
idea (maybe I misunderstood its usage).

Quoted 89 lines · Huacai Chen

Quoted 87 lines · Ard Biesheuvel

Quoted 64 lines · Huacai Chen

Quoted 62 lines · Ard Biesheuvel

Quoted 47 lines · Huacai Chen

Hi, Ard,

Quoted 43 lines · Ard Biesheuvel

Quoted 33 lines · Huacai Chen

Hi, Ard,

Quoted 15 lines · Ard Biesheuvel

Quoted 13 lines · Huacai Chen

Hi, Ard,

I think the parameters passed to the core kernel need to be discussed. The old way (so-called old world):
a0=argc, a1=argv, a1=bpi

The current way (so-called new world):
a0=efi boot flag, a1=fdt pointer

The new way (in this patchset):
a0=efi boot flag, a1=systemtable, a2=cmdline

Quoted 7 lines

Quoted 2 lines

I prefer to use the current way, there are 3 reasons:
1, both acpi system and dt system can use the same parameters passing method;

DT systems will use this too. The distinction is between EFI boot and non-EFI boot. We *really* should keep these separate, given the experience on ARM, where other projects invent ways to pass those values to the kernel without going through the stub.

In the last patch I see:

(quoted diff) +5 -0

+               void *fdt_ptr = early_memremap_ro(fw_arg1, SZ_64K);
+
+               early_init_dt_scan(fdt_ptr);
+               early_init_fdt_reserve_self();
+
                clear_bit(EFI_BOOT, &efi.flags);

Quoted 4 lines

So I suppose for the DT system that means a0=efi boot flag, a1=fdt pointer, a2=cmdline? Then it is not exactly the same as the ACPI system, but similar.

No, for non-EFI DT boot, the command line is passed via the DT, so a0=0x0 (non-efi), a1=DT, a2=0x0

Quoted 4 lines

Quoted 1 line

Do you intend to support non-EFI DT boot by the way?

I think we needn't support non-EFI DT boot, so a0=efi boot flag, a1=systemtable, a2=cmdline is just OK (or maybe we can exchange a1/a2, which looks similar to the old-world).

Excellent. If non-EFI boot is not supported, we can drop the code that deals with that.

Quoted 4 lines

But I have another question: is
it early enough to get DT from systemtable for DT boot (in the current way DT is the earliest thing)?

Quoted 5 lines

Quoted 2 lines

If you rely on DT only for the hardware description, then loading it from efi_init() should be fine.

OK, then please drop patch #12 at this time. It can be added when we add Loongson-2K support.

OK

Quoted 12 lines

Quoted 9 lines

Quoted 7 lines

Quoted 3 lines

I don't think it makes sense to allow efi=novamap on LoongArch, given that we cannot make use of the runtime services that way. So in my code, efi_novamap is set to false unconditionally.

Emm, I prefer to support "efi=novamap", the core kernel has already supported "noefi" to disable runtime, I don't want to hack efi_novamap. So please keep the efi boot flag, thanks.

Fair enough. Do you have any uses for efi_novamap in mind?

I usually use "efi=novamap" in EFI shell to see whether it can work well without runtime. And I think I will modify this patch these days because in another thread you said that this series cannot boot.

Quoted 7 lines

Quoted 5 lines

It works fine now.

However, clearing the EFI_BOOT flag is not the correct way to disable runtime services only. And note that we also have efi=noruntime - does that currently work on LoongArch?

OK, but we don't need to add a new "efi=noruntime" parameter, I can use "noefi" instead.

OK

Quoted 3 lines

But I think support "efi=novamap" is not a bad
idea (maybe I misunderstood its usage).

It basically exists to deal with broken EFI firmware on ARM laptops that were only intended to run Windows. Windows calls SetVirtualAddressMap() *after* creating the new mappings, and some broken firmware drivers will access those regions too early.

On Linux, we install the mapping first, and then much later, we actually create the mappings and only activate them on a single CPU while the EFI runtime service call is in progress.

In any case, I will reinstate the efi_novamap logic for now - we can always revisit it later.

Quoted 110 lines · Ard Biesheuvel

Quoted 89 lines · Huacai Chen

Quoted 87 lines · Ard Biesheuvel

Quoted 64 lines · Huacai Chen

Quoted 62 lines · Ard Biesheuvel

Quoted 47 lines · Huacai Chen

Hi, Ard,

Quoted 43 lines · Ard Biesheuvel

Quoted 33 lines · Huacai Chen

Hi, Ard,

Quoted 15 lines · Ard Biesheuvel

Quoted 13 lines · Huacai Chen

Hi, Ard,

I think the parameters passed to the core kernel need to be discussed. The old way (so-called old world):
a0=argc, a1=argv, a1=bpi

The current way (so-called new world):
a0=efi boot flag, a1=fdt pointer

The new way (in this patchset):
a0=efi boot flag, a1=systemtable, a2=cmdline

Quoted 7 lines

Quoted 2 lines

I prefer to use the current way, there are 3 reasons:
1, both acpi system and dt system can use the same parameters passing method;

DT systems will use this too. The distinction is between EFI boot and non-EFI boot. We *really* should keep these separate, given the experience on ARM, where other projects invent ways to pass those values to the kernel without going through the stub.

In the last patch I see:

(quoted diff) +5 -0

+               void *fdt_ptr = early_memremap_ro(fw_arg1, SZ_64K);
+
+               early_init_dt_scan(fdt_ptr);
+               early_init_fdt_reserve_self();
+
                clear_bit(EFI_BOOT, &efi.flags);

Quoted 4 lines

So I suppose for the DT system that means a0=efi boot flag, a1=fdt pointer, a2=cmdline? Then it is not exactly the same as the ACPI system, but similar.

No, for non-EFI DT boot, the command line is passed via the DT, so a0=0x0 (non-efi), a1=DT, a2=0x0

Quoted 4 lines

Quoted 1 line

Do you intend to support non-EFI DT boot by the way?

I think we needn't support non-EFI DT boot, so a0=efi boot flag, a1=systemtable, a2=cmdline is just OK (or maybe we can exchange a1/a2, which looks similar to the old-world).

Excellent. If non-EFI boot is not supported, we can drop the code that deals with that.

Quoted 4 lines

But I have another question: is
it early enough to get DT from systemtable for DT boot (in the current way DT is the earliest thing)?

Quoted 5 lines

Quoted 2 lines

If you rely on DT only for the hardware description, then loading it from efi_init() should be fine.

OK, then please drop patch #12 at this time. It can be added when we add Loongson-2K support.

OK

Quoted 12 lines

Quoted 9 lines

Quoted 7 lines

Quoted 3 lines

I don't think it makes sense to allow efi=novamap on LoongArch, given that we cannot make use of the runtime services that way. So in my code, efi_novamap is set to false unconditionally.

Emm, I prefer to support "efi=novamap", the core kernel has already supported "noefi" to disable runtime, I don't want to hack efi_novamap. So please keep the efi boot flag, thanks.

Fair enough. Do you have any uses for efi_novamap in mind?

I usually use "efi=novamap" in EFI shell to see whether it can work well without runtime. And I think I will modify this patch these days because in another thread you said that this series cannot boot.

Quoted 7 lines

Quoted 5 lines

It works fine now.

However, clearing the EFI_BOOT flag is not the correct way to disable runtime services only. And note that we also have efi=noruntime - does that currently work on LoongArch?

OK, but we don't need to add a new "efi=noruntime" parameter, I can use "noefi" instead.

OK

Quoted 3 lines

But I think support "efi=novamap" is not a bad
idea (maybe I misunderstood its usage).

It basically exists to deal with broken EFI firmware on ARM laptops that were only intended to run Windows. Windows calls SetVirtualAddressMap() *after* creating the new mappings, and some broken firmware drivers will access those regions too early.

Quoted 6 lines

On Linux, we install the mapping first, and then much later, we actually create the mappings and only activate them on a single CPU while the EFI runtime service call is in progress.

In any case, I will reinstate the efi_novamap logic for now - we can always revisit it later.

OK, now I think there are no big problems. Only some bikesheddings:
1, Please use "a0=efi boot flag, a1=cmdline a2=systemtable" to pass boot information, it looks most similar to the old-world, and we can distinguish old-world/new-world by judging whether a0 is greater than 1.
2, I prefer "early return" in efi_init, i.e., if (boot_memmap == EFI_INVALID_TABLE_ADDR) then return immediately, this makes indentation look better.
3, Declare "cmdline" out of the if() condition in init_environ() looks better. 4, I prefer "kernel_addr" rather than "image_addr" in efi_boot_kernel(). 5, It seems that one line is enough for the last statement in efi_boot_kernel().

Hope this series will be stable as soon as possible, our kexec/kdump work needs to adjust because of this change. :)

Quoted 118 lines · Huacai Chen

Quoted 110 lines · Ard Biesheuvel

Quoted 89 lines · Huacai Chen

Quoted 87 lines · Ard Biesheuvel

Quoted 64 lines · Huacai Chen

Quoted 62 lines · Ard Biesheuvel

Quoted 47 lines · Huacai Chen

Hi, Ard,

Quoted 43 lines · Ard Biesheuvel

Quoted 33 lines · Huacai Chen

Hi, Ard,

Quoted 15 lines · Ard Biesheuvel

Quoted 13 lines · Huacai Chen

Hi, Ard,

I think the parameters passed to the core kernel need to be discussed. The old way (so-called old world):
a0=argc, a1=argv, a1=bpi

The current way (so-called new world):
a0=efi boot flag, a1=fdt pointer

The new way (in this patchset):
a0=efi boot flag, a1=systemtable, a2=cmdline

Quoted 7 lines

Quoted 2 lines

I prefer to use the current way, there are 3 reasons:
1, both acpi system and dt system can use the same parameters passing method;

DT systems will use this too. The distinction is between EFI boot and non-EFI boot. We *really* should keep these separate, given the experience on ARM, where other projects invent ways to pass those values to the kernel without going through the stub.

In the last patch I see:

(quoted diff) +5 -0

+               void *fdt_ptr = early_memremap_ro(fw_arg1, SZ_64K);
+
+               early_init_dt_scan(fdt_ptr);
+               early_init_fdt_reserve_self();
+
                clear_bit(EFI_BOOT, &efi.flags);

Quoted 4 lines

So I suppose for the DT system that means a0=efi boot flag, a1=fdt pointer, a2=cmdline? Then it is not exactly the same as the ACPI system, but similar.

No, for non-EFI DT boot, the command line is passed via the DT, so a0=0x0 (non-efi), a1=DT, a2=0x0

Quoted 4 lines

Quoted 1 line

Do you intend to support non-EFI DT boot by the way?

I think we needn't support non-EFI DT boot, so a0=efi boot flag, a1=systemtable, a2=cmdline is just OK (or maybe we can exchange a1/a2, which looks similar to the old-world).

Excellent. If non-EFI boot is not supported, we can drop the code that deals with that.

Quoted 4 lines

But I have another question: is
it early enough to get DT from systemtable for DT boot (in the current way DT is the earliest thing)?

Quoted 5 lines

Quoted 2 lines

If you rely on DT only for the hardware description, then loading it from efi_init() should be fine.

OK, then please drop patch #12 at this time. It can be added when we add Loongson-2K support.

OK

Quoted 12 lines

Quoted 9 lines

Quoted 7 lines

Quoted 3 lines

I don't think it makes sense to allow efi=novamap on LoongArch, given that we cannot make use of the runtime services that way. So in my code, efi_novamap is set to false unconditionally.

Emm, I prefer to support "efi=novamap", the core kernel has already supported "noefi" to disable runtime, I don't want to hack efi_novamap. So please keep the efi boot flag, thanks.

Fair enough. Do you have any uses for efi_novamap in mind?

I usually use "efi=novamap" in EFI shell to see whether it can work well without runtime. And I think I will modify this patch these days because in another thread you said that this series cannot boot.

Quoted 7 lines

Quoted 5 lines

It works fine now.

However, clearing the EFI_BOOT flag is not the correct way to disable runtime services only. And note that we also have efi=noruntime - does that currently work on LoongArch?

OK, but we don't need to add a new "efi=noruntime" parameter, I can use "noefi" instead.

OK

Quoted 3 lines

But I think support "efi=novamap" is not a bad
idea (maybe I misunderstood its usage).

It basically exists to deal with broken EFI firmware on ARM laptops that were only intended to run Windows. Windows calls SetVirtualAddressMap() *after* creating the new mappings, and some broken firmware drivers will access those regions too early.

Quoted 6 lines

On Linux, we install the mapping first, and then much later, we actually create the mappings and only activate them on a single CPU while the EFI runtime service call is in progress.

In any case, I will reinstate the efi_novamap logic for now - we can always revisit it later.

Quoted 12 lines

OK, now I think there are no big problems. Only some bikesheddings:
1, Please use "a0=efi boot flag, a1=cmdline a2=systemtable" to pass boot information, it looks most similar to the old-world, and we can distinguish old-world/new-world by judging whether a0 is greater than 1.
2, I prefer "early return" in efi_init, i.e., if (boot_memmap == EFI_INVALID_TABLE_ADDR) then return immediately, this makes indentation look better.
3, Declare "cmdline" out of the if() condition in init_environ() looks better. 4, I prefer "kernel_addr" rather than "image_addr" in efi_boot_kernel(). 5, It seems that one line is enough for the last statement in efi_boot_kernel().

OK

Quoted 3 lines

Hope this series will be stable as soon as possible, our kexec/kdump work needs to adjust because of this change. :)

Do you have a link to those patches?

Quoted 135 lines · Ard Biesheuvel

Quoted 118 lines · Huacai Chen

Quoted 110 lines · Ard Biesheuvel

Quoted 89 lines · Huacai Chen

Quoted 87 lines · Ard Biesheuvel

Quoted 64 lines · Huacai Chen

Quoted 62 lines · Ard Biesheuvel

Quoted 47 lines · Huacai Chen

Hi, Ard,

Quoted 43 lines · Ard Biesheuvel

Quoted 33 lines · Huacai Chen

Hi, Ard,

Quoted 15 lines · Ard Biesheuvel

Quoted 13 lines · Huacai Chen

Hi, Ard,

I think the parameters passed to the core kernel need to be discussed.
The old way (so-called old world):
a0=argc, a1=argv, a1=bpi

The current way (so-called new world):
a0=efi boot flag, a1=fdt pointer

The new way (in this patchset):
a0=efi boot flag, a1=systemtable, a2=cmdline

Quoted 7 lines

Quoted 2 lines

I prefer to use the current way, there are 3 reasons:
1, both acpi system and dt system can use the same parameters passing method;

DT systems will use this too. The distinction is between EFI boot and non-EFI boot. We *really* should keep these separate, given the experience on ARM, where other projects invent ways to pass those values to the kernel without going through the stub.

In the last patch I see:

(quoted diff) +5 -0

+               void *fdt_ptr = early_memremap_ro(fw_arg1, SZ_64K);
+
+               early_init_dt_scan(fdt_ptr);
+               early_init_fdt_reserve_self();
+
                clear_bit(EFI_BOOT, &efi.flags);

Quoted 4 lines

So I suppose for the DT system that means a0=efi boot flag, a1=fdt pointer, a2=cmdline? Then it is not exactly the same as the ACPI system, but similar.

No, for non-EFI DT boot, the command line is passed via the DT, so a0=0x0 (non-efi), a1=DT, a2=0x0

Quoted 4 lines

Quoted 1 line

Do you intend to support non-EFI DT boot by the way?

I think we needn't support non-EFI DT boot, so a0=efi boot flag, a1=systemtable, a2=cmdline is just OK (or maybe we can exchange a1/a2, which looks similar to the old-world).

Excellent. If non-EFI boot is not supported, we can drop the code that deals with that.

Quoted 4 lines

But I have another question: is
it early enough to get DT from systemtable for DT boot (in the current way DT is the earliest thing)?

Quoted 5 lines

Quoted 2 lines

If you rely on DT only for the hardware description, then loading it from efi_init() should be fine.

OK, then please drop patch #12 at this time. It can be added when we add Loongson-2K support.

OK

Quoted 12 lines

Quoted 9 lines

Quoted 7 lines

Quoted 3 lines

I don't think it makes sense to allow efi=novamap on LoongArch, given that we cannot make use of the runtime services that way. So in my code, efi_novamap is set to false unconditionally.

Emm, I prefer to support "efi=novamap", the core kernel has already supported "noefi" to disable runtime, I don't want to hack efi_novamap. So please keep the efi boot flag, thanks.

Fair enough. Do you have any uses for efi_novamap in mind?

I usually use "efi=novamap" in EFI shell to see whether it can work well without runtime. And I think I will modify this patch these days because in another thread you said that this series cannot boot.

Quoted 7 lines

Quoted 5 lines

It works fine now.

However, clearing the EFI_BOOT flag is not the correct way to disable runtime services only. And note that we also have efi=noruntime - does that currently work on LoongArch?

OK, but we don't need to add a new "efi=noruntime" parameter, I can use "noefi" instead.

OK

Quoted 3 lines

But I think support "efi=novamap" is not a bad
idea (maybe I misunderstood its usage).

It basically exists to deal with broken EFI firmware on ARM laptops that were only intended to run Windows. Windows calls SetVirtualAddressMap() *after* creating the new mappings, and some broken firmware drivers will access those regions too early.

Quoted 6 lines

On Linux, we install the mapping first, and then much later, we actually create the mappings and only activate them on a single CPU while the EFI runtime service call is in progress.

In any case, I will reinstate the efi_novamap logic for now - we can always revisit it later.

Quoted 12 lines

OK, now I think there are no big problems. Only some bikesheddings:
1, Please use "a0=efi boot flag, a1=cmdline a2=systemtable" to pass boot information, it looks most similar to the old-world, and we can distinguish old-world/new-world by judging whether a0 is greater than 1.
2, I prefer "early return" in efi_init, i.e., if (boot_memmap == EFI_INVALID_TABLE_ADDR) then return immediately, this makes indentation look better.
3, Declare "cmdline" out of the if() condition in init_environ() looks better. 4, I prefer "kernel_addr" rather than "image_addr" in efi_boot_kernel(). 5, It seems that one line is enough for the last statement in efi_boot_kernel().

OK

Quoted 5 lines

Quoted 3 lines

Hope this series will be stable as soon as possible, our kexec/kdump work needs to adjust because of this change. :)

Do you have a link to those patches?

There is a snapshot for patches pending for 6.1:
https://github.com/loongson/linux/commits/loongarch-next

Quoted 137 lines · Huacai Chen

Quoted 135 lines · Ard Biesheuvel

Quoted 118 lines · Huacai Chen

Quoted 110 lines · Ard Biesheuvel

Quoted 89 lines · Huacai Chen

Quoted 87 lines · Ard Biesheuvel

Quoted 64 lines · Huacai Chen

Quoted 62 lines · Ard Biesheuvel

Quoted 47 lines · Huacai Chen

Hi, Ard,

Quoted 43 lines · Ard Biesheuvel

Quoted 33 lines · Huacai Chen

Hi, Ard,

Quoted 22 lines · Ard Biesheuvel

On Mon, 19 Sept 2022 at 03:58, Huacai Chen <[email protected]> wrote:

Hi, Ard,

I think the parameters passed to the core kernel need to be discussed.
The old way (so-called old world):
a0=argc, a1=argv, a1=bpi

The current way (so-called new world):
a0=efi boot flag, a1=fdt pointer

The new way (in this patchset):
a0=efi boot flag, a1=systemtable, a2=cmdline

I prefer to use the current way, there are 3 reasons:
1, both acpi system and dt system can use the same parameters passing method;

DT systems will use this too. The distinction is between EFI boot and
non-EFI boot. We *really* should keep these separate, given the
experience on ARM, where other projects invent ways to pass those
values to the kernel without going through the stub.

In the last patch I see:

(quoted diff) +5 -0

+               void *fdt_ptr = early_memremap_ro(fw_arg1, SZ_64K);
+
+               early_init_dt_scan(fdt_ptr);
+               early_init_fdt_reserve_self();
+
                clear_bit(EFI_BOOT, &efi.flags);

Quoted 4 lines

So I suppose for the DT system that means a0=efi boot flag, a1=fdt pointer, a2=cmdline? Then it is not exactly the same as the ACPI system, but similar.

No, for non-EFI DT boot, the command line is passed via the DT, so a0=0x0 (non-efi), a1=DT, a2=0x0

Quoted 4 lines

Quoted 1 line

Do you intend to support non-EFI DT boot by the way?

I think we needn't support non-EFI DT boot, so a0=efi boot flag, a1=systemtable, a2=cmdline is just OK (or maybe we can exchange a1/a2, which looks similar to the old-world).

Excellent. If non-EFI boot is not supported, we can drop the code that deals with that.

Quoted 4 lines

But I have another question: is
it early enough to get DT from systemtable for DT boot (in the current way DT is the earliest thing)?

Quoted 5 lines

Quoted 2 lines

If you rely on DT only for the hardware description, then loading it from efi_init() should be fine.

OK, then please drop patch #12 at this time. It can be added when we add Loongson-2K support.

OK

Quoted 12 lines

Quoted 9 lines

Quoted 7 lines

Quoted 3 lines

I don't think it makes sense to allow efi=novamap on LoongArch, given that we cannot make use of the runtime services that way. So in my code, efi_novamap is set to false unconditionally.

Emm, I prefer to support "efi=novamap", the core kernel has already supported "noefi" to disable runtime, I don't want to hack efi_novamap. So please keep the efi boot flag, thanks.

Fair enough. Do you have any uses for efi_novamap in mind?

I usually use "efi=novamap" in EFI shell to see whether it can work well without runtime. And I think I will modify this patch these days because in another thread you said that this series cannot boot.

Quoted 7 lines

Quoted 5 lines

It works fine now.

However, clearing the EFI_BOOT flag is not the correct way to disable runtime services only. And note that we also have efi=noruntime - does that currently work on LoongArch?

OK, but we don't need to add a new "efi=noruntime" parameter, I can use "noefi" instead.

OK

Quoted 3 lines

But I think support "efi=novamap" is not a bad
idea (maybe I misunderstood its usage).

It basically exists to deal with broken EFI firmware on ARM laptops that were only intended to run Windows. Windows calls SetVirtualAddressMap() *after* creating the new mappings, and some broken firmware drivers will access those regions too early.

Quoted 6 lines

On Linux, we install the mapping first, and then much later, we actually create the mappings and only activate them on a single CPU while the EFI runtime service call is in progress.

In any case, I will reinstate the efi_novamap logic for now - we can always revisit it later.

Quoted 12 lines

OK, now I think there are no big problems. Only some bikesheddings:
1, Please use "a0=efi boot flag, a1=cmdline a2=systemtable" to pass boot information, it looks most similar to the old-world, and we can distinguish old-world/new-world by judging whether a0 is greater than 1.
2, I prefer "early return" in efi_init, i.e., if (boot_memmap == EFI_INVALID_TABLE_ADDR) then return immediately, this makes indentation look better.
3, Declare "cmdline" out of the if() condition in init_environ() looks better. 4, I prefer "kernel_addr" rather than "image_addr" in efi_boot_kernel(). 5, It seems that one line is enough for the last statement in efi_boot_kernel().

OK

Quoted 7 lines

Quoted 5 lines

Quoted 3 lines

Hope this series will be stable as soon as possible, our kexec/kdump work needs to adjust because of this change. :)

Do you have a link to those patches?

There is a snapshot for patches pending for 6.1:
https://github.com/loongson/linux/commits/loongarch-next

Thanks. I already spotted an issue there which is exactly the kind of thing I am trying to avoid with this series.

arch/loongarch/kernel/mem.c +5 -0

index 7423361b0ebc9b..c68c88e40cc0b9 100644
@@ -61,4 +62,9 @@ void __init memblock_init(void)
 
  /* Reserve the initrd */
  reserve_initrd_mem();
+
+ /* Main reserved memory for the elf core head */
+ early_init_fdt_scan_reserved_mem();
+ /* Parse linux,usable-memory-range for crash dump kernel */
+ early_init_dt_check_for_usable_mem_range();
 }

So here, we are adding support for DT memory reservations, which kdump apparently needs.

However, this parsing of the DT not only happens on kexec boot: all ACPI and DT kernels will now honour FDT memory reservations passed in via a DT when booting the first kernel, and external projects may use this and start to depend on this.

Once something like this hits the upstream kernel, it is *very* difficult to change or remove.

If you only need to pass the usable memory range for kcrash/kdump, it's probably better to use a command line option for that, instead of relying on DT memory reservations.

Hi, Ard,

Quoted 176 lines · Ard Biesheuvel

Quoted 137 lines · Huacai Chen

Quoted 135 lines · Ard Biesheuvel

Quoted 118 lines · Huacai Chen

Quoted 110 lines · Ard Biesheuvel

Quoted 89 lines · Huacai Chen

Quoted 87 lines · Ard Biesheuvel

Quoted 64 lines · Huacai Chen

Quoted 62 lines · Ard Biesheuvel

Quoted 47 lines · Huacai Chen

Hi, Ard,

Quoted 43 lines · Ard Biesheuvel

Quoted 37 lines · Huacai Chen

Hi, Ard,

On Mon, Sep 19, 2022 at 1:15 PM Ard Biesheuvel <[email protected]> wrote:

On Mon, 19 Sept 2022 at 03:58, Huacai Chen <[email protected]> wrote:

Hi, Ard,

I think the parameters passed to the core kernel need to be discussed.
The old way (so-called old world):
a0=argc, a1=argv, a1=bpi

The current way (so-called new world):
a0=efi boot flag, a1=fdt pointer

The new way (in this patchset):
a0=efi boot flag, a1=systemtable, a2=cmdline

I prefer to use the current way, there are 3 reasons:
1, both acpi system and dt system can use the same parameters passing method;

DT systems will use this too. The distinction is between EFI boot and
non-EFI boot. We *really* should keep these separate, given the
experience on ARM, where other projects invent ways to pass those
values to the kernel without going through the stub.
In the last patch I see:
+ void *fdt_ptr = early_memremap_ro(fw_arg1, SZ_64K);
+
+ early_init_dt_scan(fdt_ptr);
+ early_init_fdt_reserve_self();
+
clear_bit(EFI_BOOT, &efi.flags);
So I suppose for the DT system that means a0=efi boot flag, a1=fdt
pointer, a2=cmdline? Then it is not exactly the same as the ACPI
system, but similar.

No, for non-EFI DT boot, the command line is passed via the DT, so a0=0x0 (non-efi), a1=DT, a2=0x0

Quoted 4 lines

Quoted 1 line

Do you intend to support non-EFI DT boot by the way?

I think we needn't support non-EFI DT boot, so a0=efi boot flag, a1=systemtable, a2=cmdline is just OK (or maybe we can exchange a1/a2, which looks similar to the old-world).

Excellent. If non-EFI boot is not supported, we can drop the code that deals with that.

Quoted 4 lines

But I have another question: is
it early enough to get DT from systemtable for DT boot (in the current way DT is the earliest thing)?

Quoted 5 lines

Quoted 2 lines

If you rely on DT only for the hardware description, then loading it from efi_init() should be fine.

OK, then please drop patch #12 at this time. It can be added when we add Loongson-2K support.

OK

Quoted 12 lines

Quoted 9 lines

Quoted 7 lines

Quoted 3 lines

I don't think it makes sense to allow efi=novamap on LoongArch, given that we cannot make use of the runtime services that way. So in my code, efi_novamap is set to false unconditionally.

Emm, I prefer to support "efi=novamap", the core kernel has already supported "noefi" to disable runtime, I don't want to hack efi_novamap. So please keep the efi boot flag, thanks.

Fair enough. Do you have any uses for efi_novamap in mind?

I usually use "efi=novamap" in EFI shell to see whether it can work well without runtime. And I think I will modify this patch these days because in another thread you said that this series cannot boot.

Quoted 7 lines

Quoted 5 lines

It works fine now.

However, clearing the EFI_BOOT flag is not the correct way to disable runtime services only. And note that we also have efi=noruntime - does that currently work on LoongArch?

OK, but we don't need to add a new "efi=noruntime" parameter, I can use "noefi" instead.

OK

Quoted 3 lines

But I think support "efi=novamap" is not a bad
idea (maybe I misunderstood its usage).

It basically exists to deal with broken EFI firmware on ARM laptops that were only intended to run Windows. Windows calls SetVirtualAddressMap() *after* creating the new mappings, and some broken firmware drivers will access those regions too early.

Quoted 6 lines

On Linux, we install the mapping first, and then much later, we actually create the mappings and only activate them on a single CPU while the EFI runtime service call is in progress.

In any case, I will reinstate the efi_novamap logic for now - we can always revisit it later.

Quoted 12 lines

OK, now I think there are no big problems. Only some bikesheddings:
1, Please use "a0=efi boot flag, a1=cmdline a2=systemtable" to pass boot information, it looks most similar to the old-world, and we can distinguish old-world/new-world by judging whether a0 is greater than 1.
2, I prefer "early return" in efi_init, i.e., if (boot_memmap == EFI_INVALID_TABLE_ADDR) then return immediately, this makes indentation look better.
3, Declare "cmdline" out of the if() condition in init_environ() looks better. 4, I prefer "kernel_addr" rather than "image_addr" in efi_boot_kernel(). 5, It seems that one line is enough for the last statement in efi_boot_kernel().

OK

Quoted 7 lines

Quoted 5 lines

Quoted 3 lines

Hope this series will be stable as soon as possible, our kexec/kdump work needs to adjust because of this change. :)

Do you have a link to those patches?

There is a snapshot for patches pending for 6.1:
https://github.com/loongson/linux/commits/loongarch-next

Thanks. I already spotted an issue there which is exactly the kind of thing I am trying to avoid with this series.

arch/loongarch/kernel/mem.c +5 -0

index 7423361b0ebc9b..c68c88e40cc0b9 100644
@@ -61,4 +62,9 @@ void __init memblock_init(void)
 
  /* Reserve the initrd */
  reserve_initrd_mem();
+
+ /* Main reserved memory for the elf core head */
+ early_init_fdt_scan_reserved_mem();
+ /* Parse linux,usable-memory-range for crash dump kernel */
+ early_init_dt_check_for_usable_mem_range();
 }

So here, we are adding support for DT memory reservations, which kdump apparently needs.

However, this parsing of the DT not only happens on kexec boot: all ACPI and DT kernels will now honour FDT memory reservations passed in via a DT when booting the first kernel, and external projects may use this and start to depend on this.

Once something like this hits the upstream kernel, it is *very* difficult to change or remove.

Quoted 3 lines

If you only need to pass the usable memory range for kcrash/kdump, it's probably better to use a command line option for that, instead of relying on DT memory reservations.

Thank you for your suggestion, but we found some trouble when handle initrd in kexec.
In current implementation, we generate a fdt in kexec-tools, then fill "linux,usable-memory-range", "linux,elfcorehdr" and initrd information in it. After this series, we can use "mem=xxx" "elfcorehdr=" in cmdline, but how to handle initrd information which is stored in a system table?

Quoted 180 lines · Huacai Chen

Hi, Ard,

Quoted 176 lines · Ard Biesheuvel

Quoted 137 lines · Huacai Chen

Quoted 135 lines · Ard Biesheuvel

Quoted 118 lines · Huacai Chen

Quoted 110 lines · Ard Biesheuvel

Quoted 89 lines · Huacai Chen

Quoted 87 lines · Ard Biesheuvel

Quoted 64 lines · Huacai Chen

Quoted 62 lines · Ard Biesheuvel

Quoted 47 lines · Huacai Chen

Hi, Ard,

Quoted 43 lines · Ard Biesheuvel

On Mon, 19 Sept 2022 at 08:06, Huacai Chen <[email protected]> wrote:

Hi, Ard,

On Mon, Sep 19, 2022 at 1:15 PM Ard Biesheuvel <[email protected]> wrote:

On Mon, 19 Sept 2022 at 03:58, Huacai Chen <[email protected]> wrote:

Hi, Ard,

I think the parameters passed to the core kernel need to be discussed.
The old way (so-called old world):
a0=argc, a1=argv, a1=bpi

The current way (so-called new world):
a0=efi boot flag, a1=fdt pointer

The new way (in this patchset):
a0=efi boot flag, a1=systemtable, a2=cmdline

I prefer to use the current way, there are 3 reasons:
1, both acpi system and dt system can use the same parameters passing method;

DT systems will use this too. The distinction is between EFI boot and
non-EFI boot. We *really* should keep these separate, given the
experience on ARM, where other projects invent ways to pass those
values to the kernel without going through the stub.
In the last patch I see:
+ void *fdt_ptr = early_memremap_ro(fw_arg1, SZ_64K);
+
+ early_init_dt_scan(fdt_ptr);
+ early_init_fdt_reserve_self();
+
clear_bit(EFI_BOOT, &efi.flags);
So I suppose for the DT system that means a0=efi boot flag, a1=fdt
pointer, a2=cmdline? Then it is not exactly the same as the ACPI
system, but similar.

No, for non-EFI DT boot, the command line is passed via the DT, so
a0=0x0 (non-efi), a1=DT, a2=0x0

Quoted 4 lines

Quoted 1 line

Do you intend to support non-EFI DT boot by the way?

I think we needn't support non-EFI DT boot, so a0=efi boot flag, a1=systemtable, a2=cmdline is just OK (or maybe we can exchange a1/a2, which looks similar to the old-world).

Excellent. If non-EFI boot is not supported, we can drop the code that deals with that.

Quoted 4 lines

But I have another question: is
it early enough to get DT from systemtable for DT boot (in the current way DT is the earliest thing)?

Quoted 5 lines

Quoted 2 lines

If you rely on DT only for the hardware description, then loading it from efi_init() should be fine.

OK, then please drop patch #12 at this time. It can be added when we add Loongson-2K support.

OK

Quoted 12 lines

Quoted 9 lines

Quoted 7 lines

Quoted 3 lines

I don't think it makes sense to allow efi=novamap on LoongArch, given
that we cannot make use of the runtime services that way. So in my
code, efi_novamap is set to false unconditionally.

Emm, I prefer to support "efi=novamap", the core kernel has already supported "noefi" to disable runtime, I don't want to hack efi_novamap. So please keep the efi boot flag, thanks.

Fair enough. Do you have any uses for efi_novamap in mind?

I usually use "efi=novamap" in EFI shell to see whether it can work well without runtime. And I think I will modify this patch these days because in another thread you said that this series cannot boot.

Quoted 7 lines

Quoted 5 lines

It works fine now.

However, clearing the EFI_BOOT flag is not the correct way to disable runtime services only. And note that we also have efi=noruntime - does that currently work on LoongArch?

OK, but we don't need to add a new "efi=noruntime" parameter, I can use "noefi" instead.

OK

Quoted 3 lines

But I think support "efi=novamap" is not a bad
idea (maybe I misunderstood its usage).

It basically exists to deal with broken EFI firmware on ARM laptops that were only intended to run Windows. Windows calls SetVirtualAddressMap() *after* creating the new mappings, and some broken firmware drivers will access those regions too early.

Quoted 6 lines

On Linux, we install the mapping first, and then much later, we actually create the mappings and only activate them on a single CPU while the EFI runtime service call is in progress.

In any case, I will reinstate the efi_novamap logic for now - we can always revisit it later.

Quoted 12 lines

OK, now I think there are no big problems. Only some bikesheddings:
1, Please use "a0=efi boot flag, a1=cmdline a2=systemtable" to pass boot information, it looks most similar to the old-world, and we can distinguish old-world/new-world by judging whether a0 is greater than 1.
2, I prefer "early return" in efi_init, i.e., if (boot_memmap == EFI_INVALID_TABLE_ADDR) then return immediately, this makes indentation look better.
3, Declare "cmdline" out of the if() condition in init_environ() looks better. 4, I prefer "kernel_addr" rather than "image_addr" in efi_boot_kernel(). 5, It seems that one line is enough for the last statement in efi_boot_kernel().

OK

Quoted 7 lines

Quoted 5 lines

Quoted 3 lines

Hope this series will be stable as soon as possible, our kexec/kdump work needs to adjust because of this change. :)

Do you have a link to those patches?

There is a snapshot for patches pending for 6.1:
https://github.com/loongson/linux/commits/loongarch-next

Thanks. I already spotted an issue there which is exactly the kind of thing I am trying to avoid with this series.

arch/loongarch/kernel/mem.c +5 -0

index 7423361b0ebc9b..c68c88e40cc0b9 100644
@@ -61,4 +62,9 @@ void __init memblock_init(void)
 
  /* Reserve the initrd */
  reserve_initrd_mem();
+
+ /* Main reserved memory for the elf core head */
+ early_init_fdt_scan_reserved_mem();
+ /* Parse linux,usable-memory-range for crash dump kernel */
+ early_init_dt_check_for_usable_mem_range();
 }

So here, we are adding support for DT memory reservations, which kdump apparently needs.

However, this parsing of the DT not only happens on kexec boot: all ACPI and DT kernels will now honour FDT memory reservations passed in via a DT when booting the first kernel, and external projects may use this and start to depend on this.

Once something like this hits the upstream kernel, it is *very* difficult to change or remove.

Quoted 11 lines

Quoted 3 lines

If you only need to pass the usable memory range for kcrash/kdump, it's probably better to use a command line option for that, instead of relying on DT memory reservations.

Thank you for your suggestion, but we found some trouble when handle initrd in kexec.
In current implementation, we generate a fdt in kexec-tools, then fill "linux,usable-memory-range", "linux,elfcorehdr" and initrd information in it. After this series, we can use "mem=xxx" "elfcorehdr=" in cmdline, but how to handle initrd information which is stored in a system table?