Hi Nick,

On Wed, Jun 16, 2021 at 1:19 AM Nick Kossifidis [off-list ref] wrote:

Στις 2021-06-15 22:54, Rob Herring έγραψε:

quoted

On Tue, Jun 15, 2021 at 12:40 PM Nick Kossifidis [off-list ref]
wrote:

quoted

Στις 2021-06-15 21:17, Geert Uytterhoeven έγραψε:

quoted

RISC-V uses platform-specific code to locate the elf core header in
memory.  However, this does not conform to the standard
"linux,elfcorehdr" DT bindings, as it relies on a reserved memory node
with the "linux,elfcorehdr" compatible value, instead of on a
"linux,elfcorehdr" property under the "/chosen" node.

The non-compliant code can just be removed, as the standard behavior is
already implemented by platform-agnostic handling in the FDT core code.

Fixes: 5640975003d0234d ("RISC-V: Add crash kernel support")
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

NACK

There is nothing standard about "linux,elfcorehdr", it's an

It is and it is documented which is more than we can say for
"linux,elfcorehdr" as a node.

Standard stuff goes on /drivers/of, not on /arch/arm64. The

... which is what I'm fixing ;-)

Once in a while, new code is added where it's used, and moved to
common code later.

reserved-memory binding I use is on /drivers/of, is definitely a
standard / documented binding and the only issue here is that the
compatible string I used matched that property from arm64.

It's always a good idea to document your compatible strings, and run
your patches through the devicetree list, exactly to avoid issues
like this.

quoted

quoted

arm64-specific property on /chosen and it's suboptimal, it gets the
addr/length of ELF core of the previous kernel through that property
and
then goes on to reserve that region at:
https://elixir.bootlin.com/linux/v5.13-rc6/source/arch/arm64/mm/init.c#L155

Why on earth is this cleaner than just defining a reserved-region in
the
first place (a standard binding) with and hook up a callback with
RESERVEDMEM_OF_DECLARE for it to also initialize elfcorehdr_addr/size
?
If you don't like the compatible string I'm ok to change it, but this
patch breaks kdump on riscv since that region won't be reserved any
more
and kernel will corrupt it.

I might agree if we were designing this all from scratch, but we're
not. We've got powerpc doing /memreserve/ + kernel cmdline, arm64
using chosen, and RiscV a 3rd way.

I get it and I'd also like to consolidate things, but forcing riscv to
use a suboptimal approach just because arm64 uses it doesn't make sense
either, the goal should be for all to use the best possible approach
(disclaimer: I'm not saying my approach is the best possible, I'm saying
it's cleaner than arm64's).

quoted

What happens when/if RiscV wants to add an IMA buffer? That's no
different than this case. The 2 architectures supporting it both use
/chosen. Specifying an initrd is no different either.

Those two are already on drivers/of/fdt.c and drivers/of/kexec.c, it's
also interesting to note that for both of them, including
"linux,elfcorehdr", the newly added drivers/of/kexec.c adds an entry to
the fdt's memory reservation map when creating the fdt for the next
kernel, so they are all basically reserved regions. Why this was chosen
(a property on /chosen + an entry on the reservation map), effectively
adding each region twice on the fdt, instead of just adding a
reserved-memory node for each one beats me. Note that in case of arm64
this is not what happens on kexec-tools, which is probably the reason
why arm64 still reserves them in any case.

I can't comment on the duplication on arm64, but to me, /chosen
sounds like the natural place for both "linux,elfcorehdr" and
"linux,usable-memory-range".  First rule of DT is "DT describes
hardware, not software policy", with /chosen describing some software
configuration.

Anyway I guess switching arm64 to reserved-memory is too much to ask
since they would have to also change kexec-tools, handle different
versions etc, and although I don't like it consolidation is more
important than a duplicate region on the fdt, so let's go with
"linux,elfcorehdr" on /chosen + entry on the reservation map. I'll
update my kexec-tools patch instead.

OK, thanks!
But do you need the entry on the reservation map?

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