Hi Vicenç,
Sorry I got busy and just got some spare time to look at this issue. I
am able to fix it partially, but I am still not completely happy with
it, but mainly here is the issue for the 'kexec load' option - we are
passing wrong 'nodeoffset' while calling 'fdt_add_subnode()', which
should be 0 instead of FDT_ERR_NOTFOUND when the chosen node is not
seen in the passed dtb :
>From 2433daf279744ae30fa9c74ec5b39507740d7163 Mon Sep 17 00:00:00 2001
From: Bhupesh Sharma <bhsharma@xxxxxxxxxx>
Date: Thu, 29 Nov 2018 00:41:06 +0530
Subject: [PATCH] Add some debug logs
Signed-off-by: Bhupesh Sharma <bhsharma@xxxxxxxxxx>
---
kexec/arch/arm64/kexec-arm64.c | 8 ++++++++
kexec/dt-ops.c | 3 ++-
kexec/libfdt/fdt_ro.c | 14 +++++++-------
kexec/libfdt/fdt_rw.c | 2 ++
4 files changed, 19 insertions(+), 8 deletions(-)
diff --git a/kexec/arch/arm64/kexec-arm64.c b/kexec/arch/arm64/kexec-arm64.c
index 6c7d2512b1d4..e29afc30db43 100644
--- a/kexec/arch/arm64/kexec-arm64.c
+++ b/kexec/arch/arm64/kexec-arm64.c
@@ -207,6 +207,9 @@ static int set_bootargs(struct dtb *dtb, const
char *command_line)
if (!command_line || !command_line[0])
return 0;
+ dbgprintf("%s: found %s\n", __func__, dtb->path);
+ dump_fdt(dtb->buf);
+
result = dtb_set_bootargs(&dtb->buf, &dtb->size, command_line);
if (result) {
@@ -413,6 +416,11 @@ static int setup_2nd_dtb(struct dtb *dtb, char
*command_line, int on_crash)
}
result = set_bootargs(dtb, command_line);
+ if (result) {
+ fprintf(stderr, "kexec: cannot set bootargs.\n");
+ result = -EINVAL;
+ goto on_error;
+ }
/* determine #address-cells and #size-cells */
result = get_cells_size(dtb->buf, &address_cells, &size_cells);
diff --git a/kexec/dt-ops.c b/kexec/dt-ops.c
index a0276f1e48b3..79b0404bafc4 100644
--- a/kexec/dt-ops.c
+++ b/kexec/dt-ops.c
@@ -86,13 +86,14 @@ int dtb_set_property(char **dtb, off_t *dtb_size,
const char *node,
nodeoffset = fdt_path_offset(new_dtb, node);
if (nodeoffset == -FDT_ERR_NOTFOUND) {
- result = fdt_add_subnode(new_dtb, nodeoffset, node);
+ result = fdt_add_subnode(new_dtb, 0, node);
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));
diff --git a/kexec/libfdt/fdt_ro.c b/kexec/libfdt/fdt_ro.c
index 129b532bcc1a..0dce53083c96 100644
--- a/kexec/libfdt/fdt_ro.c
+++ b/kexec/libfdt/fdt_ro.c
@@ -105,14 +105,14 @@ int fdt_subnode_offset_namelen(const void *fdt,
int offset,
FDT_CHECK_HEADER(fdt);
for (depth = 0;
- offset >= 0;
- offset = fdt_next_node(fdt, offset, &depth)) {
- if (depth < 0)
- return -FDT_ERR_NOTFOUND;
- else if ((depth == 1)
- && _fdt_nodename_eq(fdt, offset, name, namelen))
+ (offset >= 0) && (depth >= 0);
+ offset = fdt_next_node(fdt, offset, &depth))
+ if ((depth == 1)
+ && _fdt_nodename_eq(fdt, offset, name, namelen))
return offset;
- }
+
+ if (depth < 0)
+ return -FDT_ERR_NOTFOUND;
return offset; /* error */
}
diff --git a/kexec/libfdt/fdt_rw.c b/kexec/libfdt/fdt_rw.c
index 8e7ec4cb7bcd..2fac7c0731c0 100644
--- a/kexec/libfdt/fdt_rw.c
+++ b/kexec/libfdt/fdt_rw.c
@@ -101,6 +101,8 @@ static int _fdt_splice(void *fdt, void
*splicepoint, int oldlen, int newlen)
if (((p + oldlen) < p) || ((p + oldlen) > end))
return -FDT_ERR_BADOFFSET;
+ if ((p < (char *)fdt) || ((end - oldlen + newlen) < (char *)fdt))
+ return -FDT_ERR_BADOFFSET;
if ((end - oldlen + newlen) > ((char *)fdt + fdt_totalsize(fdt)))
return -FDT_ERR_NOSPACE;
memmove(p + newlen, p + oldlen, end - p - oldlen);
--
2.7.4
Here are some logs at my end after this fix (I used some earlier work
I did for dump'ing dtb on arm64 boards to get the dtb logs:
<https://www.spinics.net/lists/kexec/msg20951.html>):
# kexec -d -l /root/Image --append 'debug' --dtb /root/rk3399-sapphire.dtb
<..snip..>
image_arm64_load: PE format: yes
/ {
compatible = "rockchip,rk3399-sapphire";
interrupt-parent = <0x00000001>;
#address-cells = <0x00000002>;
#size-cells = <0x00000002>;
model = "Sapphire-RK3399 Board";
/chosen {
bootargs = "debug";
};
aliases {
<..snip..>
};
get_cells_size: #address-cells:2 #size-cells:2
cells_size_fitted: 0-0
cells_size_fitted: 0-0
setup_2nd_dtb: no kaslr-seed found
dtb: base 81ef0000, size d0a1h (53409)
sym: sha256_starts info: 12 other: 00 shndx: 1 value: ec0 size: 6c
sym: sha256_starts value: 81f00ec0 addr: 81f00018
machine_apply_elf_rel: CALL26 580006b394000000->580006b3940003aa
sym: sha256_update info: 12 other: 00 shndx: 1 value: 5168 size: c
sym: sha256_update value: 81f05168 addr: 81f00034
machine_apply_elf_rel: CALL26 9100427394000000->910042739400144d
sym: sha256_finish info: 12 other: 00 shndx: 1 value: 5174 size: 1cc
sym: sha256_finish value: 81f05174 addr: 81f00050
machine_apply_elf_rel: CALL26 aa1403e094000000->aa1403e094001449
sym: memcmp info: 12 other: 00 shndx: 1 value: 64c size: 34
sym: memcmp value: 81f0064c addr: 81f00060
machine_apply_elf_rel: CALL26 340003c094000000->340003c09400017b
sym: printf info: 12 other: 00 shndx: 1 value: 55c size: 90
sym: printf value: 81f0055c addr: 81f00070
machine_apply_elf_rel: CALL26 5800046094000000->580004609400013b
sym: printf info: 12 other: 00 shndx: 1 value: 55c size: 90
sym: printf value: 81f0055c addr: 81f00078
machine_apply_elf_rel: CALL26 5800047594000000->5800047594000139
sym: printf info: 12 other: 00 shndx: 1 value: 55c size: 90
sym: printf value: 81f0055c addr: 81f00088
machine_apply_elf_rel: CALL26 9100067394000000->9100067394000135
sym: printf info: 12 other: 00 shndx: 1 value: 55c size: 90
sym: printf value: 81f0055c addr: 81f000a8
machine_apply_elf_rel: CALL26 5800036094000000->580003609400012d
sym: printf info: 12 other: 00 shndx: 1 value: 55c size: 90
sym: printf value: 81f0055c addr: 81f000b0
machine_apply_elf_rel: CALL26 910402e194000000->910402e19400012b
sym: printf info: 12 other: 00 shndx: 1 value: 55c size: 90
sym: printf value: 81f0055c addr: 81f000c0
machine_apply_elf_rel: CALL26 9100067394000000->9100067394000127
sym: printf info: 12 other: 00 shndx: 1 value: 55c size: 90
sym: printf value: 81f0055c addr: 81f000d4
machine_apply_elf_rel: CALL26 5280002094000000->5280002094000122
sym: .data info: 03 other: 00 shndx: 4 value: 0 size: 0
sym: .data value: 81f053b8 addr: 81f000f0
machine_apply_elf_rel: ABS64 0000000000000000->0000000081f053b8
sym: .rodata.str1.1 info: 03 other: 00 shndx: 3 value: 0 size: 0
sym: .rodata.str1.1 value: 81f05348 addr: 81f000f8
machine_apply_elf_rel: ABS64 0000000000000000->0000000081f05348
sym: .rodata.str1.1 info: 03 other: 00 shndx: 3 value: 0 size: 0
sym: .rodata.str1.1 value: 81f05368 addr: 81f00100
machine_apply_elf_rel: ABS64 0000000000000000->0000000081f05368
sym: .rodata.str1.1 info: 03 other: 00 shndx: 3 value: 0 size: 0
sym: .rodata.str1.1 value: 81f05378 addr: 81f00108
machine_apply_elf_rel: ABS64 0000000000000000->0000000081f05378
sym: .rodata.str1.1 info: 03 other: 00 shndx: 3 value: 0 size: 0
sym: .rodata.str1.1 value: 81f0537e addr: 81f00110
machine_apply_elf_rel: ABS64 0000000000000000->0000000081f0537e
sym: .rodata.str1.1 info: 03 other: 00 shndx: 3 value: 0 size: 0
sym: .rodata.str1.1 value: 81f05380 addr: 81f00118
machine_apply_elf_rel: ABS64 0000000000000000->0000000081f05380
sym: printf info: 12 other: 00 shndx: 1 value: 55c size: 90
sym: printf value: 81f0055c addr: 81f0012c
machine_apply_elf_rel: CALL26 9400000094000000->940000009400010c
sym: setup_arch info: 12 other: 00 shndx: 1 value: eb8 size: 4
sym: setup_arch value: 81f00eb8 addr: 81f00130
machine_apply_elf_rel: CALL26 5800016094000000->5800016094000362
sym: verify_sha256_digest info: 12 other: 00 shndx: 1 value: 0 size: f0
sym: verify_sha256_digest value: 81f00000 addr: 81f00140
machine_apply_elf_rel: CALL26 3400004094000000->3400004097ffffb0
sym: post_verification_setup_arch info: 12 other: 00 shndx: 1 value: eb4 size: 4
sym: post_verification_setup_arch value: 81f00eb4 addr: 81f00150
machine_apply_elf_rel: JUMP26 d503201f14000000->d503201f14000359
sym: .rodata.str1.1 info: 03 other: 00 shndx: 3 value: 0 size: 0
sym: .rodata.str1.1 value: 81f05390 addr: 81f00158
machine_apply_elf_rel: ABS64 0000000000000000->0000000081f05390
sym: .data info: 03 other: 00 shndx: 4 value: 0 size: 0
sym: .data value: 81f053b8 addr: 81f00160
machine_apply_elf_rel: ABS64 0000000000000000->0000000081f053b8
sym: putchar info: 12 other: 00 shndx: 1 value: eb0 size: 4
sym: putchar value: 81f00eb0 addr: 81f001c4
machine_apply_elf_rel: CALL26 f94037a194000000->f94037a19400033b
sym: putchar info: 12 other: 00 shndx: 1 value: eb0 size: 4
sym: putchar value: 81f00eb0 addr: 81f00238
machine_apply_elf_rel: CALL26 910006f794000000->910006f79400031e
sym: putchar info: 12 other: 00 shndx: 1 value: eb0 size: 4
sym: putchar value: 81f00eb0 addr: 81f00490
machine_apply_elf_rel: CALL26 9100073994000000->9100073994000288
sym: .rodata.str1.1 info: 03 other: 00 shndx: 3 value: 0 size: 0
sym: .rodata.str1.1 value: 81f053a2 addr: 81f004d0
machine_apply_elf_rel: ABS64 0000000000000000->0000000081f053a2
sym: vsprintf info: 12 other: 00 shndx: 1 value: 168 size: 364
sym: vsprintf value: 81f00168 addr: 81f00550
machine_apply_elf_rel: CALL26 a8d07bfd94000000->a8d07bfd97ffff06
sym: vsprintf info: 12 other: 00 shndx: 1 value: 168 size: 364
sym: vsprintf value: 81f00168 addr: 81f005e0
machine_apply_elf_rel: CALL26 a8d17bfd94000000->a8d17bfd97fffee2
sym: purgatory info: 12 other: 00 shndx: 1 value: 120 size: 34
sym: purgatory value: 81f00120 addr: 81f00688
machine_apply_elf_rel: CALL26 5800001194000000->5800001197fffea6
sym: arm64_kernel_entry info: 10 other: 00 shndx: 4 value: 128 size: 8
sym: arm64_kernel_entry value: 81f054e0 addr: 81f0068c
machine_apply_elf_rel: LD_PREL_LO19 5800000058000011->58000000580272b1
sym: arm64_dtb_addr info: 10 other: 00 shndx: 4 value: 130 size: 8
sym: arm64_dtb_addr value: 81f054e8 addr: 81f00690
machine_apply_elf_rel: LD_PREL_LO19 aa1f03e158000000->aa1f03e1580272c0
sym: sha256_process info: 12 other: 00 shndx: 1 value: f2c size: 4134
sym: sha256_process value: 81f00f2c addr: 81f050cc
machine_apply_elf_rel: CALL26 d101029494000000->d101029497ffef98
sym: memcpy info: 12 other: 00 shndx: 1 value: 62c size: 20
sym: memcpy value: 81f0062c addr: 81f05128
machine_apply_elf_rel: JUMP26 b4fffc5814000000->b4fffc5817ffed41
sym: memcpy info: 12 other: 00 shndx: 1 value: 62c size: 20
sym: memcpy value: 81f0062c addr: 81f05140
machine_apply_elf_rel: CALL26 aa1503e094000000->aa1503e097ffed3b
sym: sha256_process info: 12 other: 00 shndx: 1 value: f2c size: 4134
sym: sha256_process value: 81f00f2c addr: 81f0514c
machine_apply_elf_rel: CALL26 cb1302d694000000->cb1302d697ffef78
sym: .data info: 03 other: 00 shndx: 4 value: 0 size: 0
sym: .data value: 81f054f0 addr: 81f05340
machine_apply_elf_rel: ABS64 0000000000000000->0000000081f054f0
kexec_load: entry = 0x81f00680 flags = 0xb70000
nr_segments = 3
segment[0].buf = 0xffff8bab0010
segment[0].bufsz = 0x1999a00
segment[0].mem = 0x80480000
segment[0].memsz = 0x1a70000
segment[1].buf = 0x16984e10
segment[1].bufsz = 0xd0a1
segment[1].mem = 0x81ef0000
segment[1].memsz = 0x10000
segment[2].buf = 0x16992230
segment[2].bufsz = 0x5530
segment[2].mem = 0x81f00000
segment[2].memsz = 0x10000
I will try to stabilize this more both for 'kexec load' and 'kexec
dump' use-cases and try to send a patch soon to fix this issue.
Regards,
Bhupesh
On Fri, Nov 16, 2018 at 8:59 PM Vicente Bergas <vicencb@xxxxxxxxx> wrote:
>
> On Fri, Nov 16, 2018 at 1:04 PM Bhupesh Sharma <bhsharma@xxxxxxxxxx> wrote:
> >
> > Hi Vicenç,
> >
> > I tried the patch at my end and it works fine as per my checks.
> > So, can I request you to provide some additional data which will help
> > me understand your environment better. See inline:
> >
> > On Fri, Nov 16, 2018 at 12:08 AM Vicente Bergas <vicencb@xxxxxxxxx> wrote:
> > >
> > > On Thu, Nov 15, 2018 at 3:14 PM Simon Horman <horms@xxxxxxxxxxxx> wrote:
> > > >
> > > > On Thu, Nov 15, 2018 at 06:10:36AM -0800, Simon Horman wrote:
> > > > > On Mon, Nov 12, 2018 at 11:24:07AM +0900, AKASHI Takahiro wrote:
> > > > > > On Mon, Nov 12, 2018 at 02:00:16AM +0530, Bhupesh Sharma wrote:
> > > > > > > Vicenç reported (via [1]) that currently executing kexec with
> > > > > > > '--dtb' option and passing a .dtb which doesn't have a '/chosen' node
> > > > > > > leads to the following error:
> > > > > > >
> > > > > > > # kexec -d --dtb dtb_without_chosen_node.dtb --append 'cmdline' --load Image
> > > > > > >
> > > > > > > dtb_set_property: fdt_add_subnode failed: <valid offset/length>
> > > > > > > kexec: Set device tree bootargs failed.
> > > > > > >
> > > > > > > This happens because currently we check the return value of
> > > > > > > 'fdt_add_subnode()' function call in 'dt-ops.c' incorrectly:
> > > > > > >
> > > > > > > result = fdt_add_subnode(new_dtb, nodeoffset, node);
> > > > > > > if (result) {
> > > > > > > dbgprintf("%s: fdt_add_subnode failed: %s\n", _func__,
> > > > > > > fdt_strerror(result));
> > > > > > > goto on_error;
> > > > > > > }
> > > > > > >
> > > > > > > As we can see in 'fdt_rw.c', a positive return value from
> > > > > > > 'fdt_add_subnode()' function doesn't indicate an error.
> > > > > > >
> > > > > > > We can see that the Linux kernel (see 'drivers/firmware/efi/libstub/fdt.c'
> > > > > > > for example) also checks the 'fdt_add_subnode()' function against negative
> > > > > > > return values for errors. See an example below from 'update_fdt()' function in
> > > > > > > 'drivers/firmware/efi/libstub/fdt.c':
> > > > > > >
> > > > > > > node = fdt_add_subnode(fdt, 0, "chosen");
> > > > > > > if (node < 0) {
> > > > > > > status = node;
> > > > > > > <..snip..>
> > > > > > > goto fdt_set_fail;
> > > > > > > }
> > > > > > >
> > > > > > > This patch fixes the same in 'kexec-tools'.
> > > > > >
> > > > > > Looks good.
> > > > > > Thank you.
> > > > > > -Takahiro Akashi
> > > > >
> > > > > Thanks, applied.
> > > >
> > > > Perhaps I was a little too hasty there, I have dropped the patch for now.
> > > >
> > > > Vicente, does this patch work for you?
> > >
> > > The patch looks fine, but the end result is still non-working:
> > > # kexec -d --load Image --append 'debug' --dtb no_chosen.dtb
> >
> > Can you share the .dts with which you see this issue and steps you use
> > to create the .dtb file at your end?
> >
> > I normally use at my end the following command:
> > dtc -o no-chosen.dtb -I dts -O dtb no-chosen.dts
> >
> > Thanks,
> > Bhupesh
>
> Hi Bhupesh,
> the issue is not with dtc, it is with kexec. The following script
> reproduces the issue:
> #!/bin/sh -ve
>
> uname -m
> # aarch64
>
> git clone 'https://git.kernel.org/pub/scm/utils/kernel/kexec/kexec-tools.git'
> kexec-tools-orig
> cp -a kexec-tools-orig kexec-tools-test
> git -C kexec-tools-orig checkout -b orig 'v2.0.18'
> git -C kexec-tools-test checkout -b test 'v2.0.18'
>
> cat patch.diff
> #--- a/kexec/dt-ops.c
> #+++ b/kexec/dt-ops.c
> #@@ -84,7 +84,7 @@ int dtb_set_property(char **dtb, off_t *dtb_size,
> const char *node,
> # if (nodeoffset == -FDT_ERR_NOTFOUND) {
> # result = fdt_add_subnode(new_dtb, nodeoffset, node);
> #
> #- if (result) {
> #+ if (result < 0) {
> # dbgprintf("%s: fdt_add_subnode failed: %s\n", __func__,
> # fdt_strerror(result));
> # goto on_error;
> patch -d kexec-tools-test -p1 < patch.diff
>
> ( cd kexec-tools-orig ; ./bootstrap )
> ( cd kexec-tools-test ; ./bootstrap )
> ( cd kexec-tools-orig ; ./configure )
> ( cd kexec-tools-test ; ./configure )
> make -C kexec-tools-orig
> make -C kexec-tools-test
>
> wget 'http://mirror.archlinuxarm.org/aarch64/core/linux-aarch64-4.19.2-1-aarch64.pkg.tar.xz'
> bsdtar xf 'linux-aarch64-4.19.2-1-aarch64.pkg.tar.xz'
>
> sudo kexec-tools-orig/build/sbin/kexec -d --load boot/Image --append
> 'debug' --dtb boot/dtbs/rockchip/rk3399-sapphire.dtb
> # dtb_set_property: fdt_add_subnode failed: <valid offset/length>
> # kexec: Set device tree bootargs failed.
>
> sudo kexec-tools-test/build/sbin/kexec -d --load boot/Image --append
> 'debug' --dtb boot/dtbs/rockchip/rk3399-sapphire.dtb
> # dtb_set_property: fdt_setprop failed: FDT_ERR_BADOFFSET
> # kexec: Set device tree bootargs failed.
>
> Regards,
> Vicenç.
>
> > > ...
> > > dtb_set_property: fdt_setprop failed: FDT_ERR_BADOFFSET
> > > kexec: Set device tree bootargs failed.
> > >
> > > Also tested this with the same result:
> > > --- a/kexec/dt-ops.c
> > > +++ b/kexec/dt-ops.c
> > > @@ -84,11 +84,13 @@
> > > if (nodeoffset == -FDT_ERR_NOTFOUND) {
> > > result = fdt_add_subnode(new_dtb, nodeoffset, 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));
> > >
> > > Regards,
> > > Vicenç.
_______________________________________________
kexec mailing list
kexec@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/kexec
