[PATCH 0/4] Assorted fixes for RV32

[PATCH 0/4] Assorted fixes for RV32

[Atish Patra]

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

[PATCH 1/4] RISC-V: Do not allocate memblock while iterating reserved memblocks

[Atish Patra]

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 <atish.patra@wdc.com>
---
 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

Re: [PATCH 1/4] RISC-V: Do not allocate memblock while iterating reserved memblocks

[Geert Uytterhoeven]

Hi Atish,

On Thu, Jan 7, 2021 at 10:28 AM Atish Patra <atish.patra@wdc.com> 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 <atish.patra@wdc.com>

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 <geert@linux-m68k.org>

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 -- geert@linux-m68k.org

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

Re: [PATCH 1/4] RISC-V: Do not allocate memblock while iterating reserved memblocks

[Anup Patel]

On Thu, Jan 7, 2021 at 2:57 PM Atish Patra <atish.patra@wdc.com> 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 <atish.patra@wdc.com>

Looks good to me.

Reviewed-by: Anup Patel <anup@brainfault.org>

> ---
>  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
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

Regards,
Anup

[PATCH 2/4] RISC-V: Set current memblock limit

[Atish Patra]

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 <atish.patra@wdc.com>
---
 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

Re: [PATCH 2/4] RISC-V: Set current memblock limit

[Anup Patel]

On Thu, Jan 7, 2021 at 2:57 PM Atish Patra <atish.patra@wdc.com> 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 <atish.patra@wdc.com>
> ---
>  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 <anup@brainfault.org>

>         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
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

Regards,
Anup

Re: [PATCH 2/4] RISC-V: Set current memblock limit

[Atish Patra]

On Sun, Jan 10, 2021 at 7:59 PM Anup Patel <anup@brainfault.org> wrote:
>
> On Thu, Jan 7, 2021 at 2:57 PM Atish Patra <atish.patra@wdc.com> 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 <atish.patra@wdc.com>
> > ---
> >  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 <anup@brainfault.org>
>
> >         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
> > linux-riscv@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-riscv
>
> Regards,
> Anup
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv



-- 
Regards,
Atish

[PATCH 3/4] RISC-V: Fix L1_CACHE_BYTES for RV32

[Atish Patra]

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 <atish.patra@wdc.com>
---
 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

Re: [PATCH 3/4] RISC-V: Fix L1_CACHE_BYTES for RV32

[Geert Uytterhoeven]

On Thu, Jan 7, 2021 at 10:28 AM Atish Patra <atish.patra@wdc.com> 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 <atish.patra@wdc.com>

Tested-by: Geert Uytterhoeven <geert@linux-m68k.org>
(on vexriscv)

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

Re: [PATCH 3/4] RISC-V: Fix L1_CACHE_BYTES for RV32

[Anup Patel]

On Thu, Jan 7, 2021 at 2:57 PM Atish Patra <atish.patra@wdc.com> 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 <atish.patra@wdc.com>

Looks good to me.

Reviewed-by: Anup Patel <anup@brainfault.org>

> ---
>  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
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

Regards,
Anup

Re: [PATCH 3/4] RISC-V: Fix L1_CACHE_BYTES for RV32

[Palmer Dabbelt]

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 <atish.patra@wdc.com>
> ---
>  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.

Re: [PATCH 3/4] RISC-V: Fix L1_CACHE_BYTES for RV32

[Atish Patra]

On Wed, Jan 13, 2021 at 9:10 PM Palmer Dabbelt <palmer@dabbelt.com> 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 <atish.patra@wdc.com>
> > ---
> >  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
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv



-- 
Regards,
Atish

Re: [PATCH 3/4] RISC-V: Fix L1_CACHE_BYTES for RV32

[Palmer Dabbelt]

On Thu, 14 Jan 2021 10:33:01 PST (-0800), atishp@atishpatra.org wrote:
> On Wed, Jan 13, 2021 at 9:10 PM Palmer Dabbelt <palmer@dabbelt.com> 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 <atish.patra@wdc.com>
>> > ---
>> >  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
>> linux-riscv@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 3/4] RISC-V: Fix L1_CACHE_BYTES for RV32

[Atish Patra]

On Thu, Jan 14, 2021 at 11:46 AM Palmer Dabbelt <palmer@dabbelt.com> wrote:
>
> On Thu, 14 Jan 2021 10:33:01 PST (-0800), atishp@atishpatra.org wrote:
> > On Wed, Jan 13, 2021 at 9:10 PM Palmer Dabbelt <palmer@dabbelt.com> 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 <atish.patra@wdc.com>
> >> > ---
> >> >  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
> >> linux-riscv@lists.infradead.org
> >> http://lists.infradead.org/mailman/listinfo/linux-riscv



-- 
Regards,
Atish

Re: [PATCH 3/4] RISC-V: Fix L1_CACHE_BYTES for RV32

[Palmer Dabbelt]

On Thu, 14 Jan 2021 13:11:14 PST (-0800), atishp@atishpatra.org wrote:
> On Thu, Jan 14, 2021 at 11:46 AM Palmer Dabbelt <palmer@dabbelt.com> wrote:
>>
>> On Thu, 14 Jan 2021 10:33:01 PST (-0800), atishp@atishpatra.org wrote:
>> > On Wed, Jan 13, 2021 at 9:10 PM Palmer Dabbelt <palmer@dabbelt.com> 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 <atish.patra@wdc.com>
>> >> > ---
>> >> >  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
>> >> linux-riscv@lists.infradead.org
>> >> http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 3/4] RISC-V: Fix L1_CACHE_BYTES for RV32

[Geert Uytterhoeven]

Hi Atish,

On Thu, Jan 14, 2021 at 10:11 PM Atish Patra <atishp@atishpatra.org> wrote:
> On Thu, Jan 14, 2021 at 11:46 AM Palmer Dabbelt <palmer@dabbelt.com> wrote:
> > On Thu, 14 Jan 2021 10:33:01 PST (-0800), atishp@atishpatra.org wrote:
> > > On Wed, Jan 13, 2021 at 9:10 PM Palmer Dabbelt <palmer@dabbelt.com> 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 <atish.patra@wdc.com>
> > >> > ---
> > >> >  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 -- geert@linux-m68k.org

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

Re: [PATCH 3/4] RISC-V: Fix L1_CACHE_BYTES for RV32

[Palmer Dabbelt]

On Thu, 14 Jan 2021 23:59:04 PST (-0800), geert@linux-m68k.org wrote:
> Hi Atish,
>
> On Thu, Jan 14, 2021 at 10:11 PM Atish Patra <atishp@atishpatra.org> wrote:
>> On Thu, Jan 14, 2021 at 11:46 AM Palmer Dabbelt <palmer@dabbelt.com> wrote:
>> > On Thu, 14 Jan 2021 10:33:01 PST (-0800), atishp@atishpatra.org wrote:
>> > > On Wed, Jan 13, 2021 at 9:10 PM Palmer Dabbelt <palmer@dabbelt.com> 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 <atish.patra@wdc.com>
>> > >> > ---
>> > >> >  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

Re: [PATCH 3/4] RISC-V: Fix L1_CACHE_BYTES for RV32

[Geert Uytterhoeven]

Hi Palmer,

On Fri, Jan 15, 2021 at 11:44 PM Palmer Dabbelt <palmer@dabbelt.com> wrote:
> On Thu, 14 Jan 2021 23:59:04 PST (-0800), geert@linux-m68k.org wrote:
> > On Thu, Jan 14, 2021 at 10:11 PM Atish Patra <atishp@atishpatra.org> wrote:
> >> On Thu, Jan 14, 2021 at 11:46 AM Palmer Dabbelt <palmer@dabbelt.com> wrote:
> >> > On Thu, 14 Jan 2021 10:33:01 PST (-0800), atishp@atishpatra.org wrote:
> >> > > On Wed, Jan 13, 2021 at 9:10 PM Palmer Dabbelt <palmer@dabbelt.com> 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 <atish.patra@wdc.com>
> >> > >> > ---
> >> > >> >  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 -- geert@linux-m68k.org

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

Re: [PATCH 3/4] RISC-V: Fix L1_CACHE_BYTES for RV32

[Geert Uytterhoeven]

On Sun, Jan 17, 2021 at 8:03 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Fri, Jan 15, 2021 at 11:44 PM Palmer Dabbelt <palmer@dabbelt.com> wrote:
> > On Thu, 14 Jan 2021 23:59:04 PST (-0800), geert@linux-m68k.org wrote:
> > > On Thu, Jan 14, 2021 at 10:11 PM Atish Patra <atishp@atishpatra.org> wrote:
> > >> On Thu, Jan 14, 2021 at 11:46 AM Palmer Dabbelt <palmer@dabbelt.com> wrote:
> > >> > On Thu, 14 Jan 2021 10:33:01 PST (-0800), atishp@atishpatra.org wrote:
> > >> > > On Wed, Jan 13, 2021 at 9:10 PM Palmer Dabbelt <palmer@dabbelt.com> 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 <atish.patra@wdc.com>
> > >> > >> > ---
> > >> > >> >  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 -- geert@linux-m68k.org

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

Re: [PATCH 3/4] RISC-V: Fix L1_CACHE_BYTES for RV32

[Atish Patra]

On Thu, Jan 14, 2021 at 11:59 PM Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
>
> Hi Atish,
>
> On Thu, Jan 14, 2021 at 10:11 PM Atish Patra <atishp@atishpatra.org> wrote:
> > On Thu, Jan 14, 2021 at 11:46 AM Palmer Dabbelt <palmer@dabbelt.com> wrote:
> > > On Thu, 14 Jan 2021 10:33:01 PST (-0800), atishp@atishpatra.org wrote:
> > > > On Wed, Jan 13, 2021 at 9:10 PM Palmer Dabbelt <palmer@dabbelt.com> 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 <atish.patra@wdc.com>
> > > >> > ---
> > > >> >  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 -- geert@linux-m68k.org
>
> 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

Re: [PATCH 3/4] RISC-V: Fix L1_CACHE_BYTES for RV32

[Geert Uytterhoeven]

Hi Atish,

On Sat, Jan 16, 2021 at 2:39 AM Atish Patra <atishp@atishpatra.org> wrote:
> On Thu, Jan 14, 2021 at 11:59 PM Geert Uytterhoeven
> <geert@linux-m68k.org> wrote:
> > On Thu, Jan 14, 2021 at 10:11 PM Atish Patra <atishp@atishpatra.org> wrote:
> > > On Thu, Jan 14, 2021 at 11:46 AM Palmer Dabbelt <palmer@dabbelt.com> wrote:
> > > > On Thu, 14 Jan 2021 10:33:01 PST (-0800), atishp@atishpatra.org wrote:
> > > > > On Wed, Jan 13, 2021 at 9:10 PM Palmer Dabbelt <palmer@dabbelt.com> 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 <atish.patra@wdc.com>
> > > > >> > ---
> > > > >> >  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 -- geert@linux-m68k.org

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

Re: [PATCH 3/4] RISC-V: Fix L1_CACHE_BYTES for RV32

[Geert Uytterhoeven]

On Thu, Jan 14, 2021 at 7:34 PM Atish Patra <atishp@atishpatra.org> wrote:
> On Wed, Jan 13, 2021 at 9:10 PM Palmer Dabbelt <palmer@dabbelt.com> 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 <atish.patra@wdc.com>
> > > ---
> > >  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 -- geert@linux-m68k.org

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

[PATCH 4/4] RISC-V: Fix maximum allowed phsyical memory for RV32

[Atish Patra]

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 <atish.patra@wdc.com>
---
 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

Re: [PATCH 4/4] RISC-V: Fix maximum allowed phsyical memory for RV32

[Anup Patel]

On Thu, Jan 7, 2021 at 2:57 PM Atish Patra <atish.patra@wdc.com> 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 <atish.patra@wdc.com>

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 <anup@brainfault.org>

> ---
>  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
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

Regards,
Anup

Re: [PATCH 0/4] Assorted fixes for RV32

[Anup Patel]

Hi Palmer,

On Thu, Jan 7, 2021 at 2:57 PM Atish Patra <atish.patra@wdc.com> 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
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

Regards,
Anup

Re: [PATCH 0/4] Assorted fixes for RV32

[Palmer Dabbelt]

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!

Re: [PATCH 0/4] Assorted fixes for RV32

[Palmer Dabbelt]

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.