| Message ID | bcc5c666f4ada9a8bbc26f559751f0da67f769f8.1623780059.git.geert+renesas@glider.be (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add generic-support for linux,elfcorehdr and fix riscv | expand |
Στις 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.
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
Στις 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
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
Στις 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.
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();
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(-)