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

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

[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

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.

Signed-off-by: Hoeun Ryu <hoeun.ryu@gmail.com>
---
 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) {
 		default:
@@ -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
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

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

[Dave Young]

On 06/23/17 at 05:55pm, Hoeun Ryu wrote:
> 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

> 
> 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.
> 
> Signed-off-by: Hoeun Ryu <hoeun.ryu@gmail.com>
> ---
>  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) {
>  		default:
> @@ -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
> kexec@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

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

[Russell King]

On Fri, Jun 23, 2017 at 05:55:38PM +0900, Hoeun Ryu wrote:
> 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.

-- 
Russell King

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

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

[Pratyush Anand]

On Friday 23 June 2017 02:25 PM, Hoeun Ryu wrote:
> 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
>
> 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.

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"?

>
> Signed-off-by: Hoeun Ryu <hoeun.ryu@gmail.com>
> ---
>  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) {
>  		default:
> @@ -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.

> +					/*
> +					 * 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

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

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

[Hoeun Ryu]

On Mon, 2017-06-26 at 10:52 +0800, Dave Young wrote:
> On 06/23/17 at 05:55pm, Hoeun Ryu wrote:
> > 
> > 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.

> > 
> > 
> > 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.
> > 
> > Signed-off-by: Hoeun Ryu <hoeun.ryu@gmail.com>
> > ---
> >  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) {
> >  		default:
> > @@ -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

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

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

[Hoeun Ryu]

On Mon, 2017-06-26 at 10:14 +0100, Russell King wrote:
> On Fri, Jun 23, 2017 at 05:55:38PM +0900, Hoeun Ryu wrote:
> > 
> > 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.


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

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

[Hoeun Ryu]

On Mon, 2017-06-26 at 14:45 +0530, Pratyush Anand wrote:
> 
> On Friday 23 June 2017 02:25 PM, Hoeun Ryu wrote:
> > 
> > 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
> > 
> > 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.
> 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"?
> 

I thought that DTBs from command line should be priotized over /sys or
/proc.
What I intended is to use command line `--load(-panic) zImage_with_dtb`
instead of `--load(-panic) zImage --dtb dtb_file`.
So I thought they should have the identical priority in both cases.

> > 
> > Signed-off-by: Hoeun Ryu <hoeun.ryu@gmail.com>
> > ---
> >  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) {
> >  		default:
> > @@ -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.
> 

Oh. I'll fix the bug in the next version.

Thank you for the review.

> > 
> > +					/*
> > +					 * 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

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

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

[Russell King]

On Tue, Jun 27, 2017 at 11:36:43AM +0900, Hoeun Ryu wrote:
> 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

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

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

[Hoeun Ryu]

> On Jun 27, 2017, at 5:53 PM, Russell King <rmk@armlinux.org.uk> wrote:
> 
>> On Tue, Jun 27, 2017 at 11:36:43AM +0900, Hoeun Ryu wrote:
>> 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
> ?
> 

I'm sorry. This is what I mean.

> 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
> 
> ?

Right.

> 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.

> 
> -- 
> Russell King

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

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

[Russell King]

On Tue, Jun 27, 2017 at 06:51:50PM +0900, Hoeun Ryu wrote:
> > 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

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

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

[Dave Young]

On 07/03/17 at 05:41pm, Russell King wrote:
> On Tue, Jun 27, 2017 at 06:51:50PM +0900, Hoeun Ryu wrote:
> > > 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.

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.

> 
> 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

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

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

[Hoeun Ryu]

On Mon, 2017-07-03 at 17:41 +0100, Russell King wrote:
> On Tue, Jun 27, 2017 at 06:51:50PM +0900, Hoeun Ryu wrote:
> > 
> > > 
> > > 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.


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

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

[Hoeun Ryu]

On Tue, 2017-07-04 at 09:41 +0800, Dave Young wrote:
> On 07/03/17 at 05:41pm, Russell King wrote:
> > 
> > On Tue, Jun 27, 2017 at 06:51:50PM +0900, Hoeun Ryu wrote:
> > > 
> > > > 
> > > > 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.
> 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.

> > 
> > 
> > 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.
> > 

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec