Re: [PATCH 02/14] MIPS: memblock: Surely map BSS kernel memory section

From: Matt Redfearn
Date: Tue Jan 23 2018 - 06:05:34 EST


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.

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();