On Fri, Dec 14, 2018 at 6:44 PM Vicente Bergas <vicencb@xxxxxxxxx> wrote:
>
> On Sun, Dec 9, 2018 at 10:45 PM Bhupesh Sharma <bhsharma@xxxxxxxxxx> wrote:
> >
> > Hello Akashi,
> >
> > Thanks for your review and sorry for my late reply.
> > It seems I found the root cause of the issue and it is quite different
> > from what I was thinking before.
> >
> > The issue happens because we pass '/chosen' string to
> > 'fdt_add_subnode()' in 'kexec/dt-ops.c' instead of 'chosen' string.
> >
> > The different strings being passed to 'fdt_add_subnode()' and
> > 'fdt_path_offset()' are 'chosen' and '/chosen' respectively for
> > adding/finding offset of the '/chosen' node in the kernel, see
> > 'drivers/firmware/efi/libstub/fdt.c' for details of such use-cases.
> >
> > When I fix the 'kexec-tools' to pass the same strings according to the
> > function being called, it fixes all the 'kexec -l' and 'kexec -p'
> > use-cases I could try on my boards, including the '--dtb' use cases.
> > So, I think this should be a workable solution.
> >
> > @Vicente: Can you please drop the earlier approaches I shared and just
> > try the hack'ish patch below. If this works for all the use-cases at
> > your side, I can try and send a formal version of this one out.
>
> Hi Bhupesh,
> I have tried several combinations of --append, --reuse-cmdline, --dtb
> and --ramdisk for the --load option: all of them worked fine.
> At the moment I do not have a proper setup to test the --exec option
> on that board without the chosen node in its dtb. I will report back
> when done.
> Thank you very much.
Thanks a lot for your quick reply. Glad that this patch makes things
better for you.
I will send a formal patch soon on the same lines.
Thanks,
Bhupesh
> > /----------------------/
> > From 7fb5e848cef268f6c3c61e001050cfd9202f5790 Mon Sep 17 00:00:00 2001
> > From: Bhupesh Sharma <bhsharma@xxxxxxxxxx>
> > Date: Sun, 2 Dec 2018 02:07:56 +0530
> > Subject: [PATCH] arm64: Fix fdt handling
> >
> > Signed-off-by: Bhupesh Sharma <bhsharma@xxxxxxxxxx>
> > ---
> > kexec/dt-ops.c | 20 +++++++++++---------
> > 1 file changed, 11 insertions(+), 9 deletions(-)
> >
> > diff --git a/kexec/dt-ops.c b/kexec/dt-ops.c
> > index 915dbf55afd2..33d9b14900a9 100644
> > --- a/kexec/dt-ops.c
> > +++ b/kexec/dt-ops.c
> > @@ -9,6 +9,7 @@
> > #include "dt-ops.h"
> >
> > static const char n_chosen[] = "/chosen";
> > +static const char chosen[] = "chosen";
> >
> > static const char p_bootargs[] = "bootargs";
> > static const char p_initrd_start[] = "linux,initrd-start";
> > @@ -26,7 +27,7 @@ int dtb_set_initrd(char **dtb, off_t *dtb_size,
> > off_t start, off_t end)
> >
> > value = cpu_to_fdt64(start);
> >
> > - result = dtb_set_property(dtb, dtb_size, n_chosen, p_initrd_start,
> > + result = dtb_set_property(dtb, dtb_size, chosen, p_initrd_start,
> > &value, sizeof(value));
> >
> > if (result)
> > @@ -34,11 +35,11 @@ int dtb_set_initrd(char **dtb, off_t *dtb_size,
> > off_t start, off_t end)
> >
> > value = cpu_to_fdt64(end);
> >
> > - result = dtb_set_property(dtb, dtb_size, n_chosen, p_initrd_end,
> > + result = dtb_set_property(dtb, dtb_size, chosen, p_initrd_end,
> > &value, sizeof(value));
> >
> > if (result) {
> > - dtb_delete_property(*dtb, n_chosen, p_initrd_start);
> > + dtb_delete_property(*dtb, chosen, p_initrd_start);
> > return result;
> > }
> >
> > @@ -47,7 +48,7 @@ int dtb_set_initrd(char **dtb, off_t *dtb_size,
> > off_t start, off_t end)
> >
> > int dtb_set_bootargs(char **dtb, off_t *dtb_size, const char *command_line)
> > {
> > - return dtb_set_property(dtb, dtb_size, n_chosen, p_bootargs,
> > + return dtb_set_property(dtb, dtb_size, chosen, p_bootargs,
> > command_line, strlen(command_line) + 1);
> > }
> >
> > @@ -79,16 +80,17 @@ int dtb_set_property(char **dtb, off_t *dtb_size,
> > const char *node,
> > goto on_error;
> > }
> >
> > - nodeoffset = fdt_path_offset(new_dtb, node);
> > -
> > + nodeoffset = fdt_path_offset(new_dtb, n_chosen);
> > +
> > if (nodeoffset == -FDT_ERR_NOTFOUND) {
> > - result = fdt_add_subnode(new_dtb, nodeoffset, node);
> > + result = fdt_add_subnode(new_dtb, 0, node);
> >
> > - if (result) {
> > + if (result < 0) {
> > dbgprintf("%s: fdt_add_subnode failed: %s\n", __func__,
> > fdt_strerror(result));
> > goto on_error;
> > }
> > + nodeoffset = result;
> > } else if (nodeoffset < 0) {
> > dbgprintf("%s: fdt_path_offset failed: %s\n", __func__,
> > fdt_strerror(nodeoffset));
> > @@ -127,7 +129,7 @@ on_error:
> > int dtb_delete_property(char *dtb, const char *node, const char *prop)
> > {
> > int result;
> > - int nodeoffset = fdt_path_offset(dtb, node);
> > + int nodeoffset = fdt_path_offset(dtb, n_chosen);
> >
> > if (nodeoffset < 0) {
> > dbgprintf("%s: fdt_path_offset failed: %s\n", __func__,
> > --
> > 2.7.4
> > On Tue, Dec 4, 2018 at 8:39 AM AKASHI Takahiro
> > <takahiro.akashi@xxxxxxxxxx> wrote:
> > >
> > > Bhupesh,
> > >
> > > Thank you for submitting a patch. One comment:
> > >
> > > On Fri, Nov 30, 2018 at 11:02:45AM +0530, Bhupesh Sharma wrote:
> > > > While creating the 2nd dtb to be passed to the secondary kernel
> > > > for arm64, we need to handle the return values from helper fdt
> > > > functions properly, to make sure that we can handle various command-line
> > > > options being passed to 'kexec' both for kexec load and kdump
> > > > purposes.
> > > >
> > > > This will provide proper error reporting to the calling
> > > > function in case something goes wrong.
> > > >
> > > > Without this patch, we get misleading error FDT_ERR_BADOFFSET reported
> > > > when we pass a .dtb to 'kexec -p' using '--dtb' option, which doesn't
> > > > contain the '/chosen' node (for e.g. the rockchip sapphire board
> > > > dtb -> rk3399-sapphire.dtb).
> > > >
> > > > # kexec -d -p Image --reuse-cmdline --dtb rk3399-sapphire.dtb
> > > > <..snip..>
> > > > load_crashdump_segments: elfcorehdr 0xbfff0000-0xbfff33ff
> > > > get_cells_size: #address-cells:2 #size-cells:2
> > > > cells_size_fitted: bfff0000-bfff33ff
> > > > cells_size_fitted: 8ee00000-bfffffff
> > > > setup_2nd_dtb: no kaslr-seed found
> > > > setup_2nd_dtb: fdt_setprop failed: FDT_ERR_BADOFFSET
> > > > kexec: setup_2nd_dtb failed.
> > > > kexec: load failed.
> > > >
> > > > Now after the fix, we get the correct error FDT_ERR_NOTFOUND reported
> > > > to the calling function:
> > > >
> > > > # kexec -d -l Image --append 'debug' --dtb rk3399-sapphire.dtb
> > > > <..snip..>
> > > > load_crashdump_segments: elfcorehdr 0xbfff0000-0xbfff33ff
> > > > get_cells_size: #address-cells:2 #size-cells:2
> > > > cells_size_fitted: bfff0000-bfff33ff
> > > > cells_size_fitted: 8ee00000-bfffffff
> > > > setup_2nd_dtb: /chosen node not found - can't create dtb for dump kernel: FDT_ERR_NOTFOUND
> > > > kexec: setup_2nd_dtb failed.
> > > > kexec: load failed.
> > > >
> > > >
> > > > Cc: Simon Horman <horms@xxxxxxxxxxxx>
> > > > Cc: AKASHI Takahiro <takahiro.akashi@xxxxxxxxxx>
> > > > Signed-off-by: Bhupesh Sharma <bhsharma@xxxxxxxxxx>
> > > > ---
> > > > kexec/arch/arm64/kexec-arm64.c | 21 +++++++++++++++++++++
> > > > 1 file changed, 21 insertions(+)
> > > >
> > > > diff --git a/kexec/arch/arm64/kexec-arm64.c b/kexec/arch/arm64/kexec-arm64.c
> > > > index f4913b2e9480..560d83455f95 100644
> > > > --- a/kexec/arch/arm64/kexec-arm64.c
> > > > +++ b/kexec/arch/arm64/kexec-arm64.c
> > > > @@ -515,6 +515,11 @@ static int setup_2nd_dtb(struct dtb *dtb, char *command_line, int on_crash)
> > > > }
> > > >
> > > > nodeoffset = fdt_path_offset(new_buf, "/chosen");
> > > > + if (nodeoffset < 0) {
> > > > + result = nodeoffset;
> > > > + goto on_error;
> > > > + }
> > > > +
> > >
> > > I think that the issue should be fixed much earlier:
> > >
> > > | /* fixup 'kaslr-seed' with a random value, if supported */
> > > | nodeoffset = fdt_path_offset(new_buf, "/chosen");
> > > | prop = fdt_getprop_w(new_buf, nodeoffset,
> > > | "kaslr-seed", &len);
> > >
> > > When we fail to locate "/chosen" here, we'd be better off to create it
> > > (if necessary). So, *logically*, we won't have to worry about it later on.
> > >
> > > -Takahiro Akashi
> > >
> > >
> > > > result = fdt_setprop_inplace(new_buf,
> > > > nodeoffset, "kaslr-seed",
> > > > &fdt_val64, sizeof(fdt_val64));
> > > > @@ -529,6 +534,14 @@ static int setup_2nd_dtb(struct dtb *dtb, char *command_line, int on_crash)
> > > > if (on_crash) {
> > > > /* add linux,elfcorehdr */
> > > > nodeoffset = fdt_path_offset(new_buf, "/chosen");
> > > > + if (nodeoffset < 0) {
> > > > + result = nodeoffset;
> > > > + dbgprintf("%s: /chosen node not found - can't create "
> > > > + "dtb for dump kernel: %s\n", __func__,
> > > > + fdt_strerror(result));
> > > > + goto on_error;
> > > > + }
> > > > +
> > > > result = fdt_setprop_range(new_buf, nodeoffset,
> > > > PROP_ELFCOREHDR, &elfcorehdr_mem,
> > > > address_cells, size_cells);
> > > > @@ -541,6 +554,14 @@ static int setup_2nd_dtb(struct dtb *dtb, char *command_line, int on_crash)
> > > >
> > > > /* add linux,usable-memory-range */
> > > > nodeoffset = fdt_path_offset(new_buf, "/chosen");
> > > > + if (nodeoffset < 0) {
> > > > + result = nodeoffset;
> > > > + dbgprintf("%s: /chosen node not found - can't create "
> > > > + "dtb for dump kernel: %s\n", __func__,
> > > > + fdt_strerror(result));
> > > > + goto on_error;
> > > > + }
> > > > +
> > > > result = fdt_setprop_range(new_buf, nodeoffset,
> > > > PROP_USABLE_MEM_RANGE, &crash_reserved_mem,
> > > > address_cells, size_cells);
> > > > --
> > > > 2.7.4
> > > >
_______________________________________________
kexec mailing list
kexec@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/kexec
