| Message ID | cover.1623780059.git.geert+renesas@glider.be |
|---|---|
| Headers | show |
| Series | Add generic-support for linux,elfcorehdr and fix riscv | expand |
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 >
Στις 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.
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
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
Στις 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
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/
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.
Στις 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.
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(-)