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

[Jessica Clarke]

On 23 Nov 2021, at 03:44, Anup Patel <anup@xxxxxxxxxxxxxx> wrote:
> 
> +Alex
> 
> On Tue, Nov 23, 2021 at 7:27 AM <guoren@xxxxxxxxxx> wrote:
>> 
>> From: Guo Ren <guoren@xxxxxxxxxxxxxxxxx>
>> 
>> 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@xxxxxxxxxxxxxxxxx>
>> Cc: Palmer Dabbelt <palmer@xxxxxxxxxxx>
>> Cc: Anup Patel <anup.patel@xxxxxxx>
>> Cc: Atish Patra <atishp@xxxxxxxxxxxx>
>> ---
>> 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