MIPS: mm: Fix some memory-related issues

dmi_early_remap() has been defined as ioremap_cache() which on MIPS32 gets to be converted to the VM-based mapping. DMI early remapping is performed at the setup_arch() stage with no VM available. So calling the dmi_early_remap() for MIPS32 causes the system to crash at the early boot time. Fix that by converting dmi_early_remap() to the uncached remapping which is always available on both 32 and 64-bits MIPS systems.

Fixes be8fa1cb444c ("MIPS: Add support for Desktop Management Interface (DMI)")

Signed-off-by Serge Semin <[email protected]>

---
 arch/mips/include/asm/dmi.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

arch/mips/include/asm/dmi.h +0 -0

index 27415a288adf..525aad1572d1 100644

Quoted 18 lines · Serge Semin

dmi_early_remap() has been defined as ioremap_cache() which on MIPS32 gets to be converted to the VM-based mapping. DMI early remapping is performed at the setup_arch() stage with no VM available. So calling the dmi_early_remap() for MIPS32 causes the system to crash at the early boot time. Fix that by converting dmi_early_remap() to the uncached remapping which is always available on both 32 and 64-bits MIPS systems.

Fixes be8fa1cb444c ("MIPS: Add support for Desktop Management Interface (DMI)")

Signed-off-by Serge Semin <[email protected]>

---
 arch/mips/include/asm/dmi.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

arch/mips/include/asm/dmi.h +0 -0

index 27415a288adf..525aad1572d1 100644

arch/mips/include/asm/dmi.h line 8

Quoted 5 lines

arch/mips/include/asm/dmi.h +1 -1

@@ -5,7 +5,7 @@
 #include <linux/io.h>
 #include <linux/memblock.h>
 
-#define dmi_early_remap(x, l)		ioremap_cache(x, l)
+#define dmi_early_remap(x, l)		ioremap_uc(x, l)

Please don't use ioremap_uc() in new code, we are in the (long) process of removing it from the kernel for everything except x86-32, and it already returns NULL on most of them.

Would the normal ioremap() work for you here? It seems to do the same thing as ioremap_uc() on mips and a couple of other architectures that have not yet killed it off.

Arnd

Hi Arnd

Quoted 30 lines · Arnd Bergmann

Quoted 18 lines · Serge Semin

dmi_early_remap() has been defined as ioremap_cache() which on MIPS32 gets to be converted to the VM-based mapping. DMI early remapping is performed at the setup_arch() stage with no VM available. So calling the dmi_early_remap() for MIPS32 causes the system to crash at the early boot time. Fix that by converting dmi_early_remap() to the uncached remapping which is always available on both 32 and 64-bits MIPS systems.

Fixes be8fa1cb444c ("MIPS: Add support for Desktop Management Interface (DMI)")

Signed-off-by Serge Semin <[email protected]>

---
 arch/mips/include/asm/dmi.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

arch/mips/include/asm/dmi.h +0 -0

index 27415a288adf..525aad1572d1 100644

arch/mips/include/asm/dmi.h line 8

Quoted 5 lines

arch/mips/include/asm/dmi.h +1 -1

@@ -5,7 +5,7 @@
 #include <linux/io.h>
 #include <linux/memblock.h>
 
-#define dmi_early_remap(x, l)		ioremap_cache(x, l)
+#define dmi_early_remap(x, l)		ioremap_uc(x, l)

Please don't use ioremap_uc() in new code, we are in the (long) process of removing it from the kernel for everything except x86-32, and it already returns NULL on most of them.

Quoted 3 lines

Would the normal ioremap() work for you here? It seems to do the same thing as ioremap_uc() on mips and a couple of other architectures that have not yet killed it off.

Ok. Thanks for the heads up. I'll fix the patch to be using ioremap() in v2. ioremap_uc() is just an macro-alias of ioremap() on MIPS.

-Serge(y)

Quoted 2 lines

Arnd

Quoted 2 lines

Ok. Thanks for the heads up. I'll fix the patch to be using ioremap() in v2. ioremap_uc() is just an macro-alias of ioremap() on MIPS.

Perhaps we need to fix ioremap_cache so it can give a KSEG1 address?

AFAIK for Loongson DMI is located at cached memory so using ioremap_uc blindly will cause inconsistency.

Quoted 4 lines · Jiaxun Yang

Quoted 2 lines

Ok. Thanks for the heads up. I'll fix the patch to be using ioremap() in v2. ioremap_uc() is just an macro-alias of ioremap() on MIPS.

Perhaps we need to fix ioremap_cache so it can give a KSEG1 address?

KSEG0 ?

Quoted 3 lines

AFAIK for Loongson DMI is located at cached memory so using ioremap_uc blindly will cause inconsistency.

why ?

Thomas.

Signature · 2 lines

Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea.                                                [ RFC1925, 2.3 ]

Quoted 1 line · 在2023年11月23日十一月 下午12:29，Thomas Bogendoerfer写道：

On Thu, Nov 23, 2023 at 12:13:11PM +0000, Jiaxun Yang wrote:

Quoted 6 lines

Quoted 4 lines

Quoted 2 lines

Ok. Thanks for the heads up. I'll fix the patch to be using ioremap() in v2. ioremap_uc() is just an macro-alias of ioremap() on MIPS.

Perhaps we need to fix ioremap_cache so it can give a KSEG1 address?

KSEG0 ?

Ah yes it's KSEG0.

Quoted 4 lines

Quoted 2 lines

AFAIK for Loongson DMI is located at cached memory so using ioremap_uc blindly will cause inconsistency.

why ?

Firmware sometimes does not flush those tables from cache back to memory. For Loongson systems (as well as most MTI systems) cache is enabled by firmware.

Quoted 1 line · Jiaxun Yang

the problem with all 32bit unmapped segments is their limitations in size. But there is always room to try to use unmapped and fall back to mapped, if it doesn't work. But I doubt anybody is going to implement that.

Quoted 9 lines

Quoted 5 lines

Quoted 3 lines

AFAIK for Loongson DMI is located at cached memory so using ioremap_uc blindly will cause inconsistency.

why ?

Firmware sometimes does not flush those tables from cache back to memory. For Loongson systems (as well as most MTI systems) cache is enabled by firmware.

kernel flushes all caches on startup, so there shouldn't be a problem.

Thomas.

Signature · 2 lines

Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea.                                                [ RFC1925, 2.3 ]

Quoted 2 lines · 在2023年11月23日十一月 下午4:07，Thomas Bogendoerfer写道：

Quoted 1 line · Jiaxun Yang

[...]

Quoted 5 lines

the problem with all 32bit unmapped segments is their limitations in size. But there is always room to try to use unmapped and fall back to mapped, if it doesn't work. But I doubt anybody is going to implement that.

Yep, I guess fallback should be implemented for ioremap_cache as well.

Quoted 6 lines

Quoted 6 lines

Quoted 5 lines

Quoted 3 lines

AFAIK for Loongson DMI is located at cached memory so using ioremap_uc blindly will cause inconsistency.

why ?

Quoted 5 lines

Quoted 3 lines

Firmware sometimes does not flush those tables from cache back to memory. For Loongson systems (as well as most MTI systems) cache is enabled by firmware.

kernel flushes all caches on startup, so there shouldn't be a problem.

Actually dmi_setup() is called before cpu_cache_init().

Thanks

Quoted 6 lines

Thomas.

Signature · 2 lines

Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea.                                                [ RFC1925, 2.3 ]

Signature · 1 line

- Jiaxun

Quoted 1 line · Jiaxun Yang

Quoted 1 line

Actually dmi_setup() is called before cpu_cache_init().

To preliminary sum the discussion, indeed there can be issues on the platforms which have DMI initialized on the cached region. Here are several solutions and additional difficulties I think may be caused by implementing them:

1. Use unmapped cached region utilization in the MIPS32 ioremap_prot() method.
This solution a bit clumsy than it looks on the first glance. ioremap_prot() can be used for various types of the cachability mapping. Currently it's a default-cacheable CA preserved in the _page_cachable_default variable and Write-combined CA saved in boot_cpu_data.writecombine. Based on that we would have needed to use the unmapped cached region utilized for the IO-remaps called with the "_page_cachable_default" mapping flags passed only. The rest of the IO range mappings, including the write-combined ones, would have been handled by VM means. This would have made the ioremap_prot() a bit less maintainable, but still won't be that hard to implement (unless I miss something):

arch/mips/mm/ioremap.c +8 -5

        /*
-        * Map uncached objects in the low 512mb of address space using KSEG1,
-        * otherwise map using page tables.
+        * Map uncached/default-cached objects in the low 512mb of address
+        * space using KSEG1/KSEG0, otherwise map using page tables.
         */
-       if (IS_LOW512(phys_addr) && IS_LOW512(last_addr) &&
-           flags == _CACHE_UNCACHED)
-               return (void __iomem *) CKSEG1ADDR(phys_addr);
+       if (IS_LOW512(phys_addr) && IS_LOW512(last_addr)) {
+               if (flags == _CACHE_UNCACHED)
+                       return (void __iomem *) CKSEG1ADDR(phys_addr);
+               else if (flags == _page_cachable_default)
+                       return (void __iomem *) CKSEG0ADDR(phys_addr);
+       }

Currently I can't figure out what obvious problems it may cause. But It seems suspicious that the cacheable IO-mapping hasn't been implemented by the unmapped cacheable region in the first place. In anyway this solution looks more dangerous than solution 2. because it affects all the MIPS32 platforms at once.

2. Convert dmi_remap_early() to ioremap_uc() (actually just ioremap() as noted by Arnd).
As Jiaxun correctly noted this may cause problems on the platforms which don't flush caches before jumping out to the kernel. Thomas said that kernel flushes the caches early on boot, but Jiaxun noted that it's still done after early DMI setup. So the issue with solution 2 is that the setup_arch() method calls dmi_setup() before it flushes the caches by means of the cpu_cache_init() method. I guess it can be fixed just by moving the dmi_setup() method invocation to be after the cpu_cache_init() is called. This solution looks much less invasive than solution 1.

So what do you think? What solution do you prefer? Perhaps alternative?

-Serge(y)

Quoted 11 lines

Thanks

Quoted 6 lines

Thomas.

Signature · 2 lines

Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea.                                                [ RFC1925, 2.3 ]

Signature · 1 line

- Jiaxun

Quoted 2 lines · 在2023年11月24日十一月 下午6:52，Serge Semin写道：

Quoted 1 line · Jiaxun Yang

[...]

Quoted 6 lines

Quoted 1 line

Actually dmi_setup() is called before cpu_cache_init().

To preliminary sum the discussion, indeed there can be issues on the platforms which have DMI initialized on the cached region. Here are several solutions and additional difficulties I think may be caused by implementing them:

Thanks for such detailed conclusion!
I'd prefer go solution 1, with comments below.

Quoted 32 lines

1. Use unmapped cached region utilization in the MIPS32 ioremap_prot() method.
This solution a bit clumsy than it looks on the first glance. ioremap_prot() can be used for various types of the cachability mapping. Currently it's a default-cacheable CA preserved in the _page_cachable_default variable and Write-combined CA saved in boot_cpu_data.writecombine. Based on that we would have needed to use the unmapped cached region utilized for the IO-remaps called with the "_page_cachable_default" mapping flags passed only. The rest of the IO range mappings, including the write-combined ones, would have been handled by VM means. This would have made the ioremap_prot() a bit less maintainable, but still won't be that hard to implement (unless I miss something):

arch/mips/mm/ioremap.c +8 -5

        /*
-        * Map uncached objects in the low 512mb of address space using KSEG1,
-        * otherwise map using page tables.
+        * Map uncached/default-cached objects in the low 512mb of address
+        * space using KSEG1/KSEG0, otherwise map using page tables.
         */
-       if (IS_LOW512(phys_addr) && IS_LOW512(last_addr) &&
-           flags == _CACHE_UNCACHED)
-               return (void __iomem *) CKSEG1ADDR(phys_addr);
+       if (IS_LOW512(phys_addr) && IS_LOW512(last_addr)) {
+               if (flags == _CACHE_UNCACHED)
+                       return (void __iomem *) CKSEG1ADDR(phys_addr);
+               else if (flags == _page_cachable_default)
+                       return (void __iomem *) CKSEG0ADDR(phys_addr);
+       }

Quoted 5 lines

Currently I can't figure out what obvious problems it may cause. But It seems suspicious that the cacheable IO-mapping hasn't been implemented by the unmapped cacheable region in the first place. In anyway this solution looks more dangerous than solution 2. because it affects all the MIPS32 platforms at once.

I just made a quick grep in tree, and it seems like we don't have much user of ioremap_cache (as well as ioremap_uc/wc) here so I think it is a safe assumption.

A nip, _page_cachable_default is set in cpu_cache_init() as well. We'd better move it to cpu-probe.c, or give it a reasonable default value.

Quoted 13 lines

2. Convert dmi_remap_early() to ioremap_uc() (actually just ioremap() as noted by Arnd).
As Jiaxun correctly noted this may cause problems on the platforms which don't flush caches before jumping out to the kernel. Thomas said that kernel flushes the caches early on boot, but Jiaxun noted that it's still done after early DMI setup. So the issue with solution 2 is that the setup_arch() method calls dmi_setup() before it flushes the caches by means of the cpu_cache_init() method. I guess it can be fixed just by moving the dmi_setup() method invocation to be after the cpu_cache_init() is called. This solution looks much less invasive than solution 1.

I recall Tiezhu made dmi_setup() here for reasons. The first reason is that DMI is placed at memory space that is not reserved, so it may get clobbered after mm is up.

The second is we may have some early quirks depends on DMI information.

Thanks.

Quoted 17 lines

So what do you think? What solution do you prefer? Perhaps alternative?

-Serge(y)

Quoted 11 lines

Thanks

Quoted 6 lines

Thomas.

Signature · 2 lines

Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea.                                                [ RFC1925, 2.3 ]

Signature · 1 line

- Jiaxun

Signature · 1 line

- Jiaxun

Quoted 3 lines

I just made a quick grep in tree, and it seems like we don't have much user of ioremap_cache (as well as ioremap_uc/wc) here so I think it is a safe assumption.

I wouldn't say there aren't much users. ioremap_wc() and it's devm-version is widely utilized in the GPU and network and some other subsystems. ioremap_cache() isn't widespread indeed. In anyway even a single user must be supported in safely calling the method if it's provided by the arch-code, otherwise the method could be considered as just a bogus stub to have the kernel successfully built. I bet you'll agree with that. But that's not the point in this case,

A bit later you also noted:

Quoted 2 lines · Jiaxun Yang

A nip, _page_cachable_default is set in cpu_cache_init() as well. We'd better move it to cpu-probe.c, or give it a reasonable default value.

Right. Thanks. To be honest I haven't noticed that before your message. _page_cachable_default is indeed initialized in the cpu_cache_init() method, several steps after it would be used in the framework of dmi_remap_early(). On the other hand ioremap_cache() is defined as ioremap_prot() with the _page_cachable_default variable passed. So my code will still correctly work unless _page_cachable_default is pre-initialized with something other than zero. On the other hand we can't easily change its default value because it will affect and likely break the r3k (CPU_R3000) and Octeon based platforms, because it's utilized to initialize the protection-map table. Of course we can fix the r3k_cache_init() and octeon_cache_init() methods too so they would get the _page_cachable_default variable back to zero, but it will also make things around it more complicated.

Also note, moving the _page_cachable_default initialization to the earlier stages like cpu_probe() won't work better because the field value may get change for instance in the framework of the smp_setup() function (see cps_smp_setup()).

So after all the considerations above this solution now looks even clumsier than before.( Any idea how to make it better?

u64 base = (flags == _CACHE_UNCACHED ? IO_BASE : UNCAC_BASE);

Quoted 1 line

Note the memory might be clobbered even before dmi_setup() for instance by means of the early_memtest() method. In anyway it would be better if the system booloader would have already reserved the DMI memory (in DTB) or it would have been done by the platform-specific plat_mem_setup() method.

Quoted 3 lines

The second is we may have some early quirks depends on DMI information.

Which quirks do you mean to be dependent in between the current dmi_setup() call place and the cpu_cache_init() method invocation?

在2023年11月27日十一月 下午4:23，Serge Semin写道：
[...]

Quoted 33 lines

Quoted 3 lines

I just made a quick grep in tree, and it seems like we don't have much user of ioremap_cache (as well as ioremap_uc/wc) here so I think it is a safe assumption.

I wouldn't say there aren't much users. ioremap_wc() and it's devm-version is widely utilized in the GPU and network and some other subsystems. ioremap_cache() isn't widespread indeed. In anyway even a single user must be supported in safely calling the method if it's provided by the arch-code, otherwise the method could be considered as just a bogus stub to have the kernel successfully built. I bet you'll agree with that. But that's not the point in this case,

A bit later you also noted:

Quoted 2 lines · Jiaxun Yang

A nip, _page_cachable_default is set in cpu_cache_init() as well. We'd better move it to cpu-probe.c, or give it a reasonable default value.

Right. Thanks. To be honest I haven't noticed that before your message. _page_cachable_default is indeed initialized in the cpu_cache_init() method, several steps after it would be used in the framework of dmi_remap_early(). On the other hand ioremap_cache() is defined as ioremap_prot() with the _page_cachable_default variable passed. So my code will still correctly work unless _page_cachable_default is pre-initialized with something other than zero. On the other hand we can't easily change its default value because it will affect and likely break the r3k (CPU_R3000) and Octeon based platforms, because it's utilized to initialize the protection-map table. Of course we can fix the r3k_cache_init() and octeon_cache_init() methods too so they would get the _page_cachable_default variable back to zero, but it will also make things around it more complicated.

Quoted 7 lines

Also note, moving the _page_cachable_default initialization to the earlier stages like cpu_probe() won't work better because the field value may get change for instance in the framework of the smp_setup() function (see cps_smp_setup()).

So after all the considerations above this solution now looks even clumsier than before.( Any idea how to make it better?

I think the best solution maybe just use CKSEG0 to setup map here.

Btw I was thinking about 64 bit here, I thought for 64bit we would just embedded prot into XKPHYS, however I quickly figure out ioremap_cache was never implemented properly on 64-bit system, so does ioremap_wc.

Quoted 1 line

u64 base = (flags == _CACHE_UNCACHED ? IO_BASE : UNCAC_BASE);

Which is always uncached mapping.

Quoted 2 lines

Quoted 2 lines

[...]

Quoted 6 lines

Note the memory might be clobbered even before dmi_setup() for instance by means of the early_memtest() method. In anyway it would be better if the system booloader would have already reserved the DMI memory (in DTB) or it would have been done by the platform-specific plat_mem_setup() method.

Unfortunately, too many machines are shipped with those badly designed firmware. We rely on dmi_setup code to scan and preserve dmi table from random location in memory.

Quoted 6 lines

Quoted 3 lines

The second is we may have some early quirks depends on DMI information.

Which quirks do you mean to be dependent in between the current dmi_setup() call place and the cpu_cache_init() method invocation?

I think we don't have any for now.

Signature · 1 line

- Jiaxun

On Mon, Nov 27, 2023 at 09:08:11PM +0000, Jiaxun Yang wrote:

Indeed. Thanks for pointing that out. In the last days several times I was looking at that line and for some reason UNCAC_BASE seemed as CAC_BASE to me.) Based on what both IO_BASE and UNCAC_BASE are defined as of the uncached region anyway, then it should be safe for any currently supported MIPS64 (including the Loongson's) to use ioremap() in place of dmi_early_remap(). So basically my current patch in the subject won't change the method semantics. Let's not to try to fix a problem which doesn't exist then, and keep the patch as is especially seeing that the alternatives might still cause some troubles. Will you be ok with that?

-Serge(y)

Quoted 1 line

Quoted 1 line · 在2023年11月28日十一月 上午11:34，Serge Semin写道：

On Mon, Nov 27, 2023 at 09:08:11PM +0000, Jiaxun Yang wrote:

[...]

Quoted 11 lines

Indeed. Thanks for pointing that out. In the last days several times I was looking at that line and for some reason UNCAC_BASE seemed as CAC_BASE to me.) Based on what both IO_BASE and UNCAC_BASE are defined as of the uncached region anyway, then it should be safe for any currently supported MIPS64 (including the Loongson's) to use ioremap() in place of dmi_early_remap(). So basically my current patch in the subject won't change the method semantics. Let's not to try to fix a problem which doesn't exist then, and keep the patch as is especially seeing that the alternatives might still cause some troubles. Will you be ok with that?

I'd say the safest option is to use CKSEG0 or TO_CAC here,

but I'm fine
with ioremap as long as the semantic remains uncached on Loongson.

Thanks.

Quoted 4 lines

-Serge(y)

Quoted 1 line

[...]

Signature · 1 line

- Jiaxun

On Tue, Nov 28, 2023 at 03:46:37PM +0000, Jiaxun Yang wrote:

Quoted 2 lines

I'd say the safest option is to use CKSEG0 or TO_CAC here,

I would have agreed with you if MIPS didn't have that special _page_cachable_default variable which is undefined for some platforms and which might be re-defined during the boot-up process, and if MIPS64 didn't have ioremap_prot() always mapping to the uncached region. But IMO updating ioremap_prot() currently seems more risky than just converting dmi_early_remap() to the uncached version especially seeing it won't change anything. MIPS64 always have IO remapped to the uncached region. MIPS32 won't be able to have cached mapping until VM is available, and paging and slabs are initialized. So on the early MIPS32 bootup stages ioremap_cache() wouldn't have worked anyway.

Quoted 1 line · 在2023年11月30日十一月 下午7:16，Serge Semin写道：

On Tue, Nov 28, 2023 at 03:46:37PM +0000, Jiaxun Yang wrote:

[...]

Quoted 14 lines

Quoted 2 lines

I'd say the safest option is to use CKSEG0 or TO_CAC here,

I would have agreed with you if MIPS didn't have that special _page_cachable_default variable which is undefined for some platforms and which might be re-defined during the boot-up process, and if MIPS64 didn't have ioremap_prot() always mapping to the uncached region. But IMO updating ioremap_prot() currently seems more risky than just converting dmi_early_remap() to the uncached version especially seeing it won't change anything. MIPS64 always have IO remapped to the uncached region. MIPS32 won't be able to have cached mapping until VM is available, and paging and slabs are initialized. So on the early MIPS32 bootup stages ioremap_cache() wouldn't have worked anyway.

I really didn't get that, using CKSEG0 on 32bit system and TO_CAC on 64bit system won't hurt.

Something like:
#ifdef CONFIG_64BIT
#define dmi_remap(x, l) (void *)TO_CAC(x)
#else
#define dmi_remap(x, l) (void *)CKSEG0(x)
#endif

Can help us avoid all the hassle. Since it always ensures we are using same CCA to access DMI tables. We can always trust Config.K0 left by firmware in this case.

You may add some sanity check on 32 bit to avoid generating invalid pointer. (And perhaps implement it as ioremap_early.....)

Thanks

Signature · 1 line

- Jiaxun

Quoted 31 lines · Jiaxun Yang

Quoted 1 line · 在2023年11月30日十一月 下午7:16，Serge Semin写道：

On Tue, Nov 28, 2023 at 03:46:37PM +0000, Jiaxun Yang wrote:

[...]

Quoted 14 lines

Quoted 2 lines

I'd say the safest option is to use CKSEG0 or TO_CAC here,

I would have agreed with you if MIPS didn't have that special _page_cachable_default variable which is undefined for some platforms and which might be re-defined during the boot-up process, and if MIPS64 didn't have ioremap_prot() always mapping to the uncached region. But IMO updating ioremap_prot() currently seems more risky than just converting dmi_early_remap() to the uncached version especially seeing it won't change anything. MIPS64 always have IO remapped to the uncached region. MIPS32 won't be able to have cached mapping until VM is available, and paging and slabs are initialized. So on the early MIPS32 bootup stages ioremap_cache() wouldn't have worked anyway.

I really didn't get that, using CKSEG0 on 32bit system and TO_CAC on 64bit system won't hurt.

Something like:
#ifdef CONFIG_64BIT
#define dmi_remap(x, l) (void *)TO_CAC(x)
#else
#define dmi_remap(x, l) (void *)CKSEG0(x)
#endif

Quoted 3 lines

Can help us avoid all the hassle. Since it always ensures we are using same CCA to access DMI tables. We can always trust Config.K0 left by firmware in this case.

Please note my only concern is about dmi_early_remap(), not dmi_remap(). The later one can be safely left backended by the ioremap_cache() method because at the stage it's utilized MIPS32 version of ioremap_prot() will be able to create any mapping it's requested to. The dmi_early_remap() function is called very early with no paging or VM or even cache stuff initialized. So currently AFAICS it just doesn't work on _all_ _MIPS32_ platform, because ioremap_prot() relies on VM and slab being available to have any cacheable mapping, which aren't at the moment of the dmi_setup() function invocation. Seeing the ioremap_cache() is just a stub on MIPS64 which always performs the uncached mapping, it will be completely safe to just convert dmi_early_remap() to ioremap() with no risk to beak anything. dmi_early_remap() semantics won't be actually changed, it will work as before on MIPS64 and will be fixed on MIPS32. This (AFAICS) is a completely safe fix of the problem with just a few affected platforms around.

Quoted 36 lines · 在2023年12月1日十二月 下午2:54，Serge Semin写道：

Quoted 31 lines · Jiaxun Yang

Quoted 1 line · 在2023年11月30日十一月 下午7:16，Serge Semin写道：

On Tue, Nov 28, 2023 at 03:46:37PM +0000, Jiaxun Yang wrote:

[...]

Quoted 14 lines

Quoted 2 lines

I'd say the safest option is to use CKSEG0 or TO_CAC here,

I would have agreed with you if MIPS didn't have that special _page_cachable_default variable which is undefined for some platforms and which might be re-defined during the boot-up process, and if MIPS64 didn't have ioremap_prot() always mapping to the uncached region. But IMO updating ioremap_prot() currently seems more risky than just converting dmi_early_remap() to the uncached version especially seeing it won't change anything. MIPS64 always have IO remapped to the uncached region. MIPS32 won't be able to have cached mapping until VM is available, and paging and slabs are initialized. So on the early MIPS32 bootup stages ioremap_cache() wouldn't have worked anyway.

I really didn't get that, using CKSEG0 on 32bit system and TO_CAC on 64bit system won't hurt.

Something like:
#ifdef CONFIG_64BIT
#define dmi_remap(x, l) (void *)TO_CAC(x)
#else
#define dmi_remap(x, l) (void *)CKSEG0(x)
#endif

Quoted 3 lines

Can help us avoid all the hassle. Since it always ensures we are using same CCA to access DMI tables. We can always trust Config.K0 left by firmware in this case.

Quoted 17 lines

Please note my only concern is about dmi_early_remap(), not dmi_remap(). The later one can be safely left backended by the ioremap_cache() method because at the stage it's utilized MIPS32 version of ioremap_prot() will be able to create any mapping it's requested to. The dmi_early_remap() function is called very early with no paging or VM or even cache stuff initialized. So currently AFAICS it just doesn't work on _all_ _MIPS32_ platform, because ioremap_prot() relies on VM and slab being available to have any cacheable mapping, which aren't at the moment of the dmi_setup() function invocation. Seeing the ioremap_cache() is just a stub on MIPS64 which always performs the uncached mapping, it will be completely safe to just convert dmi_early_remap() to ioremap() with no risk to beak anything. dmi_early_remap() semantics won't be actually changed, it will work as before on MIPS64 and will be fixed on MIPS32. This (AFAICS) is a completely safe fix of the problem with just a few affected platforms around.

The only platform enabled DMI in upstream kernel is Loongson64, which I'm perfectly sure that the mapping for DMI tables *should* be Cached.

It is an accident that ioremap_cache is misused here, so I'm proposing to replace it with CKSEG0/TO_CAC. Also as per MIPS UHI spec, all the data passed from bootloader to firmware should lay in KSEG0,

please
let me know if your platform is an exception here.

Using ioremap_cache at dmi_early_remap does not sound safe to me as well. What if DMI code tried to remap something beyond KSEG range at this place?

The safest option here is just bypassing ioremap framework, which does not give you any advantage but only burden.

I'll propose a patch later.

Signature · 1 line

- Jiaxun

Quoted 45 lines · Jiaxun Yang

Quoted 2 lines · 在2023年11月24日十一月 下午6:52，Serge Semin写道：

Quoted 1 line · Jiaxun Yang

[...]

Quoted 6 lines

Quoted 1 line

Actually dmi_setup() is called before cpu_cache_init().

To preliminary sum the discussion, indeed there can be issues on the platforms which have DMI initialized on the cached region. Here are several solutions and additional difficulties I think may be caused by implementing them:

Thanks for such detailed conclusion!
I'd prefer go solution 1, with comments below.

Quoted 32 lines

1. Use unmapped cached region utilization in the MIPS32 ioremap_prot() method.
This solution a bit clumsy than it looks on the first glance. ioremap_prot() can be used for various types of the cachability mapping. Currently it's a default-cacheable CA preserved in the _page_cachable_default variable and Write-combined CA saved in boot_cpu_data.writecombine. Based on that we would have needed to use the unmapped cached region utilized for the IO-remaps called with the "_page_cachable_default" mapping flags passed only. The rest of the IO range mappings, including the write-combined ones, would have been handled by VM means. This would have made the ioremap_prot() a bit less maintainable, but still won't be that hard to implement (unless I miss something):

arch/mips/mm/ioremap.c +8 -5

        /*
-        * Map uncached objects in the low 512mb of address space using KSEG1,
-        * otherwise map using page tables.
+        * Map uncached/default-cached objects in the low 512mb of address
+        * space using KSEG1/KSEG0, otherwise map using page tables.
         */
-       if (IS_LOW512(phys_addr) && IS_LOW512(last_addr) &&
-           flags == _CACHE_UNCACHED)
-               return (void __iomem *) CKSEG1ADDR(phys_addr);
+       if (IS_LOW512(phys_addr) && IS_LOW512(last_addr)) {
+               if (flags == _CACHE_UNCACHED)
+                       return (void __iomem *) CKSEG1ADDR(phys_addr);
+               else if (flags == _page_cachable_default)
+                       return (void __iomem *) CKSEG0ADDR(phys_addr);
+       }

Quoted 9 lines

Quoted 5 lines

Currently I can't figure out what obvious problems it may cause. But It seems suspicious that the cacheable IO-mapping hasn't been implemented by the unmapped cacheable region in the first place. In anyway this solution looks more dangerous than solution 2. because it affects all the MIPS32 platforms at once.

I just made a quick grep in tree, and it seems like we don't have much user of ioremap_cache (as well as ioremap_uc/wc) here so I think it is a safe assumption.

I wouldn't say there aren't much users. ioremap_wc() and it's devm-version is widely utilized in the GPU and network and some other subsystems. ioremap_cache() isn't widespread indeed. In anyway even a single user must be supported in safely calling the method if it's provided by the arch-code, otherwise the method could be considered as just a bogus stub to have the kernel successfully built. I bet you'll agree with that. But that's not the point in this case.

Quoted 56 lines · Serge Semin

Quoted 45 lines · Jiaxun Yang

Quoted 2 lines · 在2023年11月24日十一月 下午6:52，Serge Semin写道：

Quoted 1 line · Jiaxun Yang

[...]

Quoted 6 lines

Quoted 1 line

Actually dmi_setup() is called before cpu_cache_init().

To preliminary sum the discussion, indeed there can be issues on the platforms which have DMI initialized on the cached region. Here are several solutions and additional difficulties I think may be caused by implementing them:

Thanks for such detailed conclusion!
I'd prefer go solution 1, with comments below.

Quoted 32 lines

1. Use unmapped cached region utilization in the MIPS32 ioremap_prot() method.
This solution a bit clumsy than it looks on the first glance. ioremap_prot() can be used for various types of the cachability mapping. Currently it's a default-cacheable CA preserved in the _page_cachable_default variable and Write-combined CA saved in boot_cpu_data.writecombine. Based on that we would have needed to use the unmapped cached region utilized for the IO-remaps called with the "_page_cachable_default" mapping flags passed only. The rest of the IO range mappings, including the write-combined ones, would have been handled by VM means. This would have made the ioremap_prot() a bit less maintainable, but still won't be that hard to implement (unless I miss something):

arch/mips/mm/ioremap.c +8 -5

        /*
-        * Map uncached objects in the low 512mb of address space using KSEG1,
-        * otherwise map using page tables.
+        * Map uncached/default-cached objects in the low 512mb of address
+        * space using KSEG1/KSEG0, otherwise map using page tables.
         */
-       if (IS_LOW512(phys_addr) && IS_LOW512(last_addr) &&
-           flags == _CACHE_UNCACHED)
-               return (void __iomem *) CKSEG1ADDR(phys_addr);
+       if (IS_LOW512(phys_addr) && IS_LOW512(last_addr)) {
+               if (flags == _CACHE_UNCACHED)
+                       return (void __iomem *) CKSEG1ADDR(phys_addr);
+               else if (flags == _page_cachable_default)
+                       return (void __iomem *) CKSEG0ADDR(phys_addr);
+       }

Quoted 9 lines

Quoted 5 lines

Currently I can't figure out what obvious problems it may cause. But It seems suspicious that the cacheable IO-mapping hasn't been implemented by the unmapped cacheable region in the first place. In anyway this solution looks more dangerous than solution 2. because it affects all the MIPS32 platforms at once.

I just made a quick grep in tree, and it seems like we don't have much user of ioremap_cache (as well as ioremap_uc/wc) here so I think it is a safe assumption.

Quoted 7 lines

I wouldn't say there aren't much users. ioremap_wc() and it's devm-version is widely utilized in the GPU and network and some other subsystems. ioremap_cache() isn't widespread indeed. In anyway even a single user must be supported in safely calling the method if it's provided by the arch-code, otherwise the method could be considered as just a bogus stub to have the kernel successfully built. I bet you'll agree with that. But that's not the point in this case.

ioremap_wc() is useful for mapping PCI attached memory such as frame buffers,

but ioremap_cache() is generally underspecified because the resulting pointer is neither safe to dereference nor to pass into readl()/writel()/memcpy_fromio() on all architectures.

There was an effort to convert the remaining ioremap_cache() calls into memremap() a few years ago, not sure if that's still being worked on but it would be the right thing to do.

Arnd

Quoted 5 lines · Arnd Bergmann

Quoted 1 line · Serge Semin

On Fri, Nov 24, 2023 at 10:03:49PM +0000, Jiaxun Yang wrote:

but ioremap_cache() is generally underspecified because the resulting pointer is neither safe to dereference nor to pass into readl()/writel()/memcpy_fromio() on all architectures.

I don't know about ARM64 (which for instance has it utilized to access the DMI region), but at least in case of MIPS32 (a fortiori MIPS64 seeing the ioremap_cache() method actually returns a pointer to the uncached region) I don't see a reason why it wouldn't be safe in both cases described by you. All IO and memory regions are accessed by the generic load and store instructions. The only difference is that the MMIO-space accessors normally implies additional barriers, which just slow down the execution, but shouldn't cause any other problem. Could you clarify why do you think otherwise?

Quoted 3 lines

There was an effort to convert the remaining ioremap_cache() calls into memremap() a few years ago, not sure if that's still being worked on but it would be the right thing to do.

I see. Thanks for the pointing out to that. I guess it could be done for MIPS too (at least on our MIPS32 platform DMI is just a memory region pre-initialized by the bootloader), but the conversion would require much efforts. Alas currently I can't afford to get it implemented in the framework of this patchset. (I saved your note in my MIPS TODO list though. Let's hope eventually I'll be able to get back to this topic.)

Quoted 7 lines · Serge Semin

Quoted 5 lines · Arnd Bergmann

Quoted 1 line · Serge Semin

On Fri, Nov 24, 2023 at 10:03:49PM +0000, Jiaxun Yang wrote:

but ioremap_cache() is generally underspecified because the resulting pointer is neither safe to dereference nor to pass into readl()/writel()/memcpy_fromio() on all architectures.

Quoted 9 lines

I don't know about ARM64 (which for instance has it utilized to access the DMI region), but at least in case of MIPS32 (a fortiori MIPS64 seeing the ioremap_cache() method actually returns a pointer to the uncached region) I don't see a reason why it wouldn't be safe in both cases described by you. All IO and memory regions are accessed by the generic load and store instructions. The only difference is that the MMIO-space accessors normally implies additional barriers, which just slow down the execution, but shouldn't cause any other problem. Could you clarify why do you think otherwise?

On arch/powerpc, CONFIG_PPC_INDIRECT_MMIO makes all ioremap() type functions return a token that can be passed into the readl/writel family but that is not a pointer you can dereference.

On s390, the mechanism is different, but similarly __iomem tokens are not pointers at all.

Quoted 5 lines

Quoted 4 lines

There was an effort to convert the remaining ioremap_cache() calls into memremap() a few years ago, not sure if that's still being worked on but it would be the right thing to do.

Quoted 7 lines

I see. Thanks for the pointing out to that. I guess it could be done for MIPS too (at least on our MIPS32 platform DMI is just a memory region pre-initialized by the bootloader), but the conversion would require much efforts. Alas currently I can't afford to get it implemented in the framework of this patchset. (I saved your note in my MIPS TODO list though. Let's hope eventually I'll be able to get back to this topic.)

I just noticed that the only architectures that actually provide ioremap_cache() are x86, arm, arm64, mips, loongarch, powerpc, sh and xtensa. The ones that have ACPI support still definitely need it, most of the other ones can probably be fixed without too much trouble.

Arnd

Quoted 20 lines · Jiaxun Yang

Quoted 2 lines · 在2023年11月23日十一月 下午4:07，Thomas Bogendoerfer写道：

Quoted 1 line · Jiaxun Yang

[...]

Quoted 5 lines

the problem with all 32bit unmapped segments is their limitations in size. But there is always room to try to use unmapped and fall back to mapped, if it doesn't work. But I doubt anybody is going to implement that.

Yep, I guess fallback should be implemented for ioremap_cache as well.

Quoted 6 lines

Quoted 6 lines

Quoted 5 lines

Quoted 3 lines

AFAIK for Loongson DMI is located at cached memory so using ioremap_uc blindly will cause inconsistency.

why ?

Quoted 7 lines

Quoted 5 lines

Quoted 3 lines

Firmware sometimes does not flush those tables from cache back to memory. For Loongson systems (as well as most MTI systems) cache is enabled by firmware.

kernel flushes all caches on startup, so there shouldn't be a problem.

Actually dmi_setup() is called before cpu_cache_init().

To preliminary sum the discussion, indeed there can be issues on the platforms which have DMI initialized on the cached region. Here are several solutions and additional difficulties I think may be caused by implementing them:

1. Use unmapped cached region utilization in the MIPS32 ioremap_prot() method.
This solution a bit clumsy than it looks on the first glance. ioremap_prot() can be used for various types of the cachability mapping. Currently it's a default-cacheable CA preserved in the _page_cachable_default variable and Write-combined CA saved in boot_cpu_data.writecombine. Based on that we would have needed to use the unmapped cached region utilized for the IO-remaps called with the "_page_cachable_default" mapping flags passed only. The rest of the IO range mappings, including the write-combined ones, would have been handled by VM means. This would have made the ioremap_prot() a bit less maintainable, but still won't be that hard to implement (unless I miss something):

arch/mips/mm/ioremap.c +2 -2

        /*
-        * Map uncached objects in the low 512mb of address space using KSEG1,
-        * otherwise map using page tables.
+        * Map uncached/default-cached objects in the low 512mb of address
+        * space using KSEG1/KSEG0, otherwise map using page tables.

Zone ranges:

  DMA      [mem 0x0000000000000000-0x0000000000ffffff]
  Normal   [mem 0x0000000001000000-0x0000000007ffffff]
  HighMem  [mem 0x0000000008000000-0x000000020fffffff]

Zone ranges:

  DMA      [mem 0x0000000000000000-0x0000000000ffffff]
  Normal   [mem 0x0000000001000000-0x000000001fffffff]
  HighMem  [mem 0x0000000020000000-0x000000020fffffff]

Unhandled kernel unaligned access[#1]:

CPU: 0 PID: 1 Comm: swapper/0 Not tainted 6.7.0-rc1-00307-g0dff108838c9-dirty #2056
...
Call Trace:
[<8011ef9c>] __update_cache+0x88/0x1bc
[<80385944>] ioremap_page_range+0x110/0x2a4
[<80126948>] ioremap_prot+0x17c/0x1f4
[<80711b80>] __devm_ioremap+0x8c/0x120
[<80711e0c>] __devm_ioremap_resource+0xf4/0x218
[<808bf244>] sram_probe+0x4f4/0x930
[<80889d20>] platform_probe+0x68/0xec

...

page:81367080 is uninitialized and poisoned (pfn 8192)

page dumped because: VM_BUG_ON_PAGE(PagePoisoned(p))
Kernel bug detected[#1]:
CPU: 0 PID: 1 Comm: swapper/0 Not tainted 6.7.0-rc1-00298-g88721b1a9ad5-dirty
$ 0   : 00000000 812d0000 00000034 dced7cdf
$ 4   : dced7cdf 00594000 10000000 ffff00fe
$ 8   : 8196bfe0 00000000 00000001 818458c0
$12   : 00000000 00000000 00000000 00000216
$16   : 00002800 81227b80 00000000 00000000
$20   : 00000000 00000000 00000000 00000000
$24   : 0000022b 818458c0
$28   : 81968000 8196be68 00000000 803a0920
Hi    : 00000000
Lo    : 00000000
epc   : 8039d2a4 BUG+0x0/0x4
ra    : 803a0920 post_alloc_hook+0x0/0x128
Status: 10000003 KERNEL EXL IE
Cause : 00800424 (ExcCode 09)
PrId  : 0001a830 (MIPS P5600)
Modules linked in:
Process swapper/0 (pid: 1, threadinfo=81968000, task=819a0000, tls=00000000)
Stack : 00000000 8101ccb0 00000000 8196bd00 00000000 80359768 818a8300 00000001
        81139088 8114438c 8042e4f8 81297a2c 81297a2c 81255e90 819a1b50 dced7cdf
        81297a2c 81297a2c 00000000 81227b80 00000000 81241168 811394b0 00000000
        81140000 80e2cee0 00000000 00000000 00000000 00000000 00000000 819b0000
        81140000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
        ...
Call Trace:
[<8039d2a4>] BUG+0x0/0x4
[<803a0920>] post_alloc_hook+0x0/0x128

Code: 01001025  03e00008  24020001 <000c000d> 2403003c  27bdffd0  afb2001c  3c12812f  8e4269e4

Besides of the already described reasons the pages backended memory holes might be persistent due to having memory mapped IO spaces behind those ranges in the framework of flatmem kernel config. Add such note to the init_unavailable_range() method kdoc in order to point out to one more reason of having the function executed for such regions.

Signed-off-by Serge Semin <[email protected]>

---

Please let me know if the IO-space pages must be initialized somehow differently rather relying on free_area_init() executing the init_unavailable_range() method.

---
 mm/mm_init.c | 1 +
 1 file changed, 1 insertion(+)

mm/mm_init.c +1 -0

index 077bfe393b5e..3fa33e2d32ba 100644
@@ -796,6 +796,7 @@ overlap_memmap_init(unsigned long zone, unsigned long *pfn)
  * - physical memory bank size is not necessarily the exact multiple of the
  *   arbitrary section size
  * - early reserved memory may not be listed in memblock.memory
+ * - memory mapped IO space
  * - memory layouts defined with memmap= kernel parameter may not align
  *   nicely with memmap sections
  *

Signature · 1 line

2.42.1

Hi Serge,

Quoted 10 lines · Serge Semin

Besides of the already described reasons the pages backended memory holes might be persistent due to having memory mapped IO spaces behind those ranges in the framework of flatmem kernel config. Add such note to the init_unavailable_range() method kdoc in order to point out to one more reason of having the function executed for such regions.

Signed-off-by Serge Semin <[email protected]>

---

Quoted 3 lines

Please let me know if the IO-space pages must be initialized somehow differently rather relying on free_area_init() executing the init_unavailable_range() method.

Maybe I'm missing something, but why do you need struct pages in the IO space?

Quoted 20 lines

---
 mm/mm_init.c | 1 +
 1 file changed, 1 insertion(+)

mm/mm_init.c +1 -0

index 077bfe393b5e..3fa33e2d32ba 100644
@@ -796,6 +796,7 @@ overlap_memmap_init(unsigned long zone, unsigned long *pfn)
  * - physical memory bank size is not necessarily the exact multiple of the
  *   arbitrary section size
  * - early reserved memory may not be listed in memblock.memory
+ * - memory mapped IO space
  * - memory layouts defined with memmap= kernel parameter may not align
  *   nicely with memmap sections
  *

Signature · 1 line

2.42.1

Signature · 2 lines

Sincerely yours,
Mike.

Quoted 11 lines · Mike Rapoport

Quoted 10 lines · Serge Semin

Besides of the already described reasons the pages backended memory holes might be persistent due to having memory mapped IO spaces behind those ranges in the framework of flatmem kernel config. Add such note to the init_unavailable_range() method kdoc in order to point out to one more reason of having the function executed for such regions.

Signed-off-by Serge Semin <[email protected]>

---

Quoted 7 lines

Quoted 3 lines

Please let me know if the IO-space pages must be initialized somehow differently rather relying on free_area_init() executing the init_unavailable_range() method.

Maybe I'm missing something, but why do you need struct pages in the IO space?

In my case at the very least that's due to having a SRAM device available in the middle of the MMIO-space. The region is getting mapped using the ioremap_wc() method (Uncached Write-Combine CA), which eventually is converted to calling get_vm_area() and ioremap_page_range() (see ioremap_prot() function on MIPS), which in its turn use the page structs for mapping. Another similar case is using ioremap_wc() in the PCIe outbound ATU space mapping of the graphic/video cards framebuffers.

In general having the pages array defined for the IO-memory is required for mapping the IO-space other than just uncached (my sram case for example) or, for instance, with special access attribute for the user-space (if I am not missing something in a way VM works in that case).

-Serge(y)

Quoted 25 lines

Quoted 20 lines

---
 mm/mm_init.c | 1 +
 1 file changed, 1 insertion(+)

mm/mm_init.c +1 -0

index 077bfe393b5e..3fa33e2d32ba 100644
@@ -796,6 +796,7 @@ overlap_memmap_init(unsigned long zone, unsigned long *pfn)
  * - physical memory bank size is not necessarily the exact multiple of the
  *   arbitrary section size
  * - early reserved memory may not be listed in memblock.memory
+ * - memory mapped IO space
  * - memory layouts defined with memmap= kernel parameter may not align
  *   nicely with memmap sections
  *

Signature · 1 line

2.42.1

Signature · 2 lines

Sincerely yours,
Mike.

Quoted 20 lines · Serge Semin

Quoted 11 lines · Mike Rapoport

Quoted 10 lines · Serge Semin

Besides of the already described reasons the pages backended memory holes might be persistent due to having memory mapped IO spaces behind those ranges in the framework of flatmem kernel config. Add such note to the init_unavailable_range() method kdoc in order to point out to one more reason of having the function executed for such regions.

Signed-off-by Serge Semin <[email protected]>

---

Quoted 7 lines

Quoted 3 lines

Please let me know if the IO-space pages must be initialized somehow differently rather relying on free_area_init() executing the init_unavailable_range() method.

Maybe I'm missing something, but why do you need struct pages in the IO space?

Quoted 8 lines

In my case at the very least that's due to having a SRAM device available in the middle of the MMIO-space. The region is getting mapped using the ioremap_wc() method (Uncached Write-Combine CA), which eventually is converted to calling get_vm_area() and ioremap_page_range() (see ioremap_prot() function on MIPS), which in its turn use the page structs for mapping. Another similar case is using ioremap_wc() in the PCIe outbound ATU space mapping of the graphic/video cards framebuffers.

ioremap_page_range() does not need struct pages, but rather physical addresses.

Quoted 6 lines

In general having the pages array defined for the IO-memory is required for mapping the IO-space other than just uncached (my sram case for example) or, for instance, with special access attribute for the user-space (if I am not missing something in a way VM works in that case).

No, struct pages are not required to map IO space. If you need to map MMIO to userspace there's remap_pfn_range() for that.

My guess is that your system has a hole in the physical memory mappings and with FLATMEM that hole will have essentially unused struct pages, which are initialized by init_unavailable_range(). But from mm perspective this is still a hole even though there's some MMIO ranges in that hole.

Now, if that hole is large you are wasting memory for unused memory map and it maybe worth considering using SPARSEMEM.

Quoted 28 lines

-Serge(y)

Quoted 25 lines

Quoted 20 lines

---
 mm/mm_init.c | 1 +
 1 file changed, 1 insertion(+)

mm/mm_init.c +1 -0

index 077bfe393b5e..3fa33e2d32ba 100644
@@ -796,6 +796,7 @@ overlap_memmap_init(unsigned long zone, unsigned long *pfn)
  * - physical memory bank size is not necessarily the exact multiple of the
  *   arbitrary section size
  * - early reserved memory may not be listed in memblock.memory
+ * - memory mapped IO space
  * - memory layouts defined with memmap= kernel parameter may not align
  *   nicely with memmap sections
  *

Signature · 1 line

2.42.1

Signature · 2 lines

Sincerely yours,
Mike.

Signature · 2 lines

Sincerely yours,
Mike.

Quoted 30 lines · Mike Rapoport

Quoted 20 lines · Serge Semin

Quoted 11 lines · Mike Rapoport

Quoted 10 lines · Serge Semin

Besides of the already described reasons the pages backended memory holes might be persistent due to having memory mapped IO spaces behind those ranges in the framework of flatmem kernel config. Add such note to the init_unavailable_range() method kdoc in order to point out to one more reason of having the function executed for such regions.

Signed-off-by Serge Semin <[email protected]>

---

Quoted 7 lines

Quoted 3 lines

Please let me know if the IO-space pages must be initialized somehow differently rather relying on free_area_init() executing the init_unavailable_range() method.

Maybe I'm missing something, but why do you need struct pages in the IO space?

Quoted 8 lines

In my case at the very least that's due to having a SRAM device available in the middle of the MMIO-space. The region is getting mapped using the ioremap_wc() method (Uncached Write-Combine CA), which eventually is converted to calling get_vm_area() and ioremap_page_range() (see ioremap_prot() function on MIPS), which in its turn use the page structs for mapping. Another similar case is using ioremap_wc() in the PCIe outbound ATU space mapping of the graphic/video cards framebuffers.

Quoted 2 lines

ioremap_page_range() does not need struct pages, but rather physical addresses.

Unless I miss something or MIPS32 is somehow special/wrong in that matter, but from my just got experience it actually does at least in the framework of the __update_cache() implementation which is called in the set_ptes() method (former set_pte_at()), which in its turn is eventually invoked by vmap_range_noflush() and finally by ioremap_page_range(). See the patch
[PATCH 3/7] mips: Fix max_mapnr being uninitialized on early stages

Link https://lore.kernel.org/linux-mips/[email protected]/ open here

of this series and the stack-trace of the bug fixed by that patch.

Is it wrong that on MIPS32 ioremap_page_range() eventually relies on the page structs? It has been like that for, I don't know, long time. If so then the sparse memory config might be broken on MIPS32..(

Quoted 9 lines

Quoted 5 lines

In general having the pages array defined for the IO-memory is required for mapping the IO-space other than just uncached (my sram case for example) or, for instance, with special access attribute for the user-space (if I am not missing something in a way VM works in that case).

No, struct pages are not required to map IO space. If you need to map MMIO to userspace there's remap_pfn_range() for that.

Is this correct for both flat and sparse memory config? In anyway please see my comment above about the problem I recently got.

Quoted 5 lines

My guess is that your system has a hole in the physical memory mappings and with FLATMEM that hole will have essentially unused struct pages, which are initialized by init_unavailable_range(). But from mm perspective this is still a hole even though there's some MMIO ranges in that hole.

Absolutely right. Here is the physical memory layout in my system.
0 - 128MB: RAM
128MB - 512MB: Memory mapped IO
512MB - 768MB..8.256GB: RAM

Quoted 3 lines

Now, if that hole is large you are wasting memory for unused memory map and it maybe worth considering using SPARSEMEM.

Do you think it's worth to move to the sparse memory configuration in order to save the 384MB of mapping with the 16K page model? AFAIU flat memory config is more performant. Performance is critical on the most of the SoC applications especially when using the 10G ethernet or the high-speed PCIe devices.

-Serge(y)

Quoted 32 lines

Quoted 28 lines

-Serge(y)

Quoted 25 lines

Quoted 20 lines

---
 mm/mm_init.c | 1 +
 1 file changed, 1 insertion(+)

mm/mm_init.c +1 -0

index 077bfe393b5e..3fa33e2d32ba 100644
@@ -796,6 +796,7 @@ overlap_memmap_init(unsigned long zone, unsigned long *pfn)
  * - physical memory bank size is not necessarily the exact multiple of the
  *   arbitrary section size
  * - early reserved memory may not be listed in memblock.memory
+ * - memory mapped IO space
  * - memory layouts defined with memmap= kernel parameter may not align
  *   nicely with memmap sections
  *

Signature · 1 line

2.42.1

Signature · 2 lines

Sincerely yours,
Mike.

Signature · 2 lines

Sincerely yours,
Mike.

Quoted 44 lines · Serge Semin

Quoted 30 lines · Mike Rapoport

Quoted 20 lines · Serge Semin

Quoted 11 lines · Mike Rapoport

Quoted 10 lines · Serge Semin

Besides of the already described reasons the pages backended memory holes might be persistent due to having memory mapped IO spaces behind those ranges in the framework of flatmem kernel config. Add such note to the init_unavailable_range() method kdoc in order to point out to one more reason of having the function executed for such regions.

Signed-off-by Serge Semin <[email protected]>

---

Quoted 7 lines

Quoted 3 lines

Please let me know if the IO-space pages must be initialized somehow differently rather relying on free_area_init() executing the init_unavailable_range() method.

Maybe I'm missing something, but why do you need struct pages in the IO space?

Quoted 8 lines

In my case at the very least that's due to having a SRAM device available in the middle of the MMIO-space. The region is getting mapped using the ioremap_wc() method (Uncached Write-Combine CA), which eventually is converted to calling get_vm_area() and ioremap_page_range() (see ioremap_prot() function on MIPS), which in its turn use the page structs for mapping. Another similar case is using ioremap_wc() in the PCIe outbound ATU space mapping of the graphic/video cards framebuffers.

Quoted 2 lines

ioremap_page_range() does not need struct pages, but rather physical addresses.

Unless I miss something or MIPS32 is somehow special/wrong in that matter, but from my just got experience it actually does at least in the framework of the __update_cache() implementation which is called in the set_ptes() method (former set_pte_at()), which in its turn is eventually invoked by vmap_range_noflush() and finally by ioremap_page_range(). See the patch
[PATCH 3/7] mips: Fix max_mapnr being uninitialized on early stages

Link https://lore.kernel.org/linux-mips/[email protected]/ open here

of this series and the stack-trace of the bug fixed by that patch.

Quoted 3 lines

Is it wrong that on MIPS32 ioremap_page_range() eventually relies on the page structs? It has been like that for, I don't know, long time. If so then the sparse memory config might be broken on MIPS32..(

Do you mind posting your physical memory layout?

Quoted 17 lines · Mike Rapoport

...

Quoted 5 lines

My guess is that your system has a hole in the physical memory mappings and with FLATMEM that hole will have essentially unused struct pages, which are initialized by init_unavailable_range(). But from mm perspective this is still a hole even though there's some MMIO ranges in that hole.

Absolutely right. Here is the physical memory layout in my system.
0 - 128MB: RAM
128MB - 512MB: Memory mapped IO
512MB - 768MB..8.256GB: RAM

Quoted 3 lines

Now, if that hole is large you are wasting memory for unused memory map and it maybe worth considering using SPARSEMEM.

Quoted 5 lines

Do you think it's worth to move to the sparse memory configuration in order to save the 384MB of mapping with the 16K page model? AFAIU flat memory config is more performant. Performance is critical on the most of the SoC applications especially when using the 10G ethernet or the high-speed PCIe devices.

...

If I understand correctly, you have a hole in your RAM and there is MMIO region somewhere in that hole.

With FLATMEM the memory map exists for that
hole and hence pfn_valid() returns 1 for the MMIO range as well. That makes __update_cache() to check folio state and that check would fail if the memory map contained garbage. But since the hole in the memory map is initialized with init_unavailable_range() you get a valid struct page/struct folio and everything is fine.

With that, the init_unavailable_range() docs need not mention IO space at all, they should mention holes within FLATMEM memory map.

As for SPARSEMEM, if the hole does not belong to any section, pfn_valid() will be false for it and __update_cache() won't try to access memory map.

Quoted 69 lines

Quoted 10 lines

Quoted 6 lines

In general having the pages array defined for the IO-memory is required for mapping the IO-space other than just uncached (my sram case for example) or, for instance, with special access attribute for the user-space (if I am not missing something in a way VM works in that case).

No, struct pages are not required to map IO space. If you need to map MMIO to userspace there's remap_pfn_range() for that.

Is this correct for both flat and sparse memory config? In anyway please see my comment above about the problem I recently got.

Quoted 5 lines

My guess is that your system has a hole in the physical memory mappings and with FLATMEM that hole will have essentially unused struct pages, which are initialized by init_unavailable_range(). But from mm perspective this is still a hole even though there's some MMIO ranges in that hole.

Absolutely right. Here is the physical memory layout in my system.
0 - 128MB: RAM
128MB - 512MB: Memory mapped IO
512MB - 768MB..8.256GB: RAM

Quoted 3 lines

Now, if that hole is large you are wasting memory for unused memory map and it maybe worth considering using SPARSEMEM.

Do you think it's worth to move to the sparse memory configuration in order to save the 384MB of mapping with the 16K page model? AFAIU flat memory config is more performant. Performance is critical on the most of the SoC applications especially when using the 10G ethernet or the high-speed PCIe devices.

-Serge(y)

Quoted 32 lines

Quoted 28 lines

-Serge(y)

Quoted 25 lines

Quoted 20 lines

---
 mm/mm_init.c | 1 +
 1 file changed, 1 insertion(+)

mm/mm_init.c +1 -0

index 077bfe393b5e..3fa33e2d32ba 100644
@@ -796,6 +796,7 @@ overlap_memmap_init(unsigned long zone, unsigned long *pfn)
  * - physical memory bank size is not necessarily the exact multiple of the
  *   arbitrary section size
  * - early reserved memory may not be listed in memblock.memory
+ * - memory mapped IO space
  * - memory layouts defined with memmap= kernel parameter may not align
  *   nicely with memmap sections
  *

Signature · 1 line

2.42.1

Signature · 2 lines

Sincerely yours,
Mike.

Signature · 2 lines

Sincerely yours,
Mike.

Signature · 2 lines

Sincerely yours,
Mike.

Quoted 3 lines · Mike Rapoport

On Fri, Nov 24, 2023 at 02:18:44PM +0300, Serge Semin wrote:

Do you mind posting your physical memory layout?

I actually already did in response to the last part of your previous message. You must have missed it. Here is the copy of the message:

Quoted 18 lines · Serge Semin

Quoted 17 lines · Mike Rapoport

...

Quoted 5 lines

My guess is that your system has a hole in the physical memory mappings and with FLATMEM that hole will have essentially unused struct pages, which are initialized by init_unavailable_range(). But from mm perspective this is still a hole even though there's some MMIO ranges in that hole.

Absolutely right. Here is the physical memory layout in my system.
0 - 128MB: RAM
128MB - 512MB: Memory mapped IO
512MB - 768MB..8.256GB: RAM

Quoted 3 lines

Now, if that hole is large you are wasting memory for unused memory map and it maybe worth considering using SPARSEMEM.

Quoted 5 lines

Quoted 5 lines

Do you think it's worth to move to the sparse memory configuration in order to save the 384MB of mapping with the 16K page model? AFAIU flat memory config is more performant. Performance is critical on the most of the SoC applications especially when using the 10G ethernet or the high-speed PCIe devices.

Could you also answer to my question above regarding using the sparsemem instead on my hw memory layout?

Quoted 6 lines

With FLATMEM the memory map exists for that
hole and hence pfn_valid() returns 1 for the MMIO range as well. That makes __update_cache() to check folio state and that check would fail if the memory map contained garbage. But since the hole in the memory map is initialized with init_unavailable_range() you get a valid struct page/struct folio and everything is fine.

Right. That's what currently happens on MIPS32 and that's what I had to fix in the framework of this series by the next patch:

Link https://lore.kernel.org/linux-mips/[email protected]/ open here

flatmem version of the pfn_valid() method has been broken due to max_mapnr being uninitialized before mem_init() is called. So init_unavailable_range() didn't initialize the pages on the early bootup stage. Thus afterwards, when max_mapnr has finally got a valid value any attempts to call the __update_cache() method on the MMIO memory hole caused the unaligned access crash.

Quoted 3 lines

With that, the init_unavailable_range() docs need not mention IO space at all, they should mention holes within FLATMEM memory map.

Ok. I'll resend the patch with mentioning flatmem holes instead of mentioning the IO-spaces.

Quoted 1 line

Quoted 2 lines

As for SPARSEMEM, if the hole does not belong to any section, pfn_valid() will be false for it and __update_cache() won't try to access memory map.

Ah, I see. In case of the SPARSEMEM config an another version of pfn_valid() will be called. It's defined in the include/linux/mmzone.h header file. Right? If so then no problem there indeed.

-Serge(y)

Quoted 7 lines · Serge Semin

Quoted 3 lines · Mike Rapoport

On Fri, Nov 24, 2023 at 02:18:44PM +0300, Serge Semin wrote:

Do you mind posting your physical memory layout?

I actually already did in response to the last part of your previous message. You must have missed it. Here is the copy of the message:

Sorry, for some reason I didn't scroll down your previous mail :)

Quoted 18 lines

Quoted 18 lines · Serge Semin

Quoted 17 lines · Mike Rapoport

...

Quoted 5 lines

My guess is that your system has a hole in the physical memory mappings and with FLATMEM that hole will have essentially unused struct pages, which are initialized by init_unavailable_range(). But from mm perspective this is still a hole even though there's some MMIO ranges in that hole.

Absolutely right. Here is the physical memory layout in my system.
0 - 128MB: RAM
128MB - 512MB: Memory mapped IO
512MB - 768MB..8.256GB: RAM

Quoted 3 lines

Now, if that hole is large you are wasting memory for unused memory map and it maybe worth considering using SPARSEMEM.

Quoted 8 lines

Quoted 5 lines

Quoted 5 lines

Do you think it's worth to move to the sparse memory configuration in order to save the 384MB of mapping with the 16K page model? AFAIU flat memory config is more performant. Performance is critical on the most of the SoC applications especially when using the 10G ethernet or the high-speed PCIe devices.

Could you also answer to my question above regarding using the sparsemem instead on my hw memory layout?

Currently MIPS defines section size to 256MB, so with your memory layout with SPARSMEM there will be two sections of 256MB, at 0 and at 512MB, so you'll save memory map for 256M which is roughly 1M with 16k pages.

It's possible

With SPARSEMEM the pfn_to_page() and page_to_pfn() are a bit longer in terms of assembly instructions, but I really doubt you'll notice any performance difference in real world applications.

Quoted 10 lines

Quoted 7 lines

With FLATMEM the memory map exists for that
hole and hence pfn_valid() returns 1 for the MMIO range as well. That makes __update_cache() to check folio state and that check would fail if the memory map contained garbage. But since the hole in the memory map is initialized with init_unavailable_range() you get a valid struct page/struct folio and everything is fine.

Right. That's what currently happens on MIPS32 and that's what I had to fix in the framework of this series by the next patch:

Quoted 7 lines

Link https://lore.kernel.org/linux-mips/[email protected]/ open here

flatmem version of the pfn_valid() method has been broken due to max_mapnr being uninitialized before mem_init() is called. So init_unavailable_range() didn't initialize the pages on the early bootup stage. Thus afterwards, when max_mapnr has finally got a valid value any attempts to call the __update_cache() method on the MMIO memory hole caused the unaligned access crash.

The fix for max_mapnr makes pfn_valid()==1 for the entire memory map and this fixes up the struct pages in the hole.

Quoted 8 lines

Quoted 3 lines

With that, the init_unavailable_range() docs need not mention IO space at all, they should mention holes within FLATMEM memory map.

Ok. I'll resend the patch with mentioning flatmem holes instead of mentioning the IO-spaces.

Quoted 1 line

Quoted 6 lines

Quoted 2 lines

As for SPARSEMEM, if the hole does not belong to any section, pfn_valid() will be false for it and __update_cache() won't try to access memory map.

Ah, I see. In case of the SPARSEMEM config an another version of pfn_valid() will be called. It's defined in the include/linux/mmzone.h header file. Right? If so then no problem there indeed.

Yes, SPARSMEM uses pfn_valid() defined in include/linux/mmzone.h

Quoted 1 line

-Serge(y)

Signature · 2 lines

Sincerely yours,
Mike.

Based on the init_unavailable_range() method and it's callee semantics no multi-line info messages are intended to be printed to the console. Thus append the '\n' symbol to the respective info string.

Signed-off-by Serge Semin <[email protected]>

---
 mm/mm_init.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

mm/mm_init.c +1 -1

index 3fa33e2d32ba..db8b91175834 100644
@@ -827,7 +827,7 @@ static void __init init_unavailable_range(unsigned long spfn,
 	}
 
 	if (pgcnt)
-		pr_info("On node %d, zone %s: %lld pages in unavailable ranges",
+		pr_info("On node %d, zone %s: %lld pages in unavailable ranges\n",
 			node, zone_names[zone], pgcnt);
 }
 

Signature · 1 line

2.42.1

Just recently I've rebased my MIPS32-related work from kernel 6.5-rc4 onto the latest kernel 6.7-rc1 and immediately got into a bootup-time mm-related bug (see patches 3-5 in this series). After fixing it I decided it was time to submit for review the generic MIPS code fixes which I have been collecting in my local repo for the last year. I was going to submit them a bit later after I finished working on a patchset with my SoC arch-specific changes, but since it was getting bigger and bigger, it turned to be reasonable to spill out the generic part of series right away especially seeing it might get to be useful in the most recent kernel.

Quoted 9 lines · On Wed, 22 Nov 2023 21:23:58 +0300 Serge Semin <[email protected]> wrote:

Just recently I've rebased my MIPS32-related work from kernel 6.5-rc4 onto the latest kernel 6.7-rc1 and immediately got into a bootup-time mm-related bug (see patches 3-5 in this series). After fixing it I decided it was time to submit for review the generic MIPS code fixes which I have been collecting in my local repo for the last year. I was going to submit them a bit later after I finished working on a patchset with my SoC arch-specific changes, but since it was getting bigger and bigger, it turned to be reasonable to spill out the generic part of series right away especially seeing it might get to be useful in the most recent kernel.

It would have been better to separate out the two tiny unrelated MM patches from this series.

I'll steal them - if they later turn up via
the MIPS tree then that's OK.