[PATCH 0/3] MIPS: Fix some issues about arch_mem_init()

[PATCH 0/3] MIPS: Fix some issues about arch_mem_init()

[Tiezhu Yang]

Tiezhu Yang (3):
  MIPS: Do not initialise globals to 0
  MIPS: Cleanup code about plat_mem_setup()
  MIPS: Reduce possibility of kernel panic under CONFIG_SWIOTLB

 arch/mips/include/asm/bootinfo.h |  2 +-
 arch/mips/kernel/setup.c         | 14 +++++++++++---
 2 files changed, 12 insertions(+), 4 deletions(-)

-- 
2.1.0

[PATCH 1/3] MIPS: Do not initialise globals to 0

[Tiezhu Yang]

Fix the following checkpatch error:

ERROR: do not initialise globals to 0
#834: FILE: arch/mips/kernel/setup.c:834:
+int hw_coherentio = 0;	/* Actual hardware supported DMA coherency setting. */

Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
---
 arch/mips/kernel/setup.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/mips/kernel/setup.c b/arch/mips/kernel/setup.c
index 10bef8f..b1e2d43 100644
--- a/arch/mips/kernel/setup.c
+++ b/arch/mips/kernel/setup.c
@@ -831,7 +831,7 @@ arch_initcall(debugfs_mips);
 /* User defined DMA coherency from command line. */
 enum coherent_io_user_state coherentio = IO_COHERENCE_DEFAULT;
 EXPORT_SYMBOL_GPL(coherentio);
-int hw_coherentio = 0;	/* Actual hardware supported DMA coherency setting. */
+int hw_coherentio;	/* Actual hardware supported DMA coherency setting. */
 
 static int __init setcoherentio(char *str)
 {
-- 
2.1.0

[PATCH 2/3] MIPS: Cleanup code about plat_mem_setup()

[Tiezhu Yang]

In the current code, plat_mem_setup() is called by arch_mem_init() instead
of setup_arch() and has been declared in asm/bootinfo.h, so modify the code
comment to reflect the reality and remove the useless duplicate declartion
in arch/mips/kernel/setup.c.

Fixes: 2925aba4223f ("[MIPS] Cleanup memory management initialization.")
Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
---
 arch/mips/include/asm/bootinfo.h | 2 +-
 arch/mips/kernel/setup.c         | 2 --
 2 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/arch/mips/include/asm/bootinfo.h b/arch/mips/include/asm/bootinfo.h
index 6172778..dcd6a75 100644
--- a/arch/mips/include/asm/bootinfo.h
+++ b/arch/mips/include/asm/bootinfo.h
@@ -121,7 +121,7 @@ extern unsigned long fw_passed_dtb;
 #endif
 
 /*
- * Platform memory detection hook called by setup_arch
+ * Platform memory detection hook called by arch_mem_init()
  */
 extern void plat_mem_setup(void);
 
diff --git a/arch/mips/kernel/setup.c b/arch/mips/kernel/setup.c
index b1e2d43..5481a0c 100644
--- a/arch/mips/kernel/setup.c
+++ b/arch/mips/kernel/setup.c
@@ -654,8 +654,6 @@ static void __init bootcmdline_init(char **cmdline_p)
  */
 static void __init arch_mem_init(char **cmdline_p)
 {
-	extern void plat_mem_setup(void);
-
 	/* call board setup routine */
 	plat_mem_setup();
 	memblock_set_bottom_up(true);
-- 
2.1.0

[PATCH 3/3] MIPS: Reduce possibility of kernel panic under CONFIG_SWIOTLB

[Tiezhu Yang]

In the current code, if CONFIG_SWIOTLB is set, when failed to get IO TLB
memory from the low pages by plat_swiotlb_setup(), it may lead to the boot
process failed with kernel panic.

(1) On the Loongson and SiByte platform
arch/mips/loongson64/dma.c
arch/mips/sibyte/common/dma.c
void __init plat_swiotlb_setup(void)
{
	swiotlb_init(1);
}

kernel/dma/swiotlb.c
void  __init
swiotlb_init(int verbose)
{
...
	vstart = memblock_alloc_low(PAGE_ALIGN(bytes), PAGE_SIZE);
	if (vstart && !swiotlb_init_with_tbl(vstart, io_tlb_nslabs, verbose))
		return;
...
	pr_warn("Cannot allocate buffer");
	no_iotlb_memory = true;
}

phys_addr_t swiotlb_tbl_map_single()
{
...
	if (no_iotlb_memory)
		panic("Can not allocate SWIOTLB buffer earlier ...");
...
}

(2) On the Cavium OCTEON platform
arch/mips/cavium-octeon/dma-octeon.c
void __init plat_swiotlb_setup(void)
{
...
	octeon_swiotlb = memblock_alloc_low(swiotlbsize, PAGE_SIZE);
	if (!octeon_swiotlb)
		panic("%s: Failed to allocate %zu bytes align=%lx\n",
		      __func__, swiotlbsize, PAGE_SIZE);
...
}

Because IO_TLB_DEFAULT_SIZE is 64M, if the rest size of low memory is less
than 64M when call plat_swiotlb_setup(), we can easily reproduce the panic
case.

In order to reduce the possibility of kernel panic when failed to get IO
TLB memory under CONFIG_SWIOTLB, it is better to allocate low memory as
small as possible before plat_swiotlb_setup(), so make sparse_init() using
top-down allocation.

Reported-by: Juxin Gao <gaojuxin@loongson.cn>
Co-developed-by: Juxin Gao <gaojuxin@loongson.cn>
Signed-off-by: Juxin Gao <gaojuxin@loongson.cn>
Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
---
 arch/mips/kernel/setup.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/mips/kernel/setup.c b/arch/mips/kernel/setup.c
index 5481a0c..8db533c 100644
--- a/arch/mips/kernel/setup.c
+++ b/arch/mips/kernel/setup.c
@@ -700,7 +700,17 @@ static void __init arch_mem_init(char **cmdline_p)
 		memblock_reserve(crashk_res.start, resource_size(&crashk_res));
 #endif
 	device_tree_init();
+
+	/*
+	 * In order to reduce the possibility of kernel panic when failed to
+	 * get IO TLB memory under CONFIG_SWIOTLB, it is better to allocate
+	 * low memory as small as possible before plat_swiotlb_setup(), so
+	 * make sparse_init() using top-down allocation.
+	 */
+	memblock_set_bottom_up(false);
 	sparse_init();
+	memblock_set_bottom_up(true);
+
 	plat_swiotlb_setup();
 
 	dma_contiguous_reserve(PFN_PHYS(max_low_pfn));
-- 
2.1.0

Re: [PATCH 3/3] MIPS: Reduce possibility of kernel panic under CONFIG_SWIOTLB

[Jiaxun Yang]

On Tue, 21 Apr 2020 17:04:29 +0800
Tiezhu Yang <yangtiezhu@loongson.cn> wrote:

> In the current code, if CONFIG_SWIOTLB is set, when failed to get IO
> TLB memory from the low pages by plat_swiotlb_setup(), it may lead to
> the boot process failed with kernel panic.

Hi Tiezhu,

Thanks for you patch.

Firstly, your commit message should be more straight forward. Please
describe what you have changed (e.g. MIPS: Set memblock bottom up)
instead of what you solved.

> 
> (1) On the Loongson and SiByte platform
> arch/mips/loongson64/dma.c
> arch/mips/sibyte/common/dma.c
> void __init plat_swiotlb_setup(void)
> {
> 	swiotlb_init(1);
> }
> 

> kernel/dma/swiotlb.c
> void  __init
> swiotlb_init(int verbose)
> {
> ...
> 	vstart = memblock_alloc_low(PAGE_ALIGN(bytes), PAGE_SIZE);
> 	if (vstart && !swiotlb_init_with_tbl(vstart, io_tlb_nslabs,
> verbose)) return;
> ...
> 	pr_warn("Cannot allocate buffer");
> 	no_iotlb_memory = true;
> }
> 
> phys_addr_t swiotlb_tbl_map_single()
> {
> ...
> 	if (no_iotlb_memory)
> 		panic("Can not allocate SWIOTLB buffer earlier ...");
> ...
> }
> 
> (2) On the Cavium OCTEON platform
> arch/mips/cavium-octeon/dma-octeon.c
> void __init plat_swiotlb_setup(void)
> {
> ...
> 	octeon_swiotlb = memblock_alloc_low(swiotlbsize, PAGE_SIZE);
> 	if (!octeon_swiotlb)
> 		panic("%s: Failed to allocate %zu bytes align=%lx\n",
> 		      __func__, swiotlbsize, PAGE_SIZE);
> ...
> }
> 
> Because IO_TLB_DEFAULT_SIZE is 64M, if the rest size of low memory is
> less than 64M when call plat_swiotlb_setup(), we can easily reproduce
> the panic case.
> 
> In order to reduce the possibility of kernel panic when failed to get
> IO TLB memory under CONFIG_SWIOTLB, it is better to allocate low
> memory as small as possible before plat_swiotlb_setup(), so make
> sparse_init() using top-down allocation.

AFAIK there are some reasons that we set it to bottom_up.
On some platforms, bootloader won't place cmdline & devicetree into
reserved memory but place them just after kernel in memory. That means
if you set it as bottom up, then early allocate memory might collide
with these boot arguments.

I'm not even sure if it works fine on Loongson with early PMON.

I had met that issue before, the solution for me is to reduce SWIOTLB
size.

> 
> Reported-by: Juxin Gao <gaojuxin@loongson.cn>
> Co-developed-by: Juxin Gao <gaojuxin@loongson.cn>
> Signed-off-by: Juxin Gao <gaojuxin@loongson.cn>
> Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
> ---
>  arch/mips/kernel/setup.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/arch/mips/kernel/setup.c b/arch/mips/kernel/setup.c
> index 5481a0c..8db533c 100644
> --- a/arch/mips/kernel/setup.c
> +++ b/arch/mips/kernel/setup.c
> @@ -700,7 +700,17 @@ static void __init arch_mem_init(char
> **cmdline_p) memblock_reserve(crashk_res.start,
> resource_size(&crashk_res)); #endif
>  	device_tree_init();
> +
> +	/*
> +	 * In order to reduce the possibility of kernel panic when
> failed to
> +	 * get IO TLB memory under CONFIG_SWIOTLB, it is better to
> allocate
> +	 * low memory as small as possible before
> plat_swiotlb_setup(), so
> +	 * make sparse_init() using top-down allocation.
> +	 */
> +	memblock_set_bottom_up(false);
>  	sparse_init();
> +	memblock_set_bottom_up(true);
> +
>  	plat_swiotlb_setup();
>  
>  	dma_contiguous_reserve(PFN_PHYS(max_low_pfn));

--
Jiaxun Yang

Re: [PATCH 3/3] MIPS: Reduce possibility of kernel panic under CONFIG_SWIOTLB

[Jiaxun Yang]

On Tue, 21 Apr 2020 17:35:25 +0800
Jiaxun Yang <jiaxun.yang@flygoat.com> wrote:

> On Tue, 21 Apr 2020 17:04:29 +0800
> Tiezhu Yang <yangtiezhu@loongson.cn> wrote:
> 
[...]
> 
> AFAIK there are some reasons that we set it to bottom_up.
> On some platforms, bootloader won't place cmdline & devicetree into
> reserved memory but place them just after kernel in memory. That means
> if you set it as bottom up, then early allocate memory might collide
> with these boot arguments.

Sorry please ignore it.

I had a wrong impression on memory layout.

It should be fine.

Thanks.

> 
> I'm not even sure if it works fine on Loongson with early PMON.
> 
> I had met that issue before, the solution for me is to reduce SWIOTLB
> size.
> 
> > 
> > Reported-by: Juxin Gao <gaojuxin@loongson.cn>
> > Co-developed-by: Juxin Gao <gaojuxin@loongson.cn>
> > Signed-off-by: Juxin Gao <gaojuxin@loongson.cn>
> > Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
> > ---
> >  arch/mips/kernel/setup.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> > 
> > diff --git a/arch/mips/kernel/setup.c b/arch/mips/kernel/setup.c
> > index 5481a0c..8db533c 100644
> > --- a/arch/mips/kernel/setup.c
> > +++ b/arch/mips/kernel/setup.c
> > @@ -700,7 +700,17 @@ static void __init arch_mem_init(char
> > **cmdline_p) memblock_reserve(crashk_res.start,
> > resource_size(&crashk_res)); #endif
> >  	device_tree_init();
> > +
> > +	/*
> > +	 * In order to reduce the possibility of kernel panic when
> > failed to
> > +	 * get IO TLB memory under CONFIG_SWIOTLB, it is better to
> > allocate
> > +	 * low memory as small as possible before
> > plat_swiotlb_setup(), so
> > +	 * make sparse_init() using top-down allocation.
> > +	 */
> > +	memblock_set_bottom_up(false);
> >  	sparse_init();
> > +	memblock_set_bottom_up(true);
> > +
> >  	plat_swiotlb_setup();
> >  
> >  	dma_contiguous_reserve(PFN_PHYS(max_low_pfn));  
> 
> --
> Jiaxun Yang
> 

Re: [PATCH 3/3] MIPS: Reduce possibility of kernel panic under CONFIG_SWIOTLB

[Tiezhu Yang]

On 04/21/2020 05:35 PM, Jiaxun Yang wrote:
> On Tue, 21 Apr 2020 17:04:29 +0800
> Tiezhu Yang <yangtiezhu@loongson.cn> wrote:
>
>> In the current code, if CONFIG_SWIOTLB is set, when failed to get IO
>> TLB memory from the low pages by plat_swiotlb_setup(), it may lead to
>> the boot process failed with kernel panic.
> Hi Tiezhu,
>
> Thanks for you patch.
>
> Firstly, your commit message should be more straight forward. Please
> describe what you have changed (e.g. MIPS: Set memblock bottom up)
> instead of what you solved.

HI Jiaxun,

Thanks for your suggestion.

I will send a v2 patch used with the following subject:
"MIPS: Make sparse_init() using top-down allocation"

>
>> (1) On the Loongson and SiByte platform
>> arch/mips/loongson64/dma.c
>> arch/mips/sibyte/common/dma.c
>> void __init plat_swiotlb_setup(void)
>> {
>> 	swiotlb_init(1);
>> }
>>
>> kernel/dma/swiotlb.c
>> void  __init
>> swiotlb_init(int verbose)
>> {
>> ...
>> 	vstart = memblock_alloc_low(PAGE_ALIGN(bytes), PAGE_SIZE);
>> 	if (vstart && !swiotlb_init_with_tbl(vstart, io_tlb_nslabs,
>> verbose)) return;
>> ...
>> 	pr_warn("Cannot allocate buffer");
>> 	no_iotlb_memory = true;
>> }
>>
>> phys_addr_t swiotlb_tbl_map_single()
>> {
>> ...
>> 	if (no_iotlb_memory)
>> 		panic("Can not allocate SWIOTLB buffer earlier ...");
>> ...
>> }
>>
>> (2) On the Cavium OCTEON platform
>> arch/mips/cavium-octeon/dma-octeon.c
>> void __init plat_swiotlb_setup(void)
>> {
>> ...
>> 	octeon_swiotlb = memblock_alloc_low(swiotlbsize, PAGE_SIZE);
>> 	if (!octeon_swiotlb)
>> 		panic("%s: Failed to allocate %zu bytes align=%lx\n",
>> 		      __func__, swiotlbsize, PAGE_SIZE);
>> ...
>> }
>>
>> Because IO_TLB_DEFAULT_SIZE is 64M, if the rest size of low memory is
>> less than 64M when call plat_swiotlb_setup(), we can easily reproduce
>> the panic case.
>>
>> In order to reduce the possibility of kernel panic when failed to get
>> IO TLB memory under CONFIG_SWIOTLB, it is better to allocate low
>> memory as small as possible before plat_swiotlb_setup(), so make
>> sparse_init() using top-down allocation.
> AFAIK there are some reasons that we set it to bottom_up.
> On some platforms, bootloader won't place cmdline & devicetree into
> reserved memory but place them just after kernel in memory. That means
> if you set it as bottom up, then early allocate memory might collide
> with these boot arguments.
>
> I'm not even sure if it works fine on Loongson with early PMON.
>
> I had met that issue before, the solution for me is to reduce SWIOTLB
> size.
>
>> Reported-by: Juxin Gao <gaojuxin@loongson.cn>
>> Co-developed-by: Juxin Gao <gaojuxin@loongson.cn>
>> Signed-off-by: Juxin Gao <gaojuxin@loongson.cn>
>> Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
>> ---
>>   arch/mips/kernel/setup.c | 10 ++++++++++
>>   1 file changed, 10 insertions(+)
>>
>> diff --git a/arch/mips/kernel/setup.c b/arch/mips/kernel/setup.c
>> index 5481a0c..8db533c 100644
>> --- a/arch/mips/kernel/setup.c
>> +++ b/arch/mips/kernel/setup.c
>> @@ -700,7 +700,17 @@ static void __init arch_mem_init(char
>> **cmdline_p) memblock_reserve(crashk_res.start,
>> resource_size(&crashk_res)); #endif
>>   	device_tree_init();
>> +
>> +	/*
>> +	 * In order to reduce the possibility of kernel panic when
>> failed to
>> +	 * get IO TLB memory under CONFIG_SWIOTLB, it is better to
>> allocate
>> +	 * low memory as small as possible before
>> plat_swiotlb_setup(), so
>> +	 * make sparse_init() using top-down allocation.
>> +	 */
>> +	memblock_set_bottom_up(false);
>>   	sparse_init();
>> +	memblock_set_bottom_up(true);
>> +
>>   	plat_swiotlb_setup();
>>   
>>   	dma_contiguous_reserve(PFN_PHYS(max_low_pfn));
> --
> Jiaxun Yang