| Message ID | 1707524971-146908-2-git-send-email-quic_obabatun@quicinc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Restructure init sequence to set aside reserved memory earlier | expand |
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
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
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(-) --