[PATCH 0/3] Add generic-support for linux,elfcorehdr and fix riscv

[PATCH 0/3] Add generic-support for linux,elfcorehdr and fix riscv

[Geert Uytterhoeven]

	Hi all,

While working on v4 of "[PATCH v3] ARM: Parse kdump DT properties", I
noticed the recently (v5.13-rc1) introduced RISC-V crash kernel support
uses "linux,elfcorehdr" in a non-standard way.  Instead of relying on a
"linux,elfcorehdr" property under the "/chosen" node, RISC-V uses a
reserved memory node with the "linux,elfcorehdr" compatible value.

As we may want to fix riscv before the release of v5.13, I decided not
to wait until my full v4 is ready, but fast-track generic
"linux,elfcorehdr" handling instead.

This series consists of 3 patches:
  1. Generic handling of "linux,elfcorehdr", as requested by Rob in a
     review comment for [1],
  2. Drop the non-standard code from riscv.  It can just use the generic
     code instead (needs corresponding changes to WIP kexec-tools),
  3. Drop the now duplicate code from arm64.  This can be postponed, as
     it can co-exist safely with the generic code.

This has been tested on arm32 (with a WIP successor of [1]), and
compile-tested on riscv64 and arm64.

Thanks for your comments!

[1] "[PATCH v3] ARM: Parse kdump DT properties"
    https://lore.kernel.org/linux-devicetree/20210317113130.2554368-1-geert+renesas@glider.be/

Geert Uytterhoeven (3):
  of: fdt: Add generic support for parsing elf core header properties
  riscv: Remove non-standard linux,elfcorehdr handling
  arm64: kdump: Remove custom linux,elfcorehdr parsing

 Documentation/devicetree/bindings/chosen.txt |  6 ++--
 arch/arm64/mm/init.c                         | 21 -----------
 arch/riscv/mm/init.c                         | 20 -----------
 drivers/of/fdt.c                             | 37 ++++++++++++++++++--
 4 files changed, 37 insertions(+), 47 deletions(-)

-- 
2.25.1

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 0/3] Add generic-support for linux,elfcorehdr and fix riscv

[Geert Uytterhoeven]

	Hi all,

While working on v4 of "[PATCH v3] ARM: Parse kdump DT properties", I
noticed the recently (v5.13-rc1) introduced RISC-V crash kernel support
uses "linux,elfcorehdr" in a non-standard way.  Instead of relying on a
"linux,elfcorehdr" property under the "/chosen" node, RISC-V uses a
reserved memory node with the "linux,elfcorehdr" compatible value.

As we may want to fix riscv before the release of v5.13, I decided not
to wait until my full v4 is ready, but fast-track generic
"linux,elfcorehdr" handling instead.

This series consists of 3 patches:
  1. Generic handling of "linux,elfcorehdr", as requested by Rob in a
     review comment for [1],
  2. Drop the non-standard code from riscv.  It can just use the generic
     code instead (needs corresponding changes to WIP kexec-tools),
  3. Drop the now duplicate code from arm64.  This can be postponed, as
     it can co-exist safely with the generic code.

This has been tested on arm32 (with a WIP successor of [1]), and
compile-tested on riscv64 and arm64.

Thanks for your comments!

[1] "[PATCH v3] ARM: Parse kdump DT properties"
    https://lore.kernel.org/linux-devicetree/20210317113130.2554368-1-geert+renesas@glider.be/

Geert Uytterhoeven (3):
  of: fdt: Add generic support for parsing elf core header properties
  riscv: Remove non-standard linux,elfcorehdr handling
  arm64: kdump: Remove custom linux,elfcorehdr parsing

 Documentation/devicetree/bindings/chosen.txt |  6 ++--
 arch/arm64/mm/init.c                         | 21 -----------
 arch/riscv/mm/init.c                         | 20 -----------
 drivers/of/fdt.c                             | 37 ++++++++++++++++++--
 4 files changed, 37 insertions(+), 47 deletions(-)

-- 
2.25.1

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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH 0/3] Add generic-support for linux,elfcorehdr and fix riscv

[Geert Uytterhoeven]

	Hi all,

While working on v4 of "[PATCH v3] ARM: Parse kdump DT properties", I
noticed the recently (v5.13-rc1) introduced RISC-V crash kernel support
uses "linux,elfcorehdr" in a non-standard way.  Instead of relying on a
"linux,elfcorehdr" property under the "/chosen" node, RISC-V uses a
reserved memory node with the "linux,elfcorehdr" compatible value.

As we may want to fix riscv before the release of v5.13, I decided not
to wait until my full v4 is ready, but fast-track generic
"linux,elfcorehdr" handling instead.

This series consists of 3 patches:
  1. Generic handling of "linux,elfcorehdr", as requested by Rob in a
     review comment for [1],
  2. Drop the non-standard code from riscv.  It can just use the generic
     code instead (needs corresponding changes to WIP kexec-tools),
  3. Drop the now duplicate code from arm64.  This can be postponed, as
     it can co-exist safely with the generic code.

This has been tested on arm32 (with a WIP successor of [1]), and
compile-tested on riscv64 and arm64.

Thanks for your comments!

[1] "[PATCH v3] ARM: Parse kdump DT properties"
    https://lore.kernel.org/linux-devicetree/20210317113130.2554368-1-geert+renesas@glider.be/

Geert Uytterhoeven (3):
  of: fdt: Add generic support for parsing elf core header properties
  riscv: Remove non-standard linux,elfcorehdr handling
  arm64: kdump: Remove custom linux,elfcorehdr parsing

 Documentation/devicetree/bindings/chosen.txt |  6 ++--
 arch/arm64/mm/init.c                         | 21 -----------
 arch/riscv/mm/init.c                         | 20 -----------
 drivers/of/fdt.c                             | 37 ++++++++++++++++++--
 4 files changed, 37 insertions(+), 47 deletions(-)

-- 
2.25.1

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/3] of: fdt: Add generic support for parsing elf core header properties

[Geert Uytterhoeven]

There are two methods to specify the location of the elf core header:
using the "elfcorehdr=" kernel parameter, as handled by generic code in
kernel/crash_dump.c, or using the "linux,elfcorehdr" property under the
"/chosen" node in the Device Tree, as handled by architecture-specific
code in arch/arm64/mm/init.c.

Extend support for "linux,elfcorehdr" to all platforms supporting DT by
adding platform-agnostic handling for parsing this property to the FDT
core code.  This can co-exist safely with the architecture-specific
handling, until the latter has been removed.

This requires moving the call to of_scan_flat_dt() up, as the code
scanning the "/chosen" node now needs to be aware of the values of
"#address-cells" and "#size-cells".

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 Documentation/devicetree/bindings/chosen.txt |  6 ++--
 drivers/of/fdt.c                             | 37 ++++++++++++++++++--
 2 files changed, 37 insertions(+), 6 deletions(-)

diff --git a/Documentation/devicetree/bindings/chosen.txt b/Documentation/devicetree/bindings/chosen.txt
index 45e79172a646c537..5b0b94eb2d04e79d 100644
--- a/Documentation/devicetree/bindings/chosen.txt
+++ b/Documentation/devicetree/bindings/chosen.txt
@@ -106,9 +106,9 @@ respectively, of the root node.
 linux,elfcorehdr
 ----------------
 
-This property (currently used only on arm64) holds the memory range,
-the address and the size, of the elf core header which mainly describes
-the panicked kernel's memory layout as PT_LOAD segments of elf format.
+This property holds the memory range, the address and the size, of the elf
+core header which mainly describes the panicked kernel's memory layout as
+PT_LOAD segments of elf format.
 e.g.
 
 / {
diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index a03d43f95495d8e1..f13db831c8028cce 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -8,6 +8,7 @@
 
 #define pr_fmt(fmt)	"OF: fdt: " fmt
 
+#include <linux/crash_dump.h>
 #include <linux/crc32.h>
 #include <linux/kernel.h>
 #include <linux/initrd.h>
@@ -909,6 +910,35 @@ static inline void early_init_dt_check_for_initrd(unsigned long node)
 }
 #endif /* CONFIG_BLK_DEV_INITRD */
 
+#ifdef CONFIG_CRASH_DUMP
+/**
+ * early_init_dt_check_for_elfcorehdr - Decode elfcorehdr location from flat
+ * tree
+ * @node: reference to node containing elfcorehdr location ('chosen')
+ */
+static void __init early_init_dt_check_for_elfcorehdr(unsigned long node)
+{
+	const __be32 *prop;
+	int len;
+
+	pr_debug("Looking for elfcorehdr property... ");
+
+	prop = of_get_flat_dt_prop(node, "linux,elfcorehdr", &len);
+	if (!prop || (len < (dt_root_addr_cells + dt_root_size_cells)))
+		return;
+
+	elfcorehdr_addr = dt_mem_next_cell(dt_root_addr_cells, &prop);
+	elfcorehdr_size = dt_mem_next_cell(dt_root_size_cells, &prop);
+
+	pr_debug("elfcorehdr_start=0x%llx elfcorehdr_size=0x%llx\n",
+		 elfcorehdr_addr, elfcorehdr_size);
+}
+#else
+static inline void early_init_dt_check_for_elfcorehdr(unsigned long node)
+{
+}
+#endif
+
 #ifdef CONFIG_SERIAL_EARLYCON
 
 int __init early_init_dt_scan_chosen_stdout(void)
@@ -1057,6 +1087,7 @@ int __init early_init_dt_scan_chosen(unsigned long node, const char *uname,
 		return 0;
 
 	early_init_dt_check_for_initrd(node);
+	early_init_dt_check_for_elfcorehdr(node);
 
 	/* Retrieve command line */
 	p = of_get_flat_dt_prop(node, "bootargs", &l);
@@ -1201,14 +1232,14 @@ void __init early_init_dt_scan_nodes(void)
 {
 	int rc = 0;
 
+	/* Initialize {size,address}-cells info */
+	of_scan_flat_dt(early_init_dt_scan_root, NULL);
+
 	/* Retrieve various information from the /chosen node */
 	rc = of_scan_flat_dt(early_init_dt_scan_chosen, boot_command_line);
 	if (!rc)
 		pr_warn("No chosen node found, continuing without\n");
 
-	/* Initialize {size,address}-cells info */
-	of_scan_flat_dt(early_init_dt_scan_root, NULL);
-
 	/* Setup memory, calling early_init_dt_add_memory_arch */
 	of_scan_flat_dt(early_init_dt_scan_memory, NULL);
 }
-- 
2.25.1

[PATCH 1/3] of: fdt: Add generic support for parsing elf core header properties

[Geert Uytterhoeven]

There are two methods to specify the location of the elf core header:
using the "elfcorehdr=" kernel parameter, as handled by generic code in
kernel/crash_dump.c, or using the "linux,elfcorehdr" property under the
"/chosen" node in the Device Tree, as handled by architecture-specific
code in arch/arm64/mm/init.c.

Extend support for "linux,elfcorehdr" to all platforms supporting DT by
adding platform-agnostic handling for parsing this property to the FDT
core code.  This can co-exist safely with the architecture-specific
handling, until the latter has been removed.

This requires moving the call to of_scan_flat_dt() up, as the code
scanning the "/chosen" node now needs to be aware of the values of
"#address-cells" and "#size-cells".

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 Documentation/devicetree/bindings/chosen.txt |  6 ++--
 drivers/of/fdt.c                             | 37 ++++++++++++++++++--
 2 files changed, 37 insertions(+), 6 deletions(-)

diff --git a/Documentation/devicetree/bindings/chosen.txt b/Documentation/devicetree/bindings/chosen.txt
index 45e79172a646c537..5b0b94eb2d04e79d 100644
--- a/Documentation/devicetree/bindings/chosen.txt
+++ b/Documentation/devicetree/bindings/chosen.txt
@@ -106,9 +106,9 @@ respectively, of the root node.
 linux,elfcorehdr
 ----------------
 
-This property (currently used only on arm64) holds the memory range,
-the address and the size, of the elf core header which mainly describes
-the panicked kernel's memory layout as PT_LOAD segments of elf format.
+This property holds the memory range, the address and the size, of the elf
+core header which mainly describes the panicked kernel's memory layout as
+PT_LOAD segments of elf format.
 e.g.
 
 / {
diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index a03d43f95495d8e1..f13db831c8028cce 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -8,6 +8,7 @@
 
 #define pr_fmt(fmt)	"OF: fdt: " fmt
 
+#include <linux/crash_dump.h>
 #include <linux/crc32.h>
 #include <linux/kernel.h>
 #include <linux/initrd.h>
@@ -909,6 +910,35 @@ static inline void early_init_dt_check_for_initrd(unsigned long node)
 }
 #endif /* CONFIG_BLK_DEV_INITRD */
 
+#ifdef CONFIG_CRASH_DUMP
+/**
+ * early_init_dt_check_for_elfcorehdr - Decode elfcorehdr location from flat
+ * tree
+ * @node: reference to node containing elfcorehdr location ('chosen')
+ */
+static void __init early_init_dt_check_for_elfcorehdr(unsigned long node)
+{
+	const __be32 *prop;
+	int len;
+
+	pr_debug("Looking for elfcorehdr property... ");
+
+	prop = of_get_flat_dt_prop(node, "linux,elfcorehdr", &len);
+	if (!prop || (len < (dt_root_addr_cells + dt_root_size_cells)))
+		return;
+
+	elfcorehdr_addr = dt_mem_next_cell(dt_root_addr_cells, &prop);
+	elfcorehdr_size = dt_mem_next_cell(dt_root_size_cells, &prop);
+
+	pr_debug("elfcorehdr_start=0x%llx elfcorehdr_size=0x%llx\n",
+		 elfcorehdr_addr, elfcorehdr_size);
+}
+#else
+static inline void early_init_dt_check_for_elfcorehdr(unsigned long node)
+{
+}
+#endif
+
 #ifdef CONFIG_SERIAL_EARLYCON
 
 int __init early_init_dt_scan_chosen_stdout(void)
@@ -1057,6 +1087,7 @@ int __init early_init_dt_scan_chosen(unsigned long node, const char *uname,
 		return 0;
 
 	early_init_dt_check_for_initrd(node);
+	early_init_dt_check_for_elfcorehdr(node);
 
 	/* Retrieve command line */
 	p = of_get_flat_dt_prop(node, "bootargs", &l);
@@ -1201,14 +1232,14 @@ void __init early_init_dt_scan_nodes(void)
 {
 	int rc = 0;
 
+	/* Initialize {size,address}-cells info */
+	of_scan_flat_dt(early_init_dt_scan_root, NULL);
+
 	/* Retrieve various information from the /chosen node */
 	rc = of_scan_flat_dt(early_init_dt_scan_chosen, boot_command_line);
 	if (!rc)
 		pr_warn("No chosen node found, continuing without\n");
 
-	/* Initialize {size,address}-cells info */
-	of_scan_flat_dt(early_init_dt_scan_root, NULL);
-
 	/* Setup memory, calling early_init_dt_add_memory_arch */
 	of_scan_flat_dt(early_init_dt_scan_memory, NULL);
 }
-- 
2.25.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH 1/3] of: fdt: Add generic support for parsing elf core header properties

[Geert Uytterhoeven]

There are two methods to specify the location of the elf core header:
using the "elfcorehdr=" kernel parameter, as handled by generic code in
kernel/crash_dump.c, or using the "linux,elfcorehdr" property under the
"/chosen" node in the Device Tree, as handled by architecture-specific
code in arch/arm64/mm/init.c.

Extend support for "linux,elfcorehdr" to all platforms supporting DT by
adding platform-agnostic handling for parsing this property to the FDT
core code.  This can co-exist safely with the architecture-specific
handling, until the latter has been removed.

This requires moving the call to of_scan_flat_dt() up, as the code
scanning the "/chosen" node now needs to be aware of the values of
"#address-cells" and "#size-cells".

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 Documentation/devicetree/bindings/chosen.txt |  6 ++--
 drivers/of/fdt.c                             | 37 ++++++++++++++++++--
 2 files changed, 37 insertions(+), 6 deletions(-)

diff --git a/Documentation/devicetree/bindings/chosen.txt b/Documentation/devicetree/bindings/chosen.txt
index 45e79172a646c537..5b0b94eb2d04e79d 100644
--- a/Documentation/devicetree/bindings/chosen.txt
+++ b/Documentation/devicetree/bindings/chosen.txt
@@ -106,9 +106,9 @@ respectively, of the root node.
 linux,elfcorehdr
 ----------------
 
-This property (currently used only on arm64) holds the memory range,
-the address and the size, of the elf core header which mainly describes
-the panicked kernel's memory layout as PT_LOAD segments of elf format.
+This property holds the memory range, the address and the size, of the elf
+core header which mainly describes the panicked kernel's memory layout as
+PT_LOAD segments of elf format.
 e.g.
 
 / {
diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index a03d43f95495d8e1..f13db831c8028cce 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -8,6 +8,7 @@
 
 #define pr_fmt(fmt)	"OF: fdt: " fmt
 
+#include <linux/crash_dump.h>
 #include <linux/crc32.h>
 #include <linux/kernel.h>
 #include <linux/initrd.h>
@@ -909,6 +910,35 @@ static inline void early_init_dt_check_for_initrd(unsigned long node)
 }
 #endif /* CONFIG_BLK_DEV_INITRD */
 
+#ifdef CONFIG_CRASH_DUMP
+/**
+ * early_init_dt_check_for_elfcorehdr - Decode elfcorehdr location from flat
+ * tree
+ * @node: reference to node containing elfcorehdr location ('chosen')
+ */
+static void __init early_init_dt_check_for_elfcorehdr(unsigned long node)
+{
+	const __be32 *prop;
+	int len;
+
+	pr_debug("Looking for elfcorehdr property... ");
+
+	prop = of_get_flat_dt_prop(node, "linux,elfcorehdr", &len);
+	if (!prop || (len < (dt_root_addr_cells + dt_root_size_cells)))
+		return;
+
+	elfcorehdr_addr = dt_mem_next_cell(dt_root_addr_cells, &prop);
+	elfcorehdr_size = dt_mem_next_cell(dt_root_size_cells, &prop);
+
+	pr_debug("elfcorehdr_start=0x%llx elfcorehdr_size=0x%llx\n",
+		 elfcorehdr_addr, elfcorehdr_size);
+}
+#else
+static inline void early_init_dt_check_for_elfcorehdr(unsigned long node)
+{
+}
+#endif
+
 #ifdef CONFIG_SERIAL_EARLYCON
 
 int __init early_init_dt_scan_chosen_stdout(void)
@@ -1057,6 +1087,7 @@ int __init early_init_dt_scan_chosen(unsigned long node, const char *uname,
 		return 0;
 
 	early_init_dt_check_for_initrd(node);
+	early_init_dt_check_for_elfcorehdr(node);
 
 	/* Retrieve command line */
 	p = of_get_flat_dt_prop(node, "bootargs", &l);
@@ -1201,14 +1232,14 @@ void __init early_init_dt_scan_nodes(void)
 {
 	int rc = 0;
 
+	/* Initialize {size,address}-cells info */
+	of_scan_flat_dt(early_init_dt_scan_root, NULL);
+
 	/* Retrieve various information from the /chosen node */
 	rc = of_scan_flat_dt(early_init_dt_scan_chosen, boot_command_line);
 	if (!rc)
 		pr_warn("No chosen node found, continuing without\n");
 
-	/* Initialize {size,address}-cells info */
-	of_scan_flat_dt(early_init_dt_scan_root, NULL);
-
 	/* Setup memory, calling early_init_dt_add_memory_arch */
 	of_scan_flat_dt(early_init_dt_scan_memory, NULL);
 }
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/3] riscv: Remove non-standard linux,elfcorehdr handling

[Geert Uytterhoeven]

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>
---
 arch/riscv/mm/init.c | 20 --------------------
 1 file changed, 20 deletions(-)

diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
index 6fd36af52a8520b3..2c5c8a24199002a3 100644
--- a/arch/riscv/mm/init.c
+++ b/arch/riscv/mm/init.c
@@ -839,26 +839,6 @@ static void __init reserve_crashkernel(void)
 }
 #endif /* CONFIG_KEXEC_CORE */
 
-#ifdef CONFIG_CRASH_DUMP
-/*
- * We keep track of the ELF core header of the crashed
- * kernel with a reserved-memory region with compatible
- * string "linux,elfcorehdr". Here we register a callback
- * to populate elfcorehdr_addr/size when this region is
- * present. Note that this region will be marked as
- * reserved once we call early_init_fdt_scan_reserved_mem()
- * later on.
- */
-static int __init elfcore_hdr_setup(struct reserved_mem *rmem)
-{
-	elfcorehdr_addr = rmem->base;
-	elfcorehdr_size = rmem->size;
-	return 0;
-}
-
-RESERVEDMEM_OF_DECLARE(elfcorehdr, "linux,elfcorehdr", elfcore_hdr_setup);
-#endif
-
 void __init paging_init(void)
 {
 	setup_bootmem();
-- 
2.25.1

[PATCH 2/3] riscv: Remove non-standard linux,elfcorehdr handling

[Geert Uytterhoeven]

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>
---
 arch/riscv/mm/init.c | 20 --------------------
 1 file changed, 20 deletions(-)

diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
index 6fd36af52a8520b3..2c5c8a24199002a3 100644
--- a/arch/riscv/mm/init.c
+++ b/arch/riscv/mm/init.c
@@ -839,26 +839,6 @@ static void __init reserve_crashkernel(void)
 }
 #endif /* CONFIG_KEXEC_CORE */
 
-#ifdef CONFIG_CRASH_DUMP
-/*
- * We keep track of the ELF core header of the crashed
- * kernel with a reserved-memory region with compatible
- * string "linux,elfcorehdr". Here we register a callback
- * to populate elfcorehdr_addr/size when this region is
- * present. Note that this region will be marked as
- * reserved once we call early_init_fdt_scan_reserved_mem()
- * later on.
- */
-static int __init elfcore_hdr_setup(struct reserved_mem *rmem)
-{
-	elfcorehdr_addr = rmem->base;
-	elfcorehdr_size = rmem->size;
-	return 0;
-}
-
-RESERVEDMEM_OF_DECLARE(elfcorehdr, "linux,elfcorehdr", elfcore_hdr_setup);
-#endif
-
 void __init paging_init(void)
 {
 	setup_bootmem();
-- 
2.25.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH 2/3] riscv: Remove non-standard linux,elfcorehdr handling

[Geert Uytterhoeven]

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>
---
 arch/riscv/mm/init.c | 20 --------------------
 1 file changed, 20 deletions(-)

diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
index 6fd36af52a8520b3..2c5c8a24199002a3 100644
--- a/arch/riscv/mm/init.c
+++ b/arch/riscv/mm/init.c
@@ -839,26 +839,6 @@ static void __init reserve_crashkernel(void)
 }
 #endif /* CONFIG_KEXEC_CORE */
 
-#ifdef CONFIG_CRASH_DUMP
-/*
- * We keep track of the ELF core header of the crashed
- * kernel with a reserved-memory region with compatible
- * string "linux,elfcorehdr". Here we register a callback
- * to populate elfcorehdr_addr/size when this region is
- * present. Note that this region will be marked as
- * reserved once we call early_init_fdt_scan_reserved_mem()
- * later on.
- */
-static int __init elfcore_hdr_setup(struct reserved_mem *rmem)
-{
-	elfcorehdr_addr = rmem->base;
-	elfcorehdr_size = rmem->size;
-	return 0;
-}
-
-RESERVEDMEM_OF_DECLARE(elfcorehdr, "linux,elfcorehdr", elfcore_hdr_setup);
-#endif
-
 void __init paging_init(void)
 {
 	setup_bootmem();
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 3/3] arm64: kdump: Remove custom linux,elfcorehdr parsing

[Geert Uytterhoeven]

Remove the architecture-specific code for handling the
"linux,elfcorehdr" property under the "/chosen" node in DT, as the
platform-agnostic handling in the FDT core code already takes care of
this.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 arch/arm64/mm/init.c | 21 ---------------------
 1 file changed, 21 deletions(-)

diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
index 6e1ca044ca907cd0..af06bfb6e2838d0f 100644
--- a/arch/arm64/mm/init.c
+++ b/arch/arm64/mm/init.c
@@ -125,25 +125,6 @@ static void __init reserve_crashkernel(void)
 #endif /* CONFIG_KEXEC_CORE */
 
 #ifdef CONFIG_CRASH_DUMP
-static int __init early_init_dt_scan_elfcorehdr(unsigned long node,
-		const char *uname, int depth, void *data)
-{
-	const __be32 *reg;
-	int len;
-
-	if (depth != 1 || strcmp(uname, "chosen") != 0)
-		return 0;
-
-	reg = of_get_flat_dt_prop(node, "linux,elfcorehdr", &len);
-	if (!reg || (len < (dt_root_addr_cells + dt_root_size_cells)))
-		return 1;
-
-	elfcorehdr_addr = dt_mem_next_cell(dt_root_addr_cells, &reg);
-	elfcorehdr_size = dt_mem_next_cell(dt_root_size_cells, &reg);
-
-	return 1;
-}
-
 /*
  * reserve_elfcorehdr() - reserves memory for elf core header
  *
@@ -154,8 +135,6 @@ static int __init early_init_dt_scan_elfcorehdr(unsigned long node,
  */
 static void __init reserve_elfcorehdr(void)
 {
-	of_scan_flat_dt(early_init_dt_scan_elfcorehdr, NULL);
-
 	if (!elfcorehdr_size)
 		return;
 
-- 
2.25.1

[PATCH 3/3] arm64: kdump: Remove custom linux,elfcorehdr parsing

[Geert Uytterhoeven]

Remove the architecture-specific code for handling the
"linux,elfcorehdr" property under the "/chosen" node in DT, as the
platform-agnostic handling in the FDT core code already takes care of
this.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 arch/arm64/mm/init.c | 21 ---------------------
 1 file changed, 21 deletions(-)

diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
index 6e1ca044ca907cd0..af06bfb6e2838d0f 100644
--- a/arch/arm64/mm/init.c
+++ b/arch/arm64/mm/init.c
@@ -125,25 +125,6 @@ static void __init reserve_crashkernel(void)
 #endif /* CONFIG_KEXEC_CORE */
 
 #ifdef CONFIG_CRASH_DUMP
-static int __init early_init_dt_scan_elfcorehdr(unsigned long node,
-		const char *uname, int depth, void *data)
-{
-	const __be32 *reg;
-	int len;
-
-	if (depth != 1 || strcmp(uname, "chosen") != 0)
-		return 0;
-
-	reg = of_get_flat_dt_prop(node, "linux,elfcorehdr", &len);
-	if (!reg || (len < (dt_root_addr_cells + dt_root_size_cells)))
-		return 1;
-
-	elfcorehdr_addr = dt_mem_next_cell(dt_root_addr_cells, &reg);
-	elfcorehdr_size = dt_mem_next_cell(dt_root_size_cells, &reg);
-
-	return 1;
-}
-
 /*
  * reserve_elfcorehdr() - reserves memory for elf core header
  *
@@ -154,8 +135,6 @@ static int __init early_init_dt_scan_elfcorehdr(unsigned long node,
  */
 static void __init reserve_elfcorehdr(void)
 {
-	of_scan_flat_dt(early_init_dt_scan_elfcorehdr, NULL);
-
 	if (!elfcorehdr_size)
 		return;
 
-- 
2.25.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH 3/3] arm64: kdump: Remove custom linux,elfcorehdr parsing

[Geert Uytterhoeven]

Remove the architecture-specific code for handling the
"linux,elfcorehdr" property under the "/chosen" node in DT, as the
platform-agnostic handling in the FDT core code already takes care of
this.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 arch/arm64/mm/init.c | 21 ---------------------
 1 file changed, 21 deletions(-)

diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
index 6e1ca044ca907cd0..af06bfb6e2838d0f 100644
--- a/arch/arm64/mm/init.c
+++ b/arch/arm64/mm/init.c
@@ -125,25 +125,6 @@ static void __init reserve_crashkernel(void)
 #endif /* CONFIG_KEXEC_CORE */
 
 #ifdef CONFIG_CRASH_DUMP
-static int __init early_init_dt_scan_elfcorehdr(unsigned long node,
-		const char *uname, int depth, void *data)
-{
-	const __be32 *reg;
-	int len;
-
-	if (depth != 1 || strcmp(uname, "chosen") != 0)
-		return 0;
-
-	reg = of_get_flat_dt_prop(node, "linux,elfcorehdr", &len);
-	if (!reg || (len < (dt_root_addr_cells + dt_root_size_cells)))
-		return 1;
-
-	elfcorehdr_addr = dt_mem_next_cell(dt_root_addr_cells, &reg);
-	elfcorehdr_size = dt_mem_next_cell(dt_root_size_cells, &reg);
-
-	return 1;
-}
-
 /*
  * reserve_elfcorehdr() - reserves memory for elf core header
  *
@@ -154,8 +135,6 @@ static int __init early_init_dt_scan_elfcorehdr(unsigned long node,
  */
 static void __init reserve_elfcorehdr(void)
 {
-	of_scan_flat_dt(early_init_dt_scan_elfcorehdr, NULL);
-
 	if (!elfcorehdr_size)
 		return;
 
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/3] riscv: Remove non-standard linux,elfcorehdr handling

[Nick Kossifidis]

Στις 2021-06-15 21:17, Geert Uytterhoeven έγραψε:
> 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 
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.

Re: [PATCH 2/3] riscv: Remove non-standard linux,elfcorehdr handling

[Nick Kossifidis]

Στις 2021-06-15 21:17, Geert Uytterhoeven έγραψε:
> 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 
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.

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 2/3] riscv: Remove non-standard linux,elfcorehdr handling

[Nick Kossifidis]

Στις 2021-06-15 21:17, Geert Uytterhoeven έγραψε:
> 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 
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.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/3] riscv: Remove non-standard linux,elfcorehdr handling

[Rob Herring]

On Tue, Jun 15, 2021 at 12:40 PM Nick Kossifidis <mick@ics.forth.gr> wrote:
>
> Στις 2021-06-15 21:17, Geert Uytterhoeven έγραψε:
> > 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.

> 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.

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.

Rob

Re: [PATCH 2/3] riscv: Remove non-standard linux,elfcorehdr handling

[Rob Herring]

On Tue, Jun 15, 2021 at 12:40 PM Nick Kossifidis <mick@ics.forth.gr> wrote:
>
> Στις 2021-06-15 21:17, Geert Uytterhoeven έγραψε:
> > 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.

> 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.

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.

Rob

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 2/3] riscv: Remove non-standard linux,elfcorehdr handling

[Rob Herring]

On Tue, Jun 15, 2021 at 12:40 PM Nick Kossifidis <mick@ics.forth.gr> wrote:
>
> Στις 2021-06-15 21:17, Geert Uytterhoeven έγραψε:
> > 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.

> 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.

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.

Rob

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/3] of: fdt: Add generic support for parsing elf core header properties

[Rob Herring]

On Tue, Jun 15, 2021 at 12:17 PM Geert Uytterhoeven
<geert+renesas@glider.be> wrote:
>
> There are two methods to specify the location of the elf core header:
> using the "elfcorehdr=" kernel parameter, as handled by generic code in
> kernel/crash_dump.c, or using the "linux,elfcorehdr" property under the
> "/chosen" node in the Device Tree, as handled by architecture-specific
> code in arch/arm64/mm/init.c.
>
> Extend support for "linux,elfcorehdr" to all platforms supporting DT by
> adding platform-agnostic handling for parsing this property to the FDT
> core code.  This can co-exist safely with the architecture-specific
> handling, until the latter has been removed.
>
> This requires moving the call to of_scan_flat_dt() up, as the code
> scanning the "/chosen" node now needs to be aware of the values of
> "#address-cells" and "#size-cells".
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
>  Documentation/devicetree/bindings/chosen.txt |  6 ++--
>  drivers/of/fdt.c                             | 37 ++++++++++++++++++--
>  2 files changed, 37 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/chosen.txt b/Documentation/devicetree/bindings/chosen.txt
> index 45e79172a646c537..5b0b94eb2d04e79d 100644
> --- a/Documentation/devicetree/bindings/chosen.txt
> +++ b/Documentation/devicetree/bindings/chosen.txt
> @@ -106,9 +106,9 @@ respectively, of the root node.
>  linux,elfcorehdr
>  ----------------
>
> -This property (currently used only on arm64) holds the memory range,
> -the address and the size, of the elf core header which mainly describes
> -the panicked kernel's memory layout as PT_LOAD segments of elf format.
> +This property holds the memory range, the address and the size, of the elf
> +core header which mainly describes the panicked kernel's memory layout as
> +PT_LOAD segments of elf format.
>  e.g.
>
>  / {
> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> index a03d43f95495d8e1..f13db831c8028cce 100644
> --- a/drivers/of/fdt.c
> +++ b/drivers/of/fdt.c
> @@ -8,6 +8,7 @@
>
>  #define pr_fmt(fmt)    "OF: fdt: " fmt
>
> +#include <linux/crash_dump.h>
>  #include <linux/crc32.h>
>  #include <linux/kernel.h>
>  #include <linux/initrd.h>
> @@ -909,6 +910,35 @@ static inline void early_init_dt_check_for_initrd(unsigned long node)
>  }
>  #endif /* CONFIG_BLK_DEV_INITRD */
>
> +#ifdef CONFIG_CRASH_DUMP
> +/**
> + * early_init_dt_check_for_elfcorehdr - Decode elfcorehdr location from flat
> + * tree
> + * @node: reference to node containing elfcorehdr location ('chosen')
> + */
> +static void __init early_init_dt_check_for_elfcorehdr(unsigned long node)
> +{
> +       const __be32 *prop;
> +       int len;
> +
> +       pr_debug("Looking for elfcorehdr property... ");
> +
> +       prop = of_get_flat_dt_prop(node, "linux,elfcorehdr", &len);
> +       if (!prop || (len < (dt_root_addr_cells + dt_root_size_cells)))
> +               return;
> +
> +       elfcorehdr_addr = dt_mem_next_cell(dt_root_addr_cells, &prop);
> +       elfcorehdr_size = dt_mem_next_cell(dt_root_size_cells, &prop);

If these declarations were moved outside the '#ifdef
CONFIG_CRASH_DUMP' in crash_dump.h, then IS_ENABLED() could be used in
this function.


> +
> +       pr_debug("elfcorehdr_start=0x%llx elfcorehdr_size=0x%llx\n",
> +                elfcorehdr_addr, elfcorehdr_size);
> +}
> +#else
> +static inline void early_init_dt_check_for_elfcorehdr(unsigned long node)
> +{
> +}
> +#endif
> +
>  #ifdef CONFIG_SERIAL_EARLYCON
>
>  int __init early_init_dt_scan_chosen_stdout(void)
> @@ -1057,6 +1087,7 @@ int __init early_init_dt_scan_chosen(unsigned long node, const char *uname,
>                 return 0;
>
>         early_init_dt_check_for_initrd(node);
> +       early_init_dt_check_for_elfcorehdr(node);
>
>         /* Retrieve command line */
>         p = of_get_flat_dt_prop(node, "bootargs", &l);
> @@ -1201,14 +1232,14 @@ void __init early_init_dt_scan_nodes(void)
>  {
>         int rc = 0;
>
> +       /* Initialize {size,address}-cells info */
> +       of_scan_flat_dt(early_init_dt_scan_root, NULL);
> +
>         /* Retrieve various information from the /chosen node */
>         rc = of_scan_flat_dt(early_init_dt_scan_chosen, boot_command_line);
>         if (!rc)
>                 pr_warn("No chosen node found, continuing without\n");
>
> -       /* Initialize {size,address}-cells info */
> -       of_scan_flat_dt(early_init_dt_scan_root, NULL);
> -
>         /* Setup memory, calling early_init_dt_add_memory_arch */
>         of_scan_flat_dt(early_init_dt_scan_memory, NULL);
>  }
> --
> 2.25.1
>

Re: [PATCH 1/3] of: fdt: Add generic support for parsing elf core header properties

[Rob Herring]

On Tue, Jun 15, 2021 at 12:17 PM Geert Uytterhoeven
<geert+renesas@glider.be> wrote:
>
> There are two methods to specify the location of the elf core header:
> using the "elfcorehdr=" kernel parameter, as handled by generic code in
> kernel/crash_dump.c, or using the "linux,elfcorehdr" property under the
> "/chosen" node in the Device Tree, as handled by architecture-specific
> code in arch/arm64/mm/init.c.
>
> Extend support for "linux,elfcorehdr" to all platforms supporting DT by
> adding platform-agnostic handling for parsing this property to the FDT
> core code.  This can co-exist safely with the architecture-specific
> handling, until the latter has been removed.
>
> This requires moving the call to of_scan_flat_dt() up, as the code
> scanning the "/chosen" node now needs to be aware of the values of
> "#address-cells" and "#size-cells".
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
>  Documentation/devicetree/bindings/chosen.txt |  6 ++--
>  drivers/of/fdt.c                             | 37 ++++++++++++++++++--
>  2 files changed, 37 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/chosen.txt b/Documentation/devicetree/bindings/chosen.txt
> index 45e79172a646c537..5b0b94eb2d04e79d 100644
> --- a/Documentation/devicetree/bindings/chosen.txt
> +++ b/Documentation/devicetree/bindings/chosen.txt
> @@ -106,9 +106,9 @@ respectively, of the root node.
>  linux,elfcorehdr
>  ----------------
>
> -This property (currently used only on arm64) holds the memory range,
> -the address and the size, of the elf core header which mainly describes
> -the panicked kernel's memory layout as PT_LOAD segments of elf format.
> +This property holds the memory range, the address and the size, of the elf
> +core header which mainly describes the panicked kernel's memory layout as
> +PT_LOAD segments of elf format.
>  e.g.
>
>  / {
> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> index a03d43f95495d8e1..f13db831c8028cce 100644
> --- a/drivers/of/fdt.c
> +++ b/drivers/of/fdt.c
> @@ -8,6 +8,7 @@
>
>  #define pr_fmt(fmt)    "OF: fdt: " fmt
>
> +#include <linux/crash_dump.h>
>  #include <linux/crc32.h>
>  #include <linux/kernel.h>
>  #include <linux/initrd.h>
> @@ -909,6 +910,35 @@ static inline void early_init_dt_check_for_initrd(unsigned long node)
>  }
>  #endif /* CONFIG_BLK_DEV_INITRD */
>
> +#ifdef CONFIG_CRASH_DUMP
> +/**
> + * early_init_dt_check_for_elfcorehdr - Decode elfcorehdr location from flat
> + * tree
> + * @node: reference to node containing elfcorehdr location ('chosen')
> + */
> +static void __init early_init_dt_check_for_elfcorehdr(unsigned long node)
> +{
> +       const __be32 *prop;
> +       int len;
> +
> +       pr_debug("Looking for elfcorehdr property... ");
> +
> +       prop = of_get_flat_dt_prop(node, "linux,elfcorehdr", &len);
> +       if (!prop || (len < (dt_root_addr_cells + dt_root_size_cells)))
> +               return;
> +
> +       elfcorehdr_addr = dt_mem_next_cell(dt_root_addr_cells, &prop);
> +       elfcorehdr_size = dt_mem_next_cell(dt_root_size_cells, &prop);

If these declarations were moved outside the '#ifdef
CONFIG_CRASH_DUMP' in crash_dump.h, then IS_ENABLED() could be used in
this function.


> +
> +       pr_debug("elfcorehdr_start=0x%llx elfcorehdr_size=0x%llx\n",
> +                elfcorehdr_addr, elfcorehdr_size);
> +}
> +#else
> +static inline void early_init_dt_check_for_elfcorehdr(unsigned long node)
> +{
> +}
> +#endif
> +
>  #ifdef CONFIG_SERIAL_EARLYCON
>
>  int __init early_init_dt_scan_chosen_stdout(void)
> @@ -1057,6 +1087,7 @@ int __init early_init_dt_scan_chosen(unsigned long node, const char *uname,
>                 return 0;
>
>         early_init_dt_check_for_initrd(node);
> +       early_init_dt_check_for_elfcorehdr(node);
>
>         /* Retrieve command line */
>         p = of_get_flat_dt_prop(node, "bootargs", &l);
> @@ -1201,14 +1232,14 @@ void __init early_init_dt_scan_nodes(void)
>  {
>         int rc = 0;
>
> +       /* Initialize {size,address}-cells info */
> +       of_scan_flat_dt(early_init_dt_scan_root, NULL);
> +
>         /* Retrieve various information from the /chosen node */
>         rc = of_scan_flat_dt(early_init_dt_scan_chosen, boot_command_line);
>         if (!rc)
>                 pr_warn("No chosen node found, continuing without\n");
>
> -       /* Initialize {size,address}-cells info */
> -       of_scan_flat_dt(early_init_dt_scan_root, NULL);
> -
>         /* Setup memory, calling early_init_dt_add_memory_arch */
>         of_scan_flat_dt(early_init_dt_scan_memory, NULL);
>  }
> --
> 2.25.1
>

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 1/3] of: fdt: Add generic support for parsing elf core header properties

[Rob Herring]

On Tue, Jun 15, 2021 at 12:17 PM Geert Uytterhoeven
<geert+renesas@glider.be> wrote:
>
> There are two methods to specify the location of the elf core header:
> using the "elfcorehdr=" kernel parameter, as handled by generic code in
> kernel/crash_dump.c, or using the "linux,elfcorehdr" property under the
> "/chosen" node in the Device Tree, as handled by architecture-specific
> code in arch/arm64/mm/init.c.
>
> Extend support for "linux,elfcorehdr" to all platforms supporting DT by
> adding platform-agnostic handling for parsing this property to the FDT
> core code.  This can co-exist safely with the architecture-specific
> handling, until the latter has been removed.
>
> This requires moving the call to of_scan_flat_dt() up, as the code
> scanning the "/chosen" node now needs to be aware of the values of
> "#address-cells" and "#size-cells".
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
>  Documentation/devicetree/bindings/chosen.txt |  6 ++--
>  drivers/of/fdt.c                             | 37 ++++++++++++++++++--
>  2 files changed, 37 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/chosen.txt b/Documentation/devicetree/bindings/chosen.txt
> index 45e79172a646c537..5b0b94eb2d04e79d 100644
> --- a/Documentation/devicetree/bindings/chosen.txt
> +++ b/Documentation/devicetree/bindings/chosen.txt
> @@ -106,9 +106,9 @@ respectively, of the root node.
>  linux,elfcorehdr
>  ----------------
>
> -This property (currently used only on arm64) holds the memory range,
> -the address and the size, of the elf core header which mainly describes
> -the panicked kernel's memory layout as PT_LOAD segments of elf format.
> +This property holds the memory range, the address and the size, of the elf
> +core header which mainly describes the panicked kernel's memory layout as
> +PT_LOAD segments of elf format.
>  e.g.
>
>  / {
> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> index a03d43f95495d8e1..f13db831c8028cce 100644
> --- a/drivers/of/fdt.c
> +++ b/drivers/of/fdt.c
> @@ -8,6 +8,7 @@
>
>  #define pr_fmt(fmt)    "OF: fdt: " fmt
>
> +#include <linux/crash_dump.h>
>  #include <linux/crc32.h>
>  #include <linux/kernel.h>
>  #include <linux/initrd.h>
> @@ -909,6 +910,35 @@ static inline void early_init_dt_check_for_initrd(unsigned long node)
>  }
>  #endif /* CONFIG_BLK_DEV_INITRD */
>
> +#ifdef CONFIG_CRASH_DUMP
> +/**
> + * early_init_dt_check_for_elfcorehdr - Decode elfcorehdr location from flat
> + * tree
> + * @node: reference to node containing elfcorehdr location ('chosen')
> + */
> +static void __init early_init_dt_check_for_elfcorehdr(unsigned long node)
> +{
> +       const __be32 *prop;
> +       int len;
> +
> +       pr_debug("Looking for elfcorehdr property... ");
> +
> +       prop = of_get_flat_dt_prop(node, "linux,elfcorehdr", &len);
> +       if (!prop || (len < (dt_root_addr_cells + dt_root_size_cells)))
> +               return;
> +
> +       elfcorehdr_addr = dt_mem_next_cell(dt_root_addr_cells, &prop);
> +       elfcorehdr_size = dt_mem_next_cell(dt_root_size_cells, &prop);

If these declarations were moved outside the '#ifdef
CONFIG_CRASH_DUMP' in crash_dump.h, then IS_ENABLED() could be used in
this function.


> +
> +       pr_debug("elfcorehdr_start=0x%llx elfcorehdr_size=0x%llx\n",
> +                elfcorehdr_addr, elfcorehdr_size);
> +}
> +#else
> +static inline void early_init_dt_check_for_elfcorehdr(unsigned long node)
> +{
> +}
> +#endif
> +
>  #ifdef CONFIG_SERIAL_EARLYCON
>
>  int __init early_init_dt_scan_chosen_stdout(void)
> @@ -1057,6 +1087,7 @@ int __init early_init_dt_scan_chosen(unsigned long node, const char *uname,
>                 return 0;
>
>         early_init_dt_check_for_initrd(node);
> +       early_init_dt_check_for_elfcorehdr(node);
>
>         /* Retrieve command line */
>         p = of_get_flat_dt_prop(node, "bootargs", &l);
> @@ -1201,14 +1232,14 @@ void __init early_init_dt_scan_nodes(void)
>  {
>         int rc = 0;
>
> +       /* Initialize {size,address}-cells info */
> +       of_scan_flat_dt(early_init_dt_scan_root, NULL);
> +
>         /* Retrieve various information from the /chosen node */
>         rc = of_scan_flat_dt(early_init_dt_scan_chosen, boot_command_line);
>         if (!rc)
>                 pr_warn("No chosen node found, continuing without\n");
>
> -       /* Initialize {size,address}-cells info */
> -       of_scan_flat_dt(early_init_dt_scan_root, NULL);
> -
>         /* Setup memory, calling early_init_dt_add_memory_arch */
>         of_scan_flat_dt(early_init_dt_scan_memory, NULL);
>  }
> --
> 2.25.1
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/3] riscv: Remove non-standard linux,elfcorehdr handling

[Nick Kossifidis]

Στις 2021-06-15 22:54, Rob Herring έγραψε:
> On Tue, Jun 15, 2021 at 12:40 PM Nick Kossifidis <mick@ics.forth.gr> 
> wrote:
>> 
>> Στις 2021-06-15 21:17, Geert Uytterhoeven έγραψε:
>> > 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 
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.

>> 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).

> 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.

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.

Regards,
Nick

Re: [PATCH 2/3] riscv: Remove non-standard linux,elfcorehdr handling

[Nick Kossifidis]

Στις 2021-06-15 22:54, Rob Herring έγραψε:
> On Tue, Jun 15, 2021 at 12:40 PM Nick Kossifidis <mick@ics.forth.gr> 
> wrote:
>> 
>> Στις 2021-06-15 21:17, Geert Uytterhoeven έγραψε:
>> > 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 
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.

>> 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).

> 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.

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.

Regards,
Nick



_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 2/3] riscv: Remove non-standard linux,elfcorehdr handling

[Nick Kossifidis]

Στις 2021-06-15 22:54, Rob Herring έγραψε:
> On Tue, Jun 15, 2021 at 12:40 PM Nick Kossifidis <mick@ics.forth.gr> 
> wrote:
>> 
>> Στις 2021-06-15 21:17, Geert Uytterhoeven έγραψε:
>> > 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 
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.

>> 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).

> 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.

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.

Regards,
Nick



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/3] of: fdt: Add generic support for parsing elf core header properties

[Nick Kossifidis]

Στις 2021-06-15 21:17, Geert Uytterhoeven έγραψε:
> 
> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c

I believe drivers/of/kexec.c is better suited for this.

Re: [PATCH 1/3] of: fdt: Add generic support for parsing elf core header properties

[Nick Kossifidis]

Στις 2021-06-15 21:17, Geert Uytterhoeven έγραψε:
> 
> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c

I believe drivers/of/kexec.c is better suited for this.

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 1/3] of: fdt: Add generic support for parsing elf core header properties

[Nick Kossifidis]

Στις 2021-06-15 21:17, Geert Uytterhoeven έγραψε:
> 
> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c

I believe drivers/of/kexec.c is better suited for this.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/3] riscv: Remove non-standard linux,elfcorehdr handling

[Geert Uytterhoeven]

Hi Nick,

On Wed, Jun 16, 2021 at 1:19 AM Nick Kossifidis <mick@ics.forth.gr> wrote:
> Στις 2021-06-15 22:54, Rob Herring έγραψε:
> > On Tue, Jun 15, 2021 at 12:40 PM Nick Kossifidis <mick@ics.forth.gr>
> > wrote:
> >> Στις 2021-06-15 21:17, Geert Uytterhoeven έγραψε:
> >> > 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.

> >> 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).
>
> > 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

Re: [PATCH 2/3] riscv: Remove non-standard linux,elfcorehdr handling

[Geert Uytterhoeven]

Hi Nick,

On Wed, Jun 16, 2021 at 1:19 AM Nick Kossifidis <mick@ics.forth.gr> wrote:
> Στις 2021-06-15 22:54, Rob Herring έγραψε:
> > On Tue, Jun 15, 2021 at 12:40 PM Nick Kossifidis <mick@ics.forth.gr>
> > wrote:
> >> Στις 2021-06-15 21:17, Geert Uytterhoeven έγραψε:
> >> > 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.

> >> 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).
>
> > 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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 2/3] riscv: Remove non-standard linux,elfcorehdr handling

[Geert Uytterhoeven]

Hi Nick,

On Wed, Jun 16, 2021 at 1:19 AM Nick Kossifidis <mick@ics.forth.gr> wrote:
> Στις 2021-06-15 22:54, Rob Herring έγραψε:
> > On Tue, Jun 15, 2021 at 12:40 PM Nick Kossifidis <mick@ics.forth.gr>
> > wrote:
> >> Στις 2021-06-15 21:17, Geert Uytterhoeven έγραψε:
> >> > 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.

> >> 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).
>
> > 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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/3] of: fdt: Add generic support for parsing elf core header properties

[Geert Uytterhoeven]

Hi Nick,

On Wed, Jun 16, 2021 at 1:28 AM Nick Kossifidis <mick@ics.forth.gr> wrote:
>
> Στις 2021-06-15 21:17, Geert Uytterhoeven έγραψε:
> >
> > diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
>
> I believe drivers/of/kexec.c is better suited for this.

From an earlier response from me to a comment from Rob:

| > Also, note that there is now a drivers/of/kexec.c (in -next) though
| > not sure if all this would go there or stay in fdt.c with the rest of
| > the memory parsing.
|
| It's gonna be the latter, as that file handles the FDT during early
| kernel startup, for both normal and kdump kernels.
|
| Despite the name, drivers/of/kexec.c is not for kexec, but for
| kexec_file.  This is the "new" fancy syscall that prepares the DTB
| for the new kernel itself, unlike the classic kexec syscall, where
| userspace is responsible for preparing the DTB for the new kernel.

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/3] of: fdt: Add generic support for parsing elf core header properties

[Geert Uytterhoeven]

Hi Nick,

On Wed, Jun 16, 2021 at 1:28 AM Nick Kossifidis <mick@ics.forth.gr> wrote:
>
> Στις 2021-06-15 21:17, Geert Uytterhoeven έγραψε:
> >
> > diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
>
> I believe drivers/of/kexec.c is better suited for this.

From an earlier response from me to a comment from Rob:

| > Also, note that there is now a drivers/of/kexec.c (in -next) though
| > not sure if all this would go there or stay in fdt.c with the rest of
| > the memory parsing.
|
| It's gonna be the latter, as that file handles the FDT during early
| kernel startup, for both normal and kdump kernels.
|
| Despite the name, drivers/of/kexec.c is not for kexec, but for
| kexec_file.  This is the "new" fancy syscall that prepares the DTB
| for the new kernel itself, unlike the classic kexec syscall, where
| userspace is responsible for preparing the DTB for the new kernel.

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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 1/3] of: fdt: Add generic support for parsing elf core header properties

[Geert Uytterhoeven]

Hi Nick,

On Wed, Jun 16, 2021 at 1:28 AM Nick Kossifidis <mick@ics.forth.gr> wrote:
>
> Στις 2021-06-15 21:17, Geert Uytterhoeven έγραψε:
> >
> > diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
>
> I believe drivers/of/kexec.c is better suited for this.

From an earlier response from me to a comment from Rob:

| > Also, note that there is now a drivers/of/kexec.c (in -next) though
| > not sure if all this would go there or stay in fdt.c with the rest of
| > the memory parsing.
|
| It's gonna be the latter, as that file handles the FDT during early
| kernel startup, for both normal and kdump kernels.
|
| Despite the name, drivers/of/kexec.c is not for kexec, but for
| kexec_file.  This is the "new" fancy syscall that prepares the DTB
| for the new kernel itself, unlike the classic kexec syscall, where
| userspace is responsible for preparing the DTB for the new kernel.

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/3] riscv: Remove non-standard linux,elfcorehdr handling

[Nick Kossifidis]

Στις 2021-06-16 10:56, Geert Uytterhoeven έγραψε:
> 
> 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.
> 

We already have "linux,usable-memory" on /memory node:
https://elixir.bootlin.com/linux/v5.13-rc6/source/drivers/of/fdt.c#L1011
and it makes perfect sense to be there since it overrides /memory's reg 
property.

Why define another binding for the same thing on /chosen ?

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

I'll add the entry from kexec-tools, so that the kernel will reserve the 
region as part of:
https://elixir.bootlin.com/linux/v5.13-rc6/source/drivers/of/fdt.c#L605

Re: [PATCH 2/3] riscv: Remove non-standard linux,elfcorehdr handling

[Nick Kossifidis]

Στις 2021-06-16 10:56, Geert Uytterhoeven έγραψε:
> 
> 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.
> 

We already have "linux,usable-memory" on /memory node:
https://elixir.bootlin.com/linux/v5.13-rc6/source/drivers/of/fdt.c#L1011
and it makes perfect sense to be there since it overrides /memory's reg 
property.

Why define another binding for the same thing on /chosen ?

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

I'll add the entry from kexec-tools, so that the kernel will reserve the 
region as part of:
https://elixir.bootlin.com/linux/v5.13-rc6/source/drivers/of/fdt.c#L605

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 2/3] riscv: Remove non-standard linux,elfcorehdr handling

[Nick Kossifidis]

Στις 2021-06-16 10:56, Geert Uytterhoeven έγραψε:
> 
> 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.
> 

We already have "linux,usable-memory" on /memory node:
https://elixir.bootlin.com/linux/v5.13-rc6/source/drivers/of/fdt.c#L1011
and it makes perfect sense to be there since it overrides /memory's reg 
property.

Why define another binding for the same thing on /chosen ?

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

I'll add the entry from kexec-tools, so that the kernel will reserve the 
region as part of:
https://elixir.bootlin.com/linux/v5.13-rc6/source/drivers/of/fdt.c#L605

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/3] riscv: Remove non-standard linux,elfcorehdr handling

[Rob Herring]

On Wed, Jun 16, 2021 at 4:43 AM Nick Kossifidis <mick@ics.forth.gr> wrote:
>
> Στις 2021-06-16 10:56, Geert Uytterhoeven έγραψε:
> >
> > 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.
> >
>
> We already have "linux,usable-memory" on /memory node:
> https://elixir.bootlin.com/linux/v5.13-rc6/source/drivers/of/fdt.c#L1011
> and it makes perfect sense to be there since it overrides /memory's reg
> property.
>
> Why define another binding for the same thing on /chosen ?

Go look at the thread adding "linux,usable-memory-range". There were
only 35 versions of it[1]. I wasn't happy with a 2nd way either, but
as I've mentioned before we don't always have /memory node.

Rob

[1] https://lore.kernel.org/linux-arm-kernel/20170403022606.12609-1-takahiro.akashi@linaro.org/

Re: [PATCH 2/3] riscv: Remove non-standard linux,elfcorehdr handling

[Rob Herring]

On Wed, Jun 16, 2021 at 4:43 AM Nick Kossifidis <mick@ics.forth.gr> wrote:
>
> Στις 2021-06-16 10:56, Geert Uytterhoeven έγραψε:
> >
> > 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.
> >
>
> We already have "linux,usable-memory" on /memory node:
> https://elixir.bootlin.com/linux/v5.13-rc6/source/drivers/of/fdt.c#L1011
> and it makes perfect sense to be there since it overrides /memory's reg
> property.
>
> Why define another binding for the same thing on /chosen ?

Go look at the thread adding "linux,usable-memory-range". There were
only 35 versions of it[1]. I wasn't happy with a 2nd way either, but
as I've mentioned before we don't always have /memory node.

Rob

[1] https://lore.kernel.org/linux-arm-kernel/20170403022606.12609-1-takahiro.akashi@linaro.org/

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 2/3] riscv: Remove non-standard linux,elfcorehdr handling

[Rob Herring]

On Wed, Jun 16, 2021 at 4:43 AM Nick Kossifidis <mick@ics.forth.gr> wrote:
>
> Στις 2021-06-16 10:56, Geert Uytterhoeven έγραψε:
> >
> > 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.
> >
>
> We already have "linux,usable-memory" on /memory node:
> https://elixir.bootlin.com/linux/v5.13-rc6/source/drivers/of/fdt.c#L1011
> and it makes perfect sense to be there since it overrides /memory's reg
> property.
>
> Why define another binding for the same thing on /chosen ?

Go look at the thread adding "linux,usable-memory-range". There were
only 35 versions of it[1]. I wasn't happy with a 2nd way either, but
as I've mentioned before we don't always have /memory node.

Rob

[1] https://lore.kernel.org/linux-arm-kernel/20170403022606.12609-1-takahiro.akashi@linaro.org/

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/3] riscv: Remove non-standard linux,elfcorehdr handling

[Palmer Dabbelt]

On Wed, 16 Jun 2021 07:47:46 PDT (-0700), robh+dt@kernel.org wrote:
> On Wed, Jun 16, 2021 at 4:43 AM Nick Kossifidis <mick@ics.forth.gr> wrote:
>>
>> Στις 2021-06-16 10:56, Geert Uytterhoeven έγραψε:
>> >
>> > 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.
>> >
>>
>> We already have "linux,usable-memory" on /memory node:
>> https://elixir.bootlin.com/linux/v5.13-rc6/source/drivers/of/fdt.c#L1011
>> and it makes perfect sense to be there since it overrides /memory's reg
>> property.
>>
>> Why define another binding for the same thing on /chosen ?
>
> Go look at the thread adding "linux,usable-memory-range". There were
> only 35 versions of it[1]. I wasn't happy with a 2nd way either, but
> as I've mentioned before we don't always have /memory node.

I don't really understand what's going on here, but IIUC what I merged 
in 5.13 doesn't match the behavior that other architectures have.  In 
that case I'm happy moving RISC-V over to the more standard way of doing 
things and just calling what we have in 5.13 a screwup.

Sorry for the confusion.

Re: [PATCH 2/3] riscv: Remove non-standard linux,elfcorehdr handling

[Palmer Dabbelt]

On Wed, 16 Jun 2021 07:47:46 PDT (-0700), robh+dt@kernel.org wrote:
> On Wed, Jun 16, 2021 at 4:43 AM Nick Kossifidis <mick@ics.forth.gr> wrote:
>>
>> Στις 2021-06-16 10:56, Geert Uytterhoeven έγραψε:
>> >
>> > 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.
>> >
>>
>> We already have "linux,usable-memory" on /memory node:
>> https://elixir.bootlin.com/linux/v5.13-rc6/source/drivers/of/fdt.c#L1011
>> and it makes perfect sense to be there since it overrides /memory's reg
>> property.
>>
>> Why define another binding for the same thing on /chosen ?
>
> Go look at the thread adding "linux,usable-memory-range". There were
> only 35 versions of it[1]. I wasn't happy with a 2nd way either, but
> as I've mentioned before we don't always have /memory node.

I don't really understand what's going on here, but IIUC what I merged 
in 5.13 doesn't match the behavior that other architectures have.  In 
that case I'm happy moving RISC-V over to the more standard way of doing 
things and just calling what we have in 5.13 a screwup.

Sorry for the confusion.

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 2/3] riscv: Remove non-standard linux,elfcorehdr handling

[Palmer Dabbelt]

On Wed, 16 Jun 2021 07:47:46 PDT (-0700), robh+dt@kernel.org wrote:
> On Wed, Jun 16, 2021 at 4:43 AM Nick Kossifidis <mick@ics.forth.gr> wrote:
>>
>> Στις 2021-06-16 10:56, Geert Uytterhoeven έγραψε:
>> >
>> > 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.
>> >
>>
>> We already have "linux,usable-memory" on /memory node:
>> https://elixir.bootlin.com/linux/v5.13-rc6/source/drivers/of/fdt.c#L1011
>> and it makes perfect sense to be there since it overrides /memory's reg
>> property.
>>
>> Why define another binding for the same thing on /chosen ?
>
> Go look at the thread adding "linux,usable-memory-range". There were
> only 35 versions of it[1]. I wasn't happy with a 2nd way either, but
> as I've mentioned before we don't always have /memory node.

I don't really understand what's going on here, but IIUC what I merged 
in 5.13 doesn't match the behavior that other architectures have.  In 
that case I'm happy moving RISC-V over to the more standard way of doing 
things and just calling what we have in 5.13 a screwup.

Sorry for the confusion.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/3] riscv: Remove non-standard linux,elfcorehdr handling

[Nick Kossifidis]

Στις 2021-07-01 05:52, Palmer Dabbelt έγραψε:
> On Wed, 16 Jun 2021 07:47:46 PDT (-0700), robh+dt@kernel.org wrote:
>> On Wed, Jun 16, 2021 at 4:43 AM Nick Kossifidis <mick@ics.forth.gr> 
>> wrote:
>>> 
>>> Στις 2021-06-16 10:56, Geert Uytterhoeven έγραψε:
>>> >
>>> > 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.
>>> >
>>> 
>>> We already have "linux,usable-memory" on /memory node:
>>> https://elixir.bootlin.com/linux/v5.13-rc6/source/drivers/of/fdt.c#L1011
>>> and it makes perfect sense to be there since it overrides /memory's 
>>> reg
>>> property.
>>> 
>>> Why define another binding for the same thing on /chosen ?
>> 
>> Go look at the thread adding "linux,usable-memory-range". There were
>> only 35 versions of it[1]. I wasn't happy with a 2nd way either, but
>> as I've mentioned before we don't always have /memory node.
> 
> I don't really understand what's going on here, but IIUC what I merged
> in 5.13 doesn't match the behavior that other architectures have.  In
> that case I'm happy moving RISC-V over to the more standard way of
> doing things and just calling what we have in 5.13 a screwup.
> 
> Sorry for the confusion.

Long story short:

a) We use "linux,usable-memory" on /memory node to limit the memory of 
the kdump kernel, it's a standard binding defined at:
https://elixir.bootlin.com/linux/v5.13-rc6/source/drivers/of/fdt.c#L1011

b) We used a reserved region (again a standard binding) named 
"linux,elfcorehdr" which has the same name as a property on /chosen used 
by arm64 for the same thing. With this patch we 'll use arm64's 
approach, although it's a bit worse since we'll need to add the same 
region twice on the fdt (once in /chosen as a property and another one 
in the reservation map so that it gets reserved during early boot).

Fortunately I (still) haven't posted the kexec-tools patches on the 
mailing list so we don't break userspace by doing this.

Re: [PATCH 2/3] riscv: Remove non-standard linux,elfcorehdr handling

[Nick Kossifidis]

Στις 2021-07-01 05:52, Palmer Dabbelt έγραψε:
> On Wed, 16 Jun 2021 07:47:46 PDT (-0700), robh+dt@kernel.org wrote:
>> On Wed, Jun 16, 2021 at 4:43 AM Nick Kossifidis <mick@ics.forth.gr> 
>> wrote:
>>> 
>>> Στις 2021-06-16 10:56, Geert Uytterhoeven έγραψε:
>>> >
>>> > 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.
>>> >
>>> 
>>> We already have "linux,usable-memory" on /memory node:
>>> https://elixir.bootlin.com/linux/v5.13-rc6/source/drivers/of/fdt.c#L1011
>>> and it makes perfect sense to be there since it overrides /memory's 
>>> reg
>>> property.
>>> 
>>> Why define another binding for the same thing on /chosen ?
>> 
>> Go look at the thread adding "linux,usable-memory-range". There were
>> only 35 versions of it[1]. I wasn't happy with a 2nd way either, but
>> as I've mentioned before we don't always have /memory node.
> 
> I don't really understand what's going on here, but IIUC what I merged
> in 5.13 doesn't match the behavior that other architectures have.  In
> that case I'm happy moving RISC-V over to the more standard way of
> doing things and just calling what we have in 5.13 a screwup.
> 
> Sorry for the confusion.

Long story short:

a) We use "linux,usable-memory" on /memory node to limit the memory of 
the kdump kernel, it's a standard binding defined at:
https://elixir.bootlin.com/linux/v5.13-rc6/source/drivers/of/fdt.c#L1011

b) We used a reserved region (again a standard binding) named 
"linux,elfcorehdr" which has the same name as a property on /chosen used 
by arm64 for the same thing. With this patch we 'll use arm64's 
approach, although it's a bit worse since we'll need to add the same 
region twice on the fdt (once in /chosen as a property and another one 
in the reservation map so that it gets reserved during early boot).

Fortunately I (still) haven't posted the kexec-tools patches on the 
mailing list so we don't break userspace by doing this.

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 2/3] riscv: Remove non-standard linux,elfcorehdr handling

[Nick Kossifidis]

Στις 2021-07-01 05:52, Palmer Dabbelt έγραψε:
> On Wed, 16 Jun 2021 07:47:46 PDT (-0700), robh+dt@kernel.org wrote:
>> On Wed, Jun 16, 2021 at 4:43 AM Nick Kossifidis <mick@ics.forth.gr> 
>> wrote:
>>> 
>>> Στις 2021-06-16 10:56, Geert Uytterhoeven έγραψε:
>>> >
>>> > 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.
>>> >
>>> 
>>> We already have "linux,usable-memory" on /memory node:
>>> https://elixir.bootlin.com/linux/v5.13-rc6/source/drivers/of/fdt.c#L1011
>>> and it makes perfect sense to be there since it overrides /memory's 
>>> reg
>>> property.
>>> 
>>> Why define another binding for the same thing on /chosen ?
>> 
>> Go look at the thread adding "linux,usable-memory-range". There were
>> only 35 versions of it[1]. I wasn't happy with a 2nd way either, but
>> as I've mentioned before we don't always have /memory node.
> 
> I don't really understand what's going on here, but IIUC what I merged
> in 5.13 doesn't match the behavior that other architectures have.  In
> that case I'm happy moving RISC-V over to the more standard way of
> doing things and just calling what we have in 5.13 a screwup.
> 
> Sorry for the confusion.

Long story short:

a) We use "linux,usable-memory" on /memory node to limit the memory of 
the kdump kernel, it's a standard binding defined at:
https://elixir.bootlin.com/linux/v5.13-rc6/source/drivers/of/fdt.c#L1011

b) We used a reserved region (again a standard binding) named 
"linux,elfcorehdr" which has the same name as a property on /chosen used 
by arm64 for the same thing. With this patch we 'll use arm64's 
approach, although it's a bit worse since we'll need to add the same 
region twice on the fdt (once in /chosen as a property and another one 
in the reservation map so that it gets reserved during early boot).

Fortunately I (still) haven't posted the kexec-tools patches on the 
mailing list so we don't break userspace by doing this.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel