This series fixes various issues observed in latest kernel on RV32.
The first two patches fixes an resource tree introduced in 5.11-rc1
while the last two fixes the case where 2GB physical memory is used
on RV32.
There are may be better way to fix the issue pointed out in PATCH 3
as it seems a generic kernel issue where kernel pointers can not use
last 4k of addressable memory. I am open to other better alternate
suggestions.
Atish Patra (4):
RISC-V: Do not allocate memblock while iterating reserved memblocks
RISC-V: Set current memblock limit
RISC-V: Fix L1_CACHE_BYTES for RV32
RISC-V: Fix maximum allowed phsyical memory for RV32
arch/riscv/Kconfig | 6 ++++--
arch/riscv/include/asm/cache.h | 4 ++++
arch/riscv/kernel/setup.c | 24 +++++++++++++-----------
arch/riscv/mm/init.c | 16 ++++++++++++++--
4 files changed, 35 insertions(+), 15 deletions(-)
--
2.25.1
Currently, resource tree allocates memory blocks while iterating on the
list. It leads to following kernel warning because memblock allocation
also invokes memory block reservation API.
[ 0.000000] ------------[ cut here ]------------
[ 0.000000] WARNING: CPU: 0 PID: 0 at kernel/resource.c:795
__insert_resource+0x8e/0xd0
[ 0.000000] Modules linked in:
[ 0.000000] CPU: 0 PID: 0 Comm: swapper Not tainted
5.10.0-00022-ge20097fb37e2-dirty #549
[ 0.000000] epc: c00125c2 ra : c001262c sp : c1c01f50
[ 0.000000] gp : c1d456e0 tp : c1c0a980 t0 : ffffcf20
[ 0.000000] t1 : 00000000 t2 : 00000000 s0 : c1c01f60
[ 0.000000] s1 : ffffcf00 a0 : ffffff00 a1 : c1c0c0c4
[ 0.000000] a2 : 80c12b15 a3 : 80402000 a4 : 80402000
[ 0.000000] a5 : c1c0c0c4 a6 : 80c12b15 a7 : f5faf600
[ 0.000000] s2 : c1c0c0c4 s3 : c1c0e000 s4 : c1009a80
[ 0.000000] s5 : c1c0c000 s6 : c1d48000 s7 : c1613b4c
[ 0.000000] s8 : 00000fff s9 : 80000200 s10: c1613b40
[ 0.000000] s11: 00000000 t3 : c1d4a000 t4 : ffffffff
This is also unnecessary as we can pre-compute the total memblocks required
for each memory region and allocate it before the loop. It save precious
boot time not going through memblock allocation code every time.
Fixes: 00ab027a3b82 ("RISC-V: Add kernel image sections to the resource tree")
Signed-off-by: Atish Patra <[email protected]>
---
arch/riscv/kernel/setup.c | 24 +++++++++++++-----------
1 file changed, 13 insertions(+), 11 deletions(-)
diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
index 1d85e9bf783c..3fa3f26dde85 100644
--- a/arch/riscv/kernel/setup.c
+++ b/arch/riscv/kernel/setup.c
@@ -127,7 +127,9 @@ static void __init init_resources(void)
{
struct memblock_region *region = NULL;
struct resource *res = NULL;
- int ret = 0;
+ struct resource *mem_res = NULL;
+ size_t mem_res_sz = 0;
+ int ret = 0, i = 0;
code_res.start = __pa_symbol(_text);
code_res.end = __pa_symbol(_etext) - 1;
@@ -145,16 +147,17 @@ static void __init init_resources(void)
bss_res.end = __pa_symbol(__bss_stop) - 1;
bss_res.flags = IORESOURCE_SYSTEM_RAM | IORESOURCE_BUSY;
+ mem_res_sz = (memblock.memory.cnt + memblock.reserved.cnt) * sizeof(*mem_res);
+ mem_res = memblock_alloc(mem_res_sz, SMP_CACHE_BYTES);
+ if (!mem_res)
+ panic("%s: Failed to allocate %zu bytes\n", __func__, mem_res_sz);
/*
* Start by adding the reserved regions, if they overlap
* with /memory regions, insert_resource later on will take
* care of it.
*/
for_each_reserved_mem_region(region) {
- res = memblock_alloc(sizeof(struct resource), SMP_CACHE_BYTES);
- if (!res)
- panic("%s: Failed to allocate %zu bytes\n", __func__,
- sizeof(struct resource));
+ res = &mem_res[i++];
res->name = "Reserved";
res->flags = IORESOURCE_MEM | IORESOURCE_BUSY;
@@ -171,8 +174,10 @@ static void __init init_resources(void)
* Ignore any other reserved regions within
* system memory.
*/
- if (memblock_is_memory(res->start))
+ if (memblock_is_memory(res->start)) {
+ memblock_free((phys_addr_t) res, sizeof(struct resource));
continue;
+ }
ret = add_resource(&iomem_resource, res);
if (ret < 0)
@@ -181,10 +186,7 @@ static void __init init_resources(void)
/* Add /memory regions to the resource tree */
for_each_mem_region(region) {
- res = memblock_alloc(sizeof(struct resource), SMP_CACHE_BYTES);
- if (!res)
- panic("%s: Failed to allocate %zu bytes\n", __func__,
- sizeof(struct resource));
+ res = &mem_res[i++];
if (unlikely(memblock_is_nomap(region))) {
res->name = "Reserved";
@@ -205,9 +207,9 @@ static void __init init_resources(void)
return;
error:
- memblock_free((phys_addr_t) res, sizeof(struct resource));
/* Better an empty resource tree than an inconsistent one */
release_child_resources(&iomem_resource);
+ memblock_free((phys_addr_t) mem_res, mem_res_sz);
}
--
2.25.1
SMP_CACHE_BYTES/L1_CACHE_BYTES should be defined as 32 instead of
64 for RV32. Otherwise, there will be hole of 32 bytes with each memblock
allocation if it is requested to be aligned with SMP_CACHE_BYTES.
Signed-off-by: Atish Patra <[email protected]>
---
arch/riscv/include/asm/cache.h | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/arch/riscv/include/asm/cache.h b/arch/riscv/include/asm/cache.h
index 9b58b104559e..c9c669ea2fe6 100644
--- a/arch/riscv/include/asm/cache.h
+++ b/arch/riscv/include/asm/cache.h
@@ -7,7 +7,11 @@
#ifndef _ASM_RISCV_CACHE_H
#define _ASM_RISCV_CACHE_H
+#ifdef CONFIG_64BIT
#define L1_CACHE_SHIFT 6
+#else
+#define L1_CACHE_SHIFT 5
+#endif
#define L1_CACHE_BYTES (1 << L1_CACHE_SHIFT)
--
2.25.1
Currently, linux kernel can not use last 4k bytes of addressable space because
IS_ERR_VALUE macro treats those as an error. This will be an issue for RV32
as any memblock allocator potentially allocate chunk of memory from the end
of DRAM (2GB) leading bad address error even though the address was technically
valid.
Fix this issue by limiting the memblock if available memory spans the entire
address space.
Signed-off-by: Atish Patra <[email protected]>
---
arch/riscv/mm/init.c | 16 ++++++++++++++--
1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
index bf5379135e39..da53902ef0fc 100644
--- a/arch/riscv/mm/init.c
+++ b/arch/riscv/mm/init.c
@@ -157,9 +157,10 @@ static void __init setup_initrd(void)
void __init setup_bootmem(void)
{
phys_addr_t mem_start = 0;
- phys_addr_t start, end = 0;
+ phys_addr_t start, dram_end, end = 0;
phys_addr_t vmlinux_end = __pa_symbol(&_end);
phys_addr_t vmlinux_start = __pa_symbol(&_start);
+ phys_addr_t max_mapped_addr = __pa(PHYS_ADDR_MAX);
u64 i;
/* Find the memory region containing the kernel */
@@ -181,7 +182,18 @@ void __init setup_bootmem(void)
/* Reserve from the start of the kernel to the end of the kernel */
memblock_reserve(vmlinux_start, vmlinux_end - vmlinux_start);
- max_pfn = PFN_DOWN(memblock_end_of_DRAM());
+ dram_end = memblock_end_of_DRAM();
+
+ /*
+ * memblock allocator is not aware of the fact that last 4K bytes of
+ * the addressable memory can not be mapped because of IS_ERR_VALUE
+ * macro. Make sure that last 4k bytes are not usable by memblock
+ * if end of dram is equal to maximum addressable memory.
+ */
+ if (max_mapped_addr == (dram_end - 1))
+ memblock_set_current_limit(max_mapped_addr - 4096);
+
+ max_pfn = PFN_DOWN(dram_end);
max_low_pfn = max_pfn;
dma32_phys_limit = min(4UL * SZ_1G, (unsigned long)PFN_PHYS(max_low_pfn));
set_max_mapnr(max_low_pfn);
--
2.25.1
Linux kernel can only map 1GB of address space for RV32 as the page offset
is set to 0xC0000000. The current description in the Kconfig is confusing
as it indicates that RV32 can support 2GB of physical memory. That is
simply not true for current kernel. In future, a 2GB split support can be
added to allow 2GB physical address space.
Signed-off-by: Atish Patra <[email protected]>
---
arch/riscv/Kconfig | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
index 81b76d44725d..e9e2c1f0a690 100644
--- a/arch/riscv/Kconfig
+++ b/arch/riscv/Kconfig
@@ -137,7 +137,7 @@ config PA_BITS
config PAGE_OFFSET
hex
- default 0xC0000000 if 32BIT && MAXPHYSMEM_2GB
+ default 0xC0000000 if 32BIT && MAXPHYSMEM_1GB
default 0x80000000 if 64BIT && !MMU
default 0xffffffff80000000 if 64BIT && MAXPHYSMEM_2GB
default 0xffffffe000000000 if 64BIT && MAXPHYSMEM_128GB
@@ -247,10 +247,12 @@ config MODULE_SECTIONS
choice
prompt "Maximum Physical Memory"
- default MAXPHYSMEM_2GB if 32BIT
+ default MAXPHYSMEM_1GB if 32BIT
default MAXPHYSMEM_2GB if 64BIT && CMODEL_MEDLOW
default MAXPHYSMEM_128GB if 64BIT && CMODEL_MEDANY
+ config MAXPHYSMEM_1GB
+ bool "1GiB"
config MAXPHYSMEM_2GB
bool "2GiB"
config MAXPHYSMEM_128GB
--
2.25.1
Hi Atish,
On Thu, Jan 7, 2021 at 10:28 AM Atish Patra <[email protected]> wrote:
> Currently, resource tree allocates memory blocks while iterating on the
> list. It leads to following kernel warning because memblock allocation
> also invokes memory block reservation API.
>
> [ 0.000000] ------------[ cut here ]------------
> [ 0.000000] WARNING: CPU: 0 PID: 0 at kernel/resource.c:795
> __insert_resource+0x8e/0xd0
> [ 0.000000] Modules linked in:
> [ 0.000000] CPU: 0 PID: 0 Comm: swapper Not tainted
> 5.10.0-00022-ge20097fb37e2-dirty #549
> [ 0.000000] epc: c00125c2 ra : c001262c sp : c1c01f50
> [ 0.000000] gp : c1d456e0 tp : c1c0a980 t0 : ffffcf20
> [ 0.000000] t1 : 00000000 t2 : 00000000 s0 : c1c01f60
> [ 0.000000] s1 : ffffcf00 a0 : ffffff00 a1 : c1c0c0c4
> [ 0.000000] a2 : 80c12b15 a3 : 80402000 a4 : 80402000
> [ 0.000000] a5 : c1c0c0c4 a6 : 80c12b15 a7 : f5faf600
> [ 0.000000] s2 : c1c0c0c4 s3 : c1c0e000 s4 : c1009a80
> [ 0.000000] s5 : c1c0c000 s6 : c1d48000 s7 : c1613b4c
> [ 0.000000] s8 : 00000fff s9 : 80000200 s10: c1613b40
> [ 0.000000] s11: 00000000 t3 : c1d4a000 t4 : ffffffff
>
> This is also unnecessary as we can pre-compute the total memblocks required
> for each memory region and allocate it before the loop. It save precious
> boot time not going through memblock allocation code every time.
>
> Fixes: 00ab027a3b82 ("RISC-V: Add kernel image sections to the resource tree")
>
> Signed-off-by: Atish Patra <[email protected]>
Thanks for your patch!
I never saw the warning (on linux-on-litex-vexriscv), but instead I got:
Failed to add a Kernel code resource at 40001000
after Initmem setup. Adding some debug info to init_resources() showed
that the memblock.reserved list kept on increasing, until memory
corruption happened (pointers started to look like ASCII strings), the
error message above is printed, and after which the boot continued.
Changing L1_CACHE_SHIFT in arch/riscv/include/asm/cache.h to 5 fixed that.
With this patch, the error message above is no longer printed, so
Tested-by: Geert Uytterhoeven <[email protected]>
Noted that the kernel still crashes later, with
Unable to handle kernel paging request at virtual address 61636473
Again, 0x61636473 looks like ASCII, and your PATCH 3/4 for
L1_CACHE_SHIFT fixes that, too.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Thu, Jan 7, 2021 at 10:28 AM Atish Patra <[email protected]> wrote:
> SMP_CACHE_BYTES/L1_CACHE_BYTES should be defined as 32 instead of
> 64 for RV32. Otherwise, there will be hole of 32 bytes with each memblock
> allocation if it is requested to be aligned with SMP_CACHE_BYTES.
>
> Signed-off-by: Atish Patra <[email protected]>
Tested-by: Geert Uytterhoeven <[email protected]>
(on vexriscv)
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Thu, Jan 7, 2021 at 2:57 PM Atish Patra <[email protected]> wrote:
>
> Currently, resource tree allocates memory blocks while iterating on the
> list. It leads to following kernel warning because memblock allocation
> also invokes memory block reservation API.
>
> [ 0.000000] ------------[ cut here ]------------
> [ 0.000000] WARNING: CPU: 0 PID: 0 at kernel/resource.c:795
> __insert_resource+0x8e/0xd0
> [ 0.000000] Modules linked in:
> [ 0.000000] CPU: 0 PID: 0 Comm: swapper Not tainted
> 5.10.0-00022-ge20097fb37e2-dirty #549
> [ 0.000000] epc: c00125c2 ra : c001262c sp : c1c01f50
> [ 0.000000] gp : c1d456e0 tp : c1c0a980 t0 : ffffcf20
> [ 0.000000] t1 : 00000000 t2 : 00000000 s0 : c1c01f60
> [ 0.000000] s1 : ffffcf00 a0 : ffffff00 a1 : c1c0c0c4
> [ 0.000000] a2 : 80c12b15 a3 : 80402000 a4 : 80402000
> [ 0.000000] a5 : c1c0c0c4 a6 : 80c12b15 a7 : f5faf600
> [ 0.000000] s2 : c1c0c0c4 s3 : c1c0e000 s4 : c1009a80
> [ 0.000000] s5 : c1c0c000 s6 : c1d48000 s7 : c1613b4c
> [ 0.000000] s8 : 00000fff s9 : 80000200 s10: c1613b40
> [ 0.000000] s11: 00000000 t3 : c1d4a000 t4 : ffffffff
>
> This is also unnecessary as we can pre-compute the total memblocks required
> for each memory region and allocate it before the loop. It save precious
> boot time not going through memblock allocation code every time.
>
> Fixes: 00ab027a3b82 ("RISC-V: Add kernel image sections to the resource tree")
>
> Signed-off-by: Atish Patra <[email protected]>
Looks good to me.
Reviewed-by: Anup Patel <[email protected]>
> ---
> arch/riscv/kernel/setup.c | 24 +++++++++++++-----------
> 1 file changed, 13 insertions(+), 11 deletions(-)
>
> diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
> index 1d85e9bf783c..3fa3f26dde85 100644
> --- a/arch/riscv/kernel/setup.c
> +++ b/arch/riscv/kernel/setup.c
> @@ -127,7 +127,9 @@ static void __init init_resources(void)
> {
> struct memblock_region *region = NULL;
> struct resource *res = NULL;
> - int ret = 0;
> + struct resource *mem_res = NULL;
> + size_t mem_res_sz = 0;
> + int ret = 0, i = 0;
>
> code_res.start = __pa_symbol(_text);
> code_res.end = __pa_symbol(_etext) - 1;
> @@ -145,16 +147,17 @@ static void __init init_resources(void)
> bss_res.end = __pa_symbol(__bss_stop) - 1;
> bss_res.flags = IORESOURCE_SYSTEM_RAM | IORESOURCE_BUSY;
>
> + mem_res_sz = (memblock.memory.cnt + memblock.reserved.cnt) * sizeof(*mem_res);
> + mem_res = memblock_alloc(mem_res_sz, SMP_CACHE_BYTES);
> + if (!mem_res)
> + panic("%s: Failed to allocate %zu bytes\n", __func__, mem_res_sz);
> /*
> * Start by adding the reserved regions, if they overlap
> * with /memory regions, insert_resource later on will take
> * care of it.
> */
> for_each_reserved_mem_region(region) {
> - res = memblock_alloc(sizeof(struct resource), SMP_CACHE_BYTES);
> - if (!res)
> - panic("%s: Failed to allocate %zu bytes\n", __func__,
> - sizeof(struct resource));
> + res = &mem_res[i++];
>
> res->name = "Reserved";
> res->flags = IORESOURCE_MEM | IORESOURCE_BUSY;
> @@ -171,8 +174,10 @@ static void __init init_resources(void)
> * Ignore any other reserved regions within
> * system memory.
> */
> - if (memblock_is_memory(res->start))
> + if (memblock_is_memory(res->start)) {
> + memblock_free((phys_addr_t) res, sizeof(struct resource));
> continue;
> + }
>
> ret = add_resource(&iomem_resource, res);
> if (ret < 0)
> @@ -181,10 +186,7 @@ static void __init init_resources(void)
>
> /* Add /memory regions to the resource tree */
> for_each_mem_region(region) {
> - res = memblock_alloc(sizeof(struct resource), SMP_CACHE_BYTES);
> - if (!res)
> - panic("%s: Failed to allocate %zu bytes\n", __func__,
> - sizeof(struct resource));
> + res = &mem_res[i++];
>
> if (unlikely(memblock_is_nomap(region))) {
> res->name = "Reserved";
> @@ -205,9 +207,9 @@ static void __init init_resources(void)
> return;
>
> error:
> - memblock_free((phys_addr_t) res, sizeof(struct resource));
> /* Better an empty resource tree than an inconsistent one */
> release_child_resources(&iomem_resource);
> + memblock_free((phys_addr_t) mem_res, mem_res_sz);
> }
>
>
> --
> 2.25.1
>
>
> _______________________________________________
> linux-riscv mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-riscv
Regards,
Anup
On Thu, Jan 7, 2021 at 2:57 PM Atish Patra <[email protected]> wrote:
>
> Currently, linux kernel can not use last 4k bytes of addressable space because
> IS_ERR_VALUE macro treats those as an error. This will be an issue for RV32
> as any memblock allocator potentially allocate chunk of memory from the end
> of DRAM (2GB) leading bad address error even though the address was technically
> valid.
>
> Fix this issue by limiting the memblock if available memory spans the entire
> address space.
>
> Signed-off-by: Atish Patra <[email protected]>
> ---
> arch/riscv/mm/init.c | 16 ++++++++++++++--
> 1 file changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
> index bf5379135e39..da53902ef0fc 100644
> --- a/arch/riscv/mm/init.c
> +++ b/arch/riscv/mm/init.c
> @@ -157,9 +157,10 @@ static void __init setup_initrd(void)
> void __init setup_bootmem(void)
> {
> phys_addr_t mem_start = 0;
> - phys_addr_t start, end = 0;
> + phys_addr_t start, dram_end, end = 0;
> phys_addr_t vmlinux_end = __pa_symbol(&_end);
> phys_addr_t vmlinux_start = __pa_symbol(&_start);
> + phys_addr_t max_mapped_addr = __pa(PHYS_ADDR_MAX);
Using PHYS_ADDR_MAX as the max virtual address does not look right.
Better use __pa(~(ulong)0) here. Otherwise looks good to me.
Reviewed-by: Anup Patel <[email protected]>
> u64 i;
>
> /* Find the memory region containing the kernel */
> @@ -181,7 +182,18 @@ void __init setup_bootmem(void)
> /* Reserve from the start of the kernel to the end of the kernel */
> memblock_reserve(vmlinux_start, vmlinux_end - vmlinux_start);
>
> - max_pfn = PFN_DOWN(memblock_end_of_DRAM());
> + dram_end = memblock_end_of_DRAM();
> +
> + /*
> + * memblock allocator is not aware of the fact that last 4K bytes of
> + * the addressable memory can not be mapped because of IS_ERR_VALUE
> + * macro. Make sure that last 4k bytes are not usable by memblock
> + * if end of dram is equal to maximum addressable memory.
> + */
> + if (max_mapped_addr == (dram_end - 1))
> + memblock_set_current_limit(max_mapped_addr - 4096);
> +
> + max_pfn = PFN_DOWN(dram_end);
> max_low_pfn = max_pfn;
> dma32_phys_limit = min(4UL * SZ_1G, (unsigned long)PFN_PHYS(max_low_pfn));
> set_max_mapnr(max_low_pfn);
> --
> 2.25.1
>
>
> _______________________________________________
> linux-riscv mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-riscv
Regards,
Anup
On Thu, Jan 7, 2021 at 2:57 PM Atish Patra <[email protected]> wrote:
>
> SMP_CACHE_BYTES/L1_CACHE_BYTES should be defined as 32 instead of
> 64 for RV32. Otherwise, there will be hole of 32 bytes with each memblock
> allocation if it is requested to be aligned with SMP_CACHE_BYTES.
>
> Signed-off-by: Atish Patra <[email protected]>
Looks good to me.
Reviewed-by: Anup Patel <[email protected]>
> ---
> arch/riscv/include/asm/cache.h | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/arch/riscv/include/asm/cache.h b/arch/riscv/include/asm/cache.h
> index 9b58b104559e..c9c669ea2fe6 100644
> --- a/arch/riscv/include/asm/cache.h
> +++ b/arch/riscv/include/asm/cache.h
> @@ -7,7 +7,11 @@
> #ifndef _ASM_RISCV_CACHE_H
> #define _ASM_RISCV_CACHE_H
>
> +#ifdef CONFIG_64BIT
> #define L1_CACHE_SHIFT 6
> +#else
> +#define L1_CACHE_SHIFT 5
> +#endif
>
> #define L1_CACHE_BYTES (1 << L1_CACHE_SHIFT)
>
> --
> 2.25.1
>
>
> _______________________________________________
> linux-riscv mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-riscv
Regards,
Anup
On Thu, Jan 7, 2021 at 2:57 PM Atish Patra <[email protected]> wrote:
>
> Linux kernel can only map 1GB of address space for RV32 as the page offset
> is set to 0xC0000000. The current description in the Kconfig is confusing
> as it indicates that RV32 can support 2GB of physical memory. That is
> simply not true for current kernel. In future, a 2GB split support can be
> added to allow 2GB physical address space.
>
> Signed-off-by: Atish Patra <[email protected]>
Just for information, Alex's also has a patch to simplify this. Refer,
"[RFC PATCH 05/12] riscv: Simplify MAXPHYSMEM config"
Looks good to me.
Reviewed-by: Anup Patel <[email protected]>
> ---
> arch/riscv/Kconfig | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> index 81b76d44725d..e9e2c1f0a690 100644
> --- a/arch/riscv/Kconfig
> +++ b/arch/riscv/Kconfig
> @@ -137,7 +137,7 @@ config PA_BITS
>
> config PAGE_OFFSET
> hex
> - default 0xC0000000 if 32BIT && MAXPHYSMEM_2GB
> + default 0xC0000000 if 32BIT && MAXPHYSMEM_1GB
> default 0x80000000 if 64BIT && !MMU
> default 0xffffffff80000000 if 64BIT && MAXPHYSMEM_2GB
> default 0xffffffe000000000 if 64BIT && MAXPHYSMEM_128GB
> @@ -247,10 +247,12 @@ config MODULE_SECTIONS
>
> choice
> prompt "Maximum Physical Memory"
> - default MAXPHYSMEM_2GB if 32BIT
> + default MAXPHYSMEM_1GB if 32BIT
> default MAXPHYSMEM_2GB if 64BIT && CMODEL_MEDLOW
> default MAXPHYSMEM_128GB if 64BIT && CMODEL_MEDANY
>
> + config MAXPHYSMEM_1GB
> + bool "1GiB"
> config MAXPHYSMEM_2GB
> bool "2GiB"
> config MAXPHYSMEM_128GB
> --
> 2.25.1
>
>
> _______________________________________________
> linux-riscv mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-riscv
Regards,
Anup
Hi Palmer,
On Thu, Jan 7, 2021 at 2:57 PM Atish Patra <[email protected]> wrote:
>
> This series fixes various issues observed in latest kernel on RV32.
> The first two patches fixes an resource tree introduced in 5.11-rc1
> while the last two fixes the case where 2GB physical memory is used
> on RV32.
>
> There are may be better way to fix the issue pointed out in PATCH 3
> as it seems a generic kernel issue where kernel pointers can not use
> last 4k of addressable memory. I am open to other better alternate
> suggestions.
>
> Atish Patra (4):
> RISC-V: Do not allocate memblock while iterating reserved memblocks
> RISC-V: Set current memblock limit
> RISC-V: Fix L1_CACHE_BYTES for RV32
> RISC-V: Fix maximum allowed phsyical memory for RV32
Please consider these fixes for Linux-5.11-rcX.
>
> arch/riscv/Kconfig | 6 ++++--
> arch/riscv/include/asm/cache.h | 4 ++++
> arch/riscv/kernel/setup.c | 24 +++++++++++++-----------
> arch/riscv/mm/init.c | 16 ++++++++++++++--
> 4 files changed, 35 insertions(+), 15 deletions(-)
>
> --
> 2.25.1
>
>
> _______________________________________________
> linux-riscv mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-riscv
Regards,
Anup
On Sun, Jan 10, 2021 at 7:59 PM Anup Patel <[email protected]> wrote:
>
> On Thu, Jan 7, 2021 at 2:57 PM Atish Patra <[email protected]> wrote:
> >
> > Currently, linux kernel can not use last 4k bytes of addressable space because
> > IS_ERR_VALUE macro treats those as an error. This will be an issue for RV32
> > as any memblock allocator potentially allocate chunk of memory from the end
> > of DRAM (2GB) leading bad address error even though the address was technically
> > valid.
> >
> > Fix this issue by limiting the memblock if available memory spans the entire
> > address space.
> >
> > Signed-off-by: Atish Patra <[email protected]>
> > ---
> > arch/riscv/mm/init.c | 16 ++++++++++++++--
> > 1 file changed, 14 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
> > index bf5379135e39..da53902ef0fc 100644
> > --- a/arch/riscv/mm/init.c
> > +++ b/arch/riscv/mm/init.c
> > @@ -157,9 +157,10 @@ static void __init setup_initrd(void)
> > void __init setup_bootmem(void)
> > {
> > phys_addr_t mem_start = 0;
> > - phys_addr_t start, end = 0;
> > + phys_addr_t start, dram_end, end = 0;
> > phys_addr_t vmlinux_end = __pa_symbol(&_end);
> > phys_addr_t vmlinux_start = __pa_symbol(&_start);
> > + phys_addr_t max_mapped_addr = __pa(PHYS_ADDR_MAX);
>
> Using PHYS_ADDR_MAX as the max virtual address does not look right.
>
> Better use __pa(~(ulong)0) here. Otherwise looks good to me.
>
ok will change it. Thanks for the review.
> Reviewed-by: Anup Patel <[email protected]>
>
> > u64 i;
> >
> > /* Find the memory region containing the kernel */
> > @@ -181,7 +182,18 @@ void __init setup_bootmem(void)
> > /* Reserve from the start of the kernel to the end of the kernel */
> > memblock_reserve(vmlinux_start, vmlinux_end - vmlinux_start);
> >
> > - max_pfn = PFN_DOWN(memblock_end_of_DRAM());
> > + dram_end = memblock_end_of_DRAM();
> > +
> > + /*
> > + * memblock allocator is not aware of the fact that last 4K bytes of
> > + * the addressable memory can not be mapped because of IS_ERR_VALUE
> > + * macro. Make sure that last 4k bytes are not usable by memblock
> > + * if end of dram is equal to maximum addressable memory.
> > + */
> > + if (max_mapped_addr == (dram_end - 1))
> > + memblock_set_current_limit(max_mapped_addr - 4096);
> > +
> > + max_pfn = PFN_DOWN(dram_end);
> > max_low_pfn = max_pfn;
> > dma32_phys_limit = min(4UL * SZ_1G, (unsigned long)PFN_PHYS(max_low_pfn));
> > set_max_mapnr(max_low_pfn);
> > --
> > 2.25.1
> >
> >
> > _______________________________________________
> > linux-riscv mailing list
> > [email protected]
> > http://lists.infradead.org/mailman/listinfo/linux-riscv
>
> Regards,
> Anup
>
> _______________________________________________
> linux-riscv mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-riscv
--
Regards,
Atish
On Thu, 07 Jan 2021 01:26:51 PST (-0800), Atish Patra wrote:
> SMP_CACHE_BYTES/L1_CACHE_BYTES should be defined as 32 instead of
> 64 for RV32. Otherwise, there will be hole of 32 bytes with each memblock
> allocation if it is requested to be aligned with SMP_CACHE_BYTES.
>
> Signed-off-by: Atish Patra <[email protected]>
> ---
> arch/riscv/include/asm/cache.h | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/arch/riscv/include/asm/cache.h b/arch/riscv/include/asm/cache.h
> index 9b58b104559e..c9c669ea2fe6 100644
> --- a/arch/riscv/include/asm/cache.h
> +++ b/arch/riscv/include/asm/cache.h
> @@ -7,7 +7,11 @@
> #ifndef _ASM_RISCV_CACHE_H
> #define _ASM_RISCV_CACHE_H
>
> +#ifdef CONFIG_64BIT
> #define L1_CACHE_SHIFT 6
> +#else
> +#define L1_CACHE_SHIFT 5
> +#endif
>
> #define L1_CACHE_BYTES (1 << L1_CACHE_SHIFT)
Should we not instead just
#define SMP_CACHE_BYTES L1_CACHE_BYTES
like a handful of architectures do?
The cache size is sort of fake here, as we don't have any non-coherent
mechanisms, but IIRC we wrote somewhere that it's recommended to have 64-byte
cache lines in RISC-V implementations as software may assume that for
performance reasons. Not really a strong reason, but I'd prefer to just make
these match.
On Thu, 07 Jan 2021 01:26:48 PST (-0800), Atish Patra wrote:
> This series fixes various issues observed in latest kernel on RV32.
> The first two patches fixes an resource tree introduced in 5.11-rc1
> while the last two fixes the case where 2GB physical memory is used
> on RV32.
>
> There are may be better way to fix the issue pointed out in PATCH 3
> as it seems a generic kernel issue where kernel pointers can not use
> last 4k of addressable memory. I am open to other better alternate
> suggestions.
>
> Atish Patra (4):
> RISC-V: Do not allocate memblock while iterating reserved memblocks
> RISC-V: Set current memblock limit
> RISC-V: Fix L1_CACHE_BYTES for RV32
> RISC-V: Fix maximum allowed phsyical memory for RV32
>
> arch/riscv/Kconfig | 6 ++++--
> arch/riscv/include/asm/cache.h | 4 ++++
> arch/riscv/kernel/setup.c | 24 +++++++++++++-----------
> arch/riscv/mm/init.c | 16 ++++++++++++++--
> 4 files changed, 35 insertions(+), 15 deletions(-)
I took all of them but that L1_CACHE_BYTES one, which I had a comment on.
Thanks!
On Wed, 13 Jan 2021 21:09:36 PST (-0800), Palmer Dabbelt wrote:
> On Thu, 07 Jan 2021 01:26:48 PST (-0800), Atish Patra wrote:
>> This series fixes various issues observed in latest kernel on RV32.
>> The first two patches fixes an resource tree introduced in 5.11-rc1
>> while the last two fixes the case where 2GB physical memory is used
>> on RV32.
>>
>> There are may be better way to fix the issue pointed out in PATCH 3
>> as it seems a generic kernel issue where kernel pointers can not use
>> last 4k of addressable memory. I am open to other better alternate
>> suggestions.
>>
>> Atish Patra (4):
>> RISC-V: Do not allocate memblock while iterating reserved memblocks
>> RISC-V: Set current memblock limit
>> RISC-V: Fix L1_CACHE_BYTES for RV32
>> RISC-V: Fix maximum allowed phsyical memory for RV32
>>
>> arch/riscv/Kconfig | 6 ++++--
>> arch/riscv/include/asm/cache.h | 4 ++++
>> arch/riscv/kernel/setup.c | 24 +++++++++++++-----------
>> arch/riscv/mm/init.c | 16 ++++++++++++++--
>> 4 files changed, 35 insertions(+), 15 deletions(-)
>
> I took all of them but that L1_CACHE_BYTES one, which I had a comment on.
> Thanks!
Oops, I just saw the v2. I took those instead, the comment still applies.
On Wed, Jan 13, 2021 at 9:10 PM Palmer Dabbelt <[email protected]> wrote:
>
> On Thu, 07 Jan 2021 01:26:51 PST (-0800), Atish Patra wrote:
> > SMP_CACHE_BYTES/L1_CACHE_BYTES should be defined as 32 instead of
> > 64 for RV32. Otherwise, there will be hole of 32 bytes with each memblock
> > allocation if it is requested to be aligned with SMP_CACHE_BYTES.
> >
> > Signed-off-by: Atish Patra <[email protected]>
> > ---
> > arch/riscv/include/asm/cache.h | 4 ++++
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/arch/riscv/include/asm/cache.h b/arch/riscv/include/asm/cache.h
> > index 9b58b104559e..c9c669ea2fe6 100644
> > --- a/arch/riscv/include/asm/cache.h
> > +++ b/arch/riscv/include/asm/cache.h
> > @@ -7,7 +7,11 @@
> > #ifndef _ASM_RISCV_CACHE_H
> > #define _ASM_RISCV_CACHE_H
> >
> > +#ifdef CONFIG_64BIT
> > #define L1_CACHE_SHIFT 6
> > +#else
> > +#define L1_CACHE_SHIFT 5
> > +#endif
> >
> > #define L1_CACHE_BYTES (1 << L1_CACHE_SHIFT)
>
> Should we not instead just
>
> #define SMP_CACHE_BYTES L1_CACHE_BYTES
>
> like a handful of architectures do?
>
The generic code already defines it that way in include/linux/cache.h
> The cache size is sort of fake here, as we don't have any non-coherent
> mechanisms, but IIRC we wrote somewhere that it's recommended to have 64-byte
> cache lines in RISC-V implementations as software may assume that for
> performance reasons. Not really a strong reason, but I'd prefer to just make
> these match.
>
If it is documented somewhere in the kernel, we should update that. I
think SMP_CACHE_BYTES being 64
actually degrades the performance as there will be a fragmented memory
blocks with 32 bit bytes gap wherever
SMP_CACHE_BYTES is used as an alignment requirement.
In addition to that, Geert Uytterhoeven mentioned some panic on vex32
without this patch.
I didn't see anything in Qemu though.
> _______________________________________________
> linux-riscv mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-riscv
--
Regards,
Atish
On Thu, 14 Jan 2021 10:33:01 PST (-0800), [email protected] wrote:
> On Wed, Jan 13, 2021 at 9:10 PM Palmer Dabbelt <[email protected]> wrote:
>>
>> On Thu, 07 Jan 2021 01:26:51 PST (-0800), Atish Patra wrote:
>> > SMP_CACHE_BYTES/L1_CACHE_BYTES should be defined as 32 instead of
>> > 64 for RV32. Otherwise, there will be hole of 32 bytes with each memblock
>> > allocation if it is requested to be aligned with SMP_CACHE_BYTES.
>> >
>> > Signed-off-by: Atish Patra <[email protected]>
>> > ---
>> > arch/riscv/include/asm/cache.h | 4 ++++
>> > 1 file changed, 4 insertions(+)
>> >
>> > diff --git a/arch/riscv/include/asm/cache.h b/arch/riscv/include/asm/cache.h
>> > index 9b58b104559e..c9c669ea2fe6 100644
>> > --- a/arch/riscv/include/asm/cache.h
>> > +++ b/arch/riscv/include/asm/cache.h
>> > @@ -7,7 +7,11 @@
>> > #ifndef _ASM_RISCV_CACHE_H
>> > #define _ASM_RISCV_CACHE_H
>> >
>> > +#ifdef CONFIG_64BIT
>> > #define L1_CACHE_SHIFT 6
>> > +#else
>> > +#define L1_CACHE_SHIFT 5
>> > +#endif
>> >
>> > #define L1_CACHE_BYTES (1 << L1_CACHE_SHIFT)
>>
>> Should we not instead just
>>
>> #define SMP_CACHE_BYTES L1_CACHE_BYTES
>>
>> like a handful of architectures do?
>>
>
> The generic code already defines it that way in include/linux/cache.h
>
>> The cache size is sort of fake here, as we don't have any non-coherent
>> mechanisms, but IIRC we wrote somewhere that it's recommended to have 64-byte
>> cache lines in RISC-V implementations as software may assume that for
>> performance reasons. Not really a strong reason, but I'd prefer to just make
>> these match.
>>
>
> If it is documented somewhere in the kernel, we should update that. I
> think SMP_CACHE_BYTES being 64
> actually degrades the performance as there will be a fragmented memory
> blocks with 32 bit bytes gap wherever
> SMP_CACHE_BYTES is used as an alignment requirement.
I don't buy that: if you're trying to align to the cache size then the gaps are
the whole point. IIUC the 64-byte cache lines come from DDR, not XLEN, so
there's really no reason for these to be different between the base ISAs.
> In addition to that, Geert Uytterhoeven mentioned some panic on vex32
> without this patch.
> I didn't see anything in Qemu though.
Something like that is probably only going to show up on real hardware, QEMU
doesn't really do anything with the cache line size. That said, as there's
nothing in our kernel now related to non-coherent memory there really should
only be performance issue (at least until we have non-coherent systems).
I'd bet that the change is just masking some other bug, either in the software
or the hardware. I'd prefer to root cause this rather than just working around
it, as it'll probably come back later and in a more difficult way to find.
>> _______________________________________________
>> linux-riscv mailing list
>> [email protected]
>> http://lists.infradead.org/mailman/listinfo/linux-riscv
On Thu, Jan 14, 2021 at 11:46 AM Palmer Dabbelt <[email protected]> wrote:
>
> On Thu, 14 Jan 2021 10:33:01 PST (-0800), [email protected] wrote:
> > On Wed, Jan 13, 2021 at 9:10 PM Palmer Dabbelt <[email protected]> wrote:
> >>
> >> On Thu, 07 Jan 2021 01:26:51 PST (-0800), Atish Patra wrote:
> >> > SMP_CACHE_BYTES/L1_CACHE_BYTES should be defined as 32 instead of
> >> > 64 for RV32. Otherwise, there will be hole of 32 bytes with each memblock
> >> > allocation if it is requested to be aligned with SMP_CACHE_BYTES.
> >> >
> >> > Signed-off-by: Atish Patra <[email protected]>
> >> > ---
> >> > arch/riscv/include/asm/cache.h | 4 ++++
> >> > 1 file changed, 4 insertions(+)
> >> >
> >> > diff --git a/arch/riscv/include/asm/cache.h b/arch/riscv/include/asm/cache.h
> >> > index 9b58b104559e..c9c669ea2fe6 100644
> >> > --- a/arch/riscv/include/asm/cache.h
> >> > +++ b/arch/riscv/include/asm/cache.h
> >> > @@ -7,7 +7,11 @@
> >> > #ifndef _ASM_RISCV_CACHE_H
> >> > #define _ASM_RISCV_CACHE_H
> >> >
> >> > +#ifdef CONFIG_64BIT
> >> > #define L1_CACHE_SHIFT 6
> >> > +#else
> >> > +#define L1_CACHE_SHIFT 5
> >> > +#endif
> >> >
> >> > #define L1_CACHE_BYTES (1 << L1_CACHE_SHIFT)
> >>
> >> Should we not instead just
> >>
> >> #define SMP_CACHE_BYTES L1_CACHE_BYTES
> >>
> >> like a handful of architectures do?
> >>
> >
> > The generic code already defines it that way in include/linux/cache.h
> >
> >> The cache size is sort of fake here, as we don't have any non-coherent
> >> mechanisms, but IIRC we wrote somewhere that it's recommended to have 64-byte
> >> cache lines in RISC-V implementations as software may assume that for
> >> performance reasons. Not really a strong reason, but I'd prefer to just make
> >> these match.
> >>
> >
> > If it is documented somewhere in the kernel, we should update that. I
> > think SMP_CACHE_BYTES being 64
> > actually degrades the performance as there will be a fragmented memory
> > blocks with 32 bit bytes gap wherever
> > SMP_CACHE_BYTES is used as an alignment requirement.
>
> I don't buy that: if you're trying to align to the cache size then the gaps are
> the whole point. IIUC the 64-byte cache lines come from DDR, not XLEN, so
> there's really no reason for these to be different between the base ISAs.
>
Got your point. I noticed this when fixing the resource tree issue
where the SMP_CACHE_BYTES
alignment was not intentional but causing the issue. The real issue
was solved via another patch in this series though.
Just to clarify, if the allocation function intends to allocate
consecutive memory, it should use 32 instead of SMP_CACHE_BYTES.
This will lead to a #ifdef macro in the code.
> > In addition to that, Geert Uytterhoeven mentioned some panic on vex32
> > without this patch.
> > I didn't see anything in Qemu though.
>
> Something like that is probably only going to show up on real hardware, QEMU
> doesn't really do anything with the cache line size. That said, as there's
> nothing in our kernel now related to non-coherent memory there really should
> only be performance issue (at least until we have non-coherent systems).
>
> I'd bet that the change is just masking some other bug, either in the software
> or the hardware. I'd prefer to root cause this rather than just working around
> it, as it'll probably come back later and in a more difficult way to find.
>
Agreed. @Geert Uytterhoeven Can you do a further analysis of the panic
you were saying ?
We may need to change an alignment requirement to 32 for RV32 manually
at some place in code.
> >> _______________________________________________
> >> linux-riscv mailing list
> >> [email protected]
> >> http://lists.infradead.org/mailman/listinfo/linux-riscv
--
Regards,
Atish
On Thu, 14 Jan 2021 13:11:14 PST (-0800), [email protected] wrote:
> On Thu, Jan 14, 2021 at 11:46 AM Palmer Dabbelt <[email protected]> wrote:
>>
>> On Thu, 14 Jan 2021 10:33:01 PST (-0800), [email protected] wrote:
>> > On Wed, Jan 13, 2021 at 9:10 PM Palmer Dabbelt <[email protected]> wrote:
>> >>
>> >> On Thu, 07 Jan 2021 01:26:51 PST (-0800), Atish Patra wrote:
>> >> > SMP_CACHE_BYTES/L1_CACHE_BYTES should be defined as 32 instead of
>> >> > 64 for RV32. Otherwise, there will be hole of 32 bytes with each memblock
>> >> > allocation if it is requested to be aligned with SMP_CACHE_BYTES.
>> >> >
>> >> > Signed-off-by: Atish Patra <[email protected]>
>> >> > ---
>> >> > arch/riscv/include/asm/cache.h | 4 ++++
>> >> > 1 file changed, 4 insertions(+)
>> >> >
>> >> > diff --git a/arch/riscv/include/asm/cache.h b/arch/riscv/include/asm/cache.h
>> >> > index 9b58b104559e..c9c669ea2fe6 100644
>> >> > --- a/arch/riscv/include/asm/cache.h
>> >> > +++ b/arch/riscv/include/asm/cache.h
>> >> > @@ -7,7 +7,11 @@
>> >> > #ifndef _ASM_RISCV_CACHE_H
>> >> > #define _ASM_RISCV_CACHE_H
>> >> >
>> >> > +#ifdef CONFIG_64BIT
>> >> > #define L1_CACHE_SHIFT 6
>> >> > +#else
>> >> > +#define L1_CACHE_SHIFT 5
>> >> > +#endif
>> >> >
>> >> > #define L1_CACHE_BYTES (1 << L1_CACHE_SHIFT)
>> >>
>> >> Should we not instead just
>> >>
>> >> #define SMP_CACHE_BYTES L1_CACHE_BYTES
>> >>
>> >> like a handful of architectures do?
>> >>
>> >
>> > The generic code already defines it that way in include/linux/cache.h
>> >
>> >> The cache size is sort of fake here, as we don't have any non-coherent
>> >> mechanisms, but IIRC we wrote somewhere that it's recommended to have 64-byte
>> >> cache lines in RISC-V implementations as software may assume that for
>> >> performance reasons. Not really a strong reason, but I'd prefer to just make
>> >> these match.
>> >>
>> >
>> > If it is documented somewhere in the kernel, we should update that. I
>> > think SMP_CACHE_BYTES being 64
>> > actually degrades the performance as there will be a fragmented memory
>> > blocks with 32 bit bytes gap wherever
>> > SMP_CACHE_BYTES is used as an alignment requirement.
>>
>> I don't buy that: if you're trying to align to the cache size then the gaps are
>> the whole point. IIUC the 64-byte cache lines come from DDR, not XLEN, so
>> there's really no reason for these to be different between the base ISAs.
>>
>
> Got your point. I noticed this when fixing the resource tree issue
> where the SMP_CACHE_BYTES
> alignment was not intentional but causing the issue. The real issue
> was solved via another patch in this series though.
>
> Just to clarify, if the allocation function intends to allocate
> consecutive memory, it should use 32 instead of SMP_CACHE_BYTES.
> This will lead to a #ifdef macro in the code.
Well, if you want to be allocating XLEN-byte sized chunks then you should use
an XLEN-based define and if you want to allocate cache-line-sized chunks then
you should use some cache-line-based define. I guess I'd have to see the code
to know if an ifdef is the right way to go, but the right thing is probably to
just change over to something that already exists.
>> > In addition to that, Geert Uytterhoeven mentioned some panic on vex32
>> > without this patch.
>> > I didn't see anything in Qemu though.
>>
>> Something like that is probably only going to show up on real hardware, QEMU
>> doesn't really do anything with the cache line size. That said, as there's
>> nothing in our kernel now related to non-coherent memory there really should
>> only be performance issue (at least until we have non-coherent systems).
>>
>> I'd bet that the change is just masking some other bug, either in the software
>> or the hardware. I'd prefer to root cause this rather than just working around
>> it, as it'll probably come back later and in a more difficult way to find.
>>
>
> Agreed. @Geert Uytterhoeven Can you do a further analysis of the panic
> you were saying ?
> We may need to change an alignment requirement to 32 for RV32 manually
> at some place in code.
>
>> >> _______________________________________________
>> >> linux-riscv mailing list
>> >> [email protected]
>> >> http://lists.infradead.org/mailman/listinfo/linux-riscv
Hi Atish,
On Thu, Jan 14, 2021 at 10:11 PM Atish Patra <[email protected]> wrote:
> On Thu, Jan 14, 2021 at 11:46 AM Palmer Dabbelt <[email protected]> wrote:
> > On Thu, 14 Jan 2021 10:33:01 PST (-0800), [email protected] wrote:
> > > On Wed, Jan 13, 2021 at 9:10 PM Palmer Dabbelt <[email protected]> wrote:
> > >>
> > >> On Thu, 07 Jan 2021 01:26:51 PST (-0800), Atish Patra wrote:
> > >> > SMP_CACHE_BYTES/L1_CACHE_BYTES should be defined as 32 instead of
> > >> > 64 for RV32. Otherwise, there will be hole of 32 bytes with each memblock
> > >> > allocation if it is requested to be aligned with SMP_CACHE_BYTES.
> > >> >
> > >> > Signed-off-by: Atish Patra <[email protected]>
> > >> > ---
> > >> > arch/riscv/include/asm/cache.h | 4 ++++
> > >> > 1 file changed, 4 insertions(+)
> > >> >
> > >> > diff --git a/arch/riscv/include/asm/cache.h b/arch/riscv/include/asm/cache.h
> > >> > index 9b58b104559e..c9c669ea2fe6 100644
> > >> > --- a/arch/riscv/include/asm/cache.h
> > >> > +++ b/arch/riscv/include/asm/cache.h
> > >> > @@ -7,7 +7,11 @@
> > >> > #ifndef _ASM_RISCV_CACHE_H
> > >> > #define _ASM_RISCV_CACHE_H
> > >> >
> > >> > +#ifdef CONFIG_64BIT
> > >> > #define L1_CACHE_SHIFT 6
> > >> > +#else
> > >> > +#define L1_CACHE_SHIFT 5
> > >> > +#endif
> > >> >
> > >> > #define L1_CACHE_BYTES (1 << L1_CACHE_SHIFT)
> > >>
> > >> Should we not instead just
> > >>
> > >> #define SMP_CACHE_BYTES L1_CACHE_BYTES
> > >>
> > >> like a handful of architectures do?
> > >>
> > >
> > > The generic code already defines it that way in include/linux/cache.h
> > >
> > >> The cache size is sort of fake here, as we don't have any non-coherent
> > >> mechanisms, but IIRC we wrote somewhere that it's recommended to have 64-byte
> > >> cache lines in RISC-V implementations as software may assume that for
> > >> performance reasons. Not really a strong reason, but I'd prefer to just make
> > >> these match.
> > >>
> > >
> > > If it is documented somewhere in the kernel, we should update that. I
> > > think SMP_CACHE_BYTES being 64
> > > actually degrades the performance as there will be a fragmented memory
> > > blocks with 32 bit bytes gap wherever
> > > SMP_CACHE_BYTES is used as an alignment requirement.
> >
> > I don't buy that: if you're trying to align to the cache size then the gaps are
> > the whole point. IIUC the 64-byte cache lines come from DDR, not XLEN, so
> > there's really no reason for these to be different between the base ISAs.
> >
>
> Got your point. I noticed this when fixing the resource tree issue
> where the SMP_CACHE_BYTES
> alignment was not intentional but causing the issue. The real issue
> was solved via another patch in this series though.
>
> Just to clarify, if the allocation function intends to allocate
> consecutive memory, it should use 32 instead of SMP_CACHE_BYTES.
> This will lead to a #ifdef macro in the code.
>
> > > In addition to that, Geert Uytterhoeven mentioned some panic on vex32
> > > without this patch.
> > > I didn't see anything in Qemu though.
> >
> > Something like that is probably only going to show up on real hardware, QEMU
> > doesn't really do anything with the cache line size. That said, as there's
> > nothing in our kernel now related to non-coherent memory there really should
> > only be performance issue (at least until we have non-coherent systems).
> >
> > I'd bet that the change is just masking some other bug, either in the software
> > or the hardware. I'd prefer to root cause this rather than just working around
> > it, as it'll probably come back later and in a more difficult way to find.
> >
>
> Agreed. @Geert Uytterhoeven Can you do a further analysis of the panic
> you were saying ?
> We may need to change an alignment requirement to 32 for RV32 manually
> at some place in code.
My findings were in
https://lore.kernel.org/linux-riscv/CAMuHMdWf6K-5y02+WJ6Khu1cD6P0n5x1wYQikrECkuNtAA1pgg@mail.gmail.com/
Note that when the memblock.reserved list kept increasing, it kept on
adding the same entry to the list. But that was fixed by "[PATCH 1/4]
RISC-V: Do not allocate memblock while iterating reserved memblocks".
After that, only the (reproducible) "Unable to handle kernel paging
request at virtual address 61636473" was left, always at the same place.
No idea where the actual corruption happened.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Thu, 14 Jan 2021 23:59:04 PST (-0800), [email protected] wrote:
> Hi Atish,
>
> On Thu, Jan 14, 2021 at 10:11 PM Atish Patra <[email protected]> wrote:
>> On Thu, Jan 14, 2021 at 11:46 AM Palmer Dabbelt <[email protected]> wrote:
>> > On Thu, 14 Jan 2021 10:33:01 PST (-0800), [email protected] wrote:
>> > > On Wed, Jan 13, 2021 at 9:10 PM Palmer Dabbelt <[email protected]> wrote:
>> > >>
>> > >> On Thu, 07 Jan 2021 01:26:51 PST (-0800), Atish Patra wrote:
>> > >> > SMP_CACHE_BYTES/L1_CACHE_BYTES should be defined as 32 instead of
>> > >> > 64 for RV32. Otherwise, there will be hole of 32 bytes with each memblock
>> > >> > allocation if it is requested to be aligned with SMP_CACHE_BYTES.
>> > >> >
>> > >> > Signed-off-by: Atish Patra <[email protected]>
>> > >> > ---
>> > >> > arch/riscv/include/asm/cache.h | 4 ++++
>> > >> > 1 file changed, 4 insertions(+)
>> > >> >
>> > >> > diff --git a/arch/riscv/include/asm/cache.h b/arch/riscv/include/asm/cache.h
>> > >> > index 9b58b104559e..c9c669ea2fe6 100644
>> > >> > --- a/arch/riscv/include/asm/cache.h
>> > >> > +++ b/arch/riscv/include/asm/cache.h
>> > >> > @@ -7,7 +7,11 @@
>> > >> > #ifndef _ASM_RISCV_CACHE_H
>> > >> > #define _ASM_RISCV_CACHE_H
>> > >> >
>> > >> > +#ifdef CONFIG_64BIT
>> > >> > #define L1_CACHE_SHIFT 6
>> > >> > +#else
>> > >> > +#define L1_CACHE_SHIFT 5
>> > >> > +#endif
>> > >> >
>> > >> > #define L1_CACHE_BYTES (1 << L1_CACHE_SHIFT)
>> > >>
>> > >> Should we not instead just
>> > >>
>> > >> #define SMP_CACHE_BYTES L1_CACHE_BYTES
>> > >>
>> > >> like a handful of architectures do?
>> > >>
>> > >
>> > > The generic code already defines it that way in include/linux/cache.h
>> > >
>> > >> The cache size is sort of fake here, as we don't have any non-coherent
>> > >> mechanisms, but IIRC we wrote somewhere that it's recommended to have 64-byte
>> > >> cache lines in RISC-V implementations as software may assume that for
>> > >> performance reasons. Not really a strong reason, but I'd prefer to just make
>> > >> these match.
>> > >>
>> > >
>> > > If it is documented somewhere in the kernel, we should update that. I
>> > > think SMP_CACHE_BYTES being 64
>> > > actually degrades the performance as there will be a fragmented memory
>> > > blocks with 32 bit bytes gap wherever
>> > > SMP_CACHE_BYTES is used as an alignment requirement.
>> >
>> > I don't buy that: if you're trying to align to the cache size then the gaps are
>> > the whole point. IIUC the 64-byte cache lines come from DDR, not XLEN, so
>> > there's really no reason for these to be different between the base ISAs.
>> >
>>
>> Got your point. I noticed this when fixing the resource tree issue
>> where the SMP_CACHE_BYTES
>> alignment was not intentional but causing the issue. The real issue
>> was solved via another patch in this series though.
>>
>> Just to clarify, if the allocation function intends to allocate
>> consecutive memory, it should use 32 instead of SMP_CACHE_BYTES.
>> This will lead to a #ifdef macro in the code.
>>
>> > > In addition to that, Geert Uytterhoeven mentioned some panic on vex32
>> > > without this patch.
>> > > I didn't see anything in Qemu though.
>> >
>> > Something like that is probably only going to show up on real hardware, QEMU
>> > doesn't really do anything with the cache line size. That said, as there's
>> > nothing in our kernel now related to non-coherent memory there really should
>> > only be performance issue (at least until we have non-coherent systems).
>> >
>> > I'd bet that the change is just masking some other bug, either in the software
>> > or the hardware. I'd prefer to root cause this rather than just working around
>> > it, as it'll probably come back later and in a more difficult way to find.
>> >
>>
>> Agreed. @Geert Uytterhoeven Can you do a further analysis of the panic
>> you were saying ?
>> We may need to change an alignment requirement to 32 for RV32 manually
>> at some place in code.
>
> My findings were in
> https://lore.kernel.org/linux-riscv/CAMuHMdWf6K-5y02+WJ6Khu1cD6P0n5x1wYQikrECkuNtAA1pgg@mail.gmail.com/
>
> Note that when the memblock.reserved list kept increasing, it kept on
> adding the same entry to the list. But that was fixed by "[PATCH 1/4]
> RISC-V: Do not allocate memblock while iterating reserved memblocks".
>
> After that, only the (reproducible) "Unable to handle kernel paging
> request at virtual address 61636473" was left, always at the same place.
> No idea where the actual corruption happened.
Thanks. Presumably I need an FPGA to run this? That will make it tricky to
find anything here on my end.
>
> Gr{oetje,eeting}s,
>
> Geert
On Thu, Jan 14, 2021 at 11:59 PM Geert Uytterhoeven
<[email protected]> wrote:
>
> Hi Atish,
>
> On Thu, Jan 14, 2021 at 10:11 PM Atish Patra <[email protected]> wrote:
> > On Thu, Jan 14, 2021 at 11:46 AM Palmer Dabbelt <[email protected]> wrote:
> > > On Thu, 14 Jan 2021 10:33:01 PST (-0800), [email protected] wrote:
> > > > On Wed, Jan 13, 2021 at 9:10 PM Palmer Dabbelt <[email protected]> wrote:
> > > >>
> > > >> On Thu, 07 Jan 2021 01:26:51 PST (-0800), Atish Patra wrote:
> > > >> > SMP_CACHE_BYTES/L1_CACHE_BYTES should be defined as 32 instead of
> > > >> > 64 for RV32. Otherwise, there will be hole of 32 bytes with each memblock
> > > >> > allocation if it is requested to be aligned with SMP_CACHE_BYTES.
> > > >> >
> > > >> > Signed-off-by: Atish Patra <[email protected]>
> > > >> > ---
> > > >> > arch/riscv/include/asm/cache.h | 4 ++++
> > > >> > 1 file changed, 4 insertions(+)
> > > >> >
> > > >> > diff --git a/arch/riscv/include/asm/cache.h b/arch/riscv/include/asm/cache.h
> > > >> > index 9b58b104559e..c9c669ea2fe6 100644
> > > >> > --- a/arch/riscv/include/asm/cache.h
> > > >> > +++ b/arch/riscv/include/asm/cache.h
> > > >> > @@ -7,7 +7,11 @@
> > > >> > #ifndef _ASM_RISCV_CACHE_H
> > > >> > #define _ASM_RISCV_CACHE_H
> > > >> >
> > > >> > +#ifdef CONFIG_64BIT
> > > >> > #define L1_CACHE_SHIFT 6
> > > >> > +#else
> > > >> > +#define L1_CACHE_SHIFT 5
> > > >> > +#endif
> > > >> >
> > > >> > #define L1_CACHE_BYTES (1 << L1_CACHE_SHIFT)
> > > >>
> > > >> Should we not instead just
> > > >>
> > > >> #define SMP_CACHE_BYTES L1_CACHE_BYTES
> > > >>
> > > >> like a handful of architectures do?
> > > >>
> > > >
> > > > The generic code already defines it that way in include/linux/cache.h
> > > >
> > > >> The cache size is sort of fake here, as we don't have any non-coherent
> > > >> mechanisms, but IIRC we wrote somewhere that it's recommended to have 64-byte
> > > >> cache lines in RISC-V implementations as software may assume that for
> > > >> performance reasons. Not really a strong reason, but I'd prefer to just make
> > > >> these match.
> > > >>
> > > >
> > > > If it is documented somewhere in the kernel, we should update that. I
> > > > think SMP_CACHE_BYTES being 64
> > > > actually degrades the performance as there will be a fragmented memory
> > > > blocks with 32 bit bytes gap wherever
> > > > SMP_CACHE_BYTES is used as an alignment requirement.
> > >
> > > I don't buy that: if you're trying to align to the cache size then the gaps are
> > > the whole point. IIUC the 64-byte cache lines come from DDR, not XLEN, so
> > > there's really no reason for these to be different between the base ISAs.
> > >
> >
> > Got your point. I noticed this when fixing the resource tree issue
> > where the SMP_CACHE_BYTES
> > alignment was not intentional but causing the issue. The real issue
> > was solved via another patch in this series though.
> >
> > Just to clarify, if the allocation function intends to allocate
> > consecutive memory, it should use 32 instead of SMP_CACHE_BYTES.
> > This will lead to a #ifdef macro in the code.
> >
> > > > In addition to that, Geert Uytterhoeven mentioned some panic on vex32
> > > > without this patch.
> > > > I didn't see anything in Qemu though.
> > >
> > > Something like that is probably only going to show up on real hardware, QEMU
> > > doesn't really do anything with the cache line size. That said, as there's
> > > nothing in our kernel now related to non-coherent memory there really should
> > > only be performance issue (at least until we have non-coherent systems).
> > >
> > > I'd bet that the change is just masking some other bug, either in the software
> > > or the hardware. I'd prefer to root cause this rather than just working around
> > > it, as it'll probably come back later and in a more difficult way to find.
> > >
> >
> > Agreed. @Geert Uytterhoeven Can you do a further analysis of the panic
> > you were saying ?
> > We may need to change an alignment requirement to 32 for RV32 manually
> > at some place in code.
>
> My findings were in
> https://lore.kernel.org/linux-riscv/CAMuHMdWf6K-5y02+WJ6Khu1cD6P0n5x1wYQikrECkuNtAA1pgg@mail.gmail.com/
>
> Note that when the memblock.reserved list kept increasing, it kept on
> adding the same entry to the list. But that was fixed by "[PATCH 1/4]
> RISC-V: Do not allocate memblock while iterating reserved memblocks".
>
> After that, only the (reproducible) "Unable to handle kernel paging
> request at virtual address 61636473" was left, always at the same place.
> No idea where the actual corruption happened.
>
Yes. I was asking about this panic. I don't have the litex fpga to
reproduce this as well.
Can you take a look at the epc & ra to figure out where exactly is the fault ?
That will help to understand the real cause for this panic.
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
> -- Linus Torvalds
--
Regards,
Atish
Hi Atish,
On Sat, Jan 16, 2021 at 2:39 AM Atish Patra <[email protected]> wrote:
> On Thu, Jan 14, 2021 at 11:59 PM Geert Uytterhoeven
> <[email protected]> wrote:
> > On Thu, Jan 14, 2021 at 10:11 PM Atish Patra <[email protected]> wrote:
> > > On Thu, Jan 14, 2021 at 11:46 AM Palmer Dabbelt <[email protected]> wrote:
> > > > On Thu, 14 Jan 2021 10:33:01 PST (-0800), [email protected] wrote:
> > > > > On Wed, Jan 13, 2021 at 9:10 PM Palmer Dabbelt <[email protected]> wrote:
> > > > >>
> > > > >> On Thu, 07 Jan 2021 01:26:51 PST (-0800), Atish Patra wrote:
> > > > >> > SMP_CACHE_BYTES/L1_CACHE_BYTES should be defined as 32 instead of
> > > > >> > 64 for RV32. Otherwise, there will be hole of 32 bytes with each memblock
> > > > >> > allocation if it is requested to be aligned with SMP_CACHE_BYTES.
> > > > >> >
> > > > >> > Signed-off-by: Atish Patra <[email protected]>
> > > > >> > ---
> > > > >> > arch/riscv/include/asm/cache.h | 4 ++++
> > > > >> > 1 file changed, 4 insertions(+)
> > > > >> >
> > > > >> > diff --git a/arch/riscv/include/asm/cache.h b/arch/riscv/include/asm/cache.h
> > > > >> > index 9b58b104559e..c9c669ea2fe6 100644
> > > > >> > --- a/arch/riscv/include/asm/cache.h
> > > > >> > +++ b/arch/riscv/include/asm/cache.h
> > > > >> > @@ -7,7 +7,11 @@
> > > > >> > #ifndef _ASM_RISCV_CACHE_H
> > > > >> > #define _ASM_RISCV_CACHE_H
> > > > >> >
> > > > >> > +#ifdef CONFIG_64BIT
> > > > >> > #define L1_CACHE_SHIFT 6
> > > > >> > +#else
> > > > >> > +#define L1_CACHE_SHIFT 5
> > > > >> > +#endif
> > > > >> >
> > > > >> > #define L1_CACHE_BYTES (1 << L1_CACHE_SHIFT)
> > > > >>
> > > > >> Should we not instead just
> > > > >>
> > > > >> #define SMP_CACHE_BYTES L1_CACHE_BYTES
> > > > >>
> > > > >> like a handful of architectures do?
> > > > >>
> > > > >
> > > > > The generic code already defines it that way in include/linux/cache.h
> > > > >
> > > > >> The cache size is sort of fake here, as we don't have any non-coherent
> > > > >> mechanisms, but IIRC we wrote somewhere that it's recommended to have 64-byte
> > > > >> cache lines in RISC-V implementations as software may assume that for
> > > > >> performance reasons. Not really a strong reason, but I'd prefer to just make
> > > > >> these match.
> > > > >>
> > > > >
> > > > > If it is documented somewhere in the kernel, we should update that. I
> > > > > think SMP_CACHE_BYTES being 64
> > > > > actually degrades the performance as there will be a fragmented memory
> > > > > blocks with 32 bit bytes gap wherever
> > > > > SMP_CACHE_BYTES is used as an alignment requirement.
> > > >
> > > > I don't buy that: if you're trying to align to the cache size then the gaps are
> > > > the whole point. IIUC the 64-byte cache lines come from DDR, not XLEN, so
> > > > there's really no reason for these to be different between the base ISAs.
> > > >
> > >
> > > Got your point. I noticed this when fixing the resource tree issue
> > > where the SMP_CACHE_BYTES
> > > alignment was not intentional but causing the issue. The real issue
> > > was solved via another patch in this series though.
> > >
> > > Just to clarify, if the allocation function intends to allocate
> > > consecutive memory, it should use 32 instead of SMP_CACHE_BYTES.
> > > This will lead to a #ifdef macro in the code.
> > >
> > > > > In addition to that, Geert Uytterhoeven mentioned some panic on vex32
> > > > > without this patch.
> > > > > I didn't see anything in Qemu though.
> > > >
> > > > Something like that is probably only going to show up on real hardware, QEMU
> > > > doesn't really do anything with the cache line size. That said, as there's
> > > > nothing in our kernel now related to non-coherent memory there really should
> > > > only be performance issue (at least until we have non-coherent systems).
> > > >
> > > > I'd bet that the change is just masking some other bug, either in the software
> > > > or the hardware. I'd prefer to root cause this rather than just working around
> > > > it, as it'll probably come back later and in a more difficult way to find.
> > > >
> > >
> > > Agreed. @Geert Uytterhoeven Can you do a further analysis of the panic
> > > you were saying ?
> > > We may need to change an alignment requirement to 32 for RV32 manually
> > > at some place in code.
> >
> > My findings were in
> > https://lore.kernel.org/linux-riscv/CAMuHMdWf6K-5y02+WJ6Khu1cD6P0n5x1wYQikrECkuNtAA1pgg@mail.gmail.com/
> >
> > Note that when the memblock.reserved list kept increasing, it kept on
> > adding the same entry to the list. But that was fixed by "[PATCH 1/4]
> > RISC-V: Do not allocate memblock while iterating reserved memblocks".
> >
> > After that, only the (reproducible) "Unable to handle kernel paging
> > request at virtual address 61636473" was left, always at the same place.
> > No idea where the actual corruption happened.
> >
>
> Yes. I was asking about this panic. I don't have the litex fpga to
> reproduce this as well.
> Can you take a look at the epc & ra to figure out where exactly is the fault ?
>
> That will help to understand the real cause for this panic.
[...]
Freeing initrd memory: 8192K
workingset: timestamp_bits=30 max_order=15 bucket_order=0
Block layer SCSI generic (bsg) driver version 0.4 loaded (major 253)
io scheduler mq-deadline registered
io scheduler kyber registered
Unable to handle kernel paging request at virtual address 61636473
Oops [#1]
CPU: 0 PID: 1 Comm: swapper/0 Not tainted
5.11.0-rc3-orangecrab-00068-g267ecb2e2e9d-dirty #37
epc: c000f8e4 ra : c00110e8 sp : c082bc70
gp : c0665948 tp : c0830000 t0 : c08ba500
t1 : 00000002 t2 : 00000000 s0 : c082bc80
s1 : 00000000 a0 : c05e2dec a1 : c08ba4e0
a2 : c0665d38 a3 : 61636473 a4 : f0004003
a5 : f0004000 a6 : c7efffb8 a7 : c08ba4e0
s2 : 01001f00 s3 : c0666000 s4 : c05e2e0c
s5 : c0666000 s6 : 80000000 s7 : 00000006
s8 : c05a4000 s9 : c08ba4e0 s10: c05e2dec
s11: 00000000 t3 : c08ba500 t4 : 00000001
t5 : 00076400 t6 : c08bbb5e
status: 00000120 badaddr: 61636473 cause: 0000000d
---[ end trace 50524759df172195 ]---
Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b
---[ end Kernel panic - not syncing: Attempted to kill init!
exitcode=0x0000000b ]---
Looking up epc and ra in System.map, closest symbols are:
c000f8b0 t __request_resource
c0010ff4 T __request_region
The above is with a kernel built from my own config, but using
litex_vexriscv_defconfig with https://github.com/geertu/linux branch
litex-v5.11 and commit 718aaf7d1c351035 ("RISC-V: Fix L1_CACHE_BYTES for
RV32") reverted gives the exact same results.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Hi Palmer,
On Fri, Jan 15, 2021 at 11:44 PM Palmer Dabbelt <[email protected]> wrote:
> On Thu, 14 Jan 2021 23:59:04 PST (-0800), [email protected] wrote:
> > On Thu, Jan 14, 2021 at 10:11 PM Atish Patra <[email protected]> wrote:
> >> On Thu, Jan 14, 2021 at 11:46 AM Palmer Dabbelt <[email protected]> wrote:
> >> > On Thu, 14 Jan 2021 10:33:01 PST (-0800), [email protected] wrote:
> >> > > On Wed, Jan 13, 2021 at 9:10 PM Palmer Dabbelt <[email protected]> wrote:
> >> > >>
> >> > >> On Thu, 07 Jan 2021 01:26:51 PST (-0800), Atish Patra wrote:
> >> > >> > SMP_CACHE_BYTES/L1_CACHE_BYTES should be defined as 32 instead of
> >> > >> > 64 for RV32. Otherwise, there will be hole of 32 bytes with each memblock
> >> > >> > allocation if it is requested to be aligned with SMP_CACHE_BYTES.
> >> > >> >
> >> > >> > Signed-off-by: Atish Patra <[email protected]>
> >> > >> > ---
> >> > >> > arch/riscv/include/asm/cache.h | 4 ++++
> >> > >> > 1 file changed, 4 insertions(+)
> >> > >> >
> >> > >> > diff --git a/arch/riscv/include/asm/cache.h b/arch/riscv/include/asm/cache.h
> >> > >> > index 9b58b104559e..c9c669ea2fe6 100644
> >> > >> > --- a/arch/riscv/include/asm/cache.h
> >> > >> > +++ b/arch/riscv/include/asm/cache.h
> >> > >> > @@ -7,7 +7,11 @@
> >> > >> > #ifndef _ASM_RISCV_CACHE_H
> >> > >> > #define _ASM_RISCV_CACHE_H
> >> > >> >
> >> > >> > +#ifdef CONFIG_64BIT
> >> > >> > #define L1_CACHE_SHIFT 6
> >> > >> > +#else
> >> > >> > +#define L1_CACHE_SHIFT 5
> >> > >> > +#endif
> >> > >> >
> >> > >> > #define L1_CACHE_BYTES (1 << L1_CACHE_SHIFT)
> >> > >>
> >> > >> Should we not instead just
> >> > >>
> >> > >> #define SMP_CACHE_BYTES L1_CACHE_BYTES
> >> > >>
> >> > >> like a handful of architectures do?
> >> > >>
> >> > >
> >> > > The generic code already defines it that way in include/linux/cache.h
> >> > >
> >> > >> The cache size is sort of fake here, as we don't have any non-coherent
> >> > >> mechanisms, but IIRC we wrote somewhere that it's recommended to have 64-byte
> >> > >> cache lines in RISC-V implementations as software may assume that for
> >> > >> performance reasons. Not really a strong reason, but I'd prefer to just make
> >> > >> these match.
> >> > >>
> >> > >
> >> > > If it is documented somewhere in the kernel, we should update that. I
> >> > > think SMP_CACHE_BYTES being 64
> >> > > actually degrades the performance as there will be a fragmented memory
> >> > > blocks with 32 bit bytes gap wherever
> >> > > SMP_CACHE_BYTES is used as an alignment requirement.
> >> >
> >> > I don't buy that: if you're trying to align to the cache size then the gaps are
> >> > the whole point. IIUC the 64-byte cache lines come from DDR, not XLEN, so
> >> > there's really no reason for these to be different between the base ISAs.
> >> >
> >>
> >> Got your point. I noticed this when fixing the resource tree issue
> >> where the SMP_CACHE_BYTES
> >> alignment was not intentional but causing the issue. The real issue
> >> was solved via another patch in this series though.
> >>
> >> Just to clarify, if the allocation function intends to allocate
> >> consecutive memory, it should use 32 instead of SMP_CACHE_BYTES.
> >> This will lead to a #ifdef macro in the code.
> >>
> >> > > In addition to that, Geert Uytterhoeven mentioned some panic on vex32
> >> > > without this patch.
> >> > > I didn't see anything in Qemu though.
> >> >
> >> > Something like that is probably only going to show up on real hardware, QEMU
> >> > doesn't really do anything with the cache line size. That said, as there's
> >> > nothing in our kernel now related to non-coherent memory there really should
> >> > only be performance issue (at least until we have non-coherent systems).
> >> >
> >> > I'd bet that the change is just masking some other bug, either in the software
> >> > or the hardware. I'd prefer to root cause this rather than just working around
> >> > it, as it'll probably come back later and in a more difficult way to find.
> >> >
> >>
> >> Agreed. @Geert Uytterhoeven Can you do a further analysis of the panic
> >> you were saying ?
> >> We may need to change an alignment requirement to 32 for RV32 manually
> >> at some place in code.
> >
> > My findings were in
> > https://lore.kernel.org/linux-riscv/CAMuHMdWf6K-5y02+WJ6Khu1cD6P0n5x1wYQikrECkuNtAA1pgg@mail.gmail.com/
> >
> > Note that when the memblock.reserved list kept increasing, it kept on
> > adding the same entry to the list. But that was fixed by "[PATCH 1/4]
> > RISC-V: Do not allocate memblock while iterating reserved memblocks".
> >
> > After that, only the (reproducible) "Unable to handle kernel paging
> > request at virtual address 61636473" was left, always at the same place.
> > No idea where the actual corruption happened.
>
> Thanks. Presumably I need an FPGA to run this? That will make it tricky to
> find anything here on my end.
In theory, it should work with the LiteX simulation, too.
I.e. follow the instructions at
https://github.com/litex-hub/linux-on-litex-vexriscv
You can find prebuilt binaries at
https://github.com/litex-hub/linux-on-litex-vexriscv/issues/164
Take images/opensbi.bin from opensbi_2020_12_15.zip, and
images/rootfs.cpio from linux_2021_01_11.zip.
Take images/Image from your own kernel build.
Unfortunately it seems the simulator is currently broken, and kernels
(both prebuilt and my own config) hang after
"sched_clock: 64 bits at 1000kHz, resolution 1000ns, wraps every
2199023255500ns"
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Sun, Jan 17, 2021 at 8:03 PM Geert Uytterhoeven <[email protected]> wrote:
> On Fri, Jan 15, 2021 at 11:44 PM Palmer Dabbelt <[email protected]> wrote:
> > On Thu, 14 Jan 2021 23:59:04 PST (-0800), [email protected] wrote:
> > > On Thu, Jan 14, 2021 at 10:11 PM Atish Patra <[email protected]> wrote:
> > >> On Thu, Jan 14, 2021 at 11:46 AM Palmer Dabbelt <[email protected]> wrote:
> > >> > On Thu, 14 Jan 2021 10:33:01 PST (-0800), [email protected] wrote:
> > >> > > On Wed, Jan 13, 2021 at 9:10 PM Palmer Dabbelt <[email protected]> wrote:
> > >> > >>
> > >> > >> On Thu, 07 Jan 2021 01:26:51 PST (-0800), Atish Patra wrote:
> > >> > >> > SMP_CACHE_BYTES/L1_CACHE_BYTES should be defined as 32 instead of
> > >> > >> > 64 for RV32. Otherwise, there will be hole of 32 bytes with each memblock
> > >> > >> > allocation if it is requested to be aligned with SMP_CACHE_BYTES.
> > >> > >> >
> > >> > >> > Signed-off-by: Atish Patra <[email protected]>
> > >> > >> > ---
> > >> > >> > arch/riscv/include/asm/cache.h | 4 ++++
> > >> > >> > 1 file changed, 4 insertions(+)
> > >> > >> >
> > >> > >> > diff --git a/arch/riscv/include/asm/cache.h b/arch/riscv/include/asm/cache.h
> > >> > >> > index 9b58b104559e..c9c669ea2fe6 100644
> > >> > >> > --- a/arch/riscv/include/asm/cache.h
> > >> > >> > +++ b/arch/riscv/include/asm/cache.h
> > >> > >> > @@ -7,7 +7,11 @@
> > >> > >> > #ifndef _ASM_RISCV_CACHE_H
> > >> > >> > #define _ASM_RISCV_CACHE_H
> > >> > >> >
> > >> > >> > +#ifdef CONFIG_64BIT
> > >> > >> > #define L1_CACHE_SHIFT 6
> > >> > >> > +#else
> > >> > >> > +#define L1_CACHE_SHIFT 5
> > >> > >> > +#endif
> > >> > >> >
> > >> > >> > #define L1_CACHE_BYTES (1 << L1_CACHE_SHIFT)
> > >> > >>
> > >> > >> Should we not instead just
> > >> > >>
> > >> > >> #define SMP_CACHE_BYTES L1_CACHE_BYTES
> > >> > >>
> > >> > >> like a handful of architectures do?
> > >> > >>
> > >> > >
> > >> > > The generic code already defines it that way in include/linux/cache.h
> > >> > >
> > >> > >> The cache size is sort of fake here, as we don't have any non-coherent
> > >> > >> mechanisms, but IIRC we wrote somewhere that it's recommended to have 64-byte
> > >> > >> cache lines in RISC-V implementations as software may assume that for
> > >> > >> performance reasons. Not really a strong reason, but I'd prefer to just make
> > >> > >> these match.
> > >> > >>
> > >> > >
> > >> > > If it is documented somewhere in the kernel, we should update that. I
> > >> > > think SMP_CACHE_BYTES being 64
> > >> > > actually degrades the performance as there will be a fragmented memory
> > >> > > blocks with 32 bit bytes gap wherever
> > >> > > SMP_CACHE_BYTES is used as an alignment requirement.
> > >> >
> > >> > I don't buy that: if you're trying to align to the cache size then the gaps are
> > >> > the whole point. IIUC the 64-byte cache lines come from DDR, not XLEN, so
> > >> > there's really no reason for these to be different between the base ISAs.
> > >> >
> > >>
> > >> Got your point. I noticed this when fixing the resource tree issue
> > >> where the SMP_CACHE_BYTES
> > >> alignment was not intentional but causing the issue. The real issue
> > >> was solved via another patch in this series though.
> > >>
> > >> Just to clarify, if the allocation function intends to allocate
> > >> consecutive memory, it should use 32 instead of SMP_CACHE_BYTES.
> > >> This will lead to a #ifdef macro in the code.
> > >>
> > >> > > In addition to that, Geert Uytterhoeven mentioned some panic on vex32
> > >> > > without this patch.
> > >> > > I didn't see anything in Qemu though.
> > >> >
> > >> > Something like that is probably only going to show up on real hardware, QEMU
> > >> > doesn't really do anything with the cache line size. That said, as there's
> > >> > nothing in our kernel now related to non-coherent memory there really should
> > >> > only be performance issue (at least until we have non-coherent systems).
> > >> >
> > >> > I'd bet that the change is just masking some other bug, either in the software
> > >> > or the hardware. I'd prefer to root cause this rather than just working around
> > >> > it, as it'll probably come back later and in a more difficult way to find.
> > >> >
> > >>
> > >> Agreed. @Geert Uytterhoeven Can you do a further analysis of the panic
> > >> you were saying ?
> > >> We may need to change an alignment requirement to 32 for RV32 manually
> > >> at some place in code.
> > >
> > > My findings were in
> > > https://lore.kernel.org/linux-riscv/CAMuHMdWf6K-5y02+WJ6Khu1cD6P0n5x1wYQikrECkuNtAA1pgg@mail.gmail.com/
> > >
> > > Note that when the memblock.reserved list kept increasing, it kept on
> > > adding the same entry to the list. But that was fixed by "[PATCH 1/4]
> > > RISC-V: Do not allocate memblock while iterating reserved memblocks".
> > >
> > > After that, only the (reproducible) "Unable to handle kernel paging
> > > request at virtual address 61636473" was left, always at the same place.
> > > No idea where the actual corruption happened.
> >
> > Thanks. Presumably I need an FPGA to run this? That will make it tricky to
> > find anything here on my end.
>
> In theory, it should work with the LiteX simulation, too.
> I.e. follow the instructions at
> https://github.com/litex-hub/linux-on-litex-vexriscv
> You can find prebuilt binaries at
> https://github.com/litex-hub/linux-on-litex-vexriscv/issues/164
> Take images/opensbi.bin from opensbi_2020_12_15.zip, and
> images/rootfs.cpio from linux_2021_01_11.zip.
> Take images/Image from your own kernel build.
>
> Unfortunately it seems the simulator is currently broken, and kernels
> (both prebuilt and my own config) hang after
> "sched_clock: 64 bits at 1000kHz, resolution 1000ns, wraps every
> 2199023255500ns"
In the mean time, the simulator got fixed.
Unfortunately the crash does not happen on the simulator.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Thu, Jan 14, 2021 at 7:34 PM Atish Patra <[email protected]> wrote:
> On Wed, Jan 13, 2021 at 9:10 PM Palmer Dabbelt <[email protected]> wrote:
> > On Thu, 07 Jan 2021 01:26:51 PST (-0800), Atish Patra wrote:
> > > SMP_CACHE_BYTES/L1_CACHE_BYTES should be defined as 32 instead of
> > > 64 for RV32. Otherwise, there will be hole of 32 bytes with each memblock
> > > allocation if it is requested to be aligned with SMP_CACHE_BYTES.
> > >
> > > Signed-off-by: Atish Patra <[email protected]>
> > > ---
> > > arch/riscv/include/asm/cache.h | 4 ++++
> > > 1 file changed, 4 insertions(+)
> > >
> > > diff --git a/arch/riscv/include/asm/cache.h b/arch/riscv/include/asm/cache.h
> > > index 9b58b104559e..c9c669ea2fe6 100644
> > > --- a/arch/riscv/include/asm/cache.h
> > > +++ b/arch/riscv/include/asm/cache.h
> > > @@ -7,7 +7,11 @@
> > > #ifndef _ASM_RISCV_CACHE_H
> > > #define _ASM_RISCV_CACHE_H
> > >
> > > +#ifdef CONFIG_64BIT
> > > #define L1_CACHE_SHIFT 6
> > > +#else
> > > +#define L1_CACHE_SHIFT 5
> > > +#endif
> > >
> > > #define L1_CACHE_BYTES (1 << L1_CACHE_SHIFT)
> >
> > Should we not instead just
> >
> > #define SMP_CACHE_BYTES L1_CACHE_BYTES
> >
> > like a handful of architectures do?
> >
>
> The generic code already defines it that way in include/linux/cache.h
>
> > The cache size is sort of fake here, as we don't have any non-coherent
> > mechanisms, but IIRC we wrote somewhere that it's recommended to have 64-byte
> > cache lines in RISC-V implementations as software may assume that for
> > performance reasons. Not really a strong reason, but I'd prefer to just make
> > these match.
> >
>
> If it is documented somewhere in the kernel, we should update that. I
> think SMP_CACHE_BYTES being 64
> actually degrades the performance as there will be a fragmented memory
> blocks with 32 bit bytes gap wherever
> SMP_CACHE_BYTES is used as an alignment requirement.
>
> In addition to that, Geert Uytterhoeven mentioned some panic on vex32
> without this patch.
> I didn't see anything in Qemu though.
FTR, I no longer need this patch on vexriscv.
It seems it just masked an issue.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds