Re: [PATCH v4 5/5] RISC-V: Add crash kernel support

Re: [PATCH v4 5/5] RISC-V: Add crash kernel support

[Geert Uytterhoeven]

Hi Nick,

CC rob, dt

Thanks for your patch, which is now commit 5640975003d0234d ("RISC-V:
Add crash kernel support") in v5.13-rc1.

On Mon, Apr 19, 2021 at 2:56 AM Nick Kossifidis <mick@ics.forth.gr> wrote:
> This patch allows Linux to act as a crash kernel for use with
> kdump. Userspace will let the crash kernel know about the
> memory region it can use through linux,usable-memory property
> on the /memory node (overriding its reg property), and about the
> memory region where the elf core header of the previous kernel
> is saved, through a reserved-memory node with a compatible string
> of "linux,elfcorehdr". This approach is the least invasive and
> re-uses functionality already present.

This does not match
https://github.com/devicetree-org/dt-schema/blob/master/schemas/chosen.yaml#L77:

    $ref: types.yaml#/definitions/uint64-array
    maxItems: 2
    description:
      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.

Hence "linux,elfcorehdr" should be a property of the /chosen node,
instead of a memory node with a compatible value of "linux,elfcorehdr".

> I tested this on riscv64 qemu and it works as expected, you
> may test it by retrieving the dmesg of the previous kernel
> through /proc/vmcore, using the vmcore-dmesg utility from
> kexec-tools.
>
> v4:
>  * Rebase on top of "fixes" branch
>
> v3:
>  * Rebase
>
> v2:
>  * Use linux,usable-memory on /memory instead of a new binding

This part seems to have been removed in v3 and later?
Note that "linux,usable-memory-range" should be a property of the
/chosen node, too, cfr.
https://github.com/devicetree-org/dt-schema/blob/master/schemas/chosen.yaml#L85

>  * Use a reserved-memory node for ELF core header
>
> Signed-off-by: Nick Kossifidis <mick@ics.forth.gr>


> --- a/arch/riscv/mm/init.c
> +++ b/arch/riscv/mm/init.c
> @@ -653,6 +666,26 @@ 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 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_vm_final();
> --
> 2.26.2

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 v4 5/5] RISC-V: Add crash kernel support

[Nick Kossifidis]

Hello Geert,

Στις 2021-06-15 16:19, Geert Uytterhoeven έγραψε:
> 
> This does not match
> https://github.com/devicetree-org/dt-schema/blob/master/schemas/chosen.yaml#L77:
> 
>     $ref: types.yaml#/definitions/uint64-array
>     maxItems: 2
>     description:
>       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.
> 
> Hence "linux,elfcorehdr" should be a property of the /chosen node,
> instead of a memory node with a compatible value of "linux,elfcorehdr".
> 

That's a binding for a property on the /chosen node, that as the text 
says it's defined for arm64 only and the code that handled it was also 
on arm64. Instead the reserved-region binding I used is a standard 
binding, if you don't like the name used for the compatible string 
because it overlaps with that property we can change it. I want to use a 
reserved-region for this because we'll have to reserve it anyway so 
using a property on /chosen and then using that property to reserve the 
region seemed suboptimal.

>> v2:
>>  * Use linux,usable-memory on /memory instead of a new binding
> 
> This part seems to have been removed in v3 and later?
> Note that "linux,usable-memory-range" should be a property of the
> /chosen node, too, cfr.
> https://github.com/devicetree-org/dt-schema/blob/master/schemas/chosen.yaml#L85
> 

No special handling is needed when using linux,usable-memory on /memory, 
limiting the available memory is handled by generic code at 
drivers/of/fdt.c

Regards,
Nick

Re: [PATCH v4 5/5] RISC-V: Add crash kernel support

[Geert Uytterhoeven]

Hi Nick,

On Tue, Jun 15, 2021 at 8:29 PM Nick Kossifidis <mick@ics.forth.gr> wrote:
> Στις 2021-06-15 16:19, Geert Uytterhoeven έγραψε:
> > This does not match
> > https://github.com/devicetree-org/dt-schema/blob/master/schemas/chosen.yaml#L77:
> >
> >     $ref: types.yaml#/definitions/uint64-array
> >     maxItems: 2
> >     description:
> >       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.
> >
> > Hence "linux,elfcorehdr" should be a property of the /chosen node,
> > instead of a memory node with a compatible value of "linux,elfcorehdr".
> >
>
> That's a binding for a property on the /chosen node, that as the text
> says it's defined for arm64 only and the code that handled it was also

That doesn't mean it must not be used on other architectures ;-)
Arm64 was just the first one to use it...

> on arm64. Instead the reserved-region binding I used is a standard
> binding, if you don't like the name used for the compatible string
> because it overlaps with that property we can change it. I want to use a
> reserved-region for this because we'll have to reserve it anyway so
> using a property on /chosen and then using that property to reserve the
> region seemed suboptimal.
>
> >> v2:
> >>  * Use linux,usable-memory on /memory instead of a new binding
> >
> > This part seems to have been removed in v3 and later?
> > Note that "linux,usable-memory-range" should be a property of the
> > /chosen node, too, cfr.
> > https://github.com/devicetree-org/dt-schema/blob/master/schemas/chosen.yaml#L85
> >
>
> No special handling is needed when using linux,usable-memory on /memory,
> limiting the available memory is handled by generic code at
> drivers/of/fdt.c

It was my understanding both properties under /chosen are the
recommended methods for new platforms... Let's see what Rob has
to say...

Anyway, I sent a patch series to switch to generic "linux,elfcorehdr"
handling
https://lore.kernel.org/r/cover.1623780059.git.geert+renesas@glider.be/

Thanks!

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 v4 5/5] RISC-V: Add crash kernel support

[Rob Herring]

On Tue, Jun 15, 2021 at 12:48 PM Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
>
> Hi Nick,
>
> On Tue, Jun 15, 2021 at 8:29 PM Nick Kossifidis <mick@ics.forth.gr> wrote:
> > Στις 2021-06-15 16:19, Geert Uytterhoeven έγραψε:
> > > This does not match
> > > https://github.com/devicetree-org/dt-schema/blob/master/schemas/chosen.yaml#L77:
> > >
> > >     $ref: types.yaml#/definitions/uint64-array
> > >     maxItems: 2
> > >     description:
> > >       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.
> > >
> > > Hence "linux,elfcorehdr" should be a property of the /chosen node,
> > > instead of a memory node with a compatible value of "linux,elfcorehdr".
> > >
> >
> > That's a binding for a property on the /chosen node, that as the text
> > says it's defined for arm64 only and the code that handled it was also
>
> That doesn't mean it must not be used on other architectures ;-)
> Arm64 was just the first one to use it...

It is used on arm64 because memory is often passed by UEFI tables and
not with /memory node. As riscv is also supporting EFI, I'd think they
would do the same.

> > on arm64. Instead the reserved-region binding I used is a standard
> > binding, if you don't like the name used for the compatible string
> > because it overlaps with that property we can change it. I want to use a
> > reserved-region for this because we'll have to reserve it anyway so
> > using a property on /chosen and then using that property to reserve the
> > region seemed suboptimal.
> >
> > >> v2:
> > >>  * Use linux,usable-memory on /memory instead of a new binding
> > >
> > > This part seems to have been removed in v3 and later?
> > > Note that "linux,usable-memory-range" should be a property of the
> > > /chosen node, too, cfr.
> > > https://github.com/devicetree-org/dt-schema/blob/master/schemas/chosen.yaml#L85
> > >
> >
> > No special handling is needed when using linux,usable-memory on /memory,
> > limiting the available memory is handled by generic code at
> > drivers/of/fdt.c
>
> It was my understanding both properties under /chosen are the
> recommended methods for new platforms... Let's see what Rob has
> to say...
>
> Anyway, I sent a patch series to switch to generic "linux,elfcorehdr"
> handling
> https://lore.kernel.org/r/cover.1623780059.git.geert+renesas@glider.be/
>
> Thanks!
>
> 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 v4 5/5] RISC-V: Add crash kernel support

[Nick Kossifidis]

Στις 2021-06-15 22:21, Rob Herring έγραψε:
> On Tue, Jun 15, 2021 at 12:48 PM Geert Uytterhoeven
> <geert@linux-m68k.org> wrote:
>> 
>> Hi Nick,
>> 
>> On Tue, Jun 15, 2021 at 8:29 PM Nick Kossifidis <mick@ics.forth.gr> 
>> wrote:
>> > Στις 2021-06-15 16:19, Geert Uytterhoeven έγραψε:
>> > > This does not match
>> > > https://github.com/devicetree-org/dt-schema/blob/master/schemas/chosen.yaml#L77:
>> > >
>> > >     $ref: types.yaml#/definitions/uint64-array
>> > >     maxItems: 2
>> > >     description:
>> > >       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.
>> > >
>> > > Hence "linux,elfcorehdr" should be a property of the /chosen node,
>> > > instead of a memory node with a compatible value of "linux,elfcorehdr".
>> > >
>> >
>> > That's a binding for a property on the /chosen node, that as the text
>> > says it's defined for arm64 only and the code that handled it was also
>> 
>> That doesn't mean it must not be used on other architectures ;-)
>> Arm64 was just the first one to use it...
> 
> It is used on arm64 because memory is often passed by UEFI tables and
> not with /memory node. As riscv is also supporting EFI, I'd think they
> would do the same.
> 

We've had this discussion before, riscv uses /memory for now and even if 
we switched to getting memory from ACPI/UEFI tables, the elf core header 
is passed from the crashed kernel to the kdump kernel, it has nothing to 
do with UEFI since the bootloader is the kernel itself. Am I missing 
something ?

Regards,
Nick

Re: [PATCH v4 5/5] RISC-V: Add crash kernel support

[Rob Herring]

+Ard

On Tue, Jun 15, 2021 at 5:29 PM Nick Kossifidis <mick@ics.forth.gr> wrote:
>
> Στις 2021-06-15 22:21, Rob Herring έγραψε:
> > On Tue, Jun 15, 2021 at 12:48 PM Geert Uytterhoeven
> > <geert@linux-m68k.org> wrote:
> >>
> >> Hi Nick,
> >>
> >> On Tue, Jun 15, 2021 at 8:29 PM Nick Kossifidis <mick@ics.forth.gr>
> >> wrote:
> >> > Στις 2021-06-15 16:19, Geert Uytterhoeven έγραψε:
> >> > > This does not match
> >> > > https://github.com/devicetree-org/dt-schema/blob/master/schemas/chosen.yaml#L77:
> >> > >
> >> > >     $ref: types.yaml#/definitions/uint64-array
> >> > >     maxItems: 2
> >> > >     description:
> >> > >       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.
> >> > >
> >> > > Hence "linux,elfcorehdr" should be a property of the /chosen node,
> >> > > instead of a memory node with a compatible value of "linux,elfcorehdr".
> >> > >
> >> >
> >> > That's a binding for a property on the /chosen node, that as the text
> >> > says it's defined for arm64 only and the code that handled it was also
> >>
> >> That doesn't mean it must not be used on other architectures ;-)
> >> Arm64 was just the first one to use it...
> >
> > It is used on arm64 because memory is often passed by UEFI tables and
> > not with /memory node. As riscv is also supporting EFI, I'd think they
> > would do the same.
> >
>
> We've had this discussion before, riscv uses /memory for now and even if
> we switched to getting memory from ACPI/UEFI tables, the elf core header
> is passed from the crashed kernel to the kdump kernel, it has nothing to
> do with UEFI since the bootloader is the kernel itself. Am I missing
> something ?

I believe if we originally booted using UEFI tables, then those are
passed the kdump kernel as well. The original DT may have had a
/memory node, but it's possible it didn't match what was in the UEFI
tables. So using the DT /memory nodes for kdump could give surprising
results. I think reserved regions also come from UEFI. Ard can
probably comment better.

Rob

Re: [PATCH v4 5/5] RISC-V: Add crash kernel support

[Ard Biesheuvel]

On Wed, 16 Jun 2021 at 16:55, Rob Herring <robh+dt@kernel.org> wrote:
>
> +Ard
>
> On Tue, Jun 15, 2021 at 5:29 PM Nick Kossifidis <mick@ics.forth.gr> wrote:
> >
> > Στις 2021-06-15 22:21, Rob Herring έγραψε:
> > > On Tue, Jun 15, 2021 at 12:48 PM Geert Uytterhoeven
> > > <geert@linux-m68k.org> wrote:
> > >>
> > >> Hi Nick,
> > >>
> > >> On Tue, Jun 15, 2021 at 8:29 PM Nick Kossifidis <mick@ics.forth.gr>
> > >> wrote:
> > >> > Στις 2021-06-15 16:19, Geert Uytterhoeven έγραψε:
> > >> > > This does not match
> > >> > > https://github.com/devicetree-org/dt-schema/blob/master/schemas/chosen.yaml#L77:
> > >> > >
> > >> > >     $ref: types.yaml#/definitions/uint64-array
> > >> > >     maxItems: 2
> > >> > >     description:
> > >> > >       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.
> > >> > >
> > >> > > Hence "linux,elfcorehdr" should be a property of the /chosen node,
> > >> > > instead of a memory node with a compatible value of "linux,elfcorehdr".
> > >> > >
> > >> >
> > >> > That's a binding for a property on the /chosen node, that as the text
> > >> > says it's defined for arm64 only and the code that handled it was also
> > >>
> > >> That doesn't mean it must not be used on other architectures ;-)
> > >> Arm64 was just the first one to use it...
> > >
> > > It is used on arm64 because memory is often passed by UEFI tables and
> > > not with /memory node. As riscv is also supporting EFI, I'd think they
> > > would do the same.
> > >
> >
> > We've had this discussion before, riscv uses /memory for now and even if
> > we switched to getting memory from ACPI/UEFI tables, the elf core header
> > is passed from the crashed kernel to the kdump kernel, it has nothing to
> > do with UEFI since the bootloader is the kernel itself. Am I missing
> > something ?
>
> I believe if we originally booted using UEFI tables, then those are
> passed the kdump kernel as well. The original DT may have had a
> /memory node, but it's possible it didn't match what was in the UEFI
> tables. So using the DT /memory nodes for kdump could give surprising
> results. I think reserved regions also come from UEFI. Ard can
> probably comment better.
>

Anything that executes in the context of the UEFI boot firmware
(loaders, drivers, etc) may use the UEFI memory allocation routines to
allocate memory, and these allocations are communicated via the UEFI
memory map, not via the /memory node.

So it depends whether it matters if the kexec kernel tramples over
those regions. For kdump scenarios, it might be reasonable, but in the
general case, we should really respect what UEFI tells us about the
memory map when booting via UEFI.