Skip to contentSkip to conversation

introduce genimg_get_kernel_addr()

PATCH New Bryan Wu 4 participants 13 messages Aug 5, 2014 – Aug 15, 2014

lore
Share & tools
lore

Bryan Wu

author
#0 lore raw

v2 cover introduce genimg_get_kernel_addr()

When trying to fix the error message issue in pxe sysboot, we found it's need a real kernel address for further image format detection.

So I take out such code from boot_get_kernel() of bootm.c and create the new API functin genimg_get_kernel_addr(). Then convert pxe/sysboot and bootm.c to use it.

v2:
- move function description to header file
- fix coding style issue
- use argc < 1 check when calling genimg_get_kernel_addr() in bootm.c

Bryan Wu (3):
image: introduce genimg_get_kernel_addr()
pxe: detect image format before calling bootm/bootz
bootm: use genimg_get_kernel_addr()

 common/bootm.c   | 25 +++++--------------------
 common/cmd_pxe.c | 15 +++++++++++----
 common/image.c   | 32 ++++++++++++++++++++++++++++++++
 include/image.h  | 12 ++++++++++++
 4 files changed, 60 insertions(+), 24 deletions(-)
Signature · 1 line
1.9.1
author
#1 lore raw

PATCH v2 1/3 image: introduce genimg_get_kernel_addr()

Kernel address is normally stored as a string argument of bootm or bootz. This function is taken out from boot_get_kernel() of bootm.c, which can be reused by others.

---
 common/image.c  | 32 ++++++++++++++++++++++++++++++++
 include/image.h | 12 ++++++++++++
 2 files changed, 44 insertions(+)
common/image.c +32 -0
index 11b3cf5..36fe487 100644
@@ -642,6 +642,38 @@ int genimg_get_comp_id(const char *name)
 }
 
 #ifndef USE_HOSTCC
+ulong genimg_get_kernel_addr(char * const img_addr)
+{
+#if defined(CONFIG_FIT)
+	const char *fit_uname_config = NULL;
+	const char *fit_uname_kernel = NULL;
+#endif
+	ulong kernel_addr;
+
+	/* find out kernel image address */
+	if (!img_addr) {
+		kernel_addr = load_addr;
+		debug("*  kernel: default image load address = 0x%08lx\n",
+		      load_addr);
+#if defined(CONFIG_FIT)
+	} else if (fit_parse_conf(img_addr, load_addr, &kernel_addr,
+				  fit_uname_config)) {
+		debug("*  kernel: config '%s' from image at 0x%08lx\n",
+		      *fit_uname_config, kernel_addr);
+	} else if (fit_parse_subimage(img_addr, load_addr, &kernel_addr,
+				     fit_uname_kernel)) {
+		debug("*  kernel: subimage '%s' from image at 0x%08lx\n",
+		      *fit_uname_kernel, kernel_addr);
+#endif
+	} else {
+		kernel_addr = simple_strtoul(img_addr, NULL, 16);
+		debug("*  kernel: cmdline image address = 0x%08lx\n",
+		      kernel_addr);
+	}
+
+	return kernel_addr;
+}
+
 /**
  * genimg_get_format - get image format type
  * @img_addr: image start address
include/image.h +12 -0
index 3e8f78d..049a400 100644
@@ -424,6 +424,18 @@ enum fit_load_op {
 #define IMAGE_FORMAT_FIT	0x02	/* new, libfdt based format */
 #define IMAGE_FORMAT_ANDROID	0x03	/* Android boot image */
 
+/**
+ * genimg_get_kernel_addr - get the real kernel address
+ * @img_addr: a string might contain real image address. If img_addr is NULL,
+ *            return kernel_addr as load_addr.
+ *
+ * genimg_get_kernel_addr() get the real kernel start address from a string
+ * which is normally the first argv of bootm/bootz
+ *
+ * returns:
+ *     kernel start address
+ */
+ulong genimg_get_kernel_addr(char * const img_addr);
 int genimg_get_format(const void *img_addr);
 int genimg_has_config(bootm_headers_t *images);
 ulong genimg_get_image(ulong img_addr);
Signature · 1 line
1.9.1
author
#3 lore raw

PATCH v2 2/3 pxe: detect image format before calling bootm/bootz

In reply to Bryan Wu

Trying bootm for zImage will print out several error message which is not necessary for this case. So detect image format firstly, only try bootm for legacy and FIT format image then try bootz for others.

This patch needs new function genimg_get_kernel_addr().

---
 common/cmd_pxe.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)
common/cmd_pxe.c +11 -4
index ba48692..59b3598 100644
@@ -1,5 +1,6 @@
 /*
  * Copyright 2010-2011 Calxeda, Inc.
+ * Copyright (c) 2014, NVIDIA CORPORATION.  All rights reserved.
  *
  * SPDX-License-Identifier:	GPL-2.0+
  */
@@ -609,6 +610,8 @@ static int label_boot(cmd_tbl_t *cmdtp, struct pxe_label *label)
 	char *bootargs;
 	int bootm_argc = 3;
 	int len = 0;
+	ulong kernel_addr;
+	void *buf;
 
 	label_print(label);
 
@@ -771,11 +774,15 @@ static int label_boot(cmd_tbl_t *cmdtp, struct pxe_label *label)
 	if (bootm_argv[3])
 		bootm_argc = 4;
 
-	do_bootm(cmdtp, 0, bootm_argc, bootm_argv);
-
+	kernel_addr = genimg_get_kernel_addr(bootm_argv[1]);
+	buf = map_sysmem(kernel_addr, 0);
+	/* Try bootm for legacy and FIT format image */
+	if (genimg_get_format(buf) != IMAGE_FORMAT_INVALID)
+		do_bootm(cmdtp, 0, bootm_argc, bootm_argv);
 #ifdef CONFIG_CMD_BOOTZ
-	/* Try booting a zImage if do_bootm returns */
-	do_bootz(cmdtp, 0, bootm_argc, bootm_argv);
+	/* Try booting a zImage */
+	else
+		do_bootz(cmdtp, 0, bootm_argc, bootm_argv);
 #endif
 	return 1;
 }
Signature · 1 line
1.9.1
author
#4 lore raw

PATCH v2 3/3 bootm: use genimg_get_kernel_addr()

In reply to Bryan Wu

Use the new API which is originally taken out from boot_get_kernel of bootm.c

---
 common/bootm.c | 25 +++++--------------------
 1 file changed, 5 insertions(+), 20 deletions(-)
common/bootm.c +5 -20
index 7ec2ed8..621bff2 100644
@@ -731,26 +731,7 @@ static const void *boot_get_kernel(cmd_tbl_t *cmdtp, int flag, int argc,
 	int		os_noffset;
 #endif
 
-	/* find out kernel image address */
-	if (argc < 1) {
-		img_addr = load_addr;
-		debug("*  kernel: default image load address = 0x%08lx\n",
-		      load_addr);
-#if defined(CONFIG_FIT)
-	} else if (fit_parse_conf(argv[0], load_addr, &img_addr,
-				  &fit_uname_config)) {
-		debug("*  kernel: config '%s' from image at 0x%08lx\n",
-		      fit_uname_config, img_addr);
-	} else if (fit_parse_subimage(argv[0], load_addr, &img_addr,
-				     &fit_uname_kernel)) {
-		debug("*  kernel: subimage '%s' from image@0x%08lx\n",
-		      fit_uname_kernel, img_addr);
-#endif
-	} else {
-		img_addr = simple_strtoul(argv[0], NULL, 16);
-		debug("*  kernel: cmdline image address = 0x%08lx\n",
-		      img_addr);
-	}
+	img_addr = genimg_get_kernel_addr(argc < 1 ? NULL : argv[0]);
 
 	bootstage_mark(BOOTSTAGE_ID_CHECK_MAGIC);
 
@@ -807,6 +788,10 @@ static const void *boot_get_kernel(cmd_tbl_t *cmdtp, int flag, int argc,
 #endif
 #if defined(CONFIG_FIT)
 	case IMAGE_FORMAT_FIT:
+		if (!fit_parse_conf(argv[0], load_addr, &img_addr,
+					fit_uname_config))
+			fit_parse_subimage(argv[0], load_addr, &img_addr,
+					fit_uname_kernel);
 		os_noffset = fit_image_load(images, img_addr,
 				&fit_uname_kernel, &fit_uname_config,
 				IH_ARCH_DEFAULT, IH_TYPE_KERNEL,
Signature · 1 line
1.9.1
#6 lore raw

PATCH v2 3/3 bootm: use genimg_get_kernel_addr()

In reply to Bryan Wu
Quoted 14 lines · Bryan Wu

Use the new API which is originally taken out from boot_get_kernel of bootm.c

---
 common/bootm.c | 25 +++++--------------------
 1 file changed, 5 insertions(+), 20 deletions(-)
common/bootm.c +0 -0
index 7ec2ed8..621bff2 100644

<snip>

Quoted 4 lines
common/bootm.c +0 -0
 	bootstage_mark(BOOTSTAGE_ID_CHECK_MAGIC);

common/bootm.c static const void *boot_get_kernel(cmd_tbl_t *cmdtp, int flag, int argc, · line 797

Quoted 10 lines
common/bootm.c +4 -0
@@ -807,6 +788,10 @@ static const void *boot_get_kernel(cmd_tbl_t *cmdtp, int flag, int argc,
 #endif
 #if defined(CONFIG_FIT)
 	case IMAGE_FORMAT_FIT:
+		if (!fit_parse_conf(argv[0], load_addr, &img_addr,
+					fit_uname_config))
+			fit_parse_subimage(argv[0], load_addr, &img_addr,
+					fit_uname_kernel);
 		os_noffset = fit_image_load(images, img_addr,
 				&fit_uname_kernel, &fit_uname_config,
 				IH_ARCH_DEFAULT, IH_TYPE_KERNEL,

Did anyone test this addition? It breaks my booting. The variable img_addr get changed for calling fit_parse_conf(). If I don't use load_addr variable and run "bootm <addr>" with a FIT image, this will fail. It took me hours to figure out.

Why is this change needed? What does it fix?

York

author
#7 lore raw

PATCH v2 3/3 bootm: use genimg_get_kernel_addr()

Quoted 37 lines · York Sun
Quoted 14 lines · Bryan Wu

Use the new API which is originally taken out from boot_get_kernel of bootm.c

---
 common/bootm.c | 25 +++++--------------------
 1 file changed, 5 insertions(+), 20 deletions(-)
common/bootm.c +0 -0
index 7ec2ed8..621bff2 100644

<snip>

Quoted 4 lines
      bootstage_mark(BOOTSTAGE_ID_CHECK_MAGIC);

common/bootm.c static const void *boot_get_kernel(cmd_tbl_t *cmdtp, int flag, int argc, · line 797

Quoted 10 lines
common/bootm.c +4 -0
@@ -807,6 +788,10 @@ static const void *boot_get_kernel(cmd_tbl_t *cmdtp, int flag, int argc,
 #endif
 #if defined(CONFIG_FIT)
      case IMAGE_FORMAT_FIT:
+             if (!fit_parse_conf(argv[0], load_addr, &img_addr,
+                                     fit_uname_config))
+                     fit_parse_subimage(argv[0], load_addr, &img_addr,
+                                     fit_uname_kernel);
              os_noffset = fit_image_load(images, img_addr,
                              &fit_uname_kernel, &fit_uname_config,
                              IH_ARCH_DEFAULT, IH_TYPE_KERNEL,

Did anyone test this addition? It breaks my booting. The variable img_addr get changed for calling fit_parse_conf(). If I don't use load_addr variable and run "bootm <addr>" with a FIT image, this will fail. It took me hours to figure out.

Quoted 4 lines

Why is this change needed? What does it fix?

York

I think Tom merged one with the fixing:

----
+ if (!fit_parse_conf(argv[0], load_addr, &img_addr,
+ &fit_uname_config))
+ fit_parse_subimage(argv[0], load_addr, &img_addr,
+ &fit_uname_kernel);
----

Can you try that?
-Bryan

#8 lore raw

PATCH v2 3/3 bootm: use genimg_get_kernel_addr()

Sorry for top posting from my phone.

I tested with the latest code merged, not your original patch.

York

________________________________
From: Bryan Wu
Sent: Thu, 14/08/2014 18:05
To: Sun York-R58495 <[email protected]>
CC: Tom Rini <[email protected]>; Stephen Warren <[email protected]>; u-boot at lists.denx.de; simon Glass <[email protected]>; Basu Arnab-B45036 <[email protected]>
Subject: Re: [U-Boot] [PATCH v2 3/3] bootm: use genimg_get_kernel_addr()

Quoted 37 lines · York Sun
Quoted 14 lines · Bryan Wu

Use the new API which is originally taken out from boot_get_kernel of bootm.c

---
 common/bootm.c | 25 +++++--------------------
 1 file changed, 5 insertions(+), 20 deletions(-)
common/bootm.c +0 -0
index 7ec2ed8..621bff2 100644

<snip>

Quoted 4 lines
      bootstage_mark(BOOTSTAGE_ID_CHECK_MAGIC);

common/bootm.c static const void *boot_get_kernel(cmd_tbl_t *cmdtp, int flag, int argc, · line 797

Quoted 10 lines
common/bootm.c +4 -0
@@ -807,6 +788,10 @@ static const void *boot_get_kernel(cmd_tbl_t *cmdtp, int flag, int argc,
 #endif
 #if defined(CONFIG_FIT)
      case IMAGE_FORMAT_FIT:
+             if (!fit_parse_conf(argv[0], load_addr, &img_addr,
+                                     fit_uname_config))
+                     fit_parse_subimage(argv[0], load_addr, &img_addr,
+                                     fit_uname_kernel);
              os_noffset = fit_image_load(images, img_addr,
                              &fit_uname_kernel, &fit_uname_config,
                              IH_ARCH_DEFAULT, IH_TYPE_KERNEL,

Did anyone test this addition? It breaks my booting. The variable img_addr get changed for calling fit_parse_conf(). If I don't use load_addr variable and run "bootm <addr>" with a FIT image, this will fail. It took me hours to figure out.

Quoted 4 lines

Why is this change needed? What does it fix?

York

I think Tom merged one with the fixing:

----
+ if (!fit_parse_conf(argv[0], load_addr, &img_addr,
+ &fit_uname_config))
+ fit_parse_subimage(argv[0], load_addr, &img_addr,
+ &fit_uname_kernel);
----

Can you try that?
-Bryan

#9 lore raw

PATCH v2 3/3 bootm: use genimg_get_kernel_addr()

In reply to York Sun
Quoted 20 lines · York Sun
Quoted 14 lines · Bryan Wu

Use the new API which is originally taken out from boot_get_kernel of bootm.c

---
 common/bootm.c | 25 +++++--------------------
 1 file changed, 5 insertions(+), 20 deletions(-)
common/bootm.c +0 -0
index 7ec2ed8..621bff2 100644

<snip>

Quoted 2 lines

bootstage_mark(BOOTSTAGE_ID_CHECK_MAGIC);

Quoted 16 lines

common/bootm.c static const void *boot_get_kernel(cmd_tbl_t *cmdtp, int flag, int argc, · line 797

Quoted 12 lines
common/bootm.c +4 -0
@@ -807,6 +788,10 @@ static const void *boot_get_kernel(cmd_tbl_t *cmdtp, int flag, int argc,
 #endif
 #if defined(CONFIG_FIT)
 	case IMAGE_FORMAT_FIT:
+		if (!fit_parse_conf(argv[0], load_addr, &img_addr,
+					fit_uname_config))
+			fit_parse_subimage(argv[0], load_addr, &img_addr,
+					fit_uname_kernel);
 		os_noffset = fit_image_load(images, img_addr,
 				&fit_uname_kernel, &fit_uname_config,
 				IH_ARCH_DEFAULT, IH_TYPE_KERNEL,

Did anyone test this addition? It breaks my booting. The variable img_addr get changed for calling fit_parse_conf(). If I don't use load_addr variable and run "bootm <addr>" with a FIT image, this will fail. It took me hours to figure out.

How exactly are you booting? I tested a fit with bootm addr#conf at 1 or so.

Signature · 8 lines
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20140814/b7f54ff8/attachment.pgp>
#10 lore raw

PATCH v2 3/3 bootm: use genimg_get_kernel_addr()

Tom,

I tested with "bootm 806f0000". My FIT image is loaded there. I don't have load_addr variable. My default load_addr (from CONFIG macro) is different from this address.

York

________________________________
From: Tom Rini
Sent: Thu, 14/08/2014 18:38
To: Sun York-R58495 <[email protected]>
CC: Bryan Wu <[email protected]>; sjg at chromium.org; swarren at wwwdotorg.org; u-boot at lists.denx.de
Subject: Re: [U-Boot] [PATCH v2 3/3] bootm: use genimg_get_kernel_addr()

Quoted 20 lines · York Sun
Quoted 14 lines · Bryan Wu

Use the new API which is originally taken out from boot_get_kernel of bootm.c

---
 common/bootm.c | 25 +++++--------------------
 1 file changed, 5 insertions(+), 20 deletions(-)
common/bootm.c +0 -0
index 7ec2ed8..621bff2 100644

<snip>

Quoted 2 lines
     bootstage_mark(BOOTSTAGE_ID_CHECK_MAGIC);
Quoted 16 lines

common/bootm.c static const void *boot_get_kernel(cmd_tbl_t *cmdtp, int flag, int argc, · line 797

Quoted 12 lines
common/bootm.c +4 -0
@@ -807,6 +788,10 @@ static const void *boot_get_kernel(cmd_tbl_t *cmdtp, int flag, int argc,
 #endif
 #if defined(CONFIG_FIT)
     case IMAGE_FORMAT_FIT:
+           if (!fit_parse_conf(argv[0], load_addr, &img_addr,
+                                   fit_uname_config))
+                   fit_parse_subimage(argv[0], load_addr, &img_addr,
+                                   fit_uname_kernel);
             os_noffset = fit_image_load(images, img_addr,
                             &fit_uname_kernel, &fit_uname_config,
                             IH_ARCH_DEFAULT, IH_TYPE_KERNEL,

Did anyone test this addition? It breaks my booting. The variable img_addr get changed for calling fit_parse_conf(). If I don't use load_addr variable and run "bootm <addr>" with a FIT image, this will fail. It took me hours to figure out.

How exactly are you booting? I tested a fit with bootm addr#conf at 1 or so.

--
Tom

#11 lore raw

PATCH v2 3/3 bootm: use genimg_get_kernel_addr()

Hi York,

Quoted 5 lines · York Sun

Tom,

I tested with "bootm 806f0000". My FIT image is loaded there. I don't have load_addr variable. My default load_addr (from CONFIG macro) is different from this address.

Yes unfortunately this is completely wrong. I did not catch this in my review unfortunately, perhaps because the & was removed in the code I reviewed.

fit_uname_config and fit_uname_kernel must be passed back from genimg_get_kernel_addr() - that function will need two new parameters.

Regards,
Simon

Quoted 59 lines

York

________________________________
From: Tom Rini
Sent: Thu, 14/08/2014 18:38
To: Sun York-R58495 <[email protected]>
CC: Bryan Wu <[email protected]>; sjg at chromium.org; swarren at wwwdotorg.org; u-boot at lists.denx.de

Subject: Re: [U-Boot] [PATCH v2 3/3] bootm: use genimg_get_kernel_addr()

Quoted 32 lines · York Sun
Quoted 14 lines · Bryan Wu

Use the new API which is originally taken out from boot_get_kernel of bootm.c

---
 common/bootm.c | 25 +++++--------------------
 1 file changed, 5 insertions(+), 20 deletions(-)
common/bootm.c +0 -0
index 7ec2ed8..621bff2 100644

<snip>

Quoted 14 lines
     bootstage_mark(BOOTSTAGE_ID_CHECK_MAGIC);
common/bootm.c +4 -0
@@ -807,6 +788,10 @@ static const void *boot_get_kernel(cmd_tbl_t
 *cmdtp, int flag, int argc,
 #endif
 #if defined(CONFIG_FIT)
     case IMAGE_FORMAT_FIT:
+           if (!fit_parse_conf(argv[0], load_addr, &img_addr,
+                                   fit_uname_config))
+                   fit_parse_subimage(argv[0], load_addr, &img_addr,
+                                   fit_uname_kernel);
             os_noffset = fit_image_load(images, img_addr,
                             &fit_uname_kernel, &fit_uname_config,

IH_ARCH_DEFAULT, IH_TYPE_KERNEL,

Quoted 8 lines
Quoted 1 line

Did anyone test this addition? It breaks my booting. The variable img_addr get
changed for calling fit_parse_conf(). If I don't use load_addr variable and run
"bootm <addr>" with a FIT image, this will fail. It took me hours to figure out.

How exactly are you booting? I tested a fit with bootm addr#conf at 1 or so.

--
Tom

author
#12 lore raw

PATCH v2 3/3 bootm: use genimg_get_kernel_addr()

Quoted 9 lines · Simon Glass

Hi York,

Quoted 5 lines · York Sun

Tom,

I tested with "bootm 806f0000". My FIT image is loaded there. I don't have load_addr variable. My default load_addr (from CONFIG macro) is different from this address.

Quoted 7 lines

Yes unfortunately this is completely wrong. I did not catch this in my review unfortunately, perhaps because the & was removed in the code I reviewed.

fit_uname_config and fit_uname_kernel must be passed back from genimg_get_kernel_addr() - that function will need two new parameters.

I'm working on a fix to add this 2 new parameters.

-Bryan

Quoted 60 lines
Quoted 60 lines

York

________________________________
From: Tom Rini
Sent: Thu, 14/08/2014 18:38
To: Sun York-R58495 <[email protected]>
CC: Bryan Wu <[email protected]>; sjg at chromium.org; swarren at wwwdotorg.org; u-boot at lists.denx.de

Subject: Re: [U-Boot] [PATCH v2 3/3] bootm: use genimg_get_kernel_addr()

Quoted 32 lines · York Sun
Quoted 14 lines · Bryan Wu

Use the new API which is originally taken out from boot_get_kernel of bootm.c

---
 common/bootm.c | 25 +++++--------------------
 1 file changed, 5 insertions(+), 20 deletions(-)
common/bootm.c +0 -0
index 7ec2ed8..621bff2 100644

<snip>

Quoted 14 lines
     bootstage_mark(BOOTSTAGE_ID_CHECK_MAGIC);
common/bootm.c +4 -0
@@ -807,6 +788,10 @@ static const void *boot_get_kernel(cmd_tbl_t
 *cmdtp, int flag, int argc,
 #endif
 #if defined(CONFIG_FIT)
     case IMAGE_FORMAT_FIT:
+           if (!fit_parse_conf(argv[0], load_addr, &img_addr,
+                                   fit_uname_config))
+                   fit_parse_subimage(argv[0], load_addr, &img_addr,
+                                   fit_uname_kernel);
             os_noffset = fit_image_load(images, img_addr,
                             &fit_uname_kernel, &fit_uname_config,

IH_ARCH_DEFAULT, IH_TYPE_KERNEL,

Quoted 8 lines
Quoted 1 line

Did anyone test this addition? It breaks my booting. The variable img_addr get
changed for calling fit_parse_conf(). If I don't use load_addr variable and run
"bootm <addr>" with a FIT image, this will fail. It took me hours to figure out.

How exactly are you booting? I tested a fit with bootm addr#conf at 1 or so.

--
Tom