[PATCH RFC] MIPS: Remove detect_memory_region()

[PATCH RFC] MIPS: Remove detect_memory_region()

[Jinyang He]

detect_memory_region() was committed by Commit 4d9f77d25268 ("MIPS: add
detect_memory_region()"). Then it was equipped by Commit dd63b00804a5
("MIPS: ralink: make use of the new memory detection code") and
Commit 9b75733b7b5e ("MIPS: ath79: make use of the new memory detection
code"). Its code is based on early ath79 platform code.

What puzzles me is that how memcmp() detect the memory region. If `break`
was touched, the function could make sense. That means memcmp() should
return zero. Otherwise, the loop will be end by size > sz_max.

I have tested detect_memory_region() on Loongson64 3A3000. On our design,
kseg0 low 256MB maps real memory and kseg0 high 256MB maps IO/PCI. The
function runs and last stopped on kseg1 where is uncached. In this process
memcmp also returned non-zero when detected kseg0 high 256MB. Then I did
another thing. memcpy first and test memcmp then (after &_end). It works
well on 3A3000 but badly on 3A4000. Maybe because kseg0 high 256MB maps
IO/PCI and it is dangerous to write like write memory.

At last, read memory from where is not memory region may always return 0.
(Or trigger exception.) This function have been used several years and
seems no error occur. Maybe it's a fallback way.

Signed-off-by: Jinyang He <hejinyang@loongson.cn>
---
 arch/mips/ath79/setup.c          |  2 +-
 arch/mips/include/asm/bootinfo.h |  2 --
 arch/mips/kernel/setup.c         | 21 ---------------------
 arch/mips/ralink/of.c            |  5 ++---
 4 files changed, 3 insertions(+), 27 deletions(-)

diff --git a/arch/mips/ath79/setup.c b/arch/mips/ath79/setup.c
index 891f495..d2a41fb 100644
--- a/arch/mips/ath79/setup.c
+++ b/arch/mips/ath79/setup.c
@@ -232,7 +232,7 @@ void __init plat_mem_setup(void)
 	ath79_detect_sys_type();
 	ath79_ddr_ctrl_init();
 
-	detect_memory_region(0, ATH79_MEM_SIZE_MIN, ATH79_MEM_SIZE_MAX);
+	memblock_add(0, ATH79_MEM_SIZE_MAX);
 
 	_machine_restart = ath79_restart;
 	_machine_halt = ath79_halt;
diff --git a/arch/mips/include/asm/bootinfo.h b/arch/mips/include/asm/bootinfo.h
index 5be10ece..0d5637e 100644
--- a/arch/mips/include/asm/bootinfo.h
+++ b/arch/mips/include/asm/bootinfo.h
@@ -90,8 +90,6 @@ const char *get_system_type(void);
 
 extern unsigned long mips_machtype;
 
-extern void detect_memory_region(phys_addr_t start, phys_addr_t sz_min,  phys_addr_t sz_max);
-
 extern void prom_init(void);
 extern void prom_free_prom_memory(void);
 extern void prom_cleanup(void);
diff --git a/arch/mips/kernel/setup.c b/arch/mips/kernel/setup.c
index 279be01..fbbc51e 100644
--- a/arch/mips/kernel/setup.c
+++ b/arch/mips/kernel/setup.c
@@ -86,32 +86,11 @@ static struct resource bss_resource = { .name = "Kernel bss", };
 unsigned long __kaslr_offset __ro_after_init;
 EXPORT_SYMBOL(__kaslr_offset);
 
-static void *detect_magic __initdata = detect_memory_region;
-
 #ifdef CONFIG_MIPS_AUTO_PFN_OFFSET
 unsigned long ARCH_PFN_OFFSET;
 EXPORT_SYMBOL(ARCH_PFN_OFFSET);
 #endif
 
-void __init detect_memory_region(phys_addr_t start, phys_addr_t sz_min, phys_addr_t sz_max)
-{
-	void *dm = &detect_magic;
-	phys_addr_t size;
-
-	for (size = sz_min; size < sz_max; size <<= 1) {
-		if (!memcmp(dm, dm + size, sizeof(detect_magic)))
-			break;
-	}
-
-	pr_debug("Memory: %lluMB of RAM detected at 0x%llx (min: %lluMB, max: %lluMB)\n",
-		((unsigned long long) size) / SZ_1M,
-		(unsigned long long) start,
-		((unsigned long long) sz_min) / SZ_1M,
-		((unsigned long long) sz_max) / SZ_1M);
-
-	memblock_add(start, size);
-}
-
 /*
  * Manage initrd
  */
diff --git a/arch/mips/ralink/of.c b/arch/mips/ralink/of.c
index 8286c35..2de0075 100644
--- a/arch/mips/ralink/of.c
+++ b/arch/mips/ralink/of.c
@@ -81,9 +81,8 @@ void __init plat_mem_setup(void)
 	else if (soc_info.mem_size)
 		memblock_add(soc_info.mem_base, soc_info.mem_size * SZ_1M);
 	else
-		detect_memory_region(soc_info.mem_base,
-				     soc_info.mem_size_min * SZ_1M,
-				     soc_info.mem_size_max * SZ_1M);
+		memblock_add(soc_info.mem_base,
+			     soc_info.mem_size_max * SZ_1M);
 }
 
 static int __init plat_of_setup(void)
-- 
2.1.0

Re: [PATCH RFC] MIPS: Remove detect_memory_region()

[Jiaxun Yang]

On Wed, Feb 24, 2021, at 9:02 PM, Jinyang He wrote:
> detect_memory_region() was committed by Commit 4d9f77d25268 ("MIPS: add
> detect_memory_region()"). Then it was equipped by Commit dd63b00804a5
> ("MIPS: ralink: make use of the new memory detection code") and
> Commit 9b75733b7b5e ("MIPS: ath79: make use of the new memory detection
> code"). Its code is based on early ath79 platform code.
> 
> What puzzles me is that how memcmp() detect the memory region. If `break`
> was touched, the function could make sense. That means memcmp() should
> return zero. Otherwise, the loop will be end by size > sz_max.
> 
> I have tested detect_memory_region() on Loongson64 3A3000. On our design,
> kseg0 low 256MB maps real memory and kseg0 high 256MB maps IO/PCI. The
> function runs and last stopped on kseg1 where is uncached. In this process
> memcmp also returned non-zero when detected kseg0 high 256MB. Then I did
> another thing. memcpy first and test memcmp then (after &_end). It works
> well on 3A3000 but badly on 3A4000. Maybe because kseg0 high 256MB maps
> IO/PCI and it is dangerous to write like write memory.
> 
> At last, read memory from where is not memory region may always return 0.
> (Or trigger exception.) This function have been used several years and
> seems no error occur. Maybe it's a fallback way.

That is not true for other platforms like ath79 or mtk.
They'll wrap around or return 0xffffffff for out of boundary accessing.

Loongson does not apply to this case as it have special "Address Window"
design to accurately describe address regions.
Any access beyond described windows will be handled by MC and return 0 or random stuff.

Again, please don't make changes because you can.

Thanks.

- Jiaxun

Re: [PATCH RFC] MIPS: Remove detect_memory_region()

[Jinyang He]

On 02/24/2021 11:40 PM, Jiaxun Yang wrote:

>
> On Wed, Feb 24, 2021, at 9:02 PM, Jinyang He wrote:
>> detect_memory_region() was committed by Commit 4d9f77d25268 ("MIPS: add
>> detect_memory_region()"). Then it was equipped by Commit dd63b00804a5
>> ("MIPS: ralink: make use of the new memory detection code") and
>> Commit 9b75733b7b5e ("MIPS: ath79: make use of the new memory detection
>> code"). Its code is based on early ath79 platform code.
>>
>> What puzzles me is that how memcmp() detect the memory region. If `break`
>> was touched, the function could make sense. That means memcmp() should
>> return zero. Otherwise, the loop will be end by size > sz_max.
>>
>> I have tested detect_memory_region() on Loongson64 3A3000. On our design,
>> kseg0 low 256MB maps real memory and kseg0 high 256MB maps IO/PCI. The
>> function runs and last stopped on kseg1 where is uncached. In this process
>> memcmp also returned non-zero when detected kseg0 high 256MB. Then I did
>> another thing. memcpy first and test memcmp then (after &_end). It works
>> well on 3A3000 but badly on 3A4000. Maybe because kseg0 high 256MB maps
>> IO/PCI and it is dangerous to write like write memory.
>>
>> At last, read memory from where is not memory region may always return 0.
>> (Or trigger exception.) This function have been used several years and
>> seems no error occur. Maybe it's a fallback way.
> That is not true for other platforms like ath79 or mtk.
> They'll wrap around or return 0xffffffff for out of boundary accessing.
>
> Loongson does not apply to this case as it have special "Address Window"
> design to accurately describe address regions.
> Any access beyond described windows will be handled by MC and return 0 or random stuff.
>
> Again, please don't make changes because you can.
>
> Thanks.
>
> - Jiaxun

Hi, Jiaxun,

Thank you for answering this puzzle for me in detail.

Assume that the machine has 8MB real memory and dm address is (base + 3M).
When size = 8MB, there will be a phenomenon of `wrap around`, the actual
content of (dm + 8M + 3M) is content of (dm + 3M), so it will trigger
`break`, right? At this time, the kernel will add 8M to the memory.

Thanks,
Jinyang

Re: [PATCH RFC] MIPS: Remove detect_memory_region()

[Jiaxun Yang]

在 2021/2/26 上午9:37, Jinyang He 写道:
> On 02/24/2021 11:40 PM, Jiaxun Yang wrote:
>
>>
>> On Wed, Feb 24, 2021, at 9:02 PM, Jinyang He wrote:
>>> detect_memory_region() was committed by Commit 4d9f77d25268 ("MIPS: add
>>> detect_memory_region()"). Then it was equipped by Commit dd63b00804a5
>>> ("MIPS: ralink: make use of the new memory detection code") and
>>> Commit 9b75733b7b5e ("MIPS: ath79: make use of the new memory detection
>>> code"). Its code is based on early ath79 platform code.
>>>
>>> What puzzles me is that how memcmp() detect the memory region. If 
>>> `break`
>>> was touched, the function could make sense. That means memcmp() should
>>> return zero. Otherwise, the loop will be end by size > sz_max.
>>>
>>> I have tested detect_memory_region() on Loongson64 3A3000. On our 
>>> design,
>>> kseg0 low 256MB maps real memory and kseg0 high 256MB maps IO/PCI. The
>>> function runs and last stopped on kseg1 where is uncached. In this 
>>> process
>>> memcmp also returned non-zero when detected kseg0 high 256MB. Then I 
>>> did
>>> another thing. memcpy first and test memcmp then (after &_end). It 
>>> works
>>> well on 3A3000 but badly on 3A4000. Maybe because kseg0 high 256MB maps
>>> IO/PCI and it is dangerous to write like write memory.
>>>
>>> At last, read memory from where is not memory region may always 
>>> return 0.
>>> (Or trigger exception.) This function have been used several years and
>>> seems no error occur. Maybe it's a fallback way.
>> That is not true for other platforms like ath79 or mtk.
>> They'll wrap around or return 0xffffffff for out of boundary accessing.
>>
>> Loongson does not apply to this case as it have special "Address Window"
>> design to accurately describe address regions.
>> Any access beyond described windows will be handled by MC and return 
>> 0 or random stuff.
>>
>> Again, please don't make changes because you can.
>>
>> Thanks.
>>
>> - Jiaxun
>
> Hi, Jiaxun,
>
> Thank you for answering this puzzle for me in detail.
>
> Assume that the machine has 8MB real memory and dm address is (base + 
> 3M).
> When size = 8MB, there will be a phenomenon of `wrap around`, the actual
> content of (dm + 8M + 3M) is content of (dm + 3M), so it will trigger
> `break`, right? At this time, the kernel will add 8M to the memory.

Hi Jingyang,

How can you boot kernel with 8M memory in present days ;-)
(Ohh with respect to Nintendo64 developer who had proven it's possible)

For what I can say, detect_memory_region exists because many devices
doesn't have a way to pass memory size information from bootloader to
kernel. Or their bootloader even don't care about memory size.

Kernel needs it to get memory size correctly. Although it seems fragile.

That's life, we must accept imperfect past and don't repeat it in future.

Thanks.

- Jiaxun


>
> Thanks,
> Jinyang
>

Re: [PATCH RFC] MIPS: Remove detect_memory_region()

[Jinyang He]

On 02/26/2021 02:52 PM, Jiaxun Yang wrote:

> 在 2021/2/26 上午9:37, Jinyang He 写道:
>> On 02/24/2021 11:40 PM, Jiaxun Yang wrote:
>>
>>>
>>> On Wed, Feb 24, 2021, at 9:02 PM, Jinyang He wrote:
>>>> detect_memory_region() was committed by Commit 4d9f77d25268 ("MIPS: 
>>>> add
>>>> detect_memory_region()"). Then it was equipped by Commit dd63b00804a5
>>>> ("MIPS: ralink: make use of the new memory detection code") and
>>>> Commit 9b75733b7b5e ("MIPS: ath79: make use of the new memory 
>>>> detection
>>>> code"). Its code is based on early ath79 platform code.
>>>>
>>>> What puzzles me is that how memcmp() detect the memory region. If 
>>>> `break`
>>>> was touched, the function could make sense. That means memcmp() should
>>>> return zero. Otherwise, the loop will be end by size > sz_max.
>>>>
>>>> I have tested detect_memory_region() on Loongson64 3A3000. On our 
>>>> design,
>>>> kseg0 low 256MB maps real memory and kseg0 high 256MB maps IO/PCI. The
>>>> function runs and last stopped on kseg1 where is uncached. In this 
>>>> process
>>>> memcmp also returned non-zero when detected kseg0 high 256MB. Then 
>>>> I did
>>>> another thing. memcpy first and test memcmp then (after &_end). It 
>>>> works
>>>> well on 3A3000 but badly on 3A4000. Maybe because kseg0 high 256MB 
>>>> maps
>>>> IO/PCI and it is dangerous to write like write memory.
>>>>
>>>> At last, read memory from where is not memory region may always 
>>>> return 0.
>>>> (Or trigger exception.) This function have been used several years and
>>>> seems no error occur. Maybe it's a fallback way.
>>> That is not true for other platforms like ath79 or mtk.
>>> They'll wrap around or return 0xffffffff for out of boundary accessing.
>>>
>>> Loongson does not apply to this case as it have special "Address 
>>> Window"
>>> design to accurately describe address regions.
>>> Any access beyond described windows will be handled by MC and return 
>>> 0 or random stuff.
>>>
>>> Again, please don't make changes because you can.
>>>
>>> Thanks.
>>>
>>> - Jiaxun
>>
>> Hi, Jiaxun,
>>
>> Thank you for answering this puzzle for me in detail.
>>
>> Assume that the machine has 8MB real memory and dm address is (base + 
>> 3M).
>> When size = 8MB, there will be a phenomenon of `wrap around`, the actual
>> content of (dm + 8M + 3M) is content of (dm + 3M), so it will trigger
>> `break`, right? At this time, the kernel will add 8M to the memory.
>
> Hi Jingyang,
>
> How can you boot kernel with 8M memory in present days ;-)
> (Ohh with respect to Nintendo64 developer who had proven it's possible)
>
> For what I can say, detect_memory_region exists because many devices
> doesn't have a way to pass memory size information from bootloader to
> kernel. Or their bootloader even don't care about memory size.
>
> Kernel needs it to get memory size correctly. Although it seems fragile.
>
> That's life, we must accept imperfect past and don't repeat it in future.
>
> Thanks.
>
> - Jiaxun
>
>
That's just a assume. Because it looks fresh to me.
Thank you very much. :-D

>>
>> Thanks,
>> Jinyang
>>