[PATCH 0/3] Restructure init sequence to set aside reserved memory earlier

[PATCH 0/3] Restructure init sequence to set aside reserved memory earlier

[Oreoluwa Babatunde]

The loongarch, openric, and sh architectures allocate memory from
memblock before it gets the chance to set aside reserved memory regions.
This means that there is a possibility for memblock to allocate from
memory regions that are supposed to be reserved.

This series makes changes to the arch specific setup code to call the
functions responsible for setting aside the reserved memory regions earlier
in the init sequence.
Hence, by the time memblock starts being used to allocate memory, the
reserved memory regions should already be set aside, and it will no
longer be possible for allocations to come from them.

I am currnetly using an arm64 device, and so I will need assistance from
the relevant arch maintainers to help check if this breaks anything from
compilation to device bootup.

Oreoluwa Babatunde (3):
  loongarch: Call arch_mem_init() before platform_init() in the init
    sequence
  openrisc: Call setup_memory() earlier in the init sequence
  sh: Call paging_init() earlier in the init sequence

 arch/loongarch/kernel/setup.c | 2 +-
 arch/openrisc/kernel/setup.c  | 6 +++---
 arch/sh/kernel/setup.c        | 4 ++--
 3 files changed, 6 insertions(+), 6 deletions(-)

-- 

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

[Oreoluwa Babatunde]

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();
 
 	resource_init();
 #ifdef CONFIG_SMP
-- 

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

[Huacai Chen]

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

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

[Oreoluwa Babatunde]

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

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@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

[PATCH 2/3] openrisc: Call setup_memory() earlier in the init sequence

[Oreoluwa Babatunde]

The unflatten_and_copy_device_tree() function contains a call to
memblock_alloc(). This means that memblock is allocating memory before
any of the reserved memory regions are set aside in the setup_memory()
function which calls early_init_fdt_scan_reserved_mem(). Therefore,
there is a possibility for memblock to allocate from any of the
reserved memory regions.

Hence, move the call to setup_memory() to be earlier in the init
sequence so that the reserved memory regions are set aside before any
allocations are done using memblock.

Signed-off-by: Oreoluwa Babatunde <quic_obabatun@quicinc.com>
---
 arch/openrisc/kernel/setup.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/openrisc/kernel/setup.c b/arch/openrisc/kernel/setup.c
index 9cf7fb6..be56eaa 100644
--- a/arch/openrisc/kernel/setup.c
+++ b/arch/openrisc/kernel/setup.c
@@ -255,6 +255,9 @@ void calibrate_delay(void)
 
 void __init setup_arch(char **cmdline_p)
 {
+	/* setup memblock allocator */
+	setup_memory();
+
 	unflatten_and_copy_device_tree();
 
 	setup_cpuinfo();
@@ -278,9 +281,6 @@ void __init setup_arch(char **cmdline_p)
 	}
 #endif
 
-	/* setup memblock allocator */
-	setup_memory();
-
 	/* paging_init() sets up the MMU and marks all pages as reserved */
 	paging_init();
 
-- 

Re: [PATCH 2/3] openrisc: Call setup_memory() earlier in the init sequence

[Stafford Horne]

On Fri, Feb 09, 2024 at 04:29:30PM -0800, Oreoluwa Babatunde wrote:
> The unflatten_and_copy_device_tree() function contains a call to
> memblock_alloc(). This means that memblock is allocating memory before
> any of the reserved memory regions are set aside in the setup_memory()
> function which calls early_init_fdt_scan_reserved_mem(). Therefore,
> there is a possibility for memblock to allocate from any of the
> reserved memory regions.
> 
> Hence, move the call to setup_memory() to be earlier in the init
> sequence so that the reserved memory regions are set aside before any
> allocations are done using memblock.
> 
> Signed-off-by: Oreoluwa Babatunde <quic_obabatun@quicinc.com>
> ---
>  arch/openrisc/kernel/setup.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/openrisc/kernel/setup.c b/arch/openrisc/kernel/setup.c
> index 9cf7fb6..be56eaa 100644
> --- a/arch/openrisc/kernel/setup.c
> +++ b/arch/openrisc/kernel/setup.c
> @@ -255,6 +255,9 @@ void calibrate_delay(void)
>  
>  void __init setup_arch(char **cmdline_p)
>  {
> +	/* setup memblock allocator */
> +	setup_memory();
> +
>  	unflatten_and_copy_device_tree();
>  
>  	setup_cpuinfo();
> @@ -278,9 +281,6 @@ void __init setup_arch(char **cmdline_p)
>  	}
>  #endif
>  
> -	/* setup memblock allocator */
> -	setup_memory();
> -
>  	/* paging_init() sets up the MMU and marks all pages as reserved */
>  	paging_init();

This looks good.  I will merge it via the openrisc queue as you requested.

-Stafford

[PATCH 3/3] sh: Call paging_init() earlier in the init sequence

[Oreoluwa Babatunde]

The unflatten_device_tree() function contains a call to
memblock_alloc(). This is a problem because this allocation is done
before any of the reserved memory is set aside in paging_init().
This means that there is a possibility for memblock to allocate from
any of the memory regions that are supposed to be set aside as reserved.

Hence, move the call to paging_init() to be earlier in the init
sequence so that the reserved memory regions are set aside before any
allocations are done using memblock.

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

diff --git a/arch/sh/kernel/setup.c b/arch/sh/kernel/setup.c
index d3175f0..ea40798 100644
--- a/arch/sh/kernel/setup.c
+++ b/arch/sh/kernel/setup.c
@@ -322,6 +322,8 @@ void __init setup_arch(char **cmdline_p)
 	/* Let earlyprintk output early console messages */
 	sh_early_platform_driver_probe("earlyprintk", 1, 1);
 
+	paging_init();
+
 #ifdef CONFIG_OF_EARLY_FLATTREE
 #ifdef CONFIG_USE_BUILTIN_DTB
 	unflatten_and_copy_device_tree();
@@ -330,8 +332,6 @@ void __init setup_arch(char **cmdline_p)
 #endif
 #endif
 
-	paging_init();
-
 	/* Perform the machine specific initialisation */
 	if (likely(sh_mv.mv_setup))
 		sh_mv.mv_setup(cmdline_p);
-- 

Re: [PATCH 3/3] sh: Call paging_init() earlier in the init sequence

[Rob Herring]

On Fri, Feb 9, 2024 at 5:29 PM Oreoluwa Babatunde
<quic_obabatun@quicinc.com> wrote:
>
> The unflatten_device_tree() function contains a call to
> memblock_alloc(). This is a problem because this allocation is done
> before any of the reserved memory is set aside in paging_init().
> This means that there is a possibility for memblock to allocate from
> any of the memory regions that are supposed to be set aside as reserved.
>
> Hence, move the call to paging_init() to be earlier in the init
> sequence so that the reserved memory regions are set aside before any
> allocations are done using memblock.
>
> Signed-off-by: Oreoluwa Babatunde <quic_obabatun@quicinc.com>
> ---
>  arch/sh/kernel/setup.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH 0/3] Restructure init sequence to set aside reserved memory earlier

[Oreoluwa Babatunde]

On 2/9/2024 4:29 PM, Oreoluwa Babatunde wrote:
> The loongarch, openric, and sh architectures allocate memory from
> memblock before it gets the chance to set aside reserved memory regions.
> This means that there is a possibility for memblock to allocate from
> memory regions that are supposed to be reserved.
>
> This series makes changes to the arch specific setup code to call the
> functions responsible for setting aside the reserved memory regions earlier
> in the init sequence.
> Hence, by the time memblock starts being used to allocate memory, the
> reserved memory regions should already be set aside, and it will no
> longer be possible for allocations to come from them.
>
> I am currnetly using an arm64 device, and so I will need assistance from
> the relevant arch maintainers to help check if this breaks anything from
> compilation to device bootup.
>
> Oreoluwa Babatunde (3):
>   loongarch: Call arch_mem_init() before platform_init() in the init
>     sequence
>   openrisc: Call setup_memory() earlier in the init sequence
>   sh: Call paging_init() earlier in the init sequence
>
>  arch/loongarch/kernel/setup.c | 2 +-
>  arch/openrisc/kernel/setup.c  | 6 +++---
>  arch/sh/kernel/setup.c        | 4 ++--
>  3 files changed, 6 insertions(+), 6 deletions(-)
Hello,

Loongarch patch has already merged for this, but review is still pending
from openrisc and sh architectures.
Could someone please comment on these?

Regards,

Oreoluwa

Re: [PATCH 0/3] Restructure init sequence to set aside reserved memory earlier

[Stafford Horne]

On Tue, Mar 05, 2024 at 10:59:20AM -0800, Oreoluwa Babatunde wrote:
> 
> On 2/9/2024 4:29 PM, Oreoluwa Babatunde wrote:
> > The loongarch, openric, and sh architectures allocate memory from
> > memblock before it gets the chance to set aside reserved memory regions.
> > This means that there is a possibility for memblock to allocate from
> > memory regions that are supposed to be reserved.
> >
> > This series makes changes to the arch specific setup code to call the
> > functions responsible for setting aside the reserved memory regions earlier
> > in the init sequence.
> > Hence, by the time memblock starts being used to allocate memory, the
> > reserved memory regions should already be set aside, and it will no
> > longer be possible for allocations to come from them.
> >
> > I am currnetly using an arm64 device, and so I will need assistance from
> > the relevant arch maintainers to help check if this breaks anything from
> > compilation to device bootup.
> >
> > Oreoluwa Babatunde (3):
> >   loongarch: Call arch_mem_init() before platform_init() in the init
> >     sequence
> >   openrisc: Call setup_memory() earlier in the init sequence
> >   sh: Call paging_init() earlier in the init sequence
> >
> >  arch/loongarch/kernel/setup.c | 2 +-
> >  arch/openrisc/kernel/setup.c  | 6 +++---
> >  arch/sh/kernel/setup.c        | 4 ++--
> >  3 files changed, 6 insertions(+), 6 deletions(-)
> Hello,
> 
> Loongarch patch has already merged for this, but review is still pending
> from openrisc and sh architectures.
> Could someone please comment on these?

Hello,

The OpenRISC patch looks fine to me.  I will test it out.  Sorry, I thought you
were getting this merged via other means.

-Stafford