From: Guo Ren <[email protected]>
The firmware of riscv (such as opensbi) occupy 2MB(64bit) /
4MB(32bit) in Linux. It's very wasteful to small memory footprint
soc chip such as Allwinner D1s/F133. The kernel parameter gives a
chance to users to set the proper size of the firmware and get
more than 1.5MB of memory.
Guo Ren (3):
riscv: Remove 2MB offset in the mm layout
riscv: Add early_param to decrease firmware region
riscv: Add riscv.fwsz kernel parameter
.../admin-guide/kernel-parameters.txt | 3 +++
arch/riscv/include/asm/page.h | 8 +++++++
arch/riscv/kernel/head.S | 10 +++-----
arch/riscv/kernel/vmlinux.lds.S | 5 ++--
arch/riscv/mm/init.c | 23 ++++++++++++++++---
5 files changed, 36 insertions(+), 13 deletions(-)
--
2.25.1
From: Guo Ren <[email protected]>
The current RISC-V's mm layout is based on a 2MB offset and wasting
memory. Remove 2MB offset and map PAGE_OFFSET at start_of_DRAM.
Then we could reduce the memory reserved for opensbi in the next
patch.
Signed-off-by: Guo Ren <[email protected]>
Cc: Palmer Dabbelt <[email protected]>
Cc: Anup Patel <[email protected]>
Cc: Atish Patra <[email protected]>
---
arch/riscv/include/asm/page.h | 8 ++++++++
arch/riscv/kernel/head.S | 10 +++-------
arch/riscv/kernel/vmlinux.lds.S | 5 ++---
arch/riscv/mm/init.c | 11 ++++++++---
4 files changed, 21 insertions(+), 13 deletions(-)
diff --git a/arch/riscv/include/asm/page.h b/arch/riscv/include/asm/page.h
index b3e5ff0125fe..299147c78b4a 100644
--- a/arch/riscv/include/asm/page.h
+++ b/arch/riscv/include/asm/page.h
@@ -16,6 +16,14 @@
#define PAGE_SIZE (_AC(1, UL) << PAGE_SHIFT)
#define PAGE_MASK (~(PAGE_SIZE - 1))
+#if __riscv_xlen == 64
+/* Image load offset(2MB) from start of RAM */
+#define LOAD_OFFSET 0x200000
+#else
+/* Image load offset(4MB) from start of RAM */
+#define LOAD_OFFSET 0x400000
+#endif
+
#ifdef CONFIG_64BIT
#define HUGE_MAX_HSTATE 2
#else
diff --git a/arch/riscv/kernel/head.S b/arch/riscv/kernel/head.S
index f52f01ecbeea..a6ac892d2ccf 100644
--- a/arch/riscv/kernel/head.S
+++ b/arch/riscv/kernel/head.S
@@ -61,13 +61,7 @@ ENTRY(_start)
/* Image load offset (0MB) from start of RAM for M-mode */
.dword 0
#else
-#if __riscv_xlen == 64
- /* Image load offset(2MB) from start of RAM */
- .dword 0x200000
-#else
- /* Image load offset(4MB) from start of RAM */
- .dword 0x400000
-#endif
+ .dword LOAD_OFFSET
#endif
/* Effective size of kernel image */
.dword _end - _start
@@ -94,6 +88,8 @@ relocate:
la a1, kernel_map
XIP_FIXUP_OFFSET a1
REG_L a1, KERNEL_MAP_VIRT_ADDR(a1)
+ li a2, LOAD_OFFSET
+ add a1, a1, a2
la a2, _start
sub a1, a1, a2
add ra, ra, a1
diff --git a/arch/riscv/kernel/vmlinux.lds.S b/arch/riscv/kernel/vmlinux.lds.S
index 5104f3a871e3..75b7c72cd4bd 100644
--- a/arch/riscv/kernel/vmlinux.lds.S
+++ b/arch/riscv/kernel/vmlinux.lds.S
@@ -11,10 +11,9 @@
#else
#include <asm/pgtable.h>
-#define LOAD_OFFSET KERNEL_LINK_ADDR
-#include <asm/vmlinux.lds.h>
#include <asm/page.h>
+#include <asm/vmlinux.lds.h>
#include <asm/cache.h>
#include <asm/thread_info.h>
#include <asm/set_memory.h>
@@ -32,7 +31,7 @@ PECOFF_FILE_ALIGNMENT = 0x200;
SECTIONS
{
/* Beginning of code and text segment */
- . = LOAD_OFFSET;
+ . = LOAD_OFFSET + KERNEL_LINK_ADDR;
_start = .;
HEAD_TEXT_SECTION
. = ALIGN(PAGE_SIZE);
diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
index 24b2b8044602..920e78f8c3e4 100644
--- a/arch/riscv/mm/init.c
+++ b/arch/riscv/mm/init.c
@@ -221,6 +221,11 @@ static void __init setup_bootmem(void)
if (!IS_ENABLED(CONFIG_BUILTIN_DTB))
memblock_reserve(dtb_early_pa, fdt_totalsize(dtb_early_va));
+ /*
+ * Reserve OpenSBI region and depends on PMP to deny accesses.
+ */
+ memblock_reserve(__pa(PAGE_OFFSET), LOAD_OFFSET);
+
early_init_fdt_scan_reserved_mem();
dma_contiguous_reserve(dma32_phys_limit);
if (IS_ENABLED(CONFIG_64BIT))
@@ -604,7 +609,7 @@ asmlinkage void __init setup_vm(uintptr_t dtb_pa)
kernel_map.va_kernel_xip_pa_offset = kernel_map.virt_addr - kernel_map.xiprom;
#else
- kernel_map.phys_addr = (uintptr_t)(&_start);
+ kernel_map.phys_addr = (uintptr_t)(&_start) - LOAD_OFFSET;
kernel_map.size = (uintptr_t)(&_end) - kernel_map.phys_addr;
#endif
kernel_map.va_pa_offset = PAGE_OFFSET - kernel_map.phys_addr;
@@ -645,8 +650,8 @@ asmlinkage void __init setup_vm(uintptr_t dtb_pa)
create_pmd_mapping(trampoline_pmd, kernel_map.virt_addr,
kernel_map.xiprom, PMD_SIZE, PAGE_KERNEL_EXEC);
#else
- create_pmd_mapping(trampoline_pmd, kernel_map.virt_addr,
- kernel_map.phys_addr, PMD_SIZE, PAGE_KERNEL_EXEC);
+ create_pmd_mapping(trampoline_pmd, kernel_map.virt_addr + LOAD_OFFSET,
+ kernel_map.phys_addr + LOAD_OFFSET, PMD_SIZE, PAGE_KERNEL_EXEC);
#endif
#else
/* Setup trampoline PGD */
--
2.25.1
From: Guo Ren <[email protected]>
Using riscv.fw_size in cmdline to tell the kernel what the
firmware (opensbi) size is. Then reserve the proper size of
firmware to save memory instead of the whole 2MB. It's helpful
to satisfy a small memory system (D1s/F133 from Allwinner).
Signed-off-by: Guo Ren <[email protected]>
Cc: Palmer Dabbelt <[email protected]>
Cc: Anup Patel <[email protected]>
Cc: Atish Patra <[email protected]>
---
arch/riscv/mm/init.c | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
index 920e78f8c3e4..f7db6d40213d 100644
--- a/arch/riscv/mm/init.c
+++ b/arch/riscv/mm/init.c
@@ -159,6 +159,15 @@ static int __init early_mem(char *p)
}
early_param("mem", early_mem);
+static phys_addr_t firmware_size __initdata;
+static int __init early_get_firmware_size(char *arg)
+{
+ firmware_size = memparse(arg, &arg);
+
+ return 0;
+}
+early_param("riscv.fwsz", early_get_firmware_size);
+
static void __init setup_bootmem(void)
{
phys_addr_t vmlinux_end = __pa_symbol(&_end);
@@ -224,7 +233,10 @@ static void __init setup_bootmem(void)
/*
* Reserve OpenSBI region and depends on PMP to deny accesses.
*/
- memblock_reserve(__pa(PAGE_OFFSET), LOAD_OFFSET);
+ if (firmware_size > PAGE_SIZE)
+ memblock_reserve(__pa(PAGE_OFFSET), firmware_size);
+ else
+ memblock_reserve(__pa(PAGE_OFFSET), LOAD_OFFSET);
early_init_fdt_scan_reserved_mem();
dma_contiguous_reserve(dma32_phys_limit);
--
2.25.1
From: Guo Ren <[email protected]>
The firmware of riscv (such as opensbi) occupy 2MB(64bit) /
4MB(32bit) in Linux. It's very wasteful to small memory footprint
soc chip such as Allwinner D1s/F133. The kernel parameter gives a
chance to users to set the proper size of the firmware and get
more than 1.5MB of memory.
Signed-off-by: Guo Ren <[email protected]>
Cc: Palmer Dabbelt <[email protected]>
Cc: Anup Patel <[email protected]>
Cc: Atish Patra <[email protected]>
---
Documentation/admin-guide/kernel-parameters.txt | 3 +++
1 file changed, 3 insertions(+)
diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 9725c546a0d4..ee505743c8f4 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -4964,6 +4964,9 @@
[KNL] Disable ring 3 MONITOR/MWAIT feature on supported
CPUs.
+ riscv.fwsz=nn[KMG]
+ [RISC-V] Determine firmware size to save memory
+
ro [KNL] Mount root device read-only on boot
rodata= [KNL]
--
2.25.1
On 11/22/21 5:57 PM, [email protected] wrote:
> From: Guo Ren <[email protected]>
>
> The firmware of riscv (such as opensbi) occupy 2MB(64bit) /
> 4MB(32bit) in Linux. It's very wasteful to small memory footprint
> soc chip such as Allwinner D1s/F133. The kernel parameter gives a
> chance to users to set the proper size of the firmware and get
> more than 1.5MB of memory.
>
> Signed-off-by: Guo Ren <[email protected]>
> Cc: Palmer Dabbelt <[email protected]>
> Cc: Anup Patel <[email protected]>
> Cc: Atish Patra <[email protected]>
> ---
> Documentation/admin-guide/kernel-parameters.txt | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 9725c546a0d4..ee505743c8f4 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -4964,6 +4964,9 @@
> [KNL] Disable ring 3 MONITOR/MWAIT feature on supported
> CPUs.
>
> + riscv.fwsz=nn[KMG]
> + [RISC-V] Determine firmware size to save memory
Is "Determine" like "Set"? The user is setting (telling the software)
the firmware size?
"Determine" makes it sound to me like the Linux software is somehow
helping to determine the firmware size.
> +
> ro [KNL] Mount root device read-only on boot
>
> rodata= [KNL]
>
--
~Randy
On Tue, Nov 23, 2021 at 10:34 AM Randy Dunlap <[email protected]> wrote:
>
> On 11/22/21 5:57 PM, [email protected] wrote:
> > From: Guo Ren <[email protected]>
> >
> > The firmware of riscv (such as opensbi) occupy 2MB(64bit) /
> > 4MB(32bit) in Linux. It's very wasteful to small memory footprint
> > soc chip such as Allwinner D1s/F133. The kernel parameter gives a
> > chance to users to set the proper size of the firmware and get
> > more than 1.5MB of memory.
> >
> > Signed-off-by: Guo Ren <[email protected]>
> > Cc: Palmer Dabbelt <[email protected]>
> > Cc: Anup Patel <[email protected]>
> > Cc: Atish Patra <[email protected]>
> > ---
> > Documentation/admin-guide/kernel-parameters.txt | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> > index 9725c546a0d4..ee505743c8f4 100644
> > --- a/Documentation/admin-guide/kernel-parameters.txt
> > +++ b/Documentation/admin-guide/kernel-parameters.txt
> > @@ -4964,6 +4964,9 @@
> > [KNL] Disable ring 3 MONITOR/MWAIT feature on supported
> > CPUs.
> >
> > + riscv.fwsz=nn[KMG]
> > + [RISC-V] Determine firmware size to save memory
>
> Is "Determine" like "Set"? The user is setting (telling the software)
> the firmware size?
I mean "Set" here, thx for pointing it out.
>
> "Determine" makes it sound to me like the Linux software is somehow
> helping to determine the firmware size.
>
> > +
> > ro [KNL] Mount root device read-only on boot
> >
> > rodata= [KNL]
> >
>
>
> --
> ~Randy
--
Best Regards
Guo Ren
ML: https://lore.kernel.org/linux-csky/
+Alex
On Tue, Nov 23, 2021 at 7:27 AM <[email protected]> wrote:
>
> From: Guo Ren <[email protected]>
>
> Using riscv.fw_size in cmdline to tell the kernel what the
> firmware (opensbi) size is. Then reserve the proper size of
> firmware to save memory instead of the whole 2MB. It's helpful
> to satisfy a small memory system (D1s/F133 from Allwinner).
>
> Signed-off-by: Guo Ren <[email protected]>
> Cc: Palmer Dabbelt <[email protected]>
> Cc: Anup Patel <[email protected]>
> Cc: Atish Patra <[email protected]>
> ---
> arch/riscv/mm/init.c | 14 +++++++++++++-
> 1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
> index 920e78f8c3e4..f7db6d40213d 100644
> --- a/arch/riscv/mm/init.c
> +++ b/arch/riscv/mm/init.c
> @@ -159,6 +159,15 @@ static int __init early_mem(char *p)
> }
> early_param("mem", early_mem);
>
> +static phys_addr_t firmware_size __initdata;
> +static int __init early_get_firmware_size(char *arg)
> +{
> + firmware_size = memparse(arg, &arg);
> +
> + return 0;
> +}
> +early_param("riscv.fwsz", early_get_firmware_size);
> +
We have avoided any RISC-V specific kernel parameter till now
and I don't think adding "riscv.fwsz" is the right approach.
OpenSBI adds a reserved memory node (mmode_resv@8000000)
to mark the memory where it is running as reserved. In fact, all
M-mode runtime firmware should be adding a reserved memory
node just like OpenSBI.
Regards,
Anup
> static void __init setup_bootmem(void)
> {
> phys_addr_t vmlinux_end = __pa_symbol(&_end);
> @@ -224,7 +233,10 @@ static void __init setup_bootmem(void)
> /*
> * Reserve OpenSBI region and depends on PMP to deny accesses.
> */
> - memblock_reserve(__pa(PAGE_OFFSET), LOAD_OFFSET);
> + if (firmware_size > PAGE_SIZE)
> + memblock_reserve(__pa(PAGE_OFFSET), firmware_size);
> + else
> + memblock_reserve(__pa(PAGE_OFFSET), LOAD_OFFSET);
>
> early_init_fdt_scan_reserved_mem();
> dma_contiguous_reserve(dma32_phys_limit);
> --
> 2.25.1
>
On Tue, Nov 23, 2021 at 7:27 AM <[email protected]> wrote:
>
> From: Guo Ren <[email protected]>
>
> The firmware of riscv (such as opensbi) occupy 2MB(64bit) /
> 4MB(32bit) in Linux. It's very wasteful to small memory footprint
> soc chip such as Allwinner D1s/F133. The kernel parameter gives a
> chance to users to set the proper size of the firmware and get
> more than 1.5MB of memory.
This kernel parameter is redundant see my comment on other patch.
regards,
Anup
>
> Signed-off-by: Guo Ren <[email protected]>
> Cc: Palmer Dabbelt <[email protected]>
> Cc: Anup Patel <[email protected]>
> Cc: Atish Patra <[email protected]>
> ---
> Documentation/admin-guide/kernel-parameters.txt | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 9725c546a0d4..ee505743c8f4 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -4964,6 +4964,9 @@
> [KNL] Disable ring 3 MONITOR/MWAIT feature on supported
> CPUs.
>
> + riscv.fwsz=nn[KMG]
> + [RISC-V] Determine firmware size to save memory
> +
> ro [KNL] Mount root device read-only on boot
>
> rodata= [KNL]
> --
> 2.25.1
>
+Alex
On Tue, Nov 23, 2021 at 7:27 AM <[email protected]> wrote:
>
> From: Guo Ren <[email protected]>
>
> The current RISC-V's mm layout is based on a 2MB offset and wasting
> memory. Remove 2MB offset and map PAGE_OFFSET at start_of_DRAM.
> Then we could reduce the memory reserved for opensbi in the next
> patch.
The real problem is that the generic kernel marks memory before
__pa(PAGE_OFFSET) as reserved which is evident from the boot
print "OF: fdt: Ignoring memory range 0x80000000 - 0x80200000".
One simple way to re-claim the first 2MB of memory is by:
1) Not placing OpenSBI firmware at start of RAM and rather
place it towards end/middle or RAM away from kernel and initrd
2) Load kernel at start of the RAM
The point#1 is already supported by OpenSBI firmwares using
position independent compilation. In fact, U-Boot SPL does
not load OpenSBI firmware at the start of RAM.
I would suggest Allwinner D1 to follow U-Boot SPL and have
the booting stage before OpenSBI to load OpenSBI firmware
somewhere else.
Regards,
Anup
>
> Signed-off-by: Guo Ren <[email protected]>
> Cc: Palmer Dabbelt <[email protected]>
> Cc: Anup Patel <[email protected]>
> Cc: Atish Patra <[email protected]>
> ---
> arch/riscv/include/asm/page.h | 8 ++++++++
> arch/riscv/kernel/head.S | 10 +++-------
> arch/riscv/kernel/vmlinux.lds.S | 5 ++---
> arch/riscv/mm/init.c | 11 ++++++++---
> 4 files changed, 21 insertions(+), 13 deletions(-)
>
> diff --git a/arch/riscv/include/asm/page.h b/arch/riscv/include/asm/page.h
> index b3e5ff0125fe..299147c78b4a 100644
> --- a/arch/riscv/include/asm/page.h
> +++ b/arch/riscv/include/asm/page.h
> @@ -16,6 +16,14 @@
> #define PAGE_SIZE (_AC(1, UL) << PAGE_SHIFT)
> #define PAGE_MASK (~(PAGE_SIZE - 1))
>
> +#if __riscv_xlen == 64
> +/* Image load offset(2MB) from start of RAM */
> +#define LOAD_OFFSET 0x200000
> +#else
> +/* Image load offset(4MB) from start of RAM */
> +#define LOAD_OFFSET 0x400000
> +#endif
> +
> #ifdef CONFIG_64BIT
> #define HUGE_MAX_HSTATE 2
> #else
> diff --git a/arch/riscv/kernel/head.S b/arch/riscv/kernel/head.S
> index f52f01ecbeea..a6ac892d2ccf 100644
> --- a/arch/riscv/kernel/head.S
> +++ b/arch/riscv/kernel/head.S
> @@ -61,13 +61,7 @@ ENTRY(_start)
> /* Image load offset (0MB) from start of RAM for M-mode */
> .dword 0
> #else
> -#if __riscv_xlen == 64
> - /* Image load offset(2MB) from start of RAM */
> - .dword 0x200000
> -#else
> - /* Image load offset(4MB) from start of RAM */
> - .dword 0x400000
> -#endif
> + .dword LOAD_OFFSET
> #endif
> /* Effective size of kernel image */
> .dword _end - _start
> @@ -94,6 +88,8 @@ relocate:
> la a1, kernel_map
> XIP_FIXUP_OFFSET a1
> REG_L a1, KERNEL_MAP_VIRT_ADDR(a1)
> + li a2, LOAD_OFFSET
> + add a1, a1, a2
> la a2, _start
> sub a1, a1, a2
> add ra, ra, a1
> diff --git a/arch/riscv/kernel/vmlinux.lds.S b/arch/riscv/kernel/vmlinux.lds.S
> index 5104f3a871e3..75b7c72cd4bd 100644
> --- a/arch/riscv/kernel/vmlinux.lds.S
> +++ b/arch/riscv/kernel/vmlinux.lds.S
> @@ -11,10 +11,9 @@
> #else
>
> #include <asm/pgtable.h>
> -#define LOAD_OFFSET KERNEL_LINK_ADDR
>
> -#include <asm/vmlinux.lds.h>
> #include <asm/page.h>
> +#include <asm/vmlinux.lds.h>
> #include <asm/cache.h>
> #include <asm/thread_info.h>
> #include <asm/set_memory.h>
> @@ -32,7 +31,7 @@ PECOFF_FILE_ALIGNMENT = 0x200;
> SECTIONS
> {
> /* Beginning of code and text segment */
> - . = LOAD_OFFSET;
> + . = LOAD_OFFSET + KERNEL_LINK_ADDR;
> _start = .;
> HEAD_TEXT_SECTION
> . = ALIGN(PAGE_SIZE);
> diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
> index 24b2b8044602..920e78f8c3e4 100644
> --- a/arch/riscv/mm/init.c
> +++ b/arch/riscv/mm/init.c
> @@ -221,6 +221,11 @@ static void __init setup_bootmem(void)
> if (!IS_ENABLED(CONFIG_BUILTIN_DTB))
> memblock_reserve(dtb_early_pa, fdt_totalsize(dtb_early_va));
>
> + /*
> + * Reserve OpenSBI region and depends on PMP to deny accesses.
> + */
> + memblock_reserve(__pa(PAGE_OFFSET), LOAD_OFFSET);
> +
> early_init_fdt_scan_reserved_mem();
> dma_contiguous_reserve(dma32_phys_limit);
> if (IS_ENABLED(CONFIG_64BIT))
> @@ -604,7 +609,7 @@ asmlinkage void __init setup_vm(uintptr_t dtb_pa)
>
> kernel_map.va_kernel_xip_pa_offset = kernel_map.virt_addr - kernel_map.xiprom;
> #else
> - kernel_map.phys_addr = (uintptr_t)(&_start);
> + kernel_map.phys_addr = (uintptr_t)(&_start) - LOAD_OFFSET;
> kernel_map.size = (uintptr_t)(&_end) - kernel_map.phys_addr;
> #endif
> kernel_map.va_pa_offset = PAGE_OFFSET - kernel_map.phys_addr;
> @@ -645,8 +650,8 @@ asmlinkage void __init setup_vm(uintptr_t dtb_pa)
> create_pmd_mapping(trampoline_pmd, kernel_map.virt_addr,
> kernel_map.xiprom, PMD_SIZE, PAGE_KERNEL_EXEC);
> #else
> - create_pmd_mapping(trampoline_pmd, kernel_map.virt_addr,
> - kernel_map.phys_addr, PMD_SIZE, PAGE_KERNEL_EXEC);
> + create_pmd_mapping(trampoline_pmd, kernel_map.virt_addr + LOAD_OFFSET,
> + kernel_map.phys_addr + LOAD_OFFSET, PMD_SIZE, PAGE_KERNEL_EXEC);
> #endif
> #else
> /* Setup trampoline PGD */
> --
> 2.25.1
>
On Tue, Nov 23, 2021 at 11:56 AM Anup Patel <[email protected]> wrote:
>
> +Alex
>
> On Tue, Nov 23, 2021 at 7:27 AM <[email protected]> wrote:
> >
> > From: Guo Ren <[email protected]>
> >
> > The current RISC-V's mm layout is based on a 2MB offset and wasting
> > memory. Remove 2MB offset and map PAGE_OFFSET at start_of_DRAM.
> > Then we could reduce the memory reserved for opensbi in the next
> > patch.
>
> The real problem is that the generic kernel marks memory before
> __pa(PAGE_OFFSET) as reserved which is evident from the boot
> print "OF: fdt: Ignoring memory range 0x80000000 - 0x80200000".
>
> One simple way to re-claim the first 2MB of memory is by:
> 1) Not placing OpenSBI firmware at start of RAM and rather
> place it towards end/middle or RAM away from kernel and initrd
> 2) Load kernel at start of the RAM
>
> The point#1 is already supported by OpenSBI firmwares using
> position independent compilation. In fact, U-Boot SPL does
> not load OpenSBI firmware at the start of RAM.
This deviates from the original intention of this patch. Some users
have been used to 2MB/4MB LOAD_OFFSET and we also should save the
memory of opensbi for them.
>
> I would suggest Allwinner D1 to follow U-Boot SPL and have
> the booting stage before OpenSBI to load OpenSBI firmware
>
> Regards,
> Anup
>
> >
> > Signed-off-by: Guo Ren <[email protected]>
> > Cc: Palmer Dabbelt <[email protected]>
> > Cc: Anup Patel <[email protected]>
> > Cc: Atish Patra <[email protected]>
> > ---
> > arch/riscv/include/asm/page.h | 8 ++++++++
> > arch/riscv/kernel/head.S | 10 +++-------
> > arch/riscv/kernel/vmlinux.lds.S | 5 ++---
> > arch/riscv/mm/init.c | 11 ++++++++---
> > 4 files changed, 21 insertions(+), 13 deletions(-)
> >
> > diff --git a/arch/riscv/include/asm/page.h b/arch/riscv/include/asm/page.h
> > index b3e5ff0125fe..299147c78b4a 100644
> > --- a/arch/riscv/include/asm/page.h
> > +++ b/arch/riscv/include/asm/page.h
> > @@ -16,6 +16,14 @@
> > #define PAGE_SIZE (_AC(1, UL) << PAGE_SHIFT)
> > #define PAGE_MASK (~(PAGE_SIZE - 1))
> >
> > +#if __riscv_xlen == 64
> > +/* Image load offset(2MB) from start of RAM */
> > +#define LOAD_OFFSET 0x200000
> > +#else
> > +/* Image load offset(4MB) from start of RAM */
> > +#define LOAD_OFFSET 0x400000
> > +#endif
> > +
> > #ifdef CONFIG_64BIT
> > #define HUGE_MAX_HSTATE 2
> > #else
> > diff --git a/arch/riscv/kernel/head.S b/arch/riscv/kernel/head.S
> > index f52f01ecbeea..a6ac892d2ccf 100644
> > --- a/arch/riscv/kernel/head.S
> > +++ b/arch/riscv/kernel/head.S
> > @@ -61,13 +61,7 @@ ENTRY(_start)
> > /* Image load offset (0MB) from start of RAM for M-mode */
> > .dword 0
> > #else
> > -#if __riscv_xlen == 64
> > - /* Image load offset(2MB) from start of RAM */
> > - .dword 0x200000
> > -#else
> > - /* Image load offset(4MB) from start of RAM */
> > - .dword 0x400000
> > -#endif
> > + .dword LOAD_OFFSET
> > #endif
> > /* Effective size of kernel image */
> > .dword _end - _start
> > @@ -94,6 +88,8 @@ relocate:
> > la a1, kernel_map
> > XIP_FIXUP_OFFSET a1
> > REG_L a1, KERNEL_MAP_VIRT_ADDR(a1)
> > + li a2, LOAD_OFFSET
> > + add a1, a1, a2
> > la a2, _start
> > sub a1, a1, a2
> > add ra, ra, a1
> > diff --git a/arch/riscv/kernel/vmlinux.lds.S b/arch/riscv/kernel/vmlinux.lds.S
> > index 5104f3a871e3..75b7c72cd4bd 100644
> > --- a/arch/riscv/kernel/vmlinux.lds.S
> > +++ b/arch/riscv/kernel/vmlinux.lds.S
> > @@ -11,10 +11,9 @@
> > #else
> >
> > #include <asm/pgtable.h>
> > -#define LOAD_OFFSET KERNEL_LINK_ADDR
> >
> > -#include <asm/vmlinux.lds.h>
> > #include <asm/page.h>
> > +#include <asm/vmlinux.lds.h>
> > #include <asm/cache.h>
> > #include <asm/thread_info.h>
> > #include <asm/set_memory.h>
> > @@ -32,7 +31,7 @@ PECOFF_FILE_ALIGNMENT = 0x200;
> > SECTIONS
> > {
> > /* Beginning of code and text segment */
> > - . = LOAD_OFFSET;
> > + . = LOAD_OFFSET + KERNEL_LINK_ADDR;
> > _start = .;
> > HEAD_TEXT_SECTION
> > . = ALIGN(PAGE_SIZE);
> > diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
> > index 24b2b8044602..920e78f8c3e4 100644
> > --- a/arch/riscv/mm/init.c
> > +++ b/arch/riscv/mm/init.c
> > @@ -221,6 +221,11 @@ static void __init setup_bootmem(void)
> > if (!IS_ENABLED(CONFIG_BUILTIN_DTB))
> > memblock_reserve(dtb_early_pa, fdt_totalsize(dtb_early_va));
> >
> > + /*
> > + * Reserve OpenSBI region and depends on PMP to deny accesses.
> > + */
> > + memblock_reserve(__pa(PAGE_OFFSET), LOAD_OFFSET);
> > +
> > early_init_fdt_scan_reserved_mem();
> > dma_contiguous_reserve(dma32_phys_limit);
> > if (IS_ENABLED(CONFIG_64BIT))
> > @@ -604,7 +609,7 @@ asmlinkage void __init setup_vm(uintptr_t dtb_pa)
> >
> > kernel_map.va_kernel_xip_pa_offset = kernel_map.virt_addr - kernel_map.xiprom;
> > #else
> > - kernel_map.phys_addr = (uintptr_t)(&_start);
> > + kernel_map.phys_addr = (uintptr_t)(&_start) - LOAD_OFFSET;
> > kernel_map.size = (uintptr_t)(&_end) - kernel_map.phys_addr;
> > #endif
> > kernel_map.va_pa_offset = PAGE_OFFSET - kernel_map.phys_addr;
> > @@ -645,8 +650,8 @@ asmlinkage void __init setup_vm(uintptr_t dtb_pa)
> > create_pmd_mapping(trampoline_pmd, kernel_map.virt_addr,
> > kernel_map.xiprom, PMD_SIZE, PAGE_KERNEL_EXEC);
> > #else
> > - create_pmd_mapping(trampoline_pmd, kernel_map.virt_addr,
> > - kernel_map.phys_addr, PMD_SIZE, PAGE_KERNEL_EXEC);
> > + create_pmd_mapping(trampoline_pmd, kernel_map.virt_addr + LOAD_OFFSET,
> > + kernel_map.phys_addr + LOAD_OFFSET, PMD_SIZE, PAGE_KERNEL_EXEC);
> > #endif
> > #else
> > /* Setup trampoline PGD */
> > --
> > 2.25.1
> >
--
Best Regards
Guo Ren
ML: https://lore.kernel.org/linux-csky/
On 23 Nov 2021, at 03:44, Anup Patel <[email protected]> wrote:
>
> +Alex
>
> On Tue, Nov 23, 2021 at 7:27 AM <[email protected]> wrote:
>>
>> From: Guo Ren <[email protected]>
>>
>> Using riscv.fw_size in cmdline to tell the kernel what the
>> firmware (opensbi) size is. Then reserve the proper size of
>> firmware to save memory instead of the whole 2MB. It's helpful
>> to satisfy a small memory system (D1s/F133 from Allwinner).
>>
>> Signed-off-by: Guo Ren <[email protected]>
>> Cc: Palmer Dabbelt <[email protected]>
>> Cc: Anup Patel <[email protected]>
>> Cc: Atish Patra <[email protected]>
>> ---
>> arch/riscv/mm/init.c | 14 +++++++++++++-
>> 1 file changed, 13 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
>> index 920e78f8c3e4..f7db6d40213d 100644
>> --- a/arch/riscv/mm/init.c
>> +++ b/arch/riscv/mm/init.c
>> @@ -159,6 +159,15 @@ static int __init early_mem(char *p)
>> }
>> early_param("mem", early_mem);
>>
>> +static phys_addr_t firmware_size __initdata;
>> +static int __init early_get_firmware_size(char *arg)
>> +{
>> + firmware_size = memparse(arg, &arg);
>> +
>> + return 0;
>> +}
>> +early_param("riscv.fwsz", early_get_firmware_size);
>> +
>
> We have avoided any RISC-V specific kernel parameter till now
> and I don't think adding "riscv.fwsz" is the right approach.
>
> OpenSBI adds a reserved memory node (mmode_resv@8000000)
> to mark the memory where it is running as reserved. In fact, all
> M-mode runtime firmware should be adding a reserved memory
> node just like OpenSBI.
BBL does not do this and, even if it’s modified today, older versions
will still need to be supported for quite a while longer.
In FreeBSD[1] we only reserve the first 2 MiB of DRAM (we don’t care
about RV32) if there is no reserved memory node covering the DRAM base
address, which avoids this issue. The only downside with that approach
is that if firmware occupies a different region than the beginning of
DRAM (or there is no firmware resident in the supervisor’s physical
address space, as is the case for a virtualised guest) then it
unnecessarily reserves that first 2 MiB, but that’s not a huge deal,
and can’t be avoided so long as BBL continues to exist (well, I guess
you could probe the SBI implementation ID if you really cared about
that, but I’ve yet to hear of a platform where the SBI implementation,
if it exists, isn’t at the start of DRAM, and if you’re virtualising
then you probably have enough DRAM that you don’t notice 2 MiB going
missing).
Jess
[1] https://github.com/freebsd/freebsd-src/blob/main/sys/riscv/riscv/machdep.c#L554-L568
On Tue, Nov 23, 2021 at 4:56 AM Anup Patel <[email protected]> wrote:
>
> +Alex
>
> On Tue, Nov 23, 2021 at 7:27 AM <[email protected]> wrote:
> >
> > From: Guo Ren <[email protected]>
> >
> > The current RISC-V's mm layout is based on a 2MB offset and wasting
> > memory. Remove 2MB offset and map PAGE_OFFSET at start_of_DRAM.
> > Then we could reduce the memory reserved for opensbi in the next
> > patch.
>
> The real problem is that the generic kernel marks memory before
> __pa(PAGE_OFFSET) as reserved which is evident from the boot
> print "OF: fdt: Ignoring memory range 0x80000000 - 0x80200000".
Agreed, memblock 'rejects' this region because MIN_MEMBLOCK_ADDR is
defined as __pa(PAGE_OFFSET) which points to 0x80200000. I have a
patch to enable the use of hugepages for the linear mapping [1] that
does just that, things are not that easy since then it breaks initrd
initialization which is an early caller of __va, I updated this
patchset a few months ago, I can push that soon @Guo Ren.
[1] https://lkml.org/lkml/2020/6/3/696
>
> One simple way to re-claim the first 2MB of memory is by:
> 1) Not placing OpenSBI firmware at start of RAM and rather
> place it towards end/middle or RAM away from kernel and initrd
> 2) Load kernel at start of the RAM
>
> The point#1 is already supported by OpenSBI firmwares using
> position independent compilation. In fact, U-Boot SPL does
> not load OpenSBI firmware at the start of RAM.
>
> I would suggest Allwinner D1 to follow U-Boot SPL and have
> the booting stage before OpenSBI to load OpenSBI firmware
> somewhere else.
>
> Regards,
> Anup
>
> >
> > Signed-off-by: Guo Ren <[email protected]>
> > Cc: Palmer Dabbelt <[email protected]>
> > Cc: Anup Patel <[email protected]>
> > Cc: Atish Patra <[email protected]>
> > ---
> > arch/riscv/include/asm/page.h | 8 ++++++++
> > arch/riscv/kernel/head.S | 10 +++-------
> > arch/riscv/kernel/vmlinux.lds.S | 5 ++---
> > arch/riscv/mm/init.c | 11 ++++++++---
> > 4 files changed, 21 insertions(+), 13 deletions(-)
> >
> > diff --git a/arch/riscv/include/asm/page.h b/arch/riscv/include/asm/page.h
> > index b3e5ff0125fe..299147c78b4a 100644
> > --- a/arch/riscv/include/asm/page.h
> > +++ b/arch/riscv/include/asm/page.h
> > @@ -16,6 +16,14 @@
> > #define PAGE_SIZE (_AC(1, UL) << PAGE_SHIFT)
> > #define PAGE_MASK (~(PAGE_SIZE - 1))
> >
> > +#if __riscv_xlen == 64
> > +/* Image load offset(2MB) from start of RAM */
> > +#define LOAD_OFFSET 0x200000
> > +#else
> > +/* Image load offset(4MB) from start of RAM */
> > +#define LOAD_OFFSET 0x400000
> > +#endif
> > +
> > #ifdef CONFIG_64BIT
> > #define HUGE_MAX_HSTATE 2
> > #else
> > diff --git a/arch/riscv/kernel/head.S b/arch/riscv/kernel/head.S
> > index f52f01ecbeea..a6ac892d2ccf 100644
> > --- a/arch/riscv/kernel/head.S
> > +++ b/arch/riscv/kernel/head.S
> > @@ -61,13 +61,7 @@ ENTRY(_start)
> > /* Image load offset (0MB) from start of RAM for M-mode */
> > .dword 0
> > #else
> > -#if __riscv_xlen == 64
> > - /* Image load offset(2MB) from start of RAM */
> > - .dword 0x200000
> > -#else
> > - /* Image load offset(4MB) from start of RAM */
> > - .dword 0x400000
> > -#endif
> > + .dword LOAD_OFFSET
> > #endif
> > /* Effective size of kernel image */
> > .dword _end - _start
> > @@ -94,6 +88,8 @@ relocate:
> > la a1, kernel_map
> > XIP_FIXUP_OFFSET a1
> > REG_L a1, KERNEL_MAP_VIRT_ADDR(a1)
> > + li a2, LOAD_OFFSET
> > + add a1, a1, a2
> > la a2, _start
> > sub a1, a1, a2
> > add ra, ra, a1
> > diff --git a/arch/riscv/kernel/vmlinux.lds.S b/arch/riscv/kernel/vmlinux.lds.S
> > index 5104f3a871e3..75b7c72cd4bd 100644
> > --- a/arch/riscv/kernel/vmlinux.lds.S
> > +++ b/arch/riscv/kernel/vmlinux.lds.S
> > @@ -11,10 +11,9 @@
> > #else
> >
> > #include <asm/pgtable.h>
> > -#define LOAD_OFFSET KERNEL_LINK_ADDR
> >
> > -#include <asm/vmlinux.lds.h>
> > #include <asm/page.h>
> > +#include <asm/vmlinux.lds.h>
> > #include <asm/cache.h>
> > #include <asm/thread_info.h>
> > #include <asm/set_memory.h>
> > @@ -32,7 +31,7 @@ PECOFF_FILE_ALIGNMENT = 0x200;
> > SECTIONS
> > {
> > /* Beginning of code and text segment */
> > - . = LOAD_OFFSET;
> > + . = LOAD_OFFSET + KERNEL_LINK_ADDR;
> > _start = .;
> > HEAD_TEXT_SECTION
> > . = ALIGN(PAGE_SIZE);
> > diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
> > index 24b2b8044602..920e78f8c3e4 100644
> > --- a/arch/riscv/mm/init.c
> > +++ b/arch/riscv/mm/init.c
> > @@ -221,6 +221,11 @@ static void __init setup_bootmem(void)
> > if (!IS_ENABLED(CONFIG_BUILTIN_DTB))
> > memblock_reserve(dtb_early_pa, fdt_totalsize(dtb_early_va));
> >
> > + /*
> > + * Reserve OpenSBI region and depends on PMP to deny accesses.
> > + */
> > + memblock_reserve(__pa(PAGE_OFFSET), LOAD_OFFSET);
> > +
> > early_init_fdt_scan_reserved_mem();
> > dma_contiguous_reserve(dma32_phys_limit);
> > if (IS_ENABLED(CONFIG_64BIT))
> > @@ -604,7 +609,7 @@ asmlinkage void __init setup_vm(uintptr_t dtb_pa)
> >
> > kernel_map.va_kernel_xip_pa_offset = kernel_map.virt_addr - kernel_map.xiprom;
> > #else
> > - kernel_map.phys_addr = (uintptr_t)(&_start);
> > + kernel_map.phys_addr = (uintptr_t)(&_start) - LOAD_OFFSET;
> > kernel_map.size = (uintptr_t)(&_end) - kernel_map.phys_addr;
> > #endif
> > kernel_map.va_pa_offset = PAGE_OFFSET - kernel_map.phys_addr;
> > @@ -645,8 +650,8 @@ asmlinkage void __init setup_vm(uintptr_t dtb_pa)
> > create_pmd_mapping(trampoline_pmd, kernel_map.virt_addr,
> > kernel_map.xiprom, PMD_SIZE, PAGE_KERNEL_EXEC);
> > #else
> > - create_pmd_mapping(trampoline_pmd, kernel_map.virt_addr,
> > - kernel_map.phys_addr, PMD_SIZE, PAGE_KERNEL_EXEC);
> > + create_pmd_mapping(trampoline_pmd, kernel_map.virt_addr + LOAD_OFFSET,
> > + kernel_map.phys_addr + LOAD_OFFSET, PMD_SIZE, PAGE_KERNEL_EXEC);
> > #endif
> > #else
> > /* Setup trampoline PGD */
> > --
> > 2.25.1
> >
On Tue, Nov 23, 2021 at 12:53 PM Jessica Clarke <[email protected]> wrote:
>
> On 23 Nov 2021, at 03:44, Anup Patel <[email protected]> wrote:
> >
> > +Alex
> >
> > On Tue, Nov 23, 2021 at 7:27 AM <[email protected]> wrote:
> >>
> >> From: Guo Ren <[email protected]>
> >>
> >> Using riscv.fw_size in cmdline to tell the kernel what the
> >> firmware (opensbi) size is. Then reserve the proper size of
> >> firmware to save memory instead of the whole 2MB. It's helpful
> >> to satisfy a small memory system (D1s/F133 from Allwinner).
> >>
> >> Signed-off-by: Guo Ren <[email protected]>
> >> Cc: Palmer Dabbelt <[email protected]>
> >> Cc: Anup Patel <[email protected]>
> >> Cc: Atish Patra <[email protected]>
> >> ---
> >> arch/riscv/mm/init.c | 14 +++++++++++++-
> >> 1 file changed, 13 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
> >> index 920e78f8c3e4..f7db6d40213d 100644
> >> --- a/arch/riscv/mm/init.c
> >> +++ b/arch/riscv/mm/init.c
> >> @@ -159,6 +159,15 @@ static int __init early_mem(char *p)
> >> }
> >> early_param("mem", early_mem);
> >>
> >> +static phys_addr_t firmware_size __initdata;
> >> +static int __init early_get_firmware_size(char *arg)
> >> +{
> >> + firmware_size = memparse(arg, &arg);
> >> +
> >> + return 0;
> >> +}
> >> +early_param("riscv.fwsz", early_get_firmware_size);
> >> +
> >
> > We have avoided any RISC-V specific kernel parameter till now
> > and I don't think adding "riscv.fwsz" is the right approach.
> >
> > OpenSBI adds a reserved memory node (mmode_resv@8000000)
> > to mark the memory where it is running as reserved. In fact, all
> > M-mode runtime firmware should be adding a reserved memory
> > node just like OpenSBI.
Yes I agree that this should be in the device tree, IMO there is no
need to introduce a new kernel parameter.
>
> BBL does not do this and, even if it’s modified today, older versions
> will still need to be supported for quite a while longer.
It's fair to expect the firmware to advertise its existence: we
briefly discussed that last year with Atish [1] and he proposed to
introduce a document that describes what the kernel expects from the
'platform' when it boots, that would be a way to drop those old legacy
bootloaders.
[1] https://lkml.org/lkml/2020/6/3/696
>
> In FreeBSD[1] we only reserve the first 2 MiB of DRAM (we don’t care
> about RV32) if there is no reserved memory node covering the DRAM base
> address, which avoids this issue. The only downside with that approach
> is that if firmware occupies a different region than the beginning of
> DRAM (or there is no firmware resident in the supervisor’s physical
> address space, as is the case for a virtualised guest) then it
> unnecessarily reserves that first 2 MiB, but that’s not a huge deal,
> and can’t be avoided so long as BBL continues to exist (well, I guess
> you could probe the SBI implementation ID if you really cared about
> that, but I’ve yet to hear of a platform where the SBI implementation,
> if it exists, isn’t at the start of DRAM, and if you’re virtualising
> then you probably have enough DRAM that you don’t notice 2 MiB going
> missing).
>
> Jess
>
> [1] https://github.com/freebsd/freebsd-src/blob/main/sys/riscv/riscv/machdep.c#L554-L568
>
Hi Guo,
Am Dienstag, 23. November 2021, 02:57:14 CET schrieb [email protected]:
> From: Guo Ren <[email protected]>
>
> The firmware of riscv (such as opensbi) occupy 2MB(64bit) /
> 4MB(32bit) in Linux. It's very wasteful to small memory footprint
> soc chip such as Allwinner D1s/F133. The kernel parameter gives a
> chance to users to set the proper size of the firmware and get
> more than 1.5MB of memory.
is this kernel parameter approach a result of the T-Head Ice-SoC
currently loading its openSBI from inside the main u-boot via extfs-load,
directly before the kernel itself [0] ?
Because that approach in general looks not ideal.
Normally you want the main u-boot already running with less privileges
so firmware like openSBI should've been already loaded before that.
Even more true when you're employing methods to protect memory regions
from less privileged access.
A lot of socs set u-boot as opensbi payload, but for the example the D1
mainline approach uses the Allwinner TOC1 image format to load both
opensbi and the main uboot into memory from its 1st stage loader.
Of course the best way would be to just mimic what a number of
arm64 and also riscv socs do and use already existing u-boot utilities.
U-Boot can create a FIT image containing both main u-boot, dtb and
firmware images that all get loaded from SPL and placed at the correct
addresses before having the SPL jump into opensbi and from there
into u-boot [1] .
And as Anup was writing, reserved-memory should then be the way
to go to tell the kernel what regions to omit.
And mainline u-boot has already the means to even take the reserved-memory
from the devicetree used by opensbi and copy it to a new devicetree,
if the second one is different.
Heiko
[0] https://github.com/T-head-Semi/u-boot/blob/main/include/configs/ice-c910.h#L46
[1] see spl_invoke_opensbi() in common/spl/spl_opensbi.c
[2] see riscv_board_reserved_mem_fixup() in arch/riscv/lib/fdt_fixup.c
>
> Guo Ren (3):
> riscv: Remove 2MB offset in the mm layout
> riscv: Add early_param to decrease firmware region
> riscv: Add riscv.fwsz kernel parameter
>
> .../admin-guide/kernel-parameters.txt | 3 +++
> arch/riscv/include/asm/page.h | 8 +++++++
> arch/riscv/kernel/head.S | 10 +++-----
> arch/riscv/kernel/vmlinux.lds.S | 5 ++--
> arch/riscv/mm/init.c | 23 ++++++++++++++++---
> 5 files changed, 36 insertions(+), 13 deletions(-)
>
>
On Tue, Nov 23, 2021 at 11:33 AM Heiko Stübner <[email protected]> wrote:
>
> Hi Guo,
>
> Am Dienstag, 23. November 2021, 02:57:14 CET schrieb [email protected]:
> > From: Guo Ren <[email protected]>
> >
> > The firmware of riscv (such as opensbi) occupy 2MB(64bit) /
> > 4MB(32bit) in Linux. It's very wasteful to small memory footprint
> > soc chip such as Allwinner D1s/F133. The kernel parameter gives a
> > chance to users to set the proper size of the firmware and get
> > more than 1.5MB of memory.
>
> is this kernel parameter approach a result of the T-Head Ice-SoC
> currently loading its openSBI from inside the main u-boot via extfs-load,
> directly before the kernel itself [0] ?
Looking at the defconfig[1], it may be U-Boot SPL not U-Boot proper. I
may be looking at the wrong config though.
If U-Boot SPL is actually used, you don't even need to manually load
OpenSBI "fw_jump" binary.
As Heiko pointed, you should just follow how U-Boot SPL works on
hifive unmatched (creating the FIT image)
The standard U-Boot SPL uses with fw_dynamic which provides all the
flexibility you want.
[1] https://github.com/T-head-Semi/u-boot/blob/main/configs/ice_evb_c910_defconfig
>
> Because that approach in general looks not ideal.
>
> Normally you want the main u-boot already running with less privileges
> so firmware like openSBI should've been already loaded before that.
> Even more true when you're employing methods to protect memory regions
> from less privileged access.
>
> A lot of socs set u-boot as opensbi payload, but for the example the D1
> mainline approach uses the Allwinner TOC1 image format to load both
> opensbi and the main uboot into memory from its 1st stage loader.
>
>
> Of course the best way would be to just mimic what a number of
> arm64 and also riscv socs do and use already existing u-boot utilities.
>
> U-Boot can create a FIT image containing both main u-boot, dtb and
> firmware images that all get loaded from SPL and placed at the correct
> addresses before having the SPL jump into opensbi and from there
> into u-boot [1] .
>
> And as Anup was writing, reserved-memory should then be the way
> to go to tell the kernel what regions to omit.
>
> And mainline u-boot has already the means to even take the reserved-memory
> from the devicetree used by opensbi and copy it to a new devicetree,
> if the second one is different.
>
>
> Heiko
>
>
> [0] https://github.com/T-head-Semi/u-boot/blob/main/include/configs/ice-c910.h#L46
> [1] see spl_invoke_opensbi() in common/spl/spl_opensbi.c
> [2] see riscv_board_reserved_mem_fixup() in arch/riscv/lib/fdt_fixup.c
>
> >
> > Guo Ren (3):
> > riscv: Remove 2MB offset in the mm layout
> > riscv: Add early_param to decrease firmware region
> > riscv: Add riscv.fwsz kernel parameter
> >
> > .../admin-guide/kernel-parameters.txt | 3 +++
> > arch/riscv/include/asm/page.h | 8 +++++++
> > arch/riscv/kernel/head.S | 10 +++-----
> > arch/riscv/kernel/vmlinux.lds.S | 5 ++--
> > arch/riscv/mm/init.c | 23 ++++++++++++++++---
> > 5 files changed, 36 insertions(+), 13 deletions(-)
> >
> >
>
>
>
>
>
> _______________________________________________
> linux-riscv mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-riscv
--
Regards,
Atish
Am Dienstag, 23. November 2021, 21:01:29 CET schrieb Atish Patra:
> On Tue, Nov 23, 2021 at 11:33 AM Heiko St?bner <[email protected]> wrote:
> >
> > Hi Guo,
> >
> > Am Dienstag, 23. November 2021, 02:57:14 CET schrieb [email protected]:
> > > From: Guo Ren <[email protected]>
> > >
> > > The firmware of riscv (such as opensbi) occupy 2MB(64bit) /
> > > 4MB(32bit) in Linux. It's very wasteful to small memory footprint
> > > soc chip such as Allwinner D1s/F133. The kernel parameter gives a
> > > chance to users to set the proper size of the firmware and get
> > > more than 1.5MB of memory.
> >
> > is this kernel parameter approach a result of the T-Head Ice-SoC
> > currently loading its openSBI from inside the main u-boot via extfs-load,
> > directly before the kernel itself [0] ?
>
> Looking at the defconfig[1], it may be U-Boot SPL not U-Boot proper. I
> may be looking at the wrong config though.
It is actually uboot-spl + uboot proper (aka the standard spl loads u-boot
way) which is the reason I pointed to using the existing u-boot facilities :-)
> If U-Boot SPL is actually used, you don't even need to manually load
> OpenSBI "fw_jump" binary.
>
> As Heiko pointed, you should just follow how U-Boot SPL works on
> hifive unmatched (creating the FIT image)
> The standard U-Boot SPL uses with fw_dynamic which provides all the
> flexibility you want.
>
> [1] https://github.com/T-head-Semi/u-boot/blob/main/configs/ice_evb_c910_defconfig
> >
> > Because that approach in general looks not ideal.
> >
> > Normally you want the main u-boot already running with less privileges
> > so firmware like openSBI should've been already loaded before that.
> > Even more true when you're employing methods to protect memory regions
> > from less privileged access.
> >
> > A lot of socs set u-boot as opensbi payload, but for the example the D1
> > mainline approach uses the Allwinner TOC1 image format to load both
> > opensbi and the main uboot into memory from its 1st stage loader.
> >
> >
> > Of course the best way would be to just mimic what a number of
> > arm64 and also riscv socs do and use already existing u-boot utilities.
> >
> > U-Boot can create a FIT image containing both main u-boot, dtb and
> > firmware images that all get loaded from SPL and placed at the correct
> > addresses before having the SPL jump into opensbi and from there
> > into u-boot [1] .
> >
> > And as Anup was writing, reserved-memory should then be the way
> > to go to tell the kernel what regions to omit.
> >
> > And mainline u-boot has already the means to even take the reserved-memory
> > from the devicetree used by opensbi and copy it to a new devicetree,
> > if the second one is different.
> >
> >
> > Heiko
> >
> >
> > [0] https://github.com/T-head-Semi/u-boot/blob/main/include/configs/ice-c910.h#L46
> > [1] see spl_invoke_opensbi() in common/spl/spl_opensbi.c
> > [2] see riscv_board_reserved_mem_fixup() in arch/riscv/lib/fdt_fixup.c
> >
> > >
> > > Guo Ren (3):
> > > riscv: Remove 2MB offset in the mm layout
> > > riscv: Add early_param to decrease firmware region
> > > riscv: Add riscv.fwsz kernel parameter
> > >
> > > .../admin-guide/kernel-parameters.txt | 3 +++
> > > arch/riscv/include/asm/page.h | 8 +++++++
> > > arch/riscv/kernel/head.S | 10 +++-----
> > > arch/riscv/kernel/vmlinux.lds.S | 5 ++--
> > > arch/riscv/mm/init.c | 23 ++++++++++++++++---
> > > 5 files changed, 36 insertions(+), 13 deletions(-)
> > >
> > >
> >
> >
> >
> >
> >
> > _______________________________________________
> > linux-riscv mailing list
> > [email protected]
> > http://lists.infradead.org/mailman/listinfo/linux-riscv
>
>
>
>
On Wed, Nov 24, 2021 at 3:33 AM Heiko Stübner <[email protected]> wrote:
>
> Hi Guo,
>
> Am Dienstag, 23. November 2021, 02:57:14 CET schrieb [email protected]:
> > From: Guo Ren <[email protected]>
> >
> > The firmware of riscv (such as opensbi) occupy 2MB(64bit) /
> > 4MB(32bit) in Linux. It's very wasteful to small memory footprint
> > soc chip such as Allwinner D1s/F133. The kernel parameter gives a
> > chance to users to set the proper size of the firmware and get
> > more than 1.5MB of memory.
>
> is this kernel parameter approach a result of the T-Head Ice-SoC
> currently loading its openSBI from inside the main u-boot via extfs-load,
> directly before the kernel itself [0] ?
The patch is not related to that issue. The patch just helps users who
put opensbi at 0~2MB paddr to save memory.
>
> Because that approach in general looks not ideal.
>
> Normally you want the main u-boot already running with less privileges
> so firmware like openSBI should've been already loaded before that.
> Even more true when you're employing methods to protect memory regions
> from less privileged access.
>
> A lot of socs set u-boot as opensbi payload, but for the example the D1
> mainline approach uses the Allwinner TOC1 image format to load both
> opensbi and the main uboot into memory from its 1st stage loader.
>
>
> Of course the best way would be to just mimic what a number of
> arm64 and also riscv socs do and use already existing u-boot utilities.
>
> U-Boot can create a FIT image containing both main u-boot, dtb and
> firmware images that all get loaded from SPL and placed at the correct
> addresses before having the SPL jump into opensbi and from there
> into u-boot [1] .
>
> And as Anup was writing, reserved-memory should then be the way
> to go to tell the kernel what regions to omit.
>
> And mainline u-boot has already the means to even take the reserved-memory
> from the devicetree used by opensbi and copy it to a new devicetree,
> if the second one is different.
>
>
> Heiko
>
>
> [0] https://github.com/T-head-Semi/u-boot/blob/main/include/configs/ice-c910.h#L46
> [1] see spl_invoke_opensbi() in common/spl/spl_opensbi.c
> [2] see riscv_board_reserved_mem_fixup() in arch/riscv/lib/fdt_fixup.c
>
> >
> > Guo Ren (3):
> > riscv: Remove 2MB offset in the mm layout
> > riscv: Add early_param to decrease firmware region
> > riscv: Add riscv.fwsz kernel parameter
> >
> > .../admin-guide/kernel-parameters.txt | 3 +++
> > arch/riscv/include/asm/page.h | 8 +++++++
> > arch/riscv/kernel/head.S | 10 +++-----
> > arch/riscv/kernel/vmlinux.lds.S | 5 ++--
> > arch/riscv/mm/init.c | 23 ++++++++++++++++---
> > 5 files changed, 36 insertions(+), 13 deletions(-)
> >
> >
>
>
>
>
--
Best Regards
Guo Ren
ML: https://lore.kernel.org/linux-csky/
On Wed, Nov 24, 2021 at 4:01 AM Atish Patra <[email protected]> wrote:
>
> On Tue, Nov 23, 2021 at 11:33 AM Heiko Stübner <[email protected]> wrote:
> >
> > Hi Guo,
> >
> > Am Dienstag, 23. November 2021, 02:57:14 CET schrieb [email protected]:
> > > From: Guo Ren <[email protected]>
> > >
> > > The firmware of riscv (such as opensbi) occupy 2MB(64bit) /
> > > 4MB(32bit) in Linux. It's very wasteful to small memory footprint
> > > soc chip such as Allwinner D1s/F133. The kernel parameter gives a
> > > chance to users to set the proper size of the firmware and get
> > > more than 1.5MB of memory.
> >
> > is this kernel parameter approach a result of the T-Head Ice-SoC
> > currently loading its openSBI from inside the main u-boot via extfs-load,
> > directly before the kernel itself [0] ?
>
> Looking at the defconfig[1], it may be U-Boot SPL not U-Boot proper. I
> may be looking at the wrong config though.
> If U-Boot SPL is actually used, you don't even need to manually load
> OpenSBI "fw_jump" binary.
>
> As Heiko pointed, you should just follow how U-Boot SPL works on
> hifive unmatched (creating the FIT image)
> The standard U-Boot SPL uses with fw_dynamic which provides all the
> flexibility you want.
I've no right to force users' flavor of boot flow.
1) SPL -> opensbi M-mode -> u-boot S-mode -> Linux
2) SPL -> u-boot M-mode -> opensbi M-mode -> Linux
All are okay for me. I think the most straightforward reason for
people choosing 2) is that they want to try the newest OpenSBI & Linux
and 2) is more convenient for replacing.
>
> [1] https://github.com/T-head-Semi/u-boot/blob/main/configs/ice_evb_c910_defconfig
> >
> > Because that approach in general looks not ideal.
> >
> > Normally you want the main u-boot already running with less privileges
> > so firmware like openSBI should've been already loaded before that.
> > Even more true when you're employing methods to protect memory regions
> > from less privileged access.
> >
> > A lot of socs set u-boot as opensbi payload, but for the example the D1
> > mainline approach uses the Allwinner TOC1 image format to load both
> > opensbi and the main uboot into memory from its 1st stage loader.
> >
> >
> > Of course the best way would be to just mimic what a number of
> > arm64 and also riscv socs do and use already existing u-boot utilities.
> >
> > U-Boot can create a FIT image containing both main u-boot, dtb and
> > firmware images that all get loaded from SPL and placed at the correct
> > addresses before having the SPL jump into opensbi and from there
> > into u-boot [1] .
> >
> > And as Anup was writing, reserved-memory should then be the way
> > to go to tell the kernel what regions to omit.
> >
> > And mainline u-boot has already the means to even take the reserved-memory
> > from the devicetree used by opensbi and copy it to a new devicetree,
> > if the second one is different.
> >
> >
> > Heiko
> >
> >
> > [0] https://github.com/T-head-Semi/u-boot/blob/main/include/configs/ice-c910.h#L46
> > [1] see spl_invoke_opensbi() in common/spl/spl_opensbi.c
> > [2] see riscv_board_reserved_mem_fixup() in arch/riscv/lib/fdt_fixup.c
> >
> > >
> > > Guo Ren (3):
> > > riscv: Remove 2MB offset in the mm layout
> > > riscv: Add early_param to decrease firmware region
> > > riscv: Add riscv.fwsz kernel parameter
> > >
> > > .../admin-guide/kernel-parameters.txt | 3 +++
> > > arch/riscv/include/asm/page.h | 8 +++++++
> > > arch/riscv/kernel/head.S | 10 +++-----
> > > arch/riscv/kernel/vmlinux.lds.S | 5 ++--
> > > arch/riscv/mm/init.c | 23 ++++++++++++++++---
> > > 5 files changed, 36 insertions(+), 13 deletions(-)
> > >
> > >
> >
> >
> >
> >
> >
> > _______________________________________________
> > linux-riscv mailing list
> > [email protected]
> > http://lists.infradead.org/mailman/listinfo/linux-riscv
>
>
>
> --
> Regards,
> Atish
--
Best Regards
Guo Ren
ML: https://lore.kernel.org/linux-csky/
On Tue, Nov 23, 2021 at 9:37 PM Alexandre Ghiti
<[email protected]> wrote:
>
> On Tue, Nov 23, 2021 at 4:56 AM Anup Patel <[email protected]> wrote:
> >
> > +Alex
> >
> > On Tue, Nov 23, 2021 at 7:27 AM <[email protected]> wrote:
> > >
> > > From: Guo Ren <[email protected]>
> > >
> > > The current RISC-V's mm layout is based on a 2MB offset and wasting
> > > memory. Remove 2MB offset and map PAGE_OFFSET at start_of_DRAM.
> > > Then we could reduce the memory reserved for opensbi in the next
> > > patch.
> >
> > The real problem is that the generic kernel marks memory before
> > __pa(PAGE_OFFSET) as reserved which is evident from the boot
> > print "OF: fdt: Ignoring memory range 0x80000000 - 0x80200000".
>
> Agreed, memblock 'rejects' this region because MIN_MEMBLOCK_ADDR is
> defined as __pa(PAGE_OFFSET) which points to 0x80200000. I have a
> patch to enable the use of hugepages for the linear mapping [1] that
> does just that, things are not that easy since then it breaks initrd
> initialization which is an early caller of __va, I updated this
> patchset a few months ago, I can push that soon @Guo Ren.
Seems your patch makes the mapping of early_pg_dir & swapper_pg_dir different.
early_pg_dir: 0x80200000 -> 0xffffffe000000000
swapper_pg_dir: 0x80000000 -> 0xffffffe000000000
It breaks the vmlinux.ld.S, doesn't it?
>
> [1] https://lkml.org/lkml/2020/6/3/696
>
>
>
> >
> > One simple way to re-claim the first 2MB of memory is by:
> > 1) Not placing OpenSBI firmware at start of RAM and rather
> > place it towards end/middle or RAM away from kernel and initrd
> > 2) Load kernel at start of the RAM
> >
> > The point#1 is already supported by OpenSBI firmwares using
> > position independent compilation. In fact, U-Boot SPL does
> > not load OpenSBI firmware at the start of RAM.
> >
> > I would suggest Allwinner D1 to follow U-Boot SPL and have
> > the booting stage before OpenSBI to load OpenSBI firmware
> > somewhere else.
> >
> > Regards,
> > Anup
> >
> > >
> > > Signed-off-by: Guo Ren <[email protected]>
> > > Cc: Palmer Dabbelt <[email protected]>
> > > Cc: Anup Patel <[email protected]>
> > > Cc: Atish Patra <[email protected]>
> > > ---
> > > arch/riscv/include/asm/page.h | 8 ++++++++
> > > arch/riscv/kernel/head.S | 10 +++-------
> > > arch/riscv/kernel/vmlinux.lds.S | 5 ++---
> > > arch/riscv/mm/init.c | 11 ++++++++---
> > > 4 files changed, 21 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/arch/riscv/include/asm/page.h b/arch/riscv/include/asm/page.h
> > > index b3e5ff0125fe..299147c78b4a 100644
> > > --- a/arch/riscv/include/asm/page.h
> > > +++ b/arch/riscv/include/asm/page.h
> > > @@ -16,6 +16,14 @@
> > > #define PAGE_SIZE (_AC(1, UL) << PAGE_SHIFT)
> > > #define PAGE_MASK (~(PAGE_SIZE - 1))
> > >
> > > +#if __riscv_xlen == 64
> > > +/* Image load offset(2MB) from start of RAM */
> > > +#define LOAD_OFFSET 0x200000
> > > +#else
> > > +/* Image load offset(4MB) from start of RAM */
> > > +#define LOAD_OFFSET 0x400000
> > > +#endif
> > > +
> > > #ifdef CONFIG_64BIT
> > > #define HUGE_MAX_HSTATE 2
> > > #else
> > > diff --git a/arch/riscv/kernel/head.S b/arch/riscv/kernel/head.S
> > > index f52f01ecbeea..a6ac892d2ccf 100644
> > > --- a/arch/riscv/kernel/head.S
> > > +++ b/arch/riscv/kernel/head.S
> > > @@ -61,13 +61,7 @@ ENTRY(_start)
> > > /* Image load offset (0MB) from start of RAM for M-mode */
> > > .dword 0
> > > #else
> > > -#if __riscv_xlen == 64
> > > - /* Image load offset(2MB) from start of RAM */
> > > - .dword 0x200000
> > > -#else
> > > - /* Image load offset(4MB) from start of RAM */
> > > - .dword 0x400000
> > > -#endif
> > > + .dword LOAD_OFFSET
> > > #endif
> > > /* Effective size of kernel image */
> > > .dword _end - _start
> > > @@ -94,6 +88,8 @@ relocate:
> > > la a1, kernel_map
> > > XIP_FIXUP_OFFSET a1
> > > REG_L a1, KERNEL_MAP_VIRT_ADDR(a1)
> > > + li a2, LOAD_OFFSET
> > > + add a1, a1, a2
> > > la a2, _start
> > > sub a1, a1, a2
> > > add ra, ra, a1
> > > diff --git a/arch/riscv/kernel/vmlinux.lds.S b/arch/riscv/kernel/vmlinux.lds.S
> > > index 5104f3a871e3..75b7c72cd4bd 100644
> > > --- a/arch/riscv/kernel/vmlinux.lds.S
> > > +++ b/arch/riscv/kernel/vmlinux.lds.S
> > > @@ -11,10 +11,9 @@
> > > #else
> > >
> > > #include <asm/pgtable.h>
> > > -#define LOAD_OFFSET KERNEL_LINK_ADDR
> > >
> > > -#include <asm/vmlinux.lds.h>
> > > #include <asm/page.h>
> > > +#include <asm/vmlinux.lds.h>
> > > #include <asm/cache.h>
> > > #include <asm/thread_info.h>
> > > #include <asm/set_memory.h>
> > > @@ -32,7 +31,7 @@ PECOFF_FILE_ALIGNMENT = 0x200;
> > > SECTIONS
> > > {
> > > /* Beginning of code and text segment */
> > > - . = LOAD_OFFSET;
> > > + . = LOAD_OFFSET + KERNEL_LINK_ADDR;
> > > _start = .;
> > > HEAD_TEXT_SECTION
> > > . = ALIGN(PAGE_SIZE);
> > > diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
> > > index 24b2b8044602..920e78f8c3e4 100644
> > > --- a/arch/riscv/mm/init.c
> > > +++ b/arch/riscv/mm/init.c
> > > @@ -221,6 +221,11 @@ static void __init setup_bootmem(void)
> > > if (!IS_ENABLED(CONFIG_BUILTIN_DTB))
> > > memblock_reserve(dtb_early_pa, fdt_totalsize(dtb_early_va));
> > >
> > > + /*
> > > + * Reserve OpenSBI region and depends on PMP to deny accesses.
> > > + */
> > > + memblock_reserve(__pa(PAGE_OFFSET), LOAD_OFFSET);
> > > +
> > > early_init_fdt_scan_reserved_mem();
> > > dma_contiguous_reserve(dma32_phys_limit);
> > > if (IS_ENABLED(CONFIG_64BIT))
> > > @@ -604,7 +609,7 @@ asmlinkage void __init setup_vm(uintptr_t dtb_pa)
> > >
> > > kernel_map.va_kernel_xip_pa_offset = kernel_map.virt_addr - kernel_map.xiprom;
> > > #else
> > > - kernel_map.phys_addr = (uintptr_t)(&_start);
> > > + kernel_map.phys_addr = (uintptr_t)(&_start) - LOAD_OFFSET;
> > > kernel_map.size = (uintptr_t)(&_end) - kernel_map.phys_addr;
> > > #endif
> > > kernel_map.va_pa_offset = PAGE_OFFSET - kernel_map.phys_addr;
> > > @@ -645,8 +650,8 @@ asmlinkage void __init setup_vm(uintptr_t dtb_pa)
> > > create_pmd_mapping(trampoline_pmd, kernel_map.virt_addr,
> > > kernel_map.xiprom, PMD_SIZE, PAGE_KERNEL_EXEC);
> > > #else
> > > - create_pmd_mapping(trampoline_pmd, kernel_map.virt_addr,
> > > - kernel_map.phys_addr, PMD_SIZE, PAGE_KERNEL_EXEC);
> > > + create_pmd_mapping(trampoline_pmd, kernel_map.virt_addr + LOAD_OFFSET,
> > > + kernel_map.phys_addr + LOAD_OFFSET, PMD_SIZE, PAGE_KERNEL_EXEC);
> > > #endif
> > > #else
> > > /* Setup trampoline PGD */
> > > --
> > > 2.25.1
> > >
--
Best Regards
Guo Ren
ML: https://lore.kernel.org/linux-csky/
Am Mittwoch, 24. November 2021, 07:49:26 CET schrieb Guo Ren:
> On Wed, Nov 24, 2021 at 4:01 AM Atish Patra <[email protected]> wrote:
> >
> > On Tue, Nov 23, 2021 at 11:33 AM Heiko St?bner <[email protected]> wrote:
> > >
> > > Hi Guo,
> > >
> > > Am Dienstag, 23. November 2021, 02:57:14 CET schrieb [email protected]:
> > > > From: Guo Ren <[email protected]>
> > > >
> > > > The firmware of riscv (such as opensbi) occupy 2MB(64bit) /
> > > > 4MB(32bit) in Linux. It's very wasteful to small memory footprint
> > > > soc chip such as Allwinner D1s/F133. The kernel parameter gives a
> > > > chance to users to set the proper size of the firmware and get
> > > > more than 1.5MB of memory.
> > >
> > > is this kernel parameter approach a result of the T-Head Ice-SoC
> > > currently loading its openSBI from inside the main u-boot via extfs-load,
> > > directly before the kernel itself [0] ?
> >
> > Looking at the defconfig[1], it may be U-Boot SPL not U-Boot proper. I
> > may be looking at the wrong config though.
> > If U-Boot SPL is actually used, you don't even need to manually load
> > OpenSBI "fw_jump" binary.
> >
> > As Heiko pointed, you should just follow how U-Boot SPL works on
> > hifive unmatched (creating the FIT image)
> > The standard U-Boot SPL uses with fw_dynamic which provides all the
> > flexibility you want.
> I've no right to force users' flavor of boot flow.
>
> 1) SPL -> opensbi M-mode -> u-boot S-mode -> Linux
> 2) SPL -> u-boot M-mode -> opensbi M-mode -> Linux
>
> All are okay for me. I think the most straightforward reason for
> people choosing 2) is that they want to try the newest OpenSBI & Linux
> and 2) is more convenient for replacing.
Though that second option is merely a hack during development.
Having u-boot run in M-mode creates an attack surface that is a lot
bigger (with it running usb, ethernet and whatnot) compared to shedding
privileges before that.
I'd consider openSBI as part of the device firmware, so that shouldn't be
a component you replace daily. Also U-Boot for example already provides
established ways to sign and verify the parts loaded by SPL, by signing
the created FIT image this would also include the openSBI image.
So in case (1) you can add more security by simply adding the necessary
key references during u-boot build, where on the other hand if you _want_
security in case (2) you're back to hand-rolling any verification
[with less review and thus more prone to have issues]
Having the _ability_ to verify the loaded firmware components can be a
requirement in projects, so I think the default should always case (1),
to not encourage insecure implementations any more than necessary ;-) .
Heiko
> >
> > [1] https://github.com/T-head-Semi/u-boot/blob/main/configs/ice_evb_c910_defconfig
> > >
> > > Because that approach in general looks not ideal.
> > >
> > > Normally you want the main u-boot already running with less privileges
> > > so firmware like openSBI should've been already loaded before that.
> > > Even more true when you're employing methods to protect memory regions
> > > from less privileged access.
> > >
> > > A lot of socs set u-boot as opensbi payload, but for the example the D1
> > > mainline approach uses the Allwinner TOC1 image format to load both
> > > opensbi and the main uboot into memory from its 1st stage loader.
> > >
> > >
> > > Of course the best way would be to just mimic what a number of
> > > arm64 and also riscv socs do and use already existing u-boot utilities.
> > >
> > > U-Boot can create a FIT image containing both main u-boot, dtb and
> > > firmware images that all get loaded from SPL and placed at the correct
> > > addresses before having the SPL jump into opensbi and from there
> > > into u-boot [1] .
> > >
> > > And as Anup was writing, reserved-memory should then be the way
> > > to go to tell the kernel what regions to omit.
> > >
> > > And mainline u-boot has already the means to even take the reserved-memory
> > > from the devicetree used by opensbi and copy it to a new devicetree,
> > > if the second one is different.
> > >
> > >
> > > Heiko
> > >
> > >
> > > [0] https://github.com/T-head-Semi/u-boot/blob/main/include/configs/ice-c910.h#L46
> > > [1] see spl_invoke_opensbi() in common/spl/spl_opensbi.c
> > > [2] see riscv_board_reserved_mem_fixup() in arch/riscv/lib/fdt_fixup.c
> > >
> > > >
> > > > Guo Ren (3):
> > > > riscv: Remove 2MB offset in the mm layout
> > > > riscv: Add early_param to decrease firmware region
> > > > riscv: Add riscv.fwsz kernel parameter
> > > >
> > > > .../admin-guide/kernel-parameters.txt | 3 +++
> > > > arch/riscv/include/asm/page.h | 8 +++++++
> > > > arch/riscv/kernel/head.S | 10 +++-----
> > > > arch/riscv/kernel/vmlinux.lds.S | 5 ++--
> > > > arch/riscv/mm/init.c | 23 ++++++++++++++++---
> > > > 5 files changed, 36 insertions(+), 13 deletions(-)
> > > >
> > > >
> > >
> > >
> > >
> > >
> > >
> > > _______________________________________________
> > > linux-riscv mailing list
> > > [email protected]
> > > http://lists.infradead.org/mailman/listinfo/linux-riscv
> >
> >
> >
> > --
> > Regards,
> > Atish
>
>
>
>
Am Mittwoch, 24. November 2021, 07:42:17 CET schrieb Guo Ren:
> On Wed, Nov 24, 2021 at 3:33 AM Heiko St?bner <[email protected]> wrote:
> >
> > Hi Guo,
> >
> > Am Dienstag, 23. November 2021, 02:57:14 CET schrieb [email protected]:
> > > From: Guo Ren <[email protected]>
> > >
> > > The firmware of riscv (such as opensbi) occupy 2MB(64bit) /
> > > 4MB(32bit) in Linux. It's very wasteful to small memory footprint
> > > soc chip such as Allwinner D1s/F133. The kernel parameter gives a
> > > chance to users to set the proper size of the firmware and get
> > > more than 1.5MB of memory.
> >
> > is this kernel parameter approach a result of the T-Head Ice-SoC
> > currently loading its openSBI from inside the main u-boot via extfs-load,
> > directly before the kernel itself [0] ?
> The patch is not related to that issue. The patch just helps users who
> put opensbi at 0~2MB paddr to save memory.
So as Anup wrote, this should just be solved by using a correct reserved memory
node in the devicetree passed to the kernel. And the firmware will know what
memory region it actually occupies ;-)
>
> >
> > Because that approach in general looks not ideal.
> >
> > Normally you want the main u-boot already running with less privileges
> > so firmware like openSBI should've been already loaded before that.
> > Even more true when you're employing methods to protect memory regions
> > from less privileged access.
> >
> > A lot of socs set u-boot as opensbi payload, but for the example the D1
> > mainline approach uses the Allwinner TOC1 image format to load both
> > opensbi and the main uboot into memory from its 1st stage loader.
> >
> >
> > Of course the best way would be to just mimic what a number of
> > arm64 and also riscv socs do and use already existing u-boot utilities.
> >
> > U-Boot can create a FIT image containing both main u-boot, dtb and
> > firmware images that all get loaded from SPL and placed at the correct
> > addresses before having the SPL jump into opensbi and from there
> > into u-boot [1] .
> >
> > And as Anup was writing, reserved-memory should then be the way
> > to go to tell the kernel what regions to omit.
> >
> > And mainline u-boot has already the means to even take the reserved-memory
> > from the devicetree used by opensbi and copy it to a new devicetree,
> > if the second one is different.
> >
> >
> > Heiko
> >
> >
> > [0] https://github.com/T-head-Semi/u-boot/blob/main/include/configs/ice-c910.h#L46
> > [1] see spl_invoke_opensbi() in common/spl/spl_opensbi.c
> > [2] see riscv_board_reserved_mem_fixup() in arch/riscv/lib/fdt_fixup.c
> >
> > >
> > > Guo Ren (3):
> > > riscv: Remove 2MB offset in the mm layout
> > > riscv: Add early_param to decrease firmware region
> > > riscv: Add riscv.fwsz kernel parameter
> > >
> > > .../admin-guide/kernel-parameters.txt | 3 +++
> > > arch/riscv/include/asm/page.h | 8 +++++++
> > > arch/riscv/kernel/head.S | 10 +++-----
> > > arch/riscv/kernel/vmlinux.lds.S | 5 ++--
> > > arch/riscv/mm/init.c | 23 ++++++++++++++++---
> > > 5 files changed, 36 insertions(+), 13 deletions(-)
> > >
> > >
> >
> >
> >
> >
>
>
>
On Wed, Nov 24, 2021 at 8:15 PM Heiko Stübner <[email protected]> wrote:
>
> Am Mittwoch, 24. November 2021, 07:49:26 CET schrieb Guo Ren:
> > On Wed, Nov 24, 2021 at 4:01 AM Atish Patra <[email protected]> wrote:
> > >
> > > On Tue, Nov 23, 2021 at 11:33 AM Heiko Stübner <[email protected]> wrote:
> > > >
> > > > Hi Guo,
> > > >
> > > > Am Dienstag, 23. November 2021, 02:57:14 CET schrieb [email protected]:
> > > > > From: Guo Ren <[email protected]>
> > > > >
> > > > > The firmware of riscv (such as opensbi) occupy 2MB(64bit) /
> > > > > 4MB(32bit) in Linux. It's very wasteful to small memory footprint
> > > > > soc chip such as Allwinner D1s/F133. The kernel parameter gives a
> > > > > chance to users to set the proper size of the firmware and get
> > > > > more than 1.5MB of memory.
> > > >
> > > > is this kernel parameter approach a result of the T-Head Ice-SoC
> > > > currently loading its openSBI from inside the main u-boot via extfs-load,
> > > > directly before the kernel itself [0] ?
> > >
> > > Looking at the defconfig[1], it may be U-Boot SPL not U-Boot proper. I
> > > may be looking at the wrong config though.
> > > If U-Boot SPL is actually used, you don't even need to manually load
> > > OpenSBI "fw_jump" binary.
> > >
> > > As Heiko pointed, you should just follow how U-Boot SPL works on
> > > hifive unmatched (creating the FIT image)
> > > The standard U-Boot SPL uses with fw_dynamic which provides all the
> > > flexibility you want.
> > I've no right to force users' flavor of boot flow.
> >
> > 1) SPL -> opensbi M-mode -> u-boot S-mode -> Linux
> > 2) SPL -> u-boot M-mode -> opensbi M-mode -> Linux
> >
> > All are okay for me. I think the most straightforward reason for
> > people choosing 2) is that they want to try the newest OpenSBI & Linux
> > and 2) is more convenient for replacing.
>
> Though that second option is merely a hack during development.
>
> Having u-boot run in M-mode creates an attack surface that is a lot
> bigger (with it running usb, ethernet and whatnot) compared to shedding
> privileges before that.
>
> I'd consider openSBI as part of the device firmware, so that shouldn't be
> a component you replace daily. Also U-Boot for example already provides
> established ways to sign and verify the parts loaded by SPL, by signing
> the created FIT image this would also include the openSBI image.
>
> So in case (1) you can add more security by simply adding the necessary
> key references during u-boot build, where on the other hand if you _want_
> security in case (2) you're back to hand-rolling any verification
> [with less review and thus more prone to have issues]
>
> Having the _ability_ to verify the loaded firmware components can be a
> requirement in projects, so I think the default should always case (1),
> to not encourage insecure implementations any more than necessary ;-) .
I think nobody would use case (2) in production.
>
>
> Heiko
>
>
> > >
> > > [1] https://github.com/T-head-Semi/u-boot/blob/main/configs/ice_evb_c910_defconfig
> > > >
> > > > Because that approach in general looks not ideal.
> > > >
> > > > Normally you want the main u-boot already running with less privileges
> > > > so firmware like openSBI should've been already loaded before that.
> > > > Even more true when you're employing methods to protect memory regions
> > > > from less privileged access.
> > > >
> > > > A lot of socs set u-boot as opensbi payload, but for the example the D1
> > > > mainline approach uses the Allwinner TOC1 image format to load both
> > > > opensbi and the main uboot into memory from its 1st stage loader.
> > > >
> > > >
> > > > Of course the best way would be to just mimic what a number of
> > > > arm64 and also riscv socs do and use already existing u-boot utilities.
> > > >
> > > > U-Boot can create a FIT image containing both main u-boot, dtb and
> > > > firmware images that all get loaded from SPL and placed at the correct
> > > > addresses before having the SPL jump into opensbi and from there
> > > > into u-boot [1] .
> > > >
> > > > And as Anup was writing, reserved-memory should then be the way
> > > > to go to tell the kernel what regions to omit.
> > > >
> > > > And mainline u-boot has already the means to even take the reserved-memory
> > > > from the devicetree used by opensbi and copy it to a new devicetree,
> > > > if the second one is different.
> > > >
> > > >
> > > > Heiko
> > > >
> > > >
> > > > [0] https://github.com/T-head-Semi/u-boot/blob/main/include/configs/ice-c910.h#L46
> > > > [1] see spl_invoke_opensbi() in common/spl/spl_opensbi.c
> > > > [2] see riscv_board_reserved_mem_fixup() in arch/riscv/lib/fdt_fixup.c
> > > >
> > > > >
> > > > > Guo Ren (3):
> > > > > riscv: Remove 2MB offset in the mm layout
> > > > > riscv: Add early_param to decrease firmware region
> > > > > riscv: Add riscv.fwsz kernel parameter
> > > > >
> > > > > .../admin-guide/kernel-parameters.txt | 3 +++
> > > > > arch/riscv/include/asm/page.h | 8 +++++++
> > > > > arch/riscv/kernel/head.S | 10 +++-----
> > > > > arch/riscv/kernel/vmlinux.lds.S | 5 ++--
> > > > > arch/riscv/mm/init.c | 23 ++++++++++++++++---
> > > > > 5 files changed, 36 insertions(+), 13 deletions(-)
> > > > >
> > > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > > _______________________________________________
> > > > linux-riscv mailing list
> > > > [email protected]
> > > > http://lists.infradead.org/mailman/listinfo/linux-riscv
> > >
> > >
> > >
> > > --
> > > Regards,
> > > Atish
> >
> >
> >
> >
>
>
>
>
--
Best Regards
Guo Ren
ML: https://lore.kernel.org/linux-csky/
On 11/24/21 12:58, Guo Ren wrote:
> On Tue, Nov 23, 2021 at 9:37 PM Alexandre Ghiti
> <[email protected]> wrote:
>> On Tue, Nov 23, 2021 at 4:56 AM Anup Patel <[email protected]> wrote:
>>> +Alex
>>>
>>> On Tue, Nov 23, 2021 at 7:27 AM <[email protected]> wrote:
>>>> From: Guo Ren <[email protected]>
>>>>
>>>> The current RISC-V's mm layout is based on a 2MB offset and wasting
>>>> memory. Remove 2MB offset and map PAGE_OFFSET at start_of_DRAM.
>>>> Then we could reduce the memory reserved for opensbi in the next
>>>> patch.
>>> The real problem is that the generic kernel marks memory before
>>> __pa(PAGE_OFFSET) as reserved which is evident from the boot
>>> print "OF: fdt: Ignoring memory range 0x80000000 - 0x80200000".
>> Agreed, memblock 'rejects' this region because MIN_MEMBLOCK_ADDR is
>> defined as __pa(PAGE_OFFSET) which points to 0x80200000. I have a
>> patch to enable the use of hugepages for the linear mapping [1] that
>> does just that, things are not that easy since then it breaks initrd
>> initialization which is an early caller of __va, I updated this
>> patchset a few months ago, I can push that soon @Guo Ren.
> Seems your patch makes the mapping of early_pg_dir & swapper_pg_dir different.
>
> early_pg_dir: 0x80200000 -> 0xffffffe000000000
> swapper_pg_dir: 0x80000000 -> 0xffffffe000000000
>
> It breaks the vmlinux.ld.S, doesn't it?
Indeed, early_pg_dir and swapper_pg_dir have different mappings, but
that's because when establishing the early_pg_dir mapping, the only
piece of information we have is the load address of the kernel, which is
0x8020_0000 (or whatever). And that breaks initrd because
__early_init_dt_declare_initrd calls __va in between and then when
swapper_pg_dir is used, it breaks because the mappings differ. I did not
find any better way to do that, and IIRC arm64 has a similar issue.
>
>> [1] https://lkml.org/lkml/2020/6/3/696
>>
>>
>>
>>> One simple way to re-claim the first 2MB of memory is by:
>>> 1) Not placing OpenSBI firmware at start of RAM and rather
>>> place it towards end/middle or RAM away from kernel and initrd
>>> 2) Load kernel at start of the RAM
>>>
>>> The point#1 is already supported by OpenSBI firmwares using
>>> position independent compilation. In fact, U-Boot SPL does
>>> not load OpenSBI firmware at the start of RAM.
>>>
>>> I would suggest Allwinner D1 to follow U-Boot SPL and have
>>> the booting stage before OpenSBI to load OpenSBI firmware
>>> somewhere else.
>>>
>>> Regards,
>>> Anup
>>>
>>>> Signed-off-by: Guo Ren <[email protected]>
>>>> Cc: Palmer Dabbelt <[email protected]>
>>>> Cc: Anup Patel <[email protected]>
>>>> Cc: Atish Patra <[email protected]>
>>>> ---
>>>> arch/riscv/include/asm/page.h | 8 ++++++++
>>>> arch/riscv/kernel/head.S | 10 +++-------
>>>> arch/riscv/kernel/vmlinux.lds.S | 5 ++---
>>>> arch/riscv/mm/init.c | 11 ++++++++---
>>>> 4 files changed, 21 insertions(+), 13 deletions(-)
>>>>
>>>> diff --git a/arch/riscv/include/asm/page.h b/arch/riscv/include/asm/page.h
>>>> index b3e5ff0125fe..299147c78b4a 100644
>>>> --- a/arch/riscv/include/asm/page.h
>>>> +++ b/arch/riscv/include/asm/page.h
>>>> @@ -16,6 +16,14 @@
>>>> #define PAGE_SIZE (_AC(1, UL) << PAGE_SHIFT)
>>>> #define PAGE_MASK (~(PAGE_SIZE - 1))
>>>>
>>>> +#if __riscv_xlen == 64
>>>> +/* Image load offset(2MB) from start of RAM */
>>>> +#define LOAD_OFFSET 0x200000
>>>> +#else
>>>> +/* Image load offset(4MB) from start of RAM */
>>>> +#define LOAD_OFFSET 0x400000
>>>> +#endif
>>>> +
>>>> #ifdef CONFIG_64BIT
>>>> #define HUGE_MAX_HSTATE 2
>>>> #else
>>>> diff --git a/arch/riscv/kernel/head.S b/arch/riscv/kernel/head.S
>>>> index f52f01ecbeea..a6ac892d2ccf 100644
>>>> --- a/arch/riscv/kernel/head.S
>>>> +++ b/arch/riscv/kernel/head.S
>>>> @@ -61,13 +61,7 @@ ENTRY(_start)
>>>> /* Image load offset (0MB) from start of RAM for M-mode */
>>>> .dword 0
>>>> #else
>>>> -#if __riscv_xlen == 64
>>>> - /* Image load offset(2MB) from start of RAM */
>>>> - .dword 0x200000
>>>> -#else
>>>> - /* Image load offset(4MB) from start of RAM */
>>>> - .dword 0x400000
>>>> -#endif
>>>> + .dword LOAD_OFFSET
>>>> #endif
>>>> /* Effective size of kernel image */
>>>> .dword _end - _start
>>>> @@ -94,6 +88,8 @@ relocate:
>>>> la a1, kernel_map
>>>> XIP_FIXUP_OFFSET a1
>>>> REG_L a1, KERNEL_MAP_VIRT_ADDR(a1)
>>>> + li a2, LOAD_OFFSET
>>>> + add a1, a1, a2
>>>> la a2, _start
>>>> sub a1, a1, a2
>>>> add ra, ra, a1
>>>> diff --git a/arch/riscv/kernel/vmlinux.lds.S b/arch/riscv/kernel/vmlinux.lds.S
>>>> index 5104f3a871e3..75b7c72cd4bd 100644
>>>> --- a/arch/riscv/kernel/vmlinux.lds.S
>>>> +++ b/arch/riscv/kernel/vmlinux.lds.S
>>>> @@ -11,10 +11,9 @@
>>>> #else
>>>>
>>>> #include <asm/pgtable.h>
>>>> -#define LOAD_OFFSET KERNEL_LINK_ADDR
>>>>
>>>> -#include <asm/vmlinux.lds.h>
>>>> #include <asm/page.h>
>>>> +#include <asm/vmlinux.lds.h>
>>>> #include <asm/cache.h>
>>>> #include <asm/thread_info.h>
>>>> #include <asm/set_memory.h>
>>>> @@ -32,7 +31,7 @@ PECOFF_FILE_ALIGNMENT = 0x200;
>>>> SECTIONS
>>>> {
>>>> /* Beginning of code and text segment */
>>>> - . = LOAD_OFFSET;
>>>> + . = LOAD_OFFSET + KERNEL_LINK_ADDR;
>>>> _start = .;
>>>> HEAD_TEXT_SECTION
>>>> . = ALIGN(PAGE_SIZE);
>>>> diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
>>>> index 24b2b8044602..920e78f8c3e4 100644
>>>> --- a/arch/riscv/mm/init.c
>>>> +++ b/arch/riscv/mm/init.c
>>>> @@ -221,6 +221,11 @@ static void __init setup_bootmem(void)
>>>> if (!IS_ENABLED(CONFIG_BUILTIN_DTB))
>>>> memblock_reserve(dtb_early_pa, fdt_totalsize(dtb_early_va));
>>>>
>>>> + /*
>>>> + * Reserve OpenSBI region and depends on PMP to deny accesses.
>>>> + */
>>>> + memblock_reserve(__pa(PAGE_OFFSET), LOAD_OFFSET);
>>>> +
>>>> early_init_fdt_scan_reserved_mem();
>>>> dma_contiguous_reserve(dma32_phys_limit);
>>>> if (IS_ENABLED(CONFIG_64BIT))
>>>> @@ -604,7 +609,7 @@ asmlinkage void __init setup_vm(uintptr_t dtb_pa)
>>>>
>>>> kernel_map.va_kernel_xip_pa_offset = kernel_map.virt_addr - kernel_map.xiprom;
>>>> #else
>>>> - kernel_map.phys_addr = (uintptr_t)(&_start);
>>>> + kernel_map.phys_addr = (uintptr_t)(&_start) - LOAD_OFFSET;
>>>> kernel_map.size = (uintptr_t)(&_end) - kernel_map.phys_addr;
>>>> #endif
>>>> kernel_map.va_pa_offset = PAGE_OFFSET - kernel_map.phys_addr;
>>>> @@ -645,8 +650,8 @@ asmlinkage void __init setup_vm(uintptr_t dtb_pa)
>>>> create_pmd_mapping(trampoline_pmd, kernel_map.virt_addr,
>>>> kernel_map.xiprom, PMD_SIZE, PAGE_KERNEL_EXEC);
>>>> #else
>>>> - create_pmd_mapping(trampoline_pmd, kernel_map.virt_addr,
>>>> - kernel_map.phys_addr, PMD_SIZE, PAGE_KERNEL_EXEC);
>>>> + create_pmd_mapping(trampoline_pmd, kernel_map.virt_addr + LOAD_OFFSET,
>>>> + kernel_map.phys_addr + LOAD_OFFSET, PMD_SIZE, PAGE_KERNEL_EXEC);
>>>> #endif
>>>> #else
>>>> /* Setup trampoline PGD */
>>>> --
>>>> 2.25.1
>>>>
>
>