[U-Boot] [PATCH v2 REPOST 2/3] image: Implement IH_TYPE_KERNEL_REL
Stephen Warren
swarren at nvidia.com
Wed Oct 19 17:55:30 CEST 2011
Simon Glass wrote at Tuesday, October 18, 2011 6:57 PM:
> Hi Stephen,
>
> Here are a few comments which may or may not be useful. In general it
> seems that the need to reduce code size change to absolute minimum is
> making the code a bit painful. Maybe relax that by a few 10s of bytes?
Wolfgang,
You had asked for this config option. How strongly do you feel about it;
should it eliminate absolutely all size increase, or should I simplify
the code a bit and remove some of the ifdefs?
> The code size increase you see may be section alignment - e.g. if you
> increase the size of a section by 4 that might push the next section
> up to the next 32-byte boundary.
Ah yes, that makes sense.
> On Tue, Oct 18, 2011 at 2:11 PM, Stephen Warren <swarren at nvidia.com> wrote:
>
> [snip]
> > --- a/arch/arm/cpu/armv7/omap-common/spl.c
> > +++ b/arch/arm/cpu/armv7/omap-common/spl.c
> > @@ -68,7 +68,7 @@ void spl_parse_image_header(const struct image_header *header)
> >
> > if (__be32_to_cpu(header->ih_magic) == IH_MAGIC) {
> > spl_image.size = __be32_to_cpu(header->ih_size) + header_size;
> > - spl_image.entry_point = __be32_to_cpu(header->ih_load);
> > + spl_image.entry_point = __be32_to_cpu(header->ih_load_raw);
>
> I don't completely understand why raw is used here (and in some other
> boards) - is it simply because the abs and raw are always the same in
> SPL/these boards, or something else?
Since I don't have all platforms to test with, my strategy was to use the
"rel" values where I knew they were correct (i.e. stuff used by the bootm
command), and continue to use the "raw" values anywhere else, to avoid any
possibility of regressions. It's just possible that some of the other
places should use "rel" values.
> > --- a/common/image.c
> > +++ b/common/image.c
> > @@ -133,7 +133,14 @@ static const table_entry_t uimage_type[] = {
> > { IH_TYPE_FILESYSTEM, "filesystem", "Filesystem Image", },
> > { IH_TYPE_FIRMWARE, "firmware", "Firmware", },
> > { IH_TYPE_FLATDT, "flat_dt", "Flat Device Tree", },
> > +#ifdef CONFIG_SYS_RELATIVE_IMAGES
> > + { IH_TYPE_FLATDT_REL, "flat_dt_rel",
> > + "Relative Flat Device Tree",},
> > +#endif
> > { IH_TYPE_KERNEL, "kernel", "Kernel Image", },
> > +#ifdef CONFIG_SYS_RELATIVE_IMAGES
> > + { IH_TYPE_KERNEL_REL, "kernel_rel", "Relative Kernel Image",},
> > +#endif
>
> You could perhaps put the two relative items together as it doesn't
> look like the order matters.
I don't feel strongly, but I figured keeping things grouped my basic image
type, ignoring rel or not, was better.
> > @@ -188,6 +198,20 @@ int image_check_dcrc(const image_header_t *hdr)
> > return (dcrc == image_get_dcrc(hdr));
> > }
> >
> > +#ifdef CONFIG_SYS_RELATIVE_IMAGES
> > +uint32_t image_get_load_abs(const image_header_t *hdr)
> > +{
> > + return image_addr_raw_to_abs(image_get_type(hdr),
> > + image_get_load_raw(hdr));
> > +}
> > +
> > +uint32_t image_get_ep_abs(const image_header_t *hdr)
> > +{
> > + return image_addr_raw_to_abs(image_get_type(hdr),
> > + image_get_ep_raw(hdr));
> > +}
> > +#endif
>
> Perhaps if you have a function like image_addr_raw_to_abs() for the
> fdt case, you could reduce the code in
> fit_image_get_load/entry_abs()?
Those functions already use image_addr_raw_to_abs(). I suppose I could
lift the call to fit_image_get_type() from fit_image_get_load/entry_abs()
into a helper that then calls image_addr_raw_to_abs().
> > @@ -314,8 +342,24 @@ void image_print_contents(const void *ptr)
> > image_print_type(hdr);
> > printf("%sData Size: ", p);
> > genimg_print_size(image_get_data_size(hdr));
> > - printf("%sLoad Address: %08x\n", p, image_get_load(hdr));
> > - printf("%sEntry Point: %08x\n", p, image_get_ep(hdr));
> > +
> > + abs = image_get_load_abs(hdr);
> > +#ifdef CONFIG_SYS_RELATIVE_IMAGES
> > + raw = image_get_load_raw(hdr);
> > + if (abs != raw)
> > + printf("%sLoad Address: %08x (relative %08x)\n", p, abs, raw);
> > + else
> > +#endif
> > + printf("%sLoad Address: %08x\n", p, abs);
> > +
> > + abs = image_get_ep_abs(hdr);
> > +#ifdef CONFIG_SYS_RELATIVE_IMAGES
> > + raw = image_get_ep_raw(hdr);
> > + if (abs != raw)
> > + printf("%sEntry Point: %08x (relative %08x)\n", p, abs, raw);
> > + else
> > +#endif
>
> Any mileage in something like this?
>
> print_address(p, "Load", image_get_load_abs(hdr), image_get_load_raw(hdr));
> print_address(p, "Entry", image_get_entry_abs(hdr), image_get_entry_raw(hdr));
>
> static void do_address(const char *p, const char *name, ulong abs, ulong raw)
> {
> printf("%s%s Point: %08x", p, name, abs);
> #ifdef CONFIG_SYS_RELATIVE_IMAGES
> if (abs != raw)
> printf(" (relative %08x)", p, raw);
> #endif
> puts("\n");
> }
That'd end up slightly increasing binary size without the option enabled,
size it'd always call both get_load_abs() and get_load_raw(). I'm not sure
exactly how far Wolfgang wants to limit binary size increases without the
config option.
I also wondered about just printing both addresses all the time, even
when they're the same, although that'd mean a slight change in behavior
without the config option turned on, and with non-relative images.
> > @@ -2003,35 +2079,75 @@ void fit_image_print(const void *fit, int image_noffset, const char *p)
> > genimg_print_size(size);
> >
> > /* Remaining, type dependent properties */
> > - if ((type == IH_TYPE_KERNEL) || (type == IH_TYPE_STANDALONE) ||
> > - (type == IH_TYPE_RAMDISK) || (type == IH_TYPE_FIRMWARE) ||
> > - (type == IH_TYPE_FLATDT)) {
> > + if ((type == IH_TYPE_KERNEL) || (type == IH_TYPE_FLATDT) ||
> > +#ifdef CONFIG_SYS_RELATIVE_IMAGES
> > + (type == IH_TYPE_KERNEL_REL) || (type == IH_TYPE_FLATDT_REL) ||
> > +#endif
> > + (type == IH_TYPE_STANDALONE) || (type == IH_TYPE_RAMDISK) ||
> > + (type == IH_TYPE_FIRMWARE)) {
>
> Perhaps define a macro like ih_type_has_architecture() in the header?
> Not sure that Wolfgang likes that sort of thing though.
Wolfgang, what do you think?
> > fit_image_get_arch(fit, image_noffset, &arch);
> > printf("%s Architecture: %s\n", p, genimg_get_arch_name(arch));
> > }
> >
> > - if (type == IH_TYPE_KERNEL) {
> > + if ((type == IH_TYPE_KERNEL)
> > +#ifdef CONFIG_SYS_RELATIVE_IMAGES
> > + || (type == IH_TYPE_KERNEL_REL)
> > +#endif
> > + ) {
> > fit_image_get_os(fit, image_noffset, &os);
> > printf("%s OS: %s\n", p, genimg_get_os_name(os));
> > }
> >
> > - if ((type == IH_TYPE_KERNEL) || (type == IH_TYPE_STANDALONE) ||
> > - (type == IH_TYPE_FIRMWARE)) {
> > - ret = fit_image_get_load(fit, image_noffset, &load);
> > + if ((type == IH_TYPE_KERNEL) || (type == IH_TYPE_FLATDT) ||
> > +#ifdef CONFIG_SYS_RELATIVE_IMAGES
> > + (type == IH_TYPE_KERNEL_REL) || (type == IH_TYPE_FLATDT_REL) ||
> > +#endif
>
> Again here and above wonder whether the code size reduction is worth the #ifdefs
Wolfgang, what do you think?
> > -/**
> > - * fit_image_get_entry - get entry point address property for a given component image node
> > +#ifdef CONFIG_SYS_RELATIVE_IMAGES
> > +/*
> > + * fit_image_get_load_abs - get absolute load address property for a given
> > + * component image node
> > + * @fit: pointer to the FIT format image header
> > + * @noffset: component image node offset
> > + * @load: pointer to the uint32_t, will hold load address
> > + *
> > + * fit_image_get_load_abs() finds load address property in a given component
> > + * image node. If the property is found, its value is returned to the caller.
> > + *
> > + * Note that this function returns the absolute value that U-Boot should
> > + * use when actually loading images, or relocating them to the load address.
> > + *
> > + * returns:
> > + * 0, on success
> > + * -1, on failure
> > + */
> > +int fit_image_get_load_abs(const void *fit, int noffset, ulong *load)
> > +{
> > + int ret;
> > + ulong raw;
> > + uint8_t type;
> > +
> > + ret = fit_image_get_load_raw(fit, noffset, &raw);
> > + if (ret)
> > + return ret;
> > +
> > + ret = fit_image_get_type(fit, noffset, &type);
> > + if (ret)
> > + return ret;
> > +
> > + *load = image_addr_raw_to_abs(type, raw);
> > + return 0;
> > +}
> > +#endif
>
> I wonder (given there are so few callers) whether it might be worth
> just having a function (for each of raw and abs) that returns both the
> load and entry addresses in one shot?
I did wonder about that too, but that'd force a bunch of call sites to
declare new variables to receive the unused extra data, and increase
code size.
> > --- a/include/image.h
> > +++ b/include/image.h
> > @@ -160,6 +168,10 @@
> > #define IH_TYPE_IMXIMAGE 10 /* Freescale IMXBoot Image */
> > #define IH_TYPE_UBLIMAGE 11 /* Davinci UBL Image */
> > #define IH_TYPE_OMAPIMAGE 12 /* TI OMAP Config Header Image */
> > +#ifdef CONFIG_SYS_RELATIVE_IMAGES
> > +#define IH_TYPE_KERNEL_REL 13 /* OS Kernel Image */
> > +#define IH_TYPE_FLATDT_REL 14 /* Binary Flat Device Tree Blob */
> > +#endif
>
> From what I can tell it doesn't seem like the order matters here. You
> could perhaps move the last ones up into the main area, to avoid the
> new code in image_check_image_types().
Aren't these values embedded into the images themselves, and hence
immutable; if they were re-ordered, a mkimage built from a different
U-Boot source tree wouldn't work with the U-Boot binary with the change.
> Also I don't think the #ifdef really helps here, except perhaps for
> testing that you haven't left something bad in the code.
Yes, that'd why I ifdef'd it.
> It might be
> better to display an error like 'relative images not supported' when
> CONFIG_SYS_RELATIVE_IMAGES is not defined.
I think U-Boot will already detect that it doesn't know what the image
format is, since it'll have no case to handle it. I suppose the error
message won't be quite as explicit what's wrong, but again anything else
would increase binary size:-(
> > @@ -372,10 +384,25 @@ image_get_hdr_l(magic) /* image_get_magic */
> > image_get_hdr_l(hcrc) /* image_get_hcrc */
> > image_get_hdr_l(time) /* image_get_time */
> > image_get_hdr_l(size) /* image_get_size */
> > -image_get_hdr_l(load) /* image_get_load */
> > -image_get_hdr_l(ep) /* image_get_ep */
> > +image_get_hdr_l(load_raw) /* image_get_load_raw */
> > +image_get_hdr_l(ep_raw) /* image_get_ep_raw */
>
> might need another tab here (although it isn't visible on email)
>
> > @@ -430,8 +457,8 @@ image_set_hdr_l(magic) /* image_set_magic */
> > image_set_hdr_l(hcrc) /* image_set_hcrc */
> > image_set_hdr_l(time) /* image_set_time */
> > image_set_hdr_l(size) /* image_set_size */
> > -image_set_hdr_l(load) /* image_set_load */
> > -image_set_hdr_l(ep) /* image_set_ep */
> > +image_set_hdr_l(load_raw) /* image_set_load_raw */
> > +image_set_hdr_l(ep_raw) /* image_set_ep_raw */
>
> might need another tab here (although it isn't visible on email)
True; fixed up locally.
Thanks for the review. Depending on what Wolfgang says about the binary
size increases, maybe I'll be able to take some of your suggestions.
--
nvpublic
More information about the U-Boot
mailing list