Hi Serge,
On 23/01/18 19:27, Serge Semin wrote:
Hello Matt,
On Tue, Jan 23, 2018 at 11:03:27AM +0000, Matt Redfearn <matt.redfearn@xxxxxxxx> wrote:
Hi Serge,
On 22/01/18 21:47, Serge Semin wrote:
Hello Matt,
On Mon, Jan 22, 2018 at 04:35:26PM +0000, Matt Redfearn <matt.redfearn@xxxxxxxx> wrote:
Hi Serge,
On 17/01/18 22:23, Serge Semin wrote:
The current MIPS code makes sure the kernel code/data/init
sections are in the maps, but BSS should also be there.
Quite right - it should. But this was protected against by reserving all
bootmem up to the _end symbol here:
http://elixir.free-electrons.com/linux/v4.15-rc8/source/arch/mips/kernel/setup.c#L388
Which you remove in the next patch in this series. I'm not sure it is worth
Right. Missed that part. The old code just doesn't set the kernel memory free
calling the free_bootmem() method for non-reserved parts below reserved_end.
disentangling the reserved_end stuff from the next patch to make this into a
single logical change of reserving just .bss rather than everything below
_end.
Good point. I'll move this change into the "[PATCH 05/14] MIPS: memblock:
Add reserved memory regions to memblock". It logically belongs to that place.
Since basically by the arch_mem_addpart() calls we reserve all the kernel
Actually I was wrong - it's not this sequence of arch_mem_addpart's that
reserves the kernels memory. At least on DT based systems, it's pretty
likely that these regions will overlap with the system memory already added.
of_scan_flat_dt will look for the memory node and add it via
early_init_dt_add_memory_arch.
These calls to add the kernel text, init and bss detect that they overlap
with the already present system memory, so don't get added, here:
http://elixir.free-electrons.com/linux/v4.15-rc9/source/arch/mips/kernel/setup.c#L759
As such, when we print out the content of boot_mem_map, we only have a
single entry:
[ 0.000000] Determined physical RAM map:
[ 0.000000] memory: 10000000 @ 00000000 (usable)
memory now I'd also merged them into a single call for the range [_text, _end].
What do you think?
I think that this patch makes sense in case the .bss is for some reason not
covered by an existing entry, but I would leave it as a separate patch.
Your [PATCH 05/14] MIPS: memblock: Add reserved memory regions to memblock
is actually self-contained since it replaces reserving all memory up to _end
with the single reservation of the kernel's whole size
+ size = __pa_symbol(&_end) - __pa_symbol(&_text);
+ memblock_reserve(__pa_symbol(&_text), size);
Which I think is definitely an improvement since it is much clearer.
Alright lets sum it up. First of all, yeah, you are right, arch_mem_addpart()
is created to make sure the kernel memory is added to the memblock/bootmem pool.
The previous arch code was leaving such the memory range non-freed since it was
higher the reserved_end, so to make sure the early memory allocations wouldn't
be made from the pages, where kernel actually resides.
In my code I still wanted to make sure the kernel memory is in the memblock pool.
But I also noticed, that .bss memory range wouldn't be added to the pool if neither
dts nor platform-specific code added any memory to the boot_mem_map pool. So I
decided to fix it. The actual kernel memory reservation is performed after all
the memory regions are declared by the code you cited. It's essential to do
the [_text, _end] memory range reservation there, otherwise memblock may
allocate from the memory range occupied by the kernel code/data.
Since you agree with leaving it in the separate patch, I'd only suggest to
call the arch_mem_addpart() method for just one range [_text, _end] instead of
doing it three times for a separate _text, _data and bss sections. What do you
think?
I think it's best left as 3 separate reservations, mainly due to the
different attribute used for the init section. So all in all, I like
this patch as it is.
Thanks,
Matt
Regards,
-Sergey
Thanks,
Matt
Regards,
-Sergey
Reviewed-by: Matt Redfearn <matt.redfearn@xxxxxxxx>
Thanks,
Matt
Signed-off-by: Serge Semin <fancer.lancer@xxxxxxxxx>
---
arch/mips/kernel/setup.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/arch/mips/kernel/setup.c b/arch/mips/kernel/setup.c
index 76e9e2075..0d21c9e04 100644
--- a/arch/mips/kernel/setup.c
+++ b/arch/mips/kernel/setup.c
@@ -845,6 +845,9 @@ static void __init arch_mem_init(char **cmdline_p)
arch_mem_addpart(PFN_UP(__pa_symbol(&__init_begin)) << PAGE_SHIFT,
PFN_DOWN(__pa_symbol(&__init_end)) << PAGE_SHIFT,
BOOT_MEM_INIT_RAM);
+ arch_mem_addpart(PFN_DOWN(__pa_symbol(&__bss_start)) << PAGE_SHIFT,
+ PFN_UP(__pa_symbol(&__bss_stop)) << PAGE_SHIFT,
+ BOOT_MEM_RAM);
pr_info("Determined physical RAM map:\n");
print_memory_map();
