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

[Hoeun Ryu <hoeun.ryu@gmail.com>]

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