[1/3] loongarch: Call arch_mem_init() before platform_init() in the init sequence

  The platform_init() function which is called during device bootup
contains a few calls to memblock_alloc().
This is an issue because these allocations are done before reserved
memory regions are set aside in arch_mem_init().
This means that there is a possibility for memblock to allocate memory
from any of the reserved memory regions.

Hence, move the call to arch_mem_init() to be earlier in the init
sequence so that all reserved memory is set aside before any allocations
are made with memblock.

Signed-off-by: Oreoluwa Babatunde <quic_obabatun@quicinc.com>
---
 arch/loongarch/kernel/setup.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--
  

Hi, Oreoluwa,

On Sat, Feb 10, 2024 at 8:29 AM Oreoluwa Babatunde
<quic_obabatun@quicinc.com> wrote:
>
> The platform_init() function which is called during device bootup
> contains a few calls to memblock_alloc().
> This is an issue because these allocations are done before reserved
> memory regions are set aside in arch_mem_init().
> This means that there is a possibility for memblock to allocate memory
> from any of the reserved memory regions.
>
> Hence, move the call to arch_mem_init() to be earlier in the init
> sequence so that all reserved memory is set aside before any allocations
> are made with memblock.
>
> Signed-off-by: Oreoluwa Babatunde <quic_obabatun@quicinc.com>
> ---
>  arch/loongarch/kernel/setup.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/loongarch/kernel/setup.c b/arch/loongarch/kernel/setup.c
> index edf2bba..66c307c 100644
> --- a/arch/loongarch/kernel/setup.c
> +++ b/arch/loongarch/kernel/setup.c
> @@ -597,8 +597,8 @@ void __init setup_arch(char **cmdline_p)
>         parse_early_param();
>         reserve_initrd_mem();
>
> -       platform_init();
>         arch_mem_init(cmdline_p);
> +       platform_init();
Thank you for your patch, but I think we cannot simply exchange their
order. If I'm right, you try to move all memblock_reserve() as early
as possible, but both arch_mem_init() and platform_init() call
memblock_reserve(), we should do a complete refactor for this. And
since it works with the existing order, we can simply keep it as is
now.

Huacai

>
>         resource_init();
>  #ifdef CONFIG_SMP
> --
  

On 2/14/2024 5:03 AM, Huacai Chen wrote:
> Hi, Oreoluwa,
>
> On Sat, Feb 10, 2024 at 8:29 AM Oreoluwa Babatunde
> <quic_obabatun@quicinc.com> wrote:
>> The platform_init() function which is called during device bootup
>> contains a few calls to memblock_alloc().
>> This is an issue because these allocations are done before reserved
>> memory regions are set aside in arch_mem_init().
>> This means that there is a possibility for memblock to allocate memory
>> from any of the reserved memory regions.
>>
>> Hence, move the call to arch_mem_init() to be earlier in the init
>> sequence so that all reserved memory is set aside before any allocations
>> are made with memblock.
>>
>> Signed-off-by: Oreoluwa Babatunde <quic_obabatun@quicinc.com>
>> ---
>>  arch/loongarch/kernel/setup.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/loongarch/kernel/setup.c b/arch/loongarch/kernel/setup.c
>> index edf2bba..66c307c 100644
>> --- a/arch/loongarch/kernel/setup.c
>> +++ b/arch/loongarch/kernel/setup.c
>> @@ -597,8 +597,8 @@ void __init setup_arch(char **cmdline_p)
>>         parse_early_param();
>>         reserve_initrd_mem();
>>
>> -       platform_init();
>>         arch_mem_init(cmdline_p);
>> +       platform_init();
> Thank you for your patch, but I think we cannot simply exchange their
> order. If I'm right, you try to move all memblock_reserve() as early
> as possible, but both arch_mem_init() and platform_init() call
> memblock_reserve(), we should do a complete refactor for this. And
> since it works with the existing order, we can simply keep it as is
> now.
>
> Huacai
Hi Huacai,

Thank you for your response!

I'm not trying to move all memblock_reserve() to be as early as possible,
I'm trying to move the call to early_init_fdt_scan_reserved_mem() to be
as early as possible. This is the function that is used to set aside all the
reserved memory regions that are meant for certain devices/drivers.

The reserved memory regions I am referring to are explicitly defined in
the DT. These regions are set aside so that the system will have either
limited access or no access to them at all.
Some of these regions are also defined with a property called no-map
which tells the system not to create a memory mapping for them.
https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/reserved-memory/reserved-memory.yaml#L79

Hence, setting aside these memory regions should take priority and should
be done first before any memblock allocations are done so that the system
does not  unknowingly allocate memory from a region that is meant to be
reserved for a device/driver.

Eg:
    unflatten_and_copy_device_tree() eventually calls memblock_alloc():
    https://elixir.bootlin.com/linux/latest/source/drivers/of/fdt.c#L1264

    Since unflatten_and_copy_device_tree() is called in platform_init(), this
    allocation is done before we are able to set aside any of the reserved
    memory regions from the DT which is supposed to be done by
    early_init_fdt_scan_reserved_mem() in the arch_mem_init() function.

    Hence, it is possible for unflatten_and_copy_device_tree() to allocate
    memory from a region that is meant to be set aside for a device/driver
    without the system knowing.

This can create problems for a device/driver if a region of memory that was
supposed to be set aside for it ends up being allocated for another use case
by memblock_alloc*().

Regards,

Oreoluwa
  

Hi, Oreoluwa,

On Thu, Feb 15, 2024 at 5:31 AM Oreoluwa Babatunde
<quic_obabatun@quicinc.com> wrote:
>
>
> On 2/14/2024 5:03 AM, Huacai Chen wrote:
> > Hi, Oreoluwa,
> >
> > On Sat, Feb 10, 2024 at 8:29 AM Oreoluwa Babatunde
> > <quic_obabatun@quicinc.com> wrote:
> >> The platform_init() function which is called during device bootup
> >> contains a few calls to memblock_alloc().
> >> This is an issue because these allocations are done before reserved
> >> memory regions are set aside in arch_mem_init().
> >> This means that there is a possibility for memblock to allocate memory
> >> from any of the reserved memory regions.
> >>
> >> Hence, move the call to arch_mem_init() to be earlier in the init
> >> sequence so that all reserved memory is set aside before any allocations
> >> are made with memblock.
> >>
> >> Signed-off-by: Oreoluwa Babatunde <quic_obabatun@quicinc.com>
> >> ---
> >>  arch/loongarch/kernel/setup.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/arch/loongarch/kernel/setup.c b/arch/loongarch/kernel/setup.c
> >> index edf2bba..66c307c 100644
> >> --- a/arch/loongarch/kernel/setup.c
> >> +++ b/arch/loongarch/kernel/setup.c
> >> @@ -597,8 +597,8 @@ void __init setup_arch(char **cmdline_p)
> >>         parse_early_param();
> >>         reserve_initrd_mem();
> >>
> >> -       platform_init();
> >>         arch_mem_init(cmdline_p);
> >> +       platform_init();
> > Thank you for your patch, but I think we cannot simply exchange their
> > order. If I'm right, you try to move all memblock_reserve() as early
> > as possible, but both arch_mem_init() and platform_init() call
> > memblock_reserve(), we should do a complete refactor for this. And
> > since it works with the existing order, we can simply keep it as is
> > now.
> >
> > Huacai
> Hi Huacai,
>
> Thank you for your response!
>
> I'm not trying to move all memblock_reserve() to be as early as possible,
> I'm trying to move the call to early_init_fdt_scan_reserved_mem() to be
> as early as possible. This is the function that is used to set aside all the
> reserved memory regions that are meant for certain devices/drivers.
>
> The reserved memory regions I am referring to are explicitly defined in
> the DT. These regions are set aside so that the system will have either
> limited access or no access to them at all.
> Some of these regions are also defined with a property called no-map
> which tells the system not to create a memory mapping for them.
> https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/reserved-memory/reserved-memory.yaml#L79
>
> Hence, setting aside these memory regions should take priority and should
> be done first before any memblock allocations are done so that the system
> does not  unknowingly allocate memory from a region that is meant to be
> reserved for a device/driver.
>
> Eg:
>     unflatten_and_copy_device_tree() eventually calls memblock_alloc():
>     https://elixir.bootlin.com/linux/latest/source/drivers/of/fdt.c#L1264
>
>     Since unflatten_and_copy_device_tree() is called in platform_init(), this
>     allocation is done before we are able to set aside any of the reserved
>     memory regions from the DT which is supposed to be done by
>     early_init_fdt_scan_reserved_mem() in the arch_mem_init() function.
>
>     Hence, it is possible for unflatten_and_copy_device_tree() to allocate
>     memory from a region that is meant to be set aside for a device/driver
>     without the system knowing.
>
> This can create problems for a device/driver if a region of memory that was
> supposed to be set aside for it ends up being allocated for another use case
> by memblock_alloc*().
OK, then I think the best solution is move
early_init_fdt_scan_reserved_mem() to before
unflatten_and_copy_device_tree() in platform_init().

Huacai

>
> Regards,
>
> Oreoluwa