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

[Huacai Chen]

Hi, Oreoluwa,

On Thu, Feb 15, 2024 at 5:31 AM Oreoluwa Babatunde
<quic_obabatun@xxxxxxxxxxx> 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@xxxxxxxxxxx> 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@xxxxxxxxxxx>
> >> ---
> >>  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