diff mbox series

[RFC,2/3] riscv: Add early_param to decrease firmware region

Message ID 20211123015717.542631-3-guoren@kernel.org (mailing list archive)
State New, archived
Headers show
Series riscv: Add riscv.fwsz kernel parameter to save memory | expand

Commit Message

Guo Ren Nov. 23, 2021, 1:57 a.m. UTC 
From: Guo Ren <guoren@linux.alibaba.com>

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 <guoren@linux.alibaba.com>
Cc: Palmer Dabbelt <palmer@dabbelt.com>
Cc: Anup Patel <anup.patel@wdc.com>
Cc: Atish Patra <atishp@rivosinc.com>
---
 arch/riscv/mm/init.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

Comments

Anup Patel Nov. 23, 2021, 3:44 a.m. UTC | #1 
+Alex

On Tue, Nov 23, 2021 at 7:27 AM <guoren@kernel.org> wrote:
>
> From: Guo Ren <guoren@linux.alibaba.com>
>
> 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 <guoren@linux.alibaba.com>
> Cc: Palmer Dabbelt <palmer@dabbelt.com>
> Cc: Anup Patel <anup.patel@wdc.com>
> Cc: Atish Patra <atishp@rivosinc.com>
> ---
>  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
>
Jessica Clarke Nov. 23, 2021, 11:53 a.m. UTC | #2 
On 23 Nov 2021, at 03:44, Anup Patel <anup@brainfault.org> wrote:
> 
> +Alex
> 
> On Tue, Nov 23, 2021 at 7:27 AM <guoren@kernel.org> wrote:
>> 
>> From: Guo Ren <guoren@linux.alibaba.com>
>> 
>> 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 <guoren@linux.alibaba.com>
>> Cc: Palmer Dabbelt <palmer@dabbelt.com>
>> Cc: Anup Patel <anup.patel@wdc.com>
>> Cc: Atish Patra <atishp@rivosinc.com>
>> ---
>> 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
Alexandre Ghiti Nov. 23, 2021, 1:37 p.m. UTC | #3 
On Tue, Nov 23, 2021 at 12:53 PM Jessica Clarke <jrtc27@jrtc27.com> wrote:
>
> On 23 Nov 2021, at 03:44, Anup Patel <anup@brainfault.org> wrote:
> >
> > +Alex
> >
> > On Tue, Nov 23, 2021 at 7:27 AM <guoren@kernel.org> wrote:
> >>
> >> From: Guo Ren <guoren@linux.alibaba.com>
> >>
> >> 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 <guoren@linux.alibaba.com>
> >> Cc: Palmer Dabbelt <palmer@dabbelt.com>
> >> Cc: Anup Patel <anup.patel@wdc.com>
> >> Cc: Atish Patra <atishp@rivosinc.com>
> >> ---
> >> 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
>
diff mbox series

Patch

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