| Message ID | 20140521195824.GE1752@bill-the-cat |
|---|---|
| State | RFC |
| Delegated to: | Tom Rini |
| Headers | show |
Dear Tom Rini, In message <20140521195824.GE1752@bill-the-cat> you wrote: > > Something that Rob mentioned to me at ELC, and others have mentioned > before is that it would be nice if 'bootm' (which says "boot application > image stored in memory" in the help, even) would just work with zImage > or Image or whatever is spit directly out of the kernel. I don;t think this is a good idea. "application image" is supposed to mean "one of the U-Boot image formats", which means the old legacy image format (with the 64 byte header), or FIT images. To boot a zImage file, we have the "bootz" command. I also think such a patch is pushing into the wrong direction. We should rather try and improve the kernel support for FIT images. > + zi = (struct zimage_header *)map_sysmem(image, 0); > + if (zi->zi_magic == LINUX_ARM_ZIMAGE_MAGIC) > + return 0; I smell endianess issues here? > +#ifdef CONFIG_BOOTM_LINUX_RAW LINUX_RAW ? > /* Image format types, returned by _get_format() routine */ > #define IMAGE_FORMAT_INVALID 0x00 > +#define IMAGE_FORMAT_LINUX 0xFF /* Linux kernel defined formats */ > #define IMAGE_FORMAT_LEGACY 0x01 /* legacy image_header based format */ > #define IMAGE_FORMAT_FIT 0x02 /* new, libfdt based format */ This does not look clean to me. > It of course doesn't work and just shows where I think we would need to > fill things in and probably provide some __weak functions for other > arches. Looking over how we do bootz today, and how I wrote booti for > arm64, it should be possible to do the correct callouts at the correct > places for "oh, we don't have a legacy or FIT header, we have a per > Linux architecture defined header". > > What does everyone think about extending things in this direction? I am no real friend of trying to be clever and automatically guess image formats. In my opinion it is more reliable to be able to verify if an image os OK or corrupted, instead of passing a corrupted image on to guesswork which might then belive it could be some other image format (without checksums or such) and try to run it. The user should give U-Boot a clear hint which image format to expect, and allow U-Boot to complain otherwise. If someone really wants such a trial and error approach, he can still do this without adding code that affects everybody with just a little scripting - like "bootm $addr || bootz $addr || ...". Best regards, Wolfgang Denk
On Wed, May 21, 2014 at 10:10:50PM +0200, Wolfgang Denk wrote: > Dear Tom Rini, > > In message <20140521195824.GE1752@bill-the-cat> you wrote: > > > > Something that Rob mentioned to me at ELC, and others have mentioned > > before is that it would be nice if 'bootm' (which says "boot application > > image stored in memory" in the help, even) would just work with zImage > > or Image or whatever is spit directly out of the kernel. > > I don;t think this is a good idea. "application image" is supposed to > mean "one of the U-Boot image formats", which means the old legacy > image format (with the 64 byte header), or FIT images. To boot a > zImage file, we have the "bootz" command. Yes, it's historically meant something with an essentially (technically no, practically, yes) U-Boot centric header on it. But that's not what the help text says. And yes we have bootz for zImages and I added booti for Image images. That resulted in "You mean I have to type different things for arm and arm64? *sigh*" when explaining this in person. > I also think such a patch is pushing into the wrong direction. We > should rather try and improve the kernel support for FIT images. That's neither here nor there. You can create and boot FIT images today, anywhere it's enabled (including arm64). You can do the same with legacy images (which also resulted in sighs when I mentioned this). The kernel doesn't want any of this in the kernel tree. Developers want to have as few steps between "build my kernel" and "now I'm testing my kernel". Adding in "create / grab stub FIT file, run mkimage" results in more unhappy developers. > > + zi = (struct zimage_header *)map_sysmem(image, 0); > > + if (zi->zi_magic == LINUX_ARM_ZIMAGE_MAGIC) > > + return 0; > > I smell endianess issues here? We don't do BE ARM atm anyhow, so one more in the pile of many, most likely. > > +#ifdef CONFIG_BOOTM_LINUX_RAW > > LINUX_RAW ? Yeah. We can grab the color wheel for the bikeshed later, if we go down this path :) > > /* Image format types, returned by _get_format() routine */ > > #define IMAGE_FORMAT_INVALID 0x00 > > +#define IMAGE_FORMAT_LINUX 0xFF /* Linux kernel defined formats */ > > #define IMAGE_FORMAT_LEGACY 0x01 /* legacy image_header based format */ > > #define IMAGE_FORMAT_FIT 0x02 /* new, libfdt based format */ > > This does not look clean to me. I could call it 0x03 too. I was trying to signify that it's basically a lack of well defined header. A legacy image is a legacy image, a FIT image is a FIT image, a Linux image depends on what arch you're talking about. > > It of course doesn't work and just shows where I think we would need to > > fill things in and probably provide some __weak functions for other > > arches. Looking over how we do bootz today, and how I wrote booti for > > arm64, it should be possible to do the correct callouts at the correct > > places for "oh, we don't have a legacy or FIT header, we have a per > > Linux architecture defined header". > > > > What does everyone think about extending things in this direction? > > I am no real friend of trying to be clever and automatically guess > image formats. In my opinion it is more reliable to be able to verify > if an image os OK or corrupted, instead of passing a corrupted image > on to guesswork which might then belive it could be some other image > format (without checksums or such) and try to run it. It is not impossible that a FIT image could become corrupt in just the wrong way to have the zImage (or Image or whatever the arch supports) magic value and then go horribly awry. It seems exceedingly unlikely however. > The user should give U-Boot a clear hint which image format to expect, > and allow U-Boot to complain otherwise. > > If someone really wants such a trial and error approach, he can still > do this without adding code that affects everybody with just a little > scripting - like "bootm $addr || bootz $addr || ...". That's not the usecase really. The usecase is roughly "I just want to boot what the kernel build spit out and not have to guess the command". How about if we added a new command, 'boota[uto]' to make a few stabs at guessing the format? The first problem that pops to mind here is passing around some flag to make sure that bootm doesn't get this new behaviour.
Hi Tom, On 21 May 2014 10:46, Tom Rini <trini@ti.com> wrote: > On Wed, May 21, 2014 at 10:10:50PM +0200, Wolfgang Denk wrote: >> Dear Tom Rini, >> >> In message <20140521195824.GE1752@bill-the-cat> you wrote: >> > >> > Something that Rob mentioned to me at ELC, and others have mentioned >> > before is that it would be nice if 'bootm' (which says "boot application >> > image stored in memory" in the help, even) would just work with zImage >> > or Image or whatever is spit directly out of the kernel. >> >> I don;t think this is a good idea. "application image" is supposed to >> mean "one of the U-Boot image formats", which means the old legacy >> image format (with the 64 byte header), or FIT images. To boot a >> zImage file, we have the "bootz" command. > > Yes, it's historically meant something with an essentially (technically > no, practically, yes) U-Boot centric header on it. But that's not what > the help text says. And yes we have bootz for zImages and I added booti > for Image images. That resulted in "You mean I have to type different > things for arm and arm64? *sigh*" when explaining this in person. > >> I also think such a patch is pushing into the wrong direction. We >> should rather try and improve the kernel support for FIT images. > > That's neither here nor there. You can create and boot FIT images > today, anywhere it's enabled (including arm64). You can do the same > with legacy images (which also resulted in sighs when I mentioned this). > The kernel doesn't want any of this in the kernel tree. Developers want > to have as few steps between "build my kernel" and "now I'm testing my > kernel". Adding in "create / grab stub FIT file, run mkimage" results > in more unhappy developers. Unless I'm imagining it, some years ago I could type 'make fit_image' or similar for the kernel and get an image ready to boot. Did someone remove that feature from Linux and expect the number of steps needed to build a kernel to stay the same? It surprises me the lengths to which people are going to try to shoehorn .dtbs, compression, multiple dtbs, multi-arch etc. into the kenel zImage format. The decompression header is ugly, plus it is slower than doing these things in U-Boot. Regards, Simon
On Fri, May 23, 2014 at 03:57:34PM -1000, Simon Glass wrote: > Hi Tom, > > On 21 May 2014 10:46, Tom Rini <trini@ti.com> wrote: > > On Wed, May 21, 2014 at 10:10:50PM +0200, Wolfgang Denk wrote: > >> Dear Tom Rini, > >> > >> In message <20140521195824.GE1752@bill-the-cat> you wrote: > >> > > >> > Something that Rob mentioned to me at ELC, and others have mentioned > >> > before is that it would be nice if 'bootm' (which says "boot application > >> > image stored in memory" in the help, even) would just work with zImage > >> > or Image or whatever is spit directly out of the kernel. > >> > >> I don;t think this is a good idea. "application image" is supposed to > >> mean "one of the U-Boot image formats", which means the old legacy > >> image format (with the 64 byte header), or FIT images. To boot a > >> zImage file, we have the "bootz" command. > > > > Yes, it's historically meant something with an essentially (technically > > no, practically, yes) U-Boot centric header on it. But that's not what > > the help text says. And yes we have bootz for zImages and I added booti > > for Image images. That resulted in "You mean I have to type different > > things for arm and arm64? *sigh*" when explaining this in person. > > > >> I also think such a patch is pushing into the wrong direction. We > >> should rather try and improve the kernel support for FIT images. > > > > That's neither here nor there. You can create and boot FIT images > > today, anywhere it's enabled (including arm64). You can do the same > > with legacy images (which also resulted in sighs when I mentioned this). > > The kernel doesn't want any of this in the kernel tree. Developers want > > to have as few steps between "build my kernel" and "now I'm testing my > > kernel". Adding in "create / grab stub FIT file, run mkimage" results > > in more unhappy developers. > > Unless I'm imagining it, some years ago I could type 'make fit_image' > or similar for the kernel and get an image ready to boot. Did someone > remove that feature from Linux and expect the number of steps needed > to build a kernel to stay the same? It wasn't in mainline, I'm fairly certain. Or maybe it was an arch/ppc thing that got dropped along the way. > It surprises me the lengths to which people are going to try to > shoehorn .dtbs, compression, multiple dtbs, multi-arch etc. into the > kenel zImage format. The decompression header is ugly, plus it is > slower than doing these things in U-Boot. Well, with arm64 the kernel is just getting out of the business, hence booti (or however we add Image support) and not do the zImage dance.
Hi Tom, On 24 May 2014 06:21, Tom Rini <trini@ti.com> wrote: > On Fri, May 23, 2014 at 03:57:34PM -1000, Simon Glass wrote: > > Hi Tom, > > > > On 21 May 2014 10:46, Tom Rini <trini@ti.com> wrote: > > > On Wed, May 21, 2014 at 10:10:50PM +0200, Wolfgang Denk wrote: > > >> Dear Tom Rini, > > >> > > >> In message <20140521195824.GE1752@bill-the-cat> you wrote: > > >> > > > >> > Something that Rob mentioned to me at ELC, and others have mentioned > > >> > before is that it would be nice if 'bootm' (which says "boot > application > > >> > image stored in memory" in the help, even) would just work with > zImage > > >> > or Image or whatever is spit directly out of the kernel. > > >> > > >> I don;t think this is a good idea. "application image" is supposed to > > >> mean "one of the U-Boot image formats", which means the old legacy > > >> image format (with the 64 byte header), or FIT images. To boot a > > >> zImage file, we have the "bootz" command. > > > > > > Yes, it's historically meant something with an essentially (technically > > > no, practically, yes) U-Boot centric header on it. But that's not what > > > the help text says. And yes we have bootz for zImages and I added > booti > > > for Image images. That resulted in "You mean I have to type different > > > things for arm and arm64? *sigh*" when explaining this in person. > > > > > >> I also think such a patch is pushing into the wrong direction. We > > >> should rather try and improve the kernel support for FIT images. > > > > > > That's neither here nor there. You can create and boot FIT images > > > today, anywhere it's enabled (including arm64). You can do the same > > > with legacy images (which also resulted in sighs when I mentioned > this). > > > The kernel doesn't want any of this in the kernel tree. Developers > want > > > to have as few steps between "build my kernel" and "now I'm testing my > > > kernel". Adding in "create / grab stub FIT file, run mkimage" results > > > in more unhappy developers. > > > > Unless I'm imagining it, some years ago I could type 'make fit_image' > > or similar for the kernel and get an image ready to boot. Did someone > > remove that feature from Linux and expect the number of steps needed > > to build a kernel to stay the same? > > It wasn't in mainline, I'm fairly certain. Or maybe it was an arch/ppc > thing that got dropped along the way. > I took at look and now think I just imagined it, or perhaps had some patches applied. > > > It surprises me the lengths to which people are going to try to > > shoehorn .dtbs, compression, multiple dtbs, multi-arch etc. into the > > kenel zImage format. The decompression header is ugly, plus it is > > slower than doing these things in U-Boot. > > Well, with arm64 the kernel is just getting out of the business, hence > booti (or however we add Image support) and not do the zImage dance. > Does this mean a clean Image with no add-ons? Regards, Simon
On Mon, Jun 02, 2014 at 07:11:14PM -0600, Simon Glass wrote: > Hi Tom, > > On 24 May 2014 06:21, Tom Rini <trini@ti.com> wrote: > > > On Fri, May 23, 2014 at 03:57:34PM -1000, Simon Glass wrote: [snip] > > > It surprises me the lengths to which people are going to try to > > > shoehorn .dtbs, compression, multiple dtbs, multi-arch etc. into the > > > kenel zImage format. The decompression header is ugly, plus it is > > > slower than doing these things in U-Boot. > > > > Well, with arm64 the kernel is just getting out of the business, hence > > booti (or however we add Image support) and not do the zImage dance. > > Does this mean a clean Image with no add-ons? Yes, Image only, and different from arch/arm/boot/Image. Leaving the details of how to find/place the device tree and initrd up to the bootloader.
Hi Tom, On 3 June 2014 10:59, Tom Rini <trini@ti.com> wrote: > On Mon, Jun 02, 2014 at 07:11:14PM -0600, Simon Glass wrote: >> Hi Tom, >> >> On 24 May 2014 06:21, Tom Rini <trini@ti.com> wrote: >> >> > On Fri, May 23, 2014 at 03:57:34PM -1000, Simon Glass wrote: > [snip] >> > > It surprises me the lengths to which people are going to try to >> > > shoehorn .dtbs, compression, multiple dtbs, multi-arch etc. into the >> > > kenel zImage format. The decompression header is ugly, plus it is >> > > slower than doing these things in U-Boot. >> > >> > Well, with arm64 the kernel is just getting out of the business, hence >> > booti (or however we add Image support) and not do the zImage dance. >> >> Does this mean a clean Image with no add-ons? > > Yes, Image only, and different from arch/arm/boot/Image. Leaving the > details of how to find/place the device tree and initrd up to the > bootloader. That sounds promising. So no more decompression / serial UART / cache-enable header? Regards, Simon
On Tue, Jun 03, 2014 at 11:01:21AM -0600, Simon Glass wrote: > Hi Tom, > > On 3 June 2014 10:59, Tom Rini <trini@ti.com> wrote: > > On Mon, Jun 02, 2014 at 07:11:14PM -0600, Simon Glass wrote: > >> Hi Tom, > >> > >> On 24 May 2014 06:21, Tom Rini <trini@ti.com> wrote: > >> > >> > On Fri, May 23, 2014 at 03:57:34PM -1000, Simon Glass wrote: > > [snip] > >> > > It surprises me the lengths to which people are going to try to > >> > > shoehorn .dtbs, compression, multiple dtbs, multi-arch etc. into the > >> > > kenel zImage format. The decompression header is ugly, plus it is > >> > > slower than doing these things in U-Boot. > >> > > >> > Well, with arm64 the kernel is just getting out of the business, hence > >> > booti (or however we add Image support) and not do the zImage dance. > >> > >> Does this mean a clean Image with no add-ons? > > > > Yes, Image only, and different from arch/arm/boot/Image. Leaving the > > details of how to find/place the device tree and initrd up to the > > bootloader. > > That sounds promising. So no more decompression / serial UART / > cache-enable header? For the time being, yes. We'll see what happens when more people are using real hardware rather than simulator and not the boot-wrapper tool.
> > If someone really wants such a trial and error approach, he can still > > do this without adding code that affects everybody with just a little > > scripting - like "bootm $addr || bootz $addr || ...". > > That's not the usecase really. The usecase is roughly "I just want to > boot what the kernel build spit out and not have to guess the command". > > How about if we added a new command, 'boota[uto]' to make a few stabs at > guessing the format? The first problem that pops to mind here is > passing around some flag to make sure that bootm doesn't get this new > behaviour. For the record, boota would be nice. Count me as one of developers that dislike the extra steps... Pavel
diff --git a/arch/arm/lib/bootm.c b/arch/arm/lib/bootm.c index 0706086..14319b2 100644 --- a/arch/arm/lib/bootm.c +++ b/arch/arm/lib/bootm.c @@ -332,6 +332,17 @@ struct zimage_header { #define LINUX_ARM_ZIMAGE_MAGIC 0x016f2818 +int check_image_linux_kernel(ulong image) +{ + struct zimage_header *zi; + + zi = (struct zimage_header *)map_sysmem(image, 0); + if (zi->zi_magic == LINUX_ARM_ZIMAGE_MAGIC) + return 0; + + return 1; +} + int bootz_setup(ulong image, ulong *start, ulong *end) { struct zimage_header *zi; diff --git a/common/cmd_bootm.c b/common/cmd_bootm.c index 30fd643..6bed825 100644 --- a/common/cmd_bootm.c +++ b/common/cmd_bootm.c @@ -274,6 +274,11 @@ static int bootm_find_os(cmd_tbl_t *cmdtp, int flag, int argc, } break; #endif +#ifdef CONFIG_BOOTM_LINUX_RAW + case IMAGE_FORMAT_LINUX: + /* Call something to set images.os.things */ + return 0; +#endif default: puts("ERROR: unknown image format type!\n"); return 1; @@ -1002,6 +1007,11 @@ static const void *boot_get_kernel(cmd_tbl_t *cmdtp, int flag, int argc, images->fit_noffset_os = os_noffset; break; #endif +#ifdef CONFIG_BOOTM_LINUX_RAW + case IMAGE_FORMAT_LINUX: + /* Set loadaddr? */ + return 0; +#endif default: printf("Wrong Image Format for %s command\n", cmdtp->name); bootstage_error(BOOTSTAGE_ID_FIT_KERNEL_INFO); diff --git a/common/image.c b/common/image.c index fcc5a9c..32237a3 100644 --- a/common/image.c +++ b/common/image.c @@ -665,6 +665,10 @@ int genimg_get_format(const void *img_addr) format = IMAGE_FORMAT_FIT; } #endif +#ifdef CONFIG_BOOTM_LINUX_RAW + if (check_image_linux_kernel((ulong)img_addr) == 0) + return IMAGE_FORMAT_LINUX; +#endif return format; } diff --git a/include/image.h b/include/image.h index b278778..985c6f0 100644 --- a/include/image.h +++ b/include/image.h @@ -411,6 +411,7 @@ enum fit_load_op { #ifndef USE_HOSTCC /* Image format types, returned by _get_format() routine */ #define IMAGE_FORMAT_INVALID 0x00 +#define IMAGE_FORMAT_LINUX 0xFF /* Linux kernel defined formats */ #define IMAGE_FORMAT_LEGACY 0x01 /* legacy image_header based format */ #define IMAGE_FORMAT_FIT 0x02 /* new, libfdt based format */ @@ -660,6 +661,15 @@ int image_setup_libfdt(bootm_headers_t *images, void *blob, int image_setup_linux(bootm_headers_t *images); /** + * Check if the given location has some form of Linux-kernel generated + * image. + * + * @param image Potential image location + * @return 0 if OK, 1 if not a known Linux-kernel generated image. + */ +int check_image_linux_kernel(ulong image); + +/** * bootz_setup() - Extract stat and size of a Linux xImage * * @image: Address of image