* [v3 PATCH 0/5] Add UEFI support for RISC-V
@ 2020-04-15 19:54 Atish Patra
2020-04-15 19:54 ` [v3 PATCH 1/5] efi: Move arm-stub to a common file Atish Patra
` (5 more replies)
0 siblings, 6 replies; 20+ messages in thread
From: Atish Patra @ 2020-04-15 19:54 UTC (permalink / raw)
To: linux-kernel
Cc: Atish Patra, Ard Biesheuvel, Arnd Bergmann, Catalin Marinas,
Greg Kroah-Hartman, linux-efi, linux-riscv, linux-arm-kernel,
Masahiro Yamada, Palmer Dabbelt, Russell King, Will Deacon
This series adds UEFI support for RISC-V. Currently, only boot time
services have been added. Runtime services will be added in a separate
series. This series depends on some core EFI patches
present in current in efi-next and following other patches.
U-Boot: Adds the boot hartid under chosen node.
https://lists.denx.de/pipermail/u-boot/2020-April/405726.html
Linux kernel: 5.7-rc1
OpenSBI: master
Patch 1 just moves arm-stub code to a generic code so that it can be used
across different architecture.
Patch 3 adds fixmap bindings so that CONFIG_EFI can be compiled and we do not
have create separate config to enable boot time services.
As runtime services are not enabled at this time, full generic early ioremap
support is also not added in this series.
Patch 4 and 5 adds the PE/COFF header and EFI stub code support for RISC-V
respectively.
The patches can also be found in following git repo.
https://github.com/atishp04/linux/tree/wip_uefi_riscv_v3
The patches have been verified on Qemu using bootefi command in U-Boot.
Changes from v2->v3:
1. Rebased on top of latest efi patches.
2. Improved handle_kernel_image().
Changes from v1->v2:
1. Rebased on 5.7-rc1.
2. Fixed minor typos and removed redundant macros/comments.
Changes from previous version:
1. Renamed to the generic efi stub macro.
2. Address all redundant comments.
3. Supported EFI kernel image with normal booti command.
4. Removed runtime service related macro defines.
Atish Patra (5):
efi: Move arm-stub to a common file
include: pe.h: Add RISC-V related PE definition
RISC-V: Define fixmap bindings for generic early ioremap support
RISC-V: Add PE/COFF header for EFI stub
RISC-V: Add EFI stub support.
arch/arm/Kconfig | 2 +-
arch/arm64/Kconfig | 2 +-
arch/riscv/Kconfig | 21 ++++
arch/riscv/Makefile | 1 +
arch/riscv/configs/defconfig | 1 +
arch/riscv/include/asm/Kbuild | 1 +
arch/riscv/include/asm/efi.h | 44 +++++++
arch/riscv/include/asm/fixmap.h | 18 +++
arch/riscv/include/asm/io.h | 1 +
arch/riscv/include/asm/sections.h | 13 ++
arch/riscv/kernel/Makefile | 4 +
arch/riscv/kernel/efi-header.S | 99 ++++++++++++++++
arch/riscv/kernel/head.S | 16 +++
arch/riscv/kernel/image-vars.h | 53 +++++++++
arch/riscv/kernel/vmlinux.lds.S | 20 +++-
drivers/firmware/efi/Kconfig | 4 +-
drivers/firmware/efi/libstub/Makefile | 19 ++-
.../efi/libstub/{arm-stub.c => efi-stub.c} | 0
drivers/firmware/efi/libstub/riscv-stub.c | 111 ++++++++++++++++++
include/linux/pe.h | 3 +
20 files changed, 421 insertions(+), 12 deletions(-)
create mode 100644 arch/riscv/include/asm/efi.h
create mode 100644 arch/riscv/include/asm/sections.h
create mode 100644 arch/riscv/kernel/efi-header.S
create mode 100644 arch/riscv/kernel/image-vars.h
rename drivers/firmware/efi/libstub/{arm-stub.c => efi-stub.c} (100%)
create mode 100644 drivers/firmware/efi/libstub/riscv-stub.c
--
2.24.0
^ permalink raw reply [flat|nested] 20+ messages in thread
* [v3 PATCH 1/5] efi: Move arm-stub to a common file
2020-04-15 19:54 [v3 PATCH 0/5] Add UEFI support for RISC-V Atish Patra
@ 2020-04-15 19:54 ` Atish Patra
2020-04-15 19:54 ` [v3 PATCH 2/5] include: pe.h: Add RISC-V related PE definition Atish Patra
` (4 subsequent siblings)
5 siblings, 0 replies; 20+ messages in thread
From: Atish Patra @ 2020-04-15 19:54 UTC (permalink / raw)
To: linux-kernel
Cc: Atish Patra, Ard Biesheuvel, Arnd Bergmann, Catalin Marinas,
Greg Kroah-Hartman, linux-efi, linux-riscv, linux-arm-kernel,
Masahiro Yamada, Palmer Dabbelt, Russell King, Will Deacon
Most of the arm-stub code is written in an architecture independent manner.
As a result, RISC-V can reuse most of the arm-stub code.
Rename the arm-stub.c to efi-stub.c so that ARM, ARM64 and RISC-V can use it.
This patch doesn't introduce any functional changes.
Signed-off-by: Atish Patra <atish.patra@wdc.com>
---
arch/arm/Kconfig | 2 +-
arch/arm64/Kconfig | 2 +-
drivers/firmware/efi/Kconfig | 4 ++--
drivers/firmware/efi/libstub/Makefile | 12 ++++++------
.../firmware/efi/libstub/{arm-stub.c => efi-stub.c} | 0
5 files changed, 10 insertions(+), 10 deletions(-)
rename drivers/firmware/efi/libstub/{arm-stub.c => efi-stub.c} (100%)
diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 66a04f6f4775..165987aa5bcd 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -1954,7 +1954,7 @@ config EFI
select UCS2_STRING
select EFI_PARAMS_FROM_FDT
select EFI_STUB
- select EFI_ARMSTUB
+ select EFI_GENERIC_STUB
select EFI_RUNTIME_WRAPPERS
---help---
This option provides support for runtime services provided
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 40fb05d96c60..32d818c5ccda 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -1785,7 +1785,7 @@ config EFI
select EFI_PARAMS_FROM_FDT
select EFI_RUNTIME_WRAPPERS
select EFI_STUB
- select EFI_ARMSTUB
+ select EFI_GENERIC_STUB
default y
help
This option provides support for runtime services provided
diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
index 613828d3f106..2a2b2b96a1dc 100644
--- a/drivers/firmware/efi/Kconfig
+++ b/drivers/firmware/efi/Kconfig
@@ -106,12 +106,12 @@ config EFI_PARAMS_FROM_FDT
config EFI_RUNTIME_WRAPPERS
bool
-config EFI_ARMSTUB
+config EFI_GENERIC_STUB
bool
config EFI_ARMSTUB_DTB_LOADER
bool "Enable the DTB loader"
- depends on EFI_ARMSTUB
+ depends on EFI_GENERIC_STUB
default y
help
Select this config option to add support for the dtb= command
diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile
index 2dbe4394f818..2b4e09bf987c 100644
--- a/drivers/firmware/efi/libstub/Makefile
+++ b/drivers/firmware/efi/libstub/Makefile
@@ -23,7 +23,7 @@ cflags-$(CONFIG_ARM) := $(subst $(CC_FLAGS_FTRACE),,$(KBUILD_CFLAGS)) \
-fno-builtin -fpic \
$(call cc-option,-mno-single-pic-base)
-cflags-$(CONFIG_EFI_ARMSTUB) += -I$(srctree)/scripts/dtc/libfdt
+cflags-$(CONFIG_EFI_GENERIC_STUB) += -I$(srctree)/scripts/dtc/libfdt
KBUILD_CFLAGS := $(cflags-y) -DDISABLE_BRANCH_PROFILING \
-include $(srctree)/drivers/firmware/efi/libstub/hidden.h \
@@ -46,13 +46,13 @@ lib-y := efi-stub-helper.o gop.o secureboot.o tpm.o \
alignedmem.o relocate.o
# include the stub's generic dependencies from lib/ when building for ARM/arm64
-arm-deps-y := fdt_rw.c fdt_ro.c fdt_wip.c fdt.c fdt_empty_tree.c fdt_sw.c
+efi-deps-y := fdt_rw.c fdt_ro.c fdt_wip.c fdt.c fdt_empty_tree.c fdt_sw.c
$(obj)/lib-%.o: $(srctree)/lib/%.c FORCE
$(call if_changed_rule,cc_o_c)
-lib-$(CONFIG_EFI_ARMSTUB) += arm-stub.o fdt.o string.o \
- $(patsubst %.c,lib-%.o,$(arm-deps-y))
+lib-$(CONFIG_EFI_GENERIC_STUB) += efi-stub.o fdt.o string.o \
+ $(patsubst %.c,lib-%.o,$(efi-deps-y))
lib-$(CONFIG_ARM) += arm32-stub.o
lib-$(CONFIG_ARM64) += arm64-stub.o
@@ -74,8 +74,8 @@ CFLAGS_arm64-stub.o := -DTEXT_OFFSET=$(TEXT_OFFSET)
# a verification pass to see if any absolute relocations exist in any of the
# object files.
#
-extra-$(CONFIG_EFI_ARMSTUB) := $(lib-y)
-lib-$(CONFIG_EFI_ARMSTUB) := $(patsubst %.o,%.stub.o,$(lib-y))
+extra-$(CONFIG_EFI_GENERIC_STUB) := $(lib-y)
+lib-$(CONFIG_EFI_GENERIC_STUB) := $(patsubst %.o,%.stub.o,$(lib-y))
STUBCOPY_FLAGS-$(CONFIG_ARM64) += --prefix-alloc-sections=.init \
--prefix-symbols=__efistub_
diff --git a/drivers/firmware/efi/libstub/arm-stub.c b/drivers/firmware/efi/libstub/efi-stub.c
similarity index 100%
rename from drivers/firmware/efi/libstub/arm-stub.c
rename to drivers/firmware/efi/libstub/efi-stub.c
--
2.24.0
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [v3 PATCH 2/5] include: pe.h: Add RISC-V related PE definition
2020-04-15 19:54 [v3 PATCH 0/5] Add UEFI support for RISC-V Atish Patra
2020-04-15 19:54 ` [v3 PATCH 1/5] efi: Move arm-stub to a common file Atish Patra
@ 2020-04-15 19:54 ` Atish Patra
2020-09-18 8:30 ` [tip: efi/core] " tip-bot2 for Atish Patra
2020-04-15 19:54 ` [v3 PATCH 3/5] RISC-V: Define fixmap bindings for generic early ioremap support Atish Patra
` (3 subsequent siblings)
5 siblings, 1 reply; 20+ messages in thread
From: Atish Patra @ 2020-04-15 19:54 UTC (permalink / raw)
To: linux-kernel
Cc: Atish Patra, Ard Biesheuvel, Arnd Bergmann, Catalin Marinas,
Greg Kroah-Hartman, linux-efi, linux-riscv, linux-arm-kernel,
Masahiro Yamada, Palmer Dabbelt, Russell King, Will Deacon
Define RISC-V related machine types.
Signed-off-by: Atish Patra <atish.patra@wdc.com>
Reviewed-by: Ard Biesheuvel <ardb@kernel.org>
---
include/linux/pe.h | 3 +++
1 file changed, 3 insertions(+)
diff --git a/include/linux/pe.h b/include/linux/pe.h
index 8ad71d763a77..daf09ffffe38 100644
--- a/include/linux/pe.h
+++ b/include/linux/pe.h
@@ -55,6 +55,9 @@
#define IMAGE_FILE_MACHINE_POWERPC 0x01f0
#define IMAGE_FILE_MACHINE_POWERPCFP 0x01f1
#define IMAGE_FILE_MACHINE_R4000 0x0166
+#define IMAGE_FILE_MACHINE_RISCV32 0x5032
+#define IMAGE_FILE_MACHINE_RISCV64 0x5064
+#define IMAGE_FILE_MACHINE_RISCV128 0x5128
#define IMAGE_FILE_MACHINE_SH3 0x01a2
#define IMAGE_FILE_MACHINE_SH3DSP 0x01a3
#define IMAGE_FILE_MACHINE_SH3E 0x01a4
--
2.24.0
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [v3 PATCH 3/5] RISC-V: Define fixmap bindings for generic early ioremap support
2020-04-15 19:54 [v3 PATCH 0/5] Add UEFI support for RISC-V Atish Patra
2020-04-15 19:54 ` [v3 PATCH 1/5] efi: Move arm-stub to a common file Atish Patra
2020-04-15 19:54 ` [v3 PATCH 2/5] include: pe.h: Add RISC-V related PE definition Atish Patra
@ 2020-04-15 19:54 ` Atish Patra
2020-04-15 19:54 ` [v3 PATCH 4/5] RISC-V: Add PE/COFF header for EFI stub Atish Patra
` (2 subsequent siblings)
5 siblings, 0 replies; 20+ messages in thread
From: Atish Patra @ 2020-04-15 19:54 UTC (permalink / raw)
To: linux-kernel
Cc: Atish Patra, Ard Biesheuvel, Arnd Bergmann, Catalin Marinas,
Greg Kroah-Hartman, linux-efi, linux-riscv, linux-arm-kernel,
Masahiro Yamada, Palmer Dabbelt, Russell King, Will Deacon
UEFI uses early IO or memory mappings for runtime services before
normal ioremap() is usable. This patch only adds minimum necessary
fixmap bindings and headers for generic ioremap support to work.
Signed-off-by: Atish Patra <atish.patra@wdc.com>
Acked-by: Ard Biesheuvel <ardb@kernel.org>
---
arch/riscv/Kconfig | 1 +
arch/riscv/include/asm/Kbuild | 1 +
arch/riscv/include/asm/fixmap.h | 18 ++++++++++++++++++
arch/riscv/include/asm/io.h | 1 +
4 files changed, 21 insertions(+)
diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
index a197258595ef..f39e326a7a42 100644
--- a/arch/riscv/Kconfig
+++ b/arch/riscv/Kconfig
@@ -68,6 +68,7 @@ config RISCV
select ARCH_HAS_GCOV_PROFILE_ALL
select HAVE_COPY_THREAD_TLS
select HAVE_ARCH_KASAN if MMU && 64BIT
+ select GENERIC_EARLY_IOREMAP
config ARCH_MMAP_RND_BITS_MIN
default 18 if 64BIT
diff --git a/arch/riscv/include/asm/Kbuild b/arch/riscv/include/asm/Kbuild
index 3d9410bb4de0..59dd7be55005 100644
--- a/arch/riscv/include/asm/Kbuild
+++ b/arch/riscv/include/asm/Kbuild
@@ -1,4 +1,5 @@
# SPDX-License-Identifier: GPL-2.0
+generic-y += early_ioremap.h
generic-y += extable.h
generic-y += flat.h
generic-y += kvm_para.h
diff --git a/arch/riscv/include/asm/fixmap.h b/arch/riscv/include/asm/fixmap.h
index 2368d49eb4ef..ba5096d65fb0 100644
--- a/arch/riscv/include/asm/fixmap.h
+++ b/arch/riscv/include/asm/fixmap.h
@@ -30,6 +30,24 @@ enum fixed_addresses {
FIX_TEXT_POKE1,
FIX_TEXT_POKE0,
FIX_EARLYCON_MEM_BASE,
+ /*
+ * Make sure that it is 2MB aligned.
+ */
+#define NR_FIX_SZ_2M (SZ_2M / PAGE_SIZE)
+ FIX_THOLE = NR_FIX_SZ_2M - FIX_PMD - 1,
+
+ __end_of_permanent_fixed_addresses,
+ /*
+ * Temporary boot-time mappings, used by early_ioremap(),
+ * before ioremap() is functional.
+ */
+#define NR_FIX_BTMAPS (SZ_256K / PAGE_SIZE)
+#define FIX_BTMAPS_SLOTS 7
+#define TOTAL_FIX_BTMAPS (NR_FIX_BTMAPS * FIX_BTMAPS_SLOTS)
+
+ FIX_BTMAP_END = __end_of_permanent_fixed_addresses,
+ FIX_BTMAP_BEGIN = FIX_BTMAP_END + TOTAL_FIX_BTMAPS - 1,
+
__end_of_fixed_addresses
};
diff --git a/arch/riscv/include/asm/io.h b/arch/riscv/include/asm/io.h
index 0f477206a4ed..047f414b6948 100644
--- a/arch/riscv/include/asm/io.h
+++ b/arch/riscv/include/asm/io.h
@@ -14,6 +14,7 @@
#include <linux/types.h>
#include <asm/mmiowb.h>
#include <asm/pgtable.h>
+#include <asm/early_ioremap.h>
/*
* MMIO access functions are separated out to break dependency cycles
--
2.24.0
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [v3 PATCH 4/5] RISC-V: Add PE/COFF header for EFI stub
2020-04-15 19:54 [v3 PATCH 0/5] Add UEFI support for RISC-V Atish Patra
` (2 preceding siblings ...)
2020-04-15 19:54 ` [v3 PATCH 3/5] RISC-V: Define fixmap bindings for generic early ioremap support Atish Patra
@ 2020-04-15 19:54 ` Atish Patra
2020-04-15 19:54 ` [v3 PATCH 5/5] RISC-V: Add EFI stub support Atish Patra
2020-04-16 7:44 ` [v3 PATCH 0/5] Add UEFI support for RISC-V Ard Biesheuvel
5 siblings, 0 replies; 20+ messages in thread
From: Atish Patra @ 2020-04-15 19:54 UTC (permalink / raw)
To: linux-kernel
Cc: Atish Patra, Ard Biesheuvel, Arnd Bergmann, Catalin Marinas,
Greg Kroah-Hartman, linux-efi, linux-riscv, linux-arm-kernel,
Masahiro Yamada, Palmer Dabbelt, Russell King, Will Deacon
Linux kernel Image can appear as an EFI application With appropriate
PE/COFF header fields in the beginning of the Image header. An EFI
application loader can directly load a Linux kernel Image and an EFI
stub residing in kernel can boot Linux kernel directly.
Add the necessary PE/COFF header.
Signed-off-by: Atish Patra <atish.patra@wdc.com>
---
arch/riscv/include/asm/sections.h | 13 ++++
arch/riscv/kernel/Makefile | 4 ++
| 99 +++++++++++++++++++++++++++++++
arch/riscv/kernel/head.S | 16 +++++
arch/riscv/kernel/image-vars.h | 53 +++++++++++++++++
arch/riscv/kernel/vmlinux.lds.S | 20 ++++++-
6 files changed, 203 insertions(+), 2 deletions(-)
create mode 100644 arch/riscv/include/asm/sections.h
create mode 100644 arch/riscv/kernel/efi-header.S
create mode 100644 arch/riscv/kernel/image-vars.h
diff --git a/arch/riscv/include/asm/sections.h b/arch/riscv/include/asm/sections.h
new file mode 100644
index 000000000000..3a9971b1210f
--- /dev/null
+++ b/arch/riscv/include/asm/sections.h
@@ -0,0 +1,13 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (C) 2020 Western Digital Corporation or its affiliates.
+ */
+#ifndef __ASM_SECTIONS_H
+#define __ASM_SECTIONS_H
+
+#include <asm-generic/sections.h>
+
+extern char _start[];
+extern char _start_kernel[];
+
+#endif /* __ASM_SECTIONS_H */
diff --git a/arch/riscv/kernel/Makefile b/arch/riscv/kernel/Makefile
index 86c83081044f..86ca755f8a9f 100644
--- a/arch/riscv/kernel/Makefile
+++ b/arch/riscv/kernel/Makefile
@@ -32,6 +32,10 @@ obj-y += patch.o
obj-$(CONFIG_MMU) += vdso.o vdso/
obj-$(CONFIG_RISCV_M_MODE) += clint.o traps_misaligned.o
+OBJCOPYFLAGS := --prefix-symbols=__efistub_
+$(obj)/%.stub.o: $(obj)/%.o FORCE
+ $(call if_changed,objcopy)
+
obj-$(CONFIG_FPU) += fpu.o
obj-$(CONFIG_SMP) += smpboot.o
obj-$(CONFIG_SMP) += smp.o
--git a/arch/riscv/kernel/efi-header.S b/arch/riscv/kernel/efi-header.S
new file mode 100644
index 000000000000..69dde8268527
--- /dev/null
+++ b/arch/riscv/kernel/efi-header.S
@@ -0,0 +1,99 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (C) 2020 Western Digital Corporation or its affiliates.
+ * Adapted from arch/arm64/kernel/efi-header.S
+ */
+
+#include <linux/pe.h>
+#include <linux/sizes.h>
+
+ .macro __EFI_PE_HEADER
+ .long PE_MAGIC
+coff_header:
+ .short IMAGE_FILE_MACHINE_RISCV64 // Machine
+ .short section_count // NumberOfSections
+ .long 0 // TimeDateStamp
+ .long 0 // PointerToSymbolTable
+ .long 0 // NumberOfSymbols
+ .short section_table - optional_header // SizeOfOptionalHeader
+ .short IMAGE_FILE_DEBUG_STRIPPED | \
+ IMAGE_FILE_EXECUTABLE_IMAGE | \
+ IMAGE_FILE_LINE_NUMS_STRIPPED // Characteristics
+
+optional_header:
+ .short PE_OPT_MAGIC_PE32PLUS // PE32+ format
+ .byte 0x02 // MajorLinkerVersion
+ .byte 0x14 // MinorLinkerVersion
+ .long __text_end - efi_header_end // SizeOfCode
+ .long _end - __text_end // SizeOfInitializedData
+ .long 0 // SizeOfUninitializedData
+ .long __efistub_efi_entry - _start // AddressOfEntryPoint
+ .long efi_header_end - _start // BaseOfCode
+
+extra_header_fields:
+ .quad 0 // ImageBase
+ .long SZ_4K // SectionAlignment
+ .long PECOFF_FILE_ALIGNMENT // FileAlignment
+ .short 0 // MajorOperatingSystemVersion
+ .short 0 // MinorOperatingSystemVersion
+ .short LINUX_EFISTUB_MAJOR_VERSION // MajorImageVersion
+ .short LINUX_EFISTUB_MINOR_VERSION // MinorImageVersion
+ .short 0 // MajorSubsystemVersion
+ .short 0 // MinorSubsystemVersion
+ .long 0 // Win32VersionValue
+
+ .long _end - _start // SizeOfImage
+
+ // Everything before the kernel image is considered part of the header
+ .long efi_header_end - _start // SizeOfHeaders
+ .long 0 // CheckSum
+ .short IMAGE_SUBSYSTEM_EFI_APPLICATION // Subsystem
+ .short 0 // DllCharacteristics
+ .quad 0 // SizeOfStackReserve
+ .quad 0 // SizeOfStackCommit
+ .quad 0 // SizeOfHeapReserve
+ .quad 0 // SizeOfHeapCommit
+ .long 0 // LoaderFlags
+ .long (section_table - .) / 8 // NumberOfRvaAndSizes
+
+ .quad 0 // ExportTable
+ .quad 0 // ImportTable
+ .quad 0 // ResourceTable
+ .quad 0 // ExceptionTable
+ .quad 0 // CertificationTable
+ .quad 0 // BaseRelocationTable
+
+ // Section table
+section_table:
+ .ascii ".text\0\0\0"
+ .long __text_end - efi_header_end // VirtualSize
+ .long efi_header_end - _start // VirtualAddress
+ .long __text_end - efi_header_end // SizeOfRawData
+ .long efi_header_end - _start // PointerToRawData
+
+ .long 0 // PointerToRelocations
+ .long 0 // PointerToLineNumbers
+ .short 0 // NumberOfRelocations
+ .short 0 // NumberOfLineNumbers
+ .long IMAGE_SCN_CNT_CODE | \
+ IMAGE_SCN_MEM_READ | \
+ IMAGE_SCN_MEM_EXECUTE // Characteristics
+
+ .ascii ".data\0\0\0"
+ .long __data_virt_size // VirtualSize
+ .long __text_end - _start // VirtualAddress
+ .long __data_raw_size // SizeOfRawData
+ .long __text_end - _start // PointerToRawData
+
+ .long 0 // PointerToRelocations
+ .long 0 // PointerToLineNumbers
+ .short 0 // NumberOfRelocations
+ .short 0 // NumberOfLineNumbers
+ .long IMAGE_SCN_CNT_INITIALIZED_DATA | \
+ IMAGE_SCN_MEM_READ | \
+ IMAGE_SCN_MEM_WRITE // Characteristics
+
+ .set section_count, (. - section_table) / 40
+
+efi_header_end:
+ .endm
diff --git a/arch/riscv/kernel/head.S b/arch/riscv/kernel/head.S
index 98a406474e7d..ddd613dac9d6 100644
--- a/arch/riscv/kernel/head.S
+++ b/arch/riscv/kernel/head.S
@@ -13,6 +13,7 @@
#include <asm/csr.h>
#include <asm/hwcap.h>
#include <asm/image.h>
+#include "efi-header.S"
__HEAD
ENTRY(_start)
@@ -22,10 +23,18 @@ ENTRY(_start)
* Do not modify it without modifying the structure and all bootloaders
* that expects this header format!!
*/
+#ifdef CONFIG_EFI
+ /*
+ * This instruction decodes to "MZ" ASCII required by UEFI.
+ */
+ li s4,-13
+ j _start_kernel
+#else
/* jump to start kernel */
j _start_kernel
/* reserved */
.word 0
+#endif
.balign 8
#if __riscv_xlen == 64
/* Image load offset(2MB) from start of RAM */
@@ -43,7 +52,14 @@ ENTRY(_start)
.ascii RISCV_IMAGE_MAGIC
.balign 4
.ascii RISCV_IMAGE_MAGIC2
+#ifdef CONFIG_EFI
+ .word pe_head_start - _start
+pe_head_start:
+
+ __EFI_PE_HEADER
+#else
.word 0
+#endif
.align 2
#ifdef CONFIG_MMU
diff --git a/arch/riscv/kernel/image-vars.h b/arch/riscv/kernel/image-vars.h
new file mode 100644
index 000000000000..bd8b764f0ad9
--- /dev/null
+++ b/arch/riscv/kernel/image-vars.h
@@ -0,0 +1,53 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (C) 2020 Western Digital Corporation or its affiliates.
+ * Linker script variables to be set after section resolution, as
+ * ld.lld does not like variables assigned before SECTIONS is processed.
+ * Based on arch/arm64/kerne/image-vars.h
+ */
+#ifndef __RISCV_KERNEL_IMAGE_VARS_H
+#define __RISCV_KERNEL_IMAGE_VARS_H
+
+#ifndef LINKER_SCRIPT
+#error This file should only be included in vmlinux.lds.S
+#endif
+
+#ifdef CONFIG_EFI
+
+__efistub_stext_offset = _start_kernel - _start;
+
+/*
+ * The EFI stub has its own symbol namespace prefixed by __efistub_, to
+ * isolate it from the kernel proper. The following symbols are legally
+ * accessed by the stub, so provide some aliases to make them accessible.
+ * Only include data symbols here, or text symbols of functions that are
+ * guaranteed to be safe when executed at another offset than they were
+ * linked at. The routines below are all implemented in assembler in a
+ * position independent manner
+ */
+__efistub_memcmp = memcmp;
+__efistub_memchr = memchr;
+__efistub_memcpy = memcpy;
+__efistub_memmove = memmove;
+__efistub_memset = memset;
+__efistub_strlen = strlen;
+__efistub_strnlen = strnlen;
+__efistub_strcmp = strcmp;
+__efistub_strncmp = strncmp;
+__efistub_strrchr = strrchr;
+
+#ifdef CONFIG_KASAN
+__efistub___memcpy = memcpy;
+__efistub___memmove = memmove;
+__efistub___memset = memset;
+#endif
+
+__efistub__start = _start;
+__efistub__start_kernel = _start_kernel;
+__efistub__end = _end;
+__efistub__edata = _edata;
+__efistub_screen_info = screen_info;
+
+#endif
+
+#endif /* __RISCV_KERNEL_IMAGE_VARS_H */
diff --git a/arch/riscv/kernel/vmlinux.lds.S b/arch/riscv/kernel/vmlinux.lds.S
index 0339b6bbe11a..20ebf7e8c215 100644
--- a/arch/riscv/kernel/vmlinux.lds.S
+++ b/arch/riscv/kernel/vmlinux.lds.S
@@ -10,6 +10,7 @@
#include <asm/cache.h>
#include <asm/thread_info.h>
#include <asm/set_memory.h>
+#include "image-vars.h"
#include <linux/sizes.h>
OUTPUT_ARCH(riscv)
@@ -17,6 +18,14 @@ ENTRY(_start)
jiffies = jiffies_64;
+PECOFF_FILE_ALIGNMENT = 0x200;
+#ifdef CONFIG_EFI
+#define PECOFF_EDATA_PADDING \
+ .pecoff_edata_padding : { BYTE(0); . = ALIGN(PECOFF_FILE_ALIGNMENT); }
+#else
+#define PECOFF_EDATA_PADDING
+#endif
+
SECTIONS
{
/* Beginning of code and text segment */
@@ -62,6 +71,8 @@ SECTIONS
_etext = .;
}
+ __text_end = .;
+
/* Start of data section */
_sdata = .;
RO_DATA(SECTION_ALIGN)
@@ -78,9 +89,12 @@ SECTIONS
.sdata : {
__global_pointer$ = . + 0x800;
*(.sdata*)
- /* End of data section */
- _edata = .;
}
+ PECOFF_EDATA_PADDING
+ __data_raw_size = ABSOLUTE(. - __text_end);
+
+ /* End of data section */
+ _edata = .;
BSS_SECTION(PAGE_SIZE, PAGE_SIZE, 0)
@@ -88,6 +102,8 @@ SECTIONS
*(.rel.dyn*)
}
+ __data_virt_size = ABSOLUTE(. - __text_end);
+
_end = .;
STABS_DEBUG
--
2.24.0
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [v3 PATCH 5/5] RISC-V: Add EFI stub support.
2020-04-15 19:54 [v3 PATCH 0/5] Add UEFI support for RISC-V Atish Patra
` (3 preceding siblings ...)
2020-04-15 19:54 ` [v3 PATCH 4/5] RISC-V: Add PE/COFF header for EFI stub Atish Patra
@ 2020-04-15 19:54 ` Atish Patra
2020-04-16 1:13 ` kbuild test robot
2020-04-16 7:41 ` Ard Biesheuvel
2020-04-16 7:44 ` [v3 PATCH 0/5] Add UEFI support for RISC-V Ard Biesheuvel
5 siblings, 2 replies; 20+ messages in thread
From: Atish Patra @ 2020-04-15 19:54 UTC (permalink / raw)
To: linux-kernel
Cc: Atish Patra, Ard Biesheuvel, Arnd Bergmann, Catalin Marinas,
Greg Kroah-Hartman, linux-efi, linux-riscv, linux-arm-kernel,
Masahiro Yamada, Palmer Dabbelt, Russell King, Will Deacon
Add a RISC-V architecture specific stub code that actually copies the
actual kernel image to a valid address and jump to it after boot services
are terminated. Enable UEFI related kernel configs as well for RISC-V.
Signed-off-by: Atish Patra <atish.patra@wdc.com>
---
arch/riscv/Kconfig | 20 ++++
arch/riscv/Makefile | 1 +
arch/riscv/configs/defconfig | 1 +
arch/riscv/include/asm/efi.h | 44 +++++++++
drivers/firmware/efi/Kconfig | 2 +-
drivers/firmware/efi/libstub/Makefile | 7 ++
drivers/firmware/efi/libstub/riscv-stub.c | 111 ++++++++++++++++++++++
7 files changed, 185 insertions(+), 1 deletion(-)
create mode 100644 arch/riscv/include/asm/efi.h
create mode 100644 drivers/firmware/efi/libstub/riscv-stub.c
diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
index f39e326a7a42..eb4f41c8f3ce 100644
--- a/arch/riscv/Kconfig
+++ b/arch/riscv/Kconfig
@@ -379,10 +379,30 @@ config CMDLINE_FORCE
endchoice
+config EFI_STUB
+ bool
+
+config EFI
+ bool "UEFI runtime support"
+ depends on OF
+ select LIBFDT
+ select UCS2_STRING
+ select EFI_PARAMS_FROM_FDT
+ select EFI_STUB
+ select EFI_GENERIC_STUB
+ default y
+ help
+ This option provides support for runtime services provided
+ by UEFI firmware (such as non-volatile variables, realtime
+ clock, and platform reset). A UEFI stub is also provided to
+ allow the kernel to be booted as an EFI application. This
+ is only useful on systems that have UEFI firmware.
+
endmenu
menu "Power management options"
source "kernel/power/Kconfig"
+source "drivers/firmware/Kconfig"
endmenu
diff --git a/arch/riscv/Makefile b/arch/riscv/Makefile
index fb6e37db836d..079435804d6d 100644
--- a/arch/riscv/Makefile
+++ b/arch/riscv/Makefile
@@ -80,6 +80,7 @@ head-y := arch/riscv/kernel/head.o
core-y += arch/riscv/
libs-y += arch/riscv/lib/
+core-$(CONFIG_EFI_STUB) += $(objtree)/drivers/firmware/efi/libstub/lib.a
PHONY += vdso_install
vdso_install:
diff --git a/arch/riscv/configs/defconfig b/arch/riscv/configs/defconfig
index 4da4886246a4..ae69e12d306a 100644
--- a/arch/riscv/configs/defconfig
+++ b/arch/riscv/configs/defconfig
@@ -129,3 +129,4 @@ CONFIG_DEBUG_BLOCK_EXT_DEVT=y
# CONFIG_RUNTIME_TESTING_MENU is not set
CONFIG_MEMTEST=y
# CONFIG_SYSFS_SYSCALL is not set
+CONFIG_EFI=y
diff --git a/arch/riscv/include/asm/efi.h b/arch/riscv/include/asm/efi.h
new file mode 100644
index 000000000000..62d7d5eafed8
--- /dev/null
+++ b/arch/riscv/include/asm/efi.h
@@ -0,0 +1,44 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2020 Western Digital Corporation or its affiliates.
+ * Based on arch/arm64/include/asm/efi.h
+ */
+#ifndef _ASM_EFI_H
+#define _ASM_EFI_H
+
+#include <asm/io.h>
+#include <asm/mmu_context.h>
+#include <asm/ptrace.h>
+#include <asm/tlbflush.h>
+
+#define VA_BITS_MIN 39
+
+/* on RISC-V, the FDT may be located anywhere in system RAM */
+static inline unsigned long efi_get_max_fdt_addr(unsigned long dram_base)
+{
+ return ULONG_MAX;
+}
+
+/* Load initrd at enough distance from DRAM start */
+static inline unsigned long efi_get_max_initrd_addr(unsigned long dram_base,
+ unsigned long image_addr)
+{
+ return dram_base + SZ_256M;
+}
+
+#define efi_bs_call(func, ...) efi_system_table()->boottime->func(__VA_ARGS__)
+#define efi_rt_call(func, ...) efi_system_table()->runtime->func(__VA_ARGS__)
+#define efi_is_native() (true)
+
+#define efi_table_attr(inst, attr) (inst->attr)
+
+#define efi_call_proto(inst, func, ...) inst->func(inst, ##__VA_ARGS__)
+
+#define alloc_screen_info(x...) (&screen_info)
+extern char stext_offset[];
+
+static inline void free_screen_info(struct screen_info *si)
+{
+}
+
+#endif /* _ASM_EFI_H */
diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
index 2a2b2b96a1dc..fcdc789d3f87 100644
--- a/drivers/firmware/efi/Kconfig
+++ b/drivers/firmware/efi/Kconfig
@@ -111,7 +111,7 @@ config EFI_GENERIC_STUB
config EFI_ARMSTUB_DTB_LOADER
bool "Enable the DTB loader"
- depends on EFI_GENERIC_STUB
+ depends on EFI_GENERIC_STUB && !RISCV
default y
help
Select this config option to add support for the dtb= command
diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile
index 2b4e09bf987c..7d46b70b51f2 100644
--- a/drivers/firmware/efi/libstub/Makefile
+++ b/drivers/firmware/efi/libstub/Makefile
@@ -22,6 +22,8 @@ cflags-$(CONFIG_ARM64) := $(subst $(CC_FLAGS_FTRACE),,$(KBUILD_CFLAGS)) \
cflags-$(CONFIG_ARM) := $(subst $(CC_FLAGS_FTRACE),,$(KBUILD_CFLAGS)) \
-fno-builtin -fpic \
$(call cc-option,-mno-single-pic-base)
+cflags-$(CONFIG_RISCV) := $(subst $(CC_FLAGS_FTRACE),,$(KBUILD_CFLAGS)) \
+ -fpic
cflags-$(CONFIG_EFI_GENERIC_STUB) += -I$(srctree)/scripts/dtc/libfdt
@@ -57,6 +59,7 @@ lib-$(CONFIG_EFI_GENERIC_STUB) += efi-stub.o fdt.o string.o \
lib-$(CONFIG_ARM) += arm32-stub.o
lib-$(CONFIG_ARM64) += arm64-stub.o
lib-$(CONFIG_X86) += x86-stub.o
+lib-$(CONFIG_RISCV) += riscv-stub.o
CFLAGS_arm32-stub.o := -DTEXT_OFFSET=$(TEXT_OFFSET)
CFLAGS_arm64-stub.o := -DTEXT_OFFSET=$(TEXT_OFFSET)
@@ -81,6 +84,10 @@ STUBCOPY_FLAGS-$(CONFIG_ARM64) += --prefix-alloc-sections=.init \
--prefix-symbols=__efistub_
STUBCOPY_RELOC-$(CONFIG_ARM64) := R_AARCH64_ABS
+STUBCOPY_FLAGS-$(CONFIG_RISCV) += --prefix-alloc-sections=.init \
+ --prefix-symbols=__efistub_
+STUBCOPY_RELOC-$(CONFIG_RISCV) := R_RISCV_HI20
+
$(obj)/%.stub.o: $(obj)/%.o FORCE
$(call if_changed,stubcopy)
diff --git a/drivers/firmware/efi/libstub/riscv-stub.c b/drivers/firmware/efi/libstub/riscv-stub.c
new file mode 100644
index 000000000000..69d13e0ebaea
--- /dev/null
+++ b/drivers/firmware/efi/libstub/riscv-stub.c
@@ -0,0 +1,111 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2013, 2014 Linaro Ltd; <roy.franz@linaro.org>
+ * Copyright (C) 2020 Western Digital Corporation or its affiliates.
+ *
+ * This file implements the EFI boot stub for the RISC-V kernel.
+ * Adapted from ARM64 version at drivers/firmware/efi/libstub/arm64-stub.c.
+ */
+
+#include <linux/efi.h>
+#include <linux/libfdt.h>
+#include <linux/libfdt_env.h>
+#include <asm/efi.h>
+#include <asm/sections.h>
+
+#include "efistub.h"
+/*
+ * RISCV requires the kernel image to placed TEXT_OFFSET bytes beyond a 2 MB
+ * aligned base for 64 bit and 4MB for 32 bit.
+ */
+#ifdef CONFIG_64BIT
+#define MIN_KIMG_ALIGN SZ_2M
+#else
+#define MIN_KIMG_ALIGN SZ_4M
+#endif
+
+typedef __attribute__((noreturn)) void (*jump_kernel_func)(unsigned int,
+ unsigned long);
+efi_status_t check_platform_features(void)
+{
+ return EFI_SUCCESS;
+}
+
+static u32 get_boot_hartid_from_fdt(unsigned long fdt)
+{
+ int chosen_node, len;
+ const fdt32_t *prop;
+
+ chosen_node = fdt_path_offset((void *)fdt, "/chosen");
+ if (chosen_node < 0)
+ return U32_MAX;
+ prop = fdt_getprop((void *)fdt, chosen_node, "boot-hartid", &len);
+ if (!prop || len != sizeof(u32))
+ return U32_MAX;
+
+ return fdt32_to_cpu(*prop);
+}
+
+/*
+ * Jump to real kernel here with following constraints.
+ * 1. MMU should be disabled.
+ * 2. a0 should contain hartid
+ * 3. a1 should DT address
+ */
+void __noreturn efi_enter_kernel(unsigned long entrypoint, unsigned long fdt,
+ unsigned long fdt_size)
+{
+ unsigned long kernel_entry = entrypoint + (unsigned long)stext_offset;
+ jump_kernel_func jump_kernel = (jump_kernel_func) kernel_entry;
+ u32 hartid = get_boot_hartid_from_fdt(fdt);
+
+ if (hartid == U32_MAX)
+ /* We can not use panic or BUG at this point */
+ __asm__ __volatile__ ("ebreak");
+ /* Disable MMU */
+ csr_write(CSR_SATP, 0);
+ jump_kernel(hartid, fdt);
+}
+
+efi_status_t handle_kernel_image(unsigned long *image_addr,
+ unsigned long *image_size,
+ unsigned long *reserve_addr,
+ unsigned long *reserve_size,
+ unsigned long dram_base,
+ efi_loaded_image_t *image)
+{
+ efi_status_t status;
+ unsigned long kernel_size, kernel_memsize = 0;
+ unsigned long max_alloc_address;
+
+ if (image->image_base != _start)
+ pr_efi_err("FIRMWARE BUG: efi_loaded_image_t::image_base has bogus value\n");
+
+ kernel_size = _edata - _start;
+ kernel_memsize = kernel_size + (_end - _edata);
+ max_alloc_address = round_up(dram_base, MIN_KIMG_ALIGN) +
+ kernel_memsize;
+
+ if (IS_ALIGNED((u64)_start, MIN_KIMG_ALIGN)) {
+ /*
+ * Just execute from wherever we were loaded by the
+ * UEFI PE/COFF loader if the alignment is suitable.
+ */
+ *image_addr = (u64)_start;
+ *reserve_size = 0;
+ return EFI_SUCCESS;
+ }
+ status = efi_allocate_pages_aligned(*reserve_size, reserve_addr,
+ max_alloc_address, MIN_KIMG_ALIGN);
+
+ if (status != EFI_SUCCESS) {
+ pr_efi_err("Failed to relocate kernel\n");
+ *reserve_size = 0;
+ return status;
+ }
+ *image_addr = *reserve_addr;
+
+ memcpy((void *)*image_addr, _start, kernel_size);
+
+ return EFI_SUCCESS;
+}
--
2.24.0
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [v3 PATCH 5/5] RISC-V: Add EFI stub support.
2020-04-15 19:54 ` [v3 PATCH 5/5] RISC-V: Add EFI stub support Atish Patra
@ 2020-04-16 1:13 ` kbuild test robot
2020-04-16 7:41 ` Ard Biesheuvel
1 sibling, 0 replies; 20+ messages in thread
From: kbuild test robot @ 2020-04-16 1:13 UTC (permalink / raw)
To: Atish Patra, linux-kernel
Cc: kbuild-all, Atish Patra, Ard Biesheuvel, Arnd Bergmann,
Catalin Marinas, Greg Kroah-Hartman, linux-efi, linux-riscv,
linux-arm-kernel, Masahiro Yamada
[-- Attachment #1: Type: text/plain, Size: 3673 bytes --]
Hi Atish,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on efi/next]
[also build test WARNING on linus/master v5.7-rc1 next-20200415]
[cannot apply to arm/for-next arm64/for-next/core linux/master atish-riscv-linux/topo_v3]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Atish-Patra/Add-UEFI-support-for-RISC-V/20200416-065438
base: https://git.kernel.org/pub/scm/linux/kernel/git/efi/efi.git next
config: riscv-rv32_defconfig (attached as .config)
compiler: riscv32-linux-gcc (GCC) 9.3.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day GCC_VERSION=9.3.0 make.cross ARCH=riscv
If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
In file included from include/linux/list.h:9,
from include/linux/preempt.h:11,
from include/linux/spinlock.h:51,
from include/linux/seqlock.h:36,
from include/linux/time.h:6,
from include/linux/efi.h:17,
from drivers/firmware/efi/libstub/riscv-stub.c:10:
drivers/firmware/efi/libstub/riscv-stub.c: In function 'handle_kernel_image':
>> drivers/firmware/efi/libstub/riscv-stub.c:89:17: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
89 | if (IS_ALIGNED((u64)_start, MIN_KIMG_ALIGN)) {
| ^
include/linux/kernel.h:37:30: note: in definition of macro 'IS_ALIGNED'
37 | #define IS_ALIGNED(x, a) (((x) & ((typeof(x))(a) - 1)) == 0)
| ^
>> drivers/firmware/efi/libstub/riscv-stub.c:89:17: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
89 | if (IS_ALIGNED((u64)_start, MIN_KIMG_ALIGN)) {
| ^
include/linux/kernel.h:37:44: note: in definition of macro 'IS_ALIGNED'
37 | #define IS_ALIGNED(x, a) (((x) & ((typeof(x))(a) - 1)) == 0)
| ^
drivers/firmware/efi/libstub/riscv-stub.c:94:17: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
94 | *image_addr = (u64)_start;
| ^
vim +89 drivers/firmware/efi/libstub/riscv-stub.c
69
70 efi_status_t handle_kernel_image(unsigned long *image_addr,
71 unsigned long *image_size,
72 unsigned long *reserve_addr,
73 unsigned long *reserve_size,
74 unsigned long dram_base,
75 efi_loaded_image_t *image)
76 {
77 efi_status_t status;
78 unsigned long kernel_size, kernel_memsize = 0;
79 unsigned long max_alloc_address;
80
81 if (image->image_base != _start)
82 pr_efi_err("FIRMWARE BUG: efi_loaded_image_t::image_base has bogus value\n");
83
84 kernel_size = _edata - _start;
85 kernel_memsize = kernel_size + (_end - _edata);
86 max_alloc_address = round_up(dram_base, MIN_KIMG_ALIGN) +
87 kernel_memsize;
88
> 89 if (IS_ALIGNED((u64)_start, MIN_KIMG_ALIGN)) {
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 19000 bytes --]
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [v3 PATCH 5/5] RISC-V: Add EFI stub support.
2020-04-15 19:54 ` [v3 PATCH 5/5] RISC-V: Add EFI stub support Atish Patra
2020-04-16 1:13 ` kbuild test robot
@ 2020-04-16 7:41 ` Ard Biesheuvel
2020-04-18 3:03 ` Atish Patra
1 sibling, 1 reply; 20+ messages in thread
From: Ard Biesheuvel @ 2020-04-16 7:41 UTC (permalink / raw)
To: Atish Patra
Cc: Linux Kernel Mailing List, Arnd Bergmann, Catalin Marinas,
Greg Kroah-Hartman, linux-efi, linux-riscv, Linux ARM,
Masahiro Yamada, Palmer Dabbelt, Russell King, Will Deacon
On Wed, 15 Apr 2020 at 21:54, Atish Patra <atish.patra@wdc.com> wrote:
>
> Add a RISC-V architecture specific stub code that actually copies the
> actual kernel image to a valid address and jump to it after boot services
> are terminated. Enable UEFI related kernel configs as well for RISC-V.
>
> Signed-off-by: Atish Patra <atish.patra@wdc.com>
> ---
> arch/riscv/Kconfig | 20 ++++
> arch/riscv/Makefile | 1 +
> arch/riscv/configs/defconfig | 1 +
> arch/riscv/include/asm/efi.h | 44 +++++++++
> drivers/firmware/efi/Kconfig | 2 +-
> drivers/firmware/efi/libstub/Makefile | 7 ++
> drivers/firmware/efi/libstub/riscv-stub.c | 111 ++++++++++++++++++++++
> 7 files changed, 185 insertions(+), 1 deletion(-)
> create mode 100644 arch/riscv/include/asm/efi.h
> create mode 100644 drivers/firmware/efi/libstub/riscv-stub.c
>
> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> index f39e326a7a42..eb4f41c8f3ce 100644
> --- a/arch/riscv/Kconfig
> +++ b/arch/riscv/Kconfig
> @@ -379,10 +379,30 @@ config CMDLINE_FORCE
>
> endchoice
>
> +config EFI_STUB
> + bool
> +
> +config EFI
> + bool "UEFI runtime support"
> + depends on OF
> + select LIBFDT
> + select UCS2_STRING
> + select EFI_PARAMS_FROM_FDT
> + select EFI_STUB
> + select EFI_GENERIC_STUB
> + default y
> + help
> + This option provides support for runtime services provided
> + by UEFI firmware (such as non-volatile variables, realtime
> + clock, and platform reset). A UEFI stub is also provided to
> + allow the kernel to be booted as an EFI application. This
> + is only useful on systems that have UEFI firmware.
> +
> endmenu
>
> menu "Power management options"
>
> source "kernel/power/Kconfig"
> +source "drivers/firmware/Kconfig"
>
> endmenu
> diff --git a/arch/riscv/Makefile b/arch/riscv/Makefile
> index fb6e37db836d..079435804d6d 100644
> --- a/arch/riscv/Makefile
> +++ b/arch/riscv/Makefile
> @@ -80,6 +80,7 @@ head-y := arch/riscv/kernel/head.o
> core-y += arch/riscv/
>
> libs-y += arch/riscv/lib/
> +core-$(CONFIG_EFI_STUB) += $(objtree)/drivers/firmware/efi/libstub/lib.a
>
> PHONY += vdso_install
> vdso_install:
> diff --git a/arch/riscv/configs/defconfig b/arch/riscv/configs/defconfig
> index 4da4886246a4..ae69e12d306a 100644
> --- a/arch/riscv/configs/defconfig
> +++ b/arch/riscv/configs/defconfig
> @@ -129,3 +129,4 @@ CONFIG_DEBUG_BLOCK_EXT_DEVT=y
> # CONFIG_RUNTIME_TESTING_MENU is not set
> CONFIG_MEMTEST=y
> # CONFIG_SYSFS_SYSCALL is not set
> +CONFIG_EFI=y
> diff --git a/arch/riscv/include/asm/efi.h b/arch/riscv/include/asm/efi.h
> new file mode 100644
> index 000000000000..62d7d5eafed8
> --- /dev/null
> +++ b/arch/riscv/include/asm/efi.h
> @@ -0,0 +1,44 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (C) 2020 Western Digital Corporation or its affiliates.
> + * Based on arch/arm64/include/asm/efi.h
> + */
> +#ifndef _ASM_EFI_H
> +#define _ASM_EFI_H
> +
> +#include <asm/io.h>
> +#include <asm/mmu_context.h>
> +#include <asm/ptrace.h>
> +#include <asm/tlbflush.h>
> +
> +#define VA_BITS_MIN 39
> +
> +/* on RISC-V, the FDT may be located anywhere in system RAM */
> +static inline unsigned long efi_get_max_fdt_addr(unsigned long dram_base)
> +{
> + return ULONG_MAX;
> +}
> +
> +/* Load initrd at enough distance from DRAM start */
> +static inline unsigned long efi_get_max_initrd_addr(unsigned long dram_base,
> + unsigned long image_addr)
> +{
> + return dram_base + SZ_256M;
> +}
> +
> +#define efi_bs_call(func, ...) efi_system_table()->boottime->func(__VA_ARGS__)
> +#define efi_rt_call(func, ...) efi_system_table()->runtime->func(__VA_ARGS__)
> +#define efi_is_native() (true)
> +
> +#define efi_table_attr(inst, attr) (inst->attr)
> +
> +#define efi_call_proto(inst, func, ...) inst->func(inst, ##__VA_ARGS__)
> +
> +#define alloc_screen_info(x...) (&screen_info)
> +extern char stext_offset[];
> +
> +static inline void free_screen_info(struct screen_info *si)
> +{
> +}
> +
> +#endif /* _ASM_EFI_H */
> diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
> index 2a2b2b96a1dc..fcdc789d3f87 100644
> --- a/drivers/firmware/efi/Kconfig
> +++ b/drivers/firmware/efi/Kconfig
> @@ -111,7 +111,7 @@ config EFI_GENERIC_STUB
>
> config EFI_ARMSTUB_DTB_LOADER
> bool "Enable the DTB loader"
> - depends on EFI_GENERIC_STUB
> + depends on EFI_GENERIC_STUB && !RISCV
> default y
> help
> Select this config option to add support for the dtb= command
> diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile
> index 2b4e09bf987c..7d46b70b51f2 100644
> --- a/drivers/firmware/efi/libstub/Makefile
> +++ b/drivers/firmware/efi/libstub/Makefile
> @@ -22,6 +22,8 @@ cflags-$(CONFIG_ARM64) := $(subst $(CC_FLAGS_FTRACE),,$(KBUILD_CFLAGS)) \
> cflags-$(CONFIG_ARM) := $(subst $(CC_FLAGS_FTRACE),,$(KBUILD_CFLAGS)) \
> -fno-builtin -fpic \
> $(call cc-option,-mno-single-pic-base)
> +cflags-$(CONFIG_RISCV) := $(subst $(CC_FLAGS_FTRACE),,$(KBUILD_CFLAGS)) \
> + -fpic
>
> cflags-$(CONFIG_EFI_GENERIC_STUB) += -I$(srctree)/scripts/dtc/libfdt
>
> @@ -57,6 +59,7 @@ lib-$(CONFIG_EFI_GENERIC_STUB) += efi-stub.o fdt.o string.o \
> lib-$(CONFIG_ARM) += arm32-stub.o
> lib-$(CONFIG_ARM64) += arm64-stub.o
> lib-$(CONFIG_X86) += x86-stub.o
> +lib-$(CONFIG_RISCV) += riscv-stub.o
> CFLAGS_arm32-stub.o := -DTEXT_OFFSET=$(TEXT_OFFSET)
> CFLAGS_arm64-stub.o := -DTEXT_OFFSET=$(TEXT_OFFSET)
>
> @@ -81,6 +84,10 @@ STUBCOPY_FLAGS-$(CONFIG_ARM64) += --prefix-alloc-sections=.init \
> --prefix-symbols=__efistub_
> STUBCOPY_RELOC-$(CONFIG_ARM64) := R_AARCH64_ABS
>
> +STUBCOPY_FLAGS-$(CONFIG_RISCV) += --prefix-alloc-sections=.init \
> + --prefix-symbols=__efistub_
> +STUBCOPY_RELOC-$(CONFIG_RISCV) := R_RISCV_HI20
> +
> $(obj)/%.stub.o: $(obj)/%.o FORCE
> $(call if_changed,stubcopy)
>
> diff --git a/drivers/firmware/efi/libstub/riscv-stub.c b/drivers/firmware/efi/libstub/riscv-stub.c
> new file mode 100644
> index 000000000000..69d13e0ebaea
> --- /dev/null
> +++ b/drivers/firmware/efi/libstub/riscv-stub.c
> @@ -0,0 +1,111 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2013, 2014 Linaro Ltd; <roy.franz@linaro.org>
> + * Copyright (C) 2020 Western Digital Corporation or its affiliates.
> + *
> + * This file implements the EFI boot stub for the RISC-V kernel.
> + * Adapted from ARM64 version at drivers/firmware/efi/libstub/arm64-stub.c.
> + */
> +
> +#include <linux/efi.h>
> +#include <linux/libfdt.h>
> +#include <linux/libfdt_env.h>
> +#include <asm/efi.h>
> +#include <asm/sections.h>
> +
> +#include "efistub.h"
> +/*
> + * RISCV requires the kernel image to placed TEXT_OFFSET bytes beyond a 2 MB
> + * aligned base for 64 bit and 4MB for 32 bit.
> + */
> +#ifdef CONFIG_64BIT
> +#define MIN_KIMG_ALIGN SZ_2M
> +#else
> +#define MIN_KIMG_ALIGN SZ_4M
> +#endif
> +
> +typedef __attribute__((noreturn)) void (*jump_kernel_func)(unsigned int,
> + unsigned long);
> +efi_status_t check_platform_features(void)
> +{
> + return EFI_SUCCESS;
> +}
> +
> +static u32 get_boot_hartid_from_fdt(unsigned long fdt)
> +{
> + int chosen_node, len;
> + const fdt32_t *prop;
> +
> + chosen_node = fdt_path_offset((void *)fdt, "/chosen");
> + if (chosen_node < 0)
> + return U32_MAX;
> + prop = fdt_getprop((void *)fdt, chosen_node, "boot-hartid", &len);
> + if (!prop || len != sizeof(u32))
> + return U32_MAX;
> +
> + return fdt32_to_cpu(*prop);
> +}
> +
> +/*
> + * Jump to real kernel here with following constraints.
> + * 1. MMU should be disabled.
> + * 2. a0 should contain hartid
> + * 3. a1 should DT address
> + */
> +void __noreturn efi_enter_kernel(unsigned long entrypoint, unsigned long fdt,
> + unsigned long fdt_size)
> +{
> + unsigned long kernel_entry = entrypoint + (unsigned long)stext_offset;
> + jump_kernel_func jump_kernel = (jump_kernel_func) kernel_entry;
> + u32 hartid = get_boot_hartid_from_fdt(fdt);
> +
> + if (hartid == U32_MAX)
> + /* We can not use panic or BUG at this point */
> + __asm__ __volatile__ ("ebreak");
> + /* Disable MMU */
> + csr_write(CSR_SATP, 0);
> + jump_kernel(hartid, fdt);
> +}
> +
> +efi_status_t handle_kernel_image(unsigned long *image_addr,
> + unsigned long *image_size,
> + unsigned long *reserve_addr,
> + unsigned long *reserve_size,
> + unsigned long dram_base,
> + efi_loaded_image_t *image)
> +{
> + efi_status_t status;
> + unsigned long kernel_size, kernel_memsize = 0;
> + unsigned long max_alloc_address;
> +
> + if (image->image_base != _start)
> + pr_efi_err("FIRMWARE BUG: efi_loaded_image_t::image_base has bogus value\n");
> +
I don't think you need this.
> + kernel_size = _edata - _start;
> + kernel_memsize = kernel_size + (_end - _edata);
> + max_alloc_address = round_up(dram_base, MIN_KIMG_ALIGN) +
> + kernel_memsize;
> +
You said the kernel could be anywhere in memory, as long as it is
aligned correctly, right?
In that case, you don't need this, you can simply pass ULONG_MAX as
the max address.
> + if (IS_ALIGNED((u64)_start, MIN_KIMG_ALIGN)) {
> + /*
> + * Just execute from wherever we were loaded by the
> + * UEFI PE/COFF loader if the alignment is suitable.
> + */
> + *image_addr = (u64)_start;
Change these casts to (unsigned long), as reported by the robot.
So you no longer need the placement to be TEXT_OFFSET bytes past a
MIN_KIMG_ALIGN aligned boundary, right?
> + *reserve_size = 0;
> + return EFI_SUCCESS;
> + }
> + status = efi_allocate_pages_aligned(*reserve_size, reserve_addr,
> + max_alloc_address, MIN_KIMG_ALIGN);
> +
> + if (status != EFI_SUCCESS) {
> + pr_efi_err("Failed to relocate kernel\n");
> + *reserve_size = 0;
> + return status;
> + }
> + *image_addr = *reserve_addr;
> +
> + memcpy((void *)*image_addr, _start, kernel_size);
> +
> + return EFI_SUCCESS;
> +}
> --
> 2.24.0
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [v3 PATCH 0/5] Add UEFI support for RISC-V
2020-04-15 19:54 [v3 PATCH 0/5] Add UEFI support for RISC-V Atish Patra
` (4 preceding siblings ...)
2020-04-15 19:54 ` [v3 PATCH 5/5] RISC-V: Add EFI stub support Atish Patra
@ 2020-04-16 7:44 ` Ard Biesheuvel
5 siblings, 0 replies; 20+ messages in thread
From: Ard Biesheuvel @ 2020-04-16 7:44 UTC (permalink / raw)
To: Atish Patra
Cc: Linux Kernel Mailing List, Arnd Bergmann, Catalin Marinas,
Greg Kroah-Hartman, linux-efi, linux-riscv, Linux ARM,
Masahiro Yamada, Palmer Dabbelt, Russell King, Will Deacon
(+ Arvind)
On Wed, 15 Apr 2020 at 21:54, Atish Patra <atish.patra@wdc.com> wrote:
>
> This series adds UEFI support for RISC-V. Currently, only boot time
> services have been added. Runtime services will be added in a separate
> series. This series depends on some core EFI patches
> present in current in efi-next and following other patches.
>
> U-Boot: Adds the boot hartid under chosen node.
> https://lists.denx.de/pipermail/u-boot/2020-April/405726.html
>
> Linux kernel: 5.7-rc1
>
> OpenSBI: master
>
> Patch 1 just moves arm-stub code to a generic code so that it can be used
> across different architecture.
>
> Patch 3 adds fixmap bindings so that CONFIG_EFI can be compiled and we do not
> have create separate config to enable boot time services.
> As runtime services are not enabled at this time, full generic early ioremap
> support is also not added in this series.
>
> Patch 4 and 5 adds the PE/COFF header and EFI stub code support for RISC-V
> respectively.
>
> The patches can also be found in following git repo.
>
> https://github.com/atishp04/linux/tree/wip_uefi_riscv_v3
>
> The patches have been verified on Qemu using bootefi command in U-Boot.
>
> Changes from v2->v3:
> 1. Rebased on top of latest efi patches.
> 2. Improved handle_kernel_image().
>
> Changes from v1->v2:
> 1. Rebased on 5.7-rc1.
> 2. Fixed minor typos and removed redundant macros/comments.
>
> Changes from previous version:
> 1. Renamed to the generic efi stub macro.
> 2. Address all redundant comments.
> 3. Supported EFI kernel image with normal booti command.
> 4. Removed runtime service related macro defines.
>
> Atish Patra (5):
> efi: Move arm-stub to a common file
> include: pe.h: Add RISC-V related PE definition
> RISC-V: Define fixmap bindings for generic early ioremap support
> RISC-V: Add PE/COFF header for EFI stub
> RISC-V: Add EFI stub support.
>
I had some comments regarding patch #5, but the other ones look fine.
Given that there are two series in flight now that touch the same
code, I am going to merge the patches #1 and #2 of this series into
efi/next.
Once the remaining changes are good to go, I can take them as well,
but I'll need one of the RISC-V maintainers to ack them first.
> arch/arm/Kconfig | 2 +-
> arch/arm64/Kconfig | 2 +-
> arch/riscv/Kconfig | 21 ++++
> arch/riscv/Makefile | 1 +
> arch/riscv/configs/defconfig | 1 +
> arch/riscv/include/asm/Kbuild | 1 +
> arch/riscv/include/asm/efi.h | 44 +++++++
> arch/riscv/include/asm/fixmap.h | 18 +++
> arch/riscv/include/asm/io.h | 1 +
> arch/riscv/include/asm/sections.h | 13 ++
> arch/riscv/kernel/Makefile | 4 +
> arch/riscv/kernel/efi-header.S | 99 ++++++++++++++++
> arch/riscv/kernel/head.S | 16 +++
> arch/riscv/kernel/image-vars.h | 53 +++++++++
> arch/riscv/kernel/vmlinux.lds.S | 20 +++-
> drivers/firmware/efi/Kconfig | 4 +-
> drivers/firmware/efi/libstub/Makefile | 19 ++-
> .../efi/libstub/{arm-stub.c => efi-stub.c} | 0
> drivers/firmware/efi/libstub/riscv-stub.c | 111 ++++++++++++++++++
> include/linux/pe.h | 3 +
> 20 files changed, 421 insertions(+), 12 deletions(-)
> create mode 100644 arch/riscv/include/asm/efi.h
> create mode 100644 arch/riscv/include/asm/sections.h
> create mode 100644 arch/riscv/kernel/efi-header.S
> create mode 100644 arch/riscv/kernel/image-vars.h
> rename drivers/firmware/efi/libstub/{arm-stub.c => efi-stub.c} (100%)
> create mode 100644 drivers/firmware/efi/libstub/riscv-stub.c
>
> --
> 2.24.0
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [v3 PATCH 5/5] RISC-V: Add EFI stub support.
2020-04-16 7:41 ` Ard Biesheuvel
@ 2020-04-18 3:03 ` Atish Patra
2020-04-18 10:51 ` Ard Biesheuvel
0 siblings, 1 reply; 20+ messages in thread
From: Atish Patra @ 2020-04-18 3:03 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Atish Patra, linux-efi, Arnd Bergmann, Catalin Marinas,
Masahiro Yamada, Linux Kernel Mailing List, Russell King,
Palmer Dabbelt, Greg Kroah-Hartman, linux-riscv, Will Deacon,
Linux ARM
On Thu, Apr 16, 2020 at 12:41 AM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Wed, 15 Apr 2020 at 21:54, Atish Patra <atish.patra@wdc.com> wrote:
> >
> > Add a RISC-V architecture specific stub code that actually copies the
> > actual kernel image to a valid address and jump to it after boot services
> > are terminated. Enable UEFI related kernel configs as well for RISC-V.
> >
> > Signed-off-by: Atish Patra <atish.patra@wdc.com>
> > ---
> > arch/riscv/Kconfig | 20 ++++
> > arch/riscv/Makefile | 1 +
> > arch/riscv/configs/defconfig | 1 +
> > arch/riscv/include/asm/efi.h | 44 +++++++++
> > drivers/firmware/efi/Kconfig | 2 +-
> > drivers/firmware/efi/libstub/Makefile | 7 ++
> > drivers/firmware/efi/libstub/riscv-stub.c | 111 ++++++++++++++++++++++
> > 7 files changed, 185 insertions(+), 1 deletion(-)
> > create mode 100644 arch/riscv/include/asm/efi.h
> > create mode 100644 drivers/firmware/efi/libstub/riscv-stub.c
> >
> > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> > index f39e326a7a42..eb4f41c8f3ce 100644
> > --- a/arch/riscv/Kconfig
> > +++ b/arch/riscv/Kconfig
> > @@ -379,10 +379,30 @@ config CMDLINE_FORCE
> >
> > endchoice
> >
> > +config EFI_STUB
> > + bool
> > +
> > +config EFI
> > + bool "UEFI runtime support"
> > + depends on OF
> > + select LIBFDT
> > + select UCS2_STRING
> > + select EFI_PARAMS_FROM_FDT
> > + select EFI_STUB
> > + select EFI_GENERIC_STUB
> > + default y
> > + help
> > + This option provides support for runtime services provided
> > + by UEFI firmware (such as non-volatile variables, realtime
> > + clock, and platform reset). A UEFI stub is also provided to
> > + allow the kernel to be booted as an EFI application. This
> > + is only useful on systems that have UEFI firmware.
> > +
> > endmenu
> >
> > menu "Power management options"
> >
> > source "kernel/power/Kconfig"
> > +source "drivers/firmware/Kconfig"
> >
> > endmenu
> > diff --git a/arch/riscv/Makefile b/arch/riscv/Makefile
> > index fb6e37db836d..079435804d6d 100644
> > --- a/arch/riscv/Makefile
> > +++ b/arch/riscv/Makefile
> > @@ -80,6 +80,7 @@ head-y := arch/riscv/kernel/head.o
> > core-y += arch/riscv/
> >
> > libs-y += arch/riscv/lib/
> > +core-$(CONFIG_EFI_STUB) += $(objtree)/drivers/firmware/efi/libstub/lib.a
> >
> > PHONY += vdso_install
> > vdso_install:
> > diff --git a/arch/riscv/configs/defconfig b/arch/riscv/configs/defconfig
> > index 4da4886246a4..ae69e12d306a 100644
> > --- a/arch/riscv/configs/defconfig
> > +++ b/arch/riscv/configs/defconfig
> > @@ -129,3 +129,4 @@ CONFIG_DEBUG_BLOCK_EXT_DEVT=y
> > # CONFIG_RUNTIME_TESTING_MENU is not set
> > CONFIG_MEMTEST=y
> > # CONFIG_SYSFS_SYSCALL is not set
> > +CONFIG_EFI=y
> > diff --git a/arch/riscv/include/asm/efi.h b/arch/riscv/include/asm/efi.h
> > new file mode 100644
> > index 000000000000..62d7d5eafed8
> > --- /dev/null
> > +++ b/arch/riscv/include/asm/efi.h
> > @@ -0,0 +1,44 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Copyright (C) 2020 Western Digital Corporation or its affiliates.
> > + * Based on arch/arm64/include/asm/efi.h
> > + */
> > +#ifndef _ASM_EFI_H
> > +#define _ASM_EFI_H
> > +
> > +#include <asm/io.h>
> > +#include <asm/mmu_context.h>
> > +#include <asm/ptrace.h>
> > +#include <asm/tlbflush.h>
> > +
> > +#define VA_BITS_MIN 39
> > +
> > +/* on RISC-V, the FDT may be located anywhere in system RAM */
> > +static inline unsigned long efi_get_max_fdt_addr(unsigned long dram_base)
> > +{
> > + return ULONG_MAX;
> > +}
> > +
> > +/* Load initrd at enough distance from DRAM start */
> > +static inline unsigned long efi_get_max_initrd_addr(unsigned long dram_base,
> > + unsigned long image_addr)
> > +{
> > + return dram_base + SZ_256M;
> > +}
> > +
> > +#define efi_bs_call(func, ...) efi_system_table()->boottime->func(__VA_ARGS__)
> > +#define efi_rt_call(func, ...) efi_system_table()->runtime->func(__VA_ARGS__)
> > +#define efi_is_native() (true)
> > +
> > +#define efi_table_attr(inst, attr) (inst->attr)
> > +
> > +#define efi_call_proto(inst, func, ...) inst->func(inst, ##__VA_ARGS__)
> > +
> > +#define alloc_screen_info(x...) (&screen_info)
> > +extern char stext_offset[];
> > +
> > +static inline void free_screen_info(struct screen_info *si)
> > +{
> > +}
> > +
> > +#endif /* _ASM_EFI_H */
> > diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
> > index 2a2b2b96a1dc..fcdc789d3f87 100644
> > --- a/drivers/firmware/efi/Kconfig
> > +++ b/drivers/firmware/efi/Kconfig
> > @@ -111,7 +111,7 @@ config EFI_GENERIC_STUB
> >
> > config EFI_ARMSTUB_DTB_LOADER
> > bool "Enable the DTB loader"
> > - depends on EFI_GENERIC_STUB
> > + depends on EFI_GENERIC_STUB && !RISCV
> > default y
> > help
> > Select this config option to add support for the dtb= command
> > diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile
> > index 2b4e09bf987c..7d46b70b51f2 100644
> > --- a/drivers/firmware/efi/libstub/Makefile
> > +++ b/drivers/firmware/efi/libstub/Makefile
> > @@ -22,6 +22,8 @@ cflags-$(CONFIG_ARM64) := $(subst $(CC_FLAGS_FTRACE),,$(KBUILD_CFLAGS)) \
> > cflags-$(CONFIG_ARM) := $(subst $(CC_FLAGS_FTRACE),,$(KBUILD_CFLAGS)) \
> > -fno-builtin -fpic \
> > $(call cc-option,-mno-single-pic-base)
> > +cflags-$(CONFIG_RISCV) := $(subst $(CC_FLAGS_FTRACE),,$(KBUILD_CFLAGS)) \
> > + -fpic
> >
> > cflags-$(CONFIG_EFI_GENERIC_STUB) += -I$(srctree)/scripts/dtc/libfdt
> >
> > @@ -57,6 +59,7 @@ lib-$(CONFIG_EFI_GENERIC_STUB) += efi-stub.o fdt.o string.o \
> > lib-$(CONFIG_ARM) += arm32-stub.o
> > lib-$(CONFIG_ARM64) += arm64-stub.o
> > lib-$(CONFIG_X86) += x86-stub.o
> > +lib-$(CONFIG_RISCV) += riscv-stub.o
> > CFLAGS_arm32-stub.o := -DTEXT_OFFSET=$(TEXT_OFFSET)
> > CFLAGS_arm64-stub.o := -DTEXT_OFFSET=$(TEXT_OFFSET)
> >
> > @@ -81,6 +84,10 @@ STUBCOPY_FLAGS-$(CONFIG_ARM64) += --prefix-alloc-sections=.init \
> > --prefix-symbols=__efistub_
> > STUBCOPY_RELOC-$(CONFIG_ARM64) := R_AARCH64_ABS
> >
> > +STUBCOPY_FLAGS-$(CONFIG_RISCV) += --prefix-alloc-sections=.init \
> > + --prefix-symbols=__efistub_
> > +STUBCOPY_RELOC-$(CONFIG_RISCV) := R_RISCV_HI20
> > +
> > $(obj)/%.stub.o: $(obj)/%.o FORCE
> > $(call if_changed,stubcopy)
> >
> > diff --git a/drivers/firmware/efi/libstub/riscv-stub.c b/drivers/firmware/efi/libstub/riscv-stub.c
> > new file mode 100644
> > index 000000000000..69d13e0ebaea
> > --- /dev/null
> > +++ b/drivers/firmware/efi/libstub/riscv-stub.c
> > @@ -0,0 +1,111 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2013, 2014 Linaro Ltd; <roy.franz@linaro.org>
> > + * Copyright (C) 2020 Western Digital Corporation or its affiliates.
> > + *
> > + * This file implements the EFI boot stub for the RISC-V kernel.
> > + * Adapted from ARM64 version at drivers/firmware/efi/libstub/arm64-stub.c.
> > + */
> > +
> > +#include <linux/efi.h>
> > +#include <linux/libfdt.h>
> > +#include <linux/libfdt_env.h>
> > +#include <asm/efi.h>
> > +#include <asm/sections.h>
> > +
> > +#include "efistub.h"
> > +/*
> > + * RISCV requires the kernel image to placed TEXT_OFFSET bytes beyond a 2 MB
> > + * aligned base for 64 bit and 4MB for 32 bit.
> > + */
Fixed the comment.
> > +#ifdef CONFIG_64BIT
> > +#define MIN_KIMG_ALIGN SZ_2M
> > +#else
> > +#define MIN_KIMG_ALIGN SZ_4M
> > +#endif
> > +
> > +typedef __attribute__((noreturn)) void (*jump_kernel_func)(unsigned int,
> > + unsigned long);
> > +efi_status_t check_platform_features(void)
> > +{
> > + return EFI_SUCCESS;
> > +}
> > +
> > +static u32 get_boot_hartid_from_fdt(unsigned long fdt)
> > +{
> > + int chosen_node, len;
> > + const fdt32_t *prop;
> > +
> > + chosen_node = fdt_path_offset((void *)fdt, "/chosen");
> > + if (chosen_node < 0)
> > + return U32_MAX;
> > + prop = fdt_getprop((void *)fdt, chosen_node, "boot-hartid", &len);
> > + if (!prop || len != sizeof(u32))
> > + return U32_MAX;
> > +
> > + return fdt32_to_cpu(*prop);
> > +}
> > +
> > +/*
> > + * Jump to real kernel here with following constraints.
> > + * 1. MMU should be disabled.
> > + * 2. a0 should contain hartid
> > + * 3. a1 should DT address
> > + */
> > +void __noreturn efi_enter_kernel(unsigned long entrypoint, unsigned long fdt,
> > + unsigned long fdt_size)
> > +{
> > + unsigned long kernel_entry = entrypoint + (unsigned long)stext_offset;
> > + jump_kernel_func jump_kernel = (jump_kernel_func) kernel_entry;
> > + u32 hartid = get_boot_hartid_from_fdt(fdt);
> > +
> > + if (hartid == U32_MAX)
> > + /* We can not use panic or BUG at this point */
> > + __asm__ __volatile__ ("ebreak");
> > + /* Disable MMU */
> > + csr_write(CSR_SATP, 0);
> > + jump_kernel(hartid, fdt);
> > +}
> > +
> > +efi_status_t handle_kernel_image(unsigned long *image_addr,
> > + unsigned long *image_size,
> > + unsigned long *reserve_addr,
> > + unsigned long *reserve_size,
> > + unsigned long dram_base,
> > + efi_loaded_image_t *image)
> > +{
> > + efi_status_t status;
> > + unsigned long kernel_size, kernel_memsize = 0;
> > + unsigned long max_alloc_address;
> > +
> > + if (image->image_base != _start)
> > + pr_efi_err("FIRMWARE BUG: efi_loaded_image_t::image_base has bogus value\n");
> > +
>
> I don't think you need this.
>
Sure. I will remove it. I guess ARM64 code has the error print for
legacy loader code ?
> > + kernel_size = _edata - _start;
> > + kernel_memsize = kernel_size + (_end - _edata);
> > + max_alloc_address = round_up(dram_base, MIN_KIMG_ALIGN) +
> > + kernel_memsize;
> > +
>
> You said the kernel could be anywhere in memory, as long as it is
> aligned correctly, right?
Sorry I was wrong about this. RISC-V kernel maps PAGE_OFFSET virtual
address to the
physical address <xyz> where it is booted. That means memory between
dram start and and <xyz> address
will be unusable.
I also realized that the above computing max_address as above also
won't work for the following reason.
efi_allocate_pages_aligned actually ALIGN_DOWN the max_address. Thus,
efi won't find enough
free memory in this case if the max_address is computed from the dram_base.
Is there an implicit requirement for efi_allocate_pages_aligned or
efi_low_alloc_above should be tried in case of failure?
> In that case, you don't need this, you can simply pass ULONG_MAX as
> the max address.
>
As RISC-V should allocate memory as low as possible to avoid memory
wastage, I think the following should work.
efi_low_alloc_above(*reserve_size, MIN_KIMG_ALIGN, reserve_addr, dram_base);
If this looks okay to you, efi_low_alloc_above should be moved back to
mem.c from relocate.c.
Should I do it in a separate patch or the original patch should be
modified so that efi_low_alloc_above was never moved to relocate.c
> > + if (IS_ALIGNED((u64)_start, MIN_KIMG_ALIGN)) {
> > + /*
> > + * Just execute from wherever we were loaded by the
> > + * UEFI PE/COFF loader if the alignment is suitable.
> > + */
> > + *image_addr = (u64)_start;
>
> Change these casts to (unsigned long), as reported by the robot.
>
Done.
> So you no longer need the placement to be TEXT_OFFSET bytes past a
> MIN_KIMG_ALIGN aligned boundary, right?
>
Nope. EFI memory marked the firmware area as reserved. So EFI memory
allocator will make sure that
it never allocates memory from that region.
> > + *reserve_size = 0;
> > + return EFI_SUCCESS;
> > + }
> > + status = efi_allocate_pages_aligned(*reserve_size, reserve_addr,
> > + max_alloc_address, MIN_KIMG_ALIGN);
> > +
> > + if (status != EFI_SUCCESS) {
> > + pr_efi_err("Failed to relocate kernel\n");
> > + *reserve_size = 0;
> > + return status;
> > + }
> > + *image_addr = *reserve_addr;
> > +
> > + memcpy((void *)*image_addr, _start, kernel_size);
> > +
> > + return EFI_SUCCESS;
> > +}
> > --
> > 2.24.0
> >
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
--
Regards,
Atish
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [v3 PATCH 5/5] RISC-V: Add EFI stub support.
2020-04-18 3:03 ` Atish Patra
@ 2020-04-18 10:51 ` Ard Biesheuvel
2020-04-18 12:39 ` Ard Biesheuvel
0 siblings, 1 reply; 20+ messages in thread
From: Ard Biesheuvel @ 2020-04-18 10:51 UTC (permalink / raw)
To: Atish Patra
Cc: Atish Patra, linux-efi, Arnd Bergmann, Catalin Marinas,
Masahiro Yamada, Linux Kernel Mailing List, Russell King,
Palmer Dabbelt, Greg Kroah-Hartman, linux-riscv, Will Deacon,
Linux ARM
On Sat, 18 Apr 2020 at 05:03, Atish Patra <atishp@atishpatra.org> wrote:
>
> On Thu, Apr 16, 2020 at 12:41 AM Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > On Wed, 15 Apr 2020 at 21:54, Atish Patra <atish.patra@wdc.com> wrote:
> > >
> > > Add a RISC-V architecture specific stub code that actually copies the
> > > actual kernel image to a valid address and jump to it after boot services
> > > are terminated. Enable UEFI related kernel configs as well for RISC-V.
> > >
> > > Signed-off-by: Atish Patra <atish.patra@wdc.com>
> > > ---
> > > arch/riscv/Kconfig | 20 ++++
> > > arch/riscv/Makefile | 1 +
> > > arch/riscv/configs/defconfig | 1 +
> > > arch/riscv/include/asm/efi.h | 44 +++++++++
> > > drivers/firmware/efi/Kconfig | 2 +-
> > > drivers/firmware/efi/libstub/Makefile | 7 ++
> > > drivers/firmware/efi/libstub/riscv-stub.c | 111 ++++++++++++++++++++++
> > > 7 files changed, 185 insertions(+), 1 deletion(-)
> > > create mode 100644 arch/riscv/include/asm/efi.h
> > > create mode 100644 drivers/firmware/efi/libstub/riscv-stub.c
> > >
> > > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> > > index f39e326a7a42..eb4f41c8f3ce 100644
> > > --- a/arch/riscv/Kconfig
> > > +++ b/arch/riscv/Kconfig
> > > @@ -379,10 +379,30 @@ config CMDLINE_FORCE
> > >
> > > endchoice
> > >
> > > +config EFI_STUB
> > > + bool
> > > +
> > > +config EFI
> > > + bool "UEFI runtime support"
> > > + depends on OF
> > > + select LIBFDT
> > > + select UCS2_STRING
> > > + select EFI_PARAMS_FROM_FDT
> > > + select EFI_STUB
> > > + select EFI_GENERIC_STUB
> > > + default y
> > > + help
> > > + This option provides support for runtime services provided
> > > + by UEFI firmware (such as non-volatile variables, realtime
> > > + clock, and platform reset). A UEFI stub is also provided to
> > > + allow the kernel to be booted as an EFI application. This
> > > + is only useful on systems that have UEFI firmware.
> > > +
> > > endmenu
> > >
> > > menu "Power management options"
> > >
> > > source "kernel/power/Kconfig"
> > > +source "drivers/firmware/Kconfig"
> > >
> > > endmenu
> > > diff --git a/arch/riscv/Makefile b/arch/riscv/Makefile
> > > index fb6e37db836d..079435804d6d 100644
> > > --- a/arch/riscv/Makefile
> > > +++ b/arch/riscv/Makefile
> > > @@ -80,6 +80,7 @@ head-y := arch/riscv/kernel/head.o
> > > core-y += arch/riscv/
> > >
> > > libs-y += arch/riscv/lib/
> > > +core-$(CONFIG_EFI_STUB) += $(objtree)/drivers/firmware/efi/libstub/lib.a
> > >
> > > PHONY += vdso_install
> > > vdso_install:
> > > diff --git a/arch/riscv/configs/defconfig b/arch/riscv/configs/defconfig
> > > index 4da4886246a4..ae69e12d306a 100644
> > > --- a/arch/riscv/configs/defconfig
> > > +++ b/arch/riscv/configs/defconfig
> > > @@ -129,3 +129,4 @@ CONFIG_DEBUG_BLOCK_EXT_DEVT=y
> > > # CONFIG_RUNTIME_TESTING_MENU is not set
> > > CONFIG_MEMTEST=y
> > > # CONFIG_SYSFS_SYSCALL is not set
> > > +CONFIG_EFI=y
> > > diff --git a/arch/riscv/include/asm/efi.h b/arch/riscv/include/asm/efi.h
> > > new file mode 100644
> > > index 000000000000..62d7d5eafed8
> > > --- /dev/null
> > > +++ b/arch/riscv/include/asm/efi.h
> > > @@ -0,0 +1,44 @@
> > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > +/*
> > > + * Copyright (C) 2020 Western Digital Corporation or its affiliates.
> > > + * Based on arch/arm64/include/asm/efi.h
> > > + */
> > > +#ifndef _ASM_EFI_H
> > > +#define _ASM_EFI_H
> > > +
> > > +#include <asm/io.h>
> > > +#include <asm/mmu_context.h>
> > > +#include <asm/ptrace.h>
> > > +#include <asm/tlbflush.h>
> > > +
> > > +#define VA_BITS_MIN 39
> > > +
> > > +/* on RISC-V, the FDT may be located anywhere in system RAM */
> > > +static inline unsigned long efi_get_max_fdt_addr(unsigned long dram_base)
> > > +{
> > > + return ULONG_MAX;
> > > +}
> > > +
> > > +/* Load initrd at enough distance from DRAM start */
> > > +static inline unsigned long efi_get_max_initrd_addr(unsigned long dram_base,
> > > + unsigned long image_addr)
> > > +{
> > > + return dram_base + SZ_256M;
> > > +}
> > > +
> > > +#define efi_bs_call(func, ...) efi_system_table()->boottime->func(__VA_ARGS__)
> > > +#define efi_rt_call(func, ...) efi_system_table()->runtime->func(__VA_ARGS__)
> > > +#define efi_is_native() (true)
> > > +
> > > +#define efi_table_attr(inst, attr) (inst->attr)
> > > +
> > > +#define efi_call_proto(inst, func, ...) inst->func(inst, ##__VA_ARGS__)
> > > +
> > > +#define alloc_screen_info(x...) (&screen_info)
> > > +extern char stext_offset[];
> > > +
> > > +static inline void free_screen_info(struct screen_info *si)
> > > +{
> > > +}
> > > +
> > > +#endif /* _ASM_EFI_H */
> > > diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
> > > index 2a2b2b96a1dc..fcdc789d3f87 100644
> > > --- a/drivers/firmware/efi/Kconfig
> > > +++ b/drivers/firmware/efi/Kconfig
> > > @@ -111,7 +111,7 @@ config EFI_GENERIC_STUB
> > >
> > > config EFI_ARMSTUB_DTB_LOADER
> > > bool "Enable the DTB loader"
> > > - depends on EFI_GENERIC_STUB
> > > + depends on EFI_GENERIC_STUB && !RISCV
> > > default y
> > > help
> > > Select this config option to add support for the dtb= command
> > > diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile
> > > index 2b4e09bf987c..7d46b70b51f2 100644
> > > --- a/drivers/firmware/efi/libstub/Makefile
> > > +++ b/drivers/firmware/efi/libstub/Makefile
> > > @@ -22,6 +22,8 @@ cflags-$(CONFIG_ARM64) := $(subst $(CC_FLAGS_FTRACE),,$(KBUILD_CFLAGS)) \
> > > cflags-$(CONFIG_ARM) := $(subst $(CC_FLAGS_FTRACE),,$(KBUILD_CFLAGS)) \
> > > -fno-builtin -fpic \
> > > $(call cc-option,-mno-single-pic-base)
> > > +cflags-$(CONFIG_RISCV) := $(subst $(CC_FLAGS_FTRACE),,$(KBUILD_CFLAGS)) \
> > > + -fpic
> > >
> > > cflags-$(CONFIG_EFI_GENERIC_STUB) += -I$(srctree)/scripts/dtc/libfdt
> > >
> > > @@ -57,6 +59,7 @@ lib-$(CONFIG_EFI_GENERIC_STUB) += efi-stub.o fdt.o string.o \
> > > lib-$(CONFIG_ARM) += arm32-stub.o
> > > lib-$(CONFIG_ARM64) += arm64-stub.o
> > > lib-$(CONFIG_X86) += x86-stub.o
> > > +lib-$(CONFIG_RISCV) += riscv-stub.o
> > > CFLAGS_arm32-stub.o := -DTEXT_OFFSET=$(TEXT_OFFSET)
> > > CFLAGS_arm64-stub.o := -DTEXT_OFFSET=$(TEXT_OFFSET)
> > >
> > > @@ -81,6 +84,10 @@ STUBCOPY_FLAGS-$(CONFIG_ARM64) += --prefix-alloc-sections=.init \
> > > --prefix-symbols=__efistub_
> > > STUBCOPY_RELOC-$(CONFIG_ARM64) := R_AARCH64_ABS
> > >
> > > +STUBCOPY_FLAGS-$(CONFIG_RISCV) += --prefix-alloc-sections=.init \
> > > + --prefix-symbols=__efistub_
> > > +STUBCOPY_RELOC-$(CONFIG_RISCV) := R_RISCV_HI20
> > > +
> > > $(obj)/%.stub.o: $(obj)/%.o FORCE
> > > $(call if_changed,stubcopy)
> > >
> > > diff --git a/drivers/firmware/efi/libstub/riscv-stub.c b/drivers/firmware/efi/libstub/riscv-stub.c
> > > new file mode 100644
> > > index 000000000000..69d13e0ebaea
> > > --- /dev/null
> > > +++ b/drivers/firmware/efi/libstub/riscv-stub.c
> > > @@ -0,0 +1,111 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Copyright (C) 2013, 2014 Linaro Ltd; <roy.franz@linaro.org>
> > > + * Copyright (C) 2020 Western Digital Corporation or its affiliates.
> > > + *
> > > + * This file implements the EFI boot stub for the RISC-V kernel.
> > > + * Adapted from ARM64 version at drivers/firmware/efi/libstub/arm64-stub.c.
> > > + */
> > > +
> > > +#include <linux/efi.h>
> > > +#include <linux/libfdt.h>
> > > +#include <linux/libfdt_env.h>
> > > +#include <asm/efi.h>
> > > +#include <asm/sections.h>
> > > +
> > > +#include "efistub.h"
> > > +/*
> > > + * RISCV requires the kernel image to placed TEXT_OFFSET bytes beyond a 2 MB
> > > + * aligned base for 64 bit and 4MB for 32 bit.
> > > + */
>
> Fixed the comment.
>
> > > +#ifdef CONFIG_64BIT
> > > +#define MIN_KIMG_ALIGN SZ_2M
> > > +#else
> > > +#define MIN_KIMG_ALIGN SZ_4M
> > > +#endif
> > > +
> > > +typedef __attribute__((noreturn)) void (*jump_kernel_func)(unsigned int,
> > > + unsigned long);
> > > +efi_status_t check_platform_features(void)
> > > +{
> > > + return EFI_SUCCESS;
> > > +}
> > > +
> > > +static u32 get_boot_hartid_from_fdt(unsigned long fdt)
> > > +{
> > > + int chosen_node, len;
> > > + const fdt32_t *prop;
> > > +
> > > + chosen_node = fdt_path_offset((void *)fdt, "/chosen");
> > > + if (chosen_node < 0)
> > > + return U32_MAX;
> > > + prop = fdt_getprop((void *)fdt, chosen_node, "boot-hartid", &len);
> > > + if (!prop || len != sizeof(u32))
> > > + return U32_MAX;
> > > +
> > > + return fdt32_to_cpu(*prop);
> > > +}
> > > +
> > > +/*
> > > + * Jump to real kernel here with following constraints.
> > > + * 1. MMU should be disabled.
> > > + * 2. a0 should contain hartid
> > > + * 3. a1 should DT address
> > > + */
> > > +void __noreturn efi_enter_kernel(unsigned long entrypoint, unsigned long fdt,
> > > + unsigned long fdt_size)
> > > +{
> > > + unsigned long kernel_entry = entrypoint + (unsigned long)stext_offset;
> > > + jump_kernel_func jump_kernel = (jump_kernel_func) kernel_entry;
> > > + u32 hartid = get_boot_hartid_from_fdt(fdt);
> > > +
> > > + if (hartid == U32_MAX)
> > > + /* We can not use panic or BUG at this point */
> > > + __asm__ __volatile__ ("ebreak");
> > > + /* Disable MMU */
> > > + csr_write(CSR_SATP, 0);
> > > + jump_kernel(hartid, fdt);
> > > +}
> > > +
> > > +efi_status_t handle_kernel_image(unsigned long *image_addr,
> > > + unsigned long *image_size,
> > > + unsigned long *reserve_addr,
> > > + unsigned long *reserve_size,
> > > + unsigned long dram_base,
> > > + efi_loaded_image_t *image)
> > > +{
> > > + efi_status_t status;
> > > + unsigned long kernel_size, kernel_memsize = 0;
> > > + unsigned long max_alloc_address;
> > > +
> > > + if (image->image_base != _start)
> > > + pr_efi_err("FIRMWARE BUG: efi_loaded_image_t::image_base has bogus value\n");
> > > +
> >
> > I don't think you need this.
> >
>
> Sure. I will remove it. I guess ARM64 code has the error print for
> legacy loader code ?
>
No, for broken distro versions of GRUB
> > > + kernel_size = _edata - _start;
> > > + kernel_memsize = kernel_size + (_end - _edata);
> > > + max_alloc_address = round_up(dram_base, MIN_KIMG_ALIGN) +
> > > + kernel_memsize;
> > > +
> >
> > You said the kernel could be anywhere in memory, as long as it is
> > aligned correctly, right?
>
> Sorry I was wrong about this. RISC-V kernel maps PAGE_OFFSET virtual
> address to the
> physical address <xyz> where it is booted. That means memory between
> dram start and and <xyz> address
> will be unusable.
>
OK
> I also realized that the above computing max_address as above also
> won't work for the following reason.
> efi_allocate_pages_aligned actually ALIGN_DOWN the max_address. Thus,
> efi won't find enough
> free memory in this case if the max_address is computed from the dram_base.
>
> Is there an implicit requirement for efi_allocate_pages_aligned or
> efi_low_alloc_above should be tried in case of failure?
>
No not really. What ever works for your particular use case is acceptable to me.
> > In that case, you don't need this, you can simply pass ULONG_MAX as
> > the max address.
> >
> As RISC-V should allocate memory as low as possible to avoid memory
> wastage, I think the following should work.
>
> efi_low_alloc_above(*reserve_size, MIN_KIMG_ALIGN, reserve_addr, dram_base);
>
> If this looks okay to you, efi_low_alloc_above should be moved back to
> mem.c from relocate.c.
> Should I do it in a separate patch or the original patch should be
> modified so that efi_low_alloc_above was never moved to relocate.c
>
No, please keep efi_low_alloc_above() where it is, but drop the
static, and put back the declaration in efistub.h
> > > + if (IS_ALIGNED((u64)_start, MIN_KIMG_ALIGN)) {
> > > + /*
> > > + * Just execute from wherever we were loaded by the
> > > + * UEFI PE/COFF loader if the alignment is suitable.
> > > + */
> > > + *image_addr = (u64)_start;
> >
> > Change these casts to (unsigned long), as reported by the robot.
> >
> Done.
>
> > So you no longer need the placement to be TEXT_OFFSET bytes past a
> > MIN_KIMG_ALIGN aligned boundary, right?
> >
>
> Nope. EFI memory marked the firmware area as reserved. So EFI memory
> allocator will make sure that
> it never allocates memory from that region.
>
OK, good.
> > > + *reserve_size = 0;
> > > + return EFI_SUCCESS;
> > > + }
> > > + status = efi_allocate_pages_aligned(*reserve_size, reserve_addr,
> > > + max_alloc_address, MIN_KIMG_ALIGN);
> > > +
> > > + if (status != EFI_SUCCESS) {
> > > + pr_efi_err("Failed to relocate kernel\n");
> > > + *reserve_size = 0;
> > > + return status;
> > > + }
> > > + *image_addr = *reserve_addr;
> > > +
> > > + memcpy((void *)*image_addr, _start, kernel_size);
> > > +
> > > + return EFI_SUCCESS;
> > > +}
> > > --
> > > 2.24.0
> > >
> >
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
>
>
> --
> Regards,
> Atish
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [v3 PATCH 5/5] RISC-V: Add EFI stub support.
2020-04-18 10:51 ` Ard Biesheuvel
@ 2020-04-18 12:39 ` Ard Biesheuvel
2020-04-18 19:19 ` Atish Patra
0 siblings, 1 reply; 20+ messages in thread
From: Ard Biesheuvel @ 2020-04-18 12:39 UTC (permalink / raw)
To: Atish Patra
Cc: Atish Patra, linux-efi, Arnd Bergmann, Catalin Marinas,
Masahiro Yamada, Linux Kernel Mailing List, Russell King,
Palmer Dabbelt, Greg Kroah-Hartman, linux-riscv, Will Deacon,
Linux ARM
On Sat, 18 Apr 2020 at 12:51, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Sat, 18 Apr 2020 at 05:03, Atish Patra <atishp@atishpatra.org> wrote:
> >
> > On Thu, Apr 16, 2020 at 12:41 AM Ard Biesheuvel <ardb@kernel.org> wrote:
> > >
> > > On Wed, 15 Apr 2020 at 21:54, Atish Patra <atish.patra@wdc.com> wrote:
> > > >
> > > > Add a RISC-V architecture specific stub code that actually copies the
> > > > actual kernel image to a valid address and jump to it after boot services
> > > > are terminated. Enable UEFI related kernel configs as well for RISC-V.
> > > >
> > > > Signed-off-by: Atish Patra <atish.patra@wdc.com>
> > > > ---
> > > > arch/riscv/Kconfig | 20 ++++
> > > > arch/riscv/Makefile | 1 +
> > > > arch/riscv/configs/defconfig | 1 +
> > > > arch/riscv/include/asm/efi.h | 44 +++++++++
> > > > drivers/firmware/efi/Kconfig | 2 +-
> > > > drivers/firmware/efi/libstub/Makefile | 7 ++
> > > > drivers/firmware/efi/libstub/riscv-stub.c | 111 ++++++++++++++++++++++
> > > > 7 files changed, 185 insertions(+), 1 deletion(-)
> > > > create mode 100644 arch/riscv/include/asm/efi.h
> > > > create mode 100644 drivers/firmware/efi/libstub/riscv-stub.c
> > > >
> > > > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> > > > index f39e326a7a42..eb4f41c8f3ce 100644
> > > > --- a/arch/riscv/Kconfig
> > > > +++ b/arch/riscv/Kconfig
> > > > @@ -379,10 +379,30 @@ config CMDLINE_FORCE
> > > >
> > > > endchoice
> > > >
> > > > +config EFI_STUB
> > > > + bool
> > > > +
> > > > +config EFI
> > > > + bool "UEFI runtime support"
> > > > + depends on OF
> > > > + select LIBFDT
> > > > + select UCS2_STRING
> > > > + select EFI_PARAMS_FROM_FDT
> > > > + select EFI_STUB
> > > > + select EFI_GENERIC_STUB
> > > > + default y
> > > > + help
> > > > + This option provides support for runtime services provided
> > > > + by UEFI firmware (such as non-volatile variables, realtime
> > > > + clock, and platform reset). A UEFI stub is also provided to
> > > > + allow the kernel to be booted as an EFI application. This
> > > > + is only useful on systems that have UEFI firmware.
> > > > +
> > > > endmenu
> > > >
> > > > menu "Power management options"
> > > >
> > > > source "kernel/power/Kconfig"
> > > > +source "drivers/firmware/Kconfig"
> > > >
> > > > endmenu
> > > > diff --git a/arch/riscv/Makefile b/arch/riscv/Makefile
> > > > index fb6e37db836d..079435804d6d 100644
> > > > --- a/arch/riscv/Makefile
> > > > +++ b/arch/riscv/Makefile
> > > > @@ -80,6 +80,7 @@ head-y := arch/riscv/kernel/head.o
> > > > core-y += arch/riscv/
> > > >
> > > > libs-y += arch/riscv/lib/
> > > > +core-$(CONFIG_EFI_STUB) += $(objtree)/drivers/firmware/efi/libstub/lib.a
> > > >
> > > > PHONY += vdso_install
> > > > vdso_install:
> > > > diff --git a/arch/riscv/configs/defconfig b/arch/riscv/configs/defconfig
> > > > index 4da4886246a4..ae69e12d306a 100644
> > > > --- a/arch/riscv/configs/defconfig
> > > > +++ b/arch/riscv/configs/defconfig
> > > > @@ -129,3 +129,4 @@ CONFIG_DEBUG_BLOCK_EXT_DEVT=y
> > > > # CONFIG_RUNTIME_TESTING_MENU is not set
> > > > CONFIG_MEMTEST=y
> > > > # CONFIG_SYSFS_SYSCALL is not set
> > > > +CONFIG_EFI=y
> > > > diff --git a/arch/riscv/include/asm/efi.h b/arch/riscv/include/asm/efi.h
> > > > new file mode 100644
> > > > index 000000000000..62d7d5eafed8
> > > > --- /dev/null
> > > > +++ b/arch/riscv/include/asm/efi.h
> > > > @@ -0,0 +1,44 @@
> > > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > > +/*
> > > > + * Copyright (C) 2020 Western Digital Corporation or its affiliates.
> > > > + * Based on arch/arm64/include/asm/efi.h
> > > > + */
> > > > +#ifndef _ASM_EFI_H
> > > > +#define _ASM_EFI_H
> > > > +
> > > > +#include <asm/io.h>
> > > > +#include <asm/mmu_context.h>
> > > > +#include <asm/ptrace.h>
> > > > +#include <asm/tlbflush.h>
> > > > +
> > > > +#define VA_BITS_MIN 39
> > > > +
> > > > +/* on RISC-V, the FDT may be located anywhere in system RAM */
> > > > +static inline unsigned long efi_get_max_fdt_addr(unsigned long dram_base)
> > > > +{
> > > > + return ULONG_MAX;
> > > > +}
> > > > +
> > > > +/* Load initrd at enough distance from DRAM start */
> > > > +static inline unsigned long efi_get_max_initrd_addr(unsigned long dram_base,
> > > > + unsigned long image_addr)
> > > > +{
> > > > + return dram_base + SZ_256M;
> > > > +}
> > > > +
> > > > +#define efi_bs_call(func, ...) efi_system_table()->boottime->func(__VA_ARGS__)
> > > > +#define efi_rt_call(func, ...) efi_system_table()->runtime->func(__VA_ARGS__)
> > > > +#define efi_is_native() (true)
> > > > +
> > > > +#define efi_table_attr(inst, attr) (inst->attr)
> > > > +
> > > > +#define efi_call_proto(inst, func, ...) inst->func(inst, ##__VA_ARGS__)
> > > > +
> > > > +#define alloc_screen_info(x...) (&screen_info)
> > > > +extern char stext_offset[];
> > > > +
> > > > +static inline void free_screen_info(struct screen_info *si)
> > > > +{
> > > > +}
> > > > +
> > > > +#endif /* _ASM_EFI_H */
> > > > diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
> > > > index 2a2b2b96a1dc..fcdc789d3f87 100644
> > > > --- a/drivers/firmware/efi/Kconfig
> > > > +++ b/drivers/firmware/efi/Kconfig
> > > > @@ -111,7 +111,7 @@ config EFI_GENERIC_STUB
> > > >
> > > > config EFI_ARMSTUB_DTB_LOADER
> > > > bool "Enable the DTB loader"
> > > > - depends on EFI_GENERIC_STUB
> > > > + depends on EFI_GENERIC_STUB && !RISCV
> > > > default y
> > > > help
> > > > Select this config option to add support for the dtb= command
> > > > diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile
> > > > index 2b4e09bf987c..7d46b70b51f2 100644
> > > > --- a/drivers/firmware/efi/libstub/Makefile
> > > > +++ b/drivers/firmware/efi/libstub/Makefile
> > > > @@ -22,6 +22,8 @@ cflags-$(CONFIG_ARM64) := $(subst $(CC_FLAGS_FTRACE),,$(KBUILD_CFLAGS)) \
> > > > cflags-$(CONFIG_ARM) := $(subst $(CC_FLAGS_FTRACE),,$(KBUILD_CFLAGS)) \
> > > > -fno-builtin -fpic \
> > > > $(call cc-option,-mno-single-pic-base)
> > > > +cflags-$(CONFIG_RISCV) := $(subst $(CC_FLAGS_FTRACE),,$(KBUILD_CFLAGS)) \
> > > > + -fpic
> > > >
> > > > cflags-$(CONFIG_EFI_GENERIC_STUB) += -I$(srctree)/scripts/dtc/libfdt
> > > >
> > > > @@ -57,6 +59,7 @@ lib-$(CONFIG_EFI_GENERIC_STUB) += efi-stub.o fdt.o string.o \
> > > > lib-$(CONFIG_ARM) += arm32-stub.o
> > > > lib-$(CONFIG_ARM64) += arm64-stub.o
> > > > lib-$(CONFIG_X86) += x86-stub.o
> > > > +lib-$(CONFIG_RISCV) += riscv-stub.o
> > > > CFLAGS_arm32-stub.o := -DTEXT_OFFSET=$(TEXT_OFFSET)
> > > > CFLAGS_arm64-stub.o := -DTEXT_OFFSET=$(TEXT_OFFSET)
> > > >
> > > > @@ -81,6 +84,10 @@ STUBCOPY_FLAGS-$(CONFIG_ARM64) += --prefix-alloc-sections=.init \
> > > > --prefix-symbols=__efistub_
> > > > STUBCOPY_RELOC-$(CONFIG_ARM64) := R_AARCH64_ABS
> > > >
> > > > +STUBCOPY_FLAGS-$(CONFIG_RISCV) += --prefix-alloc-sections=.init \
> > > > + --prefix-symbols=__efistub_
> > > > +STUBCOPY_RELOC-$(CONFIG_RISCV) := R_RISCV_HI20
> > > > +
> > > > $(obj)/%.stub.o: $(obj)/%.o FORCE
> > > > $(call if_changed,stubcopy)
> > > >
> > > > diff --git a/drivers/firmware/efi/libstub/riscv-stub.c b/drivers/firmware/efi/libstub/riscv-stub.c
> > > > new file mode 100644
> > > > index 000000000000..69d13e0ebaea
> > > > --- /dev/null
> > > > +++ b/drivers/firmware/efi/libstub/riscv-stub.c
> > > > @@ -0,0 +1,111 @@
> > > > +// SPDX-License-Identifier: GPL-2.0
> > > > +/*
> > > > + * Copyright (C) 2013, 2014 Linaro Ltd; <roy.franz@linaro.org>
> > > > + * Copyright (C) 2020 Western Digital Corporation or its affiliates.
> > > > + *
> > > > + * This file implements the EFI boot stub for the RISC-V kernel.
> > > > + * Adapted from ARM64 version at drivers/firmware/efi/libstub/arm64-stub.c.
> > > > + */
> > > > +
> > > > +#include <linux/efi.h>
> > > > +#include <linux/libfdt.h>
> > > > +#include <linux/libfdt_env.h>
> > > > +#include <asm/efi.h>
> > > > +#include <asm/sections.h>
> > > > +
> > > > +#include "efistub.h"
> > > > +/*
> > > > + * RISCV requires the kernel image to placed TEXT_OFFSET bytes beyond a 2 MB
> > > > + * aligned base for 64 bit and 4MB for 32 bit.
> > > > + */
> >
> > Fixed the comment.
> >
> > > > +#ifdef CONFIG_64BIT
> > > > +#define MIN_KIMG_ALIGN SZ_2M
> > > > +#else
> > > > +#define MIN_KIMG_ALIGN SZ_4M
> > > > +#endif
> > > > +
> > > > +typedef __attribute__((noreturn)) void (*jump_kernel_func)(unsigned int,
> > > > + unsigned long);
> > > > +efi_status_t check_platform_features(void)
> > > > +{
> > > > + return EFI_SUCCESS;
> > > > +}
> > > > +
> > > > +static u32 get_boot_hartid_from_fdt(unsigned long fdt)
> > > > +{
> > > > + int chosen_node, len;
> > > > + const fdt32_t *prop;
> > > > +
> > > > + chosen_node = fdt_path_offset((void *)fdt, "/chosen");
> > > > + if (chosen_node < 0)
> > > > + return U32_MAX;
> > > > + prop = fdt_getprop((void *)fdt, chosen_node, "boot-hartid", &len);
> > > > + if (!prop || len != sizeof(u32))
> > > > + return U32_MAX;
> > > > +
> > > > + return fdt32_to_cpu(*prop);
> > > > +}
> > > > +
> > > > +/*
> > > > + * Jump to real kernel here with following constraints.
> > > > + * 1. MMU should be disabled.
> > > > + * 2. a0 should contain hartid
> > > > + * 3. a1 should DT address
> > > > + */
> > > > +void __noreturn efi_enter_kernel(unsigned long entrypoint, unsigned long fdt,
> > > > + unsigned long fdt_size)
> > > > +{
> > > > + unsigned long kernel_entry = entrypoint + (unsigned long)stext_offset;
> > > > + jump_kernel_func jump_kernel = (jump_kernel_func) kernel_entry;
> > > > + u32 hartid = get_boot_hartid_from_fdt(fdt);
> > > > +
> > > > + if (hartid == U32_MAX)
> > > > + /* We can not use panic or BUG at this point */
> > > > + __asm__ __volatile__ ("ebreak");
> > > > + /* Disable MMU */
> > > > + csr_write(CSR_SATP, 0);
> > > > + jump_kernel(hartid, fdt);
> > > > +}
> > > > +
> > > > +efi_status_t handle_kernel_image(unsigned long *image_addr,
> > > > + unsigned long *image_size,
> > > > + unsigned long *reserve_addr,
> > > > + unsigned long *reserve_size,
> > > > + unsigned long dram_base,
> > > > + efi_loaded_image_t *image)
> > > > +{
> > > > + efi_status_t status;
> > > > + unsigned long kernel_size, kernel_memsize = 0;
> > > > + unsigned long max_alloc_address;
> > > > +
> > > > + if (image->image_base != _start)
> > > > + pr_efi_err("FIRMWARE BUG: efi_loaded_image_t::image_base has bogus value\n");
> > > > +
> > >
> > > I don't think you need this.
> > >
> >
> > Sure. I will remove it. I guess ARM64 code has the error print for
> > legacy loader code ?
> >
>
> No, for broken distro versions of GRUB
>
> > > > + kernel_size = _edata - _start;
> > > > + kernel_memsize = kernel_size + (_end - _edata);
> > > > + max_alloc_address = round_up(dram_base, MIN_KIMG_ALIGN) +
> > > > + kernel_memsize;
> > > > +
> > >
> > > You said the kernel could be anywhere in memory, as long as it is
> > > aligned correctly, right?
> >
> > Sorry I was wrong about this. RISC-V kernel maps PAGE_OFFSET virtual
> > address to the
> > physical address <xyz> where it is booted. That means memory between
> > dram start and and <xyz> address
> > will be unusable.
> >
>
> OK
>
> > I also realized that the above computing max_address as above also
> > won't work for the following reason.
> > efi_allocate_pages_aligned actually ALIGN_DOWN the max_address. Thus,
> > efi won't find enough
> > free memory in this case if the max_address is computed from the dram_base.
> >
> > Is there an implicit requirement for efi_allocate_pages_aligned or
> > efi_low_alloc_above should be tried in case of failure?
> >
>
> No not really. What ever works for your particular use case is acceptable to me.
>
> > > In that case, you don't need this, you can simply pass ULONG_MAX as
> > > the max address.
> > >
> > As RISC-V should allocate memory as low as possible to avoid memory
> > wastage, I think the following should work.
> >
> > efi_low_alloc_above(*reserve_size, MIN_KIMG_ALIGN, reserve_addr, dram_base);
> >
> > If this looks okay to you, efi_low_alloc_above should be moved back to
> > mem.c from relocate.c.
> > Should I do it in a separate patch or the original patch should be
> > modified so that efi_low_alloc_above was never moved to relocate.c
> >
>
> No, please keep efi_low_alloc_above() where it is, but drop the
> static, and put back the declaration in efistub.h
>
Alternatively, can you check whether efi_relocate_kernel() already
does what you need?
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [v3 PATCH 5/5] RISC-V: Add EFI stub support.
2020-04-18 12:39 ` Ard Biesheuvel
@ 2020-04-18 19:19 ` Atish Patra
2020-04-18 19:24 ` Ard Biesheuvel
0 siblings, 1 reply; 20+ messages in thread
From: Atish Patra @ 2020-04-18 19:19 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Atish Patra, linux-efi, Arnd Bergmann, Catalin Marinas,
Masahiro Yamada, Linux Kernel Mailing List, Russell King,
Palmer Dabbelt, Greg Kroah-Hartman, linux-riscv, Will Deacon,
Linux ARM
On Sat, Apr 18, 2020 at 5:39 AM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Sat, 18 Apr 2020 at 12:51, Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > On Sat, 18 Apr 2020 at 05:03, Atish Patra <atishp@atishpatra.org> wrote:
> > >
> > > On Thu, Apr 16, 2020 at 12:41 AM Ard Biesheuvel <ardb@kernel.org> wrote:
> > > >
> > > > On Wed, 15 Apr 2020 at 21:54, Atish Patra <atish.patra@wdc.com> wrote:
> > > > >
> > > > > Add a RISC-V architecture specific stub code that actually copies the
> > > > > actual kernel image to a valid address and jump to it after boot services
> > > > > are terminated. Enable UEFI related kernel configs as well for RISC-V.
> > > > >
> > > > > Signed-off-by: Atish Patra <atish.patra@wdc.com>
> > > > > ---
> > > > > arch/riscv/Kconfig | 20 ++++
> > > > > arch/riscv/Makefile | 1 +
> > > > > arch/riscv/configs/defconfig | 1 +
> > > > > arch/riscv/include/asm/efi.h | 44 +++++++++
> > > > > drivers/firmware/efi/Kconfig | 2 +-
> > > > > drivers/firmware/efi/libstub/Makefile | 7 ++
> > > > > drivers/firmware/efi/libstub/riscv-stub.c | 111 ++++++++++++++++++++++
> > > > > 7 files changed, 185 insertions(+), 1 deletion(-)
> > > > > create mode 100644 arch/riscv/include/asm/efi.h
> > > > > create mode 100644 drivers/firmware/efi/libstub/riscv-stub.c
> > > > >
> > > > > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> > > > > index f39e326a7a42..eb4f41c8f3ce 100644
> > > > > --- a/arch/riscv/Kconfig
> > > > > +++ b/arch/riscv/Kconfig
> > > > > @@ -379,10 +379,30 @@ config CMDLINE_FORCE
> > > > >
> > > > > endchoice
> > > > >
> > > > > +config EFI_STUB
> > > > > + bool
> > > > > +
> > > > > +config EFI
> > > > > + bool "UEFI runtime support"
> > > > > + depends on OF
> > > > > + select LIBFDT
> > > > > + select UCS2_STRING
> > > > > + select EFI_PARAMS_FROM_FDT
> > > > > + select EFI_STUB
> > > > > + select EFI_GENERIC_STUB
> > > > > + default y
> > > > > + help
> > > > > + This option provides support for runtime services provided
> > > > > + by UEFI firmware (such as non-volatile variables, realtime
> > > > > + clock, and platform reset). A UEFI stub is also provided to
> > > > > + allow the kernel to be booted as an EFI application. This
> > > > > + is only useful on systems that have UEFI firmware.
> > > > > +
> > > > > endmenu
> > > > >
> > > > > menu "Power management options"
> > > > >
> > > > > source "kernel/power/Kconfig"
> > > > > +source "drivers/firmware/Kconfig"
> > > > >
> > > > > endmenu
> > > > > diff --git a/arch/riscv/Makefile b/arch/riscv/Makefile
> > > > > index fb6e37db836d..079435804d6d 100644
> > > > > --- a/arch/riscv/Makefile
> > > > > +++ b/arch/riscv/Makefile
> > > > > @@ -80,6 +80,7 @@ head-y := arch/riscv/kernel/head.o
> > > > > core-y += arch/riscv/
> > > > >
> > > > > libs-y += arch/riscv/lib/
> > > > > +core-$(CONFIG_EFI_STUB) += $(objtree)/drivers/firmware/efi/libstub/lib.a
> > > > >
> > > > > PHONY += vdso_install
> > > > > vdso_install:
> > > > > diff --git a/arch/riscv/configs/defconfig b/arch/riscv/configs/defconfig
> > > > > index 4da4886246a4..ae69e12d306a 100644
> > > > > --- a/arch/riscv/configs/defconfig
> > > > > +++ b/arch/riscv/configs/defconfig
> > > > > @@ -129,3 +129,4 @@ CONFIG_DEBUG_BLOCK_EXT_DEVT=y
> > > > > # CONFIG_RUNTIME_TESTING_MENU is not set
> > > > > CONFIG_MEMTEST=y
> > > > > # CONFIG_SYSFS_SYSCALL is not set
> > > > > +CONFIG_EFI=y
> > > > > diff --git a/arch/riscv/include/asm/efi.h b/arch/riscv/include/asm/efi.h
> > > > > new file mode 100644
> > > > > index 000000000000..62d7d5eafed8
> > > > > --- /dev/null
> > > > > +++ b/arch/riscv/include/asm/efi.h
> > > > > @@ -0,0 +1,44 @@
> > > > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > > > +/*
> > > > > + * Copyright (C) 2020 Western Digital Corporation or its affiliates.
> > > > > + * Based on arch/arm64/include/asm/efi.h
> > > > > + */
> > > > > +#ifndef _ASM_EFI_H
> > > > > +#define _ASM_EFI_H
> > > > > +
> > > > > +#include <asm/io.h>
> > > > > +#include <asm/mmu_context.h>
> > > > > +#include <asm/ptrace.h>
> > > > > +#include <asm/tlbflush.h>
> > > > > +
> > > > > +#define VA_BITS_MIN 39
> > > > > +
> > > > > +/* on RISC-V, the FDT may be located anywhere in system RAM */
> > > > > +static inline unsigned long efi_get_max_fdt_addr(unsigned long dram_base)
> > > > > +{
> > > > > + return ULONG_MAX;
> > > > > +}
> > > > > +
> > > > > +/* Load initrd at enough distance from DRAM start */
> > > > > +static inline unsigned long efi_get_max_initrd_addr(unsigned long dram_base,
> > > > > + unsigned long image_addr)
> > > > > +{
> > > > > + return dram_base + SZ_256M;
> > > > > +}
> > > > > +
> > > > > +#define efi_bs_call(func, ...) efi_system_table()->boottime->func(__VA_ARGS__)
> > > > > +#define efi_rt_call(func, ...) efi_system_table()->runtime->func(__VA_ARGS__)
> > > > > +#define efi_is_native() (true)
> > > > > +
> > > > > +#define efi_table_attr(inst, attr) (inst->attr)
> > > > > +
> > > > > +#define efi_call_proto(inst, func, ...) inst->func(inst, ##__VA_ARGS__)
> > > > > +
> > > > > +#define alloc_screen_info(x...) (&screen_info)
> > > > > +extern char stext_offset[];
> > > > > +
> > > > > +static inline void free_screen_info(struct screen_info *si)
> > > > > +{
> > > > > +}
> > > > > +
> > > > > +#endif /* _ASM_EFI_H */
> > > > > diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
> > > > > index 2a2b2b96a1dc..fcdc789d3f87 100644
> > > > > --- a/drivers/firmware/efi/Kconfig
> > > > > +++ b/drivers/firmware/efi/Kconfig
> > > > > @@ -111,7 +111,7 @@ config EFI_GENERIC_STUB
> > > > >
> > > > > config EFI_ARMSTUB_DTB_LOADER
> > > > > bool "Enable the DTB loader"
> > > > > - depends on EFI_GENERIC_STUB
> > > > > + depends on EFI_GENERIC_STUB && !RISCV
> > > > > default y
> > > > > help
> > > > > Select this config option to add support for the dtb= command
> > > > > diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile
> > > > > index 2b4e09bf987c..7d46b70b51f2 100644
> > > > > --- a/drivers/firmware/efi/libstub/Makefile
> > > > > +++ b/drivers/firmware/efi/libstub/Makefile
> > > > > @@ -22,6 +22,8 @@ cflags-$(CONFIG_ARM64) := $(subst $(CC_FLAGS_FTRACE),,$(KBUILD_CFLAGS)) \
> > > > > cflags-$(CONFIG_ARM) := $(subst $(CC_FLAGS_FTRACE),,$(KBUILD_CFLAGS)) \
> > > > > -fno-builtin -fpic \
> > > > > $(call cc-option,-mno-single-pic-base)
> > > > > +cflags-$(CONFIG_RISCV) := $(subst $(CC_FLAGS_FTRACE),,$(KBUILD_CFLAGS)) \
> > > > > + -fpic
> > > > >
> > > > > cflags-$(CONFIG_EFI_GENERIC_STUB) += -I$(srctree)/scripts/dtc/libfdt
> > > > >
> > > > > @@ -57,6 +59,7 @@ lib-$(CONFIG_EFI_GENERIC_STUB) += efi-stub.o fdt.o string.o \
> > > > > lib-$(CONFIG_ARM) += arm32-stub.o
> > > > > lib-$(CONFIG_ARM64) += arm64-stub.o
> > > > > lib-$(CONFIG_X86) += x86-stub.o
> > > > > +lib-$(CONFIG_RISCV) += riscv-stub.o
> > > > > CFLAGS_arm32-stub.o := -DTEXT_OFFSET=$(TEXT_OFFSET)
> > > > > CFLAGS_arm64-stub.o := -DTEXT_OFFSET=$(TEXT_OFFSET)
> > > > >
> > > > > @@ -81,6 +84,10 @@ STUBCOPY_FLAGS-$(CONFIG_ARM64) += --prefix-alloc-sections=.init \
> > > > > --prefix-symbols=__efistub_
> > > > > STUBCOPY_RELOC-$(CONFIG_ARM64) := R_AARCH64_ABS
> > > > >
> > > > > +STUBCOPY_FLAGS-$(CONFIG_RISCV) += --prefix-alloc-sections=.init \
> > > > > + --prefix-symbols=__efistub_
> > > > > +STUBCOPY_RELOC-$(CONFIG_RISCV) := R_RISCV_HI20
> > > > > +
> > > > > $(obj)/%.stub.o: $(obj)/%.o FORCE
> > > > > $(call if_changed,stubcopy)
> > > > >
> > > > > diff --git a/drivers/firmware/efi/libstub/riscv-stub.c b/drivers/firmware/efi/libstub/riscv-stub.c
> > > > > new file mode 100644
> > > > > index 000000000000..69d13e0ebaea
> > > > > --- /dev/null
> > > > > +++ b/drivers/firmware/efi/libstub/riscv-stub.c
> > > > > @@ -0,0 +1,111 @@
> > > > > +// SPDX-License-Identifier: GPL-2.0
> > > > > +/*
> > > > > + * Copyright (C) 2013, 2014 Linaro Ltd; <roy.franz@linaro.org>
> > > > > + * Copyright (C) 2020 Western Digital Corporation or its affiliates.
> > > > > + *
> > > > > + * This file implements the EFI boot stub for the RISC-V kernel.
> > > > > + * Adapted from ARM64 version at drivers/firmware/efi/libstub/arm64-stub.c.
> > > > > + */
> > > > > +
> > > > > +#include <linux/efi.h>
> > > > > +#include <linux/libfdt.h>
> > > > > +#include <linux/libfdt_env.h>
> > > > > +#include <asm/efi.h>
> > > > > +#include <asm/sections.h>
> > > > > +
> > > > > +#include "efistub.h"
> > > > > +/*
> > > > > + * RISCV requires the kernel image to placed TEXT_OFFSET bytes beyond a 2 MB
> > > > > + * aligned base for 64 bit and 4MB for 32 bit.
> > > > > + */
> > >
> > > Fixed the comment.
> > >
> > > > > +#ifdef CONFIG_64BIT
> > > > > +#define MIN_KIMG_ALIGN SZ_2M
> > > > > +#else
> > > > > +#define MIN_KIMG_ALIGN SZ_4M
> > > > > +#endif
> > > > > +
> > > > > +typedef __attribute__((noreturn)) void (*jump_kernel_func)(unsigned int,
> > > > > + unsigned long);
> > > > > +efi_status_t check_platform_features(void)
> > > > > +{
> > > > > + return EFI_SUCCESS;
> > > > > +}
> > > > > +
> > > > > +static u32 get_boot_hartid_from_fdt(unsigned long fdt)
> > > > > +{
> > > > > + int chosen_node, len;
> > > > > + const fdt32_t *prop;
> > > > > +
> > > > > + chosen_node = fdt_path_offset((void *)fdt, "/chosen");
> > > > > + if (chosen_node < 0)
> > > > > + return U32_MAX;
> > > > > + prop = fdt_getprop((void *)fdt, chosen_node, "boot-hartid", &len);
> > > > > + if (!prop || len != sizeof(u32))
> > > > > + return U32_MAX;
> > > > > +
> > > > > + return fdt32_to_cpu(*prop);
> > > > > +}
> > > > > +
> > > > > +/*
> > > > > + * Jump to real kernel here with following constraints.
> > > > > + * 1. MMU should be disabled.
> > > > > + * 2. a0 should contain hartid
> > > > > + * 3. a1 should DT address
> > > > > + */
> > > > > +void __noreturn efi_enter_kernel(unsigned long entrypoint, unsigned long fdt,
> > > > > + unsigned long fdt_size)
> > > > > +{
> > > > > + unsigned long kernel_entry = entrypoint + (unsigned long)stext_offset;
> > > > > + jump_kernel_func jump_kernel = (jump_kernel_func) kernel_entry;
> > > > > + u32 hartid = get_boot_hartid_from_fdt(fdt);
> > > > > +
> > > > > + if (hartid == U32_MAX)
> > > > > + /* We can not use panic or BUG at this point */
> > > > > + __asm__ __volatile__ ("ebreak");
> > > > > + /* Disable MMU */
> > > > > + csr_write(CSR_SATP, 0);
> > > > > + jump_kernel(hartid, fdt);
> > > > > +}
> > > > > +
> > > > > +efi_status_t handle_kernel_image(unsigned long *image_addr,
> > > > > + unsigned long *image_size,
> > > > > + unsigned long *reserve_addr,
> > > > > + unsigned long *reserve_size,
> > > > > + unsigned long dram_base,
> > > > > + efi_loaded_image_t *image)
> > > > > +{
> > > > > + efi_status_t status;
> > > > > + unsigned long kernel_size, kernel_memsize = 0;
> > > > > + unsigned long max_alloc_address;
> > > > > +
> > > > > + if (image->image_base != _start)
> > > > > + pr_efi_err("FIRMWARE BUG: efi_loaded_image_t::image_base has bogus value\n");
> > > > > +
> > > >
> > > > I don't think you need this.
> > > >
> > >
> > > Sure. I will remove it. I guess ARM64 code has the error print for
> > > legacy loader code ?
> > >
> >
> > No, for broken distro versions of GRUB
> >
> > > > > + kernel_size = _edata - _start;
> > > > > + kernel_memsize = kernel_size + (_end - _edata);
> > > > > + max_alloc_address = round_up(dram_base, MIN_KIMG_ALIGN) +
> > > > > + kernel_memsize;
> > > > > +
> > > >
> > > > You said the kernel could be anywhere in memory, as long as it is
> > > > aligned correctly, right?
> > >
> > > Sorry I was wrong about this. RISC-V kernel maps PAGE_OFFSET virtual
> > > address to the
> > > physical address <xyz> where it is booted. That means memory between
> > > dram start and and <xyz> address
> > > will be unusable.
> > >
> >
> > OK
> >
> > > I also realized that the above computing max_address as above also
> > > won't work for the following reason.
> > > efi_allocate_pages_aligned actually ALIGN_DOWN the max_address. Thus,
> > > efi won't find enough
> > > free memory in this case if the max_address is computed from the dram_base.
> > >
> > > Is there an implicit requirement for efi_allocate_pages_aligned or
> > > efi_low_alloc_above should be tried in case of failure?
> > >
> >
> > No not really. What ever works for your particular use case is acceptable to me.
> >
> > > > In that case, you don't need this, you can simply pass ULONG_MAX as
> > > > the max address.
> > > >
> > > As RISC-V should allocate memory as low as possible to avoid memory
> > > wastage, I think the following should work.
> > >
> > > efi_low_alloc_above(*reserve_size, MIN_KIMG_ALIGN, reserve_addr, dram_base);
> > >
> > > If this looks okay to you, efi_low_alloc_above should be moved back to
> > > mem.c from relocate.c.
> > > Should I do it in a separate patch or the original patch should be
> > > modified so that efi_low_alloc_above was never moved to relocate.c
> > >
> >
> > No, please keep efi_low_alloc_above() where it is, but drop the
> > static, and put back the declaration in efistub.h
> >
>
> Alternatively, can you check whether efi_relocate_kernel() already
> does what you need?
Yeah. efi_relocate_kernel works too. It's just that
efi_relocate_kernel expects a preferred address
which RISC-V doesn't care about. I can pass ULONG_MAX so that
efi_bs_call will fail and
efi_low_alloc_above will be invoked eventually.
I am also thinking to put a check in relocate_kernel for preferred
address != ULONG_MAX to avoid
an extra efi_bs_call. The extra call doesn't actually create an issue,
just a minor optimization.
But if that is not acceptable for generic code, that is fine as well.
Thanks for your suggestions and time.
--
Regards,
Atish
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [v3 PATCH 5/5] RISC-V: Add EFI stub support.
2020-04-18 19:19 ` Atish Patra
@ 2020-04-18 19:24 ` Ard Biesheuvel
2020-04-20 4:20 ` Atish Patra
0 siblings, 1 reply; 20+ messages in thread
From: Ard Biesheuvel @ 2020-04-18 19:24 UTC (permalink / raw)
To: Atish Patra
Cc: Atish Patra, linux-efi, Arnd Bergmann, Catalin Marinas,
Masahiro Yamada, Linux Kernel Mailing List, Russell King,
Palmer Dabbelt, Greg Kroah-Hartman, linux-riscv, Will Deacon,
Linux ARM
On Sat, 18 Apr 2020 at 21:19, Atish Patra <atishp@atishpatra.org> wrote:
>
> On Sat, Apr 18, 2020 at 5:39 AM Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > On Sat, 18 Apr 2020 at 12:51, Ard Biesheuvel <ardb@kernel.org> wrote:
> > >
> > > On Sat, 18 Apr 2020 at 05:03, Atish Patra <atishp@atishpatra.org> wrote:
> > > >
> > > > On Thu, Apr 16, 2020 at 12:41 AM Ard Biesheuvel <ardb@kernel.org> wrote:
> > > > >
> > > > > On Wed, 15 Apr 2020 at 21:54, Atish Patra <atish.patra@wdc.com> wrote:
> > > > > >
> > > > > > Add a RISC-V architecture specific stub code that actually copies the
> > > > > > actual kernel image to a valid address and jump to it after boot services
> > > > > > are terminated. Enable UEFI related kernel configs as well for RISC-V.
> > > > > >
> > > > > > Signed-off-by: Atish Patra <atish.patra@wdc.com>
> > > > > > ---
> > > > > > arch/riscv/Kconfig | 20 ++++
> > > > > > arch/riscv/Makefile | 1 +
> > > > > > arch/riscv/configs/defconfig | 1 +
> > > > > > arch/riscv/include/asm/efi.h | 44 +++++++++
> > > > > > drivers/firmware/efi/Kconfig | 2 +-
> > > > > > drivers/firmware/efi/libstub/Makefile | 7 ++
> > > > > > drivers/firmware/efi/libstub/riscv-stub.c | 111 ++++++++++++++++++++++
> > > > > > 7 files changed, 185 insertions(+), 1 deletion(-)
> > > > > > create mode 100644 arch/riscv/include/asm/efi.h
> > > > > > create mode 100644 drivers/firmware/efi/libstub/riscv-stub.c
> > > > > >
> > > > > > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> > > > > > index f39e326a7a42..eb4f41c8f3ce 100644
> > > > > > --- a/arch/riscv/Kconfig
> > > > > > +++ b/arch/riscv/Kconfig
> > > > > > @@ -379,10 +379,30 @@ config CMDLINE_FORCE
> > > > > >
> > > > > > endchoice
> > > > > >
> > > > > > +config EFI_STUB
> > > > > > + bool
> > > > > > +
> > > > > > +config EFI
> > > > > > + bool "UEFI runtime support"
> > > > > > + depends on OF
> > > > > > + select LIBFDT
> > > > > > + select UCS2_STRING
> > > > > > + select EFI_PARAMS_FROM_FDT
> > > > > > + select EFI_STUB
> > > > > > + select EFI_GENERIC_STUB
> > > > > > + default y
> > > > > > + help
> > > > > > + This option provides support for runtime services provided
> > > > > > + by UEFI firmware (such as non-volatile variables, realtime
> > > > > > + clock, and platform reset). A UEFI stub is also provided to
> > > > > > + allow the kernel to be booted as an EFI application. This
> > > > > > + is only useful on systems that have UEFI firmware.
> > > > > > +
> > > > > > endmenu
> > > > > >
> > > > > > menu "Power management options"
> > > > > >
> > > > > > source "kernel/power/Kconfig"
> > > > > > +source "drivers/firmware/Kconfig"
> > > > > >
> > > > > > endmenu
> > > > > > diff --git a/arch/riscv/Makefile b/arch/riscv/Makefile
> > > > > > index fb6e37db836d..079435804d6d 100644
> > > > > > --- a/arch/riscv/Makefile
> > > > > > +++ b/arch/riscv/Makefile
> > > > > > @@ -80,6 +80,7 @@ head-y := arch/riscv/kernel/head.o
> > > > > > core-y += arch/riscv/
> > > > > >
> > > > > > libs-y += arch/riscv/lib/
> > > > > > +core-$(CONFIG_EFI_STUB) += $(objtree)/drivers/firmware/efi/libstub/lib.a
> > > > > >
> > > > > > PHONY += vdso_install
> > > > > > vdso_install:
> > > > > > diff --git a/arch/riscv/configs/defconfig b/arch/riscv/configs/defconfig
> > > > > > index 4da4886246a4..ae69e12d306a 100644
> > > > > > --- a/arch/riscv/configs/defconfig
> > > > > > +++ b/arch/riscv/configs/defconfig
> > > > > > @@ -129,3 +129,4 @@ CONFIG_DEBUG_BLOCK_EXT_DEVT=y
> > > > > > # CONFIG_RUNTIME_TESTING_MENU is not set
> > > > > > CONFIG_MEMTEST=y
> > > > > > # CONFIG_SYSFS_SYSCALL is not set
> > > > > > +CONFIG_EFI=y
> > > > > > diff --git a/arch/riscv/include/asm/efi.h b/arch/riscv/include/asm/efi.h
> > > > > > new file mode 100644
> > > > > > index 000000000000..62d7d5eafed8
> > > > > > --- /dev/null
> > > > > > +++ b/arch/riscv/include/asm/efi.h
> > > > > > @@ -0,0 +1,44 @@
> > > > > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > > > > +/*
> > > > > > + * Copyright (C) 2020 Western Digital Corporation or its affiliates.
> > > > > > + * Based on arch/arm64/include/asm/efi.h
> > > > > > + */
> > > > > > +#ifndef _ASM_EFI_H
> > > > > > +#define _ASM_EFI_H
> > > > > > +
> > > > > > +#include <asm/io.h>
> > > > > > +#include <asm/mmu_context.h>
> > > > > > +#include <asm/ptrace.h>
> > > > > > +#include <asm/tlbflush.h>
> > > > > > +
> > > > > > +#define VA_BITS_MIN 39
> > > > > > +
> > > > > > +/* on RISC-V, the FDT may be located anywhere in system RAM */
> > > > > > +static inline unsigned long efi_get_max_fdt_addr(unsigned long dram_base)
> > > > > > +{
> > > > > > + return ULONG_MAX;
> > > > > > +}
> > > > > > +
> > > > > > +/* Load initrd at enough distance from DRAM start */
> > > > > > +static inline unsigned long efi_get_max_initrd_addr(unsigned long dram_base,
> > > > > > + unsigned long image_addr)
> > > > > > +{
> > > > > > + return dram_base + SZ_256M;
> > > > > > +}
> > > > > > +
> > > > > > +#define efi_bs_call(func, ...) efi_system_table()->boottime->func(__VA_ARGS__)
> > > > > > +#define efi_rt_call(func, ...) efi_system_table()->runtime->func(__VA_ARGS__)
> > > > > > +#define efi_is_native() (true)
> > > > > > +
> > > > > > +#define efi_table_attr(inst, attr) (inst->attr)
> > > > > > +
> > > > > > +#define efi_call_proto(inst, func, ...) inst->func(inst, ##__VA_ARGS__)
> > > > > > +
> > > > > > +#define alloc_screen_info(x...) (&screen_info)
> > > > > > +extern char stext_offset[];
> > > > > > +
> > > > > > +static inline void free_screen_info(struct screen_info *si)
> > > > > > +{
> > > > > > +}
> > > > > > +
> > > > > > +#endif /* _ASM_EFI_H */
> > > > > > diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
> > > > > > index 2a2b2b96a1dc..fcdc789d3f87 100644
> > > > > > --- a/drivers/firmware/efi/Kconfig
> > > > > > +++ b/drivers/firmware/efi/Kconfig
> > > > > > @@ -111,7 +111,7 @@ config EFI_GENERIC_STUB
> > > > > >
> > > > > > config EFI_ARMSTUB_DTB_LOADER
> > > > > > bool "Enable the DTB loader"
> > > > > > - depends on EFI_GENERIC_STUB
> > > > > > + depends on EFI_GENERIC_STUB && !RISCV
> > > > > > default y
> > > > > > help
> > > > > > Select this config option to add support for the dtb= command
> > > > > > diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile
> > > > > > index 2b4e09bf987c..7d46b70b51f2 100644
> > > > > > --- a/drivers/firmware/efi/libstub/Makefile
> > > > > > +++ b/drivers/firmware/efi/libstub/Makefile
> > > > > > @@ -22,6 +22,8 @@ cflags-$(CONFIG_ARM64) := $(subst $(CC_FLAGS_FTRACE),,$(KBUILD_CFLAGS)) \
> > > > > > cflags-$(CONFIG_ARM) := $(subst $(CC_FLAGS_FTRACE),,$(KBUILD_CFLAGS)) \
> > > > > > -fno-builtin -fpic \
> > > > > > $(call cc-option,-mno-single-pic-base)
> > > > > > +cflags-$(CONFIG_RISCV) := $(subst $(CC_FLAGS_FTRACE),,$(KBUILD_CFLAGS)) \
> > > > > > + -fpic
> > > > > >
> > > > > > cflags-$(CONFIG_EFI_GENERIC_STUB) += -I$(srctree)/scripts/dtc/libfdt
> > > > > >
> > > > > > @@ -57,6 +59,7 @@ lib-$(CONFIG_EFI_GENERIC_STUB) += efi-stub.o fdt.o string.o \
> > > > > > lib-$(CONFIG_ARM) += arm32-stub.o
> > > > > > lib-$(CONFIG_ARM64) += arm64-stub.o
> > > > > > lib-$(CONFIG_X86) += x86-stub.o
> > > > > > +lib-$(CONFIG_RISCV) += riscv-stub.o
> > > > > > CFLAGS_arm32-stub.o := -DTEXT_OFFSET=$(TEXT_OFFSET)
> > > > > > CFLAGS_arm64-stub.o := -DTEXT_OFFSET=$(TEXT_OFFSET)
> > > > > >
> > > > > > @@ -81,6 +84,10 @@ STUBCOPY_FLAGS-$(CONFIG_ARM64) += --prefix-alloc-sections=.init \
> > > > > > --prefix-symbols=__efistub_
> > > > > > STUBCOPY_RELOC-$(CONFIG_ARM64) := R_AARCH64_ABS
> > > > > >
> > > > > > +STUBCOPY_FLAGS-$(CONFIG_RISCV) += --prefix-alloc-sections=.init \
> > > > > > + --prefix-symbols=__efistub_
> > > > > > +STUBCOPY_RELOC-$(CONFIG_RISCV) := R_RISCV_HI20
> > > > > > +
> > > > > > $(obj)/%.stub.o: $(obj)/%.o FORCE
> > > > > > $(call if_changed,stubcopy)
> > > > > >
> > > > > > diff --git a/drivers/firmware/efi/libstub/riscv-stub.c b/drivers/firmware/efi/libstub/riscv-stub.c
> > > > > > new file mode 100644
> > > > > > index 000000000000..69d13e0ebaea
> > > > > > --- /dev/null
> > > > > > +++ b/drivers/firmware/efi/libstub/riscv-stub.c
> > > > > > @@ -0,0 +1,111 @@
> > > > > > +// SPDX-License-Identifier: GPL-2.0
> > > > > > +/*
> > > > > > + * Copyright (C) 2013, 2014 Linaro Ltd; <roy.franz@linaro.org>
> > > > > > + * Copyright (C) 2020 Western Digital Corporation or its affiliates.
> > > > > > + *
> > > > > > + * This file implements the EFI boot stub for the RISC-V kernel.
> > > > > > + * Adapted from ARM64 version at drivers/firmware/efi/libstub/arm64-stub.c.
> > > > > > + */
> > > > > > +
> > > > > > +#include <linux/efi.h>
> > > > > > +#include <linux/libfdt.h>
> > > > > > +#include <linux/libfdt_env.h>
> > > > > > +#include <asm/efi.h>
> > > > > > +#include <asm/sections.h>
> > > > > > +
> > > > > > +#include "efistub.h"
> > > > > > +/*
> > > > > > + * RISCV requires the kernel image to placed TEXT_OFFSET bytes beyond a 2 MB
> > > > > > + * aligned base for 64 bit and 4MB for 32 bit.
> > > > > > + */
> > > >
> > > > Fixed the comment.
> > > >
> > > > > > +#ifdef CONFIG_64BIT
> > > > > > +#define MIN_KIMG_ALIGN SZ_2M
> > > > > > +#else
> > > > > > +#define MIN_KIMG_ALIGN SZ_4M
> > > > > > +#endif
> > > > > > +
> > > > > > +typedef __attribute__((noreturn)) void (*jump_kernel_func)(unsigned int,
> > > > > > + unsigned long);
> > > > > > +efi_status_t check_platform_features(void)
> > > > > > +{
> > > > > > + return EFI_SUCCESS;
> > > > > > +}
> > > > > > +
> > > > > > +static u32 get_boot_hartid_from_fdt(unsigned long fdt)
> > > > > > +{
> > > > > > + int chosen_node, len;
> > > > > > + const fdt32_t *prop;
> > > > > > +
> > > > > > + chosen_node = fdt_path_offset((void *)fdt, "/chosen");
> > > > > > + if (chosen_node < 0)
> > > > > > + return U32_MAX;
> > > > > > + prop = fdt_getprop((void *)fdt, chosen_node, "boot-hartid", &len);
> > > > > > + if (!prop || len != sizeof(u32))
> > > > > > + return U32_MAX;
> > > > > > +
> > > > > > + return fdt32_to_cpu(*prop);
> > > > > > +}
> > > > > > +
> > > > > > +/*
> > > > > > + * Jump to real kernel here with following constraints.
> > > > > > + * 1. MMU should be disabled.
> > > > > > + * 2. a0 should contain hartid
> > > > > > + * 3. a1 should DT address
> > > > > > + */
> > > > > > +void __noreturn efi_enter_kernel(unsigned long entrypoint, unsigned long fdt,
> > > > > > + unsigned long fdt_size)
> > > > > > +{
> > > > > > + unsigned long kernel_entry = entrypoint + (unsigned long)stext_offset;
> > > > > > + jump_kernel_func jump_kernel = (jump_kernel_func) kernel_entry;
> > > > > > + u32 hartid = get_boot_hartid_from_fdt(fdt);
> > > > > > +
> > > > > > + if (hartid == U32_MAX)
> > > > > > + /* We can not use panic or BUG at this point */
> > > > > > + __asm__ __volatile__ ("ebreak");
> > > > > > + /* Disable MMU */
> > > > > > + csr_write(CSR_SATP, 0);
> > > > > > + jump_kernel(hartid, fdt);
> > > > > > +}
> > > > > > +
> > > > > > +efi_status_t handle_kernel_image(unsigned long *image_addr,
> > > > > > + unsigned long *image_size,
> > > > > > + unsigned long *reserve_addr,
> > > > > > + unsigned long *reserve_size,
> > > > > > + unsigned long dram_base,
> > > > > > + efi_loaded_image_t *image)
> > > > > > +{
> > > > > > + efi_status_t status;
> > > > > > + unsigned long kernel_size, kernel_memsize = 0;
> > > > > > + unsigned long max_alloc_address;
> > > > > > +
> > > > > > + if (image->image_base != _start)
> > > > > > + pr_efi_err("FIRMWARE BUG: efi_loaded_image_t::image_base has bogus value\n");
> > > > > > +
> > > > >
> > > > > I don't think you need this.
> > > > >
> > > >
> > > > Sure. I will remove it. I guess ARM64 code has the error print for
> > > > legacy loader code ?
> > > >
> > >
> > > No, for broken distro versions of GRUB
> > >
> > > > > > + kernel_size = _edata - _start;
> > > > > > + kernel_memsize = kernel_size + (_end - _edata);
> > > > > > + max_alloc_address = round_up(dram_base, MIN_KIMG_ALIGN) +
> > > > > > + kernel_memsize;
> > > > > > +
> > > > >
> > > > > You said the kernel could be anywhere in memory, as long as it is
> > > > > aligned correctly, right?
> > > >
> > > > Sorry I was wrong about this. RISC-V kernel maps PAGE_OFFSET virtual
> > > > address to the
> > > > physical address <xyz> where it is booted. That means memory between
> > > > dram start and and <xyz> address
> > > > will be unusable.
> > > >
> > >
> > > OK
> > >
> > > > I also realized that the above computing max_address as above also
> > > > won't work for the following reason.
> > > > efi_allocate_pages_aligned actually ALIGN_DOWN the max_address. Thus,
> > > > efi won't find enough
> > > > free memory in this case if the max_address is computed from the dram_base.
> > > >
> > > > Is there an implicit requirement for efi_allocate_pages_aligned or
> > > > efi_low_alloc_above should be tried in case of failure?
> > > >
> > >
> > > No not really. What ever works for your particular use case is acceptable to me.
> > >
> > > > > In that case, you don't need this, you can simply pass ULONG_MAX as
> > > > > the max address.
> > > > >
> > > > As RISC-V should allocate memory as low as possible to avoid memory
> > > > wastage, I think the following should work.
> > > >
> > > > efi_low_alloc_above(*reserve_size, MIN_KIMG_ALIGN, reserve_addr, dram_base);
> > > >
> > > > If this looks okay to you, efi_low_alloc_above should be moved back to
> > > > mem.c from relocate.c.
> > > > Should I do it in a separate patch or the original patch should be
> > > > modified so that efi_low_alloc_above was never moved to relocate.c
> > > >
> > >
> > > No, please keep efi_low_alloc_above() where it is, but drop the
> > > static, and put back the declaration in efistub.h
> > >
> >
> > Alternatively, can you check whether efi_relocate_kernel() already
> > does what you need?
>
> Yeah. efi_relocate_kernel works too. It's just that
> efi_relocate_kernel expects a preferred address
> which RISC-V doesn't care about.
There is a preferred address, no? The base of DRAM?
> I can pass ULONG_MAX so that
> efi_bs_call will fail and
> efi_low_alloc_above will be invoked eventually.
>
> I am also thinking to put a check in relocate_kernel for preferred
> address != ULONG_MAX to avoid
> an extra efi_bs_call. The extra call doesn't actually create an issue,
> just a minor optimization.
> But if that is not acceptable for generic code, that is fine as well.
>
> Thanks for your suggestions and time.
> --
> Regards,
> Atish
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [v3 PATCH 5/5] RISC-V: Add EFI stub support.
2020-04-18 19:24 ` Ard Biesheuvel
@ 2020-04-20 4:20 ` Atish Patra
2020-04-20 7:03 ` Ard Biesheuvel
0 siblings, 1 reply; 20+ messages in thread
From: Atish Patra @ 2020-04-20 4:20 UTC (permalink / raw)
To: atishp, ardb
Cc: masahiroy, linux-riscv, linux, gregkh, linux-efi, palmer,
catalin.marinas, linux-kernel, arnd, will
On Sat, 2020-04-18 at 21:24 +0200, Ard Biesheuvel wrote:
> On Sat, 18 Apr 2020 at 21:19, Atish Patra <atishp@atishpatra.org>
> wrote:
> > On Sat, Apr 18, 2020 at 5:39 AM Ard Biesheuvel <ardb@kernel.org>
> > wrote:
> > > On Sat, 18 Apr 2020 at 12:51, Ard Biesheuvel <ardb@kernel.org>
> > > wrote:
> > > > On Sat, 18 Apr 2020 at 05:03, Atish Patra <
> > > > atishp@atishpatra.org> wrote:
> > > > > On Thu, Apr 16, 2020 at 12:41 AM Ard Biesheuvel <
> > > > > ardb@kernel.org> wrote:
> > > > > > On Wed, 15 Apr 2020 at 21:54, Atish Patra <
> > > > > > atish.patra@wdc.com> wrote:
> > > > > > > Add a RISC-V architecture specific stub code that
> > > > > > > actually copies the
> > > > > > > actual kernel image to a valid address and jump to it
> > > > > > > after boot services
> > > > > > > are terminated. Enable UEFI related kernel configs as
> > > > > > > well for RISC-V.
> > > > > > >
> > > > > > > Signed-off-by: Atish Patra <atish.patra@wdc.com>
> > > > > > > ---
> > > > > > > arch/riscv/Kconfig | 20 ++++
> > > > > > > arch/riscv/Makefile | 1 +
> > > > > > > arch/riscv/configs/defconfig | 1 +
> > > > > > > arch/riscv/include/asm/efi.h | 44
> > > > > > > +++++++++
> > > > > > > drivers/firmware/efi/Kconfig | 2 +-
> > > > > > > drivers/firmware/efi/libstub/Makefile | 7 ++
> > > > > > > drivers/firmware/efi/libstub/riscv-stub.c | 111
> > > > > > > ++++++++++++++++++++++
> > > > > > > 7 files changed, 185 insertions(+), 1 deletion(-)
> > > > > > > create mode 100644 arch/riscv/include/asm/efi.h
> > > > > > > create mode 100644 drivers/firmware/efi/libstub/riscv-
> > > > > > > stub.c
> > > > > > >
> > > > > > > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> > > > > > > index f39e326a7a42..eb4f41c8f3ce 100644
> > > > > > > --- a/arch/riscv/Kconfig
> > > > > > > +++ b/arch/riscv/Kconfig
> > > > > > > @@ -379,10 +379,30 @@ config CMDLINE_FORCE
> > > > > > >
> > > > > > > endchoice
> > > > > > >
> > > > > > > +config EFI_STUB
> > > > > > > + bool
> > > > > > > +
> > > > > > > +config EFI
> > > > > > > + bool "UEFI runtime support"
> > > > > > > + depends on OF
> > > > > > > + select LIBFDT
> > > > > > > + select UCS2_STRING
> > > > > > > + select EFI_PARAMS_FROM_FDT
> > > > > > > + select EFI_STUB
> > > > > > > + select EFI_GENERIC_STUB
> > > > > > > + default y
> > > > > > > + help
> > > > > > > + This option provides support for runtime
> > > > > > > services provided
> > > > > > > + by UEFI firmware (such as non-volatile
> > > > > > > variables, realtime
> > > > > > > + clock, and platform reset). A UEFI stub is
> > > > > > > also provided to
> > > > > > > + allow the kernel to be booted as an EFI
> > > > > > > application. This
> > > > > > > + is only useful on systems that have UEFI
> > > > > > > firmware.
> > > > > > > +
> > > > > > > endmenu
> > > > > > >
> > > > > > > menu "Power management options"
> > > > > > >
> > > > > > > source "kernel/power/Kconfig"
> > > > > > > +source "drivers/firmware/Kconfig"
> > > > > > >
> > > > > > > endmenu
> > > > > > > diff --git a/arch/riscv/Makefile b/arch/riscv/Makefile
> > > > > > > index fb6e37db836d..079435804d6d 100644
> > > > > > > --- a/arch/riscv/Makefile
> > > > > > > +++ b/arch/riscv/Makefile
> > > > > > > @@ -80,6 +80,7 @@ head-y := arch/riscv/kernel/head.o
> > > > > > > core-y += arch/riscv/
> > > > > > >
> > > > > > > libs-y += arch/riscv/lib/
> > > > > > > +core-$(CONFIG_EFI_STUB) +=
> > > > > > > $(objtree)/drivers/firmware/efi/libstub/lib.a
> > > > > > >
> > > > > > > PHONY += vdso_install
> > > > > > > vdso_install:
> > > > > > > diff --git a/arch/riscv/configs/defconfig
> > > > > > > b/arch/riscv/configs/defconfig
> > > > > > > index 4da4886246a4..ae69e12d306a 100644
> > > > > > > --- a/arch/riscv/configs/defconfig
> > > > > > > +++ b/arch/riscv/configs/defconfig
> > > > > > > @@ -129,3 +129,4 @@ CONFIG_DEBUG_BLOCK_EXT_DEVT=y
> > > > > > > # CONFIG_RUNTIME_TESTING_MENU is not set
> > > > > > > CONFIG_MEMTEST=y
> > > > > > > # CONFIG_SYSFS_SYSCALL is not set
> > > > > > > +CONFIG_EFI=y
> > > > > > > diff --git a/arch/riscv/include/asm/efi.h
> > > > > > > b/arch/riscv/include/asm/efi.h
> > > > > > > new file mode 100644
> > > > > > > index 000000000000..62d7d5eafed8
> > > > > > > --- /dev/null
> > > > > > > +++ b/arch/riscv/include/asm/efi.h
> > > > > > > @@ -0,0 +1,44 @@
> > > > > > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > > > > > +/*
> > > > > > > + * Copyright (C) 2020 Western Digital Corporation or its
> > > > > > > affiliates.
> > > > > > > + * Based on arch/arm64/include/asm/efi.h
> > > > > > > + */
> > > > > > > +#ifndef _ASM_EFI_H
> > > > > > > +#define _ASM_EFI_H
> > > > > > > +
> > > > > > > +#include <asm/io.h>
> > > > > > > +#include <asm/mmu_context.h>
> > > > > > > +#include <asm/ptrace.h>
> > > > > > > +#include <asm/tlbflush.h>
> > > > > > > +
> > > > > > > +#define VA_BITS_MIN 39
> > > > > > > +
> > > > > > > +/* on RISC-V, the FDT may be located anywhere in system
> > > > > > > RAM */
> > > > > > > +static inline unsigned long
> > > > > > > efi_get_max_fdt_addr(unsigned long dram_base)
> > > > > > > +{
> > > > > > > + return ULONG_MAX;
> > > > > > > +}
> > > > > > > +
> > > > > > > +/* Load initrd at enough distance from DRAM start */
> > > > > > > +static inline unsigned long
> > > > > > > efi_get_max_initrd_addr(unsigned long dram_base,
> > > > > > > + unsig
> > > > > > > ned long image_addr)
> > > > > > > +{
> > > > > > > + return dram_base + SZ_256M;
> > > > > > > +}
> > > > > > > +
> > > > > > > +#define efi_bs_call(func, ...) efi_system_table()-
> > > > > > > >boottime->func(__VA_ARGS__)
> > > > > > > +#define efi_rt_call(func, ...) efi_system_table()-
> > > > > > > >runtime->func(__VA_ARGS__)
> > > > > > > +#define efi_is_native() (true)
> > > > > > > +
> > > > > > > +#define efi_table_attr(inst, attr) (inst->attr)
> > > > > > > +
> > > > > > > +#define efi_call_proto(inst, func, ...) inst->func(inst,
> > > > > > > ##__VA_ARGS__)
> > > > > > > +
> > > > > > > +#define
> > > > > > > alloc_screen_info(x...) (&screen_info)
> > > > > > > +extern char stext_offset[];
> > > > > > > +
> > > > > > > +static inline void free_screen_info(struct screen_info
> > > > > > > *si)
> > > > > > > +{
> > > > > > > +}
> > > > > > > +
> > > > > > > +#endif /* _ASM_EFI_H */
> > > > > > > diff --git a/drivers/firmware/efi/Kconfig
> > > > > > > b/drivers/firmware/efi/Kconfig
> > > > > > > index 2a2b2b96a1dc..fcdc789d3f87 100644
> > > > > > > --- a/drivers/firmware/efi/Kconfig
> > > > > > > +++ b/drivers/firmware/efi/Kconfig
> > > > > > > @@ -111,7 +111,7 @@ config EFI_GENERIC_STUB
> > > > > > >
> > > > > > > config EFI_ARMSTUB_DTB_LOADER
> > > > > > > bool "Enable the DTB loader"
> > > > > > > - depends on EFI_GENERIC_STUB
> > > > > > > + depends on EFI_GENERIC_STUB && !RISCV
> > > > > > > default y
> > > > > > > help
> > > > > > > Select this config option to add support for
> > > > > > > the dtb= command
> > > > > > > diff --git a/drivers/firmware/efi/libstub/Makefile
> > > > > > > b/drivers/firmware/efi/libstub/Makefile
> > > > > > > index 2b4e09bf987c..7d46b70b51f2 100644
> > > > > > > --- a/drivers/firmware/efi/libstub/Makefile
> > > > > > > +++ b/drivers/firmware/efi/libstub/Makefile
> > > > > > > @@ -22,6 +22,8 @@ cflags-
> > > > > > > $(CONFIG_ARM64) := $(subst
> > > > > > > $(CC_FLAGS_FTRACE),,$(KBUILD_CFLAGS)) \
> > > > > > > cflags-$(CONFIG_ARM) := $(subst
> > > > > > > $(CC_FLAGS_FTRACE),,$(KBUILD_CFLAGS)) \
> > > > > > > -fno-builtin -fpic \
> > > > > > > $(call cc-option,-mno-
> > > > > > > single-pic-base)
> > > > > > > +cflags-$(CONFIG_RISCV) := $(subst
> > > > > > > $(CC_FLAGS_FTRACE),,$(KBUILD_CFLAGS)) \
> > > > > > > + -fpic
> > > > > > >
> > > > > > > cflags-$(CONFIG_EFI_GENERIC_STUB) +=
> > > > > > > -I$(srctree)/scripts/dtc/libfdt
> > > > > > >
> > > > > > > @@ -57,6 +59,7 @@ lib-
> > > > > > > $(CONFIG_EFI_GENERIC_STUB) += efi-stub.o
> > > > > > > fdt.o string.o \
> > > > > > > lib-$(CONFIG_ARM) += arm32-stub.o
> > > > > > > lib-$(CONFIG_ARM64) += arm64-stub.o
> > > > > > > lib-$(CONFIG_X86) += x86-stub.o
> > > > > > > +lib-$(CONFIG_RISCV) += riscv-stub.o
> > > > > > > CFLAGS_arm32-stub.o :=
> > > > > > > -DTEXT_OFFSET=$(TEXT_OFFSET)
> > > > > > > CFLAGS_arm64-stub.o :=
> > > > > > > -DTEXT_OFFSET=$(TEXT_OFFSET)
> > > > > > >
> > > > > > > @@ -81,6 +84,10 @@ STUBCOPY_FLAGS-
> > > > > > > $(CONFIG_ARM64) += --prefix-alloc-sections=.init \
> > > > > > > --prefix-
> > > > > > > symbols=__efistub_
> > > > > > > STUBCOPY_RELOC-$(CONFIG_ARM64) := R_AARCH64_ABS
> > > > > > >
> > > > > > > +STUBCOPY_FLAGS-$(CONFIG_RISCV) += --prefix-alloc-
> > > > > > > sections=.init \
> > > > > > > + --prefix-
> > > > > > > symbols=__efistub_
> > > > > > > +STUBCOPY_RELOC-$(CONFIG_RISCV) := R_RISCV_HI20
> > > > > > > +
> > > > > > > $(obj)/%.stub.o: $(obj)/%.o FORCE
> > > > > > > $(call if_changed,stubcopy)
> > > > > > >
> > > > > > > diff --git a/drivers/firmware/efi/libstub/riscv-stub.c
> > > > > > > b/drivers/firmware/efi/libstub/riscv-stub.c
> > > > > > > new file mode 100644
> > > > > > > index 000000000000..69d13e0ebaea
> > > > > > > --- /dev/null
> > > > > > > +++ b/drivers/firmware/efi/libstub/riscv-stub.c
> > > > > > > @@ -0,0 +1,111 @@
> > > > > > > +// SPDX-License-Identifier: GPL-2.0
> > > > > > > +/*
> > > > > > > + * Copyright (C) 2013, 2014 Linaro Ltd; <
> > > > > > > roy.franz@linaro.org>
> > > > > > > + * Copyright (C) 2020 Western Digital Corporation or its
> > > > > > > affiliates.
> > > > > > > + *
> > > > > > > + * This file implements the EFI boot stub for the RISC-V
> > > > > > > kernel.
> > > > > > > + * Adapted from ARM64 version at
> > > > > > > drivers/firmware/efi/libstub/arm64-stub.c.
> > > > > > > + */
> > > > > > > +
> > > > > > > +#include <linux/efi.h>
> > > > > > > +#include <linux/libfdt.h>
> > > > > > > +#include <linux/libfdt_env.h>
> > > > > > > +#include <asm/efi.h>
> > > > > > > +#include <asm/sections.h>
> > > > > > > +
> > > > > > > +#include "efistub.h"
> > > > > > > +/*
> > > > > > > + * RISCV requires the kernel image to placed TEXT_OFFSET
> > > > > > > bytes beyond a 2 MB
> > > > > > > + * aligned base for 64 bit and 4MB for 32 bit.
> > > > > > > + */
> > > > >
> > > > > Fixed the comment.
> > > > >
> > > > > > > +#ifdef CONFIG_64BIT
> > > > > > > +#define MIN_KIMG_ALIGN SZ_2M
> > > > > > > +#else
> > > > > > > +#define MIN_KIMG_ALIGN SZ_4M
> > > > > > > +#endif
> > > > > > > +
> > > > > > > +typedef __attribute__((noreturn)) void
> > > > > > > (*jump_kernel_func)(unsigned int,
> > > > > > > +
> > > > > > > unsigned long);
> > > > > > > +efi_status_t check_platform_features(void)
> > > > > > > +{
> > > > > > > + return EFI_SUCCESS;
> > > > > > > +}
> > > > > > > +
> > > > > > > +static u32 get_boot_hartid_from_fdt(unsigned long fdt)
> > > > > > > +{
> > > > > > > + int chosen_node, len;
> > > > > > > + const fdt32_t *prop;
> > > > > > > +
> > > > > > > + chosen_node = fdt_path_offset((void *)fdt,
> > > > > > > "/chosen");
> > > > > > > + if (chosen_node < 0)
> > > > > > > + return U32_MAX;
> > > > > > > + prop = fdt_getprop((void *)fdt, chosen_node,
> > > > > > > "boot-hartid", &len);
> > > > > > > + if (!prop || len != sizeof(u32))
> > > > > > > + return U32_MAX;
> > > > > > > +
> > > > > > > + return fdt32_to_cpu(*prop);
> > > > > > > +}
> > > > > > > +
> > > > > > > +/*
> > > > > > > + * Jump to real kernel here with following constraints.
> > > > > > > + * 1. MMU should be disabled.
> > > > > > > + * 2. a0 should contain hartid
> > > > > > > + * 3. a1 should DT address
> > > > > > > + */
> > > > > > > +void __noreturn efi_enter_kernel(unsigned long
> > > > > > > entrypoint, unsigned long fdt,
> > > > > > > + unsigned long fdt_size)
> > > > > > > +{
> > > > > > > + unsigned long kernel_entry = entrypoint +
> > > > > > > (unsigned long)stext_offset;
> > > > > > > + jump_kernel_func jump_kernel = (jump_kernel_func)
> > > > > > > kernel_entry;
> > > > > > > + u32 hartid = get_boot_hartid_from_fdt(fdt);
> > > > > > > +
> > > > > > > + if (hartid == U32_MAX)
> > > > > > > + /* We can not use panic or BUG at this
> > > > > > > point */
> > > > > > > + __asm__ __volatile__ ("ebreak");
> > > > > > > + /* Disable MMU */
> > > > > > > + csr_write(CSR_SATP, 0);
> > > > > > > + jump_kernel(hartid, fdt);
> > > > > > > +}
> > > > > > > +
> > > > > > > +efi_status_t handle_kernel_image(unsigned long
> > > > > > > *image_addr,
> > > > > > > + unsigned long
> > > > > > > *image_size,
> > > > > > > + unsigned long
> > > > > > > *reserve_addr,
> > > > > > > + unsigned long
> > > > > > > *reserve_size,
> > > > > > > + unsigned long dram_base,
> > > > > > > + efi_loaded_image_t
> > > > > > > *image)
> > > > > > > +{
> > > > > > > + efi_status_t status;
> > > > > > > + unsigned long kernel_size, kernel_memsize = 0;
> > > > > > > + unsigned long max_alloc_address;
> > > > > > > +
> > > > > > > + if (image->image_base != _start)
> > > > > > > + pr_efi_err("FIRMWARE BUG:
> > > > > > > efi_loaded_image_t::image_base has bogus value\n");
> > > > > > > +
> > > > > >
> > > > > > I don't think you need this.
> > > > > >
> > > > >
> > > > > Sure. I will remove it. I guess ARM64 code has the error
> > > > > print for
> > > > > legacy loader code ?
> > > > >
> > > >
> > > > No, for broken distro versions of GRUB
> > > >
> > > > > > > + kernel_size = _edata - _start;
> > > > > > > + kernel_memsize = kernel_size + (_end - _edata);
> > > > > > > + max_alloc_address = round_up(dram_base,
> > > > > > > MIN_KIMG_ALIGN) +
> > > > > > > + kernel_memsize;
> > > > > > > +
> > > > > >
> > > > > > You said the kernel could be anywhere in memory, as long as
> > > > > > it is
> > > > > > aligned correctly, right?
> > > > >
> > > > > Sorry I was wrong about this. RISC-V kernel maps PAGE_OFFSET
> > > > > virtual
> > > > > address to the
> > > > > physical address <xyz> where it is booted. That means memory
> > > > > between
> > > > > dram start and and <xyz> address
> > > > > will be unusable.
> > > > >
> > > >
> > > > OK
> > > >
> > > > > I also realized that the above computing max_address as above
> > > > > also
> > > > > won't work for the following reason.
> > > > > efi_allocate_pages_aligned actually ALIGN_DOWN the
> > > > > max_address. Thus,
> > > > > efi won't find enough
> > > > > free memory in this case if the max_address is computed from
> > > > > the dram_base.
> > > > >
> > > > > Is there an implicit requirement for
> > > > > efi_allocate_pages_aligned or
> > > > > efi_low_alloc_above should be tried in case of failure?
> > > > >
> > > >
> > > > No not really. What ever works for your particular use case is
> > > > acceptable to me.
> > > >
> > > > > > In that case, you don't need this, you can simply pass
> > > > > > ULONG_MAX as
> > > > > > the max address.
> > > > > >
> > > > > As RISC-V should allocate memory as low as possible to avoid
> > > > > memory
> > > > > wastage, I think the following should work.
> > > > >
> > > > > efi_low_alloc_above(*reserve_size, MIN_KIMG_ALIGN,
> > > > > reserve_addr, dram_base);
> > > > >
> > > > > If this looks okay to you, efi_low_alloc_above should be
> > > > > moved back to
> > > > > mem.c from relocate.c.
> > > > > Should I do it in a separate patch or the original patch
> > > > > should be
> > > > > modified so that efi_low_alloc_above was never moved to
> > > > > relocate.c
> > > > >
> > > >
> > > > No, please keep efi_low_alloc_above() where it is, but drop the
> > > > static, and put back the declaration in efistub.h
> > > >
> > >
> > > Alternatively, can you check whether efi_relocate_kernel()
> > > already
> > > does what you need?
> >
> > Yeah. efi_relocate_kernel works too. It's just that
> > efi_relocate_kernel expects a preferred address
> > which RISC-V doesn't care about.
>
> There is a preferred address, no? The base of DRAM?
>
I just realized that my reply from my gmail never landed in any of the
mailing lists. If you have already received it, sorry for the spam.
"If the preferred address is set as the base of DRAM, efi_bs_call is
bound to fail as well because the base of DRAM is reserved by the
firmware. So the efi memory allocator can't allocate at that address.
Technically, it will work but it is no different than passing
ULONG_MAX. So I thought ULONG_MAX will avoid the confusion.
We try to allocate as low as possible so I am passing dram_base as the
minimum address anyways. As the firmware reserved the first few KBs,
> > I can pass ULONG_MAX so that
> > efi_bs_call will fail and
> > efi_low_alloc_above will be invoked eventually.
> >
> > I am also thinking to put a check in relocate_kernel for preferred
> > address != ULONG_MAX to avoid
> > an extra efi_bs_call. The extra call doesn't actually create an
> > issue,
> > just a minor optimization.
> > But if that is not acceptable for generic code, that is fine as
> > well.
> >
> > Thanks for your suggestions and time.
> > --
> > Regards,
> > Atish
--
Regards,
Atish
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [v3 PATCH 5/5] RISC-V: Add EFI stub support.
2020-04-20 4:20 ` Atish Patra
@ 2020-04-20 7:03 ` Ard Biesheuvel
2020-04-20 7:59 ` Atish Patra
0 siblings, 1 reply; 20+ messages in thread
From: Ard Biesheuvel @ 2020-04-20 7:03 UTC (permalink / raw)
To: Atish Patra
Cc: atishp, masahiroy, linux-riscv, linux, gregkh, linux-efi, palmer,
catalin.marinas, linux-kernel, arnd, will
On Mon, 20 Apr 2020 at 06:20, Atish Patra <Atish.Patra@wdc.com> wrote:
>
> On Sat, 2020-04-18 at 21:24 +0200, Ard Biesheuvel wrote:
> > On Sat, 18 Apr 2020 at 21:19, Atish Patra <atishp@atishpatra.org>
> > wrote:
> > > On Sat, Apr 18, 2020 at 5:39 AM Ard Biesheuvel <ardb@kernel.org>
> > > wrote:
> > > > On Sat, 18 Apr 2020 at 12:51, Ard Biesheuvel <ardb@kernel.org>
> > > > wrote:
> > > > > On Sat, 18 Apr 2020 at 05:03, Atish Patra <
> > > > > atishp@atishpatra.org> wrote:
> > > > > > On Thu, Apr 16, 2020 at 12:41 AM Ard Biesheuvel <
> > > > > > ardb@kernel.org> wrote:
> > > > > > > On Wed, 15 Apr 2020 at 21:54, Atish Patra <
> > > > > > > atish.patra@wdc.com> wrote:
> > > > > > > > Add a RISC-V architecture specific stub code that
> > > > > > > > actually copies the
> > > > > > > > actual kernel image to a valid address and jump to it
> > > > > > > > after boot services
> > > > > > > > are terminated. Enable UEFI related kernel configs as
> > > > > > > > well for RISC-V.
> > > > > > > >
> > > > > > > > Signed-off-by: Atish Patra <atish.patra@wdc.com>
> > > > > > > > ---
> > > > > > > > arch/riscv/Kconfig | 20 ++++
> > > > > > > > arch/riscv/Makefile | 1 +
> > > > > > > > arch/riscv/configs/defconfig | 1 +
> > > > > > > > arch/riscv/include/asm/efi.h | 44
> > > > > > > > +++++++++
> > > > > > > > drivers/firmware/efi/Kconfig | 2 +-
> > > > > > > > drivers/firmware/efi/libstub/Makefile | 7 ++
> > > > > > > > drivers/firmware/efi/libstub/riscv-stub.c | 111
> > > > > > > > ++++++++++++++++++++++
> > > > > > > > 7 files changed, 185 insertions(+), 1 deletion(-)
> > > > > > > > create mode 100644 arch/riscv/include/asm/efi.h
> > > > > > > > create mode 100644 drivers/firmware/efi/libstub/riscv-
> > > > > > > > stub.c
> > > > > > > >
> > > > > > > > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> > > > > > > > index f39e326a7a42..eb4f41c8f3ce 100644
> > > > > > > > --- a/arch/riscv/Kconfig
> > > > > > > > +++ b/arch/riscv/Kconfig
> > > > > > > > @@ -379,10 +379,30 @@ config CMDLINE_FORCE
> > > > > > > >
> > > > > > > > endchoice
> > > > > > > >
> > > > > > > > +config EFI_STUB
> > > > > > > > + bool
> > > > > > > > +
> > > > > > > > +config EFI
> > > > > > > > + bool "UEFI runtime support"
> > > > > > > > + depends on OF
> > > > > > > > + select LIBFDT
> > > > > > > > + select UCS2_STRING
> > > > > > > > + select EFI_PARAMS_FROM_FDT
> > > > > > > > + select EFI_STUB
> > > > > > > > + select EFI_GENERIC_STUB
> > > > > > > > + default y
> > > > > > > > + help
> > > > > > > > + This option provides support for runtime
> > > > > > > > services provided
> > > > > > > > + by UEFI firmware (such as non-volatile
> > > > > > > > variables, realtime
> > > > > > > > + clock, and platform reset). A UEFI stub is
> > > > > > > > also provided to
> > > > > > > > + allow the kernel to be booted as an EFI
> > > > > > > > application. This
> > > > > > > > + is only useful on systems that have UEFI
> > > > > > > > firmware.
> > > > > > > > +
> > > > > > > > endmenu
> > > > > > > >
> > > > > > > > menu "Power management options"
> > > > > > > >
> > > > > > > > source "kernel/power/Kconfig"
> > > > > > > > +source "drivers/firmware/Kconfig"
> > > > > > > >
> > > > > > > > endmenu
> > > > > > > > diff --git a/arch/riscv/Makefile b/arch/riscv/Makefile
> > > > > > > > index fb6e37db836d..079435804d6d 100644
> > > > > > > > --- a/arch/riscv/Makefile
> > > > > > > > +++ b/arch/riscv/Makefile
> > > > > > > > @@ -80,6 +80,7 @@ head-y := arch/riscv/kernel/head.o
> > > > > > > > core-y += arch/riscv/
> > > > > > > >
> > > > > > > > libs-y += arch/riscv/lib/
> > > > > > > > +core-$(CONFIG_EFI_STUB) +=
> > > > > > > > $(objtree)/drivers/firmware/efi/libstub/lib.a
> > > > > > > >
> > > > > > > > PHONY += vdso_install
> > > > > > > > vdso_install:
> > > > > > > > diff --git a/arch/riscv/configs/defconfig
> > > > > > > > b/arch/riscv/configs/defconfig
> > > > > > > > index 4da4886246a4..ae69e12d306a 100644
> > > > > > > > --- a/arch/riscv/configs/defconfig
> > > > > > > > +++ b/arch/riscv/configs/defconfig
> > > > > > > > @@ -129,3 +129,4 @@ CONFIG_DEBUG_BLOCK_EXT_DEVT=y
> > > > > > > > # CONFIG_RUNTIME_TESTING_MENU is not set
> > > > > > > > CONFIG_MEMTEST=y
> > > > > > > > # CONFIG_SYSFS_SYSCALL is not set
> > > > > > > > +CONFIG_EFI=y
> > > > > > > > diff --git a/arch/riscv/include/asm/efi.h
> > > > > > > > b/arch/riscv/include/asm/efi.h
> > > > > > > > new file mode 100644
> > > > > > > > index 000000000000..62d7d5eafed8
> > > > > > > > --- /dev/null
> > > > > > > > +++ b/arch/riscv/include/asm/efi.h
> > > > > > > > @@ -0,0 +1,44 @@
> > > > > > > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > > > > > > +/*
> > > > > > > > + * Copyright (C) 2020 Western Digital Corporation or its
> > > > > > > > affiliates.
> > > > > > > > + * Based on arch/arm64/include/asm/efi.h
> > > > > > > > + */
> > > > > > > > +#ifndef _ASM_EFI_H
> > > > > > > > +#define _ASM_EFI_H
> > > > > > > > +
> > > > > > > > +#include <asm/io.h>
> > > > > > > > +#include <asm/mmu_context.h>
> > > > > > > > +#include <asm/ptrace.h>
> > > > > > > > +#include <asm/tlbflush.h>
> > > > > > > > +
> > > > > > > > +#define VA_BITS_MIN 39
> > > > > > > > +
> > > > > > > > +/* on RISC-V, the FDT may be located anywhere in system
> > > > > > > > RAM */
> > > > > > > > +static inline unsigned long
> > > > > > > > efi_get_max_fdt_addr(unsigned long dram_base)
> > > > > > > > +{
> > > > > > > > + return ULONG_MAX;
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +/* Load initrd at enough distance from DRAM start */
> > > > > > > > +static inline unsigned long
> > > > > > > > efi_get_max_initrd_addr(unsigned long dram_base,
> > > > > > > > + unsig
> > > > > > > > ned long image_addr)
> > > > > > > > +{
> > > > > > > > + return dram_base + SZ_256M;
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +#define efi_bs_call(func, ...) efi_system_table()-
> > > > > > > > >boottime->func(__VA_ARGS__)
> > > > > > > > +#define efi_rt_call(func, ...) efi_system_table()-
> > > > > > > > >runtime->func(__VA_ARGS__)
> > > > > > > > +#define efi_is_native() (true)
> > > > > > > > +
> > > > > > > > +#define efi_table_attr(inst, attr) (inst->attr)
> > > > > > > > +
> > > > > > > > +#define efi_call_proto(inst, func, ...) inst->func(inst,
> > > > > > > > ##__VA_ARGS__)
> > > > > > > > +
> > > > > > > > +#define
> > > > > > > > alloc_screen_info(x...) (&screen_info)
> > > > > > > > +extern char stext_offset[];
> > > > > > > > +
> > > > > > > > +static inline void free_screen_info(struct screen_info
> > > > > > > > *si)
> > > > > > > > +{
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +#endif /* _ASM_EFI_H */
> > > > > > > > diff --git a/drivers/firmware/efi/Kconfig
> > > > > > > > b/drivers/firmware/efi/Kconfig
> > > > > > > > index 2a2b2b96a1dc..fcdc789d3f87 100644
> > > > > > > > --- a/drivers/firmware/efi/Kconfig
> > > > > > > > +++ b/drivers/firmware/efi/Kconfig
> > > > > > > > @@ -111,7 +111,7 @@ config EFI_GENERIC_STUB
> > > > > > > >
> > > > > > > > config EFI_ARMSTUB_DTB_LOADER
> > > > > > > > bool "Enable the DTB loader"
> > > > > > > > - depends on EFI_GENERIC_STUB
> > > > > > > > + depends on EFI_GENERIC_STUB && !RISCV
> > > > > > > > default y
> > > > > > > > help
> > > > > > > > Select this config option to add support for
> > > > > > > > the dtb= command
> > > > > > > > diff --git a/drivers/firmware/efi/libstub/Makefile
> > > > > > > > b/drivers/firmware/efi/libstub/Makefile
> > > > > > > > index 2b4e09bf987c..7d46b70b51f2 100644
> > > > > > > > --- a/drivers/firmware/efi/libstub/Makefile
> > > > > > > > +++ b/drivers/firmware/efi/libstub/Makefile
> > > > > > > > @@ -22,6 +22,8 @@ cflags-
> > > > > > > > $(CONFIG_ARM64) := $(subst
> > > > > > > > $(CC_FLAGS_FTRACE),,$(KBUILD_CFLAGS)) \
> > > > > > > > cflags-$(CONFIG_ARM) := $(subst
> > > > > > > > $(CC_FLAGS_FTRACE),,$(KBUILD_CFLAGS)) \
> > > > > > > > -fno-builtin -fpic \
> > > > > > > > $(call cc-option,-mno-
> > > > > > > > single-pic-base)
> > > > > > > > +cflags-$(CONFIG_RISCV) := $(subst
> > > > > > > > $(CC_FLAGS_FTRACE),,$(KBUILD_CFLAGS)) \
> > > > > > > > + -fpic
> > > > > > > >
> > > > > > > > cflags-$(CONFIG_EFI_GENERIC_STUB) +=
> > > > > > > > -I$(srctree)/scripts/dtc/libfdt
> > > > > > > >
> > > > > > > > @@ -57,6 +59,7 @@ lib-
> > > > > > > > $(CONFIG_EFI_GENERIC_STUB) += efi-stub.o
> > > > > > > > fdt.o string.o \
> > > > > > > > lib-$(CONFIG_ARM) += arm32-stub.o
> > > > > > > > lib-$(CONFIG_ARM64) += arm64-stub.o
> > > > > > > > lib-$(CONFIG_X86) += x86-stub.o
> > > > > > > > +lib-$(CONFIG_RISCV) += riscv-stub.o
> > > > > > > > CFLAGS_arm32-stub.o :=
> > > > > > > > -DTEXT_OFFSET=$(TEXT_OFFSET)
> > > > > > > > CFLAGS_arm64-stub.o :=
> > > > > > > > -DTEXT_OFFSET=$(TEXT_OFFSET)
> > > > > > > >
> > > > > > > > @@ -81,6 +84,10 @@ STUBCOPY_FLAGS-
> > > > > > > > $(CONFIG_ARM64) += --prefix-alloc-sections=.init \
> > > > > > > > --prefix-
> > > > > > > > symbols=__efistub_
> > > > > > > > STUBCOPY_RELOC-$(CONFIG_ARM64) := R_AARCH64_ABS
> > > > > > > >
> > > > > > > > +STUBCOPY_FLAGS-$(CONFIG_RISCV) += --prefix-alloc-
> > > > > > > > sections=.init \
> > > > > > > > + --prefix-
> > > > > > > > symbols=__efistub_
> > > > > > > > +STUBCOPY_RELOC-$(CONFIG_RISCV) := R_RISCV_HI20
> > > > > > > > +
> > > > > > > > $(obj)/%.stub.o: $(obj)/%.o FORCE
> > > > > > > > $(call if_changed,stubcopy)
> > > > > > > >
> > > > > > > > diff --git a/drivers/firmware/efi/libstub/riscv-stub.c
> > > > > > > > b/drivers/firmware/efi/libstub/riscv-stub.c
> > > > > > > > new file mode 100644
> > > > > > > > index 000000000000..69d13e0ebaea
> > > > > > > > --- /dev/null
> > > > > > > > +++ b/drivers/firmware/efi/libstub/riscv-stub.c
> > > > > > > > @@ -0,0 +1,111 @@
> > > > > > > > +// SPDX-License-Identifier: GPL-2.0
> > > > > > > > +/*
> > > > > > > > + * Copyright (C) 2013, 2014 Linaro Ltd; <
> > > > > > > > roy.franz@linaro.org>
> > > > > > > > + * Copyright (C) 2020 Western Digital Corporation or its
> > > > > > > > affiliates.
> > > > > > > > + *
> > > > > > > > + * This file implements the EFI boot stub for the RISC-V
> > > > > > > > kernel.
> > > > > > > > + * Adapted from ARM64 version at
> > > > > > > > drivers/firmware/efi/libstub/arm64-stub.c.
> > > > > > > > + */
> > > > > > > > +
> > > > > > > > +#include <linux/efi.h>
> > > > > > > > +#include <linux/libfdt.h>
> > > > > > > > +#include <linux/libfdt_env.h>
> > > > > > > > +#include <asm/efi.h>
> > > > > > > > +#include <asm/sections.h>
> > > > > > > > +
> > > > > > > > +#include "efistub.h"
> > > > > > > > +/*
> > > > > > > > + * RISCV requires the kernel image to placed TEXT_OFFSET
> > > > > > > > bytes beyond a 2 MB
> > > > > > > > + * aligned base for 64 bit and 4MB for 32 bit.
> > > > > > > > + */
> > > > > >
> > > > > > Fixed the comment.
> > > > > >
> > > > > > > > +#ifdef CONFIG_64BIT
> > > > > > > > +#define MIN_KIMG_ALIGN SZ_2M
> > > > > > > > +#else
> > > > > > > > +#define MIN_KIMG_ALIGN SZ_4M
> > > > > > > > +#endif
> > > > > > > > +
> > > > > > > > +typedef __attribute__((noreturn)) void
> > > > > > > > (*jump_kernel_func)(unsigned int,
> > > > > > > > +
> > > > > > > > unsigned long);
> > > > > > > > +efi_status_t check_platform_features(void)
> > > > > > > > +{
> > > > > > > > + return EFI_SUCCESS;
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +static u32 get_boot_hartid_from_fdt(unsigned long fdt)
> > > > > > > > +{
> > > > > > > > + int chosen_node, len;
> > > > > > > > + const fdt32_t *prop;
> > > > > > > > +
> > > > > > > > + chosen_node = fdt_path_offset((void *)fdt,
> > > > > > > > "/chosen");
> > > > > > > > + if (chosen_node < 0)
> > > > > > > > + return U32_MAX;
> > > > > > > > + prop = fdt_getprop((void *)fdt, chosen_node,
> > > > > > > > "boot-hartid", &len);
> > > > > > > > + if (!prop || len != sizeof(u32))
> > > > > > > > + return U32_MAX;
> > > > > > > > +
> > > > > > > > + return fdt32_to_cpu(*prop);
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +/*
> > > > > > > > + * Jump to real kernel here with following constraints.
> > > > > > > > + * 1. MMU should be disabled.
> > > > > > > > + * 2. a0 should contain hartid
> > > > > > > > + * 3. a1 should DT address
> > > > > > > > + */
> > > > > > > > +void __noreturn efi_enter_kernel(unsigned long
> > > > > > > > entrypoint, unsigned long fdt,
> > > > > > > > + unsigned long fdt_size)
> > > > > > > > +{
> > > > > > > > + unsigned long kernel_entry = entrypoint +
> > > > > > > > (unsigned long)stext_offset;
> > > > > > > > + jump_kernel_func jump_kernel = (jump_kernel_func)
> > > > > > > > kernel_entry;
> > > > > > > > + u32 hartid = get_boot_hartid_from_fdt(fdt);
> > > > > > > > +
> > > > > > > > + if (hartid == U32_MAX)
> > > > > > > > + /* We can not use panic or BUG at this
> > > > > > > > point */
> > > > > > > > + __asm__ __volatile__ ("ebreak");
> > > > > > > > + /* Disable MMU */
> > > > > > > > + csr_write(CSR_SATP, 0);
> > > > > > > > + jump_kernel(hartid, fdt);
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +efi_status_t handle_kernel_image(unsigned long
> > > > > > > > *image_addr,
> > > > > > > > + unsigned long
> > > > > > > > *image_size,
> > > > > > > > + unsigned long
> > > > > > > > *reserve_addr,
> > > > > > > > + unsigned long
> > > > > > > > *reserve_size,
> > > > > > > > + unsigned long dram_base,
> > > > > > > > + efi_loaded_image_t
> > > > > > > > *image)
> > > > > > > > +{
> > > > > > > > + efi_status_t status;
> > > > > > > > + unsigned long kernel_size, kernel_memsize = 0;
> > > > > > > > + unsigned long max_alloc_address;
> > > > > > > > +
> > > > > > > > + if (image->image_base != _start)
> > > > > > > > + pr_efi_err("FIRMWARE BUG:
> > > > > > > > efi_loaded_image_t::image_base has bogus value\n");
> > > > > > > > +
> > > > > > >
> > > > > > > I don't think you need this.
> > > > > > >
> > > > > >
> > > > > > Sure. I will remove it. I guess ARM64 code has the error
> > > > > > print for
> > > > > > legacy loader code ?
> > > > > >
> > > > >
> > > > > No, for broken distro versions of GRUB
> > > > >
> > > > > > > > + kernel_size = _edata - _start;
> > > > > > > > + kernel_memsize = kernel_size + (_end - _edata);
> > > > > > > > + max_alloc_address = round_up(dram_base,
> > > > > > > > MIN_KIMG_ALIGN) +
> > > > > > > > + kernel_memsize;
> > > > > > > > +
> > > > > > >
> > > > > > > You said the kernel could be anywhere in memory, as long as
> > > > > > > it is
> > > > > > > aligned correctly, right?
> > > > > >
> > > > > > Sorry I was wrong about this. RISC-V kernel maps PAGE_OFFSET
> > > > > > virtual
> > > > > > address to the
> > > > > > physical address <xyz> where it is booted. That means memory
> > > > > > between
> > > > > > dram start and and <xyz> address
> > > > > > will be unusable.
> > > > > >
> > > > >
> > > > > OK
> > > > >
> > > > > > I also realized that the above computing max_address as above
> > > > > > also
> > > > > > won't work for the following reason.
> > > > > > efi_allocate_pages_aligned actually ALIGN_DOWN the
> > > > > > max_address. Thus,
> > > > > > efi won't find enough
> > > > > > free memory in this case if the max_address is computed from
> > > > > > the dram_base.
> > > > > >
> > > > > > Is there an implicit requirement for
> > > > > > efi_allocate_pages_aligned or
> > > > > > efi_low_alloc_above should be tried in case of failure?
> > > > > >
> > > > >
> > > > > No not really. What ever works for your particular use case is
> > > > > acceptable to me.
> > > > >
> > > > > > > In that case, you don't need this, you can simply pass
> > > > > > > ULONG_MAX as
> > > > > > > the max address.
> > > > > > >
> > > > > > As RISC-V should allocate memory as low as possible to avoid
> > > > > > memory
> > > > > > wastage, I think the following should work.
> > > > > >
> > > > > > efi_low_alloc_above(*reserve_size, MIN_KIMG_ALIGN,
> > > > > > reserve_addr, dram_base);
> > > > > >
> > > > > > If this looks okay to you, efi_low_alloc_above should be
> > > > > > moved back to
> > > > > > mem.c from relocate.c.
> > > > > > Should I do it in a separate patch or the original patch
> > > > > > should be
> > > > > > modified so that efi_low_alloc_above was never moved to
> > > > > > relocate.c
> > > > > >
> > > > >
> > > > > No, please keep efi_low_alloc_above() where it is, but drop the
> > > > > static, and put back the declaration in efistub.h
> > > > >
> > > >
> > > > Alternatively, can you check whether efi_relocate_kernel()
> > > > already
> > > > does what you need?
> > >
> > > Yeah. efi_relocate_kernel works too. It's just that
> > > efi_relocate_kernel expects a preferred address
> > > which RISC-V doesn't care about.
> >
> > There is a preferred address, no? The base of DRAM?
> >
>
> I just realized that my reply from my gmail never landed in any of the
> mailing lists. If you have already received it, sorry for the spam.
>
> "If the preferred address is set as the base of DRAM, efi_bs_call is
> bound to fail as well because the base of DRAM is reserved by the
> firmware. So the efi memory allocator can't allocate at that address.
> Technically, it will work but it is no different than passing
> ULONG_MAX. So I thought ULONG_MAX will avoid the confusion.
>
> We try to allocate as low as possible so I am passing dram_base as the
> minimum address anyways. As the firmware reserved the first few KBs,
>
OK, so the preferred address *is* the base of DRAM (assuming it is 2
MB aligned). However, in the general case, you keep some firmware
state there (couple of KB) and so you typically end up at DRAM base
plus 2 MB?
So first question: why does the firmware put this stuff at the base of
DRAM in the first place? Does it *have* to live there?
Then, if the base of DRAM is guaranteed to be occupied, why not make
the preferred address base of DRAM + 2 MB ? (or 4 MB for the 32-bit
case)
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [v3 PATCH 5/5] RISC-V: Add EFI stub support.
2020-04-20 7:03 ` Ard Biesheuvel
@ 2020-04-20 7:59 ` Atish Patra
2020-04-20 8:02 ` Ard Biesheuvel
0 siblings, 1 reply; 20+ messages in thread
From: Atish Patra @ 2020-04-20 7:59 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Atish Patra, masahiroy, linux-riscv, linux-efi, palmer,
catalin.marinas, linux-kernel, arnd, will
On Mon, Apr 20, 2020 at 12:04 AM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Mon, 20 Apr 2020 at 06:20, Atish Patra <Atish.Patra@wdc.com> wrote:
> >
> > On Sat, 2020-04-18 at 21:24 +0200, Ard Biesheuvel wrote:
> > > On Sat, 18 Apr 2020 at 21:19, Atish Patra <atishp@atishpatra.org>
> > > wrote:
> > > > On Sat, Apr 18, 2020 at 5:39 AM Ard Biesheuvel <ardb@kernel.org>
> > > > wrote:
> > > > > On Sat, 18 Apr 2020 at 12:51, Ard Biesheuvel <ardb@kernel.org>
> > > > > wrote:
> > > > > > On Sat, 18 Apr 2020 at 05:03, Atish Patra <
> > > > > > atishp@atishpatra.org> wrote:
> > > > > > > On Thu, Apr 16, 2020 at 12:41 AM Ard Biesheuvel <
> > > > > > > ardb@kernel.org> wrote:
> > > > > > > > On Wed, 15 Apr 2020 at 21:54, Atish Patra <
> > > > > > > > atish.patra@wdc.com> wrote:
> > > > > > > > > Add a RISC-V architecture specific stub code that
> > > > > > > > > actually copies the
> > > > > > > > > actual kernel image to a valid address and jump to it
> > > > > > > > > after boot services
> > > > > > > > > are terminated. Enable UEFI related kernel configs as
> > > > > > > > > well for RISC-V.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Atish Patra <atish.patra@wdc.com>
> > > > > > > > > ---
> > > > > > > > > arch/riscv/Kconfig | 20 ++++
> > > > > > > > > arch/riscv/Makefile | 1 +
> > > > > > > > > arch/riscv/configs/defconfig | 1 +
> > > > > > > > > arch/riscv/include/asm/efi.h | 44
> > > > > > > > > +++++++++
> > > > > > > > > drivers/firmware/efi/Kconfig | 2 +-
> > > > > > > > > drivers/firmware/efi/libstub/Makefile | 7 ++
> > > > > > > > > drivers/firmware/efi/libstub/riscv-stub.c | 111
> > > > > > > > > ++++++++++++++++++++++
> > > > > > > > > 7 files changed, 185 insertions(+), 1 deletion(-)
> > > > > > > > > create mode 100644 arch/riscv/include/asm/efi.h
> > > > > > > > > create mode 100644 drivers/firmware/efi/libstub/riscv-
> > > > > > > > > stub.c
> > > > > > > > >
> > > > > > > > > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> > > > > > > > > index f39e326a7a42..eb4f41c8f3ce 100644
> > > > > > > > > --- a/arch/riscv/Kconfig
> > > > > > > > > +++ b/arch/riscv/Kconfig
> > > > > > > > > @@ -379,10 +379,30 @@ config CMDLINE_FORCE
> > > > > > > > >
> > > > > > > > > endchoice
> > > > > > > > >
> > > > > > > > > +config EFI_STUB
> > > > > > > > > + bool
> > > > > > > > > +
> > > > > > > > > +config EFI
> > > > > > > > > + bool "UEFI runtime support"
> > > > > > > > > + depends on OF
> > > > > > > > > + select LIBFDT
> > > > > > > > > + select UCS2_STRING
> > > > > > > > > + select EFI_PARAMS_FROM_FDT
> > > > > > > > > + select EFI_STUB
> > > > > > > > > + select EFI_GENERIC_STUB
> > > > > > > > > + default y
> > > > > > > > > + help
> > > > > > > > > + This option provides support for runtime
> > > > > > > > > services provided
> > > > > > > > > + by UEFI firmware (such as non-volatile
> > > > > > > > > variables, realtime
> > > > > > > > > + clock, and platform reset). A UEFI stub is
> > > > > > > > > also provided to
> > > > > > > > > + allow the kernel to be booted as an EFI
> > > > > > > > > application. This
> > > > > > > > > + is only useful on systems that have UEFI
> > > > > > > > > firmware.
> > > > > > > > > +
> > > > > > > > > endmenu
> > > > > > > > >
> > > > > > > > > menu "Power management options"
> > > > > > > > >
> > > > > > > > > source "kernel/power/Kconfig"
> > > > > > > > > +source "drivers/firmware/Kconfig"
> > > > > > > > >
> > > > > > > > > endmenu
> > > > > > > > > diff --git a/arch/riscv/Makefile b/arch/riscv/Makefile
> > > > > > > > > index fb6e37db836d..079435804d6d 100644
> > > > > > > > > --- a/arch/riscv/Makefile
> > > > > > > > > +++ b/arch/riscv/Makefile
> > > > > > > > > @@ -80,6 +80,7 @@ head-y := arch/riscv/kernel/head.o
> > > > > > > > > core-y += arch/riscv/
> > > > > > > > >
> > > > > > > > > libs-y += arch/riscv/lib/
> > > > > > > > > +core-$(CONFIG_EFI_STUB) +=
> > > > > > > > > $(objtree)/drivers/firmware/efi/libstub/lib.a
> > > > > > > > >
> > > > > > > > > PHONY += vdso_install
> > > > > > > > > vdso_install:
> > > > > > > > > diff --git a/arch/riscv/configs/defconfig
> > > > > > > > > b/arch/riscv/configs/defconfig
> > > > > > > > > index 4da4886246a4..ae69e12d306a 100644
> > > > > > > > > --- a/arch/riscv/configs/defconfig
> > > > > > > > > +++ b/arch/riscv/configs/defconfig
> > > > > > > > > @@ -129,3 +129,4 @@ CONFIG_DEBUG_BLOCK_EXT_DEVT=y
> > > > > > > > > # CONFIG_RUNTIME_TESTING_MENU is not set
> > > > > > > > > CONFIG_MEMTEST=y
> > > > > > > > > # CONFIG_SYSFS_SYSCALL is not set
> > > > > > > > > +CONFIG_EFI=y
> > > > > > > > > diff --git a/arch/riscv/include/asm/efi.h
> > > > > > > > > b/arch/riscv/include/asm/efi.h
> > > > > > > > > new file mode 100644
> > > > > > > > > index 000000000000..62d7d5eafed8
> > > > > > > > > --- /dev/null
> > > > > > > > > +++ b/arch/riscv/include/asm/efi.h
> > > > > > > > > @@ -0,0 +1,44 @@
> > > > > > > > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > > > > > > > +/*
> > > > > > > > > + * Copyright (C) 2020 Western Digital Corporation or its
> > > > > > > > > affiliates.
> > > > > > > > > + * Based on arch/arm64/include/asm/efi.h
> > > > > > > > > + */
> > > > > > > > > +#ifndef _ASM_EFI_H
> > > > > > > > > +#define _ASM_EFI_H
> > > > > > > > > +
> > > > > > > > > +#include <asm/io.h>
> > > > > > > > > +#include <asm/mmu_context.h>
> > > > > > > > > +#include <asm/ptrace.h>
> > > > > > > > > +#include <asm/tlbflush.h>
> > > > > > > > > +
> > > > > > > > > +#define VA_BITS_MIN 39
> > > > > > > > > +
> > > > > > > > > +/* on RISC-V, the FDT may be located anywhere in system
> > > > > > > > > RAM */
> > > > > > > > > +static inline unsigned long
> > > > > > > > > efi_get_max_fdt_addr(unsigned long dram_base)
> > > > > > > > > +{
> > > > > > > > > + return ULONG_MAX;
> > > > > > > > > +}
> > > > > > > > > +
> > > > > > > > > +/* Load initrd at enough distance from DRAM start */
> > > > > > > > > +static inline unsigned long
> > > > > > > > > efi_get_max_initrd_addr(unsigned long dram_base,
> > > > > > > > > + unsig
> > > > > > > > > ned long image_addr)
> > > > > > > > > +{
> > > > > > > > > + return dram_base + SZ_256M;
> > > > > > > > > +}
> > > > > > > > > +
> > > > > > > > > +#define efi_bs_call(func, ...) efi_system_table()-
> > > > > > > > > >boottime->func(__VA_ARGS__)
> > > > > > > > > +#define efi_rt_call(func, ...) efi_system_table()-
> > > > > > > > > >runtime->func(__VA_ARGS__)
> > > > > > > > > +#define efi_is_native() (true)
> > > > > > > > > +
> > > > > > > > > +#define efi_table_attr(inst, attr) (inst->attr)
> > > > > > > > > +
> > > > > > > > > +#define efi_call_proto(inst, func, ...) inst->func(inst,
> > > > > > > > > ##__VA_ARGS__)
> > > > > > > > > +
> > > > > > > > > +#define
> > > > > > > > > alloc_screen_info(x...) (&screen_info)
> > > > > > > > > +extern char stext_offset[];
> > > > > > > > > +
> > > > > > > > > +static inline void free_screen_info(struct screen_info
> > > > > > > > > *si)
> > > > > > > > > +{
> > > > > > > > > +}
> > > > > > > > > +
> > > > > > > > > +#endif /* _ASM_EFI_H */
> > > > > > > > > diff --git a/drivers/firmware/efi/Kconfig
> > > > > > > > > b/drivers/firmware/efi/Kconfig
> > > > > > > > > index 2a2b2b96a1dc..fcdc789d3f87 100644
> > > > > > > > > --- a/drivers/firmware/efi/Kconfig
> > > > > > > > > +++ b/drivers/firmware/efi/Kconfig
> > > > > > > > > @@ -111,7 +111,7 @@ config EFI_GENERIC_STUB
> > > > > > > > >
> > > > > > > > > config EFI_ARMSTUB_DTB_LOADER
> > > > > > > > > bool "Enable the DTB loader"
> > > > > > > > > - depends on EFI_GENERIC_STUB
> > > > > > > > > + depends on EFI_GENERIC_STUB && !RISCV
> > > > > > > > > default y
> > > > > > > > > help
> > > > > > > > > Select this config option to add support for
> > > > > > > > > the dtb= command
> > > > > > > > > diff --git a/drivers/firmware/efi/libstub/Makefile
> > > > > > > > > b/drivers/firmware/efi/libstub/Makefile
> > > > > > > > > index 2b4e09bf987c..7d46b70b51f2 100644
> > > > > > > > > --- a/drivers/firmware/efi/libstub/Makefile
> > > > > > > > > +++ b/drivers/firmware/efi/libstub/Makefile
> > > > > > > > > @@ -22,6 +22,8 @@ cflags-
> > > > > > > > > $(CONFIG_ARM64) := $(subst
> > > > > > > > > $(CC_FLAGS_FTRACE),,$(KBUILD_CFLAGS)) \
> > > > > > > > > cflags-$(CONFIG_ARM) := $(subst
> > > > > > > > > $(CC_FLAGS_FTRACE),,$(KBUILD_CFLAGS)) \
> > > > > > > > > -fno-builtin -fpic \
> > > > > > > > > $(call cc-option,-mno-
> > > > > > > > > single-pic-base)
> > > > > > > > > +cflags-$(CONFIG_RISCV) := $(subst
> > > > > > > > > $(CC_FLAGS_FTRACE),,$(KBUILD_CFLAGS)) \
> > > > > > > > > + -fpic
> > > > > > > > >
> > > > > > > > > cflags-$(CONFIG_EFI_GENERIC_STUB) +=
> > > > > > > > > -I$(srctree)/scripts/dtc/libfdt
> > > > > > > > >
> > > > > > > > > @@ -57,6 +59,7 @@ lib-
> > > > > > > > > $(CONFIG_EFI_GENERIC_STUB) += efi-stub.o
> > > > > > > > > fdt.o string.o \
> > > > > > > > > lib-$(CONFIG_ARM) += arm32-stub.o
> > > > > > > > > lib-$(CONFIG_ARM64) += arm64-stub.o
> > > > > > > > > lib-$(CONFIG_X86) += x86-stub.o
> > > > > > > > > +lib-$(CONFIG_RISCV) += riscv-stub.o
> > > > > > > > > CFLAGS_arm32-stub.o :=
> > > > > > > > > -DTEXT_OFFSET=$(TEXT_OFFSET)
> > > > > > > > > CFLAGS_arm64-stub.o :=
> > > > > > > > > -DTEXT_OFFSET=$(TEXT_OFFSET)
> > > > > > > > >
> > > > > > > > > @@ -81,6 +84,10 @@ STUBCOPY_FLAGS-
> > > > > > > > > $(CONFIG_ARM64) += --prefix-alloc-sections=.init \
> > > > > > > > > --prefix-
> > > > > > > > > symbols=__efistub_
> > > > > > > > > STUBCOPY_RELOC-$(CONFIG_ARM64) := R_AARCH64_ABS
> > > > > > > > >
> > > > > > > > > +STUBCOPY_FLAGS-$(CONFIG_RISCV) += --prefix-alloc-
> > > > > > > > > sections=.init \
> > > > > > > > > + --prefix-
> > > > > > > > > symbols=__efistub_
> > > > > > > > > +STUBCOPY_RELOC-$(CONFIG_RISCV) := R_RISCV_HI20
> > > > > > > > > +
> > > > > > > > > $(obj)/%.stub.o: $(obj)/%.o FORCE
> > > > > > > > > $(call if_changed,stubcopy)
> > > > > > > > >
> > > > > > > > > diff --git a/drivers/firmware/efi/libstub/riscv-stub.c
> > > > > > > > > b/drivers/firmware/efi/libstub/riscv-stub.c
> > > > > > > > > new file mode 100644
> > > > > > > > > index 000000000000..69d13e0ebaea
> > > > > > > > > --- /dev/null
> > > > > > > > > +++ b/drivers/firmware/efi/libstub/riscv-stub.c
> > > > > > > > > @@ -0,0 +1,111 @@
> > > > > > > > > +// SPDX-License-Identifier: GPL-2.0
> > > > > > > > > +/*
> > > > > > > > > + * Copyright (C) 2013, 2014 Linaro Ltd; <
> > > > > > > > > roy.franz@linaro.org>
> > > > > > > > > + * Copyright (C) 2020 Western Digital Corporation or its
> > > > > > > > > affiliates.
> > > > > > > > > + *
> > > > > > > > > + * This file implements the EFI boot stub for the RISC-V
> > > > > > > > > kernel.
> > > > > > > > > + * Adapted from ARM64 version at
> > > > > > > > > drivers/firmware/efi/libstub/arm64-stub.c.
> > > > > > > > > + */
> > > > > > > > > +
> > > > > > > > > +#include <linux/efi.h>
> > > > > > > > > +#include <linux/libfdt.h>
> > > > > > > > > +#include <linux/libfdt_env.h>
> > > > > > > > > +#include <asm/efi.h>
> > > > > > > > > +#include <asm/sections.h>
> > > > > > > > > +
> > > > > > > > > +#include "efistub.h"
> > > > > > > > > +/*
> > > > > > > > > + * RISCV requires the kernel image to placed TEXT_OFFSET
> > > > > > > > > bytes beyond a 2 MB
> > > > > > > > > + * aligned base for 64 bit and 4MB for 32 bit.
> > > > > > > > > + */
> > > > > > >
> > > > > > > Fixed the comment.
> > > > > > >
> > > > > > > > > +#ifdef CONFIG_64BIT
> > > > > > > > > +#define MIN_KIMG_ALIGN SZ_2M
> > > > > > > > > +#else
> > > > > > > > > +#define MIN_KIMG_ALIGN SZ_4M
> > > > > > > > > +#endif
> > > > > > > > > +
> > > > > > > > > +typedef __attribute__((noreturn)) void
> > > > > > > > > (*jump_kernel_func)(unsigned int,
> > > > > > > > > +
> > > > > > > > > unsigned long);
> > > > > > > > > +efi_status_t check_platform_features(void)
> > > > > > > > > +{
> > > > > > > > > + return EFI_SUCCESS;
> > > > > > > > > +}
> > > > > > > > > +
> > > > > > > > > +static u32 get_boot_hartid_from_fdt(unsigned long fdt)
> > > > > > > > > +{
> > > > > > > > > + int chosen_node, len;
> > > > > > > > > + const fdt32_t *prop;
> > > > > > > > > +
> > > > > > > > > + chosen_node = fdt_path_offset((void *)fdt,
> > > > > > > > > "/chosen");
> > > > > > > > > + if (chosen_node < 0)
> > > > > > > > > + return U32_MAX;
> > > > > > > > > + prop = fdt_getprop((void *)fdt, chosen_node,
> > > > > > > > > "boot-hartid", &len);
> > > > > > > > > + if (!prop || len != sizeof(u32))
> > > > > > > > > + return U32_MAX;
> > > > > > > > > +
> > > > > > > > > + return fdt32_to_cpu(*prop);
> > > > > > > > > +}
> > > > > > > > > +
> > > > > > > > > +/*
> > > > > > > > > + * Jump to real kernel here with following constraints.
> > > > > > > > > + * 1. MMU should be disabled.
> > > > > > > > > + * 2. a0 should contain hartid
> > > > > > > > > + * 3. a1 should DT address
> > > > > > > > > + */
> > > > > > > > > +void __noreturn efi_enter_kernel(unsigned long
> > > > > > > > > entrypoint, unsigned long fdt,
> > > > > > > > > + unsigned long fdt_size)
> > > > > > > > > +{
> > > > > > > > > + unsigned long kernel_entry = entrypoint +
> > > > > > > > > (unsigned long)stext_offset;
> > > > > > > > > + jump_kernel_func jump_kernel = (jump_kernel_func)
> > > > > > > > > kernel_entry;
> > > > > > > > > + u32 hartid = get_boot_hartid_from_fdt(fdt);
> > > > > > > > > +
> > > > > > > > > + if (hartid == U32_MAX)
> > > > > > > > > + /* We can not use panic or BUG at this
> > > > > > > > > point */
> > > > > > > > > + __asm__ __volatile__ ("ebreak");
> > > > > > > > > + /* Disable MMU */
> > > > > > > > > + csr_write(CSR_SATP, 0);
> > > > > > > > > + jump_kernel(hartid, fdt);
> > > > > > > > > +}
> > > > > > > > > +
> > > > > > > > > +efi_status_t handle_kernel_image(unsigned long
> > > > > > > > > *image_addr,
> > > > > > > > > + unsigned long
> > > > > > > > > *image_size,
> > > > > > > > > + unsigned long
> > > > > > > > > *reserve_addr,
> > > > > > > > > + unsigned long
> > > > > > > > > *reserve_size,
> > > > > > > > > + unsigned long dram_base,
> > > > > > > > > + efi_loaded_image_t
> > > > > > > > > *image)
> > > > > > > > > +{
> > > > > > > > > + efi_status_t status;
> > > > > > > > > + unsigned long kernel_size, kernel_memsize = 0;
> > > > > > > > > + unsigned long max_alloc_address;
> > > > > > > > > +
> > > > > > > > > + if (image->image_base != _start)
> > > > > > > > > + pr_efi_err("FIRMWARE BUG:
> > > > > > > > > efi_loaded_image_t::image_base has bogus value\n");
> > > > > > > > > +
> > > > > > > >
> > > > > > > > I don't think you need this.
> > > > > > > >
> > > > > > >
> > > > > > > Sure. I will remove it. I guess ARM64 code has the error
> > > > > > > print for
> > > > > > > legacy loader code ?
> > > > > > >
> > > > > >
> > > > > > No, for broken distro versions of GRUB
> > > > > >
> > > > > > > > > + kernel_size = _edata - _start;
> > > > > > > > > + kernel_memsize = kernel_size + (_end - _edata);
> > > > > > > > > + max_alloc_address = round_up(dram_base,
> > > > > > > > > MIN_KIMG_ALIGN) +
> > > > > > > > > + kernel_memsize;
> > > > > > > > > +
> > > > > > > >
> > > > > > > > You said the kernel could be anywhere in memory, as long as
> > > > > > > > it is
> > > > > > > > aligned correctly, right?
> > > > > > >
> > > > > > > Sorry I was wrong about this. RISC-V kernel maps PAGE_OFFSET
> > > > > > > virtual
> > > > > > > address to the
> > > > > > > physical address <xyz> where it is booted. That means memory
> > > > > > > between
> > > > > > > dram start and and <xyz> address
> > > > > > > will be unusable.
> > > > > > >
> > > > > >
> > > > > > OK
> > > > > >
> > > > > > > I also realized that the above computing max_address as above
> > > > > > > also
> > > > > > > won't work for the following reason.
> > > > > > > efi_allocate_pages_aligned actually ALIGN_DOWN the
> > > > > > > max_address. Thus,
> > > > > > > efi won't find enough
> > > > > > > free memory in this case if the max_address is computed from
> > > > > > > the dram_base.
> > > > > > >
> > > > > > > Is there an implicit requirement for
> > > > > > > efi_allocate_pages_aligned or
> > > > > > > efi_low_alloc_above should be tried in case of failure?
> > > > > > >
> > > > > >
> > > > > > No not really. What ever works for your particular use case is
> > > > > > acceptable to me.
> > > > > >
> > > > > > > > In that case, you don't need this, you can simply pass
> > > > > > > > ULONG_MAX as
> > > > > > > > the max address.
> > > > > > > >
> > > > > > > As RISC-V should allocate memory as low as possible to avoid
> > > > > > > memory
> > > > > > > wastage, I think the following should work.
> > > > > > >
> > > > > > > efi_low_alloc_above(*reserve_size, MIN_KIMG_ALIGN,
> > > > > > > reserve_addr, dram_base);
> > > > > > >
> > > > > > > If this looks okay to you, efi_low_alloc_above should be
> > > > > > > moved back to
> > > > > > > mem.c from relocate.c.
> > > > > > > Should I do it in a separate patch or the original patch
> > > > > > > should be
> > > > > > > modified so that efi_low_alloc_above was never moved to
> > > > > > > relocate.c
> > > > > > >
> > > > > >
> > > > > > No, please keep efi_low_alloc_above() where it is, but drop the
> > > > > > static, and put back the declaration in efistub.h
> > > > > >
> > > > >
> > > > > Alternatively, can you check whether efi_relocate_kernel()
> > > > > already
> > > > > does what you need?
> > > >
> > > > Yeah. efi_relocate_kernel works too. It's just that
> > > > efi_relocate_kernel expects a preferred address
> > > > which RISC-V doesn't care about.
> > >
> > > There is a preferred address, no? The base of DRAM?
> > >
> >
> > I just realized that my reply from my gmail never landed in any of the
> > mailing lists. If you have already received it, sorry for the spam.
> >
> > "If the preferred address is set as the base of DRAM, efi_bs_call is
> > bound to fail as well because the base of DRAM is reserved by the
> > firmware. So the efi memory allocator can't allocate at that address.
> > Technically, it will work but it is no different than passing
> > ULONG_MAX. So I thought ULONG_MAX will avoid the confusion.
> >
> > We try to allocate as low as possible so I am passing dram_base as the
> > minimum address anyways. As the firmware reserved the first few KBs,
> >
>
>
> OK, so the preferred address *is* the base of DRAM (assuming it is 2
> MB aligned). However, in the general case, you keep some firmware
> state there (couple of KB) and so you typically end up at DRAM base
> plus 2 MB?
>
Yes.
> So first question: why does the firmware put this stuff at the base of
> DRAM in the first place? Does it *have* to live there?
>
The firmware includes the RISC-V specific supervisor binary interface (SBI)[[1]
implementation. As it is a RISC-V specific run time service provider,
it has to be resident in memory.
The arm equivalent of SBI would be PSCI.
[1] https://github.com/riscv/riscv-sbi-doc/blob/master/riscv-sbi.adoc
> Then, if the base of DRAM is guaranteed to be occupied, why not make
> the preferred address base of DRAM + 2 MB ? (or 4 MB for the 32-bit
> case)
I guess that will work too. I was inclined to use low_alloc_above so
that we ensure that the kernel
boots from the lowest possible address given the alignment
restriction. If the alignment restrictions are relaxed,
in future, we just have to change the macro.
However, I think calling relocate_kernel with a preferred offset
(dram_base + KIMG_ALIGN) is
better because it will always fall back to low_alloc_above anyways. I
will send the patch.
--
Regards,
Atish
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [v3 PATCH 5/5] RISC-V: Add EFI stub support.
2020-04-20 7:59 ` Atish Patra
@ 2020-04-20 8:02 ` Ard Biesheuvel
2020-04-21 5:06 ` Atish Patra
0 siblings, 1 reply; 20+ messages in thread
From: Ard Biesheuvel @ 2020-04-20 8:02 UTC (permalink / raw)
To: Atish Patra
Cc: Atish Patra, masahiroy, linux-riscv, linux-efi, palmer,
catalin.marinas, linux-kernel, arnd, will
On Mon, 20 Apr 2020 at 09:59, Atish Patra <atishp@atishpatra.org> wrote:
>
> On Mon, Apr 20, 2020 at 12:04 AM Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > On Mon, 20 Apr 2020 at 06:20, Atish Patra <Atish.Patra@wdc.com> wrote:
> > >
...
> > >
> > > "If the preferred address is set as the base of DRAM, efi_bs_call is
> > > bound to fail as well because the base of DRAM is reserved by the
> > > firmware. So the efi memory allocator can't allocate at that address.
> > > Technically, it will work but it is no different than passing
> > > ULONG_MAX. So I thought ULONG_MAX will avoid the confusion.
> > >
> > > We try to allocate as low as possible so I am passing dram_base as the
> > > minimum address anyways. As the firmware reserved the first few KBs,
> > >
> >
> >
> > OK, so the preferred address *is* the base of DRAM (assuming it is 2
> > MB aligned). However, in the general case, you keep some firmware
> > state there (couple of KB) and so you typically end up at DRAM base
> > plus 2 MB?
> >
>
> Yes.
>
> > So first question: why does the firmware put this stuff at the base of
> > DRAM in the first place? Does it *have* to live there?
> >
>
> The firmware includes the RISC-V specific supervisor binary interface (SBI)[[1]
> implementation. As it is a RISC-V specific run time service provider,
> it has to be resident in memory.
> The arm equivalent of SBI would be PSCI.
>
> [1] https://github.com/riscv/riscv-sbi-doc/blob/master/riscv-sbi.adoc
>
I am not asking why it has to be resident in memory. I am asking why
it has to be at the *base* of memory.
> > Then, if the base of DRAM is guaranteed to be occupied, why not make
> > the preferred address base of DRAM + 2 MB ? (or 4 MB for the 32-bit
> > case)
>
> I guess that will work too. I was inclined to use low_alloc_above so
> that we ensure that the kernel
> boots from the lowest possible address given the alignment
> restriction. If the alignment restrictions are relaxed,
> in future, we just have to change the macro.
>
> However, I think calling relocate_kernel with a preferred offset
> (dram_base + KIMG_ALIGN) is
> better because it will always fall back to low_alloc_above anyways. I
> will send the patch.
Indeed. In the common case, it will just do the allocate_pages()
directly, which is preferred. It will fall back to
efi_low_alloc_aboce() [and do the memory map parsing and traversal
etc] only if needed.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [v3 PATCH 5/5] RISC-V: Add EFI stub support.
2020-04-20 8:02 ` Ard Biesheuvel
@ 2020-04-21 5:06 ` Atish Patra
0 siblings, 0 replies; 20+ messages in thread
From: Atish Patra @ 2020-04-21 5:06 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Atish Patra, linux-riscv, linux-efi, palmer, linux-kernel, arnd,
Anup Patel
On Mon, Apr 20, 2020 at 1:02 AM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Mon, 20 Apr 2020 at 09:59, Atish Patra <atishp@atishpatra.org> wrote:
> >
> > On Mon, Apr 20, 2020 at 12:04 AM Ard Biesheuvel <ardb@kernel.org> wrote:
> > >
> > > On Mon, 20 Apr 2020 at 06:20, Atish Patra <Atish.Patra@wdc.com> wrote:
> > > >
> ...
> > > >
> > > > "If the preferred address is set as the base of DRAM, efi_bs_call is
> > > > bound to fail as well because the base of DRAM is reserved by the
> > > > firmware. So the efi memory allocator can't allocate at that address.
> > > > Technically, it will work but it is no different than passing
> > > > ULONG_MAX. So I thought ULONG_MAX will avoid the confusion.
> > > >
> > > > We try to allocate as low as possible so I am passing dram_base as the
> > > > minimum address anyways. As the firmware reserved the first few KBs,
> > > >
> > >
> > >
> > > OK, so the preferred address *is* the base of DRAM (assuming it is 2
> > > MB aligned). However, in the general case, you keep some firmware
> > > state there (couple of KB) and so you typically end up at DRAM base
> > > plus 2 MB?
> > >
> >
> > Yes.
> >
> > > So first question: why does the firmware put this stuff at the base of
> > > DRAM in the first place? Does it *have* to live there?
> > >
> >
> > The firmware includes the RISC-V specific supervisor binary interface (SBI)[[1]
> > implementation. As it is a RISC-V specific run time service provider,
> > it has to be resident in memory.
> > The arm equivalent of SBI would be PSCI.
> >
> > [1] https://github.com/riscv/riscv-sbi-doc/blob/master/riscv-sbi.adoc
> >
>
> I am not asking why it has to be resident in memory. I am asking why
> it has to be at the *base* of memory.
>
Sorry. I misunderstood the question. I think it is kept at the start
of dram to accommodate
embedded systems with smaller memory. It was also easier to manage at
the beginning of
the memory because all other next stages in the boot process just
ignores first few KBs of
the memory. We also did not have a memory reservation scheme back then.
Having said that, this parameter is configurable for each platform in
OpenSBI. In future, this restriction
can be relaxed if a platform vendor wants. IIRC, linux kernel can't
use the memory below the kernel start
address (where PAGE_OFFSET is mapped) because of generic mm code limitations.
Added Anup (In case he wants to add something to this)
> > > Then, if the base of DRAM is guaranteed to be occupied, why not make
> > > the preferred address base of DRAM + 2 MB ? (or 4 MB for the 32-bit
> > > case)
> >
> > I guess that will work too. I was inclined to use low_alloc_above so
> > that we ensure that the kernel
> > boots from the lowest possible address given the alignment
> > restriction. If the alignment restrictions are relaxed,
> > in future, we just have to change the macro.
> >
> > However, I think calling relocate_kernel with a preferred offset
> > (dram_base + KIMG_ALIGN) is
> > better because it will always fall back to low_alloc_above anyways. I
> > will send the patch.
>
> Indeed. In the common case, it will just do the allocate_pages()
> directly, which is preferred. It will fall back to
> efi_low_alloc_aboce() [and do the memory map parsing and traversal
> etc] only if needed.
--
Regards,
Atish
^ permalink raw reply [flat|nested] 20+ messages in thread
* [tip: efi/core] include: pe.h: Add RISC-V related PE definition
2020-04-15 19:54 ` [v3 PATCH 2/5] include: pe.h: Add RISC-V related PE definition Atish Patra
@ 2020-09-18 8:30 ` tip-bot2 for Atish Patra
0 siblings, 0 replies; 20+ messages in thread
From: tip-bot2 for Atish Patra @ 2020-09-18 8:30 UTC (permalink / raw)
To: linux-tip-commits; +Cc: Atish Patra, Palmer Dabbelt, Ard Biesheuvel, x86, LKML
The following commit has been merged into the efi/core branch of tip:
Commit-ID: 6d0fd536183034953bf84826fecb37e47779d24b
Gitweb: https://git.kernel.org/tip/6d0fd536183034953bf84826fecb37e47779d24b
Author: Atish Patra <atish.patra@wdc.com>
AuthorDate: Fri, 28 Aug 2020 10:20:31 -07:00
Committer: Ard Biesheuvel <ardb@kernel.org>
CommitterDate: Fri, 11 Sep 2020 09:30:01 +03:00
include: pe.h: Add RISC-V related PE definition
Define RISC-V related machine types.
Signed-off-by: Atish Patra <atish.patra@wdc.com>
Reviewed-by: Palmer Dabbelt <palmerdabbelt@google.com>
Link: https://lore.kernel.org/r/20200415195422.19866-3-atish.patra@wdc.com
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
include/linux/pe.h | 3 +++
1 file changed, 3 insertions(+)
diff --git a/include/linux/pe.h b/include/linux/pe.h
index 8ad71d7..daf09ff 100644
--- a/include/linux/pe.h
+++ b/include/linux/pe.h
@@ -55,6 +55,9 @@
#define IMAGE_FILE_MACHINE_POWERPC 0x01f0
#define IMAGE_FILE_MACHINE_POWERPCFP 0x01f1
#define IMAGE_FILE_MACHINE_R4000 0x0166
+#define IMAGE_FILE_MACHINE_RISCV32 0x5032
+#define IMAGE_FILE_MACHINE_RISCV64 0x5064
+#define IMAGE_FILE_MACHINE_RISCV128 0x5128
#define IMAGE_FILE_MACHINE_SH3 0x01a2
#define IMAGE_FILE_MACHINE_SH3DSP 0x01a3
#define IMAGE_FILE_MACHINE_SH3E 0x01a4
^ permalink raw reply related [flat|nested] 20+ messages in thread
end of thread, other threads:[~2020-09-18 8:31 UTC | newest]
Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-15 19:54 [v3 PATCH 0/5] Add UEFI support for RISC-V Atish Patra
2020-04-15 19:54 ` [v3 PATCH 1/5] efi: Move arm-stub to a common file Atish Patra
2020-04-15 19:54 ` [v3 PATCH 2/5] include: pe.h: Add RISC-V related PE definition Atish Patra
2020-09-18 8:30 ` [tip: efi/core] " tip-bot2 for Atish Patra
2020-04-15 19:54 ` [v3 PATCH 3/5] RISC-V: Define fixmap bindings for generic early ioremap support Atish Patra
2020-04-15 19:54 ` [v3 PATCH 4/5] RISC-V: Add PE/COFF header for EFI stub Atish Patra
2020-04-15 19:54 ` [v3 PATCH 5/5] RISC-V: Add EFI stub support Atish Patra
2020-04-16 1:13 ` kbuild test robot
2020-04-16 7:41 ` Ard Biesheuvel
2020-04-18 3:03 ` Atish Patra
2020-04-18 10:51 ` Ard Biesheuvel
2020-04-18 12:39 ` Ard Biesheuvel
2020-04-18 19:19 ` Atish Patra
2020-04-18 19:24 ` Ard Biesheuvel
2020-04-20 4:20 ` Atish Patra
2020-04-20 7:03 ` Ard Biesheuvel
2020-04-20 7:59 ` Atish Patra
2020-04-20 8:02 ` Ard Biesheuvel
2020-04-21 5:06 ` Atish Patra
2020-04-16 7:44 ` [v3 PATCH 0/5] Add UEFI support for RISC-V Ard Biesheuvel
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).