Message-ID: <7a86cf8e-f40f-29ae-bc7c-8a20cc5b9850@microchip.com>
Date:   Tue, 23 Aug 2022 09:36:01 +0000
From:   <Conor.Dooley@...rochip.com>
To:     <heinrich.schuchardt@...onical.com>
CC:     <linux-kernel@...r.kernel.org>, <devicetree@...r.kernel.org>,
        <frowand.list@...il.com>, <robh+dt@...nel.org>,
        <mick@....forth.gr>, <paul.walmsley@...ive.com>,
        <palmer@...belt.com>, <aou@...s.berkeley.edu>,
        <Valentina.FernandezAlanis@...rochip.com>,
        <Daire.McNamara@...rochip.com>, <linux-riscv@...ts.infradead.org>
Subject: Re: RISC-V reserved memory problems

Hey Heinrich,
Thanks for chiming in.

On 18/08/2022 15:32, Heinrich Schuchardt wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 8/16/22 22:41, Conor.Dooley@...rochip.com wrote:
>> Hey all,
>> We've run into a bit of a problem with reserved memory on PolarFire, or
>> more accurately a pair of problems that seem to have opposite fixes.
>>
>> The first of these problems is triggered when trying to implement a
>> remoteproc driver. To get the reserved memory buffer, remoteproc
>> does an of_reserved_mem_lookup(), something like:
>>
>>       np = of_parse_phandle(pdev->of_node, "memory-region", 0);
>>       if (!np)
>>               return -EINVAL;
>>
>>       rmem = of_reserved_mem_lookup(np);
>>       if (!rmem)
>>               return -EINVAL;
>>
>> of_reserved_mem_lookup() then uses reserved_mem[i].name to try and find
>> a match - but this was triggering kernel panics for us. We did some
>> debugging and found that the name string's pointer was pointing to an
>> address in the 0x4000_0000 range. The minimum reproduction for this
>> crash is attached - it hacks in some print_reserved_mem()s into
>> setup_vm_final() around a tlb flush so you can see the before/after.
>> (You'll need a reserved memory node in your dts to replicate)
>>
>> The output is like so, with the same crash as in the remoteproc driver:
>>
>> [    0.000000] Linux version 6.0.0-rc1-00001-g0d9d6953d834 (conor@...dy) (riscv64-unknown-linux-gnu-gcc (g5964b5cd727) 11.1.0, GNU ld (GNU Binutils) 2.37) #1 SMP Tue Aug 16 13:42:09 IST 2022
>> [    0.000000] OF: fdt: Ignoring memory range 0x80000000 - 0x80200000
>> [    0.000000] Machine model: Microchip PolarFire-SoC Icicle Kit
>> [    0.000000] earlycon: ns16550a0 at MMIO32 0x0000000020100000 (options '115200n8')
>> [    0.000000] printk: bootconsole [ns16550a0] enabled
>> [    0.000000] printk: debug: skip boot console de-registration.
>> [    0.000000] efi: UEFI not found.
>> [    0.000000] before flush
>> [    0.000000] OF: reserved mem: debug name is fabricbuf@...00000
>> [    0.000000] after flush
>> [    0.000000] Unable to handle kernel paging request at virtual address 00000000401c31ac
>> [    0.000000] Oops [#1]
>> [    0.000000] Modules linked in:
>> [    0.000000] CPU: 0 PID: 0 Comm: swapper Not tainted 6.0.0-rc1-00001-g0d9d6953d834 #1
>> [    0.000000] Hardware name: Microchip PolarFire-SoC Icicle Kit (DT)
>> [    0.000000] epc : string+0x4a/0xea
>> [    0.000000]  ra : vsnprintf+0x1e4/0x336
>> [    0.000000] epc : ffffffff80335ea0 ra : ffffffff80338936 sp : ffffffff81203be0
>> [    0.000000]  gp : ffffffff812e0a98 tp : ffffffff8120de40 t0 : 0000000000000000
>> [    0.000000]  t1 : ffffffff81203e28 t2 : 7265736572203a46 s0 : ffffffff81203c20
>> [    0.000000]  s1 : ffffffff81203e28 a0 : ffffffff81203d22 a1 : 0000000000000000
>> [    0.000000]  a2 : ffffffff81203d08 a3 : 0000000081203d21 a4 : ffffffffffffffff
>> [    0.000000]  a5 : 00000000401c31ac a6 : ffff0a00ffffff04 a7 : ffffffffffffffff
>> [    0.000000]  s2 : ffffffff81203d08 s3 : ffffffff81203d00 s4 : 0000000000000008
>> [    0.000000]  s5 : ffffffff000000ff s6 : 0000000000ffffff s7 : 00000000ffffff00
>> [    0.000000]  s8 : ffffffff80d9821a s9 : ffffffff81203d22 s10: 0000000000000002
>> [    0.000000]  s11: ffffffff80d9821c t3 : ffffffff812f3617 t4 : ffffffff812f3617
>> [    0.000000]  t5 : ffffffff812f3618 t6 : ffffffff81203d08
>> [    0.000000] status: 0000000200000100 badaddr: 00000000401c31ac cause: 000000000000000d
>> [    0.000000] [<ffffffff80338936>] vsnprintf+0x1e4/0x336
>> [    0.000000] [<ffffffff80055ae2>] vprintk_store+0xf6/0x344
>> [    0.000000] [<ffffffff80055d86>] vprintk_emit+0x56/0x192
>> [    0.000000] [<ffffffff80055ed8>] vprintk_default+0x16/0x1e
>> [    0.000000] [<ffffffff800563d2>] vprintk+0x72/0x80
>> [    0.000000] [<ffffffff806813b2>] _printk+0x36/0x50
>> [    0.000000] [<ffffffff8068af48>] print_reserved_mem+0x1c/0x24
>> [    0.000000] [<ffffffff808057ec>] paging_init+0x528/0x5bc
>> [    0.000000] [<ffffffff808031ae>] setup_arch+0xd0/0x592
>> [    0.000000] [<ffffffff8080070e>] start_kernel+0x82/0x73c
>> [    0.000000] ---[ end trace 0000000000000000 ]---
>> [    0.000000] Kernel panic - not syncing: Attempted to kill the idle task!
>> [    0.000000] ---[ end Kernel panic - not syncing: Attempted to kill the idle task! ]---
>>
>> We traced this back to early_init_fdt_scan_reserved_mem() in
>> setup_bootmem() - moving it later back up the boot sequence to
>> after the dt has been remapped etc has fixed the problem for us.
>>
>> The least movement to get it working is attached, and also pushed
>> here: git.kernel.org/conor/c/1735589baefc
>>
>> The second problem is a bit more complicated to explain - but we
>> found the solution conflicted with the remoteproc fix as we had
>> to move early_init_fdt_scan_reserved_mem() _earlier_ in the boot
>> process to solve this one.
>>
>> We want to have a node in our devicetree that contains some memory
>> that is non-cached & marked as reserved-memory. Maybe we have just
>> missed something, but from what we've seen:
>> - the really early setup looks at the dtb, picks the highest bit
>>     of memory and puts the dtb etc there so it can start using it
>> - early_init_fdt_scan_reserved_mem() is then called, which figures
>>     out if memory is reserved or not.
>>
>> Unfortunately, the highest bit of memory is the non-cached bit so
>> everything falls over, but we can avoid this by moving the call to
>> early_init_fdt_scan_reserved_mem() above the dtb memblock alloc that
>> takes place right before it in setup_bootmem().
>>
>> Obviously, both of these changes are moving the function call in
>> opposite directions and we can only really do one of them. We are not
>> sure if what we are doing with the non-cached reserved-memory section
>> is just not permitted & cannot work - or if this is something that
>> was overlooked for RISC-V specifically and works for other archs.
>>
>> It does seem like the first issue is a real bug, and I am happy to
>> submit the patch for that whenever - but having two problems with
>> opposite fixes seemed as if there was something else lurking that we
>> just don't have enough understanding to detect.
>>
>> Any help would be great!

> could you, please, provide the relevant device-tree sniplets.

Sure. That "might" have been a good thing to do from the start..
For the first problem it is actually fairly straightforward,
something like the following triggered it for me:
	reserved-memory {
		ranges;
		#size-cells = <2>;
		#address-cells = <2>;

		fabricbuf0: fabricbuf@0 {
			compatible = "shared-dma-pool";
			reg = <0x0 0xae000000 0x0 0x2000000>;
			label = "fabricbuf0-ddr-c";
		};
	};

I was able to repro this with the stanza in u-boot's dt and
not in Linux's too.

> 
> Please, have a look at the no-map property in
> Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt.

For those playing along at home, this has been moved to:
Documentation/devicetree/bindings/reserved-memory/reserved-memory.yaml

For this case, it is something *like* the following:
	reserved-memory {
		ranges;
		#size-cells = <2>;
		#address-cells = <2>;

		dma_nc_hi: linux,dma {
			compatible = "shared-dma-pool";
			size = <0x0 0x1000000>;
			no-map;
			linux,dma-default;
			alloc-ranges = <0x14 0x00000000 0x0 0x1000000>;
			dma-ranges = <0x14 0x00000000 0x14 0x00000000 0x0 0x1000000>;
		};
	};

	ddrc_cache_lo: memory@...00000 {
		device_type = "memory";
		reg = <0x0 0x80000000 0x0 0x2e000000>;
		status = "okay";
		label = "cache-lo";
	};

	ddrc_cache_hi: memory@...0000000 {
		device_type = "memory";
		reg = <0x10 0x0 0x0 0x20000000>;
		status = "okay";
		label = "cache-hi";
	};

	ddr_nc_hi: memory@...0000000 {
		device_type = "memory";
		reg = <0x14 0x00000000 0x0 0x1000000>;
		status = "okay";
		label = "non-cache-hi";
	};

As you can see, that does in fact have a no-map in it. I have adapted
this slightly so that it would resemble the existing dts, so it not
the *exact* one the issue was found with but it is functionally the
same. Hope that helps explain things a little better.

Thanks,
Conor.

Powered by blists - more mailing lists