Re: [PATCH v19 05/13] of: Add a common kexec FDT setup function

[Rob Herring]

On Tue, Jun 15, 2021 at 8:23 PM Michael Ellerman <mpe@xxxxxxxxxxxxxx> wrote:
>
> Rob Herring <robh@xxxxxxxxxx> writes:
> > On Tue, Jun 15, 2021 at 10:13 AM nramas <nramas@xxxxxxxxxxxxxxxxxxx> wrote:
> >>
> >> On Tue, 2021-06-15 at 08:01 -0600, Rob Herring wrote:
> >> > On Tue, Jun 15, 2021 at 6:18 AM Geert Uytterhoeven <
> >> > geert@xxxxxxxxxxxxxx> wrote:
> >> > >
> >> > > > +void *of_kexec_alloc_and_setup_fdt(const struct kimage *image,
> >> > > > +                                  unsigned long
> >> > > > initrd_load_addr,
> >> > > > +                                  unsigned long initrd_len,
> >> > > > +                                  const char *cmdline, size_t
> >> > > > extra_fdt_size)
> >> > > > +{
> >> > > > +       /* Did we boot using an initrd? */
> >> > > > +       prop = fdt_getprop(fdt, chosen_node, "linux,initrd-
> >> > > > start", NULL);
> >> > > > +       if (prop) {
> >> > > > +               u64 tmp_start, tmp_end, tmp_size;
> >> > > > +
> >> > > > +               tmp_start = fdt64_to_cpu(*((const fdt64_t *)
> >> > > > prop));
> >> > > > +
> >> > > > +               prop = fdt_getprop(fdt, chosen_node,
> >> > > > "linux,initrd-end", NULL);
> >> > > > +               if (!prop) {
> >> > > > +                       ret = -EINVAL;
> >> > > > +                       goto out;
> >> > > > +               }
> >> > > > +
> >> > > > +               tmp_end = fdt64_to_cpu(*((const fdt64_t *)
> >> > > > prop));
> >> > >
> >> > > Some kernel code assumes "linux,initrd-{start,end}" are 64-bit,
> >> > > other code assumes 32-bit.
> >> >
> >> > It can be either. The above code was a merge of arm64 and powerpc >> > both
> >> > of which use 64-bit and still only runs on those arches. It looks >> > like
> >> > some powerpc platforms may use 32-bit, but this would have been >> > broken
> >> > before.
>
> >> of_kexec_alloc_and_setup_fdt() is called from elf_64.c (in
> >> arch/powerpc/kexec) which is for 64-bit powerpc platform only.
> >
> > 64-bit PPC could be writing 32-bit property values. The architecture
> > size doesn't necessarily matter. And if the values came from the
> > bootloader, who knows what size it used.
> >
> > This code is 32-bit powerpc only?:
> >
> > arch/powerpc/boot/main.c-       /* Tell the kernel initrd address via device tree */
> > arch/powerpc/boot/main.c:       setprop_val(chosen, "linux,initrd-start", (u32)(initrd_addr));
> > arch/powerpc/boot/main.c-       setprop_val(chosen, "linux,initrd-end", (u32)(initrd_addr+initrd_size));
>
> Historically that code was always built 32-bit, even when used with a
> 64-bit kernel.
>
> These days it is also built 64-bit (for ppc64le).

How it is built is immaterial. It's always writing a 32-bit value due
to the u32 cast.

> It looks like the drivers/of/fdt.c code can handle either 64 or 32-bit,
> so I guess that's why it seems to be working.

Yes, that works, but that's not the issue. The question is does the
main.c code run in combination with kexec. The kexec code above
(copied straight from PPC code) would not work if linux,initrd-* are
written by the main.c code.

Rob