[PATCH] kexec:arm: support zImage with appended device tree

Post by Hoeun Ryu
Arm linux supports zImage with appended dtb (CONFIG_ARM_APPENDED_DTB) and
the concatenated image is generated like `cat zImage dtb > zImage_w_dtb`.
This patch is to support the concatednated zImage. This changes the
priority of source of dtb file.
1. --dtb dtb_file
2. zImage_w_dtb <= newly added
3. /sys/firmware/fdt
4. /proc/device-tree

The original default fdt is /sys/firmware/fdt, after your changes it
becomes zImage attached dtb, presumably one knows his zImage appended a
dtb it is fine, but if one does not notice it, it changes the behavior
he expects.

How about add a new option for zImage_dtb, like --zImage-dtb

Post by Hoeun Ryu
Users don't need to specify dtb file in the command line and don't have to
have /sys/firmware/fdt or /proc/device-tree if dtb is available in zImage.
---
kexec/arch/arm/kexec-arm.h | 1 +
kexec/arch/arm/kexec-zImage-arm.c | 47 ++++++++++++++++++++++++++++++---------
2 files changed, 37 insertions(+), 11 deletions(-)
diff --git a/kexec/arch/arm/kexec-arm.h b/kexec/arch/arm/kexec-arm.h
index a74cce2..94695b7 100644
--- a/kexec/arch/arm/kexec-arm.h
+++ b/kexec/arch/arm/kexec-arm.h
@@ -4,6 +4,7 @@
#include <sys/types.h>
#define SYSFS_FDT "/sys/firmware/fdt"
+#define APPENDED_FDT ((char *)-1) /* it doesn't mean to be opened. */
#define BOOT_BLOCK_VERSION 17
#define BOOT_BLOCK_LAST_COMP_VERSION 16
diff --git a/kexec/arch/arm/kexec-zImage-arm.c b/kexec/arch/arm/kexec-zImage-arm.c
index 7f02b93..59f0003 100644
--- a/kexec/arch/arm/kexec-zImage-arm.c
+++ b/kexec/arch/arm/kexec-zImage-arm.c
@@ -390,6 +390,7 @@ int zImage_arm_load(int argc, char **argv, const char *buf, off_t len,
initrd_size = 0;
use_atags = 0;
dtb_file = NULL;
+ dtb_buf = NULL;
while((opt = getopt_long(argc, argv, short_options, options, 0)) != -1) {
switch(opt) {
@@ -424,14 +425,6 @@ int zImage_arm_load(int argc, char **argv, const char *buf, off_t len,
return -1;
}
- if (!use_atags && !dtb_file) {
- int f;
-
- f = have_sysfs_fdt();
- if (f)
- dtb_file = SYSFS_FDT;
- }
-
if (command_line) {
command_line_len = strlen(command_line) + 1;
if (command_line_len > COMMAND_LINE_SIZE)
@@ -440,9 +433,6 @@ int zImage_arm_load(int argc, char **argv, const char *buf, off_t len,
if (ramdisk)
ramdisk_buf = slurp_file(ramdisk, &initrd_size);
- if (dtb_file)
- dtb_buf = slurp_file(dtb_file, &dtb_length);
-
if (len > 0x34) {
const struct zimage_header *hdr;
off_t size;
@@ -459,6 +449,25 @@ int zImage_arm_load(int argc, char **argv, const char *buf, off_t len,
(unsigned long long)size,
(unsigned long long)len);
+ /* check if zImage has an appended dtb */
+ if (!dtb_file) {
+ if (fdt_check_header(buf + size) == 0) {
+ /*
+ * dtb_file won't be opened. It's set
+ * just to pass NULL checking.
+ */
+ dtb_file = APPENDED_FDT;
+ dtb_length = fdt_totalsize(buf + size);
+ dtb_buf = xmalloc(dtb_length);
+ memcpy(dtb_buf, buf+size, dtb_length);
+
+ dbgprintf("found appended dtb, size 0x%llx\n",
+ (unsigned long long)dtb_length);
+
+
+ }
+ }
+
if (size > len) {
fprintf(stderr,
"zImage is truncated - file 0x%llx vs header 0x%llx\n",
@@ -575,6 +584,22 @@ int zImage_arm_load(int argc, char **argv, const char *buf, off_t len,
initrd_base = kernel_base + _ALIGN(len * 5, getpagesize());
}
+ if (!use_atags && !dtb_file) {
+ int f;
+
+ f = have_sysfs_fdt();
+ if (f)
+ dtb_file = SYSFS_FDT;
+ }
+
+ /*
+ * dtb_buf can be allocated when checking zImage_with_dtb.
+ * dtb_buf won't be allocated in the logic if dtb_file is handed over
+ * in argv[].
+ */
+ if (dtb_file && !dtb_buf)
+ dtb_buf = slurp_file(dtb_file, &dtb_length);
+
if (use_atags) {
/*
* use ATAGs from /proc/atags
--
2.7.4
_______________________________________________
kexec mailing list
http://lists.infradead.org/mailman/listinfo/kexec

Hoeun Ryu

2017-06-27 02:13:17 UTC

Post by Hoeun Ryu
Arm linux supports zImage with appended dtb
(CONFIG_ARM_APPENDED_DTB) and
the concatenated image is generated like `cat zImage dtb >
zImage_w_dtb`.
This patch is to support the concatednated zImage. This changes the
priority of source of dtb file.
1. --dtb dtb_file
2. zImage_w_dtb <= newly added
3. /sys/firmware/fdt
4. /proc/device-tree

The original default fdt is /sys/firmware/fdt, after your changes it
becomes zImage attached dtb, presumably one knows his zImage appended a
dtb it is fine, but if one does not notice it, it changes the
behavior
he expects.
How about add a new option for zImage_dtb, like --zImage-dtb

It sounds good to me too.
I think I can accept your suggestion in the next version if the others
agree.

Thank you for the review.

Post by Hoeun Ryu
Users don't need to specify dtb file in the command line and don't have to
have /sys/firmware/fdt or /proc/device-tree if dtb is available in zImage.
---
kexec/arch/arm/kexec-arm.h        |  1 +
kexec/arch/arm/kexec-zImage-arm.c | 47
++++++++++++++++++++++++++++++---------
2 files changed, 37 insertions(+), 11 deletions(-)
diff --git a/kexec/arch/arm/kexec-arm.h b/kexec/arch/arm/kexec-
arm.h
index a74cce2..94695b7 100644
--- a/kexec/arch/arm/kexec-arm.h
+++ b/kexec/arch/arm/kexec-arm.h
@@ -4,6 +4,7 @@
#include <sys/types.h>

#define SYSFS_FDT "/sys/firmware/fdt"
+#define APPENDED_FDT ((char *)-1) /* it doesn't mean to be opened. */
#define BOOT_BLOCK_VERSION 17
#define BOOT_BLOCK_LAST_COMP_VERSION 16

diff --git a/kexec/arch/arm/kexec-zImage-arm.c
b/kexec/arch/arm/kexec-zImage-arm.c
index 7f02b93..59f0003 100644
--- a/kexec/arch/arm/kexec-zImage-arm.c
+++ b/kexec/arch/arm/kexec-zImage-arm.c
@@ -390,6 +390,7 @@ int zImage_arm_load(int argc, char **argv,
const char *buf, off_t len,
initrd_size = 0;
use_atags = 0;
dtb_file = NULL;
+ dtb_buf = NULL;
while((opt = getopt_long(argc, argv, short_options,
options, 0)) != -1) {
switch(opt) {
@@ -424,14 +425,6 @@ int zImage_arm_load(int argc, char **argv,
const char *buf, off_t len,
return -1;
}

- if (!use_atags && !dtb_file) {
- int f;
-
- f = have_sysfs_fdt();
- if (f)
- dtb_file = SYSFS_FDT;
- }
-
if (command_line) {
command_line_len = strlen(command_line) + 1;
if (command_line_len > COMMAND_LINE_SIZE)
@@ -440,9 +433,6 @@ int zImage_arm_load(int argc, char **argv,
const char *buf, off_t len,
if (ramdisk)
ramdisk_buf = slurp_file(ramdisk, &initrd_size);

- if (dtb_file)
- dtb_buf = slurp_file(dtb_file, &dtb_length);
-
if (len > 0x34) {
const struct zimage_header *hdr;
off_t size;
@@ -459,6 +449,25 @@ int zImage_arm_load(int argc, char **argv,
const char *buf, off_t len,
  (unsigned long long)size,
  (unsigned long long)len);

+ /* check if zImage has an appended dtb */
+ if (!dtb_file) {
+ if (fdt_check_header(buf + size)
== 0) {
+ /*
+ * dtb_file won't be
opened. It's set
+ * just to pass NULL
checking.
+ */
+ dtb_file = APPENDED_FDT;
+ dtb_length =
fdt_totalsize(buf + size);
+ dtb_buf =
xmalloc(dtb_length);
+ memcpy(dtb_buf, buf+size,
dtb_length);
+
+ dbgprintf("found appended
dtb, size 0x%llx\n",
+   (unsigned long
long)dtb_length);
+
+
+ }
+ }
+
if (size > len) {
fprintf(stderr,
"zImage is truncated -
file 0x%llx vs header 0x%llx\n",
@@ -575,6 +584,22 @@ int zImage_arm_load(int argc, char **argv,
const char *buf, off_t len,
initrd_base = kernel_base + _ALIGN(len * 5,
getpagesize());
}

+ if (!use_atags && !dtb_file) {
+ int f;
+
+ f = have_sysfs_fdt();
+ if (f)
+ dtb_file = SYSFS_FDT;
+ }
+
+ /*
+ * dtb_buf can be allocated when checking zImage_with_dtb.
+ * dtb_buf won't be allocated in the logic if dtb_file is
handed over
+ * in argv[].
+ */
+ if (dtb_file && !dtb_buf)
+ dtb_buf = slurp_file(dtb_file, &dtb_length);
+
if (use_atags) {
/*
* use ATAGs from /proc/atags

Russell King

2017-06-26 09:14:12 UTC

This post might be inappropriate. Click to display it.

Hoeun Ryu

2017-06-27 02:36:43 UTC

Post by Hoeun Ryu
Arm linux supports zImage with appended dtb
(CONFIG_ARM_APPENDED_DTB) and
the concatenated image is generated like `cat zImage dtb >
zImage_w_dtb`.

We support that only for the purpose of allowing old boot loaders that
are not DT aware to load kernels that require DT. If it weren't for
that, we wouldn't have it.
I don't see why we should propagate this hack to other systems such as
kexec, especially when they have native DT support.

We have some cases when we would like to use different dtb from the
running system when using kexec and I think that's why kexec-tools
supports --dtb command line option.

For example, I have the second kernel for the crash dump with different
kernel configuration and the different dtb from the running system. I'd
like to exclude some nodes/properties from dtb like memory reservations
or unnecessary devices to keep the second kernel/dtb minimal.

The concatenated zImage for arm has a benefit (whether it's intended or
not) to make it possible for users to merge multiple boot images into a
simple single file.
What I'd like to do is just to support the concatenated zImage so that
users can use --load(-panic) zImage_with_dtb_from_the_running_system
instead of --load(-panic) zImage --dtb
different_dtb_from_the_running_system.

Thank you for the review.

Russell King

2017-06-27 08:53:59 UTC

Post by Hoeun Ryu
We have some cases when we would like to use different dtb from the
running system when using kexec and I think that's why kexec-tools
supports --dtb command line option.
For example, I have the second kernel for the crash dump with different
kernel configuration and the different dtb from the running system. I'd
like to exclude some nodes/properties from dtb like memory reservations
or unnecessary devices to keep the second kernel/dtb minimal.
The concatenated zImage for arm has a benefit (whether it's intended or
not) to make it possible for users to merge multiple boot images into a
simple single file.
What I'd like to do is just to support the concatenated zImage so that
users can use --load(-panic) zImage_with_dtb_from_the_running_system
instead of --load(-panic) zImage --dtb
different_dtb_from_the_running_system.

Don't you mean --load(-panic) zImage_with_different_dtb_from_the_running_system
?

So, what you seem to be saying is that you think it's easier for the user
to do this:

# cat zImage some-other-dtb > zImage.dtb
# kexec --load-panic zImage.dtb

rather than:

# kexec --load-panic zImage --dtb some-other-dtb

?

What about the initrd? Do you want to append that as well?

--
Russell King

Hoeun Ryu

2017-06-27 09:51:50 UTC

Don't you mean --load(-panic) zImage_with_different_dtb_from_the_running_system
?

I'm sorry. This is what I mean.

Post by Russell King
So, what you seem to be saying is that you think it's easier for the user
# cat zImage some-other-dtb > zImage.dtb
# kexec --load-panic zImage.dtb
# kexec --load-panic zImage --dtb some-other-dtb
?

Right.

Post by Russell King
What about the initrd? Do you want to append that as well?

I have thought of it.
I think It would be better to have it.
But I think this is the first step to do so.

Post by Russell King
--
Russell King

Russell King

2017-07-03 16:41:10 UTC

Post by Russell King
What about the initrd? Do you want to append that as well?

I have thought of it.
I think It would be better to have it.
But I think this is the first step to do so.

That is something we don't support with the kernel, and would be insane
to do so - it would mean that the very dumb decompressor would have
to relocate not only the dtb image, but also the initrd image to
some other part of memory. It moves the appended dtb image along with
the rest of the zImage as one complete blob, but that doesn't work
so well for an appended initrd. It will also be rather slow.

So I'd like to continue my discouragement of this entire approach and
say that kexec-tools should *not* add support for an appended DTB nor
an appended initrd.

The kernel build process gives you the kernel image and dtb file.
The appended-dtb image support that we have in the kernel is for
backwards compatibility with non-DT aware boot loaders that only
know how to deal with one or two images at boot time.

--
Russell King

Dave Young

2017-07-04 01:41:45 UTC

Post by Russell King
What about the initrd? Do you want to append that as well?

I have thought of it.
I think It would be better to have it.
But I think this is the first step to do so.

I also agree that we'd better not adding this in kexec-tools, --dtb
works well, the appended dtb does not gain much but it introduces more
code to maintain.

Post by Russell King
The kernel build process gives you the kernel image and dtb file.
The appended-dtb image support that we have in the kernel is for
backwards compatibility with non-DT aware boot loaders that only
know how to deal with one or two images at boot time.
--
Russell King

Thanks
Dave

Hoeun Ryu

2017-07-04 02:09:47 UTC

What about the initrd? Do you want to append that as well?

I have thought of it.
I think It would be better to have it.
But I think this is the first step to do so.

I also agree that we'd better not adding this in kexec-tools, --dtb
works well, the appended dtb does not gain much but it introduces more
code to maintain.

I understand and respect your opinion.
Thank you for the review.

Hoeun Ryu

2017-07-04 02:09:04 UTC

What about the initrd? Do you want to append that as well?

I have thought of it.
I think It would be better to have it.
But I think this is the first step to do so.

That is something we don't support with the kernel, and would be insane
to do so - it would mean that the very dumb decompressor would have
to relocate not only the dtb image, but also the initrd image to
some other part of memory. It moves the appended dtb image along
with
the rest of the zImage as one complete blob, but that doesn't work
so well for an appended initrd. It will also be rather slow.
So I'd like to continue my discouragement of this entire approach and
say that kexec-tools should *not* add support for an appended DTB nor
an appended initrd.
The kernel build process gives you the kernel image and dtb file.
The appended-dtb image support that we have in the kernel is for
backwards compatibility with non-DT aware boot loaders that only
know how to deal with one or two images at boot time.

I understand and respect your opinion.
Thank you for the review.

Pratyush Anand

2017-06-26 09:15:08 UTC

any specific reason to not to add the new method as last preferred option?
Does it not work if there exist a /sys/firmware/fdt in case of a kernel booted
with "dtb appended zImage"?

Post by Hoeun Ryu
---
kexec/arch/arm/kexec-arm.h | 1 +
kexec/arch/arm/kexec-zImage-arm.c | 47 ++++++++++++++++++++++++++++++---------
2 files changed, 37 insertions(+), 11 deletions(-)
diff --git a/kexec/arch/arm/kexec-arm.h b/kexec/arch/arm/kexec-arm.h
index a74cce2..94695b7 100644
--- a/kexec/arch/arm/kexec-arm.h
+++ b/kexec/arch/arm/kexec-arm.h
@@ -4,6 +4,7 @@
#include <sys/types.h>
#define SYSFS_FDT "/sys/firmware/fdt"
+#define APPENDED_FDT ((char *)-1) /* it doesn't mean to be opened. */
#define BOOT_BLOCK_VERSION 17
#define BOOT_BLOCK_LAST_COMP_VERSION 16
diff --git a/kexec/arch/arm/kexec-zImage-arm.c b/kexec/arch/arm/kexec-zImage-arm.c
index 7f02b93..59f0003 100644
--- a/kexec/arch/arm/kexec-zImage-arm.c
+++ b/kexec/arch/arm/kexec-zImage-arm.c
@@ -390,6 +390,7 @@ int zImage_arm_load(int argc, char **argv, const char *buf, off_t len,
initrd_size = 0;
use_atags = 0;
dtb_file = NULL;
+ dtb_buf = NULL;
while((opt = getopt_long(argc, argv, short_options, options, 0)) != -1) {
switch(opt) {
@@ -424,14 +425,6 @@ int zImage_arm_load(int argc, char **argv, const char *buf, off_t len,
return -1;
}
- if (!use_atags && !dtb_file) {
- int f;
-
- f = have_sysfs_fdt();
- if (f)
- dtb_file = SYSFS_FDT;
- }
-
if (command_line) {
command_line_len = strlen(command_line) + 1;
if (command_line_len > COMMAND_LINE_SIZE)
@@ -440,9 +433,6 @@ int zImage_arm_load(int argc, char **argv, const char *buf, off_t len,
if (ramdisk)
ramdisk_buf = slurp_file(ramdisk, &initrd_size);
- if (dtb_file)
- dtb_buf = slurp_file(dtb_file, &dtb_length);
-
if (len > 0x34) {
const struct zimage_header *hdr;
off_t size;
@@ -459,6 +449,25 @@ int zImage_arm_load(int argc, char **argv, const char *buf, off_t len,
(unsigned long long)size,
(unsigned long long)len);
+ /* check if zImage has an appended dtb */
+ if (!dtb_file) {
+ if (fdt_check_header(buf + size) == 0) {

What if size >= len? I think, in that case there might not be a valid address
at buf+size and reading those area would lead in segmentation fault.

Post by Hoeun Ryu
+ /*
+ * dtb_file won't be opened. It's set
+ * just to pass NULL checking.
+ */
+ dtb_file = APPENDED_FDT;
+ dtb_length = fdt_totalsize(buf + size);
+ dtb_buf = xmalloc(dtb_length);
+ memcpy(dtb_buf, buf+size, dtb_length);
+
+ dbgprintf("found appended dtb, size 0x%llx\n",
+ (unsigned long long)dtb_length);
+
+
+ }
+ }
+
if (size > len) {
fprintf(stderr,
"zImage is truncated - file 0x%llx vs header 0x%llx\n",
@@ -575,6 +584,22 @@ int zImage_arm_load(int argc, char **argv, const char *buf, off_t len,
initrd_base = kernel_base + _ALIGN(len * 5, getpagesize());
}
+ if (!use_atags && !dtb_file) {
+ int f;
+
+ f = have_sysfs_fdt();
+ if (f)
+ dtb_file = SYSFS_FDT;
+ }
+
+ /*
+ * dtb_buf can be allocated when checking zImage_with_dtb.
+ * dtb_buf won't be allocated in the logic if dtb_file is handed over
+ * in argv[].
+ */
+ if (dtb_file && !dtb_buf)
+ dtb_buf = slurp_file(dtb_file, &dtb_length);
+
if (use_atags) {
/*
* use ATAGs from /proc/atags

--
Pratyush

Hoeun Ryu

2017-06-27 02:52:17 UTC